Skip to content

Fix two bugs in the flag_existing_quarantined_media background update #19901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikjohnston merged 6 commits into from
Jul 2, 2026
Merged

Fix two bugs in the flag_existing_quarantined_media background update #19901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6ab33f
Fix flag_existing_quarantined_media skipping some quarantined remote …
erikjohnston Jul 1, 2026
16bce21
Stop re-running exhausted queries in flag_existing_quarantined_media
erikjohnston Jul 1, 2026
defef8d
Re-run flag_existing_quarantined_media after fixing it
erikjohnston Jul 1, 2026
5ed45d6
Add changelog
erikjohnston Jul 1, 2026
1c9879b
Update changelog.d/19901.bugfix
erikjohnston Jul 2, 2026
99b235f
Fixup comment
erikjohnston Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19901.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix the `flag_existing_quarantined_media` background update skipping some quarantined remote media. Introduced in v1.152.0.
115 changes: 71 additions & 44 deletions synapse/storage/databases/main/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -266,54 +272,75 @@ 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)

# 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.
Comment thread
MadLittleMods marked this conversation as resolved.
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 ?
""",
(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)
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)`. 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(
"""
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)
Expand Down
25 changes: 25 additions & 0 deletions synapse/storage/schema/main/delta/94/05_rerun_flag_existing_quarantined_media.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:
-- <https://www.gnu.org/licenses/agpl-3.0.html>.

-- 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';

@turt2live turt2live Jul 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a huge shame to lose so many months of progress, especially when we're still running the background update on matrix.org. Can we update the job to run after the existing one or otherwise de-duplicate data going into the table?

@MadLittleMods MadLittleMods Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is at odds with what you wrote above. Why do you care?

fwiw, we spotted the remote media bug after the merge but didn't consider it important enough to fix.

The initial import is fully intended to be best effort, not complete.

@erikjohnston erikjohnston Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On matrix.org we can manually set to "last_local_media_id":"xJFXwTD" and then it will start from where it left of for local media.

This is at odds with what you wrote above. Why do you care?

I think we care about local media, but not about remote media, maybe?

@turt2live turt2live Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matrix.org's scale is very different than other servers. We do indeed care about local media a lot more than remote media on matrix.org


INSERT INTO background_updates (ordering, update_name, progress_json) VALUES
(9405, 'flag_existing_quarantined_media', '{}');
Loading