Page MenuHomePhabricator

Huggle edit-warring with itself when reverting edits on user talk pages
Closed, ResolvedPublic

Description

See [https://en.wikipedia.org/w/index.php?title=User_talk:2A02:2908:5100:1D9E:7DC9:BFEE:EC3F:A19C&action=history]. {{u|Dark-World25}} was reverting edits made by the IP to their own user talk page, but in the process, Huggle self-reverted itself twice, it appears, and reintroduced the unwanted edits. Looks like Huggle is somehow overlapping its own edits. [[User:Home Lander|Home Lander]] ([[User talk:Home Lander|talk]]) 22:17, 17 March 2020 (UTC)

Event Timeline

Petrb created this task.Mar 24 2020, 11:54 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMar 24 2020, 11:54 PM
Petrb triaged this task as High priority.Mar 24 2020, 11:54 PM
Petrb added a comment.EditedMar 29 2020, 11:22 AM

So I investigated this and found the reason, it's related to flow of current message system in connection with reverting:

  • When user hit revert and warn, 2 async jobs are created, one for revert and one for message with revert as related edit
  • Both actions are processed in parallel, reverting naturally takes couple of API calls, so does the messaging
  • Due to current implementation talk page is in-memory edited first and then it waits for revert to finish, then it's actually written
  • Later talk page is overwritten, restoring the revert

Current message flow is:

  • Send()
  • PreflightCheck()
  • Download the current talk page
  • Modify the talk page in memory
  • Wait for related edit to finish
  • Write the talk page

Potentially fixed flow is:

  • Send()
  • Wait for related edit to finish (maybe as part of preflight?)
  • PreflightCheck()
  • Download the current talk page
  • Modify the talk page in memory
  • Write the talk page

Before changing the flow I have to think if this has potential to break anything.

Note to self: current check for related edit is around line 80 here:

bool Message::IsFinished()
{
    if (this->Done())
    {
        return true;
    }
    if (this->Dependency != nullptr)
    {
        if (!this->Dependency->IsProcessed())
        {

That would need to be moved somewhere else

Another note to self: why the hell isn't the mechanism that checks if talk page wasn't edited meanwhile (since last text retrieval) preventing this from happening? BaseTimestamp vs StartTimestamp should prevent this.

Petrb closed this task as Resolved.Mar 29 2020, 12:27 PM
Petrb claimed this task.

https://github.com/huggle/huggle3-qt-lx/commit/d1670770aa24a986bd0c09a2936da6c5da89f434

Really curious what else did I break while fixing this bug :)