Local manually upgraded rooms should run upgrade logic for local users

[MadLittleMods]

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 (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	❌	✅

❌ 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 -> copy_user_state_on_room_upgrade ->
copy_push_rules_from_room_to_room_for_user 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: matrix-org/complement#819

Looking at the code, I could see that we only run transfer_room_state_on_room_upgrade when a) the /upgrade endpoint is used or b) when joining remote rooms.

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:

synapse/synapse/handlers/room_member.py

Lines 1344 to 1348 in 322481c

	async def transfer_room_state_on_room_upgrade(
	self, old_room_id: str, room_id: str
	) -> None:
	"""Upon our server becoming aware of an upgraded room, either by upgrading a room
	ourselves or joining one, we can transfer over information from the previous room.

synapse/synapse/handlers/room_member.py

Lines 1374 to 1378 in 322481c

	async def copy_user_state_on_room_upgrade(
	self, old_room_id: str, new_room_id: str, user_ids: Iterable[str]
	) -> None:
	"""Copy user-specific information when they join a new room when that new room is the
	result of a room upgrade

And even comparing to Dendrite, where it runs handleRoomUpgrade -> copyPushrules whenever it sees a m.room.tombstone event.

Dev notes

Complement tests: matrix-org/complement#819

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