Quorum queue ignores some received acknowledgments when channel is closed

[grujicd]

Describe the bug

When manually acknowledging messages received from a quorum queue, some acks sent immediately before the channel is closed are sometimes ignored by the RabbitMQ server - these messages will be returned to the queue instead of removed. I could not reproduce this on non-quorum queues.

In my test code I'm first fetching all messages using basic.get, and sending all acks afterwards. I know this is not optimal usage, we could issue single ack with multiple flag, or perform ack after each get, etc. However, scenario which lead to this discovery was more complex with mix of acks and nacks. This way simply shows the issue more easily. AFAIK .Net client blocks on each ack call, so we should not proceed to channel close before server confirmed all these acks.

I have a sample .Net code which demonstrates this issue https://github.com/grujicd/RabbitMQAckProblemTest
This program runs in a loop until issue is detected since it doesn't always show up immediatelly. You should call it with the name of quorum queue and a number of messages that will be sent/acked. Please note that queue will be purged!

dotnet run quorum 100

The workaround is to add some delay between last ack and channel close. Thread.Sleep(200) solves the problem in my setup.

Reproduction steps

1. BasicGet number of messages, without acknowledging them
2. Send BasicAck for each of these messages using delivery tags
3. Close channel

Expected behavior

All of acked messages should be removed from the quorum queue. However, sometimes some messages are returned to the queue.

Additional context

RabbitMQ 3.12.4 on Windows. RabbitMQ 3.12.10 on MacOS. RabbitMQ .Net client 6.8.1 (the latest one official).

[michaelklishin]

@grujicd your code creates a race condition between confirmations and channel closure. Depending on the timing of these events, the ack may or may not get to the node, the channel or eventually the queue leader.

You are positively acknowledging the delivery, which means

• If the ack does get a chance of being fully processed before the channel is closed, the messages will be marked for deletion (deleted as observed by consumers)
• If it does not, the unacknowledged delivery will be automatically requeued

As with all race conditions, you have to run this enough times to observe both behaviors.

I do not see any evidence of a bug, and I see a very limited number of options for this potentially can be avoided, all potentially having negative effect on throughput with an unclear benefit.

[michaelklishin]

To clarify: RabbitMQ channels will begin processing the basic.ack frame before basic.close, this is guaranteed.

What is not guaranteed is that the channel can finish the first step before it is shut down by the connection, as the application instructs it to do. It does not coordinate received but pending ack processing with either its connection or the target queue, and making it coordinate with either or both is guaranteed to result in a throughput hit and suboptimal CPU utilization for no gain for the absolute majority of users.

Use long lived channels, as the docs explicitly recommend, and you will avoid this problem.

[grujicd]

Thanks for a quick reply. Dot Net client exposes basic.ack as a blocking call. I assumed once we get a response from the server that ack is 100% guaranteed on the server side. Based on your reply it's not the case? If so, there's no way on the client side to know whether these acks are completed on the server. I didn't find it documented on https://www.rabbitmq.com/confirms.html nor in .Net client docs.

Although my sample code is deliberately made to expose this issue, there's a non-zero possibility that this could happen in real usage whenever the channel is closed immediately after ack.

[michaelklishin]

There is no response for basic.ack in the protocol. It's just how historically the .NET client API has been structured, and for 7.x and 8.x, there is a lot of ongoing work to make it "as asynchronous as possible".

Some operations in the protocol are indeed blocking in that they do have a response; basic.ack, basic.nack and basic.reject are not in this group, and strictly speaking neither is basic.publish (for efficiency reasons), although with publisher confirms this statement can be challenged (a response does exist but it is delivered and generally meant to be handled asynchronously), of course.

[michaelklishin]

Not only using basic.get this way is inefficient, using basic.get (polling) in any way is very inefficient. On top of that, your workload explicitly creates channel churn.

To summarize:

• This is a behavior your workload creates, and a workload RabbitMQ connections, channels and queues do not optimize for (in fact, they optimize for the opposite case)
• Your workload create channel churn, something we explicitly recommend against
• Your workload uses basic.get, which we highly recommend against

This is not a bug in RabbitMQ, and I'm afraid this scenario, if I understand it correctly, is not something our team would be willing spend much time on.

[grujicd]

My use case is not typical and I understand that RabbitMQ is not optimized for that. I'm the author of QueueExplorer, management too which works with RabbitMQ among other queueing systems. So it's not an app that constantly consumes messages, but "opens" a queue from time to time, shows queued messages to the user, etc. User then click on another queue, etc. So closing channels and connections is a given in this scenario since we're not constantly processing the queue. This is typically used during development/debugging, or for handling deadletter messages, requeing some of them, deleting poison messages etc - legal use cases which are not very well covered by RabbitMQ management portal.

I know that polling is inefficient but we have to use it because when using consume/subscribe way we don't know when the end of queue is reached. And as I explained we don't want to wait for new messages and process them, we need to display what's currently in the queue. Btw. we do use consume when user turns on "exclusive mode" since we can rely on message count to know when to stop, and it is indeed much faster.

What we could really use are few new operations: peek (for browsing message), and delete/reject message based on some id, without having to receive all messages in the front first. This was all available in ancient MSMQ, but to support RabbitMQ we had to "abuse" API in a way it's not meant to be used, but that's the only way to provide what our users need.

Sorry for diverting this discussion to other topics, I just wanted to explain that I'm aware of your points but we're "forced" to use suboptimal parts of API.

[michaelklishin]

Someone on our team suggests that this is important to mention: the above limitation means that consumers sometimes can get redeliveries (which will be explicitly marked as such in delivery properties).

This is true in other scenarios: if a proper consumer (and not a polling loop) with a long lived channel loses its TCP connection to the node, it will get redeliveries and will have to be ready to deal with them.

This is just one specific scenario or that, where the client itself initiates channel closure with pending consumer acknowledgements. In this specific case, with some adjustments all received but pending acks on the node could be processed by a queue as long as the queue replica or the entire node do not fail. But it will involve a lot of work and risks for a very specific and relatively unlikely scenario, and would by no means free consumer implementations from having to have an answer to the question "what do we do with redeliveries?"

[grujicd]

We do have a workaround for our use case, and that's a simple Thread.Sleep before channel close. I'm not suggesting that it should be changed in the server - you're right it shouldn't affect majority of users. But it wouldn't hurt if this potential issue (acks are not guaranteed to be completed if channel/connection is closed) is mentioned in official documentation.

[kjnilsson]

you could try to perform a synchronous command such as a passive queue declare before terminating the channel, this will give any acks sent prior a better chance of actually making it to the broker.

[michaelklishin]

I have added a note to the channel lifecycle section in the doc guide on channels. Should be live in a few minutes.