From d6ef0c801378d665f95552c052b50301e3cf3b15 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sun, 6 Sep 2026 20:08:18 +0700 Subject: [PATCH] fleetd #368 review: only agent_not_found may forget a lead binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isLive treated any RuntimeException from the liveness probe as "the lead is gone", which forgetDelegation then acted on destructively and permanently. That made a transient herdr hiccup (socket blip, decode error) on a perfectly live lead indistinguishable from the lead actually being dead — the same one-bad-reading mistake #359 shipped a guard against for lead-tab liveness. Narrow isLive to match AgentControl.agentCall's own rule: only an affirmative HerdrException("agent_not_found") counts as gone. Every other failure is treated as still live and the binding is left alone. Adds aTransientLivenessFailureMustNotForgetABindingToAStillLiveLead, which fails with the bare RuntimeException catch and passes with the narrowed one. --- .../dev/ltms/fleet/msg/ReplyPushLoop.java | 36 ++++++++- .../dev/ltms/fleet/msg/ReplyPushLoopTest.java | 80 +++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/msg/ReplyPushLoop.java b/fleetd/src/main/java/dev/ltms/fleet/msg/ReplyPushLoop.java index 45d9dd2..0f5abd3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/ReplyPushLoop.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/ReplyPushLoop.java @@ -2,6 +2,7 @@ package dev.ltms.fleet.msg; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.AgentStatus; +import dev.ltms.fleet.herdr.HerdrException; import dev.ltms.fleet.mcp.PrimaryRegistry; import dev.ltms.fleet.metrics.FleetMetrics; import dev.ltms.fleet.metrics.Metrics; @@ -398,6 +399,18 @@ public final class ReplyPushLoop { * {@code AgentControl.paneByTerminal} already does on {@code agent_not_found}) and resolution is * retried, which now reaches the fallback {@code nudgeTargetFor} was built to reach — the same * empty-map state its javadoc already argues is correct. + * + *

fleetd #368 review — only a positive "gone" reading forgets the binding. + * The first version of this method treated any {@code RuntimeException} from the probe + * as death, which is the #359 mistake repeated: a transient socket blip or a codec error on a + * perfectly live lead would silently and permanently unbind it, with no re-record ever coming. + * That is destructive on one bad reading, exactly what #359 shipped a two-reading guard to avoid + * for the analogous lead-tab-liveness question. {@link #isLive} now matches + * {@code AgentControl.agentCall}'s own narrower rule (see its {@code agent_not_found} check): only + * that specific, affirmative "herdr has no such agent" signal counts as gone. Every other failure + * — timeout, transport error, a decode error — is treated as still live and the binding is left + * alone, because guessing wrong here is unrecoverable while guessing "live" merely costs one more + * retry on the next tick, which {@link #decide} already tolerates. */ private Optional resolveLiveLead(String target) { Optional lead = primaryRegistry.nudgeTargetFor(target); @@ -410,14 +423,31 @@ public final class ReplyPushLoop { return primaryRegistry.nudgeTargetFor(target); } - /** Whether herdr still reports a status for {@code lead} — false for a closed/dead terminal. */ + /** + * Whether {@code lead} should still be trusted: {@code false} only when herdr affirmatively + * reports the terminal gone ({@code agent_not_found}), never on a merely inconclusive failure. + * + *

fleetd #368 review: an earlier version returned {@code false} for any {@code RuntimeException}, + * which made a transient herdr hiccup on a live lead indistinguishable from the lead actually + * being dead — and the caller's response to {@code false} ({@code forgetDelegation}) is + * destructive and permanent. Narrowed to the one code {@code AgentControl.agentCall} itself + * already treats as a genuine, resolvable absence (see its {@code agent_not_found} handling) — + * every other {@code RuntimeException} is treated as "still live" and the binding survives to be + * probed again next time, which costs nothing worse than one more retry. + */ private boolean isLive(String lead) { try { agents.status(lead); return true; } catch (RuntimeException e) { - log.debug("push: liveness check failed for lead {}: {}", lead, e.toString()); - return false; + boolean gone = e instanceof HerdrException he && "agent_not_found".equals(he.code()); + if (gone) { + log.debug("push: lead {} no longer exists ({})", lead, e.toString()); + } else { + log.debug("push: liveness check for lead {} was inconclusive ({}); treating as live " + + "rather than risk destroying a live binding", lead, e.toString()); + } + return !gone; } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/ReplyPushLoopTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/ReplyPushLoopTest.java index bdb5e2a..2543044 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/msg/ReplyPushLoopTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/ReplyPushLoopTest.java @@ -271,6 +271,39 @@ class ReplyPushLoopTest { "the stale binding must be forgotten even when there is no fallback to hand back"); } + /** + * fleetd #368 review, must-fix: the first version of {@code isLive} treated any + * {@code RuntimeException} from the liveness probe as "the lead is gone" — indistinguishable + * from a transient herdr hiccup (a socket blip, a decode error) on a lead that is actually + * still live. The consequence of that misdiagnosis is destructive and permanent + * ({@code forgetDelegation}), which is the exact #359 mistake repeated two days later: a single + * bad reading must never destroy a live binding. This pins the narrower rule — only an + * affirmative {@code agent_not_found} may forget a binding; a merely inconclusive failure must + * leave the binding alone, and the lead must still be nudged once the probe recovers. + */ + @Test + void aTransientLivenessFailureMustNotForgetABindingToAStillLiveLead() throws Exception { + // OTHER_PRIMARY is delegated to and genuinely live — its FIRST agent.get call fails with a + // transient, non-agent_not_found HerdrException (a transport-level failure, code null, + // exactly what a socket blip looks like), then succeeds on every call after. + registry.recordDelegation(WORKER, OTHER_PRIMARY); + + var rec = new FlakyThenLiveHerdrClient(OTHER_PRIMARY); + agents = new AgentControl(rec); + inbox.publish(WORKER, "m1", "hello"); + + loop(2, 50).onReplyQueued(WORKER); + + assertTrue(rec.sendLatch.await(3, TimeUnit.SECONDS), + "the nudge must still reach the live lead once the transient failure clears"); + assertEquals(List.of(OTHER_PRIMARY), rec.promptTargets(), + "the nudge must go to the lead that was only transiently unreachable, not the " + + "unrelated pinned primary"); + assertEquals(OTHER_PRIMARY, registry.nudgeTargetFor(WORKER).orElseThrow(), + "a merely transient failure must not forget the binding to a lead that is actually " + + "still live"); + } + // --- nudge format -------------------------------------------------------------------------- @Test @@ -1212,4 +1245,51 @@ class ReplyPushLoopTest { public void close() { } } + + /** + * Fake herdr client for fleetd #368 review: {@code flakyTarget}'s FIRST {@code agent.get} call + * fails with a transient, non-{@code agent_not_found} {@code HerdrException} — a transport-level + * failure (code {@code null}), exactly what a socket blip or a decode error on a perfectly live + * lead looks like — then succeeds ({@code idle}) on every call after. Used to prove a merely + * inconclusive failure must not be treated as the lead being gone. + */ + private static final class FlakyThenLiveHerdrClient implements HerdrClient { + private final String flakyTarget; + private final AtomicInteger getCalls = new AtomicInteger(); + private final List promptTargets = Collections.synchronizedList(new ArrayList<>()); + volatile CountDownLatch sendLatch = new CountDownLatch(1); + + FlakyThenLiveHerdrClient(String flakyTarget) { + this.flakyTarget = flakyTarget; + } + + @Override + @SuppressWarnings("unchecked") + public JsonNode call(String method, Object params) { + Map p = params instanceof Map ? (Map) params : Map.of(); + if ("agent.get".equals(method)) { + String target = String.valueOf(p.get("target")); + if (flakyTarget.equals(target) && getCalls.getAndIncrement() == 0) { + throw new HerdrException("herdr socket read timed out"); // transport failure, code == null + } + return MAPPER.createObjectNode() + .set("agent", MAPPER.createObjectNode() + .put("terminal_id", target) + .put("agent_status", "idle")); + } + if ("agent.prompt".equals(method)) { + promptTargets.add(String.valueOf(p.get("target"))); + sendLatch.countDown(); + } + return MAPPER.createObjectNode(); + } + + List promptTargets() { + return List.copyOf(promptTargets); + } + + @Override + public void close() { + } + } }