Skip to content

perf(stats): bound public-review-volume-trend's own-ledger query by the trailing window, not a post-aggregation HAVING #4723

Description

@JSONbored

Context

Flagged by the Gittensory review agent on #4722 (merged) as a non-blocking nit.

loadOwnLedgerDayRows in src/services/public-review-volume-trend.ts aggregates MIN(ev.created_at)/MAX(pr.merged_at) over a PR's entire audit_events history before the outer GROUP BY day HAVING date(first_seen) >= date(?) discards everything outside the trailing 8-week window. As audit_events grows this becomes an effectively-unbounded scan on every /v1/public/stats request.

Why this wasn't fixed inline

The naive fix (filter ev.created_at >= sinceIso in the inner subquery's WHERE before the MIN()) is not safe: if a PR's surface got published more than once (a re-review after new commits, say), MIN(ev.created_at) exists specifically to find the PR's TRUE first-publish time across all its publish events. Pre-filtering the raw rows by sinceIso could silently pick a LATER publish event as if it were the first one, shifting that PR into the wrong week's cohort -- a correctness bug, not just a performance one. Landing that under review-cycle time pressure was worse than shipping the current (correct, just not maximally scoped) query.

Task

Find a bound that is provably safe against that failure mode -- e.g. a generous look-back margin on the RAW audit_events scan (wide enough that no in-window PR's true first-publish event could fall outside it, verified against real re-publish-event data/timing) pushed into the inner subquery, or restructure the query so the MIN() is computed only over rows already known to belong to a PR whose true first-seen is in-window. Validate with EXPLAIN QUERY PLAN and a regression test that a PR re-published outside the naive bound still lands in its correct (true first-publish) week.

Also worth fixing while touching this file (same review, same nit list): the bare first_seen reference in the outer HAVING clause relies on SQLite's implementation-defined column resolution after GROUP BY -- correct today (every row in a day group shares the same date(first_seen)) but fragile to a future reshape. Rename to HAVING day >= date(?) for clarity while in there.

Effort

S

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions