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

MessageService's use of Cancellation.DELIVERED is unpinned — the seam is tested, the caller is not #345

New Issue
Closed
opened 2026-09-04 11:01:04 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 11:01:04 +02:00
Owner
Copy Link
Copy Source

Follow-up to #338, found by my own mutation while merging it. Small, and not urgent — the shipped
behaviour is correct. What is missing is the test that keeps it correct.

What #338 added

MessageService's timeout branch now cancels the exact queued Pending, and reads the result to
resolve the race with the injector picking that same message up:

// MessageService.java, the TimeoutException branch
if (!wasDelivered) {
    // The target monitor makes cancellation atomic with onStatus picking this
    // Pending up. If pickup won, report TIMED_OUT_WORKING because the text landed.
    wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED;
}

That is the right call and it is the question the ticket asked the implementer to answer. Reporting
TIMED_OUT_QUEUED while the text actually landed would be the original bug with a smaller window.

The gap

Mutation W, run on main at a8cadd9: I replaced that line with a bare
injector.cancel(delivery);, so the lost race is ignored and the branch always reports
TIMED_OUT_QUEUED. Full suite, unpiped:

Tests run: 1357, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Nothing failed. Reverted.

InjectorTest.java:190-197 (cancellationReportsDeliveredWhenPickupWonTheRace) does pin
Injector.cancel returning DELIVERED. So the seam is covered and the caller is not. That
is the recurring shape in this repo: a test proves the collaborator behaves, and nothing proves the
caller reads what it returns.

Why it was not just fixed on the spot

Injector is final, so a test cannot stub cancel to return DELIVERED on demand. Driving the
real interleaving needs the pickup to happen between delivery.completion().isDone() returning
false and cancel taking the target monitor — which needs a test seam.

MessageService already carries three test-only hooks from #329. Adding a fourth on my own judgment
felt like the wrong default, so I am filing it instead of doing it quietly.

Goal

A test that fails if MessageService stops reading cancel's result — that is, if Mutation W above
stops being caught.

Candidate mechanisms, as candidates only — pick one and justify it:

  1. A test-only hook in the timeout branch, mirroring the existing finishAsyncTaskRaceHook pattern,
    letting a test drive the pickup into the window. Consistent with what is already there; adds a
    fourth production seam to this class.
  2. Drop final from Injector so a test double can return DELIVERED. Cheaper for the test, but it
    weakens a deliberate constraint on a class in the delivery path — say why that is acceptable
    before choosing it.
  3. Extract the decision — "given wasDelivered and a Cancellation, what Outcome?" — into a small pure
    method and test that directly. No seam, no final change, but it proves the decision rather than
    the wiring, so say plainly what it still does not cover.

Option 3 is the cheapest and the weakest. That may still be the right trade for a branch this narrow.
Whichever you pick, write into the test's javadoc what it does not prove — that habit is what
made the last three gaps here findable in minutes.

Not a criticism of #338

The implementer answered the race question correctly, in the code and in its PR body, and its own
mutation proof covered the main fix. This is the second-order case, and I found it only by mutating
a different line than it did. That is what the merge-side mutation is for.

Follow-up to #338, found by my own mutation while merging it. Small, and not urgent — the shipped behaviour is correct. What is missing is the test that keeps it correct. ## What #338 added `MessageService`'s timeout branch now cancels the exact queued `Pending`, and reads the result to resolve the race with the injector picking that same message up: ```java // MessageService.java, the TimeoutException branch if (!wasDelivered) { // The target monitor makes cancellation atomic with onStatus picking this // Pending up. If pickup won, report TIMED_OUT_WORKING because the text landed. wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED; } ``` That is the right call and it is the question the ticket asked the implementer to answer. Reporting `TIMED_OUT_QUEUED` while the text actually landed would be the original bug with a smaller window. ## The gap **Mutation W**, run on `main` at `a8cadd9`: I replaced that line with a bare `injector.cancel(delivery);`, so the lost race is ignored and the branch always reports `TIMED_OUT_QUEUED`. Full suite, unpiped: ``` Tests run: 1357, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` Nothing failed. Reverted. `InjectorTest.java:190-197` (`cancellationReportsDeliveredWhenPickupWonTheRace`) does pin `Injector.cancel` returning `DELIVERED`. So the **seam** is covered and the **caller** is not. That is the recurring shape in this repo: a test proves the collaborator behaves, and nothing proves the caller reads what it returns. ## Why it was not just fixed on the spot `Injector` is `final`, so a test cannot stub `cancel` to return `DELIVERED` on demand. Driving the real interleaving needs the pickup to happen between `delivery.completion().isDone()` returning false and `cancel` taking the target monitor — which needs a test seam. `MessageService` already carries three test-only hooks from #329. Adding a fourth on my own judgment felt like the wrong default, so I am filing it instead of doing it quietly. ## Goal A test that fails if `MessageService` stops reading `cancel`'s result — that is, if Mutation W above stops being caught. **Candidate mechanisms, as candidates only — pick one and justify it:** 1. A test-only hook in the timeout branch, mirroring the existing `finishAsyncTaskRaceHook` pattern, letting a test drive the pickup into the window. Consistent with what is already there; adds a fourth production seam to this class. 2. Drop `final` from `Injector` so a test double can return `DELIVERED`. Cheaper for the test, but it weakens a deliberate constraint on a class in the delivery path — say why that is acceptable before choosing it. 3. Extract the decision — "given wasDelivered and a Cancellation, what Outcome?" — into a small pure method and test that directly. No seam, no `final` change, but it proves the decision rather than the wiring, so say plainly what it still does not cover. Option 3 is the cheapest and the weakest. That may still be the right trade for a branch this narrow. Whichever you pick, write into the test's javadoc what it does **not** prove — that habit is what made the last three gaps here findable in minutes. ## Not a criticism of #338 The implementer answered the race question correctly, in the code and in its PR body, and its own mutation proof covered the main fix. This is the second-order case, and I found it only by mutating a different line than it did. That is what the merge-side mutation is for.
ltms commented 2026-09-04 11:35:40 +02:00
Author
Owner
Copy Link
Copy Source

Merged as f379847 (--no-ff). Branch was up to date with main.

Mechanism chosen: option 1 — a fourth package-private test seam,
timeoutCancellationRaceHookForTest, invoked between the incomplete completion() read and
injector.cancel. Injector stays final, and the test drives the real cancel result rather
than a stubbed one. I accept the caveat the worker reported: this class now carries four test-only
hooks. That is the cost of testing a class whose collaborator is final and whose bug is an
interleaving.

The worker's own proof. With the line replaced by a bare injector.cancel(delivery);,
sendTimeoutUsesCancellationDeliveredWhenPickupWinsTheRace failed at MessageServiceTest.java:429
— expected: <TIMED_OUT_WORKING> but was: <TIMED_OUT_QUEUED>. That is exactly Mutation W from this
issue, now caught.

My own mutation, on merge — Mutation BB, a different line than the worker used. I moved the
hook call to after injector.cancel, so the pickup happens outside the window instead of inside
it. Full suite, unpiped:

MessageServiceTest.sendTimeoutUsesCancellationDeliveredWhenPickupWinsTheRace:429
  expected: <TIMED_OUT_WORKING> but was: <TIMED_OUT_QUEUED>
Tests run: 1364, Failures: 1, Errors: 0, Skipped: 0
BUILD FAILURE

This was the question I actually wanted answered. A hook-driven race test can pass for the wrong
reason — because any pickup makes the send look delivered, with the window playing no part. It
does not: the test fails when the pickup moves one line later, so it really pins the interleaving.
Restored, git diff --stat empty, and main is green at Tests run: 1364, Failures: 0, Errors: 0, Skipped: 0.

What is still not proven, and the worker wrote this into the test's javadoc rather than leaving
it implied: nothing shows this interleaving happens on its own under production timing. The test
forces it. It proves the caller handles Cancellation.DELIVERED when the race is lost, which is
what this issue asked for and no more.

Closing.

Merged as `f379847` (`--no-ff`). Branch was up to date with `main`. **Mechanism chosen: option 1** — a fourth package-private test seam, `timeoutCancellationRaceHookForTest`, invoked between the incomplete `completion()` read and `injector.cancel`. `Injector` stays `final`, and the test drives the real `cancel` result rather than a stubbed one. I accept the caveat the worker reported: this class now carries four test-only hooks. That is the cost of testing a class whose collaborator is final and whose bug is an interleaving. **The worker's own proof.** With the line replaced by a bare `injector.cancel(delivery);`, `sendTimeoutUsesCancellationDeliveredWhenPickupWinsTheRace` failed at `MessageServiceTest.java:429` — `expected: <TIMED_OUT_WORKING> but was: <TIMED_OUT_QUEUED>`. That is exactly Mutation W from this issue, now caught. **My own mutation, on merge — Mutation BB, a different line than the worker used.** I moved the hook call to *after* `injector.cancel`, so the pickup happens outside the window instead of inside it. Full suite, unpiped: ``` MessageServiceTest.sendTimeoutUsesCancellationDeliveredWhenPickupWinsTheRace:429 expected: <TIMED_OUT_WORKING> but was: <TIMED_OUT_QUEUED> Tests run: 1364, Failures: 1, Errors: 0, Skipped: 0 BUILD FAILURE ``` This was the question I actually wanted answered. A hook-driven race test can pass for the wrong reason — because *any* pickup makes the send look delivered, with the window playing no part. It does not: the test fails when the pickup moves one line later, so it really pins the interleaving. Restored, `git diff --stat` empty, and `main` is green at `Tests run: 1364, Failures: 0, Errors: 0, Skipped: 0`. **What is still not proven**, and the worker wrote this into the test's javadoc rather than leaving it implied: nothing shows this interleaving happens on its own under production timing. The test forces it. It proves the caller handles `Cancellation.DELIVERED` when the race is lost, which is what this issue asked for and no more. Closing.
ltms closed this issue 2026-09-04 11:35:46 +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#345
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?