Page MenuHomePhabricator

[keyserver] Use message ID as tiebreaker in message-fetchers code
ClosedPublic

Authored by ashoat on Aug 2 2022, 12:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 3:32 AM
Unknown Object (File)
Thu, Dec 19, 1:40 PM
Unknown Object (File)
Nov 9 2024, 11:38 AM
Unknown Object (File)
Nov 8 2024, 8:36 PM
Unknown Object (File)
Nov 8 2024, 8:36 PM
Unknown Object (File)
Nov 8 2024, 8:36 PM
Unknown Object (File)
Nov 5 2024, 2:06 AM
Unknown Object (File)
Oct 4 2024, 7:39 PM

Details

Summary

In D4675, @tomek pointed out that our results may be inconsistent in the case where two messages have the same timestamp. While this scenario is unlikely, it is technically possible.

To make the results consistent we can add a "tiebreaker" as a second ORDER BY parameter. This diff adds id as the tiebreaker for all message-fetchers.js functions that return ordered lists.

Tracked in ENG-1488.

Test Plan
  1. I compared the web inbox in my local environment between master and with this diff to make sure the same chats appeared in the same order with the same set of messages inside them.
  2. First, I ran the query with the PREDICATE manually in MariaDB to make sure it worked, since that was a special case.
  3. Then I patched this diff onto prod and deployed it to make sure that the queries still worked without issue (including without performance issues).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/fetchers/message-fetchers.js
331 ↗(On Diff #15236)

Note that I added m. in front of time here for consistency. I tested and confirmed this query still works correctly

Hmm something is wrong here, @abosh noticed some issues on prod

Maintain DESC order on m.time, duh... was really dumb to test this on prod

Test plan still needs to be updated

ashoat edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Aug 8 2022, 6:59 AM