#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.
This commit is contained in:
@@ -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/<branch>} (CB-578 stage C). A
|
||||
* failure here must never escalate: the caller has already decided to preserve the worktree
|
||||
|
||||
@@ -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<Boolean> 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
|
||||
|
||||
Reference in New Issue
Block a user