Follow-up for #2752961: automatically deleting compiled Twig templates across multiple webheads

This seems less like a feature to me, and more like necessary clean-up, unless I'm wrong? Otherwise, any Drupal 8 site in a multi-headed environment (which is probably between many and most of them, given D8's suitability for "ambitious digital experiences") is going to hit this, unless their hosting platform is somehow handling this for them behind the scenes (which introduces logic to said platform that's highly specialized to this one application, so definitely not a guarantee).

The current implementation seems to contain a race condition:

  public function invalidate() {
    PhpStorageFactory::get('twig')->deleteAll();
    $this->templateClasses = [];
    $this->loadedTemplates = [];
    $this->state->delete(static::CACHE_PREFIX_METADATA_KEY);
  }

This code starts deleting files without a lock and before changing the key. I believe rendering processes may have cached templates deleted while they are in the process of reading them or may regenerate them in the now expired key.

There is this rather old todo about the general problem beyond just Twig (thanks Wim for pointing this out):

 * @todo Add a global lock to ensure that caches are not primed in concurrent
 * requests.
 */
function drupal_flush_all_caches() {

I believe this race impacts any Drupal installation, not just installations using multiple webheads. Cache clear can be called by a parallel request or by CLI drush even on a single server.

Proposal

I'm proposing a lockless solution so that Twig cache invalidation does not require locking against rendering processes.

Avoid race conditions against rendering PHP requests by not deleting cached templates while invalidating the “twigCachePrefix”.

Change TwigEnvironment::invalidate() to not delete cached templates by removing this line:

PhpStorageFactory::get('twig')->deleteAll();

Allow asynchronous cleanup of old cached templates.

Change TwigEnvironment::invalidate() to set last invalidation time in state:

$this->state->set(static::CACHE_LAST_INVALIDATE_TIMESTAMP_KEY, time());

Add and implement PhpStorageInterface::->deleteAllOlderThan($time)

Create TwigEnvironment::cleanInvalidatedCacheTemplates():

public function cleanInvalidatedCacheTemplates() {
    $invalidateTime = $this->state->get(static::CACHE_LAST_INVALIDATE_TIMESTAMP_KEY);
    PhpStorageFactory::get('twig')->deleteAllOlderThan($invalidateTime);
}

Automatically cleanup old cached templates in cron.

Create (or add to existing) hook_cron() to call TwigEnvironment::cleanInvalidatedCacheTemplates().

Create or document a pattern for calling TwigEnvironment::cleanInvalidatedCacheTemplates() on every webnode for hosts that do not wish to store Twig cache on a shared filesystem.

Thanks @glennpratt for the proposal, and sorry for the delay on the feedback in my end.

I'm wondering if we are concerned at all if there are sites that doesn't have cron configured and as consequence would end up having their disks filled with cache garbage.

From the point of view of a subsystem maintainer, I'm happy with the proposal in #4. Let's update the issue summary to include that. Would be also awesome if we could get a review from someone who has more in-depth knowledge about caching.

I think it could help if twig used cache tags as mentioned in https://www.drupal.org/project/drupal/issues/2975479 or if twig cache files were put in a completely different directory each time they are cleared as mentioned above, so that the base twig cache path was something like 'php/twig/{hash}/{filename}' rather than 'php/twig/{hash}_{filename}' and twig creates cache subfolders in there where are all the subfolder names are prefixed with the hash.

Another downside to the current core code is that PhpStorageFactory::get('twig')->deleteAll(); deletes everything, including active/valid twig cache.

Throwing this out for discussion -- to delete only old/invalid cache files, something like this could be added to \Drupal\Core\Template\TwigEnvironment, or used in a service decorator, or with getTwigCachePrefix() being a public method this could be tweaked to ependency inject the twig service (`\Drupal::service('twig')`) in a custom class or drush command:

  /**
   * Deletes invalidated Twig cache files.
   */
  public function deleteInvalidatedFiles() {
    if ($prefix = $this->getTwigCachePrefix()) {
      /** @var \Drupal\Component\PhpStorage\PhpStorageInterface $twig_storage */
      $twig_storage = PhpStorageFactory::get('twig');
      $file_paths = $twig_storage->listAll();
      $invalidated_file_paths = preg_grep('/^' . $prefix . '/', $file_paths, PREG_GREP_INVERT);
      foreach ($invalidated_file_paths as $invalidated_file_path) {
        $status = $twig_storage->delete($invalidated_file_path);
      }
    }
  }

It would be easier to check 'php/twig/{hash}/' for a match if the twig files were put in a new base folder each time the hash changed.

This could serve as an interim fix to alleviate the issue where twig cache files are not invalidated across load balanced web servers. Although that would require a deploy hook, or a scheduled/cron job run against each server on a routine basis. Or maybe offer it as an option to enable during core cron

> or if twig cache files were put in a completely different directory each time they are cleared as mentioned above

You can do that yourself by configuring the php_storage directly, here's what I do:

$settings['php_storage']['twig']['directory'] = '/tmp/' . getenv('PLATFORM_PROJECT') . '/phpstorage/twig/' . getenv('PLATFORM_TREE_ID');

Until that issue does it somehow automatically, but I'm honestly not sure how that would work exactly. I'm doing the cleaning in a daily cleanup cronjob then that deletes all folders except the current TREE_ID, which is an automatic identifier generated by platform.sh.

Is this major task somehow a duplicate of the normal bug report #3032078: Multiple webheads can cause infinite growth of Twig cache? (Which has a patch.)

@Eric_A That issue is different, I initially thought they were the same too, but this one is specifically about making sure inactive Twig compiled files are deleted across multiple webheads, that other issue did not fix this, one still needs to delete them separately.