Skip to content

[PM-33289] Stop 500-retry loop on incomplete_expired subs#7525

Merged
amorask-bitwarden merged 2 commits intomainfrom
billing/PM-33289/fix-incomplete-expired-retry-loop
May 7, 2026
Merged

[PM-33289] Stop 500-retry loop on incomplete_expired subs#7525
amorask-bitwarden merged 2 commits intomainfrom
billing/PM-33289/fix-incomplete-expired-retry-loop

Conversation

@amorask-bitwarden
Copy link
Copy Markdown
Contributor

@amorask-bitwarden amorask-bitwarden commented Apr 22, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-33289

📔 Objective

Fixes a webhook processing bug where personal Premium subscriptions were being silently disabled and staying disabled despite Support's manual re-enables. Confirmed in the field against five affected customers, with Stripe delivery logs showing up to five automatic disables per customer over three days.

Root cause. SubscriptionUpdatedHandler.HandleAsync combined the SubscriptionWentUnpaid and SubscriptionWentIncompleteExpired branches, calling SetSubscriptionToCancelAsync for both. That method invokes stripeAdapter.UpdateSubscriptionAsync(..., CancelAt = now + 7d), which Stripe rejects on a subscription that is already incomplete_expired (a terminal state). The resulting StripeException propagates out of the webhook controller as HTTP 500; Stripe retries 5xx responses for up to 72 hours, and every retry re-executes DisablePremiumAsync(userId) on the customer — silently flipping user.Premium = false after Support had flipped it back true.

Primary fix. Split the branches so the IncompleteExpired path only disables the subscriber and does not attempt a Stripe API update on the already-terminal subscription. Verified PriceIncreaseScheduler.Release is a no-op on terminal subs (it filters for Active schedules only), so skipping the whole call is safe. Inverted three existing tests that were encoding the buggy behavior (asserting UpdateSubscriptionAsync was called on IncompleteExpired) to now assert DidNotReceive().

Scope-expansion fix. While reviewing the handler module, found a latent bug in PaymentSucceededHandler and PaymentFailedHandler: both short-circuited on hardcoded Premium price ID constants ("premium-annually" / "premium-annually-app") that did not match the current premium-annually-2026 Stripe price. Today the bug was masked by CreatePremiumCloudHostedSubscriptionCommand setting user.Premium = true synchronously at signup — but any flow that depends on the webhook to enable Premium (e.g., a customer paying a stuck open invoice via the hosted-invoice page) was silently failing. Replaced the hardcoded constants with a dynamic IPricingClient.ListPremiumPlans() lookup against the Password Manager seat price (aligning with UpcomingInvoiceHandler's convention — storage is an add-on, not identity). Added try/catch around the pricing call, empty-list guards with logged errors, and null-safe plan access; on pricing-service uncertainty both handlers fall back to "not Premium" so we preserve the default behavior (don't enable Premium we can't verify; don't apply the Premium-only early-stop of pay retries we can't confirm applies). Injected ILogger into PaymentFailedHandler (previously had none). Added seven new tests: four for PaymentSucceededHandler (happy path, non-Premium sub, pricing throws, empty plan list) and three for PaymentFailedHandler (happy path, non-Premium sub still attempts, pricing throws falls back to default retries).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Logo
Checkmarx One – Scan Summary & Detailsaf1e4bd7-cb86-4bf5-8894-c643623c13aa

Great job! No new security vulnerabilities introduced in this pull request

@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.21%. Comparing base (dbd0d52) to head (933478b).

Files with missing lines Patch % Lines
...g/Services/Implementations/PaymentFailedHandler.cs 75.00% 5 Missing and 2 partials ⚠️
...ervices/Implementations/PaymentSucceededHandler.cs 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7525      +/-   ##
==========================================
+ Coverage   59.13%   59.21%   +0.07%     
==========================================
  Files        2077     2077              
  Lines       91848    91898      +50     
  Branches     8175     8179       +4     
==========================================
+ Hits        54315    54414      +99     
+ Misses      35601    35540      -61     
- Partials     1932     1944      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amorask-bitwarden amorask-bitwarden marked this pull request as ready for review April 22, 2026 15:22
@amorask-bitwarden amorask-bitwarden requested a review from a team as a code owner April 22, 2026 15:22
@bre-deploy bre-deploy Bot temporarily deployed to EU-QA Cloud May 7, 2026 12:29 Inactive
@amorask-bitwarden amorask-bitwarden merged commit d25553e into main May 7, 2026
46 of 47 checks passed
@amorask-bitwarden amorask-bitwarden deleted the billing/PM-33289/fix-incomplete-expired-retry-loop branch May 7, 2026 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants