diff --git a/CHANGELOG.md b/CHANGELOG.md index ed94412b..a4d4da66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to ### Fixed - ♻(frontend) standardize role terminology across localizations +- 🐛(backend) make start-recording atomic and fault-tolerant ## [1.15.0] - 2026-04-30 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 50b2efc4..fc1308ec 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -6,7 +6,9 @@ from logging import getLogger from urllib.parse import unquote, urlparse from django.conf import settings +from django.core.exceptions import ValidationError as DjangoValidationError from django.core.files.storage import default_storage +from django.db import IntegrityError, transaction from django.db.models import Q from django.http import Http404 from django.shortcuts import get_object_or_404 @@ -320,16 +322,27 @@ class RoomViewSet( options = serializer.validated_data.get("options") room = self.get_object() - # May raise exception if an active or initiated recording already exist for the room - recording = models.Recording.objects.create( - room=room, - mode=mode, - options=options.model_dump(exclude_none=True) if options else {}, - ) + try: + with transaction.atomic(): + recording = models.Recording.objects.create( + room=room, + mode=mode, + options=options.model_dump(exclude_none=True) if options else {}, + ) + models.RecordingAccess.objects.create( + user=self.request.user, + role=models.RoleChoices.OWNER, + recording=recording, + ) - models.RecordingAccess.objects.create( - user=self.request.user, role=models.RoleChoices.OWNER, recording=recording - ) + except (DjangoValidationError, IntegrityError): + # DjangoValidationError covers the Python-level check (full_clean); + # IntegrityError covers the race where two concurrent requests both + # pass that check and the DB-level UNIQUE constraint catches the loser. + return drf_response.Response( + {"error": f"A recording is already in progress for room {room.slug}"}, + status=drf_status.HTTP_409_CONFLICT, + ) worker_service = get_worker_service(mode=recording.mode) worker_manager = WorkerServiceMediator(worker_service=worker_service) @@ -337,9 +350,12 @@ class RoomViewSet( try: worker_manager.start(recording) except RecordingStartError: + models.Recording.objects.filter(pk=recording.pk).update( + status=models.RecordingStatusChoices.FAILED_TO_START + ) return drf_response.Response( {"error": f"Recording failed to start for room {room.slug}"}, - status=drf_status.HTTP_500_INTERNAL_SERVER_ERROR, + status=drf_status.HTTP_502_BAD_GATEWAY, ) if settings.METADATA_COLLECTOR_ENABLED and ( diff --git a/src/backend/core/tests/rooms/test_api_rooms_start_recording.py b/src/backend/core/tests/rooms/test_api_rooms_start_recording.py index 07895051..1624cbaf 100644 --- a/src/backend/core/tests/rooms/test_api_rooms_start_recording.py +++ b/src/backend/core/tests/rooms/test_api_rooms_start_recording.py @@ -140,16 +140,18 @@ def test_start_recording_worker_error( mock_worker_service_factory.assert_called_once_with(mode="screen_recording") - assert response.status_code == 500 + assert response.status_code == 502 assert response.json() == { "error": f"Recording failed to start for room {room.slug}" } - # Recording object should be created even if worker fails + # Recording object should be created even if worker fails, and moved out + # of the unique-constraint window so the room is not locked. assert Recording.objects.count() == 1 recording = Recording.objects.first() assert recording.room == room assert recording.mode == "screen_recording" + assert recording.status == "failed_to_start" # Verify recording access details assert recording.accesses.count() == 1 @@ -158,6 +160,72 @@ def test_start_recording_worker_error( assert access.role == "owner" +@pytest.mark.parametrize( + "status", + ["active", "initiated"], +) +def test_start_recording_conflict_when_already_in_progress( + status, mock_worker_service_factory, mock_worker_manager, settings +): + """Should return 409 when a second start is attempted while a recording is already active.""" + settings.RECORDING_ENABLE = True + + room = RoomFactory() + user = UserFactory() + room.accesses.create(user=user, role="owner") + + # Pre-existing active recording for the same room. + Recording.objects.create(room=room, mode="screen_recording", status="active") + + client = APIClient() + client.force_login(user) + + response = client.post( + f"/api/v1.0/rooms/{room.id}/start-recording/", + {"mode": "screen_recording"}, + ) + + assert response.status_code == 409 + assert response.json() == { + "error": f"A recording is already in progress for room {room.slug}" + } + # No new recording row, no access row leaked from the rolled-back transaction. + assert Recording.objects.count() == 1 + assert Recording.objects.first().accesses.count() == 0 + mock_worker_manager.start.assert_not_called() + + +def test_start_recording_after_worker_failure_unblocks_room( + mock_worker_service_factory, mock_worker_manager, settings +): + """Should allow a new recording when the previous recording failed.""" + settings.RECORDING_ENABLE = True + + room = RoomFactory() + user = UserFactory() + room.accesses.create(user=user, role="owner") + + mock_worker_manager.start = mock.Mock( + side_effect=[RecordingStartError("boom"), None] + ) + + client = APIClient() + client.force_login(user) + + first = client.post( + f"/api/v1.0/rooms/{room.id}/start-recording/", + {"mode": "screen_recording"}, + ) + assert first.status_code == 502 + + second = client.post( + f"/api/v1.0/rooms/{room.id}/start-recording/", + {"mode": "screen_recording"}, + ) + assert second.status_code == 201 + assert Recording.objects.count() == 2 + + def test_start_recording_success( mock_worker_service_factory, mock_worker_manager, settings ):