[go: up one dir, main page]

Page MenuHomePhabricator

FlaggedRevs: moved page gets unreviewed
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Use account with (autoreview) permission
  • Navigate to a wiki with FlaggedRevs enabled (eg. plwiki)
  • Find a page where the newest version is already reviewed
  • Move the page

What happens?:
The page is unreviewed after being moved.

What should have happened instead?:
The page should be still reviewed.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia): plwiki

Other information (browser name/version, screenshots, etc.):

image.png (600×1 px, 117 KB)

You can find in move log that every page that was moved from and to FlaggedRevs namespace has been later manually reviewed (it should be done automatically, though).

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Unable to reproduce on localhost using plwiki settings and an admin account (which has autoreview). The steps to reproduce may be a bit more complicated than shown in the ticket. Any ideas on how to improve the steps to reproduce?

image.png (669×1 px, 159 KB)

image.png (615×1 px, 93 KB)

Renaming without a redirect definitely produces an unpatrolled page 100% of the time; can you check that?

Good idea, but also didn't work for me in localhost.

image.png (139×1 px, 42 KB)

Try to rename any page on a live ruwiki (see merged task).

Here's screen recording of me reproducing the bug on plwiki: (I was doing it in Help: namespace but it's not significant for the problem)

Hi, The bug is related to git a2989af2, I reverted and bug was solved for Patch Demo.

Hi, The bug is related to git a2989af2, I reverted and bug was solved for Patch Demo.

I don't see how that patch could have caused the issue described here, since it's not yet live on huwiki or plwiki.
That change was only morged on July 1, it is being rolled out with this week's train - it's in the 1.43.0-wmf.12 branch, which hasn't hit group 2 yet (it will tomorrow, if the train stays on schedule). But right now, https://pl.wikipedia.org/wiki/Specjalna:Wersja still has 1.43.0-wmf.11.

Can you reproduce the issue in Patch Demo on 1.43.0-wmf.11?

Based on timing, this patch is a more likely culprit: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/1047111. Though I wouldn't have expecte it to go live on the relevant wikis before June 27, while the bug was reported on June 25.

Another option would be https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/1043047

None of these seems problematic in an obvious way, but who knows.

@daniel, I have no idea, I found this issue on arwiki and found this as an open ticket so I reported what I knew.

I'm also confused.

If this is API-related, it'd help to know what the request and response actually look like (including request body). If it is convenient, could someone reproduce the issue and post the relevant information from their browser developer tools?

If that's not convenient, I can set up FlaggedRevs locally and do it, but it'd be next week.

Auto-reviews should not involve the web API at all, right?...

They shouldn’t. Which also means ruling out d8c86b1238b3e4e300356140d36a70051599eb7b (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/1047111). (By the way, the timing also rules it out: it landed with wmf.11 – the bug was first reported on Tuesday, June 25, but wmf.11 reached group2 wikis like plwiki only on Thursday, June 27 (T366956#9931834).)

The only non-localization update patch in wmf/1.43.0-wmf.9..wmf/1.43.0-wmf.10 is b4c528fe2d9e59f319a112e93f9d3cd3c1c7d2fd (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/1043047). There are three possibilities:

I've browsed through the move log at plwiki and the issue started appearing between Wed, 2024-06-19 19:39 UTC (the last move that was automatically reviewed) and Thu, 2024-06-20 09:11 UTC (the first move that wasn't auto-reviewed but should be).

wmf.10 was indeed deployed in this timeslot (at 08:16 UTC according to T361404#9908513)

FWIW, I tried to reproduce this locally, but without success. That indicates that it's not an obvious bug in FlaggedRevs, but some subtle interaction, possibly with caching, possibly with something else.

I've browsed through the move log at plwiki and the issue started appearing between Wed, 2024-06-19 19:39 UTC (the last move that was automatically reviewed) and Thu, 2024-06-20 09:11 UTC (the first move that wasn't auto-reviewed but should be).

wmf.10 was indeed deployed in this timeslot (at 08:16 UTC according to T361404#9908513)

Indeed, the dewiki logs give an even more accurate approximation, which confirms this: autoreviewed move at 07:22 UTC (09:22 CEST), non-autoreviewed move by autochecked user at 08:19 UTC (10:19 CEST).

FWIW, I tried to reproduce this locally, but without success. That indicates that it's not an obvious bug in FlaggedRevs, but some subtle interaction, possibly with caching, possibly with something else.

Maybe another extension aborts a hook FlaggedRevs depends on?

We could try writing a page move integration test. If it passes in localhost and fails in CI (which loads a ton of extensions), that would support the hypothesis of another extension interfering with something in FlaggedRevs.

Change #1052142 had a related patch set uploaded (by Novem Linguae; author: Novem Linguae):

[mediawiki/extensions/FlaggedRevs@master] tests: add page move test

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

OK, I wrote a page move integration test (see patch above). It passes in both my localhost and CI 🤷‍♂️

OK, I wrote a page move integration test (see patch above). It passes in both my localhost and CI 🤷‍♂️

We don't run tests for all the extensions in CI...

But it could be related to something else, like some complex combination of permissions. Or caching.

Change #1052142 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] tests: add page move test

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

On wikisource we move quite often pages with several subpages (move subpages). It is a very convenient semi-batch mode. The bug is most disturbing...

This comment was removed by Wargo.

Maybe it should be test on Parsoid? Do we need $poutput stuff during autoreview?

Maybe another extension aborts a hook FlaggedRevs depends on?

At a quick skim I don't see any PageMoveComplete hook handler in Wikimedia production that would ever return false.

Maybe another extension aborts a hook FlaggedRevs depends on?

At a quick skim I don't see any PageMoveComplete hook handler in Wikimedia production that would ever return false.

RevisionFromEditComplete is also called during moving.

aaron triaged this task as Medium priority.Jul 25 2024, 3:36 PM
aaron added a project: MW-Interfaces-Team.

This is a really annoying bug when you have to do a lot of renames because then the redirects and the renamed pages need reviewing. Maybe we should test it in Patchdemo instead of locally? Or is it not close enough, too?

Is the issue reproducible in Beta? If not, I think the path of least resistance is to book a deploy session, add some logging on one of the production debug hosts, and check via X-Wikimedia-Debug whether 1) the hook handler actually executes, 2) if not, which hook handlers execute and which don't, 3) if yes, just dump a lot of information inside the hook and hope to spot what's going wrong.

Locally, I am unable to reproduce this issue. Is there any update?

Locally, I am unable to reproduce this issue.

It was unreproduceable locally from the beginning (T368380#9937740) but still persists on production.

doctaxon raised the priority of this task from Medium to High.Oct 2 2024, 1:40 PM

Because it increases the amount of rework required more and more, I raise the priority to High.

Aklapper lowered the priority of this task from High to Medium.Oct 2 2024, 3:13 PM

@doctaxon: Do you plan to work on fixing this task, as you increased the priority of this task? You are welcome to set priority && set yourself as task assignee if you plan to work on fixing this, as the Priority field summarizes and reflects reality and does not cause it.

To be precise: edits after (inclusive) page move are unrevieved. All edits before retain status. Redirect is autoreviewed, even after T379218.

You can test on de.wikipedia.beta.wmflabs.org

New info.
If bot reviews these edits, they are marked as... autoreviewed instead of reviewed by <username>.

Examples here: https://pl.wikipedia.org/wiki/Dyskusja_wikipedysty:MszBot/Przegl%C4%85danie_stron#Rejestr

If anyone is interested in that, I've created a bot to automatically review moved and created pages (this is the bot that Wargo talks about above). It works in real time, based on EventStreams, so as to make an impression that this issue is no longer present. (Review log from plwiki).

I can run it on other wikis as well if the community is interested.

Change #1088672 had a related patch set uploaded (by Wargo; author: Wargo):

[mediawiki/extensions/FlaggedRevs@master] Use RevisionRecord provided by onPageMoveComplete

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

Unfortunately, fix to T379218: New pages are not autoreviewed despite creator having autoreview right doesn't fix this issue, so moved pages still become unreviewed.

I still can't reproduce this locally.

Has anyone successfully reproduced this on beta cluster? If so, maybe we can try merging https://gerrit.wikimedia.org/r/1088672 and seeing if that patch fixes this.

Has anyone successfully reproduced this on beta cluster?

On Beta Cluster this problem also appears. But not on wikis non-prepared to be on Wikimedia (Patchdemo, private wikis...).

https://de.wikipedia.beta.wmflabs.org/w/index.php?title=Flaggedrevs-202411/move/try2/2&action=history

Change #1088672 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Use RevisionRecord provided by onPageMoveComplete

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

Wargo claimed this task.