-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(eco): async deletions for sentry app installations #100930
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.
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
base: master
Are you sure you want to change the base?
Conversation
sentry_app_installation=install, user=request.user, action="deleted" | ||
).run() | ||
deletions.exec_sync(install) | ||
ScheduledDeletion.schedule(install, days=0, actor=request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to do this if we already collect installations in the sentry app deletion task?
ModelRelation(SentryAppInstallation, {"sentry_app_id": instance.id}), |
I'm not sure if this is necessary at all. It seems like we delete the installations regardless of whether the sentry app is internal or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Notification Timing Mismatch on App Deletion
Sentry App installation "deleted" notifications are sent immediately, but the actual installation deletion is now delayed to a scheduled task. This timing mismatch means external systems receive deletion notifications for installations that still exist in the database.
src/sentry/sentry_apps/api/endpoints/sentry_app_details.py#L223-L234
sentry/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py
Lines 223 to 234 in 4fc6206
if not sentry_app.is_internal: | |
for install in sentry_app.installations.all(): | |
try: | |
with transaction.atomic(using=router.db_for_write(SentryAppInstallation)): | |
assert ( | |
request.user.is_authenticated | |
), "User must be authenticated to delete installation" | |
SentryAppInstallationNotifier( | |
sentry_app_installation=install, user=request.user, action="deleted" | |
).run() | |
except RequestException as exc: | |
sentry_sdk.capture_exception(exc) |
except RequestException as exc: | ||
sentry_sdk.capture_exception(exc) | ||
deletions.exec_sync(sentry_app_installation) | ||
ScheduledDeletion.schedule(sentry_app_installation, days=0, actor=request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async Deletion Causes Audit Log Inconsistency
Switching to asynchronous deletion means the API returns success (204) and audit/analytics events are recorded before the installation is actually removed, creating a race condition and inconsistent state. The audit log entry also lacks a link to the scheduled deletion, and the scheduled deletion record itself may be routed to the wrong database.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Deleting a sentry app installation should use async deletions.
The tests for async sentry app deletion cover the below case:
sentry/src/sentry/deletions/defaults/sentry_app.py
Line 14 in e49b651
Required for #100813