Problem/Motivation
Discovered via #3583040: Cache the container definition in APCu.
When there is a valid cache entry in the persistent backend, but no last_write_timestamp entry, chained fast will return a valid cache item from the persistent backend and write back to the fast backend.
This means that last_write_timestamp doesn't get set until there's an explicit cache set (due to a cache miss).
Steps to reproduce
Proposed resolution
When last_write_timestamp is missing from the persistent backend, write back a value with the current time immediately.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Comments
Comment #2
catchComment #4
catchSomething like this, will need some test coverage but want to see if any existing test coverage fails with this change.
Comment #5
catchAdded unit test coverage.
Comment #6
smustgrave commentedSo had to run this locally testFallThroughToConsistentCache is passing still should it fail?
Comment #7
catchSomehow managed to replace a test method instead of purely adding a new one - updated the MR and also kicking off the test only job.
Comment #8
smustgrave commentedYea I guess since you're a committer I can't run your test-only jobs so thanks for doing that https://git.drupalcode.org/project/drupal/-/jobs/9345118
Nothing else to add.
Comment #9
dries commentedShouldn't that be?
Comment #10
catchUmm yes it should, obviously was thinking about the complicated comment and not the simple fix... Fixed that and added an expectation to one of the other test methods that would have caught it.
Comment #11
dries commentedLooks ready to me.
Comment #12
kristiaanvandeneyndeMR looks good to me, comment reads really well and test is also very clear.
I was wondering how we get to this situation, though. It can only ever occur once per bin, right? And only if you somehow managed to write to the consistent cache backend outside of the CFB.
So while it's good to have this safeguard in place (as it costs nothing when the last write is not zero), I wonder if we'd ever run into this at all outside of the experiment Dries ran in #3583040: Cache the container definition in APCu or some very edge cases where a consistent cache has multiple ways to reach it: A CFB and direct backend.
Either way, +1 on RTBC
Comment #13
catch@kristiaanvaneyde so I think a more high level version of what happened in #3583040: Cache the container definition in APCu would be converting any regular cache bin to chained fast, where the container gets rebuilt, but the cache is not cleared. In that case, the persistent cache bin has a lot of warm cache entries, and the fast cache bin would be empty. Because there are entries in the persistent cache bin, there is no cache miss from the consumer's point of view, so nothing triggers a cache set. You'd get repeated writes to the chained fast backend, but no actual cache set until a real miss on the persistent backend too.
Another example would be an LRU cache eviction where the last_write_timestamp gets evicted but other cache items are left in the bin, which is probably impossible with a correct LRU strategy but bugs do happen and neither redis nor memcache would guarantee that frequently used cache items won't arbitrarily disappear.
I can't think of any other situations, so it should be extremely rare, and that's probably why it hasn't come up before. Also the worst side-effect here is APCu writes which are relatively cheap, so a site would see degraded performance if it happens, but maybe not degraded enough that they'd diagnose the problem before it fixed itself.
Comment #14
alexpottCommitted and pushed c3a6c1a5285 to main and 2946c12023f to 11.x and 4332e36a5a6 to 11.3.x. Thanks!