fleetd #234 round 3: make ExhaustionSink's forwarder a shared factory, not a rebuilt-per-caller shape
Round 2's tests never reached Fleetd.java at all: both new tests declared
their OWN local copy of the forwarding shape instead of calling production's.
Mutating Fleetd.java's real forwarder back into the broken lambda left those
copies untouched, so the whole suite stayed green while production had
regressed to exactly the bug being fixed -- proven live by the reviewer.
Fix: extracted the forwarding shape into one named factory,
ExhaustionSink.forwardingTo(Supplier<ExhaustionSink> target), with the
"why a lambda here is wrong" explanation moved onto it (the one place the
shape is now written). Fleetd.java's forwarder collapses to one line:
ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get);
Both new tests now call this same factory instead of rebuilding an anonymous
class inline, so they exercise the identical object production builds:
- ExhaustionSinkForwardingHazardTest: calls ExhaustionSink.forwardingTo
directly and asserts the hint reaches the real sink through it.
- OpenCodeLauncherTest#theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop:
same factory call, inside the full Fleetd-shaped construction order
(forwarder built first, real sink pointed at via the AtomicReference
afterward), driven through the real SessionManager.acquire() path.
Mutation proof, this time on production code only: deleted the factory's
3-arg override (falls back to the interface default, dropping the hint) --
both new tests go red with no test file touched:
ExhaustionSinkForwardingHazardTest...: expected: <gx> but was: <null>
OpenCodeLauncherTest...ForwardingHop: expected: <true> but was: <false>
Tests run: 68, Failures: 2
Restored, re-ran: green (Tests run: 68, Failures: 0). Confirmed Fleetd.java
carries no lambda ExhaustionSink anywhere (grep). ExhaustionSink.none() stays
a lambda on purpose -- both its overloads are true no-ops regardless of
arity, so there is no hint to drop.
mvn clean install: Tests run: 1129, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
This commit is contained in:
@@ -174,25 +174,12 @@ public final class Fleetd {
|
|||||||
// genuine cycle. Break it exactly like liveCountRef below: a forwarding sink built now,
|
// genuine cycle. Break it exactly like liveCountRef below: a forwarding sink built now,
|
||||||
// pointed at the real one once it exists.
|
// pointed at the real one once it exists.
|
||||||
AtomicReference<ExhaustionSink> exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none());
|
AtomicReference<ExhaustionSink> exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none());
|
||||||
// fleetd #234: this MUST be an anonymous class, not a lambda. A lambda can only implement
|
// fleetd #234, round 3: this MUST go through ExhaustionSink.forwardingTo, never a lambda or
|
||||||
// the interface's single abstract method (the 2-arg overload) — it then inherits the 3-arg
|
// a hand-written anonymous class here — see that factory's javadoc for why a lambda at this
|
||||||
// method's DEFAULT body, which drops the profile hint and calls back into the 2-arg
|
// call site silently drops OpenCodeLauncher's profile hint. Routing through the shared
|
||||||
// overload here, so OpenCodeLauncher's hint (its own already-known profile name, passed
|
// factory also lets a test call the exact same object this line builds, instead of
|
||||||
// for exactly the reason explained below at the real sink's construction) never reaches
|
// asserting a copy of its shape.
|
||||||
// the real sink at all. That silently reproduced the very bug this hint exists to fix: a
|
ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get);
|
||||||
// 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
|
// 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)
|
// bridge configured with no workers, or opencode-only, still has a well-defined base adapter)
|
||||||
// unless opencode is the only kind configured.
|
// unless opencode is the only kind configured.
|
||||||
|
|||||||
@@ -1,5 +1,7 @@
|
|||||||
package dev.ltms.fleet.inject;
|
package dev.ltms.fleet.inject;
|
||||||
|
|
||||||
|
import java.util.function.Supplier;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Notified when {@link CompletionResolver} actually delivers a {@code BACKEND_EXHAUSTED}
|
* 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
|
* 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
|
* 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
|
* 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() {
|
static ExhaustionSink none() {
|
||||||
return (target, reason) -> { };
|
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<ExhaustionSink>} 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.
|
||||||
|
*
|
||||||
|
* <p><strong>This is the one place that forwarding shape is written, on purpose.</strong> 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<ExhaustionSink> 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);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+13
-62
@@ -5,32 +5,21 @@ import org.junit.jupiter.api.Test;
|
|||||||
import java.util.concurrent.atomic.AtomicReference;
|
import java.util.concurrent.atomic.AtomicReference;
|
||||||
|
|
||||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
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
|
* fleetd #234, round 3: calls the REAL production factory, {@link ExhaustionSink#forwardingTo},
|
||||||
* interface's single abstract method — the 2-arg {@link ExhaustionSink#onExhausted(String, String)}
|
* rather than rebuilding its shape locally. A round-2 version of this test built its own copy of
|
||||||
* — so it silently inherits the 3-arg overload's {@code default} body, which drops whatever profile
|
* the forwarder inline — mutating {@code Fleetd.java}'s actual forwarder back into a lambda left
|
||||||
* hint a caller supplied and calls back into the 2-arg method instead. {@code Fleetd.main} built
|
* that copy untouched, so the test kept passing while production had regressed to exactly the bug
|
||||||
* exactly this shape at {@code Fleetd.java:177} — a forwarding sink standing in for the real one
|
* it was meant to catch. Calling the shared factory here means the same object under test IS the
|
||||||
* until {@code sessions} exists (a genuine construction-order cycle) — as a lambda, so every hint
|
* object {@code Fleetd.java} builds via {@code ExhaustionSink.forwardingTo(exhaustionSinkRef::get)}
|
||||||
* {@link dev.ltms.fleet.member.OpenCodeLauncher} passed through it was thrown away before it ever
|
* — a defect in the factory body, or a call site reverting to a hand-written lambda instead of the
|
||||||
* reached the real sink. The fix (defect 2, round 2) reported ERROR and correctly declined to
|
* factory, has exactly one place left to hide, and this test reaches into it.
|
||||||
* 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.
|
|
||||||
*
|
|
||||||
* <p>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 {
|
class ExhaustionSinkForwardingHazardTest {
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void aLambdaForwarderDropsTheProfileHintBeforeItReachesTheRealSink() {
|
void forwardingToDeliversTheProfileHintToWhateverSinkTheSupplierCurrentlyReturns() {
|
||||||
AtomicReference<String> hintSeenByRealSink = new AtomicReference<>("NEVER CALLED");
|
AtomicReference<String> hintSeenByRealSink = new AtomicReference<>("NEVER CALLED");
|
||||||
ExhaustionSink real = new ExhaustionSink() {
|
ExhaustionSink real = new ExhaustionSink() {
|
||||||
@Override
|
@Override
|
||||||
@@ -44,53 +33,15 @@ class ExhaustionSinkForwardingHazardTest {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Same shape Fleetd.java uses: a reference that starts at none() and is repointed later.
|
||||||
AtomicReference<ExhaustionSink> ref = new AtomicReference<>(ExhaustionSink.none());
|
AtomicReference<ExhaustionSink> ref = new AtomicReference<>(ExhaustionSink.none());
|
||||||
// The broken shape: a lambda can only implement the 2-arg method.
|
ExhaustionSink forwarding = ExhaustionSink.forwardingTo(ref::get);
|
||||||
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<String> 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<ExhaustionSink> 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);
|
ref.set(real);
|
||||||
|
|
||||||
forwarding.onExhausted("term_x", "model mismatch", "gx");
|
forwarding.onExhausted("term_x", "model mismatch", "gx");
|
||||||
|
|
||||||
assertEquals("gx", hintSeenByRealSink.get(),
|
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");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1293,14 +1293,16 @@ class OpenCodeLauncherTest {
|
|||||||
* Fleetd.java} builds the adapters (including {@code OpenCodeLauncher}) before {@code sessions}
|
* Fleetd.java} builds the adapters (including {@code OpenCodeLauncher}) before {@code sessions}
|
||||||
* exists — a genuine construction-order cycle — so it hands the launcher a <em>forwarding</em>
|
* exists — a genuine construction-order cycle — so it hands the launcher a <em>forwarding</em>
|
||||||
* sink pointed at an {@code AtomicReference<ExhaustionSink>}, and only later {@code .set(...)}s
|
* sink pointed at an {@code AtomicReference<ExhaustionSink>}, and only later {@code .set(...)}s
|
||||||
* that reference to the real sink once {@code sessions} is built. That forwarding sink was
|
* that reference to the real sink once {@code sessions} is built.
|
||||||
* 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
|
* <p><strong>Round 3:</strong> a round-2 version of this test built its own copy of that
|
||||||
* profile hint on every call, so the roster-miss branch fired even after the hint fix above
|
* forwarder's shape as an anonymous class. Mutating {@code Fleetd.java}'s ACTUAL forwarder back
|
||||||
* shipped. This test reproduces that exact shape (construct with the forwarder, {@code .set()}
|
* into a broken lambda left this test's own copy untouched, so it kept passing while production
|
||||||
* the real sink afterward, same order as {@code Fleetd.main}) and would fail if the forwarder
|
* had regressed to the exact bug being fixed. This version instead calls {@link
|
||||||
* were ever written as a lambda again — see the class javadoc on {@code
|
* ExhaustionSink#forwardingTo}, the same factory {@code Fleetd.java} calls — the identical
|
||||||
* ExhaustionSinkForwardingHazardTest} for the isolated proof of the underlying mechanism.
|
* 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
|
@Test
|
||||||
void theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop(@TempDir Path configRoot,
|
void theSpawnTimeQuarantineSurvivesTheFleetdStyleForwardingHop(@TempDir Path configRoot,
|
||||||
@@ -1311,23 +1313,16 @@ class OpenCodeLauncherTest {
|
|||||||
BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.SECONDS.toNanos(1800));
|
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
|
// 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<ExhaustionSink> exhaustionSinkRef =
|
java.util.concurrent.atomic.AtomicReference<ExhaustionSink> exhaustionSinkRef =
|
||||||
new java.util.concurrent.atomic.AtomicReference<>(ExhaustionSink.none());
|
new java.util.concurrent.atomic.AtomicReference<>(ExhaustionSink.none());
|
||||||
// Fleetd.java:177 fixed shape — an anonymous class forwarding BOTH overloads. Rewriting
|
ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get);
|
||||||
// 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();
|
FakeHerdr herdr = new FakeHerdr();
|
||||||
OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, forwardingExhaustionSink);
|
OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, forwardingExhaustionSink);
|
||||||
|
|||||||
Reference in New Issue
Block a user