From 9a1c92fafc30fa987eec5fe6945663727e4acfac Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 12 Mar 2026 17:37:19 -0600 Subject: [PATCH 01/68] Create a stream to track quarantine state changes Sticky events was used as reference for creating this. --- synapse/_scripts/synapse_port_db.py | 1 + synapse/replication/tcp/streams/__init__.py | 3 + synapse/replication/tcp/streams/_base.py | 44 ++++++++++++ synapse/storage/databases/main/room.py | 69 +++++++++++++++++++ .../93/05_quarantined_media_tracking.sql | 29 ++++++++ ...uarantined_media_tracking_seq.sql.postgres | 18 +++++ 6 files changed, 164 insertions(+) create mode 100644 synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql create mode 100644 synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index 79b2a0c528e..c1589a93a6b 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -136,6 +136,7 @@ "users": ["shadow_banned", "approved", "locked", "suspended"], "un_partial_stated_event_stream": ["rejection_status_changed"], "users_who_share_rooms": ["share_private"], + "quarantined_media_changes": ["quarantined"], } diff --git a/synapse/replication/tcp/streams/__init__.py b/synapse/replication/tcp/streams/__init__.py index 067847617fa..e41573cf689 100644 --- a/synapse/replication/tcp/streams/__init__.py +++ b/synapse/replication/tcp/streams/__init__.py @@ -39,6 +39,7 @@ PresenceStream, PushersStream, PushRulesStream, + QuarantinedMediaStream, ReceiptsStream, StickyEventsStream, Stream, @@ -73,6 +74,7 @@ ThreadSubscriptionsStream, UnPartialStatedRoomStream, UnPartialStatedEventStream, + QuarantinedMediaStream, ) } @@ -96,4 +98,5 @@ "ThreadSubscriptionsStream", "UnPartialStatedRoomStream", "UnPartialStatedEventStream", + "QuarantinedMediaStream", ] diff --git a/synapse/replication/tcp/streams/_base.py b/synapse/replication/tcp/streams/_base.py index 1ea6b4fa857..c55cba3bad9 100644 --- a/synapse/replication/tcp/streams/_base.py +++ b/synapse/replication/tcp/streams/_base.py @@ -808,3 +808,47 @@ async def _update_function( return [], to_token, False return rows, rows[-1][0], len(updates) == limit + + +@attr.s(slots=True, auto_attribs=True) +class QuarantinedMediaStreamRow: + """Row for QuarantinedMediaStream""" + + origin: str + media_id: str + quarantined: bool + + +class QuarantinedMediaStream(_StreamFromIdGen): + """Stream to track changes to (un)quarantined media.""" + + NAME = "quarantined_media" + ROW_TYPE = QuarantinedMediaStreamRow + + def __init__(self, hs: "HomeServer"): + self.store = hs.get_datastores().main + super().__init__( + hs.get_instance_name(), + self._update_function, + self.store._quarantined_media_changes_id_gen, + ) + + async def _update_function( + self, instance_name: str, from_token: int, to_token: int, limit: int + ) -> StreamUpdateResult: + updates = await self.store.get_quarantined_media_changes( + from_id=from_token, to_id=to_token, limit=limit + ) + rows = [ + ( + update.stream_id, + # Args to `QuarantinedMediaStreamRow` + (update.origin, update.media_id, update.quarantined), + ) + for update in updates + ] + + if not rows: + return [], to_token, False + + return rows, rows[-1][0], len(updates) == limit diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 633df077367..5b0be0bfcd4 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -21,6 +21,7 @@ # import logging +from dataclasses import dataclass from enum import Enum from typing import ( TYPE_CHECKING, @@ -44,6 +45,7 @@ from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.events import EventBase +from synapse.replication.tcp.streams._base import QuarantinedMediaStream from synapse.replication.tcp.streams.partial_state import UnPartialStatedRoomStream from synapse.storage._base import ( db_to_json, @@ -102,6 +104,14 @@ class RoomStats(LargestRoomStats): public: bool +@dataclass(frozen=True) +class QuarantinedMediaUpdate: + stream_id: int + origin: str + media_id: str + quarantined: bool + + class RoomSortOrder(Enum): """ Enum to define the sorting method used when returning rooms with get_rooms_paginate @@ -162,11 +172,25 @@ def __init__( writers=["master"], ) + self._quarantined_media_changes_id_gen: MultiWriterIdGenerator = MultiWriterIdGenerator( + db_conn=db_conn, + db=database, + notifier=hs.get_replication_notifier(), + stream_name=QuarantinedMediaStream.NAME, + server_name=self.server_name, + instance_name=self._instance_name, + tables=[("quarantined_media_changes", "instance_name", "stream_id")], + sequence_name="quarantined_media_id_seq", + writers=[], # we don't use `get_current_token` or `get_positions`, per docs + ) + def process_replication_position( self, stream_name: str, instance_name: str, token: int ) -> None: if stream_name == UnPartialStatedRoomStream.NAME: self._un_partial_stated_rooms_stream_id_gen.advance(instance_name, token) + elif stream_name == QuarantinedMediaStream.NAME: + self._quarantined_media_changes_id_gen.advance(instance_name, token) return super().process_replication_position(stream_name, instance_name, token) async def store_room( @@ -1128,6 +1152,51 @@ def _get_media_ids_by_user_txn( return local_media_ids + async def get_quarantined_media_changes( + self, *, from_id: int, to_id: int, limit: int + ) -> list[QuarantinedMediaUpdate]: + """Get updates to quarantined media between two stream IDs. + + Bounds: from_id < ... <= to_id + + Args: + from_id: The starting stream ID (exclusive) + to_id: The ending stream ID (inclusive) + limit: The maximum number of rows to return + + Returns: + list of QuarantinedMediaUpdate update rows + """ + return await self.db_pool.runInteraction( + "get_quarantined_media_changes", + self._get_quarantined_media_changes_txn, + from_id, + to_id, + limit, + ) + + def _get_quarantined_media_changes_txn( + self, txn: LoggingTransaction, from_id: int, to_id: int, limit: int + ) -> list[QuarantinedMediaUpdate]: + txn.execute( + """ + SELECT stream_id, origin, media_id, quarantined + FROM quarantined_media_changes + WHERE ? < stream_id AND stream_id <= ? + LIMIT ? + """, + (from_id, to_id, limit), + ) + return [ + QuarantinedMediaUpdate( + stream_id=stream_id, + origin=origin, + media_id=media_id, + quarantined=quarantined, + ) + for stream_id, origin, media_id, quarantined in txn + ] + def _quarantine_local_media_txn( self, txn: LoggingTransaction, diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql new file mode 100644 index 00000000000..b5935c51f4a --- /dev/null +++ b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql @@ -0,0 +1,29 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2026 Element Creations Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + +CREATE TABLE quarantined_media_changes ( + -- Position in the quarantined media stream + stream_id INTEGER NOT NULL PRIMARY KEY, + + -- Name of the worker sending this (makes us compatible with multiple writers) + instance_name TEXT NOT NULL, + + -- Media origin. NULL if local media. + origin TEXT NULL, + + -- Media ID at the origin. + media_id TEXT NOT NULL, + + -- True if quarantined at this position, false otherwise. + quarantined BOOLEAN NOT NULL +); diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres new file mode 100644 index 00000000000..85f50ba8e79 --- /dev/null +++ b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres @@ -0,0 +1,18 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2026 Element Creations Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + +CREATE SEQUENCE quarantined_media_id_seq; +-- Synapse streams start at 2, because the default position is 1 +-- so any item inserted at position 1 is ignored. +-- We have to use nextval not START WITH 2, see https://github.com/element-hq/synapse/issues/18712 +SELECT nextval('quarantined_media_id_seq'); From c3a0e4ab6571561c4b43266b259eab1388d24b34 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 13 Mar 2026 15:27:31 -0600 Subject: [PATCH 02/68] Insert into quarantined media stream upon changes --- synapse/storage/databases/main/room.py | 90 ++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 7 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 5b0be0bfcd4..8bc14592505 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1197,6 +1197,49 @@ def _get_quarantined_media_changes_txn( for stream_id, origin, media_id, quarantined in txn ] + def _insert_quarantine_change_txn( + self, + txn: LoggingTransaction, + origins_and_media_ids: list[tuple[str | None, str]], + quarantined: bool, + ) -> None: + """Records media being (un)quarantined in the stream. + + Args: + txn (cursor) + origins_and_media_ids: The [origin, media_id] tuples to record. The origin + may be None if the media is local. + quarantined: Whether the media is being quarantined or unquarantined. + """ + medias_with_stream_ids = zip( + origins_and_media_ids, + self._quarantined_media_changes_id_gen.get_next_mult_txn( + txn, len(origins_and_media_ids) + ), + strict=True, + ) + self.db_pool.simple_insert_many_txn( + txn, + "quarantined_media_changes", + keys=( + "instance_name", + "stream_id", + "origin", + "media_id", + "quarantined", + ), + values=[ + ( + self._instance_name, + stream_id, + origin, + media_id, + quarantined, + ) + for (origin, media_id), stream_id in medias_with_stream_ids + ], + ) + def _quarantine_local_media_txn( self, txn: LoggingTransaction, @@ -1230,9 +1273,17 @@ def _quarantine_local_media_txn( if quarantined_by is not None: sql += " AND safe_from_quarantine = FALSE" + sql += " RETURNING media_id" + txn.execute(sql, [quarantined_by] + sql_many_clause_args) - # Note that a rowcount of -1 can be used to indicate no rows were affected. - total_media_quarantined += txn.rowcount if txn.rowcount > 0 else 0 + media_ids_affected = txn.fetchall() + total_media_quarantined += len(media_ids_affected) + if len(media_ids_affected) > 0: + self._insert_quarantine_change_txn( + txn, + [(None, media_id) for (media_id,) in media_ids_affected], + quarantined_by is not None, + ) # Update any media that was identified via hash. if hashes: @@ -1247,8 +1298,17 @@ def _quarantine_local_media_txn( if quarantined_by is not None: sql += " AND safe_from_quarantine = FALSE" + sql += " RETURNING media_id" + txn.execute(sql, [quarantined_by] + sql_many_clause_args) - total_media_quarantined += txn.rowcount if txn.rowcount > 0 else 0 + media_ids_affected = txn.fetchall() + total_media_quarantined += len(media_ids_affected) + if len(media_ids_affected) > 0: + self._insert_quarantine_change_txn( + txn, + [(None, media_id) for (media_id,) in media_ids_affected], + quarantined_by is not None, + ) return total_media_quarantined @@ -1281,10 +1341,18 @@ def _quarantine_remote_media_txn( sql = f""" UPDATE remote_media_cache SET quarantined_by = ? - WHERE {sql_in_list_clause}""" + WHERE {sql_in_list_clause} + RETURNING media_origin, media_id""" txn.execute(sql, [quarantined_by] + sql_args) - total_media_quarantined += txn.rowcount if txn.rowcount > 0 else 0 + media_ids_affected = cast(list[tuple[str | None, str]], txn.fetchall()) + total_media_quarantined += len(media_ids_affected) + if len(media_ids_affected) > 0: + self._insert_quarantine_change_txn( + txn, + media_ids_affected, + quarantined_by is not None, + ) total_media_quarantined = 0 if hashes: @@ -1294,9 +1362,17 @@ def _quarantine_remote_media_txn( sql = f""" UPDATE remote_media_cache SET quarantined_by = ? - WHERE {sql_many_clause_sql}""" + WHERE {sql_many_clause_sql} + RETURNING media_origin, media_id""" txn.execute(sql, [quarantined_by] + sql_many_clause_args) - total_media_quarantined += txn.rowcount if txn.rowcount > 0 else 0 + media_ids_affected = cast(list[tuple[str | None, str]], txn.fetchall()) + total_media_quarantined += len(media_ids_affected) + if len(media_ids_affected) > 0: + self._insert_quarantine_change_txn( + txn, + media_ids_affected, + quarantined_by is not None, + ) return total_media_quarantined From 7386f04788f4dee567df07b4f370e0d1eee300fb Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 13 Mar 2026 16:39:30 -0600 Subject: [PATCH 03/68] Add admin API to access stream data --- docs/admin_api/media_admin_api.md | 26 +++++++ synapse/rest/admin/media.py | 41 +++++++++++ tests/rest/admin/test_media.py | 113 ++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index 6b96eb33564..3d923e25169 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -247,6 +247,32 @@ Response: {} ``` +## Listing quarantined media changes + +When media is quarantined or unquarantined, a change record is created in the +database. This API returns those change records. + +Request: + +``` +GET /_synapse/admin/v1/media/quarantine_changes?from=2 +``` + +Where `from` is the `next_batch` value from a previous request. It is optional. + +Response: + +```json +{ + "next_batch": 4, + "rows": [ + { "origin": "example.org", "media_id": "abcdefg12345...", "quarantined": true }, + { "origin": "example.org", "media_id": "abcdefg12345...", "quarantined": false }, + { "origin": "another.example.org", "media_id": "abcdefg12345...", "quarantined": true } + ] +} +``` + # Delete local media This API deletes the *local* media from the disk of your own server. This includes any local thumbnails and copies of media downloaded from diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index d5346fe0d5c..ea131fd5b1b 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -230,6 +230,46 @@ async def on_POST( return HTTPStatus.OK, {} +class ListQuarantineChanges(RestServlet): + """Lists the quarantine changes to media.""" + + PATTERNS = admin_patterns("/media/quarantine_changes$") + + def __init__(self, hs: "HomeServer"): + self.store = hs.get_datastores().main + self.auth = hs.get_auth() + self.server_name = hs.hostname + + async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: + await assert_requester_is_admin(self.auth, request) + + from_id = parse_integer(request, "from", default=0) + limit = 100 # arbitrary; not enough to cause problems (hopefully) + to_id = ( + from_id + limit + ) # somewhat implied, but makes our call to the store easier + + changes = await self.store.get_quarantined_media_changes( + from_id=from_id, + to_id=to_id, + limit=limit, + ) + + rows = [ + { + "origin": c.origin if c.origin is not None else self.server_name, + "media_id": c.media_id, + "quarantined": c.quarantined, + } + for c in changes + ] + next_batch = max( + c.stream_id for c in changes + ) # `from` is exclusive, so don't +1 + + return HTTPStatus.OK, {"next_batch": next_batch, "rows": rows} + + class ProtectMediaByID(RestServlet): """Protect local media from being quarantined.""" @@ -529,6 +569,7 @@ def register_servlets_for_media_repo(hs: "HomeServer", http_server: HttpServer) QuarantineMediaByID(hs).register(http_server) UnquarantineMediaByID(hs).register(http_server) QuarantineMediaByUser(hs).register(http_server) + ListQuarantineChanges(hs).register(http_server) ProtectMediaByID(hs).register(http_server) UnprotectMediaByID(hs).register(http_server) ListMediaInRoom(hs).register(http_server) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 8cc54cc80c2..6828d9c82d5 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -756,6 +756,119 @@ def _access_media( self.assertFalse(os.path.exists(local_path)) +class ListQuarantinedMediaChangesTestCase(_AdminMediaTests): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + self.server_name = hs.hostname + + def test_no_auth(self) -> None: + """ + Try to list quarantined media changes without authentication. + """ + + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes", + ) + + self.assertEqual(401, channel.code, msg=channel.json_body) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + def test_requester_is_not_admin(self) -> None: + """ + If the user is not a server admin, an error is returned. + """ + self.other_user = self.register_user("user", "pass") + self.other_user_token = self.login("user", "pass") + + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes", + access_token=self.other_user_token, + ) + + self.assertEqual(403, channel.code, msg=channel.json_body) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + def test_list_quarantined_media(self) -> None: + """ + Ensure we actually get results for each page. We can't really test that + remote media is quarantined, but we can test that local media is. + """ + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + def _upload() -> str: + return self.helper.upload_media( + SMALL_PNG, tok=self.admin_user_tok, expect_code=200 + )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + + self.media_id_1 = _upload() + self.media_id_2 = _upload() + self.media_id_3 = _upload() + + def _quarantine(media_id: str) -> None: + channel = self.make_request( + "POST", + "/_synapse/admin/v1/media/quarantine/%s/%s" + % ( + self.server_name, + media_id, + ), + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # No rows before quarantine + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes", + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(0, len(channel.json_body["rows"])) + + # Quarantine by hash should kick in to get the other two media objects + _quarantine(self.media_id_1) + + # Page 1 (implied ?from=0) + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes", + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(3, len(channel.json_body["rows"])) + for row in channel.json_body["rows"]: + self.assertIn( + row["media_id"], (self.media_id_1, self.media_id_2, self.media_id_3) + ) + self.assertEqual(row["origin"], self.server_name) + self.assertEqual(row["quarantined"], True) + + # Page 1 (explicit ?from) + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes?from=2", # streams start at 2 + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(2, len(channel.json_body["rows"])) + for row in channel.json_body["rows"]: + self.assertIn(row["media_id"], (self.media_id_2, self.media_id_3)) + self.assertEqual(row["origin"], self.server_name) + self.assertEqual(row["quarantined"], True) + + # Page 2 (?from out of range) + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes?from=900000", + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(0, len(channel.json_body["rows"])) + + class QuarantineMediaByIDTestCase(_AdminMediaTests): def upload_media_and_return_media_id(self, data: bytes) -> str: # Upload some media into the room From 0f782ce59c9a1236f6bd146e34fef06f1f244f50 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 16 Mar 2026 13:23:58 -0600 Subject: [PATCH 04/68] Add background update to insert existing rows --- synapse/storage/databases/main/room.py | 54 +++++++++++++++++++ .../93/05_quarantined_media_tracking.sql | 3 ++ 2 files changed, 57 insertions(+) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 8bc14592505..b0895da77e3 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -184,6 +184,60 @@ def __init__( writers=[], # we don't use `get_current_token` or `get_positions`, per docs ) + self.db_pool.updates.register_background_update_handler( + "flag_existing_quarantined_media", self._flag_existing_quarantined_media + ) + + async def _flag_existing_quarantined_media( + self, progress: JsonDict, batch_size: int + ) -> int: + last_row_num: int = progress.get("last_row_num", 0) + + # The `ORDER BY` here would normally miss records if the admin (un)quarantined a + # record, but that doesn't affect the background update because we also insert + # into the stream table upon quarantine status changing. Worst case is the admin + # newly quarantines some media, adding a row to the stream table, then we run + # over it again in the background update, adding a second row. Duplicate rows are + # non-issues for us. + def flag_quarantined(txn: LoggingTransaction) -> int: + txn.execute( + """ + SELECT NULL AS media_origin, media_id + FROM local_media_repository + WHERE quarantined_by IS NOT NULL + + UNION + + SELECT media_origin, media_id + FROM remote_media_cache + WHERE quarantined_by IS NOT NULL + + ORDER BY media_origin, media_id + LIMIT ? OFFSET ? + """, + (batch_size, last_row_num), + ) + res = cast(list[tuple[str | None, str]], txn.fetchall()) + if len(res) > 0: + self._insert_quarantine_change_txn(txn, res, True) + self.db_pool.updates._background_update_progress_txn( + txn, + "flag_existing_quarantined_media", + {"last_row_num": last_row_num + len(res)}, + ) + return len(res) + + logger.info("Flagging existing quarantined media with offset %s", last_row_num) + num_flagged = await self.db_pool.runInteraction( + "_flag_existing_quarantined_media", + flag_quarantined, + ) + if num_flagged <= 0: # probably never negative, but why trust computers? + await self.db_pool.updates._end_background_update( + "flag_existing_quarantined_media" + ) + return num_flagged + def process_replication_position( self, stream_name: str, instance_name: str, token: int ) -> None: diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql index b5935c51f4a..cfd33911f1f 100644 --- a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql +++ b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql @@ -27,3 +27,6 @@ CREATE TABLE quarantined_media_changes ( -- True if quarantined at this position, false otherwise. quarantined BOOLEAN NOT NULL ); + +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (9305, 'flag_existing_quarantined_media', '{}'); From f1a35fa7e073b48b6e410de1f11cdb6db9aae1c6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 16 Mar 2026 13:23:31 -0600 Subject: [PATCH 05/68] changelog --- changelog.d/19558.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19558.feature diff --git a/changelog.d/19558.feature b/changelog.d/19558.feature new file mode 100644 index 00000000000..f4c8f042bf0 --- /dev/null +++ b/changelog.d/19558.feature @@ -0,0 +1 @@ +Add a ["Listing quarantined media changes" Admin API](https://element-hq.github.io/synapse/latest/admin_api/media_admin_api.html#listing-quarantined-media-changes) for retrieving a paginated record of when media became (un)quarantined. \ No newline at end of file From a78c03c723e14b31d749631230d2d65a12f227ca Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 16 Mar 2026 14:00:44 -0600 Subject: [PATCH 06/68] fix out of bounds on max --- synapse/rest/admin/media.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index ea131fd5b1b..2642dd00309 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -263,9 +263,11 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: } for c in changes ] + + # `from` is exclusive, so don't +1 next_batch = max( c.stream_id for c in changes - ) # `from` is exclusive, so don't +1 + ) if changes else from_id return HTTPStatus.OK, {"next_batch": next_batch, "rows": rows} From 7963593093f57e6b58634479e91fe6201fcf367a Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Mon, 16 Mar 2026 20:06:58 +0000 Subject: [PATCH 07/68] Attempt to fix linting --- synapse/rest/admin/media.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 2642dd00309..2ec9851c5d9 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -265,9 +265,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: ] # `from` is exclusive, so don't +1 - next_batch = max( - c.stream_id for c in changes - ) if changes else from_id + next_batch = max(c.stream_id for c in changes) if changes else from_id return HTTPStatus.OK, {"next_batch": next_batch, "rows": rows} From d77a76dcfc9b01efb224f47137759b5606a50681 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 16 Mar 2026 14:11:17 -0600 Subject: [PATCH 08/68] bump for ci From d7437114fd66748137ca27514aa6cc5167dc51c2 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 19:35:12 -0600 Subject: [PATCH 09/68] Add comments --- docs/admin_api/media_admin_api.md | 8 +++- synapse/storage/databases/main/room.py | 40 ++++++++++++++++++- .../93/05_quarantined_media_tracking.sql | 2 + 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index 3d923e25169..4d95e8a0eb5 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -250,7 +250,10 @@ Response: ## Listing quarantined media changes When media is quarantined or unquarantined, a change record is created in the -database. This API returns those change records. +database. This API returns those change records in the order they were created. + +Each page has a maximum of 100 records. The first page has the oldest records, +paginating forwards with each `next_batch` value. Request: @@ -258,7 +261,8 @@ Request: GET /_synapse/admin/v1/media/quarantine_changes?from=2 ``` -Where `from` is the `next_batch` value from a previous request. It is optional. +Where `from` is the `next_batch` value from a previous request. It is optional +and defaults to the first page (the value `0`). Response: diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index df1d680e66d..bacb3e352b1 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -184,6 +184,10 @@ def __init__( writers=[], # we don't use `get_current_token` or `get_positions`, per docs ) + # Register a background update to flag already-quarantined media in the quaranine + # media changes table. This is to populate the API endpoint which consumes the + # table with initial data that callers expect (namely, a list of currently + # quarantined media). self.db_pool.updates.register_background_update_handler( "flag_existing_quarantined_media", self._flag_existing_quarantined_media ) @@ -191,6 +195,31 @@ def __init__( async def _flag_existing_quarantined_media( self, progress: JsonDict, batch_size: int ) -> int: + """Background update function to flag existing already-quarantined media in + the new `quarantine_media_changes` table. + + This only flags quarantined media as the API which reads the table is only + concerned with *changes* to the quarantined state - media does not start as + quarantined, so if it's already quarantined then it has changed state at + some point. Media which isn't quarantined has not changed state (as far as + this function can tell). + + When media was quarantined is not recorded, so this uses the current time as + the time when the change happened. + + Further, due to lack of timestamp or history, media which was quarantined then + unquarantined will not be picked up by this background task. + + Function signature is as per `register_background_update_handler` requirements. + + Args: + progress: The progress dictionary from the background update. + batch_size: The number of rows to process in each batch. + + Returns: + The number of rows inserted. + """ + # The last row OFFSET we got to in the UNION query below last_row_num: int = progress.get("last_row_num", 0) # The `ORDER BY` here would normally miss records if the admin (un)quarantined a @@ -199,6 +228,9 @@ async def _flag_existing_quarantined_media( # newly quarantines some media, adding a row to the stream table, then we run # over it again in the background update, adding a second row. Duplicate rows are # non-issues for us. + # + # Note: Already-quarantined media is indicated by the `quarantined_by` field being + # non-null. We only want quarantined media, per docstring above. def flag_quarantined(txn: LoggingTransaction) -> int: txn.execute( """ @@ -1219,7 +1251,7 @@ async def get_quarantined_media_changes( limit: The maximum number of rows to return Returns: - list of QuarantinedMediaUpdate update rows + list of QuarantinedMediaUpdate update rows in stream ordering. """ return await self.db_pool.runInteraction( "get_quarantined_media_changes", @@ -1331,8 +1363,14 @@ def _quarantine_local_media_txn( txn.execute(sql, [quarantined_by] + sql_many_clause_args) media_ids_affected = txn.fetchall() + # We use `len(media_ids_affected)` here and below because both queries may + # affect fewer or more rows than the `media_ids` input. For example, if the + # media_ids point to already-quarantined media, then nothing was updated. + # Similarly, the below query might find more media than the `media_ids` + # because it's searching for hashes instead. total_media_quarantined += len(media_ids_affected) if len(media_ids_affected) > 0: + # Flag media that was newly (un)quarantined in the changes table. self._insert_quarantine_change_txn( txn, [(None, media_id) for (media_id,) in media_ids_affected], diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql index cfd33911f1f..311add3314e 100644 --- a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql +++ b/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql @@ -11,6 +11,7 @@ -- See the GNU Affero General Public License for more details: -- . +-- Represents a stream of when media is quarantined and unquarantined. CREATE TABLE quarantined_media_changes ( -- Position in the quarantined media stream stream_id INTEGER NOT NULL PRIMARY KEY, @@ -28,5 +29,6 @@ CREATE TABLE quarantined_media_changes ( quarantined BOOLEAN NOT NULL ); +-- Start the background update to populate existing quarantined media in the table. See update handler for more details. INSERT INTO background_updates (ordering, update_name, progress_json) VALUES (9305, 'flag_existing_quarantined_media', '{}'); From 81ce22c2ac9e520f4ff056fc8ecbc22b661709a0 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 19:52:18 -0600 Subject: [PATCH 10/68] Use current token for stream, requiring writer configuration --- docs/workers.md | 5 +++++ synapse/config/workers.py | 6 ++++++ synapse/rest/admin/media.py | 10 +++++----- synapse/storage/databases/main/room.py | 10 +++++++++- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/workers.md b/docs/workers.md index c2aef33e168..c16d4def034 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -576,6 +576,11 @@ configured as stream writer for the `device_lists` stream: ^/_matrix/client/(api/v1|r0|v3|unstable)/keys/device_signing/upload$ ^/_matrix/client/(api/v1|r0|v3|unstable)/keys/signatures/upload$ +##### The `quarantined_media_changes` stream + +[`synapse.app.media_repository`](#synapseappmedia_repository) workers should be +configured as stream writers for the `quarantined_media_changes` stream. + #### Restrict outbound federation traffic to a specific set of workers The diff --git a/synapse/config/workers.py b/synapse/config/workers.py index 996be88cb26..fb7378bfc81 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -142,6 +142,8 @@ class WriterLocations: push_rules: The instances that write to the push stream. Currently can only be a single instance. device_lists: The instances that write to the device list stream. + quarantined_media_changes: The instances that write to the quarantined media + changes stream. """ events: list[str] = attr.ib( @@ -180,6 +182,10 @@ class WriterLocations: default=["master"], converter=_instance_to_list_converter, ) + quarantined_media_changes: list[str] = attr.ib( + default=[MAIN_PROCESS_INSTANCE_NAME], + converter=_instance_to_list_converter, + ) @attr.s(auto_attribs=True) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 2ec9851c5d9..fe4bcd07749 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -245,9 +245,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: from_id = parse_integer(request, "from", default=0) limit = 100 # arbitrary; not enough to cause problems (hopefully) - to_id = ( - from_id + limit - ) # somewhat implied, but makes our call to the store easier + to_id = await self.store.get_current_quarantined_media_stream_id() changes = await self.store.get_quarantined_media_changes( from_id=from_id, @@ -264,8 +262,10 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: for c in changes ] - # `from` is exclusive, so don't +1 - next_batch = max(c.stream_id for c in changes) if changes else from_id + # `from` is exclusive, so don't +1 this. We also know the last record will have + # the highest stream ID, so use that one. If there aren't any records, just + # return the `from` value. + next_batch = changes[-1].stream_id if len(changes) > 0 else from_id return HTTPStatus.OK, {"next_batch": next_batch, "rows": rows} diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index bacb3e352b1..3a2013d7cdf 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -181,7 +181,7 @@ def __init__( instance_name=self._instance_name, tables=[("quarantined_media_changes", "instance_name", "stream_id")], sequence_name="quarantined_media_id_seq", - writers=[], # we don't use `get_current_token` or `get_positions`, per docs + writers=hs.config.worker.writers.quarantined_media_changes, ) # Register a background update to flag already-quarantined media in the quaranine @@ -1238,6 +1238,14 @@ def _get_media_ids_by_user_txn( return local_media_ids + async def get_current_quarantined_media_stream_id(self) -> int: + """Gets the position of the quarantined media changes stream. + + Returns: + int - the current stream ID + """ + return self._quarantined_media_changes_id_gen.get_current_token() + async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: From 282e6709402d7c87a8bfaa1ac170bb6c870570f4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 19:56:49 -0600 Subject: [PATCH 11/68] Add extra safety --- synapse/storage/databases/main/room.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 3a2013d7cdf..b11c303acc8 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -172,6 +172,10 @@ def __init__( writers=["master"], ) + self._can_write_quarantined_media_changes = ( + self._instance_name in hs.config.worker.writers.quarantined_media_changes + ) + self._quarantined_media_changes_id_gen: MultiWriterIdGenerator = MultiWriterIdGenerator( db_conn=db_conn, db=database, @@ -1277,6 +1281,7 @@ def _get_quarantined_media_changes_txn( SELECT stream_id, origin, media_id, quarantined FROM quarantined_media_changes WHERE ? < stream_id AND stream_id <= ? + ORDER BY stream_id ASC LIMIT ? """, (from_id, to_id, limit), @@ -1305,6 +1310,7 @@ def _insert_quarantine_change_txn( may be None if the media is local. quarantined: Whether the media is being quarantined or unquarantined. """ + assert self._can_write_quarantined_media_changes medias_with_stream_ids = zip( origins_and_media_ids, self._quarantined_media_changes_id_gen.get_next_mult_txn( From 616e9c8515aec7c36b2aac587ff0a1b28f13cd4c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 20:15:34 -0600 Subject: [PATCH 12/68] Split and expand tests --- tests/rest/admin/test_media.py | 79 ++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 28 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 6828d9c82d5..df37c0f3f60 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -790,24 +790,7 @@ def test_requester_is_not_admin(self) -> None: self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) - def test_list_quarantined_media(self) -> None: - """ - Ensure we actually get results for each page. We can't really test that - remote media is quarantined, but we can test that local media is. - """ - self.admin_user = self.register_user("admin", "pass", admin=True) - self.admin_user_tok = self.login("admin", "pass") - - def _upload() -> str: - return self.helper.upload_media( - SMALL_PNG, tok=self.admin_user_tok, expect_code=200 - )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain - - self.media_id_1 = _upload() - self.media_id_2 = _upload() - self.media_id_3 = _upload() - - def _quarantine(media_id: str) -> None: + def _quarantine_local_media(self, media_id: str, admin_user_tok: str) -> None: channel = self.make_request( "POST", "/_synapse/admin/v1/media/quarantine/%s/%s" @@ -815,10 +798,27 @@ def _quarantine(media_id: str) -> None: self.server_name, media_id, ), - access_token=self.admin_user_tok, + access_token=admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) + def _local_upload(self, admin_user_tok: str) -> str: + return self.helper.upload_media( + SMALL_PNG, tok=admin_user_tok, expect_code=200 + )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + + def test_list_quarantined_media(self) -> None: + """ + Ensure we actually get results for each page and that pagination is seamless. + We can't really test that remote media is quarantined, but we can test that local + media is. + """ + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + # Upload 105 media objects to test multiple pages + self.media_ids = [self._local_upload(self.admin_user_tok) for _ in range(105)] + # No rows before quarantine channel = self.make_request( "GET", @@ -828,8 +828,11 @@ def _quarantine(media_id: str) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(0, len(channel.json_body["rows"])) - # Quarantine by hash should kick in to get the other two media objects - _quarantine(self.media_id_1) + # We expect to continue from `from` because we have no rows + self.assertEqual(0, channel.json_body["next_batch"]) + + # Quarantine by hash should kick in to get the other 104 media objects + self._quarantine_local_media(self.media_ids[0], self.admin_user_tok) # Page 1 (implied ?from=0) channel = self.make_request( @@ -838,28 +841,39 @@ def _quarantine(media_id: str) -> None: access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(3, len(channel.json_body["rows"])) + self.assertEqual(100, len(channel.json_body["rows"])) + self.assertEqual(103, channel.json_body["next_batch"]) # streams start at 2 for row in channel.json_body["rows"]: self.assertIn( - row["media_id"], (self.media_id_1, self.media_id_2, self.media_id_3) + row["media_id"], self.media_ids[0:100], ) self.assertEqual(row["origin"], self.server_name) self.assertEqual(row["quarantined"], True) - # Page 1 (explicit ?from) + # Page 2 (explicit ?from, using next_batch) channel = self.make_request( "GET", - "/_synapse/admin/v1/media/quarantine_changes?from=2", # streams start at 2 + "/_synapse/admin/v1/media/quarantine_changes?from=103", access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(2, len(channel.json_body["rows"])) + self.assertEqual(5, len(channel.json_body["rows"])) + self.assertEqual(106, channel.json_body["next_batch"]) for row in channel.json_body["rows"]: - self.assertIn(row["media_id"], (self.media_id_2, self.media_id_3)) + self.assertIn( + row["media_id"], self.media_ids[100:], + ) self.assertEqual(row["origin"], self.server_name) self.assertEqual(row["quarantined"], True) - # Page 2 (?from out of range) + def test_list_quarantined_media_bounds(self) -> None: + """ + Ensure out of bounds requests are handled gracefully. + """ + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + # Page that's very much out of range, so should have no results channel = self.make_request( "GET", "/_synapse/admin/v1/media/quarantine_changes?from=900000", @@ -868,6 +882,15 @@ def _quarantine(media_id: str) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(0, len(channel.json_body["rows"])) + # The same, but negative + channel = self.make_request( + "GET", + "/_synapse/admin/v1/media/quarantine_changes?from=-1", + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(0, len(channel.json_body["rows"])) + class QuarantineMediaByIDTestCase(_AdminMediaTests): def upload_media_and_return_media_id(self, data: bytes) -> str: From 914e252e79e5c0a80ff3275842e1243014402580 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Tue, 31 Mar 2026 02:26:25 +0000 Subject: [PATCH 13/68] Attempt to fix linting --- synapse/storage/databases/main/room.py | 22 ++++++++++-------- tests/rest/admin/test_media.py | 32 ++++++++++++++------------ 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index b11c303acc8..79740212651 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -176,16 +176,18 @@ def __init__( self._instance_name in hs.config.worker.writers.quarantined_media_changes ) - self._quarantined_media_changes_id_gen: MultiWriterIdGenerator = MultiWriterIdGenerator( - db_conn=db_conn, - db=database, - notifier=hs.get_replication_notifier(), - stream_name=QuarantinedMediaStream.NAME, - server_name=self.server_name, - instance_name=self._instance_name, - tables=[("quarantined_media_changes", "instance_name", "stream_id")], - sequence_name="quarantined_media_id_seq", - writers=hs.config.worker.writers.quarantined_media_changes, + self._quarantined_media_changes_id_gen: MultiWriterIdGenerator = ( + MultiWriterIdGenerator( + db_conn=db_conn, + db=database, + notifier=hs.get_replication_notifier(), + stream_name=QuarantinedMediaStream.NAME, + server_name=self.server_name, + instance_name=self._instance_name, + tables=[("quarantined_media_changes", "instance_name", "stream_id")], + sequence_name="quarantined_media_id_seq", + writers=hs.config.worker.writers.quarantined_media_changes, + ) ) # Register a background update to flag already-quarantined media in the quaranine diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index df37c0f3f60..78d481a81c4 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -791,21 +791,21 @@ def test_requester_is_not_admin(self) -> None: self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def _quarantine_local_media(self, media_id: str, admin_user_tok: str) -> None: - channel = self.make_request( - "POST", - "/_synapse/admin/v1/media/quarantine/%s/%s" - % ( - self.server_name, - media_id, - ), - access_token=admin_user_tok, - ) - self.assertEqual(200, channel.code, msg=channel.json_body) + channel = self.make_request( + "POST", + "/_synapse/admin/v1/media/quarantine/%s/%s" + % ( + self.server_name, + media_id, + ), + access_token=admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) def _local_upload(self, admin_user_tok: str) -> str: - return self.helper.upload_media( - SMALL_PNG, tok=admin_user_tok, expect_code=200 - )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + return self.helper.upload_media(SMALL_PNG, tok=admin_user_tok, expect_code=200)[ + "content_uri" + ][6:].split("/")[1] # Cut off 'mxc://' and domain def test_list_quarantined_media(self) -> None: """ @@ -845,7 +845,8 @@ def test_list_quarantined_media(self) -> None: self.assertEqual(103, channel.json_body["next_batch"]) # streams start at 2 for row in channel.json_body["rows"]: self.assertIn( - row["media_id"], self.media_ids[0:100], + row["media_id"], + self.media_ids[0:100], ) self.assertEqual(row["origin"], self.server_name) self.assertEqual(row["quarantined"], True) @@ -861,7 +862,8 @@ def test_list_quarantined_media(self) -> None: self.assertEqual(106, channel.json_body["next_batch"]) for row in channel.json_body["rows"]: self.assertIn( - row["media_id"], self.media_ids[100:], + row["media_id"], + self.media_ids[100:], ) self.assertEqual(row["origin"], self.server_name) self.assertEqual(row["quarantined"], True) From 090e2206bbd95b4ff3b9e9ef95423815ea11f1be Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 20:27:38 -0600 Subject: [PATCH 14/68] bump ci From cb71d465a0d073cbad881ea0eccb29f1e945611a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 20:29:12 -0600 Subject: [PATCH 15/68] Move schema deltas --- .../03_quarantined_media_tracking.sql} | 0 .../03_quarantined_media_tracking_seq.sql.postgres} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename synapse/storage/schema/main/delta/{93/05_quarantined_media_tracking.sql => 94/03_quarantined_media_tracking.sql} (100%) rename synapse/storage/schema/main/delta/{93/05_quarantined_media_tracking_seq.sql.postgres => 94/03_quarantined_media_tracking_seq.sql.postgres} (100%) diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql similarity index 100% rename from synapse/storage/schema/main/delta/93/05_quarantined_media_tracking.sql rename to synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql diff --git a/synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking_seq.sql.postgres similarity index 100% rename from synapse/storage/schema/main/delta/93/05_quarantined_media_tracking_seq.sql.postgres rename to synapse/storage/schema/main/delta/94/03_quarantined_media_tracking_seq.sql.postgres From 07ec8f473f792ce8bd2e25863acac83e04e2575b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 30 Mar 2026 20:44:17 -0600 Subject: [PATCH 16/68] Fix tests --- tests/rest/admin/test_media.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 78d481a81c4..5041d2f238d 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -842,7 +842,7 @@ def test_list_quarantined_media(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(100, len(channel.json_body["rows"])) - self.assertEqual(103, channel.json_body["next_batch"]) # streams start at 2 + self.assertEqual(101, channel.json_body["next_batch"]) for row in channel.json_body["rows"]: self.assertIn( row["media_id"], @@ -854,7 +854,7 @@ def test_list_quarantined_media(self) -> None: # Page 2 (explicit ?from, using next_batch) channel = self.make_request( "GET", - "/_synapse/admin/v1/media/quarantine_changes?from=103", + "/_synapse/admin/v1/media/quarantine_changes?from=101", access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) @@ -890,8 +890,8 @@ def test_list_quarantined_media_bounds(self) -> None: "/_synapse/admin/v1/media/quarantine_changes?from=-1", access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(0, len(channel.json_body["rows"])) + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) class QuarantineMediaByIDTestCase(_AdminMediaTests): From d288ef5d0fb4349ccd25f6e26d4b028f75cfca32 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 31 Mar 2026 13:22:54 -0600 Subject: [PATCH 17/68] Apply changes from review comments --- docker/configure_workers_and_start.py | 2 +- synapse/replication/tcp/streams/_base.py | 3 +++ synapse/rest/admin/media.py | 5 ++++- synapse/storage/databases/main/room.py | 6 +++--- .../main/delta/94/03_quarantined_media_tracking.sql | 1 + tests/rest/admin/test_media.py | 11 ++++------- 6 files changed, 16 insertions(+), 12 deletions(-) diff --git a/docker/configure_workers_and_start.py b/docker/configure_workers_and_start.py index 1b8d4f9989e..26c8556eff4 100755 --- a/docker/configure_workers_and_start.py +++ b/docker/configure_workers_and_start.py @@ -119,7 +119,7 @@ }, "media_repository": { "app": "synapse.app.generic_worker", - "listener_resources": ["media", "client"], + "listener_resources": ["media", "client", "replication"], "endpoint_patterns": [ "^/_matrix/media/", "^/_synapse/admin/v1/purge_media_cache$", diff --git a/synapse/replication/tcp/streams/_base.py b/synapse/replication/tcp/streams/_base.py index c55cba3bad9..8dd505e984b 100644 --- a/synapse/replication/tcp/streams/_base.py +++ b/synapse/replication/tcp/streams/_base.py @@ -814,6 +814,9 @@ async def _update_function( class QuarantinedMediaStreamRow: """Row for QuarantinedMediaStream""" + # We store the origin and media_id as media is scoped to the origin and are uniquely + # identified by (origin, media_id). + origin: str media_id: str quarantined: bool diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index fe4bcd07749..bc1db591c50 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -231,7 +231,10 @@ async def on_POST( class ListQuarantineChanges(RestServlet): - """Lists the quarantine changes to media.""" + """Lists the quarantine changes to media. + + Uses the pagination format described by https://spec.matrix.org/v1.18/appendices/#pagination + """ PATTERNS = admin_patterns("/media/quarantine_changes$") diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 79740212651..f121761d8e8 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -106,7 +106,7 @@ class RoomStats(LargestRoomStats): @dataclass(frozen=True) class QuarantinedMediaUpdate: - stream_id: int + stream_id: int # for the quarantined_media_changes stream origin: str media_id: str quarantined: bool @@ -210,8 +210,8 @@ async def _flag_existing_quarantined_media( some point. Media which isn't quarantined has not changed state (as far as this function can tell). - When media was quarantined is not recorded, so this uses the current time as - the time when the change happened. + When media was quarantined is not recorded, so this inserts as if the media was + quarantined at the current time. Further, due to lack of timestamp or history, media which was quarantined then unquarantined will not be picked up by this background task. diff --git a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql index 311add3314e..5d5fa5f0152 100644 --- a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql +++ b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql @@ -20,6 +20,7 @@ CREATE TABLE quarantined_media_changes ( instance_name TEXT NOT NULL, -- Media origin. NULL if local media. + -- We store the origin and media_id as media is scoped to the origin and are uniquely identified by (origin, media_id). origin TEXT NULL, -- Media ID at the origin. diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 5041d2f238d..9c045370b4e 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -760,6 +760,8 @@ class ListQuarantinedMediaChangesTestCase(_AdminMediaTests): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main self.server_name = hs.hostname + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") def test_no_auth(self) -> None: """ @@ -813,9 +815,6 @@ def test_list_quarantined_media(self) -> None: We can't really test that remote media is quarantined, but we can test that local media is. """ - self.admin_user = self.register_user("admin", "pass", admin=True) - self.admin_user_tok = self.login("admin", "pass") - # Upload 105 media objects to test multiple pages self.media_ids = [self._local_upload(self.admin_user_tok) for _ in range(105)] @@ -854,7 +853,7 @@ def test_list_quarantined_media(self) -> None: # Page 2 (explicit ?from, using next_batch) channel = self.make_request( "GET", - "/_synapse/admin/v1/media/quarantine_changes?from=101", + f"/_synapse/admin/v1/media/quarantine_changes?from={channel.json_body['next_batch']}", access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) @@ -872,9 +871,6 @@ def test_list_quarantined_media_bounds(self) -> None: """ Ensure out of bounds requests are handled gracefully. """ - self.admin_user = self.register_user("admin", "pass", admin=True) - self.admin_user_tok = self.login("admin", "pass") - # Page that's very much out of range, so should have no results channel = self.make_request( "GET", @@ -883,6 +879,7 @@ def test_list_quarantined_media_bounds(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(0, len(channel.json_body["rows"])) + self.assertEqual(900000, channel.json_body["next_batch"]) # The same, but negative channel = self.make_request( From 471b3dc4c59b6f3dbb20243bcf8e22ff76e8dc66 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Tue, 31 Mar 2026 19:27:59 +0000 Subject: [PATCH 18/68] Attempt to fix linting --- rust/src/http_client.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/src/http_client.rs b/rust/src/http_client.rs index 398ba9041f2..68283e7316b 100644 --- a/rust/src/http_client.rs +++ b/rust/src/http_client.rs @@ -245,10 +245,7 @@ impl HttpClient { .await .map_err(anyhow::Error::from_boxed) .with_context(|| { - format!( - "Response body exceeded response limit ({} bytes)", - response_limit - ) + format!("Response body exceeded response limit ({response_limit} bytes)") })?; let bytes: bytes::Bytes = collected.to_bytes(); From 373ed830023486b1bf3d15fb8ace075aa932235f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 15:54:20 -0600 Subject: [PATCH 19/68] Record quarantine changes in more sites --- synapse/media/media_repository.py | 3 +++ synapse/media/url_previewer.py | 1 + synapse/storage/databases/main/room.py | 15 +++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/synapse/media/media_repository.py b/synapse/media/media_repository.py index 3344e4c7beb..e550c717d97 100644 --- a/synapse/media/media_repository.py +++ b/synapse/media/media_repository.py @@ -422,6 +422,9 @@ async def create_or_update_content( quarantined_by="system" if should_quarantine else None, ) + if should_quarantine: + await self.store.record_media_quarantine_change(None, media_id, True) + try: await self._generate_thumbnails(None, media_id, media_id, media_type) except Exception as e: diff --git a/synapse/media/url_previewer.py b/synapse/media/url_previewer.py index 7782905a7ab..e4324357602 100644 --- a/synapse/media/url_previewer.py +++ b/synapse/media/url_previewer.py @@ -621,6 +621,7 @@ async def _handle_url( logger.warning( "Media has been automatically quarantined as it matched existing quarantined media" ) + await self.store.record_media_quarantine_change(None, file_id, True) time_now_ms = self.clock.time_msec() diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index f121761d8e8..d936760ee8c 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1298,6 +1298,21 @@ def _get_quarantined_media_changes_txn( for stream_id, origin, media_id, quarantined in txn ] + async def record_media_quarantine_change(self, origin: str | None, media_id: str, quarantined: bool) -> None: + """Records a change to the quarantine state of a piece of media. + + Args: + origin: The media origin, or None if the media is local. + media_id: The media ID at the origin. + quarantined: Whether the media is being quarantined or unquarantined. + """ + return await self.db_pool.runInteraction( + "record_media_quarantine_change", + self._insert_quarantine_change_txn, + [[origin, media_id]], + quarantined, + ) + def _insert_quarantine_change_txn( self, txn: LoggingTransaction, From e5791a328b64f56e88a29b46e9a1c8e6d549b0d6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 16:04:11 -0600 Subject: [PATCH 20/68] spelling --- synapse/storage/databases/main/room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index d936760ee8c..3448beda797 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -190,7 +190,7 @@ def __init__( ) ) - # Register a background update to flag already-quarantined media in the quaranine + # Register a background update to flag already-quarantined media in the quarantine # media changes table. This is to populate the API endpoint which consumes the # table with initial data that callers expect (namely, a list of currently # quarantined media). From 067f659c04e4313477dfde1a012dce9df9824a2f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 16:04:25 -0600 Subject: [PATCH 21/68] Apply suggestions from code review Co-authored-by: Eric Eastwood --- docs/workers.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/workers.md b/docs/workers.md index c16d4def034..6fe7887b6b4 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -578,8 +578,11 @@ configured as stream writer for the `device_lists` stream: ##### The `quarantined_media_changes` stream -[`synapse.app.media_repository`](#synapseappmedia_repository) workers should be -configured as stream writers for the `quarantined_media_changes` stream. +The `quarantined_media_changes` stream supports multiple writers. The following endpoints +can be handled by any worker, but should be routed directly to one of the workers +configured as stream writer for the `quarantined_media_changes` stream: + + ^/_synapse/admin/v1/quarantine_media/.*$ #### Restrict outbound federation traffic to a specific set of workers From 99b4bf24560f21f662a6e9402d9c51a161333817 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:16:05 +0000 Subject: [PATCH 22/68] Attempt to fix linting --- synapse/storage/databases/main/room.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 3448beda797..df6e3aeb769 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1298,7 +1298,9 @@ def _get_quarantined_media_changes_txn( for stream_id, origin, media_id, quarantined in txn ] - async def record_media_quarantine_change(self, origin: str | None, media_id: str, quarantined: bool) -> None: + async def record_media_quarantine_change( + self, origin: str | None, media_id: str, quarantined: bool + ) -> None: """Records a change to the quarantine state of a piece of media. Args: From 5a4ac328bb38cadce51d44e2c9b4c76f7761c1f2 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 16:26:00 -0600 Subject: [PATCH 23/68] bump ci From 5eac826aee4b5d3ff6b8d33ae8efd3f9e3a27f61 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 16:35:55 -0600 Subject: [PATCH 24/68] Use `Token` type --- synapse/replication/tcp/streams/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/replication/tcp/streams/_base.py b/synapse/replication/tcp/streams/_base.py index 8dd505e984b..a73f767add2 100644 --- a/synapse/replication/tcp/streams/_base.py +++ b/synapse/replication/tcp/streams/_base.py @@ -837,7 +837,7 @@ def __init__(self, hs: "HomeServer"): ) async def _update_function( - self, instance_name: str, from_token: int, to_token: int, limit: int + self, instance_name: str, from_token: Token, to_token: Token, limit: int ) -> StreamUpdateResult: updates = await self.store.get_quarantined_media_changes( from_id=from_token, to_id=to_token, limit=limit From b755aae93efa950eef1c61669d2db71c8c68f7ed Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 16:59:50 -0600 Subject: [PATCH 25/68] Add more token stuff --- synapse/storage/databases/main/room.py | 13 +++++++++++++ synapse/streams/events.py | 1 + synapse/types/__init__.py | 1 + 3 files changed, 15 insertions(+) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index df6e3aeb769..c4ab4be04f7 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -157,6 +157,8 @@ def __init__( self.config: HomeServerConfig = hs.config + self._replication = hs.get_replication_data_handler() + self._un_partial_stated_rooms_stream_id_gen: MultiWriterIdGenerator self._un_partial_stated_rooms_stream_id_gen = MultiWriterIdGenerator( @@ -1252,6 +1254,9 @@ async def get_current_quarantined_media_stream_id(self) -> int: """ return self._quarantined_media_changes_id_gen.get_current_token() + def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: + return self._quarantined_media_changes_id_gen + async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: @@ -1267,6 +1272,14 @@ async def get_quarantined_media_changes( Returns: list of QuarantinedMediaUpdate update rows in stream ordering. """ + + # Wait to ensure the current worker can actually read the stream up to to_id + await self._replication.wait_for_stream_position( + self._instance_name, + QuarantinedMediaStream.NAME, + to_id, + ) + return await self.db_pool.runInteraction( "get_quarantined_media_changes", self._get_quarantined_media_changes_txn, diff --git a/synapse/streams/events.py b/synapse/streams/events.py index d2720fb9592..81d23ac7b95 100644 --- a/synapse/streams/events.py +++ b/synapse/streams/events.py @@ -128,6 +128,7 @@ async def bound_future_token(self, token: StreamToken) -> StreamToken: StreamKeyType.UN_PARTIAL_STATED_ROOMS: self.store.get_un_partial_stated_rooms_id_generator(), StreamKeyType.THREAD_SUBSCRIPTIONS: self.store.get_thread_subscriptions_stream_id_generator(), StreamKeyType.STICKY_EVENTS: self.store.get_sticky_events_stream_id_generator(), + StreamKeyType.QUARANTINED_MEDIA: self.store.get_quarantined_media_stream_id_generator(), } for _, key in StreamKeyType.__members__.items(): diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 02889795bbd..10fdd4c74a4 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1057,6 +1057,7 @@ class StreamKeyType(Enum): UN_PARTIAL_STATED_ROOMS = "un_partial_stated_rooms_key" THREAD_SUBSCRIPTIONS = "thread_subscriptions_key" STICKY_EVENTS = "sticky_events_key" + QUARANTINED_MEDIA = "quarantined_media_key" @attr.s(slots=True, frozen=True, auto_attribs=True) From c15769737fa3fcae95ff69f5d935c48d352da350 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:06:34 -0600 Subject: [PATCH 26/68] split tests, again --- tests/rest/admin/test_media.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 9c045370b4e..e73b041369e 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -812,8 +812,6 @@ def _local_upload(self, admin_user_tok: str) -> str: def test_list_quarantined_media(self) -> None: """ Ensure we actually get results for each page and that pagination is seamless. - We can't really test that remote media is quarantined, but we can test that local - media is. """ # Upload 105 media objects to test multiple pages self.media_ids = [self._local_upload(self.admin_user_tok) for _ in range(105)] @@ -867,9 +865,9 @@ def test_list_quarantined_media(self) -> None: self.assertEqual(row["origin"], self.server_name) self.assertEqual(row["quarantined"], True) - def test_list_quarantined_media_bounds(self) -> None: + def test_list_quarantined_media_bounds_high(self) -> None: """ - Ensure out of bounds requests are handled gracefully. + Ensure out of bounds requests with high `from` values are handled gracefully. """ # Page that's very much out of range, so should have no results channel = self.make_request( @@ -881,7 +879,11 @@ def test_list_quarantined_media_bounds(self) -> None: self.assertEqual(0, len(channel.json_body["rows"])) self.assertEqual(900000, channel.json_body["next_batch"]) - # The same, but negative + def test_list_quarantined_media_bounds_low(self) -> None: + """ + Ensure out of bounds requests with low `from` values are handled gracefully. + """ + # The same as above, but negative channel = self.make_request( "GET", "/_synapse/admin/v1/media/quarantine_changes?from=-1", From dbc3445278012aa35ecc29f4bce5d5cfcc314a7b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:25:48 -0600 Subject: [PATCH 27/68] Use multi stream tokens to fix types? --- synapse/replication/tcp/streams/_base.py | 2 +- synapse/rest/admin/media.py | 10 ++++---- synapse/storage/databases/main/room.py | 29 +++++++++++++++++++++--- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/synapse/replication/tcp/streams/_base.py b/synapse/replication/tcp/streams/_base.py index a73f767add2..56dbaa322e1 100644 --- a/synapse/replication/tcp/streams/_base.py +++ b/synapse/replication/tcp/streams/_base.py @@ -833,7 +833,7 @@ def __init__(self, hs: "HomeServer"): super().__init__( hs.get_instance_name(), self._update_function, - self.store._quarantined_media_changes_id_gen, + self.store.get_quarantined_media_stream_id_generator(), ) async def _update_function( diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index bc1db591c50..a38fbbc6ab3 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -43,7 +43,7 @@ from synapse.storage.databases.main.media_repository import ( MediaSortOrder, ) -from synapse.types import JsonDict, UserID +from synapse.types import JsonDict, UserID, MultiWriterStreamToken if TYPE_CHECKING: from synapse.server import HomeServer @@ -248,11 +248,11 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: from_id = parse_integer(request, "from", default=0) limit = 100 # arbitrary; not enough to cause problems (hopefully) - to_id = await self.store.get_current_quarantined_media_stream_id() + to_token = await self.store.get_current_quarantined_media_stream_id() - changes = await self.store.get_quarantined_media_changes( - from_id=from_id, - to_id=to_id, + changes = await self.store.get_quarantined_media_changes_between_tokens( + from_token=MultiWriterStreamToken(stream=from_id), + to_token=to_token, limit=limit, ) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index c4ab4be04f7..7d4af721d32 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,7 +60,8 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import JsonDict, RetentionPolicy, StrCollection, ThirdPartyInstanceID +from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ + ThirdPartyInstanceID, MultiWriterStreamToken from synapse.util.caches.descriptors import cached, cachedList from synapse.util.json import json_encoder from synapse.util.stringutils import MXC_REGEX @@ -1246,17 +1247,39 @@ def _get_media_ids_by_user_txn( return local_media_ids - async def get_current_quarantined_media_stream_id(self) -> int: + async def get_current_quarantined_media_stream_id(self) -> MultiWriterStreamToken: """Gets the position of the quarantined media changes stream. Returns: int - the current stream ID """ - return self._quarantined_media_changes_id_gen.get_current_token() + return MultiWriterStreamToken.from_generator(self._quarantined_media_changes_id_gen) def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: return self._quarantined_media_changes_id_gen + async def get_quarantined_media_changes_between_tokens( + self, *, from_token: MultiWriterStreamToken, to_token: MultiWriterStreamToken, limit: int + ) -> list[QuarantinedMediaUpdate]: + """Get updates to quarantined media between two stream tokens. + + Bounds: from_token < ... <= to_token + + Args: + from_token: The starting stream token (exclusive) + to_token: The ending stream token (inclusive) + limit: The maximum number of rows to return + + Returns: + list of QuarantinedMediaUpdate update rows in stream ordering. + """ + assert from_token.is_before_or_eq(to_token) + return await self.get_quarantined_media_changes( + from_id=from_token.stream, + to_id=to_token.stream, + limit=limit, + ) + async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: From 668872369e22ed81c642491e0c3fef05d1d9465e Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Wed, 1 Apr 2026 23:28:07 +0000 Subject: [PATCH 28/68] Attempt to fix linting --- synapse/rest/admin/media.py | 2 +- synapse/storage/databases/main/room.py | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index a38fbbc6ab3..45350c3830a 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -43,7 +43,7 @@ from synapse.storage.databases.main.media_repository import ( MediaSortOrder, ) -from synapse.types import JsonDict, UserID, MultiWriterStreamToken +from synapse.types import JsonDict, MultiWriterStreamToken, UserID if TYPE_CHECKING: from synapse.server import HomeServer diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 7d4af721d32..27e0567bb1e 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,8 +60,13 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ - ThirdPartyInstanceID, MultiWriterStreamToken +from synapse.types import ( + JsonDict, + MultiWriterStreamToken, + RetentionPolicy, + StrCollection, + ThirdPartyInstanceID, +) from synapse.util.caches.descriptors import cached, cachedList from synapse.util.json import json_encoder from synapse.util.stringutils import MXC_REGEX @@ -1253,13 +1258,19 @@ async def get_current_quarantined_media_stream_id(self) -> MultiWriterStreamToke Returns: int - the current stream ID """ - return MultiWriterStreamToken.from_generator(self._quarantined_media_changes_id_gen) + return MultiWriterStreamToken.from_generator( + self._quarantined_media_changes_id_gen + ) def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: return self._quarantined_media_changes_id_gen async def get_quarantined_media_changes_between_tokens( - self, *, from_token: MultiWriterStreamToken, to_token: MultiWriterStreamToken, limit: int + self, + *, + from_token: MultiWriterStreamToken, + to_token: MultiWriterStreamToken, + limit: int, ) -> list[QuarantinedMediaUpdate]: """Get updates to quarantined media between two stream tokens. From 237f0a6e047e16a855db87434278dc0537baec11 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:29:15 -0600 Subject: [PATCH 29/68] Revert "Attempt to fix linting" This reverts commit 668872369e22ed81c642491e0c3fef05d1d9465e. --- synapse/rest/admin/media.py | 2 +- synapse/storage/databases/main/room.py | 19 ++++--------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 45350c3830a..a38fbbc6ab3 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -43,7 +43,7 @@ from synapse.storage.databases.main.media_repository import ( MediaSortOrder, ) -from synapse.types import JsonDict, MultiWriterStreamToken, UserID +from synapse.types import JsonDict, UserID, MultiWriterStreamToken if TYPE_CHECKING: from synapse.server import HomeServer diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 27e0567bb1e..7d4af721d32 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,13 +60,8 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import ( - JsonDict, - MultiWriterStreamToken, - RetentionPolicy, - StrCollection, - ThirdPartyInstanceID, -) +from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ + ThirdPartyInstanceID, MultiWriterStreamToken from synapse.util.caches.descriptors import cached, cachedList from synapse.util.json import json_encoder from synapse.util.stringutils import MXC_REGEX @@ -1258,19 +1253,13 @@ async def get_current_quarantined_media_stream_id(self) -> MultiWriterStreamToke Returns: int - the current stream ID """ - return MultiWriterStreamToken.from_generator( - self._quarantined_media_changes_id_gen - ) + return MultiWriterStreamToken.from_generator(self._quarantined_media_changes_id_gen) def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: return self._quarantined_media_changes_id_gen async def get_quarantined_media_changes_between_tokens( - self, - *, - from_token: MultiWriterStreamToken, - to_token: MultiWriterStreamToken, - limit: int, + self, *, from_token: MultiWriterStreamToken, to_token: MultiWriterStreamToken, limit: int ) -> list[QuarantinedMediaUpdate]: """Get updates to quarantined media between two stream tokens. From 190bda83c1caa2586ba3ed8bfb87854750fe2961 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:29:23 -0600 Subject: [PATCH 30/68] Revert "Use multi stream tokens to fix types?" This reverts commit dbc3445278012aa35ecc29f4bce5d5cfcc314a7b. --- synapse/replication/tcp/streams/_base.py | 2 +- synapse/rest/admin/media.py | 10 ++++---- synapse/storage/databases/main/room.py | 29 +++--------------------- 3 files changed, 9 insertions(+), 32 deletions(-) diff --git a/synapse/replication/tcp/streams/_base.py b/synapse/replication/tcp/streams/_base.py index 56dbaa322e1..a73f767add2 100644 --- a/synapse/replication/tcp/streams/_base.py +++ b/synapse/replication/tcp/streams/_base.py @@ -833,7 +833,7 @@ def __init__(self, hs: "HomeServer"): super().__init__( hs.get_instance_name(), self._update_function, - self.store.get_quarantined_media_stream_id_generator(), + self.store._quarantined_media_changes_id_gen, ) async def _update_function( diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index a38fbbc6ab3..bc1db591c50 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -43,7 +43,7 @@ from synapse.storage.databases.main.media_repository import ( MediaSortOrder, ) -from synapse.types import JsonDict, UserID, MultiWriterStreamToken +from synapse.types import JsonDict, UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -248,11 +248,11 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: from_id = parse_integer(request, "from", default=0) limit = 100 # arbitrary; not enough to cause problems (hopefully) - to_token = await self.store.get_current_quarantined_media_stream_id() + to_id = await self.store.get_current_quarantined_media_stream_id() - changes = await self.store.get_quarantined_media_changes_between_tokens( - from_token=MultiWriterStreamToken(stream=from_id), - to_token=to_token, + changes = await self.store.get_quarantined_media_changes( + from_id=from_id, + to_id=to_id, limit=limit, ) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 7d4af721d32..c4ab4be04f7 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,8 +60,7 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ - ThirdPartyInstanceID, MultiWriterStreamToken +from synapse.types import JsonDict, RetentionPolicy, StrCollection, ThirdPartyInstanceID from synapse.util.caches.descriptors import cached, cachedList from synapse.util.json import json_encoder from synapse.util.stringutils import MXC_REGEX @@ -1247,39 +1246,17 @@ def _get_media_ids_by_user_txn( return local_media_ids - async def get_current_quarantined_media_stream_id(self) -> MultiWriterStreamToken: + async def get_current_quarantined_media_stream_id(self) -> int: """Gets the position of the quarantined media changes stream. Returns: int - the current stream ID """ - return MultiWriterStreamToken.from_generator(self._quarantined_media_changes_id_gen) + return self._quarantined_media_changes_id_gen.get_current_token() def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: return self._quarantined_media_changes_id_gen - async def get_quarantined_media_changes_between_tokens( - self, *, from_token: MultiWriterStreamToken, to_token: MultiWriterStreamToken, limit: int - ) -> list[QuarantinedMediaUpdate]: - """Get updates to quarantined media between two stream tokens. - - Bounds: from_token < ... <= to_token - - Args: - from_token: The starting stream token (exclusive) - to_token: The ending stream token (inclusive) - limit: The maximum number of rows to return - - Returns: - list of QuarantinedMediaUpdate update rows in stream ordering. - """ - assert from_token.is_before_or_eq(to_token) - return await self.get_quarantined_media_changes( - from_id=from_token.stream, - to_id=to_token.stream, - limit=limit, - ) - async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: From ef63a72b17bda8c55615f3054ed60b6dc6db5543 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:30:41 -0600 Subject: [PATCH 31/68] Partial revert "Add more token stuff" This reverts commit b755aae93efa950eef1c61669d2db71c8c68f7ed. --- synapse/storage/databases/main/room.py | 5 ----- synapse/streams/events.py | 1 - synapse/types/__init__.py | 1 - 3 files changed, 7 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index c4ab4be04f7..17c43931691 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -157,8 +157,6 @@ def __init__( self.config: HomeServerConfig = hs.config - self._replication = hs.get_replication_data_handler() - self._un_partial_stated_rooms_stream_id_gen: MultiWriterIdGenerator self._un_partial_stated_rooms_stream_id_gen = MultiWriterIdGenerator( @@ -1254,9 +1252,6 @@ async def get_current_quarantined_media_stream_id(self) -> int: """ return self._quarantined_media_changes_id_gen.get_current_token() - def get_quarantined_media_stream_id_generator(self) -> MultiWriterIdGenerator: - return self._quarantined_media_changes_id_gen - async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: diff --git a/synapse/streams/events.py b/synapse/streams/events.py index 81d23ac7b95..d2720fb9592 100644 --- a/synapse/streams/events.py +++ b/synapse/streams/events.py @@ -128,7 +128,6 @@ async def bound_future_token(self, token: StreamToken) -> StreamToken: StreamKeyType.UN_PARTIAL_STATED_ROOMS: self.store.get_un_partial_stated_rooms_id_generator(), StreamKeyType.THREAD_SUBSCRIPTIONS: self.store.get_thread_subscriptions_stream_id_generator(), StreamKeyType.STICKY_EVENTS: self.store.get_sticky_events_stream_id_generator(), - StreamKeyType.QUARANTINED_MEDIA: self.store.get_quarantined_media_stream_id_generator(), } for _, key in StreamKeyType.__members__.items(): diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 10fdd4c74a4..02889795bbd 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -1057,7 +1057,6 @@ class StreamKeyType(Enum): UN_PARTIAL_STATED_ROOMS = "un_partial_stated_rooms_key" THREAD_SUBSCRIPTIONS = "thread_subscriptions_key" STICKY_EVENTS = "sticky_events_key" - QUARANTINED_MEDIA = "quarantined_media_key" @attr.s(slots=True, frozen=True, auto_attribs=True) From 519512c7567f9bf1cc54862356e53d4ea875207a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:34:28 -0600 Subject: [PATCH 32/68] we do need this though --- synapse/storage/databases/main/room.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 17c43931691..196a5545f8c 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -157,6 +157,8 @@ def __init__( self.config: HomeServerConfig = hs.config + self._replication = hs.get_replication_data_handler() + self._un_partial_stated_rooms_stream_id_gen: MultiWriterIdGenerator self._un_partial_stated_rooms_stream_id_gen = MultiWriterIdGenerator( From c36311a561ff4ca43858aa9b97994820124f9ae3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:39:33 -0600 Subject: [PATCH 33/68] Revert "we do need this though" This reverts commit 519512c7567f9bf1cc54862356e53d4ea875207a. --- synapse/storage/databases/main/room.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 196a5545f8c..17c43931691 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -157,8 +157,6 @@ def __init__( self.config: HomeServerConfig = hs.config - self._replication = hs.get_replication_data_handler() - self._un_partial_stated_rooms_stream_id_gen: MultiWriterIdGenerator self._un_partial_stated_rooms_stream_id_gen = MultiWriterIdGenerator( From 8d6d9c813e8eae98ca0fee37685acf4454ca8655 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:39:47 -0600 Subject: [PATCH 34/68] Revert "Add more token stuff" This reverts commit b755aae93efa950eef1c61669d2db71c8c68f7ed. --- synapse/storage/databases/main/room.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 17c43931691..df6e3aeb769 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1267,14 +1267,6 @@ async def get_quarantined_media_changes( Returns: list of QuarantinedMediaUpdate update rows in stream ordering. """ - - # Wait to ensure the current worker can actually read the stream up to to_id - await self._replication.wait_for_stream_position( - self._instance_name, - QuarantinedMediaStream.NAME, - to_id, - ) - return await self.db_pool.runInteraction( "get_quarantined_media_changes", self._get_quarantined_media_changes_txn, From 2de37446b14da3ed3791b6eaacb2c645be040b19 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:41:20 -0600 Subject: [PATCH 35/68] Move stream wait to servlet I guess --- synapse/rest/admin/media.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index bc1db591c50..83a5a86752e 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -35,6 +35,7 @@ parse_string, ) from synapse.http.site import SynapseRequest +from synapse.replication.tcp.streams import QuarantinedMediaStream from synapse.rest.admin._base import ( admin_patterns, assert_requester_is_admin, @@ -242,6 +243,7 @@ def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self.auth = hs.get_auth() self.server_name = hs.hostname + self.replication = hs.get_replication_data_handler() async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: await assert_requester_is_admin(self.auth, request) @@ -250,6 +252,12 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit = 100 # arbitrary; not enough to cause problems (hopefully) to_id = await self.store.get_current_quarantined_media_stream_id() + await self.replication.wait_for_stream_position( + self.server_name, + QuarantinedMediaStream.NAME, + to_id, + ) + changes = await self.store.get_quarantined_media_changes( from_id=from_id, to_id=to_id, From 2325d9b29a6827f1f9b242874bebc2abfb1411c2 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 1 Apr 2026 17:49:11 -0600 Subject: [PATCH 36/68] Remove excess change from lint fixing action --- rust/src/http_client.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rust/src/http_client.rs b/rust/src/http_client.rs index 68283e7316b..398ba9041f2 100644 --- a/rust/src/http_client.rs +++ b/rust/src/http_client.rs @@ -245,7 +245,10 @@ impl HttpClient { .await .map_err(anyhow::Error::from_boxed) .with_context(|| { - format!("Response body exceeded response limit ({response_limit} bytes)") + format!( + "Response body exceeded response limit ({} bytes)", + response_limit + ) })?; let bytes: bytes::Bytes = collected.to_bytes(); From 2558b6176ca881e3d4d8ce3a7b7078b5b33944a9 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 Apr 2026 16:07:13 -0600 Subject: [PATCH 37/68] API changes --- synapse/rest/admin/media.py | 22 ++++---- synapse/storage/databases/main/room.py | 75 +++++++++++++++++++++++++- tests/rest/admin/test_media.py | 19 ++++--- 3 files changed, 95 insertions(+), 21 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 83a5a86752e..b1e4a8cf10a 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -252,11 +252,15 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit = 100 # arbitrary; not enough to cause problems (hopefully) to_id = await self.store.get_current_quarantined_media_stream_id() - await self.replication.wait_for_stream_position( - self.server_name, - QuarantinedMediaStream.NAME, - to_id, - ) + if to_id < from_id or from_id < 0: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Query parameter from must be a positive integer and behind the current stream position.", + errcode=Codes.INVALID_PARAM, + ) + + # We need to wait to ensure that our current worker is actually caught up with + # the stream position, otherwise we might not return what we think we're returning. changes = await self.store.get_quarantined_media_changes( from_id=from_id, @@ -264,7 +268,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit=limit, ) - rows = [ + serialized_changes = [ { "origin": c.origin if c.origin is not None else self.server_name, "media_id": c.media_id, @@ -275,10 +279,10 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: # `from` is exclusive, so don't +1 this. We also know the last record will have # the highest stream ID, so use that one. If there aren't any records, just - # return the `from` value. - next_batch = changes[-1].stream_id if len(changes) > 0 else from_id + # return the `to_id` value because it'll be the furthest stream position possible. + next_batch = changes[-1].stream_id if len(changes) > 0 else to_id - return HTTPStatus.OK, {"next_batch": next_batch, "rows": rows} + return HTTPStatus.OK, {"next_batch": next_batch, "changes": serialized_changes} class ProtectMediaByID(RestServlet): diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index df6e3aeb769..2fafa129c4b 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,8 +60,10 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import JsonDict, RetentionPolicy, StrCollection, ThirdPartyInstanceID +from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ + ThirdPartyInstanceID, AbstractMultiWriterStreamToken from synapse.util.caches.descriptors import cached, cachedList +from synapse.util.duration import Duration from synapse.util.json import json_encoder from synapse.util.stringutils import MXC_REGEX @@ -1252,12 +1254,81 @@ async def get_current_quarantined_media_stream_id(self) -> int: """ return self._quarantined_media_changes_id_gen.get_current_token() + async def wait_for_quarantined_media_stream_id(self, target_id: int) -> bool: + """Waits until the quarantined media changes stream reaches the given stream ID. + + See https://github.com/element-hq/synapse/pull/19644 for more details. + + TODO: Replace function and call sites with https://github.com/element-hq/synapse/pull/19644 + + Args: + target_id: The stream ID to wait for. + + Returns: + True when caught up to the target stream ID. + False when timing out while waiting. + """ + # We ideally would use something like `wait_for_stream_position` in the meantime, + # but that short circuits if the instance name matches the current instance name. + # Doing so means that if *another* writer is actually leading the to_id, then we'll + # assume that we're caught up when we aren't. + # + # NOTE: Because this is implemented to wait for stream positions by integer ID, + # we're technically waiting for *all* workers to catch up rather than just waiting + # for *our* worker to catch up. This is okay for now because the quarantined media + # stream should be pretty fast to update, and if it's not then the only thing we're + # affecting is an admin API that probably has a tool automatically retrying requests + # anyway. https://github.com/element-hq/synapse/pull/19644 does the waiting properly + # so this should be replaced by that (or similar). + + # Get the minimum shared position/ID across all workers + current_id = self._quarantined_media_changes_id_gen.get_current_token() + if current_id >= target_id: + return True # nothing to wait for: we're already caught up. + + # "This should never happen". Tokens we hand out via the API should exist. If they + # don't, then we're in a bad state and need to explode. + max_persisted_position = await self._quarantined_media_changes_id_gen.get_max_allocated_token() + assert max_persisted_position >= target_id, ( + f"Unable to wait for invalid future token (token={target_id} has positions " + f"ahead of our max persisted position={max_persisted_position})" + ) + + # Start waiting until we've caught up to the `stream_token` + start = self.clock.time_msec() + logged = False + while True: + # Like above, get the minimum shared ID across all workers + current_id = self._quarantined_media_changes_id_gen.get_current_token() + if current_id >= target_id: + return True + + now = self.clock.time_msec() + + # Timed out + if now - start > 10_000: + return False + + if not logged: + logger.info( + "Waiting for current token to reach %s; currently at %s", + target_id, + current_id, + ) + logged = True + + # TODO: be better + await self.clock.sleep(Duration(milliseconds=500)) + async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: """Get updates to quarantined media between two stream IDs. - Bounds: from_id < ... <= to_id + QuarantinedMediaUpdates are returned in ascending stream order provided + the from_id is before the to_id. The returned list will exclude the from_id + but not the to_id, if an update shares a stream position with either of those + stream IDs. Args: from_id: The starting stream ID (exclusive) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index e73b041369e..39557c773ba 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -816,16 +816,16 @@ def test_list_quarantined_media(self) -> None: # Upload 105 media objects to test multiple pages self.media_ids = [self._local_upload(self.admin_user_tok) for _ in range(105)] - # No rows before quarantine + # No changes before quarantine channel = self.make_request( "GET", "/_synapse/admin/v1/media/quarantine_changes", access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(0, len(channel.json_body["rows"])) + self.assertEqual(0, len(channel.json_body["changes"])) - # We expect to continue from `from` because we have no rows + # We expect to continue from the current stream position because we have no changes self.assertEqual(0, channel.json_body["next_batch"]) # Quarantine by hash should kick in to get the other 104 media objects @@ -838,9 +838,9 @@ def test_list_quarantined_media(self) -> None: access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(100, len(channel.json_body["rows"])) + self.assertEqual(100, len(channel.json_body["changes"])) self.assertEqual(101, channel.json_body["next_batch"]) - for row in channel.json_body["rows"]: + for row in channel.json_body["changes"]: self.assertIn( row["media_id"], self.media_ids[0:100], @@ -855,9 +855,9 @@ def test_list_quarantined_media(self) -> None: access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(5, len(channel.json_body["rows"])) + self.assertEqual(5, len(channel.json_body["changes"])) self.assertEqual(106, channel.json_body["next_batch"]) - for row in channel.json_body["rows"]: + for row in channel.json_body["changes"]: self.assertIn( row["media_id"], self.media_ids[100:], @@ -875,9 +875,8 @@ def test_list_quarantined_media_bounds_high(self) -> None: "/_synapse/admin/v1/media/quarantine_changes?from=900000", access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(0, len(channel.json_body["rows"])) - self.assertEqual(900000, channel.json_body["next_batch"]) + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) def test_list_quarantined_media_bounds_low(self) -> None: """ From 7469b49336ca2ff1f7e8db637dd5b402eaeb82e2 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 Apr 2026 16:28:46 -0600 Subject: [PATCH 38/68] Change background update --- synapse/storage/databases/main/room.py | 62 +++++++++++++++++--------- 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 2fafa129c4b..e9d5bb1c59e 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -227,8 +227,14 @@ async def _flag_existing_quarantined_media( Returns: The number of rows inserted. """ - # The last row OFFSET we got to in the UNION query below - last_row_num: int = progress.get("last_row_num", 0) + # We track the progress for the local and remote tables separately just in case + # there are media ID collisions that would make a mess of `ORDER BY media_id + # LIMIT ?`. If there are collisions towards the end of the returned set, the LIMIT + # might cut off some rows and make a future call with `WHERE media_id > ?` miss + # them too. By tracking each table separately, we avoid this kind of issue. + last_local_media_id = progress.get("last_local_media_id", "") + last_remote_media_id = progress.get("last_remote_media_id", "") + last_remote_origin = progress.get("last_remote_origin", "") # The `ORDER BY` here would normally miss records if the admin (un)quarantined a # record, but that doesn't affect the background update because we also insert @@ -240,34 +246,50 @@ async def _flag_existing_quarantined_media( # Note: Already-quarantined media is indicated by the `quarantined_by` field being # non-null. We only want quarantined media, per docstring above. def flag_quarantined(txn: LoggingTransaction) -> int: - txn.execute( - """ + # It doesn't matter which order we do these in, as long as we do both of them. + txn.execute(""" SELECT NULL AS media_origin, media_id FROM local_media_repository WHERE quarantined_by IS NOT NULL + AND media_id > ? + ORDER BY media_id + LIMIT ? + """, + (last_local_media_id, batch_size) + ) + local_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) + if len(local_media_result) > 0: + self._insert_quarantine_change_txn(txn, local_media_result, True) - UNION - + # We use a >= ? on the media origin to avoid missing records when media IDs + # collide between origins. + txn.execute(""" SELECT media_origin, media_id FROM remote_media_cache WHERE quarantined_by IS NOT NULL - + AND media_origin >= ? AND media_id > ? ORDER BY media_origin, media_id - LIMIT ? OFFSET ? - """, - (batch_size, last_row_num), + LIMIT ? + """, + (last_remote_origin, last_remote_media_id, batch_size) ) - res = cast(list[tuple[str | None, str]], txn.fetchall()) - if len(res) > 0: - self._insert_quarantine_change_txn(txn, res, True) - self.db_pool.updates._background_update_progress_txn( - txn, - "flag_existing_quarantined_media", - {"last_row_num": last_row_num + len(res)}, - ) - return len(res) + remote_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) + if len(remote_media_result) > 0: + self._insert_quarantine_change_txn(txn, remote_media_result, True) + + self.db_pool._background_update_progress_txn( + txn, + "flag_existing_quarantined_media", + { + "last_local_media_id": local_media_result[-1][1] if len(local_media_result) > 0 else last_local_media_id, + "last_remote_media_id": remote_media_result[-1][1] if len(remote_media_result) > 0 else last_remote_media_id, + "last_remote_origin": remote_media_result[-1][0] if len(remote_media_result) > 0 else last_remote_origin, + } + ) + + return len(local_media_result) + len(remote_media_result) - logger.info("Flagging existing quarantined media with offset %s", last_row_num) + logger.info("Flagging existing quarantined media with local offset %s and remote offset %s/%s", last_local_media_id, last_remote_origin, last_remote_media_id) num_flagged = await self.db_pool.runInteraction( "_flag_existing_quarantined_media", flag_quarantined, From 8255d7b0131d0bd414075a7a766fb87e14f7458c Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Thu, 2 Apr 2026 22:57:44 +0000 Subject: [PATCH 39/68] Attempt to fix linting --- synapse/rest/admin/media.py | 1 - synapse/storage/databases/main/room.py | 43 +++++++++++++++++++------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index b1e4a8cf10a..4abb54332fd 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -35,7 +35,6 @@ parse_string, ) from synapse.http.site import SynapseRequest -from synapse.replication.tcp.streams import QuarantinedMediaStream from synapse.rest.admin._base import ( admin_patterns, assert_requester_is_admin, diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index e9d5bb1c59e..1890480f2aa 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -60,8 +60,12 @@ from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore from synapse.storage.types import Cursor from synapse.storage.util.id_generators import IdGenerator, MultiWriterIdGenerator -from synapse.types import JsonDict, RetentionPolicy, StrCollection, \ - ThirdPartyInstanceID, AbstractMultiWriterStreamToken +from synapse.types import ( + JsonDict, + RetentionPolicy, + StrCollection, + ThirdPartyInstanceID, +) from synapse.util.caches.descriptors import cached, cachedList from synapse.util.duration import Duration from synapse.util.json import json_encoder @@ -247,7 +251,8 @@ async def _flag_existing_quarantined_media( # non-null. We only want quarantined media, per docstring above. def flag_quarantined(txn: LoggingTransaction) -> int: # It doesn't matter which order we do these in, as long as we do both of them. - txn.execute(""" + txn.execute( + """ SELECT NULL AS media_origin, media_id FROM local_media_repository WHERE quarantined_by IS NOT NULL @@ -255,7 +260,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: ORDER BY media_id LIMIT ? """, - (last_local_media_id, batch_size) + (last_local_media_id, batch_size), ) local_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) if len(local_media_result) > 0: @@ -263,7 +268,8 @@ def flag_quarantined(txn: LoggingTransaction) -> int: # We use a >= ? on the media origin to avoid missing records when media IDs # collide between origins. - txn.execute(""" + txn.execute( + """ SELECT media_origin, media_id FROM remote_media_cache WHERE quarantined_by IS NOT NULL @@ -271,7 +277,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: ORDER BY media_origin, media_id LIMIT ? """, - (last_remote_origin, last_remote_media_id, batch_size) + (last_remote_origin, last_remote_media_id, batch_size), ) remote_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) if len(remote_media_result) > 0: @@ -281,15 +287,26 @@ def flag_quarantined(txn: LoggingTransaction) -> int: txn, "flag_existing_quarantined_media", { - "last_local_media_id": local_media_result[-1][1] if len(local_media_result) > 0 else last_local_media_id, - "last_remote_media_id": remote_media_result[-1][1] if len(remote_media_result) > 0 else last_remote_media_id, - "last_remote_origin": remote_media_result[-1][0] if len(remote_media_result) > 0 else last_remote_origin, - } + "last_local_media_id": local_media_result[-1][1] + if len(local_media_result) > 0 + else last_local_media_id, + "last_remote_media_id": remote_media_result[-1][1] + if len(remote_media_result) > 0 + else last_remote_media_id, + "last_remote_origin": remote_media_result[-1][0] + if len(remote_media_result) > 0 + else last_remote_origin, + }, ) return len(local_media_result) + len(remote_media_result) - logger.info("Flagging existing quarantined media with local offset %s and remote offset %s/%s", last_local_media_id, last_remote_origin, last_remote_media_id) + logger.info( + "Flagging existing quarantined media with local offset %s and remote offset %s/%s", + last_local_media_id, + last_remote_origin, + last_remote_media_id, + ) num_flagged = await self.db_pool.runInteraction( "_flag_existing_quarantined_media", flag_quarantined, @@ -1310,7 +1327,9 @@ async def wait_for_quarantined_media_stream_id(self, target_id: int) -> bool: # "This should never happen". Tokens we hand out via the API should exist. If they # don't, then we're in a bad state and need to explode. - max_persisted_position = await self._quarantined_media_changes_id_gen.get_max_allocated_token() + max_persisted_position = ( + await self._quarantined_media_changes_id_gen.get_max_allocated_token() + ) assert max_persisted_position >= target_id, ( f"Unable to wait for invalid future token (token={target_id} has positions " f"ahead of our max persisted position={max_persisted_position})" From 455f749b1458d01df03c4d3dc7cec1ea88b58d61 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 Apr 2026 17:07:00 -0600 Subject: [PATCH 40/68] *ahem* --- synapse/storage/databases/main/room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 1890480f2aa..aae0e23d06e 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -283,7 +283,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: if len(remote_media_result) > 0: self._insert_quarantine_change_txn(txn, remote_media_result, True) - self.db_pool._background_update_progress_txn( + self.db_pool.updates._background_update_progress_txn( txn, "flag_existing_quarantined_media", { From cbc6ec370a7abaee2fdcc75a17e38923e81b3ec7 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 Apr 2026 17:25:12 -0600 Subject: [PATCH 41/68] 1 --- tests/rest/admin/test_media.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 39557c773ba..b1713ec56b6 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -826,7 +826,7 @@ def test_list_quarantined_media(self) -> None: self.assertEqual(0, len(channel.json_body["changes"])) # We expect to continue from the current stream position because we have no changes - self.assertEqual(0, channel.json_body["next_batch"]) + self.assertEqual(1, channel.json_body["next_batch"]) # Quarantine by hash should kick in to get the other 104 media objects self._quarantine_local_media(self.media_ids[0], self.admin_user_tok) From 79b7a8aa334498270e21cfc18ff7c5ac31d061c7 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 10:20:02 -0600 Subject: [PATCH 42/68] Back out changes which try to capture additional sites --- synapse/media/media_repository.py | 3 --- synapse/media/url_previewer.py | 1 - synapse/storage/databases/main/room.py | 17 ----------------- 3 files changed, 21 deletions(-) diff --git a/synapse/media/media_repository.py b/synapse/media/media_repository.py index e550c717d97..3344e4c7beb 100644 --- a/synapse/media/media_repository.py +++ b/synapse/media/media_repository.py @@ -422,9 +422,6 @@ async def create_or_update_content( quarantined_by="system" if should_quarantine else None, ) - if should_quarantine: - await self.store.record_media_quarantine_change(None, media_id, True) - try: await self._generate_thumbnails(None, media_id, media_id, media_type) except Exception as e: diff --git a/synapse/media/url_previewer.py b/synapse/media/url_previewer.py index e4324357602..7782905a7ab 100644 --- a/synapse/media/url_previewer.py +++ b/synapse/media/url_previewer.py @@ -621,7 +621,6 @@ async def _handle_url( logger.warning( "Media has been automatically quarantined as it matched existing quarantined media" ) - await self.store.record_media_quarantine_change(None, file_id, True) time_now_ms = self.clock.time_msec() diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index aae0e23d06e..08b94fa32e3 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1410,23 +1410,6 @@ def _get_quarantined_media_changes_txn( for stream_id, origin, media_id, quarantined in txn ] - async def record_media_quarantine_change( - self, origin: str | None, media_id: str, quarantined: bool - ) -> None: - """Records a change to the quarantine state of a piece of media. - - Args: - origin: The media origin, or None if the media is local. - media_id: The media ID at the origin. - quarantined: Whether the media is being quarantined or unquarantined. - """ - return await self.db_pool.runInteraction( - "record_media_quarantine_change", - self._insert_quarantine_change_txn, - [[origin, media_id]], - quarantined, - ) - def _insert_quarantine_change_txn( self, txn: LoggingTransaction, From 6006bc14e204659c97893e616a86fb487a5fd418 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 10:23:47 -0600 Subject: [PATCH 43/68] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/rest/admin/media.py | 4 ++-- synapse/storage/databases/main/room.py | 17 ++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 4abb54332fd..4e07a3104a4 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -251,7 +251,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit = 100 # arbitrary; not enough to cause problems (hopefully) to_id = await self.store.get_current_quarantined_media_stream_id() - if to_id < from_id or from_id < 0: + if to_id < from_id: raise SynapseError( HTTPStatus.BAD_REQUEST, "Query parameter from must be a positive integer and behind the current stream position.", @@ -276,7 +276,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: for c in changes ] - # `from` is exclusive, so don't +1 this. We also know the last record will have + # We know the last record will have # the highest stream ID, so use that one. If there aren't any records, just # return the `to_id` value because it'll be the furthest stream position possible. next_batch = changes[-1].stream_id if len(changes) > 0 else to_id diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index aae0e23d06e..121b823d07a 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -267,7 +267,8 @@ def flag_quarantined(txn: LoggingTransaction) -> int: self._insert_quarantine_change_txn(txn, local_media_result, True) # We use a >= ? on the media origin to avoid missing records when media IDs - # collide between origins. + # collide between origins (the table's unique constraint is on `(media_origin, media_id)`). + # Filtering by `(media_origin, media_id)` also makes sure we're using an index. txn.execute( """ SELECT media_origin, media_id @@ -308,7 +309,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: last_remote_media_id, ) num_flagged = await self.db_pool.runInteraction( - "_flag_existing_quarantined_media", + "_flag_existing_quarantined_media.flag_quarantined", flag_quarantined, ) if num_flagged <= 0: # probably never negative, but why trust computers? @@ -1364,12 +1365,10 @@ async def wait_for_quarantined_media_stream_id(self, target_id: int) -> bool: async def get_quarantined_media_changes( self, *, from_id: int, to_id: int, limit: int ) -> list[QuarantinedMediaUpdate]: - """Get updates to quarantined media between two stream IDs. + """ + Get updates to quarantined media in stream ordering since `from_id`. - QuarantinedMediaUpdates are returned in ascending stream order provided - the from_id is before the to_id. The returned list will exclude the from_id - but not the to_id, if an update shares a stream position with either of those - stream IDs. + Paginating forwards: from_id < x <= to_id, (ascending order) Args: from_id: The starting stream ID (exclusive) @@ -1377,7 +1376,7 @@ async def get_quarantined_media_changes( limit: The maximum number of rows to return Returns: - list of QuarantinedMediaUpdate update rows in stream ordering. + List of `QuarantinedMediaUpdate` update rows in stream ordering (ascending order). """ return await self.db_pool.runInteraction( "get_quarantined_media_changes", @@ -1427,7 +1426,7 @@ async def record_media_quarantine_change( quarantined, ) - def _insert_quarantine_change_txn( + def _insert_quarantine_changes_txn( self, txn: LoggingTransaction, origins_and_media_ids: list[tuple[str | None, str]], From 4a7f8fae37009e293312fb0318e27df69cf969b4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 10:25:50 -0600 Subject: [PATCH 44/68] Fix from code review --- synapse/rest/admin/media.py | 6 +++--- synapse/storage/databases/main/room.py | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 4e07a3104a4..fcb17d0ec28 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -276,9 +276,9 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: for c in changes ] - # We know the last record will have - # the highest stream ID, so use that one. If there aren't any records, just - # return the `to_id` value because it'll be the furthest stream position possible. + # We know the last record will have the highest stream ID, so use that one. If + # there aren't any records, just return the `to_id` value because it'll be the + # furthest stream position possible. next_batch = changes[-1].stream_id if len(changes) > 0 else to_id return HTTPStatus.OK, {"next_batch": next_batch, "changes": serialized_changes} diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index c89a7700cb8..e9665b89d65 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -264,7 +264,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: ) local_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) if len(local_media_result) > 0: - self._insert_quarantine_change_txn(txn, local_media_result, True) + self._insert_quarantine_changes_txn(txn, local_media_result, True) # We use a >= ? on the media origin to avoid missing records when media IDs # collide between origins (the table's unique constraint is on `(media_origin, media_id)`). @@ -282,7 +282,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: ) remote_media_result = cast(list[tuple[str | None, str]], txn.fetchall()) if len(remote_media_result) > 0: - self._insert_quarantine_change_txn(txn, remote_media_result, True) + self._insert_quarantine_changes_txn(txn, remote_media_result, True) self.db_pool.updates._background_update_progress_txn( txn, @@ -1498,7 +1498,7 @@ def _quarantine_local_media_txn( total_media_quarantined += len(media_ids_affected) if len(media_ids_affected) > 0: # Flag media that was newly (un)quarantined in the changes table. - self._insert_quarantine_change_txn( + self._insert_quarantine_changes_txn( txn, [(None, media_id) for (media_id,) in media_ids_affected], quarantined_by is not None, @@ -1523,7 +1523,7 @@ def _quarantine_local_media_txn( media_ids_affected = txn.fetchall() total_media_quarantined += len(media_ids_affected) if len(media_ids_affected) > 0: - self._insert_quarantine_change_txn( + self._insert_quarantine_changes_txn( txn, [(None, media_id) for (media_id,) in media_ids_affected], quarantined_by is not None, @@ -1567,7 +1567,7 @@ def _quarantine_remote_media_txn( media_ids_affected = cast(list[tuple[str | None, str]], txn.fetchall()) total_media_quarantined += len(media_ids_affected) if len(media_ids_affected) > 0: - self._insert_quarantine_change_txn( + self._insert_quarantine_changes_txn( txn, media_ids_affected, quarantined_by is not None, @@ -1586,7 +1586,7 @@ def _quarantine_remote_media_txn( media_ids_affected = cast(list[tuple[str | None, str]], txn.fetchall()) total_media_quarantined += len(media_ids_affected) if len(media_ids_affected) > 0: - self._insert_quarantine_change_txn( + self._insert_quarantine_changes_txn( txn, media_ids_affected, quarantined_by is not None, From bf589b7ed77c9c5c6ff47b224e194b4b53783aca Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 10:40:38 -0600 Subject: [PATCH 45/68] More code review changes --- synapse/rest/admin/media.py | 7 ----- synapse/storage/databases/main/room.py | 29 ++++++++++++++++--- .../94/03_quarantined_media_tracking.sql | 4 ++- tests/rest/admin/test_media.py | 6 ++-- 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index fcb17d0ec28..389868d25eb 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -251,13 +251,6 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit = 100 # arbitrary; not enough to cause problems (hopefully) to_id = await self.store.get_current_quarantined_media_stream_id() - if to_id < from_id: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a positive integer and behind the current stream position.", - errcode=Codes.INVALID_PARAM, - ) - # We need to wait to ensure that our current worker is actually caught up with # the stream position, otherwise we might not return what we think we're returning. diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index e9665b89d65..94e24eb6256 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -23,6 +23,7 @@ import logging from dataclasses import dataclass from enum import Enum +from http import HTTPStatus from typing import ( TYPE_CHECKING, AbstractSet, @@ -41,7 +42,7 @@ JoinRules, PublicRoomsFilterFields, ) -from synapse.api.errors import StoreError +from synapse.api.errors import StoreError, SynapseError, Codes from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.events import EventBase @@ -201,7 +202,7 @@ def __init__( # table with initial data that callers expect (namely, a list of currently # quarantined media). self.db_pool.updates.register_background_update_handler( - "flag_existing_quarantined_media", self._flag_existing_quarantined_media + _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA, self._flag_existing_quarantined_media ) async def _flag_existing_quarantined_media( @@ -286,7 +287,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: self.db_pool.updates._background_update_progress_txn( txn, - "flag_existing_quarantined_media", + _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA, { "last_local_media_id": local_media_result[-1][1] if len(local_media_result) > 0 @@ -314,7 +315,7 @@ def flag_quarantined(txn: LoggingTransaction) -> int: ) if num_flagged <= 0: # probably never negative, but why trust computers? await self.db_pool.updates._end_background_update( - "flag_existing_quarantined_media" + _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA ) return num_flagged @@ -1377,7 +1378,26 @@ async def get_quarantined_media_changes( Returns: List of `QuarantinedMediaUpdate` update rows in stream ordering (ascending order). + + Raises: + SynapseError: If `from_id` is ahead of `to_id`, or if waiting for `to_id` + took too long. """ + if to_id < from_id: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Query parameter from must be a positive integer and behind the current stream position.", + errcode=Codes.INVALID_PARAM, + ) + + # Wait for the to_id stream position (or time out) + if not await self.wait_for_quarantined_media_stream_id(to_id): + raise SynapseError( + HTTPStatus.INTERNAL_SERVER_ERROR, + "Timed out while waiting for stream position", + errcode=Codes.UNKNOWN, + ) + return await self.db_pool.runInteraction( "get_quarantined_media_changes", self._get_quarantined_media_changes_txn, @@ -2365,6 +2385,7 @@ class _BackgroundUpdates: REPLACE_ROOM_DEPTH_MIN_DEPTH = "replace_room_depth_min_depth" POPULATE_ROOMS_CREATOR_COLUMN = "populate_rooms_creator_column" ADD_ROOM_TYPE_COLUMN = "add_room_type_column" + FLAG_EXISTING_QUARANTINED_MEDIA = "flag_existing_quarantined_media" _REPLACE_ROOM_DEPTH_SQL_COMMANDS = ( diff --git a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql index 5d5fa5f0152..496c132126c 100644 --- a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql +++ b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql @@ -11,7 +11,9 @@ -- See the GNU Affero General Public License for more details: -- . --- Represents a stream of when media is quarantined and unquarantined. +-- Represents a stream of when media is quarantined and unquarantined. Note that it's possible for duplicate transitions +-- to exist in the stream. This typically happens if the background update happens to catch media that was quarantined +-- elsewhere, leading to a `quarantined=true` record being inserted twice for the same origin and media_id. CREATE TABLE quarantined_media_changes ( -- Position in the quarantined media stream stream_id INTEGER NOT NULL PRIMARY KEY, diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index b1713ec56b6..70493f458b8 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -867,7 +867,8 @@ def test_list_quarantined_media(self) -> None: def test_list_quarantined_media_bounds_high(self) -> None: """ - Ensure out of bounds requests with high `from` values are handled gracefully. + Ensure out of bounds requests with high `from` values are met with an appropriate + error. """ # Page that's very much out of range, so should have no results channel = self.make_request( @@ -880,7 +881,8 @@ def test_list_quarantined_media_bounds_high(self) -> None: def test_list_quarantined_media_bounds_low(self) -> None: """ - Ensure out of bounds requests with low `from` values are handled gracefully. + Ensure out of bounds requests with low `from` values are met with an appropriate + error. """ # The same as above, but negative channel = self.make_request( From 61ed17fc0d6feebe8ea7c8d0e264e73f9e57216e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 11:03:53 -0600 Subject: [PATCH 46/68] Add background update test case --- tests/storage/test_room.py | 62 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index f8c5260fa2e..f3a3c275ce0 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -23,6 +23,7 @@ from synapse.api.room_versions import RoomVersions from synapse.server import HomeServer +from synapse.storage.databases.main.room import _BackgroundUpdates, RoomWorkerStore from synapse.types import RoomAlias, RoomID, UserID from synapse.util.clock import Clock @@ -67,3 +68,64 @@ def test_get_room_with_stats_unknown_room(self) -> None: self.assertIsNone( self.get_success(self.store.get_room_with_stats("!uknown:test")) ) + + +class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): + """ + Test the `flag_existing_quarantined_media` background update. + """ + + def test_populates_quarantined_only(self) -> None: + admin_user_tok = self.register_user("admin", "pass", admin=True) + + # Upload two distinct media items so we can quarantine one. If they shared content, + # then the quarantine-by-hash code would hit both. + unaffected_media_id = self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200)[ + "content_uri" + ][6:].split("/")[1] # Cut off 'mxc://' and domain + quarantined_media_id = self.helper.upload_media(b"second content", tok=admin_user_tok, expect_code=200)[ + "content_uri" + ][6:].split("/")[1] # Cut off 'mxc://' and domain + + # Update the quarantined media ID to actually be quarantined manually. We do this + # direct to the database to avoid hitting any code which might flag the media in + # the changes table for us. This simulates having existing media already quarantined. + self.get_success( + self.store.db_pool.simple_update_one( + table="local_media_repository", + keyvalues={"media_id": quarantined_media_id}, + updatevalues={"quarantined_by": "system"}, + desc="local_media_repository.test_populates_quarantined_only", + ) + ) + + # Insert and run the background update + self.get_success( + self.store.db_pool.simple_insert( + table="background_updates", + values={ + "update_name": _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA, + "progress_json": "{}", + }, + ) + ) + self.store.db_pool.updates._all_done = False + self.wait_for_background_updates() + + # Check that the changes table is now populated, and has exactly 1 quarantined + # media object in it (the one we quarantined). + changes: list[tuple[str | None, str, bool]] = self.get_success( + self.store.db_pool.simple_select_list( + "quarantined_media_changes", + None, + retcols=( + "origin", + "media_id", + "quarantined" + ) + ) + ) + self.assertEqual(len(changes), 1) + self.assertEqual(changes[0][0], None) # origin (local media) + self.assertEqual(changes[0][1], quarantined_media_id) # media_id + self.assertEqual(changes[0][2], True) # quarantined flag From 1a5d3b838fbf2877ac3e5dd7c03fbf981e941289 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Mon, 6 Apr 2026 17:05:58 +0000 Subject: [PATCH 47/68] Attempt to fix linting --- synapse/storage/databases/main/room.py | 5 +++-- tests/storage/test_room.py | 20 ++++++++------------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 94e24eb6256..e633a566b49 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -42,7 +42,7 @@ JoinRules, PublicRoomsFilterFields, ) -from synapse.api.errors import StoreError, SynapseError, Codes +from synapse.api.errors import Codes, StoreError, SynapseError from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.events import EventBase @@ -202,7 +202,8 @@ def __init__( # table with initial data that callers expect (namely, a list of currently # quarantined media). self.db_pool.updates.register_background_update_handler( - _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA, self._flag_existing_quarantined_media + _BackgroundUpdates.FLAG_EXISTING_QUARANTINED_MEDIA, + self._flag_existing_quarantined_media, ) async def _flag_existing_quarantined_media( diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index f3a3c275ce0..63f6822da1b 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -23,7 +23,7 @@ from synapse.api.room_versions import RoomVersions from synapse.server import HomeServer -from synapse.storage.databases.main.room import _BackgroundUpdates, RoomWorkerStore +from synapse.storage.databases.main.room import _BackgroundUpdates from synapse.types import RoomAlias, RoomID, UserID from synapse.util.clock import Clock @@ -80,12 +80,12 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. - unaffected_media_id = self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200)[ - "content_uri" - ][6:].split("/")[1] # Cut off 'mxc://' and domain - quarantined_media_id = self.helper.upload_media(b"second content", tok=admin_user_tok, expect_code=200)[ - "content_uri" - ][6:].split("/")[1] # Cut off 'mxc://' and domain + unaffected_media_id = self.helper.upload_media( + b"first content", tok=admin_user_tok, expect_code=200 + )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + quarantined_media_id = self.helper.upload_media( + b"second content", tok=admin_user_tok, expect_code=200 + )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain # Update the quarantined media ID to actually be quarantined manually. We do this # direct to the database to avoid hitting any code which might flag the media in @@ -118,11 +118,7 @@ def test_populates_quarantined_only(self) -> None: self.store.db_pool.simple_select_list( "quarantined_media_changes", None, - retcols=( - "origin", - "media_id", - "quarantined" - ) + retcols=("origin", "media_id", "quarantined"), ) ) self.assertEqual(len(changes), 1) From 0851400a91b4852e331d420e864817ec7eb958e1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 11:09:00 -0600 Subject: [PATCH 48/68] incorporate hidden suggestion --- synapse/storage/databases/main/room.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index e633a566b49..25334e84e1a 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -218,8 +218,8 @@ async def _flag_existing_quarantined_media( some point. Media which isn't quarantined has not changed state (as far as this function can tell). - When media was quarantined is not recorded, so this inserts as if the media was - quarantined at the current time. + We don't know when the media was originally quarantined, so this inserts as if + the media was quarantined now and are processed in an arbitrary order. Further, due to lack of timestamp or history, media which was quarantined then unquarantined will not be picked up by this background task. From 44ddb62c66672b9db028562f3519bf386dcc783e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 11:22:22 -0600 Subject: [PATCH 49/68] define store --- tests/storage/test_room.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 63f6822da1b..ff346620b5f 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -74,6 +74,8 @@ class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): """ Test the `flag_existing_quarantined_media` background update. """ + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main def test_populates_quarantined_only(self) -> None: admin_user_tok = self.register_user("admin", "pass", admin=True) From 3c1f0ea595a6a1e56edd7ce2eeacae1375ff868c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 11:24:11 -0600 Subject: [PATCH 50/68] remove unused var --- tests/storage/test_room.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index ff346620b5f..8a440426378 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -82,7 +82,8 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. - unaffected_media_id = self.helper.upload_media( + # noinspection PyStatementEffect + self.helper.upload_media( b"first content", tok=admin_user_tok, expect_code=200 )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain quarantined_media_id = self.helper.upload_media( From da290fa410799ed6302dd28bb4b6ce1142956953 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Mon, 6 Apr 2026 17:59:25 +0000 Subject: [PATCH 51/68] Attempt to fix linting --- tests/storage/test_room.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 8a440426378..1f13c5131a7 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -74,6 +74,7 @@ class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): """ Test the `flag_existing_quarantined_media` background update. """ + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main @@ -83,9 +84,9 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. # noinspection PyStatementEffect - self.helper.upload_media( - b"first content", tok=admin_user_tok, expect_code=200 - )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200)[ + "content_uri" + ][6:].split("/")[1] # Cut off 'mxc://' and domain quarantined_media_id = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain From 35ac9dce9efcee934005aa659fba4fd8b0b21785 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 12:03:27 -0600 Subject: [PATCH 52/68] bump ci From b4e37558746802605416a787c8cc591d6e55ab00 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 12:24:16 -0600 Subject: [PATCH 53/68] fix --- synapse/rest/admin/media.py | 3 --- synapse/storage/databases/main/room.py | 3 ++- tests/storage/test_room.py | 8 ++++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 389868d25eb..3a7cfae267c 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -251,9 +251,6 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: limit = 100 # arbitrary; not enough to cause problems (hopefully) to_id = await self.store.get_current_quarantined_media_stream_id() - # We need to wait to ensure that our current worker is actually caught up with - # the stream position, otherwise we might not return what we think we're returning. - changes = await self.store.get_quarantined_media_changes( from_id=from_id, to_id=to_id, diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 25334e84e1a..1ee23990588 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1391,7 +1391,8 @@ async def get_quarantined_media_changes( errcode=Codes.INVALID_PARAM, ) - # Wait for the to_id stream position (or time out) + # We need to wait to ensure that our current worker is actually caught up with + # the stream position, otherwise we might not return what we think we're returning. if not await self.wait_for_quarantined_media_stream_id(to_id): raise SynapseError( HTTPStatus.INTERNAL_SERVER_ERROR, diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 1f13c5131a7..557a5164fa6 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -21,7 +21,9 @@ from twisted.internet.testing import MemoryReactor +import synapse.rest.admin from synapse.api.room_versions import RoomVersions +from synapse.rest.client import login, media from synapse.server import HomeServer from synapse.storage.databases.main.room import _BackgroundUpdates from synapse.types import RoomAlias, RoomID, UserID @@ -74,6 +76,12 @@ class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): """ Test the `flag_existing_quarantined_media` background update. """ + servlets = [ + synapse.rest.admin.register_servlets, + synapse.rest.admin.register_servlets_for_media_repo, + login.register_servlets, + media.register_servlets, + ] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main From f4bbc9980fb4211a2784defce0c7298da13f7908 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Mon, 6 Apr 2026 18:29:33 +0000 Subject: [PATCH 54/68] Attempt to fix linting --- tests/storage/test_room.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 557a5164fa6..0127c06a8c0 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -76,6 +76,7 @@ class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): """ Test the `flag_existing_quarantined_media` background update. """ + servlets = [ synapse.rest.admin.register_servlets, synapse.rest.admin.register_servlets_for_media_repo, From af05ff3769890d7fa8df6d6f9a972e9537e0076b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 12:44:34 -0600 Subject: [PATCH 55/68] Different base I guess --- tests/storage/test_room.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 0127c06a8c0..19678c50753 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -21,14 +21,13 @@ from twisted.internet.testing import MemoryReactor -import synapse.rest.admin from synapse.api.room_versions import RoomVersions -from synapse.rest.client import login, media from synapse.server import HomeServer from synapse.storage.databases.main.room import _BackgroundUpdates from synapse.types import RoomAlias, RoomID, UserID from synapse.util.clock import Clock +from tests.rest.admin.test_media import _AdminMediaTests from tests.unittest import HomeserverTestCase @@ -72,18 +71,11 @@ def test_get_room_with_stats_unknown_room(self) -> None: ) -class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(HomeserverTestCase): +class FlagExistingQuarantinedMediaBackgroundUpdatesTestCase(_AdminMediaTests): """ Test the `flag_existing_quarantined_media` background update. """ - servlets = [ - synapse.rest.admin.register_servlets, - synapse.rest.admin.register_servlets_for_media_repo, - login.register_servlets, - media.register_servlets, - ] - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main From 8c348045393966bad0b16959ea813173ac006c39 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Apr 2026 13:00:29 -0600 Subject: [PATCH 56/68] login --- tests/storage/test_room.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 19678c50753..fb6470c95ad 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -80,7 +80,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main def test_populates_quarantined_only(self) -> None: - admin_user_tok = self.register_user("admin", "pass", admin=True) + self.register_user("admin", "pass", admin=True) + admin_user_tok = self.login("admin", "pass") # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. From 78beb967dccb94caccb6650a16f0b7467907b610 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 13:33:17 -0600 Subject: [PATCH 57/68] Add sequence to portdb setup --- synapse/_scripts/synapse_port_db.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index 113c82ca839..0b8a289d92d 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -913,6 +913,10 @@ def alter_table(txn: LoggingTransaction) -> None: await self._setup_autoincrement_sequence( "state_groups_pending_deletion", "sequence_number" ) + await self._setup_sequence( + "quarantined_media_id_seq", + [("quarantined_media_changes", "stream_id")], + ) # Step 3. Get tables. self.progress.set_state("Fetching tables") From 53752b0b26e52820c128ab75d14394d012554d73 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 14:31:48 -0600 Subject: [PATCH 58/68] docs and test changes --- docs/admin_api/media_admin_api.md | 14 ++++++ synapse/storage/databases/main/room.py | 18 ++++---- .../94/03_quarantined_media_tracking.sql | 15 +++++-- tests/rest/admin/test_media.py | 23 +++------- tests/storage/test_room.py | 44 +++++++++++++------ 5 files changed, 71 insertions(+), 43 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index 4d95e8a0eb5..cbf5070e8a3 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -252,6 +252,20 @@ Response: When media is quarantined or unquarantined, a change record is created in the database. This API returns those change records in the order they were created. +**Note**: Some media is quarantined immediately upon upload due to it sharing a +hash with an existing quarantined media item. Some of these automatic quarantine +changes are not returned by this API. Callers are generally expected to be using +the hashes of media returned by this API rather than the specific media IDs which +can still capture media records that may have been missed. However, not including +all records is considered a bug, tracked by issue [#19672](https://github.com/element-hq/synapse/issues/19672). + +**Note**: Servers which had media quarantined before Synapse 1.152.0 may see +duplicate records returned by this API. This is due to how the background update +works to populate the underlying database table. + +**Note**: Due to the above, this API should be considered *best effort* and expected +to have missing or duplicate records. + Each page has a maximum of 100 records. The first page has the oldest records, paginating forwards with each `next_batch` value. diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 1ee23990588..8e8f8f0a169 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -228,7 +228,8 @@ async def _flag_existing_quarantined_media( Args: progress: The progress dictionary from the background update. - batch_size: The number of rows to process in each batch. + batch_size: The number of rows in each of the local_media_repository and + remote_media_cache tables to process in each batch. Returns: The number of rows inserted. @@ -251,6 +252,11 @@ async def _flag_existing_quarantined_media( # # Note: Already-quarantined media is indicated by the `quarantined_by` field being # non-null. We only want quarantined media, per docstring above. + # + # This background update is considered *best effort* for the reasons above. Data + # might be missing or duplicated, and that's just going to have to be okay. This + # is further reinforced by not all changes being captured by the table anyway. + # See https://github.com/element-hq/synapse/issues/19672 for more details. def flag_quarantined(txn: LoggingTransaction) -> int: # It doesn't matter which order we do these in, as long as we do both of them. txn.execute( @@ -1381,15 +1387,11 @@ async def get_quarantined_media_changes( List of `QuarantinedMediaUpdate` update rows in stream ordering (ascending order). Raises: - SynapseError: If `from_id` is ahead of `to_id`, or if waiting for `to_id` - took too long. + SynapseError: If waiting for `to_id` took too long. """ if to_id < from_id: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a positive integer and behind the current stream position.", - errcode=Codes.INVALID_PARAM, - ) + # the to_id is behind the from_id, which means no results + return [] # We need to wait to ensure that our current worker is actually caught up with # the stream position, otherwise we might not return what we think we're returning. diff --git a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql index 496c132126c..0eb74d85d49 100644 --- a/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql +++ b/synapse/storage/schema/main/delta/94/03_quarantined_media_tracking.sql @@ -11,9 +11,18 @@ -- See the GNU Affero General Public License for more details: -- . --- Represents a stream of when media is quarantined and unquarantined. Note that it's possible for duplicate transitions --- to exist in the stream. This typically happens if the background update happens to catch media that was quarantined --- elsewhere, leading to a `quarantined=true` record being inserted twice for the same origin and media_id. +-- Represents a stream of when media is quarantined and unquarantined. Note that it's possible for duplicate rows to +-- exist in this table. When the background update which backfills this table is running, it orders quarantined media +-- by `media_id`. This table is also populated when an admin newly quarantines a piece of media. If the admin happens +-- to newly quarantine media that hasn't yet been processed by the background update, both the admin's API call and the +-- background update will add a row to this table. If the background update has already progressed past the media's ID, +-- then only the admin's API call will add a row to this table. +-- +-- Note also that this table might not be inserted with all possible cases of media being quarantined. For example, if +-- media is quarantined by hash upon upload or URL preview, it might not show up here. See https://github.com/element-hq/synapse/issues/19672 +-- for more details. +-- +-- Overall, this table is very much intended to be *best effort* for media that was quarantined before the table existed. CREATE TABLE quarantined_media_changes ( -- Position in the quarantined media stream stream_id INTEGER NOT NULL PRIMARY KEY, diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 70493f458b8..5a6aefe1e9f 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -867,8 +867,7 @@ def test_list_quarantined_media(self) -> None: def test_list_quarantined_media_bounds_high(self) -> None: """ - Ensure out of bounds requests with high `from` values are met with an appropriate - error. + Ensure out of bounds requests with high `from` values return zero results. """ # Page that's very much out of range, so should have no results channel = self.make_request( @@ -876,22 +875,10 @@ def test_list_quarantined_media_bounds_high(self) -> None: "/_synapse/admin/v1/media/quarantine_changes?from=900000", access_token=self.admin_user_tok, ) - self.assertEqual(400, channel.code, msg=channel.json_body) - self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) - - def test_list_quarantined_media_bounds_low(self) -> None: - """ - Ensure out of bounds requests with low `from` values are met with an appropriate - error. - """ - # The same as above, but negative - channel = self.make_request( - "GET", - "/_synapse/admin/v1/media/quarantine_changes?from=-1", - access_token=self.admin_user_tok, - ) - self.assertEqual(400, channel.code, msg=channel.json_body) - self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(0, len(channel.json_body["changes"])) + # we should be returning a value for `from` which actually makes sense + self.assertEqual(0, channel.json_body["next_batch"]) class QuarantineMediaByIDTestCase(_AdminMediaTests): diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index fb6470c95ad..db12982d601 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -24,6 +24,7 @@ from synapse.api.room_versions import RoomVersions from synapse.server import HomeServer from synapse.storage.databases.main.room import _BackgroundUpdates +from synapse.storage.types import Cursor from synapse.types import RoomAlias, RoomID, UserID from synapse.util.clock import Clock @@ -85,23 +86,38 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. - # noinspection PyStatementEffect - self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200)[ - "content_uri" - ][6:].split("/")[1] # Cut off 'mxc://' and domain - quarantined_media_id = self.helper.upload_media( + _unaffected_media_response = self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200) + quarantined_media_uri = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 - )["content_uri"][6:].split("/")[1] # Cut off 'mxc://' and domain + )["content_uri"] + quarantined_media_origin_and_media_id = quarantined_media_uri[6:] # cut off 'mxc://' + quarantined_media_origin, quarantined_media_id = quarantined_media_origin_and_media_id.split("/") + + # Ideally we'd also upload remote media to ensure that gets picked up, but that's + # a little tricky to set up in the test here. We hope that local and remote media + # are treated similarly during the background update. + + # Quarantine the media like an admin would. Because the quarantine API also inserts + # a record into the database for us, we'll clear out the `quarantined_media_changes` + # table before running the background update. This will simulate already-quarantined + # media being in the database prior to the background update. + channel = self.make_request( + "POST", + "/_synapse/admin/v1/media/quarantine/%s/%s" + % ( + quarantined_media_origin, + quarantined_media_id, + ), + access_token=admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) - # Update the quarantined media ID to actually be quarantined manually. We do this - # direct to the database to avoid hitting any code which might flag the media in - # the changes table for us. This simulates having existing media already quarantined. + # Do that table clear we mentioned above + def _wipe_table(txn: Cursor) -> None: + txn.execute("DELETE FROM quarantined_media_changes") self.get_success( - self.store.db_pool.simple_update_one( - table="local_media_repository", - keyvalues={"media_id": quarantined_media_id}, - updatevalues={"quarantined_by": "system"}, - desc="local_media_repository.test_populates_quarantined_only", + self.store.db_pool.runInteraction( + "test_populates_quarantined_only._wipe_table", _wipe_table ) ) From 0d086a43ac1ed487aec094d54970bfd4925bc2ac Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Wed, 8 Apr 2026 20:35:09 +0000 Subject: [PATCH 59/68] Attempt to fix linting --- tests/storage/test_room.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index db12982d601..b6dc1b94d52 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -86,12 +86,18 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. - _unaffected_media_response = self.helper.upload_media(b"first content", tok=admin_user_tok, expect_code=200) + _unaffected_media_response = self.helper.upload_media( + b"first content", tok=admin_user_tok, expect_code=200 + ) quarantined_media_uri = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 )["content_uri"] - quarantined_media_origin_and_media_id = quarantined_media_uri[6:] # cut off 'mxc://' - quarantined_media_origin, quarantined_media_id = quarantined_media_origin_and_media_id.split("/") + quarantined_media_origin_and_media_id = quarantined_media_uri[ + 6: + ] # cut off 'mxc://' + quarantined_media_origin, quarantined_media_id = ( + quarantined_media_origin_and_media_id.split("/") + ) # Ideally we'd also upload remote media to ensure that gets picked up, but that's # a little tricky to set up in the test here. We hope that local and remote media @@ -115,6 +121,7 @@ def test_populates_quarantined_only(self) -> None: # Do that table clear we mentioned above def _wipe_table(txn: Cursor) -> None: txn.execute("DELETE FROM quarantined_media_changes") + self.get_success( self.store.db_pool.runInteraction( "test_populates_quarantined_only._wipe_table", _wipe_table From 1e94897a489be62d1e9968788ef346a00619fa85 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 14:57:53 -0600 Subject: [PATCH 60/68] E_OFF_BY_1 --- tests/rest/admin/test_media.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 5a6aefe1e9f..400e3422f83 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -878,7 +878,7 @@ def test_list_quarantined_media_bounds_high(self) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(0, len(channel.json_body["changes"])) # we should be returning a value for `from` which actually makes sense - self.assertEqual(0, channel.json_body["next_batch"]) + self.assertEqual(1, channel.json_body["next_batch"]) class QuarantineMediaByIDTestCase(_AdminMediaTests): From 9f03d6052b53e315fc7b65f919fc70303bdad925 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 16:08:54 -0600 Subject: [PATCH 61/68] Fix docs --- docs/admin_api/media_admin_api.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index cbf5070e8a3..c695295d68b 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -261,7 +261,7 @@ all records is considered a bug, tracked by issue [#19672](https://github.com/el **Note**: Servers which had media quarantined before Synapse 1.152.0 may see duplicate records returned by this API. This is due to how the background update -works to populate the underlying database table. +works to populate the underlying database table. **Note**: Due to the above, this API should be considered *best effort* and expected to have missing or duplicate records. @@ -283,7 +283,7 @@ Response: ```json { "next_batch": 4, - "rows": [ + "changes": [ { "origin": "example.org", "media_id": "abcdefg12345...", "quarantined": true }, { "origin": "example.org", "media_id": "abcdefg12345...", "quarantined": false }, { "origin": "another.example.org", "media_id": "abcdefg12345...", "quarantined": true } From 045dfec57937a4f57442cf4f43c92334450f710f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 17:16:44 -0600 Subject: [PATCH 62/68] Apply suggestions from code review Co-authored-by: Eric Eastwood --- docs/admin_api/media_admin_api.md | 18 +++++------------- tests/storage/test_room.py | 2 +- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index c695295d68b..750be85bbe9 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -252,19 +252,11 @@ Response: When media is quarantined or unquarantined, a change record is created in the database. This API returns those change records in the order they were created. -**Note**: Some media is quarantined immediately upon upload due to it sharing a -hash with an existing quarantined media item. Some of these automatic quarantine -changes are not returned by this API. Callers are generally expected to be using -the hashes of media returned by this API rather than the specific media IDs which -can still capture media records that may have been missed. However, not including -all records is considered a bug, tracked by issue [#19672](https://github.com/element-hq/synapse/issues/19672). - -**Note**: Servers which had media quarantined before Synapse 1.152.0 may see -duplicate records returned by this API. This is due to how the background update -works to populate the underlying database table. - -**Note**: Due to the above, this API should be considered *best effort* and expected -to have missing or duplicate records. +**Note**: This API should be considered *best-effort* and expected to have missing or +duplicate records. Currently, this only captures any media explicitly (un)quarantined by +the media quarantine admin API, and the other cases are tracked by +https://github.com/element-hq/synapse/issues/19672. Historical media uploaded before +Synapse 1.152.0 is backfilled in a background update on a best-effort basis. Each page has a maximum of 100 records. The first page has the oldest records, paginating forwards with each `next_batch` value. diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index b6dc1b94d52..2df3a16534f 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -99,7 +99,7 @@ def test_populates_quarantined_only(self) -> None: quarantined_media_origin_and_media_id.split("/") ) - # Ideally we'd also upload remote media to ensure that gets picked up, but that's + # Ideally we'd also upload remote media to ensure that `remote_media_cache` gets picked up, but that's # a little tricky to set up in the test here. We hope that local and remote media # are treated similarly during the background update. From af5dd7abe42f2e1405a42938721f91c0217456d3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 17:34:59 -0600 Subject: [PATCH 63/68] address code review again --- synapse/rest/admin/media.py | 11 ++++++++++- synapse/storage/databases/main/room.py | 22 +++++++++++----------- tests/rest/admin/test_media.py | 18 +++++++++--------- tests/storage/test_room.py | 8 +++----- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 3a7cfae267c..d8104faa0c3 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -249,8 +249,17 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: from_id = parse_integer(request, "from", default=0) limit = 100 # arbitrary; not enough to cause problems (hopefully) - to_id = await self.store.get_current_quarantined_media_stream_id() + # We need to wait to ensure that our current worker is actually caught up with + # the stream position, otherwise we might not return what we think we're returning. + if not await self.store.wait_for_quarantined_media_stream_id(from_id): + raise SynapseError( + HTTPStatus.INTERNAL_SERVER_ERROR, + "Timed out while waiting for stream position", + errcode=Codes.UNKNOWN, + ) + + to_id = await self.store.get_current_quarantined_media_stream_id() changes = await self.store.get_quarantined_media_changes( from_id=from_id, to_id=to_id, diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 8e8f8f0a169..8bef2e5fcaf 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -228,12 +228,15 @@ async def _flag_existing_quarantined_media( Args: progress: The progress dictionary from the background update. - batch_size: The number of rows in each of the local_media_repository and - remote_media_cache tables to process in each batch. + batch_size: The number of rows to process in each batch. Returns: The number of rows inserted. """ + # Note: we actually process 2x the batch_size per batch because we use it twice: + # once for the local_media_repository table, and once for the remote_media_cache + # table. This is fine though - we're still doing the work in batches. + # We track the progress for the local and remote tables separately just in case # there are media ID collisions that would make a mess of `ORDER BY media_id # LIMIT ?`. If there are collisions towards the end of the returned set, the LIMIT @@ -250,6 +253,12 @@ async def _flag_existing_quarantined_media( # over it again in the background update, adding a second row. Duplicate rows are # non-issues for us. # + # Another similar issue is if Synapse is downgraded partway through the background + # update then has more media quarantined. If Synapse is later upgraded again, the + # media that was quarantined while downgraded will only be imported if the media + # IDs are ordered higher than the last processed media ID. Media that has a lower + # ID will be skipped by the background update. + # # Note: Already-quarantined media is indicated by the `quarantined_by` field being # non-null. We only want quarantined media, per docstring above. # @@ -1393,15 +1402,6 @@ async def get_quarantined_media_changes( # the to_id is behind the from_id, which means no results return [] - # We need to wait to ensure that our current worker is actually caught up with - # the stream position, otherwise we might not return what we think we're returning. - if not await self.wait_for_quarantined_media_stream_id(to_id): - raise SynapseError( - HTTPStatus.INTERNAL_SERVER_ERROR, - "Timed out while waiting for stream position", - errcode=Codes.UNKNOWN, - ) - return await self.db_pool.runInteraction( "get_quarantined_media_changes", self._get_quarantined_media_changes_txn, diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 400e3422f83..e7318c057b4 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -805,9 +805,10 @@ def _quarantine_local_media(self, media_id: str, admin_user_tok: str) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) def _local_upload(self, admin_user_tok: str) -> str: - return self.helper.upload_media(SMALL_PNG, tok=admin_user_tok, expect_code=200)[ - "content_uri" - ][6:].split("/")[1] # Cut off 'mxc://' and domain + response = self.helper.upload_media(SMALL_PNG, tok=admin_user_tok, expect_code=200) + origin_and_media_id = response["content_uri"][6:] # Cut off 'mxc://' + _origin, media_id = origin_and_media_id.split("/") + return media_id def test_list_quarantined_media(self) -> None: """ @@ -867,18 +868,17 @@ def test_list_quarantined_media(self) -> None: def test_list_quarantined_media_bounds_high(self) -> None: """ - Ensure out of bounds requests with high `from` values return zero results. + Ensure out of bounds (token stream position greater than our furthest persisted + position) requests with high `from` values are met with an appropriate error. """ - # Page that's very much out of range, so should have no results + # Page that's very much out of range channel = self.make_request( "GET", "/_synapse/admin/v1/media/quarantine_changes?from=900000", access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(0, len(channel.json_body["changes"])) - # we should be returning a value for `from` which actually makes sense - self.assertEqual(1, channel.json_body["next_batch"]) + self.assertEqual(500, channel.code, msg=channel.json_body) + self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) class QuarantineMediaByIDTestCase(_AdminMediaTests): diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 2df3a16534f..e344711058f 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -89,12 +89,10 @@ def test_populates_quarantined_only(self) -> None: _unaffected_media_response = self.helper.upload_media( b"first content", tok=admin_user_tok, expect_code=200 ) - quarantined_media_uri = self.helper.upload_media( + response = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 - )["content_uri"] - quarantined_media_origin_and_media_id = quarantined_media_uri[ - 6: - ] # cut off 'mxc://' + ) + quarantined_media_origin_and_media_id = response["content_uri"][6:] # cut off 'mxc://' quarantined_media_origin, quarantined_media_id = ( quarantined_media_origin_and_media_id.split("/") ) From c78e67fea92cc818db56d55ef8b68f22e0940ce6 Mon Sep 17 00:00:00 2001 From: turt2live <1190097+turt2live@users.noreply.github.com> Date: Wed, 8 Apr 2026 23:39:34 +0000 Subject: [PATCH 64/68] Attempt to fix linting --- synapse/storage/databases/main/room.py | 3 +-- tests/rest/admin/test_media.py | 4 +++- tests/storage/test_room.py | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 8bef2e5fcaf..0a9e69e1bf5 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -23,7 +23,6 @@ import logging from dataclasses import dataclass from enum import Enum -from http import HTTPStatus from typing import ( TYPE_CHECKING, AbstractSet, @@ -42,7 +41,7 @@ JoinRules, PublicRoomsFilterFields, ) -from synapse.api.errors import Codes, StoreError, SynapseError +from synapse.api.errors import StoreError from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.events import EventBase diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index e7318c057b4..895d3500cd8 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -805,7 +805,9 @@ def _quarantine_local_media(self, media_id: str, admin_user_tok: str) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) def _local_upload(self, admin_user_tok: str) -> str: - response = self.helper.upload_media(SMALL_PNG, tok=admin_user_tok, expect_code=200) + response = self.helper.upload_media( + SMALL_PNG, tok=admin_user_tok, expect_code=200 + ) origin_and_media_id = response["content_uri"][6:] # Cut off 'mxc://' _origin, media_id = origin_and_media_id.split("/") return media_id diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index e344711058f..1884ca6bdf4 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -92,7 +92,9 @@ def test_populates_quarantined_only(self) -> None: response = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 ) - quarantined_media_origin_and_media_id = response["content_uri"][6:] # cut off 'mxc://' + quarantined_media_origin_and_media_id = response["content_uri"][ + 6: + ] # cut off 'mxc://' quarantined_media_origin, quarantined_media_id = ( quarantined_media_origin_and_media_id.split("/") ) From 5269e19538fe6e70c08e078c439d5606f94fbe1b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Apr 2026 17:39:45 -0600 Subject: [PATCH 65/68] bump ci From 62c533a91989753432a6f8b28f0a46dae29f6b6e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Apr 2026 08:46:03 -0600 Subject: [PATCH 66/68] Update synapse/rest/admin/media.py Co-authored-by: Eric Eastwood --- synapse/rest/admin/media.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index d8104faa0c3..5f391e69a5a 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -255,7 +255,10 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: if not await self.store.wait_for_quarantined_media_stream_id(from_id): raise SynapseError( HTTPStatus.INTERNAL_SERVER_ERROR, - "Timed out while waiting for stream position", + "Timed out while waiting for the worker serving this request to catch up to the given " + "`from` stream position. Assuming this is a valid `from` token, this indicates an issue " + "with Synapse or the worker deployment lagging behind the replication stream. Please try " + "the request again later.", errcode=Codes.UNKNOWN, ) From 083c00fdb99ad4175963e4d2dcd6cbc5f8ceac81 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Apr 2026 08:48:07 -0600 Subject: [PATCH 67/68] Update tests/storage/test_room.py Co-authored-by: Eric Eastwood --- tests/storage/test_room.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/storage/test_room.py b/tests/storage/test_room.py index 1884ca6bdf4..b4fb5d56288 100644 --- a/tests/storage/test_room.py +++ b/tests/storage/test_room.py @@ -86,13 +86,15 @@ def test_populates_quarantined_only(self) -> None: # Upload two distinct media items so we can quarantine one. If they shared content, # then the quarantine-by-hash code would hit both. - _unaffected_media_response = self.helper.upload_media( + _unaffected_media_upload_response = self.helper.upload_media( b"first content", tok=admin_user_tok, expect_code=200 ) - response = self.helper.upload_media( + # Upload the media we're going to quarantine + media_upload_response = self.helper.upload_media( b"second content", tok=admin_user_tok, expect_code=200 ) - quarantined_media_origin_and_media_id = response["content_uri"][ + # Extract media ID from the response + quarantined_media_origin_and_media_id = media_upload_response["content_uri"][ 6: ] # cut off 'mxc://' quarantined_media_origin, quarantined_media_id = ( From 51f9f0e2f0123e9999591f8fd1efd6d75c1968f4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Apr 2026 09:50:01 -0600 Subject: [PATCH 68/68] Give more friendly feedback to callers who try to get future data --- synapse/rest/admin/media.py | 10 +++++++++- tests/rest/admin/test_media.py | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 5f391e69a5a..5539b01af01 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -249,6 +249,15 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: from_id = parse_integer(request, "from", default=0) limit = 100 # arbitrary; not enough to cause problems (hopefully) + to_id = await self.store.get_current_quarantined_media_stream_id() + + if to_id < from_id: + # The caller is trying to get future data, which isn't possible. + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "The `from` position is ahead of the currently persisted position.", + errcode=Codes.INVALID_PARAM, + ) # We need to wait to ensure that our current worker is actually caught up with # the stream position, otherwise we might not return what we think we're returning. @@ -262,7 +271,6 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]: errcode=Codes.UNKNOWN, ) - to_id = await self.store.get_current_quarantined_media_stream_id() changes = await self.store.get_quarantined_media_changes( from_id=from_id, to_id=to_id, diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 895d3500cd8..6469d305b24 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -879,8 +879,8 @@ def test_list_quarantined_media_bounds_high(self) -> None: "/_synapse/admin/v1/media/quarantine_changes?from=900000", access_token=self.admin_user_tok, ) - self.assertEqual(500, channel.code, msg=channel.json_body) - self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) class QuarantineMediaByIDTestCase(_AdminMediaTests):