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

Issue fork drupal-3584324

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

catch created an issue. See original summary.

catch’s picture

Category: Task » Bug report

catch’s picture

Status: Active » Needs work

Something like this, will need some test coverage but want to see if any existing test coverage fails with this change.

catch’s picture

Status: Needs work » Needs review

Added unit test coverage.

smustgrave’s picture

So had to run this locally testFallThroughToConsistentCache is passing still should it fail?

catch’s picture

Somehow managed to replace a test method instead of purely adding a new one - updated the MR and also kicking off the test only job.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Yea 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.

dries’s picture

Shouldn't that be?

if ($last_write_timestamp === 0) {
  $this->markAsOutdated();                                                                                                                                                               
}     
catch’s picture

Status: Reviewed & tested by the community » Needs review

Umm 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.

dries’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to me.

kristiaanvandeneynde’s picture

MR 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

catch’s picture

@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.

alexpott’s picture

Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed c3a6c1a5285 to main and 2946c12023f to 11.x and 4332e36a5a6 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 4332e36a on 11.3.x
    fix: #3584324 Make the chained fast backend self-heal when...

  • alexpott committed 2946c120 on 11.x
    fix: #3584324 Make the chained fast backend self-heal when...

  • alexpott committed c3a6c1a5 on main
    fix: #3584324 Make the chained fast backend self-heal when...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.