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..f06d74e 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,68 @@ 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")); + // 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"); + + // 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"); + // 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 " + + "(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 void aQuarantineLiftsOnTheInjectedClockAndTheProfileBecomesSpawnableAgain() { FakeHerdr herdr = new FakeHerdr();