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

The waiter cleanup in MessageService is maintained at three sites, and one of them is outside any finally #575

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

Follow-up to #572, same shape, different invariant. Spotted by the #572 worker as an out-of-scope
one-liner; measured by me on main at 3f8c38f before filing.

The invariant

"A rendezvous waiter and its asyncTasksByWaiter entry are always cleaned up." It is maintained by
this exact pair:

asyncTasksByWaiter.remove(reply);
rendezvous.close(<session>, reply);

The three sites

# line where protected by a finally?
1 :1007-1008 send(...)'s inner finally yes
2 :1147-1148 answer(...), inline before an early return NO
3 :1227-1228 answer(...)'s inner finally yes

Site 2 in full:

Task task = asyncTasksByTurn.get(turnId);
if (task != null) {
    asyncTasksByWaiter.put(reply, task);          // :1144  entry created here
}
if (!rendezvous.answerAsk(turnId, content)) {
    asyncTasksByWaiter.remove(reply);             // :1147  hand-rolled cleanup
    rendezvous.close(workerSession, reply);       // :1148
    return new Reply(Outcome.STALE_TURN, null);   // :1149
}
clearAsyncQuestion(turnId, false);                // :1151
try {                                             // :1152  the finally-protected region STARTS here

The gap

The try whose finally does this cleanup opens at :1152. The entry is created at :1144. So
there is a window — :1144 to :1152 — in which the waiter and its asyncTasksByWaiter entry
exist and no finally will clean them up. The hand-rolled pair at :1147-1148 covers exactly
one exit from that window: the answerAsk returning false path. It does not cover a throw.

Concretely: if clearAsyncQuestion(turnId, false) at :1151 throws, or if
rendezvous.answerAsk(...) at :1146 throws rather than returning false, control leaves the method
without ever entering the try. The outer finally { lock.unlock(); } still runs — so the session
lock is fine, thanks to #572 — but reply stays in asyncTasksByWaiter and its rendezvous waiter
is never closed.

This is the same shape as #572: one invariant, N sites, and the assertion/protection is not
uniform across them. There it was 23 assertions at one site and zero at the other. Here it is a
finally at two sites and hand-rolled coverage of a single exit at the third.

What is NOT established, and must be before this is called a defect

I have not shown either throw is reachable. A defect on paper is not a reachable defect. Whoever
picks this up must answer, with the code:

  1. Can rendezvous.answerAsk(turnId, content) throw rather than return false?
  2. Can clearAsyncQuestion(turnId, false) throw?

If both are total, this is a robustness/structure issue rather than a live leak, and the right fix
may simply be moving the try up to :1144 so one finally covers every exit — which also deletes
the duplicated pair at :1147-1148 and removes the third site entirely.

If either can throw, it is a real leak and needs a test.

Also unmeasured: whether sites 1 and 3 are themselves asserted. #572 found that a finally being
present says nothing about whether anything pins it. Do not assume these two are covered because they
are structurally correct — that assumption is exactly what #572 disproved. Mutate each one and
report what happens.

Acceptance criteria

  1. Answer the two reachability questions above, with the code, in the PR.
  2. Mutate all three sites independently — delete the pair, one site at a time — and report the
    result for each. Anchor by line number, count the anchor with grep -Fxc (-F literal, -x
    whole line), and expect the count to drop by exactly one. Do not use awk -v: it
    backslash-escape-processes the assigned value, so an anchor holding a tab silently counts 0,
    which reads as "mutation not applied". A pristine count of 0 is impossible.
  3. Locate every line number fresh. The numbers above are for 3f8c38f and this file moves —
    #551 shifted the #572 target by 13 lines between the ticket being written and the fix landing.
  4. If the fix is to widen the try, prove it changes no behaviour on the STALE_TURN path: that
    path must still return STALE_TURN and must still clean up exactly once, not twice.
  5. mvn -o clean install from fleetd/ (no POM at the repo root), exit 0. Report the Maven test
    line and an independent sum over target/surefire-reports/*.txt. Never mvn -q — it hides
    the count.

Related: #572 (the lock-release instance), #561 (the listener-composition instance).

Follow-up to #572, same shape, different invariant. Spotted by the #572 worker as an out-of-scope one-liner; measured by me on `main` at `3f8c38f` before filing. ## The invariant "A rendezvous waiter and its `asyncTasksByWaiter` entry are always cleaned up." It is maintained by this exact pair: ```java asyncTasksByWaiter.remove(reply); rendezvous.close(<session>, reply); ``` ## The three sites | # | line | where | protected by a `finally`? | |---|---|---|---| | 1 | `:1007-1008` | `send(...)`'s inner `finally` | **yes** | | 2 | `:1147-1148` | `answer(...)`, inline before an early `return` | **NO** | | 3 | `:1227-1228` | `answer(...)`'s inner `finally` | **yes** | Site 2 in full: ```java Task task = asyncTasksByTurn.get(turnId); if (task != null) { asyncTasksByWaiter.put(reply, task); // :1144 entry created here } if (!rendezvous.answerAsk(turnId, content)) { asyncTasksByWaiter.remove(reply); // :1147 hand-rolled cleanup rendezvous.close(workerSession, reply); // :1148 return new Reply(Outcome.STALE_TURN, null); // :1149 } clearAsyncQuestion(turnId, false); // :1151 try { // :1152 the finally-protected region STARTS here ``` ## The gap The `try` whose `finally` does this cleanup opens at `:1152`. The entry is created at `:1144`. So there is a window — `:1144` to `:1152` — in which the waiter and its `asyncTasksByWaiter` entry exist and **no `finally` will clean them up**. The hand-rolled pair at `:1147-1148` covers exactly one exit from that window: the `answerAsk` returning false path. It does not cover a throw. Concretely: if `clearAsyncQuestion(turnId, false)` at `:1151` throws, or if `rendezvous.answerAsk(...)` at `:1146` throws rather than returning false, control leaves the method without ever entering the `try`. The outer `finally { lock.unlock(); }` still runs — so the session lock is fine, thanks to #572 — but `reply` stays in `asyncTasksByWaiter` and its rendezvous waiter is never closed. This is the same shape as #572: **one invariant, N sites, and the assertion/protection is not uniform across them.** There it was 23 assertions at one site and zero at the other. Here it is a `finally` at two sites and hand-rolled coverage of a single exit at the third. ## What is NOT established, and must be before this is called a defect **I have not shown either throw is reachable.** A defect on paper is not a reachable defect. Whoever picks this up must answer, with the code: 1. Can `rendezvous.answerAsk(turnId, content)` throw rather than return `false`? 2. Can `clearAsyncQuestion(turnId, false)` throw? If both are total, this is a robustness/structure issue rather than a live leak, and the right fix may simply be moving the `try` up to `:1144` so one `finally` covers every exit — which also deletes the duplicated pair at `:1147-1148` and removes the third site entirely. If either can throw, it is a real leak and needs a test. **Also unmeasured: whether sites 1 and 3 are themselves asserted.** #572 found that a `finally` being present says nothing about whether anything pins it. Do not assume these two are covered because they are structurally correct — that assumption is exactly what #572 disproved. Mutate each one and report what happens. ## Acceptance criteria 1. Answer the two reachability questions above, with the code, in the PR. 2. Mutate all three sites independently — delete the pair, one site at a time — and report the result for each. Anchor by line number, count the anchor with `grep -Fxc` (`-F` literal, `-x` whole line), and expect the count to drop by exactly one. Do **not** use `awk -v`: it backslash-escape-processes the assigned value, so an anchor holding a tab silently counts 0, which reads as "mutation not applied". A pristine count of 0 is impossible. 3. **Locate every line number fresh.** The numbers above are for `3f8c38f` and this file moves — #551 shifted the #572 target by 13 lines between the ticket being written and the fix landing. 4. If the fix is to widen the `try`, prove it changes no behaviour on the `STALE_TURN` path: that path must still return `STALE_TURN` and must still clean up exactly once, not twice. 5. `mvn -o clean install` from `fleetd/` (no POM at the repo root), exit 0. Report the Maven test line **and** an independent sum over `target/surefire-reports/*.txt`. Never `mvn -q` — it hides the count. Related: #572 (the lock-release instance), #561 (the listener-composition instance).
ltms closed this issue 2026-09-12 14:05:24 +02:00
ltms commented 2026-09-12 14:06:21 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged to main as 204da67 (change b091c51, PR #576).

The ticket's fallback case is what happened. I asked for the two reachability questions to be answered before anyone called this a defect, and the answers are: Rendezvous.answerAsk is asks.get(turnId) != null && w.answer().complete(answer), and clearAsyncQuestion(turnId, false) only does a ConcurrentHashMap remove/get and a plain field write. Both are total — neither can throw. So no live leak ever escaped through the gap. This is a structure fix, and the code comment now says so instead of claiming a bug that was not there.

What was actually wrong is still worth fixing: answer()'s inner try opened after the Task lookup/registration and the STALE_TURN early return, so that return was covered only by a hand-rolled copy of the finally's own cleanup pair. Two copies of one cleanup, and only one of them asserted. The fix widens the try upward and deletes the copy. rendezvous.open(workerSession) correctly stays outside it — nothing to clean if it never opened.

Proof, measured here rather than taken from the report. Baseline on the merged tree: Tests run: 1766, Failures: 0 from Maven and from an independent sum over target/surefire-reports/*.txt. Then my own mutation, which is deliberately not the same as the worker's: rather than removing the whole finally (which kills 8 pre-existing tests and so proves nothing about this path), I rebuilt the exact pre-fix shape — moved the inner try { back down below the STALE_TURN return, without restoring the hand-rolled pair. Result:

Tests run: 1766, Failures: 1, Errors: 0, Skipped: 0
MessageServiceTest.answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce:545

One failure, and it is the new test. Nothing else in 1766 tests notices when that path loses its cleanup.

Third instance of one shape on one file. #572 was lock.unlock() at two sites with one asserted. This is the waiter cleanup pair, same file, same miss. The rule: count assertions per site, not per invariant. One invariant kept at N places needs N assertions, and the non-zero total is exactly what hides the zero at place N.

Closing.

Fixed and merged to `main` as **204da67** (change b091c51, PR #576). **The ticket's fallback case is what happened.** I asked for the two reachability questions to be answered before anyone called this a defect, and the answers are: `Rendezvous.answerAsk` is `asks.get(turnId) != null && w.answer().complete(answer)`, and `clearAsyncQuestion(turnId, false)` only does a `ConcurrentHashMap` remove/get and a plain field write. Both are total — neither can throw. So **no live leak ever escaped through the gap**. This is a structure fix, and the code comment now says so instead of claiming a bug that was not there. **What was actually wrong** is still worth fixing: `answer()`'s inner `try` opened *after* the Task lookup/registration and the `STALE_TURN` early return, so that return was covered only by a hand-rolled copy of the `finally`'s own cleanup pair. Two copies of one cleanup, and only one of them asserted. The fix widens the `try` upward and deletes the copy. `rendezvous.open(workerSession)` correctly stays outside it — nothing to clean if it never opened. **Proof, measured here rather than taken from the report.** Baseline on the merged tree: `Tests run: 1766, Failures: 0` from Maven and from an independent sum over `target/surefire-reports/*.txt`. Then my own mutation, which is deliberately not the same as the worker's: rather than removing the whole `finally` (which kills 8 pre-existing tests and so proves nothing about *this* path), I rebuilt the exact pre-fix shape — moved the inner `try {` back down below the `STALE_TURN` return, without restoring the hand-rolled pair. Result: ``` Tests run: 1766, Failures: 1, Errors: 0, Skipped: 0 MessageServiceTest.answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce:545 ``` One failure, and it is the new test. Nothing else in 1766 tests notices when that path loses its cleanup. **Third instance of one shape on one file.** #572 was `lock.unlock()` at two sites with one asserted. This is the waiter cleanup pair, same file, same miss. The rule: **count assertions per site, not per invariant.** One invariant kept at N places needs N assertions, and the non-zero total is exactly what hides the zero at place N. Closing.
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#575
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?