diff --git a/.gitignore b/.gitignore index 5a0e8dd..d8308c4 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,14 @@ # Settings backups inherit the env block — and secrets with it. .claude/settings.local.json.bak* +# The default profile parityOverlay copies these primary→worktree, so they appear in EVERY worker +# worktree. Two reasons they must be ignored. They hold environment values, which is reason enough. +# And since CB-576 a release preserves any worktree that `git status --porcelain` calls dirty — +# untracked files included, deliberately. An untracked overlay file would therefore make every +# COMPLETED release preserve its worktree, and worktrees would pile up with no error to notice. +.env +.envrc + # Daemon runtime artefacts. bridged appends its log wherever it is launched from, so both the # repo root and bridged/ collect one; neither belongs in git. bridged.out diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index 6ff7fc5..56e6051 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -172,7 +172,21 @@ public record BridgedConfig( * @param cwd fixed working directory for this profile's workers (CB-112 "told otherwise"); * {@code null}/blank → inherit the primary's cwd, else the daemon's * @param parityOverlay repo-relative paths copied primary→worktree for config parity; null/empty - * defaults to a sensible set of local config files + * defaults to a sensible set of local config files. + *

Every overlay path must be gitignored or tracked-and-skipped. + * CB-576 made {@code release()} preserve a worktree that {@code git status + * --porcelain} reports as dirty, and it deliberately counts untracked files — + * the work lost in CB-576 was a new file nobody had added. So an overlay path + * that is neither gitignored nor tracked lands in every worktree as an + * untracked file, makes every {@code COMPLETED} release preserve, and + * worktrees then accumulate with no error anywhere. + *

Checked on 2026-08-15 (CB-581): inert as configured. Tracked overlay + * files carry {@code --skip-worktree} so {@code --porcelain} cannot see them, + * {@code bridged.yaml} is gitignored, and the default pair {@code .env} / + * {@code .envrc} does not exist in this repo. Note the default applies to + * every profile, so creating either file at the repo root is enough + * to make it live. Add a new overlay path to {@code .gitignore} in the same + * change that adds it here. * @param gitTokenEnv name of the host env var holding the git-forge API token; when set, its * value is injected as {@code GITEA_TOKEN} so the worker can open its own PR * at checkpoint (CB-302). {@code null}/blank ⇒ no token is injected diff --git a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java index 0a7563c..ad94c60 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java @@ -197,27 +197,44 @@ public final class SessionManager implements TurnListener { MemberSession removed = registry.remove(paneId); boolean preserveWorktree = cause == ReleaseCause.SHUTDOWN; if (removed != null) { - memberLifecycle.released(removed.terminalId()); - log.debug("releasing session pane={} terminal={} state={} cause={}", - 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. + try { + memberLifecycle.released(removed.terminalId()); + log.debug("releasing session pane={} terminal={} state={} cause={}", + 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()); + } + } catch (RuntimeException e) { + // CB-581: hasUncommitted shells out to `git status` and can throw on a non-zero + // exit. We can no longer tell whether the worktree holds uncommitted work, so fail + // toward the safe answer and preserve it — deleting on a guess can destroy work + // that has no other copy (CB-576), while keeping it on a false alarm only costs + // disk. The exception must not propagate: the pane still has to stop below. 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()); + log.warn("release {} could not tell whether worktree {} for pane={} terminal={} has " + + "uncommitted changes; preserving it rather than risk destroying unsaved work: {}", + cause, removed.worktree(), removed.paneId(), removed.terminalId(), e.toString()); + } finally { + // CB-516/CB-581: a send still waiting on this worker can never be answered now, no + // matter what happened above. 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()); } - // 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()); } + // CB-581: the pane must always stop, even if the dirty check above threw. A session removed + // 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) { worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree()); @@ -539,8 +556,15 @@ public final class SessionManager implements TurnListener { log.debug("reaping idle session terminal={} pane={}: idle {}s exceeds the {}s ttl", s.terminalId(), s.paneId(), TimeUnit.NANOSECONDS.toSeconds(idleNanos), TimeUnit.NANOSECONDS.toSeconds(idleTtlNanos)); - release(s.paneId()); - reaped++; + // CB-581: one session that fails to release must not abort the whole reaping pass — + // match drainAll's per-session try/catch so the rest of the roster still gets reaped. + try { + release(s.paneId()); + reaped++; + } catch (RuntimeException e) { + log.warn("reap failed for pane={} terminal={} worktree={}; continuing with " + + "remaining sessions", s.paneId(), s.terminalId(), s.worktree(), e); + } } } return reaped; diff --git a/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java b/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java index d8d9009..41ea200 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/SessionManagerTest.java @@ -46,6 +46,82 @@ class SessionManagerTest { return sessionManager(herdr, clock, 0); } + private SessionManager sessionManager(FakeHerdr herdr, Worktrees worktrees) { + return sessionManager(herdr, worktrees, System::nanoTime); + } + + private SessionManager sessionManager(FakeHerdr herdr, Worktrees worktrees, LongSupplier clock) { + BridgedConfig.Profile cfg = new BridgedConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "BRIDGED_WORKER_TOKEN", + List.of("ccs", "ltms-local"), "tab", "bridged-workers", + "worker: {profile} #{n}", null, null, null); + ClaudeCodeLauncher workers = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null); + return new SessionManager(workers, worktrees, clock); + } + + /** + * CB-581: a {@link Worktrees} test double whose {@code hasUncommitted} and {@code remove} can + * be told to throw, so {@link SessionManager#release} can be exercised against exactly the + * failure {@code GitWorktrees} produces when {@code git status}/{@code git worktree remove} + * exits non-zero. + */ + private static final class RecordingWorktrees implements Worktrees { + private final List removeCalls = new java.util.ArrayList<>(); + private final java.util.Set failRemoveFor = new java.util.HashSet<>(); + private volatile boolean dirty = false; + private volatile RuntimeException hasUncommittedFailure; + + RecordingWorktrees dirty(boolean dirty) { + this.dirty = dirty; + return this; + } + + RecordingWorktrees failHasUncommittedWith(RuntimeException e) { + this.hasUncommittedFailure = e; + return this; + } + + RecordingWorktrees failRemoveFor(String worktreePath) { + failRemoveFor.add(worktreePath); + return this; + } + + @Override + public String add(String repoRoot, String branch, String baseRef) { + return "/wt/" + branch.replace('/', '_'); + } + + @Override + public void remove(String repoRoot, String worktreePath) { + if (failRemoveFor.contains(worktreePath)) { + throw new WorktreeException("simulated remove failure for " + worktreePath); + } + removeCalls.add(worktreePath); + } + + @Override + public boolean hasUncommitted(String worktreePath) { + if (hasUncommittedFailure != null) { + throw hasUncommittedFailure; + } + return dirty; + } + + @Override + public void overlayParity(String repoRoot, String worktreePath, List overlay) { + } + + @Override + public String repoRoot(String cwd) { + return "/repo"; + } + + List removeCalls() { + return List.copyOf(removeCalls); + } + } + private SessionManager sessionManager(FakeHerdr herdr, LongSupplier clock, int contextCap) { return sessionManager(herdr, clock, contextCap, false); } @@ -525,4 +601,172 @@ class SessionManagerTest { "a listener failure must never prevent the teardown it is reacting to"); assertTrue(sessions.get(s.paneId()).isEmpty(), "and the session is still deregistered"); } + + // --- CB-581: a throw inside release() must not orphan the pane or abort reapIdle ----------- + + @Test + void releasePreservesWorktreeWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581a", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + try { + assertDoesNotThrow(() -> sessions.release(s.paneId()), + "a throwing dirty check must not abort the release"); + + assertTrue(worktrees.removeCalls().isEmpty(), + "the worktree is preserved when its dirty state cannot be determined"); + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains(s.worktree())) + .findFirst() + .orElse("no warn logged naming the worktree"); + assertTrue(warn.contains(s.paneId()), "the WARN names the pane: " + warn); + assertTrue(warn.contains(s.terminalId()), "the WARN names the terminal: " + warn); + } finally { + sessionLog.detachAppender(appender); + } + } + + @Test + void releaseStillStopsThePaneWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581b", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + sessions.release(s.paneId()); + + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_1"), + "the pane is stopped exactly once even though the dirty check threw"); + } + + @Test + void releaseStillNotifiesTheListenerWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + java.util.List released = new java.util.concurrent.CopyOnWriteArrayList<>(); + sessions.onRelease(released::add); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581c", null)); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + sessions.release(s.paneId()); + + assertEquals(java.util.List.of(s.terminalId()), released, + "a blocked caller must still be told the terminal was released, even though the " + + "dirty check threw"); + } + + @Test + void reapIdleSurvivesOneSessionThatFailsToRelease() { + long[] clock = {0}; + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees, () -> clock[0]); + MemberSession a = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581d", null)); + MemberSession b = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581e", null)); + MemberSession c = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581f", null)); + sessions.asPresence().markPresent(a.terminalId()); + sessions.asPresence().markPresent(b.terminalId()); + sessions.asPresence().markPresent(c.terminalId()); + // The middle session's worktree removal fails — release() propagates that, so this is the + // one call reapIdle's per-session guard must survive without skipping the rest of the pass. + worktrees.failRemoveFor(b.worktree()); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + int reaped; + try { + clock[0] = 100; + reaped = sessions.reapIdle(10); + + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains(b.paneId())) + .findFirst() + .orElse("no reap-failure WARN logged"); + assertTrue(warn.contains(b.terminalId()), "the WARN names the failed session's terminal: " + warn); + assertTrue(warn.contains(b.worktree()), "the WARN names the failed session's worktree: " + warn); + } finally { + sessionLog.detachAppender(appender); + } + + assertEquals(2, reaped, "the middle session's failure is logged, not counted as reaped"); + assertTrue(sessions.get(a.paneId()).isEmpty(), "the first session is still released"); + assertTrue(sessions.get(c.paneId()).isEmpty(), "the third session is still released"); + assertTrue(sessions.get(b.paneId()).isEmpty(), + "the middle session is still deregistered even though its worktree removal threw"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_1"), "the first pane is stopped"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_2"), + "the middle pane is still stopped even though its worktree removal failed"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_3"), "the third pane is stopped"); + } + + @Test + void unchangedRegressionCleanCompletedReleaseStillRemovesTheWorktree() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581g", null)); + + sessions.release(s.paneId()); + + assertEquals(List.of(s.worktree()), worktrees.removeCalls(), + "COMPLETED release of a clean worktree still removes it"); + } + + @Test + void unchangedRegressionDirtyCompletedReleaseStillPreservesTheWorktree() { + 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-581h", null)); + + sessions.release(s.paneId()); + + assertTrue(worktrees.removeCalls().isEmpty(), + "COMPLETED release of a dirty worktree still preserves it"); + } + + @Test + void unchangedRegressionShutdownDrainStillPreservesTheWorktree() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-581i", null)); + sessions.asPresence().markPresent(s.terminalId()); + + sessions.drainAll(TimeUnit.MILLISECONDS.toNanos(100)); + + assertTrue(worktrees.removeCalls().isEmpty(), "SHUTDOWN drain still preserves the worktree"); + } } diff --git a/docs/M4-Fleet-Health.md b/docs/M4-Fleet-Health.md index c605172..0ea6043 100644 --- a/docs/M4-Fleet-Health.md +++ b/docs/M4-Fleet-Health.md @@ -1,7 +1,9 @@ # M4 - Fleet health, recovery, routing, and capacity **Status:** Design accepted on 2026-08-15. CB-573 part 1 has shipped the classification model and -the `bridge_list` capacity view; the remaining M4 units are not yet shipped. +the `bridge_list` capacity view; the remaining M4 units are not yet shipped. See +[Unit 2 — what has landed so far](#unit-2---what-has-landed-so-far) before planning Unit 2 work: +some of its criteria were met by separate CB tickets, and one of them contradicts the unit text. **Scope:** Fleet evidence, safe mechanical repair, lead routing, capacity reporting, and optional human notification. **Grounded in:** `health/FleetHealth`, `health/PaneBudget`, `inject/StatusPoller`, @@ -799,6 +801,39 @@ Acceptance criteria: 21. Tests cover both real traces, all repair refusals, clipping, explicit-reply and next-turn races, restart without capture, concurrent send and release, and preserved discovery after restart. +#### Unit 2 - what has landed so far + +Checked against `main` at `e09cac6` on 2026-08-15. Unit 2 was written as one block, but parts of it +have since been built by separate CB tickets. Read this before planning the rest, or that work gets +done twice. + +The check was a symbol survey of `bridged/src/main/java` plus the merge history. It tells you whether +the machinery exists at all. It is **not** a line-by-line audit of whether each criterion is fully +met, and I did not run one. + +| Criterion | Marker searched for | Found in main source | Reading | +|---|---|---|---| +| 1 | `TurnToken` | 8 files | **Done** — unit 2a, merged as `fec284e`. Criterion 1 was corrected first; see the note under it. | +| 2-5, 9 | `REPAIRED` | 0 files | Not started. The whole guarded-repair path is absent. | +| 6, 7, 10 | `reconcileLostBoundary` | 0 files | Not started. | +| 12 | CB-568 failure operation | via CB-580 | **Partial.** CB-580 (`0af902e`) routes `GONE` and `NEVER_READY` into the one idempotent target-wide failure. I did not check that release and abnormal stop go through the same call. | +| 14 | `DELEGATION_ORPHANED` | 3 files | **Partial.** The health state exists. The teardown-invariant check that creates it, and the retry rule, do not. | +| 15 | `SPAWN_ROLLBACK` | 0 files | **Contradicted — see below.** | +| 16 | — | — | Partial at best. CB-576 made release preserve a dirty worktree; whether explicit stop is state-aware is not checked. | +| 17, 18 | `preservedWorktrees` | 0 files | Not started. No manifest, and no lead-only `bridge_list` field. | +| 19 | `WORK_PRODUCT_AT_RISK` | 0 files | Not started. | + +**Criterion 15 no longer matches the code, and the code is right.** It says "normal `COMPLETED` +remove worktrees". Since CB-576 (`500bfa2`) that is false on purpose: a `COMPLETED` release now +preserves the worktree when it still holds uncommitted work, because deleting it destroys work +nobody can get back. CB-576 was filed after exactly that loss. CB-581 goes further — if the +dirty-check itself fails, the worktree is preserved rather than removed, since "we could not tell" +must not be treated as "it is clean". + +So criterion 15 should be rewritten as: `SPAWN_ROLLBACK` and a `COMPLETED` release with a **clean** +worktree remove it; abnormal causes, shutdown, a dirty worktree, and a failed dirty-check all +preserve it. `SPAWN_ROLLBACK` itself does not exist yet. + ### Unit 3 - Typed inbox and member routing Scope: semantic record, AMQP migration, both adapters, member routing, polling, and member health in