Ack only the completed delivery, not every lower tag (GH-3706) - #3737
Merged
Conversation
…H-3706) Every ack went out as `BasicAckAsync(tag, multiple: true)`, which tells the broker "and every lower delivery tag on this channel too". Cumulative acks are only correct when completions happen in delivery order, and they do not: out-of-order completion already exists today with `ConsumerDispatchConcurrency` above 1, and it becomes the normal case for the planned native-ack parallel endpoint mode. Acking tag N silently swept up every lower unacked tag, including deliveries whose handlers were still running, and a crash at that moment is silent message loss. This flip was attempted during the GH-3492 perf wave and reverted: it measured throughput-neutral but leaked one message into `quorum1`, because the cumulative sweep was covering for settle paths that never acknowledged a delivery on their own. Those are settled at the source now: - `WorkerQueueMessageConsumer`'s un-mappable-message branch dead lettered the envelope through the receiver and returned **without touching the delivery at all**. This is the genuinely unsettled path. - `RabbitMqInteropFriendlyCallback.MoveToErrorsAsync` posts an enriched copy to the dead letter queue and does not settle. On the normal error-handling path that survived only because `MoveToErrorQueue.ExecuteAsync` calls `lifecycle.CompleteAsync()` immediately afterwards, as does `NoHandlerContinuation` -- so unlike the above it was not actually orphaning deliveries. Settled here anyway so the delivery does not depend on a later step in a different layer for the only thing that ever reclaims it. Both settle with an ACK rather than a nack. The sibling `RabbitMqChannelCallback.moveToErrorQueueAsync` nacks with `requeue: false` precisely so the broker's own `x-dead-letter-exchange` routes the original -- that is native dead lettering and it sends no copy of its own. These two paths have already taken responsibility by writing the message elsewhere, so a nack would either be discarded (`InteropFriendly` removes the DLX argument from the queue) or, under `UseEnhancedDeadLettering` against a `Native`-mode queue that still has a DLX, put a SECOND copy in the dead letter queue. `CanSettle` stays. Its doc comment justified itself partly on cumulative acks making a replayed stale tag catastrophic; per-message acks narrow that blast radius to one wrong message but do not make settling on a replaced channel correct, so the comment is corrected rather than the guard removed. Deliberately NOT resurrected: ack coalescing/batching. It was prototyped and rejected at -10% throughput -- `basic.ack` is a fire-and-forget frame, not an RPC, so batching saves no round trips. Tests assert that a delivery which has been through a dead-letter path leaves the source queue empty *after the channel closes*, which is how the GH-3492 leak manifested (the broker requeues anything still unacked when the channel goes away), and that exactly one copy lands in the dead letter queue rather than two. Covers both the interop-friendly path and Native mode as the control. Docs: an "Acknowledgements are per message" section in the RabbitMQ performance guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 30, 2026
Merged
This was referenced Jul 30, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3706. Supersedes the draft #3675, which stopped at the flip itself.
The change
BasicAckAsync(tag, multiple: true)→multiple: false.Cumulative acks are only correct when completions happen in delivery order, and they do not — out-of-order completion already exists today with
ConsumerDispatchConcurrency > 1, and it becomes the normal case for the planned native-ack parallel endpoint mode. Acking tag N silently swept up every lower unacked tag on the channel, including deliveries whose handlers were still running; a crash at that moment is silent message loss.Why the GH-3492 attempt leaked, and what actually leaked
The previous attempt measured throughput-neutral but leaked one message into
quorum1, because the cumulative sweep was covering for settle paths that never acknowledged a delivery of their own. I audited every use ofRabbitMqEnvelopefor a path where neither ack nor nack fires and found one genuine orphan — plus one that the issue flags which turns out not to be:WorkerQueueMessageConsumer's un-mappable-message branch — dead-letters the envelope through the receiver andreturns without touching the delivery at all. This is the real leak, and it isn't the one the issue names.RabbitMqInteropFriendlyCallback.MoveToErrorsAsync— the issue's candidate. It genuinely doesn't settle, but on the normal error-handling path that survives, becauseMoveToErrorQueue.ExecuteAsynccallslifecycle.CompleteAsync()immediately afterMoveToDeadLetterQueueAsync, andNoHandlerContinuationdoes the same. I verified this empirically — disabling my settle here and re-running the repro test still passed — so the issue's "the cumulative sweep from later acks is what currently reclaims those orphaned deliveries" isn't accurate for this path. Settled anyway, so the delivery doesn't depend on a later step in a different layer for the only thing that ever reclaims it.Both settle with an ack, not a nack. The sibling
RabbitMqChannelCallback.moveToErrorQueueAsyncnacks withrequeue: falseprecisely so the broker's ownx-dead-letter-exchangeroutes the original — that's native dead lettering and it sends no copy of its own. These two paths have already taken responsibility by writing the message elsewhere, so a nack would either be discarded (InteropFriendlyremoves the DLX argument from the queue) or, underUseEnhancedDeadLetteringagainst aNative-mode queue that still has a DLX, put a second copy in the dead letter queue.CanSettlestays. Its doc comment justified itself partly on cumulative acks making a replayed stale tag catastrophic; per-message acks narrow that blast radius to one wrong message but don't make settling on a replaced channel correct, so the comment is corrected rather than the guard removed.Not resurrected: ack coalescing/batching, prototyped and rejected at −10% (ledger row 216) —
basic.ackis a fire-and-forget frame, not an RPC, so batching saves no round trips.Tests
Bug_3706_dead_letter_paths_settle_the_original_deliveryasserts that a delivery which has been through a dead-letter path leaves the source queue empty after the channel closes — the broker requeues anything still unacked when a channel goes away, which is exactly how the GH-3492 leak manifested — and that exactly one copy lands in the dead letter queue rather than two. Covers the interop-friendly path and Native mode as the control.Full local suite (the acceptance criterion on the issue)
Wolverine.RabbitMQ.Tests, 10m26s: 488 passed / 2 failed. Both failures investigated rather than assumed:multi_tenancy_through_virtual_hosts.send_message_to_a_specific_tenant— fails identically on unmodifiedmain(same test, same line 171). Pre-existing; it's a tracked-session condition gap, the response is visiblySentin the activity table before the session gives up.end_to_end.use_direct_exchange_with_binding_key— passes in isolation on bothmainand this branch, and the wholeend_to_endclass passes 20/20 here. Cross-class order dependence, matching the chronic first-round pattern tracked in CIRabbitMQ: three tests fail round one on every run and are masked by the flaky-retry harness #3726.No message leaked into a quorum queue — the specific failure the previous attempt hit.
Full
wolverine.slnxRelease build clean.Docs
New "Acknowledgements are per message" section in the RabbitMQ performance guide.
Unblocks the native-ack parallel endpoint mode issue.
🤖 Generated with Claude Code