Stop the ad dashboard reporting a cancelled query as zero delivery - #226
Merged
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. |
ralyodio
added a commit
that referenced
this pull request
Sep 2, 2026
…#227) Every project on /dashboard read "0 pageviews" while ingest was writing a row a second. Same shape as the ad dashboard bug (#226), different cause, and the fix that worked there would have been unsafe here. dashboard_project_pageviews was being cancelled by the 8s statement_timeout and returning HTTP 500 on roughly half of loads; /dashboard/analytics fires eleven of these RPCs concurrently and was failing on nearly all of them. Every loader read only `data` -- `const { data } = await supabase.rpc(...)` -- so a cancelled query and a genuinely quiet week were byte-identical and nothing was logged. Not RLS this time. The same query as `postgres`, with no policy in the plan, still took 8.4s: tracker_event_daily_stats_project_event_idx is (project_id, event) with no `day`, so the planner matched 373,506 index entries, heap-fetched every one, and discarded 228,872 on the day filter. So this is NOT a repeat of #226, and making these definer would also be unsafe -- the ad RPCs each authorised themselves, while every tracker_*_multi takes p_projects straight from the caller and leans entirely on RLS. Definer as they stand, any authenticated user could read another account's analytics by passing their project ids. Two covering indexes instead, so the aggregates run index-only, plus work_mem raised on the three panels that spilled their HashAggregate to disk. Measured on prod as the 48-project owner with RLS on: dashboard_project_pageviews 9,401ms / 152,894 buf -> 115ms / 19,647 tracker_top_pages_multi 5,764ms / 77,663 buf -> 752ms / 22,764 tracker_top_actions_multi 1,927ms / 326,441 buf -> 635ms / 41,379 tracker_top_referrers_multi 1,518ms / 328,030 buf -> ~1.7s / 41,358 A stranger's JWT still returns 0 rows from all of them; the functions stay security invoker. The zero-fill itself stays -- one dead panel must not take the page down -- but `Loaded<T>` now carries a `failed` flag and both surfaces render the "couldn't load" banner instead of a confident 0. That helper and the banner move out of lib/ads and components/ads, since both halves of the product have now had this same bug. Migration applied to prod 2026-09-02; indexes were built CONCURRENTLY against the live table, so the file is `if not exists`. Claude-Session: https://claude.ai/code/session_01CT7T7ZR3v1VuV6VRH93cdT Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Sep 2, 2026
/dashboard/ads renders no graphs, intermittently: the chart is replaced by "No delivery in this range." and the campaign sparklines by "no traffic yet", over a network that delivered 177,844 impressions in the window. Delivery was healthy the whole time. Three separate faults. 1. The reporting RPCs sat at the statement_timeout wall. Measured over 24h: ad_account_series avg 1,453ms / max 8,039ms, and ad_campaign_daily_series avg 1,719ms / max 8,027ms, against the 8s timeout on `authenticated` -- 14 of ~150 calls returned HTTP 500, so roughly one load in ten came back empty and a reload "fixed" it. Not the RLS nested loop from #226; that fix held and the plan is a clean hash join. Every load simply re-aggregated the whole archive: Seq Scan on ad_impressions (actual rows=177858) Filter: ((NOT duplicate) AND (ts >= now() - '30 days')) Rows Removed by Filter: 198394 Buffers: shared hit=9767 -- the entire 78MB heap, per load and the page fires three such RPCs per render. ad_impressions is 376k rows growing ~90k/day, so cost tracked the archive rather than the window asked for. No index fixes that: the 30-day window selects 47% of the table. A covering partial index measured 157ms against 194ms for the seq scan -- a 20% gain on a query needing to be 10x faster. So pre-aggregate. Three rollups, each at the grain its surface plots (owner/hour, campaign/day, slot/day), refreshed by pg_cron every ten minutes. Closed periods come from the rollup, the current hour and current day always from raw, so a stale or un-run refresh can never show a wrong live edge and the raw slice is never wider than a day. A 30-day account query now reads ~720 rows where it read 246,506. Verified by swapping the functions inside a REPEATABLE READ transaction and diffing old against new across all 8 ranges and 3 day windows: byte-identical output, 3,312ms -> 380ms for the battery. 2. ad_campaign_daily_series had outgrown PostgREST's 1000-row cap. 139 campaigns x 30 days is 2,731 rows and the call was returning `206 Partial Content, content-range 0-999/2731`. With no ORDER BY, which two thirds got dropped was down to join order: campaigns lost days off their sparkline and five lost every row, rendering "no traffic yet" beside a row reading "Impressions: 24". It returns one jsonb array now -- one row, whatever the campaign count. Live, that took the page from 9 "no traffic yet" rows to the 4 that really have no delivery. 3. AccountTrend and MiniTrend still asserted emptiness on failure. Both receive a zero-filled series when the query dies, so "No delivery in this range." was a claim about the network made from a query that never returned -- and it sat directly under the banner #226 added saying the figures could not be loaded. They now take `failed` and say so. Tracked per loader, since the chart and the sparklines come from different RPCs and either can fail alone. Both migrations are applied to prod. The rollups reconcile exactly against raw events across all 57 closed days on both the advertiser and publisher sides, including after the first scheduled mid-day refresh. Claude-Session: https://claude.ai/code/session_01Y7rVKbcZdT7tABboosr1Re Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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