Cancel Stripe subscription when an organization is deleted

[epompeii]

Problem

When an organization has a paid Stripe subscription, deleting the organization left the subscription live in Stripe, so the customer kept being billed for an organization that no longer exists. Organization deletion (delete_inner) never touched the plan table or Stripe in either the soft-delete or the admin hard-delete path.

Fix

Cascade an immediate subscription cancellation on organization deletion, reusing the existing cancellation logic:

• New cancel_and_remove_plan helper (lib/api_organizations/src/plan.rs) cancels any live Stripe subscription and removes the local plan row. It is a no-op when there is no biller (Self-Hosted) or no plan row.
• delete_inner (lib/api_organizations/src/organizations.rs) calls it under the plus feature in both the soft-delete and admin hard-delete branches, before the organization is deleted. Cancel-first means a Stripe failure aborts the deletion, so a live subscription is never orphaned (the delete stays retryable).
• delete_plan is now status-aware for metered plans: it checks metered_plan_status and only calls cancel_metered_subscription when the subscription is still live (reusing existing_plan_action). An already-canceled subscription is skipped instead of erroring, which also makes the admin DELETE .../plan endpoint idempotent.

Edge cases

• Already-canceled or lapsed subscription (stale row): skips Stripe, removes the row, deletion succeeds.
• Transient Stripe error: surfaces as 503, deletion aborts and is retryable, never orphans.
• Licensed / Self-Hosted plans: canceled and the organization license cleared via the existing delete_plan branch.
• No biller (Self-Hosted server): no-op.

Testing

• cargo nextest run -p api_organizations --features plus: 40/40, including every deletion test (the new #[cfg(feature = "plus")] path cleanly no-ops with no biller configured).
• cargo clippy --all-targets --all-features -Dwarnings, cargo check --no-default-features, cargo test --doc, cargo fmt --check: all clean.
• OpenAPI spec regenerated (cargo gen-types); only the organization-delete endpoint description changed.

The full end-to-end "delete org cancels Stripe" path is not exercised in the standard suite because the API test harness uses biller: None with no mock Stripe. The decision logic (existing_plan_action) and the Stripe building blocks (the TEST_BILLING_KEY-gated biller() test) are covered. A biller mock for deterministic end-to-end coverage is a suggested follow-up.

[github-actions]

🤖 Claude Code Review

PR: #946
Base: devel
Head: u/ep/cancel-subscription-on-org-delete
Commit: 9c9ae4cbf6ef7b3ff788f023086e2c73b1871e86

---

Code Review: Cancel Stripe subscription when an organization is deleted

Overall this is a clean, well-reasoned change. The refactor of existing_plan_action (enum) into a single subscription_is_live predicate is a good simplification that unifies the two decisions (re-subscribe gating and cancel-vs-skip) behind one exhaustively-matched question. Error handling and ordering are carefully thought through. A few observations below, none blocking.

Correctness & robustness (positives worth confirming)

• Cancel-first ordering + error propagation is right. In cancel_and_remove_plan, the Stripe cancel is done before the org delete, and any error ?-propagates to abort the deletion. The non-atomic-window reasoning in the doc comment is sound: a failed org-delete leaves a recoverable state rather than an orphaned live subscription.
• .optional() usage is correct and important. Mapping only NotFound to None while propagating real DB errors avoids the "mistook a DB error for no-plan → deleted org with live subscription" trap. Good catch to spell this out.
• context.biller() early-return is safe. biller() returns Err only when the biller is None (Self-Hosted), so let Ok(biller) = ... else { return Ok(()) } cannot swallow a transient error. Correct.
• Transient-vs-gone distinction (metered_plan_status/licensed_plan_status → 503 on transient, None on 404) is the right safety posture, and the wildcard map_status → Unpaid (treated as live) errs toward attempting a cancel rather than silently orphaning. The new map_status test locks this in.

Observations / minor concerns

1. Soft-delete hard-removes the plan row. In the soft-delete path, cancel_and_remove_plan permanently deletes the plan row while the organization is only soft-deleted (recoverable). This is an intentional asymmetry (deletion = cancel subscription), but if soft-deleted orgs are ever restorable, the billing linkage is gone. Given the intent, this is acceptable, but worth a one-line note in the code that the plan row is intentionally hard-deleted even on soft delete.

2. Standalone plan-delete endpoint (delete_inner, plan.rs:441) still deletes the local row on a transient 503. With the new subscription_is_live gate, a transient Stripe error now surfaces as 503 from delete_plan, but the local plan row is still deleted (lines 469-471 run before the result is returned). So a transient outage can leave a live Stripe subscription with no local plan row. This is pre-existing behavior (delete-local-first), not introduced here, and cancel_and_remove_plan correctly avoids the pattern by propagating first. Flagging only so it's a conscious choice; the new org-delete path is the safer model.

3. Extra Stripe round-trip. The standalone plan cancel now issues a GET (*_plan_status) before the cancel, doubling Stripe calls on that path. Negligible given it's a rare path, and it buys the 404-idempotency, so a reasonable trade.

Standards compliance (CLAUDE.md)

• No emdashes; comments use colons/parentheses. ✅
• DateTime::now() → context.clock.now() in delete_plan — good alignment with the deterministic-clock rule. ✅
• Strong types used throughout (MeteredPlanId, LicensedPlanId, SubscriptionId, PlanStatus). ✅
• #[cfg(feature = "plus")] gating on the new import and call sites is consistent. ✅
• Exhaustive matching preserved so a new PlanStatus forces a decision. ✅
• openapi.json regenerated for the endpoint description change (no other API surface changed). ✅

Tests

Unit coverage (subscription_is_live_decides, map_status_maps_subscription_status) is solid for the pure logic. The cancel_and_remove_plan orchestration itself isn't unit-tested (it needs a live Biller), which is understandable, but consider whether an integration/test-api case exercising org-delete-with-plan is feasible to guard the wiring and cancel-first ordering.

Verdict: Approve. The two items to consider before merge are the doc note on intentional plan-row hard-delete during soft-delete (#1) and confirming the standalone-endpoint transient-error behavior (#2) is acceptable; neither blocks.

---

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

[epompeii]

Thanks for the review. Addressed in 87d81e66:

Issue #1 (licensed asymmetry) - fixed. The licensed branch in delete_plan is now status-aware, symmetric with the metered branch: it checks licensed_plan_status and only calls cancel_licensed_subscription when the subscription is still live (existing_plan_action == Conflict). Added Biller::licensed_plan_status (mirroring metered_plan_status, mapping a Stripe 404 to None) and factored the shared 404-to-None logic into a private subscription_status helper. An already-canceled or gone licensed subscription no longer aborts organization deletion.

Issue #2 (503 on Stripe outage) - intentional. Aborting the delete when the subscription status can't be resolved is the deliberate fail-safe: better to block deletion transiently than to delete the org and orphan a possibly-live subscription. It matches the prune_or_conflict_existing_plan precedent, and the delete is retryable once Stripe recovers.

Issue #3 (non-atomic partial-failure window) - acknowledged. A Stripe call can't sit inside the DB transaction, so cancel plus local-row-delete, then org-delete, is inherently two steps. As noted it is recoverable: a retry short-circuits on the now-missing plan row (and a gone/terminal subscription is skipped), then proceeds.

Testing. The no biller -> Ok early return is exercised by the existing integration tests (organizations_delete, organizations_hard_delete_as_admin) run under --features plus with biller: None (they pass). The no plan row -> Ok return needs a biller present with no plan row, which the harness can't set up without a Stripe mock; a biller mock for deterministic end-to-end coverage remains the suggested follow-up. The shared decision logic stays covered by existing_plan_action_decides.

[github-actions]

Bencher Report

Branch	u/ep/cancel-subscription-on-org-delete
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.70 µs (+1.24%) Baseline: 4.64 µs	4.90 µs (95.86%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.57 µs (+1.27%) Baseline: 4.51 µs	4.72 µs (96.78%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.47 µs (-0.55%) Baseline: 25.61 µs	26.77 µs (95.16%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.50 µs (+0.29%) Baseline: 3.49 µs	3.61 µs (97.08%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.50 µs (+0.41%) Baseline: 3.49 µs	3.60 µs (97.29%)

🐰 View full continuous benchmarking report in Bencher

[epompeii]

Thanks, another good catch. Addressed in 69813289:

Finding #1 (swallowing all DB errors as "no plan") - fixed. cancel_and_remove_plan now uses diesel .optional(), so only NotFound becomes the no-op and every other database error propagates (mapped to a conflict) and aborts the delete. A transient DB error can no longer let the org be deleted while its subscription is still live.

Finding #2 (tests). Added a deterministic unit test (map_status_maps_subscription_status) for the Stripe-status to PlanStatus mapping that feeds metered_plan_status/licensed_plan_status and therefore the cancel-vs-skip decision. Together with the existing existing_plan_action_decides, the substantive new decision logic ("cancel only when live; skip when gone/terminal") is unit-covered.

The remaining paths you list (cancel_and_remove_plan past the biller guard, the cancel-then-remove and error-propagation flow, and subscription_status/licensed_plan_status 404-vs-error) all require a Biller, which the API test harness constructs as None and cannot build without live Stripe credentials (Biller::new resolves products against Stripe on startup). They are therefore unreachable in the deterministic suite; the TEST_BILLING_KEY-gated biller() test is where the real Stripe round-trips run. A Biller trait/mock to make the full cancel path unit-testable is the right follow-up, but a larger, separate refactor than this fix should carry.

Observations.

• Partial-failure window: added a doc comment on cancel_and_remove_plan explaining the non-atomic Stripe+DB boundary and why cancel-first is the safe ordering.
• Naming overlap: added doc cross-references between licensed_plan_status (Option, 404 to None) and get_licensed_plan_status (errors on 404).
• Soft delete removing the plan row: agreed there is no org restore path today, so this is intended; a live subscription must not survive the delete.