From c325054242201e3d7d93b5c4de91d543c13c9dec Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 3 Sep 2026 10:22:31 +0700 Subject: [PATCH] fleetd #234 round 2: fix the ExhaustionSink forwarding hop Fleetd.java actually uses The round-1 fix was dead on the real production path. Fleetd.java:177 builds a forwarding sink (needed because the adapters are constructed before `sessions` exists, breaking a genuine cycle) as a LAMBDA: ExhaustionSink forwardingExhaustionSink = (target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason); A lambda can only implement the interface's one abstract method (the 2-arg overload), so it silently inherited the 3-arg overload's default body, which drops the profile hint and calls back into the 2-arg method. OpenCodeLauncher is constructed with this forwarder, so the hint it supplies (its own already-known profile name) was thrown away before it ever reached the real sink built later in Fleetd.main -- reproducing the exact silent no-op round 1 was sent to fix. The 1127 tests from round 1 all injected a sink directly into OpenCodeLauncher and never went through this forwarding hop, so none of them could see it. Fix: forwardingExhaustionSink is now an anonymous class overriding both overloads, each delegating to whatever exhaustionSinkRef currently holds. Audited every other ExhaustionSink value in main/: the only other one is ExhaustionSink.none() (a lambda), which is safe regardless of arity since both its 2-arg body and the inherited 3-arg default are true no-ops. New tests: - ExhaustionSinkForwardingHazardTest: isolates the hazard at the interface level (a lambda forwarder drops the hint; an anonymous-class forwarder does not), independent of Fleetd.java's specific wiring. - OpenCodeLauncherTest#theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop: replicates Fleetd.java's actual construction order (forwarder built and handed to the launcher first, real sink built and pointed at via the AtomicReference afterward) and drives the quarantine through it via the real SessionManager.acquire() path. Both proven by mutation: temporarily rewriting each fixed forwarder back into the pre-fix lambda makes its test fail with a real assertion message (both matched exactly: "expected: but was: " for the interface proof, "expected: but was: " for the composed-wiring test); restoring makes it pass again. No reverts were committed. mvn clean install: Tests run: 1130, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 20 +++- .../ExhaustionSinkForwardingHazardTest.java | 96 +++++++++++++++++++ .../fleet/member/OpenCodeLauncherTest.java | 77 +++++++++++++++ 3 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index a8eed1b..5580446 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -174,7 +174,25 @@ public final class Fleetd { // genuine cycle. Break it exactly like liveCountRef below: a forwarding sink built now, // pointed at the real one once it exists. AtomicReference exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none()); - ExhaustionSink forwardingExhaustionSink = (target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason); + // fleetd #234: this MUST be an anonymous class, not a lambda. A lambda can only implement + // the interface's single abstract method (the 2-arg overload) — it then inherits the 3-arg + // method's DEFAULT body, which drops the profile hint and calls back into the 2-arg + // overload here, so OpenCodeLauncher's hint (its own already-known profile name, passed + // for exactly the reason explained below at the real sink's construction) never reaches + // the real sink at all. That silently reproduced the very bug this hint exists to fix: a + // lambda here means the roster-miss path always fires, even after the hint was supplied. + // Both overloads must forward explicitly to whatever exhaustionSinkRef currently holds. + ExhaustionSink forwardingExhaustionSink = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + exhaustionSinkRef.get().onExhausted(target, reason); + } + + @Override + public void onExhausted(String target, String reason, String profile) { + exhaustionSinkRef.get().onExhausted(target, reason, profile); + } + }; // The claude-code adapter is the always-present default; keep it even with no profiles (so a // bridge configured with no workers, or opencode-only, still has a well-defined base adapter) // unless opencode is the only kind configured. diff --git a/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java b/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java new file mode 100644 index 0000000..c788234 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java @@ -0,0 +1,96 @@ +package dev.ltms.fleet.inject; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * fleetd #234 follow-up: a lambda implementing {@link ExhaustionSink} can only ever implement the + * interface's single abstract method — the 2-arg {@link ExhaustionSink#onExhausted(String, String)} + * — so it silently inherits the 3-arg overload's {@code default} body, which drops whatever profile + * hint a caller supplied and calls back into the 2-arg method instead. {@code Fleetd.main} built + * exactly this shape at {@code Fleetd.java:177} — a forwarding sink standing in for the real one + * until {@code sessions} exists (a genuine construction-order cycle) — as a lambda, so every hint + * {@link dev.ltms.fleet.member.OpenCodeLauncher} passed through it was thrown away before it ever + * reached the real sink. The fix (defect 2, round 2) reported ERROR and correctly declined to + * quarantine in that situation — which is exactly why the earlier {@code OpenCodeLauncherTest} + * mutation tests, which inject a sink directly into the launcher and never go through this + * forwarding hop, could not see the bug: the hop itself was the defect. + * + *

This test pins the hazard at the interface level, independent of {@code Fleetd.java}'s + * specific wiring: ANY forwarding sink standing in front of another {@link ExhaustionSink} must be + * an anonymous class (or otherwise override both overloads) — a lambda there is a silent regression + * of this exact bug. See {@code OpenCodeLauncherTest#theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop} + * for the composed, production-shaped reproduction (launcher mismatch → forwarder → real sink → + * quarantine). + */ +class ExhaustionSinkForwardingHazardTest { + + @Test + void aLambdaForwarderDropsTheProfileHintBeforeItReachesTheRealSink() { + AtomicReference hintSeenByRealSink = new AtomicReference<>("NEVER CALLED"); + ExhaustionSink real = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + onExhausted(target, reason, null); + } + + @Override + public void onExhausted(String target, String reason, String profileHint) { + hintSeenByRealSink.set(profileHint); + } + }; + + AtomicReference ref = new AtomicReference<>(ExhaustionSink.none()); + // The broken shape: a lambda can only implement the 2-arg method. + ExhaustionSink brokenForwarder = (target, reason) -> ref.get().onExhausted(target, reason); + ref.set(real); + + brokenForwarder.onExhausted("term_x", "model mismatch", "gx"); + + assertNull(hintSeenByRealSink.get(), + "documents the hazard: a lambda forwarder can only implement the 2-arg overload, so " + + "it forwards through that overload alone — the real sink's 3-arg method " + + "still runs (via its own default), but with the hint already discarded"); + } + + @Test + void profileHintSurvivesTheForwardingHopUsedInProduction() { + AtomicReference hintSeenByRealSink = new AtomicReference<>("NEVER CALLED"); + ExhaustionSink real = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + onExhausted(target, reason, null); + } + + @Override + public void onExhausted(String target, String reason, String profileHint) { + hintSeenByRealSink.set(profileHint); + } + }; + + AtomicReference ref = new AtomicReference<>(ExhaustionSink.none()); + // The fixed shape (fleetd #234, matching Fleetd.java:177): an anonymous class overriding + // BOTH overloads, each forwarding to the currently-held sink. + ExhaustionSink forwarding = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + ref.get().onExhausted(target, reason); + } + + @Override + public void onExhausted(String target, String reason, String profile) { + ref.get().onExhausted(target, reason, profile); + } + }; + ref.set(real); + + forwarding.onExhausted("term_x", "model mismatch", "gx"); + + assertEquals("gx", hintSeenByRealSink.get(), + "the profile hint must reach the real sink through the forwarding hop"); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index bdde1e5..bb6a563 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -1286,4 +1286,81 @@ class OpenCodeLauncherTest { "a roster-only sink cannot see this target yet — the quarantine silently never " + "happens, which is exactly fleetd #234 defect 2"); } + + /** + * fleetd #234, round 2: the two tests above inject a sink DIRECTLY into {@link OpenCodeLauncher}, + * which is not what {@code Fleetd.main} actually does. Production has one more hop: {@code + * Fleetd.java} builds the adapters (including {@code OpenCodeLauncher}) before {@code sessions} + * exists — a genuine construction-order cycle — so it hands the launcher a forwarding + * sink pointed at an {@code AtomicReference}, and only later {@code .set(...)}s + * that reference to the real sink once {@code sessions} is built. That forwarding sink was + * written as a lambda ({@code Fleetd.java:177}), which can only implement the 2-arg overload — + * it silently inherited the 3-arg method's default body, dropping {@link OpenCodeLauncher}'s + * profile hint on every call, so the roster-miss branch fired even after the hint fix above + * shipped. This test reproduces that exact shape (construct with the forwarder, {@code .set()} + * the real sink afterward, same order as {@code Fleetd.main}) and would fail if the forwarder + * were ever written as a lambda again — see the class javadoc on {@code + * ExhaustionSinkForwardingHazardTest} for the isolated proof of the underlying mechanism. + */ + @Test + void theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + FleetConfig.Profile cfg = opencodeCfgWithCredential( + "terra", "opencode/nemotron-3-ultra-free", "openai-shared"); + Map profiles = Map.of(cfg.profile(), cfg); + BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.SECONDS.toNanos(1800)); + + // Fleetd.java:176 — the forwarding sink is built BEFORE the real one can exist, and the + // launcher below is constructed against this forwarder, exactly like Fleetd.main. + java.util.concurrent.atomic.AtomicReference exhaustionSinkRef = + new java.util.concurrent.atomic.AtomicReference<>(ExhaustionSink.none()); + // Fleetd.java:177 fixed shape — an anonymous class forwarding BOTH overloads. Rewriting + // this as `(target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason)` is + // exactly the regression this test exists to catch. + ExhaustionSink forwardingExhaustionSink = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + exhaustionSinkRef.get().onExhausted(target, reason); + } + + @Override + public void onExhausted(String target, String reason, String profile) { + exhaustionSinkRef.get().onExhausted(target, reason, profile); + } + }; + + FakeHerdr herdr = new FakeHerdr(); + OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, forwardingExhaustionSink); + SessionManager sessions = new SessionManager(launcher); + + // Fleetd.java:359-390 — the real sink is only built and pointed to AFTER `sessions` exists, + // same order as production. + ExhaustionSink realSink = new ExhaustionSink() { + @Override + public void onExhausted(String target, String reason) { + onExhausted(target, reason, null); // no roster resolution modelled — mirrors a miss + } + + @Override + public void onExhausted(String target, String reason, String profileHint) { + FleetConfig.Profile profile = profileHint == null ? null : profiles.get(profileHint); + if (profile != null) { + quarantine.quarantine(profile.effectiveCredentialId()); + } + } + }; + exhaustionSinkRef.set(realSink); + + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + + assertFalse(quarantine.isQuarantined("openai-shared"), "nothing quarantined before the spawn"); + + MemberSession acquired = sessions.acquire(cfg.profile(), "/work/dir", null, null); + + assertEquals("ses_x", acquired.agentSessionId(), "the id itself still resolves correctly"); + assertTrue(quarantine.isQuarantined("openai-shared"), + "the profile hint must survive the Fleetd-style forwarding hop and reach the real " + + "sink — a lambda forwarder drops it and this must go red"); + } }