fix(backend): harden statistics rpc retries#1930
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a retry/backoff wrapper for statistics Supabase queries, unifies app→owner_org resolution with retry and not-found signaling, normalizes null storage sizes to 0, exposes statistics test utilities, and adds unit tests for retry and owner-resolution behaviors. Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant Handler as "Statistics Handler"
participant Retry as "Retry Wrapper\n(executeStatsQueryWithRetry)"
participant Supabase as "Supabase RPC/DB"
Client->>Handler: GET /statistics/app/:app_id
Handler->>Retry: request metrics RPC (app/org), storage RPC, owner lookup
Retry->>Supabase: execute RPC / query
Supabase-->>Retry: { error: 502 }
Note right of Retry: detect retryable error\napply backoff & retry
Retry->>Retry: wait (exponential backoff)
Retry->>Supabase: retry RPC / query
Supabase-->>Retry: { data: ... , error: null }
Retry-->>Handler: consolidated results (metrics, storage normalized, ownerOrg)
Handler-->>Client: HTTP 200 with statistics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/statistics-retries.unit.test.ts (1)
8-44: These unit tests can run withit.concurrent().They are independent and only assert local mocks/results, so keeping them serial just slows the file down.
♻️ Suggested change
describe('statistics retry helpers', () => { - it('retries transient statistics query failures and returns the recovered result', async () => { + it.concurrent('retries transient statistics query failures and returns the recovered result', async () => { // ... }) - it('does not retry non-retryable statistics query failures', async () => { + it.concurrent('does not retry non-retryable statistics query failures', async () => { // ... }) - it('marks missing apps as not found when the lookup returns no rows', async () => { + it.concurrent('marks missing apps as not found when the lookup returns no rows', async () => { // ... }) })As per coding guidelines
tests/**/*.{ts,js}: Use it.concurrent() instead of it() when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/statistics-retries.unit.test.ts` around lines 8 - 44, Change the three serial tests in the "statistics retry helpers" suite to run concurrently by replacing it(...) with it.concurrent(...); specifically update the tests that call statisticsTestUtils.executeStatsQueryWithRetry (the two cases using the mocked query) and the test that calls statisticsTestUtils.resolveAppOwnerOrg (the supabase maybeSingle mock) so they use it.concurrent to allow parallel execution without changing test logic or mocks.
🤖 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/functions/_backend/public/statistics/index.ts`:
- Around line 195-201: The missing-app 404 from resolveAppOwnerOrg is being
swallowed by the aggregator that collapses every stat.error into a generic
cannot_get_user_statistics; update the aggregator that processes stat.error (the
code path that currently converts all errors into cannot_get_user_statistics for
the /user statistics path) to detect and propagate the not-found/discriminated
error produced by resolveAppOwnerOrg (e.g., the error object with error:
'app_not_found' or a status: 404) instead of turning it into a 500; ensure
ownerOrgId resolution code (resolveAppOwnerOrg) can return a discriminated
result and that the aggregator checks stat.error.status === 404 or
stat.error.error === 'app_not_found' and returns that 404-shaped response up the
stack so callers of the /user endpoint receive the correct not-found result
rather than a generic cannot_get_user_statistics.
---
Nitpick comments:
In `@tests/statistics-retries.unit.test.ts`:
- Around line 8-44: Change the three serial tests in the "statistics retry
helpers" suite to run concurrently by replacing it(...) with it.concurrent(...);
specifically update the tests that call
statisticsTestUtils.executeStatsQueryWithRetry (the two cases using the mocked
query) and the test that calls statisticsTestUtils.resolveAppOwnerOrg (the
supabase maybeSingle mock) so they use it.concurrent to allow parallel execution
without changing test logic or mocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0334c59-3210-4345-8d3e-cdfc3e2d8652
📒 Files selected for processing (2)
supabase/functions/_backend/public/statistics/index.tstests/statistics-retries.unit.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77052282e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/functions/_backend/public/statistics/index.ts`:
- Around line 100-107: The getMissingAppStatsError detector currently treats any
404-shaped error as an app-missing signal; tighten it to match the synthetic app
error shape by returning true only when appError.error === 'app_not_found' OR
when appError.status === 404 AND typeof (appError as any).app_id === 'string'.
Update the getMissingAppStatsError function accordingly and make the identical
fix at the other occurrence that uses the same 404-based check (the similar
block around lines 828-830) so both places only treat the specific app-shaped
error as "app not found".
- Around line 55-58: The QueryResult<T> type only captures data and error so
HTTP status is lost; add a top-level status?: number | null to the
QueryResult<T> interface and update the retry logic (the function(s) referencing
getRetryableStatus or checking error.status) to inspect the response.status (not
error.status) for 5xx detection; ensure any function that returns or forwards
Supabase/PostgREST responses (the code paths that construct QueryResult from
Supabase responses) include the response.status in the QueryResult so retry
checks can accurately detect 5xx HTTP errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: caea3e01-5976-47b4-b056-c88a9b2e33f5
📒 Files selected for processing (2)
supabase/functions/_backend/public/statistics/index.tstests/statistics-retries.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/statistics-retries.unit.test.ts
|
1 participant
Summary (AI generated)
app_not_foundinstead of falling through to a generic app statistics failure when the app lookup returns no rowMotivation (AI generated)
The PostHog backend error stream shows live
statisticsfailures (Cannot get organization statistics) and older matching app statistics failures. Those routes were treating transient upstream query failures as hard failures and also ignored one app lookup error path entirely.Business Impact (AI generated)
This reduces noisy backend alerts and prevents users from seeing intermittent statistics failures when the underlying PostgREST layer briefly returns 5xx errors. It also makes missing-app cases fail with a clearer API response instead of a generic server error.
Test Plan (AI generated)
bunx vitest run tests/statistics-retries.unit.test.tsbunx eslint supabase/functions/_backend/public/statistics/index.ts tests/statistics-retries.unit.test.tsbun typecheckGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests