From bc99d647862aa6abb324fe55295367b9b7f1fece Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 09:28:29 +0700 Subject: [PATCH] CB-185: fix ambiguous-pane message and unreachable-daemon abort in probeOwner (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../fleet/member/CompositePeerLauncher.java | 32 +++++++++++++++---- .../member/CompositePeerLauncherTest.java | 21 +++++++++++- 2 files changed, 45 insertions(+), 8 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 c329741..ee83b17 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.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.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. *
  • 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 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.
    • + * (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. *
  • Fleet-wide — {@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. * + *

    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 different, 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 owners = new ArrayList<>(); for (HerdrPeerLauncher representative : byDaemon.values()) { - boolean knows = representative.list().stream() - .anyMatch(a -> id.equals(a.paneId())); + List 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; 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 cf6cbe7..1e791d5 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -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.