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 e40e3b7..3a33f43 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -14,6 +14,7 @@ import dev.ltms.fleet.placement.BackendOutagePolicy; import dev.ltms.fleet.placement.BackendQuarantine; import dev.ltms.fleet.placement.PlacementCandidate; import dev.ltms.fleet.placement.PlacementContext; +import dev.ltms.fleet.placement.PlacementDecision; import dev.ltms.fleet.placement.PlacementException; import dev.ltms.fleet.placement.PlacementPolicies; import dev.ltms.fleet.placement.PlacementPolicy; @@ -402,21 +403,10 @@ public final class CompositePeerLauncher implements PeerLauncher { // the whole profile list. An EXPLICIT profile (above) is left alone on purpose — it is the // operator overriding, and refusing it would break `fleet_spawn{profile:"opus"}`, which // carries no role and so would be judged against the dev pool it was never meant for. - List candidates = candidates(req.role()); - String roleDefault = defaultProfileFor(req.role()); Set unreachable = new HashSet<>(); - // CB-578 stage B: computed once up front — a quarantine's expiry cannot pass within one spawn - // call, so re-deriving it per retry would only cost work, never change the answer. - Set quarantined = quarantinedProfiles(candidates); - // fleetd #201 Unit 5: a distinct set from quarantined — see PlacementContext.coolingOff. - Set coolingOff = coolingOffProfiles(candidates); - // fleetd #422: read live per spawn, same as quarantined/coolingOff above — a config reload - // that flips a model's enabled state is visible to the very next unqualified spawn. - Set modelOff = modelOffProfiles(candidates); - PlacementContext ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable, - quarantined, coolingOff, modelOff); + PlacementContext ctx = placementContextFor(req.role(), unreachable); - int maxAttempts = candidates.isEmpty() ? 1 : candidates.size(); + int maxAttempts = ctx.candidates().isEmpty() ? 1 : ctx.candidates().size(); for (int attempt = 0; attempt < maxAttempts; attempt++) { // Deliberately uncaught: when no candidate is left (all at cap, or all unreachable) the // policy already throws a clear message. Catching it to rethrow a generic @@ -443,8 +433,7 @@ public final class CompositePeerLauncher implements PeerLauncher { chosen.profile(), e.getMessage()); unreachable.add(chosen.profile()); // Update the context for the next selection so the policy excludes this profile. - ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable, - quarantined, coolingOff, modelOff); + ctx = placementContextFor(req.role(), unreachable); } } @@ -615,8 +604,23 @@ public final class CompositePeerLauncher implements PeerLauncher { return known.isEmpty() ? List.copyOf(configured.keySet()) : known; } - /** The profile an unqualified spawn for {@code role} falls back to under {@code fixed} placement. */ - private String defaultProfileFor(MemberRole role) { + /** + * {@inheritDoc} + * + *

Live: reads {@link #poolFor}, which reads {@link #profileConfigs} and {@link #fleet} fresh + * on every call, so a config reload is visible without a restart (fleetd #425) — unlike {@link + * #defaultProfile}, the field captured once at construction, which this falls back to only when + * {@link #poolFor} has nothing to offer at all (no profiles configured for this composite). + * + *

Exact only under the {@code fixed} placement policy — the one that reads this value + * ({@code FixedPlacementPolicy}, package-private, hence not linked) as its first, preferred + * candidate. {@code weighted}/{@code round-robin} placement can choose a different candidate + * from {@code role}'s pool even on the very first spawn; this method does not simulate that + * choice, matching what the {@code defaultProfile:}-derived reporting this replaces has always + * done. + */ + @Override + public String defaultProfileFor(MemberRole role) { List pool = poolFor(role); return pool.isEmpty() ? defaultProfile : pool.getFirst(); } @@ -633,6 +637,142 @@ public final class CompositePeerLauncher implements PeerLauncher { return out; } + /** + * Build the {@link PlacementContext} an unqualified spawn of {@code role} would be judged + * against right now — the single source both {@link #spawn} and {@link #place} read, so the two + * can never disagree about which conditions (quarantine, cool-off, model-off) apply to which + * candidate (fleetd #425 rework: round 1 duplicated this into a second, blind resolver — + * {@link #defaultProfileFor} — which is why it regressed; round 2 found that even a single + * shared resolver is not enough on its own if the CALLER re-resolves through an explicit + * profile afterwards — see {@link PlacementDecision}). + * + * @param unreachable the caller's mutable unreachable set; {@link #spawn} grows this across + * retries and rebuilds the context from it, {@link #place} passes a fresh + * empty one since it never retries + */ + private PlacementContext placementContextFor(MemberRole role, Set unreachable) { + List candidates = candidates(role); + String roleDefault = defaultProfileFor(role); + // CB-578 stage B: computed once up front — a quarantine's expiry cannot pass within one spawn + // call, so re-deriving it per retry would only cost work, never change the answer. + Set quarantined = quarantinedProfiles(candidates); + // fleetd #201 Unit 5: a distinct set from quarantined — see PlacementContext.coolingOff. + Set coolingOff = coolingOffProfiles(candidates); + // fleetd #422: read live per spawn, same as quarantined/coolingOff above — a config reload + // that flips a model's enabled state is visible to the very next unqualified spawn. + Set modelOff = modelOffProfiles(candidates); + return new PlacementContext(roleDefault, candidates, liveCount, unreachable, + quarantined, coolingOff, modelOff); + } + + /** + * {@inheritDoc} + * + *

fleetd #425 rework, round 2: runs the exact same selection {@link #spawn} uses for a + * blank-profile request — {@link #placementContextFor} plus one {@link PlacementPolicy#select} + * — rather than {@link #defaultProfileFor}'s blind "pool's first entry", so a quarantined, + * cooling-off, or model-off pool-first candidate is routed around here exactly as it would be + * by a real spawn. Unlike {@link #spawn}, this never retries on {@link + * PeerUnreachableException}: there is no spawn attempt to fail, so "unreachable" never grows + * past the empty set it starts with, and a single {@link PlacementPolicy#select} call already + * reflects the live quarantine/cool-off/model-off state. + * + *

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". 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) + */ + @Override + public PlacementDecision place(MemberRole role) { + PlacementContext ctx = placementContextFor(role, new HashSet<>()); + return new PlacementDecision(placementPolicy.get().select(ctx).profile()); + } + + /** + * {@inheritDoc} + * + *

Delegates to {@link #place}, so the two can never disagree about the answer for the same + * {@code role} at the same instant — kept as a convenience for a caller that only wants the + * resolved name (a status report, a log line), never for a caller that will act on it by + * spawning: that caller must hold the {@link PlacementDecision} itself and pass it to {@link + * #spawn(SpawnRequest, PlacementDecision)} — see {@link PlacementDecision}'s javadoc for why + * resolving here and spawning separately, with the name fed back in as an explicit profile, + * regressed fleetd #425 twice. + */ + @Override + public String routedProfileFor(MemberRole role) { + return place(role).profile(); + } + + /** + * {@inheritDoc} + * + *

Routes {@code decision.profile()} directly to its owning delegate — the identical + * {@code d.spawn(routedReq)} call {@link #spawn(SpawnRequest)}'s blank-profile branch makes for + * its first pick — WITHOUT re-running {@link #enforceNotQuarantined}, {@link + * #enforceNotCoolingOff}, {@link #enforceMaxLoad}, or {@link #enforceModelEnabled}: those are + * 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). + * + *

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 + * re-place the caller onto a different profile than the one {@code decision} named, behind the + * back of a caller that may already have provisioned something (a worktree's {@code repoRoot}, + * parity overlay) specifically for that name. A caller that wants the composite's own failover + * should call {@link #spawn(SpawnRequest)} with a blank profile directly, not resolve through + * {@link #place} first. Losing that retry on a resolve-then-spawn path is an accepted, unrelated + * cost — see {@code SessionManager.acquireWithWorktree}'s own comment on it — never widened by + * this round to include {@code maxLoad}, which is what round 1 actually lost. + */ + @Override + public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { + HerdrPeerLauncher d = route(decision.profile()); + SpawnRequest routedReq = req.withProfile(decision.profile()); + PeerHandle handle = d.spawn(routedReq); + spawnedBy.put(handle.id(), d); + return handle; + } + @Override public String effectiveCwd(SpawnRequest req) { return route(req.profileName()).effectiveCwd(req); @@ -780,9 +920,21 @@ public final class CompositePeerLauncher implements PeerLauncher { return byProfile.keySet(); } + /** + * {@inheritDoc} + * + *

fleetd #425: reports the live {@code dev} pool's first entry — the same value + * {@link #defaultProfileFor} computes for {@link MemberRole#DEV} — not the {@link + * #defaultProfile} field captured at construction. An unqualified {@code fleet_spawn} defaults + * to {@code MemberRole#DEV} (see {@link dev.ltms.fleet.peer.SpawnRequest}), so "the dev pool's + * live first entry" is exactly the profile such a spawn actually lands on right now — the + * question {@code fleet_profiles}' {@code "default"} field exists to answer. The frozen field is + * a role-agnostic fallback used only when {@link #poolFor} has nothing to report at all (no + * profiles configured), which {@link #defaultProfileFor} already handles. + */ @Override public String defaultProfile() { - return defaultProfile; + return defaultProfileFor(MemberRole.DEV); } /** 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 fa6a9cf..c19b419 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java @@ -1,5 +1,7 @@ package dev.ltms.fleet.peer; +import dev.ltms.fleet.placement.PlacementDecision; + import java.nio.file.Path; import java.util.List; import java.util.Set; @@ -143,9 +145,124 @@ public interface PeerLauncher { /** * The profile a no-argument {@link #spawn(SpawnRequest)} uses, or {@code null} if none is configured. + * + *

fleetd #425: for an implementation with role pools (a no-argument spawn is read as {@link + * MemberRole#DEV}, see {@link SpawnRequest}), this must be the profile a live spawn of that role + * would actually be placed on right now, not a value captured once at startup — a caller such as + * {@code fleet_profiles} relies on this to report a live, not frozen, fact. */ String defaultProfile(); + /** + * The profile an unqualified spawn of {@code role} would resolve to right now — the role-aware, + * live counterpart of {@link #defaultProfile()} (fleetd #425). + * + *

A caller that must provision something profile-specific (working directory, parity overlay + * files) before the actual spawn — {@code SessionManager.acquireWithWorktree} is the one + * that exists today — needs the exact profile that spawn will use, for the caller's real role, + * not a role-agnostic guess. Calling {@link #defaultProfile()} for that purpose reads {@code + * MemberRole#DEV}'s answer regardless of the caller's actual role, which is wrong for any other + * role and can provision for a profile the spawn never lands on. + * + *

Default implementation returns {@link #defaultProfile()}, ignoring {@code role} — the right + * answer for a launcher with no role-pool concept of its own (e.g. a single {@code + * HerdrPeerLauncher} adapter, which is never reached this way in production: {@code + * CompositePeerLauncher} always fronts it and resolves roles itself). + */ + default String defaultProfileFor(MemberRole role) { + return defaultProfile(); + } + + /** + * The profile an unqualified spawn of {@code role} would actually be routed to right + * now — the same candidate list, the same {@code quarantined}/{@code coolingOff}/{@code + * modelOff} filtering, and the same {@code PlacementPolicy} that {@link #spawn} itself + * consults for a blank-profile request (fleetd #425 rework). + * + *

This is not {@link #defaultProfileFor}: that method answers "what is first in + * {@code role}'s pool", blind to quarantine, cool-off, and the model on/off gate — the right + * answer for a role-agnostic, best-effort report ({@code fleet_profiles}' {@code "default"} + * field), but the wrong one for a caller that needs the profile a spawn will actually land on. + * A quarantined or model-off pool-first profile makes {@link #defaultProfileFor} return a name + * an unqualified spawn will never be routed to. + * + *

Just the resolved name, not the full {@link PlacementDecision} — a caller that only wants + * to know the answer (a status report, a log line) can call this; a caller that will later + * act on the answer by spawning — provisioning a worktree for a specific profile + * 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, 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 + */ + default String routedProfileFor(MemberRole role) { + return place(role).profile(); + } + + /** + * Resolve, without spawning, the {@link PlacementDecision} an unqualified spawn of + * {@code role} would make right now — the same candidate list, the same {@code + * quarantined}/{@code coolingOff}/{@code modelOff} filtering, and the same {@code + * PlacementPolicy} {@link #spawn(SpawnRequest)}'s blank-profile branch itself consults (fleetd + * #425 rework). + * + *

Pair this with {@link #spawn(SpawnRequest, PlacementDecision)}, never with {@link + * #spawn(SpawnRequest)} fed the decision's profile as an explicit name — see {@link + * PlacementDecision}'s own javadoc for why that second form regressed. + * + *

Default implementation wraps {@link #defaultProfile()}, ignoring {@code role} and every + * placement condition — the right answer for a launcher with no pool or placement-policy + * concept of its own, matching {@link #defaultProfileFor}'s own default. + * + * @throws RuntimeException (implementation-specific, typically a placement exception) if no + * candidate in {@code role}'s pool is currently placeable + */ + default PlacementDecision place(MemberRole role) { + return new PlacementDecision(defaultProfile()); + } + + /** + * Spawn against an already-resolved {@link PlacementDecision} from {@link #place}, honoring it + * 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 + * matching profile; passing a request that names a different, explicit profile than + * the decision it is paired with is a caller bug this method does not attempt to detect. + * + *

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. + * + * @throws IllegalArgumentException if the decision names an unknown profile + */ + default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { + return spawn(req.withProfile(decision.profile())); + } + /** * Resolve the effective working directory for a spawn {@code req} without actually spawning. * Resolution order: requestedCwd → profile cwd → callerCwd → daemon cwd. diff --git a/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java b/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java new file mode 100644 index 0000000..caa7763 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java @@ -0,0 +1,49 @@ +package dev.ltms.fleet.placement; + +import dev.ltms.fleet.peer.MemberRole; +import dev.ltms.fleet.peer.PeerLauncher; +import dev.ltms.fleet.peer.SpawnRequest; + +/** + * An already-completed placement choice — the outcome of one {@link PeerLauncher#place} call, + * carried forward so a later {@link PeerLauncher#spawn(SpawnRequest, PlacementDecision)} can honor + * it directly instead of re-resolving the profile a second time (fleetd #425 rework, round 2). + * + *

The problem this exists to close: a caller that must know the profile before it can + * spawn — {@code SessionManager.acquireWithWorktree} provisions a worktree's {@code repoRoot} and + * parity overlay for a specific profile before the peer process exists — used to resolve that name + * with {@code PeerLauncher.routedProfileFor(role)} and then hand the SAME string back to {@link + * PeerLauncher#spawn(SpawnRequest)} as an EXPLICIT profile. That re-resolution is not free: naming + * 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 — 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. 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()} + * already tolerates) + */ +public record PlacementDecision(String profile) { +} 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 c1f3977..9831d32 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -10,6 +10,7 @@ import dev.ltms.fleet.peer.MemberRole; import dev.ltms.fleet.peer.PeerHandle; import dev.ltms.fleet.peer.PeerLauncher; import dev.ltms.fleet.peer.SpawnRequest; +import dev.ltms.fleet.placement.PlacementDecision; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -584,8 +585,65 @@ public final class SessionManager implements TurnListener { String ownerTerminal, WorktreeRequest wt, String sessionName, String resumeSessionId, MemberLifecycle.SlotReservation reservation) { - String preResolvedProfile = (profile == null || profile.isBlank()) - ? launcher.defaultProfile() : profile; + // fleetd #425 rework (round 2): resolved through launcher.place(memberRole) — the same + // candidate list, quarantine/cool-off/model-off filtering, and PlacementPolicy an unqualified + // spawn of this role is actually judged against right now — never launcher.defaultProfile() + // (MemberRole.DEV only, wrong for any other role) and never launcher.defaultProfileFor() + // (the role's pool FIRST entry, blind to quarantine/cool-off/model-off: a first-round fix + // used exactly this and regressed fleetd #429's "the fleet keeps working when a model is + // turned off" guarantee — a quarantined or model-off pool-first profile made this throw + // instead of routing around it, which an unqualified spawn is supposed to do). This same + // resolved decision is reused below for repoRoot, parityOverlay, AND the spawn itself so the + // worktree is always provisioned for the profile the member actually runs on — the two could + // disagree before fleetd #425: this name picked repoRoot/overlay, but the spawn below passed + // the ORIGINAL (blank) profile through to placement, which re-resolves live and can pick a + // different profile if the pool changed between the two reads, or a genuinely different one + // 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, 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 + // PeerUnreachableException raised by the backend itself at spawn time (a transport-level + // failure placement cannot see in advance) — spawn(req, decision) commits to the one profile + // place() already chose, the same way an explicit-profile spawn commits to its one name. That + // trade is deliberate: a worktree provisioned for the wrong backend (the #425 hazard) is worse + // than a spawn that fails cleanly and can be retried by the caller. Nothing else is lost: + // maxLoad, quarantine, cool-off and model-off all behave identically whether or not a + // worktree was requested — that agreement is the invariant this rework exists to hold. + boolean unqualifiedProfile = profile == null || profile.isBlank(); + PlacementDecision decision = unqualifiedProfile ? launcher.place(memberRole) : new PlacementDecision(profile); + String preResolvedProfile = decision.profile(); // CB-507: resolve through the launcher's CB-112 chain (requested → profile cwd → caller → // daemon cwd → "."), never the raw args. A plain REST spawn supplies neither a requested // nor a caller cwd, so taking the first non-blank of those two yielded null and put @@ -608,7 +666,15 @@ public final class SessionManager implements TurnListener { // copies more files into the worktree after add() returns, so sharing the group any earlier // leaves those overlay files operator-owned and read-only for a different-uid member. worktrees.shareWithGroup(repoRoot, path); - handle = launcher.spawn(new SpawnRequest(profile, path, callerCwd, sessionName, resumeSessionId, memberRole)); + // fleetd #425: preResolvedProfile, not the original (possibly blank) profile — see the + // comment above where it is resolved. The overlay/repoRoot above and the spawn here must + // name the same profile. An unqualified request stays unqualified here and is honored via + // the PlacementDecision already captured above (spawn(req, decision) — the routing branch, + // no enforce* re-check); an explicitly-named profile still goes through the single-arg + // spawn(req) and its throwing branch, exactly as before this rework. + SpawnRequest spawnReq = new SpawnRequest(unqualifiedProfile ? null : preResolvedProfile, + path, callerCwd, sessionName, resumeSessionId, memberRole); + handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq); } catch (RuntimeException e) { log.warn("spawn failed for profile={} role={} branch={} path={}: {}", preResolvedProfile, memberRole, branch, path, e.getMessage()); @@ -636,7 +702,7 @@ public final class SessionManager implements TurnListener { } throw e; } - String resolvedProfile = resolveProfile(handle, profile); + String resolvedProfile = resolveProfile(handle, preResolvedProfile); String cwd = launcher.effectiveCwd(new SpawnRequest(resolvedProfile, path, callerCwd)); long now = nowNanos.getAsLong(); // CB-619: see the no-worktree path above — bind before recording, and store the returned diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesLiveDefaultTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesLiveDefaultTest.java new file mode 100644 index 0000000..08e571e --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesLiveDefaultTest.java @@ -0,0 +1,114 @@ +package dev.ltms.fleet.mcp; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.guard.SubscriptionGuard; +import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.WorkspaceControl; +import dev.ltms.fleet.member.ClaudeCodeLauncher; +import dev.ltms.fleet.member.CompositePeerLauncher; +import dev.ltms.fleet.peer.MemberRole; +import dev.ltms.fleet.peer.PeerLauncher; +import dev.ltms.fleet.peer.SpawnRequest; +import dev.ltms.fleet.placement.BackendQuarantine; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #425: {@code fleet_profiles}' {@code "default"} field was captured once at boot + * ({@code cfg.effectiveDefaultProfile()}, frozen into {@code CompositePeerLauncher.defaultProfile} + * at construction) while an unqualified spawn resolves the same underlying key + * ({@code fleet.developers}' first entry) live, on every call. Reordering {@code fleet.developers} + * and reloading changed where a spawn landed without ever changing what {@code fleet_profiles} + * reported — a lead following {@code CLAUDE.md}'s "check {@code fleet_profiles} once per session" + * instruction was told a stale answer. + * + *

This test drives the exact caller {@code fleet_profiles} uses — + * {@link FleetMcp#profilesView(PeerLauncher, FleetMcp.QuarantineSource, FleetMcp.OutageSource)} — + * against a real, reloadable {@link ConfigRef}, so it fails if the reporting path is ever recoupled + * to a frozen value instead of {@link CompositePeerLauncher#defaultProfile()}'s live answer. + */ +class FleetProfilesLiveDefaultTest { + + /** A minimal fleetd.yaml whose dev pool is {@code profilesInOrder}, in that definition order. */ + private static String yamlWithDevPool(String... profilesInOrder) { + StringBuilder devPool = new StringBuilder(); + for (int i = 0; i < profilesInOrder.length; i++) { + devPool.append(" slot").append(i).append(":\n profile: ") + .append(profilesInOrder[i]).append('\n'); + } + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + opus: + baseUrl: http://gx00.gw:8000 + model: opus-coder + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet-coder + guard: + offSubscriptionHosts: + - gx00.gw + fleet: + developers: + """ + devPool; + } + + @Test + void fleetProfilesDefaultTracksALiveDevPoolReorderAfterReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yamlWithDevPool("opus", "sonnet")); + ConfigRef ref = new ConfigRef(f, FleetConfig.load(f)); + + Map profiles = Map.of( + "opus", new FleetConfig.Profile("opus", "http://gx00.gw:8000", "opus-coder", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, null, null), + "sonnet", new FleetConfig.Profile("sonnet", "http://gx00.gw:8000", "sonnet-coder", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, null, null)); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "opus", _ -> null); + PeerLauncher workers = new CompositePeerLauncher( + List.of(adapter), "opus", ref, _ -> 0, BackendQuarantine.none()); + + assertReportedDefaultMatchesAnUnqualifiedSpawn(workers, "opus"); + + Files.writeString(f, yamlWithDevPool("sonnet", "opus")); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), () -> "reload should apply cleanly: " + out.error()); + + assertReportedDefaultMatchesAnUnqualifiedSpawn(workers, "sonnet"); + } + + /** + * Asserts BOTH that {@code fleet_profiles}' {@code "default"} equals {@code expected}, AND that + * it equals what a real unqualified {@code MemberRole#DEV} spawn actually gets placed on right + * now — the two facts fleetd #425 found disagreeing. + */ + private static void assertReportedDefaultMatchesAnUnqualifiedSpawn(PeerLauncher workers, String expected) { + Map view = FleetMcp.profilesView( + workers, FleetMcp.QuarantineSource.none(), FleetMcp.OutageSource.none()); + assertEquals(expected, view.get("default"), + "fleet_profiles' \"default\" must be the live dev-pool answer, not a boot-time snapshot"); + + String placed = workers.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile(); + assertEquals(expected, placed, + "sanity: the profile an unqualified dev spawn actually lands on"); + } +} 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 26fdd29..9de90d1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -944,6 +944,89 @@ class CompositePeerLauncherTest { assertTrue(e.getMessage().contains("maxLoad"), e.getMessage()); } + // ── fleetd #425: defaultProfile()/defaultProfileFor() must track a live reload ───────────── + + /** A minimal fleetd.yaml whose dev pool is {@code profilesInOrder}, in that definition order. */ + private static String yamlWithDevPool(String... profilesInOrder) { + StringBuilder devPool = new StringBuilder(); + for (int i = 0; i < profilesInOrder.length; i++) { + devPool.append(" slot").append(i).append(":\n profile: ") + .append(profilesInOrder[i]).append('\n'); + } + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + opus: + baseUrl: http://gx00.gw:8000 + model: opus-coder + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet-coder + guard: + offSubscriptionHosts: + - gx00.gw + fleet: + developers: + """ + devPool; + } + + /** + * Criterion 1 (fleetd #425): reorder {@code fleet.developers}, reload, and assert the reported + * default ({@link CompositePeerLauncher#defaultProfile()} — what {@code fleet_profiles}' {@code + * "default"} is built from, see {@code FleetMcp.profilesView}) matches what an unqualified + * {@code MemberRole#DEV} spawn is actually placed on, both before and after the reorder. Asserts + * {@code applied()} so the test proves the reload actually took, not that nothing changed. + */ + @Test + void defaultProfileTracksALiveDevPoolReorderAfterReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yamlWithDevPool("opus", "sonnet")); + ConfigRef ref = new ConfigRef(f, FleetConfig.load(f)); + + FakeHerdr herdr = new FakeHerdr(); + StubLauncher adapter = new StubLauncher("claude", herdr, threeProfiles(), "opus", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "opus", ref, _ -> 0, BackendQuarantine.none()); + + assertEquals("opus", composite.defaultProfile(), + "reported default starts at the dev pool's first entry"); + assertEquals("opus", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile(), + "an unqualified dev spawn must land on the same profile that was just reported"); + + Files.writeString(f, yamlWithDevPool("sonnet", "opus")); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), () -> "reload should apply cleanly: " + out.error()); + + assertEquals("sonnet", composite.defaultProfile(), + "the reported default must follow the reorder with no daemon restart"); + assertEquals("sonnet", composite.spawn( + new SpawnRequest(null, null, null, null, null, MemberRole.DEV)).profile(), + "and it must still be exactly what an unqualified spawn actually gets"); + } + + /** + * Criterion 2 — the mirror, and the load-bearing half (fleetd #425): with NOTHING configured (no + * profiles at all, hence an empty pool for every role), the frozen {@code defaultProfile} field + * is still what gets reported. A fix that always returns {@code poolFor(role).getFirst()} with no + * empty-pool fallback throws or returns the wrong thing here even though criterion 1 above still + * passes — this is the test that catches it. + */ + @Test + void defaultProfileFallsBackToTheFrozenFieldWhenNothingIsConfiguredAtAll() { + FakeHerdr herdr = new FakeHerdr(); + StubLauncher adapter = new StubLauncher("claude", herdr, Map.of(), "opus", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "opus", Map.of(), PlacementPolicies.fixed(), _ -> 0); + + assertEquals("opus", composite.defaultProfile(), + "with no profiles configured at all, the frozen field is the only answer available"); + assertEquals("opus", composite.defaultProfileFor(MemberRole.DEV)); + } + // ── CB-578 stage B: a BACKEND_EXHAUSTED classification quarantines the credential ────────── @Test @@ -1010,6 +1093,36 @@ class CompositePeerLauncherTest { assertEquals(0, adapter.spawnCount("sol")); } + /** + * fleetd #425 rework, acceptance 1: {@link CompositePeerLauncher#routedProfileFor} must apply + * the SAME quarantine filtering {@link CompositePeerLauncher#spawn} does, under {@code fixed()} + * — the DEFAULT placement policy, deliberately not {@code weighted()} (which the regressed + * round's own tests all used, and which never exercises {@code FixedPlacementPolicy}'s own + * inline filter). This is the exact defect: the previous round's {@code defaultProfileFor} + * blindly returns the pool's first entry ("sol", quarantined here) with no awareness of + * quarantine at all, which is what turned a routine unqualified spawn into a hard throw once + * {@code acquireWithWorktree} pre-resolved through it. + */ + @Test + void routedProfileForSkipsAQuarantinedPoolFirstProfileUnderFixedPolicy() { + 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)); + quarantine.quarantine("shared-openai"); + CompositePeerLauncher composite = new CompositePeerLauncher(List.of(adapter), "sol", profiles, + PlacementPolicies.fixed(), _ -> 0, null, quarantine); + + assertEquals("b", composite.routedProfileFor(MemberRole.DEV), + "sol (the pool's first entry) is quarantined, so the routed answer must be b"); + assertEquals("sol", composite.defaultProfileFor(MemberRole.DEV), + "sanity: defaultProfileFor stays blind to quarantine — that's the gap routedProfileFor closes"); + assertEquals(0, adapter.spawnCount("sol"), "routedProfileFor never spawns anything"); + assertEquals(0, adapter.spawnCount("b"), "routedProfileFor never spawns anything"); + } + @Test void aQuarantineLiftsOnTheInjectedClockAndTheProfileBecomesSpawnableAgain() { FakeHerdr herdr = new FakeHerdr(); @@ -1403,6 +1516,37 @@ class CompositePeerLauncherTest { assertEquals(0, adapter.spawnCount("local")); } + /** + * fleetd #425 rework, acceptance 2: same shape as {@link #fixedPlacementSkipsAnOffModelProfileToo} + * above, but through {@link CompositePeerLauncher#routedProfileFor} rather than an actual + * {@link CompositePeerLauncher#spawn} — the exact call {@code SessionManager.acquireWithWorktree} + * makes to pre-resolve a profile for provisioning. This is the fleetd #429 case named in the + * ticket: an operator turns a model off, and an unqualified worktree spawn must still route + * around it instead of throwing "names model, which the operator has turned off" — the throw + * {@link CompositePeerLauncher#enforceModelEnabled} raises only on the EXPLICIT-profile branch, + * which is exactly the branch the regressed round accidentally routed every worktree spawn onto. + */ + @Test + void routedProfileForSkipsAModelOffPoolFirstProfileUnderFixedPolicy() { + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "local", stubWorkerModel("local", "deepseek-v4-flash"), + "sonnet", stubWorkerModel("sonnet", "claude-sonnet-5")); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "local", Set.of()); + FleetConfig.Models models = new FleetConfig.Models(List.of( + new FleetConfig.Models.ModelEntry("deepseek-v4-flash", false))); + CompositePeerLauncher composite = new CompositePeerLauncher(List.of(adapter), "local", profiles, + PlacementPolicies.fixed(), _ -> 0, null, BackendQuarantine.none(), NO_OUTAGE, + () -> models); + + assertEquals("sonnet", composite.routedProfileFor(MemberRole.DEV), + "local (the pool's first entry) names an off model, so the routed answer must be sonnet"); + assertEquals("local", composite.defaultProfileFor(MemberRole.DEV), + "sanity: defaultProfileFor stays blind to model-off — that's the gap routedProfileFor closes"); + assertEquals(0, adapter.spawnCount("local"), "routedProfileFor never spawns anything"); + assertEquals(0, adapter.spawnCount("sonnet"), "routedProfileFor never spawns anything"); + } + /** * Criterion 4: turning a model off/on is HOT — no restart — proven through a REAL * {@code ConfigRef.reload()}, not a hand-rolled supplier swap. Also proves {@code models} is diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java index 77e1ff2..2daf2fc 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -6,12 +6,14 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.read.ListAppender; import dev.ltms.fleet.auth.MemberRegistry; import dev.ltms.fleet.auth.MemberLifecycle; +import dev.ltms.fleet.config.ConfigRef; import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.guard.SubscriptionGuard; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.FakeHerdr; import dev.ltms.fleet.herdr.WorkspaceControl; import dev.ltms.fleet.member.ClaudeCodeLauncher; +import dev.ltms.fleet.member.CompositePeerLauncher; import dev.ltms.fleet.msg.TestTurnTokens; import dev.ltms.fleet.peer.Capability; import dev.ltms.fleet.peer.CharterReceipt; @@ -20,9 +22,15 @@ import dev.ltms.fleet.peer.PeerHandle; import dev.ltms.fleet.peer.PeerLauncher; import dev.ltms.fleet.peer.PeerUnreachableException; import dev.ltms.fleet.peer.SpawnRequest; +import dev.ltms.fleet.placement.BackendQuarantine; +import dev.ltms.fleet.placement.PlacementPolicies; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.slf4j.LoggerFactory; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -33,6 +41,7 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.LongSupplier; +import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.*; @@ -1991,4 +2000,296 @@ class SessionManagerTest { sessions.rosterResolved(); assertEquals(2, handle.callCount(), "once resolved, the id must not be looked up again"); } + + // ── fleetd #425 criterion 3: acquireWithWorktree must provision for the profile it actually + // spawns, never a name resolved before a live pool change is accounted for ──────────────────── + + /** Two profiles with distinct {@code cwd}/{@code parityOverlay}, and a dev pool of {@code first,second}. */ + private static String worktreeReorderYaml(String first, String second) { + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + a: + baseUrl: http://gx00.gw:8000 + model: coder-a + b: + baseUrl: http://gx00.gw:8000 + model: coder-b + guard: + offSubscriptionHosts: + - gx00.gw + fleet: + developers: + slot0: + profile: %s + slot1: + profile: %s + """.formatted(first, second); + } + + @Test + void acquireWithWorktreeProvisionsTheOverlayForTheProfileActuallySpawned( + @TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, worktreeReorderYaml("a", "b")); + ConfigRef ref = new ConfigRef(f, FleetConfig.load(f)); + + Map profiles = Map.of( + "a", new FleetConfig.Profile("a", "http://gx00.gw:8000", "coder-a", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/a", List.of("a.mcp.json")), + "b", new FleetConfig.Profile("b", "http://gx00.gw:8000", "coder-b", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/b", List.of("b.mcp.json"))); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "a", _ -> null); + PeerLauncher launcher = new CompositePeerLauncher( + List.of(adapter), "a", ref, _ -> 0, BackendQuarantine.none()); + + // The pool changes AFTER the composite/launcher is built, and BEFORE the unqualified + // worktree spawn — exactly the fleetd #425 scenario: the live pool's first entry is "b" by + // the time acquireWithWorktree runs, even though nothing here was rebuilt. + Files.writeString(f, worktreeReorderYaml("b", "a")); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), () -> "reload should apply cleanly: " + out.error()); + + FakeWorktrees worktrees = new FakeWorktrees(); + SessionManager sessions = new SessionManager(launcher, worktrees, () -> 0L); + + MemberSession s = sessions.acquire(null, null, "/caller", + null, new WorktreeRequest("fleetd-425", null)); + + assertEquals("b", s.profile(), + "the live dev pool now starts at b, so the unqualified spawn must land there"); + FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); + assertNotNull(overlay, "overlayParity must have been called"); + assertEquals(List.of("b.mcp.json"), overlay.requested(), + "the worktree must be provisioned with profile b's overlay — the one actually " + + "spawned — never a's, the pool's stale first entry"); + } + + /** + * The deterministic, mutation-pinning half of criterion 3: {@code launcher.defaultProfile()} + * only ever answers for {@link MemberRole#DEV} (see {@link CompositePeerLauncher#defaultProfile()}), + * so resolving a worktree spawn's profile through it — instead of through {@link + * PeerLauncher#defaultProfileFor(MemberRole)}, resolved against the CALLER's actual role — picks + * the wrong pool's answer for any role other than DEV. No reload or race is needed to see it: an + * ARCHITECT pool and a DEV pool that simply disagree, held constant, are enough. + */ + @Test + void acquireWithWorktreeForANonDevRoleUsesThatRolesPoolNotTheDevPool() { + Map profiles = Map.of( + "a", new FleetConfig.Profile("a", "http://gx00.gw:8000", "coder-a", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/a", List.of("a.mcp.json")), + "b", new FleetConfig.Profile("b", "http://gx00.gw:8000", "coder-b", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/b", List.of("b.mcp.json"))); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "a", _ -> null); + // developers -> a (first/only entry); architects -> b (first/only entry). The two pools + // disagree on purpose, so a role-blind resolution (DEV's answer, "a") is visibly wrong for + // an ARCHITECT spawn, which must land on "b". + FleetConfig.Fleet fleet = new FleetConfig.Fleet(Map.of(), + Map.of("s0", new FleetConfig.Slot("b")), + Map.of("s0", new FleetConfig.Slot("a")), + Map.of(), null); + PeerLauncher launcher = new CompositePeerLauncher(List.of(adapter), "a", profiles, + PlacementPolicies.fixed(), _ -> 0, fleet); + + FakeWorktrees worktrees = new FakeWorktrees(); + SessionManager sessions = new SessionManager(launcher, worktrees, () -> 0L); + + MemberSession s = sessions.acquire(null, MemberRole.ARCHITECT, null, "/caller", + null, new WorktreeRequest("fleetd-425b", null)); + + assertEquals("b", s.profile(), + "an unqualified ARCHITECT worktree spawn must land on the architect pool's profile"); + FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); + assertNotNull(overlay, "overlayParity must have been called"); + assertEquals(List.of("b.mcp.json"), overlay.requested(), + "the worktree must be provisioned with profile b's overlay — the ARCHITECT pool's " + + "answer, the one actually spawned — never a's, the DEV pool's answer that " + + "launcher.defaultProfile() alone would have given"); + } + + /** + * fleetd #425 rework, acceptance 3: repoRoot, parityOverlay, AND the actual spawn must all name + * the SAME routed profile, proven on the ROUTED path — a quarantine skips the pool's first entry + * — not just the "pool reordered by a live reload" path the two tests above already cover. + * + *

This is the exact regression the rework fixes: the first round resolved + * {@code acquireWithWorktree}'s profile through {@code launcher.defaultProfileFor(memberRole)}, + * which is blind to quarantine and just returns the pool's first entry ("a" here, quarantined). + * That name went on to provision repoRoot/overlay for "a", and then the spawn itself — now an + * EXPLICIT-profile spawn naming "a" — hit {@code CompositePeerLauncher.enforceNotQuarantined} + * and threw, where the pre-fix code (a blank-profile spawn) would have routed around "a" onto + * "b" without any trouble. {@code launcher.routedProfileFor(memberRole)} closes that gap by + * running the SAME quarantine-aware selection {@code spawn} itself uses, so all three — repoRoot, + * overlay, and the spawn — land on "b" together. + */ + @Test + void acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile() { + Map profiles = new LinkedHashMap<>(); + profiles.put("a", new FleetConfig.Profile("a", "http://gx00.gw:8000", "coder-a", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/a", List.of("a.mcp.json"), + null, null, null, null, null, null, null, null, "shared-cred", null)); + profiles.put("b", new FleetConfig.Profile("b", "http://gx00.gw:8000", "coder-b", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/b", List.of("b.mcp.json"))); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "a", _ -> null); + BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.MINUTES.toNanos(30)); + quarantine.quarantine("shared-cred"); + PeerLauncher launcher = new CompositePeerLauncher(List.of(adapter), "a", profiles, + PlacementPolicies.fixed(), _ -> 0, null, quarantine); + + FakeWorktrees worktrees = new FakeWorktrees(); + SessionManager sessions = new SessionManager(launcher, worktrees, () -> 0L); + + MemberSession s = sessions.acquire(null, null, "/caller", + null, new WorktreeRequest("fleetd-425c", null)); + + assertEquals("b", s.profile(), + "a is quarantined, so the unqualified worktree spawn must route to b"); + FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); + assertNotNull(overlay, "overlayParity must have been called"); + assertEquals(List.of("b.mcp.json"), overlay.requested(), + "parityOverlay must be provisioned for b — the profile actually spawned, never a's, " + + "the quarantined pool-first entry"); + FakeWorktrees.RepoRootCall repoRootCall = worktrees.repoRootCalls().getLast(); + assertTrue(repoRootCall.cwd().contains("/repo/b"), + "repoRoot must be resolved through b's effectiveCwd, not a's: " + repoRootCall.cwd()); + } + + /** + * fleetd #425 rework, round 2: this is the exact probe that found round 1's maxLoad + * regression. One dev profile ("a") is configured with {@code maxLoad: 1} and a liveCount + * pinned at 1 — permanently at cap — under {@code PlacementPolicies.fixed()}, the default + * policy, which deliberately never evaluates {@code maxLoad} during automatic selection (see + * {@code CompositePeerLauncher}'s own javadoc on {@code place}/{@code FixedPlacementPolicy}). + * + *

Round 1 resolved {@code acquireWithWorktree}'s profile through + * {@code launcher.routedProfileFor(memberRole)} and then fed that name back into + * {@code launcher.spawn(SpawnRequest)} as an EXPLICIT profile. Naming a profile explicitly + * takes {@code CompositePeerLauncher.spawn}'s THROWING branch, which calls + * {@code enforceMaxLoad} — so the worktree path died with a {@code PlacementException} while + * the exact same unqualified request, with no worktree, still spawned cleanly through the + * routing branch that never checks {@code maxLoad} at all. One intent, two different answers, + * depending only on whether a worktree was asked for — the #425 shape, moved to a different + * filter instead of closed. + * + *

This test does not hardcode which of the two outcomes is correct — whether an unqualified + * spawn SHOULD respect {@code maxLoad} is fleetd #435, a separate ticket. It only asserts that + * the WITH-worktree and WITHOUT-worktree paths agree: both spawn on the same profile, or both + * fail with the same exception type and message. That way this test stays correct however + * #435 is eventually resolved, and only breaks if the two paths disagree again. + */ + @Test + void unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad() { + Map profiles = new LinkedHashMap<>(); + profiles.put("a", new FleetConfig.Profile("a", "http://gx00.gw:8000", "coder-a", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/a", List.of("a.mcp.json"), + null, null, null, null, 1.0f, 1)); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "a", _ -> null); + // liveCount pinned at 1 for "a", exactly matching maxLoad — "a" is permanently at cap, + // regardless of how many times either branch below actually spawns. + PeerLauncher launcher = new CompositePeerLauncher(List.of(adapter), "a", profiles, + PlacementPolicies.fixed(), name -> "a".equals(name) ? 1 : 0); + + Object without = attemptAcquire(() -> + new SessionManager(launcher, new FakeWorktrees(), () -> 0L) + .acquire(null, null, "/caller", null)); + Object with = attemptAcquire(() -> + new SessionManager(launcher, new FakeWorktrees(), () -> 0L) + .acquire(null, null, "/caller", null, new WorktreeRequest("fleetd-425-maxload", null))); + + assertEquals(without, with, "an unqualified spawn on a profile at maxLoad must agree " + + "whether or not a worktree was requested — no condition may become newly fatal " + + "on the worktree path alone (fleetd #425 rework, round 2)"); + } + + /** + * fleetd #425 rework, round 4: the exact regression a mutation test found that 186 green tests + * missed — {@code acquireWithWorktree} dropping the {@link + * dev.ltms.fleet.placement.PlacementDecision} it already resolved via {@code launcher.place}, + * and letting the unqualified spawn re-run placement a second time (a blank-profile {@code + * launcher.spawn(spawnReq)}) instead of carrying that decision forward via {@code + * launcher.spawn(spawnReq, decision)}. Every earlier test in this file uses {@code + * PlacementPolicies.fixed()}, which returns the same answer on every {@code select()} call, so + * dropping the decision is invisible under it — two {@code select()} calls simply agree by + * accident. {@code PlacementPolicies.roundRobin()} is deterministic AND stateful: its {@code + * select()} advances an internal index on every call, so two consecutive calls for the SAME + * spawn (one from {@code place()} to provision the worktree, a second from a dropped-decision + * blank-profile {@code spawn(spawnReq)}) land on DIFFERENT profiles from a two-profile pool — + * index 0 ("a"), then index 1 ("b"). + * + *

This test does not hardcode which profile wins — asserting one specific name would pass + * for the wrong reason the moment the rotation order changes (round-4 brief invariant 3). It + * asserts AGREEMENT instead: whichever profile the worktree's parity overlay was provisioned + * for must be the SAME profile the member actually spawned on. Each profile's overlay list is + * named after the profile itself ({@code "a.mcp.json"}/{@code "b.mcp.json"}), so comparing the + * recorded overlay against {@code s.profile() + ".mcp.json"} checks agreement without ever + * naming an expected winner. + */ + @Test + void acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy() { + Map profiles = new LinkedHashMap<>(); + profiles.put("a", new FleetConfig.Profile("a", "http://gx00.gw:8000", "coder-a", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/a", List.of("a.mcp.json"))); + profiles.put("b", new FleetConfig.Profile("b", "http://gx00.gw:8000", "coder-b", null, + "FLEETD_WORKER_TOKEN", List.of("claude"), "tab", "fleetd-workers", + "worker: {profile} #{n}", null, "/repo/b", List.of("b.mcp.json"))); + FakeHerdr herdr = new FakeHerdr(); + ClaudeCodeLauncher adapter = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), profiles, "a", _ -> null); + // roundRobin is deterministic AND stateful: the first select() call picks index 0 ("a"), + // and the SAME policy instance's second select() call (reached only if the + // PlacementDecision is dropped) picks index 1 ("b") — the two-call disagreement this test + // needs to make a dropped decision observable, rather than merely probable. + PeerLauncher launcher = new CompositePeerLauncher(List.of(adapter), "a", profiles, + PlacementPolicies.roundRobin(), _ -> 0); + + FakeWorktrees worktrees = new FakeWorktrees(); + SessionManager sessions = new SessionManager(launcher, worktrees, () -> 0L); + + MemberSession s = sessions.acquire(null, null, "/caller", + null, new WorktreeRequest("fleetd-425-round4", null)); + + FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); + assertNotNull(overlay, "overlayParity must have been called"); + assertEquals(List.of(s.profile() + ".mcp.json"), overlay.requested(), + "the worktree must be provisioned for the SAME profile the member actually spawned " + + "on — under a rotating policy, dropping the PlacementDecision makes the " + + "second, spawn-time select() call disagree with the first, place()-time " + + "call, so the member ends up on a profile whose worktree (repoRoot/parity " + + "overlay) was built for a DIFFERENT profile (fleetd #425 rework, round 4)"); + } + + /** + * Reduce one {@code acquire(...)} attempt to a value comparable across the with-worktree and + * without-worktree paths: the spawned profile name on success, or the thrown exception's class + * and message on failure. Comparing THIS — instead of asserting "both spawn" or "both throw" as + * a hardcoded direction — is what keeps {@link + * #unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad} valid whichever + * way fleetd #435 eventually resolves whether an unqualified spawn should respect maxLoad. + */ + private static Object attemptAcquire(Supplier call) { + try { + return "spawned:" + call.get().profile(); + } catch (RuntimeException e) { + return "threw:" + e.getClass().getName() + ":" + e.getMessage(); + } + } }