Move run ttl and delays from graphile to redis worker #1672
Conversation
|
WalkthroughThe changes update the job handling and scheduling logic within several services. New job handlers for expiration and delayed runs are added to the common worker. Existing calls to dequeue a run have been replaced by an acknowledgment method. Additionally, the static methods for enqueuing and rescheduling delayed runs are introduced and refactored. The modifications also simplify method signatures, remove unnecessary transaction parameters, and adjust retry configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant T as TriggerTaskService
participant E as EnqueueDelayedRunService
participant CW as commonWorker
T->>E: enqueue(runId, runAt)
E->>CW: commonWorker.enqueue({ job:"v3.enqueueDelayedRun", payload: runId, availableAt: runAt })
CW-->>T: Job enqueued
sequenceDiagram
participant S as TaskRunService\n(CreateTaskRunAttempt/FinalizeTaskRun)
participant EE as ExpireEnqueuedRunService
participant CW as commonWorker
S->>EE: ack(runId)
EE->>CW: commonWorker.ack(runId)
CW-->>EE: Acknowledgment complete
EE-->>S: Process finished
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/webapp/app/v3/commonWorker.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/v3/services/createTaskRunAttempt.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/v3/services/enqueueDelayedRun.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webapp/app/v3/commonWorker.server.ts(3 hunks)apps/webapp/app/v3/services/createTaskRunAttempt.server.ts(1 hunks)apps/webapp/app/v3/services/enqueueDelayedRun.server.ts(2 hunks)apps/webapp/app/v3/services/expireEnqueuedRun.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeTaskRun.server.ts(1 hunks)apps/webapp/app/v3/services/rescheduleTaskRun.server.ts(2 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
apps/webapp/app/v3/services/rescheduleTaskRun.server.ts (2)
1-4: LGTM! Import changes align with the PR objective.The addition of
EnqueueDelayedRunServiceimport supports the migration of delay handling to the redis worker.
8-17: LGTM! Robust validation logic preserved.The service maintains essential validation checks:
- Ensures task run is in "DELAYED" status
- Validates the delay parameter
apps/webapp/app/v3/services/expireEnqueuedRun.server.ts (2)
9-13: LGTM! Theackmethod is a good replacement fordequeue.The implementation correctly acknowledges the run without redundant dequeuing, which is safer and more efficient.
15-21: LGTM! Theenqueuemethod has been simplified.The implementation correctly structures the job object with all necessary fields and uses a consistent ID format.
apps/webapp/app/v3/services/enqueueDelayedRun.server.ts (2)
11-18: LGTM! Theenqueuemethod is well-implemented.The implementation correctly structures the job object and uses a consistent ID format.
20-33: Verify the job ID format consistency.The implementation correctly handles old job cleanup, but there's an inconsistency in the job ID format:
- Line 25 uses:
v3.enqueueDelayedRun.${runId}- Line 31 uses:
v3.enqueueDelayed:${runId}Consider using the same separator consistently to avoid potential issues:
- await workerQueue.dequeue(`v3.enqueueDelayedRun.${runId}`); + await workerQueue.dequeue(`v3.enqueueDelayed:${runId}`);apps/webapp/app/v3/commonWorker.server.ts (2)
99-108: LGTM! The job handlers are consistently implemented.The implementations are simple, consistent, and correctly handle the job payloads.
57-74: Verify the higher retry limits for new jobs.The new jobs have higher retry limits (6 attempts) compared to other jobs (3 attempts). While this might be intentional due to the critical nature of these operations, it's worth verifying.
Consider documenting the reason for higher retry limits in comments:
"v3.expireRun": { schema: z.object({ runId: z.string(), }), visibilityTimeoutMs: 60_000, retry: { + // Higher retry limit due to critical nature of run expiration maxAttempts: 6, }, },✅ Verification successful
Document rationale for elevated retry limits in new jobs
The configuration forv3.expireRunandv3.enqueueDelayedRunindeed sets a higher retry limit (6 attempts) compared to other jobs (typically using 3 attempts). This appears intentional—likely to accommodate the critical nature of these operations—so no functional change is required. However, adding an inline comment to document the rationale can help improve code clarity and future maintainability.
Location:
apps/webapp/app/v3/commonWorker.server.ts(lines 57–74)Observation: New jobs use
maxAttempts: 6, while similar jobs in the codebase generally use a lower retry limit (3 attempts).Recommendation: Add a comment explaining that the higher retry limit is intentional due to the critical nature of these new jobs, as illustrated below:
"v3.expireRun": { schema: z.object({ runId: z.string(), }), visibilityTimeoutMs: 60_000, retry: { + // Higher retry limit due to critical nature of run expiration maxAttempts: 6, }, },apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (1)
159-159: LGTM! The change to useackis consistent.The implementation correctly uses the new
ackmethod and maintains transaction integrity.apps/webapp/app/v3/services/finalizeTaskRun.server.ts (1)
104-104: LGTM! The change aligns with the refactor.The change from
dequeuetoackis consistent with the broader refactor to simplify the interaction with the worker queue.apps/webapp/app/v3/services/triggerTask.server.ts (3)
31-31: LGTM! Import aligns with the refactor.The import of
EnqueueDelayedRunServiceis necessary for the refactor.
519-519: LGTM! The change aligns with the refactor.The change to use
EnqueueDelayedRunService.enqueueis consistent with moving run delays to redis worker.
526-526: LGTM! The change aligns with the refactor.The removal of the transaction parameter from
ExpireEnqueuedRunService.enqueueis consistent with simplifying the method signature.
Summary by CodeRabbit
New Features
Refactor