Fix case of lost messages: Do not delete events from the last forward chunk #5878
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We get end up with missing messages by the combination of - deleting the last forward chunk when receiving a new one - not adding events to a chunk that are already found in another chunk Accordingly, when using chunk tokens to load more messages, those messages that were not added to a chunk due to a /sync chunk will get lost. More thorough steps to reproduce: - Receive e.g. 30 new messages while offline - Use /sync in the room overview, this will fetch the latest 10 events - Open a chat in the past before the latest unread messages - Scroll down a little, in order to fill the message gap and load all unread messages - Close the chat - Receive another e.g. 60 messages while offline - Re-open the chat at some time in the past, before the latest 70 messages => messages from the old /sync chunk will be missing Change-Id: Ia3f2d2715a3edfd0b3fe5c3d48a02ade4ea49c4d
If we link chunks in pagination direction, and discard all events after that, we assume that we reached a point in the chunk that is already covered by a different chunk. If we however haven't seen any new events in that chunk yet, chances are this is the wrong direction we are linking. So in this case, better just skip related events and continue processing later events - making sure we don't lose new events and don't link in the wrong direction. Note we could also enforce links into the opposite direction in this case. Since in the cases I observed so far, such link already existed, so I think this is probably not necessary. Change-Id: Ia4d2fd87188b9757ed68416e883c3fb489cdfa6e
SpiritCroc
changed the title
15 tasks
|
I'm wondering if we should instead always add TimelineEvent to chunk and let the timeline remove duplicates if any |
Sounds to me like the logic for that approach would be easier to understand and less prone to bugs at least. 🤔 |
If we already have some events in a previously linked chunk, that doesn't mean we have all of them. So we still want to continue processing later events in that case.
|
See #6318 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Motivation and context
We get end up with missing messages by the combination of
Accordingly, when using chunk tokens to load more messages, those
messages that were not added to a chunk due to a /sync chunk will get
lost.
Content
Before deleting the latest chunk, move the events to the previous chunk, if applicable.
Also make sure we do not link in the wrong direction because we found these events, to prevent timeline loops.
Tests
unread messages
messages
=> messages from the old /sync chunk will be missing
Tested devices
Checklist
Signed-off-by: Tobias Büttner [email protected]