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

fleetd#329: fix silent async-ticket bugs in MessageService (F1/F2/F3) #332

Closed
agent wants to merge 0 commits from worker/fleetd-329-11bdbb-16 into main
pull from: worker/fleetd-329-11bdbb-16
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 10:21:43 +02:00
Member
Copy Link
Copy Source

Fixes fleetd#329 (follow-up to #324). All three findings are in fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java.

F2 — the silent sink (fixed first, as the issue asked)

sendAsync's executor task ended with catch (Throwable t) { task.future.completeExceptionally(t); }. finishAsyncTask completes that same future on its first line, so anything thrown afterward hits a completed future — completeExceptionally returns false and does nothing, silently.

Fix (the issue's own candidate mechanism): check the return value of completeExceptionally; when it is false, log.error with the ticket and target.

if (!task.future.completeExceptionally(t)) {
    log.error("async send {} -> {} threw after its ticket was already resolved", ticket, target, t);
}

I did not stop completing the future first — that ordering is what makes finishAsyncTask completing before its cleanup harmless to an already-delivered answer (per the issue's own invariant). I only added the missing visibility for the case where something after that step throws.

Test: anExceptionAfterTheTicketFutureCompletesStillReachesTheLog. There is no reachable production call site left where finishAsyncTask throws after completing the future (fleetd #324 already closed the one that used to, via the turnId != null guard) — every code path that could set Task.turnId non-null before reaching finishAsyncTask's call from sendAsync's own executor also diverts through the QUESTION branch instead, so I could not construct a genuinely reachable repro the way the ticket's own probe (temporarily dropping the #324 guard) did either. I added a dedicated test-only hook, afterFinishAsyncTaskCompleteHookForTest, invoked unconditionally right after task.future.complete(result) inside finishAsyncTask — mirroring the existing finishAsyncTaskRaceHook pattern from #324 — so the test can force an exception into exactly that post-completion window and assert it reaches the log via a ListAppender.

Mutation proof: reverted the catch block to the old bare task.future.completeExceptionally(t);, ran only this test:

org.opentest4j.AssertionFailedError: an exception thrown after the ticket's future already completed must still reach the log, not vanish silently ==> expected: <true> but was: <false>

Restored the fix.

F1 — the stranded async ticket

answer() completed the async ticket via finishAsyncTask(turnId, result), which did its own asyncTasksByTurn.get(turnId) lookup. That second lookup races ask()'s own unlocked timeout cleanup (markAskTimedOut + clearAsyncQuestion(turnId, true)), which can forget turnId (remove it from asyncTasksByTurn, null Task.turnId) between answer()'s own rendezvous.answerAsk(turnId, ...) succeeding and the later finishAsyncTask call. When that happens, answer() still correctly returns REPLIED, but the async ticket's future is never completed — fleet_poll{ticket} stays PENDING forever.

Fix: answer() already looks the Task up once, earlier, at the point it registers asyncTasksByWaiter (to support #282's chained-ask re-association). I reuse that same Task reference for the final completion instead of re-resolving it from turnId a second time:

if (result.outcome() != Outcome.QUESTION && task != null) {
    finishAsyncTask(task, result);
}

Invariant 5 (chained ask, #282): unaffected. The chained-ask case is still told apart purely by result.outcome() == Outcome.QUESTION — a fact about the resolution, not about whether the by-turnId lookup succeeds — so reusing the captured Task reference cannot complete a ticket the chained ask deliberately left open; that guard is untouched.

Removed the now-unused finishAsyncTask(String turnId, Reply result) overload (its only caller was this one).

Test: aReplyRacingAsksTimeoutCleanupStillCompletesTheAsyncTicket, using the forgetTurnForTest seam #324 already merged (per acceptance criterion 4) to fire ask()'s exact production cleanup at the point between "the worker's own ask() call has unblocked" and "the worker's real fleet_reply arrives."

Mutation proof: reverted to the by-turnId lookup (temporarily restored the removed overload too), ran only this test:

org.opentest4j.AssertionFailedError: expected: <DONE> but was: <PENDING>

Restored the fix.

Did fixing F2 change how I tested F1? Yes, in one respect: F2 makes a swallowed exception observable (logged), but F1 itself isn't an exception-throwing bug — it's a silent no-op lookup miss, so nothing throws and F2's fix doesn't by itself surface F1. What actually let me pin F1 with a test (rather than only "read the code") was reusing the forgetTurnForTest seam from #324 to force the exact interleaving deterministically, the same technique #324's own race test used. Without F2 fixed, verifying via a hook-based mutation proof would still work the same way — F2 and F1 are independent in that sense. Where F2 matters is for any future regression in this region that throws instead of silently missing a lookup: before this PR, such a regression could still slip through 74/74 (now 1345+) green tests with zero log output, exactly as measured in the ticket.

F3 — the same double read #324 fixed, still present

reply()'s orphan-recovery path read orphan.turnId twice — once for the null check, once as the asyncTasksByTurn.remove key — the same shape #324 fixed in finishAsyncTask.

Fix: read it once into a local, mirroring #324's fix exactly:

String turnId = orphan.turnId;
if (turnId != null) {
    asyncTasksByTurn.remove(turnId, orphan);
}

Reachability: I did not just assert this by pattern-matching #324. The askAnsweredAsyncTasks(...) candidate reaching this branch (turnId != null, matched via the "answered" arm, not the askTimedOut arm) genuinely can have its turnId field nulled between these two reads: the worker can chain a second fleet_ask (#282) onto the same Task after being answered once (moving Task.turnId to a fresh id, still non-null), and if that second ask times out, ask()'s own unlocked clearAsyncQuestion(turnId, true) nulls the field — landing, in the worst case, in the tiny window between this method's two field reads. This is narrower than F1's race (it needs a chained ask in the middle) but is not a hypothetical: the writer that nulls the field is the same production code path ask() always runs on a real timeout, unconditional on which call site is currently reading the field.

Test: replyToAnOrphanedTaskSurvivesTurnIdGoingNullBetweenItsTwoReads, using a new test-only hook replyOrphanTurnIdRaceHookForTest (same pattern as finishAsyncTaskRaceHook, positioned identically — right after the single local read's null-check passes, before its use as the removal key) to force forgetTurnForTest to null the field in that exact window, deterministically rather than assembling the full chained-ask race.

Mutation proof: reverted to the double read (temporarily repositioning the hook call between the two reads to match), ran only this test:

java.lang.NullPointerException
	at java.base/java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:1566)
	at dev.ltms.fleet.msg.MessageService.reply(MessageService.java:478)

Restored the fix.

Find what else has this shape (reported only, not fixed, per the issue)

  • MessageService.java:1100-1102 (sendAsync) — task.future.whenComplete((reply, ex) -> pushLoop.onTicketTerminal(...)): if onTicketTerminal throws, CompletableFuture swallows a whenComplete callback's own exception internally and never propagates it back to the completer — even more silent than F2's original shape, since it never even reaches sendAsync's own catch (Throwable t).
  • MessageService.java:712-724 (abandon) — after task.future.complete(outcome) succeeds, asyncTasksByTurn.remove(turnId, task) and rendezvous.closeAsk(turnId) run with no surrounding catch; an exception there propagates uncaught out of abandon() even though the ticket's future already holds a valid terminal result.
  • MessageService.java:887-889 (send) and :1057-1059 (answer) — the finally { asyncTasksByWaiter.remove(reply); rendezvous.close(...); } blocks run after the try has already computed a successful Reply to return; if rendezvous.close(...) throws, Java's finally-overrides-return semantics discard that already-good result in favor of the cleanup's exception.

Full suite

cd fleetd && mvn clean install

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

(MessageServiceTest alone: Tests run: 77, Failures: 0, Errors: 0, Skipped: 0 — 74 pre-existing + 3 new.)

Caveats for review

  • Two new test-only hooks were added (afterFinishAsyncTaskCompleteHookForTest for F2, replyOrphanTurnIdRaceHookForTest for F3), both null in production, mirroring the existing finishAsyncTaskRaceHook pattern from #324. F1's test reuses the existing forgetTurnForTest seam per acceptance criterion 4 — no new seam was added for F1.
  • Removed the now-dead finishAsyncTask(String, Reply) overload as part of the F1 fix.
  • fleetd/fleetd.yaml is gitignored and absent from my worktree; not reported on.
  • Out of scope, noted only: the three "same shape" call sites above were left untouched per the issue's instruction.
Fixes fleetd#329 (follow-up to #324). All three findings are in `fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java`. ## F2 — the silent sink (fixed first, as the issue asked) `sendAsync`'s executor task ended with `catch (Throwable t) { task.future.completeExceptionally(t); }`. `finishAsyncTask` completes that same future on its first line, so anything thrown afterward hits a completed future — `completeExceptionally` returns `false` and does nothing, silently. **Fix (the issue's own candidate mechanism):** check the return value of `completeExceptionally`; when it is `false`, `log.error` with the ticket and target. ```java if (!task.future.completeExceptionally(t)) { log.error("async send {} -> {} threw after its ticket was already resolved", ticket, target, t); } ``` I did not stop completing the future first — that ordering is what makes `finishAsyncTask` completing before its cleanup harmless to an already-delivered answer (per the issue's own invariant). I only added the missing visibility for the case where something after that step throws. **Test:** `anExceptionAfterTheTicketFutureCompletesStillReachesTheLog`. There is no reachable *production* call site left where `finishAsyncTask` throws after completing the future (fleetd #324 already closed the one that used to, via the `turnId != null` guard) — every code path that could set `Task.turnId` non-null before reaching `finishAsyncTask`'s call from `sendAsync`'s own executor also diverts through the `QUESTION` branch instead, so I could not construct a genuinely reachable repro the way the ticket's own probe (temporarily dropping the #324 guard) did either. I added a dedicated test-only hook, `afterFinishAsyncTaskCompleteHookForTest`, invoked unconditionally right after `task.future.complete(result)` inside `finishAsyncTask` — mirroring the existing `finishAsyncTaskRaceHook` pattern from #324 — so the test can force an exception into exactly that post-completion window and assert it reaches the log via a `ListAppender`. **Mutation proof:** reverted the catch block to the old bare `task.future.completeExceptionally(t);`, ran only this test: ``` org.opentest4j.AssertionFailedError: an exception thrown after the ticket's future already completed must still reach the log, not vanish silently ==> expected: <true> but was: <false> ``` Restored the fix. ## F1 — the stranded async ticket `answer()` completed the async ticket via `finishAsyncTask(turnId, result)`, which did its **own** `asyncTasksByTurn.get(turnId)` lookup. That second lookup races `ask()`'s own unlocked timeout cleanup (`markAskTimedOut` + `clearAsyncQuestion(turnId, true)`), which can forget `turnId` (remove it from `asyncTasksByTurn`, null `Task.turnId`) between `answer()`'s own `rendezvous.answerAsk(turnId, ...)` succeeding and the later `finishAsyncTask` call. When that happens, `answer()` still correctly returns `REPLIED`, but the async ticket's future is never completed — `fleet_poll{ticket}` stays `PENDING` forever. **Fix:** `answer()` already looks the `Task` up once, earlier, at the point it registers `asyncTasksByWaiter` (to support #282's chained-ask re-association). I reuse that *same* `Task` reference for the final completion instead of re-resolving it from `turnId` a second time: ```java if (result.outcome() != Outcome.QUESTION && task != null) { finishAsyncTask(task, result); } ``` **Invariant 5 (chained ask, #282):** unaffected. The chained-ask case is still told apart purely by `result.outcome() == Outcome.QUESTION` — a fact about the *resolution*, not about whether the by-turnId lookup succeeds — so reusing the captured `Task` reference cannot complete a ticket the chained ask deliberately left open; that guard is untouched. Removed the now-unused `finishAsyncTask(String turnId, Reply result)` overload (its only caller was this one). **Test:** `aReplyRacingAsksTimeoutCleanupStillCompletesTheAsyncTicket`, using the `forgetTurnForTest` seam #324 already merged (per acceptance criterion 4) to fire `ask()`'s exact production cleanup at the point between "the worker's own `ask()` call has unblocked" and "the worker's real `fleet_reply` arrives." **Mutation proof:** reverted to the by-turnId lookup (temporarily restored the removed overload too), ran only this test: ``` org.opentest4j.AssertionFailedError: expected: <DONE> but was: <PENDING> ``` Restored the fix. **Did fixing F2 change how I tested F1?** Yes, in one respect: F2 makes a swallowed exception observable (logged), but F1 itself isn't an exception-throwing bug — it's a silent no-op lookup miss, so nothing throws and F2's fix doesn't by itself surface F1. What actually let me pin F1 with a *test* (rather than only "read the code") was reusing the `forgetTurnForTest` seam from #324 to force the exact interleaving deterministically, the same technique #324's own race test used. Without F2 fixed, verifying via a hook-based mutation proof would still work the same way — F2 and F1 are independent in that sense. Where F2 matters is for any *future* regression in this region that throws instead of silently missing a lookup: before this PR, such a regression could still slip through 74/74 (now 1345+) green tests with zero log output, exactly as measured in the ticket. ## F3 — the same double read #324 fixed, still present `reply()`'s orphan-recovery path read `orphan.turnId` twice — once for the null check, once as the `asyncTasksByTurn.remove` key — the same shape #324 fixed in `finishAsyncTask`. **Fix:** read it once into a local, mirroring #324's fix exactly: ```java String turnId = orphan.turnId; if (turnId != null) { asyncTasksByTurn.remove(turnId, orphan); } ``` **Reachability:** I did not just assert this by pattern-matching #324. The `askAnsweredAsyncTasks(...)` candidate reaching this branch (turnId != null, matched via the "answered" arm, not the `askTimedOut` arm) genuinely can have its `turnId` field nulled between these two reads: the worker can chain a second `fleet_ask` (#282) onto the *same* `Task` after being answered once (moving `Task.turnId` to a fresh id, still non-null), and if *that* second ask times out, `ask()`'s own unlocked `clearAsyncQuestion(turnId, true)` nulls the field — landing, in the worst case, in the tiny window between this method's two field reads. This is narrower than F1's race (it needs a chained ask in the middle) but is not a hypothetical: the writer that nulls the field is the same production code path `ask()` always runs on a real timeout, unconditional on which call site is currently reading the field. **Test:** `replyToAnOrphanedTaskSurvivesTurnIdGoingNullBetweenItsTwoReads`, using a new test-only hook `replyOrphanTurnIdRaceHookForTest` (same pattern as `finishAsyncTaskRaceHook`, positioned identically — right after the single local read's null-check passes, before its use as the removal key) to force `forgetTurnForTest` to null the field in that exact window, deterministically rather than assembling the full chained-ask race. **Mutation proof:** reverted to the double read (temporarily repositioning the hook call between the two reads to match), ran only this test: ``` java.lang.NullPointerException at java.base/java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:1566) at dev.ltms.fleet.msg.MessageService.reply(MessageService.java:478) ``` Restored the fix. ## Find what else has this shape (reported only, not fixed, per the issue) - `MessageService.java:1100-1102` (`sendAsync`) — `task.future.whenComplete((reply, ex) -> pushLoop.onTicketTerminal(...))`: if `onTicketTerminal` throws, `CompletableFuture` swallows a `whenComplete` callback's own exception internally and never propagates it back to the completer — even more silent than F2's original shape, since it never even reaches `sendAsync`'s own `catch (Throwable t)`. - `MessageService.java:712-724` (`abandon`) — after `task.future.complete(outcome)` succeeds, `asyncTasksByTurn.remove(turnId, task)` and `rendezvous.closeAsk(turnId)` run with no surrounding catch; an exception there propagates uncaught out of `abandon()` even though the ticket's future already holds a valid terminal result. - `MessageService.java:887-889` (`send`) and `:1057-1059` (`answer`) — the `finally { asyncTasksByWaiter.remove(reply); rendezvous.close(...); }` blocks run after the `try` has already computed a successful `Reply` to return; if `rendezvous.close(...)` throws, Java's finally-overrides-return semantics discard that already-good result in favor of the cleanup's exception. ## Full suite ``` cd fleetd && mvn clean install ``` ``` Tests run: 1345, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` (`MessageServiceTest` alone: `Tests run: 77, Failures: 0, Errors: 0, Skipped: 0` — 74 pre-existing + 3 new.) ## Caveats for review - Two new test-only hooks were added (`afterFinishAsyncTaskCompleteHookForTest` for F2, `replyOrphanTurnIdRaceHookForTest` for F3), both `null` in production, mirroring the existing `finishAsyncTaskRaceHook` pattern from #324. F1's test reuses the existing `forgetTurnForTest` seam per acceptance criterion 4 — no new seam was added for F1. - Removed the now-dead `finishAsyncTask(String, Reply)` overload as part of the F1 fix. - `fleetd/fleetd.yaml` is gitignored and absent from my worktree; not reported on. - Out of scope, noted only: the three "same shape" call sites above were left untouched per the issue's instruction.
agent added 1 commit 2026-09-04 10:21:43 +02:00
fleetd#329: fix silent async-ticket bugs in MessageService (F1/F2/F3)
CI / contract (pull_request) Successful in 50s
Details
CI / build (pull_request) Failing after 1m28s
Details
ea41bbf6b9 
F2 (sendAsync executor catch): log when completeExceptionally returns
false, so an exception thrown after finishAsyncTask already completed
the ticket's future is no longer silently lost.

F1 (answer()'s stranded async ticket): reuse the Task reference answer()
already looked up before rendezvous.answerAsk(), instead of a second
asyncTasksByTurn lookup by turnId in finishAsyncTask. The second lookup
raced ask()'s unlocked timeout cleanup, which could forget turnId first
and leave the ticket stuck PENDING even though answer() itself returned
REPLIED. The #282 chained-ask guard is unaffected: it is still keyed on
result.outcome() == QUESTION, not on this lookup. Removed the now-unused
finishAsyncTask(String, Reply) overload.

F3 (reply()'s orphan recovery path): read orphan.turnId once instead of
twice, closing the same double-read shape fleetd #324 fixed in
finishAsyncTask.

Each fix has its own test plus a test-only race hook (mirroring #324's
finishAsyncTaskRaceHook) to force the exact interleaving deterministically.
Mutation-tested each fix by reverting it, confirming the real failure
(swallowed exception / PENDING ticket / NullPointerException), then
restoring it.

mvn clean install: Tests run: 1345, Failures: 0, Errors: 0, Skipped: 0,
BUILD SUCCESS.
ltms closed this pull request 2026-09-04 10:35:29 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 50s
Details
CI / build (pull_request) Failing after 1m28s
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#332
Delete Branch "worker/fleetd-329-11bdbb-16"

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?