Loading entity during save will cache old version

This sounds a lot like a very rare and hard to reproduce issue we've seen on our production Acquia sites. In our case, this manifested as the incorrect revision being marked as the default revision, and showing stale information to end-users on the front-end.

Following, and will try to reproduce in stage! Thanks for filing this issue! It could help to explain the "that can't happen" thing that certainly happens 🤣!

I think this is a 'known' issue in the following case:

1. Empty entity cache for the entity.

2. Process A loads the entity, get a cache miss.

3. Process B saves the same entity and clears the cache.

4. Process A writes to the cache.

You would think that process A would be able to write to the cache before process B is simultaneously able to load and save the entity, but there are probably at least two scenarios that could make this more likely.

Scenario 1:

Taking the steps above, we can add a step zero.
0. Process B, a long running drush process, loads the entity (cache hit or miss doesn't matter).

This means that step 3 is only required to modify and save the entity, not load it first, because that already happened in step zero.

--
Scenario 2:

Something happens during entity loading (loading other entities in hook_entity_load(), querying from custom tables etc., just having a lot of fields) that makes it take a long time. e.g. if there is 50ms or 100ms between getting a cache miss and writing back to the cache, that allows another process to save in the meantime.

There's at least a couple of ways to mitigate this:

1. Add some kind of lock/validation. For example when revisions are enabled we could check that the default revision ID in the database is the same as the one we're about to write a cache item for. But there are problems with this, extra work during an operation that needs to be fast, and still a potential race condition between running that query and writing the cache item, just a shorter one.

2. Modify entity caching so that when we save an entity, instead of deleting the cache item, we overwrite it with one where $cache->data === FALSE - a tombstone record.

Entity cache gets would need to be modified to treat $cache->data === FALSE as a miss.

Entity cache sets would need to be modified to first get from cache, and if the timestamp is newer than REQUEST_TIME or a stored timestamp of when we attempted the cache get, discard the cache write (whether the content of the cache is FALSE or the entity).

In terms of performance this means switching a cache delete for a cache set on save (should be neutral), and adding a cache get just before cache sets (extra work but fast). However in the stampede situation that we're talking about, being able to skip one or two cache writes probably isn't a bad thing either (especially with the database cache).

3. Add a cache backend decorator that keeps track of cache IDs (in a class property) and then invalidates them end of request as well as always returning FALSE for them until then. This could either be in-addition to the invalidation during save or instead of it. We already did similar for cache tags in #2966607: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock. We'd need to move the delayed cache tag invalidation to end of request too probably to match.

This would result in the following sequence:

1. Empty entity cache for the entity.

2. Process A loads the entity, get a cache miss.

3. Process B saves the same entity and clears the cache.

4. Process A writes to the cache.

5. Process B invalidates the cache backend/tags at the end of the request.

I think the tombstone record could be the best option - it could only go wrong if the 'empty' cache item gets invalidated or cycled out in the window between it being created and the cache set, but otherwise it should completely eliminate the race condition without adding a lot of overhead.

I see this a lot and I had kind of traced it to the DatabaseCacheTagsChecksum counters but never got around to writing up a test of sorts to check it.

I kind of see the following in my head:

1. Empty cache for something

2. Process A gets a cache miss, starts to generate it, loading things

3. Process B changes some things that got loaded by process A, updating DatabaseCacheTagsChecksum counters

4. Process A starts to set the cache, loading current DatabaseCacheTagsChecksum counters, and saves to cache using those counters

5. Cache contains an item where the counters match the new version from B incorrectly

This doesn't apply to existing cache entries that are expired because the DatabaseCacheTagsChecksum counters for those are loaded at time of cache verification, thus if there is a subsequent edit and save it is correct as it does not reload the DatabaseCacheTagsChecksum counters during save it uses the ones that were existing at the point the cache was loaded - if any changes happened since then this new save will be ignored in cache.

It always seemed to me that when loading a cache item the DatabaseCacheTagsChecksum counters need to have been loaded already. In case of an empty cache, we don't know what cache tags are used so can't load them. So maybe it be that cache tags need to be noted at the time of loading cache in case it is empty - or perhaps during save it saves just the tags used, and not any data, if any counters were missing at the time of load. But then this implicitly requires knowledge of the load at the point of save. It feels like PSR-6 style objects would help as it could carry that state from get to set on what tags it had checksummed and properly populate the cache to ensure the next get/set can fully 100% validate the checksum without inadvertently storing the wrong checksum

I think I figured out how to reproduce and test this issue. If this approach seems reasonable, I can roll a core MR.

https://github.com/agentrickard/entity_load_delay

The module implements a hook_entity_load() that:

• Monitors entity loading operations
• Introduces a configurable delay (default: 10 seconds) when enabled
• Can target specific entity types and IDs for precise testing
• Automatically disables itself after one use to prevent interference with subsequent operations

The module includes comprehensive kernel tests that demonstrate:

• Normal entity loading behavior (baseline)
• Race condition reproduction with concurrent operations
• Stale data scenarios that affect edit forms

AI was used in creating this code.

Let's try...

We can see the failure in https://git.drupalcode.org/issue/drupal-3474843/-/jobs/11671277. I hope the test is solid enough to properly simulate concurrency

I had a short chat with @catch and, indeed, in #6 he suggested the 2nd option. I've closed the initial MR and opened !16781. Few notes:

• I've used LLM to help understand better the #6/2 suggestion and also to design the fix, but I carefully curated and checked every single byte
• Not sure this needs a CR
• This pipeline https://git.drupalcode.org/issue/drupal-3474843/-/pipelines/930902 runs only against the test and shows the bug

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Claude is suggesting to look also from the opposite direction

// core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php

protected array $uncommittedIds = [];
protected array $uncommittedRevisionIds = [];

protected function doPostSave(EntityInterface $entity, $update) {
  if ($this->database->inTransaction()) {
    if (!$this->uncommittedIds) {
      $this->database->transactionManager()
        ->addPostTransactionCallback([$this, 'rootTransactionEndCallback']);
    }
    $this->uncommittedIds[$entity->id()] = TRUE;
    if ($entity instanceof RevisionableInterface && $entity->getRevisionId()) {
      $this->uncommittedRevisionIds[$entity->getRevisionId()] = TRUE;
    }
  }

  parent::doPostSave($entity, $update);
}

public function rootTransactionEndCallback(bool $success): void {
  $this->uncommittedIds = [];
  $this->uncommittedRevisionIds = [];
}

protected function setPersistentCache($entities) {
  parent::setPersistentCache(array_diff_key($entities, $this->uncommittedIds));
}

protected function setPersistentRevisionCache(array $entities): void {
  parent::setPersistentRevisionCache(array_filter(
    $entities,
    fn ($entity) => !isset($this->uncommittedRevisionIds[$entity->getRevisionId()]),
  ));
}

I don’t believe this can be fixed in the save pathway. As soon as save finishes the bets are off as the issue is on the load side when it comes to cache issues. Maybe this is a classification of a similar issue but the issue has and always has landed on the load side.

When loading an entity there is no guarantee that the current epoch (usually defined by cache tags checksum) the cache uses is aligned, because cache tags are unknown at the time of load and so the checksum is unknown - unless a cache item is loaded (valid or not) in which case it is known. The checksum for a new cache item (empty cache or evicted) is calculated on cache save and loaded based on the current state of the database at save time - not the state at the time the entity was loaded.

I genuinely believe the only approach that is holistic is to prevent saving to cache if the checksum was unknown at load. It means either a change in load contract to specify all tags ahead of time or a Variation Cache derivative that takes optional expected tags but is able to then save only an instantly invalid stub first with the cache tags so that the next load can calculate the true checksum and then save the true cache data (assuming the tags remain the same)

It does shift from cache populating immediately to cache populating after two attempts though but with the option to pass in expected tags it could be optimised in the hot paths where at load time we know what the tags will be and can load their checksum before the entity.

Kind of impacts all caches. I had patches somewhere but I apologise I get so busy elsewhere and the impact is relatively minor for me that I haven’t got around to sharing somewhere. If interested I can dig them out.