From 96c406b9688df98df494fdb9a291c9c196ac11ec Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 15:54:52 +0700 Subject: [PATCH] #359 review: require two independent readings before destroying a lead's tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (LeadLauncher): closing a labelled tab on a single agent.list miss could destroy a live lead's session — the ticket's own evidence showed that exact signal missing a genuinely running agent. A dead reading now only flags the tab (PendingCloseMarker); it is closed only if a later, independently-connected reconcile still finds it dead while flagged. A tab found live again has its flag cleared instead. Finding 2 (LeadTabScanner): the new agent.list cross-check in scan() was not covered by get()'s "keep the cache on a failed scan" contract, which only fires on a thrown HerdrException. A successful-but-short agent.list could silently drop a lead CallerResolver had already resolved, demoting it to Role.WORKER. A terminal already reported live now gets one grace scan before being dropped; a terminal never reported live gets none, so the original #359 exclusion is unaffected. Both mechanisms were mutation-tested: reverting either change turns exactly its own new tests red and nothing else. --- .../dev/ltms/fleet/herdr/LeadTabScanner.java | 50 +++++++- .../ltms/fleet/herdr/PendingCloseMarker.java | 42 +++++++ .../dev/ltms/fleet/lead/LeadLauncher.java | 118 ++++++++++++++---- .../ltms/fleet/herdr/LeadTabScannerTest.java | 78 ++++++++++++ .../dev/ltms/fleet/lead/LeadLauncherTest.java | 100 ++++++++++++--- 5 files changed, 341 insertions(+), 47 deletions(-) create mode 100644 fleetd/src/main/java/dev/ltms/fleet/herdr/PendingCloseMarker.java 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 674fdef..9459409 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java @@ -74,9 +74,20 @@ import java.util.function.Supplier; * 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 - * mid-session. + * is TTL-cached and a stale-but-valid map is preferred to a herdr round-trip. A failed scan (herdr + * throws) keeps the previous answer instead of emptying it — a herdr hiccup must not silently + * demote a live lead mid-session. + * + *

fleetd #359 review, finding 2 — a successful-but-wrong scan is the same hazard. + * The catch above only fires when a call throws. It does nothing for a call that returns 200 with an + * incomplete answer — exactly what the ticket's own evidence showed {@code agent.list} can do. Once + * this class started trusting that signal, an empty read would otherwise get cached as fact and + * silently drop a lead {@code CallerResolver} had, until then, correctly resolved — turning it into a + * {@code Role.WORKER}, which refuses every orchestration call. So a terminal this class already + * reported as live is not dropped the first time {@code agent.list} loses it: {@link #scan()} grants + * it one grace scan (see {@code gracedTerminals}) and only drops it if a later scan still + * finds no agent. A terminal never reported live before gets no grace — that would weaken the + * original #359 fix itself, which this class's own test suite already pins. */ public final class LeadTabScanner implements Supplier> { @@ -92,6 +103,15 @@ public final class LeadTabScanner implements Supplier> { private long scannedAtNanos; private boolean everScanned; + /** + * Terminals currently on their one grace scan: {@code cached} reported them live, the most + * recent {@link #scan()} found no agent for them, and they were re-included anyway. Cleared for + * a terminal the instant it is seen live again; a terminal still here on the next scan + * is finally dropped. Scoped separately from {@link #cached} so a graced terminal cannot renew + * its own grace forever just by staying in the exposed map (fleetd #359 review, finding 2). + */ + private Set gracedTerminals = Set.of(); + /** * @param herdr the herdr client to query ({@code workspace.list}, * {@code tab.list}, {@code pane.list} — all read-only) @@ -172,6 +192,7 @@ public final class LeadTabScanner implements Supplier> { } if (nameByTab.isEmpty()) { + gracedTerminals = Set.of(); return Map.of(); } @@ -187,15 +208,30 @@ public final class LeadTabScanner implements Supplier> { } Map byTerminal = new LinkedHashMap<>(); + Set stillGraced = new HashSet<>(); // 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)) { + if (name == null || terminal == null || terminal.isBlank()) { + continue; + } + if (tabsWithAgent.contains(tabId)) { byTerminal.put(terminal, name); + continue; + } + // No agent reported for this tab, but its tab/pane are still here — this is the + // ambiguous case review finding 2 named: a successful agent.list that came back short + // does not prove the lead is dead. Grant one grace scan to a terminal we had already + // reported as live; a terminal we never reported live gets none, so the original #359 + // fix (a genuinely dead tab is never reported) is unaffected for the common case. + if (cached.containsKey(terminal) && !gracedTerminals.contains(terminal)) { + byTerminal.put(terminal, name); + stillGraced.add(terminal); } } + gracedTerminals = stillGraced; return Collections.unmodifiableMap(byTerminal); } @@ -204,12 +240,14 @@ public final class LeadTabScanner implements Supplier> { * *

Exact match (case-insensitive, ends stripped) against {@link #tabToName} — no prefix * stripping, so an operator's {@code "lead: something-else"} tab is never mistaken for a - * configured lead just because it shares a prefix. + * configured lead just because it shares a prefix. The match strips a trailing + * {@link PendingCloseMarker} first, so a tab {@code LeadLauncher} has flagged as maybe-dead but + * not yet closed keeps resolving normally while that reconcile is pending. */ private String leadNameOf(String label) { if (label == null) { return null; } - return tabToName.get(label.strip().toLowerCase(Locale.ROOT)); + return tabToName.get(PendingCloseMarker.strip(label).toLowerCase(Locale.ROOT)); } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/PendingCloseMarker.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/PendingCloseMarker.java new file mode 100644 index 0000000..f6cdcd3 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/PendingCloseMarker.java @@ -0,0 +1,42 @@ +package dev.ltms.fleet.herdr; + +/** + * The suffix {@code dev.ltms.fleet.lead.LeadLauncher} appends to a lead tab's label the first time a + * reconcile finds no running agent in it, before it is sure enough to close the tab outright. + * + *

fleetd #359 review, finding 1. The daemon's own evidence showed + * {@code agent.list} can read "no agent" for a tab that genuinely has one running — so a single such + * reading must never be treated as proof a tab is dead. {@code LeadLauncher} now writes this marker + * on the first miss, and only closes the tab if a later, independent reconcile still finds + * it dead while the marker is still there. Two consecutive misses, one restart apart, is a much + * stronger claim than one. + * + *

{@link LeadTabScanner} strips the same suffix before matching a label against a configured + * lead's {@code tab}, so a flagged-but-actually-still-live tab keeps resolving normally — the marker + * changes nothing about which pane {@code LeadCoordLoop} can reach while the flag is pending. Both + * classes must use exactly this suffix, which is why it lives here rather than as a private constant + * on either. + */ +public final class PendingCloseMarker { + + public static final String SUFFIX = " [fleetd:pending-close]"; + + private PendingCloseMarker() { + } + + /** The label with any trailing pending-close marker removed, for name matching. */ + public static String strip(String label) { + if (label == null) { + return null; + } + String stripped = label.strip(); + return stripped.endsWith(SUFFIX) + ? stripped.substring(0, stripped.length() - SUFFIX.length()).strip() + : stripped; + } + + /** Whether a label currently carries the marker. */ + public static boolean isFlagged(String label) { + return label != null && label.strip().endsWith(SUFFIX); + } +} 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 0bdc303..e322d67 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadLauncher.java @@ -4,6 +4,7 @@ import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.Agent; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.HerdrException; +import dev.ltms.fleet.herdr.PendingCloseMarker; import dev.ltms.fleet.herdr.Tab; import dev.ltms.fleet.herdr.Workspace; import dev.ltms.fleet.herdr.WorkspaceControl; @@ -53,9 +54,26 @@ import dev.ltms.fleet.peer.PeerLauncher; * 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. + * {@code LeadTabScanner} (before its own #359 fix) reported every one of them as a lead. + * + *

Review finding 1 — closing on one reading is worse than the bug. The first + * version of this fix closed a name's dead tabs the moment a single {@link #countLeads} reading + * called them dead. The ticket's own live evidence rules that out: on a real host, {@code + * agent.list} was seen reporting "0 live" for a tab that a plain {@code ps} confirmed was running a + * real session. Closing on that reading would have destroyed the operator's actual lead — a worse + * failure than the extra tab it replaces. So {@link #ensureLeads()} now needs the same dead reading + * twice, one restart apart, before it closes anything: the first time a labelled tab reads + * dead, it is only flagged ({@link PendingCloseMarker}), left running, untouched; it is closed only + * if a later, independently-connected reconcile still finds it dead while the flag is + * still there. A transient miss self-heals — the next reconcile sees the agent again and clears the + * flag (see {@code toUnflag} below) — so the worst case for a single bad reading is one extra tab + * surviving one more restart, never a live session destroyed. Two alternatives were considered and + * rejected: corroborating {@code agent.list} against a second, truly independent signal was dropped + * because nothing else herdr exposes proves "is a process attached to this pane" any better — a + * second call to the same unreliable source is not independent evidence; capping the close to "all + * but the most recent dead tab" was dropped because "most recent" has no reliable ordering across + * tab ids and would leave the true failure mode (a name that is never reconfirmed) growing + * by one tab per bad reading forever, which is the exact defect this ticket exists to fix. */ public final class LeadLauncher { @@ -107,20 +125,46 @@ public final class LeadLauncher { 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()); + // fleetd #359 review finding 1: a tab already flagged pending-close, still labelled for + // this lead, and STILL hosting no agent on this separate reconcile — two independent + // readings agree, so close it. A tab found dead for the first time is only flagged below, + // never closed on the spot. + for (String tabId : state.toClose()) { + log.info("lead '{}': closing tab {} — flagged pending-close on a previous reconcile " + + "and still no agent running in it", name, tabId); try { - spaces.closeTab(deadTabId); + spaces.closeTab(tabId); } catch (RuntimeException cleanup) { log.warn("could not close stale tab {} for lead '{}': {}", - deadTabId, name, cleanup.getMessage()); + tabId, name, cleanup.getMessage()); + } + } + // A tab labelled for this lead, with no agent running in it, seen dead for the first + // time — flag it rather than closing it. One reading of `agent.list` is not enough + // evidence to destroy a tab that might genuinely be live (see the class javadoc). + for (String tabId : state.toFlag()) { + String flagged = lead.tabLabel() + PendingCloseMarker.SUFFIX; + log.info("lead '{}': tab {} has no agent running in it this reconcile — flagging it " + + "'{}' rather than closing; it is only closed if a later reconcile still " + + "finds it dead", name, tabId, flagged); + try { + spaces.renameTab(tabId, flagged); + } catch (RuntimeException cleanup) { + log.warn("could not flag stale tab {} for lead '{}': {}", + tabId, name, cleanup.getMessage()); + } + } + // A previously-flagged tab that is running an agent again — the miss that flagged it was + // transient. Clear the flag so a future, unrelated miss starts its own two-reading count + // rather than closing on the strength of this one's already-spent flag. + for (String tabId : state.toUnflag()) { + log.info("lead '{}': tab {} is running an agent again — clearing its pending-close flag", + name, tabId); + try { + spaces.renameTab(tabId, lead.tabLabel()); + } catch (RuntimeException cleanup) { + log.warn("could not clear the pending-close flag on tab {} for lead '{}': {}", + tabId, name, cleanup.getMessage()); } } @@ -165,13 +209,15 @@ public final class LeadLauncher { * 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. + *

fleetd #359 review finding 1: a labelled tab with nothing running in it is split into + * {@code toClose} (already flagged pending-close by a previous reconcile, and still dead — two + * independent readings agree) and {@code toFlag} (dead for the first time — not enough evidence + * to close yet). {@code toUnflag} is the reverse: a tab flagged pending-close that is running an + * agent again, so the flag it carries no longer means anything and {@link #ensureLeads()} clears + * it. */ - private record LeadCount(int running, List deadTabIds) { - static final LeadCount NONE = new LeadCount(0, List.of()); + private record LeadCount(int running, List toClose, List toFlag, List toUnflag) { + static final LeadCount NONE = new LeadCount(0, List.of(), List.of(), List.of()); } private Map countLeads(Map leaders) { @@ -182,6 +228,7 @@ public final class LeadLauncher { // profile's `worker: {profile} #{n}` template. These never collide, so an exact-label match // separates them without needing to know which workspace anyone is in. Map nameByTab = new LinkedHashMap<>(); + Set flaggedTabIds = new LinkedHashSet<>(); for (Workspace ws : spaces.listWorkspaces()) { if (ws.workspaceId() == null) { continue; @@ -190,6 +237,9 @@ public final class LeadLauncher { String declared = leadNameOf(tab.label(), leaders); if (declared != null && tab.tabId() != null) { nameByTab.put(tab.tabId(), declared); + if (PendingCloseMarker.isFlagged(tab.label())) { + flaggedTabIds.add(tab.tabId()); + } } } } @@ -204,17 +254,31 @@ public final class LeadLauncher { } } - Map> deadTabsByName = new LinkedHashMap<>(); + Map> toCloseByName = new LinkedHashMap<>(); + Map> toFlagByName = new LinkedHashMap<>(); + Map> toUnflagByName = new LinkedHashMap<>(); nameByTab.forEach((tabId, name) -> { - if (!liveTabIds.contains(tabId)) { - deadTabsByName.computeIfAbsent(name, k -> new ArrayList<>()).add(tabId); + boolean live = liveTabIds.contains(tabId); + boolean flagged = flaggedTabIds.contains(tabId); + if (live) { + if (flagged) { + toUnflagByName.computeIfAbsent(name, k -> new ArrayList<>()).add(tabId); + } + return; + } + if (flagged) { + toCloseByName.computeIfAbsent(name, k -> new ArrayList<>()).add(tabId); + } else { + toFlagByName.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()))); + toCloseByName.getOrDefault(name, List.of()), + toFlagByName.getOrDefault(name, List.of()), + toUnflagByName.getOrDefault(name, List.of()))); } return out; } @@ -223,13 +287,15 @@ public final class LeadLauncher { * The configured lead a tab label names, or {@code null} for a label that names none. * *

Matched exactly (case-insensitively) against each lead's configured {@code tab}, so an - * operator's {@code "lead: something-else"} tab is not mistaken for a configured lead. + * operator's {@code "lead: something-else"} tab is not mistaken for a configured lead. A + * trailing {@link PendingCloseMarker} is stripped first, so a tab this class flagged on a + * previous reconcile is still recognised as the same lead's tab on this one. */ private String leadNameOf(String label, Map leaders) { if (label == null) { return null; } - String l = label.strip(); + String l = PendingCloseMarker.strip(label); for (Map.Entry e : leaders.entrySet()) { String tab = e.getValue().tabLabel(); if (tab != null && l.equalsIgnoreCase(tab.strip())) { 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 e395220..ad91a13 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/LeadTabScannerTest.java @@ -66,6 +66,12 @@ class LeadTabScannerTest { return this; } + /** Undo {@link #deadAgent} — models {@code agent.list} reporting the tab live again. */ + TopologyHerdr reviveAgent(String tabId) { + deadTabs.remove(tabId); + return this; + } + @Override public JsonNode call(String method, Object params) { calls++; @@ -270,6 +276,78 @@ class LeadTabScannerTest { assertEquals("gpt-sol-5.6", leads.get("term_gpt")); } + /** + * fleetd #359 review, finding 2 — the exact scenario the ticket's own evidence showed: + * {@code agent.list} can come back successfully but short, without the herdr call ever throwing. + * A lead this class already reported as live must not be dropped on the strength of one such + * read: {@link LeadTabScanner#get()}'s "keep the cache on failure" contract only fires on an + * exception, so without a fix a single short {@code agent.list} silently empties the cached map — + * which would resolve that lead's pane as {@code Role.WORKER} downstream, refusing every + * orchestration call. Mutate this away (drop the one-scan grace in {@link + * LeadTabScanner#scan()}) and this test must fail. + */ + @Test + void aTransientAgentListMissDoesNotDemoteALeadAlreadyKnownLive() { + TopologyHerdr herdr = twoLeads(); + AtomicLong clock = new AtomicLong(); + LeadTabScanner s = scanner(herdr, twoLeadsConfigured(), clock); + assertTrue(s.get().containsKey("term_opus"), "opus must be known live before the miss"); + + // One scan where agent.list comes back without opus's tab, even though the tab and pane are + // completely unchanged — the tab/pane are still there, only the liveness read is short. + herdr.deadAgent("w1:t1"); + clock.addAndGet(TTL); + + assertTrue(s.get().containsKey("term_opus"), + "a single missed detection must not empty the cached map for a lead already known live"); + assertEquals("gpt-sol-5.6", s.get().get("term_gpt"), "the unaffected lead is unchanged"); + } + + /** + * The other half of finding 2, so the grace above cannot be mistaken for permanent amnesty: the + * original #359 invariant (a genuinely dead tab is not reported forever) must still hold once a + * SECOND, independent scan agrees the agent is gone. + */ + @Test + void aLeadMissingFromAgentListOnTwoConsecutiveScansIsFinallyDropped() { + TopologyHerdr herdr = twoLeads(); + AtomicLong clock = new AtomicLong(); + LeadTabScanner s = scanner(herdr, twoLeadsConfigured(), clock); + assertTrue(s.get().containsKey("term_opus")); + + herdr.deadAgent("w1:t1"); + clock.addAndGet(TTL); + assertTrue(s.get().containsKey("term_opus"), "first miss is a grace period, not a verdict"); + + clock.addAndGet(TTL); // a second, independent scan — still no agent + assertFalse(s.get().containsKey("term_opus"), + "a second consecutive miss for the same terminal must finally drop it"); + } + + /** A lead that recovers between the two misses keeps its grace spent, not renewed for free. */ + @Test + void aLeadThatRecoversBetweenMissesIsReportedNormallyAndResetsItsGrace() { + TopologyHerdr herdr = twoLeads(); + AtomicLong clock = new AtomicLong(); + LeadTabScanner s = scanner(herdr, twoLeadsConfigured(), clock); + assertTrue(s.get().containsKey("term_opus")); + + herdr.deadAgent("w1:t1"); + clock.addAndGet(TTL); + assertTrue(s.get().containsKey("term_opus"), "graced on the first miss"); + + herdr.reviveAgent("w1:t1"); // the miss really was transient + clock.addAndGet(TTL); + assertTrue(s.get().containsKey("term_opus"), "found live again — reported normally"); + + // A later, unrelated miss must get its own fresh grace scan rather than being dropped + // immediately because the earlier miss had already "used up" a slot for this terminal. + herdr.deadAgent("w1:t1"); + clock.addAndGet(TTL); + assertTrue(s.get().containsKey("term_opus"), + "a miss after a genuine recovery is a new event and deserves its own grace scan"); + } + /** * 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 0da75a1..2a3578a 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadLauncherTest.java @@ -125,38 +125,76 @@ class LeadLauncherTest { } /** - * 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. + * fleetd #359 review finding 1 — the exact scenario the ticket's own live evidence produced: on a + * real host, {@code agent.list} reported "0 live" for a tab a plain {@code ps} confirmed was + * running a real session. A single such reading must never close the tab outright — that would + * destroy the operator's actual lead, a worse failure than the stale-tab bug this ticket exists + * to fix. The first dead reading only flags the tab; mutate this away (make the first reading + * close instead of flag) and this test must fail. */ @Test - void aStaleLabelledTabIsClosedWhenTheLeadIsRelaunched() { + void aStaleLabelledTabIsFlaggedRatherThanClosedOnTheFirstReconcile() { FakeHerdr herdr = new FakeHerdr() .withWorkspace("wL", "fleet") - .withTab("wL", "wL:t1", "lead: opus"); // label only — the debris from a previous life + .withTab("wL", "wL:t1", "lead: opus"); // label only, first look — could be a live session agent.list missed 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")); + assertFalse(herdr.called("tab.close"), + "one missed reading must never close a tab that might be hosting a live session"); + assertTrue(flaggedTabIds(herdr).contains("wL:t1"), + "the stale tab must be flagged pending-close so a later reconcile can confirm it"); } /** * 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. + * tabs sitting side by side. Every one of them is flagged on its first dead reading, not closed — + * none is more or less trustworthy than another. */ @Test - void allStaleLabelledTabsAreClosedWhenTheLeadIsRelaunched() { + void allStaleLabelledTabsAreFlaggedRatherThanClosedOnTheFirstReconcile() { 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 + .withTab("wL", "wL:t3", "lead: opus"); // three restarts' worth of debris, none confirmed twice yet + + assertEquals(1, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); + + assertFalse(herdr.called("tab.close"), "no tab may be closed on its first dead reading"); + assertEquals(Set.of("wL:t1", "wL:t2", "wL:t3"), flaggedTabIds(herdr), + "every dead labelled tab must be flagged, not just the first one found"); + } + + /** + * fleetd #359 review finding 1, the other half — a tab already flagged pending-close by an + * earlier reconcile, and STILL dead on this one, has now been read dead on two independent, + * separately-connected reconciles. That is strong enough evidence to actually close it. Mutate + * this away (never close a flagged tab) and this test must fail — the original #359 growth bug + * would come back for good. + */ + @Test + void aTabAlreadyFlaggedPendingCloseIsClosedWhenStillDeadOnALaterReconcile() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "fleet") + .withTab("wL", "wL:t1", "lead: opus [fleetd:pending-close]"); // flagged last reconcile, still dead + + assertEquals(1, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); + + assertTrue(herdr.called("tab.close"), + "a tab dead on two independent reconciles must finally be closed"); + assertEquals("wL:t1", ((Map) herdr.lastCall("tab.close").params()).get("tab_id")); + } + + /** All of several already-flagged, still-dead tabs are closed — not just the first found. */ + @Test + void allTabsAlreadyFlaggedPendingCloseAreClosedWhenStillDead() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "fleet") + .withTab("wL", "wL:t1", "lead: opus [fleetd:pending-close]") + .withTab("wL", "wL:t2", "lead: opus [fleetd:pending-close]") + .withTab("wL", "wL:t3", "lead: opus [fleetd:pending-close]"); assertEquals(1, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads()); @@ -165,10 +203,42 @@ class LeadLauncherTest { .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"); + "every confirmed-dead labelled tab must be closed, not just the first one found"); assertEquals(Set.of("wL:t1", "wL:t2", "wL:t3"), Set.copyOf(closedTabIds)); } + /** + * A tab flagged pending-close on a previous reconcile that is running an agent again — the miss + * that flagged it was transient. It must never be closed, and its flag must be cleared so a + * future, unrelated miss starts its own two-reading count from zero. + */ + @Test + void aFlaggedTabRunningAnAgentAgainHasItsFlagClearedInsteadOfBeingClosed() { + FakeHerdr herdr = new FakeHerdr() + .withWorkspace("wL", "fleet") + .withTab("wL", "wL:t1", "lead: opus [fleetd:pending-close]") + .withAgent("lead-opus", "term_lead", "wL:p1", "wL:t1"); // it recovered — really alive now + + assertEquals(0, launcher(herdr, configWith(lead("opus", "lead: opus", 1))).ensureLeads(), + "the lead is live again — nothing to relaunch"); + + assertFalse(herdr.called("tab.close"), "a tab running an agent again must never be closed"); + assertFalse(herdr.called("agent.start"), "the lead is live again — nothing to relaunch"); + assertEquals("wL:t1", ((Map) herdr.lastCall("tab.rename").params()).get("tab_id")); + assertEquals("lead: opus", ((Map) herdr.lastCall("tab.rename").params()).get("label"), + "the pending-close flag must be cleared once the tab is confirmed live again"); + } + + /** Every {@code tab.rename} call whose label carries the pending-close marker, by tab id. */ + private static Set flaggedTabIds(FakeHerdr herdr) { + return herdr.calls.stream() + .filter(c -> c.method().equals("tab.rename")) + .filter(c -> String.valueOf(((Map) c.params()).get("label")) + .endsWith("[fleetd:pending-close]")) + .map(c -> String.valueOf(((Map) c.params()).get("tab_id"))) + .collect(java.util.stream.Collectors.toUnmodifiableSet()); + } + /** * 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