mirror of
https://github.com/suitenumerique/meet.git
synced 2026-08-27 18:56:58 +00:00
🐛(backend) make start-recording atomic and fault-tolerant
Wrap Recording and RecordingAccess creation in a single transaction so a partial failure does not leave orphan rows, and return 409 instead of 500 when a recording is already in progress for the room. When the worker fails to start, transition the Recording to FAILED_TO_START so the unique partial constraint on (room, status) no longer blocks future recording attempts on the same room.
This commit is contained in:
committed by
aleb_the_flash
parent
6830250f2c
commit
a2bccf4f4f
@@ -15,6 +15,7 @@ and this project adheres to
|
|||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
- ♻(frontend) standardize role terminology across localizations
|
- ♻(frontend) standardize role terminology across localizations
|
||||||
|
- 🐛(backend) make start-recording atomic and fault-tolerant
|
||||||
|
|
||||||
## [1.15.0] - 2026-04-30
|
## [1.15.0] - 2026-04-30
|
||||||
|
|
||||||
|
|||||||
@@ -6,7 +6,9 @@ from logging import getLogger
|
|||||||
from urllib.parse import unquote, urlparse
|
from urllib.parse import unquote, urlparse
|
||||||
|
|
||||||
from django.conf import settings
|
from django.conf import settings
|
||||||
|
from django.core.exceptions import ValidationError as DjangoValidationError
|
||||||
from django.core.files.storage import default_storage
|
from django.core.files.storage import default_storage
|
||||||
|
from django.db import IntegrityError, transaction
|
||||||
from django.db.models import Q
|
from django.db.models import Q
|
||||||
from django.http import Http404
|
from django.http import Http404
|
||||||
from django.shortcuts import get_object_or_404
|
from django.shortcuts import get_object_or_404
|
||||||
@@ -320,16 +322,27 @@ class RoomViewSet(
|
|||||||
options = serializer.validated_data.get("options")
|
options = serializer.validated_data.get("options")
|
||||||
room = self.get_object()
|
room = self.get_object()
|
||||||
|
|
||||||
# May raise exception if an active or initiated recording already exist for the room
|
try:
|
||||||
recording = models.Recording.objects.create(
|
with transaction.atomic():
|
||||||
room=room,
|
recording = models.Recording.objects.create(
|
||||||
mode=mode,
|
room=room,
|
||||||
options=options.model_dump(exclude_none=True) if options else {},
|
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(
|
except (DjangoValidationError, IntegrityError):
|
||||||
user=self.request.user, role=models.RoleChoices.OWNER, recording=recording
|
# 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_service = get_worker_service(mode=recording.mode)
|
||||||
worker_manager = WorkerServiceMediator(worker_service=worker_service)
|
worker_manager = WorkerServiceMediator(worker_service=worker_service)
|
||||||
@@ -337,9 +350,12 @@ class RoomViewSet(
|
|||||||
try:
|
try:
|
||||||
worker_manager.start(recording)
|
worker_manager.start(recording)
|
||||||
except RecordingStartError:
|
except RecordingStartError:
|
||||||
|
models.Recording.objects.filter(pk=recording.pk).update(
|
||||||
|
status=models.RecordingStatusChoices.FAILED_TO_START
|
||||||
|
)
|
||||||
return drf_response.Response(
|
return drf_response.Response(
|
||||||
{"error": f"Recording failed to start for room {room.slug}"},
|
{"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 (
|
if settings.METADATA_COLLECTOR_ENABLED and (
|
||||||
|
|||||||
@@ -140,16 +140,18 @@ def test_start_recording_worker_error(
|
|||||||
|
|
||||||
mock_worker_service_factory.assert_called_once_with(mode="screen_recording")
|
mock_worker_service_factory.assert_called_once_with(mode="screen_recording")
|
||||||
|
|
||||||
assert response.status_code == 500
|
assert response.status_code == 502
|
||||||
assert response.json() == {
|
assert response.json() == {
|
||||||
"error": f"Recording failed to start for room {room.slug}"
|
"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
|
assert Recording.objects.count() == 1
|
||||||
recording = Recording.objects.first()
|
recording = Recording.objects.first()
|
||||||
assert recording.room == room
|
assert recording.room == room
|
||||||
assert recording.mode == "screen_recording"
|
assert recording.mode == "screen_recording"
|
||||||
|
assert recording.status == "failed_to_start"
|
||||||
|
|
||||||
# Verify recording access details
|
# Verify recording access details
|
||||||
assert recording.accesses.count() == 1
|
assert recording.accesses.count() == 1
|
||||||
@@ -158,6 +160,72 @@ def test_start_recording_worker_error(
|
|||||||
assert access.role == "owner"
|
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(
|
def test_start_recording_success(
|
||||||
mock_worker_service_factory, mock_worker_manager, settings
|
mock_worker_service_factory, mock_worker_manager, settings
|
||||||
):
|
):
|
||||||
|
|||||||
Reference in New Issue
Block a user