diff --git a/config/locales/en.yml b/config/locales/en.yml index 4d6343d9c..87ffc7298 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -175,6 +175,7 @@ en: currency_switch_unavailable: Currency switching is not available for this account unsupported_currency: This currency is not supported same_currency: This account is already billed in the selected currency + switch_in_progress: A currency switch is already in progress. Please try again in a moment. stripe_customer_not_configured: Stripe customer not configured switch_requires_active_subscription: Currency can only be changed when you have a single active subscription. Please resolve any pending billing first. unknown_plan: Could not determine the current plan diff --git a/enterprise/app/services/enterprise/billing/switch_currency_service.rb b/enterprise/app/services/enterprise/billing/switch_currency_service.rb index 155aec1d6..d8739de62 100644 --- a/enterprise/app/services/enterprise/billing/switch_currency_service.rb +++ b/enterprise/app/services/enterprise/billing/switch_currency_service.rb @@ -1,9 +1,10 @@ # Orchestrates a billing currency switch: -# eligibility (no mutation) -> resolve target price -> mark pending -> Stripe swap (self-reverting) -# -> persist local state (last). Each concern lives in its own collaborator so this stays a thin -# coordinator. Any failure aborts before persisting, so Chatwoot is never left ahead of Stripe; the -# rare window where Stripe succeeds but the local persist fails is reconciled by the subscription -# webhook, which also clears the pending marker. +# acquire per-account lock -> eligibility (no mutation) -> resolve target price -> Stripe swap +# (self-reverting) -> persist local state (last). Each concern lives in its own collaborator so this +# stays a thin coordinator. The pending marker is set under a row lock first, so a second concurrent +# switch is rejected before it can create a duplicate Stripe subscription. Any failure aborts before +# persisting, so Chatwoot is never left ahead of Stripe; the rare window where Stripe succeeds but the +# local persist fails is reconciled by the subscription webhook, which also clears the pending marker. class Enterprise::Billing::SwitchCurrencyService include BillingHelper @@ -12,33 +13,60 @@ class Enterprise::Billing::SwitchCurrencyService # Tags a cancelled sub so the deleted-webhook skips re-subscribing the default plan. SWITCH_METADATA_KEY = 'chatwoot_currency_switch'.freeze - # Records the in-flight target currency so a crash mid-switch is visible; cleared on success or by - # the subscription webhook once it reconciles the final state from Stripe. + # Records the in-flight switch so a second concurrent request is rejected and a crash mid-switch is + # visible; cleared on success or by the subscription webhook once it reconciles the state from Stripe. PENDING_CURRENCY_KEY = 'billing_currency_switch_pending'.freeze + # A pending marker older than this is treated as abandoned (e.g. a crashed prior attempt), so a stuck + # marker can't block switches forever. Switches complete in seconds, so this is comfortably generous. + STALE_SWITCH_SECONDS = 10.minutes.to_i + pattr_initialize [:account!, :currency!] def perform - subscription = eligibility.subscription! - resolver = Enterprise::Billing::PlanPriceResolver.new(subscription: subscription, target_currency: target_currency) - plan = resolver.plan - change = change_for(subscription, resolver.target_price_id, default_plan: Enterprise::Billing::PlanConfiguration.default_plan?(plan)) + acquire_switch_lock! - mark_pending - new_subscription = executor.execute(subscription: subscription, change: change) + begin + subscription = eligibility.subscription! + resolver = Enterprise::Billing::PlanPriceResolver.new(subscription: subscription, target_currency: target_currency) + plan = resolver.plan + change = change_for(subscription, resolver.target_price_id, default_plan: Enterprise::Billing::PlanConfiguration.default_plan?(plan)) - persist_currency(build_custom_attributes(new_subscription, plan)) - Enterprise::Billing::ReconcilePlanFeaturesService.new(account: account).perform - rescue Enterprise::Billing::CurrencySwitchEligibility::Error, - Enterprise::Billing::PlanPriceResolver::Error, - Enterprise::Billing::StripeCurrencySwitchExecutor::Error => e - # The Stripe swap self-reverted, so drop the pending marker and surface a single error type. - clear_pending - raise Error, e.message + new_subscription = executor.execute(subscription: subscription, change: change) + + persist_currency(build_custom_attributes(new_subscription, plan)) + Enterprise::Billing::ReconcilePlanFeaturesService.new(account: account).perform + rescue Enterprise::Billing::CurrencySwitchEligibility::Error, + Enterprise::Billing::PlanPriceResolver::Error, + Enterprise::Billing::StripeCurrencySwitchExecutor::Error => e + # The Stripe swap self-reverted, so drop the pending marker and surface a single error type. + clear_pending + raise Error, e.message + end end private + # Reject a second switch for this account while one is in flight. The check-and-set runs under a row + # lock so two concurrent requests can't both pass it and create duplicate Stripe subscriptions. + def acquire_switch_lock! + account.with_lock do + raise Error, I18n.t('errors.billing.switch_in_progress') if switch_in_progress? + + account.update!(custom_attributes: account.custom_attributes.merge( + PENDING_CURRENCY_KEY => { 'currency' => target_currency, 'started_at' => Time.current.to_i } + )) + end + end + + def switch_in_progress? + marker = account.custom_attributes[PENDING_CURRENCY_KEY] + return false if marker.blank? + + started_at = marker.is_a?(Hash) ? marker['started_at'].to_i : 0 + Time.current.to_i - started_at < STALE_SWITCH_SECONDS + end + def eligibility @eligibility ||= Enterprise::Billing::CurrencySwitchEligibility.new(account: account, currency: currency) end @@ -76,10 +104,6 @@ class Enterprise::Billing::SwitchCurrencyService ) end - def mark_pending - account.update!(custom_attributes: account.custom_attributes.merge(PENDING_CURRENCY_KEY => target_currency)) - end - def clear_pending account.update!(custom_attributes: account.custom_attributes.except(PENDING_CURRENCY_KEY)) end diff --git a/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb b/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb index de2f754ba..100c888ec 100644 --- a/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb +++ b/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb @@ -117,6 +117,25 @@ describe Enterprise::Billing::SwitchCurrencyService do expect { service.perform }.to raise_error(described_class::Error, I18n.t('errors.billing.switch_requires_active_subscription')) end + it 'rejects a switch while another is already in progress and leaves the marker intact' do + account.update!(custom_attributes: account.custom_attributes.merge( + 'billing_currency_switch_pending' => { 'currency' => 'brl', 'started_at' => Time.current.to_i } + )) + + expect { service.perform }.to raise_error(described_class::Error, I18n.t('errors.billing.switch_in_progress')) + expect(Stripe::Subscription).not_to have_received(:create) + expect(account.reload.custom_attributes['billing_currency_switch_pending']).to be_present + end + + it 'proceeds when the in-progress marker is stale' do + account.update!(custom_attributes: account.custom_attributes.merge( + 'billing_currency_switch_pending' => { 'currency' => 'brl', 'started_at' => 1.hour.ago.to_i } + )) + + expect { service.perform }.not_to raise_error + expect(account.reload.custom_attributes['billing_currency']).to eq('brl') + end + it 'raises when the target currency is not configured for the plan' do InstallationConfig.find_by(name: 'CHATWOOT_CLOUD_PLANS').update!(value: [ { 'name' => 'Business', 'product_id' => ['prod_business'],