feat: never partially create a managed room#49
Conversation
When hitting a network exception such as `ConnectionRefused`, the module created a partial room and there was no way to recover from this
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 93.27% 93.28% +0.01%
==========================================
Files 10 10
Lines 877 923 +46
Branches 131 137 +6
==========================================
+ Hits 818 861 +43
- Misses 34 36 +2
- Partials 25 26 +1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview
After the room exists,
Tests cover “no create on fetch failure” and “delete + queue cleanup when assign fails after create.” Reviewed by Cursor Bugbot for commit 6545444. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Failed delete leaves partial room
- Rollback now catches and logs delete failures while re-raising the original setup error to the client.
- ✅ Fixed: Rollback leaves orphan retry queue
- Rollback now removes and persists removal of retry-queue entries for the newly created room before deletion.
Or push these changes by commenting:
@cursor push a042b24c91
Preview (a042b24c91)
diff --git a/famedly_control_synapse/rest/room.py b/famedly_control_synapse/rest/room.py
--- a/famedly_control_synapse/rest/room.py
+++ b/famedly_control_synapse/rest/room.py
@@ -125,8 +125,8 @@
)
# 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.
+ # managed room behind. Clean up local retry state, attempt to delete the room,
+ # then re-raise the original error to the client.
try:
await self.repository.initialize_sync_token(admin_user_id)
@@ -142,7 +142,23 @@
"avoid a partial managed room",
room_id,
)
- await self.api.delete_room(room_id)
+ try:
+ await self.room_handler.remove_room_from_retry_queue(
+ room_id, admin_user_id
+ )
+ except Exception:
+ logger.exception(
+ "Failed to clean retry queue for newly created room %s during "
+ "rollback",
+ room_id,
+ )
+ try:
+ await self.api.delete_room(room_id)
+ except Exception:
+ logger.exception(
+ "Failed to delete newly created room %s during rollback",
+ 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
--- a/famedly_control_synapse/room_handler.py
+++ b/famedly_control_synapse/room_handler.py
@@ -633,6 +633,16 @@
self.retry_queue.model_dump(),
)
+ async def remove_room_from_retry_queue(
+ self, room_id: str, admin_user_id: str
+ ) -> None:
+ """
+ Remove all retry queue entries for a room and persist the updated queue.
+ """
+ async with self.retry_queue_lock:
+ if self.retry_queue.rooms.pop(room_id, None) is not None:
+ await self.save_retry_queue_snapshot(admin_user_id)
+
async def process_retry_queue(self, admin_user_id: str | None = None) -> None:
skipped = False
try:
diff --git a/tests/rest/test_room.py b/tests/rest/test_room.py
--- 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
@@ -401,6 +401,86 @@
deleted_room_id = mock_delete_room.call_args.args[0]
assert deleted_room_id.startswith("!"), deleted_room_id
+ def test_assign_error_preserved_when_rollback_delete_fails(
+ self, mock_get_group_members
+ ) -> None:
+ """A failed rollback delete is logged but does not mask the original error."""
+ 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"
+ )
+ mock_delete_room.side_effect = RuntimeError("delete failed")
+ 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 channel.json_body["error"] == "boom"
+ mock_delete_room.assert_called_once()
+
+ def test_assign_failure_rollback_cleans_retry_queue(
+ self, mock_get_group_members
+ ) -> None:
+ """Rollback removes retry entries created before assignment failed."""
+ mock_get_group_members.return_value = [self.invitee]
+ missing_external_id = "@missing:test"
+ room_handler = self.hs.room_control.room_handler # type: ignore[attr-defined]
+
+ async def fail_after_queue_update(
+ room_id: str,
+ admin_user: str,
+ list_of_groups: list[str],
+ expected_member_external_ids: set[str] | None = None,
+ ) -> None:
+ room_handler.retry_queue.add_external_id_to_room_queue(
+ room_id,
+ missing_external_id,
+ ActionReason(
+ reason="External User ID mapping not Found",
+ is_removal=False,
+ retry_count=0,
+ first_attempt_utc_ms=room_handler.clock.time_msec(),
+ latest_attempt_utc_ms=room_handler.clock.time_msec(),
+ ),
+ )
+ raise FamedlyControlError(HTTPStatus.INTERNAL_SERVER_ERROR, "boom")
+
+ with (
+ patch.object(
+ room_handler,
+ "assign_groups_to_room",
+ new=AsyncMock(side_effect=fail_after_queue_update),
+ ),
+ patch(
+ "synapse.module_api.ModuleApi.delete_room", new_callable=AsyncMock
+ ) as mock_delete_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
+ deleted_room_id = mock_delete_room.call_args.args[0]
+ assert deleted_room_id not in room_handler.retry_queue.rooms
+
def room_config_custom(
self,
creation_content: JsonDict | None = None,You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Rollback omits retry snapshot persist
- The rollback now persists the retry queue snapshot while holding its lock after removing the deleted room, with regression coverage verifying the save.
Or push these changes by commenting:
@cursor push 5f13fc22e5
Preview (5f13fc22e5)
diff --git a/famedly_control_synapse/rest/room.py b/famedly_control_synapse/rest/room.py
--- a/famedly_control_synapse/rest/room.py
+++ b/famedly_control_synapse/rest/room.py
@@ -154,6 +154,7 @@
# retry-queue processing.
async with self.room_handler.retry_queue_lock:
self.room_handler.retry_queue.rooms.pop(room_id, None)
+ await self.room_handler.save_retry_queue_snapshot(admin_user_id)
raise
return 200, {"room_id": room_id, "groups": validated_room_config.groups}
diff --git a/tests/rest/test_room.py b/tests/rest/test_room.py
--- a/tests/rest/test_room.py
+++ b/tests/rest/test_room.py
@@ -383,6 +383,10 @@
patch(
"synapse.module_api.ModuleApi.delete_room", new_callable=AsyncMock
) as mock_delete_room,
+ patch(
+ "famedly_control_synapse.room_handler.ManagedRoomHandler.save_retry_queue_snapshot",
+ new_callable=AsyncMock,
+ ) as mock_save_snapshot,
):
mock_assign.side_effect = FamedlyControlError(
HTTPStatus.INTERNAL_SERVER_ERROR, "boom"
@@ -400,6 +404,7 @@
mock_delete_room.assert_called_once()
deleted_room_id = mock_delete_room.call_args.args[0]
assert deleted_room_id.startswith("!"), deleted_room_id
+ mock_save_snapshot.assert_awaited_once_with(self.creator)
def room_config_custom(
self,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 50466c0. Configure here.
50466c0 to
59a01c3
Compare
partially created room (id)
| room_id, | ||
| ) | ||
| try: | ||
| await self.api.delete_room(room_id) |
There was a problem hiding this comment.
I think this will only work if the room has no local members, like the room's creator. We may need to reach deeper into Synapse to pull out something that will force the issue, or proactively remove anyone that is already in the room first.
(I think the second option may actually be "safer", for what its worth since we have most of the tooling for that. Then can call this function)
There was a problem hiding this comment.
A much deeper look suggests that no, the "shutdown" part of the equation should be kicking all users. Still, a test for that would be much better to prove it works, if you please?
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
Largely, I think I'm ok with this. I would have done it a bit differently, but not necessarily better 😆 I'm getting a little antsy about having so much handler-looking code on the endpoints, but I suspect that ship has sailed a bit. We'll clean it up later if it needs it. Probably should move the assign to account data to after the room is created too.
Let's add to the tests to make sure that the room is actually gone based on that comment I left about the room creator holding open that door
2 participants
When hitting a network exception such as
ConnectionRefused, the module created a partial room and there was no way to recover from thisSYN-81