Problem/Motivation
OrderStorage::loadForUpdate() is the API introduced in #3043180 to guarantee "acquire the update lock, then load the freshest revision of the order". It implements the fresh read with loadUnchanged(), based on this assumption (#3043180-59):
The loadForUpdate() method does a loadUnchanged() to make sure we really get the current order entity directly from the database, so we bypasss the entity static and persistent cache.
That assumption is not true. Since core #2753675 (Drupal 8.3), ContentEntityStorageBase::loadUnchanged() only resets the static entity cache and deliberately prefers the persistent entity cache over the database. So loadForUpdate() returns whatever is in the shared cache_entity bin, lock or no lock.
On non-transaction-aware cache backends (Redis, Memcache), the persistent cache can legitimately hold a stale copy of an order:
- Late write-back:
resetCache()removes entries with a raw delete. A concurrent request that read the pre-save row can complete itssetPersistentCache()write-back after the saving process deleted the entry, resurrecting the old row in the shared cache. - Pre-commit invalidation (core #2347867): during an entity save, the cache delete in
EntityStorageBase::doPostSave()runs inside the still-open database transaction, before the post-save hooks and the COMMIT. With Redis the delete is executed immediately, so any process that loads the order in the delete->commit window gets a cache miss, reads the old committed row from the database, and writes it back into the shared cache. The heavier the post-save hooks, the wider the window.
The consequence is worse than a stale read: the subsequent save is a silent lost update. When the caller saves the order, $order->original is populated via loadUnchanged() from the same stale cache entry, so the version comparison in Order::preSave() passes and no OrderVersionMismatchException is thrown or logged. Every change from the overwritten save is reverted without any error, log entry or state transition event.
A concrete reproduction in the wild, on a high-traffic site using Redis for the entity cache: a payment gateway notification loads an order while the commerce_recurring recurring order close job is saving it, writing the pre-save row back into the shared cache; a later notification then calls loadForUpdate(), receives the stale order, and its save silently reverts a completed order back to needs_payment. The affected orders end up exactly one version behind, with no mismatch exception and no transition log entry.
The database cache backend is immune to mechanism 2.
Steps to reproduce
See the included kernel test (OrderLockingTest::testLoadForUpdateSkipsStalePersistentCache): it saves a change to an order from a "concurrent request", simulates that request's stale copy being written back into the persistent cache, then asserts loadForUpdate() returns the committed data. It fails against the current code: loadForUpdate() returns the stale copy.
Proposed resolution
Make loadForUpdate() honor its contract: call $this->resetCache([$order_id]) (which clears the static and persistent entries) before loadUnchanged(), on both the immediate-acquire and the lock-wait branches.
Performance impact is negligible: loadForUpdate() only runs on write paths, so this adds one uncached entity load per locked update, and that load immediately repopulates the persistent cache with fresh data, so read paths are unaffected.
This intentionally does not change loadUnchanged() itself, its cache preference is core behavior (#2753675) on a hot path (every entity save), and changing it is out of Commerce's hands.
Remaining tasks
- Review the fix and test.
- Follow-up candidates: a docblock on
OrderStorage::loadUnchanged()clarifying that it does not bypass the persistent entity cache, and an audit of save paths that modify orders withoutloadForUpdate()(e.g.commerce_recurring's close job), which remain exposed to the same class of staleness.
User interface changes
None.
API changes
None. loadForUpdate() keeps its signature; it now actually returns database-fresh data as documented.
Data model changes
None.
AI usage
This issue summary and the attached patch were drafted with AI assistance and reviewed by a human before posting.
| Comment | File | Size | Author |
|---|---|---|---|
| commerce-loadforupdate-reset-persistent-cache-3.x-upstream.patch | 4.91 KB | redwan jamous |
Issue fork commerce-3612725
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
redwan jamous commentedComment #4
redwan jamous commented