diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index 808f6fe..5371411 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -55,6 +55,9 @@ public final class Bridged { WorkspaceControl spaces = new WorkspaceControl(herdr); WorkerService workers = new WorkerService(agents, spaces, guard, cfg.workerProfiles(), cfg.defaultProfile(), System::getenv); + // CB-117: herdr keeps worker panes alive across a daemon restart, and their ids died with + // the previous process — reap those leaked orphans now, before we start serving. + workers.reapOrphanWorkers(); // Status-gated injector (CB-103): the single writer into workers, fed by a poller. // The blocking message endpoint (CB-104) is the producer; the poller is inert until then. diff --git a/bridged/src/main/java/dev/ltms/bridged/herdr/Agent.java b/bridged/src/main/java/dev/ltms/bridged/herdr/Agent.java index b84efb1..69753e3 100644 --- a/bridged/src/main/java/dev/ltms/bridged/herdr/Agent.java +++ b/bridged/src/main/java/dev/ltms/bridged/herdr/Agent.java @@ -15,6 +15,10 @@ import com.fasterxml.jackson.databind.JsonNode; * @param agentType detected agent kind, e.g. {@code "claude"}, or {@code null} before herdr * has detected it (the start-time shape) * @param status current lifecycle state + * @param name the unique label the agent was started with — for a bridge worker this is + * {@code claude---} (CB-117 keys orphan reaping on the + * nonce); {@code null} for agents the bridge did not start, e.g. a user's own + * Claude session */ public record Agent( String terminalId, @@ -23,7 +27,8 @@ public record Agent( String tabId, String sessionId, String agentType, - AgentStatus status) { + AgentStatus status, + String name) { /** Project a herdr {@code agent} node. Tolerates the start-time shape (no session yet). */ public static Agent from(JsonNode a) { @@ -42,6 +47,7 @@ public record Agent( a.path("tab_id").asText(null), sessionId, type, - AgentStatus.fromWire(a.path("agent_status").asText(null))); + AgentStatus.fromWire(a.path("agent_status").asText(null)), + a.path("name").asText(null)); } } diff --git a/bridged/src/main/java/dev/ltms/bridged/worker/WorkerService.java b/bridged/src/main/java/dev/ltms/bridged/worker/WorkerService.java index 171124e..1a159f2 100644 --- a/bridged/src/main/java/dev/ltms/bridged/worker/WorkerService.java +++ b/bridged/src/main/java/dev/ltms/bridged/worker/WorkerService.java @@ -19,6 +19,8 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * Spawns and lists worker sessions — the safe path from a delegation request to a @@ -42,6 +44,14 @@ public final class WorkerService { /** herdr rejects a duplicate agent {@code name}; we retry a bumped name this many times. */ private static final int NAME_RETRIES = 8; + /** + * A bridge-spawned worker label {@code claude---} (see + * {@link #startUniquelyNamed}); group 1 captures the 6-hex per-process {@code nonce}. The + * profile segment may itself contain {@code -}, so the nonce/seq are anchored at the tail. + * Names not matching this shape are not workers we started and are never reaped (CB-117). + */ + private static final Pattern WORKER_NAME = Pattern.compile("claude-.*-([0-9a-f]{6})-\\d+"); + private final AgentControl agents; private final WorkspaceControl spaces; private final SubscriptionGuard guard; @@ -271,6 +281,70 @@ public final class WorkerService { return agents.list(); } + /** + * Reap worker panes left behind by an earlier daemon process (CB-117). herdr keeps a worker's + * pane alive across a daemon restart by design, and that pane's id is held only by its + * spawner — so a worker whose owning process exited before issuing the matching teardown leaks + * with nothing tracking it (there is no registry; {@link #list()} only asks herdr). On boot we + * scan herdr for agents whose name matches our {@code claude---} scheme with + * a nonce other than this process's {@link #nameNonce}, and tear each one down (its pane + * and, via {@link #stop}, its now-empty dedicated tab). A current-nonce worker is ours and live, + * so it is left running; a user's own {@code claude} session carries no such name and is never + * touched. Best-effort: a failed listing, or a failure to stop any one worker, is logged and + * never aborts startup. + * + * @return the number of orphaned workers reaped + */ + public int reapOrphanWorkers() { + List all; + try { + all = agents.list(); + } catch (RuntimeException e) { + log.warn("orphan-worker reap skipped — agent.list failed: {}", e.getMessage()); + return 0; + } + int reaped = 0; + for (Agent a : all) { + if (!isForeignWorker(a.name(), nameNonce)) continue; + try { + stop(a.paneId()); + reaped++; + log.info("reaped orphan worker {} (pane={} tab={}) left by a prior daemon", + a.name(), a.paneId(), a.tabId()); + } catch (RuntimeException e) { + log.warn("could not reap orphan worker {} (pane={}): {}", + a.name(), a.paneId(), e.getMessage()); + } + } + if (reaped > 0) { + log.info("orphan-worker reap complete — {} stale worker(s) removed at startup", reaped); + } + return reaped; + } + + /** + * Whether {@code name} is a bridge worker started by a different process than + * {@code currentNonce} — the reap predicate (CB-117). True only for our naming scheme with a + * foreign nonce: a non-worker name (no match, e.g. a user session) or our own live nonce is + * excluded. Pure and package-private so the decision is unit-testable without herdr. + */ + static boolean isForeignWorker(String name, String currentNonce) { + String nonce = workerNonce(name); + return nonce != null && !nonce.equals(currentNonce); + } + + /** The 6-hex nonce embedded in a bridge worker name, or {@code null} if {@code name} isn't one. */ + static String workerNonce(String name) { + if (name == null) return null; + Matcher m = WORKER_NAME.matcher(name); + return m.matches() ? m.group(1) : null; + } + + /** This process's worker-name nonce (a label component only; exposed for reaper tests). */ + String nameNonce() { + return nameNonce; + } + /** * Tear a worker down by pane id: close the pane, and close its tab only when the * worker is that tab's sole occupant. The single-pane check is what makes this safe diff --git a/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java b/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java index f32bc39..675b44d 100644 --- a/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java +++ b/bridged/src/test/java/dev/ltms/bridged/herdr/FakeHerdr.java @@ -23,6 +23,7 @@ public final class FakeHerdr implements HerdrClient { public final List calls = new ArrayList<>(); private boolean healthy = true; private final List extraWorkspaces = new ArrayList<>(); + private final List extraAgents = new ArrayList<>(); private int agentNameTakenFor = 0; private int workerTabPaneCount = 1; private String paneCloseErrorCode = null; @@ -72,6 +73,19 @@ public final class FakeHerdr implements HerdrClient { } + /** + * Seed a named agent into {@code agent.list} (e.g. an orphaned worker for CB-117 reaper tests). + * The {@code name} carries the worker label the reaper keys on; {@code paneId}/{@code tabId} + * locate its pane for teardown. + */ + public FakeHerdr withAgent(String name, String terminalId, String paneId, String tabId) { + extraAgents.add(("{\"terminal_id\":\"%s\",\"agent\":\"claude\",\"agent_status\":\"idle\"," + + "\"name\":\"%s\",\"agent_session\":{\"kind\":\"id\",\"value\":\"sess-%s\"}," + + "\"workspace_id\":\"wQ\",\"tab_id\":\"%s\",\"pane_id\":\"%s\"}") + .formatted(terminalId, name, terminalId, tabId, paneId)); + return this; + } + /** Seed an additional workspace into {@code workspace.list} (e.g. a pre-existing worker space). */ public FakeHerdr withWorkspace(String id, String label) { extraWorkspaces.add(("{\"workspace_id\":\"%s\",\"label\":\"%s\",\"focused\":false," @@ -102,11 +116,12 @@ public final class FakeHerdr implements HerdrClient { {"workspace_id":"w1","label":"dev-mgnl","focused":true,"pane_count":7,"agent_status":"unknown"}, {"workspace_id":"w2","label":"ltms","focused":false,"pane_count":5,"agent_status":"done"}%s]}""") .formatted(extraWorkspaces.isEmpty() ? "" : "," + String.join(",", extraWorkspaces))); - case "agent.list" -> mapper.readTree(""" + case "agent.list" -> mapper.readTree((""" {"type":"agent_list","agents":[ {"terminal_id":"term_a","agent":"claude","agent_status":"idle", "agent_session":{"kind":"id","value":"sess-1111"}, - "workspace_id":"w2","tab_id":"w2:t7","pane_id":"w2:p7"}]}"""); + "workspace_id":"w2","tab_id":"w2:t7","pane_id":"w2:p7"}%s]}""") + .formatted(extraAgents.isEmpty() ? "" : "," + String.join(",", extraAgents))); case "agent.send" -> { if (agentSendErrorCode != null) { throw new HerdrException("herdr error [" + agentSendErrorCode + "]: agent.send failed", diff --git a/bridged/src/test/java/dev/ltms/bridged/worker/WorkerServiceTest.java b/bridged/src/test/java/dev/ltms/bridged/worker/WorkerServiceTest.java index 83ba30a..7bb290e 100644 --- a/bridged/src/test/java/dev/ltms/bridged/worker/WorkerServiceTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/worker/WorkerServiceTest.java @@ -110,4 +110,67 @@ class WorkerServiceTest { service(herdr, List.of("ccs", "ltms-local"), null).spawn("ltms-local", null, "/primary/project"); assertEquals("/primary/project", startCwd(herdr), "no explicit/config cwd → inherit the primary's"); } + + // --- CB-117 orphan reap: the pure predicate -------------------------------- + + @Test + void isForeignWorkerMatchesOurSchemeWithANonSelfNonce() { + assertTrue(WorkerService.isForeignWorker("claude-ollama-be09c2-2", "aaaaaa"), + "a bridge worker name with a different nonce is a prior daemon's orphan"); + assertTrue(WorkerService.isForeignWorker("claude-gx10-4127af-11", "aaaaaa"), + "profile and multi-digit seq are still parsed; foreign nonce ⇒ reap"); + } + + @Test + void isForeignWorkerSparesOurOwnLiveWorkersAndNonWorkers() { + assertFalse(WorkerService.isForeignWorker("claude-ollama-abcdef-3", "abcdef"), + "a worker with THIS process's nonce is ours and live — never reap it"); + assertFalse(WorkerService.isForeignWorker(null, "abcdef"), "an unnamed agent is not a worker"); + assertFalse(WorkerService.isForeignWorker("claude", "abcdef"), "a bare kind name is not a worker"); + assertFalse(WorkerService.isForeignWorker("my-repl", "abcdef"), "a user's own label is not a worker"); + assertFalse(WorkerService.isForeignWorker("claude-ollama-XYZ123-2", "abcdef"), + "a non-hex nonce does not match our scheme"); + } + + // --- CB-117 orphan reap: the wiring through stop() ------------------------- + + private static long paneCloseCount(FakeHerdr herdr, String paneId) { + return herdr.calls.stream() + .filter(c -> c.method().equals("pane.close")) + .filter(c -> paneId.equals(((Map) c.params()).get("pane_id"))) + .count(); + } + + @Test + void reapsAForeignOrphanButSparesOurOwnWorkerAndUserSessions() { + FakeHerdr herdr = new FakeHerdr(); + WorkerService svc = multiProfile(herdr); + herdr.withAgent("claude-ollama-be09c2-2", "term_orphan", "wQ:pF", "wQ:t8") // prior daemon's leak + .withAgent("claude-gx10-" + svc.nameNonce() + "-1", "term_mine", "wQ:pMine", "wQ:tMine"); // ours, live + // (the fake's default unnamed term_a stands in for a user's own Claude session) + + int reaped = svc.reapOrphanWorkers(); + + assertEquals(1, reaped, "exactly the one foreign-nonce orphan is reaped"); + assertEquals(1, paneCloseCount(herdr, "wQ:pF"), "the orphan's pane is closed"); + assertEquals(0, paneCloseCount(herdr, "wQ:pMine"), "our own live worker's pane is left running"); + assertEquals(0, paneCloseCount(herdr, "w2:p7"), "a user's own session is never touched"); + assertTrue(herdr.called("tab.close"), "the orphan's now-empty dedicated tab is closed too"); + } + + @Test + void reapCountsAnAlreadyGoneOrphanAsReaped() { + FakeHerdr herdr = new FakeHerdr().paneCloseFailsWith("pane_not_found"); + WorkerService svc = multiProfile(herdr); + herdr.withAgent("claude-ollama-0d856d-3", "term_gone", "wQ:pS", "wQ:tD"); + + assertEquals(1, svc.reapOrphanWorkers(), + "a pane that vanished between list and close is a successful reap, not a failure"); + } + + @Test + void reapIsSkippedWhenHerdrCannotBeListed() { + FakeHerdr herdr = new FakeHerdr().healthy(false); // agent.list throws + assertEquals(0, multiProfile(herdr).reapOrphanWorkers(), "a listing failure reaps nothing and does not throw"); + } }