Correctly changes to required state config in sliding sync (#17785)

Fixes https://github.com/element-hq/synapse/issues/17698

This handles `required_state` changes by checking if new state has been
added to the config, and if so fetching and returning that from the
current state.

This also takes care to ensure that given a state entry S that is added,
removed and then re-added that we do *not* send S down a second time if
there have been no changes to S in the current state. This is fine for
Rust SDK (as it just remembers all state), but we might decide not to do
this behaviour in the MSC. If we decide to always send down S then its
easy enough to rip out all the code.

---------

Co-authored-by: Eric Eastwood <eric.eastwood@beta.gouv.fr>

changelog.d/17785.bugfix

@@ -0,0 +1 @@
+Fix bug with sliding sync where the server would not return state that was added to the `required_state` config.

changelog.d/17805.bugfix

@@ -0,0 +1 @@
+Fix bug with sliding sync where the server would not return state that was added to the `required_state` config.

synapse/handlers/sliding_sync/__init__.py

@@ -14,7 +14,7 @@
 
 import logging
 from itertools import chain
-from typing import TYPE_CHECKING, Dict, List, Mapping, Optional, Set, Tuple
+from typing import TYPE_CHECKING, AbstractSet, Dict, List, Mapping, Optional, Set, Tuple
 
 from prometheus_client import Histogram
 from typing_extensions import assert_never
@@ -522,6 +522,8 @@ class SlidingSyncHandler:
 
             state_reset_out_of_room = True
 
+        prev_room_sync_config = previous_connection_state.room_configs.get(room_id)
+
         # Determine whether we should limit the timeline to the token range.
         #
         # We should return historical messages (before token range) in the
@@ -550,7 +552,6 @@ class SlidingSyncHandler:
         # or `limited` mean for clients that interpret them correctly. In future this
         # behavior is almost certainly going to change.
         #
-        # TODO: Also handle changes to `required_state`
         from_bound = None
         initial = True
         ignore_timeline_bound = False
@@ -571,7 +572,6 @@ class SlidingSyncHandler:
 
             log_kv({"sliding_sync.room_status": room_status})
 
-            prev_room_sync_config = previous_connection_state.room_configs.get(room_id)
             if prev_room_sync_config is not None:
                 # Check if the timeline limit has increased, if so ignore the
                 # timeline bound and record the change (see "XXX: Odd behavior"
@@ -582,8 +582,6 @@ class SlidingSyncHandler:
                 ):
                     ignore_timeline_bound = True
 
-                # TODO: Check for changes in `required_state``
-
         log_kv(
             {
                 "sliding_sync.from_bound": from_bound,
@@ -997,6 +995,10 @@ class SlidingSyncHandler:
                 include_others=required_state_filter.include_others,
             )
 
+        # The required state map to store in the room sync config, if it has
+        # changed.
+        changed_required_state_map: Optional[Mapping[str, AbstractSet[str]]] = None
+
         # We can return all of the state that was requested if this was the first
         # time we've sent the room down this connection.
         room_state: StateMap[EventBase] = {}
@@ -1010,6 +1012,29 @@ class SlidingSyncHandler:
         else:
             assert from_bound is not None
 
+            if prev_room_sync_config is not None:
+                # Check if there are any changes to the required state config
+                # that we need to handle.
+                changed_required_state_map, added_state_filter = (
+                    _required_state_changes(
+                        user.to_string(),
+                        previous_room_config=prev_room_sync_config,
+                        room_sync_config=room_sync_config,
+                        state_deltas=room_state_delta_id_map,
+                    )
+                )
+
+                if added_state_filter:
+                    # Some state entries got added, so we pull out the current
+                    # state for them. If we don't do this we'd only send down new deltas.
+                    state_ids = await self.get_current_state_ids_at(
+                        room_id=room_id,
+                        room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                        state_filter=added_state_filter,
+                        to_token=to_token,
+                    )
+                    room_state_delta_id_map.update(state_ids)
+
             events = await self.store.get_events(
                 state_filter.filter_state(room_state_delta_id_map).values()
             )
@@ -1108,10 +1133,13 @@ class SlidingSyncHandler:
             # sensible order again.
             bump_stamp = 0
 
-        unstable_expanded_timeline = False
+        room_sync_required_state_map_to_persist = room_sync_config.required_state_map
-        prev_room_sync_config = previous_connection_state.room_configs.get(room_id)
+        if changed_required_state_map:
+            room_sync_required_state_map_to_persist = changed_required_state_map
+
         # Record the `room_sync_config` if we're `ignore_timeline_bound` (which means
         # that the `timeline_limit` has increased)
+        unstable_expanded_timeline = False
         if ignore_timeline_bound:
             # FIXME: We signal the fact that we're sending down more events to
             # the client by setting `unstable_expanded_timeline` to true (see
@@ -1120,7 +1148,7 @@ class SlidingSyncHandler:
 
             new_connection_state.room_configs[room_id] = RoomSyncConfig(
                 timeline_limit=room_sync_config.timeline_limit,
-                required_state_map=room_sync_config.required_state_map,
+                required_state_map=room_sync_required_state_map_to_persist,
             )
         elif prev_room_sync_config is not None:
             # If the result is `limited` then we need to record that the
@@ -1149,10 +1177,14 @@ class SlidingSyncHandler:
             ):
                 new_connection_state.room_configs[room_id] = RoomSyncConfig(
                     timeline_limit=room_sync_config.timeline_limit,
-                    required_state_map=room_sync_config.required_state_map,
+                    required_state_map=room_sync_required_state_map_to_persist,
                 )
 
-            # TODO: Record changes in required_state.
+            elif changed_required_state_map is not None:
+                new_connection_state.room_configs[room_id] = RoomSyncConfig(
+                    timeline_limit=room_sync_config.timeline_limit,
+                    required_state_map=room_sync_required_state_map_to_persist,
+                )
 
         else:
             new_connection_state.room_configs[room_id] = room_sync_config
@@ -1285,3 +1317,185 @@ class SlidingSyncHandler:
                     return new_bump_event_pos.stream
 
             return None
+
+
+def _required_state_changes(
+    user_id: str,
+    *,
+    previous_room_config: "RoomSyncConfig",
+    room_sync_config: RoomSyncConfig,
+    state_deltas: StateMap[str],
+) -> Tuple[Optional[Mapping[str, AbstractSet[str]]], StateFilter]:
+    """Calculates the changes between the required state room config from the
+    previous requests compared with the current request.
+
+    This does two things. First, it calculates if we need to update the room
+    config due to changes to required state. Secondly, it works out which state
+    entries we need to pull from current state and return due to the state entry
+    now appearing in the required state when it previously wasn't (on top of the
+    state deltas).
+
+    This function tries to ensure to handle the case where a state entry is
+    added, removed and then added again to the required state. In that case we
+    only want to re-send that entry down sync if it has changed.
+
+    Returns:
+        A 2-tuple of updated required state config (or None if there is no update)
+        and the state filter to use to fetch extra current state that we need to
+        return.
+    """
+
+    prev_required_state_map = previous_room_config.required_state_map
+    request_required_state_map = room_sync_config.required_state_map
+
+    if prev_required_state_map == request_required_state_map:
+        # There has been no change. Return immediately.
+        return None, StateFilter.none()
+
+    prev_wildcard = prev_required_state_map.get(StateValues.WILDCARD, set())
+    request_wildcard = request_required_state_map.get(StateValues.WILDCARD, set())
+
+    # If we were previously fetching everything ("*", "*"), always update the effective
+    # room required state config to match the request. And since we we're previously
+    # already fetching everything, we don't have to fetch anything now that they've
+    # narrowed.
+    if StateValues.WILDCARD in prev_wildcard:
+        return request_required_state_map, StateFilter.none()
+
+    # If a event type wildcard has been added or removed we don't try and do
+    # anything fancy, and instead always update the effective room required
+    # state config to match the request.
+    if request_wildcard - prev_wildcard:
+        # Some keys were added, so we need to fetch everything
+        return request_required_state_map, StateFilter.all()
+    if prev_wildcard - request_wildcard:
+        # Keys were only removed, so we don't have to fetch everything.
+        return request_required_state_map, StateFilter.none()
+
+    # Contains updates to the required state map compared with the previous room
+    # config. This has the same format as `RoomSyncConfig.required_state`
+    changes: Dict[str, AbstractSet[str]] = {}
+
+    # The set of types/state keys that we need to fetch and return to the
+    # client. Passed to `StateFilter.from_types(...)`
+    added: List[Tuple[str, Optional[str]]] = []
+
+    # First we calculate what, if anything, has been *added*.
+    for event_type in (
+        prev_required_state_map.keys() | request_required_state_map.keys()
+    ):
+        old_state_keys = prev_required_state_map.get(event_type, set())
+        request_state_keys = request_required_state_map.get(event_type, set())
+
+        if old_state_keys == request_state_keys:
+            # No change to this type
+            continue
+
+        if not request_state_keys - old_state_keys:
+            # Nothing *added*, so we skip. Removals happen below.
+            continue
+
+        # Always update changes to include the newly added keys
+        changes[event_type] = request_state_keys
+
+        if StateValues.WILDCARD in old_state_keys:
+            # We were previously fetching everything for this type, so we don't need to
+            # fetch anything new.
+            continue
+
+        # Record the new state keys to fetch for this type.
+        if StateValues.WILDCARD in request_state_keys:
+            # If we have added a wildcard then we always just fetch everything.
+            added.append((event_type, None))
+        else:
+            for state_key in request_state_keys - old_state_keys:
+                if state_key == StateValues.ME:
+                    added.append((event_type, user_id))
+                elif state_key == StateValues.LAZY:
+                    # We handle lazy loading separately (outside this function),
+                    # so don't need to explicitly add anything here.
+                    #
+                    # LAZY values should also be ignore for event types that are
+                    # not membership.
+                    pass
+                else:
+                    added.append((event_type, state_key))
+
+    added_state_filter = StateFilter.from_types(added)
+
+    # Convert the list of state deltas to map from type to state_keys that have
+    # changed.
+    changed_types_to_state_keys: Dict[str, Set[str]] = {}
+    for event_type, state_key in state_deltas:
+        changed_types_to_state_keys.setdefault(event_type, set()).add(state_key)
+
+    # Figure out what changes we need to apply to the effective required state
+    # config.
+    for event_type, changed_state_keys in changed_types_to_state_keys.items():
+        old_state_keys = prev_required_state_map.get(event_type, set())
+        request_state_keys = request_required_state_map.get(event_type, set())
+
+        if old_state_keys == request_state_keys:
+            # No change.
+            continue
+
+        if request_state_keys - old_state_keys:
+            # We've expanded the set of state keys, so we just clobber the
+            # current set with the new set.
+            #
+            # We could also ensure that we keep entries where the state hasn't
+            # changed, but are no longer in the requested required state, but
+            # that's a sufficient edge case that we can ignore (as its only a
+            # performance optimization).
+            changes[event_type] = request_state_keys
+            continue
+
+        old_state_key_wildcard = StateValues.WILDCARD in old_state_keys
+        request_state_key_wildcard = StateValues.WILDCARD in request_state_keys
+
+        if old_state_key_wildcard != request_state_key_wildcard:
+            # If a state_key wildcard has been added or removed, we always update the
+            # effective room required state config to match the request.
+            changes[event_type] = request_state_keys
+            continue
+
+        if event_type == EventTypes.Member:
+            old_state_key_lazy = StateValues.LAZY in old_state_keys
+            request_state_key_lazy = StateValues.LAZY in request_state_keys
+
+            if old_state_key_lazy != request_state_key_lazy:
+                # If a "$LAZY" has been added or removed we always update the effective room
+                # required state config to match the request.
+                changes[event_type] = request_state_keys
+                continue
+
+        # Handle "$ME" values by adding "$ME" if the state key matches the user
+        # ID.
+        if user_id in changed_state_keys:
+            changed_state_keys.add(StateValues.ME)
+
+        # At this point there are no wildcards and no additions to the set of
+        # state keys requested, only deletions.
+        #
+        # We only remove state keys from the effective state if they've been
+        # removed from the request *and* the state has changed. This ensures
+        # that if a client removes and then re-adds a state key, we only send
+        # down the associated current state event if its changed (rather than
+        # sending down the same event twice).
+        invalidated = (old_state_keys - request_state_keys) & changed_state_keys
+        if invalidated:
+            changes[event_type] = old_state_keys - invalidated
+
+    if changes:
+        # Update the required state config based on the changes.
+        new_required_state_map = dict(prev_required_state_map)
+        for event_type, state_keys in changes.items():
+            if state_keys:
+                new_required_state_map[event_type] = state_keys
+            else:
+                # Remove entries with empty state keys.
+                new_required_state_map.pop(event_type, None)
+
+        return new_required_state_map, added_state_filter
+    else:
+        return None, added_state_filter

synapse/storage/databases/main/sliding_sync.py

@@ -386,8 +386,8 @@ class SlidingSyncStore(SQLBaseStore):
         required_state_map: Dict[int, Dict[str, Set[str]]] = {}
         for row in rows:
             state = required_state_map[row[0]] = {}
-            for event_type, state_keys in db_to_json(row[1]):
+            for event_type, state_key in db_to_json(row[1]):
-                state[event_type] = set(state_keys)
+                state.setdefault(event_type, set()).add(state_key)
 
         # Get all the room configs, looking up the required state from the map
         # above.

synapse/types/state.py

@@ -616,6 +616,13 @@ class StateFilter:
 
         return False
 
+    def __bool__(self) -> bool:
+        """Returns true if this state filter will match any state, or false if
+        this is the empty filter"""
+        if self.include_others:
+            return True
+        return bool(self.types)
+
 
 _ALL_STATE_FILTER = StateFilter(types=immutabledict(), include_others=True)
 _ALL_NON_MEMBER_STATE_FILTER = StateFilter(

tests/handlers/test_sliding_sync.py

@@ -18,9 +18,10 @@
 #
 #
 import logging
-from typing import AbstractSet, Dict, Optional, Tuple
+from typing import AbstractSet, Dict, Mapping, Optional, Set, Tuple
 from unittest.mock import patch
 
+import attr
 from parameterized import parameterized
 
 from twisted.test.proto_helpers import MemoryReactor
@@ -35,15 +36,18 @@ from synapse.handlers.sliding_sync import (
     RoomsForUserType,
     RoomSyncConfig,
     StateValues,
+    _required_state_changes,
 )
 from synapse.rest import admin
 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, StreamToken, UserID
+from synapse.types import JsonDict, StateMap, StreamToken, UserID
 from synapse.types.handlers.sliding_sync import SlidingSyncConfig
+from synapse.types.state import StateFilter
 from synapse.util import Clock
 
+from tests import unittest
 from tests.replication._base import BaseMultiWorkerStreamTestCase
 from tests.unittest import HomeserverTestCase, TestCase
 
@@ -3213,3 +3217,689 @@ class SortRoomsTestCase(HomeserverTestCase):
             # We only care about the *latest* event in the room.
             [room_id1, room_id2],
         )
+
+
+@attr.s(slots=True, auto_attribs=True, frozen=True)
+class RequiredStateChangesTestParameters:
+    previous_required_state_map: Dict[str, Set[str]]
+    request_required_state_map: Dict[str, Set[str]]
+    state_deltas: StateMap[str]
+    expected_with_state_deltas: Tuple[
+        Optional[Mapping[str, AbstractSet[str]]], StateFilter
+    ]
+    expected_without_state_deltas: Tuple[
+        Optional[Mapping[str, AbstractSet[str]]], StateFilter
+    ]
+
+
+class RequiredStateChangesTestCase(unittest.TestCase):
+    """Test cases for `_required_state_changes`"""
+
+    @parameterized.expand(
+        [
+            (
+                "simple_no_change",
+                """Test no change to required state""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {"state_key"}},
+                    request_required_state_map={"type1": {"state_key"}},
+                    state_deltas={("type1", "state_key"): "$event_id"},
+                    # No changes
+                    expected_with_state_deltas=(None, StateFilter.none()),
+                    expected_without_state_deltas=(None, StateFilter.none()),
+                ),
+            ),
+            (
+                "simple_add_type",
+                """Test adding a type to the config""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {"state_key"}},
+                    request_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {"state_key"},
+                    },
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've added a type so we should persist the changed required state
+                        # config.
+                        {"type1": {"state_key"}, "type2": {"state_key"}},
+                        # We should see the new type added
+                        StateFilter.from_types([("type2", "state_key")]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"state_key"}, "type2": {"state_key"}},
+                        StateFilter.from_types([("type2", "state_key")]),
+                    ),
+                ),
+            ),
+            (
+                "simple_add_type_from_nothing",
+                """Test adding a type to the config when previously requesting nothing""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={},
+                    request_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {"state_key"},
+                    },
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've added a type so we should persist the changed required state
+                        # config.
+                        {"type1": {"state_key"}, "type2": {"state_key"}},
+                        # We should see the new types added
+                        StateFilter.from_types(
+                            [("type1", "state_key"), ("type2", "state_key")]
+                        ),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"state_key"}, "type2": {"state_key"}},
+                        StateFilter.from_types(
+                            [("type1", "state_key"), ("type2", "state_key")]
+                        ),
+                    ),
+                ),
+            ),
+            (
+                "simple_add_state_key",
+                """Test adding a state key to the config""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type": {"state_key1"}},
+                    request_required_state_map={"type": {"state_key1", "state_key2"}},
+                    state_deltas={("type", "state_key2"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've added a key so we should persist the changed required state
+                        # config.
+                        {"type": {"state_key1", "state_key2"}},
+                        # We should see the new state_keys added
+                        StateFilter.from_types([("type", "state_key2")]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type": {"state_key1", "state_key2"}},
+                        StateFilter.from_types([("type", "state_key2")]),
+                    ),
+                ),
+            ),
+            (
+                "simple_remove_type",
+                """
+                Test removing a type from the config when there are a matching state
+                delta does cause the persisted required state config to change
+
+                Test removing a type from the config when there are no matching state
+                deltas does *not* cause the persisted required state config to change
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {"state_key"},
+                    },
+                    request_required_state_map={"type1": {"state_key"}},
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # Remove `type2` since there's been a change to that state,
+                        # (persist the change to required state). That way next time,
+                        # they request `type2`, we see that we haven't sent it before
+                        # and send the new state. (we should still keep track that we've
+                        # sent `type1` before).
+                        {"type1": {"state_key"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `type2` is no longer requested but since that state hasn't
+                        # changed, nothing should change (we should still keep track
+                        # that we've sent `type2` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "simple_remove_type_to_nothing",
+                """
+                Test removing a type from the config and no longer requesting any state
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {"state_key"},
+                    },
+                    request_required_state_map={},
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # Remove `type2` since there's been a change to that state,
+                        # (persist the change to required state). That way next time,
+                        # they request `type2`, we see that we haven't sent it before
+                        # and send the new state. (we should still keep track that we've
+                        # sent `type1` before).
+                        {"type1": {"state_key"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `type2` is no longer requested but since that state hasn't
+                        # changed, nothing should change (we should still keep track
+                        # that we've sent `type2` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "simple_remove_state_key",
+                """
+                Test removing a state_key from the config
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type": {"state_key1", "state_key2"}},
+                    request_required_state_map={"type": {"state_key1"}},
+                    state_deltas={("type", "state_key2"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # Remove `(type, state_key2)` since there's been a change
+                        # to that state (persist the change to required state).
+                        # That way next time, they request `(type, state_key2)`, we see
+                        # that we haven't sent it before and send the new state. (we
+                        # should still keep track that we've sent `(type, state_key1)`
+                        # before).
+                        {"type": {"state_key1"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `(type, state_key2)` is no longer requested but since that
+                        # state hasn't changed, nothing should change (we should still
+                        # keep track that we've sent `(type, state_key1)` and `(type,
+                        # state_key2)` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "type_wildcards_add",
+                """
+                Test adding a wildcard type causes the persisted required state config
+                to change and we request everything.
+
+                If a event type wildcard has been added or removed we don't try and do
+                anything fancy, and instead always update the effective room required
+                state config to match the request.
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {"state_key2"}},
+                    request_required_state_map={
+                        "type1": {"state_key2"},
+                        StateValues.WILDCARD: {"state_key"},
+                    },
+                    state_deltas={
+                        ("other_type", "state_key"): "$event_id",
+                    },
+                    # We've added a wildcard, so we persist the change and request everything
+                    expected_with_state_deltas=(
+                        {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}},
+                        StateFilter.all(),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}},
+                        StateFilter.all(),
+                    ),
+                ),
+            ),
+            (
+                "type_wildcards_remove",
+                """
+                Test removing a wildcard type causes the persisted required state config
+                to change and request nothing.
+
+                If a event type wildcard has been added or removed we don't try and do
+                anything fancy, and instead always update the effective room required
+                state config to match the request.
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key2"},
+                        StateValues.WILDCARD: {"state_key"},
+                    },
+                    request_required_state_map={"type1": {"state_key2"}},
+                    state_deltas={
+                        ("other_type", "state_key"): "$event_id",
+                    },
+                    # We've removed a type wildcard, so we persist the change but don't request anything
+                    expected_with_state_deltas=(
+                        {"type1": {"state_key2"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"state_key2"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_wildcards_add",
+                """Test adding a wildcard state_key""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {"state_key"}},
+                    request_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {StateValues.WILDCARD},
+                    },
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    # We've added a wildcard state_key, so we persist the change and
+                    # request all of the state for that type
+                    expected_with_state_deltas=(
+                        {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}},
+                        StateFilter.from_types([("type2", None)]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}},
+                        StateFilter.from_types([("type2", None)]),
+                    ),
+                ),
+            ),
+            (
+                "state_key_wildcards_remove",
+                """Test removing a wildcard state_key""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key"},
+                        "type2": {StateValues.WILDCARD},
+                    },
+                    request_required_state_map={"type1": {"state_key"}},
+                    state_deltas={("type2", "state_key"): "$event_id"},
+                    # We've removed a state_key wildcard, so we persist the change and
+                    # request nothing
+                    expected_with_state_deltas=(
+                        {"type1": {"state_key"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    # We've removed a state_key wildcard but there have been no matching
+                    # state changes, so no changes needed, just persist the
+                    # `request_required_state_map` as-is.
+                    expected_without_state_deltas=(
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_remove_some",
+                """
+                Test that removing state keys work when only some of the state keys have
+                changed
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key1", "state_key2", "state_key3"}
+                    },
+                    request_required_state_map={"type1": {"state_key1"}},
+                    state_deltas={("type1", "state_key3"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've removed some state keys from the type, but only state_key3 was
+                        # changed so only that one should be removed.
+                        {"type1": {"state_key1", "state_key2"}},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # No changes needed, just persist the
+                        # `request_required_state_map` as-is
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_me_add",
+                """
+                Test adding state keys work when using "$ME"
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={},
+                    request_required_state_map={"type1": {StateValues.ME}},
+                    state_deltas={("type1", "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've added a type so we should persist the changed required state
+                        # config.
+                        {"type1": {StateValues.ME}},
+                        # We should see the new state_keys added
+                        StateFilter.from_types([("type1", "@user:test")]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {StateValues.ME}},
+                        StateFilter.from_types([("type1", "@user:test")]),
+                    ),
+                ),
+            ),
+            (
+                "state_key_me_remove",
+                """
+                Test removing state keys work when using "$ME"
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {StateValues.ME}},
+                    request_required_state_map={},
+                    state_deltas={("type1", "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # Remove `type1` since there's been a change to that state,
+                        # (persist the change to required state). That way next time,
+                        # they request `type1`, we see that we haven't sent it before
+                        # and send the new state. (if we were tracking that we sent any
+                        # other state, we should still keep track that).
+                        {},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `type1` is no longer requested but since that state hasn't
+                        # changed, nothing should change (we should still keep track
+                        # that we've sent `type1` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_user_id_add",
+                """
+                Test adding state keys work when using your own user ID
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={},
+                    request_required_state_map={"type1": {"@user:test"}},
+                    state_deltas={("type1", "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # We've added a type so we should persist the changed required state
+                        # config.
+                        {"type1": {"@user:test"}},
+                        # We should see the new state_keys added
+                        StateFilter.from_types([("type1", "@user:test")]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {"@user:test"}},
+                        StateFilter.from_types([("type1", "@user:test")]),
+                    ),
+                ),
+            ),
+            (
+                "state_key_me_remove",
+                """
+                Test removing state keys work when using your own user ID
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {"@user:test"}},
+                    request_required_state_map={},
+                    state_deltas={("type1", "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # Remove `type1` since there's been a change to that state,
+                        # (persist the change to required state). That way next time,
+                        # they request `type1`, we see that we haven't sent it before
+                        # and send the new state. (if we were tracking that we sent any
+                        # other state, we should still keep track that).
+                        {},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `type1` is no longer requested but since that state hasn't
+                        # changed, nothing should change (we should still keep track
+                        # that we've sent `type1` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_lazy_add",
+                """
+                Test adding state keys work when using "$LAZY"
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={},
+                    request_required_state_map={EventTypes.Member: {StateValues.LAZY}},
+                    state_deltas={(EventTypes.Member, "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # If a "$LAZY" has been added or removed we always update the
+                        # required state to what was requested for simplicity.
+                        {EventTypes.Member: {StateValues.LAZY}},
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        {EventTypes.Member: {StateValues.LAZY}},
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_lazy_remove",
+                """
+                Test removing state keys work when using "$LAZY"
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={EventTypes.Member: {StateValues.LAZY}},
+                    request_required_state_map={},
+                    state_deltas={(EventTypes.Member, "@user:test"): "$event_id"},
+                    expected_with_state_deltas=(
+                        # If a "$LAZY" has been added or removed we always update the
+                        # required state to what was requested for simplicity.
+                        {},
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        # `EventTypes.Member` is no longer requested but since that
+                        # state hasn't changed, nothing should change (we should still
+                        # keep track that we've sent `EventTypes.Member` before).
+                        None,
+                        # We don't need to request anything more if they are requesting
+                        # less state now
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "type_wildcard_with_state_key_wildcard_to_explicit_state_keys",
+                """
+                Test switching from a wildcard ("*", "*") to explicit state keys
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        StateValues.WILDCARD: {StateValues.WILDCARD}
+                    },
+                    request_required_state_map={
+                        StateValues.WILDCARD: {"state_key1", "state_key2", "state_key3"}
+                    },
+                    state_deltas={("type1", "state_key1"): "$event_id"},
+                    # If we were previously fetching everything ("*", "*"), always update the effective
+                    # room required state config to match the request. And since we we're previously
+                    # already fetching everything, we don't have to fetch anything now that they've
+                    # narrowed.
+                    expected_with_state_deltas=(
+                        {
+                            StateValues.WILDCARD: {
+                                "state_key1",
+                                "state_key2",
+                                "state_key3",
+                            }
+                        },
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        {
+                            StateValues.WILDCARD: {
+                                "state_key1",
+                                "state_key2",
+                                "state_key3",
+                            }
+                        },
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "type_wildcard_with_explicit_state_keys_to_wildcard_state_key",
+                """
+                Test switching from explicit to wildcard state keys ("*", "*")
+                """,
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        StateValues.WILDCARD: {"state_key1", "state_key2", "state_key3"}
+                    },
+                    request_required_state_map={
+                        StateValues.WILDCARD: {StateValues.WILDCARD}
+                    },
+                    state_deltas={("type1", "state_key1"): "$event_id"},
+                    # We've added a wildcard, so we persist the change and request everything
+                    expected_with_state_deltas=(
+                        {StateValues.WILDCARD: {StateValues.WILDCARD}},
+                        StateFilter.all(),
+                    ),
+                    expected_without_state_deltas=(
+                        {StateValues.WILDCARD: {StateValues.WILDCARD}},
+                        StateFilter.all(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_wildcard_to_explicit_state_keys",
+                """Test switching from a wildcard to explicit state keys with a concrete type""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={"type1": {StateValues.WILDCARD}},
+                    request_required_state_map={
+                        "type1": {"state_key1", "state_key2", "state_key3"}
+                    },
+                    state_deltas={("type1", "state_key1"): "$event_id"},
+                    # If a state_key wildcard has been added or removed, we always
+                    # update the effective room required state config to match the
+                    # request. And since we we're previously already fetching
+                    # everything, we don't have to fetch anything now that they've
+                    # narrowed.
+                    expected_with_state_deltas=(
+                        {
+                            "type1": {
+                                "state_key1",
+                                "state_key2",
+                                "state_key3",
+                            }
+                        },
+                        StateFilter.none(),
+                    ),
+                    expected_without_state_deltas=(
+                        {
+                            "type1": {
+                                "state_key1",
+                                "state_key2",
+                                "state_key3",
+                            }
+                        },
+                        StateFilter.none(),
+                    ),
+                ),
+            ),
+            (
+                "state_key_wildcard_to_explicit_state_keys",
+                """Test switching from a wildcard to explicit state keys with a concrete type""",
+                RequiredStateChangesTestParameters(
+                    previous_required_state_map={
+                        "type1": {"state_key1", "state_key2", "state_key3"}
+                    },
+                    request_required_state_map={"type1": {StateValues.WILDCARD}},
+                    state_deltas={("type1", "state_key1"): "$event_id"},
+                    # If a state_key wildcard has been added or removed, we always
+                    # update the effective room required state config to match the
+                    # request. And we need to request all of the state for that type
+                    # because we previously, only sent down a few keys.
+                    expected_with_state_deltas=(
+                        {"type1": {StateValues.WILDCARD}},
+                        StateFilter.from_types([("type1", None)]),
+                    ),
+                    expected_without_state_deltas=(
+                        {"type1": {StateValues.WILDCARD}},
+                        StateFilter.from_types([("type1", None)]),
+                    ),
+                ),
+            ),
+        ]
+    )
+    def test_xxx(
+        self,
+        _test_label: str,
+        _test_description: str,
+        test_parameters: RequiredStateChangesTestParameters,
+    ) -> None:
+        # Without `state_deltas`
+        changed_required_state_map, added_state_filter = _required_state_changes(
+            user_id="@user:test",
+            previous_room_config=RoomSyncConfig(
+                timeline_limit=0,
+                required_state_map=test_parameters.previous_required_state_map,
+            ),
+            room_sync_config=RoomSyncConfig(
+                timeline_limit=0,
+                required_state_map=test_parameters.request_required_state_map,
+            ),
+            state_deltas={},
+        )
+
+        self.assertEqual(
+            changed_required_state_map,
+            test_parameters.expected_without_state_deltas[0],
+            "changed_required_state_map does not match (without state_deltas)",
+        )
+        self.assertEqual(
+            added_state_filter,
+            test_parameters.expected_without_state_deltas[1],
+            "added_state_filter does not match (without state_deltas)",
+        )
+
+        # With `state_deltas`
+        changed_required_state_map, added_state_filter = _required_state_changes(
+            user_id="@user:test",
+            previous_room_config=RoomSyncConfig(
+                timeline_limit=0,
+                required_state_map=test_parameters.previous_required_state_map,
+            ),
+            room_sync_config=RoomSyncConfig(
+                timeline_limit=0,
+                required_state_map=test_parameters.request_required_state_map,
+            ),
+            state_deltas=test_parameters.state_deltas,
+        )
+
+        self.assertEqual(
+            changed_required_state_map,
+            test_parameters.expected_with_state_deltas[0],
+            "changed_required_state_map does not match (with state_deltas)",
+        )
+        self.assertEqual(
+            added_state_filter,
+            test_parameters.expected_with_state_deltas[1],
+            "added_state_filter does not match (with state_deltas)",
+        )

tests/rest/client/sliding_sync/test_rooms_required_state.py

@@ -862,3 +862,264 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
                 exact=True,
                 message=f"Expected only fully-stated rooms to show up for test_key={list_key}.",
             )
+
+    def test_rooms_required_state_expand(self) -> None:
+        """Test that when we expand the required state argument we get the
+        expanded state, and not just the changes to the new expanded."""
+
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        # Create a room with a room name.
+        room_id1 = self.helper.create_room_as(
+            user1_id, tok=user1_tok, extra_content={"name": "Foo"}
+        )
+
+        # Only request the state event to begin with
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [
+                        [EventTypes.Create, ""],
+                    ],
+                    "timeline_limit": 1,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        state_map = self.get_success(
+            self.storage_controllers.state.get_current_state(room_id1)
+        )
+
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Create, "")],
+            },
+            exact=True,
+        )
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to include the room name
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+            [EventTypes.Name, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should see the room name, even though there haven't been any
+        # changes.
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Name, "")],
+            },
+            exact=True,
+        )
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # We should not see any state changes.
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+        self.assertIsNone(response_body["rooms"][room_id1].get("required_state"))
+
+    def test_rooms_required_state_expand_retract_expand(self) -> None:
+        """Test that when expanding, retracting and then expanding the required
+        state, we get the changes that happened."""
+
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        # Create a room with a room name.
+        room_id1 = self.helper.create_room_as(
+            user1_id, tok=user1_tok, extra_content={"name": "Foo"}
+        )
+
+        # Only request the state event to begin with
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [
+                        [EventTypes.Create, ""],
+                    ],
+                    "timeline_limit": 1,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        state_map = self.get_success(
+            self.storage_controllers.state.get_current_state(room_id1)
+        )
+
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Create, "")],
+            },
+            exact=True,
+        )
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to include the room name
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+            [EventTypes.Name, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should see the room name, even though there haven't been any
+        # changes.
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Name, "")],
+            },
+            exact=True,
+        )
+
+        # Update the room name
+        self.helper.send_state(
+            room_id1, "m.room.name", {"name": "Bar"}, state_key="", tok=user1_tok
+        )
+
+        # Update the sliding sync requests to exclude the room name again
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should not see the updated room name in state (though it will be in
+        # the timeline).
+        self.assertIsNone(response_body["rooms"][room_id1].get("required_state"))
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to include the room name again
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+            [EventTypes.Name, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should see the *new* room name, even though there haven't been any
+        # changes.
+        state_map = self.get_success(
+            self.storage_controllers.state.get_current_state(room_id1)
+        )
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Name, "")],
+            },
+            exact=True,
+        )
+
+    def test_rooms_required_state_expand_deduplicate(self) -> None:
+        """Test that when expanding, retracting and then expanding the required
+        state, we don't get the state down again if it hasn't changed"""
+
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        # Create a room with a room name.
+        room_id1 = self.helper.create_room_as(
+            user1_id, tok=user1_tok, extra_content={"name": "Foo"}
+        )
+
+        # Only request the state event to begin with
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [
+                        [EventTypes.Create, ""],
+                    ],
+                    "timeline_limit": 1,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        state_map = self.get_success(
+            self.storage_controllers.state.get_current_state(room_id1)
+        )
+
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Create, "")],
+            },
+            exact=True,
+        )
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to include the room name
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+            [EventTypes.Name, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should see the room name, even though there haven't been any
+        # changes.
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                state_map[(EventTypes.Name, "")],
+            },
+            exact=True,
+        )
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to exclude the room name again
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should not see any state updates
+        self.assertIsNone(response_body["rooms"][room_id1].get("required_state"))
+
+        # Send a message so the room comes down sync.
+        self.helper.send(room_id1, "msg", tok=user1_tok)
+
+        # Update the sliding sync requests to include the room name again
+        sync_body["lists"]["foo-list"]["required_state"] = [
+            [EventTypes.Create, ""],
+            [EventTypes.Name, ""],
+        ]
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # We should not see the room name again, as we have already sent that
+        # down.
+        self.assertIsNone(response_body["rooms"][room_id1].get("required_state"))