Skip to content

Conversation

evanpurkhiser
Copy link
Member

Move uptime subscription limit checking from exception handling in
create() to validator validation method. This ensures limits are checked
during validation phase when creating new uptime monitors, providing
consistent error responses and cleaner code.

Follows the same pattern as cron monitors for consistency.

Move uptime subscription limit checking from exception handling in
create() to validator validation method. This ensures limits are checked
during validation phase when creating new uptime monitors, providing
consistent error responses and cleaner code.

Follows the same pattern as cron monitors for consistency.
Comment on lines +279 to 283
target_object=detector.id,
event=audit_log.get_event_id("UPTIME_MONITOR_ADD"),
data=get_audit_log_data(detector),
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: The create method no longer handles the MaxManualUptimeSubscriptionsReached exception, which can be raised during a race condition, causing a 500 error instead of a validation error.
  • Description: The create method in UptimeMonitorValidator no longer handles the MaxManualUptimeSubscriptionsReached exception. This exception can be raised by check_uptime_subscription_limit, which is called within the creation logic. While a limit check was added to the validate method, a race condition can occur where concurrent requests cause the subscription limit to be exceeded between the validation and creation steps. Because the try...except block was removed from the create method, this race condition will lead to an unhandled exception, resulting in a 500 Internal Server Error instead of the expected 400 Bad Request with a validation error.

  • Suggested fix: Wrap the call to super().create() within the create method in a try...except MaxManualUptimeSubscriptionsReached block. In the except block, raise a ValidationError to ensure subscription limit errors are handled gracefully as 400 responses, preventing 500 errors from race conditions.
    severity: 0.65, confidence: 0.98

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

Copy link

codecov bot commented Oct 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #100904      +/-   ##
===========================================
- Coverage   81.15%    81.14%   -0.01%     
===========================================
  Files        8615      8616       +1     
  Lines      382250    382254       +4     
  Branches    24033     24033              
===========================================
- Hits       310199    310194       -5     
- Misses      71723     71732       +9     
  Partials      328       328              

@evanpurkhiser evanpurkhiser merged commit ca3c674 into master Oct 3, 2025
66 checks passed
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/ref-uptime-move-subscription-limit-check-to-validator branch October 3, 2025 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants