From 38dbcab82fa178a850108ed30ce37c60698d4a71 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Thu, 27 Aug 2026 14:32:49 +0100 Subject: [PATCH 1/6] fix(webapp): scan every run-ops store for the batches list The batches list read exactly two databases, so a batch living on any other configured run-ops store was missing from the page with no error and nothing logged. The scan now covers one leg per store, in ascending precedence order, and the existing keyset merge generalises unchanged: every leg runs the same query, so the merged first page is still the true first page. The empty-state check keeps its existing pair and issues the rest in one round trip. A store that declares itself an alias of another shares its client by reference and contributes no leg, matching how the routing store and the boot sentinels treat one. With no extra store configured the page is byte-identical to today. --- .../v3/BatchListPresenter.server.ts | 68 ++++- .../route.tsx | 2 + .../shardHandles.server.test.ts | 27 +- .../v3/runOpsMigration/shardHandles.server.ts | 41 ++- .../test/batchListPresenter.readroute.test.ts | 275 +++++++++++++++++- 5 files changed, 399 insertions(+), 14 deletions(-) diff --git a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts index 5440b602877..e489e260973 100644 --- a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts +++ b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts @@ -75,6 +75,14 @@ export class BatchListPresenter extends BasePresenter { runOpsNew?: RunOpsPrismaClient; // new run-ops client (run-ops brand ⇒ guard classifies as runops) runOpsLegacyReplica?: RunOpsPrismaClient; // legacy run-ops READ REPLICA only — never the legacy primary controlPlaneReplica?: PrismaClientOrTransaction; // control-plane DB (for project) + // Gen-2 shard replicas to fan out over, in configured order — ONE ENTRY PER PHYSICAL DATABASE. + // The route passes `runOpsNonAliasedShardReplicas`, which has already dropped every shard that + // declares `aliasOf` (an alias shares its target's client by reference, so a leg for it would + // scan one database twice). Empty (RUN_OPS_SHARDS unset) keeps today's exact two legs. + // NOTE: a shard configured without a `replicaUrl` reads its WRITER. That is the established + // behaviour of every shard handle consumer; the "never the legacy primary" rule below is + // scoped to the LEGACY store, which has no replica-less arm. + shardReplicas?: ReadonlyArray<{ key: string; replica: RunOpsPrismaClient }>; splitEnabled?: boolean; // resolved boot constant } ) { @@ -106,16 +114,36 @@ export class BatchListPresenter extends BasePresenter { return scan(passthrough); } - // Always read BOTH stores and merge. The old "skip legacy when new fills the page" shortcut is + // Always read EVERY store and merge. The old "skip legacy when new fills the page" shortcut is // unsound across the residency split: legacy cuid ids ("c…") sort ABOVE new run-ops ids ("0…") // under id order, so a new-only page can hide pre-flip legacy batches that belong ahead of it. - // Ordering is by createdAt (id tiebreak), which is chronologically correct across both schemes. - const [newRows, legacyRows] = await Promise.all([ + // Ordering is by createdAt (id tiebreak), which is chronologically correct across every scheme. + // + // Every leg runs the SAME closure, so `take: pageSize + 1`, the cursor predicate and the two-key + // order are shared. A row's rank within its own leg is never worse than its global rank, so the + // true global first (pageSize + 1) rows are all present among the per-leg results — an argument + // in the number of legs, not in two. + // + // Promise.all, NOT allSettled: one store down must fail the page. A tolerant fan-out would + // return a SHORT page with no error, which is exactly the silent absence this routing exists to + // remove, with a wider blast radius. The routing store's own list fan-out is Promise.all for the + // same reason. + const shardReplicas = this.readRoute.shardReplicas ?? []; + const [newRows, legacyRows, ...shardRows] = await Promise.all([ scan(this.readRoute.runOpsNew ?? passthrough), scan(this.readRoute.runOpsLegacyReplica ?? passthrough), + ...shardReplicas.map((shard) => scan(shard.replica)), ]); - // De-dupe by id (new wins), re-sort under the page's keyset order, re-apply the over-fetch LIMIT. + // De-dupe by id, re-sort under the page's keyset order, re-apply the over-fetch LIMIT. + // + // Precedence ascends legacy → new → gen-2 shards, last write wins — the routing store's + // `#mergeById` semantics, which inserts unconditionally over legs supplied in precedence order. + // The gen-1 pair keeps its original conditional form so its result stays byte-identical; the + // shard legs then overwrite. Do NOT "simplify" the conditional insert below into an + // unconditional one without also reordering the legs, or legacy would start winning over new. + // A gen-1 id and a gen-2 id cannot collide in any case: a batch is created on exactly one store, + // routed by its own id shape. const byId = new Map(); for (const row of newRows) { byId.set(row.id, row); @@ -125,6 +153,11 @@ export class BatchListPresenter extends BasePresenter { byId.set(row.id, row); } } + for (const rows of shardRows) { + for (const row of rows) { + byId.set(row.id, row); + } + } // forward => newest-first (createdAt DESC), backward => oldest-first (ASC); id is the stable // tiebreak (ASCII codepoint, NEVER localeCompare). @@ -140,7 +173,8 @@ export class BatchListPresenter extends BasePresenter { } // Empty-state probe. Split on: probe the new run-ops DB first, then the legacy READ REPLICA only - // (never the legacy primary). Split off (single-DB / self-host): one plain `_replica` probe. + // (never the legacy primary), then every configured gen-2 shard in parallel. Split off (single-DB + // / self-host): one plain `_replica` probe. async #probeAnyBatch(environmentId: string): Promise { // Single-DB / passthrough: `_replica` IS the run-ops database, and it is the SAME client the // scan uses, so the empty-state hint can't disagree with the page. Carry the run-ops brand @@ -167,7 +201,29 @@ export class BatchListPresenter extends BasePresenter { ).batchTaskRun.findFirst({ where: { runtimeEnvironmentId: environmentId }, }); - return Boolean(onLegacy); + if (onLegacy) { + return true; + } + + // The gen-1 pair above keeps its sequential short-circuit — it is two legs, and a project with + // no batches yet is the common case for this path. The shards then go in ONE round trip instead + // of N: the probe answers a boolean, so there is no precedence to resolve and nothing to + // short-circuit for. `RoutingRunStore.#probeFirst` parallelises above two legs for the same + // reason, though it switches on the TOTAL leg count rather than keeping a sequential head — so + // an empty project with N shards costs 3 round trips here, not 1. + const shardReplicas = this.readRoute.shardReplicas ?? []; + if (shardReplicas.length === 0) { + return false; + } + + const onShards = await Promise.all( + shardReplicas.map((shard) => + shard.replica.batchTaskRun.findFirst({ + where: { runtimeEnvironmentId: environmentId }, + }) + ) + ); + return onShards.some(Boolean); } public async call({ diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsx index ab139d8d312..5a8574c799a 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsx @@ -56,6 +56,7 @@ import { runOpsSplitReadEnabled, type PrismaClientOrTransaction, } from "~/db.server"; +import { runOpsNonAliasedShardReplicas } from "~/v3/runOpsMigration/shardHandles.server"; import { docsPath, EnvironmentParamSchema, @@ -104,6 +105,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { runOpsNew: runOpsNewReplicaClient, runOpsLegacyReplica: runOpsLegacyReplicaClient, controlPlaneReplica: $replica as unknown as PrismaClientOrTransaction, + shardReplicas: runOpsNonAliasedShardReplicas, splitEnabled: runOpsSplitReadEnabled, }); const list = await presenter.call({ diff --git a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts index b90c1dfe7c4..f42fe1d8d5c 100644 --- a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { buildShardHandleMaps } from "./shardHandles.server"; +import { buildShardHandleMaps, nonAliasedShardReplicas } from "./shardHandles.server"; // Two distinct sentinels per shard: the maps must not cross writer and replica. function handle(key: string) { @@ -35,3 +35,28 @@ describe("buildShardHandleMaps", () => { expect(replicas.get("a")).not.toEqual({ tag: "a-writer" }); }); }); + +describe("nonAliasedShardReplicas", () => { + it("yields an empty list when no shard is configured", () => { + expect(nonAliasedShardReplicas([])).toEqual([]); + }); + + it("keeps the configured order and carries each shard's replica", () => { + expect(nonAliasedShardReplicas([handle("b"), handle("a")])).toEqual([ + { key: "b", replica: { tag: "b-replica" } }, + { key: "a", replica: { tag: "a-replica" } }, + ]); + }); + + // An aliased shard shares its target's client BY REFERENCE, so a leg for it scans one database + // twice. The router drops it the same way, on the DECLARATION rather than object identity. + it("drops a shard that declares aliasOf", () => { + expect(nonAliasedShardReplicas([{ ...handle("a"), aliasOf: "new" }, handle("b")])).toEqual([ + { key: "b", replica: { tag: "b-replica" } }, + ]); + }); + + it("never carries a writer in place of a replica", () => { + expect(nonAliasedShardReplicas([handle("a")])[0]?.replica).not.toEqual({ tag: "a-writer" }); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts index cbe827be4dc..2f20ee6b33b 100644 --- a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts @@ -5,14 +5,17 @@ * is what keeps every gen-2 arm unreachable today. */ import type { PrismaClient } from "@trigger.dev/database"; +import type { RunOpsPrismaClient } from "@internal/run-ops-database"; import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; import type { PrismaReplicaClient } from "~/db.server"; import { runOpsShardHandles } from "~/db.server"; type ShardHandle = { key: string; - writer: unknown; - replica: unknown; + writer: RunOpsPrismaClient; + replica: RunOpsPrismaClient; + /** The DECLARED alias, if any. See nonAliasedShardReplicas. */ + aliasOf?: string; }; export function buildShardHandleMaps(handles: ShardHandle[]): { @@ -22,8 +25,8 @@ export function buildShardHandleMaps(handles: ShardHandle[]): { const replicas = new Map(); const writers = new Map(); for (const handle of handles) { - replicas.set(handle.key, handle.replica as PrismaReplicaClient); - writers.set(handle.key, handle.writer as PrismaClient); + replicas.set(handle.key, handle.replica as unknown as PrismaReplicaClient); + writers.set(handle.key, handle.writer as unknown as PrismaClient); } return { replicas, writers }; } @@ -40,7 +43,35 @@ function resolveShardHandles(): ShardHandle[] { } } -const maps = buildShardHandleMaps(resolveShardHandles()); +/** + * The shards a FAN-OUT should visit: one entry per physical database, in configured order. + * + * A shard that declares `aliasOf` shares its target's client BY REFERENCE (db.server.ts sets the + * alias target's own client into the shard map), so a leg for it would scan one database twice and + * return rows the target's own leg already returned. `RoutingRunStore` drops aliased keys from its + * store list for exactly this reason, and the discriminator there is the DECLARATION, not object + * identity — identity cannot tell an alias from its target, and a shard that shares a database + * WITHOUT declaring it is a misconfiguration the boot sentinel is there to catch, not something to + * paper over here. + * + * Routed lookups keyed by a single id want `runOpsShardReplicas` instead: an aliased key is a + * legitimate route target, it just is not a second database to scan. + * + * Generic in the client type so the caller keeps whatever type its handles carry — no cast. + */ +export function nonAliasedShardReplicas( + handles: ReadonlyArray<{ key: string; replica: TClient; aliasOf?: string }> +): ReadonlyArray<{ key: string; replica: TClient }> { + return handles + .filter((handle) => handle.aliasOf === undefined) + .map((handle) => ({ key: handle.key, replica: handle.replica })); +} + +// One resolve, both derivations — `resolveShardHandles` reads a module export behind a try/catch. +const handles = resolveShardHandles(); +const maps = buildShardHandleMaps(handles); export const runOpsShardReplicas = maps.replicas; export const runOpsShardWriters = maps.writers; +// Empty unless RUN_OPS_SHARDS is configured, which is what keeps every fan-out leg unreachable today. +export const runOpsNonAliasedShardReplicas = nonAliasedShardReplicas(handles); diff --git a/apps/webapp/test/batchListPresenter.readroute.test.ts b/apps/webapp/test/batchListPresenter.readroute.test.ts index 9b1d55c9591..af5fcdbe2c6 100644 --- a/apps/webapp/test/batchListPresenter.readroute.test.ts +++ b/apps/webapp/test/batchListPresenter.readroute.test.ts @@ -16,14 +16,18 @@ vi.mock("~/db.server", () => ({ import { heteroPostgresTest, heteroRunOpsPostgresTest, + makeNShardRunOpsPostgresTest, postgresTest, } from "@internal/testcontainers"; +import { generateRunOpsId, generateRunOpsIdV2 } from "@trigger.dev/core/v3/isomorphic"; import type { PrismaClient } from "@trigger.dev/database"; import type { RunOpsPrismaClient } from "@internal/run-ops-database"; import { + type BatchList, type BatchListOptions, BatchListPresenter, } from "~/presenters/v3/BatchListPresenter.server"; +import { nonAliasedShardReplicas } from "~/v3/runOpsMigration/shardHandles.server"; vi.setConfig({ testTimeout: 120_000 }); @@ -163,7 +167,7 @@ async function mirrorEnvParents( } async function createBatch( - prisma: PrismaClient, + prisma: PrismaClient | RunOpsPrismaClient, ctx: SeedContext, batch: { id: string; @@ -174,7 +178,7 @@ async function createBatch( createdAt?: Date; } ) { - return prisma.batchTaskRun.create({ + return (prisma as PrismaClient).batchTaskRun.create({ data: { id: batch.id, friendlyId: batch.friendlyId, @@ -621,3 +625,270 @@ describe("BatchListPresenter run-ops read routing (PG14 control-plane/legacy + P } ); }); + +// Gen-2 shards. `#scanBatchTaskRun` used to scan exactly two clients — the gen-1 new store and the +// legacy replica — so a batch minted onto a gen-2 shard lived on neither and was silently absent +// from the list: no error, nothing logged, and a keyset merge with no leg that could return it. +describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard PG17 databases)", () => { + const twoShardTest = makeNShardRunOpsPostgresTest(2); + + // legacyPrisma carries the full control-plane schema, so it serves as BOTH the control-plane read + // handle and the legacy run-ops replica — the coresident topology, as the gen-1 cases above do. + const shardPresenter = ( + legacyPrisma: PrismaClient, + newPrisma: RunOpsPrismaClient, + shardReplicas: ReadonlyArray<{ key: string; replica: RunOpsPrismaClient }> + ) => + new BatchListPresenter(legacyPrisma, legacyPrisma, { + runOpsNew: newPrisma, + runOpsLegacyReplica: legacyPrisma as unknown as RunOpsPrismaClient, + controlPlaneReplica: legacyPrisma, + splitEnabled: true, + shardReplicas, + }); + + twoShardTest( + "a gen-2 batch on its shard appears in the list alongside gen-1 and legacy batches", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const shardA = shardPrismas[0]!; + const ctx = await seedParents(legacyPrisma, "gen2-visible"); + + const legacyId = "cmm00000000000000000legac"; + const newId = generateRunOpsId(); + const shardId = generateRunOpsIdV2("a"); + + // Oldest to newest, so the expected page order is the reverse. + await createBatch(legacyPrisma, ctx, { + id: legacyId, + friendlyId: "fr_legacy", + createdAt: new Date(Date.now() - 3 * 60_000), + }); + await createBatch(newPrisma, ctx, { + id: newId, + friendlyId: "fr_new", + createdAt: new Date(Date.now() - 2 * 60_000), + }); + await createBatch(shardA, ctx, { + id: shardId, + friendlyId: "fr_shard_a", + createdAt: new Date(Date.now() - 1 * 60_000), + }); + + const page = await shardPresenter(legacyPrisma, newPrisma, [ + { key: "a", replica: shardA }, + ]).call(baseCall(ctx, { pageSize: 10 })); + + expect(page.batches.map((b) => b.id)).toEqual([shardId, newId, legacyId]); + expect(page.batches.map((b) => b.friendlyId)).toContain("fr_shard_a"); + + // Without the shard leg the gen-2 batch is invisible — the defect this ticket fixes. + const withoutShardLeg = await shardPresenter(legacyPrisma, newPrisma, []).call( + baseCall(ctx, { pageSize: 10 }) + ); + expect(withoutShardLeg.batches.map((b) => b.id)).toEqual([newId, legacyId]); + } + ); + + twoShardTest( + "a page spanning legacy, new and two shards is ordered by createdAt then id across all stores", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const [shardA, shardB] = [shardPrismas[0]!, shardPrismas[1]!]; + const ctx = await seedParents(legacyPrisma, "gen2-order"); + + const t0 = new Date(Date.now() - 10 * 60_000); + const at = (minutes: number) => new Date(t0.getTime() + minutes * 60_000); + + const legacyId = "cmm00000000000000000order"; + const newId = generateRunOpsId(); + const shardAId = generateRunOpsIdV2("a"); + const shardBId = generateRunOpsIdV2("b"); + + await createBatch(legacyPrisma, ctx, { + id: legacyId, + friendlyId: "fr_o_legacy", + createdAt: at(0), + }); + await createBatch(newPrisma, ctx, { id: newId, friendlyId: "fr_o_new", createdAt: at(1) }); + await createBatch(shardA, ctx, { id: shardAId, friendlyId: "fr_o_a", createdAt: at(2) }); + await createBatch(shardB, ctx, { id: shardBId, friendlyId: "fr_o_b", createdAt: at(3) }); + + // An exact createdAt tie ACROSS two stores, so the id tiebreak is exercised over the seam. + const tieTime = at(4); + const tieOnNew = generateRunOpsId(); + const tieOnShardB = generateRunOpsIdV2("b"); + await createBatch(newPrisma, ctx, { + id: tieOnNew, + friendlyId: "fr_tie_new", + createdAt: tieTime, + }); + await createBatch(shardB, ctx, { + id: tieOnShardB, + friendlyId: "fr_tie_b", + createdAt: tieTime, + }); + + const page = await shardPresenter(legacyPrisma, newPrisma, [ + { key: "a", replica: shardA }, + { key: "b", replica: shardB }, + ]).call(baseCall(ctx, { pageSize: 10 })); + + // createdAt DESC, then id DESC (ASCII codepoint) — computed from the seed, not hand-written, + // so the assertion cannot drift from the ids the generators produce. + const tieHead = tieOnNew > tieOnShardB ? tieOnNew : tieOnShardB; + const tieTail = tieOnNew > tieOnShardB ? tieOnShardB : tieOnNew; + expect(page.batches.map((b) => b.id)).toEqual([ + tieHead, + tieTail, + shardBId, + shardAId, + newId, + legacyId, + ]); + } + ); + + twoShardTest( + "paging forward then backward across boundaries that span stores loses and repeats no batch", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const [shardA, shardB] = [shardPrismas[0]!, shardPrismas[1]!]; + const ctx = await seedParents(legacyPrisma, "gen2-paging"); + + const t0 = Date.now() - 60 * 60_000; + // Round-robin across all four stores so EVERY page boundary spans a store. + const seeded: string[] = []; + for (let i = 0; i < 8; i++) { + const store = [legacyPrisma, newPrisma, shardA, shardB][i % 4]!; + const id = + i % 4 === 0 + ? `cmm0000000000000000pag${i}` + : i % 4 === 1 + ? generateRunOpsId() + : generateRunOpsIdV2(i % 4 === 2 ? "a" : "b"); + await createBatch(store, ctx, { + id, + friendlyId: `fr_pag_${i}`, + createdAt: new Date(t0 + i * 60_000), + }); + seeded.push(id); + } + const newestFirst = [...seeded].reverse(); + + const presenter = shardPresenter(legacyPrisma, newPrisma, [ + { key: "a", replica: shardA }, + { key: "b", replica: shardB }, + ]); + + // Forward: walk every page to the end. + const forward: string[][] = []; + let cursor: string | undefined; + for (let guard = 0; guard < 10; guard++) { + const page = await presenter.call( + baseCall(ctx, { pageSize: 3, direction: "forward", cursor }) + ); + forward.push(page.batches.map((b) => b.id)); + if (!page.pagination.next) break; + cursor = page.pagination.next; + } + expect(forward.flat()).toEqual(newestFirst); + + // Backward: re-walk forward to capture the LAST page's `previous`, then page back to the start. + let backCursor: string | undefined; + cursor = undefined; + for (let guard = 0; guard < 10; guard++) { + const page = await presenter.call( + baseCall(ctx, { pageSize: 3, direction: "forward", cursor }) + ); + backCursor = page.pagination.previous; + if (!page.pagination.next) break; + cursor = page.pagination.next; + } + + const backward: string[][] = []; + for (let guard = 0; guard < 10 && backCursor; guard++) { + const page: BatchList = await presenter.call( + baseCall(ctx, { pageSize: 3, direction: "backward", cursor: backCursor }) + ); + backward.unshift(page.batches.map((b) => b.id)); + backCursor = page.pagination.previous; + } + + const backwardIds = backward.flat(); + // 8 rows at pageSize 3 => forward pages of 3, 3, 2. Paging back from the last page's cursor + // returns the 6 rows above it, in two pages. Assert the count so an empty backward walk + // cannot pass the ordering assertion vacuously. + expect(backwardIds).toHaveLength(6); + expect(new Set(backwardIds).size).toBe(backwardIds.length); + expect(backwardIds).toEqual(newestFirst.slice(0, 6)); + } + ); + + twoShardTest( + "the empty-state probe reports batches present when only a shard holds batches", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const shardB = shardPrismas[1]!; + const ctx = await seedParents(legacyPrisma, "gen2-probe"); + + // Nothing on legacy, nothing on new — only the shard. + await createBatch(shardB, ctx, { + id: generateRunOpsIdV2("b"), + friendlyId: "fr_probe_shard", + }); + + const presenter = shardPresenter(legacyPrisma, newPrisma, [ + { key: "a", replica: shardPrismas[0]! }, + { key: "b", replica: shardB }, + ]); + + // A friendlyId filter that matches nothing => empty page; the probe must still find the row. + const page = await presenter.call(baseCall(ctx, { friendlyId: "fr_does_not_exist" })); + expect(page.batches).toHaveLength(0); + expect(page.hasAnyBatches).toBe(true); + + // With no shard leg the same probe reports empty — the defect, on the empty-state path. + const withoutShardLeg = await shardPresenter(legacyPrisma, newPrisma, []).call( + baseCall(ctx, { friendlyId: "fr_does_not_exist" }) + ); + expect(withoutShardLeg.hasAnyBatches).toBe(false); + } + ); + + // Composition proof for the alias rule: a descriptor that declares `aliasOf` shares its target's + // client BY REFERENCE, so `nonAliasedShardReplicas` drops it and the target's own leg returns the + // rows. This is the soak topology — gen-2 ids living on the gen-1 new database. + twoShardTest( + "an aliased shard contributes no leg, and its rows still arrive once via the aliased store", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const shardB = shardPrismas[1]!; + const ctx = await seedParents(legacyPrisma, "gen2-alias"); + + // Soak: the gen-2 id is minted onto shard "a", which IS the gen-1 new database. + const soakId = generateRunOpsIdV2("a"); + const realShardId = generateRunOpsIdV2("b"); + await createBatch(newPrisma, ctx, { + id: soakId, + friendlyId: "fr_soak", + createdAt: new Date(Date.now() - 60_000), + }); + await createBatch(shardB, ctx, { id: realShardId, friendlyId: "fr_real_shard" }); + + // Spy on the aliased client so a kept alias leg is OBSERVABLE. Without this the dedupe would + // absorb the duplicate scan and the page assertion below could not fail. + const aliasedSpy = spyClient(newPrisma as unknown as PrismaClient); + + const legs = nonAliasedShardReplicas([ + { key: "a", replica: aliasedSpy.client as unknown as RunOpsPrismaClient, aliasOf: "new" }, + { key: "b", replica: shardB }, + ]); + expect(legs.map((leg) => leg.key)).toEqual(["b"]); + + const page = await shardPresenter( + legacyPrisma, + aliasedSpy.client as unknown as RunOpsPrismaClient, + legs + ).call(baseCall(ctx, { pageSize: 10 })); + expect(page.batches.map((b) => b.id)).toEqual([realShardId, soakId]); + // Scanned exactly once — as the gen-1 `new` leg, never again as an alias leg. + expect(aliasedSpy.counts.findMany).toBe(1); + } + ); +}); From 8d266f94a26b19652c3bb2e3ef538554c8bb096e Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 28 Aug 2026 11:32:46 +0100 Subject: [PATCH 2/6] test(webapp): pin the batches-list merge precedence The merge was only ever exercised as a union, with each id on exactly one store, so nothing caught a leg-order regression: a union is insensitive to the order its legs are applied in. Seed one id on two stores and assert the higher-authority copy is the one the page shows. Verified by mutation: making the lower-authority insert unconditional, and applying the legs in the wrong order, both fail it. Also corrects a comment that justified the leg order with a claim about duplicate ids being impossible. A single writer routes creates by id shape, but a row can still sit on two stores while data is moved between them, which is why the routing store dedupes batches without alarming. --- .../v3/BatchListPresenter.server.ts | 9 +++- .../test/batchListPresenter.readroute.test.ts | 52 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts index e489e260973..edb06e69c2d 100644 --- a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts +++ b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts @@ -142,8 +142,13 @@ export class BatchListPresenter extends BasePresenter { // The gen-1 pair keeps its original conditional form so its result stays byte-identical; the // shard legs then overwrite. Do NOT "simplify" the conditional insert below into an // unconditional one without also reordering the legs, or legacy would start winning over new. - // A gen-1 id and a gen-2 id cannot collide in any case: a batch is created on exactly one store, - // routed by its own id shape. + // + // Precedence is load-bearing, not decoration. Today a batch row has exactly one writer + // (PostgresRunStore.createBatchTaskRun), which routes by id shape, so a duplicate id is not + // produced by ordinary creates. But a row can still exist on two stores while data is being + // moved between them, which is why the routing store dedupes batches WITHOUT alarming + // (alarmOnDuplicate: false). When that happens the page must show the higher-authority copy, + // and that is what the leg order below decides. const byId = new Map(); for (const row of newRows) { byId.set(row.id, row); diff --git a/apps/webapp/test/batchListPresenter.readroute.test.ts b/apps/webapp/test/batchListPresenter.readroute.test.ts index af5fcdbe2c6..3c08d88c065 100644 --- a/apps/webapp/test/batchListPresenter.readroute.test.ts +++ b/apps/webapp/test/batchListPresenter.readroute.test.ts @@ -852,6 +852,58 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard } ); + // The merge is not just a union: when one id exists on more than one store, precedence decides + // which copy the page shows. Ascending legacy -> new -> shards, last write wins, mirroring the + // routing store's #mergeById. Without this case every leg-order regression still passes, because + // a union is order-insensitive. + twoShardTest( + "a duplicated id resolves by precedence: a shard copy outranks new, and new outranks legacy", + async ({ legacyPrisma, newPrisma, shardPrismas }) => { + const shardA = shardPrismas[0]!; + const ctx = await seedParents(legacyPrisma, "gen2-precedence"); + + // Same id on legacy AND new. New is the higher authority, so its copy must win. + const onBothGenOne = "cmm000000000000000000prec"; + await createBatch(legacyPrisma, ctx, { + id: onBothGenOne, + friendlyId: "fr_prec_gen1", + status: "PENDING", + createdAt: new Date(Date.now() - 60_000), + }); + await createBatch(newPrisma, ctx, { + id: onBothGenOne, + friendlyId: "fr_prec_gen1", + status: "COMPLETED", + createdAt: new Date(Date.now() - 60_000), + }); + + // Same id on new AND a shard. The shard is the higher authority, so its copy must win. + const onNewAndShard = generateRunOpsIdV2("a"); + await createBatch(newPrisma, ctx, { + id: onNewAndShard, + friendlyId: "fr_prec_shard", + status: "PENDING", + createdAt: new Date(Date.now() - 30_000), + }); + await createBatch(shardA, ctx, { + id: onNewAndShard, + friendlyId: "fr_prec_shard", + status: "COMPLETED", + createdAt: new Date(Date.now() - 30_000), + }); + + const page = await shardPresenter(legacyPrisma, newPrisma, [ + { key: "a", replica: shardA }, + ]).call(baseCall(ctx, { pageSize: 10 })); + + // Each id appears exactly once, and each carries the higher-authority store's status. + expect(page.batches.map((b) => b.id).filter((id) => id === onBothGenOne)).toHaveLength(1); + expect(page.batches.map((b) => b.id).filter((id) => id === onNewAndShard)).toHaveLength(1); + expect(page.batches.find((b) => b.id === onBothGenOne)?.status).toBe("COMPLETED"); + expect(page.batches.find((b) => b.id === onNewAndShard)?.status).toBe("COMPLETED"); + } + ); + // Composition proof for the alias rule: a descriptor that declares `aliasOf` shares its target's // client BY REFERENCE, so `nonAliasedShardReplicas` drops it and the target's own leg returns the // rows. This is the soak topology — gen-2 ids living on the gen-1 new database. From ce4539520d4b14904ce40d38045d472e55d3b659 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 28 Aug 2026 12:18:03 +0100 Subject: [PATCH 3/6] chore(webapp): drop the added comments from the batches-list scan The precedence test now guards the leg order that one of them warned about. --- .../v3/BatchListPresenter.server.ts | 45 ++----------------- .../shardHandles.server.test.ts | 2 - .../v3/runOpsMigration/shardHandles.server.ts | 19 -------- .../test/batchListPresenter.readroute.test.ts | 33 -------------- 4 files changed, 4 insertions(+), 95 deletions(-) diff --git a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts index edb06e69c2d..0d186279175 100644 --- a/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts +++ b/apps/webapp/app/presenters/v3/BatchListPresenter.server.ts @@ -75,13 +75,6 @@ export class BatchListPresenter extends BasePresenter { runOpsNew?: RunOpsPrismaClient; // new run-ops client (run-ops brand ⇒ guard classifies as runops) runOpsLegacyReplica?: RunOpsPrismaClient; // legacy run-ops READ REPLICA only — never the legacy primary controlPlaneReplica?: PrismaClientOrTransaction; // control-plane DB (for project) - // Gen-2 shard replicas to fan out over, in configured order — ONE ENTRY PER PHYSICAL DATABASE. - // The route passes `runOpsNonAliasedShardReplicas`, which has already dropped every shard that - // declares `aliasOf` (an alias shares its target's client by reference, so a leg for it would - // scan one database twice). Empty (RUN_OPS_SHARDS unset) keeps today's exact two legs. - // NOTE: a shard configured without a `replicaUrl` reads its WRITER. That is the established - // behaviour of every shard handle consumer; the "never the legacy primary" rule below is - // scoped to the LEGACY store, which has no replica-less arm. shardReplicas?: ReadonlyArray<{ key: string; replica: RunOpsPrismaClient }>; splitEnabled?: boolean; // resolved boot constant } @@ -114,20 +107,10 @@ export class BatchListPresenter extends BasePresenter { return scan(passthrough); } - // Always read EVERY store and merge. The old "skip legacy when new fills the page" shortcut is + // Always read BOTH stores and merge. The old "skip legacy when new fills the page" shortcut is // unsound across the residency split: legacy cuid ids ("c…") sort ABOVE new run-ops ids ("0…") // under id order, so a new-only page can hide pre-flip legacy batches that belong ahead of it. - // Ordering is by createdAt (id tiebreak), which is chronologically correct across every scheme. - // - // Every leg runs the SAME closure, so `take: pageSize + 1`, the cursor predicate and the two-key - // order are shared. A row's rank within its own leg is never worse than its global rank, so the - // true global first (pageSize + 1) rows are all present among the per-leg results — an argument - // in the number of legs, not in two. - // - // Promise.all, NOT allSettled: one store down must fail the page. A tolerant fan-out would - // return a SHORT page with no error, which is exactly the silent absence this routing exists to - // remove, with a wider blast radius. The routing store's own list fan-out is Promise.all for the - // same reason. + // Ordering is by createdAt (id tiebreak), which is chronologically correct across both schemes. const shardReplicas = this.readRoute.shardReplicas ?? []; const [newRows, legacyRows, ...shardRows] = await Promise.all([ scan(this.readRoute.runOpsNew ?? passthrough), @@ -135,20 +118,7 @@ export class BatchListPresenter extends BasePresenter { ...shardReplicas.map((shard) => scan(shard.replica)), ]); - // De-dupe by id, re-sort under the page's keyset order, re-apply the over-fetch LIMIT. - // - // Precedence ascends legacy → new → gen-2 shards, last write wins — the routing store's - // `#mergeById` semantics, which inserts unconditionally over legs supplied in precedence order. - // The gen-1 pair keeps its original conditional form so its result stays byte-identical; the - // shard legs then overwrite. Do NOT "simplify" the conditional insert below into an - // unconditional one without also reordering the legs, or legacy would start winning over new. - // - // Precedence is load-bearing, not decoration. Today a batch row has exactly one writer - // (PostgresRunStore.createBatchTaskRun), which routes by id shape, so a duplicate id is not - // produced by ordinary creates. But a row can still exist on two stores while data is being - // moved between them, which is why the routing store dedupes batches WITHOUT alarming - // (alarmOnDuplicate: false). When that happens the page must show the higher-authority copy, - // and that is what the leg order below decides. + // De-dupe by id (new wins), re-sort under the page's keyset order, re-apply the over-fetch LIMIT. const byId = new Map(); for (const row of newRows) { byId.set(row.id, row); @@ -178,8 +148,7 @@ export class BatchListPresenter extends BasePresenter { } // Empty-state probe. Split on: probe the new run-ops DB first, then the legacy READ REPLICA only - // (never the legacy primary), then every configured gen-2 shard in parallel. Split off (single-DB - // / self-host): one plain `_replica` probe. + // (never the legacy primary). Split off (single-DB / self-host): one plain `_replica` probe. async #probeAnyBatch(environmentId: string): Promise { // Single-DB / passthrough: `_replica` IS the run-ops database, and it is the SAME client the // scan uses, so the empty-state hint can't disagree with the page. Carry the run-ops brand @@ -210,12 +179,6 @@ export class BatchListPresenter extends BasePresenter { return true; } - // The gen-1 pair above keeps its sequential short-circuit — it is two legs, and a project with - // no batches yet is the common case for this path. The shards then go in ONE round trip instead - // of N: the probe answers a boolean, so there is no precedence to resolve and nothing to - // short-circuit for. `RoutingRunStore.#probeFirst` parallelises above two legs for the same - // reason, though it switches on the TOTAL leg count rather than keeping a sequential head — so - // an empty project with N shards costs 3 round trips here, not 1. const shardReplicas = this.readRoute.shardReplicas ?? []; if (shardReplicas.length === 0) { return false; diff --git a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts index f42fe1d8d5c..170b08b98ec 100644 --- a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts @@ -48,8 +48,6 @@ describe("nonAliasedShardReplicas", () => { ]); }); - // An aliased shard shares its target's client BY REFERENCE, so a leg for it scans one database - // twice. The router drops it the same way, on the DECLARATION rather than object identity. it("drops a shard that declares aliasOf", () => { expect(nonAliasedShardReplicas([{ ...handle("a"), aliasOf: "new" }, handle("b")])).toEqual([ { key: "b", replica: { tag: "b-replica" } }, diff --git a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts index 2f20ee6b33b..89c673663cc 100644 --- a/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts @@ -14,7 +14,6 @@ type ShardHandle = { key: string; writer: RunOpsPrismaClient; replica: RunOpsPrismaClient; - /** The DECLARED alias, if any. See nonAliasedShardReplicas. */ aliasOf?: string; }; @@ -43,22 +42,6 @@ function resolveShardHandles(): ShardHandle[] { } } -/** - * The shards a FAN-OUT should visit: one entry per physical database, in configured order. - * - * A shard that declares `aliasOf` shares its target's client BY REFERENCE (db.server.ts sets the - * alias target's own client into the shard map), so a leg for it would scan one database twice and - * return rows the target's own leg already returned. `RoutingRunStore` drops aliased keys from its - * store list for exactly this reason, and the discriminator there is the DECLARATION, not object - * identity — identity cannot tell an alias from its target, and a shard that shares a database - * WITHOUT declaring it is a misconfiguration the boot sentinel is there to catch, not something to - * paper over here. - * - * Routed lookups keyed by a single id want `runOpsShardReplicas` instead: an aliased key is a - * legitimate route target, it just is not a second database to scan. - * - * Generic in the client type so the caller keeps whatever type its handles carry — no cast. - */ export function nonAliasedShardReplicas( handles: ReadonlyArray<{ key: string; replica: TClient; aliasOf?: string }> ): ReadonlyArray<{ key: string; replica: TClient }> { @@ -67,11 +50,9 @@ export function nonAliasedShardReplicas( .map((handle) => ({ key: handle.key, replica: handle.replica })); } -// One resolve, both derivations — `resolveShardHandles` reads a module export behind a try/catch. const handles = resolveShardHandles(); const maps = buildShardHandleMaps(handles); export const runOpsShardReplicas = maps.replicas; export const runOpsShardWriters = maps.writers; -// Empty unless RUN_OPS_SHARDS is configured, which is what keeps every fan-out leg unreachable today. export const runOpsNonAliasedShardReplicas = nonAliasedShardReplicas(handles); diff --git a/apps/webapp/test/batchListPresenter.readroute.test.ts b/apps/webapp/test/batchListPresenter.readroute.test.ts index 3c08d88c065..e7026f2663d 100644 --- a/apps/webapp/test/batchListPresenter.readroute.test.ts +++ b/apps/webapp/test/batchListPresenter.readroute.test.ts @@ -626,14 +626,9 @@ describe("BatchListPresenter run-ops read routing (PG14 control-plane/legacy + P ); }); -// Gen-2 shards. `#scanBatchTaskRun` used to scan exactly two clients — the gen-1 new store and the -// legacy replica — so a batch minted onto a gen-2 shard lived on neither and was silently absent -// from the list: no error, nothing logged, and a keyset merge with no leg that could return it. describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard PG17 databases)", () => { const twoShardTest = makeNShardRunOpsPostgresTest(2); - // legacyPrisma carries the full control-plane schema, so it serves as BOTH the control-plane read - // handle and the legacy run-ops replica — the coresident topology, as the gen-1 cases above do. const shardPresenter = ( legacyPrisma: PrismaClient, newPrisma: RunOpsPrismaClient, @@ -657,7 +652,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard const newId = generateRunOpsId(); const shardId = generateRunOpsIdV2("a"); - // Oldest to newest, so the expected page order is the reverse. await createBatch(legacyPrisma, ctx, { id: legacyId, friendlyId: "fr_legacy", @@ -681,7 +675,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard expect(page.batches.map((b) => b.id)).toEqual([shardId, newId, legacyId]); expect(page.batches.map((b) => b.friendlyId)).toContain("fr_shard_a"); - // Without the shard leg the gen-2 batch is invisible — the defect this ticket fixes. const withoutShardLeg = await shardPresenter(legacyPrisma, newPrisma, []).call( baseCall(ctx, { pageSize: 10 }) ); @@ -712,7 +705,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard await createBatch(shardA, ctx, { id: shardAId, friendlyId: "fr_o_a", createdAt: at(2) }); await createBatch(shardB, ctx, { id: shardBId, friendlyId: "fr_o_b", createdAt: at(3) }); - // An exact createdAt tie ACROSS two stores, so the id tiebreak is exercised over the seam. const tieTime = at(4); const tieOnNew = generateRunOpsId(); const tieOnShardB = generateRunOpsIdV2("b"); @@ -732,8 +724,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard { key: "b", replica: shardB }, ]).call(baseCall(ctx, { pageSize: 10 })); - // createdAt DESC, then id DESC (ASCII codepoint) — computed from the seed, not hand-written, - // so the assertion cannot drift from the ids the generators produce. const tieHead = tieOnNew > tieOnShardB ? tieOnNew : tieOnShardB; const tieTail = tieOnNew > tieOnShardB ? tieOnShardB : tieOnNew; expect(page.batches.map((b) => b.id)).toEqual([ @@ -754,7 +744,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard const ctx = await seedParents(legacyPrisma, "gen2-paging"); const t0 = Date.now() - 60 * 60_000; - // Round-robin across all four stores so EVERY page boundary spans a store. const seeded: string[] = []; for (let i = 0; i < 8; i++) { const store = [legacyPrisma, newPrisma, shardA, shardB][i % 4]!; @@ -778,7 +767,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard { key: "b", replica: shardB }, ]); - // Forward: walk every page to the end. const forward: string[][] = []; let cursor: string | undefined; for (let guard = 0; guard < 10; guard++) { @@ -791,7 +779,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard } expect(forward.flat()).toEqual(newestFirst); - // Backward: re-walk forward to capture the LAST page's `previous`, then page back to the start. let backCursor: string | undefined; cursor = undefined; for (let guard = 0; guard < 10; guard++) { @@ -813,9 +800,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard } const backwardIds = backward.flat(); - // 8 rows at pageSize 3 => forward pages of 3, 3, 2. Paging back from the last page's cursor - // returns the 6 rows above it, in two pages. Assert the count so an empty backward walk - // cannot pass the ordering assertion vacuously. expect(backwardIds).toHaveLength(6); expect(new Set(backwardIds).size).toBe(backwardIds.length); expect(backwardIds).toEqual(newestFirst.slice(0, 6)); @@ -828,7 +812,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard const shardB = shardPrismas[1]!; const ctx = await seedParents(legacyPrisma, "gen2-probe"); - // Nothing on legacy, nothing on new — only the shard. await createBatch(shardB, ctx, { id: generateRunOpsIdV2("b"), friendlyId: "fr_probe_shard", @@ -839,12 +822,10 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard { key: "b", replica: shardB }, ]); - // A friendlyId filter that matches nothing => empty page; the probe must still find the row. const page = await presenter.call(baseCall(ctx, { friendlyId: "fr_does_not_exist" })); expect(page.batches).toHaveLength(0); expect(page.hasAnyBatches).toBe(true); - // With no shard leg the same probe reports empty — the defect, on the empty-state path. const withoutShardLeg = await shardPresenter(legacyPrisma, newPrisma, []).call( baseCall(ctx, { friendlyId: "fr_does_not_exist" }) ); @@ -852,17 +833,12 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard } ); - // The merge is not just a union: when one id exists on more than one store, precedence decides - // which copy the page shows. Ascending legacy -> new -> shards, last write wins, mirroring the - // routing store's #mergeById. Without this case every leg-order regression still passes, because - // a union is order-insensitive. twoShardTest( "a duplicated id resolves by precedence: a shard copy outranks new, and new outranks legacy", async ({ legacyPrisma, newPrisma, shardPrismas }) => { const shardA = shardPrismas[0]!; const ctx = await seedParents(legacyPrisma, "gen2-precedence"); - // Same id on legacy AND new. New is the higher authority, so its copy must win. const onBothGenOne = "cmm000000000000000000prec"; await createBatch(legacyPrisma, ctx, { id: onBothGenOne, @@ -877,7 +853,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard createdAt: new Date(Date.now() - 60_000), }); - // Same id on new AND a shard. The shard is the higher authority, so its copy must win. const onNewAndShard = generateRunOpsIdV2("a"); await createBatch(newPrisma, ctx, { id: onNewAndShard, @@ -896,7 +871,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard { key: "a", replica: shardA }, ]).call(baseCall(ctx, { pageSize: 10 })); - // Each id appears exactly once, and each carries the higher-authority store's status. expect(page.batches.map((b) => b.id).filter((id) => id === onBothGenOne)).toHaveLength(1); expect(page.batches.map((b) => b.id).filter((id) => id === onNewAndShard)).toHaveLength(1); expect(page.batches.find((b) => b.id === onBothGenOne)?.status).toBe("COMPLETED"); @@ -904,16 +878,12 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard } ); - // Composition proof for the alias rule: a descriptor that declares `aliasOf` shares its target's - // client BY REFERENCE, so `nonAliasedShardReplicas` drops it and the target's own leg returns the - // rows. This is the soak topology — gen-2 ids living on the gen-1 new database. twoShardTest( "an aliased shard contributes no leg, and its rows still arrive once via the aliased store", async ({ legacyPrisma, newPrisma, shardPrismas }) => { const shardB = shardPrismas[1]!; const ctx = await seedParents(legacyPrisma, "gen2-alias"); - // Soak: the gen-2 id is minted onto shard "a", which IS the gen-1 new database. const soakId = generateRunOpsIdV2("a"); const realShardId = generateRunOpsIdV2("b"); await createBatch(newPrisma, ctx, { @@ -923,8 +893,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard }); await createBatch(shardB, ctx, { id: realShardId, friendlyId: "fr_real_shard" }); - // Spy on the aliased client so a kept alias leg is OBSERVABLE. Without this the dedupe would - // absorb the duplicate scan and the page assertion below could not fail. const aliasedSpy = spyClient(newPrisma as unknown as PrismaClient); const legs = nonAliasedShardReplicas([ @@ -939,7 +907,6 @@ describe("BatchListPresenter gen-2 shard legs (legacy PG14 + new PG17 + 2 shard legs ).call(baseCall(ctx, { pageSize: 10 })); expect(page.batches.map((b) => b.id)).toEqual([realShardId, soakId]); - // Scanned exactly once — as the gen-1 `new` leg, never again as an alias leg. expect(aliasedSpy.counts.findMany).toBe(1); } ); From 50d18a8762a4b60af8d6e9587f2a1aaad00ab678 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 28 Aug 2026 12:25:04 +0100 Subject: [PATCH 4/6] fix(webapp): refuse to start when a run-ops shard is configured without the split Shard clients are only built on the split-on arm of the topology selector, so a shard configured while the split flag is off was dropped in silence: no client, no fan-out leg, and any row already resident on that database missing from every list with no error. The other two ways the split ends up disabled, a missing URL and a sentinel that cannot confirm distinct databases, already refuse to boot. This closes the one that did not, and names the dropped shards so the misconfiguration is obvious. --- apps/webapp/app/db.server.ts | 7 +++++ .../v3/runOpsMigration/splitMode.server.ts | 21 +++++++++++++ apps/webapp/test/runOpsSplitMode.test.ts | 30 +++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/apps/webapp/app/db.server.ts b/apps/webapp/app/db.server.ts index c63e0aa2d4f..a31151f9029 100644 --- a/apps/webapp/app/db.server.ts +++ b/apps/webapp/app/db.server.ts @@ -28,6 +28,7 @@ import { singleton } from "./utils/singleton"; import { registerDatabaseMetricsSource } from "./utils/databaseMetrics.server"; import { isSplitEnabled, + assertShardsRequireSplit, assertSplitRealtimeInterlock, } from "./v3/runOpsMigration/splitMode.server"; import { computeRunOpsSplitReadEnabled } from "./v3/runOpsMigration/runOpsSplitReadGate"; @@ -617,6 +618,12 @@ export const runOpsSplitReadEnabled: boolean = computeRunOpsSplitReadEnabled({ // interlock). Async, so it cannot live in the synchronous singleton factory — called // fire-and-forget from the eager-boot path (routing is wired synchronously at module load). export async function assertRunOpsSplitSentinel(): Promise { + // Shard interlock first: shard clients are only built on the split-on arm, so this case has to be + // checked BEFORE the split-off early return below, which would otherwise skip it in silence. + assertShardsRequireSplit({ + splitFlagEnabled: env.RUN_OPS_SPLIT_ENABLED, + shardKeys: env.RUN_OPS_SHARDS.map((shard) => shard.key), + }); if (!env.RUN_OPS_SPLIT_ENABLED) return; // Realtime interlock (synchronous): Electric replicates only from the control-plane // DB, so split-on without the native realtime backend leaves NEW-resident runs diff --git a/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts b/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts index b9a4e3dfdf2..b5819c7c5c5 100644 --- a/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts @@ -73,6 +73,27 @@ export function assertSplitRealtimeInterlock(config: SplitRealtimeInterlockConfi } } +export type ShardsRequireSplitConfig = { + splitFlagEnabled: boolean; + shardKeys: string[]; +}; + +/** + * Boot-time shard interlock (pure predicate). Shard clients are only built on the split-on arm of + * `selectRunOpsTopology`, so a shard configured while the split flag is off is dropped in silence: + * no client, no fan-out leg, and any row already resident on that database vanishes from every + * list with no error. The other two ways split can end up disabled (URLs missing, sentinel not + * distinct) already refuse to boot; this closes the one that does not. + */ +export function assertShardsRequireSplit(config: ShardsRequireSplitConfig): void { + if (config.splitFlagEnabled || config.shardKeys.length === 0) { + return; + } + throw new Error( + `RUN_OPS_SHARDS configures shard(s) ${config.shardKeys.join(", ")} but RUN_OPS_SPLIT_ENABLED is off, so no shard client is built and rows on those databases would be silently missing; refusing to start.` + ); +} + let cached: Promise | undefined; export function isSplitEnabled(): Promise { diff --git a/apps/webapp/test/runOpsSplitMode.test.ts b/apps/webapp/test/runOpsSplitMode.test.ts index fd7da6f356c..56b579b632c 100644 --- a/apps/webapp/test/runOpsSplitMode.test.ts +++ b/apps/webapp/test/runOpsSplitMode.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it, vi } from "vitest"; import { PostgreSqlContainer } from "@testcontainers/postgresql"; import { computeSplitEnabled, + assertShardsRequireSplit, assertSplitRealtimeInterlock, } from "~/v3/runOpsMigration/splitMode.server"; import { probeDistinctDatabases } from "~/v3/runOpsMigration/distinctDbSentinel.server"; @@ -155,6 +156,35 @@ describe("assertSplitRealtimeInterlock (pure)", () => { }); }); +describe("assertShardsRequireSplit (pure)", () => { + it("allows shards when the split flag is on", () => { + expect(() => + assertShardsRequireSplit({ splitFlagEnabled: true, shardKeys: ["a"] }) + ).not.toThrow(); + }); + + it("allows the split flag off when no shard is configured", () => { + expect(() => + assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: [] }) + ).not.toThrow(); + }); + + // Shards are only ever built on the split-on arm of selectRunOpsTopology, so configuring one + // while the split flag is off silently drops it: no shard client, no shard leg, and any row + // already resident on that database disappears from every list with no error. + it("refuses to boot when a shard is configured but the split flag is off", () => { + expect(() => + assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: ["a", "b"] }) + ).toThrow(/RUN_OPS_SHARDS/); + }); + + it("names the configured shards so the operator can see which were dropped", () => { + expect(() => + assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: ["a", "b"] }) + ).toThrow(/a, b/); + }); +}); + describe("distinct-DB sentinel (real Postgres)", () => { it("reports NOT distinct when both URLs hit the same physical cluster", async () => { const pg = await new PostgreSqlContainer("docker.io/postgres:14").start(); From 6973ca5bac95a069853ecc9cfc35009b8661fa3f Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 28 Aug 2026 12:51:04 +0100 Subject: [PATCH 5/6] fix(webapp): exempt aliased shards from the shard boot interlock An aliased shard owns no database: it shares its target's client by reference, so its rows are still read with the split off and nothing is dropped. Feeding every configured key to the interlock refused an alias-only config for no reason. The exemption now lives inside the interlock rather than at its call site, so it takes the raw descriptors and no caller can forget to apply it. This matches where the distinctness sentinel, the coresidency loop and replication already draw the line. --- apps/webapp/app/db.server.ts | 2 +- .../v3/runOpsMigration/splitMode.server.ts | 20 +++++++-- apps/webapp/test/runOpsSplitMode.test.ts | 45 ++++++++++++++----- 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/apps/webapp/app/db.server.ts b/apps/webapp/app/db.server.ts index a31151f9029..3b697eb22df 100644 --- a/apps/webapp/app/db.server.ts +++ b/apps/webapp/app/db.server.ts @@ -622,7 +622,7 @@ export async function assertRunOpsSplitSentinel(): Promise { // checked BEFORE the split-off early return below, which would otherwise skip it in silence. assertShardsRequireSplit({ splitFlagEnabled: env.RUN_OPS_SPLIT_ENABLED, - shardKeys: env.RUN_OPS_SHARDS.map((shard) => shard.key), + shards: env.RUN_OPS_SHARDS, }); if (!env.RUN_OPS_SPLIT_ENABLED) return; // Realtime interlock (synchronous): Electric replicates only from the control-plane diff --git a/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts b/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts index b5819c7c5c5..b62942cb87d 100644 --- a/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/splitMode.server.ts @@ -7,7 +7,11 @@ import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; import { probeDistinctStores as defaultProbe } from "./distinctDbSentinel.server"; -import { nonAliasedShards, type ShardTarget } from "~/v3/runOpsShards.server"; +import { + nonAliasedShards, + type RunOpsShardDescriptor, + type ShardTarget, +} from "~/v3/runOpsShards.server"; export type SplitModeConfig = { flagEnabled: boolean; @@ -75,7 +79,8 @@ export function assertSplitRealtimeInterlock(config: SplitRealtimeInterlockConfi export type ShardsRequireSplitConfig = { splitFlagEnabled: boolean; - shardKeys: string[]; + /** Raw descriptors. The alias exemption is applied here so no call site can forget it. */ + shards: RunOpsShardDescriptor[]; }; /** @@ -86,11 +91,18 @@ export type ShardsRequireSplitConfig = { * distinct) already refuse to boot; this closes the one that does not. */ export function assertShardsRequireSplit(config: ShardsRequireSplitConfig): void { - if (config.splitFlagEnabled || config.shardKeys.length === 0) { + if (config.splitFlagEnabled) { + return; + } + // An aliased shard owns no database: it shares its target's client by reference, so its rows are + // still read with the split off and nothing is dropped. Exempt here exactly as it is exempt from + // the distinctness sentinel, the coresidency loop and replication. + const owning = nonAliasedShards(config.shards).map((shard) => shard.key); + if (owning.length === 0) { return; } throw new Error( - `RUN_OPS_SHARDS configures shard(s) ${config.shardKeys.join(", ")} but RUN_OPS_SPLIT_ENABLED is off, so no shard client is built and rows on those databases would be silently missing; refusing to start.` + `RUN_OPS_SHARDS configures shard(s) ${owning.join(", ")} but RUN_OPS_SPLIT_ENABLED is off, so no shard client is built and rows on those databases would be silently missing; refusing to start.` ); } diff --git a/apps/webapp/test/runOpsSplitMode.test.ts b/apps/webapp/test/runOpsSplitMode.test.ts index 56b579b632c..efe2ddca16a 100644 --- a/apps/webapp/test/runOpsSplitMode.test.ts +++ b/apps/webapp/test/runOpsSplitMode.test.ts @@ -157,32 +157,55 @@ describe("assertSplitRealtimeInterlock (pure)", () => { }); describe("assertShardsRequireSplit (pure)", () => { + const owning = (key: string) => ({ + key, + region: "local", + url: `postgres://${key}`, + replication: { slotName: `s_${key}`, publicationName: `p_${key}`, originGeneration: 2 }, + }); + const aliased = (key: string) => ({ key, region: "local", aliasOf: "new" as const }); + it("allows shards when the split flag is on", () => { expect(() => - assertShardsRequireSplit({ splitFlagEnabled: true, shardKeys: ["a"] }) + assertShardsRequireSplit({ splitFlagEnabled: true, shards: [owning("a")] }) ).not.toThrow(); }); it("allows the split flag off when no shard is configured", () => { - expect(() => - assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: [] }) - ).not.toThrow(); + expect(() => assertShardsRequireSplit({ splitFlagEnabled: false, shards: [] })).not.toThrow(); }); - // Shards are only ever built on the split-on arm of selectRunOpsTopology, so configuring one - // while the split flag is off silently drops it: no shard client, no shard leg, and any row - // already resident on that database disappears from every list with no error. - it("refuses to boot when a shard is configured but the split flag is off", () => { + // Shards are only built on the split-on arm of selectRunOpsTopology, so configuring one while + // the split flag is off silently drops it: no client, no leg, and any row already resident on + // that database disappears from every list with no error. + it("refuses to boot when a shard that owns a database is configured but the split flag is off", () => { expect(() => - assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: ["a", "b"] }) + assertShardsRequireSplit({ splitFlagEnabled: false, shards: [owning("a"), owning("b")] }) ).toThrow(/RUN_OPS_SHARDS/); }); - it("names the configured shards so the operator can see which were dropped", () => { + it("names the dropped shards so the operator can see which ones they are", () => { expect(() => - assertShardsRequireSplit({ splitFlagEnabled: false, shardKeys: ["a", "b"] }) + assertShardsRequireSplit({ splitFlagEnabled: false, shards: [owning("a"), owning("b")] }) ).toThrow(/a, b/); }); + + // An aliased shard owns no database: it shares its target's client by reference, so its rows are + // still read with the split off. Refusing to boot for one is a false positive. + it("allows an alias-only config with the split flag off", () => { + expect(() => + assertShardsRequireSplit({ splitFlagEnabled: false, shards: [aliased("a")] }) + ).not.toThrow(); + }); + + it("refuses only for the owning shards when the config mixes both", () => { + expect(() => + assertShardsRequireSplit({ + splitFlagEnabled: false, + shards: [aliased("a"), owning("b")], + }) + ).toThrow(/shard\(s\) b /); + }); }); describe("distinct-DB sentinel (real Postgres)", () => { From 2c8a0514da24f1ef66d8f2c8be5b54da61341eec Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 28 Aug 2026 15:16:07 +0100 Subject: [PATCH 6/6] fix(webapp): retry the distinct-database probe before failing closed The probe fails closed, so one store being briefly unreachable collapsed the deployment to single-DB and the boot interlock then refused the boot. With more than two stores configured that turns a transient blip on any one of them into a fleet-wide startup failure. Each target now gets a bounded number of attempts with a short backoff before the probe gives up. Failing closed is unchanged once the budget is exhausted: "distinct" stays a positive claim a failed probe cannot support. A genuine duplicate is a final answer and is never retried, so a misconfigured deployment still fails on the first pass. Retries are per target, so one slow store does not re-probe the stores that already answered. --- .../distinctDbSentinel.server.ts | 60 ++++++++++++++- .../distinctDbSentinel.server.test.ts | 74 ++++++++++++++++++- 2 files changed, 129 insertions(+), 5 deletions(-) diff --git a/apps/webapp/app/v3/runOpsMigration/distinctDbSentinel.server.ts b/apps/webapp/app/v3/runOpsMigration/distinctDbSentinel.server.ts index ed7fb0cb237..b8349eba269 100644 --- a/apps/webapp/app/v3/runOpsMigration/distinctDbSentinel.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/distinctDbSentinel.server.ts @@ -64,6 +64,48 @@ export async function probeControlPlaneCoresidency( export type DistinctTarget = { id: string; url: string }; +/** Injection seam for the retry tests: no containers, no real waiting. */ +export type DistinctProbeOptions = { + logger?: { warn: (msg: string, meta?: Record) => void }; + readFingerprint?: (url: string) => Promise; + /** Total attempts per target, including the first. Bounded so boot latency stays bounded. */ + attempts?: number; + sleep?: (ms: number) => Promise; +}; + +const DEFAULT_PROBE_ATTEMPTS = 3; +const RETRY_BASE_DELAY_MS = 250; + +const defaultSleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Read one fingerprint, retrying a bounded number of times. + * + * The probe fails CLOSED, and that must not change: "distinct" is a positive claim a failed probe + * cannot support. But failing closed on the first blip means one shard being briefly unreachable + * collapses the deployment to single-DB, and the boot interlock then refuses the boot for the whole + * fleet. A transient error deserves a retry; a persistent one still fails closed, just later. + */ +async function readFingerprintWithRetry( + url: string, + read: (url: string) => Promise, + attempts: number, + sleep: (ms: number) => Promise +): Promise { + let lastError: unknown; + for (let attempt = 1; attempt <= attempts; attempt++) { + try { + return await read(url); + } catch (error) { + lastError = error; + if (attempt < attempts) { + await sleep(RETRY_BASE_DELAY_MS * attempt); + } + } + } + throw lastError; +} + /** * Set uniqueness over every store that owns its own database. Fail-closed: a probe that cannot * answer returns NOT distinct, because "distinct" is a positive claim a failed probe cannot support. @@ -77,14 +119,22 @@ export type DistinctTarget = { id: string; url: string }; */ export async function probeDistinctStores( targets: DistinctTarget[], - opts?: { logger?: { warn: (msg: string, meta?: Record) => void } } + opts?: DistinctProbeOptions ): Promise<{ distinct: true } | { distinct: false; reason: string }> { if (targets.length < 2) { return { distinct: true }; } + const read = opts?.readFingerprint ?? readDatabaseFingerprint; + const attempts = opts?.attempts ?? DEFAULT_PROBE_ATTEMPTS; + const sleep = opts?.sleep ?? defaultSleep; + try { - const fingerprints = await Promise.all(targets.map((t) => readDatabaseFingerprint(t.url))); + // Retry per TARGET, not around the whole set: one slow shard must not re-probe the stores that + // already answered. A duplicate verdict below is final and is never retried. + const fingerprints = await Promise.all( + targets.map((t) => readFingerprintWithRetry(t.url, read, attempts, sleep)) + ); const seen = new Map(); for (const [index, target] of targets.entries()) { @@ -104,7 +154,9 @@ export async function probeDistinctStores( return { distinct: true }; } catch (error) { - const reason = `distinct-db sentinel probe failed; failing closed (single-DB). ${String(error)}`; + const reason = + `distinct-db sentinel probe failed after ${opts?.attempts ?? DEFAULT_PROBE_ATTEMPTS} ` + + `attempt(s); failing closed (single-DB). ${String(error)}`; opts?.logger?.warn(reason, { error }); return { distinct: false, reason }; } @@ -115,7 +167,7 @@ export async function probeDistinctStores( export async function probeDistinctDatabases( legacyUrl: string, newUrl: string, - opts?: { logger?: { warn: (msg: string, meta?: Record) => void } } + opts?: DistinctProbeOptions ): Promise<{ distinct: true } | { distinct: false; reason: string }> { return probeDistinctStores( [ diff --git a/apps/webapp/test/v3/runOpsMigration/distinctDbSentinel.server.test.ts b/apps/webapp/test/v3/runOpsMigration/distinctDbSentinel.server.test.ts index 562d50b63d5..5ada87c5f9d 100644 --- a/apps/webapp/test/v3/runOpsMigration/distinctDbSentinel.server.test.ts +++ b/apps/webapp/test/v3/runOpsMigration/distinctDbSentinel.server.test.ts @@ -1,6 +1,6 @@ import { heteroPostgresTest } from "@internal/testcontainers"; import { PrismaClient } from "@trigger.dev/database"; -import { describe, expect, vi } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { probeDistinctDatabases, probeDistinctStores, @@ -66,6 +66,78 @@ describe("probeDistinctDatabases", () => { ); }); +// A transient failure on ONE store must not refuse the boot fleet-wide. The probe fails closed, +// so an unretried blip on any shard collapses the whole deployment to single-DB and the boot +// interlock then throws. Retry a bounded number of times, then fail closed exactly as before. +describe("probeDistinctStores bounded retry", () => { + const fp = (sysId: string, db: string) => ({ systemIdentifier: sysId, databaseName: db }); + + it("recovers when a transient failure clears within the retry budget", async () => { + let calls = 0; + const readFingerprint = vi.fn(async (url: string) => { + calls++; + if (calls === 2) throw new Error("ECONNREFUSED"); + return fp("sys", url); + }); + const result = await probeDistinctStores( + [ + { id: "new", url: "a" }, + { id: "shard-a", url: "b" }, + ], + { readFingerprint, attempts: 3, sleep: async () => {} } + ); + expect(result).toEqual({ distinct: true }); + }); + + it("fails closed once the retry budget is exhausted", async () => { + const readFingerprint = vi.fn(async () => { + throw new Error("ECONNREFUSED"); + }); + const result = await probeDistinctStores( + [ + { id: "new", url: "a" }, + { id: "shard-a", url: "b" }, + ], + { readFingerprint, attempts: 3, sleep: async () => {} } + ); + expect(result).toMatchObject({ distinct: false }); + }); + + it("bounds the attempts it makes", async () => { + const readFingerprint = vi.fn(async () => { + throw new Error("ECONNREFUSED"); + }); + await probeDistinctStores( + [ + { id: "a", url: "a" }, + { id: "b", url: "b" }, + ], + { + readFingerprint, + attempts: 3, + sleep: async () => {}, + } + ); + // 2 targets x 3 attempts each, and no more. + expect(readFingerprint).toHaveBeenCalledTimes(6); + }); + + // A duplicate is a correct, final answer. Retrying it would delay every boot of a genuinely + // misconfigured deployment for no benefit. + it("does not retry a genuine duplicate", async () => { + const readFingerprint = vi.fn(async () => fp("sys", "same")); + const result = await probeDistinctStores( + [ + { id: "new", url: "a" }, + { id: "shard-a", url: "b" }, + ], + { readFingerprint, attempts: 3, sleep: async () => {} } + ); + expect(result).toMatchObject({ distinct: false }); + expect(readFingerprint).toHaveBeenCalledTimes(2); + }); +}); + describe("probeDistinctStores (set uniqueness at N)", () => { heteroPostgresTest( "reports distinct for two separate physical clusters",