Idempotency ignores failed runs in v4 #2376
Conversation
|
WalkthroughThe changes introduce new logic for handling idempotency keys and error event tracking in a web application. In the idempotency key handling, the system now checks if an idempotency key should be cleared based on the run status, using a new function, and clears it when appropriate, altering the control flow for cached runs. The event repository logic for determining error states was updated to allow explicit error marking in addition to error detection from failures. A new exported function was added to encapsulate the logic for when idempotency keys should be cleared, based on run status. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 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
🔭 Outside diff range comments (1)
apps/webapp/app/v3/taskStatus.ts (1)
63-66: Bug: Incorrect FATAL_RUN_STATUSES type aliasThe alias references FAILED_RUN_STATUSES instead of FATAL_RUN_STATUSES, which is a type correctness issue.
Apply this fix:
export const FATAL_RUN_STATUSES = ["SYSTEM_FAILURE", "CRASHED"] satisfies TaskRunStatus[]; -export type FATAL_RUN_STATUSES = (typeof FAILED_RUN_STATUSES)[number]; +export type FATAL_RUN_STATUSES = (typeof FATAL_RUN_STATUSES)[number];
🧹 Nitpick comments (3)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts (2)
63-67: Nit: Log the computed idempotencyKey variableThe log currently uses request.options?.idempotencyKey, which misses keys coming from the body. Use the computed idempotencyKey variable for accurate logging.
- logger.debug("[TriggerTaskService][call] Idempotency key should be cleared", { - idempotencyKey: request.options?.idempotencyKey, + logger.debug("[TriggerTaskService][call] Idempotency key should be cleared", { + idempotencyKey, runStatus: existingRun.status, runId: existingRun.id, });
1-18: Confirm target engine (legacy vs RE 2.0) and parityOrg guidance is to avoid adding code to the legacy run engine. If this file is part of the legacy engine, ensure equivalent logic exists in Run Engine 2.0 (internal/run-engine), or track a follow-up to port this behavior.
Want me to open an issue to track parity in RE 2.0 and propose a minimal patch?
apps/webapp/app/v3/eventRepository.server.ts (1)
1100-1107: Check consistency: isError may be true while status stays "OK"You now allow explicit isError regardless of failedWithError, but status is still derived only from failedWithError. That can yield isError: true with status: "OK". If intentional (styling/logging only), fine. If not, derive status from isError too.
- status: failedWithError ? "ERROR" : "OK", + status: (options.isError === true || !!failedWithError) ? "ERROR" : "OK",If you keep the current behavior, consider a brief comment explaining why status can be "OK" when isError is true to avoid confusion later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts(3 hunks)apps/webapp/app/v3/eventRepository.server.ts(1 hunks)apps/webapp/app/v3/taskStatus.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/eventRepository.server.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/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/eventRepository.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/eventRepository.server.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using idempotency, use the `idempotencyKeys` API and `idempotencyKey`/`idempotencyKeyTTL` options as shown.
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using idempotency, use the `idempotencyKeys` API and `idempotencyKey`/`idempotencyKeyTTL` options as shown.
Applied to files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Tasks must be exported, even subtasks in the same file.
Applied to files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2024-10-18T15:41:52.352Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1418
File: packages/core/src/v3/errors.ts:364-371
Timestamp: 2024-10-18T15:41:52.352Z
Learning: In `packages/core/src/v3/errors.ts`, within the `taskRunErrorEnhancer` function, `error.message` is always defined, so it's safe to directly call `error.message.includes("SIGTERM")` without additional checks.
Applied to files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/v3/eventRepository.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `trigger.dev/sdk/v3` and export them as shown in the correct pattern.
Applied to files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST `export` every task, including subtasks, in Trigger.dev task files.
Applied to files:
apps/webapp/app/v3/taskStatus.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Before generating any code for Trigger.dev tasks, verify: (1) Are you importing from `trigger.dev/sdk/v3`? (2) Have you exported every task? (3) Have you generated any deprecated code patterns?
Applied to files:
apps/webapp/app/v3/taskStatus.ts
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
Applied to files:
apps/webapp/app/v3/taskStatus.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : The `run` function contains your task logic in Trigger.dev tasks.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:49:47.180Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Do not use or add new code to the legacy run engine; focus on using and migrating to Run Engine 2.0 in `internal/run-engine`.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using Realtime features, use the `runs.subscribeToRun`, `runs.subscribeToRunsWithTag`, and `runs.subscribeToBatch` APIs as shown.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : NEVER generate deprecated code patterns using `client.defineJob` and related deprecated APIs, as shown in the prohibited code block.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing scheduled (cron) tasks, use `schedules.task` from `trigger.dev/sdk/v3` and follow the shown patterns.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using metadata in tasks, use the `metadata` API as shown, and only inside run functions or task lifecycle hooks.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Each task needs a unique ID within your project.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` as shown in the examples.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When logging in tasks, use the `logger` API (`logger.debug`, `logger.log`, `logger.info`, `logger.warn`, `logger.error`) as shown.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing schema tasks, use `schemaTask` from `trigger.dev/sdk/v3` and validate payloads as shown.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them as shown in the examples for Trigger.dev tasks.
Applied to files:
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/eventRepository.server.ts
📚 Learning: 2024-10-12T01:08:24.066Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1353
File: apps/webapp/app/v3/services/alerts/deliverAlert.server.ts:76-76
Timestamp: 2024-10-12T01:08:24.066Z
Learning: When extending the `Error` class in JavaScript/TypeScript, it's not necessary to manually set the `name` property because it defaults to the class name.
Applied to files:
apps/webapp/app/v3/eventRepository.server.ts
⏰ 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 / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/v3/taskStatus.ts (1)
132-134: Idempotency: clear keys on failed and expired runs — aligns with PR objectiveLogic matches the desired behavior (don’t cache failures, also clear on EXPIRED).
Do we also want to clear idempotency for CANCELED runs? If you intend reruns for cancellations, consider including "CANCELED" here. I can send a tiny patch if confirmed.
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts (5)
6-6: Import of status helper is correctImport path and usage are appropriate for centralizing status logic.
45-59: Expiration handling remains soundClearing expired idempotency keys and returning non-cached is correct and preserves prior behavior.
61-76: Clear key on failed/expired statuses — correct behavior for idempotency in v4This ensures failed/expired runs aren’t returned from cache and a new run can trigger.
70-76: Concurrency note: two concurrent clears may both proceed to create a new runUsing updateMany avoids throwing when another request clears first, which is good. However, two concurrent requests could both attempt to create a new run with the same idempotency key. Ensure the downstream creation path handles a potential unique constraint race (e.g., retry-on-unique-violation then fetch existing).
Would you like a patch to add a small “create-or-fetch on unique violation” guard where runs are created?
78-112: Cached-run handling with waitpoint is preservedReturning the existing run and blocking the parent when appropriate remains intact and correct after the new clearing branch.
If you trigger a run with an idempotency key but that run fails it won’t be returned as a cached run in v4.
It’s hard to come up with a situation where you would want the old behaviour. If you did want to cache a failure, you should return a
Resulttype instead of throwing an error. Failures should only be used when something has gone wrong, not to indicate a known false case.