From 667254df4709c9d5a69fb962079bc7688abc637b Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 14:12:49 +0700 Subject: [PATCH] #316: re-check worktree dirtiness after the pane stops, before removing it SessionManager.releaseRemoved read hasUncommitted() once, while the worker could still write, then used that stale boolean after launcher.stop() to authorise `git worktree remove --force`. The same stale read also gated trySnapshot, so a worker that wrote between the read and the stop lost its work with neither a preserve nor a snapshot. Add a second, best-effort hasUncommitted read immediately before the removal, taken only on the path that is actually about to delete something (never on a release that already decided to preserve, and never for SHUTDOWN, which preserves unconditionally). If the tree is now dirty, preserve it and attempt a fresh snapshot, since the original snapshot never ran when the pre-stop read said clean. A failing re-check also preserves, matching the existing CB-581 fail-safe rule. --- .../ltms/fleet/session/SessionManager.java | 43 ++++++++ .../fleet/session/SessionManagerTest.java | 98 +++++++++++++++++++ 2 files changed, 141 insertions(+) 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 0fec284..c1f3977 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -386,6 +386,31 @@ public final class SessionManager implements TurnListener { // 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) { + // fleetd #316: the `dirty` read above ran while the worker could still write to this + // worktree, so a stale `false` must not be trusted to authorise the --force removal + // below. Re-read the worktree's state one more time, right here — immediately before + // the one step that would destroy it, and only on the path that is actually about to + // do that (invariant 4: no second unconditional `git status` on a release that already + // decided to preserve). By now `launcher.stop` has returned, so this read reflects + // whatever the worker managed to write up to and including its teardown, not whatever + // it had written at release-start time. + if (dirtyImmediatelyBeforeRemoval(removed)) { + preserveWorktree = true; + // The pre-stop snapshot above never ran for this session (the pre-stop read said + // clean), so this is the only chance to get the newly-discovered work into + // refs/wip/* rather than leaving the on-disk preserve as the sole copy. Best-effort, + // like every other snapshot attempt — trySnapshot logs and swallows its own failure. + String lateSnapshotRef = trySnapshot(removed, cause); + log.warn("release {} preserves worktree {} for pane={} terminal={}: it reported " + + "clean before the pane stopped but dirty immediately before removal — the " + + "worker wrote to it during teardown, and --force removing it now would " + + "have destroyed that work{}", + cause, removed.worktree(), paneId, removed.terminalId(), + lateSnapshotRef == null ? "" : " (snapshotted to refs/wip/" + removed.branch() + + " commit=" + lateSnapshotRef + ")"); + } + } if (removed != null && !preserveWorktree && removed.worktree() != null) { // 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 @@ -404,6 +429,24 @@ public final class SessionManager implements TurnListener { } } + /** + * fleetd #316: the read that actually authorises {@code worktrees.remove}, taken with the + * worker's pane already stopped. Fails toward preserving (returns {@code true}) on any + * exception — the same rule the pre-stop check applies (CB-581): once we can no longer tell + * whether the worktree is dirty, preserving costs disk while deleting on a guess can destroy + * work that has no other copy. + */ + private boolean dirtyImmediatelyBeforeRemoval(MemberSession removed) { + try { + return worktrees.hasUncommitted(removed.worktree()); + } catch (RuntimeException e) { + log.warn("release could not re-check worktree {} for pane={} terminal={} immediately " + + "before removal; preserving it rather than risk destroying unsaved work: {}", + removed.worktree(), removed.paneId(), removed.terminalId(), e.toString()); + return true; + } + } + /** * Best-effort snapshot of a dirty worktree into {@code refs/wip/} (CB-578 stage C). A * failure here must never escalate: the caller has already decided to preserve the worktree 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 b9c4ad0..1b3e0e5 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -86,12 +86,32 @@ class SessionManagerTest { private volatile RuntimeException hasUncommittedFailure; private volatile RuntimeException snapshotFailure; private final java.util.concurrent.atomic.AtomicLong snapshotSeq = new java.util.concurrent.atomic.AtomicLong(); + /** fleetd #316: successive {@code hasUncommitted} answers, one per call, last one sticky + * once exhausted — models a worktree whose state changes between reads. Empty (the + * default) falls back to the plain {@link #dirty} flag, so every existing test using this + * fake keeps returning one fixed answer. */ + private final List dirtySequence = new java.util.concurrent.CopyOnWriteArrayList<>(); + private final java.util.concurrent.atomic.AtomicInteger hasUncommittedCalls = + new java.util.concurrent.atomic.AtomicInteger(); RecordingWorktrees dirty(boolean dirty) { this.dirty = dirty; return this; } + /** fleetd #316: return {@code answers[0]} on the first {@code hasUncommitted} call, + * {@code answers[1]} on the second, and so on; the last element repeats after that. */ + RecordingWorktrees dirtySequence(boolean... answers) { + for (boolean a : answers) { + dirtySequence.add(a); + } + return this; + } + + int hasUncommittedCallCount() { + return hasUncommittedCalls.get(); + } + RecordingWorktrees failHasUncommittedWith(RuntimeException e) { this.hasUncommittedFailure = e; return this; @@ -127,9 +147,13 @@ class SessionManagerTest { @Override public boolean hasUncommitted(String worktreePath) { + int call = hasUncommittedCalls.getAndIncrement(); if (hasUncommittedFailure != null) { throw hasUncommittedFailure; } + if (!dirtySequence.isEmpty()) { + return dirtySequence.get(Math.min(call, dirtySequence.size() - 1)); + } return dirty; } @@ -1207,6 +1231,80 @@ class SessionManagerTest { + "dirty check threw"); } + // --- fleetd #316: the dirty check must be re-taken after the worker is stopped, not trusted + // stale from before it ------------------------------------------------------------------------ + + @Test + void releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval() { + // Models the exact race #316 reports: hasUncommitted answers clean while the worker is + // still running (call 1), the worker then writes new work, and by the time release is + // about to force-remove the worktree a second read (call 2) would see it as dirty. Without + // the fix this test fails: release() never re-reads and force-removes the worktree anyway. + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees().dirtySequence(false, true); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-316a", null)); + + sessions.release(s.paneId()); + + assertTrue(worktrees.removeCalls().isEmpty(), + "a worktree that turned dirty between the pre-stop read and removal must be preserved"); + assertEquals(2, worktrees.hasUncommittedCallCount(), + "the fix re-reads hasUncommitted exactly once more, immediately before removal"); + } + + @Test + void releaseSnapshotsWorkFoundOnlyByTheLateRecheck() { + // #316's second half: the pre-stop dirty=false means trySnapshot never ran for this + // session, so the late-discovered work would otherwise have no refs/wip/* copy at all — + // only the on-disk preserve. The re-check path must snapshot it too. + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees().dirtySequence(false, true); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-316b", null)); + + sessions.release(s.paneId()); + + assertEquals(java.util.List.of(s.worktree()), worktrees.snapshotCalls(), + "the newly-dirty worktree is snapshotted even though the pre-stop check saw it clean"); + } + + @Test + void releaseStillRemovesAWorktreeThatStaysCleanOnTheLateRecheck() { + // The ordinary, non-racing case: nothing else changes behaviour when the second read + // agrees with the first. + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees().dirty(false); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-316c", null)); + + sessions.release(s.paneId()); + + assertEquals(java.util.List.of(s.worktree()), worktrees.removeCalls(), + "a worktree that is still clean on the late recheck is removed as before"); + } + + @Test + void releaseNeverReChecksAWorktreeAlreadyPreservedByTheFirstDirtyCheck() { + // Invariant 4 from #316: no second unconditional git status. A release that already + // decided to preserve (the ordinary CB-576 dirty path) must not pay for a second read. + 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-316d", null)); + + sessions.release(s.paneId()); + + assertEquals(1, worktrees.hasUncommittedCallCount(), + "a release that already preserves on the first read must not re-check before " + + "skipping the removal it was never going to do"); + assertTrue(worktrees.removeCalls().isEmpty()); + } + /** * 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