Fix the OPC ajax race family: per-cart serialization, single-flight autosaves, persist-first client reads#121
Draft
ThbPS wants to merge 8 commits into
Draft
Fix the OPC ajax race family: per-cart serialization, single-flight autosaves, persist-first client reads#121ThbPS wants to merge 8 commits into
ThbPS wants to merge 8 commits into
Conversation
bd0c1a1 to
9687287
Compare
9687287 to
f138d89
Compare
494887a to
3f61eef
Compare
…sory lock
Concurrent OPC ajax requests (autosave, carriers/payment refreshes,
selectaddress) all mutate the same cart row, several through full-row
Cart::save() calls built from each request's own snapshot, so
interleavings lose writes: duplicated/orphaned inline addresses and an
invoice pointer reverted to a stale value (nightly
inline-billing-no-duplicate: addressCount 6-9 instead of 2, and the
15s pollInvoiceIsSeparate timeout).
GET_LOCK('opc_cart_<id>', 10) around handleOpcRequest serializes them:
sequential flows only ever see an uncontended lock (~sub-ms), and on
timeout — or any error acquiring the lock — the request proceeds
unlocked, i.e. degrades to today's behavior, never worse. The lock
auto-releases if PHP dies.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…me_address The selectcarrier id-branch defaulted a MISSING use_same_address key to '1', so a carrier selection carrying no billing intent at all silently reset cart.id_address_invoice to the delivery address, clobbering a separate billing the buyer had persisted. Align on the carriers handler's guard (array_key_exists + === '1'): no key, no mirror. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aborting Aborting the superseded in-flight savedraft is client-only: the POST keeps executing server-side and the address it inserts is never adopted client-side (its id only lands with the response the abort threw away). An interleaved use_same re-check then strands it on the customer record — removeAbandonedInlineInvoiceAddress can only delete the id the front knows. On slow shops this accumulates orphan 'Invoice address' rows (nightly counts of 6-9 for an expected 2). Never keep more than one savedraft in flight and never abort: 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). views/public bundle rebuilt from sources. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GET_LOCK's namespace is server-wide: two PrestaShop installs sharing one MySQL server would contend on bare 'opc_cart_<id>' names — spurious serialization, never corruption, but easy to rule out. The discriminator is hashed to a fixed length because lock names are capped at 64 characters and an over-long name errors out, which the degrade path would turn into a silently never-locking controller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regression test ported from the deterministic fixture that proved the bug on main: a selectcarrier request carrying a delivery id but no use_same_address key must not mirror the invoice pointer onto the delivery address (was: missing key defaulted to '1' and clobbered a persisted separate billing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An environment where _DB_NAME_/_DB_PREFIX_ are undefined (the module's own dockerized unit runner; exotic bootstraps) fataled on the lock-name construction instead of degrading unlocked — the whole acquisition now sits inside the same try/catch as the GET_LOCK call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fetches wait for the in-flight persist The guest inline flow made the server mount-and-delete temp address rows on routine rounds because the client kept sending raw fields even once the autosave had persisted them. The client now resolves the persisted state first, so the server takes its battle-tested id-branch and the temp branches become cold fallbacks (third-party raw-fields integrations, degraded windows) — no server behavior change: - selectaddress previews resolve the autosave-persisted hidden id when the form is untouched since its persist (dirty-tracking via an opcDraftPending dataset flag set on input, cleared per address type by the persist response — a use_same=1 save only MIRRORS the delivery id onto the invoice pointer and must not lift the billing hold); - previews triggered while an edit awaits its autosave are held and re-run on opcAddressPersisted; inconclusive autosaves (incomplete address mid-typing) keep the hold armed — the eventual successful persist re-runs the refresh, readiness owns the retract meanwhile; - a carriers fetch racing the FIRST persist (core updatedCart, retry click) shows the loader and lets the EXISTING persist rails follow up (refreshReadiness fetches any non-AVAILABLE section on opcAddressPersisted; the failed/inconclusive paths retract) — no extra listener, a second driver would double the coalesced round (one fetch per section per round is a pinned contract). TDD: specs/local-persist-first/persist-first-address-reads.spec.ts (PrestaShopCorp/opc.e2e.tests#58) — C1/C2/C3 red on the previous client, green here. views/public bundles rebuilt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Degrading to the unserialized behavior is deliberate (timeout or any acquisition error — never worse than before the lock), but a shop that degrades repeatedly is running the old race windows again. Surface it in the shop logs; the logging itself can never break the request it instruments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3f61eef to
46e869f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
inline-billing-no-duplicate.spec.ts:109failure (addressCount 6-9 instead of 2, and the 15spollInvoiceIsSeparatetimeout): orphan invoice-address inserts from client-only savedraft aborts, invoice-pointer clobbers from stale-state writers, and routine mid-persist pricing. Server serialization + client sequencing;OpcTempAddressis left strictly untouched (the persist-first client makes its branches cold paths).opc-payment-eligibilitysystematic failure since 2026-07-13 (runs 29263323883 / 29302624359 / 29385661715 on PrestaShopCorp/opc.e2e.tests).main, GREEN here; red-first client specs; full e2e CI dispatches.The race mechanisms (proven from run 29385661715 traces)
Received: 9) — aborting a superseded savedraft XHR is client-only: the POST keeps executing server-side and inserts a realInvoice addressrow whose id the client never learns.removeAbandonedInlineInvoiceAddresscan only delete the front-known id, so every round inserts anew (trace: 8/16 savedraft POSTs diednet::ERR_ABORTEDwhile the invoice-id ladder 27→36→44 shows their inserts landed).Cart::save()calls built from request-start snapshots silently revert concurrent pointer commits; the cart can be left pointing at a deleted address.The fixes (7 commits, reviewable independently)
Server (mechanisms 1-2):
GET_LOCK('opc_<hash>_cart_<id>', 10)around every OPC ajax request (all 17 controllers) — uncontended in sequential flows, degrades to today's behavior on timeout or any error (lock name built inside the guard).use_same_addresskey (aligned on the carriers handler's guard; was: a missing key defaulted to '1' and clobbered a persisted separate billing).Client (mechanisms 1-3):
opcDraftPendingdirty-flag; ause_same=1MIRROR response must not lift the billing hold); previews on a dirty form are held and re-run onopcAddressPersisted(inconclusive autosaves keep the hold armed); a carriers fetch racing the FIRST persist holds the loader and re-fetches with the persisted id. The server takes its battle-tested id-branch everywhere — the temp branches become cold fallbacks, untouched in this diff.The one observable contract change (deliberate)
Routing the carrier click through the id-branch moves the selection onto the CART (instead of the temp-keyed cookie): re-validated against every address round — dropped when unavailable (unchanged protection, still asserted by spe42's no-carrier leg) and restored when still valid (was: silently lost).
Merge pairing required: merge together with PrestaShopCorp/opc.e2e.tests#58 (red-first C1-C3 specs + the spe42 contract update).
Proofs
invoice_clobbered: true→ false), plus the CI traces above for the abort/orphan mechanism.opc-payment-eligibilitylot green locally; A/B baseline shows the residual ~1/6 local flake on two billing specs pre-exists on main-level content.:109first-attempt pass on the runners that failed it 3/3 nightly).Follow-up (out of scope)
Full temp-machinery retirement (server awaiting contracts + deletion of
OpcTempAddress/storages/guard mount, −651 lines) — already TDD-validated 11/11 in CI (run 29418233325), parked for a future refactor PR once this has soaked.🤖 Generated with Claude Code