Page MenuHomePhabricator

Forcelinkupdate appears to have stopped working (on ENWIKI at least)
Closed, ResolvedPublic

Description

Purge's forcelinkupdate option appears in several contexts to have become non-functional (as installed on ENWIKI), *probably* more than one and less than three weeks ago.

The earliest entries in https://en.wikipedia.org/wiki/Category:AfC_pending_submissions_by_age/0_days_ago will usually demonstrate the problem. Note that by reading the page, the articles appear categorized as much older than "0 days ago." A null edit will (as expected) correct this, a purge by itself (as expected) will not correct this. Until recently, purge with the forcelinkupdate option did (as expected) correct this, now it does not.

I believe I have observed similar changes in the behavior of my PERL-based bot, the Pywikibot touch.py bot, and the direct URL API.

Background: Multiple tools make use of the forcelinkupdate option on ENWIKI to update pages, including but not limited to Pywikibot and Joe's Null Bot. Generally these tools are used to deal with the fact that category inclusion for an article is not constantly reevaluated, so, a template, say, conditionally includes a category if the time has passed a certain timestamp will not be included in the appropriate category until or unless a full null edit or a purge/forcelinkupdate is performed. The latter is strongly preferred for many use cases to avoid polluting the article history.

See also: https://en.wikipedia.org/wiki/User_talk:Joe_Decker#CAT:AFC

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 23 2018, 8:36 PM

Hi @joedecker, thanks for taking the time to report this and welcome to Wikimedia Phabricator!

(not sure what's the right project tag for this, trying API)

Anomie moved this task from Unsorted to Needs details or plan on the MediaWiki-API board.
Anomie added subscribers: daniel, Tgr, Anomie.

The only change to the API module recently was rMW51602a436c72: [MCR] Move getSecondaryDataUpdates to the page level. @Tgr or @daniel would be better qualified to look into it since it involves DerivedPageDataUpdater.

JJMC89 added a subscriber: JJMC89.Nov 23 2018, 9:41 PM
daniel triaged this task as High priority.Nov 24 2018, 3:59 PM
daniel added a project: Core Platform Team.
daniel added a subscriber: aaron.

Analysis:

  • RefreshLinksJob is hitting the ( $page->getLinksTimestamp() > $skewedTimestamp ) condition and bails out, logging 'refreshlinks.update_skipped'.
  • This is caused by $skewedTimestamp being incorrectly set to a time based on the revision's timestamp, not based on the time the ParserOutput was re-created by ApiPurge.
  • $skewedTimestamp comes from the 'rootJobTimestamp' job param, which is set by LinksUpdate::getAsJobSpecification() to $this->mParserOutput->getCacheTime().
  • The ParserOutput's cache time is set in DerivedPageDataUpdater::doParserCacheUpdate (via ParserCache::set).
  • The line that determins the time stamp is $timestamp = $this->options['changed'] ? $this->revision->getTimestamp() : $output->getTimestamp();. This is incorrecct, it should use $output->getCacheTime();.
  • As it turns out, this still does not fix the problem, because $this->options['changed'] is incorrectly set to true, because $oldId is initialized $revision->getParentId() in prepareUpdate()
  • This seems to be caused by some confusion about what $this->options['changed'] means. Should it be true or false for a "dummy" revision? Should it indicate the difference between the new/current revision, and the old revision, or should it indicate whether the present DerivedPageDataUpdater is for a newly created revision, as opposed to a null edit or forced update?
  • It would be nice if we could clean up the code in prepareUpdate(), it currently tries to cater to a variety of callers which all have slightly different expectations.

Pinging @aaron since he probably knows best how all the pieces interact.

Change 475593 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Fix the cache timestamp for forced updates.

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

daniel claimed this task.Nov 26 2018, 3:21 PM

This should probably be in the 1.32 release.

Change 475593 merged by jenkins-bot:
[mediawiki/core@master] Fix the cache timestamp for forced updates.

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

Change 481015 had a related patch set uploaded (by Reedy; owner: Daniel Kinzler):
[mediawiki/core@REL1_32] Fix the cache timestamp for forced updates.

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

Reedy added a subscriber: Reedy.EditedDec 20 2018, 6:57 PM

Sooo.. To backport to REL1_32, we also need https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/434544/ from T194046...

That's done in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/481018 (no idea if tests will pass), and put https://gerrit.wikimedia.org/r/481015 ontop of it to see how things fare...

Don't know at this point if more backports/dependancies will be needed. If they are, we should potentially be considering another way to fix the bug in REL1_32 rather than backporting many changes in this RC time

Reedy added a comment.Dec 20 2018, 7:03 PM

Yeah, that doesn't work standalone either

18:57:12 [5d6ddd0c1841c937341f4ecd] [no req]   Error from line 429 of /workspace/src/includes/ServiceWiring.php: Call to undefined method MediaWiki\Revision\RevisionRenderer::setLogger()

So brought in https://github.com/wikimedia/mediawiki/commit/a442f7e6dd5d3ded88e3aa7d117b5c8f6931abb3 before those two as https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/481020...

daniel added a comment.EditedDec 20 2018, 8:15 PM

@Reedy this is really more of a conflict than a dependency. I would recommend against backportinng SlotRoleHandler to 1.32. I'll touch up the original patch tomorrow. Ping me if I forget.

I pushed a new version of https://gerrit.wikimedia.org/r/481015. The reference to SlotRoleHandler is simply not needed in 1.32. I just removed it. No need to drag in the new service.

Change 481015 merged by jenkins-bot:
[mediawiki/core@REL1_32] Fix the cache timestamp for forced updates.

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

I don't think that this is something urgent enough to cherry-pick to production in the first week of January, so we'll just have it ride the train. Needs something in the 1.32 release notes, which I'll write now.

Change 481241 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@REL1_32] RELEASE-NOTES-1.32: Add notes for T210307

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

Change 481241 merged by jenkins-bot:
[mediawiki/core@REL1_32] RELEASE-NOTES-1.32: Add notes for T210307

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

Is this no longer blocking the 1.32 release?

Is this no longer blocking the 1.32 release?

Yeah, it's back-ported to REL1_32 with release notes. I was leaving open until we confirm in production that it's fixed.

Risker added a subscriber: Risker.Jan 8 2019, 9:00 AM
KylieTastic removed a subscriber: KylieTastic.
KylieTastic added a subscriber: KylieTastic.

@Jdforrester-WMF Is this ready to be marked Resolved?

Jdforrester-WMF closed this task as Resolved.Jan 17 2019, 5:40 PM

Sorry, yes, but didn't want to bypass any Core Platform Team processes.