From 3f036b2a6299beabaa9f7f102203287cb786c4e5 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 16:41:52 +0700 Subject: [PATCH 1/2] fleetd #444: pin the place()-to-spawn() window PlacementDecision closes Add a test that resolves place(role) while nothing is quarantined, then quarantines the resolved profile's credential BEFORE spawning against the held PlacementDecision. CompositePeerLauncher.spawn(req, decision) must still honor the decision and land on the quarantined profile, since it never re-runs the explicit-profile enforce* checks. Verified the test kills the regression: with the override's body replaced by the re-entering spawn(req.withProfile(...)) form, this exact test goes RED with a PlacementException naming the now- quarantined profile; restored, the full suite is green (1575 tests). Also documents on PeerLauncher's default spawn(req, decision) that a launcher routing across more than one profile MUST override it, naming the four enforce* checks the default's re-entry re-applies. --- .../dev/ltms/fleet/peer/PeerLauncher.java | 13 ++++- .../member/CompositePeerLauncherTest.java | 49 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java index c19b419..7b22325 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java @@ -255,7 +255,18 @@ public interface PeerLauncher { * *

Default implementation for a launcher with no placement concept of its own: delegates to * {@link #spawn(SpawnRequest)} with the decision's profile named explicitly — its only spawn - * contract, since there is no separate routing path to honor. + * contract, since there is no separate routing path to honor. This default is correct ONLY for + * a launcher that spawns a single profile of its own (e.g. {@code HerdrPeerLauncher}), where + * the explicit-profile branch it re-enters and the routing branch {@link #place} would have + * used are the same thing. A launcher that routes across more than one profile — the + * way {@code CompositePeerLauncher} routes across every configured adapter — MUST override + * this method instead of inheriting this default. Re-entering {@link + * #spawn(SpawnRequest)} re-applies that single-argument method's explicit-profile checks + * ({@code enforceNotQuarantined}, {@code enforceNotCoolingOff}, {@code enforceMaxLoad}, {@code + * enforceModelEnabled} in {@code CompositePeerLauncher}), which can refuse the very profile + * {@link #place} just chose, if the underlying placement state moved in the window between the + * {@link #place} call and this one — the exact window this method and {@link PlacementDecision} + * exist to close (fleetd #444). * * @throws IllegalArgumentException if the decision names an unknown profile */ 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 9de90d1..afbe51f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -20,6 +20,7 @@ import dev.ltms.fleet.peer.PeerUnreachableException; import dev.ltms.fleet.peer.SpawnRequest; import dev.ltms.fleet.placement.BackendOutagePolicy; import dev.ltms.fleet.placement.BackendQuarantine; +import dev.ltms.fleet.placement.PlacementDecision; import dev.ltms.fleet.placement.PlacementException; import dev.ltms.fleet.placement.PlacementPolicies; import org.junit.jupiter.api.Test; @@ -1123,6 +1124,54 @@ class CompositePeerLauncherTest { assertEquals(0, adapter.spawnCount("b"), "routedProfileFor never spawns anything"); } + /** + * fleetd #444: {@link PlacementDecision} exists to close the window between {@link + * CompositePeerLauncher#place} and {@link CompositePeerLauncher#spawn(SpawnRequest, + * PlacementDecision)} — the placement state must be free to move in that window without the + * held decision being re-checked against the new state. Every quarantine test above resolves + * and spawns in one call, so none of them ever open that window; this test is the one that + * does: "sol" is placed FIRST, while nothing is quarantined yet, and only THEN is its + * credential quarantined, before the held decision is spawned. + * + *

This is the test that tells the real override apart from the alternative body the ticket + * measured: routing {@code decision.profile()} straight to its adapter (the real override) + * never re-runs {@code enforceNotQuarantined}, so the spawn against the held decision still + * succeeds on sol. Re-entering {@code spawn(req.withProfile(decision.profile()))} instead + * lands in the explicit-profile branch, which refuses a now-quarantined sol outright — before + * this test existed, replacing the real override's body with that re-entering call left the + * whole suite green. + */ + @Test + void spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace() { + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "sol", stubWorker("sol", "shared-openai"), + "b", stubWorker("b")); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "sol", Set.of()); + BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.MINUTES.toNanos(30)); + CompositePeerLauncher composite = new CompositePeerLauncher(List.of(adapter), "sol", profiles, + PlacementPolicies.fixed(), _ -> 0, null, quarantine); + + // 1. Resolve BEFORE anything is quarantined — sol (definition order first, fixed policy) wins. + PlacementDecision decision = composite.place(MemberRole.DEV); + assertEquals("sol", decision.profile(), "sanity: nothing is quarantined yet, so sol is placed"); + + // 2. Move the placement state IN THE WINDOW between place() and spawn() — sol's credential + // is now quarantined. A fresh place()/spawn(req) pair would fall through to b instead; the + // held decision must not be re-evaluated against this new state at all. + quarantine.quarantine("shared-openai"); + + // 3. Spawn against the HELD decision, not a fresh resolve. + SpawnRequest req = new SpawnRequest(null, null, null, null, null, MemberRole.DEV); + PeerHandle handle = composite.spawn(req, decision); + + assertEquals("sol", handle.profile(), + "the decision from place() is honored even though sol is now quarantined"); + assertEquals(1, adapter.spawnCount("sol"), + "the request that reached the delegate actually carried sol as its profile"); + assertEquals(0, adapter.spawnCount("b"), "b must never be touched — the decision named sol"); + } + @Test void aQuarantineLiftsOnTheInjectedClockAndTheProfileBecomesSpawnableAgain() { FakeHerdr herdr = new FakeHerdr(); From e4c703a51af990c26333f74acfc313d3f46d795f Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 16:50:39 +0700 Subject: [PATCH 2/2] fleetd #444: separate the adapter's fallback default from the decided profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the fixture's StubLauncher fell back to 'sol' too — the same profile place() decides — so an UNSTAMPED request could land on spawnCount('sol') by coincidence, and the assertion's claim that the request 'actually carried sol' was unproven. Dropping the stamping (SpawnRequest routedReq = req) while keeping the routing survived the test unchanged. Fix: give the adapter 'b' as its own fallback default instead, so an unstamped request counts against 'b', not 'sol'. Verified both mutations against the single test in isolation: - drop-stamping (routedReq = req): RED, expected but was - re-entering (return spawn(req.withProfile(decision.profile()))) i.e. M1 from the first round: still RED, PlacementException naming the now-quarantined 'sol' Restored both; full suite green at 1575 tests. No changes to src/main — PeerLauncher's javadoc from the first round is unchanged. --- .../member/CompositePeerLauncherTest.java | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) 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 afbe51f..f06d74e 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -1147,12 +1147,17 @@ class CompositePeerLauncherTest { Map profiles = ordered( "sol", stubWorker("sol", "shared-openai"), "b", stubWorker("b")); - StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "sol", Set.of()); + // The adapter's OWN fallback default is "b", deliberately different from the profile place() + // decides ("sol") — see the note below on why this must not be "sol" too. + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "b", Set.of()); BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.MINUTES.toNanos(30)); CompositePeerLauncher composite = new CompositePeerLauncher(List.of(adapter), "sol", profiles, PlacementPolicies.fixed(), _ -> 0, null, quarantine); // 1. Resolve BEFORE anything is quarantined — sol (definition order first, fixed policy) wins. + // composite's own defaultProfile ("sol", the constructor arg above) never enters this: the + // pool poolFor(DEV) resolves to is never empty here, so place() only ever reads that field as + // a fallback for an empty pool, which this test does not exercise. PlacementDecision decision = composite.place(MemberRole.DEV); assertEquals("sol", decision.profile(), "sanity: nothing is quarantined yet, so sol is placed"); @@ -1167,9 +1172,18 @@ class CompositePeerLauncherTest { assertEquals("sol", handle.profile(), "the decision from place() is honored even though sol is now quarantined"); + // A fixture whose adapter falls back to "sol" too would let an UNSTAMPED request (one + // routed but never given req.withProfile("sol")) land on spawnCount("sol") == 1 by + // COINCIDENCE, since StubLauncher.spawn falls back to its own defaultProfile whenever + // req.profileName() is blank. Giving the adapter "b" as its fallback instead means only an + // actually-stamped request can produce this count — an unstamped one would count against + // "b" and this assertion would fail. assertEquals(1, adapter.spawnCount("sol"), - "the request that reached the delegate actually carried sol as its profile"); - assertEquals(0, adapter.spawnCount("b"), "b must never be touched — the decision named sol"); + "the request that reached the delegate actually carried sol as its profile " + + "(the adapter's own fallback default is 'b', so this can't happen by accident)"); + assertEquals(0, adapter.spawnCount("b"), + "b must never be touched — neither as the decision's profile nor as an unstamped " + + "request's accidental fallback"); } @Test