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

#283: fix two teardown-cleanup leaks in SessionManager #288

Closed
agent wants to merge 0 commits from worker/fix-283-teardown-leaks-f40dfa-9 into main
pull from: worker/fix-283-teardown-leaks-f40dfa-9
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 05:51:39 +02:00
Member
Copy Link
Copy Source

Fixes #283 — two teardown-cleanup leaks in SessionManager, same shape as #274 (one path cleaned up, its sibling not).

Defect 1 — release()'s bare worktree remove (SessionManager.java:335-338, pre-fix)

Every other cleanup step in release() is wrapped in try/catch, because exec() can throw on a non-zero exit or its own 30s timeout. The last step — removing the released session's worktree — was the one left bare. By the time it runs, registry.remove, handles.remove, and launcher.stop(paneId) have all already happened, so a throw here escaped release() after the session was already fully torn down: no retry path (a second stop on the same paneId is a no-op), and the caller saw a "failed stop" for a session that was in fact gone, with the worktree directory leaked forever.

Fix: wrapped in try/catch with a WARN, matching the pattern every sibling step in this method already uses.

Defect 2 — spawn-failure catch removes the worktree but forgets the branch (acquireWithWorktree, SessionManager.java:503-513 pre-fix)

This catch covers every failure after worktrees.add() returns — overlayParity, shareWithGroup, launcher.spawn itself — so branch was actually created in git by the time it runs. #274 already fixed the sibling failure inside add() (GitWorktrees.cleanupAfterAddFailure deletes both the worktree and the branch), but this path removed only the worktree and left the branch orphaned. A spawn failure here is routine (a quarantined credential, a backend refusal), so every occurrence leaked a worker/<slug>-<nonce> branch.

Fix: extracted the branch-delete exec("git","branch","-D",...) out of GitWorktrees.cleanupAfterAddFailure into a new Worktrees.deleteBranch(repoRoot, branch) method, and reused it from both cleanupAfterAddFailure (refactor, same behavior) and the acquireWithWorktree catch (the actual fix). Best-effort and log-only in both callers — a cleanup failure never masks the original exception.

Keeping the two paths apart

A normal release deliberately never deletes a branch — only the failed-provisioning path does, per the ticket's explicit warning. releaseRemovesWorktreeButDoesNotDeleteBranch now also asserts worktrees.deleteBranchCalls() is empty after a normal release. unchangedRegressionCleanCompletedReleaseStillRemovesTheWorktree / unchangedRegressionDirtyCompletedReleaseStillPreservesTheWorktree / unchangedRegressionShutdownDrainStillPreservesTheWorktree (pre-existing, untouched) continue to pin release()'s worktree-removal behavior. spawnFailureAfterAddDeletesTheOrphanedBranch proves the only path that calls deleteBranch is the failed-spawn catch, and asserts the branch deleted is the exact one add() created.

Tests added (WorktreeSessionManagerTest.java)

  • releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows (defect 1): FakeWorktrees.failRemove(...) makes worktree removal throw; asserts release() completes without throwing, the pane is stopped, and the registry is clean.
  • spawnFailureAfterAddDeletesTheOrphanedBranch (defect 2): FakeWorktrees.failOverlay(...) makes a post-add() step throw; asserts the branch add() created is deleted (and the worktree is still removed as before).

Revert-and-verify (both defects), quoted

Defect 1 — reverted release()'s try/catch back to the bare call, ran only the new test:

[ERROR] dev.ltms.fleet.session.WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows -- Time elapsed: 0.199 s <<< ERROR!
dev.ltms.fleet.session.WorktreeException: stale index lock
	at dev.ltms.fleet.session.FakeWorktrees.failRemove(FakeWorktrees.java:111)
	at dev.ltms.fleet.session.WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows(WorktreeSessionManagerTest.java:235)
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0

Restored, then reverted defect 2's deleteBranch call in the acquireWithWorktree catch, ran only that test:

[ERROR] WorktreeSessionManagerTest.spawnFailureAfterAddDeletesTheOrphanedBranch:414 the orphaned branch that add() actually created must also be deleted ==> expected: <1> but was: <0>
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

Restored both fixes and re-ran the full suite green (below).

Full build (unpiped mvn clean install in fleetd/)

[INFO] Tests run: 1285, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Note on an existing test whose premise the defect-1 fix changed

SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease (CB-581) used a worktree-removal failure to prove reapIdle's own per-session try/catch survives one session's release() throwing. Now that release() itself catches a worktree-removal failure (this ticket) and only logs a WARN, release() no longer throws for that specific reason, so the test's own scenario no longer triggers a throw. Renamed it to reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails and updated its assertions (reaped == 3 instead of 2, the WARN is still asserted). reapIdle's own guard against a release() failure it genuinely cannot swallow (e.g. launcher.stop throwing) is untouched by this ticket, but this specific test no longer exercises it — I did not add new coverage for that, since building the per-pane failure injection it would need (the fake herdr's pane.close failure is global, not per-pane) is outside this ticket's scope.

Reused helper

GitWorktrees already had the branch-delete logic from #274, inlined in the private cleanupAfterAddFailure. I extracted it into a new Worktrees.deleteBranch(repoRoot, branch) interface method (implemented by GitWorktrees, and by the test doubles FakeWorktrees/RecordingWorktrees), and both cleanupAfterAddFailure and SessionManager.acquireWithWorktree's catch now call it — no second copy of the git command.

Out of scope (spotted, not fixed, per the ticket's instruction)

None — the ticket's own auditor note says the "guard assigned only at the end" shape does not recur elsewhere in this package, and I did not find any other one-sided-cleanup shape while working this file.

Fixes #283 — two teardown-cleanup leaks in `SessionManager`, same shape as #274 (one path cleaned up, its sibling not). ## Defect 1 — `release()`'s bare worktree remove (SessionManager.java:335-338, pre-fix) Every other cleanup step in `release()` is wrapped in try/catch, because `exec()` can throw on a non-zero exit or its own 30s timeout. The last step — removing the released session's worktree — was the one left bare. By the time it runs, `registry.remove`, `handles.remove`, and `launcher.stop(paneId)` have all already happened, so a throw here escaped `release()` after the session was already fully torn down: no retry path (a second stop on the same `paneId` is a no-op), and the caller saw a "failed stop" for a session that was in fact gone, with the worktree directory leaked forever. **Fix:** wrapped in try/catch with a WARN, matching the pattern every sibling step in this method already uses. ## Defect 2 — spawn-failure catch removes the worktree but forgets the branch (`acquireWithWorktree`, SessionManager.java:503-513 pre-fix) This catch covers every failure *after* `worktrees.add()` returns — `overlayParity`, `shareWithGroup`, `launcher.spawn` itself — so `branch` was actually created in git by the time it runs. `#274` already fixed the sibling failure *inside* `add()` (`GitWorktrees.cleanupAfterAddFailure` deletes both the worktree and the branch), but this path removed only the worktree and left the branch orphaned. A spawn failure here is routine (a quarantined credential, a backend refusal), so every occurrence leaked a `worker/<slug>-<nonce>` branch. **Fix:** extracted the branch-delete `exec("git","branch","-D",...)` out of `GitWorktrees.cleanupAfterAddFailure` into a new `Worktrees.deleteBranch(repoRoot, branch)` method, and reused it from both `cleanupAfterAddFailure` (refactor, same behavior) and the `acquireWithWorktree` catch (the actual fix). Best-effort and log-only in both callers — a cleanup failure never masks the original exception. ## Keeping the two paths apart A **normal** release deliberately never deletes a branch — only the **failed-provisioning** path does, per the ticket's explicit warning. `releaseRemovesWorktreeButDoesNotDeleteBranch` now also asserts `worktrees.deleteBranchCalls()` is empty after a normal release. `unchangedRegressionCleanCompletedReleaseStillRemovesTheWorktree` / `unchangedRegressionDirtyCompletedReleaseStillPreservesTheWorktree` / `unchangedRegressionShutdownDrainStillPreservesTheWorktree` (pre-existing, untouched) continue to pin `release()`'s worktree-removal behavior. `spawnFailureAfterAddDeletesTheOrphanedBranch` proves the *only* path that calls `deleteBranch` is the failed-spawn catch, and asserts the branch deleted is the exact one `add()` created. ## Tests added (WorktreeSessionManagerTest.java) - `releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows` (defect 1): `FakeWorktrees.failRemove(...)` makes worktree removal throw; asserts `release()` completes without throwing, the pane is stopped, and the registry is clean. - `spawnFailureAfterAddDeletesTheOrphanedBranch` (defect 2): `FakeWorktrees.failOverlay(...)` makes a post-`add()` step throw; asserts the branch `add()` created is deleted (and the worktree is still removed as before). ## Revert-and-verify (both defects), quoted **Defect 1** — reverted `release()`'s try/catch back to the bare call, ran only the new test: ``` [ERROR] dev.ltms.fleet.session.WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows -- Time elapsed: 0.199 s <<< ERROR! dev.ltms.fleet.session.WorktreeException: stale index lock at dev.ltms.fleet.session.FakeWorktrees.failRemove(FakeWorktrees.java:111) at dev.ltms.fleet.session.WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows(WorktreeSessionManagerTest.java:235) [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0 ``` Restored, then reverted defect 2's `deleteBranch` call in the `acquireWithWorktree` catch, ran only that test: ``` [ERROR] WorktreeSessionManagerTest.spawnFailureAfterAddDeletesTheOrphanedBranch:414 the orphaned branch that add() actually created must also be deleted ==> expected: <1> but was: <0> [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 ``` Restored both fixes and re-ran the full suite green (below). ## Full build (unpiped `mvn clean install` in `fleetd/`) ``` [INFO] Tests run: 1285, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ## Note on an existing test whose premise the defect-1 fix changed `SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease` (CB-581) used a worktree-removal failure to prove `reapIdle`'s own per-session try/catch survives one session's `release()` throwing. Now that `release()` itself catches a worktree-removal failure (this ticket) and only logs a WARN, `release()` no longer throws for that specific reason, so the test's own scenario no longer triggers a throw. Renamed it to `reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails` and updated its assertions (`reaped == 3` instead of `2`, the WARN is still asserted). `reapIdle`'s own guard against a `release()` failure it genuinely cannot swallow (e.g. `launcher.stop` throwing) is untouched by this ticket, but this specific test no longer exercises it — I did not add new coverage for that, since building the per-pane failure injection it would need (the fake herdr's `pane.close` failure is global, not per-pane) is outside this ticket's scope. ## Reused helper `GitWorktrees` already had the branch-delete logic from #274, inlined in the private `cleanupAfterAddFailure`. I extracted it into a new `Worktrees.deleteBranch(repoRoot, branch)` interface method (implemented by `GitWorktrees`, and by the test doubles `FakeWorktrees`/`RecordingWorktrees`), and both `cleanupAfterAddFailure` and `SessionManager.acquireWithWorktree`'s catch now call it — no second copy of the git command. ## Out of scope (spotted, not fixed, per the ticket's instruction) None — the ticket's own auditor note says the "guard assigned only at the end" shape does not recur elsewhere in this package, and I did not find any other one-sided-cleanup shape while working this file.
agent added 1 commit 2026-09-04 05:51:40 +02:00
#283: guard release()'s bare worktree remove; delete orphaned branch on spawn failure
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Successful in 2m42s
Details
19cacf5b62 
Two teardown-cleanup leaks in SessionManager, same shape as #274.

Defect 1: release()'s last step (removing a released session's worktree)
was the one cleanup step in the method left unguarded, even though every
sibling step is wrapped because exec() can throw on a non-zero exit or its
own 30s timeout. By the time it ran, the registry entry, retained handle,
and pane were already gone, so a throw here escaped release() with no
retry path and made a fully-torn-down session look like a failed stop.
Now wrapped in try/catch with a WARN, matching the pattern already used
by every other step in this method.

Defect 2: acquireWithWorktree's catch (covering failures after add()
returns — overlayParity, shareWithGroup, launcher.spawn) removed the
worktree but left the branch it provisioned orphaned. #274 already fixed
the sibling failure inside add() itself (GitWorktrees.cleanupAfterAddFailure
deletes both). Extracted that branch-delete into a new Worktrees.deleteBranch
method, reused by both cleanupAfterAddFailure and this catch, so a routine
spawn failure (quarantined credential, backend refusal) no longer leaks a
worker/<slug>-<nonce> branch.

A normal release() still never deletes a branch — only the failed-provision
path does. releaseRemovesWorktreeButDoesNotDeleteBranch pins this, and
spawnFailureAfterAddDeletesTheOrphanedBranch / unchangedRegression* prove
the two paths stay apart.
ltms closed this pull request 2026-09-04 06:00:35 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Successful in 2m42s
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#288
Delete Branch "worker/fix-283-teardown-leaks-f40dfa-9"

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?