From 85907befd3c58636ad34e18b9adf4cdad931c33d Mon Sep 17 00:00:00 2001 From: elizabeth-ilina Date: Mon, 8 Jun 2026 12:55:01 -0400 Subject: [PATCH] fix(payments-next): PaidInvoiceOnFailedCartError: Paid invoice found on failed cart Because: * When a customer checks out with PayPal in tab 1 and then sends a second request from tab 2 while the first cart is almost done processing, they end up charged by the first tab while the second tab's stale cart version throws and transitions the cart into FAIL for both tabs. The customer sees the error page, tries to subscribe again, and finds they are already subscribed to the product. This commit: * Checks if the cart is already processing, and, if it is, prevents a second request from failing a cart that another request is actively processing, so the owning request finishes rather than both tabs erroring out. Closes #[PAY-3664](https://mozilla-hub.atlassian.net/browse/PAY-3664) --- libs/payments/cart/src/lib/cart.error.ts | 7 + .../cart/src/lib/cart.manager.in.spec.ts | 45 +++++ libs/payments/cart/src/lib/cart.repository.ts | 15 ++ .../cart/src/lib/cart.service.spec.ts | 186 +++++++++++++++++- libs/payments/cart/src/lib/cart.service.ts | 121 ++++++++---- .../payments/cart/src/lib/checkout.service.ts | 4 + 6 files changed, 337 insertions(+), 41 deletions(-) diff --git a/libs/payments/cart/src/lib/cart.error.ts b/libs/payments/cart/src/lib/cart.error.ts index 3332bd325c0..e59a3e2567b 100644 --- a/libs/payments/cart/src/lib/cart.error.ts +++ b/libs/payments/cart/src/lib/cart.error.ts @@ -190,6 +190,13 @@ export class CartProcessingConflictError extends CartError { } } +export class ConcurrentCartCheckoutError extends CartError { + constructor(cartId: string, cause?: Error) { + super('Concurrent checkout detected for cart', { cartId }, cause); + this.name = 'ConcurrentCartCheckoutError'; + } +} + export class CartStateProcessingError extends CartError { constructor(message: string, cartId: string, cause: Error) { super(message, { cartId }, cause); diff --git a/libs/payments/cart/src/lib/cart.manager.in.spec.ts b/libs/payments/cart/src/lib/cart.manager.in.spec.ts index f5e771a0bc2..16434cf1ca9 100644 --- a/libs/payments/cart/src/lib/cart.manager.in.spec.ts +++ b/libs/payments/cart/src/lib/cart.manager.in.spec.ts @@ -28,6 +28,7 @@ import { UpdateProcessingCartFactory, } from './cart.factories'; import { CartManager } from './cart.manager'; +import { setCartProcessing } from './cart.repository'; import { ResultCart } from './cart.types'; import { type StatsD } from '@fxa/shared/metrics/statsd'; import { LoggerService } from '@nestjs/common'; @@ -378,6 +379,8 @@ describe('CartManager', () => { const STALE_PROCESSING_UID = '77777777777777777777777777777777'; const CONCURRENT_UID = '88888888888888888888888888888888'; const TERMINAL_SIBLING_UID = '99999999999999999999999999999999'; + const SELF_CONCURRENT_UID = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'; + const SELF_PROCESSING_UID = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; async function seedAccount(db: AccountDatabase, uidHex: string) { await db @@ -556,6 +559,48 @@ describe('CartManager', () => { ).length; expect(processingCount).toEqual(1); }); + + it('fails when the same cart is already processing', async () => { + await seedAccount(db, SELF_PROCESSING_UID); + const cart = await cartManager.createCart( + SetupCartFactory({ uid: SELF_PROCESSING_UID }) + ); + + await directUpdate( + db, + { state: CartState.PROCESSING, updatedAt: Date.now() }, + cart.id + ); + + await expect( + setCartProcessing( + db, + Buffer.from(cart.id, 'hex'), + Buffer.from(SELF_PROCESSING_UID, 'hex'), + cart.version + ) + ).rejects.toBeInstanceOf(CartProcessingConflictError); + }); + + it('allows only one of two concurrent checkouts for the same cart to enter processing', async () => { + await seedAccount(db, SELF_CONCURRENT_UID); + const cart = await cartManager.createCart( + SetupCartFactory({ uid: SELF_CONCURRENT_UID }) + ); + + const results = await Promise.allSettled([ + cartManager.setProcessingCart(cart.id), + cartManager.setProcessingCart(cart.id), + ]); + + const fulfillments = results.filter((r) => r.status === 'fulfilled'); + const rejections = results.filter((r) => r.status === 'rejected'); + expect(fulfillments).toHaveLength(1); + expect(rejections).toHaveLength(1); + + const updated = await cartManager.fetchCartById(cart.id); + expect(updated.state).toEqual(CartState.PROCESSING); + }); }); describe('finishErrorCart', () => { diff --git a/libs/payments/cart/src/lib/cart.repository.ts b/libs/payments/cart/src/lib/cart.repository.ts index 5da184560f5..fd4841ddbe6 100644 --- a/libs/payments/cart/src/lib/cart.repository.ts +++ b/libs/payments/cart/src/lib/cart.repository.ts @@ -124,6 +124,21 @@ export async function setCartProcessing( conflictingCart.id.toString('hex') ); } + + const cartIsProcessing = carts.find( + (cart) => + cart.id.equals(cartId) && + cart.state === CartState.PROCESSING && + cart.updatedAt > now - CART_PROCESSING_STALE_TIMEOUT_MS + ); + + if (cartIsProcessing) { + throw new CartProcessingConflictError( + cartId.toString('hex'), + uid.toString('hex'), + cartId.toString('hex') + ); + } } const updatedRows = await trx diff --git a/libs/payments/cart/src/lib/cart.service.spec.ts b/libs/payments/cart/src/lib/cart.service.spec.ts index 3d610039a22..10b051d60a2 100644 --- a/libs/payments/cart/src/lib/cart.service.spec.ts +++ b/libs/payments/cart/src/lib/cart.service.spec.ts @@ -110,7 +110,11 @@ import { CartSetupInvalidPromoCodeError, CartRestartInvalidPromoCodeError, SetupCartAccountNotFoundError, + ConcurrentCartCheckoutError, + CartProcessingConflictError, + UpdatePayPalProcessingCartError, } from './cart.error'; +import { CART_PROCESSING_STALE_TIMEOUT_MS } from './cart.repository'; import { CurrencyManager } from '@fxa/payments/currency'; import { LocationConfig, @@ -1400,9 +1404,10 @@ describe('CartService', () => { const mockRequestArgs = CommonMetricsFactory(); it('accepts payment with Paypal', async () => { - const mockCart = ResultCartFactory(); + const mockCart = ResultCartFactory({ state: CartState.START }); const mockToken = faker.string.uuid(); + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); jest .spyOn(cartManager, 'fetchAndValidateCartVersion') .mockResolvedValue(mockCart); @@ -1430,9 +1435,10 @@ describe('CartService', () => { }); it('reject with CartStateProcessingError if cart could not be set to processing', async () => { - const mockCart = ResultCartFactory(); + const mockCart = ResultCartFactory({ state: CartState.START }); const mockToken = faker.string.uuid(); + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); jest .spyOn(cartManager, 'fetchAndValidateCartVersion') .mockRejectedValue(new Error('test')); @@ -1453,6 +1459,137 @@ describe('CartService', () => { expect(checkoutService.payWithPaypal).not.toHaveBeenCalled(); expect(cartManager.finishErrorCart).toHaveBeenCalled(); }); + + it('rejects with ConcurrentCartCheckoutError when the cart is already actively processing', async () => { + const mockCart = ResultCartFactory({ + state: CartState.PROCESSING, + updatedAt: Date.now(), + }); + const mockToken = faker.string.uuid(); + + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + const setProcessingSpy = jest + .spyOn(cartManager, 'setProcessingCart') + .mockResolvedValue(); + jest.spyOn(checkoutService, 'payWithPaypal').mockResolvedValue(); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await expect( + cartService.checkoutCartWithPaypal( + mockCart.id, + mockCart.version, + mockAttributionData, + mockRequestArgs, + mockCart.uid, + mockToken + ) + ).rejects.toBeInstanceOf(ConcurrentCartCheckoutError); + + expect(setProcessingSpy).not.toHaveBeenCalled(); + expect(checkoutService.payWithPaypal).not.toHaveBeenCalled(); + expect(finishErrorSpy).not.toHaveBeenCalled(); + }); + + it('rejects with ConcurrentCartCheckoutError without finalizing when the claim hits a CartProcessingConflictError', async () => { + const mockCart = ResultCartFactory({ state: CartState.START }); + const mockToken = faker.string.uuid(); + + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + jest + .spyOn(cartManager, 'fetchAndValidateCartVersion') + .mockResolvedValue(mockCart); + jest + .spyOn(cartManager, 'setProcessingCart') + .mockRejectedValue( + new CartProcessingConflictError( + mockCart.id, + mockCart.uid ?? faker.string.uuid(), + faker.string.uuid() + ) + ); + jest.spyOn(checkoutService, 'payWithPaypal').mockResolvedValue(); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await expect( + cartService.checkoutCartWithPaypal( + mockCart.id, + mockCart.version, + mockAttributionData, + mockRequestArgs, + mockCart.uid, + mockToken + ) + ).rejects.toBeInstanceOf(ConcurrentCartCheckoutError); + + expect(finishErrorSpy).not.toHaveBeenCalled(); + }); + + it('rejects with ConcurrentCartCheckoutError without finalizing when the claim fails but the cart is still processing', async () => { + const startCart = ResultCartFactory({ state: CartState.START }); + const processingCart = ResultCartFactory({ + id: startCart.id, + state: CartState.PROCESSING, + updatedAt: Date.now(), + }); + const mockToken = faker.string.uuid(); + + jest + .spyOn(cartManager, 'fetchCartById') + .mockResolvedValueOnce(startCart) + .mockResolvedValue(processingCart); + jest + .spyOn(cartManager, 'fetchAndValidateCartVersion') + .mockRejectedValue(new CartVersionMismatchError(startCart.id)); + jest.spyOn(checkoutService, 'payWithPaypal').mockResolvedValue(); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await expect( + cartService.checkoutCartWithPaypal( + startCart.id, + startCart.version, + mockAttributionData, + mockRequestArgs, + startCart.uid, + mockToken + ) + ).rejects.toBeInstanceOf(ConcurrentCartCheckoutError); + + expect(finishErrorSpy).not.toHaveBeenCalled(); + }); + + it('finalizes with UpdatePayPalProcessingCartError when the claim fails and the cart is not processing', async () => { + const mockCart = ResultCartFactory({ state: CartState.START }); + const mockToken = faker.string.uuid(); + + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + jest + .spyOn(cartManager, 'fetchAndValidateCartVersion') + .mockRejectedValue(new CartVersionMismatchError(mockCart.id)); + jest.spyOn(checkoutService, 'payWithPaypal').mockResolvedValue(); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await expect( + cartService.checkoutCartWithPaypal( + mockCart.id, + mockCart.version, + mockAttributionData, + mockRequestArgs, + mockCart.uid, + mockToken + ) + ).rejects.toBeInstanceOf(UpdatePayPalProcessingCartError); + + expect(checkoutService.payWithPaypal).not.toHaveBeenCalled(); + expect(finishErrorSpy).toHaveBeenCalled(); + }); }); describe('finalizeProcessingCart', () => { @@ -1529,9 +1666,10 @@ describe('CartService', () => { describe('finalizeCartWithError', () => { it('calls cartManager.finishErrorCart', async () => { - const mockCart = ResultCartFactory(); + const mockCart = ResultCartFactory({ state: CartState.START }); const mockErrorCart = FinishErrorCartFactory(); + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); jest.spyOn(cartManager, 'finishErrorCart').mockResolvedValue(); await cartService.finalizeCartWithError( @@ -1544,6 +1682,48 @@ describe('CartService', () => { }); }); + it('does not finalize a cart that is actively processing', async () => { + const mockCart = ResultCartFactory({ + state: CartState.PROCESSING, + updatedAt: Date.now(), + }); + const mockErrorCart = FinishErrorCartFactory(); + + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await cartService.finalizeCartWithError( + mockCart.id, + mockErrorCart.errorReasonId + ); + + expect(finishErrorSpy).not.toHaveBeenCalled(); + }); + + it('finalizes a cart whose processing has exceeded the stale timeout', async () => { + const mockCart = ResultCartFactory({ + state: CartState.PROCESSING, + updatedAt: Date.now() - CART_PROCESSING_STALE_TIMEOUT_MS - 1000, + }); + const mockErrorCart = FinishErrorCartFactory(); + + jest.spyOn(cartManager, 'fetchCartById').mockResolvedValue(mockCart); + const finishErrorSpy = jest + .spyOn(cartManager, 'finishErrorCart') + .mockResolvedValue(); + + await cartService.finalizeCartWithError( + mockCart.id, + mockErrorCart.errorReasonId + ); + + expect(finishErrorSpy).toHaveBeenCalledWith(mockCart.id, { + errorReasonId: mockErrorCart.errorReasonId, + }); + }); + it('should swallow error if cart already in fail state', async () => { const mockCart = ResultCartFactory({ state: CartState.FAIL, diff --git a/libs/payments/cart/src/lib/cart.service.ts b/libs/payments/cart/src/lib/cart.service.ts index 7ff1d901bb0..0b052a61e67 100644 --- a/libs/payments/cart/src/lib/cart.service.ts +++ b/libs/payments/cart/src/lib/cart.service.ts @@ -78,6 +78,8 @@ import { InvalidPromoCodeCartError, CartVersionMismatchError, CartInvalidStateForActionError, + CartProcessingConflictError, + ConcurrentCartCheckoutError, GetCartMissingTaxAddressError, GetCartFromPriceMissingError, GetCartCustomerMissingError, @@ -94,6 +96,9 @@ import { CartSubscriptionDeletionFailedError, } from './cart.error'; import { CartManager } from './cart.manager'; +import { + CART_PROCESSING_STALE_TIMEOUT_MS +} from './cart.repository'; import type { CartDTO, FromPrice, @@ -143,6 +148,10 @@ const IGNORED_EXPECTED_ERRORS_STATSD = new Set([ 'CouponErrorInvalidCode', ]); +const isCartActivelyProcessing = (cart: ResultCart): boolean => + cart.state === CartState.PROCESSING && + cart.updatedAt > Date.now() - CART_PROCESSING_STALE_TIMEOUT_MS; + @Injectable() export class CartService { constructor( @@ -224,6 +233,9 @@ export class CartService { errorReasonId, }); + // TODO: uncomment to test, remove after testing + //await new Promise((r) => setTimeout(r, 8_000)); + const cart = await this.cartManager.fetchCartById(cartId); const store = this.asyncLocalStorage.getStore(); const subscriptionId = @@ -376,6 +388,21 @@ export class CartService { } } + private async isOwnedByAnotherCheckout( + cartId: string, + error: unknown + ): Promise { + if (error instanceof CartProcessingConflictError) { + return true; + } + + const current = await this.cartManager + .fetchCartById(cartId) + .catch(() => null); + + return !!current && isCartActivelyProcessing(current); + } + @SanitizeExceptions() async getCoupon(args: { cartId: string; @@ -722,48 +749,59 @@ export class CartService { }, }); }; - return this.wrapWithCartCatch(cartId, { onCheckoutFail }, async () => { - let updatedCart: ResultCart | null = null; - try { - //Ensure that the cart version matches the value passed in from FE - await this.cartManager.fetchAndValidateCartVersion(cartId, version); + return this.wrapWithCartCatch( + cartId, + { onCheckoutFail, errorAllowList: [ConcurrentCartCheckoutError] }, + async () => { + const existing = await this.cartManager.fetchCartById(cartId); + if (isCartActivelyProcessing(existing)) { + throw new ConcurrentCartCheckoutError(cartId); + } - await this.cartManager.setProcessingCart(cartId); + let updatedCart: ResultCart | null = null; + try { + //Ensure that the cart version matches the value passed in from FE + await this.cartManager.fetchAndValidateCartVersion(cartId, version); - // Ensure we have a positive lock on the processing cart - updatedCart = await this.cartManager.fetchAndValidateCartVersion( - cartId, - version + 1 - ); - } catch (error) { - throw new UpdatePayPalProcessingCartError(cartId, error); - } + await this.cartManager.setProcessingCart(cartId); - this.asyncLocalStorage.run( - { checkout: { subscriptionId: undefined } }, - () => { - // Intentionally non-blocking - this.wrapWithCartCatch(cartId, { onCheckoutFail }, async () => { - await this.checkoutService.payWithPaypal( - updatedCart, - attribution, - requestArgs, - sessionUid, - token - ); - }).catch((error) => { - handleException({ - error, - className: 'CartService', - methodName: 'checkoutCartWithPaypal', - allowlist: [], - logger: this.log as Logger, - statsd: this.statsd, - }); - }); + // Ensure we have a positive lock on the processing cart + updatedCart = await this.cartManager.fetchAndValidateCartVersion( + cartId, + version + 1 + ); + } catch (error) { + if (await this.isOwnedByAnotherCheckout(cartId, error)) { + throw new ConcurrentCartCheckoutError(cartId, error); + } + throw new UpdatePayPalProcessingCartError(cartId, error); } - ); - }); + + this.asyncLocalStorage.run( + { checkout: { subscriptionId: undefined } }, + () => { + // Intentionally non-blocking + this.wrapWithCartCatch(cartId, { onCheckoutFail }, async () => { + await this.checkoutService.payWithPaypal( + updatedCart, + attribution, + requestArgs, + sessionUid, + token + ); + }).catch((error) => { + handleException({ + error, + className: 'CartService', + methodName: 'checkoutCartWithPaypal', + allowlist: [], + logger: this.log as Logger, + statsd: this.statsd, + }); + }); + } + ); + }); } @SanitizeExceptions() @@ -816,6 +854,8 @@ export class CartService { /** * Update a cart in the database by ID or with an existing cart reference + * Do not fail a cart that another request is actively processing, since + * we may have already charged the customer. * **Note**: This method is currently a placeholder. The arguments will likely change, and the internal implementation is far from complete. */ @SanitizeExceptions() @@ -823,6 +863,11 @@ export class CartService { cartId: string, errorReasonId: CartErrorReasonId | string ): Promise { + const cart = await this.cartManager.fetchCartById(cartId); + if (isCartActivelyProcessing(cart)) { + return; + } + try { await this.cartManager.finishErrorCart(cartId, { errorReasonId, diff --git a/libs/payments/cart/src/lib/checkout.service.ts b/libs/payments/cart/src/lib/checkout.service.ts index 1cf5a8c3135..fcd64ea8342 100644 --- a/libs/payments/cart/src/lib/checkout.service.ts +++ b/libs/payments/cart/src/lib/checkout.service.ts @@ -352,6 +352,10 @@ export class CheckoutService { }); } + // TODO: uncomment to test, remove after testing + //console.log('postPaySteps: about to finish cart, sleeping, START TAB 2') + //await new Promise((r) => setTimeout(r, 5_000)); + await this.cartManager.finishCart(cart.id, version, {}); if (args.isCancelInterstitialOffer && args.requestArgs) {