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

#293: wrap the bare tab.close in HerdrPeerLauncher.stop() #295

Closed
agent wants to merge 0 commits from worker/fix-293-bare-closetab-eb22b5-3 into main
pull from: worker/fix-293-bare-closetab-eb22b5-3
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 06:42:37 +02:00
Member
Copy Link
Copy Source

Fixes #293: a failing tab.close during teardown leaked both the ZDOTDIR and the worktree.

Decision

HerdrPeerLauncher.stop() ran three teardown steps: agents.close (guarded — tolerates *_not_found), spaces.closeTab (bare), releaseZdotdir (guarded). By the time closeTab runs, the pane is already closed, so a failing closeTab is cosmetic workspace tidying, not a real teardown failure. Left bare it masked two real cleanups below it: releaseZdotdir (a ZDOTDIR leak that self-heals slowly via reapOrphans/deleteOnExit) and, worse, SessionManager.release()'s worktree removal — which does not self-heal, because registry.remove(paneId) has already run by the time the throw happens, so a second stop on that pane is a no-op with no retry path.

I checked every caller named in the ticket:

  • SessionManager.release() (:335) calls launcher.stop(paneId) bare — this is the throw site the ticket is about; fixing stop() fixes it here.
  • FleetMcp.stop catches only HerdrException and converts it to an error-text tool result — before the fix, a closeTab failure would have surfaced this way (the session was already deregistered by then, so the response was misleading: "error" for a teardown that mostly succeeded). After the fix this path no longer sees an exception at all.
  • FleetApp.stopMember catches nothing — before the fix a closeTab failure meant an unhandled exception (Javalin's default 500) instead of the intended 204. After the fix, sessions.release no longer throws for this reason, so stopMember returns 204 as intended.
  • drainAll and reapIdle already wrap each session's release call in a per-session try/catch (CB-581), so neither aborts its pass either way — but before the fix, the ZDOTDIR release and worktree removal for the failing session were skipped; after the fix they run.
  • The spawn-readiness gate's self-reap path (waitUntilInjectableOrThrow, failFastOnGoneBackend) calls stop(paneId) bare before throwing its own PeerUnreachableException. Before the fix, a closeTab failure there would have thrown the wrong exception (a raw HerdrException instead of the intended PeerUnreachableException naming the real cause — the pane not becoming injectable). After the fix, stop() no longer throws for this reason, so the gate's own exception is what actually surfaces.

No caller needs a closeTab failure to surface as a failed stop — every one either already tolerates stop() throwing for other reasons (masking, not depending on it) or is actively made more correct by stop() no longer throwing here. So the fix is: wrap closeTab in a try/catch, log a WARN naming the tab id and saying it may need manual cleanup, and continue. isAlreadyGone is untouched — I did not widen it; this continues past any closeTab failure (not just *_not_found), because by this point the failure is cosmetic regardless of its cause, matching the existing "must not mask a real teardown failure above" comment on releaseZdotdir.

What changed

  • fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java: wrapped spaces.closeTab(loc.tabId()) in stop() in a try/catch that logs a WARN naming the tab id ("...may need manual cleanup"), and updated the surrounding javadoc to describe the new per-step failure semantics (only agents.close can still fail the whole teardown; closeTab cannot).
  • fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java: added tabCloseFailsWith(code) (every tab) and tabCloseFailsForTab(tabId, code) (one tab only) — the tab.close counterparts to #290's paneCloseFailsWith/paneCloseFailsForPane. Named "ForTab" rather than "ForPane" (the ticket's suggested name) since tab.close keys on tab_id, not a pane id — noted here since it's a deliberate deviation from the ticket's exact wording.
  • fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java: new test stopStillReleasesZdotdirWhenCloseTabFailsWithANonNotFoundCode — spawns under memberCredentials policy=allow-list + zsh (so a real ZDOTDIR is generated), fails tab.close with internal_error, asserts stop() does not throw, the generated ZDOTDIR is deleted (proving releaseZdotdir ran — its last line is EnvAllowListScrub.deleteRecursively), and a WARN naming the tab id is logged.
  • fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java: new test releaseStillRemovesTheWorktreeWhenCloseTabFails — acquires a session, fails tab.close with internal_error, asserts release() does not throw and still removes the worktree (RecordingWorktrees.removeCalls()), and the session is still deregistered.

SessionManagerTest#reapIdleSurvivesOneSessionWhoseLauncherStopFails (from #290) is left unchanged and still passes as-is. That test uses pane.close (not tab.close) failure injection, so it still exercises agents.close's bare-propagation path through stop() -> release() -> reapIdle's own per-session catch — a different trigger than this ticket's, unaffected by this fix. Its assertion ("the middle session's worktree removal never runs") is still correct: this ticket didn't touch the agents.close guard.

Mutation proof

Reverted the try/catch around spaces.closeTab back to bare, ran the two new tests — both failed with the pre-fix exception propagating exactly as described:

ClaudeCodeLauncherTest#stopStillReleasesZdotdirWhenCloseTabFailsWithANonNotFoundCode:

org.opentest4j.AssertionFailedError: fleetd #293: a failing tab.close is cosmetic — it must not propagate out of stop() ==> Unexpected exception thrown: dev.ltms.fleet.herdr.HerdrException: herdr error [internal_error]: tab.close failed
	at dev.ltms.fleet.member.HerdrPeerLauncher.stop(HerdrPeerLauncher.java:942)
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

SessionManagerTest#releaseStillRemovesTheWorktreeWhenCloseTabFails:

org.opentest4j.AssertionFailedError: a failing tab.close is cosmetic (the pane is already closed by then) — it must not propagate out of release() ==> Unexpected exception thrown: dev.ltms.fleet.herdr.HerdrException: herdr error [internal_error]: tab.close failed
	at dev.ltms.fleet.member.HerdrPeerLauncher.stop(HerdrPeerLauncher.java:942)
	at dev.ltms.fleet.session.SessionManager.release(SessionManager.java:335)
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

Restored the fix; both pass again, and the full suite is green (see below).

Build

cd fleetd && mvn clean install, unpiped, full output read:

Tests run: 1299, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Shape check (report only, not fixed)

A fork is auditing HerdrPeerLauncher and WorkspaceControl for the same "guard on one branch, bare on its sibling" shape as #274/#283/#293. Findings will be added as a follow-up comment on this PR rather than fixed here, per the ticket's instructions.

Fixes #293: a failing `tab.close` during teardown leaked both the ZDOTDIR and the worktree. ## Decision `HerdrPeerLauncher.stop()` ran three teardown steps: `agents.close` (guarded — tolerates `*_not_found`), `spaces.closeTab` (bare), `releaseZdotdir` (guarded). By the time `closeTab` runs, the pane is already closed, so a failing `closeTab` is cosmetic workspace tidying, not a real teardown failure. Left bare it masked two real cleanups below it: `releaseZdotdir` (a ZDOTDIR leak that self-heals slowly via `reapOrphans`/`deleteOnExit`) and, worse, `SessionManager.release()`'s worktree removal — which does **not** self-heal, because `registry.remove(paneId)` has already run by the time the throw happens, so a second `stop` on that pane is a no-op with no retry path. I checked every caller named in the ticket: - `SessionManager.release()` (`:335`) calls `launcher.stop(paneId)` bare — this is the throw site the ticket is about; fixing `stop()` fixes it here. - `FleetMcp.stop` catches only `HerdrException` and converts it to an error-text tool result — before the fix, a `closeTab` failure would have surfaced this way (the session was already deregistered by then, so the response was misleading: "error" for a teardown that mostly succeeded). After the fix this path no longer sees an exception at all. - `FleetApp.stopMember` catches nothing — before the fix a `closeTab` failure meant an unhandled exception (Javalin's default 500) instead of the intended `204`. After the fix, `sessions.release` no longer throws for this reason, so `stopMember` returns `204` as intended. - `drainAll` and `reapIdle` already wrap each session's `release` call in a per-session try/catch (CB-581), so neither aborts its pass either way — but before the fix, the ZDOTDIR release and worktree removal for the failing session were skipped; after the fix they run. - The spawn-readiness gate's self-reap path (`waitUntilInjectableOrThrow`, `failFastOnGoneBackend`) calls `stop(paneId)` bare before throwing its own `PeerUnreachableException`. Before the fix, a `closeTab` failure there would have thrown the wrong exception (a raw `HerdrException` instead of the intended `PeerUnreachableException` naming the real cause — the pane not becoming injectable). After the fix, `stop()` no longer throws for this reason, so the gate's own exception is what actually surfaces. No caller needs a `closeTab` failure to surface as a failed `stop` — every one either already tolerates `stop()` throwing for other reasons (masking, not depending on it) or is actively made more correct by `stop()` no longer throwing here. So the fix is: wrap `closeTab` in a try/catch, log a WARN naming the tab id and saying it may need manual cleanup, and continue. `isAlreadyGone` is untouched — I did not widen it; this continues past **any** `closeTab` failure (not just `*_not_found`), because by this point the failure is cosmetic regardless of its cause, matching the existing "must not mask a real teardown failure above" comment on `releaseZdotdir`. ## What changed - `fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java`: wrapped `spaces.closeTab(loc.tabId())` in `stop()` in a try/catch that logs a WARN naming the tab id ("...may need manual cleanup"), and updated the surrounding javadoc to describe the new per-step failure semantics (only `agents.close` can still fail the whole teardown; `closeTab` cannot). - `fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java`: added `tabCloseFailsWith(code)` (every tab) and `tabCloseFailsForTab(tabId, code)` (one tab only) — the `tab.close` counterparts to #290's `paneCloseFailsWith`/`paneCloseFailsForPane`. Named "ForTab" rather than "ForPane" (the ticket's suggested name) since `tab.close` keys on `tab_id`, not a pane id — noted here since it's a deliberate deviation from the ticket's exact wording. - `fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java`: new test `stopStillReleasesZdotdirWhenCloseTabFailsWithANonNotFoundCode` — spawns under `memberCredentials policy=allow-list` + zsh (so a real ZDOTDIR is generated), fails `tab.close` with `internal_error`, asserts `stop()` does not throw, the generated ZDOTDIR is deleted (proving `releaseZdotdir` ran — its last line is `EnvAllowListScrub.deleteRecursively`), and a WARN naming the tab id is logged. - `fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java`: new test `releaseStillRemovesTheWorktreeWhenCloseTabFails` — acquires a session, fails `tab.close` with `internal_error`, asserts `release()` does not throw and still removes the worktree (`RecordingWorktrees.removeCalls()`), and the session is still deregistered. **`SessionManagerTest#reapIdleSurvivesOneSessionWhoseLauncherStopFails` (from #290) is left unchanged and still passes as-is.** That test uses `pane.close` (not `tab.close`) failure injection, so it still exercises `agents.close`'s bare-propagation path through `stop()` -> `release()` -> `reapIdle`'s own per-session catch — a different trigger than this ticket's, unaffected by this fix. Its assertion ("the middle session's worktree removal never runs") is still correct: this ticket didn't touch the `agents.close` guard. ## Mutation proof Reverted the try/catch around `spaces.closeTab` back to bare, ran the two new tests — both failed with the pre-fix exception propagating exactly as described: `ClaudeCodeLauncherTest#stopStillReleasesZdotdirWhenCloseTabFailsWithANonNotFoundCode`: ``` org.opentest4j.AssertionFailedError: fleetd #293: a failing tab.close is cosmetic — it must not propagate out of stop() ==> Unexpected exception thrown: dev.ltms.fleet.herdr.HerdrException: herdr error [internal_error]: tab.close failed at dev.ltms.fleet.member.HerdrPeerLauncher.stop(HerdrPeerLauncher.java:942) Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 ``` `SessionManagerTest#releaseStillRemovesTheWorktreeWhenCloseTabFails`: ``` org.opentest4j.AssertionFailedError: a failing tab.close is cosmetic (the pane is already closed by then) — it must not propagate out of release() ==> Unexpected exception thrown: dev.ltms.fleet.herdr.HerdrException: herdr error [internal_error]: tab.close failed at dev.ltms.fleet.member.HerdrPeerLauncher.stop(HerdrPeerLauncher.java:942) at dev.ltms.fleet.session.SessionManager.release(SessionManager.java:335) Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 ``` Restored the fix; both pass again, and the full suite is green (see below). ## Build `cd fleetd && mvn clean install`, unpiped, full output read: ``` Tests run: 1299, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` ## Shape check (report only, not fixed) A fork is auditing `HerdrPeerLauncher` and `WorkspaceControl` for the same "guard on one branch, bare on its sibling" shape as #274/#283/#293. Findings will be added as a follow-up comment on this PR rather than fixed here, per the ticket's instructions.
agent added 1 commit 2026-09-04 06:42:38 +02:00
#293: wrap the bare tab.close in HerdrPeerLauncher.stop()
CI / contract (pull_request) Successful in 1m41s
Details
CI / build (pull_request) Successful in 1m59s
Details
0c10079755 
The pane is already closed by the time spaces.closeTab runs, so a failing
tab.close is cosmetic workspace tidying, not a real teardown failure. Left
bare, it propagated out of stop() and masked releaseZdotdir (ZDOTDIR leak)
and, worse, SessionManager.release()'s worktree removal (no self-heal,
no retry — the registry entry is already gone by then).

Wrap it in a try/catch that logs a WARN naming the tab id, matching the
"must not mask a real teardown failure above" comment already on
releaseZdotdir. isAlreadyGone is untouched — this continues past *any*
tab.close failure, not just *_not_found, since the failure is cosmetic
regardless of its cause.

Adds FakeHerdr#tabCloseFailsWith/tabCloseFailsForTab (the tab.close
counterpart to #290's paneCloseFailsForPane) plus two tests: one proving
releaseZdotdir still runs (the generated ZDOTDIR is deleted) and one
proving SessionManager.release() still removes the worktree, both with a
non-not_found tab.close failure.
ltms closed this pull request 2026-09-04 06:50:47 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m41s
Details
CI / build (pull_request) Successful in 1m59s
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#295
Delete Branch "worker/fix-293-bare-closetab-eb22b5-3"

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?