CB-544: shutdown drain preserves worker worktrees (no data loss)
This commit is contained in:
@@ -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}.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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);
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user