Stop the ad dashboard reporting a cancelled query as zero delivery - #226
Merged
Conversation
/dashboard/ads read 0 for everything, intermittently, while the account was delivering 176,264 impressions over the window. The measure was right this time -- #199 and #225 both hold -- and the data was there. The RPCs were being cancelled. ad_account_series, ad_campaign_totals and the two daily-series functions are security invoker, so the RLS policy on ad_impressions ("slot is mine OR campaign is mine") joins the plan. With it the planner abandons the hash join for a nested loop: one index scan per owned campaign, 139 loops, ~176k random heap fetches, 401,791 buffers (~3GB) touched per page load. ad_impressions passed 364k rows / 154MB and traffic ran 10x baseline on 2026-09-01, which tipped it over the 8s statement_timeout on `authenticated` -- 34 cancellations in two hours, surfacing as HTTP 500 on three RPCs. Each function already did its own authorisation and never relied on RLS for it: every read is gated by `<x>_id in (select id from owned)` where owned is `owner_id = auth.uid()`. Running them as definer drops the RLS subplans and the planner picks the hash join again: 11,818 buffers / 208ms against 401,791 / 932ms, byte-identical output. Verified with a stranger's JWT that all five still return 0 rows. Note the guard is `in` and not `not in`, so an anon caller gets an empty `owned` rather than everything. The second half is why this took a log dive to find. Every loader swallowed the error into a zero-filled result, so a cancelled query and a genuinely quiet range produced identical output and the page reported four confident zeros over a live network. The zero-fill stays -- one bad panel should not take the page down -- but the loaders now return Loaded<T> carrying `failed`, log the error instead of discarding it, and the four ad surfaces render "couldn't load" in place of the zeros. The PDF report says so too: that document goes to accountants, where a silent zero is read as fact. Migration is already applied to prod. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01318XDMF7H8AtH7h4ZjweTS
ThreatCrush Security Scan39 finding(s) HIGH/CRITICAL: 2 | MEDIUM: 28 | LOW: 9
Snippets are redacted; ThreatCrush never prints matched credential material. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
/dashboard/adsread 0 for everything, intermittently, while the account was delivering 176,264 impressions over the window. The free-tier measure was fine — #199 and #225 both hold — and the data was all there. The RPCs were being cancelled.Two independent faults, stacked:
1. The queries got too slow.
ad_account_series,ad_campaign_totalsand the two daily-series RPCs aresecurity invoker, so the RLS policy onad_impressions("slot is mine OR campaign is mine") joins the plan. With it the planner abandons the hash join for a nested loop — one index scan per owned campaign, 139 loops, ~176k random heap fetches, 401,791 buffers (~3GB) touched per page load.ad_impressionspassed 364k rows / 154MB, and traffic ran 10x baseline on 2026-09-01 (14,580/hr against ~500/hr). That tipped it over the 8sstatement_timeoutonauthenticated: 34canceling statement due to statement timeouterrors in two hours, surfacing as HTTP 500 on three RPCs.2. A failed query was indistinguishable from no data. Every loader swallowed the error into a zero-filled result (
error ? [] : rows), so the page reported four confident zeros over a live network — and the error object was discarded at the point of failure, so the only evidence lived in Postgres' own logs.The fix
DB — run the five reporting RPCs as
security definer. Each already did its own authorisation and never relied on RLS for it: every read is gated by<x>_id in (select id from owned)whereownedisowner_id = auth.uid(). Dropping the RLS subplans lets the planner pick the hash join again:Byte-identical output (
3/176264/0/5570/0both ways). Verified a stranger's JWT still returns 0 rows from all five. The guard isin, notnot in, so an anon caller gets an emptyownedrather than everything.App — the zero-fill stays (one bad panel should not take the page down), but the loaders now return
Loaded<T>carryingfailed, log the error instead of discarding it, and the four ad surfaces render "couldn't load" in place of the zeros. The PDF report says so too — that document goes to accountants, where a silent zero is read as fact.Status
The migration is already applied to prod (via MCP, per this repo's by-hand convention) — the dashboard is reading correctly again as of now. This PR carries the migration file so the repo matches, plus the app-side change.
Testing
Updated
tests/ads-stats-box.test.tsandtests/ads-earnings-free-tier.test.tsfor the new return shape, and added a case asserting the thing that was missing: a failing client and an empty-result client sum to the same zeros, andfailedis the only thing that tells them apart.node_moduleshas notypescriptorvitest(known-broken install), so CI needs to gate this one.🤖 Generated with Claude Code
https://claude.ai/code/session_01318XDMF7H8AtH7h4ZjweTS