Skip to content

Commit 45ee043

Browse files
committed
test(run-engine): bind each waitpoint mint site to its anchor, and make the census site-granular
An adversarial review found the census guard was file-granular where the requirement is site-granular, and that no test bound a create site to its anchor. Both were real: a fifth mint added inside an already-catalogued file passed, and swapping any site's anchor for undefined passed every test on the branch while silently reverting that site to a cuid. The catalog now records the exact mint expression per site, and the proof test counts each one per file. It walks the whole engine tree rather than the coordinator directory alone, so a mint moved back into systems/ -- where they all lived before the coordinator seam -- is visible. Test-support trees are excluded explicitly, since a helper writing through raw Prisma never reaches the routing store. Both holes were confirmed closed by reintroducing them and watching the guard fail. The site tests now drive the real create sites through a capturing run store rather than calling the mint helper with a hand-written literal, including the standalone-token arms and the precedence of an owning run over the environment shard. Also: deletes a test that duplicated another file while claiming to guard the failed-run path it never imported; adds the missing gen-2 batch-anchor case for batch items; corrects the standaloneShardKey contract text, which stated a rule its only caller does not follow; and corrects the BATCH comment, which claimed stamping from the run "would throw" when on the normal path both stamps agree and it would not.
1 parent 13f6124 commit 45ee043

8 files changed

Lines changed: 265 additions & 130 deletions

File tree

apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,16 @@ describe("mintAnchoredRunFriendlyId", () => {
2828
expect(parsed.format).toBe("b32hex");
2929
expect(parsed.format === "b32hex" && parsed.region).toBe(REGION_CODES["us-east-1"]);
3030
});
31+
32+
it("a gen-2 batch anchor mints an item on the batch's shard", () => {
33+
const body = mintAnchoredRunFriendlyId(`batch_${"a".repeat(24)}a2`).slice("run_".length);
34+
expect(body).toHaveLength(26);
35+
expect(body[24]).toBe("a");
36+
expect(body[25]).toBe("2");
37+
});
38+
39+
it("a gen-2 batch anchor ignores a caller region: the shard owns index 24", () => {
40+
const body = mintAnchoredRunFriendlyId(`batch_${"a".repeat(24)}a2`, "us-east-1").slice(4);
41+
expect(body[24]).toBe("a");
42+
});
3143
});

apps/webapp/test/engine/gen2ChildMintInheritance.test.ts

Lines changed: 0 additions & 16 deletions
This file was deleted.

internal-packages/run-engine/src/engine/index.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1824,10 +1824,10 @@ export class RunEngine {
18241824
standaloneResidency?: "NEW" | "LEGACY";
18251825
/**
18261826
* The environment's mint shard, for a STANDALONE token with no owning run. It selects the
1827-
* shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also
1828-
* set `standaloneResidency`: a residency hint outranks the id shape in the router and can
1829-
* only name a gen-1 store, so the row would land there while its completion routes to the
1830-
* shard. Only a Postgres implementation reads this.
1827+
* shard the token's id is stamped for. When it names a gen-2 shard the implementation must
1828+
* IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and
1829+
* can only name a gen-1 store, so honouring it would land the row there while its
1830+
* completion routes to the shard. Only a Postgres implementation reads this.
18311831
*/
18321832
standaloneShardKey?: ShardKey;
18331833
}): Promise<{ waitpoint: Waitpoint; isCached: boolean }> {
@@ -1868,8 +1868,10 @@ export class RunEngine {
18681868
data: {
18691869
// Stamped from the BATCH, not the blocked run: this create passes only
18701870
// completedByBatchId, so the routing store resolves the owner from the batch and
1871-
// validates the stamp against the batch's shard. The two match, because the batch
1872-
// inherited this run's shard when it was minted.
1871+
// validates the stamp against the BATCH's shard. On the normal path the two are
1872+
// the same char anyway, because the batch inherited this run's shard. They differ
1873+
// only if a batch ever blocks a run from another shard -- and then this is the
1874+
// stamp that matches the owner the router actually checks.
18731875
...mintWaitpointIdFor(batchId),
18741876
type: "BATCH",
18751877
idempotencyKey: batchId,

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200,10 +200,10 @@ export class WaitpointSystem {
200200
standaloneResidency?: "NEW" | "LEGACY";
201201
/**
202202
* The environment's mint shard, for a STANDALONE token with no owning run. It selects the
203-
* shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also
204-
* set `standaloneResidency`: a residency hint outranks the id shape in the router and can
205-
* only name a gen-1 store, so the row would land there while its completion routes to the
206-
* shard. Only a Postgres implementation reads this.
203+
* shard the token's id is stamped for. When it names a gen-2 shard the implementation must
204+
* IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and
205+
* can only name a gen-1 store, so honouring it would land the row there while its
206+
* completion routes to the shard. Only a Postgres implementation reads this.
207207
*/
208208
standaloneShardKey?: ShardKey;
209209
}): Promise<{ waitpoint: Waitpoint; isCached: boolean }> {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,10 @@ export type CreateManualWaitpointParams = {
138138
standaloneResidency?: "NEW" | "LEGACY";
139139
/**
140140
* The environment's mint shard, for a STANDALONE token with no owning run. It selects the
141-
* shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also
142-
* set `standaloneResidency`: a residency hint outranks the id shape in the router and can
143-
* only name a gen-1 store, so the row would land there while its completion routes to the
144-
* shard. Only a Postgres implementation reads this.
141+
* shard the token's id is stamped for. When it names a gen-2 shard the implementation must
142+
* IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and
143+
* can only name a gen-1 store, so honouring it would land the row there while its
144+
* completion routes to the shard. Only a Postgres implementation reads this.
145145
*/
146146
standaloneShardKey?: ShardKey;
147147
};

internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts

Lines changed: 89 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
1-
import { existsSync, readdirSync, readFileSync } from "node:fs";
1+
import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
22
import path from "node:path";
33
import { describe, expect, it } from "vitest";
4-
import { mintWaitpointIdFor } from "@trigger.dev/core/v3/isomorphic";
54
import { WAITPOINT_MINT_SITES } from "./waitpointMintCatalog";
65

7-
const GEN2_ANCHOR = `${"a".repeat(24)}a2`;
8-
const GEN1_ANCHOR = `${"a".repeat(24)}01`;
9-
106
function repoRoot(): string {
117
let dir = process.cwd();
128
while (!existsSync(path.join(dir, "pnpm-workspace.yaml"))) {
@@ -25,63 +21,92 @@ function count(source: string, pattern: RegExp): number {
2521
return (source.match(pattern) ?? []).length;
2622
}
2723

28-
// Every source file that may create a Postgres waitpoint row. The coordinator directory is
29-
// WALKED rather than listed, so a mint added in a new coordinator file cannot hide here.
30-
function scannedFiles(): string[] {
31-
const coordinatorDir = "internal-packages/run-engine/src/engine/waitpointCoordinator";
32-
const walked = readdirSync(path.join(repoRoot(), coordinatorDir))
33-
.filter((name) => name.endsWith(".ts") && !name.includes(".test."))
34-
.map((name) => `${coordinatorDir}/${name}`);
35-
36-
return [
37-
...walked,
38-
"internal-packages/run-engine/src/engine/index.ts",
39-
"internal-packages/run-store/src/PostgresRunStore.ts",
40-
];
24+
// Every production `.ts` under a root, walked rather than listed: a mint added in a new
25+
// file, or moved back into `systems/` where these all lived until the coordinator seam was
26+
// extracted, has to be visible here or the census is decorative.
27+
//
28+
// Test-support trees are excluded deliberately. A helper that writes a row through raw
29+
// Prisma never reaches the routing store, so it cannot misroute; requiring it to be
30+
// catalogued would fill the census with sites that carry no risk.
31+
const TEST_SUPPORT_DIRS = new Set(["tests", "__tests__", "fixtures"]);
32+
33+
function walk(relativeRoot: string): string[] {
34+
const absolute = path.join(repoRoot(), relativeRoot);
35+
return readdirSync(absolute).flatMap((name) => {
36+
const child = `${relativeRoot}/${name}`;
37+
if (statSync(path.join(absolute, name)).isDirectory()) {
38+
return TEST_SUPPORT_DIRS.has(name) ? [] : walk(child);
39+
}
40+
return name.endsWith(".ts") && !name.includes(".test.") ? [child] : [];
41+
});
4142
}
4243

43-
describe("waitpoint mint census — behaviour per catalogued site", () => {
44-
for (const site of WAITPOINT_MINT_SITES) {
45-
it(`${site.id} (${site.type}) stamps a gen-2 anchor's shard char`, () => {
46-
const r = mintWaitpointIdFor(GEN2_ANCHOR);
47-
expect(r.id[24]).toBe("a");
48-
expect(r.id[25]).toBe("2");
49-
});
50-
51-
it(`${site.id} (${site.type}) keeps a cuid for a gen-1 anchor`, () => {
52-
expect(mintWaitpointIdFor(GEN1_ANCHOR).id.length).toBe(25);
53-
});
44+
// The mint helpers are the only sanctioned way to produce a Postgres waitpoint id.
45+
const MINT_CALL = /mintWaitpointIdFor(?:Shard)?\(/g;
46+
const UNSTAMPED_MINT = /WaitpointId\.generate\(/g;
47+
const WAITPOINT_WRITE = /waitpoint\.create\(|upsertWaitpoint\(|createWaitpoint\(/g;
48+
49+
// The catalog holds the mint expressions as string data, so scanning it would count them.
50+
const CATALOG_ITSELF =
51+
"internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts";
52+
53+
const ENGINE_SOURCES = walk("internal-packages/run-engine/src/engine").filter(
54+
(f) => f !== CATALOG_ITSELF
55+
);
56+
const SCANNED = [...ENGINE_SOURCES, "internal-packages/run-store/src/PostgresRunStore.ts"];
57+
58+
// expression -> how many times the catalog says it appears in this file
59+
function expectedMints(file: string): Map<string, number> {
60+
const expected = new Map<string, number>();
61+
for (const site of WAITPOINT_MINT_SITES.filter((s) => s.site === file)) {
62+
for (const expr of site.mints) {
63+
expected.set(expr, (expected.get(expr) ?? 0) + 1);
64+
}
5465
}
55-
});
66+
return expected;
67+
}
5668

57-
describe("waitpoint mint census — source drift guard", () => {
58-
it("no scanned source mints a waitpoint id with the un-stamped helper", () => {
59-
// The regex matches tokens inside comments too — deliberate. Any textual addition
60-
// forces the census to be reconciled, so a new site cannot land without an entry.
61-
for (const file of scannedFiles()) {
62-
expect({ file, hits: count(read(file), /WaitpointId\.generate\(/g) }).toEqual({
63-
file,
64-
hits: 0,
69+
function escapeRegExp(value: string): string {
70+
return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
71+
}
72+
73+
describe("waitpoint mint census — the catalog matches the source", () => {
74+
it("scans the engine tree and the run-store writer, and finds files to scan", () => {
75+
expect(ENGINE_SOURCES.length).toBeGreaterThan(10);
76+
expect(SCANNED).toContain("internal-packages/run-engine/src/engine/systems/waitpointSystem.ts");
77+
});
78+
79+
// Per EXPRESSION, not per file: this fails for a fifth mint added inside an
80+
// already-catalogued file, AND for a swapped anchor — mintWaitpointIdFor(undefined) in
81+
// place of the run id — which a bare call-count would wave through.
82+
it.each(SCANNED)("%s has exactly the mint expressions the catalog claims", (file) => {
83+
const source = read(file);
84+
const expected = expectedMints(file);
85+
86+
for (const [expr, n] of expected) {
87+
expect({ expr, found: count(source, new RegExp(escapeRegExp(expr), "g")) }).toEqual({
88+
expr,
89+
found: n,
6590
});
6691
}
92+
93+
// No mint in the file beyond the ones the catalog accounts for.
94+
const accounted = [...expected.values()].reduce((a, b) => a + b, 0);
95+
expect(count(source, MINT_CALL)).toBe(accounted);
6796
});
6897

69-
it("every file that writes a waitpoint row is catalogued", () => {
70-
const catalogued = new Set(WAITPOINT_MINT_SITES.map((s) => s.site));
71-
72-
for (const file of scannedFiles()) {
73-
const source = read(file);
74-
// A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid
75-
// on a gen-2 shard after the write, which no stamp check can see.
76-
const writes =
77-
count(source, /waitpoint\.create\(/g) +
78-
count(source, /upsertWaitpoint\(/g) +
79-
count(source, /createWaitpoint\(/g);
80-
81-
if (writes > 0) {
82-
expect({ file, catalogued: catalogued.has(file) }).toEqual({ file, catalogued: true });
83-
}
84-
}
98+
it.each(SCANNED)("%s mints no waitpoint id with the un-stamped helper", (file) => {
99+
// The regex matches tokens inside comments too — deliberate. Any textual addition
100+
// forces the census to be reconciled, so a new site cannot land unnoticed.
101+
expect(count(read(file), UNSTAMPED_MINT)).toBe(0);
102+
});
103+
104+
it.each(SCANNED)("%s writes a waitpoint row only if it is catalogued", (file) => {
105+
// A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid on
106+
// a gen-2 shard after the write, which no stamp check can see.
107+
const writes = count(read(file), WAITPOINT_WRITE);
108+
const catalogued = WAITPOINT_MINT_SITES.some((s) => s.site === file);
109+
expect(writes === 0 || catalogued).toBe(true);
85110
});
86111

87112
it("every catalogued site names a file that exists", () => {
@@ -93,6 +118,16 @@ describe("waitpoint mint census — source drift guard", () => {
93118
}
94119
});
95120

121+
it("every catalogued site names its enclosing symbol in that file", () => {
122+
for (const site of WAITPOINT_MINT_SITES) {
123+
const symbol = site.symbol.split(" ")[0]!.replace("#", "");
124+
expect({ site: site.id, present: read(site.site).includes(symbol) }).toEqual({
125+
site: site.id,
126+
present: true,
127+
});
128+
}
129+
});
130+
96131
it("no catalogued symbol is a line number", () => {
97132
for (const site of WAITPOINT_MINT_SITES) {
98133
expect(site.symbol).not.toMatch(/:\d+/);

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

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,13 @@ export type WaitpointMintSite = {
1616
site: string;
1717
/** Enclosing method or symbol name — NEVER a line number. */
1818
symbol: string;
19+
/**
20+
* The exact mint expressions this site contains, verbatim. The proof test counts each one
21+
* per file, so both a new mint inside an already-catalogued file and a swapped anchor
22+
* (`mintWaitpointIdFor(undefined)` in place of the run id) fail until reconciled here.
23+
* Empty for a site that writes a row from an id minted elsewhere.
24+
*/
25+
mints: readonly string[];
1926
};
2027

2128
const COORDINATOR =
@@ -26,35 +33,52 @@ const RUN_STORE = "internal-packages/run-store/src/PostgresRunStore.ts";
2633
export const WAITPOINT_MINT_SITES: readonly WaitpointMintSite[] = [
2734
{
2835
id: "coordinator.datetime",
36+
mints: ["mintWaitpointIdFor(runId)"],
2937
type: "DATETIME",
3038
site: COORDINATOR,
3139
symbol: "createDateTimeWaitpoint",
3240
},
33-
{ id: "coordinator.manual", type: "MANUAL", site: COORDINATOR, symbol: "createManualWaitpoint" },
41+
{
42+
id: "coordinator.manual",
43+
mints: ["mintWaitpointIdForShard(standaloneShard)", "mintWaitpointIdFor(runId)"],
44+
type: "MANUAL",
45+
site: COORDINATOR,
46+
symbol: "createManualWaitpoint",
47+
},
3448
{
3549
id: "coordinator.associated.mint",
50+
mints: ["mintWaitpointIdFor(anchorRunId)"],
3651
type: "RUN",
3752
site: COORDINATOR,
3853
symbol: "mintAssociatedWaitpointData",
3954
},
4055
{
4156
id: "coordinator.associated.create",
57+
mints: [],
4258
type: "RUN",
4359
site: COORDINATOR,
4460
symbol: "createAssociatedWaitpoint",
4561
},
46-
{ id: "engine.batch", type: "BATCH", site: ENGINE, symbol: "blockRunWithCreatedBatch" },
62+
{
63+
id: "engine.batch",
64+
mints: ["mintWaitpointIdFor(batchId)"],
65+
type: "BATCH",
66+
site: ENGINE,
67+
symbol: "blockRunWithCreatedBatch",
68+
},
4769
// The physical writers of the RUN row. They take an already-minted id rather than
4870
// minting one, but they are the writes that bypass the routing store's stamp check, so a
4971
// new writer here must be seen.
5072
{
5173
id: "runStore.createRun.nested",
74+
mints: [],
5275
type: "RUN",
5376
site: RUN_STORE,
5477
symbol: "createRun (nested associatedWaitpoint create)",
5578
},
5679
{
5780
id: "runStore.createRun.dedicated",
81+
mints: [],
5882
type: "RUN",
5983
site: RUN_STORE,
6084
symbol: "#createAssociatedWaitpoint",

0 commit comments

Comments
 (0)