ChainedFastBackend should reject cache items from the fast backend that have identical timestamps to last_write_timestamp

This is some really impressive debugging. Nice work as always.

I think the fail/pass with the installer test is quite convincing, but.....is there any way we can write a less elaborate test for this? And something a little (ok, a lot) more explicit?

And I'm also wondering why this doesn't occur in WebTestBase install tests, are they just slower? Or are we doing something funky in BrowserTestBase?

Having an explicit test might help answer those questions too.

Ok so if you set things up like this, you have a failing test.

If you get into a state where lastWriteTimestamp is equal to the create timestamp in the entry, the ChainedFastBackend no longer knows what the sequence of events was, so it should default to the consistent cache, not the fast cache entry as it currently does.

But.....it's all well and good in a test scenario, but I don't actually see how you can end up in such a state in a real environment.

#12 is failing randomly.... even locally it turns out. That doesn't make sense to me, but that's what happening.

Here is a version of the patch that I can consistently get to fail randomly (interesting sentence :-))

Running this locally gets me a fail rate of 25% to 50% fails.

\Drupal\Core\Cache\ChainedFastBackendFactory::__construct():

// Do not use the fast chained backend during installation. In those cases,
    // we expect many cache invalidations and writes, the fast chained cache
    // backend performs badly in such a scenario.
    if (!drupal_installation_attempted()) {
      $this->fastServiceName = $fast_service_name;
    }

So yes, we do not use the chained backend during the installer.

It looks like there's a couple of approaches here, using <= seems like a good plan though. Tagging for issue summary update.

Is this still relevant since #3498193: Optimize last_write_timestamp writes in ChainedFastBackend?

I converted the test-only patch to an MR, (edit: unsuccessfully...).

The < check is still in core and I don't think it would hurt at all to change that to <=

#3498193: Optimize last_write_timestamp writes in ChainedFastBackend means that when there is one cache write on a page, the two servers would have to be more out of sync for there to be a problem, but it's only 50ms which is within NTP drift expectations, so it only makes the problem less likely and doesn't eliminate it. I think therre might be an issue around to try to use something else (like a special cache tag) that doesn't rely on clock time. The random failure discussed here appears to be something that isn't actually due to NTP clock sync at all but more rounding errors, with the 50ms ahead that's almost certainly solved now because it's further ahead, but still think changing to <= is more correct.

The test only job didn't fail, but reverting the fix causes the new test to fail - here's the failing job:

https://git.drupalcode.org/project/drupal/-/jobs/4790389

There are more tests that are failing on the last test run. All in ChainedFastBackend.

The change however makes sense and it has good test coverage.

hmm the new test that fails is the one that's being added so something is still wrong here.

Forgot about this issue but I think that #3526080: Reduce write contention to the fast and consistent backend in ChainedFastBackend makes this outdated now.

Going to close as outdated since #3526080: Reduce write contention to the fast and consistent backend in ChainedFastBackend landed.