Skip to content

Commit 62a4bc4

Browse files
erikjohnstonclaude
andauthored
Fix quarantined_media stream not being replicated (#20085)
The registration of `QuarantinedMediaStream` in `ReplicationCommandHandler._streams_to_replicate` was missed when the stream was added, so an instance configured as the quarantined_media_changes stream writer never sent RDATA/POSITION for it unless it was the main process. Also add the stream to the `instance_map` config validation. Stream was introduced in #19558 Fixes #20080 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 81a42c4 commit 62a4bc4

7 files changed

Lines changed: 112 additions & 2 deletions

File tree

changelog.d/20085.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix the `quarantined_media` replication stream never being sent when the configured `quarantined_media_changes` stream writer is a worker. Introduced in v1.152.0.

docker/configure_workers_and_start.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,7 @@ def add_worker_roles_to_shared_config(
517517
"typing",
518518
"push_rules",
519519
"thread_subscriptions",
520+
"quarantined_media_changes",
520521
}
521522

522523
# Worker-type specific sharding config. Now a single worker can fulfill multiple

docs/development/synapse_architecture/streams.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ necessary registration and event handling.
164164
- will need an [ID generator](https://github.com/element-hq/synapse/blob/4367fb2d078c52959aeca0fe6874539c53e8360d/synapse/storage/databases/main/thread_subscriptions.py#L75)
165165
- may need [writer configuration](https://github.com/element-hq/synapse/blob/4367fb2d078c52959aeca0fe6874539c53e8360d/synapse/config/workers.py#L177), if there isn't already an obvious source of configuration for which workers should be designated as writers to your new stream.
166166
- if adding new writer configuration, add Docker-worker configuration, which lets us configure the writer worker in Complement tests: [[1]](https://github.com/element-hq/synapse/blob/4367fb2d078c52959aeca0fe6874539c53e8360d/docker/configure_workers_and_start.py#L331), [[2]](https://github.com/element-hq/synapse/blob/4367fb2d078c52959aeca0fe6874539c53e8360d/docker/configure_workers_and_start.py#L440)
167+
- Ensure that it's been correctly added to `synapse/replication/tcp/handler.py` and it's `streams_to_replicate` attribute to ensure that changes are actually replicated.
167168
- most of the time, you will likely introduce a new datastore class for the concept represented by the new stream, unless there is already an obvious datastore that covers it.
168169
- consider whether it may make sense to introduce a handler
169170

docs/workers.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -572,10 +572,15 @@ configured as stream writer for the `device_lists` stream:
572572
##### The `quarantined_media_changes` stream
573573

574574
The `quarantined_media_changes` stream supports multiple writers. The following endpoints
575-
can be handled by any worker, but should be routed directly to one of the workers
576-
configured as stream writer for the `quarantined_media_changes` stream:
575+
must be routed directly to one of the workers configured as stream writer for the
576+
`quarantined_media_changes` stream (which must also be able to run the media
577+
repository, as these endpoints are only registered on media-capable workers):
577578

578579
^/_synapse/admin/v1/quarantine_media/.*$
580+
^/_synapse/admin/v1/room/.*/media/quarantine$
581+
^/_synapse/admin/v1/user/.*/media/quarantine$
582+
^/_synapse/admin/v1/media/quarantine/.*$
583+
^/_synapse/admin/v1/media/unquarantine/.*$
579584

580585
#### Restrict outbound federation traffic to a specific set of workers
581586

synapse/config/workers.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,7 @@ def read_config(
375375
"push_rules",
376376
"device_lists",
377377
"thread_subscriptions",
378+
"quarantined_media_changes",
378379
):
379380
instances = _instance_to_list_converter(getattr(self.writers, stream))
380381
for instance in instances:
@@ -430,6 +431,11 @@ def read_config(
430431
"Must specify at least one instance to handle `thread_subscriptions` messages."
431432
)
432433

434+
if len(self.writers.quarantined_media_changes) == 0:
435+
raise ConfigError(
436+
"Must specify at least one instance to handle `quarantined_media_changes` messages."
437+
)
438+
433439
self.events_shard_config = RoutableShardedWorkerHandlingConfig(
434440
self.writers.events
435441
)

synapse/replication/tcp/handler.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
from synapse.replication.tcp.streams._base import (
6969
DeviceListsStream,
7070
ProfileUpdatesStream,
71+
QuarantinedMediaStream,
7172
StickyEventsStream,
7273
ThreadSubscriptionsStream,
7374
)
@@ -237,6 +238,15 @@ def __init__(self, hs: "HomeServer"):
237238

238239
continue
239240

241+
if isinstance(stream, QuarantinedMediaStream):
242+
if (
243+
hs.get_instance_name()
244+
in hs.config.worker.writers.quarantined_media_changes
245+
):
246+
self._streams_to_replicate.append(stream)
247+
248+
continue
249+
240250
# Only add any other streams if we're on master.
241251
if hs.config.worker.worker_app is not None:
242252
continue
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
#
2+
# This file is licensed under the Affero General Public License (AGPL) version 3.
3+
#
4+
# Copyright (C) 2026 Element Creations Ltd
5+
#
6+
# This program is free software: you can redistribute it and/or modify
7+
# it under the terms of the GNU Affero General Public License as
8+
# published by the Free Software Foundation, either version 3 of the
9+
# License, or (at your option) any later version.
10+
#
11+
# See the GNU Affero General Public License for more details:
12+
# <https://www.gnu.org/licenses/agpl-3.0.html>.
13+
#
14+
15+
from synapse.replication.tcp.streams import QuarantinedMediaStream
16+
from synapse.types import UserID
17+
18+
from tests.replication._base import BaseMultiWorkerStreamTestCase
19+
20+
21+
class QuarantinedMediaWorkerWriterTestCase(BaseMultiWorkerStreamTestCase):
22+
"""Checks that the quarantined_media stream is replicated when the
23+
configured stream writer is a worker rather than the main process.
24+
"""
25+
26+
def default_config(self) -> dict:
27+
conf = super().default_config()
28+
conf["stream_writers"] = {"quarantined_media_changes": ["worker1"]}
29+
conf["instance_map"] = {
30+
"main": {"host": "testserv", "port": 8765},
31+
"worker1": {"host": "testserv", "port": 1001},
32+
}
33+
return conf
34+
35+
def test_quarantine_on_worker_writer_replicates_to_main(self) -> None:
36+
main_store = self.hs.get_datastores().main
37+
38+
worker_hs = self.make_worker_hs(
39+
"synapse.app.generic_worker", {"worker_name": "worker1"}
40+
)
41+
worker_store = worker_hs.get_datastores().main
42+
43+
# The worker must consider itself a source of the stream...
44+
self.assertIn(
45+
QuarantinedMediaStream.NAME,
46+
{
47+
stream.NAME
48+
for stream in worker_hs.get_replication_command_handler().get_streams_to_replicate()
49+
},
50+
)
51+
# ... and the main process must not, as it isn't a writer.
52+
self.assertNotIn(
53+
QuarantinedMediaStream.NAME,
54+
{
55+
stream.NAME
56+
for stream in self.hs.get_replication_command_handler().get_streams_to_replicate()
57+
},
58+
)
59+
60+
# Quarantining only records a change for media that exists.
61+
self.get_success(
62+
main_store.store_local_media(
63+
media_id="media_id1",
64+
media_type="text/plain",
65+
time_now_ms=self.clock.time_msec(),
66+
upload_name=None,
67+
media_length=100,
68+
user_id=UserID.from_string("@user:test"),
69+
)
70+
)
71+
72+
initial_token = main_store.get_current_quarantined_media_stream_id()
73+
74+
# Quarantine the media on the worker, i.e. the configured writer.
75+
self.get_success(
76+
worker_store.quarantine_media_by_id("test", "media_id1", "@admin:test")
77+
)
78+
79+
self.replicate()
80+
81+
# The main process only learns of the new stream ID over replication,
82+
# even though the two instances share a database.
83+
self.assertEqual(
84+
main_store.get_current_quarantined_media_stream_id(),
85+
initial_token + 1,
86+
)

0 commit comments

Comments
 (0)