fix(deletion): Fix MonitorCheckIn deletion failures #103786
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Our previous attmept at correcting processing timeouts for MonitorCheckIn deletions failed, and introduced integrity errors because MonitorCheckIn cant be bulk deleted without manually removing child dependancies (no CASCADE in db FKs). Instead we add proper deletion tasks to the different models. Note we also use `get_child_relations_bulk` here which is actually an old method but not in use in our codebase today. Using it though we might be able to speed up querying for child relations of models with complex relations, when they cant just be outright bulk deleted using BulkModelDeletionTask.
yuvmen
force-pushed
the
yuvmen/fix-monitorcheckin-deletion
branch
from
966cfe3 to
1bea4ed
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
wedamija
reviewed
Nov 20, 2025
Member
wedamija
left a comment
wedamija
left a comment
There was a problem hiding this comment.
I think maybe we should cascade this like
Monitor -> MonitorIncident -> MonitorCheckin rather than Monitor -> MonitorCheckin -> MonitorIncident
If we delete the MonitorIncident rows first, then the checkins won't have to cascade delete to the MonitorIncident table and you can use the bulk deletion stuff.
This also helps with the MonitorEnvironment deletions, since they won't be referenced by MonitorIncident either
Comment on lines
7
to
29
| class MonitorCheckInDeletionTask(ModelDeletionTask[MonitorCheckIn]): | ||
| def get_child_relations_bulk( | ||
| self, instance_list: Sequence[MonitorCheckIn] | ||
| ) -> list[BaseRelation]: | ||
| """ | ||
| Return bulk child relations for MonitorCheckIn deletion. | ||
| Uses __in queries to efficiently delete MonitorIncidents that reference these check-ins. | ||
| """ | ||
| from sentry.monitors import models | ||
|
|
||
| checkin_ids = [ci.id for ci in instance_list] | ||
|
|
||
| return [ | ||
| ModelRelation( | ||
| models.MonitorIncident, | ||
| {"starting_checkin_id__in": checkin_ids}, | ||
| ), | ||
| ModelRelation( | ||
| models.MonitorIncident, | ||
| {"resolving_checkin_id__in": checkin_ids}, | ||
| ), | ||
| ] |
Member
There was a problem hiding this comment.
I think we can remove this if we cascade in the other direction
…itor and monitor_environment flows allow MonitorIncident to be deleted not in bulk - there shouldnt be so many to cause problems without it
Member
Author
|
Refactored according to discussion with Dan, will leverage two facts:
|
wedamija
approved these changes
Nov 20, 2025
yuvmen
added a commit
that referenced
this pull request
Nov 24, 2025
In #103786, 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our previous attempt (#103495) at correcting processing timeouts for MonitorCheckIn deletions failed , and introduced integrity errors because MonitorCheckIn cant be bulk deleted without manually removing child dependancies (no CASCADE in db FKs).
Added deletion of MonitorIncident directly before attempting to bulk delete MonitorCheckIns, basically removing the children of MonitorCheckins before deleting them.