fix(db): add a rowid tiebreak to signal_snapshots' latest-row reads

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})
         )

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