Add non-retryable exception middleware #958
Conversation
📝 WalkthroughWalkthroughA new middleware class is added to handle non-retryable exceptions by immediately deleting SQS messages instead of retrying. The middleware is integrated into the default server middleware chain and can be configured per-worker via the Changes
Sequence DiagramsequenceDiagram
participant Worker
participant NonRetryableException as NonRetryableException<br/>Middleware
participant Shoryuken as Shoryuken::Client
participant SQS as SQS Queue
Worker->>NonRetryableException: call(worker, queue, sqs_msg)
NonRetryableException->>Worker: yield (next middleware)
alt Exception Occurs
Worker-->>NonRetryableException: raises exception
NonRetryableException->>NonRetryableException: check worker options
alt Exception in non_retryable_exceptions config
NonRetryableException->>NonRetryableException: log warning + backtrace
NonRetryableException->>NonRetryableException: normalize messages to list
NonRetryableException->>Shoryuken: delete_messages(entries)
Shoryuken->>SQS: delete from queue
SQS-->>Shoryuken: success
else Exception not configured
NonRetryableException-->>Worker: re-raise exception
end
else No Exception
Worker-->>NonRetryableException: returns normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/shoryuken/middleware/server/non_retryable_exception.rb`:
- Around line 52-54: Wrap the call to
Shoryuken::Client.queues(queue).delete_messages(entries: entries) in a
safe-check: inspect the returned response for failures and log a warning if
deletion reports failures (include queue name, entries and response details),
and also rescue any exceptions around the call to log an error/warning instead
of swallowing; update the code in non_retryable_exception.rb around the entries
creation/delete call to perform these checks and logging so failed deletes are
visible.
🧹 Nitpick comments (2)
lib/shoryuken/worker.rb (1)
126-126: Line exceeds maximum length (135/125 characters).Consider wrapping this documentation line to comply with the line length limit.
🔧 Suggested fix
- # `@option` opts [Array<Class>] :non_retryable_exceptions Exception classes that should skip retries and delete message immediately + # `@option` opts [Array<Class>] :non_retryable_exceptions Exception classes that should + # skip retries and delete message immediatelyspec/lib/shoryuken/middleware/server/non_retryable_exception_spec.rb (1)
21-31: Consider cleaning up worker options after each test.The tests directly modify
TestWorker.get_shoryuken_options['non_retryable_exceptions']. If tests run in a different order or a test fails mid-execution, the state could leak. Consider resetting the options in anafterblock.♻️ Suggested improvement
before do allow(Shoryuken::Client).to receive(:queues).with(queue).and_return(sqs_queue) end + + after do + TestWorker.get_shoryuken_options.delete('non_retryable_exceptions') + end
| # 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) |
There was a problem hiding this comment.
Consider handling delete_messages failures.
The delete_messages method (from lib/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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 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) | |
| # Delete the message(s) immediately | |
| entries = messages.map.with_index { |message, i| { id: i.to_s, receipt_handle: message.receipt_handle } } | |
| 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 |
🤖 Prompt for AI Agents
In `@lib/shoryuken/middleware/server/non_retryable_exception.rb` around lines 52 -
54, Wrap the call to Shoryuken::Client.queues(queue).delete_messages(entries:
entries) in a safe-check: inspect the returned response for failures and log a
warning if deletion reports failures (include queue name, entries and response
details), and also rescue any exceptions around the call to log an error/warning
instead of swallowing; update the code in non_retryable_exception.rb around the
entries creation/delete call to perform these checks and logging so failed
deletes are visible.
1 participant
Resolves #923
Adds middleware support for configuring exceptions that should skip retries and delete messages immediately. This is useful for permanent failures like invalid input data, business logic validation failures, or resource not found errors.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.