Fix rc-entitlement-drift-check aborting on transient RC 5xx#85
Conversation
A single 503 from RevenueCat's GetCustomer API during the per-profile loop threw all the way to the top-level handler, aborting drift detection for every other profile in that run. Retry once on transient failures (5xx/timeout/network error) and isolate a still-failing customer instead of throwing, reporting incomplete coverage via a separately-fingerprinted Sentry event so it never collapses with the existing drift alert. Also adds the first CI for this repo: separate GitHub Actions workflows for the new Deno edge-function tests and for the existing npm lint/typecheck scripts (previously only enforced locally via husky). Fixes MAPVAULT-J Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesThe pull request centralizes RevenueCat entitlement checks, adds retry-aware drift classification and partial-failure reporting, updates payment documentation, and introduces GitHub Actions workflows for Deno tests, linting, and typechecking. Entitlement drift reliability
Continuous integration validation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (1)
supabase/functions/rc-entitlement-drift-check/rc-client.test.ts (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the network-error and timeout retry branches.
The retry contract includes
TypeErrornetwork failures and timeout/abortDOMExceptions, but tests only exercise HTTP responses. Extend the fetch stub to return rejected promises and verify both branches retry exactly once.Also applies to: 43-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/rc-entitlement-drift-check/rc-client.test.ts` around lines 29 - 36, Extend withStubbedFetch and the related tests to support rejected fetch promises, then add cases covering TypeError network failures and timeout/abort DOMExceptions. Assert each failure is retried exactly once and that the subsequent successful response completes the expected flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deno-tests.yml:
- Line 16: Add persist-credentials: false to the actions/checkout@v4 step in
both .github/workflows/deno-tests.yml (lines 16-16) and
.github/workflows/lint-typecheck.yml (lines 13-13), preserving the existing
checkout behavior while preventing the GitHub token from being stored in local
git configuration.
- Line 32: Update the Deno test command in the workflow to remove the broad
--allow-net permission, leaving the supabase/functions test suite without
outbound network access. Do not add replacement permissions; future
network-dependent tests should scope access to their specific host.
In `@docs/payments.md`:
- Line 101: Update the stale “What it does” procedure step around the RevenueCat
customer listing to describe loading Supabase profiles and performing individual
RevenueCat customer lookups, matching the later documented workflow instead of
pagination through all RevenueCat customers.
In `@supabase/functions/rc-entitlement-drift-check/rc-client.ts`:
- Around line 6-7: Bound the sequential profile-check retry path by an overall
deadline shorter than the 10-minute lock lifetime, rather than allowing each
profile to consume its full retry budget indefinitely. Before each attempt and
between profiles, check the remaining time; stop processing when the budget is
exhausted and classify all untouched profiles as failed. Apply this to the retry
logic associated with RC_CUSTOMER_CHECK_MAX_ATTEMPTS and
RC_CUSTOMER_CHECK_RETRY_DELAY_MS while preserving existing per-profile retry
behavior within the remaining budget.
---
Nitpick comments:
In `@supabase/functions/rc-entitlement-drift-check/rc-client.test.ts`:
- Around line 29-36: Extend withStubbedFetch and the related tests to support
rejected fetch promises, then add cases covering TypeError network failures and
timeout/abort DOMExceptions. Assert each failure is retried exactly once and
that the subsequent successful response completes the expected flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2c2d086-4663-49f0-91f1-a80a292241b4
📒 Files selected for processing (6)
.github/workflows/deno-tests.yml.github/workflows/lint-typecheck.ymldocs/payments.mdsupabase/functions/rc-entitlement-drift-check/index.tssupabase/functions/rc-entitlement-drift-check/rc-client.test.tssupabase/functions/rc-entitlement-drift-check/rc-client.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode with noanytypes
Use ES module imports (import/export), not require
Destructure imports:import { useState } from 'react'
Use@/path alias for all imports; never use relative imports across directories
File naming: kebab-case for files, PascalCase for component exports
Usetrack(),identifyUser(),resetUser(),updateUserProperties()fromlib/analytics.ts— never call PostHog directly
CheckFEATURE_FLAGS.featureNameto gate behavior; add or toggle flags inlib/feature-flags.ts
Usesupabase-jsSDK for all queries (auto-handles auth tokens)
Never hardcode API keys. Use environment variables via.env
**/*.{ts,tsx}: Use TypeScript strict mode with noanytypes
Use functional components only, never class components
Use ES module imports (import/export), not require
Destructure imports:import { useState } from 'react'
Use@/path alias for all imports instead of relative imports across directories
File naming: use kebab-case for files, PascalCase for component files
UseclassNameprop for all styling with NativeWind; never useStyleSheet.create
Call Edge Functions viasupabase.functions.invoke('fn-name', { body: {...} })
Userouter.push()orrouter.replace()fromexpo-routerfor navigation
Use<Link href="">fromexpo-routerfor navigation links
Usetrack(),identifyUser(),resetUser(),updateUserProperties()fromlib/analytics.ts; never call PostHog directly
CheckFEATURE_FLAGS.featureNameto gate behavior; add or toggle flags inlib/feature-flags.ts
Usesupabase-jsSDK for all queries (auto-handles auth tokens)
Mutations enforcing business rules (freemium limits, invites) must go through Edge Functions, not direct client inserts
Never hardcode API keys; use environment variables via.env
When creating Supabase queries, always handle the error case
Files:
supabase/functions/rc-entitlement-drift-check/rc-client.test.tssupabase/functions/rc-entitlement-drift-check/rc-client.tssupabase/functions/rc-entitlement-drift-check/index.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: Use functional components only; no class components
UseclassNameprop for all styling; never useStyleSheet.create
Import View, Text, Pressable fromreact-native(NativeWind patches them)
Conditional classes: use template literals with ternary operators
Bottom sheets (@gorhom/bottom-sheet) use their own styling API, not className
Userouter.push()/router.replace()fromexpo-routerfor navigation
All user-visible strings must uset()fromreact-i18next— never hardcode strings in JSX or Alert calls
Files:
supabase/functions/rc-entitlement-drift-check/rc-client.test.tssupabase/functions/rc-entitlement-drift-check/rc-client.tssupabase/functions/rc-entitlement-drift-check/index.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
supabase/functions/**/*.ts: Mutations that enforce business rules (freemium limits, invites) go through Edge Functions, not direct client inserts
Always deploy Edge Functions with--no-verify-jwt— functions validate auth internally viaauth.getUser()
Scheduled background work usespg_cron+pg_net; bearers for cron → Edge Function calls live insupabase_vault, not function env vars
Files:
supabase/functions/rc-entitlement-drift-check/rc-client.test.tssupabase/functions/rc-entitlement-drift-check/rc-client.tssupabase/functions/rc-entitlement-drift-check/index.ts
🧠 Learnings (2)
📚 Learning: 2026-03-18T13:36:07.355Z
Learnt from: Patchalv
Repo: Patchalv/mapvault PR: 10
File: supabase/functions/grant-beta-premium/index.ts:78-89
Timestamp: 2026-03-18T13:36:07.355Z
Learning: In Patchalv/mapvault, Supabase Database Webhooks are handled by Edge Functions that are fire-and-forget. Do not return non-200 responses for errors; always return 200 for webhook endpoints. Surface errors to Sentry for manual recovery, not as 5xx retries. This guideline applies to all Edge Functions triggered by Supabase Database Webhooks in the repository.
Applied to files:
supabase/functions/rc-entitlement-drift-check/rc-client.test.tssupabase/functions/rc-entitlement-drift-check/rc-client.tssupabase/functions/rc-entitlement-drift-check/index.ts
📚 Learning: 2026-03-12T20:10:29.256Z
Learnt from: Patchalv
Repo: Patchalv/mapvault PR: 6
File: docs/mailerlite-integration-prd.md:98-129
Timestamp: 2026-03-12T20:10:29.256Z
Learning: Do not flag markdownlint warnings (e.g., MD040 fenced-code-language) for files under the docs/ directory in this repository. These are internal PRD/documentation files that are not shipped code, and lint churn on documentation should be avoided. Apply this guideline broadly to all markdown files within docs/ (docs/**/*.md); if stricter linting is ever required for docs, consider an explicit CI exception or documentation-only lint setting.
Applied to files:
docs/payments.md
🪛 LanguageTool
docs/payments.md
[style] ~101-~101: Consider using “who” when you are referring to a person instead of an object.
Context: ...t) before being given up on; a customer that still fails isn't retried further withi...
(THAT_WHO)
🪛 zizmor (1.26.1)
.github/workflows/deno-tests.yml
[warning] 16-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/lint-typecheck.yml
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🔇 Additional comments (4)
supabase/functions/rc-entitlement-drift-check/rc-client.ts (1)
1-5: LGTM!Also applies to: 8-70, 77-122, 144-180, 188-203
supabase/functions/rc-entitlement-drift-check/rc-client.test.ts (1)
9-28: LGTM!Also applies to: 123-180
supabase/functions/rc-entitlement-drift-check/index.ts (1)
4-19: LGTM!Also applies to: 117-136, 163-199
docs/payments.md (1)
96-99: LGTM!
| const RC_CUSTOMER_CHECK_MAX_ATTEMPTS = 2; | ||
| const RC_CUSTOMER_CHECK_RETRY_DELAY_MS = 300; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the sequential retry path to the lock lifetime.
Each timed-out profile can consume roughly 20.3 seconds across two attempts. At 50 profiles, this run can exceed 17 minutes, while index.ts considers the lock stale after 10 minutes, allowing overlapping drift checks.
Add an overall deadline or circuit breaker and classify untouched profiles as failed once the remaining time budget is exhausted.
Also applies to: 71-76, 123-143, 181-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/functions/rc-entitlement-drift-check/rc-client.ts` around lines 6 -
7, Bound the sequential profile-check retry path by an overall deadline shorter
than the 10-minute lock lifetime, rather than allowing each profile to consume
its full retry budget indefinitely. Before each attempt and between profiles,
check the remaining time; stop processing when the budget is exhausted and
classify all untouched profiles as failed. Apply this to the retry logic
associated with RC_CUSTOMER_CHECK_MAX_ATTEMPTS and
RC_CUSTOMER_CHECK_RETRY_DELAY_MS while preserving existing per-profile retry
behavior within the remaining budget.
There was a problem hiding this comment.
Valid concern, verified the math (worst case ~20.3s/profile under a sustained outage, so ~50 profiles could exceed the 10-minute lock staleness window and risk an overlapping run) — but deliberately leaving it out of this PR. This same risk was flagged by an internal review before this PR was opened; the call was to keep this fix scoped to the retry/isolation change (which addresses the reported issue — a single flaky customer no longer aborts the whole run) rather than take on deadline/circuit-breaker design in the same change. Tracking as a follow-up rather than implementing here — happy to open a dedicated PR for it if useful.
There was a problem hiding this comment.
@Patchalv, agreed—keeping the retry/isolation fix focused in this PR is reasonable. The lock-lifetime bound needs its own design and validation, so I’m happy to track it as a follow-up rather than block this change.
If you’d like, I can open a GitHub issue with the deadline/circuit-breaker requirements and acceptance criteria.
🐇
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh issue create \
--repo Patchalv/mapvault \
--assignee Patchalv \
--title "Bound RevenueCat entitlement drift checks to the job lock lifetime" \
--body "$(cat <<'EOF'
## Summary
Bound the RevenueCat entitlement drift-check run so sustained transient failures cannot outlive the job's lock-staleness window and permit overlapping executions.
## Rationale
Customer checks run sequentially and each transient failure may use the full retry budget. Under a sustained RevenueCat outage, the cumulative duration can exceed the 10-minute stale-lock threshold in the drift-check handler. A second invocation could then acquire the lock and overlap with the still-running first invocation.
## Affected areas
- `supabase/functions/rc-entitlement-drift-check/rc-client.ts`
- Customer retry and profile-classification flow.
- `supabase/functions/rc-entitlement-drift-check/index.ts`
- Drift-check mutex lifetime and reporting of incomplete coverage.
## Required changes
- Introduce an overall execution deadline or circuit breaker shorter than the lock staleness lifetime.
- Check the remaining budget before each customer check, before retrying a transient failure, and between profiles.
- Preserve the current per-customer retry behavior when sufficient budget remains.
- When the deadline is reached, stop customer calls and classify every unprocessed profile as failed so the run remains explicitly partial.
- Ensure reporting continues to distinguish entitlement drift from partial-check failures.
## Acceptance criteria
- A prolonged sequence of timeouts/retryable RevenueCat failures cannot run beyond the configured deadline.
- The configured deadline is safely below the 10-minute lock-staleness threshold.
- No additional RevenueCat request is made once the budget is exhausted.
- Profiles not processed because the deadline elapsed appear in `driftCheckFailed` and contribute to `failed_count` / the partial-failure signal.
- Existing behavior remains unchanged for successful checks, 404 responses, and non-retryable 4xx responses.
- Tests cover deadline exhaustion during a retry and between profiles.
## Backlinks
- PR `#85`: https://github.com/Patchalv/mapvault/pull/85
- Review discussion: https://github.com/Patchalv/mapvault/pull/85#discussion_r3594314164
Requested by `@Patchalv`.
EOF
)"Length of output: 2532
@Patchalv, created follow-up issue #86: Bound RevenueCat entitlement drift checks to the job lock lifetime.
It is assigned to you and includes the deadline/circuit-breaker requirements, failure classification behavior, tests, and backlinks to PR #85 and this discussion.
- persist-credentials: false on both workflows' checkout steps - Drop --allow-net from the Deno test command entirely — the test suite fully stubs globalThis.fetch and never makes a real network call, so no permissions are needed - Fix docs/payments.md's stale "lists every RevenueCat customer, paginated" step, which described an earlier bulk implementation; the code has done per-profile individual RC lookups for a while - Add test coverage for the network-error (TypeError) and timeout (DOMException) retry branches, previously only HTTP-status paths were exercised Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
supabase/functions/rc-entitlement-drift-check/rc-client.ts(dependency-free, no Sentry/Supabase imports) withcheckCustomerActivePremiumWithRetry()(one retry on 5xx/timeout/network error, never on 4xx) andclassifyProfilesDrift(), which isolates a still-failing customer instead of throwing.rc_drift_check_partial_failure) for runs with incomplete coverage, so it never collapses with or suppresses the existingrc_entitlement_driftalert. Heartbeat log and response body gain afailed_countfield.docs/payments.mdwith the new signal and operator guidance.deno-tests.yml(runs the newrc-client.test.tssuite via Deno) andlint-typecheck.yml(runs the existingnpm run lint/npm run typecheck, previously only enforced locally via husky).Test plan
deno test --allow-net supabase/functions— 5/5 pass (retry-then-succeed, retry-exhausted-isolated, 404 healthy path, 4xx fail-fast, batch isolation via the realclassifyProfilesDrift)deno check rc-client.ts— cleannpm run typecheck/npm run lint— clean (verified passing with no.envpresent, matching a CI checkout)failed_countappears in real logs (not done in this session — recommend before/after deploy)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores