#185: refuse an unowned paneId when more than one herdr daemon could own it #187
@@ -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<String, Object> extra) {
|
||||
String resolved = resolveTarget(target);
|
||||
|
||||
@@ -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.</li>
|
||||
* <li><strong>By pane id</strong> — {@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.</li>
|
||||
* (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.</li>
|
||||
* <li><strong>Fleet-wide</strong> — {@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.</li>
|
||||
* 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.</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>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<HerdrClient> 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).
|
||||
*
|
||||
* <p>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<Agent> list() {
|
||||
Map<HerdrClient, Integer> daemonIndex = new IdentityHashMap<>();
|
||||
Map<String, Agent> 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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -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<Agent> 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<Agent> 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();
|
||||
|
||||
Reference in New Issue
Block a user