msyavuz commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3675274049
##########
superset/daos/report.py:
##########
@@ -285,11 +285,34 @@ def update(
@staticmethod
def find_active() -> list[ReportSchedule]:
"""
- Find all active reports.
+ Find all active reports. Excludes reports that are currently retrying
+ unless their retry window has gone stale (retry task crashed or was
Review Comment:
Excluding RETRYING from `find_active()` also drops the schedule from its
normal cron windows for the whole retry chain, which is a behavior change to a
widely-used DAO method that isn't in the PR description — and it makes
`_is_retry_window_stale()` largely unreachable, since a new window can no
longer fire mid-retry.
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
+ base: int = app.config.get("ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS",
60)
+ cap: int = app.config.get("ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS",
3600)
+ return min(base * (2**attempt), cap)
+
+ def _is_retry_window_stale(self) -> bool:
+ """
+ Return True if a new crontab window fired while the previous one was
+ still retrying. When stale the retry counter must be reset so the new
+ window gets a fresh retry budget.
+ """
+ anchor = self._report_schedule.retry_scheduled_dttm
+ return anchor is not None and anchor != self._scheduled_dttm
Review Comment:
`retry_scheduled_dttm` is a naive column but `_scheduled_dttm` is tz-aware
in production (`cron_schedule_window` yields naive UTC, Celery's
`maybe_make_aware` makes the eta aware, and `scheduler.py` now parses it back
with `fromisoformat`; `ExecuteNow` passes an aware `datetime.now(tz=utc)` too)
— so the anchor reloads naive, this is always unequal, the counter resets every
attempt, and the report retries forever without ever reaching ERROR or sending
a notification. Suggest normalizing to naive UTC once in `scheduler.py` before
it reaches the command.
##########
superset/commands/report/execute.py:
##########
@@ -1403,6 +1564,8 @@ def next(self) -> None:
warning_message = (
";".join(self._filter_warnings) if self._filter_warnings else
None
)
+ # Clear any retry state from previous failed attempts in this
window.
+ self._reset_retry_counter()
Review Comment:
`ReportSuccessState` only got the counter reset — its `except` branch still
goes straight to ERROR with no retry logic, so a report whose last run
succeeded is never retried on its first failure; retries only engage on the
second consecutive failure once `last_state` is already ERROR.
##########
superset/reports/notifications/email.py:
##########
@@ -161,7 +161,71 @@ def _error_template(self, text: str) -> str:
call_to_action=call_to_action,
)
+ def _retry_error_template(self, text: str) -> tuple[str, dict[str, bytes]]:
+ """HTML body for a per-retry-attempt failure notification."""
+ attempt = self._content.retry_attempt
+ max_attempts = self._content.retry_max_attempts
+ retries_remaining = (max_attempts or 0) - (attempt or 0)
+ # pylint: disable=no-member
+ safe_text = nh3.clean(text, tags=set(), attributes={})
+ call_to_action = self._get_call_to_action()
+
+ img_tags = ""
+ if self._content.screenshots:
Review Comment:
Dead branch — neither `send_retry_notification` nor
`send_final_failure_report` sets `screenshots`, so `img_tags` is always empty
here.
##########
superset/reports/schemas.py:
##########
@@ -441,6 +485,34 @@ class ReportSchedulePutSchema(Schema):
required=False,
dump_default=None,
)
+ retry_on_failure = fields.Boolean(
+ metadata={"description": _("Enable automatic retries on report
failure")},
+ required=False,
+ )
+ retry_max_attempts = fields.Integer(
+ metadata={
+ "description": _("Maximum number of retry attempts (1–10)"),
+ "example": 3,
+ },
+ required=False,
+ validate=[Range(min=1, max=10, error=_("Must be between 1 and 10"))],
+ )
+ send_failed_reports = fields.Boolean(
Review Comment:
`ReportSchedulePutSchema` has no `validate_retry_config`, so a PUT can
persist `send_failed_reports=true` with `retry_on_failure=false` while the
equivalent POST 400s.
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2730,6 +2735,115 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
</>
),
},
+ ...(isReport
+ ? [
+ {
+ key: 'error-handling',
+ label: (
+ <CollapseLabelInModal
+ title={t('Error handling')}
+ subtitle={t(
+ 'Configure retry behavior on delivery failure.',
+ )}
+ testId="error-handling-panel"
+ />
+ ),
+ children: (
+ <div className="header-section">
+ <StyledSwitchContainer
+ css={css`
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ `}
+ >
+ <Switch
+ checked={!!currentAlert?.retry_on_failure}
+ onChange={(checked: boolean) =>
+ updateAlertState('retry_on_failure', checked)
+ }
+ />
+ <div className="switch-label">
+ {t('Enable Retries')}
+ </div>
+ <InfoTooltip
+ tooltip={t(
+ 'Automatically retry sending the report when
delivery fails.',
+ )}
+ />
+ </StyledSwitchContainer>
+ {currentAlert?.retry_on_failure && (
+ <>
+ <ModalFormField label={t('Maximum Retry
Attempts')}>
+ <InputNumber
+ min={1}
+ max={10}
+ value={currentAlert?.retry_max_attempts ?? 3}
+ onChange={(value: number | null) =>
+ updateAlertState(
+ 'retry_max_attempts',
+ value ?? 3,
+ )
+ }
+ />
+ </ModalFormField>
+ <StyledSwitchContainer
+ css={css`
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ `}
+ >
+ <Switch
+ checked={!!currentAlert?.send_failed_reports}
+ onChange={(checked: boolean) =>
+ updateAlertState(
+ 'send_failed_reports',
+ checked,
+ )
+ }
+ />
+ <div className="switch-label">
+ {t('Send Failed Reports')}
+ </div>
+ <InfoTooltip
+ tooltip={t(
+ 'By default, recipients only receive reports
when all charts successfully load. ' +
Review Comment:
This copy promises the report is delivered anyway, but
`send_final_failure_report` builds `NotificationContent(text=...)` with no
screenshots, so `_get_content` takes the error-template branch and recipients
just get a plain "All Retries Exhausted" error email.
##########
superset-frontend/src/features/reports/ReportModal/index.tsx:
##########
@@ -191,6 +196,11 @@ function ReportModal({
crontab: currentReport.crontab,
report_format: currentReport.report_format || defaultNotificationFormat,
timezone: currentReport.timezone,
+ retry_on_failure: currentReport.retry_on_failure ?? false,
+ retry_max_attempts: currentReport.retry_max_attempts ?? 3,
+ send_failed_reports: currentReport.send_failed_reports ?? false,
Review Comment:
Unchecking "Enable Retries" hides the dependent fields but doesn't clear
`send_failed_reports`, so the payload trips `validate_retry_config` and 400s —
same path in `AlertReportModal`.
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
+ base: int = app.config.get("ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS",
60)
+ cap: int = app.config.get("ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS",
3600)
+ return min(base * (2**attempt), cap)
+
+ def _is_retry_window_stale(self) -> bool:
+ """
+ Return True if a new crontab window fired while the previous one was
+ still retrying. When stale the retry counter must be reset so the new
+ window gets a fresh retry budget.
+ """
+ anchor = self._report_schedule.retry_scheduled_dttm
+ return anchor is not None and anchor != self._scheduled_dttm
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new
count."""
+ self._report_schedule.retry_attempt += 1
+ self._report_schedule.retry_scheduled_dttm = self._scheduled_dttm
+ return self._report_schedule.retry_attempt
+
+ def _reset_retry_counter(self) -> None:
+ """Reset retry state after a terminal outcome."""
+ self._report_schedule.retry_attempt = 0
+ self._report_schedule.retry_scheduled_dttm = None
+
+ def _schedule_retry(self, delay_seconds: int) -> None:
+ """Re-queue the execute task with the given countdown (seconds)."""
+ # Lazy import to avoid a circular dependency between execute.py and
scheduler.py
+ from superset.tasks.scheduler import execute as execute_task # noqa:
PLC0415
+
+ # Pass the original crontab-trigger timestamp so the retry task
+ # shares the same window identity. This lets _is_retry_window_stale()
+ # detect when a *new* crontab window fires while retries are in-flight.
+ execute_task.apply_async(
Review Comment:
This `apply_async` runs inside `ReportScheduleStateMachine.run()`'s
`@transaction()` — if the commit rolls back, does the retry fire against the
un-incremented counter?
##########
superset-frontend/src/features/alerts/types.ts:
##########
@@ -151,6 +151,11 @@ export type AlertObject = {
};
validator_type?: string;
working_timeout?: number;
+ retry_on_failure?: boolean;
Review Comment:
`AlertState` (line 167) never got `Retrying`, so `AlertStatusIcon` falls to
its `default` branch and a retrying report renders with the calendar icon and
the "not yet run" label.
##########
superset/daos/report.py:
##########
@@ -285,11 +285,34 @@ def update(
@staticmethod
def find_active() -> list[ReportSchedule]:
"""
- Find all active reports.
+ Find all active reports. Excludes reports that are currently retrying
+ unless their retry window has gone stale (retry task crashed or was
+ lost), in which case they are re-included so the scheduler can
+ recover them on the next crontab tick.
"""
+ from flask import current_app # noqa: PLC0415
+
+ # A retry is considered stale if retry_scheduled_dttm is older than
+ # the maximum possible delay plus a generous buffer.
+ max_delay: int = current_app.config.get(
+ "ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS", 3600
+ )
+ stale_cutoff = datetime.now(tz=timezone.utc).replace(tzinfo=None) -
timedelta(
+ seconds=max_delay * 2
Review Comment:
The 2×max-delay cutoff (2h default) is shorter than the real backoff budget
— 10 attempts of `min(60*2^n, 3600)` sums to ~6.6h — so a healthy retry chain
gets "rescued" past 2h and the next cron tick fires an execution concurrent
with the still-queued retry. Worth deriving the cutoff from
`retry_max_attempts` instead.
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
Review Comment:
`ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS` /
`ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS` aren't declared in
`superset/config.py`, so the defaults are duplicated here and in
`daos/report.py` and the keys are undiscoverable to operators.
##########
superset/reports/types.py:
##########
@@ -19,5 +19,5 @@
from superset.dashboards.permalink.types import DashboardPermalinkState
-class ReportScheduleExtra(TypedDict):
+class ReportScheduleExtra(TypedDict, total=False):
Review Comment:
`total=False` looks unrelated to retries — what needs it?
##########
superset/migrations/versions/2026-07-24_00-00_f3a8c1d2e9b7_add_report_retry_state_columns.py:
##########
@@ -0,0 +1,84 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_report_retry_columns
+
+Revision ID: f3a8c1d2e9b7
+Revises: e5f6a7b8c9d0
+Create Date: 2026-07-24 00:00:00.000000
+
+"""
+
+import logging
+
Review Comment:
Docstring says `Revises: e5f6a7b8c9d0` but `down_revision` is
`d3b9a1f6c204`; the filename date also predates its parent's (2026-07-27).
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -2667,3 +2667,428 @@ def test__send_with_server_errors(notification_mock,
logger_mock):
logger_mock.warning.assert_called_with(
"SupersetError(message='',
error_type=<SupersetErrorType.REPORT_NOTIFICATION_ERROR:
'REPORT_NOTIFICATION_ERROR'>, level=<ErrorLevel.ERROR: 'error'>, extra=None)"
# noqa: E501
)
+
+
+# ---------------------------------------------------------------------------
+# Retry tests
+# ---------------------------------------------------------------------------
+
+
[email protected]("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
Review Comment:
`_schedule_retry` is mocked in every retry test and the anchors are
hand-built naive datetimes with microseconds truncated, so nothing covers the
`scheduled_dttm` round-trip the whole termination logic depends on — which is
why the naive/aware bug above is green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
