Fixes#19561
`GET /_synapse/admin/v1/statistics/database/rooms` returns a 500 when
`pg_stats` has no common `room_id` values for one of the tables it
checks. This happens in two cases:
- The table is empty or not yet analyzed, so `pg_stats` has no row for
it.
- No value is more common than the others, so `most_common_vals` is
`NULL`. `event_edges.room_id` hits this on servers where all rows were
written after schema 72 stopped filling the column.
The change skips the table in both cases, since it adds nothing to the
estimate.
Thanks to @christianarielli for finding the cause and suggesting the
fix. They're credited as co-author on the commit.
The new test runs only on Postgres, as the endpoint doesn't support
SQLite. It creates a room, runs `ANALYZE` while `event_push_actions` is
empty and `event_edges.room_id` is all `NULL`, and checks for a 200 with
the room listed. I ran it against Postgres 17.
`event_edges.room_id` isn't written any more, so dropping that table
from the list might make sense. I left that out to keep this PR small.
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [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: Christian Arielli <903609+christianarielli@users.noreply.github.com>
Co-authored-by: Eric Eastwood <erice@element.io>
This is something that came up during the Rust spike, where running lots
of unit tests in parallel used lots of memory.
This is primarily due to a) `HomeServer` object having circular
references during tests, and b) us not doing full GC all the time.
The fix is to ensure that we drop references to the `HomeServer` at the
end of each test (or only keep weakrefs around), so that the GC-0
collection we run at the end of each test correctly cleans it up.
Requires https://github.com/element-hq/synapse/pull/20512 to fully clean
everything up.
This fixes the test `ThreadedMemoryReactorClock` to support
`removeSystemEventTrigger`, which in turn allows the shutdown triggers
to run when `HomeServer.shutdown()` is called in the tests. Previously,
`ThreadedMemoryReactorClock.removeSystemEventTrigger` raised a
`NotImplementedError`,
`cleanup_test_reactor_system_event_triggers` cleared the triggers by
hand so that a homeserver could be garbage collected. `shutdown()` now
removes them itself, so the helper goes.
We can also remove all the `tearDown` implementations that tried to
manually trigger the shutdown as its not longer necessary to do so
manually. They also didn't work, as they indexed `reactor.triggers` as
`[eventType][phase]` when the keys are `[phase][eventType]`
Fixes#20035
`_MultipartParserProtocol` assigned each `on_header_field` /
`on_header_value` / `on_part_data` value instead of accumulating it.
python-multipart calls these once per `dataReceived` chunk, so any
header name, header value or JSON part split across two chunks kept only
its last piece. A long `Location` in a federated media redirect was the
visible case: the truncated URL had no host and `get_file` failed with
`TypeError`.
The protocol now buffers header names, header values and the JSON part
in `bytearray`s and acts on them in `on_header_end` / `on_part_end`,
with a part counter replacing the `in_json` / `json_done` / `in_*`
flags. The file part still streams straight to the output stream, and
the existing `max_length` check still bounds everything that gets
buffered.
The new tests parse a file response and a redirect response split at
every byte offset, and byte by byte, and check the result matches the
unsplit parse. Before the fix, 100 and 115 split offsets respectively
produced a wrong result.
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [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))
---------
Signed-off-by: Ankit Jha <ankit.jha@tradomate.one>
Signed-off-by: Ankit Jha <jhaankit373@gmail.com>
`DeviceHandler.get_user_ids_changed`, which the sliding sync e2ee
extension and `GET /keys/changes` use, adds a member to
`newly_joined_or_invited_or_knocked_users` only when the previous
membership was none of join, invite or knock. A user going from invite
to join is skipped, so other members are never told to refresh the
devices of a user whose device list changed while they were only
invited. Classic sync does report them.
A change to join from any membership other than join is now counted.
join -> join (a profile change) is still ignored.
`test_extension_e2ee.py` covers invite -> join, invite -> join -> leave
in one window and a profile change, `test_keys.py` covers invite -> join
through `/keys/changes`. Not covered: device list changes made while a
user is only invited (#3504), which are still not announced until the
user joins.
Fixes#20486
Follows on from #20289 and #20433. Properly fixes handling of concurrent
requests (on single workers).
This tries to fix two cases:
1. We are *computing* responses for multiple syncs at the same time,
which currently causes the cache to get cross-contaminated.
2. Two syncs concurrent syncs on the wire (but don't overlap during
processing). When the second request is processed we correctly forget
any entries from the first request, however we currently still update
the cache with members returned on the second request. However, there is
no guarantee that the second request is what the client actually reads.
We use three rules to handle concurrent requests:
1. A response only uses cached members recorded at or before the
request's `since`. Members recorded after it cannot be from an ancestor
sync response.
2. Once a response has been built, we forget all already cached members
recorded after its `since`. This handles the client reusing an older
`since` token. Doing it at the end of the build rather than the start
also forgets members recorded by responses built at the same time.
3. We track the highest position seen for the device (`_newest`), from
the `since` of each request and the `next_batch` of each built response.
A response only records its members if its `next_batch` is after that
position. This handles responses that end at the same position, or that
finish out of order.
With these rules, when concurrent requests happen we throw away all
existing cached members and don't record any further members until one
sync has advanced sufficiently such that there are no longer any
concurrent sync requests.
Note that ideally we'd have some sort of per-connection identity so that
we can track this properly (like in sliding sync). However, that is a
bigger change. Instead, we note that we just need to detect concurrent
requests, and not update the cache in those cases.
Also note, can only detect concurrent requests that go to the same
worker.
Fixes#17480
Once `RemoteHandler` is connected, `whenConnected` returns a Deferred
that has already fired, so `writer` runs synchronously inside
`addCallbacks` and clears `_connection_waiter`. The next line then set
the waiter to that fired Deferred, so every later `_connect` call
returned early and logs stopped after the first line or two.
This sets the waiter before adding the callbacks, so a callback that
runs right away can still clear it. The new test logs a few more lines
over an existing connection and checks that they all arrive.
The bug came in with #9591, hence v1.30.0 in the changelog.
Thanks to @haarp for tracking this down and for the detailed
reproduction steps.
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [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))
`refresh_certificate` used to build a new `ServerContextFactory` on
every SIGHUP and then replace the factory on each listening port with a
fresh `TLSMemoryBIOFactory` wrapping the new one, reaching into the
port's `factory` attribute to do so.
Twisted asks the context factory for a context on every new connection,
so rebuilding the `SSL.Context` inside the existing factory is enough
for new connections to pick up the new certificate. `start()` now builds
the factory once, and `refresh_certificate` is only the SIGHUP handler.
This is spawning of #20452: Twisted 26.10 annotates `tcp.Port.factory`
as `Factory | None`, and mypy does not accept a `TLSMemoryBIOFactory`
there because its protocol type parameter does not match, so the old
assignment no longer type-checks. The change removes the assignment
rather than suppressing the error.
Verified with trial against Twisted 23.8.0 (our minimum declared dep),
26.4.0 (the current version in the lockfile) and 26.10.0 (the next
version), and with mypy against 26.4.0 and 26.10.0.
Reloading the log config on SIGHUP writes every record back into
Twisted's logging. `redirect_stdio_to_logs()` replaces `sys.stderr` with
a `LoggingFile` after startup, and `_load_logging_config` then rebuilds
`StreamHandler`s on it, so records hit the `logging during logging`
guard in `one_time_logging_setup` and JSON lines get wrapped a second
time.
`dictConfig` now runs with `sys.__stdout__` and `sys.__stderr__` swapped
in, and the redirected streams are put back afterwards, including when
the config is invalid. `tests/config/test_logger.py` covers both. Not
covered: Windows and configs that use a `FileHandler`.
Fixes#20091
### 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 (ruff check, ruff format and mypy run on the changed files)
Adds a `synapse_thread_cpu_seconds_total{thread_type, mode}` metric,
grouping threads into:
1. the `main` thread
2. each registered threadpool (`threadpool_*`)
3. `tokio`
4. `other`
The kernel only reports CPU time for live threads, so the collector sums
per-thread deltas between scrapes. A thread that exits loses the CPU it
used since the last scrape.
This is primarily useful for seeing how busy the main thread is vs other
ones.
### Pull Request Checklist
A notary key query that leaves out minimum_valid_until_ts is rejected
with errcode M_MISSING_PARAM and an error naming that field as required.
Callers that follow the server-server API, where the timestamp is
optional and the server is supposed to use the current time, cannot
complete the query. The criteria model typed minimum_valid_until_ts as
an integer or null but gave it no default, so validation treated the
field as required and failed before the query ran.
An explicit null was already a separate accepted value, used for
half-lifetime cache behaviour, and was not the same as omitting the
field.
The field now defaults to null, so an omitted timestamp validates. After
parsing, each key whose criteria did not include the field is replaced
with a copy that sets minimum_valid_until_ts to the current time in
milliseconds. Explicit null is left unchanged, and an empty key-id map
is left unchanged so a request for every cached key still ignores
validity. Tests cover the omitted timestamp against cache lifetime,
mixed criteria including zero and null, non-integer timestamps, and a
body without server_keys, and a bugfix newsfragment records the default.
Ran `python3 -m pytest -q tests/rest/key/v2/test_remote_key_resource.py`
in a Linux container (`python:3.12`) for the synapse RemoteKey change; 7
tests passed.
The tests in `tests/rest/key/v2/test_remote_key_resource.py` fail
without the fix in the synapse RemoteKey change (checked by reverting
the source changes).
Fixes#20466
---------
Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
This PR should be reviewable on a commit-by-commit basis. If you would
prefer that it is broken down into multiple PRs then I can do so.
Fixes to match latest MSC4388:
- Return `200` with `create_available=false` for `GET` when configured
in `authenticated` mode and no access token presented
- Return `401` for a `GET` where an invalid/expired token is presented
- Return `404` for `DELETE` where session has expired but not yet
evicted
Features:
- Rate limiting which is configurable via
`experimental_features.msc4388_rc_create` (for `POST` and is per
requester for authenticated requests or per client IP for
unauthenticated requests) and
`experimental_features.msc4388_rc_requests` (for `GET`/`PUT`/`DELETE`).
- Allow the session TTL to be configured via the
`experimental_features.msc4388_session_ttl`. As per spec this is between
2 and 5 minutes.
- Concurrent sessions are unlimited for authenticated requesters
(subject to the rate limits).
- Unauthenticated requesters have a maximum concurrent sessions limit
(default 1,000) which is configurable by
`experimental_features.msc4388_max_unauthenticated_sessions`.
Misc:
- Code and comment cleanup
The experimental feature is tracked by
https://github.com/element-hq/synapse/issues/19433 and I will update it
with the new config options when this PR merges.
Fix a bug introduced in Synapse v1.146.0 where failed remote media
downloads and URL previews would leave behind empty files in the media
store.
Regressed in #19204 which restructured
`MediaStorage.store_into_file(...)` to support running without a local
media store. As part of that, the `yield` (where the caller writes into
the file) was moved outside of the `try`/`except` that cleans up the
file on failure. Since `store_into_file(...)` is an
`asynccontextmanager`, an exception raised by the caller is thrown back
in at the `yield`, so it now bypasses the cleanup entirely.
This means any failed remote media download leaves a zero-byte file in
`remote_content/` (and similarly `url_cache/` for URL previews) without
a corresponding database row. Most of these fail before a single byte is
written, for example `NotRetryingDestination` while a remote server is
backed off, which fails instantly without any network request. Media
retention works from the database so it never cleans these up, and
because each attempt uses a fresh `filesystem_id`, clients repeatedly
requesting media (e.g. avatars of members on dead servers) keep
accumulating new ones. On a small homeserver, this added up to ~6000
orphaned files (and ~36000 directories) since upgrading to v1.146.0.
This PR moves the `yield` back inside the `try` (as it was before
#19204) so the file is removed again on failure. This applies to both
the local media store and the temporary file used when
`enable_local_media_storage` is disabled.
### Dev notes
To find orphaned files on an affected homeserver (zero-byte files with
no matching `remote_media_cache` row):
```shell
find /path/to/media_store/remote_content /path/to/media_store/url_cache -type f -size 0
```
Any of these without a matching row in `remote_media_cache` (or
`local_media_repository` for `url_cache/`) are orphans and safe to
delete, along with any empty directories left behind.
`KeyStore.get_server_keys_json` orders the `server_keys_json` rows of a
key by `ts_added_ms` and each row overwrites the previous one, so the
last notary to answer wins even when its `ts_valid_until_ms` is lower
than an earlier answer's. When that answer is stale, `StoreKeyFetcher`
returns a key below `minimum_valid_until_ts` and event signature
verification fails although a long enough key is stored.
The query now orders by `ts_valid_until_ms, ts_added_ms`, so the
longest-valid entry wins and the most recent one still wins a tie.
`tests/crypto/test_keyring.py` stores two notary responses for one key,
the longer-lived one first, and checks that the lookup and
`verify_json_for_server` use it. Not covered:
`get_server_keys_json_for_remote` and
`get_all_server_keys_json_for_remote`, which still prefer the most
recently added row.
Fixes#20358
### 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 (ruff check, ruff format and mypy run on the changed files)
Follow-up to https://github.com/element-hq/synapse/pull/20407, which
added the `excluded_user_ids` option without enforcing it. This makes
legacy `/sync` respect it: users listed in a room's `excluded_user_ids`
no longer get that room through the synthetic join. They can still read
it, as it is world-readable.
The cached lookup now maps room ID to config ID, so the per-user filter
runs in memory and adds no database query to `/sync`.
`get_server_announcement_room_ids()` is kept as a wrapper, so the
stacked PRs are unaffected.
Known limitation, documented in the config docs: changing the list does
not update rooms that clients already have. A newly excluded user keeps
the room, which stops updating. A user removed from the list gets new
events without the room's state until their next initial sync. I think
that's an okay limitation for now, as we don't really expect this list
to be used for real users
---------
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
> [!NOTE]
> Epic: [Efficient Server
Announcements](https://github.com/element-hq/synapse/issues/20240)
> Follows: https://github.com/element-hq/synapse/pull/20372
This PR first tests that every user can read the server announcements
through the "history" endpoints like
- [x] `GET /rooms/{roomId}/messages`
- [x] `GET /rooms/{roomId}/context/{eventId}`
- [x] `GET /rooms/{roomId}/event/{eventId}`
- [x] `GET /rooms/{roomId}/relations/{eventId}`
- [x] `GET /rooms/{roomId}/threads`
- [x] `GET /rooms/{roomId}/initialSync`
This already works for users who have never been in the room, because
announcement rooms are world-readable.
It doesn't for a sender who was removed from the room's config: Synapse
still uses their `leave` membership and shows them the room as it was
when they left.
This PR then treats a user who left a configured announcement room like
a user who has never been in it.
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [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: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Quentin Gliech <quenting@element.io>
- For the dedicated delayed event scheduling endpoint, drop support for
setting the sticky duration in a query parameter, as MSC4354 is slated
to use only a body parameter for that
- Restrict the sticky duration for delayed events to being strictly
positive, analogously to #20350
Fixes#19257.
When an identity server returns a non-2xx response to
`/_matrix/identity/v2/store-invite` during a third-party invite,
`ask_id_server_for_third_party_invite` lets the `HttpResponseException`
escape. The client receives a 500 `M_UNKNOWN` error and the real cause
(for example, `M_THREEPID_IN_USE`) is lost.
This change catches `HttpResponseException` and converts it with
`to_synapse_error()`, which matches how the other identity server calls
in `IdentityHandler` (bind, `requestToken`) already handle error
responses.
A new test in `ThreepidInviteTestCase` makes the identity server return
a 400 `M_THREEPID_IN_USE` error and checks that the client receives the
same code and errcode. The test fails on `develop` with `500 != 400` and
passes with this change.
Broke by #20289. Hopefully fixeselement-hq/element-web#35567.
One subtle change in #20289 was that we started to filter rooms with
`full_state` set through the cache. This *should* be safe since none of
the membership events should have been previously sent down the sync.
However, there is a bug with the lazy-loaded member cache when two
concurrent syncs for the same device happens. The updates to the cache
from the handling of one request are immediately seen by the other
request, polluting the cache.
This is a relatively common scenario in Element Web when a user reloads
the page. The sync from before the reload is still being processed when
Synapse receives the initial sync from after the reload.
The full fix to correctly handle overlapping requests is a separate PR,
but this is a stop-gap measure that should help Element Web.
This includes the following:
- Implements ['Implicit Forward
Pagination'](https://github.com/matrix-org/matrix-spec/issues/2378).
Remove the extension's `since` and `next_batch` fields. The position is
now tracked via the global `pos`, with per-room connection state
recording how far each room has been sent so that `limit` paginates
without holding `pos` back and lost responses are re-delivered.
- Discussed in
https://github.com/matrix-org/matrix-spec-proposals/pull/4480#discussion_r4165887827
- Honour the `lists` and `rooms` fields.
- Send all unexpired sticky events when a room enters scope for the
first time or is newly joined, and catch up on events sent while a room
was out of scope when it re-enters.
- Enabling the extension partway through a connection returns all
unexpired sticky events for in-scope rooms, and re-enabling it after
disabling it returns the unexpired sticky events sent in the meantime.
I tried to split this up into standalone commits though the bulk of the
change is in the third commit.
---------
Co-authored-by: Olivier 'reivilibre <git.oo@emunest.net>
## What the MSC says
From MSC4140's [Auth rule
evaluation](https://github.com/matrix-org/matrix-spec-proposals/blob/b7cb09664fed684388ba656459d08cfaf786c1d1/proposals/4140-delayed-events-futures.md#auth-rule-evaluation):
> The server SHOULD also check the event could be sent when it is
scheduled and reject the request if not. In particular, servers SHOULD
check the sender is in the room (or otherwise authorized to send the
event) to avoid storing events for rooms the user is not in.
## This PR
- Checks the event against the room's auth rules as if it were sent now,
and returns the same 403 `M_FORBIDDEN` that `/send` and `/state` would.
- Changes to the sender's own membership are exempt: a join or knock can
be sent from outside the room, which is how this PR reads the MSC's "or
otherwise authorized". They are checked by the auth rules when they are
sent.
Here are a few examples:
| Delayed event | Before | After |
| ----------------------------------------------------------------- |
--------------------------------------- |
---------------------------------------- |
| `m.room.message` from outside the room | 200 `delay_id`, fails at send
time | 403 `M_FORBIDDEN` |
| Kick of another member from outside the room | 200 `delay_id`, fails
at send time | 403 `M_FORBIDDEN` |
| `m.room.name` from a member whose power level is too low | 200
`delay_id`, fails at send time | 403 `M_FORBIDDEN` |
| **Exception**: join of a public room from outside it | 200 `delay_id`,
joins at send time | unchanged |
| Join from outside the room, then a message due after it | both
accepted, message sent once joined | join accepted, message 403
`M_FORBIDDEN` |
Part of: MSC4354 (Sticky Events, Experimental Feature tracking issue
#19409)
When a user is the sender of a sticky event that comes down sync, that
user is now in scope for having their lazy-loaded membership sent down
in sync.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Part of: MSC4354 (Experimental Feature tracking issue #19409)
Follows: #20165 (Sticky Event federation backlog + catch-up
transactions)
The MSC says that we need to tell a newly-joining server about all of
our sticky events (yes, even the ones before the server joined the
room).
This PR introduces the mechanism, building on the 'Sticky Event backlog'
mechanism introduced in #20165, to make that happen.
Most of the logic is taken from the original Sticky Events PR #18968,
but the Sticky Event backlog system did not exist back then.
(The benefit of having the persistent Sticky Event backlog is that we
don't need to rely on the federation senders not crashing to make sure
we get around to sending the Sticky Events out.)
- Add NEW_SERVER_JOINED replication command. (This is a cross-worker
'wake-up call' to check for a sticky event backlog)
We send it when persisting a state change that causes a new server to be
in the room.
We handle it by waking up federation senders for the destination,
including checking the sticky event backlog tables.
- When a server newly joins a room, we insert a sticky event backlog row
for that server, if there are any Sticky Events needing to be sent.
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Depends on: https://github.com/matrix-org/complement/pull/914
`displayname` and `avatar_url` already did not permit nulls, seemingly
from #8223.
The specification at v1.15 does not permit `null` for those fields
either, so this seems consistent with historical specification.
This only changes behaviour for 'custom' fields.
---------
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Follows: #18375
This bugfix is a prerequisite for MSC4354 (Sticky Events, Experimental
Feature tracking issue #19409) changes.
- Add a failing test for join->join being newly_joined
- Fix the join->join transition being treated as a new join
(when prev_events didn't coincidentally mask the problem)
Rationale:
`event_edges` / `prev_events` does NOT link a membership event
to its previous state.
(It could even be someone else's membership!)
In most real-world cases, this would generate a `prev_membership` of
NULL
for every normal membership change.
It appears as though the author intended some kind of 'previous state'
relationship, but if you fix the first branch of the `UNION` query
to use `current_state_delta_stream` for this (which is the source of
state transitions), then it looks very redundant with the second
branch.
It looks like the real reason we want to use the snapshots table is for
out-of-band memberships (outlier memberships).
So we continue using that for those, but ONLY for those.
The `SUSPICIOUS` if block is removed because it appears to have been
papering over this issue by deduplicating the spurious `NULL` ->
non-null transitions,
but when you remove those duplicates at the source, it causes more
trouble
because it doesn't know that what it's 'deduplicating' is actually the
same membership transition.
This means if you join, then out-of-band leave (e.g. last member on the
server
to leave), that room would appear in `newly_joined` on a new initial
sync.
(Tests did in fact catch this, too.)
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
