Page MenuHomePhabricator
Create Task
Maniphest T58849

Edit conflict detection by timestamp should be deprecated
Open, MediumPublic
Actions

Description

In theory, we perform an inexpensive 1-second granularity timestamp check to detect conflicts, then a second test is performed atomically during updateRevisionOn, which uses the actual revision ID of the article content that was used as the edit base. However, this atomic test is broken because getLatest is returning the *new* latest id rather than the oldid stored at the time the user opened the article edit page.

This can be easily confirmed by disabling the timestamp test at EditPage.php line 1613, opening two tabs, and editing then submitting in both. This allows you to simulate two edits occurring within one second of each other. The outcome will be a silently ignored edit conflict, and the second edit will overwrite the first.

One issue that jumps out immediately is that we were relying on the "oldid" hidden form parameter, but this always has a value of "0".

The edittime method has been deprecated by editRevId, so we can remove the parameter.

See Also:

Details

Reference
bz56849
Show related patches Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 OpenNoneT72163 Edit conflicts (tracking)
 OpenNoneT58849 Edit conflict detection by timestamp should be deprecated

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
awight set Security to None.Mar 11 2015, 2:04 AM
gerritbot subscribed.Mar 11 2015, 7:28 AM

Change 94584 had a related patch set uploaded (by Awight):
WIP (bug 56849) Pass true oldid for atomic page save

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

gerritbot added a project: Patch-For-Review.Mar 11 2015, 7:28 AM
gerritbot added a comment.Apr 5 2015, 7:00 AM

Change 201928 had a related patch set uploaded (by Awight):
Accessor to get EditPage parent revision ID

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

gerritbot added a comment.Apr 5 2015, 7:00 AM

Change 201929 had a related patch set uploaded (by Awight):
Helper to get a page's revision ID at a given time

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

Nemo_bis added a project: Epic.Apr 8 2015, 8:29 AM
awight updated the task description. (Show Details)Apr 9 2015, 7:59 AM
awight mentioned this in T34037: API: Allow edit conflict detection based on revid.Apr 9 2015, 8:08 AM
awight mentioned this in T88734: Use parentRevId field for section change merging instead of timestamps.
awight added a subscriber: Schnark.
He7d3r updated the task description. (Show Details)Apr 9 2015, 2:10 PM
gerritbot added a comment.Apr 12 2015, 1:30 AM

Change 203632 had a related patch set uploaded (by Awight):
Use parentRevId for three-way merge

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

gerritbot added a comment.Apr 17 2015, 7:46 AM

Change 204713 had a related patch set uploaded (by Awight):
Introduce parentrevid parameter in the API

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

gerritbot added a comment.Apr 17 2015, 7:46 AM

Change 204714 had a related patch set uploaded (by Awight):
WIP Compatibility to guess the parentRevId from legacy timestamp

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

Ricordisamoa subscribed.Jul 26 2015, 7:48 AM
gerritbot added a comment.Nov 24 2015, 10:23 PM

Change 201928 merged by jenkins-bot:
Accessor to get EditPage parent revision ID

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

aaron updated the task description. (Show Details)Nov 24 2015, 10:43 PM
ReleaseTaggerBot added projects: MW-1.27-release-notes, MW-1.27-release (WMF-deploy-2015-12-08_(1.27.0-wmf.8)).Nov 24 2015, 11:00 PM
gerritbot added a comment.Nov 24 2015, 11:22 PM

Change 255278 had a related patch set uploaded (by Aaron Schulz):
[WIP] Switch EditForm to using editRevId in place of edittimestamp

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

JeroenDeDauw unsubscribed.Dec 8 2015, 11:10 AM
Quiddity mentioned this in T120462: Reduce edit conflicts by treating different parts of the page as separate.Apr 26 2016, 12:19 AM
Lea_WMDE subscribed.Jun 8 2016, 3:44 PM

@awight @aaron are you still working on this? Since edit conflicts are a big topic on both the interational wishlist and the technical wishes of the German-speaking community, we are thinking to further work on your patches at the Wikimania hackathon. Would that be ok for you? And if yes, what would be the next steps to do?

aaron added a comment.Jun 8 2016, 7:21 PM

I can probably rebase that one patch and remove the WIP tag. Aside from that, I probably won't make more of these patches soon.

matej_suchanek updated the task description. (Show Details)Jun 8 2016, 7:25 PM
WMDE-leszek subscribed.Jun 13 2016, 1:03 PM
WMDE-Fisch subscribed.Jun 14 2016, 8:00 AM
Tobi_WMDE_SW subscribed.Jun 14 2016, 1:20 PM
WMDE-Fisch added a comment.Jun 14 2016, 2:28 PM

I reviewed the latest patch addressing the edit conflict race condition with timestamps by @aaron ( https://gerrit.wikimedia.org/r/#/c/255278/ )

Apart from minor general code improvements, for me some questions remain and I added comments in the patch.

As I understand it, this patch adds checks to compare the revision ids of the the expected base and the actual ( potentially updated ) base. Prior to the patch theses checks relied on the timestamp instead. The patch keeps the timestamp checks for cases, where the expected base id is not set.

Q:

  • In which cases can the base rev id be null?
  • Do we still need the additional timestamp checks?
  • If we still need the timestamp check, is there still the possibility that we have problems there?
  • Do we still need a timestamp fallback when loading the content for the resolve conflict page?
WMDE-Fisch added a comment.Jun 20 2016, 9:49 AM

Answers to my questions were given in the patch. I will sum them up in this comment:

Timestamp checks are kept to be backwards compatible, bots or other scripts might not use the new fields and therefore it can be null in some cases. The original problem should be solved with the patch.

Lea_WMDE mentioned this in T135422: Wishlist tasks suggested for the Wikimania Hackathon 2016.Jun 20 2016, 10:32 AM
awight added a comment.Jun 20 2016, 3:50 PM

@WMDE-Fisch Thank you for throwing new energy at this problem! I'll experiment with Aaron's patch, and I can give lots of time during the Hackathon. This is still high on my list of public and personal vendettas in MediaWiki.

gerritbot added a comment.Jun 23 2016, 11:19 AM

Change 255278 merged by jenkins-bot:
Switch EditForm to using editRevId in place of edittimestamp

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

ReleaseTaggerBot added projects: MW-1.28-release (WMF-deploy-2016-06-28_(1.28.0-wmf.8)), MW-1.28-release-notes.Jun 23 2016, 12:00 PM
awight added a comment.Jun 24 2016, 4:22 PM

Woohoo! That patch should do it...

The next step is to write some tests to confirm the fix and prevent regressions. FWIW, this draft patch might have some useful bits that can be reused when writing the tests: https://gerrit.wikimedia.org/r/#/c/94584/

Tgr mentioned this in T36423: Edit conflicts with yourself are not detected when doing no section edit - inconsistent behavior.Nov 20 2016, 6:15 AM
daniel subscribed.Jun 19 2018, 2:37 PM

I'm proposing to remove parentRevId because its semantics is unclear, see T197685: Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/441047

daniel mentioned this in T197685: Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater.Jun 19 2018, 2:38 PM
awight added a comment.Jul 2 2018, 1:11 PM

It doesn't look like we ever solved the race condition for ApiEditPage, which doesn't use editRevId. I'd like to require this param everywhere, it's quite risky to edit a page without knowing what revision we're basing changes on.

daniel added a project: User-Daniel.Jul 2 2018, 1:30 PM
In T58849#4382015, @awight wrote:

It doesn't look like we ever solved the race condition for ApiEditPage, which doesn't use editRevId. I'd like to require this param everywhere, it's quite risky to edit a page without knowing what revision we're basing changes on.

Yes, please !

awight added a comment.Jul 3 2018, 9:26 AM

Now I'm thinking that we shouldn't make the editRevId parameter mandatory, and can point to the other ApiEditPage params being optional as precedent. The default behavior of guessing editRevid = page.latest is sane and is usually correct as well, so requiring clients to track page.latest before making an edit adds complexity with no gain.

But I'll go ahead and pass the parameter through ApiEditPage, and recommend adding it for any integration experiencing edit conflicts.

daniel added a comment.Jul 3 2018, 10:56 AM

@awight Passing editRevId is important if and only if the new content is based on some old content. And if that is the case, the old revision is known to the client.

So, editRevId needs to be included if the edit was, e.g., a search and replace operation, or inserting categories into an existing page text.

Since blindly replacing the entire page without looking at its current content should be rare, supplying editRevId should be the norm. It probably doesn't need to be a hard requirement, but I'd support issuing a warning of page content is overwritten without editRevId being present. We could support "editRevId=blind" to suppress the warning.

The append/prepend modes are a bit of a special case. Passing editRevId doesn't make sense for them, they are always "blind". The same is true for section=new.

When creatign a new page, clients should send editRevId=0 to indicate that they intended to create a page. That would be equivalent to the EDIT_NEW flag used internally.

Other section edits however should be assumed to be based on that section's previous content.

awight renamed this task from Edit conflict detection suffers a race condition to Edit conflict by epoch seconds has a race condition.Jul 5 2018, 12:23 PM
awight lowered the priority of this task from High to Medium.
awight updated the task description. (Show Details)

Lowered the priority because the epoch seconds check is a fallback to more accurate editRevId, the latest revision ID. The remaining work is to remove the fallback.

gerritbot added a comment.Jul 5 2018, 12:46 PM

Change 443963 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/core@master] Drop "edittime" parameter

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

awight renamed this task from Edit conflict by epoch seconds has a race condition to Edit conflict detection by timestamp should be deprecated.Jul 5 2018, 4:28 PM
gerritbot added a comment.Jul 11 2018, 6:39 PM

Change 445230 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/FlaggedRevs@master] Use editRevId rather than edittime

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

gerritbot added a comment.Jul 11 2018, 6:50 PM

Change 445236 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/core@master] Make $editRevId public for naughty extensions to access

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

gerritbot added a comment.Jul 11 2018, 6:55 PM

Change 445237 had a related patch set uploaded (by Legoktm; owner: Awight):
[mediawiki/extensions/TemplateSandbox@master] Stop relying on deprecated edittime

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

gerritbot added a comment.Jul 11 2018, 7:11 PM

Change 445240 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Commentbox@master] Update to use editRevId

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

gerritbot added a comment.Jul 11 2018, 7:15 PM

Change 445245 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/BlogPage@master] Remove deprecated wpEdittime reference

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

gerritbot added a comment.Jul 11 2018, 7:17 PM

Change 445246 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/DynamicPageList@master] Use editRevId for compatibility with newer MediaWikis

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

gerritbot added a comment.Jul 11 2018, 7:19 PM

Change 445249 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/CreateRedirect@master] Use newer "editRevId" conflict detection

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

gerritbot added a comment.Jul 11 2018, 7:23 PM

Change 445250 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/PageForms@master] Use editRevId for better edit conflict detection

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

awight mentioned this in rEPFM7c8e64dcc40b: Use editRevId for better edit conflict detection.Jul 11 2018, 7:28 PM
gerritbot added a comment.Jul 11 2018, 7:34 PM

Change 445253 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Drafts@master] Update to use editRevId conflict detection

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

gerritbot added a comment.Jul 11 2018, 11:09 PM

Change 204714 abandoned by Awight:
WIP Compatibility to guess the parentRevId from legacy timestamp

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

gerritbot added a comment.Jul 11 2018, 11:10 PM

Change 204713 abandoned by Awight:
[WIP] Introduce parentrevid parameter in the API

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

gerritbot added a comment.Jul 11 2018, 11:11 PM

Change 201929 abandoned by Awight:
[WIP] Helper to get a page's revision ID at a given time

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

gerritbot added a comment.Jul 11 2018, 11:12 PM

Change 94584 abandoned by Awight:
WIP: Use the parentRevId for more atomic editing

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

awight mentioned this in rEPFM24a472d7bbbd: Use editRevId for better edit conflict detection.Jul 11 2018, 11:40 PM
awight mentioned this in T199504: Editing of content model other than wikitext fails.Aug 27 2018, 5:38 PM
gerritbot added a comment.Mar 21 2019, 10:25 AM

Change 445245 merged by jenkins-bot:
[mediawiki/extensions/BlogPage@master] Remove deprecated wpEdittime reference

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

WMDE-Fisch mentioned this in T219727: UnexpectedValueException: The title "Discussioni utente:Barstein" does not refer to an existing page.Apr 29 2019, 10:43 AM
WMDE-Fisch mentioned this in T222805: Edit conflicts for no reason with older revisions or yourself.May 8 2019, 4:01 PM
gerritbot added a comment.Jul 19 2019, 1:25 PM

Change 445237 abandoned by Awight:
Stop relying on deprecated edittime

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

gerritbot added a comment.Jul 19 2019, 1:28 PM

Change 443963 abandoned by Awight:
Drop "edittime" parameter

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

• Elitre unsubscribed.Aug 29 2019, 9:09 AM
Kizule moved this task from Backlog to Edit conflict on the MediaWiki-Page-editing board.Aug 31 2019, 5:46 PM
gerritbot added a comment.Feb 4 2020, 3:20 AM

Change 445249 abandoned by Edward Chernenko:
Use newer "editRevId" conflict detection

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

gerritbot added a comment.Feb 26 2020, 10:58 PM

Change 443963 restored by Awight:
Drop "edittime" parameter

Reason:
Working on this again.

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

gerritbot added a comment.Feb 26 2020, 11:00 PM

Change 445237 restored by Awight:
Stop relying on deprecated edittime

Reason:
I'm sure this can be accomplished.

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

awight added a subscriber: thiemowmde.Feb 26 2020, 11:01 PM
daniel added a comment.Feb 27 2020, 10:30 AM

Platform Engineering is interested in this in the context of the REST/CRUD interface, see T230843.

Amorymeltzer subscribed.Mar 8 2020, 7:24 PM
awight added a comment.Mar 8 2020, 8:59 PM

I found something surprising while playing with a proof-of-concept for T245523: Investigation: Permanent or long-term storage of edit conflict text. It seems that wpEdittime is required and without it, EditPage will ingore edit conflicts based on editRevId. So far, I have no reason to think this causes MediaWiki to ignore real conflicts in the wild, since the server should in theory calculate this timestamp correctly, but it's certainly fragile and will have to be fixed before we can fully deprecate.

daniel updated the task description. (Show Details)Mar 9 2020, 1:45 PM
daniel added a comment.Mar 9 2020, 1:50 PM
In T58849#5952476, @awight wrote:

I found something surprising while playing with a proof-of-concept for T245523: Investigation: Permanent or long-term storage of edit conflict text. It seems that wpEdittime is required and without it, EditPage will ingore edit conflicts based on editRevId.

Please see if this patch fixes that issue for you:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/577345

So far, I have no reason to think this causes MediaWiki to ignore real conflicts in the wild, since the server should in theory calculate this timestamp correctly, but it's certainly fragile and will have to be fixed before we can fully deprecate.

Well, edits in the wild come either from the UI, which provides wpEdittime in addition to the revision ID, or from the API, which so far supports conflict detection only based on the basetimestamp parameter. So the only way to get into this situation in the wild would be to fake a POST request from the edit form, providing a revision ID but no timestamp. I think doing so would currently indeed cause the conflict to be ignored.

daniel added a comment.Mar 9 2020, 1:52 PM

@aweight please also note my comment on T34037#5948430 about the fact that conflict detection based on timestamp will ignore self-conflicts, while detection based on revision id will not.

awight removed awight as the assignee of this task.EditedMay 26 2021, 2:30 PM
awight added a subscriber: Daniel.Cardenas.

Unassigning myself. I think the next step would be to deprecate edittime, but this is a big undertaking. Lots of external code relies on the parameter, and there's the self-conflict question brought up by @daniel.

awight removed a subscriber: Daniel.Cardenas.May 26 2021, 2:31 PM
gerritbot added a comment.Aug 18 2021, 8:51 PM

Change 445250 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Use editRevId for better edit conflict detection

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

matmarex mentioned this in T299724: Use of starttimestamp or basetimestamp in API calls doesn't seem to prevent edit conflicts.Jan 21 2022, 12:52 AM
gerritbot added a comment.Nov 30 2022, 9:15 AM

Change 445246 merged by jenkins-bot:

[mediawiki/extensions/DynamicPageList@master] Use editRevId for compatibility with newer MediaWikis

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

gerritbot added a comment.Nov 30 2022, 9:15 AM

Change 445240 merged by jenkins-bot:

[mediawiki/extensions/Commentbox@master] Update to use editRevId

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

Pppery edited projects, added Patch-Needs-Improvement; removed Patch-For-Review.Apr 8 2023, 5:32 PM
gerritbot added a comment.Sep 26 2023, 8:01 AM

Change 445236 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/core@master] Make $editRevId public for naughty extensions to access

Reason:

Unclear which exact code needs this. Can easily be restored/redone if needed.

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

matmarex mentioned this in T61113: Remove methods in MW core deprecated since MW 1.25 or earlier.Dec 10 2023, 1:53 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL