-
Notifications
You must be signed in to change notification settings - Fork 289
support AJ continuation in rails 8.1 #899
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
Conversation
WalkthroughImplements ActiveJob Continuations support for Rails 8.1+ by adding shutdown state tracking through new Changes
Sequence DiagramsequenceDiagram
participant App as Rails App
participant AJ as ActiveJob
participant Adapter as ShoryukenAdapter
participant Runner as Runner
participant Launcher as Launcher
participant Job as Job
App->>AJ: Graceful shutdown signal
AJ->>Adapter: Query stopping?
Adapter->>Runner: Get launcher instance
Runner->>Adapter: Return launcher
Adapter->>Launcher: Call stopping?
par Launcher State Update
Runner->>Launcher: Call stop! / stop
Launcher->>Launcher: Set @stopping = true
end
Launcher->>Adapter: Return stopping? = true
Adapter->>AJ: Return true
AJ->>Job: Signal to checkpoint<br/>instead of reject
Job->>AJ: Enqueue continuation<br/>with checkpoint state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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 PR adds support for ActiveJob Continuations (Rails 8.1+) to Shoryuken by implementing the stopping? method in the ActiveJob adapter. This allows jobs to detect when Shoryuken is shutting down and checkpoint their progress for graceful resumption.
Key changes:
- Added
stopping?method to the ActiveJob adapter that checks Shoryuken's shutdown state - Added
stopping?flag to the Launcher class to track shutdown state - Exposed the Launcher instance through the Runner for adapter access
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/shoryuken/launcher.rb | Added @stopping flag and stopping? method to track shutdown state |
| lib/shoryuken/runner.rb | Exposed launcher instance via attr_reader for external access |
| lib/shoryuken/extensions/active_job_adapter.rb | Implemented stopping? method to support ActiveJob Continuations |
| spec/shoryuken/launcher_spec.rb | Added tests for stopping? method behavior |
| spec/shoryuken/extensions/active_job_continuation_spec.rb | Added unit tests for ActiveJob Continuation support |
| spec/integration/active_job_continuation_spec.rb | Added integration tests for continuable jobs |
| CHANGELOG.md | Documented the new ActiveJob Continuations feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/specs.yml(1 hunks)CHANGELOG.md(1 hunks)lib/shoryuken/extensions/active_job_adapter.rb(1 hunks)lib/shoryuken/launcher.rb(3 hunks)lib/shoryuken/runner.rb(1 hunks)spec/integration/active_job_continuation_spec.rb(1 hunks)spec/shoryuken/extensions/active_job_continuation_spec.rb(1 hunks)spec/shoryuken/launcher_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
spec/shoryuken/launcher_spec.rb (3)
lib/shoryuken/extensions/active_job_adapter.rb (1)
stopping?(48-51)lib/shoryuken/launcher.rb (3)
stopping?(19-21)stop(43-56)stop!(30-41)lib/shoryuken/manager.rb (1)
stop_new_dispatching(30-32)
CHANGELOG.md (2)
lib/shoryuken/extensions/active_job_concurrent_send_adapter.rb (2)
ActiveJob(5-50)ShoryukenConcurrentSendAdapter(20-48)spec/integration/launcher_spec.rb (1)
include(107-125)
spec/integration/active_job_continuation_spec.rb (3)
lib/shoryuken/extensions/active_job_adapter.rb (7)
include(103-113)perform(108-112)stopping?(48-51)instance(19-121)instance(21-24)enqueue_at(30-32)enqueue_at(64-66)lib/shoryuken/launcher.rb (2)
include(4-120)stopping?(19-21)lib/shoryuken/runner.rb (1)
include(15-139)
spec/shoryuken/extensions/active_job_continuation_spec.rb (1)
lib/shoryuken/extensions/active_job_adapter.rb (3)
stopping?(48-51)enqueue_at(30-32)enqueue_at(64-66)
lib/shoryuken/extensions/active_job_adapter.rb (1)
lib/shoryuken/launcher.rb (1)
stopping?(19-21)
lib/shoryuken/launcher.rb (1)
lib/shoryuken/extensions/active_job_adapter.rb (1)
stopping?(48-51)
🪛 GitHub Check: CodeQL
.github/workflows/specs.yml
[warning] 75-88: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🪛 RuboCop (1.81.6)
spec/integration/active_job_continuation_spec.rb
[warning] 15-51: Do not define constants this way within a block.
(Lint/ConstantDefinitionInBlock)
🔇 Additional comments (16)
.github/workflows/specs.yml (1)
74-88: LGTM! Standard CI success pattern.The CI success job correctly aggregates the results of all spec jobs, ensuring the overall CI status reflects all test outcomes.
The static analysis warning about missing
permissionsis valid. Consider adding an explicit permissions block to follow security best practices:ci-success: name: CI Success runs-on: ubuntu-latest if: always() permissions: contents: read needs: - all_specs - rails_specsThis limits the GITHUB_TOKEN to read-only access, reducing security risk if the workflow is compromised.
CHANGELOG.md (1)
2-9: Excellent documentation of the new feature.The changelog entry clearly describes the ActiveJob Continuations support, including key implementation details and the Rails PR reference.
lib/shoryuken/runner.rb (1)
19-20: Clean API addition with good documentation.Exposing the launcher instance enables external components (like the ActiveJob adapter) to query the shutdown state. The documentation clearly indicates the return type.
spec/shoryuken/launcher_spec.rb (1)
105-125: Comprehensive test coverage for stopping? behavior.The tests verify the default state and transitions after both
stopandstop!are called, with appropriate mocking of manager dependencies.lib/shoryuken/launcher.rb (3)
7-21: Well-documented stopping state implementation.The
stopping?method and@stoppingflag are clearly documented and correctly initialized. The YARD documentation explains the purpose and usage for ActiveJob Continuations.
30-31: Correct state transition in stop!Setting
@stopping = trueat the beginning ofstop!ensures the flag is set before any shutdown logic executes.
43-44: Correct state transition in stop.Setting
@stopping = trueat the beginning ofstopensures the flag is set before any shutdown logic executes, consistent withstop!.lib/shoryuken/extensions/active_job_adapter.rb (2)
40-51: Robust implementation with safe navigation.The
stopping?method correctly uses the safe navigation operator to handle cases where the launcher is not yet initialized, returningfalseas a safe default.
70-75: Correct handling of past timestamps for continuations.The
calculate_delaymethod correctly allows negative delays (past timestamps), which SQS treats as immediate delivery. The 15-minute check only applies to future delays, which is the correct behavior for ActiveJob Continuations that may need to retry immediately.spec/shoryuken/extensions/active_job_continuation_spec.rb (2)
22-51: Comprehensive stopping? behavior tests.The tests cover all scenarios: launcher not initialized, launcher initialized but not stopping, and launcher stopping. Good use of test doubles to isolate the behavior.
53-95: Thorough timestamp handling tests.The tests verify correct behavior for past, future, and current timestamps, including the critical requirement that past timestamps result in non-positive
delay_secondsfor immediate delivery.spec/integration/active_job_continuation_spec.rb (5)
10-12: Appropriate version guard for Rails 8+ feature.The skip condition correctly prevents these tests from running on Rails versions that don't support ActiveJob::Continuable.
15-51: Well-structured test job for continuation testing.The
ContinuableTestJobimplementation correctly uses the step/checkpoint pattern with cursor tracking, execution logging, and checkpoint recording for validation.Note: The RuboCop warning about defining constants in a block is acceptable in test context, as this is a standard RSpec pattern for test-specific classes.
53-70: Thorough stopping state tests.The tests verify both uninitialized and initialized launcher scenarios, including the state transition when
@stoppingis set to true.
72-90: Critical test for continuation retry semantics.This test verifies the key behavior that past timestamps (used in continuation retries) result in
delay_seconds <= 0, ensuring immediate delivery through SQS.
93-144: Comprehensive timestamp boundary testing.The tests cover all timestamp scenarios with appropriate assertions on
delay_secondsvalues, including tolerance for timing variations.
|
LGTM🚀 |
Summary by CodeRabbit
New Features
Chores
Tests