Skip to content

fix(rabbitmq): ack only the completed delivery, not every lower tag (GH-3492) - #3675

Closed
jeremydmiller wants to merge 1 commit into
mainfrom
fix/3492-rabbitmq-ack-semantics
Closed

fix(rabbitmq): ack only the completed delivery, not every lower tag (GH-3492)#3675
jeremydmiller wants to merge 1 commit into
mainfrom
fix/3492-rabbitmq-ack-semantics

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Draft — this change is incomplete on its own and must not be merged as-is. Split out of #3672
so the measured RO6 win there is not blocked on it.

The bug being fixed

RabbitMqListener.CompleteAsync calls BasicAckAsync(deliveryTag, multiple: true). In AMQP,
multiple: true means "ack this delivery tag and every lower unacked tag on this channel".

Completions finish out of order under any concurrent listener — BufferedInMemory or Durable
with MaxDegreeOfParallelism above 1. So completing tag 10 silently acks tags 1-9 while those
messages are still in the handler pipeline. If the process crashes in that window, the broker
considers them delivered and they are gone. That is a real at-least-once hole, and it is invisible
in normal operation because the messages usually do complete a moment later.

The one-line fix is multiple: false.

Why this is a draft: the sweep is load-bearing

CI caught what my local baseline did not. The cumulative ack is currently the only thing
settling deliveries that some code paths never acknowledge at all. Removing it makes those leak.

Measured, after a full Wolverine.RabbitMQ.Tests run against a freshly deleted queue:

Build quorum1 after the full suite
origin/main 0 messages
this branch 1 message ready (leaked)

The consequence in the test suite: that leaked message is redelivered into a later run and shows up
as the last MessageSucceeded record in a tracked session, failing
quorum_queue_compliance.will_requeue_and_increment_attempts. The CI trace shows the intruder
arriving 3,010 ms into the test — a scheduled retry from a previous test method — with a different
message id than the one under test.

The consequence in production is worse than a flaky test: leaked deliveries sit unacked until the
channel closes, then get redelivered and reprocessed as duplicates.

Evidence trail

What has to happen before this can merge

Find and settle the leaking path, rather than keeping the sweep to paper over it.

Prime suspect: RabbitMqInteropFriendlyCallback.MoveToErrorsAsync posts a copy of the envelope to
the dead-letter queue and never acks or nacks the original delivery — unlike
RabbitMqChannelCallback.moveToErrorQueueAsync, which does nack. There may be more than one such
path; the dead-letter route with DisableDeadLetterQueueing() (where NativeDeadLetterQueueEnabled
is false) is worth auditing too.

Note the leak does not reproduce when the quorum compliance class runs alone — only under the full
suite — so whatever it is, it is timing or ordering sensitive.

Reproduction

docker exec wolverine-rabbitmq-1 rabbitmqctl delete_queue quorum1
dotnet test src/Transports/RabbitMQ/Wolverine.RabbitMQ.Tests/Wolverine.RabbitMQ.Tests.csproj --framework net9.0
docker exec wolverine-rabbitmq-1 rabbitmqctl list_queues name messages messages_ready messages_unacknowledged

quorum1 should end at 0. On this branch it ends at 1.

🤖 Generated with Claude Code

…H-3492)

BasicAckAsync(tag, multiple: true) tells the broker to ack every lower
delivery tag on the channel as well. Completions finish out of order under
any concurrent listener -- Buffered or Durable with MaxDegreeOfParallelism
above 1 -- so completing tag 10 silently acked tags 1-9 while they were still
in the handler pipeline. A crash in that window lost them.

DRAFT: this change is NOT complete on its own. The cumulative sweep turns out
to be load-bearing -- it is currently the only thing settling deliveries that
some paths never acknowledge at all, so flipping it in isolation leaks them.
Measured after a full Wolverine.RabbitMQ.Tests run against a freshly deleted
queue: origin/main leaves 0 messages in quorum1, this build leaves 1. The
leaking settle path has to be found and fixed before this can merge.

Split out of #3672 so the measured RO6 win there is not blocked on it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremydmiller

Copy link
Copy Markdown
Member Author

Both sides of the leak measurement are now complete — full Wolverine.RabbitMQ.Tests run on the same box, same broker, quorum1 deleted before each:

Build Suite result quorum1 after
origin/main 480 passed / 3 failed (9m51s) 0 messages
this branch 483 passed / 3 failed (10m41s) 1 message ready

(The 3 extra tests on this branch are unrelated unit tests that were present when the measurement ran. Both sides had the same 3 pre-existing suite failures, so the leak — not a new test failure — is the differentiator locally.)

This is the reproduction to work against: whatever settle path fails to acknowledge its delivery shows up as exactly one stranded message, and only under the full suite, not when the quorum compliance class runs alone.

@jeremydmiller

Copy link
Copy Markdown
Member Author

Superseded by #3737, which lands the same flip plus the settle fixes that make it safe. See that PR for why this one leaked into quorum1: the genuinely unsettled path turned out to be WorkerQueueMessageConsumer's un-mappable-message branch, not the interop DLQ callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant