Page MenuHomePhabricator

Fix TransactionSquasher crashes
Open, Needs TriagePublic

Description

A bit more details: the issue was caused by code we deployed this week which was supposed to examine the changes made before saving, and record some data for the Edit Check project. Unfortunately it apparently fails on some kinds of changes (not all edits, but a significant number). In the en.wp VPT discussion someone suggested in happens when editing templates, but I couldn't reproduce the problem yet. For now I just disabled this code, and we will investigate it next week. Sorry about this.

There are two kinds of errors logged:

  • "Unexpected prior attribute value"
at ve.dm.TransactionSquasher.changeElement
at ve.dm.TransactionSquasher.processRemove
at ve.dm.TransactionSquasher.squashIn
at ve.dm.TransactionSquasher.static.squash
at ve.dm.Change.squash
at mw.editcheck.doesAddedContentNeedReference
at ve.dm.TransactionSquasher.changeElement
at ve.dm.TransactionSquasher.processAttribute
at ve.dm.TransactionSquasher.squashIn
at ve.dm.TransactionSquasher.static.squash
at ve.dm.Change.squash
at mw.editcheck.doesAddedContentNeedReference
at ve.dm.TransactionSquasher.processAttribute
at ve.dm.TransactionSquasher.squashIn
at ve.dm.TransactionSquasher.static.squash
at ve.dm.Change.squash
at mw.editcheck.doesAddedContentNeedReference
  • "Remove does not match insert"
at ve.dm.TransactionSquasher.processRemove
at ve.dm.TransactionSquasher.squashIn
at ve.dm.TransactionSquasher.static.squash
at ve.dm.Change.squash
at mw.editcheck.doesAddedContentNeedReference

Event Timeline

Change 908230 had a related patch set uploaded (by Divec; author: Divec):

[VisualEditor/VisualEditor@master] Allow attribute values that are reference types to compare equal

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

The patch set above fixes the "Unexpected prior attribute value" errors.

Could someone with logstash access provide me with an example change URL for which the "Remove does not match insert" error happened?

After investigating, I think the "Remove does not match insert" errors are caused by actual invalid transactions: see T334677 .

Change 908230 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Allow attribute values that are reference types to compare equal

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

Change 910809 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (da7624d69)

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

Change 910810 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Log TransactionSquasher errors, in case any still occur

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

Change 910809 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (da7624d69)

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

Change 910810 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Log TransactionSquasher errors, in case any still occur

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

There have been a few new entries, 26 in total over the last week, with error messages:

  • "Cannot read properties of undefined (reading 'operations')" (6 times)
  • "Remove does not match insert" (20 times)

@dchan: can you tell if there is anything else to be done to decrease the occurrence of these errors?

There's more now -- 607 over the last 7 days. Mostly "Remove does not match insert", but occasional "Cannot read properties of undefined (reading 'operations')".

Change 949045 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Edit check: Guard against completeHistory being empty

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

The above patch should fix the Cannot read properties of undefined (reading 'operations') errors.

Change 949045 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Edit check: Guard against completeHistory being empty

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

Per today's meeting, we're going to close this task.

Reason: this fix has been out for ~1 year and the logging we implemented has not yet alerted us of anything being wrong.

Still happening after all, see T380234 and https://logstash.wikimedia.org/goto/588dc4f21cd758aa605f4f4de1dd952d

We're getting ~80-150 logged errors per day. Almost all Remove does not match insert, but occasionally Unexpected prior attribute value.

Change #1092388 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/VisualEditor@master] Revert "editcheck: Remove try/catch around transaction squashing"

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

Change #1092388 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Revert "editcheck: Remove try/catch around transaction squashing"

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

Change #1092850 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/VisualEditor@wmf/1.44.0-wmf.3] Revert "editcheck: Remove try/catch around transaction squashing"

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

Change #1092851 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/VisualEditor@wmf/1.44.0-wmf.4] Revert "editcheck: Remove try/catch around transaction squashing"

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

Change #1092850 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@wmf/1.44.0-wmf.3] Revert "editcheck: Remove try/catch around transaction squashing"

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

Change #1092851 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@wmf/1.44.0-wmf.4] Revert "editcheck: Remove try/catch around transaction squashing"

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

Mentioned in SAL (#wikimedia-operations) [2024-11-19T21:39:39Z] <urbanecm@deploy2002> Started scap sync-world: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]]

Mentioned in SAL (#wikimedia-operations) [2024-11-19T21:45:09Z] <urbanecm@deploy2002> cscott, kemayo, urbanecm: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]] synced to the testservers (https://wikitech.wikimedia.or

Mentioned in SAL (#wikimedia-operations) [2024-11-19T22:00:19Z] <urbanecm@deploy2002> Finished scap sync-world: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]] (duration: 20m 39s)

Looking at the logs, I'm fairly certain that this diff had a Remove does not match insert error. Nothing really jumps out at me in it as being something particularly likely to cause an issue.