fix(batch): clear idempotency keys pointing at dead runs in batchTrigger - #4912
fix(batch): clear idempotency keys pointing at dead runs in batchTrigger#4912breken-ai wants to merge 1 commit into
Conversation
The single-trigger path already clears an idempotency key when the cached run reached a clearable terminal state (shouldIdempotencyKeyBeCleared: failed statuses + EXPIRED). The batch path only tested time expiry, so batchTrigger with an idempotency key pointing at a FAILED run returned the dead run as isCached forever. - PostgresRunStore.findRunsByIdempotencyKeys now selects the status column - IdempotencyKeyRunMatch gains status - batchTriggerV3 clears the key and mints a fresh run when shouldIdempotencyKeyBeCleared(status) holds, same branch as time expiry Credit: reported by @Jaimin2687 in triggerdotdev#4819; fix shape follows @itzzdev09's analysis in the issue thread.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
WalkthroughThe run-store idempotency lookup now returns each matched run’s Severity of issue fixed: Medium ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hi @breken-ai, 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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // isCached - clear the key and mint a fresh run, same as a time-expired one. | ||
| if ( | ||
| (cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date()) || | ||
| shouldIdempotencyKeyBeCleared(cachedRun.status) |
There was a problem hiding this comment.
🔴 Late revived runs are skipped
When a failed cached run appears late in a parallel batch, shouldIdempotencyKeyBeCleared marks it new. Parallel ranges cover only newRunCount positions in the full item list. The batch returns a minted run ID that never executes.
Prompt for agents
The new failed-status branch in apps/webapp/app/v3/services/batchTriggerV3.server.ts can turn an item anywhere in runs into a fresh run. In #createAndProcessBatchTaskRun, parallel ranges are sized from newRunCount, but #processBatchTaskRunItems applies those ranges to the complete batch.runIds array, where cached and fresh entries remain interleaved. A fresh item after the covered prefix is never processed. Make parallel processing cover every position that can contain a fresh run, or preserve enough metadata to schedule exactly the fresh positions. Add a regression test with more than the async threshold, mostly cached items, and a failed cached item after the first computed range.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Parity with the single-trigger path (IdempotencyKeyConcern.handleExistingRun): a cached | ||
| // run that reached a clearable terminal state (failed/expired) must not be returned as | ||
| // isCached - clear the key and mint a fresh run, same as a time-expired one. | ||
| if ( | ||
| (cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date()) || | ||
| shouldIdempotencyKeyBeCleared(cachedRun.status) |
There was a problem hiding this comment.
🔍 User-facing fix lacks release note
This server fix changes visible batch retry behavior. Repository rules require a .server-changes/ entry for user-facing server fixes.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #4819.
The single-trigger path already guards this:
IdempotencyKeyConcern.handleExistingRunclears the key and mints a fresh run whenshouldIdempotencyKeyBeCleared(existingRun.status)holds (failed statuses + EXPIRED). The batch path (batchTriggerV3.server.ts:453) only tested time expiry, sobatchTriggerwith an idempotency key pointing at a dead run returned that FAILED run asisCached: true- and kept returning it on every retry.Three changes, following the shape @itzzdev09 sketched in the issue thread:
internal-packages/run-store/src/PostgresRunStore.ts-findRunsByIdempotencyKeysnow selects thestatuscolumn (the wrinkle he flagged:cachedRunpreviously had nostatusto check).delegatingRunStore/runOpsStoreforward unchanged.internal-packages/run-store/src/types.ts-IdempotencyKeyRunMatchgainsstatus: TaskRunStatus.apps/webapp/app/v3/services/batchTriggerV3.server.ts- the cached-run check becomestimeExpired || shouldIdempotencyKeyBeCleared(cachedRun.status)in the same branch, so the key is cleared and a fresh run is minted, matching single-trigger behavior. Policy stays owned by the webapp; the store just returns one more column.Credit: @Jaimin2687 for the report, @itzzdev09 for the analysis and fix sketch in #4819.
Verification (honest status):
statuscorrectly for CRASHED/PENDING rows.taskStatus.ts): all 6 clearable statuses (INTERRUPTED, COMPLETED_WITH_ERRORS, SYSTEM_FAILURE, CRASHED, TIMED_OUT, EXPIRED) flip from stale-cache to clear-and-fresh-run; COMPLETED_SUCCESSFULLY / EXECUTING unchanged; time-expiry path unchanged.PostgresRunStore.findRunsByIdempotencyKeys.test.ts). The repo's test harness requires Docker/testcontainers, which my environment does not have, so it was not run locally - it should run in CI.