From 7e1bad8c0df137f575ae0df1e1c7d8e1557d5261 Mon Sep 17 00:00:00 2001 From: Bilal Godil Date: Thu, 16 Jul 2026 18:15:46 -0700 Subject: [PATCH] Shorten comments in subscription wind-down handling --- .../payments/transactions/refund/route.tsx | 3 +- .../[customer_id]/[product_id]/route.ts | 22 +++---- .../[customer_type]/[customer_id]/route.ts | 11 ++-- .../[customer_id]/switch/route.ts | 13 ++-- .../purchases/purchase-session/route.tsx | 12 ++-- apps/backend/src/lib/payments.test.tsx | 5 +- apps/backend/src/lib/payments.tsx | 26 ++++---- apps/backend/src/lib/stripe.test.ts | 11 +--- apps/backend/src/lib/stripe.tsx | 59 +++++++------------ 9 files changed, 58 insertions(+), 104 deletions(-) diff --git a/apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx b/apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx index 1b8135bc8..cad4abe49 100644 --- a/apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx +++ b/apps/backend/src/app/api/latest/internal/payments/transactions/refund/route.tsx @@ -702,8 +702,7 @@ async function handleSubscriptionRefund(options: { cancel_at_period_end: true, }); // Same as the cancel route: Stripe's response is the authority on - // the period boundary; the local row can be stale around a renewal - // whose webhook hasn't synced yet. + // the boundary; the local row can be stale around a renewal stripePeriodEnd = getStripeSubscriptionPeriodEnd(updated, { tenancyId: tenancy.id }); } catch (e: unknown) { if (!isStripeSubscriptionAlreadyTerminalError(e)) { diff --git a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/[product_id]/route.ts b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/[product_id]/route.ts index 973bddca3..30d022db3 100644 --- a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/[product_id]/route.ts +++ b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/[product_id]/route.ts @@ -81,9 +81,8 @@ export const DELETE = createSmartRouteHandler({ const allSubs = Object.values(subMap); const nowMillis = Date.now(); - // Explains why a matched-but-uncancelable sub can't be canceled: already - // winding down (double cancel), or in-effect in a state we don't cancel - // from (e.g. past_due). No-op when no in-effect sub matches. + // Gives double-cancels and past_due-style states an honest error; no-op + // when no in-effect sub matches. const throwIfUncancelableButInEffect = (candidates: typeof allSubs): void => { const inEffect = candidates.find(s => isSubscriptionInEffect(s, nowMillis)); if (!inEffect) return; @@ -142,18 +141,13 @@ export const DELETE = createSmartRouteHandler({ if (subscription.stripeSubscriptionId) { const stripeClient = stripe ?? throwErr(500, "Stripe client missing for subscription cancellation."); // Cancel at period end, not `subscriptions.cancel()` (immediate) — - // matches the confirm dialog and the local-sub branch below. Update - // the local row eagerly (mirroring the refund route) so the product - // list reflects the wind-down before the webhook sync arrives; the - // Stripe sub stays `active` with cancel_at_period_end until Stripe - // ends it at the period boundary. + // matches the confirm dialog and the local-sub branch below. The + // eager local write (mirroring the refund route) shows the wind-down + // before the webhook sync arrives. const updated = await stripeClient.subscriptions.update(subscription.stripeSubscriptionId, { cancel_at_period_end: true }); - // Take the boundary from Stripe's response, not the bulldozer - // snapshot read at request start — around a renewal whose webhook - // hasn't synced yet, the snapshot's period end is the previous - // (possibly already-past) one. The snapshot fallback only covers - // item-less mocked responses; the webhook sync reconciles endedAt - // either way. + // Stripe's response is the authority on the boundary — the bulldozer + // snapshot can be a stale pre-renewal period end. Snapshot fallback + // only covers item-less mocked responses. const endedAt = getStripeSubscriptionPeriodEnd(updated, { tenancyId: auth.tenancy.id }) ?? new Date(subscription.currentPeriodEndMillis); updatedSub = await prisma.subscription.update({ diff --git a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/route.ts b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/route.ts index cd30cd3b0..dbc33a763 100644 --- a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/route.ts +++ b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/route.ts @@ -68,13 +68,10 @@ export const GET = createSmartRouteHandler({ ]); // Deprecated: map productId → subscription for backward-compat fields. // ownedProducts keys use '__null__' for inline products (null productId), - // so we normalize subscription productIds to match. In-effect (not - // active): a canceled-at-period-end sub still backs its owned product - // and must not fall through to the one-time-purchase branch below. - // When several in-effect subs share a productId (e.g. one quantity of a - // stackable product was canceled by subscription_id), prefer the highest - // subscriptionDisplayRank (cancelable > active > in-effect) so a - // winding-down sub can't shadow one the customer can still act on. + // so we normalize subscription productIds to match. In-effect, not + // active-only: a winding-down sub still backs its product and must not + // fall through to the one_time branch below; productId ties pick the + // highest subscriptionDisplayRank. const nowMillis = Date.now(); const inEffectSubByProductId = new Map(); for (const s of Object.values(subMap)) { diff --git a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/switch/route.ts b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/switch/route.ts index ea4552a4a..927d6d410 100644 --- a/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/switch/route.ts +++ b/apps/backend/src/app/api/latest/payments/products/[customer_type]/[customer_id]/switch/route.ts @@ -267,10 +267,9 @@ export const POST = createSmartRouteHandler({ payment_behavior: "error_if_incomplete", payment_settings: { save_default_payment_method: "on_subscription" }, default_payment_method: resolvedPaymentMethodId, - // Paying to switch plans is an explicit "keep me subscribed": clear a - // pending cancel-at-period-end, otherwise the new plan the customer - // just paid for still dies at the period boundary. Stripe persists - // the flag across item updates unless reset. + // Paying to switch is "keep me subscribed" — Stripe persists a + // pending cancel across item updates unless reset, and the new plan + // would otherwise still die at the period boundary. cancel_at_period_end: false, items: [{ id: existingItem.id, @@ -315,10 +314,8 @@ export const POST = createSmartRouteHandler({ currentPeriodStart: sanitizedUpdateDates.start, currentPeriodEnd: sanitizedUpdateDates.end, cancelAtPeriodEnd: updatedSubscription.cancel_at_period_end, - // Clear the wind-down marks the cancel route wrote eagerly. The - // webhook sync now reconciles these from Stripe's state anyway; - // the manual clear only covers the pre-webhook window so the - // products list doesn't show "Ends on" right after a switch. + // Clear the eagerly-written wind-down marks; the webhook sync + // reconciles these anyway, this just covers the pre-webhook window canceledAt: null, endedAt: null, }, diff --git a/apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx b/apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx index d513d8139..a041b3baf 100644 --- a/apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx +++ b/apps/backend/src/app/api/latest/payments/purchases/purchase-session/route.tsx @@ -131,14 +131,10 @@ export const POST = createSmartRouteHandler({ payment_behavior: 'default_incomplete', payment_settings: { save_default_payment_method: 'on_subscription' }, expand: ['latest_invoice.confirmation_secret'], - // Note: we deliberately do NOT clear cancel_at_period_end here. - // This update runs at session creation, before the customer has - // paid (default_incomplete leaves the invoice open, and unlike - // the switch route's error_if_incomplete there is no rollback on - // payment failure) — clearing it now would reactivate an - // explicitly-canceled sub on a declined card. The webhook clears - // the flag on invoice.paid, once the re-purchase has actually - // been paid for. + // Deliberately NOT clearing cancel_at_period_end here: this runs + // before payment (default_incomplete has no rollback), so a + // declined card would reactivate an explicitly-canceled sub. The + // webhook clears the flag on invoice.paid instead. items: [{ id: existingItem.id, price_data: { diff --git a/apps/backend/src/lib/payments.test.tsx b/apps/backend/src/lib/payments.test.tsx index dd63abcc6..0cd57751d 100644 --- a/apps/backend/src/lib/payments.test.tsx +++ b/apps/backend/src/lib/payments.test.tsx @@ -296,9 +296,8 @@ describe('isSubscriptionCancelable', () => { describe('subscriptionDisplayRank', () => { it('ranks cancelable above pending-cancel Stripe subs above wound-down subs', () => { const cancelable = { status: 'active', cancelAtPeriodEnd: false }; - // A pending-cancel Stripe sub stays `active` — this is the tier that an - // active-only preference gets wrong (it would tie with the cancelable - // sibling and shadow it depending on iteration order). + // Pending-cancel Stripe subs stay `active` — the tier an active-only + // preference gets wrong const stripePendingCancel = { status: 'active', cancelAtPeriodEnd: true }; const localWindingDown = { status: 'canceled', cancelAtPeriodEnd: true }; expect(subscriptionDisplayRank(cancelable)).toBeGreaterThan(subscriptionDisplayRank(stripePendingCancel)); diff --git a/apps/backend/src/lib/payments.tsx b/apps/backend/src/lib/payments.tsx index acd35d637..6a2c41e97 100644 --- a/apps/backend/src/lib/payments.tsx +++ b/apps/backend/src/lib/payments.tsx @@ -127,22 +127,19 @@ export function isActiveSubscription(subscription: { status: string }): boolean } /** - * The subscription can be canceled (again): it is active and not already - * winding down. Keep this in sync between the product list's `is_cancelable` - * field and the cancel route's filter, so `is_cancelable: false` always - * implies the DELETE returns 400. + * Active and not already winding down. Used by both the product list's + * `is_cancelable` and the cancel route's filter — keep them in sync so + * `is_cancelable: false` always implies the DELETE returns 400. */ export function isSubscriptionCancelable(subscription: { status: string, cancelAtPeriodEnd: boolean }): boolean { return isActiveSubscription(subscription) && !subscription.cancelAtPeriodEnd; } /** - * Rank for picking the representative subscription when several in-effect - * subs share a productId (stackable products): cancelable > merely-active > - * merely-in-effect. Cancelable > active matters for Stripe subs — a - * pending-cancel Stripe sub stays `active` (with cancelAtPeriodEnd), so an - * active-only preference would let it shadow a cancelable sibling and report - * is_cancelable: false while a DELETE would in fact succeed. + * Picks the representative sub when several in-effect subs share a productId + * (stackable products): cancelable > active > in-effect. The middle tier is + * for pending-cancel Stripe subs, which stay `active` and would otherwise + * shadow a cancelable sibling. */ export function subscriptionDisplayRank(subscription: { status: string, cancelAtPeriodEnd: boolean }): number { return isSubscriptionCancelable(subscription) ? 2 : isActiveSubscription(subscription) ? 1 : 0; @@ -484,11 +481,10 @@ export async function validatePurchaseSession(options: { // a duplicate request during lag would slip past it and silently re-grant. // We query Prisma directly so the sub guard is symmetric with the OTP guard // and works for products with no productLineId. - // Includes canceled-at-period-end subs (canceled with a future endedAt) — - // they still back their product, so a purchase in the same line must treat - // them as replaceable conflicts, not as OTPs. Deliberately narrower than - // isSubscriptionInEffect: incomplete/past_due subs are excluded so a retry - // after a failed payment isn't rejected as ProductAlreadyGranted below. + // Includes canceled-at-period-end subs: they still back their product, so + // a same-line purchase must treat them as replaceable conflicts, not OTPs. + // Narrower than isSubscriptionInEffect so a payment retry of an incomplete + // sub isn't rejected as ProductAlreadyGranted below. const replaceableSubs = await prisma.subscription.findMany({ where: { tenancyId, diff --git a/apps/backend/src/lib/stripe.test.ts b/apps/backend/src/lib/stripe.test.ts index 39fff1ab1..6bf47cc18 100644 --- a/apps/backend/src/lib/stripe.test.ts +++ b/apps/backend/src/lib/stripe.test.ts @@ -2,11 +2,9 @@ import type Stripe from "stripe"; import { describe, expect, it } from "vitest"; import { getCanceledAtForSync, getEndedAtForSync } from "./stripe"; -// The helpers only read status / cancel_at_period_end / ended_at / -// canceled_at, but are typed against the full Stripe.Subscription. Building a -// real Stripe.Subscription in a unit test is impractical, so we cast a -// minimal object; a missing field the helpers start relying on would surface -// as a failing assertion here, not as a silent wrong value. +// Minimal cast: the helpers only read status / cancel_at_period_end / +// ended_at / canceled_at, and a newly-relied-on field would surface as a +// failing assertion here rather than a silent wrong value. function stripeSub(fields: { status: Stripe.Subscription.Status, cancelAtPeriodEnd?: boolean, @@ -25,9 +23,6 @@ describe("getEndedAtForSync", () => { const periodEnd = new Date("2026-08-16T00:00:00Z"); it("clears endedAt for a non-terminal sub without a pending cancel (reactivation path)", () => { - // Reversing a pending cancellation via Stripe's Dashboard/API must clear - // the endedAt the cancel route wrote eagerly — otherwise bulldozer still - // ends the grants at the old boundary while Stripe keeps billing. expect(getEndedAtForSync(stripeSub({ status: "active" }), periodEnd)).toEqual({ endedAt: null }); }); diff --git a/apps/backend/src/lib/stripe.tsx b/apps/backend/src/lib/stripe.tsx index c2a4ef9ff..10913c3f3 100644 --- a/apps/backend/src/lib/stripe.tsx +++ b/apps/backend/src/lib/stripe.tsx @@ -238,13 +238,10 @@ const TERMINAL_STRIPE_STATUSES = ["canceled", "incomplete_expired", "unpaid"] as export function getEndedAtForSync(subscription: Stripe.Subscription, sanitizedEnd: Date): { endedAt: Date | null } { if (!TERMINAL_STRIPE_STATUSES.includes(subscription.status as typeof TERMINAL_STRIPE_STATUSES[number])) { - // Non-terminal: reconcile the locally scheduled end with Stripe's flag. - // The cancel route eagerly writes endedAt = period end; Stripe is the - // authority on both whether that end is still scheduled (the pending - // cancel can be reversed via Stripe's Dashboard/API) and on the exact - // boundary (the eager write may have used a stale pre-renewal period - // end). isSubscriptionInEffect treats endedAt == null as "no end - // scheduled", so clearing it here is what reactivates entitlements. + // Non-terminal: reconcile the eagerly-written wind-down endedAt with + // Stripe's flag — a pending cancel can be reversed via Stripe's + // Dashboard/API, and clearing endedAt here is what reactivates + // entitlements. return { endedAt: subscription.cancel_at_period_end ? sanitizedEnd : null }; } // Prefer Stripe's `ended_at` — real Stripe always sets it on transitions into @@ -262,18 +259,15 @@ export function getEndedAtForSync(subscription: Stripe.Subscription, sanitizedEn } export function getCanceledAtForSync(subscription: Stripe.Subscription): { canceledAt: Date | null } { - // Mirror Stripe exactly, including null: reversing a pending cancellation - // clears canceled_at on Stripe's side, and the local mark must follow. + // Mirror Stripe including null, so reversing a pending cancel clears the local mark return { canceledAt: subscription.canceled_at ? new Date(subscription.canceled_at * 1000) : null }; } /** * Sanitized period end of the subscription's first item, or null when the - * response carries no items (mocked Stripe). Used to eagerly write the - * wind-down `endedAt` from Stripe's authoritative response instead of a - * possibly stale (pre-renewal) local or bulldozer period end. Items with - * invalid period values fall through to sanitizeStripePeriodDates' own - * now→now+1mo fallback (plus a captureError), not to the caller's fallback. + * response has no items (mocked Stripe). Lets wind-down writers take the + * boundary from Stripe's authoritative response instead of a possibly stale + * (pre-renewal) local period end. */ export function getStripeSubscriptionPeriodEnd(subscription: Stripe.Subscription, context: { tenancyId: string }): Date | null { if (subscription.items.data.length === 0) { @@ -355,17 +349,11 @@ export async function syncStripeSubscriptions(stripe: Stripe, stripeAccountId: s currentPeriodEnd: sanitizedDates.end, currentPeriodStart: sanitizedDates.start, cancelAtPeriodEnd: subscription.cancel_at_period_end, - // Same endedAt/canceledAt derivation as the update branch: the first - // sync we see for a sub can already be terminal (out-of-order - // webhooks, or a purchase-session sub whose first local write is a - // late webhook). Without this, a terminal row is created with - // endedAt = null, which isSubscriptionInEffect reads as "entitled - // forever". Note: rows created before this derivation existed can - // still hold that bad shape (terminal status, endedAt = null) and - // only self-heal when another webhook for the same customer sweeps - // them — a backfill migration (status IN terminal AND endedAt IS - // NULL) is tracked as a follow-up, since it also needs the bulldozer - // rows re-emitted. + // Same derivation as the update branch: the first sync for a sub can + // already be terminal (out-of-order webhooks), and a terminal row + // created with endedAt = null reads as "entitled forever" to + // isSubscriptionInEffect. Pre-existing rows in that shape need a + // backfill (follow-up; also requires re-emitting bulldozer rows). ...getEndedAtForSync(subscription, sanitizedDates.end), ...getCanceledAtForSync(subscription), creationSource: "PURCHASE_PAGE" @@ -384,20 +372,13 @@ export async function syncStripeSubscriptions(stripe: Stripe, stripeAccountId: s } /** - * If a paid invoice belongs to a pending-cancel subscription and was issued - * AFTER the cancellation, the customer has just paid to keep the sub (the - * purchase-session conflict-reuse path re-prices the winding-down sub and - * lets the customer pay via Stripe Elements) — so clear cancel_at_period_end - * and re-sync, which also clears the local endedAt/canceledAt wind-down - * marks via getEndedAtForSync/getCanceledAtForSync. - * - * The invoice.created > canceledAt guard is what makes this safe: a - * late-delivered invoice.paid for the ORIGINAL creation invoice predates the - * cancel and must not undo it, and pending-cancel subs generate no renewal - * invoices, so a paid invoice issued after canceledAt can only be the - * re-purchase. We can't clear the flag at session creation instead — that - * runs before payment (default_incomplete has no rollback), and a declined - * card would silently reactivate an explicitly-canceled sub. + * A paid invoice issued AFTER a sub's cancellation can only be the + * purchase-session re-price of a winding-down sub (pending-cancel subs + * generate no renewal invoices), so paying it means "keep me subscribed": + * clear cancel_at_period_end and re-sync. The created-after-cancel guard + * also keeps a late-delivered invoice.paid for the original creation invoice + * from undoing a cancel. Clearing at session creation instead would + * reactivate on declined cards (default_incomplete has no rollback). */ export async function reactivateSubscriptionIfRepurchasePaid(stripe: Stripe, stripeAccountId: string, invoice: Stripe.Invoice) { if (invoice.status !== "paid") {