element-hq / element-hq/synapse
Local manually upgraded rooms should run upgrade logic for local users
- 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
Assessment
This issue has not been assessed yet.