OOM retrying on larger machines #1691
Conversation
🦋 Changeset detectedLatest commit: 2de9e8d 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 |
|
Warning Rate limit exceeded@matt-aitken has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request implements a new mechanism for retrying tasks that encounter Out Of Memory (OOM) errors. It revises method signatures and error handling logic in task run management and task attempt services. Changes include a new helper method to compute retry configurations for OOM scenarios, updates to the machine configuration format in both documentation and schemas, and the introduction of a new task that simulates OOM conditions for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Task Runner
participant CAS as CompleteAttemptService
participant FTR as FailedTaskRunRetryHelper
TR->>CAS: Execute task attempt
CAS->>CAS: Call isOOMError(error)
alt OOM Error Detected
CAS->>FTR: getExecutionRetry(taskRun, execution)
FTR->>FTR: Execute getRetryConfig(taskRun, execution)
FTR-->>CAS: Return retry configuration (new machine preset)
CAS->>TR: Requeue task with forceRequeue flag
else Normal Error
CAS->>TR: Proceed with standard retry logic
end
Possibly related PRs
Suggested reviewers
Poem
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 (4)
references/hello-world/src/trigger/oom.ts (2)
12-16: Consider clarifying the parameter name.
succeedOnLargerMachineis understandable for demonstration purposes. However, if you plan to expand usage beyond a simple test scenario, a more descriptive parameter name (e.g.,shouldSucceedIfMachineIsLarger) might improve readability.
17-18: Optional: Make timeout configurable.
Currently, the 2-second wait might introduce unnecessary delays in certain environments. If you plan to keep this in production, consider making the timeout configurable or documented.apps/webapp/app/v3/services/completeAttempt.server.ts (1)
253-285: Revisiting machinePreset on OOM.
Assigning a newmachinePresetfor OOM is valid; however, confirm that permanently updating the DB aligns with the desired design. If the intention is to revert back after just one attempt, additional logic might be needed.docs/machines.mdx (1)
29-34: Consider improving error message formatting.The error message could be more readable with proper punctuation.
Apply this diff to improve readability:
-TASK_PROCESS_OOM_KILLED. Your task ran out of memory. Try increasing the machine specs. If this doesn't fix it there might be a memory leak. +TASK_PROCESS_OOM_KILLED: Your task ran out of memory. Try increasing the machine specs. If this doesn't fix it, there might be a memory leak.🧰 Tools
🪛 LanguageTool
[typographical] ~33-~33: Consider adding a comma.
Context: ...he machine specs. If this doesn't fix it there might be a memory leak. If this happen...(IF_THERE_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/forty-windows-shop.md(1 hunks)apps/webapp/app/v3/failedTaskRun.server.ts(3 hunks)apps/webapp/app/v3/services/completeAttempt.server.ts(9 hunks)docs/machines.mdx(2 hunks)packages/core/src/v3/schemas/schemas.ts(2 hunks)packages/core/src/v3/types/tasks.ts(1 hunks)references/hello-world/src/trigger/oom.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/machines.mdx
[typographical] ~33-~33: Consider adding a comma.
Context: ...he machine specs. If this doesn't fix it there might be a memory leak. If this happen...
(IF_THERE_COMMA)
[uncategorized] ~35-~35: Possible missing comma found.
Context: ...ight be a memory leak. If this happens regularly you need to either optimize the memory-...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
references/hello-world/src/trigger/oom.ts (5)
1-2: Imports look good.
No concerns. The imports from@trigger.dev/sdk/v3andtimers/promisesare appropriate and align with the usage below.
4-11: Nice approach for specifying out-of-memory retry logic.
Defining theoutOfMemoryretry option with an alternate machine is a great demonstration of how to handle OOM errors by switching to a higher configuration.
19-24: Logic is straightforward.
The success path is well-defined by checking the current machine and thesucceedOnLargerMachineflag. This makes the behavior transparent.
26-31: Intentionally triggering OOM.
Using a while loop to continuously double a string will reliably push memory usage. This is fine for a demonstration, but be cautious if run in environments with limited safeguards.
32-39: Catch block duplicates OOM attempt.
The second infinite loop further ensures an OOM scenario. As this is purposeful, just be careful in a production environment to avoid system instability.apps/webapp/app/v3/services/completeAttempt.server.ts (8)
4-4: ImportedTaskRunErrorusage looks correct.
Good move to leverage the typed error, making it easier to discern internal error subtypes.
239-249: Ensuring retry logic is inferred during system failures.
RecomputingexecutionRetryviaFailedTaskRunRetryHelper.getExecutionRetry()if none is provided is a clean approach to unify retry logic.
249-251: IntroducingretriableErrorandisOOMRetry.
Naming these variables explicitly clarifies branching logic for different error scenarios.
299-299: Forcing requeue on OOM.
TyingforceRequeueto theisOOMRetryflag ensures these attempts enter the queue. This appears consistent with the new retry mechanism.
421-429: Introduction offorceRequeueparameter.
Allowing the caller to forcibly re-queue improves flexibility in orchestrating retry attempts.
479-483: Conditional re-queue logic.
This ensures a fallback path if the checkpoint-based approach is unavailable. The branching logic is clear and robust.
516-525: Extending#retryAttemptwithforceRequeue.
The new parameter nicely complements the existing retry logic, providing a structured way to push attempts through the queue flow.
688-695:isOOMErrorfunction.
DetectingTASK_PROCESS_OOM_KILLEDorTASK_PROCESS_MAYBE_OOM_KILLEDis an elegant and straightforward approach. Consider verifying if there are any other relevant error codes to catch.Below is a script to search the codebase for additional OOM-related codes:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Checking for OOM or 'killed' references in code to confirm coverage. rg -i 'oom|killed'Length of output: 65557
No Additional OOM Error Codes Found.
The repository search only returns references to "TASK_PROCESS_OOM_KILLED" and "TASK_PROCESS_MAYBE_OOM_KILLED" (with "OOMKilled" mapping to TASK_PROCESS_OOM_KILLED in the Kubernetes provider). Based on our grep, there aren’t any other OOM-related error codes to address, so the current implementation of theisOOMErrorfunction appears sufficient.packages/core/src/v3/schemas/schemas.ts (2)
3-3: New import for MachinePresetName.
This import is necessary for the new OOM configuration below. Good addition.
99-108:outOfMemoryconfiguration is a valuable extension.
This new optional property inRetryOptionsallows specifying a different machine for OOM errors, aligning well with the PR objectives.apps/webapp/app/v3/failedTaskRun.server.ts (2)
183-220: Improved error handling in getExecutionRetry method.The method has been simplified by removing the async keyword and adding proper error handling with logging.
222-299: Verify retry configuration parsing.The new
getRetryConfigmethod includes robust error handling and version checking for SDK compatibility. However, we should verify that the retry configuration is correctly parsed.Run this script to check retry configuration parsing:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check retry configuration parsing in the codebase. # Test: Search for retry configuration usage rg -A 5 'retry: \{.*outOfMemory'Length of output: 34
Manual Verification Needed: Confirm Retry Configuration Parsing
- The initial shell script produced no output for the pattern used.
- The
getRetryConfigimplementation appears robust, usingRetryOptions.nullable().safeParsealong with proper error and version checks.- However, we did not find automated evidence (e.g., specific configuration keys like
outOfMemory) validating that the retry configuration is parsed as expected.- I recommend manually verifying the retry configuration behavior (or adding targeted tests) to ensure that various configurations are correctly parsed and that fallback behaviors are triggered as intended.
packages/core/src/v3/types/tasks.ts (1)
205-219: Updated machine configuration documentation and type.The machine configuration has been simplified to use presets, improving usability. The documentation link to machine specifications enhances clarity.
.changeset/forty-windows-shop.md (1)
1-6: LGTM! Clear and concise changeset.The patch version bump is appropriate for this feature addition.
docs/machines.mdx (3)
8-16: Clear example of machine configuration.The example demonstrates the simplified machine configuration using presets.
41-54: Comprehensive example of OOM retry configuration.The example clearly demonstrates how to configure OOM retry with a larger machine.
56-59: Clear note about retry behavior.The note effectively clarifies that OOM retry is temporary and doesn't permanently change the machine configuration.
Sometimes you might see one of your runs fail with an "Out Of Memory" error.
If this happens regularly you need to either optimize the memory-efficiency of your code, or increase the machine.
If you are seeing rare OOM errors, you can add a setting to your task to retry with a large machine if you get an OOM error:
Summary by CodeRabbit
New Features
Documentation