From f647d8f25de00e7500dd8146b3962a15e6063f5f Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 24 Jul 2026 18:38:38 +0500 Subject: [PATCH 1/4] feat: raise descriptive error when proctoring provider cannot remove an attempt --- edx_proctoring/backends/rest.py | 5 +++- edx_proctoring/backends/tests/test_rest.py | 22 ++++++++++++++++ edx_proctoring/exceptions.py | 9 +++++++ edx_proctoring/handlers.py | 29 +++++++++++++++++++++- edx_proctoring/tests/test_handlers.py | 28 +++++++++++++++++++++ 5 files changed, 91 insertions(+), 2 deletions(-) diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index f10e756ab6a..205921e88ff 100644 --- a/edx_proctoring/backends/rest.py +++ b/edx_proctoring/backends/rest.py @@ -41,6 +41,9 @@ class BaseRestProctoringProvider(ProctoringBackendProvider): has_dashboard = True supports_onboarding = True passing_statuses = (SoftwareSecureReviewStatus.clean,) + # Timeout (in seconds) applied to every outbound request to the provider so + # that a slow or unavailable provider cannot hang the request indefinitely. + timeout = 30 @property def exam_attempt_url(self): @@ -384,7 +387,7 @@ def _make_attempt_request(self, exam, attempt, method='POST', status=None, **pay if method == 'GET': headers.update(self._get_language_headers()) log.debug('Making %r attempt request at %r', method, url) - response = self.session.request(method, url, json=payload, headers=headers) + response = self.session.request(method, url, json=payload, headers=headers, timeout=self.timeout) try: data = response.json() except ValueError: diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index f328beb71ef..445561a9bdf 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/edx_proctoring/backends/tests/test_rest.py @@ -277,6 +277,28 @@ def test_remove_attempt_no_attempt(self): status = self.provider.remove_exam_attempt(self.backend_exam['external_id'], None) self.assertFalse(status) + def test_make_attempt_request_passes_timeout(self): + """ + Every outbound attempt request must include an explicit timeout so that a + slow/unavailable provider cannot hang the request indefinitely. + """ + attempt_id = 2 + with patch.object(self.provider.session, 'request') as request_mock: + request_mock.return_value.json.return_value = {'status': 'deleted'} + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + _, kwargs = request_mock.call_args + self.assertEqual(kwargs['timeout'], self.provider.timeout) + + def test_remove_attempt_provider_unavailable(self): + """ + A provider connection error must propagate out of the backend rather than + being swallowed, so the caller can surface a descriptive error. + """ + attempt_id = 2 + with patch.object(self.provider.session, 'request', side_effect=ConnectionError('boom')): + with self.assertRaises(ConnectionError): + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + def test_on_review_callback(self): """ on_review_callback should just return the payload diff --git a/edx_proctoring/exceptions.py b/edx_proctoring/exceptions.py index 26581c65ac5..feec824c167 100644 --- a/edx_proctoring/exceptions.py +++ b/edx_proctoring/exceptions.py @@ -95,6 +95,15 @@ class BackendProviderSentNoAttemptID(BackendProviderCannotRegisterAttempt): """ +class BackendProviderCannotRemoveAttempt(ProctoredBaseException): + """ + Raised when a back-end provider cannot remove an attempt, e.g. because the + provider is slow, unreachable, or returned an error while trying to delete + the attempt on their side. + """ + http_status = status.HTTP_502_BAD_GATEWAY + + class BackendProviderOnboardingException(ProctoredBaseException): """ Raised when a back-end provider cannot register an attempt diff --git a/edx_proctoring/handlers.py b/edx_proctoring/handlers.py index 44fecc7c9af..787f412ea36 100644 --- a/edx_proctoring/handlers.py +++ b/edx_proctoring/handlers.py @@ -7,6 +7,7 @@ from edx_proctoring import api, constants, models from edx_proctoring.backends import get_backend_provider +from edx_proctoring.exceptions import BackendProviderCannotRemoveAttempt from edx_proctoring.runtime import get_runtime_service from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus from edx_proctoring.utils import emit_event, locate_attempt_by_attempt_code @@ -138,7 +139,33 @@ def on_attempt_changed(sender, instance, signal, **kwargs): if instance.proctored_exam.is_proctored: backend = get_backend_provider(name=instance.proctored_exam.backend) if backend: - result = backend.remove_exam_attempt(instance.proctored_exam.external_id, instance.external_id) + try: + result = backend.remove_exam_attempt( + instance.proctored_exam.external_id, instance.external_id + ) + except Exception as exc: # pylint: disable=broad-exception-caught + # A slow or unavailable provider must not surface as an unhandled 500, + # and must not silently leave the attempt half-removed. Log with enough + # context for monitoring/Sentry and re-raise a typed exception so the + # instructor-facing MFE receives a descriptive ``detail`` message. + log.exception( + 'Failed to remove attempt_id=%(attempt_id)s (external_id=%(external_id)s) for ' + 'exam_id=%(exam_id)s user_id=%(user_id)s from backend=%(backend)s due to a ' + 'provider error.', + { + 'attempt_id': instance.id, + 'external_id': instance.external_id, + 'exam_id': instance.proctored_exam.id, + 'user_id': instance.user_id, + 'backend': instance.proctored_exam.backend, + } + ) + raise BackendProviderCannotRemoveAttempt( + # Translators: shown to an instructor when an exam attempt could not be reset + # because the external proctoring provider is temporarily unavailable. + 'The proctoring provider is temporarily unavailable, so this attempt could not ' + 'be fully reset. Please try again in a few minutes.' + ) from exc if not result: log.error( 'Failed to remove attempt_id=%s from backend=%s', diff --git a/edx_proctoring/tests/test_handlers.py b/edx_proctoring/tests/test_handlers.py index 1dcd4e6b2c8..7479d2a04c2 100644 --- a/edx_proctoring/tests/test_handlers.py +++ b/edx_proctoring/tests/test_handlers.py @@ -6,9 +6,11 @@ import ddt from httmock import HTTMock +from django.db import transaction from django.db.models.signals import pre_delete, pre_save from edx_proctoring.api import update_attempt_status +from edx_proctoring.exceptions import BackendProviderCannotRemoveAttempt from edx_proctoring.models import ProctoredExam, ProctoredExamStudentAttempt from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus from edx_proctoring.tests.test_services import MockInstructorService @@ -47,6 +49,32 @@ def test_backend_fails_to_delete_attempt(self, logger_mock): log_format_string = 'Failed to remove attempt_id=%s from backend=%s' logger_mock.assert_any_call(log_format_string, 1, self.backend_name) + @patch('logging.Logger.exception') + @patch('edx_proctoring.handlers.get_backend_provider') + def test_provider_unavailable_raises_and_keeps_attempt(self, get_backend_mock, logger_mock): + """ + If the provider raises (slow/unavailable) while removing the attempt, a typed + BackendProviderCannotRemoveAttempt must be raised, the failure logged with + context, and the local attempt must NOT be deleted. + """ + attempt_id = self.attempt.id + backend = get_backend_mock.return_value + backend.remove_exam_attempt.side_effect = ConnectionError('provider down') + + # ``delete()`` opens an inner atomic(savepoint=False); when the pre_delete + # handler raises, that marks the connection as needing rollback. Wrapping the + # call in an explicit atomic block creates a savepoint that absorbs the + # rollback, so the connection is usable again for the assertion below. + with self.assertRaises(BackendProviderCannotRemoveAttempt) as context: + with transaction.atomic(): + self.attempt.delete_exam_attempt() + + self.assertIn('temporarily unavailable', str(context.exception)) + self.assertEqual(context.exception.http_status, 502) + logger_mock.assert_called_once() + # the attempt must still exist because the removal failed + self.assertTrue(ProctoredExamStudentAttempt.objects.filter(id=attempt_id).exists()) + @ddt.data(None, MockInstructorService()) @patch('edx_proctoring.handlers.get_runtime_service') @patch('edx_proctoring.tests.test_services.MockInstructorService.complete_student_attempt') From bbc6d676bea6532102a5b152513d6af13ffc9470 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 24 Jul 2026 18:53:14 +0500 Subject: [PATCH 2/4] chore: premerge actions --- CHANGELOG.rst | 6 ++++++ edx_proctoring/__init__.py | 2 +- package.json | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ffe09487c6c..74842889054 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,12 @@ Change Log Unreleased ~~~~~~~~~~ +[6.1.0] - 2026-07-24 +* Add an explicit request timeout to the REST proctoring backend, and log and raise a + typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) when the provider fails to + remove an attempt, so callers can return a descriptive error to the instructor instead + of an unhandled 500. + [5.2.1] - 2025-12-05 * Remove all references to Proctortrack diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py index be497164f09..7b715fdba21 100644 --- a/edx_proctoring/__init__.py +++ b/edx_proctoring/__init__.py @@ -3,4 +3,4 @@ """ # Be sure to update the version number in edx_proctoring/package.json -__version__ = '6.0.0' +__version__ = '6.1.0' diff --git a/package.json b/package.json index c6cc63a68b8..d8f93290427 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@edx/edx-proctoring", "//": "Note that the version format is slightly different than that of the Python version when using prereleases.", - "version": "6.0.0", + "version": "6.1.0", "main": "edx_proctoring/static/index.js", "scripts": { "test": "gulp test" From 0421637b47a424cccc86effe3a37415e48b3ca63 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Mon, 27 Jul 2026 18:51:03 +0500 Subject: [PATCH 3/4] fix: issues --- CHANGELOG.rst | 10 ++++++---- edx_proctoring/apps.py | 1 + edx_proctoring/backends/rest.py | 2 ++ edx_proctoring/backends/tests/test_rest.py | 15 +++++++++++++++ 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 74842889054..eab27a314d6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,10 +14,12 @@ Change Log Unreleased ~~~~~~~~~~ [6.1.0] - 2026-07-24 -* Add an explicit request timeout to the REST proctoring backend, and log and raise a - typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) when the provider fails to - remove an attempt, so callers can return a descriptive error to the instructor instead - of an unhandled 500. + +* Add an explicit request timeout to the REST proctoring backend (default 30s, + overridable per backend via a ``timeout`` key in ``PROCTORING_BACKENDS``), and log and + raise a typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) when the provider fails + to remove an attempt, so callers can return a descriptive error to the instructor + instead of an unhandled 500. [5.2.1] - 2025-12-05 * Remove all references to Proctortrack diff --git a/edx_proctoring/apps.py b/edx_proctoring/apps.py index eba34af3077..32ebc8747da 100644 --- a/edx_proctoring/apps.py +++ b/edx_proctoring/apps.py @@ -37,6 +37,7 @@ 'supports_onboarding', 'tech_support_email', 'tech_support_phone', + 'timeout', 'token_expiration_time', 'verbose_name', 'video_review_aes_key', diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index 205921e88ff..8fe1870d6e5 100644 --- a/edx_proctoring/backends/rest.py +++ b/edx_proctoring/backends/rest.py @@ -43,6 +43,8 @@ class BaseRestProctoringProvider(ProctoringBackendProvider): passing_statuses = (SoftwareSecureReviewStatus.clean,) # Timeout (in seconds) applied to every outbound request to the provider so # that a slow or unavailable provider cannot hang the request indefinitely. + # Operators can override this per backend via a ``timeout`` key in the + # backend's ``PROCTORING_BACKENDS`` configuration. timeout = 30 @property diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index 445561a9bdf..342531a5c3b 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/edx_proctoring/backends/tests/test_rest.py @@ -12,6 +12,7 @@ from django.test import TestCase, override_settings from django.utils import translation +from edx_proctoring.apps import BACKEND_CONFIGURATION_ALLOW_LIST from edx_proctoring.backends.rest import BaseRestProctoringProvider from edx_proctoring.exceptions import ( BackendProviderCannotRegisterAttempt, @@ -289,6 +290,20 @@ def test_make_attempt_request_passes_timeout(self): _, kwargs = request_mock.call_args self.assertEqual(kwargs['timeout'], self.provider.timeout) + def test_default_timeout(self): + """The backend applies a sane default request timeout when none is configured.""" + self.assertEqual(self.provider.timeout, 30) + + def test_timeout_is_configurable(self): + """ + Operators can override the request timeout per backend. The value arrives as a + constructor kwarg sourced from the backend's PROCTORING_BACKENDS configuration, + so the key must also be present in the backend configuration allow list. + """ + provider = BaseRestProctoringProvider('client_id', 'client_secret', timeout=60) + self.assertEqual(provider.timeout, 60) + self.assertIn('timeout', BACKEND_CONFIGURATION_ALLOW_LIST) + def test_remove_attempt_provider_unavailable(self): """ A provider connection error must propagate out of the backend rather than From 573bec1b893bc2a14b04a5a4ad686383eebc81c6 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Tue, 28 Jul 2026 20:43:01 +0500 Subject: [PATCH 4/4] feat: surface proctoring provider errors on attempt reset (configurable per backend) --- CHANGELOG.rst | 21 +++++--- edx_proctoring/apps.py | 1 + edx_proctoring/backends/rest.py | 37 +++++++++++-- edx_proctoring/backends/tests/test_rest.py | 63 ++++++++++++++++++++++ edx_proctoring/handlers.py | 37 +++++++------ edx_proctoring/tests/test_handlers.py | 22 ++++++++ 6 files changed, 155 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index eab27a314d6..ef331f43887 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,13 +13,20 @@ Change Log Unreleased ~~~~~~~~~~ -[6.1.0] - 2026-07-24 - -* Add an explicit request timeout to the REST proctoring backend (default 30s, - overridable per backend via a ``timeout`` key in ``PROCTORING_BACKENDS``), and log and - raise a typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) when the provider fails - to remove an attempt, so callers can return a descriptive error to the instructor - instead of an unhandled 500. +[6.1.0] - 2026-07-28 + +* Make proctored exam attempt removal fail loudly when the proctoring provider errors, + instead of a silent local-only reset or an unhandled 500: + + * Add an explicit request timeout to the REST proctoring backend (default 30s, + overridable per backend via a ``timeout`` key in ``PROCTORING_BACKENDS``). + * Raise a typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) both when the + provider is unreachable/times out and when it responds with an HTTP error status, + returning a clear message (including the provider's HTTP status) and logging the raw + provider response for debugging. Callers can then return a descriptive error to the + instructor instead of a silent local-only reset or an unhandled 500. Raising on a + provider HTTP error is on by default and overridable per backend via a + ``raise_on_remove_error`` key in ``PROCTORING_BACKENDS``. [5.2.1] - 2025-12-05 * Remove all references to Proctortrack diff --git a/edx_proctoring/apps.py b/edx_proctoring/apps.py index 32ebc8747da..145d66e6851 100644 --- a/edx_proctoring/apps.py +++ b/edx_proctoring/apps.py @@ -30,6 +30,7 @@ 'organization', 'passing_statuses', 'ping_interval', + 'raise_on_remove_error', 'secret_key', 'secret_key_id', 'send_email', diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index 8fe1870d6e5..b3886c1321b 100644 --- a/edx_proctoring/backends/rest.py +++ b/edx_proctoring/backends/rest.py @@ -19,6 +19,7 @@ from edx_proctoring.backends.backend import ProctoringBackendProvider from edx_proctoring.exceptions import ( BackendProviderCannotRegisterAttempt, + BackendProviderCannotRemoveAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, BackendProviderOnboardingProfilesException, @@ -46,6 +47,10 @@ class BaseRestProctoringProvider(ProctoringBackendProvider): # Operators can override this per backend via a ``timeout`` key in the # backend's ``PROCTORING_BACKENDS`` configuration. timeout = 30 + # Whether a provider HTTP error while removing an attempt should raise a descriptive + # error (so it is surfaced to the instructor) rather than being logged and ignored. + # Overridable per backend via a ``raise_on_remove_error`` key in ``PROCTORING_BACKENDS``. + raise_on_remove_error = True @property def exam_attempt_url(self): @@ -232,12 +237,18 @@ def stop_exam_attempt(self, exam, attempt): def remove_exam_attempt(self, exam, attempt): """ - Removes the exam attempt on the backend provider's server + Removes the exam attempt on the backend provider's server. + + Raises BackendProviderCannotRemoveAttempt if the provider responds with an + HTTP error status (unless ``raise_on_remove_error`` is disabled for this backend), + so the caller can surface a descriptive error instead of silently leaving the + attempt on the provider's side. """ response = self._make_attempt_request( exam, attempt, - method='DELETE') + method='DELETE', + raise_on_error=self.raise_on_remove_error) return response.get('status', None) == 'deleted' def mark_erroneous_exam_attempt(self, exam, attempt): @@ -374,9 +385,14 @@ def _get_language_headers(self): lang_header = f'{current_lang};{default_lang}' return {'Accept-Language': lang_header} - def _make_attempt_request(self, exam, attempt, method='POST', status=None, **payload): + def _make_attempt_request(self, exam, attempt, method='POST', status=None, raise_on_error=False, **payload): """ - Calls backend attempt API + Calls backend attempt API. + + When ``raise_on_error`` is True, a provider HTTP error response (status >= 400) + raises BackendProviderCannotRemoveAttempt, surfacing the provider's own message + when it supplies one and a generic message otherwise. This is distinct from the + provider being unreachable/timing out, where ``session.request`` itself raises. """ if not attempt: return {} @@ -395,4 +411,17 @@ def _make_attempt_request(self, exam, attempt, method='POST', status=None, **pay except ValueError: log.exception("Decoding attempt %r -> %r", attempt, response.content) data = {} + if raise_on_error and not response.ok: + # Log the raw provider response (status + body) for support/debugging; the + # provider's error schema is not standardised, so we do not try to parse a + # message out of it and instead surface a clean, provider-status-aware error. + log.error( + 'Proctoring provider returned HTTP %s while removing attempt %r: %r', + response.status_code, attempt, response.content, + ) + raise BackendProviderCannotRemoveAttempt( + 'The proctoring provider could not remove this attempt ' + f'(provider returned HTTP {response.status_code}). ' + 'Please try again or contact support.' + ) return data diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index 342531a5c3b..1dc1091efee 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/edx_proctoring/backends/tests/test_rest.py @@ -16,6 +16,7 @@ from edx_proctoring.backends.rest import BaseRestProctoringProvider from edx_proctoring.exceptions import ( BackendProviderCannotRegisterAttempt, + BackendProviderCannotRemoveAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, BackendProviderOnboardingProfilesException, @@ -314,6 +315,68 @@ def test_remove_attempt_provider_unavailable(self): with self.assertRaises(ConnectionError): self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + @responses.activate + def test_remove_attempt_provider_error_raises_with_status(self): + """ + A provider HTTP error response (>= 400) raises BackendProviderCannotRemoveAttempt + with a clean, provider-status-aware message and an instructor-facing 502. + """ + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + json={'detail': 'Attempt is locked'}, + status=400, + ) + with self.assertRaises(BackendProviderCannotRemoveAttempt) as ctx: + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertIn('HTTP 400', str(ctx.exception)) + self.assertIn('could not remove this attempt', str(ctx.exception)) + self.assertEqual(ctx.exception.http_status, 502) + + @responses.activate + def test_remove_attempt_provider_error_no_body(self): + """ + A provider HTTP error with no response body still raises with the clean + provider-status-aware message. + """ + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + status=502, + ) + with self.assertRaises(BackendProviderCannotRemoveAttempt) as ctx: + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertIn('HTTP 502', str(ctx.exception)) + self.assertEqual(ctx.exception.http_status, 502) + + def test_raise_on_remove_error_default_and_configurable(self): + """ + The raise-on-remove-error behaviour defaults to True and is overridable per + backend via PROCTORING_BACKENDS, so the key must be in the allow list. + """ + self.assertTrue(self.provider.raise_on_remove_error) + self.assertIn('raise_on_remove_error', BACKEND_CONFIGURATION_ALLOW_LIST) + provider = BaseRestProctoringProvider('client_id', 'client_secret', raise_on_remove_error=False) + self.assertFalse(provider.raise_on_remove_error) + + @responses.activate + def test_remove_attempt_provider_error_not_raised_when_disabled(self): + """ + When raise_on_remove_error is disabled for a backend, a provider HTTP error is + not raised; removal returns False (the pre-existing log-and-continue behaviour). + """ + self.provider.raise_on_remove_error = False + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + status=502, + ) + status = self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertFalse(status) + def test_on_review_callback(self): """ on_review_callback should just return the payload diff --git a/edx_proctoring/handlers.py b/edx_proctoring/handlers.py index 787f412ea36..ff13631452d 100644 --- a/edx_proctoring/handlers.py +++ b/edx_proctoring/handlers.py @@ -139,27 +139,34 @@ def on_attempt_changed(sender, instance, signal, **kwargs): if instance.proctored_exam.is_proctored: backend = get_backend_provider(name=instance.proctored_exam.backend) if backend: + log_context = { + 'attempt_id': instance.id, + 'external_id': instance.external_id, + 'exam_id': instance.proctored_exam.id, + 'user_id': instance.user_id, + 'backend': instance.proctored_exam.backend, + } + log_message = ( + 'Failed to remove attempt_id=%(attempt_id)s (external_id=%(external_id)s) for ' + 'exam_id=%(exam_id)s user_id=%(user_id)s from backend=%(backend)s due to a ' + 'provider error.' + ) try: result = backend.remove_exam_attempt( instance.proctored_exam.external_id, instance.external_id ) + except BackendProviderCannotRemoveAttempt: + # The provider responded with an HTTP error and the backend already built a + # descriptive (possibly provider-supplied) message. Log and re-raise it as-is + # so that message reaches the instructor-facing MFE unchanged. + log.exception(log_message, log_context) + raise except Exception as exc: # pylint: disable=broad-exception-caught - # A slow or unavailable provider must not surface as an unhandled 500, - # and must not silently leave the attempt half-removed. Log with enough - # context for monitoring/Sentry and re-raise a typed exception so the + # The provider is slow/unavailable (connection error or timeout). This must not + # surface as an unhandled 500 or silently leave the attempt half-removed. Log + # with enough context for monitoring/Sentry and re-raise a typed exception so the # instructor-facing MFE receives a descriptive ``detail`` message. - log.exception( - 'Failed to remove attempt_id=%(attempt_id)s (external_id=%(external_id)s) for ' - 'exam_id=%(exam_id)s user_id=%(user_id)s from backend=%(backend)s due to a ' - 'provider error.', - { - 'attempt_id': instance.id, - 'external_id': instance.external_id, - 'exam_id': instance.proctored_exam.id, - 'user_id': instance.user_id, - 'backend': instance.proctored_exam.backend, - } - ) + log.exception(log_message, log_context) raise BackendProviderCannotRemoveAttempt( # Translators: shown to an instructor when an exam attempt could not be reset # because the external proctoring provider is temporarily unavailable. diff --git a/edx_proctoring/tests/test_handlers.py b/edx_proctoring/tests/test_handlers.py index 7479d2a04c2..43c0a0b0c32 100644 --- a/edx_proctoring/tests/test_handlers.py +++ b/edx_proctoring/tests/test_handlers.py @@ -75,6 +75,28 @@ def test_provider_unavailable_raises_and_keeps_attempt(self, get_backend_mock, l # the attempt must still exist because the removal failed self.assertTrue(ProctoredExamStudentAttempt.objects.filter(id=attempt_id).exists()) + @patch('logging.Logger.exception') + @patch('edx_proctoring.handlers.get_backend_provider') + def test_provider_error_response_preserves_message_and_keeps_attempt(self, get_backend_mock, logger_mock): + """ + If the backend raises BackendProviderCannotRemoveAttempt (the provider responded + with an HTTP error), the handler re-raises it unchanged so the provider-supplied + message reaches the caller, and the local attempt must NOT be deleted. + """ + attempt_id = self.attempt.id + backend = get_backend_mock.return_value + backend.remove_exam_attempt.side_effect = BackendProviderCannotRemoveAttempt('Attempt is locked on provider') + + with self.assertRaises(BackendProviderCannotRemoveAttempt) as context: + with transaction.atomic(): + self.attempt.delete_exam_attempt() + + # the provider's message is preserved (not overwritten by the generic one) + self.assertIn('Attempt is locked on provider', str(context.exception)) + self.assertEqual(context.exception.http_status, 502) + logger_mock.assert_called_once() + self.assertTrue(ProctoredExamStudentAttempt.objects.filter(id=attempt_id).exists()) + @ddt.data(None, MockInstructorService()) @patch('edx_proctoring.handlers.get_runtime_service') @patch('edx_proctoring.tests.test_services.MockInstructorService.complete_student_attempt')