Skip to content

Commit 7011b57

Browse files
committed
fix(run-engine,run-store): close the review findings on the waitpoint envelope
Coverage check now runs over the whole membership, not the order. The order omits every index-less wait by construction, so an order-scoped check could not see an index-less id whose record was missing — the exact loss the resolver exists to prevent. Adds distinctIds to the resolver args and updates the jointly-owned freeze pin. A refused copy-forward no longer mints a records-less cycle. Copy-forward appends carry no records of their own, and the append script can refuse a pointer and mint a replacement from the carried refs, so the decorator reads the surviving cycle's records and carries those. A deriveFromRun record whose run output is gone now fails loud instead of resolving to an empty output. Postgres does not lose it: the back-reference nulls on delete but the stored output stays, so returning undefined would resolve a triggerAndWait with silently wrong data. The legacy arm passes the routing hint it was dropping, so a resume reads the run's own store instead of fanning out across every run-ops database, and reuses the chunked fetch rather than reading a large fan-in whole. The envelope read issues one command per id concurrently rather than as a pipeline. Each id is its own hash tag, so N ids are N cluster slots and a pipeline spanning them is rejected under cluster mode — which a single-node test server would never surface. Also: shares one row-to-source mapper between the legacy arm and the equivalence suite, so a bug in the arm can no longer hide from the oracle; pins the deliberate BATCH-output divergence and corrects the comment that gave the wrong reason for it; gates the record build on id format rather than claiming residency; and builds the record set inside the two branches that append rather than before the statuses that return without appending.
1 parent 7f9da73 commit 7011b57

15 files changed

Lines changed: 478 additions & 138 deletions

internal-packages/run-engine/src/engine/systems/completedWaitpointFreeze.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ const _pointerKeys: Exact<keyof CompletedWaitpointsPointer, "cycleSeq" | "count"
4040

4141
const _argsKeys: Exact<
4242
keyof ResolveCompletedWaitpointsArgs,
43-
"runId" | "batchId" | "pointer" | "order" | "records"
43+
"runId" | "batchId" | "pointer" | "order" | "distinctIds" | "records"
4444
> = true;
4545
import { enhanceExecutionSnapshotWithWaitpoints } from "./executionSnapshotSystem.js";
4646

@@ -193,6 +193,7 @@ async function assertParity(
193193
batchId: batchId ?? undefined,
194194
pointer: { cycleSeq: 1, count: order.length },
195195
order,
196+
distinctIds: [...new Set(waitpoints.map((w) => w.id))],
196197
records: waitpoints.map(toRecord),
197198
};
198199
// count-carried-forward behaviour (order.length, not the record count) is covered by
@@ -406,6 +407,7 @@ describe("the completed-waitpoints freeze", () => {
406407
batchId: undefined,
407408
pointer: { cycleSeq: 1, count: 1 },
408409
order: ["wp_hook"],
410+
distinctIds: ["wp_hook"],
409411
records: [toRecord(w)],
410412
});
411413
expect(resolved).toHaveLength(1);
@@ -611,6 +613,7 @@ describe("the exhaustive parity grid", () => {
611613
batchId: readingBatchId ?? undefined,
612614
pointer: { cycleSeq: 1, count: order.length },
613615
order,
616+
distinctIds: [w.id],
614617
records: [toRecord(w)],
615618
};
616619
const resolved = await referenceResolver(

internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ async function getSnapshotWaitpointIdsWithPresence(
173173
* This is necessary because waitpoints can have large outputs (100KB+),
174174
* and fetching many at once can exceed Node.js string limits.
175175
*/
176-
async function fetchWaitpointsInChunks(
176+
export async function fetchWaitpointsInChunks(
177177
prisma: PrismaClientOrTransaction,
178178
waitpointIds: string[],
179179
runStore?: RunStore,

internal-packages/run-engine/src/engine/systems/waitpointSystem.ts

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -487,14 +487,6 @@ export class WaitpointSystem {
487487
};
488488
}
489489

490-
// The record set rides the wait cycle's key once per resume, so build it here rather
491-
// than at each append site. Nothing mints a store-format waitpoint yet, so
492-
// #completedWaitpointRecordsFor returns undefined on every live path today.
493-
const completedWaitpointRecords = await this.#completedWaitpointRecordsFor(
494-
runId,
495-
blockingWaitpoints
496-
);
497-
498490
// 3. Get the run (run-ops scalars) + resolve its environment via the control-plane resolver,
499491
// so the run-ops DB can split without a cross-provider join.
500492
const run = await this.$.runStore.findRun(
@@ -612,6 +604,13 @@ export class WaitpointSystem {
612604
};
613605
}
614606
case "EXECUTING_WITH_WAITPOINTS": {
607+
// Built inside the branch, not before the switch: the statuses above return without
608+
// appending, and they must not pay an envelope read to do it.
609+
const completedWaitpointRecords = await this.#completedWaitpointRecordsFor(
610+
runId,
611+
blockingWaitpoints
612+
);
613+
615614
const newSnapshot = await this.executionSnapshotSystem.createExecutionSnapshot(
616615
this.$.prisma,
617616
{
@@ -680,6 +679,11 @@ export class WaitpointSystem {
680679
);
681680
}
682681

682+
const completedWaitpointRecords = await this.#completedWaitpointRecordsFor(
683+
runId,
684+
blockingWaitpoints
685+
);
686+
683687
//put it back in the queue, with the original timestamp (w/ priority)
684688
//this prioritizes dequeuing waiting runs over new runs
685689
const newSnapshot = await this.enqueueSystem.enqueueRun({
@@ -742,31 +746,36 @@ export class WaitpointSystem {
742746
}
743747

744748
/**
745-
* The record set for one resume, or undefined when this wait has no store-resident half.
749+
* The record set for one resume, or undefined when no blocking waitpoint carries a store-format
750+
* id.
751+
*
752+
* Gated on id FORMAT, not residency. The two are not the same during a migration: a
753+
* store-format id can still be served by the Postgres arm, exactly as run-ops ids were for
754+
* runs. Whichever arm owns it answers, so the gate only decides whether to ask at all.
746755
*
747-
* The classification gate is what keeps this inert. `parseWaitpointId` reports legacy for
748-
* every id minted today, so no live resume reads an envelope or writes a record until a
749-
* waitpoint mints in store format.
756+
* That gate is what keeps this inert. `parseWaitpointId` reports legacy for every id minted
757+
* today, so no live resume reads an envelope or writes a record until a waitpoint mints in
758+
* store format.
750759
*/
751760
async #completedWaitpointRecordsFor(
752761
runId: string,
753762
blockingWaitpoints: RunBlockEdge[]
754763
): Promise<CompletedWaitpointRecord[] | undefined> {
755-
const storeResidentIds = [
764+
const storeFormatIds = [
756765
...new Set(
757766
blockingWaitpoints
758767
.map((b) => b.waitpoint.id)
759768
.filter((id) => parseWaitpointId(id).format === "b32hexW")
760769
),
761770
];
762771

763-
if (storeResidentIds.length === 0) {
772+
if (storeFormatIds.length === 0) {
764773
return undefined;
765774
}
766775

767776
const sources = await this.coordinator.readCompletionEnvelopes({
768777
runId,
769-
waitpointIds: storeResidentIds,
778+
waitpointIds: storeFormatIds,
770779
});
771780

772781
return buildCompletedWaitpointRecords(sources);

internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { describe, expect, it } from "vitest";
66
import { enhanceExecutionSnapshotWithWaitpoints } from "../systems/executionSnapshotSystem.js";
77
import { buildCompletedWaitpointRecords } from "./completedWaitpointRecords.js";
88
import { createCompletedWaitpointResolver } from "./completedWaitpointResolver.js";
9+
import { envelopeSourceFromWaitpointRow } from "./completionEnvelopeSource.js";
910
import type { CompletionEnvelopeSource } from "./types.js";
1011

1112
const COMPLETED_AT = new Date("2026-08-25T00:00:00.000Z");
@@ -30,20 +31,15 @@ function pair(overrides: {
3031
userProvidedIdempotencyKey?: boolean;
3132
inactiveIdempotencyKey?: string | null;
3233
}): { row: Waitpoint; source: CompletionEnvelopeSource } {
33-
const outputType = overrides.outputType ?? "application/json";
34-
const outputIsError = overrides.outputIsError ?? false;
35-
const output = overrides.output ?? null;
36-
const isRef = outputType === "application/store";
37-
3834
const row = {
3935
id: overrides.id,
4036
friendlyId: `waitpoint_${overrides.id}`,
4137
type: overrides.type,
4238
status: "COMPLETED",
4339
completedAt: COMPLETED_AT,
44-
output,
45-
outputType,
46-
outputIsError,
40+
output: overrides.output ?? null,
41+
outputType: overrides.outputType ?? "application/json",
42+
outputIsError: overrides.outputIsError ?? false,
4743
completedByTaskRunId: overrides.completedByTaskRunId ?? null,
4844
completedByBatchId: overrides.completedByBatchId ?? null,
4945
completedAfter: overrides.completedAfter ?? null,
@@ -52,27 +48,9 @@ function pair(overrides: {
5248
inactiveIdempotencyKey: overrides.inactiveIdempotencyKey ?? null,
5349
} as unknown as Waitpoint;
5450

55-
const source: CompletionEnvelopeSource = {
56-
id: overrides.id,
57-
friendlyId: `waitpoint_${overrides.id}`,
58-
type: overrides.type,
59-
completedAt: COMPLETED_AT,
60-
outputType,
61-
outputIsError,
62-
...(output !== null ? (isRef ? { outputRef: output } : { output }) : {}),
63-
...(overrides.completedByTaskRunId && {
64-
completedByTaskRunId: overrides.completedByTaskRunId,
65-
}),
66-
...(overrides.completedByBatchId && { completedByBatchId: overrides.completedByBatchId }),
67-
...(overrides.completedAfter && { completedAfter: overrides.completedAfter }),
68-
...(overrides.userProvidedIdempotencyKey &&
69-
!overrides.inactiveIdempotencyKey &&
70-
overrides.idempotencyKey
71-
? { idempotencyKey: overrides.idempotencyKey }
72-
: {}),
73-
};
74-
75-
return { row, source };
51+
// Through the SHARED mapper the legacy arm uses. A hand-rolled copy here would make a bug in
52+
// that arm invisible to every case below, because the oracle chain would never touch it.
53+
return { row, source: envelopeSourceFromWaitpointRow(row) };
7654
}
7755

7856
function snapshot(batchId: string | null) {
@@ -116,6 +94,7 @@ async function bothPaths(
11694
...(batchId ? { batchId } : {}),
11795
pointer: { cycleSeq: 1, count: order.length },
11896
order,
97+
distinctIds: [...new Set(pairs.map((p) => p.row.id))],
11998
records: buildCompletedWaitpointRecords(pairs.map((p) => p.source)),
12099
});
121100

@@ -297,6 +276,29 @@ describe("the resolver reproduces the existing hydration", () => {
297276
expect(actual.find((w) => w.id === "wp_indexless")?.index).toBeUndefined();
298277
});
299278

279+
// The ONE intentional divergence from the oracle. A BATCH waitpoint really is completed with
280+
// an output, but the executor never reads it (sharedRuntimeManager.resolveWaitpoint
281+
// early-returns on type). Pinned so that if that early return ever goes away, this fails and
282+
// says why, instead of the output silently being missing at resume.
283+
it("deliberately drops a BATCH output, unlike the oracle", async () => {
284+
const { expected, actual } = await bothPaths(
285+
[
286+
pair({
287+
id: "wp_batch",
288+
type: "BATCH",
289+
completedByBatchId: BATCH_ID,
290+
output: '{"message":"batch expired"}',
291+
outputIsError: true,
292+
}),
293+
],
294+
[]
295+
);
296+
297+
expect(expected[0]?.output).toBe('{"message":"batch expired"}');
298+
expect(actual[0]?.output).toBeUndefined();
299+
expect(actual[0]?.outputIsError).toBe(true);
300+
});
301+
300302
it("for every type at once, under a batch", async () => {
301303
const { expected, actual } = await bothPaths(
302304
[

internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,10 @@ function chooseOutput(source: CompletionEnvelopeSource): CompletedWaitpointRecor
5050
return { deriveFromRun: true };
5151
}
5252

53-
// The runtime discards a batch output at source, so there is nothing to carry.
53+
// Deliberately dropped, and this is the one place the record set does NOT reproduce the row.
54+
// A BATCH waitpoint IS completed with an output (see batchSystem), but the executor ignores
55+
// it: sharedRuntimeManager.resolveWaitpoint early-returns for type === "BATCH" and never
56+
// reads the body. Carrying it would put bytes in the cycle key that nothing can observe.
5457
if (source.type === "BATCH") {
5558
return null;
5659
}

internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.ts

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { describe, expect, it } from "vitest";
44
import {
55
createCompletedWaitpointResolver,
66
UnresolvableWaitpointId,
7+
type ResolveArgs,
78
} from "./completedWaitpointResolver.js";
89

910
function record(overrides: Partial<CompletedWaitpointRecord> = {}): CompletedWaitpointRecord {
@@ -21,8 +22,17 @@ function record(overrides: Partial<CompletedWaitpointRecord> = {}): CompletedWai
2122

2223
const noRunOutput = { readRunOutput: async () => undefined };
2324

25+
type CaseArgs = Omit<ResolveArgs, "distinctIds"> & { distinctIds?: string[] };
26+
27+
/**
28+
* Fills `distinctIds` from the records when a case does not name it, because most cases are
29+
* about the expansion rather than the membership. The coverage-check cases set it explicitly,
30+
* since there it IS the subject.
31+
*/
2432
function resolver(readRunOutput?: (taskRunId: string) => Promise<string | undefined>) {
25-
return createCompletedWaitpointResolver(readRunOutput ? { readRunOutput } : noRunOutput);
33+
const resolve = createCompletedWaitpointResolver(readRunOutput ? { readRunOutput } : noRunOutput);
34+
return (over: CaseArgs) =>
35+
resolve({ ...over, distinctIds: over.distinctIds ?? over.records.map((r) => r.id) });
2636
}
2737

2838
const CYCLE = { cycleSeq: 1, count: 0 };
@@ -215,17 +225,21 @@ describe("the output hydration", () => {
215225
expect(entry?.output).toBe('{"derived":true}');
216226
});
217227

218-
it("leaves the output undefined when the run row is gone", async () => {
219-
const [entry] = await resolver()({
228+
// Postgres does not lose this: the back-reference nulls on delete but Waitpoint.output stays,
229+
// so the legacy path still emits it. Resolving to undefined would resolve the parent's
230+
// triggerAndWait successfully with no output, which is silent wrong data.
231+
it("refuses when the run row it defers to is gone", async () => {
232+
const failure = await resolver()({
220233
runId: "run_1",
221234
pointer: CYCLE,
222235
order: [],
223236
records: [
224237
record({ type: "RUN", completedByTaskRunId: "run_gone", output: { deriveFromRun: true } }),
225238
],
226-
});
239+
}).catch((caught: unknown) => caught as UnresolvableWaitpointId);
227240

228-
expect(entry?.output).toBeUndefined();
241+
expect(failure).toBeInstanceOf(UnresolvableWaitpointId);
242+
expect(failure.reason).toBe("lost-run-output");
229243
});
230244

231245
it("reads the run once for a record that expands to several entries", async () => {
@@ -324,6 +338,36 @@ describe("the coverage check", () => {
324338
expect(result[0]?.index).toBe(1);
325339
});
326340

341+
// The check must run over the whole membership. An index-less wait is absent from `order` by
342+
// construction, so an order-scoped check returns [] here and the run resumes having silently
343+
// lost its result.
344+
it("throws when an index-less id in the membership has no record", async () => {
345+
const failure = await resolver()({
346+
runId: "run_1",
347+
pointer: CYCLE,
348+
order: [],
349+
distinctIds: ["wp_indexless"],
350+
records: [],
351+
}).catch((caught: unknown) => caught as UnresolvableWaitpointId);
352+
353+
expect(failure).toBeInstanceOf(UnresolvableWaitpointId);
354+
expect(failure.waitpointId).toBe("wp_indexless");
355+
expect(failure.reason).toBe("no-source");
356+
});
357+
358+
it("accepts an index-less id the caller resolved from a row", async () => {
359+
const result = await resolver()({
360+
runId: "run_1",
361+
pointer: CYCLE,
362+
order: [],
363+
distinctIds: ["wp_legacy"],
364+
records: [],
365+
resolvedElsewhere: ["wp_legacy"],
366+
});
367+
368+
expect(result).toEqual([]);
369+
});
370+
327371
it("resolves an empty cycle to nothing", async () => {
328372
await expect(
329373
resolver()({ runId: "run_1", pointer: CYCLE, order: [], records: [] })

internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,23 @@ import type { CompletedWaitpoint } from "@trigger.dev/core/v3/schemas";
99
* classifies as legacy, finds no row, and would otherwise disappear from the resumed run's
1010
* completed set with no error at all.
1111
*/
12+
export type UnresolvableReason = "no-source" | "two-sources" | "lost-run-output";
13+
14+
const MESSAGES: Record<UnresolvableReason, (id: string) => string> = {
15+
"no-source": (id) =>
16+
`Waitpoint ${id} has neither a cycle record nor a fetched row. Refusing to resume without it.`,
17+
"two-sources": (id) =>
18+
`Waitpoint ${id} resolved twice, from a cycle record and from a fetched row.`,
19+
"lost-run-output": (id) =>
20+
`Waitpoint ${id} defers its output to its completing run, and that run's output is gone. Refusing to resume with an empty output.`,
21+
};
22+
1223
export class UnresolvableWaitpointId extends Error {
1324
readonly waitpointId: string;
14-
readonly reason: "no-source" | "two-sources";
15-
16-
constructor(waitpointId: string, reason: "no-source" | "two-sources") {
17-
super(
18-
reason === "no-source"
19-
? `Waitpoint ${waitpointId} has neither a cycle record nor a fetched row. Refusing to resume without it.`
20-
: `Waitpoint ${waitpointId} resolved twice, from a cycle record and from a fetched row.`
21-
);
25+
readonly reason: UnresolvableReason;
26+
27+
constructor(waitpointId: string, reason: UnresolvableReason) {
28+
super(MESSAGES[reason](waitpointId));
2229
this.name = "UnresolvableWaitpointId";
2330
this.waitpointId = waitpointId;
2431
this.reason = reason;
@@ -59,7 +66,10 @@ export function createCompletedWaitpointResolver(deps: CompletedWaitpointResolve
5966
}
6067
}
6168

62-
for (const id of args.order) {
69+
// Over the WHOLE membership, not `order`. The order omits every index-less wait, so a
70+
// check scoped to it cannot see an index-less id whose record is missing — which is the
71+
// exact loss this resolver exists to make impossible.
72+
for (const id of new Set([...args.distinctIds, ...args.order])) {
6373
if (!recordIds.has(id) && !resolvedElsewhere.has(id)) {
6474
throw new UnresolvableWaitpointId(id, "no-source");
6575
}
@@ -145,5 +155,13 @@ async function hydrateOutput(
145155
return undefined;
146156
}
147157

148-
return deps.readRunOutput(record.completedByTaskRunId);
158+
// Postgres does not lose this: the back-reference nulls on delete but Waitpoint.output stays,
159+
// so the legacy path still emits it. Returning undefined here instead would resolve the
160+
// parent's triggerAndWait successfully with no output, which is silent wrong data.
161+
const output = await deps.readRunOutput(record.completedByTaskRunId);
162+
if (output === undefined) {
163+
throw new UnresolvableWaitpointId(record.id, "lost-run-output");
164+
}
165+
166+
return output;
149167
}

0 commit comments

Comments
 (0)