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 <oliverw@matrix.org>

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.

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

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: