From bfba0adf2feaac2d2529e794261ad5cb51749b4c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 15 Feb 2022 13:10:45 +0000 Subject: [PATCH 1/4] Fix incorrect `get_rooms_for_user` for remote user When the server leaves a room the `get_rooms_for_user` cache is not correctly invalidated for the remote users in the room. This means that subsequent calls to `get_rooms_for_user` for the remote users would incorrectly include the room (it shouldn't be included because the server no longer knows anything about the room). --- synapse/storage/databases/main/events.py | 27 ++++++----- tests/storage/test_events.py | 60 ++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 5246fccad57a..2f88eb3f60b7 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -975,6 +975,17 @@ def _update_current_state_txn( to_delete = delta_state.to_delete to_insert = delta_state.to_insert + # Figure out the changes of membership to invalidate the + # `get_rooms_for_user` cache. + # We find out which membership events we may have deleted + # and which we have added, then we invalidate the caches for all + # those users. + members_changed = { + state_key + for ev_type, state_key in itertools.chain(to_delete, to_insert) + if ev_type == EventTypes.Member + } + if delta_state.no_longer_in_room: # Server is no longer in the room so we delete the room from # current_state_events, being careful we've already updated the @@ -993,6 +1004,11 @@ def _update_current_state_txn( """ txn.execute(sql, (stream_id, self._instance_name, room_id)) + # We also want to invalidate the membership caches for remote + # users that were in the room. + users_in_room = self.store.get_users_in_room_txn(txn, room_id) + members_changed.update(users_in_room) + self.db_pool.simple_delete_txn( txn, table="current_state_events", @@ -1102,17 +1118,6 @@ def _update_current_state_txn( # Invalidate the various caches - # Figure out the changes of membership to invalidate the - # `get_rooms_for_user` cache. - # We find out which membership events we may have deleted - # and which we have added, then we invalidate the caches for all - # those users. - members_changed = { - state_key - for ev_type, state_key in itertools.chain(to_delete, to_insert) - if ev_type == EventTypes.Member - } - for member in members_changed: txn.call_after( self.store.get_rooms_for_user_with_stream_ordering.invalidate, diff --git a/tests/storage/test_events.py b/tests/storage/test_events.py index f462a8b1c721..66bdb962e558 100644 --- a/tests/storage/test_events.py +++ b/tests/storage/test_events.py @@ -329,3 +329,63 @@ def test_do_not_prune_gap_if_not_dummy(self): # Check the new extremity is just the new remote event. self.assert_extremities([local_message_event_id, remote_event_2.event_id]) + + +class InvalideUsersInRoomCacheTestCase(HomeserverTestCase): + servlets = [ + admin.register_servlets, + room.register_servlets, + login.register_servlets, + ] + + def prepare(self, reactor, clock, homeserver): + self.state = self.hs.get_state_handler() + self.persistence = self.hs.get_storage().persistence + self.store = self.hs.get_datastore() + + def test_remote_user_cache_invalidated(self): + """Test that if the server leaves a room the `get_users_in_room` cache + is invalidated for remote users. + """ + + # Set up a room with a local and remote user in it. + user_id = self.register_user("user", "pass") + token = self.login("user", "pass") + + room_id = self.helper.create_room_as( + "user", room_version=RoomVersions.V6.identifier, tok=token + ) + + body = self.helper.send(room_id, body="Test", tok=token) + local_message_event_id = body["event_id"] + + # Fudge a join event for a remote user. + remote_user = "@user:other" + remote_event_1 = event_from_pdu_json( + { + "type": EventTypes.Member, + "state_key": remote_user, + "content": {"membership": Membership.JOIN}, + "room_id": room_id, + "sender": remote_user, + "depth": 5, + "prev_events": [local_message_event_id], + "auth_events": [], + "origin_server_ts": self.clock.time_msec(), + }, + RoomVersions.V6, + ) + + context = self.get_success(self.state.compute_event_context(remote_event_1)) + self.get_success(self.persistence.persist_event(remote_event_1, context)) + + # Call `get_users_in_room` to add the remote user to the cache + rooms = self.get_success(self.store.get_rooms_for_user(remote_user)) + self.assertEqual(set(rooms), {room_id}) + + # Now we have the local server leave the room, and check that calling + # `get_user_in_room` for the remote user no longer includes the room. + self.helper.leave(room_id, user_id, tok=token) + + rooms = self.get_success(self.store.get_rooms_for_user(remote_user)) + self.assertEqual(set(rooms), set()) From 1ee530647a27eb5b8c8ce28ec1e15fa9a0061d30 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 15 Feb 2022 13:18:09 +0000 Subject: [PATCH 2/4] Newsfile --- changelog.d/11999.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11999.bugfix diff --git a/changelog.d/11999.bugfix b/changelog.d/11999.bugfix new file mode 100644 index 000000000000..fd8409590002 --- /dev/null +++ b/changelog.d/11999.bugfix @@ -0,0 +1 @@ +Fix long standing bug where `get_rooms_for_user` was not correctly invalidated for remote users when the server left a room. From 1dad80522b98f5f3dafccebc6110c26511c011af Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 15 Feb 2022 13:48:28 +0000 Subject: [PATCH 3/4] Fix comments --- synapse/storage/databases/main/events.py | 4 ++-- tests/storage/test_events.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 2f88eb3f60b7..a1d7a9b41300 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1004,8 +1004,8 @@ def _update_current_state_txn( """ txn.execute(sql, (stream_id, self._instance_name, room_id)) - # We also want to invalidate the membership caches for remote - # users that were in the room. + # We also want to invalidate the membership caches for users + # that were in the room. users_in_room = self.store.get_users_in_room_txn(txn, room_id) members_changed.update(users_in_room) diff --git a/tests/storage/test_events.py b/tests/storage/test_events.py index 66bdb962e558..7c30ae9598b7 100644 --- a/tests/storage/test_events.py +++ b/tests/storage/test_events.py @@ -344,7 +344,7 @@ def prepare(self, reactor, clock, homeserver): self.store = self.hs.get_datastore() def test_remote_user_cache_invalidated(self): - """Test that if the server leaves a room the `get_users_in_room` cache + """Test that if the server leaves a room the `get_rooms_for_user` cache is invalidated for remote users. """ @@ -379,7 +379,7 @@ def test_remote_user_cache_invalidated(self): context = self.get_success(self.state.compute_event_context(remote_event_1)) self.get_success(self.persistence.persist_event(remote_event_1, context)) - # Call `get_users_in_room` to add the remote user to the cache + # Call `get_rooms_for_user` to add the remote user to the cache rooms = self.get_success(self.store.get_rooms_for_user(remote_user)) self.assertEqual(set(rooms), {room_id}) From 383641efd318e16d65ec3baba85f380c6a132537 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 15 Feb 2022 13:54:19 +0000 Subject: [PATCH 4/4] Also test `get_users_in_room` is correctly invalidated --- tests/storage/test_events.py | 49 +++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_events.py b/tests/storage/test_events.py index 7c30ae9598b7..a8639d8f8216 100644 --- a/tests/storage/test_events.py +++ b/tests/storage/test_events.py @@ -343,7 +343,7 @@ def prepare(self, reactor, clock, homeserver): self.persistence = self.hs.get_storage().persistence self.store = self.hs.get_datastore() - def test_remote_user_cache_invalidated(self): + def test_remote_user_rooms_cache_invalidated(self): """Test that if the server leaves a room the `get_rooms_for_user` cache is invalidated for remote users. """ @@ -389,3 +389,50 @@ def test_remote_user_cache_invalidated(self): rooms = self.get_success(self.store.get_rooms_for_user(remote_user)) self.assertEqual(set(rooms), set()) + + def test_room_remote_user_cache_invalidated(self): + """Test that if the server leaves a room the `get_users_in_room` cache + is invalidated for remote users. + """ + + # Set up a room with a local and remote user in it. + user_id = self.register_user("user", "pass") + token = self.login("user", "pass") + + room_id = self.helper.create_room_as( + "user", room_version=RoomVersions.V6.identifier, tok=token + ) + + body = self.helper.send(room_id, body="Test", tok=token) + local_message_event_id = body["event_id"] + + # Fudge a join event for a remote user. + remote_user = "@user:other" + remote_event_1 = event_from_pdu_json( + { + "type": EventTypes.Member, + "state_key": remote_user, + "content": {"membership": Membership.JOIN}, + "room_id": room_id, + "sender": remote_user, + "depth": 5, + "prev_events": [local_message_event_id], + "auth_events": [], + "origin_server_ts": self.clock.time_msec(), + }, + RoomVersions.V6, + ) + + context = self.get_success(self.state.compute_event_context(remote_event_1)) + self.get_success(self.persistence.persist_event(remote_event_1, context)) + + # Call `get_users_in_room` to add the remote user to the cache + users = self.get_success(self.store.get_users_in_room(room_id)) + self.assertEqual(set(users), {user_id, remote_user}) + + # Now we have the local server leave the room, and check that calling + # `get_user_in_room` for the remote user no longer includes the room. + self.helper.leave(room_id, user_id, tok=token) + + users = self.get_success(self.store.get_users_in_room(room_id)) + self.assertEqual(users, [])