fix(backend): harden cron_stat_app follow-up failures#1923
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughExtracted org-metadata fetch, stats-timestamp persistence, and queueing into helpers; persistence errors are logged (not fatal) and queuing is performed only when a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (cron request)
participant Handler as Cron Handler
participant DB as Supabase DB
participant RPC as queue_cron_stat_org_for_org
participant Logger as cloudlogErr
Client->>Handler: POST /cron_stat_app
Handler->>DB: getOrgStatsRefreshTarget(org_id)
DB-->>Handler: { customerId, previousStatsUpdatedAt } / error
opt loaded
Handler->>DB: syncOrgStatsRefresh(...) (persist timestamps)
alt persist succeeds
DB-->>Handler: success
else persist fails
DB-->>Handler: error
Handler->>Logger: cloudlogErr(error)
end
alt customerId exists
Handler->>RPC: queueOrgPlanRefresh(customerId)
RPC-->>Handler: success / error
end
end
Handler-->>Client: HTTP 200 | 500 (depends on queue RPC)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cron_stat_app_followup.unit.test.ts (1)
98-119: The queue-based builder dispatch is fragile.The
orgBuildersarray relies on the exact call order in production code (firstfrom('orgs').select(), thenfrom('orgs').update()). If the production code changes order or adds anotherorgsquery, this test silently breaks.Consider using a more explicit dispatch based on the method called:
♻️ Suggested improvement for more robust mocking
- const orgBuilders = [orgSelectBuilder, orgUpdateBuilder] - const client = { from: vi.fn((table: string) => { switch (table) { // ... case 'orgs': { - const nextBuilder = orgBuilders.shift() - if (!nextBuilder) { - throw new Error('Unexpected orgs builder call') - } - return nextBuilder + // Return a combined builder that routes based on method called + return { + select: vi.fn().mockReturnValue(orgSelectBuilder), + update: vi.fn().mockReturnValue(orgUpdateBuilder), + } } // ... } }),This way, the mock is method-aware rather than call-order-aware.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cron_stat_app_followup.unit.test.ts` around lines 98 - 119, The test's client.from mock uses a fragile call-order queue (orgBuilders) for 'orgs' which breaks if production call order changes; replace the queue with a method-aware dispatch: when client.from('orgs') is invoked return an object whose select and update methods delegate to orgSelectBuilder and orgUpdateBuilder respectively (use orgSelectBuilder for select calls and orgUpdateBuilder for update calls) so the mock behavior depends on the method invoked (select/update) rather than the sequence in orgBuilders or call order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cron_stat_app_followup.unit.test.ts`:
- Around line 98-119: The test's client.from mock uses a fragile call-order
queue (orgBuilders) for 'orgs' which breaks if production call order changes;
replace the queue with a method-aware dispatch: when client.from('orgs') is
invoked return an object whose select and update methods delegate to
orgSelectBuilder and orgUpdateBuilder respectively (use orgSelectBuilder for
select calls and orgUpdateBuilder for update calls) so the mock behavior depends
on the method invoked (select/update) rather than the sequence in orgBuilders or
call order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea352952-6b0c-42b3-a92e-f6d4b2d20065
📒 Files selected for processing (2)
supabase/functions/_backend/triggers/cron_stat_app.tstests/cron_stat_app_followup.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: b2561ee8f2
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cron_stat_app_followup.unit.test.ts (1)
136-142: Regression coverage should also prove primary stats writes were executed.Current assertions only verify follow-up paths (
orgsupdate + queue RPC). To protect the “stats rows already written” guarantee, assert daily upsert write calls too.Suggested enhancement
return { client, builders: { + dailyMauBuilder, + dailyBandwidthBuilder, + dailyStorageBuilder, + dailyVersionBuilder, orgUpdateBuilder, queueBuilder, }, } @@ expect(response.status).toBe(500) + expect(builders.dailyMauBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyBandwidthBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyStorageBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyVersionBuilder.throwOnError).toHaveBeenCalledTimes(1) expect(builders.orgUpdateBuilder.throwOnError).toHaveBeenCalledTimes(1) expect(builders.queueBuilder.throwOnError).toHaveBeenCalledTimes(1) @@ expect(response.status).toBe(200) + expect(builders.dailyMauBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyBandwidthBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyStorageBuilder.throwOnError).toHaveBeenCalledTimes(1) + expect(builders.dailyVersionBuilder.throwOnError).toHaveBeenCalledTimes(1) expect(builders.orgUpdateBuilder.throwOnError).toHaveBeenCalledTimes(1) expect(builders.queueBuilder.throwOnError).toHaveBeenCalledTimes(1)Also applies to: 203-205, 231-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cron_stat_app_followup.unit.test.ts` around lines 136 - 142, The test currently only asserts follow-up behavior (orgUpdateBuilder + queueBuilder) but misses verifying that the primary daily stats upsert was executed; update the test to assert the mocked stats write was called (e.g., assert the mock on the client/stats writer — such as client.upsertDailyStats or the stats mock used in the factory — received the expected calls) alongside the existing orgUpdateBuilder and queueBuilder assertions; add equivalent assertions in the other two test sections that the comment references so each block verifies the daily upsert write calls as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cron_stat_app_followup.unit.test.ts`:
- Around line 182-208: The test asserts queuing failure is fatal but the
intended behavior is best-effort; update the assertions in the 'returns 500 when
queuing plan processing fails after stats writes' test so that a queue error
from createSupabaseStub (builders.queueBuilder.throwOnError) does not make the
request fail: change expect(response.status).toBe(500) to
expect(response.status).toBe(200) (or to a success status used elsewhere), keep
the check that builders.orgUpdateBuilder.throwOnError was called once and that
builders.queueBuilder.throwOnError was invoked, and remove or adjust the payload
error assertion (remove expect(payload.error).toBe('unknown_error') and any
checks that treat the queue error as a fatal error) so the test reflects the
best-effort contract for queue_cron_stat_org_for_org.
---
Nitpick comments:
In `@tests/cron_stat_app_followup.unit.test.ts`:
- Around line 136-142: The test currently only asserts follow-up behavior
(orgUpdateBuilder + queueBuilder) but misses verifying that the primary daily
stats upsert was executed; update the test to assert the mocked stats write was
called (e.g., assert the mock on the client/stats writer — such as
client.upsertDailyStats or the stats mock used in the factory — received the
expected calls) alongside the existing orgUpdateBuilder and queueBuilder
assertions; add equivalent assertions in the other two test sections that the
comment references so each block verifies the daily upsert write calls as well.
🪄 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: 89f034b0-7362-4b40-8670-9a5646419cdf
📒 Files selected for processing (2)
supabase/functions/_backend/triggers/cron_stat_app.tstests/cron_stat_app_followup.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/triggers/cron_stat_app.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d146c81585
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/triggers/cron_stat_app.ts`:
- Around line 209-210: The call to queueOrgPlanRefresh(c, supabase, body.orgId,
orgStatsRefreshTarget.customerId) must be made best-effort so transient
PostgREST failures don't turn the whole job into a 5xx; wrap that await in a
try/catch (or equivalent non-throwing guard) and swallow/log the error instead
of letting queueOrgPlanRefresh's .throwOnError() propagate; reference the
queueOrgPlanRefresh invocation and ensure any thrown error is caught and logged
(with context: orgId and customerId) but does not rethrow.
🪄 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: a49694e6-75a6-4970-ba48-72ee7a98de29
📒 Files selected for processing (2)
supabase/functions/_backend/triggers/cron_stat_app.tstests/cron_stat_app_followup.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cron_stat_app_followup.unit.test.ts
|
This was referenced Apr 21, 2026
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 (AI generated)
cron_stat_apporg timestamp refresh best-effort after stats rows are already writtenqueue_cron_stat_org_for_orgcall best-effort so transient PostgREST failures do not turn a successful stats refresh into a 5xxMotivation (AI generated)
PostHog still shows a backend error bucket on
POST /triggers/cron_stat_appwithPostgrestError502 responses. The trigger completes the core stats upserts first, then performs secondary org refresh and plan queue work. A transient failure in that trailing follow-up step should not fail the whole request after the primary stats write already succeeded.Business Impact (AI generated)
This reduces noisy backend failures in PostHog and keeps usage/stat refreshes reliable for billing and reporting flows even when the follow-up queue path is temporarily flaky. It lowers false-negative cron failures without changing the successful data path.
Test Plan (AI generated)
bunx vitest run tests/cron_stat_app_followup.unit.test.tsbun run lint:backendbun run typecheckGenerated with AI
Summary by CodeRabbit