Repository navigation
BUGFIX: Treat Redis cache entries with an evicted tag index as missing - #3637
Open
gradinarufelix wants to merge 1 commit into
Open
gradinarufelix wants to merge 1 commit into
gradinarufelix wants to merge 1 commit into
Conversation
The RedisBackend stores an entry, the set of its tags and one set per tag as independent keys. Under memory pressure, e.g. with the "allkeys-lru" eviction policy, Redis can evict a tag set while the frequently read entry survives. The entry can then no longer be flushed by tag and is served until it expires. get() and has() now verify in a Lua script that the entry's tags set exists and that the entry is still a member of each tag set. Otherwise the entry is removed and reported as missing, so it gets rebuilt. set() adds a marker to every tags set, so that an evicted tags set is also detectable for untagged entries.
Member
|
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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RedisBackendstores each cache entry in three kinds of independent keys: the entry itself, the set of its tags (tags:<entry>), and one set per tag (tag:<tag>). Under memory pressure, for example withmaxmemory-policy allkeys-lru, Redis can evict a tag set and keep the frequently read entry. After that,flushByTag()can no longer find the entry, so the outdated content is served until the entry expires or someone flushes the whole cache. For the Neos content cache, the result is that a published change never shows up on the page.get()andhas()now run a Lua script that checks two things: the entry's tags set still exists, and the entry is still a member of every one of its tag sets. If either check fails, the script removes the entry and its remaining tag memberships and reports a cache miss, so the entry is rebuilt with a complete tag index.set()now adds a marker to the tags set of every entry. Without it, an evicted tags set could not be detected for untagged entries.We saw this on a Neos 8.4 site whose content cache uses Redis with
allkeys-lru. After a publish, pages kept their old content untilNeos_Fusion_Contentwas flushed by hand. An earlier variant of this change has been running on that site since July 2026 as a fork offlownative/flow-redis-backend(flownative/flow-redis-backend#1), where @kitsunet suggested fixing it in Flow directly.Upgrade instructions
None. Tagged entries written by older versions stay valid. Untagged entries written by older versions have no marker, so each one is rebuilt once after the update.
Review instructions
get()sends the script and a plainGETin one pipeline, which is still a single round trip. Passing the value through Lua costs time in proportion to its size: on a 64 KB entry, an earlier version that returned the data from the script took about 155–180 µs of Redis time per read, against 2–4 µs for a plainGET. A pipeline is not atomic, but the worst case is one read that returns an entry whose tag set was evicted between the two commands. The next read detects that and removes the entry.INFO commandstats(local Docker setup, so the numbers are noisy): about 4–6 µs with no tags, 10–18 µs with 10 tags and 23–28 µs with 30 tags, regardless of entry size. On the unpatched branch, aGETtakes 0.3–4 µs and anEXISTS0.2 µs. A sample of 105 Neos content cache entries had a median of 2 tags and a 90th percentile of 4.FrontendInterface::PATTERN_TAG, so a tag can never contain the null byte in the marker.entriesWithAnEvictedTagIndexAreRemovedfails because the stale value is returned.taggedEntriesWrittenWithoutTagsMarkerAreStillReturnedcovers entries written by older versions.Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions