CB-117: reap orphaned worker panes on startup
herdr keeps worker panes alive across a daemon restart by design, and a worker's paneId is held only by its spawner — so a worker whose owning process exited before its DELETE leaks with nothing tracking it (there is no registry; list() only asks herdr). Observed as three idle claude-ollama panes left in the worker space from earlier runs. On boot, WorkerService.reapOrphanWorkers() scans herdr for agents whose name matches our claude-<profile>-<nonce>-<seq> scheme with a nonce other than this process's nameNonce, and tears each down (pane + its now-empty dedicated tab). A current-nonce worker is ours and live (spared); a user's own claude session carries no such name (untouched). Keyed on the nonce so it survives kill -9 and reaps a *previous* daemon's leaks — the actual case shutdown-hook reaping and an in-memory registry both miss. - Agent now projects herdr's 'name' (was dropped) so the reaper can key on it. - isForeignWorker/workerNonce are pure + package-private for unit testing. - FakeHerdr.withAgent seeds named agents into agent.list. - Wired best-effort into Bridged startup before serving. Closes lms/claude-bridge#1
This commit is contained in:
Dai Ha
@@ -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.
|
||||
|
||||
@@ -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-<profile>-<nonce>-<seq>} (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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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-<profile>-<nonce>-<seq>} (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 <em>by design</em>, 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-<profile>-<nonce>-<seq>} scheme with
|
||||
* a nonce <em>other</em> 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<Agent> 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 <em>different</em> 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 <em>only</em> when the
|
||||
* worker is that tab's sole occupant. The single-pane check is what makes this safe
|
||||
|
||||
@@ -23,6 +23,7 @@ public final class FakeHerdr implements HerdrClient {
|
||||
public final List<Call> calls = new ArrayList<>();
|
||||
private boolean healthy = true;
|
||||
private final List<String> extraWorkspaces = new ArrayList<>();
|
||||
private final List<String> 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",
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user