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 Sliding Sync /sync endpoint (initial implementation) #17187

Merged
merged 123 commits into from
Jun 6, 2024

Conversation

MadLittleMods
Copy link
Collaborator

@MadLittleMods MadLittleMods commented May 13, 2024

Add initial implementation of the Sliding Sync /sync endpoint (not fully functional).

Based on MSC3575: Sliding Sync

This iteration only focuses on returning the list of room IDs in the sliding window API (without sorting/filtering).

Rooms appear in the Sliding sync response based on:

  • invite, join, knock, ban membership events
  • Kicks (leave membership events where sender is different from the user_id/state_key)
  • newly_left (rooms that were left during the given token range, > from_token and <= to_token)
  • In order for bans/kicks to not show up, you need to /forget those rooms. This doesn't modify the event itself though and only adds the forgotten flag to room_memberships in Synapse. There isn't a way to tell when a room was forgotten at the moment so we can't factor it into the from/to range.

Example request

POST http://localhost:8008/_matrix/client/unstable/org.matrix.msc3575/sync

{
  "lists": {
    "foo-list": {
      "ranges": [ [0, 99] ],
      "sort": [ "by_notification_level", "by_recency", "by_name" ],
      "required_state": [
        ["m.room.join_rules", ""],
        ["m.room.history_visibility", ""],
        ["m.space.child", "*"]
      ],
      "timeline_limit": 100
    }
  }
}

Response:

{
  "next_pos": "s58_224_0_13_10_1_1_16_0_1",
  "lists": {
    "foo-list": {
      "count": 1,
      "ops": [
        {
          "op": "SYNC",
          "range": [0, 99],
          "room_ids": [
            "!MmgikIyFzsuvtnbvVG:my.synapse.linux.server"
          ]
        }
      ]
    }
  },
  "rooms": {},
  "extensions": {}
}

Dev notes


Avoid shadowing arguments - disallow reassignment - masking existing variables:

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).
MadLittleMods added a commit that referenced this pull request Jun 4, 2024
…User` (#17265)

Use fully-qualified `PersistedEventPosition` (`instance_name` and `stream_ordering`) when returning `RoomsForUser` to facilitate proper comparisons and `RoomStreamToken` generation.

Spawning from #17187 where we want to utilize this change
Conflicts:
	synapse/storage/databases/main/roommember.py
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.
synapse/handlers/sliding_sync.py Outdated Show resolved Hide resolved
synapse/handlers/sliding_sync.py Outdated Show resolved Hide resolved
synapse/handlers/sliding_sync.py Outdated Show resolved Hide resolved
synapse/handlers/sliding_sync.py Outdated Show resolved Hide resolved
MadLittleMods and others added 6 commits June 5, 2024 10:52
Co-authored-by: Erik Johnston <erikj@element.io>
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).
Comment on lines +295 to +296
if not isinstance(membership, str):
raise SynapseError(400, "Invalid membership (must be a string)")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address failing CI:

poetry run trial tests.rest.client.test_rooms.RoomMemberStateTestCase.test_invalid_puts tests.rest.client.test_rooms
poetry run trial tests.rest.client.test_rooms.RoomMemberStateTestCase.test_invalid_puts
tests.rest.client.test_rooms
  RoomMemberStateTestCase
    test_invalid_puts ...                                                [FAIL]

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/eric/Documents/github/element/synapse/tests/rest/client/test_rooms.py", line 1069, in test_invalid_puts
    self.assertEqual(
  File "/home/eric/.cache/pypoetry/virtualenvs/matrix-synapse-xCtC9ulO-py3.12/lib/python3.12/site-packages/twisted/trial/_synctest.py", line 444, in assertEqual
    super().assertEqual(first, second, msg)
  File "/usr/lib/python3.12/unittest/case.py", line 885, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.12/unittest/case.py", line 878, in _baseAssertEqual
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: <HTTPStatus.BAD_REQUEST: 400> != 500 : b'{"errcode":"M_UNKNOWN","error":"Internal server error"}'

tests.rest.client.test_rooms.RoomMemberStateTestCase.test_invalid_puts
-------------------------------------------------------------------------------
Ran 1 tests in 1.203s
_trial_temp/test.log
2024-06-05 11:49:09-0500 [-] synapse.http.server - 146 - ERROR - PUT-7 - Failed handle request via 'RoomStateEventRestServlet': <SynapseRequest at 0x7f408b359a00 method='PUT' uri='/_matrix/client/r0/rooms/!iqYybTUtBNrlPoqnZY:red/state/m.room.member/@sid1:red' clientproto='1.1' site='red'>
	Traceback (most recent call last):
	  File "/home/eric/Documents/github/element/synapse/synapse/http/server.py", line 332, in _async_render_wrapper
	    callback_return = await self._async_render(request)
	                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "/home/eric/Documents/github/element/synapse/synapse/http/server.py", line 544, in _async_render
	    callback_return = await raw_callback_return
	                      ^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "/home/eric/Documents/github/element/synapse/synapse/rest/client/room.py", line 295, in on_PUT
	    event_id, _ = await self.room_member_handler.update_membership(
	                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "/home/eric/Documents/github/element/synapse/synapse/handlers/room_member.py", line 666, in update_membership
	    result = await self.update_membership_locked(
	             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "/home/eric/Documents/github/element/synapse/synapse/handlers/room_member.py", line 832, in update_membership_locked
	    if effective_membership_state not in Membership.LIST:
	       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	TypeError: unhashable type: 'list'

Previously, we would just pass whatever we received from the event content into the handler layer that expected a plain string.

Since I changed Membership.LIST to a set, when people sent things like arrays/lists, we got unhashable errors. Whereas previously, the comparison worked fine with a Tuple.

This just adds some validation to the rest layer so the handler can always expect a plain str as the type signature says.

synapse/handlers/sliding_sync.py Outdated Show resolved Hide resolved
@MadLittleMods MadLittleMods merged commit 4a7c586 into develop Jun 6, 2024
38 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/msc3575-sliding-sync-0.0.1 branch June 6, 2024 19:44
@MadLittleMods
Copy link
Collaborator Author

Thanks for the reviews @erikjohnston 🐙

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.

None yet

2 participants