Fix issue with heavy contention on TaskQueue updating concurrency limit #1653
Conversation
🦋 Changeset detectedLatest commit: 96bd406 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 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/createBackgroundWorker.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/triggerTask.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. references/v3-catalog/src/trigger/queues.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 pull request introduces changes to how concurrency limits are handled in task queues across multiple files. The primary modification allows setting the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ 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: 2
🧹 Nitpick comments (2)
references/v3-catalog/src/trigger/queues.ts (1)
78-78: Update log message to match queue name.The log message "named-queue 2" is inconsistent with the queue name "controller".
Apply this diff to update the log message:
- logger.info("named-queue 2"); + logger.info("controller");.changeset/funny-emus-pay.md (1)
5-5: Enhance the changeset description with more details.While the current description is clear, it would be helpful to add more context about:
- The previous behavior and why this change was needed
- The impact on existing queues
- Example usage of the new null concurrencyLimit feature
Here's a suggested enhancement:
-Allow setting concurrencyLimit to null to signal removing the concurrency limit on the queue +Allow setting concurrencyLimit to null to remove queue concurrency limits. Previously, queues always had a concurrency limit, which could cause heavy contention in high-throughput scenarios. This change allows unlimited concurrent operations when concurrencyLimit is explicitly set to null. Existing queues with undefined concurrencyLimit remain unchanged.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Did you mean “too”?
Context: ...tch --- Allow setting concurrencyLimit to null to signal removing the concurrency...(TOO_JJ_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/funny-emus-pay.md(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(3 hunks)apps/webapp/app/v3/services/replayTaskRun.server.ts(0 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(3 hunks)packages/core/src/v3/schemas/schemas.ts(1 hunks)references/v3-catalog/src/trigger/queues.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/v3/services/replayTaskRun.server.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/funny-emus-pay.md
[uncategorized] ~5-~5: Did you mean “too”?
Context: ...tch --- Allow setting concurrencyLimit to null to signal removing the concurrency...
(TOO_JJ_TO)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/core/src/v3/schemas/schemas.ts (1)
137-138: LGTM! Documentation updated to reflect schema change.The addition of
.nullable()toconcurrencyLimitaligns with the PR objective and maintains backward compatibility. The documentation comment has been appropriately updated.apps/webapp/app/v3/services/createBackgroundWorker.server.ts (3)
183-183: LGTM! Improved concurrency limit handling.The change correctly preserves
nullvalues from the queue configuration, aligning with the schema update and PR objective.
Line range hint
205-227: LGTM! Improved logging for concurrency limit changes.The added logging enhances observability by tracking concurrency limit updates and removals.
185-203: Verify race condition handling in high-concurrency scenarios.The two-step find-then-create approach could lead to race conditions where multiple workers attempt to create the same queue simultaneously. Consider using a transaction or handling unique constraint violations gracefully.
Run the following script to check for potential race conditions:
✅ Verification successful
Race conditions are prevented by database constraints
The code is safe from race conditions because:
- The
friendlyIdfield has a unique constraint in the Prisma schema- If concurrent queue creations occur, the database will enforce uniqueness
- Only one creation will succeed while others will fail with a constraint violation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unique constraint violations in task queue creation # Search for error logs related to task queue creation rg -A 5 "Failed to create task queue.*unique constraint" # Search for concurrent queue creation attempts rg -A 5 "Creating task queue.*already exists"Length of output: 106
Script:
#!/bin/bash # Check Prisma schema for task queue constraints and surrounding code context fd "schema.prisma" --exec cat {} \; | grep -A 10 "model TaskQueue" # Check for error handling and transaction usage ast-grep --pattern 'try { $$$ taskQueue = await prisma.taskQueue.create $$$ }' # Look for transaction usage rg -B 5 -A 5 "prisma.\$transaction" apps/webapp/app/v3/services/Length of output: 484
apps/webapp/app/v3/services/triggerTask.server.ts (2)
599-610: LGTM! Improved error handling for queue creation failures.The added error handling provides clear feedback when queue creation fails due to unique constraint violations.
Line range hint
453-474: Consider using a transaction for queue creation.Similar to
createBackgroundWorker.server.ts, the two-step find-then-create approach could lead to race conditions. Consider using a transaction to ensure atomicity.Apply this diff to use a transaction:
- let taskQueue = await tx.taskQueue.findFirst({ - where: { - runtimeEnvironmentId: environment.id, - name: queueName, - }, - }); - - if (!taskQueue) { - // handle conflicts with existing queues - taskQueue = await tx.taskQueue.create({ - data: { - friendlyId: generateFriendlyId("queue"), - name: queueName, - concurrencyLimit, - runtimeEnvironmentId: environment.id, - projectId: environment.projectId, - type: "NAMED", - }, - }); - } + let taskQueue = await tx.taskQueue.upsert({ + where: { + runtimeEnvironmentId_name: { + runtimeEnvironmentId: environment.id, + name: queueName, + }, + }, + create: { + friendlyId: generateFriendlyId("queue"), + name: queueName, + concurrencyLimit, + runtimeEnvironmentId: environment.id, + projectId: environment.projectId, + type: "NAMED", + }, + update: {}, + });
@trigger.dev/build
@trigger.dev/react-hooks
trigger.dev
@trigger.dev/rsc
@trigger.dev/sdk
@trigger.dev/core
commit: |
ericallam
force-pushed
the
hotfix/task-queue-contention
branch
from
b30697c to
96bd406
Compare
Summary by CodeRabbit
Release Notes
New Features
concurrencyLimittonull, allowing for more flexible queue management.Improvements
Schema Changes
concurrencyLimitto acceptnullvalues in queue configurations.