search

Adyen / adyen-magento2

Adyen Payment plugin for Magento2
MIT License
155 stars 210 forks source link

Race condition may cause the order status to be reverted to pending #2726

Closed pmzandbergen closed 1 week ago

pmzandbergen commented 3 weeks ago

Description When a Customer is delayed between the PSP and the Magento return page the order status can be reversed from processing, or in fact any possible state (like holded) back to pending.

Cause The Controller for the return page calls the PaymentResponseHandler, which in turn updates the order according to the response.

Code

  1. https://github.com/Adyen/adyen-magento2/blob/365cd45433fd0922c7fbd985a8e732e94ee99376/Controller/Return/Index.php#L98
  2. https://github.com/Adyen/adyen-magento2/blob/365cd45433fd0922c7fbd985a8e732e94ee99376/Controller/Return/Index.php#L164
  3. https://github.com/Adyen/adyen-magento2/blob/365cd45433fd0922c7fbd985a8e732e94ee99376/Helper/PaymentResponseHandler.php#L210

Solution Check the current order status before doing any changes. If the status isn't pending_payment leave the order as-is, since either a previous executing of the controller or the webhook processor has already updated the order.

I also highly recommend to implement locking to prevent race conditions in case the return controller is executed at the same time as the notification processor (Adyen\Payment\Helper\Webhook::processNotification(...)). If no lock is applied (the current situation) those processes may collide. Please note you'll have to place the lock before loading the order, or you'll be using a cached and possible out-of-date model.

Note: Finished my previous LEGO set đŸ˜‡

dimitriBouteille commented 3 weeks ago

Hi @pmzandbergen

Probably same issue https://github.com/Adyen/adyen-magento2/issues/2708

pmzandbergen commented 3 weeks ago

Hi @pmzandbergen

Probably same issue #2708

Indeed, that issue should also be fixed if the suggestion above has been implemented. Patch as quick-fix (without locking as suggested in my original message):

--- Helper/PaymentResponseHandler.php.org   2024-08-29 13:33:51
+++ Helper/PaymentResponseHandler.php   2024-08-29 13:36:52
@@ -203,8 +203,10 @@
              * if no additional action is required according to /paymentsDetails response.
              * Otherwise keep the order state as pending_payment.
              */
-            $order = $this->orderHelper->setStatusOrderCreation($order);
-            $this->orderRepository->save($order);
+            if ($order->getState() === \Magento\Sales\Model\Order::STATE_PENDING_PAYMENT) {
+                $order = $this->orderHelper->setStatusOrderCreation($order);
+                $this->orderRepository->save($order);
+            }
         }

         // Cleanup state data if exists.
@@ -238,8 +240,10 @@
             case self::PENDING:
                 /* Change order state from pending_payment to new and expect authorisation webhook
                  * if no additional action is required according to /paymentDetails response. */
-                $order = $this->orderHelper->setStatusOrderCreation($order);
-                $this->orderRepository->save($order);
+                if ($order->getState() === \Magento\Sales\Model\Order::STATE_PENDING_PAYMENT) {
+                    $order = $this->orderHelper->setStatusOrderCreation($order);
+                    $this->orderRepository->save($order);
+                }

                 // do nothing wait for the notification
                 if (strpos((string) $paymentMethod, "bankTransfer") !== false) {
kpitn commented 2 weeks ago

We have the same problem sometimes order in the workflow are not the same :

Wrong workflow

Sep 2, 2024 6:34:18 PM Pending Adyen paymentsDetails response: authResult: Authorised pspReference: XXXXXXXXXXXXXX paymentMethod: mc Sep 2, 2024 6:34:18 PM Processing Adyen Payment Successfully completed Sep 2, 2024 6:34:18 PM Processing AUTHORISATION webhook notification w/amount EUR XXX was processed Sep 2, 2024 6:34:18 PM Pending Adyen HTTP Notification(s): eventCode: AUTHORISATION pspReference: XXXXXXXXXXXXXX paymentMethod: mc success: true reason:152400:9559:07/2026 Sep 2, 2024 6:33:33 PM Pending Payment ThreeDS2 action is required to complete the payment. Result code: ChallengeShopper Sep 2, 2024 6:33:33 PM Pending Sep 2, 2024 6:33:33 PM Adyen Result response: authResult: ChallengeShopper

Correct workflow

Sep 3, 2024 6:59:53 AM Processing Adyen Payment Successfully completed Sep 3, 2024 6:59:53 AM Processing AUTHORISATION webhook notification w/amount EUR XXX was processed Sep 3, 2024 6:59:53 AM Pending Adyen HTTP Notification(s): eventCode: AUTHORISATION pspReference: XXXXXXXXXXXXXX paymentMethod: mc success: true reason:588273:4777:06/2029 Sep 3, 2024 6:59:53 AM Pending Adyen paymentsDetails response: authResult: Authorised pspReference: XXXXXXXXXXXXXX paymentMethod: mc Sep 3, 2024 6:59:28 AM Pending Payment ThreeDS2 action is required to complete the payment. Result code: ChallengeShopper Sep 3, 2024 6:59:28 AM Pending Sep 3, 2024 6:59:28 AM Adyen Result response: authResult: ChallengeShopper

pmzandbergen commented 2 weeks ago

@kpitn the quick-fix in my previous comment will fix this issue.

Note that this issue is also easy to reproduce:

  1. Place an order in the webshop using a payment method which is using an external payment page
  2. Finish your payment, but do not return to the webshop yet
  3. Wait a few minutes for the notifications to be processed (the order will proceed to "processing")
  4. Click the "Return to webshop" button and the order is back to "pending"
candemiralp commented 2 weeks ago

Hello @pmzandbergen,

Thank you for raising this question and providing a fix for it. This issue duplicates #2708 where @dimitriBouteille has already created a PR. The fix will be released soon.

Best Regards, Can

pmzandbergen commented 2 weeks ago

@candemiralp will you also implement locking, to prevent race conditions when the notification is being processed at the same time as the customer is returning to the webshop? This can occur with the current changes in #2709.

sidalto commented 2 weeks ago

@candemiralp We are facing an issue that after the order is placed (Brazilian PIX payment method), we are receiving a payment canceled response, but soon after the order paid notification is received. In orders where this issue occur, we observed that the webhook response changing the order status is being processed before the authorization response.

Wrong workflow:

imagem1

Correct workflow:

imagem3

pmzandbergen commented 1 week ago

@candemiralp will you also implement locking, to prevent race conditions when the notification is being processed at the same time as the customer is returning to the webshop? This can occur with the current changes in #2709.

Should we open another issue for this case?

candemiralp commented 1 week ago

Hello @pmzandbergen,

Thank you for raising your concerns about the fix. I will test it and get back to you.

Best Regards, Can