From 9c9ae4cbf6ef7b3ff788f023086e2c73b1871e86 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Thu, 16 Jul 2026 03:13:45 +0000 Subject: [PATCH] Cancel Stripe subscription when an organization is deleted Deleting an organization left its Stripe subscription live, so the customer kept being billed for an organization that no longer exists. Cancel any active subscription and remove the local plan row before deleting the organization, in both the soft-delete and admin hard-delete paths. Cancel first so a Stripe failure aborts the deletion and never orphans a live subscription. Make delete_plan status-aware for both metered and licensed plans so an already-canceled or gone subscription is skipped instead of erroring, which keeps the user-facing delete from failing and also makes the admin plan-delete endpoint idempotent. --- lib/api_organizations/src/organizations.rs | 15 ++ lib/api_organizations/src/plan.rs | 155 ++++++++++++++------- plus/bencher_billing/src/biller.rs | 87 +++++++++++- services/api/openapi.json | 2 +- 4 files changed, 203 insertions(+), 56 deletions(-) diff --git a/lib/api_organizations/src/organizations.rs b/lib/api_organizations/src/organizations.rs index e4c96db875..6ff90e325d 100644 --- a/lib/api_organizations/src/organizations.rs +++ b/lib/api_organizations/src/organizations.rs @@ -25,6 +25,9 @@ use dropshot::{HttpError, Path, Query, RequestContext, TypedBody, endpoint}; use schemars::JsonSchema; use serde::Deserialize; +#[cfg(feature = "plus")] +use crate::plan::cancel_and_remove_plan; + pub type OrganizationsPagination = JsonPagination; #[derive(Debug, Clone, Copy, Default, Deserialize, JsonSchema)] @@ -334,6 +337,7 @@ async fn patch_inner( /// The user must have `delete` permissions for the organization. /// By default, organizations are soft-deleted (along with their child projects). /// Set the `hard` query parameter to `true` to permanently delete the organization (requires server admin). +/// On Bencher Cloud, deleting an organization also immediately cancels any active subscription. #[endpoint { method = DELETE, path = "/v0/organizations/{organization}", @@ -384,6 +388,12 @@ async fn delete_inner( Organization, &path_params.organization ))?; + + // Cancel any active Stripe subscription and remove the plan row before deleting the + // organization so no dangling subscription is left in Stripe. + #[cfg(feature = "plus")] + cancel_and_remove_plan(context, &query_organization).await?; + diesel::delete( schema::organization::table.filter(schema::organization::id.eq(query_organization.id)), ) @@ -399,6 +409,11 @@ async fn delete_inner( Permission::Delete, )?; + // Cancel any active Stripe subscription and remove the plan row before deleting the + // organization so no dangling subscription is left in Stripe. + #[cfg(feature = "plus")] + cancel_and_remove_plan(context, &query_organization).await?; + // Soft delete the organization and all its child projects let now = context.clock.now(); query_organization.soft_delete(context, log, now).await?; diff --git a/lib/api_organizations/src/plan.rs b/lib/api_organizations/src/plan.rs index 9389efa843..f779dda070 100644 --- a/lib/api_organizations/src/plan.rs +++ b/lib/api_organizations/src/plan.rs @@ -5,7 +5,7 @@ use bencher_endpoint::{ CorsResponse, Delete, Endpoint, Get, Patch, Post, ResponseCreated, ResponseDeleted, ResponseOk, }; use bencher_json::{ - DateTime, OrganizationResourceId, PlanStatus, + OrganizationResourceId, PlanStatus, organization::plan::{JsonNewPlan, JsonPlan, JsonUpdatePlan}, }; use bencher_rbac::organization::Permission; @@ -28,7 +28,10 @@ use bencher_schema::{ }, schema, write_conn, }; -use diesel::{BelongingToDsl as _, ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; +use diesel::{ + BelongingToDsl as _, ExpressionMethods as _, OptionalExtension as _, QueryDsl as _, + RunQueryDsl as _, +}; use dropshot::{HttpError, Path, Query, RequestContext, TypedBody, endpoint}; use schemars::JsonSchema; use serde::Deserialize; @@ -350,14 +353,16 @@ async fn prune_or_conflict_existing_plan( .await .map_err(service_unavailable_error)?; - match existing_plan_action(status) { - ExistingPlan::Conflict => Err(plan_conflict(query_organization, query_plan)), - ExistingPlan::Prune => { - diesel::delete(schema::plan::table.filter(schema::plan::id.eq(query_plan.id))) - .execute(write_conn!(context)) - .map_err(resource_conflict_err!(Plan, query_plan))?; - Ok(()) - }, + if subscription_is_live(status) { + // Still live in Stripe (active, trialing, or dunning): block, so we never orphan a + // live subscription and let the org create a duplicate. + Err(plan_conflict(query_organization, query_plan)) + } else { + // Gone or terminal: prune the stale row so the org can subscribe again. + diesel::delete(schema::plan::table.filter(schema::plan::id.eq(query_plan.id))) + .execute(write_conn!(context)) + .map_err(resource_conflict_err!(Plan, query_plan))?; + Ok(()) } } @@ -371,23 +376,21 @@ fn plan_conflict(query_organization: &QueryOrganization, query_plan: QueryPlan) ) } -/// Whether an organization's existing metered plan row blocks creating a new plan, or is -/// a stale row to prune. `status` is `None` when Stripe reports the subscription gone -/// (404), else its live status. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum ExistingPlan { - Conflict, - Prune, -} - -/// Prune (allow re-subscribe) only when the subscription is gone or *terminal* (canceled -/// / incomplete-expired). An active, trialing, or recoverable (dunning) subscription -/// still exists in Stripe and must block, so we never orphan a live subscription and let -/// the org create a duplicate. Matched exhaustively so a new `PlanStatus` forces a -/// decision here. -fn existing_plan_action(status: Option) -> ExistingPlan { +/// Whether a subscription is still live in Stripe, given its status (`None` when Stripe +/// reports it gone via a 404). +/// +/// Live means active, trialing, or in a recoverable dunning state (`past_due` / `unpaid` / +/// `incomplete` / `paused`): the subscription still exists in Stripe. Not live means gone +/// (404) or *terminal* (canceled / incomplete-expired). Matched exhaustively so a new +/// `PlanStatus` forces a decision here. +/// +/// This answers the single question behind two separate decisions: plan creation blocks on +/// a live subscription (otherwise it prunes the stale row so the org can re-subscribe), and +/// plan deletion cancels a live subscription (otherwise it skips, since canceling a +/// gone/terminal one would 404). +fn subscription_is_live(status: Option) -> bool { match status { - None | Some(PlanStatus::Canceled | PlanStatus::IncompleteExpired) => ExistingPlan::Prune, + None | Some(PlanStatus::Canceled | PlanStatus::IncompleteExpired) => false, Some( PlanStatus::Active | PlanStatus::Trialing @@ -395,7 +398,7 @@ fn existing_plan_action(status: Option) -> ExistingPlan { | PlanStatus::Unpaid | PlanStatus::Incomplete | PlanStatus::Paused, - ) => ExistingPlan::Conflict, + ) => true, } } @@ -470,6 +473,46 @@ async fn delete_inner( delete_plan_result } +/// Cancel an organization's live Stripe subscription (if any) and remove its local plan +/// row. Called from organization deletion so no dangling subscription is left in Stripe +/// when an organization is deleted. A no-op when the server has no biller (Self-Hosted) or +/// the organization has no plan row. +/// +/// The Stripe cancel cannot share a database transaction with the caller's organization +/// delete, so there is a small non-atomic window. Cancel-first ordering keeps it on the safe +/// side: if the org delete then fails, the org is briefly present with its subscription +/// already canceled (recoverable on retry) rather than deleted with a live subscription. +pub(crate) async fn cancel_and_remove_plan( + context: &ApiContext, + query_organization: &QueryOrganization, +) -> Result<(), HttpError> { + // Only Bencher Cloud has a biller and plan rows; nothing to cancel otherwise. + let Ok(biller) = context.biller() else { + return Ok(()); + }; + // A missing plan row means nothing to cancel, but a real database error must not be + // mistaken for "no plan" (that would delete the org while its subscription is still + // live). `.optional()` maps only `NotFound` to `None` and propagates every other error. + let Some(query_plan) = QueryPlan::belonging_to(query_organization) + .first::(auth_conn!(context)) + .optional() + .map_err(resource_conflict_err!(Plan, query_organization))? + else { + return Ok(()); + }; + + // Cancel in Stripe first; propagate the error (aborting the organization deletion) if it + // fails so we never delete the organization while its subscription is still live. + delete_plan(context, biller, query_organization, &query_plan, true).await?; + + // The subscription is canceled (or was already gone); remove the local plan row. + diesel::delete(schema::plan::table.filter(schema::plan::id.eq(query_plan.id))) + .execute(write_conn!(context)) + .map_err(resource_conflict_err!(Plan, query_plan))?; + + Ok(()) +} + async fn delete_plan( context: &ApiContext, biller: &Biller, @@ -479,17 +522,35 @@ async fn delete_plan( ) -> Result<(), HttpError> { if let Some(metered_plan_id) = query_plan.metered_plan.as_ref() { if remote { - biller - .cancel_metered_subscription(metered_plan_id) + // Only cancel a subscription still live in Stripe; a gone/terminal one is + // already effectively canceled, so canceling it again would 404. A transient + // Stripe error surfaces as 503 rather than being mistaken for "gone". + let status = biller + .metered_plan_status(metered_plan_id) .await - .map_err(resource_not_found_err!(Plan, query_plan))?; + .map_err(service_unavailable_error)?; + if subscription_is_live(status) { + biller + .cancel_metered_subscription(metered_plan_id) + .await + .map_err(resource_not_found_err!(Plan, query_plan))?; + } } } else if let Some(licensed_plan_id) = query_plan.licensed_plan.as_ref() { if remote { - biller - .cancel_licensed_subscription(licensed_plan_id) + // Only cancel a subscription still live in Stripe; a gone/terminal one is + // already effectively canceled, so canceling it again would 404. A transient + // Stripe error surfaces as 503 rather than being mistaken for "gone". + let status = biller + .licensed_plan_status(licensed_plan_id) .await - .map_err(resource_not_found_err!(Plan, query_plan))?; + .map_err(service_unavailable_error)?; + if subscription_is_live(status) { + biller + .cancel_licensed_subscription(licensed_plan_id) + .await + .map_err(resource_not_found_err!(Plan, query_plan))?; + } } if query_organization.license.is_some() { @@ -499,7 +560,7 @@ async fn delete_plan( name: None, slug: None, license: Some(None), - modified: DateTime::now(), + modified: context.clock.now(), }; diesel::update(organization_query) .set(&update_organization) @@ -523,22 +584,18 @@ async fn delete_plan( mod tests { use bencher_json::PlanStatus; - use super::{ExistingPlan, existing_plan_action}; + use super::subscription_is_live; #[test] - fn existing_plan_action_decides() { - // Gone in Stripe (404) or terminal: prune the stale row. - assert_eq!(existing_plan_action(None), ExistingPlan::Prune); - assert_eq!( - existing_plan_action(Some(PlanStatus::Canceled)), - ExistingPlan::Prune, - ); - assert_eq!( - existing_plan_action(Some(PlanStatus::IncompleteExpired)), - ExistingPlan::Prune, - ); - // Active, trialing, or recoverable (dunning) still exists in Stripe: block, so we - // never orphan a live subscription and let the org create a duplicate. + fn subscription_is_live_decides() { + // Gone in Stripe (404) or terminal: not live, so plan creation prunes the stale row + // and plan deletion skips canceling. + assert!(!subscription_is_live(None)); + assert!(!subscription_is_live(Some(PlanStatus::Canceled))); + assert!(!subscription_is_live(Some(PlanStatus::IncompleteExpired))); + // Active, trialing, or recoverable (dunning) still exists in Stripe: live, so plan + // creation blocks (never orphan a live subscription and let the org create a + // duplicate) and plan deletion cancels. for status in [ PlanStatus::Active, PlanStatus::Trialing, @@ -547,7 +604,7 @@ mod tests { PlanStatus::Incomplete, PlanStatus::Paused, ] { - assert_eq!(existing_plan_action(Some(status)), ExistingPlan::Conflict); + assert!(subscription_is_live(Some(status))); } } } diff --git a/plus/bencher_billing/src/biller.rs b/plus/bencher_billing/src/biller.rs index 8d87bd81b6..c421728027 100644 --- a/plus/bencher_billing/src/biller.rs +++ b/plus/bencher_billing/src/biller.rs @@ -836,18 +836,41 @@ impl Biller { }) } - /// Resolve a metered subscription's live status for re-subscription gating: - /// `Some(status)` when the subscription exists, and `None` when Stripe reports it no - /// longer exists (a definitive 404, safe to treat as gone). Returns an error only for - /// indeterminate failures (network, 5xx, rate limit), so a transient outage is never - /// mistaken for "gone" (which would risk pruning a still-live subscription). The + /// Resolve a metered subscription's live status for the two decisions keyed on whether + /// it is still live in Stripe (re-subscription gating on plan creation, and + /// cancel-vs-skip on plan deletion): `Some(status)` when the subscription exists, and + /// `None` when Stripe reports it no longer exists (a definitive 404, safe to treat as + /// gone). Returns an error only for indeterminate failures (network, 5xx, rate limit), + /// so a transient outage is never mistaken for "gone" (which would risk orphaning a + /// still-live subscription: pruned on creation, or left uncanceled on deletion). The /// caller distinguishes terminal from recoverable (dunning) statuses. pub async fn metered_plan_status( &self, metered_plan_id: &MeteredPlanId, ) -> Result, BillingError> { let subscription_id: SubscriptionId = metered_plan_id.as_ref().into(); - match self.get_subscription(&subscription_id).await { + self.subscription_status(&subscription_id).await + } + + /// The live status of a licensed subscription, or `None` when Stripe reports it gone + /// (404). Mirrors [`Self::metered_plan_status`] so callers can tell a still-live + /// subscription from one that is already gone or terminal. Unlike + /// [`Self::get_licensed_plan_status`], a 404 is reported as `None` rather than an error. + pub async fn licensed_plan_status( + &self, + licensed_plan_id: &LicensedPlanId, + ) -> Result, BillingError> { + let subscription_id: SubscriptionId = licensed_plan_id.as_ref().into(); + self.subscription_status(&subscription_id).await + } + + /// The live status of a subscription, or `None` when Stripe reports it gone (404). A + /// transient Stripe error is returned as-is so callers can distinguish it from "gone". + async fn subscription_status( + &self, + subscription_id: &SubscriptionId, + ) -> Result, BillingError> { + match self.get_subscription(subscription_id).await { Ok(subscription) => Ok(Some(Self::map_status(&subscription.status))), Err(BillingError::Stripe(stripe::StripeError::Stripe(_, 404))) => Ok(None), Err(e) => Err(e), @@ -868,6 +891,9 @@ impl Biller { ) } + /// The status of a licensed subscription, erroring if Stripe reports it gone (404). For + /// callers that must treat a gone subscription as absent rather than an error, see + /// [`Self::licensed_plan_status`]. pub async fn get_licensed_plan_status( &self, licensed_plan_id: &LicensedPlanId, @@ -1063,6 +1089,55 @@ mod tests { assert_eq!(plan_level_from_pro_price(false), PlanLevel::Team); } + #[test] + fn map_status_maps_subscription_status() { + use stripe_billing::SubscriptionStatus; + + // Each Stripe subscription status maps to the matching plan status. This mapping + // feeds `metered_plan_status`/`licensed_plan_status`, which drive the + // cancel-vs-skip decision in `subscription_is_live`, so a wrong mapping (e.g. a + // terminal status reading as `Active`) would cancel or skip incorrectly. + assert_eq!( + Biller::map_status(&SubscriptionStatus::Active), + PlanStatus::Active, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::Canceled), + PlanStatus::Canceled, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::Incomplete), + PlanStatus::Incomplete, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::IncompleteExpired), + PlanStatus::IncompleteExpired, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::PastDue), + PlanStatus::PastDue, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::Paused), + PlanStatus::Paused, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::Trialing), + PlanStatus::Trialing, + ); + assert_eq!( + Biller::map_status(&SubscriptionStatus::Unpaid), + PlanStatus::Unpaid, + ); + // An unrecognized/future Stripe status falls through the wildcard arm to `Unpaid`. + // That is the safe side for deletion: `Unpaid` reads as live, so a cancel is + // attempted rather than silently skipped and left to orphan. + assert_eq!( + Biller::map_status(&SubscriptionStatus::Unknown("future_status".to_owned())), + PlanStatus::Unpaid, + ); + } + #[test] fn get_plan_unit_amount_reads_price() { let mut item = make_subscription_item("price_metrics"); diff --git a/services/api/openapi.json b/services/api/openapi.json index 8c8abe65ab..f79ed7d066 100644 --- a/services/api/openapi.json +++ b/services/api/openapi.json @@ -1172,7 +1172,7 @@ "organizations" ], "summary": "Delete an organization", - "description": "Delete an organization where the user is a member. The user must have `delete` permissions for the organization. By default, organizations are soft-deleted (along with their child projects). Set the `hard` query parameter to `true` to permanently delete the organization (requires server admin).", + "description": "Delete an organization where the user is a member. The user must have `delete` permissions for the organization. By default, organizations are soft-deleted (along with their child projects). Set the `hard` query parameter to `true` to permanently delete the organization (requires server admin). On Bencher Cloud, deleting an organization also immediately cancels any active subscription.", "operationId": "organization_delete", "parameters": [ {