Page MenuHomePhabricator

Parseroutput cache issue with videojs
Open, MediumPublic


< Krinkle> ParserCache::save Memc set key: enwiki:pcache:idoptions:12108 - ok: true
< Krinkle> ParserCache::save Memc set key: enwiki:pcache:idhash:12108-0!canonical!tmh-videojs true

< Krinkle> Logstash: channel:AdHocDebug OR channel:ParserCache OR message:parser OR e:parsercache
< Krinkle> ParserOutput key valid, but rejected by RejectParserCacheValue hook handler.
< Krinkle> Prod extensions using this MW hook: Wikibase, ops/mediawiki-config, Kartographer, TimedMediaHandler

Krinkle> So if you user pref and current object is different from site config, it always refuses to persist it
Krinkle> so likely just induced by the beta pref more popular

See also:

So on RejectParserCacheValue of TimedMediaHandler only considers the site default, and not the activePlayerMode for the player. This was done for a site preference switch, but didn't account for user preference switches via beta player mode and the parsercachekey already being adapted according to this activePlayerMode.

The mode has been available for a while already. Likely got more pervasive as parsercache values started expiring, more and more people get enrolled in the betafeature etc. Additionally the issue is only really noticeable if the parsercache performance actually matters aka. big pages only

< James_F> is the ncy disable patch if it's needed.
< James_F> It'll be messy, but it'd stop people adding the beta feature.
< James_F> (And as they touch their preferences, existing people would get emptied out.)

Event Timeline

TheDJ triaged this task as High priority.Aug 14 2019, 6:09 PM
TheDJ updated the task description. (Show Details)
TheDJ added subscribers: Jdforrester-WMF, Krinkle.

Change 530191 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/TimedMediaHandler@master] onRejectParserCacheValue: Correct name of target videojs module; add comment

I think default mode is checked because there's a separate pcache suffix for folks using the beta-mode -- this is targeted at clearing the non-beta-mode entries after a site-config switch.

So it probably shouldn't be rejecting anything on the beta-mode suffix variant...?

I also think it's unwise to call purgeSquid here; it won't actually work in all views (eg it never gets called if you were on a Varnish cache hit). Simply rejecting the pcache should be enough for logged-in views without unnecessarily purging other cache data, whereas if we need to reset all the pages with A/V to handle the updated output & modules, then we should have a script purge just those pages.

Note the parser cache key is not available to the hook. Fun!

I think what will work is:

  • change to use activePlayerMode instead of defaultPlayerMode
  • don't actively purge anything

This will allow old default cached page views to update transparently after the default is switched (if they get past Varnish cache layer), and should be harmless for users in the beta group, who see a different suffix on their cache keys.

(If we find we need to do a mass-update of pages after the switch (to deal with cached old modules that won't exist in RL eventually) then we'll want to purge just those pages with a batch or something.)

Change 530200 had a related patch set uploaded (by Brion VIBBER; owner: Brion VIBBER):
[mediawiki/extensions/TimedMediaHandler@master] Adjustments to RejectParserCacheValue for TMH player mode

Change 530191 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] onRejectParserCacheValue: Correct name of target videojs module; add comment

Change 530200 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Adjustments to RejectParserCacheValue for TMH player mode

Jdforrester-WMF lowered the priority of this task from High to Medium.Aug 18 2019, 5:12 PM

Provisionally we think this is fixed in master for the current usage in production (and will roll out with the train this week); there are further fixes we'll want to do before closing this task, however.