CB-576: release preserves a dirty worktree instead of deleting it #53
@@ -167,6 +167,22 @@ public final class GitWorktrees implements Worktrees {
|
|||||||
exec("git", "-C", repoRoot, "worktree", "remove", "--force", worktreePath);
|
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
|
@Override
|
||||||
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
||||||
if (overlay == null || overlay.isEmpty()) {
|
if (overlay == null || overlay.isEmpty()) {
|
||||||
|
|||||||
@@ -201,6 +201,16 @@ public final class SessionManager implements TurnListener {
|
|||||||
removed.paneId(), removed.terminalId(), removed.state(), cause);
|
removed.paneId(), removed.terminalId(), removed.state(), cause);
|
||||||
if (preserveWorktree && removed.worktree() != null) {
|
if (preserveWorktree && removed.worktree() != null) {
|
||||||
logPreservedForShutdown(removed);
|
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
|
// 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
|
// 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). */
|
/** git -C <repoRoot> worktree remove --force <path>. Idempotent (already-gone tolerated). */
|
||||||
void remove(String repoRoot, String worktreePath);
|
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. */
|
/** Copy each existing overlay path repoRoot→worktree; mark tracked ones --skip-worktree. */
|
||||||
void overlayParity(String repoRoot, String worktreePath, List<String> overlay);
|
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> existingPaths = ConcurrentHashMap.newKeySet();
|
||||||
private final Set<String> trackedPaths = ConcurrentHashMap.newKeySet();
|
private final Set<String> trackedPaths = ConcurrentHashMap.newKeySet();
|
||||||
private volatile RuntimeException addFailure;
|
private volatile RuntimeException addFailure;
|
||||||
|
private volatile boolean dirty = false;
|
||||||
private volatile String repoRoot = "/repo";
|
private volatile String repoRoot = "/repo";
|
||||||
private volatile String prefix = "/worktrees";
|
private volatile String prefix = "/worktrees";
|
||||||
|
|
||||||
@@ -61,6 +62,12 @@ public final class FakeWorktrees implements Worktrees {
|
|||||||
return this;
|
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
|
@Override
|
||||||
public String add(String repoRoot, String branch, String baseRef) {
|
public String add(String repoRoot, String branch, String baseRef) {
|
||||||
addCalls.add(new AddCall(repoRoot, branch, baseRef));
|
addCalls.add(new AddCall(repoRoot, branch, baseRef));
|
||||||
@@ -77,6 +84,11 @@ public final class FakeWorktrees implements Worktrees {
|
|||||||
removeCalls.add(new RemoveCall(repoRoot, worktreePath));
|
removeCalls.add(new RemoveCall(repoRoot, worktreePath));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public boolean hasUncommitted(String worktreePath) {
|
||||||
|
return dirty;
|
||||||
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
public void overlayParity(String repoRoot, String worktreePath, List<String> overlay) {
|
||||||
List<String> copied = new java.util.ArrayList<>();
|
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");
|
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. */
|
/** All three protected configs are covered: each one present in a worktree is neutralized and hidden. */
|
||||||
@Test
|
@Test
|
||||||
void allThreeConfigsAreNeutralizedWhenPresent(@TempDir Path tmp) throws Exception {
|
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.AgentControl;
|
||||||
import dev.ltms.bridged.herdr.FakeHerdr;
|
import dev.ltms.bridged.herdr.FakeHerdr;
|
||||||
import dev.ltms.bridged.herdr.WorkspaceControl;
|
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.member.ClaudeCodeLauncher;
|
||||||
import dev.ltms.bridged.msg.TestTurnTokens;
|
import dev.ltms.bridged.msg.TestTurnTokens;
|
||||||
import dev.ltms.bridged.peer.MemberRole;
|
import dev.ltms.bridged.peer.MemberRole;
|
||||||
import org.junit.jupiter.api.Test;
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.slf4j.LoggerFactory;
|
||||||
|
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
@@ -179,6 +184,49 @@ class WorktreeSessionManagerTest {
|
|||||||
assertTrue(sessions.get(paneId).isEmpty(), "released session is no longer retrievable");
|
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
|
@Test
|
||||||
void drainAllPreservesWorktreeOfIdleSession() {
|
void drainAllPreservesWorktreeOfIdleSession() {
|
||||||
FakeHerdr herdr = new FakeHerdr();
|
FakeHerdr herdr = new FakeHerdr();
|
||||||
|
|||||||
@@ -237,7 +237,7 @@ state never presents stop as the only action.
|
|||||||
| Release cause | Process action | Provisioned worktree |
|
| Release cause | Process action | Provisioned worktree |
|
||||||
|---|---|---|
|
|---|---|---|
|
||||||
| `SPAWN_ROLLBACK` before registration or delivery | Stop and clean up | Remove |
|
| `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 |
|
| `NEVER_READY` | Stop | Preserve |
|
||||||
| `GONE` | Best-effort stop | Preserve |
|
| `GONE` | Best-effort stop | Preserve |
|
||||||
| `TURN_FAILED` or lead abort while `BUSY` or `FAILED` | Stop | Preserve |
|
| `TURN_FAILED` or lead abort while `BUSY` or `FAILED` | Stop | Preserve |
|
||||||
|
|||||||
Reference in New Issue
Block a user