From 64689d45f4de5d21b4f2ced2bbd8961f243e1a10 Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Thu, 15 Oct 2026 09:48:51 +0100 Subject: [PATCH] Fix Sliding Sync considering a displayname/avatar change to mean the room was newly-joined (#20339) Follows: #18375 This bugfix is a prerequisite for MSC4354 (Sticky Events, Experimental Feature tracking issue #19409) changes. - Add a failing test for join->join being newly_joined - Fix the join->join transition being treated as a new join (when prev_events didn't coincidentally mask the problem) Rationale: `event_edges` / `prev_events` does NOT link a membership event to its previous state. (It could even be someone else's membership!) In most real-world cases, this would generate a `prev_membership` of NULL for every normal membership change. It appears as though the author intended some kind of 'previous state' relationship, but if you fix the first branch of the `UNION` query to use `current_state_delta_stream` for this (which is the source of state transitions), then it looks very redundant with the second branch. It looks like the real reason we want to use the snapshots table is for out-of-band memberships (outlier memberships). So we continue using that for those, but ONLY for those. The `SUSPICIOUS` if block is removed because it appears to have been papering over this issue by deduplicating the spurious `NULL` -> non-null transitions, but when you remove those duplicates at the source, it causes more trouble because it doesn't know that what it's 'deduplicating' is actually the same membership transition. This means if you join, then out-of-band leave (e.g. last member on the server to leave), that room would appear in `newly_joined` on a new initial sync. (Tests did in fact catch this, too.) --------- Signed-off-by: Olivier 'reivilibre --- changelog.d/20339.bugfix | 1 + synapse/storage/databases/main/stream.py | 32 ++++++------ tests/handlers/test_sliding_sync.py | 62 ++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 18 deletions(-) create mode 100644 changelog.d/20339.bugfix 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: