Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .server-changes/batch-trigger-idempotency-key-status-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

Re-trigger runs in batchTrigger when an idempotency key matches a failed or dead run.
23 changes: 20 additions & 3 deletions apps/webapp/app/v3/services/batchTriggerV3.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type {
IOPacket,
} from "@trigger.dev/core/v3";
import { packetRequiresOffloading, parsePacket } from "@trigger.dev/core/v3";
import type { BatchTaskRun, TaskRunAttempt } from "@trigger.dev/database";
import type { BatchTaskRun, TaskRunAttempt, TaskRunStatus } from "@trigger.dev/database";
import { isUniqueConstraintError, Prisma } from "@trigger.dev/database";
import type { RunStore } from "@internal/run-store";
import pMap from "p-map";
Expand All @@ -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";
Expand Down Expand Up @@ -377,6 +381,14 @@ export class BatchTriggerV3Service extends BaseService {
return mintFriendlyIdForKind({ ...target, region });
}

private async prepareRunData(
environment: AuthenticatedEnvironment,
body: BatchTriggerTaskV2RequestBody,
batchFriendlyId: string
): Promise<Array<RunItemData>> {
return this.#prepareRunData(environment, body, batchFriendlyId);
}

async #prepareRunData(
environment: AuthenticatedEnvironment,
body: BatchTriggerTaskV2RequestBody,
Expand Down Expand Up @@ -450,7 +462,12 @@ export class BatchTriggerV3Service extends BaseService {
);

if (cachedRun) {
if (cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date()) {
const isExpired =
cachedRun.idempotencyKeyExpiresAt && cachedRun.idempotencyKeyExpiresAt < new Date();
const shouldClear =
isExpired || shouldIdempotencyKeyBeCleared(cachedRun.status as TaskRunStatus);
Comment on lines +467 to +468

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.


if (shouldClear) {
expiredRunIds.add(cachedRun.friendlyId);

return {
Comment on lines +470 to 473

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down
339 changes: 339 additions & 0 deletions apps/webapp/test/batchTriggerV3IdempotencyKeyStatus.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,339 @@
import { describe, expect, it, vi } from "vitest";

// 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";
Comment on lines +3 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.


vi.setConfig({ testTimeout: 60_000 });

function fakeEnv(): AuthenticatedEnvironment {
return {
id: "env_test_123",
organizationId: "org_test_123",
organization: { featureFlags: {} },
type: "DEVELOPMENT",
} as unknown as AuthenticatedEnvironment;
}

describe("shouldIdempotencyKeyBeCleared (unit)", () => {
const CLEARABLE_STATUSES: TaskRunStatus[] = [
"CRASHED",
"SYSTEM_FAILURE",
"TIMED_OUT",
"EXPIRED",
"COMPLETED_WITH_ERRORS",
"INTERRUPTED",
];

const NON_CLEARABLE_STATUSES: TaskRunStatus[] = [
"COMPLETED_SUCCESSFULLY",
"EXECUTING",
"PENDING",
"WAITING_FOR_DEPLOY",
"PAUSED",
"DELAYED",
"CANCELED",
];

for (const status of CLEARABLE_STATUSES) {
it(`returns true for clearable failure status: ${status}`, () => {
expect(shouldIdempotencyKeyBeCleared(status)).toBe(true);
});
}

for (const status of NON_CLEARABLE_STATUSES) {
it(`returns false for non-clearable status: ${status}`, () => {
expect(shouldIdempotencyKeyBeCleared(status)).toBe(false);
});
}
});

describe("BatchTriggerV3Service #prepareRunData idempotency key status check", () => {
const CLEARABLE_FAILURE_STATUSES: TaskRunStatus[] = [
"CRASHED",
"SYSTEM_FAILURE",
"TIMED_OUT",
"EXPIRED",
"COMPLETED_WITH_ERRORS",
"INTERRUPTED",
];

const NON_CLEARABLE_STATUSES: TaskRunStatus[] = [
"COMPLETED_SUCCESSFULLY",
"EXECUTING",
"PENDING",
"WAITING_FOR_DEPLOY",
];

for (const status of CLEARABLE_FAILURE_STATUSES) {
it(`clears idempotency key and mints fresh run for clearable status: ${status}`, async () => {
const clearIdempotencyKeyMock = vi.fn().mockResolvedValue({ count: 1 });
const mockRunStore = {
findRunsByIdempotencyKeys: vi.fn().mockResolvedValue([
{
id: "run_internal_dead",
createdAt: new Date(),
friendlyId: "run_dead_123",
idempotencyKey: "key_dead",
idempotencyKeyExpiresAt: null,
status,
},
]),
clearIdempotencyKey: clearIdempotencyKeyMock,
};

const service = new BatchTriggerV3Service(
undefined,
undefined,
{} as any,
mockRunStore as any,
async () => "cuid"
);

const body = {
items: [
{
task: "test-task",
payload: "{}",
options: {
idempotencyKey: "key_dead",
},
},
],
};

const runs = await (service as any).prepareRunData(fakeEnv(), body, "batch_123");

// Verify clearIdempotencyKey was called for the dead run
expect(clearIdempotencyKeyMock).toHaveBeenCalledTimes(1);
expect(clearIdempotencyKeyMock).toHaveBeenCalledWith(
{ byFriendlyIds: ["run_dead_123"] },
expect.anything()
);

// Verify the run returned is NOT cached and has a freshly minted ID
expect(runs).toHaveLength(1);
expect(runs[0].isCached).toBe(false);
expect(runs[0].id).not.toBe("run_dead_123");
expect(runs[0].taskIdentifier).toBe("test-task");
expect(runs[0].idempotencyKey).toBe("key_dead");
});
}

for (const status of NON_CLEARABLE_STATUSES) {
it(`reuses cached run and does NOT clear key for status: ${status}`, async () => {
const clearIdempotencyKeyMock = vi.fn().mockResolvedValue({ count: 0 });
const mockRunStore = {
findRunsByIdempotencyKeys: vi.fn().mockResolvedValue([
{
id: "run_internal_live",
createdAt: new Date(),
friendlyId: "run_live_123",
idempotencyKey: "key_live",
idempotencyKeyExpiresAt: null,
status,
},
]),
clearIdempotencyKey: clearIdempotencyKeyMock,
};

const service = new BatchTriggerV3Service(
undefined,
undefined,
{} as any,
mockRunStore as any,
async () => "cuid"
);

const body = {
items: [
{
task: "test-task",
payload: "{}",
options: {
idempotencyKey: "key_live",
},
},
],
};

const runs = await (service as any).prepareRunData(fakeEnv(), body, "batch_123");

// Verify clearIdempotencyKey was NOT called
expect(clearIdempotencyKeyMock).not.toHaveBeenCalled();

// Verify the run returned IS cached and preserves existing run ID
expect(runs).toHaveLength(1);
expect(runs[0].isCached).toBe(true);
expect(runs[0].id).toBe("run_live_123");
expect(runs[0].taskIdentifier).toBe("test-task");
expect(runs[0].idempotencyKey).toBe("key_live");
});
}

it("handles a mixed batch with fresh keys, live cached runs, and dead runs correctly", async () => {
const clearIdempotencyKeyMock = vi.fn().mockResolvedValue({ count: 7 });
const mockRunStore = {
findRunsByIdempotencyKeys: vi.fn().mockImplementation(async ({ idempotencyKeys }) => {
const matches = [
{
id: "r1",
createdAt: new Date(),
friendlyId: "run_crashed",
idempotencyKey: "k_crashed",
idempotencyKeyExpiresAt: null,
status: "CRASHED",
},
{
id: "r2",
createdAt: new Date(),
friendlyId: "run_success",
idempotencyKey: "k_success",
idempotencyKeyExpiresAt: null,
status: "COMPLETED_SUCCESSFULLY",
},
{
id: "r3",
createdAt: new Date(),
friendlyId: "run_sys_fail",
idempotencyKey: "k_sys_fail",
idempotencyKeyExpiresAt: null,
status: "SYSTEM_FAILURE",
},
{
id: "r4",
createdAt: new Date(),
friendlyId: "run_executing",
idempotencyKey: "k_executing",
idempotencyKeyExpiresAt: null,
status: "EXECUTING",
},
{
id: "r5",
createdAt: new Date(),
friendlyId: "run_timeout",
idempotencyKey: "k_timeout",
idempotencyKeyExpiresAt: null,
status: "TIMED_OUT",
},
{
id: "r6",
createdAt: new Date(),
friendlyId: "run_errors",
idempotencyKey: "k_errors",
idempotencyKeyExpiresAt: null,
status: "COMPLETED_WITH_ERRORS",
},
{
id: "r7",
createdAt: new Date(),
friendlyId: "run_interrupted",
idempotencyKey: "k_interrupted",
idempotencyKeyExpiresAt: null,
status: "INTERRUPTED",
},
{
id: "r8",
createdAt: new Date(),
friendlyId: "run_expired_status",
idempotencyKey: "k_expired_status",
idempotencyKeyExpiresAt: null,
status: "EXPIRED",
},
{
id: "r9",
createdAt: new Date(),
friendlyId: "run_expired_ttl",
idempotencyKey: "k_expired_ttl",
idempotencyKeyExpiresAt: new Date(Date.now() - 10_000),
status: "PENDING",
},
];
return matches.filter((m) => idempotencyKeys.includes(m.idempotencyKey));
}),
clearIdempotencyKey: clearIdempotencyKeyMock,
};

const service = new BatchTriggerV3Service(
undefined,
undefined,
{} as any,
mockRunStore as any,
async () => "cuid"
);

const body = {
items: [
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_crashed" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_success" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_sys_fail" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_executing" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_timeout" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_errors" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_interrupted" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_expired_status" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_expired_ttl" } },
{ task: "t1", payload: "{}", options: { idempotencyKey: "k_brand_new" } },
],
};

const runs = await (service as any).prepareRunData(fakeEnv(), body, "batch_mixed_123");

// All clearable failure statuses + expired TTL run must be cleared
expect(clearIdempotencyKeyMock).toHaveBeenCalledTimes(1);
const clearedIds = clearIdempotencyKeyMock.mock.calls[0][0].byFriendlyIds.sort();
expect(clearedIds).toEqual(
[
"run_crashed",
"run_sys_fail",
"run_timeout",
"run_errors",
"run_interrupted",
"run_expired_status",
"run_expired_ttl",
].sort()
);

expect(runs).toHaveLength(10);
// k_crashed: cleared, minted new
expect(runs[0].isCached).toBe(false);
expect(runs[0].id).not.toBe("run_crashed");
// k_success: live, cached
expect(runs[1].isCached).toBe(true);
expect(runs[1].id).toBe("run_success");
// k_sys_fail: cleared, minted new
expect(runs[2].isCached).toBe(false);
expect(runs[2].id).not.toBe("run_sys_fail");
// k_executing: live, cached
expect(runs[3].isCached).toBe(true);
expect(runs[3].id).toBe("run_executing");
// k_timeout: cleared, minted new
expect(runs[4].isCached).toBe(false);
expect(runs[4].id).not.toBe("run_timeout");
// k_errors: cleared, minted new
expect(runs[5].isCached).toBe(false);
expect(runs[5].id).not.toBe("run_errors");
// k_interrupted: cleared, minted new
expect(runs[6].isCached).toBe(false);
expect(runs[6].id).not.toBe("run_interrupted");
// k_expired_status: cleared, minted new
expect(runs[7].isCached).toBe(false);
expect(runs[7].id).not.toBe("run_expired_status");
// k_expired_ttl: cleared, minted new
expect(runs[8].isCached).toBe(false);
expect(runs[8].id).not.toBe("run_expired_ttl");
// k_brand_new: fresh, minted new
expect(runs[9].isCached).toBe(false);
});
});
Loading