fix(subscription): fence a billing cycle's charge against an operator paying or waiving it

Merged
EV
13d ago
+4912-689

What

A billing pass now fences the cycle it is charging, so an operator can no longer pay or waive the same subscription_billing row while the order path is taking the money.

CodeRabbit raised this in round 4 of #11565, and the race already exists on develop. SubscriptionService.executeCycle prices a period, then raiseCycleOrder calls the external orderGateway.raiseSubscriptionOrder. Until now, an operator's pay / waive during that call was only noticed afterwards: markPaid hit SUBSCRIPTION_BILLING_TRANSITION_INVALID, the operator's status was kept and a warning was logged, after the money was taken. The period idempotency claim in billCycle didn't help, because the settlement entrypoints never take it.

How

The claim. Two nullable columns on subscription_billing: chargeClaimToken (varchar 36) and chargeClaimedAt.

  • SubscriptionBillingService.claimCharge writes a random token and the instant in one conditional UPDATE, only while the row is PENDING or FAILED and no other pass holds a live claim. No row lock outlives that statement, so none is held across the external call.
  • executeCycle takes the claim after the cycle row exists and before the discount, the credit and the gateway call. When the claim is refused, the pass charges nothing and answers replayed: true: with the raised cycle if it was settled meanwhile, or with "another billing pass is charging this cycle".

Finalize or release.

  • markPaid (the run's own payment, which names its claim), markInvoiced and markFailed end the claim in the same statement that records the outcome.
  • All three record the run's outcome only while the claim is still the pass's (… AND chargeClaimToken = :token). If the claim went stale and another pass recovered it, the late pass is refused (SUBSCRIPTION_BILLING_CHARGE_CLAIM_LOST) and records nothing: no payment, no invoice, no attempt count, no dunning step, no subscription.payment-failed. The recovering pass records the order, because it charges under the same attempt key. A late invoice also never overwrites a cycle an operator settled after the claim went stale.
  • A finally releases the claim (token-conditioned) on every other exit, including an exception. If the release itself fails, the claim is left to go stale.
  • A pass that meets a cycle another pass already paid (it carries an order, and only the run writes orderId) is refused the same way, so it doesn't record the success twice. An operator-paid cycle without an order is still a no-op.
  • Once the outcome is recorded, a failed bookkeeping write (setup fee, deferred credit) is not a failed charge. It propagates, so the period's key is not completed and nothing charges the cycle again. On develop, that write's failure overwrote the outcome with FAILED, and the dunning retry charged a second time.

With #11565 and #11602 (merged in from develop at 0582eb94a00). A retry now reprices its row (repriceUnsettled), and a scheduled change restates it (restateUnsettled, inside applyScheduledChange's transaction).

  • executeCycle claims an existing cycle before either write, and a new cycle right after createPending, so both writes run under the pass's claim.
  • Both writes are predicated on the claim the pass names (SUBSCRIPTION_BILLING_CHARGE_CLAIM_LOST otherwise), or on no live claim when none is named.
  • A payment recorded between the cycle's read and its claim is answered as a settled replay, before anything is repriced or charged.

The 409. While a claim is live, these are refused with 409 SUBSCRIPTION_BILLING_CHARGE_IN_FLIGHT. The details carry chargeClaimedAt and claimLiveUntil.

  • An operator's markPaid (POST /subscription-billings/:id/pay, paySubscriptionBilling).
  • markWaived (POST …/waive, waiveSubscriptionBilling). The waiver writes the row's metadata back from its own read, so it is also predicated on the cycle's amount and periodEnd. A restatement that commits in between refuses it with 409 SUBSCRIPTION_BILLING_CYCLE_RESTATED, instead of keeping the new amount next to the old gross and discount. This was found by Copilot on #11602.
  • The same check covers a correction (PUT/updateSubscriptionBilling), reschedulePending, retiring a cycle (DELETE /subscription-billings/:id, DELETE …/:id/soft, softDeleteSubscriptionBilling, all through softRemove) and the hard delete. Each would change a cycle whose money is being taken.

For pay, waive, correction and reschedule, the condition rides in each statement's own WHERE (chargeClaimedAt IS NULL OR chargeClaimedAt < now − window, via Or(IsNull(), LessThan(…))). So a pass that claims the cycle between an operator's read and the operator's write still wins.

A retirement or a hard delete is the base class's write: the soft removal's subscriber stamps deletedByUserId, and the delete also runs on MikroORM. So each PENDING/FAILED cycle is fenced for the write instead (fencedAgainstCharges): it is claimed exactly as a pass claims it, and the claim is released after the write lands. Settled history is not fenced and still retires. The candidates are read through the base class's own scope (findOneByIdString / find), the scope its write runs in. Each fence is written under the cycle's own tenant and organization, so a tenant admin's cross-organization retirement is fenced too.

Stale claims. The window is based on the order path's own idempotency.

  • The order path fences one attempt's key for IdempotencyService.DEFAULT_STALE_LOCK_MS (2 min) before letting another caller take it over.
  • The cycle's claim lasts two of those windows (4 min), because it is taken slightly before the order path's claim.
  • A pass that recovers a stale claim re-reads the row, so it calls the order path with the same attempt key as the earlier call. The order path's idempotency then decides:
    • it returns the earlier call's order (no second charge);
    • it runs the attempt the earlier call abandoned;
    • or it refuses with ORDER_SUBSCRIPTION_IN_FLIGHT while that call is still working. In that case the pass records no failure, because a failure would move the attempt count and so the next order key. The pass keeps its fresh claim, so a live order call doesn't lose its fence.
  • The port's doc now states the ORDER_SUBSCRIPTION_IN_FLIGHT contract.

Migration. 1791000003520-AddSubscriptionBillingChargeClaim covers postgres, mysql and sqlite. It is hand-written per dialect, each column is probed before it is added (a re-run issues no DDL), and the down migration drops the columns in reverse order. It is registered in subscription.plugin.ts. 3500 and 3510 are taken by open PRs #11567 and #11543.

Tests

  • subscription-billing.service.spec.ts: 28 new cases.
    • A waiver over a cycle restated between its read and its write is refused.
    • Restate and reprice happen only under the claim of the pass that names it, or with no live claim.
    • Claim, refuse a second claim, and refuse a settled cycle.
    • An operator's pay or waive refused during a charge, including a claim that lands between the operator's read and write.
    • Correction, delete, retirement and reschedule refused, including a claim that lands after the retirement's or the delete's read.
    • The claim ended by the run's paid, invoiced and failed writes.
    • A payment, an invoice or a failed attempt not recorded after the claim was recovered, nor a late payment of a cycle another pass paid.
    • A late invoice that does not overwrite an operator's payment.
    • The token-scoped release.
    • Stale recovery, and an operator settling a stale-claimed cycle.
  • subscription.service.spec.ts: 17 new cases.
    • An operator's pay or waive during the gateway call is refused and the charge is recorded.
    • The claim is released on paid, invoiced, a declined charge, and an exception while recording.
    • A pass doesn't charge a cycle another pass holds.
    • A stale claim is recovered under attempt key :1.
    • ORDER_SUBSCRIPTION_IN_FLIGHT keeps the fence and records no failure.
    • An attempt, payment or invoice that outlived its claim records nothing.
    • A late invoice doesn't overwrite an operator's payment, and a late payment of a cycle the recovering pass paid records no second success.
    • A failed setup-fee write after the payment isn't a failed charge: no attempt counted, no retry, the period key left open, and a later pass charges nothing.
  • One develop spec changed meaning with the merge: "charges nothing when the row a retry reprices was settled meanwhile". The settlement is now met by the claim, so the expected answer is a settled replay instead of a SUBSCRIPTION_BILLING_CYCLE_SETTLED throw. It still asserts that no order is raised.
  • Local run on Node 26, after merging develop:
    • Whole plugin 465/465 tests in 13 suites. Two suites (write-routes, upcoming-billing) do not load locally because of an OpenTelemetry type mismatch in core tracer.ts from the shared node_modules, which this PR does not touch.
    • Billing and service specs together: 299/299.
    • tsc -p packages/plugins/subscription/tsconfig.lib.json shows 0 errors in the plugin.
    • Gates pass: constraint parity, sqlite rebuild, nullable read, dual-ORM metadata, entity column dialect, migration duplicate column, plugin contract, jest virtual mock and API parity.

Not in this PR

  • Finishing a recorded cycle whose success never ran is a follow-up task. This is a pass that dies, or whose bookkeeping or markPaid read fails, between recording the outcome and recordSuccess. Develop already has it: every later pass replays the paid cycle and the calendar doesn't move. Review round 2 showed it needs four things: once-only finalization, the row's persisted period, consumed-credit identity, and invoice bookkeeping recovery. That is a design change of its own, so a first attempt (finishPaidCycle) was withdrawn from this PR.

  • The GraphQL SubscriptionBilling type is unchanged, so the new columns are not exposed there. REST answers carry them as read-only fields.

  • #11565, the NEXT_PERIOD fix, touches executeCycle and subscription-billing.service.ts too. Whichever merges second needs a small rebase. Its repriceUnsettled write should then also be predicated on the claim.

🤖 Generated with Claude Code