Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add is_dm filtering to Sliding Sync /sync #17244

Closed

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented May 29, 2024

Add is_dm filtering to Sliding Sync /sync

Based on MSC3575: Sliding Sync

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

Based on:

 - MSC3575: Sliding Sync (aka Sync v3): matrix-org/matrix-spec-proposals#3575
 - MSC3885: Sliding Sync Extension: To-Device messages: matrix-org/matrix-spec-proposals#3885
 - MSC3884: Sliding Sync Extension: E2EE: matrix-org/matrix-spec-proposals#3884
We were using the enum just to distinguish /sync v2
vs Sliding Sync /sync/e2ee so we should just make an
enum for that instead of trying to glom onto the
existing `sync_type` (overloading it).
Conflicts:
	synapse/handlers/sync.py
	tests/handlers/test_sync.py
Conflicts:
	synapse/storage/databases/main/roommember.py
…emods/msc3575-sliding-sync-filtering

Conflicts:
	tests/handlers/test_sliding_sync.py
	tests/rest/client/test_sync.py
@MadLittleMods MadLittleMods force-pushed the madlittlemods/msc3575-sliding-sync-filtering branch from f3a6985 to 555ba4b Compare June 4, 2024 18:27
MadLittleMods and others added 20 commits June 4, 2024 13:27
Previously, we added back newly_left rooms and our second
fixup was removing them. We can just switch the order
of the fixups to solve this.
…sync_room_ids_for_user` implementation

The new implementation catches the problem with an assert
but I think it's possible to make it work as well.

```
SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.GetSyncRoomIdsForUserEventShardTestCase
```
…ltiple event persisters

Before, the problem scenario would get caught in one of the assertions because
we expect the to_token <= membership_snapshot_token or vice-versa but it's
possible the tokens are intertwined and neither is ahead of each other.
Especially since the `instance_map` in `membership_snapshot_token` is made up
from the `stream_ordering` of membership events at various stream positions
and processed on different instances (not current stream positions).

We get into trouble when stream positions are lagging between workers and our
now/`to_token` doesn't cleanly compare to `membership_snapshot_token`.

What we really want to assert is that the `to_token` <= the stream positions
at the time we asked for the room membership snapshot. Since
`get_rooms_for_local_user_where_membership_is()` doesn't return that
information, the closest we can get is to get the stream positions before we
ask for the room membership snapshot and consider that good enough to compare
against.
Co-authored-by: Erik Johnston <[email protected]>
See #17187 (comment)

`get_membership_changes_for_user(from_key=xxx, to_key=xxx)` will handle
getting out what we need and filter the results based on the tokens
(even in cases where the from_key is ahead of the to_key).
…emods/msc3575-sliding-sync-filtering

Conflicts:
	tests/handlers/test_sliding_sync.py
Base automatically changed from madlittlemods/msc3575-sliding-sync-0.0.1 to develop June 6, 2024 19:44
Conflicts:
	synapse/handlers/sliding_sync.py
	tests/handlers/test_sliding_sync.py
	tests/rest/client/test_sync.py
@MadLittleMods
Copy link
Contributor Author

Closing in favor of #17277 to get rid of the commits that keep following us from being based on the initial branch

@MadLittleMods MadLittleMods removed the request for review from a team June 6, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant