From 8e9634f7a31ecb2b1470fa8067f06656b87199fb Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 26 Nov 2025 06:09:53 -0800 Subject: [PATCH 1/6] reuse taskworker alarm for webhooks --- src/sentry/options/defaults.py | 7 + src/sentry/sentry_apps/metrics.py | 1 + src/sentry/utils/sentry_apps/webhooks.py | 38 ++- .../utils/sentry_apps/test_webhook_timeout.py | 250 ++++++++++++++++++ 4 files changed, 291 insertions(+), 5 deletions(-) create mode 100644 tests/sentry/utils/sentry_apps/test_webhook_timeout.py diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 950e31fc7fbcbf..b3267960cbb1c7 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -2393,6 +2393,13 @@ flags=FLAG_AUTOMATOR_MODIFIABLE, ) +# Hard timeout for webhook requests to prevent indefinite hangs +register( + "sentry-apps.webhook.hard-timeout.sec", + default=3.0, + flags=FLAG_AUTOMATOR_MODIFIABLE, +) + # Enables statistical detectors for a project register( "statistical_detectors.enable", diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index 95f465d4aed186..fce8a042340efa 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -70,6 +70,7 @@ class SentryAppWebhookHaltReason(StrEnum): MISSING_INSTALLATION = "missing_installation" RESTRICTED_IP = "restricted_ip" CONNECTION_RESET = "connection_reset" + HARD_TIMEOUT = "hard_timeout" class SentryAppExternalRequestFailureReason(StrEnum): diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 97399851e0306a..01380246c60353 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -2,6 +2,7 @@ import logging from collections.abc import Callable, Mapping +from types import FrameType from typing import TYPE_CHECKING, Any, Concatenate, ParamSpec, TypeVar import sentry_sdk @@ -20,6 +21,7 @@ from sentry.sentry_apps.models.sentry_app import SentryApp, track_response_code from sentry.sentry_apps.utils.errors import SentryAppSentryError from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError +from sentry.taskworker.workerchild import timeout_alarm from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer if TYPE_CHECKING: @@ -36,6 +38,23 @@ T = TypeVar("T", bound=Mapping[str, Any]) +class WebhookTimeoutError(BaseException): + """This error represents a user set hard timeout for when a + webhook request should've completed within X seconds + """ + + pass + + +def _handle_webhook_timeout(signum: int, frame: FrameType | None) -> None: + """Handler for when a webhook request exceeds the hard timeout deadline. + - This is a workaround for safe_create_connection sockets hanging when the given url + cannot be reached or resolved. + - TODO: Add sentry app disabling logic here + """ + raise WebhookTimeoutError("Webhook request exceeded hard timeout deadline") + + def ignore_unpublished_app_errors( func: Callable[Concatenate[SentryApp | RpcSentryApp, P], R], ) -> Callable[Concatenate[SentryApp | RpcSentryApp, P], R | None]: @@ -98,13 +117,22 @@ def send_and_save_webhook_request( ) assert url is not None + + timeout_seconds = int(options.get("sentry-apps.webhook.hard-timeout.sec")) + try: - response = safe_urlopen( - url=url, - data=app_platform_event.body, - headers=app_platform_event.headers, - timeout=options.get("sentry-apps.webhook.timeout.sec"), + with timeout_alarm(timeout_seconds, _handle_webhook_timeout): + response = safe_urlopen( + url=url, + data=app_platform_event.body, + headers=app_platform_event.headers, + timeout=options.get("sentry-apps.webhook.timeout.sec"), + ) + except WebhookTimeoutError: + lifecycle.record_halt( + halt_reason=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.HARD_TIMEOUT}" ) + raise except (Timeout, ConnectionError) as e: error_type = e.__class__.__name__.lower() lifecycle.add_extras( diff --git a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py new file mode 100644 index 00000000000000..a492ac9260e26d --- /dev/null +++ b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py @@ -0,0 +1,250 @@ +import signal +import time +from unittest.mock import Mock, patch + +import pytest +from requests import Response + +from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent +from sentry.sentry_apps.metrics import SentryAppWebhookHaltReason +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import region_silo_test +from sentry.utils.sentry_apps.webhooks import WebhookTimeoutError, send_and_save_webhook_request + + +@region_silo_test +class WebhookTimeoutTest(TestCase): + def setUp(self): + self.organization = self.create_organization() + self.sentry_app = self.create_sentry_app( + name="TestApp", + organization=self.organization, + webhook_url="https://example.com/webhook", + ) + self.install = self.create_sentry_app_installation( + organization=self.organization, slug=self.sentry_app.slug + ) + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + def test_webhook_completes_before_timeout(self, mock_options, mock_safe_urlopen): + """Test that webhook completes successfully when it finishes before timeout""" + + # Configure timeouts + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 3.0 + elif key == "sentry-apps.webhook.timeout.sec": + return 1.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + elif key == "sentry-apps.webhook-logging.enabled": + return {"installation_uuid": [], "sentry_app_slug": []} + return None + + mock_options.side_effect = options_side_effect + + # Mock successful response + mock_response = Mock(spec=Response) + mock_response.status_code = 200 + mock_response.headers = {} + mock_safe_urlopen.return_value = mock_response + + # Create event + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + # Should complete without raising WebhookTimeoutError + response = send_and_save_webhook_request(self.sentry_app, app_platform_event) + + assert response == mock_response + mock_safe_urlopen.assert_called_once() + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + def test_webhook_interrupted_by_hard_timeout(self, mock_options, mock_safe_urlopen): + """Test that webhook is interrupted when it exceeds hard timeout""" + + # Configure timeouts + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 1.0 # 1 second hard timeout + elif key == "sentry-apps.webhook.timeout.sec": + return 10.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + elif key == "sentry-apps.webhook-logging.enabled": + return {"installation_uuid": [], "sentry_app_slug": []} + return None + + mock_options.side_effect = options_side_effect + + # Make safe_urlopen sleep longer than the timeout + def slow_urlopen(*args, **kwargs): + time.sleep(2.0) # Sleep longer than 1 second timeout + mock_response = Mock(spec=Response) + mock_response.status_code = 200 + return mock_response + + mock_safe_urlopen.side_effect = slow_urlopen + + # Create event + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + # Should raise WebhookTimeoutError + with pytest.raises(WebhookTimeoutError, match="Webhook request exceeded hard timeout"): + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + def test_timeout_exception_propagates(self, mock_options, mock_safe_urlopen): + """Test that WebhookTimeoutError propagates correctly""" + + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 1.0 + elif key == "sentry-apps.webhook.timeout.sec": + return 10.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + return None + + mock_options.side_effect = options_side_effect + + # Make safe_urlopen sleep + def slow_urlopen(*args, **kwargs): + time.sleep(2.0) + return Mock(spec=Response) + + mock_safe_urlopen.side_effect = slow_urlopen + + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + # Verify exception type and inheritance + with pytest.raises(BaseException): # WebhookTimeoutError inherits from BaseException + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + with pytest.raises(WebhookTimeoutError): + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + @patch("sentry.sentry_apps.metrics.SentryAppInteractionEvent") + def test_lifecycle_record_halt_called_on_timeout( + self, mock_interaction_event, mock_options, mock_safe_urlopen + ): + """Test that lifecycle.record_halt() is called when timeout occurs""" + + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 1.0 + elif key == "sentry-apps.webhook.timeout.sec": + return 10.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + return None + + mock_options.side_effect = options_side_effect + + # Setup mock lifecycle + mock_lifecycle = Mock() + mock_interaction_event.return_value.capture.return_value.__enter__.return_value = ( + mock_lifecycle + ) + + # Make safe_urlopen sleep + def slow_urlopen(*args, **kwargs): + time.sleep(2.0) + return Mock(spec=Response) + + mock_safe_urlopen.side_effect = slow_urlopen + + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + # Should raise WebhookTimeoutError + with pytest.raises(WebhookTimeoutError): + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + # Verify lifecycle.record_halt() was called with correct halt reason + mock_lifecycle.record_halt.assert_called_once() + halt_reason = mock_lifecycle.record_halt.call_args[1]["halt_reason"] + assert SentryAppWebhookHaltReason.HARD_TIMEOUT in halt_reason + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + def test_timeout_alarm_restores_signal_handler(self, mock_options, mock_safe_urlopen): + """Test that signal handler is properly restored after timeout""" + + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 1.0 + elif key == "sentry-apps.webhook.timeout.sec": + return 10.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + elif key == "sentry-apps.webhook-logging.enabled": + return {"installation_uuid": [], "sentry_app_slug": []} + return None + + mock_options.side_effect = options_side_effect + + # Get original handler + original_handler = signal.signal(signal.SIGALRM, signal.SIG_DFL) + signal.signal(signal.SIGALRM, original_handler) + + # Mock quick response + mock_response = Mock(spec=Response) + mock_response.status_code = 200 + mock_response.headers = {} + mock_safe_urlopen.return_value = mock_response + + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + # Verify signal handler was restored + current_handler = signal.signal(signal.SIGALRM, signal.SIG_DFL) + signal.signal(signal.SIGALRM, current_handler) + assert current_handler == original_handler + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + @patch("sentry.utils.sentry_apps.webhooks.options.get") + def test_alarm_cancelled_after_successful_webhook(self, mock_options, mock_safe_urlopen): + """Test that alarm is cancelled after webhook completes successfully""" + + def options_side_effect(key): + if key == "sentry-apps.webhook.hard-timeout.sec": + return 5.0 + elif key == "sentry-apps.webhook.timeout.sec": + return 1.0 + elif key == "sentry-apps.webhook.restricted-webhook-sending": + return [] + elif key == "sentry-apps.webhook-logging.enabled": + return {"installation_uuid": [], "sentry_app_slug": []} + return None + + mock_options.side_effect = options_side_effect + + mock_response = Mock(spec=Response) + mock_response.status_code = 200 + mock_response.headers = {} + mock_safe_urlopen.return_value = mock_response + + app_platform_event = AppPlatformEvent( + resource="issue", action="created", install=self.install, data={"test": "data"} + ) + + send_and_save_webhook_request(self.sentry_app, app_platform_event) + + # Verify no alarm is pending + remaining = signal.alarm(0) + assert remaining == 0 # No alarm was pending From 62644f79f9373c299d1256896f2d2128cb904e3a Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 18 Dec 2025 15:58:11 -0800 Subject: [PATCH 2/6] update tests for webhook alarm --- src/sentry/features/temporary.py | 2 + src/sentry/options/defaults.py | 2 +- src/sentry/utils/sentry_apps/webhooks.py | 26 ++- .../utils/sentry_apps/test_webhook_timeout.py | 187 +++++++----------- 4 files changed, 90 insertions(+), 127 deletions(-) diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 0da81a26bb6836..b5a17ac2f18c65 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -642,6 +642,8 @@ def register_temporary_features(manager: FeatureManager) -> None: manager.add("organizations:sentry-app-manual-token-refresh", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Enables Conduit demo endpoint and UI manager.add("organizations:conduit-demo", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) + # Enable hard timeout alarm for webhooks + manager.add("organizations:sentry-app-webhook-hard-timeout", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Enables organization access to the new notification platform manager.add("organizations:notification-platform.internal-testing", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 953c8f8dfc1c0c..464d2fe26097a8 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -2426,7 +2426,7 @@ # Hard timeout for webhook requests to prevent indefinite hangs register( "sentry-apps.webhook.hard-timeout.sec", - default=3.0, + default=5.0, flags=FLAG_AUTOMATOR_MODIFIABLE, ) diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 01380246c60353..c7627b45425353 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -10,9 +10,10 @@ from requests.exceptions import ChunkedEncodingError, ConnectionError, Timeout from rest_framework import status -from sentry import options +from sentry import features, options from sentry.exceptions import RestrictedIPAddress from sentry.http import safe_urlopen +from sentry.organizations.services.organization.service import organization_service from sentry.sentry_apps.metrics import ( SentryAppEventType, SentryAppWebhookFailureReason, @@ -50,7 +51,7 @@ def _handle_webhook_timeout(signum: int, frame: FrameType | None) -> None: """Handler for when a webhook request exceeds the hard timeout deadline. - This is a workaround for safe_create_connection sockets hanging when the given url cannot be reached or resolved. - - TODO: Add sentry app disabling logic here + - TODO(christinarlong): Add sentry app disabling logic here """ raise WebhookTimeoutError("Webhook request exceeded hard timeout deadline") @@ -117,17 +118,30 @@ def send_and_save_webhook_request( ) assert url is not None - - timeout_seconds = int(options.get("sentry-apps.webhook.hard-timeout.sec")) - try: - with timeout_alarm(timeout_seconds, _handle_webhook_timeout): + organization_context = organization_service.get_organization_by_id( + id=app_platform_event.install.organization_id, + ) + if organization_context is not None and features.has( + "organizations:sentry-app-webhook-hard-timeout", + organization_context.organization, + ): + timeout_seconds = int(options.get("sentry-apps.webhook.hard-timeout.sec")) + with timeout_alarm(timeout_seconds, _handle_webhook_timeout): + response = safe_urlopen( + url=url, + data=app_platform_event.body, + headers=app_platform_event.headers, + timeout=options.get("sentry-apps.webhook.timeout.sec"), + ) + else: response = safe_urlopen( url=url, data=app_platform_event.body, headers=app_platform_event.headers, timeout=options.get("sentry-apps.webhook.timeout.sec"), ) + except WebhookTimeoutError: lifecycle.record_halt( halt_reason=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.HARD_TIMEOUT}" diff --git a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py index a492ac9260e26d..475fcf2882492b 100644 --- a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py +++ b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py @@ -7,7 +7,10 @@ from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent from sentry.sentry_apps.metrics import SentryAppWebhookHaltReason +from sentry.testutils.asserts import assert_halt_metric from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.features import with_feature +from sentry.testutils.helpers.options import override_options from sentry.testutils.silo import region_silo_test from sentry.utils.sentry_apps.webhooks import WebhookTimeoutError, send_and_save_webhook_request @@ -25,32 +28,23 @@ def setUp(self): organization=self.organization, slug=self.sentry_app.slug ) + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 5.0, + "sentry-apps.webhook.timeout.sec": 1.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + "sentry-apps.webhook-logging.enabled": {"installation_uuid": [], "sentry_app_slug": []}, + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - def test_webhook_completes_before_timeout(self, mock_options, mock_safe_urlopen): - """Test that webhook completes successfully when it finishes before timeout""" - - # Configure timeouts - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 3.0 - elif key == "sentry-apps.webhook.timeout.sec": - return 1.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - elif key == "sentry-apps.webhook-logging.enabled": - return {"installation_uuid": [], "sentry_app_slug": []} - return None - - mock_options.side_effect = options_side_effect - + def test_webhook_completes_before_timeout(self, mock_safe_urlopen): # Mock successful response mock_response = Mock(spec=Response) mock_response.status_code = 200 mock_response.headers = {} mock_safe_urlopen.return_value = mock_response - # Create event app_platform_event = AppPlatformEvent( resource="issue", action="created", install=self.install, data={"test": "data"} ) @@ -61,28 +55,20 @@ def options_side_effect(key): assert response == mock_response mock_safe_urlopen.assert_called_once() + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 1.0, + "sentry-apps.webhook.timeout.sec": 10.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + "sentry-apps.webhook-logging.enabled": {"installation_uuid": [], "sentry_app_slug": []}, + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - def test_webhook_interrupted_by_hard_timeout(self, mock_options, mock_safe_urlopen): - """Test that webhook is interrupted when it exceeds hard timeout""" - - # Configure timeouts - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 1.0 # 1 second hard timeout - elif key == "sentry-apps.webhook.timeout.sec": - return 10.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - elif key == "sentry-apps.webhook-logging.enabled": - return {"installation_uuid": [], "sentry_app_slug": []} - return None - - mock_options.side_effect = options_side_effect - + def test_webhook_interrupted_by_hard_timeout(self, mock_safe_urlopen): # Make safe_urlopen sleep longer than the timeout def slow_urlopen(*args, **kwargs): - time.sleep(2.0) # Sleep longer than 1 second timeout + time.sleep(6.0) # Sleep longer than 1 second timeout mock_response = Mock(spec=Response) mock_response.status_code = 200 return mock_response @@ -98,22 +84,16 @@ def slow_urlopen(*args, **kwargs): with pytest.raises(WebhookTimeoutError, match="Webhook request exceeded hard timeout"): send_and_save_webhook_request(self.sentry_app, app_platform_event) + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 1.0, + "sentry-apps.webhook.timeout.sec": 10.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - def test_timeout_exception_propagates(self, mock_options, mock_safe_urlopen): - """Test that WebhookTimeoutError propagates correctly""" - - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 1.0 - elif key == "sentry-apps.webhook.timeout.sec": - return 10.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - return None - - mock_options.side_effect = options_side_effect - + def test_timeout_exception_propagates(self, mock_safe_urlopen): # Make safe_urlopen sleep def slow_urlopen(*args, **kwargs): time.sleep(2.0) @@ -125,38 +105,20 @@ def slow_urlopen(*args, **kwargs): resource="issue", action="created", install=self.install, data={"test": "data"} ) - # Verify exception type and inheritance - with pytest.raises(BaseException): # WebhookTimeoutError inherits from BaseException - send_and_save_webhook_request(self.sentry_app, app_platform_event) - with pytest.raises(WebhookTimeoutError): send_and_save_webhook_request(self.sentry_app, app_platform_event) + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 1.0, + "sentry-apps.webhook.timeout.sec": 10.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - @patch("sentry.sentry_apps.metrics.SentryAppInteractionEvent") - def test_lifecycle_record_halt_called_on_timeout( - self, mock_interaction_event, mock_options, mock_safe_urlopen - ): - """Test that lifecycle.record_halt() is called when timeout occurs""" - - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 1.0 - elif key == "sentry-apps.webhook.timeout.sec": - return 10.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - return None - - mock_options.side_effect = options_side_effect - - # Setup mock lifecycle - mock_lifecycle = Mock() - mock_interaction_event.return_value.capture.return_value.__enter__.return_value = ( - mock_lifecycle - ) - + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_lifecycle_record_halt_called_on_timeout(self, mock_record, mock_safe_urlopen): # Make safe_urlopen sleep def slow_urlopen(*args, **kwargs): time.sleep(2.0) @@ -168,33 +130,25 @@ def slow_urlopen(*args, **kwargs): resource="issue", action="created", install=self.install, data={"test": "data"} ) - # Should raise WebhookTimeoutError with pytest.raises(WebhookTimeoutError): send_and_save_webhook_request(self.sentry_app, app_platform_event) - # Verify lifecycle.record_halt() was called with correct halt reason - mock_lifecycle.record_halt.assert_called_once() - halt_reason = mock_lifecycle.record_halt.call_args[1]["halt_reason"] - assert SentryAppWebhookHaltReason.HARD_TIMEOUT in halt_reason + assert_halt_metric( + mock_record=mock_record, + error_msg=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.HARD_TIMEOUT}", + ) + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 5.0, + "sentry-apps.webhook.timeout.sec": 10.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + "sentry-apps.webhook-logging.enabled": {"installation_uuid": [], "sentry_app_slug": []}, + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - def test_timeout_alarm_restores_signal_handler(self, mock_options, mock_safe_urlopen): - """Test that signal handler is properly restored after timeout""" - - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 1.0 - elif key == "sentry-apps.webhook.timeout.sec": - return 10.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - elif key == "sentry-apps.webhook-logging.enabled": - return {"installation_uuid": [], "sentry_app_slug": []} - return None - - mock_options.side_effect = options_side_effect - + def test_timeout_alarm_restores_signal_handler(self, mock_safe_urlopen): # Get original handler original_handler = signal.signal(signal.SIGALRM, signal.SIG_DFL) signal.signal(signal.SIGALRM, original_handler) @@ -216,24 +170,17 @@ def options_side_effect(key): signal.signal(signal.SIGALRM, current_handler) assert current_handler == original_handler + @with_feature("organizations:sentry-app-webhook-hard-timeout") + @override_options( + { + "sentry-apps.webhook.hard-timeout.sec": 5.0, + "sentry-apps.webhook.timeout.sec": 1.0, + "sentry-apps.webhook.restricted-webhook-sending": [], + "sentry-apps.webhook-logging.enabled": {"installation_uuid": [], "sentry_app_slug": []}, + } + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - @patch("sentry.utils.sentry_apps.webhooks.options.get") - def test_alarm_cancelled_after_successful_webhook(self, mock_options, mock_safe_urlopen): - """Test that alarm is cancelled after webhook completes successfully""" - - def options_side_effect(key): - if key == "sentry-apps.webhook.hard-timeout.sec": - return 5.0 - elif key == "sentry-apps.webhook.timeout.sec": - return 1.0 - elif key == "sentry-apps.webhook.restricted-webhook-sending": - return [] - elif key == "sentry-apps.webhook-logging.enabled": - return {"installation_uuid": [], "sentry_app_slug": []} - return None - - mock_options.side_effect = options_side_effect - + def test_alarm_cancelled_after_successful_webhook(self, mock_safe_urlopen): mock_response = Mock(spec=Response) mock_response.status_code = 200 mock_response.headers = {} From 21d422cc27360fda245dffeaa5cbba7f7cb42a3c Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 19 Feb 2026 12:01:36 -0800 Subject: [PATCH 3/6] update typing in tests --- .../utils/sentry_apps/test_webhook_timeout.py | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py index 475fcf2882492b..62bcfcbd427e2c 100644 --- a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py +++ b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py @@ -7,6 +7,7 @@ from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent from sentry.sentry_apps.metrics import SentryAppWebhookHaltReason +from sentry.sentry_apps.utils.webhooks import IssueActionType, SentryAppResourceType from sentry.testutils.asserts import assert_halt_metric from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature @@ -46,7 +47,10 @@ def test_webhook_completes_before_timeout(self, mock_safe_urlopen): mock_safe_urlopen.return_value = mock_response app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) # Should complete without raising WebhookTimeoutError @@ -77,7 +81,10 @@ def slow_urlopen(*args, **kwargs): # Create event app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) # Should raise WebhookTimeoutError @@ -102,7 +109,10 @@ def slow_urlopen(*args, **kwargs): mock_safe_urlopen.side_effect = slow_urlopen app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) with pytest.raises(WebhookTimeoutError): @@ -127,7 +137,10 @@ def slow_urlopen(*args, **kwargs): mock_safe_urlopen.side_effect = slow_urlopen app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) with pytest.raises(WebhookTimeoutError): @@ -160,7 +173,10 @@ def test_timeout_alarm_restores_signal_handler(self, mock_safe_urlopen): mock_safe_urlopen.return_value = mock_response app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) send_and_save_webhook_request(self.sentry_app, app_platform_event) @@ -187,7 +203,10 @@ def test_alarm_cancelled_after_successful_webhook(self, mock_safe_urlopen): mock_safe_urlopen.return_value = mock_response app_platform_event = AppPlatformEvent( - resource="issue", action="created", install=self.install, data={"test": "data"} + resource=SentryAppResourceType.ISSUE, + action=IssueActionType.CREATED, + install=self.install, + data={"test": "data"}, ) send_and_save_webhook_request(self.sentry_app, app_platform_event) From cf8357b39d92fc7f4103100a65afafb24566d913 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 19 Feb 2026 13:32:15 -0800 Subject: [PATCH 4/6] move notifying to after the commit --- .../api/endpoints/installation_details.py | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_details.py b/src/sentry/sentry_apps/api/endpoints/installation_details.py index f2ada20c049c78..ce488d9b68f928 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_details.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_details.py @@ -45,19 +45,9 @@ def get(self, request: Request, installation) -> Response: def delete(self, request: Request, installation) -> Response: sentry_app_installation = SentryAppInstallation.objects.get(id=installation.id) - with transaction.atomic(using=router.db_for_write(SentryAppInstallation)): - try: - assert request.user.is_authenticated, ( - "User must be authenticated to delete installation" - ) - SentryAppInstallationNotifier( - sentry_app_installation=sentry_app_installation, - user=request.user, - action="deleted", - ).run() - # if the error is from a request exception, log the error and continue - except RequestException as exc: - sentry_sdk.capture_exception(exc) + db = router.db_for_write(SentryAppInstallation) + + with transaction.atomic(using=db): sentry_app_installation.update(status=SentryAppInstallationStatus.PENDING_DELETION) ScheduledDeletion.schedule(sentry_app_installation, days=0, actor=request.user) create_audit_entry( @@ -67,7 +57,22 @@ def delete(self, request: Request, installation) -> Response: event=audit_log.get_event_id("SENTRY_APP_UNINSTALL"), data={"sentry_app": sentry_app_installation.sentry_app.name}, ) - if request.user.is_authenticated: + + def notify_on_commit() -> None: + try: + assert request.user.is_authenticated, ( + "User must be authenticated to delete installation" + ) + SentryAppInstallationNotifier( + sentry_app_installation=sentry_app_installation, + user=request.user, + action="deleted", + ).run() + except RequestException as exc: + sentry_sdk.capture_exception(exc) + + transaction.on_commit(notify_on_commit, using=db) + analytics.record( SentryAppUninstalledEvent( user_id=request.user.id, From 1d1b9049055f82d0e4c1558b5ceb133c1bf48fd5 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 19 Feb 2026 13:45:14 -0800 Subject: [PATCH 5/6] add back the is_authenticated guard --- src/sentry/sentry_apps/api/endpoints/installation_details.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_details.py b/src/sentry/sentry_apps/api/endpoints/installation_details.py index ce488d9b68f928..8e06393a72836a 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_details.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_details.py @@ -72,7 +72,7 @@ def notify_on_commit() -> None: sentry_sdk.capture_exception(exc) transaction.on_commit(notify_on_commit, using=db) - + if request.user.is_authenticated: analytics.record( SentryAppUninstalledEvent( user_id=request.user.id, From 0efb7827b2299e64924c2eddcfb7dcf79374222c Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 19 Feb 2026 16:52:49 -0800 Subject: [PATCH 6/6] change WebhookTimeoutError from BaseException to Exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With Exception, ignore_unpublished_app_errors swallows timeouts for unpublished (dev/test) apps, which is the correct behavior — the lifecycle halt metric still fires before the re-raise. Tests updated to use published=True so the exception propagates for assertion. --- src/sentry/utils/sentry_apps/webhooks.py | 2 +- tests/sentry/utils/sentry_apps/test_webhook_timeout.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index c7627b45425353..f50e7e923f474a 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -39,7 +39,7 @@ T = TypeVar("T", bound=Mapping[str, Any]) -class WebhookTimeoutError(BaseException): +class WebhookTimeoutError(Exception): """This error represents a user set hard timeout for when a webhook request should've completed within X seconds """ diff --git a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py index 62bcfcbd427e2c..145c1a7145f08d 100644 --- a/tests/sentry/utils/sentry_apps/test_webhook_timeout.py +++ b/tests/sentry/utils/sentry_apps/test_webhook_timeout.py @@ -24,6 +24,7 @@ def setUp(self): name="TestApp", organization=self.organization, webhook_url="https://example.com/webhook", + published=True, ) self.install = self.create_sentry_app_installation( organization=self.organization, slug=self.sentry_app.slug