Merge CB-576: release preserves a dirty worktree instead of deleting it
Verified by the lead onb525b0f: mvn -f bridged/pom.xml clean install, unpiped, in a scratch worktree — MVN_EXIT=0, Tests run: 686, Failures: 0, Errors: 0, BUILD SUCCESS. Review accepted the required-interface-method shape (no defaulting overload) and the decision to count untracked files as dirty — the work lost in the incident was a file that was never added. One blocking defect was found and fixed inb525b0f: hasUncommitted called git with no existence check, so a missing worktree threw WorktreeException from inside release() after registry.remove() but before notifyReleased() and launcher.stop(), orphaning the pane and stranding a blocked bridge_send caller. It now mirrors remove()'s already-gone tolerance. Waived on merge, tracked as follow-up: a SessionManager-level test that teardown completes when the worktree is gone, and the stronger fix behind it — reapIdle calls release() with no try/catch while drainAll wraps it, so any exception in that window aborts the whole reaping pass.
This commit was merged in pull request #53.
This commit is contained in:
@@ -167,6 +167,22 @@ public final class GitWorktrees implements Worktrees {
|
||||
exec("git", "-C", repoRoot, "worktree", "remove", "--force", worktreePath);
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean hasUncommitted(String worktreePath) {
|
||||
// A worktree that is already gone holds no work to lose, and it must not break teardown:
|
||||
// git -C <missing-dir> status exits non-zero and would throw where release() is mid-way
|
||||
// through stopping a pane. Mirror remove()'s already-gone tolerance by treating it as clean.
|
||||
Path p = Path.of(worktreePath);
|
||||
if (!Files.exists(p)) {
|
||||
log.debug("worktree {} already gone — nothing can be uncommitted", worktreePath);
|
||||
return false;
|
||||
}
|
||||
// No --untracked-files=no: the exact shape of the work lost in CB-576 was a new file
|
||||
// that was never added, so an untracked-only worktree is still dirty.
|
||||
String out = exec("git", "-C", worktreePath, "status", "--porcelain");
|
||||
return !out.isBlank();
|
||||
}
|
||||
|
||||
@Override
|
||||
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
||||
if (overlay == null || overlay.isEmpty()) {
|
||||
|
||||
@@ -201,6 +201,16 @@ public final class SessionManager implements TurnListener {
|
||||
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());
|
||||
}
|
||||
// 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
|
||||
|
||||
@@ -10,6 +10,18 @@ public interface Worktrees {
|
||||
/** git -C <repoRoot> worktree remove --force <path>. Idempotent (already-gone tolerated). */
|
||||
void remove(String repoRoot, String worktreePath);
|
||||
|
||||
/**
|
||||
* True when the worktree holds uncommitted changes the bridge cannot see: tracked
|
||||
* modifications, staged files, or untracked files. {@code git status --porcelain} is the
|
||||
* test; an empty result means clean. Callers use this to decide whether removing the
|
||||
* worktree would silently destroy a worker's only copy of its work.
|
||||
*
|
||||
* <p>An already-gone worktree is reported as clean (no throw), matching {@link #remove}'s
|
||||
* idempotent contract: a path that does not exist holds no work to lose, and must not break
|
||||
* a teardown that is mid-way through stopping the pane.
|
||||
*/
|
||||
boolean hasUncommitted(String worktreePath);
|
||||
|
||||
/** Copy each existing overlay path repoRoot→worktree; mark tracked ones --skip-worktree. */
|
||||
void overlayParity(String repoRoot, String worktreePath, List<String> overlay);
|
||||
|
||||
|
||||
@@ -29,6 +29,7 @@ public final class FakeWorktrees implements Worktrees {
|
||||
private final Set<String> existingPaths = ConcurrentHashMap.newKeySet();
|
||||
private final Set<String> trackedPaths = ConcurrentHashMap.newKeySet();
|
||||
private volatile RuntimeException addFailure;
|
||||
private volatile boolean dirty = false;
|
||||
private volatile String repoRoot = "/repo";
|
||||
private volatile String prefix = "/worktrees";
|
||||
|
||||
@@ -61,6 +62,12 @@ public final class FakeWorktrees implements Worktrees {
|
||||
return this;
|
||||
}
|
||||
|
||||
/** Mark the worktree dirty so {@link #hasUncommitted} reports true (simulates uncommitted work). */
|
||||
public FakeWorktrees withDirty(boolean dirty) {
|
||||
this.dirty = dirty;
|
||||
return this;
|
||||
}
|
||||
|
||||
@Override
|
||||
public String add(String repoRoot, String branch, String baseRef) {
|
||||
addCalls.add(new AddCall(repoRoot, branch, baseRef));
|
||||
@@ -77,6 +84,11 @@ public final class FakeWorktrees implements Worktrees {
|
||||
removeCalls.add(new RemoveCall(repoRoot, worktreePath));
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean hasUncommitted(String worktreePath) {
|
||||
return dirty;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
||||
List<String> copied = new java.util.ArrayList<>();
|
||||
|
||||
@@ -175,6 +175,46 @@ class GitWorktreesTest {
|
||||
assertTrue(Files.exists(Path.of(wt).resolve(".mcp.json")), ".mcp.json stub was dropped");
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-576. {@code hasUncommitted} must treat a freshly-provisioned worktree as clean, but a
|
||||
* worktree holding a brand-new, never-added file as dirty. The untracked-file-only shape is
|
||||
* exactly the work lost in the incident — a worker's draft that compiled but was never
|
||||
* committed because it stopped to ask its lead a question.
|
||||
*/
|
||||
@Test
|
||||
void anUntrackedOnlyWorktreeCountsAsDirty(@TempDir Path tmp) throws Exception {
|
||||
Path repo = initRepo(tmp.resolve("repo"));
|
||||
GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString());
|
||||
String wt = gitWorktrees.add(repo.toString(), "cb-576-u", "HEAD");
|
||||
|
||||
assertFalse(gitWorktrees.hasUncommitted(wt),
|
||||
"a freshly provisioned worktree must read as clean");
|
||||
|
||||
Files.writeString(Path.of(wt).resolve("brand-new.txt"), "draft that was never added\n");
|
||||
|
||||
assertTrue(gitWorktrees.hasUncommitted(wt),
|
||||
"an untracked-only file must count as dirty");
|
||||
|
||||
Files.writeString(Path.of(wt).resolve("README.md"), "edited tracked file\n");
|
||||
assertTrue(gitWorktrees.hasUncommitted(wt),
|
||||
"a tracked modification must also count as dirty");
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-576 review. {@code hasUncommitted} must tolerate a missing worktree exactly like
|
||||
* {@code remove}: an already-gone directory holds no work to lose, and throwing here would
|
||||
* break teardown — SessionManager.release() calls it before stopping the pane, so an
|
||||
* exception would orphan a live pane and skip the release notification (CB-516).
|
||||
*/
|
||||
@Test
|
||||
void hasUncommittedOnAMissingWorktreeReturnsFalseWithoutThrowing(@TempDir Path tmp) {
|
||||
GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString());
|
||||
String gone = tmp.resolve("wts").resolve("does-not-exist").toString();
|
||||
|
||||
assertFalse(gitWorktrees.hasUncommitted(gone),
|
||||
"a missing worktree is reported clean, not an error");
|
||||
}
|
||||
|
||||
/** All three protected configs are covered: each one present in a worktree is neutralized and hidden. */
|
||||
@Test
|
||||
void allThreeConfigsAreNeutralizedWhenPresent(@TempDir Path tmp) throws Exception {
|
||||
|
||||
@@ -6,10 +6,15 @@ import dev.ltms.bridged.guard.SubscriptionGuard;
|
||||
import dev.ltms.bridged.herdr.AgentControl;
|
||||
import dev.ltms.bridged.herdr.FakeHerdr;
|
||||
import dev.ltms.bridged.herdr.WorkspaceControl;
|
||||
import ch.qos.logback.classic.Level;
|
||||
import ch.qos.logback.classic.LoggerContext;
|
||||
import ch.qos.logback.classic.spi.ILoggingEvent;
|
||||
import ch.qos.logback.core.read.ListAppender;
|
||||
import dev.ltms.bridged.member.ClaudeCodeLauncher;
|
||||
import dev.ltms.bridged.msg.TestTurnTokens;
|
||||
import dev.ltms.bridged.peer.MemberRole;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
@@ -179,6 +184,49 @@ class WorktreeSessionManagerTest {
|
||||
assertTrue(sessions.get(paneId).isEmpty(), "released session is no longer retrievable");
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-576. A normal {@code COMPLETED} release whose worktree holds uncommitted work must NOT
|
||||
* remove it — {@code --force} would destroy the worker's only copy. The bridge cannot see
|
||||
* uncommitted files, so the worktree is preserved and the release logged at WARN naming the
|
||||
* path, the session, and the cause an operator needs to find the work.
|
||||
*/
|
||||
@Test
|
||||
void releasePreservesDirtyWorktreeAndLogsWarn() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt")
|
||||
.withDirty(true);
|
||||
SessionManager sessions = new SessionManager(workerService(herdr), worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-576", null));
|
||||
|
||||
LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory();
|
||||
ch.qos.logback.classic.Logger sessionLog =
|
||||
(ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.setContext(ctx);
|
||||
appender.start();
|
||||
sessionLog.addAppender(appender);
|
||||
sessionLog.setLevel(Level.WARN);
|
||||
try {
|
||||
sessions.release(s.paneId());
|
||||
|
||||
assertTrue(herdr.called("pane.close"), "release still tears the worker pane down");
|
||||
assertTrue(worktrees.removeCalls().isEmpty(),
|
||||
"a dirty worktree is never removed — it holds the only copy of the work");
|
||||
String warn = appender.list.stream()
|
||||
.filter(e -> e.getLevel().equals(Level.WARN))
|
||||
.map(ILoggingEvent::getFormattedMessage)
|
||||
.filter(m -> m.contains("dirty worktree"))
|
||||
.findFirst()
|
||||
.orElse("no dirty-release WARN logged");
|
||||
assertTrue(warn.contains(s.worktree()), "the WARN names the worktree path: " + warn);
|
||||
assertTrue(warn.contains(s.terminalId()), "the WARN names the session: " + warn);
|
||||
assertTrue(warn.contains("COMPLETED"), "the WARN names the release cause: " + warn);
|
||||
} finally {
|
||||
sessionLog.detachAppender(appender);
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void drainAllPreservesWorktreeOfIdleSession() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
|
||||
@@ -237,7 +237,7 @@ state never presents stop as the only action.
|
||||
| Release cause | Process action | Provisioned worktree |
|
||||
|---|---|---|
|
||||
| `SPAWN_ROLLBACK` before registration or delivery | Stop and clean up | Remove |
|
||||
| `COMPLETED` for `READY` or `DONE` without pending work, idle TTL, or successful context-cap completion | Stop | Remove under completed policy |
|
||||
| `COMPLETED` for `READY` or `DONE` without pending work, idle TTL, or successful context-cap completion | Stop | Remove only if clean; preserve a dirty worktree (CB-576) |
|
||||
| `NEVER_READY` | Stop | Preserve |
|
||||
| `GONE` | Best-effort stop | Preserve |
|
||||
| `TURN_FAILED` or lead abort while `BUSY` or `FAILED` | Stop | Preserve |
|
||||
|
||||
Reference in New Issue
Block a user