From cd468080c2e64ef13aa560154895559db885c1f3 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 12 May 2025 15:54:08 -0700 Subject: [PATCH 1/7] fail quietly for token replication --- .../sentry_apps/token_exchange/refresher.py | 54 +++++++++++++------ 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index 3c9d5e2a457735..9566d46e380a19 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -1,7 +1,7 @@ import logging from dataclasses import dataclass -from django.db import router, transaction +from django.db import OperationalError, router, transaction from django.utils.functional import cached_property from sentry import analytics @@ -30,22 +30,44 @@ class Refresher: user: User def run(self) -> ApiToken: - with transaction.atomic(router.db_for_write(ApiToken)): - try: - self._validate() - self.token.delete() - - self._record_analytics() - return self._create_new_token() - except (SentryAppIntegratorError, SentryAppSentryError): - logger.info( - "refresher.context", - extra={ - "application_id": self.application.id, - "refresh_token": self.refresh_token[-4:], - }, + try: + with transaction.atomic(router.db_for_write(ApiToken)): + try: + token = None + self._validate() + self.token.delete() + + self._record_analytics() + token = self._create_new_token() + return token + except (SentryAppIntegratorError, SentryAppSentryError): + logger.info( + "refresher.context", + extra={ + "application_id": self.application.id, + "refresh_token": self.refresh_token[-4:], + }, + ) + raise + except OperationalError as e: + context = { + "installation_uuid": self.install.uuid, + "client_id": self.application.client_id[:SENSITIVE_CHARACTER_LIMIT], + "sentry_app_id": self.install.sentry_app.id, + } + if token is not None: + logger.warning( + "refresher.database-failure", + extra=context, + exc_info=e, ) - raise + return token + + raise SentryAppSentryError( + message="Failed to delete or create current token", + status_code=500, + webhook_context=context, + ) from e def _record_analytics(self) -> None: analytics.record( From e1730eee02b2dc845b551b0c6ec466aebb86a26f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 12 May 2025 16:24:44 -0700 Subject: [PATCH 2/7] remove old logs --- .../sentry_apps/token_exchange/refresher.py | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index 9566d46e380a19..28242c0efb8272 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -32,29 +32,20 @@ class Refresher: def run(self) -> ApiToken: try: with transaction.atomic(router.db_for_write(ApiToken)): - try: - token = None - self._validate() - self.token.delete() - - self._record_analytics() - token = self._create_new_token() - return token - except (SentryAppIntegratorError, SentryAppSentryError): - logger.info( - "refresher.context", - extra={ - "application_id": self.application.id, - "refresh_token": self.refresh_token[-4:], - }, - ) - raise + token = None + self._validate() + self.token.delete() + + self._record_analytics() + token = self._create_new_token() + return token except OperationalError as e: context = { "installation_uuid": self.install.uuid, "client_id": self.application.client_id[:SENSITIVE_CHARACTER_LIMIT], "sentry_app_id": self.install.sentry_app.id, } + if token is not None: logger.warning( "refresher.database-failure", @@ -64,7 +55,7 @@ def run(self) -> ApiToken: return token raise SentryAppSentryError( - message="Failed to delete or create current token", + message="Failed to refresh given token", status_code=500, webhook_context=context, ) from e From 34fd420a4f99ab70520cfe3594c1a4a4520ba428 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 12 May 2025 16:54:25 -0700 Subject: [PATCH 3/7] add tests and fix some consistency issues --- .../sentry_apps/token_exchange/refresher.py | 2 +- .../sentry_apps/token_exchange/validator.py | 2 +- .../token_exchange/test_refresher.py | 31 ++++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index 28242c0efb8272..c0448d7c29664d 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -31,8 +31,8 @@ class Refresher: def run(self) -> ApiToken: try: + token = None with transaction.atomic(router.db_for_write(ApiToken)): - token = None self._validate() self.token.delete() diff --git a/src/sentry/sentry_apps/token_exchange/validator.py b/src/sentry/sentry_apps/token_exchange/validator.py index 085e156efd10a1..b3a363cab30107 100644 --- a/src/sentry/sentry_apps/token_exchange/validator.py +++ b/src/sentry/sentry_apps/token_exchange/validator.py @@ -68,7 +68,7 @@ def application(self) -> ApiApplication: try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise SentryAppSentryError( + raise SentryAppIntegratorError( "Application does not exist", webhook_context={"client_id": self.client_id[:SENSITIVE_CHARACTER_LIMIT]}, ) diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 6a10e18ec05b37..686af2eecbe8bc 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -1,6 +1,7 @@ from unittest.mock import PropertyMock, patch import pytest +from django.db.utils import OperationalError from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken @@ -92,8 +93,9 @@ def test_token_must_exist(self, _): } assert e.value.public_context == {} + @patch("sentry.sentry_apps.token_exchange.refresher.Refresher._validate") @patch("sentry.models.ApiApplication.objects.get", side_effect=ApiApplication.DoesNotExist) - def test_api_application_must_exist(self, _): + def test_api_application_must_exist(self, _, mock_validate): with pytest.raises(SentryAppIntegratorError) as e: self.refresher.run() @@ -133,3 +135,30 @@ def test_records_analytics(self, record): sentry_app_installation_id=self.install.id, exchange_type="refresh", ) + + def test_returns_token_on_outbox_error(self): + # Mock the transaction to raise OperationalError after token creation + with patch( + "sentry.hybridcloud.models.outbox.process_region_outbox.send" + ) as mock_transaction: + mock_transaction.atomic.side_effect = OperationalError("Outbox issue") + + # The refresher should return the token even though there was an error + token = self.refresher.run() + assert SentryAppInstallation.objects.get(id=self.install.id).api_token == token + + def test_raises_error_on_operational_error_before_creation(self): + # Mock the transaction to raise OperationalError before token creation + with patch("sentry.sentry_apps.token_exchange.refresher.transaction") as mock_transaction: + mock_transaction.atomic.side_effect = OperationalError("Database error") + + with pytest.raises(SentryAppSentryError) as e: + self.refresher.run() + + assert e.value.message == "Failed to refresh given token" + assert e.value.status_code == 500 + assert e.value.webhook_context == { + "installation_uuid": self.install.uuid, + "client_id": self.client_id[:SENSITIVE_CHARACTER_LIMIT], + "sentry_app_id": self.install.sentry_app.id, + } From d0f42389b81b4dc0d1cfecf7ab9551584f78c747 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 12 May 2025 17:01:01 -0700 Subject: [PATCH 4/7] align missing apiapplication errors --- src/sentry/sentry_apps/token_exchange/refresher.py | 2 +- src/sentry/sentry_apps/token_exchange/validator.py | 2 +- tests/sentry/sentry_apps/token_exchange/test_refresher.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index c0448d7c29664d..e5d47a762ef3c0 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -119,7 +119,7 @@ def application(self) -> ApiApplication: try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise SentryAppIntegratorError( + raise SentryAppSentryError( message="Could not find matching Application for given client_id", status_code=401, webhook_context={ diff --git a/src/sentry/sentry_apps/token_exchange/validator.py b/src/sentry/sentry_apps/token_exchange/validator.py index b3a363cab30107..085e156efd10a1 100644 --- a/src/sentry/sentry_apps/token_exchange/validator.py +++ b/src/sentry/sentry_apps/token_exchange/validator.py @@ -68,7 +68,7 @@ def application(self) -> ApiApplication: try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise SentryAppIntegratorError( + raise SentryAppSentryError( "Application does not exist", webhook_context={"client_id": self.client_id[:SENSITIVE_CHARACTER_LIMIT]}, ) diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 686af2eecbe8bc..416f81202ab7b4 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -96,7 +96,7 @@ def test_token_must_exist(self, _): @patch("sentry.sentry_apps.token_exchange.refresher.Refresher._validate") @patch("sentry.models.ApiApplication.objects.get", side_effect=ApiApplication.DoesNotExist) def test_api_application_must_exist(self, _, mock_validate): - with pytest.raises(SentryAppIntegratorError) as e: + with pytest.raises(SentryAppSentryError) as e: self.refresher.run() assert e.value.message == "Could not find matching Application for given client_id" From 5d63e30b89722dcd36543515c4aa7637303694a7 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 19 May 2025 11:12:40 -0700 Subject: [PATCH 5/7] move error catching to outbox --- src/sentry/hybridcloud/models/outbox.py | 12 +++++++++-- .../sentry_apps/token_exchange/refresher.py | 7 ++++--- .../token_exchange/test_refresher.py | 20 +------------------ 3 files changed, 15 insertions(+), 24 deletions(-) diff --git a/src/sentry/hybridcloud/models/outbox.py b/src/sentry/hybridcloud/models/outbox.py index 7555a871014333..6a57d7c7f8447b 100644 --- a/src/sentry/hybridcloud/models/outbox.py +++ b/src/sentry/hybridcloud/models/outbox.py @@ -9,7 +9,7 @@ import sentry_sdk from django import db -from django.db import OperationalError, connections, models, router, transaction +from django.db import DatabaseError, OperationalError, connections, models, router, transaction from django.db.models import Count, Max, Min from django.db.models.functions import Now from django.db.transaction import Atomic @@ -195,7 +195,15 @@ def save(self, *args: Any, **kwargs: Any) -> None: ) if _outbox_context.flushing_enabled: - transaction.on_commit(lambda: self.drain_shard(), using=router.db_for_write(type(self))) + try: + transaction.on_commit( + lambda: self.drain_shard(), using=router.db_for_write(type(self)) + ) + except DatabaseError as e: + raise OutboxFlushError( + f"Failed to process Outbox, {OutboxCategory(self.category).name} due to database error", + self, + ) from e tags = {"category": OutboxCategory(self.category).name} metrics.incr("outbox.saved", 1, tags=tags) diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index e5d47a762ef3c0..03ca86941f4da2 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -1,10 +1,11 @@ import logging from dataclasses import dataclass -from django.db import OperationalError, router, transaction +from django.db import router, transaction from django.utils.functional import cached_property from sentry import analytics +from sentry.hybridcloud.models.outbox import OutboxFlushError from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp @@ -39,7 +40,7 @@ def run(self) -> ApiToken: self._record_analytics() token = self._create_new_token() return token - except OperationalError as e: + except OutboxFlushError as e: context = { "installation_uuid": self.install.uuid, "client_id": self.application.client_id[:SENSITIVE_CHARACTER_LIMIT], @@ -48,7 +49,7 @@ def run(self) -> ApiToken: if token is not None: logger.warning( - "refresher.database-failure", + "refresher.outbox-failure", extra=context, exc_info=e, ) diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 416f81202ab7b4..277619c042436d 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -138,27 +138,9 @@ def test_records_analytics(self, record): def test_returns_token_on_outbox_error(self): # Mock the transaction to raise OperationalError after token creation - with patch( - "sentry.hybridcloud.models.outbox.process_region_outbox.send" - ) as mock_transaction: + with patch("sentry.hybridcloud.models.outbox.OutboxBase.drain_shard") as mock_transaction: mock_transaction.atomic.side_effect = OperationalError("Outbox issue") # The refresher should return the token even though there was an error token = self.refresher.run() assert SentryAppInstallation.objects.get(id=self.install.id).api_token == token - - def test_raises_error_on_operational_error_before_creation(self): - # Mock the transaction to raise OperationalError before token creation - with patch("sentry.sentry_apps.token_exchange.refresher.transaction") as mock_transaction: - mock_transaction.atomic.side_effect = OperationalError("Database error") - - with pytest.raises(SentryAppSentryError) as e: - self.refresher.run() - - assert e.value.message == "Failed to refresh given token" - assert e.value.status_code == 500 - assert e.value.webhook_context == { - "installation_uuid": self.install.uuid, - "client_id": self.client_id[:SENSITIVE_CHARACTER_LIMIT], - "sentry_app_id": self.install.sentry_app.id, - } From efa18d535a4ab8a2646de2a1c82a856717b5e60f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 20 May 2025 09:39:03 -0700 Subject: [PATCH 6/7] add test for exception catching --- src/sentry/hybridcloud/models/outbox.py | 64 ++++++++++--------- .../sentry_apps/token_exchange/refresher.py | 4 +- .../sentry/hybridcloud/models/test_outbox.py | 23 ++++++- .../token_exchange/test_refresher.py | 4 +- 4 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/sentry/hybridcloud/models/outbox.py b/src/sentry/hybridcloud/models/outbox.py index 6a57d7c7f8447b..55d481d66a8b1e 100644 --- a/src/sentry/hybridcloud/models/outbox.py +++ b/src/sentry/hybridcloud/models/outbox.py @@ -48,6 +48,10 @@ def __init__(self, message: str, outbox: OutboxBase) -> None: self.outbox = outbox +class OutboxDatabaseError(DatabaseError): + pass + + class InvalidOutboxError(Exception): pass @@ -195,15 +199,7 @@ def save(self, *args: Any, **kwargs: Any) -> None: ) if _outbox_context.flushing_enabled: - try: - transaction.on_commit( - lambda: self.drain_shard(), using=router.db_for_write(type(self)) - ) - except DatabaseError as e: - raise OutboxFlushError( - f"Failed to process Outbox, {OutboxCategory(self.category).name} due to database error", - self, - ) from e + transaction.on_commit(lambda: self.drain_shard(), using=router.db_for_write(type(self))) tags = {"category": OutboxCategory(self.category).name} metrics.incr("outbox.saved", 1, tags=tags) @@ -333,32 +329,38 @@ def drain_shard( in_test_assert_no_transaction( "drain_shard should only be called outside of any active transaction!" ) - # When we are flushing in a local context, we don't care about outboxes created concurrently -- - # at best our logic depends on previously created outboxes. - latest_shard_row: OutboxBase | None = None - if not flush_all: - latest_shard_row = self.selected_messages_in_shard().last() - # If we're not flushing all possible shards, and we don't see any immediate values, - # drop. - if latest_shard_row is None: - return - - shard_row: OutboxBase | None - while True: - with self.process_shard(latest_shard_row) as shard_row: - if shard_row is None: - break - if _test_processing_barrier: - _test_processing_barrier.wait() + try: + # When we are flushing in a local context, we don't care about outboxes created concurrently -- + # at best our logic depends on previously created outboxes. + latest_shard_row: OutboxBase | None = None + if not flush_all: + latest_shard_row = self.selected_messages_in_shard().last() + # If we're not flushing all possible shards, and we don't see any immediate values, + # drop. + if latest_shard_row is None: + return + + shard_row: OutboxBase | None + while True: + with self.process_shard(latest_shard_row) as shard_row: + if shard_row is None: + break - processed = shard_row.process(is_synchronous_flush=not flush_all) + if _test_processing_barrier: + _test_processing_barrier.wait() - if _test_processing_barrier: - _test_processing_barrier.wait() + processed = shard_row.process(is_synchronous_flush=not flush_all) - if not processed: - break + if _test_processing_barrier: + _test_processing_barrier.wait() + + if not processed: + break + except DatabaseError as e: + raise OutboxDatabaseError( + f"Failed to process Outbox, {OutboxCategory(self.category).name} due to database error", + ) from e @classmethod def get_shard_depths_descending(cls, limit: int | None = 10) -> list[dict[str, int | str]]: diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index 03ca86941f4da2..287b50d0530b23 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -5,7 +5,7 @@ from django.utils.functional import cached_property from sentry import analytics -from sentry.hybridcloud.models.outbox import OutboxFlushError +from sentry.hybridcloud.models.outbox import OutboxDatabaseError from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp @@ -40,7 +40,7 @@ def run(self) -> ApiToken: self._record_analytics() token = self._create_new_token() return token - except OutboxFlushError as e: + except OutboxDatabaseError as e: context = { "installation_uuid": self.install.uuid, "client_id": self.application.client_id[:SENSITIVE_CHARACTER_LIMIT], diff --git a/tests/sentry/hybridcloud/models/test_outbox.py b/tests/sentry/hybridcloud/models/test_outbox.py index 152100ef7a3179..1eff4ba21b9119 100644 --- a/tests/sentry/hybridcloud/models/test_outbox.py +++ b/tests/sentry/hybridcloud/models/test_outbox.py @@ -6,11 +6,12 @@ from unittest.mock import Mock, call, patch import pytest -from django.db import connections +from django.db import OperationalError, connections from pytest import raises from sentry.hybridcloud.models.outbox import ( ControlOutbox, + OutboxDatabaseError, OutboxFlushError, RegionOutbox, outbox_context, @@ -518,6 +519,26 @@ def test_scheduling_with_past_and_future_outbox_times(self) -> None: # message is in the past. assert RegionOutbox.objects.count() == 0 + @patch("sentry.hybridcloud.models.outbox.OutboxBase.process_coalesced") + def test_catches_random_database_errors(self, mock_process) -> None: + mock_process.side_effect = OperationalError("ruh roh") + + with pytest.raises(OutboxDatabaseError) as e: + with outbox_context(flush=False): + Organization(id=10).outbox_for_update().save() + assert RegionOutbox.objects.count() == 1 + + with outbox_runner(): + # drain outboxes + pass + + assert ( + str(e.value) + == f"Failed to process Outbox, {Organization(id=10).outbox_for_update().category.name} due to database error" + ) + + assert RegionOutbox.objects.count() == 1 + class TestOutboxesManager(TestCase): def test_bulk_operations(self) -> None: diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 277619c042436d..058690ca942c09 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -138,7 +138,9 @@ def test_records_analytics(self, record): def test_returns_token_on_outbox_error(self): # Mock the transaction to raise OperationalError after token creation - with patch("sentry.hybridcloud.models.outbox.OutboxBase.drain_shard") as mock_transaction: + with patch( + "sentry.hybridcloud.models.outbox.OutboxBase.process_coalesced" + ) as mock_transaction: mock_transaction.atomic.side_effect = OperationalError("Outbox issue") # The refresher should return the token even though there was an error From ae7b0617efe72a11adc7e09d337b34aba3c70aa3 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 20 May 2025 13:16:31 -0700 Subject: [PATCH 7/7] fix tests and typing --- tests/sentry/hybridcloud/models/test_outbox.py | 2 +- tests/sentry/sentry_apps/token_exchange/test_refresher.py | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/sentry/hybridcloud/models/test_outbox.py b/tests/sentry/hybridcloud/models/test_outbox.py index 1eff4ba21b9119..b26ad5823c5c90 100644 --- a/tests/sentry/hybridcloud/models/test_outbox.py +++ b/tests/sentry/hybridcloud/models/test_outbox.py @@ -520,7 +520,7 @@ def test_scheduling_with_past_and_future_outbox_times(self) -> None: assert RegionOutbox.objects.count() == 0 @patch("sentry.hybridcloud.models.outbox.OutboxBase.process_coalesced") - def test_catches_random_database_errors(self, mock_process) -> None: + def test_catches_random_database_errors(self, mock_process: Mock) -> None: mock_process.side_effect = OperationalError("ruh roh") with pytest.raises(OutboxDatabaseError) as e: diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 058690ca942c09..281264d1af3392 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -138,10 +138,8 @@ def test_records_analytics(self, record): def test_returns_token_on_outbox_error(self): # Mock the transaction to raise OperationalError after token creation - with patch( - "sentry.hybridcloud.models.outbox.OutboxBase.process_coalesced" - ) as mock_transaction: - mock_transaction.atomic.side_effect = OperationalError("Outbox issue") + with patch("sentry.hybridcloud.models.outbox.OutboxBase.process_coalesced") as mock_process: + mock_process.side_effect = OperationalError("Outbox issue") # The refresher should return the token even though there was an error token = self.refresher.run()