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) {