From 92b53e4f8c77b3d0e17e0cf69ad82a887dfbad6e Mon Sep 17 00:00:00 2001 From: Devon Hudson Date: Mon, 5 May 2025 15:54:57 -0600 Subject: [PATCH 01/22] Convert tests to use compute_interested_rooms --- tests/handlers/test_sliding_sync.py | 669 ++++++++++++++++++++++++---- 1 file changed, 573 insertions(+), 96 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 5b7e2937f83..7a22ee876e0 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -22,7 +22,7 @@ from unittest.mock import patch import attr -from parameterized import parameterized +from parameterized import parameterized, parameterized_class from twisted.test.proto_helpers import MemoryReactor @@ -43,13 +43,14 @@ 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, StateMap, StreamToken, UserID -from synapse.types.handlers.sliding_sync import SlidingSyncConfig +from synapse.types import JsonDict, StateMap, StreamToken, UserID, create_requester +from synapse.types.handlers.sliding_sync import PerConnectionState, SlidingSyncConfig from synapse.types.state import StateFilter from synapse.util import Clock from tests import unittest from tests.replication._base import BaseMultiWorkerStreamTestCase +from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase from tests.unittest import HomeserverTestCase, TestCase logger = logging.getLogger(__name__) @@ -572,9 +573,23 @@ def test_combine_room_sync_config( self._assert_room_config_equal(combined_config, expected, "A into B") -class GetRoomMembershipForUserAtToTokenTestCase(HomeserverTestCase): +# FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the +# foreground update for +# `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by +# https://github.com/element-hq/synapse/issues/17623) +@parameterized_class( + ("use_new_tables",), + [ + (True,), + (False,), + ], + class_name_func=lambda cls, + num, + params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}", +) +class ComputeInterestedRoomsTestCase(SlidingSyncBase): """ - Tests Sliding Sync handler `get_room_membership_for_user_at_to_token()` to make sure it returns + Tests Sliding Sync handler `compute_interested_rooms()` to make sure it returns the correct list of rooms IDs. """ @@ -597,6 +612,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.event_sources = hs.get_event_sources() self.storage_controllers = hs.get_storage_controllers() + super().prepare(reactor, clock, hs) + def test_no_rooms(self) -> None: """ Test when the user has never joined any rooms before @@ -606,13 +623,26 @@ def test_no_rooms(self) -> None: now_token = self.event_sources.get_current_token() - room_id_results, _, _ = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=now_token, to_token=now_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map self.assertEqual(room_id_results.keys(), set()) @@ -633,13 +663,28 @@ def test_get_newly_joined_room(self) -> None: after_room_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room_token, to_token=after_room_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id}) # It should be pointing to the join event (latest membership event in the @@ -668,13 +713,28 @@ def test_get_already_joined_room(self) -> None: after_room_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room_token, to_token=after_room_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id}) # It should be pointing to the join event (latest membership event in the @@ -742,13 +802,28 @@ def test_get_invited_banned_knocked_room(self) -> None: after_room_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room_token, to_token=after_room_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Ensure that the invited, ban, and knock rooms show up self.assertEqual( @@ -814,13 +889,28 @@ def test_get_kicked_room(self) -> None: after_kick_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_kick_token, to_token=after_kick_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # The kicked room should show up self.assertEqual(room_id_results.keys(), {kick_room_id}) @@ -907,13 +997,26 @@ def test_forgotten_rooms(self) -> None: ) self.assertEqual(channel.code, 200, channel.result) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room_forgets, to_token=before_room_forgets, ) ) + room_id_results = interested_rooms.room_membership_for_user_map # We shouldn't see the room because it was forgotten self.assertEqual(room_id_results.keys(), set()) @@ -937,13 +1040,28 @@ def test_newly_left_rooms(self) -> None: after_room2_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room2_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) @@ -987,13 +1105,28 @@ def test_no_joins_after_to_token(self) -> None: room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) self.helper.join(room_id2, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id1}) # It should be pointing to the latest membership event in the from/to range @@ -1027,13 +1160,28 @@ def test_join_during_range_and_left_room_after_to_token(self) -> None: # Leave the room after we already have our tokens leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # We should still see the room because we were joined during the # from_token/to_token time period. @@ -1074,13 +1222,28 @@ def test_join_before_range_and_left_room_after_to_token(self) -> None: # Leave the room after we already have our tokens leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # We should still see the room because we were joined before the `from_token` self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1138,13 +1301,28 @@ def test_kicked_before_range_and_left_after_to_token(self) -> None: join_response2 = self.helper.join(kick_room_id, user1_id, tok=user1_tok) leave_response = self.helper.leave(kick_room_id, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_kick_token, to_token=after_kick_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # We shouldn't see the room because it was forgotten self.assertEqual(room_id_results.keys(), {kick_room_id}) @@ -1194,13 +1372,28 @@ def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: join_response2 = self.helper.join(room_id1, user1_id, tok=user1_tok) leave_response2 = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should still show up because it's newly_left during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1249,13 +1442,28 @@ def test_newly_left_during_range_and_join_after_to_token(self) -> None: # Join the room after we already have our tokens join_response2 = self.helper.join(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should still show up because it's newly_left during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1308,13 +1516,28 @@ def test_no_from_token(self) -> None: # Join the room2 after we already have our tokens self.helper.join(room_id2, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=None, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Only rooms we were joined to before the `to_token` should show up self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) @@ -1390,13 +1613,28 @@ def test_from_token_ahead_of_to_token(self) -> None: # Join the room4 after we already have our tokens self.helper.join(room_id4, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=from_token, to_token=to_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # In the "current" state snapshot, we're joined to all of the rooms but in the # from/to token range... @@ -1463,13 +1701,28 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: self.helper.join(room_id1, user1_id, tok=user1_tok) self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id1}) # It should be pointing to the latest membership event in the from/to range @@ -1506,13 +1759,28 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: # Join the room after we already have our tokens self.helper.join(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual(room_id_results.keys(), {room_id1}) # It should be pointing to the latest membership event in the from/to range @@ -1556,13 +1824,28 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( join_response3 = self.helper.join(room_id1, user1_id, tok=user1_tok) leave_response3 = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because it was newly_left and joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1618,13 +1901,28 @@ def test_join_leave_multiple_times_before_range_and_after_to_token( join_response3 = self.helper.join(room_id1, user1_id, tok=user1_tok) leave_response3 = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined before the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1677,13 +1975,28 @@ def test_invite_before_range_and_join_leave_after_to_token( join_respsonse = self.helper.join(room_id1, user1_id, tok=user1_tok) leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were invited before the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1751,13 +2064,28 @@ def test_join_and_display_name_changes_in_token_range( tok=user1_tok, ) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1816,13 +2144,28 @@ def test_display_name_changes_in_token_range( after_change1_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_change1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1888,13 +2231,28 @@ def test_display_name_changes_before_and_after_token_range( tok=user1_tok, ) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined before the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -1970,13 +2328,28 @@ def test_display_name_changes_leave_after_token_range( # Leave after the token self.helper.leave(room_id1, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -2038,13 +2411,26 @@ def test_display_name_changes_join_after_token_range( tok=user1_tok, ) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map # Room shouldn't show up because we joined after the from/to range self.assertEqual(room_id_results.keys(), set()) @@ -2074,13 +2460,28 @@ def test_newly_joined_with_leave_join_in_token_range( after_more_changes_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=after_room1_token, to_token=after_more_changes_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -2139,13 +2540,28 @@ def test_newly_joined_only_joins_during_token_range( after_room1_token = self.event_sources.get_current_token() - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room1_token, to_token=after_room1_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room should show up because it was newly_left and joined during the from/to range self.assertEqual(room_id_results.keys(), {room_id1}) @@ -2215,13 +2631,28 @@ def test_multiple_rooms_are_not_confused( # Leave room3 self.helper.leave(room_id3, user1_id, tok=user1_tok) - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_room3_token, to_token=after_room3_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual( room_id_results.keys(), @@ -2351,13 +2782,28 @@ def test_state_reset(self) -> None: after_reset_token = self.event_sources.get_current_token() # The function under test - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_reset_token, to_token=after_reset_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms # Room1 should show up because it was `newly_left` via state reset during the from/to range self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) @@ -2375,7 +2821,23 @@ def test_state_reset(self) -> None: self.assertTrue(room_id1 in newly_left) -class GetRoomMembershipForUserAtToTokenShardTestCase(BaseMultiWorkerStreamTestCase): +# FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the +# foreground update for +# `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by +# https://github.com/element-hq/synapse/issues/17623) +@parameterized_class( + ("use_new_tables",), + [ + (True,), + (False,), + ], + class_name_func=lambda cls, + num, + params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}", +) +class GetRoomMembershipForUserAtToTokenShardTestCase( + BaseMultiWorkerStreamTestCase, SlidingSyncBase +): """ Tests Sliding Sync handler `get_room_membership_for_user_at_to_token()` to make sure it works with sharded event stream_writers enabled @@ -2565,13 +3027,28 @@ def test_sharded_event_persisters(self) -> None: self.get_success(actx.__aexit__(None, None, None)) # The function under test - room_id_results, newly_joined, newly_left = self.get_success( - self.sliding_sync_handler.room_lists.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), from_token=before_stuck_activity_token, to_token=stuck_activity_token, ) ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms self.assertEqual( room_id_results.keys(), From cd4520ed5fa1de2532a123bdfe63b48c28c43cb1 Mon Sep 17 00:00:00 2001 From: Devon Hudson Date: Mon, 5 May 2025 15:57:14 -0600 Subject: [PATCH 02/22] Add changelog entry --- changelog.d/18399.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18399.misc diff --git a/changelog.d/18399.misc b/changelog.d/18399.misc new file mode 100644 index 00000000000..07d3d5a8f72 --- /dev/null +++ b/changelog.d/18399.misc @@ -0,0 +1 @@ +Convert tests to use compute_interested_rooms. From a4348927736dbef74dc72a98ee8d2da641cd1bf8 Mon Sep 17 00:00:00 2001 From: Devon Hudson Date: Mon, 5 May 2025 16:18:28 -0600 Subject: [PATCH 03/22] Rename test class --- tests/handlers/test_sliding_sync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 7a22ee876e0..9eb592fb696 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2835,11 +2835,11 @@ def test_state_reset(self) -> None: num, params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}", ) -class GetRoomMembershipForUserAtToTokenShardTestCase( +class ComputeInterestedRoomsShardTestCase( BaseMultiWorkerStreamTestCase, SlidingSyncBase ): """ - Tests Sliding Sync handler `get_room_membership_for_user_at_to_token()` to make sure it works with + Tests Sliding Sync handler `compute_interested_rooms()` to make sure it works with sharded event stream_writers enabled """ From de8057439101b809191f33786026d14a20cc628b Mon Sep 17 00:00:00 2001 From: Devon Hudson Date: Tue, 6 May 2025 09:45:03 -0600 Subject: [PATCH 04/22] Remove leave membership filter when getting rooms for user --- synapse/storage/databases/main/roommember.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index dfa7dd48d9a..0eeb251eb5b 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -1452,7 +1452,6 @@ def get_sliding_sync_rooms_for_user_txn( LEFT JOIN sliding_sync_joined_rooms AS j ON (j.room_id = m.room_id AND m.membership = 'join') WHERE user_id = ? AND m.forgotten = 0 - AND (m.membership != 'leave' OR m.user_id != m.sender) """ txn.execute(sql, (user_id,)) return { From 1b4eb2bfa2cf9ddfaeaa294253a2a7b9318afb67 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 07:24:31 -0500 Subject: [PATCH 05/22] Add extra test for not `newly_joined` or `newly_left` display name change --- tests/handlers/test_sliding_sync.py | 109 +++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 9eb592fb696..b35f90f70eb 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2278,13 +2278,14 @@ def test_display_name_changes_before_and_after_token_range( self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) - def test_display_name_changes_leave_after_token_range( + def test_newly_joined_display_name_changes_leave_after_token_range( self, ) -> None: """ Test that we point to the correct membership event within the from/to range even - if there are multiple `join` membership events in a row indicating - `displayname`/`avatar_url` updates and we leave after the `to_token`. + if we are `newly_joined` and there are multiple `join` membership events in a + row indicating `displayname`/`avatar_url` updates and we leave after the + `to_token`. See condition "1a)" comments in the `get_room_membership_for_user_at_to_token()` method. """ @@ -2299,6 +2300,7 @@ def test_display_name_changes_leave_after_token_range( # leave and can still re-join. room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) join_response = self.helper.join(room_id1, user1_id, tok=user1_tok) + # Update the displayname during the token range displayname_change_during_token_range_response = self.helper.send_state( room_id1, @@ -2375,6 +2377,107 @@ def test_display_name_changes_leave_after_token_range( self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) + def test_display_name_changes_leave_after_token_range( + self, + ) -> None: + """ + Test that we point to the correct membership event within the from/to range even + if there are multiple `join` membership events in a row indicating + `displayname`/`avatar_url` updates and we leave after the `to_token`. + + See condition "1a)" comments in the `get_room_membership_for_user_at_to_token()` method. + """ + 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") + + _before_room1_token = self.event_sources.get_current_token() + + # We create the room with user2 so the room isn't left with no members when we + # leave and can still re-join. + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) + join_response = self.helper.join(room_id1, user1_id, tok=user1_tok) + + after_join_token = self.event_sources.get_current_token() + + # Update the displayname during the token range + displayname_change_during_token_range_response = self.helper.send_state( + room_id1, + event_type=EventTypes.Member, + state_key=user1_id, + body={ + "membership": Membership.JOIN, + "displayname": "displayname during token range", + }, + tok=user1_tok, + ) + + after_display_name_change_token = self.event_sources.get_current_token() + + # Update the displayname after the token range + displayname_change_after_token_range_response = self.helper.send_state( + room_id1, + event_type=EventTypes.Member, + state_key=user1_id, + body={ + "membership": Membership.JOIN, + "displayname": "displayname after token range", + }, + tok=user1_tok, + ) + + # Leave after the token + self.helper.leave(room_id1, user1_id, tok=user1_tok) + + interested_rooms = self.get_success( + self.sliding_sync_handler.room_lists.compute_interested_rooms( + SlidingSyncConfig( + user=UserID.from_string(user1_id), + requester=create_requester(user_id=user1_id), + lists={ + "foo-list": SlidingSyncConfig.SlidingSyncList( + ranges=[(0, 99)], + required_state=[], + timeline_limit=1, + ) + }, + conn_id=None, + ), + PerConnectionState(), + from_token=after_join_token, + to_token=after_display_name_change_token, + ) + ) + room_id_results = interested_rooms.room_membership_for_user_map + newly_joined = interested_rooms.newly_joined_rooms + newly_left = interested_rooms.newly_left_rooms + + # Room should show up because we were joined during the from/to range + self.assertEqual(room_id_results.keys(), {room_id1}) + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id1].event_id, + displayname_change_during_token_range_response["event_id"], + "Corresponding map to disambiguate the opaque event IDs: " + + str( + { + "join_response": join_response["event_id"], + "displayname_change_during_token_range_response": displayname_change_during_token_range_response[ + "event_id" + ], + "displayname_change_after_token_range_response": displayname_change_after_token_range_response[ + "event_id" + ], + } + ), + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + # We only changed our display name during the token range so we shouldn't be + # considered `newly_joined` or `newly_left` + self.assertTrue(room_id1 not in newly_joined) + self.assertTrue(room_id1 not in newly_left) + def test_display_name_changes_join_after_token_range( self, ) -> None: From 1a046bf1790d3f7fbd6d3d0368ab8ad15d1df4aa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 07:25:08 -0500 Subject: [PATCH 06/22] Remove extra copies --- synapse/handlers/sliding_sync/room_lists.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index a1730b7e05b..1eb86f45119 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -251,8 +251,6 @@ async def _compute_interested_rooms_new_tables( # 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 - 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 for room_id in list(room_membership_for_user_map.keys()): @@ -267,8 +265,6 @@ async def _compute_interested_rooms_new_tables( sync_config.user, room_membership_for_user_map, to_token=to_token ) if changes: - # TODO: It would be nice to avoid these copies - room_membership_for_user_map = dict(room_membership_for_user_map) for room_id, change in changes.items(): if change is None: # Remove rooms that the user joined after the `to_token` @@ -286,6 +282,11 @@ async def _compute_interested_rooms_new_tables( event_pos=change.event_pos, room_version_id=change.room_version_id, # We keep the state of the room though + # + # TODO: Seems slightly flawed to use the state of the room that + # may be after our `to_token` but I guess we're assuming that + # the `is_encrypted` status of the room doesn't change that + # often and `room_type` never changes. has_known_state=existing_room.has_known_state, room_type=existing_room.room_type, is_encrypted=existing_room.is_encrypted, @@ -310,8 +311,6 @@ 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 - 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] # This should be a given @@ -342,6 +341,11 @@ async def _compute_interested_rooms_new_tables( event_pos=change.event_pos, room_version_id=change.room_version_id, # We keep the state of the room though + # + # TODO: Seems slightly flawed to use the state of the room that + # may be after our `to_token` but I guess we're assuming that + # the `is_encrypted` status of the room doesn't change that + # often and `room_type` never changes. has_known_state=newly_left_room_for_user_sliding_sync.has_known_state, room_type=newly_left_room_for_user_sliding_sync.room_type, is_encrypted=newly_left_room_for_user_sliding_sync.is_encrypted, @@ -493,9 +497,6 @@ 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 - room_membership_for_user_map = dict(room_membership_for_user_map) - # Find which rooms are partially stated and may need to be filtered out # depending on the `required_state` requested (see below). partial_state_rooms = await self.store.get_partial_rooms() From 1794c552cad4332cffde8d67d6acae842587acc1 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 07:34:43 -0500 Subject: [PATCH 07/22] Revert "Remove extra copies" This reverts commit d2a4179960e266dc35a06e28c08015570c9a4b21. --- synapse/handlers/sliding_sync/room_lists.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 1eb86f45119..a1730b7e05b 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -251,6 +251,8 @@ async def _compute_interested_rooms_new_tables( # 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 + 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 for room_id in list(room_membership_for_user_map.keys()): @@ -265,6 +267,8 @@ async def _compute_interested_rooms_new_tables( sync_config.user, room_membership_for_user_map, to_token=to_token ) if changes: + # TODO: It would be nice to avoid these copies + room_membership_for_user_map = dict(room_membership_for_user_map) for room_id, change in changes.items(): if change is None: # Remove rooms that the user joined after the `to_token` @@ -282,11 +286,6 @@ async def _compute_interested_rooms_new_tables( event_pos=change.event_pos, room_version_id=change.room_version_id, # We keep the state of the room though - # - # TODO: Seems slightly flawed to use the state of the room that - # may be after our `to_token` but I guess we're assuming that - # the `is_encrypted` status of the room doesn't change that - # often and `room_type` never changes. has_known_state=existing_room.has_known_state, room_type=existing_room.room_type, is_encrypted=existing_room.is_encrypted, @@ -311,6 +310,8 @@ 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 + 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] # This should be a given @@ -341,11 +342,6 @@ async def _compute_interested_rooms_new_tables( event_pos=change.event_pos, room_version_id=change.room_version_id, # We keep the state of the room though - # - # TODO: Seems slightly flawed to use the state of the room that - # may be after our `to_token` but I guess we're assuming that - # the `is_encrypted` status of the room doesn't change that - # often and `room_type` never changes. has_known_state=newly_left_room_for_user_sliding_sync.has_known_state, room_type=newly_left_room_for_user_sliding_sync.room_type, is_encrypted=newly_left_room_for_user_sliding_sync.is_encrypted, @@ -497,6 +493,9 @@ 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 + room_membership_for_user_map = dict(room_membership_for_user_map) + # Find which rooms are partially stated and may need to be filtered out # depending on the `required_state` requested (see below). partial_state_rooms = await self.store.get_partial_rooms() From a980e10445f6b1812254a1fb35ba052d6256bd1b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 08:31:45 -0500 Subject: [PATCH 08/22] Fix missing self-leave rooms when looking at token range before the leave --- synapse/handlers/sliding_sync/room_lists.py | 15 ++++++++- synapse/storage/databases/main/roommember.py | 35 ++++++++++++++++++-- tests/handlers/test_sliding_sync.py | 22 ++++++------ 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index a1730b7e05b..b09e9e9d1d2 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -245,7 +245,7 @@ async def _compute_interested_rooms_new_tables( # `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 + user_id, to_token ) # Remove invites from ignored users @@ -263,6 +263,10 @@ async def _compute_interested_rooms_new_tables( ): room_membership_for_user_map.pop(room_id, None) + logger.info( + "asdf room_membership_for_user_map %s", room_membership_for_user_map + ) + changes = await self._get_rewind_changes_to_current_membership_to_token( sync_config.user, room_membership_for_user_map, to_token=to_token ) @@ -299,6 +303,9 @@ async def _compute_interested_rooms_new_tables( ) dm_room_ids = await self._get_dm_rooms_for_user(user_id) + logger.info("asdf newly_joined_room_ids: %s", newly_joined_room_ids) + logger.info("asdf newly_left_room_map: %s", newly_left_room_map) + # 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 @@ -309,6 +316,7 @@ async def _compute_interested_rooms_new_tables( missing_newly_left_rooms = ( newly_left_room_map.keys() - room_membership_for_user_map.keys() ) + logger.info("asdf missing_newly_left_rooms: %s", missing_newly_left_rooms) if missing_newly_left_rooms: # TODO: It would be nice to avoid these copies room_membership_for_user_map = dict(room_membership_for_user_map) @@ -332,6 +340,11 @@ async def _compute_interested_rooms_new_tables( ) change = changes.get(room_id) + logger.info( + "asdf newly_left_room_for_user_sliding_sync: %s change %s", + newly_left_room_for_user_sliding_sync, + change, + ) 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( diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 842a1f9cf23..c80a923e9be 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -65,6 +65,7 @@ PersistedEventPosition, StateMap, StrCollection, + StreamToken, get_domain_from_id, ) from synapse.util.caches.descriptors import _CacheContext, cached, cachedList @@ -1424,10 +1425,14 @@ async def update_room_forgetter_stream_pos(self, stream_id: int) -> None: async def get_sliding_sync_rooms_for_user( self, user_id: str, + to_token: StreamToken, ) -> Mapping[str, RoomsForUserSlidingSync]: """Get all the rooms for a user to handle a sliding sync request. - Ignores forgotten rooms and rooms that the user has left themselves. + Ignores forgotten rooms. Also ignores rooms that the user has left themselves + unless the user left *after* the token range + (the token range is > `from_token` and <= `to_token`). + Returns: Map from room ID to membership info @@ -1440,6 +1445,15 @@ def get_sliding_sync_rooms_for_user_txn( # `sliding_sync_joined_rooms` or `forgotten`), make sure to bust the # `get_sliding_sync_rooms_for_user` cache in the appropriate places (and add # tests). + + sql = """ + SELECT * FROM sliding_sync_membership_snapshots + WHERE user_id = ? + AND forgotten = 0 + """ + txn.execute(sql, (user_id,)) + logger.info("asdf raw sliding_sync_membership_snapshots %s", txn.fetchall()) + sql = """ SELECT m.room_id, m.sender, m.membership, m.membership_event_id, r.room_version, @@ -1452,8 +1466,25 @@ def get_sliding_sync_rooms_for_user_txn( LEFT JOIN sliding_sync_joined_rooms AS j ON (j.room_id = m.room_id AND m.membership = 'join') WHERE user_id = ? AND m.forgotten = 0 + AND ( + (m.membership != 'leave' OR m.user_id != m.sender) + OR (m.event_stream_ordering > ?) + ) """ - txn.execute(sql, (user_id,)) + logger.info("asdf to_token %s", 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 to return + # here and apply our rewind logic (outside of this function). + # + # We use the min token position to ensure we don't miss any updates. With a + # naive stream_ordering comparison, we might catch a few membership updates + # that are within the token range but then they are relevant to sync anyway + # from being with in the token range so we don't need to to any + # post-filtering after we get the results back from the database. + min_to_token_position = to_token.room_key.stream + txn.execute(sql, (user_id, min_to_token_position)) + return { row[0]: RoomsForUserSlidingSync( room_id=row[0], diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index b35f90f70eb..9017731a7ac 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -1030,7 +1030,7 @@ def test_newly_left_rooms(self) -> None: # Leave before we calculate the `from_token` room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) - leave_response1 = self.helper.leave(room_id1, user1_id, tok=user1_tok) + _leave_response1 = self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1063,17 +1063,19 @@ def test_newly_left_rooms(self) -> None: newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) - + # `room_id1` should not show up because it was left before the token range. + # `room_id2` should show up because it is `newly_left` within the token range. self.assertEqual( - room_id_results[room_id1].event_id, - leave_response1["event_id"], + room_id_results.keys(), + {room_id2}, + "Corresponding map to disambiguate the opaque room IDs: " + + str( + { + "room_id1": room_id1, + "room_id2": room_id2, + } + ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) - # We should *NOT* be `newly_joined` or `newly_left` because that happened before - # the from/to range - self.assertTrue(room_id1 not in newly_joined) - self.assertTrue(room_id1 not in newly_left) self.assertEqual( room_id_results[room_id2].event_id, From 6c4e8779fd53e0039532fc8d930c108cb210519d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 08:49:32 -0500 Subject: [PATCH 09/22] Add better notes on how why we do this specific logic --- synapse/handlers/sliding_sync/room_lists.py | 14 +++--- synapse/storage/_base.py | 10 ++-- synapse/storage/databases/main/cache.py | 23 +++++++--- synapse/storage/databases/main/roommember.py | 48 ++++++++++++++------ tests/handlers/test_sliding_sync.py | 8 +++- 5 files changed, 72 insertions(+), 31 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index b09e9e9d1d2..20f1563a91d 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -241,11 +241,13 @@ async def _compute_interested_rooms_new_tables( # include rooms that are outside the list ranges. all_rooms: Set[str] = set() - # 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, to_token + # Note: this won't include rooms the user has left themselves (for the most + # part). 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_from_membership_snapshots( + user_id, to_token + ) ) # Remove invites from ignored users @@ -308,7 +310,7 @@ async def _compute_interested_rooms_new_tables( # 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. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 7251e72e3ad..b5fe7dd8589 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -130,7 +130,7 @@ 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. @@ -138,7 +138,9 @@ def _invalidate_state_caches( 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]] diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py index f364464c23a..9418fb6dd75 100644 --- a/synapse/storage/databases/main/cache.py +++ b/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) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index c80a923e9be..4ddb98024da 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -1390,7 +1390,9 @@ def f(txn: LoggingTransaction) -> None: txn, self.get_forgotten_rooms_for_user, (user_id,) ) self._invalidate_cache_and_stream( - txn, self.get_sliding_sync_rooms_for_user, (user_id,) + txn, + self.get_sliding_sync_rooms_for_user_from_membership_snapshots, + (user_id,), ) await self.db_pool.runInteraction("forget_membership", f) @@ -1422,29 +1424,47 @@ async def update_room_forgetter_stream_pos(self, stream_id: int) -> None: ) @cached(iterable=True, max_entries=10000) - async def get_sliding_sync_rooms_for_user( + async def get_sliding_sync_rooms_for_user_from_membership_snapshots( self, user_id: str, - to_token: StreamToken, + to_token_self_leave_hint: StreamToken, ) -> Mapping[str, RoomsForUserSlidingSync]: - """Get all the rooms for a user to handle a sliding sync request. + """ + Get all the rooms for a user to handle a sliding sync request from the + `sliding_sync_membership_snapshots` table. These will be current memberships and + need to be rewound to the token range. + + Ignores forgotten rooms. - Ignores forgotten rooms. Also ignores rooms that the user has left themselves - unless the user left *after* the token range - (the token range is > `from_token` and <= `to_token`). + Also ignores rooms that the user has left themselves unless [1] as it's more + efficient to add them back here than to fetch all rooms and then filter out the + old left rooms. + [1]: We ignore rooms that the user has left themselves unless the user left + *after* the token range (the token range is > `from_token` and <= `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 to return here and apply + our rewind logic (outside of this function). + + Args: + user_id: The user ID to get the rooms for. + to_token_self_leave_hint: This should be the `to_token` from the sync + requests. We do not filter out or rewind results to this token. It's simply + a hint, to return any self-leave membership if they're after this token (see + why above). Returns: Map from room ID to membership info """ - def get_sliding_sync_rooms_for_user_txn( + def _txn( txn: LoggingTransaction, ) -> Dict[str, RoomsForUserSlidingSync]: # XXX: If you use any new columns that can change (like from # `sliding_sync_joined_rooms` or `forgotten`), make sure to bust the - # `get_sliding_sync_rooms_for_user` cache in the appropriate places (and add - # tests). + # `get_sliding_sync_rooms_for_user_from_membership_snapshots` cache in the + # appropriate places (and add tests). sql = """ SELECT * FROM sliding_sync_membership_snapshots @@ -1471,7 +1491,7 @@ def get_sliding_sync_rooms_for_user_txn( OR (m.event_stream_ordering > ?) ) """ - logger.info("asdf to_token %s", to_token) + logger.info("asdf to_token %s", to_token_self_leave_hint) # 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 to return @@ -1482,7 +1502,7 @@ def get_sliding_sync_rooms_for_user_txn( # that are within the token range but then they are relevant to sync anyway # from being with in the token range so we don't need to to any # post-filtering after we get the results back from the database. - min_to_token_position = to_token.room_key.stream + min_to_token_position = to_token_self_leave_hint.room_key.stream txn.execute(sql, (user_id, min_to_token_position)) return { @@ -1505,8 +1525,8 @@ def get_sliding_sync_rooms_for_user_txn( } return await self.db_pool.runInteraction( - "get_sliding_sync_rooms_for_user", - get_sliding_sync_rooms_for_user_txn, + "get_sliding_sync_rooms_for_user_from_membership_snapshots", + _txn, ) async def get_sliding_sync_room_for_user( diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 9017731a7ac..aef10f0f587 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2824,7 +2824,7 @@ def test_state_reset(self) -> None: # Join another room so we don't hit the short-circuit and return early if they # have no room membership room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) - self.helper.join(room_id2, user1_id, tok=user1_tok) + join_response2 = self.helper.join(room_id2, user1_id, tok=user1_tok) before_reset_token = self.event_sources.get_current_token() @@ -2917,6 +2917,12 @@ def test_state_reset(self) -> None: self.assertEqual( room_id_results[room_id1].event_id, None, + "Corresponding map to disambiguate the opaque event IDs: " + + str( + { + "join_response1": join_response1["event_id"], + } + ), ) # State reset caused us to leave the room and there is no corresponding leave event self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) From 235a52eb9d3815255e88082bf4302dbc77ff8413 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 09:03:43 -0500 Subject: [PATCH 10/22] Make state reset test more real and passes with new Sliding Sync path --- synapse/handlers/sliding_sync/room_lists.py | 9 +-- tests/handlers/test_sliding_sync.py | 84 ++++++++------------- 2 files changed, 35 insertions(+), 58 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 20f1563a91d..b26ce86f3cc 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -334,6 +334,10 @@ async def _compute_interested_rooms_new_tables( newly_left_room_for_user_sliding_sync = ( await self.store.get_sliding_sync_room_for_user(user_id, room_id) ) + logger.info( + "asdf newly_left_room_for_user_sliding_sync: %s", + newly_left_room_for_user_sliding_sync, + ) # 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: @@ -342,11 +346,6 @@ async def _compute_interested_rooms_new_tables( ) change = changes.get(room_id) - logger.info( - "asdf newly_left_room_for_user_sliding_sync: %s change %s", - newly_left_room_for_user_sliding_sync, - change, - ) 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( diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index aef10f0f587..4be470e24b6 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -52,6 +52,7 @@ from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase from tests.unittest import HomeserverTestCase, TestCase +from tests.test_utils.event_injection import create_event logger = logging.getLogger(__name__) @@ -611,6 +612,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = self.hs.get_datastores().main self.event_sources = hs.get_event_sources() self.storage_controllers = hs.get_storage_controllers() + persistence = self.hs.get_storage_controllers().persistence + assert persistence is not None + self.persistence = persistence super().prepare(reactor, clock, hs) @@ -2818,71 +2822,45 @@ def test_state_reset(self) -> None: user2_tok = self.login(user2_id, "pass") # The room where the state reset will happen - room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + room_id1 = self.helper.create_room_as( + user2_id, + is_public=True, + tok=user2_tok, + ) + # Create a dummy event for us to point back to for the state reset + dummy_event_response = self.helper.send(room_id1, "test", tok=user2_tok) + dummy_event_id = dummy_event_response["event_id"] + + # Join after the dummy event join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) # Join another room so we don't hit the short-circuit and return early if they # have no room membership room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) - join_response2 = self.helper.join(room_id2, user1_id, tok=user1_tok) + self.helper.join(room_id2, user1_id, tok=user1_tok) before_reset_token = self.event_sources.get_current_token() - # Send another state event to make a position for the state reset to happen at - dummy_state_response = self.helper.send_state( - room_id1, - event_type="foobarbaz", - state_key="", - body={"foo": "bar"}, - tok=user2_tok, - ) - dummy_state_pos = self.get_success( - self.store.get_position_for_event(dummy_state_response["event_id"]) - ) - - # Mock a state reset removing the membership for user1 in the current state - self.get_success( - self.store.db_pool.simple_delete( - table="current_state_events", - keyvalues={ - "room_id": room_id1, - "type": EventTypes.Member, - "state_key": user1_id, - }, - desc="state reset user in current_state_events", + # Trigger a state reset + join_rule_event, join_rule_context = self.get_success( + create_event( + self.hs, + prev_event_ids=[dummy_event_id], + type=EventTypes.JoinRules, + state_key="", + content={"join_rule": JoinRules.INVITE}, + sender=user2_id, + room_id=room_id1, + room_version=self.get_success(self.store.get_room_version_id(room_id1)), ) ) - self.get_success( - self.store.db_pool.simple_delete( - table="local_current_membership", - keyvalues={ - "room_id": room_id1, - "user_id": user1_id, - }, - desc="state reset user in local_current_membership", - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - table="current_state_delta_stream", - values={ - "stream_id": dummy_state_pos.stream, - "room_id": room_id1, - "type": EventTypes.Member, - "state_key": user1_id, - "event_id": None, - "prev_event_id": join_response1["event_id"], - "instance_name": dummy_state_pos.instance_name, - }, - desc="state reset user in current_state_delta_stream", - ) + _, join_rule_event_pos, _ = self.get_success( + self.persistence.persist_event(join_rule_event, join_rule_context) ) - # Manually bust the cache since we we're just manually messing with the database - # and not causing an actual state reset. - self.store._membership_stream_cache.entity_has_changed( - user1_id, dummy_state_pos.stream - ) + # Ensure that the state reset worked and only user2 is in the room now + users_in_room = self.get_success(self.store.get_users_in_room(room_id1)) + self.assertIncludes(set(users_in_room), {user2_id}, exact=True) after_reset_token = self.event_sources.get_current_token() From 4ad96716a8b473ce8b51d96978b6cd859ffe4127 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 11:59:46 -0500 Subject: [PATCH 11/22] Align other test with real state reset --- tests/handlers/test_sliding_sync.py | 85 +++++++++++------------------ 1 file changed, 31 insertions(+), 54 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 4be470e24b6..c8625409fa2 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -51,8 +51,8 @@ from tests import unittest from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase -from tests.unittest import HomeserverTestCase, TestCase from tests.test_utils.event_injection import create_event +from tests.unittest import HomeserverTestCase, TestCase logger = logging.getLogger(__name__) @@ -3211,6 +3211,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = self.hs.get_datastores().main self.event_sources = hs.get_event_sources() self.storage_controllers = hs.get_storage_controllers() + persistence = self.hs.get_storage_controllers().persistence + assert persistence is not None + self.persistence = persistence def _get_sync_room_ids_for_user( self, @@ -3459,8 +3462,17 @@ def test_state_reset(self) -> None: user2_tok = self.login(user2_id, "pass") # The room where the state reset will happen - room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) - join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) + room_id1 = self.helper.create_room_as( + user2_id, + is_public=True, + tok=user2_tok, + ) + # Create a dummy event for us to point back to for the state reset + dummy_event_response = self.helper.send(room_id1, "test", tok=user2_tok) + dummy_event_id = dummy_event_response["event_id"] + + # Join after the dummy event + self.helper.join(room_id1, user1_id, tok=user1_tok) # Join another room so we don't hit the short-circuit and return early if they # have no room membership @@ -3469,61 +3481,26 @@ def test_state_reset(self) -> None: before_reset_token = self.event_sources.get_current_token() - # Send another state event to make a position for the state reset to happen at - dummy_state_response = self.helper.send_state( - room_id1, - event_type="foobarbaz", - state_key="", - body={"foo": "bar"}, - tok=user2_tok, - ) - dummy_state_pos = self.get_success( - self.store.get_position_for_event(dummy_state_response["event_id"]) - ) - - # Mock a state reset removing the membership for user1 in the current state - self.get_success( - self.store.db_pool.simple_delete( - table="current_state_events", - keyvalues={ - "room_id": room_id1, - "type": EventTypes.Member, - "state_key": user1_id, - }, - desc="state reset user in current_state_events", - ) - ) - self.get_success( - self.store.db_pool.simple_delete( - table="local_current_membership", - keyvalues={ - "room_id": room_id1, - "user_id": user1_id, - }, - desc="state reset user in local_current_membership", + # Trigger a state reset + join_rule_event, join_rule_context = self.get_success( + create_event( + self.hs, + prev_event_ids=[dummy_event_id], + type=EventTypes.JoinRules, + state_key="", + content={"join_rule": JoinRules.INVITE}, + sender=user2_id, + room_id=room_id1, + room_version=self.get_success(self.store.get_room_version_id(room_id1)), ) ) - self.get_success( - self.store.db_pool.simple_insert( - table="current_state_delta_stream", - values={ - "stream_id": dummy_state_pos.stream, - "room_id": room_id1, - "type": EventTypes.Member, - "state_key": user1_id, - "event_id": None, - "prev_event_id": join_response1["event_id"], - "instance_name": dummy_state_pos.instance_name, - }, - desc="state reset user in current_state_delta_stream", - ) + _, join_rule_event_pos, _ = self.get_success( + self.persistence.persist_event(join_rule_event, join_rule_context) ) - # Manually bust the cache since we we're just manually messing with the database - # and not causing an actual state reset. - self.store._membership_stream_cache.entity_has_changed( - user1_id, dummy_state_pos.stream - ) + # Ensure that the state reset worked and only user2 is in the room now + users_in_room = self.get_success(self.store.get_users_in_room(room_id1)) + self.assertIncludes(set(users_in_room), {user2_id}, exact=True) after_reset_token = self.event_sources.get_current_token() From 2fe8e355ceb4136788a15156a8e32709083095ed Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 13:16:16 -0500 Subject: [PATCH 12/22] Fix test in fallback path (more loose criteria) --- synapse/handlers/sliding_sync/room_lists.py | 5 +++++ tests/handlers/test_sliding_sync.py | 9 ++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index b26ce86f3cc..60ede2d6418 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -594,6 +594,11 @@ async def _compute_interested_rooms_fallback( ) = await self.get_room_membership_for_user_at_to_token( sync_config.user, to_token, from_token ) + logger.info( + "asdf room_membership_for_user_map: %s", room_membership_for_user_map + ) + logger.info("asdf newly_joined_room_ids: %s", newly_joined_room_ids) + logger.info("asdf newly_left_room_ids: %s", newly_left_room_ids) dm_room_ids = await self._get_dm_rooms_for_user(sync_config.user.to_string()) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index c8625409fa2..d8880bcd9df 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -1067,12 +1067,15 @@ def test_newly_left_rooms(self) -> None: newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - # `room_id1` should not show up because it was left before the token range. + # Ideally, `room_id1` should not show up because it was left before the token range. # `room_id2` should show up because it is `newly_left` within the token range. - self.assertEqual( + self.assertIncludes( room_id_results.keys(), {room_id2}, - "Corresponding map to disambiguate the opaque room IDs: " + # This isn't exact because the new vs fallback path include left rooms + # differently (`room_id1` differs) + exact=False, + message="Corresponding map to disambiguate the opaque room IDs: " + str( { "room_id1": room_id1, From e4b9d01b4c58b54963bac49cc66e335588505e88 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 13:39:17 -0500 Subject: [PATCH 13/22] Refactor to look at room ID's in actual list --- tests/handlers/test_sliding_sync.py | 397 ++++++++++++++++++---------- 1 file changed, 256 insertions(+), 141 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index d8880bcd9df..ee5ea767ded 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -646,9 +646,9 @@ def test_no_rooms(self) -> None: to_token=now_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) - self.assertEqual(room_id_results.keys(), set()) + self.assertIncludes(room_id_results, set(), exact=True) def test_get_newly_joined_room(self) -> None: """ @@ -686,18 +686,25 @@ def test_get_newly_joined_room(self) -> None: to_token=after_room_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id}) + self.assertIncludes( + room_id_results, + {room_id}, + exact=True, + ) # It should be pointing to the join event (latest membership event in the # from/to range) self.assertEqual( - room_id_results[room_id].event_id, + interested_rooms.room_membership_for_user_map[room_id].event_id, join_response["event_id"], ) - self.assertEqual(room_id_results[room_id].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id].membership, + Membership.JOIN, + ) # We should be considered `newly_joined` because we joined during the token # range self.assertTrue(room_id in newly_joined) @@ -736,18 +743,21 @@ def test_get_already_joined_room(self) -> None: to_token=after_room_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id}) + self.assertIncludes(room_id_results, {room_id}, exact=True) # It should be pointing to the join event (latest membership event in the # from/to range) self.assertEqual( - room_id_results[room_id].event_id, + interested_rooms.room_membership_for_user_map[room_id].event_id, join_response["event_id"], ) - self.assertEqual(room_id_results[room_id].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id not in newly_joined) self.assertTrue(room_id not in newly_left) @@ -825,13 +835,13 @@ def test_get_invited_banned_knocked_room(self) -> None: to_token=after_room_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Ensure that the invited, ban, and knock rooms show up self.assertEqual( - room_id_results.keys(), + room_id_results, { invited_room_id, ban_room_id, @@ -841,26 +851,35 @@ def test_get_invited_banned_knocked_room(self) -> None: # It should be pointing to the the respective membership event (latest # membership event in the from/to range) self.assertEqual( - room_id_results[invited_room_id].event_id, + interested_rooms.room_membership_for_user_map[invited_room_id].event_id, invite_response["event_id"], ) - self.assertEqual(room_id_results[invited_room_id].membership, Membership.INVITE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[invited_room_id].membership, + Membership.INVITE, + ) self.assertTrue(invited_room_id not in newly_joined) self.assertTrue(invited_room_id not in newly_left) self.assertEqual( - room_id_results[ban_room_id].event_id, + interested_rooms.room_membership_for_user_map[ban_room_id].event_id, ban_response["event_id"], ) - self.assertEqual(room_id_results[ban_room_id].membership, Membership.BAN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[ban_room_id].membership, + Membership.BAN, + ) self.assertTrue(ban_room_id not in newly_joined) self.assertTrue(ban_room_id not in newly_left) self.assertEqual( - room_id_results[knock_room_id].event_id, + interested_rooms.room_membership_for_user_map[knock_room_id].event_id, knock_room_membership_state_event.event_id, ) - self.assertEqual(room_id_results[knock_room_id].membership, Membership.KNOCK) + self.assertEqual( + interested_rooms.room_membership_for_user_map[knock_room_id].membership, + Membership.KNOCK, + ) self.assertTrue(knock_room_id not in newly_joined) self.assertTrue(knock_room_id not in newly_left) @@ -912,19 +931,24 @@ def test_get_kicked_room(self) -> None: to_token=after_kick_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # The kicked room should show up - self.assertEqual(room_id_results.keys(), {kick_room_id}) + self.assertIncludes(room_id_results, {kick_room_id}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[kick_room_id].event_id, + interested_rooms.room_membership_for_user_map[kick_room_id].event_id, kick_response["event_id"], ) - self.assertEqual(room_id_results[kick_room_id].membership, Membership.LEAVE) - self.assertNotEqual(room_id_results[kick_room_id].sender, user1_id) + self.assertEqual( + interested_rooms.room_membership_for_user_map[kick_room_id].membership, + Membership.LEAVE, + ) + self.assertNotEqual( + interested_rooms.room_membership_for_user_map[kick_room_id].sender, user1_id + ) # We should *NOT* be `newly_joined` because we were not joined at the the time # of the `to_token`. self.assertTrue(kick_room_id not in newly_joined) @@ -1020,10 +1044,10 @@ def test_forgotten_rooms(self) -> None: to_token=before_room_forgets, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) # We shouldn't see the room because it was forgotten - self.assertEqual(room_id_results.keys(), set()) + self.assertIncludes(room_id_results, set(), exact=True) def test_newly_left_rooms(self) -> None: """ @@ -1063,18 +1087,16 @@ def test_newly_left_rooms(self) -> None: to_token=after_room2_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - # Ideally, `room_id1` should not show up because it was left before the token range. + # `room_id1` should not show up because it was left before the token range. # `room_id2` should show up because it is `newly_left` within the token range. self.assertIncludes( - room_id_results.keys(), + room_id_results, {room_id2}, - # This isn't exact because the new vs fallback path include left rooms - # differently (`room_id1` differs) - exact=False, + exact=True, message="Corresponding map to disambiguate the opaque room IDs: " + str( { @@ -1085,10 +1107,13 @@ def test_newly_left_rooms(self) -> None: ) self.assertEqual( - room_id_results[room_id2].event_id, + interested_rooms.room_membership_for_user_map[room_id2].event_id, leave_response2["event_id"], ) - self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id2].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined` because we are instead `newly_left` self.assertTrue(room_id2 not in newly_joined) self.assertTrue(room_id2 in newly_left) @@ -1133,17 +1158,20 @@ def test_no_joins_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response1["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -1188,16 +1216,16 @@ def test_join_during_range_and_left_room_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # We should still see the room because we were joined during the # from_token/to_token time period. - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1207,7 +1235,10 @@ def test_join_during_range_and_left_room_after_to_token(self) -> None: } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -1250,15 +1281,15 @@ def test_join_before_range_and_left_room_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # We should still see the room because we were joined before the `from_token` - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1268,7 +1299,10 @@ def test_join_before_range_and_left_room_after_to_token(self) -> None: } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -1329,15 +1363,15 @@ def test_kicked_before_range_and_left_after_to_token(self) -> None: to_token=after_kick_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # We shouldn't see the room because it was forgotten - self.assertEqual(room_id_results.keys(), {kick_room_id}) + self.assertIncludes(room_id_results, {kick_room_id}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[kick_room_id].event_id, + interested_rooms.room_membership_for_user_map[kick_room_id].event_id, kick_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1349,8 +1383,13 @@ def test_kicked_before_range_and_left_after_to_token(self) -> None: } ), ) - self.assertEqual(room_id_results[kick_room_id].membership, Membership.LEAVE) - self.assertNotEqual(room_id_results[kick_room_id].sender, user1_id) + self.assertEqual( + interested_rooms.room_membership_for_user_map[kick_room_id].membership, + Membership.LEAVE, + ) + self.assertNotEqual( + interested_rooms.room_membership_for_user_map[kick_room_id].sender, user1_id + ) # We should *NOT* be `newly_joined` because we were kicked self.assertTrue(kick_room_id not in newly_joined) self.assertTrue(kick_room_id not in newly_left) @@ -1400,15 +1439,15 @@ def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should still show up because it's newly_left during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, leave_response1["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1420,7 +1459,10 @@ def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined` because we are actually `newly_left` during # the token range self.assertTrue(room_id1 not in newly_joined) @@ -1470,15 +1512,15 @@ def test_newly_left_during_range_and_join_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should still show up because it's newly_left during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, leave_response1["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1489,7 +1531,10 @@ def test_newly_left_during_range_and_join_after_to_token(self) -> None: } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined` because we are actually `newly_left` during # the token range self.assertTrue(room_id1 not in newly_joined) @@ -1544,20 +1589,23 @@ def test_no_from_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Only rooms we were joined to before the `to_token` should show up - self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + self.assertIncludes(room_id_results, {room_id1, room_id2}, exact=True) # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response1["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined`/`newly_left` because there is no # `from_token` to define a "live" range to compare against self.assertTrue(room_id1 not in newly_joined) @@ -1566,10 +1614,13 @@ def test_no_from_token(self) -> None: # Room2 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id2].event_id, + interested_rooms.room_membership_for_user_map[room_id2].event_id, leave_response2["event_id"], ) - self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id2].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined`/`newly_left` because there is no # `from_token` to define a "live" range to compare against self.assertTrue(room_id2 not in newly_joined) @@ -1641,14 +1692,14 @@ def test_from_token_ahead_of_to_token(self) -> None: to_token=to_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # In the "current" state snapshot, we're joined to all of the rooms but in the # from/to token range... self.assertIncludes( - room_id_results.keys(), + room_id_results, { # Included because we were joined before both tokens room_id1, @@ -1665,10 +1716,13 @@ def test_from_token_ahead_of_to_token(self) -> None: # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_room1_response1["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined`/`newly_left` because we joined `room1` # before either of the tokens self.assertTrue(room_id1 not in newly_joined) @@ -1677,10 +1731,13 @@ def test_from_token_ahead_of_to_token(self) -> None: # Room2 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id2].event_id, + interested_rooms.room_membership_for_user_map[room_id2].event_id, leave_room2_response1["event_id"], ) - self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id2].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined`/`newly_left` because we joined and left # `room1` before either of the tokens self.assertTrue(room_id2 not in newly_joined) @@ -1729,17 +1786,20 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, leave_response["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined`/`newly_left` because we joined and left # `room1` before either of the tokens self.assertTrue(room_id1 not in newly_joined) @@ -1787,17 +1847,20 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, leave_response["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined`/`newly_left` because we joined and left # `room1` before either of the tokens self.assertTrue(room_id1 not in newly_joined) @@ -1852,15 +1915,15 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because it was newly_left and joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response2["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1874,7 +1937,10 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) # We should *NOT* be `newly_left` because we joined during the token range and @@ -1929,15 +1995,15 @@ def test_join_leave_multiple_times_before_range_and_after_to_token( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined before the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response2["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -1951,7 +2017,10 @@ def test_join_leave_multiple_times_before_range_and_after_to_token( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2003,15 +2072,15 @@ def test_invite_before_range_and_join_leave_after_to_token( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were invited before the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, invite_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2022,7 +2091,10 @@ def test_invite_before_range_and_join_leave_after_to_token( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.INVITE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.INVITE, + ) # We should *NOT* be `newly_joined` because we were only invited before the # token range self.assertTrue(room_id1 not in newly_joined) @@ -2092,15 +2164,15 @@ def test_join_and_display_name_changes_in_token_range( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_during_token_range_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2115,7 +2187,10 @@ def test_join_and_display_name_changes_in_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2172,15 +2247,15 @@ def test_display_name_changes_in_token_range( to_token=after_change1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_during_token_range_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2192,7 +2267,10 @@ def test_display_name_changes_in_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2259,15 +2337,15 @@ def test_display_name_changes_before_and_after_token_range( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined before the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_before_token_range_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2282,7 +2360,10 @@ def test_display_name_changes_before_and_after_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2358,15 +2439,15 @@ def test_newly_joined_display_name_changes_leave_after_token_range( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_during_token_range_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2381,7 +2462,10 @@ def test_newly_joined_display_name_changes_leave_after_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2458,15 +2542,15 @@ def test_display_name_changes_leave_after_token_range( to_token=after_display_name_change_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_during_token_range_response["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2481,7 +2565,10 @@ def test_display_name_changes_leave_after_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We only changed our display name during the token range so we shouldn't be # considered `newly_joined` or `newly_left` self.assertTrue(room_id1 not in newly_joined) @@ -2542,10 +2629,10 @@ def test_display_name_changes_join_after_token_range( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) # Room shouldn't show up because we joined after the from/to range - self.assertEqual(room_id_results.keys(), set()) + self.assertIncludes(room_id_results, set(), exact=True) def test_newly_joined_with_leave_join_in_token_range( self, @@ -2591,18 +2678,21 @@ def test_newly_joined_with_leave_join_in_token_range( to_token=after_more_changes_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because we were joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_response2["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be considered `newly_joined` because there is some non-join event in # between our latest join event. self.assertTrue(room_id1 in newly_joined) @@ -2671,15 +2761,15 @@ def test_newly_joined_only_joins_during_token_range( to_token=after_room1_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room should show up because it was newly_left and joined during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, displayname_change_during_token_range_response2["event_id"], "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2694,7 +2784,10 @@ def test_newly_joined_only_joins_during_token_range( } ), ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we first joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -2762,12 +2855,12 @@ def test_multiple_rooms_are_not_confused( to_token=after_room3_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms self.assertEqual( - room_id_results.keys(), + room_id_results, { # Left before the from/to range room_id1, @@ -2781,10 +2874,13 @@ def test_multiple_rooms_are_not_confused( # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, leave_room1_response["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined`/`newly_left` because we were invited and left # before the token range self.assertTrue(room_id1 not in newly_joined) @@ -2793,10 +2889,13 @@ def test_multiple_rooms_are_not_confused( # Room2 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id2].event_id, + interested_rooms.room_membership_for_user_map[room_id2].event_id, invite_room2_response["event_id"], ) - self.assertEqual(room_id_results[room_id2].membership, Membership.INVITE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id2].membership, + Membership.INVITE, + ) # We should *NOT* be `newly_joined`/`newly_left` because we were invited before # the token range self.assertTrue(room_id2 not in newly_joined) @@ -2805,10 +2904,13 @@ def test_multiple_rooms_are_not_confused( # Room3 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id3].event_id, + interested_rooms.room_membership_for_user_map[room_id3].event_id, leave_room3_response["event_id"], ) - self.assertEqual(room_id_results[room_id3].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id3].membership, + Membership.LEAVE, + ) # We should be `newly_left` because we were invited and left during # the token range self.assertTrue(room_id3 not in newly_joined) @@ -2887,16 +2989,16 @@ def test_state_reset(self) -> None: to_token=after_reset_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms # Room1 should show up because it was `newly_left` via state reset during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + self.assertIncludes(room_id_results, {room_id1, room_id2}, exact=True) # It should be pointing to no event because we were removed from the room # without a corresponding leave event self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, None, "Corresponding map to disambiguate the opaque event IDs: " + str( @@ -2906,7 +3008,10 @@ def test_state_reset(self) -> None: ), ) # State reset caused us to leave the room and there is no corresponding leave event - self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.LEAVE, + ) # We should *NOT* be `newly_joined` because we joined before the token range self.assertTrue(room_id1 not in newly_joined) # We should be `newly_left` because we were removed via state reset during the from/to range @@ -3138,12 +3243,12 @@ def test_sharded_event_persisters(self) -> None: to_token=stuck_activity_token, ) ) - room_id_results = interested_rooms.room_membership_for_user_map + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms self.assertEqual( - room_id_results.keys(), + room_id_results, { room_id1, room_id2, @@ -3154,10 +3259,13 @@ def test_sharded_event_persisters(self) -> None: # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id1].event_id, + interested_rooms.room_membership_for_user_map[room_id1].event_id, join_room1_response["event_id"], ) - self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id1].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) @@ -3165,10 +3273,13 @@ def test_sharded_event_persisters(self) -> None: # Room2 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id2].event_id, + interested_rooms.room_membership_for_user_map[room_id2].event_id, leave_room2_response["event_id"], ) - self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id2].membership, + Membership.LEAVE, + ) # room_id2 should *NOT* be considered `newly_left` because we left before the # from/to range and the join event during the range happened while worker2 was # stuck. This means that from the perspective of the master, where the @@ -3181,10 +3292,13 @@ def test_sharded_event_persisters(self) -> None: # Room3 # It should be pointing to the latest membership event in the from/to range self.assertEqual( - room_id_results[room_id3].event_id, + interested_rooms.room_membership_for_user_map[room_id3].event_id, join_on_worker3_response["event_id"], ) - self.assertEqual(room_id_results[room_id3].membership, Membership.JOIN) + self.assertEqual( + interested_rooms.room_membership_for_user_map[room_id3].membership, + Membership.JOIN, + ) # We should be `newly_joined` because we joined during the token range self.assertTrue(room_id3 in newly_joined) self.assertTrue(room_id3 not in newly_left) @@ -3325,7 +3439,7 @@ def test_basic_rooms(self) -> None: ) # Ensure that the invited, ban, and knock rooms show up - self.assertEqual( + self.assertIncludes( room_id_results.keys(), { join_room_id, @@ -3333,6 +3447,7 @@ def test_basic_rooms(self) -> None: ban_room_id, knock_room_id, }, + exact=True, ) # It should be pointing to the the respective membership event (latest # membership event in the from/to range) From 2e2b8bf36dc1678696fa2391ff768d0ac3e1e219 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:07:29 -0500 Subject: [PATCH 14/22] Fix changes after token showing up in new path --- synapse/handlers/sliding_sync/room_lists.py | 64 ++++++++++++++++----- tests/handlers/test_sliding_sync.py | 38 ++---------- 2 files changed, 54 insertions(+), 48 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 60ede2d6418..862d9f8005b 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -269,9 +269,17 @@ async def _compute_interested_rooms_new_tables( "asdf room_membership_for_user_map %s", room_membership_for_user_map ) + ( + 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 ) + logger.info("asdf rewind changes %s", changes) if changes: # TODO: It would be nice to avoid these copies room_membership_for_user_map = dict(room_membership_for_user_map) @@ -284,7 +292,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, @@ -296,14 +304,14 @@ 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) logger.info("asdf newly_joined_room_ids: %s", newly_joined_room_ids) logger.info("asdf newly_left_room_map: %s", newly_left_room_map) @@ -341,14 +349,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, @@ -360,6 +375,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 @@ -381,7 +404,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, @@ -392,6 +415,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 @@ -426,6 +459,9 @@ async def _compute_interested_rooms_new_tables( if room_id not in partial_state_rooms } + logger.info( + "asdf filtered_sync_room_map %s", filtered_sync_room_map + ) all_rooms.update(filtered_sync_room_map) ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index ee5ea767ded..8decfb64554 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -1563,7 +1563,7 @@ def test_no_from_token(self) -> None: # Join and leave the room2 before the `to_token` self.helper.join(room_id2, user1_id, tok=user1_tok) - leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) + _leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1594,7 +1594,7 @@ def test_no_from_token(self) -> None: newly_left = interested_rooms.newly_left_rooms # Only rooms we were joined to before the `to_token` should show up - self.assertIncludes(room_id_results, {room_id1, room_id2}, exact=True) + self.assertIncludes(room_id_results, {room_id1}, exact=True) # Room1 # It should be pointing to the latest membership event in the from/to range @@ -1611,21 +1611,6 @@ def test_no_from_token(self) -> None: self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) - # Room2 - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].event_id, - leave_response2["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].membership, - Membership.LEAVE, - ) - # We should *NOT* be `newly_joined`/`newly_left` because there is no - # `from_token` to define a "live" range to compare against - self.assertTrue(room_id2 not in newly_joined) - self.assertTrue(room_id2 not in newly_left) - def test_from_token_ahead_of_to_token(self) -> None: """ Test when the provided `from_token` comes after the `to_token`. We should @@ -1821,7 +1806,7 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) # Join and leave the room before the from/to range self.helper.join(room_id1, user1_id, tok=user1_tok) - leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) + self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1848,23 +1833,8 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: ) ) room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) - newly_joined = interested_rooms.newly_joined_rooms - newly_left = interested_rooms.newly_left_rooms - self.assertIncludes(room_id_results, {room_id1}, exact=True) - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].event_id, - leave_response["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].membership, - Membership.LEAVE, - ) - # We should *NOT* be `newly_joined`/`newly_left` because we joined and left - # `room1` before either of the tokens - self.assertTrue(room_id1 not in newly_joined) - self.assertTrue(room_id1 not in newly_left) + self.assertIncludes(room_id_results, set(), exact=True) def test_join_leave_multiple_times_during_range_and_after_to_token( self, From 94efd8b9ff7cda628e3734a68b986ed0b1c6c6c3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:16:32 -0500 Subject: [PATCH 15/22] Fix tests --- tests/handlers/test_sliding_sync.py | 69 +++++++---------------------- 1 file changed, 17 insertions(+), 52 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 8decfb64554..f5c3eec44df 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -1688,14 +1688,23 @@ def test_from_token_ahead_of_to_token(self) -> None: { # Included because we were joined before both tokens room_id1, - # Included because we had membership before the to_token - room_id2, + # Excluded because we left before the `from_token` and `to_token` + # room_id2, # Excluded because we joined after the `to_token` # room_id3, # Excluded because we joined after the `to_token` # room_id4, }, exact=True, + message="Corresponding map to disambiguate the opaque room IDs: " + + str( + { + "room_id1": room_id1, + "room_id2": room_id2, + "room_id3": room_id3, + "room_id4": room_id4, + } + ), ) # Room1 @@ -1713,21 +1722,6 @@ def test_from_token_ahead_of_to_token(self) -> None: self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) - # Room2 - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].event_id, - leave_room2_response1["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].membership, - Membership.LEAVE, - ) - # We should *NOT* be `newly_joined`/`newly_left` because we joined and left - # `room1` before either of the tokens - self.assertTrue(room_id2 not in newly_joined) - self.assertTrue(room_id2 not in newly_left) - def test_leave_before_range_and_join_leave_after_to_token(self) -> None: """ Test old left rooms. But we're also testing that joining and leaving after the @@ -1744,7 +1738,7 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) # Join and leave the room before the from/to range self.helper.join(room_id1, user1_id, tok=user1_tok) - leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) + self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1772,23 +1766,8 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: ) ) room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) - newly_joined = interested_rooms.newly_joined_rooms - newly_left = interested_rooms.newly_left_rooms - self.assertIncludes(room_id_results, {room_id1}, exact=True) - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].event_id, - leave_response["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].membership, - Membership.LEAVE, - ) - # We should *NOT* be `newly_joined`/`newly_left` because we joined and left - # `room1` before either of the tokens - self.assertTrue(room_id1 not in newly_joined) - self.assertTrue(room_id1 not in newly_left) + self.assertIncludes(room_id_results, set(), exact=True) def test_leave_before_range_and_join_after_to_token(self) -> None: """ @@ -2829,33 +2808,19 @@ def test_multiple_rooms_are_not_confused( newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual( + self.assertIncludes( room_id_results, { - # Left before the from/to range - room_id1, + # Excluded because we left before the from/to range + # room_id1, # Invited before the from/to range room_id2, # `newly_left` during the from/to range room_id3, }, + exact=True, ) - # Room1 - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].event_id, - leave_room1_response["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id1].membership, - Membership.LEAVE, - ) - # We should *NOT* be `newly_joined`/`newly_left` because we were invited and left - # before the token range - self.assertTrue(room_id1 not in newly_joined) - self.assertTrue(room_id1 not in newly_left) - # Room2 # It should be pointing to the latest membership event in the from/to range self.assertEqual( From 59b9cffc503151401e2a23ffb7f36ee0536e890b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:26:52 -0500 Subject: [PATCH 16/22] Fix sharded event persisting test case --- tests/handlers/test_sliding_sync.py | 41 ++++++++++++----------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index f5c3eec44df..1c76ea17d5e 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -1633,7 +1633,7 @@ def test_from_token_ahead_of_to_token(self) -> None: # Join and leave the room2 before `to_token` _join_room2_response1 = self.helper.join(room_id2, user1_id, tok=user1_tok) - leave_room2_response1 = self.helper.leave(room_id2, user1_id, tok=user1_tok) + _leave_room2_response1 = self.helper.leave(room_id2, user1_id, tok=user1_tok) # Note: These are purposely swapped. The `from_token` should come after # the `to_token` in this test @@ -2762,7 +2762,7 @@ def test_multiple_rooms_are_not_confused( # Invited and left the room before the token self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) - leave_room1_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) + _leave_room1_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) # Invited to room2 invite_room2_response = self.helper.invite( room_id2, src=user2_id, targ=user1_id, tok=user2_tok @@ -3069,7 +3069,7 @@ def test_sharded_event_persisters(self) -> None: join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) join_response2 = self.helper.join(room_id2, user1_id, tok=user1_tok) # Leave room2 - leave_room2_response = self.helper.leave(room_id2, user1_id, tok=user1_tok) + _leave_room2_response = self.helper.leave(room_id2, user1_id, tok=user1_tok) join_response3 = self.helper.join(room_id3, user1_id, tok=user1_tok) # Leave room3 self.helper.leave(room_id3, user1_id, tok=user1_tok) @@ -3182,13 +3182,25 @@ def test_sharded_event_persisters(self) -> None: newly_joined = interested_rooms.newly_joined_rooms newly_left = interested_rooms.newly_left_rooms - self.assertEqual( + self.assertIncludes( room_id_results, { room_id1, - room_id2, + # Excluded because we left before the from/to range and the second join + # event happened while worker2 was stuck and technically occurs after + # the `stuck_activity_token`. + # room_id2, room_id3, }, + exact=True, + message="Corresponding map to disambiguate the opaque room IDs: " + + str( + { + "room_id1": room_id1, + "room_id2": room_id2, + "room_id3": room_id3, + } + ), ) # Room1 @@ -3205,25 +3217,6 @@ def test_sharded_event_persisters(self) -> None: self.assertTrue(room_id1 in newly_joined) self.assertTrue(room_id1 not in newly_left) - # Room2 - # It should be pointing to the latest membership event in the from/to range - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].event_id, - leave_room2_response["event_id"], - ) - self.assertEqual( - interested_rooms.room_membership_for_user_map[room_id2].membership, - Membership.LEAVE, - ) - # room_id2 should *NOT* be considered `newly_left` because we left before the - # from/to range and the join event during the range happened while worker2 was - # stuck. This means that from the perspective of the master, where the - # `stuck_activity_token` is generated, the stream position for worker2 wasn't - # advanced to the join yet. Looking at the `instance_map`, the join technically - # comes after `stuck_activity_token`. - self.assertTrue(room_id2 not in newly_joined) - self.assertTrue(room_id2 not in newly_left) - # Room3 # It should be pointing to the latest membership event in the from/to range self.assertEqual( From 5fadd6169e61d690d0ea4f62cec153b4bed69207 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:30:42 -0500 Subject: [PATCH 17/22] Align more tests to use `assertIncludes` --- tests/handlers/test_sliding_sync.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 1c76ea17d5e..cbacf21ae78 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -840,13 +840,14 @@ def test_get_invited_banned_knocked_room(self) -> None: newly_left = interested_rooms.newly_left_rooms # Ensure that the invited, ban, and knock rooms show up - self.assertEqual( + self.assertIncludes( room_id_results, { invited_room_id, ban_room_id, knock_room_id, }, + exact=True, ) # It should be pointing to the the respective membership event (latest # membership event in the from/to range) @@ -3301,7 +3302,7 @@ def test_no_rooms(self) -> None: to_token=now_token, ) - self.assertEqual(room_id_results.keys(), set()) + self.assertIncludes(room_id_results.keys(), set(), exact=True) def test_basic_rooms(self) -> None: """ @@ -3439,7 +3440,7 @@ def test_only_newly_left_rooms_show_up(self) -> None: ) # Only the `newly_left` room should show up - self.assertEqual(room_id_results.keys(), {room_id2}) + self.assertIncludes(room_id_results.keys(), {room_id2}, exact=True) self.assertEqual( room_id_results[room_id2].event_id, _leave_response2["event_id"], @@ -3484,7 +3485,7 @@ def test_get_kicked_room(self) -> None: ) # The kicked room should show up - self.assertEqual(room_id_results.keys(), {kick_room_id}) + self.assertIncludes(room_id_results.keys(), {kick_room_id}, exact=True) # It should be pointing to the latest membership event in the from/to range self.assertEqual( room_id_results[kick_room_id].event_id, @@ -3558,7 +3559,7 @@ def test_state_reset(self) -> None: ) # Room1 should show up because it was `newly_left` via state reset during the from/to range - self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + self.assertIncludes(room_id_results.keys(), {room_id1, room_id2}, exact=True) # It should be pointing to no event because we were removed from the room # without a corresponding leave event self.assertEqual( From 2e520f530c1b5b7f42ef4f0c30374906ab040e54 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:32:12 -0500 Subject: [PATCH 18/22] Remove debug logs --- synapse/handlers/sliding_sync/room_lists.py | 21 -------------------- synapse/storage/databases/main/roommember.py | 10 ---------- 2 files changed, 31 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 862d9f8005b..6709c753390 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -265,10 +265,6 @@ async def _compute_interested_rooms_new_tables( ): room_membership_for_user_map.pop(room_id, None) - logger.info( - "asdf room_membership_for_user_map %s", room_membership_for_user_map - ) - ( newly_joined_room_ids, newly_left_room_map, @@ -279,7 +275,6 @@ async def _compute_interested_rooms_new_tables( changes = await self._get_rewind_changes_to_current_membership_to_token( sync_config.user, room_membership_for_user_map, to_token=to_token ) - logger.info("asdf rewind changes %s", changes) if changes: # TODO: It would be nice to avoid these copies room_membership_for_user_map = dict(room_membership_for_user_map) @@ -313,9 +308,6 @@ async def _compute_interested_rooms_new_tables( else: room_membership_for_user_map.pop(room_id, None) - logger.info("asdf newly_joined_room_ids: %s", newly_joined_room_ids) - logger.info("asdf newly_left_room_map: %s", newly_left_room_map) - # Add back `newly_left` rooms (rooms left in the from -> to token range). # # We do this because `get_sliding_sync_rooms_for_user_from_membership_snapshots(...)` doesn't include @@ -326,7 +318,6 @@ async def _compute_interested_rooms_new_tables( missing_newly_left_rooms = ( newly_left_room_map.keys() - room_membership_for_user_map.keys() ) - logger.info("asdf missing_newly_left_rooms: %s", missing_newly_left_rooms) if missing_newly_left_rooms: # TODO: It would be nice to avoid these copies room_membership_for_user_map = dict(room_membership_for_user_map) @@ -342,10 +333,6 @@ async def _compute_interested_rooms_new_tables( newly_left_room_for_user_sliding_sync = ( await self.store.get_sliding_sync_room_for_user(user_id, room_id) ) - logger.info( - "asdf newly_left_room_for_user_sliding_sync: %s", - newly_left_room_for_user_sliding_sync, - ) # 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: @@ -459,9 +446,6 @@ async def _compute_interested_rooms_new_tables( if room_id not in partial_state_rooms } - logger.info( - "asdf filtered_sync_room_map %s", filtered_sync_room_map - ) all_rooms.update(filtered_sync_room_map) ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] @@ -630,11 +614,6 @@ async def _compute_interested_rooms_fallback( ) = await self.get_room_membership_for_user_at_to_token( sync_config.user, to_token, from_token ) - logger.info( - "asdf room_membership_for_user_map: %s", room_membership_for_user_map - ) - logger.info("asdf newly_joined_room_ids: %s", newly_joined_room_ids) - logger.info("asdf newly_left_room_ids: %s", newly_left_room_ids) dm_room_ids = await self._get_dm_rooms_for_user(sync_config.user.to_string()) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 4ddb98024da..d8e95e9cc62 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -1465,15 +1465,6 @@ def _txn( # `sliding_sync_joined_rooms` or `forgotten`), make sure to bust the # `get_sliding_sync_rooms_for_user_from_membership_snapshots` cache in the # appropriate places (and add tests). - - sql = """ - SELECT * FROM sliding_sync_membership_snapshots - WHERE user_id = ? - AND forgotten = 0 - """ - txn.execute(sql, (user_id,)) - logger.info("asdf raw sliding_sync_membership_snapshots %s", txn.fetchall()) - sql = """ SELECT m.room_id, m.sender, m.membership, m.membership_event_id, r.room_version, @@ -1491,7 +1482,6 @@ def _txn( OR (m.event_stream_ordering > ?) ) """ - logger.info("asdf to_token %s", to_token_self_leave_hint) # 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 to return From addb43c66b6bc3fcf1a8bdd25da6552477580478 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 14:51:21 -0500 Subject: [PATCH 19/22] Better changelog --- changelog.d/18399.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/18399.misc b/changelog.d/18399.misc index 07d3d5a8f72..847dc9a2b1e 100644 --- a/changelog.d/18399.misc +++ b/changelog.d/18399.misc @@ -1 +1 @@ -Convert tests to use compute_interested_rooms. +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. From dd4104cabe3aceed5760ca1998ba706b575d3d6c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 15:59:03 -0500 Subject: [PATCH 20/22] Fix cache messing up our self-leave after `to_token` fetching See https://github.com/element-hq/synapse/pull/18399#discussion_r2076177350 --- synapse/handlers/sliding_sync/room_lists.py | 42 +++-- synapse/storage/databases/main/roommember.py | 152 ++++++++++++++----- synapse/storage/databases/main/stream.py | 2 + 3 files changed, 149 insertions(+), 47 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 6709c753390..bf525d4ef23 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -241,20 +241,44 @@ async def _compute_interested_rooms_new_tables( # include rooms that are outside the list ranges. all_rooms: Set[str] = set() - # Note: this won't include rooms the user has left themselves (for the most - # part). We add back `newly_left` rooms below. This is more efficient than - # fetching all rooms and then filtering out the old left rooms. + # 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_from_membership_snapshots( + user_id + ) + ) + # 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. + room_membership_for_user_map = dict(room_membership_for_user_map) + # 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. + room_membership_for_user_map.update( + await self.store.get_sliding_sync_self_leave_rooms_after_to_token( user_id, to_token ) ) + logger.info( + "asdf room_membership_for_user_map: %s", 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 - 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 for room_id in list(room_membership_for_user_map.keys()): @@ -275,9 +299,8 @@ async def _compute_interested_rooms_new_tables( changes = await self._get_rewind_changes_to_current_membership_to_token( sync_config.user, room_membership_for_user_map, to_token=to_token ) + logger.info("asdf rewind changes: %s", changes) if changes: - # TODO: It would be nice to avoid these copies - room_membership_for_user_map = dict(room_membership_for_user_map) for room_id, change in changes.items(): if change is None: # Remove rooms that the user joined after the `to_token` @@ -319,8 +342,6 @@ 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 - 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] # This should be a given @@ -527,9 +548,6 @@ 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 - room_membership_for_user_map = dict(room_membership_for_user_map) - # Find which rooms are partially stated and may need to be filtered out # depending on the `required_state` requested (see below). partial_state_rooms = await self.store.get_partial_rooms() diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index d8e95e9cc62..f5adc1ea906 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -53,6 +53,7 @@ ) from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.databases.main.events_worker import EventsWorkerStore +from synapse.storage.databases.main.stream import _filter_results_by_stream from synapse.storage.engines import Sqlite3Engine from synapse.storage.roommember import ( MemberSummary, @@ -1425,34 +1426,17 @@ async def update_room_forgetter_stream_pos(self, stream_id: int) -> None: @cached(iterable=True, max_entries=10000) async def get_sliding_sync_rooms_for_user_from_membership_snapshots( - self, - user_id: str, - to_token_self_leave_hint: StreamToken, + self, user_id: str ) -> Mapping[str, RoomsForUserSlidingSync]: """ Get all the rooms for a user to handle a sliding sync request from the `sliding_sync_membership_snapshots` table. These will be current memberships and need to be rewound to the token range. - Ignores forgotten rooms. - - Also ignores rooms that the user has left themselves unless [1] as it's more - efficient to add them back here than to fetch all rooms and then filter out the - old left rooms. - - [1]: We ignore rooms that the user has left themselves unless the user left - *after* the token range (the token range is > `from_token` and <= `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 to return here and apply - our rewind logic (outside of this function). + Ignores forgotten rooms and rooms that the user has left themselves. Args: user_id: The user ID to get the rooms for. - to_token_self_leave_hint: This should be the `to_token` from the sync - requests. We do not filter out or rewind results to this token. It's simply - a hint, to return any self-leave membership if they're after this token (see - why above). Returns: Map from room ID to membership info @@ -1461,6 +1445,13 @@ async def get_sliding_sync_rooms_for_user_from_membership_snapshots( def _txn( txn: LoggingTransaction, ) -> Dict[str, RoomsForUserSlidingSync]: + sql = """ + SELECT room_id, membership, forgotten, event_stream_ordering FROM sliding_sync_membership_snapshots + WHERE user_id = ? + """ + txn.execute(sql, (user_id,)) + logger.info("asdf raw sliding_sync_membership_snapshots %s", txn.fetchall()) + # XXX: If you use any new columns that can change (like from # `sliding_sync_joined_rooms` or `forgotten`), make sure to bust the # `get_sliding_sync_rooms_for_user_from_membership_snapshots` cache in the @@ -1477,23 +1468,9 @@ def _txn( LEFT JOIN sliding_sync_joined_rooms AS j ON (j.room_id = m.room_id AND m.membership = 'join') WHERE user_id = ? AND m.forgotten = 0 - AND ( - (m.membership != 'leave' OR m.user_id != m.sender) - OR (m.event_stream_ordering > ?) - ) + AND (m.membership != 'leave' OR m.user_id != m.sender) """ - # 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 to return - # here and apply our rewind logic (outside of this function). - # - # We use the min token position to ensure we don't miss any updates. With a - # naive stream_ordering comparison, we might catch a few membership updates - # that are within the token range but then they are relevant to sync anyway - # from being with in the token range so we don't need to to any - # post-filtering after we get the results back from the database. - min_to_token_position = to_token_self_leave_hint.room_key.stream - txn.execute(sql, (user_id, min_to_token_position)) + txn.execute(sql, (user_id,)) return { row[0]: RoomsForUserSlidingSync( @@ -1519,6 +1496,111 @@ def _txn( _txn, ) + async def get_sliding_sync_self_leave_rooms_after_to_token( + self, + user_id: str, + to_token: StreamToken, + ) -> Dict[str, RoomsForUserSlidingSync]: + """ + Get all the self-leave rooms for a user after the `to_token` (outside the token + range) that are potentially relevant[1] and needed to handle a sliding sync + request. The results are from the `sliding_sync_membership_snapshots` table and + will be current memberships and need to be rewound to the token range. + + [1] 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. + + This is basically a sister-function to + `get_sliding_sync_rooms_for_user_from_membership_snapshots`. We could + alternatively incorporate this logic into + `get_sliding_sync_rooms_for_user_from_membership_snapshots` but 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. + + Args: + user_id: The user ID to get the rooms for. + to_token: Any self-leave memberships after this position will be returned. + + Returns: + Map from room ID to membership info + """ + # TODO: Potential to check + # `self._membership_stream_cache.has_entity_changed(...)` as an early-return + # shortcut. + + def _txn( + txn: LoggingTransaction, + ) -> Dict[str, RoomsForUserSlidingSync]: + sql = """ + SELECT m.room_id, m.sender, m.membership, m.membership_event_id, + r.room_version, + m.event_instance_name, m.event_stream_ordering, + m.has_known_state, + m.room_type, + m.is_encrypted + FROM sliding_sync_membership_snapshots AS m + INNER JOIN rooms AS r USING (room_id) + WHERE user_id = ? + AND m.forgotten = 0 + AND m.membership == 'leave' + AND m.user_id == m.sender + AND (m.event_stream_ordering > ?) + """ + # 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 handle tokens with a non-empty instance_map we fetch more + # results than necessary and then filter down + min_to_token_position = to_token.room_key.stream + txn.execute(sql, (user_id, min_to_token_position)) + + # Map from room_id to membership info + room_membership_for_user_map: Dict[str, RoomsForUserSlidingSync] = {} + for row in txn: + room_for_user = RoomsForUserSlidingSync( + room_id=row[0], + sender=row[1], + membership=row[2], + event_id=row[3], + room_version_id=row[4], + event_pos=PersistedEventPosition(row[5], row[6]), + has_known_state=bool(row[7]), + room_type=row[8], + is_encrypted=bool(row[9]), + ) + + # We filter out unknown room versions proactively. They shouldn't go + # down sync and their metadata may be in a broken state (causing + # errors). + if row[4] in KNOWN_ROOM_VERSIONS: + continue + + # We only want to include the self-leave membership if it happened after + # the token range. + # + # Since the database pulls out more than necessary, we need to filter it + # down here. + if not _filter_results_by_stream( + lower_token=None, + upper_token=to_token.room_key, + instance_name=room_for_user.event_pos.instance_name, + stream_ordering=room_for_user.event_pos.stream, + ): + continue + + room_membership_for_user_map[room_for_user.room_id] = room_for_user + + return room_membership_for_user_map + + return await self.db_pool.runInteraction( + "get_sliding_sync_self_leave_rooms_after_to_token", + _txn, + ) + async def get_sliding_sync_room_for_user( self, user_id: str, room_id: str ) -> Optional[RoomsForUserSlidingSync]: diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 00e52086743..c52389b8a97 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -453,6 +453,8 @@ def _filter_results_by_stream( stream_ordering falls between the two tokens (taking a None token to mean unbounded). + The token range is defined by > `lower_token` and <= `upper_token`. + Used to filter results from fetching events in the DB against the given tokens. This is necessary to handle the case where the tokens include position maps, which we handle by fetching more than necessary from the DB From d6eb04a9110388960397518078027e8a7ad2172e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 16:12:38 -0500 Subject: [PATCH 21/22] Fixup function --- synapse/handlers/sliding_sync/room_lists.py | 50 +++++++++++++++----- synapse/storage/databases/main/roommember.py | 11 +---- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index bf525d4ef23..7e3cf539df2 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -249,11 +249,6 @@ async def _compute_interested_rooms_new_tables( user_id ) ) - # 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. - room_membership_for_user_map = dict(room_membership_for_user_map) # 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`. # @@ -266,19 +261,31 @@ async def _compute_interested_rooms_new_tables( # `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. - room_membership_for_user_map.update( + self_leave_room_membership_for_user_map = ( await self.store.get_sliding_sync_self_leave_rooms_after_to_token( user_id, to_token ) ) - - logger.info( - "asdf room_membership_for_user_map: %s", room_membership_for_user_map - ) + 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: + # 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 for room_id in list(room_membership_for_user_map.keys()): @@ -299,8 +306,14 @@ async def _compute_interested_rooms_new_tables( changes = await self._get_rewind_changes_to_current_membership_to_token( sync_config.user, room_membership_for_user_map, to_token=to_token ) - logger.info("asdf rewind changes: %s", changes) if changes: + # 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: # Remove rooms that the user joined after the `to_token` @@ -342,6 +355,13 @@ async def _compute_interested_rooms_new_tables( newly_left_room_map.keys() - room_membership_for_user_map.keys() ) if missing_newly_left_rooms: + # 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] # This should be a given @@ -548,6 +568,14 @@ async def _compute_interested_rooms_new_tables( if sync_config.room_subscriptions: with start_active_span("assemble_room_subscriptions"): + # 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 # depending on the `required_state` requested (see below). partial_state_rooms = await self.store.get_partial_rooms() diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index f5adc1ea906..ad5b4959470 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -1445,13 +1445,6 @@ async def get_sliding_sync_rooms_for_user_from_membership_snapshots( def _txn( txn: LoggingTransaction, ) -> Dict[str, RoomsForUserSlidingSync]: - sql = """ - SELECT room_id, membership, forgotten, event_stream_ordering FROM sliding_sync_membership_snapshots - WHERE user_id = ? - """ - txn.execute(sql, (user_id,)) - logger.info("asdf raw sliding_sync_membership_snapshots %s", txn.fetchall()) - # XXX: If you use any new columns that can change (like from # `sliding_sync_joined_rooms` or `forgotten`), make sure to bust the # `get_sliding_sync_rooms_for_user_from_membership_snapshots` cache in the @@ -1544,8 +1537,8 @@ def _txn( INNER JOIN rooms AS r USING (room_id) WHERE user_id = ? AND m.forgotten = 0 - AND m.membership == 'leave' - AND m.user_id == m.sender + AND m.membership = 'leave' + AND m.user_id = m.sender AND (m.event_stream_ordering > ?) """ # If a leave happens after the token range, we may have still been joined From 9cd2098c50716b631513156e40bb2307ece5b08a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 6 May 2025 16:22:28 -0500 Subject: [PATCH 22/22] Fixup reversed filter logic --- synapse/storage/databases/main/roommember.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index ad5b4959470..20847765439 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -1569,7 +1569,7 @@ def _txn( # We filter out unknown room versions proactively. They shouldn't go # down sync and their metadata may be in a broken state (causing # errors). - if row[4] in KNOWN_ROOM_VERSIONS: + if row[4] not in KNOWN_ROOM_VERSIONS: continue # We only want to include the self-leave membership if it happened after @@ -1577,7 +1577,7 @@ def _txn( # # Since the database pulls out more than necessary, we need to filter it # down here. - if not _filter_results_by_stream( + if _filter_results_by_stream( lower_token=None, upper_token=to_token.room_key, instance_name=room_for_user.event_pos.instance_name,