Skip to content

Commit da93e87

Browse files
committed
fix(batchTrigger): clear idempotency key when previous run failed + dedupe 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.
1 parent 21f1dcc commit da93e87

5 files changed

Lines changed: 140 additions & 6 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
`batchTrigger` now re-triggers when the idempotency key points to a run
7+
that already ended in a terminal failure state
8+
(`CRASHED` / `COMPLETED_WITH_ERRORS` / `SYSTEM_FAILURE` / `INTERRUPTED` /
9+
`TIMED_OUT` / `EXPIRED`), matching the single-trigger behaviour. Previously
10+
a batch call with the same key as a dead run silently returned the dead
11+
run as `isCached: true`, leaving the caller waiting on a run that would
12+
never produce output. Multiple batch items that share the same key now
13+
also share the same replacement run id, so the response no longer
14+
contains ghost ids pointing at runs that were never created. Closes #4819.

‎apps/webapp/app/v3/services/batchTriggerV3.server.ts‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ import { mintBatchFriendlyId } from "~/v3/runOpsMigration/mintBatchFriendlyId.se
2727
import { batchTriggerWorker } from "../batchTriggerWorker.server";
2828
import { guardQueueSizeLimitsForEnv } from "../queueSizeLimits.server";
2929
import { downloadPacketFromObjectStore, uploadPacketToObjectStore } from "../objectStore.server";
30-
import { isFinalAttemptStatus, isFinalRunStatus } from "../taskStatus";
30+
import {
31+
isFinalAttemptStatus,
32+
isFinalRunStatus,
33+
shouldIdempotencyKeyBeCleared,
34+
} from "../taskStatus";
3135
import { startActiveSpan } from "../tracer.server";
3236
import { BaseService, ServiceValidationError } from "./baseService.server";
3337
import { OutOfEntitlementError, TriggerTaskService } from "./triggerTask.server";
@@ -442,6 +446,10 @@ export class BatchTriggerV3Service extends BaseService {
442446
// Build the run IDs in order: reuse an unexpired cached id, else mint a new id (and record any
443447
// expired cached id so its idempotency key can be cleared below).
444448
const expiredRunIds = new Set<string>();
449+
// Dedupes replacement ids when multiple batch items share
450+
// (taskIdentifier, idempotencyKey) and the cached run must be cleared.
451+
// See the fuller explanation inside the loop below.
452+
const replacementIdsByKey = new Map<string, string>();
445453

446454
const runs = await Promise.all(
447455
body.items.map(async (item) => {
@@ -450,13 +458,55 @@ export class BatchTriggerV3Service extends BaseService {
450458
);
451459

452460
if (cachedRun) {
453-
if (cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date()) {
461+
// Clear the idempotency key and mint a fresh run when either:
462+
// (a) the key's time-based expiry has passed, OR
463+
// (b) the previous run ended in a terminal failure state the
464+
// single-trigger path treats as "retry allowed"
465+
// (INTERRUPTED, COMPLETED_WITH_ERRORS, SYSTEM_FAILURE,
466+
// CRASHED, TIMED_OUT, plus EXPIRED — see
467+
// `shouldIdempotencyKeyBeCleared` in `taskStatus.ts`).
468+
//
469+
// Before this fix, batchTrigger only checked (a). A failed run
470+
// with the same idempotency key was returned as `isCached: true`,
471+
// silently handing the caller back a dead run that would never
472+
// produce output (issue #4819). This mirrors the single-trigger
473+
// behaviour at IdempotencyKeyConcern.handleExistingRun
474+
// (idempotencyKeys.server.ts:463).
475+
const expired =
476+
cachedRun.idempotencyKeyExpiresAt &&
477+
cachedRun.idempotencyKeyExpiresAt < new Date();
478+
const failed = shouldIdempotencyKeyBeCleared(cachedRun.status);
479+
480+
if (expired || failed) {
454481
expiredRunIds.add(cachedRun.friendlyId);
455482

483+
// When two batch items share (task, idempotencyKey) AND the
484+
// cached run is cleared, downstream processing creates only ONE
485+
// run per key (TriggerTaskService.call honours the key and
486+
// reuses the first fresh run for every subsequent item).
487+
// Allocating distinct minted ids per item left the response
488+
// referencing ghost ids. Reuse the first replacement id for
489+
// every item sharing a (task, key). This also fixes the
490+
// pre-existing time-expiration branch. (Devin review on #5014.)
491+
const idemKey = item.options?.idempotencyKey;
492+
let replacementId: string | undefined = idemKey
493+
? replacementIdsByKey.get(`${item.task} ${idemKey}`)
494+
: undefined;
495+
if (!replacementId) {
496+
replacementId = await this.mintChildFriendlyId(
497+
environment,
498+
childAnchor,
499+
item.options?.region
500+
);
501+
if (idemKey) {
502+
replacementIdsByKey.set(`${item.task} ${idemKey}`, replacementId);
503+
}
504+
}
505+
456506
return {
457-
id: await this.mintChildFriendlyId(environment, childAnchor, item.options?.region),
507+
id: replacementId,
458508
isCached: false,
459-
idempotencyKey: item.options?.idempotencyKey ?? undefined,
509+
idempotencyKey: idemKey ?? undefined,
460510
taskIdentifier: item.task,
461511
};
462512
}

‎internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { postgresTest } from "@internal/testcontainers";
2-
import type { PrismaClient } from "@trigger.dev/database";
2+
import type { PrismaClient, TaskRunStatus } from "@trigger.dev/database";
33
import { describe, expect } from "vitest";
44
import { PostgresRunStore } from "./PostgresRunStore.js";
55

@@ -38,6 +38,7 @@ async function createRun(
3838
taskIdentifier: string;
3939
idempotencyKey: string;
4040
idempotencyKeyExpiresAt?: Date;
41+
status?: TaskRunStatus;
4142
}
4243
) {
4344
await prisma.taskRun.create({
@@ -46,6 +47,7 @@ async function createRun(
4647
taskIdentifier: params.taskIdentifier,
4748
idempotencyKey: params.idempotencyKey,
4849
idempotencyKeyExpiresAt: params.idempotencyKeyExpiresAt ?? null,
50+
status: params.status ?? "PENDING",
4951
payload: "{}",
5052
payloadType: "application/json",
5153
runtimeEnvironmentId: params.runtimeEnvironmentId,
@@ -117,4 +119,62 @@ describe("PostgresRunStore.findRunsByIdempotencyKeys", () => {
117119

118120
expect(rows).toEqual([]);
119121
});
122+
123+
// Regression for #4819 — batchTrigger needs the row's status to decide
124+
// whether to re-trigger (same semantics as single trigger via
125+
// shouldIdempotencyKeyBeCleared). Before this fix the SQL omitted `status`,
126+
// so batchTrigger could only inspect time-based expiry and silently handed
127+
// callers back dead CRASHED / COMPLETED_WITH_ERRORS runs as `isCached: true`.
128+
postgresTest(
129+
"returns run status so callers can honour shouldIdempotencyKeyBeCleared",
130+
async ({ prisma }) => {
131+
const { project, environment } = await seedEnvironment(prisma);
132+
const store = new PostgresRunStore({ prisma, readOnlyPrisma: prisma });
133+
134+
await createRun(prisma, {
135+
runtimeEnvironmentId: environment.id,
136+
projectId: project.id,
137+
friendlyId: "run_pending",
138+
taskIdentifier: "task-x",
139+
idempotencyKey: "key-pending",
140+
status: "PENDING",
141+
});
142+
await createRun(prisma, {
143+
runtimeEnvironmentId: environment.id,
144+
projectId: project.id,
145+
friendlyId: "run_crashed",
146+
taskIdentifier: "task-x",
147+
idempotencyKey: "key-crashed",
148+
status: "CRASHED",
149+
});
150+
await createRun(prisma, {
151+
runtimeEnvironmentId: environment.id,
152+
projectId: project.id,
153+
friendlyId: "run_completed_with_errors",
154+
taskIdentifier: "task-x",
155+
idempotencyKey: "key-cwe",
156+
status: "COMPLETED_WITH_ERRORS",
157+
});
158+
await createRun(prisma, {
159+
runtimeEnvironmentId: environment.id,
160+
projectId: project.id,
161+
friendlyId: "run_completed",
162+
taskIdentifier: "task-x",
163+
idempotencyKey: "key-ok",
164+
status: "COMPLETED_SUCCESSFULLY",
165+
});
166+
167+
const rows = await store.findRunsByIdempotencyKeys({
168+
runtimeEnvironmentId: environment.id,
169+
taskIdentifier: "task-x",
170+
idempotencyKeys: ["key-pending", "key-crashed", "key-cwe", "key-ok"],
171+
});
172+
173+
const byKey = new Map(rows.map((r) => [r.idempotencyKey, r]));
174+
expect(byKey.get("key-pending")?.status).toBe("PENDING");
175+
expect(byKey.get("key-crashed")?.status).toBe("CRASHED");
176+
expect(byKey.get("key-cwe")?.status).toBe("COMPLETED_WITH_ERRORS");
177+
expect(byKey.get("key-ok")?.status).toBe("COMPLETED_SUCCESSFULLY");
178+
}
179+
);
120180
});

‎internal-packages/run-store/src/PostgresRunStore.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1924,7 +1924,9 @@ export class PostgresRunStore implements RunStore {
19241924
const branches = args.idempotencyKeys.map((key) => {
19251925
const base = params.length;
19261926
params.push(args.runtimeEnvironmentId, args.taskIdentifier, key);
1927-
return `SELECT "id", "createdAt", "friendlyId", "idempotencyKey", "idempotencyKeyExpiresAt" FROM "TaskRun" WHERE "runtimeEnvironmentId" = $${base + 1} AND "taskIdentifier" = $${base + 2} AND "idempotencyKey" = $${base + 3}`;
1927+
// "status" is required so callers can honour shouldIdempotencyKeyBeCleared
1928+
// (closes #4819 — batchTrigger previously returned stale failed runs as cached).
1929+
return `SELECT "id", "createdAt", "friendlyId", "idempotencyKey", "idempotencyKeyExpiresAt", "status" FROM "TaskRun" WHERE "runtimeEnvironmentId" = $${base + 1} AND "taskIdentifier" = $${base + 2} AND "idempotencyKey" = $${base + 3}`;
19281930
});
19291931
return prisma.$queryRawUnsafe<IdempotencyKeyRunMatch[]>(
19301932
branches.join(" UNION ALL "),

‎internal-packages/run-store/src/types.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,14 @@ export type IdempotencyKeyRunMatch = {
3030
friendlyId: string;
3131
idempotencyKey: string | null;
3232
idempotencyKeyExpiresAt: Date | null;
33+
/**
34+
* Terminal run status is needed so callers can honour the same
35+
* "clear the idempotency key if the previous run failed" semantics the
36+
* single-trigger path already applies via `shouldIdempotencyKeyBeCleared`.
37+
* Without it, `batchTrigger` silently returned stale failed runs as
38+
* `isCached: true` (issue #4819).
39+
*/
40+
status: TaskRunStatus;
3341
};
3442

3543
/**

0 commit comments

Comments
 (0)