From 65a354dedb3146a8055f34d3c15b64a0fbddfeca Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 12:49:14 +0100 Subject: [PATCH 01/26] feat(core): mint Postgres waitpoint ids stamped for a gen-2 shard Adds mintWaitpointIdForShard(key) and mintWaitpointIdFor(anchorId). A gen-2 shard key produces a 26-char body carrying that shard char at index 24 and version "2"; a reserved key, or no anchor, keeps today's cuid. The core is always freshly minted rather than derived from the anchor: a derived body would share the anchor's core, shard char and version char, so it would be byte-identical to the run's own id. Both the webapp and the run engine mint through this one function. They have to agree byte-for-byte, because the routing store refuses a waitpoint whose id is not stamped for the shard it is being written to. Kept separate from friendlyId.ts because it needs resolveShard, and runOpsResidency.ts already imports friendlyId.ts. --- packages/core/src/v3/isomorphic/index.ts | 1 + .../src/v3/isomorphic/waitpointMint.test.ts | 70 +++++++++++++++++++ .../core/src/v3/isomorphic/waitpointMint.ts | 26 +++++++ 3 files changed, 97 insertions(+) create mode 100644 packages/core/src/v3/isomorphic/waitpointMint.test.ts create mode 100644 packages/core/src/v3/isomorphic/waitpointMint.ts diff --git a/packages/core/src/v3/isomorphic/index.ts b/packages/core/src/v3/isomorphic/index.ts index 3f372854735..5207dbc2c65 100644 --- a/packages/core/src/v3/isomorphic/index.ts +++ b/packages/core/src/v3/isomorphic/index.ts @@ -1,5 +1,6 @@ export * from "./friendlyId.js"; export * from "./runOpsResidency.js"; +export * from "./waitpointMint.js"; export * from "./duration.js"; export * from "./maxDuration.js"; export * from "./queueName.js"; diff --git a/packages/core/src/v3/isomorphic/waitpointMint.test.ts b/packages/core/src/v3/isomorphic/waitpointMint.test.ts new file mode 100644 index 00000000000..24acf6c7588 --- /dev/null +++ b/packages/core/src/v3/isomorphic/waitpointMint.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from "vitest"; +import { mintWaitpointIdFor, mintWaitpointIdForShard } from "./waitpointMint.js"; +import { isValidShardChar, parseRunOpsIdV2Body } from "./friendlyId.js"; +import { resolveShard } from "./runOpsResidency.js"; + +const GEN2_RUN = `run_${"a".repeat(24)}a2`; // shard "a", version "2" +const GEN1_RUN = `run_${"a".repeat(24)}01`; // region "0", version "1" +const CUID_RUN = `run_${"b".repeat(25)}`; + +describe("mintWaitpointIdForShard", () => { + it("a gen-2 shard key mints a gen-2 body with that char at index 24", () => { + const r = mintWaitpointIdForShard("a"); + expect(r.id.length).toBe(26); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + expect(r.friendlyId).toBe(`waitpoint_${r.id}`); + expect(parseRunOpsIdV2Body(r.id)?.shard).toBe("a"); + }); + + it("the reserved key 'new' mints a cuid, unchanged from today", () => { + const r = mintWaitpointIdForShard("new"); + expect(r.id.length).toBe(25); + expect(resolveShard(r.id)).toBe("legacy"); + }); + + it("the reserved key 'legacy' mints a cuid", () => { + expect(mintWaitpointIdForShard("legacy").id.length).toBe(25); + }); + + it("two calls for one shard never collide", () => { + expect(mintWaitpointIdForShard("a").id).not.toBe(mintWaitpointIdForShard("a").id); + }); + + it("every gen-2 id it mints routes back to its own shard", () => { + for (const key of ["a", "b", "0", "z", "9"]) { + expect(isValidShardChar(key)).toBe(true); + expect(resolveShard(mintWaitpointIdForShard(key).id)).toBe(key); + } + }); +}); + +describe("mintWaitpointIdFor", () => { + it("a gen-2 anchor stamps the anchor's shard char", () => { + const r = mintWaitpointIdFor(GEN2_RUN); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + }); + + it("a gen-2 anchor yields a FRESH core, never the anchor's own body", () => { + const r = mintWaitpointIdFor(GEN2_RUN); + expect(r.id).not.toBe(GEN2_RUN.slice(4)); + expect(r.id.slice(0, 24)).not.toBe("a".repeat(24)); + }); + + it("accepts the bare internal form as well as the prefixed form", () => { + expect(mintWaitpointIdFor(GEN2_RUN.slice(4)).id[24]).toBe("a"); + }); + + it("a gen-1 v1 anchor mints a cuid", () => { + expect(mintWaitpointIdFor(GEN1_RUN).id.length).toBe(25); + }); + + it("a cuid anchor mints a cuid", () => { + expect(mintWaitpointIdFor(CUID_RUN).id.length).toBe(25); + }); + + it("no anchor mints a cuid", () => { + expect(mintWaitpointIdFor(undefined).id.length).toBe(25); + }); +}); diff --git a/packages/core/src/v3/isomorphic/waitpointMint.ts b/packages/core/src/v3/isomorphic/waitpointMint.ts new file mode 100644 index 00000000000..d05d7b60f43 --- /dev/null +++ b/packages/core/src/v3/isomorphic/waitpointMint.ts @@ -0,0 +1,26 @@ +import { generateRunOpsIdV2, WaitpointId } from "./friendlyId.js"; +import { resolveShard, type ShardKey } from "./runOpsResidency.js"; + +// A waitpoint id for a Postgres shard — NOT the Redis store format (type char at index +// 24, version "w"), which has no Postgres row to route. The core is always fresh: reusing +// the anchor's would produce a body identical to the run's own id. +export function mintWaitpointIdForShard(key: ShardKey): { id: string; friendlyId: string } { + if (key === "new" || key === "legacy") { + return WaitpointId.generate(); + } + + const id = generateRunOpsIdV2(key); + return { id, friendlyId: WaitpointId.toFriendlyId(id) }; +} + +// Every Postgres waitpoint mint goes through here, in the webapp and the engine alike: +// the router refuses a waitpoint whose id is not stamped for the shard it lands on. +// A gen-1 or legacy anchor keeps a cuid. +export function mintWaitpointIdFor(anchorId: string | undefined): { + id: string; + friendlyId: string; +} { + return anchorId === undefined + ? WaitpointId.generate() + : mintWaitpointIdForShard(resolveShard(anchorId)); +} From 0dae1c540d44e53e0d1fc68ce757fc6d44e2b3d3 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 12:50:24 +0100 Subject: [PATCH 02/26] feat(webapp): carry the shard char through a MintTarget on run inheritance resolveInheritedMintKind now returns { kind, shardChar? } instead of a bare kind, and mintFriendlyIdForKind takes that object. A gen-2 parent hands its own shard char to its children, so a run tree never splits across shards. The shard char and the region both occupy index 24 of a run-ops id, so they travel in one object rather than as two independent optional parameters: a caller cannot set two competing sources for one slot, and the gen-2 arm simply ignores the region. mintAnchoredRunFriendlyId keeps its signature, its keying on the batch id shape, and its synchronous form. Callers of the batch mint still break at this commit; the next two commits repair them. --- .../mintAnchoredRunFriendlyId.server.ts | 21 ++++--- .../app/v3/runOpsMigration/mintTarget.ts | 11 ++++ .../resolveInheritedMintKind.server.test.ts | 61 +++++++++++++++++-- .../resolveInheritedMintKind.server.ts | 14 +++-- 4 files changed, 92 insertions(+), 15 deletions(-) create mode 100644 apps/webapp/app/v3/runOpsMigration/mintTarget.ts diff --git a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts index 0f5da2e56f7..d3de7bf8cb4 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts @@ -1,15 +1,22 @@ -import { generateRunOpsId, RunId, type ResidencyKind } from "@trigger.dev/core/v3/isomorphic"; +import { generateRunOpsId, generateRunOpsIdV2, RunId } from "@trigger.dev/core/v3/isomorphic"; +import type { MintTarget } from "./mintTarget"; import { resolveInheritedMintKind } from "./resolveInheritedMintKind.server"; -// Shared id-generation branch for every run-mint path: "runOpsId" -> NEW store, "cuid" -> LEGACY. -export function mintFriendlyIdForKind(mintKind: ResidencyKind, region?: string): string { - return mintKind === "runOpsId" - ? RunId.toFriendlyId(generateRunOpsId(region)) - : RunId.generate().friendlyId; +// Shared id-generation branch for every run-mint path: "runOpsId" -> a dedicated store, +// "cuid" -> LEGACY. A shardChar selects one gen-2 shard and takes index 24; without one, +// the region takes that slot exactly as it does today. +export function mintFriendlyIdForKind(target: MintTarget): string { + if (target.kind !== "runOpsId") { + return RunId.generate().friendlyId; + } + + return RunId.toFriendlyId( + target.shardChar ? generateRunOpsIdV2(target.shardChar) : generateRunOpsId(target.region) + ); } // Anchor a batch item's mint on the BATCH's friendlyId (id-shape, zero I/O), never the per-org // flag, so the item and its BatchTaskRun stay co-resident across a mid-batch flag flip. export function mintAnchoredRunFriendlyId(batchFriendlyId: string, region?: string): string { - return mintFriendlyIdForKind(resolveInheritedMintKind(batchFriendlyId), region); + return mintFriendlyIdForKind({ ...resolveInheritedMintKind(batchFriendlyId), region }); } diff --git a/apps/webapp/app/v3/runOpsMigration/mintTarget.ts b/apps/webapp/app/v3/runOpsMigration/mintTarget.ts new file mode 100644 index 00000000000..94355d55462 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/mintTarget.ts @@ -0,0 +1,11 @@ +import type { ResidencyKind } from "@trigger.dev/core/v3/isomorphic"; + +/** + * Where one mint lands. `shardChar` and `region` both occupy index 24 of a run-ops id, so + * they travel together and cannot disagree. `shardChar` set means gen-2, region ignored. + */ +export type MintTarget = { + kind: ResidencyKind; + shardChar?: string; + region?: string; +}; diff --git a/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.test.ts b/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.test.ts index 3f135793f84..570cc496182 100644 --- a/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.test.ts @@ -1,15 +1,68 @@ import { describe, expect, it } from "vitest"; import { resolveInheritedMintKind } from "./resolveInheritedMintKind.server"; +import { mintFriendlyIdForKind } from "./mintAnchoredRunFriendlyId.server"; -const NEW_PARENT = `run_${"a".repeat(24) + "01"}`; // run-ops id-shape -> NEW +const NEW_PARENT = `run_${"a".repeat(24)}01`; // run-ops v1 id-shape -> NEW const LEGACY_PARENT = `run_${"b".repeat(25)}`; // cuid id-shape -> LEGACY +const GEN2_PARENT = `run_${"a".repeat(24)}a2`; // gen-2, shard "a" describe("resolveInheritedMintKind (pure id-shape, shared across all mint paths)", () => { - it("inherits a run-ops (NEW) parent by id-shape -> 'runOpsId' kind", () => { - expect(resolveInheritedMintKind(NEW_PARENT)).toBe("runOpsId"); + it("inherits a run-ops (NEW) parent by id-shape -> runOpsId with NO shard char", () => { + expect(resolveInheritedMintKind(NEW_PARENT)).toEqual({ kind: "runOpsId" }); }); it("inherits a cuid (LEGACY) parent by id-shape -> cuid", () => { - expect(resolveInheritedMintKind(LEGACY_PARENT)).toBe("cuid"); + expect(resolveInheritedMintKind(LEGACY_PARENT)).toEqual({ kind: "cuid" }); + }); + + it("inherits a gen-2 parent's shard char, never a freshly resolved one", () => { + expect(resolveInheritedMintKind(GEN2_PARENT)).toEqual({ kind: "runOpsId", shardChar: "a" }); + }); + + it("accepts the bare internal form", () => { + expect(resolveInheritedMintKind(GEN2_PARENT.slice(4))).toEqual({ + kind: "runOpsId", + shardChar: "a", + }); + }); +}); + +describe("mintFriendlyIdForKind", () => { + it("a shard char mints a gen-2 id with that char at index 24 and '2' at 25", () => { + const body = mintFriendlyIdForKind({ kind: "runOpsId", shardChar: "a" }).slice("run_".length); + expect(body.length).toBe(26); + expect(body[24]).toBe("a"); + expect(body[25]).toBe("2"); + }); + + it("a shard char wins over a region: index 24 has ONE source", () => { + const body = mintFriendlyIdForKind({ + kind: "runOpsId", + shardChar: "a", + region: "us-east-1", + }).slice("run_".length); + expect(body[24]).toBe("a"); // not "e", the us-east-1 region char + }); + + it("no shard char mints a gen-1 v1 id, stamping the region as today", () => { + const body = mintFriendlyIdForKind({ kind: "runOpsId", region: "us-east-1" }).slice(4); + expect(body[24]).toBe("e"); + expect(body[25]).toBe("1"); + }); + + it("no shard char and no region mints a gen-1 v1 id with the default region char", () => { + const body = mintFriendlyIdForKind({ kind: "runOpsId" }).slice(4); + expect(body[24]).toBe("0"); + expect(body[25]).toBe("1"); + }); + + it("cuid kind mints a 25-char cuid", () => { + expect(mintFriendlyIdForKind({ kind: "cuid" }).slice(4).length).toBe(25); + }); + + it("an end-to-end inherit-then-mint keeps a child on the parent's shard", () => { + const body = mintFriendlyIdForKind(resolveInheritedMintKind(GEN2_PARENT)).slice(4); + expect(body[24]).toBe("a"); + expect(body[25]).toBe("2"); }); }); diff --git a/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.ts b/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.ts index 6ec9583c94b..825d910d7d9 100644 --- a/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/resolveInheritedMintKind.server.ts @@ -1,10 +1,16 @@ -import { ownerEngine } from "@trigger.dev/core/v3/isomorphic"; -import type { RunIdMintKind } from "./runOpsMintKind.server"; +import { resolveShard } from "@trigger.dev/core/v3/isomorphic"; +import type { MintTarget } from "./mintTarget"; // Mint a child in the SAME physical store as its anchor (parent run / owning batch), // regardless of the org's current mint flag — keeps a subgraph co-resident across a // flip. With no migration/drain, residency is a pure id-shape check (zero hot-path // I/O): a run-ops (NEW) parent mints run-ops children, a cuid (LEGACY) parent mints cuid. -export function resolveInheritedMintKind(parentRunFriendlyId: string): RunIdMintKind { - return ownerEngine(parentRunFriendlyId) === "NEW" ? "runOpsId" : "cuid"; +// A gen-2 parent hands down its OWN shard char, never a freshly resolved one: two runs in +// one tree must never split across shards. +export function resolveInheritedMintKind(parentRunFriendlyId: string): MintTarget { + const shard = resolveShard(parentRunFriendlyId); + + if (shard === "legacy") return { kind: "cuid" }; + if (shard === "new") return { kind: "runOpsId" }; + return { kind: "runOpsId", shardChar: shard }; } From b20792f994139b11ea603bd40516d93819c8b2f2 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 12:51:58 +0100 Subject: [PATCH 03/26] feat(webapp): resolve a run's mint target in one place, gated off by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds resolveRunMintTarget: a parent means inherit by id-shape, no parent means resolve the org's mint kind and then, only on the run-ops path, the environment's mint shard. Three services carried this branch separately and one had already drifted, so it now lives in one function with an injectable deps parameter for tests. resolveMintShard gains an early return when no shard descriptor is configured. It matters for more than speed: the flag read happens before the routable-key bound is applied, so without this guard, merging would add a control-plane replica query to the root trigger path on every deployment that has no shards. With it, an unconfigured deployment takes a literally unchanged path — no query, no cache write, no log line. Both knip suppressions for that module are dropped now that it has real importers. --- .../resolveRunMintTarget.server.test.ts | 75 +++++++++++++++++++ .../resolveRunMintTarget.server.ts | 55 ++++++++++++++ .../runOpsMigration/runOpsMintShard.server.ts | 9 ++- knip.json | 3 +- 4 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.test.ts create mode 100644 apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts diff --git a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.test.ts b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.test.ts new file mode 100644 index 00000000000..a71fdcc2b5f --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it, vi } from "vitest"; +import { resolveRunMintTarget } from "./resolveRunMintTarget.server"; + +const environment = { organizationId: "org_1", id: "env_1", orgFeatureFlags: {} }; +const GEN2_PARENT = `run_${"a".repeat(24)}a2`; +const LEGACY_PARENT = `run_${"b".repeat(25)}`; + +describe("resolveRunMintTarget — root", () => { + it("resolves the kind, then the shard, and returns both", async () => { + const resolveRunIdMintKind = vi.fn().mockResolvedValue("runOpsId"); + const resolveMintShard = vi.fn().mockResolvedValue("a"); + + const target = await resolveRunMintTarget({ + environment, + deps: { resolveRunIdMintKind, resolveMintShard }, + }); + + expect(target).toEqual({ kind: "runOpsId", shardChar: "a", region: undefined }); + expect(resolveMintShard).toHaveBeenCalledWith({ id: "env_1", orgFeatureFlags: {} }); + }); + + it("a 'new' shard result carries NO shard char, so the mint stays gen-1", async () => { + const target = await resolveRunMintTarget({ + environment, + region: "us-east-1", + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: vi.fn().mockResolvedValue("new"), + }, + }); + expect(target).toEqual({ kind: "runOpsId", region: "us-east-1" }); + }); + + it("never resolves a shard when the kind is cuid", async () => { + const resolveMintShard = vi.fn(); + const target = await resolveRunMintTarget({ + environment, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("cuid"), + resolveMintShard, + }, + }); + expect(target).toEqual({ kind: "cuid" }); + expect(resolveMintShard).not.toHaveBeenCalled(); + }); +}); + +describe("resolveRunMintTarget — child", () => { + it("inherits a gen-2 parent's shard and consults NEITHER resolver", async () => { + const resolveRunIdMintKind = vi.fn(); + const resolveMintShard = vi.fn(); + + const target = await resolveRunMintTarget({ + environment, + parentRunFriendlyId: GEN2_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard }, + }); + + expect(target).toEqual({ kind: "runOpsId", shardChar: "a" }); + expect(resolveRunIdMintKind).not.toHaveBeenCalled(); + expect(resolveMintShard).not.toHaveBeenCalled(); + }); + + it("a cuid parent still yields cuid though the flag now says runOpsId", async () => { + const target = await resolveRunMintTarget({ + environment, + parentRunFriendlyId: LEGACY_PARENT, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: vi.fn().mockResolvedValue("a"), + }, + }); + expect(target).toEqual({ kind: "cuid" }); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts new file mode 100644 index 00000000000..89b8948c689 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts @@ -0,0 +1,55 @@ +import type { MintTarget } from "./mintTarget"; +import { resolveInheritedMintKind } from "./resolveInheritedMintKind.server"; +import { resolveRunIdMintKind as defaultResolveRunIdMintKind } from "./runOpsMintKind.server"; +import { resolveMintShard as defaultResolveMintShard } from "./runOpsMintShard.server"; + +export type RunMintDeps = { + resolveRunIdMintKind: typeof defaultResolveRunIdMintKind; + resolveMintShard: typeof defaultResolveMintShard; +}; + +const defaultDeps: RunMintDeps = { + resolveRunIdMintKind: defaultResolveRunIdMintKind, + resolveMintShard: defaultResolveMintShard, +}; + +/** + * Where one run mints. Two stages, and the second runs only for a ROOT run already on the + * run-ops path: a child inherits its parent's shard by id-shape, so a tree never splits. + * + * Every run-mint path routes through here. The branch used to be duplicated per service, + * and one copy had already drifted into minting gen-1 for a gen-2 parent. + */ +export async function resolveRunMintTarget(args: { + environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; + parentRunFriendlyId?: string; + region?: string; + deps?: Partial; +}): Promise { + if (args.parentRunFriendlyId) { + return resolveInheritedMintKind(args.parentRunFriendlyId); + } + + const deps = { ...defaultDeps, ...args.deps }; + + const kind = await deps.resolveRunIdMintKind({ + organizationId: args.environment.organizationId, + id: args.environment.id, + orgFeatureFlags: args.environment.orgFeatureFlags, + }); + + if (kind !== "runOpsId") { + return { kind }; + } + + const shard = await deps.resolveMintShard({ + id: args.environment.id, + orgFeatureFlags: args.environment.orgFeatureFlags, + }); + + // A reserved key means gen-1, which is the state of every deployment that has configured + // no shard. Only a single-char key names a gen-2 shard. + return shard === "new" || shard === "legacy" + ? { kind, region: args.region } + : { kind, shardChar: shard, region: args.region }; +} diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index c1c2b9ddd48..422af3712e9 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -69,14 +69,19 @@ function reportOverrideRejected(info: { override: string; activeSet: string[] }) /** * Which shard an environment mints new roots into. Call only after resolveRunIdMintKind has * returned "runOpsId". Returns "new" to mean a gen-1 run-ops id, which is today's behaviour. - * - * @knipignore the gen-2 write-path change is the first production caller; drop this tag there. */ export async function resolveMintShard(environment: { id: string; // Pass environment.organization.featureFlags from the trigger call site. orgFeatureFlags?: unknown; }): Promise { + // No shard descriptor means no shard can ever be minted into, so answer before reading + // anything: an unconfigured deployment keeps exactly today's code path, with no + // control-plane query on the trigger path, no cache write and no log line. + if (env.RUN_OPS_SHARDS.length === 0) { + return "new"; + } + return resolveMintShardWith(environment, { readFlags: readSetFlags, cache: liveCache, diff --git a/knip.json b/knip.json index c6e8aee8977..84456756ca1 100644 --- a/knip.json +++ b/knip.json @@ -25,8 +25,7 @@ "vite/node-globals-shim.js", "app/v3/otlpTransformWorker.ts" ], - "ignoreDependencies": ["@sentry/cli", "assert", "util"], - "ignore": ["app/v3/runOpsMigration/runOpsMintShard.server.ts"] + "ignoreDependencies": ["@sentry/cli", "assert", "util"] }, "internal-packages/dashboard-agent": { "entry": ["trigger.config.ts", "src/investigation-sweep.ts", "src/maintenance.ts"], From 845ab0651fdcc825aac56882c5b54547c057fae6 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 12:58:14 +0100 Subject: [PATCH 04/26] fix(webapp): mint a failed child run onto its parent's shard triggerFailedTask duplicated the mint branch inline rather than calling the shared helper, and it had drifted: it dropped the caller's region, and once gen-2 ids exist it would mint a gen-1 id for a child of a gen-2 parent. The router would then write that child to the gen-1 store while its parent lives on a shard, splitting one run tree across two databases. Both trigger services now call resolveRunMintTarget. triggerTask's behaviour is unchanged. The pre-minted runFriendlyId pass-through stays ahead of the resolver: batchTrigger and runEngineHandlers hand in an id already minted from the batch, and re-resolving it would move the item off its batch's shard. Added a container test for that, since no pure test can reach the guard and a typecheck will not notice if it moves below the resolver. --- .../services/triggerFailedTask.server.ts | 21 ++++---- .../runEngine/services/triggerTask.server.ts | 17 ++++--- .../engine/gen2ChildMintInheritance.test.ts | 16 ++++++ ...iggerFailedTask.withoutTraceEvents.test.ts | 49 +++++++++++++++++++ 4 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 apps/webapp/test/engine/gen2ChildMintInheritance.test.ts diff --git a/apps/webapp/app/runEngine/services/triggerFailedTask.server.ts b/apps/webapp/app/runEngine/services/triggerFailedTask.server.ts index f8ba67f3448..e1d8a0841aa 100644 --- a/apps/webapp/app/runEngine/services/triggerFailedTask.server.ts +++ b/apps/webapp/app/runEngine/services/triggerFailedTask.server.ts @@ -1,6 +1,6 @@ import type { RunEngine } from "@internal/run-engine"; import { TaskRunErrorCodes, type TaskRunError } from "@trigger.dev/core/v3"; -import { RunId, generateRunOpsId } from "@trigger.dev/core/v3/isomorphic"; +import { RunId } from "@trigger.dev/core/v3/isomorphic"; import type { PrismaClientOrTransaction, RuntimeEnvironmentType, @@ -8,8 +8,8 @@ import type { } from "@trigger.dev/database"; import type { AuthenticatedEnvironment } from "~/services/apiAuth.server"; import { logger } from "~/services/logger.server"; -import { resolveRunIdMintKind } from "~/v3/engineVersion.server"; -import { resolveInheritedMintKind } from "~/v3/runOpsMigration/resolveInheritedMintKind.server"; +import { mintFriendlyIdForKind } from "~/v3/runOpsMigration/mintAnchoredRunFriendlyId.server"; +import { resolveRunMintTarget } from "~/v3/runOpsMigration/resolveRunMintTarget.server"; import { getEventRepository } from "~/v3/eventRepository/index.server"; import { runStore as defaultRunStore } from "~/v3/runStore.server"; import type { RunStore } from "@internal/run-store"; @@ -103,17 +103,16 @@ export class TriggerFailedTaskService { return args.runFriendlyId; } - const mintKind = args.parentRunFriendlyId - ? resolveInheritedMintKind(args.parentRunFriendlyId) - : await resolveRunIdMintKind({ + return mintFriendlyIdForKind( + await resolveRunMintTarget({ + environment: { organizationId: args.organizationId, id: args.environmentId, orgFeatureFlags: args.orgFeatureFlags, - }); - - return mintKind === "runOpsId" - ? RunId.toFriendlyId(generateRunOpsId()) - : RunId.generate().friendlyId; + }, + parentRunFriendlyId: args.parentRunFriendlyId, + }) + ); } async call(request: TriggerFailedTaskRequest): Promise { diff --git a/apps/webapp/app/runEngine/services/triggerTask.server.ts b/apps/webapp/app/runEngine/services/triggerTask.server.ts index 8e9e99d7f09..d3320dbc219 100644 --- a/apps/webapp/app/runEngine/services/triggerTask.server.ts +++ b/apps/webapp/app/runEngine/services/triggerTask.server.ts @@ -28,9 +28,8 @@ import { parseDelay } from "~/utils/delays"; import { removeNullBytesFromKey } from "~/utils/nullBytes"; import { handleMetadataPacket } from "~/utils/packets"; import { startSpan } from "~/v3/tracing.server"; -import { resolveRunIdMintKind } from "~/v3/engineVersion.server"; -import { resolveInheritedMintKind } from "~/v3/runOpsMigration/resolveInheritedMintKind.server"; import { mintFriendlyIdForKind } from "~/v3/runOpsMigration/mintAnchoredRunFriendlyId.server"; +import { resolveRunMintTarget } from "~/v3/runOpsMigration/resolveRunMintTarget.server"; import type { TriggerTaskServiceOptions, TriggerTaskServiceResult, @@ -218,15 +217,17 @@ export class RunEngineTriggerTaskService { parentRunFriendlyId?: string, region?: string ): Promise { - const mintKind = parentRunFriendlyId - ? resolveInheritedMintKind(parentRunFriendlyId) - : await resolveRunIdMintKind({ + return mintFriendlyIdForKind( + await resolveRunMintTarget({ + environment: { organizationId: environment.organizationId, id: environment.id, orgFeatureFlags: environment.organization.featureFlags, - }); - - return mintFriendlyIdForKind(mintKind, region); + }, + parentRunFriendlyId, + region, + }) + ); } public async call({ diff --git a/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts b/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts new file mode 100644 index 00000000000..d687b75c942 --- /dev/null +++ b/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from "vitest"; +import { resolveInheritedMintKind } from "~/v3/runOpsMigration/resolveInheritedMintKind.server"; +import { mintFriendlyIdForKind } from "~/v3/runOpsMigration/mintAnchoredRunFriendlyId.server"; + +// The shape both trigger services must produce for a child of a gen-2 parent. Before this +// change triggerFailedTask duplicated the branch inline and minted gen-1, which put a child +// on a different database from its parent. +const GEN2_PARENT = `run_${"a".repeat(24)}a2`; + +describe("a failed child of a gen-2 parent", () => { + it("mints onto the parent's shard", () => { + const body = mintFriendlyIdForKind(resolveInheritedMintKind(GEN2_PARENT)).slice(4); + expect(body[24]).toBe("a"); + expect(body[25]).toBe("2"); + }); +}); diff --git a/apps/webapp/test/engine/triggerFailedTask.withoutTraceEvents.test.ts b/apps/webapp/test/engine/triggerFailedTask.withoutTraceEvents.test.ts index a0be900fb82..eac5d75ec8a 100644 --- a/apps/webapp/test/engine/triggerFailedTask.withoutTraceEvents.test.ts +++ b/apps/webapp/test/engine/triggerFailedTask.withoutTraceEvents.test.ts @@ -90,4 +90,53 @@ describe("TriggerFailedTaskService — failed run residency (callWithoutTraceEve await engine.quit(); } ); + + containerTest( + "a pre-minted runFriendlyId passes through untouched", + async ({ prisma, redisOptions }) => { + const engine = makeEngine(prisma, redisOptions); + const environment = await setupAuthenticatedEnvironment(prisma, "PRODUCTION"); + const taskIdentifier = "failed-residency-passthrough"; + await setupBackgroundWorker(engine, environment, taskIdentifier); + + const parentFriendlyId = RunId.toFriendlyId(generateRunOpsId()); + await engine.trigger( + { + friendlyId: parentFriendlyId, + environment, + taskIdentifier, + payload: "{}", + payloadType: "application/json", + traceId: "00000000000000000000000000000000", + spanId: "0000000000000000", + workerQueue: "main", + queue: `task/${taskIdentifier}`, + isTest: false, + tags: [], + } as any, + prisma + ); + + // A batch item arrives with its id already minted from the BATCH. Re-resolving it + // here would move the item off its batch's shard, so the pass-through has to win + // over the mint-target resolver. + const preMinted = RunId.toFriendlyId(generateRunOpsId()); + + const friendlyId = await makeService(prisma, engine).callWithoutTraceEvents({ + environmentId: environment.id, + environmentType: environment.type, + projectId: environment.projectId, + organizationId: environment.organizationId, + taskId: taskIdentifier, + payload: { test: "passthrough" }, + errorMessage: "boom", + parentRunId: parentFriendlyId, + runFriendlyId: preMinted, + }); + + expect(friendlyId).toBe(preMinted); + + await engine.quit(); + } + ); }); From bf320524f68a6694f9a9803efeed83b646394df8 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 12:59:33 +0100 Subject: [PATCH 05/26] feat(webapp): mint a batch id onto its parent run's shard batchIdForMintKind and resolveBatchMintKind now take and return the mint target, so a child batch carries its parent run's shard char and a root batch mints by the environment's policy. Batch-anchored item minting needs no change: it already keys on the shape of the batch id. This is where the type change actually bites. resolveBatchMintKind declared Promise, so the inheritance change makes it a compile error, and the obvious repair -- comparing kind.kind -- would compile while silently dropping the shard char. The rewritten tests cover both arms, including the two that pin the rule that the flag resolver is never consulted for a child. batchTriggerV3.mintChildFriendlyId keeps its own branch and its injected resolveMintKind. Its root arm is unreachable in production and that injection point is what lets a test drive it without a database. --- .../mintBatchFriendlyId.server.test.ts | 109 ++++++++++++------ .../mintBatchFriendlyId.server.ts | 50 ++++---- .../app/v3/services/batchTriggerV3.server.ts | 19 +-- 3 files changed, 109 insertions(+), 69 deletions(-) diff --git a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.test.ts b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.test.ts index 9973be57d1d..0e07a59d382 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.test.ts @@ -4,15 +4,23 @@ import { classifyKind } from "@trigger.dev/core/v3/isomorphic"; describe("batchIdForMintKind (pure)", () => { it("'runOpsId' kind -> 26-char classifiable NEW batch id (no 21-char ids)", () => { - const r = batchIdForMintKind("runOpsId"); + const r = batchIdForMintKind({ kind: "runOpsId" }); expect(r.friendlyId.startsWith("batch_")).toBe(true); expect(r.id.length).toBe(26); expect(classifyKind(r.id)).toBe("runOpsId"); expect(classifyKind(r.friendlyId)).toBe("runOpsId"); }); + it("a shard char mints a gen-2 batch id carrying that char", () => { + const r = batchIdForMintKind({ kind: "runOpsId", shardChar: "a" }); + expect(r.id.length).toBe(26); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + expect(classifyKind(r.id)).toBe("runOpsId"); + }); + it("cuid -> 25-char classifiable LEGACY batch id", () => { - const r = batchIdForMintKind("cuid"); + const r = batchIdForMintKind({ kind: "cuid" }); expect(r.id.length).toBe(25); expect(classifyKind(r.id)).toBe("cuid"); expect(classifyKind(r.friendlyId)).toBe("cuid"); @@ -20,21 +28,26 @@ describe("batchIdForMintKind (pure)", () => { it("never mints a 21-char id", () => { for (const kind of ["cuid", "runOpsId"] as const) { - expect([25, 26]).toContain(batchIdForMintKind(kind).id.length); + expect([25, 26]).toContain(batchIdForMintKind({ kind }).id.length); } }); }); describe("resolveBatchMintKind", () => { const environment = { organizationId: "org_1", id: "env_1", orgFeatureFlags: {} }; + const NEW_PARENT = `run_${"a".repeat(24)}01`; + const LEGACY_PARENT = `run_${"a".repeat(25)}`; + const GEN2_PARENT = `run_${"a".repeat(24)}a2`; it("ROOT batch (no parent) resolves per-org kind via resolveRunIdMintKind", async () => { const resolveRunIdMintKind = vi.fn().mockResolvedValue("runOpsId"); - const kind = await resolveBatchMintKind({ + const resolveMintShard = vi.fn().mockResolvedValue("new"); + const target = await resolveBatchMintKind({ environment, - deps: { resolveRunIdMintKind }, + deps: { resolveRunIdMintKind, resolveMintShard }, }); - expect(kind).toBe("runOpsId"); + expect(target.kind).toBe("runOpsId"); + expect(target.shardChar).toBeUndefined(); expect(resolveRunIdMintKind).toHaveBeenCalledWith({ organizationId: "org_1", id: "env_1", @@ -42,66 +55,96 @@ describe("resolveBatchMintKind", () => { }); }); + it("ROOT batch mints by the mint policy when a shard is active", async () => { + const target = await resolveBatchMintKind({ + environment, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: vi.fn().mockResolvedValue("a"), + }, + }); + expect(target).toEqual({ kind: "runOpsId", shardChar: "a", region: undefined }); + }); + it("ROOT batch on a non-cut-over org -> cuid", async () => { - const resolveRunIdMintKind = vi.fn().mockResolvedValue("cuid"); - const kind = await resolveBatchMintKind({ + const target = await resolveBatchMintKind({ environment, - deps: { resolveRunIdMintKind }, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("cuid"), + resolveMintShard: vi.fn(), + }, }); - expect(kind).toBe("cuid"); + expect(target.kind).toBe("cuid"); }); it("CHILD batch inherits a run-ops (NEW) parent by id-shape", async () => { - const parentRunFriendlyId = `run_${"a".repeat(24) + "01"}`; const resolveRunIdMintKind = vi.fn(); - - const kind = await resolveBatchMintKind({ + const target = await resolveBatchMintKind({ environment, - parentRunFriendlyId, - deps: { resolveRunIdMintKind }, + parentRunFriendlyId: NEW_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard: vi.fn() }, }); + expect(target).toEqual({ kind: "runOpsId" }); + expect(resolveRunIdMintKind).not.toHaveBeenCalled(); + }); - expect(kind).toBe("runOpsId"); + it("CHILD batch takes a gen-2 parent's shard char", async () => { + const resolveRunIdMintKind = vi.fn(); + const resolveMintShard = vi.fn(); + const target = await resolveBatchMintKind({ + environment, + parentRunFriendlyId: GEN2_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard }, + }); + expect(target).toEqual({ kind: "runOpsId", shardChar: "a" }); expect(resolveRunIdMintKind).not.toHaveBeenCalled(); + expect(resolveMintShard).not.toHaveBeenCalled(); }); it("CHILD batch inherits a cuid (LEGACY) parent by id-shape", async () => { - const parentRunFriendlyId = `run_${"a".repeat(25)}`; const resolveRunIdMintKind = vi.fn(); - - const kind = await resolveBatchMintKind({ + const target = await resolveBatchMintKind({ environment, - parentRunFriendlyId, - deps: { resolveRunIdMintKind }, + parentRunFriendlyId: LEGACY_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard: vi.fn() }, }); - - expect(kind).toBe("cuid"); + expect(target).toEqual({ kind: "cuid" }); expect(resolveRunIdMintKind).not.toHaveBeenCalled(); }); // mint-on-FLIP invariant: a child follows its parent's store even after the org flag // flips the other way. The flag resolver must NEVER be consulted for a child. it("FLIP 'cuid'->'runOpsId': a cuid (LEGACY) parent still mints a cuid child though the flag now says 'runOpsId'", async () => { - const parentRunFriendlyId = `run_${"a".repeat(25)}`; const resolveRunIdMintKind = vi.fn().mockResolvedValue("runOpsId"); // flag flipped to runOpsId - const kind = await resolveBatchMintKind({ + const target = await resolveBatchMintKind({ environment, - parentRunFriendlyId, - deps: { resolveRunIdMintKind }, + parentRunFriendlyId: LEGACY_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard: vi.fn() }, }); - expect(kind).toBe("cuid"); + expect(target).toEqual({ kind: "cuid" }); expect(resolveRunIdMintKind).not.toHaveBeenCalled(); }); it("FLIP 'runOpsId'->'cuid': a run-ops (NEW) parent still mints a run-ops child though the flag now says 'cuid'", async () => { - const parentRunFriendlyId = `run_${"a".repeat(24) + "01"}`; const resolveRunIdMintKind = vi.fn().mockResolvedValue("cuid"); // flag flipped back to cuid - const kind = await resolveBatchMintKind({ + const target = await resolveBatchMintKind({ environment, - parentRunFriendlyId, - deps: { resolveRunIdMintKind }, + parentRunFriendlyId: NEW_PARENT, + deps: { resolveRunIdMintKind, resolveMintShard: vi.fn() }, }); - expect(kind).toBe("runOpsId"); + expect(target).toEqual({ kind: "runOpsId" }); expect(resolveRunIdMintKind).not.toHaveBeenCalled(); }); + + it("FLIP does not move a gen-2 child off its parent's shard", async () => { + const target = await resolveBatchMintKind({ + environment, + parentRunFriendlyId: GEN2_PARENT, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("cuid"), + resolveMintShard: vi.fn().mockResolvedValue("b"), + }, + }); + expect(target).toEqual({ kind: "runOpsId", shardChar: "a" }); + }); }); diff --git a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts index e2d8511e3ff..b08d9b9b33f 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts @@ -1,45 +1,37 @@ -import { BatchId, generateRunOpsId } from "@trigger.dev/core/v3/isomorphic"; -import { - resolveRunIdMintKind as defaultResolveRunIdMintKind, - type RunIdMintKind, -} from "~/v3/engineVersion.server"; -import { resolveInheritedMintKind } from "~/v3/runOpsMigration/resolveInheritedMintKind.server"; +import { BatchId, generateRunOpsId, generateRunOpsIdV2 } from "@trigger.dev/core/v3/isomorphic"; +import type { MintTarget } from "./mintTarget"; +import { resolveRunMintTarget, type RunMintDeps } from "./resolveRunMintTarget.server"; -type ResolveDeps = { - resolveRunIdMintKind: typeof defaultResolveRunIdMintKind; -}; +export function batchIdForMintKind(target: MintTarget): { id: string; friendlyId: string } { + if (target.kind !== "runOpsId") { + return BatchId.generate(); + } -const defaultDeps: ResolveDeps = { - resolveRunIdMintKind: defaultResolveRunIdMintKind, -}; + const id = target.shardChar + ? generateRunOpsIdV2(target.shardChar) + : generateRunOpsId(target.region); -export function batchIdForMintKind(kind: RunIdMintKind): { id: string; friendlyId: string } { - if (kind === "runOpsId") { - const id = generateRunOpsId(); - return { id, friendlyId: BatchId.toFriendlyId(id) }; - } - return BatchId.generate(); + return { id, friendlyId: BatchId.toFriendlyId(id) }; } +// A batch anchors on the parent RUN's id, never on another batch, and every call site +// passes that id optionally — so one call serves a root batch and a child batch. export async function resolveBatchMintKind(args: { environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; parentRunFriendlyId?: string; - deps?: Partial; -}): Promise { - const deps = { ...defaultDeps, ...args.deps }; - return args.parentRunFriendlyId - ? resolveInheritedMintKind(args.parentRunFriendlyId) - : deps.resolveRunIdMintKind({ - organizationId: args.environment.organizationId, - id: args.environment.id, - orgFeatureFlags: args.environment.orgFeatureFlags, - }); + deps?: Partial; +}): Promise { + return resolveRunMintTarget({ + environment: args.environment, + parentRunFriendlyId: args.parentRunFriendlyId, + deps: args.deps, + }); } export async function mintBatchFriendlyId(args: { environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; parentRunFriendlyId?: string; - deps?: Partial; + deps?: Partial; }): Promise<{ id: string; friendlyId: string }> { return batchIdForMintKind(await resolveBatchMintKind(args)); } diff --git a/apps/webapp/app/v3/services/batchTriggerV3.server.ts b/apps/webapp/app/v3/services/batchTriggerV3.server.ts index 563ef446bcc..17a3bbb60d3 100644 --- a/apps/webapp/app/v3/services/batchTriggerV3.server.ts +++ b/apps/webapp/app/v3/services/batchTriggerV3.server.ts @@ -362,15 +362,20 @@ export class BatchTriggerV3Service extends BaseService { anchorFriendlyId?: string, region?: string ): Promise { - const mintKind = anchorFriendlyId + // Deliberately not routed through resolveRunMintTarget: the root arm below is + // unreachable in production (every call site passes an anchor), and resolveMintKind is + // injected so a test can drive that arm without a database. + const target = anchorFriendlyId ? resolveInheritedMintKind(anchorFriendlyId) - : await this.resolveMintKind({ - organizationId: environment.organizationId, - id: environment.id, - orgFeatureFlags: environment.organization.featureFlags, - }); + : { + kind: await this.resolveMintKind({ + organizationId: environment.organizationId, + id: environment.id, + orgFeatureFlags: environment.organization.featureFlags, + }), + }; - return mintFriendlyIdForKind(mintKind, region); + return mintFriendlyIdForKind({ ...target, region }); } async #prepareRunData( From 4359bf8aff0b661133ccbaa24d4d41ef13d81ed1 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:00:43 +0100 Subject: [PATCH 06/26] test(run-engine): add a failing census guard for waitpoint mint sites Enumerates every site that creates a Postgres waitpoint row, and asserts no scanned source still mints an id with the un-stamped helper. This commit is deliberately RED: five textual uses remain, so the drift assertion fails until the last mint site is converted. That is the point of landing it first -- the guard proves it can fail without anyone having to break a working site to demonstrate it. The four following commits each remove one or more of those uses. The guard walks the coordinator directory rather than a fixed file list, so a mint in a new coordinator file cannot hide from it, and it counts the waitpoint write calls too -- a site that omits the id entirely lets Prisma's cuid default fire after the write, which no stamp check can see. Scope includes the run store's two physical writers of the associated waitpoint row, read as text only. Those are the writes that bypass the routing store's stamp check, so they are exactly the ones a census must see. --- .../waitpointMint.proof.test.ts | 101 ++++++++++++++++++ .../waitpointMintCatalog.ts | 62 +++++++++++ 2 files changed, 163 insertions(+) create mode 100644 internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts create mode 100644 internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts new file mode 100644 index 00000000000..c31afd1be5e --- /dev/null +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts @@ -0,0 +1,101 @@ +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { mintWaitpointIdFor } from "@trigger.dev/core/v3/isomorphic"; +import { WAITPOINT_MINT_SITES } from "./waitpointMintCatalog"; + +const GEN2_ANCHOR = `${"a".repeat(24)}a2`; +const GEN1_ANCHOR = `${"a".repeat(24)}01`; + +function repoRoot(): string { + let dir = process.cwd(); + while (!existsSync(path.join(dir, "pnpm-workspace.yaml"))) { + const parent = path.dirname(dir); + if (parent === dir) throw new Error("repo root (pnpm-workspace.yaml) not found"); + dir = parent; + } + return dir; +} + +function read(relative: string): string { + return readFileSync(path.join(repoRoot(), relative), "utf8"); +} + +function count(source: string, pattern: RegExp): number { + return (source.match(pattern) ?? []).length; +} + +// Every source file that may create a Postgres waitpoint row. The coordinator directory is +// WALKED rather than listed, so a mint added in a new coordinator file cannot hide here. +function scannedFiles(): string[] { + const coordinatorDir = "internal-packages/run-engine/src/engine/waitpointCoordinator"; + const walked = readdirSync(path.join(repoRoot(), coordinatorDir)) + .filter((name) => name.endsWith(".ts") && !name.includes(".test.")) + .map((name) => `${coordinatorDir}/${name}`); + + return [ + ...walked, + "internal-packages/run-engine/src/engine/index.ts", + "internal-packages/run-store/src/PostgresRunStore.ts", + ]; +} + +describe("waitpoint mint census — behaviour per catalogued site", () => { + for (const site of WAITPOINT_MINT_SITES) { + it(`${site.id} (${site.type}) stamps a gen-2 anchor's shard char`, () => { + const r = mintWaitpointIdFor(GEN2_ANCHOR); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + }); + + it(`${site.id} (${site.type}) keeps a cuid for a gen-1 anchor`, () => { + expect(mintWaitpointIdFor(GEN1_ANCHOR).id.length).toBe(25); + }); + } +}); + +describe("waitpoint mint census — source drift guard", () => { + it("no scanned source mints a waitpoint id with the un-stamped helper", () => { + // The regex matches tokens inside comments too — deliberate. Any textual addition + // forces the census to be reconciled, so a new site cannot land without an entry. + for (const file of scannedFiles()) { + expect({ file, hits: count(read(file), /WaitpointId\.generate\(/g) }).toEqual({ + file, + hits: 0, + }); + } + }); + + it("every file that writes a waitpoint row is catalogued", () => { + const catalogued = new Set(WAITPOINT_MINT_SITES.map((s) => s.site)); + + for (const file of scannedFiles()) { + const source = read(file); + // A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid + // on a gen-2 shard after the write, which no stamp check can see. + const writes = + count(source, /waitpoint\.create\(/g) + + count(source, /upsertWaitpoint\(/g) + + count(source, /createWaitpoint\(/g); + + if (writes > 0) { + expect({ file, catalogued: catalogued.has(file) }).toEqual({ file, catalogued: true }); + } + } + }); + + it("every catalogued site names a file that exists", () => { + for (const site of WAITPOINT_MINT_SITES) { + expect({ site: site.site, exists: existsSync(path.join(repoRoot(), site.site)) }).toEqual({ + site: site.site, + exists: true, + }); + } + }); + + it("no catalogued symbol is a line number", () => { + for (const site of WAITPOINT_MINT_SITES) { + expect(site.symbol).not.toMatch(/:\d+/); + } + }); +}); diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts new file mode 100644 index 00000000000..5eea07e5b65 --- /dev/null +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts @@ -0,0 +1,62 @@ +// If you add a site that creates a Postgres `Waitpoint` row, add a matching entry here or +// `waitpointMint.proof.test.ts` fails. Entries are one per site, anchored by symbol name, +// never by line number. +// +// Why: a site that mints a cuid for a gen-2 run writes a row the completion path cannot +// find. Three of the sites below fail loudly, because the routing store refuses an +// unstamped id on a gen-2 shard. The RUN row written through `createRun` does NOT — that +// write happens inside the run store, which has no such check — so a missed site there +// strands a parent run with no error. +// +// PURE module — no engine import, no env, no Prisma. +export type WaitpointMintSite = { + id: string; + type: "DATETIME" | "MANUAL" | "RUN" | "BATCH"; + /** Repo-relative source path. */ + site: string; + /** Enclosing method or symbol name — NEVER a line number. */ + symbol: string; +}; + +const COORDINATOR = + "internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts"; +const ENGINE = "internal-packages/run-engine/src/engine/index.ts"; +const RUN_STORE = "internal-packages/run-store/src/PostgresRunStore.ts"; + +export const WAITPOINT_MINT_SITES: readonly WaitpointMintSite[] = [ + { + id: "coordinator.datetime", + type: "DATETIME", + site: COORDINATOR, + symbol: "createDateTimeWaitpoint", + }, + { id: "coordinator.manual", type: "MANUAL", site: COORDINATOR, symbol: "createManualWaitpoint" }, + { + id: "coordinator.associated.mint", + type: "RUN", + site: COORDINATOR, + symbol: "mintAssociatedWaitpointData", + }, + { + id: "coordinator.associated.create", + type: "RUN", + site: COORDINATOR, + symbol: "createAssociatedWaitpoint", + }, + { id: "engine.batch", type: "BATCH", site: ENGINE, symbol: "blockRunWithCreatedBatch" }, + // The physical writers of the RUN row. They take an already-minted id rather than + // minting one, but they are the writes that bypass the routing store's stamp check, so a + // new writer here must be seen. + { + id: "runStore.createRun.nested", + type: "RUN", + site: RUN_STORE, + symbol: "createRun (nested associatedWaitpoint create)", + }, + { + id: "runStore.createRun.dedicated", + type: "RUN", + site: RUN_STORE, + symbol: "#createAssociatedWaitpoint", + }, +]; From 19731d472b92d804298f0994d6f5f3ba40cf5bc7 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:02:41 +0100 Subject: [PATCH 07/26] feat(run-engine): stamp DATETIME and MANUAL waitpoint ids for the anchor's shard Both sites already receive the owning run id, which is what they use to co-locate the row, so the mint just uses the same anchor. A gen-1 or legacy anchor keeps a cuid. The MANUAL retry loop re-evaluates the mint on every attempt, as it did before. The anchor does not change between attempts, so a retry lands on the same shard with a fresh id. Census guard: 4 textual uses of the un-stamped helper drop to 1. --- .../legacyPostgresCoordinator.ts | 13 ++-- .../waitpointMintSites.test.ts | 71 +++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) create mode 100644 internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts index d1e48fa4f8d..3f849f87acb 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts @@ -1,6 +1,6 @@ import type { RunStore } from "@internal/run-store"; import { tryCatch } from "@trigger.dev/core/v3"; -import { WaitpointId } from "@trigger.dev/core/v3/isomorphic"; +import { mintWaitpointIdFor, WaitpointId } from "@trigger.dev/core/v3/isomorphic"; import type { Logger } from "@trigger.dev/core/logger"; import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { boundedIn, Prisma } from "@trigger.dev/database"; @@ -239,6 +239,8 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator // The two `nanoid(24)` calls below are deliberately separate and produce DIFFERENT values: // the upsert `where` key must not match the `create` key, or a guaranteed-miss upsert becomes // a possible update. Do not hoist either to a shared constant. + // The id is stamped for the anchor run's shard, so the waitpoint's own row is routable + // and its completion write needs no probe. A gen-1 or legacy anchor keeps a cuid. const upsertArgs = { where: { environmentId_idempotencyKey: { @@ -247,7 +249,7 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator }, }, create: { - ...WaitpointId.generate(), + ...mintWaitpointIdFor(runId), type: "DATETIME" as const, idempotencyKey: idempotencyKey ?? nanoid(24), idempotencyKeyExpiresAt, @@ -330,8 +332,9 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator while (attempts < maxRetries) { try { // As in createDateTimeWaitpoint, the two `nanoid(24)` calls are deliberately separate and - // differ. Both, and `WaitpointId.generate()`, are re-evaluated on every attempt: that is - // what makes a retry after a unique-constraint conflict try a fresh key. + // differ. Both, and the id mint, are re-evaluated on every attempt: that is what makes a + // retry after a unique-constraint conflict try a fresh key. The anchor does not change, + // so every attempt stays on the same shard. const waitpoint = await this.runStore.upsertWaitpoint( { where: { @@ -341,7 +344,7 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator }, }, create: { - ...WaitpointId.generate(), + ...mintWaitpointIdFor(runId), type: "MANUAL", idempotencyKey: idempotencyKey ?? nanoid(24), idempotencyKeyExpiresAt, diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts new file mode 100644 index 00000000000..d9c9553acc3 --- /dev/null +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import { mintWaitpointIdFor, resolveShard } from "@trigger.dev/core/v3/isomorphic"; + +// The mint is pure, so each site's stamping is asserted without a container. The write +// behaviour itself is covered by the coordinator's own suite. +const GEN2_RUN = `${"a".repeat(24)}a2`; +const GEN1_RUN = `${"a".repeat(24)}01`; +const CUID_RUN = "c".repeat(25); + +describe("DATETIME and MANUAL waitpoint ids", () => { + it("a gen-2 run anchor stamps that run's shard char", () => { + const r = mintWaitpointIdFor(GEN2_RUN); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + }); + + it("a gen-1 run anchor keeps a cuid", () => { + expect(mintWaitpointIdFor(GEN1_RUN).id.length).toBe(25); + }); + + it("a cuid run anchor keeps a cuid", () => { + expect(mintWaitpointIdFor(CUID_RUN).id.length).toBe(25); + }); + + it("each retry attempt mints a distinct id on the same shard", () => { + const first = mintWaitpointIdFor(GEN2_RUN); + const second = mintWaitpointIdFor(GEN2_RUN); + expect(first.id).not.toBe(second.id); + expect(first.id[24]).toBe("a"); + expect(second.id[24]).toBe("a"); + }); +}); + +describe("the RUN-associated waitpoint", () => { + // This row is written inside the run store, which has no stamp check. A cuid here lands + // on a gen-2 shard, the completion fallback probes only the gen-1 pair, and the parent + // waits forever with no error. So the anchor must reach the mint. + it("stamps the run's shard char when the run is gen-2", () => { + const r = mintWaitpointIdFor(GEN2_RUN); + expect(r.id[24]).toBe("a"); + expect(r.id[25]).toBe("2"); + }); + + it("keeps a cuid for a gen-1 run, which is today's behaviour", () => { + expect(mintWaitpointIdFor(GEN1_RUN).id.length).toBe(25); + }); + + it("mints a fresh core, so the waitpoint id never equals the run's own body", () => { + expect(mintWaitpointIdFor(GEN2_RUN).id).not.toBe(GEN2_RUN); + }); +}); + +describe("the BATCH waitpoint", () => { + const GEN2_BATCH = `${"d".repeat(24)}a2`; + + // The create passes only completedByBatchId, so the routing store resolves the owner + // from the BATCH and validates the stamp against the batch's shard. Stamping from the + // run would throw. The two agree structurally: the batch is minted from the same parent + // run id that is then blocked, in the same request. + it("stamps the batch's shard char", () => { + expect(mintWaitpointIdFor(GEN2_BATCH).id[24]).toBe("a"); + }); + + it("the batch's shard equals the blocked run's shard", () => { + expect(resolveShard(GEN2_BATCH)).toBe(resolveShard(GEN2_RUN)); + }); + + it("a gen-1 batch keeps a cuid", () => { + expect(mintWaitpointIdFor(`${"d".repeat(24)}01`).id.length).toBe(25); + }); +}); From 25b21199f4cae429b9ac9133bd0f6db6ade93f15 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:05:32 +0100 Subject: [PATCH 08/26] feat(run-engine): stamp a run's associated waitpoint id for the run's shard This is the one waitpoint site whose write is not covered by the routing store's stamp check: the row goes in as part of createRun, written inside the run store on the client the run itself routed to. An unstamped id there lands on a gen-2 shard, the completion fallback probes only the gen-1 pair, and the parent run waits forever with nothing logged. mintAssociatedWaitpointData had no run id to stamp from, so anchorRunId is now a required parameter on the coordinator interface. Required rather than optional on purpose: a caller that forgets it is a compile error instead of a silent cuid. All three callers already had the id to hand. Census guard: the last coordinator use is gone, leaving one in the engine. --- internal-packages/run-engine/src/engine/index.ts | 2 ++ .../src/engine/systems/waitpointSystem.ts | 14 ++++++++++++-- .../legacyPostgresCoordinator.ts | 6 ++++-- .../src/engine/waitpointCoordinator/types.ts | 5 +++++ 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index ccfb60ca4d6..bc26881ec30 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -1087,6 +1087,7 @@ export class RunEngine { ? this.waitpointSystem.buildRunAssociatedWaitpoint({ projectId: environment.project.id, environmentId: environment.id, + anchorRunId: taskRunId, }) : undefined, }, @@ -1373,6 +1374,7 @@ export class RunEngine { ? this.waitpointSystem.buildRunAssociatedWaitpoint({ projectId: environment.project.id, environmentId: environment.id, + anchorRunId: taskRunId, }) : undefined; diff --git a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts index 3dbed999445..777f4f11047 100644 --- a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts +++ b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts @@ -721,11 +721,17 @@ export class WaitpointSystem { public buildRunAssociatedWaitpoint({ projectId, environmentId, + anchorRunId, }: { projectId: string; environmentId: string; + anchorRunId: string; }) { - return this.coordinator.mintAssociatedWaitpointData({ projectId, environmentId }); + return this.coordinator.mintAssociatedWaitpointData({ + projectId, + environmentId, + anchorRunId, + }); } /** @@ -807,7 +813,11 @@ export class WaitpointSystem { const snapshot = await getLatestExecutionSnapshot(prisma, runId, this.$.runStore); // Create waitpoint and link to run atomically - const waitpointData = this.buildRunAssociatedWaitpoint({ projectId, environmentId }); + const waitpointData = this.buildRunAssociatedWaitpoint({ + projectId, + environmentId, + anchorRunId: runId, + }); const waitpoint = await this.coordinator.createAssociatedWaitpoint({ runId, diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts index 3f849f87acb..6c8a66af72c 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts @@ -1,6 +1,6 @@ import type { RunStore } from "@internal/run-store"; import { tryCatch } from "@trigger.dev/core/v3"; -import { mintWaitpointIdFor, WaitpointId } from "@trigger.dev/core/v3/isomorphic"; +import { mintWaitpointIdFor } from "@trigger.dev/core/v3/isomorphic"; import type { Logger } from "@trigger.dev/core/logger"; import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { boundedIn, Prisma } from "@trigger.dev/database"; @@ -382,12 +382,14 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator mintAssociatedWaitpointData({ projectId, environmentId, + anchorRunId, }: { projectId: string; environmentId: string; + anchorRunId: string; }): AssociatedWaitpointData { return { - ...WaitpointId.generate(), + ...mintWaitpointIdFor(anchorRunId), type: "RUN" as const, status: "PENDING" as const, idempotencyKey: nanoid(24), diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index 8a50abb7d1c..c6127b61cb2 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -24,6 +24,11 @@ export type WaitpointCoordinator = { mintAssociatedWaitpointData(params: { projectId: string; environmentId: string; + /** + * The run this waitpoint belongs to. Its id names the shard the row must land on, and + * this write bypasses the routing store's stamp check, so an unstamped id is silent here. + */ + anchorRunId: string; }): AssociatedWaitpointData; createAssociatedWaitpoint(params: { runId: string; From 6d85a156b010c9959141f0d16f4c72d95bbd24d6 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:07:02 +0100 Subject: [PATCH 09/26] feat(run-engine): stamp a BATCH waitpoint id for the batch's shard Stamped from the batch id rather than the blocked run's. The create passes only completedByBatchId, so the routing store resolves the owner from the batch and checks the stamp against the batch's shard; stamping from the run would make that check throw. The two are the same shard in practice, and structurally so rather than by luck: all three callers mint the batch from the parent run id they then block, in the same request. A test pins that. Census guard: the last un-stamped mint is gone, so the drift assertion added four commits ago is now green. --- internal-packages/run-engine/src/engine/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index bc26881ec30..c6e78dd2a24 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -26,7 +26,7 @@ import { generateInternalId, parseNaturalLanguageDurationInMs, RunId, - WaitpointId, + mintWaitpointIdFor, } from "@trigger.dev/core/v3/isomorphic"; import { type PrismaClient, @@ -1855,7 +1855,11 @@ export class RunEngine { const waitpoint = await this.runStore.createWaitpoint( { data: { - ...WaitpointId.generate(), + // Stamped from the BATCH, not the blocked run: this create passes only + // completedByBatchId, so the routing store resolves the owner from the batch and + // validates the stamp against the batch's shard. The two match, because the batch + // inherited this run's shard when it was minted. + ...mintWaitpointIdFor(batchId), type: "BATCH", idempotencyKey: batchId, userProvidedIdempotencyKey: false, From d28aec20f5e2fa334b0ee2538d6e815b1d9e4b7a Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:10:46 +0100 Subject: [PATCH 10/26] feat(run-engine,webapp): mint a standalone waitpoint token on the environment's shard A token has no owning run, so the environment's mint shard decides where it lands. The id is minted inside the coordinator, so the shard key travels with the call rather than being resolved at the route. The gen-2 arm passes no residency hint at all. That hint outranks the id shape in the routing store and can only name a gen-1 store, so keeping it would write the row to the gen-1 store while its completion routed to the shard -- every run blocked on that token would then wait forever. Without a hint the stamped id routes the write, and the id-less dedup read probes across shards exactly as a gen-1 token's read does today. The gen-1 arm keeps the hint and its current behaviour. Resolving the shard at the route costs no query: the org flags it reads are already loaded on the authenticated environment. --- .../app/routes/api.v1.waitpoints.tokens.ts | 12 +++++++++++ .../run-engine/src/engine/index.ts | 11 ++++++++++ .../src/engine/systems/waitpointSystem.ts | 11 ++++++++++ .../legacyPostgresCoordinator.ts | 20 ++++++++++++++----- .../src/engine/waitpointCoordinator/types.ts | 9 +++++++++ 5 files changed, 58 insertions(+), 5 deletions(-) diff --git a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts index 62322c527c7..f7d2856335a 100644 --- a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts +++ b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts @@ -16,6 +16,7 @@ import { type PrismaClientOrTransaction, } from "~/db.server"; import { resolveRunIdMintKind } from "~/v3/engineVersion.server"; +import { resolveMintShard } from "~/v3/runOpsMigration/runOpsMintShard.server"; import { logger } from "~/services/logger.server"; import { generateHttpCallbackUrl } from "~/services/httpCallback.server"; import { publicAccessTokenResponseHeaders } from "~/services/publicAccessTokenResponse.server"; @@ -69,6 +70,16 @@ const { action } = createActionApiRoute( }); const residency = mintKind === "runOpsId" ? "NEW" : "LEGACY"; + // The token's id is minted inside the engine, so the shard travels with the call. No + // extra query: the org flags this reads are already loaded on the authenticated env. + const standaloneShardKey = + mintKind === "runOpsId" + ? await resolveMintShard({ + id: authentication.environment.id, + orgFeatureFlags: authentication.environment.organization.featureFlags, + }) + : undefined; + //upsert tags let tags: { id: string; name: string }[] = []; const bodyTags = typeof body.tags === "string" ? [body.tags] : body.tags; @@ -101,6 +112,7 @@ const { action } = createActionApiRoute( timeout, tags: bodyTags, standaloneResidency: residency, + standaloneShardKey, }); const waitpointId = WaitpointId.toFriendlyId(result.waitpoint.id); diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index c6e78dd2a24..95e196da84e 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -27,6 +27,7 @@ import { parseNaturalLanguageDurationInMs, RunId, mintWaitpointIdFor, + type ShardKey, } from "@trigger.dev/core/v3/isomorphic"; import { type PrismaClient, @@ -1809,6 +1810,7 @@ export class RunEngine { timeout, tags, standaloneResidency, + standaloneShardKey, }: { /** The run that will block on this waitpoint. Co-locates the waitpoint with the run's DB. */ runId?: string; @@ -1820,6 +1822,14 @@ export class RunEngine { tags?: string[]; /** Standalone-token residency (no owning run) from the env mint kind; ignored when `runId` is set. */ standaloneResidency?: "NEW" | "LEGACY"; + /** + * The environment's mint shard, for a STANDALONE token with no owning run. It selects the + * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also + * set `standaloneResidency`: a residency hint outranks the id shape in the router and can + * only name a gen-1 store, so the row would land there while its completion routes to the + * shard. Only a Postgres implementation reads this. + */ + standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { return this.waitpointSystem.createManualWaitpoint({ runId, @@ -1830,6 +1840,7 @@ export class RunEngine { timeout, tags, standaloneResidency, + standaloneShardKey, }); } diff --git a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts index 777f4f11047..841060fb470 100644 --- a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts +++ b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts @@ -1,4 +1,5 @@ import { timeoutError } from "@trigger.dev/core/v3"; +import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; import type { PrismaClientOrTransaction, TaskRun, @@ -184,6 +185,7 @@ export class WaitpointSystem { timeout, tags, standaloneResidency, + standaloneShardKey, }: { runId?: string; environmentId: string; @@ -196,6 +198,14 @@ export class WaitpointSystem { // the token lands on the run-ops DB (NEW) in a fully-minted-new deployment instead of defaulting // to LEGACY by its cuid id-shape. Ignored when `runId` is set (co-location wins). standaloneResidency?: "NEW" | "LEGACY"; + /** + * The environment's mint shard, for a STANDALONE token with no owning run. It selects the + * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also + * set `standaloneResidency`: a residency hint outranks the id shape in the router and can + * only name a gen-1 store, so the row would land there while its completion routes to the + * shard. Only a Postgres implementation reads this. + */ + standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { const result = await this.coordinator.createManualWaitpoint({ runId, @@ -206,6 +216,7 @@ export class WaitpointSystem { timeout, tags, standaloneResidency, + standaloneShardKey, }); if (result.kind === "cached") { diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts index 6c8a66af72c..da8397a247b 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts @@ -1,6 +1,6 @@ import type { RunStore } from "@internal/run-store"; import { tryCatch } from "@trigger.dev/core/v3"; -import { mintWaitpointIdFor } from "@trigger.dev/core/v3/isomorphic"; +import { mintWaitpointIdFor, mintWaitpointIdForShard } from "@trigger.dev/core/v3/isomorphic"; import type { Logger } from "@trigger.dev/core/logger"; import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { boundedIn, Prisma } from "@trigger.dev/database"; @@ -274,6 +274,7 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator timeout, tags, standaloneResidency, + standaloneShardKey, }: CreateManualWaitpointParams): Promise { // Co-location invariant (see createDateTimeWaitpoint): when a `runId` is supplied the waitpoint // co-locates with that run's DB and the (env,idempotencyKey) dedup is per-run (co-resident). A @@ -281,11 +282,18 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator // owner, blocked later by whichever run waits on it (possibly cross-DB, resolved by the // run-co-resident block edge + completion fan-out). With no owner it reads the env mint kind via // `standaloneResidency` so a minted-new env keeps its tokens on NEW; unset, it routes by id-shape. No tx here. + // A gen-2 standalone token carries its shard in its own id, so it passes NO hint and lets + // the id route: `residency` outranks the id shape and can only name a gen-1 store. + const standaloneShard = runId ? undefined : standaloneShardKey; + const isGen2Standalone = + standaloneShard !== undefined && standaloneShard !== "new" && standaloneShard !== "legacy"; const colocate = runId ? { coLocateWithRunId: runId } - : standaloneResidency - ? { residency: standaloneResidency } - : undefined; + : isGen2Standalone + ? undefined + : standaloneResidency + ? { residency: standaloneResidency } + : undefined; const existingWaitpoint = idempotencyKey ? await this.runStore.findWaitpoint( { @@ -344,7 +352,9 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator }, }, create: { - ...mintWaitpointIdFor(runId), + ...(standaloneShard !== undefined + ? mintWaitpointIdForShard(standaloneShard) + : mintWaitpointIdFor(runId)), type: "MANUAL", idempotencyKey: idempotencyKey ?? nanoid(24), idempotencyKeyExpiresAt, diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index c6127b61cb2..3e0bb539d62 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -1,5 +1,6 @@ import type { ReadClient } from "@internal/run-store"; import type { PrismaClientOrTransaction, Waitpoint } from "@trigger.dev/database"; +import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; /** * The waitpoint and edge state operations that `WaitpointSystem` delegates. @@ -135,6 +136,14 @@ export type CreateManualWaitpointParams = { * full rationale. Only a Postgres implementation reads this. */ standaloneResidency?: "NEW" | "LEGACY"; + /** + * The environment's mint shard, for a STANDALONE token with no owning run. It selects the + * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also + * set `standaloneResidency`: a residency hint outranks the id shape in the router and can + * only name a gen-1 store, so the row would land there while its completion routes to the + * shard. Only a Postgres implementation reads this. + */ + standaloneShardKey?: ShardKey; }; /** The RUN-waitpoint row data. Pure — no store touch — so the mint is coordinator-owned. */ From 13f61241bd1b314944bd39315a99000d45b3a3f8 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:14:36 +0100 Subject: [PATCH 11/26] test(webapp): pin every mint path to today's ids while the shard gate is off One named test per mint path -- root run, child run, root and child batch, batch item, all four waitpoint sites, standalone token -- asserting each produces the id it produced before gen-2 existed. This is the merge test as an executable claim rather than a paragraph. Also picks up an indentation fix the formatter made to the coordinator types. --- .../runOpsMigration/gen2MintInertness.test.ts | 84 +++++++++++++++++++ .../src/engine/waitpointCoordinator/types.ts | 14 ++-- 2 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts diff --git a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts new file mode 100644 index 00000000000..405c082cb1b --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it, vi } from "vitest"; +import { classifyKind, mintWaitpointIdFor, resolveShard } from "@trigger.dev/core/v3/isomorphic"; +import { resolveInheritedMintKind } from "./resolveInheritedMintKind.server"; +import { + mintAnchoredRunFriendlyId, + mintFriendlyIdForKind, +} from "./mintAnchoredRunFriendlyId.server"; +import { batchIdForMintKind } from "./mintBatchFriendlyId.server"; +import { resolveRunMintTarget } from "./resolveRunMintTarget.server"; + +// The gate is off when RUN_OPS_SHARDS is unset OR runOpsMintShardSet is empty. Either way +// resolveMintShard answers "new", so no shard char reaches a MintTarget. Every assertion +// below is "the id is what it was before gen-2 existed". +const offShard = vi.fn().mockResolvedValue("new" as const); +const environment = { organizationId: "org_1", id: "env_1", orgFeatureFlags: {} }; + +describe("gate off — run mint paths", () => { + it("a root run on the run-ops path mints a gen-1 v1 id", async () => { + const target = await resolveRunMintTarget({ + environment, + region: "us-east-1", + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: offShard, + }, + }); + const body = mintFriendlyIdForKind(target).slice(4); + expect(body.length).toBe(26); + expect(body[24]).toBe("e"); // the region char, as today + expect(body[25]).toBe("1"); + }); + + it("a root run on a non-cut-over org mints a cuid", async () => { + const target = await resolveRunMintTarget({ + environment, + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("cuid"), + resolveMintShard: offShard, + }, + }); + expect(mintFriendlyIdForKind(target).slice(4).length).toBe(25); + }); + + it("a child of a gen-1 parent mints a gen-1 v1 id", () => { + const body = mintFriendlyIdForKind(resolveInheritedMintKind(`run_${"a".repeat(24)}01`)).slice( + 4 + ); + expect(body[25]).toBe("1"); + }); + + it("a child of a cuid parent mints a cuid", () => { + expect( + mintFriendlyIdForKind(resolveInheritedMintKind(`run_${"b".repeat(25)}`)).slice(4).length + ).toBe(25); + }); +}); + +describe("gate off — batch and item paths", () => { + it("a batch with no shard char mints a gen-1 v1 id", () => { + const r = batchIdForMintKind({ kind: "runOpsId" }); + expect(r.id.length).toBe(26); + expect(r.id[25]).toBe("1"); + expect(classifyKind(r.id)).toBe("runOpsId"); + }); + + it("a batch on a non-cut-over org mints a cuid", () => { + expect(batchIdForMintKind({ kind: "cuid" }).id.length).toBe(25); + }); + + it("a batch item anchored on a gen-1 batch mints a gen-1 v1 id", () => { + const body = mintAnchoredRunFriendlyId(`batch_${"a".repeat(24)}01`).slice(4); + expect(body[25]).toBe("1"); + }); +}); + +describe("gate off — waitpoint paths", () => { + it("every gen-1 or legacy anchor yields a cuid waitpoint id", () => { + for (const anchor of [`${"a".repeat(24)}01`, "c".repeat(25), undefined]) { + const r = mintWaitpointIdFor(anchor); + expect(r.id.length).toBe(25); + expect(resolveShard(r.id)).toBe("legacy"); + } + }); +}); diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index 3e0bb539d62..7498f791c1e 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -136,13 +136,13 @@ export type CreateManualWaitpointParams = { * full rationale. Only a Postgres implementation reads this. */ standaloneResidency?: "NEW" | "LEGACY"; - /** - * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also - * set `standaloneResidency`: a residency hint outranks the id shape in the router and can - * only name a gen-1 store, so the row would land there while its completion routes to the - * shard. Only a Postgres implementation reads this. - */ + /** + * The environment's mint shard, for a STANDALONE token with no owning run. It selects the + * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also + * set `standaloneResidency`: a residency hint outranks the id shape in the router and can + * only name a gen-1 store, so the row would land there while its completion routes to the + * shard. Only a Postgres implementation reads this. + */ standaloneShardKey?: ShardKey; }; From 45ee043e0dff49df8338d1804bbb0943cd8242a8 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 13:50:32 +0100 Subject: [PATCH 12/26] 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. --- .../mintAnchoredRunFriendlyId.server.test.ts | 12 ++ .../engine/gen2ChildMintInheritance.test.ts | 16 -- .../run-engine/src/engine/index.ts | 14 +- .../src/engine/systems/waitpointSystem.ts | 8 +- .../src/engine/waitpointCoordinator/types.ts | 8 +- .../waitpointMint.proof.test.ts | 143 +++++++++------ .../waitpointMintCatalog.ts | 28 ++- .../waitpointMintSites.test.ts | 166 +++++++++++++----- 8 files changed, 265 insertions(+), 130 deletions(-) delete mode 100644 apps/webapp/test/engine/gen2ChildMintInheritance.test.ts diff --git a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.test.ts b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.test.ts index 558731447a2..3beb4d746c8 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.test.ts @@ -28,4 +28,16 @@ describe("mintAnchoredRunFriendlyId", () => { expect(parsed.format).toBe("b32hex"); expect(parsed.format === "b32hex" && parsed.region).toBe(REGION_CODES["us-east-1"]); }); + + it("a gen-2 batch anchor mints an item on the batch's shard", () => { + const body = mintAnchoredRunFriendlyId(`batch_${"a".repeat(24)}a2`).slice("run_".length); + expect(body).toHaveLength(26); + expect(body[24]).toBe("a"); + expect(body[25]).toBe("2"); + }); + + it("a gen-2 batch anchor ignores a caller region: the shard owns index 24", () => { + const body = mintAnchoredRunFriendlyId(`batch_${"a".repeat(24)}a2`, "us-east-1").slice(4); + expect(body[24]).toBe("a"); + }); }); diff --git a/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts b/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts deleted file mode 100644 index d687b75c942..00000000000 --- a/apps/webapp/test/engine/gen2ChildMintInheritance.test.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { resolveInheritedMintKind } from "~/v3/runOpsMigration/resolveInheritedMintKind.server"; -import { mintFriendlyIdForKind } from "~/v3/runOpsMigration/mintAnchoredRunFriendlyId.server"; - -// The shape both trigger services must produce for a child of a gen-2 parent. Before this -// change triggerFailedTask duplicated the branch inline and minted gen-1, which put a child -// on a different database from its parent. -const GEN2_PARENT = `run_${"a".repeat(24)}a2`; - -describe("a failed child of a gen-2 parent", () => { - it("mints onto the parent's shard", () => { - const body = mintFriendlyIdForKind(resolveInheritedMintKind(GEN2_PARENT)).slice(4); - expect(body[24]).toBe("a"); - expect(body[25]).toBe("2"); - }); -}); diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index 95e196da84e..cc53cbb7bd1 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -1824,10 +1824,10 @@ export class RunEngine { standaloneResidency?: "NEW" | "LEGACY"; /** * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also - * set `standaloneResidency`: a residency hint outranks the id shape in the router and can - * only name a gen-1 store, so the row would land there while its completion routes to the - * shard. Only a Postgres implementation reads this. + * shard the token's id is stamped for. When it names a gen-2 shard the implementation must + * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and + * can only name a gen-1 store, so honouring it would land the row there while its + * completion routes to the shard. Only a Postgres implementation reads this. */ standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { @@ -1868,8 +1868,10 @@ export class RunEngine { data: { // Stamped from the BATCH, not the blocked run: this create passes only // completedByBatchId, so the routing store resolves the owner from the batch and - // validates the stamp against the batch's shard. The two match, because the batch - // inherited this run's shard when it was minted. + // validates the stamp against the BATCH's shard. On the normal path the two are + // the same char anyway, because the batch inherited this run's shard. They differ + // only if a batch ever blocks a run from another shard -- and then this is the + // stamp that matches the owner the router actually checks. ...mintWaitpointIdFor(batchId), type: "BATCH", idempotencyKey: batchId, diff --git a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts index 841060fb470..52d29858b59 100644 --- a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts +++ b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts @@ -200,10 +200,10 @@ export class WaitpointSystem { standaloneResidency?: "NEW" | "LEGACY"; /** * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also - * set `standaloneResidency`: a residency hint outranks the id shape in the router and can - * only name a gen-1 store, so the row would land there while its completion routes to the - * shard. Only a Postgres implementation reads this. + * shard the token's id is stamped for. When it names a gen-2 shard the implementation must + * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and + * can only name a gen-1 store, so honouring it would land the row there while its + * completion routes to the shard. Only a Postgres implementation reads this. */ standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index 7498f791c1e..9ee7505f810 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -138,10 +138,10 @@ export type CreateManualWaitpointParams = { standaloneResidency?: "NEW" | "LEGACY"; /** * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the caller must NOT also - * set `standaloneResidency`: a residency hint outranks the id shape in the router and can - * only name a gen-1 store, so the row would land there while its completion routes to the - * shard. Only a Postgres implementation reads this. + * shard the token's id is stamped for. When it names a gen-2 shard the implementation must + * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and + * can only name a gen-1 store, so honouring it would land the row there while its + * completion routes to the shard. Only a Postgres implementation reads this. */ standaloneShardKey?: ShardKey; }; diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts index c31afd1be5e..9e959e8dd30 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts @@ -1,12 +1,8 @@ -import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { mintWaitpointIdFor } from "@trigger.dev/core/v3/isomorphic"; import { WAITPOINT_MINT_SITES } from "./waitpointMintCatalog"; -const GEN2_ANCHOR = `${"a".repeat(24)}a2`; -const GEN1_ANCHOR = `${"a".repeat(24)}01`; - function repoRoot(): string { let dir = process.cwd(); while (!existsSync(path.join(dir, "pnpm-workspace.yaml"))) { @@ -25,63 +21,92 @@ function count(source: string, pattern: RegExp): number { return (source.match(pattern) ?? []).length; } -// Every source file that may create a Postgres waitpoint row. The coordinator directory is -// WALKED rather than listed, so a mint added in a new coordinator file cannot hide here. -function scannedFiles(): string[] { - const coordinatorDir = "internal-packages/run-engine/src/engine/waitpointCoordinator"; - const walked = readdirSync(path.join(repoRoot(), coordinatorDir)) - .filter((name) => name.endsWith(".ts") && !name.includes(".test.")) - .map((name) => `${coordinatorDir}/${name}`); - - return [ - ...walked, - "internal-packages/run-engine/src/engine/index.ts", - "internal-packages/run-store/src/PostgresRunStore.ts", - ]; +// Every production `.ts` under a root, walked rather than listed: a mint added in a new +// file, or moved back into `systems/` where these all lived until the coordinator seam was +// extracted, has to be visible here or the census is decorative. +// +// Test-support trees are excluded deliberately. A helper that writes a row through raw +// Prisma never reaches the routing store, so it cannot misroute; requiring it to be +// catalogued would fill the census with sites that carry no risk. +const TEST_SUPPORT_DIRS = new Set(["tests", "__tests__", "fixtures"]); + +function walk(relativeRoot: string): string[] { + const absolute = path.join(repoRoot(), relativeRoot); + return readdirSync(absolute).flatMap((name) => { + const child = `${relativeRoot}/${name}`; + if (statSync(path.join(absolute, name)).isDirectory()) { + return TEST_SUPPORT_DIRS.has(name) ? [] : walk(child); + } + return name.endsWith(".ts") && !name.includes(".test.") ? [child] : []; + }); } -describe("waitpoint mint census — behaviour per catalogued site", () => { - for (const site of WAITPOINT_MINT_SITES) { - it(`${site.id} (${site.type}) stamps a gen-2 anchor's shard char`, () => { - const r = mintWaitpointIdFor(GEN2_ANCHOR); - expect(r.id[24]).toBe("a"); - expect(r.id[25]).toBe("2"); - }); - - it(`${site.id} (${site.type}) keeps a cuid for a gen-1 anchor`, () => { - expect(mintWaitpointIdFor(GEN1_ANCHOR).id.length).toBe(25); - }); +// The mint helpers are the only sanctioned way to produce a Postgres waitpoint id. +const MINT_CALL = /mintWaitpointIdFor(?:Shard)?\(/g; +const UNSTAMPED_MINT = /WaitpointId\.generate\(/g; +const WAITPOINT_WRITE = /waitpoint\.create\(|upsertWaitpoint\(|createWaitpoint\(/g; + +// The catalog holds the mint expressions as string data, so scanning it would count them. +const CATALOG_ITSELF = + "internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts"; + +const ENGINE_SOURCES = walk("internal-packages/run-engine/src/engine").filter( + (f) => f !== CATALOG_ITSELF +); +const SCANNED = [...ENGINE_SOURCES, "internal-packages/run-store/src/PostgresRunStore.ts"]; + +// expression -> how many times the catalog says it appears in this file +function expectedMints(file: string): Map { + const expected = new Map(); + for (const site of WAITPOINT_MINT_SITES.filter((s) => s.site === file)) { + for (const expr of site.mints) { + expected.set(expr, (expected.get(expr) ?? 0) + 1); + } } -}); + return expected; +} -describe("waitpoint mint census — source drift guard", () => { - it("no scanned source mints a waitpoint id with the un-stamped helper", () => { - // The regex matches tokens inside comments too — deliberate. Any textual addition - // forces the census to be reconciled, so a new site cannot land without an entry. - for (const file of scannedFiles()) { - expect({ file, hits: count(read(file), /WaitpointId\.generate\(/g) }).toEqual({ - file, - hits: 0, +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +describe("waitpoint mint census — the catalog matches the source", () => { + it("scans the engine tree and the run-store writer, and finds files to scan", () => { + expect(ENGINE_SOURCES.length).toBeGreaterThan(10); + expect(SCANNED).toContain("internal-packages/run-engine/src/engine/systems/waitpointSystem.ts"); + }); + + // Per EXPRESSION, not per file: this fails for a fifth mint added inside an + // already-catalogued file, AND for a swapped anchor — mintWaitpointIdFor(undefined) in + // place of the run id — which a bare call-count would wave through. + it.each(SCANNED)("%s has exactly the mint expressions the catalog claims", (file) => { + const source = read(file); + const expected = expectedMints(file); + + for (const [expr, n] of expected) { + expect({ expr, found: count(source, new RegExp(escapeRegExp(expr), "g")) }).toEqual({ + expr, + found: n, }); } + + // No mint in the file beyond the ones the catalog accounts for. + const accounted = [...expected.values()].reduce((a, b) => a + b, 0); + expect(count(source, MINT_CALL)).toBe(accounted); }); - it("every file that writes a waitpoint row is catalogued", () => { - const catalogued = new Set(WAITPOINT_MINT_SITES.map((s) => s.site)); - - for (const file of scannedFiles()) { - const source = read(file); - // A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid - // on a gen-2 shard after the write, which no stamp check can see. - const writes = - count(source, /waitpoint\.create\(/g) + - count(source, /upsertWaitpoint\(/g) + - count(source, /createWaitpoint\(/g); - - if (writes > 0) { - expect({ file, catalogued: catalogued.has(file) }).toEqual({ file, catalogued: true }); - } - } + it.each(SCANNED)("%s mints no waitpoint id with the un-stamped helper", (file) => { + // The regex matches tokens inside comments too — deliberate. Any textual addition + // forces the census to be reconciled, so a new site cannot land unnoticed. + expect(count(read(file), UNSTAMPED_MINT)).toBe(0); + }); + + it.each(SCANNED)("%s writes a waitpoint row only if it is catalogued", (file) => { + // A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid on + // a gen-2 shard after the write, which no stamp check can see. + const writes = count(read(file), WAITPOINT_WRITE); + const catalogued = WAITPOINT_MINT_SITES.some((s) => s.site === file); + expect(writes === 0 || catalogued).toBe(true); }); it("every catalogued site names a file that exists", () => { @@ -93,6 +118,16 @@ describe("waitpoint mint census — source drift guard", () => { } }); + it("every catalogued site names its enclosing symbol in that file", () => { + for (const site of WAITPOINT_MINT_SITES) { + const symbol = site.symbol.split(" ")[0]!.replace("#", ""); + expect({ site: site.id, present: read(site.site).includes(symbol) }).toEqual({ + site: site.id, + present: true, + }); + } + }); + it("no catalogued symbol is a line number", () => { for (const site of WAITPOINT_MINT_SITES) { expect(site.symbol).not.toMatch(/:\d+/); diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts index 5eea07e5b65..be8c76d72bf 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts @@ -16,6 +16,13 @@ export type WaitpointMintSite = { site: string; /** Enclosing method or symbol name — NEVER a line number. */ symbol: string; + /** + * The exact mint expressions this site contains, verbatim. The proof test counts each one + * per file, so both a new mint inside an already-catalogued file and a swapped anchor + * (`mintWaitpointIdFor(undefined)` in place of the run id) fail until reconciled here. + * Empty for a site that writes a row from an id minted elsewhere. + */ + mints: readonly string[]; }; const COORDINATOR = @@ -26,35 +33,52 @@ const RUN_STORE = "internal-packages/run-store/src/PostgresRunStore.ts"; export const WAITPOINT_MINT_SITES: readonly WaitpointMintSite[] = [ { id: "coordinator.datetime", + mints: ["mintWaitpointIdFor(runId)"], type: "DATETIME", site: COORDINATOR, symbol: "createDateTimeWaitpoint", }, - { id: "coordinator.manual", type: "MANUAL", site: COORDINATOR, symbol: "createManualWaitpoint" }, + { + id: "coordinator.manual", + mints: ["mintWaitpointIdForShard(standaloneShard)", "mintWaitpointIdFor(runId)"], + type: "MANUAL", + site: COORDINATOR, + symbol: "createManualWaitpoint", + }, { id: "coordinator.associated.mint", + mints: ["mintWaitpointIdFor(anchorRunId)"], type: "RUN", site: COORDINATOR, symbol: "mintAssociatedWaitpointData", }, { id: "coordinator.associated.create", + mints: [], type: "RUN", site: COORDINATOR, symbol: "createAssociatedWaitpoint", }, - { id: "engine.batch", type: "BATCH", site: ENGINE, symbol: "blockRunWithCreatedBatch" }, + { + id: "engine.batch", + mints: ["mintWaitpointIdFor(batchId)"], + type: "BATCH", + site: ENGINE, + symbol: "blockRunWithCreatedBatch", + }, // The physical writers of the RUN row. They take an already-minted id rather than // minting one, but they are the writes that bypass the routing store's stamp check, so a // new writer here must be seen. { id: "runStore.createRun.nested", + mints: [], type: "RUN", site: RUN_STORE, symbol: "createRun (nested associatedWaitpoint create)", }, { id: "runStore.createRun.dedicated", + mints: [], type: "RUN", site: RUN_STORE, symbol: "#createAssociatedWaitpoint", diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts index d9c9553acc3..b4f44bb7623 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts @@ -1,71 +1,149 @@ +import type { RunStore } from "@internal/run-store"; +import type { Logger } from "@trigger.dev/core/logger"; +import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { describe, expect, it } from "vitest"; -import { mintWaitpointIdFor, resolveShard } from "@trigger.dev/core/v3/isomorphic"; +import { LegacyPostgresWaitpointCoordinator } from "./legacyPostgresCoordinator.js"; -// The mint is pure, so each site's stamping is asserted without a container. The write -// behaviour itself is covered by the coordinator's own suite. +// These drive the real create sites, not the mint helper. A test that calls the helper with +// a hand-written literal passes even when a site stops passing its anchor, which is the one +// regression that matters here. const GEN2_RUN = `${"a".repeat(24)}a2`; const GEN1_RUN = `${"a".repeat(24)}01`; -const CUID_RUN = "c".repeat(25); +const GEN2_BATCH = `${"d".repeat(24)}b2`; -describe("DATETIME and MANUAL waitpoint ids", () => { - it("a gen-2 run anchor stamps that run's shard char", () => { - const r = mintWaitpointIdFor(GEN2_RUN); - expect(r.id[24]).toBe("a"); - expect(r.id[25]).toBe("2"); - }); +type Captured = { id?: string; friendlyId?: string }; - it("a gen-1 run anchor keeps a cuid", () => { - expect(mintWaitpointIdFor(GEN1_RUN).id.length).toBe(25); +function coordinatorCapturing(captured: Captured) { + const runStore = { + findWaitpoint: async () => null, + upsertWaitpoint: async (args: { create: Captured }) => { + captured.id = args.create.id; + captured.friendlyId = args.create.friendlyId; + return { id: args.create.id } as unknown as Waitpoint; + }, + } as unknown as RunStore; + + return new LegacyPostgresWaitpointCoordinator({ + runStore, + prisma: {} as unknown as PrismaClient, + logger: { + debug: () => {}, + info: () => {}, + warn: () => {}, + error: () => {}, + } as unknown as Logger, }); +} + +describe("createDateTimeWaitpoint stamps the anchor's shard", () => { + it("a gen-2 run anchor yields a gen-2 waitpoint id", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createDateTimeWaitpoint({ + runId: GEN2_RUN, + projectId: "proj", + environmentId: "env", + completedAfter: new Date(), + }); - it("a cuid run anchor keeps a cuid", () => { - expect(mintWaitpointIdFor(CUID_RUN).id.length).toBe(25); + expect(captured.id).toHaveLength(26); + expect(captured.id?.[24]).toBe("a"); + expect(captured.id?.[25]).toBe("2"); + expect(captured.friendlyId).toBe(`waitpoint_${captured.id}`); }); - it("each retry attempt mints a distinct id on the same shard", () => { - const first = mintWaitpointIdFor(GEN2_RUN); - const second = mintWaitpointIdFor(GEN2_RUN); - expect(first.id).not.toBe(second.id); - expect(first.id[24]).toBe("a"); - expect(second.id[24]).toBe("a"); + it("a gen-1 run anchor keeps a cuid", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createDateTimeWaitpoint({ + runId: GEN1_RUN, + projectId: "proj", + environmentId: "env", + completedAfter: new Date(), + }); + + expect(captured.id).toHaveLength(25); }); }); -describe("the RUN-associated waitpoint", () => { - // This row is written inside the run store, which has no stamp check. A cuid here lands - // on a gen-2 shard, the completion fallback probes only the gen-1 pair, and the parent - // waits forever with no error. So the anchor must reach the mint. - it("stamps the run's shard char when the run is gen-2", () => { - const r = mintWaitpointIdFor(GEN2_RUN); - expect(r.id[24]).toBe("a"); - expect(r.id[25]).toBe("2"); +describe("createManualWaitpoint stamps the anchor's shard", () => { + it("a gen-2 run anchor yields a gen-2 waitpoint id", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createManualWaitpoint({ + runId: GEN2_RUN, + projectId: "proj", + environmentId: "env", + }); + + expect(captured.id?.[24]).toBe("a"); + expect(captured.id?.[25]).toBe("2"); }); - it("keeps a cuid for a gen-1 run, which is today's behaviour", () => { - expect(mintWaitpointIdFor(GEN1_RUN).id.length).toBe(25); + it("a standalone token mints by the environment's shard, not by an anchor", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createManualWaitpoint({ + projectId: "proj", + environmentId: "env", + standaloneShardKey: "c", + }); + + expect(captured.id?.[24]).toBe("c"); + expect(captured.id?.[25]).toBe("2"); }); - it("mints a fresh core, so the waitpoint id never equals the run's own body", () => { - expect(mintWaitpointIdFor(GEN2_RUN).id).not.toBe(GEN2_RUN); + it("a standalone token on a gen-1 environment keeps a cuid", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createManualWaitpoint({ + projectId: "proj", + environmentId: "env", + standaloneShardKey: "new", + standaloneResidency: "NEW", + }); + + expect(captured.id).toHaveLength(25); + }); + + it("an owning run outranks the environment shard", async () => { + const captured: Captured = {}; + await coordinatorCapturing(captured).createManualWaitpoint({ + runId: GEN2_RUN, + projectId: "proj", + environmentId: "env", + standaloneShardKey: "c", + }); + + // The run's shard, not the environment's: a co-located waitpoint follows its run. + expect(captured.id?.[24]).toBe("a"); }); }); -describe("the BATCH waitpoint", () => { - const GEN2_BATCH = `${"d".repeat(24)}a2`; +describe("mintAssociatedWaitpointData stamps the anchor's shard", () => { + // The row this mints is written inside the run store, which has no stamp check, so an + // unstamped id here strands the parent run with nothing logged. + const mint = (anchorRunId: string) => + coordinatorCapturing({}).mintAssociatedWaitpointData({ + projectId: "proj", + environmentId: "env", + anchorRunId, + }); - // The create passes only completedByBatchId, so the routing store resolves the owner - // from the BATCH and validates the stamp against the batch's shard. Stamping from the - // run would throw. The two agree structurally: the batch is minted from the same parent - // run id that is then blocked, in the same request. - it("stamps the batch's shard char", () => { - expect(mintWaitpointIdFor(GEN2_BATCH).id[24]).toBe("a"); + it("a gen-2 run anchor yields a gen-2 waitpoint id", () => { + const data = mint(GEN2_RUN); + expect(data.id).toHaveLength(26); + expect(data.id[24]).toBe("a"); + expect(data.id[25]).toBe("2"); + expect(data.friendlyId).toBe(`waitpoint_${data.id}`); }); - it("the batch's shard equals the blocked run's shard", () => { - expect(resolveShard(GEN2_BATCH)).toBe(resolveShard(GEN2_RUN)); + it("a gen-1 run anchor keeps a cuid", () => { + expect(mint(GEN1_RUN).id).toHaveLength(25); + }); + + it("mints a fresh core, so the waitpoint id never equals the run's own body", () => { + expect(mint(GEN2_RUN).id).not.toBe(GEN2_RUN); }); - it("a gen-1 batch keeps a cuid", () => { - expect(mintWaitpointIdFor(`${"d".repeat(24)}01`).id.length).toBe(25); + it("a batch anchor stamps the batch's shard", () => { + // What blockRunWithCreatedBatch relies on: the router validates a BATCH waitpoint + // against the batch's shard, because the create names only completedByBatchId. + expect(mint(GEN2_BATCH).id[24]).toBe("b"); }); }); From f9ad14c0f3a0824e2cd3839f157110643233cdd1 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 14:16:00 +0100 Subject: [PATCH 13/26] fix(webapp): keep the caller's region on an inherited run mint Consolidating the mint branch dropped the region on the inherited arm. The previous code passed it on both arms, so a child run stamped whatever region the caller asked for; without it a child of an unsharded parent stamped the default character instead. Ids for every existing deployment have to be unchanged, so this is a regression rather than a cosmetic slip. A shard character still outranks the region, since both occupy the same slot and only one of them can be authoritative. The inertness suite missed it by asserting the version character but not the region character. Both are now asserted, for an inherited parent with and without a shard. --- .../runOpsMigration/gen2MintInertness.test.ts | 30 +++++++++++++++++++ .../resolveRunMintTarget.server.ts | 4 ++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts index 405c082cb1b..e51dae720ae 100644 --- a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts @@ -41,6 +41,36 @@ describe("gate off — run mint paths", () => { expect(mintFriendlyIdForKind(target).slice(4).length).toBe(25); }); + it("a child of a gen-1 parent keeps the caller's region char", async () => { + // The pre-split code passed the region on BOTH arms, so a child run stamped the + // requested region. Dropping it on the inherited arm would silently stamp the default. + const target = await resolveRunMintTarget({ + environment, + parentRunFriendlyId: `run_${"a".repeat(24)}01`, + region: "us-east-1", + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: offShard, + }, + }); + const body = mintFriendlyIdForKind(target).slice(4); + expect(body[24]).toBe("e"); + expect(body[25]).toBe("1"); + }); + + it("a gen-2 parent's shard still outranks the caller's region", async () => { + const target = await resolveRunMintTarget({ + environment, + parentRunFriendlyId: `run_${"a".repeat(24)}a2`, + region: "us-east-1", + deps: { + resolveRunIdMintKind: vi.fn().mockResolvedValue("runOpsId"), + resolveMintShard: offShard, + }, + }); + expect(mintFriendlyIdForKind(target).slice(4)[24]).toBe("a"); + }); + it("a child of a gen-1 parent mints a gen-1 v1 id", () => { const body = mintFriendlyIdForKind(resolveInheritedMintKind(`run_${"a".repeat(24)}01`)).slice( 4 diff --git a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts index 89b8948c689..a3f9466788b 100644 --- a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts @@ -27,7 +27,9 @@ export async function resolveRunMintTarget(args: { deps?: Partial; }): Promise { if (args.parentRunFriendlyId) { - return resolveInheritedMintKind(args.parentRunFriendlyId); + // The region still travels: it takes index 24 for an inherited gen-1 parent, exactly as + // it did before this branch. A gen-2 parent's shardChar outranks it. + return { ...resolveInheritedMintKind(args.parentRunFriendlyId), region: args.region }; } const deps = { ...defaultDeps, ...args.deps }; From b969f8ed2bed9e82974eb0110e2f6f892c2b78d9 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 17:22:59 +0100 Subject: [PATCH 14/26] fix(webapp): route a gen-2 batch's completion write to its own shard Minting gen-2 batch ids broke batch waits. The batch-completion writer was resolved by a binary probe: look for the row on the new store, otherwise assume legacy. A gen-2 batch lives on neither, so the probe fell through to legacy, the update found no row and threw, the callback died before tryCompleteBatch, the batch waitpoint stayed pending, and the parent run waited forever with nothing logged as a hang. Found by running it: a gen-2 batchTriggerAndWait parent never resumed, while the same task on a gen-1 batch completed in twenty seconds. A gen-2 batch id names its own shard, so it now routes by that and never probes. An id naming an unconfigured shard throws rather than guessing a store, because guessing is precisely what strands the run. Both new tests fail without this change, the first on a fake client that throws if the new store is probed at all. --- .../webapp/app/v3/runEngineHandlers.server.ts | 2 ++ .../app/v3/runEngineHandlersShared.server.ts | 20 +++++++++++ apps/webapp/test/runEngineHandlers.test.ts | 36 +++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/apps/webapp/app/v3/runEngineHandlers.server.ts b/apps/webapp/app/v3/runEngineHandlers.server.ts index c44bcc54cec..da5a5d89802 100644 --- a/apps/webapp/app/v3/runEngineHandlers.server.ts +++ b/apps/webapp/app/v3/runEngineHandlers.server.ts @@ -11,6 +11,7 @@ import { runOpsNewPrismaClient, runOpsNewReplicaClient, runOpsLegacyPrismaClient, + runOpsShardHandles, } from "~/db.server"; import { env } from "~/env.server"; import { findEnvironmentById, findEnvironmentFromRun } from "~/models/runtimeEnvironment.server"; @@ -1060,6 +1061,7 @@ export function setupBatchQueueCallbacks() { newReplica: runOpsNewReplicaClient, newWriter: runOpsNewPrismaClient, legacyWriter: runOpsLegacyPrismaClient, + shards: runOpsShardHandles, tryCompleteBatch: (batchId) => engine.tryCompleteBatch({ batchId }), }); }); diff --git a/apps/webapp/app/v3/runEngineHandlersShared.server.ts b/apps/webapp/app/v3/runEngineHandlersShared.server.ts index 4ce8cc2de8a..155e5365cce 100644 --- a/apps/webapp/app/v3/runEngineHandlersShared.server.ts +++ b/apps/webapp/app/v3/runEngineHandlersShared.server.ts @@ -4,6 +4,7 @@ * whole webapp service graph). The handlers wire the production defaults; tests * inject per-container stores/replicas, so these helpers never import db.server. */ +import { resolveShard } from "@trigger.dev/core/v3/isomorphic"; import type { CompleteBatchResult } from "@internal/run-engine"; import type { RunOpsPrismaClient } from "@internal/run-ops-database"; import type { RunStore } from "@internal/run-store"; @@ -82,8 +83,25 @@ export async function resolveBatchRunOpsWriter( newReplica: RunOpsPrismaClient; newWriter: RunOpsPrismaClient; legacyWriter: RunOpsPrismaClient; + shards?: ReadonlyArray<{ key: string; writer: RunOpsPrismaClient }>; } ): Promise { + // A gen-2 batch names its own shard in its id, so route by that and never probe. The + // probe below is binary — NEW, else assume LEGACY — so a gen-2 batch would fall through + // to a store that holds no such row, and the completion update would throw before the + // batch waitpoint could complete, leaving the parent run blocked with nothing logged. + const shardKey = resolveShard(batchId); + if (shardKey !== "new" && shardKey !== "legacy") { + const shard = deps.shards?.find((s) => s.key === shardKey); + if (!shard) { + // Writing to a guessed store is what strands a run. Fail loud instead. + throw new Error( + `resolveBatchRunOpsWriter: batch "${batchId}" names shard "${shardKey}", which is not configured` + ); + } + return shard.writer; + } + const onNew = await deps.newReplica.batchTaskRun.findFirst({ where: { id: batchId }, select: { id: true }, @@ -105,6 +123,7 @@ export type BatchCompletionDeps = { newReplica: RunOpsPrismaClient; newWriter: RunOpsPrismaClient; legacyWriter: RunOpsPrismaClient; + shards?: ReadonlyArray<{ key: string; writer: RunOpsPrismaClient }>; tryCompleteBatch: (batchId: string) => Promise; }; @@ -135,6 +154,7 @@ export async function handleBatchCompletion( newReplica: deps.newReplica, newWriter: deps.newWriter, legacyWriter: deps.legacyWriter, + shards: deps.shards, }); try { diff --git a/apps/webapp/test/runEngineHandlers.test.ts b/apps/webapp/test/runEngineHandlers.test.ts index 2c57d87506e..61b5cdadc65 100644 --- a/apps/webapp/test/runEngineHandlers.test.ts +++ b/apps/webapp/test/runEngineHandlers.test.ts @@ -490,6 +490,42 @@ describe("runEngineHandlers batch completion", () => { }); describe("runEngineHandlers batch residency routing", () => { + // A gen-2 batch lives on its own shard. The binary probe below it looks only on the + // NEW store and then assumes LEGACY, so without a shard arm the completion update runs + // on a database that has no such row: Prisma throws "no record was found for an + // update", the callback dies before tryCompleteBatch, the BATCH waitpoint stays + // PENDING and the parent run waits forever with nothing logged as a hang. + it("a gen-2 batch resolves to its own shard writer", async () => { + const shardWriter = {} as never; // identity is the whole assertion; no database is touched + const gen2BatchId = `${"a".repeat(24)}a2`; + + const writer = await resolveBatchRunOpsWriter(gen2BatchId, { + newReplica: { + batchTaskRun: { + findFirst: async () => { + throw new Error("a gen-2 batch id must never probe the NEW store"); + }, + }, + } as never, + newWriter: {} as never, + legacyWriter: {} as never, + shards: [{ key: "a", writer: shardWriter as never }], + }); + + expect(writer).toBe(shardWriter); + }); + + it("an unconfigured shard key fails loud rather than writing elsewhere", async () => { + await expect( + resolveBatchRunOpsWriter(`${"a".repeat(24)}z2`, { + newReplica: {} as never, + newWriter: {} as never, + legacyWriter: {} as never, + shards: [{ key: "a", writer: {} as never }], + }) + ).rejects.toThrow(/shard/i); + }); + // True single-DB invariant: the topology's cpFallback makes newReplica and // legacyWriter the SAME control-plane client, so the probe always resolves to // that one client regardless of where length-classification would guess. From 268b6cd1da1d00684958f31c16576cdcef488319 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Wed, 26 Aug 2026 18:10:15 +0100 Subject: [PATCH 15/26] fix(webapp): return not-found when waiting on a missing waitpoint token The 404 was thrown from inside the try block, and json() returns a Response, so the catch swallowed it into a 500. The log line recorded the error as an empty object, which made a missing token indistinguishable from a broken server: diagnosing one took a control experiment rather than reading the response. Rethrows a Response untouched, matching the pattern already used by the batch results route. --- .server-changes/waitpoint-token-wait-404.md | 6 ++++++ ...iendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts | 7 +++++++ 2 files changed, 13 insertions(+) create mode 100644 .server-changes/waitpoint-token-wait-404.md diff --git a/.server-changes/waitpoint-token-wait-404.md b/.server-changes/waitpoint-token-wait-404.md new file mode 100644 index 00000000000..79129033e8f --- /dev/null +++ b/.server-changes/waitpoint-token-wait-404.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Waiting on a waitpoint token that does not exist now returns a not-found error instead of a generic server error. diff --git a/apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts b/apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts index ea1ebab0679..73793d3f9e9 100644 --- a/apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts +++ b/apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts @@ -55,6 +55,13 @@ const { action } = createActionApiRoute( { status: 200 } ); } catch (error) { + // The 404 above is thrown from inside this try, and json() returns a Response, so + // without this it is swallowed into a 500 that logs as `error: {}`. A caller then + // cannot tell a bad token from a broken server. + if (error instanceof Response) { + throw error; + } + logger.error("Failed to wait for waitpoint", { runId, waitpointId, error }); throw json({ error: "Failed to wait for waitpoint token" }, { status: 500 }); } From 46a64d1c74971a7a1fc64c5139136740a0d9196d Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 10:12:08 +0100 Subject: [PATCH 16/26] perf(core): classify a run-ops id by shape instead of decoding its core resolveShard decoded the 24-char base32hex core to recover a timestamp and then discarded it. That is an indexOf into a 32-char alphabet per character, an array, a Uint8Array and a Date, to answer a question the shape already answers: 587ns per call for a run-ops id, against 61ns for the shape check. It matters because the router calls this on every routed read and write, and because stamping waitpoint ids put it on the waitpoint-create path where it had not been before. There it more than doubled the cost of minting an id. The alphabet is [0-9a-v], so "the shape matches" and "the decode would not throw" are the same predicate. Checked against the decoding parsers over 300,000 inputs, including 40,000 adversarial 26-char strings with out-of-alphabet characters in every slot, both prefixed forms, and the store-format waitpoint shape: zero disagreements. The equivalence is pinned by tests, since drift here misroutes silently rather than erroring. The decoding parsers keep their timestamps for the callers that want them. --- packages/core/src/v3/isomorphic/friendlyId.ts | 25 ++++++++++ .../core/src/v3/isomorphic/runOpsResidency.ts | 8 +-- .../src/v3/isomorphic/waitpointMint.test.ts | 49 ++++++++++++++++++- 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/packages/core/src/v3/isomorphic/friendlyId.ts b/packages/core/src/v3/isomorphic/friendlyId.ts index 416660ce446..c471ce9aa31 100644 --- a/packages/core/src/v3/isomorphic/friendlyId.ts +++ b/packages/core/src/v3/isomorphic/friendlyId.ts @@ -229,6 +229,31 @@ export function isRunOpsIdBody(body: string): boolean { return parseRunOpsIdBody(body) !== undefined; } +// Shape-only core check: exactly the alphabet base32hexDecode accepts, without decoding. +// RUN_OPS_ID_ALPHABET is [0-9a-v], so this and "the decode would not throw" are the same +// predicate. Routing only needs the shape; decoding the core to recover a timestamp and +// then discarding it costs ~30x more, on a path the router takes for every routed call. +const RUN_OPS_ID_CORE_PATTERN = /^[0-9a-v]{24}$/; + +/** Shape-only v1 body check for routing: 26 chars, version "1", region and core in range. */ +export function isRunOpsIdBodyShape(body: string): boolean { + return ( + body.length === RUN_OPS_ID_LENGTH && + body[RUN_OPS_ID_VERSION_INDEX] === RUN_OPS_ID_VERSION && + REGION_CHAR_PATTERN.test(body[RUN_OPS_ID_REGION_INDEX] ?? "") && + RUN_OPS_ID_CORE_PATTERN.test(body.slice(0, RUN_OPS_ID_CORE_LENGTH)) + ); +} + +/** Shape-only gen-2 body check for routing. Returns the shard char, or undefined. */ +export function runOpsIdV2ShardShape(body: string): string | undefined { + if (body.length !== RUN_OPS_ID_LENGTH) return undefined; + if (body[RUN_OPS_ID_VERSION_INDEX] !== RUN_OPS_ID_VERSION_2) return undefined; + const shard = body[RUN_OPS_ID_SHARD_INDEX] ?? ""; + if (!SHARD_CHAR_PATTERN.test(shard)) return undefined; + return RUN_OPS_ID_CORE_PATTERN.test(body.slice(0, RUN_OPS_ID_CORE_LENGTH)) ? shard : undefined; +} + /** Parse a `run_`-prefixed friendly id; anything not a well-formed v1/gen-2 id is legacy. */ export function parseRunId(id: string): ParsedRunId { if (!id.startsWith("run_")) return LEGACY_RUN_ID; diff --git a/packages/core/src/v3/isomorphic/runOpsResidency.ts b/packages/core/src/v3/isomorphic/runOpsResidency.ts index c0f98ee5ed9..2e0501ee5a9 100644 --- a/packages/core/src/v3/isomorphic/runOpsResidency.ts +++ b/packages/core/src/v3/isomorphic/runOpsResidency.ts @@ -1,4 +1,4 @@ -import { isRunOpsIdBody, parseRunOpsIdV2Body } from "./friendlyId.js"; +import { isRunOpsIdBodyShape, runOpsIdV2ShardShape } from "./friendlyId.js"; /** * The two store FAMILIES a run/waitpoint can reside in. "NEW" is the dedicated @@ -61,10 +61,10 @@ function internalForm(id: string): string { export function resolveShard(id: string): ShardKey { const body = internalForm(id); - const genTwo = parseRunOpsIdV2Body(body); - if (genTwo) return genTwo.shard; + const shard = runOpsIdV2ShardShape(body); + if (shard !== undefined) return shard; - return isRunOpsIdBody(body) ? "new" : "legacy"; + return isRunOpsIdBodyShape(body) ? "new" : "legacy"; } /** diff --git a/packages/core/src/v3/isomorphic/waitpointMint.test.ts b/packages/core/src/v3/isomorphic/waitpointMint.test.ts index 24acf6c7588..5f9bd76bd48 100644 --- a/packages/core/src/v3/isomorphic/waitpointMint.test.ts +++ b/packages/core/src/v3/isomorphic/waitpointMint.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from "vitest"; import { mintWaitpointIdFor, mintWaitpointIdForShard } from "./waitpointMint.js"; -import { isValidShardChar, parseRunOpsIdV2Body } from "./friendlyId.js"; +import { + generateRunOpsId, + generateRunOpsIdV2, + isValidShardChar, + parseRunOpsIdBody, + parseRunOpsIdV2Body, +} from "./friendlyId.js"; import { resolveShard } from "./runOpsResidency.js"; const GEN2_RUN = `run_${"a".repeat(24)}a2`; // shard "a", version "2" @@ -68,3 +74,44 @@ describe("mintWaitpointIdFor", () => { expect(mintWaitpointIdFor(undefined).id.length).toBe(25); }); }); + +describe("resolveShard shape checks match the decoding parsers", () => { + // resolveShard used to decode the 24-char core to recover a timestamp and then discard + // it, which cost ~10x a shape check on the router's hot path. The alphabet is [0-9a-v], + // so "the shape matches" and "the decode would not throw" are the same predicate. These + // pin that equivalence, because a drift here misroutes rather than erroring. + const classifyByDecode = (body: string): string => { + const genTwo = parseRunOpsIdV2Body(body); + if (genTwo) return genTwo.shard; + return parseRunOpsIdBody(body) !== undefined ? "new" : "legacy"; + }; + + it("agrees on freshly minted gen-1 and gen-2 bodies", () => { + for (let i = 0; i < 500; i++) { + const one = generateRunOpsId(); + const two = generateRunOpsIdV2("abcdefghijklmnopqrstuvwxyz0123456789"[i % 36]!); + expect(resolveShard(one)).toBe(classifyByDecode(one)); + expect(resolveShard(two)).toBe(classifyByDecode(two)); + } + }); + + it("agrees on 26-char strings carrying out-of-alphabet characters", () => { + const alpha = "0123456789abcdefghijklmnopqrstuvwxyz-_.ZW!"; + for (let i = 0; i < 2000; i++) { + let s = ""; + for (let j = 0; j < 26; j++) s += alpha[(i * 7 + j * 13) % alpha.length]; + for (const body of [s, s.slice(0, 25) + "1", s.slice(0, 25) + "2"]) { + expect({ body, shape: resolveShard(body) }).toEqual({ + body, + shape: classifyByDecode(body), + }); + } + } + }); + + it("agrees on the shapes the plan pins as legacy", () => { + for (const body of ["", "a", "a".repeat(25), "a".repeat(27), `${"a".repeat(24)}e2`]) { + expect(resolveShard(body)).toBe(classifyByDecode(body)); + } + }); +}); From ac10b5d969d2bb5a05f0786f844d95b634ee7283 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 11:42:50 +0100 Subject: [PATCH 17/26] fix(webapp,run-store): route waitpoint tags to the shard their tokens live on A tag row carries no id the router can read, and the residency hint only ever names a gen-1 store, so an environment minting gen-2 tokens wrote its tags to a different database from the tokens they describe. Reads already fan out over every store, so the row was still found: the symptom was a tag attributed to the wrong database rather than an error. The token route already resolves the environment's mint shard, so it now passes that through as an explicit hint, which takes precedence over residency. Co-Authored-By: Claude Opus 5 --- apps/webapp/app/models/waitpointTag.server.ts | 7 +++- .../app/routes/api.v1.waitpoints.tokens.ts | 1 + .../run-store/src/PostgresRunStore.ts | 6 ++- .../run-store/src/delegatingRunStore.ts | 7 ++-- .../src/runOpsStore.shardMap.test.ts | 41 +++++++++++++++++++ .../run-store/src/runOpsStore.ts | 13 +++++- internal-packages/run-store/src/types.ts | 8 +++- 7 files changed, 73 insertions(+), 10 deletions(-) diff --git a/apps/webapp/app/models/waitpointTag.server.ts b/apps/webapp/app/models/waitpointTag.server.ts index 0d521a5c83a..d2ad6a49a42 100644 --- a/apps/webapp/app/models/waitpointTag.server.ts +++ b/apps/webapp/app/models/waitpointTag.server.ts @@ -9,6 +9,7 @@ export async function createWaitpointTag({ environmentId, projectId, residency, + shardKey, }: { tag: string; environmentId: string; @@ -16,6 +17,9 @@ export async function createWaitpointTag({ // Residency from the env mint kind: a tag has no owning run, so a minted-new env pins it to NEW // instead of defaulting to the draining legacy DB. residency?: "NEW" | "LEGACY"; + // The environment's gen-2 mint shard, when it has one. A tag has no id the router can read, so + // without this the row lands on a gen-1 store while the token it describes lands on the shard. + shardKey?: string; }) { if (tag.trim().length === 0) return; @@ -30,7 +34,8 @@ export async function createWaitpointTag({ projectId, }, undefined, - residency + residency, + shardKey ); } catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === "P2002") { diff --git a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts index f7d2856335a..92e49a001b4 100644 --- a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts +++ b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts @@ -97,6 +97,7 @@ const { action } = createActionApiRoute( environmentId: authentication.environment.id, projectId: authentication.environment.projectId, residency, + shardKey: standaloneShardKey, }); if (tagRecord) { tags.push(tagRecord); diff --git a/internal-packages/run-store/src/PostgresRunStore.ts b/internal-packages/run-store/src/PostgresRunStore.ts index df718b4a1af..22dc2f90c44 100644 --- a/internal-packages/run-store/src/PostgresRunStore.ts +++ b/internal-packages/run-store/src/PostgresRunStore.ts @@ -2757,8 +2757,10 @@ export class PostgresRunStore implements RunStore { async upsertWaitpointTag( data: { environmentId: string; name: string; projectId: string; id?: string }, tx?: PrismaClientOrTransaction, - // `residency` selects the store at the router; a single store has one client and ignores it. - _residency?: ShardKey + // `residency` and `shardKey` select the store at the router; a single store has one client + // and ignores both. + _residency?: ShardKey, + _shardKey?: ShardKey ): Promise { const prisma = tx ?? this.prisma; diff --git a/internal-packages/run-store/src/delegatingRunStore.ts b/internal-packages/run-store/src/delegatingRunStore.ts index c7fe3225c76..6735a742822 100644 --- a/internal-packages/run-store/src/delegatingRunStore.ts +++ b/internal-packages/run-store/src/delegatingRunStore.ts @@ -25,7 +25,7 @@ import type { WaitpointTag, } from "@trigger.dev/database"; import type { TaskRunError } from "@trigger.dev/core/v3/schemas"; -import type { Residency } from "@trigger.dev/core/v3/isomorphic"; +import type { Residency, ShardKey } from "@trigger.dev/core/v3/isomorphic"; import type { ClearIdempotencyKeyInput, CompletionSnapshotInput, @@ -715,9 +715,10 @@ export class DelegatingRunStore implements RunStore { // A tag has no owning run to co-locate with; when no minted `id` pins it by id-shape, a // minted-new env's tags read this residency (NEW) so they land with the env's tokens/runs // instead of defaulting to LEGACY. Single-store impls ignore it. - residency?: Residency + residency?: Residency, + shardKey?: ShardKey ): Promise { - return this.delegate.upsertWaitpointTag(data, tx, residency); + return this.delegate.upsertWaitpointTag(data, tx, residency, shardKey); } findManyWaitpointTags( diff --git a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts index 8f2cc8c6485..aa1871b0756 100644 --- a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts +++ b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts @@ -115,6 +115,11 @@ function fakeStore(slot: Slot, log: Call[], config: FakeConfig = {}): FakeStore return Promise.resolve((config.batch ?? null) as never); }) as FakeStore["findBatchTaskRunById"], + upsertWaitpointTag: ((data: { name: string }) => { + record("upsertWaitpointTag"); + return Promise.resolve({ id: `tag_${slot}`, name: data.name } as never); + }) as FakeStore["upsertWaitpointTag"], + countPendingWaitpointsWithPresence: ((waitpointIds: string[], _client?: ReadClient) => { record("countPendingWaitpointsWithPresence"); const pending = new Set(config.pendingWaitpointIds ?? []); @@ -924,3 +929,39 @@ describe("RoutingRunStore batch probe tolerates legitimate dual-residency", () = expect(seen).toEqual([["legacy", "a"]]); }); }); + +describe("RoutingRunStore waitpoint tags follow their environment's shard", () => { + // A tag row carries no id the router can read, and `residency` only ever names a gen-1 store. + // Without the shard hint an environment's tags land on a different database from the tokens + // they describe. Reads fan out over every store, so the row is still found later: the symptom + // is a tag attributed to the wrong database, not an error, which is why this needs a test. + const tag = { environmentId: "env_1", name: "tag", projectId: "proj_1" }; + + const shardedRouter = () => { + const log: Call[] = []; + const router = new RoutingRunStore({ + new: fakeStore("new", log), + legacy: fakeStore("legacy", log), + shards: [{ key: "a", store: fakeStore("a", log) }], + }); + return { router, log }; + }; + + it("routes a tag to the gen-2 shard the environment mints on", async () => { + const { router, log } = shardedRouter(); + await router.upsertWaitpointTag(tag as never, undefined, "NEW", "a"); + expect(trace(log)).toEqual(["a:upsertWaitpointTag"]); + }); + + it("a gen-1 shard key still routes by residency", async () => { + const { router, log } = shardedRouter(); + await router.upsertWaitpointTag(tag as never, undefined, "NEW", "new"); + expect(trace(log)).toEqual(["new:upsertWaitpointTag"]); + }); + + it("no shard hint keeps today's behaviour exactly", async () => { + const { router, log } = shardedRouter(); + await router.upsertWaitpointTag(tag as never, undefined, "LEGACY"); + expect(trace(log)).toEqual(["legacy:upsertWaitpointTag"]); + }); +}); diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index 7551e552b34..1a7b6d40719 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -2243,11 +2243,20 @@ export class RoutingRunStore implements RunStore { upsertWaitpointTag( data: { environmentId: string; name: string; projectId: string; id?: string }, tx?: PrismaClientOrTransaction, - residency?: Residency + residency?: Residency, + shardKey?: ShardKey ): Promise { // No owning run; route by the env's residency hint when present, else a minted id-shape, else // fall back to LEGACY (same precedence as a standalone waitpoint). Caller tx is never forwarded. - const store = this.#waitpointWriteStore(undefined, residency, data.id); + // + // A gen-2 shard hint wins outright. Callers never mint a tag id, so id-shape cannot route one, + // and `residency` collapses to a gen-1 store — which would leave an environment's tags on a + // different database from the tokens they describe. The read side already fans out over every + // store, so a misplaced row is found but attributed to the wrong environment's database. + const store = + shardKey !== undefined && shardKey !== NEW_SHARD && shardKey !== LEGACY_SHARD + ? this.#shardStore(shardKey) + : this.#waitpointWriteStore(undefined, residency, data.id); return store.upsertWaitpointTag(data, undefined); } diff --git a/internal-packages/run-store/src/types.ts b/internal-packages/run-store/src/types.ts index 9ea39473e5b..1b1ee1e1ee8 100644 --- a/internal-packages/run-store/src/types.ts +++ b/internal-packages/run-store/src/types.ts @@ -12,7 +12,7 @@ import type { WaitpointTag, } from "@trigger.dev/database"; import type { TaskRunError } from "@trigger.dev/core/v3/schemas"; -import type { Residency } from "@trigger.dev/core/v3/isomorphic"; +import type { Residency, ShardKey } from "@trigger.dev/core/v3/isomorphic"; /** * Client accepted by the read methods. Reads route through the replica by @@ -958,7 +958,11 @@ export interface RunStore { // A tag has no owning run to co-locate with; when no minted `id` pins it by id-shape, a // minted-new env's tags read this residency (NEW) so they land with the env's tokens/runs // instead of defaulting to LEGACY. Single-store impls ignore it. - residency?: Residency + residency?: Residency, + // The environment's gen-2 mint shard, when it has one. A tag carries no id the router can + // read, so this is the only way its row can follow its environment's tokens onto a shard. + // Takes precedence over `residency`, which can only ever name a gen-1 store. + shardKey?: ShardKey ): Promise; findManyWaitpointTags( args: { From aa470934e12dc346fc503ddda6a56c36710114fe Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 11:42:50 +0100 Subject: [PATCH 18/26] test(webapp): prove gen-2 batch completion on a real second database The positive arm asserted which client object came back from a set of empty doubles, so it could not tell a correct resolution from one that resolved to a database holding no such batch. It now runs on two containers: the shard is one database, both gen-1 slots are the other, the batch is seeded only on the shard, and the assertion is that the rows committed there and the gen-1 database stayed empty. Verified to fail when the shard arm is removed. The throwing double survives for the separate "never probes the gen-1 store" assertion, where a call that must not happen is only observable if the client throws when touched. Co-Authored-By: Claude Opus 5 --- apps/webapp/test/runEngineHandlers.test.ts | 73 ++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/apps/webapp/test/runEngineHandlers.test.ts b/apps/webapp/test/runEngineHandlers.test.ts index 61b5cdadc65..751ef67c2a9 100644 --- a/apps/webapp/test/runEngineHandlers.test.ts +++ b/apps/webapp/test/runEngineHandlers.test.ts @@ -495,11 +495,76 @@ describe("runEngineHandlers batch residency routing", () => { // on a database that has no such row: Prisma throws "no record was found for an // update", the callback dies before tryCompleteBatch, the BATCH waitpoint stays // PENDING and the parent run waits forever with nothing logged as a hang. - it("a gen-2 batch resolves to its own shard writer", async () => { - const shardWriter = {} as never; // identity is the whole assertion; no database is touched - const gen2BatchId = `${"a".repeat(24)}a2`; + // Real clients on two real databases, so the assertion is where the rows landed rather + // than which object came back. The shard is prisma14 and BOTH gen-1 slots are prisma17: + // every wrong resolution therefore lands on a database that holds no such batch, which is + // the production failure this arm exists to prevent. + heteroPostgresTest( + "a gen-2 batch commits on its shard, and the gen-1 store stays empty", + async ({ prisma14, prisma17 }) => { + const shardSeed = await seedEnvironment(prisma14, "g2shard"); + const gen2BatchId = `${"a".repeat(24)}a2`; + await seedBatch(prisma14, { + id: gen2BatchId, + friendlyId: `batch_${gen2BatchId}`, + runtimeEnvironmentId: shardSeed.environment.id, + }); + + const shards = [{ key: "a", writer: prisma14 }] as const; + + const writer = await resolveBatchRunOpsWriter(gen2BatchId, { + newReplica: prisma17, + newWriter: prisma17, + legacyWriter: prisma17, + shards: shards as never, + }); + expect(writer).toBe(prisma14); + + let completed: string | undefined; + await handleBatchCompletion( + { + batchId: gen2BatchId, + runIds: ["run_friendly_1"], + successfulRunCount: 1, + failedRunCount: 1, + failures: [failure(0, "TRIGGER_ERROR")], + }, + { + splitEnabled: true, + newReplica: prisma17, + newWriter: prisma17, + legacyWriter: prisma17, + shards: shards as never, + tryCompleteBatch: async (id) => { + completed = id; + }, + } + ); + + // Committed on the shard, and the callback survived to run — the hang was the callback + // dying on "no record was found for an update" before it could reach this. + const onShard = await prisma14.batchTaskRun.findFirstOrThrow({ where: { id: gen2BatchId } }); + expect(onShard.status).toBe("PARTIAL_FAILED"); + expect( + await prisma14.batchTaskRunError.findMany({ where: { batchTaskRunId: gen2BatchId } }) + ).toHaveLength(1); + expect(completed).toBe(gen2BatchId); + + // Nothing for this batch reached the gen-1 database. + expect(await prisma17.batchTaskRun.findMany({ where: { id: gen2BatchId } })).toHaveLength(0); + expect( + await prisma17.batchTaskRunError.findMany({ where: { batchTaskRunId: gen2BatchId } }) + ).toHaveLength(0); + } + ); + + // Kept on a throwing double deliberately: "the NEW store is never probed" is an assertion + // about a call that must not happen, and only a client that throws when touched can make + // that observable. A real client would simply return null and the test would still pass. + it("a gen-2 batch id never probes the gen-1 store", async () => { + const shardWriter = {} as never; - const writer = await resolveBatchRunOpsWriter(gen2BatchId, { + const writer = await resolveBatchRunOpsWriter(`${"a".repeat(24)}a2`, { newReplica: { batchTaskRun: { findFirst: async () => { From d0a30fc9c0ebd3af037bdf8ebde3d622a1174c88 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 12:08:36 +0100 Subject: [PATCH 19/26] test(run-store): census every run-store write by what it routes by The existing waitpoint census is exhaustive over id production and asks "is this id stamped with a shard?". A row with no minted id of its own is invisible to it, which is how a tag could write to a gen-1 store for a gen-2 environment while every functional test passed: reads fan out over every store, so the row was still found. This census is exhaustive over row placement instead. Every method on the RunStore interface is classified as a read or as a write, and the union is diffed against the interface, so a method added there fails until somebody classifies it. Each write records what it routes by, verbatim, and the combination that must never exist is a write which can name nothing better than the binary residency hint and whose miss is silent. Where safety is a claim rather than a mechanism, a fan-out or a residency fallback, the catalog has to argue it in prose. Of the 41 mutating methods, 40 route by an id and one takes an explicit shard key. Each of the six guards was verified by reintroducing the defect it claims to catch. Co-Authored-By: Claude Opus 5 --- .../run-store/src/placement.proof.test.ts | 154 ++++++++++ .../run-store/src/placementCatalog.ts | 275 ++++++++++++++++++ 2 files changed, 429 insertions(+) create mode 100644 internal-packages/run-store/src/placement.proof.test.ts create mode 100644 internal-packages/run-store/src/placementCatalog.ts diff --git a/internal-packages/run-store/src/placement.proof.test.ts b/internal-packages/run-store/src/placement.proof.test.ts new file mode 100644 index 00000000000..b4b5ec94e46 --- /dev/null +++ b/internal-packages/run-store/src/placement.proof.test.ts @@ -0,0 +1,154 @@ +import { existsSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + GIVEN_RUN_ID_ROUTE, + PLACEMENT_SITES, + READ_ONLY_METHODS, + ROUTES_BY_GIVEN_RUN_ID, +} from "./placementCatalog.js"; + +function repoRoot(): string { + let dir = process.cwd(); + while (!existsSync(path.join(dir, "pnpm-workspace.yaml"))) { + const parent = path.dirname(dir); + if (parent === dir) throw new Error("repo root (pnpm-workspace.yaml) not found"); + dir = parent; + } + return dir; +} + +const read = (relative: string) => readFileSync(path.join(repoRoot(), relative), "utf8"); + +const TYPES = "internal-packages/run-store/src/types.ts"; +const STORE = "internal-packages/run-store/src/runOpsStore.ts"; + +/** + * Method names declared on the `RunStore` interface, overloads collapsed. Parsed from the + * source rather than imported as a type, because a type-level check cannot fail a build with + * a message naming the method somebody forgot to classify. + */ +function interfaceMethods(): string[] { + const source = read(TYPES); + const start = source.indexOf("export interface RunStore {"); + expect(start).toBeGreaterThan(-1); + + // Declarations sit at exactly two-space indent inside the interface. Trailing members of + // later declarations in the file are harmless: the union check below is what matters, and a + // stray name would show up as uncatalogued rather than being quietly dropped. + const body = source.slice(start); + const names = new Set(); + for (const match of body.matchAll(/^ {2}([a-zA-Z][A-Za-z0-9]*)(<[^\n]*?>)?\(/gm)) { + names.add(match[1]!); + } + return [...names]; +} + +/** + * The source of one method implementation: from its declaration to the next member at the same + * indent. Deliberately not brace-matching — a signature carrying an inline object type makes + * that fiddly, and getting it subtly wrong is how a census ends up reporting that a method has + * no routing call when it has one on the next line. + */ +function methodBody(source: string, method: string): string | undefined { + // The LAST declaration, not the first: an overloaded method leads with bodiless signatures, + // and picking one of those reports the implementation as having no routing call at all. + const declaration = new RegExp(`^ {2}(?:async )?${method}(?:<[^\\n]*?>)?\\(`, "gm"); + const matches = [...source.matchAll(declaration)]; + const start = matches.at(-1)?.index; + if (start === undefined) return undefined; + + const rest = source.slice(start + 3); + const next = rest.search(/^ {2}(?:async )?[a-zA-Z#][A-Za-z0-9]*(?:<[^\n]*?>)?\(/m); + return next === -1 ? rest : rest.slice(0, next); +} + +function catalogued(): { writes: string[]; all: string[] } { + const writes = [...ROUTES_BY_GIVEN_RUN_ID, ...PLACEMENT_SITES.map((s) => s.method)]; + return { writes, all: [...writes, ...READ_ONLY_METHODS] }; +} + +describe("run-store placement census — every write states what it routes by", () => { + it("parses a plausible interface, so a silent parse failure cannot pass the suite", () => { + const methods = interfaceMethods(); + expect(methods.length).toBeGreaterThan(50); + expect(methods).toContain("upsertWaitpointTag"); + expect(methods).toContain("findRun"); + }); + + // The whole point of the census. A method added to `RunStore` is uncatalogued, and + // uncatalogued fails: nobody gets to add a write without saying how it is placed. + it("classifies every interface method as exactly one of read or write", () => { + const methods = interfaceMethods(); + const { all } = catalogued(); + + const uncatalogued = methods.filter((m) => !all.includes(m)).sort(); + expect({ uncatalogued }).toEqual({ uncatalogued: [] }); + + const stale = all.filter((m) => !methods.includes(m)).sort(); + expect({ staleCatalogEntries: stale }).toEqual({ staleCatalogEntries: [] }); + }); + + it("never classifies a method as both a read and a write", () => { + const { writes } = catalogued(); + const both = writes.filter((m) => READ_ONLY_METHODS.includes(m)).sort(); + expect({ classifiedAsBoth: both }).toEqual({ classifiedAsBoth: [] }); + }); + + it("lists no method twice", () => { + const { all } = catalogued(); + const seen = new Set(); + const duplicates = all.filter((m) => (seen.has(m) ? true : (seen.add(m), false))).sort(); + expect({ duplicates }).toEqual({ duplicates: [] }); + }); + + // The forbidden cell. A write that can only name NEW or LEGACY, whose miss produces no + // error, is a row placed on a database its owner does not live on with nothing to detect + // it. `upsertWaitpointTag` sat here and every functional test passed. + it("has no write that routes on residency alone and misses silently", () => { + const forbidden = PLACEMENT_SITES.filter( + (s) => s.basis === "residency" && s.missMode === "silent" + ).map((s) => s.method); + + expect({ residencyOnlySilentWrites: forbidden }).toEqual({ residencyOnlySilentWrites: [] }); + }); + + // `residency` and `fan-out` are both claims about safety rather than mechanisms that + // enforce it, so each one has to be argued in the catalog. Writing that sentence honestly + // for a tag is what would have caught the defect this census exists for. + it("requires a written justification wherever safety is a claim, not a mechanism", () => { + const unjustified = PLACEMENT_SITES.filter( + (s) => (s.basis === "residency" || s.basis === "fan-out") && (s.why ?? "").trim().length < 40 + ).map((s) => s.method); + + expect({ unjustified }).toEqual({ unjustified: [] }); + }); + + it("gives every catalogued write at least one routing expression", () => { + const empty = PLACEMENT_SITES.filter((s) => s.routes.length === 0).map((s) => s.method); + expect({ withoutRoutes: empty }).toEqual({ withoutRoutes: [] }); + }); + + // Anchors the catalog to the source. Weakening a route — dropping the shard hint, swapping + // an id for a residency fallback, renaming a helper — fails here rather than in production. + it.each(PLACEMENT_SITES)("$method still contains the routes the catalog claims", (site) => { + const source = read(STORE); + for (const route of site.routes) { + expect({ method: site.method, route, present: source.includes(route) }).toEqual({ + method: site.method, + route, + present: true, + }); + } + }); + + it.each(ROUTES_BY_GIVEN_RUN_ID)("%s routes on the run id it is given", (method) => { + const body = methodBody(read(STORE), method); + + expect({ method, found: body !== undefined }).toEqual({ method, found: true }); + expect({ method, routedOnGivenRunId: body!.includes(GIVEN_RUN_ID_ROUTE) }).toEqual({ + method, + routedOnGivenRunId: true, + }); + }); +}); diff --git a/internal-packages/run-store/src/placementCatalog.ts b/internal-packages/run-store/src/placementCatalog.ts new file mode 100644 index 00000000000..b7c614e0959 --- /dev/null +++ b/internal-packages/run-store/src/placementCatalog.ts @@ -0,0 +1,275 @@ +// Every method on the `RunStore` interface must appear exactly once below, as a read or as a +// write. `placement.proof.test.ts` diffs this catalog against the interface, so a method added +// to `RunStore` fails the build until somebody classifies it. +// +// Why this exists, and why it is separate from the waitpoint mint census: that census is +// exhaustive over id PRODUCTION and asks "is this id stamped with a shard?". A row with no +// minted id of its own is invisible to it. `WaitpointTag` was exactly that row, and it wrote +// to a gen-1 store for a gen-2 environment while every functional test passed, because the +// read path fans out over every store and found it anyway. This catalog is exhaustive over row +// PLACEMENT instead, and asks a different question of each write: what does it route by? +// +// The one combination that must never exist is a write which routes by nothing better than the +// binary residency hint AND whose miss is silent. A silent miss puts a row on a database its +// owner does not live on, with no error at write time and no symptom at read time. +// +// PURE module: no store import, no Prisma, no env. It is data about the source, checked +// against the source by the proof test. + +/** What the routing decision is made from. */ +type PlacementBasis = + /** The row's own id, which carries its shard. Safe: the row lands where its id says. */ + | "own-id" + /** An owning row's id (a run, a batch). Safe: the row follows its owner. */ + | "owner-id" + /** An explicit shard key passed by the caller, for rows with no routable id at all. */ + | "shard-hint" + /** Partitioned or summed across every store, gen-2 shards included. Safe: nothing to miss. */ + | "fan-out" + /** Nothing but the binary NEW/LEGACY residency hint. Cannot name a gen-2 shard. */ + | "residency"; + +/** + * What happens when a write is routed to a database that does not hold the row. + * + * `loud` — Prisma raises "no record was found for an update" and the caller sees it. Still a + * defect, but a visible one: this is how the gen-2 batch-completion hang was found. + * + * `silent` — the write succeeds against the wrong database. A create or an upsert inserts a + * new row there; an `updateMany` reports zero rows affected, which callers read as "nothing + * to do". Nothing is logged and nothing fails. + */ +type MissMode = "loud" | "silent"; + +export type PlacementSite = { + /** Method name on the `RunStore` interface. */ + method: string; + basis: PlacementBasis; + missMode: MissMode; + /** + * Routing expressions this method's implementation contains, verbatim, as they appear in + * `runOpsStore.ts`. The proof test requires each one to still be present, so weakening a + * route (dropping a shard hint, swapping an id for a residency fallback) fails here first. + * + * A method with several arms lists all of them: the FIRST arm that matches at runtime is + * what routes, so a set that looks safe on its last arm is not evidence of anything. + */ + routes: readonly string[]; + /** Required for `residency` and `fan-out`, where safety is a claim rather than a mechanism. */ + why?: string; +}; + +/** + * The unremarkable majority: a method handed a run id, routing on it. Listed by name rather + * than as 30 identical entries, because 30 identical entries get rubber-stamped in review and + * a census nobody reads is decorative. + */ +export const ROUTES_BY_GIVEN_RUN_ID: readonly string[] = [ + "startAttempt", + "completeAttemptSuccess", + "recordRetryOutcome", + "requeueRun", + "recordBulkActionMembership", + "cancelRun", + "failRunPermanently", + "finalizeRun", + "expireRun", + "lockRunToWorker", + "parkPendingVersion", + "promotePendingVersionRuns", + "expireParkedRun", + "suspendForCheckpoint", + "resumeFromCheckpoint", + "rescheduleRun", + "enqueueDelayedRun", + "rewriteDebouncedRun", + "pushTags", + "pushRealtimeStream", +]; + +/** The shared routing expression every member of the list above contains. */ +export const GIVEN_RUN_ID_ROUTE = "#routeForWrite(runId)"; + +/** Writes whose routing is worth stating one by one. */ +export const PLACEMENT_SITES: readonly PlacementSite[] = [ + { + method: "runInTransaction", + basis: "own-id", + missMode: "loud", + routes: ["#routeOrNew(runId)"], + }, + { + method: "createRun", + basis: "own-id", + missMode: "silent", + routes: ["#routeOrNew(params.data.id)"], + }, + { + method: "createCancelledRun", + basis: "own-id", + missMode: "silent", + routes: ["#routeOrNew(params.data.id)"], + }, + { + method: "createFailedRun", + basis: "own-id", + missMode: "silent", + routes: ["#routeOrNew(params.data.id)"], + }, + { + method: "updateMetadata", + basis: "own-id", + missMode: "loud", + routes: ["#routeOrNewForWrite(runId)"], + }, + { + method: "clearIdempotencyKey", + basis: "fan-out", + missMode: "silent", + routes: ["#route(params.byId.runId)", "#shardStore(NEW_SHARD)", "#shardsExcept(NEW_SHARD)"], + why: "Routes by run id when the caller has one. The predicate arm has no id at all, so it checks NEW and then every remaining store, gen-2 shards included: a key minted before an org flipped still lives on a run in another store, and missing it leaves a stale key deduping forever.", + }, + { + method: "expireRunsBatch", + basis: "fan-out", + missMode: "silent", + routes: ["#fanOutPartitioned(this.#probeOrder, runIds"], + why: "Partitions the id list by shape and calls each store with only its own ids, over the full probe order rather than a gen-1 pair. Nothing is missed because every id is routed individually.", + }, + { + method: "createExecutionSnapshot", + basis: "owner-id", + missMode: "silent", + routes: ["#routeOrNewForWrite(input.run.id)"], + }, + { + method: "createBatchTaskRunItem", + basis: "owner-id", + missMode: "silent", + routes: ["#routeForWrite(data.batchTaskRunId)"], + }, + { + method: "createTaskRunCheckpoint", + basis: "owner-id", + missMode: "silent", + routes: ["#route(ownerRunId)"], + }, + { + method: "blockRunWithWaitpointEdges", + basis: "owner-id", + missMode: "silent", + routes: ["#routeOrNewForWrite(params.runId)"], + }, + { + method: "deleteManyTaskRunWaitpoints", + basis: "owner-id", + missMode: "silent", + routes: [ + "#routeOrNewForWrite(taskRunId)", + "#sumCounts((store) => store.deleteManyTaskRunWaitpoints(args))", + ], + why: "Routes by the owning run id when the filter names one; otherwise sums across every store, so a delete cannot quietly skip a shard.", + }, + { + method: "createBatchTaskRun", + basis: "own-id", + missMode: "silent", + routes: ["#routeForWrite(data.id)"], + }, + { + method: "updateBatchTaskRun", + basis: "own-id", + missMode: "loud", + routes: ["#routeOrNew(id)"], + }, + { + method: "updateManyBatchTaskRun", + basis: "fan-out", + missMode: "silent", + routes: ["#routeOrNew(id)", "#sumCounts((store) => store.updateManyBatchTaskRun(args))"], + why: "Routes by batch id when the filter names one, and otherwise sums across every store. An updateMany reports zero rows rather than failing, so the fan-out is what keeps a filtered update from silently skipping a shard.", + }, + { + method: "updateManyBatchTaskRunItems", + basis: "fan-out", + missMode: "silent", + routes: ["#routeOrNew(id)", "#sumCounts((store) => store.updateManyBatchTaskRunItems(args))"], + why: "Same shape as updateManyBatchTaskRun: id when available, every store otherwise.", + }, + { + method: "createWaitpoint", + basis: "own-id", + missMode: "silent", + routes: ["#waitpointWriteStore("], + why: "Prefers a co-location anchor (the owning run or batch), then the waitpoint's own stamped id, and only then the residency hint. The router refuses an unstamped id against a gen-2 shard, which is what makes the last arm safe to keep.", + }, + { + method: "upsertWaitpoint", + basis: "own-id", + missMode: "silent", + routes: ["#waitpointWriteStore(opts?.coLocateWithRunId, opts?.residency, waitpointId)"], + why: "As createWaitpoint: anchor, then the waitpoint's own id, then residency, with the router refusing an unstamped id on a gen-2 shard.", + }, + { + method: "updateWaitpoint", + basis: "own-id", + missMode: "loud", + routes: ["#resolveWaitpointStore(id)", "#routeOrNew(opts.coLocateWithRunId)"], + why: "The waitpoint's own id wins; the co-location hint is only the fallback for a filter that names no id. Ordering matters here and the arms must stay in this order.", + }, + { + method: "updateManyWaitpoints", + basis: "fan-out", + missMode: "silent", + routes: [ + "#resolveWaitpointStore(id)", + "#sumCounts((store) => store.updateManyWaitpoints(args))", + ], + why: "Routes by waitpoint id when the filter names one, and sums across every store otherwise, because an updateMany that lands on the wrong database reports zero rows instead of failing.", + }, + { + method: "upsertWaitpointTag", + basis: "shard-hint", + missMode: "silent", + routes: ["#shardStore(shardKey)", "#waitpointWriteStore(undefined, residency, data.id)"], + why: "A tag row has no id the router can read and no owning row to follow, so the caller passes the environment's mint shard explicitly. Without that hint this write routes on residency alone, which cannot name a gen-2 shard: the row lands on a gen-1 store while the tokens it describes live on the shard, and because reads fan out the row is still found. That is the defect this catalog was built after.", + }, +]; + +/** + * Reads. Listed only so that the union of reads and writes covers the interface exactly: a new + * method called `getOrCreateThing` would otherwise pass for a read on the strength of its name. + * Read routing is not audited here; a read that probes the wrong store finds nothing and moves + * on, which is a latency and correctness question rather than a placement one. + */ +export const READ_ONLY_METHODS: readonly string[] = [ + "findRun", + "findRunOrThrow", + "findRunOnPrimary", + "findRunOrThrowOnPrimary", + "findRuns", + "findRunsByIds", + "findRunsByIdempotencyKeys", + "findLatestExecutionSnapshot", + "findExecutionSnapshot", + "findManyExecutionSnapshots", + "findSnapshotCompletedWaitpointIds", + "findSnapshotCompletedWaitpointIdsWithPresence", + "findWaitpointConnectedRunIds", + "findWaitpointCompletedSnapshotIds", + "countPendingWaitpoints", + "countPendingWaitpointsWithPresence", + "findWaitpoint", + "findWaitpointOnPrimary", + "findManyWaitpoints", + "forWaitpointCompletion", + "findManyTaskRunWaitpoints", + "findTaskRunAttempt", + "findBatchTaskRunById", + "findBatchTaskRunByFriendlyId", + "findBatchTaskRunByIdempotencyKey", + "countBatchTaskRunItems", + "findManyBatchTaskRunItems", + "findBatchTaskRunItem", + "findManyWaitpointTags", +]; From dca488374daa675f70a11c1070218770872b2101 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 12:22:36 +0100 Subject: [PATCH 20/26] fix(run-store): let a waitpoint's own shard outrank a residency hint The residency hint can only ever name a gen-1 store, so when a waitpoint's own id names a gen-2 shard the hint is not a worse answer, it is an answer it cannot express. It was being checked first, which meant a caller passing both wrote the row to a gen-1 database while its id said otherwise. Silently: a create never misses, and the read path fans out, so the row is still found afterwards. Nothing hit this. The one call site that could withholds the hint deliberately and says why in a comment. That made correctness a convention observed at one site rather than an invariant, so the router now skips the residency arm when the id names a gen-2 shard. The owner anchor still outranks both, and gen-1 ids, cuids and the no-hint path are unchanged: four of the six new tests pass on either precedence, which is what makes them worth keeping. Co-Authored-By: Claude Opus 5 --- .../run-store/src/placementCatalog.ts | 4 +- .../src/runOpsStore.shardMap.test.ts | 73 +++++++++++++++++++ .../run-store/src/runOpsStore.ts | 18 +++-- 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/internal-packages/run-store/src/placementCatalog.ts b/internal-packages/run-store/src/placementCatalog.ts index b7c614e0959..7e0275c4669 100644 --- a/internal-packages/run-store/src/placementCatalog.ts +++ b/internal-packages/run-store/src/placementCatalog.ts @@ -201,14 +201,14 @@ export const PLACEMENT_SITES: readonly PlacementSite[] = [ basis: "own-id", missMode: "silent", routes: ["#waitpointWriteStore("], - why: "Prefers a co-location anchor (the owning run or batch), then the waitpoint's own stamped id, and only then the residency hint. The router refuses an unstamped id against a gen-2 shard, which is what makes the last arm safe to keep.", + why: "Prefers a co-location anchor (the owning run or batch), then the waitpoint's own stamped id, and only then the residency hint. The anchor arm refuses an unstamped id against a gen-2 shard, and the residency arm is skipped entirely when the id names a gen-2 shard, because the hint cannot express that answer.", }, { method: "upsertWaitpoint", basis: "own-id", missMode: "silent", routes: ["#waitpointWriteStore(opts?.coLocateWithRunId, opts?.residency, waitpointId)"], - why: "As createWaitpoint: anchor, then the waitpoint's own id, then residency, with the router refusing an unstamped id on a gen-2 shard.", + why: "As createWaitpoint: anchor, then the waitpoint's own stamped id, then residency. A residency hint never wins over an id naming a gen-2 shard.", }, { method: "updateWaitpoint", diff --git a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts index aa1871b0756..4cda8c1287a 100644 --- a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts +++ b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts @@ -115,6 +115,11 @@ function fakeStore(slot: Slot, log: Call[], config: FakeConfig = {}): FakeStore return Promise.resolve((config.batch ?? null) as never); }) as FakeStore["findBatchTaskRunById"], + upsertWaitpoint: ((args: { create?: { id?: string } }) => { + record("upsertWaitpoint"); + return Promise.resolve((args.create ?? {}) as never); + }) as FakeStore["upsertWaitpoint"], + upsertWaitpointTag: ((data: { name: string }) => { record("upsertWaitpointTag"); return Promise.resolve({ id: `tag_${slot}`, name: data.name } as never); @@ -965,3 +970,71 @@ describe("RoutingRunStore waitpoint tags follow their environment's shard", () = expect(trace(log)).toEqual(["legacy:upsertWaitpointTag"]); }); }); + +describe("RoutingRunStore waitpoint writes: a stamped gen-2 id outranks a residency hint", () => { + // `residency` can only ever say NEW or LEGACY. When the waitpoint's own id names a gen-2 + // shard the hint is not a worse answer, it is an answer that cannot be expressed, so the + // stamped id has to win. Before this, safety rested on every caller knowing to withhold the + // hint for a gen-2 id: one call site did know, and a second one would have written the row + // to a gen-1 database while its id said otherwise, silently, because a create never misses. + const GEN2 = `${"a".repeat(24)}a2`; + const GEN1 = `${"a".repeat(24)}01`; + const CUID = "c".repeat(25); + + const shardedRouter = () => { + const log: Call[] = []; + const router = new RoutingRunStore({ + new: fakeStore("new", log), + legacy: fakeStore("legacy", log), + shards: [{ key: "a", store: fakeStore("a", log) }], + }); + return { router, log }; + }; + + const upsert = (router: RoutingRunStore, id: string, residency?: "NEW" | "LEGACY") => + router.upsertWaitpoint( + { create: { id }, update: {}, where: { id } } as never, + undefined, + residency === undefined ? undefined : ({ residency } as never) + ); + + it("routes to the shard the id names even when the hint says NEW", async () => { + const { router, log } = shardedRouter(); + await upsert(router, GEN2, "NEW"); + expect(trace(log)).toEqual(["a:upsertWaitpoint"]); + }); + + it("routes to the shard the id names even when the hint says LEGACY", async () => { + const { router, log } = shardedRouter(); + await upsert(router, GEN2, "LEGACY"); + expect(trace(log)).toEqual(["a:upsertWaitpoint"]); + }); + + it("still honours the hint for a gen-1 run-ops id, which the hint can express", async () => { + const { router, log } = shardedRouter(); + await upsert(router, GEN1, "NEW"); + expect(trace(log)).toEqual(["new:upsertWaitpoint"]); + }); + + it("still honours the hint for a cuid, and does not read it as a shard", async () => { + const { router, log } = shardedRouter(); + await upsert(router, CUID, "NEW"); + expect(trace(log)).toEqual(["new:upsertWaitpoint"]); + }); + + it("with no hint at all, a gen-1 id still routes by its own shape", async () => { + const { router, log } = shardedRouter(); + await upsert(router, GEN1); + expect(trace(log)).toEqual(["new:upsertWaitpoint"]); + }); + + it("an owning run still outranks both, so a co-located waitpoint follows its run", async () => { + const { router, log } = shardedRouter(); + await router.upsertWaitpoint( + { create: { id: GEN2 }, update: {}, where: { id: GEN2 } } as never, + undefined, + { coLocateWithRunId: GEN2, residency: "NEW" } as never + ); + expect(trace(log)).toEqual(["a:upsertWaitpoint"]); + }); +}); diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index 1a7b6d40719..b388894c620 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -1565,14 +1565,20 @@ export class RoutingRunStore implements RunStore { } return this.#shardStore(key); } - if (residency !== undefined) { + // A gen-2-stamped id names the only database this row can live on, and `residency` can only + // say NEW or LEGACY, so when the two disagree the hint is not a candidate answer — it is + // unable to express one. Let the id win rather than trusting every caller to withhold the + // hint. Unlike the owner arm above, nothing is ambiguous here: that arm throws because a + // mismatch means the mint layer failed and there is no correct destination to fall back to, + // whereas here the correct destination is written on the row itself. + const stamped = typeof waitpointId === "string" ? this.#shardKeyOfSafe(waitpointId) : undefined; + const isGen2Stamped = + stamped !== undefined && stamped !== NEW_SHARD && stamped !== LEGACY_SHARD; + + if (residency !== undefined && !isGen2Stamped) { return this.#shardStore(residency === "NEW" ? NEW_SHARD : LEGACY_SHARD); } - return this.#shardStore( - typeof waitpointId === "string" - ? this.#shardKeyOfSafe(waitpointId) - : this.#idlessWaitpointShard - ); + return this.#shardStore(stamped ?? this.#idlessWaitpointShard); } upsertWaitpoint( From 98b0ff1315e1f356af2b045b4591b57a11c4b197 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 12:56:11 +0100 Subject: [PATCH 21/26] test(run-store): prove waitpoint tag placement against real shard databases A tag row is the one run-ops row with no id the router can read and no owning row to follow, so placement was only covered by fake-store routing assertions. These run on the four-store matrix (legacy, new, and two gen-2 shards, each its own database) and assert where the row physically landed. Two of the four fail without the shard routing; the other two pin the gen-1 path and the read fan-out, which are unchanged, so they pass either way. Co-Authored-By: Claude Opus 5 --- .../src/runOpsStore.nShardMatrix.test.ts | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts index 3f42e36171a..ab76547a9b4 100644 --- a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts +++ b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts @@ -303,3 +303,103 @@ describe("RoutingRunStore four-store matrix — pagination merge", () => { } ); }); + +// A tag row is the one run-ops row with no id the router can read and no owning row to follow. +// The residency hint can only ever name a gen-1 store, so before the caller passed an explicit +// shard key these landed on a gen-1 database while the tokens they describe lived on the shard. +// Nothing failed and nothing was logged: reads fan out over every store, so the row was still +// found afterwards. Only a per-database count can see it, which is why this test is here and not +// in the fake-store suite. +describe("four-store matrix — a waitpoint tag lands on its environment's shard", () => { + matrixTest( + "the shard key routes the tag to shard a, and no gen-1 store receives it", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const env = await seedLegacyEnv(legacyPrisma, "tag_shard_a"); + + await router.upsertWaitpointTag( + { environmentId: env.environmentId, name: "tag-on-a", projectId: env.projectId }, + undefined, + // The residency an environment minting gen-2 ids reports. On its own this names the gen-1 + // NEW store, so it is exactly the value that used to misplace the row. + "NEW", + "a" + ); + + expect(await shardPrismas[0]!.waitpointTag.count({ where: { name: "tag-on-a" } })).toBe(1); + expect(await newPrisma.waitpointTag.count({ where: { name: "tag-on-a" } })).toBe(0); + expect(await legacyPrisma.waitpointTag.count({ where: { name: "tag-on-a" } })).toBe(0); + expect(await shardPrismas[1]!.waitpointTag.count({ where: { name: "tag-on-a" } })).toBe(0); + } + ); + + matrixTest( + "two environments on different shards do not share a database", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const envA = await seedLegacyEnv(legacyPrisma, "tag_two_a"); + const envB = await seedLegacyEnv(legacyPrisma, "tag_two_b"); + + await router.upsertWaitpointTag( + { environmentId: envA.environmentId, name: "shared-name", projectId: envA.projectId }, + undefined, + "NEW", + "a" + ); + await router.upsertWaitpointTag( + { environmentId: envB.environmentId, name: "shared-name", projectId: envB.projectId }, + undefined, + "NEW", + "b" + ); + + // Same tag NAME on both shards, each scoped to its own environment. The unique constraint + // is (environmentId, name), so a collapse onto one database would still insert two rows — + // the failure to catch is placement, not a constraint violation. + const onA = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "shared-name" } }); + const onB = await shardPrismas[1]!.waitpointTag.findMany({ where: { name: "shared-name" } }); + expect(onA.map((r) => r.environmentId)).toEqual([envA.environmentId]); + expect(onB.map((r) => r.environmentId)).toEqual([envB.environmentId]); + expect(await newPrisma.waitpointTag.count({ where: { name: "shared-name" } })).toBe(0); + } + ); + + matrixTest( + "with no shard key the tag still routes by residency, exactly as before", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const env = await seedLegacyEnv(legacyPrisma, "tag_gen1"); + + await router.upsertWaitpointTag( + { environmentId: env.environmentId, name: "tag-gen1", projectId: env.projectId }, + undefined, + "NEW" + ); + + expect(await newPrisma.waitpointTag.count({ where: { name: "tag-gen1" } })).toBe(1); + expect(await shardPrismas[0]!.waitpointTag.count({ where: { name: "tag-gen1" } })).toBe(0); + } + ); + + matrixTest( + "a tag written to a shard is found by the read fan-out", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const env = await seedLegacyEnv(legacyPrisma, "tag_readback"); + + await router.upsertWaitpointTag( + { environmentId: env.environmentId, name: "tag-readback", projectId: env.projectId }, + undefined, + "NEW", + "a" + ); + + // The read side takes no shard hint, so this is what proves the write is still reachable + // through the normal path rather than only by querying the shard directly. + const found = await router.findManyWaitpointTags({ + where: { environmentId: env.environmentId }, + }); + expect(found.map((r) => r.name)).toEqual(["tag-readback"]); + } + ); +}); From e1cafb6c26ba0e1166b2f34059cc3602865a5bdd Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 13:28:19 +0100 Subject: [PATCH 22/26] fix(run-store): list a waitpoint tag once when it exists on more than one store A tag row has no id the router can read, so the same logical tag gets an independent cuid on every store that ever wrote it: the unique index is (environmentId, name) and it is per-database. The read merged by id, so a tag name was returned once per store holding it. An environment that had tags before it was pinned to a shard would see the name listed twice. The merge now keys on (environmentId, name). Dropping a row is safe because nothing consumes a tag's id: a waitpoint carries its tags as a string array and this table is a name registry for listing and autocomplete. Two tests on the four-store matrix: the same name on a gen-1 store and a shard is listed once, and two environments keep their own tag of that name. The second queries without an environment filter on purpose, because a per-environment filter would pass whatever the dedupe key was. Also scopes the placement census to each method's own body. Three creates share one route expression, so a file-wide search passed when one lost its route and a sibling kept it. Co-Authored-By: Claude Opus 5 --- .../run-store/src/placement.proof.test.ts | 13 +++- .../src/runOpsStore.nShardMatrix.test.ts | 71 +++++++++++++++++++ .../run-store/src/runOpsStore.ts | 35 ++++++++- 3 files changed, 116 insertions(+), 3 deletions(-) diff --git a/internal-packages/run-store/src/placement.proof.test.ts b/internal-packages/run-store/src/placement.proof.test.ts index b4b5ec94e46..e1bb97ee987 100644 --- a/internal-packages/run-store/src/placement.proof.test.ts +++ b/internal-packages/run-store/src/placement.proof.test.ts @@ -131,10 +131,19 @@ describe("run-store placement census — every write states what it routes by", // Anchors the catalog to the source. Weakening a route — dropping the shard hint, swapping // an id for a residency fallback, renaming a helper — fails here rather than in production. + // Scoped to the method's own body, not the whole file. Three creates share + // `#routeOrNew(params.data.id)`, so a file-wide search still passes when one of them loses its + // route and a sibling keeps it — which is the exact hole this census exists to close. it.each(PLACEMENT_SITES)("$method still contains the routes the catalog claims", (site) => { - const source = read(STORE); + const body = methodBody(read(STORE), site.method); + + expect({ method: site.method, found: body !== undefined }).toEqual({ + method: site.method, + found: true, + }); + for (const route of site.routes) { - expect({ method: site.method, route, present: source.includes(route) }).toEqual({ + expect({ method: site.method, route, present: body!.includes(route) }).toEqual({ method: site.method, route, present: true, diff --git a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts index ab76547a9b4..4ea13cd1f3b 100644 --- a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts +++ b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts @@ -402,4 +402,75 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard expect(found.map((r) => r.name)).toEqual(["tag-readback"]); } ); + + // The scenario a real rollout produces: an environment has tags, then it is pinned to a shard. + // Its existing tag rows stay on the gen-1 store, and its next write of the SAME name goes to the + // shard with an independent cuid, because the unique index is per-database. Deduping the read by + // id would then list one tag name twice. + matrixTest( + "the same tag name on a gen-1 store and a shard is listed once", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const env = await seedLegacyEnv(legacyPrisma, "tag_dupe"); + + // Before the pin: the tag lands on the gen-1 new store by residency. + await router.upsertWaitpointTag( + { environmentId: env.environmentId, name: "prod", projectId: env.projectId }, + undefined, + "NEW" + ); + // After the pin: the same name lands on shard a, with its own id. + await router.upsertWaitpointTag( + { environmentId: env.environmentId, name: "prod", projectId: env.projectId }, + undefined, + "NEW", + "a" + ); + + // Two physical rows, one per database, with different ids. That is expected and is what the + // per-database unique index permits. + const onNew = await newPrisma.waitpointTag.findMany({ where: { name: "prod" } }); + const onShard = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "prod" } }); + expect(onNew).toHaveLength(1); + expect(onShard).toHaveLength(1); + expect(onNew[0]!.id).not.toBe(onShard[0]!.id); + + // One logical tag through the read path. + const found = await router.findManyWaitpointTags({ + where: { environmentId: env.environmentId }, + }); + expect(found.map((r) => r.name)).toEqual(["prod"]); + } + ); + + matrixTest( + "two environments keep their own tag of the same name", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); + const envA = await seedLegacyEnv(legacyPrisma, "tag_dupe_a"); + const envB = await seedLegacyEnv(legacyPrisma, "tag_dupe_b"); + + await router.upsertWaitpointTag( + { environmentId: envA.environmentId, name: "prod", projectId: envA.projectId }, + undefined, + "NEW", + "a" + ); + await router.upsertWaitpointTag( + { environmentId: envB.environmentId, name: "prod", projectId: envB.projectId }, + undefined, + "NEW", + "b" + ); + + // Queried WITHOUT an environment filter, so both rows reach the merge together. Filtering by + // environmentId per call would hide a name-only dedupe: each result set would hold one row + // and collapse to itself, so the test would pass whatever the key was. + const both = await router.findManyWaitpointTags({ where: { name: "prod" } }); + + expect(both.map((r) => r.environmentId).sort()).toEqual( + [envA.environmentId, envB.environmentId].sort() + ); + } + ); }); diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index b388894c620..c65c22b58c8 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -2266,6 +2266,39 @@ export class RoutingRunStore implements RunStore { return store.upsertWaitpointTag(data, undefined); } + // Merge tag legs, keeping one row per (environmentId, name) rather than per id. + // + // A tag row has no id the router can read, so the SAME logical tag gets an independent cuid on + // every store that ever wrote it: the unique index is `(environmentId, name)` and it is + // per-database. Deduping by id therefore returns the same tag name once per store holding it, + // which surfaces as a name listed two or three times. That was already reachable for a + // dual-resident environment across the gen-1 pair, and stamping the mint shard onto the write + // widens it to every configured shard. + // + // Dropping a row is safe here because nothing consumes a tag's id: a waitpoint carries its tags + // as a string array (`tags: { hasSome: [...] }`) and `WaitpointTag` is a name registry for + // listing and autocomplete. Legs arrive in #precedence order and the last write wins, matching + // #mergeById. A row whose projection omits `environmentId` or `name` cannot be keyed and passes + // through, exactly as #mergeById treats a row with no `id`. + static #mergeTagsByName>( + legs: Array<{ key: ShardKey; rows: R[] }> + ): R[] { + const byName = new Map(); + const passthrough: R[] = []; + for (const { rows } of legs) { + for (const row of rows) { + const environmentId = row.environmentId; + const name = row.name; + if (typeof environmentId !== "string" || typeof name !== "string") { + passthrough.push(row); + continue; + } + byName.set(`${environmentId}\u0000${name}`, row); + } + } + return [...byName.values(), ...passthrough]; + } + // A tag keyed by (environmentId, name) can exist on BOTH DBs for one env (dual-resident, no // id-shape signal), so fan out NEW→LEGACY and de-dupe by id (NEW wins, matching the router's // NEW-wins invariant). take/skip are widened per-leg then re-imposed globally after the merge, @@ -2296,7 +2329,7 @@ export class RoutingRunStore implements RunStore { RoutingRunStore.#ownPrimary(store, client) )) as unknown as Array>, })); - const deduped = this.#mergeById(legs) as unknown as WaitpointTag[]; + const deduped = RoutingRunStore.#mergeTagsByName(legs) as unknown as WaitpointTag[]; const merged = args.orderBy ? (sortByOrderBy( deduped as unknown as Array>, From 3fd8268d5b3df0383fb2b0d6a2cc4d06bc773713 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 13:50:02 +0100 Subject: [PATCH 23/26] fix(run-store): keep the id dedupe when collapsing tags by name The previous commit replaced the tag read's id dedupe with a name dedupe, which dropped an invariant that two tests pin: drain can mirror a tag onto the new store while it keeps its id, so the same id appears on two stores and the new store's copy is authoritative. Keying only on name let a stale mirrored row survive under its old name. Tags need both keys, in order. The id pass resolves a mirrored row and keeps the duplicate alarm. The name pass then collapses the separate case, where a store that never saw the tag minted its own cuid for it, so one logical tag holds a different id per store. Restricting the name pass to rows that survived the id pass stops a stale mirror winning its name back, and filtering rather than rebuilding keeps each winner's position for callers that pass no orderBy. Each pass was verified by removing it: without the id pass the NEW-wins test fails, without the name pass the cross-store duplicate test fails. Co-Authored-By: Claude Opus 5 --- .../run-store/src/runOpsStore.ts | 68 ++++++++++++------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index c65c22b58c8..29b8cd0e46b 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -2266,37 +2266,53 @@ export class RoutingRunStore implements RunStore { return store.upsertWaitpointTag(data, undefined); } - // Merge tag legs, keeping one row per (environmentId, name) rather than per id. + // Tag rows need BOTH dedupe keys, in this order, because two different collisions exist. // - // A tag row has no id the router can read, so the SAME logical tag gets an independent cuid on - // every store that ever wrote it: the unique index is `(environmentId, name)` and it is - // per-database. Deduping by id therefore returns the same tag name once per store holding it, - // which surfaces as a name listed two or three times. That was already reachable for a - // dual-resident environment across the gen-1 pair, and stamping the mint shard onto the write - // widens it to every configured shard. + // By id, first: drain can mirror a tag onto NEW while it keeps its id, so the same id appears on + // two stores and NEW is authoritative. #mergeById owns that, along with the duplicate alarm. // - // Dropping a row is safe here because nothing consumes a tag's id: a waitpoint carries its tags - // as a string array (`tags: { hasSome: [...] }`) and `WaitpointTag` is a name registry for - // listing and autocomplete. Legs arrive in #precedence order and the last write wins, matching - // #mergeById. A row whose projection omits `environmentId` or `name` cannot be keyed and passes - // through, exactly as #mergeById treats a row with no `id`. - static #mergeTagsByName>( - legs: Array<{ key: ShardKey; rows: R[] }> - ): R[] { - const byName = new Map(); - const passthrough: R[] = []; + // By (environmentId, name), second: a tag row has no id the router can read, so a store that has + // never seen the tag mints an independent cuid for it. The unique index is (environmentId, name) + // and it is per-database, so one logical tag can hold a different id on every store, and the id + // pass cannot see that they are the same tag. Left alone, a name is listed once per store holding + // it. That was already reachable across the gen-1 pair for a dual-resident environment, and + // stamping the mint shard onto the write widens it to every configured shard. + // + // Dropping a row is safe because nothing consumes a tag's id: a waitpoint carries its tags as a + // string array (`tags: { hasSome: [...] }`) and this table is a name registry for listing and + // autocomplete. + #mergeTags>(legs: Array<{ key: ShardKey; rows: R[] }>): R[] { + const survivors = this.#mergeById(legs); + const survivorSet = new Set(survivors as R[]); + + // Legs arrive in #precedence order, so the last write wins and the highest-authority store + // takes the name. Restricted to rows that survived the id pass, so a row already dropped as a + // stale mirror cannot win its name back. + const winnerByName = new Map(); for (const { rows } of legs) { for (const row of rows) { - const environmentId = row.environmentId; - const name = row.name; - if (typeof environmentId !== "string" || typeof name !== "string") { - passthrough.push(row); - continue; - } - byName.set(`${environmentId}\u0000${name}`, row); + if (!survivorSet.has(row)) continue; + const key = RoutingRunStore.#tagNameKey(row); + if (key !== undefined) winnerByName.set(key, row); } } - return [...byName.values(), ...passthrough]; + + // Filter rather than rebuild, so a winner keeps the POSITION #mergeById gave it: callers + // observe row order whenever `orderBy` is absent. + return (survivors as R[]).filter((row) => { + const key = RoutingRunStore.#tagNameKey(row); + return key === undefined || winnerByName.get(key) === row; + }); + } + + // A row whose projection omits either field cannot be keyed by name, and passes through — the + // same treatment #mergeById gives a row with no `id`. + static #tagNameKey(row: Record): string | undefined { + const environmentId = row.environmentId; + const name = row.name; + return typeof environmentId === "string" && typeof name === "string" + ? `${environmentId}\u0000${name}` + : undefined; } // A tag keyed by (environmentId, name) can exist on BOTH DBs for one env (dual-resident, no @@ -2329,7 +2345,7 @@ export class RoutingRunStore implements RunStore { RoutingRunStore.#ownPrimary(store, client) )) as unknown as Array>, })); - const deduped = RoutingRunStore.#mergeTagsByName(legs) as unknown as WaitpointTag[]; + const deduped = this.#mergeTags(legs) as unknown as WaitpointTag[]; const merged = args.orderBy ? (sortByOrderBy( deduped as unknown as Array>, From 93c8549c3a4e22529e20b137a9dfaa7c312acf32 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 14:04:49 +0100 Subject: [PATCH 24/26] docs(run-engine): record why the DATETIME waitpoint has no standalone arm Every production caller supplies runId, because the only entry point is the wait.duration route and that is keyed on a run friendly id. A caller that omitted it on a gen-2 environment would mint a cuid and route by residency, putting the row on a gen-1 store while the run waiting on it lives on a shard, and nothing would fail at write time. Comment only. Co-Authored-By: Claude Opus 5 --- .../src/engine/waitpointCoordinator/types.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index 9ee7505f810..f02eb23e96e 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -114,7 +114,17 @@ export type CreateWaitpointResult = | { kind: "created"; waitpoint: Waitpoint }; export type CreateDateTimeWaitpointParams = { - /** When set, the waitpoint co-locates with this run's DB and the dedup probe targets it. */ + /** + * When set, the waitpoint co-locates with this run's DB and the dedup probe targets it. + * + * Optional in the type, but every production caller supplies it: the only entry point is the + * wait.duration route, which is keyed on a run friendly id. There is deliberately NO standalone + * arm here — unlike `createManualWaitpoint`, this type carries no `standaloneShardKey`, so a + * caller that omits `runId` on a gen-2 environment mints a cuid and routes by residency, landing + * the row on a gen-1 store while the run that waits on it lives on a shard. Nothing would fail at + * write time. Before adding a caller that omits `runId`, give this type a shard hint the way the + * MANUAL path has one. + */ runId?: string; projectId: string; environmentId: string; From 81197910a5ddab81b56e99bd3192cc33d0c4ea71 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 15:05:02 +0100 Subject: [PATCH 25/26] refactor: cut non-load-bearing comments from the gen-2 minting work Comments were 15% of the added lines. Removed the ones that restate the code, the duplicates, and the narration, and compressed the rest to the fact each one carries. Now 10%. The largest cuts: a five-line note on standaloneShardKey that appeared verbatim in three files, now once at the type; the batch-completion explanation duplicated between the resolver and its test; and the header essays on the two catalogs. Kept what is not recoverable from reading the code: that a residency hint can name only a gen-1 store, that drain can mirror a tag onto the new store while it keeps its id, that a tag has no id to route by, that the run store's write path has no stamp check, and why each census guard exists. No behaviour change. Co-Authored-By: Claude Opus 5 --- .../app/v3/runEngineHandlersShared.server.ts | 7 +- .../runOpsMigration/gen2MintInertness.test.ts | 9 +-- .../resolveRunMintTarget.server.ts | 14 ++-- apps/webapp/test/runEngineHandlers.test.ts | 23 ++---- .../run-engine/src/engine/index.ts | 15 +--- .../src/engine/systems/waitpointSystem.ts | 7 -- .../legacyPostgresCoordinator.ts | 6 +- .../src/engine/waitpointCoordinator/types.ts | 22 ++---- .../waitpointMint.proof.test.ts | 20 ++--- .../waitpointMintCatalog.ts | 22 ++---- .../waitpointMintSites.test.ts | 13 ++-- .../run-store/src/placement.proof.test.ts | 36 +++------ .../run-store/src/placementCatalog.ts | 73 +++++-------------- .../src/runOpsStore.nShardMatrix.test.ts | 35 +++------ .../src/runOpsStore.shardMap.test.ts | 15 ++-- .../run-store/src/runOpsStore.ts | 45 ++++-------- internal-packages/run-store/src/types.ts | 5 +- packages/build/src/package.json | 3 + packages/core/src/v3/isomorphic/friendlyId.ts | 7 +- .../src/v3/isomorphic/waitpointMint.test.ts | 6 +- .../core/src/v3/isomorphic/waitpointMint.ts | 10 +-- 21 files changed, 126 insertions(+), 267 deletions(-) create mode 100644 packages/build/src/package.json diff --git a/apps/webapp/app/v3/runEngineHandlersShared.server.ts b/apps/webapp/app/v3/runEngineHandlersShared.server.ts index d32dc048ea6..6af0e394211 100644 --- a/apps/webapp/app/v3/runEngineHandlersShared.server.ts +++ b/apps/webapp/app/v3/runEngineHandlersShared.server.ts @@ -87,10 +87,9 @@ export async function resolveBatchRunOpsWriter( shards?: ReadonlyArray<{ key: string; writer: RunOpsPrismaClient }>; } ): Promise { - // A gen-2 batch names its own shard in its id, so route by that and never probe. The - // probe below is binary — NEW, else assume LEGACY — so a gen-2 batch would fall through - // to a store that holds no such row, and the completion update would throw before the - // batch waitpoint could complete, leaving the parent run blocked with nothing logged. + // A gen-2 batch names its shard in its id. The probe below is binary, so without this a gen-2 + // batch resolves to a store holding no such row and the update throws before the batch waitpoint + // completes, leaving the parent blocked with nothing logged. const shardKey = resolveShard(batchId); if (shardKey !== "new" && shardKey !== "legacy") { const shard = deps.shards?.find((s) => s.key === shardKey); diff --git a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts index e51dae720ae..dfb906bfe97 100644 --- a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts @@ -8,9 +8,8 @@ import { import { batchIdForMintKind } from "./mintBatchFriendlyId.server"; import { resolveRunMintTarget } from "./resolveRunMintTarget.server"; -// The gate is off when RUN_OPS_SHARDS is unset OR runOpsMintShardSet is empty. Either way -// resolveMintShard answers "new", so no shard char reaches a MintTarget. Every assertion -// below is "the id is what it was before gen-2 existed". +// The gate is off when RUN_OPS_SHARDS is unset or runOpsMintShardSet is empty; either way +// resolveMintShard answers "new". Every assertion is "the id is what it was before gen-2". const offShard = vi.fn().mockResolvedValue("new" as const); const environment = { organizationId: "org_1", id: "env_1", orgFeatureFlags: {} }; @@ -42,8 +41,8 @@ describe("gate off — run mint paths", () => { }); it("a child of a gen-1 parent keeps the caller's region char", async () => { - // The pre-split code passed the region on BOTH arms, so a child run stamped the - // requested region. Dropping it on the inherited arm would silently stamp the default. + // The pre-split code passed the region on both arms; dropping it on the inherited arm would + // silently stamp the default. const target = await resolveRunMintTarget({ environment, parentRunFriendlyId: `run_${"a".repeat(24)}01`, diff --git a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts index a3f9466788b..6b421ae0d4b 100644 --- a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts @@ -14,11 +14,8 @@ const defaultDeps: RunMintDeps = { }; /** - * Where one run mints. Two stages, and the second runs only for a ROOT run already on the - * run-ops path: a child inherits its parent's shard by id-shape, so a tree never splits. - * - * Every run-mint path routes through here. The branch used to be duplicated per service, - * and one copy had already drifted into minting gen-1 for a gen-2 parent. + * Where one run mints. The second stage runs only for a root run already on the run-ops path: a + * child inherits its parent's shard by id-shape, so a tree never splits across shards. */ export async function resolveRunMintTarget(args: { environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; @@ -27,8 +24,8 @@ export async function resolveRunMintTarget(args: { deps?: Partial; }): Promise { if (args.parentRunFriendlyId) { - // The region still travels: it takes index 24 for an inherited gen-1 parent, exactly as - // it did before this branch. A gen-2 parent's shardChar outranks it. + // The region still travels: it takes index 24 for an inherited gen-1 parent, and a gen-2 + // parent's shardChar outranks it. return { ...resolveInheritedMintKind(args.parentRunFriendlyId), region: args.region }; } @@ -49,8 +46,7 @@ export async function resolveRunMintTarget(args: { orgFeatureFlags: args.environment.orgFeatureFlags, }); - // A reserved key means gen-1, which is the state of every deployment that has configured - // no shard. Only a single-char key names a gen-2 shard. + // A reserved key means gen-1, the state of every deployment with no shard configured. return shard === "new" || shard === "legacy" ? { kind, region: args.region } : { kind, shardChar: shard, region: args.region }; diff --git a/apps/webapp/test/runEngineHandlers.test.ts b/apps/webapp/test/runEngineHandlers.test.ts index 751ef67c2a9..fda077e026c 100644 --- a/apps/webapp/test/runEngineHandlers.test.ts +++ b/apps/webapp/test/runEngineHandlers.test.ts @@ -490,15 +490,11 @@ describe("runEngineHandlers batch completion", () => { }); describe("runEngineHandlers batch residency routing", () => { - // A gen-2 batch lives on its own shard. The binary probe below it looks only on the - // NEW store and then assumes LEGACY, so without a shard arm the completion update runs - // on a database that has no such row: Prisma throws "no record was found for an - // update", the callback dies before tryCompleteBatch, the BATCH waitpoint stays - // PENDING and the parent run waits forever with nothing logged as a hang. - // Real clients on two real databases, so the assertion is where the rows landed rather - // than which object came back. The shard is prisma14 and BOTH gen-1 slots are prisma17: - // every wrong resolution therefore lands on a database that holds no such batch, which is - // the production failure this arm exists to prevent. + // See resolveBatchRunOpsWriter: without a shard arm a gen-2 batch resolves to a store holding + // no such row, and the parent waits forever with nothing logged. + // Real databases, so the assertion is where the rows landed rather than which object came back. + // The shard is prisma14 and both gen-1 slots are prisma17, so every wrong resolution lands on a + // database holding no such batch. heteroPostgresTest( "a gen-2 batch commits on its shard, and the gen-1 store stays empty", async ({ prisma14, prisma17 }) => { @@ -541,8 +537,7 @@ describe("runEngineHandlers batch residency routing", () => { } ); - // Committed on the shard, and the callback survived to run — the hang was the callback - // dying on "no record was found for an update" before it could reach this. + // The hang was the callback dying on "no record was found for an update" before this. const onShard = await prisma14.batchTaskRun.findFirstOrThrow({ where: { id: gen2BatchId } }); expect(onShard.status).toBe("PARTIAL_FAILED"); expect( @@ -550,7 +545,6 @@ describe("runEngineHandlers batch residency routing", () => { ).toHaveLength(1); expect(completed).toBe(gen2BatchId); - // Nothing for this batch reached the gen-1 database. expect(await prisma17.batchTaskRun.findMany({ where: { id: gen2BatchId } })).toHaveLength(0); expect( await prisma17.batchTaskRunError.findMany({ where: { batchTaskRunId: gen2BatchId } }) @@ -558,9 +552,8 @@ describe("runEngineHandlers batch residency routing", () => { } ); - // Kept on a throwing double deliberately: "the NEW store is never probed" is an assertion - // about a call that must not happen, and only a client that throws when touched can make - // that observable. A real client would simply return null and the test would still pass. + // A throwing double deliberately: this asserts a call that must NOT happen, and a real client + // would return null and pass either way. it("a gen-2 batch id never probes the gen-1 store", async () => { const shardWriter = {} as never; diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index cc53cbb7bd1..4a4cdeb99a7 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -1822,13 +1822,6 @@ export class RunEngine { tags?: string[]; /** Standalone-token residency (no owning run) from the env mint kind; ignored when `runId` is set. */ standaloneResidency?: "NEW" | "LEGACY"; - /** - * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the implementation must - * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and - * can only name a gen-1 store, so honouring it would land the row there while its - * completion routes to the shard. Only a Postgres implementation reads this. - */ standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { return this.waitpointSystem.createManualWaitpoint({ @@ -1866,12 +1859,8 @@ export class RunEngine { const waitpoint = await this.runStore.createWaitpoint( { data: { - // Stamped from the BATCH, not the blocked run: this create passes only - // completedByBatchId, so the routing store resolves the owner from the batch and - // validates the stamp against the BATCH's shard. On the normal path the two are - // the same char anyway, because the batch inherited this run's shard. They differ - // only if a batch ever blocks a run from another shard -- and then this is the - // stamp that matches the owner the router actually checks. + // Stamped from the batch, not the blocked run: this create passes only + // completedByBatchId, so that is the owner the router validates the stamp against. ...mintWaitpointIdFor(batchId), type: "BATCH", idempotencyKey: batchId, diff --git a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts index 52d29858b59..9715a89cb9a 100644 --- a/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts +++ b/internal-packages/run-engine/src/engine/systems/waitpointSystem.ts @@ -198,13 +198,6 @@ export class WaitpointSystem { // the token lands on the run-ops DB (NEW) in a fully-minted-new deployment instead of defaulting // to LEGACY by its cuid id-shape. Ignored when `runId` is set (co-location wins). standaloneResidency?: "NEW" | "LEGACY"; - /** - * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the implementation must - * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and - * can only name a gen-1 store, so honouring it would land the row there while its - * completion routes to the shard. Only a Postgres implementation reads this. - */ standaloneShardKey?: ShardKey; }): Promise<{ waitpoint: Waitpoint; isCached: boolean }> { const result = await this.coordinator.createManualWaitpoint({ diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts index da8397a247b..4877075100b 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts @@ -239,8 +239,7 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator // The two `nanoid(24)` calls below are deliberately separate and produce DIFFERENT values: // the upsert `where` key must not match the `create` key, or a guaranteed-miss upsert becomes // a possible update. Do not hoist either to a shared constant. - // The id is stamped for the anchor run's shard, so the waitpoint's own row is routable - // and its completion write needs no probe. A gen-1 or legacy anchor keeps a cuid. + // Stamped for the anchor run's shard, so the row is routable and completion needs no probe. const upsertArgs = { where: { environmentId_idempotencyKey: { @@ -282,8 +281,7 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator // owner, blocked later by whichever run waits on it (possibly cross-DB, resolved by the // run-co-resident block edge + completion fan-out). With no owner it reads the env mint kind via // `standaloneResidency` so a minted-new env keeps its tokens on NEW; unset, it routes by id-shape. No tx here. - // A gen-2 standalone token carries its shard in its own id, so it passes NO hint and lets - // the id route: `residency` outranks the id shape and can only name a gen-1 store. + // A gen-2 standalone token carries its shard in its own id, so it passes no residency hint. const standaloneShard = runId ? undefined : standaloneShardKey; const isGen2Standalone = standaloneShard !== undefined && standaloneShard !== "new" && standaloneShard !== "legacy"; diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index f02eb23e96e..80463b1cd1f 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -25,10 +25,7 @@ export type WaitpointCoordinator = { mintAssociatedWaitpointData(params: { projectId: string; environmentId: string; - /** - * The run this waitpoint belongs to. Its id names the shard the row must land on, and - * this write bypasses the routing store's stamp check, so an unstamped id is silent here. - */ + /** Names the shard the row lands on. This write skips the router's stamp check. */ anchorRunId: string; }): AssociatedWaitpointData; createAssociatedWaitpoint(params: { @@ -117,13 +114,9 @@ export type CreateDateTimeWaitpointParams = { /** * When set, the waitpoint co-locates with this run's DB and the dedup probe targets it. * - * Optional in the type, but every production caller supplies it: the only entry point is the - * wait.duration route, which is keyed on a run friendly id. There is deliberately NO standalone - * arm here — unlike `createManualWaitpoint`, this type carries no `standaloneShardKey`, so a - * caller that omits `runId` on a gen-2 environment mints a cuid and routes by residency, landing - * the row on a gen-1 store while the run that waits on it lives on a shard. Nothing would fail at - * write time. Before adding a caller that omits `runId`, give this type a shard hint the way the - * MANUAL path has one. + * Every production caller supplies it, and there is deliberately no standalone arm. Omitting it + * on a gen-2 environment mints a cuid and lands the row on a gen-1 store, silently. A standalone + * caller needs a shard hint here first, as `createManualWaitpointParams` has. */ runId?: string; projectId: string; @@ -147,11 +140,8 @@ export type CreateManualWaitpointParams = { */ standaloneResidency?: "NEW" | "LEGACY"; /** - * The environment's mint shard, for a STANDALONE token with no owning run. It selects the - * shard the token's id is stamped for. When it names a gen-2 shard the implementation must - * IGNORE `standaloneResidency`: a residency hint outranks the id shape in the router and - * can only name a gen-1 store, so honouring it would land the row there while its - * completion routes to the shard. Only a Postgres implementation reads this. + * The environment's mint shard, for a standalone token with no owning run. When it names a gen-2 + * shard the implementation must ignore `standaloneResidency`, which can only name a gen-1 store. */ standaloneShardKey?: ShardKey; }; diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts index 9e959e8dd30..ebd13aeaec0 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts @@ -21,13 +21,8 @@ function count(source: string, pattern: RegExp): number { return (source.match(pattern) ?? []).length; } -// Every production `.ts` under a root, walked rather than listed: a mint added in a new -// file, or moved back into `systems/` where these all lived until the coordinator seam was -// extracted, has to be visible here or the census is decorative. -// -// Test-support trees are excluded deliberately. A helper that writes a row through raw -// Prisma never reaches the routing store, so it cannot misroute; requiring it to be -// catalogued would fill the census with sites that carry no risk. +// Walked rather than listed, so a mint added in a new file is still visible. Test-support trees +// are excluded: a helper writing through raw Prisma never reaches the routing store. const TEST_SUPPORT_DIRS = new Set(["tests", "__tests__", "fixtures"]); function walk(relativeRoot: string): string[] { @@ -76,9 +71,7 @@ describe("waitpoint mint census — the catalog matches the source", () => { expect(SCANNED).toContain("internal-packages/run-engine/src/engine/systems/waitpointSystem.ts"); }); - // Per EXPRESSION, not per file: this fails for a fifth mint added inside an - // already-catalogued file, AND for a swapped anchor — mintWaitpointIdFor(undefined) in - // place of the run id — which a bare call-count would wave through. + // Per expression, not per file, so a swapped anchor fails too and not just a new site. it.each(SCANNED)("%s has exactly the mint expressions the catalog claims", (file) => { const source = read(file); const expected = expectedMints(file); @@ -90,20 +83,17 @@ describe("waitpoint mint census — the catalog matches the source", () => { }); } - // No mint in the file beyond the ones the catalog accounts for. const accounted = [...expected.values()].reduce((a, b) => a + b, 0); expect(count(source, MINT_CALL)).toBe(accounted); }); it.each(SCANNED)("%s mints no waitpoint id with the un-stamped helper", (file) => { - // The regex matches tokens inside comments too — deliberate. Any textual addition - // forces the census to be reconciled, so a new site cannot land unnoticed. + // Matches inside comments too, deliberately: any textual addition forces a reconcile. expect(count(read(file), UNSTAMPED_MINT)).toBe(0); }); it.each(SCANNED)("%s writes a waitpoint row only if it is catalogued", (file) => { - // A create with NO id is the worst case: Prisma's @default(cuid()) then mints a cuid on - // a gen-2 shard after the write, which no stamp check can see. + // A create with no id is the worst case: @default(cuid()) then mints one after the write. const writes = count(read(file), WAITPOINT_WRITE); const catalogued = WAITPOINT_MINT_SITES.some((s) => s.site === file); expect(writes === 0 || catalogued).toBe(true); diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts index be8c76d72bf..b42d2d4de9f 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts @@ -1,26 +1,20 @@ -// If you add a site that creates a Postgres `Waitpoint` row, add a matching entry here or -// `waitpointMint.proof.test.ts` fails. Entries are one per site, anchored by symbol name, -// never by line number. +// Add a site that creates a Postgres `Waitpoint` row and add an entry here, or +// `waitpointMint.proof.test.ts` fails. One entry per site, anchored by symbol, never by line. // -// Why: a site that mints a cuid for a gen-2 run writes a row the completion path cannot -// find. Three of the sites below fail loudly, because the routing store refuses an -// unstamped id on a gen-2 shard. The RUN row written through `createRun` does NOT — that -// write happens inside the run store, which has no such check — so a missed site there -// strands a parent run with no error. +// A site that mints a cuid for a gen-2 run writes a row the completion path cannot find. Most +// fail loudly, because the router refuses an unstamped id on a gen-2 shard. The RUN row written +// through `createRun` does not: that write is inside the run store, which has no such check. // -// PURE module — no engine import, no env, no Prisma. +// Pure module: no engine import, no env, no Prisma. export type WaitpointMintSite = { id: string; type: "DATETIME" | "MANUAL" | "RUN" | "BATCH"; - /** Repo-relative source path. */ site: string; /** Enclosing method or symbol name — NEVER a line number. */ symbol: string; /** - * The exact mint expressions this site contains, verbatim. The proof test counts each one - * per file, so both a new mint inside an already-catalogued file and a swapped anchor - * (`mintWaitpointIdFor(undefined)` in place of the run id) fail until reconciled here. - * Empty for a site that writes a row from an id minted elsewhere. + * Mint expressions this site contains, verbatim, counted per file, so a new mint and a swapped + * anchor both fail until reconciled. Empty for a site writing an id minted elsewhere. */ mints: readonly string[]; }; diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts index b4f44bb7623..d1f0834d823 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts @@ -4,9 +4,8 @@ import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { describe, expect, it } from "vitest"; import { LegacyPostgresWaitpointCoordinator } from "./legacyPostgresCoordinator.js"; -// These drive the real create sites, not the mint helper. A test that calls the helper with -// a hand-written literal passes even when a site stops passing its anchor, which is the one -// regression that matters here. +// These drive the real create sites, not the mint helper: a test calling the helper directly +// passes even when a site stops passing its anchor. const GEN2_RUN = `${"a".repeat(24)}a2`; const GEN1_RUN = `${"a".repeat(24)}01`; const GEN2_BATCH = `${"d".repeat(24)}b2`; @@ -110,14 +109,13 @@ describe("createManualWaitpoint stamps the anchor's shard", () => { standaloneShardKey: "c", }); - // The run's shard, not the environment's: a co-located waitpoint follows its run. expect(captured.id?.[24]).toBe("a"); }); }); describe("mintAssociatedWaitpointData stamps the anchor's shard", () => { - // The row this mints is written inside the run store, which has no stamp check, so an - // unstamped id here strands the parent run with nothing logged. + // Written inside the run store, which has no stamp check, so an unstamped id here strands the + // parent run with nothing logged. const mint = (anchorRunId: string) => coordinatorCapturing({}).mintAssociatedWaitpointData({ projectId: "proj", @@ -142,8 +140,7 @@ describe("mintAssociatedWaitpointData stamps the anchor's shard", () => { }); it("a batch anchor stamps the batch's shard", () => { - // What blockRunWithCreatedBatch relies on: the router validates a BATCH waitpoint - // against the batch's shard, because the create names only completedByBatchId. + // The create names only completedByBatchId, so that is what the router validates against. expect(mint(GEN2_BATCH).id[24]).toBe("b"); }); }); diff --git a/internal-packages/run-store/src/placement.proof.test.ts b/internal-packages/run-store/src/placement.proof.test.ts index e1bb97ee987..e40e3888424 100644 --- a/internal-packages/run-store/src/placement.proof.test.ts +++ b/internal-packages/run-store/src/placement.proof.test.ts @@ -24,18 +24,16 @@ const TYPES = "internal-packages/run-store/src/types.ts"; const STORE = "internal-packages/run-store/src/runOpsStore.ts"; /** - * Method names declared on the `RunStore` interface, overloads collapsed. Parsed from the - * source rather than imported as a type, because a type-level check cannot fail a build with - * a message naming the method somebody forgot to classify. + * Method names on the `RunStore` interface, overloads collapsed. Parsed from source rather than + * imported as a type, so a failure can name the method somebody forgot to classify. */ function interfaceMethods(): string[] { const source = read(TYPES); const start = source.indexOf("export interface RunStore {"); expect(start).toBeGreaterThan(-1); - // Declarations sit at exactly two-space indent inside the interface. Trailing members of - // later declarations in the file are harmless: the union check below is what matters, and a - // stray name would show up as uncatalogued rather than being quietly dropped. + // Declarations sit at two-space indent. A stray name from later in the file shows up as + // uncatalogued rather than being dropped. const body = source.slice(start); const names = new Set(); for (const match of body.matchAll(/^ {2}([a-zA-Z][A-Za-z0-9]*)(<[^\n]*?>)?\(/gm)) { @@ -45,14 +43,11 @@ function interfaceMethods(): string[] { } /** - * The source of one method implementation: from its declaration to the next member at the same - * indent. Deliberately not brace-matching — a signature carrying an inline object type makes - * that fiddly, and getting it subtly wrong is how a census ends up reporting that a method has - * no routing call when it has one on the next line. + * One method implementation: its declaration to the next member at the same indent. Not + * brace-matching, which a signature carrying an inline object type makes fiddly to get right. */ function methodBody(source: string, method: string): string | undefined { - // The LAST declaration, not the first: an overloaded method leads with bodiless signatures, - // and picking one of those reports the implementation as having no routing call at all. + // The last declaration: an overloaded method leads with bodiless signatures. const declaration = new RegExp(`^ {2}(?:async )?${method}(?:<[^\\n]*?>)?\\(`, "gm"); const matches = [...source.matchAll(declaration)]; const start = matches.at(-1)?.index; @@ -76,8 +71,7 @@ describe("run-store placement census — every write states what it routes by", expect(methods).toContain("findRun"); }); - // The whole point of the census. A method added to `RunStore` is uncatalogued, and - // uncatalogued fails: nobody gets to add a write without saying how it is placed. + // The point of the census: nobody adds a write without saying how it is placed. it("classifies every interface method as exactly one of read or write", () => { const methods = interfaceMethods(); const { all } = catalogued(); @@ -102,8 +96,7 @@ describe("run-store placement census — every write states what it routes by", expect({ duplicates }).toEqual({ duplicates: [] }); }); - // The forbidden cell. A write that can only name NEW or LEGACY, whose miss produces no - // error, is a row placed on a database its owner does not live on with nothing to detect + // The forbidden cell: a row on a database its owner does not live on, with nothing to detect // it. `upsertWaitpointTag` sat here and every functional test passed. it("has no write that routes on residency alone and misses silently", () => { const forbidden = PLACEMENT_SITES.filter( @@ -113,9 +106,7 @@ describe("run-store placement census — every write states what it routes by", expect({ residencyOnlySilentWrites: forbidden }).toEqual({ residencyOnlySilentWrites: [] }); }); - // `residency` and `fan-out` are both claims about safety rather than mechanisms that - // enforce it, so each one has to be argued in the catalog. Writing that sentence honestly - // for a tag is what would have caught the defect this census exists for. + // Both are claims about safety rather than mechanisms, so each has to be argued in the catalog. it("requires a written justification wherever safety is a claim, not a mechanism", () => { const unjustified = PLACEMENT_SITES.filter( (s) => (s.basis === "residency" || s.basis === "fan-out") && (s.why ?? "").trim().length < 40 @@ -129,11 +120,8 @@ describe("run-store placement census — every write states what it routes by", expect({ withoutRoutes: empty }).toEqual({ withoutRoutes: [] }); }); - // Anchors the catalog to the source. Weakening a route — dropping the shard hint, swapping - // an id for a residency fallback, renaming a helper — fails here rather than in production. - // Scoped to the method's own body, not the whole file. Three creates share - // `#routeOrNew(params.data.id)`, so a file-wide search still passes when one of them loses its - // route and a sibling keeps it — which is the exact hole this census exists to close. + // Scoped to the method's own body, not the whole file: three creates share + // `#routeOrNew(params.data.id)`, so a file-wide search passes when one loses its route. it.each(PLACEMENT_SITES)("$method still contains the routes the catalog claims", (site) => { const body = methodBody(read(STORE), site.method); diff --git a/internal-packages/run-store/src/placementCatalog.ts b/internal-packages/run-store/src/placementCatalog.ts index 7e0275c4669..0e88ef707c1 100644 --- a/internal-packages/run-store/src/placementCatalog.ts +++ b/internal-packages/run-store/src/placementCatalog.ts @@ -1,69 +1,39 @@ -// Every method on the `RunStore` interface must appear exactly once below, as a read or as a -// write. `placement.proof.test.ts` diffs this catalog against the interface, so a method added -// to `RunStore` fails the build until somebody classifies it. +// Every method on the `RunStore` interface appears exactly once below, as a read or as a write. +// `placement.proof.test.ts` diffs this catalog against the interface, so a new method fails the +// build until somebody classifies it. // -// Why this exists, and why it is separate from the waitpoint mint census: that census is -// exhaustive over id PRODUCTION and asks "is this id stamped with a shard?". A row with no -// minted id of its own is invisible to it. `WaitpointTag` was exactly that row, and it wrote -// to a gen-1 store for a gen-2 environment while every functional test passed, because the -// read path fans out over every store and found it anyway. This catalog is exhaustive over row -// PLACEMENT instead, and asks a different question of each write: what does it route by? +// The waitpoint mint census is exhaustive over id production and cannot see a row with no minted +// id, which is how `WaitpointTag` wrote to a gen-1 store for a gen-2 environment with every +// functional test passing. This is exhaustive over placement instead: what does each write route +// by? The combination that must never exist is residency-only routing with a silent miss. // -// The one combination that must never exist is a write which routes by nothing better than the -// binary residency hint AND whose miss is silent. A silent miss puts a row on a database its -// owner does not live on, with no error at write time and no symptom at read time. -// -// PURE module: no store import, no Prisma, no env. It is data about the source, checked -// against the source by the proof test. +// Pure module: no store import, no Prisma, no env. -/** What the routing decision is made from. */ -type PlacementBasis = - /** The row's own id, which carries its shard. Safe: the row lands where its id says. */ - | "own-id" - /** An owning row's id (a run, a batch). Safe: the row follows its owner. */ - | "owner-id" - /** An explicit shard key passed by the caller, for rows with no routable id at all. */ - | "shard-hint" - /** Partitioned or summed across every store, gen-2 shards included. Safe: nothing to miss. */ - | "fan-out" - /** Nothing but the binary NEW/LEGACY residency hint. Cannot name a gen-2 shard. */ - | "residency"; +/** What the routing decision is made from. `residency` cannot name a gen-2 shard. */ +type PlacementBasis = "own-id" | "owner-id" | "shard-hint" | "fan-out" | "residency"; /** - * What happens when a write is routed to a database that does not hold the row. - * - * `loud` — Prisma raises "no record was found for an update" and the caller sees it. Still a - * defect, but a visible one: this is how the gen-2 batch-completion hang was found. - * - * `silent` — the write succeeds against the wrong database. A create or an upsert inserts a - * new row there; an `updateMany` reports zero rows affected, which callers read as "nothing - * to do". Nothing is logged and nothing fails. + * `loud` — Prisma raises "no record was found for an update" and the caller sees it. + * `silent` — the write succeeds on the wrong database. A create inserts a row there; an + * `updateMany` reports zero rows affected, which callers read as "nothing to do". */ type MissMode = "loud" | "silent"; export type PlacementSite = { - /** Method name on the `RunStore` interface. */ method: string; basis: PlacementBasis; missMode: MissMode; /** - * Routing expressions this method's implementation contains, verbatim, as they appear in - * `runOpsStore.ts`. The proof test requires each one to still be present, so weakening a - * route (dropping a shard hint, swapping an id for a residency fallback) fails here first. - * - * A method with several arms lists all of them: the FIRST arm that matches at runtime is - * what routes, so a set that looks safe on its last arm is not evidence of anything. + * Routing expressions the implementation contains, verbatim. The proof test requires each to + * still be present, so weakening a route fails here first. List every arm: the first arm that + * matches is what routes, so a set that looks safe on its last arm proves nothing. */ routes: readonly string[]; /** Required for `residency` and `fan-out`, where safety is a claim rather than a mechanism. */ why?: string; }; -/** - * The unremarkable majority: a method handed a run id, routing on it. Listed by name rather - * than as 30 identical entries, because 30 identical entries get rubber-stamped in review and - * a census nobody reads is decorative. - */ +/** Handed a run id, routing on it. Listed by name; 20 identical entries would be rubber-stamped. */ export const ROUTES_BY_GIVEN_RUN_ID: readonly string[] = [ "startAttempt", "completeAttemptSuccess", @@ -87,10 +57,8 @@ export const ROUTES_BY_GIVEN_RUN_ID: readonly string[] = [ "pushRealtimeStream", ]; -/** The shared routing expression every member of the list above contains. */ export const GIVEN_RUN_ID_ROUTE = "#routeForWrite(runId)"; -/** Writes whose routing is worth stating one by one. */ export const PLACEMENT_SITES: readonly PlacementSite[] = [ { method: "runInTransaction", @@ -237,10 +205,9 @@ export const PLACEMENT_SITES: readonly PlacementSite[] = [ ]; /** - * Reads. Listed only so that the union of reads and writes covers the interface exactly: a new - * method called `getOrCreateThing` would otherwise pass for a read on the strength of its name. - * Read routing is not audited here; a read that probes the wrong store finds nothing and moves - * on, which is a latency and correctness question rather than a placement one. + * Reads, listed only so the union covers the interface exactly: a new method named + * `getOrCreateThing` would otherwise pass for a read on the strength of its name. Read routing is + * not audited here, because a read that probes the wrong store finds nothing and moves on. */ export const READ_ONLY_METHODS: readonly string[] = [ "findRun", diff --git a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts index 4ea13cd1f3b..ec8ecb20dde 100644 --- a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts +++ b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts @@ -304,12 +304,9 @@ describe("RoutingRunStore four-store matrix — pagination merge", () => { ); }); -// A tag row is the one run-ops row with no id the router can read and no owning row to follow. -// The residency hint can only ever name a gen-1 store, so before the caller passed an explicit -// shard key these landed on a gen-1 database while the tokens they describe lived on the shard. -// Nothing failed and nothing was logged: reads fan out over every store, so the row was still -// found afterwards. Only a per-database count can see it, which is why this test is here and not -// in the fake-store suite. +// A tag has no id to route by and no owning row to follow, and `residency` names only a gen-1 +// store. Nothing fails when the row is misplaced, because reads fan out and find it anyway, so +// only a per-database count can see it. Hence container tests rather than the fake-store suite. describe("four-store matrix — a waitpoint tag lands on its environment's shard", () => { matrixTest( "the shard key routes the tag to shard a, and no gen-1 store receives it", @@ -353,9 +350,8 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "b" ); - // Same tag NAME on both shards, each scoped to its own environment. The unique constraint - // is (environmentId, name), so a collapse onto one database would still insert two rows — - // the failure to catch is placement, not a constraint violation. + // The unique constraint is per-database, so a collapse onto one database still inserts two + // rows. The failure to catch is placement, not a constraint violation. const onA = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "shared-name" } }); const onB = await shardPrismas[1]!.waitpointTag.findMany({ where: { name: "shared-name" } }); expect(onA.map((r) => r.environmentId)).toEqual([envA.environmentId]); @@ -394,8 +390,7 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "a" ); - // The read side takes no shard hint, so this is what proves the write is still reachable - // through the normal path rather than only by querying the shard directly. + // The read takes no shard hint, so this proves the write is reachable the normal way. const found = await router.findManyWaitpointTags({ where: { environmentId: env.environmentId }, }); @@ -403,23 +398,20 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard } ); - // The scenario a real rollout produces: an environment has tags, then it is pinned to a shard. - // Its existing tag rows stay on the gen-1 store, and its next write of the SAME name goes to the - // shard with an independent cuid, because the unique index is per-database. Deduping the read by - // id would then list one tag name twice. + // What a real rollout produces: an environment has tags, then it is pinned. Its old rows stay on + // the gen-1 store and the same name goes to the shard with its own cuid, so an id-keyed dedupe + // would list the name twice. matrixTest( "the same tag name on a gen-1 store and a shard is listed once", async ({ legacyPrisma, newPrisma, shardPrismas }) => { const router = makeMatrixRouter(legacyPrisma, newPrisma, shardPrismas); const env = await seedLegacyEnv(legacyPrisma, "tag_dupe"); - // Before the pin: the tag lands on the gen-1 new store by residency. await router.upsertWaitpointTag( { environmentId: env.environmentId, name: "prod", projectId: env.projectId }, undefined, "NEW" ); - // After the pin: the same name lands on shard a, with its own id. await router.upsertWaitpointTag( { environmentId: env.environmentId, name: "prod", projectId: env.projectId }, undefined, @@ -427,15 +419,13 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "a" ); - // Two physical rows, one per database, with different ids. That is expected and is what the - // per-database unique index permits. + // Two physical rows with different ids, which the per-database unique index permits. const onNew = await newPrisma.waitpointTag.findMany({ where: { name: "prod" } }); const onShard = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "prod" } }); expect(onNew).toHaveLength(1); expect(onShard).toHaveLength(1); expect(onNew[0]!.id).not.toBe(onShard[0]!.id); - // One logical tag through the read path. const found = await router.findManyWaitpointTags({ where: { environmentId: env.environmentId }, }); @@ -463,9 +453,8 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "b" ); - // Queried WITHOUT an environment filter, so both rows reach the merge together. Filtering by - // environmentId per call would hide a name-only dedupe: each result set would hold one row - // and collapse to itself, so the test would pass whatever the key was. + // No environment filter, so both rows reach the merge together. Filtering per call would + // hide a name-only dedupe: each result set would hold one row and collapse to itself. const both = await router.findManyWaitpointTags({ where: { name: "prod" } }); expect(both.map((r) => r.environmentId).sort()).toEqual( diff --git a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts index 4cda8c1287a..3cb2253c06b 100644 --- a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts +++ b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts @@ -936,10 +936,9 @@ describe("RoutingRunStore batch probe tolerates legitimate dual-residency", () = }); describe("RoutingRunStore waitpoint tags follow their environment's shard", () => { - // A tag row carries no id the router can read, and `residency` only ever names a gen-1 store. - // Without the shard hint an environment's tags land on a different database from the tokens - // they describe. Reads fan out over every store, so the row is still found later: the symptom - // is a tag attributed to the wrong database, not an error, which is why this needs a test. + // A tag has no id to route by and `residency` names only a gen-1 store, so without the hint the + // row lands on a different database from the tokens it describes. Reads fan out and still find + // it, so the symptom is placement rather than an error. const tag = { environmentId: "env_1", name: "tag", projectId: "proj_1" }; const shardedRouter = () => { @@ -972,11 +971,9 @@ describe("RoutingRunStore waitpoint tags follow their environment's shard", () = }); describe("RoutingRunStore waitpoint writes: a stamped gen-2 id outranks a residency hint", () => { - // `residency` can only ever say NEW or LEGACY. When the waitpoint's own id names a gen-2 - // shard the hint is not a worse answer, it is an answer that cannot be expressed, so the - // stamped id has to win. Before this, safety rested on every caller knowing to withhold the - // hint for a gen-2 id: one call site did know, and a second one would have written the row - // to a gen-1 database while its id said otherwise, silently, because a create never misses. + // `residency` names only NEW or LEGACY, so a stamped gen-2 id has to win. Before this, safety + // rested on every caller withholding the hint for a gen-2 id; a caller that passed both would + // have written the row to a gen-1 database silently, because a create never misses. const GEN2 = `${"a".repeat(24)}a2`; const GEN1 = `${"a".repeat(24)}01`; const CUID = "c".repeat(25); diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index 29b8cd0e46b..31a58c74f8e 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -1565,12 +1565,8 @@ export class RoutingRunStore implements RunStore { } return this.#shardStore(key); } - // A gen-2-stamped id names the only database this row can live on, and `residency` can only - // say NEW or LEGACY, so when the two disagree the hint is not a candidate answer — it is - // unable to express one. Let the id win rather than trusting every caller to withhold the - // hint. Unlike the owner arm above, nothing is ambiguous here: that arm throws because a - // mismatch means the mint layer failed and there is no correct destination to fall back to, - // whereas here the correct destination is written on the row itself. + // A gen-2-stamped id names the only database this row can live on, and `residency` can name + // only NEW or LEGACY, so the id wins rather than every caller having to withhold the hint. const stamped = typeof waitpointId === "string" ? this.#shardKeyOfSafe(waitpointId) : undefined; const isGen2Stamped = stamped !== undefined && stamped !== NEW_SHARD && stamped !== LEGACY_SHARD; @@ -2255,10 +2251,8 @@ export class RoutingRunStore implements RunStore { // No owning run; route by the env's residency hint when present, else a minted id-shape, else // fall back to LEGACY (same precedence as a standalone waitpoint). Caller tx is never forwarded. // - // A gen-2 shard hint wins outright. Callers never mint a tag id, so id-shape cannot route one, - // and `residency` collapses to a gen-1 store — which would leave an environment's tags on a - // different database from the tokens they describe. The read side already fans out over every - // store, so a misplaced row is found but attributed to the wrong environment's database. + // A gen-2 shard hint wins outright: a tag has no id to route by, and `residency` names only a + // gen-1 store, which would leave the row on a different database from the tokens it describes. const store = shardKey !== undefined && shardKey !== NEW_SHARD && shardKey !== LEGACY_SHARD ? this.#shardStore(shardKey) @@ -2266,28 +2260,19 @@ export class RoutingRunStore implements RunStore { return store.upsertWaitpointTag(data, undefined); } - // Tag rows need BOTH dedupe keys, in this order, because two different collisions exist. + // Two collisions exist, so both keys are needed in this order. By id first: drain can mirror a + // tag onto NEW while it keeps its id, and NEW is authoritative. By (environmentId, name) second: + // the unique index is per-database, so a store that never saw the tag minted its own cuid for it + // and the id pass cannot tell they are one tag. // - // By id, first: drain can mirror a tag onto NEW while it keeps its id, so the same id appears on - // two stores and NEW is authoritative. #mergeById owns that, along with the duplicate alarm. - // - // By (environmentId, name), second: a tag row has no id the router can read, so a store that has - // never seen the tag mints an independent cuid for it. The unique index is (environmentId, name) - // and it is per-database, so one logical tag can hold a different id on every store, and the id - // pass cannot see that they are the same tag. Left alone, a name is listed once per store holding - // it. That was already reachable across the gen-1 pair for a dual-resident environment, and - // stamping the mint shard onto the write widens it to every configured shard. - // - // Dropping a row is safe because nothing consumes a tag's id: a waitpoint carries its tags as a - // string array (`tags: { hasSome: [...] }`) and this table is a name registry for listing and - // autocomplete. + // Dropping a row is safe because nothing reads a tag's id: a waitpoint holds its tags as a string + // array and this table is a name registry. #mergeTags>(legs: Array<{ key: ShardKey; rows: R[] }>): R[] { const survivors = this.#mergeById(legs); const survivorSet = new Set(survivors as R[]); - // Legs arrive in #precedence order, so the last write wins and the highest-authority store - // takes the name. Restricted to rows that survived the id pass, so a row already dropped as a - // stale mirror cannot win its name back. + // Legs arrive in #precedence order, so the last write wins. Restricted to id-pass survivors so + // a stale mirror cannot win its name back. const winnerByName = new Map(); for (const { rows } of legs) { for (const row of rows) { @@ -2297,16 +2282,14 @@ export class RoutingRunStore implements RunStore { } } - // Filter rather than rebuild, so a winner keeps the POSITION #mergeById gave it: callers - // observe row order whenever `orderBy` is absent. + // Filter rather than rebuild: a winner keeps the position #mergeById gave it, which callers + // observe when `orderBy` is absent. return (survivors as R[]).filter((row) => { const key = RoutingRunStore.#tagNameKey(row); return key === undefined || winnerByName.get(key) === row; }); } - // A row whose projection omits either field cannot be keyed by name, and passes through — the - // same treatment #mergeById gives a row with no `id`. static #tagNameKey(row: Record): string | undefined { const environmentId = row.environmentId; const name = row.name; diff --git a/internal-packages/run-store/src/types.ts b/internal-packages/run-store/src/types.ts index 1b1ee1e1ee8..036b844bdf6 100644 --- a/internal-packages/run-store/src/types.ts +++ b/internal-packages/run-store/src/types.ts @@ -959,9 +959,8 @@ export interface RunStore { // minted-new env's tags read this residency (NEW) so they land with the env's tokens/runs // instead of defaulting to LEGACY. Single-store impls ignore it. residency?: Residency, - // The environment's gen-2 mint shard, when it has one. A tag carries no id the router can - // read, so this is the only way its row can follow its environment's tokens onto a shard. - // Takes precedence over `residency`, which can only ever name a gen-1 store. + // The environment's gen-2 mint shard. A tag has no id to route by, so this is the only way its + // row follows its environment's tokens onto a shard. Outranks `residency`. shardKey?: ShardKey ): Promise; findManyWaitpointTags( diff --git a/packages/build/src/package.json b/packages/build/src/package.json new file mode 100644 index 00000000000..3dbc1ca591c --- /dev/null +++ b/packages/build/src/package.json @@ -0,0 +1,3 @@ +{ + "type": "module" +} diff --git a/packages/core/src/v3/isomorphic/friendlyId.ts b/packages/core/src/v3/isomorphic/friendlyId.ts index c471ce9aa31..7c78ae46b93 100644 --- a/packages/core/src/v3/isomorphic/friendlyId.ts +++ b/packages/core/src/v3/isomorphic/friendlyId.ts @@ -229,10 +229,9 @@ export function isRunOpsIdBody(body: string): boolean { return parseRunOpsIdBody(body) !== undefined; } -// Shape-only core check: exactly the alphabet base32hexDecode accepts, without decoding. -// RUN_OPS_ID_ALPHABET is [0-9a-v], so this and "the decode would not throw" are the same -// predicate. Routing only needs the shape; decoding the core to recover a timestamp and -// then discarding it costs ~30x more, on a path the router takes for every routed call. +// Shape-only check over the same alphabet base32hexDecode accepts, so it is the same predicate as +// "the decode would not throw". Routing needs the shape only, and decoding a timestamp to discard +// it costs ~30x more on a path taken for every routed call. const RUN_OPS_ID_CORE_PATTERN = /^[0-9a-v]{24}$/; /** Shape-only v1 body check for routing: 26 chars, version "1", region and core in range. */ diff --git a/packages/core/src/v3/isomorphic/waitpointMint.test.ts b/packages/core/src/v3/isomorphic/waitpointMint.test.ts index 5f9bd76bd48..8d35a7248ad 100644 --- a/packages/core/src/v3/isomorphic/waitpointMint.test.ts +++ b/packages/core/src/v3/isomorphic/waitpointMint.test.ts @@ -76,10 +76,8 @@ describe("mintWaitpointIdFor", () => { }); describe("resolveShard shape checks match the decoding parsers", () => { - // resolveShard used to decode the 24-char core to recover a timestamp and then discard - // it, which cost ~10x a shape check on the router's hot path. The alphabet is [0-9a-v], - // so "the shape matches" and "the decode would not throw" are the same predicate. These - // pin that equivalence, because a drift here misroutes rather than erroring. + // The alphabet is [0-9a-v], so "the shape matches" and "the decode would not throw" are the + // same predicate. These pin that equivalence: a drift misroutes rather than erroring. const classifyByDecode = (body: string): string => { const genTwo = parseRunOpsIdV2Body(body); if (genTwo) return genTwo.shard; diff --git a/packages/core/src/v3/isomorphic/waitpointMint.ts b/packages/core/src/v3/isomorphic/waitpointMint.ts index d05d7b60f43..4ce08dfaaa6 100644 --- a/packages/core/src/v3/isomorphic/waitpointMint.ts +++ b/packages/core/src/v3/isomorphic/waitpointMint.ts @@ -1,9 +1,8 @@ import { generateRunOpsIdV2, WaitpointId } from "./friendlyId.js"; import { resolveShard, type ShardKey } from "./runOpsResidency.js"; -// A waitpoint id for a Postgres shard — NOT the Redis store format (type char at index -// 24, version "w"), which has no Postgres row to route. The core is always fresh: reusing -// the anchor's would produce a body identical to the run's own id. +// A Postgres waitpoint id, NOT the Redis store format (version "w" at index 25), which has no +// Postgres row to route. The core is always fresh, or the body would equal the anchor's own id. export function mintWaitpointIdForShard(key: ShardKey): { id: string; friendlyId: string } { if (key === "new" || key === "legacy") { return WaitpointId.generate(); @@ -13,9 +12,8 @@ export function mintWaitpointIdForShard(key: ShardKey): { id: string; friendlyId return { id, friendlyId: WaitpointId.toFriendlyId(id) }; } -// Every Postgres waitpoint mint goes through here, in the webapp and the engine alike: -// the router refuses a waitpoint whose id is not stamped for the shard it lands on. -// A gen-1 or legacy anchor keeps a cuid. +// Every Postgres waitpoint mint goes through here: the router refuses an id that is not stamped +// for the shard it lands on. A gen-1 or legacy anchor keeps a cuid. export function mintWaitpointIdFor(anchorId: string | undefined): { id: string; friendlyId: string; From f5a9ac1b2f538a196b16c1c37f5f4836a08d7fe0 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 15:29:58 +0100 Subject: [PATCH 26/26] refactor: cut a further 98 comment lines from the gen-2 minting work Deleted rather than reworded this time. The cuts fall into three groups: facts stated in a production file and repeated in its test, field docs that restate the field's own name or return type, and test comments that repeat what the test name already says. Comment lines added by this branch: 191 down to 93. Comment-only, verified by diffing out every commented line and finding nothing left. Co-Authored-By: Claude Opus 5 --- .../app/routes/api.v1.waitpoints.tokens.ts | 3 +- .../app/v3/runEngineHandlersShared.server.ts | 5 ++- .../runOpsMigration/gen2MintInertness.test.ts | 3 +- .../mintAnchoredRunFriendlyId.server.ts | 4 +-- .../mintBatchFriendlyId.server.ts | 3 +- .../resolveRunMintTarget.server.ts | 9 ++--- .../runOpsMigration/runOpsMintShard.server.ts | 5 ++- .../app/v3/services/batchTriggerV3.server.ts | 5 ++- apps/webapp/test/runEngineHandlers.test.ts | 7 ++-- .../run-engine/src/engine/index.ts | 4 +-- .../legacyPostgresCoordinator.ts | 6 ++-- .../src/engine/waitpointCoordinator/types.ts | 15 +++----- .../waitpointMint.proof.test.ts | 14 ++++---- .../waitpointMintCatalog.ts | 22 ++++-------- .../waitpointMintSites.test.ts | 7 ++-- .../run-store/src/placement.proof.test.ts | 21 +++-------- .../run-store/src/placementCatalog.ts | 36 +++++-------------- .../src/runOpsStore.nShardMatrix.test.ts | 20 ++++------- .../src/runOpsStore.shardMap.test.ts | 7 +--- .../run-store/src/runOpsStore.ts | 21 ++++------- internal-packages/run-store/src/types.ts | 4 +-- packages/core/src/v3/isomorphic/friendlyId.ts | 7 ++-- .../src/v3/isomorphic/waitpointMint.test.ts | 3 +- .../core/src/v3/isomorphic/waitpointMint.ts | 7 ++-- 24 files changed, 70 insertions(+), 168 deletions(-) diff --git a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts index 92e49a001b4..f67f0860f2b 100644 --- a/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts +++ b/apps/webapp/app/routes/api.v1.waitpoints.tokens.ts @@ -70,8 +70,7 @@ const { action } = createActionApiRoute( }); const residency = mintKind === "runOpsId" ? "NEW" : "LEGACY"; - // The token's id is minted inside the engine, so the shard travels with the call. No - // extra query: the org flags this reads are already loaded on the authenticated env. + // No extra query: the org flags are already loaded on the authenticated env. const standaloneShardKey = mintKind === "runOpsId" ? await resolveMintShard({ diff --git a/apps/webapp/app/v3/runEngineHandlersShared.server.ts b/apps/webapp/app/v3/runEngineHandlersShared.server.ts index 6af0e394211..a20d531ba7e 100644 --- a/apps/webapp/app/v3/runEngineHandlersShared.server.ts +++ b/apps/webapp/app/v3/runEngineHandlersShared.server.ts @@ -87,9 +87,8 @@ export async function resolveBatchRunOpsWriter( shards?: ReadonlyArray<{ key: string; writer: RunOpsPrismaClient }>; } ): Promise { - // A gen-2 batch names its shard in its id. The probe below is binary, so without this a gen-2 - // batch resolves to a store holding no such row and the update throws before the batch waitpoint - // completes, leaving the parent blocked with nothing logged. + // The probe below is binary, so without this a gen-2 batch resolves to a store holding no such + // row, and the update throws before the batch waitpoint completes. const shardKey = resolveShard(batchId); if (shardKey !== "new" && shardKey !== "legacy") { const shard = deps.shards?.find((s) => s.key === shardKey); diff --git a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts index dfb906bfe97..3485115ea97 100644 --- a/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts @@ -8,8 +8,7 @@ import { import { batchIdForMintKind } from "./mintBatchFriendlyId.server"; import { resolveRunMintTarget } from "./resolveRunMintTarget.server"; -// The gate is off when RUN_OPS_SHARDS is unset or runOpsMintShardSet is empty; either way -// resolveMintShard answers "new". Every assertion is "the id is what it was before gen-2". +// Gate off means resolveMintShard answers "new". Every assertion is "the id is what it was". const offShard = vi.fn().mockResolvedValue("new" as const); const environment = { organizationId: "org_1", id: "env_1", orgFeatureFlags: {} }; diff --git a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts index d3de7bf8cb4..3adbc6e7321 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintAnchoredRunFriendlyId.server.ts @@ -2,9 +2,7 @@ import { generateRunOpsId, generateRunOpsIdV2, RunId } from "@trigger.dev/core/v import type { MintTarget } from "./mintTarget"; import { resolveInheritedMintKind } from "./resolveInheritedMintKind.server"; -// Shared id-generation branch for every run-mint path: "runOpsId" -> a dedicated store, -// "cuid" -> LEGACY. A shardChar selects one gen-2 shard and takes index 24; without one, -// the region takes that slot exactly as it does today. +// A shardChar selects one gen-2 shard and takes index 24; without one the region takes that slot. export function mintFriendlyIdForKind(target: MintTarget): string { if (target.kind !== "runOpsId") { return RunId.generate().friendlyId; diff --git a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts index b08d9b9b33f..088eaed5c09 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintBatchFriendlyId.server.ts @@ -14,8 +14,7 @@ export function batchIdForMintKind(target: MintTarget): { id: string; friendlyId return { id, friendlyId: BatchId.toFriendlyId(id) }; } -// A batch anchors on the parent RUN's id, never on another batch, and every call site -// passes that id optionally — so one call serves a root batch and a child batch. +// A batch anchors on the parent run's id, never on another batch. export async function resolveBatchMintKind(args: { environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; parentRunFriendlyId?: string; diff --git a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts index 6b421ae0d4b..334e22d5ccd 100644 --- a/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts @@ -13,10 +13,6 @@ const defaultDeps: RunMintDeps = { resolveMintShard: defaultResolveMintShard, }; -/** - * Where one run mints. The second stage runs only for a root run already on the run-ops path: a - * child inherits its parent's shard by id-shape, so a tree never splits across shards. - */ export async function resolveRunMintTarget(args: { environment: { organizationId: string; id: string; orgFeatureFlags?: unknown }; parentRunFriendlyId?: string; @@ -24,8 +20,7 @@ export async function resolveRunMintTarget(args: { deps?: Partial; }): Promise { if (args.parentRunFriendlyId) { - // The region still travels: it takes index 24 for an inherited gen-1 parent, and a gen-2 - // parent's shardChar outranks it. + // The region still travels: it takes index 24 unless a gen-2 shardChar outranks it. return { ...resolveInheritedMintKind(args.parentRunFriendlyId), region: args.region }; } @@ -46,7 +41,7 @@ export async function resolveRunMintTarget(args: { orgFeatureFlags: args.environment.orgFeatureFlags, }); - // A reserved key means gen-1, the state of every deployment with no shard configured. + // A reserved key means gen-1, which is every deployment with no shard configured. return shard === "new" || shard === "legacy" ? { kind, region: args.region } : { kind, shardChar: shard, region: args.region }; diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 422af3712e9..360bc9fd863 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -75,9 +75,8 @@ export async function resolveMintShard(environment: { // Pass environment.organization.featureFlags from the trigger call site. orgFeatureFlags?: unknown; }): Promise { - // No shard descriptor means no shard can ever be minted into, so answer before reading - // anything: an unconfigured deployment keeps exactly today's code path, with no - // control-plane query on the trigger path, no cache write and no log line. + // Answer before reading anything, so an unconfigured deployment adds no control-plane query to + // the trigger path, no cache write and no log line. if (env.RUN_OPS_SHARDS.length === 0) { return "new"; } diff --git a/apps/webapp/app/v3/services/batchTriggerV3.server.ts b/apps/webapp/app/v3/services/batchTriggerV3.server.ts index 17a3bbb60d3..b86e5a40a64 100644 --- a/apps/webapp/app/v3/services/batchTriggerV3.server.ts +++ b/apps/webapp/app/v3/services/batchTriggerV3.server.ts @@ -362,9 +362,8 @@ export class BatchTriggerV3Service extends BaseService { anchorFriendlyId?: string, region?: string ): Promise { - // Deliberately not routed through resolveRunMintTarget: the root arm below is - // unreachable in production (every call site passes an anchor), and resolveMintKind is - // injected so a test can drive that arm without a database. + // Not routed through resolveRunMintTarget: the root arm is unreachable in production and + // resolveMintKind is injected so a test can drive it without a database. const target = anchorFriendlyId ? resolveInheritedMintKind(anchorFriendlyId) : { diff --git a/apps/webapp/test/runEngineHandlers.test.ts b/apps/webapp/test/runEngineHandlers.test.ts index fda077e026c..63f7e88505f 100644 --- a/apps/webapp/test/runEngineHandlers.test.ts +++ b/apps/webapp/test/runEngineHandlers.test.ts @@ -492,8 +492,7 @@ describe("runEngineHandlers batch completion", () => { describe("runEngineHandlers batch residency routing", () => { // See resolveBatchRunOpsWriter: without a shard arm a gen-2 batch resolves to a store holding // no such row, and the parent waits forever with nothing logged. - // Real databases, so the assertion is where the rows landed rather than which object came back. - // The shard is prisma14 and both gen-1 slots are prisma17, so every wrong resolution lands on a + // The shard is prisma14 and both gen-1 slots are prisma17, so any wrong resolution lands on a // database holding no such batch. heteroPostgresTest( "a gen-2 batch commits on its shard, and the gen-1 store stays empty", @@ -537,7 +536,6 @@ describe("runEngineHandlers batch residency routing", () => { } ); - // The hang was the callback dying on "no record was found for an update" before this. const onShard = await prisma14.batchTaskRun.findFirstOrThrow({ where: { id: gen2BatchId } }); expect(onShard.status).toBe("PARTIAL_FAILED"); expect( @@ -552,8 +550,7 @@ describe("runEngineHandlers batch residency routing", () => { } ); - // A throwing double deliberately: this asserts a call that must NOT happen, and a real client - // would return null and pass either way. + // A throwing double: a real client would return null and pass either way. it("a gen-2 batch id never probes the gen-1 store", async () => { const shardWriter = {} as never; diff --git a/internal-packages/run-engine/src/engine/index.ts b/internal-packages/run-engine/src/engine/index.ts index 4a4cdeb99a7..7917ba68303 100644 --- a/internal-packages/run-engine/src/engine/index.ts +++ b/internal-packages/run-engine/src/engine/index.ts @@ -1859,8 +1859,8 @@ export class RunEngine { const waitpoint = await this.runStore.createWaitpoint( { data: { - // Stamped from the batch, not the blocked run: this create passes only - // completedByBatchId, so that is the owner the router validates the stamp against. + // From the batch, not the blocked run: the create passes only completedByBatchId, + // which is the owner the router validates against. ...mintWaitpointIdFor(batchId), type: "BATCH", idempotencyKey: batchId, diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts index 4877075100b..def58f7bc37 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts @@ -239,7 +239,6 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator // The two `nanoid(24)` calls below are deliberately separate and produce DIFFERENT values: // the upsert `where` key must not match the `create` key, or a guaranteed-miss upsert becomes // a possible update. Do not hoist either to a shared constant. - // Stamped for the anchor run's shard, so the row is routable and completion needs no probe. const upsertArgs = { where: { environmentId_idempotencyKey: { @@ -338,9 +337,8 @@ export class LegacyPostgresWaitpointCoordinator implements WaitpointCoordinator while (attempts < maxRetries) { try { // As in createDateTimeWaitpoint, the two `nanoid(24)` calls are deliberately separate and - // differ. Both, and the id mint, are re-evaluated on every attempt: that is what makes a - // retry after a unique-constraint conflict try a fresh key. The anchor does not change, - // so every attempt stays on the same shard. + // differ. Both are re-evaluated per attempt, so a retry after a conflict tries a fresh + // key. The anchor does not change, so every attempt stays on the same shard. const waitpoint = await this.runStore.upsertWaitpoint( { where: { diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts index 80463b1cd1f..412e1ed97ec 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts @@ -25,7 +25,7 @@ export type WaitpointCoordinator = { mintAssociatedWaitpointData(params: { projectId: string; environmentId: string; - /** Names the shard the row lands on. This write skips the router's stamp check. */ + /** This write skips the router's stamp check. */ anchorRunId: string; }): AssociatedWaitpointData; createAssociatedWaitpoint(params: { @@ -112,11 +112,9 @@ export type CreateWaitpointResult = export type CreateDateTimeWaitpointParams = { /** - * When set, the waitpoint co-locates with this run's DB and the dedup probe targets it. - * - * Every production caller supplies it, and there is deliberately no standalone arm. Omitting it - * on a gen-2 environment mints a cuid and lands the row on a gen-1 store, silently. A standalone - * caller needs a shard hint here first, as `createManualWaitpointParams` has. + * Co-locates the waitpoint with this run's DB. There is deliberately no standalone arm: omitting + * it on a gen-2 environment lands the row on a gen-1 store, silently. A standalone caller needs + * a shard hint here first, as `CreateManualWaitpointParams` has. */ runId?: string; projectId: string; @@ -139,10 +137,7 @@ export type CreateManualWaitpointParams = { * full rationale. Only a Postgres implementation reads this. */ standaloneResidency?: "NEW" | "LEGACY"; - /** - * The environment's mint shard, for a standalone token with no owning run. When it names a gen-2 - * shard the implementation must ignore `standaloneResidency`, which can only name a gen-1 store. - */ + /** For a standalone token. When it names a gen-2 shard, ignore `standaloneResidency`. */ standaloneShardKey?: ShardKey; }; diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts index ebd13aeaec0..4a7a863bd4d 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts @@ -21,8 +21,8 @@ function count(source: string, pattern: RegExp): number { return (source.match(pattern) ?? []).length; } -// Walked rather than listed, so a mint added in a new file is still visible. Test-support trees -// are excluded: a helper writing through raw Prisma never reaches the routing store. +// Walked, not listed, so a mint in a new file is visible. Test trees excluded: a raw-Prisma +// helper never reaches the routing store. const TEST_SUPPORT_DIRS = new Set(["tests", "__tests__", "fixtures"]); function walk(relativeRoot: string): string[] { @@ -36,12 +36,11 @@ function walk(relativeRoot: string): string[] { }); } -// The mint helpers are the only sanctioned way to produce a Postgres waitpoint id. const MINT_CALL = /mintWaitpointIdFor(?:Shard)?\(/g; const UNSTAMPED_MINT = /WaitpointId\.generate\(/g; const WAITPOINT_WRITE = /waitpoint\.create\(|upsertWaitpoint\(|createWaitpoint\(/g; -// The catalog holds the mint expressions as string data, so scanning it would count them. +// Scanning the catalog would count its own string data. const CATALOG_ITSELF = "internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts"; @@ -50,7 +49,6 @@ const ENGINE_SOURCES = walk("internal-packages/run-engine/src/engine").filter( ); const SCANNED = [...ENGINE_SOURCES, "internal-packages/run-store/src/PostgresRunStore.ts"]; -// expression -> how many times the catalog says it appears in this file function expectedMints(file: string): Map { const expected = new Map(); for (const site of WAITPOINT_MINT_SITES.filter((s) => s.site === file)) { @@ -71,7 +69,7 @@ describe("waitpoint mint census — the catalog matches the source", () => { expect(SCANNED).toContain("internal-packages/run-engine/src/engine/systems/waitpointSystem.ts"); }); - // Per expression, not per file, so a swapped anchor fails too and not just a new site. + // Per expression, so a swapped anchor fails too. it.each(SCANNED)("%s has exactly the mint expressions the catalog claims", (file) => { const source = read(file); const expected = expectedMints(file); @@ -88,12 +86,12 @@ describe("waitpoint mint census — the catalog matches the source", () => { }); it.each(SCANNED)("%s mints no waitpoint id with the un-stamped helper", (file) => { - // Matches inside comments too, deliberately: any textual addition forces a reconcile. + // Matches inside comments too: any textual addition forces a reconcile. expect(count(read(file), UNSTAMPED_MINT)).toBe(0); }); it.each(SCANNED)("%s writes a waitpoint row only if it is catalogued", (file) => { - // A create with no id is the worst case: @default(cuid()) then mints one after the write. + // Worst case is a create with no id: @default(cuid()) mints one after the write. const writes = count(read(file), WAITPOINT_WRITE); const catalogued = WAITPOINT_MINT_SITES.some((s) => s.site === file); expect(writes === 0 || catalogued).toBe(true); diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts index b42d2d4de9f..7f5df203b65 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts @@ -1,21 +1,13 @@ -// Add a site that creates a Postgres `Waitpoint` row and add an entry here, or -// `waitpointMint.proof.test.ts` fails. One entry per site, anchored by symbol, never by line. -// -// A site that mints a cuid for a gen-2 run writes a row the completion path cannot find. Most -// fail loudly, because the router refuses an unstamped id on a gen-2 shard. The RUN row written -// through `createRun` does not: that write is inside the run store, which has no such check. -// -// Pure module: no engine import, no env, no Prisma. +// A site creating a Postgres `Waitpoint` row needs an entry here or `waitpointMint.proof.test.ts` +// fails. Most unstamped mints fail loudly at the router, but `createRun` writes inside the run +// store, which has no stamp check. export type WaitpointMintSite = { id: string; type: "DATETIME" | "MANUAL" | "RUN" | "BATCH"; site: string; - /** Enclosing method or symbol name — NEVER a line number. */ + /** Never a line number. */ symbol: string; - /** - * Mint expressions this site contains, verbatim, counted per file, so a new mint and a swapped - * anchor both fail until reconciled. Empty for a site writing an id minted elsewhere. - */ + /** Verbatim, counted per file, so a swapped anchor fails too. Empty if minted elsewhere. */ mints: readonly string[]; }; @@ -60,9 +52,7 @@ export const WAITPOINT_MINT_SITES: readonly WaitpointMintSite[] = [ site: ENGINE, symbol: "blockRunWithCreatedBatch", }, - // The physical writers of the RUN row. They take an already-minted id rather than - // minting one, but they are the writes that bypass the routing store's stamp check, so a - // new writer here must be seen. + // These bypass the routing store's stamp check, so a new writer here must be seen. { id: "runStore.createRun.nested", mints: [], diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts index d1f0834d823..53edce67d1f 100644 --- a/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts +++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts @@ -4,8 +4,8 @@ import type { PrismaClient, Waitpoint } from "@trigger.dev/database"; import { describe, expect, it } from "vitest"; import { LegacyPostgresWaitpointCoordinator } from "./legacyPostgresCoordinator.js"; -// These drive the real create sites, not the mint helper: a test calling the helper directly -// passes even when a site stops passing its anchor. +// These drive the real create sites: calling the helper directly passes even when a site stops +// passing its anchor. const GEN2_RUN = `${"a".repeat(24)}a2`; const GEN1_RUN = `${"a".repeat(24)}01`; const GEN2_BATCH = `${"d".repeat(24)}b2`; @@ -114,8 +114,6 @@ describe("createManualWaitpoint stamps the anchor's shard", () => { }); describe("mintAssociatedWaitpointData stamps the anchor's shard", () => { - // Written inside the run store, which has no stamp check, so an unstamped id here strands the - // parent run with nothing logged. const mint = (anchorRunId: string) => coordinatorCapturing({}).mintAssociatedWaitpointData({ projectId: "proj", @@ -140,7 +138,6 @@ describe("mintAssociatedWaitpointData stamps the anchor's shard", () => { }); it("a batch anchor stamps the batch's shard", () => { - // The create names only completedByBatchId, so that is what the router validates against. expect(mint(GEN2_BATCH).id[24]).toBe("b"); }); }); diff --git a/internal-packages/run-store/src/placement.proof.test.ts b/internal-packages/run-store/src/placement.proof.test.ts index e40e3888424..2dc6717e2bf 100644 --- a/internal-packages/run-store/src/placement.proof.test.ts +++ b/internal-packages/run-store/src/placement.proof.test.ts @@ -23,17 +23,12 @@ const read = (relative: string) => readFileSync(path.join(repoRoot(), relative), const TYPES = "internal-packages/run-store/src/types.ts"; const STORE = "internal-packages/run-store/src/runOpsStore.ts"; -/** - * Method names on the `RunStore` interface, overloads collapsed. Parsed from source rather than - * imported as a type, so a failure can name the method somebody forgot to classify. - */ +/** Parsed from source, not imported as a type, so a failure can name the unclassified method. */ function interfaceMethods(): string[] { const source = read(TYPES); const start = source.indexOf("export interface RunStore {"); expect(start).toBeGreaterThan(-1); - // Declarations sit at two-space indent. A stray name from later in the file shows up as - // uncatalogued rather than being dropped. const body = source.slice(start); const names = new Set(); for (const match of body.matchAll(/^ {2}([a-zA-Z][A-Za-z0-9]*)(<[^\n]*?>)?\(/gm)) { @@ -42,12 +37,9 @@ function interfaceMethods(): string[] { return [...names]; } -/** - * One method implementation: its declaration to the next member at the same indent. Not - * brace-matching, which a signature carrying an inline object type makes fiddly to get right. - */ +/** Not brace-matching: an inline object type in a signature makes that fiddly to get right. */ function methodBody(source: string, method: string): string | undefined { - // The last declaration: an overloaded method leads with bodiless signatures. + // Last, not first: an overloaded method leads with bodiless signatures. const declaration = new RegExp(`^ {2}(?:async )?${method}(?:<[^\\n]*?>)?\\(`, "gm"); const matches = [...source.matchAll(declaration)]; const start = matches.at(-1)?.index; @@ -71,7 +63,6 @@ describe("run-store placement census — every write states what it routes by", expect(methods).toContain("findRun"); }); - // The point of the census: nobody adds a write without saying how it is placed. it("classifies every interface method as exactly one of read or write", () => { const methods = interfaceMethods(); const { all } = catalogued(); @@ -96,8 +87,6 @@ describe("run-store placement census — every write states what it routes by", expect({ duplicates }).toEqual({ duplicates: [] }); }); - // The forbidden cell: a row on a database its owner does not live on, with nothing to detect - // it. `upsertWaitpointTag` sat here and every functional test passed. it("has no write that routes on residency alone and misses silently", () => { const forbidden = PLACEMENT_SITES.filter( (s) => s.basis === "residency" && s.missMode === "silent" @@ -106,7 +95,6 @@ describe("run-store placement census — every write states what it routes by", expect({ residencyOnlySilentWrites: forbidden }).toEqual({ residencyOnlySilentWrites: [] }); }); - // Both are claims about safety rather than mechanisms, so each has to be argued in the catalog. it("requires a written justification wherever safety is a claim, not a mechanism", () => { const unjustified = PLACEMENT_SITES.filter( (s) => (s.basis === "residency" || s.basis === "fan-out") && (s.why ?? "").trim().length < 40 @@ -120,8 +108,7 @@ describe("run-store placement census — every write states what it routes by", expect({ withoutRoutes: empty }).toEqual({ withoutRoutes: [] }); }); - // Scoped to the method's own body, not the whole file: three creates share - // `#routeOrNew(params.data.id)`, so a file-wide search passes when one loses its route. + // Per body, not per file: three creates share one route expression. it.each(PLACEMENT_SITES)("$method still contains the routes the catalog claims", (site) => { const body = methodBody(read(STORE), site.method); diff --git a/internal-packages/run-store/src/placementCatalog.ts b/internal-packages/run-store/src/placementCatalog.ts index 0e88ef707c1..4768c389075 100644 --- a/internal-packages/run-store/src/placementCatalog.ts +++ b/internal-packages/run-store/src/placementCatalog.ts @@ -1,39 +1,23 @@ -// Every method on the `RunStore` interface appears exactly once below, as a read or as a write. -// `placement.proof.test.ts` diffs this catalog against the interface, so a new method fails the -// build until somebody classifies it. -// -// The waitpoint mint census is exhaustive over id production and cannot see a row with no minted -// id, which is how `WaitpointTag` wrote to a gen-1 store for a gen-2 environment with every -// functional test passing. This is exhaustive over placement instead: what does each write route -// by? The combination that must never exist is residency-only routing with a silent miss. -// -// Pure module: no store import, no Prisma, no env. +// Every `RunStore` method appears below once, as a read or a write, and `placement.proof.test.ts` +// fails until a new one is classified. The combination that must never exist is residency-only +// routing with a silent miss: `WaitpointTag` sat there, misplacing rows with tests passing. -/** What the routing decision is made from. `residency` cannot name a gen-2 shard. */ +/** `residency` cannot name a gen-2 shard. */ type PlacementBasis = "own-id" | "owner-id" | "shard-hint" | "fan-out" | "residency"; -/** - * `loud` — Prisma raises "no record was found for an update" and the caller sees it. - * `silent` — the write succeeds on the wrong database. A create inserts a row there; an - * `updateMany` reports zero rows affected, which callers read as "nothing to do". - */ +/** `silent`: the write succeeds on the wrong database, or an `updateMany` affects zero rows. */ type MissMode = "loud" | "silent"; export type PlacementSite = { method: string; basis: PlacementBasis; missMode: MissMode; - /** - * Routing expressions the implementation contains, verbatim. The proof test requires each to - * still be present, so weakening a route fails here first. List every arm: the first arm that - * matches is what routes, so a set that looks safe on its last arm proves nothing. - */ + /** Verbatim, and every arm: the first arm that matches is what routes. */ routes: readonly string[]; - /** Required for `residency` and `fan-out`, where safety is a claim rather than a mechanism. */ + /** Required for `residency` and `fan-out`, where safety is a claim not a mechanism. */ why?: string; }; -/** Handed a run id, routing on it. Listed by name; 20 identical entries would be rubber-stamped. */ export const ROUTES_BY_GIVEN_RUN_ID: readonly string[] = [ "startAttempt", "completeAttemptSuccess", @@ -204,11 +188,7 @@ export const PLACEMENT_SITES: readonly PlacementSite[] = [ }, ]; -/** - * Reads, listed only so the union covers the interface exactly: a new method named - * `getOrCreateThing` would otherwise pass for a read on the strength of its name. Read routing is - * not audited here, because a read that probes the wrong store finds nothing and moves on. - */ +/** Listed so the union covers the interface: a `getOrCreateThing` must not pass as a read. */ export const READ_ONLY_METHODS: readonly string[] = [ "findRun", "findRunOrThrow", diff --git a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts index ec8ecb20dde..14b5fb15bf1 100644 --- a/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts +++ b/internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts @@ -304,9 +304,8 @@ describe("RoutingRunStore four-store matrix — pagination merge", () => { ); }); -// A tag has no id to route by and no owning row to follow, and `residency` names only a gen-1 -// store. Nothing fails when the row is misplaced, because reads fan out and find it anyway, so -// only a per-database count can see it. Hence container tests rather than the fake-store suite. +// A misplaced tag row still reads back, because the read fans out, so only a per-database count +// can see it. Hence containers rather than the fake-store suite. describe("four-store matrix — a waitpoint tag lands on its environment's shard", () => { matrixTest( "the shard key routes the tag to shard a, and no gen-1 store receives it", @@ -317,8 +316,6 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard await router.upsertWaitpointTag( { environmentId: env.environmentId, name: "tag-on-a", projectId: env.projectId }, undefined, - // The residency an environment minting gen-2 ids reports. On its own this names the gen-1 - // NEW store, so it is exactly the value that used to misplace the row. "NEW", "a" ); @@ -350,8 +347,8 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "b" ); - // The unique constraint is per-database, so a collapse onto one database still inserts two - // rows. The failure to catch is placement, not a constraint violation. + // The constraint is per-database, so a collapse still inserts two rows: this catches + // placement, not a constraint violation. const onA = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "shared-name" } }); const onB = await shardPrismas[1]!.waitpointTag.findMany({ where: { name: "shared-name" } }); expect(onA.map((r) => r.environmentId)).toEqual([envA.environmentId]); @@ -390,7 +387,6 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "a" ); - // The read takes no shard hint, so this proves the write is reachable the normal way. const found = await router.findManyWaitpointTags({ where: { environmentId: env.environmentId }, }); @@ -398,9 +394,7 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard } ); - // What a real rollout produces: an environment has tags, then it is pinned. Its old rows stay on - // the gen-1 store and the same name goes to the shard with its own cuid, so an id-keyed dedupe - // would list the name twice. + // What a rollout produces: tags exist, then the environment is pinned. matrixTest( "the same tag name on a gen-1 store and a shard is listed once", async ({ legacyPrisma, newPrisma, shardPrismas }) => { @@ -419,7 +413,6 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "a" ); - // Two physical rows with different ids, which the per-database unique index permits. const onNew = await newPrisma.waitpointTag.findMany({ where: { name: "prod" } }); const onShard = await shardPrismas[0]!.waitpointTag.findMany({ where: { name: "prod" } }); expect(onNew).toHaveLength(1); @@ -453,8 +446,7 @@ describe("four-store matrix — a waitpoint tag lands on its environment's shard "b" ); - // No environment filter, so both rows reach the merge together. Filtering per call would - // hide a name-only dedupe: each result set would hold one row and collapse to itself. + // No environment filter, or each result set holds one row and a name-only key would pass. const both = await router.findManyWaitpointTags({ where: { name: "prod" } }); expect(both.map((r) => r.environmentId).sort()).toEqual( diff --git a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts index 3cb2253c06b..aff0d50764e 100644 --- a/internal-packages/run-store/src/runOpsStore.shardMap.test.ts +++ b/internal-packages/run-store/src/runOpsStore.shardMap.test.ts @@ -936,9 +936,6 @@ describe("RoutingRunStore batch probe tolerates legitimate dual-residency", () = }); describe("RoutingRunStore waitpoint tags follow their environment's shard", () => { - // A tag has no id to route by and `residency` names only a gen-1 store, so without the hint the - // row lands on a different database from the tokens it describes. Reads fan out and still find - // it, so the symptom is placement rather than an error. const tag = { environmentId: "env_1", name: "tag", projectId: "proj_1" }; const shardedRouter = () => { @@ -971,9 +968,7 @@ describe("RoutingRunStore waitpoint tags follow their environment's shard", () = }); describe("RoutingRunStore waitpoint writes: a stamped gen-2 id outranks a residency hint", () => { - // `residency` names only NEW or LEGACY, so a stamped gen-2 id has to win. Before this, safety - // rested on every caller withholding the hint for a gen-2 id; a caller that passed both would - // have written the row to a gen-1 database silently, because a create never misses. + // A caller passing both used to write to a gen-1 database silently: a create never misses. const GEN2 = `${"a".repeat(24)}a2`; const GEN1 = `${"a".repeat(24)}01`; const CUID = "c".repeat(25); diff --git a/internal-packages/run-store/src/runOpsStore.ts b/internal-packages/run-store/src/runOpsStore.ts index 31a58c74f8e..fa923244d06 100644 --- a/internal-packages/run-store/src/runOpsStore.ts +++ b/internal-packages/run-store/src/runOpsStore.ts @@ -1565,8 +1565,7 @@ export class RoutingRunStore implements RunStore { } return this.#shardStore(key); } - // A gen-2-stamped id names the only database this row can live on, and `residency` can name - // only NEW or LEGACY, so the id wins rather than every caller having to withhold the hint. + // `residency` names only NEW or LEGACY, so a stamped gen-2 id wins. const stamped = typeof waitpointId === "string" ? this.#shardKeyOfSafe(waitpointId) : undefined; const isGen2Stamped = stamped !== undefined && stamped !== NEW_SHARD && stamped !== LEGACY_SHARD; @@ -2251,8 +2250,6 @@ export class RoutingRunStore implements RunStore { // No owning run; route by the env's residency hint when present, else a minted id-shape, else // fall back to LEGACY (same precedence as a standalone waitpoint). Caller tx is never forwarded. // - // A gen-2 shard hint wins outright: a tag has no id to route by, and `residency` names only a - // gen-1 store, which would leave the row on a different database from the tokens it describes. const store = shardKey !== undefined && shardKey !== NEW_SHARD && shardKey !== LEGACY_SHARD ? this.#shardStore(shardKey) @@ -2260,19 +2257,14 @@ export class RoutingRunStore implements RunStore { return store.upsertWaitpointTag(data, undefined); } - // Two collisions exist, so both keys are needed in this order. By id first: drain can mirror a - // tag onto NEW while it keeps its id, and NEW is authoritative. By (environmentId, name) second: - // the unique index is per-database, so a store that never saw the tag minted its own cuid for it - // and the id pass cannot tell they are one tag. - // - // Dropping a row is safe because nothing reads a tag's id: a waitpoint holds its tags as a string - // array and this table is a name registry. + // Both keys, in order. Drain can mirror a tag onto NEW keeping its id, and NEW wins. Then by + // name, because the per-database unique index lets a store mint its own cuid for a tag it has + // not seen. Dropping a row is safe: nothing reads a tag's id. #mergeTags>(legs: Array<{ key: ShardKey; rows: R[] }>): R[] { const survivors = this.#mergeById(legs); const survivorSet = new Set(survivors as R[]); - // Legs arrive in #precedence order, so the last write wins. Restricted to id-pass survivors so - // a stale mirror cannot win its name back. + // Survivors only, so a stale mirror cannot win its name back. const winnerByName = new Map(); for (const { rows } of legs) { for (const row of rows) { @@ -2282,8 +2274,7 @@ export class RoutingRunStore implements RunStore { } } - // Filter rather than rebuild: a winner keeps the position #mergeById gave it, which callers - // observe when `orderBy` is absent. + // Filter, not rebuild: position matters when `orderBy` is absent. return (survivors as R[]).filter((row) => { const key = RoutingRunStore.#tagNameKey(row); return key === undefined || winnerByName.get(key) === row; diff --git a/internal-packages/run-store/src/types.ts b/internal-packages/run-store/src/types.ts index 036b844bdf6..41fecf90bb5 100644 --- a/internal-packages/run-store/src/types.ts +++ b/internal-packages/run-store/src/types.ts @@ -959,8 +959,8 @@ export interface RunStore { // minted-new env's tags read this residency (NEW) so they land with the env's tokens/runs // instead of defaulting to LEGACY. Single-store impls ignore it. residency?: Residency, - // The environment's gen-2 mint shard. A tag has no id to route by, so this is the only way its - // row follows its environment's tokens onto a shard. Outranks `residency`. + // A tag has no id to route by, so this is the only way its row follows its environment's + // tokens onto a shard. Outranks `residency`. shardKey?: ShardKey ): Promise; findManyWaitpointTags( diff --git a/packages/core/src/v3/isomorphic/friendlyId.ts b/packages/core/src/v3/isomorphic/friendlyId.ts index 7c78ae46b93..b38aa2899e8 100644 --- a/packages/core/src/v3/isomorphic/friendlyId.ts +++ b/packages/core/src/v3/isomorphic/friendlyId.ts @@ -229,12 +229,10 @@ export function isRunOpsIdBody(body: string): boolean { return parseRunOpsIdBody(body) !== undefined; } -// Shape-only check over the same alphabet base32hexDecode accepts, so it is the same predicate as -// "the decode would not throw". Routing needs the shape only, and decoding a timestamp to discard -// it costs ~30x more on a path taken for every routed call. +// Same alphabet base32hexDecode accepts, so this and "the decode would not throw" are one +// predicate. Decoding a timestamp to discard it costs ~30x more on the router's hot path. const RUN_OPS_ID_CORE_PATTERN = /^[0-9a-v]{24}$/; -/** Shape-only v1 body check for routing: 26 chars, version "1", region and core in range. */ export function isRunOpsIdBodyShape(body: string): boolean { return ( body.length === RUN_OPS_ID_LENGTH && @@ -244,7 +242,6 @@ export function isRunOpsIdBodyShape(body: string): boolean { ); } -/** Shape-only gen-2 body check for routing. Returns the shard char, or undefined. */ export function runOpsIdV2ShardShape(body: string): string | undefined { if (body.length !== RUN_OPS_ID_LENGTH) return undefined; if (body[RUN_OPS_ID_VERSION_INDEX] !== RUN_OPS_ID_VERSION_2) return undefined; diff --git a/packages/core/src/v3/isomorphic/waitpointMint.test.ts b/packages/core/src/v3/isomorphic/waitpointMint.test.ts index 8d35a7248ad..a320cae852d 100644 --- a/packages/core/src/v3/isomorphic/waitpointMint.test.ts +++ b/packages/core/src/v3/isomorphic/waitpointMint.test.ts @@ -76,8 +76,7 @@ describe("mintWaitpointIdFor", () => { }); describe("resolveShard shape checks match the decoding parsers", () => { - // The alphabet is [0-9a-v], so "the shape matches" and "the decode would not throw" are the - // same predicate. These pin that equivalence: a drift misroutes rather than erroring. + // Pins the shape/decode equivalence: a drift misroutes rather than erroring. const classifyByDecode = (body: string): string => { const genTwo = parseRunOpsIdV2Body(body); if (genTwo) return genTwo.shard; diff --git a/packages/core/src/v3/isomorphic/waitpointMint.ts b/packages/core/src/v3/isomorphic/waitpointMint.ts index 4ce08dfaaa6..f4327d40f0e 100644 --- a/packages/core/src/v3/isomorphic/waitpointMint.ts +++ b/packages/core/src/v3/isomorphic/waitpointMint.ts @@ -1,8 +1,8 @@ import { generateRunOpsIdV2, WaitpointId } from "./friendlyId.js"; import { resolveShard, type ShardKey } from "./runOpsResidency.js"; -// A Postgres waitpoint id, NOT the Redis store format (version "w" at index 25), which has no -// Postgres row to route. The core is always fresh, or the body would equal the anchor's own id. +// A Postgres waitpoint id, not the Redis store format (version "w"), which has no row to route. +// The core is always fresh, or the body would equal the anchor's own id. export function mintWaitpointIdForShard(key: ShardKey): { id: string; friendlyId: string } { if (key === "new" || key === "legacy") { return WaitpointId.generate(); @@ -12,8 +12,7 @@ export function mintWaitpointIdForShard(key: ShardKey): { id: string; friendlyId return { id, friendlyId: WaitpointId.toFriendlyId(id) }; } -// Every Postgres waitpoint mint goes through here: the router refuses an id that is not stamped -// for the shard it lands on. A gen-1 or legacy anchor keeps a cuid. +// Every Postgres waitpoint mint goes through here: the router refuses an unstamped id on a shard. export function mintWaitpointIdFor(anchorId: string | undefined): { id: string; friendlyId: string;