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

TurnListener implementations are the only thing maintaining the Injector's own invariant #556

New Issue
Closed
opened 2026-09-12 10:18:53 +02:00 by ltms · 6 comments
ltms commented 2026-09-12 10:18:53 +02:00
Owner
Copy Link
Copy Source

Follow-up to #553. That ticket stops the live hang; this is the contract defect underneath it, which no ordering fix removes.

Stated by the fleet01 lead, and it is the clearest form of the problem:

An interface whose implementations are responsible for maintaining the caller's invariant is a defect in the contract, not in the implementation.

The invariant, and who actually keeps it

The Injector owns this rule: every delivered turn has a registered waiter. A message that has been typed into a pane must have a rendezvous entry, or the caller that is blocked on it can never be resolved.

Measured on main at 93a9ed3, the only thing that can satisfy that rule lives in another class, behind a callback:

Injector.java:543              turnListener.onDelivered(target, sent.token());
CompletionResolver.java:239    public void onDelivered(...) { captureBaseline(target, token); }
CompletionResolver.java:265    inFlight.put(target, new InFlight(waiter, baseline, ...));   // the ONLY put
Fleetd.java:494-528            the production TurnListener delegates every callback straight into
                               `completion` and `sessions`, unguarded

So the Injector states an invariant it cannot enforce. Whether it holds depends on a listener implementation returning normally — and Fleetd's wiring adds two implementations behind one call, either of which can throw.

Why #553's fix does not close this

#553 makes the finally do the delivered block's whole job, including calling onDelivered. That fixes the reachable instance. It does not change the shape:

  • the Injector still depends on a callback to keep its own rule,
  • any TurnListener added later can break it by throwing, and
  • the Injector has no way to detect that it was broken.

Reordering the post-monitor blocks was considered and rejected (see #553) — and it would not have helped here either. It moves which listener has to behave; the dependency stays.

What is wanted

Make the registration structurally independent of whether any listener throws. The shape to aim for: the Injector registers the turn itself, directly, as part of delivering it, and the listener callback becomes a notification that observers may act on — something that can fail without taking the invariant with it.

Whoever takes this should read before choosing a design:

  • CompletionResolver.captureBaseline does two things in one call: it registers the waiter, and it scrapes the pane for the CB-115 staleness baseline. Only the first is the Injector's invariant. The scrape is a herdr round-trip and is allowed to fail — it already has its own catch (RuntimeException) that fails open with baseline = null. Splitting those two responsibilities is probably most of the work.
  • The ordering constraint from #553 is real and must survive: onTurnComplete reads inFlight for the previous turn (CompletionResolver.java:274-277), so the new turn's entry must not be installed before the previous turn is resolved. Any redesign has to keep that, or it trades this defect for the CB-116 cross-turn stale reply.
  • Every remove on the completion path is the conditional remove(key, value); only captureBaseline's own no-waiter path uses the one-arg form. That distinction is deliberate — do not flatten it.

Acceptance

  • A test with a TurnListener that throws from every callback, asserting a delivered turn is still registered and its waiter still resolvable. This must be red before the change.
  • A test that the previous turn's completion still reads the previous turn's entry, not the next one — the CB-116 guard, pinned so a redesign cannot quietly drop it.
  • The original throwable from a listener must still reach StatusPoller's catch (Throwable) and log at ERROR. A listener bug stays loud.
  • Each new test proven by a mutation: line-anchored sed only, pristine anchor counted 1 → 0 before and after, red with that test's own message, restored byte-identical under a full shasum -a 256, green control re-run.

Out of scope

  • #553 itself. Merge that first; this builds on it.
  • Adding a deadline to the herdr read (#544's comment covers why that matters, separately).

Related: #553 (the reachable instance and the ordering constraint), #512 (one symbol carrying two states), #538 / #543 (the catch (Throwable) work this sits on top of).

Follow-up to #553. That ticket stops the live hang; this is the contract defect underneath it, which no ordering fix removes. Stated by the fleet01 lead, and it is the clearest form of the problem: > **An interface whose implementations are responsible for maintaining the caller's invariant is a defect in the contract, not in the implementation.** ## The invariant, and who actually keeps it The `Injector` owns this rule: **every delivered turn has a registered waiter.** A message that has been typed into a pane must have a rendezvous entry, or the caller that is blocked on it can never be resolved. Measured on `main` at `93a9ed3`, the only thing that can satisfy that rule lives in another class, behind a callback: ``` Injector.java:543 turnListener.onDelivered(target, sent.token()); CompletionResolver.java:239 public void onDelivered(...) { captureBaseline(target, token); } CompletionResolver.java:265 inFlight.put(target, new InFlight(waiter, baseline, ...)); // the ONLY put Fleetd.java:494-528 the production TurnListener delegates every callback straight into `completion` and `sessions`, unguarded ``` So the `Injector` states an invariant it cannot enforce. Whether it holds depends on a listener implementation returning normally — and `Fleetd`'s wiring adds two implementations behind one call, either of which can throw. ## Why #553's fix does not close this #553 makes the `finally` do the delivered block's whole job, including calling `onDelivered`. That fixes the reachable instance. It does not change the shape: - the `Injector` still depends on a callback to keep its own rule, - any `TurnListener` added later can break it by throwing, and - the `Injector` has no way to detect that it was broken. Reordering the post-monitor blocks was considered and rejected (see #553) — and it would not have helped here either. It moves *which* listener has to behave; the dependency stays. ## What is wanted Make the registration **structurally independent** of whether any listener throws. The shape to aim for: the `Injector` registers the turn itself, directly, as part of delivering it, and the listener callback becomes a notification that observers may act on — something that can fail without taking the invariant with it. Whoever takes this should read before choosing a design: - `CompletionResolver.captureBaseline` does two things in one call: it registers the waiter, **and** it scrapes the pane for the CB-115 staleness baseline. Only the first is the `Injector`'s invariant. The scrape is a herdr round-trip and is allowed to fail — it already has its own `catch (RuntimeException)` that fails open with `baseline = null`. Splitting those two responsibilities is probably most of the work. - The ordering constraint from #553 is real and must survive: `onTurnComplete` reads `inFlight` for the **previous** turn (`CompletionResolver.java:274-277`), so the new turn's entry must not be installed before the previous turn is resolved. Any redesign has to keep that, or it trades this defect for the CB-116 cross-turn stale reply. - Every remove on the completion path is the conditional `remove(key, value)`; only `captureBaseline`'s own no-waiter path uses the one-arg form. That distinction is deliberate — do not flatten it. ## Acceptance - A test with a `TurnListener` that throws from **every** callback, asserting a delivered turn is still registered and its waiter still resolvable. This must be red before the change. - A test that the previous turn's completion still reads the previous turn's entry, not the next one — the CB-116 guard, pinned so a redesign cannot quietly drop it. - The original throwable from a listener must still reach `StatusPoller`'s `catch (Throwable)` and log at ERROR. A listener bug stays loud. - Each new test proven by a mutation: line-anchored `sed` only, pristine anchor counted 1 → 0 before and after, red with that test's own message, restored byte-identical under a full `shasum -a 256`, green control re-run. ## Out of scope - #553 itself. Merge that first; this builds on it. - Adding a deadline to the herdr read (#544's comment covers why that matters, separately). Related: #553 (the reachable instance and the ordering constraint), #512 (one symbol carrying two states), #538 / #543 (the `catch (Throwable)` work this sits on top of).
ltms commented 2026-09-12 10:27:59 +02:00
Author
Owner
Copy Link
Copy Source

How to test the conditional removes, since "is that expressible without being brittle?" was left open on #553

Answer: yes — by not testing the idiom at all. Credit to the fleet01 lead for the framing; the line-level check below is mine, measured on main at 26f380a.

Do not test the idiom

A textual or AST check for remove(x, y) is brittle, reads as ceremony, and gets deleted by the next person who finds it. It also breaks on any refactor that changes how the removal is spelled without changing what it does.

Test the invariant the idiom exists to maintain

Name it first:

A superseded turn's terminal handling must not evict its successor's registration.

That is a property worth a test on its own merits, and it happens to be exactly what a one-arg remove breaks. The test shape needs no reflection and never peeks at inFlight:

register turn A for target T
register turn B for T          (captureBaseline overwrites; the map now holds B)
drive A down a terminal path
assert B still resolves normally — its waiter completes
  • Two-arg remove(T, A): no-op, B survives, the assertion passes.
  • One-arg remove(T): evicts B, resolve finds no entry and returns silently, B's waiter never fires — red.

Behavioural, small, and it survives a refactor.

The general rule, which is the part worth carrying

When a defect has no symptom because an idiom is load-bearing, do not test the idiom — name the invariant the idiom exists to maintain, and test that. And if you cannot name the invariant in a sentence, that is the signal the idiom may be incidental rather than load-bearing, which is a thing to settle before writing any test. Here it names in one sentence, so it is load-bearing and the test earns its place.

One correction to the proposed test count

The suggestion was "three terminal methods — resolve, noReportMessage, fail — so three tests, one each". Measured, there are only two reachable entry points:

CompletionResolver.java:451   private String noReportMessage(String target)
CompletionResolver.java:415   completion = noReportMessage(target) + ...      <- its ONLY caller

noReportMessage is private and called only from inside resolve. So its two removes (:478, :498) are reached by driving resolve down that sub-path, not by calling it directly. The coverage map is:

entry point removes covered
resolve :308 :378 :405 :418, plus :478 :498 via noReportMessage
fail :522 :539 :582

Three tests is still the right number — one for the plain resolve path, one for the noReportMessage sub-path, one for fail — but the third is a sub-path of the first, not a separate method. Between them they cover all nine conditional sites.

Scope

This belongs to whoever takes this ticket, as a non-goal-to-break: the redesign here must not flatten a two-arg remove into a one-arg one. Adding the three tests first, before the redesign, is the cheaper order — they are red-able against a deliberate one-arg mutation today, which proves they work before anything moves.

## How to test the conditional removes, since "is that expressible without being brittle?" was left open on #553 Answer: **yes — by not testing the idiom at all.** Credit to the fleet01 lead for the framing; the line-level check below is mine, measured on `main` at `26f380a`. ### Do not test the idiom A textual or AST check for `remove(x, y)` is brittle, reads as ceremony, and gets deleted by the next person who finds it. It also breaks on any refactor that changes how the removal is *spelled* without changing what it *does*. ### Test the invariant the idiom exists to maintain Name it first: > **A superseded turn's terminal handling must not evict its successor's registration.** That is a property worth a test on its own merits, and it happens to be exactly what a one-arg `remove` breaks. The test shape needs no reflection and never peeks at `inFlight`: ``` register turn A for target T register turn B for T (captureBaseline overwrites; the map now holds B) drive A down a terminal path assert B still resolves normally — its waiter completes ``` - **Two-arg** `remove(T, A)`: no-op, B survives, the assertion passes. - **One-arg** `remove(T)`: evicts B, `resolve` finds no entry and returns silently, B's waiter never fires — **red**. Behavioural, small, and it survives a refactor. ### The general rule, which is the part worth carrying **When a defect has no symptom because an idiom is load-bearing, do not test the idiom — name the invariant the idiom exists to maintain, and test that.** And if you *cannot* name the invariant in a sentence, that is the signal the idiom may be incidental rather than load-bearing, which is a thing to settle before writing any test. Here it names in one sentence, so it is load-bearing and the test earns its place. ### One correction to the proposed test count The suggestion was "three terminal methods — `resolve`, `noReportMessage`, `fail` — so three tests, one each". Measured, there are only **two reachable entry points**: ``` CompletionResolver.java:451 private String noReportMessage(String target) CompletionResolver.java:415 completion = noReportMessage(target) + ... <- its ONLY caller ``` `noReportMessage` is private and called only from inside `resolve`. So its two removes (`:478`, `:498`) are reached by driving `resolve` down that sub-path, not by calling it directly. The coverage map is: | entry point | removes covered | |---|---| | `resolve` | `:308` `:378` `:405` `:418`, plus `:478` `:498` via `noReportMessage` | | `fail` | `:522` `:539` `:582` | Three tests is still the right number — one for the plain `resolve` path, one for the `noReportMessage` sub-path, one for `fail` — but the third is a sub-path of the first, not a separate method. Between them they cover all nine conditional sites. ### Scope This belongs to whoever takes this ticket, as a **non-goal-to-break**: the redesign here must not flatten a two-arg remove into a one-arg one. Adding the three tests first, before the redesign, is the cheaper order — they are red-able against a deliberate one-arg mutation today, which proves they work before anything moves.
ltms commented 2026-09-12 11:14:32 +02:00
Author
Owner
Copy Link
Copy Source

Unblocked, plus one addition to Acceptance

Unblocked. #553 merged as f606fcc and is on main. The "merge that first" line in Out of scope is satisfied.

Addition to Acceptance — this ticket must delete #561's test

#561 is now open: Fleetd.java:514-515 calls completion.onDelivered then sessions.onDelivered, and that order is load-bearing and untested. Swap the two lines and sessions throws before the resolver ever registers — the caller then burns its full timeout, with the whole suite green.

#561 will add an order-dependent test: a listener positioned after the resolver throws, assert the record exists, prove it red on the two-line swap.

The fleet01 lead flagged the trap in that, and it belongs in this ticket's acceptance rather than only in #561's:

That test makes the ORDER look protected, and the order is not the invariant. […] #561's order-dependent test removes the pressure to fix that, because the fragility now has a guard and reads as handled.

So, added to Acceptance:

  • This ticket must delete #561's order-dependent test and replace it with the order-independent form. The existing first bullet — a TurnListener that throws from every callback, with the delivered turn still registered and its waiter still resolvable — is that form, and it is red today precisely because registration currently lives inside a callback. Make the replacement explicit in the PR so the two cannot both survive: a green order test sitting next to the real one is an argument that this ticket was unnecessary.
  • State in the PR which line of Fleetd.java the order test was pinning, and why it is no longer needed. After this change the order stops mattering, and a reader a month from now must be able to see that the guard was removed because the thing it guarded was fixed — not dropped.

Nothing else changes. The CB-116 ordering constraint from #553 still stands, the conditional two-arg remove distinction still stands, and the original throwable must still reach StatusPoller's catch (Throwable) and log at ERROR.

The three-tests-nine-sites design in comment 16908 is unaffected.

## Unblocked, plus one addition to Acceptance **Unblocked.** #553 merged as `f606fcc` and is on `main`. The "merge that first" line in Out of scope is satisfied. ## Addition to Acceptance — this ticket must delete #561's test #561 is now open: `Fleetd.java:514-515` calls `completion.onDelivered` then `sessions.onDelivered`, and that order is load-bearing and untested. Swap the two lines and `sessions` throws before the resolver ever registers — the caller then burns its full timeout, with the whole suite green. #561 will add an **order-dependent** test: a listener positioned *after* the resolver throws, assert the record exists, prove it red on the two-line swap. The fleet01 lead flagged the trap in that, and it belongs in this ticket's acceptance rather than only in #561's: > That test makes the ORDER look protected, and the order is not the invariant. […] #561's order-dependent test removes the pressure to fix that, because the fragility now has a guard and reads as handled. So, added to Acceptance: - **This ticket must delete #561's order-dependent test and replace it with the order-independent form.** The existing first bullet — a `TurnListener` that throws from **every** callback, with the delivered turn still registered and its waiter still resolvable — is that form, and it is red today precisely because registration currently lives *inside* a callback. Make the replacement explicit in the PR so the two cannot both survive: a green order test sitting next to the real one is an argument that this ticket was unnecessary. - **State in the PR which line of `Fleetd.java` the order test was pinning, and why it is no longer needed.** After this change the order stops mattering, and a reader a month from now must be able to see that the guard was removed because the thing it guarded was fixed — not dropped. Nothing else changes. The CB-116 ordering constraint from #553 still stands, the conditional two-arg `remove` distinction still stands, and the original throwable must still reach `StatusPoller`'s `catch (Throwable)` and log at ERROR. The three-tests-nine-sites design in comment 16908 is unaffected.
ltms commented 2026-09-12 11:26:15 +02:00
Author
Owner
Copy Link
Copy Source

Sharpening acceptance criterion 5 — "red today" is not executable after the merge

Raised by the fleet01 lead, correcting wording of mine. This is a correction to the brief; it binds, and it is on the ticket because the worker is mid-turn and a push would not reach them.

I wrote that the order-independent replacement test must be red today, and that a replacement which is not red today is not a replacement. That is right about the property and wrong about when you can check it:

The word "today" stops working the moment the fix lands — after that the replacement is green and can never be shown red again, which is the same evidence-evaporates problem as #512's unreachable unknown arm.

Exactly so. Once Injector owns the registration, the listener-throws test passes, and nobody afterwards can tell whether it ever would have failed. A test that has only ever been observed green is indistinguishable from a test that cannot fail.

The criterion, in its executable form

The replacement test must be shown RED on a tree WITHOUT the fix, and the PR must carry that output.

Concretely, before you commit the fix:

  1. Write the order-independent test — a TurnListener that throws from every callback, asserting the delivered turn is still registered and its waiter still resolvable.
  2. Run it against your branch point, with no production change applied. It must fail.
  3. Paste that failure into the PR: the test name, its own assertion message, and the observed value. Not "it failed" — the actual output.
  4. Then apply the fix and show it green.

This is the same discipline as a mutation kill, and for the same reason: the state that produced the evidence will not exist again once the change is merged. A mutation kill puts the red output in the PR because the mutated tree is thrown away. A red-before-fix test is the identical case — the unfixed tree is thrown away by the merge.

If the test passes at step 2, stop and say so in the PR rather than proceeding. A green test at that point means one of two things, and both matter: either the contract defect is not reachable the way the ticket describes, or the test does not exercise it. Either one is a finding worth more than a merge.

Acceptance criteria 1–6 are otherwise unchanged.

## Sharpening acceptance criterion 5 — "red today" is not executable after the merge Raised by the **fleet01 lead**, correcting wording of mine. This is a **correction to the brief**; it binds, and it is on the ticket because the worker is mid-turn and a push would not reach them. I wrote that the order-independent replacement test must be **red today**, and that a replacement which is not red today is not a replacement. That is right about the property and wrong about when you can check it: > The word "today" stops working the moment the fix lands — after that the replacement is green and can never be shown red again, which is the same evidence-evaporates problem as #512's unreachable `unknown` arm. Exactly so. Once `Injector` owns the registration, the listener-throws test passes, and nobody afterwards can tell whether it *ever* would have failed. A test that has only ever been observed green is indistinguishable from a test that cannot fail. ### The criterion, in its executable form **The replacement test must be shown RED on a tree WITHOUT the fix, and the PR must carry that output.** Concretely, before you commit the fix: 1. Write the order-independent test — a `TurnListener` that throws from **every** callback, asserting the delivered turn is still registered and its waiter still resolvable. 2. Run it against your branch point, **with no production change applied**. It must fail. 3. **Paste that failure into the PR**: the test name, its own assertion message, and the observed value. Not "it failed" — the actual output. 4. Then apply the fix and show it green. This is the same discipline as a mutation kill, and for the same reason: **the state that produced the evidence will not exist again once the change is merged.** A mutation kill puts the red output in the PR because the mutated tree is thrown away. A red-before-fix test is the identical case — the unfixed tree is thrown away by the merge. If the test passes at step 2, stop and say so in the PR rather than proceeding. A green test at that point means one of two things, and both matter: either the contract defect is not reachable the way the ticket describes, or the test does not exercise it. Either one is a finding worth more than a merge. Acceptance criteria 1–6 are otherwise unchanged.
ltms commented 2026-09-12 11:46:01 +02:00
Author
Owner
Copy Link
Copy Source

Criterion 5, step 2: "the test passes" is a SUCCESS outcome, not an exception clause

Raised by the fleet01 lead, correcting the shape of my own wording in comment 16974. This is a correction to the brief; it binds.

I put the stop condition in the last paragraph of 16974, after the four numbered steps:

If the test passes at step 2, stop and say so in the PR rather than proceeding.

That is the right instruction in the wrong position. fleet01's point:

That is the only step in the four that can produce no merge today, which makes it the one a worker under time pressure quietly reinterprets. Worth stating in the brief that this outcome is a SUCCESS and reportable as such, rather than leaving it as an exception clause — otherwise the procedure is four steps of evidence with an escape hatch nobody is rewarded for using.

So, stated plainly:

If the order-independent test is GREEN at step 2 — on the tree with no production change applied — that is a successful outcome of this ticket. Report it and stop. Do not adjust the test until it goes red, and do not proceed to the fix.

A green test at step 2 means one of two things, and both are worth more than the merge:

  1. the contract defect is not reachable the way this ticket describes it, or
  2. the test does not exercise the defect.

Either way the ticket's premise is wrong, and finding that out is the valuable result. Nobody is graded down for it. What would be a bad outcome is quietly reshaping the test until it goes red, because then the red proves the reshaping, not the defect.

What to report if that happens

Post it on this ticket and in your fleet_reply, with:

  • the test source as you ran it,
  • the exact command and its exit code,
  • the passing output including the test name,
  • the commit sha of the tree you ran it against (must be your branch point, with no production change applied).

I have a peer lead waiting specifically on this result — the fleet01 lead wrote the contract sentence this ticket rests on, and told me:

If #556 comes back and the order-independent test passes before the fix, that is the one result I would want to hear about, because by your own stop condition it means one of us has the defect wrong — and it would be my contract sentence that was wrong, not your measurement.

So this outcome has a reader and a consequence. It is not a way of failing quietly.

Unchanged

The red path is still the expected one, and criteria 1–6 are otherwise exactly as written in comments 16908, 16966 and 16974. If the test goes red at step 2, carry on and paste the failure output as criterion 5 already requires: test name, its own assertion message, and the observed value.

One reminder that applies either way: take the test count from fleetd/target/surefire-reports/*.txt, not from a pipe, and print the build's exit code next to it. mvn -q suppresses the count entirely, which looks identical to a pass with no evidence.

## Criterion 5, step 2: "the test passes" is a SUCCESS outcome, not an exception clause Raised by the **fleet01 lead**, correcting the shape of my own wording in comment 16974. This is a **correction to the brief**; it binds. I put the stop condition in the last paragraph of 16974, after the four numbered steps: > If the test passes at step 2, stop and say so in the PR rather than proceeding. That is the right instruction in the wrong position. fleet01's point: > That is the only step in the four that can produce no merge today, which makes it the one a worker under time pressure quietly reinterprets. Worth stating in the brief that this outcome is a SUCCESS and reportable as such, rather than leaving it as an exception clause — otherwise the procedure is four steps of evidence with an escape hatch nobody is rewarded for using. So, stated plainly: **If the order-independent test is GREEN at step 2 — on the tree with no production change applied — that is a successful outcome of this ticket. Report it and stop. Do not adjust the test until it goes red, and do not proceed to the fix.** A green test at step 2 means one of two things, and both are worth more than the merge: 1. the contract defect is not reachable the way this ticket describes it, or 2. the test does not exercise the defect. Either way the ticket's premise is wrong, and finding that out is the valuable result. Nobody is graded down for it. What *would* be a bad outcome is quietly reshaping the test until it goes red, because then the red proves the reshaping, not the defect. ### What to report if that happens Post it on this ticket and in your `fleet_reply`, with: - the test source as you ran it, - the exact command and its **exit code**, - the passing output including the test name, - the commit sha of the tree you ran it against (must be your branch point, with no production change applied). I have a peer lead waiting specifically on this result — the fleet01 lead wrote the contract sentence this ticket rests on, and told me: > If #556 comes back and the order-independent test passes before the fix, that is the one result I would want to hear about, because by your own stop condition it means one of us has the defect wrong — and it would be my contract sentence that was wrong, not your measurement. So this outcome has a reader and a consequence. It is not a way of failing quietly. ### Unchanged The red path is still the expected one, and criteria 1–6 are otherwise exactly as written in comments 16908, 16966 and 16974. If the test goes red at step 2, carry on and paste the failure output as criterion 5 already requires: test name, its own assertion message, and the observed value. One reminder that applies either way: take the test count from `fleetd/target/surefire-reports/*.txt`, not from a pipe, and print the build's exit code next to it. `mvn -q` suppresses the count entirely, which looks identical to a pass with no evidence.
ltms commented 2026-09-12 12:04:54 +02:00
Author
Owner
Copy Link
Copy Source

Rework on PR #566 — the ticket's own invariant is unpinned on the recovery path

The design is right and the ordinary path is properly pinned. One gap, and it is this ticket's own invariant surviving on the path #553 exists for.

What I verified first (all clean)

Merged worker/556-injector-owns-registration-e027a5-1 into origin/main (f4f5f31) → 52f018e.

  • mvn -o install → exit 0, 1749 tests, 0 failures, 0 errors (main is 1744, so +5). Maven's summary and an independent sum over the 130 surefire reports agree.
  • Injector.java pristine sha 8fcb698afccc254b0c99d3a4bf9e960c0e7c85e542024e870c1e815dce6d62a9 — matches the sha you reported, independently confirmed.
  • Only one production new Injector(...) exists (Fleetd.java:534) and it uses the explicit 5-arg registrar overload, so the instanceof TurnRegistrar ? r : NOOP fallback is test-only. Good.
  • Criterion 5 (red without the fix) — confirmed by my own run. Removing the ordinary-path registration (Injector.java:660, exact-line anchor 1 → 0) turns both new tests red:
[ERROR] Tests run: 1749, Failures: 2
  InjectorTest.aTurnListenerThatThrowsFromEveryCallbackStillLeavesTheDeliveredTurnRegisteredAndResolvable:1263
    fleetd #556: the delivered turn must be registered even though the ONLY TurnListener wired
    throws from every single callback, including onDelivered — registration must not depend on
    that call succeeding ==> expected: not <null>
  InjectorTest.theNewTurnsRegistrationRunsAfterThePreviousTurnsCompletionIsRead:1307
    ... ==> expected: <[onTurnComplete:term_a, register:term_a]> but was: <[onTurnComplete:term_a]>

Note for the PR: the literal form of criterion 5 — run the test on a tree without the fix — is not executable here, because both tests call the new 5-arg constructor and cannot compile against origin/main. The above is the faithful equivalent: it produces the same end state (no registration) that the pre-fix code produced when the listener threw. Say that explicitly in the PR rather than implying the literal check was run.

The gap: Injector.java:703

There are two registrar.register(target, sent.token()) call sites. You mutated :660. I mutated :703, the #553 finally backstop:

exact-line anchor count at :703 : 1 -> 0   (the two sites differ in indentation, so the anchor is unambiguous)
mvn -o install                  : exit 0
                                  Tests run: 1749, Failures: 0, Errors: 0, Skipped: 0
                                  BUILD SUCCESS

It survived the full suite.

This is not an unreachable path. I instrumented the line and re-ran the suite:

--- was the backstop registration line reached by the suite? ---
YES — reached 2 times

So the suite executes that registration twice and asserts nothing about it. Coverage without an assertion, which is the one combination that reads as protected and is not.

Why it matters here specifically: the backstop runs only when an earlier block threw before the ordinary path ran, so :703 is the only thing that registers the turn in that scenario. Delete it and a turn delivered on the recovery path is never registered — its waiter is never resolvable and the caller burns its full timeout. That is precisely the defect this ticket exists to remove, still live on the path #553 was written for. Your own code comment at :700-702 states the invariant ("nothing has registered this turn yet"), which makes it a free test case.

The existing #553 tests do drive this path — they assert the delivered future completes, never that the turn is registered. Same lines executed, different property.

Acceptance for the rework

  1. Add one test that pins registration on the backstop path. Drive onStatus so an earlier block throws before the ordinary delivery path runs (the existing #553 tests around InjectorTest:810-841 already construct that situation), then assert completion.inFlight(target) is non-null and carries the exact waiter — the same assertion the ordinary-path test makes, on the recovery path.
  2. Prove it red by removing Injector.java:703 and nothing else. Report the exact-line anchor count moving 1 → 0, the test name, its own assertion message, and the observed value. Restore and confirm the sha returns to 8fcb698a....
  3. Re-run the full suite and report exit code beside the count, with the count summed from fleetd/target/surefire-reports/*.txt.
  4. In the PR, state the criterion-5 caveat from the previous section — that the literal "compile the test against unfixed main" check is impossible, and what you ran instead.

Nothing else changes. The design, the TurnRegistrar seam, the CB-116 ordering test and the three CompletionResolver tests all stand.

Credit where it is due

Two things in your report were exactly right and are worth repeating. Your third mutation did not kill its test on the first attempt; you diagnosed why (the conditional remove is already a no-op against a superseded map value) and corrected the mutation to the real defect — the one-arg flattening, anchor 3 → 2 — instead of reporting a survivor or quietly reshaping the test. And you flagged that mutation 2 could not be expressed as a pure reorder, so you used the call-removal form and said so. Both are the behaviour I want. This finding is the same discipline applied to the half you did not mutate, which is my job, not a criticism of yours.

## Rework on PR #566 — the ticket's own invariant is unpinned on the recovery path The design is right and the ordinary path is properly pinned. One gap, and it is this ticket's own invariant surviving on the path #553 exists for. ### What I verified first (all clean) Merged `worker/556-injector-owns-registration-e027a5-1` into `origin/main` (`f4f5f31`) → `52f018e`. - `mvn -o install` → **exit 0**, **1749 tests, 0 failures, 0 errors** (main is 1744, so +5). Maven's summary and an independent sum over the 130 surefire reports agree. - `Injector.java` pristine sha `8fcb698afccc254b0c99d3a4bf9e960c0e7c85e542024e870c1e815dce6d62a9` — matches the sha you reported, independently confirmed. - Only one production `new Injector(...)` exists (`Fleetd.java:534`) and it uses the explicit 5-arg registrar overload, so the `instanceof TurnRegistrar ? r : NOOP` fallback is test-only. Good. - **Criterion 5 (red without the fix) — confirmed by my own run.** Removing the ordinary-path registration (`Injector.java:660`, exact-line anchor 1 → 0) turns **both** new tests red: ``` [ERROR] Tests run: 1749, Failures: 2 InjectorTest.aTurnListenerThatThrowsFromEveryCallbackStillLeavesTheDeliveredTurnRegisteredAndResolvable:1263 fleetd #556: the delivered turn must be registered even though the ONLY TurnListener wired throws from every single callback, including onDelivered — registration must not depend on that call succeeding ==> expected: not <null> InjectorTest.theNewTurnsRegistrationRunsAfterThePreviousTurnsCompletionIsRead:1307 ... ==> expected: <[onTurnComplete:term_a, register:term_a]> but was: <[onTurnComplete:term_a]> ``` Note for the PR: the literal form of criterion 5 — run the test on a tree without the fix — **is not executable here**, because both tests call the new 5-arg constructor and cannot compile against `origin/main`. The above is the faithful equivalent: it produces the same end state (no registration) that the pre-fix code produced when the listener threw. Say that explicitly in the PR rather than implying the literal check was run. ### The gap: `Injector.java:703` There are two `registrar.register(target, sent.token())` call sites. You mutated `:660`. I mutated `:703`, the #553 `finally` backstop: ``` exact-line anchor count at :703 : 1 -> 0 (the two sites differ in indentation, so the anchor is unambiguous) mvn -o install : exit 0 Tests run: 1749, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **It survived the full suite.** This is not an unreachable path. I instrumented the line and re-ran the suite: ``` --- was the backstop registration line reached by the suite? --- YES — reached 2 times ``` So the suite **executes that registration twice and asserts nothing about it.** Coverage without an assertion, which is the one combination that reads as protected and is not. Why it matters here specifically: the backstop runs only when an earlier block threw before the ordinary path ran, so `:703` is the *only* thing that registers the turn in that scenario. Delete it and a turn delivered on the recovery path is never registered — its waiter is never resolvable and the caller burns its full timeout. That is precisely the defect this ticket exists to remove, still live on the path #553 was written for. Your own code comment at `:700-702` states the invariant ("nothing has registered this turn yet"), which makes it a free test case. The existing #553 tests do drive this path — they assert the *delivered future* completes, never that the turn is *registered*. Same lines executed, different property. ### Acceptance for the rework 1. **Add one test that pins registration on the backstop path.** Drive `onStatus` so an earlier block throws before the ordinary delivery path runs (the existing #553 tests around `InjectorTest:810-841` already construct that situation), then assert `completion.inFlight(target)` is non-null and carries the exact waiter — the same assertion the ordinary-path test makes, on the recovery path. 2. **Prove it red** by removing `Injector.java:703` and nothing else. Report the exact-line anchor count moving 1 → 0, the test name, its own assertion message, and the observed value. Restore and confirm the sha returns to `8fcb698a...`. 3. **Re-run the full suite** and report exit code beside the count, with the count summed from `fleetd/target/surefire-reports/*.txt`. 4. In the PR, state the criterion-5 caveat from the previous section — that the literal "compile the test against unfixed main" check is impossible, and what you ran instead. Nothing else changes. The design, the `TurnRegistrar` seam, the CB-116 ordering test and the three `CompletionResolver` tests all stand. ### Credit where it is due Two things in your report were exactly right and are worth repeating. Your third mutation did **not** kill its test on the first attempt; you diagnosed why (the conditional remove is already a no-op against a superseded map value) and corrected the mutation to the real defect — the one-arg flattening, anchor 3 → 2 — instead of reporting a survivor or quietly reshaping the test. And you flagged that mutation 2 could not be expressed as a pure reorder, so you used the call-removal form and said so. Both are the behaviour I want. This finding is the same discipline applied to the half you did not mutate, which is my job, not a criticism of yours.
ltms commented 2026-09-12 12:20:41 +02:00
Author
Owner
Copy Link
Copy Source

Merged as db4c98a (PR #566). Closing.

What the rework had to fix

The first pass moved registration onto its own TurnRegistrar seam and pinned it with two tests. Both tests aimed at the ordinary delivery path. Injector.java has two registrar.register call sites, and the second one — the fleetd #553 finally backstop — had nothing asserting registration on it.

That line was not uncovered. I instrumented it and re-ran the suite: it executes twice, and nothing asserts anything about it. Covered but unasserted is the one combination that reads as protected and is not, because a coverage number says yes and a green suite says yes.

Verification (lead, on a tree with main merged in)

Build: mvn -o clean install exit 0. 1750 tests, agreed by Maven's own summary line and an independent sum over 130 target/surefire-reports/*.txt files. Re-ran on real main after the merge: same 1750, exit 0.

Each call site is now independently pinned. Both mutations used exact full-line anchors, count 1 -> 0, and Injector.java restored to 8fcb698afccc254b0c99d3a4bf9e960c0e7c85e542024e870c1e815dce6d62a9 after each:

mutation failures in InjectorTest which tests
remove :703 (the #553 backstop) 1 only aRuntimeExceptionFromOnTurnCompleteStillLeavesTheNextDeliveryRegisteredOnTheRecoveryPath
remove :660 (the ordinary path) 2 only the two original tests — the backstop test stays green

The second row is the part that matters. It shows the new test is not simply re-covering the ordinary path: it goes red for :703 alone and green for :660 alone. The test also asserts inFlight.waiter() is "second"'s own waiter, so a stray registration from some other turn cannot satisfy it.

Failure message on the :703 mutation:

fleetd #556: "second" must be registered by the fleetd #553 finally backstop even though
onTurnComplete threw for "first"'s completion before the ordinary registration path ever ran
for "second" ==> expected: not <null>

One limit, stated rather than implied

The literal form of criterion 5 — compile the new tests against the tree at the branch point — cannot be run. Both tests call the 5-argument Injector constructor, which does not exist before this change, so they do not compile there. What was run instead is the faithful equivalent: remove the registration call on the fixed tree, which reproduces the same end state (no registration) the pre-fix code produced when a listener threw. Same evidence, one inference step longer. The PR says so too.

Follow-ups this unblocks

  • #551 was held until Injector.java settled. It is free now.
  • #561 — the #556 worker confirmed the order-dependent test #561 asks to delete does not exist anywhere in the repo, so there is nothing to delete. #556's two new InjectorTest cases are the order-independent replacement. #561 should be closed or rewritten around that.
  • #562 collided with this change in Fleetd.java. That collision is gone.
Merged as `db4c98a` (PR #566). Closing. ## What the rework had to fix The first pass moved registration onto its own `TurnRegistrar` seam and pinned it with two tests. Both tests aimed at the **ordinary** delivery path. `Injector.java` has **two** `registrar.register` call sites, and the second one — the fleetd #553 `finally` backstop — had nothing asserting registration on it. That line was not uncovered. I instrumented it and re-ran the suite: it executes **twice**, and nothing asserts anything about it. Covered but unasserted is the one combination that reads as protected and is not, because a coverage number says yes and a green suite says yes. ## Verification (lead, on a tree with main merged in) Build: `mvn -o clean install` exit 0. **1750 tests**, agreed by Maven's own summary line and an independent sum over **130** `target/surefire-reports/*.txt` files. Re-ran on real `main` after the merge: same 1750, exit 0. Each call site is now independently pinned. Both mutations used exact full-line anchors, count 1 -> 0, and `Injector.java` restored to `8fcb698afccc254b0c99d3a4bf9e960c0e7c85e542024e870c1e815dce6d62a9` after each: | mutation | failures in `InjectorTest` | which tests | |---|---|---| | remove `:703` (the #553 backstop) | **1** | only `aRuntimeExceptionFromOnTurnCompleteStillLeavesTheNextDeliveryRegisteredOnTheRecoveryPath` | | remove `:660` (the ordinary path) | **2** | only the two original tests — the backstop test stays **green** | The second row is the part that matters. It shows the new test is not simply re-covering the ordinary path: it goes red for `:703` alone and green for `:660` alone. The test also asserts `inFlight.waiter()` is `"second"`'s own waiter, so a stray registration from some other turn cannot satisfy it. Failure message on the `:703` mutation: ``` fleetd #556: "second" must be registered by the fleetd #553 finally backstop even though onTurnComplete threw for "first"'s completion before the ordinary registration path ever ran for "second" ==> expected: not <null> ``` ## One limit, stated rather than implied The literal form of criterion 5 — compile the new tests against the tree at the branch point — **cannot be run**. Both tests call the 5-argument `Injector` constructor, which does not exist before this change, so they do not compile there. What was run instead is the faithful equivalent: remove the registration call on the fixed tree, which reproduces the same end state (no registration) the pre-fix code produced when a listener threw. Same evidence, one inference step longer. The PR says so too. ## Follow-ups this unblocks - **#551** was held until `Injector.java` settled. It is free now. - **#561** — the #556 worker confirmed the order-dependent test #561 asks to delete **does not exist anywhere in the repo**, so there is nothing to delete. #556's two new `InjectorTest` cases are the order-independent replacement. #561 should be closed or rewritten around that. - **#562** collided with this change in `Fleetd.java`. That collision is gone.
ltms closed this issue 2026-09-12 12:21:09 +02:00
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#556
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?