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

MessageService.answer() can lose its session-lock release and the whole suite stays green #572

New Issue
Open
opened 2026-09-12 13:10:36 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 13:10:36 +02:00
Owner
Copy Link
Copy Source

The finding

MessageService releases its per-session lock in a finally at two sites. Removing one is
caught by 23 tests. Removing the other is caught by nothing.

Measured on main at ba2f4d1, file sha256 b515ed0482fc7f95..., mvn -o clean install from
fleetd/. Baseline: exit 0, 1750 tests.

mutation site enclosing method result
K MessageService.java:998 send(...) (declared :922) BUILD FAILURE — 22 failures + 1 error across MessageServiceTest, FleetMcpTest, FleetAppTest
L MessageService.java:1218 answer(...) (declared :1113) BUILD SUCCESS — 1750/1750 green

Both lines are byte-identical: lock.unlock();. Both sit in the outer finally of
their method. The mutation in each case replaced the line with a comment, leaving an empty
finally {}.

Anchor control, using grep -Fxc (exact whole-line, literal):

  • pristine count of lock.unlock(); = 2
  • after mutating one site = 1

So the mutation really was applied, and the file was restored to sha b515ed0482fc7f95...
afterwards with git status --short empty.

Why this matters

lock is the per-session lock. If answer() returns without releasing it, the thread keeps the
hold forever, and every later send or answer to that session blocks permanently. That is
the highest-consequence failure this class can have — a silently wedged session, no exception, no
log line.

The suite is clearly capable of catching a held session lock: mutation K proves it, loudly, with
23 tests. It simply never exercises answer() in a way that needs the lock again afterwards. So
this is not "hard to test" — it is untested.

This is covered-but-unasserted, not uncovered

The line executes. It is reached on every answer() call in the suite. A coverage report says yes
and a green suite says yes, and both are true — nothing asserts the consequence. That is the one
combination that reads as protected and is not.

Why the sibling site is not enough

A shared property maintained at N sites needs N assertions, not one. Here the property is "the
session lock is always released" and it is maintained at two sites. One site has 23 assertions and
the other has zero, and the total being non-zero is what makes this invisible to a
count-the-assertions check done per invariant instead of per site.

Credit: this shape was predicted by the fleet01 lead as a general rule before this instance was
found — a test written for a backstop tends to assert that recovery happened, not that recovery
was complete.

Acceptance criteria for a fix

  1. Add a test that fails when MessageService.java:1218 is removed. It must prove the lock is
    reacquirable after answer() returns — not merely that answer() returned. Suggested
    shape: drive answer(...) to each of its exits (normal reply, timeout, ExecutionException,
    InterruptedException), then assert a subsequent send/answer on the same session still
    completes rather than blocking. Give it a bounded timeout so a regression fails fast instead of
    hanging CI.
  2. Cover every exit of answer(), not just the happy one. The finally exists for the throwing
    paths.
  3. No production change. MessageService.java must end byte-identical to sha b515ed0482fc7f95....
  4. Prove the kill: re-apply mutation L exactly (sed -i '' '1218s|.*| /* mutation */|'),
    show the suite goes RED with the new test named, restore, and show the sha is back.
  5. mvn -o clean install from fleetd/ (there is no POM at the repo root), exit 0. Report the
    Maven test line and an independent sum over target/surefire-reports/*.txt. Do not use
    mvn -q — it hides the test count.

Note for whoever picks this up

Count the pristine anchor with grep -Fxc (-F literal, -x whole line). Do not use
awk -v p="$LINE" for this: awk backslash-escape-processes the assigned value, so any anchor
containing a tab silently counts 0, which reads as "mutation not applied". A pristine count of 0
is impossible — if you see one, the counter is broken, not the file.

## The finding `MessageService` releases its per-session lock in a `finally` at **two** sites. Removing one is caught by 23 tests. Removing the other is caught by **nothing**. Measured on `main` at `ba2f4d1`, file sha256 `b515ed0482fc7f95...`, `mvn -o clean install` from `fleetd/`. Baseline: exit 0, 1750 tests. | mutation | site | enclosing method | result | |---|---|---|---| | K | `MessageService.java:998` | `send(...)` (declared :922) | **BUILD FAILURE — 22 failures + 1 error across `MessageServiceTest`, `FleetMcpTest`, `FleetAppTest`** | | L | `MessageService.java:1218` | `answer(...)` (declared :1113) | **BUILD SUCCESS — 1750/1750 green** | Both lines are byte-identical: ` lock.unlock();`. Both sit in the outer `finally` of their method. The mutation in each case replaced the line with a comment, leaving an empty `finally {}`. Anchor control, using `grep -Fxc` (exact whole-line, literal): - pristine count of ` lock.unlock();` = **2** - after mutating one site = **1** So the mutation really was applied, and the file was restored to sha `b515ed0482fc7f95...` afterwards with `git status --short` empty. ## Why this matters `lock` is the per-session lock. If `answer()` returns without releasing it, the thread keeps the hold forever, and **every later `send` or `answer` to that session blocks permanently**. That is the highest-consequence failure this class can have — a silently wedged session, no exception, no log line. The suite is clearly *capable* of catching a held session lock: mutation K proves it, loudly, with 23 tests. It simply never exercises `answer()` in a way that needs the lock again afterwards. So this is not "hard to test" — it is untested. ## This is covered-but-unasserted, not uncovered The line executes. It is reached on every `answer()` call in the suite. A coverage report says yes and a green suite says yes, and both are true — nothing asserts the *consequence*. That is the one combination that reads as protected and is not. ## Why the sibling site is not enough A shared property maintained at N sites needs N assertions, not one. Here the property is "the session lock is always released" and it is maintained at two sites. One site has 23 assertions and the other has zero, and the total being non-zero is what makes this invisible to a count-the-assertions check done per *invariant* instead of per *site*. Credit: this shape was predicted by the fleet01 lead as a general rule before this instance was found — a test written for a backstop tends to assert that recovery happened, not that recovery was complete. ## Acceptance criteria for a fix 1. Add a test that fails when `MessageService.java:1218` is removed. It must prove the lock is **reacquirable** after `answer()` returns — not merely that `answer()` returned. Suggested shape: drive `answer(...)` to each of its exits (normal reply, timeout, `ExecutionException`, `InterruptedException`), then assert a subsequent `send`/`answer` on the **same session** still completes rather than blocking. Give it a bounded timeout so a regression fails fast instead of hanging CI. 2. Cover every exit of `answer()`, not just the happy one. The `finally` exists for the throwing paths. 3. No production change. `MessageService.java` must end byte-identical to sha `b515ed0482fc7f95...`. 4. Prove the kill: re-apply mutation L exactly (`sed -i '' '1218s|.*| /* mutation */|'`), show the suite goes RED with the new test named, restore, and show the sha is back. 5. `mvn -o clean install` from `fleetd/` (there is no POM at the repo root), exit 0. Report the Maven test line **and** an independent sum over `target/surefire-reports/*.txt`. Do not use `mvn -q` — it hides the test count. ## Note for whoever picks this up Count the pristine anchor with `grep -Fxc` (`-F` literal, `-x` whole line). Do **not** use `awk -v p="$LINE"` for this: `awk` backslash-escape-processes the assigned value, so any anchor containing a tab silently counts 0, which reads as "mutation not applied". A pristine count of 0 is impossible — if you see one, the counter is broken, not the file.
ltms commented 2026-09-12 13:13:43 +02:00
Author
Owner
Copy Link
Copy Source

Evidence for the "covered, not uncovered" half of the claim, since a surviving mutation has three
possible causes and only one of them is this one.

A surviving mutation means the line never executes, or it executes and nothing asserts about
it, or the tests that cover it were excluded from the profile that was run. Ruling out the
first and third:

Third cause — excluded tests. Not applicable here. MessageService.answer is exercised by
plain, untagged tests in the default profile. (fleetd/pom.xml:264 sets excludedGroups=contract,
which is what excludes the @Tag("contract") classes — that trap applies to #567, not to this.)
Both mutation K and mutation L ran under the same default profile, mvn -o clean install from
fleetd/, and both reported the same 1750-test denominator. K failed and L passed on identical
inputs, so the difference is the assertions, not the profile.

First cause — never executes. Ruled out by call-site count on ba2f4d1:

grep -c '\.answer(' per test file:
  MessageServiceTest   39
  FleetMcpTest          6
  RendezvousTest        1   <- NOT this method; it is `t.answer()` on a turn handle, RendezvousTest.java:70

So 45 call sites across the two classes that actually drive MessageService.answer(...), plus the
REST/MCP route through FleetMcp.java:757 (messages.answer(turnId, content, timeout)).

That leaves the second cause. The line at :1218 runs many times per suite and nothing asserts its
consequence — which is the finding.

Corroborating asymmetry: mutation K (the send(...) unlock) failed tests in MessageServiceTest,
FleetMcpTest and FleetAppTest. So the suite is demonstrably able to observe a wedged session
lock through all three layers. It just never asks that question after answer(...).

Evidence for the "covered, not uncovered" half of the claim, since a surviving mutation has three possible causes and only one of them is this one. A surviving mutation means the line never executes, **or** it executes and nothing asserts about it, **or** the tests that cover it were excluded from the profile that was run. Ruling out the first and third: **Third cause — excluded tests.** Not applicable here. `MessageService.answer` is exercised by plain, untagged tests in the default profile. (`fleetd/pom.xml:264` sets `excludedGroups=contract`, which is what excludes the `@Tag("contract")` classes — that trap applies to #567, not to this.) Both mutation K and mutation L ran under the same default profile, `mvn -o clean install` from `fleetd/`, and both reported the same 1750-test denominator. K failed and L passed on identical inputs, so the difference is the assertions, not the profile. **First cause — never executes.** Ruled out by call-site count on `ba2f4d1`: ``` grep -c '\.answer(' per test file: MessageServiceTest 39 FleetMcpTest 6 RendezvousTest 1 <- NOT this method; it is `t.answer()` on a turn handle, RendezvousTest.java:70 ``` So 45 call sites across the two classes that actually drive `MessageService.answer(...)`, plus the REST/MCP route through `FleetMcp.java:757` (`messages.answer(turnId, content, timeout)`). That leaves the second cause. The line at :1218 runs many times per suite and nothing asserts its consequence — which is the finding. Corroborating asymmetry: mutation K (the `send(...)` unlock) failed tests in `MessageServiceTest`, `FleetMcpTest` **and** `FleetAppTest`. So the suite is demonstrably able to observe a wedged session lock through all three layers. It just never asks that question after `answer(...)`.
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#572
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?