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

Two teardown leaks in SessionManager: an unguarded worktree removal, and a branch the spawn-failure catch forgets #283

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

Found by a teardown audit. Two small defects in the same file, so one ticket — they would collide as two PRs. I read and confirmed both myself; line numbers are from main at 66e5247.

Both are the same shape as fleetd #274, which I merged yesterday: cleanup applied to one path and not to its sibling.

Defect 1 — release()'s last step is the one that is not guarded

SessionManager.java:336-338:

launcher.stop(paneId);
if (removed != null && !preserveWorktree && removed.worktree() != null) {
    worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree());
}

Every other cleanup step in this method is wrapped, because exec() can throw on a non-zero exit or on its own 30s timeout. This last one is bare. By the time it runs, registry.remove(paneId), handles.remove(paneId) and launcher.stop(paneId) have all already happened.

So if git worktree remove --force throws — a stale index lock, a slow filesystem, the 30s timeout — the exception escapes release() after the session is already gone from the registry. A second stop is a no-op because the paneId is no longer there, so there is no retry path. The worktree directory leaks forever and is invisible to fleet_list. Meanwhile the caller sees a failed stop for a session that is in fact fully torn down: FleetMcp.stop catches only HerdrException, and FleetApp.stopMember catches nothing.

Fix: wrap it in try/catch with a WARN, matching the pattern every sibling step in this method already uses. The pane is stopped and the registry is clean — a failed directory removal must not be reported as a failed stop.

Defect 2 — the spawn-failure catch removes the worktree but keeps the branch

SessionManager.java:503-513, in acquireWithWorktree:

} catch (RuntimeException e) {
    log.warn("spawn failed for profile={} role={} branch={} path={}: {}", ...);
    if (path != null) {
        try {
            worktrees.remove(repoRoot, path);
        } catch (RuntimeException cleanup) {
            log.warn("failed to clean up worktree {} after spawn error: {}", path, cleanup.getMessage());
        }
    }
    throw e;
}

This catch covers failures after add() returns — overlayParity, shareWithGroup, and launcher.spawn itself. It removes the worktree and forgets the branch.

That is exactly what #274 fixed one layer down: GitWorktrees.cleanupAfterAddFailure deletes the worktree and force-deletes the branch, because a branch that never finished provisioning has nothing else pointing at it. #274 closed the failures inside add(). This catch is the sibling path, and it was left open. A spawn failure is routine — a quarantined credential, a backend refusal — so this leaks an orphan worker/<slug>-<nonce> branch every time one happens.

Fix: after the worktree removal, also git branch -D <branch>, best-effort and log-only, the same way cleanupAfterAddFailure does. Do not let a cleanup failure mask the original exception — it is always rethrown.

Rules for this one

  • Reuse the existing helpers. GitWorktrees already has the branch-delete logic from #274. Prefer calling it over writing a second copy; if it is not reachable from here, say why rather than duplicating it.
  • Prove each fix with a test that fails without it. For defect 1 that means making the removal throw; for defect 2 it means making a post-add() step throw and asserting the branch is gone.
  • Do not change what remove() does for a normal release. It deliberately leaves a released session's branch behind so work can be recovered. Only the failed-provision path deletes the branch. Getting this backwards would destroy a worker's committed work, so state in your report how you kept the two apart.

What was checked and ruled out

The auditor tested two other suspicions live against git 2.53.0 rather than only reasoning about them, and both came back clean: worktrees.repoRoot(removed.cwd()) returning the worktree path still lets git worktree remove --force work correctly, and a single --force is enough for a worktree containing a nested .git directory on this git version. It also reported that the #274 "guard assigned only at the end" shape does not recur elsewhere in this package — every other catch that guards on a local assigns it before the risky call.

Found by a teardown audit. Two small defects in the same file, so one ticket — they would collide as two PRs. **I read and confirmed both myself**; line numbers are from `main` at `66e5247`. Both are the same shape as fleetd #274, which I merged yesterday: cleanup applied to one path and not to its sibling. ## Defect 1 — `release()`'s last step is the one that is not guarded `SessionManager.java:336-338`: ```java launcher.stop(paneId); if (removed != null && !preserveWorktree && removed.worktree() != null) { worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree()); } ``` Every other cleanup step in this method is wrapped, because `exec()` can throw on a non-zero exit **or** on its own 30s timeout. This last one is bare. By the time it runs, `registry.remove(paneId)`, `handles.remove(paneId)` and `launcher.stop(paneId)` have all already happened. So if `git worktree remove --force` throws — a stale index lock, a slow filesystem, the 30s timeout — the exception escapes `release()` **after the session is already gone from the registry**. A second stop is a no-op because the paneId is no longer there, so there is no retry path. The worktree directory leaks forever and is invisible to `fleet_list`. Meanwhile the caller sees a failed stop for a session that is in fact fully torn down: `FleetMcp.stop` catches only `HerdrException`, and `FleetApp.stopMember` catches nothing. **Fix:** wrap it in try/catch with a WARN, matching the pattern every sibling step in this method already uses. The pane is stopped and the registry is clean — a failed directory removal must not be reported as a failed stop. ## Defect 2 — the spawn-failure catch removes the worktree but keeps the branch `SessionManager.java:503-513`, in `acquireWithWorktree`: ```java } catch (RuntimeException e) { log.warn("spawn failed for profile={} role={} branch={} path={}: {}", ...); if (path != null) { try { worktrees.remove(repoRoot, path); } catch (RuntimeException cleanup) { log.warn("failed to clean up worktree {} after spawn error: {}", path, cleanup.getMessage()); } } throw e; } ``` This catch covers failures **after** `add()` returns — `overlayParity`, `shareWithGroup`, and `launcher.spawn` itself. It removes the worktree and forgets the branch. That is exactly what #274 fixed one layer down: `GitWorktrees.cleanupAfterAddFailure` deletes the worktree **and** force-deletes the branch, because a branch that never finished provisioning has nothing else pointing at it. #274 closed the failures *inside* `add()`. This catch is the sibling path, and it was left open. A spawn failure is routine — a quarantined credential, a backend refusal — so this leaks an orphan `worker/<slug>-<nonce>` branch every time one happens. **Fix:** after the worktree removal, also `git branch -D <branch>`, best-effort and log-only, the same way `cleanupAfterAddFailure` does. Do not let a cleanup failure mask the original exception — it is always rethrown. ## Rules for this one - **Reuse the existing helpers.** `GitWorktrees` already has the branch-delete logic from #274. Prefer calling it over writing a second copy; if it is not reachable from here, say why rather than duplicating it. - Prove each fix with a test that fails without it. For defect 1 that means making the removal throw; for defect 2 it means making a post-`add()` step throw and asserting the branch is gone. - **Do not change what `remove()` does for a normal release.** It deliberately leaves a released session's branch behind so work can be recovered. Only the *failed-provision* path deletes the branch. Getting this backwards would destroy a worker's committed work, so state in your report how you kept the two apart. ## What was checked and ruled out The auditor tested two other suspicions live against git 2.53.0 rather than only reasoning about them, and both came back clean: `worktrees.repoRoot(removed.cwd())` returning the worktree path still lets `git worktree remove --force` work correctly, and a single `--force` is enough for a worktree containing a nested `.git` directory on this git version. It also reported that the #274 "guard assigned only at the end" shape does not recur elsewhere in this package — every other catch that guards on a local assigns it before the risky call.
ltms commented 2026-09-04 06:00:29 +02:00
Author
Owner
Copy Link
Copy Source

Merged as c2c2746. Real merge built green at 1287 tests. I ran my own mutations rather than trusting the worker's:

Defect 1 — put release()'s worktree removal back to a bare call:

WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows
  dev.ltms.fleet.session.WorktreeException: stale index lock

Defect 2 — deleted the worktrees.deleteBranch(repoRoot, branch) cleanup:

spawnFailureAfterAddDeletesTheOrphanedBranch:414
  the orphaned branch that add() actually created must also be deleted ==> expected: <1> but was: <0>

Both restored; tree clean.

What I checked beyond the tests

The dangerous part of this ticket was the risk of deleting a branch on a normal release, which would destroy a worker's committed work. The worker kept the paths apart and proved it: releaseRemovesWorktreeButDoesNotDeleteBranch now also asserts deleteBranch was never called after a normal release, and the three pre-existing unchangedRegression* tests are untouched. Only the failed-provisioning test ever sees deleteBranch.

It also avoided a second copy of the git command by lifting git branch -D into Worktrees.deleteBranch(repoRoot, branch), which both GitWorktrees.cleanupAfterAddFailure (#274's path) and the new call site use. I checked that the interface method is abstract, not default — a default would have let an implementation silently skip it, which is a trap this codebase has hit before.

The coverage this cost — filed as #290

Fixing defect 1 removed the trigger that SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease used to reach reapIdle's per-session try/catch. The worker renamed it to reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails, corrected the count from 2 to 3, and said so rather than letting coverage drop quietly. That report is the only reason this is a tracked follow-up instead of a silent loss.

I checked the alternatives myself before filing: notifyReleased (:461-470) already catches a listener's RuntimeException, so it cannot be the trigger. launcher.stop(paneId) at :335 is the one that remains, and reaching it needs per-pane failure injection in FakeHerdr, which is genuinely outside this ticket. That work is #290.

Merged as `c2c2746`. Real merge built green at **1287 tests**. I ran my own mutations rather than trusting the worker's: **Defect 1** — put `release()`'s worktree removal back to a bare call: ``` WorktreeSessionManagerTest.releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows dev.ltms.fleet.session.WorktreeException: stale index lock ``` **Defect 2** — deleted the `worktrees.deleteBranch(repoRoot, branch)` cleanup: ``` spawnFailureAfterAddDeletesTheOrphanedBranch:414 the orphaned branch that add() actually created must also be deleted ==> expected: <1> but was: <0> ``` Both restored; tree clean. ## What I checked beyond the tests The dangerous part of this ticket was the risk of deleting a branch on a **normal** release, which would destroy a worker's committed work. The worker kept the paths apart and proved it: `releaseRemovesWorktreeButDoesNotDeleteBranch` now also asserts `deleteBranch` was never called after a normal release, and the three pre-existing `unchangedRegression*` tests are untouched. Only the failed-provisioning test ever sees `deleteBranch`. It also avoided a second copy of the git command by lifting `git branch -D` into `Worktrees.deleteBranch(repoRoot, branch)`, which both `GitWorktrees.cleanupAfterAddFailure` (#274's path) and the new call site use. I checked that the interface method is **abstract, not `default`** — a `default` would have let an implementation silently skip it, which is a trap this codebase has hit before. ## The coverage this cost — filed as #290 Fixing defect 1 removed the trigger that `SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease` used to reach `reapIdle`'s per-session try/catch. The worker renamed it to `reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails`, corrected the count from 2 to 3, and **said so** rather than letting coverage drop quietly. That report is the only reason this is a tracked follow-up instead of a silent loss. I checked the alternatives myself before filing: `notifyReleased` (`:461-470`) already catches a listener's `RuntimeException`, so it cannot be the trigger. `launcher.stop(paneId)` at `:335` is the one that remains, and reaching it needs per-pane failure injection in `FakeHerdr`, which is genuinely outside this ticket. That work is #290.
ltms closed this issue 2026-09-04 06:00:32 +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#283
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?