Billing Review

Billing Review (Polar)

The handle is the diff. The evidence is where the change breaks a billing invariant the team enforces in review.

Not a general code review. The other lenses know Polar's conventions, contract and deploy shape. This one knows how the billing domain is supposed to behave.

Distilled from ~210 review comments François (frankie567) left on billing PRs between February and August 2026, plus domain invariants raised by pieterbeulque, psincraian, joebon and Yopi on the same PRs. Each rule cites its PR.

Read .agents/skills/polar-billing.md first. That is the map of the domain — entities, services, tasks, the cycle flow, proration, dunning, the ledger. This file is the checklist that assumes it.

Scope

polar/{subscription,order,billing_entry,meter,event,discount,checkout,checkout_link}/
polar/{payment,payment_method,refund,dispute,payout,payout_account,wallet,transaction}/
polar/{invoice,receipt,tax,product,customer_seat,account}/  ·  polar/benefit/grant/
polar/models/{subscription,order,order_item,product,product_price,discount,checkout,
              payment,billing_entry,refund,dispute,wallet,transaction}.py
migrations/ and server/scripts/ when they touch those tables

Owned elsewhere: schema and SDK impact → api-surface-review; migration and actor deploy safety → ship-safety; lazy="raise" and repository conventions → conventions-check; ADR-0006 / ADR-0007 → adr-check; existing helpers → reuse-check.

For a domain question you cannot settle from the diff, ask it rather than assert a defect. That is how this team reviews.

Checks

1. Money comes from billing entries, not derived state

BillingEntry is the ledger tying an invoice line back to the events that caused it. Anything computing an amount from a summary or a recomputation loses that trace.

• Invoicing metered usage reads billing entries, never the customer meter's computed balance. "Billing Entries are the source of truth… Otherwise, we lose the ability to track billing down to the events." (#12510)
• If consumption happened, the entries already exist. New code that recomputes usage instead of picking up pending entries is a smell. (#12510)
• Order creation is what consumes pending entries. The question for a new billing trigger is "should this create an order", not "should this compute an amount".

2. Reuse the lifecycle, do not hand-roll the transition

The cycle already moves periods, writes events, and grants or revokes benefits.

• Changing plan during a trial: call update_trial so periods and events are right, then let it cycle naturally because the new period is already past — same as ending a trial immediately. (#11898)
• New periodic behaviour gets a scheduler modelled on subscription/scheduler.py, not a bespoke loop. (#12990)
• One cycle, one invoice. Overages from a meter cycle that coincides with a billing cycle belong on the renewal invoice, not a second order with a second payment. (#12510)

3. Dunning decisions live in the dunning entry point

• The retry decision belongs in _handle_first_dunning_attempt, not the caller. Check the latest payment's decline code, set a retry date if recoverable, leave it None otherwise. Everything else (past due, benefit enqueue) is untouched. (#9744)
• Never schedule a retry for a non-recoverable decline. (#9744)
• Decline-code knowledge lives on the Payment model as a @property (UNRECOVERABLE_DECLINE_CODES), guarded by if self.processor == PaymentProcessor.stripe so a second processor does not inherit Stripe's codes. (#9744)
• Retry-exhaustion arithmetic is a classic off-by-one. Check the boundary. (#11905)

4. Payment locks release on one path

• The webhook handler for the payment outcome (handle_payment_failure) releases the lock. A caller that also releases it on its own error path is duplicated logic — delete it and its test. (#10653)
• Never release a payment lock on success. release_on_success was removed from the codebase for this reason. A diff reintroducing it is a regression. (#10653)
• with_for_update belongs in the task that owns the unit of work, not inside transfer_stripe. "This logic should be independent from Stripe behavior." (#12097)
• A wedged lock breaks dunning silently. (#13272)

5. Keep processor state in sync, keep processor names out of the domain

• Domain statuses mirror the processor's vocabulary. accepted was rejected as a dispute status because Stripe uses lost either way, and diverging breaks the sync. (#12713)
• A merchant action with a processor counterpart makes the processor call in the same flow. "The risk is way too high to forget… and let the dispute expire." (#12713)
• Name things after the domain: "Payout Account", not "Connect". (#12097)