-
-
Notifications
You must be signed in to change notification settings - Fork 836
fix(engine) truncate errors before storing them on a run and waitpoint output #2552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
1920-1926
: Improve truncation UX and edge-case handling.Append an ellipsis when truncated and guard zero/negative lengths.
Apply this diff:
-function truncateString(str: string | undefined, maxLength: number): string { - if (!str) { - return ""; - } - - return str.slice(0, maxLength); -} +function truncateString(str: string | undefined, maxLength: number): string { + if (!str) return ""; + if (maxLength <= 0) return ""; + if (str.length <= maxLength) return str; + const limit = Math.max(1, maxLength); + return str.slice(0, limit - 1) + "…"; +}
1549-1552
: Add error output type and emittruncatedError
inrunFailed
eventinternal-packages/run-engine/src/engine/systems/runAttemptSystem.ts @@ 1549,1552 - await this.waitpointSystem.completeWaitpoint({ - id: run.associatedWaitpoint.id, - output: { value: JSON.stringify(truncatedError), isError: true }, - }); + await this.waitpointSystem.completeWaitpoint({ + id: run.associatedWaitpoint.id, + output: { value: JSON.stringify(truncatedError), type: "application/json", isError: true }, + }); @@ 1554,1559 - this.$.eventBus.emit("runFailed", { - time: failedAt, - run: { - id: runId, - status: run.status, - spanId: run.spanId, - error, + this.$.eventBus.emit("runFailed", { + time: failedAt, + run: { + id: runId, + status: run.status, + spanId: run.spanId, + error: truncatedError,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}
: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
🧠 Learnings (1)
📚 Learning: 2025-08-14T12:13:20.455Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2392
File: packages/cli-v3/src/utilities/gitMeta.ts:195-218
Timestamp: 2025-08-14T12:13:20.455Z
Learning: In the GitMeta schema (packages/core/src/v3/schemas/common.ts), all fields are intentionally optional to handle partial data from various deployment contexts (local, GitHub Actions, GitHub App). Functions like getGitHubAppMeta() are designed to work with missing environment variables rather than validate their presence.
Applied to files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
🧬 Code graph analysis (1)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
packages/core/src/v3/schemas/common.ts (2)
TaskRunError
(200-205)TaskRunError
(207-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
1479-1490
: Good call: truncate before persisting the error.This prevents oversized payloads in the taskRun.error column.
1898-1909
: Implement truncation for STRING_ERROR.raw and replace magic numbers with named constants. zod schemas for TaskRunBuiltInError (name, message, stackTrace) and TaskRunStringError (raw) require string fields, and the proposed diff using ERROR_NAME_MAX and ERROR_TEXT_MAX satisfies them.
Truncate run error message and stacktrace to 16kb