Features: a failed cleanup no longer strands the tickets behind it (fleetd #335)
+38
@@ -4344,3 +4344,41 @@ after the member's pane goes anyway.
|
||||
|
||||
The same in-memory `spawnedBy` breaks `clearContext` a different way: it plainly no-ops on a cache
|
||||
miss. Open as fleetd #352, and the consequence is not measured yet.
|
||||
|
||||
---
|
||||
|
||||
## A failed cleanup no longer strands the tickets behind it
|
||||
|
||||
**What it does.** `MessageService.abandon` walks every task it must fail and completes each one. The
|
||||
per-task cleanup after that completion is now inside a `try`/`catch`, and each task's own
|
||||
`future.complete(outcome)` runs **before** the guard. A cleanup that throws costs that one task its
|
||||
bookkeeping and nothing more; the loop still reaches every task behind it. The second half of the
|
||||
same fix wraps `pushLoop.onTicketTerminal` inside `sendAsync`'s `whenComplete` action.
|
||||
|
||||
**On.** Always on (fleetd #335).
|
||||
|
||||
**Why it exists.** `abandon` runs on teardown — a release, or the health monitor's GONE sweep — and
|
||||
nothing comes along later to finish what it misses. One call in that loop reaches a broker:
|
||||
`inbox.publish` puts a recovered reply back, and `AmqpReplyInbox.publish` throws
|
||||
`IllegalStateException` on an unroutable publish, on a confirm timeout, and on an interrupt. An
|
||||
uncaught throw there aborted the loop, so every task after it stayed `PENDING` forever and its lead
|
||||
waited on a ticket that would never resolve. The `whenComplete` half is the same failure with a
|
||||
different cause: the daemon's shutdown hook closes `MessageService` before `ReplyPushLoop`, and
|
||||
`messages.close()` does not cancel a send already in flight, so a ticket completing in that window
|
||||
made the push loop's scheduler throw `RejectedExecutionException` into a discarded stage — no log,
|
||||
no metric, and the push loop never learned the ticket was terminal.
|
||||
|
||||
**One thing to know for maintenance.** A publish failure used to propagate out of `abandon` to its
|
||||
caller. It is now logged at `error` with the ticket, target and turnId, and swallowed. That is
|
||||
deliberate and matches what #293 already does for teardown in `HerdrPeerLauncher`: past the point
|
||||
where the real work is done, a cleanup failure must not mask the steps behind it.
|
||||
|
||||
A third site was reported and is **not** a defect: the `finally` blocks in `send()` and `answer()`
|
||||
call `asyncTasksByWaiter.remove` and `Rendezvous.close`, which is `waiters.remove(session, waiter)`.
|
||||
Neither can throw, so no guard was added. A guard that can never fire is worse than none — it reads
|
||||
as evidence that somebody checked.
|
||||
|
||||
Measured on merge: keeping the `catch` but adding a `break` to it leaves the suite failing at
|
||||
`aPerTaskCleanupFailureDoesNotStrandTheRemainingMatchingTasks` with `expected: <FAILED> but was:
|
||||
<PENDING>`. So the test pins the property that matters — the tasks behind the throwing one still
|
||||
finish — and not merely that no exception escapes.
|
||||
|
||||
Reference in New Issue
Block a user