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

#308: refuse a spawn once the shutdown drain has started, and sweep stragglers #313

Closed
agent wants to merge 0 commits from worker/fix-308-b4f664-7 into main
pull from: worker/fix-308-b4f664-7
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 08:22:26 +02:00
Member
Copy Link
Copy Source

Fixes fleetd #308.

What I found on the forge before deciding

Grepped SessionManager.java for shuttingDown, draining, closed — no
existing guard, matching what the issue said. Read drainAll (one-shot
roster() snapshot, no re-check after the loop) and the shutdown hook in
Fleetd.java (sessions.close() runs first, mcp.close() runs 8 calls
later) — both matched the issue's description exactly. I found nothing in
the issue that was wrong against the current code.

Fix — both candidate mechanisms, as the issue suggested

Neither alone is complete, so both:

  1. Guard. SessionManager.acquire (the one overload every other
    acquire(...) delegates to) now checks a draining AtomicBoolean,
    flipped true as the very first statement in drainAll — before the
    registry snapshot is even taken — and throws the new
    ShuttingDownException when set (invariant 3: fail loudly, say why).
    FleetMcp.spawn and FleetApp.spawnMember (the REST path) both got a
    catch for it so it surfaces as a clean tool error / HTTP 503 rather
    than an uncaught RuntimeException.
  2. Sweep. The guard alone still races: a caller that read draining == false just before the flip can still be mid-launcher.spawn() (a real
    herdr round trip — not instantaneous) when drainAll takes its
    snapshot. drainAll now re-reads the registry once its main pass
    finishes and drains whatever straggler landed there, bounded by the
    same deadline computed once at the top — never a second budget.

Invariant 1 (whole-drain budget, not a per-session grace) is preserved:
the sweep shares the original deadline; if it's already passed (the
normal case, since the sweep only matters after a real drain), a straggler
still BUSY is released immediately with no extra wait, exactly like a
BUSY session reached late in the original loop.

Invariant 2 (ReleaseCause.SHUTDOWN preserves worktrees) is untouched —
both the main pass and the sweep go through the same release(..., ReleaseCause.SHUTDOWN) call via a shared drainSnapshot helper extracted
from the old drainAll body.

Tests (mutation-proven)

Two new tests in SessionManagerTest:

  • acquireRefusesANewSpawnOnceDrainAllHasStarted — calls drainAll on an
    empty roster, then asserts acquire() throws ShuttingDownException
    with a non-blank message.
  • drainAllSweepsAStragglerThatRegisteredAfterTheInitialSnapshot — a
    RaceLauncher test double blocks the second spawn() call and the
    first stop() call so the exact TOCTOU interleaving (guard check
    passes → drainAll flips the flag and snapshots → spawn completes and
    registers) is forced deterministically with real threads + latches,
    not a timing bet. Asserts the roster ends empty and both panes (the
    original session and the straggler) were actually stopped.

Reverted SessionManager.java only (kept the new test file), reran
SessionManagerTest: both new tests failed for real —
acquireRefusesANewSpawnOnceDrainAllHasStarted with "Expected
ShuttingDownException to be thrown, but nothing was thrown", and the sweep
test with "expected: but was: " on the empty-roster
assertion. Restored the fix with git apply on the saved diff; reran —
green.

Build

cd fleetd && mvn clean install (unpiped): Tests run: 1318, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. SessionManagerTest alone:
Tests run: 58, Failures: 0, Errors: 0, Skipped: 0.

Shape check (SessionManager.java only — not fixed)

reapIdle (loops roster(), then calls release(s.paneId()) per entry
with no re-check of the session's current state before acting) has the
same shape as the bug this ticket fixes. Reporting only, per the issue's
instruction — the issue author says they're handling this one separately.

Caveat for review

FleetMcp.spawn / FleetApp.spawnMember catch clauses are new surface
added beyond SessionManager itself — in scope because invariant 3
("fail loudly and say why") is a caller-facing requirement, and an
uncaught RuntimeException from acquire() would otherwise reach the
caller as a bare 500 / unnamed MCP error instead of a clean, named refusal.

Fixes fleetd #308. ## What I found on the forge before deciding Grepped `SessionManager.java` for `shuttingDown`, `draining`, `closed` — no existing guard, matching what the issue said. Read `drainAll` (one-shot `roster()` snapshot, no re-check after the loop) and the shutdown hook in `Fleetd.java` (`sessions.close()` runs first, `mcp.close()` runs 8 calls later) — both matched the issue's description exactly. I found nothing in the issue that was wrong against the current code. ## Fix — both candidate mechanisms, as the issue suggested Neither alone is complete, so both: 1. **Guard.** `SessionManager.acquire` (the one overload every other `acquire(...)` delegates to) now checks a `draining` `AtomicBoolean`, flipped `true` as the very first statement in `drainAll` — before the registry snapshot is even taken — and throws the new `ShuttingDownException` when set (invariant 3: fail loudly, say why). `FleetMcp.spawn` and `FleetApp.spawnMember` (the REST path) both got a `catch` for it so it surfaces as a clean tool error / HTTP 503 rather than an uncaught `RuntimeException`. 2. **Sweep.** The guard alone still races: a caller that read `draining == false` just before the flip can still be mid-`launcher.spawn()` (a real herdr round trip — not instantaneous) when `drainAll` takes its snapshot. `drainAll` now re-reads the registry once its main pass finishes and drains whatever straggler landed there, bounded by the **same** `deadline` computed once at the top — never a second budget. Invariant 1 (whole-drain budget, not a per-session grace) is preserved: the sweep shares the original `deadline`; if it's already passed (the normal case, since the sweep only matters after a real drain), a straggler still `BUSY` is released immediately with no extra wait, exactly like a `BUSY` session reached late in the original loop. Invariant 2 (`ReleaseCause.SHUTDOWN` preserves worktrees) is untouched — both the main pass and the sweep go through the same `release(..., ReleaseCause.SHUTDOWN)` call via a shared `drainSnapshot` helper extracted from the old `drainAll` body. ## Tests (mutation-proven) Two new tests in `SessionManagerTest`: - `acquireRefusesANewSpawnOnceDrainAllHasStarted` — calls `drainAll` on an empty roster, then asserts `acquire()` throws `ShuttingDownException` with a non-blank message. - `drainAllSweepsAStragglerThatRegisteredAfterTheInitialSnapshot` — a `RaceLauncher` test double blocks the *second* `spawn()` call and the *first* `stop()` call so the exact TOCTOU interleaving (guard check passes → `drainAll` flips the flag and snapshots → spawn completes and registers) is forced deterministically with real threads + latches, not a timing bet. Asserts the roster ends empty and both panes (the original session and the straggler) were actually stopped. Reverted `SessionManager.java` only (kept the new test file), reran `SessionManagerTest`: both new tests failed for real — `acquireRefusesANewSpawnOnceDrainAllHasStarted` with "Expected ShuttingDownException to be thrown, but nothing was thrown", and the sweep test with "expected: <true> but was: <false>" on the empty-roster assertion. Restored the fix with `git apply` on the saved diff; reran — green. ## Build `cd fleetd && mvn clean install` (unpiped): `Tests run: 1318, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. `SessionManagerTest` alone: `Tests run: 58, Failures: 0, Errors: 0, Skipped: 0`. ## Shape check (SessionManager.java only — not fixed) `reapIdle` (loops `roster()`, then calls `release(s.paneId())` per entry with no re-check of the session's current state before acting) has the same shape as the bug this ticket fixes. Reporting only, per the issue's instruction — the issue author says they're handling this one separately. ## Caveat for review `FleetMcp.spawn` / `FleetApp.spawnMember` catch clauses are new surface added beyond `SessionManager` itself — in scope because invariant 3 ("fail loudly and say why") is a caller-facing requirement, and an uncaught `RuntimeException` from `acquire()` would otherwise reach the caller as a bare 500 / unnamed MCP error instead of a clean, named refusal.
agent added 1 commit 2026-09-04 08:22:27 +02:00
#308: refuse a spawn once the shutdown drain has started, and sweep stragglers
CI / contract (pull_request) Successful in 1m28s
Details
CI / build (pull_request) Successful in 1m36s
Details
83f2aea60f 
drainAll iterated a one-shot registry snapshot with nothing to refuse a new
fleet_spawn while the drain was still running (mcp.close() only runs 8 calls
after sessions.close() in the shutdown hook). A session registered in that
window was never visited by the drain loop: its pane kept running and its
worktree was never preserved, with the in-memory registry gone at exit.

Fix, both mechanisms as the issue asked for (neither alone is complete):

- SessionManager.acquire now checks a `draining` flag, flipped true at the
  very start of drainAll before the registry snapshot is even taken, and
  throws the new ShuttingDownException (invariant 3: fail loudly, say why).
  FleetMcp.spawn and FleetApp.spawnMember surface it as a clean error/503
  rather than an uncaught RuntimeException.
- The flag alone cannot close the whole race: a caller already past the
  check can still be mid-launcher.spawn() (a real herdr round trip) when
  drainAll snapshots the registry. drainAll now re-reads the registry once
  its main pass finishes and drains whatever straggler landed there too,
  bounded by the SAME whole-drain deadline (invariant 1: timeoutNanos stays
  a budget for the whole drain, never extended for a straggler).
- ReleaseCause.SHUTDOWN still preserves worktrees for both the initial pass
  and the sweep (invariant 2, unchanged release() path).

Tests (SessionManagerTest): a guard test proving acquire() throws once
drainAll has started, and a race test using a launcher double that blocks
the second spawn() and the first stop() call to force, deterministically,
the exact interleaving where a spawn passes the guard before drainAll flips
it and only registers after the initial snapshot — proving the post-loop
sweep catches it.

Shape check (SessionManager.java only, not fixed): reapIdle has the same
shape — a decision made from a roster() snapshot, then acted on via
release(s.paneId()) with no re-check of the session's current state.
ltms closed this pull request 2026-09-04 08:31:03 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m28s
Details
CI / build (pull_request) Successful in 1m36s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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#313
Delete Branch "worker/fix-308-b4f664-7"

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?