-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(eco): sentry app install deletion outbox #101896
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| from collections.abc import Collection, Mapping | ||
| from typing import TYPE_CHECKING, Any, ClassVar, overload | ||
|
|
||
| from django.db import models | ||
| from django.db import models, router, transaction | ||
| from django.utils import timezone | ||
| from jsonschema import ValidationError | ||
|
|
||
|
|
@@ -16,17 +16,17 @@ | |
| from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey | ||
| from sentry.db.models.manager.base_query_set import BaseQuerySet | ||
| from sentry.db.models.paranoia import ParanoidManager, ParanoidModel | ||
| from sentry.hybridcloud.models.outbox import ControlOutboxBase, outbox_context | ||
| from sentry.hybridcloud.models.outbox import ControlOutbox, ControlOutboxBase, outbox_context | ||
| from sentry.hybridcloud.outbox.base import ReplicatedControlModel | ||
| from sentry.hybridcloud.outbox.category import OutboxCategory | ||
| from sentry.hybridcloud.outbox.category import OutboxCategory, OutboxScope | ||
| from sentry.projects.services.project import RpcProject | ||
| from sentry.sentry_apps.services.app.model import RpcSentryAppComponent, RpcSentryAppInstallation | ||
| from sentry.sentry_apps.utils.errors import ( | ||
| SentryAppError, | ||
| SentryAppIntegratorError, | ||
| SentryAppSentryError, | ||
| ) | ||
| from sentry.types.region import find_regions_for_orgs | ||
| from sentry.types.region import find_all_region_names, find_regions_for_orgs | ||
|
|
||
| if TYPE_CHECKING: | ||
| from sentry.models.project import Project | ||
|
|
@@ -124,6 +124,12 @@ def save(self, *args, **kwargs): | |
| self.date_updated = timezone.now() | ||
| return super().save(*args, **kwargs) | ||
|
|
||
| def delete(self, *args, **kwargs): | ||
| 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) | ||
|
|
||
| @property | ||
| def api_application_id(self) -> int | None: | ||
| from sentry.sentry_apps.models.sentry_app import SentryApp | ||
|
|
@@ -141,6 +147,19 @@ def outboxes_for_update(self, shard_identifier: int | None = None) -> list[Contr | |
| # these isn't so important in that case. | ||
| return super().outboxes_for_update(shard_identifier=self.api_application_id or 0) | ||
|
|
||
| def outboxes_for_delete(self) -> list[ControlOutbox]: | ||
| return [ | ||
| ControlOutbox( | ||
| shard_scope=OutboxScope.APP_SCOPE, | ||
| shard_identifier=self.api_application_id or 0, | ||
| 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() | ||
| ] | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Outbox Processing InconsistencyThe |
||
| def prepare_ui_component( | ||
| self, | ||
| component: SentryAppComponent, | ||
|
|
||
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: Inconsistent Region Targeting in Delete Method
The
SentryAppInstallation.deletemethod'soutboxes_for_deletetargets all regions, unlikeoutboxes_for_updatewhich 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.Uh oh!
There was an error while loading. Please reload this page.
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.
The organization might already be deleted by the time we get here in the sentry app / install deletion task. See #101895