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

A herdr throw during a lead roll leaves status(token) reporting IN_PROGRESS forever #615

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

Found by review on 2026-09-22 of the already-merged lead-rollover work (a7aee5b, merged 2026-09-19 via PR #600). That content reached main unreviewed and is live: the running daemon's jar was built from 076cc43, which contains it.

Severity: high. It defeats the stated purpose of the feature it sits in, and it is in the lead-handover path — the operator's top-priority work.

The defect

LeadRollover.runRollover makes two agents.send calls that are not wrapped:

  • LeadRollover.java:522 — agents.send(lead, "/clear")
  • LeadRollover.java:540 — agents.send(lead, cfg.bootstrapTextFor(p.handoverPath()))

AgentControl.send → agentCall → herdr.call, and HerdrException extends RuntimeException (HerdrException.java:9), so it is unchecked and nothing forces handling. AgentControl rethrows it (AgentControl.java:52, :55) for any code other than a resolvable agent_not_found.

Why the throw is not contained

confirm() writes IN_PROGRESS into outcomes before it schedules the continuation:

LeadRollover.java:480   outcomes.put(token, new RollStatus(RollState.IN_PROGRESS, ...));
LeadRollover.java:487   continuationRunner.accept(() -> runRollover(p, cfg));

and the production runner is a bare virtual thread with no uncaught-exception handler:

LeadRollover.java:316   r -> Thread.ofVirtual().name("lead-rollover-continuation-").start(r)

So a HerdrException out of either send kills the continuation thread. No terminal outcome is ever written. status(token) returns IN_PROGRESS permanently.

Why this matters more than a stuck field

LeadRollover#status's own javadoc says it exists because "every failure past confirm used to be a log.warn a lead can never read … this is the only route back". This path makes that only route back confidently wrong. The lead is told the roll is still running, forever, for a roll that died.

It is also a sentinel conflation: IN_PROGRESS now means both "running normally" and "the continuation threw and is gone". Those need opposite handling and a caller cannot tell them apart. The other two failure exits (TURN_NEVER_SETTLED, CLEAR_NEVER_SETTLED) each write a terminal outcome with a diagnostic message; only the throw path writes nothing.

The author did think about this — just not here

waitUntilAtTurnBoundary already wraps its agents.status(target) call in try { … } catch (RuntimeException e). So agents.* throwing was considered in the settle helpers and not on the two send calls. That inconsistency reads as an oversight rather than a decision.

Test gap

No test makes send() throw. Checked with a positive control, because a zero match is not a finding on its own:

  • The tests build a real AgentControl over a fake HerdrClient, so a throw would come from the fake, not from a mocking framework — my first grep used the wrong shape.
  • The file does intercept agent.prompt twice (LeadRolloverTest.java:303, :435), and both branches return normally after flipping the fake's status. So a throwing test would have been easy to write here and simply does not exist.
  • The only three assertThrows in the file (:411, :421, :422) are all on open() argument validation.

Suggested fix

Wrap the continuation body and write a terminal outcome on the throw path. Prefer a new state over reusing an existing one: the point is to give the caller a third answer, so "died" stops being indistinguishable from "running". The message should name the exception so an operator has something to act on.

Whoever takes this should also check the same shape on any other path that writes IN_PROGRESS before handing work to the continuation runner, and report what else has it rather than fixing it in the same change.

Verification note

The reviewer reported this; I verified every link myself in the main clone before filing — the two unwrapped calls, the unchecked exception type, the write-before-schedule ordering, the handler-less virtual thread, the wrapped status call for contrast, and the test gap with its control.

Found by review on 2026-09-22 of the **already-merged** lead-rollover work (`a7aee5b`, merged 2026-09-19 via PR #600). That content reached `main` unreviewed and is live: the running daemon's jar was built from `076cc43`, which contains it. Severity: **high**. It defeats the stated purpose of the feature it sits in, and it is in the lead-handover path — the operator's top-priority work. ## The defect `LeadRollover.runRollover` makes two `agents.send` calls that are not wrapped: - `LeadRollover.java:522` — `agents.send(lead, "/clear")` - `LeadRollover.java:540` — `agents.send(lead, cfg.bootstrapTextFor(p.handoverPath()))` `AgentControl.send` → `agentCall` → `herdr.call`, and `HerdrException extends RuntimeException` (`HerdrException.java:9`), so it is unchecked and nothing forces handling. `AgentControl` rethrows it (`AgentControl.java:52`, `:55`) for any code other than a resolvable `agent_not_found`. ## Why the throw is not contained `confirm()` writes `IN_PROGRESS` into `outcomes` **before** it schedules the continuation: ``` LeadRollover.java:480 outcomes.put(token, new RollStatus(RollState.IN_PROGRESS, ...)); LeadRollover.java:487 continuationRunner.accept(() -> runRollover(p, cfg)); ``` and the production runner is a bare virtual thread with no uncaught-exception handler: ``` LeadRollover.java:316 r -> Thread.ofVirtual().name("lead-rollover-continuation-").start(r) ``` So a `HerdrException` out of either `send` kills the continuation thread. No terminal outcome is ever written. `status(token)` returns `IN_PROGRESS` **permanently**. ## Why this matters more than a stuck field `LeadRollover#status`'s own javadoc says it exists because "every failure past `confirm` used to be a `log.warn` a lead can never read … this is the only route back". This path makes that only route back confidently wrong. The lead is told the roll is still running, forever, for a roll that died. It is also a sentinel conflation: `IN_PROGRESS` now means both "running normally" and "the continuation threw and is gone". Those need opposite handling and a caller cannot tell them apart. The other two failure exits (`TURN_NEVER_SETTLED`, `CLEAR_NEVER_SETTLED`) each write a terminal outcome with a diagnostic message; only the throw path writes nothing. ## The author did think about this — just not here `waitUntilAtTurnBoundary` already wraps its `agents.status(target)` call in `try { … } catch (RuntimeException e)`. So `agents.*` throwing was considered in the settle helpers and not on the two `send` calls. That inconsistency reads as an oversight rather than a decision. ## Test gap No test makes `send()` throw. Checked with a positive control, because a zero match is not a finding on its own: - The tests build a real `AgentControl` over a fake `HerdrClient`, so a throw would come from the fake, not from a mocking framework — my first grep used the wrong shape. - The file **does** intercept `agent.prompt` twice (`LeadRolloverTest.java:303`, `:435`), and both branches return normally after flipping the fake's status. So a throwing test would have been easy to write here and simply does not exist. - The only three `assertThrows` in the file (`:411`, `:421`, `:422`) are all on `open()` argument validation. ## Suggested fix Wrap the continuation body and write a **terminal** outcome on the throw path. Prefer a new state over reusing an existing one: the point is to give the caller a third answer, so "died" stops being indistinguishable from "running". The message should name the exception so an operator has something to act on. Whoever takes this should also check the same shape on any other path that writes `IN_PROGRESS` before handing work to the continuation runner, and report what else has it rather than fixing it in the same change. ## Verification note The reviewer reported this; I verified every link myself in the main clone before filing — the two unwrapped calls, the unchecked exception type, the write-before-schedule ordering, the handler-less virtual thread, the wrapped `status` call for contrast, and the test gap with its control.
ltms commented 2026-09-22 05:21:30 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged (PR #617)

The continuation body is now wrapped in one try/catch (RuntimeException), and the catch writes a new terminal state RollState.FAILED naming the exception. The catch is scoped to the whole method rather than to each send call, so it also covers anything added to the continuation later.

Verified by me, independently of the worker's own proof

  • My own build, mvn -o clean install in a scratch worktree, unpiped: Tests run: 1866, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. Matches the worker's reported totals.
  • My own mutation, deliberately different from the worker's. The worker proved its tests red by removing the whole try/catch. That shows the tests notice a propagating exception, but not necessarily that they pin the terminal write. So I mutated the other half: kept the catch and the log, removed only the outcomes.put.
[ERROR] LeadRolloverTest.sendThrowingOnClearLeavesStatusReportingFailed:1397
  ... ==> expected: <FAILED> but was: <IN_PROGRESS>
[ERROR] LeadRolloverTest.sendThrowingOnBootstrapTextLeavesStatusReportingFailed:1441
  ... ==> expected: <FAILED> but was: <IN_PROGRESS>
[ERROR] Tests run: 45, Failures: 2, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

Both tests fail, and they fail by reproducing this ticket's exact defect — IN_PROGRESS where a terminal state belongs. That is runtime behavioural evidence, not a source-text assertion: the assertion reads what status(token) actually returns.

Tree confirmed clean after the revert — mutant markers 0, git diff --stat empty, git status --short empty.

Both exits are pinned separately, which was acceptance criterion 2: a throw on the /clear send and a throw on the bootstrap-text send are different exits, the second reachable only after /clear already succeeded and the pane settled. One test would not have covered both.

The sweep came back clean

The worker checked what else has this shape and reported, without fixing:

  • exactly one continuationRunner.accept call site in the class — the one this fix covers;
  • exactly one place writing a non-terminal state before scheduling — the same confirm(), covered;
  • the two agents.status calls and the one agents.submit call each already had their own local try/catch (RuntimeException) before this change.

So the two agents.send calls were the only unwrapped agents.* calls in the class. That matches what I read when filing this ticket.

Note for whoever reads the fix

The catch deliberately takes RuntimeException, not Exception or Throwable, following the convention waitUntilAtTurnBoundary already set locally — so an OutOfMemoryError still propagates rather than being recorded as a failed roll.

One consequence worth knowing: in tests the continuation runs synchronously via Runnable::run, so before this change a throw surfaced at the confirm() call site. It no longer does. That is the intended behaviour change, and the full suite passes with it.

Not deployed yet

The running daemon still holds the jar built from 076cc43. A merge is not a deployment, so the live daemon still has this defect until the next redeploy. I am batching the redeploy rather than doing one per merge.

Closing.

## Fixed and merged (PR #617) The continuation body is now wrapped in one `try`/`catch (RuntimeException)`, and the catch writes a new terminal state `RollState.FAILED` naming the exception. The catch is scoped to the whole method rather than to each `send` call, so it also covers anything added to the continuation later. ### Verified by me, independently of the worker's own proof - **My own build**, `mvn -o clean install` in a scratch worktree, unpiped: `Tests run: 1866, Failures: 0, Errors: 0, Skipped: 0` — BUILD SUCCESS. Matches the worker's reported totals. - **My own mutation, deliberately different from the worker's.** The worker proved its tests red by removing the whole `try`/`catch`. That shows the tests notice a propagating exception, but not necessarily that they pin the *terminal write*. So I mutated the other half: **kept the catch and the log, removed only the `outcomes.put`.** ``` [ERROR] LeadRolloverTest.sendThrowingOnClearLeavesStatusReportingFailed:1397 ... ==> expected: <FAILED> but was: <IN_PROGRESS> [ERROR] LeadRolloverTest.sendThrowingOnBootstrapTextLeavesStatusReportingFailed:1441 ... ==> expected: <FAILED> but was: <IN_PROGRESS> [ERROR] Tests run: 45, Failures: 2, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` Both tests fail, and they fail by reproducing this ticket's exact defect — `IN_PROGRESS` where a terminal state belongs. That is runtime behavioural evidence, not a source-text assertion: the assertion reads what `status(token)` actually returns. Tree confirmed clean after the revert — mutant markers 0, `git diff --stat` empty, `git status --short` empty. **Both exits are pinned separately**, which was acceptance criterion 2: a throw on the `/clear` send and a throw on the bootstrap-text send are different exits, the second reachable only after `/clear` already succeeded and the pane settled. One test would not have covered both. ### The sweep came back clean The worker checked what else has this shape and reported, without fixing: - exactly **one** `continuationRunner.accept` call site in the class — the one this fix covers; - exactly **one** place writing a non-terminal state before scheduling — the same `confirm()`, covered; - the two `agents.status` calls and the one `agents.submit` call each already had their own local `try/catch (RuntimeException)` before this change. So the two `agents.send` calls were the only unwrapped `agents.*` calls in the class. That matches what I read when filing this ticket. ### Note for whoever reads the fix The catch deliberately takes `RuntimeException`, not `Exception` or `Throwable`, following the convention `waitUntilAtTurnBoundary` already set locally — so an `OutOfMemoryError` still propagates rather than being recorded as a failed roll. One consequence worth knowing: in tests the continuation runs synchronously via `Runnable::run`, so before this change a throw surfaced at the `confirm()` call site. It no longer does. That is the intended behaviour change, and the full suite passes with it. ### Not deployed yet The running daemon still holds the jar built from `076cc43`. A merge is not a deployment, so the live daemon still has this defect until the next redeploy. I am batching the redeploy rather than doing one per merge. Closing.
ltms closed this issue 2026-09-22 05:21:34 +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#615
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?