Skip to content

Conversation

@yuvmen
Copy link
Member

@yuvmen yuvmen commented Nov 17, 2025

changed MonitorCheckIn to be deleted using BulkModelDeletionTask.
It used to need individual deletions to clean up attachments manually but attachments were removed from this model and it can now safely use Bulk deletion to speed things up.

should fix:
SENTRY-41GE

changed MonitorCheckIn to be deleted using BulkModelDeletionTask since it no
longer requires individual deletion following attachments being removed it.
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 17, 2025
@yuvmen yuvmen requested review from Swatinem and wedamija November 17, 2025 22:58
Comment on lines +15 to +17
ModelRelation(
models.MonitorCheckIn, {"monitor_id": instance.id}, BulkModelDeletionTask
),
Copy link

Choose a reason for hiding this comment

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

Bug: MonitorCheckIn deletion via BulkModelDeletionTask will fail due to unhandled MonitorIncident foreign key references.
Severity: CRITICAL | Confidence: 0.95

🔍 Detailed Analysis

When MonitorCheckIn records are deleted using BulkModelDeletionTask, and MonitorIncident records still reference them via starting_checkin or resolving_checkin foreign keys, the database will attempt to enforce foreign key constraints. Since these foreign keys lack database-level CASCADE delete configuration, this will lead to database constraint violation errors and unexpected deletion failures, preventing the bulk delete operation from completing successfully. The PR author overlooked the existing MonitorIncident foreign key relationships to MonitorCheckIn.

💡 Suggested Fix

Create a MonitorIncidentDeletionTask to handle MonitorIncident deletion before MonitorCheckIn, or configure database-level CASCADE delete on the foreign keys, or revert to ModelDeletionTask for MonitorCheckIn to process child relations.

🤖 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/deletions/defaults/monitor.py#L15-L17

Potential issue: When `MonitorCheckIn` records are deleted using
`BulkModelDeletionTask`, and `MonitorIncident` records still reference them via
`starting_checkin` or `resolving_checkin` foreign keys, the database will attempt to
enforce foreign key constraints. Since these foreign keys lack database-level `CASCADE`
delete configuration, this will lead to database constraint violation errors and
unexpected deletion failures, preventing the bulk delete operation from completing
successfully. The PR author overlooked the existing `MonitorIncident` foreign key
relationships to `MonitorCheckIn`.

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

Reference_id: 2749526

Copy link
Member Author

Choose a reason for hiding this comment

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

I was conflicted about this, we have foreign keys with CASCADE set up on these. I believe this means they should be getting deleted automatically when the bulk task deletes them at the DB level, cursor agrees, I believe this is probably wrong but I am not sure.

Copy link
Member

Choose a reason for hiding this comment

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

You can always deploy and see if there are more optimizations.

@codecov
Copy link

codecov bot commented Nov 17, 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   #103495      +/-   ##
===========================================
+ Coverage   72.69%    80.62%   +7.92%     
===========================================
  Files        9241      9246       +5     
  Lines      394715    394936     +221     
  Branches    25169     25169              
===========================================
+ Hits       286956    318399   +31443     
+ Misses     107331     76109   -31222     
  Partials      428       428              

@yuvmen yuvmen merged commit 9e3ffda into master Nov 18, 2025
97 of 99 checks passed
@yuvmen yuvmen deleted the yuvmen/fix-monitor-deletion-timeouts branch November 18, 2025 17:53
yuvmen added a commit that referenced this pull request Nov 21, 2025
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.
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.

4 participants