Skip to content

Commit fd06ef4

Browse files
committed
fix(run-store): guard findRuns skip and skip the non-candidate table on id-list reads
findRuns now throws when given skip: offset pagination cannot span the two run tables, where each would independently skip N rows from its own result rather than N from the merged result. For an id-list predicate (id in [...]), it now queries only the table whose id format can contain those ids, avoiding a wasted query against an empty task_run_v2 while it is unpopulated during rollout.
1 parent e72d9fb commit fd06ef4

2 files changed

Lines changed: 120 additions & 9 deletions

File tree

internal-packages/run-store/src/PostgresRunStore.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { postgresTest } from "@internal/testcontainers";
22
import { isKsuidId, RunId } from "@trigger.dev/core/v3/isomorphic";
33
import type { PrismaClient } from "@trigger.dev/database";
4-
import { describe, expect } from "vitest";
4+
import { describe, expect, vi } from "vitest";
55
import { PostgresRunStore } from "./PostgresRunStore.js";
66
import type { CreateCancelledRunInput, CreateFailedRunInput, CreateRunInput } from "./types.js";
77

@@ -2378,6 +2378,68 @@ describe("PostgresRunStore — table routing by id format", () => {
23782378
).rejects.toThrow(/cursor/i);
23792379
}
23802380
);
2381+
2382+
postgresTest(
2383+
"findRuns rejects `skip` (offset pagination cannot span the two tables)",
2384+
async ({ prisma }) => {
2385+
const { environment } = await seedEnvironment(prisma);
2386+
const store = new PostgresRunStore({ prisma, readOnlyPrisma: prisma });
2387+
2388+
await expect(
2389+
store.findRuns({
2390+
where: { runtimeEnvironmentId: environment.id },
2391+
select: { id: true },
2392+
skip: 10,
2393+
take: 5,
2394+
})
2395+
).rejects.toThrow(/skip/i);
2396+
}
2397+
);
2398+
2399+
postgresTest(
2400+
"findRuns with an id-list partitions by id format and skips the table with no candidate ids",
2401+
async ({ prisma }) => {
2402+
const { organization, project, environment } = await seedEnvironment(prisma);
2403+
const store = new PostgresRunStore({ prisma, readOnlyPrisma: prisma });
2404+
2405+
const cuid = RunId.generate();
2406+
const ksuid = RunId.generateKsuid();
2407+
for (const r of [cuid, ksuid]) {
2408+
await seedRoutedRun(prisma, {
2409+
id: r.id,
2410+
friendlyId: r.friendlyId,
2411+
organizationId: organization.id,
2412+
projectId: project.id,
2413+
runtimeEnvironmentId: environment.id,
2414+
});
2415+
}
2416+
2417+
const ids = (rows: unknown) =>
2418+
(rows as Array<{ id: string }>).map((r) => r.id).sort();
2419+
2420+
// Mixed list: both tables queried, both runs returned.
2421+
expect(
2422+
ids(await store.findRuns({ where: { id: { in: [cuid.id, ksuid.id] } }, select: { id: true } }))
2423+
).toEqual([cuid.id, ksuid.id].sort());
2424+
2425+
// cuid-only list: the task_run_v2 query is skipped, legacy run still returned.
2426+
const v2Spy = vi.spyOn(prisma.taskRunV2, "findMany");
2427+
const legacyOnly = await store.findRuns({ where: { id: { in: [cuid.id] } }, select: { id: true } });
2428+
expect(ids(legacyOnly)).toEqual([cuid.id]);
2429+
expect(v2Spy).not.toHaveBeenCalled();
2430+
v2Spy.mockRestore();
2431+
2432+
// ksuid-only list: the TaskRun query is skipped, v2 run still returned.
2433+
const legacySpy = vi.spyOn(prisma.taskRun, "findMany");
2434+
const v2Only = await store.findRuns({ where: { id: { in: [ksuid.id] } }, select: { id: true } });
2435+
expect(ids(v2Only)).toEqual([ksuid.id]);
2436+
expect(legacySpy).not.toHaveBeenCalled();
2437+
legacySpy.mockRestore();
2438+
2439+
// Empty list matches nothing.
2440+
expect(ids(await store.findRuns({ where: { id: { in: [] } }, select: { id: true } }))).toEqual([]);
2441+
}
2442+
);
23812443
});
23822444

23832445
describe("PostgresRunStore — v2 nested writes (run + related rows via nested Prisma create)", () => {

internal-packages/run-store/src/PostgresRunStore.ts

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -844,14 +844,31 @@ export class PostgresRunStore implements RunStore {
844844
): Promise<unknown> {
845845
const prisma = client ?? this.readOnlyPrisma;
846846

847+
// Offset pagination can't be expressed across two tables: applying `skip`
848+
// to each table independently skips N rows from each, not N from the merged
849+
// result. Reject it rather than silently double-skip. No caller uses it;
850+
// cross-table reads keyset-paginate on a where + (createdAt, id) orderBy.
851+
if (args.skip !== undefined) {
852+
throw new Error(
853+
"RunStore.findRuns: `skip` (offset pagination) is not supported across the legacy TaskRun " +
854+
"and task_run_v2 tables. Use a where-based keyset (createdAt + id) instead."
855+
);
856+
}
857+
847858
// A run lives in exactly one physical table, chosen by its id format, so a
848-
// multi-row read must hit BOTH `TaskRun` (legacy cuid) and `task_run_v2`
849-
// (new ksuid) and combine. `task_run_v2` is an identical clone of `TaskRun`
850-
// (same relation surface), so the SAME `args` — crucially the SAME `where`,
851-
// which is the security scope — run unchanged against either delegate.
859+
// multi-row read generally hits BOTH `TaskRun` (legacy cuid) and
860+
// `task_run_v2` (new ksuid) and combines. `task_run_v2` is an identical
861+
// clone of `TaskRun` (same relation surface), so the SAME `args` (crucially
862+
// the SAME `where`, which is the security scope) run unchanged against
863+
// either delegate. When the predicate is an `id: { in: [...] }` list, the
864+
// table with no candidate ids is skipped (a cuid can't live in task_run_v2,
865+
// nor a ksuid in TaskRun), avoiding an empty query while task_run_v2 is
866+
// unpopulated during rollout.
852867
const legacyModel = prisma.taskRun;
853868
const v2Model = prisma.taskRunV2 as unknown as typeof prisma.taskRun;
854869

870+
const { queryLegacy, queryV2 } = this.#tablesForWhere(args.where);
871+
855872
const ordered = this.#normalizeOrderBy(args.orderBy);
856873

857874
// ORDERED + LIMITED → bounded 2-way merge.
@@ -881,8 +898,8 @@ export class PostgresRunStore implements RunStore {
881898
const perTableArgs = { ...queryArgs, take: args.take };
882899

883900
const [legacyRows, v2Rows] = (await Promise.all([
884-
legacyModel.findMany(perTableArgs),
885-
v2Model.findMany(perTableArgs),
901+
queryLegacy ? legacyModel.findMany(perTableArgs) : Promise.resolve([]),
902+
queryV2 ? v2Model.findMany(perTableArgs) : Promise.resolve([]),
886903
])) as [Array<Record<string, unknown>>, Array<Record<string, unknown>>];
887904

888905
const merged = this.#mergeOrdered(legacyRows, v2Rows, comparator, args.take);
@@ -901,8 +918,8 @@ export class PostgresRunStore implements RunStore {
901918
: { args, addedKeys: [] as string[] };
902919

903920
const [legacyRows, v2Rows] = (await Promise.all([
904-
legacyModel.findMany(queryArgs),
905-
v2Model.findMany(queryArgs),
921+
queryLegacy ? legacyModel.findMany(queryArgs) : Promise.resolve([]),
922+
queryV2 ? v2Model.findMany(queryArgs) : Promise.resolve([]),
906923
])) as [Array<Record<string, unknown>>, Array<Record<string, unknown>>];
907924

908925
let combined = legacyRows.concat(v2Rows);
@@ -924,6 +941,38 @@ export class PostgresRunStore implements RunStore {
924941
return this.#stripAddedKeys(combined, addedKeys);
925942
}
926943

944+
/**
945+
* Which physical tables a `findRuns` predicate can match. A run id encodes
946+
* its table, so an `id: { in: [...] }` list containing only cuids cannot match
947+
* `task_run_v2` (and a ksuid-only list cannot match `TaskRun`): the table with
948+
* no candidate ids is skipped, avoiding a wasted query against an empty
949+
* `task_run_v2` during rollout. An empty `in` list matches nothing, so both
950+
* are skipped. Any other predicate must consult both tables.
951+
*/
952+
#tablesForWhere(where: Prisma.TaskRunWhereInput): { queryLegacy: boolean; queryV2: boolean } {
953+
const idFilter = where.id;
954+
const idIn =
955+
idFilter !== null && typeof idFilter === "object" && "in" in idFilter
956+
? (idFilter as { in?: unknown }).in
957+
: undefined;
958+
959+
if (Array.isArray(idIn)) {
960+
let queryLegacy = false;
961+
let queryV2 = false;
962+
for (const id of idIn) {
963+
if (typeof id === "string" && isKsuidId(id)) {
964+
queryV2 = true;
965+
} else {
966+
queryLegacy = true;
967+
}
968+
if (queryLegacy && queryV2) break;
969+
}
970+
return { queryLegacy, queryV2 };
971+
}
972+
973+
return { queryLegacy: true, queryV2: true };
974+
}
975+
927976
/**
928977
* The cross-table merge/sort compares order-key VALUES read off each returned
929978
* row, so every scalar order key must be present in the projection. When the

0 commit comments

Comments
 (0)