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

LeadRollover's settle poll has no bound of its own, so a regression shows up as a CI hang instead of a red test #486

New Issue
Open
opened 2026-09-11 02:20:12 +02:00 by ltms · 3 comments
ltms commented 2026-09-11 02:20:12 +02:00
Owner
Copy Link
Copy Source

Found while mutation-testing #480 Unit E (PR #484). Not a production defect — a test-diagnosability one.

What happens

dev.ltms.fleet.lead.LeadRollover#waitUntilAtTurnBoundary bounds its poll loop only by the
injected wall clock:

long deadline = nowMillis.getAsLong() + TimeUnit.SECONDS.toMillis(settleSeconds);
while (nowMillis.getAsLong() < deadline) {
    ...
    settleSleeper.run();
}

LeadRolloverTest injects a fixedClock — an AtomicLong that never advances — and a
settleSleeper that does not sleep. Under correct code that is harmless: the pane settles on the
first poll, so the clock is never consulted a second time. Under a regression it is not harmless:
the condition stays true for ever and the loop spins at full CPU.

How I hit it

Mutating PR #484 by dropping the DONE arm, leaving if (status == AgentStatus.IDLE):

PROOF 1 — mutant present:  grep -c 'MUT-DROP-DONE'   -> 1
PROOF 2 — original gone:   grep -c 'IDLE || status'  -> 0

single test, unmutated:  Tests run: 1, Failures: 0 — 0.172 s, BUILD SUCCESS
single test, mutated:    still running after 100 s, killed — did NOT pass

The mutant is detected, which is the point of the test. But it is detected as a hang, not a
failure. A future regression in this class burns a CI runner to its timeout and reads like
infrastructure flake rather than a red test with a name on it.

Why this is not a production bug

In production nowMillis is System::currentTimeMillis, which advances, and settleSleeper is a
real 250ms sleep (SETTLE_POLL_MS). The deadline is reached and the method returns false. Nothing
hangs on the live daemon. Confirmed by reading the production constructor, not inferred from the
tests.

Proposed fix

The Unit E worker's recommendation, which I agree with: bound the loop by poll count as well as by
the clock, inside waitUntilAtTurnBoundary. The two alternatives are weaker:

  • Advancing clock in the test helper patches the one test that happened to trip over it. Any
    future test that injects a fixed clock hits the same hole.
  • A JUnit timeout turns a hang into a slow red test, which is better, but still spins at full CPU
    for the timeout's duration and has to be remembered on every test that reaches this method.

A count bound fixes it at the source and needs no cooperation from callers.

One caveat on implementing it, so this does not become a production behaviour change: the bound
must be generous enough that it can never be reached before the wall-clock deadline on a healthy
daemon. clearSettleSeconds defaults to 20 and SETTLE_POLL_MS is 250, so a correct run is about
80 polls; a sleeper that returns early for any reason would do more. Derive the cap from
settleSeconds / SETTLE_POLL_MS with generous slack rather than picking a constant, and add a test
that a healthy 20s wait is not cut short by the new bound. Otherwise this fix silently shortens the
real settle window, which is exactly the class of change #480 has already had to correct twice.

Scope

LeadRollover.java and LeadRolloverTest.java only. Do not change AgentStatus.

Refs #480, PR #484.

Found while mutation-testing #480 Unit E (PR #484). Not a production defect — a test-diagnosability one. ## What happens `dev.ltms.fleet.lead.LeadRollover#waitUntilAtTurnBoundary` bounds its poll loop **only** by the injected wall clock: ```java long deadline = nowMillis.getAsLong() + TimeUnit.SECONDS.toMillis(settleSeconds); while (nowMillis.getAsLong() < deadline) { ... settleSleeper.run(); } ``` `LeadRolloverTest` injects a `fixedClock` — an `AtomicLong` that never advances — and a `settleSleeper` that does not sleep. Under correct code that is harmless: the pane settles on the first poll, so the clock is never consulted a second time. Under a regression it is not harmless: the condition stays true for ever and the loop spins at full CPU. ## How I hit it Mutating PR #484 by dropping the `DONE` arm, leaving `if (status == AgentStatus.IDLE)`: ``` PROOF 1 — mutant present: grep -c 'MUT-DROP-DONE' -> 1 PROOF 2 — original gone: grep -c 'IDLE || status' -> 0 single test, unmutated: Tests run: 1, Failures: 0 — 0.172 s, BUILD SUCCESS single test, mutated: still running after 100 s, killed — did NOT pass ``` The mutant *is* detected, which is the point of the test. But it is detected as a hang, not a failure. A future regression in this class burns a CI runner to its timeout and reads like infrastructure flake rather than a red test with a name on it. ## Why this is not a production bug In production `nowMillis` is `System::currentTimeMillis`, which advances, and `settleSleeper` is a real 250ms sleep (`SETTLE_POLL_MS`). The deadline is reached and the method returns `false`. Nothing hangs on the live daemon. Confirmed by reading the production constructor, not inferred from the tests. ## Proposed fix The Unit E worker's recommendation, which I agree with: **bound the loop by poll count as well as by the clock**, inside `waitUntilAtTurnBoundary`. The two alternatives are weaker: - *Advancing clock in the test helper* patches the one test that happened to trip over it. Any future test that injects a fixed clock hits the same hole. - *A JUnit timeout* turns a hang into a slow red test, which is better, but still spins at full CPU for the timeout's duration and has to be remembered on every test that reaches this method. A count bound fixes it at the source and needs no cooperation from callers. **One caveat on implementing it**, so this does not become a production behaviour change: the bound must be generous enough that it can never be reached before the wall-clock deadline on a healthy daemon. `clearSettleSeconds` defaults to 20 and `SETTLE_POLL_MS` is 250, so a correct run is about 80 polls; a sleeper that returns early for any reason would do more. Derive the cap from `settleSeconds / SETTLE_POLL_MS` with generous slack rather than picking a constant, and add a test that a healthy 20s wait is not cut short by the new bound. Otherwise this fix silently shortens the real settle window, which is exactly the class of change #480 has already had to correct twice. ## Scope `LeadRollover.java` and `LeadRolloverTest.java` only. Do not change `AgentStatus`. Refs #480, PR #484.
ltms commented 2026-09-12 02:52:33 +02:00
Author
Owner
Copy Link
Copy Source

This now has a reproduction. Found while mutation-testing #489 / PR #490 on 2026-09-12.

Mutate one character of the FIRST gate in LeadRollover.waitUntilAtTurnBoundary — drop DONE, so it accepts IDLE only:

if (status == AgentStatus.IDLE) {   // was: || status == AgentStatus.DONE
    return true;
}

Then run mvn test -Dtest=LeadRolloverTest. It does not go red. It hangs, spinning at full CPU, with no timeout and no failure. I killed it after 2 minutes 19 seconds of no log output.

The hanging test is doneStatusStillCompletesTheFullRoll (LeadRolloverTest:333). The mechanism is exactly what this ticket describes, and it is a property of the test seams:

  • newRollover passes settleSleeper as () -> { }, a no-op;
  • most tests pass nowMillis as fixedClock(clock), which is millis::get — a clock that never advances.

So while (nowMillis.getAsLong() < deadline) can never terminate. The settleSeconds bound is real in production, where nowMillis is System::currentTimeMillis, but in the suite it is inert. Any status that never reaches a boundary spins forever.

This is not specific to the first gate. PR #490 adds waitForClearPickupAndSettle with the same loop shape, so the surface doubles rather than shrinks. It is not a regression and I am not holding #490 for it — #490 fixes a live defect and its own tests pass — but the hazard is now two methods wide.

Two things any fix should cover, and the second is the one that bites:

  1. A poll-count cap derived from settleSeconds / SETTLE_POLL_MS with slack, as this ticket already says, so a wrong status ends the loop.
  2. The test seams themselves. A cap alone still leaves fixedClock + a no-op sleeper able to run the full cap at full speed rather than timing out honestly. Consider making the default test sleeper advance the fake clock by SETTLE_POLL_MS, so the injected clock and the injected sleeper agree about time passing.

Whoever takes this: the mutation above is a ready-made red-then-green check. With the fix in place, that same mutation must produce a failing test, not a hang.

**This now has a reproduction.** Found while mutation-testing #489 / PR #490 on 2026-09-12. Mutate one character of the FIRST gate in `LeadRollover.waitUntilAtTurnBoundary` — drop `DONE`, so it accepts `IDLE` only: ```java if (status == AgentStatus.IDLE) { // was: || status == AgentStatus.DONE return true; } ``` Then run `mvn test -Dtest=LeadRolloverTest`. It does **not** go red. It **hangs**, spinning at full CPU, with no timeout and no failure. I killed it after 2 minutes 19 seconds of no log output. The hanging test is `doneStatusStillCompletesTheFullRoll` (`LeadRolloverTest:333`). The mechanism is exactly what this ticket describes, and it is a property of the test seams: - `newRollover` passes `settleSleeper` as `() -> { }`, a no-op; - most tests pass `nowMillis` as `fixedClock(clock)`, which is `millis::get` — a clock that never advances. So `while (nowMillis.getAsLong() < deadline)` can never terminate. The `settleSeconds` bound is real in production, where `nowMillis` is `System::currentTimeMillis`, but in the suite it is inert. Any status that never reaches a boundary spins forever. This is not specific to the first gate. PR #490 adds `waitForClearPickupAndSettle` with the same loop shape, so the surface doubles rather than shrinks. It is not a regression and I am not holding #490 for it — #490 fixes a live defect and its own tests pass — but the hazard is now two methods wide. Two things any fix should cover, and the second is the one that bites: 1. A poll-count cap derived from `settleSeconds / SETTLE_POLL_MS` with slack, as this ticket already says, so a wrong status ends the loop. 2. The test seams themselves. A cap alone still leaves `fixedClock` + a no-op sleeper able to run the full cap at full speed rather than timing out honestly. Consider making the default test sleeper advance the fake clock by `SETTLE_POLL_MS`, so the injected clock and the injected sleeper agree about time passing. Whoever takes this: the mutation above is a ready-made red-then-green check. With the fix in place, that same mutation must produce a **failing** test, not a hang.
ltms commented 2026-09-12 04:58:21 +02:00
Author
Owner
Copy Link
Copy Source

Measured, not read — the suite hangs, and -Dsurefire.timeout does not rescue it

Re-checked on main at 8f59019 (after PR #496 merged, which touched this same loop).

The fixture, counted

LeadRolloverTest.java:

fixedClock(...) uses:   22      <- the clock never advances
clock.addAndGet(...):    9      <- self-advancing, added by #494
CONTROL, @Test count:   33

And the sleeper is still a no-op — LeadRolloverTest.java:85:

return new LeadRollover(agents, () -> config, leadWorkspace, nowMillis, () -> { }, Runnable::run);

So for 22 of the 33 tests, while (nowMillis.getAsLong() < deadline) is true forever and settleSleeper.run() costs nothing. The only way out of either wait is a status-based return.

The mutation

LeadRollover.waitUntilAtTurnBoundary — make the boundary never be observed:

if (status == AgentStatus.IDLE || status == AgentStatus.DONE) {   // original
if (false) { // MUTANT486 never a boundary                        // mutant

Proved applied with two greps using different strings: grep -n 'MUTANT486' found :508; a scoped awk over that method plus grep -c 'status == AgentStatus.IDLE' returned 0.

The result

mvn -Dtest=LeadRolloverTest -Dsurefire.timeout=90 test
  -> ran for over 300 seconds, then I killed it
  -> "Tests run:" lines produced: 0
  -> two java processes (maven + the surefire fork) alive at 5 minutes

The suite produced no test result at all. It did not go red, it did not report a timeout, it did not name a method. It spun. -Dsurefire.timeout=90 was ignored — the fork was still alive well past 90 seconds.

Restored, shasum -a 256 byte-identical to the pristine file, control run: Tests run: 33, Failures: 0.

Why this matters more now than when the ticket was filed

PR #496 just rewrote both waits in this class — new return types, a new counter, an extra clock read on the success path. That is exactly the kind of edit that can break a loop's exit condition. Today, that mistake does not produce a red build with a method name on it. It produces a CI job that runs until the runner's own wall-clock limit kills it, with no indication of which test or which loop.

A wrong status in that loop is less visible than a wrong value, which is backwards.

Asked for

  1. Bound the loop by iterations as well as by the clock. The clock bound is the production contract and must stay. Add a hard iteration cap that cannot be defeated by a frozen clock, and make hitting it a loud failure — a thrown IllegalStateException naming the method and the target, not a quiet return false that a test could read as a normal timeout.
  2. The test for it must hang without the fix and fail with it. That is the whole point, so prove it in both directions: apply the mutation above, show the suite hangs on unfixed code (say how long you waited and that zero results were produced), then apply the fix and show the same mutation produces a named failure in seconds.
  3. Do not "fix" this by making every test use a self-advancing clock. That hides the defect instead of closing it — the next test written with a fixed clock reopens it silently, and a fixed clock is legitimate for testing deadline arithmetic. The bound belongs in the production loop.
  4. While you are there, check Injector's poll loops for the same shape and report without fixing — that is a separate ticket.

Related: #494 (merged as 8f59019, rewrote both waits), #490, #489.

## Measured, not read — the suite hangs, and `-Dsurefire.timeout` does not rescue it Re-checked on `main` at `8f59019` (after PR #496 merged, which touched this same loop). ### The fixture, counted `LeadRolloverTest.java`: ``` fixedClock(...) uses: 22 <- the clock never advances clock.addAndGet(...): 9 <- self-advancing, added by #494 CONTROL, @Test count: 33 ``` And the sleeper is still a no-op — `LeadRolloverTest.java:85`: ```java return new LeadRollover(agents, () -> config, leadWorkspace, nowMillis, () -> { }, Runnable::run); ``` So for 22 of the 33 tests, `while (nowMillis.getAsLong() < deadline)` is true forever and `settleSleeper.run()` costs nothing. The only way out of either wait is a status-based `return`. ### The mutation `LeadRollover.waitUntilAtTurnBoundary` — make the boundary never be observed: ```java if (status == AgentStatus.IDLE || status == AgentStatus.DONE) { // original if (false) { // MUTANT486 never a boundary // mutant ``` Proved applied with two greps using different strings: `grep -n 'MUTANT486'` found `:508`; a scoped `awk` over that method plus `grep -c 'status == AgentStatus.IDLE'` returned `0`. ### The result ``` mvn -Dtest=LeadRolloverTest -Dsurefire.timeout=90 test -> ran for over 300 seconds, then I killed it -> "Tests run:" lines produced: 0 -> two java processes (maven + the surefire fork) alive at 5 minutes ``` **The suite produced no test result at all.** It did not go red, it did not report a timeout, it did not name a method. It spun. `-Dsurefire.timeout=90` was ignored — the fork was still alive well past 90 seconds. Restored, `shasum -a 256` byte-identical to the pristine file, control run: `Tests run: 33, Failures: 0`. ### Why this matters more now than when the ticket was filed PR #496 just rewrote both waits in this class — new return types, a new counter, an extra clock read on the success path. That is exactly the kind of edit that can break a loop's exit condition. Today, that mistake does not produce a red build with a method name on it. It produces a CI job that runs until the runner's own wall-clock limit kills it, with no indication of which test or which loop. A wrong status in that loop is **less** visible than a wrong value, which is backwards. ### Asked for 1. **Bound the loop by iterations as well as by the clock.** The clock bound is the production contract and must stay. Add a hard iteration cap that cannot be defeated by a frozen clock, and make hitting it a loud failure — a thrown `IllegalStateException` naming the method and the target, not a quiet `return false` that a test could read as a normal timeout. 2. **The test for it must hang without the fix and fail with it.** That is the whole point, so prove it in both directions: apply the mutation above, show the suite hangs on unfixed code (say how long you waited and that zero results were produced), then apply the fix and show the same mutation produces a named failure in seconds. 3. **Do not "fix" this by making every test use a self-advancing clock.** That hides the defect instead of closing it — the next test written with a fixed clock reopens it silently, and a fixed clock is legitimate for testing deadline arithmetic. The bound belongs in the production loop. 4. While you are there, check `Injector`'s poll loops for the same shape and **report without fixing** — that is a separate ticket. Related: #494 (merged as `8f59019`, rewrote both waits), #490, #489.
ltms commented 2026-09-12 05:01:28 +02:00
Author
Owner
Copy Link
Copy Source

Second instance, in a different class — found by accident, which is the point

While implementing #498 a worker tried to mutate Fleetd.awaitHerdr's interrupt check (if (false && ...)) to prove its test caught it. The test JVM hung rather than failing. They had to force-kill it twice, and reported it honestly as out of scope.

So the same shape is in a second class: a while loop whose only exits are conditional returns, driven by an injected clock that a test can freeze. Break an exit condition and the suite spins instead of going red.

Fleetd.awaitHerdr is now on main at 708f179, with nanos and poller as required parameters — so it is exactly as freezable as LeadRollover's two waits.

One correction, because it changes how severe this is

The worker's report said a real interrupt-detection regression "would also hang the daemon's startup thread forever". It would not. In production nanos is System::nanoTime, so the deadline check at

if (nanos.getAsLong() >= deadline) { return new HerdrAwaitOutcome(DEADLINE_PASSED, ...); }

still fires after HERDR_WAIT_SECONDS. The daemon is safe.

The hazard is entirely a test-fixture one, and that is what this ticket is about: the production loop is bounded by a real clock, the test loop is bounded by nothing, and that asymmetry means a broken exit condition is invisible in CI rather than loud.

What this means for the fix

The iteration cap asked for above should go in both loops in LeadRollover and in Fleetd.awaitHerdr — the same one-line shape, the same loud failure. Two classes is enough to say this is a property of "a poll loop with an injected clock", not a quirk of one file. Any future loop written this way needs it from the start.

Whoever picks this up should treat Fleetd.awaitHerdr as the second acceptance case, with the same both-directions proof: the mutation hangs before the fix, and names a failing test in seconds after it.

## Second instance, in a different class — found by accident, which is the point While implementing #498 a worker tried to mutate `Fleetd.awaitHerdr`'s interrupt check (`if (false && ...)`) to prove its test caught it. The test JVM **hung** rather than failing. They had to force-kill it twice, and reported it honestly as out of scope. So the same shape is in a second class: a `while` loop whose only exits are conditional returns, driven by an injected clock that a test can freeze. Break an exit condition and the suite spins instead of going red. `Fleetd.awaitHerdr` is now on `main` at `708f179`, with `nanos` and `poller` as required parameters — so it is exactly as freezable as `LeadRollover`'s two waits. ### One correction, because it changes how severe this is The worker's report said a real interrupt-detection regression "would also hang the daemon's startup thread forever". **It would not.** In production `nanos` is `System::nanoTime`, so the deadline check at ```java if (nanos.getAsLong() >= deadline) { return new HerdrAwaitOutcome(DEADLINE_PASSED, ...); } ``` still fires after `HERDR_WAIT_SECONDS`. The daemon is safe. **The hazard is entirely a test-fixture one**, and that is what this ticket is about: the production loop is bounded by a real clock, the test loop is bounded by nothing, and that asymmetry means a broken exit condition is invisible in CI rather than loud. ### What this means for the fix The iteration cap asked for above should go in **both** loops in `LeadRollover` **and** in `Fleetd.awaitHerdr` — the same one-line shape, the same loud failure. Two classes is enough to say this is a property of "a poll loop with an injected clock", not a quirk of one file. Any future loop written this way needs it from the start. Whoever picks this up should treat `Fleetd.awaitHerdr` as the second acceptance case, with the same both-directions proof: the mutation hangs before the fix, and names a failing test in seconds after it.
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#486
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?