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

CB-581: a throw inside release() orphans the pane and aborts the reaping pass #57

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

Why

SessionManager.release(paneId, cause) does destructive work in an order that cannot survive an
exception in the middle:

196  MemberSession removed = registry.remove(paneId);   // registry entry already dropped
199  memberLifecycle.released(removed.terminalId());    // already fired
202  ...worktrees.hasUncommitted(...)                   // any throw lands here
208  notifyReleased(removed.terminalId());              // never runs
210  launcher.stop(paneId);                             // never runs

If anything between :196 and :210 throws, the session is gone from the registry while its pane
is still alive, and the CB-516 release notification never fires — so a bridge_send caller blocked
on that member waits on a rendezvous nothing will ever resolve.

The two callers are not protected equally. drainAll (:539-562) wraps each release in
try/catch, so one failure is logged and the sweep continues — CB-544 thought about this.
reapIdle (:508-525) does not: release(s.paneId()) at :520 is bare, and it is the
COMPLETED cause, which is the branch that does the most work. One failure there aborts the whole
pass, and every other idle session survives it.

This is not hypothetical. CB-576 added worktrees.hasUncommitted into exactly that window, and its
first version threw on a missing worktree. That specific throw is fixed in b525b0f, but the
window is still unguarded for the next thing added to it.

Scope

  1. Give reapIdle the same per-session try/catch drainAll already has, so one failing release
    does not abort the pass. Log and continue.
  2. Make the teardown steps that must always happen — notifyReleased and launcher.stop — run
    even when an earlier step throws. A finally around the notify/stop pair is the obvious shape;
    whatever the shape, a blocked caller must always get its CB-516 fast-fail.
  3. Add the SessionManager-level test waived when PR #53 merged: with a worktree that is gone, a
    COMPLETED release still reaches notifyReleased and launcher.stop. FakeWorktrees needs
    the missing-path case modelled honestly, not a fake that always returns false.

Also worth folding in — the false-preserve risk from the CB-576 review

CB-576's hasUncommitted uses plain git status --porcelain, so an untracked, non-gitignored file
in a freshly-provisioned worktree would read as dirty and make every COMPLETED release
preserve the directory. Worktrees would then accumulate forever with no error.

Checked at review time and inert today: tracked overlay files get --skip-worktree so
--porcelain cannot see them, and bridged.yaml is gitignored. A future profile whose parity
overlay copies an untracked file would trip it. Decide here whether to guard it or to document the
constraint on overlayParity; do not add an untested filter on speculation.

Acceptance criteria

  1. A release that throws mid-way still stops the pane and still notifies the blocked caller.
  2. A failing release inside reapIdle is logged and the pass continues to the remaining sessions.
  3. A test covers a COMPLETED release whose worktree is already gone, asserting both notifyReleased
    and launcher.stop ran.
  4. A test covers reapIdle continuing past one failing session.
  5. The overlay false-preserve risk is either guarded with a test or documented as a constraint —
    an explicit decision either way, not silence.

Related

  • Follow-up from PR #53 (CB-576), merged with criterion 3 waived. See the merge commit message.
  • drainAll's existing try/catch at SessionManager.java:542 is the pattern to copy.
## Why `SessionManager.release(paneId, cause)` does destructive work in an order that cannot survive an exception in the middle: ``` 196 MemberSession removed = registry.remove(paneId); // registry entry already dropped 199 memberLifecycle.released(removed.terminalId()); // already fired 202 ...worktrees.hasUncommitted(...) // any throw lands here 208 notifyReleased(removed.terminalId()); // never runs 210 launcher.stop(paneId); // never runs ``` If anything between `:196` and `:210` throws, the session is gone from the registry while its pane is still alive, and the CB-516 release notification never fires — so a `bridge_send` caller blocked on that member waits on a rendezvous nothing will ever resolve. The two callers are not protected equally. `drainAll` (`:539-562`) wraps each `release` in try/catch, so one failure is logged and the sweep continues — CB-544 thought about this. `reapIdle` (`:508-525`) does not: `release(s.paneId())` at `:520` is bare, and it is the `COMPLETED` cause, which is the branch that does the most work. One failure there aborts the whole pass, and every other idle session survives it. This is not hypothetical. CB-576 added `worktrees.hasUncommitted` into exactly that window, and its first version threw on a missing worktree. That specific throw is fixed in `b525b0f`, but the window is still unguarded for the next thing added to it. ## Scope 1. Give `reapIdle` the same per-session try/catch `drainAll` already has, so one failing release does not abort the pass. Log and continue. 2. Make the teardown steps that must always happen — `notifyReleased` and `launcher.stop` — run even when an earlier step throws. A `finally` around the notify/stop pair is the obvious shape; whatever the shape, a blocked caller must always get its CB-516 fast-fail. 3. Add the `SessionManager`-level test waived when PR #53 merged: with a worktree that is gone, a `COMPLETED` release still reaches `notifyReleased` and `launcher.stop`. `FakeWorktrees` needs the missing-path case modelled honestly, not a fake that always returns `false`. ## Also worth folding in — the false-preserve risk from the CB-576 review CB-576's `hasUncommitted` uses plain `git status --porcelain`, so an untracked, non-gitignored file in a freshly-provisioned worktree would read as dirty and make **every** `COMPLETED` release preserve the directory. Worktrees would then accumulate forever with no error. Checked at review time and inert today: tracked overlay files get `--skip-worktree` so `--porcelain` cannot see them, and `bridged.yaml` is gitignored. A future profile whose parity overlay copies an untracked file would trip it. Decide here whether to guard it or to document the constraint on `overlayParity`; do not add an untested filter on speculation. ## Acceptance criteria 1. A `release` that throws mid-way still stops the pane and still notifies the blocked caller. 2. A failing release inside `reapIdle` is logged and the pass continues to the remaining sessions. 3. A test covers a `COMPLETED` release whose worktree is already gone, asserting both `notifyReleased` and `launcher.stop` ran. 4. A test covers `reapIdle` continuing past one failing session. 5. The overlay false-preserve risk is either guarded with a test or documented as a constraint — an explicit decision either way, not silence. ## Related - Follow-up from PR #53 (CB-576), merged with criterion 3 waived. See the merge commit message. - `drainAll`'s existing try/catch at `SessionManager.java:542` is the pattern to copy.
ltms commented 2026-08-15 10:15:39 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged to main as 180de84 (PR #59).

Lead-verified: own unpiped build of the branch merged onto main — 717 tests, BUILD SUCCESS, exit 0.

What landed:

  • The dirty-check block in release() is guarded, and a throw now preserves the worktree. "We could not tell whether it holds work" must not be treated as "it is clean" — deleting on a guess destroys work with no other copy, which is the exact loss that produced CB-576.
  • notifyReleased runs in a finally, and launcher.stop(paneId) runs unconditionally. A throw can no longer leave a live pane burning a fleet slot while absent from the roster.
  • reapIdle() catches per session, matching the shape drainAll already used, so one bad session cannot skip every session after it in the pass.

Checked during review, not taken on trust: notifyReleased cannot throw out of the finally — it already wraps each listener in its own try/catch and only logs. So the unconditional pane stop is genuinely unconditional, rather than moving the same bug behind a different door.

Known remaining edge, deliberately not fixed here: worktrees.remove(...) at the tail of release() is still unguarded. It now runs after launcher.stop, so a throw there cannot orphan a pane, and both loop callers (reapIdle, drainAll) catch it. A direct release() caller would still see it propagate.

This also carried CB-576's waived acceptance criterion 3 (the SessionManager teardown test), which is now covered.

Fixed and merged to `main` as **180de84** (PR #59). Lead-verified: own unpiped build of the branch merged onto `main` — **717 tests, BUILD SUCCESS, exit 0**. What landed: - The dirty-check block in `release()` is guarded, and a throw now **preserves** the worktree. "We could not tell whether it holds work" must not be treated as "it is clean" — deleting on a guess destroys work with no other copy, which is the exact loss that produced CB-576. - `notifyReleased` runs in a `finally`, and `launcher.stop(paneId)` runs unconditionally. A throw can no longer leave a live pane burning a fleet slot while absent from the roster. - `reapIdle()` catches per session, matching the shape `drainAll` already used, so one bad session cannot skip every session after it in the pass. Checked during review, not taken on trust: `notifyReleased` cannot throw out of the `finally` — it already wraps each listener in its own try/catch and only logs. So the unconditional pane stop is genuinely unconditional, rather than moving the same bug behind a different door. Known remaining edge, deliberately not fixed here: `worktrees.remove(...)` at the tail of `release()` is still unguarded. It now runs *after* `launcher.stop`, so a throw there cannot orphan a pane, and both loop callers (`reapIdle`, `drainAll`) catch it. A direct `release()` caller would still see it propagate. This also carried CB-576's waived acceptance criterion 3 (the `SessionManager` teardown test), which is now covered.
ltms closed this issue 2026-08-15 10:15:51 +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#57
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?