From fc6000c008d13a306e431ee17c5f726707ee9b33 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Nov 2025 10:43:46 +0000 Subject: [PATCH 01/56] Refactor heroes to not be added to room state We then filter them out before sending to the client, but it is unnecessary to do so and interferes with later changes. --- synapse/handlers/sliding_sync/__init__.py | 13 +++++++++++-- .../sliding_sync/test_rooms_required_state.py | 5 ----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 6a5d5c7b3cc..8ec69b990e0 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1151,6 +1151,11 @@ async def get_room_sync_data( # 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] = {} + + # Includes the state for the heroes if we need them (may contain other + # state as well). + hero_membership_state: StateMap[EventBase] = {} + if initial: room_state = await self.get_current_state_at( room_id=room_id, @@ -1158,6 +1163,10 @@ async def get_room_sync_data( state_filter=state_filter, to_token=to_token, ) + + # The `room_state` includes the hero membership state if needed. + # We'll later filter this down so we don't need to do so here. + hero_membership_state = room_state else: assert from_bound is not None @@ -1191,6 +1200,7 @@ async def get_room_sync_data( # If the membership changed and we have to get heroes, get the remaining # heroes from the state + hero_membership_state = {} if hero_user_ids: hero_membership_state = await self.get_current_state_at( room_id=room_id, @@ -1198,7 +1208,6 @@ async def get_room_sync_data( state_filter=StateFilter.from_types(hero_room_state), to_token=to_token, ) - room_state.update(hero_membership_state) required_room_state: StateMap[EventBase] = {} if required_state_filter != StateFilter.none(): @@ -1221,7 +1230,7 @@ async def get_room_sync_data( # Assemble heroes: extract the info from the state we just fetched heroes: list[SlidingSyncResult.RoomResult.StrippedHero] = [] for hero_user_id in hero_user_ids: - member_event = room_state.get((EventTypes.Member, hero_user_id)) + member_event = hero_membership_state.get((EventTypes.Member, hero_user_id)) if member_event is not None: heroes.append( SlidingSyncResult.RoomResult.StrippedHero( diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 210280bc488..47761b7fc7d 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -642,11 +642,6 @@ def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_m self._assertRequiredStateIncludes( response_body["rooms"][room_id1]["required_state"], { - # This appears because *some* membership in the room changed and the - # heroes are recalculated and is thrown in because we have it. But this - # is technically optional and not needed because we've already seen user2 - # in the last sync (and their membership hasn't changed). - state_map[(EventTypes.Member, user2_id)], # Appears because there is a message in the timeline from this user state_map[(EventTypes.Member, user4_id)], # Appears because there is a membership event in the timeline from this user From 087f6ebfc9c0d55c9f328dc392bc3351a0e05c40 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 19 Nov 2025 14:59:04 +0000 Subject: [PATCH 02/56] Always return all memberships for non-limited syncs This is so that clients know if they can use a cached `/members` response or not. --- synapse/handlers/sliding_sync/__init__.py | 25 +++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 8ec69b990e0..eeb09c0273c 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1072,10 +1072,27 @@ async def get_room_sync_data( # Update the required state filter so we pick up the new # membership - for user_id in timeline_membership: - required_state_types.append( - (EventTypes.Member, user_id) - ) + if limited or initial: + # If the timeline is limited, we only need to + # return the membership changes for people in + # the timeline. + for user_id in timeline_membership: + required_state_types.append( + (EventTypes.Member, user_id) + ) + else: + # For non-limited timelines we always return all + # membership changes. This is so that clients + # who have fetched the full membership list + # already can continue to maintain it for + # non-limited syncs. + # + # This assumes that for non-limited syncs there + # won't be many membership changes that wouldn't + # have been included already (this can only + # happen if membership state was rolled back due + # to state resolution anyway). + required_state_types.append((EventTypes.Member, None)) # Add an explicit entry for each user in the timeline # From 49fa7eb9852c29740cfef689a8f0a32b1a5f4c37 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Nov 2025 11:59:08 +0000 Subject: [PATCH 03/56] Make _required_state_changes return struct --- synapse/handlers/sliding_sync/__init__.py | 71 ++++++---- tests/handlers/test_sliding_sync.py | 152 ++++++++++++---------- 2 files changed, 130 insertions(+), 93 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index eeb09c0273c..2d79f4b6d6a 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -17,6 +17,7 @@ from itertools import chain from typing import TYPE_CHECKING, AbstractSet, Mapping +import attr from prometheus_client import Histogram from typing_extensions import assert_never @@ -1190,22 +1191,21 @@ async def get_room_sync_data( 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(), - prev_required_state_map=prev_room_sync_config.required_state_map, - request_required_state_map=expanded_required_state_map, - state_deltas=room_state_delta_id_map, - ) + changes_return = _required_state_changes( + user.to_string(), + prev_required_state_map=prev_room_sync_config.required_state_map, + request_required_state_map=expanded_required_state_map, + state_deltas=room_state_delta_id_map, ) + changed_required_state_map = changes_return.required_state_map_change - if added_state_filter: + if changes_return.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, + state_filter=changes_return.added_state_filter, to_token=to_token, ) room_state_delta_id_map.update(state_ids) @@ -1499,13 +1499,28 @@ async def _get_bump_stamp( return None +@attr.s(auto_attribs=True) +class _RequiredStateChangesReturn: + """Return type for _required_state_changes. + + Attributes: + required_state_map_change: The updated required state map to store in + the room config, or None if there is no change. + added_state_filter: The state filter to use to fetch any additional + current state that needs to be returned to the client. + """ + + required_state_map_change: Mapping[str, AbstractSet[str]] | None + added_state_filter: StateFilter + + def _required_state_changes( user_id: str, *, prev_required_state_map: Mapping[str, AbstractSet[str]], request_required_state_map: Mapping[str, AbstractSet[str]], state_deltas: StateMap[str], -) -> tuple[Mapping[str, AbstractSet[str]] | None, StateFilter]: +) -> _RequiredStateChangesReturn: """Calculates the changes between the required state room config from the previous requests compared with the current request. @@ -1518,16 +1533,13 @@ def _required_state_changes( 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. """ if prev_required_state_map == request_required_state_map: # There has been no change. Return immediately. - return None, StateFilter.none() - + return _RequiredStateChangesReturn( + required_state_map_change=None, + added_state_filter=StateFilter.none(), + ) prev_wildcard = prev_required_state_map.get(StateValues.WILDCARD, set()) request_wildcard = request_required_state_map.get(StateValues.WILDCARD, set()) @@ -1536,17 +1548,26 @@ def _required_state_changes( # 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() + return _RequiredStateChangesReturn( + required_state_map_change=request_required_state_map, + added_state_filter=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() + return _RequiredStateChangesReturn( + required_state_map_change=request_required_state_map, + added_state_filter=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() + return _RequiredStateChangesReturn( + required_state_map_change=request_required_state_map, + added_state_filter=StateFilter.none(), + ) # Contains updates to the required state map compared with the previous room # config. This has the same format as `RoomSyncConfig.required_state` @@ -1722,6 +1743,12 @@ def _required_state_changes( # Remove entries with empty state keys. new_required_state_map.pop(event_type, None) - return new_required_state_map, added_state_filter + return _RequiredStateChangesReturn( + required_state_map_change=new_required_state_map, + added_state_filter=added_state_filter, + ) else: - return None, added_state_filter + return _RequiredStateChangesReturn( + required_state_map_change=None, + added_state_filter=added_state_filter, + ) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 45829064415..4b17c27f58d 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -18,7 +18,7 @@ # # import logging -from typing import AbstractSet, Mapping +from typing import AbstractSet from unittest.mock import patch import attr @@ -38,6 +38,7 @@ RoomSyncConfig, StateValues, _required_state_changes, + _RequiredStateChangesReturn, ) from synapse.rest import admin from synapse.rest.client import knock, login, room @@ -3827,12 +3828,8 @@ 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[ - Mapping[str, AbstractSet[str]] | None, StateFilter - ] - expected_without_state_deltas: tuple[ - Mapping[str, AbstractSet[str]] | None, StateFilter - ] + expected_with_state_deltas: _RequiredStateChangesReturn + expected_without_state_deltas: _RequiredStateChangesReturn class RequiredStateChangesTestCase(unittest.TestCase): @@ -3848,8 +3845,12 @@ class RequiredStateChangesTestCase(unittest.TestCase): 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()), + expected_with_state_deltas=_RequiredStateChangesReturn( + None, StateFilter.none() + ), + expected_without_state_deltas=_RequiredStateChangesReturn( + None, StateFilter.none() + ), ), ), ( @@ -3862,14 +3863,14 @@ class RequiredStateChangesTestCase(unittest.TestCase): "type2": {"state_key"}, }, state_deltas={("type2", "state_key"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key"}, "type2": {"state_key"}}, StateFilter.from_types([("type2", "state_key")]), ), @@ -3885,7 +3886,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): "type2": {"state_key"}, }, state_deltas={("type2", "state_key"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # We've added a type so we should persist the changed required state # config. {"type1": {"state_key"}, "type2": {"state_key"}}, @@ -3894,7 +3895,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): [("type1", "state_key"), ("type2", "state_key")] ), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key"}, "type2": {"state_key"}}, StateFilter.from_types( [("type1", "state_key"), ("type2", "state_key")] @@ -3909,14 +3910,14 @@ class RequiredStateChangesTestCase(unittest.TestCase): 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type": {"state_key1", "state_key2"}}, StateFilter.from_types([("type", "state_key2")]), ), @@ -3929,7 +3930,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={"type": {"state_key1"}}, request_required_state_map={"type": {"state_key2", "state_key3"}}, state_deltas={("type", "state_key2"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # We've added a key so we should persist the changed required state # config. # @@ -3940,7 +3941,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): [("type", "state_key2"), ("type", "state_key3")] ), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type": {"state_key1", "state_key2", "state_key3"}}, StateFilter.from_types( [("type", "state_key2"), ("type", "state_key3")] @@ -3964,7 +3965,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={"type1": {"state_key"}}, state_deltas={("type2", "state_key"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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 @@ -3975,7 +3976,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -3998,7 +3999,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={}, state_deltas={("type2", "state_key"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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 @@ -4009,7 +4010,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -4029,7 +4030,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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 @@ -4041,7 +4042,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `(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, @@ -4073,11 +4074,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): ("other_type", "state_key"): "$event_id", }, # We've added a wildcard, so we persist the change and request everything - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}}, StateFilter.all(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}}, StateFilter.all(), ), @@ -4103,13 +4104,13 @@ class RequiredStateChangesTestCase(unittest.TestCase): ("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=( + expected_with_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key2"}}, # We don't need to request anything more if they are requesting # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key2"}}, # We don't need to request anything more if they are requesting # less state now @@ -4129,11 +4130,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}}, StateFilter.from_types([("type2", None)]), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}}, StateFilter.from_types([("type2", None)]), ), @@ -4151,7 +4152,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( {"type1": {"state_key"}}, # We don't need to request anything more if they are requesting # less state now @@ -4160,7 +4161,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( None, # We don't need to request anything more if they are requesting # less state now @@ -4180,7 +4181,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={"type1": {"state_key1"}}, state_deltas={("type1", "state_key3"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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"}}, @@ -4188,7 +4189,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # No changes needed, just persist the # `request_required_state_map` as-is None, @@ -4207,14 +4208,14 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={}, request_required_state_map={"type1": {StateValues.ME}}, state_deltas={("type1", "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {StateValues.ME}}, StateFilter.from_types([("type1", "@user:test")]), ), @@ -4229,7 +4230,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={"type1": {StateValues.ME}}, request_required_state_map={}, state_deltas={("type1", "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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 @@ -4240,7 +4241,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -4260,14 +4261,14 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={}, request_required_state_map={"type1": {"@user:test"}}, state_deltas={("type1", "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( {"type1": {"@user:test"}}, StateFilter.from_types([("type1", "@user:test")]), ), @@ -4282,7 +4283,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={"type1": {"@user:test"}}, request_required_state_map={}, state_deltas={("type1", "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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 @@ -4293,7 +4294,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -4313,13 +4314,13 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # 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=( + expected_without_state_deltas=_RequiredStateChangesReturn( {EventTypes.Member: {StateValues.LAZY}}, StateFilter.none(), ), @@ -4334,7 +4335,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={}, state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # If a "$LAZY" has been added or removed we always update the # required state to what was requested for simplicity. {}, @@ -4342,7 +4343,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -4370,7 +4371,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # Remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we should persist the changed to required state. That way next @@ -4387,7 +4388,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # We're not requesting any specific `EventTypes.Member` now but # since that state hasn't changed, nothing should change (we # should still keep track that we've sent specific @@ -4418,7 +4419,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): EventTypes.Member: {StateValues.LAZY, "@user4:test"} }, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. # @@ -4438,7 +4439,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. { @@ -4465,7 +4466,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the # changed required state config. # @@ -4485,7 +4486,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the # changed required state config. { @@ -4516,7 +4517,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # Remove `EventTypes.Member` since there's been a change to that # state, (persist the change to required state). That way next # time, they request `EventTypes.Member`, we see that we haven't @@ -4536,7 +4537,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # less state now StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # `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). @@ -4562,7 +4563,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {"@user4:test"}}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. # @@ -4581,7 +4582,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. { @@ -4613,7 +4614,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( { StateValues.WILDCARD: { "state_key1", @@ -4623,7 +4624,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( { StateValues.WILDCARD: { "state_key1", @@ -4649,11 +4650,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, state_deltas={("type1", "state_key1"): "$event_id"}, # We've added a wildcard, so we persist the change and request everything - expected_with_state_deltas=( + expected_with_state_deltas=_RequiredStateChangesReturn( {StateValues.WILDCARD: {StateValues.WILDCARD}}, StateFilter.all(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( {StateValues.WILDCARD: {StateValues.WILDCARD}}, StateFilter.all(), ), @@ -4673,7 +4674,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( { "type1": { "state_key1", @@ -4683,7 +4684,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, StateFilter.none(), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( { "type1": { "state_key1", @@ -4708,11 +4709,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): # 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=( + expected_with_state_deltas=_RequiredStateChangesReturn( {"type1": {StateValues.WILDCARD, "state_key2", "state_key3"}}, StateFilter.from_types([("type1", None)]), ), - expected_without_state_deltas=( + expected_without_state_deltas=_RequiredStateChangesReturn( { "type1": { StateValues.WILDCARD, @@ -4734,40 +4735,45 @@ def test_xxx( test_parameters: RequiredStateChangesTestParameters, ) -> None: # Without `state_deltas` - changed_required_state_map, added_state_filter = _required_state_changes( + state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, state_deltas={}, ) + changed_required_state_map = state_changes.required_state_map_change + added_state_filter = state_changes.added_state_filter self.assertEqual( changed_required_state_map, - test_parameters.expected_without_state_deltas[0], + test_parameters.expected_without_state_deltas.required_state_map_change, "changed_required_state_map does not match (without state_deltas)", ) self.assertEqual( added_state_filter, - test_parameters.expected_without_state_deltas[1], + test_parameters.expected_without_state_deltas.added_state_filter, "added_state_filter does not match (without state_deltas)", ) # With `state_deltas` - changed_required_state_map, added_state_filter = _required_state_changes( + state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, state_deltas=test_parameters.state_deltas, ) + changed_required_state_map = state_changes.required_state_map_change + added_state_filter = state_changes.added_state_filter + self.assertEqual( changed_required_state_map, - test_parameters.expected_with_state_deltas[0], + test_parameters.expected_with_state_deltas.required_state_map_change, "changed_required_state_map does not match (with state_deltas)", ) self.assertEqual( added_state_filter, - test_parameters.expected_with_state_deltas[1], + test_parameters.expected_with_state_deltas.added_state_filter, "added_state_filter does not match (with state_deltas)", ) @@ -4805,12 +4811,14 @@ def test_limit_retained_previous_state_keys( } # (function under test) - changed_required_state_map, added_state_filter = _required_state_changes( + state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, state_deltas={}, ) + changed_required_state_map = state_changes.required_state_map_change + assert changed_required_state_map is not None # We should only remember up to the maximum number of state keys @@ -4874,12 +4882,14 @@ def test_request_more_state_keys_than_remember_limit(self) -> None: ) # (function under test) - changed_required_state_map, added_state_filter = _required_state_changes( + state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, state_deltas={}, ) + changed_required_state_map = state_changes.required_state_map_change + assert changed_required_state_map is not None # Should include all of the requested state From 8cba3132a60350e58b4782d32b881a42443ee1c8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sat, 15 Nov 2025 10:56:10 +0000 Subject: [PATCH 04/56] Track lazy loaded members in SSS separately. This ensures that the set of required state doesn't keep growing as we add and remove member state. We then only load them from the DB when needed, rather than all state for all rooms when we get a request. --- synapse/handlers/sliding_sync/__init__.py | 223 +++++++++++++++--- .../storage/databases/main/sliding_sync.py | 152 +++++++++++- .../main/delta/93/02_sliding_sync_members.sql | 48 ++++ synapse/types/handlers/sliding_sync.py | 37 +++ tests/handlers/test_sliding_sync.py | 13 +- .../sliding_sync/test_rooms_required_state.py | 95 ++++++++ 6 files changed, 521 insertions(+), 47 deletions(-) create mode 100644 synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 2d79f4b6d6a..81e7d48b10d 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -63,6 +63,7 @@ HaveSentRoomFlag, MutablePerConnectionState, PerConnectionState, + RoomLazyMembershipChanges, RoomSyncConfig, SlidingSyncConfig, SlidingSyncResult, @@ -984,14 +985,15 @@ async def get_room_sync_data( # # Calculate the `StateFilter` based on the `required_state` for the room required_state_filter = StateFilter.none() - # The requested `required_state_map` with the lazy membership expanded and - # `$ME` replaced with the user's ID. This allows us to see what membership we've - # sent down to the client in the next request. - # - # Make a copy so we can modify it. Still need to be careful to make a copy of - # the state key sets if we want to add/remove from them. We could make a deep - # copy but this saves us some work. - expanded_required_state_map = dict(room_sync_config.required_state_map) + + # Keep track of which users' state we may need to fetch. We split this + # into explicit users and lazy loaded users. + explicit_user_state = set() + lazy_load_user_ids = set() + + # Whether lazy-loading of room members is enabled. + lazy_load_room_members = False + if room_membership_for_user_at_to_token.membership not in ( Membership.INVITE, Membership.KNOCK, @@ -1039,7 +1041,6 @@ async def get_room_sync_data( else: required_state_types: list[tuple[str, str | None]] = [] num_wild_state_keys = 0 - lazy_load_room_members = False num_others = 0 for ( state_type, @@ -1071,6 +1072,10 @@ async def get_room_sync_data( timeline_event.state_key ) + # The client needs to know the membership of everyone in + # the timeline we're returning. + lazy_load_user_ids.update(timeline_membership) + # Update the required state filter so we pick up the new # membership if limited or initial: @@ -1095,36 +1100,30 @@ async def get_room_sync_data( # to state resolution anyway). required_state_types.append((EventTypes.Member, None)) - # Add an explicit entry for each user in the timeline - # - # Make a new set or copy of the state key set so we can - # modify it without affecting the original - # `required_state_map` - expanded_required_state_map[EventTypes.Member] = ( - expanded_required_state_map.get( - EventTypes.Member, set() + # Record the extra members we're returning. + lazy_load_user_ids.update( + state_key + for event_type, state_key in room_state_delta_id_map + if event_type == EventTypes.Member ) - | timeline_membership - ) - elif state_key == StateValues.ME: + else: num_others += 1 - required_state_types.append((state_type, user.to_string())) + # Replace `$ME` with the user's ID so we can deduplicate # when someone requests the same state with `$ME` or with # their user ID. - # - # Make a new set or copy of the state key set so we can - # modify it without affecting the original - # `required_state_map` - expanded_required_state_map[EventTypes.Member] = ( - expanded_required_state_map.get( - EventTypes.Member, set() - ) - | {user.to_string()} + normalized_state_key = state_key + if state_key == StateValues.ME: + normalized_state_key = user.to_string() + + if state_type == EventTypes.Member: + # Also track explicitly requested member state for + # lazy membership tracking. + explicit_user_state.add(normalized_state_key) + + required_state_types.append( + (state_type, normalized_state_key) ) - else: - num_others += 1 - required_state_types.append((state_type, state_key)) set_tag( SynapseTags.FUNC_ARG_PREFIX @@ -1142,6 +1141,10 @@ async def get_room_sync_data( required_state_filter = StateFilter.from_types(required_state_types) + # Define `required_user_state` as all user state we want. + required_user_state = explicit_user_state | lazy_load_user_ids + lazy_load_user_ids -= explicit_user_state + # We need this base set of info for the response so let's just fetch it along # with the `required_state` for the room hero_room_state = [ @@ -1174,6 +1177,17 @@ async def get_room_sync_data( # state as well). hero_membership_state: StateMap[EventBase] = {} + # By default we mark all required user state as being added when lazy + # loaded members is enabled. + # + # We may later update this to account for previously sent members. + returned_users = {} + if lazy_load_room_members: + returned_users = dict.fromkeys(lazy_load_user_ids) + new_connection_state.room_lazy_membership[room_id] = RoomLazyMembershipChanges( + returned=returned_users + ) + if initial: room_state = await self.get_current_state_at( room_id=room_id, @@ -1186,19 +1200,38 @@ async def get_room_sync_data( # We'll later filter this down so we don't need to do so here. hero_membership_state = room_state else: + assert from_token is not None assert from_bound is not None if prev_room_sync_config is not None: + # Fetch which of the needed lazy-loaded members we have already sent. + if required_user_state: + previously_returned_user_state = ( + await self.store.get_sliding_sync_connection_lazy_members( + connection_position=from_token.connection_position, + room_id=room_id, + user_ids=required_user_state, + ) + ) + else: + previously_returned_user_state = {} + # Check if there are any changes to the required state config # that we need to handle. changes_return = _required_state_changes( user.to_string(), prev_required_state_map=prev_room_sync_config.required_state_map, - request_required_state_map=expanded_required_state_map, + request_required_state_map=room_sync_config.required_state_map, + previously_returned_user_state=previously_returned_user_state, + required_user_state=lazy_load_user_ids, state_deltas=room_state_delta_id_map, ) changed_required_state_map = changes_return.required_state_map_change + new_connection_state.room_lazy_membership[ + room_id + ].invalidated = changes_return.lazy_members_invalidated + if changes_return.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. @@ -1309,7 +1342,7 @@ async def get_room_sync_data( bump_stamp = 0 room_sync_required_state_map_to_persist: Mapping[str, AbstractSet[str]] = ( - expanded_required_state_map + room_sync_config.required_state_map ) if changed_required_state_map: room_sync_required_state_map_to_persist = changed_required_state_map @@ -1508,17 +1541,27 @@ class _RequiredStateChangesReturn: the room config, or None if there is no change. added_state_filter: The state filter to use to fetch any additional current state that needs to be returned to the client. + lazy_members_previously_returned: The set of user IDs we should add to + the lazy members cache that we had previously returned. + lazy_members_invalidated: The set of user IDs whose membership has + changed but we didn't send down, so we need to invalidate them from + the cache. """ required_state_map_change: Mapping[str, AbstractSet[str]] | None added_state_filter: StateFilter + lazy_members_previously_returned: AbstractSet[str] = frozenset() + lazy_members_invalidated: AbstractSet[str] = frozenset() + def _required_state_changes( user_id: str, *, prev_required_state_map: Mapping[str, AbstractSet[str]], request_required_state_map: Mapping[str, AbstractSet[str]], + previously_returned_user_state: Mapping[str, int | None], + required_user_state: AbstractSet[str], state_deltas: StateMap[str], ) -> _RequiredStateChangesReturn: """Calculates the changes between the required state room config from the @@ -1533,13 +1576,60 @@ def _required_state_changes( 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. + + Args: + user_id: The user ID of the user making the request. + prev_required_state_map: The required state map from the previous + request. + request_required_state_map: The required state map from the current + request. + previously_returned_user_state: The set of user IDs whose lazy-loaded + membership we have previously returned to the client. + required_user_state: The set of user IDs whose lazy-loaded membership + is required for this request. + state_deltas: The state deltas that have changed in the room since the + previous request. """ + + # First we find any lazy members that have been invalidated due to state + # changes that we are not sending down. + lazy_members_invalidated = set() + for event_type, state_key in state_deltas: + if event_type != EventTypes.Member: + continue + + if state_key in required_user_state: + # We're returning this member change. + continue + + if state_key not in previously_returned_user_state: + # We've not previously returned this member so nothing to + # invalidate. + continue + + lazy_members_invalidated.add(state_key) + if prev_required_state_map == request_required_state_map: - # There has been no change. Return immediately. + # There has been no change in state, just need to check lazy members. + newly_returned_lazy_members = ( + required_user_state - previously_returned_user_state.keys() + ) + if newly_returned_lazy_members: + # There are some new lazy members we need to fetch. + added_types: list[tuple[str, str | None]] = [] + for new_user_id in newly_returned_lazy_members: + added_types.append((EventTypes.Member, new_user_id)) + + added_state_filter = StateFilter.from_types(added_types) + else: + added_state_filter = StateFilter.none() + return _RequiredStateChangesReturn( required_state_map_change=None, - added_state_filter=StateFilter.none(), + added_state_filter=added_state_filter, + lazy_members_invalidated=lazy_members_invalidated, ) + prev_wildcard = prev_required_state_map.get(StateValues.WILDCARD, set()) request_wildcard = request_required_state_map.get(StateValues.WILDCARD, set()) @@ -1551,6 +1641,7 @@ def _required_state_changes( return _RequiredStateChangesReturn( required_state_map_change=request_required_state_map, added_state_filter=StateFilter.none(), + lazy_members_invalidated=lazy_members_invalidated, ) # If a event type wildcard has been added or removed we don't try and do @@ -1561,12 +1652,14 @@ def _required_state_changes( return _RequiredStateChangesReturn( required_state_map_change=request_required_state_map, added_state_filter=StateFilter.all(), + lazy_members_invalidated=lazy_members_invalidated, ) if prev_wildcard - request_wildcard: # Keys were only removed, so we don't have to fetch everything. return _RequiredStateChangesReturn( required_state_map_change=request_required_state_map, added_state_filter=StateFilter.none(), + lazy_members_invalidated=lazy_members_invalidated, ) # Contains updates to the required state map compared with the previous room @@ -1577,6 +1670,11 @@ def _required_state_changes( # client. Passed to `StateFilter.from_types(...)` added: list[tuple[str, str | None]] = [] + # Record any members that were previously explicitly tracked and should now + # be tracked as lazy members. This handles the case of membership changing + # from e.g. `{@alice:example.com}` to `{LAZY}`. + lazy_members_previously_returned: set[str] = set() + # 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]] = {} @@ -1599,6 +1697,39 @@ def _required_state_changes( # Nothing *added*, so we skip. Removals happen below. continue + # Handle the special case of adding LAZY membership, where we want to + # remember what explicit members we've previously sent down. + 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 not old_state_key_lazy and request_state_key_lazy: + # We're adding a LAZY flag. We therefore add any previously + # explicit members we've sent down to lazy cache. + for state_key in old_state_keys: + if ( + state_key == StateValues.WILDCARD + or state_key == StateValues.LAZY + ): + # Ignore non-user IDs. + continue + + if state_key == StateValues.ME: + # Normalize to proper user ID + state_key = user_id + + # We remember the user if either a) they haven't been + # invalidated... + if (EventTypes.Member, state_key) not in state_deltas: + lazy_members_previously_returned.add(state_key) + + # ...or b) if we are going to send the delta down in this + # sync. + if state_key in required_user_state: + lazy_members_previously_returned.add(state_key) + + changes[event_type] = request_state_keys + continue + # 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 @@ -1669,9 +1800,23 @@ def _required_state_changes( # LAZY values should also be ignore for event types that are # not membership. pass + elif event_type == EventTypes.Member: + if state_key not in previously_returned_user_state: + # Only add *explicit* members we haven't previously sent + # down. + added.append((event_type, state_key)) else: added.append((event_type, state_key)) + # We also need to pull out any lazy members that are now required but + # haven't previously been returned. + for required_user_id in ( + required_user_state + - previously_returned_user_state.keys() + - lazy_members_previously_returned + ): + added.append((EventTypes.Member, required_user_id)) + added_state_filter = StateFilter.from_types(added) # Figure out what changes we need to apply to the effective required state @@ -1746,9 +1891,13 @@ def _required_state_changes( return _RequiredStateChangesReturn( required_state_map_change=new_required_state_map, added_state_filter=added_state_filter, + lazy_members_invalidated=lazy_members_invalidated, + lazy_members_previously_returned=lazy_members_previously_returned, ) else: return _RequiredStateChangesReturn( required_state_map_change=None, added_state_filter=added_state_filter, + lazy_members_invalidated=lazy_members_invalidated, + lazy_members_previously_returned=lazy_members_previously_returned, ) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 2b67e75ac47..fbe5b31d43c 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -14,7 +14,7 @@ import logging -from typing import TYPE_CHECKING, Mapping, cast +from typing import TYPE_CHECKING, AbstractSet, Mapping, cast import attr @@ -26,12 +26,14 @@ LoggingDatabaseConnection, LoggingTransaction, ) +from synapse.storage.engines import PostgresEngine from synapse.types import MultiWriterStreamToken, RoomStreamToken from synapse.types.handlers.sliding_sync import ( HaveSentRoom, HaveSentRoomFlag, MutablePerConnectionState, PerConnectionState, + RoomLazyMembershipChanges, RoomStatusMap, RoomSyncConfig, ) @@ -349,6 +351,13 @@ def persist_per_connection_state_txn( value_values=values, ) + self._persist_sliding_sync_connection_lazy_members_txn( + txn, + connection_key, + connection_position, + per_connection_state.room_lazy_membership, + ) + return connection_position @cached(iterable=True, max_entries=100000) @@ -406,6 +415,19 @@ def _get_and_clear_connection_positions_txn( """ txn.execute(sql, (connection_key, connection_position)) + # Move any lazy membership entries for this connection position to have + # `NULL` connection position, indicating that it applies to all future + # positions on this connecetion. + self.db_pool.simple_update_txn( + txn, + table="sliding_sync_connection_lazy_members", + keyvalues={ + "connection_key": connection_key, + "connection_position": connection_position, + }, + updatevalues={"connection_position": None}, + ) + # Fetch and create a mapping from required state ID to the actual # required state for the connection. rows = self.db_pool.simple_select_list_txn( @@ -484,8 +506,131 @@ def _get_and_clear_connection_positions_txn( receipts=RoomStatusMap(receipts), account_data=RoomStatusMap(account_data), room_configs=room_configs, + room_lazy_membership={}, ) + async def get_sliding_sync_connection_lazy_members( + self, + connection_position: int, + room_id: str, + user_ids: AbstractSet[str], + ) -> Mapping[str, int]: + """Get which user IDs in the room we have previously sent lazy + membership for. + + Args: + connection_position: The sliding sync connection position. + room_id: The room ID to get lazy members for. + user_ids: The user IDs to check for lazy membership. + + Returns: + The mapping of user IDs to the last seen timestamp for those user + IDs. + """ + + def get_sliding_sync_connection_lazy_members_txn( + txn: LoggingTransaction, + ) -> Mapping[str, int]: + sql = """ + SELECT user_id, mem.connection_position, last_seen_ts + FROM sliding_sync_connection_positions AS pos + INNER JOIN sliding_sync_connection_lazy_members AS mem USING (connection_key) + WHERE pos.connection_position = ? + """ + + txn.execute(sql, (connection_position,)) + + # Filter out any cache entries that only apply to forked connection + # positions. Entries with `NULL` connection position apply to all + # positions on the connection. + return { + user_id: last_seen_ts + for user_id, db_connection_position, last_seen_ts in txn + if db_connection_position == connection_position + or db_connection_position is None + } + + return await self.db_pool.runInteraction( + "sliding_sync_connection_lazy_members", + get_sliding_sync_connection_lazy_members_txn, + db_autocommit=True, # Avoid transaction for single read + ) + + def _persist_sliding_sync_connection_lazy_members_txn( + self, + txn: LoggingTransaction, + connection_key: int, + new_connection_position: int, + all_changes: dict[str, RoomLazyMembershipChanges], + ) -> None: + """Persist that we have sent lazy membership for the given user IDs.""" + + now = self.clock.time_msec() + + # Figure out which cache entries to add or update. + # + # These are either a) new entries we've never sent before (i.e. with a + # None last_seen_ts), or b) where the `last_seen_ts` is old enough that + # we want to update it. + to_update: list[tuple[str, str]] = [] + for room_id, room_changes in all_changes.items(): + for user_id, last_seen_ts in room_changes.returned.items(): + if last_seen_ts is None: + # We've never sent this user before, so we need to record that + # we've sent it at the new connection position. + to_update.append((room_id, user_id)) + elif last_seen_ts + 60 * 60 * 1000 < now: + # We last saw this user over an hour ago, so we update the + # timestamp. + to_update.append((room_id, user_id)) + + if to_update: + # Upsert the new/updated entries. + # + # Ignore conflicts where the existing entry has a different + # connection position (i.e. from a forked connection position). This + # may mean that we lose some updates, but that's acceptable as this + # is a cache and its fine for it to *not* include rows. + sql = """ + INSERT INTO sliding_sync_connection_lazy_members + (connection_key, connection_position, room_id, user_id, last_seen_ts) + VALUES {value_placeholder} + ON CONFLICT (connection_key, room_id, user_id) + DO UPDATE SET last_seen_ts = EXCLUDED.last_seen_ts + WHERE sliding_sync_connection_lazy_members.connection_position IS NULL + OR sliding_sync_connection_lazy_members.connection_position = EXCLUDED.connection_position + """ + + args = [ + (connection_key, new_connection_position, room_id, user_id, now) + for room_id, user_id in to_update + ] + + if isinstance(self.database_engine, PostgresEngine): + sql = sql.format(value_placeholder="?") + txn.execute_values(sql, args, fetch=False) + else: + sql = sql.format(value_placeholder="(?, ?, ?, ?, ?)") + txn.execute_batch(sql, args) + + # Remove any invlalidated entries. + to_remove: list[tuple[str, str]] = [] + for room_id, room_changes in all_changes.items(): + for user_id in room_changes.invalidated: + to_remove.append((room_id, user_id)) + + if to_remove: + # We don't try and match on connection position here: it's fine to + # remove it from all forks. + self.db_pool.simple_delete_many_batch_txn( + txn, + table="sliding_sync_connection_lazy_members", + keys=("connection_key", "room_id", "user_id"), + values=[ + (connection_key, room_id, user_id) for room_id, user_id in to_remove + ], + ) + @attr.s(auto_attribs=True, frozen=True) class PerConnectionStateDB: @@ -496,6 +641,8 @@ class PerConnectionStateDB: serialized to strings. When persisting this *only* contains updates to the state. + + The `room_lazy_membership` field is only used when persisting. """ rooms: "RoomStatusMap[str]" @@ -504,6 +651,8 @@ class PerConnectionStateDB: room_configs: Mapping[str, "RoomSyncConfig"] + room_lazy_membership: dict[str, RoomLazyMembershipChanges] + @staticmethod async def from_state( per_connection_state: "MutablePerConnectionState", store: "DataStore" @@ -557,6 +706,7 @@ async def from_state( receipts=RoomStatusMap(receipts), account_data=RoomStatusMap(account_data), room_configs=per_connection_state.room_configs.maps[0], + room_lazy_membership=per_connection_state.room_lazy_membership, ) async def to_state(self, store: "DataStore") -> "PerConnectionState": diff --git a/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql new file mode 100644 index 00000000000..914a43cb24d --- /dev/null +++ b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql @@ -0,0 +1,48 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2025 Element Creations Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + + +-- Tracks which member states have been sent to the client for lazy-loaded +-- members in sliding sync. This is a *cache* as it doesn't matter if we send +-- down members we've previously sent down. +-- +-- We track a *rough* `last_seen_ts` for each user in each room which indicates +-- when we last would've sent their member state to the client. This is used so +-- that we can remove members which haven't been seen for a while to save space. +-- +-- Care must be taken when handling "forked" positions, i.e. we have responded +-- to a request with a position and then get another different request using the +-- previous position as a base. We track this by including a +-- `connection_position` for newly inserted rows. When we advance the position +-- we set this to NULL for all rows which were present at that position, and +-- delete all other rows. When reading rows we can then filter out any rows +-- which have a non-NULL `connection_position` which is not the current +-- position. +-- +-- I.e. `connection_position` is NULL for rows which are valid for *all* +-- positions on the connection, and is non-NULL for rows which are only valid +-- for a specific position. +-- +-- When invalidating rows, we can just delete them. Technically this could +-- incorrectly invalidate for a forked position, but this is acceptable as it's +-- just a cache. +CREATE TABLE sliding_sync_connection_lazy_members ( + connection_key BIGINT NOT NULL REFERENCES sliding_sync_connections(connection_key) ON DELETE CASCADE, + connection_position BIGINT REFERENCES sliding_sync_connection_positions(connection_position) ON DELETE CASCADE, + room_id TEXT NOT NULL, + user_id TEXT NOT NULL, + last_seen_ts BIGINT NOT NULL +); + +CREATE UNIQUE INDEX sliding_sync_connection_lazy_members_idx ON sliding_sync_connection_lazy_members (connection_key, room_id, user_id); +CREATE INDEX sliding_sync_connection_lazy_members_pos_idx ON sliding_sync_connection_lazy_members (connection_key, connection_position) WHERE connection_position IS NOT NULL; diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 494e3570d05..37dc92d1a58 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -885,6 +885,36 @@ def __len__(self) -> int: return len(self.rooms) + len(self.receipts) + len(self.room_configs) +@attr.s(auto_attribs=True) +class RoomLazyMembershipChanges: + """Changes to lazily-loaded room memberships for a given room. + + Attributes: + returned: Map from user ID to timestamp for users whose membership we + have lazily loaded. The timestamp indicates the time we previously + saw the membership if we have sent it down previously, or None if + we sent it down for the first time. + + Note: this will include users whose membership we would have sent + down but didn't due to us having previously sent them. + invalidated: Set of user IDs whose latest membership we have *not* sent + down + """ + + # A map from user ID -> timestamp. Indicates that those memberships have + # been lazily loaded. I.e. that either a) we sent those memberships down, or + # b) we did so previously. The timestamp indicates the time we previously + # saw the membership. + returned: Mapping[str, int | None] = attr.Factory(dict) + + # A set of user IDs whose membership change we have *not* sent + # down + invalidated: AbstractSet[str] = attr.Factory(set) + + def __bool__(self) -> bool: + return bool(self.returned or self.invalidated) + + @attr.s(auto_attribs=True) class MutablePerConnectionState(PerConnectionState): """A mutable version of `PerConnectionState`""" @@ -895,12 +925,19 @@ class MutablePerConnectionState(PerConnectionState): room_configs: typing.ChainMap[str, RoomSyncConfig] + # A map from room ID -> user ID -> timestamp. Indicates that those + # memberships have been lazily loaded. I.e. that either a) we sent those + # memberships down, or b) we did so previously. The timestamp indicates the + # time we previously saw the membership. + room_lazy_membership: dict[str, RoomLazyMembershipChanges] = attr.Factory(dict) + def has_updates(self) -> bool: return ( bool(self.rooms.get_updates()) or bool(self.receipts.get_updates()) or bool(self.account_data.get_updates()) or bool(self.get_room_config_updates()) + or bool(self.room_lazy_membership) ) def get_room_config_updates(self) -> Mapping[str, RoomSyncConfig]: diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 4b17c27f58d..faacc78d4e4 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4741,16 +4741,14 @@ def test_xxx( request_required_state_map=test_parameters.request_required_state_map, state_deltas={}, ) - changed_required_state_map = state_changes.required_state_map_change - added_state_filter = state_changes.added_state_filter self.assertEqual( - changed_required_state_map, + state_changes.required_state_map_change, test_parameters.expected_without_state_deltas.required_state_map_change, "changed_required_state_map does not match (without state_deltas)", ) self.assertEqual( - added_state_filter, + state_changes.added_state_filter, test_parameters.expected_without_state_deltas.added_state_filter, "added_state_filter does not match (without state_deltas)", ) @@ -4763,16 +4761,13 @@ def test_xxx( state_deltas=test_parameters.state_deltas, ) - changed_required_state_map = state_changes.required_state_map_change - added_state_filter = state_changes.added_state_filter - self.assertEqual( - changed_required_state_map, + state_changes.required_state_map_change, test_parameters.expected_with_state_deltas.required_state_map_change, "changed_required_state_map does not match (with state_deltas)", ) self.assertEqual( - added_state_filter, + state_changes.added_state_filter, test_parameters.expected_with_state_deltas.added_state_filter, "added_state_filter does not match (with state_deltas)", ) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 47761b7fc7d..44698302eb8 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -836,6 +836,101 @@ def test_rooms_required_state_expand_retract_expand_lazy_loading_room_members_in exact=True, ) + def test_lazy_members_limited_sync(self) -> None: + """Test that when using lazy loading for room members and a limited sync + missing a membership change, we include the membership change next time + said user says something. + """ + + 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) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Send a message from each user to the room so that both memberships are sent down. + self.helper.send(room_id1, "1", tok=user1_tok) + self.helper.send(room_id1, "2", tok=user2_tok) + + # Make a first sync to establish a position + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 2, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # We should see both membership events in required_state + 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.Member, user1_id)], + state_map[(EventTypes.Member, user2_id)], + }, + exact=True, + ) + + # User2 changes their display name (causing a membership change) + self.helper.send_state( + room_id1, + event_type=EventTypes.Member, + state_key=user2_id, + body={ + EventContentFields.MEMBERSHIP: Membership.JOIN, + EventContentFields.MEMBERSHIP_DISPLAYNAME: "New Name", + }, + tok=user2_tok, + ) + + # Send a couple of messages to the room to push out the membership change + self.helper.send(room_id1, "3", tok=user1_tok) + self.helper.send(room_id1, "4", tok=user1_tok) + + # Make an incremental Sliding Sync request + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # The membership change should *not* be included yet as user2 doesn't + # have any events in the timeline. + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1].get("required_state", []), + set(), + exact=True, + ) + + # Now user2 sends a message to the room + self.helper.send(room_id1, "5", tok=user2_tok) + + # Make another incremental Sliding Sync request + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # The membership change should now be included as user2 has an event + # in the timeline. + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1].get("required_state", []), + { + state_map[(EventTypes.Member, user2_id)], + }, + exact=True, + ) + def test_rooms_required_state_me(self) -> None: """ Test `rooms.required_state` correctly handles $ME. From 6303bb149af06434c146959bdea54ecd4346cc22 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Nov 2025 11:48:41 +0000 Subject: [PATCH 05/56] Update tests --- tests/handlers/test_sliding_sync.py | 224 +++++++++++++--------------- 1 file changed, 100 insertions(+), 124 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index faacc78d4e4..5750700db48 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -45,7 +45,10 @@ from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator from synapse.types import JsonDict, StateMap, StreamToken, UserID, create_requester -from synapse.types.handlers.sliding_sync import PerConnectionState, SlidingSyncConfig +from synapse.types.handlers.sliding_sync import ( + PerConnectionState, + SlidingSyncConfig, +) from synapse.types.state import StateFilter from synapse.util.clock import Clock @@ -3831,6 +3834,9 @@ class RequiredStateChangesTestParameters: expected_with_state_deltas: _RequiredStateChangesReturn expected_without_state_deltas: _RequiredStateChangesReturn + previously_returned_user_state: AbstractSet[str] = frozenset() + required_user_state: AbstractSet[str] = frozenset() + class RequiredStateChangesTestCase(unittest.TestCase): """Test cases for `_required_state_changes`""" @@ -4362,41 +4368,34 @@ class RequiredStateChangesTestCase(unittest.TestCase): we're sending down another response without any timeline events. """, RequiredStateChangesTestParameters( - previous_required_state_map={ - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - } - }, + previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_user_state={"@user2:test", "@user3:test"}, + required_user_state=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( + # The requested required state hasn't changed + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), # Remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we should persist the changed to required state. That way next # time, they request "@user2:test", we see that we haven't sent # it before and send the new state. (we should still keep track # that we've sent specific `EventTypes.Member` before) - { - EventTypes.Member: { - StateValues.LAZY, - "@user3:test", - } - }, - # We don't need to request anything more if they are requesting - # less state now - StateFilter.none(), + lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( - # We're not requesting any specific `EventTypes.Member` now but - # since that state hasn't changed, nothing should change (we - # should still keep track that we've sent specific - # `EventTypes.Member` before). + # The requested required state hasn't changed None, # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Nothing should change (we should still keep track that + # we've sent specific `EventTypes.Member` before). + lazy_members_invalidated=frozenset(), ), ), ), @@ -4408,50 +4407,32 @@ class RequiredStateChangesTestCase(unittest.TestCase): we're sending down another response with a new event from user4. """, RequiredStateChangesTestParameters( - previous_required_state_map={ - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - } - }, - request_required_state_map={ - EventTypes.Member: {StateValues.LAZY, "@user4:test"} - }, + previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_user_state={"@user2:test", "@user3:test"}, + required_user_state={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( - # Since "@user4:test" was added, we should persist the changed - # required state config. - # - # Also remove "@user2:test" since that state has changed and is no - # longer being requested anymore. Since something was removed, - # we also should persist the changed to required state. That way next - # time, they request "@user2:test", we see that we haven't sent - # it before and send the new state. (we should still keep track - # that we've sent specific `EventTypes.Member` before) - { - EventTypes.Member: { - StateValues.LAZY, - "@user3:test", - "@user4:test", - } - }, + # The requested required state hasn't changed + None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # Remove "@user2:test" since that state has changed and + # is no longer being requested anymore. Since something + # was removed, we also should persist the changed to + # required state. That way next time, they request + # "@user2:test", we see that we haven't sent it before + # and send the new state. (we should still keep track + # that we've sent specific `EventTypes.Member` before) + lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( - # Since "@user4:test" was added, we should persist the changed - # required state config. - { - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - "@user4:test", - } - }, + # The requested required state hasn't changed + None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # We don't invalidate user2 as they haven't changed + lazy_members_invalidated=frozenset(), ), ), ), @@ -4465,40 +4446,27 @@ class RequiredStateChangesTestCase(unittest.TestCase): EventTypes.Member: {"@user2:test", "@user3:test"} }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_user_state=frozenset(), + required_user_state={"@user3:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the # changed required state config. - # - # Also remove "@user2:test" since that state has changed and is no - # longer being requested anymore. Since something was removed, - # we also should persist the changed to required state. That way next - # time, they request "@user2:test", we see that we haven't sent - # it before and send the new state. (we should still keep track - # that we've sent specific `EventTypes.Member` before) - { - EventTypes.Member: { - StateValues.LAZY, - "@user3:test", - } - }, - # We don't need to request anything more if they are requesting - # less state now + {EventTypes.Member: {StateValues.LAZY}}, + # We have already sent @user3 down before. StateFilter.none(), + # Remember the fact that we've sent @user3 down before, + # but not @user2 as that has been invalidated. + lazy_members_previously_returned={"@user3:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the # changed required state config. - { - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - } - }, - # We don't need to request anything more if they are requesting - # less state now + {EventTypes.Member: {StateValues.LAZY}}, + # We have already sent @user3 down before. StateFilter.none(), + # Remember the fact that we've sent the users down before. + lazy_members_previously_returned={"@user2:test", "@user3:test"}, ), ), ), @@ -4508,34 +4476,20 @@ class RequiredStateChangesTestCase(unittest.TestCase): Test retracting the `required_state` to no longer lazy-loading room members. """, RequiredStateChangesTestParameters( - previous_required_state_map={ - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - } - }, + previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={}, + previously_returned_user_state={"@user2:test", "@user3:test"}, + required_user_state=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Remove `EventTypes.Member` since there's been a change to that - # state, (persist the change to required state). That way next - # time, they request `EventTypes.Member`, 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). - # - # This acts the same as the `simple_remove_type` test. It's - # possible that we could remember the specific `state_keys` that - # we have sent down before but this currently just acts the same - # as if a whole `type` was removed. Perhaps it's good that we - # "garbage collect" and forget what we've sent before for a - # given `type` when the client stops caring about a certain - # `type`. + # state, (persist the change to required state). {}, # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Explicitly remove the now invalidated @user2:test membership. + lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( # `EventTypes.Member` is no longer requested but since that @@ -4545,6 +4499,8 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Nothing has been invalidated. + lazy_members_invalidated=frozenset(), ), ), ), @@ -4554,44 +4510,29 @@ class RequiredStateChangesTestCase(unittest.TestCase): Test retracting the `required_state` to no longer lazy-loading room members. """, RequiredStateChangesTestParameters( - previous_required_state_map={ - EventTypes.Member: { - StateValues.LAZY, - "@user2:test", - "@user3:test", - } - }, + previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {"@user4:test"}}, + previously_returned_user_state={"@user2:test", "@user3:test"}, + required_user_state={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. - # + {EventTypes.Member: {"@user4:test"}}, + # We should see the new state_keys added + StateFilter.from_types([(EventTypes.Member, "@user4:test")]), # Also remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we also should persist the changed to required state. That way next # time, they request "@user2:test", we see that we haven't sent # it before and send the new state. (we should still keep track # that we've sent specific `EventTypes.Member` before) - { - EventTypes.Member: { - "@user3:test", - "@user4:test", - } - }, - # We should see the new state_keys added - StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed # required state config. - { - EventTypes.Member: { - "@user2:test", - "@user3:test", - "@user4:test", - } - }, + {EventTypes.Member: {"@user4:test"}}, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), ), @@ -4734,11 +4675,20 @@ def test_xxx( _test_description: str, test_parameters: RequiredStateChangesTestParameters, ) -> None: + # `_required_state_changes` expects this to be a map from user ID to + # timestamp, so let's convert it into one by using a dummy timestamp + # value + previously_returned_user_state = dict.fromkeys( + test_parameters.previously_returned_user_state, 1234 + ) + # Without `state_deltas` state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, + previously_returned_user_state=previously_returned_user_state, + required_user_state=test_parameters.required_user_state, state_deltas={}, ) @@ -4752,12 +4702,24 @@ def test_xxx( test_parameters.expected_without_state_deltas.added_state_filter, "added_state_filter does not match (without state_deltas)", ) + self.assertEqual( + state_changes.lazy_members_invalidated, + test_parameters.expected_without_state_deltas.lazy_members_invalidated, + "lazy_members_invalidated does not match (without state_deltas)", + ) + self.assertEqual( + state_changes.lazy_members_previously_returned, + test_parameters.expected_without_state_deltas.lazy_members_previously_returned, + "lazy_members_previously_returned does not match (without state_deltas)", + ) # With `state_deltas` state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, + previously_returned_user_state=previously_returned_user_state, + required_user_state=test_parameters.required_user_state, state_deltas=test_parameters.state_deltas, ) @@ -4771,6 +4733,16 @@ def test_xxx( test_parameters.expected_with_state_deltas.added_state_filter, "added_state_filter does not match (with state_deltas)", ) + self.assertEqual( + state_changes.lazy_members_invalidated, + test_parameters.expected_with_state_deltas.lazy_members_invalidated, + "lazy_members_invalidated does not match (with state_deltas)", + ) + self.assertEqual( + state_changes.lazy_members_previously_returned, + test_parameters.expected_with_state_deltas.lazy_members_previously_returned, + "lazy_members_previously_returned does not match (with state_deltas)", + ) @parameterized.expand( [ @@ -4810,6 +4782,8 @@ def test_limit_retained_previous_state_keys( user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, + previously_returned_user_state={}, + required_user_state=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change @@ -4881,6 +4855,8 @@ def test_request_more_state_keys_than_remember_limit(self) -> None: user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, + previously_returned_user_state={}, + required_user_state=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change From 5c4898341f5ffbfe24427217e7b7faa269c67f01 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 20 Nov 2025 13:43:00 +0000 Subject: [PATCH 06/56] Newsfile --- changelog.d/19206.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19206.bugfix diff --git a/changelog.d/19206.bugfix b/changelog.d/19206.bugfix new file mode 100644 index 00000000000..9cdfaa2571b --- /dev/null +++ b/changelog.d/19206.bugfix @@ -0,0 +1 @@ +Fix sliding sync performance slow down for long lived connections. From 498485865b8991ca259a214e751c358ee7eaf8df Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 20 Nov 2025 13:52:08 +0000 Subject: [PATCH 07/56] Fix check delta script It was thinking the table name was `IN`, as it matched `connection_positi(on IS) NULL`. --- scripts-dev/check_schema_delta.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts-dev/check_schema_delta.py b/scripts-dev/check_schema_delta.py index dd96c904bb3..7be6f3730ff 100755 --- a/scripts-dev/check_schema_delta.py +++ b/scripts-dev/check_schema_delta.py @@ -12,7 +12,7 @@ SCHEMA_FILE_REGEX = re.compile(r"^synapse/storage/schema/(.*)/delta/(.*)/(.*)$") INDEX_CREATION_REGEX = re.compile( - r"CREATE .*INDEX .*ON ([a-z_0-9]+)", flags=re.IGNORECASE + r"CREATE .*INDEX .*ON ([a-z_0-9]+)\s+\(", flags=re.IGNORECASE ) INDEX_DELETION_REGEX = re.compile(r"DROP .*INDEX ([a-z_0-9]+)", flags=re.IGNORECASE) TABLE_CREATION_REGEX = re.compile( From 7a0a8a2b9381fe8af433dc562123ae64455c620a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 14:12:49 +0000 Subject: [PATCH 08/56] Rename required_user_state --- synapse/handlers/sliding_sync/__init__.py | 30 ++++++++++++++--------- tests/handlers/test_sliding_sync.py | 20 +++++++-------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 81e7d48b10d..a5dbec9b22b 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1141,8 +1141,8 @@ async def get_room_sync_data( required_state_filter = StateFilter.from_types(required_state_types) - # Define `required_user_state` as all user state we want. - required_user_state = explicit_user_state | lazy_load_user_ids + # Remove any explicitly requested user state from the lazy-loaded set, + # as we track them separately. lazy_load_user_ids -= explicit_user_state # We need this base set of info for the response so let's just fetch it along @@ -1204,13 +1204,21 @@ async def get_room_sync_data( assert from_bound is not None if prev_room_sync_config is not None: - # Fetch which of the needed lazy-loaded members we have already sent. - if required_user_state: + # Define `required_user_state` as all user state we want. + all_required_user_state = explicit_user_state | lazy_load_user_ids + + # We need to know what user state we previously sent down the + # connection so we can determine what has changed. + # + # We don't just pull out the lazy loaded members here to handle + # the case where the client added explicit user state requests + # for users they already had lazy loaded. + if all_required_user_state: previously_returned_user_state = ( await self.store.get_sliding_sync_connection_lazy_members( connection_position=from_token.connection_position, room_id=room_id, - user_ids=required_user_state, + user_ids=all_required_user_state, ) ) else: @@ -1223,7 +1231,7 @@ async def get_room_sync_data( prev_required_state_map=prev_room_sync_config.required_state_map, request_required_state_map=room_sync_config.required_state_map, previously_returned_user_state=previously_returned_user_state, - required_user_state=lazy_load_user_ids, + lazy_load_user_ids=lazy_load_user_ids, state_deltas=room_state_delta_id_map, ) changed_required_state_map = changes_return.required_state_map_change @@ -1561,7 +1569,7 @@ def _required_state_changes( prev_required_state_map: Mapping[str, AbstractSet[str]], request_required_state_map: Mapping[str, AbstractSet[str]], previously_returned_user_state: Mapping[str, int | None], - required_user_state: AbstractSet[str], + lazy_load_user_ids: AbstractSet[str], state_deltas: StateMap[str], ) -> _RequiredStateChangesReturn: """Calculates the changes between the required state room config from the @@ -1598,7 +1606,7 @@ def _required_state_changes( if event_type != EventTypes.Member: continue - if state_key in required_user_state: + if state_key in lazy_load_user_ids: # We're returning this member change. continue @@ -1612,7 +1620,7 @@ def _required_state_changes( if prev_required_state_map == request_required_state_map: # There has been no change in state, just need to check lazy members. newly_returned_lazy_members = ( - required_user_state - previously_returned_user_state.keys() + lazy_load_user_ids - previously_returned_user_state.keys() ) if newly_returned_lazy_members: # There are some new lazy members we need to fetch. @@ -1724,7 +1732,7 @@ def _required_state_changes( # ...or b) if we are going to send the delta down in this # sync. - if state_key in required_user_state: + if state_key in lazy_load_user_ids: lazy_members_previously_returned.add(state_key) changes[event_type] = request_state_keys @@ -1811,7 +1819,7 @@ def _required_state_changes( # We also need to pull out any lazy members that are now required but # haven't previously been returned. for required_user_id in ( - required_user_state + lazy_load_user_ids - previously_returned_user_state.keys() - lazy_members_previously_returned ): diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 5750700db48..e14e9e02bce 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3835,7 +3835,7 @@ class RequiredStateChangesTestParameters: expected_without_state_deltas: _RequiredStateChangesReturn previously_returned_user_state: AbstractSet[str] = frozenset() - required_user_state: AbstractSet[str] = frozenset() + lazy_load_user_ids: AbstractSet[str] = frozenset() class RequiredStateChangesTestCase(unittest.TestCase): @@ -4371,7 +4371,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_user_state={"@user2:test", "@user3:test"}, - required_user_state=set(), + lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # The requested required state hasn't changed @@ -4410,7 +4410,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_user_state={"@user2:test", "@user3:test"}, - required_user_state={"@user4:test"}, + lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # The requested required state hasn't changed @@ -4447,7 +4447,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_user_state=frozenset(), - required_user_state={"@user3:test"}, + lazy_load_user_ids={"@user3:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the @@ -4479,7 +4479,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={}, previously_returned_user_state={"@user2:test", "@user3:test"}, - required_user_state=set(), + lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Remove `EventTypes.Member` since there's been a change to that @@ -4513,7 +4513,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {"@user4:test"}}, previously_returned_user_state={"@user2:test", "@user3:test"}, - required_user_state={"@user4:test"}, + lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed @@ -4688,7 +4688,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_user_state=previously_returned_user_state, - required_user_state=test_parameters.required_user_state, + lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas={}, ) @@ -4719,7 +4719,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_user_state=previously_returned_user_state, - required_user_state=test_parameters.required_user_state, + lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas=test_parameters.state_deltas, ) @@ -4783,7 +4783,7 @@ def test_limit_retained_previous_state_keys( prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, previously_returned_user_state={}, - required_user_state=frozenset(), + lazy_load_user_ids=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change @@ -4856,7 +4856,7 @@ def test_request_more_state_keys_than_remember_limit(self) -> None: prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, previously_returned_user_state={}, - required_user_state=frozenset(), + lazy_load_user_ids=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change From 8a3ec2050682a779b3d0a537d4e1457a4d7d7482 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 14:15:11 +0000 Subject: [PATCH 09/56] Reword the cache comments on the schema --- .../schema/main/delta/93/02_sliding_sync_members.sql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql index 914a43cb24d..6521c955ea9 100644 --- a/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql +++ b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql @@ -14,7 +14,7 @@ -- Tracks which member states have been sent to the client for lazy-loaded -- members in sliding sync. This is a *cache* as it doesn't matter if we send --- down members we've previously sent down. +-- down members we've previously sent down, i.e. it's safe to delete any rows. -- -- We track a *rough* `last_seen_ts` for each user in each room which indicates -- when we last would've sent their member state to the client. This is used so @@ -34,8 +34,8 @@ -- for a specific position. -- -- When invalidating rows, we can just delete them. Technically this could --- incorrectly invalidate for a forked position, but this is acceptable as it's --- just a cache. +-- invalidate for a forked position, but this is acceptable as equivalent to a +-- cache eviction. CREATE TABLE sliding_sync_connection_lazy_members ( connection_key BIGINT NOT NULL REFERENCES sliding_sync_connections(connection_key) ON DELETE CASCADE, connection_position BIGINT REFERENCES sliding_sync_connection_positions(connection_position) ON DELETE CASCADE, From fc0174010e3b50e39a3b6c8291ceef48b5144367 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 15:09:21 +0000 Subject: [PATCH 10/56] Rename RoomLazyMembershipChanges fields --- synapse/handlers/sliding_sync/__init__.py | 4 ++-- synapse/storage/databases/main/sliding_sync.py | 7 +++++-- synapse/types/handlers/sliding_sync.py | 8 +++++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index a5dbec9b22b..cd62ed8a29b 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1185,7 +1185,7 @@ async def get_room_sync_data( if lazy_load_room_members: returned_users = dict.fromkeys(lazy_load_user_ids) new_connection_state.room_lazy_membership[room_id] = RoomLazyMembershipChanges( - returned=returned_users + returned_user_id_to_last_seen_ts_map=returned_users ) if initial: @@ -1238,7 +1238,7 @@ async def get_room_sync_data( new_connection_state.room_lazy_membership[ room_id - ].invalidated = changes_return.lazy_members_invalidated + ].invalidated_user_ids = changes_return.lazy_members_invalidated if changes_return.added_state_filter: # Some state entries got added, so we pull out the current diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index fbe5b31d43c..4f7a552a7f5 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -574,7 +574,10 @@ def _persist_sliding_sync_connection_lazy_members_txn( # we want to update it. to_update: list[tuple[str, str]] = [] for room_id, room_changes in all_changes.items(): - for user_id, last_seen_ts in room_changes.returned.items(): + for ( + user_id, + last_seen_ts, + ) in room_changes.returned_user_id_to_last_seen_ts_map.items(): if last_seen_ts is None: # We've never sent this user before, so we need to record that # we've sent it at the new connection position. @@ -616,7 +619,7 @@ def _persist_sliding_sync_connection_lazy_members_txn( # Remove any invlalidated entries. to_remove: list[tuple[str, str]] = [] for room_id, room_changes in all_changes.items(): - for user_id in room_changes.invalidated: + for user_id in room_changes.invalidated_user_ids: to_remove.append((room_id, user_id)) if to_remove: diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 37dc92d1a58..8adadbddcb1 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -905,14 +905,16 @@ class RoomLazyMembershipChanges: # been lazily loaded. I.e. that either a) we sent those memberships down, or # b) we did so previously. The timestamp indicates the time we previously # saw the membership. - returned: Mapping[str, int | None] = attr.Factory(dict) + returned_user_id_to_last_seen_ts_map: Mapping[str, int | None] = attr.Factory(dict) # A set of user IDs whose membership change we have *not* sent # down - invalidated: AbstractSet[str] = attr.Factory(set) + invalidated_user_ids: AbstractSet[str] = attr.Factory(set) def __bool__(self) -> bool: - return bool(self.returned or self.invalidated) + return bool( + self.returned_user_id_to_last_seen_ts_map or self.invalidated_user_ids + ) @attr.s(auto_attribs=True) From ae3f5699c460dd726a17ca93da225915e72166d1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 15:10:37 +0000 Subject: [PATCH 11/56] Add RoomLazyMembershipChanges last_seen_ts comment --- synapse/types/handlers/sliding_sync.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 8adadbddcb1..4fa322364fa 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -905,6 +905,11 @@ class RoomLazyMembershipChanges: # been lazily loaded. I.e. that either a) we sent those memberships down, or # b) we did so previously. The timestamp indicates the time we previously # saw the membership. + # + # We track a *rough* `last_seen_ts` for each user in each room which + # indicates when we last would've sent their member state to the client. + # This is used so that we can remove members which haven't been seen for a + # while to save space. returned_user_id_to_last_seen_ts_map: Mapping[str, int | None] = attr.Factory(dict) # A set of user IDs whose membership change we have *not* sent From 027b422d759487acbcdc32eaaf50fac8f5e71cd4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 15:12:56 +0000 Subject: [PATCH 12/56] Clean up comments --- tests/handlers/test_sliding_sync.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index e14e9e02bce..183cbba15b8 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4374,7 +4374,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( - # The requested required state hasn't changed + # The `request_required_state_map` hasn't changed None, # We don't need to request anything more if they are requesting # less state now @@ -4388,7 +4388,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( - # The requested required state hasn't changed + # The `request_required_state_map` hasn't changed None, # We don't need to request anything more if they are requesting # less state now @@ -4413,7 +4413,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( - # The requested required state hasn't changed + # The `request_required_state_map` hasn't changed None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), @@ -4427,7 +4427,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( - # The requested required state hasn't changed + # The `request_required_state_map` hasn't changed None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), @@ -4454,6 +4454,10 @@ class RequiredStateChangesTestCase(unittest.TestCase): # changed required state config. {EventTypes.Member: {StateValues.LAZY}}, # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. StateFilter.none(), # Remember the fact that we've sent @user3 down before, # but not @user2 as that has been invalidated. @@ -4464,6 +4468,10 @@ class RequiredStateChangesTestCase(unittest.TestCase): # changed required state config. {EventTypes.Member: {StateValues.LAZY}}, # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. StateFilter.none(), # Remember the fact that we've sent the users down before. lazy_members_previously_returned={"@user2:test", "@user3:test"}, From abee4db1abfe068a224a72db890ab3eba7f88932 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 16:15:09 +0000 Subject: [PATCH 13/56] Always include lazy_members_previously_returned and lazy_members_previously_returned in tests --- tests/handlers/test_sliding_sync.py | 40 ++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 183cbba15b8..8e78dfffd92 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4379,6 +4379,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # Remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we should persist the changed to required state. That way next @@ -4393,6 +4396,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # Nothing should change (we should still keep track that # we've sent specific `EventTypes.Member` before). lazy_members_invalidated=frozenset(), @@ -4417,6 +4423,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # Remove "@user2:test" since that state has changed and # is no longer being requested anymore. Since something # was removed, we also should persist the changed to @@ -4431,6 +4440,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): None, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # We don't invalidate user2 as they haven't changed lazy_members_invalidated=frozenset(), ), @@ -4462,6 +4474,8 @@ class RequiredStateChangesTestCase(unittest.TestCase): # Remember the fact that we've sent @user3 down before, # but not @user2 as that has been invalidated. lazy_members_previously_returned={"@user3:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), ), expected_without_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the @@ -4475,6 +4489,8 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Remember the fact that we've sent the users down before. lazy_members_previously_returned={"@user2:test", "@user3:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), ), ), ), @@ -4496,7 +4512,18 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We don't need to request anything more if they are requesting # less state now StateFilter.none(), - # Explicitly remove the now invalidated @user2:test membership. + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), + # Explicitly remove the now invalidated @user2:test + # membership. + # + # We don't invalidate @user3:test as that membership + # hasn't changed. We continue to store the existing lazy + # members since they might be useful for future + # requests. (Alternatively, we could invalidate all + # members in the room when the client stops lazy + # loading, but we opt to keep track of them). lazy_members_invalidated={"@user2:test"}, ), expected_without_state_deltas=_RequiredStateChangesReturn( @@ -4507,6 +4534,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): # We don't need to request anything more if they are requesting # less state now StateFilter.none(), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # Nothing has been invalidated. lazy_members_invalidated=frozenset(), ), @@ -4529,6 +4559,9 @@ class RequiredStateChangesTestCase(unittest.TestCase): {EventTypes.Member: {"@user4:test"}}, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), # Also remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we also should persist the changed to required state. That way next @@ -4543,6 +4576,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): {EventTypes.Member: {"@user4:test"}}, # We should see the new state_keys added StateFilter.from_types([(EventTypes.Member, "@user4:test")]), + # Previous request did not include any explicit members, + # so nothing to store. + lazy_members_previously_returned=frozenset(), + # We don't invalidate user2 as they haven't changed + lazy_members_invalidated=frozenset(), ), ), ), From 99855badeb8509f8e529f8d842ee6f2ee4b7d7d5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 16:40:58 +0000 Subject: [PATCH 14/56] Fixup comment --- synapse/handlers/sliding_sync/__init__.py | 5 ++++- synapse/storage/databases/main/sliding_sync.py | 2 +- tests/rest/client/sliding_sync/test_rooms_required_state.py | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index cd62ed8a29b..994e279cd0b 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1098,6 +1098,8 @@ async def get_room_sync_data( # have been included already (this can only # happen if membership state was rolled back due # to state resolution anyway). + # + # `None` is a wildcard in the `StateFilter` required_state_types.append((EventTypes.Member, None)) # Record the extra members we're returning. @@ -1607,7 +1609,8 @@ def _required_state_changes( continue if state_key in lazy_load_user_ids: - # We're returning this member change. + # Because it's part of the `required_user_state`, we're going to + # send this member change down. continue if state_key not in previously_returned_user_state: diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 4f7a552a7f5..dc2c90c6cc6 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -616,7 +616,7 @@ def _persist_sliding_sync_connection_lazy_members_txn( sql = sql.format(value_placeholder="(?, ?, ?, ?, ?)") txn.execute_batch(sql, args) - # Remove any invlalidated entries. + # Remove any invalidated entries. to_remove: list[tuple[str, str]] = [] for room_id, room_changes in all_changes.items(): for user_id in room_changes.invalidated_user_ids: diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 44698302eb8..e64ebf33af7 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -854,7 +854,8 @@ def test_lazy_members_limited_sync(self) -> None: self.helper.send(room_id1, "1", tok=user1_tok) self.helper.send(room_id1, "2", tok=user2_tok) - # Make a first sync to establish a position + # Make a first sync with lazy loading for the room members to establish + # a position sync_body = { "lists": { "foo-list": { From 0b1ecf1c711d7c643ced61c2d82524a8effb0ab1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 24 Nov 2025 16:48:21 +0000 Subject: [PATCH 15/56] Use duration constants --- synapse/storage/databases/main/sliding_sync.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index dc2c90c6cc6..e3836381454 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -38,6 +38,7 @@ RoomSyncConfig, ) from synapse.util.caches.descriptors import cached +from synapse.util.constants import MILLISECONDS_PER_SECOND, ONE_HOUR_SECONDS from synapse.util.json import json_encoder if TYPE_CHECKING: @@ -582,7 +583,7 @@ def _persist_sliding_sync_connection_lazy_members_txn( # We've never sent this user before, so we need to record that # we've sent it at the new connection position. to_update.append((room_id, user_id)) - elif last_seen_ts + 60 * 60 * 1000 < now: + elif last_seen_ts + ONE_HOUR_SECONDS * MILLISECONDS_PER_SECOND < now: # We last saw this user over an hour ago, so we update the # timestamp. to_update.append((room_id, user_id)) From 2090d143afabbaadfc6e965fae4573de99142d40 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 09:54:13 +0000 Subject: [PATCH 16/56] Update tests/handlers/test_sliding_sync.py Co-authored-by: Eric Eastwood --- tests/handlers/test_sliding_sync.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 8e78dfffd92..41fb58009e6 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4443,7 +4443,6 @@ class RequiredStateChangesTestCase(unittest.TestCase): # Previous request did not include any explicit members, # so nothing to store. lazy_members_previously_returned=frozenset(), - # We don't invalidate user2 as they haven't changed lazy_members_invalidated=frozenset(), ), ), From 113f6ce8a9416fe72ce384c86d82377222db1622 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 10:08:55 +0000 Subject: [PATCH 17/56] Rename previously_returned_user_state param --- synapse/handlers/sliding_sync/__init__.py | 26 +++++++++++----------- tests/handlers/test_sliding_sync.py | 27 +++++++++-------------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 994e279cd0b..cb06bc89241 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1216,7 +1216,7 @@ async def get_room_sync_data( # the case where the client added explicit user state requests # for users they already had lazy loaded. if all_required_user_state: - previously_returned_user_state = ( + previously_returned_user_to_last_seen = ( await self.store.get_sliding_sync_connection_lazy_members( connection_position=from_token.connection_position, room_id=room_id, @@ -1224,7 +1224,7 @@ async def get_room_sync_data( ) ) else: - previously_returned_user_state = {} + previously_returned_user_to_last_seen = {} # Check if there are any changes to the required state config # that we need to handle. @@ -1232,7 +1232,7 @@ async def get_room_sync_data( user.to_string(), prev_required_state_map=prev_room_sync_config.required_state_map, request_required_state_map=room_sync_config.required_state_map, - previously_returned_user_state=previously_returned_user_state, + previously_returned_lazy_user_ids=previously_returned_user_to_last_seen.keys(), lazy_load_user_ids=lazy_load_user_ids, state_deltas=room_state_delta_id_map, ) @@ -1570,7 +1570,7 @@ def _required_state_changes( *, prev_required_state_map: Mapping[str, AbstractSet[str]], request_required_state_map: Mapping[str, AbstractSet[str]], - previously_returned_user_state: Mapping[str, int | None], + previously_returned_lazy_user_ids: AbstractSet[str], lazy_load_user_ids: AbstractSet[str], state_deltas: StateMap[str], ) -> _RequiredStateChangesReturn: @@ -1593,9 +1593,11 @@ def _required_state_changes( request. request_required_state_map: The required state map from the current request. - previously_returned_user_state: The set of user IDs whose lazy-loaded - membership we have previously returned to the client. - required_user_state: The set of user IDs whose lazy-loaded membership + previously_returned_lazy_user_ids: The set of user IDs whose membership + we have previously returned to the client due to lazy loading. This + is filtered to only include users who have either sent events in the + timeline or required state. + lazy_load_user_ids: The set of user IDs whose lazy-loaded membership is required for this request. state_deltas: The state deltas that have changed in the room since the previous request. @@ -1613,7 +1615,7 @@ def _required_state_changes( # send this member change down. continue - if state_key not in previously_returned_user_state: + if state_key not in previously_returned_lazy_user_ids: # We've not previously returned this member so nothing to # invalidate. continue @@ -1622,9 +1624,7 @@ def _required_state_changes( if prev_required_state_map == request_required_state_map: # There has been no change in state, just need to check lazy members. - newly_returned_lazy_members = ( - lazy_load_user_ids - previously_returned_user_state.keys() - ) + newly_returned_lazy_members = lazy_load_user_ids - previously_returned_lazy_user_ids if newly_returned_lazy_members: # There are some new lazy members we need to fetch. added_types: list[tuple[str, str | None]] = [] @@ -1812,7 +1812,7 @@ def _required_state_changes( # not membership. pass elif event_type == EventTypes.Member: - if state_key not in previously_returned_user_state: + if state_key not in previously_returned_lazy_user_ids: # Only add *explicit* members we haven't previously sent # down. added.append((event_type, state_key)) @@ -1823,7 +1823,7 @@ def _required_state_changes( # haven't previously been returned. for required_user_id in ( lazy_load_user_ids - - previously_returned_user_state.keys() + - previously_returned_lazy_user_ids - lazy_members_previously_returned ): added.append((EventTypes.Member, required_user_id)) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 41fb58009e6..bb76c4b2324 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3834,7 +3834,7 @@ class RequiredStateChangesTestParameters: expected_with_state_deltas: _RequiredStateChangesReturn expected_without_state_deltas: _RequiredStateChangesReturn - previously_returned_user_state: AbstractSet[str] = frozenset() + previously_returned_lazy_user_ids: AbstractSet[str] = frozenset() lazy_load_user_ids: AbstractSet[str] = frozenset() @@ -4370,7 +4370,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): RequiredStateChangesTestParameters( previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, - previously_returned_user_state={"@user2:test", "@user3:test"}, + previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( @@ -4415,7 +4415,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): RequiredStateChangesTestParameters( previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, - previously_returned_user_state={"@user2:test", "@user3:test"}, + previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( @@ -4457,7 +4457,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): EventTypes.Member: {"@user2:test", "@user3:test"} }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, - previously_returned_user_state=frozenset(), + previously_returned_lazy_user_ids=frozenset(), lazy_load_user_ids={"@user3:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( @@ -4501,7 +4501,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): RequiredStateChangesTestParameters( previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={}, - previously_returned_user_state={"@user2:test", "@user3:test"}, + previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( @@ -4549,7 +4549,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): RequiredStateChangesTestParameters( previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {"@user4:test"}}, - previously_returned_user_state={"@user2:test", "@user3:test"}, + previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( @@ -4720,19 +4720,12 @@ def test_xxx( _test_description: str, test_parameters: RequiredStateChangesTestParameters, ) -> None: - # `_required_state_changes` expects this to be a map from user ID to - # timestamp, so let's convert it into one by using a dummy timestamp - # value - previously_returned_user_state = dict.fromkeys( - test_parameters.previously_returned_user_state, 1234 - ) - # Without `state_deltas` state_changes = _required_state_changes( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, - previously_returned_user_state=previously_returned_user_state, + previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas={}, ) @@ -4763,7 +4756,7 @@ def test_xxx( user_id="@user:test", prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, - previously_returned_user_state=previously_returned_user_state, + previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas=test_parameters.state_deltas, ) @@ -4827,7 +4820,7 @@ def test_limit_retained_previous_state_keys( user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, - previously_returned_user_state={}, + previously_returned_lazy_user_ids=frozenset(), lazy_load_user_ids=frozenset(), state_deltas={}, ) @@ -4900,7 +4893,7 @@ def test_request_more_state_keys_than_remember_limit(self) -> None: user_id="@user:test", prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, - previously_returned_user_state={}, + previously_returned_lazy_user_ids=frozenset(), lazy_load_user_ids=frozenset(), state_deltas={}, ) From ec45e0006f896775033094e6318c42c295b3ecf8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 11:06:41 +0000 Subject: [PATCH 18/56] Fix bug where we didn't correctly filter lazy members by room When fetching previously sent lazy members we didn't filter by room, which meant that we didn't send down member events in a room if we'd previously sent that user's member event in another room. --- .../storage/databases/main/sliding_sync.py | 16 ++-- .../sliding_sync/test_rooms_required_state.py | 86 +++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index e3836381454..31561331a0f 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -25,6 +25,7 @@ DatabasePool, LoggingDatabaseConnection, LoggingTransaction, + make_in_list_sql_clause, ) from synapse.storage.engines import PostgresEngine from synapse.types import MultiWriterStreamToken, RoomStreamToken @@ -532,14 +533,17 @@ async def get_sliding_sync_connection_lazy_members( def get_sliding_sync_connection_lazy_members_txn( txn: LoggingTransaction, ) -> Mapping[str, int]: - sql = """ - SELECT user_id, mem.connection_position, last_seen_ts - FROM sliding_sync_connection_positions AS pos - INNER JOIN sliding_sync_connection_lazy_members AS mem USING (connection_key) - WHERE pos.connection_position = ? + user_clause, user_args = make_in_list_sql_clause( + txn.database_engine, "user_id", user_ids + ) + + sql = f""" + SELECT user_id, connection_position, last_seen_ts + FROM sliding_sync_connection_lazy_members AS pos + WHERE room_id = ? AND {user_clause} """ - txn.execute(sql, (connection_position,)) + txn.execute(sql, (room_id, *user_args)) # Filter out any cache entries that only apply to forked connection # positions. Entries with `NULL` connection position apply to all diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index e64ebf33af7..482bff4c884 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -932,6 +932,92 @@ def test_lazy_members_limited_sync(self) -> None: exact=True, ) + def test_lazy_members_across_multiple_rooms(self) -> None: + """Test that lazy loading room members are tracked per-room correctly. + """ + + 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") + + # Create two rooms with both users in them and send a message in each + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.send(room_id1, "room1-msg1", tok=user2_tok) + + room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id2, user1_id, tok=user1_tok) + self.helper.send(room_id2, "room2-msg1", tok=user2_tok) + + # Make a sync with lazy loading for the room members to establish + # a position + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # We expect to see only user2's membership in both rooms + 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.Member, user2_id)], + }, + exact=True, + ) + + # Send a message in room1 from user1 + self.helper.send(room_id1, "room1-msg2", tok=user1_tok) + + # Make an incremental Sliding Sync request and check that we get user1's + # membership. + response_body, from_token = self.do_sync( + sync_body, since=from_token, 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.Member, user1_id)], + }, + exact=True, + ) + + # Send a message in room2 from user1 + self.helper.send(room_id2, "room2-msg2", tok=user1_tok) + + # Make an incremental Sliding Sync request and check that we get user1's + # membership. + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id2) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id2]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + + + def test_rooms_required_state_me(self) -> None: """ Test `rooms.required_state` correctly handles $ME. From f8f6dc97d8c5bbd85231763915f3a5f80b7d0239 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 11:21:05 +0000 Subject: [PATCH 19/56] Lint --- synapse/handlers/sliding_sync/__init__.py | 4 +++- tests/rest/client/sliding_sync/test_rooms_required_state.py | 5 +---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index cb06bc89241..139479790d9 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1624,7 +1624,9 @@ def _required_state_changes( if prev_required_state_map == request_required_state_map: # There has been no change in state, just need to check lazy members. - newly_returned_lazy_members = lazy_load_user_ids - previously_returned_lazy_user_ids + newly_returned_lazy_members = ( + lazy_load_user_ids - previously_returned_lazy_user_ids + ) if newly_returned_lazy_members: # There are some new lazy members we need to fetch. added_types: list[tuple[str, str | None]] = [] diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 482bff4c884..7400314ab7d 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -933,8 +933,7 @@ def test_lazy_members_limited_sync(self) -> None: ) def test_lazy_members_across_multiple_rooms(self) -> None: - """Test that lazy loading room members are tracked per-room correctly. - """ + """Test that lazy loading room members are tracked per-room correctly.""" user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1016,8 +1015,6 @@ def test_lazy_members_across_multiple_rooms(self) -> None: exact=True, ) - - def test_rooms_required_state_me(self) -> None: """ Test `rooms.required_state` correctly handles $ME. From 5604d3ad9d04aa75c4df5f441bb64b20f2b0d4df Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 11:28:27 +0000 Subject: [PATCH 20/56] Add test for forked position --- .../sliding_sync/test_rooms_required_state.py | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 7400314ab7d..bd3052e3051 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -1015,6 +1015,78 @@ def test_lazy_members_across_multiple_rooms(self) -> None: exact=True, ) + def test_lazy_members_forked_position(self) -> None: + """Test that lazy loading room members are tracked correctly when a + connection position is reused""" + + 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) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + self.helper.send(room_id1, "1", tok=user2_tok) + + # Make a sync with lazy loading for the room members to establish + # a position + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # We expect to see only user2's membership in the room + 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.Member, user2_id)], + }, + exact=True, + ) + + # Send a message in room1 from user1 + self.helper.send(room_id1, "2", tok=user1_tok) + + # Make an incremental Sliding Sync request and check that we get user1's + # membership. + response_body, _ = self.do_sync(sync_body, since=from_token, 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.Member, user1_id)], + }, + exact=True, + ) + + # Now, reuse the original position and check we still get user1's + # membership. + response_body, _ = self.do_sync(sync_body, since=from_token, 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.Member, user1_id)], + }, + exact=True, + ) + def test_rooms_required_state_me(self) -> None: """ Test `rooms.required_state` correctly handles $ME. From 815b8522b0101e807e650d81ba524826c1383960 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 11:57:08 +0000 Subject: [PATCH 21/56] Ensure that the last_seen_ts is correctly updated --- synapse/handlers/sliding_sync/__init__.py | 15 +- .../storage/databases/main/sliding_sync.py | 16 ++- .../sliding_sync/test_rooms_required_state.py | 129 ++++++++++++++++++ 3 files changed, 154 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 139479790d9..ad2d101c9fb 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1183,11 +1183,11 @@ async def get_room_sync_data( # loaded members is enabled. # # We may later update this to account for previously sent members. - returned_users = {} + returned_user_id_to_last_seen_ts_map = {} if lazy_load_room_members: - returned_users = dict.fromkeys(lazy_load_user_ids) + returned_user_id_to_last_seen_ts_map = dict.fromkeys(lazy_load_user_ids) new_connection_state.room_lazy_membership[room_id] = RoomLazyMembershipChanges( - returned_user_id_to_last_seen_ts_map=returned_users + returned_user_id_to_last_seen_ts_map=returned_user_id_to_last_seen_ts_map ) if initial: @@ -1223,6 +1223,15 @@ async def get_room_sync_data( user_ids=all_required_user_state, ) ) + + # Update the room lazy membership changes to track which + # lazy loaded members were needed for this sync. This is so + # that we can correctly track the last time we sent down + # users' membership (and so can evict old membership state + # from the DB tables). + returned_user_id_to_last_seen_ts_map.update( + previously_returned_user_to_last_seen + ) else: previously_returned_user_to_last_seen = {} diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 31561331a0f..989be3c9d45 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -49,6 +49,11 @@ logger = logging.getLogger(__name__) +# How often to update the last seen timestamp for lazy members. We don't want to +# update it too often as that causes DB writes. +LAZY_MEMBERS_UPDATE_INTERVAL_MS = ONE_HOUR_SECONDS * MILLISECONDS_PER_SECOND + + class SlidingSyncStore(SQLBaseStore): def __init__( self, @@ -577,6 +582,10 @@ def _persist_sliding_sync_connection_lazy_members_txn( # These are either a) new entries we've never sent before (i.e. with a # None last_seen_ts), or b) where the `last_seen_ts` is old enough that # we want to update it. + # + # We don't update the timestamp every time to avoid hammering the DB + # with writes, and we don't need the timestamp to be precise. It is used + # to evict old entries that haven't been used in a while. to_update: list[tuple[str, str]] = [] for room_id, room_changes in all_changes.items(): for ( @@ -587,9 +596,10 @@ def _persist_sliding_sync_connection_lazy_members_txn( # We've never sent this user before, so we need to record that # we've sent it at the new connection position. to_update.append((room_id, user_id)) - elif last_seen_ts + ONE_HOUR_SECONDS * MILLISECONDS_PER_SECOND < now: - # We last saw this user over an hour ago, so we update the - # timestamp. + elif last_seen_ts + LAZY_MEMBERS_UPDATE_INTERVAL_MS < now: + # We last saw this user over + # `LAZY_MEMBERS_UPDATE_INTERVAL_MS` ago, so we update the + # timestamp (c.f. comment above). to_update.append((room_id, user_id)) if to_update: diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index bd3052e3051..fde3b2c34ba 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -23,7 +23,10 @@ from synapse.handlers.sliding_sync import StateValues from synapse.rest.client import knock, login, room, sync from synapse.server import HomeServer +from synapse.storage.databases.main.sliding_sync import LAZY_MEMBERS_UPDATE_INTERVAL_MS +from synapse.types import SlidingSyncStreamToken from synapse.util.clock import Clock +from synapse.util.constants import MILLISECONDS_PER_SECOND from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase from tests.test_utils.event_injection import mark_event_as_partial_state @@ -1932,3 +1935,129 @@ def test_rooms_required_state_expand_deduplicate(self) -> None: # 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")) + + def test_lazy_loaded_last_seen_ts(self) -> None: + """Test that the `last_seen_ts` column in + `sliding_sync_connection_lazy_members` is correctly kept up to date""" + + 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_id = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) + self.helper.join(room_id, user1_id, tok=user1_tok) + + # Send a message so that user1 comes down sync. + self.helper.send(room_id, "msg", tok=user1_tok) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # Check that user1 is returned + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + + # Check that we have an entry in sliding_sync_connection_lazy_members + connection_pos1 = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ).connection_position + lazy_member_entries = self.get_success( + self.store.get_sliding_sync_connection_lazy_members( + connection_pos1, room_id, {user1_id} + ) + ) + self.assertIn(user1_id, lazy_member_entries) + + prev_timestamp = lazy_member_entries[user1_id] + + # If user1 is sent down again, the last_seen_ts should NOT be updated as + # not enough time has passed. + self.helper.send(room_id, "msg2", tok=user1_tok) + + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We expect the required_state map to be empty as nothing has changed. + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id].get("required_state", []), + {}, + exact=True, + ) + + connection_pos2 = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ).connection_position + + lazy_member_entries = self.get_success( + self.store.get_sliding_sync_connection_lazy_members( + connection_pos2, room_id, {user1_id} + ) + ) + + # The timestamp should be unchanged. + self.assertEqual(lazy_member_entries[user1_id], prev_timestamp) + + # Now advance the time by `LAZY_MEMBERS_UPDATE_INTERVAL_MS` so that we + # would update the timestamp. + self.reactor.advance(LAZY_MEMBERS_UPDATE_INTERVAL_MS / MILLISECONDS_PER_SECOND) + + # Send a message from user2 + self.helper.send(room_id, "msg3", tok=user2_tok) + + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + connection_pos3 = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ).connection_position + + lazy_member_entries = self.get_success( + self.store.get_sliding_sync_connection_lazy_members( + connection_pos3, room_id, {user1_id} + ) + ) + + # The timestamp for user1 should be unchanged, as they were not sent down. + self.assertEqual(lazy_member_entries[user1_id], prev_timestamp) + + # If user1 sends a message, then the timestamp should be updated. + self.helper.send(room_id, "msg4", tok=user1_tok) + + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + connection_pos4 = self.get_success( + SlidingSyncStreamToken.from_string(self.store, from_token) + ).connection_position + + lazy_member_entries = self.get_success( + self.store.get_sliding_sync_connection_lazy_members( + connection_pos4, room_id, {user1_id} + ) + ) + # The timestamp for user1 should be updated. + self.assertGreater(lazy_member_entries[user1_id], prev_timestamp) From 2e844aab8039793023f4f8aaec58ca00a5d2c158 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 14:23:01 +0000 Subject: [PATCH 22/56] Expand comment why is fine this is a cache --- synapse/storage/databases/main/sliding_sync.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 989be3c9d45..1002ae74097 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -608,7 +608,9 @@ def _persist_sliding_sync_connection_lazy_members_txn( # Ignore conflicts where the existing entry has a different # connection position (i.e. from a forked connection position). This # may mean that we lose some updates, but that's acceptable as this - # is a cache and its fine for it to *not* include rows. + # is a cache and its fine for it to *not* include rows. (Downstream + # this will cause us to maybe send a few extra lazy members down + # sync, but we're allowed to send extra members). sql = """ INSERT INTO sliding_sync_connection_lazy_members (connection_key, connection_position, room_id, user_id, last_seen_ts) @@ -639,7 +641,9 @@ def _persist_sliding_sync_connection_lazy_members_txn( if to_remove: # We don't try and match on connection position here: it's fine to - # remove it from all forks. + # remove it from all forks. This is a cache so it's fine to expire + # arbitrary entries, the worst that happens is we send a few extra + # lazy members down sync. self.db_pool.simple_delete_many_batch_txn( txn, table="sliding_sync_connection_lazy_members", From deaf995a62f46ef3e109581f6df3e3e1f3642449 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 25 Nov 2025 15:21:06 +0000 Subject: [PATCH 23/56] Add tests for state reset and lazy loading --- .../sliding_sync/test_rooms_required_state.py | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index fde3b2c34ba..aa13273e668 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -23,6 +23,7 @@ from synapse.handlers.sliding_sync import StateValues from synapse.rest.client import knock, login, room, sync from synapse.server import HomeServer +from synapse.storage.databases.main.events import DeltaState, SlidingSyncTableChanges from synapse.storage.databases.main.sliding_sync import LAZY_MEMBERS_UPDATE_INTERVAL_MS from synapse.types import SlidingSyncStreamToken from synapse.util.clock import Clock @@ -2061,3 +2062,128 @@ def test_lazy_loaded_last_seen_ts(self) -> None: ) # The timestamp for user1 should be updated. self.assertGreater(lazy_member_entries[user1_id], prev_timestamp) + + def test_lazy_load_state_reset(self) -> None: + """Test that when using lazy-loaded members, if a membership state is + reset to a previous state and the sync is not limited, then we send down + the state reset. + """ + + 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_id = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) + content = self.helper.join(room_id, user1_id, tok=user1_tok) + first_event_id = content["event_id"] + + # Send a message so that user1 comes down sync. + self.helper.send(room_id, "msg", tok=user1_tok) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # Check that user1 is returned + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + + # user1 changes their display name + content = self.helper.send_state( + room_id, + EventTypes.Member, + body={"membership": "join", "displayname": "New display name"}, + state_key=user1_id, + tok=user1_tok, + ) + second_event_id = content["event_id"] + + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see the updated membership state + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + self.assertEqual( + response_body["rooms"][room_id]["required_state"][0]["event_id"], + second_event_id, + ) + + # Now, fake a reset the membership state to the first event + persist_event_store = self.hs.get_datastores().persist_events + assert persist_event_store is not None + + self.get_success( + persist_event_store.update_current_state( + room_id, + DeltaState( + to_insert={(EventTypes.Member, user1_id): first_event_id}, + to_delete=[], + ), + # We don't need to worry about sliding sync changes for this test + SlidingSyncTableChanges( + room_id=room_id, + joined_room_bump_stamp_to_fully_insert=None, + joined_room_updates={}, + membership_snapshot_shared_insert_values={}, + to_insert_membership_snapshots=[], + to_delete_membership_snapshots=[], + ), + ) + ) + + # Send a message from *user2* so that user1 wouldn't normally get + # synced. + self.helper.send(room_id, "msg2", tok=user2_tok) + + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # This should be a non-limited sync + self.assertFalse( + response_body["rooms"][room_id].get("limited", False), + "Expected a non-limited timeline", + ) + + # We should see the reset membership state of user1 + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + ) + self.assertEqual( + response_body["rooms"][room_id]["required_state"][0]["event_id"], + first_event_id, + ) From 65aebf41059c27ac0d54ff07126b1e2827cbf7a2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 26 Nov 2025 10:56:26 +0000 Subject: [PATCH 24/56] Fix limited sync lazy members --- synapse/handlers/sliding_sync/__init__.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index ad2d101c9fb..d5bad75451e 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1206,8 +1206,13 @@ async def get_room_sync_data( assert from_bound is not None if prev_room_sync_config is not None: - # Define `required_user_state` as all user state we want. + # Define `required_user_state` as all user state we want, which + # is the explicitly requested members, any needed for lazy + # loading, and users whose membership has changed.s all_required_user_state = explicit_user_state | lazy_load_user_ids + for state_type, state_key in room_state_delta_id_map: + if state_type == EventTypes.Member: + all_required_user_state.add(state_key) # We need to know what user state we previously sent down the # connection so we can determine what has changed. @@ -1605,7 +1610,7 @@ def _required_state_changes( previously_returned_lazy_user_ids: The set of user IDs whose membership we have previously returned to the client due to lazy loading. This is filtered to only include users who have either sent events in the - timeline or required state. + timeline, required state or whose membership changed. lazy_load_user_ids: The set of user IDs whose lazy-loaded membership is required for this request. state_deltas: The state deltas that have changed in the room since the From e6939e7a08277e44e51cd1f7492c56d5aadfa8a2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Dec 2025 13:20:02 +0000 Subject: [PATCH 25/56] Use Duration --- synapse/storage/databases/main/sliding_sync.py | 6 +++--- .../rest/client/sliding_sync/test_rooms_required_state.py | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index ffdb7ef02a6..0ee2ef7dec1 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -52,7 +52,7 @@ # How often to update the last seen timestamp for lazy members. We don't want to # update it too often as that causes DB writes. -LAZY_MEMBERS_UPDATE_INTERVAL_MS = ONE_HOUR_SECONDS * MILLISECONDS_PER_SECOND +LAZY_MEMBERS_UPDATE_INTERVAL = Duration(hours=1) # How often to update the `last_used_ts` column on # `sliding_sync_connection_positions` when the client uses a connection @@ -635,9 +635,9 @@ def _persist_sliding_sync_connection_lazy_members_txn( # We've never sent this user before, so we need to record that # we've sent it at the new connection position. to_update.append((room_id, user_id)) - elif last_seen_ts + LAZY_MEMBERS_UPDATE_INTERVAL_MS < now: + elif last_seen_ts + LAZY_MEMBERS_UPDATE_INTERVAL.as_millis() < now: # We last saw this user over - # `LAZY_MEMBERS_UPDATE_INTERVAL_MS` ago, so we update the + # `LAZY_MEMBERS_UPDATE_INTERVAL` ago, so we update the # timestamp (c.f. comment above). to_update.append((room_id, user_id)) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index aa13273e668..a451df0be11 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -24,10 +24,9 @@ from synapse.rest.client import knock, login, room, sync from synapse.server import HomeServer from synapse.storage.databases.main.events import DeltaState, SlidingSyncTableChanges -from synapse.storage.databases.main.sliding_sync import LAZY_MEMBERS_UPDATE_INTERVAL_MS +from synapse.storage.databases.main.sliding_sync import LAZY_MEMBERS_UPDATE_INTERVAL from synapse.types import SlidingSyncStreamToken from synapse.util.clock import Clock -from synapse.util.constants import MILLISECONDS_PER_SECOND from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase from tests.test_utils.event_injection import mark_event_as_partial_state @@ -2021,9 +2020,9 @@ def test_lazy_loaded_last_seen_ts(self) -> None: # The timestamp should be unchanged. self.assertEqual(lazy_member_entries[user1_id], prev_timestamp) - # Now advance the time by `LAZY_MEMBERS_UPDATE_INTERVAL_MS` so that we + # Now advance the time by `LAZY_MEMBERS_UPDATE_INTERVAL` so that we # would update the timestamp. - self.reactor.advance(LAZY_MEMBERS_UPDATE_INTERVAL_MS / MILLISECONDS_PER_SECOND) + self.reactor.advance(LAZY_MEMBERS_UPDATE_INTERVAL.as_secs()) # Send a message from user2 self.helper.send(room_id, "msg3", tok=user2_tok) From 69fc61d6b708f506c6dee1ce3154a96f49df75f5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Dec 2025 13:33:26 +0000 Subject: [PATCH 26/56] Fix bug where lazy members were shared between connections --- .../storage/databases/main/sliding_sync.py | 19 +++- .../sliding_sync/test_rooms_required_state.py | 100 ++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 0ee2ef7dec1..dcd9bb2cf30 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -581,13 +581,24 @@ def get_sliding_sync_connection_lazy_members_txn( txn.database_engine, "user_id", user_ids ) + # Fetch all the lazy membership entries for the given connection, + # room and user IDs. We don't have the `connection_key` here, so we + # join against `sliding_sync_connection_positions` to get it. + # + # Beware that there are two `connection_position` columns in the + # query which are different, the one in + # `sliding_sync_connection_positions` is the one we match to get the + # connection_key, whereas the one in + # `sliding_sync_connection_lazy_members` is what we filter against + # (it may be null or the same as the one passed in). sql = f""" - SELECT user_id, connection_position, last_seen_ts - FROM sliding_sync_connection_lazy_members AS pos - WHERE room_id = ? AND {user_clause} + SELECT user_id, mem.connection_position, last_seen_ts + FROM sliding_sync_connection_lazy_members AS mem + INNER JOIN sliding_sync_connection_positions AS pos USING (connection_key) + WHERE pos.connection_position = ? AND room_id = ? AND {user_clause} """ - txn.execute(sql, (room_id, *user_args)) + txn.execute(sql, (connection_position, room_id, *user_args)) # Filter out any cache entries that only apply to forked connection # positions. Entries with `NULL` connection position apply to all diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index a451df0be11..fb794f90dfe 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -1018,6 +1018,106 @@ def test_lazy_members_across_multiple_rooms(self) -> None: exact=True, ) + def test_lazy_members_across_multiple_connections(self) -> None: + """Test that lazy loading room members are tracked per-connection + correctly. + + This catches bugs where if a membership got sent down one connection, + it would incorrectly assume it was sent down another connection. + """ + + 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) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + self.helper.send(room_id1, "1", tok=user2_tok) + + # Make a sync with lazy loading for the room members to establish + # a position + sync_body1 = { + "conn_id": "first-connection", + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + }, + } + response_body, from_token1 = self.do_sync(sync_body1, tok=user1_tok) + + # We expect to see only user2's membership in the room + 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.Member, user2_id)], + }, + exact=True, + ) + + # Now make a new connection + sync_body2 = { + "conn_id": "second-connection", + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 1, + } + }, + } + response_body, from_token2 = self.do_sync(sync_body2, tok=user1_tok) + + # We should see user2's membership as this is a new connection + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Member, user2_id)], + }, + exact=True, + ) + + # If we send a message from user1 and sync again on the first connection, + # we should get user1's membership + self.helper.send(room_id1, "2", tok=user1_tok) + response_body, from_token1 = self.do_sync( + sync_body1, since=from_token1, tok=user1_tok + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + + # We sync again on the first connection to "ack" the position. This + # triggers the `sliding_sync_connection_lazy_members` to set its + # connection_position to null. + self.do_sync(sync_body1, since=from_token1, tok=user1_tok) + + # If we sync again on the second connection, we should also get user1's + # membership + response_body, _ = self.do_sync(sync_body2, since=from_token2, tok=user1_tok) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Member, user1_id)], + }, + exact=True, + ) + def test_lazy_members_forked_position(self) -> None: """Test that lazy loading room members are tracked correctly when a connection position is reused""" From 56ead16df526a191b6e8c8ed0640acc748c965b2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Dec 2025 17:03:40 +0000 Subject: [PATCH 27/56] Fixup lazy_members_previously_returned --- synapse/handlers/sliding_sync/__init__.py | 88 +++++++++----- tests/handlers/test_sliding_sync.py | 107 ++++++++++++++++++ .../sliding_sync/test_rooms_required_state.py | 80 +++++++++++++ 3 files changed, 246 insertions(+), 29 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 87257179877..cec4cf4ece9 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1254,6 +1254,16 @@ async def get_room_sync_data( room_id ].invalidated_user_ids = changes_return.lazy_members_invalidated + # Add any previously returned explicit memberships to the lazy + # loaded table. This happens when a client requested explicit + # members and then converted them to lazy loading. + for user_id in changes_return.lazy_members_previously_returned: + # We don't know the right timestamp to use here, as we don't + # know the last time we would have sent the membership down. + # So we don't overwrite it if we have a timestamp already, + # and fallback to `None` (which means now) if we don't. + returned_user_id_to_last_seen_ts_map.setdefault(user_id, None) + if changes_return.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. @@ -1722,36 +1732,12 @@ def _required_state_changes( # Nothing *added*, so we skip. Removals happen below. continue - # Handle the special case of adding LAZY membership, where we want to - # remember what explicit members we've previously sent down. + # Handle the special case of adding `$LAZY` membership, where we want to + # always record the change to be lazy loading. 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 not old_state_key_lazy and request_state_key_lazy: - # We're adding a LAZY flag. We therefore add any previously - # explicit members we've sent down to lazy cache. - for state_key in old_state_keys: - if ( - state_key == StateValues.WILDCARD - or state_key == StateValues.LAZY - ): - # Ignore non-user IDs. - continue - - if state_key == StateValues.ME: - # Normalize to proper user ID - state_key = user_id - - # We remember the user if either a) they haven't been - # invalidated... - if (EventTypes.Member, state_key) not in state_deltas: - lazy_members_previously_returned.add(state_key) - - # ...or b) if we are going to send the delta down in this - # sync. - if state_key in lazy_load_user_ids: - lazy_members_previously_returned.add(state_key) - changes[event_type] = request_state_keys continue @@ -1833,12 +1819,20 @@ def _required_state_changes( else: added.append((event_type, state_key)) + previously_required_state_members = set( + prev_required_state_map.get(EventTypes.Member, ()) + ) + if StateValues.ME in previously_required_state_members: + previously_required_state_members.add(user_id) + # We also need to pull out any lazy members that are now required but # haven't previously been returned. for required_user_id in ( lazy_load_user_ids + # Remove previously returned users - previously_returned_lazy_user_ids - - lazy_members_previously_returned + # Exclude previously explicitly requested members. + - previously_required_state_members ): added.append((EventTypes.Member, required_user_id)) @@ -1882,13 +1876,24 @@ def _required_state_changes( changes[event_type] = request_state_keys continue + # When handling $LAZY membership, we want to either a) not update the + # state or b) update it to match the request. This is to avoid churn of + # the effective required state for rooms (we deduplicate required state + # between rooms), and because we can store the previously returned + # explicit memberships with the lazy loaded memberships. 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 + has_lazy = old_state_key_lazy or request_state_key_lazy + # If a "$LAZY" has been added or removed we always update. 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 + + # Or if we have lazy membership and there are invalidated + # explicit memberships. + if has_lazy and invalidated_state_keys: changes[event_type] = request_state_keys continue @@ -1903,6 +1908,31 @@ def _required_state_changes( if invalidated_state_keys: changes[event_type] = old_state_keys - invalidated_state_keys + # Check for any explicit membership changes that were removed that we can + # add to the lazy members previously returned. This is so that we don't + # return a user due to lazy loading if they were previously returned as an + # explicit membership. + membership_changes = changes.get(EventTypes.Member, set()) + if membership_changes and StateValues.LAZY in request_state_keys: + for state_key in prev_required_state_map.get(EventTypes.Member, set()): + if state_key == StateValues.WILDCARD or state_key == StateValues.LAZY: + # Ignore non-user IDs. + continue + + if state_key == StateValues.ME: + # Normalize to proper user ID + state_key = user_id + + # We remember the user if either a) they haven't been + # invalidated... + if (EventTypes.Member, state_key) not in state_deltas: + lazy_members_previously_returned.add(state_key) + + # ...or b) if we are going to send the delta down in this + # sync. + if state_key in lazy_load_user_ids: + lazy_members_previously_returned.add(state_key) + if changes: # Update the required state config based on the changes. new_required_state_map = dict(prev_required_state_map) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index bb76c4b2324..f079fb99374 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4541,6 +4541,113 @@ class RequiredStateChangesTestCase(unittest.TestCase): ), ), ), + ( + "state_key_expand_lazy_keep_previous_explicit_memberships", + """ + Test removing explicit memberships from the `required_state` + when lazy-loading room members tracks previously sent + memberships. + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + EventTypes.Member: { + StateValues.LAZY, + "@user2:test", + "@user3:test", + } + }, + request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_lazy_user_ids=frozenset(), + lazy_load_user_ids={"@user3:test"}, + state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, + expected_with_state_deltas=_RequiredStateChangesReturn( + # Since an explicit membership was removed we record the + # new required state config and move them to lazy + # members. + {EventTypes.Member: {StateValues.LAZY}}, + # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. + StateFilter.none(), + # Remember the fact that we've sent @user3 down before, + # but not @user2 as that has been invalidated. + lazy_members_previously_returned={"@user3:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + expected_without_state_deltas=_RequiredStateChangesReturn( + # While some explicit memberships were removed, there were no + # state changes, so we don't need to persist the new required + # state config yet. + None, + # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. + StateFilter.none(), + # Remember the fact that we've sent the users down before. + lazy_members_previously_returned=frozenset(), + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + ), + ), + ( + "state_key_expand_lazy_keep_previous_explicit_me_memberships", + """ + Test removing explicit $ME memberships from the `required_state` + when lazy-loading room members tracks previously sent + memberships. + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + EventTypes.Member: { + StateValues.LAZY, + "$ME", + } + }, + request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_lazy_user_ids=frozenset(), + lazy_load_user_ids={"@user:test"}, + state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, + expected_with_state_deltas=_RequiredStateChangesReturn( + # Since an explicit membership was removed we record the + # new required state config and move them to lazy + # members. + {EventTypes.Member: {StateValues.LAZY}}, + # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. + StateFilter.none(), + # Remember the fact that we've sent @user down before, + # but not @user2 as that has been invalidated. + lazy_members_previously_returned={"@user:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + expected_without_state_deltas=_RequiredStateChangesReturn( + # While some explicit memberships were removed, there were no + # state changes, so we don't need to persist the new required + # state config yet. + None, + # We have already sent @user3 down before. + # + # `@user3:test` is required for lazy loading, but we've + # already sent it down before, so we don't need to + # request it again. + StateFilter.none(), + # Remember the fact that we've sent the users down before. + lazy_members_previously_returned=frozenset(), + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + ), + ), ( "state_key_retract_lazy_keep_previous_memberships_with_new_memberships", """ diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index fb794f90dfe..8a4e83be8ef 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -1190,6 +1190,86 @@ def test_lazy_members_forked_position(self) -> None: exact=True, ) + def test_lazy_members_explicit_membership_removed(self) -> None: + """Test the case where we requested explicit memberships and then later + changed to lazy loading.""" + + 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) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + self.helper.send(room_id1, "1", tok=user2_tok) + + # Make a sync with lazy loading for the room members to establish + # a position + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Member, StateValues.ME], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # We expect to see only user1's membership in the room + 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.Member, user1_id)], + }, + exact=True, + ) + + # Now change to lazy loading... + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Member, StateValues.LAZY], + ] + + # Send a message in room1 from user2 + self.helper.send(room_id1, "2", tok=user2_tok) + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see user2's membership as it's in the timeline + 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.Member, user2_id)], + }, + exact=True, + ) + + # Now send a message in room1 from user1 + self.helper.send(room_id1, "3", tok=user1_tok) + + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + + # We should not see any memberships as we've already seen user1's + # membership. + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1].get("required_state", []), + [], + exact=True, + ) + def test_rooms_required_state_me(self) -> None: """ Test `rooms.required_state` correctly handles $ME. From 2d2047d66037551176334e5f7826e92b0f1a0004 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:35:00 +0000 Subject: [PATCH 28/56] Update synapse/storage/databases/main/sliding_sync.py Co-authored-by: Eric Eastwood --- synapse/storage/databases/main/sliding_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index dcd9bb2cf30..22eef564528 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -731,7 +731,7 @@ class PerConnectionStateDB: When persisting this *only* contains updates to the state. - The `room_lazy_membership` field is only used when persisting. + The `room_lazy_membership` field is only used when persisting (not reading from the database). """ last_used_ts: int | None From da082033fcea2885f21689795ee1c6575dbbd8ad Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:39:07 +0000 Subject: [PATCH 29/56] Update synapse/storage/databases/main/sliding_sync.py Co-authored-by: Eric Eastwood --- synapse/storage/databases/main/sliding_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 22eef564528..c14350ed4c5 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -462,7 +462,7 @@ def _get_and_clear_connection_positions_txn( # Move any lazy membership entries for this connection position to have # `NULL` connection position, indicating that it applies to all future - # positions on this connecetion. + # positions on this connection. self.db_pool.simple_update_txn( txn, table="sliding_sync_connection_lazy_members", From 2546ca6d7cd705541d321d2051a1f7a75a83e493 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:33:54 +0000 Subject: [PATCH 30/56] Split state_key_expand_lazy_keep_previous_memberships --- tests/handlers/test_sliding_sync.py | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index f079fb99374..2182805c4e3 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4452,6 +4452,46 @@ class RequiredStateChangesTestCase(unittest.TestCase): """ Test expanding the `required_state` to lazy-loading room members. """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + EventTypes.Member: {"@user2:test", "@user3:test"} + }, + request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + previously_returned_lazy_user_ids=frozenset(), + lazy_load_user_ids=frozenset(), + state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, + expected_with_state_deltas=_RequiredStateChangesReturn( + # Since `StateValues.LAZY` was added, we should persist the + # changed required state config. + {EventTypes.Member: {StateValues.LAZY}}, + # No users are being lazy loaded, so nothing to request. + StateFilter.none(), + # Remember the fact that we've sent @user3 down before, + # but not @user2 as that has been invalidated. + lazy_members_previously_returned={"@user3:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + expected_without_state_deltas=_RequiredStateChangesReturn( + # Since `StateValues.LAZY` was added, we should persist the + # changed required state config. + {EventTypes.Member: {StateValues.LAZY}}, + # No users are being lazy loaded, so nothing to request. + StateFilter.none(), + # Remember the fact that we've sent the users down before. + lazy_members_previously_returned={"@user2:test", "@user3:test"}, + # Nothing to invalidate as there are no existing lazy members. + lazy_members_invalidated=frozenset(), + ), + ), + ), + ( + "state_key_expand_lazy_keep_previous_memberships_need_previous_sent", + """ + Test expanding the `required_state` to lazy-loading room + members. If a previously explicit membership is requested then + we should not send it again (as it was already sent before). + """, RequiredStateChangesTestParameters( previous_required_state_map={ EventTypes.Member: {"@user2:test", "@user3:test"} From ba5939100be8bfda71a4462686dcf887e7376bca Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:40:20 +0000 Subject: [PATCH 31/56] Update LAZY_MEMBERS_UPDATE_INTERVAL comment --- synapse/storage/databases/main/sliding_sync.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index c14350ed4c5..39eba88d912 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -50,8 +50,11 @@ logger = logging.getLogger(__name__) -# How often to update the last seen timestamp for lazy members. We don't want to -# update it too often as that causes DB writes. +# How often to update the last seen timestamp for lazy members. +# +# We don't update the timestamp every time to avoid hammering the DB with +# writes, and we don't need the timestamp to be precise (as it is used to evict +# old entries that haven't been used in a while). LAZY_MEMBERS_UPDATE_INTERVAL = Duration(hours=1) # How often to update the `last_used_ts` column on From 0a68e12c3557948b621cf1406612a977dd9415a1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:40:56 +0000 Subject: [PATCH 32/56] Fixup RoomLazyMembershipChanges comment --- synapse/types/handlers/sliding_sync.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index e66080baf09..babb4a2fbe4 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -896,15 +896,15 @@ class RoomLazyMembershipChanges: """Changes to lazily-loaded room memberships for a given room. Attributes: - returned: Map from user ID to timestamp for users whose membership we - have lazily loaded. The timestamp indicates the time we previously - saw the membership if we have sent it down previously, or None if - we sent it down for the first time. + returned_user_id_to_last_seen_ts_map: Map from user ID to timestamp for + users whose membership we have lazily loaded. The timestamp + indicates the time we previously saw the membership if we have sent + it down previously, or None if we sent it down for the first time. Note: this will include users whose membership we would have sent - down but didn't due to us having previously sent them. - invalidated: Set of user IDs whose latest membership we have *not* sent - down + down but didn't due to us having previously sent them. + invalidated_user_ids: Set of user IDs whose latest membership we have + *not* sent down """ # A map from user ID -> timestamp. Indicates that those memberships have From 45d1bfa739cd763be7a19abdd73e5b26a5f83651 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 10:57:46 +0000 Subject: [PATCH 33/56] Fixup returned_user_id_to_last_seen_ts_map docs --- synapse/types/handlers/sliding_sync.py | 29 ++++++++++---------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index babb4a2fbe4..f4adf5ae588 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -897,9 +897,15 @@ class RoomLazyMembershipChanges: Attributes: returned_user_id_to_last_seen_ts_map: Map from user ID to timestamp for - users whose membership we have lazily loaded. The timestamp - indicates the time we previously saw the membership if we have sent - it down previously, or None if we sent it down for the first time. + users whose membership we have lazily loaded in this room an + request. The timestamp indicates the time we previously needed the + membership, or None if we sent it down for the first time in this + request. + + We track a *rough* `last_seen_ts` for each user in each room which + indicates when we last would've sent their member state to the + client. This is used so that we can remove members which haven't + been seen for a while to save space. Note: this will include users whose membership we would have sent down but didn't due to us having previously sent them. @@ -907,19 +913,8 @@ class RoomLazyMembershipChanges: *not* sent down """ - # A map from user ID -> timestamp. Indicates that those memberships have - # been lazily loaded. I.e. that either a) we sent those memberships down, or - # b) we did so previously. The timestamp indicates the time we previously - # saw the membership. - # - # We track a *rough* `last_seen_ts` for each user in each room which - # indicates when we last would've sent their member state to the client. - # This is used so that we can remove members which haven't been seen for a - # while to save space. returned_user_id_to_last_seen_ts_map: Mapping[str, int | None] = attr.Factory(dict) - # A set of user IDs whose membership change we have *not* sent - # down invalidated_user_ids: AbstractSet[str] = attr.Factory(set) def __bool__(self) -> bool: @@ -940,10 +935,8 @@ class MutablePerConnectionState(PerConnectionState): room_configs: typing.ChainMap[str, RoomSyncConfig] - # A map from room ID -> user ID -> timestamp. Indicates that those - # memberships have been lazily loaded. I.e. that either a) we sent those - # memberships down, or b) we did so previously. The timestamp indicates the - # time we previously saw the membership. + # A map from room ID to the lazily-loaded memberships needed for the + # request in that room. room_lazy_membership: dict[str, RoomLazyMembershipChanges] = attr.Factory(dict) def has_updates(self) -> bool: From 4070326d245057799574b8408989641cf37f2eb6 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:06:56 +0000 Subject: [PATCH 34/56] Update synapse/handlers/sliding_sync/__init__.py Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index cec4cf4ece9..793da152775 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1619,7 +1619,7 @@ def _required_state_changes( we have previously returned to the client due to lazy loading. This is filtered to only include users who have either sent events in the timeline, required state or whose membership changed. - lazy_load_user_ids: The set of user IDs whose lazy-loaded membership + request_lazy_load_user_ids: The set of user IDs whose lazy-loaded membership is required for this request. state_deltas: The state deltas that have changed in the room since the previous request. From e2b4fe8ab93486e3a0e2a21b53ccac8ae74b4989 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:09:36 +0000 Subject: [PATCH 35/56] Update tests/rest/client/sliding_sync/test_rooms_required_state.py Co-authored-by: Eric Eastwood --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 8a4e83be8ef..7a6f7f3b5eb 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2242,7 +2242,7 @@ def test_lazy_loaded_last_seen_ts(self) -> None: # The timestamp for user1 should be updated. self.assertGreater(lazy_member_entries[user1_id], prev_timestamp) - def test_lazy_load_state_reset(self) -> None: + def test_lazy_loading_room_members_state_reset(self) -> None: """Test that when using lazy-loaded members, if a membership state is reset to a previous state and the sync is not limited, then we send down the state reset. From b75b3cbfc9b4e1ea8b6768e41ea2f85f57bad783 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:10:34 +0000 Subject: [PATCH 36/56] Update tests/rest/client/sliding_sync/test_rooms_required_state.py Co-authored-by: Eric Eastwood --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 7a6f7f3b5eb..d398bcd1af7 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2257,7 +2257,7 @@ def test_lazy_loading_room_members_state_reset(self) -> None: content = self.helper.join(room_id, user1_id, tok=user1_tok) first_event_id = content["event_id"] - # Send a message so that user1 comes down sync. + # Send a message so that the user1 membership comes down sync (because we're lazy-loading room members) self.helper.send(room_id, "msg", tok=user1_tok) sync_body = { From 6caacd1a67587f84d8338419401b4069a820607c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:15:40 +0000 Subject: [PATCH 37/56] Move 'lazy_members_previously_returned' definition --- synapse/handlers/sliding_sync/__init__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 793da152775..f3cb0824bf3 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1705,11 +1705,6 @@ def _required_state_changes( # client. Passed to `StateFilter.from_types(...)` added: list[tuple[str, str | None]] = [] - # Record any members that were previously explicitly tracked and should now - # be tracked as lazy members. This handles the case of membership changing - # from e.g. `{@alice:example.com}` to `{LAZY}`. - lazy_members_previously_returned: set[str] = set() - # 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]] = {} @@ -1912,6 +1907,8 @@ def _required_state_changes( # add to the lazy members previously returned. This is so that we don't # return a user due to lazy loading if they were previously returned as an # explicit membership. + lazy_members_previously_returned: set[str] = set() + membership_changes = changes.get(EventTypes.Member, set()) if membership_changes and StateValues.LAZY in request_state_keys: for state_key in prev_required_state_map.get(EventTypes.Member, set()): From b1bc509a577a056bc58ed48b53940ecbf72d98b6 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:15:56 +0000 Subject: [PATCH 38/56] Remove spurious lazy load user in test --- tests/handlers/test_sliding_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 2182805c4e3..88b1d3f7e09 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4697,7 +4697,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {"@user4:test"}}, previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, - lazy_load_user_ids={"@user4:test"}, + lazy_load_user_ids=frozenset(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed From 7ff3d2fdd949b809757c20b1a6e6a496873df665 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:38:13 +0000 Subject: [PATCH 39/56] Don't add to lazy_members_previously_returned what we're lazy loading --- synapse/handlers/sliding_sync/__init__.py | 8 +------- tests/handlers/test_sliding_sync.py | 18 ++++++++---------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index f3cb0824bf3..746432e8ea9 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1920,16 +1920,10 @@ def _required_state_changes( # Normalize to proper user ID state_key = user_id - # We remember the user if either a) they haven't been - # invalidated... + # We remember the user if either they haven't been invalidated if (EventTypes.Member, state_key) not in state_deltas: lazy_members_previously_returned.add(state_key) - # ...or b) if we are going to send the delta down in this - # sync. - if state_key in lazy_load_user_ids: - lazy_members_previously_returned.add(state_key) - if changes: # Update the required state config based on the changes. new_required_state_map = dict(prev_required_state_map) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 88b1d3f7e09..7efe89480ba 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4644,23 +4644,20 @@ class RequiredStateChangesTestCase(unittest.TestCase): """, RequiredStateChangesTestParameters( previous_required_state_map={ - EventTypes.Member: { - StateValues.LAZY, - "$ME", - } + EventTypes.Member: {StateValues.LAZY, "$ME", "@user2:test"} }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), lazy_load_user_ids={"@user:test"}, - state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, + state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since an explicit membership was removed we record the # new required state config and move them to lazy # members. {EventTypes.Member: {StateValues.LAZY}}, - # We have already sent @user3 down before. + # We have already sent @user down before. # - # `@user3:test` is required for lazy loading, but we've + # `@user:test` is required for lazy loading, but we've # already sent it down before, so we don't need to # request it again. StateFilter.none(), @@ -4675,13 +4672,14 @@ class RequiredStateChangesTestCase(unittest.TestCase): # state changes, so we don't need to persist the new required # state config yet. None, - # We have already sent @user3 down before. + # We have already sent @user down before. # - # `@user3:test` is required for lazy loading, but we've + # `@user:test` is required for lazy loading, but we've # already sent it down before, so we don't need to # request it again. StateFilter.none(), - # Remember the fact that we've sent the users down before. + # We don't add anything as we haven't changed the + # required state yet. lazy_members_previously_returned=frozenset(), # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), From 008cb588c3bee090bcf3e7355078d82524f96683 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:39:46 +0000 Subject: [PATCH 40/56] Add context to state reset comment --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index d398bcd1af7..ac4d7d53b72 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2346,7 +2346,9 @@ def test_lazy_loading_room_members_state_reset(self) -> None: sync_body, since=from_token, tok=user1_tok ) - # This should be a non-limited sync + # This should be a non-limited sync as there is only one timeline event. + # This is important as we don't send down state resets on limited + # timelines when using lazy loaded memberships. self.assertFalse( response_body["rooms"][room_id].get("limited", False), "Expected a non-limited timeline", From d3f3f9856aa6f718d903190609b9c35c0a4f22a0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 11:40:57 +0000 Subject: [PATCH 41/56] Note it is a regression test --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index ac4d7d53b72..c05955770cb 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2246,6 +2246,10 @@ def test_lazy_loading_room_members_state_reset(self) -> None: """Test that when using lazy-loaded members, if a membership state is reset to a previous state and the sync is not limited, then we send down the state reset. + + Regression test as previously we only returned membership relevant to + the timeline and so did not tell clients about state resets for + users who did not send any timeline events. """ user1_id = self.register_user("user1", "pass") From 0ffb32aed328fd22ac440a71b54569869c644b37 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 12:01:03 +0000 Subject: [PATCH 42/56] Update comment on update ts test --- .../client/sliding_sync/test_rooms_required_state.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index c05955770cb..8ec5a02d4e4 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2169,8 +2169,11 @@ def test_lazy_loaded_last_seen_ts(self) -> None: prev_timestamp = lazy_member_entries[user1_id] - # If user1 is sent down again, the last_seen_ts should NOT be updated as - # not enough time has passed. + # If user1 sends a message then we consider it for lazy loading. We have + # previously returned it so we don't send the state down again, but it + # is still eligible for updating the timestamp. Since we last updated + # the timestamp within the last `LAZY_MEMBERS_UPDATE_INTERVAL`, we do not + # update it. self.helper.send(room_id, "msg2", tok=user1_tok) response_body, from_token = self.do_sync( @@ -2224,7 +2227,9 @@ def test_lazy_loaded_last_seen_ts(self) -> None: # The timestamp for user1 should be unchanged, as they were not sent down. self.assertEqual(lazy_member_entries[user1_id], prev_timestamp) - # If user1 sends a message, then the timestamp should be updated. + # Now if user1 sends a message, then the timestamp should be updated as + # its been over `LAZY_MEMBERS_UPDATE_INTERVAL` since we last updated it. + # (Even though we don't send the state down again). self.helper.send(room_id, "msg4", tok=user1_tok) response_body, from_token = self.do_sync( From 17bf341dead9daa4c6b33e77220f7ea709255b94 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 3 Dec 2025 13:38:12 +0000 Subject: [PATCH 43/56] Only persist lazy members if we need to Currently we always persist a new position when using lazy loading, which is needless. --- synapse/handlers/sliding_sync/__init__.py | 6 ++- synapse/handlers/sliding_sync/store.py | 10 ++--- .../storage/databases/main/sliding_sync.py | 8 +--- synapse/types/handlers/sliding_sync.py | 43 ++++++++++++++++--- 4 files changed, 46 insertions(+), 21 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 746432e8ea9..a0a54cc8ed0 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -108,7 +108,7 @@ def __init__(self, hs: "HomeServer"): self.rooms_to_exclude_globally = hs.config.server.rooms_to_exclude_from_sync self.is_mine_id = hs.is_mine_id - self.connection_store = SlidingSyncConnectionStore(self.store) + self.connection_store = SlidingSyncConnectionStore(self.clock, self.store) self.extensions = SlidingSyncExtensionHandler(hs) self.room_lists = SlidingSyncRoomLists(hs) @@ -1233,7 +1233,9 @@ async def get_room_sync_data( # users' membership (and so can evict old membership state # from the DB tables). returned_user_id_to_last_seen_ts_map.update( - previously_returned_user_to_last_seen + (user_id, timestamp) + for user_id, timestamp in previously_returned_user_to_last_seen.items() + if user_id in lazy_load_user_ids ) else: previously_returned_user_to_last_seen = {} diff --git a/synapse/handlers/sliding_sync/store.py b/synapse/handlers/sliding_sync/store.py index d01fab271f6..65febe58aaa 100644 --- a/synapse/handlers/sliding_sync/store.py +++ b/synapse/handlers/sliding_sync/store.py @@ -13,7 +13,6 @@ # import logging -from typing import TYPE_CHECKING import attr @@ -25,9 +24,7 @@ PerConnectionState, SlidingSyncConfig, ) - -if TYPE_CHECKING: - pass +from synapse.util.clock import Clock logger = logging.getLogger(__name__) @@ -61,7 +58,8 @@ class SlidingSyncConnectionStore: to mapping of room ID to `HaveSentRoom`. """ - store: "DataStore" + clock: Clock + store: DataStore async def get_and_clear_connection_positions( self, @@ -101,7 +99,7 @@ async def record_new_state( If there are no changes to the state this may return the same token as the existing per-connection state. """ - if not new_connection_state.has_updates(): + if not new_connection_state.has_updates(self.clock): if from_token is not None: return from_token.connection_position else: diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 39eba88d912..a433a1f55de 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -31,6 +31,7 @@ from synapse.storage.engines import PostgresEngine from synapse.types import MultiWriterStreamToken, RoomStreamToken from synapse.types.handlers.sliding_sync import ( + LAZY_MEMBERS_UPDATE_INTERVAL, HaveSentRoom, HaveSentRoomFlag, MutablePerConnectionState, @@ -50,13 +51,6 @@ logger = logging.getLogger(__name__) -# How often to update the last seen timestamp for lazy members. -# -# We don't update the timestamp every time to avoid hammering the DB with -# writes, and we don't need the timestamp to be precise (as it is used to evict -# old entries that haven't been used in a while). -LAZY_MEMBERS_UPDATE_INTERVAL = Duration(hours=1) - # How often to update the `last_used_ts` column on # `sliding_sync_connection_positions` when the client uses a connection # position. We don't want to update it on every use to avoid excessive diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index f4adf5ae588..9578d60da00 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -49,12 +49,21 @@ UserID, ) from synapse.types.rest.client import SlidingSyncBody +from synapse.util.clock import Clock +from synapse.util.duration import Duration if TYPE_CHECKING: from synapse.handlers.relations import BundledAggregations logger = logging.getLogger(__name__) +# How often to update the last seen timestamp for lazy members. +# +# We don't update the timestamp every time to avoid hammering the DB with +# writes, and we don't need the timestamp to be precise (as it is used to evict +# old entries that haven't been used in a while). +LAZY_MEMBERS_UPDATE_INTERVAL = Duration(hours=1) + class SlidingSyncConfig(SlidingSyncBody): """ @@ -917,10 +926,29 @@ class RoomLazyMembershipChanges: invalidated_user_ids: AbstractSet[str] = attr.Factory(set) - def __bool__(self) -> bool: - return bool( - self.returned_user_id_to_last_seen_ts_map or self.invalidated_user_ids - ) + def has_updates(self, clock: Clock) -> bool: + """Check if there are any updates to the lazy membership changes.""" + + # We consider there to be updates if there are any invalidated user + # IDs... + if self.invalidated_user_ids: + return True + + # ...or if any of the returned user IDs have a last seen timestamp older + # than the lazy membership expiry time or is None. + now_ms = clock.time_msec() + for last_seen_ts in self.returned_user_id_to_last_seen_ts_map.values(): + if last_seen_ts is None: + # We need to record that we're sending this membership for the first + # time. + return True + + if now_ms - last_seen_ts >= LAZY_MEMBERS_UPDATE_INTERVAL.as_millis(): + # The last seen timestamp is old enough that we need to refresh + # it. + return True + + return False @attr.s(auto_attribs=True) @@ -939,13 +967,16 @@ class MutablePerConnectionState(PerConnectionState): # request in that room. room_lazy_membership: dict[str, RoomLazyMembershipChanges] = attr.Factory(dict) - def has_updates(self) -> bool: + def has_updates(self, clock: Clock) -> bool: return ( bool(self.rooms.get_updates()) or bool(self.receipts.get_updates()) or bool(self.account_data.get_updates()) or bool(self.get_room_config_updates()) - or bool(self.room_lazy_membership) + or any( + change.has_updates(clock) + for change in self.room_lazy_membership.values() + ) ) def get_room_config_updates(self) -> Mapping[str, RoomSyncConfig]: From 85c67547faa26215c82470d6e2a5133855c71680 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 10:55:31 +0000 Subject: [PATCH 44/56] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync/__init__.py | 17 ++++++++--------- synapse/storage/databases/main/sliding_sync.py | 4 ++-- tests/handlers/test_sliding_sync.py | 2 +- .../sliding_sync/test_rooms_required_state.py | 17 +++++++++-------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index a0a54cc8ed0..1ca38ac31b1 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1177,7 +1177,7 @@ async def get_room_sync_data( # state as well). hero_membership_state: StateMap[EventBase] = {} - # By default we mark all required user state as being added when lazy + # By default, we mark all required user state as being added when lazy # loaded members is enabled. # # We may later update this to account for previously sent members. @@ -1204,9 +1204,9 @@ async def get_room_sync_data( assert from_bound is not None if prev_room_sync_config is not None: - # Define `required_user_state` as all user state we want, which + # Define `all_required_user_state` as all user state we want, which # is the explicitly requested members, any needed for lazy - # loading, and users whose membership has changed.s + # loading, and users whose membership has changed. all_required_user_state = explicit_user_state | lazy_load_user_ids for state_type, state_key in room_state_delta_id_map: if state_type == EventTypes.Member: @@ -1620,11 +1620,10 @@ def _required_state_changes( previously_returned_lazy_user_ids: The set of user IDs whose membership we have previously returned to the client due to lazy loading. This is filtered to only include users who have either sent events in the - timeline, required state or whose membership changed. + `timeline`, `required_state` or whose membership changed. request_lazy_load_user_ids: The set of user IDs whose lazy-loaded membership is required for this request. - state_deltas: The state deltas that have changed in the room since the - previous request. + state_deltas: The state deltas in the room in the request token range, considering user membership. See `get_current_state_deltas_for_room` for more details. """ # First we find any lazy members that have been invalidated due to state @@ -1635,7 +1634,7 @@ def _required_state_changes( continue if state_key in lazy_load_user_ids: - # Because it's part of the `required_user_state`, we're going to + # Because it's part of the `request_lazy_load_user_ids`, we're going to # send this member change down. continue @@ -1883,7 +1882,7 @@ def _required_state_changes( request_state_key_lazy = StateValues.LAZY in request_state_keys has_lazy = old_state_key_lazy or request_state_key_lazy - # If a "$LAZY" has been added or removed we always update. + # If a "$LAZY" has been added or removed we always update to match the request. if old_state_key_lazy != request_state_key_lazy: changes[event_type] = request_state_keys continue @@ -1922,7 +1921,7 @@ def _required_state_changes( # Normalize to proper user ID state_key = user_id - # We remember the user if either they haven't been invalidated + # We remember the user if they haven't been invalidated if (EventTypes.Member, state_key) not in state_deltas: lazy_members_previously_returned.add(state_key) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index a433a1f55de..11f4a7d7e0f 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -564,7 +564,7 @@ async def get_sliding_sync_connection_lazy_members( Args: connection_position: The sliding sync connection position. room_id: The room ID to get lazy members for. - user_ids: The user IDs to check for lazy membership. + user_ids: The user IDs to check whether we've previously sent because of lazy membership. Returns: The mapping of user IDs to the last seen timestamp for those user @@ -608,7 +608,7 @@ def get_sliding_sync_connection_lazy_members_txn( } return await self.db_pool.runInteraction( - "sliding_sync_connection_lazy_members", + "get_sliding_sync_connection_lazy_members", get_sliding_sync_connection_lazy_members_txn, db_autocommit=True, # Avoid transaction for single read ) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 7efe89480ba..ad998e215b7 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4644,7 +4644,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): """, RequiredStateChangesTestParameters( previous_required_state_map={ - EventTypes.Member: {StateValues.LAZY, "$ME", "@user2:test"} + EventTypes.Member: {StateValues.LAZY, StateValues.ME, "@user2:test"} }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 8ec5a02d4e4..a11c463faf4 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -839,7 +839,7 @@ def test_rooms_required_state_expand_retract_expand_lazy_loading_room_members_in exact=True, ) - def test_lazy_members_limited_sync(self) -> None: + def test_lazy_loading_room_members_limited_sync(self) -> None: """Test that when using lazy loading for room members and a limited sync missing a membership change, we include the membership change next time said user says something. @@ -935,7 +935,7 @@ def test_lazy_members_limited_sync(self) -> None: exact=True, ) - def test_lazy_members_across_multiple_rooms(self) -> None: + def test_lazy_loading_room_members_across_multiple_rooms(self) -> None: """Test that lazy loading room members are tracked per-room correctly.""" user1_id = self.register_user("user1", "pass") @@ -1018,7 +1018,7 @@ def test_lazy_members_across_multiple_rooms(self) -> None: exact=True, ) - def test_lazy_members_across_multiple_connections(self) -> None: + def test_lazy_loading_room_members_across_multiple_connections(self) -> None: """Test that lazy loading room members are tracked per-connection correctly. @@ -1118,7 +1118,7 @@ def test_lazy_members_across_multiple_connections(self) -> None: exact=True, ) - def test_lazy_members_forked_position(self) -> None: + def test_lazy_loading_room_members_forked_position(self) -> None: """Test that lazy loading room members are tracked correctly when a connection position is reused""" @@ -1190,7 +1190,7 @@ def test_lazy_members_forked_position(self) -> None: exact=True, ) - def test_lazy_members_explicit_membership_removed(self) -> None: + def test_lazy_loading_room_members_explicit_membership_removed(self) -> None: """Test the case where we requested explicit memberships and then later changed to lazy loading.""" @@ -2116,7 +2116,7 @@ def test_rooms_required_state_expand_deduplicate(self) -> None: # down. self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) - def test_lazy_loaded_last_seen_ts(self) -> None: + def test_lazy_loading_room_members_last_seen_ts(self) -> None: """Test that the `last_seen_ts` column in `sliding_sync_connection_lazy_members` is correctly kept up to date""" @@ -2247,7 +2247,7 @@ def test_lazy_loaded_last_seen_ts(self) -> None: # The timestamp for user1 should be updated. self.assertGreater(lazy_member_entries[user1_id], prev_timestamp) - def test_lazy_loading_room_members_state_reset(self) -> None: + def test_lazy_loading_room_members_state_reset_non_limited_timeline(self) -> None: """Test that when using lazy-loaded members, if a membership state is reset to a previous state and the sync is not limited, then we send down the state reset. @@ -2355,8 +2355,9 @@ def test_lazy_loading_room_members_state_reset(self) -> None: sync_body, since=from_token, tok=user1_tok ) - # This should be a non-limited sync as there is only one timeline event. + # This should be a non-limited sync as there is only one timeline event (<= `timeline_limit). # This is important as we don't send down state resets on limited + # This is important as we're specifically testing the non-`limited` timeline scenario. And for reference, we don't send down state resets on limited # timelines when using lazy loaded memberships. self.assertFalse( response_body["rooms"][room_id].get("limited", False), From 1adcdaa311a1722e115af1637893dd7e40072f9f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 10:57:33 +0000 Subject: [PATCH 45/56] Fix wrapping --- synapse/handlers/sliding_sync/__init__.py | 8 +++++--- synapse/storage/databases/main/sliding_sync.py | 3 ++- tests/handlers/test_sliding_sync.py | 6 +++++- .../rest/client/sliding_sync/test_rooms_required_state.py | 8 ++++---- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 1ca38ac31b1..8690a9d6e67 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1621,9 +1621,11 @@ def _required_state_changes( we have previously returned to the client due to lazy loading. This is filtered to only include users who have either sent events in the `timeline`, `required_state` or whose membership changed. - request_lazy_load_user_ids: The set of user IDs whose lazy-loaded membership - is required for this request. - state_deltas: The state deltas in the room in the request token range, considering user membership. See `get_current_state_deltas_for_room` for more details. + request_lazy_load_user_ids: The set of user IDs whose lazy-loaded + membership is required for this request. + state_deltas: The state deltas in the room in the request token range, + considering user membership. See `get_current_state_deltas_for_room` + for more details. """ # First we find any lazy members that have been invalidated due to state diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 11f4a7d7e0f..bacd2e0742b 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -564,7 +564,8 @@ async def get_sliding_sync_connection_lazy_members( Args: connection_position: The sliding sync connection position. room_id: The room ID to get lazy members for. - user_ids: The user IDs to check whether we've previously sent because of lazy membership. + user_ids: The user IDs to check whether we've previously sent + because of lazy membership. Returns: The mapping of user IDs to the last seen timestamp for those user diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index ad998e215b7..bc5b81561e1 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4644,7 +4644,11 @@ class RequiredStateChangesTestCase(unittest.TestCase): """, RequiredStateChangesTestParameters( previous_required_state_map={ - EventTypes.Member: {StateValues.LAZY, StateValues.ME, "@user2:test"} + EventTypes.Member: { + StateValues.LAZY, + StateValues.ME, + "@user2:test", + } }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index a11c463faf4..dfb90360a0d 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -2355,10 +2355,10 @@ def test_lazy_loading_room_members_state_reset_non_limited_timeline(self) -> Non sync_body, since=from_token, tok=user1_tok ) - # This should be a non-limited sync as there is only one timeline event (<= `timeline_limit). - # This is important as we don't send down state resets on limited - # This is important as we're specifically testing the non-`limited` timeline scenario. And for reference, we don't send down state resets on limited - # timelines when using lazy loaded memberships. + # This should be a non-limited sync as there is only one timeline event + # (<= `timeline_limit). This is important as we're specifically testing the non-`limited` + # timeline scenario. And for reference, we don't send down state resets + # on limited timelines when using lazy loaded memberships. self.assertFalse( response_body["rooms"][room_id].get("limited", False), "Expected a non-limited timeline", From aa2c42652db9824e27b0d4e8056864d44e2bfd3e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 11:22:49 +0000 Subject: [PATCH 46/56] Explain why we don't use sliding_sync_connection_required_state --- .../schema/main/delta/93/02_sliding_sync_members.sql | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql index 6521c955ea9..4b922282847 100644 --- a/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql +++ b/synapse/storage/schema/main/delta/93/02_sliding_sync_members.sql @@ -16,6 +16,13 @@ -- members in sliding sync. This is a *cache* as it doesn't matter if we send -- down members we've previously sent down, i.e. it's safe to delete any rows. -- +-- We could have tracked these as part of the +-- `sliding_sync_connection_required_state` table, but that would bloat that +-- table significantly as most rooms will have many lazy-loaded members. Due to +-- the way deduplication is done, we always pull out all rows for the connection +-- for every request, so having a large number of rows there causes significant +-- performance issues. +-- -- We track a *rough* `last_seen_ts` for each user in each room which indicates -- when we last would've sent their member state to the client. This is used so -- that we can remove members which haven't been seen for a while to save space. From 1d7b649e4ec452a01df49c4af14a77feac7b180f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 11:25:33 +0000 Subject: [PATCH 47/56] Add comment to has_updates --- synapse/types/handlers/sliding_sync.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 9578d60da00..f45ca7db3f3 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -927,7 +927,13 @@ class RoomLazyMembershipChanges: invalidated_user_ids: AbstractSet[str] = attr.Factory(set) def has_updates(self, clock: Clock) -> bool: - """Check if there are any updates to the lazy membership changes.""" + """Check if there are any updates to the lazy membership changes. + + Called to check if we need to persist changes to the lazy membership + state for the room. We want to avoid persisting the state if there are + no changes, to avoid unnecessary writes (and cache misses due to new + connection position). + """ # We consider there to be updates if there are any invalidated user # IDs... From 6d8950eba53fe3d5cb547f95d24572e21fca99fc Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 11:54:17 +0000 Subject: [PATCH 48/56] Update comment for which prev lazy members we fetch --- synapse/handlers/sliding_sync/__init__.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 8690a9d6e67..679beb5524e 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1215,9 +1215,22 @@ async def get_room_sync_data( # We need to know what user state we previously sent down the # connection so we can determine what has changed. # - # We don't just pull out the lazy loaded members here to handle - # the case where the client added explicit user state requests - # for users they already had lazy loaded. + # We need to fetch for all users whose memberships we may want + # to send down this sync. This includes (and matches + # `all_required_user_state`): + # 1. Explicitly requested user state + # 2. Lazy loaded members, i.e. users who appear in the + # timeline. + # 3. The users whose membership has changed in the room, i.e. + # in the state deltas. + # + # This is to correctly handle the cases where a user was + # previously sent down as a lazy loaded member: + # - and is now explicitly requested (so shouldn't be sent down + # again); or + # - their membership has changed (so we need to invalidate + # their entry in the lazy loaded table if we don't send the + # change down). if all_required_user_state: previously_returned_user_to_last_seen = ( await self.store.get_sliding_sync_connection_lazy_members( From bea19c461571577a7b226ac974862b0aa03c195a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 11:58:09 +0000 Subject: [PATCH 49/56] s/changed_required_state_map/required_state_map_change/ --- synapse/handlers/sliding_sync/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 679beb5524e..7a3d806cdbf 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1167,7 +1167,7 @@ async def get_room_sync_data( # The required state map to store in the room sync config, if it has # changed. - changed_required_state_map: Mapping[str, AbstractSet[str]] | None = None + required_state_map_change: Mapping[str, AbstractSet[str]] | None = 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. @@ -1263,7 +1263,7 @@ async def get_room_sync_data( lazy_load_user_ids=lazy_load_user_ids, state_deltas=room_state_delta_id_map, ) - changed_required_state_map = changes_return.required_state_map_change + required_state_map_change = changes_return.required_state_map_change new_connection_state.room_lazy_membership[ room_id @@ -1391,8 +1391,8 @@ async def get_room_sync_data( room_sync_required_state_map_to_persist: Mapping[str, AbstractSet[str]] = ( room_sync_config.required_state_map ) - if changed_required_state_map: - room_sync_required_state_map_to_persist = changed_required_state_map + if required_state_map_change: + room_sync_required_state_map_to_persist = required_state_map_change # Record the `room_sync_config` if we're `ignore_timeline_bound` (which means # that the `timeline_limit` has increased) @@ -1437,7 +1437,7 @@ async def get_room_sync_data( required_state_map=room_sync_required_state_map_to_persist, ) - elif changed_required_state_map is not None: + elif required_state_map_change 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, From 91770fcd47c5c20fd1380c103d5a649db90ddc7d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 12:01:02 +0000 Subject: [PATCH 50/56] s/lazy_load_user_ids/request_lazy_load_user_ids/ --- synapse/handlers/sliding_sync/__init__.py | 10 +++++----- tests/handlers/test_sliding_sync.py | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 7a3d806cdbf..7d07c8613b0 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1260,7 +1260,7 @@ async def get_room_sync_data( prev_required_state_map=prev_room_sync_config.required_state_map, request_required_state_map=room_sync_config.required_state_map, previously_returned_lazy_user_ids=previously_returned_user_to_last_seen.keys(), - lazy_load_user_ids=lazy_load_user_ids, + request_lazy_load_user_ids=lazy_load_user_ids, state_deltas=room_state_delta_id_map, ) required_state_map_change = changes_return.required_state_map_change @@ -1608,7 +1608,7 @@ def _required_state_changes( prev_required_state_map: Mapping[str, AbstractSet[str]], request_required_state_map: Mapping[str, AbstractSet[str]], previously_returned_lazy_user_ids: AbstractSet[str], - lazy_load_user_ids: AbstractSet[str], + request_lazy_load_user_ids: AbstractSet[str], state_deltas: StateMap[str], ) -> _RequiredStateChangesReturn: """Calculates the changes between the required state room config from the @@ -1648,7 +1648,7 @@ def _required_state_changes( if event_type != EventTypes.Member: continue - if state_key in lazy_load_user_ids: + if state_key in request_lazy_load_user_ids: # Because it's part of the `request_lazy_load_user_ids`, we're going to # send this member change down. continue @@ -1663,7 +1663,7 @@ def _required_state_changes( if prev_required_state_map == request_required_state_map: # There has been no change in state, just need to check lazy members. newly_returned_lazy_members = ( - lazy_load_user_ids - previously_returned_lazy_user_ids + request_lazy_load_user_ids - previously_returned_lazy_user_ids ) if newly_returned_lazy_members: # There are some new lazy members we need to fetch. @@ -1839,7 +1839,7 @@ def _required_state_changes( # We also need to pull out any lazy members that are now required but # haven't previously been returned. for required_user_id in ( - lazy_load_user_ids + request_lazy_load_user_ids # Remove previously returned users - previously_returned_lazy_user_ids # Exclude previously explicitly requested members. diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index bc5b81561e1..2ff3dfbd040 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4875,7 +4875,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, - lazy_load_user_ids=test_parameters.lazy_load_user_ids, + request_lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas={}, ) @@ -4906,7 +4906,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, - lazy_load_user_ids=test_parameters.lazy_load_user_ids, + request_lazy_load_user_ids=test_parameters.lazy_load_user_ids, state_deltas=test_parameters.state_deltas, ) @@ -4970,7 +4970,7 @@ def test_limit_retained_previous_state_keys( prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids=frozenset(), + request_lazy_load_user_ids=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change @@ -5043,7 +5043,7 @@ def test_request_more_state_keys_than_remember_limit(self) -> None: prev_required_state_map=previous_required_state_map, request_required_state_map=request_required_state_map, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids=frozenset(), + request_lazy_load_user_ids=frozenset(), state_deltas={}, ) changed_required_state_map = state_changes.required_state_map_change From 31c913ea0b67b087e005ee7a0dc25d41b9bd3a1c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 15:50:54 +0000 Subject: [PATCH 51/56] Expand why we always record move to LAZY --- synapse/handlers/sliding_sync/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 7d07c8613b0..177ffa54888 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1744,7 +1744,9 @@ def _required_state_changes( continue # Handle the special case of adding `$LAZY` membership, where we want to - # always record the change to be lazy loading. + # always record the change to be lazy loading, as we immediately start + # using the lazy loading tables so there is no point *not* recording the + # change to lazy load in the effective room config. 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 From 855b448ddbe72dde6744ca149b857eb0fdb7fb28 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 15:52:53 +0000 Subject: [PATCH 52/56] Expand comment about moving positions to NULL --- synapse/storage/databases/main/sliding_sync.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index bacd2e0742b..8c7f3c770c6 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -459,7 +459,11 @@ def _get_and_clear_connection_positions_txn( # Move any lazy membership entries for this connection position to have # `NULL` connection position, indicating that it applies to all future - # positions on this connection. + # positions on this connection. This is safe because we have deleted all + # other (potentially forked) connection positions, and so all future + # positions in this connection will be a continuation of the current + # position. Thus any lazy membership entries we have sent down will still + # be valid. self.db_pool.simple_update_txn( txn, table="sliding_sync_connection_lazy_members", From 6fc746ceb9603a4565a06f291b8c6f2a917451d2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 15:53:48 +0000 Subject: [PATCH 53/56] Expand return doc comment for get_sliding_sync_connection_lazy_members --- synapse/storage/databases/main/sliding_sync.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index 8c7f3c770c6..cbeb46e986e 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -573,7 +573,8 @@ async def get_sliding_sync_connection_lazy_members( Returns: The mapping of user IDs to the last seen timestamp for those user - IDs. + IDs. Only includes user IDs that we have previously sent lazy + membership for, and so may be a subset of the `user_ids` passed in. """ def get_sliding_sync_connection_lazy_members_txn( From b63c8ad4478cc0243228529757b1cc55f70521aa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 15:54:21 +0000 Subject: [PATCH 54/56] s/mem/members/ in query --- synapse/storage/databases/main/sliding_sync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index cbeb46e986e..2d1ae3c354c 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -595,8 +595,8 @@ def get_sliding_sync_connection_lazy_members_txn( # `sliding_sync_connection_lazy_members` is what we filter against # (it may be null or the same as the one passed in). sql = f""" - SELECT user_id, mem.connection_position, last_seen_ts - FROM sliding_sync_connection_lazy_members AS mem + SELECT user_id, members.connection_position, last_seen_ts + FROM sliding_sync_connection_lazy_members AS members INNER JOIN sliding_sync_connection_positions AS pos USING (connection_key) WHERE pos.connection_position = ? AND room_id = ? AND {user_clause} """ From c1887b87de6084a3fa61cc4ea2dbaa00f360d23e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 16:06:12 +0000 Subject: [PATCH 55/56] Test s/lazy_load_user_ids/request_lazy_load_user_ids/ --- tests/handlers/test_sliding_sync.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 2ff3dfbd040..6423390e139 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3835,7 +3835,7 @@ class RequiredStateChangesTestParameters: expected_without_state_deltas: _RequiredStateChangesReturn previously_returned_lazy_user_ids: AbstractSet[str] = frozenset() - lazy_load_user_ids: AbstractSet[str] = frozenset() + request_lazy_load_user_ids: AbstractSet[str] = frozenset() class RequiredStateChangesTestCase(unittest.TestCase): @@ -4371,7 +4371,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, - lazy_load_user_ids=set(), + request_lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # The `request_required_state_map` hasn't changed @@ -4416,7 +4416,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, - lazy_load_user_ids={"@user4:test"}, + request_lazy_load_user_ids={"@user4:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # The `request_required_state_map` hasn't changed @@ -4458,7 +4458,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids=frozenset(), + request_lazy_load_user_ids=frozenset(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the @@ -4498,7 +4498,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids={"@user3:test"}, + request_lazy_load_user_ids={"@user3:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since `StateValues.LAZY` was added, we should persist the @@ -4542,7 +4542,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={}, previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, - lazy_load_user_ids=set(), + request_lazy_load_user_ids=set(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Remove `EventTypes.Member` since there's been a change to that @@ -4598,7 +4598,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids={"@user3:test"}, + request_lazy_load_user_ids={"@user3:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since an explicit membership was removed we record the @@ -4652,7 +4652,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): }, request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, previously_returned_lazy_user_ids=frozenset(), - lazy_load_user_ids={"@user:test"}, + request_lazy_load_user_ids={"@user:test"}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since an explicit membership was removed we record the @@ -4699,7 +4699,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, request_required_state_map={EventTypes.Member: {"@user4:test"}}, previously_returned_lazy_user_ids={"@user2:test", "@user3:test"}, - lazy_load_user_ids=frozenset(), + request_lazy_load_user_ids=frozenset(), state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=_RequiredStateChangesReturn( # Since "@user4:test" was added, we should persist the changed @@ -4875,7 +4875,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, - request_lazy_load_user_ids=test_parameters.lazy_load_user_ids, + request_lazy_load_user_ids=test_parameters.request_lazy_load_user_ids, state_deltas={}, ) @@ -4906,7 +4906,7 @@ def test_xxx( prev_required_state_map=test_parameters.previous_required_state_map, request_required_state_map=test_parameters.request_required_state_map, previously_returned_lazy_user_ids=test_parameters.previously_returned_lazy_user_ids, - request_lazy_load_user_ids=test_parameters.lazy_load_user_ids, + request_lazy_load_user_ids=test_parameters.request_lazy_load_user_ids, state_deltas=test_parameters.state_deltas, ) From bfe05deb4cfd82260b1ae6b1be20b74f74f94839 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 5 Dec 2025 16:16:31 +0000 Subject: [PATCH 56/56] s/lazy_members_previously_returned/users_to_add_to_lazy_cache/ --- synapse/handlers/sliding_sync/__init__.py | 18 +++++----- tests/handlers/test_sliding_sync.py | 40 +++++++++++------------ 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 177ffa54888..eb1fbdbdb1a 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1272,7 +1272,7 @@ async def get_room_sync_data( # Add any previously returned explicit memberships to the lazy # loaded table. This happens when a client requested explicit # members and then converted them to lazy loading. - for user_id in changes_return.lazy_members_previously_returned: + for user_id in changes_return.users_to_add_to_lazy_cache: # We don't know the right timestamp to use here, as we don't # know the last time we would have sent the membership down. # So we don't overwrite it if we have a timestamp already, @@ -1588,8 +1588,10 @@ class _RequiredStateChangesReturn: the room config, or None if there is no change. added_state_filter: The state filter to use to fetch any additional current state that needs to be returned to the client. - lazy_members_previously_returned: The set of user IDs we should add to - the lazy members cache that we had previously returned. + users_to_add_to_lazy_cache: The set of user IDs we should add to + the lazy members cache that we had previously returned. Handles + the case where a user was previously sent down explicitly but is + now being lazy loaded. lazy_members_invalidated: The set of user IDs whose membership has changed but we didn't send down, so we need to invalidate them from the cache. @@ -1598,7 +1600,7 @@ class _RequiredStateChangesReturn: required_state_map_change: Mapping[str, AbstractSet[str]] | None added_state_filter: StateFilter - lazy_members_previously_returned: AbstractSet[str] = frozenset() + users_to_add_to_lazy_cache: AbstractSet[str] = frozenset() lazy_members_invalidated: AbstractSet[str] = frozenset() @@ -1925,7 +1927,7 @@ def _required_state_changes( # add to the lazy members previously returned. This is so that we don't # return a user due to lazy loading if they were previously returned as an # explicit membership. - lazy_members_previously_returned: set[str] = set() + users_to_add_to_lazy_cache: set[str] = set() membership_changes = changes.get(EventTypes.Member, set()) if membership_changes and StateValues.LAZY in request_state_keys: @@ -1940,7 +1942,7 @@ def _required_state_changes( # We remember the user if they haven't been invalidated if (EventTypes.Member, state_key) not in state_deltas: - lazy_members_previously_returned.add(state_key) + users_to_add_to_lazy_cache.add(state_key) if changes: # Update the required state config based on the changes. @@ -1956,12 +1958,12 @@ def _required_state_changes( required_state_map_change=new_required_state_map, added_state_filter=added_state_filter, lazy_members_invalidated=lazy_members_invalidated, - lazy_members_previously_returned=lazy_members_previously_returned, + users_to_add_to_lazy_cache=users_to_add_to_lazy_cache, ) else: return _RequiredStateChangesReturn( required_state_map_change=None, added_state_filter=added_state_filter, lazy_members_invalidated=lazy_members_invalidated, - lazy_members_previously_returned=lazy_members_previously_returned, + users_to_add_to_lazy_cache=users_to_add_to_lazy_cache, ) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 6423390e139..af837bf459c 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -4381,7 +4381,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we should persist the changed to required state. That way next @@ -4398,7 +4398,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Nothing should change (we should still keep track that # we've sent specific `EventTypes.Member` before). lazy_members_invalidated=frozenset(), @@ -4425,7 +4425,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.from_types([(EventTypes.Member, "@user4:test")]), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Remove "@user2:test" since that state has changed and # is no longer being requested anymore. Since something # was removed, we also should persist the changed to @@ -4442,7 +4442,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.from_types([(EventTypes.Member, "@user4:test")]), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), lazy_members_invalidated=frozenset(), ), ), @@ -4468,7 +4468,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Remember the fact that we've sent @user3 down before, # but not @user2 as that has been invalidated. - lazy_members_previously_returned={"@user3:test"}, + users_to_add_to_lazy_cache={"@user3:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4479,7 +4479,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # No users are being lazy loaded, so nothing to request. StateFilter.none(), # Remember the fact that we've sent the users down before. - lazy_members_previously_returned={"@user2:test", "@user3:test"}, + users_to_add_to_lazy_cache={"@user2:test", "@user3:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4512,7 +4512,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Remember the fact that we've sent @user3 down before, # but not @user2 as that has been invalidated. - lazy_members_previously_returned={"@user3:test"}, + users_to_add_to_lazy_cache={"@user3:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4527,7 +4527,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # request it again. StateFilter.none(), # Remember the fact that we've sent the users down before. - lazy_members_previously_returned={"@user2:test", "@user3:test"}, + users_to_add_to_lazy_cache={"@user2:test", "@user3:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4553,7 +4553,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Explicitly remove the now invalidated @user2:test # membership. # @@ -4575,7 +4575,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Nothing has been invalidated. lazy_members_invalidated=frozenset(), ), @@ -4613,7 +4613,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Remember the fact that we've sent @user3 down before, # but not @user2 as that has been invalidated. - lazy_members_previously_returned={"@user3:test"}, + users_to_add_to_lazy_cache={"@user3:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4629,7 +4629,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): # request it again. StateFilter.none(), # Remember the fact that we've sent the users down before. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4667,7 +4667,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # Remember the fact that we've sent @user down before, # but not @user2 as that has been invalidated. - lazy_members_previously_returned={"@user:test"}, + users_to_add_to_lazy_cache={"@user:test"}, # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4684,7 +4684,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.none(), # We don't add anything as we haven't changed the # required state yet. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Nothing to invalidate as there are no existing lazy members. lazy_members_invalidated=frozenset(), ), @@ -4709,7 +4709,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.from_types([(EventTypes.Member, "@user4:test")]), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # Also remove "@user2:test" since that state has changed and is no # longer being requested anymore. Since something was removed, # we also should persist the changed to required state. That way next @@ -4726,7 +4726,7 @@ class RequiredStateChangesTestCase(unittest.TestCase): StateFilter.from_types([(EventTypes.Member, "@user4:test")]), # Previous request did not include any explicit members, # so nothing to store. - lazy_members_previously_returned=frozenset(), + users_to_add_to_lazy_cache=frozenset(), # We don't invalidate user2 as they haven't changed lazy_members_invalidated=frozenset(), ), @@ -4895,8 +4895,8 @@ def test_xxx( "lazy_members_invalidated does not match (without state_deltas)", ) self.assertEqual( - state_changes.lazy_members_previously_returned, - test_parameters.expected_without_state_deltas.lazy_members_previously_returned, + state_changes.users_to_add_to_lazy_cache, + test_parameters.expected_without_state_deltas.users_to_add_to_lazy_cache, "lazy_members_previously_returned does not match (without state_deltas)", ) @@ -4926,8 +4926,8 @@ def test_xxx( "lazy_members_invalidated does not match (with state_deltas)", ) self.assertEqual( - state_changes.lazy_members_previously_returned, - test_parameters.expected_with_state_deltas.lazy_members_previously_returned, + state_changes.users_to_add_to_lazy_cache, + test_parameters.expected_with_state_deltas.users_to_add_to_lazy_cache, "lazy_members_previously_returned does not match (with state_deltas)", )