The port of event serialization to Rust (#19837) removed
`format_event_raw`, `format_event_for_client_v1`,
`format_event_for_client_v2` and
`format_event_for_client_v2_without_room_id` from
`synapse.events.utils`, but there are modules in the wild that import
them from there.
Reimplement them as standalone pyfunctions in Rust, operating directly
on the Python dict so the original semantics are preserved exactly
(in-place mutation, returning the same dict, arbitrary non-JSON values
passing through, KeyError on a missing `unsigned` in the v1 format), and
re-export them from `synapse.events.utils`.
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
### Summary
This moves the synchronous core of client event serialization out of
`synapse/events/utils.py` and into Rust
(`rust/src/events/serialize.rs`).
Event serialization is on the hot path for `/sync`, `/messages`, and
most client-facing endpoints. Previously it was a recursive pure-Python
routine (`_serialize_event` / `_inject_bundled_aggregations` /
`only_fields`) that interleaved
CPU-bound formatting with `async` DB/IO. This PR separates those two
concerns and moves the CPU-bound half to Rust:
- **Async prep stays in Python.**
`EventClientSerializer._prepare_serialization` does all DB/IO up front:
batch-fetching redaction events and running the registered module
`unsigned`-callback hooks, for the top-level events *and* every bundled
sub-event (edits and thread latest events, which are themselves
serialized). The admin/MSC4354 config is resolved once via
`_update_config`, rather than re-checked on every recursive call as the
old code did.
- **The synchronous core moves to Rust.** Given an event plus the
pre-fetched `redaction_map`, `unsigned_additions`, and
`bundle_aggregations`, the Rust code produces the client JSON entirely
in Rust — including the v1/v2 format transforms, `only_event_fields`
filtering, redaction handling, and recursive bundled aggregations.
### Details
- The Rust entry point is a single batch function, `serialize_events`,
taking a list of `(event, membership)` pairs. The three lookup maps are
shared across the whole batch, so they're read out of Python and
converted to Rust
structures **once** per batch rather than once per event.
`EventClientSerializer.serialize_event` (singular) is a thin wrapper
that calls it with a one-element list.
- `SerializeEventConfig` is now a Rust `pyclass`, and the old
`event_format` callable is replaced by the `EventFormat` enum (`Raw` /
`ClientV1` / `ClientV2` / `ClientV2WithoutRoomId`). Call sites in
`rest/admin/events.py` and
`rest/client/{notifications,room,sync}.py` are updated to pass the enum.
`make_config_for_admin` and MSC4354 enablement now go through
`SerializeEventConfig.for_admin()` / `with_msc4354()`.
- New accessors on `EventInternalMetadata` (`redacted_by`, `txn_id`,
`device_id`, `token_id`, `delay_id`, `soft_failed`,
`policy_server_spammy`) expose to Rust the fields the serializer reads.
- The `_split_field` unit tests move from `tests/events/test_utils.py`
to a Rust test in `serialize.rs`, since the implementation moved.
### Behaviour
This is intended to be a behaviour-preserving refactor — the Rust core
mirrors the previous Python output (field ordering, v1 key promotion,
redaction placement per room version, null-`redacts` handling,
transaction-ID gating).
Existing serialization, relations, and sync tests pass unchanged.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
Co-authored-by: Eric Eastwood <erice@element.io>
Speeds up finding and deleting expired sliding sync connections in
`delete_old_sliding_sync_connections`, which previously required a
sequential scan.
On matrix.org I have a suspicion that this might end up blocking some
SSS connections during the delete, which can take minutes. Specifically,
I think the deletion blocks this delete:
https://github.com/element-hq/synapse/blob/ff19c034d300869e64878a15aed9a97f1cec59e4/synapse/storage/databases/main/sliding_sync.py#L233-L241
We didn't previously have an index because we wanted the postgres HOT
updates, however we also limit the update frequency to once every 5
minutes, so hopefully this is fine.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This means you can use `get_success(...)` anywhere regardless
of what kind of work needs to be done.
Spawning from adding some more async Rust things in
https://github.com/element-hq/synapse/pull/19846 and wanting something
more standard instead of the custom `till_deferred_has_result(...)` that
has crept in to a few files.
Alternative to https://github.com/element-hq/synapse/pull/19867 spurred
on by [this
comment](https://github.com/element-hq/synapse/pull/19867#discussion_r3441774685)
from @erikjohnston
### How does this work?
Previously, `get_success(...)` just ran in a hot-loop advancing the
Twisted reactor clock which didn't give any time for other threads to do
some work or acquire the GIL if necessary (whenever there is a hand-off
from Rust to Python, we need the GIL).
Now, `get_success(...)` loops until we see a result (until we hit the
~0.1s real-time timeout). In the loop, we call
[`time.sleep(0)`](https://docs.python.org/3/library/time.html#time.sleep)
which will "Suspend execution of the calling thread [...]" (CPU and GIL)
to allow other threads to do some work. Then like before, we advance the
Twisted reactor clock to run any scheduled callbacks which includes
anything the other threads may have scheduled.
### Does this slow down the entire test suite?
Seems just as fast as before. There is minutes variance in what we had
before and after but both are within the same range of each other.
(see PR for actual before/after timings)
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.
Introduced in: #17847
This 10-second wall-clock timeout was troublesome as it fails flakily on
slow/struggling CI runners, like the
default ones for private GitHub repositories.
The loop also silently relied on the reactor advance in `make_request`,
whereas we could just deterministically advance the reactor the known
amount of times
instead.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Got paged today for this. The sliding sync worker in question had loads
of deadlocks in the logs.
I restarted it and it got unwedged, but we should have a more robust
defence, which this PR proposes.
```
psycopg2.errors.DeadlockDetected: deadlock detected
DETAIL: Process 257324 waits for ShareLock on transaction 688227036; blocked by process 254908.
Process 254908 waits for ShareLock on transaction 688222971; blocked by process 256179.
Process 256179 waits for ExclusiveLock on tuple (302352,92) of relation 2962200779 of database 16403; blocked by process 257213.
Process 257213 waits for ShareLock on transaction 688225005; blocked by process 254905.
Process 254905 waits for ShareLock on transaction 688228814; blocked by process 257324.
HINT: See server log for query details.
CONTEXT: while inserting index tuple (183070,103) in relation "sliding_sync_connection_lazy_members"
```
I wonder if an unfortunate side effect is that these repeated attempts
leave a lot of dead tuples on the table,
which would then harm the performance of the next attempt to insert the
tuples,
I suspect making it more likely that they will deadlock again (?).
---
By acquring a `FOR NO KEY UPDATE` lock upfront before beginning work, we
can ensure that one
of the transactions gets queued behind the other one, meaning the first
one can succeed unimpeded.
`FOR NO KEY UPDATE` blocks other `FOR NO KEY UPDATE` locks and is the
weakest lock level that blocks itself.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Change `/org.matrix.msc3814.v1/dehydrated_device/[device_id]/events` to
accept GET requests instead of POST.
The original version of
[MSC3814](https://github.com/matrix-org/matrix-spec-proposals/pull/3814)
said we should delete keys after returning them from this endpoint, but
it is being updated to say we should not delete them, and therefore the
appropriate verb is GET.
Synapse already doesn't delete anything, so we just need to change to a
GET with a `next_batch` query param. (Currently it is a POST with
`next_batch` in the JSON content.)
This code was initially written by @ara4n and Claude, but both he and I
have read it and think it makes sense. I am far from a Synapse expert,
so feel free to tell me it's all wrong and point me in the right
direction.
I don't know what system tests will be affected by this, but I guess we
will see when the CI runs (right?).
This is a change to an unstable endpoint so no need for notifications
about breaking changes or similar.
Part of https://github.com/element-hq/element-meta/issues/2704
### 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:
* [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: Matthew Hodgson <matthew@matrix.org>
This reports the total count of users (split by appservice) which is
meant to be the monthless counterpart to the MAU metric.
Context:
> So this is largely for billing purposes and wanting to know the change
in the number of users. If a user is deactivated then we no longer want
to count them. Consumers *might* want to count appservice users, and
maybe count them based on the service (perhaps you change more for users
under bridge X or bridge Y).
>
> *-- https://github.com/element-hq/synapse/pull/19848#discussion_r3402216234*
After looking into it, just a couple of things to pick a bone at in the
old wording,
which I thought could be clarified for when I next come to look at this
again.
- the claim that there's a fundamental difference; I'd argue there isn't
really, it's just by convention
on some mainstream distros. So I have changed this to 'typically'
- statements that some distros fetch dependencies at build time
(probably does happen, but
traditional distros make a point of not doing this for the reasons you'd
expect).
- This was probably meant to be talking about Debian, but my observation
based on sample size of 3 is that some crates are packaged natively,
others are vendored in the respective application's source package (like
they do for us) and sometime they patch the bounds a bit
There could probably be room to talk about how distros vendoring
packages is a maintenance burden on them,
but I guess it's a bit moot as we would struggle to conform to wide
enough bounds to make everyone
happy (and anyway; I expect the distros that vendor packages have the
tooling to make this easy to
update and we do keep on top of security updates and release
frequently...)
---
Spawning from discussion in
[`#element-backend-internal:matrix.org`](https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$VttYPPUevn2S_W_rrzg2ZOXWI6aKebk2ganTgrLEWUc?via=jki.re&via=element.io&via=matrix.org)
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Follows: #19487
Part of: MSC4354 whose experimental feature tracking issue is #19409
This PR implements the Sliding Sync (MSC4186) extension described in
MSC4354, allowing sliding sync clients
to receive sticky events in a reliable way.
The logic is much the same as for oldschool sync (implementation in
#19487),
although in the sliding sync extension, the client can choose their own
limit
and must control their own pagination through an extra token in the
extension request/response bodies.
Note this does not yet send down existing sticky events in the
room when the room has been newly-joined.
This newly-discovered gap is tracked at #19662 and will be addressed for
both current sync and MSC4186 SSS soon.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Co-authored-by: Eric Eastwood <erice@element.io>
Bringing it up to parity with the other 404 StoreErrors naming the table
name already.
More helpful response when debugging issues with incomplete relations.
Concretly: a user that got partly removed and then reinstated, missing
an entry in 'profiles' table.
This should also contribute to a better understanding of #2807 and #2173
Not aware of an open ticket for this.
I came across it when I accidentally broke the feature even more (as
part of another piece of work),
then discovered there weren't tests for this.
So this is overall a low-priority drive-by fix.
Requires a fix to SyTest https://github.com/matrix-org/sytest/pull/1426
(as it depended on the bug).
<ol>
<li>
Add a test for purging rooms with `delete_local_events=False` \
Parameterised by room version, this test currently succeeds
on v2 but fails on v12.
This is because the condition checking for local events relies
on the old event ID format, which has not been used since v2.
</li>
<li>
Fix delete_local_events=False for room versions above v2 \
The event ID format changing means that we have to rely on `sender`
to know the origin of an event
</li>
</ol>
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Erik Johnston