[gradinarufelix]

With an allkeys-* eviction policy, Redis can evict a tag set and keep the entries it points to. flushByTag() then misses those entries, and their outdated content is served until they expire. We hit this on a Neos 8.4 site whose content cache runs on Redis with allkeys-lru.

volatile-* policies only evict keys with an expiry time. Tag sets now never get one, so under such a policy Redis only evicts cache entries. getStatus() warns about allkeys-* policies, which shows up in ./flow cache:list and ./flow cache:show, and the documentation describes the setup. set() also stops looking up the expiry time of every tag set, which saves two Redis round trips per tag.

Related: #2992 asks for tag sets to expire, which this change rules out.

Upgrade instructions

If your Redis server uses an allkeys-* policy, switch to volatile-lru or volatile-ttl. Give caches that can outgrow the memory limit a finite defaultLifetime first, because Redis rejects writes once it is full and has no key with an expiry time left to evict.

Review instructions

Tag sets that still carry an expiry time from older versions lose it on their next write. On a 4 MB Redis with 3000 entries, allkeys-lru left 163 of 435 remaining entries unflushable by tag, while volatile-lru with a 30 day lifetime evicted 2603 entries and kept every tag set.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

[kitsunet]

We already discussed that the case is real and a problem, however looking at this again, I wonder, shouldn't we just make sure the (tag) list entries never get a TTL and then document that you cannot use any "allkeys" policy for eviction if you want this to work and instead need to use "volatile" policies?

Beach would obviously switch to that ASAP, now that I am thinking about it. Might be the cleaner way? IDK, both seem sensible I just fear this increases complexity a lot.

For reference the "volatile" policies only evict entries with a set TTL, everything else cannot be evicted. So "volatile-lru" or "volatile-ttl" might be good fits (either least recently used or shortest remaining TTL)

[gradinarufelix]

Agreed, I switched the PR to your approach. Tag sets never get an expiry time now, and getStatus() warns about allkeys-* policies. Please hold the Beach switch until we've given our caches a finite lifetime, since volatile-* only evicts keys with an expiry time and ours all run with defaultLifetime: 0.

[pagani-BETTERHOMES]

Thanks for picking this up. I'd ask to hold the Beach switch for a different reason: "tag sets never expire + finite lifetime + volatile-*" is exactly the setup that took our production site down.

What happened

All our Redis caches had a finite defaultLifetime and Redis ran with volatile-lru. After about two days, Redis was full of tag:* / tags:* sets without expiry whose entries were long gone, and every write failed with OOM command not allowed when used memory > 'maxmemory'.

The current code already does almost what this PR does: calculateExpires() treats TTL -2 (set doesn't exist yet) as "no expiry", so new tag sets are persisted (#2992). This PR makes that unconditional.

Why never-expiring tag sets leak under volatile-*

• When an entry expires or is evicted, only entry:<id> disappears. Its tags:<id> set and its member in every tag:<tag> set stay, and collectGarbage() is a no-op.
• volatile-* can never evict these sets, so they grow with every distinct entry ever written.
• Evicting entries only buys time. Once no key with an expiry is left, Redis rejects writes.

So a finite lifetime doesn't make this approach safe, it's what produces the orphans: every expired or evicted entry leaves its tag sets behind. With defaultLifetime: 0, nothing is evictable at all, so it fails as well, just differently.

Alternative: tag sets outlive their entries

Invariant: a tag set always expires later than every entry it references.

• A new tag set gets the lifetime of its entry plus a margin (we use one hour).
• A tag set with an expiry is only ever extended, never shortened.
• A tag set without expiry keeps it, because it belongs to an entry with unlimited lifetime.
• An entry with unlimited lifetime persists its tag sets.

With volatile-ttl, Redis evicts the keys with the shortest remaining TTL first, so entries go before their tag sets (approximately, as Redis samples). The margin matters for tag sets with a single entry: without it, both expire at the same moment and volatile-ttl may evict the tag set first.

With volatile-lru, volatile-lfu, volatile-random or allkeys-*, a tag set can still be evicted before its entries: a tag set is only touched when an entry is written, while the entry of a popular page is read all the time. So getStatus() should recommend volatile-ttl. This resolves #2992 instead of ruling it out.

On complexity: the change stays inside set(). Entry and tag sets are written in one Lua script, which replaces calculateExpires() and its TTL lookups, needs a single round trip per set() and removes the race of reading the TTL outside of MULTI, without the WATCH approach of #2052 (reverted in #2516 because of #2483).

Code comparison: RedisBackend::set()

This PR:

$this->redis->multi();
$result = $this->redis->set($this->getPrefixedIdentifier('entry:' . $entryIdentifier), $this->compress($data), $setOptions);
if (!$result instanceof \Redis) {
    $this->verifyRedisVersionIsSupported();
}
foreach ($redisTags as $tag) {
    $this->redis->sAdd($tag['key'], $tag['value']);
    // Tag sets never expire, so a "volatile-*" eviction policy can never evict them
    $this->redis->persist($tag['key']);
}
$this->redis->exec();

What we run in production (a subclass overriding only set(); written here as it would look inside RedisBackend):

private const TAG_LIFETIME_MARGIN = 3600;

// KEYS: entry key, then tag set keys; ARGV: data, entry lifetime, tag set lifetime, then one member per tag set key
private const SET_SCRIPT = <<<'LUA'
    local lifetime = tonumber(ARGV[2])
    local tagLifetime = tonumber(ARGV[3])
    if lifetime > 0 then
        redis.call('SET', KEYS[1], ARGV[1], 'EX', lifetime)
    else
        redis.call('SET', KEYS[1], ARGV[1])
    end
    for i = 2, #KEYS do
        local ttl = redis.call('TTL', KEYS[i])
        redis.call('SADD', KEYS[i], ARGV[i + 2])
        if lifetime <= 0 then
            redis.call('PERSIST', KEYS[i])
        elseif ttl == -2 or (ttl >= 0 and ttl < tagLifetime) then
            redis.call('EXPIRE', KEYS[i], tagLifetime)
        end
    end
    return 1
    LUA;

public function set(string $entryIdentifier, string $data, array $tags = [], ?int $lifetime = null): void
{
    if ($this->isFrozen()) {
        throw new \RuntimeException(sprintf('Cannot add or modify cache entry because the backend of cache "%s" is frozen.', $this->cacheIdentifier), 1323344192);
    }

    if ($lifetime === null) {
        $lifetime = $this->defaultLifetime;
    }

    $keys = [$this->getPrefixedIdentifier('entry:' . $entryIdentifier)];
    $members = [];
    foreach ($tags as $tag) {
        $keys[] = $this->getPrefixedIdentifier('tag:' . $tag);
        $members[] = $entryIdentifier;
        $keys[] = $this->getPrefixedIdentifier('tags:' . $entryIdentifier);
        $members[] = $tag;
    }
    $arguments = [$this->compress($data), (string)$lifetime, (string)($lifetime + self::TAG_LIFETIME_MARGIN)];

    $result = $this->redis->eval(self::SET_SCRIPT, array_merge($keys, $arguments, $members), count($keys));
    if ($result !== 1) {
        throw new \RuntimeException(sprintf('Could not write cache entry "%s" of cache "%s": %s', $entryIdentifier, $this->cacheIdentifier, (string)$this->redis->getLastError()), 1793346002);
    }
}

calculateExpires() can be removed as in this PR. flushByTag(), remove() and findIdentifiersByTag() stay unchanged. The script needs Redis 2.6.12 or later (EVAL and SET … EX).

getStatus() would then warn about everything except volatile-ttl (and noeviction, which never evicts):

if (!in_array($evictionPolicy, ['volatile-ttl', 'noeviction'], true)) {
    $result->addWarning(new Warning(sprintf('The eviction policy "%s" can evict tag sets before their entries, so flushing by tag misses entries. Use "volatile-ttl" instead.', $evictionPolicy), 1793184252, [], 'Eviction policy'));
}

Tests

Instead of tagSetsNeverExpire, these are the cases we verified:

• a new tag set gets the lifetime of its entry plus the margin (tag: and tags:)
• an entry with a longer lifetime extends the tag set
• an entry with a shorter lifetime doesn't shorten it
• an entry without explicit lifetime uses defaultLifetime
• a tag set of an entry with unlimited lifetime stays without expiry when a limited entry is added
• an entry with unlimited lifetime removes the expiry of an existing tag set
• flushByTag(), remove() and compressed values still work

Upgrade instructions

Tag sets written by older versions have no expiry and can't be told apart from tag sets of entries with unlimited lifetime, so they keep it. Flush the Redis caches once after the upgrade (or restart Redis), then switch to volatile-ttl.

Two things to keep in mind

• A tag set that keeps getting new entries (e.g. a tag on almost every page) is extended on every write and collects the identifiers of expired entries until it's flushed by tag. That's bounded by the number of distinct entries.
• If Redis runs full, volatile-ttl evicts entries with a short lifetime first, regardless of how often they are read. compressionLevel helps keep Redis from running full, as HTML entries compress well.