Order can be placed twice if onReturn and onNotify called at the same time

Which gateway are you using? It's advised to call the OrderStorage::loadForUpdate() method from onNotify() if the order is loaded from the gateway plugin to avoid race conditions.

@anas_maw: I'd try patching commerce_paymob.

Instead of getting the order like the following:

$order = $payment->getOrder();, I'd call the loadForUpdate() method. See if that helps.

Some gateways handle this by not notifying right away about a payment (they sometimes notify with a 2 minutes delay, precisely to avoid race conditions.

We can't really fix this in Commerce core, because we can't call loadForUpdate() ourselves from onNotify() as the route doesn't pass the order.

On top of that... The onNotify() implementation doesn't check if a payment already exists. In some of the gateways I wrote, I return early if there is already a payment with the remote ID in the DB already. I see that logic is missing from Paymob.

Looking at the Commerce Paymob module, it doesn't place the order? So it shouldn't be placed twice? With that said, it probably doesn't hurt in Commerce to check if the place transition can be applied before applying it.

I don't think an error should be returned to the customer if the order was already placed? We could just silently ignore it?

Probably no need for that...

Thinking we could update CheckoutFlowBase::onStepChange()

Update:

      $order->getState()->applyTransitionById('place');

to:

if ($order->getState()->isTransitionAllowed('place')) {
  $order->getState()->applyTransitionById('place');
}

Once again, I do believe changes have to be made from onNotify() to ensure the order is locked, this way onReturn() would wait for the onNotify() to happen.

@anas_maw: Where is that code? Also looks like this was reported in the Paymob queue: #3588202: Implement locking mechanism to prevent duplicate order processing.

I have a reproduction for this, and it shows two things worth knowing: the
mechanism is not where the thread has been looking, and the change suggested in
#8 cannot fix it.

Reproduced on Commerce 3.x (79d5cdaf) with Drupal 11.4.7. Script attached as
.txt because drupal.org will not take a .php; rename it back and it runs under
drush php:script. It needs only commerce_payment_example and commerce_log. It does not fake threads. It just puts the site in the state the
two requests end in: the checkout flow holding an order object that was loaded
before the notification landed.

What actually happens

AFTER NOTIFY state=completed version=2 order_number='10'
checkout_complete log entries before return: 0
checkout_complete log entries after return: 1
RETURN OUTCOME redirect to the completion page (customer sees success)
FINAL state=completed version=2 order_number='11'
ORDER NUMBER CHANGED: true

So the order is not duplicated, but the place transition runs a second time and
reassigns the order number, from 10 to 11, after the customer has already been
given 10 by the notification path. The completion event fires for an order that
was already placed, so commerce_log records a checkout_complete that belongs to
a completion that already happened, and any subscriber to
CheckoutEvents::COMPLETION runs twice. placed_time survives, because it is only
set when empty, which is probably why this reads as intermittent rather than
obviously broken.

Why #8 cannot fix it

@jsacksick, the suggestion was to change

if ($order->getState()->getId() == 'draft')

to

if ($order->getState()->isTransitionAllowed('place'))

Both are evaluated on the order object the checkout flow is holding, and that
object is the stale one. On it:

state=='draft': true isTransitionAllowed('place'): true

Both are true, so the guard changes nothing, which matches @anas_maw reporting
in #10 that applying it made no difference. It is not that the guard is wrong,
it is that there is nothing wrong with the state it inspects. The order object
is simply out of date.

Where the staleness comes from

PaymentCheckoutController::returnPage() does the right thing already: it calls
$order_storage->loadForUpdate($order_id) and then setOrder() on the flow. The
trouble is that loadForUpdate() is loadUnchanged() plus a lock, and
loadUnchanged() prefers the persistent entity cache over the database. That is
exactly what #3612725 describes, and this issue looks like its user-visible
face on the checkout path.

The version guard does not catch it either, for the reason set out on that
issue: EntityStorageBase::doPreSave() populates $order->original through the
same loadUnchanged(), so Order::preSave() compares the stale copy against
itself, the versions match, no OrderVersionMismatchException is raised, and the
save goes through. In the run above the final version is still 2 and no
exception reached the customer, who sees a normal completion page with a
different order number from the one the notification generated.

Interestingly, when I forced the original to be fresh, the version guard did
fire and the customer got an uncaught OrderVersionMismatchException instead,
because redirectToStep() in PaymentCheckoutController::returnPage() sits
outside the try/catch that wraps onReturn(). So the two failure modes are a
silently renumbered order, or an error page after a successful payment.

What I would suggest

1. Link this to #3612725 rather than fixing it here. If loadForUpdate() returns
the committed row, the existing state=='draft' check is already correct and
this symptom disappears.
2. Separately, and regardless of that, consider moving redirectToStep() inside
the try/catch in returnPage(), or catching OrderVersionMismatchException
around it. A customer who has just paid should not see an exception page
because another process got there first.
3. If you want defence in depth in onStepChange() that does not depend on the
cache being right, the check has to consult the stored version rather than
the in-memory state, since the in-memory state is the thing that is wrong. I
did not write that as an MR because it would duplicate what #3612725 is
already solving properly, and because CheckoutFlowBase is widely subclassed
so adding a constructor dependency there is not free. Happy to do it if you
would like it as a belt-and-braces measure, and happy to turn the attached
script into a kernel test either way.

This work was done with AI assistance. Every figure above is output from the
attached script on the versions named, not inferred.