Add publisher confirms to support quorum queues. #4
Conversation
TreyE
force-pushed
the
cu-868gra3td
branch
3 times, most recently
from
d3880fc to
0b80688
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds publisher confirms support for RabbitMQ quorum queues, ensuring message delivery reliability through the publisher confirms pattern. The changes include creating separate publishing channels with confirm mode enabled, along with Ruby version updates and security dependency patches.
- Implements publisher confirms by calling
confirm_selecton publishing channels andwait_for_confirmsafter each publish operation - Updates Ruby version from 2.6.6 to 2.6.9 for better stability and security
- Upgrades critical dependencies (nokogiri, loofah, crass) to address security vulnerabilities
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/acapi/local_amqp_publisher.rb | Creates separate @p_channel for publishing with confirm_select enabled; adds wait_for_confirms after publish |
| lib/acapi/amqp/responder.rb | Adds confirm_select and wait_for_confirms to response exchange within with_response_exchange method |
| lib/acapi/amqp/requestor.rb | Creates dedicated p_channel for publishing with confirms; properly closes channel in ensure block |
| lib/acapi/amqp/client.rb | Establishes @republish_channel with confirms for redelivery scenarios |
| lib/acapi/requestor.rb | Improves connection check to verify both presence and connected status |
| spec/lib/acapi/local_amqp_publisher_spec.rb | Adds mocks for confirm_select and wait_for_confirms; changes create_channel expectation to allow for multiple channel creation |
| .ruby-version | Updates Ruby version from 2.6.6 to 2.6.9 |
| .github/workflows/tests.yml | Updates Ruby version to 2.6.9 and upgrades actions/cache from v1 to v4 |
| .github/workflows/downstream.yml | Removes branches filter from workflow_dispatch to allow manual runs from any branch |
| Gemfile.lock | Updates security-critical dependencies: crass, loofah, nokogiri, and adds racc |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TreyE
force-pushed
the
cu-868gra3td
branch
2 times, most recently
from
38fa31d to
d417ae8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def open_connection_if_needed | ||
| return if @connection.present? && @connection.connected? | ||
| @connection = Bunny.new | ||
| @connection = Bunny.new(connection_url) | ||
| @connection.start | ||
| @channel = @connection.create_channel | ||
| @queue = @channel.queue(QUEUE_NAME, {:durable => true}) | ||
| @exchange = @channel.fanout(EXCHANGE_NAME, {:durable => true}) | ||
| @p_channel = @connection.create_channel | ||
| @p_channel.confirm_select | ||
| @exchange = @p_channel.fanout(EXCHANGE_NAME, {:durable => true}) | ||
| @queue.bind(@exchange, {}) | ||
| end |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @connection = Bunny.new(@uri) | ||
| @connection.start |
There was a problem hiding this comment.
The parameter name uri is misleading. After the changes in lib/acapi/railties/amqp_configuration_options.rb, the boot_requestor method now passes setting.to_connection_settings (a hash) instead of a URI string. The parameter should be renamed to something like connection_settings or connection_params to better reflect that it now receives a hash rather than a URI string.
| allow(session).to receive(:create_channel).and_return(channel) | ||
| allow(channel).to receive(:queue).with(forwarding_queue_name, {:durable => true}).and_return(queue) | ||
| allow(channel).to receive(:fanout).with(forwarding_exchange_name, {:durable => true}).and_return(exchange) | ||
| allow(channel).to receive(:confirm_select) | ||
| allow(channel).to receive(:wait_for_confirms).and_return(true) | ||
| allow(queue).to receive(:bind).with(exchange, {}) | ||
| end |
There was a problem hiding this comment.
The test setup does not accurately mock the dual-channel architecture introduced in the code changes. The updated implementation creates two separate channels (one for consuming, one for publishing with confirms), but the test mock returns the same channel instance for both calls to create_channel. While the test may still pass, it doesn't properly validate that two distinct channels are being used. Consider creating two separate channel mocks to accurately test this behavior.
| allow(session).to receive(:create_channel).and_return(channel) | ||
| expect(channel).to receive(:queue).with(forwarding_queue_name, {:durable=> true}).and_return(queue) | ||
| expect(channel).to receive(:fanout).with(forwarding_exchange_name, {:durable => true}).and_return(exchange) | ||
| expect(queue).to receive(:bind).with(exchange, {}) |
There was a problem hiding this comment.
The test setup does not accurately mock the dual-channel architecture. Similar to the main test setup, this test returns the same channel instance for both calls to create_channel, when the implementation creates two distinct channels. Consider creating separate channel mocks to properly test the dual-channel publishing behavior.
https://app.clickup.com/t/868gra3td
This pull request introduces several important updates focused on improving the reliability and consistency of AMQP message publishing by enabling publisher confirms throughout the codebase. Additionally, it upgrades Ruby to version 2.6.9 and updates related workflow and caching configurations. Below are the key changes grouped by theme:
AMQP Publisher Confirms and Reliability Improvements:
Added publisher confirms (
confirm_selectandwait_for_confirms) to AMQP publishing channels inlib/acapi/amqp/client.rb,lib/acapi/amqp/requestor.rb,lib/acapi/amqp/responder.rb, andlib/acapi/local_amqp_publisher.rbto ensure that messages are reliably published and acknowledged by the broker. [1] [2] [3] [4] [5] [6] [7]Updated related specs in
spec/lib/acapi/local_amqp_publisher_spec.rbto mock and test the new publisher confirm behavior. [1] [2]Ruby Version and Workflow Updates:
.ruby-versionand updated the Ruby version in the GitHub Actions workflow (.github/workflows/tests.yml). [1] [2]actions/cache@v4.Workflow Trigger Simplification:
.github/workflows/downstream.ymlby removing thebranchesfilter fromworkflow_dispatch, so it now only triggers on pushes totrunk.Connection Handling Fix:
lib/acapi/requestor.rbto avoid unnecessary reconnections by checking if the Bunny connection is already established and connected before creating a new one.