Page MenuHomePhabricator

Cannot view diffs on Special:Undelete - ends in InvalidArgumentException
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error message
[XcTboQpAIC8AAGLFHhwAAACX] 2019-11-08 03:06:10: Fatal exception of type "InvalidArgumentException"
Impact
  • Cannot view diffs on Special:Undelete
Notes
  • The revisions being compared (20151109221820 and 20151109221748 for the request URL) can be viewed individually.

Attempting to view diffs of a deleted version leads to an internal error

Steps to reproduce:

  1. Have a page with some revisions
  2. Have the access to see deleted revisions
  3. Have the page in (1) deleted
  4. Attempt to "view diff" across deleted revisions
  5. FAIL: ERROR

Details

Request ID
XcTboQpAIC8AAGLFHhwAAACX
Request URL
https://en.wikipedia.org/w/index.php?title=Special:Undelete&target=Elliott+Bisnow&timestamp=20151109221820&diff=prev
Stack Trace
2019-11-08 03:06:10 [XcTboQpAIC8AAGLFHhwAAACX] mw1326 enwiki 1.35.0-wmf.5 exception ERROR: [XcTboQpAIC8AAGLFHhwAAACX] /w/index.php?title=Special:Undelete&target=Elliott+Bisnow&timestamp=20151109221820&diff=prev   InvalidArgumentException from line 3275 of /srv/mediawiki/php-1.35.0-wmf.5/includes/Revision/RevisionStore.php: Revision 689868905 doesn't belong to page 0 {"exception_id":"XcTboQpAIC8AAGLFHhwAAACX","exception_url":"/w/index.php?title=Special:Undelete&target=Elliott+Bisnow&timestamp=20151109221820&diff=prev","caught_by":"mwe_handler"} 
[Exception InvalidArgumentException] (/srv/mediawiki/php-1.35.0-wmf.5/includes/Revision/RevisionStore.php:3275) Revision 689868905 doesn't belong to page 0
  #0 /srv/mediawiki/php-1.35.0-wmf.5/includes/Revision/RevisionStore.php(3482): MediaWiki\Revision\RevisionStore->assertRevisionParameter(string, integer, MediaWiki\Revision\RevisionArchiveRecord)
  #1 /srv/mediawiki/php-1.35.0-wmf.5/includes/diff/DifferenceEngine.php(1552): MediaWiki\Revision\RevisionStore->countRevisionsBetween(integer, MediaWiki\Revision\RevisionArchiveRecord, MediaWiki\Revision\RevisionArchiveRecord, integer)
  #2 /srv/mediawiki/php-1.35.0-wmf.5/includes/diff/DifferenceEngine.php(1052): DifferenceEngine->getMultiNotice()
  #3 /srv/mediawiki/php-1.35.0-wmf.5/includes/specials/SpecialUndelete.php(583): DifferenceEngine->getDiff(string, string)
  #4 /srv/mediawiki/php-1.35.0-wmf.5/includes/specials/SpecialUndelete.php(429): SpecialUndelete->showDiff(Revision, Revision)
  #5 /srv/mediawiki/php-1.35.0-wmf.5/includes/specials/SpecialUndelete.php(204): SpecialUndelete->showRevision(string)
  #6 /srv/mediawiki/php-1.35.0-wmf.5/includes/specialpage/SpecialPage.php(575): SpecialUndelete->execute(NULL)
  #7 /srv/mediawiki/php-1.35.0-wmf.5/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(NULL)
  #8 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
  #9 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(967): MediaWiki->performRequest()
  #10 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(530): MediaWiki->main()
  #11 /srv/mediawiki/php-1.35.0-wmf.5/index.php(46): MediaWiki->run()
  #12 /srv/mediawiki/w/index.php(3): require(string)
  #13 {main}

Event Timeline

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

Based on the stack trace, those are not the same. This one isn't limited to a particular page or revision.

Side note: Merging duplicates is annoying, it breaks other relationship.

Xaosflux renamed this task from Cannot view diffs on Special:Undelete to Cannot view diffs on Special:Undelete - ends in InvalidArgumentException.Nov 9 2019, 9:22 PM
Xaosflux updated the task description. (Show Details)

Side note: Merging duplicates is annoying, it breaks other relationship.

Not sure what you mean here. Duplicates should not be merged?

it breaks other relationship.

I am not sure what you meant by that.

@Ammarpad @Masumrezarock100 no no they should be according to our current workflow, not a complaint on your part! Just a general complaint about phabricator. For example if a ticket has a parent/child relationship, and is merged - the other ticket (generally the earlier of the duplicates) doesn't inherit that linkage.

@Ammarpad @Masumrezarock100 no no they should be according to our current workflow, not a complaint on your part! Just a general complaint about phabricator. For example if a ticket has a parent/child relationship, and is merged - the other ticket (generally the earlier of the duplicates) doesn't inherit that linkage.

I see that. Thanks for the clarification.

Probably introduced by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/543718. Pinging @Pchelolo.

It seems like getMultiNotice() cannot work reliably for deleted revisions, since deleted revisions for the same title may have different page IDs, or may have page ID 0 for old entries. It should probably be skipped if the two revisions don't belong to the same page, or don't match $this->mNewPage->getArticleID().

since deleted revisions for the same title may have different page IDs, or may have page ID 0 for old entries.

Maybe this is (one of) the cause. I noticed it on page deleted&recreated twice - can't compare deleted revisions (even to revisions belonged to the same creation) but possible to retrieve versions. Page created and deleted today.

Change 552108 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] DifferenceEngine: Don't try counting revs between deleted revs with different page IDs

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

Change 552108 merged by jenkins-bot:
[mediawiki/core@master] DifferenceEngine: Don't try counting revs between deleted revs with different page IDs

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

Anomie closed this task as Resolved.EditedNov 20 2019, 9:53 PM
Anomie claimed this task.
Anomie subscribed.

The fix should be deployed to Wikimedia sites with 1.35.0-wmf.8 1.35.0-wmf.9.[1] See https://www.mediawiki.org/wiki/MediaWiki_1.35/Roadmap for a schedule.

[1]: 1.35.0-wmf.8 is apparently going to be deployed to group 0, but will skip groups 1 and 2 due to the US holiday next week. Presumably all wikis will go to wmf.9 as usual the following week.

I note that this was intended to have a fix, but I'm still receiving the same error on the English Wikipedia.

[Xevv8wpAAEQAADyMBbYAAAFT] 2019-12-07 18:31:16: Fatal exception of type "InvalidArgumentException"

If I'm reading the roadmap correctly, the fix is due to roll out for enwiki as part of 1.35.0-wmf.10, on December 12th.

Somebody please correct me if I've misunderstood the roadmap.

I note that this was intended to have a fix, but I'm still receiving the same error on the English Wikipedia.

[Xevv8wpAAEQAADyMBbYAAAFT] 2019-12-07 18:31:16: Fatal exception of type "InvalidArgumentException"

The "problem" is that "resolving" tickets doesn't mean resolving "incidents" or "problems".

The "problem" is that "resolving" tickets doesn't mean resolving "incidents" or "problems".

See "Resolved" on https://www.mediawiki.org/wiki/Bug_management/Bug_report_life_cycle

The "problem" is that "resolving" tickets doesn't mean resolving "incidents" or "problems".

That's true. In theory, there should be an engineering task derived from the bug report once the case has been identified. The engineering task gets closed when the patch is merged, the bug report when the fix is deployed. Unfortunately this is annoying overhead to be handled manually, so we(*) often don't do it.

(*) in this case, I.

If I'm reading the roadmap correctly, the fix is due to roll out for enwiki as part of 1.35.0-wmf.10, on December 12th.

Somebody please correct me if I've misunderstood the roadmap.

It would be in wmf.8; the assumption I made in T237709#5680065 that wmf.8 was going to be skipped in favor of wmf.9 seems to not have been accurate. But wmf.8 hasn't made it past group 0 due to various issues (T233856). At this point I don't know whether wmf.8 will wind up being deployed at all or if we'll go straight to wmf.10.

The "problem" is that "resolving" tickets doesn't mean resolving "incidents" or "problems".

I remember complaining about that 10 years ago. ;) That was back in the days when getting a fix deployed took months or years, rather than a week or two.

But from a "work tracking" perspective it does make sense: absent the issue somehow not being completely fixed after all, there's no more work to do specific to this task. The deployment that we're waiting for is a bulk deployment of all the changes since 1.35.0-wmf.5. With the way things are, if we left the task open until the deployment actually happens we'd probably actually wind up with no one remembering to actually close it. If we wanted to only close tasks post-deploy, we'd need some sort of "waiting for deployment" queue that someone (human or bot) would go through to make sure all the tasks actually get closed. Maybe that would be a good model to change to, but if so it should be proposed as its own task.

It would be in wmf.8; the assumption I made in T237709#5680065 that wmf.8 was going to be skipped in favor of wmf.9 seems to not have been accurate. But wmf.8 hasn't made it past group 0 due to various issues (T233856). At this point I don't know whether wmf.8 will wind up being deployed at all or if we'll go straight to wmf.10.

The "problem" is that "resolving" tickets doesn't mean resolving "incidents" or "problems".

I remember complaining about that 10 years ago. ;) That was back in the days when getting a fix deployed took months or years, rather than a week or two.

But from a "work tracking" perspective it does make sense: absent the issue somehow not being completely fixed after all, there's no more work to do specific to this task. The deployment that we're waiting for is a bulk deployment of all the changes since 1.35.0-wmf.5. With the way things are, if we left the task open until the deployment actually happens we'd probably actually wind up with no one remembering to actually close it. If we wanted to only close tasks post-deploy, we'd need some sort of "waiting for deployment" queue that someone (human or bot) would go through to make sure all the tasks actually get closed. Maybe that would be a good model to change to, but if so it should be proposed as its own task.

I'm more used to systems like that, so it seems I may have misinterpreted. Generally, "closed" would mean in those instances that a fix has been released to production, and generally verified to be working there. Before that, the ticket would be in an "Approved", "Passed QA", or "Release Ready" state.