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) ### 6. Crons and batch jobs - **Catch-up loops hide missed runs.** A `while` walking forward through skipped cycles computes credits wrong for the periods it skipped. Prefer an invariant alert that the cycle did not run. (psincraian, #12510) - Use `repository.stream` rather than a keyset loop over UUIDs. (#12749) - Loading every row in a script or sweep is a review stop. *"Won't that blow up in memory?"* (#11728, #13687) ### 7. Discounts - **Redemption counting is the whole game.** A failed payment should not count. A fully refunded order is an open question the team has not settled. (pieterbeulque, #13328) - Concurrent redemption of the same code needs a customer lock, and the caller acquires it — say so at the call site. (joebon, #13328) - `max_redemptions` and `max_redemptions_per_customer` are separate limits; a guard checking one usually needs both. (#13394) - Expiry is checked in `cycle` but has been missed elsewhere. New paths that apply a discount check it too — prefer extracting the shared check. (psincraian, #12510) - Multi-line discounts apply as a waterfall, not proportionally. Questioned in review as unusual for invoices; a change to allocation is a merchant-visible invoice change. (joebon, #12172) ### 8. Amounts, currency and tax - Fee values are basis points: 4% is `400`. Check the unit before trusting arithmetic. - Per-jurisdiction tax comes straight from the Numeral API response. Recomputing it from rates and `polar_round` introduces fractional-cent drift. (#11211) - Tax sits on transactions of type `payment`, which are on the **Polar** side and not linked to the merchant's account. Transactions reach an organization through `account_id`; `payment_organization_id` is a Pledge-era leftover that does not mean what it looks like. (#12204) - On imports, tax-inclusive versus exclusive must mirror the source provider, or the merchant loses money. (#12502) - Voiding or reversing an order credits back what was applied (`applied_balance_amount`), not the customer's current balance. (#11637) ### 9. Free and zero-amount paths These keep breaking because new code assumes a payment exists. - Keep `ProductPrice.is_free` / `Checkout.is_free_product_price` checks when reworking price handling. (#12225) - A new charge path supports a free price by skipping the payment step, not rejecting it. (#12089) - Renewal emails, invoices and receipts all have free-subscription branches. (#9291) ### 10. Billing-specific additions to rules owned elsewhere Short pointers only — the owning lens reports the general rule. - **Money tables are busy tables.** `orders`, `subscriptions`, `payments`, `customers`, `events`. Heavy backfills go in a script; indexes go in concurrently. → `ship-safety` - **Order foreign keys get `ondelete="restrict"`.** *"A `DELETE` statement is easy to spawn."* (#11206) → `ship-safety` - **Lazy loads in money paths are stuck jobs, not just 500s.** `assert order.customer` does not protect you — it raises the lazy-load error itself (#11900). When a denormalized column lands, the matching `joinedload` usually becomes dead; remove it (#12162). → `conventions-check` - **Customers cannot choose proration behaviour**, so it does not belong in the customer portal API. (#13095) → `api-surface-review` ## Output ``` ## Billing ### 🔴 Blocking - `file:line` — <invariant broken>. Fix: <fix> ### 🟠 Should fix - `file:line` — <claim>. Fix: <fix> ### 🟡 Question - `file:line` — <question> ### Notes - domain context the author may not have: <one line, or omit> ### Verdict ✅ Clean | ❌ n blocking, n should-fix ```