Repository navigation
Conversation
…oses triggerdotdev#4819) batchTrigger silently returned stale failed runs as isCached: true, so a caller re-batching with the same idempotency key never got a retry. The single-trigger path has done the right thing since day one via `IdempotencyKeyConcern.handleExistingRun` -> `shouldIdempotencyKeyBeCleared` (apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:463) — the batch path at batchTriggerV3.server.ts:450 just ignored the status. Root cause (as @Jaimin2687 identified on the issue): * `PostgresRunStore.findRunsByIdempotencyKeys` SELECTed 5 columns — id, createdAt, friendlyId, idempotencyKey, idempotencyKeyExpiresAt. * `status` was missing, and `IdempotencyKeyRunMatch` did not type it. * `#prepareRunData` therefore had only time-based expiry to work with and no way to see that the cached run was CRASHED / SYSTEM_FAILURE / COMPLETED_WITH_ERRORS / INTERRUPTED / TIMED_OUT / EXPIRED. Fix (three files, additive): - internal-packages/run-store/src/types.ts * `IdempotencyKeyRunMatch` gains a required `status: TaskRunStatus`. * Comment explains the invariant for the next reader. - internal-packages/run-store/src/PostgresRunStore.ts * The hot-path UNION ALL SQL now also selects `"status"`. One token added per branch, no new rows read, no index change needed — (runtimeEnvironmentId, taskIdentifier, idempotencyKey) already uniquely identifies the row. - apps/webapp/app/v3/services/batchTriggerV3.server.ts * `#prepareRunData` imports `shouldIdempotencyKeyBeCleared` and ORs it with the existing time-based expiry check. A cached run whose status is in the "clear me" set is now treated exactly like an expired cache entry: the friendlyId is added to `expiredRunIds` for cleanup, a fresh child id is minted, and the result is returned with `isCached: false`. * Behaviour on cached-but-still-good runs is unchanged. * Behaviour on cached-and-time-expired runs is unchanged. - internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts * `createRun` helper gains an optional `status` param. * New postgresTest "returns run status so callers can honour shouldIdempotencyKeyBeCleared" creates four seed rows (PENDING, CRASHED, COMPLETED_WITH_ERRORS, COMPLETED_SUCCESSFULLY) and asserts the store surfaces each status in the result. This locks the contract for batchTrigger to depend on. Reporter: @Jaimin2687 (full RCA in the issue). No in-flight PR for triggerdotdev#4819 at time of writing.
|
|
Hi @61465, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (4)
✨ Finishing Touches
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const failed = shouldIdempotencyKeyBeCleared(cachedRun.status); | ||
|
|
||
| if (expired || failed) { | ||
| expiredRunIds.add(cachedRun.friendlyId); | ||
|
|
||
| return { |
There was a problem hiding this comment.
🔴 Duplicate failed keys yield missing runs
When batch items share a failed run's key, shouldIdempotencyKeyBeCleared makes each item mint a different ID. Processing deduplicates later items against the first new run, but the response retains their nonexistent IDs.
(Refers to this code)
Learn more
The batch lookup returns one cached run per key, but #prepareRunData maps each item independently. When the cached run has failed, each matching item receives a fresh ID and the old key is cleared once. During #processBatchTaskRunItem, the first item creates a run with that key. handleTriggerRequest then caches that run for later items, leaving their preallocated IDs unused. This also affects the older expiration branch, but failed runs now activate it too.
Example: A batch contains two email items with key retry-1, whose previous run is CRASHED. Preparation returns run_new_a and run_new_b; processing creates only run_new_a, then reuses it for the second item. The response lists run_new_b, which cannot be retrieved.
Recommended fix: Deduplicate entries by (taskIdentifier, idempotencyKey) when allocating replacement IDs and return the same ID for all items sharing a key. Confirm batch item accounting and async processing still reflect the reused run.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const failed = shouldIdempotencyKeyBeCleared(cachedRun.status); | ||
|
|
||
| if (expired || failed) { |
There was a problem hiding this comment.
🔍 Missing release note for batch retries
This user-facing server fix has no .server-changes/ entry. The internal run-store changes do not replace the required server release note.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const rows = await store.findRunsByIdempotencyKeys({ | ||
| runtimeEnvironmentId: environment.id, | ||
| taskIdentifier: "task-x", | ||
| idempotencyKeys: ["key-pending", "key-crashed", "key-cwe", "key-ok"], | ||
| }); | ||
|
|
||
| const byKey = new Map(rows.map((r) => [r.idempotencyKey, r])); | ||
| expect(byKey.get("key-pending")?.status).toBe("PENDING"); | ||
| expect(byKey.get("key-crashed")?.status).toBe("CRASHED"); | ||
| expect(byKey.get("key-cwe")?.status).toBe("COMPLETED_WITH_ERRORS"); | ||
| expect(byKey.get("key-ok")?.status).toBe("COMPLETED_SUCCESSFULLY"); |
There was a problem hiding this comment.
🔍 Retry behavior lacks an integration test
The new test verifies only the status returned by the store. No test exercises a failed-key batch through BatchTriggerV3Service.call.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // (CRASHED, SYSTEM_FAILURE, INTERRUPTED, COMPLETED_WITH_ERRORS, | ||
| // EXPIRED, TIMED_OUT_WITH_ERRORS — see shouldIdempotencyKeyBeCleared). |
There was a problem hiding this comment.
🔍 Failure-status comment names the wrong status
The comment lists TIMED_OUT_WITH_ERRORS, but shouldIdempotencyKeyBeCleared clears TIMED_OUT. The mismatch obscures which timed-out runs can be retried.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ey) + release note (addresses Devin review on triggerdotdev#5014) Three fixes from the Devin review on triggerdotdev#5014: 1. **Dedupe replacement ids when multiple batch items share a cleared key.** When >=2 batch items share `(taskIdentifier, idempotencyKey)` AND the cached run should be cleared (time-expired OR, after this PR, terminally failed), downstream `#processBatchTaskRunItem` only creates ONE run per key: `TriggerTaskService.call` honours the key and reuses the first fresh run for every subsequent item. Allocating a distinct minted friendlyId per item left the response referencing ghost ids the caller could never retrieve. We now pre-allocate one replacement id per `(task, key)` combo and reuse it for every matching item. This also fixes the pre-existing time-expiration branch; the new status-based branch exposes the same bug on more inputs. 2. **Correct the failure-status list in the inline comment.** `isFailedRunStatus` is INTERRUPTED / COMPLETED_WITH_ERRORS / SYSTEM_FAILURE / CRASHED / TIMED_OUT (not TIMED_OUT_WITH_ERRORS); `shouldIdempotencyKeyBeCleared` adds EXPIRED. The old comment said `TIMED_OUT_WITH_ERRORS` which is not a status. 3. **Add .server-changes/ release note.** Per the repo's `.server-changes/README.md`, user-facing webapp fixes need a release note (the changeset only covers package publishes, and this touches `apps/webapp/`). Not addressed in this commit (deferred, happy to split if the maintainer prefers): end-to-end integration test calling `BatchTriggerV3Service.call` with a failed-key batch. The SQL-layer regression test added in the previous commit locks the data contract this fix depends on; adding a service-level test needs test fixtures for the trigger pipeline that would ~double the diff. If you'd like it in this PR, I can wire it up against `batchTriggerV3StoreRouting.test.ts`.
…edupe replacement ids (closes triggerdotdev#4819) batchTrigger silently returned stale failed runs as isCached: true, so a caller re-batching with the same idempotency key never got a retry. The single-trigger path has done the right thing since day one via `IdempotencyKeyConcern.handleExistingRun` -> `shouldIdempotencyKeyBeCleared` (apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:463). The batch path at batchTriggerV3.server.ts:450 just ignored the status. Root cause (as @Jaimin2687 identified on the issue): * `PostgresRunStore.findRunsByIdempotencyKeys` SELECTed 5 columns — id, createdAt, friendlyId, idempotencyKey, idempotencyKeyExpiresAt. * `status` was missing, and `IdempotencyKeyRunMatch` did not type it. * `#prepareRunData` therefore had only time-based expiry to work with and no way to see that the cached run was CRASHED / SYSTEM_FAILURE / COMPLETED_WITH_ERRORS / INTERRUPTED / TIMED_OUT / EXPIRED. Fix (additive, five files): - internal-packages/run-store/src/types.ts * `IdempotencyKeyRunMatch` gains a required `status: TaskRunStatus`. * Comment explains the invariant for the next reader. - internal-packages/run-store/src/PostgresRunStore.ts * The hot-path UNION ALL SQL now also selects `"status"`. One token added per branch, no new rows read, no index change needed — (runtimeEnvironmentId, taskIdentifier, idempotencyKey) already uniquely identifies the row. - apps/webapp/app/v3/services/batchTriggerV3.server.ts * `#prepareRunData` imports `shouldIdempotencyKeyBeCleared` and ORs it with the existing time-based expiry check. A cached run whose status is in the "clear me" set is treated exactly like an expired cache entry. * **Dedupe replacement ids per (task, idempotencyKey)**: when two items share a key AND the cached run is cleared, downstream `#processBatchTaskRunItem` only creates ONE run per key (TriggerTaskService.call honours the key and reuses the first fresh run for subsequent items). Allocating distinct minted ids per item left the response referencing ghost ids. We now pre- allocate one replacement id per (task, key) combo and reuse it for every matching item. This also fixes the pre-existing time- expiration branch — the status-based branch exposed the same bug on more inputs. (Devin review on triggerdotdev#5014.) - internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts * `createRun` helper gains an optional `status` param. * New postgresTest "returns run status so callers can honour shouldIdempotencyKeyBeCleared" creates four seed rows (PENDING, CRASHED, COMPLETED_WITH_ERRORS, COMPLETED_SUCCESSFULLY) and asserts the store surfaces each status in the result. Locks the contract batchTrigger depends on. - .server-changes/batchtrigger-clears-failed-idempotency-key.md * User-facing release note per `.server-changes/README.md`. Reporter: @Jaimin2687 (full RCA in the issue). Devin reviewed the first revision and surfaced the dedupe gap + the TIMED_OUT comment typo + the missing release note — all addressed here. The one remaining Devin suggestion (full integration test calling `BatchTriggerV3Service.call` with a failed-key batch) is deferred; happy to split it out if a maintainer prefers it in this PR.
|
Thanks @devin-ai-integration — I addressed three of the four points and left a note on the fourth. Branch is force-pushed to 1. 🔴 Duplicate failed keys yield missing runs — FIXED 2. 🔍 Missing release note — ADDED 3. 🔍 Failure-status comment names the wrong status — FIXED 4. 🔍 Retry behavior lacks an integration test — DEFERRED The PR is currently closed by |
1 participant
What
Fixes #4819 —
batchTriggersilently returned stale failed runs asisCached: true, so acaller re-batching with the same idempotency key never got a retry.
The single-trigger path has done the right thing since day one via
IdempotencyKeyConcern.handleExistingRun→shouldIdempotencyKeyBeCleared(
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:463). Thebatch path at
batchTriggerV3.server.ts:450just ignored the status.Repro from the issue (confirmed):
Root cause
Credit to @Jaimin2687 for the full RCA on the issue. Confirmed line-by-line:
PostgresRunStore.findRunsByIdempotencyKeysSELECTs 5 columns —id, createdAt, friendlyId, idempotencyKey, idempotencyKeyExpiresAt.statuswas missing from both the SQL and theIdempotencyKeyRunMatchtype.
#prepareRunDatatherefore had only time-based expiry to work with andno way to see that the cached run was
CRASHED/COMPLETED_WITH_ERRORS/SYSTEM_FAILURE/INTERRUPTED/TIMED_OUT_WITH_ERRORS/EXPIRED.Fix (additive, 4 files)
internal-packages/run-store/src/types.tsAdds
status: TaskRunStatustoIdempotencyKeyRunMatch, with a commentexplaining why the field is now required.
internal-packages/run-store/src/PostgresRunStore.tsHot-path UNION ALL SQL now also selects
\"status\". One token added perbranch, no new rows read, no index change — the existing
(runtimeEnvironmentId, taskIdentifier, idempotencyKey)filter alreadypinpoints each row.
apps/webapp/app/v3/services/batchTriggerV3.server.ts#prepareRunDataimportsshouldIdempotencyKeyBeClearedand ORs it withthe existing time-expiry check. A cached run whose status is in the
'clear me' set is now treated exactly like an expired cache entry: the
friendlyId is added to
expiredRunIdsfor cleanup, a fresh child id isminted, and the result returns with
isCached: false.Behaviour on cached-but-still-good runs is unchanged. Behaviour on
cached-and-time-expired runs is unchanged. Only the previously-silent
"cached but dead" case flips.
internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.tscreateRunhelper gains an optionalstatusparam.postgresTest(\"returns run status so callers can honour shouldIdempotencyKeyBeCleared\")creates four seed rows (PENDING,CRASHED,COMPLETED_WITH_ERRORS,COMPLETED_SUCCESSFULLY) andasserts the store surfaces each status in the result. This locks the
contract for batchTrigger to depend on.
Why this design
path, one type field, one test. No refactor of the batch flow, no new
code path to maintain.
shouldIdempotencyKeyBeClearedas the source of truth already expectsboth call sites to behave the same way. This PR makes them so.
statusvalue the OR evaluates tofalseand we fall through to the existing "return cached" path —identical behaviour to today.
BatchTriggerTaskV2Responsealready carriesisCachedper item. Callers that previously gotisCached: truefor adead run will now get
isCached: falseand a fresh friendlyId, whichis what they already handle for the time-expired case.
Testing
status.Impact
batchTrigger+ idempotency keys on failing tasks: silentlybroken today, silently fixed by this PR. The failure mode is nasty
because the caller sees
isCached: trueand reasonably assumes the runwill produce output — then waits forever on a dead run.
Related
Reporter: @Jaimin2687 — full RCA + line references on the issue.
Prior work this builds on:
shouldIdempotencyKeyBeClearedwas introduced in the single-triggerpath to codify the retry-on-failure contract. This PR extends the same
contract to the batch path.