From 3f036b2a6299beabaa9f7f102203287cb786c4e5 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 16:41:52 +0700 Subject: [PATCH] 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();