diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 2534c61..a9d2d86 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/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. diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java index e58bb33..5ffa5b8 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/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/-` 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; } diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java index 2005f3c..7dacf8f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java @@ -11,6 +11,15 @@ public interface Worktrees { /** git -C worktree remove --force . 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 diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java b/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java index 7c1d000..3eeedc1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java +++ b/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 requested, List copied, List skipWorktree) { } @@ -35,6 +38,7 @@ public final class FakeWorktrees implements Worktrees { private final List addCalls = new CopyOnWriteArrayList<>(); private final List removeCalls = new CopyOnWriteArrayList<>(); + private final List deleteBranchCalls = new CopyOnWriteArrayList<>(); private final List overlayCalls = new CopyOnWriteArrayList<>(); private final List repoRootCalls = new CopyOnWriteArrayList<>(); private final List 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 overlay) { + if (overlayFailure != null) { + throw overlayFailure; + } List copied = new java.util.ArrayList<>(); List 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 deleteBranchCalls() { + return List.copyOf(deleteBranchCalls); + } + + public DeleteBranchCall lastDeleteBranch() { + return deleteBranchCalls.isEmpty() ? null : deleteBranchCalls.getLast(); + } + public OverlayCall lastOverlay() { return overlayCalls.isEmpty() ? null : overlayCalls.getLast(); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java index 8ec185c..76b2d9e 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/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 removeCalls = new java.util.ArrayList<>(); + private final List deleteBranchCalls = new java.util.ArrayList<>(); private final List snapshotCalls = new java.util.ArrayList<>(); private final java.util.Set 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 deleteBranchCalls() { + return List.copyOf(deleteBranchCalls); + } + List 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(), diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java index 07a0732..284c588 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java +++ b/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/-} + * 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