Symfony version(s) affected

4.4 / all

Description

If you retrieve an item which is in the cache already, changes its value and save it again without changing/setting a new expiration time, the expiration is lost by most (all?) adapters and the item becomes stored forever (or whatever the default expiration is).

How to reproduce

<?php

include 'vendor/autoload.php';

use Symfony\Component\Cache\Adapter;

$adapters = [
    new Adapter\ApcuAdapter(),
    new Adapter\ArrayAdapter(),
    new Adapter\FilesystemAdapter(),
    new Adapter\RedisAdapter(Adapter\RedisAdapter::createConnection('redis://localhost')),
];

foreach ($adapters as $cache) {
    // create item with 3seconds expiration
    $test = $cache->getItem('test');
    $test->set('value');
    $test->expiresAfter(3);
    $cache->save($test);

    // fetch item again and save it without setting an expiration
    $test = $cache->getItem('test');
    if (!$test->isHit()) {
        echo get_class($cache).': Something is wrong, item should have been a hit here'.PHP_EOL;
    }
    $test->set('value');
    $cache->save($test);
}

sleep(4);

foreach ($adapters as $cache) {
    $test = $cache->getItem('test');
    echo (get_class($cache).' '.($test->isHit() ? 'HIT' : 'MISS')).PHP_EOL;
}

I would be expecting all MISS here as the expiration was set to 3 seconds, and then we sleep for 4. This is unfortunately not the outcome, and changing the value of an existing item resets the expiration.

Symfony\Component\Cache\Adapter\ApcuAdapter HIT
Symfony\Component\Cache\Adapter\ArrayAdapter HIT
Symfony\Component\Cache\Adapter\FilesystemAdapter HIT
Symfony\Component\Cache\Adapter\RedisAdapter HIT

Possible Solution

I am not sure if this needs fixing. This seems to be an issue as well in https://github.com/php-cache/redis-adapter and https://github.com/tedious/Stash for example (as far as I can tell from looking at a couple adapters' code).

So it seems to me like it is a design flaw of PSR-6 that there is no requirement (or at least possibility) for getItem to return a fully populated item including its expiration time.

I can see that for performance reason returning the TTL always would be very suboptimal as for the most common cache backends you would need to do additional calls to get the metadata.

Two improvements I can see that would be valuable:

• CacheItem could have its expiration set to -1 by default when the item is a hit, indicating that it came from the cache. Then if it is saved again and the expiration was not changed the CachePool could at that point reach out to the backend to figure out the current TTL of the item and store it back with the same TTL. This would need a new method outside of the PSR-6 interfaces though I believe, and could only be supported for symfony/cache CacheItem instances, but that's ok IMO.
• AdapterInterface/CachePool could have a new method added like getItemWithTTL which would retrieve an item and do the extra call to figure out the current expiration. Then people aware of the issue who are trying to update a value in the cache without changing the TTL could use that instead of getItem.
• AdapterInterface/CachePool could throw/reject any item which came from the cache (isHit) on which an expiration was not explicitly re-configured (either set to null or a time), but while it would catch misuses/mistakes, that's probably a BC break too.
• At the very least, documenting this limitation would be a good thing if it can't be fixed.

Additional Context

Some more info to show that this is hopefully not me being crazy but that there is a real problem as it's causing issues in Symfony itself.

I came upon this trying to debug why our redis DB was filling up with RateLimiter data. It turns out that this code is the root cause when using a sliding window:

This causes the CacheStorage to do exactly what I described above in the issue:

The result is that if someone gets rate limited once the TTL is set correctly on the cache entry using SETEX in redis, but then if the user does the action a second time the cache entry is overwritten using SET which removes the TTL, and the entry becomes permanently stored in the cache.

I will open a PR for this as it's a separate issue really. Edit: #44996