Fix stalled MAU and bandwidth dashboard refresh#1856
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSchedules cron-enqueue and bandwidth-tracking tasks concurrently in the TypeScript backend; adds/overwrites PL/pgSQL functions to dedupe and batch enqueue Changes
Sequence Diagram(s)sequenceDiagram
participant TS as createStatsBandwidth (TS)
participant BG as backgroundTask runner
participant Q as queueCronStatApp job
participant T as bandwidth tracking
TS->>BG: backgroundTask(async task)
BG->>Q: start queue cron stat promise
BG->>T: start trackingJob promise
BG->>BG: await Promise.all([Q, T])
BG-->>TS: resolve (undefined)
sequenceDiagram
participant Proc as process_cron_stats_jobs()
participant Sources as usage tables (app_versions/daily_mau/device/bandwidth)
participant Enq as queue_cron_stat_app_for_app()
participant Lock as advisory lock
participant Queue as pgmq.q_cron_stat_app
Proc->>Sources: scan active app_ids (30-day window)
Proc->>Enq: call per app_id with owner_org
Enq->>Lock: acquire session advisory lock (from p_app_id)
Enq->>Queue: check for existing queued payload with appId
alt no existing job
Enq->>Queue: pgmq.send(cron_stat_app payload)
end
Enq->>Lock: release advisory lock
Enq-->>Proc: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql (1)
25-37: Consider adding a comment on theLIMIT 1clause.The query uses
LIMIT 1withoutORDER BY, which is technically non-deterministic. While the business constraint thatapp_idshould exist in at most one ofappsordeleted_appsmakes this safe, a brief comment would clarify the intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql` around lines 25 - 37, The SELECT that assigns v_org_id uses LIMIT 1 without ORDER BY which is non-deterministic; add a concise comment above the SELECT/LEFT JOIN block explaining that this is intentional and safe because the business constraint guarantees p_app_id (requested_app.app_id) can exist in at most one of public.apps or public.deleted_apps, so enforcing LIMIT 1 is merely defensive and not relying on ordering — reference the variables/tables involved (v_org_id, p_app_id/requested_app, public.apps, public.deleted_apps, owner_org, LIMIT 1) in the comment so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql`:
- Around line 92-134: The process_cron_stats_jobs() function is left executable
by PUBLIC; revoke and re-grant explicit execute permissions to lock it down: add
a REVOKE ALL ON FUNCTION public.process_cron_stats_jobs() FROM PUBLIC; then
GRANT EXECUTE ON FUNCTION public.process_cron_stats_jobs() TO service_role; (or
TO postgres if you prefer the superuser-only approach) so only the cron/service
role can invoke public.process_cron_stats_jobs().
---
Nitpick comments:
In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql`:
- Around line 25-37: The SELECT that assigns v_org_id uses LIMIT 1 without ORDER
BY which is non-deterministic; add a concise comment above the SELECT/LEFT JOIN
block explaining that this is intentional and safe because the business
constraint guarantees p_app_id (requested_app.app_id) can exist in at most one
of public.apps or public.deleted_apps, so enforcing LIMIT 1 is merely defensive
and not relying on ordering — reference the variables/tables involved (v_org_id,
p_app_id/requested_app, public.apps, public.deleted_apps, owner_org, LIMIT 1) in
the comment so future maintainers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4b5f3b3-044e-4c54-9200-fce103608fc6
📒 Files selected for processing (3)
supabase/functions/_backend/utils/stats.tssupabase/migrations/20260325043000_harden_cron_stats_queue_followup.sqltests/process-cron-stats-jobs.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql (1)
99-121: Drop the innerDISTINCTs fromactive_apps.Each branch is already combined with
UNION, so the per-branchDISTINCTadds extra hash/sort work across four 30-day scans without changing the result.Suggested change
- SELECT DISTINCT av.app_id + SELECT av.app_id FROM public.app_versions av WHERE av.created_at >= pg_catalog.now() - INTERVAL '30 days' UNION - SELECT DISTINCT dm.app_id + SELECT dm.app_id FROM public.daily_mau dm WHERE dm.date >= pg_catalog.now() - INTERVAL '30 days' AND dm.mau > 0 UNION - SELECT DISTINCT du.app_id + SELECT du.app_id FROM public.device_usage du WHERE du.timestamp >= pg_catalog.now() - INTERVAL '30 days' UNION - SELECT DISTINCT bu.app_id + SELECT bu.app_id FROM public.bandwidth_usage bu WHERE bu.timestamp >= pg_catalog.now() - INTERVAL '30 days'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql` around lines 99 - 121, In the CTE active_apps, remove the inner DISTINCT keywords from each SELECT so they read plain SELECT av.app_id, SELECT dm.app_id, SELECT du.app_id, and SELECT bu.app_id respectively; keep the UNIONs as-is and ensure the referenced tables (app_versions, daily_mau, device_usage, bandwidth_usage) and their WHERE clauses remain unchanged so the CTE still deduplicates via UNION rather than per-branch DISTINCT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql`:
- Around line 102-120: Replace the unqualified NOW() calls with fully qualified
pg_catalog.now() in the four WHERE predicates: the av.created_at >= ... clause,
dm.date >= ... in the daily_mau query, du.timestamp >= ... in the device_usage
query, and bu.timestamp >= ... in the bandwidth_usage query so that each
predicate uses pg_catalog.now() - INTERVAL '30 days' (keeping the same interval
and aliases av, dm, du, bu intact).
---
Nitpick comments:
In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql`:
- Around line 99-121: In the CTE active_apps, remove the inner DISTINCT keywords
from each SELECT so they read plain SELECT av.app_id, SELECT dm.app_id, SELECT
du.app_id, and SELECT bu.app_id respectively; keep the UNIONs as-is and ensure
the referenced tables (app_versions, daily_mau, device_usage, bandwidth_usage)
and their WHERE clauses remain unchanged so the CTE still deduplicates via UNION
rather than per-branch DISTINCT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10682604-240c-4741-b79b-34d8724adc7a
📒 Files selected for processing (1)
supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql (1)
122-127: RedundantDISTINCTafterUNION.The
UNIONoperator already eliminates duplicates, so the outerSELECT DISTINCTon line 122 is redundant. It won't cause incorrect behavior, but removing it clarifies intent and marginally reduces planning overhead.♻️ Proposed simplification
) - SELECT DISTINCT + SELECT active_apps.app_id, a.owner_org FROM active_apps INNER JOIN public.apps a ON a.app_id = active_apps.app_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql` around lines 122 - 127, Remove the redundant DISTINCT in the outer SELECT that follows the UNION: locate the query selecting from active_apps and joining public.apps (the SELECT DISTINCT returning active_apps.app_id and a.owner_org) and drop the DISTINCT since UNION already deduplicates; keep the same join (INNER JOIN public.apps a ON a.app_id = active_apps.app_id) and returned columns (app_id, owner_org) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql`:
- Around line 122-127: Remove the redundant DISTINCT in the outer SELECT that
follows the UNION: locate the query selecting from active_apps and joining
public.apps (the SELECT DISTINCT returning active_apps.app_id and a.owner_org)
and drop the DISTINCT since UNION already deduplicates; keep the same join
(INNER JOIN public.apps a ON a.app_id = active_apps.app_id) and returned columns
(app_id, owner_org) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9ca9898-85d6-4ab4-9951-75b7de7a4021
📒 Files selected for processing (2)
scripts/start-cloudflare-workers.shsupabase/migrations/20260325043000_harden_cron_stats_queue_followup.sql
|
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.
Summary
cron_stat_appdirectly from live MAU and bandwidth events so active apps keep refreshing dashboard usagemainthat hardens cron queue locking and excludes deleted apps from the periodic refresh sweepdaily_mau, plus a test that/statsonly leaves one pending refresh job per appRoot cause
process_cron_stats_jobs()only re-enqueued apps that had a recentdaily_maurow or a recently created version. After MAU changed to count first-seen devices once per billing period, active apps could stop producing freshdaily_maurows even while still generating bandwidth and stats activity. Once that happened,cron_stat_appstopped running for those apps, sodaily_bandwidthand dashboard usage charts stopped updating.Migration note
This PR intentionally uses one new migration file only:
20260325043000_harden_cron_stats_queue_followup.sql. It does not rewrite the already-pushed20260324181219_fix_process_cron_stats_activity.sqlmigration that now exists onmain.Testing
bun run supabase:db:resetbun scripts/supabase-worktree.ts db lint -s public --fail-on warningbunx eslint supabase/functions/_backend/utils/stats.ts tests/process-cron-stats-jobs.test.tsbun run typecheckbun run supabase:with-env -- bunx vitest run tests/process-cron-stats-jobs.test.ts tests/cron_stat_app.test.ts tests/cron_stat_integration.test.tsSummary by CodeRabbit
Bug Fixes
Chores
Tests