diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java index 6a64b50..674fdef 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java @@ -5,6 +5,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.Locale; import java.util.Map; @@ -53,13 +54,25 @@ import java.util.function.Supplier; * daemon and found again by this scan. The trust direction above is unaffected — fleetd writing a * name for a lead it just started is not a pane promoting itself — but staleness becomes * real: a label left behind by a session that has since died would read as a live lead forever. - * This scanner does not solve that (its job is naming, and a stale name costs nothing here); the - * launcher does, by requiring a running agent in the tab before it counts the lead as live. If you - * ever make a decision that removes something based on this map, add the same check. * The remaining hazard is an operator one — a worker {@code tabLabel} template that * happens to start with the same prefix would promote the whole fleet — and that is refused at * startup by {@code FleetConfig.validateLeadTabPrefixes} rather than documented here. * + *

fleetd #359 — the staleness check this class used to skip. This used to say + * "a stale name costs nothing here" and leave liveness to {@code LeadLauncher}, on the theory that + * naming and removing are different decisions. That was wrong: {@code dev.ltms.fleet.msg.LeadCoordLoop} + * makes exactly the kind of removal decision the old javadoc warned about, by reading this map to + * pick which pane a peer message goes into — and a stale entry there is not free. On a host where + * {@code fleetd} had restarted more than once, a labelled-but-dead tab from a previous life was + * reported right alongside the live one; {@code LeadCoordLoop.resolveLocalLead()} saw more than one + * candidate and refused to guess (safe), but the fix the daemon's own WARN suggests — name a lead + * after {@code coordinator.selfId} — stops being safe once two tabs can share a label: step 1 of + * that resolution picks whichever matching entry it finds first, which can be the dead one, and + * typing a peer's message into a dead shell does not fail — it is silently gone instead of merely + * held. {@link #scan()} now cross-checks every labelled tab against {@code agent.list} (the same + * signal {@code LeadLauncher.countLeads} already trusts for the same purpose) and drops any tab + * with no agent running in it, so a dead tab is never in the map for a caller to pick at all. + * *

Caching. {@link #get()} is on the request path (every resolve), so the scan * is TTL-cached and a stale-but-valid map is preferred to a herdr round-trip. A failed scan keeps * the previous answer instead of emptying it — a herdr hiccup must not silently demote a live lead @@ -141,7 +154,7 @@ public final class LeadTabScanner implements Supplier> { return cached; } - /** One full pass: labelled tabs → their panes → those panes' terminals. */ + /** One full pass: labelled tabs → live agents in them → those panes' terminals. */ private Map scan() { Map nameByTab = new LinkedHashMap<>(); for (JsonNode w : herdr.call("workspace.list").path("workspaces")) { @@ -158,15 +171,29 @@ public final class LeadTabScanner implements Supplier> { } } + if (nameByTab.isEmpty()) { + return Map.of(); + } + + // fleetd #359: a labelled tab is only a lead when herdr also reports a running agent in + // it — the same liveness signal LeadLauncher.countLeads trusts for the identical purpose. + // Without this, a tab left behind by a session that has since died reads as live forever. + Set tabsWithAgent = new HashSet<>(); + for (JsonNode a : herdr.call("agent.list").path("agents")) { + String tabId = a.path("tab_id").asText(null); + if (tabId != null) { + tabsWithAgent.add(tabId); + } + } + Map byTerminal = new LinkedHashMap<>(); - if (!nameByTab.isEmpty()) { - // One pane.list for every tab: panes carry tab_id, so the join is local. - for (JsonNode p : herdr.call("pane.list", Map.of()).path("panes")) { - String name = nameByTab.get(p.path("tab_id").asText(null)); - String terminal = p.path("terminal_id").asText(null); - if (name != null && terminal != null && !terminal.isBlank()) { - byTerminal.put(terminal, name); - } + // One pane.list for every tab: panes carry tab_id, so the join is local. + for (JsonNode p : herdr.call("pane.list", Map.of()).path("panes")) { + String tabId = p.path("tab_id").asText(null); + String name = nameByTab.get(tabId); + String terminal = p.path("terminal_id").asText(null); + if (name != null && terminal != null && !terminal.isBlank() && tabsWithAgent.contains(tabId)) { + byTerminal.put(terminal, name); } } return Collections.unmodifiableMap(byTerminal); diff --git a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java index a852eac..0bdc303 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java @@ -12,9 +12,11 @@ import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import dev.ltms.fleet.peer.PeerLauncher; /** @@ -44,8 +46,16 @@ import dev.ltms.fleet.peer.PeerLauncher; * privilege escalation (the tab label never granted anything a pane could take for itself; see that * class's javadoc), but staleness: a label left behind by a crashed session would otherwise * read as a live lead forever, and the lead would never be relaunched. So a lead counts as live only - * when herdr also reports a running agent in that tab — see {@link #liveLeads}. A labelled + * when herdr also reports a running agent in that tab — see {@link #countLeads}. A labelled * tab with no agent in it is not a lead. + * + *

fleetd #359 — a stale label used to pile up, not just mislead. Finding "not + * live" here used to mean only one thing: launch another. The old label was left exactly where it + * was, so a daemon that restarted enough times — or hit one herdr read that missed a genuinely + * running agent — accumulated one more identically-labelled dead tab per occurrence, and + * {@code LeadTabScanner} (before its own #359 fix) reported every one of them as a lead. Now + * {@link #ensureLeads()} closes a name's dead tabs in the same pass that decides whether to launch, + * so at most one tab ever carries a given lead's label once the reconcile after any restart has run. */ public final class LeadLauncher { @@ -79,9 +89,9 @@ public final class LeadLauncher { return 0; } - Map live; + Map live; try { - live = liveLeads(leaders); + live = countLeads(leaders); } catch (HerdrException e) { // Counting is the whole safety mechanism against double-spawning. If we cannot count, we // must not guess — spawning a second orchestrator is worse than starting none. @@ -93,9 +103,27 @@ public final class LeadLauncher { for (Map.Entry e : leaders.entrySet()) { String name = e.getKey(); FleetConfig.Leader lead = e.getValue(); - int running = live.getOrDefault(name, 0); + LeadCount state = live.getOrDefault(name, LeadCount.NONE); + int running = state.running(); int wanted = lead.instances(); + // fleetd #359: a tab labelled for this lead but hosting no running agent is debris from + // a previous life (a crash, or this exact reconcile running once too many times across a + // restart) — close it now rather than leaving it next to whatever gets (re)launched + // below. Left alone, every restart that finds "0 live" for any reason adds one more of + // these forever, and each one is a candidate LeadCoordLoop.resolveLocalLead() can pick + // when several share a label — including, per its own advice, a dead one. + for (String deadTabId : state.deadTabIds()) { + log.info("lead '{}': closing stale tab {} — labelled '{}' but no agent is running in it", + name, deadTabId, lead.tabLabel()); + try { + spaces.closeTab(deadTabId); + } catch (RuntimeException cleanup) { + log.warn("could not close stale tab {} for lead '{}': {}", + deadTabId, name, cleanup.getMessage()); + } + } + if (running >= wanted) { log.info("lead '{}': {} live, {} wanted — nothing to start", name, running, wanted); continue; @@ -126,18 +154,27 @@ public final class LeadLauncher { } /** - * How many live leads exist per configured name: a running agent in a tab labelled with that - * lead's exact {@code tab} (CB-579). Member workspaces are excluded, exactly as the scanner - * excludes them: a member must not be counted as a lead because it happens to sit in a matching - * tab. + * How many live leads exist per configured name, and which of that name's labelled tabs are + * not live: a running agent in a tab labelled with that lead's exact {@code tab} + * (CB-579). Member workspaces are excluded, exactly as the scanner excludes them: a member must + * not be counted as a lead because it happens to sit in a matching tab. * *

There used to be a second path here — a running agent on the terminal a * {@code fleet.leaders..terminal} pin named, for a lead opened and pinned by hand. That * pin is retired: {@code tab} is now the only field identity depends on, and {@link Agent} * already carries {@link Agent#tabId()} directly, so a hand-opened lead is found the same way an * auto-launched one is — by labelling its tab to match. + * + *

fleetd #359: {@code deadTabIds} exists so {@link #ensureLeads()} can close a labelled tab + * with nothing running in it instead of leaving it next to a freshly (re)launched replacement — + * without this, every occasion this count comes back low (a real crash, or a herdr read that + * simply missed a still-running agent) leaves one more dead tab behind, forever. */ - private Map liveLeads(Map leaders) { + private record LeadCount(int running, List deadTabIds) { + static final LeadCount NONE = new LeadCount(0, List.of()); + } + + private Map countLeads(Map leaders) { // A lead and the members share ONE workspace now (the operator asked for a single "session" // with many tabs), so a workspace can no longer be excluded wholesale — the lead lives in the // member workspace by design. The sole discriminator is the exact tab label: a lead carries @@ -157,14 +194,29 @@ public final class LeadLauncher { } } + Set liveTabIds = new LinkedHashSet<>(); Map counts = new LinkedHashMap<>(); for (Agent a : agents.list()) { String name = nameByTab.get(a.tabId()); if (name != null) { counts.merge(name, 1, Integer::sum); + liveTabIds.add(a.tabId()); } } - return counts; + + Map> deadTabsByName = new LinkedHashMap<>(); + nameByTab.forEach((tabId, name) -> { + if (!liveTabIds.contains(tabId)) { + deadTabsByName.computeIfAbsent(name, k -> new ArrayList<>()).add(tabId); + } + }); + + Map out = new LinkedHashMap<>(); + for (String name : leaders.keySet()) { + out.put(name, new LeadCount(counts.getOrDefault(name, 0), + deadTabsByName.getOrDefault(name, List.of()))); + } + return out; } /** diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java index baea30c..e395220 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java @@ -6,6 +6,7 @@ import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -35,6 +36,12 @@ class LeadTabScannerTest { final Map tabs = new LinkedHashMap<>(); /** pane_id → [tab_id, terminal_id]. */ final Map panes = new LinkedHashMap<>(); + /** + * tab_id → whether herdr reports a running agent there. Defaults to {@code true} for every + * tab that has a pane, so every existing fixture keeps meaning "a live lead" unless a test + * says otherwise via {@link #deadAgent}. + */ + final Set deadTabs = new LinkedHashSet<>(); int calls; boolean failing; @@ -53,6 +60,12 @@ class LeadTabScannerTest { return this; } + /** Mark {@code tabId} as labelled but agent-less — a dead lead's leftover tab (fleetd #359). */ + TopologyHerdr deadAgent(String tabId) { + deadTabs.add(tabId); + return this; + } + @Override public JsonNode call(String method, Object params) { calls++; @@ -83,6 +96,19 @@ class LeadTabScannerTest { .formatted(id, p[0], p[1]))); return read("{\"panes\":[%s]}".formatted(String.join(",", items))); } + case "agent.list" -> { + // One agent per distinct tab that has a pane and isn't marked dead — mirrors + // AgentControl.list()'s "agents" shape closely enough for the scanner's join, + // which only reads tab_id off each entry. + Set seen = new LinkedHashSet<>(); + panes.forEach((paneId, p) -> { + String tabId = p[0]; + if (!deadTabs.contains(tabId) && seen.add(tabId)) { + items.add("{\"tab_id\":\"%s\"}".formatted(tabId)); + } + }); + return read("{\"agents\":[%s]}".formatted(String.join(",", items))); + } default -> throw new AssertionError("unexpected herdr call: " + method); } } @@ -208,6 +234,42 @@ class LeadTabScannerTest { scanner(herdr, twoLeadsConfigured(), new AtomicLong()).get().get("term_opus_split")); } + // ── fleetd #359: a labelled tab is only a lead when something is running in it ───────────────── + + /** + * The core invariant this ticket restores: {@code get()} must never report a terminal for a + * lead whose pane no longer runs an agent. Before this fix the scanner joined labelled tabs to + * panes with no liveness check at all, so a tab left behind by a crashed/relaunched lead (see + * {@code LeadLauncher}'s own staleness handling) was reported as live forever — which is exactly + * what let duplicate lead tabs make {@code LeadCoordLoop.resolveLocalLead()} permanently unable + * to pick one. Mutate this away (drop the {@code agent.list} cross-check in {@link + * LeadTabScanner#scan()}) and this test must fail. + */ + @Test + void aLabelledTabWithNoRunningAgentIsNotReported() { + TopologyHerdr herdr = twoLeads().deadAgent("w1:t1"); // opus-5.0's tab is labelled but dead + + Map leads = scanner(herdr, twoLeadsConfigured(), new AtomicLong()).get(); + + assertFalse(leads.containsKey("term_opus"), + "a labelled tab with no running agent must never be reported as a live lead"); + assertEquals("gpt-sol-5.6", leads.get("term_gpt"), + "the other, genuinely live lead must be unaffected"); + } + + /** + * The other direction, pinned separately so a fix cannot satisfy the test above by simply + * returning nothing: a labelled tab that DOES have a running agent must still be reported. A + * scanner that always comes back empty is worse than the bug it fixes. + */ + @Test + void aLabelledTabWithARunningAgentIsStillReported() { + Map leads = scanner(twoLeads(), twoLeadsConfigured(), new AtomicLong()).get(); + + assertEquals("opus-5.0", leads.get("term_opus")); + assertEquals("gpt-sol-5.6", leads.get("term_gpt")); + } + /** * CB-579 acceptance (6): this is the bug the ticket closes. A stale pin used to be merged back * over every scan and never expire; now a scan is the whole answer, so a lead whose tab is gone diff --git a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java index d0afddd..0da75a1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java @@ -9,6 +9,7 @@ import org.junit.jupiter.api.Test; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import static org.junit.jupiter.api.Assertions.*; @@ -106,6 +107,7 @@ class LeadLauncherTest { assertEquals(0, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); assertFalse(herdr.called("agent.start"), "the live lead must not be duplicated"); + assertFalse(herdr.called("tab.close"), "a labelled tab WITH a live agent must never be closed"); } /** @@ -122,6 +124,51 @@ class LeadLauncherTest { "a stale label is not a lead; the lead must be relaunched"); } + /** + * fleetd #359 — the growth bug itself. Relaunching used to leave the stale tab exactly where it + * was; over any number of restarts that finds "0 live" it accumulates one dead tab per + * occurrence, and each one is a candidate {@code LeadCoordLoop.resolveLocalLead()} can pick when + * it later sees several tabs sharing one label. The fix must close the dead tab as part of the + * same reconcile that decides to relaunch — mutate this away (drop the {@code tab.close} call in + * {@code LeadLauncher.ensureLeads()}) and this test must fail. + */ + @Test + void aStaleLabelledTabIsClosedWhenTheLeadIsRelaunched() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "fleet") + .withTab("wL", "wL:t1", "lead: opus"); // label only — the debris from a previous life + + assertEquals(1, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); + + assertTrue(herdr.called("tab.close"), "the dead labelled tab must be closed, not left behind"); + assertEquals("wL:t1", ((Map) herdr.lastCall("tab.close").params()).get("tab_id")); + } + + /** + * The operator's own trace on fleet01 (#359): a daemon that restarted several times, each + * occasion finding "0 live" for whatever reason, had left several identically-labelled dead + * tabs sitting side by side. All of them must be closed in the same reconcile that relaunches — + * closing only the most recent one would still let the count grow without bound over time. + */ + @Test + void allStaleLabelledTabsAreClosedWhenTheLeadIsRelaunched() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "fleet") + .withTab("wL", "wL:t1", "lead: opus") + .withTab("wL", "wL:t2", "lead: opus") + .withTab("wL", "wL:t3", "lead: opus"); // three restarts' worth of debris + + assertEquals(1, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); + + List closedTabIds = herdr.calls.stream() + .filter(c -> c.method().equals("tab.close")) + .map(c -> ((Map) c.params()).get("tab_id")) + .toList(); + assertEquals(3, closedTabIds.size(), + "every dead labelled tab must be closed, not just the most recently found one"); + assertEquals(Set.of("wL:t1", "wL:t2", "wL:t3"), Set.copyOf(closedTabIds)); + } + /** * A lead the operator opened by hand is live once its tab carries the configured `tab:` label — * CB-579 retired the `terminal:` pin, so a hand-opened lead is found the same way an