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

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

Ok, got a reproducible minimal testcase.

Note that it's only a 100x test run.
When used with bigger repeat numbers, changes are high that it won't make it in the allotted 1:10 hours time-out period.
I've seen testruns waiting for somewhere between 3 and 20 minutes before returning a single test-result after the first 10 minutes or so.

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.

Well, that's one theory shot down...

Incorrect patch, ignore this one.

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, 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 #2806009: Installing a module causes translations to be overwritten

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. (In a separate issue I tried to run only the InstallerExistingConfig[SyncDirectory]MultilingualTest tests 500x, but that didn't cause any test-failures. Only with the addition of (at least) UmamiMultilingualInstallTest things break.)
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) , and also I would expect the same results with SF DI on 6.2 an 6.3 (See #12 and #15).

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 #2806009: Installing a module causes translations to be overwritten just brought it to our attention.
But I do find it weird that downgrading SF DI makes the failure (seemingly) go away.

EDIT: Added a few more test-runs on #15 to be (a bit more) sure that it is indeed green and SF 6.3 DI is a suspect.

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

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.

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?

The problem seems to exist (sometimes) in 9.5.x already, as described by @alexpott in #3294293: The target storage in the StorageComparer can get out-of-sync with the container during installation from config.

I think that issue in combination with SF6.3 just exposes the root cause described, before the commit there we didn't have test-coverage for it.
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 #3294293: The target storage in the StorageComparer can get out-of-sync with the container during installation from config 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.
I'm planning to look into that later today

Let's see if reverting #2806009: Installing a module causes translations to be overwritten 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 #2806009: Installing a module causes translations to be overwritten 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 #3362306: Skip InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync to skip both tests whilst we sort things out in here.
Bumping this one to critical per @catch in #29.

Whoops, posted patch in wrong issue.

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 for a decorated service 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 for a decorated service 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. Probably serialization?

Hiding outdated patch

Filed upgrade issue #3363222: Update to Symfony 6.3

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?

Patch #43 was a SF6.3 re-based version of #38, 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 "kills" our canary-trio (Don't tell the RSPCA).

Patch #48 is a reroll of #37 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 knowledge 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, #3364659: Give display modes a description 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 "Witching 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.

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 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 #3364659: Give display modes a description, 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 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?