From 6ceb695557822b161c00f9d38cd14f184e84db7b Mon Sep 17 00:00:00 2001 From: FrenchGithubUser Date: Wed, 8 Jul 2026 11:48:57 +0200 Subject: [PATCH 1/3] feat: never partially create a managed room When hitting a network exception such as `ConnectionRefused`, the module created a partial room and there was no way to recover from this --- famedly_control_synapse/rest/room.py | 40 ++++++++++++++--- famedly_control_synapse/room_handler.py | 49 +++++++++++++++----- tests/rest/test_room.py | 60 +++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 18 deletions(-) diff --git a/famedly_control_synapse/rest/room.py b/famedly_control_synapse/rest/room.py index e4e10ef..d87c0d2 100644 --- a/famedly_control_synapse/rest/room.py +++ b/famedly_control_synapse/rest/room.py @@ -104,18 +104,46 @@ 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, + ) + await self.api.delete_room(room_id) + raise return 200, {"room_id": room_id, "groups": validated_room_config.groups} diff --git a/famedly_control_synapse/room_handler.py b/famedly_control_synapse/room_handler.py index b836ee4..16f6120 100644 --- a/famedly_control_synapse/room_handler.py +++ b/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( diff --git a/tests/rest/test_room.py b/tests/rest/test_room.py index 4fad6d9..8901adc 100644 --- a/tests/rest/test_room.py +++ b/tests/rest/test_room.py @@ -341,6 +341,66 @@ 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.""" + mock_get_group_members.return_value = [self.invitee] + + 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 = FamedlyControlError( + HTTPStatus.INTERNAL_SERVER_ERROR, "boom" + ) + 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 + def room_config_custom( self, creation_content: JsonDict | None = None, From 59a01c380e27e3edfde53a56e9f3772031ae39b0 Mon Sep 17 00:00:00 2001 From: FrenchGithubUser Date: Wed, 15 Jul 2026 12:25:48 +0200 Subject: [PATCH 2/3] cursor comment --- famedly_control_synapse/rest/room.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/famedly_control_synapse/rest/room.py b/famedly_control_synapse/rest/room.py index d87c0d2..99766ea 100644 --- a/famedly_control_synapse/rest/room.py +++ b/famedly_control_synapse/rest/room.py @@ -142,7 +142,21 @@ async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]: "avoid a partial managed room", room_id, ) - await self.api.delete_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} From 6545444e6e785c399c1764a8a87938e9cb0d9769 Mon Sep 17 00:00:00 2001 From: FrenchGithubUser Date: Thu, 16 Jul 2026 11:43:55 +0200 Subject: [PATCH 3/3] tests: make sure that the persisted retry queue doesn't contain the partially created room (id) --- tests/rest/test_room.py | 43 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/tests/rest/test_room.py b/tests/rest/test_room.py index 8901adc..5566531 100644 --- a/tests/rest/test_room.py +++ b/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 @@ -372,9 +372,36 @@ 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.""" + 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", @@ -384,9 +411,7 @@ def test_partial_room_deleted_when_assign_fails( "synapse.module_api.ModuleApi.delete_room", new_callable=AsyncMock ) as mock_delete_room, ): - mock_assign.side_effect = FamedlyControlError( - HTTPStatus.INTERNAL_SERVER_ERROR, "boom" - ) + mock_assign.side_effect = queue_then_fail channel = self.make_request( method="POST", path=self.CREATE_PATH, @@ -401,6 +426,14 @@ def test_partial_room_deleted_when_assign_fails( 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,