Persist-first client reads: previews carry persisted ids, refreshes wait for the in-flight persist#122
Closed
ThbPS wants to merge 8 commits into
Closed
Conversation
…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>
OpcTempAddress::cleanup() restored the cart pointers captured at request start (in-memory cart) with a blind full-row Cart::save(): a concurrent use_same re-check or savedraft persist committing between capture and cleanup was silently reverted, leaving the cart pointing at a stale — possibly deleted — address (nightly 15s pollInvoiceIsSeparate timeout). Capture the pre-swap pointers from a fresh DB read at mount time, swap with pointer-only UPDATEs instead of full-row saves, and restore each pointer only if it still holds OUR temp id (atomic UPDATE ... WHERE id_address_x = tempId). A newer concurrent commit wins; with no concurrency the behavior is unchanged. Belt-and-braces under the per-cart lock: non-OPC writers (core-theme cart ajax) stay outside it. 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>
…ess contracts Two regression tests ported from the deterministic race fixtures that proved the bugs on main: - OpcTempAddress::cleanup() under a concurrent use_same re-check must yield to the newer pointer commit — never resurrect the pointer to the now-deleted billing address (the nightly 15s pollInvoiceIsSeparate timeout mechanism). - 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) holds the loader and re-fetches with the persisted id once the autosave confirms — same inconclusive-keeps-the-defer rule. TDD: specs/local-persist-first/persist-first-address-reads.spec.ts (opc.e2e.tests branch test/persist-first-address-reads) — C1/C2/C3 red on the previous client, green here. views/public bundles rebuilt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ThbPS
force-pushed
the
feat/persist-first-address-reads
branch
from
July 15, 2026 13:39
bd0c1a1 to
7fb1f0f
Compare
Collaborator
Author
|
Consolidated into #121 per review decision: one train so everything is tested together. The final diff leaves OpcTempAddress strictly untouched (the persist-first client makes the temp branches cold; the per-cart lock serializes the OPC-vs-OPC interleavings). The full temp-machinery retirement stays a future refactor PR — already TDD-validated 11/11 in CI (run 29418233325), parked on wip/temp-retirement-full. |
ThbPS
force-pushed
the
fix/serialize-opc-ajax-per-cart
branch
from
July 15, 2026 13:56
9687287 to
f138d89
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.
The three fixes (one commit, +111/−7, client only)
buildSelectAddressPayloadresolves the autosave-persisted hidden id unless an edit is pending (opcDraftPendingdataset dirty-flag, set on input, cleared per address type by the persist response; ause_same=1save only MIRRORS the delivery id onto the invoice pointer and must not lift the billing hold).opcAddressPersisted. Inconclusive autosaves (incomplete address mid-typing) keep the hold armed — the eventual successful persist re-runs the refresh; section readiness owns the retract meanwhile.updatedCart, retry click) holds the loader and re-fetches with the persisted id once the autosave confirms — same inconclusive-keeps-the-defer rule.TDD evidence
Red-first e2e specs (
PrestaShopCorp/opc.e2e.testsbranchtest/persist-first-address-reads, not in CI lots — same convention aslocal-race-repro):id_address_delivery=0instead of the persisted id → green.Validation: full
opc-payment-eligibilitylot green locally; A/B baseline confirmed the residual ~1/6 local flake on two billing specs pre-exists on #121's content (not introduced here).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): it is re-validated against every address round —
dropped when unavailable (unchanged protection, still asserted by the no-carrier leg
of spe42) and restored when still valid (was: silently lost). spe42's cookie-clear
pin is migrated accordingly on the paired e2e branch.
Merge pairing: merge together with
PrestaShopCorp/opc.e2e.testsbranchtest/persist-first-address-reads(C1-C3 specs + the spe42 contract migration).Deliberately out of scope
The temp-address machinery itself (
OpcTempAddress, the two cookie storages, the guard mount) stays untouched as the fallback for third-party raw-fields integrations and degraded windows. Its retirement is a separate refactor PR once this has soaked — the full TDD'd retirement (server awaiting contracts + deletions, −651 lines) is already prepared and validated (10/11 lots CI, the single red being a cookie-contract e2e pin) if/when wanted.🤖 Generated with Claude Code