fleetd #425 rework round 2: stop routedProfileFor's caller re-entering the throwing branch
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.
This commit is contained in:
@@ -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<String> unreachable) {
|
||||
List<PlacementCandidate> candidates = candidates(role);
|
||||
@@ -665,22 +668,82 @@ public final class CompositePeerLauncher implements PeerLauncher {
|
||||
/**
|
||||
* {@inheritDoc}
|
||||
*
|
||||
* <p>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.
|
||||
* <p>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.
|
||||
*
|
||||
* <p>Deliberately does <em>not</em> 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}
|
||||
*
|
||||
* <p>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}
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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
|
||||
|
||||
@@ -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 <em>through</em> 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.
|
||||
*
|
||||
* <p>Default implementation returns {@link #defaultProfile()}, ignoring {@code role} and every
|
||||
* <p>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
|
||||
* <em>act</em> 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, <em>without spawning</em>, 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).
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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).
|
||||
*
|
||||
* <p>{@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 <em>different</em>, explicit profile than
|
||||
* the decision it is paired with is a caller bug this method does not attempt to detect.
|
||||
*
|
||||
* <p>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()));
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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).
|
||||
*
|
||||
* <p>The problem this exists to close: a caller that must know the profile <em>before</em> 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.
|
||||
*
|
||||
* <p>{@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) {
|
||||
}
|
||||
@@ -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());
|
||||
|
||||
@@ -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}).
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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<String, FleetConfig.Profile> 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<MemberSession> call) {
|
||||
try {
|
||||
return "spawned:" + call.get().profile();
|
||||
} catch (RuntimeException e) {
|
||||
return "threw:" + e.getClass().getName() + ":" + e.getMessage();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user