Part of #4496. P5 — medium severity, medium confidence. Same bug class as #4481 (already fixed tonight), found in two more tables.
Context
Two queries in src/db/repositories.ts order by a timestamp column alone with no secondary tiebreak, exactly the same class of bug just fixed in listPullRequestFiles (#4481) — same-millisecond ties under concurrent writes make row order (and therefore which rows survive a .slice()/.limit() cap) query-plan-dependent instead of deterministic. Three sibling functions in this SAME file already add an id tiebreak for this exact reason (repositories.ts:3046, 3095, 3132 — orderBy(desc(auditEvents.createdAt), desc(auditEvents.id)); 5315 — gateOutcomes; 5372 — agentPendingActions), so this is an established in-file convention simply not applied to these two newer functions.
1. Product-usage daily rollup (repositories.ts:6476-6492, upsertProductUsageDailyRollup): queries productUsageEvents ordered only by occurredAt, then slices at PRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT/PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT (both 5000). On a day whose volume exceeds 5000, WHICH events survive the slice is query-plan-dependent on ties — yet this sampled set feeds activeActors/byRepo/byRole/retention rates, durably upserted keyed by day (.onConflictDoUpdate({ target: productUsageDailyRollups.day, ... })). src/index.ts:184-188 re-enqueues this rollup for the trailing 7 days EVERY HOUR — so a historical day's persisted analytics can silently drift hour to hour on any day near/over the 5000-event mark, with no underlying data change.
2. Review-suppression eviction (repositories.ts:5264-5285, pruneReviewSuppressionsOverCap): selects a repo's review_suppression rows ordered only by desc(createdAt) and deletes everything past MAX_REVIEW_SUPPRESSIONS_PER_REPO (500) as the JS-sliced "overflow" — but the function's own doc comment claims "Evict the OLDEST review_suppression rows" (5261-5263), which is only true when there are no ties. A real, reachable concurrent-write path exists: the @gittensory resolve "whole PR" command builds one recordReviewSuppression write per currently-selected finding and fires them together via Promise.all (processors.ts, the resolve-whole-PR handler) — same-repo, same-millisecond writes tie on createdAt. A repo at/near its 500-signal cap that resolves several findings from one PR in one command can have the cap-eviction nondeterministically drop one of the very suppressions the maintainer just recorded (or a different pre-existing one) instead of consistently the true oldest — silently un-suppressing a finding a maintainer explicitly dismissed as a false positive.
Requirements
- Add
id (or an equivalent rowid) as a secondary sort key to both queries: repositories.ts:6476-6481 and 6487-6492 (product-usage rollup + its retention counterpart), and repositories.ts:5273 (suppression eviction) plus the read-path listReviewSuppressions at 5294 for consistency with the eviction order.
- Match the exact tiebreak pattern already established at
repositories.ts:3046, 5315, 5372.
- Invariant + regression tests (non-negotiable): for BOTH tables, a test that inserts multiple rows with an IDENTICAL
createdAt/occurredAt timestamp and asserts the query returns them in a stable, deterministic order across repeated calls (not just "some" order); a regression test for the suppression eviction specifically, reproducing the exact incident shape (a batch of same-millisecond suppression writes at/near the 500 cap via Promise.all, asserting the SAME row is evicted on repeated identical runs, not different rows each time); a regression test for the product-usage rollup reproducing a >5000-event day with ties, asserting the persisted rollup is byte-identical across repeated hourly re-runs.
Deliverables
Expected outcome
Both tables produce a genuinely deterministic, repeatable result regardless of concurrent same-millisecond writes — historical analytics stop silently drifting hour to hour, and a maintainer's explicit suppression dismissal can never be silently undone by an unrelated eviction tie.
References
Effort
S
Part of #4496. P5 — medium severity, medium confidence. Same bug class as #4481 (already fixed tonight), found in two more tables.
Context
Two queries in
src/db/repositories.tsorder by a timestamp column alone with no secondary tiebreak, exactly the same class of bug just fixed inlistPullRequestFiles(#4481) — same-millisecond ties under concurrent writes make row order (and therefore which rows survive a.slice()/.limit()cap) query-plan-dependent instead of deterministic. Three sibling functions in this SAME file already add anidtiebreak for this exact reason (repositories.ts:3046, 3095, 3132—orderBy(desc(auditEvents.createdAt), desc(auditEvents.id));5315—gateOutcomes;5372—agentPendingActions), so this is an established in-file convention simply not applied to these two newer functions.1. Product-usage daily rollup (
repositories.ts:6476-6492,upsertProductUsageDailyRollup): queriesproductUsageEventsordered only byoccurredAt, then slices atPRODUCT_USAGE_ROLLUP_EVENT_SCAN_LIMIT/PRODUCT_USAGE_RETENTION_EVENT_SCAN_LIMIT(both 5000). On a day whose volume exceeds 5000, WHICH events survive the slice is query-plan-dependent on ties — yet this sampled set feedsactiveActors/byRepo/byRole/retention rates, durably upserted keyed byday(.onConflictDoUpdate({ target: productUsageDailyRollups.day, ... })).src/index.ts:184-188re-enqueues this rollup for the trailing 7 days EVERY HOUR — so a historical day's persisted analytics can silently drift hour to hour on any day near/over the 5000-event mark, with no underlying data change.2. Review-suppression eviction (
repositories.ts:5264-5285,pruneReviewSuppressionsOverCap): selects a repo'sreview_suppressionrows ordered only bydesc(createdAt)and deletes everything pastMAX_REVIEW_SUPPRESSIONS_PER_REPO(500) as the JS-sliced "overflow" — but the function's own doc comment claims "Evict the OLDEST review_suppression rows" (5261-5263), which is only true when there are no ties. A real, reachable concurrent-write path exists: the@gittensory resolve"whole PR" command builds onerecordReviewSuppressionwrite per currently-selected finding and fires them together viaPromise.all(processors.ts, the resolve-whole-PR handler) — same-repo, same-millisecond writes tie oncreatedAt. A repo at/near its 500-signal cap that resolves several findings from one PR in one command can have the cap-eviction nondeterministically drop one of the very suppressions the maintainer just recorded (or a different pre-existing one) instead of consistently the true oldest — silently un-suppressing a finding a maintainer explicitly dismissed as a false positive.Requirements
id(or an equivalent rowid) as a secondary sort key to both queries:repositories.ts:6476-6481and6487-6492(product-usage rollup + its retention counterpart), andrepositories.ts:5273(suppression eviction) plus the read-pathlistReviewSuppressionsat5294for consistency with the eviction order.repositories.ts:3046, 5315, 5372.createdAt/occurredAttimestamp and asserts the query returns them in a stable, deterministic order across repeated calls (not just "some" order); a regression test for the suppression eviction specifically, reproducing the exact incident shape (a batch of same-millisecond suppression writes at/near the 500 cap viaPromise.all, asserting the SAME row is evicted on repeated identical runs, not different rows each time); a regression test for the product-usage rollup reproducing a >5000-event day with ties, asserting the persisted rollup is byte-identical across repeated hourly re-runs.Deliverables
idsecondary tiebreak added to both rollup queries and the suppression eviction + read queriesExpected outcome
Both tables produce a genuinely deterministic, repeatable result regardless of concurrent same-millisecond writes — historical analytics stop silently drifting hour to hour, and a maintainer's explicit suppression dismissal can never be silently undone by an unrelated eviction tie.
References
src/db/repositories.ts:6476-6503(product-usage rollup)src/db/repositories.ts:5217-5297(suppression record + prune + list)src/db/repositories.ts:3046, 3095, 3132, 5315, 5372(the established tiebreak precedent in this same file)src/index.ts:184-188(hourly 7-day rollup re-run trigger)listPullRequestFiles)Effort
S