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

A worker spawned during the shutdown drain is never drained: its pane is left running and its worktree is never preserved #308

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

Found by a delegated hunter; I read every line cited and confirmed it.

The snapshot

drainAll iterates a one-shot copy of the registry:

void drainAll(long timeoutNanos) {
    long deadline = System.nanoTime() + timeoutNanos;
    for (MemberSession s : roster()) {          // List.copyOf(registry.values()) — taken once

Nothing re-checks the registry after the loop, and there is no guard anywhere in SessionManager that refuses a new session once draining has started. I grepped for shuttingDown, draining and closed — the only hit is the word "Close" in close's own javadoc.

Meanwhile the shutdown hook keeps the MCP server up for a long time after the drain starts:

sessions.close(...);            // drainAll — takes its snapshot here
poller.stop();
messages.close();
pushLoop.close();
if (heartbeat != null) heartbeat.close();
if (leadCoordLoop != null) leadCoordLoop.close();
...
mcp.close();                    // only now does fleet_spawn stop being accepted

So a fleet_spawn accepted after the snapshot registers a session the drain loop will never visit.

The window is not small

This is the part that moves it out of "theoretical". drainAll waits for each BUSY session against a whole-drain deadline taken from lifecycle.drainTimeoutSeconds, which is 120 in the live config. A drain with a busy worker in it can therefore hold the daemon open for up to two minutes with fleet_spawn still live and still accepting.

Direction of harm

Silent orphan. The spawn returns 201/a normal sessionId and the caller has no way to know the daemon is going down. Then the session is never released: no launcher.stop, so the pane keeps running; no dirty-worktree check, so no preserve-and-log and no refs/wip snapshot; and the registry is in-memory, so the restart erases the only record that the worktree exists. Nothing downstream reclaims it — the same "nothing else can clean this up" property that made #296 worth fixing.

The worker in that pane keeps running after the daemon dies, does real work, and calls fleet_reply into nothing. Its output sits in a worktree no restart knows about.

What I want

Goal: when the daemon shuts down, no registered session is left un-drained, and a caller is never handed a session the daemon has already stopped being able to manage.

Invariants:

  1. drainAll's existing behaviour for sessions in the snapshot must not change — in particular timeoutNanos stays a whole-drain budget, not a per-session grace. A per-session grace would let a slow fleet exceed launchd's exit window and get the daemon SIGKILLed part-way through teardown, which is worse than anything in this ticket.
  2. ReleaseCause.SHUTDOWN must still preserve worktrees. A shutdown drain must never remove one — a worker's uncommitted work has no other copy.
  3. A refused spawn must fail loudly and say why. Silently returning something spawn-shaped would be a new version of this same bug.

Two candidate mechanisms — both are candidates, not instructions, and I think the answer is both together:

  • Refuse new sessions once the drain has begun, so the caller learns the daemon is going down instead of getting an orphan.
  • After the drain loop, re-check the registry and drain whatever arrived, so a spawn that got past the guard is still torn down.

Neither alone is complete: a guard still leaves a race for a spawn already past it, and a sweep alone hands the caller a session that is then immediately destroyed. Decide it yourself and justify it. If you find one is enough, or a third option is better, say so and do that — a tested, reported deviation is a good outcome here.

Rules

  • Prove it with a test that fails without the fix: register a session after the drain snapshot is taken and assert it is still torn down (or that the spawn was refused), whichever your design says.
  • Mutation proof required: revert the fix, quote the real failure output, restore it.
  • Do not run git stash — the stash is shared across worktrees and you would take another worker's in-progress work.
  • 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. A "green" run of mine earlier today had not compiled, for exactly that reason.

Shape check

When done, look in SessionManager.java only for the same shape: a decision made from a roster() snapshot and then acted on without re-checking. One line each, do not fix any of it. I already know of one and am handling it separately, so finding it is a good sign rather than a duplicate.

Found by a delegated hunter; I read every line cited and confirmed it. ## The snapshot `drainAll` iterates a one-shot copy of the registry: ```java void drainAll(long timeoutNanos) { long deadline = System.nanoTime() + timeoutNanos; for (MemberSession s : roster()) { // List.copyOf(registry.values()) — taken once ``` Nothing re-checks the registry after the loop, and there is no guard anywhere in `SessionManager` that refuses a new session once draining has started. I grepped for `shuttingDown`, `draining` and `closed` — the only hit is the word "Close" in `close`'s own javadoc. Meanwhile the shutdown hook keeps the MCP server up for a long time after the drain starts: ```java sessions.close(...); // drainAll — takes its snapshot here poller.stop(); messages.close(); pushLoop.close(); if (heartbeat != null) heartbeat.close(); if (leadCoordLoop != null) leadCoordLoop.close(); ... mcp.close(); // only now does fleet_spawn stop being accepted ``` So a `fleet_spawn` accepted after the snapshot registers a session the drain loop will never visit. ## The window is not small This is the part that moves it out of "theoretical". `drainAll` waits for each `BUSY` session against a whole-drain deadline taken from `lifecycle.drainTimeoutSeconds`, which is **120** in the live config. A drain with a busy worker in it can therefore hold the daemon open for up to two minutes with `fleet_spawn` still live and still accepting. ## Direction of harm Silent orphan. The spawn returns `201`/a normal `sessionId` and the caller has no way to know the daemon is going down. Then the session is never released: no `launcher.stop`, so the pane keeps running; no dirty-worktree check, so no preserve-and-log and no `refs/wip` snapshot; and the registry is in-memory, so the restart erases the only record that the worktree exists. Nothing downstream reclaims it — the same "nothing else can clean this up" property that made #296 worth fixing. The worker in that pane keeps running after the daemon dies, does real work, and calls `fleet_reply` into nothing. Its output sits in a worktree no restart knows about. ## What I want **Goal:** when the daemon shuts down, no registered session is left un-drained, and a caller is never handed a session the daemon has already stopped being able to manage. **Invariants:** 1. `drainAll`'s existing behaviour for sessions in the snapshot must not change — in particular `timeoutNanos` stays a **whole-drain budget**, not a per-session grace. A per-session grace would let a slow fleet exceed launchd's exit window and get the daemon `SIGKILL`ed part-way through teardown, which is worse than anything in this ticket. 2. `ReleaseCause.SHUTDOWN` must still preserve worktrees. A shutdown drain must never remove one — a worker's uncommitted work has no other copy. 3. A refused spawn must fail loudly and say why. Silently returning something spawn-shaped would be a new version of this same bug. **Two candidate mechanisms** — both are candidates, not instructions, and I think the answer is both together: - Refuse new sessions once the drain has begun, so the caller learns the daemon is going down instead of getting an orphan. - After the drain loop, re-check the registry and drain whatever arrived, so a spawn that got past the guard is still torn down. Neither alone is complete: a guard still leaves a race for a spawn already past it, and a sweep alone hands the caller a session that is then immediately destroyed. **Decide it yourself and justify it.** If you find one is enough, or a third option is better, say so and do that — a tested, reported deviation is a good outcome here. ## Rules - Prove it with a test that fails without the fix: register a session after the drain snapshot is taken and assert it is still torn down (or that the spawn was refused), whichever your design says. - Mutation proof required: revert the fix, quote the real failure output, restore it. - Do not run `git stash` — the stash is shared across worktrees and you would take another worker's in-progress work. - 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. A "green" run of mine earlier today had not compiled, for exactly that reason. ## Shape check When done, look in `SessionManager.java` only for the same shape: **a decision made from a `roster()` snapshot and then acted on without re-checking.** One line each, do **not** fix any of it. I already know of one and am handling it separately, so finding it is a good sign rather than a duplicate.
ltms commented 2026-09-04 08:30:52 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 8beae50.

What I checked myself

The two mechanisms are independently covered. The worker's own mutation reverted all of SessionManager.java, which proves the pair works but not that each half is pinned. I ran two narrower mutations instead:

A — keep the draining flag, remove only the sweep (List<MemberSession> stragglers = roster() → List.of()):

[ERROR] SessionManagerTest.drainAllSweepsAStragglerThatRegisteredAfterTheInitialSnapshot:930
        the post-loop sweep must drain the straggler too, not just the initial snapshot
        ==> expected: <true> but was: <false>

B — keep the sweep, neuter only the guard (if (draining.get()) → if (false)):

[ERROR] SessionManagerTest.acquireRefusesANewSpawnOnceDrainAllHasStarted:865
        Expected dev.ltms.fleet.session.ShuttingDownException to be thrown, but nothing was thrown.

One test each, no overlap. Neither half is dead weight.

draining is never reset — I checked that is safe. grep -rn "drainAll" src/ outside the test file returns only its own declaration and close(Integer). close(...) has exactly one caller, Fleetd.java:690, inside the shutdown hook. So the flag can only be set on a process that is dying, and a permanent latch is the right shape. If a future fleet_drain tool ever calls drainAll on a live daemon, this flag becomes a permanent spawn outage — worth remembering.

The RaceLauncher test double is the good part. It blocks the second spawn() and the first stop() on latches, so the interleaving (guard passes → flag flips → snapshot taken → spawn completes and registers) is forced, not a timing bet.

On the out-of-scope change

The worker flagged the ShuttingDownException catch clauses in FleetMcp.spawn and FleetApp.spawnMember as outside a ticket whose text centred on SessionManager. I accept them as in-scope. A new unchecked exception on the spawn path with no catch is the exact #304 shape I closed yesterday: a bare 500 from Javalin and an unnamed MCP error. Adding the throw without the catches would have shipped a fresh instance of the defect I had just fixed. Flagging it was right; the judgement was right too.

Build

cd fleetd && mvn clean install, unpiped: Tests run: 1321, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Merged to `main` in `8beae50`. ## What I checked myself **The two mechanisms are independently covered.** The worker's own mutation reverted all of `SessionManager.java`, which proves the pair works but not that each half is pinned. I ran two narrower mutations instead: **A — keep the `draining` flag, remove only the sweep** (`List<MemberSession> stragglers = roster()` → `List.of()`): ``` [ERROR] SessionManagerTest.drainAllSweepsAStragglerThatRegisteredAfterTheInitialSnapshot:930 the post-loop sweep must drain the straggler too, not just the initial snapshot ==> expected: <true> but was: <false> ``` **B — keep the sweep, neuter only the guard** (`if (draining.get())` → `if (false)`): ``` [ERROR] SessionManagerTest.acquireRefusesANewSpawnOnceDrainAllHasStarted:865 Expected dev.ltms.fleet.session.ShuttingDownException to be thrown, but nothing was thrown. ``` One test each, no overlap. Neither half is dead weight. **`draining` is never reset — I checked that is safe.** `grep -rn "drainAll" src/` outside the test file returns only its own declaration and `close(Integer)`. `close(...)` has exactly one caller, `Fleetd.java:690`, inside the shutdown hook. So the flag can only be set on a process that is dying, and a permanent latch is the right shape. If a future `fleet_drain` tool ever calls `drainAll` on a live daemon, this flag becomes a permanent spawn outage — worth remembering. **The `RaceLauncher` test double is the good part.** It blocks the second `spawn()` and the first `stop()` on latches, so the interleaving (guard passes → flag flips → snapshot taken → spawn completes and registers) is forced, not a timing bet. ## On the out-of-scope change The worker flagged the `ShuttingDownException` catch clauses in `FleetMcp.spawn` and `FleetApp.spawnMember` as outside a ticket whose text centred on `SessionManager`. **I accept them as in-scope.** A new unchecked exception on the spawn path with no catch is the exact #304 shape I closed yesterday: a bare 500 from Javalin and an unnamed MCP error. Adding the throw without the catches would have shipped a fresh instance of the defect I had just fixed. Flagging it was right; the judgement was right too. ## Build `cd fleetd && mvn clean install`, unpiped: `Tests run: 1321, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`.
ltms closed this issue 2026-09-04 08:30:57 +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#308
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?