From 5a4d971601a81952fd0b37a54ad581107d094510 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:20:45 +0900 Subject: [PATCH] fix(api): page the public anchor listing on (created_at, id), not created_at alone loadPublicLedgerAnchors's keyset cursor filtered `WHERE created_at < ?` while ordering `BY created_at DESC, id DESC`. Because the ORDER BY tiebreaks on id but the WHERE filtered on created_at alone, any group of rows sharing one created_at that straddled a page boundary was silently skipped -- the next page started strictly before that timestamp, so the tied remainder was unreachable on any page. Ties are real: recordLedgerAnchorAttempt defaults createdAt to nowIso() (millisecond precision) and one scheduler tick records an attempt per backend, so several rows land on the same created_at. An attempt -- including a failure, the row this public health endpoint exists to make observable -- could therefore be permanently unreachable through the paginated API while sitting in the table. Make the cursor match the sort key: the WHERE clause becomes the standard row-comparison form `(created_at < ? OR (created_at = ? AND id < ?))`, and nextBefore now encodes both (created_at, id) as one opaque `created_at|id` string. The field stays `string | null`, so the route and published response shape are unchanged for a caller that only round-trips the value. An unparseable `before` falls back to the first page rather than throwing, since this is a public route. The single prepared statement and the fetch-one-extra-row limit+1 technique are unchanged. Closes #9715 --- src/review/ledger-anchor-persistence.ts | 38 ++++++++++++++++---- test/unit/ledger-anchor-persistence.test.ts | 39 +++++++++++++++++++++ 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/review/ledger-anchor-persistence.ts b/src/review/ledger-anchor-persistence.ts index fadcad6346..13d5ad5ead 100644 --- a/src/review/ledger-anchor-persistence.ts +++ b/src/review/ledger-anchor-persistence.ts @@ -81,11 +81,28 @@ export async function recordLedgerAnchorAttempt(env: Env, attempt: LedgerAnchorA export type LedgerAnchorListFilter = { backend?: LedgerAnchorBackend; - /** Cursor: return rows strictly older than this ISO timestamp. Omit for the first (newest) page. */ + /** Opaque keyset cursor (a `created_at|id` pair, as returned in a prior page's `nextBefore`): resume strictly + * after that exact row in `(created_at DESC, id DESC)` order. Omit for the first (newest) page; an + * unparseable value is treated as "no cursor" rather than an error, since this is a public route. */ before?: string; limit?: number; }; +/** Encode a keyset cursor as one opaque string. `created_at` is an ISO timestamp and `id` a UUID, so neither + * contains a `|` -- the separator round-trips unambiguously and `parseAnchorCursor` splits on the first `|`. */ +function encodeAnchorCursor(createdAt: string, id: string): string { + return `${createdAt}|${id}`; +} + +/** Parse a `created_at|id` cursor. Returns null for a missing or malformed value (no `|`, or an empty + * component) so a bad `before` on this unauthenticated public route falls back to the first page, never throws. */ +function parseAnchorCursor(before: string | undefined): { createdAt: string; id: string } | null { + if (!before) return null; + const separator = before.indexOf("|"); + if (separator <= 0 || separator === before.length - 1) return null; + return { createdAt: before.slice(0, separator), id: before.slice(separator + 1) }; +} + const DEFAULT_ANCHOR_LIST_LIMIT = 50; const MAX_ANCHOR_LIST_LIMIT = 200; @@ -103,9 +120,14 @@ export async function loadPublicLedgerAnchors(env: Env, filter: LedgerAnchorList conditions.push("backend = ?"); binds.push(filter.backend); } - if (filter.before) { - conditions.push("created_at < ?"); - binds.push(filter.before); + const cursor = parseAnchorCursor(filter.before); + if (cursor) { + // Row-comparison keyset predicate matching ORDER BY created_at DESC, id DESC: resume strictly after the + // cursor row. Filtering on created_at alone (the prior behavior) silently skipped any rows that shared the + // cursor's created_at but sorted after it on the id tiebreak -- e.g. the several backends written in one + // scheduler tick at the same millisecond nowIso() (#9715). + conditions.push("(created_at < ? OR (created_at = ? AND id < ?))"); + binds.push(cursor.createdAt, cursor.createdAt, cursor.id); } const where = conditions.length > 0 ? `WHERE ${conditions.join(" AND ")}` : ""; // Fetch one extra row to know whether a next page exists, without a separate COUNT query. @@ -122,9 +144,11 @@ export async function loadPublicLedgerAnchors(env: Env, filter: LedgerAnchorList * guards a driver-shape change, mirroring loadPublicRulePrecision's identical note on COUNT(*). */ const results = rows.results ?? []; const page = results.slice(0, limit); - // `> limit` guarantees `page` has exactly `limit` (>=1) elements, so `page[page.length - 1]` is always - /* v8 ignore next -- defined; the ?. only guards TypeScript's array-index type, not a reachable runtime case. */ - const nextBefore = results.length > limit ? (page[page.length - 1]?.createdAt ?? null) : null; + // A fetched extra row (results.length > limit) means another page exists; encode BOTH (created_at, id) of the + // last returned row into the cursor so the next page resumes exactly after it and no created_at tie is skipped + // (#9715). `> limit` guarantees `page` has exactly `limit` (>=1) elements, so the last element is defined. + const lastRow = results.length > limit ? page[page.length - 1] : undefined; + const nextBefore = lastRow ? encodeAnchorCursor(lastRow.createdAt, lastRow.id) : null; const anchors: PublicLedgerAnchor[] = page.map((row) => ({ id: row.id, diff --git a/test/unit/ledger-anchor-persistence.test.ts b/test/unit/ledger-anchor-persistence.test.ts index a8884b058c..5d4cbb8285 100644 --- a/test/unit/ledger-anchor-persistence.test.ts +++ b/test/unit/ledger-anchor-persistence.test.ts @@ -103,6 +103,45 @@ describe("recordLedgerAnchorAttempt / loadPublicLedgerAnchors (#9271)", () => { expect(third.nextBefore).toBeNull(); // no more pages }); + it("REGRESSION (#9715): rows sharing one created_at are ALL returned across a page boundary, none skipped", async () => { + const env = createTestEnv(); + // One scheduler tick records an attempt per backend at the SAME millisecond createdAt -- exactly the tie + // the old `created_at < cursor` predicate skipped once it straddled a page. Four tied rows here. + const tiedAt = "2026-07-27T12:00:00.000Z"; + for (let seq = 1; seq <= 4; seq += 1) { + await recordLedgerAnchorAttempt( + env, + okAttempt({ payload: buildLedgerAnchorPayload({ seq, rowHash: `${seq}`.repeat(64).slice(0, 64), totalCount: seq }, tiedAt) }), + tiedAt, + ); + } + + // Page through the tie group in 2s. Before the fix the second page started strictly before `tiedAt`, so the + // two tied rows after the first page's boundary were silently unreachable. Collect every id across pages. + const seen = new Set(); + let before: string | null = null; + for (let guard = 0; guard < 10; guard += 1) { + const pageResult: { anchors: { id: string }[]; nextBefore: string | null } = await loadPublicLedgerAnchors(env, { limit: 2, ...(before ? { before } : {}) }); + for (const anchor of pageResult.anchors) seen.add(anchor.id); + before = pageResult.nextBefore; + if (before === null) break; + } + + // All four tied rows must be reachable through the paginated API -- the whole point of this endpoint. + expect(seen.size).toBe(4); + }); + + it("#9715: an unparseable `before` cursor falls back to the first page rather than throwing (public route)", async () => { + const env = createTestEnv(); + await recordLedgerAnchorAttempt(env, okAttempt()); + + // No `|` separator, an empty component, and a leading `|` are all malformed -> treated as no cursor. + for (const before of ["not-a-cursor", "2026-07-27T12:00:00.000Z", "|only-id", "ts-only|"]) { + const result = await loadPublicLedgerAnchors(env, { before }); + expect(result.anchors).toHaveLength(1); // first page, not an error, not empty + } + }); + it("clamps limit to [1, 200] rather than trusting caller input", async () => { const env = createTestEnv(); await recordLedgerAnchorAttempt(env, okAttempt());