wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
688 stars 216 forks source link

Varnish cache is not deleted after expiration of Rocket's cache #2619

Closed piotrbak closed 2 months ago

piotrbak commented 4 years ago

Describe the bug We are not clearing Varnish cache when purging expired cache: https://github.com/wp-media/wp-rocket/blob/e40b824350a165cf52f2285a681d5b6796ed5555/inc/Addon/Varnish/Subscriber.php#L44-L46

To Reproduce Steps to reproduce the behavior:

  1. Enable Varnish Add-on
  2. Set the Cache Lifespan to 10 minutes
  3. Add error_log here: https://github.com/wp-media/wp-rocket/blob/e40b824350a165cf52f2285a681d5b6796ed5555/inc/Addon/Varnish/Varnish.php#L116 http://snippi.com/s/q9danzj
  4. Preload the cache
  5. Update one post, just to be sure that we send the request to Varnish correctly
  6. Wait a couple of minutes until the cache expiration event clears the cache and confirm that it hasn't sent the request to Varnish

Expected behavior We should clear the Varnish cache just as ours, it can potentially cause broken layout.

Additional context What about our compatibility with NGINX Helper + 3rd parties like hostings?

Backlog Grooming (for WP Media dev team use only)

piotrbak commented 4 years ago

It seems to also be a problem when Cloudflare is set to cache everything with Page Rules. We'll hit Cloudflare cache then.

The customer has just sent me the access log and we didn't hit the server, the request was made correctly though: https://secure.helpscout.net/conversation/1146831025/160692/

crystinutzaa commented 4 years ago

Reproduce the issue ✅ Identified this issue on localhost.

Identify the root cause ✅ The root cause is that Varnish Addons needs to be hooked in Expired Cache Purge event rocket_after_automatic_cache_purge.

Scope a solution ✅ The solution is to hook Varnish Addon into: rocket_after_automatic_cache_purge and clean Varnish cache for the deleted urls: https://github.com/wp-media/wp-rocket/blob/d6c62d85364f422f4c9f575f5ee661a350856fa7/inc/classes/Cache/class-expired-cache-purge.php#L249

Effort ✅ Effort is minimal [S]

Additional context ✅ What about our compatibility with NGINX Helper + 3rd parties like hostings? @Tabrisrp @hellofromtonya I've checked a couple of 3rd party hostings like: LiteSpeed , Godaddy, Kinsta, o2switch all of these will need this hook. Should we create a new github issue for each of these plugins? Who should investigate and see which third party integration will need this functionality too? This could be a good oportunity to draw a diagram to see who and in which situations WP Rocket cache needs to be cleaned and Preload should be triggered?

hellofromtonya commented 4 years ago

I've checked a couple of 3rd party hostings like: LiteSpeed , Godaddy, Kinsta, o2switch all of these will need this hook. Should we create a new github issue for each of these plugins? Who should investigate and see which third party integration will need this functionality too? This could be a good oportunity to draw a diagram to see who and in which situations WP Rocket cache needs to be cleaned and Preload should be triggered?

@Tabrisrp What do you think?

Tabrisrp commented 4 years ago

We don't need it in cases where our own cache is disabled, like on Kinsta.

Let's also remember that the automatic cache purge only purges the cache files, not the minified files, so a broken layout should not be happening in this case.

What could be happening is outdated content, but all the hostings using server cache have a cache TTL pretty short.

I'd tend to be cautious around all this, and only plan/make changes if we have confirmed reported issues related to this specifically.

hellofromtonya commented 4 years ago

I'd tend to be cautious around all this, and only plan/make changes if we have confirmed reported issues related to this specifically.

Good approach in general. If we find a common problem or enhancement, we should be open to discuss and explore. We can validate each through an extensive QA process to thwart potential problems yet to be reported.

hellofromtonya commented 4 years ago

@crystinutzaa @Tabrisrp With the proposed solution, we will blast Varnish with purge requests. What are the risks?

Tabrisrp commented 4 years ago

That's one of the things I'm worried about, it could cause an overload of the Varnish server on busy sites.

hellofromtonya commented 4 years ago

I'm marking this one as blocked until we can discuss and figure out what we want to do with it.

hellofromtonya commented 4 years ago

For this issue, we are concerned about blasting Varnish and overloading it. In order to test this concern, we'll need to do some R&D with a test strategy and then measure the results.

For WP Media internal: I've added this to our R&D Backlog.

girlie commented 4 years ago

FYI, another customer who may be affected by this issue: https://secure.helpscout.net/conversation/1255425029/187045/

jazir555 commented 3 years ago

Please fix this, as there are issues where the varnish cache is out of sync and references previous cache files and comes up with a 404 error.

vmanthos commented 3 years ago

Related ticket: https://secure.helpscout.net/conversation/1468064251/251658?folderId=2135277

NataliaDrause commented 2 years ago

Possibly related: https://secure.helpscout.net/conversation/1718387896/312901?folderId=3864740

piotrbak commented 2 months ago

This is too risky to be implemented anywhere soon. We might revisit this in the future