chore(crons): Move monitor update logic to be entirely inside the serializer take 2 #97943
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| MonitorCheckIn.objects.filter( | ||
| monitor_id=instance.id, status=CheckInStatus.IN_PROGRESS | ||
| ).update(timeout_at=TruncMinute(F("date_added")) + get_max_runtime(max_runtime)) | ||
|
|
There was a problem hiding this comment.
Bug: Serializer Logic Mismatch Causes Unnecessary Updates
In MonitorSerializer.update, the logic for checkin_margin and max_runtime updates compares against validated_data["config"]. This partial data can incorrectly trigger updates to MonitorEnvironment and MonitorCheckIn objects, even when these values haven't changed, potentially passing None to update functions.
We had a bug where we accidentally didn't save all the required fields for the monitor config. This causes a bug when we copy the config over to the monitor checkin where we end up storing a null value. We should instead treat this as a warning - if the config is already stored in the monitor, it should be replicated regardless of whether it's valid or not. The validation should happen at mutation time, which is being fixed in #97943 Fixes SENTRY-4DDN
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| if "config" in validated_data: | ||
| new_config = validated_data["config"] | ||
| checkin_margin = new_config.get("checkin_margin") | ||
| if checkin_margin != existing_margin: | ||
| MonitorEnvironment.objects.filter(monitor_id=instance.id).update( | ||
| next_checkin_latest=F("next_checkin") + get_checkin_margin(checkin_margin) | ||
| ) | ||
|
|
||
| max_runtime = new_config.get("max_runtime") | ||
| if max_runtime != existing_max_runtime: | ||
| MonitorCheckIn.objects.filter( | ||
| monitor_id=instance.id, status=CheckInStatus.IN_PROGRESS | ||
| ).update(timeout_at=TruncMinute(F("date_added")) + get_max_runtime(max_runtime)) |
There was a problem hiding this comment.
Potential bug: The timeout update logic in MonitorValidator uses a partial config, causing custom timeouts to be reset to default values during unrelated partial updates.
-
Description: When a monitor's configuration is partially updated (e.g., only changing the schedule), the timeout update logic incorrectly uses the partial
validated_data["config"]. This dictionary lacks keys likecheckin_marginormax_runtimeif they were not part of the update. The code then comparesNone(fromnew_config.get("checkin_margin")) with the existing value, which evaluates to true and triggers an update. This incorrectly resets the monitor'scheckin_marginandmax_runtimeto their default values, causing users to lose their custom timeout settings and potentially leading to incorrect monitor alerting. -
Suggested fix: The timeout update logic should check if
checkin_marginandmax_runtimewere present in the incomingvalidated_data["config"]before comparing and updating their values. This ensures that only explicitly changed values are updated and prevents accidental resets.
severity: 0.7, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97943 +/- ##
==========================================
+ Coverage 79.55% 80.63% +1.08%
==========================================
Files 8593 8595 +2
Lines 379066 379100 +34
Branches 24700 24700
==========================================
+ Hits 301558 305683 +4125
+ Misses 77130 73039 -4091
Partials 378 378 |
We had a bug where we accidentally didn't save all the required fields for the monitor config. This causes a bug when we copy the config over to the monitor checkin where we end up storing a null value. We should instead treat this as a warning - if the config is already stored in the monitor, it should be replicated regardless of whether it's valid or not. The validation should happen at mutation time, which is being fixed in #97943 Fixes SENTRY-4DDN
This consolidates update logic to be inside of the serializer. It also fixes some weirdness where we seem to save the monitor first, but then perform other validation that can fail.
The second commit here fixes a problem where when we update a monitor with a parital config, we end up dropping some required fields