fix(billing): prevent duplicate checkout subscriptions - #2335
Merged
Conversation
SirTenzin
marked this pull request as ready for review
July 21, 2026 15:44
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/external/redis/redisUtils.ts">
<violation number="1" location="server/src/external/redis/redisUtils.ts:33">
P1: Long-running billing materialization can still outlive its lock TTL and allow a concurrent holder, because `renewLock` is added but never invoked. Wire periodic owner-checked renewal into the lock-running path and fail/stop work when renewal returns false.
(Based on your team's feedback about renewing locks while work is running.) [FEEDBACK_USED]</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
server/tests/_temp/runable-dual-checkout-race.test.ts, the paid-invoice check usesinvoice.total === 1, which is likely mismatched with Stripe’s cent-based totals ($1.00should be100) and can cause the test to assert the wrong behavior or fail for the wrong reason; that weakens confidence in checkout race-condition coverage if merged as-is — update the assertion to the correct units (or normalize currency values) before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/tests/_temp/runable-dual-checkout-race.test.ts">
<violation number="1" location="server/tests/_temp/runable-dual-checkout-race.test.ts:159">
P2: The `invoice.total === 1` filter in the paid-invoice assertion is brittle and likely incorrect. The pro plan is $25/mo with a $24 coupon, so the actual invoice total is $1.00 = 100¢. If `total` is in Stripe cents, 100 !== 1 and the filter matches zero invoices, rendering the assertion meaningless. If `total` is in dollars, `=== 1` still silently fails on any proration, rounding, or fee that shifts the total. Replace with a range or meaningful threshold (e.g. `total > 0`) or verify the actual field semantics before merging.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
3 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/internal/billing/v2/utils/billingLock/withBillingLock.ts">
<violation number="1" location="server/src/internal/billing/v2/utils/billingLock/withBillingLock.ts:51">
P1: Checkout materialization can overlap another billing operation after five minutes because this is the only lease refresh and `fn` is unbounded. Keep renewing owner-checked leases through `fn` (and abort/retry if ownership is lost) so the billing lock remains held until materialization finishes.
(Based on your team's feedback about renewing long-lived locks.) [FEEDBACK_USED]</violation>
</file>
<file name="server/src/internal/billing/v2/actions/locks/checkoutSessionLock/checkCheckoutSessionLock.ts">
<violation number="1" location="server/src/internal/billing/v2/actions/locks/checkoutSessionLock/checkCheckoutSessionLock.ts:54">
P1: A conflicting non-checkout attach now cancels the customer's open Checkout session and proceeds with billing instead of returning 423. Preserve the non-checkout guard here so a direct attach cannot silently invalidate an in-progress checkout flow.</violation>
</file>
<file name="server/src/queue/processMessage.ts">
<violation number="1" location="server/src/queue/processMessage.ts:65">
P2: A retried lock collision still emits an `auto_topup_failed` webhook before SQS retries it, so customers receive a failure notification even when the later delivery succeeds. Treat `LockAlreadyExists` as retryable in `autoTopup` before sending failure webhooks (and preserve pending state until retry handling completes).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Earlier keys' leases burned down while waiting for later ones — re-arm | ||
| // once so every lease covers the full critical section. | ||
| for (const lockKey of heldKeys) { | ||
| await refreshLockLease({ lockKey, token, ttlMs: BILLING_LOCK_TTL_MS }); |
Contributor
There was a problem hiding this comment.
P1: Checkout materialization can overlap another billing operation after five minutes because this is the only lease refresh and fn is unbounded. Keep renewing owner-checked leases through fn (and abort/retry if ownership is lost) so the billing lock remains held until materialization finishes.
(Based on your team's feedback about renewing long-lived locks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/utils/billingLock/withBillingLock.ts, line 51:
<comment>Checkout materialization can overlap another billing operation after five minutes because this is the only lease refresh and `fn` is unbounded. Keep renewing owner-checked leases through `fn` (and abort/retry if ownership is lost) so the billing lock remains held until materialization finishes.
(Based on your team's feedback about renewing long-lived locks.) </comment>
<file context>
@@ -38,20 +45,14 @@ export const withBillingLock = async <T>({
+ // Earlier keys' leases burned down while waiting for later ones — re-arm
+ // once so every lease covers the full critical section.
+ for (const lockKey of heldKeys) {
+ await refreshLockLease({ lockKey, token, ttlMs: BILLING_LOCK_TTL_MS });
+ }
</file context>
| } | ||
|
|
||
| if (ctx.env === AppEnv.Sandbox) { | ||
| const cleared = await checkoutSessionLock.expireAndClearIfOwned({ |
Contributor
There was a problem hiding this comment.
P1: A conflicting non-checkout attach now cancels the customer's open Checkout session and proceeds with billing instead of returning 423. Preserve the non-checkout guard here so a direct attach cannot silently invalidate an in-progress checkout flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/actions/locks/checkoutSessionLock/checkCheckoutSessionLock.ts, line 54:
<comment>A conflicting non-checkout attach now cancels the customer's open Checkout session and proceeds with billing instead of returning 423. Preserve the non-checkout guard here so a direct attach cannot silently invalidate an in-progress checkout flow.</comment>
<file context>
@@ -50,35 +51,25 @@ export const checkCheckoutSessionLock = async <T extends BillingContext>({
- });
- return null;
- }
+ const cleared = await checkoutSessionLock.expireAndClearIfOwned({
+ ctx,
+ customerId,
</file context>
| // Top-up shares the customer billing lock — retry instead of dropping the job | ||
| // when it collides with an attach or checkout materialization. | ||
| case JobName.AutoTopUp: | ||
| return ( |
Contributor
There was a problem hiding this comment.
P2: A retried lock collision still emits an auto_topup_failed webhook before SQS retries it, so customers receive a failure notification even when the later delivery succeeds. Treat LockAlreadyExists as retryable in autoTopup before sending failure webhooks (and preserve pending state until retry handling completes).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/queue/processMessage.ts, line 65:
<comment>A retried lock collision still emits an `auto_topup_failed` webhook before SQS retries it, so customers receive a failure notification even when the later delivery succeeds. Treat `LockAlreadyExists` as retryable in `autoTopup` before sending failure webhooks (and preserve pending state until retry handling completes).</comment>
<file context>
@@ -58,6 +59,12 @@ export const shouldRetrySqsJobError = ({
+ // Top-up shares the customer billing lock — retry instead of dropping the job
+ // when it collides with an attach or checkout materialization.
+ case JobName.AutoTopUp:
+ return (
+ error instanceof RecaseError && error.code === ErrCode.LockAlreadyExists
+ );
</file context>
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.
Problem
A Runable customer got two active subscriptions and was double-charged for the same plan. The production sequence (from Axiom + Stripe):
billing.attach(pro)→ Stripe Checkout session A created.checkout.session.completedwebhook was still materializing the plan in Autumn (~11s), a thirdbilling.attacharrived. It read stale customer state (still on the free plan), saw the newly saved payment method, chose direct billing, and created subscription added org creation / switching #2.Result: 2 active customer products, 2 Stripe subscriptions, 2 paid invoices.
Neither existing guard covers this overlap: the HTTP billing lock only serializes API requests (the webhook doesn't use it), and the checkout metadata claim only dedupes redelivery of the same session.
Fix
No new locks or queues — the two existing Redis keys are made correct:
checkout_lock:*) lives as long as the session is payable. TTL now derives from Stripe'sexpires_atinstead of a fixed 2 minutes.stripe_checkoutmode — the exact gap the third attach fell through). Conflicting non-checkout attaches still get423.billing.attach(withBillingLock, bounded retry), so webhook materialization and API attaches serialize instead of interleaving. The webhook holds the lock under both customer identifiers (public + internal), since route locks key on whichever one the API caller passed.acquireLock/clearLockgained an optional token (Lua compare-and-delete on release), wired through the route handler lock,withLock, andwithBillingLock— a holder that outlives its 120s lease can no longer delete a successor's lock. (Addresses the greptile/Codex P1s from the previous PR fix/dual checkout race #2334.)Test
server/tests/_temp/runable-dual-checkout-race.test.tsreproduces the exact production sequence (expired reservation → second session → completion racing a third attach). Red on the old code (2/2/2), green with this fix: the third attach returns the pending session's URL and exactly one product / subscription / invoice exists.Supersedes #2334 (closed to re-run reviews on the review-fix commit).
Summary by cubic
Prevents duplicate Stripe subscriptions by fixing races between
billing.attachand Stripe webhooks. Checkout reservations last for the session lifetime and webhook materialization now serializes with API attaches via a background-onlywithBillingLock.expires_at(default 1h) in primary Redis; reuse same-params Checkout URL; session-owned compare-and-delete.attach/createScheduleentry with a canonical-key fallback to avoid mid-request clearing.billing.attach,checkout.session.completed, and setup-payment webhooks with background-onlywithBillingLock(ordered multi-key, bounded wait, one-shot lease refresh); HTTP routes do not wait.AutoTopUpjobs on lock conflicts.dual-checkout-completion-racetest and update checkout lock tests.Written for commit df352a7. Summary will update on new commits.
Greptile Summary
This PR prevents duplicate checkout subscriptions by coordinating pending sessions and customer billing work. The main changes are:
Confidence Score: 5/5
No additional blocking issue qualifies for this review round.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'dev' into fix/dual-checkou..." | Re-trigger Greptile
Context used (4)