diff --git a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs index e46a579b3a..4f7876832e 100644 --- a/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs +++ b/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs @@ -115,12 +115,7 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler await VoidOpenInvoices(subscription.Id); } - // Only disable premium if the user doesn't have other active premium subscriptions - // This prevents incorrectly disabling premium when webhook events arrive out of order - if (!await HasOtherActivePremiumSubscriptionsAsync(userId.Value, subscription.Id)) - { - await _userService.DisablePremiumAsync(userId.Value, currentPeriodEnd); - } + await _userService.DisablePremiumAsync(userId.Value, currentPeriodEnd); break; } @@ -134,12 +129,7 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler await CancelSubscription(subscription.Id); await VoidOpenInvoices(subscription.Id); - // Only disable premium if the user doesn't have other active premium subscriptions - // This prevents incorrectly disabling premium when webhook events arrive out of order - if (!await HasOtherActivePremiumSubscriptionsAsync(userId.Value, subscription.Id)) - { - await _userService.DisablePremiumAsync(userId.Value, currentPeriodEnd); - } + await _userService.DisablePremiumAsync(userId.Value, currentPeriodEnd); } break; @@ -173,11 +163,20 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler } case StripeSubscriptionStatus.Active: { - if (userId.HasValue) + if (userId.HasValue) + { + // When a Premium user subscription becomes active, proactively clean up any + // other active premium subscriptions left over from previous failed attempts. + // This prevents users from being double-charged and avoids duplicate + // subscriptions lingering on the account. + if (await IsPremiumSubscriptionAsync(subscription)) { - await _userService.EnablePremiumAsync(userId.Value, currentPeriodEnd); + await CancelOtherPremiumSubscriptionsAsync(userId.Value, subscription.Id); } - break; + + await _userService.EnablePremiumAsync(userId.Value, currentPeriodEnd); + } + break; } } @@ -219,64 +218,65 @@ public class SubscriptionUpdatedHandler : ISubscriptionUpdatedHandler var premiumPlans = await _pricingClient.ListPremiumPlans(); var premiumPriceIds = premiumPlans.SelectMany(p => new[] { p.Seat.StripePriceId, p.Storage.StripePriceId }).ToHashSet(); - // Add hardcoded fallbacks for safety to ensure all premium plan variants are covered - premiumPriceIds.Add(IStripeEventUtilityService.PremiumPlanId); - premiumPriceIds.Add(IStripeEventUtilityService.PremiumPlanIdAppStore); - return subscription.Items.Any(i => premiumPriceIds.Contains(i.Price.Id)); } /// - /// Checks if a user has other active premium subscriptions besides the current one being processed. - /// This prevents incorrectly disabling premium when a user has multiple subscriptions due to payment retry scenarios. + /// Cancels any other premium subscriptions for the given user, leaving the current + /// subscription as the single source of truth. This is used when a new subscription + /// successfully becomes active after previous failed attempts, to prevent duplicate + /// subscriptions and overcharging. /// - /// The user ID to check - /// The current subscription being processed (to exclude from the check) - /// True if the user has other active premium subscriptions, false otherwise - private async Task HasOtherActivePremiumSubscriptionsAsync(Guid userId, string currentSubscriptionId) + /// The user ID whose subscriptions should be cleaned up. + /// The active subscription that should be kept. + private async Task CancelOtherPremiumSubscriptionsAsync(Guid userId, string currentSubscriptionId) { var user = await _userService.GetUserByIdAsync(userId); if (user == null || string.IsNullOrEmpty(user.Email)) { - return false; + return; } - // Find the customer by email var customerOptions = new CustomerListOptions { Email = user.Email, - Limit = 1 + Limit = 100 }; + var customers = await _stripeFacade.ListCustomers(customerOptions); - var customer = customers.FirstOrDefault(); - if (customer == null) + foreach (var customer in customers) { - return false; - } - - // List all active subscriptions for this customer - var subscriptionOptions = new SubscriptionListOptions - { - Customer = customer.Id, - Status = "active" - }; - - var activeSubscriptions = await _stripeFacade.ListSubscriptions(subscriptionOptions); - - // Check if any other subscription (not the current one) is a premium subscription - foreach (var sub in activeSubscriptions.Where(s => s.Id != currentSubscriptionId)) - { - if (await IsPremiumSubscriptionAsync(sub)) + var subscriptionOptions = new SubscriptionListOptions { - _logger.LogInformation( - "User {UserId} has another active premium subscription {SubscriptionId} while processing {CurrentSubscriptionId}", - userId, sub.Id, currentSubscriptionId); - return true; + Customer = customer.Id + }; + + var subscriptions = await _stripeFacade.ListSubscriptions(subscriptionOptions); + + foreach (var sub in subscriptions.Where(s => s.Id != currentSubscriptionId)) + { + // Only cancel subscriptions that are premium and in a status where cancellation + // makes sense (they are or were granting access / being billed). + if (sub.Status is StripeSubscriptionStatus.Active + or StripeSubscriptionStatus.Trialing + or StripeSubscriptionStatus.PastDue + or StripeSubscriptionStatus.Incomplete + or StripeSubscriptionStatus.Unpaid + or StripeSubscriptionStatus.IncompleteExpired) + { + if (await IsPremiumSubscriptionAsync(sub)) + { + _logger.LogInformation( + "Cancelling duplicate premium subscription {SubscriptionId} for user {UserId} while keeping {CurrentSubscriptionId}", + sub.Id, userId, currentSubscriptionId); + + await CancelSubscription(sub.Id); + await VoidOpenInvoices(sub.Id); + } + } } } - - return false; } /// diff --git a/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs b/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs index 53ab91f9c9..c68437703c 100644 --- a/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs +++ b/test/Billing.Test/Services/SubscriptionUpdatedHandlerTests.cs @@ -548,6 +548,7 @@ public class SubscriptionUpdatedHandlerTests { // Arrange var userId = Guid.NewGuid(); + var userEmail = "active-user@example.com"; var currentPeriodEnd = DateTime.UtcNow.AddDays(30); var subscription = new Subscription { @@ -564,6 +565,16 @@ public class SubscriptionUpdatedHandlerTests var parsedEvent = new Event { Data = new EventData() }; + var user = new Core.Entities.User { Id = userId, Email = userEmail }; + _userService.GetUserByIdAsync(userId).Returns(user); + + var customer = new Customer { Id = "cus_123", Email = userEmail }; + _stripeFacade.ListCustomers(Arg.Is(o => o.Email == userEmail)) + .Returns(new StripeList { Data = new List { customer } }); + + _stripeFacade.ListSubscriptions(Arg.Is(o => o.Customer == customer.Id)) + .Returns(new StripeList { Data = new List { subscription } }); + _stripeEventService.GetSubscription(Arg.Any(), Arg.Any(), Arg.Any>()) .Returns(subscription); @@ -578,6 +589,8 @@ public class SubscriptionUpdatedHandlerTests .EnablePremiumAsync(userId, currentPeriodEnd); await _userService.Received(1) .UpdatePremiumExpirationAsync(userId, currentPeriodEnd); + await _stripeFacade.DidNotReceive() + .CancelSubscription(subscription.Id, Arg.Any()); } [Fact] @@ -1309,6 +1322,98 @@ public class SubscriptionUpdatedHandlerTests .CancelSubscription(incompleteSubscriptionId, Arg.Any()); } + [Fact] + public async Task HandleAsync_ActiveUserSubscription_CancelsOtherActivePremiumSubscriptions() + { + // Arrange + var userId = Guid.NewGuid(); + var userEmail = "duplicate-premium@example.com"; + var currentPeriodEnd = DateTime.UtcNow.AddDays(30); + var currentSubscriptionId = "sub_current"; + var otherSubscriptionId = "sub_other"; + var customerId = "cus_123"; + + var currentSubscription = new Subscription + { + Id = currentSubscriptionId, + Status = StripeSubscriptionStatus.Active, + Metadata = new Dictionary { { "userId", userId.ToString() } }, + Items = new StripeList + { + Data = + [ + new SubscriptionItem + { + CurrentPeriodEnd = currentPeriodEnd, + Price = new Price { Id = IStripeEventUtilityService.PremiumPlanId } + } + ] + } + }; + + var otherActiveSubscription = new Subscription + { + Id = otherSubscriptionId, + Status = StripeSubscriptionStatus.Active, + Items = new StripeList + { + Data = + [ + new SubscriptionItem + { + CurrentPeriodEnd = currentPeriodEnd, + Price = new Price { Id = IStripeEventUtilityService.PremiumPlanId } + } + ] + } + }; + + var parsedEvent = new Event { Data = new EventData() }; + + var premiumPlan = new PremiumPlan + { + Name = "Premium", + Available = true, + LegacyYear = null, + Seat = new PremiumPurchasable { Price = 10M, StripePriceId = IStripeEventUtilityService.PremiumPlanId }, + Storage = new PremiumPurchasable { Price = 4M, StripePriceId = "storage-plan-personal" } + }; + _pricingClient.ListPremiumPlans().Returns(new List { premiumPlan }); + + _stripeEventService.GetSubscription(Arg.Any(), Arg.Any(), Arg.Any>()) + .Returns(currentSubscription); + + _stripeEventUtilityService.GetIdsFromMetadata(Arg.Any>()) + .Returns(Tuple.Create(null, userId, null)); + + var user = new Core.Entities.User { Id = userId, Email = userEmail }; + _userService.GetUserByIdAsync(userId).Returns(user); + + var customer = new Customer { Id = customerId, Email = userEmail }; + _stripeFacade.ListCustomers(Arg.Is(o => o.Email == userEmail)) + .Returns(new StripeList { Data = new List { customer } }); + + _stripeFacade.ListSubscriptions(Arg.Is(o => o.Customer == customerId)) + .Returns(new StripeList + { + Data = new List { currentSubscription, otherActiveSubscription } + }); + + _stripeFacade.ListInvoices(Arg.Any()) + .Returns(new StripeList { Data = new List() }); + + // Act + await _sut.HandleAsync(parsedEvent); + + // Assert - the duplicate active subscription should be cancelled, but the current one kept + await _stripeFacade.Received(1) + .CancelSubscription(otherSubscriptionId, Arg.Any()); + await _stripeFacade.DidNotReceive() + .CancelSubscription(currentSubscriptionId, Arg.Any()); + await _userService.Received(1) + .EnablePremiumAsync(userId, currentPeriodEnd); + } + public static IEnumerable GetNonActiveSubscriptions() { return new List