Page MenuHomePhabricator

History pages' caches not being invalidated after edits
Closed, ResolvedPublic

Description

While logged out:

Screenshot 2022-09-05 at 17-23-04 Wikipedia Administrators' noticeboard_Incidents Revision history - Wikipedia.png (940×1 px, 525 KB)

$ curl -I 'https://en.wikipedia.org/w/index.php?title=Wikipedia:Administrators%27_noticeboard/Incidents&action=history'
HTTP/2 200 
date: Mon, 05 Sep 2022 10:43:55 GMT
server: mw1351.eqiad.wmnet
x-content-type-options: nosniff
content-language: en
vary: Accept-Encoding,Cookie,Authorization
last-modified: Mon, 05 Sep 2022 10:32:48 GMT
content-type: text/html; charset=UTF-8
age: 38417
x-cache: cp1083 hit, cp1079 hit/2
x-cache-status: hit-front
server-timing: cache;desc="hit-front", host;desc="cp1079"
strict-transport-security: max-age=106384710; includeSubDomains; preload
report-to: { "group": "wm_nel", "max_age": 86400, "endpoints": [{ "url": "https://intake-logging.wikimedia.org/v1/events?stream=w3c.reportingapi.network_error&schema_uri=/w3c/reportingapi/network_error/1.0.0" }] }
nel: { "report_to": "wm_nel", "max_age": 86400, "failure_fraction": 0.05, "success_fraction": 0.0}
set-cookie: WMF-Last-Access=05-Sep-2022;Path=/;HttpOnly;secure;Expires=Fri, 07 Oct 2022 12:00:00 GMT
set-cookie: WMF-Last-Access-Global=05-Sep-2022;Path=/;Domain=.wikipedia.org;HttpOnly;secure;Expires=Fri, 07 Oct 2022 12:00:00 GMT
accept-ch: Sec-CH-UA-Arch,Sec-CH-UA-Bitness,Sec-CH-UA-Full-Version-List,Sec-CH-UA-Model,Sec-CH-UA-Platform-Version
permissions-policy: interest-cohort=(),ch-ua-arch=(self "intake-analytics.wikimedia.org"),ch-ua-bitness=(self "intake-analytics.wikimedia.org"),ch-ua-full-version-list=(self "intake-analytics.wikimedia.org"),ch-ua-model=(self "intake-analytics.wikimedia.org"),ch-ua-platform-version=(self "intake-analytics.wikimedia.org")
x-client-ip: <removed>
cache-control: private, s-maxage=0, max-age=0, must-revalidate
set-cookie: GeoIP=<removed>; Path=/; secure; Domain=.wikipedia.org
accept-ranges: bytes
content-length: 186958

While logged in:

Screenshot 2022-09-05 at 17-23-13 Wikipedia Administrators' noticeboard_Incidents Revision history - Wikipedia.png (940×1 px, 431 KB)

I suspect this is fallout from the URL query sorting change (cc @ori) not invalidating the cache of history pages properly.

Also reported at https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Article_History_not_up-to-date_when_logged_out.

Event Timeline

I suspect this is fallout from the URL query sorting change (cc @ori) not invalidating the cache of history pages properly.

@Vgutierrez, any clue what might be happening?

As best as I can tell, purge URLs are getting query-sorted (as expected):

[cp1083:~] $ sudo varnishlog -k1 -n frontend -q 'ReqMethod eq "PURGE" and ReqURL ~ "action=history"' | grep -P '(ReqURL|purge$)'
-   ReqURL         /w/index.php?title=Q123487&action=history
-   ReqURL         /w/index.php?action=history&title=Q123487
-   VCL_return     purge

I'm interpreting this log snippet as showing that the query parameters get sorted before the purge is processed. Is that right?

yes, your assessment is right @ori, query parameters are sorted before triggering the purge

@ori, my current theory (and it needs to be tested) is that varnish frontend purges the history page as expected, but ATS isn't applying any query sorting to the purge request that it receives. So after the purge the unauthenticated request against https://en.wikipedia.org/w/index.php?title=Wikipedia:Administrators%27_noticeboard/Incidents&action=history triggers a hit in ATS.

This doesn't happen while being logged in cause those requests skip ATS as Vary:Cookie is there

I've just confirmed it in testwiki, first unauthenticated GET against action=history after an edit triggers x-cache-status: hit-local meaning that it was a HIT in ATS rather than varnish:

# before the edit
$ curl "https://test.wikipedia.org/w/index.php?title=Lara_van_Ruijven&action=history" -v -o /dev/null -s
[...]
< x-cache: cp6012 miss, cp6009 hit/2                                                     
< x-cache-status: hit-front
[...]
# after the edit
$ curl "https://test.wikipedia.org/w/index.php?title=Lara_van_Ruijven&action=history" -v -o /dev/null -s
[...]
< x-cache: cp6012 hit, cp6009 miss
< x-cache-status: hit-local

ATS provides a similar feature to libvmod-querysort as part of the Cache Key manipulation plugin (https://docs.trafficserver.apache.org/en/8.1.x/admin-guide/plugins/cachekey.en.html):

[...]
The plugin can

* sort query parameters to prevent query parameter reordering being a cache miss
[...]

query parameter sorting is performed by leveraging the StringSet C++ container:

https://github.com/apache/trafficserver/blob/8.0.8/plugins/cachekey/cachekey.cc#L617-L623
/* Use the corresponding container based on whether we need
 * to sort the parameters (set) or keep the order (list) */
String keyQuery;
if (config.toBeSorted()) {
  keyQuery = getKeyQuery<StringSet>(query, length, config);
} else {
  keyQuery = getKeyQuery<StringList>(query, length, config);
}

if we decide to go down this path we should check that:

  1. libvmod-querysort and ATS Cache Key Manipulation plugin produces the same output
  2. ATS 8.x and ATS 9.x perform query sorting in the same way (or wait till ATS 9.x is deployed globally)

What do you think @ori?

Another option is to do the query sorting for purges, which are a special case, in either:

  1. mediawiki itself: we can make Title::getInternalURL sort its parameters. Not sure of the fallout
  2. purged: this would make the fix more general, given at the moment we sort URLs incoming into varnish but not in ATS (which I think is a nice optimization) so the problem extends to any purge request, not just MediaWiki

I can take a stab at doing the sorting in purged.

Change 830124 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/software/purged@master] [WiP] sort query parameters in URLs

https://gerrit.wikimedia.org/r/830124

Change 830124 merged by Giuseppe Lavagetto:

[operations/software/purged@master] Sort query parameters in URLs

https://gerrit.wikimedia.org/r/830124

Change 830782 had a related patch set uploaded (by Vgutierrez; author: Vgutierrez):

[operations/software/purged@master] Release 0.18

https://gerrit.wikimedia.org/r/830782

Change 830784 had a related patch set uploaded (by Vgutierrez; author: Vgutierrez):

[operations/software/purged@master] querysort: Backport strings.Cut

https://gerrit.wikimedia.org/r/830784

Change 830784 merged by Vgutierrez:

[operations/software/purged@master] querysort: Backport strings.Cut

https://gerrit.wikimedia.org/r/830784

Change 830782 merged by Vgutierrez:

[operations/software/purged@master] Release 0.18

https://gerrit.wikimedia.org/r/830782

Mentioned in SAL (#wikimedia-operations) [2022-09-08T09:31:14Z] <vgutierrez> upload purged 0.18 to apt.wm.o (buster) - T317064

Mentioned in SAL (#wikimedia-operations) [2022-09-08T09:31:51Z] <vgutierrez> rolling restart of purged - T317064

Vgutierrez closed this task as Resolved.EditedSep 8 2022, 10:06 AM
Vgutierrez assigned this task to Joe.
#before the edit
vgutierrez@carrot:~$ curl "https://test.wikipedia.org/w/index.php?title=Sardinella_aurita&action=history" -v -o /dev/null -s 2>&1 |grep x-cache
< x-cache: cp6009 miss, cp6016 hit/3
< x-cache-status: hit-front
# after edit
vgutierrez@carrot:~$ curl "https://test.wikipedia.org/w/index.php?title=Sardinella_aurita&action=history" -v -o /dev/null -s 2>&1 |grep x-cache
< x-cache: cp6009 miss, cp6016 miss
< x-cache-status: miss
vgutierrez@carrot:~$ curl "https://test.wikipedia.org/w/index.php?title=Sardinella_aurita&action=history" -v -o /dev/null -s 2>&1 |grep x-cache
< x-cache: cp6009 miss, cp6016 hit/1
< x-cache-status: hit-front

Thanks @Joe for working on purged