From 3cbbc509234da1f893692705975d09f0cf8e70b4 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 10:33:09 +0700 Subject: [PATCH] audit: teardown/cleanup review of dev.ltms.fleet.session --- AUDIT.md | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 AUDIT.md diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..bdfde3c --- /dev/null +++ b/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/-` 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 remove --force ` 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.