fleetd #425 rework: resolve acquireWithWorktree via real placement routing #433

Merged

ltms merged 7 commits from worker/425-rework-placement-resolve-c58ba1-9 into main 2026-09-10 09:35:02 +02:00

Conversation 3 Commits 7 Files Changed 7

+965 -22

agent commented 2026-09-10 08:11:05 +02:00

Member

Copy Link

Copy Source

fleetd #425 rework

Follows up on PR #430 (commit e1d7dde), which got two of three things right and regressed the third. This PR cherry-picks e1d7dde onto current main (already has #429/#428), keeps the two good fixes, and replaces the bad one.

Kept from e1d7dde

1. CompositePeerLauncher.defaultProfile() delegates to defaultProfileFor(MemberRole.DEV) — fleet_profiles' "default" is live, not frozen at construction.
2. PeerLauncher.defaultProfileFor(MemberRole) default method.
3. Their tests: FleetProfilesLiveDefaultTest, and the live-default additions to CompositePeerLauncherTest.

The regression, and the fix

acquireWithWorktree pre-resolved a profile via launcher.defaultProfileFor(memberRole) — the role pool's first entry, blind to quarantine/cool-off/model-off. That name then went 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. A quarantined or model-off pool-first profile turned a routine unqualified worktree 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: added PeerLauncher.routedProfileFor(MemberRole) — the profile an unqualified spawn of that role would actually route to right now, using the same candidate list, the same quarantined/coolingOff/modelOff filtering, and the 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. acquireWithWorktree now resolves once through routedProfileFor and reuses that name for repoRoot, parityOverlay, and the spawn.

An explicit profile named by the caller is untouched — still hits the throwing branch, correct for an operator override.

Trade-off (named explicitly, per the ticket's request)

An unqualified worktree spawn still loses CompositePeerLauncher's cross-candidate retry on a live PeerUnreachableException — a transport failure at spawn time that placement cannot see in advance. It does not lose quarantine/cool-off/model-off routing — routedProfileFor already resolves around those 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: routedProfileForSkipsAQuarantinedPoolFirstProfileUnderFixedPolicy, routedProfileForSkipsAModelOffPoolFirstProfileUnderFixedPolicy — both under PlacementPolicies.fixed() (the default policy; the previous round's tests all used weighted() and never exercised FixedPlacementPolicy's own inline filter).
• SessionManagerTest: acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile — proves repoRoot/parityOverlay/spawn all agree on the routed profile (quarantine skips the pool's first entry), not just the pool-reordered-by-reload case the existing #425 tests already cover.
• Kept the two existing SessionManagerTest tests (acquireWithWorktreeProvisionsTheOverlayForTheProfileActuallySpawned, acquireWithWorktreeForANonDevRoleUsesThatRolesPoolNotTheDevPool) unmodified — they pass unchanged because routedProfileFor agrees with defaultProfileFor whenever nothing is quarantined/cooling-off/model-off.

Build

mvn -f fleetd/pom.xml clean install, unpiped, full output read:
Tests run: 1543, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Mutation proof

Three mutations, one per filter site added/moved, each killed exactly the expected test(s), reverted after. A control run (unmutated) passed 140/140 on the three affected classes. Full table in the PR description below / reported to the lead.

---

Round 2 — closing the maxLoad gap the lead found

The lead measured a real regression in the round-1 fix above: an at-cap pool-first profile survives
routedProfileFor (which only filters quarantine/cool-off/model-off), and that name is then fed to
launcher.spawn(SpawnRequest) as an explicit profile — which takes the throwing branch, including
enforceMaxLoad. FixedPlacementPolicy (the default) deliberately never evaluates maxLoad during
automatic selection, so a plain unqualified spawn with no worktree never hits this. The worktree path
now could, purely because its route to the spawn passed through an explicit profile name:

main    7667727  with worktree    -> SPAWNED on profile=a
b066eb1 (round 1) with worktree   -> THREW PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap
b066eb1 (round 1) without worktree -> SPAWNED on profile=a

Same #425 shape as before, moved to a different filter instead of closed.

Mechanism chosen: place(role) + spawn(req, decision)

Took the lead's candidate mechanism directly, since it removes the second code path instead of moving
it again:

• PeerLauncher.place(MemberRole) — resolves a PlacementDecision (an opaque record wrapping the
  chosen profile name) using the exact same candidate list, filtering, and PlacementPolicy a blank
  spawn's routing branch already uses. It does not spawn anything.
• PeerLauncher.spawn(SpawnRequest, PlacementDecision) — spawns by honoring that decision through the
  routing branch (CompositePeerLauncher routes straight to the delegate, no enforce* re-check),
  never the throwing branch. routedProfileFor(role) is kept as a convenience delegating to
  place(role).profile() — the two round-1 tests that call it directly still pass unchanged.
• SessionManager.acquireWithWorktree now keeps the PlacementDecision from place() for an
  unqualified request, and hands it straight to spawn(req, decision) instead of re-resolving through
  an explicit profile name. An explicitly-named profile is untouched: it still goes through
  spawn(req) and the throwing branch, exactly as before this rework — an operator naming one profile
  still gets every enforce* check.

This makes a resolve-then-spawn caller (the worktree path) and a blank-profile spawn(req) caller (the
no-worktree path) go through the identical routing code for the identical decision, so they can never
disagree about which conditions apply — maxLoad included. maxLoad itself is not touched either
way: it stays exactly as unenforced for an unqualified spawn as it is on main today. Whether that
should change is fleetd #435, not this ticket.

The one accepted, unchanged cost from round 1: spawn(req, decision) commits to the one profile
place() already chose, so an unqualified worktree spawn still does not get CompositePeerLauncher's
cross-candidate retry on a live PeerUnreachableException (a transport failure at spawn time placement
cannot see in advance). That trade was already accepted in round 1 and is not widened here.

Also corrected the SessionManager.acquireWithWorktree comment's false claim that round 1 "loses
nothing else" beside that retry — maxLoad was lost too, as a new hard failure, not a retry. The
comment now names maxLoad explicitly, and explains the round-2 mechanism.

Tests

Kept all four round-1 tests unmodified — they still pass, since routedProfileFor now just delegates
to place(role).profile() and behaves identically.

Added one new test, SessionManagerTest#unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad,
implementing the lead's probe: one dev profile a, maxLoad: 1, liveCount pinned at 1,
PlacementPolicies.fixed(), unqualified spawn, run once with a worktree and once without. It asserts
the pair agrees — same profile spawned, or the same exception type + message — never a hardcoded
"spawns" or "throws", so it stays correct however #435 eventually resolves whether maxLoad should
gate an unqualified spawn.

Build

mvn -f <abs>/fleetd/pom.xml clean install, unpiped, full output read:
Tests run: 1544, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Mutation proof (two, one per side of the new seam)

1. SessionManager.java:663 — mutated handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq);
   back to the round-1 shape, handle = launcher.spawn(spawnReq); (always explicit). New probe test
   failed exactly as expected: expected: <spawned:a> but was: <threw:...PlacementException:worker profile 'a' is at maxLoad: 1 live >= 1 cap...> — this is the exact bug the lead found, reproduced
   live in this tree. Reverted; control run passed again (Tests run: 1, Failures: 0).
2. CompositePeerLauncher.java:742 (mirror, the other side of the same seam) — added
   enforceMaxLoad(decision.profile()); as the first line of spawn(SpawnRequest, PlacementDecision).
   Same probe test failed the same way (with-worktree now throws, without-worktree still spawns).
   Reverted; control run passed again.

Before/after probe, run in this tree

Before (round-1 commit b066eb1, per the lead's own measurement — not re-run here since that commit
is superseded, but reproduced live via mutation #1 above, same exception text):

b066eb1 with worktree    -> THREW PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap
b066eb1 without worktree -> SPAWNED on profile=a

After (this round, commit on this branch — from the passing probe test):

with worktree    -> spawned:a
without worktree -> spawned:a

Out of scope, not fixed (per the ticket's instruction)

One line, spotted but not investigated further to confirm it actually diverges: SessionManager. acquire's non-worktree branch and acquireWithWorktree each build their own SpawnRequest inline
instead of sharing one construction helper — structurally the same "one intent, more than one code
path" shape as this ticket, but I have not checked whether the two ever disagree on a check the way
maxLoad did here.

---

Round 3 — main merged fleetd #435, rewrote the prose it made stale

fleetd #435 (merged to main as 5d422f8 while round 2 was in review) made FixedPlacementPolicy
evaluate maxLoad during automatic selection, the same way weighted/round-robin already did.
Round 2's justification for place()/spawn(req, decision) rested partly on fixed NOT doing
that, so several comments now described behaviour that no longer exists.

Unit 1 — merge

git merge origin/main (merge commit 84034b3, on top of 5d422f8). No conflicts — git merge-tree and the actual merge agreed. CompositePeerLauncherTest.java (edited by both sides)
auto-merged cleanly.

Build after merge, before any prose edit:

• mvn -f <abs>/fleetd/pom.xml compile — 0 errors.
• mvn -f <abs>/fleetd/pom.xml test-compile — 0 errors.

Compile errors from the merge: 0.

Unit 2 — the stale prose

Re-grepped myself (grep -n "fixed.*maxLoad\|maxLoad.*fixed\|FixedPlacementPolicy" … across the
four named files) rather than trusting the lead's line numbers, since the merge moved them. Found
and rewrote six sites (five named, plus one the lead's grep pattern didn't catch — the place()
javadoc in CompositePeerLauncher.java had its own independent copy of the same stale claim):

1. CompositePeerLauncher.java — place()'s own javadoc (not one of the five named, found by
   re-grepping myself).
2. CompositePeerLauncher.java — spawn(SpawnRequest, PlacementDecision)'s javadoc (the lead's
   line 728, pre-merge).
3. PeerLauncher.java — routedProfileFor's javadoc (the lead's line 198, pre-merge).
4. PeerLauncher.java — spawn(SpawnRequest, PlacementDecision)'s javadoc.
5. PlacementDecision.java — both paragraphs (the lead's lines 20 and 29, pre-merge).
6. SessionManager.java — acquireWithWorktree's long comment (the lead's line 608, pre-merge).

Each was rewritten, not just trimmed, along the lines the lead laid out:

• The two-path shape is still real and still deliberate: the routing branch (and place()) falls
  through an excluded candidate to the next one; the explicit-profile branch refuses on the same
  condition — correct, because an operator who names a profile should get a refusal, not a silent
  substitution onto a different backend.
• What round 1 got wrong, unchanged: turning a fall-through into a refusal by accident, by
  resolving a name through the routing side and then re-entering the refusing side with it.
• What is no longer true: that fixed ignores maxLoad, and that a PlacementDecision could
  therefore name an at-cap profile that would die at enforceMaxLoad one call later. After #435,
  place() cannot return an at-cap candidate, so that specific failure is gone.
• 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 (another spawn landing on the same profile, a config reload) could
  otherwise move — not a failure #435 already prevents.

Re-grepped again after editing (deliberately never evaluates|deliberately would|ignores maxLoad|does not evaluate maxLoad|never evaluates.*maxLoad|maxLoad.*never evaluat) across all
four files: 0 matches. No stale claim remains.

Unit 3 — re-measured the sibling paths

Same probe as round 2 (SessionManagerTest#unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad,
unchanged): one profile a, maxLoad: 1, liveCount pinned at 1, PlacementPolicies.fixed(),
unqualified spawn, with a worktree and without.

Outcome, verbatim, in this tree after the merge:

without worktree -> threw dev.ltms.fleet.placement.PlacementException:
    worker profile 'a' is at maxLoad (1 live >= 1 cap), and no available candidate remains
with worktree    -> threw dev.ltms.fleet.placement.PlacementException:
    worker profile 'a' is at maxLoad (1 live >= 1 cap), and no available candidate remains

They agree — the observable asymmetry this PR was filed to fix is now closed by #435, not by
anything in this branch. Both paths fail at the same place, for the same reason, before either one
ever reaches a spawn call: launcher.place(memberRole) (called before acquireWithWorktree's
try block) throws the identical PlacementException that the no-worktree path's blank
launcher.spawn(req) throws, because both now go through FixedPlacementPolicy.select()'s new
at-cap refusal. What remains is the structural argument in Unit 2, not an observable failure.

I additionally tried to re-run round 2's mutation proof unchanged (mutate SessionManager.java's
spawn call back to always-explicit) to see if it still had teeth against this exact scenario. It
did not: with only one profile configured, launcher.place(memberRole) now throws before the
mutated line is ever reached, so the mutation was inert — a mutation that never applies looks
exactly like one that passes. To actually re-prove spawn(req, decision) still buys something, I
wrote a throwaway test (not part of this PR — written, run, and deleted, never committed) with a
liveCount supplier that returns 0 on its first call (what place() reads) and 1 on every call
after (simulating a second spawn landing on the profile in the gap before this one commits — a
real race between two separate calls in production). place() approved a; spawn(req, decision) still spawned on a without re-reading liveCount. Mutating
CompositePeerLauncher.java:769 (spawn(SpawnRequest req, PlacementDecision decision)'s first
line) to add enforceMaxLoad(decision.profile()); made it throw
PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap; refusing spawn — no fallback to another profile instead. Reverted; control passed again. This is the throwaway proof
behind the "closes the window" claim in Unit 2 — it is not part of the committed test suite.

Build (full, after the merge and all prose edits, run myself, unpiped, full output read)

mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/0f8770-9/fleetd/pom.xml clean install
Tests run: 1564, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. Compile errors: 0.
(1564 vs. round 2's 1544 — the +20 came from main's #435 merge, not from this branch.)

Other sites with this shape (not fixed, per the ticket's instruction)

Swept every file under src/main/java/dev/ltms/fleet/ referencing PlacementPolicy/placement policy/FixedPlacementPolicy/select( for a comment that justifies itself by what a placement
policy does or does not evaluate. Found none beyond the six already rewritten above — the only
other files with the same wording pattern (ConfigRef.java, PlacementPolicyUtil.java,
RoundRobinPlacementPolicy.java, etc.) use "never"/"deliberately" for unrelated, still-accurate
things (deferred config keys, null maxLoad meaning unlimited).

No behaviour change beyond the merge itself: place() / PlacementDecision /
spawn(req, decision) are untouched, and FixedPlacementPolicy.java / PlacementPolicyUtil.java
were taken wholesale from main, never edited on this branch.

Round 4 — mutation-pinning test for the dropped PlacementDecision

The lead mutated a line I had not touched in prior rounds — SessionManager.java:677:

before: handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq);
after:  handle = launcher.spawn(spawnReq);

That drops the PlacementDecision entirely for the unqualified case. 186 tests, 0 failures — it
survived, because every SessionManager test in this file uses PlacementPolicies.fixed(), and
fixed() answers select() the same way on every call. weighted()/round-robin() do not:
WeightedRoundRobinPolicy mutates a current score map on every call, and
RoundRobinPlacementPolicy advances an AtomicInteger index on every call — two consecutive
select() calls on the SAME policy instance disagree by design. So with the decision dropped:
place() picks profile A (the worktree's repoRoot/parity overlay get built for A), then the
blank-profile launcher.spawn(spawnReq) runs placement a second time and can pick B — the member
spawns on B inside a worktree provisioned for A. Both live hosts run placement: weighted, so
this is the configured case.

New test (no production change)

SessionManagerTest#acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy

Two profiles (a, b) in a LinkedHashMap (definition order fixed), PlacementPolicies.roundRobin()
instead of fixed(). Round-robin is deterministic AND stateful: the first select() call (from
place()) lands on index 0 ("a"); a second select() call on the SAME policy instance (only
reached if the decision is dropped) lands on index 1 ("b"). The assertion checks AGREEMENT, not a
hardcoded profile name: whichever profile the worktree's parity overlay was recorded for
(FakeWorktrees.lastOverlay().requested()) must equal s.profile() + ".mcp.json" — the profile
the member actually spawned on. FakeWorktrees already recorded what it was asked to provision
(OverlayCall.requested()); no test-infrastructure change was needed.

Mutation proof (both outcomes, verbatim)

Applied the exact mutation above to SessionManager.java:677, ran only the new test:

mvn -f <worktree>/fleetd/pom.xml -Dtest=SessionManagerTest#acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy test
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
org.opentest4j.AssertionFailedError: the worktree must be provisioned for the SAME profile the
member actually spawned on — ... ==> expected: <[b.mcp.json]> but was: <[a.mcp.json]>
	at dev.ltms.fleet.session.SessionManagerTest.acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy(SessionManagerTest.java:2272)
[INFO] BUILD FAILURE

RED, and it failed with exactly the predicted mismatch — overlay built for "a" (the first
select(), from place()), member spawned on "b" (the second select(), from the blank-profile
spawn) — confirming the mutated line is actually reached, not short-circuited.

Reverted SessionManager.java:677 to the exact original line (git diff --stat on that file shows
no diff after reverting). Ran the same test again:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

GREEN.

Full build (after revert, run myself, unpiped, full output read)

mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/0f8770-9/fleetd/pom.xml clean install

Tests run: 1572, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. Compile errors (grep -c "^\[ERROR\]"): 0. (1572 vs. round 3's 1564 — the +1 is this round's new test; the other +7 came
from main commits already merged into this branch before this round started — fleetd #421/#434/#436/#438
— not from anything I authored this round.)

Other sites with this shape (report only, not fixed — per the ticket's instruction)

Grepped every caller of launcher.place(/routedProfileFor(/defaultProfileFor( in
src/main/java/dev/ltms/fleet: SessionManager.acquireWithWorktree is the only production caller
of place(), and it now carries the PlacementDecision through correctly — no other caller
resolves a placement decision and then re-derives it. Broader sweep completed (report only,
nothing changed): every other stateful sequence generator in src/main/java/dev/ltms/fleet
(RoundRobinPlacementPolicy.index, WeightedRoundRobinPolicy.current,
HerdrPeerLauncher.labelSeq/nextLabelSeq, GitWorktrees.seq (worktree-path nonce),
SessionManager.nonceSeq (branch nonce), MessageService.ticketSeq, Rendezvous.askSeq,
BackendOutagePolicy.incidentSequence, UnixSocketHerdrClient.ids) is consulted from exactly
ONE call site for its one logical action — none of them has a second, independent caller that
re-derives the same value a first caller already resolved and used. This shape is specific to
placement: place() and the blank-profile branch of spawn() are two separate public entry
points into the SAME stateful select(), which is what let a caller resolve once and (if the
decision is dropped) re-derive via the other entry point. No other stateful resolver in this
codebase currently has two such entry points.

Scope discipline

Only src/test/java/dev/ltms/fleet/session/SessionManagerTest.java was changed and committed.
SessionManager.java:677 was mutated and reverted for the proof above, and git diff --stat on
it shows no diff — confirmed no production change shipped this round.

## fleetd #425 rework Follows up on PR #430 (commit e1d7dde), which got two of three things right and regressed the third. This PR cherry-picks e1d7dde onto current main (already has #429/#428), keeps the two good fixes, and replaces the bad one. ### Kept from e1d7dde 1. `CompositePeerLauncher.defaultProfile()` delegates to `defaultProfileFor(MemberRole.DEV)` — `fleet_profiles`' `"default"` is live, not frozen at construction. 2. `PeerLauncher.defaultProfileFor(MemberRole)` default method. 3. Their tests: `FleetProfilesLiveDefaultTest`, and the live-default additions to `CompositePeerLauncherTest`. ### The regression, and the fix `acquireWithWorktree` pre-resolved a profile via `launcher.defaultProfileFor(memberRole)` — the role pool's *first entry*, blind to quarantine/cool-off/model-off. That name then went 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. A quarantined or model-off pool-first profile turned a routine unqualified worktree 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: added `PeerLauncher.routedProfileFor(MemberRole)` — the profile an unqualified spawn of that role would actually route to right now, using the same candidate list, the same `quarantined`/`coolingOff`/`modelOff` filtering, and the 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. `acquireWithWorktree` now resolves once through `routedProfileFor` and reuses that name for `repoRoot`, `parityOverlay`, and the spawn. An explicit profile named by the caller is untouched — still hits the throwing branch, correct for an operator override. ### Trade-off (named explicitly, per the ticket's request) An unqualified worktree spawn still loses `CompositePeerLauncher`'s cross-candidate retry on a **live** `PeerUnreachableException` — a transport failure at spawn time that placement cannot see in advance. It does **not** lose quarantine/cool-off/model-off routing — `routedProfileFor` already resolves around those 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`: `routedProfileForSkipsAQuarantinedPoolFirstProfileUnderFixedPolicy`, `routedProfileForSkipsAModelOffPoolFirstProfileUnderFixedPolicy` — both under `PlacementPolicies.fixed()` (the default policy; the previous round's tests all used `weighted()` and never exercised `FixedPlacementPolicy`'s own inline filter). - `SessionManagerTest`: `acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile` — proves `repoRoot`/`parityOverlay`/spawn all agree on the *routed* profile (quarantine skips the pool's first entry), not just the pool-reordered-by-reload case the existing #425 tests already cover. - Kept the two existing SessionManagerTest tests (`acquireWithWorktreeProvisionsTheOverlayForTheProfileActuallySpawned`, `acquireWithWorktreeForANonDevRoleUsesThatRolesPoolNotTheDevPool`) unmodified — they pass unchanged because `routedProfileFor` agrees with `defaultProfileFor` whenever nothing is quarantined/cooling-off/model-off. ### Build `mvn -f fleetd/pom.xml clean install`, unpiped, full output read: `Tests run: 1543, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ### Mutation proof Three mutations, one per filter site added/moved, each killed exactly the expected test(s), reverted after. A control run (unmutated) passed 140/140 on the three affected classes. Full table in the PR description below / reported to the lead. --- ## Round 2 — closing the maxLoad gap the lead found The lead measured a real regression in the round-1 fix above: an at-cap pool-first profile survives `routedProfileFor` (which only filters quarantine/cool-off/model-off), and that name is then fed to `launcher.spawn(SpawnRequest)` as an **explicit** profile — which takes the throwing branch, including `enforceMaxLoad`. `FixedPlacementPolicy` (the default) deliberately never evaluates `maxLoad` during automatic selection, so a plain unqualified spawn with no worktree never hits this. The worktree path now could, purely because its route to the spawn passed through an explicit profile name: ``` main 7667727 with worktree -> SPAWNED on profile=a b066eb1 (round 1) with worktree -> THREW PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap b066eb1 (round 1) without worktree -> SPAWNED on profile=a ``` Same #425 shape as before, moved to a different filter instead of closed. ### Mechanism chosen: `place(role)` + `spawn(req, decision)` Took the lead's candidate mechanism directly, since it removes the second code path instead of moving it again: - `PeerLauncher.place(MemberRole)` — resolves a `PlacementDecision` (an opaque record wrapping the chosen profile name) using the exact same candidate list, filtering, and `PlacementPolicy` a blank spawn's routing branch already uses. It does not spawn anything. - `PeerLauncher.spawn(SpawnRequest, PlacementDecision)` — spawns by honoring that decision through the **routing** branch (`CompositePeerLauncher` routes straight to the delegate, no `enforce*` re-check), never the throwing branch. `routedProfileFor(role)` is kept as a convenience delegating to `place(role).profile()` — the two round-1 tests that call it directly still pass unchanged. - `SessionManager.acquireWithWorktree` now keeps the `PlacementDecision` from `place()` for an unqualified request, and hands it straight to `spawn(req, decision)` instead of re-resolving through an explicit profile name. An explicitly-named profile is untouched: it still goes through `spawn(req)` and the throwing branch, exactly as before this rework — an operator naming one profile still gets every `enforce*` check. This makes a resolve-then-spawn caller (the worktree path) and a blank-profile `spawn(req)` caller (the no-worktree path) go through the identical routing code for the identical decision, so they can never disagree about which conditions apply — `maxLoad` included. `maxLoad` itself is **not** touched either way: it stays exactly as unenforced for an unqualified spawn as it is on `main` today. Whether that should change is fleetd #435, not this ticket. The one accepted, unchanged cost from round 1: `spawn(req, decision)` commits to the one profile `place()` already chose, so an unqualified worktree spawn still does not get `CompositePeerLauncher`'s cross-candidate retry on a live `PeerUnreachableException` (a transport failure at spawn time placement cannot see in advance). That trade was already accepted in round 1 and is not widened here. Also corrected the `SessionManager.acquireWithWorktree` comment's false claim that round 1 "loses nothing else" beside that retry — `maxLoad` was lost too, as a new hard failure, not a retry. The comment now names `maxLoad` explicitly, and explains the round-2 mechanism. ### Tests Kept all four round-1 tests unmodified — they still pass, since `routedProfileFor` now just delegates to `place(role).profile()` and behaves identically. Added one new test, `SessionManagerTest#unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad`, implementing the lead's probe: one dev profile `a`, `maxLoad: 1`, `liveCount` pinned at 1, `PlacementPolicies.fixed()`, unqualified spawn, run once with a worktree and once without. It asserts the **pair** agrees — same profile spawned, or the same exception type + message — never a hardcoded "spawns" or "throws", so it stays correct however #435 eventually resolves whether `maxLoad` should gate an unqualified spawn. ### Build `mvn -f <abs>/fleetd/pom.xml clean install`, unpiped, full output read: `Tests run: 1544, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ### Mutation proof (two, one per side of the new seam) 1. `SessionManager.java:663` — mutated `handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq);` back to the round-1 shape, `handle = launcher.spawn(spawnReq);` (always explicit). New probe test failed exactly as expected: `expected: <spawned:a> but was: <threw:...PlacementException:worker profile 'a' is at maxLoad: 1 live >= 1 cap...>` — this is the exact bug the lead found, reproduced live in this tree. Reverted; control run passed again (`Tests run: 1, Failures: 0`). 2. `CompositePeerLauncher.java:742` (mirror, the other side of the same seam) — added `enforceMaxLoad(decision.profile());` as the first line of `spawn(SpawnRequest, PlacementDecision)`. Same probe test failed the same way (with-worktree now throws, without-worktree still spawns). Reverted; control run passed again. ### Before/after probe, run in this tree Before (round-1 commit `b066eb1`, per the lead's own measurement — not re-run here since that commit is superseded, but reproduced live via mutation #1 above, same exception text): ``` b066eb1 with worktree -> THREW PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap b066eb1 without worktree -> SPAWNED on profile=a ``` After (this round, commit on this branch — from the passing probe test): ``` with worktree -> spawned:a without worktree -> spawned:a ``` ### Out of scope, not fixed (per the ticket's instruction) One line, spotted but not investigated further to confirm it actually diverges: `SessionManager. acquire`'s non-worktree branch and `acquireWithWorktree` each build their own `SpawnRequest` inline instead of sharing one construction helper — structurally the same "one intent, more than one code path" shape as this ticket, but I have not checked whether the two ever disagree on a check the way `maxLoad` did here. --- ## Round 3 — main merged fleetd #435, rewrote the prose it made stale fleetd #435 (merged to `main` as `5d422f8` while round 2 was in review) made `FixedPlacementPolicy` evaluate `maxLoad` during automatic selection, the same way `weighted`/`round-robin` already did. Round 2's justification for `place()`/`spawn(req, decision)` rested partly on `fixed` NOT doing that, so several comments now described behaviour that no longer exists. ### Unit 1 — merge `git merge origin/main` (merge commit `84034b3`, on top of `5d422f8`). No conflicts — `git merge-tree` and the actual merge agreed. `CompositePeerLauncherTest.java` (edited by both sides) auto-merged cleanly. **Build after merge, before any prose edit:** - `mvn -f <abs>/fleetd/pom.xml compile` — 0 errors. - `mvn -f <abs>/fleetd/pom.xml test-compile` — 0 errors. Compile errors from the merge: **0**. ### Unit 2 — the stale prose Re-grepped myself (`grep -n "fixed.*maxLoad\|maxLoad.*fixed\|FixedPlacementPolicy" …` across the four named files) rather than trusting the lead's line numbers, since the merge moved them. Found and rewrote **six** sites (five named, plus one the lead's grep pattern didn't catch — the `place()` javadoc in `CompositePeerLauncher.java` had its own independent copy of the same stale claim): 1. `CompositePeerLauncher.java` — `place()`'s own javadoc (not one of the five named, found by re-grepping myself). 2. `CompositePeerLauncher.java` — `spawn(SpawnRequest, PlacementDecision)`'s javadoc (the lead's line 728, pre-merge). 3. `PeerLauncher.java` — `routedProfileFor`'s javadoc (the lead's line 198, pre-merge). 4. `PeerLauncher.java` — `spawn(SpawnRequest, PlacementDecision)`'s javadoc. 5. `PlacementDecision.java` — both paragraphs (the lead's lines 20 and 29, pre-merge). 6. `SessionManager.java` — `acquireWithWorktree`'s long comment (the lead's line 608, pre-merge). Each was rewritten, not just trimmed, along the lines the lead laid out: - The two-path shape is still real and still deliberate: the routing branch (and `place()`) falls through an excluded candidate to the next one; the explicit-profile branch refuses on the same condition — correct, because an operator who names a profile should get a refusal, not a silent substitution onto a different backend. - What round 1 got wrong, unchanged: turning a fall-through into a refusal by accident, by resolving a name through the routing side and then re-entering the refusing side with it. - What is no longer true: that `fixed` ignores `maxLoad`, and that a `PlacementDecision` could therefore name an at-cap profile that would die at `enforceMaxLoad` one call later. After #435, `place()` cannot return an at-cap candidate, so that specific failure is gone. - 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 (another spawn landing on the same profile, a config reload) could otherwise move — not a failure #435 already prevents. Re-grepped again after editing (`deliberately never evaluates|deliberately would|ignores maxLoad|does not evaluate maxLoad|never evaluates.*maxLoad|maxLoad.*never evaluat`) across all four files: **0 matches**. No stale claim remains. ### Unit 3 — re-measured the sibling paths Same probe as round 2 (`SessionManagerTest#unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad`, unchanged): one profile `a`, `maxLoad: 1`, `liveCount` pinned at 1, `PlacementPolicies.fixed()`, unqualified spawn, with a worktree and without. **Outcome, verbatim, in this tree after the merge:** ``` without worktree -> threw dev.ltms.fleet.placement.PlacementException: worker profile 'a' is at maxLoad (1 live >= 1 cap), and no available candidate remains with worktree -> threw dev.ltms.fleet.placement.PlacementException: worker profile 'a' is at maxLoad (1 live >= 1 cap), and no available candidate remains ``` They agree — **the observable asymmetry this PR was filed to fix is now closed by #435**, not by anything in this branch. Both paths fail at the same place, for the same reason, before either one ever reaches a spawn call: `launcher.place(memberRole)` (called before `acquireWithWorktree`'s `try` block) throws the identical `PlacementException` that the no-worktree path's blank `launcher.spawn(req)` throws, because both now go through `FixedPlacementPolicy.select()`'s new at-cap refusal. What remains is the structural argument in Unit 2, not an observable failure. I additionally tried to re-run round 2's mutation proof unchanged (mutate `SessionManager.java`'s spawn call back to always-explicit) to see if it still had teeth against this exact scenario. It did not: with only one profile configured, `launcher.place(memberRole)` now throws *before* the mutated line is ever reached, so the mutation was inert — a mutation that never applies looks exactly like one that passes. To actually re-prove `spawn(req, decision)` still buys something, I wrote a throwaway test (not part of this PR — written, run, and deleted, never committed) with a `liveCount` supplier that returns 0 on its first call (what `place()` reads) and 1 on every call after (simulating a second spawn landing on the profile in the gap before this one commits — a real race between two separate calls in production). `place()` approved `a`; `spawn(req, decision)` still spawned on `a` without re-reading `liveCount`. Mutating `CompositePeerLauncher.java:769` (`spawn(SpawnRequest req, PlacementDecision decision)`'s first line) to add `enforceMaxLoad(decision.profile());` made it throw `PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap; refusing spawn — no fallback to another profile` instead. Reverted; control passed again. This is the throwaway proof behind the "closes the window" claim in Unit 2 — it is not part of the committed test suite. ### Build (full, after the merge and all prose edits, run myself, unpiped, full output read) `mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/0f8770-9/fleetd/pom.xml clean install` `Tests run: 1564, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. Compile errors: 0. (1564 vs. round 2's 1544 — the +20 came from main's #435 merge, not from this branch.) ### Other sites with this shape (not fixed, per the ticket's instruction) Swept every file under `src/main/java/dev/ltms/fleet/` referencing `PlacementPolicy`/`placement policy`/`FixedPlacementPolicy`/`select(` for a comment that justifies itself by what a placement policy does or does not evaluate. Found none beyond the six already rewritten above — the only other files with the same wording pattern (`ConfigRef.java`, `PlacementPolicyUtil.java`, `RoundRobinPlacementPolicy.java`, etc.) use "never"/"deliberately" for unrelated, still-accurate things (deferred config keys, `null` maxLoad meaning unlimited). No behaviour change beyond the merge itself: `place()` / `PlacementDecision` / `spawn(req, decision)` are untouched, and `FixedPlacementPolicy.java` / `PlacementPolicyUtil.java` were taken wholesale from main, never edited on this branch. ## Round 4 — mutation-pinning test for the dropped PlacementDecision The lead mutated a line I had not touched in prior rounds — `SessionManager.java:677`: ``` before: handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq); after: handle = launcher.spawn(spawnReq); ``` That drops the `PlacementDecision` entirely for the unqualified case. 186 tests, 0 failures — it survived, because every `SessionManager` test in this file uses `PlacementPolicies.fixed()`, and `fixed()` answers `select()` the same way on every call. `weighted()`/`round-robin()` do not: `WeightedRoundRobinPolicy` mutates a `current` score map on every call, and `RoundRobinPlacementPolicy` advances an `AtomicInteger index` on every call — two consecutive `select()` calls on the SAME policy instance disagree by design. So with the decision dropped: `place()` picks profile A (the worktree's `repoRoot`/parity overlay get built for A), then the blank-profile `launcher.spawn(spawnReq)` runs placement a second time and can pick B — the member spawns on B inside a worktree provisioned for A. Both live hosts run `placement: weighted`, so this is the configured case. ### New test (no production change) `SessionManagerTest#acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy` Two profiles (`a`, `b`) in a `LinkedHashMap` (definition order fixed), `PlacementPolicies.roundRobin()` instead of `fixed()`. Round-robin is deterministic AND stateful: the first `select()` call (from `place()`) lands on index 0 ("a"); a second `select()` call on the SAME policy instance (only reached if the decision is dropped) lands on index 1 ("b"). The assertion checks AGREEMENT, not a hardcoded profile name: whichever profile the worktree's parity overlay was recorded for (`FakeWorktrees.lastOverlay().requested()`) must equal `s.profile() + ".mcp.json"` — the profile the member actually spawned on. `FakeWorktrees` already recorded what it was asked to provision (`OverlayCall.requested()`); no test-infrastructure change was needed. ### Mutation proof (both outcomes, verbatim) Applied the exact mutation above to `SessionManager.java:677`, ran only the new test: ``` mvn -f <worktree>/fleetd/pom.xml -Dtest=SessionManagerTest#acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy test [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 org.opentest4j.AssertionFailedError: the worktree must be provisioned for the SAME profile the member actually spawned on — ... ==> expected: <[b.mcp.json]> but was: <[a.mcp.json]> at dev.ltms.fleet.session.SessionManagerTest.acquireWithWorktreeSpawnsOnTheSameProfileItProvisionedTheWorktreeForUnderARotatingPolicy(SessionManagerTest.java:2272) [INFO] BUILD FAILURE ``` RED, and it failed with exactly the predicted mismatch — overlay built for "a" (the first `select()`, from `place()`), member spawned on "b" (the second `select()`, from the blank-profile spawn) — confirming the mutated line is actually reached, not short-circuited. Reverted `SessionManager.java:677` to the exact original line (`git diff --stat` on that file shows no diff after reverting). Ran the same test again: ``` [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` GREEN. ### Full build (after revert, run myself, unpiped, full output read) `mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/0f8770-9/fleetd/pom.xml clean install` `Tests run: 1572, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. Compile errors (`grep -c "^\[ERROR\]"`): 0. (1572 vs. round 3's 1564 — the +1 is this round's new test; the other +7 came from main commits already merged into this branch before this round started — fleetd #421/#434/#436/#438 — not from anything I authored this round.) ### Other sites with this shape (report only, not fixed — per the ticket's instruction) Grepped every caller of `launcher.place(`/`routedProfileFor(`/`defaultProfileFor(` in `src/main/java/dev/ltms/fleet`: `SessionManager.acquireWithWorktree` is the only production caller of `place()`, and it now carries the `PlacementDecision` through correctly — no other caller resolves a placement decision and then re-derives it. Broader sweep completed (report only, nothing changed): every other stateful sequence generator in `src/main/java/dev/ltms/fleet` (`RoundRobinPlacementPolicy.index`, `WeightedRoundRobinPolicy.current`, `HerdrPeerLauncher.labelSeq`/`nextLabelSeq`, `GitWorktrees.seq` (worktree-path nonce), `SessionManager.nonceSeq` (branch nonce), `MessageService.ticketSeq`, `Rendezvous.askSeq`, `BackendOutagePolicy.incidentSequence`, `UnixSocketHerdrClient.ids`) is consulted from exactly ONE call site for its one logical action — none of them has a second, independent caller that re-derives the same value a first caller already resolved and used. This shape is specific to placement: `place()` and the blank-profile branch of `spawn()` are two separate public entry points into the SAME stateful `select()`, which is what let a caller resolve once and (if the decision is dropped) re-derive via the other entry point. No other stateful resolver in this codebase currently has two such entry points. ### Scope discipline Only `src/test/java/dev/ltms/fleet/session/SessionManagerTest.java` was changed and committed. `SessionManager.java:677` was mutated and reverted for the proof above, and `git diff --stat` on it shows no diff — confirmed no production change shipped this round.

agent added 2 commits 2026-09-10 08:11:05 +02:00

fleetd #425: fleet_profiles' default and worktree provisioning must read live placement ... 051d320ea0

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).

fleetd #425 rework: resolve acquireWithWorktree through real placement, not a blind pool-first read ... b066eb1903

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.

ltms commented 2026-09-10 08:23:54 +02:00

Owner

Copy Link

Copy Source

Not merging yet. The two-paths split moved one filter over instead of closing.

The design is right and most of this PR is right: routedProfileFor reading the same placementContextFor + one select is the correct single source, and killing the blind defaultProfileFor resolver was the fix #430 needed. Quarantine, cool-off and model-off are genuinely closed, and the four new tests prove it.

But maxLoad is not, and the PR's own comment says it is. This sentence in SessionManager is false:

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.

maxLoad is lost too — and not as a retry. As a new hard failure.

Why

spawn's blank-profile branch never calls enforceMaxLoad. Capacity reaches it only through the placement candidates, and FixedPlacementPolicy — the default policy — deliberately ignores capacity ("capacity gating for automatic placement is deliberately out of scope for fixed"). So a capped profile survives select, routedProfileFor returns its name, and the spawn — now explicit — hits enforceMaxLoad at CompositePeerLauncher.java:394 and throws.

Measured

One probe, run in both trees, one variable: a single dev profile a with maxLoad: 1, liveCount fixed at 1 (exactly at cap), PlacementPolicies.fixed(), unqualified spawn with a worktree.

MAIN    7667727 → PROBE_RESULT: SPAWNED on profile=a
REWORK  b066eb1 → PROBE_RESULT: THREW PlacementException: worker profile 'a' is at maxLoad:
                  1 live >= 1 cap; refusing spawn — no fallback to another profile

Then the same profile at the same cap, in the rework tree alone, with and without a worktree:

b066eb1  with worktree    → THREW PlacementException: … at maxLoad: 1 live >= 1 cap
b066eb1  without worktree → SPAWNED on profile=a

That second pair is the finding. One intent — "spawn a dev, let the fleet choose the profile" — now gets two different answers in the same build, decided by whether a worktree was asked for. That is the same shape #425 is about, one filter over.

Why this matters in real operation

fleet_spawn{worktree:true, ticket} with no profile is the standard delegation call, fixed is the default policy, and profiles do sit at cap — sonnet is at free: 0, maxLoad: 3, live: 3 on this daemon as I write this. Under this PR that call starts refusing whenever the pool's first profile is full, where today it spawns.

I am not asking you to decide whether an unqualified spawn should respect maxLoad. It probably should. But it must not start doing so as a side effect of a provisioning fix, on one of the two paths only. That is a separate ticket and I will file it.

What the next round has to hold

Goal. repoRoot, parityOverlay and the spawn all name one profile — keep that, it is this PR's whole point.

Invariant to add. For every placement policy and every profile state, an unqualified acquire with a worktree and an unqualified acquire without one either both succeed on the same profile, or both fail with the same exception. No condition may become newly fatal on the worktree path alone.

Candidate mechanism, not a requirement. Have the launcher hand out a placement decision and then honour it: something like place(role) returning an opaque decision that spawn(req, decision) accepts, skipping exactly the checks placement already applied. That removes the second path rather than moving it. If you see a better way to get one path, take it and say why.

Test. Both halves of the probe above, as one test class: pool-first profile at maxLoad, unqualified spawn, with a worktree and without, asserting the two agree. Please also keep the four tests already here.

The retry. Losing CompositePeerLauncher's cross-candidate retry on a live PeerUnreachableException for a worktree spawn is a real cost and I accept it — a worktree provisioned for the wrong backend is worse. Keep that part of the comment; just correct the claim that nothing else is lost.

One thing to fix in the prose either way

The long SessionManager comment is good and I want it kept, but the "loses nothing else" sentence has to name maxLoad. A comment that lists its own exceptions and gets the list wrong is worse than no comment — a later session reads it as the audit.

## Not merging yet. The two-paths split moved one filter over instead of closing. The design is right and most of this PR is right: `routedProfileFor` reading the same `placementContextFor` + one `select` is the correct single source, and killing the blind `defaultProfileFor` resolver was the fix #430 needed. Quarantine, cool-off and model-off are genuinely closed, and the four new tests prove it. But `maxLoad` is not, and the PR's own comment says it is. This sentence in `SessionManager` is false: > 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. `maxLoad` is lost too — and not as a retry. As a new hard failure. ### Why `spawn`'s blank-profile branch never calls `enforceMaxLoad`. Capacity reaches it only through the placement candidates, and `FixedPlacementPolicy` — the default policy — deliberately ignores capacity ("capacity gating for automatic placement is deliberately out of scope for `fixed`"). So a capped profile survives `select`, `routedProfileFor` returns its name, and the spawn — now explicit — hits `enforceMaxLoad` at `CompositePeerLauncher.java:394` and throws. ### Measured One probe, run in both trees, one variable: a single dev profile `a` with `maxLoad: 1`, `liveCount` fixed at 1 (exactly at cap), `PlacementPolicies.fixed()`, unqualified spawn with a worktree. ``` MAIN 7667727 → PROBE_RESULT: SPAWNED on profile=a REWORK b066eb1 → PROBE_RESULT: THREW PlacementException: worker profile 'a' is at maxLoad: 1 live >= 1 cap; refusing spawn — no fallback to another profile ``` Then the same profile at the same cap, in the **rework tree alone**, with and without a worktree: ``` b066eb1 with worktree → THREW PlacementException: … at maxLoad: 1 live >= 1 cap b066eb1 without worktree → SPAWNED on profile=a ``` That second pair is the finding. One intent — "spawn a dev, let the fleet choose the profile" — now gets two different answers in the same build, decided by whether a worktree was asked for. That is the same shape #425 is about, one filter over. ### Why this matters in real operation `fleet_spawn{worktree:true, ticket}` with no `profile` is the standard delegation call, `fixed` is the default policy, and profiles do sit at cap — `sonnet` is at `free: 0, maxLoad: 3, live: 3` on this daemon as I write this. Under this PR that call starts refusing whenever the pool's first profile is full, where today it spawns. I am not asking you to decide whether an unqualified spawn *should* respect `maxLoad`. It probably should. But it must not start doing so as a side effect of a provisioning fix, on one of the two paths only. That is a separate ticket and I will file it. ### What the next round has to hold **Goal.** `repoRoot`, `parityOverlay` and the spawn all name one profile — keep that, it is this PR's whole point. **Invariant to add.** For every placement policy and every profile state, an unqualified `acquire` with a worktree and an unqualified `acquire` without one either both succeed on the same profile, or both fail with the same exception. No condition may become newly fatal on the worktree path alone. **Candidate mechanism, not a requirement.** Have the launcher hand out a placement decision and then honour it: something like `place(role)` returning an opaque decision that `spawn(req, decision)` accepts, skipping exactly the checks placement already applied. That removes the second path rather than moving it. If you see a better way to get one path, take it and say why. **Test.** Both halves of the probe above, as one test class: pool-first profile at `maxLoad`, unqualified spawn, with a worktree and without, asserting the two agree. Please also keep the four tests already here. **The retry.** Losing `CompositePeerLauncher`'s cross-candidate retry on a live `PeerUnreachableException` for a worktree spawn is a real cost and I accept it — a worktree provisioned for the wrong backend is worse. Keep that part of the comment; just correct the claim that nothing else is lost. ### One thing to fix in the prose either way The long `SessionManager` comment is good and I want it kept, but the "loses nothing else" sentence has to name `maxLoad`. A comment that lists its own exceptions and gets the list wrong is worse than no comment — a later session reads it as the audit.

ltms referenced this pull request 2026-09-10 08:26:46 +02:00

maxLoad says it is excluded from "every automatic policy" — fixed, the default, ignores it #435

ltms referenced this pull request 2026-09-10 08:27:46 +02:00

fleetd #425: fleet_profiles' default and worktree provisioning must read live placement #430

agent added 1 commit 2026-09-10 08:42:15 +02:00

fleetd #425 rework round 2: stop routedProfileFor's caller re-entering the throwing branch ... 6b0a99b2b7

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.

ltms referenced this pull request 2026-09-10 08:44:55 +02:00

maxLoad says it is excluded from "every automatic policy" — fixed, the default, ignores it #435

ltms commented 2026-09-10 08:48:51 +02:00

Owner

Copy Link

Copy Source

Retracting one paragraph of my rejection: the live-daemon claim was wrong

My rejection of this PR stands. The same-tree asymmetry table is unaffected, and round 2 (task-15) proceeds unchanged. But one paragraph of my reasoning was false, and it was the paragraph that made the problem sound urgent. I wrote:

fleet_spawn{worktree:true, ticket} with no profile is the standard delegation call, fixed is the default policy, and profiles do sit at cap — sonnet is at free: 0, maxLoad: 3, live: 3 on this daemon as I write this. Under this PR that call starts refusing whenever the pool's first profile is full, where today it spawns.

The first clause is right. The second is wrong, and it invalidates the rest of the sentence.

This daemon does not run fixed. It sets the policy explicitly:

$ grep -n '^placement:' fleetd/fleetd.yaml
210:placement: weighted
$ grep -cE '^[a-z][A-Za-z0-9_]*:' fleetd/fleetd.yaml      # control: the anchor does match top-level keys
14

I checked the other host too, because I had made the same assumption about both:

$ ssh fleet01 "grep -n '^placement:' /home/ltms/LTMS/fleetd/fleetd/fleetd.yaml"
172:placement: weighted

weighted routes through PlacementPolicyUtil.available(), which skips an at-cap candidate:

Integer cap = c.maxLoad();
if (cap != null) {
    int live = ctx.liveCount().apply(c.profile());
    if (live >= cap) {
        continue;
    }
}

Callers of that helper, measured just now — RoundRobinPlacementPolicy.java:17 and WeightedRoundRobinPolicy.java:21. Not FixedPlacementPolicy.

So sonnet sitting at free: 0, maxLoad: 3, live: 3 was a true reading of an irrelevant number. On a weighted daemon a full profile is already skipped, and the unqualified delegation spawn lands on the next candidate. The behaviour change I predicted for the live fleet cannot happen here, because the branch I predicted it in is not the branch this daemon takes.

Why I got it wrong. My first pass used grep '^placement:\s*$'. That anchor needs the value on the next line, so a same-line placement: weighted matches nothing. I read the empty result as "no key set" and went to fromName(""), which really does return fixed():

if (n.isBlank() || "fixed".equals(n)) {
    return fixed();
}

Every step after the grep was sound. The grep was the defect. A pattern that finds nothing and a file that says nothing produce the same output, and I did not run a control that would tell them apart.

What this changes for round 2

Nothing in the requirements. The asymmetry is a real defect on its own terms: within one tree, with worktree → THREW and without worktree → SPAWNED for the same request. That measurement never depended on which policy the live host runs.

What changes is the justification you should write in the PR body. Do not carry my "the live fleet starts refusing" argument forward — it is false and a reviewer will find it. The honest case is:

1. Two code paths serve one intent and apply different filter sets. The explicit branch calls enforceMaxLoad; the blank branch never does.
2. That inconsistency is wrong regardless of which policy is configured, because the caller cannot predict which branch its own arguments select.
3. It is currently masked on both live hosts by placement: weighted. Masked is not fixed.

Point 3 is worth stating out loud rather than leaving out. A reviewer who checks the live config will find the masking anyway, and a PR that names its own limited blast radius reads as more careful than one that oversells it.

## Retracting one paragraph of my rejection: the live-daemon claim was wrong My rejection of this PR stands. The same-tree asymmetry table is unaffected, and round 2 (task-15) proceeds unchanged. But one paragraph of my reasoning was false, and it was the paragraph that made the problem sound urgent. I wrote: > `fleet_spawn{worktree:true, ticket}` with no `profile` is the standard delegation call, `fixed` is the default policy, and profiles do sit at cap — `sonnet` is at `free: 0, maxLoad: 3, live: 3` on this daemon as I write this. Under this PR that call starts refusing whenever the pool's first profile is full, where today it spawns. The first clause is right. The second is wrong, and it invalidates the rest of the sentence. **This daemon does not run `fixed`.** It sets the policy explicitly: ``` $ grep -n '^placement:' fleetd/fleetd.yaml 210:placement: weighted $ grep -cE '^[a-z][A-Za-z0-9_]*:' fleetd/fleetd.yaml # control: the anchor does match top-level keys 14 ``` I checked the other host too, because I had made the same assumption about both: ``` $ ssh fleet01 "grep -n '^placement:' /home/ltms/LTMS/fleetd/fleetd/fleetd.yaml" 172:placement: weighted ``` `weighted` routes through `PlacementPolicyUtil.available()`, which skips an at-cap candidate: ```java Integer cap = c.maxLoad(); if (cap != null) { int live = ctx.liveCount().apply(c.profile()); if (live >= cap) { continue; } } ``` Callers of that helper, measured just now — `RoundRobinPlacementPolicy.java:17` and `WeightedRoundRobinPolicy.java:21`. Not `FixedPlacementPolicy`. So `sonnet` sitting at `free: 0, maxLoad: 3, live: 3` was a true reading of an irrelevant number. On a `weighted` daemon a full profile is already skipped, and the unqualified delegation spawn lands on the next candidate. The behaviour change I predicted for the live fleet cannot happen here, because the branch I predicted it in is not the branch this daemon takes. **Why I got it wrong.** My first pass used `grep '^placement:\s*$'`. That anchor needs the value on the *next* line, so a same-line `placement: weighted` matches nothing. I read the empty result as "no key set" and went to `fromName("")`, which really does return `fixed()`: ```java if (n.isBlank() || "fixed".equals(n)) { return fixed(); } ``` Every step after the grep was sound. The grep was the defect. A pattern that finds nothing and a file that says nothing produce the same output, and I did not run a control that would tell them apart. ## What this changes for round 2 Nothing in the requirements. The asymmetry is a real defect on its own terms: within one tree, `with worktree → THREW` and `without worktree → SPAWNED` for the same request. That measurement never depended on which policy the live host runs. What changes is the **justification** you should write in the PR body. Do not carry my "the live fleet starts refusing" argument forward — it is false and a reviewer will find it. The honest case is: 1. Two code paths serve one intent and apply different filter sets. The explicit branch calls `enforceMaxLoad`; the blank branch never does. 2. That inconsistency is wrong regardless of which policy is configured, because the caller cannot predict which branch its own arguments select. 3. It is currently masked on both live hosts by `placement: weighted`. Masked is not fixed. Point 3 is worth stating out loud rather than leaving out. A reviewer who checks the live config will find the masking anyway, and a PR that names its own limited blast radius reads as more careful than one that oversells it.

agent added 2 commits 2026-09-10 09:06:25 +02:00

Merge remote-tracking branch 'origin/main' into worker/425-rework-placement-resolve-c58ba1-9 84034b34d1

fleetd #425 rework round 3: rewrite prose after #435 made fixed honour maxLoad ... 9f3671b801

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.

ltms commented 2026-09-10 09:14:43 +02:00

Owner

Copy Link

Copy Source

Round 3 verified, and one gap found. Round 4 delegated — test only, no production change.

I re-ran everything rather than reading the report. Merged current main (12cff28) into the branch myself: no conflicts, 1571 tests green, 0 compile errors. The prose claim checks out — 0 remaining sites saying fixed ignores maxLoad, across all four files.

Round 3's own corrections to me are accepted. There were six stale sites, not the five I listed; my grep pattern missed a second copy in CompositePeerLauncher.place()'s javadoc. Handing a worker a count as fact was the error, and the worker re-deriving it was right.

Mutation battery — control 186 green

I mutated two lines the implementer did not.

M2 — place() returns the pool-first default (the original #425 defect): KILLED.

public PlacementDecision place(MemberRole role) {
    return new PlacementDecision(defaultProfileFor(role));   // was: placementPolicy.get().select(ctx).profile()
}

Failed 4 tests, two of them at the caller: SessionManagerTest.acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile, SessionManagerTest.unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad, plus two CompositePeerLauncherTest routedProfileFor tests. So "placement, not a blind pool-first read" is genuinely pinned.

M1 — the SessionManager call site drops the decision: SURVIVED.

before: handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq);
after:  handle = launcher.spawn(spawnReq);

186 tests, 0 failures. Nothing objects to the whole mechanism being removed at its only production call site.

This is not an equivalent mutant

I checked before calling it a gap, because "the mutation changed nothing observable" is the honest alternative explanation. It does change behaviour, and the reason is that select() mutates state:

// WeightedRoundRobinPolicy.select()
double score = current.merge(c.profile(), (double) c.weight(), (old, add) -> old + add);
...
current.put(best.profile(), current.get(best.profile()) - total);

// RoundRobinPlacementPolicy.select()
return available.get(index.getAndIncrement() % available.size());

Two consecutive calls return different profiles by design. So under the mutation:

• place() picks A; repoRoot and the parity overlay are provisioned for A.
• the blank spawn(spawnReq) runs placement a second time and picks B.
• the member runs on B in a worktree built for A.

That is precisely the invariant this branch's own comment claims to hold, at SessionManager.java:643: "the overlay/repoRoot above and the spawn here must name the same profile … that agreement is the invariant this rework exists to hold."

And it is the configured case, not a corner — both live hosts run placement: weighted (Mac fleetd.yaml:210, fleet01 fleetd.yaml:172).

A second consequence, not part of the fix: two select() calls for one spawn also consume two steps of the rotation, skewing distribution away from the configured ratios.

Why the suite could not see it

$ grep -cE 'weighted\(\)|roundRobin\(\)' SessionManagerTest.java
0
$ grep -cE 'fixed\(\)' SessionManagerTest.java
4

Every SessionManager test uses fixed(). Under fixed, two select() calls agree, so dropping the decision is invisible. The fixture's policy choice is what hides the defect — the tests are not weak about the assertion, they are weak about the setup.

This is the same shape as the lesson this repo keeps relearning: a test on the seam does not prove the caller. Round 2's mutation proved spawn(req, decision) behaves; round 3's race test called the launcher directly. Neither drove acquireWithWorktree.

Round 4 — what I asked for

One test, no production change. It must use a rotating policy, and it must assert the agreement between the provisioned profile and the spawned profile rather than a hardcoded name. I told the worker that a green run is not evidence here — the buggy code is green too — so it must apply my M1 mutation, show the new test red, revert, show it green, and paste both.

The production code in this branch is, as far as I can measure, correct. It is the pin that is missing, and the branch is not mergeable without it: an unpinned invariant that only breaks under the policy both live fleets run is exactly the thing that regresses on the next refactor. This is the third round on #425 for the same reason each time — the mechanism was right and the proof was aimed one layer too low.

## Round 3 verified, and one gap found. Round 4 delegated — test only, no production change. I re-ran everything rather than reading the report. Merged current main (`12cff28`) into the branch myself: no conflicts, **1571 tests green, 0 compile errors**. The prose claim checks out — 0 remaining sites saying `fixed` ignores `maxLoad`, across all four files. Round 3's own corrections to me are accepted. There were **six** stale sites, not the five I listed; my grep pattern missed a second copy in `CompositePeerLauncher.place()`'s javadoc. Handing a worker a count as fact was the error, and the worker re-deriving it was right. ## Mutation battery — control 186 green I mutated two lines the implementer did not. **M2 — `place()` returns the pool-first default (the original #425 defect): KILLED.** ```java public PlacementDecision place(MemberRole role) { return new PlacementDecision(defaultProfileFor(role)); // was: placementPolicy.get().select(ctx).profile() } ``` Failed 4 tests, two of them at the caller: `SessionManagerTest.acquireWithWorktreeRoutesAroundAQuarantinedPoolFirstProfile`, `SessionManagerTest.unqualifiedAcquireAgreesWithAndWithoutAWorktreeWhenTheOnlyProfileIsAtMaxLoad`, plus two `CompositePeerLauncherTest` `routedProfileFor` tests. So "placement, not a blind pool-first read" is genuinely pinned. **M1 — the `SessionManager` call site drops the decision: SURVIVED.** ``` before: handle = unqualifiedProfile ? launcher.spawn(spawnReq, decision) : launcher.spawn(spawnReq); after: handle = launcher.spawn(spawnReq); ``` 186 tests, 0 failures. Nothing objects to the whole mechanism being removed at its only production call site. ## This is not an equivalent mutant I checked before calling it a gap, because "the mutation changed nothing observable" is the honest alternative explanation. It does change behaviour, and the reason is that `select()` mutates state: ```java // WeightedRoundRobinPolicy.select() double score = current.merge(c.profile(), (double) c.weight(), (old, add) -> old + add); ... current.put(best.profile(), current.get(best.profile()) - total); // RoundRobinPlacementPolicy.select() return available.get(index.getAndIncrement() % available.size()); ``` Two consecutive calls return different profiles **by design**. So under the mutation: - `place()` picks A; `repoRoot` and the parity overlay are provisioned for A. - the blank `spawn(spawnReq)` runs placement a second time and picks B. - the member runs on B in a worktree built for A. That is precisely the invariant this branch's own comment claims to hold, at `SessionManager.java:643`: *"the overlay/repoRoot above and the spawn here must name the same profile … that agreement is the invariant this rework exists to hold."* And it is the configured case, not a corner — both live hosts run `placement: weighted` (Mac `fleetd.yaml:210`, fleet01 `fleetd.yaml:172`). A second consequence, not part of the fix: two `select()` calls for one spawn also consume two steps of the rotation, skewing distribution away from the configured ratios. ## Why the suite could not see it ``` $ grep -cE 'weighted\(\)|roundRobin\(\)' SessionManagerTest.java 0 $ grep -cE 'fixed\(\)' SessionManagerTest.java 4 ``` Every `SessionManager` test uses `fixed()`. Under `fixed`, two `select()` calls agree, so dropping the decision is invisible. The fixture's policy choice is what hides the defect — the tests are not weak about the assertion, they are weak about the setup. This is the same shape as the lesson this repo keeps relearning: a test on the seam does not prove the caller. Round 2's mutation proved `spawn(req, decision)` behaves; round 3's race test called the launcher directly. Neither drove `acquireWithWorktree`. ## Round 4 — what I asked for One test, no production change. It must use a rotating policy, and it must assert the **agreement** between the provisioned profile and the spawned profile rather than a hardcoded name. I told the worker that a green run is not evidence here — the buggy code is green too — so it must apply my M1 mutation, show the new test red, revert, show it green, and paste both. The production code in this branch is, as far as I can measure, correct. It is the pin that is missing, and the branch is not mergeable without it: an unpinned invariant that only breaks under the policy both live fleets run is exactly the thing that regresses on the next refactor. This is the third round on #425 for the same reason each time — the mechanism was right and the proof was aimed one layer too low.

agent added 2 commits 2026-09-10 09:24:21 +02:00

Merge main into #425 rework branch (brings #438 held-peer-mail read) c5fbfdbf4a

fleetd #425 rework round 4: mutation-pinning test for the dropped PlacementDecision ... 4b10d02207

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.

ltms merged commit 1a1e586b62 into main 2026-09-10 09:35:02 +02:00

ltms referenced this pull request 2026-09-10 09:35:37 +02:00

PlacementDecision's javadoc claims the place()-to-spawn() window is closed, but no test pins it — and PeerLauncher's default spawn(req, decision) reopens it #444

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

blocked

Cannot start until something else lands. The body says what.

needs-live-proof

Merged and green, but never shown working on the running daemon. Not the same as done.

ready-to-delegate

Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.

silent-default

A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent kevin ltms

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#433