fix(webapp): clear idempotency key and re-trigger dead runs in batchT… - #4969
kaiizer777 wants to merge 1 commit into
Conversation
|
|
Hi @kaiizer777, 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const shouldClear = | ||
| isExpired || shouldIdempotencyKeyBeCleared(cachedRun.status as TaskRunStatus); |
There was a problem hiding this comment.
🟡 Late dead runs remain untriggered
When shouldClear marks a late run in a mostly cached batch, parallel processing can skip it. The parallel scheduler covers a prefix sized from newRunCount, then seals the batch.
Learn more
Parallel batch jobs process positional slices of batch.runIds. Cached entries remain in that array, so the number of uncached runs does not determine the last position requiring processing. Marking a dead run uncached increases newRunCount, but the parallel scheduler still starts every range within the first ceil(newRunCount / 50) * 50 positions. A dead run after that prefix is never passed to TriggerTaskService, although the final processing job seals the batch.
Example: A 100-item batch has 99 live cached runs and one CRASHED run at index 99. newRunCount is 1, so only range 0–49 is enqueued. The batch seals after that job, while the fresh ID at index 99 never becomes a run.
Recommended fix: Build parallel ranges from runs.length, not newRunCount, because processing is indexed over the complete ordered arrays. Keep newRunCount only for queue-limit accounting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (shouldClear) { | ||
| expiredRunIds.add(cachedRun.friendlyId); | ||
|
|
||
| return { |
There was a problem hiding this comment.
🟡 Repeated keys expose nonexistent runs
When repeated items share a failed key, shouldClear mints a different ID for each item. Later triggers deduplicate to the first run, but the batch retains every unused ID.
(Refers to this code)
Learn more
Every repeated item finds the same failed row in the preloaded cachedRuns snapshot. Each item therefore enters this branch and receives a separate minted ID before the old key is cleared. During processing, the first item creates a run with that key; TriggerTaskService returns that run as cached for later items. The batch's stored runIds and returned runs are never reconciled with the actual cached run ID.
Example: Two items use key invoice-42, which belongs to a CRASHED run. Preparation returns fresh IDs run_A and run_B. Processing creates run_A, then deduplicates the second item to run_A, while the response still exposes nonexistent run_B.
Recommended fix: Resolve repeated (taskIdentifier, idempotencyKey) entries as one preparation unit and assign the same fresh ID to all matching items. Also cover concurrent batches, where a competing trigger can win after preparation; reconcile the actual TriggerTaskService result with the batch's stored run ID or serialize key replacement and creation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Mock DB layer singletons | ||
| vi.mock("~/db.server", () => ({ | ||
| prisma: {}, | ||
| $replica: {}, | ||
| runOpsNewPrisma: {}, | ||
| runOpsLegacyPrisma: {}, | ||
| runOpsNewReplica: {}, | ||
| runOpsLegacyReplica: {}, | ||
| })); | ||
|
|
||
| import type { TaskRunStatus } from "@trigger.dev/database"; | ||
| import type { AuthenticatedEnvironment } from "~/services/apiAuth.server"; | ||
| import { BatchTriggerV3Service } from "~/v3/services/batchTriggerV3.server"; | ||
| import { shouldIdempotencyKeyBeCleared } from "~/v3/taskStatus"; |
There was a problem hiding this comment.
🔍 Tests bypass the integration policy
The new suite mocks database singletons and the run store, contrary to the mandatory testcontainers policy. It also tests through an added private wrapper instead of the public service contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
…rigger
Closes #
✅ Checklist
Testing
[Describe the steps you took to test this change]
Changelog
[Short description of what has changed]
Screenshots
[Screenshots]
💯