From 04e21c92433c126f8b9be18e9dbaaaa7de4181c0 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 13 Aug 2026 20:36:32 +0200 Subject: [PATCH] CB-544: shutdown drain preserves worker worktrees (no data loss) --- .../ltms/bridged/session/SessionManager.java | 71 ++++++++++++++++--- .../session/WorktreeSessionManagerTest.java | 37 ++++++++++ 2 files changed, 100 insertions(+), 8 deletions(-) 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 b560bf7..76f84d1 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java @@ -145,21 +145,70 @@ public final class SessionManager implements TurnListener { /** Tear a worker down by pane id and remove it from the registry. Idempotent. */ public void release(String paneId) { + release(paneId, ReleaseCause.COMPLETED); + } + + /** + * Core teardown: always stops the worker pane and deregisters the session; whether the worker's + * git worktree is also removed depends on {@code cause}. + * + *

CB-544: these are two concerns that used to be fused. Stopping the pane is correct on every + * teardown — the worker process must end. Removing the worktree is a destructive act that is only + * correct for a deliberately-finished teardown (an explicit stop of a completed session, the + * reaper releasing a genuinely idle one, a context-capped or recycled session). A shutdown drain + * must stop panes but preserve worktrees: a worker's uncommitted work exists in exactly one + * place — its worktree — so deleting it while the daemon simply goes down is silent data loss, + * with no copy and no error. Do NOT fuse these back together; the cost of an orphaned worktree + * is a logged path an operator can reclaim, the cost of a deleted one is unrecoverable work. + */ + private void release(String paneId, ReleaseCause cause) { WorkerSession removed = registry.remove(paneId); + boolean preserveWorktree = cause == ReleaseCause.SHUTDOWN; if (removed != null) { - log.debug("releasing session pane={} terminal={} state={}", - removed.paneId(), removed.terminalId(), removed.state()); + log.debug("releasing session pane={} terminal={} state={} cause={}", + removed.paneId(), removed.terminalId(), removed.state(), cause); + if (preserveWorktree && removed.worktree() != null) { + logPreservedForShutdown(removed); + } // 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()); } launcher.stop(paneId); - if (removed != null && removed.worktree() != null) { + if (removed != null && !preserveWorktree && removed.worktree() != null) { worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree()); } } + /** + * Why a session is being released — governs whether its worktree is preserved or removed. + * Worktree removal is reserved for the one case that is genuinely finished; everything else + * must keep the worker's only copy of its work. + */ + public enum ReleaseCause { + /** Deliberate teardown of a finished session. Stops the pane and removes the worktree. */ + COMPLETED, + /** Daemon shutdown drain. Stops the pane but PRESERVES the worktree. */ + SHUTDOWN + } + + /** + * CB-544 shutdown drain log for a worktree we deliberately kept. A session still {@code BUSY} + * when the drain timeout expired was abandoned mid-turn — that work may be uncommitted and is + * the only copy — so the message is loud and points at the path an operator needs to reclaim. + */ + private void logPreservedForShutdown(WorkerSession session) { + if (session.state() == WorkerSession.State.BUSY) { + log.warn("shutdown drain abandoned BUSY session pane={} terminal={} mid-turn; " + + "worktree preserved at {}", session.paneId(), session.terminalId(), + session.worktree()); + } else { + log.info("shutdown drain preserved worktree at {} for pane={}", + session.worktree(), session.paneId()); + } + } + /** * Register a callback invoked with a session's {@code terminalId} whenever it is acquired * (CB-520). This is the hook that lets the reply inbox {@code own} a target's queue. @@ -438,10 +487,16 @@ public final class SessionManager implements TurnListener { } /** - * Gracefully drain all registered sessions. For each session that is {@code BUSY}, poll up to - * {@code timeoutNanos} for it to leave {@code BUSY}, then release it regardless. Non-busy - * sessions are released immediately. A failure releasing one session is logged and does not - * abort the rest. + * Gracefully drain all registered sessions on daemon shutdown. For each session that is + * {@code BUSY}, poll up to {@code timeoutNanos} for it to leave {@code BUSY}, then release it + * regardless. Non-busy sessions are released immediately. A failure releasing one session is + * logged and does not abort the rest. + * + *

CB-544: this is a {@link ReleaseCause#SHUTDOWN} release — the worker's pane is stopped + * (the process must end) but its worktree is preserved and its path logged. Shutdown is never + * a reason to delete a worker's only copy of its uncommitted work. A session still {@code BUSY} + * when the timeout expired is abandoned mid-turn and logged loudly so an operator can find its + * kept worktree. */ void drainAll(long timeoutNanos) { long deadline = System.nanoTime() + timeoutNanos; @@ -462,7 +517,7 @@ public final class SessionManager implements TurnListener { } } } - release(s.paneId()); + release(s.paneId(), ReleaseCause.SHUTDOWN); } catch (RuntimeException e) { log.warn("drain failed for pane={}; continuing with remaining sessions", s.paneId(), e); } diff --git a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java index 7b2bf9f..fd9ac91 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java @@ -11,6 +11,7 @@ import org.junit.jupiter.api.Test; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; import static org.junit.jupiter.api.Assertions.*; @@ -125,6 +126,42 @@ class WorktreeSessionManagerTest { assertTrue(sessions.get(paneId).isEmpty(), "released session is no longer retrievable"); } + @Test + void drainAllPreservesWorktreeOfIdleSession() { + FakeHerdr herdr = new FakeHerdr(); + FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt"); + SessionManager sessions = new SessionManager(workerService(herdr), worktrees); + WorkerSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-544", null)); + sessions.asPresence().markPresent(s.terminalId()); // READY (idle) + + sessions.drainAll(TimeUnit.MILLISECONDS.toNanos(100)); + + assertTrue(herdr.called("pane.close"), "shutdown drain still stops the worker pane"); + assertTrue(worktrees.removeCalls().isEmpty(), + "shutdown drain must NOT remove the worktree — it is the only copy of the work"); + assertTrue(sessions.roster().isEmpty(), "shutdown drain still deregisters the session"); + } + + @Test + void drainAllPreservesWorktreeOfSessionStillBusyAtTimeout() { + FakeHerdr herdr = new FakeHerdr(); + FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt"); + SessionManager sessions = new SessionManager(workerService(herdr), worktrees); + WorkerSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-544", null)); + String terminal = s.terminalId(); + sessions.asPresence().markPresent(terminal); + sessions.onDelivered(terminal); // BUSY, never completes → still BUSY when the timeout hits + + sessions.drainAll(TimeUnit.MILLISECONDS.toNanos(100)); + + assertTrue(herdr.called("pane.close"), "shutdown drain stops the worker pane"); + assertTrue(worktrees.removeCalls().isEmpty(), + "a session still BUSY at timeout must preserve its worktree unconditionally"); + assertTrue(sessions.roster().isEmpty(), "shutdown drain still deregisters the session"); + } + @Test void sharedTreeReleaseMakesNoWorktreesCalls() { FakeHerdr herdr = new FakeHerdr();