refactor: Fix several bugs in upgrade endpoint, update MSC4168 #1807

Merged
nex merged 10 commits from nex/fix/room-upgrades into main 2026-05-25 19:06:41 +00:00
Owner

This pull request changes major parts of the POST /_matrix/client/v3/rooms/{roomId}/upgrade endpoint handler, which fixes several performance issues and bugs related to upgrading, and also updates the MSC4168: Update m.space.* state on room upgrade implementation to fully implement the upstream proposal (the previous implementation was buggy and partial).

Fixes #1806, fixes #1805

Pull request checklist:

  • This pull request targets the main branch, and the branch is named something other than
    main.
  • I have written an appropriate pull request title and my description is clear.
  • I understand I am responsible for the contents of this pull request.
  • I have followed the contributing guidelines:
This pull request changes major parts of the `POST /_matrix/client/v3/rooms/{roomId}/upgrade` endpoint handler, which fixes several performance issues and bugs related to upgrading, and also updates the [MSC4168: Update `m.space.*` state on room upgrade](https://github.com/matrix-org/matrix-spec-proposals/pull/4168) implementation to fully implement the upstream proposal (the previous implementation was buggy and partial). Fixes #1806, fixes #1805 **Pull request checklist:** <!-- You need to complete these before your PR can be considered. If you aren't sure about some, feel free to ask for clarification in #dev:continuwuity.org. --> - [x] This pull request targets the `main` branch, and the branch is named something other than `main`. - [x] I have written an appropriate pull request title and my description is clear. - [x] I understand I am responsible for the contents of this pull request. - I have followed the [contributing guidelines][c1]: - [x] My contribution follows the [code style][c2], if applicable. - [x] I ran [pre-commit checks][c1pc] before opening/drafting this pull request. - [x] I have [tested my contribution][c1t] (or proof-read it for documentation-only changes) myself, if applicable. This includes ensuring code compiles. - [x] My commit messages follow the [commit message format][c1cm] and are descriptive. <!-- Notes on these requirements: - While not required, we encourage you to sign your commits with GPG or SSH to attest the authenticity of your changes. - While we allow LLM-assisted contributions, we do not appreciate contributions that are low quality, which is typical of machine-generated contributions that have not had a lot of love and care from a human. Please do not open a PR if all you have done is asked ChatGPT to tidy up the codebase with a +-100,000 diff. - In the case of code style violations, reviewers may leave review comments/change requests indicating what the ideal change would look like. For example, a reviewer may suggest you lower a log level, or use `match` instead of `if/else` etc. - In the case of code style violations, pre-commit check failures, minor things like typos/spelling errors, and in some cases commit format violations, reviewers may modify your branch directly, typically by making changes and adding a commit. Particularly in the latter case, a reviewer may rebase your commits to squash "spammy" ones (like "fix", "fix", "actually fix"), and reword commit messages that don't satisfy the format. - Pull requests MUST pass the `Checks` CI workflows to be capable of being merged. This can only be bypassed in exceptional circumstances. If your CI flakes, let us know in matrix:r/dev:continuwuity.org. - Pull requests have to be based on the latest `main` commit before being merged. If the main branch changes while you're making your changes, you should make sure you rebase on main before opening a PR. Your branch will be rebased on main before it is merged if it has fallen behind. - We typically only do fast-forward merges, so your entire commit log will be included. Once in main, it's difficult to get out cleanly, so put on your best dress, smile for the cameras! --> [c1]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md [c2]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/docs/development/code_style.mdx [c1pc]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#pre-commit-checks [c1t]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#running-tests-locally [c1cm]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#commit-messages
nex self-assigned this 2026-05-24 15:08:06 +00:00
refactor: Fix several bugs in upgrade endpoint, update MSC4168 impl
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 30s
Checks / Prek / Check changed files (pull_request) Successful in 30s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m16s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m32s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 32s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m46s
7444849e04
fix: Adhere to MSC4168 more strongly & in definition order
Some checks failed
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 35s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m35s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m49s
Documentation / Build and Deploy Documentation (pull_request) Successful in 10m48s
0d10fe1198
fix: Include sender in older room versions
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 35s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m16s
Checks / Prek / Pre-commit & Formatting (pull_request) Has been cancelled
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
7c43a6336d
nex force-pushed nex/fix/room-upgrades from 7c43a6336d
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 35s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m16s
Checks / Prek / Pre-commit & Formatting (pull_request) Has been cancelled
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
to de353d08da
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 29s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m18s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m29s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 32m19s
2026-05-24 15:46:49 +00:00
Compare
nex requested review from Owners 2026-05-24 15:46:56 +00:00
fix: Correctly copy parents and children during upgrade
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 28s
Checks / Prek / Check changed files (pull_request) Successful in 28s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m24s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m30s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 32m17s
7e13d2fd11
@ -46,0 +49,4 @@
/// the new room ID.
///
/// See: https://github.com/matrix-org/matrix-spec-proposals/pull/4168
async fn msc4168_update_parent_spaces(
Owner

I don't think these need the MSC prefix in the function name

I don't think these need the MSC prefix in the function name
Author
Owner

idk i'd like some indication that the functionality is for an unstable proposal. realistically I'd like us to be able to actually do feature gating too but that's out of scope. Would something like unstable_update_parent_spaces suffice?

idk i'd like some indication that the functionality is for an unstable proposal. realistically I'd like us to be able to actually do feature gating too but that's out of scope. Would something like `unstable_update_parent_spaces` suffice?
Owner

shrug, I think that having the MSC link in the doc comment is fine. saves us from having to rename the functions when the MSC becomes stable (or more likely forgetting to do that)

shrug, I think that having the MSC link in the doc comment is fine. saves us from having to rename the functions when the MSC becomes stable (or more likely forgetting to do that)
nex marked this conversation as resolved
@ -69,0 +270,4 @@
if !services.config.allow_unstable_room_versions
&& UNSTABLE_ROOM_VERSIONS.contains(&body.new_version)
{
return Err(Error::BadRequest(
Owner

this could be combined with the conditional ahead instead of returning the same error message twice

this could be combined with the conditional ahead instead of returning the same error message twice
Author
Owner

I tried that but it looked ugly and hard to follow, I'd rather be explicit

I tried that but it looked ugly and hard to follow, I'd rather be explicit
nex marked this conversation as resolved
@ -85,3 +293,1 @@
// Note that this does internally call the policy server with a fake room ID,
// which may not be good?
let tombstone_test_result = services
// Check tombstone permission by attempting to create (but not send) the event.
Owner

why can't we check power levels?

why can't we check power levels?
Author
Owner

forward extremities might mean the tombstone would soft fail (well more accurately we'd refuse to create the event anyway), so it's more efficient to just create and discard an event. Also technically has a micro advantage in that it'll warm up the cache for the room about to be tombstoned before it actually gets tombstoned

forward extremities might mean the tombstone would soft fail (well more accurately we'd refuse to create the event anyway), so it's more efficient to just create and discard an event. Also technically has a micro advantage in that it'll warm up the cache for the room about to be tombstoned before it actually gets tombstoned
@ -230,0 +377,4 @@
// NOTE: Using a hardcoded room ID for the temporary mutex means only one room
// can be created at a time. This is actually beneficial, as it reduces the
// risk of concurrent in-flight collisions.
services.rooms.state.mutex.lock("!new-room").await
Owner

this needs to be refactored at some point in the future so we don't need to have a lock to create a room

this needs to be refactored at some point in the future so we don't need to have a lock to create a room
Author
Owner

I've got a branch I'm doing that in but it's a rather large refactor so this will make do for the time being

I've got a branch I'm doing that in but it's a rather large refactor so this will make do for the time being
nex marked this conversation as resolved
@ -46,0 +149,4 @@
// In rooms that reference the old room via m.space.parent events...
// NOTE: Doing that would be expensive. We'll instead fetch rooms which the
// space claims are children.
Owner

We would only need to do this for rooms which the upgrading user is joined to and has permission to send the state event to. There are a few other optimisations regarding the rules of what is a valid parent reference too. This is probably a later enhancement, though.

We would only need to do this for rooms which the upgrading user is joined to and has permission to send the state event to. There are a few other optimisations regarding the rules of what is a valid parent reference too. This is probably a later enhancement, though.
nex marked this conversation as resolved
nex force-pushed nex/fix/room-upgrades from 7e13d2fd11
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 28s
Checks / Prek / Check changed files (pull_request) Successful in 28s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m24s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m30s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 32m17s
to 5f88abf341
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m19s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m30s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-05-25 18:37:42 +00:00
Compare
style: Combine "unsupported version" checks
All checks were successful
Checks / Prek / Check changed files (pull_request) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m16s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 10m9s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 27s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m16s
Documentation / Build and Deploy Documentation (push) Successful in 10m50s
Checks / Prek / Check changed files (push) Successful in 29s
Checks / Prek / Pre-commit & Formatting (push) Successful in 21m24s
Checks / Prek / Clippy and Cargo Tests (push) Successful in 9m10s
Release Docker Image / Build linux-arm64 (release) (push) Successful in 11m57s
Release Docker Image / Build linux-amd64 (release) (push) Successful in 53m31s
Release Docker Image / Create Multi-arch Release Manifest (push) Successful in 10m4s
Release Docker Image / Build linux-arm64 (max-perf) (push) Successful in 30m46s
Release Docker Image / Build linux-amd64 (max-perf) (push) Successful in 1h14m51s
Release Docker Image / Release Binaries (push) Has been skipped
Release Docker Image / Create Max-Perf Manifest (push) Successful in 17s
Release Docker Image / Mirror Images (push) Successful in 2m11s
5cba4b126f
Jade approved these changes 2026-05-25 18:47:59 +00:00
nex scheduled this pull request to auto merge when all checks succeed 2026-05-25 18:50:31 +00:00
nex merged commit 5cba4b126f into main 2026-05-25 19:06:41 +00:00
nex deleted branch nex/fix/room-upgrades 2026-05-25 19:06:42 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
continuwuation/continuwuity!1807
No description provided.