From 2159a5a94a1efb690bdce5fbc9c2dbd2378fb789 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 13:52:29 +0700 Subject: [PATCH] #315: FixedPlacementPolicy now honors the retry loop's unreachable set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CompositePeerLauncher.spawn retries a failed candidate on the next one and rebuilds PlacementContext "so the policy excludes this profile" (its own comment), but FixedPlacementPolicy.select never read ctx.unreachable(). Under the default `fixed` placement policy (used when `placement` is unset or set to `fixed`), every retry re-picked the same dead default and a second, healthy, configured profile was never tried. This also covers the wiring-bug branch (a candidate profile with no owning adapter), which hit the exact same symptom for the same reason. Not live on this fleet: fleetd.yaml sets placement: weighted, which already consults ctx.unreachable() via PlacementPolicyUtil.available(). This is live only for a deployment that leaves placement unset or sets it to fixed. Fix is in FixedPlacementPolicy: consult ctx.unreachable() in the same two places it already consults quarantined/coolingOff (the default check and the fallback walk over candidates()), and add a fourth reason to the "no candidate remains" exception. Considered fixing this in CompositePeerLauncher's retry loop instead (break when select() returns an already-unreachable profile), but that only fails faster on the same dead profile — it cannot make the loop advance to a different candidate, because only the policy decides which candidate is next. The defect is that one policy implementation does not honor the loop's stated contract, so the fix belongs in that policy, matching how weighted/round-robin already behave. Also fixed: the "no reachable worker profile" exception message said "trying N candidate(s)" where N was unreachable.size(), a count of DISTINCT profiles (a HashSet dedupes a profile added twice), under wording that reads as a count of attempts. Reworded to "N distinct candidate(s)" so the count matches what is measured and the profile list that follows it. Tests: two new failover tests next to the three existing ones in CompositePeerLauncherTest (which all use PlacementPolicies.weighted(), which is why this had no coverage) — one pinned to PlacementPolicies.fixed() for the unreachable-default case, one for the wiring-bug (no adapter) case. Mutation-proofed: reverted FixedPlacementPolicy.java, both new tests failed with the exact bug ("no reachable worker profile available after trying 1 distinct candidate(s): a" / "...c"), then restored the fix. --- .../fleet/member/CompositePeerLauncher.java | 5 +- .../fleet/placement/FixedPlacementPolicy.java | 26 +++++--- .../member/CompositePeerLauncherTest.java | 61 +++++++++++++++++++ 3 files changed, 84 insertions(+), 8 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index 80d0237..f28f23f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -385,9 +385,12 @@ public final class CompositePeerLauncher implements PeerLauncher { } } + // unreachable.size() counts DISTINCT profiles, not attempts (a HashSet dedupes a profile + // added twice) — say "distinct" so the count matches the sentence and the profile list that + // follows, rather than reading as a count of attempts made (fleetd #315). throw new PeerUnreachableException( "no reachable worker profile available after trying " + unreachable.size() - + " candidate(s): " + String.join(", ", unreachable)); + + " distinct candidate(s): " + String.join(", ", unreachable)); } /** diff --git a/fleetd/src/main/java/dev/ltms/fleet/placement/FixedPlacementPolicy.java b/fleetd/src/main/java/dev/ltms/fleet/placement/FixedPlacementPolicy.java index b56635d..cfa3bbb 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/placement/FixedPlacementPolicy.java +++ b/fleetd/src/main/java/dev/ltms/fleet/placement/FixedPlacementPolicy.java @@ -8,7 +8,7 @@ import java.util.List; * profile, exactly as {@code CompositePeerLauncher} did before CB-518. This ignores caps and * reachability so that a pre-existing config behaves identically after upgrade. * - *

Three exceptions walk past the default instead of returning it unconditionally: + *

Four exceptions walk past the default instead of returning it unconditionally: *

- * A fleet where nothing is ever quarantined, cooling off, or weight-0 never exercises any of these - * paths, so today's behaviour is unchanged. + * A fleet where nothing is ever quarantined, cooling off, unreachable, or weight-0 never exercises + * any of these paths, so today's behaviour is unchanged — in particular, the very first selection + * of a spawn call always sees an empty {@code unreachable} set, so the first choice is untouched. */ final class FixedPlacementPolicy implements PlacementPolicy { @@ -30,12 +38,12 @@ final class FixedPlacementPolicy implements PlacementPolicy { public PlacementCandidate select(PlacementContext ctx) { String d = ctx.defaultProfile(); if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !ctx.coolingOff().contains(d) - && !weightExcluded(ctx, d)) { + && !ctx.unreachable().contains(d) && !weightExcluded(ctx, d)) { return new PlacementCandidate(d, null, 1.0f, null); } for (PlacementCandidate c : ctx.candidates()) { if (!ctx.quarantined().contains(c.profile()) && !ctx.coolingOff().contains(c.profile()) - && !c.excluded()) { + && !ctx.unreachable().contains(c.profile()) && !c.excluded()) { return new PlacementCandidate(c.profile(), null, c.weight(), c.maxLoad()); } } @@ -44,8 +52,9 @@ final class FixedPlacementPolicy implements PlacementPolicy { // Exhaustion quarantine takes priority: reported only when quarantine is absent, so the // message never claims "cooling off" for a profile that is really backend-exhausted. boolean dCoolingOff = !dQuarantined && ctx.coolingOff().contains(d); + boolean dUnreachable = ctx.unreachable().contains(d); boolean dWeightExcluded = weightExcluded(ctx, d); - if (dQuarantined || dCoolingOff || dWeightExcluded) { + if (dQuarantined || dCoolingOff || dUnreachable || dWeightExcluded) { List reasons = new ArrayList<>(); if (dQuarantined) { reasons.add("is quarantined (backend exhausted)"); @@ -53,6 +62,9 @@ final class FixedPlacementPolicy implements PlacementPolicy { if (dCoolingOff) { reasons.add("is cooling off after repeated backend errors"); } + if (dUnreachable) { + reasons.add("is unreachable"); + } if (dWeightExcluded) { reasons.add("has weight 0 (excluded from automatic selection)"); } @@ -62,7 +74,7 @@ final class FixedPlacementPolicy implements PlacementPolicy { } if (!ctx.candidates().isEmpty()) { throw new PlacementException("all worker profiles are excluded from automatic " - + "selection (quarantined, cooling off, or weight-0)"); + + "selection (quarantined, cooling off, unreachable, or weight-0)"); } throw new PlacementException("no worker profiles configured"); } 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 b50d0cd..7520cbc 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -615,6 +615,67 @@ class CompositePeerLauncherTest { assertEquals(1, adapter.spawnCount("b")); } + /** + * fleetd #315: {@code CompositePeerLauncher.spawn} rebuilds the {@link PlacementContext} after + * every failed attempt "so the policy excludes this profile" (see the comment at the retry call + * site) — but {@code FixedPlacementPolicy} never read {@code ctx.unreachable()}, so under the + * default {@code fixed} placement every retry re-picked the same dead default and a second, + * healthy, configured profile was never tried. This is the same scenario as + * {@link #failoverRetriesNextCandidateWhenProfileIsUnreachable}, but pinned to {@code fixed()} + * instead of {@code weighted()} — the three existing failover tests all use {@code weighted()}, + * which is exactly why nobody caught this: the retry loop's contract has no coverage under its + * own default policy. + */ + @Test + void failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable() { + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "a", stubWorker("a"), + "b", stubWorker("b")); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a")); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 0); + + PeerHandle h = composite.spawn(new SpawnRequest(null, null, null)); + assertEquals("b", h.profile(), + "fixed placement must fail over from the unreachable default a to the healthy b"); + assertEquals(1, adapter.spawnCount("a"), "a was tried once and failed"); + assertEquals(1, adapter.spawnCount("b"), "b was tried once and succeeded"); + } + + /** + * fleetd #315: the same fix — {@code FixedPlacementPolicy} consulting {@code ctx.unreachable()} + * — also covers the wiring-bug branch in {@code CompositePeerLauncher.spawn}: a profile that + * placement is allowed to choose (it is in the configured candidate list) but that no delegate + * declares ({@code byProfile.get(chosen.profile()) == null}). That branch adds the profile to + * {@code unreachable} and {@code continue}s without ever calling a launcher, so before this fix + * {@code fixed} handed back the same adapterless profile on every remaining attempt too. + */ + @Test + void failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement() { + FakeHerdr herdr = new FakeHerdr(); + // Placement's candidate list has three profiles, in this order (LinkedHashMap preserves it, + // and the fixed default resolves to the first — see the `ordered` helper's own javadoc). + Map profiles = new LinkedHashMap<>(); + profiles.put("c", stubWorker("c")); + profiles.put("a", stubWorker("a")); + profiles.put("b", stubWorker("b")); + // The adapter only declares a and b — c is a configured profile with no owning adapter, + // the "wiring bug" the comment in CompositePeerLauncher.spawn calls out. + Map adapterProfiles = new LinkedHashMap<>(); + adapterProfiles.put("a", stubWorker("a")); + adapterProfiles.put("b", stubWorker("b")); + StubLauncher adapter = new StubLauncher("claude", herdr, adapterProfiles, "a", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 0); + + PeerHandle h = composite.spawn(new SpawnRequest(null, null, null)); + assertEquals("a", h.profile(), + "c has no adapter, so fixed placement must skip it and land on the next candidate, a"); + assertEquals(0, adapter.spawnCount("c"), "c is never spawned — no adapter owns it"); + assertEquals(1, adapter.spawnCount("a")); + } + @Test void explicitSpawnAtMaxLoadThrowsPlacementExceptionNamingProfileLiveAndCap() { FakeHerdr herdr = new FakeHerdr();