Thanks! There's a possible new case from the last few days at https://commons.wikimedia.org/wiki/Category:Henrik_Harpestrengs_Vej / https://www.wikidata.org/wiki/Q81792970

I haven't done a full analysis, but I'm wondering why this is using the ContentAlterParserOutput hook, instead of RevisionDataUpdates? Conceptually, ContentAlterParserOutput is a filter-style hook that shouldn't have side effects. RevisionDataUpdates is specifically designed for recording information into the database after an edit or re-parse.

EDIT: I think I got this wrong. RevisionDataUpdates is for transferring information from ParserOutput into the database. But this issue is about the opposite - transferring information (namely, the linked item) from the database into the ParserOutput, right?
ContentAlterParserOutput sounds right for that. I'll have to look again.

So, after a little more investigation and reproduction on testwikipedia nd test wikidata

• Create a page on wikipedia that includes some data data from a statement on wikidata item
• The wikipedia page will be parsed and rendered with the data from wikidata
• Add a sitelink from the item to the page
• The page_props will not be updated for the page
• Alter the statement that is being included on the page by editing wikidata
• keep refreshing the page on the wikipedia and the data included on the page will be updated
• page_props will still not be updated
• Perform a null edit on the page
• page_props are finally updated.

Adding core platform team to get some feedback and figure out:

• When exactly this change in behaviour happened?
  • I have looked through various gerrit changes and code paths but i'm sure someone that has worked on this code would be able to figure out exactly when this happened.
  • It's slightly confusing as some stages look like a partial implementation, in some places, that may not have worked as expected etc.
• Does it make sense currently that our property parser function is updated on the page, but the stuff in the ContentAlterParserOutput is not done?
• Is RevisionDataUpdates the right way to go for the page_props update?

Thank you, @Addshore, for investigating this issue!

In T233520#5805312, @Addshore wrote:

Adding core platform team to get some feedback and figure out:

• When exactly this change in behaviour happened?
  • I have looked through various gerrit changes and code paths but i'm sure someone that has worked on this code would be able to figure out exactly when this happened.
  • It's slightly confusing as some stages look like a partial implementation, in some places, that may not have worked as expected etc.

To be honest, I find this code rather confusing myself. Maybe @Anomie or @aaron can help.

• Does it make sense currently that our property parser function is updated on the page, but the stuff in the ContentAlterParserOutput is not done?

That seems wrong. If a new ParserOutput is generated and cached, ContentAlterParserOutput should run. Since it is triggered by Content::getParserOutput, I can't imagine how the PO would get into the ParserCache without this hook running.

Maybe it does run, but doesn't do the deed?

• Is RevisionDataUpdates the right way to go for the page_props update?

No. RevisionDataUpdates is the right place to pull things from ParserOutput and write it to the database. But for page_props, that's already done by the standard update. If the data is in ParserOutput (via setProperty), it should be recorded in the database.

*poke* @Anomie and @aaron to take a look at this.

I’ve just read a (ca. three months old) report mentioning that Yair rand’s WikidataInfo.js tells after connecting that no Wikidata item found. This script queries directly the Wikidata API, so the bug may be that the information gets lost in the Wikibase repo, before the client had any chance to process it. Is it possible?

Poke @CCicalese_WMF This is a pretty large bug on Wikidata.org currently still waiting for feedback from Platform Engineering
Will make sure to add this to SOS(scrum of scrums) again this week.

Thanks for the reminder. I will check on this.

@Addshore Investigating this requires a dev setup with Wikibase. I don't have one handy... I think it would be best if we could spend an hour or so investigating this together. Maybe via hangout next week? Send me an invite...

In T233520#5835442, @daniel wrote:

@Addshore Investigating this requires a dev setup with Wikibase. I don't have one handy... I think it would be best if we could spend an hour or so investigating this together. Maybe via hangout next week? Send me an invite...

Scheduled :)

So we had a call and determined that:

• We really don't know how long this has been broken for
• updating page_touched would probably trigger a reparse of the page (and currently that doesn't happen for the case described)

Other notes:

• Should check that AffectedPagesFinder actually lists the page when adding or removing a sitelink.
• Check the compact diff used for change propagation actually has the sitelink change in it (it probably does)
• Virtual usages comes into play here.

In T233520#5855751, @Addshore wrote:

• updating page_touched would probably trigger a reparse of the page (and currently that doesn't happen for the case described)

In practical terms, WikiPageUpdater should probably just call WikiPage::doPurge(), instead of trying to duplicate the functionality.

• Virtual usages comes into play here.

Being connected via sitelink is the original "virtual usage" considered by AffectedPagesFinder. We discussed also treating the description from wikidata as a virtual usage, since it's visible in the mobile view. Don't remember how that played out.

• Virtual usages comes into play here.

Being connected via sitelink is the original "virtual usage" considered by AffectedPagesFinder. We discussed also treating the description from wikidata as a virtual usage, since it's visible in the mobile view. Don't remember how that played out.

T191831: Track implicit use of non-overridden wikidata description is still open, at least.

Okay, I spent several hours debugging and trying to figure out what's going on and how I0cd13c424 broke it.

Spoiler alert: It's not logic in the hook or issues with virtual usages.

Let's break down the steps to reproduce it:

• Create a page on wikipedia that includes some data from a statement on wikidata item
  • This creates a ParserCache entry without the wikidata data (stuff that goes into page_props) because it's not yet connected to Wikidata
• Add a sitelink from the item to the page
  • This happens in Wikidata, it means it triggers a row in wb_changes table that later gets dispatched to the client wiki
  • The dispatched script, sees a need to run refreshLinks on that page and thus enqueues ChangeNotification job
  • The ChangeNotification job enqueues a refreshLinks job
  • The great refreshLinks job needs a ParserOutput object, it tries to load it but with the change merged it tries to load PO object from PC:
    • Oh there's one PC object already for when the page was made originally and my rootJobTimestamp + 10 seconds is still younger than the original PC
    • Look, the revision id hasn't changed since then, same as page_touched
    • Cool, let's just use the cached PO instead

As the result:

• The page_props will not be updated for the page

The code at fault is this:

	/**
	 * Get the parser output from cache if it reflects the change that triggered this job
	 *
	 * @param ParserCache $parserCache
	 * @param WikiPage $page
	 * @param RevisionRecord $currentRevision
	 * @param StatsdDataFactoryInterface $stats
	 * @return ParserOutput|null
	 */
	private function getParserOutputFromCache(
		ParserCache $parserCache,
		WikiPage $page,
		RevisionRecord $currentRevision,
		StatsdDataFactoryInterface $stats
	) {
		$cachedOutput = null;
		// If page_touched changed after this root job, then it is likely that
		// any views of the pages already resulted in re-parses which are now in
		// cache. The cache can be reused to avoid expensive parsing in some cases.
		$rootTimestamp = $this->params['rootJobTimestamp'] ?? null;
		if ( $rootTimestamp !== null ) {
			$opportunistic = !empty( $this->params['isOpportunistic'] );
			if ( $opportunistic ) {
				// Neither clock skew nor DB snapshot/replica DB lag matter much for
				// such updates; focus on reusing the (often recently updated) cache
				$lagAwareTimestamp = $rootTimestamp;
			} else {
				// For transclusion updates, the template changes must be reflected
				$lagAwareTimestamp = wfTimestamp(
					TS_MW,
					wfTimestamp( TS_UNIX, $rootTimestamp ) + self::NORMAL_MAX_LAG
				);
			}

			if ( $page->getTouched() >= $rootTimestamp || $opportunistic ) {
				// Cache is suspected to be up-to-date so it's worth the I/O of checking.
				// As long as the cache rev ID matches the current rev ID and it reflects
				// the job's triggering change, then it is usable.
				$parserOptions = $page->makeParserOptions( 'canonical' );
				$output = $parserCache->getDirty( $page, $parserOptions );
				if (
					$output &&
					$output->getCacheRevisionId() == $currentRevision->getId() &&
					$output->getCacheTime() >= $lagAwareTimestamp
				) {
					$cachedOutput = $output;
				}
			}
		}

		if ( $cachedOutput ) {
			$stats->increment( 'refreshlinks.parser_cached' );
		} else {
			$stats->increment( 'refreshlinks.parser_uncached' );
		}

		return $cachedOutput;
	}

I was able to confirm that this patch causes it by making the cache function return null all the time and saw the records appearing in page_props.

Options:

• Introduce an opposite of isOpportunistic option and make the refreshlinks jobs enqueued by wikibase to set that.
  • Side note: isOpportunistic is a pretty vague name, what does an "opportunistic" job do? It's also completely undocumented. It's also a string flag, very easily typo-able.
• Make the refreslinks job run the hook when it gets the cached PC. You might need to move it to a public function and invoke it there. It's rather complicated, you need the base content object there.
• Completely drop the whole logic of "let's grab the cached version first". The stats says it's used quite a lot but I think most of them is indeed Wikibase jobs which also makes the first option and this option the same performance-wise.

Once again, the code base seems extremely convoluted and complex, I'm not sure if this complexity is the only option given our complex infra or it can be improved.

cc @aaron @tstarling

Updating page touched would also work?

In T233520#5855858, @daniel wrote:

In T233520#5855751, @Addshore wrote:

• updating page_touched would probably trigger a reparse of the page (and currently that doesn't happen for the case described)

In practical terms, WikiPageUpdater should probably just call WikiPage::doPurge(), instead of trying to duplicate the functionality.

This would update page touched via Title::invalidateCache as far as I can see.

In T233520#5895999, @Addshore wrote:

Updating page touched would also work?

In T233520#5855858, @daniel wrote:

In T233520#5855751, @Addshore wrote:

• updating page_touched would probably trigger a reparse of the page (and currently that doesn't happen for the case described)

In practical terms, WikiPageUpdater should probably just call WikiPage::doPurge(), instead of trying to duplicate the functionality.

This would update page touched via Title::invalidateCache as far as I can see.

This already happens (changing page_touched) through HTMLCacheUpdateJob and that's one of the reasons the problem exists, because the try_to_get_cached_parser_output method (the aforementioned method I pasted) checks if the page_touched is newer than the rootTimeStamp which by definition is (root job is the change notification one that spawns HTMLCacheUpdateJob)

Moving to waiting, waiting on core platform again now I guess

In T233520#5897446, @Ladsgroup wrote:

This already happens (changing page_touched) through HTMLCacheUpdateJob and that's one of the reasons the problem exists, because the try_to_get_cached_parser_output method (the aforementioned method I pasted) checks if the page_touched is newer than the rootTimeStamp which by definition is (root job is the change notification one that spawns HTMLCacheUpdateJob)

So the solution would be for the LinksUpdate job to not inherit the root timestamp?
If that's not possible for some reason, LinksUpdate could have a flag for suppressing the opportunistic behavior, or for providing a different timestamp.

Change 574868 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Remove rootJobTimestamp from refreshLinks jobs in client

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

In T233520#5917452, @daniel wrote:

In T233520#5897446, @Ladsgroup wrote:

This already happens (changing page_touched) through HTMLCacheUpdateJob and that's one of the reasons the problem exists, because the try_to_get_cached_parser_output method (the aforementioned method I pasted) checks if the page_touched is newer than the rootTimeStamp which by definition is (root job is the change notification one that spawns HTMLCacheUpdateJob)

So the solution would be for the LinksUpdate job to not inherit the root timestamp?
If that's not possible for some reason, LinksUpdate could have a flag for suppressing the opportunistic behavior, or for providing a different timestamp.

That's a rather easy way to fix it, yes.

Change 574868 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove rootJobTimestamp from refreshLinks jobs in client

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

Can someone confirm this is working for you now please?

In T233520#5941300, @Lydia_Pintscher wrote:

Can someone confirm this is working for you now please?

It will go live tomorrow evening. Then it can be checked.

Another example: https://www.wikidata.org/wiki/Q100535082 (created 18 October), linking to https://commons.wikimedia.org/wiki/Category:J%C3%BCdenstra%C3%9Fe_(Gotha) .

Interestingly for one of the pages you link to the page_prop now appears, for the other it does not.

# https://commons.wikimedia.org/w/index.php?title=Category:Serangoon_Secondary_School&action=info
mysql:research@dbstore1004.eqiad.wmnet [commonswiki]> select * from page_props where pp_page = 5982384;
Empty set (0.001 sec)

# https://commons.wikimedia.org/w/index.php?title=Category:J%C3%BCdenstra%C3%9Fe_(Gotha)&action=info
mysql:research@dbstore1004.eqiad.wmnet [commonswiki]> select * from page_props where pp_page = 39602411;
+----------+---------------+------------+------------+
| pp_page  | pp_propname   | pp_value   | pp_sortkey |
+----------+---------------+------------+------------+
| 39602411 | wikibase_item | Q100535082 |       NULL |
+----------+---------------+------------+------------+

It looks like the one that does have a page_prop now exist was edited 2 days after your comment.

I've been adding infoboxes on Commons by finding the categories a different way (walking the category tree), when that edit is done it seems to fix the problem. However, I can't always do that, since I'm using page_props to find the candidates to add infoboxes to! See related discussion at https://commons.wikimedia.org/w/index.php?title=User_talk:Mike_Peel&oldid=506602467

I think this is another issue then the one originally reported (the end-user issue is the same though). I think we should open a new ticket.

In T233520#6616386, @Ladsgroup wrote:

I think this is another issue then the one originally reported (the end-user issue is the same though). I think we should open a new ticket.

I'm happy for this task to be split if that helps solve it! Another example seems to be https://commons.wikimedia.org/wiki/Category:Serra_da_Paulista