Compare commits

...

1 Commits

Author SHA1 Message Date
Dai Ha 3fd23ecafa fleetd #342: base tab-cleanup teardown on the pane's real placement, not a delegate's static config
CI / contract (pull_request) Successful in 1m22s
CI / build (pull_request) Failing after 1m38s
HerdrPeerLauncher.stop() used to gate spaces.locatePane() on usesTabPlacement(),
which reads the delegate's OWN configured profiles. When CompositePeerLauncher's
single-daemon stop() shortcut hands a pane to a delegate that never spawned it
(spawnedBy empty after a daemon restart, herdrDaemonCount()==1), that delegate's
placement config says nothing true about how the pane was actually placed, and a
dedicated tab could be skipped and leaked.

Resolve the tab unconditionally instead — WorkspaceControl#locatePane already
tolerates a missing pane by returning null — and let the existing single-occupant
check (tabPaneCount()==1) be the only thing that decides whether to close it, same
as it already protects a shared tab regardless of declared placement.

Adds a mixed-placement CompositePeerLauncherTest (every existing stop-fallback test
configured both adapters as tab placement, so the mis-routing never showed) and
updates FleetAppTest#stopWorkerInPanePlacementClosesOnlyThePane, whose old
assertion (no pane.get on pane placement) documented exactly the skip this fix
removes.
2026-09-04 16:36:11 +07:00
3 changed files with 56 additions and 12 deletions
@@ -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");
@@ -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
@@ -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