Stop the dashboard reporting a timed-out tracker query as 0 pageviews - #227
Open
ralyodio wants to merge 1 commit into
Open
Stop the dashboard reporting a timed-out tracker query as 0 pageviews#227ralyodio wants to merge 1 commit into
ralyodio wants to merge 1 commit into
Conversation
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`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CT7T7ZR3v1VuV6VRH93cdT
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.
Every project on
/dashboardread "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.What was happening
dashboard_project_pageviewswas cancelled by the 8sstatement_timeoutand returned HTTP 500 on roughly half of loads./dashboard/analyticsfires eleven of these RPCs concurrently and was failing on nearly all of them (tracker_top_actions_multi,tracker_event_mix_multiandtracker_top_referrers_multiwere at 7/7 in 24h).Every loader read only
data—const { data } = await supabase.rpc(...)— so a cancelled query and a genuinely quiet week were byte-identical, and the error object was discarded. 55 "canceling statement due to statement timeout" in Postgres' logs was the only surviving evidence.Not #226 again
Worth stating plainly, because the obvious move is to flip these to
security defineras #226 did. That is both wrong and unsafe here:postgres, with no policy in the plan, still took 8.4s.tracker_event_daily_stats_project_event_idxis(project_id, event)with noday, so forproject in (...) and event = 'pageview' and day >= Xthe planner matched 373,506 index entries, heap-fetched every one to readdayandcount, and threw 228,872 away on the filter.<x>_id in (select id from owned)). Everytracker_*_multitakesp_projectsstraight from the caller and leans entirely on RLS. Made definer as they stand, any authenticated user could pass another account's project ids and read their analytics.They stay
security invoker. A stranger's JWT was verified to return 0 rows from all of them after the change.The fix
Two covering indexes so the aggregates run index-only, the trap index dropped, and
work_memraised (per function, not globally — eleven run concurrently) on the three panels that spilled their HashAggregate to disk.Measured on prod as the 48-project owner, with RLS on:
dashboard_project_pageviewstracker_top_pages_multitracker_top_actions_multitracker_top_referrers_multipage_pathrides in the INCLUDE of the(project_id, event, day)index specifically sotracker_top_pages_multiis index-only; without it the planner still picks that index for the event predicate but heap-fetches all 373,710 matching rows — measured at 111,534 buffers, worse than before the index existed.Trade-off worth knowing:
countis now an indexed value, so the per-event upsert can no longer take the HOT path. Ingest is ~1-3 events/sec against 1.2M rows, so this is the right side of the trade, but it is the thing to watch if ingest grows an order of magnitude. Longer term this is still the rollup problem #226 flagged.So it cannot happen silently a fifth time
The zero-fill stays — one dead panel must not take the page down — but
Loaded<T>now carries afailedflag, the error is logged, and both surfaces render the "couldn't load" banner instead of a confident 0. That helper and the banner move out oflib/ads/components/ads, since both halves of the product have now had this same bug (#199, #225, #226, this).Deploy
The migration is already applied to prod (2026-09-02) — the indexes were built
CONCURRENTLYagainst the live 326MB table under ingest, so the file isif not existsand replay is a no-op. Prod is already fast; merging ships the error-surfacing UI.🤖 Generated with Claude Code
https://claude.ai/code/session_01CT7T7ZR3v1VuV6VRH93cdT