Race conditions can theoretically change placed orders

I think I might be running into this occasionally in production. It's difficult to know for sure but we have a cron job that checks orders and sends them off to a third party system. We also have the occasional order that has received a payment, but is still in draft state, cart is set to 1, and the order number is missing. I thought it originally was payment method related but now I have seen this in orders using both Stripe and PayPal so I think this might be a Commerce core issue.

Now I've found this issue I will try to debug it further although I'm still not certain what the trigger is - it has happened so far to maybe 1 in 1000 orders.

We're expenceriencing issues with some orders that have their state randomly reverted back to draft as well, and I've never really had time to dig into that further.

Looking at OrderRefresh::shouldRefresh(), I'm wondering about making one, possibly 2 changes to the logic that could prevent this from happening for the use case described by @eiriksm:

1. Skip refreshing the order if the current PHP process runs on CLI?
2. There's a setting to only refresh the order that belongs to the current customer, and this is the check currently done:
   if ($order->getCustomerId() != $this->currentUser->id()) {
   The problem is, for anonymous users, that condition will return TRUE (if the order belongs to an anonymous user). So perhaps we should only do this if the order belongs to an authenticated user?

   Perhaps it should be:
   if ($order->getCustomer()->isAuthenticated() && ($order->getCustomerId() != $this->currentUser->id())) instead?

   Or... We should keep the current check, but additionally check if the current user has access to "update" (or probably view) the order? This way if commerce_cart is installed, the access logic that ensures the anonymous user has access to update its order will kick in).

Thoughts?

Might make sense to skip the auto refresh on load for loads triggered by CLI as in 99% of the cases, that is probably not the intented behavior (i.e loading an order to do some check shouldn't trigger a "write" operation).

And, if the order is a draft and it's saved by a CLI script, then it'll get refreshed... So it should be a safe change?

I actually like the idea of skipping the order refresh on load, it's a straightforward patch that should solve the issue initially described.

But also wondering about the other change too.

> Skip refreshing the order if the current PHP process runs on CLI?

Cron hooks etc don't have to be run from CLI, they can be run at the end of any page request if there is no external cron runner - I would suggest that CLI and HTTP requests should be treated no differently or it risks making things even trickier to debug.

Regarding the anonymous check, that's a good call that I hadn't spotted - on the site that is experiencing this behaviour, all our orders are anonymous, we don't create customer accounts. We have other Commerce sites that I haven't noticed it happening on, and they generally have authenticated users making orders.

> additionally check if the current user has access to "update" (or probably view) the order? This way if commerce_cart is installed, the access logic that ensures the anonymous user has access to update its order will kick in).

I like this idea, this should mean that only the actual anonymous user would trigger an update of their own order.

Cron hooks etc don't have to be run from CLI, they can be run at the end of any page request if there is no external cron runner - I would suggest that CLI and HTTP requests should be treated no differently or it risks making things even trickier to debug.

Good point, perhaps it makes sense to work on the other proposal though that would only potentially solve automatic refresh when the refresh is configured to refresh only if the order belongs to the current user.

There is already some code in Order::preSave() that attempts to protect against this:

    if (isset($this->original) && !$this->isNew() && $this->original->getVersion() > $this->getVersion()) {
      $mismatch_exception = new OrderVersionMismatchException(sprintf('Attempted to save order %s with version %s. Current version is %s.', $this->id(), $this->getVersion(), $this->original->getVersion()));

However I'm not sure this is good enough, as it appears $this->original could still be out of date? Instead of $this->original->getVersion() should an uncached value be loaded directly?

$this->original is in theory not cached:

if ($id_exists && !isset($entity->original)) {
$entity->original = $this->loadUnchanged($id);
}

Did you configure your site to throw exceptions or log them?

Ah, it's set to log only, but I don't think I have watchdog logs going back as far as the last time this happened. Maybe I should set this to throw an exception...

I think we are also running into this in a pretty simple case, also related to having a short refresh frequency, where we are unable to save an order from the edit form because it was refreshed/saved during the course of the request. I needed to patch our postLoad to the following to allow us to save the order:

  /**
   * {@inheritdoc}
   */
  protected function postLoad(array &$entities) {
    $request = \Drupal::requestStack()->getMainRequest();
    $is_edit = str_contains($request->getPathInfo(), 'commerce/orders')
      && str_ends_with($request->getPathInfo(), 'edit');

    if (!$this->skipRefresh && !$is_edit) {
      /** @var \Drupal\commerce_order\Entity\OrderInterface[] $entities */
      foreach ($entities as $entity) {
        $explicitly_requested = $entity->getRefreshState() == OrderInterface::REFRESH_ON_LOAD;

        if ($explicitly_requested || $this->orderRefresh->shouldRefresh($entity)) {
          // Reuse the doPostLoad logic.
          $entity->setRefreshState(OrderInterface::REFRESH_ON_SAVE);
          $entity->save();
        }
      }
    }

    return parent::postLoad($entities);
  }

I should note that I attempted to do this check using \Drupal::routeMatch() to check the route name instead, but the route name was null, so I had to rely on the request itself. Probably not ideal for all cases, since the uri might have been altered on any particular site.

Related: #3043180: The changes made to the order on the onNotify method are not applied on the onReturn method

So going back to this, my recommendation to skip the refresh on load... If there is no intent to save the order would be to call the loadUnchanged() method.

Also, Now that #3043180: The changes made to the order on the onNotify method are not applied on the onReturn method is in, we now have a loadForUpdate() method that you can be called if needed. So I wonder if we should close this?