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 3a2a6f9..f1004de 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -402,21 +402,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 +432,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); } } @@ -648,6 +636,53 @@ 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 #routedProfileFor} read, + * so the two can never disagree about which conditions (quarantine, cool-off, model-off) apply + * to which candidate (fleetd #425 rework: the previous round duplicated this into a second, + * blind resolver — {@link #defaultProfileFor} — which is why it regressed). + * + * @param unreachable the caller's mutable unreachable set; {@link #spawn} grows this across + * retries and rebuilds the context from it, {@link #routedProfileFor} 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: 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. + * + * @throws PlacementException if no candidate in {@code role}'s pool is currently placeable + * (mirrors what an actual unqualified spawn would throw) + */ + @Override + public String routedProfileFor(MemberRole role) { + PlacementContext ctx = placementContextFor(role, new HashSet<>()); + return placementPolicy.get().select(ctx).profile(); + } + @Override public String effectiveCwd(SpawnRequest req) { return route(req.profileName()).effectiveCwd(req); 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 352c8e1..0200f76 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java @@ -171,6 +171,35 @@ public interface PeerLauncher { 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, and provisioning against that name (working + * directory, parity overlay) sets a worktree up for a backend the member never runs on — the + * defect this method exists to avoid, without reintroducing the fix that regressed it: naming + * an explicit profile through the throwing enforcement branch of {@link #spawn} + * (quarantine/cool-off/maxLoad/model-off) rather than routing around it the way an unqualified + * spawn does. + * + *

Default implementation returns {@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 String routedProfileFor(MemberRole role) { + return defaultProfile(); + } + /** * 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/session/SessionManager.java b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java index eb955a5..6209180 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -584,21 +584,33 @@ public final class SessionManager implements TurnListener { String ownerTerminal, WorktreeRequest wt, String sessionName, String resumeSessionId, MemberLifecycle.SlotReservation reservation) { - // fleetd #425: resolved through the role's live pool (launcher.defaultProfileFor(memberRole)), - // never launcher.defaultProfile() — that answers for MemberRole.DEV only, and a worktree spawn - // can be for any role. This same resolved name is reused below for repoRoot, parityOverlay, - // AND the spawn itself (an explicit profile, not a blank one) so the worktree is always - // provisioned for the profile the member actually runs on. Before this fix the two could - // disagree: 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. The cost is that an unqualified worktree-provisioned spawn - // no longer gets CompositePeerLauncher's cross-candidate retry on PeerUnreachableException — - // it is now a single explicit-profile spawn, same as one where the caller names a profile. - // 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. + // fleetd #425 rework: resolved through launcher.routedProfileFor(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 name is reused below for repoRoot, parityOverlay, AND the spawn itself (an + // explicit profile, not a blank one) 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. + // The cost is that an unqualified worktree-provisioned spawn no longer gets + // CompositePeerLauncher's cross-candidate retry on PeerUnreachableException — it is now a + // single explicit-profile spawn, same as one where the caller names a profile. That trade is + // still 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. Unlike the first round, + // this loses nothing else: routedProfileFor already routed AROUND every quarantined/ + // cooling-off/model-off candidate before this line ever ran, so the only retry actually lost + // is the one for a live PeerUnreachableException raised by the backend itself at spawn time + // (a transport-level failure placement cannot see in advance) — the same residual gap + // enforceNotQuarantined/enforceMaxLoad/enforceModelEnabled already accept for every + // explicit-profile spawn (see CompositePeerLauncher.spawn's own "no fallback" javadoc). String preResolvedProfile = (profile == null || profile.isBlank()) - ? launcher.defaultProfileFor(memberRole) : profile; + ? launcher.routedProfileFor(memberRole) : 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 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 94e8ff7..c878911 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -1068,6 +1068,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(); @@ -1461,6 +1491,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 b09ee1b..185a400 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -30,6 +30,7 @@ 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; @@ -2115,4 +2116,55 @@ class SessionManagerTest { + "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()); + } }