Page MenuHomePhabricator

Edit conflict detection by timestamp should be deprecated
Open, NormalPublic

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

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
awight created this task.Nov 9 2013, 9:54 PM

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

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

The solution should go beyond the above patch. Ideally, we remove the cheesy timestamp check and rely entirely on an atomic update attempt. If that fails due to a conflict, perhaps WikiPage::doEditContent falls back to 3-way merge (moving that code down from EditPage), and then we attempt a second, atomic update.

aaron added a comment.Nov 13 2013, 6:41 PM

Ideally we'd also avoid the SELECT FOR UPDATE (followed by the parsing) and just do CAS style logic at the last second. The advantage would be that if a page takes 10 seconds to render and two people save at the same time, it would take about 10 seconds for each rather than 20 right now (people have complained about it taking forever to even get edit conflict pages).

Since this has been linked to bug 57022, I tried applying the patch to my 1.21.2 wiki. Did not fix the edit conflict problem on this wiki.

A minor detail to add: we should always include the oldid in GET query parameters to action=edit.

Patch didn't pass jenkins-bot.
Anybody planning to pick up the work on this again?

Patch didn't pass jenkins-bot.
Anybody planning to pick up the work on this again?

Plus wondering if fixing this would influence bug 53646, bug 53446, bug 42163.

@Aklapper: Thanks for linking the related bugs! I'm expanding my earlier patch to clean up oldid handling and implement compare-and-swap as @aschulz suggests. I expect that will resolve bug 42163, but for the other two I can't say. It would be very useful to have unit tests to detect improvement or regression of all four bugs.

I came across something disturbing while exploring the oldid fix: apparently, there is a 'redirect' parameter to the edit action that allows a page save to change the contents of the redirected page, rather than the contents of the redirect article passed as the 'title' parameter to edit.

Maybe I'm being willfully blind, but I cannot imagine a case where this is a good idea.

This seems like a nasty hurdle to reimplementing oldid, because a process making a 'redirect' edit will not necessarily have any information about the revision level of the redirect target. These cases would have to fall back to the badly flawed timestamp check.

Aha: so the 'redirect' thing was introduced to satisfy bug 24330, but along the way the "appendtext" or "prependtext" requirement was lost in implementation, commit 9f025aeb.

IMO this is a misfeature, I'll write a separate patch to... break API backwards-compatibility... as part of this work.

@ASchulz: It looks like the CAS logic you suggested has already been implemented, in WikiPage::updateRevisionOn(). I'm trying to provide the $lastRevision param whenever possible, which should improve many cases.

There is another function updateIfNewerOn which does require two queries, but it's only used in SpecialUndelete and Import, and IMO should be retired.

Was there another query you know of which would explain the long load during a conflict?

Change 134049 had a related patch set uploaded by Adamw:
(bug 56849) Deprecate dangerous edittime-based content update functions

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

Change 134049 merged by jenkins-bot:
(bug 56849) Deprecate dangerous edittime-based content update functions

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

(In reply to Adam Wight from comment #13)

I'm still working on the main patch, but these predecessors are ready for
review:
https://gerrit.wikimedia.org/r/#/c/133955 (also tracking under bug 24330)
https://gerrit.wikimedia.org/r/#/c/134045
https://gerrit.wikimedia.org/r/#/c/134046
https://gerrit.wikimedia.org/r/#/c/134047
https://gerrit.wikimedia.org/r/#/c/134049

For the records, all patches merged or abandoned.
Not sure about the patch in comment 1 which didn't pass Jenkins.

(In reply to Andre Klapper from comment #16)

For the records, all patches merged or abandoned.
Not sure about the patch in comment 1 which didn't pass Jenkins.

Adam: What's the status here?
Is more work needed, or can this be considered fixed?

This patch is mean to fix the issue:
https://gerrit.wikimedia.org/r/#/c/94584/

Everything else has been a preliminary... Sorry!

@awight: This issue has been assigned to you a while ago.
Could you please provide a status update and inform us whether you are still working (or still plan to work) on this issue?
Only in case you do not plan to work on this issue anymore, should the assignee be removed? Thanks.

@Aklapper, thanks for the ping. I received some helpful CR comments on commit e285dd6 and need to make time to work on the final patch for this issue. There is a complementary task that might help me finish the work here: if anyone wants to write documentation for the edit logic, that would make it more clear whether I'm introducing new or broken special cases, or preserving existing crazy.

awight updated the task description. (Show Details)Mar 11 2015, 2:04 AM
awight set Security to None.

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

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

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

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

awight updated the task description. (Show Details)Apr 9 2015, 7:59 AM
He7d3r updated the task description. (Show Details)Apr 9 2015, 2:10 PM

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

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

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

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

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

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

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

@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.

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?

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.

@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.

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

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

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/

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.

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.

@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 Normal.
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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Change 445237 abandoned by Awight:
Stop relying on deprecated edittime

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

Change 443963 abandoned by Awight:
Drop "edittime" parameter

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

Elitre removed a subscriber: Elitre.Aug 29 2019, 9:09 AM