Page MenuHomePhabricator

[server] Don't set EXHAUSTIVE when there is a time filter
ClosedPublic

Authored by ashoat on Mar 11 2022, 11:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 9:16 AM
Unknown Object (File)
Sat, Nov 2, 12:22 PM
Unknown Object (File)
Sat, Nov 2, 9:53 AM
Unknown Object (File)
Sat, Nov 2, 9:53 AM
Unknown Object (File)
Sat, Nov 2, 8:31 AM
Unknown Object (File)
Oct 7 2024, 6:39 AM
Unknown Object (File)
Oct 7 2024, 6:39 AM
Unknown Object (File)
Oct 7 2024, 6:38 AM

Details

Summary

This solves ENG-854: History Lost in Bird App Channel. The issue was that fetchMessageInfos was set up to assume that if we queried for 20 messages, but got fewer than that back from MySQL (0-19), then we could conclude that we've reached the start of the thread (messageTruncationStatuses.EXHAUSTIVE).

This is no longer true if we have an active time filter. The time filter can cause us to return fewer than requested messages simply because there weren't enough newer than the time filter.

We should stick with UNCHANGED in these cases, which in freshMessageStore will result in startReached: false, and in mergeNewMessages will not change startReached.

Test Plan
  1. I tested different values of defaultMaxMessageAge such that some threads would return fewer than defaultNumberPerThread, and some threads would return 0
  2. I tested threads that needed a fetchMostRecentMessages, and threads that needed multiple fetchMessagesBeforeCursor to get to the start
  3. I made sure all threads loaded all the way to the start of a thread if I kept scrolling

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
server/src/fetchers/message-fetchers.js
416–419 ↗(On Diff #10311)

It sounds risky to not enforce this relationship. What do you think about adding e.g. an invariant to messageSelectionCriteriaToSQLClause which checks whether time condition is set if and only if messageSelectionCriteriaHasTimeFilterForThread returns true?

This revision is now accepted and ready to land.Mar 14 2022, 6:12 AM

Will look to resolve this in a follow-up diff – right now I just want to get this bug solved.

server/src/fetchers/message-fetchers.js
416–419 ↗(On Diff #10311)

Yeah, I agree it's kind of risky.

Having main function determine time filter info

Another approach would be to rename messageSelectionCriteriaToSQLClause (to eg. parseMessageSelectionCriteria) and have it return { +sqlClause: SQL, +timeFilterData: TimeFilterData }. TimeFilterData would contain the information necessary for messageSelectionCriteriaHasTimeFilterForThread to determine an answer without looking at MessageSelectionCriteria.

The complexity here is that TimeFilterData would not be simple. It would need to be something like:

type TimeFilterData =
  | { +globalTimeFilter: true }
  | { +globalTimeFilter: false, +threadSpecificTimeFilter: $ReadOnlySet<string> };

The logic for determining TimeFilterData might be somewhat brittle, although it's probably less brittle than the current approach.

Your suggestion of adding an invariant

I'm a bit worried about adding an invariant for this sort of thing... it's technically correct (the invariant should never trigger unless the programmer made a mistake), but it seems somewhat likely that we would fail to trigger the invariant in testing and it would get triggered in production, which would be a negative user experience.

I'll look into my solution above first, and if it doesn't work I'll add an invariant.