diff --git a/changelog.d/20169.bugfix b/changelog.d/20169.bugfix new file mode 100644 index 00000000000..1845478a223 --- /dev/null +++ b/changelog.d/20169.bugfix @@ -0,0 +1 @@ +Fix `/sync` returning membership events from after the user's leave in `state_after` for left rooms when lazy-loading room members (experimental [MSC4222](https://github.com/matrix-org/matrix-spec-proposals/pull/4222) implementation). diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index f8a766feee7..049726a97e9 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1249,6 +1249,7 @@ async def compute_state_delta( end_token, members_to_fetch, timeline_state, + joined, ) # If we only have partial state for the room, `state_ids` may be missing the @@ -1471,6 +1472,7 @@ async def _compute_state_delta_for_incremental_sync( end_token: StreamToken, members_to_fetch: set[str] | None, timeline_state: StateMap[str], + joined: bool, ) -> StateMap[str]: """Calculate the state events to be included in an incremental sync response. @@ -1495,6 +1497,7 @@ async def _compute_state_delta_for_incremental_sync( events in the timeline. Otherwise, `None`. timeline_state: The contribution to the room state from state events in `batch`. Only contains the last event for any given state key. + joined: whether the user is currently joined to the room Returns: A map from (type, state_key) to event_id, for each event that we believe @@ -1520,13 +1523,28 @@ async def _compute_state_delta_for_incremental_sync( # events to understand the events in this timeline. So we always # fish out all the member events corresponding to the timeline # here. The caller will then dedupe any redundant ones. - member_ids = await self._state_storage_controller.get_current_state_ids( - room_id=room_id, - state_filter=StateFilter.from_types( - (EventTypes.Member, member) for member in members_to_fetch - ), - await_full_state=await_full_state, + member_filter = StateFilter.from_types( + (EventTypes.Member, member) for member in members_to_fetch ) + if joined: + member_ids = ( + await self._state_storage_controller.get_current_state_ids( + room_id=room_id, + state_filter=member_filter, + await_full_state=await_full_state, + ) + ) + else: + # The user is no longer in the room, so `end_token` points + # at the user's leave/etc event, and the current state may + # include state from after that point. Use state groups to + # get the memberships as of `end_token` instead. + member_ids = await self._state_storage_controller.get_state_ids_at( + room_id, + stream_position=end_token, + state_filter=member_filter, + await_full_state=await_full_state, + ) delta_state_ids.update(member_ids) # We don't do LL filtering for incremental syncs - see diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index b732c501d96..e100943413f 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -2888,6 +2888,7 @@ def test_incremental_sync_multiple_deltas(self) -> None: end_token=end_stream_token, members_to_fetch=None, timeline_state={}, + joined=True, ) ) self.assertEqual(state[("m.test_event", "")], second_state["event_id"]) @@ -2919,6 +2920,7 @@ def test_incremental_sync_lazy_loaded_no_timeline(self) -> None: end_token=end_stream_token, members_to_fetch=set(), timeline_state={}, + joined=True, ) ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 74a8678ae99..039aea4d781 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -33,6 +33,7 @@ ReceiptTypes, RelationTypes, ) +from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -1272,3 +1273,117 @@ def test_incremental_sync(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.result["body"]) + + +class SyncStateAfterArchivedRoomTestCase(unittest.HomeserverTestCase): + """Tests MSC4222 `state_after` behaviour for rooms the syncing user has + left (i.e. rooms in the `leave` section of the sync response).""" + + servlets = [ + synapse.rest.admin.register_servlets, + room.register_servlets, + login.register_servlets, + sync.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + + def test_archived_room_state_after_not_newer_than_leave(self) -> None: + """`state_after` for a left room must be the state at the end of that + room's timeline, i.e. at the user's leave point — never state from + after the leave. + + Scenario: with lazy-loading of members and `use_state_after` enabled, + Alice does an incremental sync covering the window in which Bob sent a + message and Alice then left. Bob changed his per-room displayname + *after* Alice's leave; that post-leave membership event must NOT + appear in Alice's `state_after` for the left room. + """ + alice = self.register_user("alice", "password") + alice_tok = self.login("alice", "password") + bob = self.register_user("bob", "password") + bob_tok = self.login("bob", "password") + + # Opt Alice in to MSC4222. + self.get_success( + self.store.set_features_for_user(alice, {ExperimentalFeature.MSC4222: True}) + ) + + # Name the room to avoid heroes: those come from the *current* + # summary — a separate leak path from the one under test. + room_id = self.helper.create_room_as( + alice, tok=alice_tok, extra_content={"name": "Some room name"} + ) + self.helper.join(room_id, bob, tok=bob_tok) + + # Bob's membership as it will stand at Alice's leave point. + channel = self.make_request( + "GET", + f"/_matrix/client/v3/rooms/{room_id}/state/m.room.member/{bob}?format=event", + access_token=alice_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + bob_member_event_id_at_leave = channel.json_body["event_id"] + + # Lazy-load members; `include_redundant_members` bypasses the members + # cache so Bob's membership appears in the incremental sync below. + sync_filter = json.dumps( + { + "room": { + "state": { + "lazy_load_members": True, + "include_redundant_members": True, + }, + } + } + ) + sync_url = f"/sync?filter={sync_filter}&org.matrix.msc4222.use_state_after=true" + + # Initial sync. + channel = self.make_request("GET", sync_url, access_token=alice_tok) + self.assertEqual(channel.code, 200, channel.result) + since = channel.json_body["next_batch"] + + # Bob becomes a timeline sender in the next sync window. + self.helper.send(room_id, body="hello", tok=bob_tok) + + # Alice leaves the room. + self.helper.leave(room_id, alice, tok=alice_tok) + + # Bob's membership changes AFTER Alice's leave. + post_leave_member_event = self.helper.send_state( + room_id, + EventTypes.Member, + {"membership": "join", "displayname": "bob-post-leave"}, + tok=bob_tok, + state_key=bob, + ) + post_leave_member_event_id = post_leave_member_event["event_id"] + + # Incremental sync: the room is in the `leave` section. + channel = self.make_request( + "GET", f"{sync_url}&since={since}", access_token=alice_tok + ) + self.assertEqual(channel.code, 200, channel.result) + + left_room = channel.json_body["rooms"]["leave"][room_id] + state_after_events = left_room["org.matrix.msc4222.state_after"]["events"] + + # Post-leave state must not appear in `state_after`. + self.assertNotIn( + post_leave_member_event_id, + [e["event_id"] for e in state_after_events], + f"state_after contains state from after the user's leave: " + f"{state_after_events}", + ) + + # Bob's membership must be the one at the leave point. + self.assertEqual( + [ + e["event_id"] + for e in state_after_events + if e["type"] == EventTypes.Member and e["state_key"] == bob + ], + [bob_member_event_id_at_leave], + )