CompletionResolver: 9 sites maintain the CAS-remove invariant, only 2 are asserted (from the #577 sweep) #581

Closed
opened 2026-09-12 15:05:24 +02:00 by ltms · 3 comments
Owner

Found by the #577 per-site assertion sweep. Both survivors below were proven by mutation, not by reading. Severity 1 of the two findings that sweep returned.

The invariant

When a turn's completion resolves, the code must remove its own entry from inFlight with the compare-and-remove form:

inFlight.remove(target, turn);   // deletes only if the map still holds THIS turn

The class javadoc in CompletionResolver.java names the exact danger: if a later turn has already registered under the same target, a blind inFlight.remove(target) deletes the new turn's entry by mistake. That is the CB-116 bug this code exists to prevent.

The gap

Measured with grep -n "inFlight.remove" in fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java:

  • 9 sites use the safe CAS form: lines 335, 405, 432, 445, 505, 525, 549, 566, 609.
  • 2 sites use the plain 1-arg form (lines 252, 278) inside register()/captureBaseline(), reached only when waiter == null. That is a different case and does not count against this invariant.

Tests in CompletionResolverTest.java that guard "must not evict a successor's registration" — there are exactly 3, and they cover 2 sites:

test (source line) site reached
aSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration (531) 445
aSupersededTurnsEchoedNoReportSubPathMustNotEvictItsSuccessorsRegistration (565) 445 again, different branch
aSupersededTurnsFailMustNotEvictItsSuccessorsRegistration (600) 566

7 of 9 sites have no test that would catch a regression to the unsafe form: 335, 405, 432, 505, 525, 549, 609.

What makes this the sharpest instance of the per-site rule so far: this is not a forgotten invariant. Each covering test's own comment spells out why the CAS form matters ("a one-arg remove(target) here would evict it even though the map no longer holds turnA"). The authors understood the risk precisely and guarded 2 of 9 sites.

Mutation proof (two sites, both survived)

Mutation 1 — line 405, in resolve()'s BACKEND_EXHAUSTED success branch. Anchor inFlight.remove(target, turn); counts 5 with grep -Fxc (shared by 405, 432, 505, 525, 609). Changed to the 1-arg form with a line-anchored sed; recount 5 → 4, so the edit hit exactly one line.

BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0

Mutation 2 — line 549, the early-return guard in fail(). Anchor made unique by its trailing comment, count 1 → 0.

BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0

Both survived. Both restored, sha256 matched pristine 9b5f2f6d810fa45ee838d098256e472cf3af8bd668d20213e1a3dc8ffac8c8be, git status --short clean.

The other two explanations were ruled out, not assumed:

  • Does the line run? Yes. Line 405 runs on any BACKEND_EXHAUSTED completion where rendezvous.resolveExhausted succeeds; existing tests drive that path and still pass after the mutation — which proves the line executes and that nothing checks which turn was removed. Line 549 runs whenever fail() is called with no live waiter, also driven by existing tests.
  • Were the covering tests excluded? No. Both builds were plain mvn -o clean install on the default profile (excludedGroups=contract, pom.xml:264). CompletionResolverTest is not tagged contract, so all 62 of its tests ran in both runs — the 1766 total is identical both times.

Why severity 1

If any of the 7 unguarded lines is "simplified" back to the 1-arg form during a refactor, a live turn's waiter can be evicted by a stale one. The caller then hangs or receives the wrong answer, and no test fails.

The work

Add one assertion per unguarded site: 335, 405, 432, 505, 525, 549, 609. Follow the three existing tests as the model — they are the correct shape, there are just not enough of them.

Acceptance:

  1. Seven new tests, one per site. One combined test is not acceptable: a non-zero total is exactly what hides a gap at a single site.
  2. Each test must be red under its own mutation. For each site, replace inFlight.remove(target, turn) with inFlight.remove(target), show the named failure, restore, and paste the matching sha256. A test that is not red under that exact change is not the test being asked for.
  3. Do not change production code. This is a test-only unit.
Found by the #577 per-site assertion sweep. Both survivors below were proven by mutation, not by reading. Severity 1 of the two findings that sweep returned. ## The invariant When a turn's completion resolves, the code must remove **its own** entry from `inFlight` with the compare-and-remove form: ```java inFlight.remove(target, turn); // deletes only if the map still holds THIS turn ``` The class javadoc in `CompletionResolver.java` names the exact danger: if a later turn has already registered under the same target, a blind `inFlight.remove(target)` deletes the **new** turn's entry by mistake. That is the CB-116 bug this code exists to prevent. ## The gap Measured with `grep -n "inFlight.remove"` in `fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java`: - **9 sites use the safe CAS form**: lines 335, 405, 432, 445, 505, 525, 549, 566, 609. - 2 sites use the plain 1-arg form (lines 252, 278) inside `register()`/`captureBaseline()`, reached only when `waiter == null`. That is a different case and does **not** count against this invariant. Tests in `CompletionResolverTest.java` that guard "must not evict a successor's registration" — there are exactly **3**, and they cover **2** sites: | test (source line) | site reached | |---|---| | `aSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration` (531) | 445 | | `aSupersededTurnsEchoedNoReportSubPathMustNotEvictItsSuccessorsRegistration` (565) | 445 again, different branch | | `aSupersededTurnsFailMustNotEvictItsSuccessorsRegistration` (600) | 566 | **7 of 9 sites have no test that would catch a regression to the unsafe form**: 335, 405, 432, 505, 525, 549, 609. What makes this the sharpest instance of the per-site rule so far: this is not a forgotten invariant. Each covering test's own comment spells out why the CAS form matters ("a one-arg `remove(target)` here would evict it even though the map no longer holds turnA"). **The authors understood the risk precisely and guarded 2 of 9 sites.** ## Mutation proof (two sites, both survived) **Mutation 1 — line 405**, in `resolve()`'s `BACKEND_EXHAUSTED` success branch. Anchor ` inFlight.remove(target, turn);` counts 5 with `grep -Fxc` (shared by 405, 432, 505, 525, 609). Changed to the 1-arg form with a line-anchored `sed`; recount 5 → 4, so the edit hit exactly one line. ``` BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0 ``` **Mutation 2 — line 549**, the early-return guard in `fail()`. Anchor made unique by its trailing comment, count 1 → 0. ``` BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0 ``` Both survived. Both restored, `sha256` matched pristine `9b5f2f6d810fa45ee838d098256e472cf3af8bd668d20213e1a3dc8ffac8c8be`, `git status --short` clean. **The other two explanations were ruled out, not assumed:** - *Does the line run?* Yes. Line 405 runs on any `BACKEND_EXHAUSTED` completion where `rendezvous.resolveExhausted` succeeds; existing tests drive that path and still pass after the mutation — which proves the line executes and that nothing checks **which** turn was removed. Line 549 runs whenever `fail()` is called with no live waiter, also driven by existing tests. - *Were the covering tests excluded?* No. Both builds were plain `mvn -o clean install` on the default profile (`excludedGroups=contract`, `pom.xml:264`). `CompletionResolverTest` is not tagged `contract`, so all 62 of its tests ran in both runs — the 1766 total is identical both times. ## Why severity 1 If any of the 7 unguarded lines is "simplified" back to the 1-arg form during a refactor, a live turn's waiter can be evicted by a stale one. The caller then hangs or receives the wrong answer, and **no test fails**. ## The work Add one assertion per unguarded site: 335, 405, 432, 505, 525, 549, 609. Follow the three existing tests as the model — they are the correct shape, there are just not enough of them. Acceptance: 1. **Seven new tests, one per site.** One combined test is not acceptable: a non-zero total is exactly what hides a gap at a single site. 2. **Each test must be red under its own mutation.** For each site, replace `inFlight.remove(target, turn)` with `inFlight.remove(target)`, show the named failure, restore, and paste the matching `sha256`. A test that is not red under that exact change is not the test being asked for. 3. Do not change production code. This is a test-only unit.
Author
Owner

NOTE — an alternative fix was proposed and measured. The brief is UNCHANGED; here is why, so nobody re-opens it.

The fleet01 lead proposed a better-looking remedy than seven tests: extract one private removeIfCurrent(target, turn), so the raw 1-arg call appears at no business site and one test guards all nine forever. Their argument is sound in general — a per-site rule should find the defect, not dictate a per-site remedy, and seven near-identical tests is the kind of ceremony someone deletes in two years because they cannot tell which one is load-bearing.

I measured it before acting, and it does not hold here. Three findings:

1. A private helper does not make the unsafe form unrepresentable

inFlight is a private final ConcurrentHashMap declared at CompletionResolver.java:145, with 19 raw map calls inside the class. A private helper sits beside that field, not in front of it. Any future author working in the same class can still write inFlight.remove(target), because the map is right there.

So the helper would reduce how many sites you can see, not how many exist — which is precisely the warning already written into #577's own ticket body: "extracting the duplicated lines into one method does not reduce the number of call sites that must maintain the invariant; it only reduces how many of them you can see."

2. Real encapsulation is possible — and still would not enforce it

inFlight is confined to this class (the only two other mentions, Injector.java:643 and :715, are comments). So the map could be wrapped in a small type that exposes only named operations.

It still would not work, because the unsafe form is a LEGITIMATE operation at two sites. register() at :252 and captureBaseline() at :278 use the plain 1-arg inFlight.remove(target) correctly — they run when waiter == null, which is a different case.

So the wrapper would have to expose both removeIfCurrent(target, turn) and removeAny(target). The choice between them then exists at every single site, exactly as it does today. The best available outcome is two better-named methods — a readability win, not an enforcement mechanism. "Make the unsafe form unrepresentable" is not reachable when the unsafe form is somebody's correct answer.

3. The nine sites are nine branches, not nine copies

They sit in three methods:

method sites
resolve(String, InFlight) 335, 405, 432, 445
noReportMessage(String) 505, 525
fail(String, InFlight, String) 549, 566, 609

Each is a distinct exit path needing its own arrangement to reach. They will look similar in their assertion and differ in their setup. That is not boilerplate; that is nine reachability scenarios, and the two already-covered sites (445, 566) are covered by three tests written exactly that way.

Conclusion

The brief stands: one test per unguarded site, each red under its own mutation. Tests remain the only thing that can enforce this invariant, because the type system cannot be made to.

Two refinements worth taking from the proposal, neither of which changes the acceptance:

  • Naming is still worth it. If you find the seven tests genuinely repetitive within a method, factor the arrangement into a helper in the test class. Do not factor the assertion — each test must still fail for its own site.
  • Say in the PR which site each test pins, by line and by branch. That is what stops a future reader deleting one because it "looks like the others" — the objection the proposal correctly raised.
## NOTE — an alternative fix was proposed and measured. The brief is UNCHANGED; here is why, so nobody re-opens it. The fleet01 lead proposed a better-looking remedy than seven tests: extract one private `removeIfCurrent(target, turn)`, so the raw 1-arg call appears at no business site and **one** test guards all nine forever. Their argument is sound in general — a per-site rule should find the defect, not dictate a per-site remedy, and seven near-identical tests is the kind of ceremony someone deletes in two years because they cannot tell which one is load-bearing. I measured it before acting, and it does not hold **here**. Three findings: ### 1. A private helper does not make the unsafe form unrepresentable `inFlight` is a `private final ConcurrentHashMap` declared at `CompletionResolver.java:145`, with **19 raw map calls inside the class**. A private helper sits *beside* that field, not in front of it. Any future author working in the same class can still write `inFlight.remove(target)`, because the map is right there. So the helper would reduce **how many sites you can see**, not how many exist — which is precisely the warning already written into #577's own ticket body: *"extracting the duplicated lines into one method does not reduce the number of call sites that must maintain the invariant; it only reduces how many of them you can see."* ### 2. Real encapsulation is possible — and still would not enforce it `inFlight` is confined to this class (the only two other mentions, `Injector.java:643` and `:715`, are comments). So the map *could* be wrapped in a small type that exposes only named operations. **It still would not work, because the unsafe form is a LEGITIMATE operation at two sites.** `register()` at `:252` and `captureBaseline()` at `:278` use the plain 1-arg `inFlight.remove(target)` **correctly** — they run when `waiter == null`, which is a different case. So the wrapper would have to expose both `removeIfCurrent(target, turn)` and `removeAny(target)`. The choice between them then exists at every single site, exactly as it does today. **The best available outcome is two better-named methods — a readability win, not an enforcement mechanism.** "Make the unsafe form unrepresentable" is not reachable when the unsafe form is somebody's correct answer. ### 3. The nine sites are nine branches, not nine copies They sit in three methods: | method | sites | |---|---| | `resolve(String, InFlight)` | 335, 405, 432, 445 | | `noReportMessage(String)` | 505, 525 | | `fail(String, InFlight, String)` | 549, 566, 609 | Each is a distinct exit path needing its own arrangement to reach. They will *look* similar in their assertion and differ in their setup. That is not boilerplate; that is nine reachability scenarios, and the two already-covered sites (445, 566) are covered by three tests written exactly that way. ### Conclusion **The brief stands: one test per unguarded site, each red under its own mutation.** Tests remain the only thing that can enforce this invariant, because the type system cannot be made to. Two refinements worth taking from the proposal, neither of which changes the acceptance: - **Naming is still worth it.** If you find the seven tests genuinely repetitive within a method, factor the *arrangement* into a helper in the test class. Do not factor the assertion — each test must still fail for its own site. - **Say in the PR which site each test pins**, by line and by branch. That is what stops a future reader deleting one because it "looks like the others" — the objection the proposal correctly raised.
Author
Owner

Merged. Seven sites, seven tests, each red under its own mutation.

Merged to main as part of 634d33b, via PR #585. Test-only, 143 insertions, one file — confirmed by git diff --stat, not taken on report.

My own verification:

  • Merged build with #571 and #562: Tests run: 1784, Failures: 0, Errors: 0, from the Results: block and an independent sum over 132 report files.
  • I re-ran one of the seven mutations myself on the merged tree — the 1-arg remove(target) at site 405 — and it is killed by CompletionResolverTest.aSupersededExhaustedTurnMustNotEvictItsSuccessorsRegistration:662. Exactly one test. The anchor count dropped 5 → 4 as it should, since that exact line is shared by five sites.

The worker named each test after the branch it pins, which is what the earlier note asked for. The seven names read as a list of the actual exit paths — done, exhausted, backend-error, raw-exhausted, raw-backend-error, done-failed, too-fast-backend-error — so a future reader can tell which one is load-bearing for which site. That was the real substance of the objection recorded in the note above, and it is answered.

The alternative fix stays rejected, and the reason is on the record in the note above: a private helper does not make the unsafe form unrepresentable, because inFlight is a private field with 19 raw map calls in the same class; and real encapsulation does not help either, because the 1-arg form is the correct answer at register():252 and captureBaseline():278. The general rule that came out of it — "make the wrong form unrepresentable" is unavailable when the wrong form is somebody's correct answer — is worth more than this ticket.

The count is now five. Instances of the per-site shape: #561, #572, #575, 3f8c38f (merged as #567), and this one plus #582.

Closing.

## Merged. Seven sites, seven tests, each red under its own mutation. Merged to `main` as part of `634d33b`, via PR #585. Test-only, 143 insertions, one file — confirmed by `git diff --stat`, not taken on report. **My own verification:** - Merged build with #571 and #562: **`Tests run: 1784, Failures: 0, Errors: 0`**, from the `Results:` block and an independent sum over 132 report files. - I re-ran one of the seven mutations myself on the merged tree — the 1-arg `remove(target)` at site 405 — and it is killed by `CompletionResolverTest.aSupersededExhaustedTurnMustNotEvictItsSuccessorsRegistration:662`. **Exactly one test.** The anchor count dropped 5 → 4 as it should, since that exact line is shared by five sites. The worker named each test after the branch it pins, which is what the earlier note asked for. The seven names read as a list of the actual exit paths — done, exhausted, backend-error, raw-exhausted, raw-backend-error, done-failed, too-fast-backend-error — so a future reader can tell which one is load-bearing for which site. That was the real substance of the objection recorded in the note above, and it is answered. **The alternative fix stays rejected, and the reason is on the record** in the note above: a private helper does not make the unsafe form unrepresentable, because `inFlight` is a private field with 19 raw map calls in the same class; and real encapsulation does not help either, because the 1-arg form is the **correct** answer at `register():252` and `captureBaseline():278`. The general rule that came out of it — *"make the wrong form unrepresentable" is unavailable when the wrong form is somebody's correct answer* — is worth more than this ticket. **The count is now five.** Instances of the per-site shape: #561, #572, #575, `3f8c38f` (merged as #567), and this one plus #582. Closing.
ltms closed this issue 2026-09-12 15:31:17 +02:00
Author
Owner

Follow-up measurement: all NINE CAS sites are pinned, and none of them is an equivalent mutant

The fleet01 lead asked the right question after this merged: were the survivors checked for
equivalence?
An equivalent mutant is a mutation that no test can ever kill, because the two
forms behave identically on every reachable input. If any of the nine inFlight.remove(target, turn)
sites sat in a branch where no successor can be registered, the CAS there is inert, a one-arg
mutation is unkillable, and a test for it would be asserting on something that cannot fail.

I measured it instead of answering from the brief. Two builds on main 634d33b, each mutating
two sites from the two-argument form to the one-argument form, in-place inside the call so the
statement structure cannot change:

sed -i '' -e '335s/inFlight\.remove(target, turn)/inFlight.remove(target)/' \
          -e '549s/inFlight\.remove(target, turn)/inFlight.remove(target)/' CompletionResolver.java
# two-arg calls in file: 9 -> 7   · mvn -o -B -q compile gate passed first

Result — every site kills at least one test, and the kills are narrow

site branch test killed
:335 resolve() early return (waiter == null || isDone()) aSupersededDoneTurnMustNotEvictItsSuccessorsRegistration:643
:405 resolveExhausted one of the five proven by the implementer
:432 resolveFailure one of the five proven by the implementer
:445 resolveCompletion success arm aSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration:555 and …EchoedNoReportSubPath…:592
:505 raw-scrape exhausted one of the five proven by the implementer
:525 raw-scrape backend error one of the five proven by the implementer
:549 fail() early return aSupersededDoneFailedTurnMustNotEvictItsSuccessorsRegistration:738
:566 fail() resolveFailure success arm aSupersededTurnsFailMustNotEvictItsSuccessorsRegistration:620
:609 failTooFast backend error one of the five proven by the implementer

Mutating :335 and :549 together produced exactly two named failures. Mutating :445 and
:566 together produced exactly three — :445 takes two tests because two different paths reach
that arm. No mutation took a crowd with it. Both runs restored the file to its pristine sha
9b5f2f6d810fa45e.

So: no equivalent mutants here, and the four sites this ticket did not add a test for were already
covered
— :445 and :566 by tests that predate it (:555, :592, :620, from the #572/#575
work), and :335/:549 by two of this ticket's own seven.

Two corrections to what I wrote earlier

  1. I described the seven new tests as "one per unguarded site". That was wrong about which sites.
    Two of the seven (Done, DoneFailed) pin the two early-return sites :335 and :549;
    the other five pin :405, :432, :505, :525, :609. Nothing is missing, but my map of
    test→site was not measured when I wrote it. It is now.
  2. Nine sites, ten killing assertions, because :445 is reached two ways. Counting assertions per
    site is still right; counting sites per assertion is not the same number.

The rule this adds

A SURVIVING MUTANT HAS THREE EXPLANATIONS, AND ONLY ONE OF THEM IS A TEST GAP: the test is
missing, the test is weak, or the mutation is equivalent and no test could ever kill it. The
third invalidates the question rather than answering it, and it is the one nobody checks, because
the first two both end with "write a test" and that feels like progress. Ask it before commissioning
assertions, not after. Here the answer was clean — all nine are killable — but the ticket would have
been wrong to assume that.

## Follow-up measurement: all NINE CAS sites are pinned, and none of them is an equivalent mutant The fleet01 lead asked the right question after this merged: *were the survivors checked for equivalence?* An **equivalent mutant** is a mutation that no test can ever kill, because the two forms behave identically on every reachable input. If any of the nine `inFlight.remove(target, turn)` sites sat in a branch where no successor can be registered, the CAS there is inert, a one-arg mutation is unkillable, and a test for it would be asserting on something that cannot fail. I measured it instead of answering from the brief. Two builds on `main` `634d33b`, each mutating two sites from the two-argument form to the one-argument form, in-place inside the call so the statement structure cannot change: ```bash sed -i '' -e '335s/inFlight\.remove(target, turn)/inFlight.remove(target)/' \ -e '549s/inFlight\.remove(target, turn)/inFlight.remove(target)/' CompletionResolver.java # two-arg calls in file: 9 -> 7 · mvn -o -B -q compile gate passed first ``` ### Result — every site kills at least one test, and the kills are narrow | site | branch | test killed | |---|---|---| | `:335` | `resolve()` early return (`waiter == null \|\| isDone()`) | `aSupersededDoneTurnMustNotEvictItsSuccessorsRegistration:643` | | `:405` | `resolveExhausted` | one of the five proven by the implementer | | `:432` | `resolveFailure` | one of the five proven by the implementer | | `:445` | `resolveCompletion` success arm | `aSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration:555` **and** `…EchoedNoReportSubPath…:592` | | `:505` | raw-scrape exhausted | one of the five proven by the implementer | | `:525` | raw-scrape backend error | one of the five proven by the implementer | | `:549` | `fail()` early return | `aSupersededDoneFailedTurnMustNotEvictItsSuccessorsRegistration:738` | | `:566` | `fail()` `resolveFailure` success arm | `aSupersededTurnsFailMustNotEvictItsSuccessorsRegistration:620` | | `:609` | `failTooFast` backend error | one of the five proven by the implementer | Mutating `:335` and `:549` together produced **exactly two** named failures. Mutating `:445` and `:566` together produced **exactly three** — `:445` takes two tests because two different paths reach that arm. No mutation took a crowd with it. Both runs restored the file to its pristine sha `9b5f2f6d810fa45e`. **So: no equivalent mutants here, and the four sites this ticket did not add a test for were already covered** — `:445` and `:566` by tests that predate it (`:555`, `:592`, `:620`, from the #572/#575 work), and `:335`/`:549` by two of this ticket's own seven. ### Two corrections to what I wrote earlier 1. I described the seven new tests as *"one per unguarded site"*. That was wrong about which sites. Two of the seven (`Done`, `DoneFailed`) pin the two **early-return** sites `:335` and `:549`; the other five pin `:405`, `:432`, `:505`, `:525`, `:609`. Nothing is missing, but my map of test→site was not measured when I wrote it. It is now. 2. Nine sites, ten killing assertions, because `:445` is reached two ways. **Counting assertions per site is still right; counting sites per assertion is not the same number.** ### The rule this adds **A SURVIVING MUTANT HAS THREE EXPLANATIONS, AND ONLY ONE OF THEM IS A TEST GAP:** the test is missing, the test is weak, or **the mutation is equivalent and no test could ever kill it**. The third invalidates the question rather than answering it, and it is the one nobody checks, because the first two both end with "write a test" and that feels like progress. Ask it *before* commissioning assertions, not after. Here the answer was clean — all nine are killable — but the ticket would have been wrong to assume that.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#581