From 30263b43c29620244e277f613b9bdcff9c7ed00b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 11:20:23 +0100 Subject: [PATCH 01/47] Add SlidingSyncStreamToken --- synapse/types/__init__.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 3962ecc9960..23ac1842f83 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1137,6 +1137,43 @@ def is_before_or_eq(self, other_token: "StreamToken") -> bool: ) +@attr.s(slots=True, frozen=True, auto_attribs=True) +class SlidingSyncStreamToken: + """The same as a `StreamToken`, but includes an extra field at the start for + the sliding sync connection token (separated by a '/'). This is used to + store per-connection state. + + This then looks something like: + 5/s2633508_17_338_6732159_1082514_541479_274711_265584_1_379 + """ + + stream_token: StreamToken + connection_token: int + + @staticmethod + @cancellable + async def from_string(store: "DataStore", string: str) -> "SlidingSyncStreamToken": + """Creates a SlidingSyncStreamToken from its textual representation.""" + try: + connection_token_str, stream_token_str = string.split("/", 1) + connection_token = int(connection_token_str) + stream_token = await StreamToken.from_string(store, stream_token_str) + + return SlidingSyncStreamToken( + stream_token=stream_token, + connection_token=connection_token, + ) + except CancelledError: + raise + except Exception: + raise SynapseError(400, "Invalid stream token") + + async def to_string(self, store: "DataStore") -> str: + """Serializes the token to a string""" + stream_token_str = await self.stream_token.to_string(store) + return f"{self.connection_token}/{stream_token_str}" + + @attr.s(slots=True, frozen=True, auto_attribs=True) class PersistedPosition: """Position of a newly persisted row with instance that persisted it.""" From 1ad1cce3f2137f6be2c8d48121de27ea194fef4f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Jul 2024 12:13:40 +0100 Subject: [PATCH 02/47] Pass throught SlidingSyncStreamToken --- synapse/handlers/sliding_sync.py | 24 +++++++++++++++--------- synapse/rest/client/sync.py | 6 ++++-- synapse/types/handlers/__init__.py | 12 +++++++++--- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 1b5262d6670..3aa139ff1c8 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -36,6 +36,7 @@ PersistedEventPosition, Requester, RoomStreamToken, + SlidingSyncStreamToken, StateMap, StreamKeyType, StreamToken, @@ -343,7 +344,7 @@ async def wait_for_sync_for_user( self, requester: Requester, sync_config: SlidingSyncConfig, - from_token: Optional[StreamToken] = None, + from_token: Optional[SlidingSyncStreamToken] = None, timeout_ms: int = 0, ) -> SlidingSyncResult: """ @@ -378,7 +379,7 @@ async def wait_for_sync_for_user( # this returns false, it means we timed out waiting, and we should # just return an empty response. before_wait_ts = self.clock.time_msec() - if not await self.notifier.wait_for_stream_token(from_token): + if not await self.notifier.wait_for_stream_token(from_token.stream_token): logger.warning( "Timed out waiting for worker to catch up. Returning empty response" ) @@ -416,7 +417,7 @@ async def current_sync_callback( sync_config.user.to_string(), timeout_ms, current_sync_callback, - from_token=from_token, + from_token=from_token.stream_token, ) return result @@ -425,7 +426,7 @@ async def current_sync_for_user( self, sync_config: SlidingSyncConfig, to_token: StreamToken, - from_token: Optional[StreamToken] = None, + from_token: Optional[SlidingSyncStreamToken] = None, ) -> SlidingSyncResult: """ Generates the response body of a Sliding Sync result, represented as a @@ -458,7 +459,7 @@ async def current_sync_for_user( await self.get_room_membership_for_user_at_to_token( user=sync_config.user, to_token=to_token, - from_token=from_token, + from_token=from_token.stream_token if from_token else None, ) ) @@ -609,8 +610,11 @@ async def current_sync_for_user( sync_config=sync_config, to_token=to_token ) + # TODO: Update this when we implement per-connection state + connection_token = 0 + return SlidingSyncResult( - next_pos=to_token, + next_pos=SlidingSyncStreamToken(to_token, connection_token), lists=lists, rooms=rooms, extensions=extensions, @@ -1346,7 +1350,7 @@ async def get_room_sync_data( room_id: str, room_sync_config: RoomSyncConfig, room_membership_for_user_at_to_token: _RoomMembershipForUser, - from_token: Optional[StreamToken], + from_token: Optional[SlidingSyncStreamToken], to_token: StreamToken, ) -> SlidingSyncResult.RoomResult: """ @@ -1410,7 +1414,7 @@ async def get_room_sync_data( # - TODO: For an incremental sync where we haven't sent it down this # connection before to_bound = ( - from_token.room_key + from_token.stream_token.room_key if from_token is not None and not room_membership_for_user_at_to_token.newly_joined else None @@ -1477,7 +1481,9 @@ async def get_room_sync_data( instance_name=timeline_event.internal_metadata.instance_name, stream=timeline_event.internal_metadata.stream_ordering, ) - if persisted_position.persisted_after(from_token.room_key): + if persisted_position.persisted_after( + from_token.stream_token.room_key + ): num_live += 1 else: # Since we're iterating over the timeline events in diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 1d8cbfdf00c..b3967db18d8 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -54,7 +54,7 @@ from synapse.http.site import SynapseRequest from synapse.logging.opentracing import trace_with_opname from synapse.rest.admin.experimental_features import ExperimentalFeature -from synapse.types import JsonDict, Requester, StreamToken +from synapse.types import JsonDict, Requester, SlidingSyncStreamToken, StreamToken from synapse.types.rest.client import SlidingSyncBody from synapse.util import json_decoder from synapse.util.caches.lrucache import LruCache @@ -889,7 +889,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: from_token = None if from_token_string is not None: - from_token = await StreamToken.from_string(self.store, from_token_string) + from_token = await SlidingSyncStreamToken.from_string( + self.store, from_token_string + ) # TODO: We currently don't know whether we're going to use sticky params or # maybe some filters like sync v2 where they are built up once and referenced diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 409120470a3..642e5483a9c 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -31,7 +31,13 @@ from pydantic import Extra from synapse.events import EventBase -from synapse.types import JsonDict, JsonMapping, StreamToken, UserID +from synapse.types import ( + JsonDict, + JsonMapping, + SlidingSyncStreamToken, + StreamToken, + UserID, +) from synapse.types.rest.client import SlidingSyncBody if TYPE_CHECKING: @@ -287,7 +293,7 @@ def __bool__(self) -> bool: def __bool__(self) -> bool: return bool(self.to_device) - next_pos: StreamToken + next_pos: SlidingSyncStreamToken lists: Dict[str, SlidingWindowList] rooms: Dict[str, RoomResult] extensions: Extensions @@ -300,7 +306,7 @@ def __bool__(self) -> bool: return bool(self.lists or self.rooms or self.extensions) @staticmethod - def empty(next_pos: StreamToken) -> "SlidingSyncResult": + def empty(next_pos: SlidingSyncStreamToken) -> "SlidingSyncResult": "Return a new empty result" return SlidingSyncResult( next_pos=next_pos, From e8df0d78a2cae99aaeb0ef0e5edd74a397beab96 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Jul 2024 15:53:25 +0100 Subject: [PATCH 03/47] Don't create tokens manually in SSS tests --- tests/rest/client/test_sync.py | 140 ++++++++++++++++++++++++++++----- 1 file changed, 119 insertions(+), 21 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f5d57e689cd..a0b25bc6d60 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2608,7 +2608,22 @@ def test_rooms_newly_joined_incremental_sync(self) -> None: room_id1, "activity before token2", tok=user2_tok ) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] # Join the room after the `from_token` which will make us consider this room as # `newly_joined`. @@ -2630,8 +2645,7 @@ def test_rooms_newly_joined_incremental_sync(self) -> None: # Make an incremental Sliding Sync request (what we're trying to test) channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { @@ -2817,7 +2831,22 @@ def test_rooms_invite_shared_history_incremental_sync(self) -> None: self.helper.send(room_id1, "activity after invite3", tok=user2_tok) self.helper.send(room_id1, "activity after invite4", tok=user2_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] self.helper.send(room_id1, "activity after token5", tok=user2_tok) self.helper.send(room_id1, "activity after toekn6", tok=user2_tok) @@ -2825,8 +2854,7 @@ def test_rooms_invite_shared_history_incremental_sync(self) -> None: # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { @@ -3074,7 +3102,22 @@ def test_rooms_invite_world_readable_history_incremental_sync(self) -> None: self.helper.send(room_id1, "activity after invite3", tok=user2_tok) self.helper.send(room_id1, "activity after invite4", tok=user2_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] self.helper.send(room_id1, "activity after token5", tok=user2_tok) self.helper.send(room_id1, "activity after toekn6", tok=user2_tok) @@ -3082,8 +3125,7 @@ def test_rooms_invite_world_readable_history_incremental_sync(self) -> None: # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { @@ -3239,7 +3281,22 @@ def test_rooms_ban_incremental_sync1(self) -> None: self.helper.send(room_id1, "activity before2", tok=user2_tok) self.helper.join(room_id1, user1_id, tok=user1_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] event_response3 = self.helper.send(room_id1, "activity after3", tok=user2_tok) event_response4 = self.helper.send(room_id1, "activity after4", tok=user2_tok) @@ -3255,8 +3312,7 @@ def test_rooms_ban_incremental_sync1(self) -> None: # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { @@ -3316,15 +3372,29 @@ def test_rooms_ban_incremental_sync2(self) -> None: self.helper.send(room_id1, "activity after3", tok=user2_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] self.helper.send(room_id1, "activity after4", tok=user2_tok) # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { @@ -3451,13 +3521,27 @@ def test_rooms_required_state_incremental_sync(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id1, user1_id, tok=user1_tok) - after_room_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + after_room_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(after_room_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={after_room_token}", { "lists": { "foo-list": { @@ -3729,7 +3813,22 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None: user3_id = self.register_user("user3", "pass") user3_tok = self.login(user3_id, "pass") - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + from_token = channel.json_body["pos"] room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id1, user1_id, tok=user1_tok) @@ -3767,8 +3866,7 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None: # Make the Sliding Sync request with lazy loading for the room members channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": { "foo-list": { From f3030af575d034c82b556eea2fba69e8ce833d14 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Jul 2024 15:58:31 +0100 Subject: [PATCH 04/47] Fix to use new token format --- tests/rest/client/test_sync.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index a0b25bc6d60..60f26e2beb9 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -50,7 +50,14 @@ sync, ) from synapse.server import HomeServer -from synapse.types import JsonDict, RoomStreamToken, StreamKeyType, StreamToken, UserID +from synapse.types import ( + JsonDict, + RoomStreamToken, + SlidingSyncStreamToken, + StreamKeyType, + StreamToken, + UserID, +) from synapse.util import Clock from tests import unittest @@ -1448,7 +1455,7 @@ def test_wait_for_sync_token(self) -> None: ) future_position_token_serialized = self.get_success( - future_position_token.to_string(self.store) + SlidingSyncStreamToken(future_position_token, 0).to_string(self.store) ) # Make the Sliding Sync request From f3a4cfb8b43dd3a1a1d7065363807fc078e2c33f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 12:16:54 +0100 Subject: [PATCH 05/47] Newsfile --- changelog.d/17452.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17452.misc diff --git a/changelog.d/17452.misc b/changelog.d/17452.misc new file mode 100644 index 00000000000..4fd07f617bf --- /dev/null +++ b/changelog.d/17452.misc @@ -0,0 +1 @@ +Change sliding sync to use their own token format in preparation for storing per-connection state. From d44f7e12b1a206b23e4f70f390834e88aba67561 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 15 Jul 2024 14:12:01 +0100 Subject: [PATCH 06/47] WIP/PoC of storing whether we have sent rooms down to clients --- synapse/handlers/sliding_sync.py | 197 +++++++++++++++++++++++++++++++ 1 file changed, 197 insertions(+) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 3aa139ff1c8..1c2ed956609 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -18,6 +18,7 @@ # # import logging +from enum import Enum from itertools import chain from typing import TYPE_CHECKING, Any, Dict, Final, List, Mapping, Optional, Set, Tuple @@ -38,6 +39,7 @@ RoomStreamToken, SlidingSyncStreamToken, StateMap, + StrCollection, StreamKeyType, StreamToken, UserID, @@ -1861,3 +1863,198 @@ async def get_to_device_extensions_response( next_batch=f"{stream_id}", events=messages, ) + + +class HaveSentRoomFlag(Enum): + """Flag for whether we have sent the room down a sliding sync connection. + + The valid state changes here are: + NEVER -> LIVE + LIVE -> PREVIOUSLY + PREVIOUSLY -> LIVE + """ + + # The room has never been sent down (or we have forgotten we have sent it + # down). + NEVER = 1 + + # We have previously sent the room down, but there are updates that we + # haven't sent down. + PREVIOUSLY = 2 + + # We have sent the room down and the client has received all updates. + LIVE = 3 + + +@attr.s(auto_attribs=True, slots=True, frozen=True) +class HaveSentRoom: + """Whether we have sent the room down a sliding sync connection. + + Attributes: + status: Flag of if we have or haven't sent down the room + last_token: If the flag is `PREVIOUSLY` then this is non-null and + contains the last stream token of the last updates we sent down + the room, i.e. we still need to send everything since then to the + client. + """ + + status: HaveSentRoomFlag + last_token: Optional[RoomStreamToken] + + @staticmethod + def previously(last_token: RoomStreamToken) -> "HaveSentRoom": + """Constructor for `PREVIOUSLY` flag.""" + return HaveSentRoom(HaveSentRoomFlag.PREVIOUSLY, last_token) + + +HAVE_SENT_ROOM_NEVER = HaveSentRoom(HaveSentRoomFlag.NEVER, None) +HAVE_SENT_ROOM_LIVE = HaveSentRoom(HaveSentRoomFlag.LIVE, None) + + +@attr.s(auto_attribs=True) +class SlidingSyncConnectionStore: + """In-memory store of per-connection state, including what rooms we have + previously sent down a sliding sync connection. + + Note: This is NOT safe to run in a worker setup. + + The complication here is that we need to handle requests being resent, i.e. + if we sent down a room in a response that the client received, we must + consider the room *not* sent when we get the request again. + + This is handled by using an integer "token", which is returned to the client + as part of the sync token. For each connection we store a mapping from + tokens to the room states, and create a new entry when we send down new + rooms. + + Note that for any given sliding sync connection we will only store a maximum + of two different tokens: the previous token from the request and a new token + sent in the response. When we receive a request with a given token, we then + clear out all other entries with a different token. + + Attributes: + _connections: Mapping from `(user_id, conn_id)` to mapping of `token` + to mapping of room ID to `HaveSentRoom`. + """ + + # `(user_id, conn_id)` -> `token` -> `room_id` -> `HaveSentRoom` + _connections: Dict[Tuple[str, str], Dict[int, Dict[str, HaveSentRoom]]] = ( + attr.Factory(dict) + ) + + async def have_sent_room( + self, user_id: str, conn_id: str, connection_token: int, room_id: str + ) -> HaveSentRoom: + """Whether for the given user_id/conn_id/token, return whether we have + previously sent the room down + """ + + sync_statuses = self._connections.setdefault((user_id, conn_id), {}) + room_status = sync_statuses.get(connection_token, {}).get( + room_id, HAVE_SENT_ROOM_NEVER + ) + + return room_status + + async def record_rooms( + self, + user_id: str, + conn_id: str, + from_token: Optional[SlidingSyncStreamToken], + *, + sent_room_ids: StrCollection, + unsent_room_ids: StrCollection, + ) -> int: + """Record which rooms we have/haven't sent down in a new response + + Attributes: + user_id + conn_id + from_token: The since token from the request, if any + sent_room_ids: The set of room IDs that we have sent down as + part of this request (only needs to be ones we didn't + previously sent down). + unsent_room_ids: The set of room IDs that have had updates + since the `last_room_token`, but which were not included in + this request + """ + prev_connection_token = 0 + if from_token is not None: + prev_connection_token = from_token.connection_token + + # If there are no changes then this is a noop. + if not sent_room_ids and not unsent_room_ids: + return prev_connection_token + + sync_statuses = self._connections.setdefault((user_id, conn_id), {}) + + # Generate a new token, removing any existing entries in that token + # (which can happen if requests get resent). + new_store_token = prev_connection_token + 1 + sync_statuses.pop(new_store_token, None) + + # Copy over and update the room mappings. + new_room_statuses = dict(sync_statuses.get(prev_connection_token, {})) + + # Whether we have updated the `new_room_statuses`, if we don't by the + # end we can treat this as a noop. + have_updated = False + for room_id in sent_room_ids: + new_room_statuses[room_id] = HAVE_SENT_ROOM_LIVE + have_updated = True + + # Whether we add/update the entries for unsent rooms depends on the + # existing entry: + # - LIVE: We have previously sent down everything up to + # `last_room_token, so we update the entry to be `PREVIOUSLY` with + # `last_room_token`. + # - PREVIOUSLY: We have previously sent down everything up to *a* + # given token, so we don't need to update the entry. + # - NEVER: We have never previously sent down the room, and we haven't + # sent anything down this time either so we leave it as NEVER. + + # Work out the new state for unsent rooms that were `LIVE`. + if from_token: + new_unsent_state = HaveSentRoom.previously(from_token.stream_token.room_key) + else: + new_unsent_state = HAVE_SENT_ROOM_NEVER + + for room_id in unsent_room_ids: + prev_state = new_room_statuses.get(room_id) + if prev_state is not None and prev_state.status == HaveSentRoomFlag.LIVE: + new_room_statuses[room_id] = new_unsent_state + have_updated = True + + if not have_updated: + return prev_connection_token + + sync_statuses[new_store_token] = new_room_statuses + + return new_store_token + + async def mark_token_seen( + self, + user_id: str, + conn_id: str, + from_token: Optional[SlidingSyncStreamToken], + ) -> None: + """We have received a request with the given token, so we can clear out + any other tokens associated with the connection. + + If there is no from token then we have started afresh, and so we delete + all tokens associated with the device. + """ + # Clear out any tokens for the connection that doesn't match the one + # from the request. + + sync_statuses = self._connections.pop((user_id, conn_id), {}) + if from_token is None: + return + + sync_statuses = { + i: room_statuses + for i, room_statuses in sync_statuses.items() + if i == from_token.connection_token + } + if sync_statuses: + self._connections[(user_id, conn_id)] = sync_statuses From 53273db3e8f21c302cec6f0d6e699c49e5ef28a2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 13:54:00 +0100 Subject: [PATCH 07/47] Add conn_id field --- synapse/handlers/sliding_sync.py | 2 +- synapse/rest/client/sync.py | 6 +++--- synapse/types/handlers/__init__.py | 28 ++++++++++++++++++++++++++- synapse/types/rest/client/__init__.py | 5 +++++ 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 1c2ed956609..b108e8a4a2d 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1807,7 +1807,7 @@ async def get_to_device_extensions_response( """ user_id = sync_config.user.to_string() - device_id = sync_config.device_id + device_id = sync_config.requester.device_id # Check that this request has a valid device ID (not all requests have # to belong to a device, and so device_id is None), and that the diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index b3967db18d8..2909052bbf8 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -881,7 +881,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ) user = requester.user - device_id = requester.device_id timeout = parse_integer(request, "timeout", default=0) # Position in the stream @@ -902,11 +901,12 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: sync_config = SlidingSyncConfig( user=user, - device_id=device_id, + requester=requester, # FIXME: Currently, we're just manually copying the fields from the - # `SlidingSyncBody` into the config. How can we gurantee into the future + # `SlidingSyncBody` into the config. How can we guarantee into the future # that we don't forget any? I would like something more structured like # `copy_attributes(from=body, to=config)` + conn_id=body.conn_id, lists=body.lists, room_subscriptions=body.room_subscriptions, extensions=body.extensions, diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 642e5483a9c..0c2ab13c93e 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -34,6 +34,7 @@ from synapse.types import ( JsonDict, JsonMapping, + Requester, SlidingSyncStreamToken, StreamToken, UserID, @@ -108,7 +109,7 @@ class SlidingSyncConfig(SlidingSyncBody): """ user: UserID - device_id: Optional[str] + requester: Requester # Pydantic config class Config: @@ -119,6 +120,31 @@ class Config: # Allow custom types like `UserID` to be used in the model arbitrary_types_allowed = True + def connection_id(self) -> str: + """Return a string identifier for this connection. May clash with + connection IDs from different users. + + This is generally a combination of device ID and conn_id. However, both + these two are optional (e.g. puppet access tokens don't have device + IDs), so this handles those edge cases. + """ + + # `conn_id` can be null, in which case we default to the empty string + # (if conn ID is empty then the client can't have multiple sync loops) + conn_id = self.conn_id or "" + + if self.requester.device_id: + return f"D/{self.requester.device_id}/{conn_id}" + + if self.requester.access_token_id: + # If we don't have a device, then the access token ID should be a + # stable ID. + return f"A/{self.requester.access_token_id}/{conn_id}" + + # If we have neither then its likely an AS or some weird token. Either + # way we can just fail here. + raise Exception("Cannot use sliding sync with access token type") + class OperationType(Enum): """ diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index dbe37bc7126..5be8cf53899 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -120,6 +120,9 @@ class SlidingSyncBody(RequestBodyModel): Sliding Sync API request body. Attributes: + conn_id: An optional string to identify this connection to the server. If this + is missing, only 1 sliding sync connection can be made to the server at + any one time. lists: Sliding window API. A map of list key to list information (:class:`SlidingSyncList`). Max lists: 100. The list keys should be arbitrary strings which the client is using to refer to the list. Keep this @@ -315,6 +318,8 @@ def since_token_check( to_device: Optional[ToDeviceExtension] = None + conn_id: Optional[str] + # mypy workaround via https://github.com/pydantic/pydantic/issues/156#issuecomment-1130883884 if TYPE_CHECKING: lists: Optional[Dict[str, SlidingSyncList]] = None From e2a88e44ef6b33a691411ff7ae93d68c2011297e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Jul 2024 12:32:24 +0100 Subject: [PATCH 08/47] Use new room store to track if we've sent a room down --- synapse/handlers/sliding_sync.py | 55 +++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b108e8a4a2d..618c69d5fee 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -24,6 +24,7 @@ import attr from immutabledict import immutabledict +from typing_extensions import assert_never from synapse.api.constants import AccountDataTypes, Direction, EventTypes, Membership from synapse.events import EventBase @@ -342,6 +343,8 @@ def __init__(self, hs: "HomeServer"): self.relations_handler = hs.get_relations_handler() self.rooms_to_exclude_globally = hs.config.server.rooms_to_exclude_from_sync + self.connection_store = SlidingSyncConnectionStore() + async def wait_for_sync_for_user( self, requester: Requester, @@ -449,6 +452,12 @@ async def current_sync_for_user( # See https://github.com/matrix-org/matrix-doc/issues/1144 raise NotImplementedError() + await self.connection_store.mark_token_seen( + user_id, + conn_id=sync_config.connection_id(), + from_token=from_token, + ) + # Get all of the room IDs that the user should be able to see in the sync # response has_lists = sync_config.lists is not None and len(sync_config.lists) > 0 @@ -596,7 +605,7 @@ async def current_sync_for_user( rooms: Dict[str, SlidingSyncResult.RoomResult] = {} for room_id, room_sync_config in relevant_room_map.items(): room_sync_result = await self.get_room_sync_data( - user=sync_config.user, + sync_config=sync_config, room_id=room_id, room_sync_config=room_sync_config, room_membership_for_user_at_to_token=room_membership_for_user_map[ @@ -612,8 +621,18 @@ async def current_sync_for_user( sync_config=sync_config, to_token=to_token ) - # TODO: Update this when we implement per-connection state - connection_token = 0 + if has_lists or has_room_subscriptions: + connection_token = await self.connection_store.record_rooms( + user_id, + conn_id=sync_config.connection_id(), + from_token=from_token, + sent_room_ids=relevant_room_map.keys(), + unsent_room_ids=[], # TODO: We currently ssume that we have sent down all updates. + ) + elif from_token: + connection_token = from_token.connection_token + else: + connection_token = 0 return SlidingSyncResult( next_pos=SlidingSyncStreamToken(to_token, connection_token), @@ -1348,7 +1367,7 @@ async def get_current_state_at( async def get_room_sync_data( self, - user: UserID, + sync_config: SlidingSyncConfig, room_id: str, room_sync_config: RoomSyncConfig, room_membership_for_user_at_to_token: _RoomMembershipForUser, @@ -1370,6 +1389,7 @@ async def get_room_sync_data( from_token: The point in the stream to sync from. to_token: The point in the stream to sync up to. """ + user = sync_config.user # Assemble the list of timeline events # @@ -1413,14 +1433,27 @@ async def get_room_sync_data( # screen of information: # - Initial sync (because no `from_token` to limit us anyway) # - When users `newly_joined` - # - TODO: For an incremental sync where we haven't sent it down this + # - For an incremental sync where we haven't sent it down this # connection before - to_bound = ( - from_token.stream_token.room_key - if from_token is not None - and not room_membership_for_user_at_to_token.newly_joined - else None - ) + + if from_token and not room_membership_for_user_at_to_token.newly_joined: + room_status = await self.connection_store.have_sent_room( + user_id=user.to_string(), + conn_id=sync_config.connection_id(), + connection_token=from_token.connection_token, + room_id=room_id, + ) + if room_status.status == HaveSentRoomFlag.LIVE: + to_bound = from_token.stream_token.room_key + elif room_status.status == HaveSentRoomFlag.PREVIOUSLY: + assert room_status.last_token is not None + to_bound = room_status.last_token + elif room_status.status == HaveSentRoomFlag.NEVER: + to_bound = None + else: + assert_never(room_status.status) + else: + to_bound = None timeline_events, new_room_key = await self.store.paginate_room_events( room_id=room_id, From 185831754ead63f1728674497b0dcb91059091ee Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 10:32:31 +0100 Subject: [PATCH 09/47] Handle initial flag correctly --- synapse/handlers/sliding_sync.py | 66 +++++++++++++++----------------- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 618c69d5fee..32686036b46 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1391,6 +1391,37 @@ async def get_room_sync_data( """ user = sync_config.user + # Determine whether we should limit the timeline to the token range. + # + # We should return historical messages (before token range) in the + # following cases because we want clients to be able to show a basic + # screen of information: + # - Initial sync (because no `from_token` to limit us anyway) + # - When users `newly_joined` + # - For an incremental sync where we haven't sent it down this + # connection before + to_bound = None + initial = True + if from_token and not room_membership_for_user_at_to_token.newly_joined: + room_status = await self.connection_store.have_sent_room( + user_id=user.to_string(), + conn_id=sync_config.connection_id(), + connection_token=from_token.connection_token, + room_id=room_id, + ) + if room_status.status == HaveSentRoomFlag.LIVE: + to_bound = from_token.stream_token.room_key + initial = False + elif room_status.status == HaveSentRoomFlag.PREVIOUSLY: + assert room_status.last_token is not None + to_bound = room_status.last_token + initial = False + elif room_status.status == HaveSentRoomFlag.NEVER: + to_bound = None + initial = True + else: + assert_never(room_status.status) + # Assemble the list of timeline events # # FIXME: It would be nice to make the `rooms` response more uniform regardless of @@ -1426,35 +1457,6 @@ async def get_room_sync_data( room_membership_for_user_at_to_token.event_pos.to_room_stream_token() ) - # Determine whether we should limit the timeline to the token range. - # - # We should return historical messages (before token range) in the - # following cases because we want clients to be able to show a basic - # screen of information: - # - Initial sync (because no `from_token` to limit us anyway) - # - When users `newly_joined` - # - For an incremental sync where we haven't sent it down this - # connection before - - if from_token and not room_membership_for_user_at_to_token.newly_joined: - room_status = await self.connection_store.have_sent_room( - user_id=user.to_string(), - conn_id=sync_config.connection_id(), - connection_token=from_token.connection_token, - room_id=room_id, - ) - if room_status.status == HaveSentRoomFlag.LIVE: - to_bound = from_token.stream_token.room_key - elif room_status.status == HaveSentRoomFlag.PREVIOUSLY: - assert room_status.last_token is not None - to_bound = room_status.last_token - elif room_status.status == HaveSentRoomFlag.NEVER: - to_bound = None - else: - assert_never(room_status.status) - else: - to_bound = None - timeline_events, new_room_key = await self.store.paginate_room_events( room_id=room_id, from_key=from_bound, @@ -1575,12 +1577,6 @@ async def get_room_sync_data( # indicate to the client that a state reset happened. Perhaps we should indicate # this by setting `initial: True` and empty `required_state`. - # TODO: Since we can't determine whether we've already sent a room down this - # Sliding Sync connection before (we plan to add this optimization in the - # future), we're always returning the requested room state instead of - # updates. - initial = True - # Check whether the room has a name set name_state_ids = await self.get_current_state_ids_at( room_id=room_id, From de6e3bdee8715669a76b40eaacdc8728cc02d4eb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 10:32:50 +0100 Subject: [PATCH 10/47] Handle state deltas in non-initial rooms --- synapse/handlers/sliding_sync.py | 14 +++++-- .../storage/databases/main/state_deltas.py | 38 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 32686036b46..6d0d9b425c3 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1720,9 +1720,17 @@ async def get_room_sync_data( to_token=to_token, ) else: - # TODO: Once we can figure out if we've sent a room down this connection before, - # we can return updates instead of the full required state. - raise NotImplementedError() + assert to_bound is not None + + deltas = await self.store.get_current_state_deltas_for_room( + room_id, to_bound, to_token.room_key + ) + # TODO: Filter room state before fetching events + # TODO: Handle state resets where event_id is None + events = await self.store.get_events( + [d.event_id for d in deltas if d.event_id] + ) + room_state = {(s.type, s.state_key): s for s in events.values()} required_room_state: StateMap[EventBase] = {} if required_state_filter != StateFilter.none(): diff --git a/synapse/storage/databases/main/state_deltas.py b/synapse/storage/databases/main/state_deltas.py index 036972ac257..cd6cb2c7a98 100644 --- a/synapse/storage/databases/main/state_deltas.py +++ b/synapse/storage/databases/main/state_deltas.py @@ -26,6 +26,8 @@ from synapse.storage._base import SQLBaseStore from synapse.storage.database import LoggingTransaction +from synapse.storage.databases.main.stream import _filter_results_by_stream +from synapse.types import RoomStreamToken from synapse.util.caches.stream_change_cache import StreamChangeCache logger = logging.getLogger(__name__) @@ -156,3 +158,39 @@ async def get_max_stream_id_in_current_state_deltas(self) -> int: "get_max_stream_id_in_current_state_deltas", self._get_max_stream_id_in_current_state_deltas_txn, ) + + async def get_current_state_deltas_for_room( + self, room_id: str, from_token: RoomStreamToken, to_token: RoomStreamToken + ) -> List[StateDelta]: + """Get the state deltas between that have happened between two + tokens.""" + + def get_current_state_deltas_for_room_txn( + txn: LoggingTransaction, + ) -> List[StateDelta]: + sql = """ + SELECT instance_name, stream_id, type, state_key, event_id, prev_event_id + FROM current_state_delta_stream + WHERE room_id = ? AND ? < stream_id AND stream_id <= ? + ORDER BY stream_id ASC + """ + txn.execute( + sql, (room_id, from_token.stream, to_token.get_max_stream_pos()) + ) + + return [ + StateDelta( + stream_id=row[1], + room_id=room_id, + event_type=row[2], + state_key=row[3], + event_id=row[4], + prev_event_id=row[5], + ) + for row in txn + if _filter_results_by_stream(from_token, to_token, row[0], row[1]) + ] + + return await self.db_pool.runInteraction( + "get_current_state_deltas_for_room", get_current_state_deltas_for_room_txn + ) From e2c47bf4e82f32b1394a204dc1b3cc33d9ca8212 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 13:02:31 +0100 Subject: [PATCH 11/47] Fix tests --- tests/rest/client/test_sync.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 60f26e2beb9..b8d2a0a2d0e 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -3567,21 +3567,9 @@ def test_rooms_required_state_incremental_sync(self) -> None: ) self.assertEqual(channel.code, 200, channel.json_body) - state_map = self.get_success( - self.storage_controllers.state.get_current_state(room_id1) - ) - - # The returned state doesn't change from initial to incremental sync. In the - # future, we will only return updates but only if we've sent the room down the + # We only return updates but only if we've sent the room down the # connection before. - self._assertRequiredStateIncludes( - channel.json_body["rooms"][room_id1]["required_state"], - { - state_map[(EventTypes.Create, "")], - state_map[(EventTypes.RoomHistoryVisibility, "")], - }, - exact=True, - ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_wildcard(self) -> None: From a90c40812aed1e9217703eb141568de7df63cfad Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 14:02:17 +0100 Subject: [PATCH 12/47] Newsfile --- changelog.d/17447.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17447.feature diff --git a/changelog.d/17447.feature b/changelog.d/17447.feature new file mode 100644 index 00000000000..6f80e298aea --- /dev/null +++ b/changelog.d/17447.feature @@ -0,0 +1 @@ +Track which rooms have been sent to clients in the experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From 2968f2e3b82838a5668a03d789670a250b54d078 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 14:10:37 +0100 Subject: [PATCH 13/47] Bump typing-extensions for 'assert_never' --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 41de90f9f68..3df42ae4486 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -201,8 +201,8 @@ netaddr = ">=0.7.18" # add a lower bound to the Jinja2 dependency. Jinja2 = ">=3.0" bleach = ">=1.4.3" -# We use `Self`, which were added in `typing-extensions` 4.0. -typing-extensions = ">=4.0" +# We use `assert_never`, which were added in `typing-extensions` 4.1. +typing-extensions = ">=4.1" # We enforce that we have a `cryptography` version that bundles an `openssl` # with the latest security patches. cryptography = ">=3.4.7" From 27848818c28e721f19a2a11dd5d819ad971d015d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Jul 2024 14:27:55 +0100 Subject: [PATCH 14/47] Add tests --- tests/rest/client/test_sync.py | 181 +++++++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index b8d2a0a2d0e..ada95bba22e 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4323,6 +4323,187 @@ def test_room_subscriptions_world_readable(self) -> None: channel.json_body["rooms"].get(room_id1), channel.json_body["rooms"] ) + def test_incremental_sync_incremental_state(self) -> None: + """Test that we only get state updates in incremental sync for rooms + we've already seen. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + from_token = channel.json_body["pos"] + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + state_map[(EventTypes.RoomHistoryVisibility, "")], + }, + exact=True, + ) + + # Send a state event + self.helper.send_state( + room_id1, EventTypes.Name, body={"name": "foo"}, tok=user2_tok + ) + + channel = self.make_request( + "POST", + self.sync_endpoint + f"?pos={from_token}", + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + [EventTypes.Name, ""], + ], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + + def test_incremental_sync_full_state_new_room(self) -> None: + """Test that we get state all state in incremental sync for rooms that + we haven't seen before. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id2, user1_id, tok=user1_tok) + + # Make the Sliding Sync request, we'll only receive room_id2 + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + from_token = channel.json_body["pos"] + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id2) + ) + + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id2]["required_state"], + { + state_map[(EventTypes.Create, "")], + state_map[(EventTypes.RoomHistoryVisibility, "")], + }, + exact=True, + ) + self.assertNotIn(room_id1, channel.json_body["rooms"]) + + # Send a state event in room 1 + self.helper.send_state( + room_id1, EventTypes.Name, body={"name": "foo"}, tok=user2_tok + ) + + # We should get room_id1 down sync, with the full state. + channel = self.make_request( + "POST", + self.sync_endpoint + f"?pos={from_token}", + { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + [EventTypes.Name, ""], + ], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + state_map[(EventTypes.RoomHistoryVisibility, "")], + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + class SlidingSyncToDeviceExtensionTestCase(unittest.HomeserverTestCase): """Tests for the to-device sliding sync extension""" From d689204aa6714fa12720b0ed3f5ea1c7f4baaf87 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 11:52:14 +0100 Subject: [PATCH 15/47] Add docstring --- synapse/types/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 23ac1842f83..53401ade844 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1145,6 +1145,12 @@ class SlidingSyncStreamToken: This then looks something like: 5/s2633508_17_338_6732159_1082514_541479_274711_265584_1_379 + + Attributes: + stream_token: Token representing the position of all the standard + streams. + connection_token: Token used by sliding sync to track updates to any + per-connection state stored by Synapse. """ stream_token: StreamToken From 560087bf87f501145858b52e11b2dd4d6a555a53 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 12:00:48 +0100 Subject: [PATCH 16/47] Remove '_token' prefix --- synapse/handlers/sliding_sync.py | 12 +++++------- synapse/types/__init__.py | 16 ++++++++-------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 3aa139ff1c8..80ca3fa587e 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -379,7 +379,7 @@ async def wait_for_sync_for_user( # this returns false, it means we timed out waiting, and we should # just return an empty response. before_wait_ts = self.clock.time_msec() - if not await self.notifier.wait_for_stream_token(from_token.stream_token): + if not await self.notifier.wait_for_stream_token(from_token.stream): logger.warning( "Timed out waiting for worker to catch up. Returning empty response" ) @@ -417,7 +417,7 @@ async def current_sync_callback( sync_config.user.to_string(), timeout_ms, current_sync_callback, - from_token=from_token.stream_token, + from_token=from_token.stream, ) return result @@ -459,7 +459,7 @@ async def current_sync_for_user( await self.get_room_membership_for_user_at_to_token( user=sync_config.user, to_token=to_token, - from_token=from_token.stream_token if from_token else None, + from_token=from_token.stream if from_token else None, ) ) @@ -1414,7 +1414,7 @@ async def get_room_sync_data( # - TODO: For an incremental sync where we haven't sent it down this # connection before to_bound = ( - from_token.stream_token.room_key + from_token.stream.room_key if from_token is not None and not room_membership_for_user_at_to_token.newly_joined else None @@ -1481,9 +1481,7 @@ async def get_room_sync_data( instance_name=timeline_event.internal_metadata.instance_name, stream=timeline_event.internal_metadata.stream_ordering, ) - if persisted_position.persisted_after( - from_token.stream_token.room_key - ): + if persisted_position.persisted_after(from_token.stream.room_key): num_live += 1 else: # Since we're iterating over the timeline events in diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 53401ade844..a0956febc06 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1147,14 +1147,14 @@ class SlidingSyncStreamToken: 5/s2633508_17_338_6732159_1082514_541479_274711_265584_1_379 Attributes: - stream_token: Token representing the position of all the standard + stream: Token representing the position of all the standard streams. - connection_token: Token used by sliding sync to track updates to any + connection: Token used by sliding sync to track updates to any per-connection state stored by Synapse. """ - stream_token: StreamToken - connection_token: int + stream: StreamToken + connection: int @staticmethod @cancellable @@ -1166,8 +1166,8 @@ async def from_string(store: "DataStore", string: str) -> "SlidingSyncStreamToke stream_token = await StreamToken.from_string(store, stream_token_str) return SlidingSyncStreamToken( - stream_token=stream_token, - connection_token=connection_token, + stream=stream_token, + connection=connection_token, ) except CancelledError: raise @@ -1176,8 +1176,8 @@ async def from_string(store: "DataStore", string: str) -> "SlidingSyncStreamToke async def to_string(self, store: "DataStore") -> str: """Serializes the token to a string""" - stream_token_str = await self.stream_token.to_string(store) - return f"{self.connection_token}/{stream_token_str}" + stream_token_str = await self.stream.to_string(store) + return f"{self.connection}/{stream_token_str}" @attr.s(slots=True, frozen=True, auto_attribs=True) From 40d9587e04b6e38d4dcbd940c0ccee62e8656de3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 13:53:26 +0100 Subject: [PATCH 17/47] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync.py | 14 ++++++++------ synapse/storage/databases/main/state_deltas.py | 3 +-- tests/rest/client/test_sync.py | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 6d0d9b425c3..b70d050512f 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -627,11 +627,13 @@ async def current_sync_for_user( conn_id=sync_config.connection_id(), from_token=from_token, sent_room_ids=relevant_room_map.keys(), - unsent_room_ids=[], # TODO: We currently ssume that we have sent down all updates. + # TODO: We need to calculate which rooms have had updates since the `from_token` but were not included in the `sent_room_ids` + unsent_room_ids=[], ) elif from_token: connection_token = from_token.connection_token else: + # Initial sync without a `from_token` starts a `0` connection_token = 0 return SlidingSyncResult( @@ -1982,7 +1984,7 @@ class SlidingSyncConnectionStore: async def have_sent_room( self, user_id: str, conn_id: str, connection_token: int, room_id: str ) -> HaveSentRoom: - """Whether for the given user_id/conn_id/token, return whether we have + """For the given user_id/conn_id/token, return whether we have previously sent the room down """ @@ -2012,7 +2014,7 @@ async def record_rooms( part of this request (only needs to be ones we didn't previously sent down). unsent_room_ids: The set of room IDs that have had updates - since the `last_room_token`, but which were not included in + since the `from_token`, but which were not included in this request """ prev_connection_token = 0 @@ -2089,9 +2091,9 @@ async def mark_token_seen( return sync_statuses = { - i: room_statuses - for i, room_statuses in sync_statuses.items() - if i == from_token.connection_token + connection_token: room_statuses + for connection_token, room_statuses in sync_statuses.items() + if connection_token == from_token.connection_token } if sync_statuses: self._connections[(user_id, conn_id)] = sync_statuses diff --git a/synapse/storage/databases/main/state_deltas.py b/synapse/storage/databases/main/state_deltas.py index cd6cb2c7a98..da3ebe66b88 100644 --- a/synapse/storage/databases/main/state_deltas.py +++ b/synapse/storage/databases/main/state_deltas.py @@ -162,8 +162,7 @@ async def get_max_stream_id_in_current_state_deltas(self) -> int: async def get_current_state_deltas_for_room( self, room_id: str, from_token: RoomStreamToken, to_token: RoomStreamToken ) -> List[StateDelta]: - """Get the state deltas between that have happened between two - tokens.""" + """Get the state deltas between two tokens.""" def get_current_state_deltas_for_room_txn( txn: LoggingTransaction, diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index ada95bba22e..8ab72c48a30 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4411,7 +4411,7 @@ def test_incremental_sync_incremental_state(self) -> None: ) def test_incremental_sync_full_state_new_room(self) -> None: - """Test that we get state all state in incremental sync for rooms that + """Test that we get all state in incremental sync for rooms that we haven't seen before. """ From 14eb781905fb52a8caf4e579125d1843085d4508 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 13:56:21 +0100 Subject: [PATCH 18/47] Reword doc --- synapse/types/handlers/__init__.py | 4 ++-- synapse/types/rest/client/__init__.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 0c2ab13c93e..f98b3cad0c5 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -129,8 +129,8 @@ def connection_id(self) -> str: IDs), so this handles those edge cases. """ - # `conn_id` can be null, in which case we default to the empty string - # (if conn ID is empty then the client can't have multiple sync loops) + # If this is missing, only one sliding sync connection is allowed per + # given conn_id. conn_id = self.conn_id or "" if self.requester.device_id: diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index 5be8cf53899..c7d806de774 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -121,8 +121,7 @@ class SlidingSyncBody(RequestBodyModel): Attributes: conn_id: An optional string to identify this connection to the server. If this - is missing, only 1 sliding sync connection can be made to the server at - any one time. + is missing, only one sliding sync connection is allowed per given conn_id. lists: Sliding window API. A map of list key to list information (:class:`SlidingSyncList`). Max lists: 100. The list keys should be arbitrary strings which the client is using to refer to the list. Keep this From 9ae2551389a56bd9e2056496f51748a9d09a1f81 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 13:59:22 +0100 Subject: [PATCH 19/47] Keyword args --- synapse/handlers/sliding_sync.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b70d050512f..5348385c546 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1725,7 +1725,9 @@ async def get_room_sync_data( assert to_bound is not None deltas = await self.store.get_current_state_deltas_for_room( - room_id, to_bound, to_token.room_key + room_id=room_id, + from_token=to_bound, + to_token=to_token.room_key, ) # TODO: Filter room state before fetching events # TODO: Handle state resets where event_id is None From 51f7602cd3fb6afe15173383a140051eb3b85649 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 14:04:56 +0100 Subject: [PATCH 20/47] Rename bounds --- synapse/handlers/sliding_sync.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 5348385c546..cb45af79ea0 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1402,7 +1402,7 @@ async def get_room_sync_data( # - When users `newly_joined` # - For an incremental sync where we haven't sent it down this # connection before - to_bound = None + from_bound = None initial = True if from_token and not room_membership_for_user_at_to_token.newly_joined: room_status = await self.connection_store.have_sent_room( @@ -1412,14 +1412,14 @@ async def get_room_sync_data( room_id=room_id, ) if room_status.status == HaveSentRoomFlag.LIVE: - to_bound = from_token.stream_token.room_key + from_bound = from_token.stream_token.room_key initial = False elif room_status.status == HaveSentRoomFlag.PREVIOUSLY: assert room_status.last_token is not None - to_bound = room_status.last_token + from_bound = room_status.last_token initial = False elif room_status.status == HaveSentRoomFlag.NEVER: - to_bound = None + from_bound = None initial = True else: assert_never(room_status.status) @@ -1449,20 +1449,22 @@ async def get_room_sync_data( prev_batch_token = to_token # We're going to paginate backwards from the `to_token` - from_bound = to_token.room_key + to_bound = to_token.room_key # People shouldn't see past their leave/ban event if room_membership_for_user_at_to_token.membership in ( Membership.LEAVE, Membership.BAN, ): - from_bound = ( + to_bound = ( room_membership_for_user_at_to_token.event_pos.to_room_stream_token() ) timeline_events, new_room_key = await self.store.paginate_room_events( room_id=room_id, - from_key=from_bound, - to_key=to_bound, + # Because we want the latest events first the bounds are + # reversed. + from_key=to_bound, + to_key=from_bound, direction=Direction.BACKWARDS, # We add one so we can determine if there are enough events to saturate # the limit or not (see `limited`) @@ -1722,11 +1724,11 @@ async def get_room_sync_data( to_token=to_token, ) else: - assert to_bound is not None + assert from_bound is not None deltas = await self.store.get_current_state_deltas_for_room( room_id=room_id, - from_token=to_bound, + from_token=from_bound, to_token=to_token.room_key, ) # TODO: Filter room state before fetching events From eab092b7b4ca813c8a90e4b8c1b48e05a424584d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 14:06:50 +0100 Subject: [PATCH 21/47] Add context to conn_id --- synapse/types/handlers/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index f98b3cad0c5..6396be839f1 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -127,6 +127,11 @@ def connection_id(self) -> str: This is generally a combination of device ID and conn_id. However, both these two are optional (e.g. puppet access tokens don't have device IDs), so this handles those edge cases. + + We use this over the raw `conn_id` to avoid clashes between different + clients that use the same `conn_id`. Imagine a user uses a web client + that uses `conn_id: main_sync_loop` and an Android client that also has + a `conn_id: main_sync_loop`. """ # If this is missing, only one sliding sync connection is allowed per From 605b358d4d719b81e52800c1bc53d9547905c180 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 14:14:38 +0100 Subject: [PATCH 22/47] Refactor to avoid SyncConfig.connection_id() --- synapse/handlers/sliding_sync.py | 65 ++++++++++++++++++++++-------- synapse/types/handlers/__init__.py | 30 -------------- 2 files changed, 48 insertions(+), 47 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index cb45af79ea0..75603b6f759 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -453,8 +453,7 @@ async def current_sync_for_user( raise NotImplementedError() await self.connection_store.mark_token_seen( - user_id, - conn_id=sync_config.connection_id(), + sync_config=sync_config, from_token=from_token, ) @@ -623,8 +622,7 @@ async def current_sync_for_user( if has_lists or has_room_subscriptions: connection_token = await self.connection_store.record_rooms( - user_id, - conn_id=sync_config.connection_id(), + sync_config=sync_config, from_token=from_token, sent_room_ids=relevant_room_map.keys(), # TODO: We need to calculate which rooms have had updates since the `from_token` but were not included in the `sent_room_ids` @@ -1406,8 +1404,7 @@ async def get_room_sync_data( initial = True if from_token and not room_membership_for_user_at_to_token.newly_joined: room_status = await self.connection_store.have_sent_room( - user_id=user.to_string(), - conn_id=sync_config.connection_id(), + sync_config=sync_config, connection_token=from_token.connection_token, room_id=room_id, ) @@ -1986,13 +1983,14 @@ class SlidingSyncConnectionStore: ) async def have_sent_room( - self, user_id: str, conn_id: str, connection_token: int, room_id: str + self, sync_config: SlidingSyncConfig, connection_token: int, room_id: str ) -> HaveSentRoom: """For the given user_id/conn_id/token, return whether we have previously sent the room down """ - sync_statuses = self._connections.setdefault((user_id, conn_id), {}) + conn_key = self._get_connection_key(sync_config) + sync_statuses = self._connections.setdefault(conn_key, {}) room_status = sync_statuses.get(connection_token, {}).get( room_id, HAVE_SENT_ROOM_NEVER ) @@ -2001,8 +1999,7 @@ async def have_sent_room( async def record_rooms( self, - user_id: str, - conn_id: str, + sync_config: SlidingSyncConfig, from_token: Optional[SlidingSyncStreamToken], *, sent_room_ids: StrCollection, @@ -2011,8 +2008,7 @@ async def record_rooms( """Record which rooms we have/haven't sent down in a new response Attributes: - user_id - conn_id + sync_config from_token: The since token from the request, if any sent_room_ids: The set of room IDs that we have sent down as part of this request (only needs to be ones we didn't @@ -2029,7 +2025,8 @@ async def record_rooms( if not sent_room_ids and not unsent_room_ids: return prev_connection_token - sync_statuses = self._connections.setdefault((user_id, conn_id), {}) + conn_key = self._get_connection_key(sync_config) + sync_statuses = self._connections.setdefault(conn_key, {}) # Generate a new token, removing any existing entries in that token # (which can happen if requests get resent). @@ -2077,8 +2074,7 @@ async def record_rooms( async def mark_token_seen( self, - user_id: str, - conn_id: str, + sync_config: SlidingSyncConfig, from_token: Optional[SlidingSyncStreamToken], ) -> None: """We have received a request with the given token, so we can clear out @@ -2090,7 +2086,8 @@ async def mark_token_seen( # Clear out any tokens for the connection that doesn't match the one # from the request. - sync_statuses = self._connections.pop((user_id, conn_id), {}) + conn_key = self._get_connection_key(sync_config) + sync_statuses = self._connections.pop(conn_key, {}) if from_token is None: return @@ -2100,4 +2097,38 @@ async def mark_token_seen( if connection_token == from_token.connection_token } if sync_statuses: - self._connections[(user_id, conn_id)] = sync_statuses + self._connections[conn_key] = sync_statuses + + @staticmethod + def _get_connection_key(sync_config: SlidingSyncConfig) -> Tuple[str, str]: + """Return a unique identifier for this connection. + + The first part is simply the user ID. + + The second part is generally a combination of device ID and conn_id. + However, both these two are optional (e.g. puppet access tokens don't + have device IDs), so this handles those edge cases. + + We use this over the raw `conn_id` to avoid clashes between different + clients that use the same `conn_id`. Imagine a user uses a web client + that uses `conn_id: main_sync_loop` and an Android client that also has + a `conn_id: main_sync_loop`. + """ + + user_id = sync_config.user.to_string() + + # If this is missing, only one sliding sync connection is allowed per + # given conn_id. + conn_id = sync_config.conn_id or "" + + if sync_config.requester.device_id: + return (user_id, f"D/{sync_config.requester.device_id}/{conn_id}") + + if sync_config.requester.access_token_id: + # If we don't have a device, then the access token ID should be a + # stable ID. + return (user_id, f"A/{sync_config.requester.access_token_id}/{conn_id}") + + # If we have neither then its likely an AS or some weird token. Either + # way we can just fail here. + raise Exception("Cannot use sliding sync with access token type") diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 6396be839f1..1b9de129ba8 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -120,36 +120,6 @@ class Config: # Allow custom types like `UserID` to be used in the model arbitrary_types_allowed = True - def connection_id(self) -> str: - """Return a string identifier for this connection. May clash with - connection IDs from different users. - - This is generally a combination of device ID and conn_id. However, both - these two are optional (e.g. puppet access tokens don't have device - IDs), so this handles those edge cases. - - We use this over the raw `conn_id` to avoid clashes between different - clients that use the same `conn_id`. Imagine a user uses a web client - that uses `conn_id: main_sync_loop` and an Android client that also has - a `conn_id: main_sync_loop`. - """ - - # If this is missing, only one sliding sync connection is allowed per - # given conn_id. - conn_id = self.conn_id or "" - - if self.requester.device_id: - return f"D/{self.requester.device_id}/{conn_id}" - - if self.requester.access_token_id: - # If we don't have a device, then the access token ID should be a - # stable ID. - return f"A/{self.requester.access_token_id}/{conn_id}" - - # If we have neither then its likely an AS or some weird token. Either - # way we can just fail here. - raise Exception("Cannot use sliding sync with access token type") - class OperationType(Enum): """ From 37c4463c8ed5af357f66c89bbfda2430fe45384f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 15:01:51 +0100 Subject: [PATCH 23/47] Add some unit tests for 'get_room_sync_data' --- tests/handlers/test_sliding_sync.py | 399 +++++++++++++++++++++++++++- 1 file changed, 396 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index a7aa9bb8afe..6c63719ca53 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -19,8 +19,8 @@ # import logging from copy import deepcopy -from typing import Dict, Optional -from unittest.mock import patch +from typing import Dict, Optional, Tuple +from unittest.mock import Mock, patch from parameterized import parameterized @@ -46,7 +46,13 @@ from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import JsonDict, StreamToken, UserID +from synapse.types import ( + JsonDict, + SlidingSyncStreamToken, + StreamToken, + UserID, + create_requester, +) from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock @@ -3759,3 +3765,390 @@ def test_default_bump_event_types(self) -> None: # We only care about the *latest* event in the room. [room_id1, room_id2], ) + + +class GetRoomSyncDataTestCase(HomeserverTestCase): + """ + Tests for Sliding Sync handler `get_room_sync_data()` + """ + + servlets = [ + admin.register_servlets, + knock.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def default_config(self) -> JsonDict: + config = super().default_config() + # Enable sliding sync + config["experimental_features"] = {"msc3575_enabled": True} + return config + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.sliding_sync_handler = self.hs.get_sliding_sync_handler() + self.store = self.hs.get_datastores().main + self.event_sources = hs.get_event_sources() + + def _create_sync_configs( + self, + user_id, + room_id, + timeline_limit=5, + ) -> Tuple[SlidingSyncConfig, RoomSyncConfig, _RoomMembershipForUser]: + """Create the configs necessary to call `get_room_sync_data`""" + requester = create_requester(user_id, device_id="foo_device") + sync_config = SlidingSyncConfig( + user=requester.user, + requester=requester, + conn_id="conn_id", + lists={ + "list": SlidingSyncConfig.SlidingSyncList( + timeline_limit=timeline_limit, + required_state=[ + (EventTypes.Name, ""), + ], + ), + }, + room_subscriptions={}, + extensions=None, + ) + + room_sync_config = RoomSyncConfig(timeline_limit, {EventTypes.Name: {""}}) + + rooms = self.get_success( + self.store.get_rooms_for_local_user_where_membership_is( + user_id, membership_list=[Membership.JOIN] + ) + ) + room_for_user = rooms[0] + assert room_for_user.room_id == room_id + + room_membership_for_user_at_to_token = _RoomMembershipForUser( + room_id=room_id, + event_id=room_for_user.event_id, + event_pos=room_for_user.event_pos, + membership=Membership.JOIN, + sender=user_id, + newly_joined=False, + newly_left=False, + is_dm=False, + ) + + return (sync_config, room_sync_config, room_membership_for_user_at_to_token) + + def test_room_sync_data_initial(self) -> None: + """Tests getting room sync data with no from token""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as( + user1_id, + tok=user1_tok, + ) + + sync_config, room_sync_config, room_membership_for_user_at_to_token = ( + self._create_sync_configs(user1_id, room_id1, timeline_limit=5) + ) + + # Timeline limit is 5 so let's send 5 messages that we'll expect to get + # back. + expected_timeline = [] + for _ in range(5): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + to_token = self.event_sources.get_current_token() + + result = self.get_success( + self.sliding_sync_handler.get_room_sync_data( + sync_config, + room_id=room_id1, + room_sync_config=room_sync_config, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + from_token=None, + to_token=to_token, + ) + ) + + self.assertTrue(result.initial) + self.assertTrue(result.limited) + self.assertEqual( + [e.event_id for e in result.timeline_events], expected_timeline + ) + + def test_room_sync_data_incremental_live(self) -> None: + """Test getting room data where we have previously sent down the room + and its state is considered LIVE""" + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as( + user1_id, + tok=user1_tok, + ) + + sync_config, room_sync_config, room_membership_for_user_at_to_token = ( + self._create_sync_configs(user1_id, room_id1) + ) + + # These messages are sent before the `from_token`, so we don't expect to + # see these messages. + for _ in range(5): + r = self.helper.send(room_id1, "message", tok=user1_tok) + + # Mark the room as having been sent down, and create an appropriate + # `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] + ) + ) + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # These messages are sent after the `from_token`, so we expect to only + # see these messages. + expected_timeline = [] + for _ in range(2): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + to_token = self.event_sources.get_current_token() + + result = self.get_success( + self.sliding_sync_handler.get_room_sync_data( + sync_config, + room_id=room_id1, + room_sync_config=room_sync_config, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + from_token=from_token, + to_token=to_token, + ) + ) + + self.assertFalse(result.initial) + self.assertFalse(result.limited) + self.assertEqual( + [e.event_id for e in result.timeline_events], expected_timeline + ) + + def test_room_sync_data_incremental_previously_not_limited(self) -> None: + """Test getting room data where we have previously sent down the room, + but we missed sending down some data previously and so its state is + considered PREVIOUSLY. + + In this case there has been more than timeline limit events. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as( + user1_id, + tok=user1_tok, + ) + + sync_config, room_sync_config, room_membership_for_user_at_to_token = ( + self._create_sync_configs(user1_id, room_id1) + ) + + # These messages are sent before the initial `from_token`, so we don't + # expect to see these messages. + for _ in range(5): + r = self.helper.send(room_id1, "message", tok=user1_tok) + + # Mark the room as having been sent down, and create an appropriate + # `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] + ) + ) + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # These messages are sent after the initial `from_token`, so we expect + # to see these messages. + expected_timeline = [] + for _ in range(2): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + # Mark the room as *not* having been sent down, and create a new + # `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, from_token, sent_room_ids=[], unsent_room_ids=[room_id1] + ) + ) + + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # We should also receive new messages + for _ in range(2): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + to_token = self.event_sources.get_current_token() + + result = self.get_success( + self.sliding_sync_handler.get_room_sync_data( + sync_config, + room_id=room_id1, + room_sync_config=room_sync_config, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + from_token=from_token, + to_token=to_token, + ) + ) + + self.assertFalse(result.initial) + self.assertFalse(result.limited) + self.assertEqual( + [e.event_id for e in result.timeline_events], expected_timeline + ) + + def test_room_sync_data_incremental_previously_limited(self) -> None: + """Test getting room data where we have previously sent down the room, + but we missed sending down some data previously and so its state is + considered PREVIOUSLY. + + In this case there has been fewer than timeline limit events. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as( + user1_id, + tok=user1_tok, + ) + + sync_config, room_sync_config, room_membership_for_user_at_to_token = ( + self._create_sync_configs(user1_id, room_id1) + ) + + # These messages are sent before the initial `from_token`, so we don't + # expect to see these messages. + for _ in range(5): + r = self.helper.send(room_id1, "message", tok=user1_tok) + + # Mark the room as having been sent down, and create an appropriate + # `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] + ) + ) + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # These messages are sent after the initial `from_token`, but are before + # the timeline limit, so we don't expect to see these messages. + for _ in range(5): + r = self.helper.send(room_id1, "message", tok=user1_tok) + + # ... but these messages are within the timeline limit, so we do expect + # to see them + expected_timeline = [] + for _ in range(3): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + # Mark the room as *not* having been sent down, and create a new + # `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, from_token, sent_room_ids=[], unsent_room_ids=[room_id1] + ) + ) + + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # We should also receive new messages + for _ in range(2): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + to_token = self.event_sources.get_current_token() + + result = self.get_success( + self.sliding_sync_handler.get_room_sync_data( + sync_config, + room_id=room_id1, + room_sync_config=room_sync_config, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + from_token=from_token, + to_token=to_token, + ) + ) + + self.assertFalse(result.initial) + self.assertTrue(result.limited) + self.assertEqual( + [e.event_id for e in result.timeline_events], expected_timeline + ) + + def test_room_sync_data_incremental_never(self) -> None: + """Test getting room data where we have not previously sent down the room, + so its state is considered NEVER. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as( + user1_id, + tok=user1_tok, + ) + + sync_config, room_sync_config, room_membership_for_user_at_to_token = ( + self._create_sync_configs(user1_id, room_id1) + ) + + # We expect to see these messages even though they're before the + # `from_token`, as the room has not been sent down. + expected_timeline = [] + for _ in range(2): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + # Create a new `from_token`. + connection_token = self.get_success( + self.sliding_sync_handler.connection_store.record_rooms( + sync_config, None, sent_room_ids=[], unsent_room_ids=[] + ) + ) + from_token = SlidingSyncStreamToken( + self.event_sources.get_current_token(), connection_token + ) + + # We should also receive new messages + for _ in range(3): + r = self.helper.send(room_id1, "message", tok=user1_tok) + expected_timeline.append(r["event_id"]) + + to_token = self.event_sources.get_current_token() + + result = self.get_success( + self.sliding_sync_handler.get_room_sync_data( + sync_config, + room_id=room_id1, + room_sync_config=room_sync_config, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + from_token=from_token, + to_token=to_token, + ) + ) + + self.assertTrue(result.initial) + self.assertTrue(result.limited) + self.assertEqual( + [e.event_id for e in result.timeline_events], expected_timeline + ) From 532594e08d922f7cf97ddf50227dc3f4d3de1ff6 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 18 Jul 2024 15:52:00 +0100 Subject: [PATCH 24/47] Fix linting in tests --- tests/handlers/test_sliding_sync.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 6c63719ca53..5e91be00265 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -20,7 +20,7 @@ import logging from copy import deepcopy from typing import Dict, Optional, Tuple -from unittest.mock import Mock, patch +from unittest.mock import patch from parameterized import parameterized @@ -3792,9 +3792,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def _create_sync_configs( self, - user_id, - room_id, - timeline_limit=5, + user_id: str, + room_id: str, + timeline_limit: int = 5, ) -> Tuple[SlidingSyncConfig, RoomSyncConfig, _RoomMembershipForUser]: """Create the configs necessary to call `get_room_sync_data`""" requester = create_requester(user_id, device_id="foo_device") From 6f738a4ab854cbae206ca3b0f4476aaf99c78202 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 23 Jul 2024 11:38:09 +0100 Subject: [PATCH 25/47] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 12385929178..543a1f88366 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -640,7 +640,7 @@ async def handle_room(room_id: str) -> None: elif from_token: connection_token = from_token.connection else: - # Initial sync without a `from_token` starts a `0` + # Initial sync without a `from_token` starts at `0` connection_token = 0 return SlidingSyncResult( @@ -1467,8 +1467,9 @@ async def get_room_sync_data( timeline_events, new_room_key = await self.store.paginate_room_events( room_id=room_id, - # Because we want the latest events first the bounds are - # reversed. + # The bounds are reversed so we can paginate backwards + # (from newer to older events) starting at to_bound. + # This ensures we fill the `limit` with the newest events first, from_key=to_bound, to_key=from_bound, direction=Direction.BACKWARDS, From 0c4e633cac52881edc23478797a6fd45e0b8fb40 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 23 Jul 2024 17:42:16 +0100 Subject: [PATCH 26/47] Fix tests --- tests/rest/client/test_sync.py | 132 ++++++++++++++++++++++++++------- 1 file changed, 106 insertions(+), 26 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 843e9adca11..68954a0a47a 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1551,14 +1551,26 @@ def test_wait_for_new_data(self) -> None: room_id = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id, user1_id, tok=user1_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [], + "timeline_limit": 1, + } + } + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?timeout=10000&pos={from_token}", { "lists": { "foo-list": { @@ -4819,14 +4831,25 @@ def test_wait_for_new_data(self) -> None: user2_id = self.register_user("u2", "pass") user2_tok = self.login(user2_id, "pass", "d2") - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + "?timeout=10000" + f"&pos={from_token}", { "lists": {}, "extensions": { @@ -4870,14 +4893,25 @@ def test_wait_for_new_data_timeout(self) -> None: user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + "?timeout=10000" + f"&pos={from_token}", { "lists": {}, "extensions": { @@ -5029,13 +5063,25 @@ def test_no_data_incremental_sync(self) -> None: user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "e2ee": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make an incremental Sliding Sync request with the e2ee extension enabled channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": {}, "extensions": { @@ -5097,14 +5143,25 @@ def test_wait_for_new_data(self) -> None: self.helper.join(room_id, user1_id, tok=user1_tok) self.helper.join(room_id, user3_id, tok=user3_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "e2ee": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + "?timeout=10000" + f"&pos={from_token}", { "lists": {}, "extensions": { @@ -5158,14 +5215,25 @@ def test_wait_for_new_data_timeout(self) -> None: user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "e2ee": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + "?timeout=10000" + f"&pos={from_token}", { "lists": {}, "extensions": { @@ -5243,7 +5311,20 @@ def test_device_lists(self) -> None: self.helper.join(room_id, user3_id, tok=user3_tok) self.helper.join(room_id, user4_id, tok=user4_tok) - from_token = self.event_sources.get_current_token() + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "e2ee": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + from_token = channel.json_body["pos"] # Have user3 update their device list channel = self.make_request( @@ -5262,8 +5343,7 @@ def test_device_lists(self) -> None: # Make an incremental Sliding Sync request with the e2ee extension enabled channel = self.make_request( "POST", - self.sync_endpoint - + f"?pos={self.get_success(from_token.to_string(self.store))}", + self.sync_endpoint + f"?pos={from_token}", { "lists": {}, "extensions": { From 60790d651289fa040e2692c0a0c193246207ad92 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 23 Jul 2024 17:44:44 +0100 Subject: [PATCH 27/47] Change token names again --- synapse/handlers/sliding_sync.py | 14 ++++++++------ synapse/types/__init__.py | 12 ++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 1173f1a1449..36665db8e18 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -394,7 +394,7 @@ async def wait_for_sync_for_user( # this returns false, it means we timed out waiting, and we should # just return an empty response. before_wait_ts = self.clock.time_msec() - if not await self.notifier.wait_for_stream_token(from_token.stream): + if not await self.notifier.wait_for_stream_token(from_token.stream_token): logger.warning( "Timed out waiting for worker to catch up. Returning empty response" ) @@ -432,7 +432,7 @@ async def current_sync_callback( sync_config.user.to_string(), timeout_ms, current_sync_callback, - from_token=from_token.stream, + from_token=from_token.stream_token, ) return result @@ -474,7 +474,7 @@ async def current_sync_for_user( await self.get_room_membership_for_user_at_to_token( user=sync_config.user, to_token=to_token, - from_token=from_token.stream if from_token else None, + from_token=from_token.stream_token if from_token else None, ) ) @@ -1435,7 +1435,7 @@ async def get_room_sync_data( # - TODO: For an incremental sync where we haven't sent it down this # connection before to_bound = ( - from_token.stream.room_key + from_token.stream_token.room_key if from_token is not None and not room_membership_for_user_at_to_token.newly_joined else None @@ -1502,7 +1502,9 @@ async def get_room_sync_data( instance_name=timeline_event.internal_metadata.instance_name, stream=timeline_event.internal_metadata.stream_ordering, ) - if persisted_position.persisted_after(from_token.stream.room_key): + if persisted_position.persisted_after( + from_token.stream_token.room_key + ): num_live += 1 else: # Since we're iterating over the timeline events in @@ -1926,7 +1928,7 @@ async def get_e2ee_extension_response( # TODO: This should take into account the `from_token` and `to_token` device_list_updates = await self.device_handler.get_user_ids_changed( user_id=user_id, - from_token=from_token.stream, + from_token=from_token.stream_token, ) device_one_time_keys_count: Mapping[str, int] = {} diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 2294bc22e00..a99f5d1a924 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1176,8 +1176,8 @@ class SlidingSyncStreamToken: per-connection state stored by Synapse. """ - stream: StreamToken - connection: int + stream_token: StreamToken + connection_position: int @staticmethod @cancellable @@ -1189,8 +1189,8 @@ async def from_string(store: "DataStore", string: str) -> "SlidingSyncStreamToke stream_token = await StreamToken.from_string(store, stream_token_str) return SlidingSyncStreamToken( - stream=stream_token, - connection=connection_token, + stream_token=stream_token, + connection_position=connection_token, ) except CancelledError: raise @@ -1199,8 +1199,8 @@ async def from_string(store: "DataStore", string: str) -> "SlidingSyncStreamToke async def to_string(self, store: "DataStore") -> str: """Serializes the token to a string""" - stream_token_str = await self.stream.to_string(store) - return f"{self.connection}/{stream_token_str}" + stream_token_str = await self.stream_token.to_string(store) + return f"{self.connection_position}/{stream_token_str}" @attr.s(slots=True, frozen=True, auto_attribs=True) From 4ce3e51f8db599301413b051b52a9b0cff9caf1c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 23 Jul 2024 18:28:15 +0100 Subject: [PATCH 28/47] Ensure there is only one SlidingSyncHandler --- synapse/server.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/server.py b/synapse/server.py index 4a3f9ff9349..46b9d83a044 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -559,6 +559,7 @@ def get_jwt_handler(self) -> "JwtHandler": def get_sync_handler(self) -> SyncHandler: return SyncHandler(self) + @cache_in_self def get_sliding_sync_handler(self) -> SlidingSyncHandler: return SlidingSyncHandler(self) From 602b6c83628febd5f4aea4526301c2f32f049566 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 23 Jul 2024 18:28:22 +0100 Subject: [PATCH 29/47] Add test for cache being cleared --- tests/rest/client/test_sync.py | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 80c7bfdca7d..00c491c764a 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -3569,6 +3569,77 @@ def test_rooms_required_state_incremental_sync(self) -> None: self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) + def test_rooms_required_state_incremental_sync_restart(self) -> None: + """ + Test `rooms.required_state` returns requested state events in the room during an + incremental sync, after a restart (and so the in memory caches are reset). + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 4, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + after_room_token = channel.json_body["pos"] + + # Reset the in-memory cache + self.hs.get_sliding_sync_handler().connection_store._connections.clear() + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint + f"?pos={after_room_token}", + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Tombstone, ""], + ], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # If the cache has been cleared then we do expect the state to come down + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + state_map[(EventTypes.RoomHistoryVisibility, "")], + }, + exact=True, + ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) + def test_rooms_required_state_wildcard(self) -> None: """ Test `rooms.required_state` returns all state events when using wildcard `["*", "*"]`. From 75f7c01b79b13311d1d0c2f11fcef44b928e8a7d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 24 Jul 2024 16:13:51 +0100 Subject: [PATCH 30/47] Only send rooms with updates down sliding sync --- synapse/handlers/sliding_sync.py | 29 +++++++++++++- synapse/types/handlers/__init__.py | 8 ++++ tests/rest/client/test_sync.py | 63 +++++++++++++++++++++++------- 3 files changed, 85 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 69be1131175..306c4286e75 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -617,6 +617,31 @@ async def current_sync_for_user( # Fetch room data rooms: Dict[str, SlidingSyncResult.RoomResult] = {} + # Filter out rooms that haven't received updates and we've sent down + # previously. + if from_token: + rooms_should_send = set() + for room_id in relevant_room_map: + status = await self.connection_store.have_sent_room( + sync_config, + from_token.connection_position, + room_id, + ) + if status.status != HaveSentRoomFlag.LIVE: + rooms_should_send.add(room_id) + + # We only need to check for any new events and not state changes, as + # state changes can only happen if an event has also been sent. + rooms_that_have_updates = ( + self.store._events_stream_cache.get_entities_changed( + relevant_room_map, from_token.stream_token.room_key.stream + ) + ) + rooms_should_send.update(rooms_that_have_updates) + relevant_room_map = { + r: c for r, c in relevant_room_map.items() if r in rooms_should_send + } + @trace @tag_args async def handle_room(room_id: str) -> None: @@ -631,7 +656,9 @@ async def handle_room(room_id: str) -> None: to_token=to_token, ) - rooms[room_id] = room_sync_result + # Filter out empty room results. + if room_sync_result: + rooms[room_id] = room_sync_result with start_active_span("sliding_sync.generate_room_entries"): await concurrently_execute(handle_room, relevant_room_map, 10) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 7c7fe130cb5..58530bba706 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -238,6 +238,14 @@ class StrippedHero: notification_count: int highlight_count: int + def __bool__(self) -> bool: + return ( + self.initial + or bool(self.required_state) + or bool(self.timeline_events) + or bool(self.stripped_state) + ) + @attr.s(slots=True, frozen=True, auto_attribs=True) class SlidingWindowList: """ diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index cbb673df2c3..7f3d5907cf5 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -3540,19 +3540,7 @@ def test_rooms_ban_incremental_sync2(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) # Nothing to see for this banned user in the room in the token range - self.assertIsNone(channel.json_body["rooms"][room_id1].get("timeline")) - # No events returned in the timeline so nothing is "live" - self.assertEqual( - channel.json_body["rooms"][room_id1]["num_live"], - 0, - channel.json_body["rooms"][room_id1], - ) - # There aren't anymore events to paginate to in this range - self.assertEqual( - channel.json_body["rooms"][room_id1]["limited"], - False, - channel.json_body["rooms"][room_id1], - ) + self.assertIsNone(channel.json_body["rooms"].get(room_id1)) def test_rooms_no_required_state(self) -> None: """ @@ -3662,12 +3650,15 @@ def test_rooms_required_state_incremental_sync(self) -> None: # This one doesn't exist in the room [EventTypes.Tombstone, ""], ], - "timeline_limit": 0, + "timeline_limit": 1, } } } _, after_room_token = self.do_sync(sync_body, tok=user1_tok) + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + # Make the Sliding Sync request channel = self.make_request( "POST", @@ -4745,6 +4736,50 @@ def test_incremental_sync_full_state_new_room(self) -> None: exact=True, ) + def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: + """ + Test that rooms with no updates are returned in subsequent incremental + syncs. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Tombstone, ""], + ], + "timeline_limit": 0, + } + } + } + + _, after_room_token = self.do_sync(sync_body, tok=user1_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint + f"?pos={after_room_token}", + content=sync_body, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Nothing has happened in the room, so the room should not come down + # /sync. + self.assertIsNone(channel.json_body["rooms"].get(room_id1)) + class SlidingSyncToDeviceExtensionTestCase(SlidingSyncBase): """Tests for the to-device sliding sync extension""" From b36cadacd92168bb8799d5e9169128ba2b3fde93 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 24 Jul 2024 16:16:22 +0100 Subject: [PATCH 31/47] Newsfile --- changelog.d/17479.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17479.misc diff --git a/changelog.d/17479.misc b/changelog.d/17479.misc new file mode 100644 index 00000000000..4502f71662f --- /dev/null +++ b/changelog.d/17479.misc @@ -0,0 +1 @@ +Do not send down empty room entries down experimental sliding sync endpoint. From 0812894e674c724e6e1bf0c841c90f56857d10e7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 25 Jul 2024 17:13:59 +0100 Subject: [PATCH 32/47] Update comments --- synapse/handlers/sliding_sync.py | 4 ++-- synapse/types/rest/client/__init__.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 69be1131175..57c8934752e 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -2207,8 +2207,8 @@ def _get_connection_key(sync_config: SlidingSyncConfig) -> Tuple[str, str]: user_id = sync_config.user.to_string() - # If this is missing, only one sliding sync connection is allowed per - # given conn_id. + # Only one sliding sync connection is allowed per given conn_id (empty + # or not). conn_id = sync_config.conn_id or "" if sync_config.requester.device_id: diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index 511e6e2d9f7..4fb30d05b46 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -120,8 +120,9 @@ class SlidingSyncBody(RequestBodyModel): Sliding Sync API request body. Attributes: - conn_id: An optional string to identify this connection to the server. If this - is missing, only one sliding sync connection is allowed per given conn_id. + conn_id: An optional string to identify this connection to the server. + Only one sliding sync connection is allowed per given conn_id (empty + or not). lists: Sliding window API. A map of list key to list information (:class:`SlidingSyncList`). Max lists: 100. The list keys should be arbitrary strings which the client is using to refer to the list. Keep this From 2c48784149aa49a60a9a46544cd0130c243b2d95 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 25 Jul 2024 17:24:56 +0100 Subject: [PATCH 33/47] Use do_sync in tests --- tests/rest/client/test_sync.py | 168 ++++++++++----------------------- 1 file changed, 50 insertions(+), 118 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 0bdf39d41a3..bce16a4eb3d 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -3695,47 +3695,27 @@ def test_rooms_required_state_incremental_sync_restart(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id1, user1_id, tok=user1_tok) - channel = self.make_request( - "POST", - self.sync_endpoint, - { - "lists": { - "foo-list": { - "ranges": [[0, 1]], - "required_state": [], - "timeline_limit": 4, - } + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Tombstone, ""], + ], + "timeline_limit": 1, } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - after_room_token = channel.json_body["pos"] + } + } + _, from_token = self.do_sync(sync_body, tok=user1_tok) # Reset the in-memory cache self.hs.get_sliding_sync_handler().connection_store._connections.clear() # Make the Sliding Sync request - channel = self.make_request( - "POST", - self.sync_endpoint + f"?pos={after_room_token}", - { - "lists": { - "foo-list": { - "ranges": [[0, 1]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - # This one doesn't exist in the room - [EventTypes.Tombstone, ""], - ], - "timeline_limit": 0, - } - } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) # If the cache has been cleared then we do expect the state to come down state_map = self.get_success( @@ -4507,35 +4487,29 @@ def test_incremental_sync_incremental_state(self) -> None: self.helper.join(room_id1, user1_id, tok=user1_tok) # Make the Sliding Sync request - channel = self.make_request( - "POST", - self.sync_endpoint, - { - "lists": { - "foo-list": { - "ranges": [[0, 1]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - # This one doesn't exist in the room - [EventTypes.Name, ""], - ], - "timeline_limit": 0, - } + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": 0, } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) + } + } - from_token = channel.json_body["pos"] + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) state_map = self.get_success( self.storage_controllers.state.get_current_state(room_id1) ) self._assertRequiredStateIncludes( - channel.json_body["rooms"][room_id1]["required_state"], + response_body["rooms"][room_id1]["required_state"], { state_map[(EventTypes.Create, "")], state_map[(EventTypes.RoomHistoryVisibility, "")], @@ -4548,32 +4522,14 @@ def test_incremental_sync_incremental_state(self) -> None: room_id1, EventTypes.Name, body={"name": "foo"}, tok=user2_tok ) - channel = self.make_request( - "POST", - self.sync_endpoint + f"?pos={from_token}", - { - "lists": { - "foo-list": { - "ranges": [[0, 1]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - [EventTypes.Name, ""], - ], - "timeline_limit": 0, - } - } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) state_map = self.get_success( self.storage_controllers.state.get_current_state(room_id1) ) self._assertRequiredStateIncludes( - channel.json_body["rooms"][room_id1]["required_state"], + response_body["rooms"][room_id1]["required_state"], { state_map[(EventTypes.Name, "")], }, @@ -4597,42 +4553,36 @@ def test_incremental_sync_full_state_new_room(self) -> None: self.helper.join(room_id2, user1_id, tok=user1_tok) # Make the Sliding Sync request, we'll only receive room_id2 - channel = self.make_request( - "POST", - self.sync_endpoint, - { - "lists": { - "foo-list": { - "ranges": [[0, 0]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - # This one doesn't exist in the room - [EventTypes.Name, ""], - ], - "timeline_limit": 0, - } + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": 0, } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) + } + } - from_token = channel.json_body["pos"] + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) state_map = self.get_success( self.storage_controllers.state.get_current_state(room_id2) ) self._assertRequiredStateIncludes( - channel.json_body["rooms"][room_id2]["required_state"], + response_body["rooms"][room_id2]["required_state"], { state_map[(EventTypes.Create, "")], state_map[(EventTypes.RoomHistoryVisibility, "")], }, exact=True, ) - self.assertNotIn(room_id1, channel.json_body["rooms"]) + self.assertNotIn(room_id1, response_body["rooms"]) # Send a state event in room 1 self.helper.send_state( @@ -4640,32 +4590,14 @@ def test_incremental_sync_full_state_new_room(self) -> None: ) # We should get room_id1 down sync, with the full state. - channel = self.make_request( - "POST", - self.sync_endpoint + f"?pos={from_token}", - { - "lists": { - "foo-list": { - "ranges": [[0, 0]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - [EventTypes.Name, ""], - ], - "timeline_limit": 0, - } - } - }, - access_token=user1_tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) state_map = self.get_success( self.storage_controllers.state.get_current_state(room_id1) ) self._assertRequiredStateIncludes( - channel.json_body["rooms"][room_id1]["required_state"], + response_body["rooms"][room_id1]["required_state"], { state_map[(EventTypes.Create, "")], state_map[(EventTypes.RoomHistoryVisibility, "")], From abd7a5b600c2819e1a87a7aee95f5785ac101e35 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 25 Jul 2024 18:23:33 +0100 Subject: [PATCH 34/47] Move to integration tests --- tests/handlers/test_sliding_sync.py | 397 +--------------------------- tests/rest/client/test_sync.py | 114 ++++++++ 2 files changed, 116 insertions(+), 395 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 5e91be00265..a7aa9bb8afe 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -19,7 +19,7 @@ # import logging from copy import deepcopy -from typing import Dict, Optional, Tuple +from typing import Dict, Optional from unittest.mock import patch from parameterized import parameterized @@ -46,13 +46,7 @@ from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import ( - JsonDict, - SlidingSyncStreamToken, - StreamToken, - UserID, - create_requester, -) +from synapse.types import JsonDict, StreamToken, UserID from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock @@ -3765,390 +3759,3 @@ def test_default_bump_event_types(self) -> None: # We only care about the *latest* event in the room. [room_id1, room_id2], ) - - -class GetRoomSyncDataTestCase(HomeserverTestCase): - """ - Tests for Sliding Sync handler `get_room_sync_data()` - """ - - servlets = [ - admin.register_servlets, - knock.register_servlets, - login.register_servlets, - room.register_servlets, - ] - - def default_config(self) -> JsonDict: - config = super().default_config() - # Enable sliding sync - config["experimental_features"] = {"msc3575_enabled": True} - return config - - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - self.sliding_sync_handler = self.hs.get_sliding_sync_handler() - self.store = self.hs.get_datastores().main - self.event_sources = hs.get_event_sources() - - def _create_sync_configs( - self, - user_id: str, - room_id: str, - timeline_limit: int = 5, - ) -> Tuple[SlidingSyncConfig, RoomSyncConfig, _RoomMembershipForUser]: - """Create the configs necessary to call `get_room_sync_data`""" - requester = create_requester(user_id, device_id="foo_device") - sync_config = SlidingSyncConfig( - user=requester.user, - requester=requester, - conn_id="conn_id", - lists={ - "list": SlidingSyncConfig.SlidingSyncList( - timeline_limit=timeline_limit, - required_state=[ - (EventTypes.Name, ""), - ], - ), - }, - room_subscriptions={}, - extensions=None, - ) - - room_sync_config = RoomSyncConfig(timeline_limit, {EventTypes.Name: {""}}) - - rooms = self.get_success( - self.store.get_rooms_for_local_user_where_membership_is( - user_id, membership_list=[Membership.JOIN] - ) - ) - room_for_user = rooms[0] - assert room_for_user.room_id == room_id - - room_membership_for_user_at_to_token = _RoomMembershipForUser( - room_id=room_id, - event_id=room_for_user.event_id, - event_pos=room_for_user.event_pos, - membership=Membership.JOIN, - sender=user_id, - newly_joined=False, - newly_left=False, - is_dm=False, - ) - - return (sync_config, room_sync_config, room_membership_for_user_at_to_token) - - def test_room_sync_data_initial(self) -> None: - """Tests getting room sync data with no from token""" - - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - - room_id1 = self.helper.create_room_as( - user1_id, - tok=user1_tok, - ) - - sync_config, room_sync_config, room_membership_for_user_at_to_token = ( - self._create_sync_configs(user1_id, room_id1, timeline_limit=5) - ) - - # Timeline limit is 5 so let's send 5 messages that we'll expect to get - # back. - expected_timeline = [] - for _ in range(5): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - to_token = self.event_sources.get_current_token() - - result = self.get_success( - self.sliding_sync_handler.get_room_sync_data( - sync_config, - room_id=room_id1, - room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, - from_token=None, - to_token=to_token, - ) - ) - - self.assertTrue(result.initial) - self.assertTrue(result.limited) - self.assertEqual( - [e.event_id for e in result.timeline_events], expected_timeline - ) - - def test_room_sync_data_incremental_live(self) -> None: - """Test getting room data where we have previously sent down the room - and its state is considered LIVE""" - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - - room_id1 = self.helper.create_room_as( - user1_id, - tok=user1_tok, - ) - - sync_config, room_sync_config, room_membership_for_user_at_to_token = ( - self._create_sync_configs(user1_id, room_id1) - ) - - # These messages are sent before the `from_token`, so we don't expect to - # see these messages. - for _ in range(5): - r = self.helper.send(room_id1, "message", tok=user1_tok) - - # Mark the room as having been sent down, and create an appropriate - # `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] - ) - ) - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # These messages are sent after the `from_token`, so we expect to only - # see these messages. - expected_timeline = [] - for _ in range(2): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - to_token = self.event_sources.get_current_token() - - result = self.get_success( - self.sliding_sync_handler.get_room_sync_data( - sync_config, - room_id=room_id1, - room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, - from_token=from_token, - to_token=to_token, - ) - ) - - self.assertFalse(result.initial) - self.assertFalse(result.limited) - self.assertEqual( - [e.event_id for e in result.timeline_events], expected_timeline - ) - - def test_room_sync_data_incremental_previously_not_limited(self) -> None: - """Test getting room data where we have previously sent down the room, - but we missed sending down some data previously and so its state is - considered PREVIOUSLY. - - In this case there has been more than timeline limit events. - """ - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - - room_id1 = self.helper.create_room_as( - user1_id, - tok=user1_tok, - ) - - sync_config, room_sync_config, room_membership_for_user_at_to_token = ( - self._create_sync_configs(user1_id, room_id1) - ) - - # These messages are sent before the initial `from_token`, so we don't - # expect to see these messages. - for _ in range(5): - r = self.helper.send(room_id1, "message", tok=user1_tok) - - # Mark the room as having been sent down, and create an appropriate - # `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] - ) - ) - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # These messages are sent after the initial `from_token`, so we expect - # to see these messages. - expected_timeline = [] - for _ in range(2): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - # Mark the room as *not* having been sent down, and create a new - # `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, from_token, sent_room_ids=[], unsent_room_ids=[room_id1] - ) - ) - - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # We should also receive new messages - for _ in range(2): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - to_token = self.event_sources.get_current_token() - - result = self.get_success( - self.sliding_sync_handler.get_room_sync_data( - sync_config, - room_id=room_id1, - room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, - from_token=from_token, - to_token=to_token, - ) - ) - - self.assertFalse(result.initial) - self.assertFalse(result.limited) - self.assertEqual( - [e.event_id for e in result.timeline_events], expected_timeline - ) - - def test_room_sync_data_incremental_previously_limited(self) -> None: - """Test getting room data where we have previously sent down the room, - but we missed sending down some data previously and so its state is - considered PREVIOUSLY. - - In this case there has been fewer than timeline limit events. - """ - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - - room_id1 = self.helper.create_room_as( - user1_id, - tok=user1_tok, - ) - - sync_config, room_sync_config, room_membership_for_user_at_to_token = ( - self._create_sync_configs(user1_id, room_id1) - ) - - # These messages are sent before the initial `from_token`, so we don't - # expect to see these messages. - for _ in range(5): - r = self.helper.send(room_id1, "message", tok=user1_tok) - - # Mark the room as having been sent down, and create an appropriate - # `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, None, sent_room_ids=[room_id1], unsent_room_ids=[] - ) - ) - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # These messages are sent after the initial `from_token`, but are before - # the timeline limit, so we don't expect to see these messages. - for _ in range(5): - r = self.helper.send(room_id1, "message", tok=user1_tok) - - # ... but these messages are within the timeline limit, so we do expect - # to see them - expected_timeline = [] - for _ in range(3): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - # Mark the room as *not* having been sent down, and create a new - # `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, from_token, sent_room_ids=[], unsent_room_ids=[room_id1] - ) - ) - - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # We should also receive new messages - for _ in range(2): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - to_token = self.event_sources.get_current_token() - - result = self.get_success( - self.sliding_sync_handler.get_room_sync_data( - sync_config, - room_id=room_id1, - room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, - from_token=from_token, - to_token=to_token, - ) - ) - - self.assertFalse(result.initial) - self.assertTrue(result.limited) - self.assertEqual( - [e.event_id for e in result.timeline_events], expected_timeline - ) - - def test_room_sync_data_incremental_never(self) -> None: - """Test getting room data where we have not previously sent down the room, - so its state is considered NEVER. - """ - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - - room_id1 = self.helper.create_room_as( - user1_id, - tok=user1_tok, - ) - - sync_config, room_sync_config, room_membership_for_user_at_to_token = ( - self._create_sync_configs(user1_id, room_id1) - ) - - # We expect to see these messages even though they're before the - # `from_token`, as the room has not been sent down. - expected_timeline = [] - for _ in range(2): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - # Create a new `from_token`. - connection_token = self.get_success( - self.sliding_sync_handler.connection_store.record_rooms( - sync_config, None, sent_room_ids=[], unsent_room_ids=[] - ) - ) - from_token = SlidingSyncStreamToken( - self.event_sources.get_current_token(), connection_token - ) - - # We should also receive new messages - for _ in range(3): - r = self.helper.send(room_id1, "message", tok=user1_tok) - expected_timeline.append(r["event_id"]) - - to_token = self.event_sources.get_current_token() - - result = self.get_success( - self.sliding_sync_handler.get_room_sync_data( - sync_config, - room_id=room_id1, - room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, - from_token=from_token, - to_token=to_token, - ) - ) - - self.assertTrue(result.initial) - self.assertTrue(result.limited) - self.assertEqual( - [e.event_id for e in result.timeline_events], expected_timeline - ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index bce16a4eb3d..05a9da11011 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -59,6 +59,7 @@ StreamToken, UserID, ) +from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock from synapse.util.stringutils import random_string @@ -4606,6 +4607,119 @@ def test_incremental_sync_full_state_new_room(self) -> None: exact=True, ) + @parameterized.expand([(False,), (True,)]) + def test_incremental_sync_full_state_previously(self, limited: bool) -> None: + """ + Test getting room data where we have previously sent down the room, but + we missed sending down some data previously and so its state is + considered PREVIOUSLY. + + There are two versions of this test, one where there are more messages + than the timeline limit, and one where there isn't. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) + room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) + + self.helper.send(room_id1, "msg", tok=user1_tok) + + timeline_limit = 5 + conn_id = "conn_id" + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": timeline_limit, + } + }, + "conn_id": "conn_id", + } + + # The first room gets sent down the initial sync + _, initial_from_token = self.do_sync(sync_body, tok=user1_tok) + + # We now send down some events in room1 (depending on the test param). + expected_events = [] # The set of events in the timeline + if limited: + for _ in range(10): + resp = self.helper.send(room_id1, "msg1", tok=user1_tok) + expected_events.append(resp["event_id"]) + else: + resp = self.helper.send(room_id1, "msg1", tok=user1_tok) + expected_events.append(resp["event_id"]) + + # A second messages happens in the other room, so room1 won't get sent down. + self.helper.send(room_id2, "msg", tok=user1_tok) + + # Only the second room gets sent down sync. + _, from_token = self.do_sync(sync_body, since=initial_from_token, tok=user1_tok) + + # FIXME: This is a hack to record that the first room wasn't sent down + # sync, as we don't implement that currently. + sliding_sync_handler = self.hs.get_sliding_sync_handler() + requester = self.get_success( + self.hs.get_auth().get_user_by_access_token(user1_tok) + ) + sync_config = SlidingSyncConfig( + user=requester.user, + requester=requester, + conn_id=conn_id, + lists={ + "list": SlidingSyncConfig.SlidingSyncList( + timeline_limit=1, + required_state=[ + (EventTypes.Name, ""), + ], + ), + }, + room_subscriptions={}, + extensions=None, + ) + + parsed_initial_from_token = self.get_success( + SlidingSyncStreamToken.from_string(self.store, initial_from_token) + ) + connection_position = self.get_success( + sliding_sync_handler.connection_store.record_rooms( + sync_config, + parsed_initial_from_token, + sent_room_ids=[], + unsent_room_ids=[room_id1], + ) + ) + + # FIXME: Now fix up `from_token` with new connect position above. + parsed_from_token = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ) + parsed_from_token = SlidingSyncStreamToken( + stream_token=parsed_from_token.stream_token, + connection_position=connection_position, + ) + from_token = self.get_success(parsed_from_token.to_string(self.store)) + + # We now send another event to room1, so we should sync all the missing events. + resp = self.helper.send(room_id1, "msg2", tok=user1_tok) + expected_events.append(resp["event_id"]) + + # This sync should contain the messages from room1 not yet sent down. + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + + self.assertEqual( + [ev["event_id"] for ev in response_body["rooms"][room_id1]["timeline"]], + expected_events[-timeline_limit:], + ) + self.assertEqual(response_body["rooms"][room_id1]["limited"], limited) + class SlidingSyncToDeviceExtensionTestCase(SlidingSyncBase): """Tests for the to-device sliding sync extension""" From 470c5d34d10d3794617ed3efb05fc4910319dcfa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 10:28:04 +0100 Subject: [PATCH 35/47] Review comments --- synapse/handlers/sliding_sync.py | 3 +++ tests/rest/client/test_sync.py | 33 +++++++++++++++++--------------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 4e8d6e02d45..e714662a02d 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1750,6 +1750,9 @@ async def get_room_sync_data( else: assert from_bound is not None + # TODO: Limit the number of state events we're about to send down + # the room, if its too many we should change this to an + # `initial=True`? deltas = await self.store.get_current_state_deltas_for_room( room_id=room_id, from_token=from_bound, diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 05a9da11011..e1199a33366 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4476,7 +4476,7 @@ def test_room_subscriptions_world_readable(self) -> None: def test_incremental_sync_incremental_state(self) -> None: """Test that we only get state updates in incremental sync for rooms - we've already seen. + we've already seen (LIVE). """ user1_id = self.register_user("user1", "pass") @@ -4611,8 +4611,8 @@ def test_incremental_sync_full_state_new_room(self) -> None: def test_incremental_sync_full_state_previously(self, limited: bool) -> None: """ Test getting room data where we have previously sent down the room, but - we missed sending down some data previously and so its state is - considered PREVIOUSLY. + we missed sending down some timeline events previously and so its status + is considered PREVIOUSLY. There are two versions of this test, one where there are more messages than the timeline limit, and one where there isn't. @@ -4645,7 +4645,10 @@ def test_incremental_sync_full_state_previously(self, limited: bool) -> None: } # The first room gets sent down the initial sync - _, initial_from_token = self.do_sync(sync_body, tok=user1_tok) + response_body, initial_from_token = self.do_sync(sync_body, tok=user1_tok) + self.assertCountEqual( + response_body["rooms"].keys(), {room_id1}, response_body["rooms"] + ) # We now send down some events in room1 (depending on the test param). expected_events = [] # The set of events in the timeline @@ -4661,7 +4664,13 @@ def test_incremental_sync_full_state_previously(self, limited: bool) -> None: self.helper.send(room_id2, "msg", tok=user1_tok) # Only the second room gets sent down sync. - _, from_token = self.do_sync(sync_body, since=initial_from_token, tok=user1_tok) + response_body, from_token = self.do_sync( + sync_body, since=initial_from_token, tok=user1_tok + ) + + self.assertCountEqual( + response_body["rooms"].keys(), {room_id2}, response_body["rooms"] + ) # FIXME: This is a hack to record that the first room wasn't sent down # sync, as we don't implement that currently. @@ -4673,16 +4682,6 @@ def test_incremental_sync_full_state_previously(self, limited: bool) -> None: user=requester.user, requester=requester, conn_id=conn_id, - lists={ - "list": SlidingSyncConfig.SlidingSyncList( - timeline_limit=1, - required_state=[ - (EventTypes.Name, ""), - ], - ), - }, - room_subscriptions={}, - extensions=None, ) parsed_initial_from_token = self.get_success( @@ -4714,6 +4713,10 @@ def test_incremental_sync_full_state_previously(self, limited: bool) -> None: # This sync should contain the messages from room1 not yet sent down. response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + self.assertCountEqual( + response_body["rooms"].keys(), {room_id1}, response_body["rooms"] + ) + self.assertEqual( [ev["event_id"] for ev in response_body["rooms"][room_id1]["timeline"]], expected_events[-timeline_limit:], From 90f2184833c9dcf31649bd54d0349e68d4769d8f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 10:28:11 +0100 Subject: [PATCH 36/47] Previously state test --- tests/rest/client/test_sync.py | 111 +++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index e1199a33366..bd9a687ce94 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4722,6 +4722,117 @@ def test_incremental_sync_full_state_previously(self, limited: bool) -> None: expected_events[-timeline_limit:], ) self.assertEqual(response_body["rooms"][room_id1]["limited"], limited) + self.assertEqual(response_body["rooms"][room_id1].get("required_state"), None) + + def test_incremental_sync_full_state_previously_state(self) -> None: + """ + Test getting room data where we have previously sent down the room, but + we missed sending down some state previously and so its status is + considered PREVIOUSLY. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) + room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) + + self.helper.send(room_id1, "msg", tok=user1_tok) + + timeline_limit = 5 + conn_id = "conn_id" + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": timeline_limit, + } + }, + "conn_id": "conn_id", + } + + # The first room gets sent down the initial sync + response_body, initial_from_token = self.do_sync(sync_body, tok=user1_tok) + self.assertCountEqual( + response_body["rooms"].keys(), {room_id1}, response_body["rooms"] + ) + + # We now send down some state in room1 (depending on the test param). + resp = self.helper.send_state( + room_id1, EventTypes.Name, {"name": "foo"}, tok=user1_tok + ) + name_change_id = resp["event_id"] + + # A second messages happens in the other room, so room1 won't get sent down. + self.helper.send(room_id2, "msg", tok=user1_tok) + + # Only the second room gets sent down sync. + response_body, from_token = self.do_sync( + sync_body, since=initial_from_token, tok=user1_tok + ) + + self.assertCountEqual( + response_body["rooms"].keys(), {room_id2}, response_body["rooms"] + ) + + # FIXME: This is a hack to record that the first room wasn't sent down + # sync, as we don't implement that currently. + sliding_sync_handler = self.hs.get_sliding_sync_handler() + requester = self.get_success( + self.hs.get_auth().get_user_by_access_token(user1_tok) + ) + sync_config = SlidingSyncConfig( + user=requester.user, + requester=requester, + conn_id=conn_id, + ) + + parsed_initial_from_token = self.get_success( + SlidingSyncStreamToken.from_string(self.store, initial_from_token) + ) + connection_position = self.get_success( + sliding_sync_handler.connection_store.record_rooms( + sync_config, + parsed_initial_from_token, + sent_room_ids=[], + unsent_room_ids=[room_id1], + ) + ) + + # FIXME: Now fix up `from_token` with new connect position above. + parsed_from_token = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ) + parsed_from_token = SlidingSyncStreamToken( + stream_token=parsed_from_token.stream_token, + connection_position=connection_position, + ) + from_token = self.get_success(parsed_from_token.to_string(self.store)) + + # We now send another event to room1, so we should sync all the missing state. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # This sync should contain the state changes from room1. + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + + self.assertCountEqual( + response_body["rooms"].keys(), {room_id1}, response_body["rooms"] + ) + + # We should only see the name change. + self.assertEqual( + [ + ev["event_id"] + for ev in response_body["rooms"][room_id1]["required_state"] + ], + [name_change_id], + ) class SlidingSyncToDeviceExtensionTestCase(SlidingSyncBase): From e7a6c1968899db4c4165098131ddeeec015af508 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 10:32:34 +0100 Subject: [PATCH 37/47] Add never test --- tests/rest/client/test_sync.py | 70 ++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index bd9a687ce94..68283dc31ed 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4834,6 +4834,76 @@ def test_incremental_sync_full_state_previously_state(self) -> None: [name_change_id], ) + def test_incremental_sync_full_state_never(self) -> None: + """ + Test getting room data where we have not previously sent down the room + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) + room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) + + self.helper.send(room_id1, "msg", tok=user1_tok) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.RoomHistoryVisibility, ""], + # This one doesn't exist in the room + [EventTypes.Name, ""], + ], + "timeline_limit": 1, + } + }, + } + + # A message happens in the other room, so room1 won't get sent down. + self.helper.send(room_id2, "msg", tok=user1_tok) + + # Only the second room gets sent down sync. + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + self.assertCountEqual( + response_body["rooms"].keys(), {room_id2}, response_body["rooms"] + ) + + # We now send another event to room1, so we should send down the full + # room. + resp = self.helper.send(room_id1, "msg2", tok=user1_tok) + latest_message_event = resp["event_id"] + + # This sync should contain the messages from room1 not yet sent down. + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + + self.assertCountEqual( + response_body["rooms"].keys(), {room_id1}, response_body["rooms"] + ) + + self.assertEqual( + [ev["event_id"] for ev in response_body["rooms"][room_id1]["timeline"]], + [latest_message_event], + ) + self.assertEqual(response_body["rooms"][room_id1]["limited"], True) + self.assertEqual(response_body["rooms"][room_id1]["initial"], True) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + state_map[(EventTypes.RoomHistoryVisibility, "")], + }, + exact=True, + ) + class SlidingSyncToDeviceExtensionTestCase(SlidingSyncBase): """Tests for the to-device sliding sync extension""" From b9facafea0c5a8666c7ceac3b5b57927f2670b99 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 10:50:26 +0100 Subject: [PATCH 38/47] Review comments --- synapse/handlers/sliding_sync.py | 4 +++- tests/rest/client/test_sync.py | 7 +------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 306c4286e75..b4ecf87f2ef 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -639,7 +639,9 @@ async def current_sync_for_user( ) rooms_should_send.update(rooms_that_have_updates) relevant_room_map = { - r: c for r, c in relevant_room_map.items() if r in rooms_should_send + room_id: room_sync_config + for room_id, room_sync_config in relevant_room_map.items() + if room_id in rooms_should_send } @trace diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 7f3d5907cf5..fc170ebd4e6 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4754,12 +4754,7 @@ def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: "lists": { "foo-list": { "ranges": [[0, 1]], - "required_state": [ - [EventTypes.Create, ""], - [EventTypes.RoomHistoryVisibility, ""], - # This one doesn't exist in the room - [EventTypes.Tombstone, ""], - ], + "required_state": [], "timeline_limit": 0, } } From b396450006c0cc70893f76c80ec31ae160e35ff3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 11:05:58 +0100 Subject: [PATCH 39/47] Add test that empty room comes down initial sync --- tests/rest/client/test_sync.py | 40 ++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index fc170ebd4e6..d649ae636b3 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4744,11 +4744,8 @@ def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - user2_id = self.register_user("user2", "pass") - user2_tok = self.login(user2_id, "pass") - room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) - self.helper.join(room_id1, user1_id, tok=user1_tok) + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) sync_body = { "lists": { @@ -4763,17 +4760,38 @@ def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: _, after_room_token = self.do_sync(sync_body, tok=user1_tok) # Make the Sliding Sync request - channel = self.make_request( - "POST", - self.sync_endpoint + f"?pos={after_room_token}", - content=sync_body, - access_token=user1_tok, + response_body, _ = self.do_sync( + sync_body, since=after_room_token, tok=user1_tok ) - self.assertEqual(channel.code, 200, channel.json_body) # Nothing has happened in the room, so the room should not come down # /sync. - self.assertIsNone(channel.json_body["rooms"].get(room_id1)) + self.assertIsNone(response_body["rooms"].get(room_id1)) + + def test_empty_room_comes_down_sync(self) -> None: + """ + Test that rooms come down /sync even with empty required state and + timeline limit in initial sync. + """ + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + } + + # Make the Sliding Sync request + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertEqual(response_body["rooms"][room_id1]["initial"], True) class SlidingSyncToDeviceExtensionTestCase(SlidingSyncBase): From c56a745f23df17cca2f87f1610059da763c4ea16 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 11:07:07 +0100 Subject: [PATCH 40/47] Make it clear we only filter out rooms in incremental sync --- synapse/handlers/sliding_sync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b4ecf87f2ef..07545e94963 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -658,8 +658,8 @@ async def handle_room(room_id: str) -> None: to_token=to_token, ) - # Filter out empty room results. - if room_sync_result: + # Filter out empty room results during incremental sync + if room_sync_result or not from_token: rooms[room_id] = room_sync_result with start_active_span("sliding_sync.generate_room_entries"): From c53e83ded57af3d3e35f7036113f1352a7d5d134 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 11:15:14 +0100 Subject: [PATCH 41/47] Don't send down rooms if nothing has happened --- synapse/types/handlers/__init__.py | 2 +- tests/rest/client/test_sync.py | 40 +++++++++++------------------- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 58530bba706..0f611e3fef7 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -355,7 +355,7 @@ def __bool__(self) -> bool: to tell if the notifier needs to wait for more events when polling for events. """ - return bool(self.lists or self.rooms or self.extensions) + return bool(self.rooms or self.extensions) @staticmethod def empty(next_pos: SlidingSyncStreamToken) -> "SlidingSyncResult": diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index d649ae636b3..bd78f0fddb2 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -66,7 +66,6 @@ ) from tests.server import FakeChannel, TimedOutException from tests.test_utils.event_injection import mark_event_as_partial_state -from tests.unittest import skip_unless logger = logging.getLogger(__name__) @@ -1625,12 +1624,6 @@ def test_wait_for_new_data(self) -> None: channel.json_body["rooms"][room_id]["timeline"], ) - # TODO: Once we remove `ops`, we should be able to add a `RoomResult.__bool__` to - # check if there are any updates since the `from_token`. - @skip_unless( - False, - "Once we remove ops from the Sliding Sync response, this test should pass", - ) def test_wait_for_new_data_timeout(self) -> None: """ Test to make sure that the Sliding Sync request waits for new data to arrive but @@ -1645,23 +1638,22 @@ def test_wait_for_new_data_timeout(self) -> None: room_id = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id, user1_id, tok=user1_tok) - from_token = self.event_sources.get_current_token() + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 0]], + "required_state": [], + "timeline_limit": 1, + } + } + } + _, from_token = self.do_sync(sync_body, tok=user1_tok) # Make the Sliding Sync request channel = self.make_request( "POST", - self.sync_endpoint - + "?timeout=10000" - + f"&pos={self.get_success(from_token.to_string(self.store))}", - { - "lists": { - "foo-list": { - "ranges": [[0, 0]], - "required_state": [], - "timeline_limit": 1, - } - } - }, + self.sync_endpoint + f"?timeout=10000&pos={from_token}", + content=sync_body, access_token=user1_tok, await_result=False, ) @@ -1679,12 +1671,8 @@ def test_wait_for_new_data_timeout(self) -> None: channel.await_result(timeout_ms=1200) self.assertEqual(channel.code, 200, channel.json_body) - # We still see rooms because that's how Sliding Sync lists work but we reached - # the timeout before seeing them - self.assertEqual( - [event["event_id"] for event in channel.json_body["rooms"].keys()], - [room_id], - ) + # There should be no room sent down. + self.assertFalse(channel.json_body["rooms"]) def test_filter_list(self) -> None: """ From fd6167248c193f9a8832aa346572881a6abe99e3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 26 Jul 2024 13:32:30 +0100 Subject: [PATCH 42/47] Add dedicated function 'get_rooms_that_might_have_updates' --- synapse/handlers/sliding_sync.py | 6 ++---- synapse/storage/databases/main/stream.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index c0e5192f00d..6b01dd048c1 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -634,10 +634,8 @@ async def current_sync_for_user( # We only need to check for any new events and not state changes, as # state changes can only happen if an event has also been sent. - rooms_that_have_updates = ( - self.store._events_stream_cache.get_entities_changed( - relevant_room_map, from_token.stream_token.room_key.stream - ) + rooms_that_have_updates = self.store.get_rooms_that_might_have_updates( + relevant_room_map.keys(), from_token.stream_token.room_key ) rooms_should_send.update(rooms_that_have_updates) relevant_room_map = { diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index b034361aec6..4207e73c7f9 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -2104,3 +2104,13 @@ async def get_timeline_gaps( return RoomStreamToken(stream=last_position.stream - 1) return None + + def get_rooms_that_might_have_updates( + self, room_ids: StrCollection, from_token: RoomStreamToken + ) -> StrCollection: + """Filters given room IDs down to those that might have updates, i.e. + removes rooms that definitely do not have updates. + """ + return self._events_stream_cache.get_entities_changed( + room_ids, from_token.stream + ) From 7ae5f78ae154a3fba0b7850c9b9bec8577e22eb9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 29 Jul 2024 20:05:32 +0100 Subject: [PATCH 43/47] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync.py | 4 ++-- synapse/types/handlers/__init__.py | 3 +++ tests/rest/client/test_sync.py | 8 ++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 6b01dd048c1..b024402bbf1 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -632,8 +632,8 @@ async def current_sync_for_user( if status.status != HaveSentRoomFlag.LIVE: rooms_should_send.add(room_id) - # We only need to check for any new events and not state changes, as - # state changes can only happen if an event has also been sent. + # We only need to check for new events since any state changes + # will also come down as new events. rooms_that_have_updates = self.store.get_rooms_that_might_have_updates( relevant_room_map.keys(), from_token.stream_token.room_key ) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index e5c38a191dd..3cb21a79c36 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -240,7 +240,10 @@ class StrippedHero: def __bool__(self) -> bool: return ( + // If this is the first time the client is seeing the room, we should not filter it out + // under any circumstance. self.initial + // We need to let the client know if there are any new events or bool(self.required_state) or bool(self.timeline_events) or bool(self.stripped_state) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index d695ac4ebac..c820b2bba27 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -4905,18 +4905,18 @@ def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: } } - _, after_room_token = self.do_sync(sync_body, tok=user1_tok) + _, from_token = self.do_sync(sync_body, tok=user1_tok) - # Make the Sliding Sync request + # Make the incremental Sliding Sync request response_body, _ = self.do_sync( - sync_body, since=after_room_token, tok=user1_tok + sync_body, since=from_token, tok=user1_tok ) # Nothing has happened in the room, so the room should not come down # /sync. self.assertIsNone(response_body["rooms"].get(room_id1)) - def test_empty_room_comes_down_sync(self) -> None: + def test_empty_initial_room_comes_down_sync(self) -> None: """ Test that rooms come down /sync even with empty required state and timeline limit in initial sync. From 76c69e6fabace300a8e09b1c89354f73ac7dc809 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 29 Jul 2024 20:09:24 +0100 Subject: [PATCH 44/47] Add clarifying comments --- synapse/handlers/sliding_sync.py | 5 +++++ synapse/types/handlers/__init__.py | 3 +++ 2 files changed, 8 insertions(+) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b024402bbf1..7a684d0c3c1 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -623,6 +623,11 @@ async def current_sync_for_user( # previously. if from_token: rooms_should_send = set() + + # First we check if there are rooms that match a list/room + # subscription and have updates we need to send (i.e. either because + # we haven't sent the room down, or we have but there are missing + # updates). for room_id in relevant_room_map: status = await self.connection_store.have_sent_room( sync_config, diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 3cb21a79c36..a3dc7951078 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -378,6 +378,9 @@ def __bool__(self) -> bool: to tell if the notifier needs to wait for more events when polling for events. """ + # We don't include list changes here, as a) `lists` is always non-empty + # even if there are no changes, and b) any changes that happen should + # result in an entry added to `rooms`. return bool(self.rooms or self.extensions) @staticmethod From 4646edba770367728686ce59e7cb8923a5b4ac32 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 29 Jul 2024 22:52:56 +0100 Subject: [PATCH 45/47] Fix comment --- synapse/types/handlers/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index a3dc7951078..c8d44ec5df4 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -240,10 +240,10 @@ class StrippedHero: def __bool__(self) -> bool: return ( - // If this is the first time the client is seeing the room, we should not filter it out - // under any circumstance. + # If this is the first time the client is seeing the room, we should not filter it out + # under any circumstance. self.initial - // We need to let the client know if there are any new events + # We need to let the client know if there are any new events or bool(self.required_state) or bool(self.timeline_events) or bool(self.stripped_state) From 7088cb0a8633b4a5e7ddaa50c6acb31ca11db411 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 29 Jul 2024 22:53:26 +0100 Subject: [PATCH 46/47] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/types/handlers/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index c8d44ec5df4..f26cc0e903d 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -378,9 +378,10 @@ def __bool__(self) -> bool: to tell if the notifier needs to wait for more events when polling for events. """ - # We don't include list changes here, as a) `lists` is always non-empty - # even if there are no changes, and b) any changes that happen should - # result in an entry added to `rooms`. + # We don't include `self.lists` here, as a) `lists` is always non-empty even if + # there are no changes, and b) since we're sorting rooms by `stream_ordering` of + # the latest activity, anything that would cause the order to change would end + # up in `self.rooms` and cause us to send down the change. return bool(self.rooms or self.extensions) @staticmethod From 07c05618bd9b5bb0593862280008e11091ca42fa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 29 Jul 2024 22:57:56 +0100 Subject: [PATCH 47/47] Change comment and andd assertion --- synapse/handlers/sliding_sync.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 2699e61aba1..84f2fa18ff5 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -634,8 +634,23 @@ async def current_sync_for_user( from_token.connection_position, room_id, ) - if status.status != HaveSentRoomFlag.LIVE: + if ( + # The room was never sent down before so the client needs to know + # about it regardless of any updates. + status.status == HaveSentRoomFlag.NEVER + # `PREVIOUSLY` literally means the "room was sent down before *AND* + # there are updates we haven't sent down" so we already know this + # room has updates. + or status.status == HaveSentRoomFlag.PREVIOUSLY + ): rooms_should_send.add(room_id) + elif status.status == HaveSentRoomFlag.LIVE: + # We know that we've sent all updates up until `from_token`, + # so we just need to check if there have been updates since + # then. + pass + else: + assert_never(status.status) # We only need to check for new events since any state changes # will also come down as new events.