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

MessageService: three more places a thrown exception has nowhere to go #335

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

Spotted by the #329 worker while fixing F1/F2/F3, outside that scope. I have re-read all three on
main at 4aa1fae and confirmed the code shapes below. Line numbers here are mine, not the
worker's — its numbers predate two commits.

What I did NOT do: prove any of these is reachable. Each needs a collaborator to throw. Before
fixing any of them, name the path that makes the throw happen — see the "a defect on paper is not a
reachable defect" rule. Rank by that, not by how bad the shape looks.

1. abandon's loop has no guard around its cleanup — the strongest of the three

MessageService.java:708-733. The loop walks matching and completes every task:

for (Task task : matching) {
    ...
    if (task.future.complete(outcome)) {
        ...
        if (turnId != null) {
            asyncTasksByTurn.remove(turnId, task);
            rendezvous.closeAsk(turnId);      // <- no try/catch
        }
    } else if (isRecovery) {
        inbox.publish(target, UUID.randomUUID().toString(), recovered.text());  // <- no try/catch
    }
}

If rendezvous.closeAsk or inbox.publish throws, the loop aborts. Every task after the
throwing one is never completed, so it sits at PENDING forever — on the teardown path, where
nothing will come along later to finish it. That is the same stranded-ticket class as #329 and #334,
reached a different way.

Candidate: wrap the per-task body so one task's cleanup failure cannot strand its siblings, and log
what threw.

2. whenComplete discards what its action throws

MessageService.java:1113-1116:

task.future.whenComplete((reply, ex) -> {
    boolean failed = ex != null || reply == null || !reply.completed();
    pushLoop.onTicketTerminal(ticket, target, failed);
});

The stage returned by whenComplete is dropped. If pushLoop.onTicketTerminal throws, nothing sees
it: no log, no failure. The push loop then silently never learns this ticket went terminal, which is
the exact thing CB-107 added this hook for.

This is the same shape as #329's F2 — a failure reported into something nobody reads. Candidate: a
try/catch (Throwable) inside the action with a log.error.

3. A throw from a finally replaces the real result

Two sites, same shape:

  • send() — MessageService.java:887-890
  • answer() — MessageService.java:1070-1073
} finally {
    asyncTasksByWaiter.remove(reply);
    rendezvous.close(target, reply);
}

If rendezvous.close throws, that exception replaces whatever the try was returning or throwing.
A good Reply is lost, or a real diagnostic exception is swapped for a cleanup one. This is the
weakest of the three: remove and close are both cheap map operations today, so the throw may be
unreachable. Check that before changing anything — and if it is unreachable, say so on this issue
and close it rather than adding a catch that can never fire.

Related: #329 (the three fixed), #334 (the gap F1 leaves open).

Spotted by the #329 worker while fixing F1/F2/F3, outside that scope. I have re-read all three on `main` at `4aa1fae` and confirmed the code shapes below. Line numbers here are mine, not the worker's — its numbers predate two commits. **What I did NOT do: prove any of these is reachable.** Each needs a collaborator to throw. Before fixing any of them, name the path that makes the throw happen — see the "a defect on paper is not a reachable defect" rule. Rank by that, not by how bad the shape looks. ## 1. `abandon`'s loop has no guard around its cleanup — the strongest of the three `MessageService.java:708-733`. The loop walks `matching` and completes every task: ```java for (Task task : matching) { ... if (task.future.complete(outcome)) { ... if (turnId != null) { asyncTasksByTurn.remove(turnId, task); rendezvous.closeAsk(turnId); // <- no try/catch } } else if (isRecovery) { inbox.publish(target, UUID.randomUUID().toString(), recovered.text()); // <- no try/catch } } ``` If `rendezvous.closeAsk` or `inbox.publish` throws, the loop aborts. Every task **after** the throwing one is never completed, so it sits at `PENDING` forever — on the teardown path, where nothing will come along later to finish it. That is the same stranded-ticket class as #329 and #334, reached a different way. Candidate: wrap the per-task body so one task's cleanup failure cannot strand its siblings, and log what threw. ## 2. `whenComplete` discards what its action throws `MessageService.java:1113-1116`: ```java task.future.whenComplete((reply, ex) -> { boolean failed = ex != null || reply == null || !reply.completed(); pushLoop.onTicketTerminal(ticket, target, failed); }); ``` The stage returned by `whenComplete` is dropped. If `pushLoop.onTicketTerminal` throws, nothing sees it: no log, no failure. The push loop then silently never learns this ticket went terminal, which is the exact thing CB-107 added this hook for. This is the same shape as #329's F2 — a failure reported into something nobody reads. Candidate: a `try`/`catch (Throwable)` inside the action with a `log.error`. ## 3. A throw from a `finally` replaces the real result Two sites, same shape: - `send()` — `MessageService.java:887-890` - `answer()` — `MessageService.java:1070-1073` ```java } finally { asyncTasksByWaiter.remove(reply); rendezvous.close(target, reply); } ``` If `rendezvous.close` throws, that exception replaces whatever the `try` was returning or throwing. A good `Reply` is lost, or a real diagnostic exception is swapped for a cleanup one. This is the weakest of the three: `remove` and `close` are both cheap map operations today, so the throw may be unreachable. Check that before changing anything — and if it is unreachable, say so on this issue and close it rather than adding a `catch` that can never fire. Related: #329 (the three fixed), #334 (the gap F1 leaves open).
ltms commented 2026-09-04 11:49:18 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 65a7893 (--no-ff). The branch was behind main — it missed #342 and #345 — so this
was a real merge. main is green at Tests run: 1367, Failures: 0, Errors: 0, Skipped: 0.

The ticket asked for reachability first and got it. Three different verdicts, which is the right
outcome for three sites I ranked by shape rather than by evidence.

Site 1 — reachable, fixed. inbox.publish in the put-back branch reaches a broker. I checked
AmqpReplyInbox.publish myself rather than take the report: it throws IllegalStateException on an
IOException from basicPublish (:359), on a confirm timeout (:368) and on an interrupt (:372).
So the abort is real, and everything after the throwing task in matching stayed PENDING forever
on a teardown path.

The fix is better than the one I suggested. Instead of only wrapping the body, it separates the two
things: task.future.complete(outcome) and the asyncFailed bookkeeping now run before the
try, so every task is owed its own outcome whatever the cleanup does. The catch then loses only
that one task's bookkeeping, logged at error. My "wrap the per-task body" wording would have left
the completion inside the guard.

Site 2 — reachable, fixed, and it reproduces without a hook. The worker found the real trigger,
which I had not: Fleetd's shutdown hook runs messages.close() before pushLoop.close(), and
messages.close() only stops the executor taking new work — it does not cancel a send already in
flight. A ticket completing in that window makes onTicketTerminal's own scheduler.schedule throw
RejectedExecutionException, and the discarded stage swallowed it.

Site 3 — not a defect, and I agree. I verified both calls myself. Rendezvous.close is
waiters.remove(session, waiter) at Rendezvous.java:107-109, and asyncTasksByWaiter is a
ConcurrentHashMap. Neither can throw. No catch was added, which is what this ticket asked for —
a guard that can never fire is worse than none. That half is closed as not-a-defect.

My own mutation on merge — Mutation DD, aimed at what the worker's mutation could not reach.
The worker proved each fix by deleting its try/catch, which shows the exception escapes. It does
not show the loop keeps going — a version that catches, logs, and then stops would pass that test
just as well. So I kept the catch and added a break to it. Full suite, unpiped:

MessageServiceTest.aPerTaskCleanupFailureDoesNotStrandTheRemainingMatchingTasks:841
  ->assertFailedTicket:1835->awaitTicketPhase:1849
  expected: <FAILED> but was: <PENDING>
Tests run: 1367, Failures: 1, Errors: 0, Skipped: 0
BUILD FAILURE

Caught. The test really asserts the tasks behind the throwing one still reach FAILED, which is the
whole point of the ticket and not merely that nothing escapes. Restored, git diff --stat on src/
empty.

One consequence worth writing down. A publish failure used to propagate out of abandon to its
caller — the release path, or the health monitor's sweep. It is now logged and swallowed. That is
the deliberate trade this ticket asked for, and it matches what #293 already does for teardown in
HerdrPeerLauncher: past the point where the work is done, a cleanup failure must not mask the
steps behind it. The log.error carries the ticket, target and turnId.

MessageService now carries five test-only hooks. That is worth watching, though each one exists
because the race it drives is unreachable through the public API — which is the same reason the bug
was invisible.

Closing.

Merged as `65a7893` (`--no-ff`). The branch was **behind main** — it missed #342 and #345 — so this was a real merge. `main` is green at `Tests run: 1367, Failures: 0, Errors: 0, Skipped: 0`. The ticket asked for reachability first and got it. Three different verdicts, which is the right outcome for three sites I ranked by shape rather than by evidence. **Site 1 — reachable, fixed.** `inbox.publish` in the put-back branch reaches a broker. I checked `AmqpReplyInbox.publish` myself rather than take the report: it throws `IllegalStateException` on an `IOException` from `basicPublish` (:359), on a confirm timeout (:368) and on an interrupt (:372). So the abort is real, and everything after the throwing task in `matching` stayed `PENDING` forever on a teardown path. The fix is better than the one I suggested. Instead of only wrapping the body, it separates the two things: `task.future.complete(outcome)` and the `asyncFailed` bookkeeping now run **before** the `try`, so every task is owed its own outcome whatever the cleanup does. The `catch` then loses only that one task's bookkeeping, logged at `error`. My "wrap the per-task body" wording would have left the completion inside the guard. **Site 2 — reachable, fixed, and it reproduces without a hook.** The worker found the real trigger, which I had not: `Fleetd`'s shutdown hook runs `messages.close()` before `pushLoop.close()`, and `messages.close()` only stops the executor taking *new* work — it does not cancel a send already in flight. A ticket completing in that window makes `onTicketTerminal`'s own `scheduler.schedule` throw `RejectedExecutionException`, and the discarded stage swallowed it. **Site 3 — not a defect, and I agree.** I verified both calls myself. `Rendezvous.close` is `waiters.remove(session, waiter)` at `Rendezvous.java:107-109`, and `asyncTasksByWaiter` is a `ConcurrentHashMap`. Neither can throw. No `catch` was added, which is what this ticket asked for — a guard that can never fire is worse than none. That half is closed as not-a-defect. **My own mutation on merge — Mutation DD, aimed at what the worker's mutation could not reach.** The worker proved each fix by deleting its `try`/`catch`, which shows the exception escapes. It does not show the *loop keeps going* — a version that catches, logs, and then stops would pass that test just as well. So I kept the `catch` and added a `break` to it. Full suite, unpiped: ``` MessageServiceTest.aPerTaskCleanupFailureDoesNotStrandTheRemainingMatchingTasks:841 ->assertFailedTicket:1835->awaitTicketPhase:1849 expected: <FAILED> but was: <PENDING> Tests run: 1367, Failures: 1, Errors: 0, Skipped: 0 BUILD FAILURE ``` Caught. The test really asserts the tasks behind the throwing one still reach `FAILED`, which is the whole point of the ticket and not merely that nothing escapes. Restored, `git diff --stat` on `src/` empty. **One consequence worth writing down.** A publish failure used to propagate out of `abandon` to its caller — the release path, or the health monitor's sweep. It is now logged and swallowed. That is the deliberate trade this ticket asked for, and it matches what #293 already does for teardown in `HerdrPeerLauncher`: past the point where the work is done, a cleanup failure must not mask the steps behind it. The `log.error` carries the ticket, target and turnId. `MessageService` now carries five test-only hooks. That is worth watching, though each one exists because the race it drives is unreachable through the public API — which is the same reason the bug was invisible. Closing.
ltms closed this issue 2026-09-04 11:49:24 +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#335
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?