diff --git a/lib/api_organizations/src/organizations.rs b/lib/api_organizations/src/organizations.rs index e4c96db87..6ff90e325 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 9389efa84..f779dda07 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 8d87bd81b..c42172802 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 8c8abe65a..f79ed7d06 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": [ {