From 4fd17d479027711c7d2f40a813720ff22e3e6893 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 10 Oct 2025 03:31:07 -0400 Subject: [PATCH 01/47] MSC4140: finalised delayed events, and more - Store sent/cancelled/failed delayed events, i.e. finalised delayed events, and support looking them up to inspect whether a delayed event was sent or not. Set limits on how many finalised events to store. - Support looking up delayed events by ID - Return 200 when retrying the same action (send/cancel) on an already-finalised delayed event, or 409 for a conflicting action - Limit how many delayed events a user may have scheduled at a time --- synapse/config/experimental.py | 15 + synapse/handlers/delayed_events.py | 124 +++- synapse/rest/client/delayed_events.py | 35 +- .../storage/databases/main/delayed_events.py | 533 +++++++++++++++--- synapse/storage/schema/__init__.py | 6 +- .../93/01_add_finalised_delayed_events.sql | 25 + tests/rest/client/test_delayed_events.py | 183 ++++-- tests/rest/client/test_rooms.py | 50 ++ 8 files changed, 826 insertions(+), 145 deletions(-) create mode 100644 synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 04ca6e3c517..2e077f8afc3 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -556,6 +556,21 @@ def read_config( # MSC4133: Custom profile fields self.msc4133_enabled: bool = experimental.get("msc4133_enabled", False) + # MSC4140: How many delayed events a user is allowed to have scheduled at a time. + self.msc4140_max_delayed_events_per_user = experimental.get( + "msc4140_max_delayed_events_per_user", 100 + ) + + # MSC4140: How long to keep finalised delayed events in the database before deleting them. + self.msc4140_finalised_retention_period = self.parse_duration( + config.get("msc4140_finalised_retention_period", "7d") + ) + + # MSC4140: How many finalised delayed events to keep per user before deleting them. + self.msc4140_finalised_retention_limit = experimental.get( + "msc4140_finalised_retention_limit", 1000 + ) + # MSC4143: Matrix RTC Transport using Livekit Backend self.msc4143_enabled: bool = experimental.get("msc4143_enabled", False) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 79dd3e84165..3a3c6ae22a1 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -13,17 +13,18 @@ # import logging -from typing import TYPE_CHECKING, List, Optional, Set, Tuple +from typing import TYPE_CHECKING, Dict, List, Optional, Set, Tuple from twisted.internet.interfaces import IDelayedCall from synapse.api.constants import EventTypes -from synapse.api.errors import ShadowBanError, SynapseError +from synapse.api.errors import ShadowBanError, SynapseError, cs_error from synapse.api.ratelimiting import Ratelimiter from synapse.config.workers import MAIN_PROCESS_INSTANCE_NAME from synapse.logging.context import make_deferred_yieldable from synapse.logging.opentracing import set_tag from synapse.metrics import SERVER_NAME_LABEL, event_processing_positions +from synapse.metrics.background_process_metrics import wrap_as_background_process from synapse.replication.http.delayed_events import ( ReplicationAddedDelayedEventRestServlet, ) @@ -43,6 +44,7 @@ UserID, create_requester, ) +from synapse.util.constants import MILLISECONDS_PER_SECOND, ONE_MINUTE_SECONDS from synapse.util.events import generate_fake_event_id from synapse.util.metrics import Measure from synapse.util.sentinel import Sentinel @@ -125,6 +127,12 @@ async def _schedule_db_events() -> None: else: self._repl_client = ReplicationAddedDelayedEventRestServlet.make_client(hs) + if hs.config.worker.run_background_tasks: + self._clock.looping_call( + self._prune_finalised_events, + 5 * ONE_MINUTE_SECONDS * MILLISECONDS_PER_SECOND, + ) + @property def _is_master(self) -> bool: return self._repl_client is None @@ -132,7 +140,7 @@ def _is_master(self) -> bool: def notify_new_event(self) -> None: """ Called when there may be more state event deltas to process, - which should cancel pending delayed events for the same state. + which should cancel scheduled delayed events for the same state. """ if self._event_processing: return @@ -156,8 +164,7 @@ async def _unsafe_process_new_event(self) -> None: room_max_stream_ordering = self._store.get_room_max_stream_ordering() # Check that there are actually any delayed events to process. If not, bail early. - delayed_events_count = await self._store.get_count_of_delayed_events() - if delayed_events_count == 0: + if not await self._store.has_scheduled_delayed_events(): # There are no delayed events to process. Update the # `delayed_events_stream_pos` to the latest `events` stream pos and # exit early. @@ -228,7 +235,7 @@ async def _unsafe_process_new_event(self) -> None: async def _handle_state_deltas(self, deltas: List[StateDelta]) -> None: """ - Process current state deltas to cancel other users' pending delayed events + Process current state deltas to cancel other users' scheduled delayed events that target the same state. """ # Get the senders of each delta's state event (as sender information is @@ -316,11 +323,20 @@ async def _handle_state_deltas(self, deltas: List[StateDelta]) -> None: if sender.domain == self._config.server.server_name else "" ), + finalised_ts=self._get_current_ts(), ) if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) + @wrap_as_background_process("_prune_finalised_events") + async def _prune_finalised_events(self) -> None: + await self._store.prune_finalised_delayed_events( + self._get_current_ts(), + self.hs.config.experimental.msc4140_finalised_retention_period, + self.hs.config.experimental.msc4140_finalised_retention_limit, + ) + async def add( self, requester: Requester, @@ -380,6 +396,7 @@ async def add( origin_server_ts=origin_server_ts, content=content, delay=delay, + limit=self.hs.config.experimental.msc4140_max_delayed_events_per_user, ) if self._repl_client is not None: @@ -420,6 +437,7 @@ async def cancel(self, requester: Requester, delay_id: str) -> None: next_send_ts = await self._store.cancel_delayed_event( delay_id=delay_id, user_localpart=requester.user.localpart, + finalised_ts=self._get_current_ts(), ) if self._next_send_ts_changed(next_send_ts): @@ -477,18 +495,19 @@ async def send(self, requester: Requester, delay_id: str) -> None: if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) - await self._send_event( - DelayedEventDetails( - delay_id=DelayID(delay_id), - user_localpart=UserLocalpart(requester.user.localpart), - room_id=event.room_id, - type=event.type, - state_key=event.state_key, - origin_server_ts=event.origin_server_ts, - content=event.content, - device_id=event.device_id, + if event: + await self._send_event( + DelayedEventDetails( + delay_id=DelayID(delay_id), + user_localpart=UserLocalpart(requester.user.localpart), + room_id=event.room_id, + type=event.type, + state_key=event.state_key, + origin_server_ts=event.origin_server_ts, + content=event.content, + device_id=event.device_id, + ) ) - ) async def _send_on_timeout(self) -> None: self._next_delayed_event_call = None @@ -513,17 +532,19 @@ async def _send_events(self, events: List[DelayedEventDetails]) -> None: state_info = None try: # TODO: send in background if message event or non-conflicting state event - await self._send_event(event) + finalised_ts = await self._send_event(event) if state_info is not None: sent_state.add(state_info) except Exception: logger.exception("Failed to send delayed event") + finalised_ts = self._get_current_ts() for room_id, event_type, state_key in sent_state: - await self._store.delete_processed_delayed_state_events( + await self._store.finalise_processed_delayed_state_events( room_id=str(room_id), event_type=event_type, state_key=state_key, + finalised_ts=finalised_ts, ) def _schedule_next_at_or_none(self, next_send_ts: Optional[Timestamp]) -> None: @@ -547,21 +568,49 @@ def _schedule_next_at(self, next_send_ts: Timestamp) -> None: else: self._next_delayed_event_call.reset(delay_sec) - async def get_all_for_user(self, requester: Requester) -> List[JsonDict]: - """Return all pending delayed events requested by the given user.""" + async def get_delayed_events_for_user( + self, + requester: Requester, + delay_ids: Optional[List[str]], + get_scheduled: bool, + get_finalised: bool, + ) -> Dict[str, List[JsonDict]]: + """ + Return all scheduled delayed events for the given user. + + Args: + requester: The user whose delayed events to get. + delay_ids: The IDs of the delayed events to get, or None to get all of them. + get_scheduled: Whether to look up scheduled delayed events. + get_finalised: Whether to look up finalised delayed events. + """ await self._delayed_event_mgmt_ratelimiter.ratelimit( requester, (requester.user.to_string(), requester.device_id), ) - return await self._store.get_all_delayed_events_for_user( - requester.user.localpart - ) + + # TODO: Support Pagination stream API + ret = {} + if get_scheduled: + ret["scheduled"] = await self._store.get_scheduled_delayed_events_for_user( + requester.user.localpart, + delay_ids, + ) + if get_finalised: + ret["finalised"] = await self._store.get_finalised_delayed_events_for_user( + requester.user.localpart, + delay_ids, + self._get_current_ts(), + self.hs.config.experimental.msc4140_finalised_retention_period, + self.hs.config.experimental.msc4140_finalised_retention_limit, + ) + return ret async def _send_event( self, event: DelayedEventDetails, txn_id: Optional[str] = None, - ) -> None: + ) -> Timestamp: user_id = UserID(event.user_localpart, self._config.server.server_name) user_id_str = user_id.to_string() # Create a new requester from what data is currently available @@ -571,6 +620,7 @@ async def _send_event( device_id=event.device_id, ) + finalised_ts = None try: if event.state_key is not None and event.type == EventTypes.Member: membership = event.content.get("membership") @@ -606,18 +656,34 @@ async def _send_event( txn_id=txn_id, ) event_id = sent_event.event_id + if event.origin_server_ts is None: + finalised_ts = Timestamp(sent_event.origin_server_ts) except ShadowBanError: event_id = generate_fake_event_id() + send_error = None + except SynapseError as e: + send_error = e.error_dict(None) + except Exception: + send_error = cs_error("Internal server error") + else: + send_error = None finally: # TODO: If this is a temporary error, retry. Otherwise, consider notifying clients of the failure + if finalised_ts is None: + finalised_ts = self._get_current_ts() try: - await self._store.delete_processed_delayed_event( - event.delay_id, event.user_localpart + await self._store.finalise_processed_delayed_event( + event.delay_id, + event.user_localpart, + send_error or event_id, + finalised_ts, ) except Exception: - logger.exception("Failed to delete processed delayed event") + logger.exception("Failed to finalise processed delayed event") - set_tag("event_id", event_id) + if send_error is None: + set_tag("event_id", event_id) + return finalised_ts def _get_current_ts(self) -> Timestamp: return Timestamp(self._clock.time_msec()) diff --git a/synapse/rest/client/delayed_events.py b/synapse/rest/client/delayed_events.py index 2dd5a60b2b1..e069fdd835e 100644 --- a/synapse/rest/client/delayed_events.py +++ b/synapse/rest/client/delayed_events.py @@ -21,7 +21,12 @@ from synapse.api.errors import Codes, SynapseError from synapse.http.server import HttpServer -from synapse.http.servlet import RestServlet, parse_json_object_from_request +from synapse.http.servlet import ( + RestServlet, + parse_json_object_from_request, + parse_string_from_args, + parse_strings_from_args, +) from synapse.http.site import SynapseRequest from synapse.rest.client._base import client_patterns from synapse.types import JsonDict @@ -38,6 +43,11 @@ class _UpdateDelayedEventAction(Enum): SEND = "send" +class _DelayedEventStatus(Enum): + SCHEDULED = "scheduled" + FINALISED = "finalised" + + class UpdateDelayedEventServlet(RestServlet): PATTERNS = client_patterns( r"/org\.matrix\.msc4140/delayed_events/(?P[^/]+)$", @@ -97,10 +107,27 @@ def __init__(self, hs: "HomeServer"): async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - # TODO: Support Pagination stream API ("from" query parameter) - delayed_events = await self.delayed_events_handler.get_all_for_user(requester) - ret = {"delayed_events": delayed_events} + # twisted.web.server.Request.args is incorrectly defined as Optional[Any] + args: dict[bytes, list[bytes]] = request.args # type: ignore + statuses = parse_strings_from_args( + args, + "status", + allowed_values=tuple(s.value for s in _DelayedEventStatus), + ) + delay_ids = parse_strings_from_args(args, "delay_id") + # TODO: Support Pagination stream API + _from_token = parse_string_from_args(args, "from") + + ret = await self.delayed_events_handler.get_delayed_events_for_user( + requester, + delay_ids, + statuses is None or _DelayedEventStatus.SCHEDULED.value in statuses, + statuses is None or _DelayedEventStatus.FINALISED.value in statuses, + ) + # TODO: This is here for backwards compatibility. Remove eventually + if statuses is None: + ret["delayed_events"] = ret[_DelayedEventStatus.SCHEDULED.value] return 200, ret diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 78f55b983f0..b5fc0edc851 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -13,13 +13,17 @@ # import logging -from typing import List, NewType, Optional, Tuple +from http import HTTPStatus +from typing import List, NewType, Optional, Tuple, Union import attr -from synapse.api.errors import NotFoundError +from synapse.api.errors import NotFoundError, SynapseError, cs_error from synapse.storage._base import SQLBaseStore, db_to_json -from synapse.storage.database import LoggingTransaction, StoreError +from synapse.storage.database import ( + LoggingTransaction, + make_in_list_sql_clause, +) from synapse.storage.engines import PostgresEngine from synapse.types import JsonDict, RoomID from synapse.util import stringutils @@ -58,7 +62,7 @@ class DelayedEventsStore(SQLBaseStore): async def get_delayed_events_stream_pos(self) -> int: """ Gets the stream position of the background process to watch for state events - that target the same piece of state as any pending delayed events. + that target the same piece of state as any scheduled delayed events. """ return await self.db_pool.simple_select_one_onecol( table="delayed_events_stream_pos", @@ -70,7 +74,7 @@ async def get_delayed_events_stream_pos(self) -> int: async def update_delayed_events_stream_pos(self, stream_id: Optional[int]) -> None: """ Updates the stream position of the background process to watch for state events - that target the same piece of state as any pending delayed events. + that target the same piece of state as any scheduled delayed events. Must only be used by the worker running the background process. """ @@ -93,6 +97,7 @@ async def add_delayed_event( origin_server_ts: Optional[int], content: JsonDict, delay: int, + limit: int, ) -> Tuple[DelayID, Timestamp]: """ Inserts a new delayed event in the DB. @@ -100,11 +105,36 @@ async def add_delayed_event( Returns: The generated ID assigned to the added delayed event, and the send time of the next delayed event to be sent, which is either the event just added or one added earlier. + + Raises: + SynapseError: if the user has reached the limit of how many + delayed events they may have scheduled at a time. """ delay_id = _generate_delay_id() send_ts = Timestamp(creation_ts + delay) def add_delayed_event_txn(txn: LoggingTransaction) -> Timestamp: + txn.execute( + """ + SELECT COUNT(*) FROM delayed_events + WHERE user_localpart = ? + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + """, + (user_localpart,), + ) + num_existing: int = txn.fetchall()[0][0] + if num_existing >= limit: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "The maximum number of delayed events has been reached.", + additional_fields={ + "org.matrix.msc4140.errcode": "M_MAX_DELAY_UNSUPPORTED", + }, + ) + self.db_pool.simple_insert_txn( txn, table="delayed_events", @@ -154,6 +184,7 @@ async def restart_delayed_event( Raises: NotFoundError: if there is no matching delayed event. + SynapseError: if the delayed event has already been finalised. """ def restart_delayed_event_txn( @@ -165,6 +196,10 @@ def restart_delayed_event_txn( SET send_ts = ? + delay WHERE delay_id = ? AND user_localpart = ? AND NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) """, ( current_ts, @@ -173,7 +208,19 @@ def restart_delayed_event_txn( ), ) if txn.rowcount == 0: - raise NotFoundError("Delayed event not found") + sent = self.db_pool.simple_select_one_onecol_txn( + txn, + "finalised_delayed_events", + keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, + retcol="event_id IS NOT NULL", + allow_none=True, + ) + if sent is None: + raise NotFoundError("Delayed event not found") + raise SynapseError( + HTTPStatus.CONFLICT, + f"Delayed event has already been {'sent' if sent else 'cancelled'}", + ) next_send_ts = self._get_next_delayed_event_send_ts_txn(txn) assert next_send_ts is not None @@ -183,30 +230,123 @@ def restart_delayed_event_txn( "restart_delayed_event", restart_delayed_event_txn ) - async def get_count_of_delayed_events(self) -> int: - """Returns the number of pending delayed events in the DB.""" + async def has_scheduled_delayed_events(self) -> bool: + """Returns whether there are any scheduled delayed events in the DB.""" - def _get_count_of_delayed_events(txn: LoggingTransaction) -> int: - sql = "SELECT count(*) FROM delayed_events" + rows = await self.db_pool.execute( + "has_scheduled_delayed_events", + """ + SELECT 1 WHERE EXISTS ( + SELECT * FROM delayed_events + WHERE (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + ) + """, + ) + return bool(rows) - txn.execute(sql) - resp = txn.fetchone() - return resp[0] if resp is not None else 0 + async def prune_finalised_delayed_events( + self, + current_ts: Timestamp, + retention_period: int, + retention_limit: int, + ) -> None: + def prune_finalised_delayed_events(txn: LoggingTransaction) -> None: + self._prune_expired_finalised_delayed_events( + txn, current_ts, retention_period + ) - return await self.db_pool.runInteraction( - "get_count_of_delayed_events", - _get_count_of_delayed_events, + for user_localpart in self.db_pool.simple_select_onecol_txn( + txn, + "finalised_delayed_events", + keyvalues={}, + retcol="DISTINCT(user_localpart)", + ): + self._prune_excess_finalised_delayed_events_for_user( + txn, user_localpart, retention_limit + ) + + await self.db_pool.runInteraction( + "prune_finalised_delayed_events", prune_finalised_delayed_events + ) + + def _prune_expired_finalised_delayed_events( + self, txn: LoggingTransaction, current_ts: Timestamp, retention_period: int + ) -> None: + """ + Delete all finalised delayed events that had finalised + before the end of the given retention period. + """ + txn.execute( + """ + DELETE FROM delayed_events + WHERE (delay_id, user_localpart) IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + WHERE ? - finalised_ts > ? + ) + """, + ( + current_ts, + retention_period, + ), ) - async def get_all_delayed_events_for_user( + def _prune_excess_finalised_delayed_events_for_user( + self, txn: LoggingTransaction, user_localpart: str, retention_limit: int + ) -> None: + """ + Delete the oldest finalised delayed events for the given user, + such that no more of them remain than the given retention limit. + """ + num_existing = self.db_pool.simple_select_one_onecol_txn( + txn, + "finalised_delayed_events", + keyvalues={"user_localpart": user_localpart}, + retcol="COUNT(*)", + ) + if num_existing > retention_limit: + txn.execute( + """ + DELETE FROM delayed_events + WHERE (delay_id, user_localpart) IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + WHERE user_localpart = ? + ORDER BY finalised_ts + LIMIT ? + ) + """, + ( + user_localpart, + num_existing - retention_limit, + ), + ) + + async def get_scheduled_delayed_events_for_user( self, user_localpart: str, + delay_ids: Optional[List[str]], ) -> List[JsonDict]: - """Returns all pending delayed events owned by the given user.""" + """Returns all scheduled delayed events for the given user.""" # TODO: Support Pagination stream API ("next_batch" field) + sql_where = """WHERE user_localpart = ? + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + )""" + sql_args = [user_localpart] + if delay_ids: + delay_id_clause_sql, delay_id_clause_args = make_in_list_sql_clause( + self.database_engine, "delay_id", delay_ids + ) + sql_where += f" AND {delay_id_clause_sql}" + sql_args.extend(delay_id_clause_args) rows = await self.db_pool.execute( - "get_all_delayed_events_for_user", - """ + "get_scheduled_delayed_events_for_user", + f""" SELECT delay_id, room_id, @@ -216,10 +356,10 @@ async def get_all_delayed_events_for_user( send_ts, content FROM delayed_events - WHERE user_localpart = ? AND NOT is_processed + {sql_where} ORDER BY send_ts """, - user_localpart, + *sql_args, ) return [ { @@ -234,6 +374,92 @@ async def get_all_delayed_events_for_user( for row in rows ] + async def get_finalised_delayed_events_for_user( + self, + user_localpart: str, + delay_ids: Optional[List[str]], + current_ts: Timestamp, + retention_period: int, + retention_limit: int, + ) -> List[JsonDict]: + """Returns all finalised delayed events for the given user.""" + # TODO: Support Pagination stream API ("next_batch" field) + + def get_finalised_delayed_events_for_user( + txn: LoggingTransaction, + ) -> List[JsonDict]: + # Clear up some space in the DB before returning any results. + self._prune_expired_finalised_delayed_events( + txn, current_ts, retention_period + ) + self._prune_excess_finalised_delayed_events_for_user( + txn, user_localpart, retention_limit + ) + + sql_where = "WHERE user_localpart = ?" + sql_args = [user_localpart] + if delay_ids: + delay_id_clause_sql, delay_id_clause_args = make_in_list_sql_clause( + self.database_engine, "delay_id", delay_ids + ) + sql_where += f" AND {delay_id_clause_sql}" + sql_args.extend(delay_id_clause_args) + txn.execute( + f""" + SELECT + delay_id, + room_id, + event_type, + state_key, + delay, + send_ts, + content, + error, + event_id, + finalised_ts + FROM delayed_events + JOIN finalised_delayed_events + USING (delay_id, user_localpart) + {sql_where} + ORDER BY finalised_ts + """, + sql_args, + ) + return [ + { + "delayed_event": { + "delay_id": DelayID(row[0]), + "room_id": str(RoomID.from_string(row[1])), + "type": EventType(row[2]), + **( + {"state_key": StateKey(row[3])} + if row[3] is not None + else {} + ), + "delay": Delay(row[4]), + "running_since": Timestamp(row[5] - row[4]), + "content": db_to_json(row[6]), + }, + "outcome": "cancel" if row[8] is None else "send", + "reason": ( + "error" + if row[7] is not None + else "action" + if row[9] < row[5] + else "delay" + ), + **({"error": db_to_json(row[7])} if row[7] is not None else {}), + **({"event_id": str(row[8])} if row[8] is not None else {}), + "origin_server_ts": Timestamp(row[9]), + } + for row in txn + ] + + return await self.db_pool.runInteraction( + "get_finalised_delayed_events_for_user", + get_finalised_delayed_events_for_user, + ) + async def process_timeout_delayed_events( self, current_ts: Timestamp ) -> Tuple[ @@ -268,7 +494,12 @@ def process_timeout_delayed_events_txn( ) ) sql_update = "UPDATE delayed_events SET is_processed = TRUE" - sql_where = "WHERE send_ts <= ? AND NOT is_processed" + sql_where = """WHERE send_ts <= ? + AND NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + )""" sql_args = (current_ts,) sql_order = "ORDER BY send_ts" if isinstance(self.database_engine, PostgresEngine): @@ -323,7 +554,7 @@ async def process_target_delayed_event( delay_id: str, user_localpart: str, ) -> Tuple[ - EventDetails, + Optional[EventDetails], Optional[Timestamp], ]: """ @@ -339,12 +570,13 @@ async def process_target_delayed_event( Raises: NotFoundError: if there is no matching delayed event. + SynapseError: if the delayed event has already been cancelled. """ def process_target_delayed_event_txn( txn: LoggingTransaction, ) -> Tuple[ - EventDetails, + Optional[EventDetails], Optional[Timestamp], ]: sql_cols = ", ".join( @@ -358,7 +590,12 @@ def process_target_delayed_event_txn( ) ) sql_update = "UPDATE delayed_events SET is_processed = TRUE" - sql_where = "WHERE delay_id = ? AND user_localpart = ? AND NOT is_processed" + sql_where = """WHERE delay_id = ? AND user_localpart = ? + AND NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + )""" sql_args = (delay_id, user_localpart) txn.execute( ( @@ -370,7 +607,21 @@ def process_target_delayed_event_txn( ) row = txn.fetchone() if row is None: - raise NotFoundError("Delayed event not found") + sent = self.db_pool.simple_select_one_onecol_txn( + txn, + "finalised_delayed_events", + keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, + retcol="event_id IS NOT NULL", + allow_none=True, + ) + if sent is None: + raise NotFoundError("Delayed event not found") + elif not sent: + raise SynapseError( + HTTPStatus.CONFLICT, + "Delayed event has already been cancelled", + ) + return None, None elif not self.database_engine.supports_returning: txn.execute(f"{sql_update} {sql_where}", sql_args) assert txn.rowcount == 1 @@ -395,6 +646,7 @@ async def cancel_delayed_event( *, delay_id: str, user_localpart: str, + finalised_ts: Timestamp, ) -> Optional[Timestamp]: """ Cancels the matching delayed event, i.e. remove it as long as it hasn't been processed. @@ -407,27 +659,56 @@ async def cancel_delayed_event( Raises: NotFoundError: if there is no matching delayed event. + SynapseError: if the delayed event has already been sent. """ def cancel_delayed_event_txn( txn: LoggingTransaction, ) -> Optional[Timestamp]: - try: - self.db_pool.simple_delete_one_txn( + txn.execute( + """ + WITH matching_delayed_events AS ( + SELECT * FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + AND NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + ) + INSERT INTO finalised_delayed_events ( + delay_id, + user_localpart, + finalised_ts + ) + SELECT + delay_id, + user_localpart, + ? + FROM matching_delayed_events + RETURNING 1 + """, + ( + delay_id, + user_localpart, + finalised_ts, + ), + ) + if not txn.fetchall(): + sent = self.db_pool.simple_select_one_onecol_txn( txn, - table="delayed_events", - keyvalues={ - "delay_id": delay_id, - "user_localpart": user_localpart, - "is_processed": False, - }, + "finalised_delayed_events", + keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, + retcol="event_id IS NOT NULL", ) - except StoreError: - if txn.rowcount == 0: + if sent is None: raise NotFoundError("Delayed event not found") - else: - raise - + elif sent: + raise SynapseError( + HTTPStatus.CONFLICT, + "Delayed event has already been sent", + ) + return None return self._get_next_delayed_event_send_ts_txn(txn) return await self.db_pool.runInteraction( @@ -441,6 +722,7 @@ async def cancel_delayed_state_events( event_type: str, state_key: str, not_from_localpart: str, + finalised_ts: Timestamp, ) -> Optional[Timestamp]: """ Cancels all matching delayed state events, i.e. remove them as long as they haven't been processed. @@ -460,16 +742,36 @@ def cancel_delayed_state_events_txn( ) -> Optional[Timestamp]: txn.execute( """ - DELETE FROM delayed_events - WHERE room_id = ? AND event_type = ? AND state_key = ? - AND user_localpart <> ? - AND NOT is_processed + WITH matching_delayed_events AS ( + SELECT * FROM delayed_events + WHERE room_id = ? AND event_type = ? AND state_key = ? + AND user_localpart <> ? + AND NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + ) + INSERT INTO finalised_delayed_events ( + delay_id, + user_localpart, + error, + finalised_ts + ) + SELECT + delay_id, + user_localpart, + ?, + ? + FROM matching_delayed_events """, ( room_id, event_type, state_key, not_from_localpart, + _generate_cancelled_by_state_update_json(), + finalised_ts, ), ) return self._get_next_delayed_event_send_ts_txn(txn) @@ -478,57 +780,118 @@ def cancel_delayed_state_events_txn( "cancel_delayed_state_events", cancel_delayed_state_events_txn ) - async def delete_processed_delayed_event( + async def finalise_processed_delayed_event( self, delay_id: DelayID, user_localpart: UserLocalpart, + result_or_error: Union[str, JsonDict], + finalised_ts: Timestamp, ) -> None: """ - Delete the matching delayed event, as long as it has been marked as processed. + Finalise the matching delayed event, as long as it has been marked as processed. Throws: StoreError: if there is no matching delayed event, or if it has not yet been processed. """ - return await self.db_pool.simple_delete_one( - table="delayed_events", - keyvalues={ - "delay_id": delay_id, - "user_localpart": user_localpart, - "is_processed": True, - }, - desc="delete_processed_delayed_event", + if isinstance(result_or_error, str): + event_id = result_or_error + send_error = None + else: + event_id = None + send_error = result_or_error + + await self.db_pool.execute( + "finalise_processed_delayed_event", + """ + WITH matching_delayed_events AS ( + SELECT * FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + AND is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + ) + INSERT INTO finalised_delayed_events ( + delay_id, + user_localpart, + error, + event_id, + finalised_ts + ) + SELECT + delay_id, + user_localpart, + ?, + ?, + ? + FROM matching_delayed_events + """, + delay_id, + user_localpart, + json_encoder.encode(send_error) if send_error else None, + event_id, + finalised_ts, ) - async def delete_processed_delayed_state_events( + async def finalise_processed_delayed_state_events( self, *, room_id: str, event_type: str, state_key: str, + finalised_ts: Timestamp, ) -> None: """ - Delete the matching delayed state events that have been marked as processed. + Finalise the matching delayed state events that have been marked as processed. """ - await self.db_pool.simple_delete( - table="delayed_events", - keyvalues={ - "room_id": room_id, - "event_type": event_type, - "state_key": state_key, - "is_processed": True, - }, - desc="delete_processed_delayed_state_events", + await self.db_pool.execute( + "finalise_processed_delayed_state_events", + """ + WITH matching_delayed_events AS ( + SELECT * FROM delayed_events + WHERE room_id = ? AND event_type = ? AND state_key = ? + AND user_localpart <> ? + AND is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + ) + INSERT INTO finalised_delayed_events ( + delay_id, + user_localpart, + error, + finalised_ts + ) + SELECT + delay_id, + user_localpart, + ?, + ? + FROM matching_delayed_events + """, + room_id, + event_type, + state_key, + _generate_cancelled_by_state_update_json(), + finalised_ts, ) async def unprocess_delayed_events(self) -> None: """ Unmark all delayed events for processing. """ - await self.db_pool.simple_update( - table="delayed_events", - keyvalues={"is_processed": True}, - updatevalues={"is_processed": False}, - desc="unprocess_delayed_events", + await self.db_pool.execute( + "unprocess_delayed_events", + """ + UPDATE delayed_events SET is_processed = FALSE + WHERE is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + """, ) async def get_next_delayed_event_send_ts(self) -> Optional[Timestamp]: @@ -544,14 +907,18 @@ async def get_next_delayed_event_send_ts(self) -> Optional[Timestamp]: def _get_next_delayed_event_send_ts_txn( self, txn: LoggingTransaction ) -> Optional[Timestamp]: - result = self.db_pool.simple_select_one_onecol_txn( - txn, - table="delayed_events", - keyvalues={"is_processed": False}, - retcol="MIN(send_ts)", - allow_none=True, + txn.execute( + """ + SELECT MIN(send_ts) FROM delayed_events + WHERE NOT is_processed + AND (delay_id, user_localpart) NOT IN ( + SELECT delay_id, user_localpart + FROM finalised_delayed_events + ) + """ ) - return Timestamp(result) if result is not None else None + resp = txn.fetchone() + return Timestamp(resp[0]) if resp is not None else None def _generate_delay_id() -> DelayID: @@ -563,3 +930,15 @@ def _generate_delay_id() -> DelayID: # the same ID to exist for multiple users. return DelayID(f"syd_{stringutils.random_string(20)}") + + +def _generate_cancelled_by_state_update_json() -> str: + return json_encoder.encode( + cs_error( + "The delayed event did not get sent because a different user updated the same state event. " + + "So the scheduled event might change it in an undesired way.", + **{ + "org.matrix.msc4140.errcode": "M_CANCELLED_BY_STATE_UPDATE", + }, + ) + ) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index 3c3b13437ef..1171d9ed28d 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -19,7 +19,7 @@ # # -SCHEMA_VERSION = 92 # remember to update the list below when updating +SCHEMA_VERSION = 93 # remember to update the list below when updating """Represents the expectations made by the codebase about the database schema This should be incremented whenever the codebase changes its requirements on the @@ -168,6 +168,10 @@ Changes in SCHEMA_VERSION = 92 - Cleaned up a trigger that was added in #18260 and then reverted. + +Changes in SCHEMA_VERSION = 93 + - MSC4140: Add `finalised_delayed_events` table that keeps track of delayed events + that have been sent, cancelled, or failed to be sent due to an error. """ diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql new file mode 100644 index 00000000000..97826b13830 --- /dev/null +++ b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql @@ -0,0 +1,25 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2025 New Vector, 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: +-- . + +CREATE TABLE finalised_delayed_events ( + delay_id TEXT NOT NULL, + user_localpart TEXT NOT NULL, + error bytea, + event_id TEXT, + finalised_ts BIGINT NOT NULL, + PRIMARY KEY (user_localpart, delay_id), + FOREIGN KEY (user_localpart, delay_id) + REFERENCES delayed_events (user_localpart, delay_id) ON DELETE CASCADE +); + +CREATE INDEX finalised_delayed_events_finalised_ts ON finalised_delayed_events (finalised_ts); diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 221a4902f20..063cca54270 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -15,7 +15,7 @@ """Tests REST events for /delayed_events paths.""" from http import HTTPStatus -from typing import List +from typing import List, Tuple from parameterized import parameterized @@ -90,7 +90,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: ) def test_delayed_events_empty_on_startup(self) -> None: - self.assertListEqual([], self._get_delayed_events()) + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertListEqual([], finalised) def test_delayed_state_events_are_sent_on_timeout(self) -> None: state_key = "to_send_on_timeout" @@ -106,9 +108,13 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) - content = self._get_delayed_event_content(events[0]) + scheduled, finalised = self._get_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + self.assertListEqual([], finalised) + + scheduled_event = scheduled[0] + content = self._get_delayed_event_content(scheduled_event) + self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -119,7 +125,17 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: ) self.reactor.advance(1) - self.assertListEqual([], self._get_delayed_events()) + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertEqual(1, len(finalised), finalised) + + finalised_event_info = finalised[0] + self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) + self.assertEqual("send", finalised_event_info["outcome"]) + self.assertEqual("delay", finalised_event_info["reason"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) + content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -225,9 +241,11 @@ def test_cancel_delayed_state_event(self) -> None: self.assertIsNotNone(delay_id) self.reactor.advance(1) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) - content = self._get_delayed_event_content(events[0]) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + + scheduled_event = scheduled[0] + content = self._get_delayed_event_content(scheduled_event) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -244,7 +262,17 @@ def test_cancel_delayed_state_event(self) -> None: self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - self.assertListEqual([], self._get_delayed_events()) + + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertEqual(1, len(finalised), finalised) + + finalised_event_info = finalised[0] + self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) + self.assertEqual("cancel", finalised_event_info["outcome"]) + self.assertEqual("action", finalised_event_info["reason"]) + self.assertNotIn("error", finalised_event_info) + self.assertNotIn("event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( @@ -318,9 +346,11 @@ def test_send_delayed_state_event(self) -> None: self.assertIsNotNone(delay_id) self.reactor.advance(1) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) - content = self._get_delayed_event_content(events[0]) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + + scheduled_event = scheduled[0] + content = self._get_delayed_event_content(scheduled_event) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -337,7 +367,18 @@ def test_send_delayed_state_event(self) -> None: self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - self.assertListEqual([], self._get_delayed_events()) + + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertEqual(1, len(finalised), finalised) + + finalised_event_info = finalised[0] + self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) + self.assertEqual("send", finalised_event_info["outcome"]) + self.assertEqual("action", finalised_event_info["reason"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) + content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -407,9 +448,11 @@ def test_restart_delayed_state_event(self) -> None: self.assertIsNotNone(delay_id) self.reactor.advance(1) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) - content = self._get_delayed_event_content(events[0]) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + + scheduled_event = scheduled[0] + content = self._get_delayed_event_content(scheduled_event) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -428,9 +471,12 @@ def test_restart_delayed_state_event(self) -> None: self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.reactor.advance(1) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) - content = self._get_delayed_event_content(events[0]) + + scheduled, finalised = self._get_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + self.assertListEqual([], finalised) + + content = self._get_delayed_event_content(scheduled[0]) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -441,7 +487,16 @@ def test_restart_delayed_state_event(self) -> None: ) self.reactor.advance(1) - self.assertListEqual([], self._get_delayed_events()) + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertEqual(1, len(finalised), finalised) + + finalised_event_info = finalised[0] + self.assertEqual("send", finalised_event_info["outcome"]) + self.assertEqual("delay", finalised_event_info["reason"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) + content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -511,8 +566,8 @@ def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) self.helper.send_state( self.room_id, @@ -523,8 +578,8 @@ def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self.user1_access_token, state_key=state_key, ) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) self.reactor.advance(1) content = self.helper.get_state( @@ -550,8 +605,9 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - events = self._get_delayed_events() - self.assertEqual(1, len(events), events) + scheduled = self._get_scheduled_delayed_events() + self.assertEqual(1, len(scheduled), scheduled) + scheduled_event = scheduled[0] setter_expected = "other_user" self.helper.send_state( @@ -563,7 +619,23 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( self.user2_access_token, state_key=state_key, ) - self.assertListEqual([], self._get_delayed_events()) + + scheduled, finalised = self._get_delayed_events() + self.assertListEqual([], scheduled) + self.assertEqual(1, len(finalised), finalised) + + finalised_event_info = finalised[0] + self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) + self.assertEqual("cancel", finalised_event_info["outcome"]) + self.assertEqual("error", finalised_event_info["reason"]) + self.assert_dict( + { + "errcode": "M_UNKNOWN", + "org.matrix.msc4140.errcode": "M_CANCELLED_BY_STATE_UPDATE", + }, + finalised_event_info["error"], + ) + self.assertNotIn("event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( @@ -574,7 +646,7 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( ) self.assertEqual(setter_expected, content.get(setter_key), content) - def _get_delayed_events(self) -> List[JsonDict]: + def _get_delayed_events(self) -> Tuple[List[JsonDict], List[JsonDict]]: channel = self.make_request( "GET", PATH_PREFIX, @@ -582,13 +654,56 @@ def _get_delayed_events(self) -> List[JsonDict]: ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - key = "delayed_events" - self.assertIn(key, channel.json_body) + scheduled = self._validate_scheduled_delayed_events(channel.json_body) + finalised = self._validate_finalised_delayed_events(channel.json_body) + + return scheduled, finalised + + def _get_scheduled_delayed_events(self) -> List[JsonDict]: + channel = self.make_request( + "GET", + PATH_PREFIX + "?status=scheduled", + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + + scheduled = self._validate_scheduled_delayed_events(channel.json_body) + + return scheduled + + def _get_finalised_delayed_events(self) -> List[JsonDict]: + channel = self.make_request( + "GET", + PATH_PREFIX + "?status=finalised", + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + + finalised = self._validate_finalised_delayed_events(channel.json_body) + + return finalised + + def _validate_scheduled_delayed_events(self, json_body: JsonDict) -> List[JsonDict]: + key = "scheduled" + self.assertIn(key, json_body) + + scheduled = json_body[key] + self.assertIsInstance(scheduled, list) + + return scheduled + + def _validate_finalised_delayed_events(self, json_body: JsonDict) -> List[JsonDict]: + key = "finalised" + self.assertIn(key, json_body) + + finalised = json_body[key] + self.assertIsInstance(finalised, list) - events = channel.json_body[key] - self.assertIsInstance(events, list) + for item in finalised: + for key in ("delayed_event", "outcome", "reason", "origin_server_ts"): + self.assertIsNotNone(item.get(key)) - return events + return finalised def _get_delayed_event_content(self, event: JsonDict) -> JsonDict: key = "content" diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index feae5f77cdf..50098fdc370 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -2402,6 +2402,8 @@ def test_room_message_filter_wildcard(self) -> None: class RoomDelayedEventTestCase(RoomBase): """Tests delayed events.""" + servlets = RoomBase.servlets + [admin.register_servlets] + user_id = "@sid1:red" def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -2533,6 +2535,54 @@ def test_add_delayed_event_ratelimit(self) -> None: channel = self.make_request(*args) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + @unittest.override_config( + { + "max_event_delay_duration": "24h", + "experimental_features": { + "msc4140_max_delayed_events_per_user": 1, + }, + } + ) + def test_add_delayed_event_num_limit(self) -> None: + """Test that users may not have too many scheduled delayed events at once.""" + user2_user_id = self.register_user("user2", "pass") + + room_id = self.helper.create_room_as(self.user_id, is_public=True) + self.helper.join(room_id, user2_user_id) + + txn_id = 0 + + def add_delayed_event( + expect_success: bool, + user_id: str = self.user_id, + ) -> None: + nonlocal room_id, txn_id + self.helper.auth_user_id = user_id + txn_id += 1 + channel = self.make_request( + "PUT", + ( + "rooms/%s/send/m.room.message/%s?org.matrix.msc4140.delay=2000" + % (room_id, txn_id) + ).encode("ascii"), + {"body": "test", "msgtype": "m.text"}, + ) + if expect_success: + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + else: + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, channel.result) + self.assertEqual( + "M_MAX_DELAY_UNSUPPORTED", + channel.json_body.get("org.matrix.msc4140.errcode"), + channel.json_body, + ) + + add_delayed_event(True) + add_delayed_event(False) + add_delayed_event(True, user2_user_id) + self.reactor.advance(2) + add_delayed_event(True) + class RoomSearchTestCase(unittest.HomeserverTestCase): servlets = [ From bc0e1a334b19243891d407226166b5ec535a9ded Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 10 Oct 2025 04:03:08 -0400 Subject: [PATCH 02/47] Add changelog --- changelog.d/19038.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19038.feature diff --git a/changelog.d/19038.feature b/changelog.d/19038.feature new file mode 100644 index 00000000000..13a82e7c061 --- /dev/null +++ b/changelog.d/19038.feature @@ -0,0 +1 @@ +Add more support for MSC4140, namely the ability to inspect sent, cancelled, or failed delayed events, aka "finalised" delayed events. From 4a7e78ca68c454ecd7afada7ebe6f2f7dd8e6745 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 10 Oct 2025 11:51:33 -0400 Subject: [PATCH 03/47] Work around Postgres-only error --- synapse/storage/databases/main/delayed_events.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index b5fc0edc851..ed500aa2e0b 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -800,9 +800,7 @@ async def finalise_processed_delayed_event( event_id = None send_error = result_or_error - await self.db_pool.execute( - "finalise_processed_delayed_event", - """ + sql = """ WITH matching_delayed_events AS ( SELECT * FROM delayed_events WHERE delay_id = ? AND user_localpart = ? @@ -826,10 +824,16 @@ async def finalise_processed_delayed_event( ?, ? FROM matching_delayed_events - """, + """ + # TODO: Find why this is needed. An error gets thrown othersie + if isinstance(self.database_engine, PostgresEngine): + sql += "RETURNING *" + await self.db_pool.execute( + "finalise_processed_delayed_event", + sql, delay_id, user_localpart, - json_encoder.encode(send_error) if send_error else None, + json_encoder.encode(send_error) if send_error is not None else None, event_id, finalised_ts, ) From 99f8501b92d49865470c11b652b44742b269f6fe Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 31 Oct 2025 10:56:01 -0400 Subject: [PATCH 04/47] Rename experimental config per suggestion --- synapse/config/experimental.py | 4 ++-- synapse/handlers/delayed_events.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 2e077f8afc3..98e08d5f9fb 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -567,8 +567,8 @@ def read_config( ) # MSC4140: How many finalised delayed events to keep per user before deleting them. - self.msc4140_finalised_retention_limit = experimental.get( - "msc4140_finalised_retention_limit", 1000 + self.msc4140_finalised_per_user_retention_limit = experimental.get( + "msc4140_finalised_per_user_retention_limit", 1000 ) # MSC4143: Matrix RTC Transport using Livekit Backend diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 3a3c6ae22a1..33b2a782802 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -334,7 +334,7 @@ async def _prune_finalised_events(self) -> None: await self._store.prune_finalised_delayed_events( self._get_current_ts(), self.hs.config.experimental.msc4140_finalised_retention_period, - self.hs.config.experimental.msc4140_finalised_retention_limit, + self.hs.config.experimental.msc4140_finalised_per_user_retention_limit, ) async def add( @@ -602,7 +602,7 @@ async def get_delayed_events_for_user( delay_ids, self._get_current_ts(), self.hs.config.experimental.msc4140_finalised_retention_period, - self.hs.config.experimental.msc4140_finalised_retention_limit, + self.hs.config.experimental.msc4140_finalised_per_user_retention_limit, ) return ret From 96ed87a12356beba59bb671592a948042f8f378a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 31 Oct 2025 11:04:42 -0400 Subject: [PATCH 05/47] Add description comment to new table --- .../schema/main/delta/93/01_add_finalised_delayed_events.sql | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql index 97826b13830..0801443b7f5 100644 --- a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql @@ -11,6 +11,7 @@ -- See the GNU Affero General Public License for more details: -- . +-- Stores delayed events that have either been sent, cancelled, or not sent due to an error (MSC4140) CREATE TABLE finalised_delayed_events ( delay_id TEXT NOT NULL, user_localpart TEXT NOT NULL, From 2ec54e438aa22e616db46eb5300642ed83b18ea8 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 31 Oct 2025 22:20:14 -0400 Subject: [PATCH 06/47] Merge finalised_delayed_events into delayed_events Instead of storing details on finalised delayed events in a new, standalone table that must be joined on the base table of delayed events, add columns to the base table to track those details. --- .../storage/databases/main/delayed_events.py | 311 +++++++----------- .../93/01_add_finalised_delayed_events.sql | 17 +- tests/rest/client/test_delayed_events.py | 22 +- 3 files changed, 141 insertions(+), 209 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index ed500aa2e0b..29a3ed5a2dd 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -118,10 +118,7 @@ def add_delayed_event_txn(txn: LoggingTransaction) -> Timestamp: """ SELECT COUNT(*) FROM delayed_events WHERE user_localpart = ? - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) + AND finalised_ts IS NULL """, (user_localpart,), ) @@ -196,10 +193,7 @@ def restart_delayed_event_txn( SET send_ts = ? + delay WHERE delay_id = ? AND user_localpart = ? AND NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) + AND finalised_ts IS NULL """, ( current_ts, @@ -208,18 +202,24 @@ def restart_delayed_event_txn( ), ) if txn.rowcount == 0: - sent = self.db_pool.simple_select_one_onecol_txn( - txn, - "finalised_delayed_events", - keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, - retcol="event_id IS NOT NULL", - allow_none=True, + txn.execute( + """ + SELECT finalised_event_id IS NOT NULL + FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + AND finalised_ts IS NOT NULL + """, + ( + delay_id, + user_localpart, + ), ) - if sent is None: + row = txn.fetchone() + if not row: raise NotFoundError("Delayed event not found") raise SynapseError( HTTPStatus.CONFLICT, - f"Delayed event has already been {'sent' if sent else 'cancelled'}", + f"Delayed event has already been {'sent' if row[0] else 'cancelled'}", ) next_send_ts = self._get_next_delayed_event_send_ts_txn(txn) @@ -238,10 +238,7 @@ async def has_scheduled_delayed_events(self) -> bool: """ SELECT 1 WHERE EXISTS ( SELECT * FROM delayed_events - WHERE (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) + WHERE finalised_ts IS NULL ) """, ) @@ -258,12 +255,14 @@ def prune_finalised_delayed_events(txn: LoggingTransaction) -> None: txn, current_ts, retention_period ) - for user_localpart in self.db_pool.simple_select_onecol_txn( - txn, - "finalised_delayed_events", - keyvalues={}, - retcol="DISTINCT(user_localpart)", - ): + txn.execute( + """ + SELECT DISTINCT(user_localpart) + FROM delayed_events + WHERE finalised_ts IS NOT NULL + """ + ) + for [user_localpart] in txn.fetchall(): self._prune_excess_finalised_delayed_events_for_user( txn, user_localpart, retention_limit ) @@ -282,11 +281,7 @@ def _prune_expired_finalised_delayed_events( txn.execute( """ DELETE FROM delayed_events - WHERE (delay_id, user_localpart) IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - WHERE ? - finalised_ts > ? - ) + WHERE ? - finalised_ts > ? """, ( current_ts, @@ -301,20 +296,21 @@ def _prune_excess_finalised_delayed_events_for_user( Delete the oldest finalised delayed events for the given user, such that no more of them remain than the given retention limit. """ - num_existing = self.db_pool.simple_select_one_onecol_txn( - txn, - "finalised_delayed_events", - keyvalues={"user_localpart": user_localpart}, - retcol="COUNT(*)", + txn.execute( + """ + SELECT COUNT(*) FROM delayed_events + WHERE user_localpart = ? + AND finalised_ts IS NOT NULL + """, + (user_localpart,), ) + num_existing: int = txn.fetchall()[0][0] if num_existing > retention_limit: txn.execute( """ DELETE FROM delayed_events - WHERE (delay_id, user_localpart) IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - WHERE user_localpart = ? + WHERE user_localpart = ? + AND finalised_ts IS NOT NULL ORDER BY finalised_ts LIMIT ? ) @@ -332,11 +328,7 @@ async def get_scheduled_delayed_events_for_user( ) -> List[JsonDict]: """Returns all scheduled delayed events for the given user.""" # TODO: Support Pagination stream API ("next_batch" field) - sql_where = """WHERE user_localpart = ? - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - )""" + sql_where = "WHERE user_localpart = ? AND finalised_ts IS NULL" sql_args = [user_localpart] if delay_ids: delay_id_clause_sql, delay_id_clause_args = make_in_list_sql_clause( @@ -396,7 +388,7 @@ def get_finalised_delayed_events_for_user( txn, user_localpart, retention_limit ) - sql_where = "WHERE user_localpart = ?" + sql_where = "WHERE user_localpart = ? AND finalised_ts IS NOT NULL" sql_args = [user_localpart] if delay_ids: delay_id_clause_sql, delay_id_clause_args = make_in_list_sql_clause( @@ -414,12 +406,10 @@ def get_finalised_delayed_events_for_user( delay, send_ts, content, - error, - event_id, + finalised_error, + finalised_event_id, finalised_ts FROM delayed_events - JOIN finalised_delayed_events - USING (delay_id, user_localpart) {sql_where} ORDER BY finalised_ts """, @@ -442,14 +432,22 @@ def get_finalised_delayed_events_for_user( }, "outcome": "cancel" if row[8] is None else "send", "reason": ( - "error" + "finalised_error" if row[7] is not None else "action" if row[9] < row[5] else "delay" ), - **({"error": db_to_json(row[7])} if row[7] is not None else {}), - **({"event_id": str(row[8])} if row[8] is not None else {}), + **( + {"finalised_error": db_to_json(row[7])} + if row[7] is not None + else {} + ), + **( + {"finalised_event_id": str(row[8])} + if row[8] is not None + else {} + ), "origin_server_ts": Timestamp(row[9]), } for row in txn @@ -496,10 +494,8 @@ def process_timeout_delayed_events_txn( sql_update = "UPDATE delayed_events SET is_processed = TRUE" sql_where = """WHERE send_ts <= ? AND NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - )""" + AND finalised_ts IS NULL + """ sql_args = (current_ts,) sql_order = "ORDER BY send_ts" if isinstance(self.database_engine, PostgresEngine): @@ -592,10 +588,8 @@ def process_target_delayed_event_txn( sql_update = "UPDATE delayed_events SET is_processed = TRUE" sql_where = """WHERE delay_id = ? AND user_localpart = ? AND NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - )""" + AND finalised_ts IS NULL + """ sql_args = (delay_id, user_localpart) txn.execute( ( @@ -606,17 +600,23 @@ def process_target_delayed_event_txn( sql_args, ) row = txn.fetchone() - if row is None: - sent = self.db_pool.simple_select_one_onecol_txn( - txn, - "finalised_delayed_events", - keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, - retcol="event_id IS NOT NULL", - allow_none=True, + if not row: + txn.execute( + """ + SELECT finalised_event_id IS NOT NULL + FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + AND finalised_ts IS NOT NULL + """, + ( + delay_id, + user_localpart, + ), ) - if sent is None: + row = txn.fetchone() + if not row: raise NotFoundError("Delayed event not found") - elif not sent: + elif not row[0]: raise SynapseError( HTTPStatus.CONFLICT, "Delayed event has already been cancelled", @@ -667,43 +667,35 @@ def cancel_delayed_event_txn( ) -> Optional[Timestamp]: txn.execute( """ - WITH matching_delayed_events AS ( - SELECT * FROM delayed_events - WHERE delay_id = ? AND user_localpart = ? - AND NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) - ) - INSERT INTO finalised_delayed_events ( - delay_id, - user_localpart, - finalised_ts - ) - SELECT - delay_id, - user_localpart, - ? - FROM matching_delayed_events - RETURNING 1 + UPDATE delayed_events + SET finalised_ts = ? + WHERE delay_id = ? AND user_localpart = ? + AND NOT is_processed + AND finalised_ts IS NULL """, ( + finalised_ts, delay_id, user_localpart, - finalised_ts, ), ) - if not txn.fetchall(): - sent = self.db_pool.simple_select_one_onecol_txn( - txn, - "finalised_delayed_events", - keyvalues={"delay_id": delay_id, "user_localpart": user_localpart}, - retcol="event_id IS NOT NULL", + if txn.rowcount == 0: + txn.execute( + """ + SELECT finalised_event_id IS NOT NULL + FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + AND finalised_ts IS NOT NULL + """, + ( + delay_id, + user_localpart, + ), ) - if sent is None: + row = txn.fetchone() + if not row: raise NotFoundError("Delayed event not found") - elif sent: + elif row[0]: raise SynapseError( HTTPStatus.CONFLICT, "Delayed event has already been sent", @@ -742,36 +734,22 @@ def cancel_delayed_state_events_txn( ) -> Optional[Timestamp]: txn.execute( """ - WITH matching_delayed_events AS ( - SELECT * FROM delayed_events - WHERE room_id = ? AND event_type = ? AND state_key = ? - AND user_localpart <> ? - AND NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) - ) - INSERT INTO finalised_delayed_events ( - delay_id, - user_localpart, - error, - finalised_ts - ) - SELECT - delay_id, - user_localpart, - ?, - ? - FROM matching_delayed_events + UPDATE delayed_events + SET + finalised_error = ?, + finalised_ts = ? + WHERE room_id = ? AND event_type = ? AND state_key = ? + AND user_localpart <> ? + AND NOT is_processed + AND finalised_ts IS NULL """, ( + _generate_cancelled_by_state_update_json(), + finalised_ts, room_id, event_type, state_key, not_from_localpart, - _generate_cancelled_by_state_update_json(), - finalised_ts, ), ) return self._get_next_delayed_event_send_ts_txn(txn) @@ -800,42 +778,23 @@ async def finalise_processed_delayed_event( event_id = None send_error = result_or_error - sql = """ - WITH matching_delayed_events AS ( - SELECT * FROM delayed_events - WHERE delay_id = ? AND user_localpart = ? - AND is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) - ) - INSERT INTO finalised_delayed_events ( - delay_id, - user_localpart, - error, - event_id, - finalised_ts - ) - SELECT - delay_id, - user_localpart, - ?, - ?, - ? - FROM matching_delayed_events - """ - # TODO: Find why this is needed. An error gets thrown othersie - if isinstance(self.database_engine, PostgresEngine): - sql += "RETURNING *" await self.db_pool.execute( "finalise_processed_delayed_event", - sql, - delay_id, - user_localpart, + """ + UPDATE delayed_events + SET + finalised_error = ?, + finalised_event_id = ?, + finalised_ts = ? + WHERE delay_id = ? AND user_localpart = ? + AND is_processed + AND finalised_ts IS NULL + """, json_encoder.encode(send_error) if send_error is not None else None, event_id, finalised_ts, + delay_id, + user_localpart, ) async def finalise_processed_delayed_state_events( @@ -852,34 +811,20 @@ async def finalise_processed_delayed_state_events( await self.db_pool.execute( "finalise_processed_delayed_state_events", """ - WITH matching_delayed_events AS ( - SELECT * FROM delayed_events - WHERE room_id = ? AND event_type = ? AND state_key = ? - AND user_localpart <> ? - AND is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) - ) - INSERT INTO finalised_delayed_events ( - delay_id, - user_localpart, - error, - finalised_ts - ) - SELECT - delay_id, - user_localpart, - ?, - ? - FROM matching_delayed_events + UPDATE delayed_events + SET + finalised_error = ?, + finalised_ts = ? + WHERE room_id = ? AND event_type = ? AND state_key = ? + AND user_localpart <> ? + AND is_processed + AND finalised_ts IS NULL """, + _generate_cancelled_by_state_update_json(), + finalised_ts, room_id, event_type, state_key, - _generate_cancelled_by_state_update_json(), - finalised_ts, ) async def unprocess_delayed_events(self) -> None: @@ -891,10 +836,7 @@ async def unprocess_delayed_events(self) -> None: """ UPDATE delayed_events SET is_processed = FALSE WHERE is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) + AND finalised_ts IS NULL """, ) @@ -915,10 +857,7 @@ def _get_next_delayed_event_send_ts_txn( """ SELECT MIN(send_ts) FROM delayed_events WHERE NOT is_processed - AND (delay_id, user_localpart) NOT IN ( - SELECT delay_id, user_localpart - FROM finalised_delayed_events - ) + AND finalised_ts IS NULL """ ) resp = txn.fetchone() diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql index 0801443b7f5..2f2a604f450 100644 --- a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql @@ -11,16 +11,9 @@ -- See the GNU Affero General Public License for more details: -- . --- Stores delayed events that have either been sent, cancelled, or not sent due to an error (MSC4140) -CREATE TABLE finalised_delayed_events ( - delay_id TEXT NOT NULL, - user_localpart TEXT NOT NULL, - error bytea, - event_id TEXT, - finalised_ts BIGINT NOT NULL, - PRIMARY KEY (user_localpart, delay_id), - FOREIGN KEY (user_localpart, delay_id) - REFERENCES delayed_events (user_localpart, delay_id) ON DELETE CASCADE -); +-- Store when delayed events have either been sent, cancelled, or not sent due to an error (MSC4140) +ALTER TABLE delayed_events ADD COLUMN finalised_error bytea; +ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; +ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; -CREATE INDEX finalised_delayed_events_finalised_ts ON finalised_delayed_events (finalised_ts); +CREATE INDEX delayed_events_finalised_ts ON delayed_events (finalised_ts); diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 063cca54270..326619488c8 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -133,8 +133,8 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) + self.assertNotIn("finalised_error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["finalised_event_id"]) content = self.helper.get_state( self.room_id, @@ -271,8 +271,8 @@ def test_cancel_delayed_state_event(self) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("cancel", finalised_event_info["outcome"]) self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertNotIn("event_id", finalised_event_info) + self.assertNotIn("finalised_error", finalised_event_info) + self.assertNotIn("finalised_event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( @@ -376,8 +376,8 @@ def test_send_delayed_state_event(self) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) + self.assertNotIn("finalised_error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["finalised_event_id"]) content = self.helper.get_state( self.room_id, @@ -494,8 +494,8 @@ def test_restart_delayed_state_event(self) -> None: finalised_event_info = finalised[0] self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) + self.assertNotIn("finalised_error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["finalised_event_id"]) content = self.helper.get_state( self.room_id, @@ -627,15 +627,15 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( finalised_event_info = finalised[0] self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("cancel", finalised_event_info["outcome"]) - self.assertEqual("error", finalised_event_info["reason"]) + self.assertEqual("finalised_error", finalised_event_info["reason"]) self.assert_dict( { "errcode": "M_UNKNOWN", "org.matrix.msc4140.errcode": "M_CANCELLED_BY_STATE_UPDATE", }, - finalised_event_info["error"], + finalised_event_info["finalised_error"], ) - self.assertNotIn("event_id", finalised_event_info) + self.assertNotIn("finalised_event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( From 5cd80eda8bcf1683298f72f1dcae826f7717615d Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 3 Nov 2025 09:49:31 -0500 Subject: [PATCH 07/47] Add finalised_ts index in background This satisfies the check-schema-delta script. The index is present for speeding up ORDER BY on finalised_ts. --- .../storage/databases/main/delayed_events.py | 22 ++++++++++++++++++- .../93/01_add_finalised_delayed_events.sql | 3 ++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 9aaac6c3902..0f6360b167d 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -14,13 +14,15 @@ import logging from http import HTTPStatus -from typing import NewType, Optional, Union +from typing import TYPE_CHECKING, NewType, Optional, Union import attr from synapse.api.errors import NotFoundError, SynapseError, cs_error from synapse.storage._base import SQLBaseStore, db_to_json from synapse.storage.database import ( + DatabasePool, + LoggingDatabaseConnection, LoggingTransaction, make_in_list_sql_clause, ) @@ -29,6 +31,9 @@ from synapse.util import stringutils from synapse.util.json import json_encoder +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) @@ -59,6 +64,21 @@ class DelayedEventDetails(EventDetails): class DelayedEventsStore(SQLBaseStore): + def __init__( + self, + database: DatabasePool, + db_conn: LoggingDatabaseConnection, + hs: "HomeServer", + ): + super().__init__(database, db_conn, hs) + + self.db_pool.updates.register_background_index_update( + update_name="delayed_events_finalised_ts", + index_name="delayed_events_finalised_ts", + table="delayed_events", + columns=("finalised_ts",), + ) + async def get_delayed_events_stream_pos(self) -> int: """ Gets the stream position of the background process to watch for state events diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql index 2f2a604f450..a0b4933d37e 100644 --- a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql @@ -16,4 +16,5 @@ ALTER TABLE delayed_events ADD COLUMN finalised_error bytea; ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; -CREATE INDEX delayed_events_finalised_ts ON delayed_events (finalised_ts); +INSERT INTO background_updates (update_name, progress_json) VALUES + ('delayed_events_finalised_ts', '{}'); From 11c79c21d99eea1a819b2444dd969bb5c4e7211e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 3 Nov 2025 11:27:34 -0500 Subject: [PATCH 08/47] Include DelayedEventsStore in migrated DB stores This should also fix the portdb script --- synapse/_scripts/synapse_port_db.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index e83c0de5a47..3dcb0c28cfa 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -58,6 +58,7 @@ from synapse.storage.databases.main import FilteringWorkerStore from synapse.storage.databases.main.account_data import AccountDataWorkerStore from synapse.storage.databases.main.client_ips import ClientIpBackgroundUpdateStore +from synapse.storage.databases.main.delayed_events import DelayedEventsStore from synapse.storage.databases.main.deviceinbox import DeviceInboxBackgroundUpdateStore from synapse.storage.databases.main.devices import DeviceBackgroundUpdateStore from synapse.storage.databases.main.e2e_room_keys import EndToEndRoomKeyBackgroundStore @@ -272,6 +273,7 @@ class Store( RelationsWorkerStore, EventFederationWorkerStore, SlidingSyncStore, + DelayedEventsStore, ): def execute(self, f: Callable[..., R], *args: Any, **kwargs: Any) -> Awaitable[R]: return self.db_pool.runInteraction(f.__name__, f, *args, **kwargs) From d2aba30467eafb0213e19faa332bd5337dbae98e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 3 Nov 2025 15:46:49 -0500 Subject: [PATCH 09/47] Fix error code string --- synapse/storage/databases/main/delayed_events.py | 2 +- tests/rest/client/test_rooms.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 0f6360b167d..240e73b6fc9 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -148,7 +148,7 @@ def add_delayed_event_txn(txn: LoggingTransaction) -> Timestamp: HTTPStatus.BAD_REQUEST, "The maximum number of delayed events has been reached.", additional_fields={ - "org.matrix.msc4140.errcode": "M_MAX_DELAY_UNSUPPORTED", + "org.matrix.msc4140.errcode": "M_MAX_DELAYED_EVENTS_EXCEEDED", }, ) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 11a745646d5..1fe4a39b368 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -2572,7 +2572,7 @@ def add_delayed_event( else: self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, channel.result) self.assertEqual( - "M_MAX_DELAY_UNSUPPORTED", + "M_MAX_DELAYED_EVENTS_EXCEEDED", channel.json_body.get("org.matrix.msc4140.errcode"), channel.json_body, ) From e5bef013431cf467b47725ca5cbe3daba761f425 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 3 Nov 2025 16:35:14 -0500 Subject: [PATCH 10/47] Run UPDATE in transaction, and check rowcount --- .../storage/databases/main/delayed_events.py | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 240e73b6fc9..c64d415983e 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -18,7 +18,7 @@ import attr -from synapse.api.errors import NotFoundError, SynapseError, cs_error +from synapse.api.errors import NotFoundError, StoreError, SynapseError, cs_error from synapse.storage._base import SQLBaseStore, db_to_json from synapse.storage.database import ( DatabasePool, @@ -791,23 +791,36 @@ async def finalise_processed_delayed_event( event_id = None send_error = result_or_error - await self.db_pool.execute( + def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: + table = "delayed_events" + txn.execute( + f""" + UPDATE {table} + SET + finalised_error = ?, + finalised_event_id = ?, + finalised_ts = ? + WHERE delay_id = ? AND user_localpart = ? + AND is_processed + AND finalised_ts IS NULL + """, + ( + json_encoder.encode(send_error) if send_error is not None else None, + event_id, + finalised_ts, + delay_id, + user_localpart, + ), + ) + rowcount = txn.rowcount + if rowcount == 0: + raise StoreError(404, "No row found (%s)" % (table,)) + if rowcount > 1: + raise StoreError(500, "More than one row matched (%s)" % (table,)) + + await self.db_pool.runInteraction( "finalise_processed_delayed_event", - """ - UPDATE delayed_events - SET - finalised_error = ?, - finalised_event_id = ?, - finalised_ts = ? - WHERE delay_id = ? AND user_localpart = ? - AND is_processed - AND finalised_ts IS NULL - """, - json_encoder.encode(send_error) if send_error is not None else None, - event_id, - finalised_ts, - delay_id, - user_localpart, + finalise_processed_delayed_event_txn, ) async def finalise_processed_delayed_state_events( From 9a345cee0524c0d7b8e598ec61bd14b125b647a7 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 7 Nov 2025 00:09:23 -0500 Subject: [PATCH 11/47] Don't fetch after UPDATE queries Using DatabasePool.execute always calls fetchall on the transaction, which fails in Postgres if there are no results to fetch. So, use DatabasePool.runInteraction for UPDATE queries that are expected to sometimes make no updates. --- .../storage/databases/main/delayed_events.py | 58 ++++++++++++------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 8bcf9b5f3fc..f0a2aeaf9b8 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -834,36 +834,50 @@ async def finalise_processed_delayed_state_events( """ Finalise the matching delayed state events that have been marked as processed. """ - await self.db_pool.execute( + + def finalise_processed_delayed_state_events(txn: LoggingTransaction) -> None: + txn.execute( + """ + UPDATE delayed_events + SET + finalised_error = ?, + finalised_ts = ? + WHERE room_id = ? AND event_type = ? AND state_key = ? + AND user_localpart <> ? + AND is_processed + AND finalised_ts IS NULL + """, + ( + _generate_cancelled_by_state_update_json(), + finalised_ts, + room_id, + event_type, + state_key, + ), + ) + + await self.db_pool.runInteraction( "finalise_processed_delayed_state_events", - """ - UPDATE delayed_events - SET - finalised_error = ?, - finalised_ts = ? - WHERE room_id = ? AND event_type = ? AND state_key = ? - AND user_localpart <> ? - AND is_processed - AND finalised_ts IS NULL - """, - _generate_cancelled_by_state_update_json(), - finalised_ts, - room_id, - event_type, - state_key, + finalise_processed_delayed_state_events, ) async def unprocess_delayed_events(self) -> None: """ Unmark all delayed events for processing. """ - await self.db_pool.execute( + + def unprocess_delayed_events(txn: LoggingTransaction): + txn.execute( + """ + UPDATE delayed_events SET is_processed = FALSE + WHERE is_processed + AND finalised_ts IS NULL + """ + ) + + await self.db_pool.runInteraction( "unprocess_delayed_events", - """ - UPDATE delayed_events SET is_processed = FALSE - WHERE is_processed - AND finalised_ts IS NULL - """, + unprocess_delayed_events, ) async def get_next_delayed_event_send_ts(self) -> Timestamp | None: From 98fa5aaa7ae85c354a95336d45057219027b7fd5 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 7 Nov 2025 00:25:26 -0500 Subject: [PATCH 12/47] Add missing return type annotation --- synapse/storage/databases/main/delayed_events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index f0a2aeaf9b8..87a46d3693c 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -866,7 +866,7 @@ async def unprocess_delayed_events(self) -> None: Unmark all delayed events for processing. """ - def unprocess_delayed_events(txn: LoggingTransaction): + def unprocess_delayed_events(txn: LoggingTransaction) -> None: txn.execute( """ UPDATE delayed_events SET is_processed = FALSE From 25ca333d0c6ad6c143f3f5cf08e9760d8862b385 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 7 Nov 2025 00:26:19 -0500 Subject: [PATCH 13/47] Update schema change comment --- synapse/storage/schema/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index 1171d9ed28d..6c52aae2443 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -170,7 +170,7 @@ - Cleaned up a trigger that was added in #18260 and then reverted. Changes in SCHEMA_VERSION = 93 - - MSC4140: Add `finalised_delayed_events` table that keeps track of delayed events + - MSC4140: Add columns to the `delayed_events` table to keep track of delayed events that have been sent, cancelled, or failed to be sent due to an error. """ From e0c412de86de93d04bf32109536e4666a7fbbe37 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 11 Nov 2025 23:13:40 -0500 Subject: [PATCH 14/47] Remove mistaken WHERE condition --- synapse/storage/databases/main/delayed_events.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 87a46d3693c..ba91df53755 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -843,7 +843,6 @@ def finalise_processed_delayed_state_events(txn: LoggingTransaction) -> None: finalised_error = ?, finalised_ts = ? WHERE room_id = ? AND event_type = ? AND state_key = ? - AND user_localpart <> ? AND is_processed AND finalised_ts IS NULL """, From 60549abd844ef14e2fa2b535e873cb6f18f6ec93 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 13 Nov 2025 10:49:52 -0500 Subject: [PATCH 15/47] Add ordering info to background update --- .../schema/main/delta/93/01_add_finalised_delayed_events.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql index a0b4933d37e..201a50aaaff 100644 --- a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql @@ -16,5 +16,5 @@ ALTER TABLE delayed_events ADD COLUMN finalised_error bytea; ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; -INSERT INTO background_updates (update_name, progress_json) VALUES - ('delayed_events_finalised_ts', '{}'); +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (9301, 'delayed_events_finalised_ts', '{}'); From 9444a71e76554b9cefe5414953a6f7dcddbafa55 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 13 Nov 2025 22:19:57 -0500 Subject: [PATCH 16/47] Bump schema change number/order The order doesn't really matter, but having two changes share the same ID/order feels inappropriate --- ...d_delayed_events.sql => 02_add_finalised_delayed_events.sql} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename synapse/storage/schema/main/delta/93/{01_add_finalised_delayed_events.sql => 02_add_finalised_delayed_events.sql} (94%) diff --git a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/93/02_add_finalised_delayed_events.sql similarity index 94% rename from synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql rename to synapse/storage/schema/main/delta/93/02_add_finalised_delayed_events.sql index 201a50aaaff..c7941277bd7 100644 --- a/synapse/storage/schema/main/delta/93/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/93/02_add_finalised_delayed_events.sql @@ -17,4 +17,4 @@ ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (9301, 'delayed_events_finalised_ts', '{}'); + (9302, 'delayed_events_finalised_ts', '{}'); From 4de1de6182517296c744d08dc917397154968286 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 13 Nov 2025 23:03:10 -0500 Subject: [PATCH 17/47] Fix mistake in DELETE query --- synapse/storage/databases/main/delayed_events.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 289d6af181d..58504ab1d7f 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -337,9 +337,8 @@ def _prune_excess_finalised_delayed_events_for_user( DELETE FROM delayed_events WHERE user_localpart = ? AND finalised_ts IS NOT NULL - ORDER BY finalised_ts - LIMIT ? - ) + ORDER BY finalised_ts + LIMIT ? """, ( user_localpart, From b36becc5edd98503bd857d140b8604073733214a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 14 Nov 2025 13:58:32 -0500 Subject: [PATCH 18/47] Return most recent finalised events first according to the MSC's proposal to sort them in decreasing order of finalise time --- synapse/storage/databases/main/delayed_events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 58504ab1d7f..7ab850117ec 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -436,7 +436,7 @@ def get_finalised_delayed_events_for_user( finalised_ts FROM delayed_events {sql_where} - ORDER BY finalised_ts + ORDER BY finalised_ts DESC """, sql_args, ) From c9eb4673d847a1ec4d19bf4e199c6d00d05d76ff Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 14 Nov 2025 14:24:52 -0500 Subject: [PATCH 19/47] Removed leading "finalised_" from object fields Properties of finalised delayed event objects should not begin with "finalised_". Only the DB columns do, which is an implementation detail --- .../storage/databases/main/delayed_events.py | 14 +++--------- tests/rest/client/test_delayed_events.py | 22 +++++++++---------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 7ab850117ec..87de21b7a86 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -457,22 +457,14 @@ def get_finalised_delayed_events_for_user( }, "outcome": "cancel" if row[8] is None else "send", "reason": ( - "finalised_error" + "error" if row[7] is not None else "action" if row[9] < row[5] else "delay" ), - **( - {"finalised_error": db_to_json(row[7])} - if row[7] is not None - else {} - ), - **( - {"finalised_event_id": str(row[8])} - if row[8] is not None - else {} - ), + **({"error": db_to_json(row[7])} if row[7] is not None else {}), + **({"event_id": str(row[8])} if row[8] is not None else {}), "origin_server_ts": Timestamp(row[9]), } for row in txn diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 59a52c53938..859c9f5da60 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -133,8 +133,8 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("finalised_error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["finalised_event_id"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) content = self.helper.get_state( self.room_id, @@ -270,8 +270,8 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("cancel", finalised_event_info["outcome"]) self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("finalised_error", finalised_event_info) - self.assertNotIn("finalised_event_id", finalised_event_info) + self.assertNotIn("error", finalised_event_info) + self.assertNotIn("event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( @@ -350,8 +350,8 @@ def test_send_delayed_state_event(self, action_in_path: bool) -> None: self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("finalised_error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["finalised_event_id"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) content = self.helper.get_state( self.room_id, @@ -443,8 +443,8 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: finalised_event_info = finalised[0] self.assertEqual("send", finalised_event_info["outcome"]) self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("finalised_error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["finalised_event_id"]) + self.assertNotIn("error", finalised_event_info) + self.assertIsNotNone(finalised_event_info["event_id"]) content = self.helper.get_state( self.room_id, @@ -559,15 +559,15 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( finalised_event_info = finalised[0] self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) self.assertEqual("cancel", finalised_event_info["outcome"]) - self.assertEqual("finalised_error", finalised_event_info["reason"]) + self.assertEqual("error", finalised_event_info["reason"]) self.assert_dict( { "errcode": "M_UNKNOWN", "org.matrix.msc4140.errcode": "M_CANCELLED_BY_STATE_UPDATE", }, - finalised_event_info["finalised_error"], + finalised_event_info["error"], ) - self.assertNotIn("finalised_event_id", finalised_event_info) + self.assertNotIn("event_id", finalised_event_info) self.reactor.advance(1) content = self.helper.get_state( From 7497cb5b5a3ddb853d8805ae9143b7e374ba7c7f Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 24 Nov 2025 10:17:42 -0500 Subject: [PATCH 20/47] Raise errors on manual send of delayed event When sending a delayed event directly via the associated endpoint, make the request respond with whatever error may happen during the send attempt, instead of always responding with success & putting the error in the finalised event. Also remove the transaction id, which was never used. --- synapse/handlers/delayed_events.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index f9670752880..a15be0e4baf 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -474,7 +474,7 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: self._schedule_next_at_or_none(next_send_ts) if event: - await self._send_event(event) + await self._send_event(event, False) async def _send_on_timeout(self) -> None: self._next_delayed_event_call = None @@ -499,7 +499,7 @@ async def _send_events(self, events: list[DelayedEventDetails]) -> None: state_info = None try: # TODO: send in background if message event or non-conflicting state event - finalised_ts = await self._send_event(event) + finalised_ts = await self._send_event(event, True) if state_info is not None: sent_state.add(state_info) except Exception: @@ -576,7 +576,7 @@ async def get_delayed_events_for_user( async def _send_event( self, event: DelayedEventDetails, - txn_id: str | None = None, + finalise_error: bool, ) -> Timestamp: user_id = UserID(event.user_localpart, self._config.server.server_name) user_id_str = user_id.to_string() @@ -620,7 +620,6 @@ async def _send_event( ) = await self._event_creation_handler.create_and_send_nonmember_event( requester, event_dict, - txn_id=txn_id, ) event_id = sent_event.event_id if event.origin_server_ts is None: @@ -628,10 +627,14 @@ async def _send_event( except ShadowBanError: event_id = generate_fake_event_id() send_error = None - except SynapseError as e: - send_error = e.error_dict(None) - except Exception: - send_error = cs_error("Internal server error") + except Exception as e: + if finalise_error: + if isinstance(e, SynapseError): + send_error = e.error_dict(None) + else: + send_error = cs_error("Internal server error") + else: + raise else: send_error = None finally: From 4187ec15d985cc9953a5c3c9445db1c576422ddf Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Wed, 16 Sep 2026 17:33:38 +0200 Subject: [PATCH 21/47] MSC4140: store finalised_error as TEXT Store the standard error response of a failed delayed event as TEXT rather than bytea: the store already reads it via `db_to_json` and writes it with `json_encoder.encode`, and bytea storage of JSON text is what #19360 had to migrate away from for `content`. --- .../schema/main/delta/94/11_add_finalised_delayed_events.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql index 242e5c2b12f..29df47987f6 100644 --- a/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql @@ -12,7 +12,7 @@ -- . -- Store when delayed events have either been sent, cancelled, or not sent due to an error (MSC4140) -ALTER TABLE delayed_events ADD COLUMN finalised_error bytea; +ALTER TABLE delayed_events ADD COLUMN finalised_error TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; From 7cace1db1dce4385077abf886c3e43fdae797f54 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 12:24:05 +0200 Subject: [PATCH 22/47] MSC4140: drop the status and delay_id filters on the delayed events list Remove the `?status=` and `?delay_id=` query parameters, the parsed-but-unused `from`, `_DelayedEventStatus`, the handler's `get_delayed_events_for_user` and the store's `get_finalised_delayed_events_for_user`, along with the `scheduled` and `finalised` response keys. `GET /delayed_events` returns `{"delayed_events": [...]}` with scheduled rows only, as on develop. Bulk lookup of finalised delayed events with filters and pagination is MSC4486's job; single-event lookup goes through `GET /delayed_events/{delay_id}`. This also removes the pruning that ran inside the list handler. --- synapse/handlers/delayed_events.py | 40 +--- synapse/rest/client/delayed_events.py | 39 +--- .../storage/databases/main/delayed_events.py | 71 ------- tests/rest/client/test_delayed_events.py | 191 ++++-------------- 4 files changed, 56 insertions(+), 285 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 49b719a2d59..3bb193019d8 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -33,6 +33,7 @@ from synapse.storage.databases.main.delayed_events import ( DelayedEventDetails, DelayedEventResponse, + DelayedEventResponseLegacyCompat, EventType, StateKey, Timestamp, @@ -585,40 +586,19 @@ async def get_for_user( requester.user.localpart, ) - async def get_delayed_events_for_user( - self, - requester: Requester, - get_scheduled: bool, - get_finalised: bool, - ) -> dict[str, list[JsonDict]]: + async def get_all_for_user( + self, requester: Requester + ) -> list[DelayedEventResponseLegacyCompat]: """ - Return the delayed events owned by the given user. - Scheduled delayed events include fields from earlier revisions of MSC4140 - for compatibility with clients that still expect them. - - Args: - requester: The user whose delayed events to get. - get_scheduled: Whether to look up scheduled delayed events. - get_finalised: Whether to look up finalised delayed events. + Return all scheduled delayed events owned by the given user. + Includes fields from earlier revisions of MSC4140 for + compatibility with clients that still expect them. """ # TODO: Remove legacy fields once stable await self._delayed_event_mgmt_ratelimiter.ratelimit(requester) - - # TODO: Support Pagination stream API - ret: dict[str, list[JsonDict]] = {} - if get_scheduled: - scheduled = await self._store.get_all_delayed_events_for_user( - requester.user.localpart - ) - ret["scheduled"] = [delayed_event.asdict() for delayed_event in scheduled] - if get_finalised: - ret["finalised"] = await self._store.get_finalised_delayed_events_for_user( - requester.user.localpart, - self._get_current_ts(), - self.hs.config.experimental.msc4140_finalised_retention_period, - self.hs.config.experimental.msc4140_finalised_per_user_retention_limit, - ) - return ret + return await self._store.get_all_delayed_events_for_user( + requester.user.localpart + ) async def _send_event( self, diff --git a/synapse/rest/client/delayed_events.py b/synapse/rest/client/delayed_events.py index 7200f7bf6ce..07ca07b7b93 100644 --- a/synapse/rest/client/delayed_events.py +++ b/synapse/rest/client/delayed_events.py @@ -21,12 +21,7 @@ from synapse.api.errors import Codes, SynapseError from synapse.http.server import HttpServer -from synapse.http.servlet import ( - RestServlet, - parse_json_object_from_request, - parse_string_from_args, - parse_strings_from_args, -) +from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest from synapse.rest.client._base import client_patterns from synapse.types import JsonDict @@ -43,11 +38,6 @@ class _UpdateDelayedEventAction(Enum): SEND = "send" -class _DelayedEventStatus(Enum): - SCHEDULED = "scheduled" - FINALISED = "finalised" - - class UpdateDelayedEventServlet(RestServlet): PATTERNS = client_patterns( r"/org\.matrix\.msc4140/delayed_events/(?P[^/]+)$", @@ -180,26 +170,13 @@ def __init__(self, hs: "HomeServer"): async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - - # twisted.web.server.Request.args is incorrectly defined as Optional[Any] - args: dict[bytes, list[bytes]] = request.args # type: ignore - statuses = parse_strings_from_args( - args, - "status", - allowed_values=tuple(s.value for s in _DelayedEventStatus), - ) - # TODO: Support Pagination stream API - _from_token = parse_string_from_args(args, "from") - - ret = await self.delayed_events_handler.get_delayed_events_for_user( - requester, - statuses is None or _DelayedEventStatus.SCHEDULED.value in statuses, - statuses is None or _DelayedEventStatus.FINALISED.value in statuses, - ) - # TODO: This is here for backwards compatibility. Remove eventually - if statuses is None: - ret["delayed_events"] = ret[_DelayedEventStatus.SCHEDULED.value] - return 200, ret + # TODO: Support Pagination stream API ("from" query parameter) + delayed_events = await self.delayed_events_handler.get_all_for_user(requester) + return 200, { + "delayed_events": [ + delayed_event.asdict() for delayed_event in delayed_events + ] + } def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 5907b3f8819..a0256463424 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -492,77 +492,6 @@ async def get_all_delayed_events_for_user( ) return [DelayedEventResponseLegacyCompat(*row) for row in rows] - async def get_finalised_delayed_events_for_user( - self, - user_localpart: str, - current_ts: Timestamp, - retention_period: int, - retention_limit: int, - ) -> list[JsonDict]: - """Returns all finalised delayed events for the given user.""" - # TODO: Support Pagination stream API ("next_batch" field) - - def get_finalised_delayed_events_for_user( - txn: LoggingTransaction, - ) -> list[JsonDict]: - # Clear up some space in the DB before returning any results. - self._prune_expired_finalised_delayed_events( - txn, current_ts, retention_period - ) - self._prune_excess_finalised_delayed_events_for_user( - txn, user_localpart, retention_limit - ) - - txn.execute( - """ - SELECT - delay_id, - room_id, - event_type, - state_key, - delay, - send_ts, - content, - finalised_error, - finalised_event_id, - finalised_ts - FROM delayed_events - WHERE user_localpart = ? AND finalised_ts IS NOT NULL - ORDER BY finalised_ts DESC - """, - (user_localpart,), - ) - return [ - { - "delayed_event": DelayedEventResponseLegacyCompat( - row[0], - row[1], - row[2], - row[3], - row[4], - row[5] - row[4], - row[6], - ).asdict(), - "outcome": "cancel" if row[8] is None else "send", - "reason": ( - "error" - if row[7] is not None - else "action" - if row[9] < row[5] - else "delay" - ), - **({"error": db_to_json(row[7])} if row[7] is not None else {}), - **({"event_id": str(row[8])} if row[8] is not None else {}), - "origin_server_ts": Timestamp(row[9]), - } - for row in txn - ] - - return await self.db_pool.runInteraction( - "get_finalised_delayed_events_for_user", - get_finalised_delayed_events_for_user, - ) - async def process_timeout_delayed_events( self, current_ts: Timestamp, reprocess_events: bool = False ) -> tuple[ diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 8799b3243a1..3af7d13858f 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -125,9 +125,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.reactor.advance(Duration(days=1).as_secs()) def test_delayed_events_empty_on_startup(self) -> None: - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertListEqual([], finalised) + self.assertListEqual([], self._get_delayed_events()) def test_delayed_event_lookup(self) -> None: # Schedule a message event @@ -228,7 +226,7 @@ def test_delayed_event_lookup(self) -> None: # Test that the list lookup retrieves the same items (with legacy fields included) self.assertEqual( - self._get_scheduled_delayed_events(), + self._get_delayed_events(), [ event | { @@ -260,13 +258,9 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: delay_id = channel.json_body.get("delay_id") assert delay_id is not None - scheduled, finalised = self._get_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - self.assertListEqual([], finalised) - - scheduled_event = scheduled[0] - content = self._get_delayed_event_content(scheduled_event) - + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -277,17 +271,7 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: ) self.reactor.advance(1) - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertEqual(1, len(finalised), finalised) - - finalised_event_info = finalised[0] - self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) - self.assertEqual("send", finalised_event_info["outcome"]) - self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) - + self.assertListEqual([], self._get_delayed_events()) content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -318,9 +302,9 @@ def test_delayed_member_events_are_sent_on_timeout(self) -> None: delay_id = channel.json_body.get("delay_id") assert delay_id is not None - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - content = self._get_delayed_event_content(scheduled[0]) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual("leave", content.get("membership"), content) self.assertEqual("Delayed kick", content.get("reason"), content) @@ -333,7 +317,7 @@ def test_delayed_member_events_are_sent_on_timeout(self) -> None: self.assertEqual("join", content.get("membership"), content) self.reactor.advance(1) - self.assertListEqual([], self._get_scheduled_delayed_events()) + self.assertListEqual([], self._get_delayed_events()) content = self.helper.get_state( self.room_id, "m.room.member", @@ -447,11 +431,9 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: assert delay_id is not None self.reactor.advance(1) - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - - scheduled_event = scheduled[0] - content = self._get_delayed_event_content(scheduled_event) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -463,17 +445,7 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: channel = self._update_delayed_event(delay_id, "cancel", action_in_path) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertEqual(1, len(finalised), finalised) - - finalised_event_info = finalised[0] - self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) - self.assertEqual("cancel", finalised_event_info["outcome"]) - self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertNotIn("event_id", finalised_event_info) + self.assertListEqual([], self._get_delayed_events()) self.reactor.advance(1) content = self.helper.get_state( @@ -564,11 +536,9 @@ def test_send_delayed_state_event( assert delay_id is not None self.reactor.advance(1) - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - - scheduled_event = scheduled[0] - content = self._get_delayed_event_content(scheduled_event) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual(content_value, content.get(content_property_name), content) self.helper.get_state( self.room_id, @@ -580,18 +550,7 @@ def test_send_delayed_state_event( channel = self._update_delayed_event(delay_id, "send", action_in_path) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertEqual(1, len(finalised), finalised) - - finalised_event_info = finalised[0] - self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) - self.assertEqual("send", finalised_event_info["outcome"]) - self.assertEqual("action", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) - + self.assertListEqual([], self._get_delayed_events()) content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -644,11 +603,9 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: assert delay_id is not None self.reactor.advance(1) - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - - scheduled_event = scheduled[0] - content = self._get_delayed_event_content(scheduled_event) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -662,12 +619,9 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.reactor.advance(1) - - scheduled, finalised = self._get_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - self.assertListEqual([], finalised) - - content = self._get_delayed_event_content(scheduled[0]) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) + content = self._get_delayed_event_content(events[0]) self.assertEqual(setter_expected, content.get(setter_key), content) self.helper.get_state( self.room_id, @@ -678,16 +632,7 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: ) self.reactor.advance(1) - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertEqual(1, len(finalised), finalised) - - finalised_event_info = finalised[0] - self.assertEqual("send", finalised_event_info["outcome"]) - self.assertEqual("delay", finalised_event_info["reason"]) - self.assertNotIn("error", finalised_event_info) - self.assertIsNotNone(finalised_event_info["event_id"]) - + self.assertListEqual([], self._get_delayed_events()) content = self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -768,8 +713,8 @@ def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self.assertEqual(HTTPStatus.OK, channel.code, channel.result) delay_id = channel.json_body.get("delay_id") assert delay_id is not None - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) self.helper.send_state( self.room_id, @@ -780,8 +725,8 @@ def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self.user1_access_token, state_key=state_key, ) - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) self.reactor.advance(1) content = self.helper.get_state( @@ -812,9 +757,8 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( self.assertEqual(HTTPStatus.OK, channel.code, channel.result) delay_id = channel.json_body.get("delay_id") assert delay_id is not None - scheduled = self._get_scheduled_delayed_events() - self.assertEqual(1, len(scheduled), scheduled) - scheduled_event = scheduled[0] + events = self._get_delayed_events() + self.assertEqual(1, len(events), events) setter_expected = "other_user" self.helper.send_state( @@ -826,23 +770,7 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( self.user2_access_token, state_key=state_key, ) - - scheduled, finalised = self._get_delayed_events() - self.assertListEqual([], scheduled) - self.assertEqual(1, len(finalised), finalised) - - finalised_event_info = finalised[0] - self.assertDictEqual(scheduled_event, finalised_event_info["delayed_event"]) - self.assertEqual("cancel", finalised_event_info["outcome"]) - self.assertEqual("error", finalised_event_info["reason"]) - self.assert_dict( - { - "errcode": "M_UNKNOWN", - "org.matrix.msc4140.errcode": "M_CANCELLED_BY_STATE_UPDATE", - }, - finalised_event_info["error"], - ) - self.assertNotIn("event_id", finalised_event_info) + self.assertListEqual([], self._get_delayed_events()) self.reactor.advance(1) content = self.helper.get_state( @@ -856,7 +784,7 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( self._find_sent_delayed_event(self.user1_access_token, delay_id, False) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) - def _get_delayed_events(self) -> tuple[list[JsonDict], list[JsonDict]]: + def _get_delayed_events(self) -> list[JsonDict]: channel = self.make_request( "GET", PATH_PREFIX, @@ -864,56 +792,13 @@ def _get_delayed_events(self) -> tuple[list[JsonDict], list[JsonDict]]: ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - scheduled = self._validate_scheduled_delayed_events(channel.json_body) - finalised = self._validate_finalised_delayed_events(channel.json_body) - - return scheduled, finalised - - def _get_scheduled_delayed_events(self) -> list[JsonDict]: - channel = self.make_request( - "GET", - PATH_PREFIX + "?status=scheduled", - access_token=self.user1_access_token, - ) - self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - - scheduled = self._validate_scheduled_delayed_events(channel.json_body) - - return scheduled - - def _get_finalised_delayed_events(self) -> list[JsonDict]: - channel = self.make_request( - "GET", - PATH_PREFIX + "?status=finalised", - access_token=self.user1_access_token, - ) - self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - - finalised = self._validate_finalised_delayed_events(channel.json_body) - - return finalised - - def _validate_scheduled_delayed_events(self, json_body: JsonDict) -> list[JsonDict]: - key = "scheduled" - self.assertIn(key, json_body) - - scheduled = json_body[key] - self.assertIsInstance(scheduled, list) - - return scheduled - - def _validate_finalised_delayed_events(self, json_body: JsonDict) -> list[JsonDict]: - key = "finalised" - self.assertIn(key, json_body) - - finalised = json_body[key] - self.assertIsInstance(finalised, list) + key = "delayed_events" + self.assertIn(key, channel.json_body) - for item in finalised: - for key in ("delayed_event", "outcome", "reason", "origin_server_ts"): - self.assertIsNotNone(item.get(key)) + events = channel.json_body[key] + self.assertIsInstance(events, list) - return finalised + return events def _get_delayed_event_content(self, event: JsonDict) -> JsonDict: key = "content" From f9a17012a9a634fc265832a46d7b4b644a1c004b Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 12:27:57 +0200 Subject: [PATCH 23/47] MSC4140: return finalised delayed events from GET /delayed_events/{delay_id} Add an optional `finalised` field to `DelayedEventResponse`, populated by `get_delayed_event_for_user` from the `finalised_ts`, `finalised_event_id` and `finalised_error` columns: `finalised_ts` plus `event_id` if the event was sent, or `error` (the standard error response) if sending it failed, and neither if the user cancelled it. The single-event lookup now matches finalised rows too, and no longer hides rows that are being sent, so a lookup never 404s for an event the user scheduled until it is pruned. Tests cover the sent, cancelled, failed (sender left the room) and ratelimited-on-timeout outcomes, and that other users still get a 404. --- synapse/handlers/delayed_events.py | 3 +- .../storage/databases/main/delayed_events.py | 44 ++++- tests/rest/client/test_delayed_events.py | 174 ++++++++++++++++-- 3 files changed, 203 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 3bb193019d8..1f84f426b55 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -575,7 +575,8 @@ async def get_for_user( self, requester: Requester, delay_id: str ) -> DelayedEventResponse: """ - Return the specified pending delayed event requested by the given user. + Return the specified delayed event requested by the given user, + whether it is still scheduled or has been finalised. Raises: NotFoundError: if no matching delayed event could be found. diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index a0256463424..7e8e50ed3fc 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -81,6 +81,9 @@ class DelayedEventResponse: delay_ms: int delayed_since_ts: int content: JsonDict = attr.ib(converter=db_to_json) + # Present only for finalised delayed events (MSC4140): `finalised_ts`, plus + # `event_id` if the event was sent, or `error` if sending it failed. + finalised: JsonDict | None = None def asdict(self) -> JsonDict: return attr.asdict(self, filter=lambda _attr, v: v is not None) @@ -433,7 +436,8 @@ async def get_delayed_event_for_user( user_localpart: str, ) -> DelayedEventResponse: """ - Returns the specified pending delayed event owned by the given user. + Returns the specified delayed event owned by the given user, + whether it is still scheduled or has been finalised. Raises: NotFoundError: if there is no matching delayed event. @@ -447,18 +451,29 @@ async def get_delayed_event_for_user( state_key, delay, send_ts - delay, - content + content, + finalised_ts, + finalised_event_id, + finalised_error FROM delayed_events WHERE delay_id = ? AND user_localpart = ? - AND NOT is_processed - AND finalised_ts IS NULL """, delay_id, user_localpart, ) if not rows: raise NotFoundError("Delayed event not found") - return DelayedEventResponse(delay_id, *rows[0]) + row = rows[0] + return DelayedEventResponse( + delay_id, + row[0], + row[1], + row[2], + row[3], + row[4], + row[5], + _make_finalised_dict(row[6], row[7], row[8]), + ) async def get_all_delayed_events_for_user( self, @@ -920,6 +935,25 @@ def _generate_delay_id() -> DelayID: return DelayID(f"syd_{stringutils.random_string(20)}") +def _make_finalised_dict( + finalised_ts: int | None, + finalised_event_id: str | None, + finalised_error: str | None, +) -> JsonDict | None: + """ + Builds the `finalised` object of a delayed event's API representation + from its stored finalisation columns, or returns None if it is not finalised. + """ + if finalised_ts is None: + return None + finalised: JsonDict = {"finalised_ts": finalised_ts} + if finalised_event_id is not None: + finalised["event_id"] = finalised_event_id + elif finalised_error is not None: + finalised["error"] = db_to_json(finalised_error) + return finalised + + def _generate_cancelled_by_state_update_json() -> str: return json_encoder.encode( cs_error( diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 3af7d13858f..eac282b6e2f 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -280,9 +280,13 @@ def test_delayed_state_events_are_sent_on_timeout(self) -> None: ) self.assertEqual(setter_expected, content.get(setter_key), content) - self._find_sent_delayed_event(self.user1_access_token, delay_id, True) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + self._assert_finalised(delay_id, event_id=event_id) + def test_delayed_member_events_are_sent_on_timeout(self) -> None: channel = self.make_request( "PUT", @@ -327,9 +331,69 @@ def test_delayed_member_events_are_sent_on_timeout(self) -> None: self.assertEqual("leave", content.get("membership"), content) self.assertEqual("Delayed kick", content.get("reason"), content) - self._find_sent_delayed_event(self.user1_access_token, delay_id, True) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) + self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + + self._assert_finalised(delay_id, event_id=event_id) + + def test_delayed_event_is_finalised_with_error_if_timeout_send_fails( + self, + ) -> None: + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 900), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + + # Leave the room so that the delayed event can no longer be sent + self.helper.leave(self.room_id, self.user1_user_id, tok=self.user1_access_token) + + self.reactor.advance(1) + self.assertListEqual([], self._get_delayed_events()) + self._find_sent_delayed_event(self.user1_access_token, delay_id, False) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + self._assert_finalised(delay_id, errcode=Codes.FORBIDDEN) + + @unittest.override_config({"rc_message": {"per_second": 0.1, "burst_count": 3}}) + def test_delayed_event_is_finalised_with_error_if_timeout_send_is_ratelimited( + self, + ) -> None: + delay_ids = [] + for delay_ms in (800, 900): + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, delay_ms), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + delay_ids.append(delay_id) + + # Both delayed events are due, but only the first fits in the ratelimit. + # The second must not be retried, and its failure must be recorded. + self.reactor.advance(1) + self.assertListEqual([], self._get_delayed_events()) + + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_ids[0], True + ) + self._find_sent_delayed_event(self.user1_access_token, delay_ids[1], False) + + self._assert_finalised(delay_ids[0], event_id=event_id) + self._assert_finalised(delay_ids[1], errcode=Codes.LIMIT_EXCEEDED) + + self.reactor.advance(100) + self._find_sent_delayed_event(self.user1_access_token, delay_ids[1], False) + def test_get_delayed_events_auth(self) -> None: channel = self.make_request("GET", PATH_PREFIX) self.assertEqual(HTTPStatus.UNAUTHORIZED, channel.code, channel.result) @@ -446,6 +510,15 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: channel = self._update_delayed_event(delay_id, "cancel", action_in_path) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.assertListEqual([], self._get_delayed_events()) + self._assert_finalised(delay_id) + + # Other users must not be able to look up the finalised delayed event either + channel = self.make_request( + "GET", + f"{PATH_PREFIX}/{delay_id}", + access_token=self.user2_access_token, + ) + self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, channel.result) self.reactor.advance(1) content = self.helper.get_state( @@ -458,6 +531,7 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: self._find_sent_delayed_event(self.user1_access_token, delay_id, False) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + self._assert_finalised(delay_id) @parameterized.expand((True, False)) @unittest.override_config( @@ -559,9 +633,13 @@ def test_send_delayed_state_event( ) self.assertEqual(content_value, content.get(content_property_name), content) - self._find_sent_delayed_event(self.user1_access_token, delay_id, True) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + self._assert_finalised(delay_id, event_id=event_id) + @parameterized.expand((True, False)) @unittest.override_config({"rc_message": {"per_second": 2.5, "burst_count": 3}}) def test_send_delayed_event_ratelimit(self, action_in_path: bool) -> None: @@ -641,9 +719,13 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: ) self.assertEqual(setter_expected, content.get(setter_key), content) - self._find_sent_delayed_event(self.user1_access_token, delay_id, True) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) self._find_sent_delayed_event(self.user2_access_token, delay_id, False) + self._assert_finalised(delay_id, event_id=event_id) + @parameterized.expand((True, False)) @unittest.override_config( {"rc_delayed_event_mgmt": {"per_second": 0.5, "burst_count": 1}} @@ -772,6 +854,13 @@ def test_delayed_state_is_cancelled_by_new_state_from_other_user( ) self.assertListEqual([], self._get_delayed_events()) + finalised = self._assert_finalised(delay_id, errcode=Codes.UNKNOWN) + self.assertEqual( + "M_CANCELLED_BY_STATE_UPDATE", + finalised["error"].get("org.matrix.msc4140.errcode"), + finalised, + ) + self.reactor.advance(1) content = self.helper.get_state( self.room_id, @@ -800,6 +889,63 @@ def _get_delayed_events(self) -> list[JsonDict]: return events + def _get_delayed_event(self, delay_id: str) -> JsonDict: + """Look up a single delayed event of user1, which must exist.""" + channel = self.make_request( + "GET", + f"{PATH_PREFIX}/{delay_id}", + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + return channel.json_body + + def _assert_scheduled(self, delay_id: str) -> JsonDict: + """Assert that user1's delayed event is still scheduled, and return it.""" + delayed_event = self._get_delayed_event(delay_id) + self.assertNotIn("finalised", delayed_event) + return delayed_event + + def _assert_finalised( + self, + delay_id: str, + *, + event_id: str | None = None, + errcode: str | None = None, + ) -> JsonDict: + """Assert that user1's delayed event has been finalised, and return + its `finalised` object. + + Args: + delay_id: The ID of the delayed event to look up. + event_id: The ID of the event that the delayed event was sent as, if it was sent. + errcode: The error code that prevented the delayed event from being sent, if any. + If neither this nor `event_id` is set, the delayed event must have been + cancelled by the user. + """ + delayed_event = self._get_delayed_event(delay_id) + finalised = delayed_event.get("finalised") + self.assertIsInstance(finalised, dict, delayed_event) + assert finalised is not None + + finalised_ts = finalised.get("finalised_ts") + self.assertIsInstance(finalised_ts, int, finalised) + self.assertLessEqual(finalised_ts, self.clock.time_msec()) + self.assertGreaterEqual(finalised_ts, delayed_event["delayed_since_ts"]) + + if event_id is not None: + self.assertDictEqual( + {"finalised_ts": finalised_ts, "event_id": event_id}, finalised + ) + elif errcode is not None: + self.assertNotIn("event_id", finalised) + self.assertEqual( + errcode, finalised.get("error", {}).get("errcode"), finalised + ) + else: + self.assertDictEqual({"finalised_ts": finalised_ts}, finalised) + + return finalised + def _get_delayed_event_content(self, event: JsonDict) -> JsonDict: key = "content" self.assertIn(key, event) @@ -826,7 +972,7 @@ def _update_delayed_event( def _find_sent_delayed_event( self, access_token: str, delay_id: str, should_find: bool - ) -> None: + ) -> str | None: """Call /sync and look for a synced event with a specified delay_id. At most one event will ever have a matching delay_id. @@ -834,28 +980,32 @@ def _find_sent_delayed_event( access_token: The access token of the user to call /sync for. delay_id: The delay_id to search for in synced events. should_find: Whether /sync should include an event with a matching delay_id. + + Returns: The ID of the event with the matching delay_id, if one was found. """ channel = self.make_request("GET", "/sync", access_token=access_token) self.assertEqual(HTTPStatus.OK, channel.code) - rooms = channel.json_body["rooms"] + rooms = channel.json_body.get("rooms", {}) events = [] for membership in "join", "leave": - if membership in rooms: - events += rooms[membership][self.room_id]["timeline"]["events"] + room = rooms.get(membership, {}).get(self.room_id) + if room is not None: + events += room["timeline"]["events"] - found = False + found: str | None = None for event in events: if event["unsigned"].get("org.matrix.msc4140.delay_id") == delay_id: if not should_find: self.fail( "Found event with matching delay_id, but expected to not find one" ) - if found: + if found is not None: self.fail("Found multiple events with matching delay_id") - found = True - if should_find and not found: + found = event["event_id"] + if should_find and found is None: self.fail("Did not find event with matching delay_id") + return found def _get_path_for_delayed_state( From 6fb0a6c56c692551229f7180b0299b1c76d17d64 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 12:28:46 +0200 Subject: [PATCH 24/47] MSC4140: test that finalised delayed events do not count towards the user limit Finalised rows now stay in `delayed_events`, so the per-user cap's COUNT, its `Retry-After` subquery and the list query all filter on `finalised_ts IS NULL` (done while resolving the merge, as develop's cap tests depend on it). Add the regression test: a user with more finalised delayed events than `msc4140_max_delayed_events_per_user` can still schedule up to the cap, the list shows scheduled rows only, and the `Retry-After` of the cap error is computed from scheduled rows only. --- tests/rest/client/test_delayed_events.py | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index eac282b6e2f..4c4b69d8785 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -394,6 +394,58 @@ def test_delayed_event_is_finalised_with_error_if_timeout_send_is_ratelimited( self.reactor.advance(100) self._find_sent_delayed_event(self.user1_access_token, delay_ids[1], False) + @unittest.override_config( + {"experimental_features": {"msc4140_max_delayed_events_per_user": 2}} + ) + def test_finalised_delayed_events_do_not_count_towards_user_limit(self) -> None: + # Disable rate-limits for this user, to test only the limit on scheduled events + self.get_success( + self.hs.get_datastores().main.set_ratelimit_for_user( + self.user1_user_id, 0, 0 + ) + ) + + def schedule(delay_ms: int) -> FakeChannel: + return self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, delay_ms), + {}, + self.user1_access_token, + ) + + # Finalise more delayed events than the limit allows to be scheduled at once: + # one cancelled, one sent manually, one sent on timeout + finalised_delay_ids = [] + for delay_ms, action in ((1000, "cancel"), (1000, "send"), (900, None)): + channel = schedule(delay_ms) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body["delay_id"] + finalised_delay_ids.append(delay_id) + if action is not None: + channel = self._update_delayed_event(delay_id, action, True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.reactor.advance(1) + for delay_id in finalised_delay_ids: + self.assertIn("finalised", self._get_delayed_event(delay_id)) + self.assertListEqual([], self._get_delayed_events()) + + # The finalised delayed events must not count towards the limit + for delay_ms in (5000, 6000): + channel = schedule(delay_ms) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertEqual(2, len(self._get_delayed_events())) + + # ...nor towards the time to wait until the limit is no longer reached, + # which must be computed from the scheduled delayed events only + channel = schedule(7000) + self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) + self.assertEqual( + Codes.LIMIT_EXCEEDED, channel.json_body["errcode"], channel.json_body + ) + retry_after_headers = channel.headers.getRawHeaders("Retry-After") + assert retry_after_headers + self.assertEqual(5, int(retry_after_headers[0])) + def test_get_delayed_events_auth(self) -> None: channel = self.make_request("GET", PATH_PREFIX) self.assertEqual(HTTPStatus.UNAUTHORIZED, channel.code, channel.result) From 9e13d9b39f758614ffb8e227cd782bffcea4e3b7 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 17:50:40 +0200 Subject: [PATCH 25/47] MSC4140: return 409 for actions that conflict with a finalised delayed event Once a delayed event is finalised, repeating the action that matches its outcome (`send` on a sent event, `cancel` on a cancelled one) returns `200 {}`, and a conflicting action (`send` or `restart` on a cancelled event, `cancel` or `restart` on a sent one) returns `409 M_UNKNOWN`. The already-sent path of `process_target_delayed_event` and the already-cancelled path of `cancel_delayed_event` returned no next send time, which made the handler cancel the timer armed for the user's other delayed events. Return the real next send time from both. Tests cover both outcomes, and check that the user's other scheduled delayed event is still sent on time afterwards. --- synapse/handlers/delayed_events.py | 9 +- .../storage/databases/main/delayed_events.py | 4 +- tests/rest/client/test_delayed_events.py | 88 +++++++++++++++++++ 3 files changed, 97 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 1f84f426b55..a5045317c4f 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -484,20 +484,25 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: """ Immediately sends the matching delayed event, instead of waiting for its scheduled delivery. + If the delayed event has already been sent, this does nothing. + Raises: NotFoundError: if no matching delayed event could be found. + SynapseError: if the delayed event has already been cancelled, + or if sending it failed. """ assert self._is_master await self._mgmt_ratelimit(request) await make_deferred_yieldable(self._initialized_from_db) event, next_send_ts = await self._store.process_target_delayed_event(delay_id) + if event is None: + return if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) - if event: - await self._send_event(event, False) + await self._send_event(event, False) async def _mgmt_ratelimit(self, request: SynapseRequest) -> None: """ diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 7e8e50ed3fc..1c082dbecc2 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -620,6 +620,7 @@ async def process_target_delayed_event( Returns: The details of the matching delayed event, and the send time of the next delayed event to be sent, if any. + The details are None if the delayed event has already been sent. Raises: NotFoundError: if there is no matching delayed event. @@ -669,7 +670,7 @@ def process_target_delayed_event_txn( HTTPStatus.CONFLICT, "Delayed event has already been cancelled", ) - return None, None + return None, self._get_next_delayed_event_send_ts_txn(txn) event = DelayedEventDetails( RoomID.from_string(row[0]), @@ -736,7 +737,6 @@ def cancel_delayed_event_txn( HTTPStatus.CONFLICT, "Delayed event has already been sent", ) - return None return self._get_next_delayed_event_send_ts_txn(txn) return await self.db_pool.runInteraction( diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 4c4b69d8785..f5d8550ee11 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -714,6 +714,94 @@ def test_send_delayed_event_ratelimit(self, action_in_path: bool) -> None: channel = self._update_delayed_event(delay_ids.pop(0), "send", action_in_path) self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) + def test_repeated_action_on_cancelled_delayed_event(self) -> None: + delay_ids = [] + for delay_ms in (900, 100000): + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, delay_ms), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + delay_ids.append(delay_id) + delay_id = delay_ids[1] + + channel = self._update_delayed_event(delay_id, "cancel", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + + # Repeating the action that matches the outcome must succeed + channel = self._update_delayed_event(delay_id, "cancel", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertDictEqual({}, channel.json_body) + + # Actions that conflict with the outcome must fail + for action in ("send", "restart"): + channel = self._update_delayed_event(delay_id, action, True) + self.assertEqual(HTTPStatus.CONFLICT, channel.code, channel.result) + self.assertEqual( + Codes.UNKNOWN, channel.json_body["errcode"], channel.result + ) + + self._assert_finalised(delay_id) + + # None of this may have disturbed the other scheduled delayed event + self.reactor.advance(1) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_ids[0], True + ) + self._assert_finalised(delay_ids[0], event_id=event_id) + self._find_sent_delayed_event(self.user1_access_token, delay_id, False) + + def test_repeated_action_on_sent_delayed_event(self) -> None: + delay_ids = [] + for delay_ms in (900, 100000): + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, delay_ms), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + delay_ids.append(delay_id) + delay_id = delay_ids[1] + + channel = self._update_delayed_event(delay_id, "send", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) + + # Repeating the action that matches the outcome must succeed, without resending + channel = self._update_delayed_event(delay_id, "send", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertDictEqual({}, channel.json_body) + self.assertEqual( + event_id, + self._find_sent_delayed_event(self.user1_access_token, delay_id, True), + ) + + # Actions that conflict with the outcome must fail + for action in ("cancel", "restart"): + channel = self._update_delayed_event(delay_id, action, True) + self.assertEqual(HTTPStatus.CONFLICT, channel.code, channel.result) + self.assertEqual( + Codes.UNKNOWN, channel.json_body["errcode"], channel.result + ) + + self._assert_finalised(delay_id, event_id=event_id) + + # None of this may have disturbed the other scheduled delayed event + self.reactor.advance(1) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_ids[0], True + ) + self._assert_finalised(delay_ids[0], event_id=event_id) + @parameterized.expand((True, False)) def test_restart_delayed_state_event(self, action_in_path: bool) -> None: state_key = "to_send_on_restarted_timeout" From 2cda2c4304d3bdee5a96ff31e3c480237c03a22c Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 17:50:41 +0200 Subject: [PATCH 26/47] MSC4140: keep a delayed event scheduled if sending it manually fails On the `send` action, mark the row processed, attempt the send, finalise on success, and on failure restore `is_processed = FALSE` for that one row (new `unprocess_delayed_event`), re-arm the timer if needed and re-raise, so the client gets the response `/send` or `/state` would have given and can retry until the scheduled send time. Timed sends still finalise with the error and are not retried. Restructure `_send_event` without the `finally` block, whose finalisation read variables that were unbound on the error path. Tests cover a manual send that succeeds once the sender rejoins the room, a delayed state event that is still sent on timeout once the power levels allow it, and a ratelimited manual send that stays scheduled. --- synapse/handlers/delayed_events.py | 71 ++++++--- .../storage/databases/main/delayed_events.py | 31 ++++ tests/rest/client/test_delayed_events.py | 135 ++++++++++++++++-- 3 files changed, 207 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index a5045317c4f..16fde658929 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -485,6 +485,8 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: Immediately sends the matching delayed event, instead of waiting for its scheduled delivery. If the delayed event has already been sent, this does nothing. + If sending it fails, it stays scheduled, so that sending may be retried + until its scheduled send time. Raises: NotFoundError: if no matching delayed event could be found. @@ -611,6 +613,22 @@ async def _send_event( event: DelayedEventDetails, finalise_error: bool, ) -> Timestamp: + """ + Sends the given delayed event, which must have been marked as processed, + and finalises it in the DB. + + Args: + event: The delayed event to send. + finalise_error: Whether to finalise the delayed event with the error + if sending it fails. If False, the delayed event is instead + unmarked as processed, so that it stays scheduled, and the error + is re-raised. + + Returns: The timestamp at which the delayed event was finalised. + + Raises: + Exception: whatever sending the event raised, if finalise_error is False. + """ user_id = UserID(event.user_localpart, self._config.server.server_name) user_id_str = user_id.to_string() # Create a new requester from what data is currently available @@ -620,7 +638,8 @@ async def _send_event( device_id=event.device_id, ) - finalised_ts = None + finalised_ts: Timestamp | None = None + send_error: JsonDict | None = None try: if event.state_key is not None and event.type == EventTypes.Member: membership = event.content.get("membership") @@ -664,29 +683,39 @@ async def _send_event( finalised_ts = Timestamp(sent_event.origin_server_ts) except ShadowBanError: event_id = generate_fake_event_id() - send_error = None except Exception as e: - if finalise_error: - if isinstance(e, SynapseError): - send_error = e.error_dict(None) + if not finalise_error: + # Keep the delayed event scheduled, so that sending it may be + # retried until its scheduled send time. + try: + next_send_ts = await self._store.unprocess_delayed_event( + event.delay_id + ) + except Exception: + logger.exception("Failed to reschedule delayed event") else: - send_error = cs_error("Internal server error") - else: + if self._next_send_ts_changed(next_send_ts): + self._schedule_next_at_or_none(next_send_ts) raise - else: - send_error = None - finally: - # TODO: If this is a temporary error, retry. Otherwise, consider notifying clients of the failure - if finalised_ts is None: - finalised_ts = self._get_current_ts() - try: - await self._store.finalise_processed_delayed_event( - event.delay_id, - send_error or event_id, - finalised_ts, - ) - except Exception: - logger.exception("Failed to finalise processed delayed event") + + # Do not retry, even if the error may be temporary. + # Record the error for the user to find and act on instead. + if isinstance(e, SynapseError): + send_error = e.error_dict(None) + else: + logger.exception("Failed to send delayed event") + send_error = cs_error("Internal server error") + + if finalised_ts is None: + finalised_ts = self._get_current_ts() + try: + await self._store.finalise_processed_delayed_event( + event.delay_id, + send_error if send_error is not None else event_id, + finalised_ts, + ) + except Exception: + logger.exception("Failed to finalise processed delayed event") if send_error is None: set_tag("event_id", event_id) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 1c082dbecc2..75cc9ea4494 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -881,6 +881,37 @@ def finalise_processed_delayed_state_events(txn: LoggingTransaction) -> None: finalise_processed_delayed_state_events, ) + async def unprocess_delayed_event(self, delay_id: DelayID) -> Timestamp | None: + """ + Unmark the matching delayed event for processing, so that it is scheduled again. + Used when a delayed event that was marked for processing could not be sent. + + Returns: The send time of the next delayed event to be sent, if any. + + Raises: + StoreError: if there is no matching delayed event, or if it has not + been marked as processed, or if it has already been finalised. + """ + + def unprocess_delayed_event_txn(txn: LoggingTransaction) -> Timestamp | None: + txn.execute( + """ + UPDATE delayed_events SET is_processed = FALSE + WHERE delay_id = ? + AND is_processed + AND finalised_ts IS NULL + """, + (delay_id,), + ) + if txn.rowcount == 0: + raise StoreError(404, "No row found (delayed_events)") + return self._get_next_delayed_event_send_ts_txn(txn) + + return await self.db_pool.runInteraction( + "unprocess_delayed_event", + unprocess_delayed_event_txn, + ) + async def unprocess_delayed_events(self) -> None: """ Unmark all delayed events for processing. diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index f5d8550ee11..4ecc6962dde 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -711,9 +711,110 @@ def test_send_delayed_event_ratelimit(self, action_in_path: bool) -> None: channel = self._update_delayed_event(delay_ids.pop(0), "send", action_in_path) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - channel = self._update_delayed_event(delay_ids.pop(0), "send", action_in_path) + delay_id = delay_ids.pop(0) + channel = self._update_delayed_event(delay_id, "send", action_in_path) self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) + # The delayed event that could not be sent must stay scheduled + self._assert_scheduled(delay_id) + self.assertEqual(1, len(self._get_delayed_events())) + + def test_send_delayed_event_fails_and_stays_scheduled_until_retried( + self, + ) -> None: + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + + # Leave the room, so that the delayed event cannot be sent + self.helper.leave(self.room_id, self.user1_user_id, tok=self.user1_access_token) + + # The response must be the error that sending the event would have given + channel = self._update_delayed_event(delay_id, "send", True) + self.assertEqual(HTTPStatus.FORBIDDEN, channel.code, channel.result) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"], channel.result) + + # ...and the delayed event must stay scheduled + self._assert_scheduled(delay_id) + self.assertEqual(1, len(self._get_delayed_events())) + self._find_sent_delayed_event(self.user1_access_token, delay_id, False) + + # Once the cause of the error is fixed, sending must succeed + self.helper.join(self.room_id, self.user1_user_id, tok=self.user1_access_token) + channel = self._update_delayed_event(delay_id, "send", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + + self.assertListEqual([], self._get_delayed_events()) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) + self._assert_finalised(delay_id, event_id=event_id) + + def test_send_delayed_state_event_fails_and_stays_scheduled_until_timeout( + self, + ) -> None: + state_key = "to_send_on_timeout_after_failed_send" + channel = self.make_request( + "PUT", + _get_path_for_delayed_state(self.room_id, _EVENT_TYPE, state_key, 2000), + {}, + self.user2_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + + # Raise the power level required to send the state event, + # so that user2 is no longer allowed to send it + self.helper.send_state( + self.room_id, + "m.room.power_levels", + {"users": {self.user1_user_id: 100}, "events": {_EVENT_TYPE: 50}}, + self.user1_access_token, + ) + + # The response must be the error that sending the state event would have given + channel = self._update_delayed_event(delay_id, "send", True) + self.assertEqual(HTTPStatus.FORBIDDEN, channel.code, channel.result) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"], channel.result) + + # ...and the delayed event must stay scheduled + self._assert_scheduled(delay_id, self.user2_access_token) + self.helper.get_state( + self.room_id, + _EVENT_TYPE, + self.user2_access_token, + state_key=state_key, + expect_code=HTTPStatus.NOT_FOUND, + ) + + # Once the cause of the error is fixed, the delayed event must be sent on timeout + self.helper.send_state( + self.room_id, + "m.room.power_levels", + {"users": {self.user1_user_id: 100}, "events": {_EVENT_TYPE: 0}}, + self.user1_access_token, + ) + self.reactor.advance(2) + self.helper.get_state( + self.room_id, + _EVENT_TYPE, + self.user2_access_token, + state_key=state_key, + ) + event_id = self._find_sent_delayed_event( + self.user2_access_token, delay_id, True + ) + self._assert_finalised( + delay_id, event_id=event_id, access_token=self.user2_access_token + ) + def test_repeated_action_on_cancelled_delayed_event(self) -> None: delay_ids = [] for delay_ms in (900, 100000): @@ -1029,19 +1130,33 @@ def _get_delayed_events(self) -> list[JsonDict]: return events - def _get_delayed_event(self, delay_id: str) -> JsonDict: - """Look up a single delayed event of user1, which must exist.""" + def _get_delayed_event( + self, delay_id: str, access_token: str | None = None + ) -> JsonDict: + """Look up a single delayed event, which must exist. + + Args: + delay_id: The ID of the delayed event to look up. + access_token: The access token of the user who scheduled it. Defaults to user1's. + """ channel = self.make_request( "GET", f"{PATH_PREFIX}/{delay_id}", - access_token=self.user1_access_token, + access_token=access_token or self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) return channel.json_body - def _assert_scheduled(self, delay_id: str) -> JsonDict: - """Assert that user1's delayed event is still scheduled, and return it.""" - delayed_event = self._get_delayed_event(delay_id) + def _assert_scheduled( + self, delay_id: str, access_token: str | None = None + ) -> JsonDict: + """Assert that a delayed event is still scheduled, and return it. + + Args: + delay_id: The ID of the delayed event to look up. + access_token: The access token of the user who scheduled it. Defaults to user1's. + """ + delayed_event = self._get_delayed_event(delay_id, access_token) self.assertNotIn("finalised", delayed_event) return delayed_event @@ -1051,8 +1166,9 @@ def _assert_finalised( *, event_id: str | None = None, errcode: str | None = None, + access_token: str | None = None, ) -> JsonDict: - """Assert that user1's delayed event has been finalised, and return + """Assert that a delayed event has been finalised, and return its `finalised` object. Args: @@ -1061,8 +1177,9 @@ def _assert_finalised( errcode: The error code that prevented the delayed event from being sent, if any. If neither this nor `event_id` is set, the delayed event must have been cancelled by the user. + access_token: The access token of the user who scheduled it. Defaults to user1's. """ - delayed_event = self._get_delayed_event(delay_id) + delayed_event = self._get_delayed_event(delay_id, access_token) finalised = delayed_event.get("finalised") self.assertIsInstance(finalised, dict, delayed_event) assert finalised is not None From 4095b54b0c654dfedb1e16cf9f511f377ef2ddae Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 17:50:41 +0200 Subject: [PATCH 27/47] MSC4140: read finalised delayed event retention config next to the user cap Move the two retention keys out of `synapse/config/experimental.py` into `synapse/config/server.py`, next to `msc4140_max_delayed_events_per_user` and with the same validation: `experimental_features.msc4140_finalised_retention_period` (non-negative duration, default 7d, 0 disables retention) and `experimental_features.msc4140_finalised_retention_limit_per_user` (non-negative integer, default 1000). The period was previously read from the root of the config rather than from `experimental_features`. Document both, and the existing `msc4140_max_delayed_events_per_user`, in the config schema next to `max_event_delay_duration` and regenerate `config_documentation.md`. Add config validation tests mirroring the existing ones for the cap. --- .../configuration/config_documentation.md | 24 +++++++++ schema/synapse-config.schema.yaml | 34 ++++++++++++ synapse/config/experimental.py | 10 ---- synapse/config/server.py | 28 ++++++++++ synapse/handlers/delayed_events.py | 4 +- tests/config/test_server.py | 52 +++++++++++++++++++ 6 files changed, 140 insertions(+), 12 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 74ac85256f7..5c4bdcbcc4b 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -853,6 +853,30 @@ Example configuration: max_event_delay_duration: 24h ``` --- +### `experimental_features` + +*(object)* Options for experimental features. Only the options for delayed events, as per [MSC4140](https://github.com/matrix-org/matrix-spec-proposals/pull/4140), are documented here. + +This setting has the following sub-options: + +* `msc4140_max_delayed_events_per_user` (integer): The maximum number of delayed events a user may have scheduled at a time. Must be a non-negative integer. If 0, sending of delayed events is disallowed. Defaults to `100`. + +* `msc4140_finalised_retention_period` (duration): How long a delayed event is retained for lookup after it has been finalised, i.e. sent, cancelled, or failed to be sent. Must be a non-negative duration. If 0, finalised delayed events are not retained. + + Finalised delayed events past their retention are pruned every 5 minutes. + + Defaults to `"7d"`. + +* `msc4140_finalised_retention_limit_per_user` (integer): The maximum number of finalised delayed events retained per user, keeping the most recently finalised ones. Must be a non-negative integer. If 0, finalised delayed events are not retained. Defaults to `1000`. + +Example configuration: +```yaml +experimental_features: + msc4140_max_delayed_events_per_user: 100 + msc4140_finalised_retention_period: 7d + msc4140_finalised_retention_limit_per_user: 1000 +``` +--- ### `user_types` *(object)* Configuration settings related to the user types feature. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index b34bc374f31..d18daf8c791 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -1048,6 +1048,40 @@ properties: default: null examples: - 24h + experimental_features: + type: object + description: >- + Options for experimental features. Only the options for delayed events, as per + [MSC4140](https://github.com/matrix-org/matrix-spec-proposals/pull/4140), + are documented here. + properties: + msc4140_max_delayed_events_per_user: + type: integer + description: >- + The maximum number of delayed events a user may have scheduled at a time. + Must be a non-negative integer. If 0, sending of delayed events is disallowed. + default: 100 + msc4140_finalised_retention_period: + $ref: "#/$defs/duration" + description: >- + How long a delayed event is retained for lookup after it has been finalised, + i.e. sent, cancelled, or failed to be sent. Must be a non-negative duration. + If 0, finalised delayed events are not retained. + + + Finalised delayed events past their retention are pruned every 5 minutes. + default: 7d + msc4140_finalised_retention_limit_per_user: + type: integer + description: >- + The maximum number of finalised delayed events retained per user, keeping + the most recently finalised ones. Must be a non-negative integer. + If 0, finalised delayed events are not retained. + default: 1000 + examples: + - msc4140_max_delayed_events_per_user: 100 + msc4140_finalised_retention_period: 7d + msc4140_finalised_retention_limit_per_user: 1000 user_types: type: object description: >- diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index dcb9db7f2b3..1c2f021322b 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -247,16 +247,6 @@ def read_config( # MSC4133: Custom profile fields self.msc4133_enabled: bool = experimental.get("msc4133_enabled", False) - # MSC4140: How long to keep finalised delayed events in the database before deleting them. - self.msc4140_finalised_retention_period = self.parse_duration( - config.get("msc4140_finalised_retention_period", "7d") - ) - - # MSC4140: How many finalised delayed events to keep per user before deleting them. - self.msc4140_finalised_per_user_retention_limit = experimental.get( - "msc4140_finalised_per_user_retention_limit", 1000 - ) - # MSC4143: Matrix RTC Transport using Livekit Backend self.msc4143_enabled: bool = experimental.get("msc4143_enabled", False) diff --git a/synapse/config/server.py b/synapse/config/server.py index 9c5a42fc52b..76ae2933229 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -994,6 +994,34 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: ("experimental", "msc4140_max_delayed_events_per_user"), ) + # How long a finalised delayed event (one that has been sent, cancelled, or + # failed to be sent) is retained for lookup. 0 disables retention. + finalised_retention_period_ms = self.parse_duration( + experimental.get("msc4140_finalised_retention_period", "7d") + ) + if finalised_retention_period_ms < 0: + raise ConfigError( + "'msc4140_finalised_retention_period' must be a non-negative duration", + ("experimental", "msc4140_finalised_retention_period"), + ) + self.msc4140_finalised_retention_period = Duration( + milliseconds=finalised_retention_period_ms + ) + + # The maximum number of finalised delayed events retained per user. + # 0 disables retention. + self.msc4140_finalised_retention_limit_per_user: int = experimental.get( + "msc4140_finalised_retention_limit_per_user", 1000 + ) + if ( + not isinstance(self.msc4140_finalised_retention_limit_per_user, int) + or self.msc4140_finalised_retention_limit_per_user < 0 + ): + raise ConfigError( + "'msc4140_finalised_retention_limit_per_user' must be a non-negative integer", + ("experimental", "msc4140_finalised_retention_limit_per_user"), + ) + self.msc4140_enabled = bool( self.max_delayed_events_per_user and self.max_event_delay_duration ) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 16fde658929..0452ef4cc44 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -335,8 +335,8 @@ async def _handle_state_deltas(self, deltas: list[StateDelta]) -> None: async def _prune_finalised_events(self) -> None: await self._store.prune_finalised_delayed_events( self._get_current_ts(), - self.hs.config.experimental.msc4140_finalised_retention_period, - self.hs.config.experimental.msc4140_finalised_per_user_retention_limit, + self._config.server.msc4140_finalised_retention_period.as_millis(), + self._config.server.msc4140_finalised_retention_limit_per_user, ) async def add( diff --git a/tests/config/test_server.py b/tests/config/test_server.py index f718e20a2a9..0ae4ae23687 100644 --- a/tests/config/test_server.py +++ b/tests/config/test_server.py @@ -229,6 +229,48 @@ def generate_config(value: Any) -> JsonDict: with self.assertRaises(ConfigError): _read_config(generate_config(disallowed_value)) + def test_finalised_delayed_events_retention_period_enforces_non_negative( + self, + ) -> None: + """ + Test that the configured retention period of finalised delayed events + must be a non-negative duration if set, as per documentation + """ + + def generate_config(value: Any) -> JsonDict: + return { + "experimental_features": {"msc4140_finalised_retention_period": value} + } + + for allowed_value in (0, 1, "7d"): + _read_config(generate_config(allowed_value)) + + for disallowed_value in (-1, "-1d", "1x", None): + with self.assertRaises((ConfigError, TypeError, ValueError)): + _read_config(generate_config(disallowed_value)) + + def test_finalised_delayed_events_retention_limit_per_user_enforces_non_negative_int( + self, + ) -> None: + """ + Test that the configured retention limit of finalised delayed events per user + must be a non-negative integer if set, as per documentation + """ + + def generate_config(value: Any) -> JsonDict: + return { + "experimental_features": { + "msc4140_finalised_retention_limit_per_user": value + } + } + + for allowed_value in (0, 1): + _read_config(generate_config(allowed_value)) + + for disallowed_value in (-1, 0.5, "1"): + with self.assertRaises(ConfigError): + _read_config(generate_config(disallowed_value)) + @parameterized.expand( [ [ @@ -239,6 +281,16 @@ def generate_config(value: Any) -> JsonDict: } }, ], + [ + "all_msc4140", + { + "experimental_features": { + "msc4140_max_delayed_events_per_user": 3, + "msc4140_finalised_retention_period": "1d", + "msc4140_finalised_retention_limit_per_user": 10, + } + }, + ], # This has historically worked and this is being added as a regression test ["none", {"experimental_features": None}], ] From ab6b3bf73544b54d5b2c108e622c1d3f3aa84841 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 15 Sep 2026 17:50:41 +0200 Subject: [PATCH 28/47] MSC4140: prune finalised delayed events in single statements Replace the per-user loop in `prune_finalised_delayed_events` with two statements: one DELETE for rows finalised before the retention period, and one window-function DELETE for the per-user limit (`ROW_NUMBER() OVER (PARTITION BY user_localpart ORDER BY finalised_ts DESC)` in a subquery), which both PostgreSQL and Synapse's minimum SQLite support. This also replaces the per-user `DELETE ... ORDER BY ... LIMIT`, which PostgreSQL rejects. Pruning now runs only from the 5-minute looping call on the process running background tasks; nothing prunes on lookups. Tests advance the reactor past the prune interval and check that rows past the period, or beyond the per-user limit, disappear (404) while the others stay, on both SQLite and PostgreSQL. --- .../storage/databases/main/delayed_events.py | 92 +++++++----------- tests/rest/client/test_delayed_events.py | 97 +++++++++++++++++++ 2 files changed, 131 insertions(+), 58 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 75cc9ea4494..92e7435c16c 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -360,76 +360,52 @@ async def prune_finalised_delayed_events( retention_period: int, retention_limit: int, ) -> None: - def prune_finalised_delayed_events(txn: LoggingTransaction) -> None: - self._prune_expired_finalised_delayed_events( - txn, current_ts, retention_period - ) + """ + Deletes finalised delayed events that are past their retention. + + Args: + current_ts: The current timestamp. + retention_period: How long (in milliseconds) after finalisation + a finalised delayed event is retained. + retention_limit: How many finalised delayed events are retained + per user. The most recently finalised ones are kept. + """ + def prune_finalised_delayed_events_txn(txn: LoggingTransaction) -> None: txn.execute( """ - SELECT DISTINCT(user_localpart) - FROM delayed_events + DELETE FROM delayed_events WHERE finalised_ts IS NOT NULL - """ + AND finalised_ts < ? + """, + (current_ts - retention_period,), ) - for [user_localpart] in txn.fetchall(): - self._prune_excess_finalised_delayed_events_for_user( - txn, user_localpart, retention_limit - ) - await self.db_pool.runInteraction( - "prune_finalised_delayed_events", prune_finalised_delayed_events - ) - - def _prune_expired_finalised_delayed_events( - self, txn: LoggingTransaction, current_ts: Timestamp, retention_period: int - ) -> None: - """ - Delete all finalised delayed events that had finalised - before the end of the given retention period. - """ - txn.execute( - """ - DELETE FROM delayed_events - WHERE ? - finalised_ts > ? - """, - ( - current_ts, - retention_period, - ), - ) - - def _prune_excess_finalised_delayed_events_for_user( - self, txn: LoggingTransaction, user_localpart: str, retention_limit: int - ) -> None: - """ - Delete the oldest finalised delayed events for the given user, - such that no more of them remain than the given retention limit. - """ - txn.execute( - """ - SELECT COUNT(*) FROM delayed_events - WHERE user_localpart = ? - AND finalised_ts IS NOT NULL - """, - (user_localpart,), - ) - num_existing: int = txn.fetchall()[0][0] - if num_existing > retention_limit: + # FIXME: Remove "AS subquery" after dropping support for PostgreSQL <16 txn.execute( """ DELETE FROM delayed_events - WHERE user_localpart = ? - AND finalised_ts IS NOT NULL - ORDER BY finalised_ts - LIMIT ? + WHERE delay_id IN ( + SELECT delay_id FROM ( + SELECT + delay_id, + ROW_NUMBER() OVER ( + PARTITION BY user_localpart + ORDER BY finalised_ts DESC + ) AS finalised_rank + FROM delayed_events + WHERE finalised_ts IS NOT NULL + ) AS subquery + WHERE finalised_rank > ? + ) """, - ( - user_localpart, - num_existing - retention_limit, - ), + (retention_limit,), ) + await self.db_pool.runInteraction( + "prune_finalised_delayed_events", prune_finalised_delayed_events_txn + ) + async def get_delayed_event_for_user( self, delay_id: str, diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 4ecc6962dde..1f11f4e35c3 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -446,6 +446,103 @@ def schedule(delay_ms: int) -> FakeChannel: assert retry_after_headers self.assertEqual(5, int(retry_after_headers[0])) + @unittest.override_config( + {"experimental_features": {"msc4140_finalised_retention_period": "1h"}} + ) + def test_finalised_delayed_events_are_pruned_after_retention_period( + self, + ) -> None: + # Disable rate-limits for this user, as this test makes many lookups + self.get_success( + self.hs.get_datastores().main.set_ratelimit_for_user( + self.user1_user_id, 0, 0 + ) + ) + + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 900), + {}, + self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + + self.reactor.advance(1) + event_id = self._find_sent_delayed_event( + self.user1_access_token, delay_id, True + ) + self._assert_finalised(delay_id, event_id=event_id) + + # The finalised delayed event must survive prune runs within its retention period + self.reactor.advance(Duration(minutes=30).as_secs()) + self._assert_finalised(delay_id, event_id=event_id) + + # ...but not the first prune run after it + self.reactor.advance(Duration(minutes=30).as_secs()) + channel = self.make_request( + "GET", + f"{PATH_PREFIX}/{delay_id}", + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, channel.result) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"], channel.result) + + @unittest.override_config( + {"experimental_features": {"msc4140_finalised_retention_limit_per_user": 2}} + ) + def test_finalised_delayed_events_are_pruned_beyond_retention_limit( + self, + ) -> None: + # Disable rate-limits for these users, as this test makes many lookups + for user_id in (self.user1_user_id, self.user2_user_id): + self.get_success( + self.hs.get_datastores().main.set_ratelimit_for_user(user_id, 0, 0) + ) + + def schedule_and_cancel(access_token: str) -> str: + channel = self.make_request( + "POST", + _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000), + {}, + access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body.get("delay_id") + assert delay_id is not None + channel = self._update_delayed_event(delay_id, "cancel", True) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + # Space out the finalisation times, so that their order is unambiguous + self.reactor.advance(1) + return delay_id + + # Finalise one more delayed event than the limit for user1, + # and one for user2 to check that the limit applies per user + user2_delay_id = schedule_and_cancel(self.user2_access_token) + user1_delay_ids = [ + schedule_and_cancel(self.user1_access_token) for _ in range(3) + ] + + # Nothing is pruned until the next prune run + for delay_id in user1_delay_ids: + self._assert_finalised(delay_id) + self._assert_finalised(user2_delay_id, access_token=self.user2_access_token) + + self.reactor.advance(Duration(minutes=5).as_secs()) + + # Only the oldest finalised delayed event of user1 is pruned + channel = self.make_request( + "GET", + f"{PATH_PREFIX}/{user1_delay_ids[0]}", + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, channel.result) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"], channel.result) + for delay_id in user1_delay_ids[1:]: + self._assert_finalised(delay_id) + self._assert_finalised(user2_delay_id, access_token=self.user2_access_token) + def test_get_delayed_events_auth(self) -> None: channel = self.make_request("GET", PATH_PREFIX) self.assertEqual(HTTPStatus.UNAUTHORIZED, channel.code, channel.result) From f988e919d98aa4b9b310f8faf5463e9f5e9634c4 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 28 Sep 2026 12:49:11 -0400 Subject: [PATCH 29/47] Fix test for usage with room v12 creators --- tests/rest/client/test_delayed_events.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 1334e331d36..8c9cdfcfc1f 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -862,7 +862,7 @@ def test_send_delayed_state_event_fails_and_stays_scheduled_until_timeout( self.helper.send_state( self.room_id, "m.room.power_levels", - {"users": {self.user1_user_id: 100}, "events": {_EVENT_TYPE: 50}}, + {"events": {_EVENT_TYPE: 50}}, self.user1_access_token, ) @@ -885,7 +885,7 @@ def test_send_delayed_state_event_fails_and_stays_scheduled_until_timeout( self.helper.send_state( self.room_id, "m.room.power_levels", - {"users": {self.user1_user_id: 100}, "events": {_EVENT_TYPE: 0}}, + {"events": {_EVENT_TYPE: 0}}, self.user1_access_token, ) self.reactor.advance(2) From 7c041c197a78e250ece029d9e71c19986fb94bac Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 28 Sep 2026 13:22:09 -0400 Subject: [PATCH 30/47] Return None, not tuple with None, for re-sending Do this to keep typechecking for non-None delayed event details when a result is to be returned. Also skip looking up the next send time on re-send, as recent changes have the caller ignore it in that case. --- synapse/handlers/delayed_events.py | 5 ++-- .../storage/databases/main/delayed_events.py | 30 +++++++++++-------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 8cd7cb247d7..3ec025aa30d 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -525,10 +525,11 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: await self._mgmt_ratelimit(request) await make_deferred_yieldable(self._initialized_from_db) - event, next_send_ts = await self._store.process_target_delayed_event(delay_id) - if event is None: + ret = await self._store.process_target_delayed_event(delay_id) + if ret is None: return + event, next_send_ts = ret if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index c00372f120c..7feb11ba45b 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -586,17 +586,20 @@ def process_timeout_delayed_events_txn( async def process_target_delayed_event( self, delay_id: str, - ) -> tuple[ - DelayedEventDetails | None, - Timestamp | None, - ]: + ) -> ( + tuple[ + DelayedEventDetails, + Timestamp | None, + ] + | None + ): """ Marks for processing the matching delayed event, regardless of its timeout time, as long as it has not already been marked as such. - Returns: The details of the matching delayed event, - and the send time of the next delayed event to be sent, if any. - The details are None if the delayed event has already been sent. + Returns: A tuple of the details of the matching delayed event, + and the send time of the next delayed event to be sent, if any; + or None if the delayed event has already been sent. Raises: NotFoundError: if there is no matching delayed event. @@ -605,10 +608,13 @@ async def process_target_delayed_event( def process_target_delayed_event_txn( txn: LoggingTransaction, - ) -> tuple[ - DelayedEventDetails | None, - Timestamp | None, - ]: + ) -> ( + tuple[ + DelayedEventDetails, + Timestamp | None, + ] + | None + ): txn.execute( """ UPDATE delayed_events @@ -646,7 +652,7 @@ def process_target_delayed_event_txn( HTTPStatus.CONFLICT, "Delayed event has already been cancelled", ) - return None, self._get_next_delayed_event_send_ts_txn(txn) + return None event = DelayedEventDetails( RoomID.from_string(row[0]), From fa635ea1172115d7b4bc9fa74a3dda1191afee1e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 28 Sep 2026 13:47:44 -0400 Subject: [PATCH 31/47] Skip looking up next send time on re-cancel Keep the recent bugfix where re-cancelling a delayed event would disarm the timer for the next delayed event, but so do in a way that doesn't require looking up the next send time. --- synapse/handlers/delayed_events.py | 2 ++ synapse/storage/databases/main/delayed_events.py | 10 ++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 3ec025aa30d..17e71b15837 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -457,6 +457,8 @@ async def cancel(self, request: SynapseRequest, delay_id: str) -> None: next_send_ts = await self._store.cancel_delayed_event( delay_id, self._get_current_ts() ) + if next_send_ts is False: + return if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 7feb11ba45b..e25c6f9b20f 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -14,7 +14,7 @@ import logging from http import HTTPStatus -from typing import TYPE_CHECKING, NewType +from typing import TYPE_CHECKING, Literal, NewType import attr @@ -674,11 +674,12 @@ def process_target_delayed_event_txn( async def cancel_delayed_event( self, delay_id: str, finalised_ts: Timestamp - ) -> Timestamp | None: + ) -> Timestamp | None | Literal[False]: """ Cancels the matching delayed event, i.e. remove it as long as it hasn't been processed. - Returns: The send time of the next delayed event to be sent, if any. + Returns: The send time of the next delayed event to be sent, if any, + or False if the matching delayed event has already been cancelled. Raises: NotFoundError: if there is no matching delayed event. @@ -687,7 +688,7 @@ async def cancel_delayed_event( def cancel_delayed_event_txn( txn: LoggingTransaction, - ) -> Timestamp | None: + ) -> Timestamp | None | Literal[False]: txn.execute( """ UPDATE delayed_events @@ -719,6 +720,7 @@ def cancel_delayed_event_txn( HTTPStatus.CONFLICT, "Delayed event has already been sent", ) + return False return self._get_next_delayed_event_send_ts_txn(txn) return await self.db_pool.runInteraction( From 9bcb87f4458b09ab80a8a750e8acf9a1ce0c0827 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 28 Sep 2026 13:48:58 -0400 Subject: [PATCH 32/47] Replace "pending" with "scheduled" in docstring For consistency with other docstrings --- synapse/storage/databases/main/delayed_events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index e25c6f9b20f..6173c535ab2 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -456,7 +456,7 @@ async def get_all_delayed_events_for_user( user_localpart: str, ) -> list[DelayedEventResponseLegacyCompat]: """ - Return all pending delayed events owned by the given user. + Return all scheduled delayed events owned by the given user. Includes fields from earlier revisions of MSC4140 for compatibility with clients that still expect them. """ From 383280f22f5817a84ebca131c0b0caab3e270e46 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Tue, 29 Sep 2026 10:43:21 +0200 Subject: [PATCH 33/47] MSC4140: index finalised delayed events for the per-user retention prune The per-user retention DELETE ranks finalised rows with `ROW_NUMBER() OVER (PARTITION BY user_localpart ORDER BY finalised_ts DESC)`, which no index matched, so every 5-minute prune sorted all finalised rows. Add a partial background index `delayed_events_finalised_rank` on `(user_localpart, finalised_ts DESC, delay_id ASC) WHERE finalised_ts IS NOT NULL`, registered from the 94/11 delta next to `delayed_events_finalised_ts`, so the window is computed from an index scan instead of a sort. Break ties on `delay_id ASC` so the surviving row is deterministic when rows share a `finalised_ts` (as `cancel_delayed_state_events` produces), and match the outer DELETE on `(user_localpart, delay_id)` so it uses the primary key rather than depending on the `delayed_events_idx` background update. --- synapse/storage/databases/main/delayed_events.py | 15 ++++++++++++--- synapse/storage/schema/__init__.py | 1 + .../delta/94/11_add_finalised_delayed_events.sql | 3 ++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 92e7435c16c..21cf1317a1c 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -129,6 +129,14 @@ def __init__( columns=("finalised_ts",), ) + self.db_pool.updates.register_background_index_update( + update_name="delayed_events_finalised_rank", + index_name="delayed_events_finalised_rank", + table="delayed_events", + columns=("user_localpart", "finalised_ts DESC", "delay_id ASC"), + where_clause="finalised_ts IS NOT NULL", + ) + async def get_delayed_events_stream_pos(self) -> int: """ Gets the stream position of the background process to watch for state events @@ -385,13 +393,14 @@ def prune_finalised_delayed_events_txn(txn: LoggingTransaction) -> None: txn.execute( """ DELETE FROM delayed_events - WHERE delay_id IN ( - SELECT delay_id FROM ( + WHERE (user_localpart, delay_id) IN ( + SELECT user_localpart, delay_id FROM ( SELECT + user_localpart, delay_id, ROW_NUMBER() OVER ( PARTITION BY user_localpart - ORDER BY finalised_ts DESC + ORDER BY finalised_ts DESC, delay_id ASC ) AS finalised_rank FROM delayed_events WHERE finalised_ts IS NOT NULL diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index 76c74a12ff4..47ba2228190 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -179,6 +179,7 @@ - Add an `inserted_ts` column to the `state_groups_persisting` table. - MSC4140: Add columns to the `delayed_events` table to keep track of delayed events that have been sent, cancelled, or failed to be sent due to an error. + - MSC4140: Add an index on `delayed_events` to rank finalised delayed events per user. """ diff --git a/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql index 29df47987f6..c3471e43a74 100644 --- a/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/94/11_add_finalised_delayed_events.sql @@ -17,4 +17,5 @@ ALTER TABLE delayed_events ADD COLUMN finalised_event_id TEXT; ALTER TABLE delayed_events ADD COLUMN finalised_ts BIGINT; INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (9411, 'delayed_events_finalised_ts', '{}'); + (9411, 'delayed_events_finalised_ts', '{}'), + (9411, 'delayed_events_finalised_rank', '{}'); From a87ff94aba48950b6d653713d07bdf458f87ee2c Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 1 Oct 2026 21:21:39 -0400 Subject: [PATCH 34/47] Use attr class for finalised delayed event details --- .../storage/databases/main/delayed_events.py | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 16b4af4521e..c481ef860b4 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -70,6 +70,13 @@ class DelayedEventDetails(EventDetails): user_localpart: UserLocalpart +@attr.s(slots=True, frozen=True, auto_attribs=True) +class FinalisedDetails: + finalised_ts: int + event_id: str | None + error: JsonDict | None = attr.ib(converter=attr.converters.optional(db_to_json)) + + @attr.s(slots=True, frozen=True, auto_attribs=True) class DelayedEventResponse: """The representation of a delayed event in API format.""" @@ -81,9 +88,7 @@ class DelayedEventResponse: delay_ms: int delayed_since_ts: int content: JsonDict = attr.ib(converter=db_to_json) - # Present only for finalised delayed events (MSC4140): `finalised_ts`, plus - # `event_id` if the event was sent, or `error` if sending it failed. - finalised: JsonDict | None = None + finalised: FinalisedDetails | None = None def asdict(self) -> JsonDict: return attr.asdict(self, filter=lambda _attr, v: v is not None) @@ -457,7 +462,7 @@ async def get_delayed_event_for_user( row[3], row[4], row[5], - _make_finalised_dict(row[6], row[7], row[8]), + _make_finalised_details(row[6], row[7], row[8]), ) async def get_all_delayed_events_for_user( @@ -988,23 +993,19 @@ def _generate_delay_id() -> DelayID: return DelayID(f"syd_{stringutils.random_string(20)}") -def _make_finalised_dict( +def _make_finalised_details( finalised_ts: int | None, finalised_event_id: str | None, finalised_error: str | None, -) -> JsonDict | None: +) -> FinalisedDetails | None: """ Builds the `finalised` object of a delayed event's API representation from its stored finalisation columns, or returns None if it is not finalised. """ if finalised_ts is None: return None - finalised: JsonDict = {"finalised_ts": finalised_ts} - if finalised_event_id is not None: - finalised["event_id"] = finalised_event_id - elif finalised_error is not None: - finalised["error"] = db_to_json(finalised_error) - return finalised + else: + return FinalisedDetails(finalised_ts, finalised_event_id, finalised_error) def _generate_cancelled_by_state_update_json() -> str: From f85a1c3b0c390a74748d28bf0e8ccbfb39002a5c Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 1 Oct 2026 21:22:43 -0400 Subject: [PATCH 35/47] Use simple_select_one --- .../storage/databases/main/delayed_events.py | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index c481ef860b4..e89f51ded1b 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -432,28 +432,29 @@ async def get_delayed_event_for_user( Raises: NotFoundError: if there is no matching delayed event. """ - rows = await self.db_pool.execute( - "get_delayed_event_for_user", - """ - SELECT - room_id, - event_type, - state_key, - delay, - send_ts - delay, - content, - finalised_ts, - finalised_event_id, - finalised_error - FROM delayed_events - WHERE delay_id = ? AND user_localpart = ? - """, - delay_id, - user_localpart, + row = await self.db_pool.simple_select_one( + table="delayed_events", + keyvalues={ + "delay_id": delay_id, + "user_localpart": user_localpart, + }, + retcols=( + "room_id", + "event_type", + "state_key", + "delay", + "send_ts - delay", + "content", + "finalised_ts", + "finalised_event_id", + "finalised_error", + ), + allow_none=True, + desc="get_delayed_event_for_user", ) - if not rows: + if row is None: raise NotFoundError("Delayed event not found") - row = rows[0] + return DelayedEventResponse( delay_id, row[0], From 9f852b85980ab72d70adebcca03c7a3b47b5078e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 1 Oct 2026 21:22:56 -0400 Subject: [PATCH 36/47] Use named parameters --- synapse/storage/databases/main/delayed_events.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index e89f51ded1b..5cddf65ef52 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -456,14 +456,14 @@ async def get_delayed_event_for_user( raise NotFoundError("Delayed event not found") return DelayedEventResponse( - delay_id, - row[0], - row[1], - row[2], - row[3], - row[4], - row[5], - _make_finalised_details(row[6], row[7], row[8]), + delay_id=delay_id, + room_id=row[0], + type=row[1], + state_key=row[2], + delay_ms=row[3], + delayed_since_ts=row[4], + content=row[5], + finalised=_make_finalised_details(row[6], row[7], row[8]), ) async def get_all_delayed_events_for_user( From b69ffa85d7854d6db4ee98efb49140627b04d53a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 2 Oct 2026 10:57:57 -0400 Subject: [PATCH 37/47] Remove documentation for experimental configs Will move these to another PR --- .../configuration/config_documentation.md | 24 ------------- schema/synapse-config.schema.yaml | 34 ------------------- 2 files changed, 58 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index b95045ddf8b..d875337c0ea 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -856,30 +856,6 @@ Example configuration: max_event_delay_duration: 24h ``` --- -### `experimental_features` - -*(object)* Options for experimental features. Only the options for delayed events, as per [MSC4140](https://github.com/matrix-org/matrix-spec-proposals/pull/4140), are documented here. - -This setting has the following sub-options: - -* `msc4140_max_delayed_events_per_user` (integer): The maximum number of delayed events a user may have scheduled at a time. Must be a non-negative integer. If 0, sending of delayed events is disallowed. Defaults to `100`. - -* `msc4140_finalised_retention_period` (duration): How long a delayed event is retained for lookup after it has been finalised, i.e. sent, cancelled, or failed to be sent. Must be a non-negative duration. If 0, finalised delayed events are not retained. - - Finalised delayed events past their retention are pruned every 5 minutes. - - Defaults to `"7d"`. - -* `msc4140_finalised_retention_limit_per_user` (integer): The maximum number of finalised delayed events retained per user, keeping the most recently finalised ones. Must be a non-negative integer. If 0, finalised delayed events are not retained. Defaults to `1000`. - -Example configuration: -```yaml -experimental_features: - msc4140_max_delayed_events_per_user: 100 - msc4140_finalised_retention_period: 7d - msc4140_finalised_retention_limit_per_user: 1000 -``` ---- ### `user_types` *(object)* Configuration settings related to the user types feature. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index 021fc72ff02..1a929912a34 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -1059,40 +1059,6 @@ properties: default: null examples: - 24h - experimental_features: - type: object - description: >- - Options for experimental features. Only the options for delayed events, as per - [MSC4140](https://github.com/matrix-org/matrix-spec-proposals/pull/4140), - are documented here. - properties: - msc4140_max_delayed_events_per_user: - type: integer - description: >- - The maximum number of delayed events a user may have scheduled at a time. - Must be a non-negative integer. If 0, sending of delayed events is disallowed. - default: 100 - msc4140_finalised_retention_period: - $ref: "#/$defs/duration" - description: >- - How long a delayed event is retained for lookup after it has been finalised, - i.e. sent, cancelled, or failed to be sent. Must be a non-negative duration. - If 0, finalised delayed events are not retained. - - - Finalised delayed events past their retention are pruned every 5 minutes. - default: 7d - msc4140_finalised_retention_limit_per_user: - type: integer - description: >- - The maximum number of finalised delayed events retained per user, keeping - the most recently finalised ones. Must be a non-negative integer. - If 0, finalised delayed events are not retained. - default: 1000 - examples: - - msc4140_max_delayed_events_per_user: 100 - msc4140_finalised_retention_period: 7d - msc4140_finalised_retention_limit_per_user: 1000 user_types: type: object description: >- From 40dd2db452902bd9e678451a25c4968282765acf Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 2 Oct 2026 11:51:19 -0400 Subject: [PATCH 38/47] Move finalised retention config to experimental.py - Keep only the configs that control enablement of MSC4140 in server.py, and add a short comment explaining that - Tweak the comments preceding the finalised retention configs to better serve as documentation for them - Move the tests for these configs to test_experimental.py --- synapse/config/experimental.py | 33 ++++++++++++++++++ synapse/config/server.py | 31 ++--------------- synapse/handlers/delayed_events.py | 4 +-- tests/config/test_experimental.py | 55 +++++++++++++++++++++++++++++- tests/config/test_server.py | 52 ---------------------------- 5 files changed, 91 insertions(+), 84 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 1c2f021322b..ad59f4e25e3 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -29,6 +29,7 @@ from synapse.config import ConfigError from synapse.config._base import Config, read_file from synapse.types import JsonDict, StrSequence +from synapse.util.duration import Duration @cache @@ -247,6 +248,38 @@ def read_config( # MSC4133: Custom profile fields self.msc4133_enabled: bool = experimental.get("msc4133_enabled", False) + # The portion of MSC4140 related to retention of delayed events for lookup + # after they have been finalised, i.e. sent, cancelled, or failed to be sent. + # Finalised delayed events past their retention are pruned every 5 minutes. + + # How long a finalised delayed event may be retained for. + # 0 disables retention. + finalised_retention_period_ms = self.parse_duration( + experimental.get("msc4140_finalised_retention_period", "7d") + ) + if finalised_retention_period_ms < 0: + raise ConfigError( + "'msc4140_finalised_retention_period' must be a non-negative duration", + ("experimental", "msc4140_finalised_retention_period"), + ) + self.msc4140_finalised_retention_period = Duration( + milliseconds=finalised_retention_period_ms + ) + + # The maximum number of finalised delayed events retained per user. + # 0 disables retention. + self.msc4140_finalised_retention_limit_per_user: int = experimental.get( + "msc4140_finalised_retention_limit_per_user", 1000 + ) + if ( + not isinstance(self.msc4140_finalised_retention_limit_per_user, int) + or self.msc4140_finalised_retention_limit_per_user < 0 + ): + raise ConfigError( + "'msc4140_finalised_retention_limit_per_user' must be a non-negative integer", + ("experimental", "msc4140_finalised_retention_limit_per_user"), + ) + # MSC4143: Matrix RTC Transport using Livekit Backend self.msc4143_enabled: bool = experimental.get("msc4143_enabled", False) diff --git a/synapse/config/server.py b/synapse/config/server.py index dd1222317df..78285ae8c9f 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -994,7 +994,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.max_event_delay_duration = Duration() # The maximum number of delayed events a user may have scheduled at a time. - # (Defined here despite being experimental to be near the other MSC4140 config) + # (Defined here despite being experimental to be near the other MSC4140 config, + # because either config controls whether delayed events are enabled or not) experimental = config.get("experimental_features") or {} self.max_delayed_events_per_user: int = experimental.get( "msc4140_max_delayed_events_per_user", 100 @@ -1008,34 +1009,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: ("experimental", "msc4140_max_delayed_events_per_user"), ) - # How long a finalised delayed event (one that has been sent, cancelled, or - # failed to be sent) is retained for lookup. 0 disables retention. - finalised_retention_period_ms = self.parse_duration( - experimental.get("msc4140_finalised_retention_period", "7d") - ) - if finalised_retention_period_ms < 0: - raise ConfigError( - "'msc4140_finalised_retention_period' must be a non-negative duration", - ("experimental", "msc4140_finalised_retention_period"), - ) - self.msc4140_finalised_retention_period = Duration( - milliseconds=finalised_retention_period_ms - ) - - # The maximum number of finalised delayed events retained per user. - # 0 disables retention. - self.msc4140_finalised_retention_limit_per_user: int = experimental.get( - "msc4140_finalised_retention_limit_per_user", 1000 - ) - if ( - not isinstance(self.msc4140_finalised_retention_limit_per_user, int) - or self.msc4140_finalised_retention_limit_per_user < 0 - ): - raise ConfigError( - "'msc4140_finalised_retention_limit_per_user' must be a non-negative integer", - ("experimental", "msc4140_finalised_retention_limit_per_user"), - ) - self.msc4140_enabled = bool( self.max_delayed_events_per_user and self.max_event_delay_duration ) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 17e71b15837..1b6272458d3 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -339,8 +339,8 @@ async def _handle_state_deltas(self, deltas: list[StateDelta]) -> None: async def _prune_finalised_events(self) -> None: await self._store.prune_finalised_delayed_events( self._get_current_ts(), - self._config.server.msc4140_finalised_retention_period.as_millis(), - self._config.server.msc4140_finalised_retention_limit_per_user, + self._config.experimental.msc4140_finalised_retention_period.as_millis(), + self._config.experimental.msc4140_finalised_retention_limit_per_user, ) async def add( diff --git a/tests/config/test_experimental.py b/tests/config/test_experimental.py index 0fede98ee9d..879a119422f 100644 --- a/tests/config/test_experimental.py +++ b/tests/config/test_experimental.py @@ -12,10 +12,12 @@ # . +from typing import Any + import yaml from parameterized import parameterized -from synapse.config._base import RootConfig +from synapse.config._base import ConfigError, RootConfig from synapse.config.experimental import ExperimentalConfig from synapse.config.homeserver import HomeServerConfig from synapse.types import JsonDict @@ -43,6 +45,15 @@ class ExperimentalConfigTestCase(unittest.TestCase): } }, ], + [ + "msc4140", + { + "experimental_features": { + "msc4140_finalised_retention_period": "1d", + "msc4140_finalised_retention_limit_per_user": 10, + } + }, + ], # This has historically worked and this is being added as a regression test ["none", {"experimental_features": None}], ] @@ -56,6 +67,48 @@ def test_experimental_features_parsing( _read_config(config_values) + def test_finalised_delayed_events_retention_period_enforces_non_negative( + self, + ) -> None: + """ + Test that the configured retention period of finalised delayed events + must be a non-negative duration if set + """ + + def generate_config(value: Any) -> JsonDict: + return { + "experimental_features": {"msc4140_finalised_retention_period": value} + } + + for allowed_value in (0, 1, "7d"): + _read_config(generate_config(allowed_value)) + + for disallowed_value in (-1, "-1d", "1x", None): + with self.assertRaises((ConfigError, TypeError, ValueError)): + _read_config(generate_config(disallowed_value)) + + def test_finalised_delayed_events_retention_limit_per_user_enforces_non_negative_int( + self, + ) -> None: + """ + Test that the configured retention limit of finalised delayed events per user + must be a non-negative integer if set + """ + + def generate_config(value: Any) -> JsonDict: + return { + "experimental_features": { + "msc4140_finalised_retention_limit_per_user": value + } + } + + for allowed_value in (0, 1): + _read_config(generate_config(allowed_value)) + + for disallowed_value in (-1, 0.5, "1"): + with self.assertRaises(ConfigError): + _read_config(generate_config(disallowed_value)) + def _read_config(config_values: JsonDict) -> None: ExperimentalConfig(RootConfig()).read_config( diff --git a/tests/config/test_server.py b/tests/config/test_server.py index df3842ded9a..12c57d33da2 100644 --- a/tests/config/test_server.py +++ b/tests/config/test_server.py @@ -253,48 +253,6 @@ def generate_config(limit: bool, require_auth: bool) -> JsonDict: with self.assertRaises(ConfigError): _read_config({"limit_profile_requests_to_users_who_share_rooms": True}) - def test_finalised_delayed_events_retention_period_enforces_non_negative( - self, - ) -> None: - """ - Test that the configured retention period of finalised delayed events - must be a non-negative duration if set, as per documentation - """ - - def generate_config(value: Any) -> JsonDict: - return { - "experimental_features": {"msc4140_finalised_retention_period": value} - } - - for allowed_value in (0, 1, "7d"): - _read_config(generate_config(allowed_value)) - - for disallowed_value in (-1, "-1d", "1x", None): - with self.assertRaises((ConfigError, TypeError, ValueError)): - _read_config(generate_config(disallowed_value)) - - def test_finalised_delayed_events_retention_limit_per_user_enforces_non_negative_int( - self, - ) -> None: - """ - Test that the configured retention limit of finalised delayed events per user - must be a non-negative integer if set, as per documentation - """ - - def generate_config(value: Any) -> JsonDict: - return { - "experimental_features": { - "msc4140_finalised_retention_limit_per_user": value - } - } - - for allowed_value in (0, 1): - _read_config(generate_config(allowed_value)) - - for disallowed_value in (-1, 0.5, "1"): - with self.assertRaises(ConfigError): - _read_config(generate_config(disallowed_value)) - @parameterized.expand( [ [ @@ -305,16 +263,6 @@ def generate_config(value: Any) -> JsonDict: } }, ], - [ - "all_msc4140", - { - "experimental_features": { - "msc4140_max_delayed_events_per_user": 3, - "msc4140_finalised_retention_period": "1d", - "msc4140_finalised_retention_limit_per_user": 10, - } - }, - ], # This has historically worked and this is being added as a regression test ["none", {"experimental_features": None}], ] From 541372e00676903b2e2b1384e313f470cbeee0ac Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 14 Oct 2026 11:54:49 -0400 Subject: [PATCH 39/47] Drop special case for state events in bulk timeout When a batch of timed out delayed events includes state events, simply send those state events in order with all other events to be sent, rather than sending only the first state event per matching state key & doing so after sending message events. This is safe now that delayed state event intererence has been dropped (see #19495). --- synapse/handlers/delayed_events.py | 33 ++--------------- .../storage/databases/main/delayed_events.py | 37 ------------------- 2 files changed, 3 insertions(+), 67 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 07e0befbfba..1d6de3921d4 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -33,14 +33,11 @@ DelayedEventDetails, DelayedEventResponse, DelayedEventResponseLegacyCompat, - EventType, - StateKey, Timestamp, ) from synapse.types import ( JsonDict, Requester, - RoomID, UserID, create_requester, ) @@ -339,30 +336,9 @@ async def _send_on_timeout(self) -> None: await self._send_events(events) async def _send_events(self, events: list[DelayedEventDetails]) -> None: - sent_state: set[tuple[RoomID, EventType, StateKey]] = set() + # TODO: parallelize, but per room to preserve send order for event in events: - if event.state_key is not None: - state_info = (event.room_id, event.type, event.state_key) - if state_info in sent_state: - continue - else: - state_info = None - try: - # TODO: send in background if message event or non-conflicting state event - finalised_ts = await self._send_event(event, True) - if state_info is not None: - sent_state.add(state_info) - except Exception: - logger.exception("Failed to send delayed event") - finalised_ts = self._get_current_ts() - - for room_id, event_type, state_key in sent_state: - await self._store.finalise_processed_delayed_state_events( - room_id=str(room_id), - event_type=event_type, - state_key=state_key, - finalised_ts=finalised_ts, - ) + await self._send_event(event, True) def _schedule_next_at_or_none(self, next_send_ts: Timestamp | None) -> None: if next_send_ts is not None: @@ -419,7 +395,7 @@ async def _send_event( self, event: DelayedEventDetails, finalise_error: bool, - ) -> Timestamp: + ) -> None: """ Sends the given delayed event, which must have been marked as processed, and finalises it in the DB. @@ -431,8 +407,6 @@ async def _send_event( unmarked as processed, so that it stays scheduled, and the error is re-raised. - Returns: The timestamp at which the delayed event was finalised. - Raises: Exception: whatever sending the event raised, if finalise_error is False. """ @@ -545,7 +519,6 @@ async def _send_event( if send_error is None: set_tag("event_id", event_id) - return finalised_ts def _get_current_ts(self) -> Timestamp: return Timestamp(self._clock.time_msec()) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 15a9fa5a1ea..32c956d7e6f 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -805,43 +805,6 @@ def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: finalise_processed_delayed_event_txn, ) - async def finalise_processed_delayed_state_events( - self, - *, - room_id: str, - event_type: str, - state_key: str, - finalised_ts: Timestamp, - ) -> None: - """ - Finalise the matching delayed state events that have been marked as processed. - """ - - def finalise_processed_delayed_state_events(txn: LoggingTransaction) -> None: - txn.execute( - """ - UPDATE delayed_events - SET - finalised_error = ?, - finalised_ts = ? - WHERE room_id = ? AND event_type = ? AND state_key = ? - AND is_processed - AND finalised_ts IS NULL - """, - ( - _generate_cancelled_by_state_update_json(), - finalised_ts, - room_id, - event_type, - state_key, - ), - ) - - await self.db_pool.runInteraction( - "finalise_processed_delayed_state_events", - finalise_processed_delayed_state_events, - ) - async def unprocess_delayed_event(self, delay_id: DelayID) -> Timestamp | None: """ Unmark the matching delayed event for processing, so that it is scheduled again. From b2d853b61db499ee7cfb8976d4b9626700388b38 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 14 Oct 2026 14:20:04 -0400 Subject: [PATCH 40/47] Send timed out events concurrently per room When sending a batch of delayed events at their scheduled send time, allow concurrent sending of events that target different rooms. --- synapse/handlers/delayed_events.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 1d6de3921d4..c30977a82db 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -41,6 +41,7 @@ UserID, create_requester, ) +from synapse.util.async_helpers import yieldable_gather_results from synapse.util.duration import Duration from synapse.util.events import generate_fake_event_id @@ -336,9 +337,15 @@ async def _send_on_timeout(self) -> None: await self._send_events(events) async def _send_events(self, events: list[DelayedEventDetails]) -> None: - # TODO: parallelize, but per room to preserve send order + events_by_room: dict[str, list[DelayedEventDetails]] = {} for event in events: - await self._send_event(event, True) + events_by_room.setdefault(str(event.room_id), []).append(event) + + async def send_events(events: list[DelayedEventDetails]) -> None: + for event in events: + await self._send_event(event, True) + + await yieldable_gather_results(send_events, events_by_room.values()) def _schedule_next_at_or_none(self, next_send_ts: Timestamp | None) -> None: if next_send_ts is not None: From 2eaf104bb131812f78f64bf3abfdea827020a558 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 15 Oct 2026 14:32:31 -0400 Subject: [PATCH 41/47] Refine tiebreakers for finalised event pruning When breaking ties on which finalised delayed events to prune, prefer to prune those that had been scheduled earlier (i.e. ones with the lowest `delayed_since_ts`, i.e. `send_ts - delay`), followed by those with an earlier intended send time. --- synapse/storage/databases/main/delayed_events.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 32c956d7e6f..bfc97bb1fdc 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -138,7 +138,13 @@ def __init__( update_name="delayed_events_finalised_rank", index_name="delayed_events_finalised_rank", table="delayed_events", - columns=("user_localpart", "finalised_ts DESC", "delay_id ASC"), + columns=( + "user_localpart", + "finalised_ts DESC", + "(send_ts - delay) DESC", + "send_ts DESC", + "delay_id ASC", + ), where_clause="finalised_ts IS NOT NULL", ) @@ -370,7 +376,11 @@ def prune_finalised_delayed_events_txn(txn: LoggingTransaction) -> None: delay_id, ROW_NUMBER() OVER ( PARTITION BY user_localpart - ORDER BY finalised_ts DESC, delay_id ASC + ORDER BY + finalised_ts DESC, + (send_ts - delay) DESC, + send_ts DESC, + delay_id ASC ) AS finalised_rank FROM delayed_events WHERE finalised_ts IS NOT NULL From d38e30c25a28908bf07980dde0e50b752d4ecef3 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 15 Oct 2026 21:11:01 -0400 Subject: [PATCH 42/47] Prune/erase finalised events immediately Instead of pruning finalised delayed events on a 5 minute timer, prune them as soon as they exceed retention policies. Also make user deactivation finalise cancelled delayed events instead of deleting them, unless the deactivation requests to erase user data. --- synapse/config/experimental.py | 10 + synapse/handlers/deactivate_account.py | 2 +- synapse/handlers/delayed_events.py | 92 +++++-- .../storage/databases/main/delayed_events.py | 245 +++++++++++++----- tests/rest/client/test_delayed_events.py | 118 ++++++--- 5 files changed, 357 insertions(+), 110 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index cc1f0bd1adb..bb59126b741 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -283,6 +283,16 @@ def read_config( ("experimental", "msc4140_finalised_retention_limit_per_user"), ) + # Since setting either config to 0 disables finalised retention, + # set both to 0 if either of them are 0, so that it doesn't make + # a difference which of them had been explicitly set to 0. + if ( + not self.msc4140_finalised_retention_period + or not self.msc4140_finalised_retention_limit_per_user + ): + self.msc4140_finalised_retention_period = Duration() + self.msc4140_finalised_retention_limit_per_user = 0 + # MSC4143: Matrix RTC Transport using Livekit Backend self.msc4143_enabled: bool = experimental.get("msc4143_enabled", False) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index a1aa2a51dcc..0ec4fa2ddd7 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -119,7 +119,7 @@ async def deactivate_account( # Cancel the user's delayed events (MSC4140). Do this first, so that none # of them get sent while the rest of the deactivation is in progress. await self._delayed_events_handler.cancel_all_for_user( - get_localpart_from_id(user_id) + get_localpart_from_id(user_id), erase_data ) # FIXME: Theoretically there is a race here wherein user resets diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index c30977a82db..d5142b21fad 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -24,7 +24,6 @@ from synapse.config.workers import MAIN_PROCESS_INSTANCE_NAME from synapse.logging.context import make_deferred_yieldable from synapse.logging.opentracing import set_tag -from synapse.metrics.background_process_metrics import wrap_as_background_process from synapse.replication.http.delayed_events import ( ReplicationAddedDelayedEventRestServlet, ReplicationCancelDelayedEventsForUserRestServlet, @@ -74,11 +73,22 @@ def __init__(self, hs: "HomeServer"): ) self._next_delayed_event_call: Optional[IDelayedCall] = None + self._next_prune_finalised_call: Optional[IDelayedCall] = None if hs.config.worker.worker_app is None: self._repl_client = None async def _schedule_db_events() -> None: + # Before sending any more delayed events, prune all finalised ones + # (and kick off the timer for the next age-based pruning). + min_finalised_ts = await self._store.prune_finalised_delayed_events( + self._get_current_ts(), + self._config.experimental.msc4140_finalised_retention_period, + self._config.experimental.msc4140_finalised_retention_limit_per_user, + ) + if min_finalised_ts is not None: + self._schedule_prune_finalised(min_finalised_ts) + # Process any delayed events that are due to be sent. # # We set `reprocess_events` to True in case any events had been @@ -112,23 +122,50 @@ async def _schedule_db_events() -> None: ReplicationCancelDelayedEventsForUserRestServlet.make_client(hs) ) - if hs.config.worker.run_background_tasks: - self._clock.looping_call( - self._prune_finalised_events, - Duration(minutes=5), - ) - @property def _is_master(self) -> bool: return self._repl_client is None - @wrap_as_background_process("_prune_finalised_events") - async def _prune_finalised_events(self) -> None: - await self._store.prune_finalised_delayed_events( + async def _prune_finalised_by_age(self) -> None: + self._next_prune_finalised_call = None + + min_finalised_ts = await self._store.prune_finalised_delayed_events_by_age( self._get_current_ts(), - self._config.experimental.msc4140_finalised_retention_period.as_millis(), - self._config.experimental.msc4140_finalised_retention_limit_per_user, + self._config.experimental.msc4140_finalised_retention_period, ) + if min_finalised_ts is not None: + self._schedule_prune_finalised(min_finalised_ts) + + def _schedule_prune_finalised_or_none( + self, min_finalised_ts: Timestamp | None + ) -> None: + if min_finalised_ts is not None: + self._schedule_prune_finalised(min_finalised_ts) + elif self._next_prune_finalised_call is not None: + self._next_prune_finalised_call.cancel() + self._next_prune_finalised_call = None + + def _schedule_prune_finalised(self, min_finalised_ts: Timestamp) -> None: + current_ts = self._get_current_ts() + min_finalised_age_ms = current_ts - min_finalised_ts + delay = max( + self._config.experimental.msc4140_finalised_retention_period + - Duration(milliseconds=min_finalised_age_ms), + Duration(), + ) + + if self._next_prune_finalised_call is None: + self._next_prune_finalised_call = self._clock.call_later( + delay, + self.hs.run_as_background_process, + "_prune_finalised_by_age", + self._prune_finalised_by_age, + ) + elif ( + self._next_prune_finalised_call.getTime() * 1000 + < current_ts + delay.as_millis() + ): + self._next_prune_finalised_call.reset(delay.as_secs()) async def add( self, @@ -234,18 +271,23 @@ async def cancel(self, requester: Requester, delay_id: str) -> None: await self._delayed_event_mgmt_ratelimiter.ratelimit(requester) await make_deferred_yieldable(self._initialized_from_db) - next_send_ts = await self._store.cancel_delayed_event( + ret = await self._store.cancel_delayed_event( delay_id=delay_id, user_localpart=requester.user.localpart, finalised_ts=self._get_current_ts(), + retention_limit=self._config.experimental.msc4140_finalised_retention_limit_per_user, ) - if next_send_ts is False: + if ret is None: return + next_send_ts, min_finalised_ts = ret if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) + self._schedule_prune_finalised_or_none(min_finalised_ts) - async def cancel_all_for_user(self, user_localpart: str) -> None: + async def cancel_all_for_user( + self, user_localpart: str, erase_data: bool = False + ) -> None: """ Cancels the scheduled delivery of all delayed events owned by the local user with the given localpart, e.g. because their account is being deactivated. @@ -254,6 +296,11 @@ async def cancel_all_for_user(self, user_localpart: str) -> None: Goes through replication if this is not the main process, as only the main process handles sending delayed events. + + Args: + user_localpart: The localpart of the local user whose delayed events to cancel. + erase_data: Whether to completely erase cancelled delayed events, rather than + retaining them as finalised delayed events that the user may query later. """ if not self._is_master: await self._cancel_all_for_user_client( @@ -264,10 +311,18 @@ async def cancel_all_for_user(self, user_localpart: str) -> None: await make_deferred_yieldable(self._initialized_from_db) - next_send_ts = await self._store.cancel_delayed_events_for_user(user_localpart) + ret = await self._store.cancel_delayed_events_for_user( + user_localpart, + self._get_current_ts(), + self._config.experimental.msc4140_finalised_retention_limit_per_user + if not erase_data + else 0, + ) + next_send_ts, min_finalised_ts = ret if self._next_send_ts_changed(next_send_ts): self._schedule_next_at_or_none(next_send_ts) + self._schedule_prune_finalised_or_none(min_finalised_ts) async def restart(self, requester: Requester, delay_id: str) -> None: """ @@ -516,11 +571,14 @@ async def _send_event( if finalised_ts is None: finalised_ts = self._get_current_ts() try: - await self._store.finalise_processed_delayed_event( + min_finalised_ts = await self._store.finalise_processed_delayed_event( event.delay_id, + event.user_localpart, send_error if send_error is not None else event_id, finalised_ts, + self._config.experimental.msc4140_finalised_retention_limit_per_user, ) + self._schedule_prune_finalised_or_none(min_finalised_ts) except Exception: logger.exception("Failed to finalise processed delayed event") diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index bfc97bb1fdc..f99cbf4913c 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -14,7 +14,7 @@ import logging from http import HTTPStatus -from typing import TYPE_CHECKING, Literal, NewType +from typing import TYPE_CHECKING, NewType import attr @@ -341,60 +341,115 @@ def restart_delayed_event_txn( async def prune_finalised_delayed_events( self, current_ts: Timestamp, - retention_period: int, + retention_period: Duration, retention_limit: int, - ) -> None: + ) -> Timestamp | None: """ Deletes finalised delayed events that are past their retention. Args: current_ts: The current timestamp. - retention_period: How long (in milliseconds) after finalisation - a finalised delayed event is retained. + retention_period: How long after finalisation a finalised delayed event is retained. retention_limit: How many finalised delayed events are retained per user. The most recently finalised ones are kept. + + Returns: The finalised time of the oldest remaining finalised delayed event, if any. """ - def prune_finalised_delayed_events_txn(txn: LoggingTransaction) -> None: - txn.execute( - """ - DELETE FROM delayed_events - WHERE finalised_ts IS NOT NULL - AND finalised_ts < ? - """, - (current_ts - retention_period,), + def prune_finalised_delayed_events_txn( + txn: LoggingTransaction, + ) -> Timestamp | None: + self._prune_finalised_delayed_events_by_age_txn( + txn, current_ts, retention_period ) - - # FIXME: Remove "AS subquery" after dropping support for PostgreSQL <16 - txn.execute( - """ - DELETE FROM delayed_events - WHERE (user_localpart, delay_id) IN ( - SELECT user_localpart, delay_id FROM ( - SELECT - user_localpart, - delay_id, - ROW_NUMBER() OVER ( - PARTITION BY user_localpart - ORDER BY - finalised_ts DESC, - (send_ts - delay) DESC, - send_ts DESC, - delay_id ASC - ) AS finalised_rank - FROM delayed_events - WHERE finalised_ts IS NOT NULL - ) AS subquery - WHERE finalised_rank > ? - ) - """, - (retention_limit,), + self._prune_finalised_delayed_events_by_limit_txn( + txn, None, retention_limit ) + return self._get_min_finalised_ts_txn(txn) - await self.db_pool.runInteraction( + return await self.db_pool.runInteraction( "prune_finalised_delayed_events", prune_finalised_delayed_events_txn ) + async def prune_finalised_delayed_events_by_age( + self, + current_ts: Timestamp, + retention_period: Duration, + ) -> Timestamp | None: + """ + Deletes finalised delayed events that are past their allowed retention period. + + Args: + current_ts: The current timestamp. + retention_period: How long after finalisation a finalised delayed event is retained. + + Returns: The finalised time of the oldest remaining finalised delayed event, if any. + """ + + def prune_finalised_delayed_events_by_age_txn( + txn: LoggingTransaction, + ) -> Timestamp | None: + self._prune_finalised_delayed_events_by_age_txn( + txn, current_ts, retention_period + ) + return self._get_min_finalised_ts_txn(txn) + + return await self.db_pool.runInteraction( + "prune_finalised_delayed_events_by_age", + prune_finalised_delayed_events_by_age_txn, + ) + + def _prune_finalised_delayed_events_by_age_txn( + self, + txn: LoggingTransaction, + current_ts: Timestamp, + retention_period: Duration, + ) -> None: + txn.execute( + "DELETE FROM delayed_events WHERE finalised_ts < ?", + (current_ts - retention_period.as_millis(),), + ) + + def _prune_finalised_delayed_events_by_limit_txn( + self, + txn: LoggingTransaction, + user_localpart: str | None, + retention_limit: int, + ) -> None: + sql_args: tuple = (retention_limit,) + if user_localpart is not None: + sql_partition = "" + sql_and_where = "AND user_localpart = ?" + sql_args = (user_localpart, *sql_args) + else: + sql_partition = "PARTITION BY user_localpart" + sql_and_where = "" + + # FIXME: Remove "AS subquery" after dropping support for PostgreSQL <16 + txn.execute( + f""" + DELETE FROM delayed_events + WHERE (user_localpart, delay_id) IN ( + SELECT user_localpart, delay_id FROM ( + SELECT + *, + ROW_NUMBER() OVER ( + {sql_partition} + ORDER BY + finalised_ts DESC, + (send_ts - delay) DESC, + send_ts DESC, + delay_id ASC + ) AS finalised_rank + FROM delayed_events + WHERE finalised_ts IS NOT NULL {sql_and_where} + ) AS subquery + WHERE finalised_rank > ? + ) + """, + sql_args, + ) + async def get_delayed_event_for_user( self, delay_id: str, @@ -675,17 +730,27 @@ async def cancel_delayed_event( delay_id: str, user_localpart: str, finalised_ts: Timestamp, - ) -> Timestamp | None | Literal[False]: + retention_limit: int, + ) -> ( + tuple[ + Timestamp | None, + Timestamp | None, + ] + | None + ): """ - Cancels the matching delayed event owned by the given user, - i.e. remove it as long as it hasn't been processed. + Cancels the matching scheduled delayed event owned by the given user. Args: delay_id: The ID of the delayed event to cancel. user_localpart: The localpart of the user who owns the delayed event. + finalised_ts: The timestamp to use as the finalised time of the delayed event. + retention_limit: The maximum number of finalised delayed events to store for the given user. + The most recently finalised ones are kept. - Returns: The send time of the next delayed event to be sent, if any, - or False if the matching delayed event has already been cancelled. + Returns: A tuple of the send time of the next delayed event to be sent, if any, + and the finalised time of the oldest remaining finalised delayed event, if any; + or None if the delayed event has already been cancelled. Raises: NotFoundError: if there is no matching delayed event. @@ -694,7 +759,13 @@ async def cancel_delayed_event( def cancel_delayed_event_txn( txn: LoggingTransaction, - ) -> Timestamp | None | Literal[False]: + ) -> ( + tuple[ + Timestamp | None, + Timestamp | None, + ] + | None + ): txn.execute( """ UPDATE delayed_events @@ -729,37 +800,59 @@ def cancel_delayed_event_txn( HTTPStatus.CONFLICT, "Delayed event has already been sent", ) - return False - return self._get_next_delayed_event_send_ts_txn(txn) + return None + + self._prune_finalised_delayed_events_by_limit_txn( + txn, user_localpart, retention_limit + ) + return ( + self._get_next_delayed_event_send_ts_txn(txn), + self._get_min_finalised_ts_txn(txn), + ) return await self.db_pool.runInteraction( "cancel_delayed_event", cancel_delayed_event_txn ) async def cancel_delayed_events_for_user( - self, user_localpart: str - ) -> Timestamp | None: + self, + user_localpart: str, + finalised_ts: Timestamp, + retention_limit: int, + ) -> tuple[Timestamp | None, Timestamp | None]: """ - Cancels all delayed events owned by the given local user, i.e. remove them as long as they haven't been processed. + Cancels all scheduled delayed events owned by the given local user. Args: user_localpart: The localpart of the user whose delayed events to cancel. + finalised_ts: The timestamp to use as the finalised time of all of the cancelled delayed events. + retention_limit: The maximum number of finalised delayed events to store for the given user. + The most recently finalised ones are kept. - Returns: The send time of the next delayed event to be sent, if any. + Returns: The send time of the next delayed event to be sent, if any, + and the finalised time of the oldest remaining finalised delayed event, if any. """ def cancel_delayed_events_for_user_txn( txn: LoggingTransaction, - ) -> Timestamp | None: - self.db_pool.simple_delete_txn( - txn, - table="delayed_events", - keyvalues={ - "user_localpart": user_localpart, - "is_processed": False, - }, + ) -> tuple[Timestamp | None, Timestamp | None]: + txn.execute( + """ + UPDATE delayed_events + SET finalised_ts = ? + WHERE user_localpart = ? + AND NOT is_processed + AND finalised_ts IS NULL + """, + (finalised_ts, user_localpart), + ) + self._prune_finalised_delayed_events_by_limit_txn( + txn, user_localpart, retention_limit + ) + return ( + self._get_next_delayed_event_send_ts_txn(txn), + self._get_min_finalised_ts_txn(txn), ) - return self._get_next_delayed_event_send_ts_txn(txn) return await self.db_pool.runInteraction( "cancel_delayed_events_for_user", cancel_delayed_events_for_user_txn @@ -768,12 +861,24 @@ def cancel_delayed_events_for_user_txn( async def finalise_processed_delayed_event( self, delay_id: DelayID, + user_localpart: str, result_or_error: str | JsonDict, finalised_ts: Timestamp, - ) -> None: + retention_limit: int, + ) -> Timestamp | None: """ Finalise the matching delayed event, as long as it has been marked as processed. + Args: + delay_id: The ID of the delayed event to finalise. + user_localpart: The localpart of the user that owns the delayed event. + finalised_ts: The timestamp to use as the finalised time of the delayed event. + retention_limit: The maximum number of finalised delayed events to store + for the owner of the matching delayed event. + The most recently finalised ones are kept. + + Returns: The finalised time of the oldest remaining finalised delayed event, if any. + Throws: StoreError: if there is no matching delayed event, or if it has not yet been processed. """ @@ -784,7 +889,9 @@ async def finalise_processed_delayed_event( event_id = None send_error = result_or_error - def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: + def finalise_processed_delayed_event_txn( + txn: LoggingTransaction, + ) -> Timestamp | None: table = "delayed_events" txn.execute( f""" @@ -794,6 +901,7 @@ def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: finalised_event_id = ?, finalised_ts = ? WHERE delay_id = ? + AND user_localpart = ? AND is_processed AND finalised_ts IS NULL """, @@ -802,6 +910,7 @@ def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: event_id, finalised_ts, delay_id, + user_localpart, ), ) rowcount = txn.rowcount @@ -810,7 +919,12 @@ def finalise_processed_delayed_event_txn(txn: LoggingTransaction) -> None: if rowcount > 1: raise StoreError(500, "More than one row matched (%s)" % (table,)) - await self.db_pool.runInteraction( + self._prune_finalised_delayed_events_by_limit_txn( + txn, user_localpart, retention_limit + ) + return self._get_min_finalised_ts_txn(txn) + + return await self.db_pool.runInteraction( "finalise_processed_delayed_event", finalise_processed_delayed_event_txn, ) @@ -888,6 +1002,11 @@ def _get_next_delayed_event_send_ts_txn( resp = txn.fetchone() return Timestamp(resp[0]) if resp is not None else None + def _get_min_finalised_ts_txn(self, txn: LoggingTransaction) -> Timestamp | None: + txn.execute("SELECT MIN(finalised_ts) FROM delayed_events") + resp = txn.fetchone() + return Timestamp(resp[0]) if resp is not None else None + def _generate_delay_id() -> DelayID: """Generates an opaque string, for use as a delay ID""" diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 5b1385fc695..7e780cd46d3 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -763,7 +763,7 @@ def schedule(delay: Duration) -> FakeChannel: self.assertEqual(5, int(retry_after_headers[0])) @unittest.override_config( - {"experimental_features": {"msc4140_finalised_retention_period": "1h"}} + {"experimental_features": {"msc4140_finalised_retention_period": "1m"}} ) def test_finalised_delayed_events_are_pruned_after_retention_period( self, @@ -793,12 +793,12 @@ def test_finalised_delayed_events_are_pruned_after_retention_period( ) self._assert_finalised(delay_id, event_id=event["event_id"]) - # The finalised delayed event must survive prune runs within its retention period - self.reactor.advance(Duration(minutes=30).as_secs()) + # The finalised delayed event must be discoverable within its retention period + self.reactor.advance(Duration(seconds=30).as_secs()) self._assert_finalised(delay_id, event_id=event["event_id"]) - # ...but not the first prune run after it - self.reactor.advance(Duration(minutes=30).as_secs()) + # ...but not after it + self.reactor.advance(Duration(seconds=30).as_secs()) channel = self.make_request( "GET", f"{_MANAGEMENT_PATH_PREFIX}/{delay_id}", @@ -807,6 +807,11 @@ def test_finalised_delayed_events_are_pruned_after_retention_period( self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, channel.result) self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"], channel.result) + # The pruning scheduler should have no work left + self.assertIsNone( + self.hs.get_delayed_events_handler()._next_prune_finalised_call + ) + @unittest.override_config( {"experimental_features": {"msc4140_finalised_retention_limit_per_user": 2}} ) @@ -845,13 +850,6 @@ def schedule_and_cancel(access_token: str) -> str: schedule_and_cancel(self.user1_access_token) for _ in range(3) ] - # Nothing is pruned until the next prune run - for delay_id in user1_delay_ids: - self._assert_finalised(delay_id) - self._assert_finalised(user2_delay_id, access_token=self.user2_access_token) - - self.reactor.advance(Duration(minutes=5).as_secs()) - # Only the oldest finalised delayed event of user1 is pruned channel = self.make_request( "GET", @@ -864,6 +862,11 @@ def schedule_and_cancel(access_token: str) -> str: self._assert_finalised(delay_id) self._assert_finalised(user2_delay_id, access_token=self.user2_access_token) + # The pruning scheduler should have work left to do + self.assertIsNotNone( + self.hs.get_delayed_events_handler()._next_prune_finalised_call + ) + def test_get_delayed_events_auth(self) -> None: channel = self.make_request("GET", _MANAGEMENT_PATH_PREFIX) self.assertEqual(HTTPStatus.UNAUTHORIZED, channel.code, channel.result) @@ -1741,9 +1744,22 @@ def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self._check_for_delayed_event_in_sync(self.user1_access_token, delay_id, True) self._check_for_delayed_event_in_sync(self.user2_access_token, delay_id, False) - @parameterized.expand((("client_api", False), ("admin_api", True))) + @parameterized.expand( + ( + ( + f"{'admin' if by_admin else 'client'}_api_{'erase' if erase else 'preserve'}", + by_admin, + erase, + ) + for by_admin in (False, True) + for erase in (False, True) + ) + ) + @unittest.override_config( + {"experimental_features": {"msc4140_finalised_retention_limit_per_user": 1}} + ) def test_delayed_events_are_cancelled_on_deactivation( - self, _name: str, by_admin: bool + self, _name: str, by_admin: bool, erase: bool ) -> None: store = self.hs.get_datastores().main handler = self.hs.get_delayed_events_handler() @@ -1787,7 +1803,7 @@ def test_delayed_events_are_cancelled_on_deactivation( channel = self.make_request( "POST", f"/_synapse/admin/v1/deactivate/{self.user1_user_id}", - {}, + {"erase": erase}, self.login("admin", "pass"), ) else: @@ -1800,21 +1816,39 @@ def test_delayed_events_are_cancelled_on_deactivation( "user": self.user1_user_id, "password": "pass", }, + "erase": erase, }, self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - # user1's delayed events are gone... + # Assert that user1's delayed events are gone. self.assertListEqual( [], self.get_success(store.get_all_delayed_events_for_user("user1")) ) - for delay_id in user1_delay_ids: + + if erase: + # Assert that both delayed events are pruned. + for delay_id in user1_delay_ids: + self.get_failure( + store.get_delayed_event_for_user(delay_id, "user1"), + NotFoundError, + ) + else: + # Assert that the oldest of user1's delayed events are pruned. self.get_failure( - store.get_delayed_event_for_user(delay_id, "user1"), NotFoundError + store.get_delayed_event_for_user(user1_delay_ids[0], "user1"), + NotFoundError, + ) + + # Assert that the newest of user1's delayed events are finalised. + self.assertIsNotNone( + self.get_success( + store.get_delayed_event_for_user(user1_delay_ids[1], "user1") + ).finalised ) - # ...and the next send has been rescheduled for user2's delayed event. + # Assert that the next send has been rescheduled for user2's delayed event. assert handler._next_delayed_event_call is not None self.assertGreater( handler._next_delayed_event_call.getTime() - self.reactor.seconds(), 1 @@ -1849,8 +1883,9 @@ def test_delayed_events_are_cancelled_on_deactivation( self.user2_access_token, user2_delay_id, True ) + @parameterized.expand((("preserve", False), ("erase", True))) def test_deactivation_cancels_the_timer_when_no_delayed_events_remain( - self, + self, _name: str, erase: bool ) -> None: handler = self.hs.get_delayed_events_handler() @@ -1863,15 +1898,20 @@ def test_deactivation_cancels_the_timer_when_no_delayed_events_remain( ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.assertIsNotNone(handler._next_delayed_event_call) + self.assertIsNone(handler._next_prune_finalised_call) self.get_success( self.hs.get_deactivate_account_handler().deactivate_account( self.user1_user_id, - erase_data=False, + erase_data=erase, requester=create_requester(self.user1_user_id), ) ) self.assertIsNone(handler._next_delayed_event_call) + if erase: + self.assertIsNone(handler._next_prune_finalised_call) + else: + self.assertIsNotNone(handler._next_prune_finalised_call) class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase): @@ -2053,19 +2093,25 @@ def test_delayed_event_scheduled_on_worker_is_sent(self) -> None: self.reactor.advance(Duration(seconds=1).as_secs()) self._check_for_delayed_event_in_sync(self.access_token, delay_id, True) + @unittest.override_config( + {"experimental_features": {"msc4140_finalised_retention_limit_per_user": 1}} + ) def test_delayed_events_are_cancelled_on_deactivation_from_worker(self) -> None: worker_hs = self.make_worker_hs("synapse.app.generic_worker") store = self.hs.get_datastores().main handler = self.hs.get_delayed_events_handler() - channel = self._make_delayed_event_request( - room_id=self.room_id, - delay=Duration(milliseconds=900), - event_type=_EVENT_TYPE, - content={}, - access_token=self.access_token, - ) - self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_ids = [] + for _ in range(2): + channel = self._make_delayed_event_request( + room_id=self.room_id, + delay=Duration(milliseconds=900), + event_type=_EVENT_TYPE, + content={}, + access_token=self.access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_ids.append(channel.json_body["delay_id"]) self.assertIsNotNone(handler._next_delayed_event_call) # Deactivate the account on the worker. The main process, which is the @@ -2078,7 +2124,21 @@ def test_delayed_events_are_cancelled_on_deactivation_from_worker(self) -> None: ) ) + # Assert that user1's delayed events are gone. self.assertListEqual( [], self.get_success(store.get_all_delayed_events_for_user("user")) ) + + # Assert that the oldest of user1's delayed events are pruned. + self.get_failure( + store.get_delayed_event_for_user(delay_ids[0], "user"), NotFoundError + ) + + # Assert that the newest of user1's delayed events are finalised. + self.assertIsNotNone( + self.get_success( + store.get_delayed_event_for_user(delay_ids[1], "user") + ).finalised + ) + self.assertIsNone(handler._next_delayed_event_call) From d9ef39a6413ce8e0e1bd7e95be3313b5477d8ffa Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 15 Oct 2026 23:37:58 -0400 Subject: [PATCH 43/47] Cover case of updating in-processed delayed event When trying to send/cancel/restart a delayed event that is in the midst of being sent, return a different error than 404, which would improperly tell clients that the delayed event in question doesn't exist. This is a stopgap measure until there is a way to properly handle such requests in this scenario. --- .../storage/databases/main/delayed_events.py | 81 ++++++++++--------- tests/rest/client/test_delayed_events.py | 59 +++++++++++++- 2 files changed, 100 insertions(+), 40 deletions(-) diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index f99cbf4913c..ad8ea832a6b 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -312,22 +312,12 @@ def restart_delayed_event_txn( ), ) if txn.rowcount == 0: - txn.execute( - """ - SELECT finalised_event_id IS NOT NULL - FROM delayed_events - WHERE delay_id = ? - AND user_localpart = ? - AND finalised_ts IS NOT NULL - """, - (delay_id, user_localpart), + event_was_sent = _handle_mgmt_action_conflict_txn( + txn, delay_id, user_localpart ) - row = txn.fetchone() - if not row: - raise NotFoundError("Delayed event not found") raise SynapseError( HTTPStatus.CONFLICT, - f"Delayed event has already been {'sent' if row[0] else 'cancelled'}", + f"Delayed event has already been {'sent' if event_was_sent else 'cancelled'}", ) next_send_ts = self._get_next_delayed_event_send_ts_txn(txn) @@ -687,20 +677,10 @@ def process_target_delayed_event_txn( ) row = txn.fetchone() if not row: - txn.execute( - """ - SELECT finalised_event_id IS NOT NULL - FROM delayed_events - WHERE delay_id = ? - AND user_localpart = ? - AND finalised_ts IS NOT NULL - """, - (delay_id, user_localpart), + event_was_sent = _handle_mgmt_action_conflict_txn( + txn, delay_id, user_localpart ) - row = txn.fetchone() - if not row: - raise NotFoundError("Delayed event not found") - elif not row[0]: + if not event_was_sent: raise SynapseError( HTTPStatus.CONFLICT, "Delayed event has already been cancelled", @@ -782,20 +762,10 @@ def cancel_delayed_event_txn( ), ) if txn.rowcount == 0: - txn.execute( - """ - SELECT finalised_event_id IS NOT NULL - FROM delayed_events - WHERE delay_id = ? - AND user_localpart = ? - AND finalised_ts IS NOT NULL - """, - (delay_id, user_localpart), + event_was_sent = _handle_mgmt_action_conflict_txn( + txn, delay_id, user_localpart ) - row = txn.fetchone() - if not row: - raise NotFoundError("Delayed event not found") - elif row[0]: + if event_was_sent: raise SynapseError( HTTPStatus.CONFLICT, "Delayed event has already been sent", @@ -1008,6 +978,39 @@ def _get_min_finalised_ts_txn(self, txn: LoggingTransaction) -> Timestamp | None return Timestamp(resp[0]) if resp is not None else None +def _handle_mgmt_action_conflict_txn( + txn: LoggingTransaction, + delay_id: str, + user_localpart: str, +) -> bool: + txn.execute( + """ + SELECT + finalised_ts IS NULL AND is_processed, + finalised_event_id IS NOT NULL + FROM delayed_events + WHERE delay_id = ? AND user_localpart = ? + """, + (delay_id, user_localpart), + ) + row = txn.fetchone() + if not row: + raise NotFoundError("Delayed event not found") + + event_is_being_sent = row[0] + if event_is_being_sent: + # TODO: Replace this with something that properly handles the request, + # even for workers (e.g. when a worker handles a request to restart a + # delayed event that the main worker is in the process of sending). + raise SynapseError( + HTTPStatus.SERVICE_UNAVAILABLE, + f"Unable to manage delayed event {delay_id} that is in the process of being sent", + ) + + event_was_sent = row[1] + return event_was_sent + + def _generate_delay_id() -> DelayID: """Generates an opaque string, for use as a delay ID""" diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 7e780cd46d3..c00e215e4b1 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -16,7 +16,7 @@ import sqlite3 from http import HTTPStatus -from typing import Literal, overload +from typing import Any, Literal, overload from unittest.mock import Mock, patch from urllib.parse import urlencode @@ -1696,6 +1696,63 @@ def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None: ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + @parameterized.expand( + ( + (action, action_in_path) + for action in _UpdateDelayedEventAction + for action_in_path in (True, False) + ) + ) + def test_update_delayed_event_being_processed( + self, + action: _UpdateDelayedEventAction, + action_in_path: bool, + ) -> None: + """Test managing a delayed event that is in the process of being sent.""" + send_after = Duration(seconds=1) + channel = self._make_delayed_event_request( + room_id=self.room_id, + delay=send_after, + event_type=_EVENT_TYPE, + content={}, + access_token=self.user1_access_token, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body["delay_id"] + + # Simulate the server taking a long time to persist delayed events + simulated_send_lag = Duration(seconds=5) + event_creation_handler = self.hs.get_event_creation_handler() + orig_send_fn = event_creation_handler.create_and_send_nonmember_event + + async def slow_send_fn(*args: Any, **kwargs: Any) -> Any: + await self.clock.sleep(simulated_send_lag) + return await orig_send_fn(*args, **kwargs) + + with patch.object(event_creation_handler, orig_send_fn.__name__, slow_send_fn): + # Advance time enough so the delayed event starts being sent, + # but not enough for it to finish the sending process + self.reactor.advance(send_after.as_secs()) + channel = self._update_delayed_event( + delay_id, action, action_in_path, self.user1_access_token + ) + self.assertEqual( + HTTPStatus.SERVICE_UNAVAILABLE, channel.code, channel.result + ) + + # Advance time enough so the sending process finishes + self.reactor.advance(simulated_send_lag.as_secs()) + channel = self._update_delayed_event( + delay_id, action, action_in_path, self.user1_access_token + ) + self.assertEqual( + HTTPStatus.OK + if action == _UpdateDelayedEventAction.SEND + else HTTPStatus.CONFLICT, + channel.code, + channel.result, + ) + def test_delayed_state_is_not_cancelled_by_new_state_from_same_user( self, ) -> None: From a308f3034b589970d7735627e34be997f6616b93 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 19 Oct 2026 10:19:57 -0400 Subject: [PATCH 44/47] Add finalised coverage on new tests from merge --- tests/rest/client/test_delayed_events.py | 29 +++++++++++++++--------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 0e94652d729..130fe226fc6 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -751,10 +751,12 @@ def test_delayed_event_fails_on_timeout_if_sender_left_room(self) -> None: access_token=self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body["delay_id"] self.helper.leave(self.room_id, self.user1_user_id, tok=self.user1_access_token) - self.reactor.advance(1) + # Advance time enough to confirm that the delayed event was not sent + self.reactor.advance(Duration(seconds=1).as_secs()) self.helper.get_state( self.room_id, _EVENT_TYPE, @@ -762,6 +764,7 @@ def test_delayed_event_fails_on_timeout_if_sender_left_room(self) -> None: state_key=state_key, expect_code=HTTPStatus.NOT_FOUND, ) + self._assert_finalised(delay_id, errcode=Codes.FORBIDDEN) def test_delayed_self_join_accepted_if_sender_not_in_room(self) -> None: """A delayed join is accepted from outside a public room, and the sender is @@ -781,17 +784,18 @@ def test_delayed_self_join_accepted_if_sender_not_in_room(self) -> None: access_token=self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body["delay_id"] - self.reactor.advance(1) - content = self.helper.get_state( - room_id, - EventTypes.Member, - self.user2_access_token, - state_key=self.user1_user_id, - ) - self.assertEqual( - Membership.JOIN, content.get(EventContentFields.MEMBERSHIP), content + # Advance time enough so the delayed event is sent + self.reactor.advance(Duration(seconds=1).as_secs()) + channel = self.make_request( + "GET", + f"/rooms/{room_id}/state/{EventTypes.Member}/{self.user1_user_id}?format=event", + access_token=self.user2_access_token, ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assert_dict({EventContentFields.MEMBERSHIP: Membership.JOIN}, channel.json_body["content"]) + self._assert_finalised(delay_id, event_id=channel.json_body["event_id"]) def test_delayed_self_join_fails_on_timeout_if_join_not_allowed(self) -> None: """A delayed join is accepted from outside an invite-only room, and fails @@ -809,8 +813,10 @@ def test_delayed_self_join_fails_on_timeout_if_join_not_allowed(self) -> None: access_token=self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + delay_id = channel.json_body["delay_id"] - self.reactor.advance(1) + # Advance time enough to confirm that the delayed event was not sent + self.reactor.advance(Duration(seconds=1).as_secs()) self.helper.get_state( room_id, EventTypes.Member, @@ -818,6 +824,7 @@ def test_delayed_self_join_fails_on_timeout_if_join_not_allowed(self) -> None: state_key=self.user1_user_id, expect_code=HTTPStatus.NOT_FOUND, ) + self._assert_finalised(delay_id, errcode=Codes.FORBIDDEN) def test_delayed_event_is_finalised_with_error_if_timeout_send_fails( self, From a0301e6fa9ab208a1f262b8705aa943f27e4b4bf Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 19 Oct 2026 10:21:01 -0400 Subject: [PATCH 45/47] Add missing comments for delayed event waits --- tests/rest/client/test_delayed_events.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 130fe226fc6..9f04ab186f1 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -1169,7 +1169,8 @@ def test_update_delayed_event_without_auth( # The event is still scheduled, and is sent on its original timeout. self.assertEqual(1, len(self._get_delayed_events())) self._check_for_delayed_event_in_sync(self.user1_access_token, delay_id, False) - self.reactor.advance(1) + # Advance time enough so the delayed event is sent + self.reactor.advance(Duration(seconds=1).as_secs()) self._check_for_delayed_event_in_sync(self.user1_access_token, delay_id, True) @parameterized.expand( @@ -1196,7 +1197,8 @@ def test_update_delayed_event_of_other_user( # The event is still scheduled, and is sent on its original timeout. self.assertEqual(1, len(self._get_delayed_events())) self._check_for_delayed_event_in_sync(self.user1_access_token, delay_id, False) - self.reactor.advance(1) + # Advance time enough so the delayed event is sent + self.reactor.advance(Duration(seconds=1).as_secs()) self._check_for_delayed_event_in_sync(self.user1_access_token, delay_id, True) @parameterized.expand( @@ -1237,13 +1239,15 @@ def test_update_delayed_event_as_appservice( if action == _UpdateDelayedEventAction.CANCEL: self.assertListEqual([], self._get_delayed_events()) - self.reactor.advance(1) + # Advance time enough to confirm that the delayed event was not sent + self.reactor.advance(Duration(seconds=1).as_secs()) self._check_for_delayed_event_in_sync( self.user1_access_token, delay_id, False ) elif action == _UpdateDelayedEventAction.RESTART: self.assertEqual(1, len(self._get_delayed_events())) - self.reactor.advance(1) + # Advance time enough so the delayed event is sent + self.reactor.advance(Duration(seconds=1).as_secs()) self._check_for_delayed_event_in_sync( self.user1_access_token, delay_id, True ) From bd5f615df6fd368f5376a1af380bf49c77efc70a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 19 Oct 2026 10:22:37 -0400 Subject: [PATCH 46/47] Update copyright date on newly-added file --- .../schema/main/delta/95/01_add_finalised_delayed_events.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/schema/main/delta/95/01_add_finalised_delayed_events.sql b/synapse/storage/schema/main/delta/95/01_add_finalised_delayed_events.sql index c3471e43a74..282c2231c62 100644 --- a/synapse/storage/schema/main/delta/95/01_add_finalised_delayed_events.sql +++ b/synapse/storage/schema/main/delta/95/01_add_finalised_delayed_events.sql @@ -1,7 +1,7 @@ -- -- This file is licensed under the Affero General Public License (AGPL) version 3. -- --- Copyright (C) 2025 New Vector, Ltd +-- Copyright (C) 2026 New Vector, 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 From 02da61dbcb065c76b528dab6b62ae469a4e254e5 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 19 Oct 2026 10:50:10 -0400 Subject: [PATCH 47/47] Lint --- tests/rest/client/test_delayed_events.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index 9f04ab186f1..78f3ffa4fd5 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -794,7 +794,10 @@ def test_delayed_self_join_accepted_if_sender_not_in_room(self) -> None: access_token=self.user2_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - self.assert_dict({EventContentFields.MEMBERSHIP: Membership.JOIN}, channel.json_body["content"]) + self.assert_dict( + {EventContentFields.MEMBERSHIP: Membership.JOIN}, + channel.json_body["content"], + ) self._assert_finalised(delay_id, event_id=channel.json_body["event_id"]) def test_delayed_self_join_fails_on_timeout_if_join_not_allowed(self) -> None: