fix(webapp): worker actions now catch service validation errors and respond properly #2481
Conversation
…espond properly This also stops all the unnecessary error logging when throwing ServiceValidationErrors
|
WalkthroughThis PR standardizes validation errors by replacing the local EngineServiceValidationError with ServiceValidationError across runEngine payloads, queues, validators, and trigger service. The old EngineServiceValidationError class is removed from the webapp, while the internal run-engine package now re-exports ServiceValidationError as EngineServiceValidationError for external use. API route builders add explicit handling for both error types, returning JSON with a status (default 422). The API route importing EngineServiceValidationError now sources it from @internal/run-engine. Run-engine locking logic rethrows ServiceValidationError without logging metrics. A minor import order change occurs in engine/index.ts. No public signatures change beyond the new export alias. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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 (21)
internal-packages/run-engine/src/engine/locking.ts (1)
183-188: Avoid double-counting lock duration on failuresFailure duration is recorded here and also in #acquireAndExecute finally; that can double count. Prefer recording only inside #acquireAndExecute.
Apply:
- const lockDuration = performance.now() - lockStartTime; - this.lockDurationHistogram.record(lockDuration, { - [SemanticAttributes.LOCK_TYPE]: name, - [SemanticAttributes.LOCK_SUCCESS]: "false", - }); + // Duration recorded in #acquireAndExecute.finally; avoid duplicate metric here.Also applies to: 396-401
apps/webapp/app/runEngine/concerns/payloads.server.ts (1)
38-40: Preserve diagnostics in metadata for easier debuggingInclude filename/size/env in the error metadata so callers have actionable context.
Apply:
- throw new ServiceValidationError("Failed to upload large payload to object store", 500); // This is retryable + throw new ServiceValidationError( + "Failed to upload large payload to object store", + 500, + { filename, size, envId: request.environment.id } + ); // This is retryableapps/webapp/app/runEngine/concerns/queues.server.ts (8)
48-53: Return a 404 for missing specified queueSurface the correct status for upstream handlers and clients.
- throw new ServiceValidationError( + throw new ServiceValidationError( `Specified queue '${specifiedQueueName}' not found or not associated with locked version '${lockedBackgroundWorker.version ?? "<unknown>"}'.` - ); + , 404);
71-76: Return a 404 when the task isn’t on the locked versionAligns status with “not found”.
- throw new ServiceValidationError( + throw new ServiceValidationError( `Task '${request.taskId}' not found on locked version '${lockedBackgroundWorker.version ?? "<unknown>"}'.` - ); + , 404);
86-90: Consider 422 (bad configuration) or 500 (server) for missing default queueUse a non-404 status to reflect config/state issue; 422 keeps it client-visible without implying server fault.
- throw new ServiceValidationError( + throw new ServiceValidationError( `Default queue configuration for task '${request.taskId}' missing on locked version '${lockedBackgroundWorker.version ?? "<unknown>"}'.` - ); + , 422);
100-102: Use 404 when lockToVersion references a non-existent workerImproves accuracy of responses.
- throw new ServiceValidationError( + throw new ServiceValidationError( `Task locked to version '${request.body.options.lockToVersion}', but no worker found with that version.` - ); + , 404);
112-117: Re-sanitize fallback to guarantee a non-empty queue nameGuard against sanitize producing an empty string twice.
- if (!sanitizedQueueName) { - queueName = sanitizeQueueName(`task/${request.taskId}`); // Fallback if sanitization results in empty - } else { - queueName = sanitizedQueueName; - } + if (!sanitizedQueueName) { + const fallback = sanitizeQueueName(`task/${request.taskId}`); + queueName = fallback || "task/default"; + } else { + queueName = sanitizedQueueName; + }
223-225: Preserve status when rethrowing known validation errorsIf the service threw ServiceValidationError, propagate it; otherwise wrap generically.
- if (error) { - throw new ServiceValidationError(error.message); - } + if (error) { + if (error instanceof ServiceValidationError) throw error; + throw new ServiceValidationError(error.message); + }
228-229: Use 404 for missing worker groupMatch semantics for “not found”.
- throw new ServiceValidationError("No worker group found"); + throw new ServiceValidationError("No worker group found", 404);
159-175: Replace console.log with logger to keep logs structuredStay consistent with the rest of the module.
- console.log("Failed to get queue name: No task found", { + logger.debug("Failed to get queue name: No task found", { taskId, environmentId: environment.id, }); ... - console.log("Failed to get queue name: No queue found", { + logger.debug("Failed to get queue name: No queue found", { taskId, environmentId: environment.id, queueConfig: task.queueConfig, });Also applies to: 168-175
internal-packages/run-engine/src/index.ts (1)
2-6: Optionally re-export the base name too for gradual migration.Expose
ServiceValidationErroralongside the alias so call sites can migrate without reaching into subpaths.Apply:
export { RunDuplicateIdempotencyKeyError, RunOneTimeUseTokenError, + ServiceValidationError, ServiceValidationError as EngineServiceValidationError, } from "./engine/errors.js";apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (3)
1049-1055: Deduplicate the two instanceof checks.Combine the branches to reduce repetition and guard against subtle ordering mistakes.
Apply:
- if (error instanceof EngineServiceValidationError) { - return json({ error: error.message }, { status: error.status ?? 422 }); - } - - if (error instanceof ServiceValidationError) { - return json({ error: error.message }, { status: error.status ?? 422 }); - } + if ( + error instanceof EngineServiceValidationError || + error instanceof ServiceValidationError + ) { + return json({ error: (error as any).message }, { status: (error as any).status ?? 422 }); + }
877-877: Drop console.error and rely on structured logger.Avoid double-logging and keep output consistent with
logger.error.Apply:
- console.error("Error in API route:", error);
733-749: Mirror ServiceValidationError handling in public action routes.For parity with worker actions, return 4xx for validation errors before logging.
Apply:
} catch (error) { try { if (error instanceof Response) { return await wrapResponse(request, error, corsStrategy !== "none"); } + if ( + error instanceof EngineServiceValidationError || + error instanceof ServiceValidationError + ) { + return await wrapResponse( + request, + json({ error: (error as any).message }, { status: (error as any).status ?? 422 }), + corsStrategy !== "none" + ); + } logger.error("Error in action", {apps/webapp/app/runEngine/services/triggerTask.server.ts (4)
160-161: Set an explicit HTTP status for invalid delay.Make the intent clear for downstream mappers returning 4xx.
Apply:
- throw new ServiceValidationError(`Invalid delay ${body.options?.delay}`); + throw new ServiceValidationError(`Invalid delay ${body.options?.delay}`, 422);
213-216: Provide status for queue limit validation.Consider 429 (Too Many Requests) to reflect capacity limits.
Apply:
- throw new ServiceValidationError( - `Cannot trigger ${taskId} as the queue size limit for this environment has been reached. The maximum size is ${queueSizeGuard.maximumSize}` - ); + throw new ServiceValidationError( + `Cannot trigger ${taskId} as the queue size limit for this environment has been reached. The maximum size is ${queueSizeGuard.maximumSize}`, + 429 + );
353-357: Status for task-run error propagation.Mark as 422 to signal validation-like failure up the stack.
Apply:
- throw new ServiceValidationError( - taskRunErrorToString(taskRunErrorEnhancer(result.error)) - ); + throw new ServiceValidationError( + taskRunErrorToString(taskRunErrorEnhancer(result.error)), + 422 + );
368-371: Status for one-time-use token violation.Return 422 (or 409) rather than defaulting.
Apply:
- throw new ServiceValidationError( - `Cannot trigger ${taskId} with a one-time use token as it has already been used.` - ); + throw new ServiceValidationError( + `Cannot trigger ${taskId} with a one-time use token as it has already been used.`, + 422 + );apps/webapp/app/runEngine/validators/triggerTaskValidator.ts (3)
32-35: Return an explicit 422 for tag count violations.Helps route mappers choose the right status without guessing.
Apply:
- error: new ServiceValidationError( - `Runs can only have ${MAX_TAGS_PER_RUN} tags, you're trying to set ${tags.length}.` - ), + error: new ServiceValidationError( + `Runs can only have ${MAX_TAGS_PER_RUN} tags, you're trying to set ${tags.length}.`, + 422 + ),
68-71: Use 429 for max-attempts exceeded.Communicates retry exhaustion/rate-like limit more precisely.
Apply:
- error: new ServiceValidationError( - `Failed to trigger ${taskId} after ${MAX_ATTEMPTS} attempts.` - ), + error: new ServiceValidationError( + `Failed to trigger ${taskId} after ${MAX_ATTEMPTS} attempts.`, + 429 + ),
98-101: Use 409 for parent-run terminal-state conflict.Represents a conflict with current resource state.
Apply:
- error: new ServiceValidationError( - `Cannot trigger ${taskId} as the parent run has a status of ${parentRun.status}` - ), + error: new ServiceValidationError( + `Cannot trigger ${taskId} as the parent run has a status of ${parentRun.status}`, + 409 + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts(1 hunks)apps/webapp/app/runEngine/concerns/errors.ts(0 hunks)apps/webapp/app/runEngine/concerns/payloads.server.ts(2 hunks)apps/webapp/app/runEngine/concerns/queues.server.ts(6 hunks)apps/webapp/app/runEngine/services/triggerTask.server.ts(5 hunks)apps/webapp/app/runEngine/validators/triggerTaskValidator.ts(4 hunks)apps/webapp/app/services/routeBuilders/apiBuilder.server.ts(2 hunks)internal-packages/run-engine/src/engine/index.ts(1 hunks)internal-packages/run-engine/src/engine/locking.ts(3 hunks)internal-packages/run-engine/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/runEngine/concerns/errors.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/locking.tsinternal-packages/run-engine/src/engine/index.tsinternal-packages/run-engine/src/index.tsapps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/concerns/payloads.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/queues.server.tsapps/webapp/app/runEngine/validators/triggerTaskValidator.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/concerns/payloads.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/queues.server.tsapps/webapp/app/runEngine/validators/triggerTaskValidator.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/concerns/payloads.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/queues.server.tsapps/webapp/app/runEngine/validators/triggerTaskValidator.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/concerns/payloads.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/queues.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/concerns/payloads.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/queues.server.tsapps/webapp/app/runEngine/validators/triggerTaskValidator.ts
🧠 Learnings (8)
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Prefer Run Engine 2.0 via internal/run-engine; avoid extending legacy run engine code
Applied to files:
internal-packages/run-engine/src/index.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Do not use client.defineJob or any deprecated v2 patterns (e.g., eventTrigger) when defining tasks
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Define tasks using task({ id, run, ... }) with a unique id per project
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use schedules.task(...) for scheduled (cron) tasks; do not implement schedules as plain task() with external cron logic
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export every task (including subtasks) defined with task(), schedules.task(), or schemaTask()
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts
📚 Learning: 2025-07-18T17:49:24.468Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T17:49:24.468Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
Applied to files:
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use schemaTask({ schema, run, ... }) to validate payloads when input validation is required
Applied to files:
apps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/runEngine/validators/triggerTaskValidator.ts
🧬 Code graph analysis (6)
internal-packages/run-engine/src/engine/locking.ts (1)
internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)
apps/webapp/app/runEngine/concerns/payloads.server.ts (2)
internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)
apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (2)
packages/core/src/v3/apps/http.ts (1)
json(65-75)internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)
apps/webapp/app/runEngine/services/triggerTask.server.ts (4)
internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)apps/webapp/app/v3/services/baseService.server.ts (1)
ServiceValidationError(8-8)apps/webapp/app/v3/services/common.server.ts (1)
ServiceValidationError(1-6)
apps/webapp/app/runEngine/concerns/queues.server.ts (2)
internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)
apps/webapp/app/runEngine/validators/triggerTaskValidator.ts (2)
internal-packages/run-engine/src/index.ts (1)
ServiceValidationError(5-5)internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)
⏰ 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 (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 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 (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
internal-packages/run-engine/src/engine/locking.ts (3)
178-181: Early rethrow correctly suppresses noisy lock logs/metricsShort-circuiting on ServiceValidationError here is the right call; it prevents misattributing routine failures to locking.
18-18: Import source looks correctImporting ServiceValidationError from local errors module aligns with the new handling.
178-196: Remove name‐based fallback;instanceofis sufficientAll
ServiceValidationErrorthrow sites inrun-engineimport the same class from its ownerrors.js(no mixed definitions), so theinstanceof ServiceValidationErrorcheck will always match.apps/webapp/app/runEngine/concerns/payloads.server.ts (1)
6-6: Centralized error importSwitch to the shared ServiceValidationError is consistent with the PR’s goal.
apps/webapp/app/runEngine/concerns/queues.server.ts (1)
17-17: Centralized error importUsing the shared ServiceValidationError here keeps behavior consistent across the app.
internal-packages/run-engine/src/engine/index.ts (1)
32-32: Import reordering only — OKNo behavioral impact.
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (1)
1-1: Updated import path for EngineServiceValidationError — OKImporting from @internal/run-engine (aliasing ServiceValidationError) keeps the route compatible with engine-thrown validation errors.
internal-packages/run-engine/src/index.ts (1)
2-6: Alias export looks good for compat.Re-exporting
ServiceValidationErrorasEngineServiceValidationErrorpreserves existing imports cleanly.apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (1)
26-28: Correct imports for both validation error sources.Importing
ServiceValidationError(webapp) andEngineServiceValidationError(run-engine alias) covers cross-package instanceof issues.
This also stops all the unnecessary error logging when throwing
ServiceValidationErrors