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.

The persistent entity cache is shared between processes, and it's possible for it to legitimately hold a stale copy of an order: a request that reads an order concurrently with another process saving it can complete its cache write-back after the save's own cache invalidation ran, resurrecting the pre-save row in the shared cache. This is a generic race in core's entity cache layer, not specific to a particular cache backend's transaction timing — Commerce is simply more exposed to it than most entity types because orders tend to be saved at a high rate (checkout, payment gateway callbacks, recurring billing, etc.), which widens the window in which the race gets hit in practice.

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. Note that this isn't limited to callers that go through loadForUpdate(): core's own save process calls loadUnchanged() internally (EntityStorageBase::doPreSave()) to populate $entity->original whenever it isn't already set, so any order save can hit the same stale-cache-masks-conflict path, whether or not the caller used loadForUpdate().

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.

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

Fix this in OrderStorage::loadUnchanged() rather than in loadForUpdate(): call $this->resetCache([$id]) there before deferring to the parent implementation, clearing both the static and the persistent entries.

OrderStorage already overrides loadUnchanged(), and every relevant call — loadForUpdate()'s two branches, and core's own internal call in doPreSave() to populate $entity->original — routes through that one override. Fixing it there closes both paths in a single, smaller change, instead of only protecting the explicit loadForUpdate() call and leaving the implicit ->original population (the thing the version-mismatch check actually relies on) still exposed.

Performance impact is negligible: loadUnchanged() is only meant to be called on save/refresh paths that are already paying for a full entity load, so forcing a database round-trip there doesn't add cost to read paths; the freshly loaded entity repopulates the persistent cache immediately after.

Remaining tasks

  • Review the fix and test.

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.

CommentFileSizeAuthor
commerce-loadforupdate-reset-persistent-cache-3.x-upstream.patch4.91 KBredwan jamous

Issue fork commerce-3612725

Command icon 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:

About issue forks

Comments

redwan jamous created an issue. See original summary.

redwan jamous’s picture

redwan jamous commented
Version: 3.0.x-dev » 3.x-dev

redwan jamous opened merge request !723

redwan jamous’s picture

redwan jamous commented
Assigned: redwan jamous » Unassigned
Status: Active » Needs review

jsacksick made their first commit to this issue’s fork.

jsacksick’s picture

jsacksick commented

@redwan jamous: Thanks for working on this. FYI pinged @berdir on Slack who actually worked on adding the loadForUpdate() method. It's great that we have a test ans a detailed explanation.

That could potentially explain the behavior I'm seeing on a project, where a very small portion of orders get their state reverted (from fulfillment to draft).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> 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.

This is not correct. Redis has been relying on the post transaction callback feature since 2020 to only trigger cache tag and delete operations after the database transaction completes: #3018203: Support delayed cache tag invalidation

Memcache is a different story, that is still not fixed there.

Haven't verified the performance claims, it's true that it doesn't happen too often.

If there's a race condition on cache reset vs cache write then why would loadForUpdate() be affected in a different way compared to load and loadUnchanged()? In your test, both would return the stale cache at that point, not? how is that a problem only with that method then? Anything not using loadForUpdate(), also any other entity could run into the same race condition? I guess orders and in general commerce entities are more likely to run into it as they tend to be written a lot.

jsacksick’s picture

jsacksick commented
Issue summary: View changes

Good point, and you're right about the framing — I'll update the issue summary to describe this as a workaround for a core cache-consistency gap rather than pinning it on Redis timing specifically (that part of my original writeup doesn't hold up, since Redis has been deferring cache invalidation to post-commit since #3018203). Orders just hit it more because they're a high-write entity.

On the loadUnchanged()/->original point: good catch, that's a real gap in the patch as posted. EntityStorageBase::doPreSave() calls $this->loadUnchanged($id) on every save() to populate $entity->original when it isn't already set, and that's exactly what Order::preSave() compares against for the version check. Patching only loadForUpdate() leaves that call unprotected, so the same staleness could still slip past the version-mismatch guard even after loading the order correctly via loadForUpdate().

Since OrderStorage already overrides loadUnchanged(), and both loadForUpdate() and core's internal doPreSave() call route through that one override, I moved the resetCache() call there instead of duplicating it in loadForUpdate()'s two branches. That closes both paths with a smaller diff. Updated the MR.

jsacksick’s picture

jsacksick commented

Pushed a followup: loadForUpdate() now seeds $order->original with a clone of the order it just loaded, rather than leaving it unset.

Without that, EntityStorageBase::doPreSave() would load the order a second time (and reset the persistent cache a second time) to populate $entity->original when the caller saves it, right after loadForUpdate() already did the same load. The lock guarantees nothing else can change the order between the two, so the copy loadForUpdate() already has is a valid "before" baseline, no need to fetch it twice.

Uses setOriginal() where it exists (Drupal 11) and falls back to setting the property directly on Drupal 10, matching the same pattern already used in OrderRefresh.php for this exact core version split.

tomtech’s picture

tomtech commented

@jsacksick,

This is the issue I filed previously against core: #3563047: A new object is returned when loading a previously-saved entity.

I need to review this one a bit more, but the main similarity is that calling a load() of an entity after a save() will cause issues.