Page MenuHomePhabricator

[Bug] Fatal Wikibase PatcherException from EntityContent.php when undoing edits
Closed, ResolvedPublic8 Estimated Story PointsPRODUCTION ERROR

Description

/w/index.php?title=Q4060911&action=edit&undoafter=211322910&undo=212116913   Diff\Patcher\PatcherException from line 586 of /srv/mediawiki/php-1.26wmf2/extensions/Wikidata/extensions/Wikibase/repo/includes/content/EntityContent.php: EntityContent must not contain Entity data as well as a redirect after applying the patch!
#0 /srv/mediawiki/php-1.26wmf2/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(314): Wikibase\EntityContent->getPatchedCopy()
#1 /srv/mediawiki/php-1.26wmf2/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(262): Wikibase\EditEntityAction->showUndoForm()
#2 /srv/mediawiki/php-1.26wmf2/includes/MediaWiki.php(395): Wikibase\EditEntityAction->show()
#3 /srv/mediawiki/php-1.26wmf2/includes/MediaWiki.php(273): MediaWiki->performAction()
#4 /srv/mediawiki/php-1.26wmf2/includes/MediaWiki.php(566): MediaWiki->performRequest()
#5 /srv/mediawiki/php-1.26wmf2/includes/MediaWiki.php(414): MediaWiki->main()
#6 /srv/mediawiki/php-1.26wmf2/index.php(46): MediaWiki->run()
#7 /srv/mediawiki/w/index.php(3): include()

Possible cause: trying to undo an edit that turned an entity into a redirect (or vice versa), when that edit is not the latest edit (that is, when the undo involved diff-based patching).

Reproduction path:

  1. Create a new Item with a single label
  2. Remove the label
  3. Redirect the now empty Item to another Item
  4. Go to the history page and attempt to undo the edit that removed the label

Possible fix: when an undo is requested, first check whether the current revision has the same redirect status as the revision to be undone. If not, undo should fail gracefully.

Acceptance Criteria

  • reproducing the issue results in an error displayed neatly to the user, something along the lines of "undo not possible"
  • Error does not appear in production logs

Event Timeline

aude raised the priority of this task from to Needs Triage.
aude updated the task description. (Show Details)
aude subscribed.
Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptApr 24 2015, 4:58 PM
JeroenDeDauw renamed this task from Uncaught exception from line 586 in Diff\Patcher\PatcherException to Uncaught exception from line 586 in content/EntityContent.php.Apr 26 2015, 2:35 AM
JeroenDeDauw set Security to None.

still happens....

/w/index.php?title=Q18326133&action=edit&undoafter=174122461&undo=197893176 Diff\Patcher\PatcherException from line 586 of /srv/mediawiki/php-1.26wmf9/extensions/Wikidata/extensions/Wikibase/repo/includes/content/EntityContent.php: EntityContent must not contain Entity data as well as a redirect after applying the patch!
#0 /srv/mediawiki/php-1.26wmf9/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(313): Wikibase\EntityContent->getPatchedCopy()
#1 /srv/mediawiki/php-1.26wmf9/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(261): Wikibase\EditEntityAction->showUndoForm()
#2 /srv/mediawiki/php-1.26wmf9/includes/MediaWiki.php(413): Wikibase\EditEntityAction->show()
#3 /srv/mediawiki/php-1.26wmf9/includes/MediaWiki.php(291): MediaWiki->performAction()
#4 /srv/mediawiki/php-1.26wmf9/includes/MediaWiki.php(634): MediaWiki->performRequest()
#5 /srv/mediawiki/php-1.26wmf9/includes/MediaWiki.php(431): MediaWiki->main()
#6 /srv/mediawiki/php-1.26wmf9/index.php(41): MediaWiki->run()
#7 /srv/mediawiki/w/index.php(3): include()

aude renamed this task from Uncaught exception from line 586 in content/EntityContent.php to [Bug] Uncaught exception from line 586 in content/EntityContent.php.Aug 13 2015, 8:26 PM

probably still happens, though not 100% sure about that

this still happens

/w/index.php?title=Q17335081&action=edit&undoafter=211251020&undo=239750949 Diff\Patcher\PatcherException from line 585 of /srv/mediawiki/php-1.26wmf17/extensions/Wikidata/extensions/Wikibase/repo/includes/content/EntityContent.php: EntityContent must not contain Entity data as well as a redirect after applying the patch! {"exception":"[Exception Diff\\Patcher\\PatcherException] (/srv/mediawiki/php-1.26wmf17/extensions/Wikidata/extensions/Wikibase/repo/includes/content/EntityContent.php:585) EntityContent must not contain Entity data as well as a redirect after applying the patch!\n[stacktrace]\n#0 /srv/mediawiki/php-1.26wmf17/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(312): Wikibase\\EntityContent->getPatchedCopy(Wikibase\\Repo\\Content\\EntityContentDiff)\n#1 /srv/mediawiki/php-1.26wmf17/extensions/Wikidata/extensions/Wikibase/repo/includes/actions/EditEntityAction.php(260): Wikibase\\EditEntityAction->showUndoForm()\n#2 /srv/mediawiki/php-1.26wmf17/includes/MediaWiki.php(456): Wikibase\\EditEntityAction->show()\n#3 /srv/mediawiki/php-1.26wmf17/includes/MediaWiki.php(255): MediaWiki->performAction(Article, Title)\n#4 /srv/mediawiki/php-1.26wmf17/includes/MediaWiki.php(677): MediaWiki->performRequest()\n#5 /srv/mediawiki/php-1.26wmf17/includes/MediaWiki.php(474): MediaWiki->main()\n#6 /srv/mediawiki/php-1.26wmf17/index.php(41): MediaWiki->run()\n#7 /srv/mediawiki/w/index.php(3): include(string)\n#8 {main}\n"}

daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)

I just looked into this very briefly, because it still happens in production, and this is happens on redirected entities. Someone is trying to undo an edit on the entity content that was present before the entity got converted into a redirect.

The root cause is us trying to "patch" a diff on undo on an redirected entity, which leaves the entity redirect in place, but also (sometimes) adds actual entity content to the content object.

Change 273236 had a related patch set uploaded (by JanZerebecki):
[WIP] Handle conflicts when applying patch from undo

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

Change 273236 abandoned by Addshore:
[WIP] Handle conflicts when applying patch from undo

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

Krinkle renamed this task from [Bug] Uncaught exception from line 586 in content/EntityContent.php to [Bug] Fatal Wikibase PatcherException from EntityContent.php when undoing edits.Sep 26 2018, 3:44 AM

Still seen on 1.32.0-wmf.23 on www.wikidata.org and test.wikidata.org from edit submissions that used the "Undo" button.

Current repro at https://www.wikidata.org/w/index.php?title=Q53764875&action=edit&undoafter=783222619&undo=783223054

โ€ข mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:12 PM

Just saw 11 of these on 1.35.0-wmf.28. logspam-watch output:

11                   Patcher    wmf.28 e/W/r/i/C/EntityContent.php:544  EntityContent must not contain Entity data as well as a redirect after applying the patch!

This still happens, we just had a spike of 18 of them within two minutes around 2020-12-06T23:53Z.

Prio Notes:

  • Started off voted as 5,5,5,15. 15) Has been around for a while, would only cause marginally less logs. 5s) Michael) On the one hand it's less important than (a ticket restoring functionality), but still pretty important as its a prod error causing log spam. Shouldnt diddle too long. Lucas) Does keep happening to different deployers and IMs. <Discussion about how many logs we have in production, and how valuable this is right now>
  • We put it at 5, then slowly moved it up

From the task inspection:
On this line https://gerrit.wikimedia.org/g/mediawiki/extensions/Wikibase/+/937beeddb3a8053b91595f7771cbc07322439b48/repo/includes/Actions/EditEntityAction.php#272 as mentioned in the task description, when an undo is requested, first check whether the current revision has the same redirect status as the revision to be undone. If not, undo should fail, i.e. show an error via $this->getOutput()->addHTML and $this->getOutput()->addWikiMsg with an error message that still needs to be defined.

Change 655724 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Remove stray <p>/</p> tags from EditEntityAction

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

Change 655725 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Migrate EditEntityActionTest test cases to yield syntax

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

Change 655726 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Handle error when mixing redirects in undo

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

Change 655724 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove stray <p>/</p> tags from EditEntityAction

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

Change 655725 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Migrate EditEntityActionTest test cases to yield syntax

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

โ€ฆ when an undo is requested, first check whether the current revision has the same redirect status as the revision to be undone โ€ฆ

There is also another situation that currently throws the same PatcherException. When undoing an edit that changed a redirect back into an ordinary entity, but that edit is also not the latest edit (i.e. there were more ordinary edits later), then the current revision has the same redirect status as the revision to be undone, but the undo still crashes. In other words, when the history looks like this:

  • โ€ฆ older revisions
  • revision A: entity is redirect
  • revision B: entity is no longer redirect
  • revision C: regular edit (e.g. add label)

Undoing revisions B and C together works, but undoing revision B alone crashes.

@Lydia_Pintscher what should happen in that case? Should we turn the entity back into a redirect, ignoring the other edits that were made since then (revision C)? Or should we report an edit conflict?

I'd lean towards turning it into a redirect again. Or does anything speak against that?

Change 656136 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Permit restoring redirects to discard entity data

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

Change 655726 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Handle error when mixing redirects in undo

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

Change 656136 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Permit restoring redirects to discard entity data

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

Change 673278 had a related patch set uploaded (by Tobias Andersson; owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@REL1_35] Handle error when mixing redirects in undo

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

Change 673278 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_35] Handle error when mixing redirects in undo

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

Change 677343 had a related patch set uploaded (by WMDE-leszek; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_35] Permit restoring redirects to discard entity data

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

Change 677343 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_35] Permit restoring redirects to discard entity data

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