Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(medusa-payment-stripe): serialization error #2948

Closed
aamdmn opened this issue Jan 5, 2023 · 14 comments
Closed

(medusa-payment-stripe): serialization error #2948

aamdmn opened this issue Jan 5, 2023 · 14 comments

Comments

@aamdmn
Copy link

aamdmn commented Jan 5, 2023

Bug report

Describe the bug

In #2601, there is an error when the Stripe webhook is configured related to concurrent requests trying to complete the cart. It seems the error still occurs in the latest version.

System information

Medusa version (including plugins):

  • medusa: ^1.7.2
  • medusa-payment-stripe: 1.1.50

Node.js version: v16.18.0
Database: PSQL v15.1
Operating system: Windows 11

Steps to reproduce the behaviour

  1. Start the local Stripe webhook listener in the console
  2. Open the nextjs-starter storefront in the browser
  3. Procced thru the checkout flow
  4. See the error in the server console.

Expected behaviour

After checkout proceed to the order confirmation page.

Code snippets

::1 - - [05/Jan/2023:13:57:22 +0000] "POST /stripe/hooks HTTP/1.1" 204 - "-" "Stripe/1.0 (+https://stripe.com/docs/webhooks)"
::1 - - [05/Jan/2023:13:57:23 +0000] "POST /stripe/hooks HTTP/1.1" 200 2 "-" "Stripe/1.0 (+https://stripe.com/docs/webhooks)"
error:   could not serialize access due to concurrent update
Error: could not serialize access due to concurrent update
    at formatException (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\@medusajs+medusa@1.7.2_ezh3pqdy2lsllwffreyozeu6ja\node_modules\@medusajs\medusa\dist\utils\exception-formatter.js:26:20)
    at exports.default (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\@medusajs+medusa@1.7.2_ezh3pqdy2lsllwffreyozeu6ja\node_modules\@medusajs\medusa\dist\api\middlewares\error-handler.js:14:43)
    at Layer.handle_error (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\layer.js:71:5)
    at trim_prefix (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:326:13)
    at C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:286:9
    at Function.process_params (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:346:12)
    at next (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:280:10)
    at C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:646:15
    at next (C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:265:14)
    at C:\Users\adam\www\qer-medusa\medusa-contentful\node_modules\.pnpm\express@4.18.2\node_modules\express\lib\router\index.js:646:15 {
  type: 'conflict',
  code: undefined,
  date: 2023-01-05T13:57:23.411Z
}

Additional context

I downloaded the older version when it was solved in 1.6.5 (medusa-payment-stripe: v1.1.48) and it worked without any problems.

@olivermrbl
Copy link
Contributor

Thanks for filing the issue. We'll make sure to attend to this in the coming week :) cc @adrien2p

@adrien2p
Copy link
Member

I can see that there are two hooks that hit your server. Can you confirm that multiple stripe providers are available in the cart region? I believe that this behavior should not happen again with the latest improvements to the cart payment session management flow. Could I get you to test the next release when it is out?

cc @olivermrbl

@GamfaleanuVlad
Copy link

I can see that there are two hooks that hit your server. Can you confirm that multiple stripe providers are available in the cart region? I believe that this behavior should not happen again with the latest improvements to the cart payment session management flow. Could I get you to test the next release when it is out?

cc @olivermrbl

Hi, I seem to have stumbled upon this issue. I have only one payment provider in the region (only Stripe), and only one webhook, but this hook has multiple actions linked to it (I tried to add all of them :)) ). Once I disabled the webhook, this issue disappeared, also when I set only one action on the webhook.

Versions:
"medusa-payment-stripe": "^1.1.50",
"@medusajs/medusa": "^1.7.3",

@quiloos39
Copy link

Experiencing the same issue

For us, we also enabled every option available in webhooks. What are the implications if we were to disable the webhooks ?

@olivermrbl
Copy link
Contributor

@GamfaleanuVlad @quiloos39 This is expected behavior when requests race to complete a serializable transactional operation.

Correct me if I am wrong, but the cart is, in fact, completed, right? If so, I suggest you add a retry mechanism to your storefront that will idempotently attempt to complete the cart. If you are using our client, retries can be configured in the options when creating it.

Idempotency is supported out of the box. You just need to handle the keys in your storefront. I recommend reading through this guide.

@chemicalkosek I know you've had this issue as well. Feel free to pitch in.

@chemicalkosek
Copy link
Contributor

chemicalkosek commented Jan 24, 2023

Yeah I think this is a race condition when storefront tries to complete the cart at the same time as webhook.
Do not disable webhook in production. I have already had situations where webhook was needed to complete the cart.

I handle it sort of like this on the storefront

1. check if paymentIntent.status === 'succeeded'
2. if true, try to complete the cart
   - success: redirect to order summary.
   - error: webhook might be finishing the order right now. 
        Fetch the order by cartId (you might even add a little setTimeout here to be more sure)
            - success: webhook has created the order -> redirect to order summary using the fetched order
            - error - no order fetched (webhook didn't complete the order yet?)
                Run all of the above back and forth again from the start as a failsafe 

Might not be the prettiest but never failed me in production so I refuse to touch it.

@olivermrbl
Copy link
Contributor

@chemicalkosek I've added similar logic to projects myself, and I think it's a reasonable approach to ensure a good customer experience.

I will close this issue for now, as this is expected behavior.

@chemicalkosek
Copy link
Contributor

chemicalkosek commented Jan 25, 2023

As an addition I have noticed that the default hook doesn't create the order when payment_intent === 'succeeded'
https://github.com/medusajs/medusa/blob/master/packages/medusa-payment-stripe/src/api/routes/hooks/stripe.js#L29
It only fires completing the order on amount_capturable_updated.
That event is not fired on non-card stripe payments like Blik or Przelewy24 because those payments are autocapturable by design. Not sure if it is fired also when capture: true is set in plugin config.

I'm actually using a modified version of webhook where I also complete the cart if no order exists on payment_intent === 'succeeded'

PS. Beware: In default webhook the cart completion takes into account only stripe identifier: https://github.com/medusajs/medusa/blob/master/packages/medusa-payment-stripe/src/api/routes/hooks/stripe.js#L119
Which is ok if you are only using card payments with Stripe Elements or CardElement

But if somebody uses the new Stripe PaymentElement with other payment methods, the amount_capturable_updated might not be fired

@fxmb
Copy link

fxmb commented Aug 30, 2023

@chemicalkosek if I may ask in your stray:

Yeah I think this is a race condition when storefront tries to complete the cart at the same time as webhook. Do not disable webhook in production. I have already had situations where webhook was needed to complete the cart.

I handle it sort of like this on the storefront

1. check if paymentIntent.status === 'succeeded'
2. if true, try to complete the cart
   - success: redirect to order summary.
   - error: webhook might be finishing the order right now. 
        Fetch the order by cartId (you might even add a little setTimeout here to be more sure)
            - success: webhook has created the order -> redirect to order summary using the fetched order
            - error - no order fetched (webhook didn't complete the order yet?)
                Run all of the above back and forth again from the start as a failsafe 

Might not be the prettiest but never failed me in production so I refuse to touch it.

@chemicalkosek in my case the webhook has already completed the cart, hence reset the cart. When I try to fetch the cart from the storefront it simply returns the newly created cart after my checkout has completed. How do you manage to safely try to complete the order, if there is an concurrent update issue (due to webhook completing cart) then simply forward to the order confirmation. The problem is I cannot fetch the order since the cart is already reset 🤷
Based on your answer I assume I am missing something?

@chemicalkosek
Copy link
Contributor

chemicalkosek commented Aug 30, 2023

@fxmb Yes the cart can be reset in the store context beforehand because it has a truthy completed_at.
In my case I do it like this:

I attach the cartId to params when I'm configuring the return_url for Stripe:

        return_url: `${
          process.env.NODE_ENV === 'production'
            ? `https://${hostname}/order-completed?cartId=${cart.id}`
            : `http://localhost:8000/order-completed?cartId=${cart.id}`
        }`,

Then on the return_url page I do like this:

  const { cart: cartFromContext } = useCart();
  const stripe = useStripe();
  const { onPaymentCompleted, resetCart } = useStore();
  const router = useRouter();
  useEffect(() => {
    if (stripe && router.isReady) {
      (async () => {
        
        const { paymentIntent, error } = await stripe.retrievePaymentIntent(
          router.query.payment_intent_client_secret
        );
        if (error) {
          console.error('Stripe retrieve payment error: ', error);
        } else if (paymentIntent && paymentIntent.status === 'succeeded') {
          console.log('webhook should take care of order already');
          // check if order already exists (created in stripe webhook)
          await medusaClient.orders
            .retrieveByCartId(router.query.cartId)
            .then(({ order }) => {
              console.log('webhook took care of order');
              // Cart is reset in store context if it has completed_at truthy
              // Hence we'll skip the reset again if a different cart is in context already
              if (router.query.cartId === cartFromContext?.id) {
                resetCart();
              }
              router.push({
                pathname: '/order-summary',
                query: { orderId: order.id },
              });
            })
            .catch((err) => {
              console.error(err);
              // complete the cart if webhook haven't done that yet.
              console.log('no order yet. frontend is creating order');
              onPaymentCompleted();
            });
        } else {
          showNotification({
            title: 'Error',
            autoClose: false,
            message: 'Payment failed. Try again.',
          });
          router.push('/checkout');
        }
      })();
    }
  }, [router, onPaymentCompleted, stripe, resetCart, cartFromContext.id]);
  
    return (
    <div className="mt-4">
      <Loader /> <br />
      We are checking your payment.
      <br />
      Don't leave this page
    </div>
  );

Does that help?

@fxmb
Copy link

fxmb commented Aug 30, 2023

Thank you so much @chemicalkosek ! Makes a lot of sense. I thought about attaching the cart.id to the return_url but wasnt sure if that is a recommended pattern. Thanks!

@chemicalkosek
Copy link
Contributor

chemicalkosek commented Aug 30, 2023

@fxmb The cartId will be also at payment_intent.metadata.cart_id when fetched from Stripe. And the paymentIntent Id and client_secret are always automatically atached to the return_url by Stripe.

So you can get it also from here.

Edit: Sorry, metadata is not available when using publishable api key.

@fxmb
Copy link

fxmb commented Aug 30, 2023

@chemicalkosek huge shoutout! This works like a charm!

@chemicalkosek
Copy link
Contributor

chemicalkosek commented Aug 30, 2023

@fxmb While reading my code I see that this check

  } else if (paymentIntent && paymentIntent.status === 'succeeded') {

Should actually be:

} else if (paymentIntent && 
(paymentIntent.status === 'succeeded' ||  paymentIntent.status === "requires_capture")) {

or something like that. In my case I have autocapture on Stripe side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants