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"); + }); + }); });