Add an API to list changes to quarantine state of media by turt2live · Pull Request #19558 · element-hq/synapse

turt2live · 2026-03-14T00:24:00Z

Fixes #19352

(See issue for history of this feature and previous PRs)

First, a naive implementation of the endpoint was introduced, but it quickly ran into performance issues on query and long startup times, leading to its removal. It also didn't actually work, and would fail to expose media when it was "unquarantined", so a partial fix was attempted, where the suggested direction is to use a stream instead of a timestamp column.

This PR re-introduces the API building on the previous feedback:

Adds a stream which tracks when media becomes (un)quarantined.
Runs a background update to capture already-quarantined media.
Adds a new admin API to return rows from the stream table.

We track both quarantine and unquarantine actions in the stream to allow downstream consumers to process the records appropriately. Namely, to allow our Synapse exchange in HMA to remove hashes for unquarantined media (use case further explained in the issue).

Note: This knowingly does not capture all cases of media being quarantined. Other call sites are lower priority for T&S, and can be addressed in a future PR. ~~An issue will be created after this PR is merged to track those sites.~~ #19672

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

Sticky events was used as reference for creating this.

Just something I noticed while working on #19558 We start the function by setting `total_media_quarantined` to zero, then we do work on the `media_ids`, add the number affected, zero it out (**bug**), do work on `hashes`, add the number of affected rows, then return `total_media_quarantined`. ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

MadLittleMods · 2026-04-09T00:10:44Z

+                HTTPStatus.INTERNAL_SERVER_ERROR,
+                "Timed out while waiting for stream position",
+                errcode=Codes.UNKNOWN,
+            )


Ideally, we'd also validate from_id before waiting and throw M_INVALID_PARAM. This way people see a more sane error instead of 500 {'errcode': 'M_UNKNOWN', 'error': 'Internal server error'} from the assertion in wait_for_quarantined_media_stream_id(...).

This kind of thing is mentioned in #19644 "tokens should be validated before it reaches this point."

There isn't a helper for this so it would be a similar if from_id < max_persisted_position: error check.

Since this is an admin endpoint we could forgo this but it would be nice to have a good example in the codebase.

Should be covered in 51f9f0e - let me know if changes are needed

(will follow up in a subsequent PR if required)

The if to_id < from_id: error check that was added in 51f9f0e is a bit flawed since to_id is the current token of the current worker (which could be behind the other workers). (and it's okay for to_id == from_id as that would mean no activity has happened)

I think we want to compare to get_max_allocated_token() which actually looks at the database source of truth across all of the workers.

We'd then need to wait for that token too, I believe?

I guess we don't - if we're caught up on from then we might return fewer results, but that's fine.

Co-authored-by: Eric Eastwood <erice@element.io>

@turt2live

…portdb` (#19675) Part of #19671 Spawning from [discussion in `#synapse-dev:matrix.org`](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$Z3nqbH0Qy21FWC3qJOim6LSRCRpJ3pxV5DLXm98IA6I?via=element.io&via=matrix.org&via=beeper.com) with roots in #19558 (comment). As trialed/discovered by @turt2live alongside @reivilibre and @clokep ❤️ ### Why is this necessary? If you forget to add `_setup_sequence(...)`, you can run into the following error if there is 1 row in SQLite and then you use the `portdb` script to try to migrate to Postgres (as [explained](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$mHU6dcTNL7NMfKBCJUekCh7vDj1lr1GDjriZQl7oeeU?via=element.io&via=matrix.org&via=beeper.com) by @reivilibre) ``` Postgres sequence 'quarantined_media_id_seq' is inconsistent with associated stream position of 'quarantined_media' in the 'stream_positions' table. ```

turt2live · 2026-04-09T17:39:04Z

Thanks for working with me on this :)

MadLittleMods · 2026-04-09T19:41:53Z

Thanks for pushing through all of the hard stuff @turt2live 🦏

…#19677) Following up on #19558 (comment) Changelog for this PR is intended to overlap with the above PR. `get_current_quarantined_media_stream_id` wasn't being used anywhere else, so we can replace it like we do in this PR. ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Eric Eastwood <erice@element.io> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

Fixes #19692 Introduced by #19558 --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

…th unbounded token (#19644) Spawning from trying to find the proper way to wait for a token, see #19558 (comment) - Update `wait_for_stream_token(...)` patterns so validation/sanitization is handled upstream in usage. - Fix sync waiting for bounded token but using unbounded token to fetch data. Noticed while working on adding the new method. Part of #19647

…it_for_stream_token(...)` (#19764) In order to be able to use `wait_for_stream_token(...)`, we have to add the `quarantined_media` stream to the `StreamToken`. Even though we don't care about `/sync`'ing `quarantined_media`, this aligns with the future where all endpoints should probably use `StreamToken`, see #19647 Follow-up to #19558 and #19644

Just something I noticed while working on element-hq#19558 We start the function by setting `total_media_quarantined` to zero, then we do work on the `media_ids`, add the number affected, zero it out (**bug**), do work on `hashes`, add the number of affected rows, then return `total_media_quarantined`. ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

@turt2live

…portdb` (element-hq#19675) Part of element-hq#19671 Spawning from [discussion in `#synapse-dev:matrix.org`](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$Z3nqbH0Qy21FWC3qJOim6LSRCRpJ3pxV5DLXm98IA6I?via=element.io&via=matrix.org&via=beeper.com) with roots in element-hq#19558 (comment). As trialed/discovered by @turt2live alongside @reivilibre and @clokep ❤️ ### Why is this necessary? If you forget to add `_setup_sequence(...)`, you can run into the following error if there is 1 row in SQLite and then you use the `portdb` script to try to migrate to Postgres (as [explained](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$mHU6dcTNL7NMfKBCJUekCh7vDj1lr1GDjriZQl7oeeU?via=element.io&via=matrix.org&via=beeper.com) by @reivilibre) ``` Postgres sequence 'quarantined_media_id_seq' is inconsistent with associated stream position of 'quarantined_media' in the 'stream_positions' table. ```

…9558) Fixes element-hq#19352 (See issue for history of this feature and previous PRs) > First, a [naive implementation](element-hq#19268) of the endpoint was introduced, but it quickly ran into [performance issues on query](element-hq#19312) and [long startup times](element-hq#19349), leading to its [removal](element-hq#19351). It also didn't actually work, and would fail to expose media when it was "unquarantined", so a [partial fix](element-hq#19308) was attempted, where the suggested direction is to use a [stream](https://element-hq.github.io/synapse/latest/development/synapse_architecture/streams.html#cheatsheet-for-creating-a-new-stream) instead of a timestamp column. This PR re-introduces the API building on the previous feedback: * Adds a stream which tracks when media becomes (un)quarantined. * Runs a background update to capture already-quarantined media. * Adds a new admin API to return rows from the stream table. We track both quarantine and unquarantine actions in the stream to allow downstream consumers to process the records appropriately. Namely, to allow our Synapse exchange in HMA to remove hashes for unquarantined media (use case further explained in the [issue](element-hq#19352)). **Note**: This knowingly does not capture all cases of media being quarantined. Other call sites are lower priority for T&S, and can be addressed in a future PR. ~~An issue will be created after this PR is merged to track those sites.~~ element-hq#19672 ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: turt2live <1190097+turt2live@users.noreply.github.com> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com> Co-authored-by: Eric Eastwood <erice@element.io>

…element-hq#19677) Following up on element-hq#19558 (comment) Changelog for this PR is intended to overlap with the above PR. `get_current_quarantined_media_stream_id` wasn't being used anywhere else, so we can replace it like we do in this PR. ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Eric Eastwood <erice@element.io> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

…q#19694) Fixes element-hq#19692 Introduced by element-hq#19558 --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

…th unbounded token (element-hq#19644) Spawning from trying to find the proper way to wait for a token, see element-hq#19558 (comment) - Update `wait_for_stream_token(...)` patterns so validation/sanitization is handled upstream in usage. - Fix sync waiting for bounded token but using unbounded token to fetch data. Noticed while working on adding the new method. Part of element-hq#19647

…it_for_stream_token(...)` (element-hq#19764) In order to be able to use `wait_for_stream_token(...)`, we have to add the `quarantined_media` stream to the `StreamToken`. Even though we don't care about `/sync`'ing `quarantined_media`, this aligns with the future where all endpoints should probably use `StreamToken`, see element-hq#19647 Follow-up to element-hq#19558 and element-hq#19644

…te (#19901) The `flag_existing_quarantined_media` background update (added in #19558, shipped in v1.152.0) back-populates the `quarantined_media_changes` table with media that was already quarantined. It has two bugs. ### 1. Some quarantined remote media is silently skipped The remote-media query paged through `remote_media_cache` with: ```sql WHERE quarantined_by IS NOT NULL AND media_origin >= ? AND media_id > ? ``` This ANDs the two key columns independently rather than comparing them as a tuple. Once an origin has been fully processed (e.g. `media_id` reaches `zzz` for origin `a.example`), rows in a *later* origin whose `media_id` is `<=` the last processed `media_id` (e.g. `b.example` / `aaa`) fail the `media_id > ?` test and are never flagged. Fixed by using a proper row-value tuple comparison: ```sql WHERE quarantined_by IS NOT NULL AND (media_origin, media_id) > (?, ?) ``` Both the minimum supported SQLite (3.37.2) and PostgreSQL support row-value comparisons. ### 2. Exhausted queries keep re-running every iteration `flag_quarantined` ran *both* the local and remote queries on every iteration. When one table was exhausted but the other still had rows, the 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. This could add significant time to the transaction, as since the rows were deleted it could scan a significant portion of the table each time. Fixed by tracking per-table completion (`local_done` / `remote_done`) in the background-update progress and skipping a table's query once it has returned an empty batch. The progress dict was also restructured into an incremental build for readability.

turt2live mentioned this pull request Mar 14, 2026

Fix zeroing out remote quarantined media count #19559

Merged

3 tasks

turt2live added 2 commits March 16, 2026 13:23

Create a stream to track quarantine state changes

9a1c92f

Sticky events was used as reference for creating this.

Insert into quarantined media stream upon changes

c3a0e4a

turt2live force-pushed the travis/list-quarantined-media-mk2 branch from e76dc83 to 6328e78 Compare March 16, 2026 19:27

turt2live added 3 commits March 16, 2026 13:31

Add admin API to access stream data

7386f04

Add background update to insert existing rows

0f782ce

changelog

f1a35fa

turt2live force-pushed the travis/list-quarantined-media-mk2 branch from 258768e to f1a35fa Compare March 16, 2026 19:32

Merge branch 'develop' into travis/list-quarantined-media-mk2

780f13a

turt2live marked this pull request as ready for review March 16, 2026 19:34

turt2live requested a review from a team as a code owner March 16, 2026 19:34

MadLittleMods added A-Admin-API A-Media-Repository labels Mar 16, 2026

turt2live and others added 3 commits March 16, 2026 14:00

fix out of bounds on max

a78c03c

Attempt to fix linting

7963593

bump for ci

d77a76d

turt2live mentioned this pull request Mar 16, 2026

Update Synapse quarantine fetching to use new API matrix-org/hma-matrix#10

Merged

Merge branch 'develop' into travis/list-quarantined-media-mk2

2ef406e

MadLittleMods added the A-Abuse Reports, media quarantine, policy servers, etc label Mar 23, 2026

MadLittleMods reviewed Mar 23, 2026

View reviewed changes

turt2live and others added 9 commits March 30, 2026 10:50

Merge branch 'develop' into travis/list-quarantined-media-mk2

86a6a15

Merge branch 'develop' into travis/list-quarantined-media-mk2

62261e0

Add comments

d743711

Use current token for stream, requiring writer configuration

81ce22c

Add extra safety

282e670

Split and expand tests

616e9c8

Attempt to fix linting

914e252

bump ci

090e220

Move schema deltas

cb71d46

bump ci

5269e19

MadLittleMods approved these changes Apr 9, 2026

View reviewed changes

turt2live and others added 2 commits April 9, 2026 08:46

Update synapse/rest/admin/media.py

62c533a

Co-authored-by: Eric Eastwood <erice@element.io>

Update tests/storage/test_room.py

083c00f

Co-authored-by: Eric Eastwood <erice@element.io>

Give more friendly feedback to callers who try to get future data

51f9f0e

turt2live merged commit fe74265 into develop Apr 9, 2026
48 checks passed

turt2live deleted the travis/list-quarantined-media-mk2 branch April 9, 2026 17:41

turt2live mentioned this pull request Apr 9, 2026

Switch list quarantined media API to use max to_id instead of current #19677

Merged

3 tasks

MadLittleMods referenced this pull request Apr 10, 2026

Add sticky event storage tests for soft-failed and spammy events

e225d6d

MadLittleMods mentioned this pull request Apr 10, 2026

Introduce spam_checker_spammy internal event metadata. #19453

Merged

reivilibre mentioned this pull request Apr 14, 2026

Config changes might be required in v1.152.0rc1 #19692

Closed

turt2live mentioned this pull request Apr 14, 2026

Add upgrade notes for new quarantined_media_changes stream #19694

Merged

3 tasks

reivilibre pushed a commit that referenced this pull request Apr 16, 2026

Add upgrade notes for new quarantined_media_changes stream (#19694)

e1b5631

Fixes #19692 Introduced by #19558 --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

MadLittleMods mentioned this pull request May 7, 2026

Replace wait_for_quarantined_media_stream_id(...) with standard wait_for_stream_token(...) #19764

Merged

3 tasks

MadLittleMods mentioned this pull request May 19, 2026

Add Admin API endpoints to list, fetch and delete room reports #19648

Open

erikjohnston mentioned this pull request Jul 1, 2026

Fix two bugs in the flag_existing_quarantined_media background update #19901

Merged

Uh oh!

Conversation

turt2live commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turt2live Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

turt2live Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turt2live Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

turt2live Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

turt2live Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

turt2live commented Apr 9, 2026

Uh oh!

Uh oh!

MadLittleMods commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

turt2live commented Mar 14, 2026 •

edited

Loading

MadLittleMods Apr 9, 2026 •

edited

Loading

MadLittleMods Apr 9, 2026 •

edited

Loading