Detect ffmpeg OOM errors, added manual OutOfMemoryError #1694
Conversation
🦋 Changeset detectedLatest commit: 7066f05 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 |
WalkthroughThe changes add enhanced error handling for Out-of-Memory (OOM) scenarios. A new error class ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OOMTask as oomTask.run()
Client->>OOMTask: Call run({ succeedOnLargerMachine, ffmpeg, manual })
alt manual flag is true
OOMTask->>Client: Throw OutOfMemoryError
else ffmpeg flag is true
OOMTask->>Client: Throw "ffmpeg killed with SIGKILL" Error
else
OOMTask->>Client: Complete task normally
end
sequenceDiagram
participant Service as CompleteAttempt Service
participant OOM as isOOMError()
participant Manual as isManualOutOfMemoryError()
Service->>OOM: Check error details
OOM->>Manual: Validate for manual OOM conditions
alt Error qualifies as manual OOM
Manual-->>OOM: True
OOM-->>Service: Return detected OOM error
else
Manual-->>OOM: False
OOM-->>Service: Pass through error
end
Suggested Reviewers
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
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. packages/trigger-sdk/src/v3/index.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/hello-world/src/trigger/oom.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: 0
🧹 Nitpick comments (1)
references/hello-world/src/trigger/oom.ts (1)
34-40: Consider making the ffmpeg error message more specific to OOM.The implementation of both manual and ffmpeg OOM handlers is good, but the ffmpeg error message could be more explicit about memory issues.
Consider updating the ffmpeg error message to be more specific:
- throw new Error("ffmpeg was killed with signal SIGKILL"); + throw new Error("ffmpeg process terminated due to Out of Memory (killed with SIGKILL)");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/eighty-spies-knock.md(1 hunks)apps/webapp/app/v3/services/completeAttempt.server.ts(2 hunks)packages/core/src/v3/errors.ts(2 hunks)packages/trigger-sdk/src/v3/index.ts(1 hunks)references/hello-world/src/trigger/oom.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/v3/services/completeAttempt.server.ts
[error] 719-719: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
packages/trigger-sdk/src/v3/index.ts (1)
34-34: LGTM! Export of OutOfMemoryError.The new error type is correctly exported, maintaining the alphabetical order in the export list.
apps/webapp/app/v3/services/completeAttempt.server.ts (2)
14-14: LGTM! Import of isManualOutOfMemoryError.The import aligns with the enhanced error handling capabilities.
694-728: LGTM! Enhanced OOM error detection.The implementation adds comprehensive OOM error detection for:
- k8s scenarios with SIGKILL signals
- ffmpeg-specific OOM errors
- Manual OOM errors
This improves the robustness of memory-related error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 719-719: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/v3/errors.ts (4)
57-57: LGTM! Constant for OOM error message.The constant ensures consistency in error messages across the codebase.
59-68: LGTM! OutOfMemoryError class implementation.The class is well-documented and follows the error class pattern used throughout the codebase.
70-77: LGTM! isManualOutOfMemoryError implementation.The function provides a clean way to detect manual OOM errors.
588-593: LGTM! Enhanced error handling in taskRunErrorEnhancer.The implementation correctly integrates manual OOM error handling into the existing error enhancement logic.
.changeset/eighty-spies-knock.md (2)
4-6: LGTM! Clear and concise description.The description accurately captures the main changes: ffmpeg OOM error detection and manual OutOfMemoryError handling. This aligns well with the PR objectives.
1-3: Verify the semantic versioning choice.The patch version bump for "@trigger.dev/sdk" seems appropriate as the changes focus on bug fixes and error handling improvements without breaking existing functionality.
✅ Verification successful
🌐 Web query:
What changes warrant a patch version bump in semantic versioning?💡 Result:
A patch version bump in semantic versioning (SemVer) is reserved for changes that introduce backward-compatible bug fixes without altering existing functionality or public APIs[1][5][8]. These updates typically address internal issues while maintaining compatibility with previous versions.
Key Criteria for Patch Version Increments
Bug Fixes:
- Resolve incorrect behavior without modifying public APIs[1][6][8].
- Example: Fixing a crash in data validation logic[8].
Internal Optimizations:
- Performance improvements or code refactoring not visible to API consumers[1][4].
- Example: Optimizing database query execution time[6].
Non-Feature Dependency Updates:
- Upgrading dependencies (e.g., dev tools or libraries) without exposing new functionality[2][9].
- Example: Patching a security vulnerability in an underlying logging library[9].
When Not to Use a Patch Bump
- Adding features → Minor version
- Changing/removing public APIs → Major version
- Dependency updates introducing new capabilities → Minor version[2][4]
For instance:
- Fixing a calculation error in an existing function →
1.0.**1**(patch)[8]- Adding a new API endpoint →
1.**1**.0(minor)- Renaming an API parameter →
2.0.0 (major)[5]Patch releases allow users to safely update while trusting no breaking changes will occur[1][5].
Citations:
- 1: https://semver.org
- 2: https://8hob.io/posts/semantic-versioning-npm-packages-after-dependency-update/
- 3: https://semver.org/spec/v1.0.0.html
- 4: https://forum.graphviz.org/t/version-numbering-going-forward/237
- 5: https://designsystem.morningstar.com/getting-started/versioning-and-breaking-changes/
- 6: https://igventurelli.io/understanding-semantic-versioning-a-guide-for-developers/
- 7: https://forum.getodk.org/t/proposal-change-collect-versioning-from-major-minor-patch-to-year-release/32572
- 8: https://testsuite.io/software-version-numbering-rules
- 9: FAQ or guidance for changes in underlying dependency stack? semver/semver#791
Semantic version bump verification complete.
The changes in this changeset are limited to error handling improvements related to OutOfMemoryError and bug fixes, which are consistent with a patch-level update in semantic versioning.references/hello-world/src/trigger/oom.ts (3)
1-1: LGTM! Import of OutOfMemoryError aligns with PR objectives.The addition of
OutOfMemoryErrorimport from the SDK is appropriate for implementing the enhanced OOM error handling.
5-12: Well-structured task configuration with appropriate OOM retry policy.The task configuration is well-designed with:
- Initial execution on a micro machine
- Automatic upgrade to small-1x machine on OOM
- Specific retry policy for OOM scenarios
13-20: LGTM! Method signature changes enhance OOM testing capabilities.The new optional parameters provide flexible control over OOM scenarios:
ffmpeg: For testing ffmpeg-specific OOM errorsmanual: For manual OOM triggeringsucceedOnLargerMachine: Now optional with sensible default
* Detect ffmpeg OOM errors, added manual OutOfMemoryError * Create eighty-spies-knock.md
* Uses image cards for the frameworks * Removes old snippets * New AI agents side menu section * WIP adding new ai agent pages * Better overview page * More copy added to the agent example pages * Copy improvements * Removes “Creating a project” page and side menu section * Fixes broken links * Updates to the latest Mintlify version, fixes issues, changes theme * Adds descriptions to the main dropdown menu items * Reformatted Introduction docs ‘landing page’ * Retry heartbeat timeouts by putting back in the queue (#1689) * If there’s a heartbeat error and no attempts we put it back in the queue to try again * When nacking, return whether it was put back in the queue or not * Try and nack, if it fails then fail the run * Consolidated switch statement * Fail executing/retrying runs * OOM retrying on larger machines (#1691) * OOM retrying on larger machines * Create forty-windows-shop.md * Update forty-windows-shop.md * Only retry again if the machine is different from the original * Kubernetes OOMs appear as non-zero sigkills, adding support for treating these as OOMs * Complete the original attempt span if retrying due to an OOM * Revert "Complete the original attempt span if retrying due to an OOM" This reverts commit 5f652c6. * chore: Update version for release (#1666) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Release 3.3.14 * Set machine when triggering docs * Batch queue runs that are waiting for deploy (#1693) * Detect ffmpeg OOM errors, added manual OutOfMemoryError (#1694) * Detect ffmpeg OOM errors, added manual OutOfMemoryError * Create eighty-spies-knock.md * Improved the machines docs, including the new OutOfMemoryError * chore: Update version for release (#1695) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Release 3.3.15 * Create new partitioned TaskEvent table, and switch to it gradually as new runs are created (#1696) * Create new partitioned TaskEvent table, and switch to it gradually as new runs are created * Add env var for partition window in seconds * Make startCreatedAt required in task event store * Don't create an attempt if the run is final, batchTriggerAndWait bad continue fix (#1698) * WIP fix for ResumeAttemptService selecting the wrong attempt (which has no error or output) * Don’t create an attempt if the run is already in a final status * Don’t get all the columns for the query. Improved the logging. * Added a log to the batch example * Filter out the undefined values * Fix missing logs on child runs by using the root task run createdAt if it exists (#1697) * Provider changes to support image cache (#1700) * add env var for additional pull secrets * make static images configurable * optional image prefixes * optional labels with sample rates * add missing core paths * remove excessive logs * Fix run container exits after OOM retries (#1701) * remove unused imports * tell run to exit before force requeue * handle exit for case where we already retried after oom * improve retry span and add machine props * don't try to exit run in dev * Upgrade local dev to use electric beta.15 (#1699) * Text fixes * Removed pnpm files --------- Co-authored-by: Matt Aitken <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Eric Allam <[email protected]> Co-authored-by: nicktrn <[email protected]>
Added
OutOfMemoryErrorthat can be thrown. This causes an Out Of Memory error on the run (if it's uncaught). This can be useful if you use a native package that detects it's run out of memory but doesn't kill Node.jsAlso fixed a bug where ffmpeg's weird OOM error is now detected.
Summary by CodeRabbit
New Features
Chores