From 39aa2d63663104c73a8bfba63b2bdb4a2932747e Mon Sep 17 00:00:00 2001 From: Sahil Date: Thu, 19 Mar 2026 19:38:01 +0530 Subject: [PATCH 1/2] fix(api): delegate concurrent deletion deadlocks to TaskBackoffError --- api/environments/tasks.py | 17 ++++++- api/features/tasks.py | 14 +++++- .../test_unit_environments_tasks.py | 45 ++++++++++++++++++- .../unit/features/test_unit_features_tasks.py | 35 ++++++++++++++- 4 files changed, 106 insertions(+), 5 deletions(-) diff --git a/api/environments/tasks.py b/api/environments/tasks.py index 09a8becb3b30..08ea3d8226ba 100644 --- a/api/environments/tasks.py +++ b/api/environments/tasks.py @@ -1,3 +1,8 @@ +from django.db import IntegrityError, OperationalError, transaction +from django.db.transaction import TransactionManagementError +from task_processor.exceptions import TaskBackoffError +from django.db import transaction +from django.core.exceptions import ObjectDoesNotExist from django.db.models import Prefetch, Q from django.utils import timezone from task_processor.decorators import ( @@ -59,8 +64,16 @@ def delete_environment_from_dynamo(api_key: str, environment_id: str): # type: @register_task_handler() def delete_environment(environment_id: int) -> None: - Environment.objects.get(id=environment_id).delete() - + try: + # Wrap in atomic to safely catch DB-level errors + with transaction.atomic(): + Environment.objects.get(id=environment_id).delete() + except Environment.DoesNotExist: + # If it's already gone, our job is done! + pass + except (OperationalError, IntegrityError, TransactionManagementError): + # Someone else is locking these rows, back off and retry + raise TaskBackoffError() @register_task_handler() def clone_environment_feature_states( diff --git a/api/features/tasks.py b/api/features/tasks.py index 7ae434d9b60d..438f898916e7 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -1,3 +1,7 @@ +from django.db import IntegrityError, OperationalError, transaction +from django.db.transaction import TransactionManagementError +from task_processor.exceptions import TaskBackoffError + import logging from typing import Any @@ -159,4 +163,12 @@ def _get_previous_multivariate_values( @register_task_handler() def delete_feature(feature_id: int) -> None: - Feature.objects.get(pk=feature_id).delete() + try: + with transaction.atomic(): + Feature.objects.get(pk=feature_id).delete() + + except Feature.DoesNotExist: + pass + + except (OperationalError, IntegrityError, TransactionManagementError): + raise TaskBackoffError() diff --git a/api/tests/unit/environments/test_unit_environments_tasks.py b/api/tests/unit/environments/test_unit_environments_tasks.py index fa9f6adfd3ba..a888c38ef28d 100644 --- a/api/tests/unit/environments/test_unit_environments_tasks.py +++ b/api/tests/unit/environments/test_unit_environments_tasks.py @@ -1,5 +1,9 @@ from pytest_mock import MockerFixture - +import pytest +from django.db import OperationalError +from task_processor.exceptions import TaskBackoffError +from environments.models import Environment +from environments.tasks import delete_environment from audit.models import AuditLog from environments.models import Environment from environments.tasks import ( @@ -113,3 +117,42 @@ def test_delete_environment__calls_internal_methods_correctly( mocked_identity_wrapper.delete_all_identities.assert_called_once_with( environment_api_key ) +def test_delete_environment__environment_does_not_exist__succeeds_silently( + mocker: MockerFixture, +) -> None: + # Given + mock_get_environment = mocker.patch("environments.tasks.Environment.objects.get") + mock_get_environment.side_effect = Environment.DoesNotExist + + # When + delete_environment(environment_id=1) + + # Then + # No exception is raised, confirming silent success + mock_get_environment.assert_called_once_with(id=1) + + +def test_delete_environment__database_deadlock__raises_task_backoff_error( + mocker: MockerFixture, +) -> None: + # Given + mock_get_environment = mocker.patch("environments.tasks.Environment.objects.get") + mock_get_environment.side_effect = OperationalError + + # When + with pytest.raises(TaskBackoffError): + delete_environment(environment_id=1) + + # Then + # TaskBackoffError is raised to trigger the task-processor retry + mock_get_environment.assert_called_once_with(id=1) # Given + mock_get_environment = mocker.patch("environments.tasks.Environment.objects.get") + mock_get_environment.side_effect = OperationalError + + # When + with pytest.raises(TaskBackoffError): + delete_environment(environment_id=1) + + # Then + # TaskBackoffError is raised to trigger the task-processor retry + mock_get_environment.assert_called_once_with(id=1) diff --git a/api/tests/unit/features/test_unit_features_tasks.py b/api/tests/unit/features/test_unit_features_tasks.py index b5bfa79c2863..ec32e5dbba60 100644 --- a/api/tests/unit/features/test_unit_features_tasks.py +++ b/api/tests/unit/features/test_unit_features_tasks.py @@ -1,7 +1,10 @@ import pytest from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from pytest_mock import MockerFixture - +from unittest import mock +from django.db import OperationalError +from task_processor.exceptions import TaskBackoffError +from features.tasks import delete_feature from api_keys.models import MasterAPIKey from environments.models import Environment from features.models import Feature, FeatureState @@ -162,3 +165,33 @@ def test_trigger_feature_state_change_webhooks_for_deleted_flag_uses_fs_instance assert data["previous_state"]["feature"]["id"] == feature_state.feature.id assert event_type == WebhookEventType.FLAG_DELETED.value + +@pytest.mark.django_db +def test_delete_feature__feature_does_not_exist__succeeds_silently( + mocker: MockerFixture, +) -> None: + # Given + mock_get_feature = mocker.patch("features.tasks.Feature.objects.get") + mock_get_feature.side_effect = Feature.DoesNotExist + + # When + delete_feature(feature_id=1) + + # Then + mock_get_feature.assert_called_once_with(pk=1) + + +@pytest.mark.django_db +def test_delete_feature__database_deadlock__raises_task_backoff_error( + mocker: MockerFixture, +) -> None: + # Given + mock_get_feature = mocker.patch("features.tasks.Feature.objects.get") + mock_get_feature.side_effect = OperationalError + + # When + with pytest.raises(TaskBackoffError): + delete_feature(feature_id=1) + + # Then + mock_get_feature.assert_called_once_with(pk=1) From ceac7e2eab3c899a36254970eeff7964ec5f8d4d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 23 Mar 2026 06:41:15 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/environments/tasks.py | 7 +++---- api/features/tasks.py | 11 +++++------ .../unit/environments/test_unit_environments_tasks.py | 10 ++++++---- api/tests/unit/features/test_unit_features_tasks.py | 8 ++++---- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/api/environments/tasks.py b/api/environments/tasks.py index 08ea3d8226ba..802c5d697f2f 100644 --- a/api/environments/tasks.py +++ b/api/environments/tasks.py @@ -1,13 +1,11 @@ from django.db import IntegrityError, OperationalError, transaction -from django.db.transaction import TransactionManagementError -from task_processor.exceptions import TaskBackoffError -from django.db import transaction -from django.core.exceptions import ObjectDoesNotExist from django.db.models import Prefetch, Q +from django.db.transaction import TransactionManagementError from django.utils import timezone from task_processor.decorators import ( register_task_handler, ) +from task_processor.exceptions import TaskBackoffError from task_processor.models import TaskPriority from audit.models import AuditLog @@ -75,6 +73,7 @@ def delete_environment(environment_id: int) -> None: # Someone else is locking these rows, back off and retry raise TaskBackoffError() + @register_task_handler() def clone_environment_feature_states( source_environment_id: int, clone_environment_id: int diff --git a/api/features/tasks.py b/api/features/tasks.py index 438f898916e7..330efc588973 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -1,13 +1,12 @@ -from django.db import IntegrityError, OperationalError, transaction -from django.db.transaction import TransactionManagementError -from task_processor.exceptions import TaskBackoffError - import logging from typing import Any +from django.db import IntegrityError, OperationalError, transaction +from django.db.transaction import TransactionManagementError from task_processor.decorators import ( register_task_handler, ) +from task_processor.exceptions import TaskBackoffError from environments.models import Webhook from features.models import Feature, FeatureState @@ -166,9 +165,9 @@ def delete_feature(feature_id: int) -> None: try: with transaction.atomic(): Feature.objects.get(pk=feature_id).delete() - + except Feature.DoesNotExist: pass - + except (OperationalError, IntegrityError, TransactionManagementError): raise TaskBackoffError() diff --git a/api/tests/unit/environments/test_unit_environments_tasks.py b/api/tests/unit/environments/test_unit_environments_tasks.py index a888c38ef28d..835f6e07ee04 100644 --- a/api/tests/unit/environments/test_unit_environments_tasks.py +++ b/api/tests/unit/environments/test_unit_environments_tasks.py @@ -1,12 +1,12 @@ -from pytest_mock import MockerFixture import pytest from django.db import OperationalError +from pytest_mock import MockerFixture from task_processor.exceptions import TaskBackoffError -from environments.models import Environment -from environments.tasks import delete_environment + from audit.models import AuditLog from environments.models import Environment from environments.tasks import ( + delete_environment, delete_environment_from_dynamo, process_environment_update, rebuild_environment_document, @@ -117,6 +117,8 @@ def test_delete_environment__calls_internal_methods_correctly( mocked_identity_wrapper.delete_all_identities.assert_called_once_with( environment_api_key ) + + def test_delete_environment__environment_does_not_exist__succeeds_silently( mocker: MockerFixture, ) -> None: @@ -145,7 +147,7 @@ def test_delete_environment__database_deadlock__raises_task_backoff_error( # Then # TaskBackoffError is raised to trigger the task-processor retry - mock_get_environment.assert_called_once_with(id=1) # Given + mock_get_environment.assert_called_once_with(id=1) # Given mock_get_environment = mocker.patch("environments.tasks.Environment.objects.get") mock_get_environment.side_effect = OperationalError diff --git a/api/tests/unit/features/test_unit_features_tasks.py b/api/tests/unit/features/test_unit_features_tasks.py index ec32e5dbba60..741d30227592 100644 --- a/api/tests/unit/features/test_unit_features_tasks.py +++ b/api/tests/unit/features/test_unit_features_tasks.py @@ -1,14 +1,13 @@ import pytest +from django.db import OperationalError from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from pytest_mock import MockerFixture -from unittest import mock -from django.db import OperationalError from task_processor.exceptions import TaskBackoffError -from features.tasks import delete_feature + from api_keys.models import MasterAPIKey from environments.models import Environment from features.models import Feature, FeatureState -from features.tasks import trigger_feature_state_change_webhooks +from features.tasks import delete_feature, trigger_feature_state_change_webhooks from organisations.models import Organisation from projects.models import Project from users.models import FFAdminUser @@ -166,6 +165,7 @@ def test_trigger_feature_state_change_webhooks_for_deleted_flag_uses_fs_instance assert data["previous_state"]["feature"]["id"] == feature_state.feature.id assert event_type == WebhookEventType.FLAG_DELETED.value + @pytest.mark.django_db def test_delete_feature__feature_does_not_exist__succeeds_silently( mocker: MockerFixture,