diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 5580446..a3fbe44 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -174,25 +174,12 @@ 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()); - // 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); - } - }; + // fleetd #234, round 3: this MUST go through ExhaustionSink.forwardingTo, never a lambda or + // a hand-written anonymous class here — see that factory's javadoc for why a lambda at this + // call site silently drops OpenCodeLauncher's profile hint. Routing through the shared + // factory also lets a test call the exact same object this line builds, instead of + // asserting a copy of its shape. + ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get); // 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/main/java/dev/ltms/fleet/inject/ExhaustionSink.java b/fleetd/src/main/java/dev/ltms/fleet/inject/ExhaustionSink.java index 711e57e..5a61d11 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/inject/ExhaustionSink.java +++ b/fleetd/src/main/java/dev/ltms/fleet/inject/ExhaustionSink.java @@ -1,5 +1,7 @@ package dev.ltms.fleet.inject; +import java.util.function.Supplier; + /** * Notified when {@link CompletionResolver} actually delivers a {@code BACKEND_EXHAUSTED} * classification to a waiting send (CB-578 stage B) — never on a race that lost (see @@ -49,9 +51,52 @@ public interface ExhaustionSink { /** * Inert sink — nothing happens on exhaustion. The explicit stand-in a caller (or a test not * exercising this feature) passes instead of a defaulting overload, exactly like - * {@link ExhaustedPatternLookup#none()}. + * {@link ExhaustedPatternLookup#none()}. Safe as a lambda regardless of arity: its 2-arg body + * is empty, and the inherited 3-arg {@code default} just calls that same empty body — there is + * no hint to drop because this sink does nothing either way. */ static ExhaustionSink none() { return (target, reason) -> { }; } + + /** + * A sink that forwards BOTH overloads to whatever {@code target} currently supplies (fleetd + * #234, round 3). Exists to break a genuine construction-order cycle: {@code Fleetd.main} + * builds its adapters (including {@link dev.ltms.fleet.member.OpenCodeLauncher}) before + * {@code sessions} exists, so it cannot hand them the real sink yet — it hands them a + * forwarder pointed at an {@code AtomicReference} that starts at {@link #none()} + * and gets {@code .set()} to the real sink once {@code sessions} is built. {@code target} is + * evaluated on every call, never cached, so the forwarder keeps working after the reference is + * repointed. + * + *

This is the one place that forwarding shape is written, on purpose. A + * forwarder written directly at a call site as a lambda — + * {@code (t, r) -> target.get().onExhausted(t, r)} — implements only the interface's single + * abstract method (the 2-arg overload) and silently inherits the 3-arg overload's {@code + * default} body, which discards whatever profile hint the real caller supplied and calls back + * into the 2-arg method instead. {@link dev.ltms.fleet.member.OpenCodeLauncher}'s model-mismatch + * check relies on that hint reaching the real sink (it fires before its session is registered + * in the roster, so the roster alone cannot resolve which profile to quarantine) — a lambda + * forwarder here silently reproduces that exact bug. Routing every forwarder through this one + * factory, instead of re-writing the shape at each call site, means a test can pin the shape + * once and have both {@code Fleetd.java} and the test call the identical object — see {@code + * ExhaustionSinkForwardingHazardTest} and {@code OpenCodeLauncherTest} for the coverage this + * makes possible. + * + * @param target supplies the sink to forward to, evaluated fresh on every call + * @return a sink whose every overload delegates to {@code target.get()}'s matching overload + */ + static ExhaustionSink forwardingTo(Supplier target) { + return new ExhaustionSink() { + @Override + public void onExhausted(String t, String r) { + target.get().onExhausted(t, r); + } + + @Override + public void onExhausted(String t, String r, String p) { + target.get().onExhausted(t, r, p); + } + }; + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java b/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java index c788234..6c42381 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/inject/ExhaustionSinkForwardingHazardTest.java @@ -5,32 +5,21 @@ 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). + * fleetd #234, round 3: calls the REAL production factory, {@link ExhaustionSink#forwardingTo}, + * rather than rebuilding its shape locally. A round-2 version of this test built its own copy of + * the forwarder inline — mutating {@code Fleetd.java}'s actual forwarder back into a lambda left + * that copy untouched, so the test kept passing while production had regressed to exactly the bug + * it was meant to catch. Calling the shared factory here means the same object under test IS the + * object {@code Fleetd.java} builds via {@code ExhaustionSink.forwardingTo(exhaustionSinkRef::get)} + * — a defect in the factory body, or a call site reverting to a hand-written lambda instead of the + * factory, has exactly one place left to hide, and this test reaches into it. */ class ExhaustionSinkForwardingHazardTest { @Test - void aLambdaForwarderDropsTheProfileHintBeforeItReachesTheRealSink() { + void forwardingToDeliversTheProfileHintToWhateverSinkTheSupplierCurrentlyReturns() { AtomicReference hintSeenByRealSink = new AtomicReference<>("NEVER CALLED"); ExhaustionSink real = new ExhaustionSink() { @Override @@ -44,53 +33,15 @@ class ExhaustionSinkForwardingHazardTest { } }; + // Same shape Fleetd.java uses: a reference that starts at none() and is repointed later. 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); - } - }; + ExhaustionSink forwarding = ExhaustionSink.forwardingTo(ref::get); 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"); + "ExhaustionSink.forwardingTo must deliver the profile hint to the sink the supplier " + + "currently returns — the exact object Fleetd.java's forwarder is built from"); } } 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 bb6a563..ee67781 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -1293,14 +1293,16 @@ class OpenCodeLauncherTest { * 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. + * that reference to the real sink once {@code sessions} is built. + * + *

Round 3: a round-2 version of this test built its own copy of that + * forwarder's shape as an anonymous class. Mutating {@code Fleetd.java}'s ACTUAL forwarder back + * into a broken lambda left this test's own copy untouched, so it kept passing while production + * had regressed to the exact bug being fixed. This version instead calls {@link + * ExhaustionSink#forwardingTo}, the same factory {@code Fleetd.java} calls — the identical + * object, not a rebuilt copy of its shape — so a regression at either the {@code Fleetd.java} + * call site or inside {@code forwardingTo} itself has nowhere left to hide. See {@code + * ExhaustionSinkForwardingHazardTest} for the same factory exercised in isolation. */ @Test void theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop(@TempDir Path configRoot, @@ -1311,23 +1313,16 @@ class OpenCodeLauncherTest { 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. + // launcher below is constructed against this forwarder, exactly like Fleetd.main. Calling + // the SAME factory Fleetd.java calls — ExhaustionSink.forwardingTo — rather than rebuilding + // the forwarder's shape here is the whole point (fleetd #234, round 3): a round-2 version of + // this test built its own copy, so mutating Fleetd.java's real forwarder back into a lambda + // left this test untouched. Routed through the shared factory, a regression at either the + // Fleetd.java call site (reverting to a hand-written lambda) or inside the factory body + // itself has nowhere left to hide from this test. 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); - } - }; + ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get); FakeHerdr herdr = new FakeHerdr(); OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, forwardingExhaustionSink);