diff --git a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java index 0a7563c..ad94c60 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java @@ -197,27 +197,44 @@ public final class SessionManager implements TurnListener { MemberSession removed = registry.remove(paneId); boolean preserveWorktree = cause == ReleaseCause.SHUTDOWN; if (removed != null) { - memberLifecycle.released(removed.terminalId()); - log.debug("releasing session pane={} terminal={} state={} cause={}", - removed.paneId(), removed.terminalId(), removed.state(), cause); - if (preserveWorktree && removed.worktree() != null) { - logPreservedForShutdown(removed); - } else if (removed.worktree() != null && worktrees.hasUncommitted(removed.worktree())) { - // CB-576: a release that would otherwise remove the worktree finds it holding - // uncommitted work the bridge cannot see. A worker that ends a turn without - // committing (normally because it stopped to ask a question or refused the turn) - // has its only copy of that work in the worktree. Remove would --force-delete it, - // so preserve the directory and tell an operator where to find it. + try { + memberLifecycle.released(removed.terminalId()); + log.debug("releasing session pane={} terminal={} state={} cause={}", + removed.paneId(), removed.terminalId(), removed.state(), cause); + if (preserveWorktree && removed.worktree() != null) { + logPreservedForShutdown(removed); + } else if (removed.worktree() != null && worktrees.hasUncommitted(removed.worktree())) { + // CB-576: a release that would otherwise remove the worktree finds it holding + // uncommitted work the bridge cannot see. A worker that ends a turn without + // committing (normally because it stopped to ask a question or refused the turn) + // has its only copy of that work in the worktree. Remove would --force-delete it, + // so preserve the directory and tell an operator where to find it. + preserveWorktree = true; + log.warn("release {} preserves dirty worktree {} for pane={} terminal={}: " + + "the worktree holds uncommitted changes that --force remove would destroy", + cause, removed.worktree(), removed.paneId(), removed.terminalId()); + } + } catch (RuntimeException e) { + // CB-581: hasUncommitted shells out to `git status` and can throw on a non-zero + // exit. We can no longer tell whether the worktree holds uncommitted work, so fail + // toward the safe answer and preserve it — deleting on a guess can destroy work + // that has no other copy (CB-576), while keeping it on a false alarm only costs + // disk. The exception must not propagate: the pane still has to stop below. preserveWorktree = true; - log.warn("release {} preserves dirty worktree {} for pane={} terminal={}: " - + "the worktree holds uncommitted changes that --force remove would destroy", - cause, removed.worktree(), removed.paneId(), removed.terminalId()); + log.warn("release {} could not tell whether worktree {} for pane={} terminal={} has " + + "uncommitted changes; preserving it rather than risk destroying unsaved work: {}", + cause, removed.worktree(), removed.paneId(), removed.terminalId(), e.toString()); + } finally { + // CB-516/CB-581: a send still waiting on this worker can never be answered now, no + // matter what happened above. Tell the listener BEFORE the pane is torn down, so a + // blocked caller fails fast with a real reason instead of sitting on a rendezvous + // nothing will ever resolve. + notifyReleased(removed.terminalId()); } - // CB-516: a send still waiting on this worker can never be answered now. Tell the - // listener BEFORE the pane is torn down, so a blocked caller fails fast with a real - // reason instead of sitting on a rendezvous nothing will ever resolve. - notifyReleased(removed.terminalId()); } + // CB-581: the pane must always stop, even if the dirty check above threw. A session removed + // from the registry with no pane stop is an orphaned pane — a live terminal burning a fleet + // 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()); @@ -539,8 +556,15 @@ public final class SessionManager implements TurnListener { log.debug("reaping idle session terminal={} pane={}: idle {}s exceeds the {}s ttl", s.terminalId(), s.paneId(), TimeUnit.NANOSECONDS.toSeconds(idleNanos), TimeUnit.NANOSECONDS.toSeconds(idleTtlNanos)); - release(s.paneId()); - reaped++; + // CB-581: one session that fails to release must not abort the whole reaping pass — + // match drainAll's per-session try/catch so the rest of the roster still gets reaped. + try { + release(s.paneId()); + reaped++; + } catch (RuntimeException e) { + log.warn("reap failed for pane={} terminal={} worktree={}; continuing with " + + "remaining sessions", s.paneId(), s.terminalId(), s.worktree(), e); + } } } return reaped; diff --git a/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java b/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java index d8d9009..41ea200 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java @@ -46,6 +46,82 @@ class SessionManagerTest { return sessionManager(herdr, clock, 0); } + private SessionManager sessionManager(FakeHerdr herdr, Worktrees worktrees) { + return sessionManager(herdr, worktrees, System::nanoTime); + } + + private SessionManager sessionManager(FakeHerdr herdr, Worktrees worktrees, LongSupplier clock) { + BridgedConfig.Profile cfg = new BridgedConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "BRIDGED_WORKER_TOKEN", + List.of("ccs", "ltms-local"), "tab", "bridged-workers", + "worker: {profile} #{n}", null, null, null); + ClaudeCodeLauncher workers = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null); + return new SessionManager(workers, worktrees, clock); + } + + /** + * CB-581: a {@link Worktrees} test double whose {@code hasUncommitted} and {@code remove} can + * be told to throw, so {@link SessionManager#release} can be exercised against exactly the + * failure {@code GitWorktrees} produces when {@code git status}/{@code git worktree remove} + * exits non-zero. + */ + private static final class RecordingWorktrees implements Worktrees { + private final List removeCalls = new java.util.ArrayList<>(); + private final java.util.Set failRemoveFor = new java.util.HashSet<>(); + private volatile boolean dirty = false; + private volatile RuntimeException hasUncommittedFailure; + + RecordingWorktrees dirty(boolean dirty) { + this.dirty = dirty; + return this; + } + + RecordingWorktrees failHasUncommittedWith(RuntimeException e) { + this.hasUncommittedFailure = e; + return this; + } + + RecordingWorktrees failRemoveFor(String worktreePath) { + failRemoveFor.add(worktreePath); + return this; + } + + @Override + public String add(String repoRoot, String branch, String baseRef) { + return "/wt/" + branch.replace('/', '_'); + } + + @Override + public void remove(String repoRoot, String worktreePath) { + if (failRemoveFor.contains(worktreePath)) { + throw new WorktreeException("simulated remove failure for " + worktreePath); + } + removeCalls.add(worktreePath); + } + + @Override + public boolean hasUncommitted(String worktreePath) { + if (hasUncommittedFailure != null) { + throw hasUncommittedFailure; + } + return dirty; + } + + @Override + public void overlayParity(String repoRoot, String worktreePath, List overlay) { + } + + @Override + public String repoRoot(String cwd) { + return "/repo"; + } + + List removeCalls() { + return List.copyOf(removeCalls); + } + } + private SessionManager sessionManager(FakeHerdr herdr, LongSupplier clock, int contextCap) { return sessionManager(herdr, clock, contextCap, false); } @@ -525,4 +601,172 @@ class SessionManagerTest { "a listener failure must never prevent the teardown it is reacting to"); assertTrue(sessions.get(s.paneId()).isEmpty(), "and the session is still deregistered"); } + + // --- CB-581: a throw inside release() must not orphan the pane or abort reapIdle ----------- + + @Test + void releasePreservesWorktreeWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581a", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + try { + assertDoesNotThrow(() -> sessions.release(s.paneId()), + "a throwing dirty check must not abort the release"); + + assertTrue(worktrees.removeCalls().isEmpty(), + "the worktree is preserved when its dirty state cannot be determined"); + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains(s.worktree())) + .findFirst() + .orElse("no warn logged naming the worktree"); + assertTrue(warn.contains(s.paneId()), "the WARN names the pane: " + warn); + assertTrue(warn.contains(s.terminalId()), "the WARN names the terminal: " + warn); + } finally { + sessionLog.detachAppender(appender); + } + } + + @Test + void releaseStillStopsThePaneWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581b", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + sessions.release(s.paneId()); + + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_1"), + "the pane is stopped exactly once even though the dirty check threw"); + } + + @Test + void releaseStillNotifiesTheListenerWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + java.util.List released = new java.util.concurrent.CopyOnWriteArrayList<>(); + sessions.onRelease(released::add); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581c", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + sessions.release(s.paneId()); + + assertEquals(java.util.List.of(s.terminalId()), released, + "a blocked caller must still be told the terminal was released, even though the " + + "dirty check threw"); + } + + @Test + void reapIdleSurvivesOneSessionThatFailsToRelease() { + long[] clock = {0}; + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees, () -> clock[0]); + MemberSession a = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581d", null)); + MemberSession b = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581e", null)); + MemberSession c = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581f", null)); + 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. + worktrees.failRemoveFor(b.worktree()); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + int reaped; + try { + clock[0] = 100; + reaped = sessions.reapIdle(10); + + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains(b.paneId())) + .findFirst() + .orElse("no reap-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"); + 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(), + "the middle session is still deregistered even though its worktree removal threw"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_1"), "the first pane is stopped"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_2"), + "the middle pane is still stopped even though its worktree removal failed"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_3"), "the third pane is stopped"); + } + + @Test + void unchangedRegressionCleanCompletedReleaseStillRemovesTheWorktree() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581g", null)); + + sessions.release(s.paneId()); + + assertEquals(List.of(s.worktree()), worktrees.removeCalls(), + "COMPLETED release of a clean worktree still removes it"); + } + + @Test + void unchangedRegressionDirtyCompletedReleaseStillPreservesTheWorktree() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees().dirty(true); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581h", null)); + + sessions.release(s.paneId()); + + assertTrue(worktrees.removeCalls().isEmpty(), + "COMPLETED release of a dirty worktree still preserves it"); + } + + @Test + void unchangedRegressionShutdownDrainStillPreservesTheWorktree() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581i", null)); + sessions.asPresence().markPresent(s.terminalId()); + + sessions.drainAll(TimeUnit.MILLISECONDS.toNanos(100)); + + assertTrue(worktrees.removeCalls().isEmpty(), "SHUTDOWN drain still preserves the worktree"); + } }