Don't expire enqueued runs if they have lockedAt set #1571
Conversation
|
|
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/services/expireEnqueuedRun.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. WalkthroughThe changes focus on improving the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (1)
apps/webapp/app/v3/services/expireEnqueuedRun.server.ts (1)
52-58: Consider enhancing the debug log with timing information.To aid in debugging potential race conditions, consider including the
lockedAttimestamp in the debug log.if (run.lockedAt) { logger.debug("Run cannot be expired because it's locked", { - run, + run, + lockedAt: run.lockedAt, + timeSinceLock: Date.now() - run.lockedAt.getTime() });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/v3/services/expireEnqueuedRun.server.ts(1 hunks)
🔇 Additional comments (2)
apps/webapp/app/v3/services/expireEnqueuedRun.server.ts (2)
52-58: LGTM! The implementation correctly prevents expiring locked runs.
The added check aligns with the PR objective and follows the established pattern of guard clauses in the service.
52-58: Verify concurrent processing behavior.
The new lock check introduces a potential race condition window between checking lockedAt and performing expiration. Let's verify the atomicity of operations and usage patterns.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Lock handling is safe due to database-level transaction isolation
The code shows that all lock operations are performed through Prisma's transactional updates:
- Lock acquisition is done via atomic database updates in both dev and shared queue consumers
- Lock checks and status updates are performed within database transactions
- The
lockedAtfield is only modified through proper update operations with where clauses
The race condition window is effectively handled by the database's transaction isolation level, making the lock check and subsequent operations atomic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in lock handling
# Look for other services that might modify lockedAt
rg -A 5 "lockedAt.*="
# Check for transaction usage patterns
ast-grep --pattern 'transaction|$tx'
# Look for other expire or lock operations
rg -A 5 '\b(lock|expire|unlock).*Run'
Length of output: 22765
@trigger.dev/build
@trigger.dev/core
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
commit: |
Summary by CodeRabbit