Add broker_default with nil - #124
Open
skunkworker wants to merge 15 commits into
Open
Conversation
Introduce an Appraisal matrix covering Rails 6.1 through 8.1, with the default-gem shims (logger, mutex_m, bigdecimal, drb, base64, benchmark) that ActiveSupport < 7.1 needs on Ruby >= 3.4. CircleCI now runs build_and_test as a parameterized job across the Ruby and JRuby images and each appraisal gemfile. Rails 8.0/8.1 require Ruby >= 3.2, so they get their own matrix (cimg/ruby:3.4, jruby:10.0) rather than exclude entries against the Ruby 3.1-class images. Coverage is unchanged at 20 jobs. Also add a spec helper that waits for RabbitMQ before the integration suite starts, so a cold broker fails with one clear message instead of a flurry of reconnect warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a queue_type setting, configurable globally (config.queue_type) or per route (:queue_type => ...). Values are nil (default), :classic, :quorum and :stream, with :broker_default accepted as a readable alias for nil. nil leaves x-queue-type off the wire so RabbitMQ applies its own default_queue_type. :quorum and :stream force the route to be durable, since RabbitMQ only supports those as durable queues. Values are normalized on assignment, so an invalid value raises where it was set rather than at route-draw time or inside MessageRetry at runtime. BREAKING on JRuby. The two drivers disagreed: march_hare reads its :type option with fetch(:type, ... Types::CLASSIC), and fetch only falls back when the key is absent, so omitting :type injected x-queue-type: classic on every declare. bunny reads @options[:type] and sent no argument at all. Both drivers are now passed :type explicitly, so neither sends x-queue-type by default. MRI behavior is unchanged. On JRuby, newly declared queues change from classic to whatever the broker defaults to. Set config.queue_type to :classic to retain the previous behavior. Because queue type is fixed at declaration, redeclaring an existing queue against a vhost whose default_queue_type is not classic will fail with PRECONDITION_FAILED -- audit vhosts before upgrading a JRuby deployment. Known limitation, documented in the README: MessageRetry declares retry queues from the global config.queue_type rather than the originating route's, so a route-level type is not propagated to its retry queue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bump VERSION to 7.5.0 and date the changelog entry for the queue_type setting and the JRuby x-queue-type behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit bumped to 7.5.0, which skipped the 6.x line and read as a minor bump. Use 6.0.0 instead: a major bump is warranted because the x-queue-type change is breaking for JRuby consumers, where queues that march_hare previously declared as classic are now declared with whatever the broker defaults to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gemfiles/ directory is fully derived from the Appraisals file, so committing the stubs meant keeping generated output in sync by hand. Gitignore the whole directory and regenerate it in CI instead. The generate step has to override BUNDLE_GEMFILE to the root Gemfile: the job sets it to the target appraisal gemfile, which does not exist until this step writes it. Verified that `appraisal generate` runs from a clean checkout with no prior bundle install, and that its output is byte-identical to the stubs being removed here. Cache keys now also checksum Appraisals, so editing it busts the bundle cache, and are bumped to v3 since the previous caches predate this. Document the local workflow in the README, since gemfiles/ no longer exists after a fresh clone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generate step ran `appraisal generate` without installing the root bundle first. appraisal runs under bundler, so it aborted with: Could not find gem 'active_publisher (= 1.6.0)' in locally installed gems. Run `bundle install --gemfile Gemfile` to install missing gems. Install the root Gemfile before generating. The two bundles differ only in their Rails pins and share vendor/bundle, so the follow-up install is mostly a no-op. Also cache ./gemfiles/vendor/bundle alongside ./vendor/bundle. Because `bundle config --local` resolves relative to the directory holding BUNDLE_GEMFILE, the appraisal bundle installs under gemfiles/, so caching only ./vendor/bundle reinstalled the gems the tests actually use on every run. Cache keys bumped to v4. Verified the full sequence from a clean clone with an isolated GEM_HOME under JRuby 10: root install, generate, and the rails_8.0 target install all exit 0 and resolve activesupport 8.0.5.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Durability could only be set per route. This adds it as a configuration
setting, so it can be turned on globally -- including from
config/action_subscriber.yml, which means an operator can do it without a
code change -- or declared per subscriber:
production:
durable: true
class UserSubscriber < ::ActionSubscriber::Base
durable true
end
Precedence is route option > subscriber declaration > config.durable. The
default is unchanged at false.
This is the smallest way onto RabbitMQ 4.x, which denies the transient
queues every default route declares. config.queue_type = :quorum also
works but changes the queue type as well; config.durable leaves it alone.
Two notes on the implementation:
* :durable had to come out of Router::DEFAULT_SETTINGS and
DefaultRouting. Baking in a value makes an unspecified route
indistinguishable from one that explicitly asked for false, so the
fallback could never fire. :prefetch was already the precedent for
leaving a key out and letting Route resolve it.
* the resolution lives in Route rather than Router so that both `route`
and `default_routes_for` honor the subscriber's declaration. Note
that :acknowledgements does not do this -- manual_acknowledgement!
only takes effect through default_routes_for.
QueueType.durable? gives the "quorum and stream are always durable" rule
a single owner, since the retry path needs it too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MessageRetry declared its retry queues without passing :durable. On JRuby
that was harmless, because march_hare forces quorum and stream queues
durable internally -- but bunny does not, so on MRI a config.queue_type
of :quorum made every retry declaration fail with
PRECONDITION_FAILED - invalid property 'non-durable' for queue
Retries were therefore broken on MRI for any deployment using quorum
queues, while JRuby worked, because march_hare was silently compensating.
Retry queues now derive durability through QueueType.durable?, so they
follow config.durable as well. Without that, a deployment that set
config.durable to get onto RabbitMQ 4.x would still fall over the first
time a message was retried -- and because this path reuses env.channel
rather than opening its own the way setup_queue does, the refused
declaration takes the connection down and surfaces as a cascade of
unrelated failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groundwork for testing against more than one RabbitMQ series. The suite
previously assumed localhost:5672 and the library defaults.
* RabbitMQTestHelper reads host, ports, credentials and vhost from the
environment, so two brokers can run side by side and the suite can be
pointed at either. spec_helper configures both ActionSubscriber and
ActivePublisher from it. The port has to travel inside the host entry:
march_hare builds its address list from :hosts alone, where a bare
hostname means 5672 no matter what :port says.
* ACTION_SUBSCRIBER_QUEUE_TYPE and ACTION_SUBSCRIBER_DURABLE override
the configuration for integration examples only, so the unit specs
still assert the real defaults.
* with_action_subscriber_config and the :as_config metadata replace the
save-mutate-restore block that would otherwise be written out at every
site. It restores in an ensure -- a leaked setting reappears later as
a PRECONDITION_FAILED from an unrelated spec, because queue type and
durability are fixed when a queue is declared.
* the helper reads queues back over the management API, which is the
only way to see what a declaration actually produced when
x-queue-type was left off the wire, and it declares out of band on its
own connection so a refusal cannot poison the subscribers'.
* the suite clears the vhost at startup, for the same
fixed-at-declaration reason. consumer_cancellation already did this
mid-suite; it now calls the shared helper.
The helper deliberately opens a connection per call. Memoizing one was
tried and reverted: several specs provoke a connection-level refusal on
4.x, bunny does not reliably recover from that, and reuse produced
order-dependent failures and hangs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both assert what the broker actually created, read back over the
management API. The driver option and the wire arguments were already
covered by unit specs, and neither tells you whether the broker agreed.
queue_type_spec covers the queue each route produces, and pins the
regression that motivated ActionSubscriber::QueueType: subscribing to a
queue somebody else declared as a durable quorum queue. Master could not
do it, in three independent ways --
* on JRuby, march_hare filled in :type => "classic" for the omitted
option, so the broker rejected the redeclaration on x-queue-type;
* on MRI, bunny sent no x-queue-type, which the broker resolves against
the vhost default and rejects the same way;
* and both sent durable => false, which a quorum queue rejects on its
own.
broker_compatibility_spec pins the differences between the two supported
series. RabbitMQ 3.x has transient_nonexcl_queues in the
permitted_by_default deprecation phase; 4.x moved it to
denied_by_default. Since every default route is a transient queue, the
library cannot declare one on a stock 4.x broker at all -- the spec
asserts that limitation rather than papering over it, along with the two
settings that work around it.
The examples branch on what the broker actually permits rather than on
its version, so a 3.x broker with the feature denied, or a 4.x broker
with it permitted, still gets a true answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Rails matrix pinned one broker (3.12). This parameterises the broker image, the queue type and the durability setting, bumps the Rails matrix to 3.13, and adds a broker_compatibility_matrix that pins the broker axis against one Rails version so the two do not multiply out. Both drivers are covered deliberately: bunny and march_hare disagree about queue declaration in ways only a live broker shows. march_hare fills in x-queue-type: classic for an omitted :type where bunny sends nothing, and march_hare forces quorum queues durable where bunny does not. The 4.x jobs have to configure something, because a default route is a transient queue and 4.x denies those. Both documented ways out get a job: durable classic queues, which is the smaller change for an existing deployment, and quorum queues, which also exercise forced durability and quorum retry queues. expected_rabbitmq_major is asserted by the suite, so an image tag that stops resolving to the series a job name claims fails loudly instead of passing as a duplicate of another job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a Durability section covering the new setting and its precedence, and a Supported RabbitMQ Versions section covering the 3.x/4.x split. The 4.x limitation is worth stating plainly: routes default to :durable => false, RabbitMQ 4.x moved transient_nonexcl_queues to denied_by_default, so a stock 4.x broker refuses every default route. The failure is hard to read on both drivers -- it is a connection-level 541 that neither decodes, so march_hare reports "Unknown reply code: 541" and bunny simply blocks until continuation_timeout. Also documents that durability, unlike queue type, cannot be deferred to the broker: it is a field in the declaration frame rather than an optional argument, so a client always states a value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in specs added on this branch, both of which make an unrelated
example fail at an unrelated line. Bunny raises asynchronous connection
errors on Thread.main, so anything that disturbs the shared subscriber
connection surfaces wherever the current example happens to be -- in CI,
as a Bunny::NetworkFailure pointing at a `sleep` in
consumer_cancellation_spec.
* queue_type_spec drives RouteSet#setup_queue directly, which opens a
channel on the shared subscriber connection and never closes it. Four
examples leaked a channel each, along with its consumer work pool.
They are closed now.
* consumer_cancellation triggers the cancellation it tests by deleting
queues out from under live consumers, and was doing that for every
queue in the vhost. It only needs its own. The wider version also
deleted queues that other examples still held channels against on the
same connection.
Both reduce cross-example interference rather than change what is tested.
Note that the CI failure this addresses did not reproduce locally -- 16
runs against the same broker and driver under full CPU saturation,
including at the seed CI used -- so this is a plausible cause, not a
confirmed one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI failures on this suite are ordering- and configuration-dependent, and
the seed alone no longer reproduces one: behaviour also depends on which
broker the job talked to and which settings it ran under, and the matrix
varies all three. On failure the suite now prints a complete command.
Reproduce this run against RabbitMQ 3.13.7 with:
ACTION_SUBSCRIBER_QUEUE_TYPE=quorum \
BUNDLE_GEMFILE=gemfiles/rails_8.1.gemfile \
EXPECTED_RABBITMQ_MAJOR=3 \
RABBITMQ_PORT=5673 \
bundle exec rspec --seed 40843
The variables are recorded by RabbitMQTestHelper.env as the suite reads
them, rather than kept in a list somebody has to remember to update. A
hand-kept list had already drifted while writing this: it omitted
EXPECTED_RABBITMQ_MAJOR, which decides whether an example is defined at
all -- so the command would have produced a different example count, and
the same seed would have shuffled a different list. Deriving it from what
the run actually consumed also means only variables that were really set
get printed.
Registered as a :close listener rather than an after(:suite) hook. Suite
hooks run inside Reporter#report, so their output lands above the failure
dump and above RSpec's own seed line -- scrolled off the bottom, which is
where anyone opening a failed job starts reading. :close fires last.
Two related changes:
* the management client now sets explicit timeouts. It passed its
options straight to Faraday, which sets none of its own, so the
Net::HTTP defaults applied: a host that drops packets rather than
refusing would wedge the suite for up to two minutes with no output.
* example_status_persistence_file_path, so --only-failures and
--next-failure work locally.
No CI artifact for the status file: --only-failures reads the local copy,
so using CI's would mean downloading it and putting it in place, which
nobody will do. If flake detection is wanted, store_test_results with a
JUnit formatter is the mechanism, and that is worth doing separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
Adds a first party
queue_typesetting so queues can be declared asclassic,quorum, orstream— or, by default, with no opinion at all so the broker's owndefault_queue_typeapplies.Configurable globally or per route:
x-queue-typesentnil(default), or:broker_defaultdefault_queue_type:classicclassic:quorumquorum:streamstreamnilis the canonical "defer to the broker" value;:broker_defaultis accepted as a more readable spelling and normalizes tonil. Values are normalized on assignment, so a typo raises anArgumentErrorwhere it was set rather than at route-draw time or insideMessageRetryat runtime.:quorumand:streamforcedurable => true, since RabbitMQ only supports those as durable queues.The bug this uncovered
The two drivers have been disagreeing about queue type this whole time.
fetchonly falls back when the key is absent. Sincesetup_queuenever passed:type, JRuby has been declaring every queue withx-queue-type: classicwhile MRI sent no argument at all. Verified against a realMarchHare::Queue:Passing
:type => nilexplicitly — key present, value nil — is what suppresses the argument. That is the whole mechanism behind:broker_default, and it's why the option has to reach the driver as an explicitnilrather than by omitting it.Worth noting for reviewers: you cannot send
x-queue-typewith a null value as a middle ground.amq-protocolwill encode it as AMQP void ("\x00\x00\x00\x0E\fx-queue-typeV"), but it can't decode its own output, and RabbitMQ resolves the type viarabbit_queue_type:discover/1, which matches no known type for a void and fails the declare. Omitting the key is the only way to defer to the broker.MRI behavior is unchanged. On JRuby, newly declared queues change from
classicto whatever the broker defaults to.This is not limited to new queues. Because queue type is fixed at declaration, an existing
classicqueue is now redeclared withoutx-queue-type. That's harmless on a vhost whosedefault_queue_typeis classic, since the broker resolves to the same type — but it fails withPRECONDITION_FAILEDon a vhost defaulting toquorumorstream.Before rolling this out to a JRuby deployment, audit every vhost it connects to:
If any are non-classic, set
config.queue_type = :classicbefore deploying — that restores the previous JRuby behavior exactly.Known limitation
MessageRetrydeclares its*.retry_*queues from the globalconfig.queue_type, not the type of the route that produced the message, so a route-level type is not propagated to its retry queue. Documented in the README rather than fixed here — propagating it isn't a straight pass-through, since retry queues carryx-message-ttlandx-dead-letter-exchangeand streams support neither. Happy to take it in a follow-up.Testing
Full suite green on JRuby 10.0.4 against a live broker: 130 examples, 0 failures (1 pending is pre-existing). New coverage in
spec/lib/action_subscriber/queue_type_spec.rbincludes JRuby-only examples that assert the actualMarchHare::Queueargument table for each option — including one pinning the surprisingomitted :type -> classicbehavior, so a driver change that alters it fails loudly.Note on commit scope
This branch carries two commits.
d08277b(Appraisal matrix, CI split by Rails version, RabbitMQ spec helper) is largely pre-existing work that was already in the working tree; the only new part is giving Rails 8.0/8.1 their own CI matrix instead ofexcludeentries against the Ruby 3.1-class images — job coverage is unchanged at 20.1a348ecis thequeue_typefeature. Review the second commit for the substance here.🤖 Generated with Claude Code