Skip to content

Commit ff7f461

Browse files
committed
fix(webapp): read the batch from the primary in the checkpoint WAIT_FOR_BATCH check
The pre-check decides whether to suspend a run on batchRun.resumedAt but read the batch from a replica. A batch that had just resumed the parent still looked unresumed on a lagging replica, so the service checkpointed (suspended) an already-resumed run and it stalled until a sweep. Read the primary, matching the sibling WAIT_FOR_TASK arm which already does.
1 parent c2593ef commit ff7f461

3 files changed

Lines changed: 182 additions & 5 deletions

File tree

apps/webapp/app/v3/services/createCheckpoint.server.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,14 @@ export class CreateCheckpointService extends BaseService {
147147
}
148148
case "WAIT_FOR_BATCH": {
149149
// Routed by friendlyId so a run-ops id (NEW-resident) batch is found on the owning DB;
150-
// env-scoped to the dependent attempt's run (a batch shares its dependent's env).
150+
// env-scoped to the dependent attempt's run (a batch shares its dependent's env). Read the
151+
// primary: a batch that just resumed the parent may lag the replica, and a stale resumedAt
152+
// (null) would checkpoint (suspend) an already-resumed run -> it stalls until a sweep.
151153
const batchRun = await this.runStore.findBatchTaskRunByFriendlyId(
152154
reason.batchFriendlyId,
153-
attempt.taskRun.runtimeEnvironmentId
155+
attempt.taskRun.runtimeEnvironmentId,
156+
undefined,
157+
this._prisma
154158
);
155159

156160
if (!batchRun) {
@@ -361,11 +365,13 @@ export class CreateCheckpointService extends BaseService {
361365
});
362366
await marqs?.cancelHeartbeat(attempt.taskRunId);
363367

364-
// Routed by friendlyId so a run-ops id (NEW-resident) batch is found on the owning DB;
365-
// env-scoped to the dependent attempt's run (a batch shares its dependent's env).
368+
// Routed by friendlyId; read the primary (this._prisma) so a just-resumed batch that still
369+
// lags the replica doesn't leave a stale resumedAt and suspend an already-resumed run.
366370
const batchRun = await this.runStore.findBatchTaskRunByFriendlyId(
367371
reason.batchFriendlyId,
368-
attempt.taskRun.runtimeEnvironmentId
372+
attempt.taskRun.runtimeEnvironmentId,
373+
undefined,
374+
this._prisma
369375
);
370376

371377
if (!batchRun) {
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Unit red-green for the checkpoint WAIT_FOR_BATCH replica-lag fix (createCheckpoint.server.ts).
2+
// The service decides whether to suspend a run on `batchRun.resumedAt`; reading it from a lagging
3+
// replica makes a just-resumed batch look unresumed -> it suspends an already-resumed run -> stall.
4+
// The fix threads the primary (`this._prisma`) into `runStore.findBatchTaskRunByFriendlyId`. Here a
5+
// spy runStore records which client the service passed and simulates the lag (only the primary read
6+
// sees the fresh resumedAt): RED = no client -> stale null -> no early return; GREEN = primary -> kept alive.
7+
8+
import { describe, expect, it, vi } from "vitest";
9+
10+
vi.mock("~/services/logger.server", () => ({
11+
logger: { debug: vi.fn(), info: vi.fn(), log: vi.fn(), error: vi.fn(), warn: vi.fn() },
12+
}));
13+
vi.mock("~/v3/marqs/index.server", () => ({
14+
marqs: { replaceMessage: vi.fn(), cancelHeartbeat: vi.fn() },
15+
}));
16+
17+
import { CreateCheckpointService } from "~/v3/services/createCheckpoint.server";
18+
19+
describe("checkpoint WAIT_FOR_BATCH reads the primary, not a lagging replica", () => {
20+
it("threads the primary so an already-resumed batch keeps the run alive", async () => {
21+
// A freezable attempt so control reaches the WAIT_FOR_BATCH arm. This object IS the primary the
22+
// fix must thread into the batch read.
23+
const prisma = {
24+
taskRunAttempt: {
25+
findFirst: async () => ({
26+
id: "attempt_1",
27+
status: "EXECUTING",
28+
taskRunId: "run_1",
29+
taskRun: { id: "run_1", status: "EXECUTING", runtimeEnvironmentId: "env_1" },
30+
backgroundWorker: { id: "bw_1", deployment: { imageReference: "img:1" } },
31+
}),
32+
},
33+
};
34+
35+
let seenClient: unknown = "NOT_CALLED";
36+
const runStore = {
37+
findBatchTaskRunByFriendlyId: async (
38+
_friendlyId: string,
39+
_environmentId: string,
40+
_args: unknown,
41+
client?: unknown
42+
) => {
43+
seenClient = client;
44+
// Lagging replica: only a read handed the primary sees the just-committed resumedAt.
45+
return { resumedAt: client === prisma ? new Date() : null };
46+
},
47+
};
48+
49+
const service = new CreateCheckpointService(prisma as never, {} as never, runStore as never);
50+
51+
let result: unknown;
52+
try {
53+
result = await service.call({
54+
attemptFriendlyId: "attempt_1",
55+
reason: { type: "WAIT_FOR_BATCH", batchFriendlyId: "batch_1" },
56+
} as never);
57+
} catch {
58+
// Buggy path falls through the pre-check into checkpoint creation (unstubbed) and throws; the
59+
// recorded client below is what distinguishes RED from GREEN.
60+
}
61+
62+
expect(seenClient).toBe(prisma); // the fix: primary threaded into the batch read
63+
expect(result).toEqual({ success: false, keepRunAlive: true }); // early-return, run kept alive
64+
});
65+
});
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Repro for the checkpoint WAIT_FOR_BATCH replica-lag stall (createCheckpoint.server.ts:148-181).
2+
//
3+
// The service resolves the batch via `runStore.findBatchTaskRunByFriendlyId(friendlyId, envId)` with
4+
// NO client, so the read is served from the REPLICA. Its decision hinges on `batchRun.resumedAt`:
5+
// resumedAt set -> return keepRunAlive:true (batch already resumed; the run must keep executing)
6+
// resumedAt null -> fall through -> create the checkpoint -> SUSPEND the run
7+
// If the batch just resumed (primary has resumedAt), but the replica still lags (resumedAt null), the
8+
// service suspends a run whose batch already completed -> it stalls until a sweep. The sibling
9+
// WAIT_FOR_TASK arm reads the primary (this._prisma); only the batch arm defaults to the replica.
10+
//
11+
// This is invisible to the normal single-DB harness (no lag). We reintroduce the lag with the shared
12+
// `laggingReplica` primitive: the store's replica is frozen at the pre-resume snapshot while the
13+
// primary advances. RED = the current (client-less, replica) read -> SUSPEND. GREEN = threading the
14+
// primary (the one-line fix) -> KEEP_ALIVE.
15+
16+
import { laggingReplica, postgresTest } from "@internal/testcontainers";
17+
import type { PrismaClient } from "@trigger.dev/database";
18+
import { describe, expect } from "vitest";
19+
import { PostgresRunStore } from "./PostgresRunStore.js";
20+
import type { ReadClient } from "./types.js";
21+
22+
type BatchRow = { resumedAt: Date | null } | null;
23+
24+
// A line-for-line mirror of the service's WAIT_FOR_BATCH pre-check. `readClient` models the fix: the
25+
// buggy code passes nothing (replica default); the fix threads the primary.
26+
async function precheckWaitForBatch(
27+
store: PostgresRunStore,
28+
batchFriendlyId: string,
29+
environmentId: string,
30+
readClient?: ReadClient
31+
): Promise<"DROP_RUN" | "KEEP_ALIVE" | "SUSPEND"> {
32+
const batchRun = (await store.findBatchTaskRunByFriendlyId(
33+
batchFriendlyId,
34+
environmentId,
35+
undefined,
36+
readClient
37+
)) as BatchRow;
38+
if (!batchRun) return "DROP_RUN"; // keepRunAlive:false
39+
if (batchRun.resumedAt) return "KEEP_ALIVE"; // batch already resumed -> run continues
40+
return "SUSPEND"; // falls through -> checkpoint created -> run suspended
41+
}
42+
43+
async function seedEnvironment(prisma: PrismaClient, suffix: string) {
44+
const organization = await prisma.organization.create({
45+
data: { title: `Org ${suffix}`, slug: `org-${suffix}` },
46+
});
47+
const project = await prisma.project.create({
48+
data: {
49+
name: `Project ${suffix}`,
50+
slug: `project-${suffix}`,
51+
externalRef: `proj_${suffix}`,
52+
organizationId: organization.id,
53+
},
54+
});
55+
const environment = await prisma.runtimeEnvironment.create({
56+
data: {
57+
type: "DEVELOPMENT",
58+
slug: "dev",
59+
projectId: project.id,
60+
organizationId: organization.id,
61+
apiKey: `tr_dev_${suffix}`,
62+
pkApiKey: `pk_dev_${suffix}`,
63+
shortcode: `short_${suffix}`,
64+
},
65+
});
66+
return { organization, project, environment };
67+
}
68+
69+
describe("checkpoint WAIT_FOR_BATCH under replica lag", () => {
70+
postgresTest(
71+
"a batch resumed on the primary but stale on the replica suspends an already-resumed run",
72+
async ({ prisma }) => {
73+
const { environment } = await seedEnvironment(prisma, "ckpt_lag");
74+
const friendlyId = "batch_ckpt_lag";
75+
const batch = await prisma.batchTaskRun.create({
76+
data: { friendlyId, runtimeEnvironmentId: environment.id },
77+
});
78+
79+
// Snapshot the batch as the replica still sees it (pre-resume: resumedAt = null).
80+
const staleBatch = await prisma.batchTaskRun.findFirstOrThrow({ where: { id: batch.id } });
81+
expect(staleBatch.resumedAt).toBeNull();
82+
83+
// The batch completes and resumes the parent: primary now has resumedAt set...
84+
await prisma.batchTaskRun.update({ where: { id: batch.id }, data: { resumedAt: new Date() } });
85+
86+
// ...but the replica lags, frozen at the pre-resume snapshot.
87+
const replica = laggingReplica(prisma, [
88+
{ model: "batchTaskRun", mode: "frozen", rows: [staleBatch] },
89+
]);
90+
const store = new PostgresRunStore({
91+
prisma,
92+
readOnlyPrisma: replica.client,
93+
schemaVariant: "legacy",
94+
});
95+
96+
// RED - the current service call (no client -> replica): stale null -> SUSPEND an already-resumed run.
97+
const buggy = await precheckWaitForBatch(store, friendlyId, environment.id);
98+
expect(replica.wasHit("batchTaskRun")).toBe(true); // proves it read the (stale) replica
99+
expect(buggy).toBe("SUSPEND"); // the stall bug
100+
101+
// GREEN - the fix (thread the primary): sees resumedAt -> keep the run alive.
102+
const fixed = await precheckWaitForBatch(store, friendlyId, environment.id, prisma);
103+
expect(fixed).toBe("KEEP_ALIVE");
104+
}
105+
);
106+
});

0 commit comments

Comments
 (0)