fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #444: test that pins the place()-to-spawn() window PlacementDecision closes #447

Merged
ltms merged 2 commits from worker/444-placement-window-feb56a-2 into main 2026-09-10 12:00:59 +02:00
Conversation 0 Commits 2 Files Changed 2
+75 -1
agent commented 2026-09-10 11:42:25 +02:00
Member
Copy Link
Copy Source

fleetd #444 — pin the place()-to-spawn() window PlacementDecision exists to close

What was missing

PlacementDecision's whole point is that a profile chosen by place() is the profile actually
spawned on, even if fleet state changes in between. CompositePeerLauncher.spawn(req, decision)
routes decision.profile() straight to its owning adapter and never re-runs the explicit-profile
enforce* checks (enforceNotQuarantined, enforceNotCoolingOff, enforceMaxLoad,
enforceModelEnabled). The alternative body — re-entering spawn(req.withProfile(...)) — lands in
the throwing explicit-profile branch instead.

Nothing in the suite distinguished the two, because after fleetd #435 every enforce* check tests a
condition place()'s own routing already filtered out — the two paths only disagree when placement
state moves between the place() call and the spawn() call, and no existing test moved that
state.

What this PR adds

One test in CompositePeerLauncherTest:
spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace

  1. Calls composite.place(MemberRole.DEV) while nothing is quarantined — sol (definition-order
    first, fixed policy) is chosen.
  2. Quarantines sol's credential (shared-openai) — moving placement state in the window the
    ticket names.
  3. Calls composite.spawn(req, decision) against the held decision and asserts it still
    succeeds, lands on sol (handle.profile()), and that the request that reached the delegate
    actually carried sol (adapter.spawnCount("sol") == 1, adapter.spawnCount("b") == 0).

Also updated PeerLauncher's default spawn(req, decision) javadoc: a launcher that routes across
more than one profile must override this method, naming the four enforce* checks the default's
re-entry into the single-argument spawn would re-apply. The default itself is unchanged — kept per
the ticket's own decision, since HerdrPeerLauncher (single-profile) is correctly served by it.

Round 2 — fixture fix (review finding)

Reviewer found the fixture's own weakness: StubLauncher was built with "sol" as its own
fallback default (new StubLauncher("claude", herdr, profiles, "sol", Set.of())). StubLauncher.spawn
falls back to that field whenever req.profileName() is blank:

String p = (req.profileName() == null || req.profileName().isBlank())
        ? defaultProfile() : req.profileName();

Since that fallback was also "sol" — the same profile place() decides — an unstamped request
(one routed but never actually given req.withProfile("sol")) would land on spawnCount("sol") == 1
by coincidence. Reviewer proved it: dropping the stamping (SpawnRequest routedReq = req;) while
keeping the routing left the test green.

Fix: the adapter's own fallback default is now "b" instead of "sol", so an unstamped request
counts against "b", not "sol" — only an actually-stamped request can produce
spawnCount("sol") == 1 now. Confirmed place(MemberRole.DEV) still resolves "sol" after this
change (composite's own poolFor/candidates never reads the adapter's internal default; that
field only feeds StubLauncher.spawn's own fallback).

Evidence

Test alone, unmutated — mvn test -Dtest=CompositePeerLauncherTest#spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace:

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

Mutation 2 (this round) — drop the stamping, keep the routing — CompositePeerLauncher's 2-arg
spawn:

HerdrPeerLauncher d = route(decision.profile());
SpawnRequest routedReq = req;   // was: req.withProfile(decision.profile())
PeerHandle handle = d.spawn(routedReq);

Test alone under this mutation:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[ERROR] CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace:1173
  the decision from place() is honored even though sol is now quarantined ==> expected: <sol> but was: <b>
[INFO] BUILD FAILURE

Reverted; confirmed the diff on CompositePeerLauncher.java was empty again (git diff --stat —
no output) before moving to mutation 1.

Mutation 1 (M1, from round 1) — re-enter the explicit-profile branch:

public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) {
    return spawn(req.withProfile(decision.profile()));
}

Test alone under this mutation:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0
[ERROR] CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace:1171
  » Placement worker profile 'sol' is quarantined (credential 'shared-openai' exhausted; ~1800s remaining) — refusing spawn
[INFO] BUILD FAILURE

Both mutations kill the same test — no trade-off between them. Reverted; src/main diff is empty
again (git diff --stat -- src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java — no
output, confirming no net change to src/main).

Full build — mvn clean install, full unpiped log, no -q:

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

(CompositePeerLauncherTest: Tests run: 76, Failures: 0, Errors: 0.)

Scope note

This round touches only CompositePeerLauncherTest.java. src/main is untouched (verified via
git diff --stat) — PeerLauncher.java's javadoc from round 1 stands as-is, per the reviewer's
instruction not to change it.

## fleetd #444 — pin the `place()`-to-`spawn()` window `PlacementDecision` exists to close ### What was missing `PlacementDecision`'s whole point is that a profile chosen by `place()` is the profile actually spawned on, even if fleet state changes in between. `CompositePeerLauncher.spawn(req, decision)` routes `decision.profile()` straight to its owning adapter and never re-runs the explicit-profile `enforce*` checks (`enforceNotQuarantined`, `enforceNotCoolingOff`, `enforceMaxLoad`, `enforceModelEnabled`). The alternative body — re-entering `spawn(req.withProfile(...))` — lands in the throwing explicit-profile branch instead. Nothing in the suite distinguished the two, because after fleetd #435 every `enforce*` check tests a condition `place()`'s own routing already filtered out — the two paths only disagree when placement state moves **between** the `place()` call and the `spawn()` call, and no existing test moved that state. ### What this PR adds One test in `CompositePeerLauncherTest`: `spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace` 1. Calls `composite.place(MemberRole.DEV)` while nothing is quarantined — `sol` (definition-order first, `fixed` policy) is chosen. 2. Quarantines `sol`'s credential (`shared-openai`) — moving placement state in the window the ticket names. 3. Calls `composite.spawn(req, decision)` against the **held** decision and asserts it still succeeds, lands on `sol` (`handle.profile()`), and that the request that reached the delegate actually carried `sol` (`adapter.spawnCount("sol") == 1`, `adapter.spawnCount("b") == 0`). Also updated `PeerLauncher`'s default `spawn(req, decision)` javadoc: a launcher that routes across more than one profile **must** override this method, naming the four `enforce*` checks the default's re-entry into the single-argument `spawn` would re-apply. The default itself is unchanged — kept per the ticket's own decision, since `HerdrPeerLauncher` (single-profile) is correctly served by it. ### Round 2 — fixture fix (review finding) Reviewer found the fixture's own weakness: `StubLauncher` was built with `"sol"` as its **own** fallback default (`new StubLauncher("claude", herdr, profiles, "sol", Set.of())`). `StubLauncher.spawn` falls back to that field whenever `req.profileName()` is blank: ```java String p = (req.profileName() == null || req.profileName().isBlank()) ? defaultProfile() : req.profileName(); ``` Since that fallback was also `"sol"` — the same profile `place()` decides — an **unstamped** request (one routed but never actually given `req.withProfile("sol")`) would land on `spawnCount("sol") == 1` by coincidence. Reviewer proved it: dropping the stamping (`SpawnRequest routedReq = req;`) while keeping the routing left the test green. Fix: the adapter's own fallback default is now `"b"` instead of `"sol"`, so an unstamped request counts against `"b"`, not `"sol"` — only an actually-stamped request can produce `spawnCount("sol") == 1` now. Confirmed `place(MemberRole.DEV)` still resolves `"sol"` after this change (composite's own `poolFor`/`candidates` never reads the adapter's internal default; that field only feeds `StubLauncher.spawn`'s own fallback). ### Evidence **Test alone, unmutated** — `mvn test -Dtest=CompositePeerLauncherTest#spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace`: ``` [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` **Mutation 2 (this round) — drop the stamping, keep the routing** — `CompositePeerLauncher`'s 2-arg `spawn`: ```java HerdrPeerLauncher d = route(decision.profile()); SpawnRequest routedReq = req; // was: req.withProfile(decision.profile()) PeerHandle handle = d.spawn(routedReq); ``` Test alone under this mutation: ``` [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 [ERROR] CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace:1173 the decision from place() is honored even though sol is now quarantined ==> expected: <sol> but was: <b> [INFO] BUILD FAILURE ``` Reverted; confirmed the diff on `CompositePeerLauncher.java` was empty again (`git diff --stat` — no output) before moving to mutation 1. **Mutation 1 (M1, from round 1) — re-enter the explicit-profile branch**: ```java public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { return spawn(req.withProfile(decision.profile())); } ``` Test alone under this mutation: ``` [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0 [ERROR] CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace:1171 » Placement worker profile 'sol' is quarantined (credential 'shared-openai' exhausted; ~1800s remaining) — refusing spawn [INFO] BUILD FAILURE ``` Both mutations kill the same test — no trade-off between them. Reverted; `src/main` diff is empty again (`git diff --stat -- src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java` — no output, confirming no net change to `src/main`). **Full build** — `mvn clean install`, full unpiped log, no `-q`: ``` [INFO] Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` (`CompositePeerLauncherTest`: `Tests run: 76, Failures: 0, Errors: 0`.) ### Scope note This round touches only `CompositePeerLauncherTest.java`. `src/main` is untouched (verified via `git diff --stat`) — `PeerLauncher.java`'s javadoc from round 1 stands as-is, per the reviewer's instruction not to change it.
agent added 1 commit 2026-09-10 11:42:26 +02:00
fleetd #444: pin the place()-to-spawn() window PlacementDecision closes
CI / contract (pull_request) Successful in 1m4s
Details
CI / build (pull_request) Successful in 1m53s
Details
3f036b2a62 
Add a test that resolves place(role) while nothing is quarantined,
then quarantines the resolved profile's credential BEFORE spawning
against the held PlacementDecision. CompositePeerLauncher.spawn(req,
decision) must still honor the decision and land on the quarantined
profile, since it never re-runs the explicit-profile enforce* checks.

Verified the test kills the regression: with the override's body
replaced by the re-entering spawn(req.withProfile(...)) form, this
exact test goes RED with a PlacementException naming the now-
quarantined profile; restored, the full suite is green (1575 tests).

Also documents on PeerLauncher's default spawn(req, decision) that a
launcher routing across more than one profile MUST override it,
naming the four enforce* checks the default's re-entry re-applies.
agent added 1 commit 2026-09-10 11:50:46 +02:00
fleetd #444: separate the adapter's fallback default from the decided profile
CI / contract (pull_request) Successful in 46s
Details
CI / build (pull_request) Successful in 1m56s
Details
e4c703a51a 
Review found the fixture's StubLauncher fell back to 'sol' too — the
same profile place() decides — so an UNSTAMPED request could land on
spawnCount('sol') by coincidence, and the assertion's claim that the
request 'actually carried sol' was unproven. Dropping the stamping
(SpawnRequest routedReq = req) while keeping the routing survived the
test unchanged.

Fix: give the adapter 'b' as its own fallback default instead, so an
unstamped request counts against 'b', not 'sol'. Verified both
mutations against the single test in isolation:
  - drop-stamping (routedReq = req): RED, expected <sol> but was <b>
  - re-entering (return spawn(req.withProfile(decision.profile())))
    i.e. M1 from the first round: still RED, PlacementException
    naming the now-quarantined 'sol'
Restored both; full suite green at 1575 tests.

No changes to src/main — PeerLauncher's javadoc from the first round
is unchanged.
ltms merged commit 822327eed5 into main 2026-09-10 12:00:59 +02:00
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#447
Delete Branch "worker/444-placement-window-feb56a-2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?