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

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

Status: Active » Needs review

catch’s picture

Issue summary: View changes
catch’s picture

Status: Active » Needs review
nicxvan’s picture

Code 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_freq

larowlan’s picture

Currently 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

catch’s picture

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

larowlan’s picture

Will do and report back on Monday

larowlan’s picture

We'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

nicxvan’s picture

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

catch’s picture

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

nicxvan’s picture

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

catch’s picture

Reran the test and all green now.

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

This seems risk-free and sensible.

Committed and pushed 16d92a48fb0 to main and fef92a29f66 to 11.x and aec98644177 to 11.4.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 aec98644 on 11.4.x
    fix: #3592577 Ensure that hook attributes are never parsed from a stale...

  • alexpott committed fef92a29 on 11.x
    fix: #3592577 Ensure that hook attributes are never parsed from a stale...

  • alexpott committed 16d92a48 on main
    fix: #3592577 Ensure that hook attributes are never parsed from a stale...

Status: Fixed » Closed (fixed)

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