Page MenuHomePhabricator

Race condition when marking all notifications as read
Closed, ResolvedPublic

Description

Looks like I'm going through https://.../notification/ then marking them all as read, if new notifications have arrived in between, they are also marked as read. Sounds easy to miss some notifications because of this (especially for people like me who disabled email notifications).

Event Timeline

swisspol raised the priority of this task from to Needs Triage.
swisspol updated the task description. (Show Details)
swisspol added a subscriber: swisspol.

Notifications have a sequential ordering, so limiting the update to just notifications which are older than some time is easy. However, making sure the "Mark All Read" link in the dropdown menu always has a timestamp which aligns with user expectations may not be as easy.

chad triaged this task as Normal priority.Aug 1 2014, 2:33 AM
chad added a project: Notifications.

Hmmm, I think we need to alter one of the existing key here:

feed_storynotification | CREATE TABLE `feed_storynotification` (
`userPHID` varchar(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL,
`primaryObjectPHID` varchar(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL,
`chronologicalKey` bigint(20) unsigned NOT NULL,
`hasViewed` tinyint(1) NOT NULL,
UNIQUE KEY `userPHID` (`userPHID`,`chronologicalKey`),
KEY `userPHID_2` (`userPHID`,`hasViewed`,`primaryObjectPHID`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

The userPHID key should change to (userPHID, chronologicalKey, hasViewed). We need this so we don't update every notification ever before a certain chronologicalKey; just the ones where hasViewed = 0.

Sound right? If so, can I DROP KEY then ADD KEY in my migration script or is that going to be bogus?

On the good news front, slapping some ?cronoKey=X into the three spots we render this seems easy at the moment. :)

I think it's fine to just add a where chronologicalKey < x clause to the existing query. It can still use the userPHID, hasViewed parts of they key, which are way more important. The result set should be tiny after those parts of the key are used in all non-pathological cases.

The UI case I'm worried about is the notification menu: X should probably not be "the time when the page was loaded" there. I'm not sure if we re-render the menu every time you open it, or what we do if a notification comes in while you have the menu open.

Provided that we (a) render when you open the menu and (b) regenerate the whole menu when you re-open it after a notification comes in (I think we do?), then just using whatever the top notification was when we rendered the menu as X is probably fine. Using the most recent notification for the /notification/ page is definitely fine, I think. But if we're doing anything weird with the dropdown menu it might need a lighter touch.