diff --git a/src/sentry/monitors/models.py b/src/sentry/monitors/models.py index 79d6ced545f699..467fff59cebd6c 100644 --- a/src/sentry/monitors/models.py +++ b/src/sentry/monitors/models.py @@ -50,6 +50,7 @@ if TYPE_CHECKING: from sentry.models.project import Project + from sentry.workflow_engine.models import Detector MONITOR_CONFIG = { "type": "object", @@ -812,6 +813,15 @@ class Meta: db_table = "sentry_monitorenvbrokendetection" +def get_cron_monitor(detector: Detector) -> Monitor: + """ + Given a detector get the matching cron monitor. + """ + data_source = detector.data_sources.first() + assert data_source + return Monitor.objects.get(id=int(data_source.source_id)) + + @data_source_type_registry.register(DATA_SOURCE_CRON_MONITOR) class CronMonitorDataSourceHandler(DataSourceTypeHandler[Monitor]): @override diff --git a/src/sentry/monitors/validators.py b/src/sentry/monitors/validators.py index d2fdc2e4328ae8..28168ea0d1e300 100644 --- a/src/sentry/monitors/validators.py +++ b/src/sentry/monitors/validators.py @@ -35,6 +35,7 @@ MonitorLimitsExceeded, ScheduleType, check_organization_monitor_limit, + get_cron_monitor, ) from sentry.monitors.schedule import get_next_schedule, get_prev_schedule from sentry.monitors.types import CrontabSchedule, slugify_monitor_slug @@ -52,7 +53,7 @@ BaseDataSourceValidator, BaseDetectorTypeValidator, ) -from sentry.workflow_engine.models import DataSource, Detector +from sentry.workflow_engine.models import Detector MONITOR_STATUSES = { "active": ObjectStatus.ACTIVE, @@ -373,10 +374,12 @@ def create(self, validated_data): config=validated_data["config"], ) - # Attempt to assign a seat for this monitor - seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor) - if seat_outcome != Outcome.ACCEPTED: - monitor.update(status=ObjectStatus.DISABLED) + # Skip quota operations if requested by context (e.g., detector flow handles this) + if not self.context.get("skip_quota", False): + # Attempt to assign a seat for this monitor + seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor) + if seat_outcome != Outcome.ACCEPTED: + monitor.update(status=ObjectStatus.DISABLED) request = self.context["request"] signal_monitor_created(project, request.user, False, monitor, request) @@ -636,9 +639,12 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: if self.instance: monitor_instance = self.instance + # Skip quota operations - the detector validator handles seat assignment + context = {**self.context, "skip_quota": True} + monitor_validator = MonitorValidator( data=monitor_data, - context=self.context, + context=context, instance=monitor_instance, partial=self.partial, ) @@ -686,7 +692,52 @@ class MonitorIncidentDetectorValidator(BaseDetectorTypeValidator): data_sources = serializers.ListField(child=MonitorDataSourceValidator(), required=False) + def validate_enabled(self, value: bool) -> bool: + """ + Validate that enabling a detector is allowed based on seat availability. + """ + detector = self.instance + if detector and value and not detector.enabled: + monitor = get_cron_monitor(detector) + result = quotas.backend.check_assign_seat(DataCategory.MONITOR, monitor) + if not result.assignable: + raise serializers.ValidationError(result.reason) + return value + + def create(self, validated_data): + detector = super().create(validated_data) + + with in_test_hide_transaction_boundary(): + monitor = get_cron_monitor(detector) + + # Try to assign a seat for the monitor + seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor) + if seat_outcome != Outcome.ACCEPTED: + detector.update(enabled=False) + monitor.update(status=ObjectStatus.DISABLED) + + return detector + def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector: + was_enabled = instance.enabled + enabled = validated_data.get("enabled", was_enabled) + + # Handle enable/disable seat operations + if was_enabled != enabled: + monitor = get_cron_monitor(instance) + + if enabled: + seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor) + # We should have already validated that a seat was available in + # validate_enabled, avoid races by failing here if we can't + # accept the seat + if seat_outcome != Outcome.ACCEPTED: + raise serializers.ValidationError("Failed to update monitor") + monitor.update(status=ObjectStatus.ACTIVE) + else: + quotas.backend.disable_seat(DataCategory.MONITOR, monitor) + monitor.update(status=ObjectStatus.DISABLED) + super().update(instance, validated_data) data_source_data = None @@ -694,8 +745,7 @@ def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector data_source_data = validated_data.pop("data_sources")[0] if data_source_data is not None: - data_source = DataSource.objects.get(detectors=instance) - monitor = Monitor.objects.get(id=data_source.source_id) + monitor = get_cron_monitor(instance) monitor_validator = MonitorDataSourceValidator( instance=monitor, @@ -709,3 +759,12 @@ def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector monitor_validator.save() return instance + + def delete(self) -> None: + assert self.instance is not None + monitor = get_cron_monitor(self.instance) + + # Remove the seat immediately + quotas.backend.remove_seat(DataCategory.MONITOR, monitor) + + super().delete() diff --git a/tests/sentry/monitors/test_validators.py b/tests/sentry/monitors/test_validators.py index 37c7f980a8423a..a62757b8312c68 100644 --- a/tests/sentry/monitors/test_validators.py +++ b/tests/sentry/monitors/test_validators.py @@ -6,22 +6,25 @@ from django.test import RequestFactory from django.test.utils import override_settings from django.utils import timezone +from rest_framework.exceptions import ErrorDetail from sentry.analytics.events.cron_monitor_created import CronMonitorCreated, FirstCronMonitorCreated from sentry.constants import DataCategory, ObjectStatus from sentry.models.rule import Rule, RuleSource -from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType +from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType, get_cron_monitor +from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR from sentry.monitors.validators import ( MonitorDataSourceValidator, MonitorIncidentDetectorValidator, MonitorValidator, ) +from sentry.quotas.base import SeatAssignmentResult from sentry.testutils.cases import MonitorTestCase from sentry.testutils.helpers.analytics import assert_any_analytics_event from sentry.types.actor import Actor from sentry.utils.outcomes import Outcome from sentry.utils.slug import DEFAULT_SLUG_ERROR_MESSAGE -from sentry.workflow_engine.models import DataConditionGroup +from sentry.workflow_engine.models import DataConditionGroup, DataSource, DataSourceDetector class MonitorValidatorCreateTest(MonitorTestCase): @@ -166,7 +169,6 @@ def test_monitor_organization_limit(self): } validator = MonitorValidator(data=data, context=self.context) assert not validator.is_valid() - from rest_framework.exceptions import ErrorDetail assert validator.errors["nonFieldErrors"] == [ ErrorDetail( @@ -973,14 +975,6 @@ def _assert_monitor_attributes(self, monitor, name, slug, schedule, status=Objec assert monitor.owner_user_id is None assert monitor.owner_team_id is None - @patch("sentry.quotas.backend.assign_seat") - def test_create_source_creates_monitor(self, mock_assign_seat): - mock_assign_seat.return_value = Outcome.ACCEPTED - validator = self._create_validator() - assert validator.is_valid(), validator.errors - monitor = validator.validated_create_source(validator.validated_data) - self._assert_monitor_attributes(monitor, "Test Monitor", "test-monitor", "0 * * * *") - def test_validate_with_owner(self): team = self.create_team(organization=self.organization) data = self._get_valid_data(owner=f"team:{team.id}") @@ -1021,14 +1015,6 @@ def test_slug_uniqueness_validation(self): assert "slug" in validator.errors assert 'The slug "test-monitor" is already in use.' in str(validator.errors["slug"]) - @patch("sentry.quotas.backend.assign_seat") - def test_quota_rejection_disables_monitor(self, mock_assign_seat): - mock_assign_seat.return_value = Outcome.RATE_LIMITED - validator = self._create_validator() - assert validator.is_valid(), validator.errors - monitor = validator.validated_create_source(validator.validated_data) - assert monitor.status == ObjectStatus.DISABLED - def test_update_monitor(self): monitor = Monitor.objects.create( organization_id=self.organization.id, @@ -1123,3 +1109,203 @@ def test_create_detector_validates_data_source(self): assert validator.is_valid(), validator.errors assert "_creator" in validator.validated_data["data_sources"][0] assert validator.validated_data["data_sources"][0]["data_source_type"] == "cron_monitor" + + @patch("sentry.quotas.backend.assign_seat", return_value=Outcome.ACCEPTED) + def test_create_enabled_assigns_seat(self, mock_assign_seat): + """Test that creating an enabled detector assigns a billing seat.""" + + condition_group = DataConditionGroup.objects.create( + organization_id=self.organization.id, + logic_type=DataConditionGroup.Type.ANY, + ) + context = {**self.context, "condition_group": condition_group} + validator = MonitorIncidentDetectorValidator( + data=self.valid_data, + context=context, + ) + assert validator.is_valid(), validator.errors + detector = validator.save() + + detector.refresh_from_db() + assert detector.enabled is True + + # Verify seat was assigned exactly once (not double-called) + monitor = get_cron_monitor(detector) + mock_assign_seat.assert_called_once_with(DataCategory.MONITOR, monitor) + + @patch("sentry.quotas.backend.assign_seat", return_value=Outcome.RATE_LIMITED) + def test_create_enabled_no_seat_available(self, mock_assign_seat): + """ + Test that creating a detector with no seats available creates it but + leaves it disabled. + """ + condition_group = DataConditionGroup.objects.create( + organization_id=self.organization.id, + logic_type=DataConditionGroup.Type.ANY, + ) + context = {**self.context, "condition_group": condition_group} + validator = MonitorIncidentDetectorValidator( + data=self.valid_data, + context=context, + ) + assert validator.is_valid(), validator.errors + detector = validator.save() + + detector.refresh_from_db() + # Detector created but not enabled due to no seat assignment + assert detector.enabled is False + monitor = get_cron_monitor(detector) + assert monitor.status == ObjectStatus.DISABLED + + # Verify seat assignment was attempted exactly once (not double-called) + mock_assign_seat.assert_called_once_with(DataCategory.MONITOR, monitor) + + @patch("sentry.quotas.backend.assign_seat", return_value=Outcome.ACCEPTED) + def test_update_enable_assigns_seat(self, mock_assign_seat): + """ + Test that enabling a previously disabled detector assigns a seat. + """ + # Create a disabled detector + detector = self.create_detector( + project=self.project, + name="Test Detector", + type="monitor_check_in_failure", + enabled=False, + ) + monitor = self._create_monitor( + name="Test Monitor", + slug="test-monitor", + status=ObjectStatus.DISABLED, + ) + data_source = DataSource.objects.create( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.organization.id, + source_id=str(monitor.id), + ) + DataSourceDetector.objects.create(data_source=data_source, detector=detector) + + validator = MonitorIncidentDetectorValidator( + instance=detector, data={"enabled": True}, context=self.context, partial=True + ) + assert validator.is_valid(), validator.errors + validator.save() + + detector.refresh_from_db() + monitor.refresh_from_db() + assert detector.enabled is True + assert monitor.status == ObjectStatus.ACTIVE + + # Verify seat was assigned exactly once + mock_assign_seat.assert_called_once_with(DataCategory.MONITOR, monitor) + + @patch( + "sentry.quotas.backend.check_assign_seat", + return_value=SeatAssignmentResult(assignable=False, reason="No seats available"), + ) + def test_update_enable_no_seat_available(self, mock_check_seat): + """ + Test that enabling fails with validation error when no seats are + available. + """ + # Create a disabled detector + detector = self.create_detector( + project=self.project, + name="Test Detector", + type="monitor_check_in_failure", + enabled=False, + ) + monitor = self._create_monitor( + name="Test Monitor", + slug="test-monitor", + status=ObjectStatus.DISABLED, + ) + data_source = DataSource.objects.create( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.organization.id, + source_id=str(monitor.id), + ) + DataSourceDetector.objects.create(data_source=data_source, detector=detector) + + validator = MonitorIncidentDetectorValidator( + instance=detector, data={"enabled": True}, context=self.context, partial=True + ) + + # Validation should fail due to no seats available + assert not validator.is_valid() + assert "enabled" in validator.errors + assert validator.errors["enabled"] == ["No seats available"] + + # Detector and monitor should still be disabled + detector.refresh_from_db() + monitor.refresh_from_db() + assert detector.enabled is False + assert monitor.status == ObjectStatus.DISABLED + + # Verify seat availability check was performed + mock_check_seat.assert_called_with(DataCategory.MONITOR, monitor) + + @patch("sentry.quotas.backend.disable_seat") + def test_update_disable_disables_seat(self, mock_disable_seat): + """Test that disabling a previously enabled detector disables the seat.""" + # Create an enabled detector + detector = self.create_detector( + project=self.project, + name="Test Detector", + type="monitor_check_in_failure", + enabled=True, + ) + monitor = self._create_monitor( + name="Test Monitor", + slug="test-monitor", + status=ObjectStatus.ACTIVE, + ) + data_source = DataSource.objects.create( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.organization.id, + source_id=str(monitor.id), + ) + DataSourceDetector.objects.create(data_source=data_source, detector=detector) + + validator = MonitorIncidentDetectorValidator( + instance=detector, data={"enabled": False}, context=self.context, partial=True + ) + assert validator.is_valid(), validator.errors + validator.save() + + detector.refresh_from_db() + monitor.refresh_from_db() + assert detector.enabled is False + assert monitor.status == ObjectStatus.DISABLED + + # Verify disable_seat was called exactly once + mock_disable_seat.assert_called_once_with(DataCategory.MONITOR, monitor) + + @patch("sentry.quotas.backend.remove_seat") + def test_delete_removes_seat(self, mock_remove_seat: MagicMock) -> None: + """Test that deleting a detector removes its billing seat immediately.""" + detector = self.create_detector( + project=self.project, + name="Test Detector", + type="monitor_check_in_failure", + enabled=True, + ) + monitor = self._create_monitor( + name="Test Monitor", + slug="test-monitor", + status=ObjectStatus.ACTIVE, + ) + data_source = DataSource.objects.create( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.organization.id, + source_id=str(monitor.id), + ) + DataSourceDetector.objects.create(data_source=data_source, detector=detector) + + validator = MonitorIncidentDetectorValidator( + instance=detector, data={}, context=self.context + ) + + validator.delete() + + # Verify remove_seat was called exactly once + mock_remove_seat.assert_called_once_with(DataCategory.MONITOR, monitor)