Fundamentally broken recovery and stale delivery tags guard

[tie]

Summary

Currently Bunny has a fundamental design flaw that causes applications to lose consumers on channel error.

Details

Third-party client library amqp-client.rb has abstractions that correspond to the following entities in Bunny:

• AMQP::Client → Bunny::Session
• AMQP::Client::Connection → Bunny::Transport
• AMQP::Client::Connection::Channel → Bunny::Channel

As the naming suggests, the architecture is straightforward design:

erDiagram
    Client ||--o| Connection : manages
    Client ||..o{ Consumer : recovers
    Connection ||--o{ Channel : creates
    Channel ||--o{ Consumer : subscribes
    Channel ||..|| Connection : "sends data"

Loading

It is possible to create channels and consumers using Client instance, in which case topology is restored on reconnection, like with Bunny’s Session.

Bunny has similar design, however, Transport (i.e. Connection) is encapsulated in Session (i.e. Client), and Channels use Sessions as a Connection handle.

erDiagram
    Session ||--o| Transport : manages
    Session ||--o{ Channel : creates
    Channel ||--o{ Consumer : subscribes
    Channel ||..|| Session : "sends data"
    Channel ||..o{ Consumer : recovers

Loading

This leads to the cursed Bunny::VersionedDeliveryTag workaround along with recoveries_counter attribute on the Channel. Note that Channel is not aware of the current Transport and uses Session to send frames.

Important

If there is a reconnection when consumer is processing a delivery, there is a time interval when Session has a new Transport but has not notified Channel about the successful recovery. If the consumer acknowledges the delivery, it would send frame on a new connection with a delivery tag that does not exist.

And I’m not even touching the fact that there are a lot of other bugs like getting recoveries counter after submitting a delivery to the worker pool (the connection could have changed between receiving a delivery and starting the processing).

bunny/lib/bunny/channel.rb

Lines 1854 to 1856 in 6cb314c

	@work_pool.submit do
	begin
	consumer.call(DeliveryInfo.new(basic_deliver, consumer, self), MessageProperties.new(properties), content)

There could be other cases where this issue manifests, but in case of delivery acknowledgements, this usually leads to errors like

Session#handle_frame on 1: #<AMQ::Protocol::Channel::Close:0x00007f2ae22462d8 @reply_code=406, @reply_text="PRECONDITION_FAILED - unknown delivery tag 3", @class_id=60, @method_id=80>
Channel#handle_frame on channel 1: #<AMQ::Protocol::Channel::Close:0x00007f2ae22462d8 @reply_code=406, @reply_text="PRECONDITION_FAILED - unknown delivery tag 3", @class_id=60, @method_id=80>

Note that in this case PRECONDITION_FAILED is an unrecoverable error that leads to a channel being closed from server side. Bunny updates the internal state and removes the channel and we lose consumers. This behavior seems to be correct—a bug in the client, sure, just fix that in the code. Except that it’s Bunny that has this bug, so there is no easy workaround, and it shouldn’t be sending data intended for a different connection.

Expected behavior

It should not be possible to send invalid data over the wire under normal circumstances (e.g. unless intentionally sending an explicitly set delivery tag).

Workarounds

None. Perhaps an adapter over amqp-client.rb that is compatible with Bunny API? 😅
I’m not aware of any projects that implement that.

A potential solution would be to move recoveries counter to the Transport and also pass it to send_frame from Channel to guard against unknown delivery tags. I believe this would be non-trivial to implement and extremely hard to maintain since there a lot of edge cases to consider. It also does not address this design flaw that could be causing other issues.

Alternatively, while being a larger change, Bunny should use a proper, clean architecture like amqp-client.rb that does not have such fundamental bugs when interacting with AMQP. I think it should be possible to implement in a backwards-compatible manner.

[michaelklishin]

@tie if you believe that a couple of diagrams, strong opinions and an aggressive attitude towards the maintainer is what leads to change in open source software, you are dead wrong.

Is Bunny's Design Really Flawed? Let's See What Its History Suggests

The recovery mechanism Bunny has worked decently for about twelve years now. Twelve years is a long enough period to conclude that the design is reasonable. Moreover, exactly the same design has been in place in several other RabbitMQ clients for almost as long, in some cases replacing other connection recovery strategies (the Java client one has replaced what Spring AMQP had developed independently around 2010-2011).

That's plenty of evidence that directly contradicts your bold claims that the design is "fatally flawed".

How Did the Versioned Delivery Tags Idea Come to Be?

The versioned delivery tag aspect was added by me and Mathias Radestock, one of the founders of RabbitMQ, to resolve one specific problem: when a connection is closed, a consumer might
still have a long delivery it has to process, and, after connection recovery, try to send
a basic.ack that would result in a channel exception because a connection recovery cycle
by definition re-creates all channels and the new replacements won't have the same consumer acknowledgement state.

We knew that it was a double-edged sword and decided to try it even then. It worked reasonably well and we've kept it. It was the source of very few complaints over modern Bunny's 12-13 year long history. Compared to the publisher-side data safety, it's effectively been a non-issue.

Yes, it can result in an "incorrect" basic.ack delivery tag being sent. Just like a design
where versioned tags do not exist and a delivery acknowledgement happens mid-recovery.

Why Does Bunny::Transport Exist?

Bunny::Transport exists because Bunny originally supported JRuby (before March Hare was a thing), and sockets in JRuby back then, and even today, have enough of a difference with MRI to
warrant extracting the part that deals with socket I/O from Bunny::Session.

See those

is defined?(JRUBY_VERSION)

lines in Bunny::Transport, they matter more than you realize.

Bunny::Transport reports all I/O errors it encounters to Bunny::Session#handle_network_failure,
and I don't see how folding it into Bunny::Session would improve anything fundamentally.

In fact, I don't see how Bunny::Transport is even distantly related to the versioned tags idea.
Versioned tags would work equally well or poorly even if Bunny::Transport did not exist.

What Might Actually Help?

In the Java client, consumers are objects (and in Bunny they are usually callables/blocks
and not Bunny::Consumer objects). They have an interface spanning N methods that represents
a consumer lifecycle, from basic.consume-ok that can be handled for parts of consumer state initialization, to various error handler.

That means that a connection recovery cycle can notify a consumer telling it that it is being
shut down, and then notify it again right after channel recovery. That gives applications
a chance to design their consumers in a way that would delay all acknowledgements.

If consumers are not designed that way, then the Java client, Bunny, the .NET client and most other AMQP 0-9-1 clients would be equally prone to the fundamental problem that versioned consumer tags seek to address.

Bunny does not have anything like that because it's "improper" API uses blocks for consumers,
just the way Bunny from 2009 and the amqp gem from 2008 were designed. No Ruby user has ever
complained about that part, so we've kept it.

Therefore, removing Bunny::Queue#subscribe as an option and leaving only Bunny::Queue#subscribe_with that accepts an object would potentially make a big difference
in consumer safety, fundamentally. But I'm pretty sure 99% of Bunny users won't upgrade to such a version. Because connection recovery works decently for them and reworking every consumer they might have in every application would be a non-starter.

Bunny's Role Amongst the Modern RabbitMQ Clients

amqp-client.rb was developed many years after Bunny and several other RabbitMQ clients
were mature and widely used, so of course it could learn a thing or two.

That does not make Bunny's design "unclean", "improper" and "flawed".

Bunny's current design, originating from late 2012 and co-developed with the founder of RabbitMQ, has not only stood the test of time, it has also influenced the RabbitMQ Java and .NET clients (both of which use the same fundamental connection recovery idea), and via those two, the majority of RabbitMQ clients out there.

Conclusion

I wouldn't be against removing the version tagging part of Bunny's connection recovery code, it
surprises me that it does work decently in practice. It's a complex feature that is borderline "too smart" and is the only element in Bunny's connection recovery design that I'm willing to remove.
The rest works well and I have never seen a fundamentally different approach to connection recovery in AMQP 0-9-1 clients.

But I'm not going to do it in a response to insults, a couple of images that demonstrate how "clean design" is different from an "improper" one in just one element on a diagram, in particular from someone who spews strong opinions having contributed nothing to the RabbitMQ ecosystem.

From where does all confidence stem from, @tie? You clearly have no idea of the history of any of these projects, features, and why certain things work the way they do.

So go use amqp-client.rb, with that proper, clean architecture that virtually no one uses. And perhaps learn a thing or two about how to convince people and drive meaningful change in mature projects.

And say hi to its maintainers, who have contributed very little to RabbitMQ despite making money directly off of it, and making claims that this thing in Bunny (or RabbitMQ) or that thing should be improved, all while not putting their money where their mouth is.

[michaelklishin]

Those who find Bunny's design unbearably awful, fundamentally flawed, unclean, improper and so on can develop their own client.

I will take its current design's 12-13 years of experience as evidence that there's nothing fundamentally flawed.

Versioned tags can be removed, JRuby support can be dropped completely (we've been recommending March Hare for years anyway), and Bunny::Transport can be folded into Bunny::Session. Bunny::Channel can guard against outgoing basic.ack frames during connection recovery in a different way.

However, not with this aggressive, overly opinionated, belittling and deprecating a pioneering design (before Bunny, automatic connection recovery did not exist in any of RabbitMQ's clients), outright insulting to the maintainers style of "issue reporting".

It could have been a series of meaningful improvements in Bunny but it ended up being a walk down the 2012-2013 memory lane when Bunny's awfully flawed design has influenced multiple AMQP 0-9-1 clients used at much broader scale than Bunny (Java, .NET, JavaScript, to a certain extent the Go one where Sean T. has decided to not implement automatic connection recovery at all), and @tie's ban from this org.

His way of "convincing" OSS maintainers whose work he uses for free with virtually no contributions turned out to be fundamentally flawed. Workarounds: don't be a jerk next time!

[michaelklishin]

And I’m not even touching the fact that there are a lot of other bugs like getting recoveries counter after submitting a delivery to the worker pool

This is the part I'm facepalming about the most. @tie could have easily contributed a fix for this,
earned at least a little bit of maintainer trust, and then suggested something else as a follow-up.
Every maintainer would have been more receptive to an idea from someone who is a prior contributor,
in particular a recent one.

Instead, he is belittling dozens of Bunny contributors, "jeez, you dummies could not even get THAT part right!"

Even a sea turtle has better negotiating skills 🤦

[michaelklishin]

As of #704 #711, Bunny adopts the design of the Java client where the topology is recorded, maintained and restored by Bunny::Session, opening the door for more Java client features, such such as entity filtering and topology recovery retries, to be adopted.