From 426a2af64e505ccc1123f407959bd981cc358e7a Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 20 Apr 2023 20:40:41 +0100 Subject: [PATCH 01/10] Don't fail to update read marker if the current event has gone --- synapse/handlers/read_marker.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/read_marker.py b/synapse/handlers/read_marker.py index 05122fd5a6b4..77414f3a72dc 100644 --- a/synapse/handlers/read_marker.py +++ b/synapse/handlers/read_marker.py @@ -15,6 +15,7 @@ import logging from typing import TYPE_CHECKING +from synapse.api.errors import SynapseError from synapse.util.async_helpers import Linearizer if TYPE_CHECKING: @@ -46,12 +47,21 @@ async def received_client_read_marker( ) should_update = True + # Get event ordering, this also ensures we know about the event + event_ordering = await self.store.get_event_ordering(event_id) if existing_read_marker: - # Only update if the new marker is ahead in the stream - should_update = await self.store.is_event_after( - event_id, existing_read_marker["event_id"] - ) + try: + old_event_ordering = await self.store.get_event_ordering( + existing_read_marker["event_id"] + ) + except SynapseError: + # Old event no longer exists, assume new is ahead. This may + # happen if the old event was removed due to retention. + pass + else: + # Only update if the new marker is ahead in the stream + should_update = event_ordering > old_event_ordering if should_update: content = {"event_id": event_id} From 9cc6b84597e8d42dba50cb205da1ea69be8f7f29 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 20 Apr 2023 20:40:44 +0100 Subject: [PATCH 02/10] Remove unused method --- synapse/storage/databases/main/events_worker.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index 0cf46626d2ff..fe1aca274090 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -1976,12 +1976,6 @@ def get_deltas_for_stream_id_txn( return rows, to_token, True - async def is_event_after(self, event_id1: str, event_id2: str) -> bool: - """Returns True if event_id1 is after event_id2 in the stream""" - to_1, so_1 = await self.get_event_ordering(event_id1) - to_2, so_2 = await self.get_event_ordering(event_id2) - return (to_1, so_1) > (to_2, so_2) - @cached(max_entries=5000) async def get_event_ordering(self, event_id: str) -> Tuple[int, int]: res = await self.db_pool.simple_select_one( From fbfe5f0d63125de0358879860059ea3941262633 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 20 Apr 2023 20:45:18 +0100 Subject: [PATCH 03/10] Add changelog --- changelog.d/15464.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15464.bugfix diff --git a/changelog.d/15464.bugfix b/changelog.d/15464.bugfix new file mode 100644 index 000000000000..f1eb6cddd92b --- /dev/null +++ b/changelog.d/15464.bugfix @@ -0,0 +1 @@ +Don't fail moving read marker if current set to a missing event. Contributed by Nick @ Beeper (@fizzadar). From cbfc04efaf2df0f41dc1e93c4dd112c9ebb9f54b Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Sun, 7 May 2023 13:01:05 +0200 Subject: [PATCH 04/10] Add read marker rest tests --- tests/rest/client/test_read_marker.py | 155 ++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 tests/rest/client/test_read_marker.py diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py new file mode 100644 index 000000000000..2ce79ebd6556 --- /dev/null +++ b/tests/rest/client/test_read_marker.py @@ -0,0 +1,155 @@ +# Copyright 2022 Beeper +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from twisted.test.proto_helpers import MemoryReactor + +import synapse.rest.admin +from synapse.api.constants import EventContentFields, EventTypes +from synapse.rest import admin +from synapse.rest.client import login, read_marker, register, room +from synapse.server import HomeServer +from synapse.util import Clock + +from tests import unittest + +one_hour_ms = 3600000 +one_day_ms = one_hour_ms * 24 + + +class ReadMarkerTestCase(unittest.HomeserverTestCase): + servlets = [ + login.register_servlets, + register.register_servlets, + read_marker.register_servlets, + room.register_servlets, + synapse.rest.admin.register_servlets, + admin.register_servlets, + ] + + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: + config = self.default_config() + + # merge this default retention config with anything that was specified in + # @override_config + retention_config = { + "enabled": True, + "allowed_lifetime_min": one_day_ms, + "allowed_lifetime_max": one_day_ms * 3, + } + retention_config.update(config.get("retention", {})) + config["retention"] = retention_config + + self.hs = self.setup_test_homeserver(config=config) + + return self.hs + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.owner = self.register_user("owner", "pass") + self.owner_tok = self.login("owner", "pass") + self.store = self.hs.get_datastores().main + self.clock = self.hs.get_clock() + + def test_send_read_marker(self) -> None: + room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok) + + def send_message(): + res = self.helper.send_event( + room_id=room_id, + type=EventTypes.Message, + content={ + "msgtype": "m.text", + "body": "with right label", + EventContentFields.LABELS: ["#fun"], + }, + tok=self.owner_tok, + ) + return res["event_id"] + + # Test setting the read marker on the room + event_id_1 = send_message() + + channel = self.make_request( + "POST", + "/rooms/!abc:beep/read_markers", + content={ + "m.fully_read": event_id_1, + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + # Test moving the read marker to a newer event + event_id_2 = send_message() + channel = self.make_request( + "POST", + "/rooms/!abc:beep/read_markers", + content={ + "m.fully_read": event_id_2, + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + def test_send_read_marker_missing_previous_event(self) -> None: + """ + Test moving a read marker from an event that previously existed but was + later removed due to retention rules. + """ + + room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok) + + # Set retention rule on the room so we remove old events to test this case + self.helper.send_state( + room_id=room_id, + event_type=EventTypes.Retention, + body={"max_lifetime": one_day_ms}, + tok=self.owner_tok, + ) + + def send_message(): + res = self.helper.send(room_id=room_id, body="1", tok=self.owner_tok) + return res["event_id"] + + # Test setting the read marker on the room + event_id_1 = send_message() + + channel = self.make_request( + "POST", + "/rooms/!abc:beep/read_markers", + content={ + "m.fully_read": event_id_1, + }, + access_token=self.owner_tok, + ) + + # Send a second message (retention will not remove the latest event ever) + send_message() + # And then advance so retention rules remove the first event (where the marker is) + self.reactor.advance(one_day_ms * 2 / 1000) + + event = self.get_success(self.store.get_event(event_id_1, allow_none=True)) + assert event is None + + self.store.get_event_ordering.invalidate_all() + + # Test moving the read marker to a newer event + event_id_2 = send_message() + channel = self.make_request( + "POST", + "/rooms/!abc:beep/read_markers", + content={ + "m.fully_read": event_id_2, + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) From 4f3134e533aaa578c5db3391fe124e631021f9b0 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Sun, 7 May 2023 13:24:37 +0200 Subject: [PATCH 05/10] Fixup typing --- tests/rest/client/test_read_marker.py | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py index 2ce79ebd6556..b6de06343a20 100644 --- a/tests/rest/client/test_read_marker.py +++ b/tests/rest/client/test_read_marker.py @@ -62,17 +62,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def test_send_read_marker(self) -> None: room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok) - def send_message(): - res = self.helper.send_event( - room_id=room_id, - type=EventTypes.Message, - content={ - "msgtype": "m.text", - "body": "with right label", - EventContentFields.LABELS: ["#fun"], - }, - tok=self.owner_tok, - ) + def send_message() -> str: + res = self.helper.send(room_id=room_id, body="1", tok=self.owner_tok) return res["event_id"] # Test setting the read marker on the room @@ -116,7 +107,7 @@ def test_send_read_marker_missing_previous_event(self) -> None: tok=self.owner_tok, ) - def send_message(): + def send_message() -> str: res = self.helper.send(room_id=room_id, body="1", tok=self.owner_tok) return res["event_id"] From 09285ab2fa48b15fd09a97fe10c8120aae0f510a Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Sun, 7 May 2023 13:28:52 +0200 Subject: [PATCH 06/10] Fix linting --- tests/rest/client/test_read_marker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py index b6de06343a20..b43271d52cba 100644 --- a/tests/rest/client/test_read_marker.py +++ b/tests/rest/client/test_read_marker.py @@ -14,7 +14,7 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import EventContentFields, EventTypes +from synapse.api.constants import EventTypes from synapse.rest import admin from synapse.rest.client import login, read_marker, register, room from synapse.server import HomeServer From 2e2f98ad1f3e4085b27ccc14d3229c734e12a68b Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Tue, 16 May 2023 19:28:19 +0100 Subject: [PATCH 07/10] Better changelog Co-authored-by: Patrick Cloke --- changelog.d/15464.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/15464.bugfix b/changelog.d/15464.bugfix index f1eb6cddd92b..3c655989b3e1 100644 --- a/changelog.d/15464.bugfix +++ b/changelog.d/15464.bugfix @@ -1 +1 @@ -Don't fail moving read marker if current set to a missing event. Contributed by Nick @ Beeper (@fizzadar). +Fix a long-standing bug where setting the read marker could fail when using message retention. Contributed by Nick @ Beeper (@fizzadar). From 0987445fd04ed3214e763721ef1ffb50a31826a3 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Tue, 16 May 2023 19:28:47 +0100 Subject: [PATCH 08/10] Fix copyright date Co-authored-by: Patrick Cloke --- tests/rest/client/test_read_marker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py index b43271d52cba..3dc4c314f350 100644 --- a/tests/rest/client/test_read_marker.py +++ b/tests/rest/client/test_read_marker.py @@ -1,4 +1,4 @@ -# Copyright 2022 Beeper +# Copyright 2023 Beeper # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 8c3410039d8a95cef9df0f78989157806404075d Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Thu, 18 May 2023 18:45:47 +0100 Subject: [PATCH 09/10] Add comment about invalidation call Co-authored-by: Patrick Cloke --- tests/rest/client/test_read_marker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py index 3dc4c314f350..04295c6d8843 100644 --- a/tests/rest/client/test_read_marker.py +++ b/tests/rest/client/test_read_marker.py @@ -131,6 +131,7 @@ def send_message() -> str: event = self.get_success(self.store.get_event(event_id_1, allow_none=True)) assert event is None + # TODO See https://github.com/matrix-org/synapse/issues/13476 self.store.get_event_ordering.invalidate_all() # Test moving the read marker to a newer event From 71e135349f950a663e25c36caa047f5decdd6c7a Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Thu, 18 May 2023 18:48:16 +0100 Subject: [PATCH 10/10] Capitalize constants --- tests/rest/client/test_read_marker.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/rest/client/test_read_marker.py b/tests/rest/client/test_read_marker.py index 04295c6d8843..0eedcdb476b4 100644 --- a/tests/rest/client/test_read_marker.py +++ b/tests/rest/client/test_read_marker.py @@ -22,8 +22,8 @@ from tests import unittest -one_hour_ms = 3600000 -one_day_ms = one_hour_ms * 24 +ONE_HOUR_MS = 3600000 +ONE_DAY_MS = ONE_HOUR_MS * 24 class ReadMarkerTestCase(unittest.HomeserverTestCase): @@ -43,8 +43,8 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: # @override_config retention_config = { "enabled": True, - "allowed_lifetime_min": one_day_ms, - "allowed_lifetime_max": one_day_ms * 3, + "allowed_lifetime_min": ONE_DAY_MS, + "allowed_lifetime_max": ONE_DAY_MS * 3, } retention_config.update(config.get("retention", {})) config["retention"] = retention_config @@ -103,7 +103,7 @@ def test_send_read_marker_missing_previous_event(self) -> None: self.helper.send_state( room_id=room_id, event_type=EventTypes.Retention, - body={"max_lifetime": one_day_ms}, + body={"max_lifetime": ONE_DAY_MS}, tok=self.owner_tok, ) @@ -126,7 +126,7 @@ def send_message() -> str: # Send a second message (retention will not remove the latest event ever) send_message() # And then advance so retention rules remove the first event (where the marker is) - self.reactor.advance(one_day_ms * 2 / 1000) + self.reactor.advance(ONE_DAY_MS * 2 / 1000) event = self.get_success(self.store.get_event(event_id_1, allow_none=True)) assert event is None