Page MenuHomePhabricator

CentralNotice: Allow using Varnish cache of Special:BannerLoader HTML for logged-in users
Open, MediumPublic2 Estimated Story Points

Description

The Varnish caching of banners served via Special:BannerLoader is "hit-for-pass", which means cachability of an item isn't known until it's actually served up by the MW backend. Cache header values are set in SpecialBannerLoader.php.

We should review this setup to see if it's currently optimal. Review should include any relevant VCL code (varnish config) that comes into play, too.

As part of this update, we might well turn off caching for output for at caught errors.

Event Timeline

AndyRussG added a subscriber: aaron.

Copying some relevant comments by @aaron in T144952:

[...] To cover 11+ seconds of lag, MediaWiki lowers the CDN TTL to $wgCdnMaxageLagged when lag is 6+ seconds high. However, I see raw header() calls in the CN codebase...not sure how that interacts with this logic in MediaWiki::preOutputCommit.

The first approach might work using Varnish xkey support. I'm not how far along we are with using that in production though (AFAIK it's enabled).

Another idea is to add a cache-busting parameter to the URLs handed out, like the cache hash or a check key value.

The first approach might work using Varnish xkey support. I'm not how far along we are with using that in production though (AFAIK it's enabled).

Not available on the text cluster yet: T131503: Convert text cluster to Varnish 4 It's a goal for the Traffic team this quarter, afaik.

ggellerman moved this task from Triage to Q4 FY21-22 on the Fundraising-Backlog board.

The description and comments don't make a lot of sense from my outside (of CN/Banner code) POV. Are we talking about what the code does currently, or what it will do in some future release? Why would it be hit-for-pass if it's giving a CC header that allows caching? Why is cacheability different for logged-in and logged-out? Is it Vary-ing on Cookie if it really does need to be different for logged-in?

Another idea is to add a cache-busting parameter to the URLs handed out, like the cache hash or a check key value.

Great idea! We should definitely look into this!!!

The description and comments don't make a lot of sense from my outside (of CN/Banner code) POV.

Quite likely, I got a lot of stuff wrong. I guess I'm not even sure that "hit-for-pass" is what it really is...

Are we talking about what the code does currently, or what it will do in some future release?

I'd like to talk about how things work now, i.e., CN header-setting code as interacts w/ Varnish code, whether the effect all that has is what's desired, or needed, and how stuff might change.

Why would it be hit-for-pass if it's giving a CC header that allows caching? Why is cacheability different for logged-in and logged-out? Is it Vary-ing on Cookie if it really does need to be different for logged-in?

Yeah this is one thing we need to look into, especially check the use cases for. Maybe this is just legacy code that by default sent logged-in users directly to PHP. Hopefully we could start caching for them... :)

While it sure wouldn't hurt to look into this soon, a more pressing concern is the related but more-limited-in-scope T151418.

Thanks much, everyone!!

Are we still working on something here, or is this best closed and any remaining concerns opened in a fresh ticket? No real commentary in 4 years.

The swap of Traffic for Traffic-Icebox in this ticket's set of tags was based on a bulk action for all such tickets that haven't been updated in 6 months or more. This does not imply any human judgement about the validity or importance of the task, and is simply the first step in a larger task cleanup effort. Further manual triage and/or requests for updates will happen this month for all such tickets. For more detail, have a look at the extended explanation on the main page of Traffic-Icebox . Thank you!

Krinkle renamed this task from CentralNotice: Review and update Varnish caching for Special:BannerLoader to CentralNotice: Allow Varnish caching of Special:BannerLoader HTML.Fri, Jul 17, 7:25 PM
Krinkle updated the task description. (Show Details)
Krinkle renamed this task from CentralNotice: Allow Varnish caching of Special:BannerLoader HTML to CentralNotice: Allow using Varnish cache of Special:BannerLoader HTML for logged-in users.Fri, Jul 17, 9:32 PM
Krinkle subscribed.

I've confirmed today that the status quo is that banners are served from Special:BannerLoader (for logged-out and logged-in alike) and that for the latter these are always uncached for exactly the reason as in 2016. Varnish treats it as a pageview-like route and thus session cookies are available to it. MediaWiki then uses those cookies to initialize a RequestContext with a logged-in User object and passes it to the Special:BannerLoader route. There, regardless of what SpecialBannerLoader does really, the default in OutputPage::sendCacheControl is to disallow caching for any response to a request carrying session cookies. In practice, unless a SpecialPage or Action subclass does something to change this, CDN caching is only enabled for ViewAction+WikiPage (via ActionEntryPoint::performRequest > not SpecialPAge > performAction > CdnMaxAge)

The code in SpecialBannerLoader::sendHeaders might be reduncant, because MediaWiki would generally default to the same in OutputPage.php and possibly again independently at HeaderCallback.php. I'm not entirely sure though, because SpecialBannerLoader does disable a fair number of default behaviours.

It is desirable to get shared caching going on here, because we know the response is a fairly straight forward key-value retrieval that doesn't need to vary by anything other than its URL.

It should be feasible as well, because we already do the same thing in other entrypoints (such as load.php) and last I checked there was nothing in Varnish to make that work. It's up to MediaWiki to decide whether to emit Cache-Control:public and to not emit Vary:Cookie. Now, doing that from a special page may prove difficult due to the number of safety mechanisms that need to be circumvented and disabled. It might be easier to do from rest.php. I'd like to try improving Special:BannerLoader first, if for no other reason than to identify and document exactly what parts of the stack prevent shared caching from working on pageview-like routes. That work will benefit T432488: [Hypothesis] ST6.4.2 Increase MediaWiki backend capacity for logged-in pageviews (tracking) FY2026-2027 in two ways:

  1. We can apply what we learn to other entrypoint where we'd like to consider shared CDN caching for more logged-in user responses (as we already do for ResourceLoader load.php).
  2. By CDN-caching CentralNotice banner HTML, we free up MediaWiki backend load for logged-in pageviews.