fix(batch): clear idempotency keys pointing at dead runs in batchTrigger - #4913
fix(batch): clear idempotency keys pointing at dead runs in batchTrigger#4913itzzdev09 wants to merge 1 commit into
Conversation
The single-trigger path clears an idempotency key when the run it points at reached a clearable terminal state — `IdempotencyKeyConcern.handleExistingRun` guards on `shouldIdempotencyKeyBeCleared(existingRun.status)`. The batch path only tested time expiry, so `batchTrigger` with a key pointing at a dead run returned that FAILED run as `isCached: true`, and kept returning it on every retry. The batch path could not make that check: `findRunsByIdempotencyKeys` selected five columns and `status` was not one of them, so `cachedRun` had no status to test. The fix spans three files: - `run-store/types.ts`: `IdempotencyKeyRunMatch` gains `status`. - `run-store/PostgresRunStore.ts`: the lookup selects `"status"`. `delegatingRunStore` and `runOpsStore` forward unchanged. - `batchTriggerV3.server.ts`: the cached-run guard becomes `keyTimeExpired || shouldIdempotencyKeyBeCleared(cachedRun.status)`, in the same branch as time expiry so the run lands in `expiredRunIds` and the stale key is cleared — otherwise it would survive to the next batch. Policy stays owned by the webapp (`shouldIdempotencyKeyBeCleared` lives in `v3/taskStatus.ts`); the store just returns one more column. Fixes triggerdotdev#4819. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
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 (6)
WalkthroughThe run store now returns 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 @itzzdev09, 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}
| const keyTimeExpired = | ||
| !!cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date(); | ||
|
|
||
| if (keyTimeExpired || shouldIdempotencyKeyBeCleared(cachedRun.status)) { |
There was a problem hiding this comment.
🔴 Large mixed batches omit fresh runs
When live cached items precede dead ones, shouldIdempotencyKeyBeCleared marks only the latter for creation. Job ranges cover newRunCount slots from index zero, not the new runs' positions. New tasks beyond those ranges never run, while the API returns nonexistent run IDs.
Prompt for agents
The new dead-run classification increases newRunCount for selected positions, but the default parallel scheduler in apps/webapp/app/v3/services/batchTriggerV3.server.ts builds contiguous ranges from zero using only newRunCount. Batch runIds still contains every item, including live cached entries. If cached entries occupy early positions, scheduled ranges can end before later fresh entries, so those entries are never processed. Update parallel range construction or item processing so every position containing a non-cached run is covered, while preserving batch item indexes and completion accounting. Add a regression test with more than the async threshold, live cached entries first, and dead idempotent entries later.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #4819. Reported by @Jaimin2687.
The bug
The single-trigger path clears an idempotency key when the run it points at reached a clearable terminal state —
IdempotencyKeyConcern.handleExistingRunguards onshouldIdempotencyKeyBeCleared(existingRun.status). The batch path (batchTriggerV3.server.ts:453) only tested time expiry:So
batchTriggerwith a key pointing at a CRASHED / TIMED_OUT / SYSTEM_FAILURE run returned that run asisCached: true, and kept returning it on every retry.Why it isn't a one-line check
The batch path had no status to test.
findRunsByIdempotencyKeysselected five columns andstatuswas not one of them (PostgresRunStore.ts:1864), andIdempotencyKeyRunMatchagreed. DroppingshouldIdempotencyKeyBeCleared(cachedRun.status)in would not have compiled. Hence three files:internal-packages/run-store/src/types.ts—IdempotencyKeyRunMatchgainsstatus: TaskRunStatus.internal-packages/run-store/src/PostgresRunStore.ts— the lookup selects"status".delegatingRunStore.ts:439andrunOpsStore.ts:781both just forward, so no change there.apps/webapp/app/v3/services/batchTriggerV3.server.ts— the guard becomeskeyTimeExpired || shouldIdempotencyKeyBeCleared(cachedRun.status).Two deliberate choices:
expiredRunIds, which drives theclearIdempotencyKeycall below. A separate branch would mint a fresh run but leave the stale key in place, and the next batch would hit it again.shouldIdempotencyKeyBeClearedlives inv3/taskStatus.tsand stays there; the run store just returns one more column. The tradeoff is that the store now carries a field only one caller uses, which seemed better than pushing status policy down into the store.Notes
.server-changes/entry rather than a changeset: the webapp change is user-facing, and@internal/run-storeisprivate: true, so perCHANGESETS.mdno changeset applies.statusto theIdempotencyKeyRunMatchfixture inrunOpsStore.shardMap.test.tsso it stays faithful to the type (those rows are cast, so it was not a compile break).Verification — honest status
TaskRun.statusis a real non-null column (TaskRunStatus @default(PENDING)), so the added SELECT is valid;shouldIdempotencyKeyBeClearedisisFailedRunStatus(status) || status === "EXPIRED", i.e. exactly the six clearable statuses, matching single-trigger;shouldIdempotencyKeyBeClearedwas already reachable from../taskStatus, whichbatchTriggerV3.server.tsalready imports from.PostgresRunStore.findRunsByIdempotencyKeys.test.ts— asserts a CRASHED and an EXECUTING row come back with their statuses) needs the repo's testcontainers/Docker harness, and I could not install the workspace in this environment. It should run in CI, and I'll fix anything that falls out.Context: a bot opened #4912 for this issue off my analysis in #4819; it was auto-closed by CI within seconds. This is the same fix reworked and submitted properly, with the changeset/server-changes distinction handled and the fixture updated.
🤖 Generated with Claude Code