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

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

New Issue
Closed
opened 2026-09-10 09:35:37 +02:00 by ltms · 2 comments
ltms commented 2026-09-10 09:35:37 +02:00
Owner
Copy Link
Copy Source

Found while verifying PR #433 (fleetd #425) before merging. Not a defect in that work — #433 is merged and its own fix is properly pinned. This is a claim in the code that nothing holds down.

The surviving mutation

In the merged tree, I replaced CompositePeerLauncher.spawn(SpawnRequest, PlacementDecision)'s body with a delegation to the explicit-profile branch:

public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) {
    return spawn(req.withProfile(decision.profile()));   // the original #425 defect
}

That is the exact bug fleetd #425 exists to remove. It survived SessionManagerTest,
CompositePeerLauncherTest and FleetProfilesLiveDefaultTest (control green, tree restored
clean afterwards).

For contrast, in the same battery:

  • reverting SessionManager:677 to drop the decision → KILLED by the new round-4 test
  • dropping the withProfile stamping in the 2-arg spawn → KILLED by 3 tests

So the caller side is pinned and the routing side is pinned. What is not pinned is that the
2-arg spawn must not re-run the checks placement already made.

Why it survived, and why that is not alarming

It is a near-equivalent mutant under current state. PlacementDecision's own javadoc
records that fleetd #435 closed the maxLoad case:

fleetd #435 closed that: fixed now evaluates maxLoad exactly like every other placement
policy … so a PlacementDecision can no longer be at-cap in the first place

Post-#435 all four enforce* checks — enforceNotQuarantined, enforceNotCoolingOff,
enforceMaxLoad, enforceModelEnabled — test conditions the routing branch has already
filtered on. A profile the routing branch selected is in none of those sets, so re-entering the
refusing branch with it succeeds too. The two paths give the same answer.

They diverge in exactly one situation, and the javadoc names it:

neither one re-opens the window between the placement decision and the spawn in which the
underlying state could otherwise move

That is the claim. If state moves after place() returns and before the spawn — a credential
quarantines, a profile starts cooling off, a model is turned off through the hot models: block
— then the routing path spawns as decided while the refusing path throws. No test moves state
between those two calls. Measured: no test calls place() at all.

So the javadoc asserts a property, and the property is currently true only because nobody has
re-broken it.

The second half: the interface default already has this shape

PeerLauncher's default is the mutation, shipped:

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

Production is safe today. Fleetd.java:228 builds workers as a CompositePeerLauncher,
which overrides it, and SessionManager is constructed with that at Fleetd.java:267.
HerdrPeerLauncher is the only other src/main implementer and does not override it — but
it does no placement filtering, so for it the default is correct rather than merely harmless.

The risk is future-shaped: SessionManager holds the interface type (SessionManager.java:48),
so any new PeerLauncher that does placement and forgets to override this method gets the
original #425 defect back, silently, with the whole suite green. This repo has a note about
exactly this shape — a filter in a shared default is only applied by implementers that override
it, so the default implementation is the one that needs the test.

Acceptance criteria

  1. A test moves placement state between place() and spawn(req, decision) — quarantine a
    credential, start a cooling-off, or turn a model off — and asserts the spawn still lands on
    the profile the decision named, rather than throwing.

  2. The mutation that must fail: replacing CompositePeerLauncher.spawn(SpawnRequest, PlacementDecision)'s body with return spawn(req.withProfile(decision.profile())); must go
    red. Paste the red run with the failing method names and the restored green run.

  3. Decide and record what the default implementation should be, in one of two ways:

    • keep it and add a test proving a default-implementing launcher behaves acceptably, or
    • make it abstract so a new placement-doing launcher cannot silently inherit the defect,
      and override it explicitly in HerdrPeerLauncher with a comment saying why the
      re-entering form is correct there (no placement filtering to preserve).

    State which you chose and why. Option 2 turns a silent future regression into a compile
    error, which this repo generally prefers, but it is a wider change; I am not forcing it.

  4. If the javadoc's "window" claim cannot be pinned by a test, weaken the javadoc to say what
    is actually guaranteed. A sentence that no test holds down is worse than a weaker true one.

Out of scope

  • Do not change enforceNotQuarantined, enforceNotCoolingOff, enforceMaxLoad or
    enforceModelEnabled, and do not change the routing branch's fall-through behaviour. The
    refuse-vs-fall-through difference is deliberate and documented.
  • Do not revisit fleetd #425 or #435. Both are correct and merged.
Found while verifying PR #433 (fleetd #425) before merging. **Not a defect in that work** — #433 is merged and its own fix is properly pinned. This is a claim in the code that nothing holds down. ## The surviving mutation In the merged tree, I replaced `CompositePeerLauncher.spawn(SpawnRequest, PlacementDecision)`'s body with a delegation to the explicit-profile branch: ```java public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { return spawn(req.withProfile(decision.profile())); // the original #425 defect } ``` That is the exact bug fleetd #425 exists to remove. It **survived** `SessionManagerTest`, `CompositePeerLauncherTest` and `FleetProfilesLiveDefaultTest` (control green, tree restored clean afterwards). For contrast, in the same battery: - reverting `SessionManager:677` to drop the decision → **KILLED** by the new round-4 test - dropping the `withProfile` stamping in the 2-arg spawn → **KILLED** by 3 tests So the caller side is pinned and the routing side is pinned. What is not pinned is that the 2-arg spawn must not re-run the checks placement already made. ## Why it survived, and why that is not alarming It is a **near-equivalent mutant** under current state. `PlacementDecision`'s own javadoc records that fleetd #435 closed the `maxLoad` case: > fleetd #435 closed that: `fixed` now evaluates `maxLoad` exactly like every other placement > policy … so a `PlacementDecision` can no longer be at-cap in the first place Post-#435 all four `enforce*` checks — `enforceNotQuarantined`, `enforceNotCoolingOff`, `enforceMaxLoad`, `enforceModelEnabled` — test conditions the routing branch has *already* filtered on. A profile the routing branch selected is in none of those sets, so re-entering the refusing branch with it succeeds too. The two paths give the same answer. They diverge in exactly one situation, and the javadoc names it: > neither one re-opens the window between the placement decision and the spawn in which the > underlying state could otherwise move That is the claim. If state moves after `place()` returns and before the spawn — a credential quarantines, a profile starts cooling off, a model is turned off through the hot `models:` block — then the routing path spawns as decided while the refusing path throws. **No test moves state between those two calls.** Measured: no test calls `place()` at all. So the javadoc asserts a property, and the property is currently true only because nobody has re-broken it. ## The second half: the interface default already has this shape `PeerLauncher`'s default is the mutation, shipped: ```java default PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { return spawn(req.withProfile(decision.profile())); } ``` Production is safe today. `Fleetd.java:228` builds `workers` as a `CompositePeerLauncher`, which overrides it, and `SessionManager` is constructed with that at `Fleetd.java:267`. `HerdrPeerLauncher` is the only other `src/main` implementer and does **not** override it — but it does no placement filtering, so for it the default is correct rather than merely harmless. The risk is future-shaped: `SessionManager` holds the interface type (`SessionManager.java:48`), so any new `PeerLauncher` that does placement and forgets to override this method gets the original #425 defect back, silently, with the whole suite green. This repo has a note about exactly this shape — a filter in a shared default is only applied by implementers that override it, so the **default** implementation is the one that needs the test. ## Acceptance criteria 1. A test moves placement state between `place()` and `spawn(req, decision)` — quarantine a credential, start a cooling-off, or turn a model off — and asserts the spawn still lands on the profile the decision named, rather than throwing. 2. **The mutation that must fail:** replacing `CompositePeerLauncher.spawn(SpawnRequest, PlacementDecision)`'s body with `return spawn(req.withProfile(decision.profile()));` must go red. Paste the red run with the failing method names and the restored green run. 3. Decide and record what the **default** implementation should be, in one of two ways: - keep it and add a test proving a default-implementing launcher behaves acceptably, or - make it abstract so a new placement-doing launcher cannot silently inherit the defect, and override it explicitly in `HerdrPeerLauncher` with a comment saying why the re-entering form is correct there (no placement filtering to preserve). State which you chose and why. Option 2 turns a silent future regression into a compile error, which this repo generally prefers, but it is a wider change; I am not forcing it. 4. If the javadoc's "window" claim cannot be pinned by a test, weaken the javadoc to say what is actually guaranteed. A sentence that no test holds down is worse than a weaker true one. ## Out of scope - Do not change `enforceNotQuarantined`, `enforceNotCoolingOff`, `enforceMaxLoad` or `enforceModelEnabled`, and do not change the routing branch's fall-through behaviour. The refuse-vs-fall-through difference is deliberate and documented. - Do not revisit fleetd #425 or #435. Both are correct and merged.
ltms commented 2026-09-10 12:01:29 +02:00
Author
Owner
Copy Link
Copy Source

Closed by PR #447, merged as 822327e. The merged tree is byte-identical to the branch tip I tested (git diff --stat origin/main e4c703a printed nothing).

What landed

One test, CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace. It calls place(DEV), quarantines the chosen profile's credential, and then calls spawn(req, decision) against the held decision. src/main is javadoc-only: 12 added lines, 0 added code lines, measured by filtering the main-side diff.

My own verification, on the tree that landed

FULL BUILD  Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0  BUILD SUCCESS
            compile errors: 0
CONTROL     CompositePeerLauncherTest  Tests run: 76, Failures: 0  -> GREEN

M1  the 2-arg spawn re-enters the 1-arg spawn (the inherited default)
    -> KILLED, Errors: 1
M2  drop the stamping: route to the decided profile but do not carry it
    -> KILLED, Failures: 1

Both mutations are killed by the same test method. Each was proved applied by printing the mutated method, and the tree was clean again after each.

The part worth remembering

Round 1 of the PR had a weak fixture, and only mutation found it. StubLauncher fell back to "sol" as its own default — the same profile place() decides. StubLauncher.spawn uses that field whenever req.profileName() is blank. So a request that was routed but never stamped still counted against spawnCount("sol"), by coincidence. M2 survived, and the assertion's claim that "the request actually carried sol" was not proven by anything.

e4c703a gives the adapter "b" as its fallback instead. Now an unstamped request counts against "b" and the test fails. One fixture kills both mutations.

The general shape: a fixture's own default can equal the value under test, and then the assertion passes for the wrong reason. The test looked correct and read correctly. Only changing the code it was meant to protect showed that it was not protecting it.

Closed by PR #447, merged as `822327e`. The merged tree is byte-identical to the branch tip I tested (`git diff --stat origin/main e4c703a` printed nothing). ## What landed One test, `CompositePeerLauncherTest.spawnHonorsAPlacementDecisionEvenAfterItsProfileIsQuarantinedInTheWindowAfterPlace`. It calls `place(DEV)`, quarantines the chosen profile's credential, and then calls `spawn(req, decision)` against the held decision. `src/main` is javadoc-only: 12 added lines, 0 added code lines, measured by filtering the main-side diff. ## My own verification, on the tree that landed ``` FULL BUILD Tests run: 1575, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS compile errors: 0 CONTROL CompositePeerLauncherTest Tests run: 76, Failures: 0 -> GREEN M1 the 2-arg spawn re-enters the 1-arg spawn (the inherited default) -> KILLED, Errors: 1 M2 drop the stamping: route to the decided profile but do not carry it -> KILLED, Failures: 1 ``` Both mutations are killed by the same test method. Each was proved applied by printing the mutated method, and the tree was clean again after each. ## The part worth remembering Round 1 of the PR had a weak fixture, and only mutation found it. `StubLauncher` fell back to `"sol"` as its **own** default — the same profile `place()` decides. `StubLauncher.spawn` uses that field whenever `req.profileName()` is blank. So a request that was routed but never stamped still counted against `spawnCount("sol")`, by coincidence. M2 survived, and the assertion's claim that "the request actually carried sol" was not proven by anything. `e4c703a` gives the adapter `"b"` as its fallback instead. Now an unstamped request counts against `"b"` and the test fails. One fixture kills both mutations. **The general shape: a fixture's own default can equal the value under test, and then the assertion passes for the wrong reason.** The test looked correct and read correctly. Only changing the code it was meant to protect showed that it was not protecting it.
ltms closed this issue 2026-09-10 12:01:35 +02:00
ltms commented 2026-09-10 12:03:02 +02:00
Author
Owner
Copy Link
Copy Source

One honest note on closing this. Criterion 3 asked to either keep the default and test it, or make it abstract. #447 kept it and explained why, which is half of option 1. The test half was not done, and measured on 822327e the inherited default runs in no test: one class overrides the 2-argument spawn, four inherit it, and the only test that calls it calls it on the override.

That gap is now #450, not left inside this closed ticket.

One honest note on closing this. Criterion 3 asked to either keep the default **and test it**, or make it abstract. #447 kept it and explained why, which is half of option 1. The test half was not done, and measured on `822327e` the inherited default runs in no test: one class overrides the 2-argument `spawn`, four inherit it, and the only test that calls it calls it on the override. That gap is now **#450**, not left inside this closed ticket.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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#444
Delete Branch "%!s()"

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?