From 0689c7264f3db7bb350b76c11a080a32d5dfeae4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 14:06:22 -0700 Subject: [PATCH] perf(stats): bound the review-volume trend's own-ledger query by an index, not a full scan (#4723) loadOwnLedgerDayRows previously computed each PR's true first-publish date via MIN(created_at) over the ENTIRE github_app.pr_public_surface_published history before discarding anything outside the trailing 8-week window -- an unbounded scan that grows with the whole audit_events table. Splits it into two index-backed steps instead: which PRs had any publish event in the trailing window (existing event_type+created_at index), then each candidate's true first-publish across its FULL history (new target_key+created_at index, migrations/0142). Confirmed via EXPLAIN QUERY PLAN that both steps now SEARCH an index rather than SCAN the table. A naive single-pass prefilter (raw created_at >= sinceIso before the MIN()) would have been a correctness regression, not just a perf fix: a PR whose true first-publish is outside the window but got re-published inside it (a fresh push triggering re-review) would resolve to the wrong, too-recent date. The new regression test proves such a PR is still correctly excluded -- verified failing against the naive version first. --- ...42_audit_events_target_key_created_idx.sql | 7 +++ src/db/schema.ts | 1 + src/services/public-review-volume-trend.ts | 56 ++++++++++++++----- test/unit/public-review-volume-trend.test.ts | 29 ++++++++++ 4 files changed, 78 insertions(+), 15 deletions(-) create mode 100644 migrations/0142_audit_events_target_key_created_idx.sql diff --git a/migrations/0142_audit_events_target_key_created_idx.sql b/migrations/0142_audit_events_target_key_created_idx.sql new file mode 100644 index 0000000000..dd07674be1 --- /dev/null +++ b/migrations/0142_audit_events_target_key_created_idx.sql @@ -0,0 +1,7 @@ +-- Supports an efficient "find every audit_events row for this target (PR)" lookup (#4723): without this, +-- src/services/public-review-volume-trend.ts's weekly review-volume cohort query had to scan the entire +-- github_app.pr_public_surface_published history to find each PR's true first-publish date, growing +-- unboundedly with the whole table instead of staying proportional to the trailing trend window. target_key +-- is not unique per row (one PR accumulates one row per lifecycle event: publish, close, merge, ...), so this +-- is a lookup index, not a uniqueness constraint. +CREATE INDEX IF NOT EXISTS audit_events_target_key_created_idx ON audit_events (target_key, created_at); diff --git a/src/db/schema.ts b/src/db/schema.ts index b0045fd88f..cd1c704914 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1272,6 +1272,7 @@ export const auditEvents = sqliteTable( typeCreated: index("audit_events_type_created_idx").on(table.eventType, table.createdAt), actorCreated: index("audit_events_actor_created_idx").on(table.actor, table.createdAt), routeCreated: index("audit_events_route_created_idx").on(table.route, table.createdAt), + targetKeyCreated: index("audit_events_target_key_created_idx").on(table.targetKey, table.createdAt), }), ); diff --git a/src/services/public-review-volume-trend.ts b/src/services/public-review-volume-trend.ts index 50b86c4cb5..f20302f430 100644 --- a/src/services/public-review-volume-trend.ts +++ b/src/services/public-review-volume-trend.ts @@ -15,7 +15,7 @@ // are already durable, so a live weekly re-bucketing of the SAME rows can recompute any historical week // correctly on every request -- no cron-miss gap risk, no second copy of the number to keep in sync, and the // SAME formula as the live figure by construction. -import { PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats"; +import { publicStatsProjects, safeAll } from "../review/public-stats"; import { isoWeekStart } from "./public-quality-metrics"; import { loadOrbDayRows } from "./public-accuracy-trend"; @@ -74,27 +74,53 @@ export function buildPublicReviewVolumeTrend(dayRows: DayRow[], nowMs: number, w /** Day-bucketed own-ledger reviewed/merged COHORTS: for each PR first published on a given day, `reviewed` * credits that day and `merged` credits it too IF the PR is (as of now) merged -- regardless of which day the - * merge itself happened on. Matches getPublicStats's own weeklyRows subquery (same MIN(created_at)/ - * MAX(merged_at) shape, same GROUP BY ev.repo, ev.number), just grouped by day and scoped by a HAVING clause - * instead of a single sinceIso threshold. */ + * merge itself happened on. Matches getPublicStats's own weeklyRows subquery (the same "first published, + * current disposition" concept), just grouped by day instead of filtered by a single sinceIso threshold. + * + * Two-step, index-bound query (#4723) -- NOT a naive single-pass scan of the whole publish-event history: + * 1. `recent_keys`: which PRs had *any* publish event in the trailing window. Index-accelerated via the + * existing `audit_events_type_created_idx (event_type, created_at)` -- an equality-then-range scan, cost + * proportional to recent activity, not total history. + * 2. `true_first_seen`: for JUST those candidates, the TRUE first-publish date across ALL of a PR's publish + * events (not just the recent one) -- via the new `audit_events_target_key_created_idx (target_key, + * created_at)` (migrations/0142), an index lookup per candidate rather than a full-table scan. + * Splitting it this way (instead of filtering step 1's raw rows by sinceIso BEFORE taking MIN) matters for + * correctness, not just speed: a PR whose true first-publish is OLDER than the window but which also got a + * legitimate re-publish (e.g. a fresh push triggering re-review) INSIDE the window must still resolve to its + * true (out-of-window) first-publish date and be excluded -- not get misattributed to the re-publish's week. + * Step 2 always looks at a candidate's FULL history precisely to get this right; only step 1 is time-bounded. */ async function loadOwnLedgerDayRows(env: Env, projects: string[], sinceIso: string): Promise> { const map = new Map(); if (projects.length === 0) return map; const inList = projects.map(() => "?").join(", "); const rows = await safeAll<{ day: string; reviewed: number; merged: number }>( env, - `SELECT date(first_seen) AS day, + `WITH recent_keys AS ( + SELECT DISTINCT target_key + FROM audit_events + WHERE event_type = 'github_app.pr_public_surface_published' + AND instr(target_key, '#') > 0 + AND created_at >= ? + AND LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) IN (${inList}) + ), + true_first_seen AS ( + SELECT + substr(ae.target_key, 1, instr(ae.target_key, '#') - 1) AS repo, + CAST(substr(ae.target_key, instr(ae.target_key, '#') + 1) AS INTEGER) AS number, + MIN(ae.created_at) AS first_seen + FROM audit_events ae + JOIN recent_keys rk ON rk.target_key = ae.target_key + WHERE ae.event_type = 'github_app.pr_public_surface_published' + GROUP BY ae.target_key + ) + SELECT date(t.first_seen) AS day, COUNT(*) AS reviewed, - SUM(CASE WHEN merged_at IS NOT NULL THEN 1 ELSE 0 END) AS merged - FROM ( - SELECT ev.repo, ev.number, MIN(ev.created_at) AS first_seen, MAX(pr.merged_at) AS merged_at - FROM (${PUBLISHED_PR_KEYS}) ev - LEFT JOIN pull_requests pr ON pr.repo_full_name = ev.repo AND pr.number = ev.number - WHERE LOWER(ev.repo) IN (${inList}) - GROUP BY ev.repo, ev.number - ) - GROUP BY day - HAVING date(first_seen) >= date(?)`, + SUM(CASE WHEN pr.merged_at IS NOT NULL THEN 1 ELSE 0 END) AS merged + FROM true_first_seen t + LEFT JOIN pull_requests pr ON pr.repo_full_name = t.repo AND pr.number = t.number + WHERE date(t.first_seen) >= date(?) + GROUP BY day`, + sinceIso, ...projects, sinceIso, ); diff --git a/test/unit/public-review-volume-trend.test.ts b/test/unit/public-review-volume-trend.test.ts index 0d90388ee7..8fc70135ce 100644 --- a/test/unit/public-review-volume-trend.test.ts +++ b/test/unit/public-review-volume-trend.test.ts @@ -126,6 +126,35 @@ describe("loadPublicReviewVolumeTrend — end-to-end over the real live tables", expect(currentWeek?.merged).toBe(1); }); + it("REGRESSION (#4723): a PR re-published inside the window still resolves to its TRUE (out-of-window) first-publish date, not the recent re-publish", async () => { + // The whole point of the two-step recent_keys -> true_first_seen query (#4723): step 1 finds this PR as a + // CANDIDATE purely because it has a recent publish event, but step 2 must still discover its true, + // much-older first-publish date across its FULL history -- and once found, that date is outside the + // trend's own window, so the PR must be excluded entirely, not misattributed to the recent event's week. + // A naive single-pass query that filtered raw events by `created_at >= sinceIso` BEFORE taking MIN() would + // get this wrong: it would see only the recent event and wrongly count the PR in the current week. + const env = createTestEnv({ GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory" }); + const thisMonday = isoWeekStart(NOW); + const thisWeekIso = `${thisMonday}T09:00:00.000Z`; + // 20 weeks ago: well outside the 8-week trend window, but still a real, storable timestamp. + const longAgoIso = new Date(Date.parse(thisWeekIso) - 20 * 7 * 86_400_000).toISOString(); + + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 1); + // Still open today (a long-lived PR that keeps getting re-reviewed) -- not merged, not closed. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 9, title: "PR 9", state: "open", user: { login: "a" }, head: { sha: "s9" }, labels: [] }); + // Its TRUE first publish, 20 weeks ago -- outside the trend window on its own. + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: "JSONbored/gittensory#9", outcome: "completed", createdAt: longAgoIso }); + // A legitimate re-publish THIS week (e.g. a fresh push triggered another review pass). + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: "JSONbored/gittensory#9", outcome: "completed", createdAt: thisWeekIso }); + + const trend = await loadPublicReviewVolumeTrend(env, NOW); + const currentWeek = trend[trend.length - 1]; + + // PR #9 must NOT be counted anywhere in the trend -- its true first-publish is outside the window. + expect(currentWeek?.reviewed).toBe(0); + expect(trend.every((week) => week.reviewed === 0)).toBe(true); + }); + it("still reports the Orb-fleet side when GITTENSORY_PUBLIC_STATS_REPOS is empty (no own-ledger allowlist)", async () => { const env = createTestEnv({ GITTENSORY_PUBLIC_STATS_REPOS: "" }); const thisMonday = isoWeekStart(NOW);