audit: teardown/cleanup review of dev.ltms.fleet.session

AUDIT.md

@@ -0,0 +1,69 @@
+# Teardown/cleanup audit — dev.ltms.fleet.session
+
+Scope: `SessionManager.java`, `GitWorktrees.java`, `SessionReaper.java`, `MemberSession.java`,
+`Worktrees.java` (interface). Read-only; no code changed.
+
+## Main finding
+
+```
+1. SessionManager.java:336-338
+2. issue: the final worktree removal in release() is the one step in the whole method
+   that is not wrapped in try/catch. Every other cleanup step here (hasUncommitted check,
+   snapshot, listener notification) is defended because a `git` call can throw — exec()'s
+   own javadoc documents both a non-zero exit and its 30-second timeout as normal failure
+   modes, and every sibling worktrees.* call in this class is guarded against exactly that.
+   By the time this line runs, registry.remove(paneId) and handles.remove(paneId) have
+   already happened and launcher.stop(paneId) has already run, so if worktrees.remove()
+   throws here (e.g. `git worktree remove --force` times out on a stale lock file or a
+   slow/network filesystem, or exits non-zero), the exception escapes release() with no
+   way to retry: the paneId is already gone from the registry, so a second stop call is a
+   no-op and never re-attempts the removal. The worktree directory is now leaked forever,
+   invisible to `fleet_list`. The caller sees a stop failure — FleetMcp.stop() only catches
+   HerdrException, and FleetApp.stopMember() catches nothing — even though the session was
+   in fact fully torn down (pane stopped, deregistered, listeners notified).
+3. fix: wrap the `worktrees.remove(...)` call at the end of release() in a try/catch that
+   logs a warning, matching the pattern already used for every other cleanup step in this
+   method (e.g. cleanupAfterAddFailure's own worktree/branch removal, or the dirty-check
+   catch above it).
+4. severity: medium
+```
+
+## Secondary findings
+
+```
+1. SessionManager.java:503-514 (acquireWithWorktree's catch block)
+2. issue: after worktrees.add() succeeds, if overlayParity(), shareWithGroup(), or
+   launcher.spawn() then throws, the catch block removes only the worktree
+   (worktrees.remove(repoRoot, path)) and never deletes the branch `git worktree add`
+   created. GitWorktrees.cleanupAfterAddFailure — the sibling cleanup for failures inside
+   add() itself — explicitly deletes the branch too, with a `-D` and a documented reason
+   ("a branch that never finished provisioning has no session, no PR, nothing else
+   pointing at it"). That reasoning applies equally here, but this later catch block (the
+   one covering the three post-add() steps) omits it. Since spawn failures are a normal,
+   recurring event (this very branch already logs "spawn failed for profile=..."), this
+   leaks an orphan `worker/<slug>-<nonce>` branch in the shared repo on every such failure,
+   with nothing pointing at it once the (failed) session is never registered.
+3. fix: after worktrees.remove(...) succeeds in this catch, also delete the branch with
+   `git branch -D branch` (best-effort, log-only on failure), matching
+   cleanupAfterAddFailure's own two-step cleanup.
+4. severity: low
+```
+
+No other issue in this scope survived a read of every exit of `add()`, `remove()`,
+`snapshot()`, `overlayParity()`, `shareWithGroup()`/`shareRootWithGroup()`, `release()`,
+`reapIdle()`, `drainAll()`, and the `SessionReaper` loop. Two shapes I checked and ruled
+out as not reachable / not defects:
+
+- `release()`'s `worktrees.repoRoot(removed.cwd())` looked suspicious because `cwd` for a
+  worktree session is the worktree path itself, so `repoRoot` would equal `worktreePath` —
+  but I verified with a live git repo (`git --version` 2.53.0) that
+  `git -C <worktree> worktree remove --force <same worktree>` works correctly: git
+  resolves `-C` against the common git dir regardless of which linked worktree it's given,
+  so this is not a bug.
+- `git worktree remove --force` on a worktree containing a nested `.git` directory: I
+  expected this to need a double `--force` per older git docs, but tested it live and a
+  single `--force` succeeds on git 2.53.0. Not a live failure mode on this stack.
+
+The `if (x != null)` guard-in-catch shape from fleetd #274 (guard assigned only at the end)
+does not recur elsewhere in this scope: every `catch` block that guards on a local now
+assigns that local before the risky call it protects, not after.