From 110ff4ee44b65149e60114bf79c53c4ab4c3f8b7 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 23 Oct 2024 12:26:04 -0300 Subject: [PATCH] Handle two exceptions that are causing patron_activity sync to fail --- .../manager/celery/tasks/patron_activity.py | 33 +++++++++++- .../celery/tasks/test_patron_activity.py | 51 ++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/celery/tasks/patron_activity.py b/src/palace/manager/celery/tasks/patron_activity.py index 4310b7ccb..824ed65ff 100644 --- a/src/palace/manager/celery/tasks/patron_activity.py +++ b/src/palace/manager/celery/tasks/patron_activity.py @@ -1,6 +1,7 @@ from celery import shared_task from palace.manager.api.circulation import PatronActivityCirculationAPI +from palace.manager.api.circulation_exceptions import PatronAuthorizationFailedException from palace.manager.celery.task import Task from palace.manager.service.celery.celery import QueueNames from palace.manager.service.integration_registry.license_providers import ( @@ -11,9 +12,11 @@ from palace.manager.sqlalchemy.model.collection import Collection from palace.manager.sqlalchemy.model.patron import Patron from palace.manager.sqlalchemy.util import get_one +from palace.manager.util.backoff import exponential_backoff +from palace.manager.util.http import RemoteIntegrationException -@shared_task(queue=QueueNames.high, bind=True) +@shared_task(queue=QueueNames.high, bind=True, max_retries=4) def sync_patron_activity( task: Task, collection_id: int, patron_id: int, pin: str | None, force: bool = False ) -> None: @@ -68,7 +71,33 @@ def sync_patron_activity( ) return - api.sync_patron_activity(patron, pin) + try: + api.sync_patron_activity(patron, pin) + except PatronAuthorizationFailedException: + patron_activity_status.fail() + task.log.exception( + "Patron activity sync task failed due to PatronAuthorizationFailedException. " + "Marking patron activity as failed." + ) + return + except RemoteIntegrationException as e: + # This may have been a transient network error with the remote integration. Attempt to retry. + retries = task.request.retries + if retries < task.max_retries: + wait_time = exponential_backoff(retries) + patron_activity_status.clear() + task.log.exception( + f"Patron activity sync task failed ({e}). Retrying in {wait_time} seconds." + ) + raise task.retry(countdown=wait_time) + + # We've reached the max number of retries. Mark the status as failed, but don't fail + # the task itself, since this is likely an error that is outside our control. + patron_activity_status.fail() + task.log.exception( + f"Patron activity sync task failed ({e}). Max retries exceeded." + ) + return task.log.info( f"Patron activity sync for patron '{patron.authorization_identifier}' (id: {patron_id}) " diff --git a/tests/manager/celery/tasks/test_patron_activity.py b/tests/manager/celery/tasks/test_patron_activity.py index bc169d00d..c9a7fa1b6 100644 --- a/tests/manager/celery/tasks/test_patron_activity.py +++ b/tests/manager/celery/tasks/test_patron_activity.py @@ -3,6 +3,7 @@ import pytest from palace.manager.api.circulation import HoldInfo, LoanInfo +from palace.manager.api.circulation_exceptions import PatronAuthorizationFailedException from palace.manager.celery.task import Task from palace.manager.celery.tasks.patron_activity import sync_patron_activity from palace.manager.service.integration_registry.license_providers import ( @@ -13,6 +14,7 @@ PatronActivity, PatronActivityStatus, ) +from palace.manager.util.http import RemoteIntegrationException from tests.fixtures.celery import CeleryFixture from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.redis import RedisFixture @@ -127,7 +129,54 @@ def test_collection_not_found( assert task_status.state == PatronActivityStatus.State.FAILED assert task_status.task_id == task.id - def test_exception( + def test_patron_authorization_failed_exception( + self, sync_task_fixture: SyncTaskFixture, caplog: pytest.LogCaptureFixture + ): + sync_task_fixture.mock_collection_api.patron_activity = MagicMock( + side_effect=PatronAuthorizationFailedException() + ) + + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + task_status = sync_task_fixture.redis_record.status() + assert task_status is not None + assert task_status.state == PatronActivityStatus.State.FAILED + + assert ( + "Patron activity sync task failed due to PatronAuthorizationFailedException" + in caplog.text + ) + + @patch("palace.manager.celery.tasks.patron_activity.exponential_backoff") + def test_remote_integration_exception( + self, + mock_backoff: MagicMock, + sync_task_fixture: SyncTaskFixture, + caplog: pytest.LogCaptureFixture, + ): + # Make sure our backoff function doesn't delay the test. + mock_backoff.return_value = 0 + + mock_activity = create_autospec( + sync_task_fixture.mock_collection_api.patron_activity, + side_effect=RemoteIntegrationException("http://test.com", "boom!"), + ) + sync_task_fixture.mock_collection_api.patron_activity = mock_activity + + sync_patron_activity.apply_async( + (sync_task_fixture.collection.id, sync_task_fixture.patron.id, "pin") + ).wait() + + task_status = sync_task_fixture.redis_record.status() + assert task_status is not None + assert task_status.state == PatronActivityStatus.State.FAILED + + assert mock_activity.call_count == 5 + assert "Max retries exceeded" in caplog.text + + def test_other_exception( self, sync_task_fixture: SyncTaskFixture, caplog: pytest.LogCaptureFixture ): sync_task_fixture.mock_registry.from_collection.side_effect = Exception("Boom!")