From 77620deb4fc1cbb2d3ac3e13337fcc5922c0d873 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Fri, 24 Jul 2026 22:56:17 -0700 Subject: [PATCH] fix(reports): give each Slack channel a retry budget --- UPDATING.md | 2 +- .../configuration/alerts-reports.mdx | 27 ++++++++++ superset/config.py | 8 +-- .../reports/notifications/slack_transport.py | 14 +++--- superset/reports/notifications/slackv2.py | 21 ++------ .../reports/notifications/slack_tests.py | 50 +++++++++++++++---- 6 files changed, 81 insertions(+), 41 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 9dea2793fc4..cb70cd8dda1 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -586,7 +586,7 @@ With the flag enabled: `DELETE /api/v1/chart/` no longer hard-deletes the ch **Importer behavior:** importing a chart YAML whose UUID matches an existing **soft-deleted** chart is treated as an implicit restore-with-update — **and this happens even when `overwrite` is not set**. This is a deliberate asymmetry with active rows: an active chart imported without `overwrite=true` is returned unchanged, but a soft-deleted UUID match is restored *and* has the upload's contents applied regardless of the `overwrite` argument, on the reasoning that re-importing a deleted chart's exact UUID is an explicit request to bring it back. The restore preserves the original PK and all out-of-archive references (`dashboard_slices` junctions, `report.chart_id`, tag rows). The operation is permission-gated: non-editors get `ImportFailedError`, and callers without `can_write` get `ImportFailedError` instead of silently receiving the soft-deleted row. -- [39914](https://github.com/apache/superset/pull/39914) makes `ALERT_REPORT_SLACK_V2` default to `True` and deprecates the legacy Slack v1 integration (`Slack` recipient type and retired `files.upload` API) for removal in the next major; [42089](https://github.com/apache/superset/pull/42089) makes v1 file-bearing sends fail before attempting the retired v1 upload with actionable Slack v2 scope guidance, normalizes duplicate targets, resolves canonical IDs before case-insensitive channel names, and preserves text-only delivery through v1 when the atomic name-to-ID migration cannot finish (for example, when a later page of the full channel listing fails). Successful text fallback remains a `SUCCESS` but records a warning in the report execution log and emits fallback metrics after delivery succeeds; that warning is also retained when another recipient failure ends the execution in `ERROR`. Missing or unresolvable channel targets are classified as operator-fixable client errors instead of system errors; transient listing and delivery failures remain system errors. Grant the Slack bot both `channels:read` and `groups:read` so existing private-channel `Slack` recipients can be auto-upgraded to `SlackV2` on their next send. Operators who disable the flag or whose bot lacks those scopes receive deprecation warnings while text-only legacy delivery remains available. Application retries cover transient failures in non-terminal Slack operations. Terminal `chat.postMessage` and `files.completeUploadExternal` writes use at-most-once semantics for ambiguous server, envelope, and transport failures, while explicit Slack API HTTP 429 rejections remain retryable. Slack API responses with exposed HTTP 429 headers honor `Retry-After` and `SLACK_API_RATE_LIMIT_RETRY_COUNT`, including header-bearing errors from the raw external-upload request. A raw-upload status result without headers fails immediately rather than synthesizing a retry delay. Raw external-upload 408/5xx responses use the general application backoff, outcome-unknown transport failures are not repeated, and permanent 4xx errors fail immediately. Each recipient send shares one application retry deadline across all channel/file operations and does not start another channel/file or upload phase after that deadline, including after Slack's `Retry-After` delay. The remaining budget is divided fairly among the remaining channels so one failing channel cannot prevent later channels from making an API attempt. The total deadline defaults to 150 seconds through `SLACK_SEND_RETRY_MAX_TIME`, can be increased for high-fan-out or large-file reports, and is always at least one second longer than `SLACK_API_TIMEOUT`. Slack v2 file uploads retry their URL-creation and raw-upload phases independently; the terminal completion phase is attempted once for ambiguous failures so a lost response cannot duplicate a delivered report. Each in-flight API phase uses a request timeout capped by the remaining application budget. A full channel-list refresh occurs only when the initial miss came from cache. Cache-backend failures are best-effort and do not replace successfully fetched Slack channel data. Successfully cached refreshes record a per-workspace cooldown, including on cache backends whose successful `set` returns `None`; disabled and session-backed metadata caches use uncached refreshes without cooldown writes, and concurrent workers may still refresh in parallel when transaction-safe coordination is unavailable. +- [39914](https://github.com/apache/superset/pull/39914) defaults `ALERT_REPORT_SLACK_V2` to `True` and deprecates legacy Slack v1; [42089](https://github.com/apache/superset/pull/42089) automatically upgrades resolvable v1 recipients, preserves text-only v1 delivery with execution warnings when migration cannot finish, and rejects retired v1 file uploads with actionable scope guidance. Grant bots `channels:read` and `groups:read` for public and private channel resolution. Slack delivery uses at-most-once terminal writes and an independent per-channel retry budget configured by `SLACK_SEND_RETRY_MAX_TIME`; see [Alerts and Reports](https://superset.apache.org/docs/configuration/alerts-reports#slack-delivery-timeouts-and-retries). ### Soft delete and restore for dashboards diff --git a/docs/admin_docs/configuration/alerts-reports.mdx b/docs/admin_docs/configuration/alerts-reports.mdx index bedc88ac4c9..df62976adcc 100644 --- a/docs/admin_docs/configuration/alerts-reports.mdx +++ b/docs/admin_docs/configuration/alerts-reports.mdx @@ -96,6 +96,33 @@ SLACK_TEAM_ID = "T01234567" This defaults to `None` and only needs to be set when using an org-scoped token; it is accepted but ignored for standard workspace-level tokens. +#### Slack delivery timeouts and retries + +Slack delivery uses a request timeout and an application retry budget: + +```python +# Timeout for one Slack API request, in seconds +SLACK_API_TIMEOUT = 30 + +# Retry budget for each destination channel across its files and upload phases +SLACK_SEND_RETRY_MAX_TIME = 150 + +# Number of explicit HTTP 429 responses retried using Slack's Retry-After value +SLACK_API_RATE_LIMIT_RETRY_COUNT = 2 +``` + +Each channel receives an independent `SLACK_SEND_RETRY_MAX_TIME` budget, so a +slow or unavailable channel does not reduce the time available to later +channels. The effective per-channel budget is at least one second longer than +`SLACK_API_TIMEOUT`. Increase it for large files or slow Slack workspaces. + +To avoid posting the same report twice, Superset does not replay terminal +`chat.postMessage` or `files.completeUploadExternal` operations after ambiguous +server or transport failures. Explicit Slack HTTP 429 responses remain +retryable. These delivery settings and semantics apply to Slack v2 reports and +legacy text-only Slack delivery, independently of the +`ALERT_REPORT_SLACK_V2` feature flag. + ### Webhook integration Superset can send alert and report notifications to any HTTP endpoint — useful for chat platforms, incident management tools, or custom automation. diff --git a/superset/config.py b/superset/config.py index 12d448b414a..1ef0fb59008 100644 --- a/superset/config.py +++ b/superset/config.py @@ -2439,10 +2439,10 @@ SLACK_API_RATE_LIMIT_RETRY_COUNT = 2 # patching code, consistent with the SMTP/CSV/screenshot timeouts. SLACK_API_TIMEOUT = 30 -# Total application retry budget (in seconds) for one Slack recipient across -# all of its channels, files, and upload phases. Increase this for high fan-out -# or large-file reports. The effective budget is always at least one second -# longer than SLACK_API_TIMEOUT so the per-request timeout remains usable. +# Application retry budget (in seconds) for each Slack channel across all files +# and upload phases. Increase this for slow channels or large-file reports. Each +# channel receives an independent budget, and the effective value is always at +# least one second longer than SLACK_API_TIMEOUT. SLACK_SEND_RETRY_MAX_TIME = 150 # The webdriver to use for generating reports when using Selenium (not Playwright). diff --git a/superset/reports/notifications/slack_transport.py b/superset/reports/notifications/slack_transport.py index 60fbe491244..3e8374ed884 100644 --- a/superset/reports/notifications/slack_transport.py +++ b/superset/reports/notifications/slack_transport.py @@ -46,7 +46,7 @@ _SlackApiResult = TypeVar("_SlackApiResult") _SLACK_RETRY_DEADLINE_MESSAGE = ( "Slack send retry deadline exceeded; increase SLACK_SEND_RETRY_MAX_TIME " - "for multi-channel or large-file reports" + "for slow-channel or large-file reports" ) @@ -70,7 +70,7 @@ _SLACK_CHANNEL_FAILURES = ( def _get_slack_send_retry_max_time() -> float: - """Return the effective total send budget in seconds.""" + """Return the effective per-channel send budget in seconds.""" configured_budget = float(app.config["SLACK_SEND_RETRY_MAX_TIME"]) request_timeout = float(app.config.get("SLACK_API_TIMEOUT", 30)) return max( @@ -237,13 +237,11 @@ def send_to_slack_channels( channels: list[str], send_to_channel: Callable[[str, float], None], ) -> None: - """Send within one shared deadline while reserving time for each channel.""" - retry_deadline = time.monotonic() + _get_slack_send_retry_max_time() + """Send to each channel with an independent application retry deadline.""" + retry_max_time = _get_slack_send_retry_max_time() failures: list[tuple[str, Exception]] = [] - for index, channel in enumerate(channels): - now = time.monotonic() - remaining_channels = len(channels) - index - channel_deadline = now + max(retry_deadline - now, 0) / remaining_channels + for channel in channels: + channel_deadline = time.monotonic() + retry_max_time try: send_to_channel(channel, channel_deadline) except _SLACK_CHANNEL_FAILURES as ex: diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py index cef660f4cd8..6ca4ad062f6 100644 --- a/superset/reports/notifications/slackv2.py +++ b/superset/reports/notifications/slackv2.py @@ -16,10 +16,8 @@ # under the License. import logging import time -from collections.abc import Sequence from email.message import Message -from io import IOBase -from typing import List, Union +from typing import List from urllib.error import HTTPError from flask import g @@ -60,29 +58,18 @@ from superset.utils.slack import ( logger = logging.getLogger(__name__) -def _read_upload_data(file: str | IOBase | bytes) -> bytes: - """Read a Slack upload input using the SDK's supported single-file forms.""" - if isinstance(file, str): - with open(file, "rb") as readable: - return readable.read() - if isinstance(file, bytes): - return file - data = file.read() - return data.encode() if isinstance(data, str) else data - - def _upload_file_to_slack( client: WebClient, *, channel: str, - file: str | IOBase | bytes, + file: bytes, initial_comment: str, title: str, filename: str, retry_deadline: float, ) -> None: """Upload one file without replaying completed phases during retries.""" - data = _read_upload_data(file) + data = file upload_url_response = call_slack_api_with_timeout( client, client.files_getUploadURLExternal, @@ -166,7 +153,7 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- def _get_inline_files( self, - ) -> tuple[Union[str, None], Sequence[Union[str, IOBase, bytes]]]: + ) -> tuple[str | None, list[bytes]]: if self._content.csv: return ("csv", [self._content.csv]) if self._content.xlsx: diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index b5cd9f87f6d..db45a613dfc 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -1825,8 +1825,8 @@ def test_call_slack_api_retries_ratelimited_code_without_http_429() -> None: assert method.call_count == 5 -def test_send_to_slack_channels_reserves_time_for_later_channels() -> None: - """One failing channel cannot consume every later channel's API attempt.""" +def test_send_to_slack_channels_gives_each_channel_a_full_budget() -> None: + """A failing channel cannot reduce the retry budget of later channels.""" clock = [0.0] starts: dict[str, float] = {} deadlines: dict[str, float] = {} @@ -1837,7 +1837,7 @@ def test_send_to_slack_channels_reserves_time_for_later_channels() -> None: first_method = MagicMock() def fail_first_channel() -> None: - clock[0] = 51.0 + clock[0] = 151.0 raise error first_method.side_effect = fail_first_channel @@ -1866,15 +1866,43 @@ def test_send_to_slack_channels_reserves_time_for_later_channels() -> None: methods["private-c"].assert_called_once_with() assert deadlines == pytest.approx( { - "private-a": 50.0, - "private-b": 100.5, + "private-a": 150.0, + "private-b": 301.0, + "private-c": 301.0, + } + ) + assert { + channel: deadlines[channel] - starts[channel] for channel in deadlines + } == pytest.approx( + { + "private-a": 150.0, + "private-b": 150.0, "private-c": 150.0, } ) - assert max(deadlines.values()) == 150.0 - assert deadlines["private-c"] - starts["private-c"] > ( - deadlines["private-b"] - starts["private-b"] + + +def test_send_to_slack_channels_preserves_timeout_for_high_fanout(mocker) -> None: + """Every channel starts with more budget than one configured request.""" + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + { + "SLACK_SEND_RETRY_MAX_TIME": 150, + "SLACK_API_TIMEOUT": 30, + }, ) + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + return_value=10.0, + ) + budgets: list[float] = [] + + def send(_channel: str, retry_deadline: float) -> None: + budgets.append(retry_deadline - 10.0) + + send_to_slack_channels([f"C{index}" for index in range(10)], send) + + assert budgets == [150.0] * 10 @pytest.mark.parametrize( @@ -1967,13 +1995,13 @@ def test_call_slack_api_without_explicit_deadline_is_still_bounded() -> None: (600, 300, 600), ], ) -def test_send_deadline_respects_total_budget_and_request_timeout( +def test_send_deadline_respects_channel_budget_and_request_timeout( mocker, configured_budget: int, request_timeout: int, expected_budget: int, ) -> None: - """The total budget is configurable and cannot neutralize request timeout.""" + """Each channel budget is configurable and cannot neutralize request timeout.""" mocker.patch.dict( "superset.reports.notifications.slack_transport.app.config", { @@ -1994,7 +2022,7 @@ def test_send_deadline_respects_total_budget_and_request_timeout( def test_deadline_error_names_operator_setting() -> None: - """Expired sends identify the config knob that controls the total budget.""" + """Expired sends identify the config knob that controls channel budgets.""" with ( patch( "superset.reports.notifications.slack_transport.time.monotonic",