-
Notifications
You must be signed in to change notification settings - Fork 290
Add non-retryable exception middleware #958
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
Open
Saidbek
wants to merge
1
commit into
ruby-shoryuken:main
Choose a base branch
from
Saidbek:feature/non-retryable-exception-middleware
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+220
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
62 changes: 62 additions & 0 deletions
62
lib/shoryuken/middleware/server/non_retryable_exception.rb
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Shoryuken | ||
| module Middleware | ||
| module Server | ||
| # Middleware that handles non-retryable exceptions by deleting messages immediately. | ||
| # When a configured exception occurs, the message is deleted instead of being retried. | ||
| # | ||
| # Configure non-retryable exceptions per worker: | ||
| # | ||
| # class MyWorker | ||
| # include Shoryuken::Worker | ||
| # | ||
| # shoryuken_options queue: 'my_queue', | ||
| # non_retryable_exceptions: [InvalidInputError, RecordNotFoundError] | ||
| # | ||
| # def perform(sqs_msg, body) | ||
| # # ... | ||
| # end | ||
| # end | ||
| class NonRetryableException | ||
| include Util | ||
|
|
||
| # Processes a message and handles non-retryable exceptions | ||
| # | ||
| # @param worker [Object] the worker instance | ||
| # @param queue [String] the queue name | ||
| # @param sqs_msg [Shoryuken::Message, Array<Shoryuken::Message] the message or batch | ||
| # @param _body [Object] the parsed message body (unused) | ||
| # @yield continues to the next middleware in the chain | ||
| # @return [void] | ||
| def call(worker, queue, sqs_msg, _body) | ||
| yield | ||
| rescue => e | ||
| non_retryable_exceptions = worker.class.get_shoryuken_options['non_retryable_exceptions'] | ||
|
|
||
| return raise unless non_retryable_exceptions | ||
|
|
||
| exception_classes = Array(non_retryable_exceptions) | ||
| return raise unless exception_classes.any? { |klass| e.is_a?(klass) } | ||
|
|
||
| # Handle batch messages | ||
| messages = sqs_msg.is_a?(Array) ? sqs_msg : [sqs_msg] | ||
|
|
||
| logger.warn do | ||
| "Non-retryable exception #{e.class} occurred for message(s) #{messages.map(&:message_id).join(', ')}. " \ | ||
| "Deleting message(s) immediately. Error: #{e.message}" | ||
| end | ||
|
|
||
| logger.debug { e.backtrace.join("\n") } if e.backtrace | ||
|
|
||
| # Delete the message(s) immediately | ||
| entries = messages.map.with_index { |message, i| { id: i.to_s, receipt_handle: message.receipt_handle } } | ||
| Shoryuken::Client.queues(queue).delete_messages(entries: entries) | ||
|
|
||
| # Don't re-raise - the exception has been handled by deleting the message | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
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
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
143 changes: 143 additions & 0 deletions
143
spec/lib/shoryuken/middleware/server/non_retryable_exception_spec.rb
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| RSpec.describe Shoryuken::Middleware::Server::NonRetryableException do | ||
| let(:queue) { 'default' } | ||
| let(:sqs_queue) { double Shoryuken::Queue } | ||
|
|
||
| def build_message | ||
| double Shoryuken::Message, | ||
| queue_url: queue, | ||
| body: 'test', | ||
| message_id: SecureRandom.uuid, | ||
| receipt_handle: SecureRandom.uuid | ||
| end | ||
|
|
||
| let(:sqs_msg) { build_message } | ||
|
|
||
| before do | ||
| allow(Shoryuken::Client).to receive(:queues).with(queue).and_return(sqs_queue) | ||
| end | ||
|
|
||
| context 'when non_retryable_exceptions is not configured' do | ||
| it 're-raises the exception' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = nil | ||
|
|
||
| expect(sqs_queue).not_to receive(:delete_messages) | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise StandardError, 'test error' } | ||
| }.to raise_error(StandardError, 'test error') | ||
| end | ||
| end | ||
|
|
||
| context 'when exception is not in non_retryable_exceptions list' do | ||
| it 're-raises the exception' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [ArgumentError] | ||
|
|
||
| expect(sqs_queue).not_to receive(:delete_messages) | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise StandardError, 'test error' } | ||
| }.to raise_error(StandardError, 'test error') | ||
| end | ||
| end | ||
|
|
||
| context 'when exception is in non_retryable_exceptions list' do | ||
| it 'deletes the message and does not re-raise' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [StandardError] | ||
|
|
||
| expect(sqs_queue).to receive(:delete_messages).with(entries: [ | ||
| { id: '0', receipt_handle: sqs_msg.receipt_handle } | ||
| ]) | ||
|
|
||
| expect(Shoryuken.logger).to receive(:warn) do |&block| | ||
| expect(block.call).to match(/Non-retryable exception StandardError/) | ||
| end | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise StandardError, 'test error' } | ||
| }.not_to raise_error | ||
| end | ||
|
|
||
| it 'logs the exception backtrace in debug mode' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [StandardError] | ||
|
|
||
| error = StandardError.new('test error') | ||
| error.set_backtrace(['backtrace line 1', 'backtrace line 2']) | ||
|
|
||
| allow(sqs_queue).to receive(:delete_messages) | ||
|
|
||
| expect(Shoryuken.logger).to receive(:warn) | ||
| expect(Shoryuken.logger).to receive(:debug) do |&block| | ||
| expect(block.call).to eq("backtrace line 1\nbacktrace line 2") | ||
| end | ||
|
|
||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise error } | ||
| end | ||
|
|
||
| it 'handles multiple exception classes' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [ArgumentError, StandardError] | ||
|
|
||
| expect(sqs_queue).to receive(:delete_messages).with(entries: [ | ||
| { id: '0', receipt_handle: sqs_msg.receipt_handle } | ||
| ]) | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise ArgumentError, 'test error' } | ||
| }.not_to raise_error | ||
| end | ||
|
|
||
| it 'handles custom exception classes' do | ||
| custom_error = Class.new(StandardError) | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [custom_error] | ||
|
|
||
| expect(sqs_queue).to receive(:delete_messages).with(entries: [ | ||
| { id: '0', receipt_handle: sqs_msg.receipt_handle } | ||
| ]) | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) { raise custom_error, 'test error' } | ||
| }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
| context 'with batch messages' do | ||
| it 'deletes all messages in the batch' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [StandardError] | ||
|
|
||
| sqs_msg2 = build_message | ||
| sqs_msg3 = build_message | ||
| sqs_msgs = [sqs_msg, sqs_msg2, sqs_msg3] | ||
|
|
||
| expect(sqs_queue).to receive(:delete_messages).with(entries: [ | ||
| { id: '0', receipt_handle: sqs_msg.receipt_handle }, | ||
| { id: '1', receipt_handle: sqs_msg2.receipt_handle }, | ||
| { id: '2', receipt_handle: sqs_msg3.receipt_handle } | ||
| ]) | ||
|
|
||
| expect(Shoryuken.logger).to receive(:warn) do |&block| | ||
| expect(block.call).to match(/Non-retryable exception StandardError/) | ||
| expect(block.call).to match(/#{sqs_msg.message_id}/) | ||
| expect(block.call).to match(/#{sqs_msg2.message_id}/) | ||
| expect(block.call).to match(/#{sqs_msg3.message_id}/) | ||
| end | ||
|
|
||
| expect { | ||
| subject.call(TestWorker.new, queue, sqs_msgs, [sqs_msg.body, sqs_msg2.body, sqs_msg3.body]) do | ||
| raise StandardError, 'test error' | ||
| end | ||
| }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
| context 'when no exception occurs' do | ||
| it 'does not delete the message' do | ||
| TestWorker.get_shoryuken_options['non_retryable_exceptions'] = [StandardError] | ||
|
|
||
| expect(sqs_queue).not_to receive(:delete_messages) | ||
|
|
||
| subject.call(TestWorker.new, queue, sqs_msg, sqs_msg.body) {} | ||
| end | ||
| end | ||
| end | ||
|
|
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.
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.
Consider handling
delete_messagesfailures.The
delete_messagesmethod (fromlib/shoryuken/queue.rb:50-59) can fail and returns a truthy value when failures occur. Currently, if the delete fails, the exception is still swallowed, potentially leaving the message in the queue to be retried anyway.Consider checking the return value or wrapping in a rescue to at least log a warning if deletion fails:
🛡️ Suggested improvement
# Delete the message(s) immediately entries = messages.map.with_index { |message, i| { id: i.to_s, receipt_handle: message.receipt_handle } } - Shoryuken::Client.queues(queue).delete_messages(entries: entries) + if Shoryuken::Client.queues(queue).delete_messages(entries: entries) + logger.error { "Failed to delete some non-retryable message(s) for #{messages.map(&:message_id).join(', ')}" } + end # Don't re-raise - the exception has been handled by deleting the message📝 Committable suggestion
🤖 Prompt for AI Agents