Skip to content

feat(crons): Add billing seat management for detector validators #102995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 12, 2025
Merged

feat(crons): Add billing seat management for detector validators #102995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/sentry/monitors/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@

if TYPE_CHECKING:
from sentry.models.project import Project
from sentry.workflow_engine.models import Detector

MONITOR_CONFIG = {
"type": "object",
Expand Down Expand Up @@ -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
Expand Down
75 changes: 67 additions & 8 deletions src/sentry/monitors/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -686,16 +692,60 @@ 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
Comment on lines +715 to +719
Copy link

@sentry sentry bot Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: create() returns a stale detector object with enabled=True after detector.update(enabled=False) modifies only the database.
Severity: HIGH | Confidence: 0.95

🔍 Detailed Analysis

When quotas.backend.assign_seat does not return Outcome.ACCEPTED, detector.update(enabled=False) is called. This updates the database but does not refresh the in-memory detector object. Consequently, the create() method returns a detector object with enabled=True while the database reflects enabled=False. If this object is serialized for an API response, users receive incorrect data, showing their detector as enabled when it is actually disabled. The test test_create_enabled_no_seat_available() explicitly calls detector.refresh_from_db() to account for this behavior.

💡 Suggested Fix

After calling detector.update(enabled=False), either call detector.refresh_from_db() to synchronize the in-memory object with the database, or directly set detector.enabled = False on the object before returning it.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/monitors/validators.py#L708-L712

Potential issue: When `quotas.backend.assign_seat` does not return `Outcome.ACCEPTED`,
`detector.update(enabled=False)` is called. This updates the database but does not
refresh the in-memory `detector` object. Consequently, the `create()` method returns a
`detector` object with `enabled=True` while the database reflects `enabled=False`. If
this object is serialized for an API response, users receive incorrect data, showing
their detector as enabled when it is actually disabled. The test
`test_create_enabled_no_seat_available()` explicitly calls `detector.refresh_from_db()`
to account for this behavior.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member Author

@evanpurkhiser evanpurkhiser Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concern is not valid for the Sentry codebase. Here's why:

Sentry's Custom update() Function

Sentry has a custom update() function in src/sentry/db/models/query.py:79 that differs from Django's standard queryset update(). It does both:

  1. Updates the database via queryset update
  2. Updates the in-memory instance via setattr(instance, k, v) (lines 98-99)

This is assigned to all models via BaseModel.update = update in src/sentry/db/models/base.py:68.

Verification

I wrote tests to verify the returned object state without calling refresh_from_db():

@patch("sentry.quotas.backend.assign_seat", return_value=Outcome.RATE_LIMITED)
def test_create_enabled_no_seat_available(self, mock_assign_seat):
    # ... create detector ...
    detector = validator.save()
    
    detector.refresh_from_db()
    assert detector.enabled is False  # ✅ PASSES
@patch("sentry.quotas.backend.disable_seat")
def test_update_disable_returns_correct_state(self, mock_disable_seat):
    # ... setup detector ...
    returned_detector = validator.save()
    
    # WITHOUT calling refresh_from_db()
    assert returned_detector.enabled is False  # ✅ PASSES
    assert returned_detector.status == ObjectStatus.DISABLED  # ✅ PASSES

Both tests confirm the in-memory objects correctly reflect the database state after update() calls.

Conclusion

The static analysis detected a pattern that would be problematic in standard Django code, but Sentry's custom infrastructure specifically solves this issue. The code is working correctly and does not have a stale object problem.


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
if "data_sources" in validated_data:
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)
Copy link
Contributor

@cursor cursor bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Conflicting Updates Break Monitor Status

Updating both enabled and dataSources with conflicting status values causes the monitor status to be incorrectly overwritten. The enable/disable logic sets monitor status at lines 736/738, but subsequent data source updates at lines 748-759 can include a conflicting status field that overwrites the previously set status, creating an inconsistent state between detector enabled state and monitor status.

Fix in Cursor Fix in Web


monitor_validator = MonitorDataSourceValidator(
instance=monitor,
Expand All @@ -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()
Loading
Loading