From 95b6a4e7278090cf55d8d462041ecd5ef06b70bb Mon Sep 17 00:00:00 2001 From: Yuval Mandelboum Date: Mon, 24 Nov 2025 10:16:56 -0800 Subject: [PATCH] fix(deletions): Fix MonitorCheckIn direct deletion we refactored the way MonitorCheckIn gets deleted through Monitor and MonitorEnvironment deletion, but in the process hurt direct MonitorCheckIn cleanup which also happens daily. Added a task to handle it and set the relations to use Bulk Deletion instead of defaulting to Bulk. --- src/sentry/deletions/__init__.py | 2 +- src/sentry/deletions/defaults/__init__.py | 1 + src/sentry/deletions/defaults/monitor.py | 12 +- .../deletions/defaults/monitor_checkin.py | 20 +++ .../deletions/defaults/monitor_environment.py | 9 +- .../sentry/deletions/test_monitor_checkin.py | 117 ++++++++++++++++-- 6 files changed, 144 insertions(+), 17 deletions(-) create mode 100644 src/sentry/deletions/defaults/monitor_checkin.py diff --git a/src/sentry/deletions/__init__.py b/src/sentry/deletions/__init__.py index 8f802a58c9c9a7..b78643e72e6fec 100644 --- a/src/sentry/deletions/__init__.py +++ b/src/sentry/deletions/__init__.py @@ -106,7 +106,7 @@ def load_defaults(manager: DeletionTaskManager) -> None: manager.register(integrations.RepositoryProjectPathConfig, defaults.RepositoryProjectPathConfigDeletionTask) manager.register(monitors.Monitor, defaults.MonitorDeletionTask) manager.register(monitors.MonitorEnvironment, defaults.MonitorEnvironmentDeletionTask) - manager.register(monitors.MonitorCheckIn, BulkModelDeletionTask) + manager.register(monitors.MonitorCheckIn, defaults.MonitorCheckInDeletionTask) manager.register(monitors.MonitorIncident, defaults.MonitorIncidentDeletionTask) manager.register(monitors.MonitorEnvBrokenDetection, BulkModelDeletionTask) manager.register(sentry_apps.PlatformExternalIssue, defaults.PlatformExternalIssueDeletionTask) diff --git a/src/sentry/deletions/defaults/__init__.py b/src/sentry/deletions/defaults/__init__.py index 7c389107b2a5f5..ae5d0329bb5bc0 100644 --- a/src/sentry/deletions/defaults/__init__.py +++ b/src/sentry/deletions/defaults/__init__.py @@ -13,6 +13,7 @@ from .grouphash import * # noqa: F401,F403 from .incident import * # noqa: F401,F403 from .monitor import * # noqa: F401,F403 +from .monitor_checkin import * # noqa: F401,F403 from .monitor_environment import * # noqa: F401,F403 from .monitor_incident import * # noqa: F401,F403 from .organization import * # noqa: F401,F403 diff --git a/src/sentry/deletions/defaults/monitor.py b/src/sentry/deletions/defaults/monitor.py index 9624c178f3875a..8a88ef56d72b4c 100644 --- a/src/sentry/deletions/defaults/monitor.py +++ b/src/sentry/deletions/defaults/monitor.py @@ -1,4 +1,9 @@ -from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.deletions.base import ( + BaseRelation, + BulkModelDeletionTask, + ModelDeletionTask, + ModelRelation, +) from sentry.monitors.models import Monitor @@ -8,6 +13,9 @@ def get_child_relations(self, instance: Monitor) -> list[BaseRelation]: return [ ModelRelation(models.MonitorIncident, {"monitor_id": instance.id}), - ModelRelation(models.MonitorCheckIn, {"monitor_id": instance.id}), + # Use BulkModelDeletionTask here since MonitorIncidents are already handled above + ModelRelation( + models.MonitorCheckIn, {"monitor_id": instance.id}, BulkModelDeletionTask + ), ModelRelation(models.MonitorEnvironment, {"monitor_id": instance.id}), ] diff --git a/src/sentry/deletions/defaults/monitor_checkin.py b/src/sentry/deletions/defaults/monitor_checkin.py new file mode 100644 index 00000000000000..c0735d4a8da458 --- /dev/null +++ b/src/sentry/deletions/defaults/monitor_checkin.py @@ -0,0 +1,20 @@ +from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.monitors.models import MonitorCheckIn + + +class MonitorCheckInDeletionTask(ModelDeletionTask[MonitorCheckIn]): + def get_child_relations(self, instance: MonitorCheckIn) -> list[BaseRelation]: + from sentry.monitors import models + + # When MonitorCheckIn is deleted directly, we need to delete MonitorIncidents + # that reference it. MonitorIncident has two FKs pointing to MonitorCheckIn. + return [ + ModelRelation( + models.MonitorIncident, + {"starting_checkin_id": instance.id}, + ), + ModelRelation( + models.MonitorIncident, + {"resolving_checkin_id": instance.id}, + ), + ] diff --git a/src/sentry/deletions/defaults/monitor_environment.py b/src/sentry/deletions/defaults/monitor_environment.py index a8819c259c2df1..441f0194f9b034 100644 --- a/src/sentry/deletions/defaults/monitor_environment.py +++ b/src/sentry/deletions/defaults/monitor_environment.py @@ -1,4 +1,9 @@ -from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.deletions.base import ( + BaseRelation, + BulkModelDeletionTask, + ModelDeletionTask, + ModelRelation, +) from sentry.monitors.models import MonitorEnvironment @@ -11,8 +16,10 @@ def get_child_relations(self, instance: MonitorEnvironment) -> list[BaseRelation models.MonitorIncident, {"monitor_environment_id": instance.id}, ), + # Use BulkModelDeletionTask here since MonitorIncidents are already handled above ModelRelation( models.MonitorCheckIn, {"monitor_environment_id": instance.id}, + BulkModelDeletionTask, ), ] diff --git a/tests/sentry/deletions/test_monitor_checkin.py b/tests/sentry/deletions/test_monitor_checkin.py index 61c95474c12155..e22b2fe61da4fa 100644 --- a/tests/sentry/deletions/test_monitor_checkin.py +++ b/tests/sentry/deletions/test_monitor_checkin.py @@ -14,15 +14,106 @@ class DeleteMonitorCheckInTest(APITestCase, TransactionTestCase, HybridCloudTestMixin): - def test_delete_monitor_checkin_with_incidents_and_detections(self) -> None: + + def test_delete_checkin_directly(self) -> None: + """ + Test that deleting a MonitorCheckIn directly (not via Monitor deletion) + properly handles MonitorIncident children via MonitorCheckInDeletionTask. + """ + project = self.create_project(name="test") + env = Environment.objects.create(organization_id=project.organization_id, name="prod") + + monitor = Monitor.objects.create( + organization_id=project.organization.id, + project_id=project.id, + config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB}, + ) + monitor_env = MonitorEnvironment.objects.create( + monitor=monitor, + environment_id=env.id, + ) + + # Create check-ins + checkin1 = MonitorCheckIn.objects.create( + monitor=monitor, + monitor_environment=monitor_env, + project_id=project.id, + status=CheckInStatus.ERROR, + ) + checkin2 = MonitorCheckIn.objects.create( + monitor=monitor, + monitor_environment=monitor_env, + project_id=project.id, + status=CheckInStatus.OK, + ) + checkin3 = MonitorCheckIn.objects.create( + monitor=monitor, + monitor_environment=monitor_env, + project_id=project.id, + status=CheckInStatus.ERROR, + ) + + # Create incidents referencing checkin1 + incident1 = MonitorIncident.objects.create( + monitor=monitor, + monitor_environment=monitor_env, + starting_checkin=checkin1, + resolving_checkin=checkin2, + ) + incident2 = MonitorIncident.objects.create( + monitor=monitor, + monitor_environment=monitor_env, + starting_checkin=checkin3, + resolving_checkin=checkin1, # checkin1 is also a resolving checkin + ) + + detection1 = MonitorEnvBrokenDetection.objects.create( + monitor_incident=incident1, + ) + detection2 = MonitorEnvBrokenDetection.objects.create( + monitor_incident=incident2, + ) + + # Verify initial state + assert MonitorCheckIn.objects.count() == 3 + assert MonitorIncident.objects.count() == 2 + assert MonitorEnvBrokenDetection.objects.count() == 2 + + # Delete checkin1 directly (not via Monitor) + self.ScheduledDeletion.schedule(instance=checkin1, days=0) + + with self.tasks(): + run_scheduled_deletions() + + # Verify checkin1 is deleted + assert not MonitorCheckIn.objects.filter(id=checkin1.id).exists() + + # Verify both incidents are deleted (incident1 has checkin1 as starting_checkin, + # incident2 has checkin1 as resolving_checkin) + assert not MonitorIncident.objects.filter(id=incident1.id).exists() + assert not MonitorIncident.objects.filter(id=incident2.id).exists() + + # Verify detections are deleted + assert not MonitorEnvBrokenDetection.objects.filter(id=detection1.id).exists() + assert not MonitorEnvBrokenDetection.objects.filter(id=detection2.id).exists() + + # Verify other check-ins still exist + assert MonitorCheckIn.objects.filter(id=checkin2.id).exists() + assert MonitorCheckIn.objects.filter(id=checkin3.id).exists() + assert MonitorCheckIn.objects.count() == 2 + + # Verify monitor and environment still exist + assert Monitor.objects.filter(id=monitor.id).exists() + assert MonitorEnvironment.objects.filter(id=monitor_env.id).exists() + + def test_delete_monitor_with_incidents_and_detections(self) -> None: """ - Test that deleting MonitorCheckIns properly cascades to: - - MonitorIncidents (via starting_checkin_id and resolving_checkin_id) - - MonitorEnvBrokenDetection (via MonitorIncident) + Test that deleting a Monitor properly cascades to: + - MonitorIncidents (deleted first via child relations) + - MonitorCheckIns (bulk deleted after incidents) + - MonitorEnvBrokenDetection (via MonitorIncident deletion) - This tests the get_child_relations_bulk() implementation which should: - 1. Use __in queries for MonitorIncidents pointing to multiple check-ins - 2. Bulk delete MonitorEnvBrokenDetection via BulkModelDeletionTask + This verifies the ordered deletion: MonitorIncident → MonitorCheckIn → MonitorEnvironment """ project = self.create_project(name="test") env = Environment.objects.create(organization_id=project.organization_id, name="prod") @@ -107,10 +198,10 @@ def test_delete_monitor_checkin_with_incidents_and_detections(self) -> None: assert Environment.objects.filter(id=env.id).exists() assert self.project.__class__.objects.filter(id=self.project.id).exists() - def test_delete_multiple_checkins_with_shared_incident(self) -> None: + def test_delete_monitor_with_shared_incident(self) -> None: """ - Test edge case where one incident references multiple check-ins - (starting_checkin != resolving_checkin from the same batch). + Test that deleting a Monitor handles edge case where one incident references + multiple check-ins (starting_checkin != resolving_checkin). """ project = self.create_project(name="test") env = Environment.objects.create(organization_id=project.organization_id, name="prod") @@ -162,10 +253,10 @@ def test_delete_multiple_checkins_with_shared_incident(self) -> None: assert not MonitorIncident.objects.filter(id=incident.id).exists() assert not MonitorEnvBrokenDetection.objects.filter(id=detection.id).exists() - def test_delete_monitor_only_affects_its_own_checkins(self) -> None: + def test_delete_monitor_only_affects_its_own_data(self) -> None: """ - Test that deleting one monitor's check-ins doesn't affect another monitor's data. - This verifies that the __in queries are properly scoped. + Test that deleting one Monitor doesn't affect another Monitor's data. + This verifies that deletion queries are properly scoped by monitor_id. """ project = self.create_project(name="test") env = Environment.objects.create(organization_id=project.organization_id, name="prod")