fix(billing): stop minting duplicate Stripe customers per user - #2067
fix(billing): stop minting duplicate Stripe customers per user#2067paustint wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved concurrency and duplicate-creation risks remain, along with the funding-instructions and customer-search gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR attempts to prevent duplicate Stripe customers and improve billing-account recovery across concurrent checkout sessions.
Changes:
- Reuses customers via metadata search and idempotent creation.
- Adds deterministic duplicate resolution and funding-instruction handling.
- Upserts billing accounts by user ID and adds regression tests.
File summaries
| File | Summary and final findings |
|---|---|
apps/api/src/app/services/stripe.service.ts |
Customer reuse and checkout integration. Moderate (2 votes): existing customers can skip team funding instructions. Moderate (2 votes): mutable request fields make idempotency keys unstable. Moderate (1 vote): search failures are treated as no customer. Moderate (1 vote): the ten-result search cap can miss subscribed customers. |
apps/api/src/app/services/__tests__/stripe.service.customer.spec.ts |
Adds customer-resolution regression coverage. |
apps/api/src/app/db/user.db.ts |
Critical (3 votes): unconditional user-based repointing can race with delayed Stripe events and corrupt subscription state. |
apps/api/src/app/db/__tests__/user.db.spec.ts |
Adds coverage for user-keyed billing-account upserts. |
Review details
Suppressed comments (2)
apps/api/src/app/services/stripe.service.ts:323
- Treating any search error as “no customer” reopens the duplicate path this change is meant to close. The idempotency key only protects an identical request for Stripe's 24-hour window; an abandoned customer with no DB billing account can be duplicated after that window (or immediately after a profile/type change) if search remains unavailable. Consider failing/retrying checkout on search failure, or use a durable user-scoped creation lock instead of creating on an unknown result.
const existingCustomer = await fetchCustomerWithSubscriptionsByJetstreamId({ userId: user.id }).catch((ex) => {
// A search outage must not block checkout; falling through to create is the pre-existing behaviour
// and the idempotency key still prevents the duplicate that prompted this.
logger.warn({ userId: user.id, ...getErrorMessageAndStackObj(ex) }, 'Unable to search for an existing Stripe customer');
return null;
});
apps/api/src/app/services/stripe.service.ts:224
- The search is capped at ten results, but the original bug can create one orphan customer per abandoned checkout with no bound on the count. If Stripe returns the subscribed/paid customer outside the first ten, the reduction below can select an uncharged orphan and attach the next checkout to it, defeating the duplicate-resolution guarantee. Paginate through all matches (or continue until all subscription-bearing candidates have been considered) instead of treating ten as complete.
const { data: customers } = await stripe.customers.search({
query: `metadata["userId"]:"${userId}"`,
limit: CUSTOMER_SEARCH_LIMIT,
expand: ['data.subscriptions'],
});
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Went through Copilot's suppressed (low-confidence) comments — one was valid:
|
c56d884 to
01e344f
Compare
01e344f to
c1f3524
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved customer-search pagination, reconciliation race conditions, search-failure handling, and subscription-priority issues can still misroute or lose billing state.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
apps/api/src/app/services/stripe.service.ts:706
subscriptions.lengthcounts historical and inactive Stripe subscriptions, but the reconciliation below passesfilterInactiveSubscriptions(subscriptions)and deletes everything else. A stale duplicate with only a canceled, paused, or past-due subscription therefore bypasses this guard, repoints the account, and can remove the paying customer's active rows. Check the filtered active list here instead.
if (existingBillingAccount && existingBillingAccount.customerId !== customer.id && subscriptions.length === 0) {
apps/api/src/app/services/stripe.service.ts:243
- Equal Unix-second
createdvalues are possible, but this comparison leaves the first API result selected when timestamps tie. Because search ordering is not a deterministic tie-breaker, repeated resolutions can switch between duplicate customers; add a stable secondary key such as the customer id for equal timestamps.
return candidate.created < best.created ? candidate : best;
apps/api/src/app/services/stripe.service.ts:316
- Returning
nullon every search error makes the no-duplicates guarantee depend on Stripe's 24-hour idempotency window. If this user's existing customer is older than 24 hours and search is temporarily unavailable,createCustomercreates another customer with a newly reusable key. Failing/retrying checkout on search failure, or using a durable DB/lock lookup, is needed to preserve uniqueness.
// A search outage must not block checkout; falling through to create is the pre-existing behaviour
// and the idempotency key still prevents the duplicate that prompted this.
logger.warn({ userId: user.id, ...getErrorMessageAndStackObj(ex) }, 'Unable to search for an existing Stripe customer');
return null;
});
apps/api/src/app/services/stripe.service.ts:359
- The implementation now uses a user-only idempotency key and invariant create parameters, so a profile edit does not produce a new key. The PR description still says the key includes request fields and changes after a profile edit; update that description and rationale to match the shipped behavior, otherwise it documents a guarantee this code intentionally no longer provides.
{ idempotencyKey: `customer-create:${user.id}` },
apps/api/src/app/services/stripe.service.ts:243
subscriptions.data.length > 0treats canceled, past-due, and ended subscriptions as evidence that this is the paying customer. If an old duplicate has only a canceled subscription while a newer duplicate has an active one, both candidates tie and the earliest-created stale customer wins, routing the next checkout to the wrong account. Rank active subscriptions first, using the same active-status policy as reconciliation, before falling back to historical subscriptions.
const candidateHasSubscriptions = (candidate.subscriptions?.data.length ?? 0) > 0;
if (bestHasSubscriptions !== candidateHasSubscriptions) {
return candidateHasSubscriptions ? candidate : best;
}
return candidate.created < best.created ? candidate : best;
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
|
Went through Copilot's suppressed (low-confidence) comments — two were real:
Not changing: |
c1f3524 to
a444a8f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical billing-account race and plan-metadata issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
apps/api/src/app/services/stripe.service.ts:724
allowRepointis true for every customer with an active subscription, but two duplicate customers can both remain active after two checkout payments. Webhook order can then move the single billing account back and forth; when their price IDs differ, reconciling the later customer deletes the other paid subscription row after the FK cascade. An active subscription is not an authoritative winner here, so claim and reconciliation need per-user serialization or a deterministic owner before allowing a repoint.
allowRepoint: activeSubscriptions.length > 0,
apps/api/src/app/services/stripe.service.ts:113
activeSubscriptionStatusesincludes Stripe'sincompletestate, so this helper treats an unpaid or action-required subscription as equivalent to a successfully active/trialing one. The resolver then breaks that tie by earliestcreated, allowing an older failed duplicate to win over the customer that actually has the paidactivesubscription;isPaidUserelsewhere explicitly accepts only ACTIVE/TRIALING (apps/api/src/app/db/user.db.ts:264-276). Rank active/trialing ahead of incomplete, while still using incomplete as a fallback when no successful subscription exists.
function hasActiveSubscriptions(customer: Stripe.Customer) {
return filterInactiveSubscriptions(customer.subscriptions?.data ?? []).length > 0;
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
…a duplicate `createCheckoutSession` called `createCustomer` unconditionally whenever our database held no billing account, and the customer id it produced was only persisted once checkout completed. Every visit to the upgrade button therefore minted another Stripe customer, and a double submission minted two seconds apart. Two defences, because neither covers the other's case: - Search by the `userId` we stamp into customer metadata before creating. This closes the long tail of repeat visits, but not a rapid double submission, because Stripe's search index lags writes by up to a minute. - Pass an idempotency key on create. Stripe honours it for 24 hours, so a repeated submission resolves to the customer the first one created. Stripe rejects a reused key whose parameters changed, which would fail checkout outright, so the key is derived from `userId` alone and everything mutable - email, name, plan type - moves out of the create and into a follow-up update. That keeps one key per user, so concurrent attempts resolve to the same customer even when they disagree about the plan type or the profile changed between them. `fetchCustomerWithSubscriptionsByJetstreamId` becomes that search. It had no callers and was unusable as written: `limit: 1` plus `data[0]` picked an arbitrary customer when duplicates existed, and its `expand` omitted the `data.` prefix a list response needs while naming `entitlements`, which is not expandable on a customer. It now resolves duplicates deterministically, preferring a customer carrying subscriptions and breaking ties on the earliest created, so repeated calls always land on the same customer. A result that does not fit one page is logged rather than ranked over silently. A reused customer also has its email and name re-stamped from the account, because checkout cannot correct them once a customer is attached: `customer_email` is omitted and `customer_update` only refreshes the address fields. Also folds the three identical copies of the bank transfer funding instructions call into one helper, which a reused customer upgrading to a team now goes through.
…econd customer `upsertBillingAccount` looked the account up by the `(userId, customerId)` pair and otherwise fell through to a create. A user has at most one billing account — `userId` is unique — so a customer id that differed from the stored one did not find the existing row and the create hit the unique violation instead. That aborted `saveSubscriptionFromCompletedSession`. Stripe takes payment at checkout completion and only then calls us, so the user was charged, the subscription was never recorded, no entitlement was granted, and the webhook retried the event indefinitely. Silent unless someone was watching webhook failures. Keying on `userId` alone points the account at the customer the payment actually belongs to. `subscription.customerId` references `billing_account.customerId` with `ON UPDATE CASCADE`, so existing rows follow, and the subscription sync that runs straight afterwards then merges or removes any that are stale. That sync had to be widened to actually do so. It deleted rows whose price had disappeared, but a cascaded row keeps its own `subscriptionId`, so when both customers were on the same price — the shape of "clicked upgrade again because entitlements never appeared" — the old row survived and the new one was created alongside it. The user then had two rows for one subscription, which never matches the price count the billing page compares against, so every visit reported out of sync and rewrote without converging. A row is now stale when either its `subscriptionId` or its `priceId` is no longer current. Only a customer that is actually being paid for may take the account from another one. An abandoned duplicate emits events too, and letting one through would repoint the account off the paying customer and cascade-delete its rows. `incomplete` is deliberately excluded: it means Stripe is still holding the initial invoice unpaid, so such a customer has bought nothing yet. That is a narrower question than which subscriptions to persist and entitle, so it gets its own status set rather than overloading `activeSubscriptionStatuses`. Two customers both being paid for is a double charge that needs a human to refund one of them, so a repoint that displaces a customer still carrying paid rows is logged at error level instead of quietly changing hands. `upsertBillingAccount` is gone. It was `claimBillingAccountForCustomer` with the guard turned off, one line above it and reached moments earlier in the same flow, so the next caller that needed to attach a customer would have found the unguarded one first. Checkout completion now claims the account explicitly, and a refusal throws so the webhook retries, which is what the unique violation used to do there.
a444a8f to
2f2cb74
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate billing-ownership and subscription-reconciliation findings remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
apps/api/src/app/services/stripe.service.ts:523
- This completion path bypasses the new claim-status guard: it always passes
allowRepoint: true, even when the expanded checkout subscription isincomplete(the service's own policy treats that status as unpaid and onlyactive/trialingmay claim an existing account). If such a session is processed, an unpaid customer can repoint the account and cascade the paid customer's rows before its later subscription event is handled. DeriveallowRepointfrom the expanded subscription's account-claiming status, or defer the repoint until the active/trialing webhook.
const claimedBillingAccount = await userDbService.claimBillingAccountForCustomer({ userId, customerId, allowRepoint: true });
apps/api/src/app/services/stripe.service.ts:405
- When search fails,
findOrCreateCustomercan reach this path and Stripe may return the existing customer for the same idempotency key. The follow-up update then writesteamId: nullonto that existing customer, erasing a team ID that checkout/webhook processing may already have stored; the reuse path deliberately avoids this at lines 358-359, but the idempotent create path does not. OmitteamIdfrom this partial update unless it is actually known so retries cannot clear real metadata.
const customer = await stripe.customers.update(createdCustomer.id, {
email: user.email,
name: user.name,
metadata: { userId: user.id, teamId: null, type },
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
…ng account Extends the account-claiming guard to teams and to checkout completion, both of which still repointed unconditionally. team_subscription.customerId cascades when the team billing account moves, so an abandoned duplicate could drag the paying customer's rows across for the reconciliation to delete - teams now claim through the same compare-in-the-write rule as users, and completion derives the decision from the session subscription's status rather than assuming a completed checkout has been paid for. Stale subscription rows are matched on exact (subscriptionId, priceId) pairs. Testing each identifier on its own stranded a row whose subscription had moved onto a price that another subscription still held.
There was a problem hiding this comment.
🟡 Changes recommended
Critical billing-scope and subscription-claim issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
apps/api/src/app/db/subscription.db.ts:186
- The user-side exact
(subscriptionId, priceId)deletion predicate is covered by the newsubscription.db.spec.ts, but this team-side mirror has no direct test: that spec does not mockteamSubscription, and the service tests mockupdateTeamSubscriptionStateForCustomerwithout inspecting its Prisma filter. Add a team reconciliation case for the same stale-pair scenarios so this changed branch cannot regress independently.
where: { teamId, customerId, NOT: currentSubscriptionItems },
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| export async function fetchCustomerWithSubscriptionsByJetstreamId({ userId }: { userId: string }): Promise<Stripe.Customer | null> { | ||
| const { data: customers, has_more: hasMore } = await stripe.customers.search({ | ||
| query: `metadata["userId"]:"${userId}"`, | ||
| limit: 1, | ||
| expand: ['subscriptions', 'entitlements'], | ||
| limit: CUSTOMER_SEARCH_LIMIT, | ||
| expand: ['data.subscriptions'], |
| // the account from an abandoned duplicate. `incomplete` is the exception - a delayed payment method leaves | ||
| // the subscription there until Stripe collects - and an unpaid customer must not displace a paying one, | ||
| // which is the same rule `saveOrUpdateSubscription` applies to the webhook path. | ||
| const allowRepoint = !isObject(subscription) || accountClaimingSubscriptionStatuses.has(subscription.status); |
Problem
POST /api/billing/checkout-sessioncreated a new Stripe customer on every call. Confirmed in the live account (acct_1NDAFAIzrbxcPwHy) — two customers 7 seconds apart for the same user:createCheckoutSessioncalledcreateCustomerunconditionally wheneveruser.billingAccount?.customerIdwas null (stripe.service.ts:719-722), and the new id was only persisted once checkout completed. So every visit to the upgrade button minted another customer — not just a double-tap; abandon-and-retry-next-week adds one too.Does it actually matter?
Mostly it's litter — an unpersisted customer is unreachable by every live code path, and
BillingAccount.userIdis unique so our DB can only point at one. But there is one path that loses money silently, and that's what makes this worth fixing rather than tidying.Subscription.customerIdis a required FK toBillingAccount.customerId, andupsertBillingAccountkeyed on the composite(userId, customerId)before falling through to a create:If a user pays on session A (account → customer A) and then pays on the still-open session B:
findUniqueon(B, userId)→ not foundcreate({ customerId: B, userId })→ P2002, unique violation onuserIdThat aborts
saveSubscriptionFromCompletedSession. Stripe takes the money at checkout completion and only then calls us, so: user charged, subscription never recorded, no entitlement granted, and the webhook retries the event forever (rethrown atstripe.service.ts:93-96). Invisible unless someone is watching webhook failures.It needs deliberate double payment, not just a double-tap — but sessions stay open ~24h and the bug's natural shape is two live checkout tabs.
Fix
1. Don't mint duplicates (
ada68f4)Two defences, because neither covers the other's case:
metadata.userIdbefore creatingThe key is
customer-create:<userId>and the create request is invariant — justmetadata: { userId }. That matters because Stripe rejects a reused key whose parameters changed, which would fail checkout outright, so anything mutable has to stay out of the create call: email, name and plan type are applied in a follow-upcustomers.update. Two attempts that disagree about the plan type (USER and TEAM tabs), or that straddle a profile edit, therefore still resolve to the same customer.fetchCustomerWithSubscriptionsByJetstreamIdbecomes that search. It had zero callers and was unusable as written —limit: 1+data[0]picked an arbitrary customer when duplicates existed, andexpand: ['subscriptions', 'entitlements']both omitted thedata.prefix a list response requires and namedentitlements, which isn't expandable on a customer (active entitlements come fromfetchCustomerEntitlements). Stripe would have rejected the call, which is good evidence it was never invoked. It now resolves duplicates deterministically: prefer a customer carrying subscriptions (that's where money moved), break ties on earliestcreatedso repeated calls always land on the same one, and log a warning when more than one is found.Also folds the three identical copies of the bank-transfer funding-instructions call into one helper. A reused customer upgrading to a team now goes through it, since a customer first created under a personal plan won't have instructions yet.
2. Make the collision recoverable (
c56d884)upsertBillingAccountnow keys onuserIdalone — a real upsert that repoints the account at the customer the payment actually belongs to.Events arriving from an abandoned duplicate must not use that to steal the account back, so
saveOrUpdateSubscriptiongoes throughclaimBillingAccountForCustomerinstead: only a customer carrying an active subscription may move the account off another one, and the comparison happens inside the write (anupdateManyfilter, plus a create whose P2002 means a concurrent event claimed it first) rather than as a read followed by an unconditional write. A customer that loses the claim returns early instead of reconciling subscriptions it does not own. "Active" uses the same status policy as the reconciliation, so a stale customer holding only a canceled subscription can't slip past the check and then delete rows the reconciliation filters out.subscription.customerIdreferencesbilling_account.customerIdON UPDATE CASCADE(verified in20250120005857_billing/migration.sql:46), so existing rows follow the repoint, andupdateSubscriptionStateForCustomer— which runs immediately afterwards and deletes rows whosepriceIdis no longer active — then merges or removes the stale ones. Degrades gracefully instead of throwing.Tests
stripe.service.customer.spec.ts: reuse vs. create, a stable idempotency key and invariant create parameters across repeat calls with a changed profile and plan type, search-outage fallback, funding instructions on team reuse, the duplicate-resolution rules, and the stale-customer claim.user.db.spec.ts: asserts the upsert keys onuserId, and coversclaimBillingAccountForCustomerin all four outcomes.api:testgreen — 334 tests, 27 files.api:typecheck, oxlint, oxfmt clean.Follow-up for you (needs live access)
Worth auditing how many duplicate pairs already exist in the live account. The Stripe CLI here is configured for the
dev-sandboxprofile (acct_1QidlcIN98MQf3Vt) and can't reach live, so I couldn't count them. The new resolution logic handles them correctly on the next checkout, but the orphans will linger withmetadata.userIdstill set until cleaned up.