The changes made to the order on the onNotify method are not applied on the onReturn method

This problem sometimes arises with commerce_sofortbanking too. We there have the problem, that the order number assignment is called twice and therefore one number is skipped

Reclassifying.

We need a failing test before we start to evaluate fixes.

that the order number assignment is called twice and therefore one number is skipped

That sounds like a double-save, probably not the same as what this issue is describing.
Ultimately the only real solution there will be to introduce order locking so that the second save throws an exception, providing a backtrace that can be used to track it down.

We need to make sure this issue is not a duplicate of #3011667: Saving the order before its payment in PaymentGateway::onReturn() can cause data loss.

#4 is not related to the mentioned issue. The problem only happens once in a couple of weeks in that project. Notable is that the server-to-server notification and the user return normally happen at a very similar time, so I wouldn't wonder, if they sometimes collide at the same second

I've been battling this bug as well. Here is what I've found out. Hopefully it helps.

I created a new offsite paymentgateway plugin that implements SupportsNotificationsInterface
The documentation of the onNotify method in SupportsNotificationsInterface states, that the method should not touch the parent order, only the payment itself

My onNotify method calls the method setCompletedTime on the Payment entity and saves it
Since I save the payment object, the postSave method on the Payment entity is invoked
The postSave method on the Payment entity will call the updateTotalPaid method in the service PaymentOrderManager
The method updateTotalPaid in the service PaymentOrderManager will call the method setTotalPaid on the Order entity and then saves it
Saving the Order entity will invoke the doOrderPreSave method in OrderStorage
Since $order->isPaid() now returns true, the ORDER_PAID event is being dispatched
This invokes the onPaid method in the eventsubscriber OrderPaidSubscriber
This method applies a transition to the order so the order has now been placed

After this, the customer is sent to the return page of the checkout flow
This is handled by the returnPage method in the controller PaymentCheckoutController
The returnPage method calls the method validateStepId
The validateStepId method calls the method getChecoutStepId in the service CheckoutOrderManager
Since the state of the order changed in the notify process, the state no longer equals "draft" and the method getCheckoutStepId will return "complete" no matter the actual value in the database
Because of this the validateStepId will throw a NeedsRedirectException and redirect the user to the complete step, without executing the onReturn method of my paymentgateway plugin, nor updating the checkout_step value on the order entity.

Since the onNotify callback happens asynchronously with my payment gateway provider, it can actually happen that the onReturn is executed first, although it is uncommon in my case. When this happens, the customer gets two confirmation emails, since the order is placed twice.

I made a temporary fix by skipping this validation. I would like to stress, that I don't think this is a good solution, but in my particular case, I think it is the lesser of two evils, compared to leaving it as is.

#3011667: Saving the order before its payment in PaymentGateway::onReturn() can cause data loss has now landed, which should help us identify the remaining problems.

Please test with -dev from this point on.

Subscribing to this because issue #3085805: Two place transition events fired when Mollie is assigned to this issue.

Been discussing this with @jacksick a while ago. Starting to experimenting with some things.

There is now an optimistic locking, so instead of multiple events or race conditions that _should_ instead throw an exception if enabled, or at least log about it, but that's not a solution.

The only thing I can think of right now is to introduce a new explicit lock API that allows code that loads, changes and (more or less) immediately resaves orders to lock them explicitly using a shared API/lock. Unfortunately, the notification callback design requires that gateway plugins that use that use this new method.

This is only a proof of concept patch, will start testing that in combination with commerce_saferpay and a patch for that.

I don't know how to write functional test coverage for this yet, as the implementations will be gateway specific. We can probably test the API by directly locking in the test and then triggering a non-locked save.

The problem with the non-locked save aka non-adjusted code or things like a save in the UI is that at that point, we don't know what fields changed, not without some core issues for that, so we can't recover from that and have to abort. So at best we can fall back to the optimistic locking behavior I guess.

That said, in my experience, the most common scenario for such conflicts is between between the notify and return methods of a gateway as well as the somewhat counter-productive terminate behavior of PaymentOrderUpdater, and those cases can be handled, given the gateway plugins are adjusted for this. Someone also editing the order in the UI at the same time seems far less likely and is something that could be just denied with an exception that is properly handled.

I am also aware of the issues discovered by #3118960: Locking for ensuring unique order numbers is not locked enough but I did start out with a regular lock backend for this as a proof of concept at lesat, easier to implement. I suspect that other issue is a transaction problem when starting locks within a transaction. That's not really happening here.

When are you actually updating the updateLocks array? Isn't that missing?

Also, any reason not to inject the entity type manager in PaymentCheckoutController?

yes, I forgot that.

And non-injection is just lazyness on my side, also the missing BC. This is completely untested as of now, I'll finalize it when it works.

don't know, if it helps, but I've also played with that in my commerce_opp module, doing explicit locks and release, like here https://git.drupalcode.org/project/commerce_opp/-/blob/8.x-1.x/src/Plugi...

The implementation for commerce_saferpay is here: #3223550: Implement order locking to prevent race conditions with async payment notification.. It does open some new question marks, though. Specifically commerce_saferpay does not save the order explicitly, it creates a payment and that saves the currently loaded order then implicitly. But there's also a case where the API calls might fail and it would then actually not save anything. The lock will be freed up at the end of the request, but should we also have an explicit lock release on OrderStorage?

First testings show that the lock does work, but I forgot the wait() implementation, right now it just died immediately, I forgot about that part. Note that wait() is the one that does then a loop and re-checks in increasing intervals, so it's kinda OK to just let it wait for the max 30s, it will return as soon as possible (more or less). A lock taking more than 30s seems extremely unlikely, what is possible in theory is that another process already steals the lock again between the wait() and acquire() calls. However, that would imply at least 3 different requests all fighting for the lock and that doesn't seem to be a common scenario?

Wow, that required a lot of debugging and logging. The locking started to work, but PaymentCheckoutController, despite waiting for the lock, then still attempted to use the old order. The problem was that the checkout flow plugin gets the order it uses from the route match and it isn't explicitly injected. for extra confusion, $route_match in the controller was not the same object as $route_match in the plugin, so I have to set it explicitly on the current route match object.

I think the checkout flow plugin should have an explicit method to set the order object and it should be passed explicitly when creating it.

I was also still missing some updateLocks updates, didn't actually set it, so the lock didn't explicitly get freed.

And by a lot of logging, I mean this:

What you can see there is 3 loadForUpdate() attempts from 3 requests. First the notify callback immediately gets the lock, then the return controller has to wait, then it gets the lock, then the terminate method again has to wait for the return callback to finish before being able to update the paid amount. In my tests, the notify and return request was triggered within ~0.1s, so a save race condition happened almost always.

Still no tests and more work on DI and BC and the preSave protection (although we could ignore that and just let the version check do its thing, only difference is that you might be able to acquire a lock and *still* fail to save), but from my testing, it seems to work. No more duplicate mails and total paid is set correctly. Note that your checkout gateway must use this new method in the notify callback. or it will likely still fail.

Rerolled the patch to apply to the current 8.x-2.x-dev branch, for example the constructor in OrderStorage has been removed in the meantime.
Added some function documentation too.

I can confirm that the patch in #23 fixes the race condition problems I encountered when using the offsite payment provider Mollie.
(as originally reported in https://www.drupal.org/project/mollie/issues/3232842#comment-14332987)

Applying this patch fixes the "place" transition being performed twice, and order balance not being correctly updated.

Thanks for the feedback. Depending on what the mollie module does exactly, it might still be useful to have a patch there as well if it saves orders on its own for example in an on notify callback, as there might still be potential for a race condition, just possibly less likely. If it only has onReturn then that might not be needed.

Thanks for your work @Berdir, and your testing @svendecabooter!

Using patch #23 in production successfully for a month in combination with Mollie payment provider.

No more outdated orders so far. No more double order confirmation mails. (and in my custom case no more double invoices being created since I now use the loadForUpdate() method) Before the patch, I had double invoices due to racing conditions at least once a week, so I am quite certain it is fixed now.

The Mollie return controller uses the "commerce_payment.checkout.return" route, which is part of patch #23.
The Mollie onNotify method loads the payment by remote payment id, alters and saves it. There is no direct order loading in the onNotify method.
So no need to patch commerce_mollie.

Edit: I use the commerce_mollie module, not mollie (used by Sven)

If we go with the patch from #23, we need to introduce an OrderStorageInterface and document the new loadForUpdate() method there.

Just a few notes:

1. Using the Drupal locking API failed for the order number generation, so I have some reservations with that.
2. Using a static cache of locked orders works well only if we're attempting to load the order from within the same request.

Also, can we really not use dependency injection in PaymentCheckoutController? I see there's a comment for the route match, but about the entity type manager at least?

I concur with @jsacksick re Drupal locking API concerns, being as I was strongly affected by the order number generation problem in 3118960. The approach used in that issue was to use a SELECT ... FOR UPDATE query to retrieve the current sequence number and then perform the update, all wrapped in a transaction. A similar approach could be used here by implementing the loadForUpdate to perform a suitable SELECT ... FOR UPDATE query before calling loadUnchanged, maybe by using buildQuery to get a suitable query object.

I'm not sure if the loadForUpdate call in PaymentCheckoutController is necessarily the right place for guaranteeing getting the up-to-date order in the checkout flow plugin - wouldn't it be preferable to do this directly in CheckoutFlowBase? I'm currently carrying a patch which is half way to doing this in CheckoutFlowBase::redirectToStep:

  /**
   * {@inheritdoc}
   */
  public function redirectToStep($step_id) {
    if (!$this->isStepVisible($step_id)) {
      throw new \InvalidArgumentException(sprintf('Invalid step ID "%s" passed to redirectToStep().', $step_id));
    }

+    // If we're completing an order, forcibly reload it first as an offsite
+    // payment gateway notification may have already placed the order
+    // asynchronously since the order was originally loaded by the route
+    // matcher
+    if ($step_id == 'complete') {
+      $this->entityTypeManager->getStorage('commerce_order')->resetCache([$this->order->id()]);
+      $this->order = $this->entityTypeManager->getStorage('commerce_order')->load($this->order->id());
+    }

    $this->order->set('checkout_step', $step_id);
    $this->onStepChange($step_id);
    $this->order->save();

    throw new NeedsRedirectException(Url::fromRoute('commerce_checkout.form', [
      'commerce_order' => $this->order->id(),
      'step' => $step_id,
    ])->toString());
  }

A transaction and a call to loadForUpdate could be used here to ensure synchronisation.

I've never had problems with transactions as you described in that other issue, but one thing I suspected is that you already were inside a transaction in that case, which then might behave differently.

I don't have the full situation in my mind currently, but I'm fairly sure the load for update calls need to be exactly where they are. putting it back into the route match then ensures that the checkout plugin gets it from the same place, but note that there are chances in multiple places, the checkout plugin might make them, but also the controller does its own update logic to set the checkout step.

@Berdir I see what you mean about the update calls placement - I think the problem is that both payment plugin onReturn method and the checkout flow plugin both have chances to modify the order object, before the checkout flow plugin ultimately saves it. Normally they would implicitly be playing with the same object as the first load of the order ID would create a PHP object in the storage's static cache and all subsequent loads would reference the same php object from the static cache. Reloading the order results in creation of a new PHP object, so we have this problem of stale references. I think a better solution for the checkout plugin would be to add a setOrder function to CheckoutFlowInterface to explicitly ensure the plugin has the correct order reference before calling redirectToStep. A stale reference in the route match could potentially still be a problem for any 3rd party code using the route match in the return request, but such usage is probably a bug (is there any legit use case for using the route match in preference to an event subscriber here?)

Re locking mechanisms, the problem experienced on the order number pattern issue was that Drupal locking API pattern was broken by Mysql being able to optimise away the lock table INSERTs and DELETEs such that a competing process was able to acquire the lock and read a stale sequence value before the original UPDATE of the of the sequence value was completed. My suspicion is that the Mysql query cache plays a roll in being able to obtain the stale value when it is enabled (which I suspect it shouldn't be for most people these days), but the real takeaway is that the Drupal locking API is not robust enough by design for controlling access to data from competing threads / processes executing in parallel, which is exactly the problem in this case (either customer return vs gateway notification, or double request of a customer return which I've seen happen on several occasions). So database row level locking with the SELECT FOR UPDATE + transaction pattern is preferrable to the Drupal lock API approach currently implemented in #23 to ensure the fix is robust enough to work in all circumstances.

For me the patch sometimes solves the issue, but then sometimes it throws:

Drupal\commerce_order\Exception\OrderVersionMismatchException: Attempted to save order 380475 with version 8. Current version is 9. in Drupal\commerce_order\Entity\Order->preSave() (line 660 of /code/web/modules/contrib/commerce/modules/order/src/Entity/Order.php).

This happens on the payment return path (/checkout/{order}/payment/return), where the browser gets redirected. There is a parallel thread where Touchnet payment gateway is sending a webhook to /touchnet to record the payment object. Debugging is tough because a) it's a race condition and b) there isn't a way to test this locally.

I have order status manipulation logic in the pre-transition event for order-placed, but the patch has made it so that only happens once and in the case of the error I'm reporting, the order-placed procedure happened on the webhook thread (i.e. different thread from the error).

There is nothing in my onReturn method that should be triggering an order save, either.

I think the big question is what order save operation could be causing the error I'm seeing? If we can narrow that down, maybe there needs to be another loadForUpdate put into play.

I encountered the double invoice-email in both commerce_mollie as in the Mollie for Drupal modules. I switched from the first to the last in the hopes to solve it but it didn't work.

I tried patch #23 with Mollie for Drupal and it did not solve my problems.

Adding 3152876 as a related issue as I think a lot of the onNotify vs onReturn race problems could be made to go away entirely if this meta refresh idea were to be implemented.

I am also facing this issue with commerce_wallee and created an issue for this: #3258561: Race condition in onNotify

Are the changes to onNotify needed even if the payment gateway doesn't have any logic on it's own like this (which is the case for commerce_wallee)?

public function onNotify(Request $request) {
  parent::onNotify($request);
}

@pstewart: Even then, I don't think this problem would go away. For example, there is still the problem that some changs happen in a terminate handler, so even when waiting on the payment/IPN call came in, it's possible that process is still running and the order paid save happens in parallel.

@Lukas: commerce_wallee has it's own callback implementation that does not go through onNotify and will need to implement something based on this patch there. We are starting to use that integration too, get in touch to discuss this.

I have a similar problem with Klarna Payments.

Just for the record: I have reported the same problem related to Commerce Vipps.

Yes, I realize that this is noise for people working on the issue, but probably very useful for people using Commerce Vipps.

JS fail, seems unrelated/random, retrying.

Hit the same issue with significant consequences as we use the place transition to create a transaction for an external accounting system when the income for an order is accrued...

I'm very happy to give some time to push this forward, but I'm not quite clear what the next steps are. From looking at the latest patch (#23) and subsequent comments, it looks like:

1. Most significantly we need to decide how to handle an explicit lock from another process in OrderStorage::doOrderPreSave
2. We probably want to move towards an interface for order storage
3. Decide if we are happy using the code lock service
4. We want to use dependency injection for PaymentCheckoutController
5. Should be using OrderStorage::loadForUpdate in CheckoutFlowBase::onStepChange
   My opinion is that both places should be doing it
6. Test failure

Conversation with berdir on slack regarding point 5 above:

the route match stuff at the beginning of PaymentCheckoutController ensures that the checkout flow receives a for-update order entity, at least in on that route. imho doing it in the plugin is not correct or even harmful as you might lose changes made to the order object in the meantime (edited)

for one example of that, see \Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\ExpressCheckout::onReturn, explicitly comments on the assumption that the order will be saved with these changes later on. reloading it might break that

I'm also struggling with this issue with Commerce Paybox Payment module.
I've been able to apply patch #23, and it seems to fix this "conflict".

I had a problem with returning after (test)payment with Commerce Mollie module. After Patch #23 the problem was solved.

same problem with race conditions.
subscribe

@APolitsin: So patch #23 doesn't help?

Would be great if we could review this while in Prague... I'm just unclear on whether this fixes the issue for everyone... I really wanna make sure of that before introducing such an important change, and afaik, some of my previous feedbacks are still not addressed.

For me works, apply #23, and in the onNotify() method, after create and validate payment implements:

 $order = $payment->getOrder();
        if ($order->getState()->getId() == 'draft') {
          $order->unlock();
          $order->getState()->applyTransitionById('place');
          $order->save();
        }

Currently facing several issues regarding orders and payments:

- Orders with repeated order numbers
- Orders with skipped order numbers (double save)
- Orders with balance but completed payments

During heavy sales volume (around 3-5 concurrent orders per minutes).

Currently testing #23 during a relatively quiet sales season and so far, no issues from the above listed.

Of course, I am waiting for the next heavy sales season to start so I can check how it behaves. I will share my outcomes.

We had some issues with the checkout:

• Orders with the error message: Drupal\commerce_order\Exception\OrderVersionMismatchException: Attempted to save order X with version 6. Current version is 7.
• Order emails were sent twice.
• Orders with empty total amount.

Patch #23 solved all these problems.

• Core: 9.4.8
• commerce: 8.x-2.32
• commerce_wallee: 8.x-2.x-dev with Patch https://www.drupal.org/project/commerce_wallee/issues/3258561#comment-14...

Regarding #49, so far, no issues of:

- Orders with repeated order numbers
- Orders with skipped order numbers (double save)
- Orders with balance but completed payments

With low trough medium transaction volumes.

What a relief!

Drupal 9.4.8. Custom made payment notification with not onReturn and only onNotify.

Marking this as RBTC, #49 is still working on Drupal Core 9.5.3 + Commerce 8.x-2.33

This still needs work as the comments from comment #23 were left unadressed. A new public method was added to the order storage but it hasn't been added to the interface.

Also can't remember why we didn't use dependency injection in the PaymentCheckoutController and... yeah, not really sure how we can provide a failing test so we can safely commit this....

I've used #23 for 3 weeks without any incidents on production. We had several duplicate orders a day before the path.

Has anyone tested whether the problem can be fixed by setting the database isolation level in settings.php?

database isolation levels only apply when within a transaction. The problem here is that two separate requests load an order entity and then save them. Loading an entity happens outside of the transaction, that's why we add locking here.

Re #53, I guess you meant the comments in #27? I replied to the concerns about locking, there are like a dozen of positive reports here as well to confirm that this works, also in regards to not being able to have a test for this. I think it would be possible to start the lock within the test, and then request a page that attempts to load and save the order which should then fail.

> Using a static cache of locked orders works well only if we're attempting to load the order from within the same request.

The static cache is to check if the current request has the lock, it needs and must only work in the current request.

A bit of DI on the controller is possible, but specifically the route match must not be injected, as documented. And yes, I guess an OrderStorageInterface can be introduced.

I'll see if I can find some time for this.

The interface and DI added.

Those fails are unrelated I think?

Added a test, was quite tricky, the problem is of course that the test controller will wait to update, that's the whole point. So what I did instead is set a short timeout, let the controller wait, in the meantime change and save the order, which causes the lock to be freed up and then the test controller can make it's change.

interdiff is slightly off, forgot to add a few changes to the previous commit.

Ah. I did change the presave method to use the existing throw-or-log setting and tested that as well. I think that makes sense, because if we don't throw an exception here, it will happen for the order that used the lock API.

@Berdir: Thanks for working on the changes, the test failures are indeed unrelated... The product layout builder tests are randomly failing...
I'm wondering under which circumstance the lock could not be acquired and in this case... Should the code expect that? Because right now there is no try catch around the calls to the loadForUpdate() method even though we document that there are cases where we could return NULL.

The return null is modeled after the regular load method, if you pass an invalid order id it will return NULL. The @return doesn't go into details why, but I think load() doesn't either. If the lock can't be acquired, it will fail with an exception, that's documented on the interface.

It could be caught, but there's nothing you can do at that point, it's not possible to proceed. It's also very unlikely I think.

+++ b/modules/payment/src/Controller/PaymentCheckoutController.php
@@ -84,6 +96,18 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
     $order = $route_match->getParameter('commerce_order');
     $step_id = $route_match->getParameter('step');
     $this->validateStepId($step_id, $order);
+
+    // Reload the order and mark it for updating, redirecting to step below
+    // will save it and free the lock. This must be done before the checkout
+    // flow plugin is initiated to make sure that it has the reloaded order
+    // object. Additionally, the checkout flow plugin gets the order from
+    // the route match object, so update the order there as well with. The
+    // passed in route match object is created on-demand in
+    // \Drupal\Core\Controller\ArgumentResolver\RouteMatchValueResolver and is
+    // not the same object as the current route match service.
...
+    \Drupal::routeMatch()->getParameters()->set('commerce_order', $order);
+

Shouldn't we move this code further up? And not get the order from the route match to start with? Or alternatively, skip specyfing the route parameter type? So the param converter doesn't even attempt to load the order?

Not getting it from route match seems out of scope for this, I think that's not easy and would require API changes (passing it in explicitly). It's not this controller that is "the problem", it's the other place.

It could be moved a few lines up directly below getting the $order, but that's a difference of two lines and IMHO it makes sense to do the validate step first.

Fixed minor phpcs violations and committed the patch from #59. @Berdir: Thank you very much for this :).
Decided to trust the community on this, somehow curious about the potential performance impact though... (I personally haven't applied this to any of the projects I'm working on since I'm currently not experiencing this and the main projects I'm currently involved in are Headless).

It probably makes sense for the JSON API payment resources defined by Commerce API to also call loadForUpdate().

Awesome, thanks, one less patch to worry about!

I fear it's the opposite for me, only working on non-decoupled sites. Someone who uses that and has problems will need to look into that. It quite possibly is less of an issue there and it's mostly also specific to some payment gateways. Typically affects those offsite gateways that redirect back to the site and also have a callback. I guess JSON API also means fewer kernel shutdown shenanigans, which increases the chance of race conditions.

Any reason for reopening the issue? Just to go back to my performance concern, would there be a way to not check whether a lock is available on presave? I don't think so right?

Just to clarify, referring to the following line which is being called even whenever the order wasn't "locked".
!$this->lockBackend->lockMayBeAvailable($this->getLockId($order->id()

Perhaps the impact of this would be minimal for installations using the non DB lock backend, but could be non negligible for the ones using the DB lock backend.

Sorry, the status change was just a stale/refreshed browser window.

The lock check should be negligible, it's a single simple database query. The real performance cost here is more subtitle. The loadForUpdate() method does a loadUnchanged() to make sure we really get the current order entity directly from the database, so we bypasss the entity static and persistent cache. And actually saving does that again then (as already before this was committed). There might be some room for improvements there to statically cache the unchanged Entity, but that could also have side effects and we need to be very careful with that when an entity is saved multiple times on the same request for example.

I don't think statically caching the unchanged entity is a good idea either. I don't think there is anything to change atm... Unless we find bugs in the future ofc. Loading the unchanged entity is the right thing to do from loadForUpdate() for the reasons you mentioned (i.e making sure we really get the current order entity).

I guess there might be an easier way to optimize that, thinking about it. Entity save allows to preset the original entity, so we could do $order->original = clone $order; and it wouldn't load it a second time.

FYI we tracked down our OrderVersionMismatchException problem to some custom logic that was loading every single anonymous order object, and didn't filter for completed orders, so it ended up loading every anonymous cart too... and there were a LOT of them. And because of how the OrderStorage system works it was then trigger a save() operation. On every. Single. Anon. Cart.

Hi DamienMcKenna,
What was the logic you added to fix this issue?

We changed our custom logic to exit when the visitor was anonymous, so it never processed on anonymous carts/orders.