Result cache is not invalidated after DI container dump

[Wirone]

Imagine scenario:

• With bin/console cache:clear you dump DI container
• In code you use $container->get('foo');
• PHPStan reports Service "foo" is not registered in the container
• You fix DI definition by adding foo service
• cache:clear again, so DI definition contains foo service
• Because of result cache PHPStan reports errors even though DI container definition has changed and service is registered

Expected result: refreshing DI container invalidates result cache for files that uses DI (have errors reported by Symfony extension).

[Wirone]

Yesterday we had production incident because of this issue. Here's what happened:

• I've prepared changes around bin/console entrypoint, merge request contained mostly YAML changes (DI definition), but also in src/Kernel.php and one of the compiler passes registered there. Almost none PHP changes, all of them were low-level (files not used by other files).
• CI pipeline for the merge request was green, because my changes did not invalidate the result cache, which was reused
  
• As it turned out later after deployment, there were places in the code that directly fetched some service from the DI container, but my MR removed that service so when the code landed on production it started to throw an exception which resulted in reporting issues to our Sentry. The reason why it wasn't caught before is that all of the invalid DI fetches were in places not covered with tests (like cron entrypoints etc), but that doesn't matter in this case - just a warning for others.
• The issue was fixed, merged to main branch, deployed and synchronised with development branch, PHPStan jobs in pipelines were still green. It randomly started to report "Ignored error pattern ... was not matched in reported errors." during consequent deployment the next day after problem was initially introduced, after several green pipelines during that time.

This is not how I would expect PHPStan to work and take care of quality of our app. Please @ondrejmirtes consider looking at this, as this problem affects one of your sponsors (GetResponse) 🙂🙏.

[ondrejmirtes]

Would this problem be solved by adding the hash of the containerXmlPath file to the result cache so that it gets invalidated when it changes?

[Wirone]

I am not sure, but this is something I was thinking about too when I considered workaround on our side - to store some hash from DI-related files (did not consider containerXmlPath though, but it looks much better as it's the final result of the cache compilation). However, I am not sure whether XML file is always generated in the same way for the same set of services (moving entries representing the same content), maybe it needs some normalisations (sort by service name?). FYI in our case XML is almost 4MB.

[ondrejmirtes]

I don't know enough about this problem space to solve it myself.

[ruudk]

However, I am not sure whether XML file is always generated in the same way for the same set of services (moving entries representing the same content), maybe it needs some normalisations (sort by service name?). FYI in our case XML is almost 4MB.

The XML only regenerates when the container is compiled. So it should not change when the container does not change.

[ruudk]

Would this problem be solved by adding the hash of the containerXmlPath file to the result cache so that it gets invalidated when it changes?

Is there an easy way to attach the hash of that file to the PHPStan cache?

[Wirone]

The XML only regenerates when the container is compiled. So it should not change when the container does not change.

I know, I am just wondering if it's always in the same order or compiling may lead to

<?xml version="1.0" encoding="utf-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
  <parameters><!-- ... --></parameters>
  <services>
    <service id="Foo" class="App\Foo" public="true"/>
    <service id="Bar" class="App\Bar" public="true"/>
  </services>
</container>

vs

<?xml version="1.0" encoding="utf-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
  <parameters><!-- ... --></parameters>
  <services>
    <service id="Bar" class="App\Bar" public="true"/>
    <service id="Foo" class="App\Foo" public="true"/>
  </services>
</container>

which may result in different hash, but technically represents exactly the same DI container. @nicolas-grekas sorry for pinging, but you may probably know something and shed some light 🙂.

Anyway, ideal scenario would be to invalidate PHPStan's result cache if DI-related files were changed and even if cache:clear wasn't executed, but I believe it's pretty hard as container's definition is merged from many different files (YAML/XML/PHP configs, PHP attributes in actual code etc). So the MVP would be an invalidation based on dumped XML container, and then it's easy to enforce rebuilding cache before running PHPStan (we have phpstan:check Composer script, it can be transparently converted into a group of commands).

[ondrejmirtes]

@ruudk We'd need a new interface and container tag, something like ResultCacheMetaExtension. It'd have two methods getKey() and getHash().

We'd call these extensions from https://github.com/phpstan/phpstan-src/blob/af6a9c5f1e34bb11e2b5c9f85ba46ca318bca426/src/Analyser/ResultCache/ResultCacheManager.php#L877-L918.

It's doable on 1.12.x

[ruudk]

I know, I am just wondering if it's always in the same order or compiling may lead to

Does it matter? If the order in the XML was changed, the container was compiled again. In that case, PHPStan should bust the cache.

But I would expect Symfony to normalize the services when building the container, so that it's predictable.

[ComiR]

Would it be possible to only invalidate the cache if relevant parts of the file have changed? I.e. invalidate the cache only if the services section changed. Changes in other sections (e.g. parameters) wouldn't affect the result (at least as of now, rules added in the future might change this).

If it would complicate this feature too much, keep it simple and only use the file hash. Better to over-invalidate here imho (shouldn't happen that often anyway).

[ruudk]

Would it be possible to only invalidate the cache if relevant parts of the file have changed?

No. And if, this should be a task for Symfony DI component. It currently is very quickly to say "let's recompile" when any of the resources changed (file m time).

[Wirone]

Does it matter? If the order in the XML was changed, the container was compiled again. In that case, PHPStan should bust the cache.

@ruudk It does matter if your analysis takes half an hour for each MR when result cache from main branch isn't reused because of false negative hash check 😅. We have large runners for main branch which saves result cache in Gitlab's cache, for merge requests there is a PHPStan job on regular runner (less CPUs) that only reads cache, so it reuses PHPStan's result cache when possible, but does not write new one. If DI container's content would invalidate the result cache too often (when there are not any actual changes), it would waste much of our CI resources.

But I would expect Symfony to normalize the services when building the container, so that it's predictable.

Yes, that would be the best.

Would it be possible to only invalidate the cache if relevant parts of the file have changed? I.e. invalidate the cache only if the services section changed. Changes in other sections (e.g. parameters) wouldn't affect the result (at least as of now, rules added in the future might change this).

@ComiR I believe it's not a good idea, because fetching undefined param in your code also can lead to an error. IMHO invalidation should happen if there's any actual difference in container's definition, which means:

• added/removed params/services (rename is technically a deletion+addition)
• changed services' arguments

[ruudk]

It does matter, ... , it would waste much of our CI resources

Yes, true. I wouldn't be happy with that either on CI.

I looked at the container XML and it seems that this thing is already immutable. The XmlDumper does not do any sorting. It dumps the definitions as how they were added to the container builder. And it must be that Symfony always uses the same order of loading files / config / autowire, otherwise we would have seen many other bugs already. So that means that the output of the XML is always as it should be. Dynamic things like build times / hashes are not included in the XML, which is good.

I compared the XML before and after doing a cache:clear, and it's identical.

[ComiR]

@ComiR I believe it's not a good idea, because fetching undefined param in your code also can lead to an error. IMHO invalidation should happen if there's any actual difference in container's definition, which means:

• added/removed params/services (rename is technically a deletion+addition)
• changed services' arguments

@Wirone The rules only check for private or missing services. But I forgot about the extensions that e.g. provide the return types for ContainerInterface::getParameter(), those are using the parameters section of course.

So I don't know if there would be any benefit in checking the file for relevant changes or if it would suffice to simply use the file hash. You'd first have to find out which changes in the file would be irrelevant (e.g. the order of services, maybe the actual values of parameters, ...?) and then ignoring those.

I guess it would be best to split this. For now invalidate caches when the file hash changes to fix the caches never being invalidated (seems like a bug to me) and later add a feature that will improve caching by checking for relevant changes only.