Sliding Sync: Fix outlier re-persisting causing problems with sliding sync tables #17635
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
MadLittleMods
commented
Aug 29, 2024
| (room_id, event_stream_ordering, bump_stamp, {", ".join(sliding_sync_updates_keys)}) | ||
| VALUES ( | ||
| ?, | ||
| (SELECT stream_ordering FROM events WHERE room_id = ? ORDER BY stream_ordering DESC LIMIT 1), |
There was a problem hiding this comment.
This could be simplified to remove ORDER BY stream_ordering DESC. This just gives a more correct answer but we will end up with the correct answer anyway when _update_sliding_sync_tables_with_new_persisted_events_txn() runs right after this.
Suggested change
| (SELECT stream_ordering FROM events WHERE room_id = ? ORDER BY stream_ordering DESC LIMIT 1), | |
| (SELECT stream_ordering FROM events WHERE room_id = ? LIMIT 1), |
Up to you
MadLittleMods
commented
Aug 29, 2024
| remaining_events = await self.store.get_events( | ||
| current_state_ids_map.values() | ||
| ) | ||
| remaining_events = await self.store.get_events(missing_event_ids) |
There was a problem hiding this comment.
Found this logic bug because of the new tests. This is a separate problem though that we're fixing.
Now we're actually fetching the missing_event_ids if we have any.
…ugs-with-sliding-sync-tables
erikjohnston
approved these changes
Aug 30, 2024
erikjohnston
deleted the
madlittlemods/fix-outlier-re-persisting-bugs-with-sliding-sync-tables
branch
|
Thanks for the review and merge @erikjohnston 🐁 |
MadLittleMods
commented
Aug 30, 2024
| VALUES ( | ||
| ?, ?, ?, ?, ?, | ||
| (SELECT stream_ordering FROM events WHERE event_id = ?), | ||
| (SELECT instance_name FROM events WHERE event_id = ?) |
There was a problem hiding this comment.
We should have fallen back to "master" when this isn't filled in
Suggested change
| (SELECT instance_name FROM events WHERE event_id = ?) | |
| (SELECT COALESCE(instance_name, 'master') FROM events WHERE event_id = ?) |
Fixed up in #17636 ⏩
erikjohnston
added a commit
that referenced
this pull request
Sep 1, 2024
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.
Fix outlier re-persisting causing problems with sliding sync tables
Follow-up to #17512
When running on
matrix.org, we discovered that a remote invite is first persisted as anoutlierand then re-persisted again where it is de-outliered. The first the time, theoutlieris persisted with onestream_orderingbut when persisted again and de-outliered, it is assigned a differentstream_orderingthat won't end up being used. Since we call_calculate_sliding_sync_table_changes()before_update_outliers_txn()which fixes this discrepancy (always use thestream_orderingfrom the first time it was persisted), we're working with an unreliablestream_orderingvalue that will possibly be unused and not make it into theeventstable.TODO
Developer notes
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)