Merge #283 (PR #288): guard release()'s worktree removal, delete the orphaned branch on spawn failure

fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java

@@ -213,13 +213,18 @@ public final class GitWorktrees implements Worktrees {
                     worktreePath, cleanup.getMessage());
         }
         try {
-            exec("git", "-C", repoRoot, "branch", "-D", branch);
+            deleteBranch(repoRoot, branch);
         } catch (RuntimeException cleanup) {
             log.warn("failed to remove leaked branch {} after provisioning error: {}",
                     branch, cleanup.getMessage());
         }
     }
 
+    @Override
+    public void deleteBranch(String repoRoot, String branch) {
+        exec("git", "-C", repoRoot, "branch", "-D", branch);
+    }
+
     /**
      * A linked worktree shares its primary checkout's git config. Remove HTTPS user info before
      * adding one, so a credential accidentally embedded in that config cannot reach the member.

fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java

@@ -334,7 +334,20 @@ public final class SessionManager implements TurnListener {
         // slot that no longer appears in the roster and can never be reclaimed.
         launcher.stop(paneId);
         if (removed != null && !preserveWorktree && removed.worktree() != null) {
-            worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree());
+            // fleetd #283: this is the one cleanup step in this method that used to be bare. By the
+            // time it runs, the registry entry, the retained handle, and the pane are all already
+            // gone — so a throw here (a stale index lock, a slow filesystem, `remove`'s own 30s exec
+            // timeout) must not escape release(): there is no retry path (a second stop on this
+            // paneId is a no-op), and the caller would otherwise see a "failed stop" for a session
+            // that is in fact fully torn down. Log and swallow, matching every sibling step above.
+            try {
+                worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree());
+            } catch (RuntimeException e) {
+                log.warn("failed to remove worktree {} for pane={} terminal={} after release: the "
+                        + "pane is already stopped and the session already deregistered, so this is "
+                        + "not retryable — the directory must be reclaimed manually: {}",
+                        removed.worktree(), paneId, removed.terminalId(), e.toString());
+            }
         }
     }
 
@@ -504,11 +517,26 @@ public final class SessionManager implements TurnListener {
             log.warn("spawn failed for profile={} role={} branch={} path={}: {}",
                     preResolvedProfile, memberRole, branch, path, e.getMessage());
             if (path != null) {
+                // fleetd #283: this catch covers every failure AFTER worktrees.add() returned —
+                // overlayParity, shareWithGroup, launcher.spawn itself — so by this point `branch`
+                // was actually created in git. #274 fixed the sibling failure INSIDE add() by having
+                // GitWorktrees.cleanupAfterAddFailure delete both the worktree and the branch it
+                // provisioned; 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 nothing ever pointed at
+                // again. Reuse the same Worktrees.deleteBranch GitWorktrees already has, rather than
+                // a second copy of the git command. Best-effort and log-only, like the worktree
+                // removal right above it — neither cleanup step may mask the original exception.
                 try {
                     worktrees.remove(repoRoot, path);
                 } catch (RuntimeException cleanup) {
                     log.warn("failed to clean up worktree {} after spawn error: {}", path, cleanup.getMessage());
                 }
+                try {
+                    worktrees.deleteBranch(repoRoot, branch);
+                } catch (RuntimeException cleanup) {
+                    log.warn("failed to clean up branch {} after spawn error: {}", branch, cleanup.getMessage());
+                }
             }
             throw e;
         }

fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java

@@ -11,6 +11,15 @@ public interface Worktrees {
     /** git -C <repoRoot> worktree remove --force <path>. Idempotent (already-gone tolerated). */
     void remove(String repoRoot, String worktreePath);
 
+    /**
+     * git -C {@code repoRoot} branch -D {@code branch}. Force-deletes a branch that has no other
+     * owner — used only on the failed-provisioning path (fleetd #274, #283), never on a normal
+     * release: {@link SessionManager#release} deliberately leaves a released session's branch
+     * behind so a lead can still recover the work, and this method must never be called from
+     * that path.
+     */
+    void deleteBranch(String repoRoot, String branch);
+
     /**
      * True when the worktree holds uncommitted changes the bridge cannot see: tracked
      * modifications, staged files, or untracked files. {@code git status --porcelain} is the

fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java

@@ -17,6 +17,9 @@ public final class FakeWorktrees implements Worktrees {
     public record RemoveCall(String repoRoot, String worktreePath) {
     }
 
+    public record DeleteBranchCall(String repoRoot, String branch) {
+    }
+
     public record OverlayCall(String repoRoot, String worktreePath,
                                List<String> requested, List<String> copied, List<String> skipWorktree) {
     }
@@ -35,6 +38,7 @@ public final class FakeWorktrees implements Worktrees {
 
     private final List<AddCall> addCalls = new CopyOnWriteArrayList<>();
     private final List<RemoveCall> removeCalls = new CopyOnWriteArrayList<>();
+    private final List<DeleteBranchCall> deleteBranchCalls = new CopyOnWriteArrayList<>();
     private final List<OverlayCall> overlayCalls = new CopyOnWriteArrayList<>();
     private final List<RepoRootCall> repoRootCalls = new CopyOnWriteArrayList<>();
     private final List<SnapshotCall> snapshotCalls = new CopyOnWriteArrayList<>();
@@ -51,6 +55,8 @@ public final class FakeWorktrees implements Worktrees {
     private final AtomicLong snapshotSeq = new AtomicLong();
     private volatile RuntimeException addFailure;
     private volatile RuntimeException snapshotFailure;
+    private volatile RuntimeException removeFailure;
+    private volatile RuntimeException overlayFailure;
     private volatile boolean dirty = false;
     private volatile String repoRoot = "/repo";
     private volatile String prefix = "/worktrees";
@@ -98,6 +104,21 @@ public final class FakeWorktrees implements Worktrees {
         return this;
     }
 
+    /** Make subsequent {@link #remove} calls throw (fleetd #283: a stale index lock, a slow
+     *  filesystem, or {@code remove}'s own 30s exec timeout escaping the last, previously bare,
+     *  step of {@link SessionManager#release}). */
+    public FakeWorktrees failRemove(String message) {
+        this.removeFailure = new WorktreeException(message);
+        return this;
+    }
+
+    /** Make subsequent {@link #overlayParity} calls throw (simulates a post-{@code add()} spawn
+     *  failure — fleetd #283 defect 2 — so the {@code acquireWithWorktree} catch runs). */
+    public FakeWorktrees failOverlay(String message) {
+        this.overlayFailure = new WorktreeException(message);
+        return this;
+    }
+
     /** Configure the value returned by {@link #wipRefs}. */
     public FakeWorktrees withWipRefs(WipRefStats stats) {
         this.wipRefs = stats;
@@ -132,6 +153,14 @@ public final class FakeWorktrees implements Worktrees {
     @Override
     public void remove(String repoRoot, String worktreePath) {
         removeCalls.add(new RemoveCall(repoRoot, worktreePath));
+        if (removeFailure != null) {
+            throw removeFailure;
+        }
+    }
+
+    @Override
+    public void deleteBranch(String repoRoot, String branch) {
+        deleteBranchCalls.add(new DeleteBranchCall(repoRoot, branch));
     }
 
     @Override
@@ -146,6 +175,9 @@ public final class FakeWorktrees implements Worktrees {
 
     @Override
     public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
+        if (overlayFailure != null) {
+            throw overlayFailure;
+        }
         List<String> copied = new java.util.ArrayList<>();
         List<String> skipped = new java.util.ArrayList<>();
         for (String rel : overlay) {
@@ -218,6 +250,14 @@ public final class FakeWorktrees implements Worktrees {
         return removeCalls.isEmpty() ? null : removeCalls.getLast();
     }
 
+    public List<DeleteBranchCall> deleteBranchCalls() {
+        return List.copyOf(deleteBranchCalls);
+    }
+
+    public DeleteBranchCall lastDeleteBranch() {
+        return deleteBranchCalls.isEmpty() ? null : deleteBranchCalls.getLast();
+    }
+
     public OverlayCall lastOverlay() {
         return overlayCalls.isEmpty() ? null : overlayCalls.getLast();
     }

fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java

@@ -74,6 +74,7 @@ class SessionManagerTest {
      */
     private static final class RecordingWorktrees implements Worktrees {
         private final List<String> removeCalls = new java.util.ArrayList<>();
+        private final List<String> deleteBranchCalls = new java.util.ArrayList<>();
         private final List<String> snapshotCalls = new java.util.ArrayList<>();
         private final java.util.Set<String> failRemoveFor = new java.util.HashSet<>();
         private volatile boolean dirty = false;
@@ -114,6 +115,11 @@ class SessionManagerTest {
             removeCalls.add(worktreePath);
         }
 
+        @Override
+        public void deleteBranch(String repoRoot, String branch) {
+            deleteBranchCalls.add(branch);
+        }
+
         @Override
         public boolean hasUncommitted(String worktreePath) {
             if (hasUncommittedFailure != null) {
@@ -158,6 +164,10 @@ class SessionManagerTest {
             return List.copyOf(removeCalls);
         }
 
+        List<String> deleteBranchCalls() {
+            return List.copyOf(deleteBranchCalls);
+        }
+
         List<String> snapshotCalls() {
             return List.copyOf(snapshotCalls);
         }
@@ -989,8 +999,21 @@ class SessionManagerTest {
                         + "dirty check threw");
     }
 
+    /**
+     * fleetd #283 defect 1 changed this test's own premise, so its assertions are updated along
+     * with the production fix. Before #283, the middle session's worktree-removal failure escaped
+     * {@code release()} uncaught, and this test proved {@code reapIdle}'s own per-session try/catch
+     * (CB-581) kept the rest of the pass going regardless. Now that {@code release()} itself catches
+     * a worktree-removal failure (matching every sibling cleanup step in that method) and only logs
+     * a WARN, {@code release()} no longer throws for this reason — so all three idle sessions are
+     * released and counted, and the middle one's removal failure is now visible only as the WARN
+     * {@code release()} itself logs, not as a reap-loop catch. {@code reapIdle}'s own guard (for a
+     * failure {@code release()} still cannot swallow, e.g. from {@code launcher.stop}) is untouched
+     * by this ticket. This test no longer exercises that guard — reaching it now needs a failure
+     * that #283 does not catch inside {@code release()} itself.
+     */
     @Test
-    void reapIdleSurvivesOneSessionThatFailsToRelease() {
+    void reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails() {
         long[] clock = {0};
         FakeHerdr herdr = new FakeHerdr();
         RecordingWorktrees worktrees = new RecordingWorktrees();
@@ -1004,8 +1027,8 @@ class SessionManagerTest {
         sessions.asPresence().markPresent(a.terminalId());
         sessions.asPresence().markPresent(b.terminalId());
         sessions.asPresence().markPresent(c.terminalId());
-        // The middle session's worktree removal fails — release() propagates that, so this is the
-        // one call reapIdle's per-session guard must survive without skipping the rest of the pass.
+        // The middle session's worktree removal fails — fleetd #283 makes release() catch and log
+        // this itself, so it no longer propagates out of release() at all.
         worktrees.failRemoveFor(b.worktree());
 
         LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory();
@@ -1026,14 +1049,16 @@ class SessionManagerTest {
                     .map(ILoggingEvent::getFormattedMessage)
                     .filter(m -> m.contains(b.paneId()))
                     .findFirst()
-                    .orElse("no reap-failure WARN logged");
+                    .orElse("no worktree-removal-failure WARN logged");
             assertTrue(warn.contains(b.terminalId()), "the WARN names the failed session's terminal: " + warn);
             assertTrue(warn.contains(b.worktree()), "the WARN names the failed session's worktree: " + warn);
         } finally {
             sessionLog.detachAppender(appender);
         }
 
-        assertEquals(2, reaped, "the middle session's failure is logged, not counted as reaped");
+        assertEquals(3, reaped,
+                "fleetd #283: release() no longer throws for a worktree-removal failure, so reapIdle "
+                        + "counts all three idle sessions as reaped");
         assertTrue(sessions.get(a.paneId()).isEmpty(), "the first session is still released");
         assertTrue(sessions.get(c.paneId()).isEmpty(), "the third session is still released");
         assertTrue(sessions.get(b.paneId()).isEmpty(),

fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java

@@ -212,9 +212,41 @@ class WorktreeSessionManagerTest {
         assertEquals("/repo", remove.repoRoot());
         assertEquals(s.worktree(), remove.worktreePath());
         // The fake records no branch-delete calls because Worktrees.remove only removes the checkout.
+        assertTrue(worktrees.deleteBranchCalls().isEmpty(),
+                "a normal release must NEVER delete the branch — it is the worker's only recoverable "
+                        + "copy of committed work, and only the failed-provisioning path may remove it");
         assertTrue(sessions.get(paneId).isEmpty(), "released session is no longer retrievable");
     }
 
+    /**
+     * fleetd #283 defect 1. Every other cleanup step in {@code release()} is wrapped in try/catch,
+     * because {@code exec()} can throw on a non-zero exit or its own 30s timeout — this was the one
+     * step left bare. By the time it runs, the registry entry, the retained handle, and the pane are
+     * all already gone, so a throw here used to escape {@code release()} after the session was
+     * already fully torn down: a second stop on the same paneId is a no-op (nothing left to find),
+     * so there was no retry path, and the caller saw a failed stop for a session that was in fact
+     * gone. This test makes the worktree removal throw and asserts release() still completes with
+     * the pane stopped and the registry clean.
+     */
+    @Test
+    void releaseCompletesAndStopsPaneEvenWhenWorktreeRemovalThrows() {
+        FakeHerdr herdr = new FakeHerdr();
+        FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt")
+                .failRemove("stale index lock");
+        SessionManager sessions = new SessionManager(workerService(herdr), worktrees);
+        MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
+                new WorktreeRequest("cb-283-1", null));
+        String paneId = s.paneId();
+
+        sessions.release(paneId); // must not throw
+
+        assertTrue(herdr.called("pane.close"), "the pane is still stopped despite the removal failure");
+        assertEquals(1, worktrees.removeCalls().size(), "worktree removal was still attempted");
+        assertTrue(sessions.get(paneId).isEmpty(),
+                "the session is deregistered regardless of the removal failure");
+        assertEquals(0, sessions.size(), "the registry is left clean");
+    }
+
     /**
      * CB-576. A normal {@code COMPLETED} release whose worktree holds uncommitted work must NOT
      * remove it — {@code --force} would destroy the worker's only copy. The bridge cannot see
@@ -352,6 +384,39 @@ class WorktreeSessionManagerTest {
         assertEquals(0, sessions.size(), "failed acquire leaves no registry entry");
         assertFalse(herdr.called("agent.start"), "spawn is never reached when add fails");
         assertTrue(worktrees.removeCalls().isEmpty(), "no worktree was added, so none is removed");
+        assertTrue(worktrees.deleteBranchCalls().isEmpty(),
+                "add() itself never created the branch in git, so there is nothing to delete");
+    }
+
+    /**
+     * fleetd #283 defect 2. {@code acquireWithWorktree}'s catch covers every failure AFTER
+     * {@code worktrees.add()} returns — {@code overlayParity}, {@code shareWithGroup},
+     * {@code launcher.spawn} itself — so by the time it runs, {@code branch} was actually created in
+     * git. It removed only the worktree and forgot the branch, leaking a {@code worker/<slug>-<nonce>}
+     * branch on every routine spawn failure (a quarantined credential, a backend refusal). This test
+     * makes {@code overlayParity} (a post-add() step) throw and asserts the branch is deleted, the
+     * same way #274 already does for the sibling failure inside {@code add()} itself.
+     */
+    @Test
+    void spawnFailureAfterAddDeletesTheOrphanedBranch() {
+        FakeHerdr herdr = new FakeHerdr();
+        FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt")
+                .failOverlay("overlayParity failed");
+        SessionManager sessions = new SessionManager(workerService(herdr), worktrees);
+
+        assertThrows(WorktreeException.class, () ->
+                sessions.acquire("ltms-local", null, "/caller/proj", null,
+                        new WorktreeRequest("cb-283-2", null)));
+
+        assertEquals(0, sessions.size(), "failed acquire leaves no registry entry");
+        assertFalse(herdr.called("agent.start"), "spawn is never reached when overlayParity fails");
+        assertEquals(1, worktrees.removeCalls().size(), "the worktree checkout is still removed");
+        assertEquals(1, worktrees.deleteBranchCalls().size(),
+                "the orphaned branch that add() actually created must also be deleted");
+        FakeWorktrees.DeleteBranchCall del = worktrees.lastDeleteBranch();
+        assertEquals("/repo", del.repoRoot());
+        FakeWorktrees.AddCall add = worktrees.lastAdd();
+        assertEquals(add.branch(), del.branch(), "the branch deleted is the exact one add() created");
     }
 
     @Test