Change 863886 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: Use latest rev instead of page ID in cache key

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

It seems to be happen only if the edit is made using VisualEditor (thanks @Bean49 and JSoos for debugging!). The current code relies on the page.page_touched DB column for invalidating the cache, any chance VE doesn’t properly update that?

That seems very unlikely to me. VisualEditor uses mostly standard API calls for editing pages, so if it was broken in that way, I'd expect every other tool using the API for editing to also be affected (e.g. the mobile wikitext editor, or just about every bot).

It's not impossible that something special about VE edits is causing issues, but I'm not really convinced right now that it's worth investigating. Our of curiosity, what's leading you to think VE edits are the cause?

In T324422#8986804, @matmarex wrote:

It's not impossible that something special about VE edits is causing issues

I will report a related issue. I can reproduce it only with VisualEditor. https://hu.wikipedia.org/wiki/MediaWiki:Revreview-newest-basic-i is displayed instead of https://hu.wikipedia.org/wiki/MediaWiki:Revreview-newest-basic. You can see it here: https://hu.wikipedia.org/wiki/FlaggedRevs_%C3%BCzenetek_tesztel%C3%A9se?uselang=qqx&stable=0. I couldn't reproduce it with index.php and api.php.

In T324422#8986804, @matmarex wrote:

That seems very unlikely to me. VisualEditor uses mostly standard API calls for editing pages, so if it was broken in that way, I'd expect every other tool using the API for editing to also be affected (e.g. the mobile wikitext editor, or just about every bot).

(Flagged) bots aren’t affected by the zero vs non-zero issue – they have autoreview right, so if the page had no unreviewed edits before the bot edit, it will continue to have zero unreviewed edits. They can be affected by the smaller-but-still-positive issue, though (but that’s less likely to be spot). The mobile wikitext editor can be affected by both issues.

Our of curiosity, what's leading you to think VE edits are the cause?

The fact that @Bean49 and JSoos could only reproduce the bug using VE, not using the 2006/2010 wikitext editor or the action API. (I didn’t prove this myself, I believe them.)

In T324422#8987112, @Bean49 wrote:

I will report a related issue.

Related but separate issues should be reported separately. However, it’s not a separate issue – it’s exactly what I described in the second bullet point in the description.

I had a look today.

I was able to find an affected page quickly by just looking for edits pending review on recent changes: https://hu.wikipedia.org/wiki/Remények_földje

But the problem doesn't seem to be related to the page_touched field – you can query its value using the API, and for this page, it's the same as the time of the latest edit: https://hu.wikipedia.org/wiki/Speci%C3%A1lis:ApiSandbox#action=query&format=json&prop=info|revisions&titles=Rem%C3%A9nyek%20f%C3%B6ldje&formatversion=2&rvprop=timestamp

{
    "batchcomplete": true,
    "query": {
        "pages": [
            {
                "pageid": 1675073,
                "ns": 0,
                "title": "Remények földje",
                "contentmodel": "wikitext",
                "pagelanguage": "hu",
                "pagelanguagehtmlcode": "hu",
                "pagelanguagedir": "ltr",
                "touched": "2023-07-04T19:58:13Z",
                "lastrevid": 26271131,
                "length": 10313,
                "revisions": [
                    {
                        "timestamp": "2023-07-04T19:58:13Z"
                    }
                ]
            }
        ]
    }
}

I compared the touched and timestamp fields for all recent changes (as many as the API will give me in a single query), and in all of them, touched is the same or more recent than timestamp (regardless of using VE or not). So I'm confident that updating this field is working correctly. I used the following command to do that:

curl "https://hu.wikipedia.org/w/api.php?action=query&format=json&prop=info%7Crevisions&generator=recentchanges&formatversion=2&rvprop=timestamp&grcnamespace=0&grctag=visualeditor&grclimit=max&grctype=edit&grctoponly=1" | jq ".query.pages[] | {pageid, lastrevid, touched, timestamp: .revisions[0].timestamp} | select(.touched < .timestamp)"

However, you're absolutely right that this problem seems to be related to the edit being made using the visual editor. I looked at recent changes pending review using this query: https://hu.wikipedia.org/wiki/Speciális:Friss_változtatások?hidepreviousrevisions=1&hidenewpages=1&hidecategorization=1&hideWikibase=1&hidelog=1&hidenewuserlog=1&flaggedrevs=needreview&namespace=0&limit=500&days=7&tagfilter__visualeditor_color=c1&urlversion=2 and compared VE edits and non-VE edits. 6 out of 10 VE edits had the bug, while 0 out of 10 non-VE edits did.

By the way, purging the page (e.g. https://hu.wikipedia.org/wiki/Stefanovics_Angéla?action=purge) resolves the issue. Purging is one of the things that updates the page_touched field, so it looks like invalidation using that field works correctly at least in some cases…

My guess for how this bug happens:

I said earlier that "VisualEditor uses mostly standard API calls", but it actually has its own action=visualeditoredit API, that converts HTML to wikitext using Parsoid, then calls the standard action=edit API internally, and then renders the new HTML of the page using the standard parser and returns some metadata, so that it can be shown to the user who saved the edit.

One of the pieces of metadata it computes is the notice about pending changes. This happens here: https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/c9fceaedaa3bcf34041d9867e6b8b6f9a2327795/includes/ApiVisualEditorEdit.php#465 The FlaggedRevs code is hard to follow, but I suspect this ends up calling the code that calculates the number of pending revisions. That code reads that number from the replica database: https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/+/1f835edcaf907bb22e7c429d792a00734c2dd878/backend/FlaggableWikiPage.php#194 Since this happens in the same API request, it's likely that the newly saved revision isn't present in that replica yet, so it finds 0 pending revisions. This then gets cached. (I don't understand why it doesn't get immediately invalidated when page_touched changes, but clearly it doesn't.)

If I'm right, there are a few ways to fix this:

• Your patch. We will still compute the bogus 0 value, but the cache key will not match the one used when displaying the page afterwards, so it will be ignored.
• Make FlaggedRevs calculate the number from the primary database when called from the VisualEditor API request. We would probably need a minor complication to avoid doing that on normal page views.
• Just delete the VisualEditor–FlaggedRevs integration code. I just realized it doesn't work. We do all this work to compute the notice, and then we don't display it to the user. It seems I broke this in rEVED9f9966d1cda1: ApiVisualEditorEdit: Use action=parse 'subtitle' option nearly three years ago. I can't find any bug reports about it, but I guess no one cares to report bugs in FlaggedRevs any more.

In T324422#8989274, @matmarex wrote:

but I guess no one cares to report bugs in FlaggedRevs any more.

Well, that is not true, I do that quite frequently (I think)! :) But I am not familiar with this particular problem.

Heh, to clarify, I didn't mean to put the blame on you or other users (I realize now that what I wrote may be misunderstood) – what I meant is that, because you rarely get a response to FlaggedRevs bug reports, most users understandably don't want to spend their time filing them. I also don't report bugs myself when I know that the developers won't fix them. Thank you for doing that anyway! :)

In T324422#8989274, @matmarex wrote:

My guess for how this bug happens:

I said earlier that "VisualEditor uses mostly standard API calls", but it actually has its own action=visualeditoredit API, that converts HTML to wikitext using Parsoid, then calls the standard action=edit API internally, and then renders the new HTML of the page using the standard parser and returns some metadata, so that it can be shown to the user who saved the edit.

One of the pieces of metadata it computes is the notice about pending changes. This happens here: https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/c9fceaedaa3bcf34041d9867e6b8b6f9a2327795/includes/ApiVisualEditorEdit.php#465 The FlaggedRevs code is hard to follow, but I suspect this ends up calling the code that calculates the number of pending revisions. That code reads that number from the replica database: https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/+/1f835edcaf907bb22e7c429d792a00734c2dd878/backend/FlaggableWikiPage.php#194 Since this happens in the same API request, it's likely that the newly saved revision isn't present in that replica yet, so it finds 0 pending revisions. This then gets cached. (I don't understand why it doesn't get immediately invalidated when page_touched changes, but clearly it doesn't.)

Wait, if $this->getTouched() doesn’t change, doesn’t that mean that it’s already the new one, read from the primary database? If it is, then probably $this->getLatest() will also be the new one, and my patch won’t fix anything. I found the following code in WikiPage::loadPageData (rMW includes/page/WikiPage.php:442-447 (at 443b1b7ea511)):

			if ( !$data
				&& $index == DB_REPLICA
				&& $loadBalancer->hasReplicaServers()
				&& $loadBalancer->hasOrMadeRecentPrimaryChanges()
			) {
				$from = self::READ_LATEST;

• Just delete the VisualEditor–FlaggedRevs integration code. I just realized it doesn't work. We do all this work to compute the notice, and then we don't display it to the user. It seems I broke this in rEVED9f9966d1cda1: ApiVisualEditorEdit: Use action=parse 'subtitle' option nearly three years ago. I can't find any bug reports about it, but I guess no one cares to report bugs in FlaggedRevs any more.

I think it should be fixed rather than removed. Since the notice mostly appears for not confirmed users, i.e. newbies, who will neither find Phabricator nor notice that there’s a regression (since they don’t know how it worked before), it’s not that surprising that it hasn’t been reported before.

In T324422#8989582, @Tacsipacsi wrote:

Wait, if $this->getTouched() doesn’t change, doesn’t that mean that it’s already the new one, read from the primary database? If it is, then probably $this->getLatest() will also be the new one, and my patch won’t fix anything.

Hmm, maybe. Well, let's try it anyway, it shouldn't hurt and at least we'll learn something.

I think it should be fixed rather than removed. Since the notice mostly appears for not confirmed users, i.e. newbies, who will neither find Phabricator nor notice that there’s a regression (since they don’t know how it worked before), it’s not that surprising that it hasn’t been reported before.

I filed T341295 with an outline of a plan and some rationale. The message about requiring review would still be shown, we'd just reload the whole page to do it. I think that, given the resources we're willing to spend on FlaggedRevs, this will work better. Let's discuss in that task.

Change 863886 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: Use latest rev instead of page ID in cache key

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

Change 940475 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: If WikiPage reads from primary DB, follow it

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

(Just for future reference: the previous change did not seem to have any effect, as you predicted.)

Huwiki village pump says the issue has disappeared recently (permalink).

In T324422#9426070, @Tgr wrote:

Huwiki village pump says the issue has disappeared recently (permalink).

Indeed, repeating my test from earlier:

In T324422#8989208, @matmarex wrote:

However, you're absolutely right that this problem seems to be related to the edit being made using the visual editor. I looked at recent changes pending review using this query: https://hu.wikipedia.org/wiki/Speciális:Friss_változtatások?hidepreviousrevisions=1&hidenewpages=1&hidecategorization=1&hideWikibase=1&hidelog=1&hidenewuserlog=1&flaggedrevs=needreview&namespace=0&limit=500&days=7&tagfilter__visualeditor_color=c1&urlversion=2 and compared VE edits and non-VE edits. 6 out of 10 VE edits had the bug, while 0 out of 10 non-VE edits did.

…I don't see any affected pages.

I couldn't find any code change that could have plausibly fixed this.

The issue has returned:

• https://hu.wikipedia.org/wiki/Hallelujah!_(%C3%A9nekesk%C3%B6nyv)?stable=0&uselang=en
• https://hu.wikipedia.org/wiki/Hallelujah!_(%C3%A9nekesk%C3%B6nyv)?stable=0&uselang=en&action=history

The issue has been reported in plwiki tech village pump as well: https://pl.wikipedia.org/wiki/Wikipedia:Kawiarenka/Kwestie_techniczne#X_zmian%28a%29_oczekuje_na_przejrzenie

While checking, I've opened some random entries on RecentChanges that had at least two recent changes done by non-editor and everywhere the number of pending changes was too low by one. It did not matter if the edits were done by the same person of not (but I didn't see the problem where the edits weren't done using VE).

In T324422#9982630, @Msz2001 wrote:

(but I didn't see the problem where the edits weren't done using VE).

If this behaviour depends on VE usage and problem is where VE was used... T368380#9979324