element-hq / element-hq/synapse

Local manually upgraded rooms should run upgrade logic for local users

Open
#19,199 0 comments 0 reactions 0 assignees View on GitHub
A-Room-Upgrades O-Occasional S-Minor T-Defect
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

Local manually upgraded rooms should run upgrade logic for local users

### Symptoms

Spawning from the many v12 room upgrades being done over the past weeks for the public Matrix community rooms. I was experiencing my notification settings not being transferred over to the new room. Looking at my account data `m.push_rules`, I have push rules for the old room but not the new room.

### Tldr; problems

Synapse doesn't call the room upgrade logic like [`transfer_room_state_on_room_upgrade`](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1344-L1348) (which copies over room tags and push rules for notifications) when a room was manually upgraded by someone sending a `m.room.tombstone` event or even when a local user eventually joins the room.

We can also visualize the hole we're experiencing with this table:

Upgrade method | Local users | Remote users
--- | --- | ---
Using `/upgrade` | ✅ | ✅
Manual `m.room.tombstone`/ `m.room.create` with `predecessor` | :x: | ✅

More nuance

Upgrade method | Local users | Remote users
--- | --- | ---
Manual `m.room.create` with `predecessor` | :x: | :x: (only works when joining room that already has it set)
Manual `m.room.tombstone` | :x: | :x:

:x: means that the upgrade logic was not run (which copies over room tags and push rules for notifications, and room aliases)

### Investigation

Before knowing the exact problem, looking into the source of Synapse, I could see that we do have [`transfer_room_state_on_room_upgrade`](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1344-L1348) -> [`copy_user_state_on_room_upgrade`](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1374-L1378) ->
[`copy_push_rules_from_room_to_room_for_user`](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/storage/databases/main/push_rule.py#L943-L947) that should take of transferring over the notification settings.

I even wrote some Complement tests using `/upgrade` to try to reproduce but it worked as expected: https://github.com/matrix-org/complement/pull/819

Looking at the code, I could see that we only run [`transfer_room_state_on_room_upgrade`](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1344-L1348) when a) the [`/upgrade` endpoint](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room.py#L414-L417) is used or b) when [joining remote rooms](https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/federation.py#L780-L782).

On the surface, this appears sufficient but there is a hole in this logic where if someone just manually creates a new room (with the `predecessor` set) and sends a `m.room.tombstone` in the old room, things work fine for other remote homeservers (because they're doing remote joins) but not the local users on the homeserver where new room was created and `m.room.tombstone` sent.

I suspected that the recent room upgrades were done manually *without* the `/upgrade` endpoint (just sending a `m.room.tombstone` in the room) which would avoid all of the extra upgrade logic. At first, I thought this behavior might be expected since you're manually doing the upgrade. But this is actually completely disparate to how everyone else remotely experiences the upgrade with seamless logic.

Even the comments suggest that we should running this logic:

https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1344-L1348

https://github.com/element-hq/synapse/blob/322481cd2d694eb5fe6107f3c0cd8d48252439bc/synapse/handlers/room_member.py#L1374-L1378

And even comparing to Dendrite, where it runs [`handleRoomUpgrade`](https://github.com/element-hq/dendrite/blob/fbbdf84ac62699ee952e091b4a8cc9577bd4f6bb/userapi/consumers/roomserver.go#L209-L227) -> `copyPushrules` whenever it sees a `m.room.tombstone` event.

### Dev notes

Complement tests: https://github.com/matrix-org/complement/pull/819

This specific scenario does have a Complement test (see PR ^) but is currently skipped because Synapse fails here.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.