Conversation
…vite-only rooms (#19660)
Signed-off-by: dependabot[bot] <support@github.com>
…19591) 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>
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>
…19863) Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: dependabot[bot] <support@github.com>
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). > > *-- #19848 (comment)
…9892) `_synapse/mas/sync_devices` checks the device list against the set of devices MAS knows about, but the dehydrated device (MSC3814) is invisible to MAS, so it gets automatically deleted on each sync, which prevents dehydrated devices from working. This change excludes the dehydrated device from the check. There is similar special case code in the admin devices API (which gives it a special flag) and MAS's own legacy sync path (which filters it out). 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. A system-level test for this bug is being written here: element-hq/element-web#34034 but if you think we should have one somewhere else, please let me know. Closes #19889 ### 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 <matthew@element.io>
…19896) Change `/org.matrix.msc3814.v1/dehydrated_device/[device_id]/events` to accept GET requests instead of POST. The original version of [MSC3814](matrix-org/matrix-spec-proposals#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 element-hq/element-meta#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>
… repeated deadlocks. (#19826) 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>
…19890) 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>
Signed-off-by: dependabot[bot] <support@github.com>
…te (#19901) 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.
Modern 'MAS integration' (as we call it now) is, of course, preserved. Fixes: #19549 --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
…nc Rust (Tokio runtime/thread pool) (#19871) 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 #19846 and wanting something more standard instead of the custom `till_deferred_has_result(...)` that has crept in to a few files. Alternative to #19867 spurred on by [this comment](#19867 (comment)) 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)
…he CTE (#20182) This solves the root cause of a user opening the thread panel in Element Web DoSing synapse with recursive relation requests, starving out delayed events and causing MatrixRTC calls to drop - see matrix-org/matrix-js-sdk#5519 for papering over it clientside. Fixes #18788 Claude rationale: Postgres cannot estimate the size of a recursive CTE. When it guesses large it stops probing events by event_id and instead hashes every event in the room, so a single recursive `/relations` request in a busy room takes seconds and a Threads-panel fan-out of 30 of them can pin a client-reader's DB pool for a minute. Joining events per recursion step keeps the lookups as index probes regardless of the estimate. The recursion also moves from `UNION` to `UNION ALL`. An event carries a single `m.relates_to` and is stored as exactly one `event_relations` row (unique index on `event_id`), so the relation graph is a tree: every node is reached along one path and `UNION` never had duplicates to remove. `UNION ALL` drops the sort-and-dedupe pass over the working table on every iteration, which matters more now that each row also carries the joined events columns. A cycle from bogus events is still terminated by the depth bound, as before, and `UNION` gave no protection there anyway since such rows differ in depth. Measured on Postgres 16 against a synthetic corpus modelled on a large homeserver: 4 rooms x 300k events; one 40-reply thread rooted 280k events back in the timeline, with 2 reactions per reply; every 5th event elsewhere a reaction, plus 200 popular roots with 2000 reactions each so that the `relates_to_id statistics` are skewed the way they are in production. Default limit (6 rows), warm cache, JIT off: ``` before after single request 77 ms 1.4 ms 20 concurrent requests 1.21 s 0.14 s ``` Before: Hash Join with a Hash over all 300k events of the room (4 batches). After: Nested Loop with an Index Scan on `events_event_id_key` per row. (found/solved by fable) --------- Co-authored-by: Eric Eastwood <erice@element.io>
…tEqual` in the tests. (#20193) This also changes the rendering of our custom helper `assertIncludes`. Spawns from #20019 (comment) This PR hijacks `assertEqual` in order to substitute in our own error rendering logic for set inequality. (The motivation to do this is that we should just render sets in our preferred style by default, without having to think about `assertIncludes` with the `exact` flag or risk forgetting it.) The error rendering for `assertIncludes` is adapted to make it reusable in our `assertEqual` and to make it clearer _to me_ (I found it a bit jarring that `+` was used more like a tick, when in other test frameworks I expect to see that as a diff marker). I have tried to make it as clear as I could without being cryptic. --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Signed-off-by: dependabot[bot] <support@github.com>
Bumps [gitpython](https://github.com/gitpython-developers/GitPython) from 3.1.58 to 3.1.59. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/gitpython-developers/GitPython/releases">gitpython's releases</a>.</em></p> <blockquote> <h2>3.1.59 - Security</h2> <h2>What's Changed</h2> <ul> <li>prepare changelog for upcoming release by <a href="https://github.com/Byron"><code>@Byron</code></a> in <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2207">gitpython-developers/GitPython#2207</a></li> <li>Block file-reading Git options by <a href="https://github.com/Byron"><code>@Byron</code></a> in <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2208">gitpython-developers/GitPython#2208</a></li> <li>index: write blobs via git hash-object, not gitdb's odb.store by <a href="https://github.com/caroescm"><code>@caroescm</code></a> in <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2209">gitpython-developers/GitPython#2209</a></li> <li>Block separate git directories during clone by <a href="https://github.com/Byron"><code>@Byron</code></a> in <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2210">gitpython-developers/GitPython#2210</a></li> <li>fix: harden config parsing boundaries by <a href="https://github.com/Byron"><code>@Byron</code></a> in <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2211">gitpython-developers/GitPython#2211</a></li> <li><code>repo.index.add()</code> now respects worktree filters <a href="https://redirect.github.com/gitpython-developers/GitPython/pull/2209">gitpython-developers/GitPython#2209</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/gitpython-developers/GitPython/compare/3.1.58...3.1.59">https://github.com/gitpython-developers/GitPython/compare/3.1.58...3.1.59</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/gitpython-developers/GitPython/commit/66340d77aab9a7468f4aed3681d4ef1e3c0ec931"><code>66340d7</code></a> prepare changelog prior to release</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/a5e047d0db7047c4249c0de335585470b14d50c4"><code>a5e047d</code></a> Merge pull request <a href="https://redirect.github.com/gitpython-developers/GitPython/issues/2211">#2211</a> from gitpython-developers/config-sanitize-more</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/ef7568e3b317ce617eacda39b8b54dcdff8c3b5c"><code>ef7568e</code></a> fix: ignore includes in submodule configuration</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/4b4e47fc1224e23b0c8ee7220a7192818f2e4abb"><code>4b4e47f</code></a> fix: preserve multiline config values when writing</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/b473abb0f7de754392e1ec923f2fe296509013ab"><code>b473abb</code></a> Merge pull request <a href="https://redirect.github.com/gitpython-developers/GitPython/issues/2210">#2210</a> from gitpython-developers/fix-clone-unsafe-option</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/5ff52cccca770fd69c6caf0b8f281d3e45d599be"><code>5ff52cc</code></a> Merge pull request <a href="https://redirect.github.com/gitpython-developers/GitPython/issues/2209">#2209</a> from caroescm/fix-index-add-chmod</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/b68afff45af0f49e79a3e2d2162018986b37ad5d"><code>b68afff</code></a> Block separate git directories during clone</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/93677a00ab9dcb06cc08595fd1f88a4b4a0fa23b"><code>93677a0</code></a> fix: <code>index.add()</code> now supports filters (<a href="https://redirect.github.com/gitpython-developers/GitPython/issues/2021">#2021</a>)</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/9729ed3b948f2bde09f1f188c5311e172212b67e"><code>9729ed3</code></a> Merge pull request <a href="https://redirect.github.com/gitpython-developers/GitPython/issues/2208">#2208</a> from gitpython-developers/security-fixes</li> <li><a href="https://github.com/gitpython-developers/GitPython/commit/ce9d8e8d150e06ae2e2cc2efa229071cd3048a93"><code>ce9d8e8</code></a> prepare next release</li> <li>Additional commits viewable in <a href="https://github.com/gitpython-developers/GitPython/compare/3.1.58...3.1.59">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/element-hq/synapse/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
### Pull Request Checklist Replacement for #19752 as that has bit-rotted with #19895 being merged. `/_synapse/mas` is mounted on a worker on matrix.org and can be seen to be workerisable via * https://github.com/element-hq/synapse/blob/v1.160.0/synapse/app/generic_worker.py#L202 * https://github.com/element-hq/synapse/blob/v1.160.0/synapse/rest/synapse/client/__init__.py#L71 * https://github.com/element-hq/synapse/blob/v1.160.0/synapse/rest/synapse/mas/__init__.py#L46-L71 <!-- 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))
…le (#20210) ### Pull Request Checklist Missed off of https://github.com/element-hq/synapse/pull/19926/changes IMO. My reading of https://github.com/element-hq/synapse/blob/v1.161.0rc1/synapse/rest/client/delayed_events.py is that the new endpoint (pattern `r"/org\.matrix\.msc4140/delayed_events/(?P<delay_id>[^/]+)$"` is workerisable) given how `register_servlets` flows. However given `UpdateDelayedEventServlet` covers the same pattern but for `POST` requests, this should go in the `GET` only part of the documentation. <!-- 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))
Adds the serving functions needed for MSC4242: State DAGs. This PR adds MSC4242 support to /make_join, /send_join and /get_missing_events, as well as calculates the destinations for /send events correctly using `prev_state_events`. Built on top of #19718 for the storage functions it makes. Split out from #19425 Part of a series of 5x PRs to land the federation part of [MSC4242](matrix-org/matrix-spec-proposals#4242) ([storage](#19718), [fedclient](#20127), serving (this PR), inbound-joins, inbound-pulls). Whilst this is mostly a port of the code in #19425 there are a few changes: - `/get_missing_events` accepts message events when walking the state DAG, in which case it resolves the first hop to be that event's `prev_state_events`. The original PR made the client `/event` the message event and then set `latest=[prev_state_events]` on its own. This is not very efficient (extra round trip to fetch the event) and there's no reason why the server can't do the message->prev_state_events lookup, so we do so. This matches the MSC examples. - We cap the amount of events fetched via `/get_missing_events`. The MSC allows it, so it's a good safety check. - We sort the returned state DAG in `/send_join` by depth then event ID so it's "mostly" sorted. This is more a formality than anything else, the MSC does not mandate this, but it makes `/send_join` responses deterministic. - `notify_on_event_delivered_over_federation` is a new thing since #19425, so we include state DAG events in it like we do with state/auth_chain. This PR does remove the forced `m.federate: false` setting for MSC4242 rooms, so it makes it possible for federated MSC4242 rooms to be made. This is mostly so we can test via the endpoints. Given you must opt-in to MSC4242 via the experimental features config option, it seems reasonable to loosen this setting. The forced no-federation flag existed prior to review saying that the MSC4242 room version could itself be gated behind an experimental feature. Reviewable commit-by-commit. ### 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>
Fixes: #20167 The `Schema Diff` workflow posts a PR comment showing the effective schema diff. For PRs from forks, `GITHUB_TOKEN` is downgraded to read-only, so the comment-posting step was silently failing. ### Changes - `schema_diff.yml`: only post the comment directly when the PR is from the same repository. For forked PRs, upload the diff (and PR number) as a short-lived artifact instead of trying to comment. - `schema_diff_comment.yml` (new): triggered by `workflow_run` after `Schema Diff` completes, with `pull-requests: write` permission (granted because this workflow always runs in the context of the base repository). It downloads the artifact, if present, and posts the comment on behalf of the forked PR. This avoids `pull_request_target`, per the security concerns raised in the issue (zizmor flags it as dangerous). The new workflow only ever treats the downloaded artifact as inert comment text -- it is never executed. --------- Co-authored-by: Olivier 'reivilibre <oliverw@element.io>
Part of: #19415 TLDR: return the `M_UNKNOWN_DEVICE` error code instead of the unstable `ORG.MATRIX.MSC4326.M_UNKNOWN_DEVICE` identifier. > **[Added in `v1.17`]** Application services MAY similarly masquerade as a specific device ID belonging the user ID through use of the `device_id` query string parameter on the request. If the given device ID is not known to belong to the user, the server will return a 400 `M_UNKNOWN_DEVICE` error. > > — [Matrix v1.19, Application Service API — Identity assertion](https://spec.matrix.org/v1.19/application-service-api/#identity-assertion) Synapse returns the correct 400, but with the unstable identifier. MSC4326 was stabilized in Matrix 1.17 and its experimental flag was already removed in #19033; only the error code identifier was left behind. Before: ``` GET /_matrix/client/v3/sync?user_id=@alice:test&device_id=NOT_A_REAL_DEVICE_ID # appservice token 400 {"errcode": "ORG.MATRIX.MSC4326.M_UNKNOWN_DEVICE"} ``` After: ``` GET /_matrix/client/v3/sync?user_id=@alice:test&device_id=NOT_A_REAL_DEVICE_ID # appservice token 400 {"errcode": "M_UNKNOWN_DEVICE"} ``` I verified that no implementation was currently handling the prefixed error code.
Part of: #19414 When [MSC4133](matrix-org/matrix-spec-proposals#4133) (custom profile fields) was implemented, the returned value for an unset display name changed from `200 {}` to `200 { displayname: null }`. This happened first on the unstable `uk.tcpip.msc4133` path in #17488 (1.123.0), then on the stable path when #18635 (1.135.0) unified the `displayname`, `avatar_url` and custom field servlets. Neither PR discussed the change in review, so it looks like an unintended side effect of the refactor rather than a deliberate decision. The v1.16 spec mandated to change from returning `200 {}` to `404` but change was not identified as breaking and was eventually not implemented in other clients and server. This PR has a sister MSC that proposes to return to the pre-1.16 error codes: [MSC4537](matrix-org/matrix-spec-proposals#4537). Before: ``` GET /_matrix/client/v3/profile/@alice:test/displayname 200 {"displayname": null} ``` After: ``` GET /_matrix/client/v3/profile/@alice:test/displayname 200 {} ``` ### 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))
… changes, making federation support more reliable. (#20204) Part of: MSC4354 Experimental feature tracking issue: #19409 Related Complement tests currently in https://github.com/matrix-org/complement/pull/806/files#diff-6c9d6d169485d0848c6b20dd9b43f6fe669a8a710e42f953d08fa25a99cc8f4cR509 --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
…omplement suite fails. (#20161) Supersedes: element-hq/synapse-private#155 It would be useful to have the in-repo Complement suite give a status, even when the normal suite fails (e.g. flakes). --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org> Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <andrew@amorgan.xyz> Signed-off-by: Skye Elliot <actuallyori@gmail.com>
…eparation and completion. (#20166) A key refactoring for, and split out of, #20165 Would be easier to land first to isolate the diff. Should be a standalone change with no behavioural change. Motivation is that #20165 will round-robin between 'main queue' transactions and 'sticky event' transactions. To keep the data flow clear, I wanted to insert a typed struct (well, `attrs` dataclass) as an interface between the 'preparation' of a transaction and its 'completion'. Doing this whilst keeping the asynchronous context manager style did not lead to a readable result in my opinion. (I would also say the async context manager is a touch 'magic' / obscures control flow, but I suspect this is largely down to opinion.) Replace _TransactionQueueManager with prepare/complete transaction methods --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
…room version "12" creation events (#19768)
…ith a `null` value. (#20145) Instead, treat them as absent fields as they feel like they should be. The database implementation detail that these fields have a dedicated column with `NULL` when unset is kept to the storage layer. The goal here is to reduce the amount of special casing needed for these two original profile fields and treat them a little bit more like regular profile fields. Follows: #20003 Follows: #20147 (needed as a bugfix to continue sending them down oldschool sync when they get deleted. Without #20147, this PR would break that — which matches how custom profile fields were broken too.) --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Adds a `redis.username` config option. Details: A `username` without a `password` (or `password_path`) is refused at startup. Redis has no wire form for a username without a password, and txredisapi only sends `AUTH` when a password is set, so the username would otherwise be silently ignored. An explicitly empty password is accepted, since that is how a `nopass` ACL user is configured. This relies on txredisapi 1.4.12, the first release to accept a `username` kwarg. That upstream support was contributed by @karolyi specifically to unblock this. Fixes #19238. ### 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: Claude Sonnet 5 <noreply@anthropic.com>
…token falls inside a persist batch (#20171) This PR fixes the issue described as comment here: #18793 (comment) In Element Call, this shows up as ghost participants: someone who left the call keeps being displayed until a later state change refreshes the room. The bug is not specific to Element Call: any state event can be affected, RTC membership just changes often enough to make it visible. ## What happens Alice has a client syncing against a homeserver where events are persisted by one worker (the event persister) and `/sync` is served by another (the sync worker). Her client is parked in a long-poll: `GET /sync?since=s99&timeout=30000`. Bob joins a call at the same moment Carol sends a message. Carol's message reaches the persister first; Bob's `m.call.member` arrives while that write is still in flight, so the per-room persist queue groups them into one transaction: ``` events (each gets its own stream ordering): stream_ordering 100: m.room.message Carol stream_ordering 101: m.call.member Bob (state) current_state_delta_stream (how state_after finds state changes): stream_id 100 ────► (m.call.member, @bob) -> $bob_join_call ▲ └─ stamped with the batch MINIMUM (100), not the event's own 101 (see `_update_current_state_txn`) ``` The transaction commits: both events and the delta row are now in the database, atomically. The persister then announces the new events over replication, one RDATA token per stream ordering — rows are only merged into one token when they share a position, and 100 and 101 don't. So the sync worker's events-stream position steps 99 → 100 → 101, and on reaching 100 it pokes the notifier. Alice's long-poll wakes at exactly that moment. Her response is built at the worker's *current* position — `end = 100` — with RDATA 101 still in the queue: ``` Sync A (since=99, end=100): timeline: events 99 < ordering ≤ 100 → [Carol's message] state_after: deltas 99 < stream_id ≤ 100 → [$bob_join_call] ← delivered EARLY next_batch: s100 ← mid-batch token ``` No race on the client's side is needed: the server *hands out* the mid-batch token as `next_batch`. Alice's client re-polls with it, as every sync client does. The worker has meanwhile processed RDATA 101: ``` Sync B (since=100, end=101): timeline: events 100 < ordering ≤ 101 → [Bob's m.call.member @101] ✓ state_after: deltas 100 < stream_id ≤ 101 → [] row is stamped 100 ✗ ``` A state event in the timeline with an empty `state_after`. An MSC4222 client trusts `state_after` over timeline state events, so Alice's copy of Bob's call membership never updates from this response. On a single process this cannot happen: the batch's stream IDs are released as a whole, so the position visible to `/sync` jumps 99 → 101 and `s100` is never handed out. Only a process that learns its position from replication — any sync worker — ticks through the middle of a batch. ### 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))
) This partially fixes the bug #20116 Device list update EDUs from non-compliant (grandfathered historical) user IDs are currently accepted over federation, stored, and surfaced to clients in `/sync`'s `device_lists.changed` array. > For current room versions, servers must still accept events using such user IDs over federation; however they SHOULD NOT forward such user IDs to clients when referenced outside the context of an event. For example, device list updates from non-compliant user IDs would be dropped by the receiving server. > > -- [Matrix spec](https://spec.matrix.org/v1.14/appendices/#historical-user-ids), clarified in Matrix v1.14 by [matrix-spec#1506](matrix-org/matrix-spec#1506) ### Problem Example A remote server sends an `m.device_list_update` EDU for `@héllo:remote.example` (non-ASCII localpart, outside the compliant U+0021–U+007E range). Synapse: - accepts and processes the update (resyncing the user's device list if needed) - stores it in the remote device list cache - forwards `@héllo:remote.example` to local clients via `device_lists.changed` in `/sync` (**the leak** — a non-compliant user ID referenced outside event context) --- ### 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))
Resolve the conflicts in the delayed events handler, servlet, store, schema notes and tests: - Per-user cap on scheduled delayed events: take develop's (#19539), which answers 429 M_LIMIT_EXCEEDED as the MSC specifies, and drop the PR's experimental config key and its test. The cap's COUNT and its Retry-After subquery exclude finalised rows, as those now stay in the table. - Lookup by delay ID: take develop's GET /delayed_events/{delay_id} (#19926) and drop the PR's ?delay_id= filter on the list. The PR's ?status= filter is kept. The single lookup and the list exclude finalised rows, as on develop they only returned pending events. - Store: keep the PR's finalisation columns and methods on top of develop's Duration delays, sticky_duration_ms and reprocess_events. The finalised list builds its delayed_event objects through develop's DelayedEventResponseLegacyCompat (#19926), so that they carry the same fields as the scheduled list. - Handler: keep the PR's _send_event(event, finalise_error) and prune looping call; keep develop's delay_id plumbing (#19479), management ratelimiter (#19794) and sticky handling (#19365). - Schema: SCHEMA_VERSION is 94 on develop, so the PR's delta moves from delta/93/02 to delta/94/11 and its background update ordering to 9411.
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
20 participants
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.
Brings #19038 up to date with
develop. This is a single merge commit on top of the PR's head, nothing else.I recommend to use
git show --remerge-diff 58ee03480bto see only the relevant changes.git show --remerge-diff 58ee03480bResolution policy: keep the PR's behaviour wherever the MSC backs it or is silent; take
develop's behaviour wheredevelopalready moved to what MSC4140 (merged on 2026-09-08) now says. Each change to the PR's code is listed below with thedevelopPR that caused it.Changes to the PR's code
Per-user limit on scheduled delayed events
experimental_features.msc4140_max_delayed_events_per_user(default 100), checked in the store, answering400 M_UNKNOWNwithorg.matrix.msc4140.errcode: M_MAX_DELAYED_EVENTS_EXCEEDED.develophas the same cap asmsc4140_max_delayed_events_per_userin the server config, answering429 M_LIMIT_EXCEEDEDwith aRetry-Afterheader, which is what the MSC specifies (M_MAX_DELAYED_EVENTS_EXCEEDEDis listed under its rejected alternatives).add_delayed_eventin the store and thelimit=argument in the handler. Tookdevelop's; dropped the PR's config key, now unused, and itstest_add_delayed_event_num_limit, which asserted the PR's error. As the PR keeps finalised rows indelayed_events,develop'sCOUNT(*)and itsRetry-Aftersubquery filter onfinalised_ts IS NULL; without that,develop's cap tests fail once their events have been sent.Lookup by delay ID
?delay_id=(repeatable), alongside?status=and?from=.developaddedGET /delayed_events/{delay_id}, the MSC's single-event lookup.develop's endpoint and dropped the?delay_id=filter; the PR's?status=filter and itsfromparsing are kept.develop's single lookup and list select onis_processed = FALSE, which does not exclude the rows the PR finalises as cancelled, so both queries also filter onfinalised_ts IS NULL; the single lookup becomes raw SQL for that, assimple_select_onecannot expressIS NULL.Finalised list entries carry
develop's field namesdelayed_eventobject of each finalised entry by hand, withdelayandrunning_since.developrenamed those fields todelay_msanddelayed_since_ts, keeping the old names on the list throughDelayedEventResponseLegacyCompat.delayed_event, and the scheduled entry now carries both sets of names. The finalised entry is built throughDelayedEventResponseLegacyCompat.asdict()so that it does too.No
txn_idwhen sending a delayed eventcreate_and_send_nonmember_event, so a sent delayed event carries notransaction_idinunsigned, as the MSC says it should not.developadded adelay_id=argument right next to thetxn_id=one.develop'sdelay_id=.API changes on
develop, no behaviour changeaddtakesdelay: Durationand a requiredsticky_duration_ms(#19539, #19365).Clock.looping_calltakes aDuration, so the prune interval becomesDuration(minutes=5)(#19229).process_timeout_delayed_eventsgainedreprocess_events(#19207); itsis_processedclause is combined with the PR'sfinalised_ts IS NULL.StoreErroris imported fromsynapse.storage.database, as ondevelop.Schema delta
SCHEMA_VERSIONis 94 ondevelop, so the PR's delta moves fromdelta/93/02_todelta/94/11_, its background update ordering from 9302 to 9411, and its note to the version 94 list.Changes to the PR's tests
develop's new tests intest_delayed_events.pyrouted through the PR's helpers_get_delayed_events()returns a(scheduled, finalised)pair and adds_get_scheduled_delayed_events();develop's returns a plain list.developadded call sites intest_delayed_event_lookup(#19926), intest_delayed_member_events_are_sent_on_timeoutand the_find_sent_delayed_eventchecks (#19479), and renamed the content variables of the delayed-state tests (#19360).test_rooms.pytest_add_delayed_event_num_limit(see the cap above) and theservletsoverride that only it needed.develop's own cap tests from #19539 run unchanged.