Throw an error for concurrent waits (with nice docs link) #1618
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
packages/core/src/v3/errors.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. WalkthroughThis pull request introduces a comprehensive solution for handling parallel waits in the task execution system. The changes span multiple files and focus on preventing concurrent wait operations across both development and production runtime managers. A new error mechanism is implemented to clearly communicate and prevent multiple simultaneous wait calls, with updated documentation explaining the limitations. The modifications ensure that only one wait operation can be executed at a time, throwing a specific error when multiple waits are attempted. Changes
Sequence DiagramsequenceDiagram
participant Task as Task Executor
participant Runtime as Runtime Manager
participant Wait as Wait Prevention Mechanism
Task->>Wait: Attempt wait operation
alt No active wait
Wait-->>Runtime: Set wait flag
Runtime->>Task: Execute wait
Task->>Runtime: Complete wait
Runtime-->>Wait: Clear wait flag
else Wait already in progress
Wait->>Task: Throw TASK_DID_CONCURRENT_WAIT error
end
Possibly related PRs
Poem
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 (5)
packages/core/src/v3/runtime/preventMultipleWaits.ts (1)
4-5: Enhance error message clarity.While the current message explains what's not supported, it could be more helpful by suggesting alternatives.
Consider updating the message to:
- "Parallel waits are not supported, e.g. using Promise.all() around our wait functions."; + "Parallel waits are not supported (e.g. using Promise.all() with wait functions). Instead, use sequential waits or consider using our built-in parallel task execution features.";packages/core/src/v3/errors.ts (1)
15-44: Enhance class documentation with more details.While the basic JSDoc is present, it would be helpful to add more detailed documentation about the parameters and their purposes.
Add this enhanced documentation:
/** * If you throw this, it will get converted into an INTERNAL_ERROR + * + * @param code - The specific error code from TaskRunErrorCodes + * @param message - Optional custom error message + * @param showStackTrace - Whether to include stack trace in the error (default: true) + * @param skipRetrying - Whether to skip retrying this error (default: false) */docs/troubleshooting.mdx (3)
85-85: Fix grammatical error in the explanation.Change "more that" to "more than" for correct grammar.
-In the current version, you can't perform more that one "wait" in parallel. +In the current version, you can't perform more than one "wait" in parallel.🧰 Tools
🪛 LanguageTool
[misspelling] ~85-~85: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...(THAT_THAN)
94-94: Enhance clarity of the technical explanation.The current explanation could be more precise and avoid implying this is a temporary limitation.
-This restriction exists because we suspend the task server after a wait, and resume it when the wait is done. At the moment, if you do more than one wait, the run will never continue when deployed, so we throw this error instead. +This restriction exists because we suspend the task server after a wait, and resume it when the wait is done. If multiple waits are attempted simultaneously, the run cannot continue when deployed, so we throw this error to prevent the task from hanging.🧰 Tools
🪛 LanguageTool
[style] ~94-~94: For conciseness, consider replacing this expression with an adverb.
Context: ...t, and resume it when the wait is done. At the moment, if you do more than one wait, the run ...(AT_THE_MOMENT)
96-96: Consider adding a code example to illustrate the recommended approach.While the guidance and documentation link are helpful, a code example comparing the incorrect usage of
Promise.allwith the recommended approach using built-in functions would make this clearer for users.Consider adding a code example like this:
The most common situation this happens is if you're using `Promise.all` around some of our wait functions. Instead of doing this use our built-in functions for [triggering tasks](/triggering#triggering-from-inside-another-task). We have functions that allow you to trigger different tasks in parallel. + +```ts +// ❌ Don't do this +await Promise.all([ + wait.for("1 minute"), + wait.for("2 minutes") +]); + +// ✅ Do this instead +await task.batchTrigger([ + { name: "task1", payload: { /* ... */ } }, + { name: "task2", payload: { /* ... */ } } +]); +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/troubleshooting.mdx(1 hunks)packages/core/src/v3/errors.ts(4 hunks)packages/core/src/v3/links.ts(1 hunks)packages/core/src/v3/runtime/devRuntimeManager.ts(2 hunks)packages/core/src/v3/runtime/index.ts(1 hunks)packages/core/src/v3/runtime/preventMultipleWaits.ts(1 hunks)packages/core/src/v3/runtime/prodRuntimeManager.ts(4 hunks)packages/core/src/v3/schemas/common.ts(2 hunks)packages/core/src/v3/workers/taskExecutor.ts(2 hunks)references/hello-world/src/trigger/parallel-waits.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/v3/runtime/index.ts
🧰 Additional context used
🪛 LanguageTool
docs/troubleshooting.mdx
[misspelling] ~85-~85: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...
(THAT_THAN)
[style] ~94-~94: For conciseness, consider replacing this expression with an adverb.
Context: ...t, and resume it when the wait is done. At the moment, if you do more than one wait, the run ...
(AT_THE_MOMENT)
⏰ 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: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
references/hello-world/src/trigger/parallel-waits.ts (2)
11-14: This code block effectively demonstrates the concurrent wait scenario that should trigger an error.The parallel execution of
wait.for()andwait.until()usingPromise.all()is exactly the scenario we want to prevent. This test case will help verify that our error handling works correctly.
17-20: Consider preventing parallel task calls as well.While the focus is on preventing parallel waits, we should also consider if parallel task calls using
triggerAndWaitandbatchTriggerAndWaitshould be prevented, as they both contain "wait" in their names and could potentially cause similar issues.Let's verify if these methods internally use wait operations:
✅ Verification successful
Parallel task calls with triggerAndWait/batchTriggerAndWait should indeed be prevented
Both methods are documented wait operations that suspend the task server, and using them in parallel (e.g., with Promise.all) can cause runs to never continue when deployed. This is consistent with the platform's handling of other wait operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for wait operations within triggerAndWait and batchTriggerAndWait implementations ast-grep --pattern 'triggerAndWait($$$) { $$$wait$$$ }' ast-grep --pattern 'batchTriggerAndWait($$$) { $$$wait$$$ }'Length of output: 120
Script:
#!/bin/bash # Search for method definitions and implementations ast-grep --pattern 'async triggerAndWait($_) { $$$ }' ast-grep --pattern 'async batchTriggerAndWait($_) { $$$ }' # Backup search using ripgrep rg "triggerAndWait|batchTriggerAndWait" -A 5 -B 5 # Search for interface definitions ast-grep --pattern 'interface $_ { $$$triggerAndWait$$$batchTriggerAndWait$$$ }'Length of output: 69916
packages/core/src/v3/links.ts (1)
15-17: LGTM! Documentation link follows the established pattern.The new troubleshooting section with the concurrent waits documentation link is well-structured and consistent with the existing format.
packages/core/src/v3/runtime/preventMultipleWaits.ts (1)
7-29: LGTM! Robust implementation with proper cleanup.The implementation effectively prevents concurrent waits and ensures the flag is always reset:
- Uses a boolean flag for tracking wait state
- Throws a clear error for concurrent waits
- Ensures cleanup in finally block
- Includes appropriate error properties
packages/core/src/v3/runtime/devRuntimeManager.ts (2)
21-21: LGTM! Prevention mechanism properly initialized.The
_preventMultipleWaitsproperty is correctly initialized using the factory function.
36-52: LGTM! Comprehensive integration of prevention mechanism.The prevention mechanism is properly integrated into both
waitForTaskandwaitForBatchmethods:
- Wraps the entire async operation
- Maintains existing promise creation and resolution logic
- Preserves state management with Maps
- Properly handles metadata flushing
Also applies to: 60-91
packages/core/src/v3/runtime/prodRuntimeManager.ts (1)
25-25: LGTM! Good initialization of the prevention mechanism.The private member is correctly initialized using the imported function.
packages/core/src/v3/schemas/common.ts (1)
174-174: LGTM! Good addition of the new error code.The error code is appropriately added to the enum and the type definition is correctly updated.
Also applies to: 183-183
packages/core/src/v3/workers/taskExecutor.ts (1)
539-543: LGTM! Good enhancement of error handling.The error handling is properly updated to handle InternalError instances and respect the skipRetrying flag.
packages/core/src/v3/errors.ts (1)
481-488: LGTM! Clear error message and helpful documentation link.The error message is concise and the documentation link will help users understand and resolve the issue.
docs/troubleshooting.mdx (1)
87-93: LGTM! Clear and comprehensive list of affected operations.The list effectively covers all wait operations and follows consistent formatting.
| return this._preventMultipleWaits(async () => { | ||
| if (!params.runs.length) { | ||
| return Promise.resolve({ id: params.id, items: [] }); | ||
| } | ||
|
|
||
| const promise = Promise.all( | ||
| params.runs.map((runId) => { | ||
| return new Promise<TaskRunExecutionResult>((resolve, reject) => { | ||
| this._taskWaits.set(runId, { resolve }); | ||
| }); | ||
| }) | ||
| ); | ||
|
|
||
| await this.ipc.send("WAIT_FOR_BATCH", { | ||
| batchFriendlyId: params.id, | ||
| runFriendlyIds: params.runs, | ||
| }); | ||
|
|
||
| const results = await promise; | ||
|
|
||
| clock.reset(); | ||
|
|
||
| return { | ||
| id: params.id, | ||
| items: results, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding cleanup for task waits on error.
The waitForBatch method should clean up the task waits from the _taskWaits map if an error occurs during execution to prevent memory leaks.
Apply this diff to add cleanup:
return this._preventMultipleWaits(async () => {
if (!params.runs.length) {
return Promise.resolve({ id: params.id, items: [] });
}
+ const addedWaits: string[] = [];
const promise = Promise.all(
params.runs.map((runId) => {
return new Promise<TaskRunExecutionResult>((resolve, reject) => {
this._taskWaits.set(runId, { resolve });
+ addedWaits.push(runId);
});
})
);
- await this.ipc.send("WAIT_FOR_BATCH", {
- batchFriendlyId: params.id,
- runFriendlyIds: params.runs,
- });
-
- const results = await promise;
+ try {
+ await this.ipc.send("WAIT_FOR_BATCH", {
+ batchFriendlyId: params.id,
+ runFriendlyIds: params.runs,
+ });
+
+ const results = await promise;
+ clock.reset();
+
+ return {
+ id: params.id,
+ items: results,
+ };
+ } catch (error) {
+ // Clean up task waits on error
+ addedWaits.forEach(runId => this._taskWaits.delete(runId));
+ throw error;
+ }
- clock.reset();
-
- return {
- id: params.id,
- items: results,
- };
});📝 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.
| return this._preventMultipleWaits(async () => { | |
| if (!params.runs.length) { | |
| return Promise.resolve({ id: params.id, items: [] }); | |
| } | |
| const promise = Promise.all( | |
| params.runs.map((runId) => { | |
| return new Promise<TaskRunExecutionResult>((resolve, reject) => { | |
| this._taskWaits.set(runId, { resolve }); | |
| }); | |
| }) | |
| ); | |
| await this.ipc.send("WAIT_FOR_BATCH", { | |
| batchFriendlyId: params.id, | |
| runFriendlyIds: params.runs, | |
| }); | |
| const results = await promise; | |
| clock.reset(); | |
| return { | |
| id: params.id, | |
| items: results, | |
| }; | |
| return this._preventMultipleWaits(async () => { | |
| if (!params.runs.length) { | |
| return Promise.resolve({ id: params.id, items: [] }); | |
| } | |
| const addedWaits: string[] = []; | |
| const promise = Promise.all( | |
| params.runs.map((runId) => { | |
| return new Promise<TaskRunExecutionResult>((resolve, reject) => { | |
| this._taskWaits.set(runId, { resolve }); | |
| addedWaits.push(runId); | |
| }); | |
| }) | |
| ); | |
| try { | |
| await this.ipc.send("WAIT_FOR_BATCH", { | |
| batchFriendlyId: params.id, | |
| runFriendlyIds: params.runs, | |
| }); | |
| const results = await promise; | |
| clock.reset(); | |
| return { | |
| id: params.id, | |
| items: results, | |
| }; | |
| } catch (error) { | |
| // Clean up task waits on error | |
| addedWaits.forEach(runId => this._taskWaits.delete(runId)); | |
| throw error; | |
| } | |
| }); |
@trigger.dev/core
trigger.dev
@trigger.dev/rsc
@trigger.dev/build
@trigger.dev/sdk
@trigger.dev/react-hooks
commit: |
Problem
In the current version, you can't perform more that one "wait" in parallel.
Waits include:
wait.for()wait.until()task.triggerAndWait()task.batchTriggerAndWait()waitin the name.This restriction exists because we suspend the task server after a wait, and resume it when the wait is done. At the moment, if you do more than one wait, the run will never continue when deployed, so we throw this error instead.
The most common situation this happens is if you're using
Promise.allaround some of our wait functions. Instead of doing this use our built-in functions for triggering tasks. We have functions that allow you to trigger different tasks in parallel.Solution
Up until now we had some docs about this, but we didn't actually do anything at runtime to stop you doing this.
This PR adds an error when we detect two waits are called simultaneously. It has a link to a new section of our troubleshooting docs.
It also adds a nicer way of preventing retrying runs and hydrating errors into JSON.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes