Famedly release v1.152#253
Open
jason-famedly wants to merge 46 commits intomasterfrom
Open
Conversation
Run `poetry run ./scripts-dev/lint.sh` which exposed some formatted Complement tests that were introduced in element-hq/synapse#19509 There is no CI for this so it's easy to miss.
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
…us metrics (#19643)
# What is done?
- resolves #19403
- Adds build-time Rust compiler detection and captures the rustc
--version value during the build.
- Exposes the captured compiler version from the Rust extension via a
new Python-callable function.
- Exports a new Prometheus metric for rustc version.
# How to test?
- compile `poetry install`
- add `enable_metrics: true` and
```yaml
resources:
- compress: false
names:
- client
- federation
- metrics
```
to homeserver.yaml
- start synapse
- find the rustc version at `http://localhost:8008/_synapse/metrics`
### 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: Quentin Gliech <quenting@element.io>
Synapse uses topological ordering for initial sync (first time a room is sent down `/sync`), https://github.com/element-hq/synapse/blob/2e9b8202f0a1a8ceba9f02bb5ec227498d51dcbd/synapse/handlers/sync.py#L768-L805
…portdb` (#19675) Part of element-hq/synapse#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/synapse#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. ```
Fixes element-hq/synapse#19352 (See issue for history of this feature and previous PRs) > First, a [naive implementation](element-hq/synapse#19268) of the endpoint was introduced, but it quickly ran into [performance issues on query](element-hq/synapse#19312) and [long startup times](element-hq/synapse#19349), leading to its [removal](element-hq/synapse#19351). It also didn't actually work, and would fail to expose media when it was "unquarantined", so a [partial fix](element-hq/synapse#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/synapse#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/synapse#19672 ### 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: turt2live <1190097+turt2live@users.noreply.github.com> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com> Co-authored-by: Eric Eastwood <erice@element.io>
… (#19677) Following up on element-hq/synapse#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 <!-- 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: Eric Eastwood <erice@element.io> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
…19630) Incorrectly labeled in matrix-org/synapse#13535. `maybe_backfill` already accurately describes `limit` (introduced in matrix-org/synapse#8349) Spotted in element-hq/synapse#19611 (comment)
The spec says `device_keys` may be omitted, but not set to `null`. This was temporarily allowed as a workaround for misbehaving clients (see #19023), which have since been fixed. Fixes #19030
This is to make it easier to port to Rust, as well as making things conceptually simpler. Two changes: 1. Remove the `__getitem__` interface on events 2. Remove `.user_id` as an alias of `.sender`.
This adds a way to re-sign all locally-created events with a new signing key, which is useful when rotating server signing keys. This doesn't trigger automatically, instead needs to be triggered when needed via the admin API. c.f. matrix-org/internal-config#1670 (comment) for internal discussion. --------- Co-authored-by: Kegan Dougall <kegan@element.io> Co-authored-by: Erik Johnston <erikj@element.io>
Follows: #19365 Part of: MSC4354 Sticky Events (experimental feature #19409) This PR introduces a `spam_checker_spammy` flag, analogous to `policy_server_spammy`, as an explicit flag that an event was decided to be spammy by a spam-checker module. The original Sticky Events PR (#18968) just reused `policy_server_spammy`, but it didn't sit right with me because we (at least appear to be experimenting with features that) allow users to opt-in to seeing `policy_server_spammy` events (presumably for moderation purposes). Keeping these flags separate felt best, therefore. As for why we need this flag: soon soft-failed status won't be permanent, at least for sticky events. The spam checker modules currently work by making events soft-failed. We want to prevent spammy events from getting reconsidered/un-soft-failed, so it seems like we need a flag to track spam-checker spamminess *separately* from soft-failed. Should be commit-by-commit friendly, but is also small. --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
…ed with `SQLITE_DBCONFIG_DEFENSIVE` by default, such as macOS. (#19690) Fixes: #19616 This caused 2+ people trouble now, so worth batting away with a low-effort change if we can. Only seen on macOS so far, but nothing stops SQLite being configured in defensive mode by default on other platforms, so it is not necessarily entirely specific to macOS. We *could* also do this for Python < 3.12 but it'd be more effort and I don't know if it's worth it. (For context @kegsay says the interpreter with this problem was installed through `pyenv install`.) --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
When we return events to clients we need to annotate them with the membership of the user at the time of the event, in the `unsigned` section. We already check the membership at the event during the visibility checks, and so we annotate events there. However, since this a per-user field we end up having to clone the event in question. Instead, let's add a `FilteredEvent` class that is returned by the visibility checks, which allows returning the membership without editing the event. This has three benefits: 1. Avoids the clones of the event. 2. Allows us to statically check that we have filtered events before returning them to clients. 3. We no longer edit `unsigned` data after event deserialization, this makes it easier to port the event class to Rust. The last benefit is why we're doing this *now*, however IMV it shouldn't affect whether we want this change or not. Reviewable commit-by-commit --------- Co-authored-by: Olivier 'reivilibre' <oliverw@element.io>
This moves the reference counting from PyO3 into standard Rust types, allowing the class to be used natively from Rust without needing a Python runtime.
Fixes element-hq/synapse#19692 Introduced by element-hq/synapse#19558 --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
…hentication with Legacy Single Sign-On. (#19693) Closes: #19688 Part of: MSC4450 whose Experimental Feature tracking issue is #19691 Add an unstable, namespaced `idp_id` query parameter to `fallback/web` \ This allows clients to specify the identity provider they'd like to log in with for SSO when they have multiple upstream IdPs associated with their account. Previously, Synapse would just pick one arbitrarily. But this was undesirable as you may want to use a different one at that point in time. When logging in, the user is able to choose when IdP they use - during UIA (which uses fallback auth mechanism) they should be able to do the same. ----- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org> Co-authored-by: Andrew Morgan <andrew@amorgan.xyz> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
This implements [MSC4242: State DAGs](matrix-org/matrix-spec-proposals#4242), without support for federation. A general overview: - It adds a new room version and new event type. - It adds a new field `calculated_auth_event_ids` to internal metadata. - It stores the state DAG via new state DAG edges / forward extremities tables. - It adds new auth rules as per the MSC. - It uses the new `prev_state_events` field instead of `prev_event_ids()` when doing state resolution. Complement tests: matrix-org/complement#841 ### 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: Eric Eastwood <erice@element.io>
Adds [Admin API](https://element-hq.github.io/synapse/latest/usage/administration/admin_api/index.html) endpoints to list, fetch and delete user reports from the homeserver. Follows on from #18120, which added the endpoints to report users.
Fixes #13043 The usages of the table mostly already correctly handled if we don't have old entries, as that was needed when we first added the table. I arbitrarily set the prune time to 30 days. The only use for old entries is for sync streams that haven't synced since then, and we should very rarely see sync streams that haven't been used in 30 days. Reviewable commit-by-commit. --------- Co-authored-by: Olivier 'reivilibre' <oliverw@element.io> Co-authored-by: Olivier 'reivilibre' <olivier@librepush.net>
Spawning from the follow-up necessary when adding a new stream (element-hq/synapse#19694)
Follows on from #19473. We should be recording where we have deleted up to in the same transaction as we perform the delete, rather than at the end. This code only starts deleting rows after a month (and the original PR isn't in a release yet), so no server should have run into this problem yet. Also let's log more regularly, as the initial set of deletions will likely take a long time.
Both `__getitem__` and `.user_id` were removed in #19680 to simplify the event class. However, `EventBase` is exposed to modules who might also make use of those methods, so let's reinstate them (but otherwise not reinstate the usage of them in the code).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #253 +/- ##
==========================================
+ Coverage 80.22% 80.37% +0.14%
==========================================
Files 500 501 +1
Lines 71714 72337 +623
Branches 10790 10906 +116
==========================================
+ Hits 57536 58144 +608
+ Misses 10919 10911 -8
- Partials 3259 3282 +23
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Swap `hatch run cov` to `hatch test -p -c` as the former no longer exists
Contributor
There was a problem hiding this comment.
Pull request overview
Release bump to Synapse v1.152.0, pulling in upstream changes and associated tests/docs updates across the codebase (notably around event serialization, media quarantine tracking, and experimental features).
Changes:
- Introduces
FilteredEventas the serialization wrapper and updates many call sites/tests accordingly. - Adds new admin/media quarantine change tracking + stream wiring and related docs/upgrade notes.
- Adds experimental MSC4242 (state DAGs) scaffolding (non-federated), plus other release updates (deps, CI workflow reuse, etc.).
Reviewed changes
Copilot reviewed 126 out of 128 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| synapse/rest/client/auth.py | Adds MSC4450 idp_id query param support for legacy SSO UIA fallback. |
| synapse/handlers/auth.py | Extends SSO UIA flow to optionally select a preferred IdP. |
| synapse/rest/admin/media.py | Adds admin API to list quarantined media changes with pagination. |
| docs/workers.md | Documents new quarantined_media_changes stream writer routing. |
| docs/upgrade.md | Adds upgrade note for worker deployments re: quarantined media changes stream. |
| synapse/storage/schema/main/delta/94/04_device_lists_changes_max_pruned.sql | Adds table for tracking max-pruned stream ID for device list changes pruning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+78
to
+83
| if idp_id is not None and stagetype != LoginType.SSO: | ||
| raise SynapseError( | ||
| 400, | ||
| Codes.INVALID_PARAM, | ||
| "idp_id can only be specified for the `m.login.sso` auth type", | ||
| ) |
There was a problem hiding this comment.
SynapseError positional args are in the wrong order here: the 2nd arg is the human-readable message, but Codes.INVALID_PARAM is passed instead, and the message string ends up in the errcode field. Use SynapseError(400, "…", errcode=Codes.INVALID_PARAM) (or the equivalent keyword args) so clients receive the correct errcode and message.
Comment on lines
+1821
to
+1830
| idp_id, sso_auth_provider = next(iter(idps.items())) | ||
| if len(idps) > 0: | ||
| # We arbitrarily picked an IdP from multiple potential | ||
| # candidates. This may be undesirable for the user. | ||
| logger.warning( | ||
| "User %r has previously logged in with multiple SSO IdPs; arbitrarily " | ||
| "picking %r", | ||
| user_id_to_verify, | ||
| idp_id, | ||
| ) |
There was a problem hiding this comment.
The warning about multiple SSO IdPs will currently log whenever idps is non-empty (len(idps) > 0), including the common case where there is exactly one IdP. This should be len(idps) > 1 (or similar) so we only warn when a genuinely arbitrary choice was made.
Comment on lines
+261
to
+267
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "The `from` token is considered invalid because it includes stream positions " | ||
| "greater than the furthest persisted position across all of the workers." | ||
| "This indicates either a Synapse programming error (as we should never hand out " | ||
| "invalid future tokens) or a fabricated `from` token. If you've modified the token, " | ||
| "you can try paginating from the beginning again.", |
There was a problem hiding this comment.
The error message string concatenation here is missing spaces between sentences (e.g. it will render as workers.This indicates...). Add trailing/leading spaces (or use a single triple-quoted string) so the message is readable.
| can be handled by any worker, but should be routed directly to one of the workers | ||
| configured as stream writer for the `quarantined_media_changes` stream: | ||
|
|
||
| ^/_synapse/admin/v1/quarantine_media/.*$ |
There was a problem hiding this comment.
The worker routing regex looks incorrect: Synapse’s quarantine endpoints are under /_synapse/admin/v1/media/quarantine/... (and .../unquarantine/...), not /_synapse/admin/v1/quarantine_media/.... Please update the documented endpoint patterns to match the actual admin API paths so worker configs route requests correctly.
Suggested change
| ^/_synapse/admin/v1/quarantine_media/.*$ | |
| ^/_synapse/admin/v1/media/quarantine/.*$ | |
| ^/_synapse/admin/v1/media/unquarantine/.*$ |
Comment on lines
+125
to
+128
| introduced. Existing deployments which route the `/quarantine_media` endpoints to a | ||
| worker (instead of the main process) *must* also add those workers to the | ||
| `quarantined_media_changes` stream writer list. Quarantining media will not work without | ||
| this. |
There was a problem hiding this comment.
This upgrade note refers to routing the /quarantine_media endpoints, but the actual admin API paths are /_synapse/admin/v1/media/quarantine/... and /_synapse/admin/v1/media/unquarantine/.... Please update the endpoint name/path here to avoid misleading worker deployments.
Suggested change
| introduced. Existing deployments which route the `/quarantine_media` endpoints to a | |
| worker (instead of the main process) *must* also add those workers to the | |
| `quarantined_media_changes` stream writer list. Quarantining media will not work without | |
| this. | |
| introduced. Existing deployments which route the | |
| `/_synapse/admin/v1/media/quarantine/...` or `/_synapse/admin/v1/media/unquarantine/...` | |
| endpoints to a worker (instead of the main process) *must* also add those | |
| workers to the `quarantined_media_changes` stream writer list. Quarantining media | |
| will not work without this. |
Comment on lines
+27
to
+34
| CREATE TABLE IF NOT EXISTS device_lists_changes_in_room_max_pruned_stream_id ( | ||
| Lock CHAR(1) NOT NULL DEFAULT 'X' UNIQUE, | ||
| stream_id BIGINT NOT NULL | ||
| ); | ||
|
|
||
| -- We assume that nothing has been deleted from the device_lists_changes_in_room | ||
| -- table, so we can set the initial value to 0. | ||
| INSERT INTO device_lists_changes_in_room_max_pruned_stream_id (stream_id) VALUES (0); |
There was a problem hiding this comment.
The CREATE TABLE IF NOT EXISTS ... makes this migration partially idempotent, but the unconditional INSERT will fail if the migration is ever re-run (or if the table already exists for any reason). Consider guarding the insert (e.g. INSERT ... WHERE NOT EXISTS (...) / INSERT OR IGNORE depending on DB) so the delta is safe to apply when the table already exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Famedly Synapse release v1.152.0_1
Docker image tags available:
v1.152.0_1-mod026<- TIM 1.1v1.152.0_1-mod027<- TIM Pro(and TIM 1.2)No Famedly additions for v1.150.0_1
Nothing note worthy for Famedly
Requires: famedly/complement#15