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

fleetd #575: widen answer()'s try so one finally covers its STALE_TURN exit #576

Closed
agent wants to merge 0 commits from worker/575-waiter-cleanup-sites-62ad80-1 into main
pull from: worker/575-waiter-cleanup-sites-62ad80-1
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-12 13:56:45 +02:00
Member
Copy Link
Copy Source

Fixes #575.

Reachability (measured, with the code)

  1. Can rendezvous.answerAsk(turnId, content) throw? No. Rendezvous.answerAsk is asks.get(turnId) != null && w.answer().complete(answer). ConcurrentHashMap.get on a non-null key never throws, and CompletableFuture.complete (plain, no custom subclass — see AskWaiter's field type) never throws either.
  2. Can clearAsyncQuestion(turnId, false) throw? No. With forgetTurn=false it only does pushLoop.questionClosed(turnId) (a ConcurrentHashMap.remove on a non-null key — turnId is proven non-null earlier in answer(), since rendezvous.askSession(null) would already have thrown at the method's own top guard), a ConcurrentHashMap.get, and a plain field write. None of these throw.

Both answers are total, so this is a structure problem, not a live leak — exactly the ticket's fallback case. The window (:1144 entry created → old :1152 try opens) was real, but its one exit (answerAsk returning false) was already covered by a hand-rolled copy of the finally's own cleanup pair, so nothing ever left uncovered in practice.

Mutation results (3 sites, pristine main, one at a time, restored between runs)

site location result
1 send()'s finally (:1007-1008) killed — 27 failures + 9 errors (36 tests)
2 answer()'s hand-rolled STALE_TURN cleanup (:1147-1148) survivor — 1765/1765 green, 0 failures, 0 errors
3 answer()'s finally (:1227-1228) killed — 3 failures + 5 errors (8 tests)

Site 2 is an untested survivor — the same shape #572 found on this file (a finally asserted at one site, an identical sibling not).

Fix

Widened answer()'s try to wrap the Task lookup/registration and the STALE_TURN check, so the single finally at the bottom covers every exit, including STALE_TURN. Deleted the duplicated hand-rolled pair.

Added answerAskLapseRaceHookForTest (mirrors the existing askTimeoutRaceHookForTest/abandonCleanupHookForTest pattern) and a regression test, answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce, that deterministically reproduces the 'ask lapsed between the lookup and the unblock' case via rendezvous.answerAsk racing in from the hook. It asserts: (a) the call still returns STALE_TURN, (b) its own forward waiter is closed exactly once (rendezvous.currentWaiter(T) == null afterward — re-mutating the fixed code's now-single finally makes this exact test fail, confirming it actually covers the path), and (c) the worker's own ask() call is not stranded.

Tests

mvn -o clean install from fleetd/: BUILD SUCCESS. Maven: Tests run: 1766, Failures: 0, Errors: 0, Skipped: 0. Independent sum over target/surefire-reports/*.txt: Tests run: 1766 Failures: 0 Errors: 0 Skipped: 0 — they agree. Baseline before any change (pristine main) was 1765/1765 green, matching both Maven and the surefire-report sum.

sha256 of MessageService.java: pristine 57a071f2d5dc39f7cddbe8b22fc030cbedc5bdf0ff48f2656f381f3a0c555713, after the fix 2906ecc5c02ade533ddaeb464d19b092ed4a761a6f3795502aa9479ae70d3aa8.

Fixes #575. ## Reachability (measured, with the code) 1. Can `rendezvous.answerAsk(turnId, content)` throw? **No.** `Rendezvous.answerAsk` is `asks.get(turnId) != null && w.answer().complete(answer)`. `ConcurrentHashMap.get` on a non-null key never throws, and `CompletableFuture.complete` (plain, no custom subclass — see `AskWaiter`'s field type) never throws either. 2. Can `clearAsyncQuestion(turnId, false)` throw? **No.** With `forgetTurn=false` it only does `pushLoop.questionClosed(turnId)` (a `ConcurrentHashMap.remove` on a non-null key — `turnId` is proven non-null earlier in `answer()`, since `rendezvous.askSession(null)` would already have thrown at the method's own top guard), a `ConcurrentHashMap.get`, and a plain field write. None of these throw. Both answers are total, so this is a **structure problem, not a live leak** — exactly the ticket's fallback case. The window (:1144 entry created → old :1152 try opens) was real, but its one exit (`answerAsk` returning `false`) was already covered by a hand-rolled copy of the finally's own cleanup pair, so nothing ever left uncovered in practice. ## Mutation results (3 sites, pristine `main`, one at a time, restored between runs) | site | location | result | |---|---|---| | 1 | `send()`'s finally (:1007-1008) | **killed** — 27 failures + 9 errors (36 tests) | | 2 | `answer()`'s hand-rolled STALE_TURN cleanup (:1147-1148) | **survivor** — 1765/1765 green, 0 failures, 0 errors | | 3 | `answer()`'s finally (:1227-1228) | **killed** — 3 failures + 5 errors (8 tests) | Site 2 is an untested survivor — the same shape #572 found on this file (a finally asserted at one site, an identical sibling not). ## Fix Widened `answer()`'s try to wrap the Task lookup/registration and the STALE_TURN check, so the single finally at the bottom covers every exit, including STALE_TURN. Deleted the duplicated hand-rolled pair. Added `answerAskLapseRaceHookForTest` (mirrors the existing `askTimeoutRaceHookForTest`/`abandonCleanupHookForTest` pattern) and a regression test, `answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce`, that deterministically reproduces the 'ask lapsed between the lookup and the unblock' case via `rendezvous.answerAsk` racing in from the hook. It asserts: (a) the call still returns `STALE_TURN`, (b) its own forward waiter is closed exactly once (`rendezvous.currentWaiter(T) == null` afterward — re-mutating the fixed code's now-single finally makes this exact test fail, confirming it actually covers the path), and (c) the worker's own `ask()` call is not stranded. ## Tests `mvn -o clean install` from `fleetd/`: **BUILD SUCCESS**. Maven: `Tests run: 1766, Failures: 0, Errors: 0, Skipped: 0`. Independent sum over `target/surefire-reports/*.txt`: `Tests run: 1766 Failures: 0 Errors: 0 Skipped: 0` — they agree. Baseline before any change (pristine `main`) was 1765/1765 green, matching both Maven and the surefire-report sum. sha256 of `MessageService.java`: pristine `57a071f2d5dc39f7cddbe8b22fc030cbedc5bdf0ff48f2656f381f3a0c555713`, after the fix `2906ecc5c02ade533ddaeb464d19b092ed4a761a6f3795502aa9479ae70d3aa8`.
agent added 1 commit 2026-09-12 13:56:46 +02:00
fleetd #575: widen answer()'s try so one finally covers its STALE_TURN exit
CI / shell-tests (pull_request) Successful in 5s
Details
CI / contract (pull_request) Successful in 58s
Details
CI / build (pull_request) Successful in 2m30s
Details
b091c51eee 
The waiter cleanup pair (asyncTasksByWaiter.remove + rendezvous.close) was
duplicated: two sites sit in a finally, the third was hand-rolled inline
before answer()'s early STALE_TURN return, structurally outside any finally.

Both rendezvous.answerAsk and clearAsyncQuestion(turnId, false) are total
(cannot throw), so the gap never leaked in practice. But the duplicate was
untested: mutating it away left all 1765 tests green, while the two
finally-protected sites are each killed by 8-36 tests. Same shape as #572.

Fix: widen the try to wrap the Task registration and the STALE_TURN check,
so the single finally covers every exit and the hand-rolled copy is gone.
Added a race hook + regression test that deterministically reproduces the
'ask lapsed between the lookup and the unblock' case and proves the fix
still returns STALE_TURN and cleans up exactly once.
ltms commented 2026-09-12 14:05:59 +02:00
Owner
Copy Link
Copy Source

Merged to main as 204da67 (merge commit; the change itself is b091c51). Closing this PR by hand — the merge was done locally and pushed.

What I checked myself, not taken from the report

The tree I tested is the tree that is now on main. git rev-parse of all three gives the same tree:

main^{tree}       6f687decf0f05a7e2d8527101fd3f5671ebb2079
my build tree     6f687decf0f05a7e2d8527101fd3f5671ebb2079
b091c51^{tree}    6f687decf0f05a7e2d8527101fd3f5671ebb2079

So no separate post-merge build was needed. Same bytes, already measured.

Baseline. Tests run: 1766, Failures: 0, Errors: 0, Skipped: 0 from Maven, and the same numbers from an independent sum over target/surefire-reports/*.txt. Two sources agree. Previous main was 1765, so this adds exactly 1 test.

My own mutation — and it is a different one from yours. Yours removed the whole merged finally. That killed 8 pre-existing tests plus the new one. It proves the finally runs. It does not prove the STALE_TURN path reaches it, because the crowd of 8 could have carried the kill on its own.

So I rebuilt the exact pre-fix shape instead: move the inner try { back down, to just below the STALE_TURN return, and do not restore the hand-rolled pair. That leaves every other exit covered and only the STALE_TURN exit uncovered. I located the line fresh (try { at that indentation appears 4 times in the file, so a global substitution would have hit the wrong one), and I checked the braces still balanced (705 open, 705 close) before building.

Result:

Tests run: 1766, Failures: 1, Errors: 0, Skipped: 0
MessageServiceTest.answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce:545
  the forward waiter this answer() call opened must be closed after a STALE_TURN return
  ==> expected: <null> but was: <java.util.concurrent.CompletableFuture@28670912[Not completed]>

Exactly one failure, and it is the new test. That is the proof the PR needed: this test is the only thing holding that path, and nothing else in 1766 tests notices when it goes.

File restored afterwards, sha back to 2906ecc5c02ade53…, git status clean in the fetch tree.

On the diff itself

I read the whole production change. rendezvous.open(workerSession) correctly stays outside the widened try — there is nothing to clean if it never opened. The duplicated pair is gone, so cleanup runs exactly once, through one finally. The new hook follows the file's existing askTimeoutRaceHookForTest convention.

Your reachability work is the part I would keep. Both calls in the window are total, so this fixed no live leak, and the code comment says so plainly instead of claiming a bug that was not there. That is the right way to write up a structure fix.

For the record

This is the third instance of the same shape on this file: one invariant kept at N sites, asserted at fewer than N. #572 was the session lock (lock.unlock() at two sites, one asserted). This is the waiter cleanup. The rule that catches it is: count assertions per site, not per invariant. A non-zero total is what hides the zero at one site.

Merged to `main` as **204da67** (merge commit; the change itself is b091c51). Closing this PR by hand — the merge was done locally and pushed. ## What I checked myself, not taken from the report **The tree I tested is the tree that is now on `main`.** `git rev-parse` of all three gives the same tree: ``` main^{tree} 6f687decf0f05a7e2d8527101fd3f5671ebb2079 my build tree 6f687decf0f05a7e2d8527101fd3f5671ebb2079 b091c51^{tree} 6f687decf0f05a7e2d8527101fd3f5671ebb2079 ``` So no separate post-merge build was needed. Same bytes, already measured. **Baseline.** `Tests run: 1766, Failures: 0, Errors: 0, Skipped: 0` from Maven, and the same numbers from an independent sum over `target/surefire-reports/*.txt`. Two sources agree. Previous `main` was 1765, so this adds exactly 1 test. **My own mutation — and it is a different one from yours.** Yours removed the whole merged `finally`. That killed 8 pre-existing tests plus the new one. It proves the `finally` runs. It does **not** prove the `STALE_TURN` path reaches it, because the crowd of 8 could have carried the kill on its own. So I rebuilt the exact pre-fix shape instead: move the inner `try {` back **down**, to just below the `STALE_TURN` return, and do not restore the hand-rolled pair. That leaves every other exit covered and only the `STALE_TURN` exit uncovered. I located the line fresh (`try {` at that indentation appears 4 times in the file, so a global substitution would have hit the wrong one), and I checked the braces still balanced (705 open, 705 close) before building. Result: ``` Tests run: 1766, Failures: 1, Errors: 0, Skipped: 0 MessageServiceTest.answerLosingTheRaceToAnAlreadyAnsweredAskStillReturnsStaleTurnAndCleansUpOnce:545 the forward waiter this answer() call opened must be closed after a STALE_TURN return ==> expected: <null> but was: <java.util.concurrent.CompletableFuture@28670912[Not completed]> ``` **Exactly one failure, and it is the new test.** That is the proof the PR needed: this test is the only thing holding that path, and nothing else in 1766 tests notices when it goes. File restored afterwards, sha back to `2906ecc5c02ade53…`, `git status` clean in the fetch tree. ## On the diff itself I read the whole production change. `rendezvous.open(workerSession)` correctly stays **outside** the widened `try` — there is nothing to clean if it never opened. The duplicated pair is gone, so cleanup runs exactly once, through one `finally`. The new hook follows the file's existing `askTimeoutRaceHookForTest` convention. Your reachability work is the part I would keep. Both calls in the window are total, so this fixed no live leak, and the code comment says so plainly instead of claiming a bug that was not there. That is the right way to write up a structure fix. ## For the record This is the **third** instance of the same shape on this file: one invariant kept at N sites, asserted at fewer than N. #572 was the session lock (`lock.unlock()` at two sites, one asserted). This is the waiter cleanup. The rule that catches it is: **count assertions per site, not per invariant.** A non-zero total is what hides the zero at one site.
ltms closed this pull request 2026-09-12 14:06:05 +02:00
Some checks are pending
CI / shell-tests (pull_request) Successful in 5s
Details
CI / contract (pull_request) Successful in 58s
Details
CI / build (pull_request) Successful in 2m30s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#576
Delete Branch "worker/575-waiter-cleanup-sites-62ad80-1"

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?