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

reapIdle can release a session that just became BUSY, contradicting its own documented invariant #310

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

Raised by a delegated hunter at low confidence, which was the right call. I checked it and the race is real; the harm is smaller than the report suggested, and I want both facts recorded.

The gap between the check and the act

reapIdle decides from a snapshot and then acts without re-reading:

for (MemberSession s : roster()) {                 // snapshot
    if (s.state() != State.READY && s.state() != State.DONE) {
        continue;                                   // the state check, on the snapshot
    }
    long idleNanos = now - s.lastActivityAtNanos();
    if (idleNanos > idleTtlNanos) {
        ...
        release(s.paneId());                        // the act, with no re-check

release re-reads nothing about state. It removes the registry entry and tears the session down whatever state it is in now.

Meanwhile onDelivered flips exactly those two states to BUSY:

if (current.state() != State.READY && current.state() != State.DONE) {
    return;
}
...
MemberSession updated = current.withState(State.BUSY).bumpTurn(now);
if (replace(current, updated)) {

So a delivery landing between the loop's check and its release call tears down a session that is now BUSY — which reapIdle's own javadoc says it never does:

... never a SPAWNING or BUSY worker.

The method is documented as safe and is not.

Direction of harm — weaker than reported

The hunter wrote that "a freshly-dispatched turn silently vanishes". I do not think "silently" is right, and the distinction matters for how much this is worth:

  • release notifies the release listener, so a lead blocked on that send fails fast with a real reason rather than hanging. The lead is told.
  • The worktree is not a loss either. The turn was dispatched microseconds ago, so the tree is clean and holds no work. release would remove it, but there is nothing in it to remove.

What is actually wrong: a worker that had just been handed a turn is killed for being idle, the turn does not run, and the lead sees a failure it cannot explain from the logs, because from the reaper's point of view everything looked idle. Add that bumpTurn refreshes lastActivityAtNanos, so the very act that should have saved this session from the reaper is the one that races it.

The window is a handful of statements. Neither of us has observed it.

What I want

Goal: reapIdle must not release a session that is no longer in the state it was reaped for, and its javadoc must be true.

Invariants:

  1. One session that fails to release must still not abort the pass. The existing per-session try/catch (CB-581) stays.
  2. Do not narrow this to a re-read immediately before the call. That shrinks the window without closing it and leaves the code looking correct, which is worse than the current honest gap. If you cannot close it, say so rather than papering over it.
  3. Whatever you add must not make an ordinary reap slower or lock-heavy. The reaper runs on a timer over the whole roster.

Candidate mechanism, as a candidate only: a compare-and-release — tear down only if the registry still holds the exact session the loop decided about, so a delivery that replaced the entry causes the reap to skip instead. The registry already does compare-based replacement (replace(current, updated)), so the primitive may exist. Read how replace and registry.remove actually work before you commit to this, and if the honest answer is that release needs to take a lock the delivery path also takes, say so and argue it.

This one is a design decision more than a patch. I would rather have a well-argued "here is why the compare-and-release is not sufficient, and here is what is" than a quick change that closes the visible half.

Rules

  • Prove it with a test that fails without the fix. A deterministic test needs a seam between the state check and the release — find it, and if the only way to write the test is to reach around the class and break an invariant no real caller breaks, do not write it: say so, and record the negative result in the javadoc instead. A red test that got red by violating an invariant is not evidence.
  • Mutation proof required if you have a test: revert the fix, quote the real failure output, restore it.
  • Do not run git stash — the stash is shared across every worktree here.
  • Run cd fleetd && mvn clean install unpiped, and quote the real Tests run: and BUILD lines. Never pipe maven through tail/head, and never read $? after a pipe — after a pipe it is the last command's status, not Maven's.
  • If you conclude the race is not reachable, that is a perfectly good outcome. Report it with the reasoning and change only the javadoc. Do not invent a fix to have something to show.
Raised by a delegated hunter at low confidence, which was the right call. I checked it and the race is real; the harm is smaller than the report suggested, and I want both facts recorded. ## The gap between the check and the act `reapIdle` decides from a snapshot and then acts without re-reading: ```java for (MemberSession s : roster()) { // snapshot if (s.state() != State.READY && s.state() != State.DONE) { continue; // the state check, on the snapshot } long idleNanos = now - s.lastActivityAtNanos(); if (idleNanos > idleTtlNanos) { ... release(s.paneId()); // the act, with no re-check ``` `release` re-reads nothing about state. It removes the registry entry and tears the session down whatever state it is in now. Meanwhile `onDelivered` flips exactly those two states to `BUSY`: ```java if (current.state() != State.READY && current.state() != State.DONE) { return; } ... MemberSession updated = current.withState(State.BUSY).bumpTurn(now); if (replace(current, updated)) { ``` So a delivery landing between the loop's check and its `release` call tears down a session that is now `BUSY` — which `reapIdle`'s own javadoc says it never does: > ... never a `SPAWNING` or `BUSY` worker. The method is documented as safe and is not. ## Direction of harm — weaker than reported The hunter wrote that "a freshly-dispatched turn silently vanishes". I do not think "silently" is right, and the distinction matters for how much this is worth: - `release` notifies the release listener, so a lead blocked on that send **fails fast with a real reason** rather than hanging. The lead is told. - The worktree is not a loss either. The turn was dispatched microseconds ago, so the tree is clean and holds no work. `release` would remove it, but there is nothing in it to remove. What is actually wrong: a worker that had just been handed a turn is killed for being idle, the turn does not run, and the lead sees a failure it cannot explain from the logs, because from the reaper's point of view everything looked idle. Add that `bumpTurn` refreshes `lastActivityAtNanos`, so the very act that should have saved this session from the reaper is the one that races it. The window is a handful of statements. Neither of us has observed it. ## What I want **Goal:** `reapIdle` must not release a session that is no longer in the state it was reaped for, and its javadoc must be true. **Invariants:** 1. One session that fails to release must still not abort the pass. The existing per-session try/catch (CB-581) stays. 2. Do not narrow this to a re-read immediately before the call. That shrinks the window without closing it and leaves the code looking correct, which is worse than the current honest gap. If you cannot close it, say so rather than papering over it. 3. Whatever you add must not make an ordinary reap slower or lock-heavy. The reaper runs on a timer over the whole roster. **Candidate mechanism**, as a candidate only: a compare-and-release — tear down only if the registry still holds the exact session the loop decided about, so a delivery that replaced the entry causes the reap to skip instead. The registry already does compare-based replacement (`replace(current, updated)`), so the primitive may exist. **Read how `replace` and `registry.remove` actually work before you commit to this**, and if the honest answer is that `release` needs to take a lock the delivery path also takes, say so and argue it. **This one is a design decision more than a patch.** I would rather have a well-argued "here is why the compare-and-release is not sufficient, and here is what is" than a quick change that closes the visible half. ## Rules - Prove it with a test that fails without the fix. A deterministic test needs a seam between the state check and the release — find it, and if the only way to write the test is to reach around the class and break an invariant no real caller breaks, **do not write it**: say so, and record the negative result in the javadoc instead. A red test that got red by violating an invariant is not evidence. - Mutation proof required if you have a test: revert the fix, quote the real failure output, restore it. - Do not run `git stash` — the stash is shared across every worktree here. - Run `cd fleetd && mvn clean install` **unpiped**, and quote the real `Tests run:` and `BUILD` lines. Never pipe maven through `tail`/`head`, and never read `$?` after a pipe — after a pipe it is the last command's status, not Maven's. - If you conclude the race is not reachable, that is a perfectly good outcome. Report it with the reasoning and change only the javadoc. **Do not invent a fix to have something to show.**
ltms commented 2026-09-04 08:23:24 +02:00
Author
Owner
Copy Link
Copy Source

Merged as a49671c, with one addition of mine in f159ca7. Both on main.

The design is right

registry.remove(paneId, expected) is ConcurrentHashMap's compare-and-remove, and it is the correct linearization point: onDelivered replaces the immutable record with a BUSY one, so a delivery that lands before the remove makes the compare fail and the reap skip. No shared lock, no cost on an ordinary reap. The public release path stays unconditional, which is right — an explicit fleet_stop should not be refused because the worker just got busy.

The worker also moved the "reaping idle session" debug line to after a successful release. That is a real improvement I did not ask for: the old line announced a reap that might then not happen.

My own verification — a different mutation

The worker mutated the conditional remove back to unconditional and showed its new test go red. I ran a different one, aimed at what this change put most at risk: that the guard might reject every reap, not just racing ones, and silently stop the reaper doing its job. I forced releaseIfCurrent to always return false:

[ERROR] Tests run: 57, Failures: 5 -- in dev.ltms.fleet.session.SessionManagerTest
  readySessionPastIdleTtlIsReaped:640         READY session past TTL is reaped ==> expected: <1> but was: <0>
  doneSessionPastIdleTtlIsReaped:708          DONE session past TTL is reaped  ==> expected: <1> but was: <0>
  reapIdleReturnsCorrectCountAndSkipsBusy:725 only READY past TTL is reaped    ==> expected: <1> but was: <0>
  reapIdleSurvivesOneSessionWhoseLauncherStopFails:1144  no reap-failed WARN logged
  reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails:1076  no WARN logged

Five existing tests catch it, so the ordinary reap path is genuinely covered and this change cannot silently disable reaping. That was the question I actually had.

mvn clean install after restoring: Tests run: 1317, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

What I added

The compare-and-release declined silently. For a race that is unobservable by construction, a reaper that quietly stops reaping is about the hardest thing to diagnose after the fact, so f159ca7 adds one debug line naming the pane and the likely cause.

On the test seam

The worker added a package-private Consumer<MemberSession> hook, null in production, to interleave a real onDelivered between the eligibility check and the remove. I went looking for a way to avoid it before accepting it: nowNanos is read once before the loop, and the PeerLauncher/Worktrees fakes are only reached inside release, which is after the compare — so neither can act at the right moment. Short of real threads and luck, the hook is the only deterministic option.

It is acceptable because it does not reach around the class: it calls the real lifecycle method and breaks no invariant a real caller maintains. That was the line drawn in the ticket, and it stayed on the right side of it.

Correction to my ticket, from the worker

I wrote that MemberRegistry is the compare-based registry to look at. Wrong file — in this path SessionManager owns a plain ConcurrentHashMap, and its existing replace is that map's replace. The worker used the same map's remove(key, value). The pointer in my ticket would have sent someone to the wrong class.

Severity stands as I restated it, not as first reported: the release listener still fails a blocked send with a reason, so this was never a silent loss.

Merged as `a49671c`, with one addition of mine in `f159ca7`. Both on `main`. ## The design is right `registry.remove(paneId, expected)` is `ConcurrentHashMap`'s compare-and-remove, and it is the correct linearization point: `onDelivered` replaces the immutable record with a BUSY one, so a delivery that lands before the remove makes the compare fail and the reap skip. No shared lock, no cost on an ordinary reap. The public `release` path stays unconditional, which is right — an explicit `fleet_stop` should not be refused because the worker just got busy. The worker also moved the "reaping idle session" debug line to *after* a successful release. That is a real improvement I did not ask for: the old line announced a reap that might then not happen. ## My own verification — a different mutation The worker mutated the conditional remove back to unconditional and showed its new test go red. I ran a different one, aimed at what this change put most at risk: that the guard might reject *every* reap, not just racing ones, and silently stop the reaper doing its job. I forced `releaseIfCurrent` to always return false: ``` [ERROR] Tests run: 57, Failures: 5 -- in dev.ltms.fleet.session.SessionManagerTest readySessionPastIdleTtlIsReaped:640 READY session past TTL is reaped ==> expected: <1> but was: <0> doneSessionPastIdleTtlIsReaped:708 DONE session past TTL is reaped ==> expected: <1> but was: <0> reapIdleReturnsCorrectCountAndSkipsBusy:725 only READY past TTL is reaped ==> expected: <1> but was: <0> reapIdleSurvivesOneSessionWhoseLauncherStopFails:1144 no reap-failed WARN logged reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails:1076 no WARN logged ``` Five existing tests catch it, so the ordinary reap path is genuinely covered and this change cannot silently disable reaping. That was the question I actually had. `mvn clean install` after restoring: `Tests run: 1317, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ## What I added The compare-and-release declined **silently**. For a race that is unobservable by construction, a reaper that quietly stops reaping is about the hardest thing to diagnose after the fact, so `f159ca7` adds one debug line naming the pane and the likely cause. ## On the test seam The worker added a package-private `Consumer<MemberSession>` hook, null in production, to interleave a real `onDelivered` between the eligibility check and the remove. I went looking for a way to avoid it before accepting it: `nowNanos` is read once before the loop, and the `PeerLauncher`/`Worktrees` fakes are only reached inside `release`, which is after the compare — so neither can act at the right moment. Short of real threads and luck, the hook is the only deterministic option. It is acceptable because it does not reach around the class: it calls the real lifecycle method and breaks no invariant a real caller maintains. That was the line drawn in the ticket, and it stayed on the right side of it. ## Correction to my ticket, from the worker I wrote that `MemberRegistry` is the compare-based registry to look at. Wrong file — in this path `SessionManager` owns a plain `ConcurrentHashMap`, and its existing `replace` is that map's `replace`. The worker used the same map's `remove(key, value)`. The pointer in my ticket would have sent someone to the wrong class. Severity stands as I restated it, not as first reported: the release listener still fails a blocked send with a reason, so this was never a silent loss.
ltms closed this issue 2026-09-04 08:23:27 +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#310
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?