diff --git a/src/sentry/integrations/slack/integration.py b/src/sentry/integrations/slack/integration.py index 11b2772c5ff4..4e997d7bad93 100644 --- a/src/sentry/integrations/slack/integration.py +++ b/src/sentry/integrations/slack/integration.py @@ -27,6 +27,7 @@ from sentry.integrations.slack.sdk_client import SlackSdkClient from sentry.integrations.slack.tasks.link_slack_user_identities import link_slack_user_identities from sentry.integrations.slack.utils.constants import SlackScope +from sentry.integrations.slack.utils.errors import SLACK_SDK_MESSAGE_GONE_ERROR_CATEGORIES from sentry.integrations.types import IntegrationProviderSlug from sentry.notifications.platform.provider import ( IntegrationNotificationClient, @@ -214,7 +215,7 @@ def update_message( unfurl_media=False, ) except SlackApiError as e: - translate_slack_api_error(e) + translate_slack_api_error(e, not_found=SLACK_SDK_MESSAGE_GONE_ERROR_CATEGORIES) def has_scope(self, scope: SlackScope) -> bool: """Check whether this integration was granted the given OAuth scope. diff --git a/src/sentry/integrations/slack/metrics.py b/src/sentry/integrations/slack/metrics.py index 0d692c3c4047..840270d11350 100644 --- a/src/sentry/integrations/slack/metrics.py +++ b/src/sentry/integrations/slack/metrics.py @@ -1,13 +1,20 @@ # metrics constants +from collections.abc import Collection + from slack_sdk.errors import SlackApiError from sentry.integrations.slack.utils.errors import ( SLACK_SDK_HALT_ERROR_CATEGORIES, + SlackSdkErrorCategory, unpack_slack_api_error, ) from sentry.integrations.utils.metrics import EventLifecycle -from sentry.shared_integrations.exceptions import IntegrationConfigurationError, IntegrationError +from sentry.shared_integrations.exceptions import ( + IntegrationConfigurationError, + IntegrationError, + IntegrationResourceNotFoundError, +) # Utils SLACK_UTILS_GET_USER_LIST_SUCCESS_DATADOG_METRIC = "sentry.integrations.slack.utils.users.success" @@ -25,10 +32,16 @@ def record_lifecycle_termination_level(lifecycle: EventLifecycle, error: SlackAp lifecycle.record_failure(e) -def translate_slack_api_error(error: SlackApiError) -> None: +def translate_slack_api_error( + error: SlackApiError, not_found: Collection[SlackSdkErrorCategory] = () +) -> None: reason = unpack_slack_api_error(error) if reason is not None: - if reason in SLACK_SDK_HALT_ERROR_CATEGORIES: + if reason in not_found: + raise IntegrationResourceNotFoundError( + error_code=error.response.status_code, message=reason.message + ) from error + elif reason in SLACK_SDK_HALT_ERROR_CATEGORIES: raise IntegrationConfigurationError( error_code=error.response.status_code, message=reason.message ) from error diff --git a/src/sentry/integrations/slack/utils/errors.py b/src/sentry/integrations/slack/utils/errors.py index f07f0b248969..8769b21d5e51 100644 --- a/src/sentry/integrations/slack/utils/errors.py +++ b/src/sentry/integrations/slack/utils/errors.py @@ -39,6 +39,7 @@ class SlackSdkErrorCategory: INVALID_BLOCKS := SlackSdkErrorCategory("invalid_blocks"), INVALID_ATTACHMENTS := SlackSdkErrorCategory("invalid_attachments"), INVALID_AUTH := SlackSdkErrorCategory("invalid_auth"), + MESSAGE_NOT_FOUND := SlackSdkErrorCategory("message_not_found"), NON_JSON_RESPONSE := SlackSdkErrorCategory("non_json_response"), ) @@ -58,6 +59,13 @@ class SlackSdkErrorCategory: NON_JSON_RESPONSE, ) +# Errors that mean the message being edited no longer exists, so retrying can't succeed. +SLACK_SDK_MESSAGE_GONE_ERROR_CATEGORIES = ( + MESSAGE_NOT_FOUND, + CHANNEL_NOT_FOUND, + CHANNEL_ARCHIVED, +) + _CATEGORIES_BY_MESSAGE = {c.message: c for c in SLACK_SDK_ERROR_CATEGORIES} diff --git a/src/sentry/seer/entrypoints/metrics.py b/src/sentry/seer/entrypoints/metrics.py index 1c8f8e910a4e..87f8f4e7223a 100644 --- a/src/sentry/seer/entrypoints/metrics.py +++ b/src/sentry/seer/entrypoints/metrics.py @@ -59,6 +59,7 @@ def get_metric_tags(self) -> Mapping[str, str]: class SlackEntrypointInteractionType(StrEnum): SEND_THREAD_UPDATE = "send_thread_update" + EDIT_THREAD_MESSAGE = "edit_thread_message" PROCESS_THREAD_UPDATE = "process_thread_update" SCHEDULE_ALL_THREAD_UPDATES = "schedule_all_thread_updates" UPDATE_EXISTING_MESSAGE = "update_existing_message" diff --git a/src/sentry/seer/entrypoints/slack/entrypoint.py b/src/sentry/seer/entrypoints/slack/entrypoint.py index ed24d5c8ef98..4d816c880a8f 100644 --- a/src/sentry/seer/entrypoints/slack/entrypoint.py +++ b/src/sentry/seer/entrypoints/slack/entrypoint.py @@ -689,6 +689,18 @@ def on_agent_update( reply["pr_status_hash"] = _hash_pr_status(data.repo_pr_states) reply["missing_scope_settings_url"] = data.missing_scope_settings_url SlackSeerAgentReplyCache.set(run_id=run_id, key=key, reply=reply) + else: + # send_thread_update records why as a halt and doesn't raise, so this refresh isn't + # retried. The reply is posted on the run's next Seer completion hook, if there is one. + logger.warning( + "seer.entrypoint.slack.agent_update.reply_not_posted", + extra={ + "organization_id": organization_id, + "integration_id": integration_id, + "run_id": run_id, + "reply_key": key, + }, + ) replies = SlackSeerAgentReplyCache.get(run_id=run_id) answers = [answer for turn in state.get_turns() if (answer := turn.answer) is not None] @@ -701,6 +713,7 @@ def on_agent_update( None, ) repo_pr_states = _get_repo_pr_states(state) + edit_error: IntegrationError | None = None for answer in answers: posted = replies.get(answer.id) if posted is None or "pr_status_hash" not in posted: @@ -710,21 +723,28 @@ def on_agent_update( status_hash = _hash_pr_status(prs) if status_hash == posted["pr_status_hash"]: continue - edit_thread_message( - install=install, - channel_id=thread["channel_id"], - message_ts=posted["message_ts"], - data=SeerAgentResponse( - run_id=state.run_id, - organization_id=organization_id, - summary=answer.message.content or "", - repo_pr_states=prs, - missing_scope_settings_url=posted.get("missing_scope_settings_url"), - ), - ) + try: + edit_thread_message( + install=install, + channel_id=thread["channel_id"], + message_ts=posted["message_ts"], + data=SeerAgentResponse( + run_id=state.run_id, + organization_id=organization_id, + summary=answer.message.content or "", + repo_pr_states=prs, + missing_scope_settings_url=posted.get("missing_scope_settings_url"), + ), + ) + except IntegrationError as e: + # Keep editing the other answers, then raise so the refresh retries this one. + edit_error = edit_error or e + continue SlackSeerAgentReplyCache.set( run_id=run_id, key=answer.id, reply={**posted, "pr_status_hash": status_hash} ) + if edit_error: + raise edit_error def _get_repo_pr_states(state: SeerRunState) -> list[RepoPRState]: diff --git a/src/sentry/seer/entrypoints/slack/messaging.py b/src/sentry/seer/entrypoints/slack/messaging.py index f5c7948f2241..99d08fe71b6a 100644 --- a/src/sentry/seer/entrypoints/slack/messaging.py +++ b/src/sentry/seer/entrypoints/slack/messaging.py @@ -42,7 +42,11 @@ SlackSeerAgentMessageCache, SlackSeerAgentMessageCachePayload, ) -from sentry.shared_integrations.exceptions import IntegrationConfigurationError, IntegrationError +from sentry.shared_integrations.exceptions import ( + IntegrationConfigurationError, + IntegrationError, + IntegrationResourceNotFoundError, +) from sentry.silo.base import all_silo_function from sentry.tasks.base import instrumented_task from sentry.taskworker.namespaces import integrations_tasks @@ -148,13 +152,40 @@ def send_thread_update( def edit_thread_message( *, install: SlackIntegration, channel_id: str, message_ts: str, data: NotificationData ) -> None: - """Re-render a message we already posted from new data.""" - renderable, _ = NotificationService.render_template( - data=data, - template=template_registry.get(data.source)(), - provider=provider_registry.get(NotificationProviderKey.SLACK), - ) - install.update_message(channel_id=channel_id, message_ts=message_ts, renderable=renderable) + """ + Re-render a message we already posted from new data. + Returns once there's nothing left to do, including when the message is gone. Raises + IntegrationError otherwise, so the caller can retry. + """ + with SlackEntrypointEventLifecycleMetric( + interaction_type=SlackEntrypointInteractionType.EDIT_THREAD_MESSAGE, + integration_id=install.model.id, + organization_id=install.organization_id, + ).capture() as lifecycle: + lifecycle.add_extras( + {"channel_id": channel_id, "message_ts": message_ts, "data_source": data.source} + ) + renderable, _ = NotificationService.render_template( + data=data, + template=template_registry.get(data.source)(), + provider=provider_registry.get(NotificationProviderKey.SLACK), + ) + try: + install.update_message( + channel_id=channel_id, message_ts=message_ts, renderable=renderable + ) + except IntegrationResourceNotFoundError as e: + lifecycle.record_halt(halt_reason=e) + except TimeoutError as e: + lifecycle.record_failure(failure_reason=e) + raise IntegrationError("Slack request timed out") from e + except IntegrationConfigurationError as e: + # Halts like rate limits can clear, and the edit may carry the run's final PR status. + lifecycle.record_halt(halt_reason=e) + raise + except IntegrationError as e: + lifecycle.record_failure(failure_reason=e) + raise @instrumented_task( diff --git a/tests/sentry/seer/entrypoints/slack/test_slack.py b/tests/sentry/seer/entrypoints/slack/test_slack.py index 5820211315a9..c04cdf0fdf02 100644 --- a/tests/sentry/seer/entrypoints/slack/test_slack.py +++ b/tests/sentry/seer/entrypoints/slack/test_slack.py @@ -45,7 +45,11 @@ SeerOperatorInvestigationCache, ) from sentry.seer.entrypoints.operator import SeerAutofixOperator, process_autofix_updates -from sentry.seer.entrypoints.slack.cache import SlackSeerAgentMessageCache +from sentry.seer.entrypoints.slack.cache import ( + SlackSeerAgentMessageCache, + SlackSeerAgentReply, + SlackSeerAgentReplyCache, +) from sentry.seer.entrypoints.slack.entrypoint import ( EntrypointSetupError, SlackAgentCachePayload, @@ -67,7 +71,11 @@ from sentry.seer.entrypoints.types import SeerEntrypointKey from sentry.seer.models import SeerAutomationHandoffConfiguration, SeerProjectPreference from sentry.sentry_apps.event_types import SentryAppEventType -from sentry.shared_integrations.exceptions import IntegrationError +from sentry.shared_integrations.exceptions import ( + IntegrationConfigurationError, + IntegrationError, + IntegrationResourceNotFoundError, +) from sentry.testutils.asserts import assert_halt_metric from sentry.testutils.cases import TestCase from sentry.testutils.helpers.options import override_options @@ -1261,6 +1269,62 @@ def blocks(summary: str, *footers: str) -> list[dict[str, Any]]: ), ] + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @patch("slack_sdk.web.client.WebClient.chat_update") + def test_on_agent_update_retries_failed_edits_and_skips_deleted_replies( + self, update: Mock, mock_record: Mock + ) -> None: + state = _agent_state("First answer.") + state.blocks += [ + MemoryBlock( + id="b3", + message=Message(role="user", content="Open a PR."), + timestamp="2024-01-01T00:00:02Z", + ), + MemoryBlock( + id="b4", + message=Message(role="assistant", content="Second answer."), + timestamp="2024-01-01T00:00:03Z", + ), + ] + state.repo_pr_states["acme/web"] = RepoPRState( + repo_name="acme/web", pr_creation_status="creating" + ) + # Both answers were posted before the PR status changed, so both footers need an edit. + for key, ts in (("b2", "1.1"), ("b4", "2.2")): + SlackSeerAgentReplyCache.set( + run_id=state.run_id, + key=key, + reply=SlackSeerAgentReply(message_ts=ts, pr_status_hash="stale"), + ) + update.side_effect = [ + TypedSlackApiError("error", _slack_response({"ok": False, "error": "ratelimited"})), + TypedSlackApiError( + "error", _slack_response({"ok": False, "error": "message_not_found"}) + ), + _slack_response({"ok": True}), + ] + cache_payload = self._get_entrypoint().create_agent_cache_payload() + + def refresh() -> None: + SlackAgentEntrypoint.on_agent_update( + cache_payload=cache_payload, run_id=state.run_id, state=state + ) + + # The rate-limited edit doesn't block the next one, and raises so the refresh task retries. + with pytest.raises(IntegrationError): + refresh() + # The retry edits only the rate-limited answer; the deleted one is never edited again. + refresh() + refresh() + + assert [call.kwargs["ts"] for call in update.call_args_list] == ["1.1", "2.2", "1.1"] + assert [ + type(call.args[1]) + for call in mock_record.mock_calls + if call.args[0] == EventLifecycleOutcome.HALTED + ] == [IntegrationConfigurationError, IntegrationResourceNotFoundError] + def _slack_response(data: dict[str, Any]) -> SlackResponse: return SlackResponse(