From a39efebbe0cd812368df44985539c1cf9a73c9c6 Mon Sep 17 00:00:00 2001 From: Abd Alrahaman Date: Sat, 10 Oct 2026 20:36:21 +0300 Subject: [PATCH] fix(batchTrigger): clear idempotency key when previous run failed (closes #4819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #4819 at time of writing. --- .../app/v3/services/batchTriggerV3.server.ts | 25 +++++++- ...RunStore.findRunsByIdempotencyKeys.test.ts | 62 ++++++++++++++++++- .../run-store/src/PostgresRunStore.ts | 4 +- internal-packages/run-store/src/types.ts | 8 +++ 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/apps/webapp/app/v3/services/batchTriggerV3.server.ts b/apps/webapp/app/v3/services/batchTriggerV3.server.ts index b86e5a40a64..c2b091f7f1d 100644 --- a/apps/webapp/app/v3/services/batchTriggerV3.server.ts +++ b/apps/webapp/app/v3/services/batchTriggerV3.server.ts @@ -27,7 +27,11 @@ import { mintBatchFriendlyId } from "~/v3/runOpsMigration/mintBatchFriendlyId.se import { batchTriggerWorker } from "../batchTriggerWorker.server"; import { guardQueueSizeLimitsForEnv } from "../queueSizeLimits.server"; import { downloadPacketFromObjectStore, uploadPacketToObjectStore } from "../objectStore.server"; -import { isFinalAttemptStatus, isFinalRunStatus } from "../taskStatus"; +import { + isFinalAttemptStatus, + isFinalRunStatus, + shouldIdempotencyKeyBeCleared, +} from "../taskStatus"; import { startActiveSpan } from "../tracer.server"; import { BaseService, ServiceValidationError } from "./baseService.server"; import { OutOfEntitlementError, TriggerTaskService } from "./triggerTask.server"; @@ -450,7 +454,24 @@ export class BatchTriggerV3Service extends BaseService { ); if (cachedRun) { - if (cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date()) { + // Clear the idempotency key and mint a fresh run when either: + // (a) the key's time-based expiry has passed, OR + // (b) the previous run ended in a terminal failure state the + // single-trigger path treats as "retry allowed" + // (CRASHED, SYSTEM_FAILURE, INTERRUPTED, COMPLETED_WITH_ERRORS, + // EXPIRED, TIMED_OUT_WITH_ERRORS — see shouldIdempotencyKeyBeCleared). + // + // Before this fix, batchTrigger only checked (a). A failed run with + // the same idempotency key was returned as `isCached: true`, silently + // handing the caller back a dead run that would never produce output + // (issue #4819). This mirrors the single-trigger behaviour at + // IdempotencyKeyConcern.handleExistingRun (idempotencyKeys.server.ts:463). + const expired = + cachedRun.idempotencyKeyExpiresAt && + cachedRun.idempotencyKeyExpiresAt < new Date(); + const failed = shouldIdempotencyKeyBeCleared(cachedRun.status); + + if (expired || failed) { expiredRunIds.add(cachedRun.friendlyId); return { diff --git a/internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts b/internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts index bc108cd1122..dff3e3ca663 100644 --- a/internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts +++ b/internal-packages/run-store/src/PostgresRunStore.findRunsByIdempotencyKeys.test.ts @@ -1,5 +1,5 @@ import { postgresTest } from "@internal/testcontainers"; -import type { PrismaClient } from "@trigger.dev/database"; +import type { PrismaClient, TaskRunStatus } from "@trigger.dev/database"; import { describe, expect } from "vitest"; import { PostgresRunStore } from "./PostgresRunStore.js"; @@ -38,6 +38,7 @@ async function createRun( taskIdentifier: string; idempotencyKey: string; idempotencyKeyExpiresAt?: Date; + status?: TaskRunStatus; } ) { await prisma.taskRun.create({ @@ -46,6 +47,7 @@ async function createRun( taskIdentifier: params.taskIdentifier, idempotencyKey: params.idempotencyKey, idempotencyKeyExpiresAt: params.idempotencyKeyExpiresAt ?? null, + status: params.status ?? "PENDING", payload: "{}", payloadType: "application/json", runtimeEnvironmentId: params.runtimeEnvironmentId, @@ -117,4 +119,62 @@ describe("PostgresRunStore.findRunsByIdempotencyKeys", () => { expect(rows).toEqual([]); }); + + // Regression for #4819 — batchTrigger needs the row's status to decide + // whether to re-trigger (same semantics as single trigger via + // shouldIdempotencyKeyBeCleared). Before this fix the SQL omitted `status`, + // so batchTrigger could only inspect time-based expiry and silently handed + // callers back dead CRASHED / COMPLETED_WITH_ERRORS runs as `isCached: true`. + postgresTest( + "returns run status so callers can honour shouldIdempotencyKeyBeCleared", + async ({ prisma }) => { + const { project, environment } = await seedEnvironment(prisma); + const store = new PostgresRunStore({ prisma, readOnlyPrisma: prisma }); + + await createRun(prisma, { + runtimeEnvironmentId: environment.id, + projectId: project.id, + friendlyId: "run_pending", + taskIdentifier: "task-x", + idempotencyKey: "key-pending", + status: "PENDING", + }); + await createRun(prisma, { + runtimeEnvironmentId: environment.id, + projectId: project.id, + friendlyId: "run_crashed", + taskIdentifier: "task-x", + idempotencyKey: "key-crashed", + status: "CRASHED", + }); + await createRun(prisma, { + runtimeEnvironmentId: environment.id, + projectId: project.id, + friendlyId: "run_completed_with_errors", + taskIdentifier: "task-x", + idempotencyKey: "key-cwe", + status: "COMPLETED_WITH_ERRORS", + }); + await createRun(prisma, { + runtimeEnvironmentId: environment.id, + projectId: project.id, + friendlyId: "run_completed", + taskIdentifier: "task-x", + idempotencyKey: "key-ok", + status: "COMPLETED_SUCCESSFULLY", + }); + + 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"); + } + ); }); diff --git a/internal-packages/run-store/src/PostgresRunStore.ts b/internal-packages/run-store/src/PostgresRunStore.ts index e66c9237a21..5714f9c9da0 100644 --- a/internal-packages/run-store/src/PostgresRunStore.ts +++ b/internal-packages/run-store/src/PostgresRunStore.ts @@ -1924,7 +1924,9 @@ export class PostgresRunStore implements RunStore { const branches = args.idempotencyKeys.map((key) => { const base = params.length; params.push(args.runtimeEnvironmentId, args.taskIdentifier, key); - return `SELECT "id", "createdAt", "friendlyId", "idempotencyKey", "idempotencyKeyExpiresAt" FROM "TaskRun" WHERE "runtimeEnvironmentId" = $${base + 1} AND "taskIdentifier" = $${base + 2} AND "idempotencyKey" = $${base + 3}`; + // "status" is required so callers can honour shouldIdempotencyKeyBeCleared + // (closes #4819 — batchTrigger previously returned stale failed runs as cached). + return `SELECT "id", "createdAt", "friendlyId", "idempotencyKey", "idempotencyKeyExpiresAt", "status" FROM "TaskRun" WHERE "runtimeEnvironmentId" = $${base + 1} AND "taskIdentifier" = $${base + 2} AND "idempotencyKey" = $${base + 3}`; }); return prisma.$queryRawUnsafe( branches.join(" UNION ALL "), diff --git a/internal-packages/run-store/src/types.ts b/internal-packages/run-store/src/types.ts index 660994eb95e..6233a6f7b24 100644 --- a/internal-packages/run-store/src/types.ts +++ b/internal-packages/run-store/src/types.ts @@ -30,6 +30,14 @@ export type IdempotencyKeyRunMatch = { friendlyId: string; idempotencyKey: string | null; idempotencyKeyExpiresAt: Date | null; + /** + * Terminal run status is needed so callers can honour the same + * "clear the idempotency key if the previous run failed" semantics the + * single-trigger path already applies via `shouldIdempotencyKeyBeCleared`. + * Without it, `batchTrigger` silently returned stale failed runs as + * `isCached: true` (issue #4819). + */ + status: TaskRunStatus; }; /**