diff --git a/changelog.d/20339.bugfix b/changelog.d/20339.bugfix new file mode 100644 index 0000000000..1f4cfc53dc --- /dev/null +++ b/changelog.d/20339.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse v1.130.0 where Sliding Sync would consider a displayname or avatar change in a room to mean that room was newly-joined. \ No newline at end of file diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 1d08601071..59048c2fc6 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -1196,10 +1196,11 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): min_from_id = from_key.stream max_to_id = to_key.get_max_stream_pos() - # This query looks at membership changes in - # `sliding_sync_membership_snapshots` which will not include users - # that were state reset out of rooms; so we need to look for that - # case in `current_state_delta_stream`. + # Collect membership changes from two source tables: + # - `current_state_delta_stream` for most changes + # - `sliding_sync_membership_snapshots` for out-of-band (outlier) membership changes. + # For out-of-band memberships, we don't know the previous membership, so + # set the `prev_membership` to NULL and treat them as always having changed. sql = """ SELECT room_id, @@ -1219,19 +1220,20 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): s.event_stream_ordering, s.membership, s.sender, - m_prev.membership AS prev_membership - FROM sliding_sync_membership_snapshots as s - LEFT JOIN event_edges AS e ON e.event_id = s.membership_event_id - LEFT JOIN room_memberships AS m_prev ON m_prev.event_id = e.prev_event_id - WHERE s.user_id = ? + NULL AS prev_membership + FROM sliding_sync_membership_snapshots AS s + INNER JOIN events AS e ON e.event_id = s.membership_event_id + WHERE + s.user_id = ? + AND e.outlier UNION ALL SELECT s.room_id, - e.event_id, - s.instance_name, - s.stream_id, + e.event_id AS membership_event_id, + s.instance_name AS event_instance_name, + s.stream_id AS event_stream_ordering, m.membership, e.sender, m_prev.membership AS prev_membership @@ -1289,12 +1291,6 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): ): continue - if membership_event_id is None and room_id in membership_changes: - # SUSPICIOUS: if we join a room and get state reset out of it - # in the same queried window, - # won't this ignore the 'state reset out of it' part? - continue - # When `s.event_id = null`, we won't be able to get respective # `room_membership` but can assume the user has left the room # because this only happens when the server leaves a room diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index f91468db1e..39d50d7ad5 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2234,6 +2234,68 @@ class ComputeInterestedRoomsTestCase(SlidingSyncBase): self.assertTrue(room_id1 not in newly_joined) self.assertTrue(room_id1 not in newly_left) + def test_display_name_change_is_not_newly_joined_room( + self, + ) -> None: + """ + Regression test that a displayname change is not considered a newly-joined room. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + after_room_join_token = self.event_sources.get_current_token() + + # Our bug was only triggered if the displayname change's `prev_events` didn't + # include the member's actual previous memberships, so for that reason we + # add a dummy event here, even though it appears meaningless for the test. + self.helper.send(room_id1, "test", tok=user2_tok) + + # Update the displayname during the token range + 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_change1_token = self.event_sources.get_current_token() + + 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_join_token, + to_token=after_change1_token, + ) + ) + room_id_results = set(interested_rooms.lists["foo-list"].ops[0].room_ids) + + self.assertEqual(room_id_results, {room_id1}) + # We must NOT be `newly_joined` (or `newly_left`, for that matter) + # because this was just a displayname change (and we were joined before the token range) + self.assertEqual(interested_rooms.newly_joined_rooms, set()) + self.assertEqual(interested_rooms.newly_left_rooms, set()) + def test_display_name_changes_before_and_after_token_range( self, ) -> None: