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

MessageService: an async ticket is still stranded between clearAsyncQuestion and closeAsk #334

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

Follow-up to #329. F1 in #329 narrows this race; it does not close it.

What #329 fixed

answer() looked the task up twice: once at :991 and once inside the old
finishAsyncTask(String turnId, Reply) overload. Between the two lookups the map entry could
disappear, so the ticket was never completed. F1 removed the second lookup and completes from the
Task already captured at :991.

What is still open

The Task captured at :991 can already be null for a genuine async ticket. ask() drops the
asyncTasksByTurn entry earlier than it closes the ask:

  • clearAsyncQuestion(turnId, true) runs in ask()'s catch block — this removes the map entry.
  • rendezvous.closeAsk(turnId) runs in ask()'s finally block — this is what stops the ask
    being answerable.

Between those two the ask is still answerable while the map entry is already gone. answer()'s own
lookup then returns null, the task != null guard skips the completion, and the ticket is
stranded — the same outcome #329 set out to fix, one step earlier in the same race.

Measured

2026-09-04, throwaway probe test PROBE_forgetBeforeAnswersOwnLookup on main at 0c86503
(#329 merged). The probe fires only that first half — it drops the map entry — before answer()
runs:

PROBE-GAP ask=ANSWERED
PROBE-GAP answer=REPLIED phase=PENDING reply=null

answer() reports REPLIED, but the ticket stays PENDING with no reply. The probe was removed
again; it is not in the tree.

One limitation, stated honestly. The probe used forgetTurnForTest, which omits ask()'s
markAskTimedOut call. That cannot change the outcome: askTimedOut is read only by
askAnsweredAsyncTasks, and reply() never reaches it while answer()'s own waiter is live. I have
not reproduced the window by racing the two real threads.

Candidate fixes (decide, do not just take the first)

  1. Move clearAsyncQuestion(turnId, true) out of the catch and into the same finally as
    closeAsk, after it — so the entry never outlives answerability in the wrong direction.
  2. Have answer() fall back to the ticket registry when its captured Task is null but the
    turn was a real async ticket.

Option 1 is the smaller change and closes the ordering rather than papering over it, but it changes
teardown ordering on the timeout path — read every exit of ask() before moving anything.

Note

MessageService.answer() carries a comment naming this issue number. If this is fixed or closed
another way, that comment must be updated in the same change.

Follow-up to #329. F1 in #329 narrows this race; it does not close it. ## What #329 fixed `answer()` looked the task up twice: once at :991 and once inside the old `finishAsyncTask(String turnId, Reply)` overload. Between the two lookups the map entry could disappear, so the ticket was never completed. F1 removed the second lookup and completes from the `Task` already captured at :991. ## What is still open The `Task` captured at :991 can already be `null` for a **genuine async ticket**. `ask()` drops the `asyncTasksByTurn` entry earlier than it closes the ask: - `clearAsyncQuestion(turnId, true)` runs in `ask()`'s **catch** block — this removes the map entry. - `rendezvous.closeAsk(turnId)` runs in `ask()`'s **finally** block — this is what stops the ask being answerable. Between those two the ask is still answerable while the map entry is already gone. `answer()`'s own lookup then returns `null`, the `task != null` guard skips the completion, and the ticket is stranded — the same outcome #329 set out to fix, one step earlier in the same race. ## Measured 2026-09-04, throwaway probe test `PROBE_forgetBeforeAnswersOwnLookup` on `main` at `0c86503` (#329 merged). The probe fires only that first half — it drops the map entry — before `answer()` runs: ``` PROBE-GAP ask=ANSWERED PROBE-GAP answer=REPLIED phase=PENDING reply=null ``` `answer()` reports `REPLIED`, but the ticket stays `PENDING` with no reply. The probe was removed again; it is not in the tree. **One limitation, stated honestly.** The probe used `forgetTurnForTest`, which omits `ask()`'s `markAskTimedOut` call. That cannot change the outcome: `askTimedOut` is read only by `askAnsweredAsyncTasks`, and `reply()` never reaches it while `answer()`'s own waiter is live. I have not reproduced the window by racing the two real threads. ## Candidate fixes (decide, do not just take the first) 1. Move `clearAsyncQuestion(turnId, true)` out of the catch and into the same `finally` as `closeAsk`, after it — so the entry never outlives answerability in the wrong direction. 2. Have `answer()` fall back to the ticket registry when its captured `Task` is `null` but the turn was a real async ticket. Option 1 is the smaller change and closes the ordering rather than papering over it, but it changes teardown ordering on the timeout path — read every exit of `ask()` before moving anything. ## Note `MessageService.answer()` carries a comment naming this issue number. If this is fixed or closed another way, that comment must be updated in the same change.
ltms commented 2026-09-04 12:13:18 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 86dddfe (--no-ff, the branch was behind main), plus my own follow-up b6b88c5.
main is green at Tests run: 1372, Failures: 0, Errors: 0, Skipped: 0.

Mechanism: candidate 1, and the worker did what invariant 3 asked before moving anything. It
reported five exits from ask() and what each now does: an early NO_WAITER return before the
try; the normal ANSWERED return; the TimeoutException catch (the one changed); and the
ExecutionException / InterruptedException catches, which rethrow. All of the last four still go
through the shared finally, unchanged.

Inside the timeout catch the order is now rendezvous.closeAsk(turnId) first, then
markAskTimedOut + clearAsyncQuestion. Both run on one thread with nothing yielding between them,
so a racing answer() either sees the ask still open — and the Task mapping is then guaranteed
still present — or sees it closed and returns STALE_TURN from its own top check, before it ever
reaches asyncTasksByTurn. closeAsk is idempotent, so the shared finally re-running it is
harmless. That closes the window rather than narrowing it, which is what #329 could not do.

My markAskTimedOut argument held. The worker traced every call site: askTimedOut is read
only by askAnsweredAsyncTasks, and reply() never reaches that fallback while a live rendezvous
waiter is open. So the probe's use of forgetTurnForTest did not change the measured outcome. I
asked it to check that because it was mine and unmeasured; it is now checked.

A second fix the worker found on its own, and where I had to do the work. It also gated the
whole timeout-teardown block on ticket.fresh(). That block previously ran for a coalesced
duplicate too, and a duplicate passes its own timeoutMillis, which says nothing about whether
the shared ask is done. So a duplicate timing out first could forget the mapping and lapse an ask
the fresh owner was still legitimately holding — the same class of bug as this ticket. The finally
block and the NO_WAITER branch were both already gated this way; the timeout catch was the only
one that was not.

The worker said plainly that no existing test hit that edge. Mutation GG: I removed the
ticket.fresh() gate and kept the new order. Full suite:

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

Nothing failed. The gate shipped with the reorder and nothing held it there — a correct fix with no
test is one refactor away from being undone.

So I wrote that test myself (b6b88c5):
aCoalescedDuplicateAskTimingOutLeavesTheFreshOwnersAskOpen. A fresh ask with a 5000ms timeout, a
coalesced duplicate on the same session with 100ms, then an answer on the shared turn. With the gate
in place the fresh owner gets ANSWERED; with Mutation GG re-applied:

MessageServiceTest.aCoalescedDuplicateAskTimingOutLeavesTheFreshOwnersAskOpen:439
  a duplicate's timeout must not lapse the ask the fresh owner still holds
  ==> expected: <ANSWERED> but was: <TIMED_OUT>

Its javadoc records the measurement and says what it does not prove — the ordering inside the
gate, which is the worker's own test's job.

Housekeeping done. The answer() comment naming this issue is rewritten. It no longer says
"do not read this guard as complete"; it now says a null task there means the ordinary
blocking-send case or #329's already-fixed race, and names the test that pins this window shut.

Noted, not touched: the NO_WAITER branch has the same clear-then-close statement order, and
the worker traced it as unreachable by a racing answer() because the turnId is never disclosed to
the primary on that path. I have not re-derived that myself. It is a paper shape with an argument
against reachability, which is the right place to leave it.

MessageService now carries six test-only hooks. Each exists because its race is unreachable
through the public API — the same reason each bug was invisible — but six is enough that the next
one needs a harder look than "the file already does this".

Closing.

Merged as `86dddfe` (`--no-ff`, the branch was behind main), plus my own follow-up `b6b88c5`. `main` is green at `Tests run: 1372, Failures: 0, Errors: 0, Skipped: 0`. **Mechanism: candidate 1**, and the worker did what invariant 3 asked before moving anything. It reported five exits from `ask()` and what each now does: an early `NO_WAITER` return before the `try`; the normal `ANSWERED` return; the `TimeoutException` catch (the one changed); and the `ExecutionException` / `InterruptedException` catches, which rethrow. All of the last four still go through the shared `finally`, unchanged. Inside the timeout catch the order is now `rendezvous.closeAsk(turnId)` first, then `markAskTimedOut` + `clearAsyncQuestion`. Both run on one thread with nothing yielding between them, so a racing `answer()` either sees the ask still open — and the `Task` mapping is then guaranteed still present — or sees it closed and returns `STALE_TURN` from its own top check, before it ever reaches `asyncTasksByTurn`. `closeAsk` is idempotent, so the shared `finally` re-running it is harmless. That closes the window rather than narrowing it, which is what #329 could not do. **My `markAskTimedOut` argument held.** The worker traced every call site: `askTimedOut` is read only by `askAnsweredAsyncTasks`, and `reply()` never reaches that fallback while a live rendezvous waiter is open. So the probe's use of `forgetTurnForTest` did not change the measured outcome. I asked it to check that because it was mine and unmeasured; it is now checked. **A second fix the worker found on its own, and where I had to do the work.** It also gated the whole timeout-teardown block on `ticket.fresh()`. That block previously ran for a coalesced duplicate too, and a duplicate passes its *own* `timeoutMillis`, which says nothing about whether the shared ask is done. So a duplicate timing out first could forget the mapping and lapse an ask the fresh owner was still legitimately holding — the same class of bug as this ticket. The `finally` block and the `NO_WAITER` branch were both already gated this way; the timeout catch was the only one that was not. The worker said plainly that no existing test hit that edge. **Mutation GG**: I removed the `ticket.fresh()` gate and kept the new order. Full suite: ``` Tests run: 1371, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` Nothing failed. The gate shipped with the reorder and nothing held it there — a correct fix with no test is one refactor away from being undone. So I wrote that test myself (`b6b88c5`): `aCoalescedDuplicateAskTimingOutLeavesTheFreshOwnersAskOpen`. A fresh ask with a 5000ms timeout, a coalesced duplicate on the same session with 100ms, then an answer on the shared turn. With the gate in place the fresh owner gets `ANSWERED`; with Mutation GG re-applied: ``` MessageServiceTest.aCoalescedDuplicateAskTimingOutLeavesTheFreshOwnersAskOpen:439 a duplicate's timeout must not lapse the ask the fresh owner still holds ==> expected: <ANSWERED> but was: <TIMED_OUT> ``` Its javadoc records the measurement and says what it does not prove — the ordering *inside* the gate, which is the worker's own test's job. **Housekeeping done.** The `answer()` comment naming this issue is rewritten. It no longer says "do not read this guard as complete"; it now says a null `task` there means the ordinary blocking-send case or #329's already-fixed race, and names the test that pins this window shut. **Noted, not touched:** the `NO_WAITER` branch has the same clear-then-close statement order, and the worker traced it as unreachable by a racing `answer()` because the turnId is never disclosed to the primary on that path. I have not re-derived that myself. It is a paper shape with an argument against reachability, which is the right place to leave it. `MessageService` now carries six test-only hooks. Each exists because its race is unreachable through the public API — the same reason each bug was invisible — but six is enough that the next one needs a harder look than "the file already does this". Closing.
ltms closed this issue 2026-09-04 12:13:25 +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#334
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?