fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

#175's model read-back fires a FALSE POSITIVE on any member spawned without a worktree — and the quarantine it announces never happens #234

New Issue
Closed
opened 2026-09-03 04:26:09 +02:00 by ltms · 2 comments
ltms commented 2026-09-03 04:26:09 +02:00
Owner
Copy Link
Copy Source

Found on the live daemon within one minute of deploying the #176 merge (main @ 0f08b93, pid 45674, jar ecb9645758b3), by spawning one probe member per backend. Two separate defects, both in code merged earlier today for #175.

Neither was caught by the #175 verification. That is the most useful part of this report — see "Why my own verification missed it" at the end.

Defect 1 — the read-back compares against a foreign session, and accuses a healthy profile

I spawned a plain fleet_spawn{profile: terra} with no worktree. The daemon logged:

09:22:28.891 ERROR d.l.f.m.OpenCodeLauncher - opencode profile 'terra' requested model
'openai/gpt-5.6-terra' but the live session is actually running 'gx/deepseek-v4-flash'
— ... quarantining this profile's credential

terra was running fine. The check read a different, three-day-old session belonging to a different profile.

OpenCodeSessionDiscovery keys both its lookups on the worker's cwd:

// sessionIdForDirectory  (#209)
"SELECT id    FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1"
// actualModelForDirectory (#175)
"SELECT model FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1"

The member's cwd was the lead's cwd, /Users/dai.ha/LTMS/claude-bridge. That directory holds many old sessions:

SELECT id, model, datetime(time_updated/1000,'unixepoch') FROM session
WHERE directory='/Users/dai.ha/LTMS/claude-bridge' ORDER BY time_updated DESC LIMIT 5;

ses_fa945e505ffepqOZNfQ24cfOjs | {"id":"deepseek-v4-flash","providerID":"gx"}        | 2026-08-31 07:32:40
ses_faa05ed49ffe2kXyYlZqEMnWbl | {"id":"deepseek-v4-flash","providerID":"gx"}        | 2026-08-31 04:02:56
ses_faa514995ffeTjHkvh8Jk7sjYf | {"id":"deepseek-v4-flash","providerID":"gx"}        | 2026-08-31 02:39:19
ses_fb9bbfab9ffebz3evt9z4qFO8C | {"id":"deepseek-v4-flash","providerID":"gx"}        | 2026-08-28 02:49:36
ses_fba9d49e4ffeenLPP3u1TJqfba | {"id":"nemotron-3-ultra-free","providerID":"opencode"} | 2026-08-27 22:45:04

Note the date: 2026-08-31, three days before this spawn. A brand-new idle member has not written its own row yet, so "most recently updated session in this directory" is somebody else's old session. fleet_list duly reported agentSessionId: ses_fa945e505ffepqOZNfQ24cfOjs for the new terra member — a stale id from a gx run.

So #209's identity resolution is wrong too, not only #175's model check. #175 merely made the wrong answer visible by acting on it.

The false premise, stated in the code

OpenCodeSessionDiscovery's own javadoc says the determinism is structural:

"The determinism that makes this useful is structural, not a guess: every fleetd worker runs in its own unique git worktree, so the row's directory (its project root) equals the worker's cwd identifies its session unambiguously."

That premise is false. fleet_spawn provisions a worktree only when the caller passes worktree. Without it the member inherits the lead's cwd — the default path, and the one I took. The word "every" is doing the damage: it reads as an invariant, so nobody re-checked it.

Why this is worse than a wrong log line

It quarantines on it. A quarantine is keyed on the credential, and both sol and terra carry credentialId: openai-shared. So one false positive on terra is designed to take out sol as well — both OpenAI profiles, for quarantineCooldownSeconds: 1800. That is half the fan-out capacity, removed for 30 minutes, because of a session row from three days ago.

Suggested direction

Key on the session id, not the directory. The launcher already resolves an agentSessionId; the model check should read the row for that id and nothing else. If the id is not resolved yet, that is UNKNOWN — return, do not compare, exactly as #175 already does for absent evidence. Do not try to make the directory heuristic smarter (a time_updated >= spawn time filter would narrow it, not fix it, and would still pick a sibling member sharing the cwd).

While fixing it, correct the javadoc rather than leaving a false invariant in the file.

Defect 2 — the quarantine it announces never happens

The ERROR says "quarantining this profile's credential". It did not.

fleet_list one minute later, with quarantineCooldownSeconds: 1800:

{"profile":"sol",  "maxLoad":1,"live":0,"free":1,"reclaimable":0}
{"profile":"terra","maxLoad":2,"live":1,"free":1,"reclaimable":1}

No credentialId, no quarantinedForSeconds, and free is not forced to 0 — the three things CB-583's capacityView adds for a quarantined profile. And the sink's own log line never appears: grepping the whole log since the restart for quarantin returns exactly one line, the ERROR above. Fleetd's log.warn("credential '{}' quarantined for {}s ...") never ran.

The cause is in the sink (Fleetd.java:348):

ExhaustionSink exhaustionSink = (target, reason) -> sessions.roster().stream()
        .filter(session -> target.equals(session.terminalId()))
        .findFirst()
        .map(MemberSession::profile)
        .map(profileName -> config.get().profiles().get(profileName))
        .ifPresent(profile -> { ... quarantine.quarantine(credentialId); ... });

It resolves target → session → profile → credential through sessions.roster(). The #175 check calls it from SessionAwareHandle.agentSessionId(), which runs during spawn — timestamps show the mismatch ERROR at 09:22:28.891, 0.7s after peer pane=w8:p1Y reached injectable state at 09:22:28.205, before the member is registered in the roster. No roster entry ⇒ .ifPresent no-ops ⇒ silence.

So the check detects, logs, and does nothing. The one saving grace today: defect 2 is the only reason defect 1 did not cost me sol and terra for half an hour. Two bugs cancelling out is not a working feature.

This is the same shape as #113 and as the "a test on the seam does not prove the caller" family: the response to a detection was never proved on the real path. #175's tests all exercised the checker, and the checker is fine.

Suggested direction

.ifPresent is a silent failure on a control path. When a quarantine is requested for a target the sink cannot resolve, it must not fall through quietly — either resolve the profile from something available at spawn time (the launcher knows its own profile; it does not need the roster), or log loudly that the quarantine was requested and could not be applied. A control that cannot act must say so.

Whichever fix is chosen, the test has to start from the real call — a spawn that triggers the check — and assert the credential actually ends up quarantined. A test that calls the sink directly will pass today.

Why my own verification missed it

Recorded deliberately, because the method failed, not just the code.

I did live-prove #175 before closing it. That probe used a member on a fresh worktree — a unique directory with exactly one session row, its own. That is the one configuration in which the directory heuristic is guaranteed correct. I proved the feature on the path where its assumption holds, then reported it verified.

The probe that found this took thirty seconds and differed in one way: no worktree, which is the default for fleet_spawn. So the verification covered the special case and skipped the default.

Two rules from this:

  1. Probe the default configuration, not the convenient one. If a feature rests on an assumption, the probe must be run where the assumption is weakest — otherwise it measures agreement, not correctness.
  2. When a check has a consequence, prove the consequence, not the detection. I confirmed "no mismatch ERROR appeared" and treated that as the feature working. I never once made a mismatch fire and then checked that a credential was actually quarantined. Defect 2 would have shown up immediately if I had.

Related

#175 (introduced both, merged today) · #209 (the directory-keyed lookup both share; its identity resolution is wrong for the same reason) · #113 (checkers narrower than they look) · #232 (closed: the same read-back has no evidence at all for autoCompactWindow)

Found on the live daemon within one minute of deploying the #176 merge (`main` @ `0f08b93`, pid 45674, jar `ecb9645758b3`), by spawning one probe member per backend. Two separate defects, both in code merged earlier today for #175. Neither was caught by the #175 verification. **That is the most useful part of this report** — see "Why my own verification missed it" at the end. ## Defect 1 — the read-back compares against a foreign session, and accuses a healthy profile I spawned a plain `fleet_spawn{profile: terra}` with **no worktree**. The daemon logged: ``` 09:22:28.891 ERROR d.l.f.m.OpenCodeLauncher - opencode profile 'terra' requested model 'openai/gpt-5.6-terra' but the live session is actually running 'gx/deepseek-v4-flash' — ... quarantining this profile's credential ``` terra was running fine. The check read a **different, three-day-old session belonging to a different profile**. `OpenCodeSessionDiscovery` keys both its lookups on the worker's cwd: ```java // sessionIdForDirectory (#209) "SELECT id FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1" // actualModelForDirectory (#175) "SELECT model FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1" ``` The member's cwd was the lead's cwd, `/Users/dai.ha/LTMS/claude-bridge`. That directory holds many old sessions: ```sql SELECT id, model, datetime(time_updated/1000,'unixepoch') FROM session WHERE directory='/Users/dai.ha/LTMS/claude-bridge' ORDER BY time_updated DESC LIMIT 5; ses_fa945e505ffepqOZNfQ24cfOjs | {"id":"deepseek-v4-flash","providerID":"gx"} | 2026-08-31 07:32:40 ses_faa05ed49ffe2kXyYlZqEMnWbl | {"id":"deepseek-v4-flash","providerID":"gx"} | 2026-08-31 04:02:56 ses_faa514995ffeTjHkvh8Jk7sjYf | {"id":"deepseek-v4-flash","providerID":"gx"} | 2026-08-31 02:39:19 ses_fb9bbfab9ffebz3evt9z4qFO8C | {"id":"deepseek-v4-flash","providerID":"gx"} | 2026-08-28 02:49:36 ses_fba9d49e4ffeenLPP3u1TJqfba | {"id":"nemotron-3-ultra-free","providerID":"opencode"} | 2026-08-27 22:45:04 ``` Note the date: **2026-08-31**, three days before this spawn. A brand-new idle member has not written its own row yet, so "most recently updated session in this directory" is somebody else's old session. `fleet_list` duly reported `agentSessionId: ses_fa945e505ffepqOZNfQ24cfOjs` for the new terra member — a stale id from a `gx` run. So #209's identity resolution is wrong too, not only #175's model check. #175 merely made the wrong answer visible by acting on it. ### The false premise, stated in the code `OpenCodeSessionDiscovery`'s own javadoc says the determinism is structural: > *"The determinism that makes this useful is structural, not a guess: every fleetd worker runs in its own unique git worktree, so the row's `directory` (its project root) equals the worker's cwd identifies **its** session unambiguously."* **That premise is false.** `fleet_spawn` provisions a worktree only when the caller passes `worktree`. Without it the member inherits the lead's cwd — the default path, and the one I took. The word "every" is doing the damage: it reads as an invariant, so nobody re-checked it. ### Why this is worse than a wrong log line It quarantines on it. A quarantine is keyed on the **credential**, and both `sol` and `terra` carry `credentialId: openai-shared`. So one false positive on terra is designed to take out `sol` as well — both OpenAI profiles, for `quarantineCooldownSeconds: 1800`. That is half the fan-out capacity, removed for 30 minutes, because of a session row from three days ago. ### Suggested direction Key on the session **id**, not the directory. The launcher already resolves an `agentSessionId`; the model check should read the row for *that id* and nothing else. If the id is not resolved yet, that is UNKNOWN — return, do not compare, exactly as #175 already does for absent evidence. Do not try to make the directory heuristic smarter (a `time_updated >= spawn time` filter would narrow it, not fix it, and would still pick a sibling member sharing the cwd). While fixing it, correct the javadoc rather than leaving a false invariant in the file. ## Defect 2 — the quarantine it announces never happens The ERROR says *"quarantining this profile's credential"*. It did not. `fleet_list` one minute later, with `quarantineCooldownSeconds: 1800`: ```json {"profile":"sol", "maxLoad":1,"live":0,"free":1,"reclaimable":0} {"profile":"terra","maxLoad":2,"live":1,"free":1,"reclaimable":1} ``` No `credentialId`, no `quarantinedForSeconds`, and `free` is not forced to 0 — the three things CB-583's `capacityView` adds for a quarantined profile. And the sink's own log line never appears: grepping the whole log since the restart for `quarantin` returns **exactly one** line, the ERROR above. `Fleetd`'s `log.warn("credential '{}' quarantined for {}s ...")` never ran. The cause is in the sink (`Fleetd.java:348`): ```java ExhaustionSink exhaustionSink = (target, reason) -> sessions.roster().stream() .filter(session -> target.equals(session.terminalId())) .findFirst() .map(MemberSession::profile) .map(profileName -> config.get().profiles().get(profileName)) .ifPresent(profile -> { ... quarantine.quarantine(credentialId); ... }); ``` It resolves `target` → session → profile → credential through `sessions.roster()`. The #175 check calls it from `SessionAwareHandle.agentSessionId()`, which runs **during spawn** — timestamps show the mismatch ERROR at `09:22:28.891`, 0.7s after `peer pane=w8:p1Y reached injectable state` at `09:22:28.205`, before the member is registered in the roster. No roster entry ⇒ `.ifPresent` no-ops ⇒ silence. So the check detects, logs, and does nothing. The one saving grace today: defect 2 is the only reason defect 1 did not cost me `sol` and `terra` for half an hour. Two bugs cancelling out is not a working feature. This is the same shape as #113 and as the "a test on the seam does not prove the caller" family: the *response* to a detection was never proved on the real path. #175's tests all exercised the checker, and the checker is fine. ### Suggested direction `.ifPresent` is a silent failure on a control path. When a quarantine is requested for a target the sink cannot resolve, it must not fall through quietly — either resolve the profile from something available at spawn time (the launcher knows its own profile; it does not need the roster), or log loudly that the quarantine was requested and could not be applied. A control that cannot act must say so. Whichever fix is chosen, the test has to start from the real call — a spawn that triggers the check — and assert the credential actually ends up quarantined. A test that calls the sink directly will pass today. ## Why my own verification missed it Recorded deliberately, because the method failed, not just the code. I did live-prove #175 before closing it. That probe used a member **on a fresh worktree** — a unique directory with exactly one session row, its own. That is the one configuration in which the directory heuristic is guaranteed correct. I proved the feature on the path where its assumption holds, then reported it verified. The probe that found this took thirty seconds and differed in one way: no worktree, which is the **default** for `fleet_spawn`. So the verification covered the special case and skipped the default. Two rules from this: 1. **Probe the default configuration, not the convenient one.** If a feature rests on an assumption, the probe must be run where the assumption is weakest — otherwise it measures agreement, not correctness. 2. **When a check has a consequence, prove the consequence, not the detection.** I confirmed "no mismatch ERROR appeared" and treated that as the feature working. I never once made a mismatch fire and then checked that a credential was actually quarantined. Defect 2 would have shown up immediately if I had. ## Related #175 (introduced both, merged today) · #209 (the directory-keyed lookup both share; its identity resolution is wrong for the same reason) · #113 (checkers narrower than they look) · #232 (closed: the same read-back has no evidence at all for `autoCompactWindow`)
ltms commented 2026-09-03 05:23:03 +02:00
Author
Owner
Copy Link
Copy Source

Review of PR #236 — defect 1 accepted, defect 2's fix is dead on the production path

Defect 1 (the model check keyed on directory instead of the resolved session id) is fixed correctly. actualModelForSessionId(id) is a primary-key lookup, SessionAwareHandle caches the resolved id in an AtomicReference so a sibling session cannot make it drift, and the javadoc that claimed "every fleetd worker runs in its own unique git worktree" is corrected. Accepted.

Defect 2 is not fixed. The 3-arg onExhausted(target, reason, profile) overload never reaches the real sink.

Fleetd.java:177 builds the sink the OpenCodeLauncher is actually constructed with (:193):

ExhaustionSink forwardingExhaustionSink = (target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason);

That is a lambda. A lambda implements only the interface's one abstract method, so it inherits the new 3-arg default, which discards the profile and calls the 2-arg. The real chain is:

checkModelMatch -> forwarding.onExhausted(t, r, "gx")
   -> ExhaustionSink's DEFAULT 3-arg   [drops "gx"]
   -> forwarding.onExhausted(t, r)
   -> real sink 2-arg -> real sink 3-arg with profileHint = null
   -> roster miss + no hint -> new ERROR log -> credential still NOT quarantined

So the original bug still happens. The new ERROR log is an improvement — the failure is at least loud now instead of silent — but the quarantine the check announces still does not occur.

Proof

I wrote this into the PR's own tree and ran it. It fails: expected: <gx> but was: <null>.

AtomicReference<String> hintSeenByRealSink = new AtomicReference<>("NEVER CALLED");
ExhaustionSink real = new ExhaustionSink() {
    @Override public void onExhausted(String t, String r) { onExhausted(t, r, null); }
    @Override public void onExhausted(String t, String r, String hint) { hintSeenByRealSink.set(hint); }
};
AtomicReference<ExhaustionSink> ref = new AtomicReference<>(ExhaustionSink.none());
ExhaustionSink forwarding = (t, r) -> ref.get().onExhausted(t, r);   // Fleetd.java:177, verbatim in shape
ref.set(real);
forwarding.onExhausted("term_x", "model mismatch", "gx");
assertEquals("gx", hintSeenByRealSink.get());

Why 1127 green tests missed it

Every test injects a sink straight into OpenCodeLauncher and walks around the forwarding hop. This is the same shape as the other entries in this repo's "a test on the seam does not prove the caller" list: the test exercises the interface, and production goes through one more object than the test does.

The PR author flagged exactly the right area themselves — "Fleetd.java's anonymous-class sink logic is exercised only indirectly ... not by a standalone Fleetd-level unit test" — and the gap they flagged is where the defect is.

The general rule

A default method on an interface is invisible to a lambda. Adding an overload to an interface makes the unit of work every existing implementation of that interface, not just the caller being fixed. Any ExhaustionSink built as a lambda or a method reference silently drops the hint.

Sent back on the same branch: fix the forwarder to be an anonymous class forwarding both overloads, audit every other ExhaustionSink value in main/ for the same shape, and add a test that drives the hint through the composed production wiring so that turning the forwarder back into a lambda goes red.

## Review of PR #236 — defect 1 accepted, defect 2's fix is dead on the production path Defect 1 (the model check keyed on `directory` instead of the resolved session id) is fixed correctly. `actualModelForSessionId(id)` is a primary-key lookup, `SessionAwareHandle` caches the resolved id in an `AtomicReference` so a sibling session cannot make it drift, and the javadoc that claimed "every fleetd worker runs in its own unique git worktree" is corrected. Accepted. **Defect 2 is not fixed.** The 3-arg `onExhausted(target, reason, profile)` overload never reaches the real sink. `Fleetd.java:177` builds the sink the `OpenCodeLauncher` is actually constructed with (`:193`): ```java ExhaustionSink forwardingExhaustionSink = (target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason); ``` That is a **lambda**. A lambda implements only the interface's one abstract method, so it inherits the new 3-arg **default**, which discards the profile and calls the 2-arg. The real chain is: ``` checkModelMatch -> forwarding.onExhausted(t, r, "gx") -> ExhaustionSink's DEFAULT 3-arg [drops "gx"] -> forwarding.onExhausted(t, r) -> real sink 2-arg -> real sink 3-arg with profileHint = null -> roster miss + no hint -> new ERROR log -> credential still NOT quarantined ``` So the original bug still happens. The new ERROR log is an improvement — the failure is at least loud now instead of silent — but the quarantine the check announces still does not occur. ### Proof I wrote this into the PR's own tree and ran it. It fails: `expected: <gx> but was: <null>`. ```java AtomicReference<String> hintSeenByRealSink = new AtomicReference<>("NEVER CALLED"); ExhaustionSink real = new ExhaustionSink() { @Override public void onExhausted(String t, String r) { onExhausted(t, r, null); } @Override public void onExhausted(String t, String r, String hint) { hintSeenByRealSink.set(hint); } }; AtomicReference<ExhaustionSink> ref = new AtomicReference<>(ExhaustionSink.none()); ExhaustionSink forwarding = (t, r) -> ref.get().onExhausted(t, r); // Fleetd.java:177, verbatim in shape ref.set(real); forwarding.onExhausted("term_x", "model mismatch", "gx"); assertEquals("gx", hintSeenByRealSink.get()); ``` ### Why 1127 green tests missed it Every test injects a sink **straight into `OpenCodeLauncher`** and walks around the forwarding hop. This is the same shape as the other entries in this repo's "a test on the seam does not prove the caller" list: the test exercises the interface, and production goes through one more object than the test does. The PR author flagged exactly the right area themselves — *"`Fleetd.java`'s anonymous-class sink logic is exercised only indirectly ... not by a standalone Fleetd-level unit test"* — and the gap they flagged is where the defect is. ### The general rule **A default method on an interface is invisible to a lambda.** Adding an overload to an interface makes the unit of work *every existing implementation of that interface*, not just the caller being fixed. Any `ExhaustionSink` built as a lambda or a method reference silently drops the hint. Sent back on the same branch: fix the forwarder to be an anonymous class forwarding both overloads, audit every other `ExhaustionSink` value in `main/` for the same shape, and add a test that drives the hint through the **composed** production wiring so that turning the forwarder back into a lambda goes red.
ltms commented 2026-09-03 06:58:23 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged to main as 838a701.

ExhaustionSink's 3-argument method (carrying the profile) is now the single abstract method, so a caller must supply the profile. The false positive is gone: a member spawned without a worktree no longer trips the model read-back.

The design point is worth keeping, because it decided how the fix was shaped. The obvious fix was to add a default overload carrying the extra argument. That would have shipped dead, with a fully green suite: a lambda implements only the single abstract method, so a default overload is invisible to every existing lambda. Every call site would have kept using the old path and no test could have noticed.

Inverting it instead — making the richest method abstract and the poor one the default — forces every implementation to be revisited, because a lambda with the wrong arity stops compiling. The unit of work becomes every implementation, not just the interface.

That property then paid for itself twice at merge time. I merged this before the four parallel #201 units on purpose. It turned a collision in a shared test file into a loud compile error: git auto-merged CompletionResolverTest with no conflict markers, and the build failed with exactly one error, because a 2-argument lambda was now illegal. Git compares text, and a lambda's arity is a type fact — nothing textual is in conflict, so only a compiler can find it.

Verified: 1215 tests on main, 0 failures, 0 compile errors.

Fixed and merged to `main` as `838a701`. `ExhaustionSink`'s 3-argument method (carrying the profile) is now the single abstract method, so a caller must supply the profile. The false positive is gone: a member spawned without a worktree no longer trips the model read-back. **The design point is worth keeping, because it decided how the fix was shaped.** The obvious fix was to add a `default` overload carrying the extra argument. That would have shipped **dead**, with a fully green suite: a lambda implements only the single abstract method, so a `default` overload is invisible to every existing lambda. Every call site would have kept using the old path and no test could have noticed. Inverting it instead — making the richest method abstract and the poor one the default — forces every implementation to be revisited, because a lambda with the wrong arity stops compiling. The unit of work becomes every *implementation*, not just the interface. That property then paid for itself twice at merge time. I merged this before the four parallel #201 units on purpose. It turned a collision in a shared **test** file into a loud compile error: git auto-merged `CompletionResolverTest` with no conflict markers, and the build failed with exactly one error, because a 2-argument lambda was now illegal. Git compares text, and a lambda's arity is a type fact — nothing textual is in conflict, so only a compiler can find it. Verified: 1215 tests on `main`, 0 failures, 0 compile errors.
ltms closed this issue 2026-09-03 06:58:26 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#234
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?