Page MenuHomePhabricator

[Regression wmf.18] Messages badge sometimes shows negative number
Closed, ResolvedPublic

Description

Reported by @Whatamidoing-WMF

I haven't been able to reproduce this locally, but my theory is that this is a race condition caused by rECHOcd57ab5e5428: Clicking a marked-as-unread notification should mark it as read (https://gerrit.wikimedia.org/r/#/c/277703/). That change made all notifications be marked as read on click, even if there is also an echo_target_page row for them. I think these notifications are being marked as read twice, and subtracted from the notification count twice:

  1. User clicks unread notification
  2. JS sends API request to mark notification as read; this decrements the unread messages counter in memcached (from 1 to 0) and deletes the echo_target_page row in the database
  3. Browser navigates to page
  4. onPersonalUrls sees an already-decremented message counter (i.e. 0) in memcached, but also still sees the echo_target_page row in the DB (due to slave lag)
  5. A deferred update is scheduled for mark this notification as read (again) and to account for this, 1 is subtracted from the message count that's sent to the client
  6. 0 - 1 = -1, so the client receives a message count of -1

Details

Related Gerrit Patches:

Event Timeline

Catrope created this task.Mar 24 2016, 6:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 24 2016, 6:27 PM
Jay8g added a subscriber: Jay8g.Mar 30 2016, 4:41 AM

I just ran into this, we can probably fix it in step 5.

Change 282656 had a related patch set uploaded (by Legoktm):
Never show a negative number in the notifications badge

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

Change 282656 merged by jenkins-bot:
Never show a negative number in the notifications badge

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

Checked in betalabs - could not see any discrepancies in updating(or writing to) echo_target_page and other echo tables.

Ran general regression testing for counter to be updated for various scenarious - all seems to be fine.

jmatazzoni closed this task as Resolved.Apr 15 2016, 11:10 PM
jmatazzoni claimed this task.