From 051d320ea060d9890ca7d577ecbf2c4f5937d235 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 12:23:42 +0700 Subject: [PATCH 1/5] fleetd #425: fleet_profiles' default and worktree provisioning must read live placement fleet_profiles' "default" was CompositePeerLauncher.defaultProfile, a value frozen at construction from cfg.effectiveDefaultProfile(). An unqualified fleet_spawn instead resolves the dev pool live via defaultProfileFor(DEV) on every call, so reordering fleet.developers and reloading changed where a spawn landed without ever changing what fleet_profiles reported. - CompositePeerLauncher.defaultProfile() now delegates to defaultProfileFor(MemberRole.DEV) -- the same live, reload-aware pool read placement already uses -- falling back to the frozen field only when no profiles are configured at all. - PeerLauncher gains a default defaultProfileFor(MemberRole) method so a generic PeerLauncher reference can ask for a role's live default; the default implementation delegates to defaultProfile() for launchers with no pool concept of their own. - SessionManager.acquireWithWorktree resolved a profile via launcher.defaultProfile() (DEV-only) to provision repoRoot/parityOverlay, then spawned with the original (possibly blank) profile, which re-resolves independently through placement -- for any non-DEV role, or across a config reload between the two reads, the two resolutions could disagree and provision a worktree for a profile the member never runs on. Fixed by resolving once, through defaultProfileFor(the caller's actual role), and reusing that same resolved name for repoRoot, parityOverlay, and the spawn itself. Trade-off: this path now spawns with an explicit profile rather than a blank one, so it loses CompositePeerLauncher's cross-candidate retry on PeerUnreachableException -- accepted because a worktree provisioned for the wrong backend is worse than a spawn that fails cleanly and can be retried. Tests: CompositePeerLauncherTest (live dev-pool reorder + empty-pool fallback), FleetProfilesLiveDefaultTest (drives FleetMcp.profilesView directly), SessionManagerTest (worktree overlay follows a reorder, and a non-DEV role's worktree spawn uses that role's pool, not DEV's). --- .../fleet/member/CompositePeerLauncher.java | 33 ++++- .../dev/ltms/fleet/peer/PeerLauncher.java | 25 ++++ .../ltms/fleet/session/SessionManager.java | 22 +++- .../mcp/FleetProfilesLiveDefaultTest.java | 114 ++++++++++++++++ .../member/CompositePeerLauncherTest.java | 83 ++++++++++++ .../fleet/session/SessionManagerTest.java | 124 ++++++++++++++++++ 6 files changed, 395 insertions(+), 6 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesLiveDefaultTest.java 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 49c64fe..3a2a6f9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -615,8 +615,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(); } @@ -765,9 +780,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 2024a3e..352c8e1 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/peer/PeerLauncher.java @@ -143,9 +143,34 @@ 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(); + } + /** * 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 c1f3977..eb955a5 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -584,8 +584,21 @@ 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. String preResolvedProfile = (profile == null || profile.isBlank()) - ? launcher.defaultProfile() : profile; + ? launcher.defaultProfileFor(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 @@ -608,7 +621,10 @@ 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. + handle = launcher.spawn(new SpawnRequest(preResolvedProfile, path, callerCwd, sessionName, resumeSessionId, memberRole)); } catch (RuntimeException e) { log.warn("spawn failed for profile={} role={} branch={} path={}: {}", preResolvedProfile, memberRole, branch, path, e.getMessage()); @@ -636,7 +652,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 ad05a68..94e8ff7 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/CompositePeerLauncherTest.java @@ -919,6 +919,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 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..b09ee1b 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,14 @@ 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.List; import java.util.Map; import java.util.Set; @@ -1991,4 +1998,121 @@ 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"); + } } -- 2.52.0 From b066eb1903717465636bab0875ae809e6d4a205e Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 13:10:30 +0700 Subject: [PATCH 2/5] fleetd #425 rework: resolve acquireWithWorktree through real placement, not a blind pool-first read e1d7dde (PR #430) kept two good fixes and one regressed one. Kept: (1) CompositePeerLauncher.defaultProfile() delegating to defaultProfileFor(MemberRole.DEV) so fleet_profiles' "default" tracks a live reload, and (2) PeerLauncher.defaultProfileFor(MemberRole). Redone: acquireWithWorktree's profile pre-resolution. The regression: acquireWithWorktree pre-resolved via launcher.defaultProfileFor(memberRole), which just returns the role pool's FIRST entry, blind to quarantine/cool-off/model-off. That name was then passed to launcher.spawn as an EXPLICIT profile, which takes CompositePeerLauncher.spawn's THROWING branch (enforceNotQuarantined / enforceMaxLoad / enforceModelEnabled) instead of the ROUTING branch a blank profile gets. So a quarantined or model-off pool-first profile turned a routine unqualified spawn into a hard PlacementException -- undermining fleetd #429's "the fleet keeps working when a model is turned off" guarantee for every worktree spawn. Fix: add PeerLauncher.routedProfileFor(MemberRole), the profile an unqualified spawn of that role would actually be routed to right now -- same candidate list, same quarantined/coolingOff/modelOff filtering, same PlacementPolicy spawn() itself consults. CompositePeerLauncher implements it by extracting spawn()'s context-building into a shared private placementContextFor(role, unreachable), so spawn() and routedProfileFor() can never disagree about which conditions apply to which candidate. acquireWithWorktree now calls routedProfileFor once and reuses that name for repoRoot, parityOverlay, and the spawn -- the fleetd #425 defect (the three disagreeing) stays fixed, now on the routed path instead of the blind one. An explicit profile named by the caller is untouched -- it still hits the throwing branch, which is correct for an operator override. Trade-off carried over from e1d7dde, now precisely scoped: an unqualified worktree spawn still loses CompositePeerLauncher's cross-candidate retry on a live PeerUnreachableException (a transport failure at spawn time, which placement cannot see in advance) -- but NOT the quarantine/cool-off/ model-off routing, which routedProfileFor already resolved before spawn ever runs. Accepted: a worktree provisioned for the wrong backend is worse than a spawn that fails cleanly and can be retried by the caller. Tests: CompositePeerLauncherTest gains routedProfileForSkipsAQuarantinedPoolFirstProfileUnderFixedPolicy and ...ModelOff..., both under PlacementPolicies.fixed() (the default policy, not weighted() -- the previous round's tests all used weighted() and never exercised FixedPlacementPolicy's own inline filter, which is exactly what regressed). SessionManagerTest gains acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile, proving repoRoot/parityOverlay/spawn agree on the ROUTED profile, not just the pool-reordered-by-reload one the existing #425 tests already covered. --- .../fleet/member/CompositePeerLauncher.java | 65 ++++++++++++++----- .../dev/ltms/fleet/peer/PeerLauncher.java | 29 +++++++++ .../ltms/fleet/session/SessionManager.java | 40 ++++++++---- .../member/CompositePeerLauncherTest.java | 61 +++++++++++++++++ .../fleet/session/SessionManagerTest.java | 52 +++++++++++++++ 5 files changed, 218 insertions(+), 29 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 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()); + } } -- 2.52.0 From 6b0a99b2b7cd33cad42d7ea38bc69f32fd005697 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 13:42:06 +0700 Subject: [PATCH 3/5] fleetd #425 rework round 2: stop routedProfileFor's caller re-entering the throwing branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 closed quarantine/cool-off/model-off routing for acquireWithWorktree by resolving the profile through routedProfileFor(role) and handing that name back to launcher.spawn(SpawnRequest) as an EXPLICIT profile. That re-resolution has a cost the lead measured directly: naming a profile explicitly makes CompositePeerLauncher.spawn take its THROWING branch (enforceMaxLoad included), while the routing branch a blank spawn takes never calls enforceMaxLoad at all, and FixedPlacementPolicy (the default) deliberately never evaluates maxLoad during 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 the worktree path's route to the spawn passed through an explicit profile name — a new failure a worktree-less unqualified spawn never hits. This closes the two-path shape instead of moving it: PeerLauncher gains place(role), returning an opaque PlacementDecision, and spawn(req, decision), which honors that decision through the SAME routing branch a blank spawn uses — no enforce* check is newly applied. SessionManager. acquireWithWorktree now keeps the PlacementDecision from place() and hands it to spawn(req, decision) for an unqualified request, instead of re-resolving through an explicit profile name. An explicitly-named profile is unaffected: it still goes through spawn(req) and its throwing branch, exactly as before. Also corrects the acquireWithWorktree comment's false claim that round 1 "loses nothing else" — maxLoad was lost too, as a new hard failure, not a retry. The comment now names it explicitly. Kept the four round-1 tests (still pass — routedProfileFor now just delegates to place()). Added one class asserting the invariant itself: an unqualified spawn on a maxLoad-capped profile must land the same outcome with and without a worktree, asserting on the pair rather than a hardcoded direction, so it stays correct however fleetd #435 (not this ticket) resolves whether maxLoad should gate an unqualified spawn at all. --- .../fleet/member/CompositePeerLauncher.java | 95 +++++++++++++++---- .../dev/ltms/fleet/peer/PeerLauncher.java | 72 ++++++++++++-- .../fleet/placement/PlacementDecision.java | 38 ++++++++ .../ltms/fleet/session/SessionManager.java | 68 ++++++++----- .../fleet/session/SessionManagerTest.java | 67 +++++++++++++ 5 files changed, 293 insertions(+), 47 deletions(-) create mode 100644 fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java 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 f1004de..0bfd11f 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; @@ -638,14 +639,16 @@ public final class CompositePeerLauncher implements PeerLauncher { /** * 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). + * 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 #routedProfileFor} passes - * a fresh empty one since it never retries + * 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); @@ -665,22 +668,82 @@ public final class CompositePeerLauncher implements PeerLauncher { /** * {@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. + *

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

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 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 0200f76..5a17b7f 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; @@ -182,22 +184,74 @@ public interface PeerLauncher { * 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. + * an unqualified spawn will never be routed to. * - *

Default implementation returns {@link #defaultProfile()}, ignoring {@code role} and every + *

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 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. + * + * @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 String routedProfileFor(MemberRole role) { - return defaultProfile(); + 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 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). + * + *

{@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())); } /** 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..1f6fdea --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/placement/PlacementDecision.java @@ -0,0 +1,38 @@ +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 — 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. + * + *

{@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. + * + * @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 6209180..d9e9687 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,7 +585,7 @@ public final class SessionManager implements TurnListener { String ownerTerminal, WorktreeRequest wt, String sessionName, String resumeSessionId, MemberLifecycle.SlotReservation reservation) { - // fleetd #425 rework: resolved through launcher.routedProfileFor(memberRole) — the same + // 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() @@ -592,25 +593,43 @@ public final class SessionManager implements TurnListener { // 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.routedProfileFor(memberRole) : profile; + // 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: 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. + // + // 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 @@ -635,8 +654,13 @@ public final class SessionManager implements TurnListener { worktrees.shareWithGroup(repoRoot, path); // 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. - handle = launcher.spawn(new SpawnRequest(preResolvedProfile, path, callerCwd, sessionName, resumeSessionId, memberRole)); + // 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()); 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 185a400..3f99ffb 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -41,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.*; @@ -2167,4 +2168,70 @@ class SessionManagerTest { 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)"); + } + + /** + * 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(); + } + } } -- 2.52.0 From 9f3671b80147a8a95179e8fb4a3123d1c18a2038 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 14:06:10 +0700 Subject: [PATCH 4/5] 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 -- 2.52.0 From 4b10d02207af11620a05226772463884a50f0667 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 14:24:14 +0700 Subject: [PATCH 5/5] fleetd #425 rework round 4: mutation-pinning test for the dropped PlacementDecision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SessionManager.acquireWithWorktree's unqualified branch must carry the PlacementDecision it already resolved via launcher.place() into launcher.spawn(spawnReq, decision) rather than re-deriving it through a blank-profile launcher.spawn(spawnReq). Every existing test in this file uses PlacementPolicies.fixed(), which answers select() the same way on every call, so dropping the decision (handle = launcher.spawn(spawnReq);) was invisible: 186 tests stayed green under that mutation. acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy uses PlacementPolicies.roundRobin() instead — deterministic AND stateful, so two select() calls on the same policy instance disagree (index 0 then index 1 across a two-profile pool). It asserts AGREEMENT between the profile the worktree's parity overlay was provisioned for and the profile the member actually spawned on, never a hardcoded expected profile name. Verified as a real mutation, not a no-op: applying the exact mutation (handle = launcher.spawn(spawnReq);) turns it red — expected [b.mcp.json] but was [a.mcp.json] — and reverting turns it green again. Full build: 1572 tests, 0 failures, 0 errors, BUILD SUCCESS. --- .../fleet/session/SessionManagerTest.java | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) 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 3f99ffb..2daf2fc 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -2219,6 +2219,64 @@ class SessionManagerTest { + "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 -- 2.52.0