[random test failures] Race condition in state when individual keys are set with an empty cache

I ran WorkflowUiTest 100 times and got 3-4 fails.

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

Then with the fix, I ran it 500 times and got 0 fails:

https://git.drupalcode.org/project/drupal/-/jobs/1244933
https://git.drupalcode.org/project/drupal/-/jobs/1248600
https://git.drupalcode.org/project/drupal/-/jobs/1248679
https://git.drupalcode.org/project/drupal/-/jobs/1248737
https://git.drupalcode.org/project/drupal/-/jobs/1248755

I need to back out the test changes here, they're just for the repeated test run job so that we only run one test method 100 times instead of the entire test, but would be great to get reviews - and leaving it in for now in case we want to re-run that job even more times.

Looks good to me - the fixes make sense and the comments definitely help explain what's going on. As was discussed in Slack this was originally a concern with the cache collector but never was an issue in practice until tests started hitting cold caches occasionally in this way. For me the evidence in #6 is enough to commit this, so NW to remove the test changes.

Yeah we covered the warm (but invalid) cache situation with the cache collector, which is the main case, but cold + invalid just never came up or seemed realistic. Additionally you would never hit this with a discovery cache (which is most cache collector use cases), because it's unlikely to get invalidated twice in the same request or anything, needs to be extremely high write. I'm not sure a site would even run into this with state because fully cold cache then immediate write situation seems unlikely there too, but we're clearly hitting it in tests.

Rebased to drop the test modifications.

MR!7336 looks great.

Started off as a task but definitely a bugfix now.

Had one more look over this and realised the comment in State::set() was slightly out of date and could be clearer. Updated that but leaving RTBC since it's comment-only.

alexpott → changed the visibility of the branch 3438424-random-test-failures to hidden.

alexpott → changed the visibility of the branch 3438424-usleep to hidden.

The latest changes have the MR failing tests... see https://git.drupalcode.org/project/drupal/-/merge_requests/7336/pipelines

Managed to commit to the wrong branch... dropped from the MR now.

Still failing tests...

core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.php
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php

I think having the MR link in the issue messes up d.o so it puts the failing test link in the wrong place... I've removed that from the issue summary so hopefully this is easier to see.

OK now I can see the test failures, that's helpful.

The unit test failure is very handy, I wasn't sure how to test the race condition here, but turns out we already had a unit test for a cache item being set in the middle of the request. I've split that one method into two, so it now tests both the warm cache situation (already covered but the test wasn't explicitly testing this) and cold cache situation (what we're fixing here and what the test coverage was actually testing but with different expectations).

Performance tests are green now - it's one extra state cache set + the related key/value queries collected during that request, not surprising since we're being more conservative about writing to the cache on completely cold starts now.

Given catch's changes are to a test to fix and improved comments going to rtbc and commit. Hopefully this will reduce some of the random fails due to the state cache collector from occurring.

Committed 3a3e618 and pushed to 11.x. Thanks!
Committed af46c48 and pushed to 10.3.x. Thanks!

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