Duplicate confirmation e-mail

Based on what you're describing in #3, it might be related to #3043180: The changes made to the order on the onNotify method are not applied on the onReturn method.

Sounds very much related hence closing as duplicate.

I've successfully recreated this bug and it's not the same as in #3043180, but may be related in specific circumstances.

Steps to reproduce race condition:

1. Put product to cart, go to checkout to the very last step before placing an order (choosing on-site payment or similar that doesn't require user interaction at the moment of placing an order).
2. Open exactly same page in a separate browser tab.
3. Click to place an order in both tabs at the same time (with max 1-2 seconds delay).

As a customer you will receive 2 emails with different order number (but in fact it's the same order ID), on the order page you will se duplicated entry:
>Order moved from Draft to Validation by the Place order transition.

I would seek the core of the problem in state machine (and prolly caching). Every transition should be atomized [see ACID transactions] on the database level and thus it shouldn't be possible to execute state transition twice (if transition logic prevents that). Order number pattern executed twice, and customer email sending twice - is rather a collateral bug.

Having the same problem. Currently using Drupal 9.5.9 and Commerce 8.x-2.35. Able to recreate the issue on our dev server by using the back button after order completion. The order transitions from draft to complete two separate times.

hm.. This is going to be tricky to fix/debug... I'm wondering if calling the loadForUpdate() method from CheckoutFlowBase::getOrder() would fix this... but I'm afraid this could cause the order to be locked when the order isn't going to be saved...

It could be that PaymentProcess::isVisible() still returns TRUE even thought he order is paid because the total_paid field is not yet updated since that is done from the PaymentOrderUpdater on destruct().

Another reproduction of this, with a trigger that is not the browser back button and not the return versus notify race that #3043180 closed. It may be useful because it points at the same fix jsacksick proposed in #8, from a different direction.

The trigger: an onsite gateway that makes a remote call inside the payment step

The payment step is a GET. PaymentProcess::buildPaneForm() calls the gateway's createPayment() while the page is being built, and the redirect that follows places the order in CheckoutFlowBase::onStepChange(). When the gateway talks to a remote API inside createPayment(), the whole step stays in flight for as long as that call takes, and a second GET for the same URL runs all of it again.

So the customer gets two payment attempts, the order is placed twice, two order numbers are consumed, and every subscriber of the place transition runs twice. In our case that last part meant two documents in the ERP for one order.

What makes this variant easy to overlook is that no offsite gateway is involved and there is no webhook racing anything. It is one customer, one browser, one order, and a refresh at the wrong moment.

What the logs show

Over a year of production logs, every occurrence was on the one gateway that performs an HTTP request inside the payment step. The store's other gateways, including a reference-based one and a card one, never produced a single case, which fits the mechanism: without a remote call there is barely a window to hit.

The two requests were consistently one to two seconds apart, which matches the latency of the remote call rather than any human interval. The order's commerce_log entries are the clearest evidence: two payment_authorized with different remote ids, two checkout_complete, two Place order transitions and two receipt emails, all within the same two seconds.

On the concern in #8

The worry about loadForUpdate() in CheckoutFlowBase::getOrder() causing unwanted locking looks well founded to us, because getOrder() is called on every step and on requests that never save the order.

What we shipped instead scopes the lock to the one step that needs it, which may be a narrower option worth considering here. A subclass of PaymentProcess takes the lock at the top of buildPaneForm(), re-reads the order, and skips the step when another request already placed it or already created a payment on the same gateway. The lock is released in a finally, which matters because both the success path and the skip path leave the method by throwing NeedsRedirectException.

This is exactly what PaymentCheckoutController::returnPage() already does for offsite gateways: lock, re-read, and revalidate the step if the state moved while waiting. The onsite path simply has no equivalent, and PaymentProcess never checks whether a payment already exists for the order.

Two measurements, since the cost of locking is the open question. Acquiring the lock on a real order measured 0.2 ms, against a step that already spends seconds inside a remote call. And in the hours since it went live, no request has failed to acquire the lock and no legitimate checkout has been skipped, across every gateway the store offers. That is early, so treat it as a first data point rather than a soak test.

Where this leaves the gateway modules

We also added a guard inside our own gateway, which refuses a second request when the order already has a live one from the last seconds. It is worth saying plainly that this is not a substitute: a gateway can stop the duplicate payment, but it cannot stop the second place transition, the second order number or the second receipt email, because those happen in the checkout flow and not in the gateway. Only a lock on the step can. Until Commerce has one, every onsite gateway that calls a remote API in createPayment() has to defend itself and still cannot cover the rest.

Happy to help test a patch here, or to open a merge request with the pane approach if that direction is useful.