Skip to content

Commit 1b0f2c7

Browse files
matt-aitkenclaude
andauthored
fix(webapp): correct backward pagination slice in listRunIds (#3867)
## Problem Two backward-pagination bugs in `ClickHouseRunsRepository.listRunIds`, both pre-existing (they predate the composite-cursor work in #3852 and were spotted during/after it): **1. Wrong slice (straddled pages).** `listRunRows` fetches `page.size + 1` rows to detect `hasMore`. That extra row is the one *farthest from the cursor* in both directions (forward orders DESC; backward orders ASC), so it's always the *trailing* element. Forward correctly used `rows.slice(0, size)`, but backward+`hasMore` used `rows.slice(1, size + 1)` — dropping the row *closest* to the cursor and keeping the has-more sentinel. The page straddled two logical pages (one run from the correct previous page + one from the page before it), so paging "newer" across a boundary **repeated and skipped** runs. **2. Stranded forward cursor on a partial backward page.** In the backward `!hasMore` branch, `nextCursor` was `reversedRows.at(page.size - 1)`. On a partial page (fewer than `page.size` rows — reachable via `runs.list` by passing a forward page's cursor as `page[before]`), that index overshoots → `undefined` → `nextCursor` becomes `null`, leaving no way to page forward again. ## Fix - **Slice:** both directions now slice `rows.slice(0, size)` (the sentinel is the trailing element either way). - **Partial-page cursor:** the backward `!hasMore` branch takes the oldest row on the page, `rows.at(0)`, for `nextCursor` — equivalent to the old expression for full pages, correct for partial ones. Forward pagination, the cursor *values* for full pages, and the `hasMore === true` paths were already correct and are unchanged. ## Tests `runsRepositoryCursor.test.ts` gains two cases (both fail on `main`, pass here): - **multi-page backward walk:** forward across all pages, then backward from the last page — each backward page must *exactly* reproduce the corresponding forward page (no straddling: `main` returns `{b,c}` instead of `{c,d}`), and the full traversal covers every run once. - **partial backward page:** backward onto a partial first page must still expose a working forward cursor (and paging forward from it reaches the rest) — `main` returns a `null` nextCursor. The three existing cursor tests (forward completeness, backward round-trip, legacy cursor) still pass. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f4a96bd commit 1b0f2c7

3 files changed

Lines changed: 249 additions & 6 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Fix an off-by-one in `ClickHouseRunsRepository.listRunIds` backward pagination.
7+
When paging backward with more rows before the page (`hasMore`), the displayed
8+
page was sliced as `rows.slice(1, size + 1)`, which dropped the row closest to
9+
the cursor and kept the extra "has-more" sentinel — returning a page that
10+
straddled two logical pages (one row from the correct previous page plus one
11+
from the page before it). The result set is always the first `page.size` rows
12+
(the sentinel is the trailing element in both directions), so the slice is now
13+
`rows.slice(0, size)` for forward and backward alike. Forward pagination and the
14+
cursor values were already correct and are unchanged.

apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,17 +117,24 @@ export class ClickHouseRunsRepository implements IRunsRepository {
117117
previousCursor = cursorFor(reversedRows.at(1));
118118
nextCursor = cursorFor(reversedRows.at(options.page.size));
119119
} else {
120-
nextCursor = cursorFor(reversedRows.at(options.page.size - 1));
120+
// No newer rows, so there's no previous (newer) page. The next
121+
// (older) cursor is the oldest row on this page = rows[0] (rows are
122+
// ASC here). Index by the actual row count, not page.size — on a
123+
// partial page (fewer than page.size rows) page.size-1 overshoots
124+
// and would null the cursor, stranding forward navigation.
125+
nextCursor = cursorFor(rows.at(0));
121126
}
122127
break;
123128
}
124129
}
125130

126-
const runIds = (
127-
direction === "backward" && hasMore
128-
? rows.slice(1, options.page.size + 1)
129-
: rows.slice(0, options.page.size)
130-
).map((row) => row.runId);
131+
// The page is always the first `page.size` rows of the result. listRunRows
132+
// fetches one extra row only to detect `hasMore`; that extra row is the
133+
// farthest from the cursor in BOTH directions (forward orders DESC, backward
134+
// orders ASC), so it's always the trailing element to drop — never the
135+
// leading one. (Slicing `[1, size+1]` for backward dropped the row closest
136+
// to the cursor and kept the has-more sentinel, straddling two pages.)
137+
const runIds = rows.slice(0, options.page.size).map((row) => row.runId);
131138

132139
return { runIds, pagination: { nextCursor, previousCursor } };
133140
}

apps/webapp/test/runsRepositoryCursor.test.ts

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,4 +296,226 @@ describe("RunsRepository cursor pagination", () => {
296296
]);
297297
}
298298
);
299+
300+
replicationContainerTest(
301+
"backward pagination across multiple pages returns each page intact (no straddling)",
302+
async ({ clickhouseContainer, redisOptions, postgresContainer, prisma }) => {
303+
const { clickhouse } = await setupClickhouseReplication({
304+
prisma,
305+
databaseUrl: postgresContainer.getConnectionUri(),
306+
clickhouseUrl: clickhouseContainer.getConnectionUrl(),
307+
redisOptions,
308+
});
309+
310+
const organization = await prisma.organization.create({
311+
data: { title: "test", slug: "test" },
312+
});
313+
const project = await prisma.project.create({
314+
data: {
315+
name: "test",
316+
slug: "test",
317+
organizationId: organization.id,
318+
externalRef: "test",
319+
},
320+
});
321+
const runtimeEnvironment = await prisma.runtimeEnvironment.create({
322+
data: {
323+
slug: "test",
324+
type: "DEVELOPMENT",
325+
projectId: project.id,
326+
organizationId: organization.id,
327+
apiKey: "test",
328+
pkApiKey: "test",
329+
shortcode: "test",
330+
},
331+
});
332+
333+
// Five runs so a backward page has more rows before it (hasMore === true),
334+
// which is the case the off-by-one in the backward slice corrupts.
335+
const ids = [
336+
"aaaaaaaaaaaaaaaaaaaaaaaa",
337+
"bbbbbbbbbbbbbbbbbbbbbbbb",
338+
"cccccccccccccccccccccccc",
339+
"dddddddddddddddddddddddd",
340+
"eeeeeeeeeeeeeeeeeeeeeeee",
341+
];
342+
const base = new Date("2026-06-04T16:55:07.000Z").getTime();
343+
for (let i = 0; i < ids.length; i++) {
344+
await prisma.taskRun.create({
345+
data: {
346+
id: ids[i],
347+
createdAt: new Date(base + (ids.length - 1 - i) * 1000),
348+
friendlyId: `run_${ids[i]}`,
349+
taskIdentifier: "my-task",
350+
payload: JSON.stringify({ foo: "bar" }),
351+
traceId: `trace_${i}`,
352+
spanId: `span_${i}`,
353+
queue: "test",
354+
runtimeEnvironmentId: runtimeEnvironment.id,
355+
projectId: project.id,
356+
organizationId: organization.id,
357+
environmentType: "DEVELOPMENT",
358+
engine: "V2",
359+
},
360+
});
361+
}
362+
363+
await setTimeout(1000);
364+
365+
const runsRepository = new RunsRepository({ prisma, clickhouse });
366+
const baseOptions = {
367+
projectId: project.id,
368+
environmentId: runtimeEnvironment.id,
369+
organizationId: organization.id,
370+
};
371+
372+
const sortIds = (page: string[]) => page.slice().sort();
373+
374+
// Walk every forward page, capturing the run ids and the previousCursor.
375+
const forwardPages: Array<{ ids: string[]; previousCursor: string | null }> = [];
376+
let cursor: string | undefined = undefined;
377+
for (let guard = 0; guard < 20; guard++) {
378+
const page = await runsRepository.listRuns({
379+
...baseOptions,
380+
page: { size: 2, cursor, direction: cursor ? "forward" : undefined },
381+
});
382+
forwardPages.push({
383+
ids: page.runs.map((r) => r.id),
384+
previousCursor: page.pagination.previousCursor,
385+
});
386+
if (!page.pagination.nextCursor) break;
387+
cursor = page.pagination.nextCursor;
388+
}
389+
390+
// Forward pagination itself must cover every run exactly once, in 3 pages.
391+
expect(forwardPages.flatMap((p) => p.ids).sort()).toEqual(ids.slice().sort());
392+
expect(forwardPages).toHaveLength(3);
393+
394+
// Walk backward from the last page. Each backward page must equal the
395+
// corresponding forward page exactly — no row from an adjacent page
396+
// bleeding in (the straddle bug returned e.g. {b,c} instead of {c,d}).
397+
const backwardPages: string[][] = [];
398+
let backCursor: string | null = forwardPages[forwardPages.length - 1].previousCursor;
399+
for (let guard = 0; guard < 20 && backCursor; guard++) {
400+
const page = await runsRepository.listRuns({
401+
...baseOptions,
402+
page: { size: 2, cursor: backCursor, direction: "backward" },
403+
});
404+
backwardPages.push(page.runs.map((r) => r.id));
405+
backCursor = page.pagination.previousCursor;
406+
}
407+
408+
const expectedBackward = forwardPages
409+
.slice(0, -1)
410+
.reverse()
411+
.map((p) => sortIds(p.ids));
412+
expect(backwardPages.map(sortIds)).toEqual(expectedBackward);
413+
414+
// And the full backward traversal (last page + everything walked back to
415+
// the start) covers every run exactly once.
416+
const seen = [...forwardPages[forwardPages.length - 1].ids, ...backwardPages.flat()];
417+
expect(seen.slice().sort()).toEqual(ids.slice().sort());
418+
expect(new Set(seen).size).toBe(ids.length);
419+
}
420+
);
421+
422+
replicationContainerTest(
423+
"a partial backward page still exposes a forward cursor (no stranding)",
424+
async ({ clickhouseContainer, redisOptions, postgresContainer, prisma }) => {
425+
const { clickhouse } = await setupClickhouseReplication({
426+
prisma,
427+
databaseUrl: postgresContainer.getConnectionUri(),
428+
clickhouseUrl: clickhouseContainer.getConnectionUrl(),
429+
redisOptions,
430+
});
431+
432+
const organization = await prisma.organization.create({
433+
data: { title: "test", slug: "test" },
434+
});
435+
const project = await prisma.project.create({
436+
data: {
437+
name: "test",
438+
slug: "test",
439+
organizationId: organization.id,
440+
externalRef: "test",
441+
},
442+
});
443+
const runtimeEnvironment = await prisma.runtimeEnvironment.create({
444+
data: {
445+
slug: "test",
446+
type: "DEVELOPMENT",
447+
projectId: project.id,
448+
organizationId: organization.id,
449+
apiKey: "test",
450+
pkApiKey: "test",
451+
shortcode: "test",
452+
},
453+
});
454+
455+
// Three runs; created_at descending order is [a, b, c] (a newest).
456+
const ids = [
457+
"aaaaaaaaaaaaaaaaaaaaaaaa",
458+
"bbbbbbbbbbbbbbbbbbbbbbbb",
459+
"cccccccccccccccccccccccc",
460+
];
461+
const base = new Date("2026-06-04T16:55:07.000Z").getTime();
462+
for (let i = 0; i < ids.length; i++) {
463+
await prisma.taskRun.create({
464+
data: {
465+
id: ids[i],
466+
createdAt: new Date(base + (ids.length - 1 - i) * 1000),
467+
friendlyId: `run_${ids[i]}`,
468+
taskIdentifier: "my-task",
469+
payload: JSON.stringify({ foo: "bar" }),
470+
traceId: `trace_${i}`,
471+
spanId: `span_${i}`,
472+
queue: "test",
473+
runtimeEnvironmentId: runtimeEnvironment.id,
474+
projectId: project.id,
475+
organizationId: organization.id,
476+
environmentType: "DEVELOPMENT",
477+
engine: "V2",
478+
},
479+
});
480+
}
481+
482+
await setTimeout(1000);
483+
484+
const runsRepository = new RunsRepository({ prisma, clickhouse });
485+
const baseOptions = {
486+
projectId: project.id,
487+
environmentId: runtimeEnvironment.id,
488+
organizationId: organization.id,
489+
};
490+
491+
// First page (size 2) = {a, b}; its nextCursor sits at b's boundary.
492+
const first = await runsRepository.listRuns({ ...baseOptions, page: { size: 2 } });
493+
expect(first.runs.map((r) => r.id).sort()).toEqual([
494+
"aaaaaaaaaaaaaaaaaaaaaaaa",
495+
"bbbbbbbbbbbbbbbbbbbbbbbb",
496+
]);
497+
498+
// Paging backward from that cursor lands on a *partial* page — just the
499+
// newest run {a}, with no rows before it (hasMore === false).
500+
const back = await runsRepository.listRuns({
501+
...baseOptions,
502+
page: { size: 2, cursor: first.pagination.nextCursor!, direction: "backward" },
503+
});
504+
expect(back.runs.map((r) => r.id)).toEqual(["aaaaaaaaaaaaaaaaaaaaaaaa"]);
505+
506+
// A partial backward page must still expose a forward cursor, or the user
507+
// is stranded with no way to page back down.
508+
expect(back.pagination.nextCursor).toBeTruthy();
509+
510+
// And paging forward from it reaches the remaining runs.
511+
const forward = await runsRepository.listRuns({
512+
...baseOptions,
513+
page: { size: 2, cursor: back.pagination.nextCursor!, direction: "forward" },
514+
});
515+
expect(forward.runs.map((r) => r.id).sort()).toEqual([
516+
"bbbbbbbbbbbbbbbbbbbbbbbb",
517+
"cccccccccccccccccccccccc",
518+
]);
519+
}
520+
);
299521
});

0 commit comments

Comments
 (0)