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

An Error in StatusPoller.loop or SessionReaper.loop kills the thread permanently and silently, and start() then refuses to restart it #538

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

Found while answering a question from the fleet01 lead about Injector's delivery loop. Their hypothesis was re-delivery; the re-delivery is not reachable, but the reason it is not reachable is worse than the thing they were looking for.

Measured on main at f1640f5.

The structure — measured

Injector.onStatus delivers inside synchronized (t), and its delivery try is caught by exactly one clause (Injector.java:391):

} catch (RuntimeException e) {
    // Delivery failed at herdr; drop the poisoned message and surface it
    // rather than blocking the queue behind it.
    t.queue.poll();
    p.state = Pending.State.NOT_DELIVERED;
    ...
}

RuntimeException, not Throwable. grep -n 'catch (' over the whole file returns exactly two clauses, :391 and :495, both RuntimeException.

The only production caller of onStatus is StatusPoller.java:79, inside the per-target body of loop(). That body's catches are HerdrException and RuntimeException. Measured with grep -nE 'catch \((Throwable|Error)': no Throwable or Error catch in the file. Control: the file does contain 3 catch ( clauses, so the pattern works.

loop() is the entry point of a virtual thread (StatusPoller.java:62):

thread = Thread.ofVirtual().name("status-poller").start(this::loop);

What follows — measured from the code, not observed in production

An Error (or any non-RuntimeException Throwable) thrown inside the per-target body:

  1. is not caught by Injector's :391 clause, so t.queue.poll() never runs and p.state stays QUEUED;
  2. unwinds out of synchronized (t), releasing the monitor;
  3. is not caught by StatusPoller.loop's HerdrException or RuntimeException clauses;
  4. escapes loop(), which is the virtual thread's entry point, so the status-poller thread dies.

Then the recovery path is closed, and this is the part that matters:

  • running is only set false at :103 (an InterruptedException in sleep) and :109 (stop()). Neither runs here, so running stays true.
  • start() begins if (running) return;. So a restart attempt is a silent no-op. The poller cannot be brought back without restarting the daemon.
  • grep -rn 'UncaughtExceptionHandler\|setDefaultUncaught' fleetd/src/main/java → no matches. Nothing installs a handler.
  • Nothing anywhere watches the thread's liveness. Fleetd.java:533 constructs the poller and never checks it again.

The consequence is fleet-wide: StatusPoller is the single thread that drives Injector.onStatus for every target. With it dead, no queued message is ever delivered to any member, no turn completion is ever observed, and every blocking fleet_send rides out its timeout. /healthz stays green — it does not know about this thread.

The same shape in SessionReaper — measured

SessionReaper is documented as "Modeled on StatusPoller", and it inherited this too:

SessionReaper.java:57   thread = Thread.ofVirtual().name("session-reaper").start(this::loop);
SessionReaper.java:63   while (running) {
catch clauses:          :66 RuntimeException, :93 RuntimeException, :105 InterruptedException
running = false only at :107 (InterruptedException) and :113 (stop())

Identical. Two singleton daemon loops, one defect.

I checked the other Thread.ofVirtual() sites. The rest are per-task threads or executor factories, where a dead thread costs one task rather than a permanently dead subsystem. These two are the singleton long-lived loops, and they are the two that matter.

Is an Error actually reachable here? — DERIVED, not observed

I have not seen this happen. The argument that it is not merely theoretical:

  • NoClassDefFoundError is the realistic candidate, and it is reachable in this deployment by a documented route. #413 records that building in the fleetd tree disarms the running daemon: a mvn clean deletes target/fleetd.jar while the JVM keeps running on it. Any class not yet loaded then fails on first use. A lazily-loaded class first touched inside the poller body is exactly this bug's trigger.
  • The fleet01 lead measured a real NoClassDefFoundError on their host on 2026-09-10, in a different code path (the shutdown drain). So the error class is not hypothetical in this system; only its arrival inside these two loops is.

I am labelling this derived on purpose. The structural claim is measured; the reachability is an argument.

Why it is bad out of proportion to its likelihood

Three channels, all reassuring, which is this week's recurring shape:

channel what it says when the poller is dead
/healthz green — it does not know this thread exists
the log nothing; the Error goes to an uncaught handler that was never installed
start() returns normally, having done nothing, because running is still true

An operator who suspects the poller and calls start() gets silence and no poller. That is worse than a crash: a dead daemon gets restarted in a minute, while this one keeps answering, keeps accepting fleet_send, and delivers nothing.

The fix

  1. Catch Throwable per iteration, not RuntimeException — in both StatusPoller.loop and SessionReaper.loop. Log it at ERROR with the target, and continue to the next target. One target's Error must not end the loop. This alone removes the permanent-death path.
  2. Make the thread's death observable. On exit from loop() for any reason other than stop(), log at ERROR and set running = false so that start() can actually restart it. A finally in loop() is the right place — note this is the opposite of the finally guidance on the drain-completion line in #512, because here the signal wanted is "the loop exited", which a finally reports correctly.
  3. Consider surfacing it. The health snapshot already reports coverage; a dead injection poller is a more urgent fact than anything else it reports. Out of scope here unless it is cheap.

Do not fix this by installing a global default uncaught-exception handler. That hides the per-loop recovery this needs behind a process-wide net and still leaves running stuck true.

Acceptance

  • A test that throws an Error from the per-target body and asserts the loop survives and polls the next target. This must fail before the fix.
  • A test that a loop exiting abnormally leaves running == false, so start() restarts it. Also failing before the fix.
  • Both loops covered. A fix to one and not the other is half a fix.
  • For each new test: apply a mutation that should break it, show it going red with that test's own message, restore, confirm byte-identical with the full shasum -a 256, and run a green control.
  • No socket, no port bind, no spawn, nothing written outside a @TempDir.

Related: #413 (the route that makes NoClassDefFoundError reachable), #412 (the same family in the completion thread), #512 (a negative check over a channel that cannot carry the failure).

Found while answering a question from the fleet01 lead about `Injector`'s delivery loop. Their hypothesis was re-delivery; the re-delivery is not reachable, but the reason it is not reachable is worse than the thing they were looking for. Measured on main at `f1640f5`. ## The structure — measured `Injector.onStatus` delivers inside `synchronized (t)`, and its delivery `try` is caught by exactly one clause (`Injector.java:391`): ```java } catch (RuntimeException e) { // Delivery failed at herdr; drop the poisoned message and surface it // rather than blocking the queue behind it. t.queue.poll(); p.state = Pending.State.NOT_DELIVERED; ... } ``` `RuntimeException`, not `Throwable`. `grep -n 'catch ('` over the whole file returns exactly two clauses, `:391` and `:495`, both `RuntimeException`. The only production caller of `onStatus` is `StatusPoller.java:79`, inside the per-target body of `loop()`. That body's catches are `HerdrException` and `RuntimeException`. Measured with `grep -nE 'catch \((Throwable|Error)'`: **no `Throwable` or `Error` catch in the file.** Control: the file does contain 3 `catch (` clauses, so the pattern works. `loop()` is the entry point of a virtual thread (`StatusPoller.java:62`): ```java thread = Thread.ofVirtual().name("status-poller").start(this::loop); ``` ## What follows — measured from the code, not observed in production An `Error` (or any non-`RuntimeException` `Throwable`) thrown inside the per-target body: 1. is not caught by `Injector`'s `:391` clause, so `t.queue.poll()` never runs and `p.state` stays `QUEUED`; 2. unwinds out of `synchronized (t)`, releasing the monitor; 3. is not caught by `StatusPoller.loop`'s `HerdrException` or `RuntimeException` clauses; 4. escapes `loop()`, which is the virtual thread's entry point, so **the `status-poller` thread dies.** Then the recovery path is closed, and this is the part that matters: - `running` is only set `false` at `:103` (an `InterruptedException` in `sleep`) and `:109` (`stop()`). Neither runs here, so **`running` stays `true`.** - `start()` begins `if (running) return;`. So a restart attempt is a **silent no-op**. The poller cannot be brought back without restarting the daemon. - `grep -rn 'UncaughtExceptionHandler\|setDefaultUncaught' fleetd/src/main/java` → **no matches.** Nothing installs a handler. - Nothing anywhere watches the thread's liveness. `Fleetd.java:533` constructs the poller and never checks it again. The consequence is fleet-wide: `StatusPoller` is the single thread that drives `Injector.onStatus` for **every** target. With it dead, no queued message is ever delivered to any member, no turn completion is ever observed, and every blocking `fleet_send` rides out its timeout. `/healthz` stays green — it does not know about this thread. ## The same shape in SessionReaper — measured `SessionReaper` is documented as "Modeled on `StatusPoller`", and it inherited this too: ``` SessionReaper.java:57 thread = Thread.ofVirtual().name("session-reaper").start(this::loop); SessionReaper.java:63 while (running) { catch clauses: :66 RuntimeException, :93 RuntimeException, :105 InterruptedException running = false only at :107 (InterruptedException) and :113 (stop()) ``` Identical. Two singleton daemon loops, one defect. I checked the other `Thread.ofVirtual()` sites. The rest are per-task threads or executor factories, where a dead thread costs one task rather than a permanently dead subsystem. These two are the singleton long-lived loops, and they are the two that matter. ## Is an Error actually reachable here? — DERIVED, not observed I have **not** seen this happen. The argument that it is not merely theoretical: - `NoClassDefFoundError` is the realistic candidate, and it is reachable in this deployment by a documented route. **#413** records that building in the `fleetd` tree disarms the running daemon: a `mvn clean` deletes `target/fleetd.jar` while the JVM keeps running on it. Any class not yet loaded then fails on first use. A lazily-loaded class first touched inside the poller body is exactly this bug's trigger. - The fleet01 lead measured a real `NoClassDefFoundError` on their host on 2026-09-10, in a different code path (the shutdown drain). So the error class is not hypothetical in this system; only its arrival inside these two loops is. I am labelling this derived on purpose. The structural claim is measured; the reachability is an argument. ## Why it is bad out of proportion to its likelihood Three channels, all reassuring, which is this week's recurring shape: | channel | what it says when the poller is dead | |---|---| | `/healthz` | green — it does not know this thread exists | | the log | nothing; the `Error` goes to an uncaught handler that was never installed | | `start()` | returns normally, having done nothing, because `running` is still `true` | An operator who suspects the poller and calls `start()` gets silence and no poller. That is worse than a crash: a dead daemon gets restarted in a minute, while this one keeps answering, keeps accepting `fleet_send`, and delivers nothing. ## The fix 1. **Catch `Throwable` per iteration, not `RuntimeException`** — in both `StatusPoller.loop` and `SessionReaper.loop`. Log it at ERROR with the target, and continue to the next target. One target's `Error` must not end the loop. This alone removes the permanent-death path. 2. **Make the thread's death observable.** On exit from `loop()` for any reason other than `stop()`, log at ERROR and set `running = false` so that `start()` can actually restart it. A `finally` in `loop()` is the right place — note this is the *opposite* of the `finally` guidance on the drain-completion line in #512, because here the signal wanted is "the loop exited", which a `finally` reports correctly. 3. **Consider surfacing it.** The health snapshot already reports coverage; a dead injection poller is a more urgent fact than anything else it reports. Out of scope here unless it is cheap. Do **not** fix this by installing a global default uncaught-exception handler. That hides the per-loop recovery this needs behind a process-wide net and still leaves `running` stuck true. ## Acceptance - A test that throws an `Error` from the per-target body and asserts the loop survives and polls the next target. This must fail before the fix. - A test that a loop exiting abnormally leaves `running == false`, so `start()` restarts it. Also failing before the fix. - Both loops covered. A fix to one and not the other is half a fix. - For each new test: apply a mutation that should break it, show it going red with that test's own message, restore, confirm byte-identical with the full `shasum -a 256`, and run a green control. - No socket, no port bind, no spawn, nothing written outside a `@TempDir`. Related: #413 (the route that makes `NoClassDefFoundError` reachable), #412 (the same family in the completion thread), #512 (a negative check over a channel that cannot carry the failure).
ltms commented 2026-09-12 09:03:12 +02:00
Author
Owner
Copy Link
Copy Source

Closed by PR #543, merged after I verified it myself on the merged tree.

Build

mvn -B clean install on the merged tree: exit 0, Tests run: 1716, Failures: 0, Errors: 0, Skipped: 0. That is 1712 on main plus the 4 new tests. The PR body says 1705, measured from base
f1640f5; that is not a disagreement, the base is older than main.

Mutations — I re-applied four myself

The acceptance criterion here was that both loops are fixed, because a fix to one and not the
other is half a fix. So I mutated each half separately and checked the other half's tests stayed
green.

Mutation Killed by Other class's tests
StatusPoller per-target catch (Throwable) → catch (RuntimeException) anErrorForOneTargetDoesNotStopPollingTheNextTarget:38 » Timeout green (2/0/0)
StatusPoller finally, drop running = false anAbnormalExitClearsRunningSoStartCreatesANewLoop:50 — an abnormal loop exit must clear running ==> expected: <false> but was: <true> green (2/0/0)
SessionReaper per-iteration catch (Throwable) → catch (RuntimeException) anErrorInOneIterationDoesNotStopTheNextIteration:44 — the reaper must continue to the next iteration after an Error green (2/0/0)
SessionReaper finally, drop running = false anAbnormalExitClearsRunningSoStartCreatesANewLoop:57, same message green (2/0/0)

All four killed. A kill is its own harness proof, so none needed a separate false-assertion run.
Restores were sha-checked, and both of my shas matched the worker's independently:
273a4e48… for StatusPoller.java, d33567ba… for SessionReaper.java. Green control after the
last restore: exit 0, 1716/0/0/0, clean git status.

The proof cell caught my own mistake

My first proof cell counted the mutated text. For the two deletion mutations the mutated text
is a prefix of the pristine text, so that cell reported "applied" both before and after — a cell
that agrees with whatever you already believed.

It did not get to. I ran the cell against the untouched tree first and required it to report
not-applied, and it aborted:

[M2-poller-finally] proof cell on UNTOUCHED tree: mutated-text count = 1 (must be 0)
ABORT: proof cell cannot discriminate

I switched the cell to count the pristine anchor instead — 1 before, 0 after — and re-ran all
four. This is the one-level antidote the fleet01 lead described: run the cell against a tree where
the mutation was not applied and require it to say so. No deeper turtle is needed.

One thing I checked instead of assuming

SessionReaperResilienceTest's first test counts down a latch on the second call to the
injected clock. That only means "the next iteration" if nothing else in the same iteration calls
the clock. SessionManager.sweepWipRefs does not — it delegates straight to
worktrees.pruneWipRefs. So the second clock call really is the next reapIdle, and the
assertion message is accurate.

Not closed by this

Nothing calls start() a second time. Fleetd constructs and starts each loop exactly once
(Fleetd.java:321 for the reaper, Fleetd.java:533 for the poller). So the new log line
"… loop exited unexpectedly; it can be restarted" names a capability with no restarter behind it.

This change is still a strict improvement: before it, a dead loop left running == true, and
start() begins with if (running) return;, so a restart was impossible even in principle. Now it
is possible. Building the supervisor that actually does it is separate work, filed as a follow-up.

Injector.java was not touched, as the ticket required.

Closed by PR #543, merged after I verified it myself on the merged tree. ## Build `mvn -B clean install` on the merged tree: exit 0, `Tests run: 1716, Failures: 0, Errors: 0, Skipped: 0`. That is 1712 on `main` plus the 4 new tests. The PR body says 1705, measured from base `f1640f5`; that is not a disagreement, the base is older than `main`. ## Mutations — I re-applied four myself The acceptance criterion here was that **both** loops are fixed, because a fix to one and not the other is half a fix. So I mutated each half separately and checked the other half's tests stayed green. | Mutation | Killed by | Other class's tests | |---|---|---| | `StatusPoller` per-target `catch (Throwable)` → `catch (RuntimeException)` | `anErrorForOneTargetDoesNotStopPollingTheNextTarget:38 » Timeout` | green (2/0/0) | | `StatusPoller` `finally`, drop `running = false` | `anAbnormalExitClearsRunningSoStartCreatesANewLoop:50` — `an abnormal loop exit must clear running ==> expected: <false> but was: <true>` | green (2/0/0) | | `SessionReaper` per-iteration `catch (Throwable)` → `catch (RuntimeException)` | `anErrorInOneIterationDoesNotStopTheNextIteration:44` — `the reaper must continue to the next iteration after an Error` | green (2/0/0) | | `SessionReaper` `finally`, drop `running = false` | `anAbnormalExitClearsRunningSoStartCreatesANewLoop:57`, same message | green (2/0/0) | All four killed. A kill is its own harness proof, so none needed a separate false-assertion run. Restores were sha-checked, and both of my shas matched the worker's independently: `273a4e48…` for `StatusPoller.java`, `d33567ba…` for `SessionReaper.java`. Green control after the last restore: exit 0, `1716/0/0/0`, clean `git status`. ## The proof cell caught my own mistake My first proof cell counted the **mutated** text. For the two deletion mutations the mutated text is a prefix of the pristine text, so that cell reported "applied" both before and after — a cell that agrees with whatever you already believed. It did not get to. I ran the cell against the untouched tree first and required it to report *not-applied*, and it aborted: ``` [M2-poller-finally] proof cell on UNTOUCHED tree: mutated-text count = 1 (must be 0) ABORT: proof cell cannot discriminate ``` I switched the cell to count the **pristine** anchor instead — 1 before, 0 after — and re-ran all four. This is the one-level antidote the fleet01 lead described: run the cell against a tree where the mutation was not applied and require it to say so. No deeper turtle is needed. ## One thing I checked instead of assuming `SessionReaperResilienceTest`'s first test counts down a latch on the **second** call to the injected clock. That only means "the next iteration" if nothing else in the same iteration calls the clock. `SessionManager.sweepWipRefs` does not — it delegates straight to `worktrees.pruneWipRefs`. So the second clock call really is the next `reapIdle`, and the assertion message is accurate. ## Not closed by this **Nothing calls `start()` a second time.** `Fleetd` constructs and starts each loop exactly once (`Fleetd.java:321` for the reaper, `Fleetd.java:533` for the poller). So the new log line `"… loop exited unexpectedly; it can be restarted"` names a capability with no restarter behind it. This change is still a strict improvement: before it, a dead loop left `running == true`, and `start()` begins with `if (running) return;`, so a restart was impossible even in principle. Now it is possible. Building the supervisor that actually does it is separate work, filed as a follow-up. `Injector.java` was not touched, as the ticket required.
ltms closed this issue 2026-09-12 09:03:17 +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#538
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?