From 51c282c81312310ea2b0271321812db1a1bf24d0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 7 Aug 2024 10:56:36 +0100 Subject: [PATCH 01/11] SSS: Implement PREVIOUSLY room tracking Implement tracking of rooms that have had updates that have not been sent down to clients. --- synapse/handlers/sliding_sync.py | 39 +++++++++- .../sliding_sync/test_connection_tracking.py | 72 ------------------- 2 files changed, 37 insertions(+), 74 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 1db96ad41c..5c3af74095 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -540,6 +540,9 @@ async def current_sync_for_user( lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {} # Keep track of the rooms that we can display and need to fetch more info about relevant_room_map: Dict[str, RoomSyncConfig] = {} + # The set of room IDs of all rooms that could appear in any list. These + # include rooms that are outside the list ranges. + all_rooms: Set[str] = set() if has_lists and sync_config.lists is not None: with start_active_span("assemble_sliding_window_lists"): sync_room_map = await self.filter_rooms_relevant_for_sync( @@ -558,6 +561,8 @@ async def current_sync_for_user( to_token, ) + all_rooms.update(filtered_sync_room_map) + # Sort the list sorted_room_info = await self.sort_rooms( filtered_sync_room_map, to_token @@ -661,6 +666,8 @@ async def current_sync_for_user( if not room_membership_for_user_at_to_token: continue + all_rooms.add(room_id) + room_membership_for_user_map[room_id] = ( room_membership_for_user_at_to_token ) @@ -768,12 +775,40 @@ async def handle_room(room_id: str) -> None: ) if has_lists or has_room_subscriptions: + # We now calculate if any rooms outside the range have had updates, + # which we are not sending down. + # + # We *must* record rooms that have had updates, but it is also fine + # to record rooms as having updates even if there might not actually + # be anything new for the user (e.g. due to event filters, events + # having happened after the user left, etc). + unsent_room_ids = [] + if from_token: + # The set of rooms that the client (may) care about, but aren't + # in any list range (or subscribed to). + missing_rooms = all_rooms - relevant_room_map.keys() + + # We now just go and try fetching any events in the above rooms + # to see if anything has happened since the `from_token`. + # + # TODO: Replace this with something faster. When we land the + # sliding sync tables that record the most recent event + # positions we can use that. + missing_event_map_by_room = ( + await self.store.get_room_events_stream_for_rooms( + missing_rooms, + from_key=from_token.stream_token.room_key, + to_key=to_token.room_key, + limit=1, + ) + ) + unsent_room_ids = list(missing_event_map_by_room) + connection_position = await self.connection_store.record_rooms( sync_config=sync_config, from_token=from_token, sent_room_ids=relevant_rooms_to_send_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` - unsent_room_ids=[], + unsent_room_ids=unsent_room_ids, ) elif from_token: connection_position = from_token.connection_position diff --git a/tests/rest/client/sliding_sync/test_connection_tracking.py b/tests/rest/client/sliding_sync/test_connection_tracking.py index 4d8866b30a..6863c32f7c 100644 --- a/tests/rest/client/sliding_sync/test_connection_tracking.py +++ b/tests/rest/client/sliding_sync/test_connection_tracking.py @@ -21,8 +21,6 @@ from synapse.api.constants import EventTypes from synapse.rest.client import login, room, sync from synapse.server import HomeServer -from synapse.types import SlidingSyncStreamToken -from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase @@ -130,7 +128,6 @@ def test_rooms_timeline_incremental_sync_PREVIOUSLY(self, limited: bool) -> None self.helper.send(room_id1, "msg", tok=user1_tok) timeline_limit = 5 - conn_id = "conn_id" sync_body = { "lists": { "foo-list": { @@ -170,40 +167,6 @@ def test_rooms_timeline_incremental_sync_PREVIOUSLY(self, limited: bool) -> None 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 events. resp = self.helper.send(room_id1, "msg2", tok=user1_tok) expected_events.append(resp["event_id"]) @@ -238,7 +201,6 @@ def test_rooms_required_state_incremental_sync_PREVIOUSLY(self) -> None: self.helper.send(room_id1, "msg", tok=user1_tok) - conn_id = "conn_id" sync_body = { "lists": { "foo-list": { @@ -279,40 +241,6 @@ def test_rooms_required_state_incremental_sync_PREVIOUSLY(self) -> None: 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) From 50f47345c14fd20a38df8afd27e62fb18f42308c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 7 Aug 2024 10:58:33 +0100 Subject: [PATCH 02/11] Newsfile --- changelog.d/17535.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17535.bugfix diff --git a/changelog.d/17535.bugfix b/changelog.d/17535.bugfix new file mode 100644 index 0000000000..c5b5da0485 --- /dev/null +++ b/changelog.d/17535.bugfix @@ -0,0 +1 @@ +Fix experimental sliding sync implementation to remember any updates in rooms that were not sent down immediately. From 2226ef079015273478394f3bbead9c8d3baa1407 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 7 Aug 2024 17:26:45 +0100 Subject: [PATCH 03/11] Filter out lazy loading --- synapse/handlers/sliding_sync.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 5c3af74095..503b8f36ea 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -561,8 +561,6 @@ async def current_sync_for_user( to_token, ) - all_rooms.update(filtered_sync_room_map) - # Sort the list sorted_room_info = await self.sort_rooms( filtered_sync_room_map, to_token @@ -588,6 +586,18 @@ async def current_sync_for_user( and StateValues.LAZY in membership_state_keys ) + if lazy_loading: + # Exclude partially-stated rooms unless the `required_state` + # only has `["m.room.member", "$LAZY"]` for membership + # (lazy-loading room members). + filtered_sync_room_map = { + room_id: room + for room_id, room in filtered_sync_room_map.items() + if room_id not in partial_state_room_map + } + + all_rooms.update(filtered_sync_room_map) + ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] if list_config.ranges: for range in list_config.ranges: @@ -605,15 +615,6 @@ async def current_sync_for_user( if len(room_ids_in_list) >= max_num_rooms: break - # Exclude partially-stated rooms unless the `required_state` - # only has `["m.room.member", "$LAZY"]` for membership - # (lazy-loading room members). - if ( - partial_state_room_map.get(room_id) - and not lazy_loading - ): - continue - # Take the superset of the `RoomSyncConfig` for each room. # # Update our `relevant_room_map` with the room we're going From aa8cda95741aaee92ff503cf441938fe1cb93d85 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 7 Aug 2024 17:38:48 +0100 Subject: [PATCH 04/11] D'oh --- synapse/handlers/sliding_sync.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 503b8f36ea..45739ccd35 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -561,11 +561,6 @@ async def current_sync_for_user( to_token, ) - # Sort the list - sorted_room_info = await self.sort_rooms( - filtered_sync_room_map, to_token - ) - # Find which rooms are partially stated and may need to be filtered out # depending on the `required_state` requested (see below). partial_state_room_map = ( @@ -586,18 +581,23 @@ async def current_sync_for_user( and StateValues.LAZY in membership_state_keys ) - if lazy_loading: + if not lazy_loading: # Exclude partially-stated rooms unless the `required_state` # only has `["m.room.member", "$LAZY"]` for membership # (lazy-loading room members). filtered_sync_room_map = { room_id: room for room_id, room in filtered_sync_room_map.items() - if room_id not in partial_state_room_map + if not partial_state_room_map.get(room_id) } all_rooms.update(filtered_sync_room_map) + # Sort the list + sorted_room_info = await self.sort_rooms( + filtered_sync_room_map, to_token + ) + ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] if list_config.ranges: for range in list_config.ranges: From fcc0fe1c301b3d17de05dabb91c632bb264e6d2d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:05:19 -0500 Subject: [PATCH 05/11] Better check whether we need to exclude partially stated rooms --- synapse/handlers/sliding_sync.py | 83 +++++++++-- .../sliding_sync/test_rooms_required_state.py | 140 ++++++++++++++---- 2 files changed, 183 insertions(+), 40 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 45739ccd35..dbbe705dce 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -24,6 +24,7 @@ from typing import ( TYPE_CHECKING, Any, + Callable, Dict, Final, List, @@ -363,6 +364,73 @@ def combine_room_sync_config( else: self.required_state_map[state_type].add(state_key) + def must_await_full_state( + self, + *, + is_mine_id: Callable[[str], bool], + is_mine_server_name: Callable[[str], bool], + ) -> bool: + """ + Check if we have a we're only requesting `required_state` which is completely + satisfied even with partial state, then we don't need to `await_full_state` before + we can return it. + + Also see `StateFilter.must_await_full_state(...)` for comparison + + Args: + is_mine_id: a callable which confirms if a given state_key matches a mxid + of a local user + is_mine_server_name: a callable which confirms if a given server name + matches the local server name + """ + # Partially-stated rooms should have all state events except for remote + # membership events so if we require a remote membership event anywhere, then we + # need to return `False`. + + wildcard_state_keys = self.required_state_map.get(StateValues.WILDCARD) + if ( + wildcard_state_keys is not None + and StateValues.WILDCARD in wildcard_state_keys + ): + return True + + if wildcard_state_keys is not None: + for possible_user_id in wildcard_state_keys: + if not possible_user_id[0].startswith(UserID.SIGIL): + # Not a user ID + continue + + localpart_hostname = possible_user_id.split(":", 1) + if len(localpart_hostname) < 2: + # Not a user ID + continue + + if not is_mine_server_name(localpart_hostname[1]): + return True + + membership_state_keys = self.required_state_map.get(EventTypes.Member) + # We aren't requesting any membership events at all + if membership_state_keys is None: + return False + + # If we're requesting entirely local users, TODO + for user_id in membership_state_keys: + if user_id == StateValues.ME: + continue + # We're lazy-loading membership so we can just return the state we have. + # Lazy-loading means we include membership for any event `sender` in the + # timeline but since we had to auth those timeline events, we will have the + # membership state for them (including from remote senders). + elif user_id == StateValues.LAZY: + continue + elif user_id == StateValues.WILDCARD: + return False + elif not is_mine_id(user_id): + return True + + # local users only + return False + class StateValues: """ @@ -392,6 +460,8 @@ def __init__(self, hs: "HomeServer"): self.device_handler = hs.get_device_handler() self.push_rules_handler = hs.get_push_rules_handler() self.rooms_to_exclude_globally = hs.config.server.rooms_to_exclude_from_sync + self.is_mine_id = hs.is_mine_id + self.is_mine_server_name = hs.is_mine_server_name self.connection_store = SlidingSyncConnectionStore() @@ -572,16 +642,11 @@ async def current_sync_for_user( # Since creating the `RoomSyncConfig` takes some work, let's just do it # once and make a copy whenever we need it. room_sync_config = RoomSyncConfig.from_room_config(list_config) - membership_state_keys = room_sync_config.required_state_map.get( - EventTypes.Member - ) - # Also see `StateFilter.must_await_full_state(...)` for comparison - lazy_loading = ( - membership_state_keys is not None - and StateValues.LAZY in membership_state_keys - ) - if not lazy_loading: + if room_sync_config.must_await_full_state( + is_mine_id=self.is_mine_id, + is_mine_server_name=self.is_mine_server_name, + ): # Exclude partially-stated rooms unless the `required_state` # only has `["m.room.member", "$LAZY"]` for membership # (lazy-loading room members). diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index a13cad223f..40f7c9d3d8 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -649,43 +649,123 @@ def test_rooms_required_state_partial_state(self) -> None: mark_event_as_partial_state(self.hs, join_response2["event_id"], room_id2) ) - # Make the Sliding Sync request (NOT lazy-loading room members) + # Make the Sliding Sync request with examples where `must_await_full_state()` is + # `False` sync_body = { "lists": { - "foo-list": { + "no-state-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + }, + "other-state-list": { "ranges": [[0, 1]], "required_state": [ [EventTypes.Create, ""], ], "timeline_limit": 0, }, + "lazy-load-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + # Lazy-load room members + [EventTypes.Member, StateValues.LAZY], + # Local member + [EventTypes.Member, user2_id], + ], + "timeline_limit": 0, + }, + "local-members-only-list": { + "ranges": [[0, 1]], + "required_state": [ + # Own user ID + [EventTypes.Member, user1_id], + # Local member + [EventTypes.Member, user2_id], + ], + "timeline_limit": 0, + }, + "me-list": { + "ranges": [[0, 1]], + "required_state": [ + # Own user ID + [EventTypes.Member, StateValues.ME], + # Local member + [EventTypes.Member, user2_id], + ], + "timeline_limit": 0, + }, + "wildcard-type-local-state-key-list": { + "ranges": [[0, 1]], + "required_state": [ + ["*", user1_id], + # Not a user ID + ["*", "foobarbaz"], + # Not a user ID + ["*", "foo.bar.baz"], + # Not a user ID + ["*", "@foo"], + ], + "timeline_limit": 0, + }, } } response_body, _ = self.do_sync(sync_body, tok=user1_tok) - # Make sure the list includes room1 but room2 is excluded because it's still - # partially-stated - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 1], - "room_ids": [room_id1], - } - ], - response_body["lists"]["foo-list"], - ) - - # Make the Sliding Sync request (with lazy-loading room members) + # The list should include both rooms now because we don't need full state + for list_key in response_body["lists"].keys(): + self.assertIncludes( + set(response_body["lists"][list_key]["ops"][0]["room_ids"]), + {room_id2, room_id1}, + exact=True, + message=f"Expected all rooms to show up for list_key={list_key}. Response " + + str(response_body["lists"][list_key]), + ) + + # Make the Sliding Sync request with examples where `must_await_full_state()` is + # `True` sync_body = { "lists": { - "foo-list": { + "wildcard-list": { + "ranges": [[0, 1]], + "required_state": [ + ["*", "*"], + ], + "timeline_limit": 0, + }, + "wildcard-type-remote-state-key-list": { + "ranges": [[0, 1]], + "required_state": [ + ["*", "@some:remote"], + # Not a user ID + ["*", "foobarbaz"], + # Not a user ID + ["*", "foo.bar.baz"], + # Not a user ID + ["*", "@foo"], + ], + "timeline_limit": 0, + }, + "remote-member-list": { + "ranges": [[0, 1]], + "required_state": [ + # Own user ID + [EventTypes.Member, user1_id], + # Remote member + [EventTypes.Member, "@some:remote"], + # Local member + [EventTypes.Member, user2_id], + ], + "timeline_limit": 0, + }, + "lazy-but-remote-member-list": { "ranges": [[0, 1]], "required_state": [ - [EventTypes.Create, ""], # Lazy-load room members [EventTypes.Member, StateValues.LAZY], + # Remote member + [EventTypes.Member, "@some:remote"], ], "timeline_limit": 0, }, @@ -693,15 +773,13 @@ def test_rooms_required_state_partial_state(self) -> None: } response_body, _ = self.do_sync(sync_body, tok=user1_tok) - # The list should include both rooms now because we're lazy-loading room members - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 1], - "room_ids": [room_id2, room_id1], - } - ], - response_body["lists"]["foo-list"], - ) + # Make sure the list includes room1 but room2 is excluded because it's still + # partially-stated + for list_key in response_body["lists"].keys(): + self.assertIncludes( + set(response_body["lists"][list_key]["ops"][0]["room_ids"]), + {room_id1}, + exact=True, + message=f"Expected only fully-stated rooms to show up for list_key={list_key}. Response " + + str(response_body["lists"][list_key]), + ) From eb40440512d0f400f6e42947d6ffc076658d3303 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:08:42 -0500 Subject: [PATCH 06/11] Update comments --- synapse/handlers/sliding_sync.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index dbbe705dce..e22e56651a 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -377,23 +377,26 @@ def must_await_full_state( Also see `StateFilter.must_await_full_state(...)` for comparison + Partially-stated rooms should have all state events except for remote membership + events so if we require a remote membership event anywhere, then we need to + return `False`. + Args: is_mine_id: a callable which confirms if a given state_key matches a mxid of a local user is_mine_server_name: a callable which confirms if a given server name matches the local server name """ - # Partially-stated rooms should have all state events except for remote - # membership events so if we require a remote membership event anywhere, then we - # need to return `False`. - wildcard_state_keys = self.required_state_map.get(StateValues.WILDCARD) + # Requesting *all* state in the room so we have to wait if ( wildcard_state_keys is not None and StateValues.WILDCARD in wildcard_state_keys ): return True + # If the wildcards don't refer to remote user IDs, then we don't need to wait + # for full state. if wildcard_state_keys is not None: for possible_user_id in wildcard_state_keys: if not possible_user_id[0].startswith(UserID.SIGIL): @@ -409,11 +412,12 @@ def must_await_full_state( return True membership_state_keys = self.required_state_map.get(EventTypes.Member) - # We aren't requesting any membership events at all + # We aren't requesting any membership events at all so the partial state will + # cover us. if membership_state_keys is None: return False - # If we're requesting entirely local users, TODO + # If we're requesting entirely local users, the partial state will cover us. for user_id in membership_state_keys: if user_id == StateValues.ME: continue @@ -428,7 +432,7 @@ def must_await_full_state( elif not is_mine_id(user_id): return True - # local users only + # Local users only so the partial state will cover us. return False From 30e2efe38783b79438d3c11eb3b527fa7a8013e7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:27:22 -0500 Subject: [PATCH 07/11] Exclude room subscriptions if partially stated and must await full state --- synapse/handlers/sliding_sync.py | 39 +++++++------ .../sliding_sync/test_rooms_required_state.py | 58 +++++++++++++++++++ 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index e22e56651a..f231ce1ef0 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -366,9 +366,7 @@ def combine_room_sync_config( def must_await_full_state( self, - *, is_mine_id: Callable[[str], bool], - is_mine_server_name: Callable[[str], bool], ) -> bool: """ Check if we have a we're only requesting `required_state` which is completely @@ -384,8 +382,6 @@ def must_await_full_state( Args: is_mine_id: a callable which confirms if a given state_key matches a mxid of a local user - is_mine_server_name: a callable which confirms if a given server name - matches the local server name """ wildcard_state_keys = self.required_state_map.get(StateValues.WILDCARD) # Requesting *all* state in the room so we have to wait @@ -408,7 +404,7 @@ def must_await_full_state( # Not a user ID continue - if not is_mine_server_name(localpart_hostname[1]): + if not is_mine_id(possible_user_id): return True membership_state_keys = self.required_state_map.get(EventTypes.Member) @@ -465,7 +461,6 @@ def __init__(self, hs: "HomeServer"): self.push_rules_handler = hs.get_push_rules_handler() self.rooms_to_exclude_globally = hs.config.server.rooms_to_exclude_from_sync self.is_mine_id = hs.is_mine_id - self.is_mine_server_name = hs.is_mine_server_name self.connection_store = SlidingSyncConnectionStore() @@ -647,13 +642,9 @@ async def current_sync_for_user( # once and make a copy whenever we need it. room_sync_config = RoomSyncConfig.from_room_config(list_config) - if room_sync_config.must_await_full_state( - is_mine_id=self.is_mine_id, - is_mine_server_name=self.is_mine_server_name, - ): - # Exclude partially-stated rooms unless the `required_state` - # only has `["m.room.member", "$LAZY"]` for membership - # (lazy-loading room members). + # Exclude partially-stated rooms if we must wait for the room to be + # fully-stated + if room_sync_config.must_await_full_state(self.is_mine_id): filtered_sync_room_map = { room_id: room for room_id, room in filtered_sync_room_map.items() @@ -720,6 +711,12 @@ async def current_sync_for_user( # Handle room subscriptions if has_room_subscriptions and sync_config.room_subscriptions is not None: with start_active_span("assemble_room_subscriptions"): + # Find which rooms are partially stated and may need to be filtered out + # depending on the `required_state` requested (see below). + partial_state_room_map = await self.store.is_partial_state_room_batched( + sync_config.room_subscriptions.keys() + ) + for ( room_id, room_subscription, @@ -736,19 +733,25 @@ async def current_sync_for_user( if not room_membership_for_user_at_to_token: continue - all_rooms.add(room_id) - room_membership_for_user_map[room_id] = ( room_membership_for_user_at_to_token ) # Take the superset of the `RoomSyncConfig` for each room. - # - # Update our `relevant_room_map` with the room we're going to display - # and need to fetch more info about. room_sync_config = RoomSyncConfig.from_room_config( room_subscription ) + + # Exclude partially-stated rooms if we must wait for the room to be + # fully-stated + if room_sync_config.must_await_full_state(self.is_mine_id): + if partial_state_room_map.get(room_id): + continue + + all_rooms.add(room_id) + + # Update our `relevant_room_map` with the room we're going to display + # and need to fetch more info about. existing_room_sync_config = relevant_room_map.get(room_id) if existing_room_sync_config is not None: existing_room_sync_config.combine_room_sync_config( diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 40f7c9d3d8..0f37a87cc0 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -723,6 +723,36 @@ def test_rooms_required_state_partial_state(self) -> None: + str(response_body["lists"][list_key]), ) + # Take each of the list variants and apply them to room subscriptions to make + # sure the same rules apply + for list_key in sync_body["lists"].keys(): + sync_body_for_subscriptions = { + "room_subscriptions": { + room_id1: { + "required_state": sync_body["lists"][list_key][ + "required_state" + ], + "timeline_limit": 0, + }, + room_id2: { + "required_state": sync_body["lists"][list_key][ + "required_state" + ], + "timeline_limit": 0, + }, + } + } + response_body, _ = self.do_sync(sync_body_for_subscriptions, tok=user1_tok) + + self.assertIncludes( + set(response_body["rooms"].keys()), + {room_id2, room_id1}, + exact=True, + message=f"Expected all rooms to show up for test_key={list_key}.", + ) + + # ===================================================================== + # Make the Sliding Sync request with examples where `must_await_full_state()` is # `True` sync_body = { @@ -783,3 +813,31 @@ def test_rooms_required_state_partial_state(self) -> None: message=f"Expected only fully-stated rooms to show up for list_key={list_key}. Response " + str(response_body["lists"][list_key]), ) + + # Take each of the list variants and apply them to room subscriptions to make + # sure the same rules apply + for list_key in sync_body["lists"].keys(): + sync_body_for_subscriptions = { + "room_subscriptions": { + room_id1: { + "required_state": sync_body["lists"][list_key][ + "required_state" + ], + "timeline_limit": 0, + }, + room_id2: { + "required_state": sync_body["lists"][list_key][ + "required_state" + ], + "timeline_limit": 0, + }, + } + } + response_body, _ = self.do_sync(sync_body_for_subscriptions, tok=user1_tok) + + self.assertIncludes( + set(response_body["rooms"].keys()), + {room_id1}, + exact=True, + message=f"Expected only fully-stated rooms to show up for test_key={list_key}.", + ) From 1e63a76c3436e3ba766dff80404994c064e54e1e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:33:34 -0500 Subject: [PATCH 08/11] Add changelog --- changelog.d/17538.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17538.bugfix diff --git a/changelog.d/17538.bugfix b/changelog.d/17538.bugfix new file mode 100644 index 0000000000..9e4e31dbdb --- /dev/null +++ b/changelog.d/17538.bugfix @@ -0,0 +1 @@ +Better exclude partially stated rooms if we must await full state in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From 0c2a9c6153458b75e192c97fc435ca5c5d5299a0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:46:01 -0500 Subject: [PATCH 09/11] Fix docstring --- synapse/handlers/sliding_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index f231ce1ef0..be08f3851c 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -377,7 +377,7 @@ def must_await_full_state( Partially-stated rooms should have all state events except for remote membership events so if we require a remote membership event anywhere, then we need to - return `False`. + return `True` (requires full state). Args: is_mine_id: a callable which confirms if a given state_key matches a mxid From 458d8b786772acc60232ba201d96a27e0d4d8322 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Aug 2024 13:46:28 -0500 Subject: [PATCH 10/11] Fix test description --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 0f37a87cc0..823e7db569 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -631,8 +631,7 @@ def test_rooms_required_state_combine_superset(self) -> None: def test_rooms_required_state_partial_state(self) -> None: """ - Test partially-stated room are excluded unless `rooms.required_state` is - lazy-loading room members. + Test partially-stated room are excluded if they require full state. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") From 10b5de8c675e1c92036919d1dfcfb15428ac3df8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 8 Aug 2024 12:55:12 -0500 Subject: [PATCH 11/11] Remove duplicate filtering See https://github.com/element-hq/synapse/pull/17538#discussion_r1709649930 --- synapse/handlers/sliding_sync.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 77ec09bc85..99510254f3 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -661,23 +661,6 @@ async def current_sync_for_user( filtered_sync_room_map, to_token ) - if not lazy_loading: - # Exclude partially-stated rooms unless the `required_state` - # only has `["m.room.member", "$LAZY"]` for membership - # (lazy-loading room members). - filtered_sync_room_map = { - room_id: room - for room_id, room in filtered_sync_room_map.items() - if not partial_state_room_map.get(room_id) - } - - all_rooms.update(filtered_sync_room_map) - - # Sort the list - sorted_room_info = await self.sort_rooms( - filtered_sync_room_map, to_token - ) - ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] if list_config.ranges: for range in list_config.ranges: