Race between publish and fanout exchange queue deletion

[16pierre]

Context

We use a broadcast setup, where we register a durable, fanout exchange that delivers to multiple quorum queues (replication = 3 nodes, unsubscribed queue expiry = 2 minutes).
These quorum queues often get added/removed (we use these quorum queues much like temporary queues with persistence guarantees)

We've encountered race-conditions between deleting a queue (due to expiry) and message publish (with publish acknowledgements requested).
Symptoms are either:

• published message gets unacked by broker, partial delivery to other queues is performed
• broker encounters internal error (details below), resets the publisher channel. Partial delivery to other queues is performed

In our case, ideally we'd want to gracefully ACK the publish if one of the queues fails to deliver a message because it's being deleted.

Note that here deletion is caused by expiry, but maybe the problem exists for explicit deletions as well, but we haven't encountered the problem in production yet, probably because for these we manually delete bindings before deleting queues.

Bug details

RabbitMQ version: 3.12.5

The exception thrown upon internal errors looks like this:

  crasher:
    initial call: rabbit_channel:init/1
    pid: <0.12471.4015>
    registered_name: []
    exception exit: {{case_clause,
                      {error,
                       {{shutdown,delete},
                        {gen_statem,call,
                         [{'REDACTED_QUEUE_NAME',
                           'rabbit@rabbitmq-cluster-server-1.rabbitmq-cluster-nodes.production'},
                          {leader_call,
                           {command,normal,
                            {'$usr',
                             {register_enqueuer,<0.12471.4015>},
                             await_consensus}}},
                          65000]}}}},
                     [{rabbit_channel,deliver_to_queues,2,
                       [{file,"rabbit_channel.erl"},{line,2169}]},
                      {rabbit_channel,handle_method,3,
                       [{file,"rabbit_channel.erl"},{line,1282}]},
                      {rabbit_channel,handle_cast,2,
                       [{file,"rabbit_channel.erl"},{line,633}]},
                      {gen_server2,handle_msg,2,
                       [{file,"gen_server2.erl"},{line,1056}]},
                      {proc_lib,wake_up,3,
                       [{file,"proc_lib.erl"},{line,250}]}]}
      in function  gen_server2:terminate/3 (gen_server2.erl, line 1172)

Right before this crash log, we get Raft logs indicating queue deletion:

queue 'REDACTED_QUEUE_NAME' in vhost 'REDACTED_VHOST': leader -> terminating_leader in term: 1 machine version: 3
queue 'REDACTED_QUEUE_NAME' in vhost 'REDACTED_VHOST': terminating with reason 'delete'

And then the crash. Note that the crash can happen both on Raft followers/leader.

I suspect that there's a short timespan between the raft cluster deletion and the bindings removal propagation, which could leave a window where a published message gets routed to a shut-down quorum queue.

I tried to dive into rabbit-server's source code, was typically wondering if bindings are updated after raft cluster deletion:

rabbitmq-server/deps/rabbit/src/rabbit_quorum_queue.erl

Lines 719 to 733 in 10fb936

	case ra:delete_cluster(Servers, Timeout) of
	{ok, {_, LeaderNode} = Leader} ->
	MRef = erlang:monitor(process, Leader),
	receive
	{'DOWN', MRef, process, _, _} ->
	ok
	after Timeout ->
	erlang:demonitor(MRef, [flush]),
	ok = force_delete_queue(Servers)
	end,
	notify_decorators(QName, shutdown),
	ok = delete_queue_data(Q, ActingUser),
	_ = erpc:call(LeaderNode, rabbit_core_metrics, queue_deleted, [QName],
	?RPC_TIMEOUT),
	{ok, ReadyMsgs};

here, delete_queue_data happens after ra:delete_cluster, but I couldn't confirm how the rabbit_quorum_queue:delete method gets called by the expiry_timer codepath...

Repro

Haven't had time to build a repro script yet.

Assuming this problems exists for manual queue deletion and not just expiry, reproduction steps should look like this:

• setup a 3-nodes broker
• create a durable fanout exchange
• register a bunch of quorum queues
• trigger both a publish (with publisher acks requested) & a queue deletion at the same time

[michaelklishin]

It looks like a missing clause in rabbit_channel:deliver_to_queues/3.

Note that any change of rabbit_channel: deliver_to_queues/2 will have to go through a lot of scrutiny because any regression there affects every single user, their throughput and so on.

[16pierre]

Hey @michaelklishin, thanks for the response.
I reckon I don't feel 100% confident writing the fix myself as an Erlang noobie, would it be possible to get some help on this one ?

[gomoripeti]

It looks like the raft library had an improvement in ra 2.7.0 rabbitmq/ra#388 which is included in RabbitMQ 3.13.0. I'm almost certain it fixes this issue.

[16pierre]

Ah thanks, this looks helpful, I'm guessing this should fix the broker internal errors we experienced.

Not sure if this fixes the other part of our problem here:

Symptoms are either
published message gets unacked by broker, partial delivery to other queues is performed
broker encounters internal error (details below), resets the publisher channel. Partial delivery to other queues is performed

Even with the ra fix https://github.com/rabbitmq/ra/pull/388/files, the first symptom still applies, doesn't it ? i.e. deliver_to_queues will catch error upon trying to deliver to the queue that's shutting down, and will up sending an unack to publisher ? (this needs to be sanity-checked, I'm not a reliable Erlang reader)

[gomoripeti]

hm, you are right, I did not read the report carefully. The publish will be still unacked on 3.13 (the same way as if a quorum queue would not be available eg. because majority of members is down)

(deleting expired QQs gets triggered here

rabbitmq-server/deps/rabbit/src/rabbit_fifo.erl

Line 900 in 10fb936

[{mod_call, rabbit_quorum_queue, spawn_deleter, [QName]}];

)

The idea to delete bindings before the queue makes sense to me but it is in such a central and old part of the RabbitMQ code that I wouldn't dare to touch.

Maybe the logic behind the current order to first tear down the processes and resource before removing the definitions is that if the former fails the processes can still be recreated from the definitions (and deletion can be retried)

[16pierre]

The idea to delete bindings before the queue makes sense to me but it is in such a central and old part of the RabbitMQ code that I wouldn't dare to touch.

Yeah I can see how this could be scary.

To be fair, I'm not even sure that swapping the order would fully solve the problem. I'm not an expert on Erlang/RMQ threading model, but assuming that 2 threads run in parallel for these:

1. thread_1, timestamp 0: publish starts, gets initial binding contained Q_1
2. thread_2, timestamp 1: queue deletion is requested, binding gets updated, queue gets removed (no matter the order in which takes place)
3. thread_1, timestamp 2: publish iterates over the bindings to send message to the queues. The bindings were fetched at t=0, and contain the deleted entry Q_1, we hit an error on Q_1.

An alternative fix would be to special-case deliver_to_queues to catch the error returned by queues upon deletion ({shutdown,delete} ?), and consider the delivery successful in this specific error case. This looks less dangerous, much less risks of side-effects

[gomoripeti]

you are absolutely right.

I was thinking about your other suggestion, but there are quite a few layers of abstractions involved so it is not obvious to me. (eg after the change in rabbitmq/ra#388 the Raft layer only returns {error, shutdown}, but what cases are there when a raft/QQ member shuts down? (could be the queue is deleted or just one member is removed) I don't think all of them should be handled the same)

[16pierre]

Yeah ideally we'd want to propagate and handle a specific {error, shutdown, delete} status or something of the sort coming from Raft, which disambiguates the too-generic{error, shutdown}