From 28178c06bf88b6d26b8322b82fe14bdefa71d84d Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 28 May 2026 16:02:18 -0700 Subject: [PATCH 1/3] ref(webhooks): Add feature-flagged dual-path shim to PluginActionHandler Mirror the WebhookActionHandler dual-path pattern so PLUGIN actions also route through the new task-based legacy webhook service when the existing legacy-webhook feature flags are enabled. Without flags, behavior is unchanged (old synchronous path only). This ensures projects using the PLUGIN action type (from dual-written 'notify all legacy integrations' rules) benefit from the new service's retry logic, metrics, and feature-flagged control. Co-Authored-By: Claude Opus 4 --- .../action_handler_registry/plugin_handler.py | 22 ++- .../test_plugin_handler.py | 134 ++++++++++++++++++ 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py diff --git a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py index dcd00cdf309490..bbe9f8bba19af7 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py @@ -1,10 +1,16 @@ +import logging from typing import override +from sentry import features from sentry.notifications.notification_action.utils import execute_via_group_type_registry +from sentry.sentry_apps.services.legacy_webhook.service import send_legacy_webhooks_for_invocation +from sentry.services.eventstore.models import GroupEvent from sentry.workflow_engine.models import Action from sentry.workflow_engine.registry import action_handler_registry from sentry.workflow_engine.types import ActionHandler, ActionInvocation, ConfigTransformer +logger = logging.getLogger(__name__) + @action_handler_registry.register(Action.Type.PLUGIN) class PluginActionHandler(ActionHandler): @@ -36,4 +42,18 @@ def get_config_transformer() -> ConfigTransformer | None: @staticmethod @override def execute(invocation: ActionInvocation) -> None: - execute_via_group_type_registry(invocation) + organization = invocation.detector.project.organization + new_path = features.has("organizations:legacy-webhook-new-path", organization) + disable_old = features.has("organizations:legacy-webhook-disable-old-path", organization) + + if not disable_old: + try: + execute_via_group_type_registry(invocation) + except Exception: + logger.exception( + "plugin_action_handler.old_path_error", + extra={"invocation": invocation}, + ) + + if new_path and isinstance(invocation.event_data.event, GroupEvent): + send_legacy_webhooks_for_invocation(invocation) diff --git a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py new file mode 100644 index 00000000000000..05ef4c267ea4af --- /dev/null +++ b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py @@ -0,0 +1,134 @@ +import uuid +from unittest import mock + +import responses + +from sentry.models.activity import Activity +from sentry.models.options.project_option import ProjectOption +from sentry.notifications.notification_action.action_handler_registry.plugin_handler import ( + PluginActionHandler, +) +from sentry.plugins.base import plugins +from sentry.types.activity import ActivityType +from sentry.utils import json +from sentry.workflow_engine.models import Action +from sentry.workflow_engine.types import ActionInvocation, WorkflowEventData +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest + + +class TestPluginActionHandlerExecute(BaseWorkflowTest): + def setUp(self) -> None: + super().setUp() + self.detector = self.create_detector(project=self.project) + self.workflow = self.create_workflow(environment=self.environment) + self.action = self.create_action( + type=Action.Type.PLUGIN, + ) + self.group, self.event, self.group_event = self.create_group_event() + self.event_data = WorkflowEventData( + event=self.group_event, workflow_env=self.environment, group=self.group + ) + self.invocation = ActionInvocation( + event_data=self.event_data, + action=self.action, + detector=self.detector, + notification_uuid=str(uuid.uuid4()), + workflow_id=self.workflow.id, + ) + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + @responses.activate + def test_default_no_flags_fires_old_path_only(self) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 1 + + @responses.activate + def test_new_path_fires_both_paths(self) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(), self.feature("organizations:legacy-webhook-new-path"): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 2 + + @responses.activate + def test_new_path_disable_old_fires_new_only(self) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with ( + self.tasks(), + self.feature( + { + "organizations:legacy-webhook-new-path": True, + "organizations:legacy-webhook-disable-old-path": True, + } + ), + ): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 1 + body = json.loads(responses.calls[0].request.body) + assert body["id"] == str(self.group.id) + + @responses.activate + def test_disable_old_without_new_path_fires_nothing(self) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(), self.feature("organizations:legacy-webhook-disable-old-path"): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 0 + + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation" + ) + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry" + ) + def test_non_group_event_skips_new_path_but_old_path_still_runs( + self, mock_old_path: mock.MagicMock, mock_new_path: mock.MagicMock + ) -> None: + activity = Activity.objects.create( + project=self.project, + group=self.group, + type=ActivityType.SET_RESOLVED.value, + ) + invocation = ActionInvocation( + event_data=WorkflowEventData( + event=activity, workflow_env=self.environment, group=self.group + ), + action=self.action, + detector=self.detector, + notification_uuid=str(uuid.uuid4()), + workflow_id=self.workflow.id, + ) + + with self.feature("organizations:legacy-webhook-new-path"): + PluginActionHandler.execute(invocation) + + mock_new_path.assert_not_called() + mock_old_path.assert_called_once_with(invocation) + + @responses.activate + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry", + side_effect=Exception("legacy path error"), + ) + def test_old_path_exception_does_not_block_new_path( + self, mock_old_path: mock.MagicMock + ) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(), self.feature("organizations:legacy-webhook-new-path"): + PluginActionHandler.execute(self.invocation) + + mock_old_path.assert_called_once() + assert len(responses.calls) == 1 + body = json.loads(responses.calls[0].request.body) + assert body["id"] == str(self.group.id) From 30fa9ea2eccca56290540b266a332155f07e88f1 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 28 May 2026 19:25:57 -0700 Subject: [PATCH 2/3] ref(webhooks): Skip webhooks plugin in old path instead of disabling it The old path (execute_via_group_type_registry) notifies all legacy NotificationPlugins, not just webhooks. Disabling it entirely would break Slack, PagerDuty, OpsGenie, and other plugins still in use. Instead, always run the old path but skip the webhooks plugin in NotifyEventAction.get_plugins() when the disable-old-path flag is on. The new task-based service handles webhook delivery exclusively. Co-Authored-By: Claude --- .../action_handler_registry/plugin_handler.py | 18 +++--- src/sentry/rules/actions/notify_event.py | 7 +++ .../test_plugin_handler.py | 58 +++++++++---------- .../sentry/rules/actions/test_notify_event.py | 24 ++++++++ 4 files changed, 68 insertions(+), 39 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py index bbe9f8bba19af7..e5b59f83376354 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py @@ -44,16 +44,14 @@ def get_config_transformer() -> ConfigTransformer | None: def execute(invocation: ActionInvocation) -> None: organization = invocation.detector.project.organization new_path = features.has("organizations:legacy-webhook-new-path", organization) - disable_old = features.has("organizations:legacy-webhook-disable-old-path", organization) - - if not disable_old: - try: - execute_via_group_type_registry(invocation) - except Exception: - logger.exception( - "plugin_action_handler.old_path_error", - extra={"invocation": invocation}, - ) + + try: + execute_via_group_type_registry(invocation) + except Exception: + logger.exception( + "plugin_action_handler.old_path_error", + extra={"invocation": invocation}, + ) if new_path and isinstance(invocation.event_data.event, GroupEvent): send_legacy_webhooks_for_invocation(invocation) diff --git a/src/sentry/rules/actions/notify_event.py b/src/sentry/rules/actions/notify_event.py index 0a73015ea99796..d6698b0d9c9e85 100644 --- a/src/sentry/rules/actions/notify_event.py +++ b/src/sentry/rules/actions/notify_event.py @@ -1,5 +1,6 @@ from collections.abc import Generator, Sequence +from sentry import features from sentry.plugins.base import plugins from sentry.rules.actions.base import EventAction from sentry.rules.actions.services import LegacyPluginService @@ -19,10 +20,16 @@ class NotifyEventAction(EventAction): def get_plugins(self) -> Sequence[LegacyPluginService]: from sentry.plugins.bases.notify import NotificationPlugin + skip_webhooks = features.has( + "organizations:legacy-webhook-disable-old-path", self.project.organization + ) + results = [] for plugin in plugins.for_project(self.project, version=1): if not isinstance(plugin, NotificationPlugin): continue + if skip_webhooks and plugin.slug == "webhooks": + continue results.append(LegacyPluginService(plugin)) return results diff --git a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py index 05ef4c267ea4af..804b6e0c1de033 100644 --- a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py +++ b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py @@ -9,6 +9,7 @@ PluginActionHandler, ) from sentry.plugins.base import plugins +from sentry.testutils.helpers.features import with_feature from sentry.types.activity import ActivityType from sentry.utils import json from sentry.workflow_engine.models import Action @@ -49,42 +50,41 @@ def test_default_no_flags_fires_old_path_only(self) -> None: assert len(responses.calls) == 1 @responses.activate - def test_new_path_fires_both_paths(self) -> None: - responses.add(responses.POST, "http://example.com/hook") - - with self.tasks(), self.feature("organizations:legacy-webhook-new-path"): - PluginActionHandler.execute(self.invocation) - - assert len(responses.calls) == 2 - - @responses.activate - def test_new_path_disable_old_fires_new_only(self) -> None: + @with_feature( + { + "organizations:legacy-webhook-new-path": True, + "organizations:legacy-webhook-disable-old-path": True, + } + ) + def test_new_path_skips_webhooks_in_old_path(self) -> None: + """When new path is on and old path disabled, webhooks are sent via the + new task-based service and skipped in the old path.""" responses.add(responses.POST, "http://example.com/hook") - with ( - self.tasks(), - self.feature( - { - "organizations:legacy-webhook-new-path": True, - "organizations:legacy-webhook-disable-old-path": True, - } - ), - ): + with self.tasks(): PluginActionHandler.execute(self.invocation) assert len(responses.calls) == 1 body = json.loads(responses.calls[0].request.body) assert body["id"] == str(self.group.id) - @responses.activate - def test_disable_old_without_new_path_fires_nothing(self) -> None: - responses.add(responses.POST, "http://example.com/hook") - - with self.tasks(), self.feature("organizations:legacy-webhook-disable-old-path"): - PluginActionHandler.execute(self.invocation) + @with_feature("organizations:legacy-webhook-new-path") + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation" + ) + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry" + ) + def test_old_path_always_runs( + self, mock_old_path: mock.MagicMock, mock_new_path: mock.MagicMock + ) -> None: + """Old path runs regardless of flags to keep non-webhook plugins firing.""" + PluginActionHandler.execute(self.invocation) - assert len(responses.calls) == 0 + mock_old_path.assert_called_once_with(self.invocation) + mock_new_path.assert_called_once_with(self.invocation) + @with_feature("organizations:legacy-webhook-new-path") @mock.patch( "sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation" ) @@ -109,13 +109,13 @@ def test_non_group_event_skips_new_path_but_old_path_still_runs( workflow_id=self.workflow.id, ) - with self.feature("organizations:legacy-webhook-new-path"): - PluginActionHandler.execute(invocation) + PluginActionHandler.execute(invocation) mock_new_path.assert_not_called() mock_old_path.assert_called_once_with(invocation) @responses.activate + @with_feature("organizations:legacy-webhook-new-path") @mock.patch( "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry", side_effect=Exception("legacy path error"), @@ -125,7 +125,7 @@ def test_old_path_exception_does_not_block_new_path( ) -> None: responses.add(responses.POST, "http://example.com/hook") - with self.tasks(), self.feature("organizations:legacy-webhook-new-path"): + with self.tasks(): PluginActionHandler.execute(self.invocation) mock_old_path.assert_called_once() diff --git a/tests/sentry/rules/actions/test_notify_event.py b/tests/sentry/rules/actions/test_notify_event.py index 2403c2de7e5aff..6c558c1b7888e0 100644 --- a/tests/sentry/rules/actions/test_notify_event.py +++ b/tests/sentry/rules/actions/test_notify_event.py @@ -1,8 +1,11 @@ from unittest.mock import MagicMock +from sentry.models.options.project_option import ProjectOption +from sentry.plugins.base import plugins from sentry.rules.actions.notify_event import NotifyEventAction from sentry.rules.actions.services import LegacyPluginService from sentry.testutils.cases import RuleTestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.skips import requires_snuba pytestmark = [requires_snuba] @@ -23,3 +26,24 @@ def test_applies_correctly(self) -> None: assert len(results) == 1 assert plugin.should_notify.call_count == 1 assert results[0].callback is plugin.rule_notify + + def test_get_plugins_includes_webhooks_by_default(self) -> None: + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + rule = self.get_rule() + result_slugs = [p.service.slug for p in rule.get_plugins()] + + assert "webhooks" in result_slugs + + @with_feature("organizations:legacy-webhook-disable-old-path") + def test_get_plugins_skips_webhooks_when_old_path_disabled(self) -> None: + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + rule = self.get_rule() + result_slugs = [p.service.slug for p in rule.get_plugins()] + + assert "webhooks" not in result_slugs From b8410959cb1038af4352d44714c58ddd10f95124 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 29 May 2026 11:19:12 -0700 Subject: [PATCH 3/3] ref(webhooks): Gate on GroupEvent early and add new path error handling Move the GroupEvent isinstance check to the top of execute() as an early return since both paths require it. Add try/except error handling around the new path for logging parity with the old path. Co-Authored-By: Claude --- .../action_handler_registry/plugin_handler.py | 13 +++++++++++-- .../action_handler_registry/test_plugin_handler.py | 4 ++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py index e5b59f83376354..4e2bf77c6f89f3 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py @@ -42,6 +42,9 @@ def get_config_transformer() -> ConfigTransformer | None: @staticmethod @override def execute(invocation: ActionInvocation) -> None: + if not isinstance(invocation.event_data.event, GroupEvent): + return + organization = invocation.detector.project.organization new_path = features.has("organizations:legacy-webhook-new-path", organization) @@ -53,5 +56,11 @@ def execute(invocation: ActionInvocation) -> None: extra={"invocation": invocation}, ) - if new_path and isinstance(invocation.event_data.event, GroupEvent): - send_legacy_webhooks_for_invocation(invocation) + if new_path: + try: + send_legacy_webhooks_for_invocation(invocation) + except Exception: + logger.exception( + "plugin_action_handler.new_path_error", + extra={"invocation": invocation}, + ) diff --git a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py index 804b6e0c1de033..36e78b52612a5d 100644 --- a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py +++ b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py @@ -91,7 +91,7 @@ def test_old_path_always_runs( @mock.patch( "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry" ) - def test_non_group_event_skips_new_path_but_old_path_still_runs( + def test_non_group_event_skips_both_paths( self, mock_old_path: mock.MagicMock, mock_new_path: mock.MagicMock ) -> None: activity = Activity.objects.create( @@ -111,8 +111,8 @@ def test_non_group_event_skips_new_path_but_old_path_still_runs( PluginActionHandler.execute(invocation) + mock_old_path.assert_not_called() mock_new_path.assert_not_called() - mock_old_path.assert_called_once_with(invocation) @responses.activate @with_feature("organizations:legacy-webhook-new-path")