Convert Sliding Sync tests to use higher-level compute_interested_rooms

changelog.d/18399.misc

@@ -0,0 +1 @@
+Refactor [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Simplified Sliding Sync room list tests to cover both new and fallback logic paths.

synapse/handlers/sliding_sync/room_lists.py

@@ -244,14 +244,47 @@ async def _compute_interested_rooms_new_tables(
         # Note: this won't include rooms the user has left themselves. We add back
         # `newly_left` rooms below. This is more efficient than fetching all rooms and
         # then filtering out the old left rooms.
-        room_membership_for_user_map = await self.store.get_sliding_sync_rooms_for_user(
-            user_id
+        room_membership_for_user_map = (
+            await self.store.get_sliding_sync_rooms_for_user_from_membership_snapshots(
+                user_id
+            )
+        )
+        # To play nice with the rewind logic below, we need to go fetch the rooms the
+        # user has left themselves but only if it changed after the `to_token`.
+        #
+        # If a leave happens *after* the token range, we may have still been joined (or
+        # any non-self-leave which is relevant to sync) to the room before so we need to
+        # include it in the list of potentially relevant rooms and apply our rewind
+        # logic (outside of this function) to see if it's actually relevant.
+        #
+        # We do this separately from
+        # `get_sliding_sync_rooms_for_user_from_membership_snapshots` as those results
+        # are cached and the `to_token` isn't very cache friendly (people are constantly
+        # requesting with new tokens) so we separate it out here.
+        self_leave_room_membership_for_user_map = (
+            await self.store.get_sliding_sync_self_leave_rooms_after_to_token(
+                user_id, to_token
+            )
         )
+        if self_leave_room_membership_for_user_map:
+            # FIXME: It would be nice to avoid this copy but since
+            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
+            # can't return a mutable value like a `dict`. We make the copy to get a
+            # mutable dict that we can change. We try to only make a copy when necessary
+            # (if we actually need to change something) as in most cases, the logic
+            # doesn't need to run.
+            room_membership_for_user_map = dict(room_membership_for_user_map)
+            room_membership_for_user_map.update(self_leave_room_membership_for_user_map)
 
         # Remove invites from ignored users
         ignored_users = await self.store.ignored_users(user_id)
         if ignored_users:
-            # TODO: It would be nice to avoid these copies
+            # FIXME: It would be nice to avoid this copy but since
+            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
+            # can't return a mutable value like a `dict`. We make the copy to get a
+            # mutable dict that we can change. We try to only make a copy when necessary
+            # (if we actually need to change something) as in most cases, the logic
+            # doesn't need to run.
             room_membership_for_user_map = dict(room_membership_for_user_map)
             # Make a copy so we don't run into an error: `dictionary changed size during
             # iteration`, when we remove items
@@ -263,11 +296,23 @@ async def _compute_interested_rooms_new_tables(
                 ):
                     room_membership_for_user_map.pop(room_id, None)
 
+        (
+            newly_joined_room_ids,
+            newly_left_room_map,
+        ) = await self._get_newly_joined_and_left_rooms(
+            user_id, from_token=from_token, to_token=to_token
+        )
+
         changes = await self._get_rewind_changes_to_current_membership_to_token(
             sync_config.user, room_membership_for_user_map, to_token=to_token
         )
         if changes:
-            # TODO: It would be nice to avoid these copies
+            # FIXME: It would be nice to avoid this copy but since
+            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
+            # can't return a mutable value like a `dict`. We make the copy to get a
+            # mutable dict that we can change. We try to only make a copy when necessary
+            # (if we actually need to change something) as in most cases, the logic
+            # doesn't need to run.
             room_membership_for_user_map = dict(room_membership_for_user_map)
             for room_id, change in changes.items():
                 if change is None:
@@ -278,7 +323,7 @@ async def _compute_interested_rooms_new_tables(
                 existing_room = room_membership_for_user_map.get(room_id)
                 if existing_room is not None:
                     # Update room membership events to the point in time of the `to_token`
-                    room_membership_for_user_map[room_id] = RoomsForUserSlidingSync(
+                    room_for_user = RoomsForUserSlidingSync(
                         room_id=room_id,
                         sender=change.sender,
                         membership=change.membership,
@@ -290,18 +335,18 @@ async def _compute_interested_rooms_new_tables(
                         room_type=existing_room.room_type,
                         is_encrypted=existing_room.is_encrypted,
                     )
-
-        (
-            newly_joined_room_ids,
-            newly_left_room_map,
-        ) = await self._get_newly_joined_and_left_rooms(
-            user_id, from_token=from_token, to_token=to_token
-        )
-        dm_room_ids = await self._get_dm_rooms_for_user(user_id)
+                    if filter_membership_for_sync(
+                        user_id=user_id,
+                        room_membership_for_user=room_for_user,
+                        newly_left=room_id in newly_left_room_map,
+                    ):
+                        room_membership_for_user_map[room_id] = room_for_user
+                    else:
+                        room_membership_for_user_map.pop(room_id, None)
 
         # Add back `newly_left` rooms (rooms left in the from -> to token range).
         #
-        # We do this because `get_sliding_sync_rooms_for_user(...)` doesn't include
+        # We do this because `get_sliding_sync_rooms_for_user_from_membership_snapshots(...)` doesn't include
         # rooms that the user left themselves as it's more efficient to add them back
         # here than to fetch all rooms and then filter out the old left rooms. The user
         # only leaves a room once in a blue moon so this barely needs to run.
@@ -310,7 +355,12 @@ async def _compute_interested_rooms_new_tables(
             newly_left_room_map.keys() - room_membership_for_user_map.keys()
         )
         if missing_newly_left_rooms:
-            # TODO: It would be nice to avoid these copies
+            # FIXME: It would be nice to avoid this copy but since
+            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
+            # can't return a mutable value like a `dict`. We make the copy to get a
+            # mutable dict that we can change. We try to only make a copy when necessary
+            # (if we actually need to change something) as in most cases, the logic
+            # doesn't need to run.
             room_membership_for_user_map = dict(room_membership_for_user_map)
             for room_id in missing_newly_left_rooms:
                 newly_left_room_for_user = newly_left_room_map[room_id]
@@ -327,14 +377,21 @@ async def _compute_interested_rooms_new_tables(
                 # If the membership exists, it's just a normal user left the room on
                 # their own
                 if newly_left_room_for_user_sliding_sync is not None:
-                    room_membership_for_user_map[room_id] = (
-                        newly_left_room_for_user_sliding_sync
-                    )
+                    if filter_membership_for_sync(
+                        user_id=user_id,
+                        room_membership_for_user=newly_left_room_for_user_sliding_sync,
+                        newly_left=room_id in newly_left_room_map,
+                    ):
+                        room_membership_for_user_map[room_id] = (
+                            newly_left_room_for_user_sliding_sync
+                        )
+                    else:
+                        room_membership_for_user_map.pop(room_id, None)
 
                     change = changes.get(room_id)
                     if change is not None:
                         # Update room membership events to the point in time of the `to_token`
-                        room_membership_for_user_map[room_id] = RoomsForUserSlidingSync(
+                        room_for_user = RoomsForUserSlidingSync(
                             room_id=room_id,
                             sender=change.sender,
                             membership=change.membership,
@@ -346,6 +403,14 @@ async def _compute_interested_rooms_new_tables(
                             room_type=newly_left_room_for_user_sliding_sync.room_type,
                             is_encrypted=newly_left_room_for_user_sliding_sync.is_encrypted,
                         )
+                        if filter_membership_for_sync(
+                            user_id=user_id,
+                            room_membership_for_user=room_for_user,
+                            newly_left=room_id in newly_left_room_map,
+                        ):
+                            room_membership_for_user_map[room_id] = room_for_user
+                        else:
+                            room_membership_for_user_map.pop(room_id, None)
 
                 # If we are `newly_left` from the room but can't find any membership,
                 # then we have been "state reset" out of the room
@@ -367,7 +432,7 @@ async def _compute_interested_rooms_new_tables(
                         newly_left_room_for_user.event_pos.to_room_stream_token(),
                     )
 
-                    room_membership_for_user_map[room_id] = RoomsForUserSlidingSync(
+                    room_for_user = RoomsForUserSlidingSync(
                         room_id=room_id,
                         sender=newly_left_room_for_user.sender,
                         membership=newly_left_room_for_user.membership,
@@ -378,6 +443,16 @@ async def _compute_interested_rooms_new_tables(
                         room_type=room_type,
                         is_encrypted=is_encrypted,
                     )
+                    if filter_membership_for_sync(
+                        user_id=user_id,
+                        room_membership_for_user=room_for_user,
+                        newly_left=room_id in newly_left_room_map,
+                    ):
+                        room_membership_for_user_map[room_id] = room_for_user
+                    else:
+                        room_membership_for_user_map.pop(room_id, None)
+
+        dm_room_ids = await self._get_dm_rooms_for_user(user_id)
 
         if sync_config.lists:
             sync_room_map = room_membership_for_user_map
@@ -493,7 +568,12 @@ async def _compute_interested_rooms_new_tables(
 
         if sync_config.room_subscriptions:
             with start_active_span("assemble_room_subscriptions"):
-                # TODO: It would be nice to avoid these copies
+                # FIXME: It would be nice to avoid this copy but since
+                # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
+                # can't return a mutable value like a `dict`. We make the copy to get a
+                # mutable dict that we can change. We try to only make a copy when necessary
+                # (if we actually need to change something) as in most cases, the logic
+                # doesn't need to run.
                 room_membership_for_user_map = dict(room_membership_for_user_map)
 
                 # Find which rooms are partially stated and may need to be filtered out

synapse/storage/_base.py

@@ -130,15 +130,17 @@ def _invalidate_state_caches(
                 "_get_rooms_for_local_user_where_membership_is_inner", (user_id,)
             )
             self._attempt_to_invalidate_cache(
-                "get_sliding_sync_rooms_for_user", (user_id,)
+                "get_sliding_sync_rooms_for_user_from_membership_snapshots", (user_id,)
             )
 
         # Purge other caches based on room state.
         self._attempt_to_invalidate_cache("get_room_summary", (room_id,))
         self._attempt_to_invalidate_cache("get_partial_current_state_ids", (room_id,))
         self._attempt_to_invalidate_cache("get_room_type", (room_id,))
         self._attempt_to_invalidate_cache("get_room_encryption", (room_id,))
-        self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+        self._attempt_to_invalidate_cache(
+            "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+        )
 
     def _invalidate_state_caches_all(self, room_id: str) -> None:
         """Invalidates caches that are based on the current state, but does
@@ -168,7 +170,9 @@ def _invalidate_state_caches_all(self, room_id: str) -> None:
         self._attempt_to_invalidate_cache("get_room_summary", (room_id,))
         self._attempt_to_invalidate_cache("get_room_type", (room_id,))
         self._attempt_to_invalidate_cache("get_room_encryption", (room_id,))
-        self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+        self._attempt_to_invalidate_cache(
+            "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+        )
 
     def _attempt_to_invalidate_cache(
         self, cache_name: str, key: Optional[Collection[Any]]

synapse/storage/databases/main/cache.py

@@ -307,7 +307,7 @@ def _process_event_stream_row(self, token: int, row: EventsStreamRow) -> None:
                     "get_rooms_for_user", (data.state_key,)
                 )
                 self._attempt_to_invalidate_cache(
-                    "get_sliding_sync_rooms_for_user", None
+                    "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
                 )
                 self._membership_stream_cache.entity_has_changed(data.state_key, token)  # type: ignore[attr-defined]
             elif data.type == EventTypes.RoomEncryption:
@@ -319,7 +319,7 @@ def _process_event_stream_row(self, token: int, row: EventsStreamRow) -> None:
 
             if (data.type, data.state_key) in SLIDING_SYNC_RELEVANT_STATE_SET:
                 self._attempt_to_invalidate_cache(
-                    "get_sliding_sync_rooms_for_user", None
+                    "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
                 )
         elif row.type == EventsStreamAllStateRow.TypeId:
             assert isinstance(data, EventsStreamAllStateRow)
@@ -330,7 +330,9 @@ def _process_event_stream_row(self, token: int, row: EventsStreamRow) -> None:
             self._attempt_to_invalidate_cache("get_rooms_for_user", None)
             self._attempt_to_invalidate_cache("get_room_type", (data.room_id,))
             self._attempt_to_invalidate_cache("get_room_encryption", (data.room_id,))
-            self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+            self._attempt_to_invalidate_cache(
+                "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+            )
         else:
             raise Exception("Unknown events stream row type %s" % (row.type,))
 
@@ -394,7 +396,8 @@ def _invalidate_caches_for_event(
                 "_get_rooms_for_local_user_where_membership_is_inner", (state_key,)
             )
             self._attempt_to_invalidate_cache(
-                "get_sliding_sync_rooms_for_user", (state_key,)
+                "get_sliding_sync_rooms_for_user_from_membership_snapshots",
+                (state_key,),
             )
 
             self._attempt_to_invalidate_cache(
@@ -413,7 +416,9 @@ def _invalidate_caches_for_event(
             self._attempt_to_invalidate_cache("get_room_encryption", (room_id,))
 
         if (etype, state_key) in SLIDING_SYNC_RELEVANT_STATE_SET:
-            self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+            self._attempt_to_invalidate_cache(
+                "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+            )
 
         if relates_to:
             self._attempt_to_invalidate_cache(
@@ -470,7 +475,9 @@ def _invalidate_caches_for_room_events(self, room_id: str) -> None:
         self._attempt_to_invalidate_cache(
             "_get_rooms_for_local_user_where_membership_is_inner", None
         )
-        self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+        self._attempt_to_invalidate_cache(
+            "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+        )
         self._attempt_to_invalidate_cache("did_forget", None)
         self._attempt_to_invalidate_cache("get_forgotten_rooms_for_user", None)
         self._attempt_to_invalidate_cache("get_references_for_event", None)
@@ -529,7 +536,9 @@ def _invalidate_caches_for_room(self, room_id: str) -> None:
         self._attempt_to_invalidate_cache(
             "get_current_hosts_in_room_ordered", (room_id,)
         )
-        self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)
+        self._attempt_to_invalidate_cache(
+            "get_sliding_sync_rooms_for_user_from_membership_snapshots", None
+        )
         self._attempt_to_invalidate_cache("did_forget", None)
         self._attempt_to_invalidate_cache("get_forgotten_rooms_for_user", None)
         self._attempt_to_invalidate_cache("_get_membership_from_event_id", None)