diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java index c33863b..3d87531 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -937,14 +937,20 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { */ @Override public void stop(String idOrPane) { - // Teardown knows only the pane, not which profile spawned it. Attempt tab cleanup when any - // profile uses tab placement (so the bridge may have created a dedicated peer tab); the - // single-occupant check below is what actually protects the user's shared tabs. + // Teardown knows only the pane, not which profile spawned it — and, when this call is + // routed here through CompositePeerLauncher's single-daemon stop() shortcut (fleetd #342), + // not even which adapter's config actually governed the spawn: the shortcut can hand the + // pane to a delegate that never spawned it, whose own profiles say nothing about how THIS + // pane was placed. So the decision to look for a tab to clean up is made from the pane's + // actual state, not from this delegate's static profile config: resolve the tab + // unconditionally and let {@link WorkspaceControl#locatePane} tolerate "not found" (it + // returns null rather than throwing); the single-occupant check below is what actually + // protects the user's shared tabs, exactly as it always has. String paneId = paneByAgentId.remove(idOrPane); if (paneId == null) { paneId = idOrPane; // raw-pane fallback (reap, gate timeout, pane-addressed callers) } - WorkspaceControl.PaneLocation loc = usesTabPlacement() ? spaces.locatePane(paneId) : null; + WorkspaceControl.PaneLocation loc = spaces.locatePane(paneId); try { agents.close(paneId); } catch (HerdrException e) { @@ -985,11 +991,6 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { } } - /** Whether any configured profile places peers in their own tab (so tabs may need cleanup). */ - private boolean usesTabPlacement() { - return profiles.values().stream().anyMatch(FleetConfig.Profile::tabPlacement); - } - /** True when a herdr error means the target is already gone (safe to treat as done). */ private static boolean isAlreadyGone(HerdrException e) { return e.code() != null && e.code().endsWith("_not_found"); diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java index 7520cbc..3ef032c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -299,6 +299,41 @@ class CompositePeerLauncherTest { "two adapter kinds sharing one daemon keep the fallback route"); } + @Test + void stopThroughTheSingleDaemonShortcutStillClosesTheTabWhenTheFallbackDelegateUsesPanePlacement() { + // fleetd #342: the single-daemon shortcut (spawnedBy empty, herdrDaemonCount()==1) always + // routes stop() through delegates.getFirst() — here the claude adapter, configured for + // PANE placement (its own profiles never create a dedicated tab). The pane being torn + // down here actually belongs to the opencode adapter's TAB placement, sharing the same + // herdr daemon — mixing placements is the point: every existing stop-fallback test in this + // class configures BOTH adapters as "tab", so usesTabPlacement() was true either way and + // the mis-routing never showed. + // + // Before the fix, HerdrPeerLauncher#stop gated tab resolution on usesTabPlacement() of the + // delegate it happened to be called through, so the wrongly-routed (pane-placement) claude + // adapter never even looked for a tab to close, and the now-empty tab leaked with nothing + // to reap it. The fix (fleetd #342) resolves the pane's real tab unconditionally, so the + // decision follows the pane's actual placement rather than the fallback delegate's static + // config. + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile claudePane = new FleetConfig.Profile("claude", "http://gx00.gw:8000", + "coder", null, "FLEETD_WORKER_TOKEN", List.of("claude"), "pane", "fleetd-workers", + "w #{n}", null, null, null); + ClaudeCodeLauncher claude = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of("claude", claudePane), "claude", _ -> null); + PeerLauncher composite = new CompositePeerLauncher(List.of(claude, opencodeAdapter(herdr)), "claude"); + + // "w9:pW" was never spawned through this composite instance, so spawnedBy has no entry for + // it (the same in-memory-cache-miss shape a daemon restart leaves behind) — stop() falls + // through to the single-daemon shortcut and hands it to delegates.getFirst() (claude). + composite.stop("w9:pW"); + + assertTrue(herdr.called("pane.close"), "the pane itself is still closed on every routed path"); + assertTrue(herdr.called("tab.close"), + "the pane's real (sole-occupant) tab must be closed even though the fallback routed " + + "through a delegate configured for pane placement"); + } + @Test void listKeepsBothPanesWhenTwoDaemonsShareAPaneId() { // herdr pane ids are per-daemon counters, so two daemons really can both hold w1:p1 on diff --git a/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java b/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java index be273a1..b7a89fd 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java @@ -711,13 +711,21 @@ class FleetAppTest { @Test void stopWorkerInPanePlacementClosesOnlyThePane() throws Exception { - FakeHerdr herdr = new FakeHerdr(); + // fleetd #342: tab-cleanup resolution is no longer skipped based on a profile's declared + // placement — a stop() routed through the wrong delegate (e.g. CompositePeerLauncher's + // single-daemon shortcut after a daemon restart) could carry a placement config that says + // nothing true about how THIS pane was actually placed. So the pane's real tab is now + // always resolved, and the single-occupant check below is what protects a pane-placement + // peer's shared tab, exactly as it always protected a tab-placement one. Model that + // realistically: the peer's pane was split into an existing tab that already held another + // occupant, so the tab must never be closed. + FakeHerdr herdr = new FakeHerdr().withWorkerTabPaneCount(2); int port = start(herdr, "http://gx00.gw:8000", Set.of("gx00.gw"), "pane"); assertEquals(204, req(port, "DELETE", "/members/w9:pW").statusCode()); assertTrue(herdr.called("pane.close")); - assertFalse(herdr.called("tab.close"), "pane placement owns no tab to close"); - assertFalse(herdr.called("pane.get"), "no tab resolution in pane placement"); + assertTrue(herdr.called("pane.get"), "tab resolution now always runs, regardless of placement"); + assertFalse(herdr.called("tab.close"), "pane placement's shared tab must never be closed"); } @Test