From 28d8718224cba3ab28e104f377f89baaed6b31a0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:50:56 -0700 Subject: [PATCH] fix(db): add a rowid tiebreak to signal_snapshots' latest-row reads listSignalSnapshots ordered by generatedAt (millisecond precision) alone, and listLatestSignalSnapshotsForTargets used a secondary `id DESC` tiebreak -- id is a crypto.randomUUID(), with no relationship to insertion order. Two writes for the same (signalType, targetKey) within one millisecond (a realistic case: an API-record write immediately after a seeded/prior one) tie on generatedAt, and SQLite's own docs make no guarantee about tie order ("the order ... is undefined", sqlite.org/lang_select.html) -- so which row an unbroken ORDER BY tie returns is an accident of the current query plan, not a contract, and can change as the table grows or the engine version changes. Verified Cloudflare D1 (GA) currently routes every read for a database to a single active instance, so there's no live multi-replica divergence risk today -- but Cloudflare's own roadmap (a Sessions API with read replicas) would reintroduce exactly that risk, and SQLite's "insertion order" appearance is documented as incidental, not guaranteed, independent of replicas. Fixes both reads to use rowid (SQLite's actual monotonic per-insert counter) instead, matching this table's own documented invariant elsewhere in the codebase (retention.ts's dedupeSignalSnapshots: "'Latest' is the highest rowid per key ... rowid, unlike generated_at, can never tie") and the same pattern already used for orb_enrollments' own most-recently-inserted read (orb/relay.ts). A regression test with deliberately adversarial ids (chosen to sort alphabetically BACKWARDS from insertion order) confirms listLatestSignalSnapshotsForTargets' prior `id DESC` tiebreak was a real, reproducible bug -- reverting the fix makes that specific test fail. The listSignalSnapshots side couldn't be reproduced as a concrete bug against the local D1 test shim (its own untiebroken fallback happens to already match insertion order there), so its tests instead pin the fix's mechanism (genuine rowid-based order, not an id string comparison) as protection against a future regression, rather than proving today's local behavior is wrong. --- src/db/repositories.ts | 19 ++++++++-- test/unit/db-persistence.test.ts | 59 ++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 842b94203b..c8f7160502 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4933,7 +4933,18 @@ export async function listSignalSnapshots(env: Env, signalType: string, targetKe .select() .from(signalSnapshots) .where(and(eq(signalSnapshots.signalType, signalType), eq(signalSnapshots.targetKey, targetKey))) - .orderBy(desc(signalSnapshots.generatedAt)) + // A `rowid` tiebreak, not `id` (a random UUID with no insertion-order relationship): two writes for the + // same key within one millisecond (e.g. an API-record write immediately after a prior one) tie on + // generatedAt. SQLite's own docs guarantee nothing about which row an unbroken ORDER BY tie returns ("the + // order ... is undefined", sqlite.org/lang_select.html) -- an apparent insertion-order fallback here is an + // accident of the current query plan, not a contract, and the sibling window-function query below + // (listLatestSignalSnapshotsForTargets) proves the accident can genuinely go the wrong way: it used + // `id DESC` for this exact tiebreak and reliably picked the WRONG "latest" row once an id happened to sort + // out of insertion order (confirmed via an adversarial-id regression test, not just theory) before being + // fixed here too. Matches dedupeSignalSnapshots' own documented invariant for this exact table + // (retention.ts: "'Latest' is the highest rowid per key ... rowid, unlike generated_at, can never tie") and + // the same pattern orb/relay.ts already uses for its own "most recently inserted" read. + .orderBy(desc(signalSnapshots.generatedAt), desc(sql`rowid`)) .limit(100); return rows.map(toSignalSnapshotRecord); } @@ -4961,7 +4972,11 @@ export async function listLatestSignalSnapshotsForTargets( FROM ( SELECT id, signal_type, target_key, repo_full_name, generated_at, - row_number() OVER (PARTITION BY target_key ORDER BY generated_at DESC, id DESC) AS snapshot_rank + -- rowid, not id (a random UUID): the previous "generated_at DESC, id DESC" tiebreak reliably + -- returned the WRONG "latest" row on a same-millisecond tie whenever the two ids happened to sort + -- out of insertion order (confirmed via an adversarial-id regression test in db-persistence.test.ts, + -- not just theory). Matches listSignalSnapshots' own tiebreak -- see that function's doc comment. + row_number() OVER (PARTITION BY target_key ORDER BY generated_at DESC, rowid DESC) AS snapshot_rank FROM signal_snapshots WHERE signal_type = ? AND target_key IN (${placeholders}) ) diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index 63af43147d..106a0ad53c 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -6,10 +6,13 @@ import { hasActiveReviewForHeadSha, listContributorRepoStats, listLatestRepoGithubTotalsSnapshots, + listLatestSignalSnapshotsForTargets, listRepoPullRequestFilePaths, + listSignalSnapshots, persistBountyLifecycleEvent, persistRegistryDriftEvents, persistRepoGithubTotalsSnapshot, + persistSignalSnapshot, startActiveReviewTracking, terminalizeActiveReviewTracking, updateUpstreamDriftReportIssue, @@ -432,4 +435,60 @@ describe("active-review tracking (#review-evasion-protection)", () => { expect(await getActiveReviewStartedAt(env, "owner/repo", 1, "sha1")).toBe(row?.started_at); }); }); + + // signal_snapshots' "latest" tiebreak (investigated per an out-of-scope-flagged follow-up): generatedAt is + // millisecond-precision, so two writes for the same (signalType, targetKey) within one millisecond tie: SQLite + // itself makes no guarantee about tie order ("the order ... is undefined" -- sqlite.org/lang_select.html), so + // an id-string-based tiebreak (the desc(id) convention used elsewhere in this file, e.g. audit_events, + // review_suppression) can't substitute for real insertion order -- id here is a random crypto.randomUUID(), + // not a sortable sequence. rowid (SQLite's own monotonic per-insert counter) is the only value that actually + // reflects insertion order, matching this table's own documented invariant in retention.ts's + // dedupeSignalSnapshots ("'Latest' is the highest rowid per key ... rowid, unlike generated_at, can never + // tie") and the same tiebreak orb/relay.ts already uses for its own "most recently inserted" read. + describe("signal_snapshots: rowid tiebreak on a generatedAt tie", () => { + async function seedTiedPair(env: Env, targetKey: string, firstId: string, secondId: string, generatedAt: string) { + // Deliberately adversarial ids: `firstId` (inserted FIRST) sorts ALPHABETICALLY AFTER `secondId` (inserted + // SECOND). A tiebreak that (wrongly) compared `id` strings instead of `rowid` would pick the WRONG row -- + // this is what actually discriminates "genuine insertion order" from "an id string happens to sort right". + await persistSignalSnapshot(env, { id: firstId, signalType: "debug-signal", targetKey, repoFullName: null, payload: { marker: "first" }, generatedAt }); + await persistSignalSnapshot(env, { id: secondId, signalType: "debug-signal", targetKey, repoFullName: null, payload: { marker: "second" }, generatedAt }); + } + + it("listSignalSnapshots: the row inserted SECOND sorts first on a tie, even when its id sorts alphabetically BEFORE the first row's id", async () => { + const env = createTestEnv(); + await seedTiedPair(env, "repo-a", "zzz-inserted-first", "aaa-inserted-second", "2026-01-01T00:00:00.000Z"); + + const rows = await listSignalSnapshots(env, "debug-signal", "repo-a"); + expect(rows).toHaveLength(2); + expect(rows[0]?.id).toBe("aaa-inserted-second"); + expect(rows[0]?.payload).toMatchObject({ marker: "second" }); + expect(rows[1]?.id).toBe("zzz-inserted-first"); + }); + + it("REGRESSION: a THIRD write with an id that sorts alphabetically in the MIDDLE still slots by insertion order, not id order", async () => { + const env = createTestEnv(); + await seedTiedPair(env, "repo-b", "zzz-first", "aaa-second", "2026-01-01T00:00:00.000Z"); + await persistSignalSnapshot(env, { id: "mmm-third", signalType: "debug-signal", targetKey: "repo-b", repoFullName: null, payload: { marker: "third" }, generatedAt: "2026-01-01T00:00:00.000Z" }); + + const rows = await listSignalSnapshots(env, "debug-signal", "repo-b"); + expect(rows.map((r) => r.payload.marker)).toEqual(["third", "second", "first"]); // reverse insertion order + }); + + it("listLatestSignalSnapshotsForTargets: the row inserted SECOND wins the per-target 'latest' rank, even when its id sorts alphabetically BEFORE the first row's id", async () => { + const env = createTestEnv(); + await seedTiedPair(env, "repo-c", "zzz-inserted-first", "aaa-inserted-second", "2026-01-01T00:00:00.000Z"); + + const latest = await listLatestSignalSnapshotsForTargets(env, "debug-signal", ["repo-c"]); + expect(latest.get("repo-c")?.id).toBe("aaa-inserted-second"); + }); + + it("a genuinely later generatedAt still wins outright, tiebreak or not", async () => { + const env = createTestEnv(); + await persistSignalSnapshot(env, { id: "old-row", signalType: "debug-signal", targetKey: "repo-d", repoFullName: null, payload: { marker: "old" }, generatedAt: "2026-01-01T00:00:00.000Z" }); + await persistSignalSnapshot(env, { id: "new-row", signalType: "debug-signal", targetKey: "repo-d", repoFullName: null, payload: { marker: "new" }, generatedAt: "2026-01-02T00:00:00.000Z" }); + + const rows = await listSignalSnapshots(env, "debug-signal", "repo-d"); + expect(rows[0]?.id).toBe("new-row"); + }); + }); });