fix(db): add id tiebreak to product-usage rollup and review-suppression queries

src/db/repositories.ts

@@ -5332,7 +5332,10 @@ async function pruneReviewSuppressionsOverCap(env: Env, repoFullName: string): P
     .select({ id: reviewSuppression.id })
     .from(reviewSuppression)
     .where(eq(reviewSuppression.repoFullName, repoFullName))
-    .orderBy(desc(reviewSuppression.createdAt));
+    // #4501: an `id` tiebreak makes eviction deterministic under same-millisecond createdAt ties (e.g. a
+    // `@gittensory resolve` whole-PR command's Promise.all batch of suppression writes) -- without it, which
+    // row is "the oldest" past the cap is query-plan-dependent and can vary run to run.
+    .orderBy(desc(reviewSuppression.createdAt), desc(reviewSuppression.id));
   const overflow = rows.slice(MAX_REVIEW_SUPPRESSIONS_PER_REPO);
   if (overflow.length === 0) return;
   await db.delete(reviewSuppression).where(
@@ -5353,7 +5356,8 @@ export async function listReviewSuppressions(env: Env, repoFullName: string, lim
     .select()
     .from(reviewSuppression)
     .where(eq(reviewSuppression.repoFullName, boundedString(repoFullName, 200)))
-    .orderBy(desc(reviewSuppression.createdAt))
+    // Matches pruneReviewSuppressionsOverCap's tiebreak so the two agree on relative order under ties.
+    .orderBy(desc(reviewSuppression.createdAt), desc(reviewSuppression.id))
     .limit(clampInteger(limit, 1, MAX_REVIEW_SUPPRESSIONS_PER_REPO));
   return rows.map(toReviewSuppressionRecord);
 }
@@ -6539,7 +6543,9 @@ async function upsertProductUsageDailyRollup(env: Env, day: string, generatedAt:
     .select()
     .from(productUsageEvents)
     .where(and(gte(productUsageEvents.occurredAt, startIso), sql`${productUsageEvents.occurredAt} < ${endIso}`))
-    .orderBy(productUsageEvents.occurredAt)
+    // #4501: an `id` tiebreak makes which rows survive the cap below deterministic under same-millisecond
+    // occurredAt ties -- without it, row order (and therefore this persisted rollup) is query-plan-dependent.
+    .orderBy(productUsageEvents.occurredAt, productUsageEvents.id)
     .limit(PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT + 1);
   const capped = rows.length > PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT || sourceEventCount > PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT;
   const events = rows.slice(0, PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT).map(toProductUsageEventRecord);
@@ -6550,7 +6556,7 @@ async function upsertProductUsageDailyRollup(env: Env, day: string, generatedAt:
     .select()
     .from(productUsageEvents)
     .where(retentionWhere)
-    .orderBy(desc(productUsageEvents.occurredAt))
+    .orderBy(desc(productUsageEvents.occurredAt), desc(productUsageEvents.id))
     .limit(PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT + 1);
   const retentionCapped = retentionRows.length > PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT || Number(retentionSourceRow?.count ?? 0) > PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT;
   const retentionEvents = retentionRows.slice(0, PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT).map(toProductUsageEventRecord);

test/unit/product-usage.test.ts

@@ -1110,4 +1110,83 @@ describe("product usage events", () => {
       ]),
     });
   });
+
+  it("REGRESSION (#4501): the daily rollup's event scan is stable across repeated hourly re-runs when events tie on occurredAt at the scan-cap boundary", async () => {
+    const env = createTestEnv();
+    const day = "2026-05-29";
+    const startMs = Date.parse(`${day}T00:00:00.000Z`);
+    const FILLER_COUNT = 4996;
+    await env.DB.batch(
+      Array.from({ length: FILLER_COUNT }, (_, index) =>
+        env.DB.prepare(
+          "insert into product_usage_events (id, surface, role, event_name, route, actor_hash, session_hash, repo_full_name, target_key, outcome, latency_ms, client_name, client_version, metadata_json, occurred_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
+        ).bind(`filler-${index}`, "api", "miner", "filler_event", "/v1/filler", null, null, null, null, "success", null, null, null, "{}", new Date(startMs + index * 10).toISOString()),
+      ),
+    );
+    // 5 events sharing ONE occurredAt right at the scan-cap boundary, each with its OWN eventName so the
+    // rollup's byEvent output reveals exactly which ones survived, inserted in a SCRAMBLED (non-id-sorted)
+    // order -- without the #4501 id tiebreak, which 4 of these 5 fall inside PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT
+    // is query-plan-dependent and could silently change hour to hour as index.ts re-enqueues this rollup.
+    const tiedIso = new Date(startMs + FILLER_COUNT * 10).toISOString();
+    const scrambledTiedIds = ["tied-c", "tied-e", "tied-a", "tied-d", "tied-b"];
+    await env.DB.batch(
+      scrambledTiedIds.map((id) =>
+        env.DB.prepare(
+          "insert into product_usage_events (id, surface, role, event_name, route, actor_hash, session_hash, repo_full_name, target_key, outcome, latency_ms, client_name, client_version, metadata_json, occurred_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
+        ).bind(id, "api", "miner", id, "/v1/tied", null, null, null, null, "success", null, null, null, "{}", tiedIso),
+      ),
+    );
+
+    const firstRun = await rollupProductUsageDaily(env, { day, nowIso: `${day}T23:00:00.000Z` });
+    const secondRun = await rollupProductUsageDaily(env, { day, nowIso: `${day}T23:00:00.000Z` });
+
+    expect(firstRun.rollups[0]).toMatchObject({ status: "incomplete", totalEvents: FILLER_COUNT + 5, sourceEventCount: FILLER_COUNT + 5 });
+    // 4996 filler + 5 tied = 5001 sourced; the cap keeps the EARLIEST 5000 by (occurredAt, id) -- deterministically
+    // the 4 tied rows with the LOWEST id, never whichever 4 the query planner happens to scan first.
+    const tiedEventNames = firstRun.rollups[0]?.byEvent.filter((entry) => entry.eventName.startsWith("tied-")).map((entry) => entry.eventName);
+    expect(new Set(tiedEventNames)).toEqual(new Set(["tied-a", "tied-b", "tied-c", "tied-d"]));
+    // REGRESSION: byte-identical across the repeated ("hourly re-run") call -- no drift on unchanged source data.
+    expect(JSON.stringify(secondRun.rollups[0])).toBe(JSON.stringify(firstRun.rollups[0]));
+  });
+
+  it("INVARIANT (#4501): the retention scan's cap boundary is governed by the id tiebreak, not insertion order, when events tie on occurredAt", async () => {
+    const env = createTestEnv({ PRODUCT_USAGE_HASH_SALT: "fixed-test-salt" });
+    const day = "2026-06-20";
+    const previousDay = "2026-06-10";
+    const previousStartMs = Date.parse(`${previousDay}T00:00:00.000Z`);
+    // 4996 retention-window events with distinct timestamps AFTER (newer than) the tied group below -- the
+    // retention scan keeps the NEWEST N, so this pushes the cap boundary to land inside the tied group.
+    await env.DB.batch(
+      Array.from({ length: 4996 }, (_, index) =>
+        env.DB.prepare(
+          "insert into product_usage_events (id, surface, role, event_name, route, actor_hash, session_hash, repo_full_name, target_key, outcome, latency_ms, client_name, client_version, metadata_json, occurred_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
+        ).bind(`retention-filler-${index}`, "mcp", "miner", "mcp_request", "/mcp", `filler-actor-${index}`, null, null, null, "success", null, null, null, JSON.stringify({ role: "miner" }), new Date(previousStartMs + (index + 1) * 1000).toISOString()),
+      ),
+    );
+    // 5 retention-window events sharing ONE (earliest, boundary-straddling) occurredAt, each a distinct actor,
+    // inserted in a SCRAMBLED (non-id-sorted) order -- desc(id) keeps "tied-e" and evicts "tied-a" among ties.
+    const tiedIso = new Date(previousStartMs).toISOString();
+    const scrambledTiedIds = ["tied-c", "tied-e", "tied-a", "tied-d", "tied-b"];
+    await env.DB.batch(
+      scrambledTiedIds.map((id) =>
+        env.DB.prepare(
+          "insert into product_usage_events (id, surface, role, event_name, route, actor_hash, session_hash, repo_full_name, target_key, outcome, latency_ms, client_name, client_version, metadata_json, occurred_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
+        ).bind(id, "mcp", "miner", "mcp_request", "/mcp", `${id}-hash`, null, null, null, "success", null, null, null, JSON.stringify({ role: "miner" }), tiedIso),
+      ),
+    );
+    // Current-day event from "tied-a" -- the LOWEST id among the tied group, so #4501's desc(id) tiebreak
+    // deterministically EVICTS it from the retention scan; without the fix this could flip either way.
+    await env.DB.prepare(
+      "insert into product_usage_events (id, surface, role, event_name, route, actor_hash, session_hash, repo_full_name, target_key, outcome, latency_ms, client_name, client_version, metadata_json, occurred_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
+    )
+      .bind("retention-current-event", "mcp", "miner", "mcp_request", "/mcp", "tied-a-hash", null, null, null, "success", null, null, null, JSON.stringify({ role: "miner" }), `${day}T01:00:00.000Z`)
+      .run();
+
+    const result = await rollupProductUsageDaily(env, { day, nowIso: "2026-06-21T00:00:00.000Z" });
+
+    expect(result.rollups[0]).toMatchObject({
+      day,
+      retention: expect.arrayContaining([expect.objectContaining({ window: "previous_30_days", capped: true, activeActors: 1, retainedActors: 0 })]),
+    });
+  });
 });

test/unit/review-memory-store.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 import { MAX_REVIEW_SUPPRESSIONS_PER_REPO, listReviewSuppressions, recordReviewSuppression } from "../../src/db/repositories";
 import { createTestEnv } from "../helpers/d1";
 
@@ -87,9 +87,20 @@ describe("review-memory suppression store (#2178)", () => {
 
   it("enforces the per-repo bound: once a repo exceeds MAX_REVIEW_SUPPRESSIONS_PER_REPO rows, the OLDEST are evicted", async () => {
     const env = createTestEnv();
-    // Insert one MORE than the cap, each a distinct key so none upsert into another.
-    for (let i = 0; i < MAX_REVIEW_SUPPRESSIONS_PER_REPO + 1; i += 1) {
-      await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: `hash-${i}` });
+    // Fake timers force each insert's real createdAt (nowIso()) to be strictly increasing -- on real clocks, a
+    // fast in-memory D1 can otherwise complete several of these calls within the same millisecond, tying
+    // createdAt and leaving "which one is oldest" to the #4501 id tiebreak (a random UUID) rather than the
+    // insertion sequence this test's own assertions rely on.
+    vi.useFakeTimers();
+    try {
+      const start = new Date("2026-01-01T00:00:00.000Z");
+      // Insert one MORE than the cap, each a distinct key so none upsert into another.
+      for (let i = 0; i < MAX_REVIEW_SUPPRESSIONS_PER_REPO + 1; i += 1) {
+        vi.setSystemTime(new Date(start.getTime() + i * 1000));
+        await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: `hash-${i}` });
+      }
+    } finally {
+      vi.useRealTimers();
     }
     // REGRESSION: assert the underlying table itself shrank back to the cap, via a raw count query --
     // listReviewSuppressions clamps its OWN `limit` param to MAX_REVIEW_SUPPRESSIONS_PER_REPO (see the test
@@ -135,4 +146,62 @@ describe("review-memory suppression store (#2178)", () => {
     expect(await listReviewSuppressions(env, "owner/repo", 0)).toHaveLength(1);
     expect(await listReviewSuppressions(env, "owner/repo", 999_999)).toHaveLength(2);
   });
+
+  async function insertRawSuppression(env: Env, id: string, repoFullName: string, patternHash: string, createdAt: string) {
+    await env.DB.prepare(
+      "insert into review_suppression (id, repo_full_name, category, path_glob, pattern_hash, created_at) values (?, ?, 'ai_review_split', '', ?, ?)",
+    )
+      .bind(id, repoFullName, patternHash, createdAt)
+      .run();
+  }
+
+  it("INVARIANT (#4501): listReviewSuppressions orders same-createdAt rows deterministically by id, regardless of insertion order", async () => {
+    const env = createTestEnv();
+    // Same bug class as #4481 (listPullRequestFiles): without an id tiebreak, rows tied on createdAt have no
+    // guaranteed order. Inserted here in a SCRAMBLED (non-id-sorted) order on purpose.
+    for (const id of ["id-b", "id-d", "id-a", "id-c"]) {
+      await insertRawSuppression(env, id, "owner/repo", id, "2026-06-01T00:00:00.000Z");
+    }
+    const listed = await listReviewSuppressions(env, "owner/repo");
+    expect(listed.map((row) => row.id)).toEqual(["id-d", "id-c", "id-b", "id-a"]); // id DESC tiebreak
+  });
+
+  it("REGRESSION (#4501): eviction at the cap boundary is governed by the id tiebreak, not insertion order, when several suppressions share one createdAt", async () => {
+    const env = createTestEnv();
+    const repoFullName = "owner/repo";
+    // 496 rows with distinct, more-recent timestamps than the tied group below -- fills the table right up to
+    // where the tied group straddles the MAX_REVIEW_SUPPRESSIONS_PER_REPO cap boundary.
+    const newerStartMs = Date.parse("2026-06-01T00:00:00.000Z");
+    const NEWER_COUNT = 496;
+    await env.DB.batch(
+      Array.from({ length: NEWER_COUNT }, (_, index) =>
+        env.DB.prepare(
+          "insert into review_suppression (id, repo_full_name, category, path_glob, pattern_hash, created_at) values (?, ?, 'ai_review_split', '', ?, ?)",
+        ).bind(`newer-${index}`, repoFullName, `newer-hash-${index}`, new Date(newerStartMs + index * 1000).toISOString()),
+      ),
+    );
+    // 5 suppressions from ONE `@gittensory resolve` whole-PR Promise.all batch -- identical (same-millisecond)
+    // createdAt, inserted here in a SCRAMBLED (non-id-sorted) order to prove the eviction outcome doesn't
+    // depend on it.
+    const tiedCreatedAt = "2026-01-01T00:00:00.000Z";
+    const scrambledTiedIds = ["tied-c", "tied-e", "tied-a", "tied-d", "tied-b"];
+    await env.DB.batch(
+      scrambledTiedIds.map((id) =>
+        env.DB.prepare(
+          "insert into review_suppression (id, repo_full_name, category, path_glob, pattern_hash, created_at) values (?, ?, 'ai_review_split', '', ?, ?)",
+        ).bind(id, repoFullName, id, tiedCreatedAt),
+      ),
+    );
+    // Trigger the internal prune pass exactly how production reaches it: one more recorded suppression. Its
+    // real (current) createdAt is newest of all, so it and the 496 "newer" rows above are always kept -- the
+    // cap boundary lands squarely inside the 5-row tied group.
+    await recordReviewSuppression(env, { repoFullName, category: "ai_review_split", patternHash: "trigger-hash" });
+
+    const listed = await listReviewSuppressions(env, repoFullName, MAX_REVIEW_SUPPRESSIONS_PER_REPO);
+    const survivingTiedIds = new Set(scrambledTiedIds.filter((id) => listed.some((row) => row.id === id)));
+    // 1 trigger + 496 newer + 5 tied = 502 total; the cap keeps the newest 500 -- exactly 2 of the 5 tied rows
+    // are evicted, deterministically the two with the LOWEST id (desc(id) ranks the highest id first among ties).
+    expect(survivingTiedIds).toEqual(new Set(["tied-e", "tied-d", "tied-c"]));
+    expect(await rawCount(env, repoFullName)).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO);
+  });
 });