entity.last_installed_schema.repository has a race condition

Quick issue review. The issue description is mentioning the setter, but the patch is changing the getter.

We should also target 11.x and backport to 10.x

+++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
@@ -154,4 +154,17 @@ public function deleteLastInstalledFieldStorageDefinition(FieldStorageDefinition
+  /**
...
+   *   Entity type ID (for example node).
+   *
+   * @return \Drupal\Core\Field\FieldStorageDefinitionInterface[]
+   *   The array of installed field storage definitions for the entity type,
+   *   keyed by field name.
+   */

This PHPDoc is not complete. Missing an function description (first line)

+++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
@@ -154,4 +154,17 @@ public function deleteLastInstalledFieldStorageDefinition(FieldStorageDefinition
+  protected function uncachedGetLastInstalledFieldStorageDefinitions(string $entity_type_id): mixed {

Return type mixed? The PHPDoc only shows returning an array of storage definitions.

Fixed the phpdoc and the return type.

The patch factors out the uncached getter from the current getter and makes the setter use it instead of the current getter.

Thanks Charlie! Patch looks better now. Will leave it for NR so one of the core committers also can pick in.

Can we get a test case showing the bug

Thanks.

Steps to reproduce
Way too hard. But people can check whether they are affected by running the following with drush

I guess, if it's such a race condition it might be difficult 😅

Can we get a test case showing the bug

Sorry but no. I do not know how to write a meaningful test case for this. You need two processes running in parallel. I could simulate it with mocks but at that point it's a completely meaningless test because of the sea of mocking required. We do have many such in core, I am loath to add more.

Following the example of #3312001: Race condition with automatic deploy steps on ConfigImporter → I added more comments and I am removing the needs test tag. The enormous difficulty of writing one vs the trivial change here just not worth it.

Except I put the comment before the wrong line... out of two didn't manage to hit the right one. SIgh.

I am noting there's still a race chance here with two field storages being installed at the very same time: field1 reads, fields2 reads, field1 writes, field2 writes but this window is really really tiny against a data structure rarely written. We can decide to protect against this one as well by taking out a lock.

Should we be adding a lock here to make this multi process safe?

Maybe, it's just much messier core process wise to pass in a new argument especially because this being a bug this will need to be backported all the way to 9.x which has a deprecated parameter pass already and this race is much less likely than the one originally described in the issue. The original race merely requires an ordinary request to race with a field install one, this is between two field installs. But technically, to be super correct, yes. Note this lock doesn't mean the original race is solved with it -- we do not want to add even observing the lock to get, that's an unnecessary slowdown when the original race is solved with this rather simple change to set. Yes, set becomes marginally slower but who cares about set speed.

I really should read my patches before I upload the, not after :)

+++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
@@ -41,9 +49,14 @@ class EntityLastInstalledSchemaRepository implements EntityLastInstalledSchemaRe
+    $this->lock = $lock;

Should be lockBackend?

It totally should be, that's what phpstan was complaining about too.

1. +++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
   @@ -26,6 +27,13 @@ class EntityLastInstalledSchemaRepository implements EntityLastInstalledSchemaRe
   +   * @var \Drupal\Core\Lock\LockBackendInterface
   ...
   +  protected $lockBackend;
   

   Please use type for the new property

2. +++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
   @@ -41,9 +49,14 @@ class EntityLastInstalledSchemaRepository implements EntityLastInstalledSchemaRe
   -  public function __construct(KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache) {
   +  public function __construct(KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache, LockBackendInterface $lock) {
   

   New argument should be optional as serialized container will carry old definition before running updates

+++ b/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php
@@ -41,9 +49,14 @@ class EntityLastInstalledSchemaRepository implements EntityLastInstalledSchemaRe
    * @param \Drupal\Core\Cache\CacheBackendInterface $cache
    *   The cache backend.
    */

Just add $lock to the PHPDoc with the proper type and description and it should pass the first stage :-)

Y'all understand this drives contributors away, right?

   * @param \Drupal\Core\Lock\LockBackendInterface $lock
   *   The lock backend.

This has negative value. It adds absolutely nothing, the interface already tells you what it is and it makes the class harder to read. I'd rather add a phpcs bypass than this.

But here it is, hoping for the day when we can stop cluttering our patches with this. Is there a policy issue already not to add doxygen if the type already tells all there is?

You know what, this is not a great patch, this is not a visionary solution.

Why are we using a serialized blob here.

If we were writing one database row per field to key-value , none of this would occur.

We could still cache the information together after a single getAll. There's no discussion on this in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code → .

What do you think.

Fyi #3238192: Allow omitting @var for strictly typed class properties → - we're just blocked on governance issues I think, something to do with the TWG not having active members

Someone should file an issue to replace the meat of getLastInstalledDefinitions() with

    $this->entityTypeDefinitions = [];
    // Filter out field storage definitions and ensure that the returned array
    // is keyed by the entity type ID.
    foreach ($all_definitions as $key => $definition) {
      $parts = explode('.', $key);
      if (array_pop($parts) === 'entity_type') {
        $this->entityTypeDefinitions[$parts[0]] = $definition;
      }
    }

because IMO it's much easier to read and only iterates the array once.

But anyways.

What about the attached patch. Needs a healthy dose of an update function to unserialize, of course. And maybe still a lock this time in setLastInstalledFieldStorageDefinitions but I am not so worried about that one, all hell already breaks lose if a racing process starts just in time for an entity type to exist but EntityTypeListener has not yet called setLastInstalledFieldStorageDefinitions because then the cache will store empty. I am not even sure that's in scope for this issue.

Nice, that's a step forward but we also need to drop @param.

Anyways,

The last submitted patch, 28: 3363732_28.patch, failed testing. View results →

Oh yeah, update tests are unhappy.

The first failure is from 📌 UpdatePathTestBase saves the root user before updates have run Needs work but fixing that I found drawing the update page calls hook_theme which calls views_theme which requires a working entity field API. Then there are broken tests which presume entity field API is working, one such is 🐛 BlockContentRemoveConstraint presumes a working entity field API Active I provided something that makes that one pass.

I really can't emphasize this enough: as far as I am able to tell, all the failures above are not from this patch, it just sheds light on already broken functionality.

Whoever wants to bother with the misery core development became can continue, I am unsubscribing from this issue.

Here's fix

The last submitted patch, 33: 3363732-33.patch, failed testing. View results →

In Drupal version 11.x, I have successfully tested patch #33 in my local environment, and it is functioning as expected. However, I believe that my review may not be sufficient to ensure all aspects are covered. Therefore, I recommend moving this patch to the "Needs Review" status so that it can receive further evaluation from the community.

Thank you!