Problem/Motivation
A client site is seeing stale hook data in the key/value store after deployments. Tends to be in situations like moving an OOP hook to a different class or similar.
What follows is purely a theoretical idea of what the problem might be, I talked it through with @nicxvan in slack prior to opening this. Once there is a viable MR, will try to validate it against the client's deployment process. Also might have a brute force way to rule it in or out which hasn't been tried yet.
The theory is:
opcache.revalidate_freq is set to 2s (the default), or possibly more.
Code is deployed to a web head.
drush cr / drush deploy or similar is called immediately, clearing the container cache.
Just before the container is rebuilt, a web head gets a container cache miss and starts to rebuild the container. This happens before the 2s opcache revalidation frequency has expired so it operates on the old version of the code base.
The cli also starts rebuilding the container and clears the cache.
The web head writes back to the cache, based on the stale class attributes from opcache.
Everything after this will have the updated classes, but there is nothing to trigger a new container rebuild because the new container was already built and cached successfully - it's just the hook data that got overwritten.
Later drush crs outside a deployment will fix it - because that rebuilds the container and rebuilds the hook data on the entirely fresh code base.
Steps to reproduce
Proposed resolution
If the above is correct, it might be fixable with a call to opcache_invalidate() on a filecache miss - filecache already validates the mtime.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3592577
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
Comment #2
catchComment #4
catchComment #5
catchComment #6
nicxvan commentedCode change looks good, I think we should hold off on getting this in until it's tested on the client mentioned.
Maybe we can artificially test this locally by setting an extremely high
revalidation_freqComment #7
larowlanCurrently debugging something similar although in our case it's not limited to deploys and hooks go missing resulting in access bypasses
drush cr fives it. We have database backups from occurrences
Comment #8
catch@larowlan that's interesting - are you able to check the timestamp of the k/v hook cache entry vs. the container cache? Is the container cached in the database or in redis?
I've opened #3592624: [PP-1] Move hook parsing from container rebuild to just-in-time which is hopefully completely unrelated to this issue but came up as an idea when reviewing the hook collector pass again.
Comment #9
larowlanWill do and report back on Monday
Comment #10
larowlanWe're using database here. I got a copy of a corrupt site and I don't think its related, the hook_list in the hook_data k/v collection has the things I'd expect to see. I've traced down so dud cache contexts in dynamic page caching and it looks like bugs in custom code or assumptions in JSON:API that we need a request policy for. So in other words, not core related - sorry for the noise
Comment #11
nicxvan commented@larowlan If you could post more detail here on the cause and solution I'd really appreciate that though so I can keep an eye out for similar issues and give the right advice for how to fix it.
@catch please let me know once your client has tested this.
Comment #12
catch@nicxvan it's been deployed on some environments and nothing went wrong but they also ran into something 30 minutes after deployment that looks like hooks going missing. Hoping to get good and bad dumps of the key value entries and maybe cache items to rule missing hooks in or out. If it does turn out to be missing hooks I'll report back as best I can here of course. But also possible that it is something else, as it turned out to be in @larowlan's case.
Notably they didn't see an issue immediately after deployment yet, so maybe it has done something but feels toonearly to say that it actually has.
So at the moment I don't know if this has fixed one of two different bugs, or if it's fixing a bug that never existed in the first place.
opcache_invalidate() only actually invalidates the file if the mtime check shows a change, it effectively brings forward the check that is done every two seconds. So given that it might be good to go ahead here even if it turns out not to have been the problem in these cases. A site could set the timestamp check to much longer too.
Comment #13
nicxvan commentedCan you rerun that failure if it's random.
I think this qualifies for the bug fix without a test policy.
I think this is good to go as long as that failure is random.
Comment #14
catchReran the test and all green now.
Comment #15
nicxvan commentedComment #16
alexpottThis seems risk-free and sensible.
Committed and pushed 16d92a48fb0 to main and fef92a29f66 to 11.x and aec98644177 to 11.4.x. Thanks!