-
Notifications
You must be signed in to change notification settings - Fork 0
Add publisher confirms to support quorum queues. #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
d9f8ddb to
d3880fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @republish_channel = @channel.connection.create_channel | ||
| @republish_channel.confirm_select | ||
| @republish_queue = @republish_channel.queue(@queue.name, @queue.options) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @republish_channel is created during initialization and set up with confirm_select, but there is no corresponding cleanup logic to close this channel. While the connection closing will eventually clean up the channel, consider adding explicit channel cleanup in a cleanup/disconnect method to ensure proper resource management and avoid potential channel leaks in long-running processes.
| end | ||
| msg = Acapi::Amqp::OutMessage.new(@app_id, name, finished, finished, unique_id, data) | ||
| @exchange.publish(*msg.to_message_properties) | ||
| @p_channel.wait_for_confirms |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wait_for_confirms call doesn't check the return value or handle potential exceptions. wait_for_confirms returns false if any messages were not confirmed and can raise exceptions like Bunny::NotConnectedError. Consider checking the return value or wrapping this call in error handling to ensure message delivery failures are properly detected and handled.
| new_properties = redelivery_properties(existing_retry_count, delivery_info, properties) | ||
| queue.publish(payload, new_properties) | ||
| @republish_queue.publish(payload, new_properties) | ||
| @republish_channel.wait_for_confirms |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wait_for_confirms call doesn't check the return value or handle potential exceptions. wait_for_confirms returns false if any messages were not confirmed and can raise exceptions. The outer rescue block will catch exceptions, but the false return value indicating failed confirmations would be silently ignored, potentially leading to message loss without notification. Consider explicitly checking the return value.
https://app.clickup.com/t/868gra3td