From 25726a5ae7bdea9219e81057375eb145ff5a5b45 Mon Sep 17 00:00:00 2001 From: Ha Trong Dai Date: Fri, 28 Aug 2026 09:36:39 +0700 Subject: [PATCH 1/3] #185: reject ambiguous unowned pane ids --- .../fleet/member/CompositePeerLauncher.java | 12 +++-- .../member/CompositePeerLauncherTest.java | 48 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index 1469b30..465a6fe 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -44,9 +44,9 @@ import java.util.stream.Collectors; * the single adapter that declares it. Profiles partition cleanly across adapters: the * constructor rejects a name claimed by two. *
  • By pane id — {@link #stop} routes to the adapter that spawned that pane - * (recorded at spawn time). A pane the composite never spawned (only real for a caller that - * hand-rolls an id) falls back to the first delegate; teardown is pane-id addressed and - * tab cleanup is single-occupant guarded, so it is safe either way.
    • + * (recorded at spawn time). A pane the composite never spawned can use the single delegate + * in a one-daemon fleet. With more than one delegate, its owner is unknown, so stop refuses + * the ambiguous id rather than closing a pane on an arbitrary herdr daemon. *
  • Fleet-wide — {@link #reapOrphanWorkers} and {@link #capabilities} fan out * and combine. {@link #list} is deduplicated by pane id because every herdr-backed delegate * shares one herdr connection and so reports the same global agent set.
    • @@ -437,7 +437,11 @@ public final class CompositePeerLauncher implements PeerLauncher { public void stop(String id) { HerdrPeerLauncher d = spawnedBy.remove(id); if (d == null) { - log.debug("stop({}) — no recorded owner, routing to the first adapter (pane-addressed)", id); + if (delegates.size() != 1) { + throw new IllegalArgumentException("ambiguous paneId '" + id + + "': no owning herdr daemon was recorded"); + } + log.debug("stop({}) — no recorded owner in a single-daemon fleet", id); d = delegates.getFirst(); } d.stop(id); 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 48ed960..060fc28 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -248,6 +248,54 @@ class CompositePeerLauncherTest { "stop routes to the spawning adapter and closes exactly that worker's pane"); } + @Test + void stopKeepsSameHerdrPaneIdSeparateByOwningAdapter() { + // Separate herdr daemons can both issue w9:pRoot_1. The opaque handles identify their + // spawning adapters, so each stop reaches only its recorded owner. + FakeHerdr first = new FakeHerdr(); + FakeHerdr second = new FakeHerdr(); + PeerLauncher composite = new CompositePeerLauncher( + List.of(claudeAdapter(first), opencodeAdapter(second)), "claude"); + + PeerHandle claude = composite.spawn(new SpawnRequest("claude", null, null)); + PeerHandle opencode = composite.spawn(new SpawnRequest("gemini", null, null)); + + assertNotEquals(claude.id(), opencode.id(), "each public paneId keeps its adapter owner"); + composite.stop(claude.id()); + assertTrue(first.calls.stream().anyMatch(c -> c.method().equals("pane.close") + && "w9:pRoot_1".equals(((Map) c.params()).get("pane_id"))), + "the first daemon closes its own pane"); + assertFalse(second.called("pane.close"), "the matching pane on the second daemon stays live"); + + composite.stop(opencode.id()); + assertTrue(second.calls.stream().anyMatch(c -> c.method().equals("pane.close") + && "w9:pRoot_1".equals(((Map) c.params()).get("pane_id"))), + "the second daemon then closes its own pane"); + } + + @Test + void stopAllowsALegacyBarePaneIdWithOneDaemon() { + FakeHerdr herdr = new FakeHerdr(); + PeerLauncher composite = new CompositePeerLauncher(List.of(claudeAdapter(herdr)), "claude"); + + composite.stop("w9:pRoot_1"); + + assertTrue(herdr.calls.stream().anyMatch(c -> c.method().equals("pane.close") + && "w9:pRoot_1".equals(((Map) c.params()).get("pane_id"))), + "one daemon keeps the legacy bare-pane routing behaviour"); + } + + @Test + void stopRejectsAnUnownedPaneIdWhenMultipleDaemonsCouldOwnIt() { + PeerLauncher composite = new CompositePeerLauncher( + List.of(claudeAdapter(new FakeHerdr()), opencodeAdapter(new FakeHerdr())), "claude"); + + IllegalArgumentException error = assertThrows(IllegalArgumentException.class, + () -> composite.stop("w1:p1")); + + assertEquals("ambiguous paneId 'w1:p1': no owning herdr daemon was recorded", error.getMessage()); + } + @Test void opencodeContextResetIsANoOpAndWarnsOnlyOnce() { FakeHerdr herdr = new FakeHerdr(); From 5ba05d0bdb13e0764dc353ee6791affa27ea320b Mon Sep 17 00:00:00 2001 From: Ha Trong Dai Date: Fri, 28 Aug 2026 09:42:24 +0700 Subject: [PATCH 2/3] #185: count herdr owners for stop fallback --- .../dev/ltms/fleet/herdr/AgentControl.java | 5 +++++ .../fleet/member/CompositePeerLauncher.java | 20 ++++++++++++++++--- .../ltms/fleet/member/HerdrPeerLauncher.java | 6 ++++++ .../member/CompositePeerLauncherTest.java | 12 +++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java index 75541a7..f98675a 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java @@ -38,6 +38,11 @@ public final class AgentControl { this.herdr = herdr; } + /** The herdr daemon this control object sends its agent calls to. */ + public HerdrClient herdr() { + return herdr; + } + /** One agent-targeted call, translating a terminal id to its pane id (retrying once fresh). */ private JsonNode agentCall(String method, String target, Map extra) { String resolved = resolveTarget(target); diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index 465a6fe..30daf3b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -2,6 +2,7 @@ package dev.ltms.fleet.member; import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.Agent; +import dev.ltms.fleet.herdr.HerdrClient; import dev.ltms.fleet.peer.Capability; import dev.ltms.fleet.peer.MemberRole; import dev.ltms.fleet.peer.PeerHandle; @@ -21,6 +22,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -44,8 +46,8 @@ import java.util.stream.Collectors; * the single adapter that declares it. Profiles partition cleanly across adapters: the * constructor rejects a name claimed by two. *
  • By pane id — {@link #stop} routes to the adapter that spawned that pane - * (recorded at spawn time). A pane the composite never spawned can use the single delegate - * in a one-daemon fleet. With more than one delegate, its owner is unknown, so stop refuses + * (recorded at spawn time). A pane the composite never spawned can use the fallback route + * in a one-daemon fleet. With more than one herdr daemon, its owner is unknown, so stop refuses * the ambiguous id rather than closing a pane on an arbitrary herdr daemon.
    • *
  • Fleet-wide — {@link #reapOrphanWorkers} and {@link #capabilities} fan out * and combine. {@link #list} is deduplicated by pane id because every herdr-backed delegate @@ -437,7 +439,7 @@ public final class CompositePeerLauncher implements PeerLauncher { public void stop(String id) { HerdrPeerLauncher d = spawnedBy.remove(id); if (d == null) { - if (delegates.size() != 1) { + if (herdrDaemonCount() != 1) { throw new IllegalArgumentException("ambiguous paneId '" + id + "': no owning herdr daemon was recorded"); } @@ -447,6 +449,18 @@ public final class CompositePeerLauncher implements PeerLauncher { d.stop(id); } + /** + * Count actual herdr daemons, not peer adapter kinds. Identity is intentional: separate client + * objects may represent different daemons even if a client later implements value equality. + */ + private int herdrDaemonCount() { + Set daemons = java.util.Collections.newSetFromMap(new IdentityHashMap<>()); + for (HerdrPeerLauncher delegate : delegates) { + daemons.add(delegate.herdr()); + } + return daemons.size(); + } + @Override public boolean clearContext(String id) { HerdrPeerLauncher delegate = spawnedBy.get(id); 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 6871dda..76c8f40 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -3,6 +3,7 @@ package dev.ltms.fleet.member; import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.Agent; import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.HerdrClient; import dev.ltms.fleet.herdr.HerdrException; import dev.ltms.fleet.herdr.Tab; import dev.ltms.fleet.herdr.Workspace; @@ -520,6 +521,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { req.sessionName(), spawned.agentSessionId(), spawned.receipt()); } + /** The herdr daemon that owns this launcher's pane coordinates. */ + public HerdrClient herdr() { + return agents.herdr(); + } + @Override public String effectiveCwd(SpawnRequest req) { return effectiveCwd(req.profileName(), req.requestedCwd(), req.callerCwd()); 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 060fc28..8fc49ef 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -285,6 +285,18 @@ class CompositePeerLauncherTest { "one daemon keeps the legacy bare-pane routing behaviour"); } + @Test + void stopAllowsAnUnownedPaneIdWithTwoAdaptersSharingOneDaemon() { + FakeHerdr herdr = new FakeHerdr(); + PeerLauncher composite = composite(herdr); + + composite.stop("w9:pRoot_1"); + + assertTrue(herdr.calls.stream().anyMatch(c -> c.method().equals("pane.close") + && "w9:pRoot_1".equals(((Map) c.params()).get("pane_id"))), + "two adapter kinds sharing one daemon keep the fallback route"); + } + @Test void stopRejectsAnUnownedPaneIdWhenMultipleDaemonsCouldOwnIt() { PeerLauncher composite = new CompositePeerLauncher( From 31d551699127367b033452f4b86524d1e4dfa69f Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 29 Aug 2026 06:09:43 +0700 Subject: [PATCH 3/3] #185 review: keep the stop owner on failure, key list() by daemon Three fixes on top of the pane-id PR, from my own read and the reviewer's: - stop() removed the spawnedBy record BEFORE the delegate accepted the stop. A delegate that threw left the pane alive with its owner forgotten, so the retry fell into the ambiguous branch and refused the id for good. Remove after. - list() deduplicated on the raw pane id. Pane ids are per-daemon counters, so two daemons can each hold w1:p1 on different panes, and one of the two real agents was silently dropped from fleet_list and every view built on it. The key is now (owning daemon, pane id). Delegates sharing one daemon still collapse, which is what the dedupe was for. - The class javadoc still stated the single-herdr-connection premise as fact, next to the bullet this PR had just corrected for stop(). Fixed there too. Also drops a redundantly qualified java.util.Collections. Tests: 993 run, 0 failures, BUILD SUCCESS. --- .../fleet/member/CompositePeerLauncher.java | 27 ++++++++--- .../member/CompositePeerLauncherTest.java | 47 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index 30daf3b..70cf73e 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -50,8 +50,9 @@ import java.util.stream.Collectors; * in a one-daemon fleet. With more than one herdr daemon, its owner is unknown, so stop refuses * the ambiguous id rather than closing a pane on an arbitrary herdr daemon.
    • *
  • Fleet-wide — {@link #reapOrphanWorkers} and {@link #capabilities} fan out - * and combine. {@link #list} is deduplicated by pane id because every herdr-backed delegate - * shares one herdr connection and so reports the same global agent set.
    • + * and combine. {@link #list} is deduplicated by (owning daemon, pane id): delegates that share + * one herdr connection report the same global agent set, but two daemons can each hold a pane + * called {@code w1:p1}, so the daemon has to be part of the key. * * *

    CB-518: an unqualified spawn is routed through a {@link PlacementPolicy}. The default @@ -437,7 +438,7 @@ public final class CompositePeerLauncher implements PeerLauncher { @Override public void stop(String id) { - HerdrPeerLauncher d = spawnedBy.remove(id); + HerdrPeerLauncher d = spawnedBy.get(id); if (d == null) { if (herdrDaemonCount() != 1) { throw new IllegalArgumentException("ambiguous paneId '" + id @@ -446,7 +447,11 @@ public final class CompositePeerLauncher implements PeerLauncher { log.debug("stop({}) — no recorded owner in a single-daemon fleet", id); d = delegates.getFirst(); } + // Drop the owner record only after the delegate accepted the stop. Removing it first meant a + // delegate that threw left the pane alive with its owner forgotten, so the retry fell into + // the ambiguous branch above and refused the id for good. d.stop(id); + spawnedBy.remove(id); } /** @@ -454,7 +459,7 @@ public final class CompositePeerLauncher implements PeerLauncher { * objects may represent different daemons even if a client later implements value equality. */ private int herdrDaemonCount() { - Set daemons = java.util.Collections.newSetFromMap(new IdentityHashMap<>()); + Set daemons = Collections.newSetFromMap(new IdentityHashMap<>()); for (HerdrPeerLauncher delegate : delegates) { daemons.add(delegate.herdr()); } @@ -493,14 +498,24 @@ public final class CompositePeerLauncher implements PeerLauncher { return route(profileName).capabilities(); } - /** Every herdr agent, deduplicated by pane id (all delegates share one herdr and list globally). */ + /** + * Every herdr agent, deduplicated by (owning daemon, pane id). + * + *

    Delegates that share one {@link HerdrClient} see the same global agent set, so listing them + * both would report every agent twice — that is what the dedupe is for. But pane ids are + * per-daemon counters, so two daemons really can both hold {@code w1:p1} on different panes. + * Keying on the pane id alone would silently drop one of them from {@code fleet_list} and from + * every status view built on it. The daemon is part of the key for exactly that reason. + */ @Override public List list() { + Map daemonIndex = new IdentityHashMap<>(); Map byPane = new LinkedHashMap<>(); for (HerdrPeerLauncher d : delegates) { + int daemon = daemonIndex.computeIfAbsent(d.herdr(), _ -> daemonIndex.size()); for (Agent a : d.list()) { if (a.paneId() != null) { - byPane.putIfAbsent(a.paneId(), a); + byPane.putIfAbsent(daemon + "\u0000" + a.paneId(), a); } } } 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 8fc49ef..ade228e 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -8,6 +8,7 @@ import dev.ltms.fleet.guard.SubscriptionGuard; import dev.ltms.fleet.herdr.Agent; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.HerdrException; import dev.ltms.fleet.herdr.WorkspaceControl; import dev.ltms.fleet.peer.Capability; import dev.ltms.fleet.peer.CharterReceipt; @@ -297,6 +298,52 @@ class CompositePeerLauncherTest { "two adapter kinds sharing one daemon keep the fallback route"); } + @Test + void listKeepsBothPanesWhenTwoDaemonsShareAPaneId() { + // herdr pane ids are per-daemon counters, so two daemons really can both hold w1:p1 on + // different panes. Deduplicating on the pane id alone dropped one of the two real agents. + FakeHerdr first = new FakeHerdr().withAgent("x", "term_x", "w1:p1", "w1:t1"); + FakeHerdr second = new FakeHerdr().withAgent("y", "term_y", "w1:p1", "w1:t1"); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(claudeAdapter(first), opencodeAdapter(second)), "claude"); + + List agents = composite.list(); + + assertEquals(2, agents.stream().filter(a -> "w1:p1".equals(a.paneId())).count(), + "one w1:p1 per daemon survives — the pane id alone is not a unique key"); + assertTrue(agents.stream().anyMatch(a -> "term_x".equals(a.terminalId()))); + assertTrue(agents.stream().anyMatch(a -> "term_y".equals(a.terminalId()))); + } + + @Test + void listStillDeduplicatesTwoAdaptersSharingOneDaemon() { + // Both adapters ask the SAME daemon, so both see the same agent set. Without the dedupe this + // would report every agent twice; the daemon key must not break that. + FakeHerdr herdr = new FakeHerdr().withAgent("x", "term_x", "w1:p1", "w1:t1"); + CompositePeerLauncher composite = composite(herdr); + + List agents = composite.list(); + + assertEquals(1, agents.stream().filter(a -> "w1:p1".equals(a.paneId())).count(), + "one daemon still reports each of its agents once"); + } + + @Test + void stopKeepsTheOwnerRecordWhenTheDelegateRefusesTheStop() { + // Removing the record before the delegate accepted the stop lost the owner on failure: the + // pane was still alive, but the retry landed in the ambiguous branch and refused it for good. + FakeHerdr first = new FakeHerdr().paneCloseFailsWith("pane_busy"); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(claudeAdapter(first), opencodeAdapter(new FakeHerdr())), "claude"); + PeerHandle claude = composite.spawn(new SpawnRequest("claude", null, null)); + + assertThrows(HerdrException.class, () -> composite.stop(claude.id())); + + // The retry must still know its owner — a HerdrException, never "ambiguous paneId". + assertThrows(HerdrException.class, () -> composite.stop(claude.id()), + "the owner record survives a failed stop, so the retry is not ambiguous"); + } + @Test void stopRejectsAnUnownedPaneIdWhenMultipleDaemonsCouldOwnIt() { PeerLauncher composite = new CompositePeerLauncher(