fixes related to room v12 and sytest

changelog.d/19898.bugfix

@@ -0,0 +1,3 @@
+Fix third-party (3pid) invites over federation failing intermittently in version 12 rooms, whose room IDs no longer encode a server name.
+
+Fix `/createRoom` intermittently failing with a 500 error in version 12 rooms when the same user creates several rooms at once, due to colliding room IDs.

synapse/handlers/federation.py

@@ -1539,7 +1539,14 @@ async def exchange_third_party_invite(
                     if i == max_retries - 1:
                         raise e
         else:
-            destinations = {x.split(":", 1)[-1] for x in (sender_user_id, room_id)}
+            # The sender always tells us a server to try. Pre-v12 room IDs also
+            # encode the resident server's domain, but v12+ room IDs are a hash
+            # with no domain component, so we must not treat them as a server
+            # name -- doing so raises an invalid-destination error which can
+            # abort the whole exchange before the valid destination is tried.
+            destinations = {get_domain_from_id(sender_user_id)}
+            if ":" in room_id:
+                destinations.add(get_domain_from_id(room_id))
 
             try:
                 await self.federation_client.forward_third_party_invite(

synapse/handlers/room.py

@@ -1409,30 +1409,56 @@ async def _generate_create_event_for_room_id(
         is_public: bool,
         room_version: RoomVersion,
     ) -> tuple[EventBase, synapse.events.snapshot.EventContext]:
-        (
-            creation_event,
-            new_unpersisted_context,
-        ) = await self.event_creation_handler.create_event(
-            creator,
-            {
+        # In MSC4291 rooms the room ID is the reference hash of the create event,
+        # so two rooms whose create events have identical content collide on the
+        # same room ID. This happens in practice when the same user creates
+        # several rooms at once (e.g. concurrent /createRoom requests with the
+        # same config), as the only entropy in the create event is
+        # `origin_server_ts`, which has millisecond resolution. Retry a few times
+        # on collision, perturbing `origin_server_ts` so the create event hashes
+        # to a fresh room ID. This mirrors the collision handling in
+        # `_generate_and_create_room_id` used for older room versions.
+        attempts = 0
+        while attempts < 5:
+            event_dict: JsonDict = {
                 "content": creation_content,
                 "sender": creator.user.to_string(),
                 "type": EventTypes.Create,
                 "state_key": "",
-            },
-            prev_event_ids=[],
-            depth=1,
-            state_map={},
-            for_batch=False,
-        )
-        await self.store.store_room(
-            room_id=creation_event.room_id,
-            room_creator_user_id=creator.user.to_string(),
-            is_public=is_public,
-            room_version=room_version,
-        )
-        creation_context = await new_unpersisted_context.persist(creation_event)
-        return (creation_event, creation_context)
+            }
+            if attempts > 0:
+                # Bump the timestamp to give the create event (and hence the
+                # room ID it hashes to) different content from the colliding one.
+                event_dict["origin_server_ts"] = (
+                    self.clock.time_msec() + random.randint(1, 10)
+                )
+
+            (
+                creation_event,
+                new_unpersisted_context,
+            ) = await self.event_creation_handler.create_event(
+                creator,
+                event_dict,
+                prev_event_ids=[],
+                depth=1,
+                state_map={},
+                for_batch=False,
+            )
+            try:
+                await self.store.store_room(
+                    room_id=creation_event.room_id,
+                    room_creator_user_id=creator.user.to_string(),
+                    is_public=is_public,
+                    room_version=room_version,
+                )
+            except StoreError:
+                attempts += 1
+                continue
+
+            creation_context = await new_unpersisted_context.persist(creation_event)
+            return (creation_event, creation_context)
+
+        raise StoreError(500, "Couldn't generate a unique room ID.")
 
     async def _send_events_for_new_room(
         self,

tests/handlers/test_federation.py

@@ -108,6 +108,46 @@ def test_exchange_revoked_invite(self) -> None:
         self.assertEqual(failure.errcode, Codes.FORBIDDEN, failure)
         self.assertEqual(failure.msg, "You are not invited to this room.")
 
+    def test_exchange_third_party_invite_forwards_to_sender_for_v12_room(
+        self,
+    ) -> None:
+        """When we are not resident in the room, a 3pid invite is forwarded to
+        a remote server for exchange. Pre-v12 room IDs encode the resident
+        server's domain, but v12+ room IDs are a content hash with no domain,
+        so the room ID must not be treated as a destination as doing so raises
+        an invalid-destination error and aborts the exchange.
+
+        Regression test for 3pid invites over federation failing intermittently
+        in v12 rooms.
+        """
+        sender_user_id = "@sender:remote.example.com"
+        # A v12-style room ID: a reference hash with no ":domain" suffix.
+        room_id = "!somereferencehashwithnodomain"
+
+        # Pretend we're not in the room so we take the "forward to a remote
+        # server" branch.
+        self.handler._event_auth_handler.is_host_in_room = AsyncMock(  # type: ignore[method-assign]
+            return_value=False
+        )
+        forward = AsyncMock(return_value=None)
+        self.handler.federation_client.forward_third_party_invite = forward  # type: ignore[method-assign]
+
+        self.get_success(
+            self.handler.exchange_third_party_invite(
+                sender_user_id=sender_user_id,
+                target_user_id="@target:localhost",
+                room_id=room_id,
+                signed={"mxid": "@target:localhost", "token": "sometoken"},
+            )
+        )
+
+        forward.assert_called_once()
+        destinations = forward.call_args.args[0]
+        # Only the sender's server is a valid destination; the domainless room
+        # ID must not be misinterpreted as one.
+        self.assertEqual(destinations, {"remote.example.com"})
+        self.assertNotIn(room_id, destinations)
+
     def test_rejected_message_event_state(self) -> None:
         """
         Check that we store the state group correctly for rejected non-state events.

tests/handlers/test_room.py

@@ -1,6 +1,9 @@
+from unittest.mock import patch
+
 import synapse
 from synapse.api.constants import EventTypes, RoomEncryptionAlgorithms
 from synapse.rest.client import login, room
+from synapse.types import create_requester
 
 from tests import unittest
 from tests.unittest import override_config
@@ -106,3 +109,41 @@ def test_encrypted_by_default_config_option_off(self) -> None:
             tok=user_token,
             expect_code=404,
         )
+
+
+class RoomIDCollisionTestCase(unittest.HomeserverTestCase):
+    servlets = [
+        login.register_servlets,
+        synapse.rest.admin.register_servlets_for_client_rest_resource,
+        room.register_servlets,
+    ]
+
+    def test_colliding_v12_room_ids_are_retried(self) -> None:
+        """In v12 room the room ID is the reference hash of the
+        create event, so two rooms whose create events have identical content
+        collide on the same room ID. This happens when the same user creates
+        several rooms at once (e.g. concurrent /createRoom requests with the
+        same config within the same millisecond).
+
+        Regression test: the collision must be retried transparently and both
+        rooms created with distinct IDs.
+        """
+        handler = self.hs.get_room_creation_handler()
+        user_id = self.register_user("alice", "pass")
+        requester = create_requester(user_id)
+
+        # Freeze the clock so both create events carry the same
+        # `origin_server_ts`; with identical config this forces the two room
+        # IDs to hash to the same value, reproducing the collision.
+        with patch.object(self.hs.get_clock(), "time_msec", return_value=1234567890000):
+            room_id1, _, _ = self.get_success(
+                handler.create_room(requester, {"room_version": "12"}, ratelimit=False)
+            )
+            room_id2, _, _ = self.get_success(
+                handler.create_room(requester, {"room_version": "12"}, ratelimit=False)
+            )
+
+        self.assertNotEqual(room_id1, room_id2)
+        # v12 room IDs are content hashes with no domain component.
+        self.assertNotIn(":", room_id1)
+        self.assertNotIn(":", room_id2)