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

A lead's delegation binding outlives the lead, so reply nudges keep going to a dead terminal #368

New Issue
Closed
opened 2026-09-05 08:27:05 +02:00 by ltms · 1 comment
ltms commented 2026-09-05 08:27:05 +02:00
Owner
Copy Link
Copy Source

Same shape as #359, on the delegation and nudge path instead of the lead-coordination path: a terminal id is read from a map that outlives the thing it names, and nothing checks the thing is still there.

Found by a worker sweeping for that shape while fixing #359. I confirmed the call sites below by reading the code. I did not reproduce it on a live daemon, so the consequence section is reasoning about what the code does, not a measurement.

The lifecycle is one-sided

PrimaryRegistry.leadByTarget maps a worker's terminal to the terminal of the lead that delegated to it.

Written — PrimaryRegistry.java:83, from the accepted-delivery hook (FleetMcp.java:336):

leadByTarget.put(target, leadTerminal);

Removed — PrimaryRegistry.java:89, and there is exactly one caller, Fleetd.java:600:

primaryRegistry.forgetDelegation(detail.terminalId()); // CB-532: don't leak the lead binding

That fires when the worker is released. Nothing removes the entry when the lead terminal goes away — a lead that is closed, crashes, or is relaunched (which #359 shows happens on every daemon restart on fleet01). The map is keyed by the worker, so a dead lead's bindings sit there until each of its workers is torn down.

What then reads it

PrimaryRegistry.nudgeTargetFor (PrimaryRegistry.java:103):

String lead = target == null ? null : leadByTarget.get(target);
return lead != null ? Optional.of(lead) : Optional.ofNullable(terminal.get());

ReplyPushLoop calls this at five sites (lines 388, 416, 451, 479, 501) and acts on whatever comes back, with no liveness check on the lead terminal first.

Why the fallback makes it worse, not better

The javadoc explains the fallback well:

"The fallback matters after a daemon restart, which loses the map while the durable inbox keeps the reply. With one lead the fallback is unambiguous and correct."

That reasoning holds when the map is empty. It does not hold when the map holds a stale entry, which is the case here: a stale lead is non-null, so the Optional.of(lead) branch wins and the fallback to the live primary is never reached. The one case the fallback was written for is the case a stale binding hides.

So after a lead is relaunched, its workers' replies keep nudging the terminal of the lead that is gone, and never nudge the live one. Delivery degrades to pull. That is not data loss — the durable inbox still holds the reply — but the observable effect is a lead that stops waking up for its workers' results, with no error saying why.

What I did not check

  • Whether herdr ever reuses a term_* id. If it does, a stale binding could nudge the wrong live pane rather than a dead one, which is a different and worse bug. The ids look like random hex rather than counters, so I doubt it, but I did not verify it.
  • Whether agents.send to a dead terminal throws, logs, or is silently swallowed at each of the five sites.

Both are worth settling before choosing a fix.

Suggested direction (candidate, not a decided fix)

The parallel with #359 suggests the same shape of answer: do not trust a recorded id without checking the thing still exists. Options, none tested:

  • check the lead terminal is live before nudging it, and fall through to the existing primary fallback when it is not;
  • drop a lead's bindings when that lead terminal disappears, so the map returns to the empty state the fallback already handles correctly;
  • key the binding on something that cannot go stale across a lead relaunch.

The second one is closest to the existing design, because it restores the exact precondition the fallback's javadoc already argues is correct.

Notes

  • AgentControl.paneByTerminal has the same map shape but is not a candidate: every caller goes through agentCall, which invalidates and re-resolves once on agent_not_found, so it self-heals on use.
  • MemberRegistry.terminalToSlot was checked and is probably fine — unbind has one call site in SessionManager.releaseRemoved, which looks like the canonical teardown — but nobody has confirmed that every release cause (idle reap, drain, crash, shutdown) really funnels through it. Unconfirmed, not clean.
  • Related: #359 (the same shape, on the lead-coordination path).
Same shape as #359, on the delegation and nudge path instead of the lead-coordination path: **a terminal id is read from a map that outlives the thing it names, and nothing checks the thing is still there.** Found by a worker sweeping for that shape while fixing #359. I confirmed the call sites below by reading the code. **I did not reproduce it on a live daemon**, so the consequence section is reasoning about what the code does, not a measurement. ## The lifecycle is one-sided `PrimaryRegistry.leadByTarget` maps a worker's terminal to the terminal of the lead that delegated to it. Written — `PrimaryRegistry.java:83`, from the accepted-delivery hook (`FleetMcp.java:336`): ```java leadByTarget.put(target, leadTerminal); ``` Removed — `PrimaryRegistry.java:89`, and there is exactly **one** caller, `Fleetd.java:600`: ```java primaryRegistry.forgetDelegation(detail.terminalId()); // CB-532: don't leak the lead binding ``` That fires when the **worker** is released. Nothing removes the entry when the **lead** terminal goes away — a lead that is closed, crashes, or is relaunched (which #359 shows happens on every daemon restart on fleet01). The map is keyed by the worker, so a dead lead's bindings sit there until each of its workers is torn down. ## What then reads it `PrimaryRegistry.nudgeTargetFor` (`PrimaryRegistry.java:103`): ```java String lead = target == null ? null : leadByTarget.get(target); return lead != null ? Optional.of(lead) : Optional.ofNullable(terminal.get()); ``` `ReplyPushLoop` calls this at five sites (lines 388, 416, 451, 479, 501) and acts on whatever comes back, with no liveness check on the lead terminal first. ## Why the fallback makes it worse, not better The javadoc explains the fallback well: > *"The fallback matters after a daemon restart, which loses the map while the durable inbox keeps the reply. With one lead the fallback is unambiguous and correct."* That reasoning holds when the map is **empty**. It does not hold when the map holds a **stale** entry, which is the case here: a stale `lead` is non-null, so the `Optional.of(lead)` branch wins and the fallback to the live primary is never reached. The one case the fallback was written for is the case a stale binding hides. So after a lead is relaunched, its workers' replies keep nudging the terminal of the lead that is gone, and never nudge the live one. Delivery degrades to pull. That is not data loss — the durable inbox still holds the reply — but the observable effect is a lead that stops waking up for its workers' results, with no error saying why. ## What I did not check - Whether herdr ever reuses a `term_*` id. If it does, a stale binding could nudge the **wrong live pane** rather than a dead one, which is a different and worse bug. The ids look like random hex rather than counters, so I doubt it, but I did not verify it. - Whether `agents.send` to a dead terminal throws, logs, or is silently swallowed at each of the five sites. Both are worth settling before choosing a fix. ## Suggested direction (candidate, not a decided fix) The parallel with #359 suggests the same shape of answer: do not trust a recorded id without checking the thing still exists. Options, none tested: - check the lead terminal is live before nudging it, and fall through to the existing primary fallback when it is not; - drop a lead's bindings when that lead terminal disappears, so the map returns to the empty state the fallback already handles correctly; - key the binding on something that cannot go stale across a lead relaunch. The second one is closest to the existing design, because it restores the exact precondition the fallback's javadoc already argues is correct. ## Notes - `AgentControl.paneByTerminal` has the same map shape but is **not** a candidate: every caller goes through `agentCall`, which invalidates and re-resolves once on `agent_not_found`, so it self-heals on use. - `MemberRegistry.terminalToSlot` was checked and is probably fine — `unbind` has one call site in `SessionManager.releaseRemoved`, which looks like the canonical teardown — but nobody has confirmed that every release cause (idle reap, drain, crash, shutdown) really funnels through it. Unconfirmed, not clean. - Related: #359 (the same shape, on the lead-coordination path).
ltms commented 2026-09-06 15:17:47 +02:00
Author
Owner
Copy Link
Copy Source

Merged as fef287c (PR #371).

ReplyPushLoop now checks the recorded lead with agents.status before trusting it, at all five entry points (onReplyQueued, onTicketTerminal, onQuestionOpened, onBackendIncident, onBackendTargetUnmapped), and forgets a lead that is really gone so resolution reaches the single-lead fallback.

Round 1 treated any RuntimeException as death, and death here calls forgetDelegation — which is destructive and permanent on one reading. A socket blip or a decode error on a live lead would have unbound it forever. I measured that the breadth was unpinned (narrowing it left 1414 tests green), so it went back for round 2, which narrowed it to the one affirmative signal AgentControl.agentCall already uses: agent_not_found. Everything else is treated as live, because a wrong guess there is unrecoverable while a wrong guess the other way costs one retry on the next tick.

The fallback it lands on is live, not stale: PrimaryRegistry.record overwrites the single slot on every orchestration-side MCP call, and neither host pins primary.terminal, so a relaunched lead re-registers on its first tool call.

Verified on the merge, not taken from the worker's report:

mvn clean install   Tests run: 1423, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS

Mutation run on a half the worker did not touch — dropped forgetDelegation from resolveLiveLead while still returning the fallback, so the first nudge behaves identically and only the self-healing is lost: 2 failures in ReplyPushLoopTest, BUILD FAILURE. The cleanup is pinned, not just the fallback.

Wiki: 11-Features.md → Reply nudges follow the delegating lead (second gotcha), 42b66db.

Merged as `fef287c` (PR #371). `ReplyPushLoop` now checks the recorded lead with `agents.status` before trusting it, at all five entry points (`onReplyQueued`, `onTicketTerminal`, `onQuestionOpened`, `onBackendIncident`, `onBackendTargetUnmapped`), and forgets a lead that is really gone so resolution reaches the single-lead fallback. Round 1 treated **any** `RuntimeException` as death, and death here calls `forgetDelegation` — which is destructive and permanent on one reading. A socket blip or a decode error on a live lead would have unbound it forever. I measured that the breadth was unpinned (narrowing it left 1414 tests green), so it went back for round 2, which narrowed it to the one affirmative signal `AgentControl.agentCall` already uses: `agent_not_found`. Everything else is treated as live, because a wrong guess there is unrecoverable while a wrong guess the other way costs one retry on the next tick. The fallback it lands on is live, not stale: `PrimaryRegistry.record` overwrites the single slot on every orchestration-side MCP call, and neither host pins `primary.terminal`, so a relaunched lead re-registers on its first tool call. Verified on the merge, not taken from the worker's report: ``` mvn clean install Tests run: 1423, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS ``` Mutation run on a half the worker did not touch — dropped `forgetDelegation` from `resolveLiveLead` while still returning the fallback, so the first nudge behaves identically and only the self-healing is lost: 2 failures in `ReplyPushLoopTest`, BUILD FAILURE. The cleanup is pinned, not just the fallback. Wiki: `11-Features.md` → *Reply nudges follow the delegating lead* (second gotcha), `42b66db`.
ltms closed this issue 2026-09-06 15:17:55 +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#368
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?