Fix the OPC ajax race family: per-cart serialization, single-flight autosaves, persist-first client reads

controllers/front/AbstractOpcJsonFrontController.php

@@ -34,10 +34,63 @@ protected function handleOpcRequest(): array
             return $this->buildTechnicalErrorResponse();
         }
 
+        // Serialize OPC ajax requests PER CART with a MySQL advisory lock. Concurrent
+        // requests (autosave, carriers/payment refreshes, selectaddress) all mutate the
+        // same cart row — several through full-row Cart::save() calls (including core's
+        // carrier computation) built from each request's own snapshot, so interleavings
+        // lose writes: duplicated/orphaned inline addresses, invoice pointer reverted to
+        // a stale value. Sequential flows only ever see an uncontended lock (~sub-ms);
+        // on timeout — or any error acquiring the lock — the request proceeds unlocked,
+        // i.e. degrades to today's behavior, never worse.
+        $lockName = null;
+        $lockTaken = false;
+        $cartId = (int) ($this->context->cart->id ?? 0);
+        if ($cartId > 0) {
+            try {
+                // GET_LOCK's namespace is server-wide, so two PrestaShop installs sharing one
+                // MySQL server would contend on bare cart ids. Discriminate by database + table
+                // prefix, hashed to a fixed length: lock names are capped at 64 characters and
+                // an over-long name is an ERROR (which would silently disable the lock). Built
+                // inside the try so an environment without the constants degrades unlocked too.
+                $lockName = 'opc_' . substr(md5(_DB_NAME_ . '/' . _DB_PREFIX_), 0, 8) . '_cart_' . $cartId;
+                $lockTaken = (bool) Db::getInstance()->getValue(
+                    "SELECT GET_LOCK('" . pSQL($lockName) . "', 10)"
+                );
+            } catch (Throwable $lockException) {
+                $lockTaken = false;
+            }
+
+            if (!$lockTaken) {
+                // Degrading is deliberate (never worse than the unserialized behavior), but a
+                // shop that degrades repeatedly is running the old race windows again — make
+                // that observable. Logging must never break the request it instruments.
+                try {
+                    PrestaShopLogger::addLog(
+                        sprintf('ps_onepagecheckout: per-cart lock not acquired for cart %d — request proceeds unserialized', $cartId),
+                        2,
+                        null,
+                        'Cart',
+                        $cartId,
+                        true
+                    );
+                } catch (Throwable $logException) {
+                    // Nothing to do: the request itself must proceed.
+                }
+            }
+        }
+
         try {
             return $this->handleAvailableOpcRequest();
         } catch (Throwable $exception) {
             return $this->handleRuntimeException($exception);
+        } finally {
+            if ($lockTaken && $lockName !== null) {
+                try {
+                    Db::getInstance()->getValue("SELECT RELEASE_LOCK('" . pSQL($lockName) . "')");
+                } catch (Throwable $releaseException) {
+                    // The lock auto-releases when the connection closes.
+                }
+            }
         }
     }
 

src/Checkout/Ajax/Carrier/OnePageCheckoutSelectCarrierHandler.php

@@ -75,7 +75,10 @@ public function handle(array $requestParameters = []): array
                 // hook below fired against the real row. A separate-billing temp stays possible.
                 $deliveryAddressId = $requestedAddressId;
                 $this->context->cart->id_address_delivery = $requestedAddressId;
-                if ((string) ($requestParameters['use_same_address'] ?? '1') === '1') {
+                if (
+                    array_key_exists('use_same_address', $requestParameters)
+                    && (string) $requestParameters['use_same_address'] === '1'
+                ) {
                     $this->context->cart->id_address_invoice = $requestedAddressId;
                 }
                 $this->context->cart->save();

tests/php/Integration/Checkout/Ajax/OpcSelectCarrierHandlerAddressBranchIntegrationTest.php

@@ -139,6 +139,29 @@ public function testFrontSentInvoiceIdSkipsTheTempInvoiceMount(): void
         }
     }
 
+    public function testAMissingUseSameAddressKeyDoesNotMirrorTheInvoicePointer(): void
+    {
+        $customer = $this->createCustomer();
+        $deliveryAddress = $this->createAddress($customer, 'FR', 'Delivery draft');
+        $invoiceAddress = $this->createAddress($customer, 'FR', 'Separate billing');
+        $cart = $this->createCart($customer, (int) $deliveryAddress->id, (int) $invoiceAddress->id);
+        $context = $this->createCheckoutContext($customer, $cart);
+
+        // A carrier choice that carries the delivery id but NO use_same_address key
+        // expresses no billing intent at all: it must not touch the invoice pointer
+        // (same guard as the carriers handler — array_key_exists, not a '1' default).
+        $response = $this->createSelectCarrierHandler($context, $this->createRecordingCartPresenter($context))->handle([
+            'delivery_option' => '1,',
+            'id_address_delivery' => (string) $deliveryAddress->id,
+        ]);
+
+        self::assertTrue($response['success'] ?? false, var_export($response, true));
+
+        $freshCart = new \Cart((int) $cart->id);
+        self::assertSame((int) $deliveryAddress->id, (int) $freshCart->id_address_delivery);
+        self::assertSame((int) $invoiceAddress->id, (int) $freshCart->id_address_invoice);
+    }
+
     public function testWithoutARequestedIdTheTempFallbackStillRuns(): void
     {
         $customer = $this->createCustomer();

views/js/opc-address.js

@@ -654,6 +654,23 @@ function rememberPersistedInlineAddresses(response) {
     if ($field.length) {
       $field.val(String(id));
     }
+
+    // This type's typed state is now persisted: persisted-id reads (previews, deferred
+    // carrier fetches) may trust the hidden id again. A use_same=1 save merely MIRRORS
+    // the delivery id onto the invoice pointer (the separate billing edit itself was
+    // skipped — the server mirror-reject guarantees distinct rows for a real separate
+    // billing), so it must not lift the billing hold. Mirroring only exists while the
+    // use_same checkbox is present AND checked: a virtual cart (billing-only, no
+    // checkbox) legitimately persists its billing onto BOTH cart pointers — treating
+    // that as a mirror would hold the payment refresh forever.
+    const useSameCheckbox = document.querySelector(SAME_ADDRESS_SELECTOR);
+    const isMirroredInvoice = fieldName === 'id_address_invoice'
+      && Boolean(useSameCheckbox && useSameCheckbox.checked)
+      && id === (parseInt(response.id_address_delivery, 10) || 0);
+    const fields = document.querySelector(fieldsSelector);
+    if (fields && !isMirroredInvoice) {
+      delete fields.dataset.opcDraftPending;
+    }
   });
 }
 
@@ -777,7 +794,7 @@ function bindAddressDraftAutosave(selectors) {
 
   let debounceTimer = null;
   let pendingContainer = null;
-  let inFlightDraftRequest = null;
+  let draftRequestInFlight = false;
 
   const buildDraftPayload = (addressContainer) => {
     const payload = collectAddressDraftPayload(addressContainer);
@@ -817,6 +834,17 @@ function bindAddressDraftAutosave(selectors) {
       return;
     }
 
+    // Serialize the saves WITHOUT aborting: an aborted POST still executes server-side
+    // and the address it inserts is never adopted client-side (its id only lands with
+    // the response) — an interleaved use_same re-check then strands it on the customer
+    // record. Never keep more than one savedraft in flight: the edit stays pending and
+    // ONE request — built from the form state at SEND time, carrying the ids the
+    // in-flight response just delivered — goes out when the current one settles.
+    // (pagehide beacons are exempt: the page is going away.)
+    if (!useBeacon && draftRequestInFlight) {
+      return;
+    }
+
     const addressContainer = pendingContainer;
     const payload = buildDraftPayload(pendingContainer);
     pendingContainer = null;
@@ -829,22 +857,20 @@ function bindAddressDraftAutosave(selectors) {
       return;
     }
 
-    // Serialize the saves: two concurrent savedrafts can BOTH insert a new address (neither has
-    // the persisted id yet — it only lands with the response), leaving duplicates on the customer
-    // record on slow shops. Abort the superseded in-flight request — its server-side write (if it
-    // got that far) stays covered by the cart-pointer fallback (reusableCartAddressId), and
-    // handleDraftFailure already ignores aborts by design.
-    if (inFlightDraftRequest && inFlightDraftRequest.readyState !== 4) {
-      inFlightDraftRequest.abort();
-    }
-
     // The savedraft endpoint saves a complete & valid address as a real address attached to the cart
     // (when a customer exists). Keep the inline form intact (no list switch); just remember the saved
     // address id(s) so later edits update the same address and the final submit reuses it instead of
     // creating a duplicate. The saved-address list appears naturally on the next full page load.
-    inFlightDraftRequest = $.post(draftUrl, payload)
+    draftRequestInFlight = true;
+    $.post(draftUrl, payload)
       .done((response) => handleDraftResponse(response, addressContainer))
-      .fail(handleDraftFailure);
+      .fail(handleDraftFailure)
+      .always(() => {
+        draftRequestInFlight = false;
+        if (pendingContainer) {
+          flushPendingDraft(false);
+        }
+      });
   };
 
   const scheduleAutosave = (event) => {
@@ -865,6 +891,13 @@ function bindAddressDraftAutosave(selectors) {
       return;
     }
 
+    // Mark the edited address type dirty: its typed state and persisted address may now
+    // differ, so persisted-id reads defer until the autosave confirms the persist.
+    const fieldsContainer = target.closest(`${DELIVERY_FIELDS_SELECTOR}, ${BILLING_FIELDS_SELECTOR}`);
+    if (fieldsContainer.length) {
+      fieldsContainer.get(0).dataset.opcDraftPending = '1';
+    }
+
     pendingContainer = addressContainer;
 
     if (debounceTimer) {
@@ -900,13 +933,8 @@ function bindAddressDraftAutosave(selectors) {
       return;
     }
 
-    if (inFlightDraftRequest && inFlightDraftRequest.readyState !== 4) {
-      inFlightDraftRequest.abort();
-    }
-
-    inFlightDraftRequest = $.post(draftUrl, buildDraftPayload(addressContainer))
-      .done((response) => handleDraftResponse(response, addressContainer))
-      .fail(handleDraftFailure);
+    pendingContainer = addressContainer;
+    flushPendingDraft(false);
   };
 
   prestashop.on(OPC_EVENTS.opcGuestInitSuccess, persistCompletedAddressOnGuestInit);

views/js/opc-carrier-list.js

@@ -17,6 +17,7 @@ import {
   getSelectedAddressId,
   getUseSameAddressValue,
   hasAddressPersistFailed,
+  hasPendingInlineDraft,
   INVOICE_ADDRESS_CONTEXT_FIELDS,
   isBuyerIdentified,
   isCarrierSectionReady,
@@ -322,6 +323,22 @@ function fetchCarriers() {
     return;
   }
 
+  // Persist-first: the typed inline address has a persist in flight and no persisted id
+  // yet (first fill) — fetching now would price raw fields against nothing. Show the
+  // loader and let the EXISTING persist rails follow up: refreshReadiness (driven by
+  // opcAddressPersisted) fetches any non-AVAILABLE section, and the persist-failed /
+  // inconclusive paths retract the loader. No extra listener — a second driver here
+  // would double the round (one fetch per section per round is a pinned contract).
+  if (
+    hasPendingInlineDraft(OPC_SELECTORS.opc.deliveryFields)
+    && !getSelectedAddressId(OPC_SELECTORS.opc.deliveryList, 'id_address_delivery')
+    && !getPersistedInlineAddressId(OPC_SELECTORS.opc.deliveryFields, 'id_address_delivery')
+  ) {
+    showCarrierLoading();
+
+    return;
+  }
+
   const carriersUrl = buildCarriersUrl(getConfiguredOpcUrl(URL_KEY));
   const fallbackMessage = getConfiguredOpcMessage('loadCarriersFailed', 'Unable to load delivery methods.');
 

views/js/runtime/address/opc-address-context.js

@@ -118,20 +118,45 @@ export function getSelectedOrInlineAddressId(listSelector, fieldsSelector, field
     || getVisibleInlineAddressId(fieldsSelector, fieldName);
 }
 
+/**
+ * An inline edit is pending its autosave persist: the typed fields and the persisted
+ * address may differ, so persisted-id reads must wait for opcAddressPersisted.
+ * The flag is set on input (opc-address.js scheduleAutosave) and cleared per address
+ * type once the persist response delivers that type's id.
+ */
+export function hasPendingInlineDraft(fieldsSelector) {
+  const fields = document.querySelector(fieldsSelector);
+
+  return Boolean(fields) && fields.dataset.opcDraftPending === '1';
+}
+
+/**
+ * Persist-first: once the inline address is persisted and untouched since, the hidden
+ * persisted id IS the typed state — previews can carry it instead of raw fields. While
+ * an edit is pending (dirty), returns '' so callers defer on the persist instead.
+ */
+export function getCleanPersistedInlineAddressId(fieldsSelector, fieldName) {
+  if (hasPendingInlineDraft(fieldsSelector)) {
+    return '';
+  }
+
+  return getPersistedInlineAddressId(fieldsSelector, fieldName);
+}
+
 export function buildSelectAddressPayload(form) {
   const useSameAddress = getUseSameAddressValue();
   const deliveryAddressId = getSelectedOrInlineAddressId(
     OPC_SELECTORS.opc.deliveryList,
     OPC_SELECTORS.opc.deliveryFields,
     'id_address_delivery'
-  );
+  ) || getCleanPersistedInlineAddressId(OPC_SELECTORS.opc.deliveryFields, 'id_address_delivery');
   const invoiceAddressId = useSameAddress === '1'
     ? deliveryAddressId
-    : getSelectedOrInlineAddressId(
+    : (getSelectedOrInlineAddressId(
       OPC_SELECTORS.opc.billingList,
       OPC_SELECTORS.opc.billingFields,
       'id_address_invoice'
-    );
+    ) || getCleanPersistedInlineAddressId(OPC_SELECTORS.opc.billingFields, 'id_address_invoice'));
 
   const payload = {use_same_address: useSameAddress};
 

views/js/runtime/address/opc-address-request.js

@@ -12,6 +12,7 @@ import {
   buildSelectAddressPayload,
   carrierPricesDependOnBillingAddress,
   hasDeliveryMethodsSection,
+  hasPendingInlineDraft,
 } from './opc-address-context';
 
 let selectAddressGeneration = 0;
@@ -84,7 +85,23 @@ function handleSelectAddressFailure(response) {
   });
 }
 
+// Persist-first: while an inline edit awaits its autosave, a preview request would leave
+// with raw fields (or a stale persisted id) — hold it and re-run once a persist
+// confirms. Inconclusive autosaves (incomplete address mid-typing) keep the hold armed:
+// section readiness owns the retract meanwhile, and the eventual successful persist is
+// the one that re-runs the refresh.
+let billingRefreshDeferredOnPersist = false;
+
 export function refreshAfterBillingAddressChange() {
+  if (
+    hasPendingInlineDraft(OPC_SELECTORS.opc.deliveryFields)
+    || hasPendingInlineDraft(OPC_SELECTORS.opc.billingFields)
+  ) {
+    billingRefreshDeferredOnPersist = true;
+
+    return;
+  }
+
   selectCurrentAddress()
     .done(() => {
       if (carrierPricesDependOnBillingAddress() && hasDeliveryMethodsSection()) {
@@ -97,6 +114,14 @@ export function refreshAfterBillingAddressChange() {
     .fail(handleSelectAddressFailure);
 }
 
+prestashop.on(OPC_EVENTS.opcAddressPersisted, (response) => {
+  if (billingRefreshDeferredOnPersist && response && response.address_persisted) {
+    billingRefreshDeferredOnPersist = false;
+    // Re-runs the pending checks: a still-dirty other address type re-arms the hold.
+    refreshAfterBillingAddressChange();
+  }
+});
+
 export function refreshAfterVirtualDeliveryAddressChange() {
   if (hasDeliveryMethodsSection()) {
     return;