From 9f3671b80147a8a95179e8fb4a3123d1c18a2038 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 14:06:10 +0700 Subject: [PATCH] fleetd #425 rework round 3: rewrite prose after #435 made fixed honour maxLoad MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fleetd #435 (merged to main) made FixedPlacementPolicy evaluate maxLoad during automatic selection, the same way weighted/round-robin already did. Six comments across CompositePeerLauncher.java, PeerLauncher.java, PlacementDecision.java, and SessionManager.java justified round 2's place()/spawn(req, decision) mechanism by saying fixed "deliberately never evaluates maxLoad" — that claim is now false, and needed restating, not just deleting. The honest case after #435: the two-path shape (routing branch falls through an excluded candidate; explicit-profile branch refuses on it) is still real and still deliberate — an operator who names a profile should get a refusal, not a silent substitution. What round 1 got wrong, and what round 2 still needs to prevent, is turning a fall-through into a refusal by accident: resolving a name via place() and then feeding it back to spawn(SpawnRequest) as an explicit profile. Before #435 that accident was reachable through maxLoad specifically, because fixed never evaluated it; #435 closed that specific gap, so a PlacementDecision can no longer be at-cap in the first place. What survives as the justification for spawn(req, decision): it never re-evaluates a condition place() already decided, and it closes the window between that decision and the spawn in which the underlying state could otherwise move — not a failure #435 already prevents. Re-measured the sibling paths this round exists to keep in agreement (one profile at maxLoad: 1, liveCount pinned at 1, PlacementPolicies.fixed(), unqualified spawn): both the with-worktree and without-worktree paths now throw the identical PlacementException — "worker profile 'a' is at maxLoad (1 live >= 1 cap), and no available candidate remains" — closed upstream by #435, at place()/select(), before either path ever reaches a spawn call. The observable asymmetry this PR was filed to fix is gone; what remains is the structural argument above. No behavior change: place()/PlacementDecision/spawn(req, decision) are untouched, and FixedPlacementPolicy/PlacementPolicyUtil are taken wholesale from main's merge. --- .../fleet/member/CompositePeerLauncher.java | 47 +++++++++++++++---- .../dev/ltms/fleet/peer/PeerLauncher.java | 37 +++++++++------ .../fleet/placement/PlacementDecision.java | 29 ++++++++---- .../ltms/fleet/session/SessionManager.java | 46 +++++++++++------- 4 files changed, 110 insertions(+), 49 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 4bd68ed..3a33f43 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -680,13 +680,19 @@ public final class CompositePeerLauncher implements PeerLauncher { *

Deliberately does not apply {@link #enforceMaxLoad} (or any of the other three * {@code enforce*} checks): those belong to {@link #spawn}'s EXPLICIT-profile branch, the * operator-override path, and this method answers a different question — "where would an - * UNQUALIFIED spawn land". Under the default {@code fixed} policy, {@code select} itself never - * looks at {@code maxLoad} for automatic placement (see {@code FixedPlacementPolicy}'s own - * javadoc), so this decision can legitimately name an at-cap profile — round 1 of this fix - * turned that into a hard failure by resolving the name here and then handing it back to {@link - * #spawn(SpawnRequest)} as an explicit profile, which DOES run {@link #enforceMaxLoad}. Round 2 - * fixes that at the caller: {@link #spawn(SpawnRequest, PlacementDecision)} carries this exact - * decision to the spawn without re-resolving or re-checking it. + * UNQUALIFIED spawn land". That is not the same as {@code select} ignoring these conditions — + * every condition {@code select} filters on (quarantine, cooling off, {@code maxLoad} under + * every placement policy including the default {@code fixed}, since fleetd #435, model-off, + * unreachable, weight-0) is already reflected in the {@link PlacementDecision} this method + * returns, because {@code select} walked past every excluded candidate to find it. What this + * method's caller must not do is take that resolved name and hand it back to {@link + * #spawn(SpawnRequest)} as an explicit profile: the explicit-profile branch treats the same + * exclusion conditions as a reason to REFUSE, where {@code select} had already treated them as + * a reason to fall through — round 1 of this fix did exactly that, turning a fall-through this + * method had already resolved around into a refusal one call later. Round 2 fixes that at the + * caller: {@link #spawn(SpawnRequest, PlacementDecision)} carries this exact decision to the + * spawn without re-resolving or re-checking it, through the same routing path {@code select} + * itself was consulted from. * * @throws PlacementException if no candidate in {@code role}'s pool is currently placeable * (mirrors what an actual unqualified spawn would throw) @@ -723,9 +729,30 @@ public final class CompositePeerLauncher implements PeerLauncher { * the EXPLICIT-profile branch's checks, and {@code decision} did not come from an operator * naming a profile — it came from {@link #place}, which already applied whichever of these * conditions {@link PlacementPolicy#select} actually filters on (fleetd #425 rework, round 2). - * Re-running {@link #enforceMaxLoad} here specifically is what regressed round 1: it would - * refuse a profile placement itself just approved, since {@code FixedPlacementPolicy} — the - * default policy — deliberately never evaluates {@code maxLoad} for automatic selection. + * + *

The two branches disagree on purpose about what an excluded profile means, and that + * disagreement is not what this method removes. The blank-profile routing branch (and + * {@link #place}) treats a quarantined/cooling-off/at-cap/model-off/unreachable/weight-0 profile + * as a reason to fall through to the next candidate; the EXPLICIT-profile branch treats naming + * that same profile as a reason to refuse outright — someone who names a profile should get a + * refusal, not a silent substitution onto a different backend. That is still correct after + * fleetd #435. What round 1 got wrong, and what this method exists to stop happening again, is + * turning a fall-through into a refusal by accident: resolving a name via {@link #place} and + * then handing that same name back to {@link #spawn(SpawnRequest)} as an explicit profile takes + * the refusing branch on a decision the routing branch had already approved by falling through + * past everything else. + * + *

Before fleetd #435, this exact accident was reachable through {@code maxLoad} specifically: + * {@code FixedPlacementPolicy} — the default policy — did not evaluate {@code maxLoad} at all + * for automatic selection, so {@link #place} could approve an at-cap profile that {@link + * #enforceMaxLoad} would then refuse one call later. fleetd #435 closed that: {@code + * FixedPlacementPolicy} now walks past an at-cap candidate exactly like {@code weighted}/ + * {@code round-robin} already did, so {@link #place} can no longer return one, and this specific + * failure — an approved placement dying at {@code enforceMaxLoad} — cannot happen any more. + * What this method still buys, now that {@code maxLoad} can no longer cause it: it never + * re-evaluates a condition {@link #place} already decided, and it closes the window between + * that decision and the spawn in which the underlying state (another spawn landing on the same + * profile, a config reload) could otherwise move and make a stale explicit re-check wrong. * *

Deliberately does not retry on {@link PeerUnreachableException} across candidates the way * {@link #spawn(SpawnRequest)}'s blank-profile branch does: retrying here would silently 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 02dab26..c19b419 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java @@ -192,13 +192,19 @@ public interface PeerLauncher { * before the peer exists is the one that matters — must call {@link #place} and carry the * {@link PlacementDecision} itself through to {@link #spawn(SpawnRequest, PlacementDecision)} * instead of calling this method and feeding the string back in as an explicit profile. Doing - * that re-enters {@link #spawn(SpawnRequest)}'s explicit-profile branch and its enforcement - * checks (quarantine/cool-off/{@code maxLoad}/model-off) — a branch an unqualified spawn's - * routing side does not uniformly run, and one of those checks ({@code maxLoad} under the - * default {@code fixed} policy) placement never evaluates at all. That is exactly the - * regression fleetd #425 rework round 2 fixes: default implementation below delegates to - * {@link #place}, so the two can never drift apart, but a caller that resolves through this - * method alone and spawns separately can still recreate the round-1 defect for itself. + * that re-enters {@link #spawn(SpawnRequest)}'s explicit-profile branch, which disagrees with + * the routing branch on purpose about what an excluded profile means: the routing branch (and + * {@link #place}) falls through a quarantined/cooling-off/at-cap/model-off/unreachable/weight-0 + * profile to the next candidate, while the explicit branch refuses outright — correct for an + * operator who named that profile on purpose, wrong for a name that only ever came from placement + * itself. That accidental refusal is exactly the regression fleetd #425 rework round 2 fixes: + * the default implementation below delegates to {@link #place}, so the two can never drift apart, + * but a caller that resolves through this method alone and spawns separately can still recreate + * the round-1 defect for itself. (Before fleetd #435, this accident was also reachable through + * {@code maxLoad} specifically, because {@code FixedPlacementPolicy} — the default policy — did + * not evaluate it at all for automatic selection; #435 closed that gap, so a placement decision + * can no longer be at cap in the first place. The refusal-vs-fall-through disagreement above is + * the part that was never about {@code maxLoad} and is still real.) * * @throws RuntimeException (implementation-specific, typically a placement exception) if no * candidate in {@code role}'s pool is currently placeable @@ -231,13 +237,16 @@ public interface PeerLauncher { /** * Spawn against an already-resolved {@link PlacementDecision} from {@link #place}, honoring it - * completely: none of the conditions a real unqualified {@link #spawn(SpawnRequest)} call would - * apply (or, for {@code maxLoad} under the default {@code fixed} policy, deliberately would - * not) are re-evaluated here — {@code decision} already reflects them. This is what lets a - * resolve-then-spawn caller ({@code SessionManager.acquireWithWorktree}, which must know the - * profile before it can provision a worktree for it) and a plain blank-profile {@link - * #spawn(SpawnRequest)} caller land on the exact same outcome for the exact same placement - * state (fleetd #425 rework, round 2). + * completely: none of the conditions {@link #place} already applied — quarantine, cooling off, + * {@code maxLoad} (evaluated by every placement policy including the default {@code fixed}, + * since fleetd #435), model-off — are re-evaluated here; {@code decision} already reflects them. + * This is not skipping a check {@code place} left undone; it is not repeating one {@code place} + * already did, and not re-opening the window between that decision and this spawn in which the + * underlying state could otherwise move. This is what lets a resolve-then-spawn caller + * ({@code SessionManager.acquireWithWorktree}, which must know the profile before it can + * provision a worktree for it) and a plain blank-profile {@link #spawn(SpawnRequest)} caller + * land on the exact same outcome for the exact same placement state (fleetd #425 rework, + * round 2). * *

{@code req}'s own {@link SpawnRequest#profileName()} is ignored in favor of {@code * decision.profile()} — the caller is expected to have built {@code req} with a blank or diff --git a/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java b/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java index 1f6fdea..caa7763 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java +++ b/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java @@ -17,18 +17,29 @@ import dev.ltms.fleet.peer.SpawnRequest; * a profile explicitly makes {@code CompositePeerLauncher.spawn} take its THROWING branch * ({@code enforceNotQuarantined}/{@code enforceNotCoolingOff}/{@code enforceMaxLoad}/{@code * enforceModelEnabled}), while an unqualified spawn's ROUTING branch never runs those checks at - * all — and, under the default {@code fixed} placement policy, deliberately never evaluates {@code - * maxLoad} for automatic selection in the first place. So a profile placement itself just approved - * could still die at {@code enforceMaxLoad} one call later, purely because the caller's route to - * the spawn passed through an explicit profile name instead of the routing branch — a NEW failure - * a worktree-less unqualified spawn would never hit. + * all — it instead FALLS THROUGH to the next candidate on exactly the same conditions the throwing + * branch refuses on. That disagreement is deliberate: an operator who names a profile should get a + * refusal, not a silent substitution. The bug is turning the fall-through into a refusal by + * accident — resolving a name through the routing side and then re-entering the refusing side with + * it, for a decision the routing side had already approved by walking past everything else. + * Before fleetd #435, this accident was also reachable through {@code maxLoad} specifically: the + * default {@code fixed} placement policy did not evaluate {@code maxLoad} at all for automatic + * selection, so a profile placement itself just approved could still die at {@code enforceMaxLoad} + * one call later, purely because the caller's route to the spawn passed through an explicit + * profile name instead of the routing branch — a failure a worktree-less unqualified spawn would + * never hit. fleetd #435 closed that specific gap ({@code fixed} now evaluates {@code maxLoad} + * exactly like every other placement policy), so a {@link PlacementDecision} can no longer be + * at-cap in the first place — but the refusal-vs-fall-through disagreement above was never about + * {@code maxLoad}, and resolving a name and re-entering the refusing branch with it is still wrong + * for every OTHER condition placement filters on. * *

{@link PeerLauncher#spawn(SpawnRequest, PlacementDecision)} closes that by spawning through * the identical code path the routing branch itself uses, keyed off the SAME decision {@link - * PeerLauncher#place} returned — no re-checking of any condition placement already evaluated (or, - * for {@code maxLoad} under {@code fixed}, deliberately did not). A resolve-then-spawn caller and a - * blank-profile {@link PeerLauncher#spawn(SpawnRequest)} caller can then never disagree about - * which conditions apply to the same placement state. + * PeerLauncher#place} returned — no re-checking of any condition placement already evaluated. A + * resolve-then-spawn caller and a blank-profile {@link PeerLauncher#spawn(SpawnRequest)} caller can + * then never disagree about which conditions apply to the same placement state, and neither one + * re-opens the window between the placement decision and the spawn in which the underlying state + * could otherwise move. * * @param profile the profile this decision resolved to (may be {@code null} only when no profile is * configured at all — the same corner case {@link PeerLauncher#defaultProfile()} diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java index d9e9687..9831d32 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -601,22 +601,36 @@ public final class SessionManager implements TurnListener { // under weighted/round-robin placement. // // Round 1 of this rework fed the resolved name back into launcher.spawn(SpawnRequest) as an - // EXPLICIT profile. That was a mistake this round corrects: naming a profile explicitly makes - // CompositePeerLauncher.spawn take its THROWING branch (enforceNotQuarantined/ - // enforceNotCoolingOff/enforceMaxLoad/enforceModelEnabled), while the routing branch a blank - // spawn takes never runs those checks — and, under the default `fixed` placement policy, - // deliberately never evaluates maxLoad for automatic selection at all. So an at-cap pool-first - // profile that placement itself would have picked for a plain unqualified spawn could die at - // enforceMaxLoad one call later, purely because this method's route to the spawn passed - // through an explicit profile name — a failure a worktree-less unqualified spawn never hits. - // This round closes that by keeping the PlacementDecision from place() and handing it to - // launcher.spawn(SpawnRequest, PlacementDecision) for an unqualified request, which spawns - // through the SAME routing branch a blank spawn uses — no enforce* check is newly applied, - // and maxLoad stays exactly as unenforced here as it is on main today (fleetd #435, not this - // ticket, owns whether that is correct). An explicitly-named profile still goes through - // launcher.spawn(SpawnRequest) and its throwing branch, unchanged — that caller asked for one - // profile by name and still gets everything enforceNotQuarantined/enforceNotCoolingOff/ - // enforceMaxLoad/enforceModelEnabled decide about it. + // EXPLICIT profile. That was a mistake this round corrects, and the mistake is not that the + // two branches apply different checks — they are SUPPOSED to disagree: the routing branch a + // blank spawn takes treats a quarantined/cooling-off/at-cap/model-off/unreachable/weight-0 + // profile as a reason to fall through to the next candidate, while CompositePeerLauncher's + // THROWING branch (enforceNotQuarantined/enforceNotCoolingOff/enforceMaxLoad/ + // enforceModelEnabled) treats naming that same profile explicitly as a reason to refuse + // outright. That is correct: an operator who names a profile should get a refusal, not a + // silent substitution onto a different backend. The mistake was turning a fall-through into + // a refusal by accident — resolving a name via the routing side and then re-entering the + // refusing side with it, for a placement the routing side had already approved by walking + // past everything else. + // + // Before fleetd #435, this accident was reachable through maxLoad specifically: the default + // `fixed` placement policy did not evaluate maxLoad at all for automatic selection, so an + // at-cap pool-first profile that placement itself would have picked for a plain unqualified + // spawn could die at enforceMaxLoad one call later, purely because this method's route to + // the spawn passed through an explicit profile name — a failure a worktree-less unqualified + // spawn never hit. fleetd #435 closed that gap (`fixed` now evaluates maxLoad exactly like + // every other placement policy), so that specific failure can no longer happen — a + // PlacementDecision this method resolves can no longer be at-cap in the first place. What + // this round's fix still buys, now that maxLoad can no longer cause the accident: it keeps + // the PlacementDecision from place() and hands it to launcher.spawn(SpawnRequest, + // PlacementDecision) for an unqualified request, which spawns through the SAME routing + // branch a blank spawn uses — no enforce* check is newly applied, and the window between the + // placement decision and the spawn (in which the pool, a config reload, or another spawn + // landing on the same profile could otherwise move the state) never reopens. An + // explicitly-named profile still goes through launcher.spawn(SpawnRequest) and its throwing + // branch, unchanged — that caller asked for one profile by name and still gets everything + // enforceNotQuarantined/enforceNotCoolingOff/enforceMaxLoad/enforceModelEnabled decide about + // it, refusal included. // // The one cost that remains, unchanged from round 1: an unqualified worktree-provisioned // spawn does not get CompositePeerLauncher's cross-candidate retry on a live