[random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync

@kim.pepper also reported this in a patch run at https://www.drupal.org/pift-ci-job/2666815 →

Just seen this on https://www.drupal.org/pift-ci-job/2671868 → - seems frequent enough.

Ok, got a reproducible minimal testcase.

Problem comes from symfony/dependency-injection.

Attached patch changes _only_ symfony/dependency-injection back to ^6.2, all other symfony/* dependencies are left at ^6.3 and should pass.

To prove that it is indeed changes in symfony/dependency-injection that is causing us problems, we want to have a clean test-run with everything symfony/* on ^6.3 _except_ symfony/dependency-injection and its dependencies, those should be on ^6.2.

To get symfony/dependency-injection on ^6.2, we need its dependency symfony/http-kernel on ^6.2 as well.

To prove that this isn't causing our problems we need a faiing testrun with everything symfony/* on ^6.3 _except_ symfony/http-kernel, which should be on ^6.2.
If that is faling, we can test with both symfony/dependency-injection and symfony/http-kernel on ^6.2 and see if that is clean.

So let's start with symfony/http-kernel downgraded to ^6.2.

Oh, we also want to prove that SF6.3 is the root cause, so let's do a 250x run (which should make it in the 1:10hr time-out period) where we downgrade symfony/* to ^6.2.

We already have a test failure in #12 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed , so we can continue with trying to get clean test-run with everything symfony/* on ^6.3 _except_ symfony/dependency-injection and its dependency symfony/http-kernel, those should be on ^6.2.

If SF DI is the cause, then what can cause the language entity removal in this tests? Moreover, maybe the issue in environment?

    The website encountered an unexpected error. Please try again later.<br><em class="placeholder">Drupal\language\Exception\DeleteDefaultLanguageException</em>: Can not delete the default language in <em class="placeholder">Drupal\language\Entity\ConfigurableLanguage::preDelete()</em> (line <em class="placeholder">177</em> of <em class="placeholder">core/modules/language/src/Entity/ConfigurableLanguage.php</em>). <pre class="backtrace">Drupal\language\Entity\ConfigurableLanguage::preDelete(Object, Array) (Line: 450)
Drupal\Core\Entity\EntityStorageBase-&gt;delete(Array) (Line: 347)
Drupal\Core\Entity\EntityBase-&gt;delete() (Line: 950)
Drupal\Core\Config\ConfigImporter-&gt;checkOp('language.es', 'create', 'language.entity.en') (Line: 662)
Drupal\Core\Config\ConfigImporter-&gt;processConfigurations(Array) (Line: 561)

It means core fails importing translation from ES collection - label for English

$ cat language/es/language.entity.en.yml 
label: Inglés

It sounds more related to 🐛 Installing a module causes translations to be overwritten Fixed

The only way this could happen is when $this->storageComparer->getTargetStorage($collection)->exists($name) at https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... returns TRUE.

In that case the tests will want to delete the en-language, which is the primary language, which throws the exception that causes the test-failure.

AFAICT, this can only happen when another test, with the same $collection already set the en-language.
Which would in this case be the UmamiMultilingualInstallTest test.
Which would also prove we have leakage of the \Drupal\Core\Config\CachedStorage between tests.

Also, i8n my mind: If it would be an issue in the environment, it would also happen in 10.0.x and 11.x with downgrade to SF 6.2 would fail (See #13 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed ) , and also I would expect the same results with SF DI on 6.2 an 6.3 (See #12 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed and #15 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed ).

Hope that makes sense?

So with only the change symfony/dependency-injection to SF6.2 between #12 and #15 we have the feared test-failure.

Happy to be convinced that the error is elsewhere. I was originally only here to create a reproducible test failure, so my work is done :)
Stepping away now.

Which would also prove we have leakage of the \Drupal\Core\Config\CachedStorage between tests.

It looks like it

Meantime it still not clear why creation of translation deleting the entity checkOp('language.es', 'create', 'language.entity.en') in error logs (which is clear from the function) - maybe it needs to add if ($target_exists & $collection !== '')?

Meantime it still not clear why creation of translation deleting the entity checkOp('language.es', 'create', 'language.entity.en') in error logs (which is clear from the function)

Yeah, I'm not the greatest at explanations...

What I meant to say is that the only way we can reach the deletion is if there's already "something" that created it before.
Since the error wasn't there before, it doesn't seem to come from the test that throws it. Hence my "leak-theory".

maybe it needs to add if ($target_exists & $collection !== '')

That could help, but doesn't explain (at least to my tiny mind) why this is a random failure and not always there.
Also _if_ we have leaking between tests, that would be just fighting the symptoms instead of the underlying problem.

The leakage might have already been there before SF6.3, and 🐛 Installing a module causes translations to be overwritten Fixed just brought it to our attention.
But I do find it weird that downgrading SF DI makes the failure (seemingly) go away.

A slack conversation with @andypost and @alexpott pointed to a related issue: 🐛 The target storage in the StorageComparer can get out-of-sync with the container during installation from config Needs work .

Just for completeness, the patch in #3294293 won't save us here, since it comes too late in the import process for our use case.

I read the diff between 6.2 and 6.3 of the Symfony DI component but nothing jumps out, however the code is quite dense and I assume I missed something small but significant.

Should we try reverting 🐛 Installing a module causes translations to be overwritten Fixed in this issue to see if it's somehow a combination of that change and Symfony 6.3?

Should we try reverting #2806009: Installing a module causes translations to be overwritten in this issue to see if it's somehow a combination of that change and Symfony 6.3?

I _think_ that issue in combination with SF6.3 just exposes the root cause described in 🐛 The target storage in the StorageComparer can get out-of-sync with the container during installation from config Needs work .
Reverting it will remove the symptom (so the test failure), but doesn't solve it.

If it breaks TestBot too often it's certainly an option, but the current situation (at least) gives us a change to (randomly) find/solve the root cause, although that could be done in this issue with an applied 🐛 The target storage in the StorageComparer can get out-of-sync with the container during installation from config Needs work if we decide to do the rollback-rumba on that one.

Ideally (one can hope...) we find/create a non-random test-fail with a new, yet to build test somehow.

Let's see if reverting 🐛 Installing a module causes translations to be overwritten Fixed gets rid of our random failure.

Ok CI aborted due to time-out (seems like this whole tar-reading business is performance-wise highly dependent on traffic and/or test-runner instance), but ~240 times no failure is good enough for me.

Reverting 🐛 Installing a module causes translations to be overwritten Fixed would get rid of the error (symptoms).

Should we skip the test and keep this open as a critical issue to un-skip it?

Should we skip the test and keep this open as a critical issue to un-skip it?

Never a big fan of palying the "Skip-Test Card", IMHO it's very easy to skip a test and then nobody really cares anymore, since there's no immediate urge of fixing things anymore.

But also seeing the complexity of this (at least for me) and the (very?) frequent occurrence of this test-failure, we might have no other option than to skip both InstallerExistingConfigMultilingualTest::testConfigSync and InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync for now.

Opened 📌 Skip InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed to skip both tests whilst we sort things out in here.
Bumping this one to critical per @catch in #29 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed .

Symfony 6.3 rc2 reverts a change in the DI component, and there is a definite possibility this is our root cause: https://github.com/symfony/symfony/pull/50440

Here's update to SF 6.3RC2 (needs new issue probably) and revert from #32

_Maybe_ it fixed InstallerExistingConfigMultilingualTest::testConfigSync, but in a test run elsewhere, I still get random failures in InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync.

Let's also put that run in here.

On a more positive note (Is there a postive when you're looking for failures?) I think I've found the code that gives us woes.

Looks like this commit: https://github.com/symfony/symfony/commit/1ac07d3d381740c3fd779a83d4ac04... is somehow the culprit.

If we don't add the 'container.decorator' tag on $definitions, as in the attached patch (see also https://github.com/symfony/dependency-injection/compare/6.3...Boegie:dep...), everything seems to be going swell.

Let's give that a run in here.
(If not anything else, I might take the longest-patch-name-award...)

Since https://github.com/symfony/symfony/commit/1ac07d3d381740c3fd779a83d4ac04... seems to be used only for debug purposes, it seems unlikely that the specific tag name is the problem.

That would imply that adding _any_ tag on $definitions will (randomly) break our canary-in-a-cage trio of tests.

Let's prove that here. See https://github.com/symfony/dependency-injection/compare/6.3...Boegie:dep...

This is the moment I need people who actually have a brain, because this is getting way above my (limited) SF-knowledge.

I _think_ I've proven that the addition of any tag on a \Symfony\Component\DependencyInjection\Definition somehow breaks our sniffing canary trio of tests (\Drupal\FunctionalTests\Installer\InstallerExistingConfigMultilingualTest, \Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest and \Drupal\Tests\demo_umami\Functional\UmamiMultilingualInstallTest), but have no clue where/what/why.

Hiding outdated patch

Filed upgrade issue 📌 Update to Symfony 6.3 Fixed

Don't have any idea about #39 yet, I think we need to file an upstream issue if we still have failures.

However for now uploading a patch just to get a new test run now we're on 6.3.0

I think we need to file an upstream issue if we still have failures.

Pretty sure we will see the same (random) failure with SF6.3

Also seeing that it's random and not exactly knowing/seeing what could possible break with adding any tag on the definiton of a decorated service makes it hard to create a "meaningfull" issue for symfony/dependency-injection.

I think it's OK to file an issue with only vague steps to reproduce - we can point to the weakref revert and ask if there are any similar recent changes we should look at.

Thanks for uploading a patch that actually applies.

Or reading back, it looks like you've already tracked this down to #3361121-37: [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync → - at least as something that removes the random test failures if not the root cause of the bug?

The last submitted patch, 43: 200x-InstallerExistingConfigSync[Directory]MultilingualTest-UmamiMultilingualInstallTest-no-tag-container.decorator.patch, failed testing. View results →

Patch #43 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed was a SF6.3 re-based version of #38 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed , which added a tag with a different name.

For my tiny mind, that proves that adding any tag on the definiton of a decorated service will randomly break our 3 canary tests.

Attached patch is not adding the 'container.decorator' tag (in fact no tag at all), and should _not_ fail.

Or reading back, it looks like you've already tracked this down to #3361121-37: [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync - at least as something that removes the random test failures if not the root cause of the bug?

Yes, I think I've proven that this commit: https://github.com/symfony/symfony/commit/1ac07d3d381740c3fd779a83d4ac04...
and more precisize this line https://github.com/symfony/dependency-injection/blob/6.3/Compiler/Decora... randomly breaks our canary (Don't tell the RSPCA).

Patch #48 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed is a reroll of #37 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed on SF6.3

I assumed having (possibly?) pinpointed down the root-cause, it would be simple enough to see where Drupal goes "wrong", but the randomness and my lack of deep SF vs Drupal-container makes that hard, at the very least for me.

Will the newly release https://github.com/symfony/symfony/releases/tag/v6.3.2 help us out here?

Right....

Since we disabled the two multilangual tests, 📌 Give display modes a description Fixed landed.
Without these test enabled nobody noticed core/tests/fixtures/config_install/multilingual.tar.gz also needed updating.

Let's ignore that for now by just adding the changed config as...erm...changed config.

Hmmm, promising lack of dead canaries at 200x.
Will this hold with a 750x run?

(1000x run would be just over the 60 minutes run, which is the "Witches Hour", where tests in a repeat loop are known to get wonky.)

Wow....

That didn't scale at all.

I've been playing around with this in a separate issue, and have found that the random error seemed to have disappeared since we've last seen it, testing with 6.3RC2 in #36 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed .

Also found out that the "canary-patch" with the 3 install-tests doesn't really scale above 250x.

So, with all of the above taken into account, let's:

- Run the canary-patch with enabled tests on the current code base.
- Run it in a 250x run until we have around 8500 runs.
- To get the patch to pass we need to updat core/tests/fixtures/config_install/multilingual.tar.gz. I added description: '' to
core.entity_view_mode.user.full.yml
core.entity_form_mode.user.register.yml
core.entity_view_mode.user.compact.yml
core.entity_view_mode.node.full.yml
core.entity_view_mode.node.rss.yml
core.entity_view_mode.node.search_index.yml
core.entity_view_mode.node.search_result.yml
core.entity_view_mode.node.teaser.yml

So, in #55 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed we have 40 x 250 runs = 10.000 runs of a patch that re-enables the two disabled tests (InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync and InstallerExistingConfigMultilingualTest::testConfigSync).

It also fixes core/tests/fixtures/config_install/multilingual.tar.gz for 📌 Give display modes a description Fixed , so the two re-enables tests won't fail on that.

Out those 10.000 runs there are 18 failures, all of which are:

Drupal\Tests\demo_umami\Functional\UmamiMultilingualInstallTest::testUmami
GuzzleHttp\Exception\ConnectException: cURL error 52: Empty reply from server (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://php-apache-jenkins-drupal-patches-196424/subdirectory/

/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:210
/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:158
/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:110
/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlHandler.php:47
/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/Proxy.php:28
/var/www/html/vendor/guzzlehttp/guzzle/src/Handler/Proxy.php:48
/var/www/html/core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php:174
/var/www/html/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:29
/var/www/html/vendor/guzzlehttp/guzzle/src/PrepareBodyMiddleware.php:35
/var/www/html/vendor/guzzlehttp/guzzle/src/Middleware.php:38
/var/www/html/vendor/guzzlehttp/guzzle/src/RedirectMiddleware.php:55
/var/www/html/vendor/guzzlehttp/guzzle/src/Middleware.php:66
/var/www/html/vendor/guzzlehttp/guzzle/src/HandlerStack.php:75
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:333
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:169
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:189
/var/www/html/core/tests/Drupal/Tests/DrupalTestBrowser.php:137
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:403
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:603
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:421
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:603
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:421
/var/www/html/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:111
/var/www/html/vendor/behat/mink/src/Session.php:148
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:237
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:443
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:244
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:443
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:244
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:443
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:244
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:443
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:244
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:443
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:106
/var/www/html/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:274
/var/www/html/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:175
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

That is certainly not the random error we started out this issue with.

On top of that, in #56 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed we ran UmamiMultilingualInstallTest 1500x in isolation.
That showed the above error occuring there as well, for me proofing that this error is unrelated to the original random failure and is most probably related to running this test multiple times in one patch.

All of the above proves (at least to me) that the original random failure has either dissapeared somewhere between Symfony 6.3RC2 and 6.3, or doesn't get picked up by our canary-patch anymore.

Since the latter seems very unlikely, I think it's safe to enable the two disabled tests again.
The fact that since the time of disabling and now, the needed fix on multilingual.tar.gz shows that we're missing out on some/a lot of multilangual config install test coverage makes the re-enabling even more pressing.

And here's the patch that re-enables the two tests and fixes multilingual.tar.gz.

Reading #57 would agree with the results. 18 of 10,000 isn't bad haha

Should a follow up be updated for the error though that was triggered 18 times?

The "empty reply from server" is probably specific to these test runs; we don't normally do so many repeat full installs with config in parallel, and I guess this puts even more load on DrupalCI than a normal full set of test runs.

I don't pretend to understand the container.decorator code at all, but there have been a small number of changes around this in Symfony 6.3 releases so perhaps something low level has been fixed. The test results above are promising enough for me to commit this so we get multilingual config install testing re-enabled sooner rather than later.

Committed 99636a7 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.