From 7d44c81d5caea7ccd384de57472c875442540123 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Mon, 19 Oct 2026 11:25:23 +0200 Subject: [PATCH] Fix new state saved with the last local user's leave missing from `state_after` (#20314) > [!NOTE] > This PR will be rebased on `develop` once https://github.com/element-hq/synapse/pull/20081 is merged This PR is a follow up to https://github.com/element-hq/synapse/pull/20081 and is based on its code. The scope of that original PR is limited to recovering the leave event missing from `state_after`. I noticed that state events from the leave event batch would only be recovered if they were previously defined. That made me add a pre-condition to the test: the topic had to be set first. https://github.com/element-hq/synapse/blob/bccf1679aed0a5d86de87165c548f8c459758055/tests/handlers/test_sync.py#L1096-L1099 This PR attempts to recover new state events that only appeared first in the leave batch. ### Pull Request Checklist * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Fefaine Co-authored-by: Quentin Gliech Co-authored-by: Eric Eastwood Co-authored-by: Eric Eastwood --- changelog.d/20314.bugfix | 1 + synapse/handlers/sync.py | 28 +++++++++++++++++----------- tests/handlers/test_sync.py | 30 +++++++++++++++++++----------- 3 files changed, 37 insertions(+), 22 deletions(-) create mode 100644 changelog.d/20314.bugfix diff --git a/changelog.d/20314.bugfix b/changelog.d/20314.bugfix new file mode 100644 index 0000000000..f48f96aa82 --- /dev/null +++ b/changelog.d/20314.bugfix @@ -0,0 +1 @@ +Fix new state saved with the last local user's leave being missing from `state_after` ([MSC4222](https://github.com/matrix-org/matrix-spec-proposals/pull/4222)) on incremental `/sync`. diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index cd9f164296..d635465e5c 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1738,11 +1738,12 @@ class SyncHandler: # any earlier delta for it is stale. That batch ends the # timeline, so we look up the keys in the timeline in state # groups, which still have the state at `end_token`. - # - has no delta at all if it is new. - # TODO: Recover it. + # - has no delta at all if it is new, so we also look up all the + # keys in the timeline (see below). # Keys that only changed earlier keep their earlier delta, and # other keys need no update. - cleared_state_keys: set[tuple[str, str]] = set() + own_membership_key = (EventTypes.Member, sync_config.user.to_string()) + state_keys_to_look_up: set[tuple[str, str]] = set() for delta in deltas: key = (delta.event_type, delta.state_key) if delta.event_id is None: @@ -1750,11 +1751,8 @@ class SyncHandler: # - it is in the timeline (see above). # - OR it is the user's own membership, in case a filter # removed their leave from the timeline. - if key in timeline_state or key == ( - EventTypes.Member, - sync_config.user.to_string(), - ): - cleared_state_keys.add(key) + if key in timeline_state or key == own_membership_key: + state_keys_to_look_up.add(key) continue # Note that deltas are in stream ordering, so if there are @@ -1762,13 +1760,21 @@ class SyncHandler: # the latest one. delta_state_ids[key] = delta.event_id changed_keys.add(key) - cleared_state_keys.discard(key) + state_keys_to_look_up.discard(key) - if cleared_state_keys: + if own_membership_key in state_keys_to_look_up: + # The server left the room in this window. It only wrote deltas + # for the keys that were in the room state before the batch that + # made it leave, so the keys that batch added have no delta at + # all. Look up all the keys in the timeline: the ones with a delta + # are already in the set. + state_keys_to_look_up.update(timeline_state) + + if state_keys_to_look_up: state_at_end = await self._state_storage_controller.get_state_ids_at( room_id, stream_position=end_token, - state_filter=StateFilter.from_types(cleared_state_keys), + state_filter=StateFilter.from_types(state_keys_to_look_up), # Don't wait for the full state. Partial state has every state # event except remote memberships, and any remote membership # we look up here is from an event in the timeline, which we diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index e9f5bffd45..a4b58abb94 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -1093,18 +1093,26 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): {("m.room.member", alice): leave_event}, ) + @parameterized.expand( + [(True,), (False,)], + name_func=lambda func, num, p: ( + f"{func.__name__}_{'existing' if p.args[0] else 'new'}_topic" + ), + ) def test_state_after_leave_last_local_user_with_state_change_persisted_together( - self, + self, topic_existed: bool ) -> None: """When another state event is persisted in the same batch as the last local user's leave, both the leave and that state event must appear in state_after - on an incremental sync. + on an incremental sync, whether or not the room already had that state. This is to make sure we play nicely with this behavior: When the server leaves a room, it will insert new rows with `event_id = null` into the `current_state_delta_stream` table for all current state. - Neither the leave nor the state event persisted with it gets a row of its own, - only the `event_id = null` rows of the state they replace. + Neither the leave nor the state event persisted with it gets a row of its own: + the leave only gets the `event_id = null` row of the leaving user's membership, + and the state event only gets one if the room already had that state, so the + two cases are recovered differently. """ if not self.use_state_after: self.skipTest("Only relevant for `state_after` (MSC4222)") @@ -1115,9 +1123,10 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): alice_requester = create_requester(alice) room_id = self.helper.create_room_as(alice, tok=alice_tok) - self.helper.send_state( - room_id, EventTypes.Topic, {"topic": "before leaving"}, tok=alice_tok - ) + if topic_existed: + self.helper.send_state( + room_id, EventTypes.Topic, {"topic": "before leaving"}, tok=alice_tok + ) # Sync up to get a since_token. initial_sync_result = self.get_success( @@ -1128,10 +1137,9 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): ) ) - # Alice changes the topic and leaves, with both events persisted in one - # batch. She is the last local user, so the server clears - # current_state_events for this room, and neither event gets a delta of its - # own. + # Alice sets the topic and leaves, with both events persisted in one batch. + # She is the last local user, so the server clears current_state_events for + # this room, and neither event gets a delta of its own. self.get_success( self.hs.get_event_creation_handler().create_and_send_new_client_events( requester=alice_requester,