From fea2459a7da51c159c96a0b091d63726959976fc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 19:23:47 -0700 Subject: [PATCH] fix(db): add id tiebreak to product-usage rollup and review-suppression queries (#4501) Two queries in repositories.ts ordered by a timestamp column alone with no secondary tiebreak, the same bug class already fixed in listPullRequestFiles (#4481): same-millisecond ties under concurrent writes make row order (and therefore which rows survive a cap) query-plan-dependent instead of deterministic. Adds an id tiebreak to the product-usage daily rollup's main and retention scans, and to review-suppression's eviction and read queries, matching the established desc(id) convention elsewhere in this file. --- src/db/repositories.ts | 14 +++-- test/unit/product-usage.test.ts | 79 +++++++++++++++++++++++++++ test/unit/review-memory-store.test.ts | 77 ++++++++++++++++++++++++-- 3 files changed, 162 insertions(+), 8 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index b22e6cc95f..ca50c258b7 100644 --- a/src/db/repositories.ts +++ b/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); diff --git a/test/unit/product-usage.test.ts b/test/unit/product-usage.test.ts index 606a84c3ac..5b318180ec 100644 --- a/test/unit/product-usage.test.ts +++ b/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 })]), + }); + }); }); diff --git a/test/unit/review-memory-store.test.ts b/test/unit/review-memory-store.test.ts index e9e47c210f..5e960fb109 100644 --- a/test/unit/review-memory-store.test.ts +++ b/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); + }); });