feat: never partially create a managed room

famedly_control_synapse/rest/room.py

@@ -104,18 +104,60 @@ async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]:
                 CREATOR_POWER_LEVEL - 1
             )
 
+        # Fetch the group members *before* creating the room. If this fails (e.g. the
+        # Famedly Control API is unreachable), we skip room creation entirely and return
+        # an error, rather than leaving behind a partial room the client cannot recover.
+        try:
+            expected_member_external_ids = await self.room_handler.fetch_group_members(
+                validated_room_config.groups
+            )
+        except FamedlyControlError as e:
+            raise FamedlyControlError(
+                e.code,
+                e.msg,
+                errcode=e.errcode,
+                additional_fields={"groups": validated_room_config.groups},
+            )
+
         room_id, _ = await self.api.create_room(
             admin_user_id,
             validated_room_config.model_dump(by_alias=True, exclude_none=True),
         )
 
-        await self.repository.initialize_sync_token(admin_user_id)
+        # Once the room exists, any failure while assigning groups would leave a partial
+        # managed room behind. Delete the room immediately so we never end up in that
+        # state, then re-raise the error to the client.
+        try:
+            await self.repository.initialize_sync_token(admin_user_id)
 
-        # if there is a problem, or the members are only partially assigned, this will
-        # respond directly
-        await self.room_handler.assign_groups_to_room(
-            room_id, admin_user_id, validated_room_config.groups
-        )
+            await self.room_handler.assign_groups_to_room(
+                room_id,
+                admin_user_id,
+                validated_room_config.groups,
+                expected_member_external_ids=expected_member_external_ids,
+            )
+        except Exception:
+            logger.exception(
+                "Failed to assign groups to newly created room %s; deleting it to "
+                "avoid a partial managed room",
+                room_id,
+            )
+            try:
+                await self.api.delete_room(room_id)
+            except Exception:
+                logger.exception(
+                    "Failed to delete room %s during rollback; it may be left partial",
+                    room_id,
+                )
+            # assign_groups_to_room may have queued retry-queue entries for this room
+            # before failing. Drop them so the now-deleted room doesn't abort future
+            # retry-queue processing. Persist the removal too: the background processor
+            # may have already saved a snapshot containing this room, and an in-memory
+            # pop alone would leave the deleted room in the on-disk snapshot.
+            async with self.room_handler.retry_queue_lock:
+                if self.room_handler.retry_queue.rooms.pop(room_id, None) is not None:
+                    await self.room_handler.save_retry_queue_snapshot(admin_user_id)
+            raise
 
         return 200, {"room_id": room_id, "groups": validated_room_config.groups}
 

famedly_control_synapse/room_handler.py

@@ -481,8 +481,29 @@ async def get_room_creator(self, room_id: str) -> str:
         sender = create_event.sender
         return creator_from_content or sender
 
+    async def fetch_group_members(self, list_of_groups: list[str]) -> set[str]:
+        """
+        Fetch the union of external user IDs across all of the given groups.
+
+        This is separated out so that it can be called before creating a managed room:
+        if fetching the members fails (e.g. the Famedly Control API is unreachable), the
+        room creation can be skipped entirely instead of leaving behind a partial room.
+
+        Raises:
+            FamedlyControlError: If fetching the members of any group fails.
+        """
+        expected_member_external_ids: set[str] = set()
+        for group_id in list_of_groups:
+            members = await self.client.get_group_members(group_id)
+            expected_member_external_ids.update(members)
+        return expected_member_external_ids
+
     async def assign_groups_to_room(
-        self, room_id: str, admin_user: str, list_of_groups: list[str]
+        self,
+        room_id: str,
+        admin_user: str,
+        list_of_groups: list[str],
+        expected_member_external_ids: set[str] | None = None,
     ) -> None:
         """
         Provided a list of group IDs, parse and determine the external users that are
@@ -492,6 +513,10 @@ async def assign_groups_to_room(
         This may be used when answering the request to assign groups to a room, or when
         creating a room. A retry queue is provided for gracefully handling members that
         do not exist yet.
+
+        When the caller has already fetched the group members (e.g. before creating the
+        room to avoid a partial room), they can be passed in via
+        ``expected_member_external_ids`` to avoid fetching them again.
         """
         # Update room account data with new groups information
         await self.account_data_handler.add_account_data_to_room(
@@ -502,17 +527,17 @@ async def assign_groups_to_room(
         )
 
         # Get the new groups member state (desired state after update)
-        try:
-            expected_member_external_ids = set()
-            for group_id in list_of_groups:
-                members = await self.client.get_group_members(group_id)
-                expected_member_external_ids.update(members)
-        except FamedlyControlError as e:
-            raise FamedlyControlError(
-                e.code,
-                e.msg,
-                additional_fields={"room_id": room_id, "groups": list_of_groups},
-            )
+        if expected_member_external_ids is None:
+            try:
+                expected_member_external_ids = await self.fetch_group_members(
+                    list_of_groups
+                )
+            except FamedlyControlError as e:
+                raise FamedlyControlError(
+                    e.code,
+                    e.msg,
+                    additional_fields={"room_id": room_id, "groups": list_of_groups},
+                )
 
         member_mxids_mapping = (
             await self.batch_convert_external_user_ids_to_matrix_user_ids(

tests/rest/test_room.py

@@ -20,7 +20,7 @@
 from famedly_control_synapse.client import FamedlyControlError
 from famedly_control_synapse.rest.types import CreateManagedRoomRequest, CreationContent
 from famedly_control_synapse.room_handler import famedly_control_user_sync_error
-from famedly_control_synapse.types import MANAGED_ROOM_TYPE
+from famedly_control_synapse.types import MANAGED_ROOM_TYPE, ActionReason
 from tests.utils.homeserver_testcase import override_config
 from tests.utils.module_api_testcase import ModuleApiTestCase
 
@@ -341,6 +341,99 @@ def test_non_background_worker_managed_room_creation_endpoint_disabled(
 
         assert channel.code == HTTPStatus.NOT_FOUND, channel.result
 
+    def test_room_creation_skipped_when_group_fetch_fails(
+        self, mock_get_group_members
+    ) -> None:
+        """If fetching the group members fails (e.g. the Famedly Control API is
+        unreachable), no room is created and an error is returned to the client."""
+        mock_get_group_members.side_effect = FamedlyControlError(
+            HTTPStatus.INTERNAL_SERVER_ERROR, "Connection refused"
+        )
+
+        with patch(
+            "synapse.module_api.ModuleApi.create_room", new_callable=AsyncMock
+        ) as mock_create_room:
+            channel = self.make_request(
+                method="POST",
+                path=self.CREATE_PATH,
+                content=self.room_config(),
+                access_token=self.creator_access_token,
+                shorthand=False,
+            )
+
+        assert channel.code == HTTPStatus.INTERNAL_SERVER_ERROR, channel.result
+        assert "error" in channel.json_body
+        # The room must never have been created in the first place.
+        mock_create_room.assert_not_called()
+        # The groups are echoed back so the client knows which request failed.
+        assert channel.json_body.get("groups") == ["test_group"]
+
+    def test_partial_room_deleted_when_assign_fails(
+        self, mock_get_group_members
+    ) -> None:
+        """If assigning groups fails after the room has been created, the room is
+        deleted immediately so we never leave behind a partial managed room.
+
+        `assign_groups_to_room` may have queued retry-queue entries for the room
+        before failing, and the background processor may have persisted a snapshot
+        containing them. The rollback must drop the room from both the in-memory
+        queue and the on-disk snapshot, otherwise the deleted room poisons
+        retry-queue processing after a restart."""
+        mock_get_group_members.return_value = [self.invitee]
+
+        room_handler = self.hs.room_control.room_handler
+
+        async def queue_then_fail(room_id, admin_user_id, *args, **kwargs):
+            # Simulate assign_groups_to_room queuing an unresolved member and the
+            # snapshot being persisted (e.g. by the background retry processor)
+            # before the failure surfaces.
+            async with room_handler.retry_queue_lock:
+                room_handler.retry_queue.add_external_id_to_room_queue(
+                    room_id,
+                    "@missing:test",
+                    ActionReason(
+                        reason="External User ID mapping not Found",
+                        is_removal=False,
+                        retry_count=0,
+                        first_attempt_utc_ms=0,
+                        latest_attempt_utc_ms=0,
+                    ),
+                )
+                await room_handler.save_retry_queue_snapshot(admin_user_id)
+            raise FamedlyControlError(HTTPStatus.INTERNAL_SERVER_ERROR, "boom")
+
+        with (
+            patch(
+                "famedly_control_synapse.room_handler.ManagedRoomHandler.assign_groups_to_room",
+                new_callable=AsyncMock,
+            ) as mock_assign,
+            patch(
+                "synapse.module_api.ModuleApi.delete_room", new_callable=AsyncMock
+            ) as mock_delete_room,
+        ):
+            mock_assign.side_effect = queue_then_fail
+            channel = self.make_request(
+                method="POST",
+                path=self.CREATE_PATH,
+                content=self.room_config(),
+                access_token=self.creator_access_token,
+                shorthand=False,
+            )
+
+        assert channel.code == HTTPStatus.INTERNAL_SERVER_ERROR, channel.result
+        # The just-created room should have been deleted.
+        mock_delete_room.assert_called_once()
+        deleted_room_id = mock_delete_room.call_args.args[0]
+        assert deleted_room_id.startswith("!"), deleted_room_id
+
+        # The deleted room must be gone from the in-memory retry queue...
+        assert deleted_room_id not in room_handler.retry_queue.rooms
+        # ...and from the persisted snapshot
+        persisted_queue = self.get_success(
+            room_handler.get_retry_queue_snapshot(self.creator)
+        )
+        assert deleted_room_id not in persisted_queue.rooms
+
     def room_config_custom(
         self,
         creation_content: JsonDict | None = None,