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>
This commit is contained in:
@@ -0,0 +1,14 @@
|
||||
---
|
||||
area: webapp
|
||||
type: fix
|
||||
---
|
||||
|
||||
Fix an off-by-one in `ClickHouseRunsRepository.listRunIds` backward pagination.
|
||||
When paging backward with more rows before the page (`hasMore`), the displayed
|
||||
page was sliced as `rows.slice(1, size + 1)`, which dropped the row closest to
|
||||
the cursor and kept the extra "has-more" sentinel — returning a page that
|
||||
straddled two logical pages (one row from the correct previous page plus one
|
||||
from the page before it). The result set is always the first `page.size` rows
|
||||
(the sentinel is the trailing element in both directions), so the slice is now
|
||||
`rows.slice(0, size)` for forward and backward alike. Forward pagination and the
|
||||
cursor values were already correct and are unchanged.
|
||||
@@ -117,17 +117,24 @@ export class ClickHouseRunsRepository implements IRunsRepository {
|
||||
previousCursor = cursorFor(reversedRows.at(1));
|
||||
nextCursor = cursorFor(reversedRows.at(options.page.size));
|
||||
} else {
|
||||
nextCursor = cursorFor(reversedRows.at(options.page.size - 1));
|
||||
// No newer rows, so there's no previous (newer) page. The next
|
||||
// (older) cursor is the oldest row on this page = rows[0] (rows are
|
||||
// ASC here). Index by the actual row count, not page.size — on a
|
||||
// partial page (fewer than page.size rows) page.size-1 overshoots
|
||||
// and would null the cursor, stranding forward navigation.
|
||||
nextCursor = cursorFor(rows.at(0));
|
||||
}
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
const runIds = (
|
||||
direction === "backward" && hasMore
|
||||
? rows.slice(1, options.page.size + 1)
|
||||
: rows.slice(0, options.page.size)
|
||||
).map((row) => row.runId);
|
||||
// The page is always the first `page.size` rows of the result. listRunRows
|
||||
// fetches one extra row only to detect `hasMore`; that extra row is the
|
||||
// farthest from the cursor in BOTH directions (forward orders DESC, backward
|
||||
// orders ASC), so it's always the trailing element to drop — never the
|
||||
// leading one. (Slicing `[1, size+1]` for backward dropped the row closest
|
||||
// to the cursor and kept the has-more sentinel, straddling two pages.)
|
||||
const runIds = rows.slice(0, options.page.size).map((row) => row.runId);
|
||||
|
||||
return { runIds, pagination: { nextCursor, previousCursor } };
|
||||
}
|
||||
|
||||
@@ -296,4 +296,226 @@ describe("RunsRepository cursor pagination", () => {
|
||||
]);
|
||||
}
|
||||
);
|
||||
|
||||
replicationContainerTest(
|
||||
"backward pagination across multiple pages returns each page intact (no straddling)",
|
||||
async ({ clickhouseContainer, redisOptions, postgresContainer, prisma }) => {
|
||||
const { clickhouse } = await setupClickhouseReplication({
|
||||
prisma,
|
||||
databaseUrl: postgresContainer.getConnectionUri(),
|
||||
clickhouseUrl: clickhouseContainer.getConnectionUrl(),
|
||||
redisOptions,
|
||||
});
|
||||
|
||||
const organization = await prisma.organization.create({
|
||||
data: { title: "test", slug: "test" },
|
||||
});
|
||||
const project = await prisma.project.create({
|
||||
data: {
|
||||
name: "test",
|
||||
slug: "test",
|
||||
organizationId: organization.id,
|
||||
externalRef: "test",
|
||||
},
|
||||
});
|
||||
const runtimeEnvironment = await prisma.runtimeEnvironment.create({
|
||||
data: {
|
||||
slug: "test",
|
||||
type: "DEVELOPMENT",
|
||||
projectId: project.id,
|
||||
organizationId: organization.id,
|
||||
apiKey: "test",
|
||||
pkApiKey: "test",
|
||||
shortcode: "test",
|
||||
},
|
||||
});
|
||||
|
||||
// Five runs so a backward page has more rows before it (hasMore === true),
|
||||
// which is the case the off-by-one in the backward slice corrupts.
|
||||
const ids = [
|
||||
"aaaaaaaaaaaaaaaaaaaaaaaa",
|
||||
"bbbbbbbbbbbbbbbbbbbbbbbb",
|
||||
"cccccccccccccccccccccccc",
|
||||
"dddddddddddddddddddddddd",
|
||||
"eeeeeeeeeeeeeeeeeeeeeeee",
|
||||
];
|
||||
const base = new Date("2026-06-04T16:55:07.000Z").getTime();
|
||||
for (let i = 0; i < ids.length; i++) {
|
||||
await prisma.taskRun.create({
|
||||
data: {
|
||||
id: ids[i],
|
||||
createdAt: new Date(base + (ids.length - 1 - i) * 1000),
|
||||
friendlyId: `run_${ids[i]}`,
|
||||
taskIdentifier: "my-task",
|
||||
payload: JSON.stringify({ foo: "bar" }),
|
||||
traceId: `trace_${i}`,
|
||||
spanId: `span_${i}`,
|
||||
queue: "test",
|
||||
runtimeEnvironmentId: runtimeEnvironment.id,
|
||||
projectId: project.id,
|
||||
organizationId: organization.id,
|
||||
environmentType: "DEVELOPMENT",
|
||||
engine: "V2",
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
await setTimeout(1000);
|
||||
|
||||
const runsRepository = new RunsRepository({ prisma, clickhouse });
|
||||
const baseOptions = {
|
||||
projectId: project.id,
|
||||
environmentId: runtimeEnvironment.id,
|
||||
organizationId: organization.id,
|
||||
};
|
||||
|
||||
const sortIds = (page: string[]) => page.slice().sort();
|
||||
|
||||
// Walk every forward page, capturing the run ids and the previousCursor.
|
||||
const forwardPages: Array<{ ids: string[]; previousCursor: string | null }> = [];
|
||||
let cursor: string | undefined = undefined;
|
||||
for (let guard = 0; guard < 20; guard++) {
|
||||
const page = await runsRepository.listRuns({
|
||||
...baseOptions,
|
||||
page: { size: 2, cursor, direction: cursor ? "forward" : undefined },
|
||||
});
|
||||
forwardPages.push({
|
||||
ids: page.runs.map((r) => r.id),
|
||||
previousCursor: page.pagination.previousCursor,
|
||||
});
|
||||
if (!page.pagination.nextCursor) break;
|
||||
cursor = page.pagination.nextCursor;
|
||||
}
|
||||
|
||||
// Forward pagination itself must cover every run exactly once, in 3 pages.
|
||||
expect(forwardPages.flatMap((p) => p.ids).sort()).toEqual(ids.slice().sort());
|
||||
expect(forwardPages).toHaveLength(3);
|
||||
|
||||
// Walk backward from the last page. Each backward page must equal the
|
||||
// corresponding forward page exactly — no row from an adjacent page
|
||||
// bleeding in (the straddle bug returned e.g. {b,c} instead of {c,d}).
|
||||
const backwardPages: string[][] = [];
|
||||
let backCursor: string | null = forwardPages[forwardPages.length - 1].previousCursor;
|
||||
for (let guard = 0; guard < 20 && backCursor; guard++) {
|
||||
const page = await runsRepository.listRuns({
|
||||
...baseOptions,
|
||||
page: { size: 2, cursor: backCursor, direction: "backward" },
|
||||
});
|
||||
backwardPages.push(page.runs.map((r) => r.id));
|
||||
backCursor = page.pagination.previousCursor;
|
||||
}
|
||||
|
||||
const expectedBackward = forwardPages
|
||||
.slice(0, -1)
|
||||
.reverse()
|
||||
.map((p) => sortIds(p.ids));
|
||||
expect(backwardPages.map(sortIds)).toEqual(expectedBackward);
|
||||
|
||||
// And the full backward traversal (last page + everything walked back to
|
||||
// the start) covers every run exactly once.
|
||||
const seen = [...forwardPages[forwardPages.length - 1].ids, ...backwardPages.flat()];
|
||||
expect(seen.slice().sort()).toEqual(ids.slice().sort());
|
||||
expect(new Set(seen).size).toBe(ids.length);
|
||||
}
|
||||
);
|
||||
|
||||
replicationContainerTest(
|
||||
"a partial backward page still exposes a forward cursor (no stranding)",
|
||||
async ({ clickhouseContainer, redisOptions, postgresContainer, prisma }) => {
|
||||
const { clickhouse } = await setupClickhouseReplication({
|
||||
prisma,
|
||||
databaseUrl: postgresContainer.getConnectionUri(),
|
||||
clickhouseUrl: clickhouseContainer.getConnectionUrl(),
|
||||
redisOptions,
|
||||
});
|
||||
|
||||
const organization = await prisma.organization.create({
|
||||
data: { title: "test", slug: "test" },
|
||||
});
|
||||
const project = await prisma.project.create({
|
||||
data: {
|
||||
name: "test",
|
||||
slug: "test",
|
||||
organizationId: organization.id,
|
||||
externalRef: "test",
|
||||
},
|
||||
});
|
||||
const runtimeEnvironment = await prisma.runtimeEnvironment.create({
|
||||
data: {
|
||||
slug: "test",
|
||||
type: "DEVELOPMENT",
|
||||
projectId: project.id,
|
||||
organizationId: organization.id,
|
||||
apiKey: "test",
|
||||
pkApiKey: "test",
|
||||
shortcode: "test",
|
||||
},
|
||||
});
|
||||
|
||||
// Three runs; created_at descending order is [a, b, c] (a newest).
|
||||
const ids = [
|
||||
"aaaaaaaaaaaaaaaaaaaaaaaa",
|
||||
"bbbbbbbbbbbbbbbbbbbbbbbb",
|
||||
"cccccccccccccccccccccccc",
|
||||
];
|
||||
const base = new Date("2026-06-04T16:55:07.000Z").getTime();
|
||||
for (let i = 0; i < ids.length; i++) {
|
||||
await prisma.taskRun.create({
|
||||
data: {
|
||||
id: ids[i],
|
||||
createdAt: new Date(base + (ids.length - 1 - i) * 1000),
|
||||
friendlyId: `run_${ids[i]}`,
|
||||
taskIdentifier: "my-task",
|
||||
payload: JSON.stringify({ foo: "bar" }),
|
||||
traceId: `trace_${i}`,
|
||||
spanId: `span_${i}`,
|
||||
queue: "test",
|
||||
runtimeEnvironmentId: runtimeEnvironment.id,
|
||||
projectId: project.id,
|
||||
organizationId: organization.id,
|
||||
environmentType: "DEVELOPMENT",
|
||||
engine: "V2",
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
await setTimeout(1000);
|
||||
|
||||
const runsRepository = new RunsRepository({ prisma, clickhouse });
|
||||
const baseOptions = {
|
||||
projectId: project.id,
|
||||
environmentId: runtimeEnvironment.id,
|
||||
organizationId: organization.id,
|
||||
};
|
||||
|
||||
// First page (size 2) = {a, b}; its nextCursor sits at b's boundary.
|
||||
const first = await runsRepository.listRuns({ ...baseOptions, page: { size: 2 } });
|
||||
expect(first.runs.map((r) => r.id).sort()).toEqual([
|
||||
"aaaaaaaaaaaaaaaaaaaaaaaa",
|
||||
"bbbbbbbbbbbbbbbbbbbbbbbb",
|
||||
]);
|
||||
|
||||
// Paging backward from that cursor lands on a *partial* page — just the
|
||||
// newest run {a}, with no rows before it (hasMore === false).
|
||||
const back = await runsRepository.listRuns({
|
||||
...baseOptions,
|
||||
page: { size: 2, cursor: first.pagination.nextCursor!, direction: "backward" },
|
||||
});
|
||||
expect(back.runs.map((r) => r.id)).toEqual(["aaaaaaaaaaaaaaaaaaaaaaaa"]);
|
||||
|
||||
// A partial backward page must still expose a forward cursor, or the user
|
||||
// is stranded with no way to page back down.
|
||||
expect(back.pagination.nextCursor).toBeTruthy();
|
||||
|
||||
// And paging forward from it reaches the remaining runs.
|
||||
const forward = await runsRepository.listRuns({
|
||||
...baseOptions,
|
||||
page: { size: 2, cursor: back.pagination.nextCursor!, direction: "forward" },
|
||||
});
|
||||
expect(forward.runs.map((r) => r.id).sort()).toEqual([
|
||||
"bbbbbbbbbbbbbbbbbbbbbbbb",
|
||||
"cccccccccccccccccccccccc",
|
||||
]);
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user