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

fleetd #368: a stale lead delegation binding must not shadow the primary fallback #371

Closed
agent wants to merge 0 commits from worker/fleetd-368-stale-lead-binding-f5682e-2 into main
pull from: worker/fleetd-368-stale-lead-binding-f5682e-2
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-06 14:55:17 +02:00
Member
Copy Link
Copy Source

fleetd #368 — a lead's delegation binding outlives the lead, so reply nudges kept going to a dead terminal.

Root cause. PrimaryRegistry.forgetDelegation is wired to exactly one event — a worker's release — because that's the only teardown the daemon already observes for a session in this map. Nothing removed a binding when the LEAD half went away: a lead that is closed, crashes, or is relaunched leaves leadByTarget pointing at a terminal herdr no longer knows. nudgeTargetFor only falls back to the single known primary when the map holds nothing for the target — a stale, non-null entry always won over that fallback, which is exactly backwards: the fallback's own javadoc argues it is safe precisely in the case a stale entry now hid.

Measurements before the fix (both requested by the ticket):

  • (a) Does herdr ever reuse a term_* id? I could not find an explicit statement of herdr's id-generation algorithm in this repo. Strongest evidence: LeadTabScannerTest.java models a session restart as herdr assigning a brand-new id to the same pane (herdr.pane("w1:p1", "w1:t1", "term_opus_v2")), with a comment that "the old terminal_id is simply gone, not carried" — i.e. the codebase's own mental model treats restarts as getting fresh ids, never the old one back. A real observed id in docs/CB-307-Push-Loop.md (term_656c8cc03e1f0b1) is high-entropy/hex-like, consistent with (not proof of) non-sequential generation. Conclusion: cannot prove "never reused" from this repo alone, but nothing suggests reuse either, and the codebase's own tests assume fresh ids after a restart. I did not find evidence that would make this the worse bug the ticket asked me to rule out.
  • (b) What happens at the five ReplyPushLoop call sites when agents.send targets a dead terminal? All five (onReplyQueued, onTicketTerminal, onQuestionOpened, onBackendIncident, onBackendTargetUnmapped) behave identically: they only ever call primaryRegistry.nudgeTargetFor(target) and stash the result into a per-lead pending map; the actual herdr call (agents.status/agents.send) happens later, once per tick, in decide()/injectNudge() — shared by every source. For a genuinely dead lead, agents.status(lead) throws (HerdrException agent_not_found, since AgentControl.resolveTarget can't find the terminal in agent.list and herdr rejects the raw id). decide() catches this as a generic RuntimeException, logs at debug, and returns WAIT_BUSY — retried every tick until maxReminders is exhausted, then STOP (countNudge("exhausted")), all below warn level. Nothing ever calls agents.send for a dead lead (the status check fails first), so injectNudge's own try/catch around agents.send is never reached for this case. Net effect: a silent, capped retry-then-give-up loop with no operator-visible signal.

Direction chosen: check liveness before nudging, fall through to the existing fallback (closest to your "direction 1"). I rejected:

  • "Drop a lead's bindings when that lead terminal disappears" (your hinted direction) as the primary mechanism, because nothing in the codebase currently observes "a lead terminal disappeared" as an event — leads aren't SessionManager-managed sessions, so there's no existing hook to wire a sweep into. Building one would mean either a new scheduled reconciliation against LeadTabScanner's live-lead map (real wiring into Fleetd's startup, new coupling between PrimaryRegistry and herdr) or reusing the exact same "detect it's dead" moment decide() already has. I use the latter, but scoped per-target (see below) rather than as a blanket sweep, because PrimaryRegistry.forgetDelegation(target) — the exact removal primitive "direction 2" wants — already exists; no new registry API was needed at all.
  • "Key the binding on something that cannot go stale" (e.g. a lead name resolved through LeadTabScanner instead of a raw terminal id) as unnecessary added complexity: it would still need a liveness/resolution step against herdr at read time, which is what I do directly, without changing PrimaryRegistry's keying or adding a dependency from it onto herdr/LeadTabScanner.

The fix (ReplyPushLoop.java): a new private resolveLiveLead(target) wraps every one of the five primaryRegistry.nudgeTargetFor(target) call sites. It probes the resolved lead with agents.status(lead) — the same call decide() already makes every tick — and if that throws, treats the binding as dead: calls the existing primaryRegistry.forgetDelegation(target) (self-healing, exactly like AgentControl.paneByTerminal already does on agent_not_found — explicitly called out in the ticket as the good, non-defective pattern to imitate) and re-resolves, which now reaches the single-primary fallback (or empty, correctly, if there is none).

Tests (ReplyPushLoopTest.java), driving ReplyPushLoop itself (not just PrimaryRegistry) per the ticket's ask:

  • aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal — a worker delegated to a dead lead still gets its reply nudge delivered to the pinned live primary, and the stale binding is forgotten afterward.
  • aStaleLeadBindingWithNoFallbackNeverNudgesTheDeadTerminal — with no pinned primary, the dead lead is never nudged and the binding is still forgotten (converges to the same empty-map state the existing fallback already handles).

Both fail on the pre-fix code (Tests run: 61, Failures: 2) and pass after (Tests run: 61, Failures: 0). Full mvn clean install (unpiped): Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Revert-to-red proof: reverted ReplyPushLoop.java to HEAD (production code only, tests kept) → Tests run: 61, Failures: 2, Errors: 0 (the two new tests fail, nothing else does) → restored the fix → Tests run: 61, Failures: 0, Errors: 0.

Criterion 5 — same shape elsewhere (report only, not fixed):

  • MemberRegistry.terminalToSlot (flagged in the ticket as "probably fine, unconfirmed"): confirmed it self-heals — both SessionManager.release(paneId, cause) (explicit stop/drain/shutdown) and releaseIfCurrent (idle reaper) funnel into the single private releaseRemoved, which calls memberLifecycle.released(terminal) → MemberRegistry.unbind. I did not separately verify a herdr-detected-crash-only teardown path outside those two entry points.
  • I did not find a second clear instance of this exact defect shape (an id recorded once in a long-lived map, read back later with no liveness check, and no event wired to remove it when that specific recorded entity dies) elsewhere in msg/, inject/, or auth/. The closest-looking candidates turned out not to match: Injector.targets and CompletionResolver.inFlight are both actively drained through their own delivery/turn lifecycle (many remove call sites tied to real state transitions, not a single release hook), LeadCoordLoop's lead map is LeadTabScanner itself — already TTL-refreshed with a liveness cross-check against agent.list (the #359 fix) — and AgentControl.paneByTerminal is the already-known-good self-heal-on-use pattern this fix imitates.

Caveat for review: the liveness probe adds one extra agents.status() herdr round-trip on the (uncommon) path where a candidate lead is stale, but none on the common path since it's evaluated lazily. This is a local unix-socket call, and decide() already performs an equivalent check every scheduled tick regardless — this fix doesn't add new cost to the common case, only to the exact stale-binding case that was silently broken before.

Review round 2 (must-fix, addressed)

Finding: isLive caught bare RuntimeException and treated any failure as "the lead is gone," which resolveLiveLead then acted on with forgetDelegation — destructive and permanent. HerdrException wraps every IO/codec failure, so a transient socket blip or decode error on a perfectly live lead would silently and permanently unbind it. This is the fleetd #359 mistake repeated two days later: a single bad reading destroying a live binding.

Fix: narrowed isLive to match AgentControl.agentCall's own rule — only an affirmative HerdrException with code "agent_not_found" counts as gone (e instanceof HerdrException he && "agent_not_found".equals(he.code())); every other RuntimeException is treated as still live and the binding is left alone. Chose treat-as-live over rethrow (the reviewer's two options), because a throw here would propagate into all five public ReplyPushLoop entry points, none of which can throw today (onReplyQueued, onTicketTerminal, onQuestionOpened, onBackendIncident, onBackendTargetUnmapped — I've now read all five closely enough to be sure of this): treating an inconclusive reading as live costs nothing worse than one more retry on the next tick, which decide() already tolerates for exactly this reason.

New test aTransientLivenessFailureMustNotForgetABindingToAStillLiveLead, using a new FlakyThenLiveHerdrClient fake whose first agent.get call throws a transient HerdrException (code null, a transport-level failure) and succeeds after — proves the binding survives and the lead still gets nudged once the probe recovers.

Both directions proven, as asked:

  • With the narrowed isLive: Tests run: 62, Failures: 0, Errors: 0 (61 existing + 1 new).
  • Widened back to bare RuntimeException (temporary, reverted immediately after): Tests run: 62, Failures: 1, Errors: 0 — only the new test fails, nothing else.

Full unpiped mvn clean install: Tests run: 1415, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Pushed as d6ef0c8.

fleetd #368 — a lead's delegation binding outlives the lead, so reply nudges kept going to a dead terminal. **Root cause.** `PrimaryRegistry.forgetDelegation` is wired to exactly one event — a worker's release — because that's the only teardown the daemon already observes for a session in this map. Nothing removed a binding when the LEAD half went away: a lead that is closed, crashes, or is relaunched leaves `leadByTarget` pointing at a terminal herdr no longer knows. `nudgeTargetFor` only falls back to the single known primary when the map holds *nothing* for the target — a stale, non-null entry always won over that fallback, which is exactly backwards: the fallback's own javadoc argues it is safe precisely in the case a stale entry now hid. **Measurements before the fix (both requested by the ticket):** - **(a) Does herdr ever reuse a `term_*` id?** I could not find an explicit statement of herdr's id-generation algorithm in this repo. Strongest evidence: `LeadTabScannerTest.java` models a session restart as herdr assigning a **brand-new** id to the same pane (`herdr.pane("w1:p1", "w1:t1", "term_opus_v2")`), with a comment that "the old terminal_id is simply gone, not carried" — i.e. the codebase's own mental model treats restarts as getting fresh ids, never the old one back. A real observed id in `docs/CB-307-Push-Loop.md` (`term_656c8cc03e1f0b1`) is high-entropy/hex-like, consistent with (not proof of) non-sequential generation. Conclusion: cannot prove "never reused" from this repo alone, but nothing suggests reuse either, and the codebase's own tests assume fresh ids after a restart. I did not find evidence that would make this the *worse* bug the ticket asked me to rule out. - **(b) What happens at the five `ReplyPushLoop` call sites when `agents.send` targets a dead terminal?** All five (`onReplyQueued`, `onTicketTerminal`, `onQuestionOpened`, `onBackendIncident`, `onBackendTargetUnmapped`) behave identically: they only ever call `primaryRegistry.nudgeTargetFor(target)` and stash the result into a per-lead pending map; the actual herdr call (`agents.status`/`agents.send`) happens later, once per tick, in `decide()`/`injectNudge()` — shared by every source. For a genuinely dead lead, `agents.status(lead)` throws (`HerdrException` `agent_not_found`, since `AgentControl.resolveTarget` can't find the terminal in `agent.list` and herdr rejects the raw id). `decide()` catches this as a **generic** `RuntimeException`, logs at `debug`, and returns `WAIT_BUSY` — retried every tick until `maxReminders` is exhausted, then `STOP` (`countNudge("exhausted")`), all below `warn` level. Nothing ever calls `agents.send` for a dead lead (the status check fails first), so `injectNudge`'s own try/catch around `agents.send` is never reached for this case. Net effect: a silent, capped retry-then-give-up loop with no operator-visible signal. **Direction chosen: check liveness before nudging, fall through to the existing fallback (closest to your "direction 1").** I rejected: - *"Drop a lead's bindings when that lead terminal disappears"* (your hinted direction) as the primary mechanism, because nothing in the codebase currently observes "a lead terminal disappeared" as an event — leads aren't `SessionManager`-managed sessions, so there's no existing hook to wire a sweep into. Building one would mean either a new scheduled reconciliation against `LeadTabScanner`'s live-lead map (real wiring into `Fleetd`'s startup, new coupling between `PrimaryRegistry` and herdr) or reusing the exact same "detect it's dead" moment `decide()` already has. I use the latter, but scoped per-target (see below) rather than as a blanket sweep, because `PrimaryRegistry.forgetDelegation(target)` — the exact removal primitive "direction 2" wants — already exists; no new registry API was needed at all. - *"Key the binding on something that cannot go stale"* (e.g. a lead name resolved through `LeadTabScanner` instead of a raw terminal id) as unnecessary added complexity: it would still need a liveness/resolution step against herdr at read time, which is what I do directly, without changing `PrimaryRegistry`'s keying or adding a dependency from it onto herdr/`LeadTabScanner`. **The fix** (`ReplyPushLoop.java`): a new private `resolveLiveLead(target)` wraps every one of the five `primaryRegistry.nudgeTargetFor(target)` call sites. It probes the resolved lead with `agents.status(lead)` — the same call `decide()` already makes every tick — and if that throws, treats the binding as dead: calls the *existing* `primaryRegistry.forgetDelegation(target)` (self-healing, exactly like `AgentControl.paneByTerminal` already does on `agent_not_found` — explicitly called out in the ticket as the good, non-defective pattern to imitate) and re-resolves, which now reaches the single-primary fallback (or empty, correctly, if there is none). **Tests** (`ReplyPushLoopTest.java`), driving `ReplyPushLoop` itself (not just `PrimaryRegistry`) per the ticket's ask: - `aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal` — a worker delegated to a dead lead still gets its reply nudge delivered to the pinned live primary, and the stale binding is forgotten afterward. - `aStaleLeadBindingWithNoFallbackNeverNudgesTheDeadTerminal` — with no pinned primary, the dead lead is never nudged and the binding is still forgotten (converges to the same empty-map state the existing fallback already handles). Both fail on the pre-fix code (`Tests run: 61, Failures: 2`) and pass after (`Tests run: 61, Failures: 0`). Full `mvn clean install` (unpiped): `Tests run: 1414, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. **Revert-to-red proof:** reverted `ReplyPushLoop.java` to HEAD (production code only, tests kept) → `Tests run: 61, Failures: 2, Errors: 0` (the two new tests fail, nothing else does) → restored the fix → `Tests run: 61, Failures: 0, Errors: 0`. **Criterion 5 — same shape elsewhere (report only, not fixed):** - `MemberRegistry.terminalToSlot` (flagged in the ticket as "probably fine, unconfirmed"): confirmed it self-heals — both `SessionManager.release(paneId, cause)` (explicit stop/drain/shutdown) and `releaseIfCurrent` (idle reaper) funnel into the single private `releaseRemoved`, which calls `memberLifecycle.released(terminal)` → `MemberRegistry.unbind`. I did not separately verify a herdr-detected-crash-only teardown path outside those two entry points. - I did not find a second clear instance of this exact defect shape (an id recorded once in a long-lived map, read back later with no liveness check, and no event wired to remove it when *that specific recorded entity* dies) elsewhere in `msg/`, `inject/`, or `auth/`. The closest-looking candidates turned out not to match: `Injector.targets` and `CompletionResolver.inFlight` are both actively drained through their own delivery/turn lifecycle (many `remove` call sites tied to real state transitions, not a single release hook), `LeadCoordLoop`'s lead map is `LeadTabScanner` itself — already TTL-refreshed with a liveness cross-check against `agent.list` (the #359 fix) — and `AgentControl.paneByTerminal` is the already-known-good self-heal-on-use pattern this fix imitates. **Caveat for review:** the liveness probe adds one extra `agents.status()` herdr round-trip on the (uncommon) path where a candidate lead is stale, but none on the common path since it's evaluated lazily. This is a local unix-socket call, and `decide()` already performs an equivalent check every scheduled tick regardless — this fix doesn't add new *cost* to the common case, only to the exact stale-binding case that was silently broken before. --- ## Review round 2 (must-fix, addressed) **Finding:** `isLive` caught bare `RuntimeException` and treated *any* failure as "the lead is gone," which `resolveLiveLead` then acted on with `forgetDelegation` — destructive and permanent. `HerdrException` wraps every IO/codec failure, so a transient socket blip or decode error on a perfectly live lead would silently and permanently unbind it. This is the fleetd #359 mistake repeated two days later: a single bad reading destroying a live binding. **Fix:** narrowed `isLive` to match `AgentControl.agentCall`'s own rule — only an affirmative `HerdrException` with code `"agent_not_found"` counts as gone (`e instanceof HerdrException he && "agent_not_found".equals(he.code())`); every other `RuntimeException` is treated as still live and the binding is left alone. Chose **treat-as-live** over rethrow (the reviewer's two options), because a throw here would propagate into all five public `ReplyPushLoop` entry points, none of which can throw today (`onReplyQueued`, `onTicketTerminal`, `onQuestionOpened`, `onBackendIncident`, `onBackendTargetUnmapped` — I've now read all five closely enough to be sure of this): treating an inconclusive reading as live costs nothing worse than one more retry on the next tick, which `decide()` already tolerates for exactly this reason. **New test** `aTransientLivenessFailureMustNotForgetABindingToAStillLiveLead`, using a new `FlakyThenLiveHerdrClient` fake whose first `agent.get` call throws a transient `HerdrException` (code `null`, a transport-level failure) and succeeds after — proves the binding survives and the lead still gets nudged once the probe recovers. **Both directions proven, as asked:** - With the narrowed `isLive`: `Tests run: 62, Failures: 0, Errors: 0` (61 existing + 1 new). - Widened back to bare `RuntimeException` (temporary, reverted immediately after): `Tests run: 62, Failures: 1, Errors: 0` — only the new test fails, nothing else. **Full unpiped `mvn clean install`:** `Tests run: 1415, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. Pushed as `d6ef0c8`.
agent added 1 commit 2026-09-06 14:55:17 +02:00
fleetd #368: a stale lead delegation binding must not shadow the primary fallback
CI / contract (pull_request) Successful in 51s
Details
CI / build (pull_request) Successful in 1m55s
Details
5af786d135 
PrimaryRegistry.forgetDelegation only fires when a WORKER is released, never when
the delegating LEAD terminal itself disappears (closed, crashed, or relaunched).
A stale, non-null leadByTarget entry always beat nudgeTargetFor's single-primary
fallback, so a dead lead silently swallowed every reply nudge for its workers.

Fix: ReplyPushLoop now verifies (via the same agents.status check decide() already
uses every tick) that a recorded delegating lead is actually live before trusting
it. A dead lead is treated as if never recorded — self-healing the binding
(mirroring AgentControl.paneByTerminal's self-heal on agent_not_found) and falling
through to PrimaryRegistry's existing fallback.
agent added 1 commit 2026-09-06 15:08:27 +02:00
fleetd #368 review: only agent_not_found may forget a lead binding
CI / build (pull_request) Successful in 1m22s
Details
CI / contract (pull_request) Successful in 1m22s
Details
d6ef0c8013 
isLive treated any RuntimeException from the liveness probe as "the lead is
gone", which forgetDelegation then acted on destructively and permanently.
That made a transient herdr hiccup (socket blip, decode error) on a perfectly
live lead indistinguishable from the lead actually being dead — the same
one-bad-reading mistake #359 shipped a guard against for lead-tab liveness.

Narrow isLive to match AgentControl.agentCall's own rule: only an affirmative
HerdrException("agent_not_found") counts as gone. Every other failure is
treated as still live and the binding is left alone.

Adds aTransientLivenessFailureMustNotForgetABindingToAStillLiveLead, which
fails with the bare RuntimeException catch and passes with the narrowed one.
ltms commented 2026-09-06 15:18:05 +02:00
Owner
Copy Link
Copy Source

Merged into main as fef287c (merge commit, not via the forge button), so closing this PR.

Round 2 (d6ef0c8) is what made it mergeable: isLive now treats only agent_not_found as death, and every other failure as live. Full reasoning and the verified numbers are in #368.

Post-merge on main: mvn clean install → Tests run: 1423, Failures: 0, Errors: 0 — BUILD SUCCESS.

Merged into `main` as `fef287c` (merge commit, not via the forge button), so closing this PR. Round 2 (`d6ef0c8`) is what made it mergeable: `isLive` now treats only `agent_not_found` as death, and every other failure as live. Full reasoning and the verified numbers are in #368. Post-merge on `main`: `mvn clean install` → Tests run: 1423, Failures: 0, Errors: 0 — BUILD SUCCESS.
ltms closed this pull request 2026-09-06 15:18:09 +02:00
Some checks are pending
CI / build (pull_request) Successful in 1m22s
Details
CI / contract (pull_request) Successful in 1m22s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#371
Delete Branch "worker/fleetd-368-stale-lead-binding-f5682e-2"

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?