Generate a final invoice for metered usage on subscription cancel

[epompeii]

Problem

Biller::cancel_metered_subscription sent a bare CancelSubscription request. Stripe's invoice_now flag defaults to false, so an immediate cancel silently discarded any metered usage accrued since the last invoice. The customer was never billed for their final usage on either path that cancels a metered subscription: the admin plan DELETE endpoint and organization deletion.

Fix

cancel_metered_subscription now cancels with invoice_now(true), so Stripe generates a final invoice for any un-invoiced metered usage on the way out.

The licensed cancel path is unchanged: licensed plans bill upfront, so there is no un-invoiced usage to lose. Prorations stay off (prorate defaults to false) on both paths, so bare metal usage is billed in full and never prorated.

Tests

The metered_subscription lifecycle test records usage before exercising the DELETE path. It now captures the creation-time invoice id and asserts that the cancel returns a subscription whose latest_invoice is present and different, verifying that the final invoice was generated. The resubscribe_has_no_trial helper cancels with no recorded usage, so the gated Stripe test also covers the nothing-to-bill case.

Only the invoice ids are held across await points in the test; keeping the full Subscription objects alive tripped clippy::large_futures.

[github-actions]

🤖 Claude Code Review

PR: #951
Base: devel
Head: claude/invoice-metered-usage-on-cancel
Commit: b895b0ba57927f89b27dd54360fcf89b3863da50

---

I now have a full understanding of the change. Let me write up the review.

Code Review

Scope: A single, well-contained change to plus/bencher_billing/src/biller.rs. The admin/DELETE-path cancel_metered_subscription now sends CancelSubscription::new(...).invoice_now(true) so un-invoiced metered usage is billed on a final invoice instead of being silently discarded. The test is extended to assert a distinct final invoice is produced.

Overall

This is a solid, minimal fix. It correctly targets only the metered cancel path (licensed subscriptions have no accrued usage, so leaving cancel_licensed_subscription on the bare cancel_subscription helper is right). The comments are thorough and explain the why well. Compliance-wise it's clean: no emdashes, .map_err(Into::into) preserves the structured error, strong types throughout, and the change is test-backed per the TDD guidance.

Observations / minor notes

1. Justification is asserted but not tested (test-coverage). The new test proves the positive: with invoice_now(true), a new invoice distinct from pre_cancel_invoice_id is generated. It does not prove the counterfactual claimed in the comment ("a bare cancel would silently discard all usage") — there's no control asserting the old behavior. That's acceptable (a control test would be awkward against live Stripe), but the comment states as fact something the test doesn't verify. Worth being aware the safety net only catches regressions away from invoice generation, and only when TEST_BILLING_KEY is set (the whole biller() test early-returns otherwise, so plain CI never exercises this path).

2. Trialing-plan invoice assumptions. The test runs the same assertions for the Pro tier, which starts Trialing. It relies on Stripe (a) having created a prior invoice (the $0 trial invoice) and (b) generating a new final invoice for metered usage recorded during the trial. If Stripe ever declines to invoice trial-period metered usage, expect("final invoice for un-invoiced metered usage") would panic. Since this is validated against a real Stripe test account, presumably it passes today; just flagging it as an external-behavior dependency rather than a self-contained guarantee.

3. Minor divergence from the helper (simplification). cancel_metered_subscription now inlines the CancelSubscription builder rather than reusing the private cancel_subscription helper, which is now called only by cancel_licensed_subscription. This is fine (arguably clearer, and it also drops a .clone() of the id by taking ownership), but the two cancel paths now build the request in two different spots. Not worth changing — noting for consistency awareness only.

Correctness / security / performance

• Correctness: The assert_ne!(Some(final_invoice_id), pre_cancel_invoice_id) comparison is sound for both cases (prior invoice present → must differ; absent → Some != None holds), and Expandable::into_id avoids retaining the full Subscription across awaits (clippy::large_futures), as the comment notes.
• Security: No new exposure; this is the existing admin-only path.
• Performance: Neutral to slightly better (one fewer SubscriptionId clone).

Verdict: LGTM. No blocking issues; the notes above are informational.

---

_{Model: claude-opus-4-8}

[epompeii]

Addressed the review's actionable item (test assertion could pass trivially when the creation subscription had no invoice) in b895b0b.

Instead of comparing the final invoice against the creation invoice, the test now snapshots the latest invoice immediately before cancellation (reusing the already-fetched resumed subscription, so no extra Stripe call) and asserts the final invoice is distinct. Combined with the existing .expect(...), this proves a new final invoice is generated in both cases: distinct from the prior invoice when one exists, and present-therefore-new when none did.

I did not add the literal assert!(create_invoice_id.is_some()) suggestion because a Pro subscription starts in a native free trial and may not carry a creation-time invoice, which would make that assertion flaky.

The other two notes (org-delete billing side effect, customer-deletion ordering) are out of scope for this diff, as flagged.

[github-actions]

Bencher Report

Branch	claude/invoice-metered-usage-on-cancel
Testbed	intel-v1

Click to view all benchmark results

Benchmark	Latency	Benchmark Result microseconds (µs) (Result Δ%)	Upper Boundary microseconds (µs) (Limit %)
Adapter::Json	📈 view plot 🚷 view threshold	4.71 µs (+1.46%) Baseline: 4.64 µs	4.90 µs (96.08%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.58 µs (+1.45%) Baseline: 4.51 µs	4.72 µs (96.98%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.96 µs (+1.34%) Baseline: 25.62 µs	26.76 µs (97.02%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.52 µs (+0.81%) Baseline: 3.49 µs	3.61 µs (97.64%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.52 µs (+0.72%) Baseline: 3.49 µs	3.60 µs (97.62%)

🐰 View full continuous benchmarking report in Bencher