#359: LeadTabScanner drops dead lead tabs; LeadLauncher closes them on relaunch
LeadTabScanner.scan() joined labelled tabs to panes with no liveness check at all, so a tab left behind by a crashed/relaunched lead read as a live lead forever -- exactly the hazard its own javadoc predicted but excused. It now cross-checks agent.list, the same signal LeadLauncher already trusted, and drops any labelled tab with no agent running in it. That alone removes the duplicate candidates LeadCoordLoop.resolveLocalLead() could pick from, including the dead one its own WARN's advice (name a lead after coordinator.selfId) could land a message in. LeadLauncher never actually stopped the accumulation: relaunching on "0 live" always created a brand new tab and left the old dead-labelled one right where it was, so any restart that found 0 live for any reason (a real crash, or a herdr read that missed a still-running agent) added one more dead tab, forever. ensureLeads() now closes every dead-labelled tab for a lead as part of the same reconcile that decides to relaunch, so at most one tab survives per configured lead once a restart's reconcile has run.
This commit is contained in:
@@ -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 <em>staleness</em> 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 <em>removes</em> something based on this map, add the same check.
|
||||
* The remaining hazard is an <em>operator</em> 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.
|
||||
*
|
||||
* <p><strong>fleetd #359 — the staleness check this class used to skip.</strong> 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.
|
||||
*
|
||||
* <p><strong>Caching.</strong> {@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<Map<String, String>> {
|
||||
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<String, String> scan() {
|
||||
Map<String, String> nameByTab = new LinkedHashMap<>();
|
||||
for (JsonNode w : herdr.call("workspace.list").path("workspaces")) {
|
||||
@@ -158,15 +171,29 @@ public final class LeadTabScanner implements Supplier<Map<String, String>> {
|
||||
}
|
||||
}
|
||||
|
||||
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<String> 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<String, String> 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);
|
||||
|
||||
@@ -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 <em>staleness</em>: 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 <em>running agent</em> in that tab — see {@link #liveLeads}. A labelled
|
||||
* when herdr also reports a <em>running agent</em> in that tab — see {@link #countLeads}. A labelled
|
||||
* tab with no agent in it is not a lead.
|
||||
*
|
||||
* <p><strong>fleetd #359 — a stale label used to pile up, not just mislead.</strong> 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<String, Integer> live;
|
||||
Map<String, LeadCount> 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<String, FleetConfig.Leader> 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
|
||||
* <em>not</em> 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.
|
||||
*
|
||||
* <p>There used to be a second path here — a running agent on the terminal a
|
||||
* {@code fleet.leaders.<name>.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.
|
||||
*
|
||||
* <p>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<String, Integer> liveLeads(Map<String, FleetConfig.Leader> leaders) {
|
||||
private record LeadCount(int running, List<String> deadTabIds) {
|
||||
static final LeadCount NONE = new LeadCount(0, List.of());
|
||||
}
|
||||
|
||||
private Map<String, LeadCount> countLeads(Map<String, FleetConfig.Leader> 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<String> liveTabIds = new LinkedHashSet<>();
|
||||
Map<String, Integer> 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<String, List<String>> deadTabsByName = new LinkedHashMap<>();
|
||||
nameByTab.forEach((tabId, name) -> {
|
||||
if (!liveTabIds.contains(tabId)) {
|
||||
deadTabsByName.computeIfAbsent(name, k -> new ArrayList<>()).add(tabId);
|
||||
}
|
||||
});
|
||||
|
||||
Map<String, LeadCount> out = new LinkedHashMap<>();
|
||||
for (String name : leaders.keySet()) {
|
||||
out.put(name, new LeadCount(counts.getOrDefault(name, 0),
|
||||
deadTabsByName.getOrDefault(name, List.of())));
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<String, String[]> tabs = new LinkedHashMap<>();
|
||||
/** pane_id → [tab_id, terminal_id]. */
|
||||
final Map<String, String[]> 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<String> 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<String> 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<String, String> 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<String, String> 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
|
||||
|
||||
@@ -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<Object> closedTabIds = herdr.calls.stream()
|
||||
.filter(c -> c.method().equals("tab.close"))
|
||||
.<Object>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
|
||||
|
||||
Reference in New Issue
Block a user