Merge #447: pin the place()-to-spawn() window PlacementDecision closes (fleetd #444)

Verified by the lead on the exact tree that lands (head e4c703a, base 82fae94 is an ancestor, so this is the tree I measured): FULL BUILD Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS compile errors: 0 CONTROL CompositePeerLauncherTest Tests run: 76, Failures: 0 -> GREEN M1 the 2-arg spawn re-enters the 1-arg spawn (the inherited default this ticket forbids for a multi-profile launcher) -> KILLED Errors: 1 CompositePeerLauncherTest .spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace M2 drop the stamping: route to the decided profile but do not carry it (SpawnRequest routedReq = req) -> KILLED Failures: 1 same test method Both mutations proved applied by printing the mutated method, and the tree was restored clean after each (git status --porcelain empty). Round 1 of this PR had a fixture weakness I found by mutation: StubLauncher's own fallback default was "sol", the same profile place() decides, so an unstamped request landed on spawnCount("sol") by coincidence and M2 survived. e4c703a gives the adapter "b" as its fallback instead. One fixture now kills both mutations. src/main is javadoc-only in this PR: 12 added lines, 0 added code lines, measured by filtering the main-side diff.
2026-09-10 12:00:59 +02:00
parent 82fae94c55 e4c703a51a
commit 822327eed5
2 changed files with 75 additions and 1 deletions
@@ -255,7 +255,18 @@ public interface PeerLauncher {
     *
     * <p>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. <strong>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.</strong> 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
     */
@@ -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.
+     *
+     * <p>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<String, FleetConfig.Profile> 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();