Multiple webheads can cause infinite growth of Twig cache

This was reported earlier in #2882941: Twig cache file names in a load-balancer setup

Closed #2882941: Twig cache file names in a load-balancer setup as a duplicate.

Please credit ltimmers for his work debugging this issue and providing a patch.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigExtensionPass.php
@@ -24,7 +24,7 @@ public function process(ContainerBuilder $container) {
       // and mtime for every time we change an existing file.
-      $twig_extension_hash .= $class_name . filemtime($reflection->getFileName());
+      $twig_extension_hash .= $class_name . md5_file($reflection->getFileName());
     }
 

Unfortunately, certain governments, that we shall not name here, have some weird rules about md5 being evil, even when not used for something unrelated to cryptography.

So we'll have to use a different hashing mechanism.

We could use sha1_file() if that is acceptable? One thing that might be an issue, md5_file() / sha1_file() are significantly slower than filemtime(). Not sure how big that problem is though.

I took a look at the test failure above. What I found was that the twig_extension_hash and twig_cache_prefix values remained consistent across the test.

However, the cache file name suffix changed for request 3.

In my case, as an example

Request 1: string(59) "5efcde6a0aabe_container.html.twig_qt9-XO_ncwZG2QwJr-bcsZySQ"
Request 2: string(59) "5efcde6a0aabe_container.html.twig_qt9-XO_ncwZG2QwJr-bcsZySQ"

Request 3: string(59) "5efcde6a0aabe_container.html.twig_OyFHPs-sIZxXLEnnNhmQ-pQsh"
Request 4: string(59) "5efcde6a0aabe_container.html.twig_OyFHPs-sIZxXLEnnNhmQ-pQsh"

What I found was that the Environment::getTemplateClass() method was generating a different key value on request 3

Request 1 and 2 : core/modules/system/templates/container.html.twigcore:escaper:optimizer:sandbox:drupal_core:debug:0:7:3:1.42.5:0:\Twig\Template:0
Request 3 and 4 : core/modules/system/templates/container.html.twigcore:escaper:optimizer:sandbox:0:7:3:1.42.5:0:\Twig\Template:0

I have highlighted the difference in bold.

In this case, I believe the difference is in the values stored in 'optionsHash' in the Environment class.

Yeah, we can't re-create the twig environment by hand like that, that doesn't have the same extensions.

Here's a different approach by unsetting the service and letting it get created again by the container builder, which I think should have a similar result. Still, I'm unsure about the test, specifically the touch() part. I don't think we should expect tests to have writable core files. I get that there isn't really any other option, but this seems super risky.

Also updated the test for D9.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigExtensionPass.php
@@ -24,7 +24,7 @@ public function process(ContainerBuilder $container) {
       // and mtime for every time we change an existing file.

comment still has mtime in it and should be adjusted. Like '... and sha1 to track file changes.'

Same comment should be adjusted in the class description too.
Currently `Parameter twig_extension_hash is a hash of all extension mtimes for Twig template invalidation.`

Note
On a high-traffic site we will use patch #16 in production. We'll report back here about performance (comment #10).

CS issues still need to be handled.

#18 CS error is unrelated to this issue.

As noted in #17 we are using Berdir's patch #16 successfully in a high-traffic (300k/day visitors) site.
Regarding #10 and #11
Although we did not profile the performance degradation between filemtime() to sha1_file() with production-data. My expectation is that the cpu-time of these calls are neglectable compared to the total page-build time.
The calls are executed every request, but just 1 time per twig.extension. Probably not hundreds, but a few dozen max.
For what its worth; in a local-script where the calls are executed x-times each:

A weaker argument is that high-traffic sites now can enable caching where before they disabled it or had to clear caches regularly.

Regarding #18
The reported CS issues are not related to this patch so the should be ignored.

Regarding #19
Code comments are now in line with the actual code.

Regarding #16
I would love to move this one to RTBC., but I find it difficult to chip in what would be a better approach for the test-coverage.
Therefor keeping this issue active.

I first installed the patch #19 in Drupal 8.9 and set the twig cache folder to dump in a local environment. I did the following tests, in their correspondent order, and I confirm twig flush cache worked correctly in both cases.

1. Loaded twig cache content, executed twig flush cache and checked twig cache folder. The patch still clear all the cached content in the local folder after execute it.

2. Ran the unit test provided by the patch in Drupal\KernelTests\Core\Theme\TwigEnvironmentTest::testTwigFilePrefixChange, and it passed.

Based on these results, I could move the ticket to RBTC, but I consider this should be tested in a real multi webhead environment before confirming patch #19 fixes the issue.

Moving this ticket to Needs review.

I've applied the patch on a production environment with 2 webservers and 1 loadbalancer and the creation of indefinite twig cache files is fixed! Clearing the caches in the backend and in drush is working like a charm again.

Thank you for all the work!
I'll put it on RTBC because it's tested on a real production environment like requested in the comment before.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigExtensionPass.php
@@ -23,8 +23,8 @@ public function process(ContainerBuilder $container) {
-      // and mtime for every time we change an existing file.
-      $twig_extension_hash .= $class_name . filemtime($reflection->getFileName());
+      // and sha1 for every time we change an existing file.
+      $twig_extension_hash .= $class_name . sha1_file($reflection->getFileName());
     }

This is using a weak cryptographic hash for non-cryptographic purposes. I didn't really like the change at the time, and still don't, but a long time ago in core we stopped doing that due to the number of false-positives from automated scanning software.

We should use hash_file() with crc32 instead (see for example DrupalKernel::boot()).

Is this normal bug somehow a duplicate of the major task #2979669: Follow-up for #2752961: automatically deleting compiled Twig templates across multiple webheads?

We seem to have run into this bug on a client site. Will try to review next week. Tagging to be smashed. 😉

We are seeing consumption of up to 1G of disk space to this every 10 minutes on some of the webheads.
Recommending the patch to the customer's dev team.

I have reworked the patch to use hash_file() and crc32 as @catch recommended

attempting to fix the testing error

#40 solved the issue we encountered when a hoster delivered our website manually on two servers. Since the timestamp were different, we had ~30go of twig cache files, patch solved the issue. Many thanks !

Because I see a crc32 implementation in this patch, and although this requires PHP 8.1, I want to highlight that there are new and faster non-cryptographic hash algorithms in PHP.

Whether these are faster in this use case must be tested.

If it's a 9+ patch makes sense to keep the current hashing, maybe a followup for the new functions available for D10+?

xxHash has an issue at #3307718: Implement xxHash for non-cryptographic use-cases.

Uploading a test-only patch. If that fails OK I agree this one is ready.

Errr it can stay RTBC though!

Fail patch came back properly.

Committed/pushed to 10.1.x, cherry-picked back through to 9.4.x, thanks!