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 1469b30..70cf73e 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,12 +46,13 @@ 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 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 - * 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 @@ -435,12 +438,32 @@ 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) { - log.debug("stop({}) — no recorded owner, routing to the first adapter (pane-addressed)", id); + if (herdrDaemonCount() != 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(); } + // 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); + } + + /** + * 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 = Collections.newSetFromMap(new IdentityHashMap<>()); + for (HerdrPeerLauncher delegate : delegates) { + daemons.add(delegate.herdr()); + } + return daemons.size(); } @Override @@ -475,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/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 48ed960..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; @@ -248,6 +249,112 @@ 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 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 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( + 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();