chore(eco): sentry app install deletion outbox #101896
Conversation
github-actions
bot
added
the
Scope: Backend
❌ 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 |
| return [ | ||
| ControlOutbox( | ||
| shard_scope=OutboxScope.APP_SCOPE, | ||
| shard_identifier=self.id, | ||
| object_identifier=self.id, | ||
| category=OutboxCategory.SENTRY_APP_INSTALLATION_DELETE, | ||
| region_name=region_name, | ||
| payload={"uuid": self.uuid}, | ||
| ) | ||
| for region_name in find_all_region_names() |
There was a problem hiding this comment.
Changes look good, just needs testing
There was a problem hiding this comment.
Oh does this still need a new delete override on SentryAppInstallation as well?
There was a problem hiding this comment.
yes I found out while testing lmao
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| with outbox_context(transaction.atomic(using=router.db_for_write(SentryAppInstallation))): | ||
| for outbox in self.outboxes_for_delete(): | ||
| outbox.save() | ||
| return super().delete(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
Bug: The SentryAppInstallation.delete() override calls super().delete() outside its transaction, creating a race condition between the creation of deletion outboxes and the actual model deletion.
(Severity: High 0.80 | Confidence: 0.95)
🔍 Detailed Analysis
The override of SentryAppInstallation.delete() creates and saves deletion outboxes within one transaction but calls super().delete() outside of it. This parent method call initiates a second, separate transaction to perform the soft-delete and create its own update outboxes. This creates a race condition where the deletion outboxes can be processed by the receiver before the model is actually soft-deleted. This can lead to inconsistent states, such as actions being disabled for an installation that has not yet been marked as deleted in the database.
💡 Suggested Fix
Move the super().delete(*args, **kwargs) call inside the with outbox_context(...) block. This ensures that the creation of deletion outboxes and the execution of the parent's delete logic occur within the same atomic transaction, preventing race conditions.
🤖 Prompt for AI Agent
Fix this bug. In src/sentry/sentry_apps/models/sentry_app_installation.py at lines
128-132: The override of `SentryAppInstallation.delete()` creates and saves deletion
outboxes within one transaction but calls `super().delete()` outside of it. This parent
method call initiates a second, separate transaction to perform the soft-delete and
create its own update outboxes. This creates a race condition where the deletion
outboxes can be processed by the receiver before the model is actually soft-deleted.
This can lead to inconsistent states, such as actions being disabled for an installation
that has not yet been marked as deleted in the database.
Did we get this right? 👍 / 👎 to inform future reviews.
cathteng
force-pushed
the
cathy/eco/sentry-app-install-delete-outbox
branch
from
455ff76 to
1d88eb1
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return [ | ||
| ControlOutbox( | ||
| shard_scope=OutboxScope.APP_SCOPE, | ||
| shard_identifier=self.id, |
There was a problem hiding this comment.
Can we use the same shard_identifier as update? Having the same sharding keys ensures that all the outboxes for a specific record are processed in order.
| payload={"uuid": self.uuid}, | ||
| ) | ||
| for region_name in find_all_region_names() | ||
| ] |
There was a problem hiding this comment.
Bug: Inconsistent Region Targeting in Delete Method
The SentryAppInstallation.delete method's outboxes_for_delete targets all regions, unlike outboxes_for_update which correctly targets only regions where the organization exists. This inconsistency sends unnecessary delete messages to irrelevant regions, potentially causing processing errors or breaking replication ordering.
There was a problem hiding this comment.
The organization might already be deleted by the time we get here in the sentry app / install deletion task. See #101895
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
Add an outbox to update actions statuses when a sentry app installation is deleted. This will be eventually consistent.