This PR only handles /send_join and connected events in /send,
so everything else should return early with coherent error messages.
The join event must be connected to the main state DAG but we previously
didn't ensure this.
The cascade rejection only worked on in-flight events, not seen events.
We now overwrite events in event_map with rejected events (thus setting .rejected_reason)
to ensure we continue to cascade rejections correctly.
Complement test asserts this.
- Enable MSC4242 complement tests
- Fix an issue which caused cascade rejections to not be applied to events in the /send_join response becau
se we didn't remember in-memory which events were rejected.
- Accept rejected events in the /send_join response, it gives us a bit of robustness at no safety cost.
- Recalculate room state if backfilled events caused the state DAG fwd extrems to change.
- Don't filter out already seen events from event_map, so the statement "the event_map is the complete stat
e DAG" still holds.
- Add validation to length of prev_state_events
- Don't validate auth_events length for state DAG events as it is done too early, before we've had time to
calculate the auth events (and validation is moot anyway given it isn't attacker controlled anymore)
- Let backfilled events update state dag fwd extrems
- Add test for edge case with backfilled state events
In `/sync` there is a mode called lazy loaded (LL) members. In this mode
`/sync` does not return all memberships in the room, instead it only
returns a) membership changes, and b) the memberships of senders of
events in the timeline (if not previously sent).
Synapse implements a lazy-loaded member cache to avoid repeatedly
sending down the same memberships when a single user sends multiple
events. It does this by storing for each user/device the memberships it
has previously returned in an LRU cache. Note that it is fine for the
server to return the same membership, clients handle that fine, but it
is a waste of bandwidth.
The current lazy-loaded membership cache has three intertwined flaws:
1. It does not handle retries or clients re-using old tokens.
2. It incorrectly applies the membership cache to membership changes,
not just memberships added by lazy-loading.
3. It does not correctly handle multiple "sync streams" from the same
device.
In this PR we explicitly fix the first two issues in two commits, and
the third issue is mitigated:
1. Alongside each membership in the LL cache we store the `next_batch`
position of the response that carried it. A client that processed a
response syncs from its `next_batch` next. When a request arrives with a
`since` token, we delete every membership recorded after that token, as
they were sent in responses the client never processed. Memberships
recorded at or before `since` are kept.
2. We always return memberships that have changed, even if they appear
in the LL member cache.
These changes also half mitigate the issue of running multiple `/sync`
loops per-device (though we do not see this in the wild), as we'd at
least always return the changed membership states.
Fixes#19978 and #20278, supersedes #20279
Spawning from @Twi1ightSparkle
[spotting](https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$vbqaJ6fJJBzMe1972Z_Ny8euVPWV8IxIS3YWGxP58r0?via=jki.re&via=element.io&via=matrix.org)
a fresh Synapse [running this query every 5
seconds](https://github.com/element-hq/synapse/blob/373fa7f542d86c1dbf82c4ae87c6bec8390e263a/synapse/storage/databases/main/client_ips.py#L440-L441)
which seemed excessive. My initial sniff test thought it was fine
because the more often we run the query, the smaller number of rows we
need to process at a time but @reivilibre brought up that we still have
to scan over all of the dead tuples each time which is a fixed cost
regardless.
Ideally, we'd instead fix the pagination of the query itself to avoid
re-scanning over the tuples. That's probably also a simple change and
I'm mostly just opening this PR to have a place to chuck my worked
example somewhere (mostly for myself). We can always have another
follow-up to do the proper fix.
### `user_ips` dead tuple calculations from `matrix.org`
For `matrix.org`, our autovacuum triggers after the table has
[~5%](https://github.com/matrix-org/matrix-ansible-private/blob/1b623c950e6b4db87f1dd17fa82c83be1b3b58cb/roles/postgres_role/templates/matrix-postgresql.conf.j2#L64-L66)
dead tuples (Postgres normally has a 20% default for
[`autovacuum_vacuum_scale_factor`](https://www.postgresql.org/docs/current/runtime-config-vacuum.html#GUC-AUTOVACUUM-VACUUM-SCALE-FACTOR))
The `user_ips` table on `matrix.org` has 21.8M rows so that means we
have to wait for `(21.8M * 0.05)` = ~1.1M dead rows to accumulate before
the autovacuum kicks in and cleans up all of the dead tuples.
Upper bound napkin math: If we assume that we've reached a steady state
where we prune just as many rows as we insert over the
`user_ips_max_age` time period (defaults to [28
days](https://github.com/element-hq/synapse/blob/373fa7f542d86c1dbf82c4ae87c6bec8390e263a/synapse/config/server.py#L697));
and if the vacuum only ran because of the prune: `21.8M / 28d ~= 778k` a
day -> takes ~1.4 days to accumulate enough dead tuples.
But most of the churn probably comes from updates to the `user_ips`
since every authenticated request updates `user_ips` [every 2
minutes](https://github.com/element-hq/synapse/blob/373fa7f542d86c1dbf82c4ae87c6bec8390e263a/synapse/storage/databases/main/client_ips.py#L52-L55).
And this matches reality:
For actual metrics of `user_ips` on `matrix.org` looking at the
[Postgres
metrics](https://grafana.matrix.org/d/000000009/postgres?orgId=1&var-data_source=000000001)
we have in Prometheus/Grafana:
- ~6.2M updates per day
(`increase(pg_stat_all_tables_n_tup_upd{schemaname!~"pg_.*",
schemaname!~"information_.*", instance=~"$instance"}[1d])`)
- ~792k inserts per day
(`increase(pg_stat_all_tables_n_tup_ins{schemaname!~"pg_.*",
schemaname!~"information_.*", instance=~"$instance"}[1d])`)
- (inserts don't create dead tuples)
- ~792k deletes per day
(`increase(pg_stat_all_tables_n_tup_del{schemaname!~"pg_.*",
schemaname!~"information_.*", instance=~"$instance"}[1d])`)
- 9.17 deletes/second
-> ~7M dead tuples per day
So we wait ~3.77 hours to trigger the next autovacuum for this table
`(1.1M * (7M / 24))`. Since the majority of the dead tuples are from
updates, those dead tuples on the other side of the `last_seen` index
and we probably don't have to scan over those for the prune loop. In
between vacuums, we still end up scanning ~124k dead tuples each time we
query on the prune though `(792k * (3.7/24))`.
So even for `matrix.org` levels of busyness and more aggressive
autovacuum, the `5s` interval overkill.
Given 9.17 deletes/second, if we choose a prune loop interval duration
of `60s`, it will pick-up ~550 rows which is under the query `LIMIT` set
(`5000`) with about an order of magnitude head-room to catch-up from
downtime or peak/heavy traffic.
Part of: MSC4354,
Part of: https://github.com/element-hq/synapse/issues/19641
This PR persists events in redacted form when they already have valid
redactions. That's not directly a sticky events-specific change (and I
feel it makes sense to redact immediately before persistence when we
already have the redaction).
This causes a nice effect which is that the event's stickiness is lost
immediately before persistence, because the `msc4354_sticky` field is
redacted by the redaction algorithm.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Co-authored-by: Eric Eastwood <erice@element.io>
To use `reactor.callFromThread` we need to take the GIL. However, the
vast majority of use-cases on the Rust side involve using that from
Tokio reactor threads, where we do not want to take the GIL (as it can
potentially block for a long time).
In #20252 we added a pure Rust alternative to `callFromThread`.
`run_python_awaitable` is the last usage of `callFromThread`, and so we
replace it with `dispatch_to_twisted`.
---
The diff is a lot smaller if you hide pure whitespace changes.
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Our admin API to delete a room has a weird feature where you can supply
`new_room_user_id` and it will create a new room and join all of the
users to that room:
https://github.com/element-hq/synapse/blob/929e3524d189bf7d3402866b930cebde6bb54892/docs/admin_api/rooms.md#L640-L642
But that room can contain [suspended
users](https://spec.matrix.org/v1.19/client-server-api/#account-suspension)
which are unable to join new rooms. Currently, we use
`logger.exception(...)` for this failure which ends up in Sentry but
since this is probably expected outcome and we shouldn't elevate it to
that level of problem as it's just noise.
Spawning from seeing the error in Sentry,
https://sentry.tools.element.io/organizations/element/issues/11266279/?project=2
```
Traceback (most recent call last):
File "synapse/handlers/room.py", line 2532, in shutdown_room
await self.room_member_handler.update_membership(
File "synapse/handlers/room_member.py", line 675, in update_membership
result = await self.update_membership_locked(
File "synapse/handlers/room_member.py", line 794, in update_membership_locked
raise SynapseError(
SynapseError: 403: Joining rooms while account is suspended is not allowed.
```
Fixes#15871
### What was broken
Sending `{"auth": null}` to any endpoint that requires user-interactive
auth (for example `POST /keys/device_signing/upload`, `DELETE
/devices/{deviceId}` or `POST /register`) returned a 500 instead of the
usual 401 with the available flows. A non-object `auth` on endpoints
that do not validate the body with a model (such as `/register`) could
also 500, e.g. `{"auth": ["session"]}`.
### Root cause
`AuthHandler.check_ui_auth` did `clientdict.pop("auth", {})`, so the
`{}` default only applies when the key is missing. An explicit `null`
came through as `None` and the following `"session" in authdict` raised
`TypeError`. `get_session_id` had the same assumption that `auth` is a
dict.
### 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).
* [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))
---------
Signed-off-by: Ankit Jha <ankit.jha@tradomate.one>
Kegan Dougal