From b6ab33f28b34bd3c0f09707841cde822408a912a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 1 Jul 2026 10:15:34 +0000 Subject: [PATCH 1/6] Fix flag_existing_quarantined_media skipping some quarantined remote media The `flag_existing_quarantined_media` background update paged through `remote_media_cache` with `media_origin >= ? AND media_id > ?`, which ANDs the two columns independently. Once an origin was fully processed, rows in a later origin whose `media_id` was <= the last processed `media_id` would be silently skipped and never flagged. Use a proper row-value tuple comparison `(media_origin, media_id) > (?, ?)` (matching the table's unique constraint/index), as done elsewhere in the codebase. Co-Authored-By: Claude Opus 4.8 (1M context) --- synapse/storage/databases/main/room.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 95aa2cb7dcf..02655c75bf8 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -282,15 +282,17 @@ def flag_quarantined(txn: LoggingTransaction) -> int: if len(local_media_result) > 0: 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)`). - # Filtering by `(media_origin, media_id)` also makes sure we're using an index. + # We page through `remote_media_cache` with a tuple comparison on + # `(media_origin, media_id)` (matching the table's unique constraint and + # index). Comparing the columns independently (e.g. + # `media_origin >= ? AND media_id > ?`) would incorrectly skip rows in a + # newly-reached origin whose media_id is <= the last processed media_id. txn.execute( """ SELECT media_origin, media_id FROM remote_media_cache WHERE quarantined_by IS NOT NULL - AND media_origin >= ? AND media_id > ? + AND (media_origin, media_id) > (?, ?) ORDER BY media_origin, media_id LIMIT ? """, From 16bce21166b76da7320bc684b6946b2bfbe6ef7c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 1 Jul 2026 10:16:20 +0000 Subject: [PATCH 2/6] Stop re-running exhausted queries in flag_existing_quarantined_media `flag_quarantined` ran both the local and remote queries on every iteration. Once one table was exhausted but the other still had rows to process, the background update kept returning a positive count, so the finished table's (now empty) query needlessly re-ran on every subsequent iteration until the whole update completed. Track per-table completion (`local_done`/`remote_done`) in the background update progress and skip a table's query once it has returned an empty batch. Co-Authored-By: Claude Opus 4.8 (1M context) --- synapse/storage/databases/main/room.py | 103 +++++++++++++++---------- 1 file changed, 63 insertions(+), 40 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 02655c75bf8..fee89ac7a82 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -245,6 +245,12 @@ async def _flag_existing_quarantined_media( last_remote_media_id = progress.get("last_remote_media_id", "") last_remote_origin = progress.get("last_remote_origin", "") + # Once a table has been fully processed we record it in the progress so that + # we stop re-running its (now empty) query on every subsequent iteration while + # the other table is still being worked through. + local_done = progress.get("local_done", False) + remote_done = progress.get("remote_done", False) + # 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 @@ -266,56 +272,73 @@ async def _flag_existing_quarantined_media( # 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( - """ - 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_changes_txn(txn, local_media_result, True) + local_media_result: list[tuple[str | None, str]] = [] + remote_media_result: list[tuple[str | None, str]] = [] + + # It doesn't matter which order we do these in, as long as we do both of + # them. We skip a table once it's been fully processed so we don't keep + # running an empty query for it every iteration until the other finishes. + if not local_done: + 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_changes_txn(txn, local_media_result, True) # We page through `remote_media_cache` with a tuple comparison on # `(media_origin, media_id)` (matching the table's unique constraint and # index). Comparing the columns independently (e.g. # `media_origin >= ? AND media_id > ?`) would incorrectly skip rows in a # newly-reached origin whose media_id is <= the last processed media_id. - txn.execute( - """ - SELECT media_origin, media_id - FROM remote_media_cache - WHERE quarantined_by IS NOT NULL - AND (media_origin, media_id) > (?, ?) - ORDER BY media_origin, media_id - LIMIT ? - """, - (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: - self._insert_quarantine_changes_txn(txn, remote_media_result, True) + if not remote_done: + txn.execute( + """ + SELECT media_origin, media_id + FROM remote_media_cache + WHERE quarantined_by IS NOT NULL + AND (media_origin, media_id) > (?, ?) + ORDER BY media_origin, media_id + LIMIT ? + """, + (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: + self._insert_quarantine_changes_txn(txn, remote_media_result, True) + + # Carry the previous progress forward, then for each table advance its + # cursor to the last row we fetched, or mark it done if its query (which + # only runs while it isn't already done) came back empty. + new_progress = { + "last_local_media_id": last_local_media_id, + "last_remote_media_id": last_remote_media_id, + "last_remote_origin": last_remote_origin, + "local_done": local_done, + "remote_done": remote_done, + } + if local_media_result: + new_progress["last_local_media_id"] = local_media_result[-1][1] + else: + new_progress["local_done"] = True + if remote_media_result: + new_progress["last_remote_origin"] = remote_media_result[-1][0] + new_progress["last_remote_media_id"] = remote_media_result[-1][1] + else: + new_progress["remote_done"] = True self.db_pool.updates._background_update_progress_txn( txn, _BackgroundUpdates.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, - }, + new_progress, ) return len(local_media_result) + len(remote_media_result) From defef8d2ff91369ecc4a1ce81690142d1e3ee589 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 1 Jul 2026 10:42:00 +0000 Subject: [PATCH 3/6] Re-run flag_existing_quarantined_media after fixing it The `flag_existing_quarantined_media` background update shipped (in 94/03) with a broken remote media query that silently skipped some already-quarantined remote media, and servers may have already run it to completion. Now that the query is fixed, add a schema delta that deletes any existing background update row and re-inserts it so the update runs again from scratch and flags the media missed on the first run. Co-Authored-By: Claude Opus 4.8 (1M context) --- ..._rerun_flag_existing_quarantined_media.sql | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 synapse/storage/schema/main/delta/94/05_rerun_flag_existing_quarantined_media.sql diff --git a/synapse/storage/schema/main/delta/94/05_rerun_flag_existing_quarantined_media.sql b/synapse/storage/schema/main/delta/94/05_rerun_flag_existing_quarantined_media.sql new file mode 100644 index 00000000000..6881b9419cc --- /dev/null +++ b/synapse/storage/schema/main/delta/94/05_rerun_flag_existing_quarantined_media.sql @@ -0,0 +1,25 @@ +-- +-- 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: +-- . + +-- The `flag_existing_quarantined_media` background update (added in 94/03) originally +-- shipped with a broken remote media query that skipped some already-quarantined remote +-- media. Now that it's fixed, re-run the update from scratch so any media missed on the +-- first run gets flagged. +-- +-- We delete any existing row first: on servers where the update already completed the row +-- was removed, and on servers where it's still pending/mid-run this clears the stale +-- progress so the re-insert below starts cleanly (and avoids a primary key collision). +DELETE FROM background_updates WHERE update_name = 'flag_existing_quarantined_media'; + +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (9405, 'flag_existing_quarantined_media', '{}'); From 5ed45d67f14e55fd06e2186762779f44b528e453 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 1 Jul 2026 10:50:45 +0000 Subject: [PATCH 4/6] Add changelog Add the changelog entry for PR #19901, noting the bug was introduced in v1.152.0. Co-Authored-By: Claude Opus 4.8 (1M context) --- changelog.d/19901.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19901.bugfix diff --git a/changelog.d/19901.bugfix b/changelog.d/19901.bugfix new file mode 100644 index 00000000000..e8d9fd1bfe0 --- /dev/null +++ b/changelog.d/19901.bugfix @@ -0,0 +1 @@ +Fix the `flag_existing_quarantined_media` background update skipping some quarantined remote media and re-running exhausted queries unnecessarily. Introduced in v1.152.0. From 1c9879b66dac1307e10984b15ff5c3706173f501 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 2 Jul 2026 09:54:46 +0100 Subject: [PATCH 5/6] Update changelog.d/19901.bugfix Co-authored-by: Eric Eastwood --- changelog.d/19901.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/19901.bugfix b/changelog.d/19901.bugfix index e8d9fd1bfe0..e31c665c183 100644 --- a/changelog.d/19901.bugfix +++ b/changelog.d/19901.bugfix @@ -1 +1 @@ -Fix the `flag_existing_quarantined_media` background update skipping some quarantined remote media and re-running exhausted queries unnecessarily. Introduced in v1.152.0. +Fix the `flag_existing_quarantined_media` background update skipping some quarantined remote media. Introduced in v1.152.0. From 99b235f8fc11b3be3fb5374c73bfc998a6f3c03c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 2 Jul 2026 09:57:30 +0100 Subject: [PATCH 6/6] Fixup comment --- synapse/storage/databases/main/room.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index fee89ac7a82..768bf6e94f9 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -295,10 +295,12 @@ def flag_quarantined(txn: LoggingTransaction) -> int: self._insert_quarantine_changes_txn(txn, local_media_result, True) # We page through `remote_media_cache` with a tuple comparison on - # `(media_origin, media_id)` (matching the table's unique constraint and - # index). Comparing the columns independently (e.g. - # `media_origin >= ? AND media_id > ?`) would incorrectly skip rows in a - # newly-reached origin whose media_id is <= the last processed media_id. + # `(media_origin, media_id)`. This matches a unique index, and so + # will a) page through all rows, and b) will be fast. + # + # Comparing the columns independently (e.g. `media_origin >= ? AND + # media_id > ?`) would incorrectly skip rows in a newly-reached + # origin whose media_id is <= the last processed media_id. if not remote_done: txn.execute( """