CB-185: fix ambiguous-pane message and unreachable-daemon abort in probeOwner (review)
CI / build (pull_request) Successful in 1m6s
CI / contract (pull_request) Successful in 1m5s

Lead review of PR #196 found two issues in CompositePeerLauncher.probeOwner:

1. The "more than one daemon claims this pane" throw kept the old
   pre-fix message ("no owning herdr daemon was recorded"), which was
   only true of the code it replaced. Reworded to say what actually
   happened: N configured herdr daemons report this pane, so it is
   genuinely ambiguous. Updated the one test pinning the old string.

2. probeOwner let list() propagate straight out of the probe loop, so
   one unreachable daemon aborted the whole probe and made a pane on a
   DIFFERENT, healthy daemon un-stoppable too — resurrecting the exact
   bug blocker 1 fixes. Now catches HerdrException per daemon, logs the
   exception class only, and treats that daemon as not knowing the pane
   so probing continues. New test proves this: verified it fails with
   the try/catch removed (HerdrException propagates and the stop that
   should succeed via the healthy daemon throws instead), then restored.

Full mvn clean install: 1019 tests, 0 failures, 0 errors.
This commit is contained in:
Dai Ha
2026-08-31 09:28:29 +07:00
parent 045d229728
commit bc99d64786
2 changed files with 45 additions and 8 deletions
@@ -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.HerdrClient;
import dev.ltms.fleet.herdr.HerdrException;
import dev.ltms.fleet.peer.Capability;
import dev.ltms.fleet.peer.MemberRole;
import dev.ltms.fleet.peer.PeerHandle;
@@ -46,9 +47,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 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>
* (recorded at spawn time). A pane the composite never spawned, or one whose record was lost
* to a daemon restart (CB-185 blocker 1 — {@link #spawnedBy} is in-memory only), can use the
* fallback route in a one-daemon fleet. With more than one herdr daemon, {@link #probeOwner}
* asks each configured daemon which one actually knows the pane: exactly one match routes
* (and caches); no match is treated as already-gone; more than one match is a genuine
* ambiguity (pane ids are per-daemon counters, so two daemons really can both hold, say,
* {@code w1:p1}) and stop refuses 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 (owning daemon, pane id): delegates that share
* one herdr connection report the same global agent set, but two daemons can each hold a pane
@@ -476,6 +481,11 @@ public final class CompositePeerLauncher implements PeerLauncher {
* probed twice, and a pane on their shared daemon would look owned by two adapters instead of
* one daemon.
*
* <p>A daemon that fails to answer {@code list()} (e.g. it is down) is treated as "does not know
* this pane" rather than aborting the whole probe — one unreachable daemon must never make a
* pane that a <em>different</em>, healthy daemon actually owns un-stoppable too, which would
* resurrect the exact bug this method exists to fix.
*
* @return the owning delegate — cached into {@link #spawnedBy} so the next call is free — or
* {@code null} when no daemon knows the pane
* @throws IllegalArgumentException when more than one daemon claims the pane: pane ids are
@@ -489,15 +499,23 @@ public final class CompositePeerLauncher implements PeerLauncher {
}
List<HerdrPeerLauncher> owners = new ArrayList<>();
for (HerdrPeerLauncher representative : byDaemon.values()) {
boolean knows = representative.list().stream()
.anyMatch(a -> id.equals(a.paneId()));
List<Agent> agents;
try {
agents = representative.list();
} catch (HerdrException e) {
log.warn("stop({}) probe: a configured herdr daemon was unreachable ({}); "
+ "treating it as not knowing this pane", id, e.getClass().getSimpleName());
continue;
}
boolean knows = agents.stream().anyMatch(a -> id.equals(a.paneId()));
if (knows) {
owners.add(representative);
}
}
if (owners.size() > 1) {
throw new IllegalArgumentException("ambiguous paneId '" + id
+ "': no owning herdr daemon was recorded");
throw new IllegalArgumentException("ambiguous paneId '" + id + "': "
+ owners.size() + " configured herdr daemons report this pane — "
+ "no way to tell which one the caller means");
}
if (owners.isEmpty()) {
return null;
@@ -357,7 +357,8 @@ class CompositePeerLauncherTest {
IllegalArgumentException error = assertThrows(IllegalArgumentException.class,
() -> composite.stop("w1:p1"));
assertEquals("ambiguous paneId 'w1:p1': no owning herdr daemon was recorded", error.getMessage());
assertEquals("ambiguous paneId 'w1:p1': 2 configured herdr daemons report this pane — "
+ "no way to tell which one the caller means", error.getMessage());
}
@Test
@@ -394,6 +395,24 @@ class CompositePeerLauncherTest {
assertFalse(second.called("pane.close"), "the daemon that never held the pane is never touched");
}
@Test
void aProbeSurvivesOneUnreachableDaemonAndStillFindsTheOwnerOnTheOtherOne() {
// CB-185 blocker 1 (lead review): a daemon that is DOWN while we probe must not abort the
// whole probe — the pane the OPERATOR actually wants stopped can live on a different,
// healthy daemon, and that pane must not become un-stoppable because a third one is down.
FakeHerdr down = new FakeHerdr().healthy(false);
FakeHerdr owner = new FakeHerdr().withAgent("x", "term_x", "w1:p1", "w1:t1");
PeerLauncher composite = new CompositePeerLauncher(
List.of(claudeAdapter(down), opencodeAdapter(owner)), "claude");
assertDoesNotThrow(() -> composite.stop("w1:p1"),
"the unreachable daemon must be skipped, not fail the whole stop");
assertTrue(owner.calls.stream().anyMatch(c -> c.method().equals("pane.close")
&& "w1:p1".equals(((Map<?, ?>) c.params()).get("pane_id"))),
"the healthy daemon that actually owns the pane still closes it");
}
@Test
void aProbedOwnerIsCachedSoARetryAfterAFailedStopNeedsNoSecondProbe() {
// CB-185 blocker 1: the probe's whole point is to be cheap on repeat — a failed stop (e.g.