You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "alexandrusoare (via GitHub)" <gi...@apache.org> on 2026/07/27 13:36:28 UTC
[PR] feat(reports): add retry on failure support for alerts and reports [superset]
alexandrusoare opened a new pull request, #42481:
URL: https://github.com/apache/superset/pull/42481
### SUMMARY
Adds the ability to automatically retry report and alert delivery when it fails. Previously, a failed report would immediately transition to an error state. Now, users can configure retry behavior per report/alert:
- **Enable Retries** — automatically retry delivery on failure
- **Max Retry Attempts** — how many times to retry before giving up (1–10)
- **Send Failed Reports** — optionally deliver the report to recipients even after all retries fail
- **Failure Notifications** — choose whether to notify owners and/or recipients on each failed attempt
The retry configuration is available in both the Alerts & Reports page modal and the dashboard/chart email report modal, under a new "Error handling" section.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3675056440
##########
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:
i think this stale check might never work the way it's meant to. `_scheduled_dttm` is tz-aware in prod (celery eta / the `datetime.now(tz=timezone.utc)` fallback), but `retry_scheduled_dttm` is a plain `DateTime` column so once it's saved and read back on the next retry it comes back naive. aware != naive is always true in python, so `_is_retry_window_stale()` returns true on every retry, resets the counter each time, and `retry_attempt` just bounces 0 -> 1 -> 0 forever. so it never hits `max_attempts`, never errors out, and never sends the retry emails (that branch needs `current_attempt > 0`). basically it retries forever at a fixed interval instead of giving up after N.
`find_active()` already deals with this same column by doing `.replace(tzinfo=None)` to match the naive storage, so probably just normalize both sides here too (or compare the iso string / a window token instead of raw datetimes). heads up that the tests pass naive `datetime.utcnow()` so this path doesn't really get exercised, a case with a tz-aware scheduled_dttm that saves + reloads the schedule would catch it.
##########
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(
+ (self._report_schedule.id, self._scheduled_dttm.isoformat()),
+ countdown=delay_seconds,
+ )
+
+ def send_retry_notification(
+ self, attempt: int, max_attempts: int, error_message: str
+ ) -> None:
+ """
+ Send a per-retry-attempt failure notification to the owners and/or recipients
+ selected via the retry_notify_owners / retry_notify_recipients flags.
+ """
+ recipients: list[ReportRecipients] = []
+
+ if self._report_schedule.retry_notify_owners:
+ recipients.extend(
+ [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=json.dumps({"target": s.user.email}),
+ )
+ for s in self._report_schedule.editors
+ if s.type == SubjectType.USER and s.user
+ ]
+ )
+
+ if self._report_schedule.retry_notify_recipients:
+ recipients.extend(self._report_schedule.recipients)
+
+ if not recipients:
+ return
+
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_attempt=attempt,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, recipients)
+
+ def send_final_failure_report(self, error_message: str) -> None:
Review Comment:
quick question on intent here. the description says `send_failed_reports` will "deliver the report to recipients even after all retries fail", but this builds a `NotificationContent` with just `text=error_message` and no screenshots/csv/embedded data, so recipients get a failure notice with the error rather than the actual report. if that's what you meant then all good and maybe just reword the option, but if the idea was to send the last-attempt report itself then this doesn't quite do that yet.
##########
superset/commands/report/execute.py:
##########
@@ -1224,6 +1321,70 @@ def next(self) -> None: # noqa: C901
if isinstance(first_ex, SupersetErrorsException):
error_message = ";".join([error.message for error in first_ex.errors])
+ # --- Retry logic ---
+ retry_on_failure: bool = self._report_schedule.retry_on_failure
+ max_attempts: int = self._report_schedule.retry_max_attempts
+
+ # If a new crontab window has fired since the first failure, reset the
+ # retry counter so this window gets a fresh budget.
+ if retry_on_failure and self._is_retry_window_stale():
+ self._reset_retry_counter()
+
+ current_attempt = self._report_schedule.retry_attempt
+
+ # If this execution was itself a retry (current_attempt > 0),
+ # send the retry-failure notification *after* the attempt ran so
+ # the email reflects what happened, not what is about to be
+ # scheduled. ("You will receive an update after each retry.")
+ if retry_on_failure and current_attempt > 0:
+ try:
+ self.send_retry_notification(
+ current_attempt, max_attempts, error_message
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Failed to send retry notification for report schedule "
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+
+ if retry_on_failure and current_attempt < max_attempts:
+ # Schedule another attempt and exit cleanly (don't re-raise).
+ self._increment_retry()
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.RETRYING, error_message=error_message
+ )
+ except ReportScheduleUnexpectedError as logging_ex:
+ logger.warning(
+ "Failed to log RETRYING state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
+ raise first_ex from logging_ex
+ self._schedule_retry(
Review Comment:
small one, the backoff skips its first step. this runs after `_increment_retry()`, so `retry_attempt` is already >= 1 and `_get_retry_delay` gives `base * 2**1` at minimum, the `base * 2**0` case never happens. so the first retry waits 120s instead of 60s with the defaults. if you want it to actually start at `base`, computing the delay before the increment (or with `attempt - 1`) would do it.
##########
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)
Review Comment:
`ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS` and `ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS` are read here (and in `daos/report.py`) but they're not declared in `config.py`, so operators can't really discover them. might be worth adding them there with the defaults and a one-line comment so they're documented alongside the other `ALERT_REPORTS_*` settings.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare merged PR #42481:
URL: https://github.com/apache/superset/pull/42481
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681671443
##########
superset/commands/report/execute.py:
##########
@@ -1224,6 +1321,70 @@ def next(self) -> None: # noqa: C901
if isinstance(first_ex, SupersetErrorsException):
error_message = ";".join([error.message for error in first_ex.errors])
+ # --- Retry logic ---
+ retry_on_failure: bool = self._report_schedule.retry_on_failure
+ max_attempts: int = self._report_schedule.retry_max_attempts
+
+ # If a new crontab window has fired since the first failure, reset the
+ # retry counter so this window gets a fresh budget.
+ if retry_on_failure and self._is_retry_window_stale():
+ self._reset_retry_counter()
+
+ current_attempt = self._report_schedule.retry_attempt
+
+ # If this execution was itself a retry (current_attempt > 0),
+ # send the retry-failure notification *after* the attempt ran so
+ # the email reflects what happened, not what is about to be
+ # scheduled. ("You will receive an update after each retry.")
+ if retry_on_failure and current_attempt > 0:
+ try:
+ self.send_retry_notification(
+ current_attempt, max_attempts, error_message
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Failed to send retry notification for report schedule "
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+
+ if retry_on_failure and current_attempt < max_attempts:
+ # Schedule another attempt and exit cleanly (don't re-raise).
+ self._increment_retry()
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.RETRYING, error_message=error_message
+ )
+ except ReportScheduleUnexpectedError as logging_ex:
+ logger.warning(
+ "Failed to log RETRYING state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
+ raise first_ex from logging_ex
+ self._schedule_retry(
Review Comment:
done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681897864
##########
superset/commands/report/execute.py:
##########
@@ -1195,10 +1377,26 @@ class ReportNotTriggeredErrorState(BaseReportState):
- Error
"""
- current_states = [ReportState.NOOP, ReportState.ERROR]
+ current_states = [ReportState.NOOP, ReportState.ERROR, ReportState.RETRYING]
initial = True
def next(self) -> None: # noqa: C901
+ # If retries from a previous crontab window are still in-flight and
+ # this is a new crontab trigger, skip — let the active retry chain
+ # finish. The stale-window check uses normalized (naive) datetimes
+ # so tz-aware vs naive mismatches don't cause false positives.
+ if (
+ self._report_schedule.last_state == ReportState.RETRYING
+ and self._is_retry_window_stale()
+ ):
+ logger.info(
+ "Skipping crontab execution for report %s — retries from a "
+ "previous window are still in-flight (execution %s)",
+ self._report_schedule.id,
+ self._execution_id,
+ )
+ return
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag skipping a crontab window while retries from a previous window are active; avoiding concurrent execution is intentional to prevent duplicate emails and race conditions.
**Applied to:**
- `superset/commands/report/execute.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681746249
##########
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)
Review Comment:
added them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681759004
##########
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:
added them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3682820377
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -78,6 +78,7 @@ const AlertStateLabel: Record<AlertState, string> = {
[AlertState.Error]: t('Error'),
[AlertState.Noop]: t('Not triggered'),
[AlertState.Grace]: t('On Grace'),
+ [AlertState.Retrying]: t('Retrying'),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent filter option coverage</b></div>
<div id="fix">
The `AlertStateLabel` map now includes `Retrying` (line 81), but the Status filter dropdown at lines 581-593 is missing the corresponding filter option. Users can see alerts in 'Retrying' state but cannot filter by this state, creating an inconsistent user experience compared to other states that have both labels and filter options.
</div>
</div>
<small><i>Code Review Run #165e40</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/features/alerts/components/AlertStatusIcon.tsx:
##########
@@ -39,6 +39,8 @@ function getStatusColor(status: string, theme: SupersetTheme) {
return theme.colorTextSecondary;
case AlertState.Grace:
return theme.colorErrorText;
+ case AlertState.Retrying:
+ return theme.colorPrimaryText;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for Retrying</b></div>
<div id="fix">
The new `AlertState.Retrying` state lacks test coverage. Existing tests cover Success, Error, Grace, Noop, and empty state, but the Retrying case is untested. Per rule [6262], tests should verify behavior logic directly, not just rendering.
</div>
</div>
<small><i>Code Review Run #165e40</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2730,6 +2735,123 @@ 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);
+ if (!checked) {
+ updateAlertState('send_failed_reports', false);
+ updateAlertState('retry_notify_owners', true);
+ updateAlertState(
+ 'retry_notify_recipients',
+ false,
+ );
+ updateAlertState('retry_max_attempts', 3);
+ }
+ }}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for disable toggle</b></div>
<div id="fix">
The existing test 'shows retry options when Enable Retries is toggled on' (line 2895) only covers the enable direction. There is no test for the disable toggle, leaving the reset behavior uncovered. Adding a test for toggling OFF will prevent regressions.
</div>
</div>
<small><i>Code Review Run #165e40</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5143006216
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681786548
##########
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(
+ (self._report_schedule.id, self._scheduled_dttm.isoformat()),
+ countdown=delay_seconds,
+ )
+
+ def send_retry_notification(
+ self, attempt: int, max_attempts: int, error_message: str
+ ) -> None:
+ """
+ Send a per-retry-attempt failure notification to the owners and/or recipients
+ selected via the retry_notify_owners / retry_notify_recipients flags.
+ """
+ recipients: list[ReportRecipients] = []
+
+ if self._report_schedule.retry_notify_owners:
+ recipients.extend(
+ [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=json.dumps({"target": s.user.email}),
+ )
+ for s in self._report_schedule.editors
+ if s.type == SubjectType.USER and s.user
+ ]
+ )
+
+ if self._report_schedule.retry_notify_recipients:
+ recipients.extend(self._report_schedule.recipients)
+
+ if not recipients:
+ return
+
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_attempt=attempt,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, recipients)
+
+ def send_final_failure_report(self, error_message: str) -> None:
Review Comment:
This is intentional, `send_failed_reports` means "notify recipients about the failure after retries are exhausted," not "deliver a broken report." Recipients get an error email with the failure details and a link to the chart/dashboard.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5092027623
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3689550088
##########
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,
+ retry_notify_owners: currentReport.retry_notify_owners ?? true,
+ retry_notify_recipients: currentReport.retry_notify_recipients ?? false,
Review Comment:
**Suggestion:** When editing an existing report, `currentReport` is populated from the report list endpoint, whose `list_columns` does not include the retry fields. These nullish fallbacks therefore resolve to the defaults and the PUT payload overwrites any previously configured retry settings whenever the user saves unrelated report changes. Load the retry fields for edit state or omit them from the payload unless they were actually loaded or changed. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Editing reports can disable previously configured automatic retries.
- ⚠️ Retry and failure-notification preferences can be silently lost.
- ⚠️ Failed-report delivery behavior changes after unrelated edits.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=167dacd49d3043768e3129a2ea9f402c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=167dacd49d3043768e3129a2ea9f402c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/reports/ReportModal/index.tsx
**Line:** 199:203
**Comment:**
*Api Mismatch: When editing an existing report, `currentReport` is populated from the report list endpoint, whose `list_columns` does not include the retry fields. These nullish fallbacks therefore resolve to the defaults and the PUT payload overwrites any previously configured retry settings whenever the user saves unrelated report changes. Load the retry fields for edit state or omit them from the payload unless they were actually loaded or changed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6939ca7b0c5173a0a5a8da96ce00a28cf0b8531480d1466008eb5350e4d73d0f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6939ca7b0c5173a0a5a8da96ce00a28cf0b8531480d1466008eb5350e4d73d0f&reaction=dislike'>👎</a>
##########
superset/reports/schemas.py:
##########
@@ -311,6 +340,28 @@ def validate_report_references( # pylint: disable=unused-argument
{"database": ["Database reference is not allowed on a report"]}
)
+ @validates_schema
+ def validate_retry_config( # pylint: disable=unused-argument
+ self,
+ data: dict[str, Any],
+ **kwargs: Any,
+ ) -> None:
+ if data.get("send_failed_reports") and not data.get("retry_on_failure"):
+ raise ValidationError(
+ {
+ "send_failed_reports": [
+ _("send_failed_reports requires retry_on_failure to be enabled")
+ ]
+ }
+ )
+ # Retry is only supported for reports, not alerts.
+ if data.get("type") == ReportScheduleType.ALERT and data.get(
+ "retry_on_failure"
+ ):
+ raise ValidationError(
+ {"retry_on_failure": [_("Retries are not supported for alerts")]}
+ )
Review Comment:
**Suggestion:** The POST schema rejects `retry_on_failure` for alert schedules, while `ReportSchedulePutSchema` and `UpdateReportScheduleCommand` allow the same setting during updates. This makes the API contract inconsistent: an alert cannot be created with retries enabled but can be updated afterward to enable them. Either support retries consistently for both create and update, or apply the same alert restriction to the PUT path. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Alert creation cannot enable the advertised retry feature.
- ⚠️ Existing alerts can enable retries through PUT.
- ❌ API behavior differs between create and update workflows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0bd585ff4f2f4bc29545f70d5d56a96a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0bd585ff4f2f4bc29545f70d5d56a96a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 357:363
**Comment:**
*Api Mismatch: The POST schema rejects `retry_on_failure` for alert schedules, while `ReportSchedulePutSchema` and `UpdateReportScheduleCommand` allow the same setting during updates. This makes the API contract inconsistent: an alert cannot be created with retries enabled but can be updated afterward to enable them. Either support retries consistently for both create and update, or apply the same alert restriction to the PUT path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=060062b5c41a81693e8eb715df97ecd7965b35fd9f4d2eeb17087e12627c6767&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=060062b5c41a81693e8eb715df97ecd7965b35fd9f4d2eeb17087e12627c6767&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,191 @@ 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)
+
+ @staticmethod
+ def _normalize_dttm(dt: Optional[datetime]) -> Optional[datetime]:
+ """Strip timezone info and microseconds so naive/aware datetimes can
+ be compared safely. MySQL DateTime columns truncate microseconds,
+ so without this the round-tripped anchor would differ from the
+ in-memory value."""
+ if dt is not None:
+ return dt.replace(tzinfo=None, microsecond=0)
+ return dt
+
+ 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.
+
+ Both sides are normalized to naive datetimes because
+ retry_scheduled_dttm is a plain DateTime column (no timezone), while
+ _scheduled_dttm may be tz-aware depending on the Celery broker.
+ """
+ anchor = self._normalize_dttm(self._report_schedule.retry_scheduled_dttm)
+ current = self._normalize_dttm(self._scheduled_dttm)
+ return anchor is not None and anchor != current
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new count."""
+ self._report_schedule.retry_attempt += 1
+ # Store as naive — the column is a plain DateTime.
+ self._report_schedule.retry_scheduled_dttm = self._normalize_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(
+ (self._report_schedule.id, self._scheduled_dttm.isoformat()),
+ countdown=delay_seconds,
+ )
+
+ def send_retry_notification(
+ self, attempt: int, max_attempts: int, error_message: str
+ ) -> None:
+ """
+ Send a per-retry-attempt failure notification to the owners and/or recipients
+ selected via the retry_notify_owners / retry_notify_recipients flags.
+ """
+ recipients: list[ReportRecipients] = []
+
+ if self._report_schedule.retry_notify_owners:
+ recipients.extend(
+ [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=json.dumps({"target": s.user.email}),
+ )
+ for s in self._report_schedule.editors
+ if s.type == SubjectType.USER and s.user
+ ]
+ )
+
+ if self._report_schedule.retry_notify_recipients:
+ recipients.extend(self._report_schedule.recipients)
+
+ if not recipients:
+ return
+
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_attempt=attempt,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, recipients)
+
+ def send_final_failure_report(self, error_message: str) -> None:
+ """
+ Send the failed report notification to all configured recipients after
+ all retry attempts have been exhausted and send_failed_reports is enabled.
+ """
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ max_attempts: int = self._report_schedule.retry_max_attempts
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, self._report_schedule.recipients)
+
+ def _handle_retry_or_error(
+ self, error_message: str, original_exception: Exception
+ ) -> bool:
+ """
+ Shared retry-or-error logic. Returns True if a retry was scheduled
+ (caller should ``return`` without re-raising), or False if the caller
+ should fall through to its own error handling path.
+ """
+ retry_on_failure: bool = self._report_schedule.retry_on_failure
+ if not retry_on_failure:
+ return False
+
+ max_attempts: int = self._report_schedule.retry_max_attempts
+
+ # If a new crontab window has fired since the first failure, reset the
+ # retry counter so this window gets a fresh budget.
+ if self._is_retry_window_stale():
+ self._reset_retry_counter()
+
+ current_attempt = self._report_schedule.retry_attempt
+
+ # If this execution was itself a retry (current_attempt > 0),
+ # send the retry-failure notification *after* the attempt ran so
+ # the email reflects what happened, not what is about to be
+ # scheduled. ("You will receive an update after each retry.")
+ if current_attempt > 0:
+ try:
+ self.send_retry_notification(
+ current_attempt, max_attempts, error_message
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Failed to send retry notification for report schedule "
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+
+ if current_attempt < max_attempts:
+ # Schedule another attempt and exit cleanly (don't re-raise).
+ self._increment_retry()
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.RETRYING, error_message=error_message
+ )
+ except ReportScheduleUnexpectedError as logging_ex:
+ logger.warning(
+ "Failed to log RETRYING state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
+ raise original_exception from logging_ex
+ self._schedule_retry(
+ self._get_retry_delay(self._report_schedule.retry_attempt - 1)
+ )
Review Comment:
**Suggestion:** If `apply_async` fails after `update_report_schedule_and_log(ReportState.RETRYING, ...)` has committed, the exception escapes without restoring the schedule state. The schedule remains `RETRYING` with a retry counter but no queued task, and subsequent crontab executions hit the stale-window skip branch, potentially suppressing future deliveries indefinitely. Handle queue submission failure by recording a terminal error or rolling back/resetting the retry state. [state error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Failed queue publication can suppress future deliveries.
- ❌ Crontab executions remain stuck in `RETRYING`.
- ⚠️ Operators need manual state recovery.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a601b1127e5745b6ab800f3c41f7b6a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a601b1127e5745b6ab800f3c41f7b6a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1334:1336
**Comment:**
*State Error: If `apply_async` fails after `update_report_schedule_and_log(ReportState.RETRYING, ...)` has committed, the exception escapes without restoring the schedule state. The schedule remains `RETRYING` with a retry counter but no queued task, and subsequent crontab executions hit the stale-window skip branch, potentially suppressing future deliveries indefinitely. Handle queue submission failure by recording a terminal error or rolling back/resetting the retry state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=03e88ed81b3d0a36814c1df71495c91a1192efdfb1ebdf471b6a4014bbfcdb49&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=03e88ed81b3d0a36814c1df71495c91a1192efdfb1ebdf471b6a4014bbfcdb49&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1224,6 +1428,9 @@ def next(self) -> None: # noqa: C901
if isinstance(first_ex, SupersetErrorsException):
error_message = ";".join([error.message for error in first_ex.errors])
+ if self._handle_retry_or_error(error_message, first_ex):
+ return # retry scheduled — exit cleanly
Review Comment:
**Suggestion:** This newly added retry handler is invoked for every exception in the success block, including failures from `update_report_schedule_and_log` after `send()` has already delivered the report. If the delivery succeeded but the success-state database write fails, this schedules another delivery and can send the same report twice. Separate delivery failures from post-delivery state/logging failures before retrying. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Recipients can receive duplicate report deliveries.
- ⚠️ Database concurrency failures cause unnecessary retries.
- ⚠️ Report execution logs may not reflect delivery outcome.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b2ecb00513f4cdb97f7eb497888ce68&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0b2ecb00513f4cdb97f7eb497888ce68&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1431:1432
**Comment:**
*Logic Error: This newly added retry handler is invoked for every exception in the success block, including failures from `update_report_schedule_and_log` after `send()` has already delivered the report. If the delivery succeeded but the success-state database write fails, this schedules another delivery and can send the same report twice. Separate delivery failures from post-delivery state/logging failures before retrying.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=e504787b99452bb1f082f54cbdd5e5b43a0ee9c07ecc973e0f8d01f298f03cb9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=e504787b99452bb1f082f54cbdd5e5b43a0ee9c07ecc973e0f8d01f298f03cb9&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681670230
##########
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:
good point, addressed it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681706400
##########
superset-frontend/src/features/alerts/components/AlertStatusIcon.tsx:
##########
@@ -92,6 +94,13 @@ export default function AlertStatusIcon({
lastStateConfig.label = t('Alert Triggered, In Grace Period');
lastStateConfig.status = AlertState.Grace;
break;
+ case AlertState.Retrying:
+ lastStateConfig.icon = Icons.Running;
+ lastStateConfig.label = isReportEnabled
+ ? t('Report retrying')
+ : t('Alert retrying');
+ lastStateConfig.status = AlertState.Retrying;
+ break;
Review Comment:
**Suggestion:** The new retrying state uses the running icon, but `isRunningIcon` only checks for `AlertState.Working`, so retrying icons are rendered at the normal size instead of receiving the enlarged running-state styling. Include `AlertState.Retrying` in the running-icon condition so the new state is visually consistent with active execution. [css layout issue]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Retrying alerts display an inconsistently sized active-state icon.
- ⚠️ Alerts and reports retry status loses visual emphasis.
- ⚠️ Execution log and report list presentation differ from working-state styling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd8c568aa0ea4932b8f72498140f0194&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bd8c568aa0ea4932b8f72498140f0194&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/components/AlertStatusIcon.tsx
**Line:** 97:103
**Comment:**
*Css Layout Issue: The new retrying state uses the running icon, but `isRunningIcon` only checks for `AlertState.Working`, so retrying icons are rendered at the normal size instead of receiving the enlarged running-state styling. Include `AlertState.Retrying` in the running-icon condition so the new state is visually consistent with active execution.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9885c2b49c420a1ee098a7cd655f19b27f962d7ac3a829407ec606109bee8644&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9885c2b49c420a1ee098a7cd655f19b27f962d7ac3a829407ec606109bee8644&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681727810
##########
superset/commands/report/execute.py:
##########
@@ -1195,10 +1377,26 @@ class ReportNotTriggeredErrorState(BaseReportState):
- Error
"""
- current_states = [ReportState.NOOP, ReportState.ERROR]
+ current_states = [ReportState.NOOP, ReportState.ERROR, ReportState.RETRYING]
initial = True
def next(self) -> None: # noqa: C901
+ # If retries from a previous crontab window are still in-flight and
+ # this is a new crontab trigger, skip — let the active retry chain
+ # finish. The stale-window check uses normalized (naive) datetimes
+ # so tz-aware vs naive mismatches don't cause false positives.
+ if (
+ self._report_schedule.last_state == ReportState.RETRYING
+ and self._is_retry_window_stale()
+ ):
+ logger.info(
+ "Skipping crontab execution for report %s — retries from a "
+ "previous window are still in-flight (execution %s)",
+ self._report_schedule.id,
+ self._execution_id,
+ )
+ return
Review Comment:
**Suggestion:** This branch permanently drops a newly scheduled crontab window when an older retry chain is active. It returns without advancing `last_eval_dttm`, recording an execution, or enqueuing the new window; because the scheduler continues to produce future windows, this particular window is never retried after the old chain finishes. The new-window execution must be deferred and re-enqueued, or otherwise recorded so it is not lost. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Scheduled report windows can be silently skipped.
- ❌ Alert evaluations may miss their next scheduled check.
- ⚠️ Users receive no execution log for skipped windows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3c23da30c81b4a988efc3c60da4db602&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3c23da30c81b4a988efc3c60da4db602&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1388:1398
**Comment:**
*Incomplete Implementation: This branch permanently drops a newly scheduled crontab window when an older retry chain is active. It returns without advancing `last_eval_dttm`, recording an execution, or enqueuing the new window; because the scheduler continues to produce future windows, this particular window is never retried after the old chain finishes. The new-window execution must be deferred and re-enqueued, or otherwise recorded so it is not lost.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=cde04d64be83a84e557f972e8984cfe1bce39e9821623579bc4afb08861f78ea&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=cde04d64be83a84e557f972e8984cfe1bce39e9821623579bc4afb08861f78ea&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,188 @@ 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)
+
+ @staticmethod
+ def _normalize_dttm(dt: Optional[datetime]) -> Optional[datetime]:
+ """Strip timezone info so naive/aware datetimes can be compared."""
+ if dt is not None and dt.tzinfo is not None:
+ return dt.replace(tzinfo=None)
+ return dt
+
+ 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.
+
+ Both sides are normalized to naive datetimes because
+ retry_scheduled_dttm is a plain DateTime column (no timezone), while
+ _scheduled_dttm may be tz-aware depending on the Celery broker.
+ """
+ anchor = self._normalize_dttm(self._report_schedule.retry_scheduled_dttm)
+ current = self._normalize_dttm(self._scheduled_dttm)
+ return anchor is not None and anchor != current
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new count."""
+ self._report_schedule.retry_attempt += 1
+ # Store as naive — the column is a plain DateTime.
+ self._report_schedule.retry_scheduled_dttm = self._normalize_dttm(
+ self._scheduled_dttm
+ )
+ return self._report_schedule.retry_attempt
Review Comment:
**Suggestion:** The retry counter is modified on the ORM object and then persisted without acquiring a row lock or performing an atomic compare-and-update. Two duplicate Celery executions can both observe the same `retry_attempt`, each enqueue another retry, and overwrite the retry anchor, causing duplicate report deliveries and exceeding the configured retry budget. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Duplicate retry tasks can deliver duplicate reports.
- ❌ Retry budgets can be exceeded under overlapping execution.
- ⚠️ Retry state anchors can be overwritten by stale workers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f290fca1ee046e38014a75a53fc6b56&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2f290fca1ee046e38014a75a53fc6b56&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1195:1202
**Comment:**
*Race Condition: The retry counter is modified on the ORM object and then persisted without acquiring a row lock or performing an atomic compare-and-update. Two duplicate Celery executions can both observe the same `retry_attempt`, each enqueue another retry, and overwrite the retry anchor, causing duplicate report deliveries and exceeding the configured retry budget.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=39935f9cf1ca1e9625a44e6ba099e43e07d3e9c99427e1d2e37a88cab420d732&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=39935f9cf1ca1e9625a44e6ba099e43e07d3e9c99427e1d2e37a88cab420d732&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-07-28_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: d3b9a1f6c204
+Create Date: 2026-07-28 00:00:00.000000
+
+"""
+
+import logging
+
+import sqlalchemy as sa
+from alembic import op
+
+from superset.migrations.shared.utils import get_table_column
+
+logger = logging.getLogger("alembic.env")
+
+# revision identifiers, used by Alembic.
+revision = "f3a8c1d2e9b7"
+down_revision = "d3b9a1f6c204"
+
+# Configuration columns (user-configurable)
+_CONFIG_COLUMNS = [
+ ("retry_on_failure", sa.Boolean(), False, "0"),
+ ("retry_max_attempts", sa.Integer(), False, "3"),
+ ("send_failed_reports", sa.Boolean(), False, "0"),
+ ("retry_notify_owners", sa.Boolean(), False, "1"),
+ ("retry_notify_recipients", sa.Boolean(), False, "0"),
+]
+
+# State columns (written by the execution engine)
+_STATE_COLUMNS = [
+ ("retry_attempt", sa.Integer(), False, "0"),
+ ("retry_scheduled_dttm", sa.DateTime(), True, None),
+]
+
+
+def upgrade() -> None:
+ """Add retry config and state columns to report_schedule."""
+ if get_table_column("report_schedule", "retry_attempt") is not None:
+ logger.info(
+ "Column report_schedule.retry_attempt already exists. Skipping migration."
+ )
+ return
Review Comment:
**Suggestion:** Using only `retry_attempt` as the migration-completion marker makes the migration non-recoverable after a partial DDL application. If that column was created but any later retry column was not, a rerun returns early and leaves the model querying nonexistent columns. Check that every expected column exists, or add missing columns individually. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Failed upgrades can leave report execution unusable.
- ❌ Retry configuration fields may be missing after migration reruns.
- ⚠️ Operators must manually repair partially applied schemas.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07d912c711ab476fab8c68c980cce43d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=07d912c711ab476fab8c68c980cce43d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-07-28_00-00_f3a8c1d2e9b7_add_report_retry_state_columns.py
**Line:** 56:60
**Comment:**
*Possible Bug: Using only `retry_attempt` as the migration-completion marker makes the migration non-recoverable after a partial DDL application. If that column was created but any later retry column was not, a rerun returns early and leaves the model querying nonexistent columns. Check that every expected column exists, or add missing columns individually.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6b266d770ed29f8db49be66f8401afb93024c4a0b006f21f0889edf4f2dadfb2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6b266d770ed29f8db49be66f8401afb93024c4a0b006f21f0889edf4f2dadfb2&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1403,10 +1606,15 @@ 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()
self.update_report_schedule_and_log(
ReportState.SUCCESS, error_message=warning_message
)
except Exception as ex: # pylint: disable=broad-except
+ if self._handle_retry_or_error(str(ex), ex):
+ return # retry scheduled — exit cleanly
Review Comment:
**Suggestion:** The retry handler is invoked only from the report send failure path shown here. For alerts that are already in `SUCCESS` or `GRACE`, failures from the preceding `AlertCommand.run()` are handled by the separate alert exception branch and bypass `_handle_retry_or_error`, so configured retries do not apply to those alert evaluations. Route that alert failure path through the same retry logic. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Configured retries do not cover alert evaluation failures.
- ❌ Transient alert query failures immediately enter ERROR.
- ⚠️ Alert owners receive failure notifications without retry attempts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8578cbf23c44d808b2c78f00985a8ae&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f8578cbf23c44d808b2c78f00985a8ae&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1615:1616
**Comment:**
*Incomplete Implementation: The retry handler is invoked only from the report send failure path shown here. For alerts that are already in `SUCCESS` or `GRACE`, failures from the preceding `AlertCommand.run()` are handled by the separate alert exception branch and bypass `_handle_retry_or_error`, so configured retries do not apply to those alert evaluations. Route that alert failure path through the same retry logic.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=25c14c84d44c460dd8687439f95a53807289d0ee0c4d98f6aa7b985de84d6d58&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=25c14c84d44c460dd8687439f95a53807289d0ee0c4d98f6aa7b985de84d6d58&reaction=dislike'>👎</a>
##########
superset/reports/api.py:
##########
@@ -147,6 +147,11 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]:
"validator_type",
"working_timeout",
"email_subject",
+ "retry_on_failure",
+ "retry_max_attempts",
+ "send_failed_reports",
+ "retry_notify_owners",
+ "retry_notify_recipients",
Review Comment:
**Suggestion:** The retry fields are exposed in `show_columns`, but they are absent from `add_columns` and `edit_columns`, so FAB filters them out of create and update requests before persistence. The UI can submit these settings successfully while the database retains the defaults. Add all retry fields to the writable column configuration as well. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ UI retry settings cannot be persisted.
- ❌ Reports continue using default retry behavior.
- ⚠️ GET responses falsely suggest settings were accepted.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6429313e4c884b3c8f650cbd48934969&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6429313e4c884b3c8f650cbd48934969&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 150:154
**Comment:**
*Api Mismatch: The retry fields are exposed in `show_columns`, but they are absent from `add_columns` and `edit_columns`, so FAB filters them out of create and update requests before persistence. The UI can submit these settings successfully while the database retains the defaults. Add all retry fields to the writable column configuration as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=47b6693402f2d33275f986798ec481a50127747cccfbc2f7121f047e7be8450c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=47b6693402f2d33275f986798ec481a50127747cccfbc2f7121f047e7be8450c&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3689547745
##########
superset/commands/report/update.py:
##########
@@ -179,5 +180,17 @@ def validate(self) -> None: # noqa: C901
include_viewers=False,
)
+ # Validate retry config: send_failed_reports requires retry_on_failure.
+ # Fall back to the existing DB value for fields not in the payload.
+ send_failed = self._properties.get(
+ "send_failed_reports", self._model.send_failed_reports
+ )
+ retry_enabled = self._properties.get(
+ "retry_on_failure", self._model.retry_on_failure
+ )
+ if send_failed and not retry_enabled:
+ msg = _("send_failed_reports requires retry_on_failure to be enabled")
+ exceptions.append(ValidationError({"send_failed_reports": [msg]}))
Review Comment:
**Suggestion:** Partial updates can enable retries on alert schedules. `ReportSchedulePutSchema` does not require `type`, so an update containing only `retry_on_failure: true` bypasses the schema's alert restriction, and this command-level validation checks only the retry/send-failed dependency, not the effective `report_type`. Reject retry configuration when the existing or updated schedule type is an alert, otherwise alert executions can enter the retry flow even though retries are documented as report-only. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Alert failures can be re-executed through the report retry workflow.
- ⚠️ Alert owners or recipients may receive unsupported retry notifications.
- ⚠️ Repeated alert evaluations can create duplicate delivery attempts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=59829086919b441d8018a7dcf8ae0a1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=59829086919b441d8018a7dcf8ae0a1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/update.py
**Line:** 183:193
**Comment:**
*Api Mismatch: Partial updates can enable retries on alert schedules. `ReportSchedulePutSchema` does not require `type`, so an update containing only `retry_on_failure: true` bypasses the schema's alert restriction, and this command-level validation checks only the retry/send-failed dependency, not the effective `report_type`. Reject retry configuration when the existing or updated schedule type is an alert, otherwise alert executions can enter the retry flow even though retries are documented as report-only.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=dd23528dc4fa18fc7e606959eb77ad39fc3cc33169b8539a769fe601fb5aa713&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=dd23528dc4fa18fc7e606959eb77ad39fc3cc33169b8539a769fe601fb5aa713&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3689551599
##########
superset/reports/models.py:
##########
@@ -77,6 +77,7 @@ class ReportState(StrEnum):
ERROR = "Error"
NOOP = "Not triggered"
GRACE = "On Grace"
+ RETRYING = "Retrying"
Review Comment:
**Suggestion:** The new backend state can now be returned as `last_state: "Retrying"`, but the frontend `AlertObject.last_state` type still only permits `Success`, `Working`, `Error`, `Not triggered`, and `On Grace`. This leaves API responses in a state that the primary alert/report object contract cannot represent and can cause type errors or invalid state handling in consumers. Add `Retrying` to the shared response type wherever `ReportState.RETRYING` is exposed. [api mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Typed alert consumers cannot represent active retry state.
- ⚠️ API and frontend state contracts diverge during retry execution.
- ⚠️ Current status icon handles retrying, limiting immediate UI impact.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1138069e94fa4252a0dcdab1ecc3e121&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1138069e94fa4252a0dcdab1ecc3e121&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/models.py
**Line:** 80:80
**Comment:**
*Api Mismatch: The new backend state can now be returned as `last_state: "Retrying"`, but the frontend `AlertObject.last_state` type still only permits `Success`, `Working`, `Error`, `Not triggered`, and `On Grace`. This leaves API responses in a state that the primary alert/report object contract cannot represent and can cause type errors or invalid state handling in consumers. Add `Retrying` to the shared response type wherever `ReportState.RETRYING` is exposed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=4c82e4de077da4f47ed46b545942387361d058a74f111704482225c2379155f3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=4c82e4de077da4f47ed46b545942387361d058a74f111704482225c2379155f3&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681867812
##########
superset/commands/report/execute.py:
##########
@@ -1403,10 +1606,15 @@ 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()
self.update_report_schedule_and_log(
ReportState.SUCCESS, error_message=warning_message
)
except Exception as ex: # pylint: disable=broad-except
+ if self._handle_retry_or_error(str(ex), ex):
+ return # retry scheduled — exit cleanly
Review Comment:
The retry feature is for report delivery
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5180733798
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5092095182
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42481?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `32.78689%` with `82 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.99%. Comparing base ([`6856d0f`](https://app.codecov.io/gh/apache/superset/commit/6856d0fc671dda62f4f305212b66e1bef8e8203e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`446412d`](https://app.codecov.io/gh/apache/superset/commit/446412d954c451f36e22571834f43f70ce194a02?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42481?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/report/execute.py](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fexecute.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2V4ZWN1dGUucHk=) | 12.90% | [54 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/email.py](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Femail.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL2VtYWlsLnB5) | 4.54% | [21 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/schemas.py](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&filepath=superset%2Freports%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9zY2hlbWFzLnB5) | 77.77% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/slack\_mixin.py](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Fslack_mixin.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrX21peGluLnB5) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (6856d0f) and HEAD (446412d). Click for more details.
>
> <details><summary>HEAD has 96 uploads less than BASE</summary>
>
>| Flag | BASE (6856d0f) | HEAD (446412d) |
>|------|------|------|
>|python|83|1|
>|presto|15|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42481 +/- ##
==========================================
- Coverage 65.25% 55.99% -9.27%
==========================================
Files 2795 2795
Lines 157639 157758 +119
Branches 36052 36066 +14
==========================================
- Hits 102869 88335 -14534
- Misses 52793 68598 +15805
+ Partials 1977 825 -1152
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.29% <32.78%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.29% <32.78%> (-18.71%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42481?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681934833
##########
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:
done
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681870362
##########
superset/commands/report/execute.py:
##########
@@ -1403,10 +1606,15 @@ 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()
self.update_report_schedule_and_log(
ReportState.SUCCESS, error_message=warning_message
)
except Exception as ex: # pylint: disable=broad-except
+ if self._handle_retry_or_error(str(ex), ex):
+ return # retry scheduled — exit cleanly
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Apply retry handling only to report delivery failures; do not require retries for alert evaluation failures.
**Applied to:**
- `superset/commands/report/execute.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
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
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.usefixtures("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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681905512
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2730,6 +2735,123 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
</>
),
},
+ ...(isReport
+ ? [
+ {
+ key: 'error-handling',
Review Comment:
This is a report feature, not an alert feature
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5120424936
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681875823
##########
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:
done
##########
superset/migrations/versions/2026-07-28_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:
took care of it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681762477
##########
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:
added validation, thanks
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681754750
##########
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:
added retry logic, thank you
##########
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:
took care of it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681757302
##########
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:
found a better solution for this, thanks
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681960987
##########
superset-frontend/src/features/alerts/types.ts:
##########
@@ -170,6 +175,7 @@ export enum AlertState {
Error = 'Error',
Noop = 'Not triggered',
Grace = 'On Grace',
+ Retrying = 'Retrying',
Review Comment:
**Suggestion:** The new `Retrying` enum value is not included in `AlertObject.last_state`, so the frontend type contract cannot represent the state returned by the backend. Update the `last_state` union to include `Retrying`; otherwise consumers of `AlertObject` will reject or incorrectly narrow retrying schedules. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Typed alert consumers cannot represent retrying responses.
- ⚠️ New retrying fixtures and integrations produce TypeScript errors.
- ⚠️ Runtime icon handling remains inconsistent with the contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62865bc928f741f3910efb6b8a63c941&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=62865bc928f741f3910efb6b8a63c941&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/types.ts
**Line:** 178:178
**Comment:**
*Type Error: The new `Retrying` enum value is not included in `AlertObject.last_state`, so the frontend type contract cannot represent the state returned by the backend. Update the `last_state` union to include `Retrying`; otherwise consumers of `AlertObject` will reject or incorrectly narrow retrying schedules.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=2f00498cad7afd0ad37a5ec338cd8deddd2c95bc9f013ed9320d95b378131d66&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=2f00498cad7afd0ad37a5ec338cd8deddd2c95bc9f013ed9320d95b378131d66&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -2667,3 +2667,416 @@ 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
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_on_failure_schedules_retry(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is enabled and the report fails,
+ the state transitions to RETRYING and a retry task is enqueued with the
+ correct exponential-backoff delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ # Should NOT re-raise (retry path exits cleanly)
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 1
+ # Verify delay: base=60, attempt=1 → min(60 * 2^1, 3600) = 120
+ # Verify delay: base=60, attempt-1=0 → min(60 * 2^0, 3600) = 60
+ schedule_retry_mock.assert_called_once_with(60)
+ # No error email should be sent on the first failure (notification is
+ # sent after a *retry* fails, not after the original failure)
+ email_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_exhausted_transitions_to_error(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when all retries are exhausted the state transitions
+ to ERROR, the retry counter is reset, and the retry notification is sent
+ for the final attempt.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=2,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Pre-set retry_attempt to the max so the next execution exhausts retries.
+ # Use the same timestamp for both so _is_retry_window_stale() returns False.
+ # Truncate microseconds — MySQL DateTime columns drop them, which would make
+ # the round-tripped value differ from the in-memory one.
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ # Counter is reset after exhaustion
+ assert report_schedule.retry_attempt == 0
+ # No further retry should have been scheduled
+ schedule_retry_mock.assert_not_called()
+ # Retry notification sent for the exhausted attempt (attempt 2 of 2)
+ # The error message is wrapped by the screenshot layer, so use ANY.
+ retry_notification_mock.assert_called_once_with(2, 2, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_send_failed_reports_sends_to_recipients(
+ screenshot_mock: Mock,
+ final_failure_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when send_failed_reports is True and all retries are
+ exhausted, send_final_failure_report is called with the error message.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=1,
+ send_failed_reports=True,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # send_final_failure_report should have been called
+ final_failure_mock.assert_called_once_with(ANY)
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retrying_state_schedules_another_retry(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: a schedule with last_state=RETRYING is routed to
+ ReportNotTriggeredErrorState, which increments the counter and schedules
+ another retry with the correct delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("still failing")
+
+ # Should not raise — still within retry budget
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 2
+ # Verify delay: base=60, attempt=2 → min(60 * 2^2, 3600) = 240
+ # Verify delay: base=60, attempt-1=1 → min(60 * 2^1, 3600) = 120
+ schedule_retry_mock.assert_called_once_with(120)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_disabled_preserves_default_error_path(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is False (default), the
+ existing error behavior is unchanged — no RETRYING state, no retry
+ scheduled, the exception is re-raised normally.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ assert report_schedule.retry_attempt == 0
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_notify_owners_sends_notification(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_notify_owners is True (default) and a
+ retry attempt fails, send_retry_notification is called.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=True,
+ retry_notify_recipients=False,
+ )
+ # Set up as a retry attempt (current_attempt=1) so the notification fires
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # Notification sent for the failed retry (attempt 1 of 3)
+ retry_notification_mock.assert_called_once_with(1, 3, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_stale_retry_window_resets_counter(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when a new crontab window fires while a previous
+ window was still retrying, the retry counter is reset so the new window
+ gets a fresh budget.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Simulate: previous window set retry_attempt=2 with an old anchor
+ old_anchor = datetime(2020, 1, 1, 0, 0, 0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = old_anchor
+ report_schedule.last_state = ReportState.RETRYING
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+ # Pass a *different* scheduled_dttm (new crontab window)
+ new_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+
+ AsyncExecuteReportScheduleCommand(TEST_ID, report_schedule.id, new_dttm).run()
Review Comment:
**Suggestion:** This test sets `last_state` to `RETRYING` with an old retry anchor, but the state machine intentionally skips a new crontab execution when retries from a previous window are still in flight. Consequently, `run()` returns before resetting or incrementing the counter, so the assertion that `retry_attempt` becomes 1 fails. Align the setup and expectation with the skip behavior, or change the production behavior if stale windows are meant to reset immediately. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Integration test fails whenever the reports command suite runs.
- ⚠️ Stale-window retry behavior remains incorrectly specified.
- ⚠️ Retry scheduling is not exercised for skipped new windows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a03b708cca134add96bebb745cb3edff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a03b708cca134add96bebb745cb3edff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 2971:2973
**Comment:**
*Incorrect Condition Logic: This test sets `last_state` to `RETRYING` with an old retry anchor, but the state machine intentionally skips a new crontab execution when retries from a previous window are still in flight. Consequently, `run()` returns before resetting or incrementing the counter, so the assertion that `retry_attempt` becomes 1 fails. Align the setup and expectation with the skip behavior, or change the production behavior if stale windows are meant to reset immediately.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=076a5d253a6497aaaff9aa222d7e497449d7f154dfd1ede5df566f21ce332b1a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=076a5d253a6497aaaff9aa222d7e497449d7f154dfd1ede5df566f21ce332b1a&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681708887
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2730,6 +2735,123 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
</>
),
},
+ ...(isReport
+ ? [
+ {
+ key: 'error-handling',
Review Comment:
**Suggestion:** The retry configuration is only added when `isReport` is true, so alert schedules cannot enable retries or configure failure notifications even though the backend supports these fields and the feature is described as applying to both alerts and reports. Render this section for alerts as well, or otherwise expose the retry settings in the alert modal. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Alert schedules cannot enable automatic retries.
- ❌ Alert failure notification preferences remain unavailable.
- ⚠️ Shared retry fields remain inaccessible in alert mode.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f77adda553db4ec5898d438087290779&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f77adda553db4ec5898d438087290779&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 2738:2741
**Comment:**
*Incomplete Implementation: The retry configuration is only added when `isReport` is true, so alert schedules cannot enable retries or configure failure notifications even though the backend supports these fields and the feature is described as applying to both alerts and reports. Render this section for alerts as well, or otherwise expose the retry settings in the alert modal.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=3328b50cd611462faff3d573b50ad267c05971a9a921cbcd97808a5d28c2cda1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=3328b50cd611462faff3d573b50ad267c05971a9a921cbcd97808a5d28c2cda1&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -2667,3 +2667,468 @@ 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
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_on_failure_schedules_retry(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is enabled and the report fails,
+ the state transitions to RETRYING and a retry task is enqueued with the
+ correct exponential-backoff delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ # Should NOT re-raise (retry path exits cleanly)
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 1
+ # Verify delay: base=60, attempt=1 → min(60 * 2^1, 3600) = 120
+ # Verify delay: base=60, attempt-1=0 → min(60 * 2^0, 3600) = 60
+ schedule_retry_mock.assert_called_once_with(60)
+ # No error email should be sent on the first failure (notification is
+ # sent after a *retry* fails, not after the original failure)
+ email_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_exhausted_transitions_to_error(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when all retries are exhausted the state transitions
+ to ERROR, the retry counter is reset, and the retry notification is sent
+ for the final attempt.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=2,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Pre-set retry_attempt to the max so the next execution exhausts retries.
+ # Use the same timestamp for both so _is_retry_window_stale() returns False.
+ # Truncate microseconds — MySQL DateTime columns drop them, which would make
+ # the round-tripped value differ from the in-memory one.
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ # Counter is reset after exhaustion
+ assert report_schedule.retry_attempt == 0
+ # No further retry should have been scheduled
+ schedule_retry_mock.assert_not_called()
+ # Retry notification sent for the exhausted attempt (attempt 2 of 2)
+ # The error message is wrapped by the screenshot layer, so use ANY.
+ retry_notification_mock.assert_called_once_with(2, 2, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_send_failed_reports_sends_to_recipients(
+ screenshot_mock: Mock,
+ final_failure_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when send_failed_reports is True and all retries are
+ exhausted, send_final_failure_report is called with the error message.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=1,
+ send_failed_reports=True,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # send_final_failure_report should have been called
+ final_failure_mock.assert_called_once_with(ANY)
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retrying_state_schedules_another_retry(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: a schedule with last_state=RETRYING is routed to
+ ReportNotTriggeredErrorState, which increments the counter and schedules
+ another retry with the correct delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("still failing")
+
+ # Should not raise — still within retry budget
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 2
+ # Verify delay: base=60, attempt=2 → min(60 * 2^2, 3600) = 240
+ # Verify delay: base=60, attempt-1=1 → min(60 * 2^1, 3600) = 120
+ schedule_retry_mock.assert_called_once_with(120)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_disabled_preserves_default_error_path(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is False (default), the
+ existing error behavior is unchanged — no RETRYING state, no retry
+ scheduled, the exception is re-raised normally.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ assert report_schedule.retry_attempt == 0
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_notify_owners_sends_notification(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_notify_owners is True (default) and a
+ retry attempt fails, send_retry_notification is called.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=True,
+ retry_notify_recipients=False,
+ )
+ # Set up as a retry attempt (current_attempt=1) so the notification fires
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # Notification sent for the failed retry (attempt 1 of 3)
+ retry_notification_mock.assert_called_once_with(1, 3, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_stale_retry_window_resets_counter(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when a new crontab window fires while a previous
+ window was still retrying, the retry counter is reset so the new window
+ gets a fresh budget.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Simulate: previous window set retry_attempt=2 with an old anchor
+ old_anchor = datetime(2020, 1, 1, 0, 0, 0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = old_anchor
+ report_schedule.last_state = ReportState.RETRYING
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+ # Pass a *different* scheduled_dttm (new crontab window)
+ new_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+
+ AsyncExecuteReportScheduleCommand(TEST_ID, report_schedule.id, new_dttm).run()
+
+ db.session.refresh(report_schedule)
+ # Counter was reset to 0, then incremented to 1 for the new window
+ assert report_schedule.retry_attempt == 1
+ assert report_schedule.last_state == ReportState.RETRYING
Review Comment:
**Suggestion:** This test expects a stale `RETRYING` schedule to reset its retry counter and execute a new attempt, but `ReportNotTriggeredErrorState.next()` explicitly returns early when the retry window is stale while the schedule remains in `RETRYING`. Consequently, the counter remains `2`, no retry is scheduled, and these assertions fail. Align the test with the implemented stale-window behavior or change the production state transition before retaining this test. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ CI fails when this integration test executes.
- ⚠️ New crontab windows intentionally skip active retry chains.
- ⚠️ Retry counter remains unchanged for skipped windows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=88d03a62319740fdbe9821e6d80d9b64&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=88d03a62319740fdbe9821e6d80d9b64&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 2975:2978
**Comment:**
*Logic Error: This test expects a stale `RETRYING` schedule to reset its retry counter and execute a new attempt, but `ReportNotTriggeredErrorState.next()` explicitly returns early when the retry window is stale while the schedule remains in `RETRYING`. Consequently, the counter remains `2`, no retry is scheduled, and these assertions fail. Align the test with the implemented stale-window behavior or change the production state transition before retaining this test.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=129305d8a3ab1b0e0cb21db031edd7df9d77a8ace29a02b7bdaa9f76eb1c1223&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=129305d8a3ab1b0e0cb21db031edd7df9d77a8ace29a02b7bdaa9f76eb1c1223&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -2667,3 +2667,468 @@ 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
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_on_failure_schedules_retry(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is enabled and the report fails,
+ the state transitions to RETRYING and a retry task is enqueued with the
+ correct exponential-backoff delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ # Should NOT re-raise (retry path exits cleanly)
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 1
+ # Verify delay: base=60, attempt=1 → min(60 * 2^1, 3600) = 120
+ # Verify delay: base=60, attempt-1=0 → min(60 * 2^0, 3600) = 60
+ schedule_retry_mock.assert_called_once_with(60)
+ # No error email should be sent on the first failure (notification is
+ # sent after a *retry* fails, not after the original failure)
+ email_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_exhausted_transitions_to_error(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when all retries are exhausted the state transitions
+ to ERROR, the retry counter is reset, and the retry notification is sent
+ for the final attempt.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=2,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Pre-set retry_attempt to the max so the next execution exhausts retries.
+ # Use the same timestamp for both so _is_retry_window_stale() returns False.
+ # Truncate microseconds — MySQL DateTime columns drop them, which would make
+ # the round-tripped value differ from the in-memory one.
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ # Counter is reset after exhaustion
+ assert report_schedule.retry_attempt == 0
+ # No further retry should have been scheduled
+ schedule_retry_mock.assert_not_called()
+ # Retry notification sent for the exhausted attempt (attempt 2 of 2)
+ # The error message is wrapped by the screenshot layer, so use ANY.
+ retry_notification_mock.assert_called_once_with(2, 2, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_send_failed_reports_sends_to_recipients(
+ screenshot_mock: Mock,
+ final_failure_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when send_failed_reports is True and all retries are
+ exhausted, send_final_failure_report is called with the error message.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=1,
+ send_failed_reports=True,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # send_final_failure_report should have been called
+ final_failure_mock.assert_called_once_with(ANY)
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retrying_state_schedules_another_retry(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: a schedule with last_state=RETRYING is routed to
+ ReportNotTriggeredErrorState, which increments the counter and schedules
+ another retry with the correct delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("still failing")
+
+ # Should not raise — still within retry budget
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 2
+ # Verify delay: base=60, attempt=2 → min(60 * 2^2, 3600) = 240
+ # Verify delay: base=60, attempt-1=1 → min(60 * 2^1, 3600) = 120
+ schedule_retry_mock.assert_called_once_with(120)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_disabled_preserves_default_error_path(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is False (default), the
+ existing error behavior is unchanged — no RETRYING state, no retry
+ scheduled, the exception is re-raised normally.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ assert report_schedule.retry_attempt == 0
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_notify_owners_sends_notification(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_notify_owners is True (default) and a
+ retry attempt fails, send_retry_notification is called.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=True,
+ retry_notify_recipients=False,
+ )
+ # Set up as a retry attempt (current_attempt=1) so the notification fires
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # Notification sent for the failed retry (attempt 1 of 3)
+ retry_notification_mock.assert_called_once_with(1, 3, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_stale_retry_window_resets_counter(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when a new crontab window fires while a previous
+ window was still retrying, the retry counter is reset so the new window
+ gets a fresh budget.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Simulate: previous window set retry_attempt=2 with an old anchor
+ old_anchor = datetime(2020, 1, 1, 0, 0, 0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = old_anchor
+ report_schedule.last_state = ReportState.RETRYING
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+ # Pass a *different* scheduled_dttm (new crontab window)
+ new_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+
+ AsyncExecuteReportScheduleCommand(TEST_ID, report_schedule.id, new_dttm).run()
+
+ db.session.refresh(report_schedule)
+ # Counter was reset to 0, then incremented to 1 for the new window
+ assert report_schedule.retry_attempt == 1
+ assert report_schedule.last_state == ReportState.RETRYING
+ schedule_retry_mock.assert_called_once()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_success_after_retry_clears_retry_state(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when a retry attempt succeeds, the retry counter
+ and scheduled_dttm are reset.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.last_state = ReportState.RETRYING
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ # Screenshot succeeds this time
+ screenshot_mock.return_value = SCREENSHOT_FILE
+
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.SUCCESS
+ assert report_schedule.retry_attempt == 0
+ assert report_schedule.retry_scheduled_dttm is None
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+def test_find_active_excludes_retrying_reports() -> None:
+ """
+ ReportScheduleDAO.find_active: RETRYING reports are excluded from the
+ scheduler query, but reports with last_state=None (never executed) and
+ stale RETRYING reports are included.
+ """
+ from superset.daos.report import ReportScheduleDAO
+
+ chart = db.session.query(Slice).first()
+
+ # Normal report (NOOP state) — should be included
+ normal = create_report_notification(
+ email_target="a@test.com", chart=chart, name="normal_report"
+ )
+ # Retrying report (fresh) — should be excluded
+ retrying = create_report_notification(
+ email_target="b@test.com",
+ chart=chart,
+ retry_on_failure=True,
+ name="retrying_report",
+ )
+ retrying.last_state = ReportState.RETRYING
+ retrying.last_eval_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None)
+ retrying.retry_scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None)
+ db.session.commit()
+
+ # Stale retrying report — should be re-included
+ stale = create_report_notification(
+ email_target="c@test.com",
+ chart=chart,
+ retry_on_failure=True,
+ name="stale_retrying_report",
+ )
+ stale.last_state = ReportState.RETRYING
+ stale.last_eval_dttm = datetime(2020, 1, 1, 0, 0, 0)
+ stale.retry_scheduled_dttm = datetime(2020, 1, 1, 0, 0, 0)
+ db.session.commit()
+
+ try:
+ active = ReportScheduleDAO.find_active()
+ active_ids = {r.id for r in active}
+
+ assert normal.id in active_ids
+ assert retrying.id not in active_ids
+ assert stale.id in active_ids
Review Comment:
**Suggestion:** The test expects `ReportScheduleDAO.find_active()` to exclude fresh `RETRYING` schedules, but the DAO currently filters only `active` and returns this schedule as well. This assertion will fail consistently; either implement the corresponding DAO filtering in the production change or remove/update this expectation to match the actual contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Active retrying schedules violate the scheduler query contract.
- ⚠️ Scheduler may enqueue redundant executions during retry chains.
- ❌ The integration test fails at the retrying-schedule assertion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ddebbccc03ec4fb18221305b4a934c0c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ddebbccc03ec4fb18221305b4a934c0c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 3072:3074
**Comment:**
*Api Mismatch: The test expects `ReportScheduleDAO.find_active()` to exclude fresh `RETRYING` schedules, but the DAO currently filters only `active` and returns this schedule as well. This assertion will fail consistently; either implement the corresponding DAO filtering in the production change or remove/update this expectation to match the actual contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6e2779d9de338f48ba0d3c0f1e6d3d18137461b4bf8db11764a1aaf20c6aca3a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=6e2779d9de338f48ba0d3c0f1e6d3d18137461b4bf8db11764a1aaf20c6aca3a&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5130995478
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42481:
URL: https://github.com/apache/superset/pull/42481#issuecomment-5092049257
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 446412d954c451f36e22571834f43f70ce194a02 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a675edc3f323e00082acfb5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42481--superset-docs-preview.netlify.app](https://deploy-preview-42481--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCsMgEAXQu8w6IaIkDb1BjzHRLwlYlV8DDaF3r7u3ebdUlndt8hQZZKNmv3drwldz4PkpSkwR2iaiFrYx6pFOYiQar34CairXK_S16PKYEbyLzjoYY1arPm6z_P4) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42481--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3676185433
##########
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
+
+import sqlalchemy as sa
+from alembic import op
+
+from superset.migrations.shared.utils import get_table_column
+
+logger = logging.getLogger("alembic.env")
+
+# revision identifiers, used by Alembic.
+revision = "f3a8c1d2e9b7"
+down_revision = "d3b9a1f6c204"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent Alembic revision chain</b></div>
<div id="fix">
The `down_revision` points to `d3b9a1f6c204` but `Revises` states `e5f6a7b8c9d0`. Both migrations exist and both Revises `e5f6a7b8c9d0`, creating a branching tree rather than a linear chain. Alembic will fail with 'Multiple head migrations are detected' when running migrations. Please reconcile the dependency order.
</div>
</div>
<small><i>Code Review Run #d51984</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_on_failure_schedules_retry(
+ screenshot_mock: Mock,
+ email_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when retry_on_failure is enabled and the report fails,
+ the state transitions to RETRYING and a retry task is enqueued with the
+ correct exponential-backoff delay.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=3,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ # Should NOT re-raise (retry path exits cleanly)
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, datetime.utcnow()
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.RETRYING
+ assert report_schedule.retry_attempt == 1
+ # Verify delay: base=60, attempt=1 → min(60 * 2^1, 3600) = 120
+ schedule_retry_mock.assert_called_once_with(120)
+ # No error email should be sent on the first failure (notification is
+ # sent after a *retry* fails, not after the original failure)
+ email_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retry_exhausted_transitions_to_error(
+ screenshot_mock: Mock,
+ retry_notification_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when all retries are exhausted the state transitions
+ to ERROR, the retry counter is reset, and the retry notification is sent
+ for the final attempt.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=2,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ # Pre-set retry_attempt to the max so the next execution exhausts retries.
+ # Use the same timestamp for both so _is_retry_window_stale() returns False.
+ # Truncate microseconds — MySQL DateTime columns drop them, which would make
+ # the round-tripped value differ from the in-memory one.
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 2
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ db.session.refresh(report_schedule)
+ assert report_schedule.last_state == ReportState.ERROR
+ # Counter is reset after exhaustion
+ assert report_schedule.retry_attempt == 0
+ # No further retry should have been scheduled
+ schedule_retry_mock.assert_not_called()
+ # Retry notification sent for the exhausted attempt (attempt 2 of 2)
+ # The error message is wrapped by the screenshot layer, so use ANY.
+ retry_notification_mock.assert_called_once_with(2, 2, ANY)
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_send_failed_reports_sends_to_recipients(
+ screenshot_mock: Mock,
+ final_failure_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: when send_failed_reports is True and all retries are
+ exhausted, send_final_failure_report is called with the error message.
+ """
+ chart = db.session.query(Slice).first()
+ report_schedule = create_report_notification(
+ email_target="target@email.com",
+ chart=chart,
+ retry_on_failure=True,
+ retry_max_attempts=1,
+ send_failed_reports=True,
+ retry_notify_owners=False,
+ retry_notify_recipients=False,
+ )
+ scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0)
+ report_schedule.retry_attempt = 1
+ report_schedule.retry_scheduled_dttm = scheduled_dttm
+ db.session.commit()
+
+ try:
+ screenshot_mock.side_effect = Exception("screenshot failed")
+
+ with pytest.raises(Exception, match="screenshot failed"):
+ AsyncExecuteReportScheduleCommand(
+ TEST_ID, report_schedule.id, scheduled_dttm
+ ).run()
+
+ # send_final_failure_report should have been called
+ final_failure_mock.assert_called_once_with(ANY)
+ schedule_retry_mock.assert_not_called()
+ finally:
+ cleanup_report_schedule(report_schedule)
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_retrying_state_schedules_another_retry(
+ screenshot_mock: Mock,
+ schedule_retry_mock: Mock,
+) -> None:
+ """
+ ExecuteReport Command: a schedule with last_state=RETRYING is routed to
+ ReportNotTriggeredErrorState, which increments the counter and schedules
+ another retry with the correct delay.
+ """
+ chart = db.session.query(Slice).first()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate test setup code blocks</b></div>
<div id="fix">
Test setup code is duplicated in tests/integration_tests/reports/commands_tests.py at lines 2828-2839 and 2995-3006. Create a shared fixture or helper function for this common test initialization pattern to eliminate duplication.
</div>
</div>
<small><i>Code Review Run #d51984</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/reports/schemas.py:
##########
@@ -278,6 +278,35 @@ class ReportSchedulePostSchema(Schema):
required=False,
dump_default=None,
)
+ retry_on_failure = fields.Boolean(
+ metadata={"description": _("Enable automatic retries on report failure")},
+ load_default=False,
+ )
+ retry_max_attempts = fields.Integer(
+ metadata={
+ "description": _("Maximum number of retry attempts (1–10)"),
+ "example": 3,
+ },
+ load_default=3,
+ required=False,
+ validate=[Range(min=1, max=10, error=_("Must be between 1 and 10"))],
+ )
+ send_failed_reports = fields.Boolean(
+ metadata={
+ "description": _(
+ "Send the failed report to all recipients after retries are exhausted"
+ )
+ },
+ load_default=False,
+ )
+ retry_notify_owners = fields.Boolean(
+ metadata={"description": _("Notify report owners on each retry attempt")},
+ load_default=True,
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for retry config fields</b></div>
<div id="fix">
The diff adds 5 new retry fields (`retry_on_failure`, `retry_max_attempts`, `send_failed_reports`, `retry_notify_owners`, `retry_notify_recipients`) to both schemas, but `schemas_test.py` contains zero tests for any of them. New schema fields require test coverage to prevent regression.
</div>
</div>
<small><i>Code Review Run #d51984</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/reports/schemas.py:
##########
@@ -278,6 +278,35 @@ class ReportSchedulePostSchema(Schema):
required=False,
dump_default=None,
)
+ retry_on_failure = fields.Boolean(
+ metadata={"description": _("Enable automatic retries on report failure")},
+ load_default=False,
+ )
+ retry_max_attempts = fields.Integer(
+ metadata={
+ "description": _("Maximum number of retry attempts (1–10)"),
+ "example": 3,
+ },
+ load_default=3,
+ required=False,
+ validate=[Range(min=1, max=10, error=_("Must be between 1 and 10"))],
+ )
+ send_failed_reports = fields.Boolean(
+ metadata={
+ "description": _(
+ "Send the failed report to all recipients after retries are exhausted"
+ )
+ },
+ load_default=False,
+ )
+ retry_notify_owners = fields.Boolean(
+ metadata={"description": _("Notify report owners on each retry attempt")},
+ load_default=True,
+ )
+ retry_notify_recipients = fields.Boolean(
+ metadata={"description": _("Notify report recipients on each retry attempt")},
+ load_default=False,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate validation method in schemas</b></div>
<div id="fix">
The custom_width validation method is duplicated in superset/reports/schemas.py at lines 308-329 and 514-535. Consider extracting the validation logic into a shared helper method to improve maintainability and reduce code duplication.
</div>
</div>
<small><i>Code Review Run #d51984</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681476518
##########
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:
No, `update_report_schedule_and_log(RETRYING)` does an explicit `db.session.commit()` inside `create_log()` before `_schedule_retry()` runs. So the counter is already committed to DB before the Celery task is enqueued.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681752574
##########
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:
done
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681923092
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,188 @@ 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)
+
+ @staticmethod
+ def _normalize_dttm(dt: Optional[datetime]) -> Optional[datetime]:
+ """Strip timezone info so naive/aware datetimes can be compared."""
+ if dt is not None and dt.tzinfo is not None:
+ return dt.replace(tzinfo=None)
+ return dt
+
+ 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.
+
+ Both sides are normalized to naive datetimes because
+ retry_scheduled_dttm is a plain DateTime column (no timezone), while
+ _scheduled_dttm may be tz-aware depending on the Celery broker.
+ """
+ anchor = self._normalize_dttm(self._report_schedule.retry_scheduled_dttm)
+ current = self._normalize_dttm(self._scheduled_dttm)
+ return anchor is not None and anchor != current
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new count."""
+ self._report_schedule.retry_attempt += 1
+ # Store as naive — the column is a plain DateTime.
+ self._report_schedule.retry_scheduled_dttm = self._normalize_dttm(
+ self._scheduled_dttm
+ )
+ return self._report_schedule.retry_attempt
Review Comment:
Following the existing pattern — last_state, last_eval_dttm, etc. are all updated the same way without row locks.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681895397
##########
superset/commands/report/execute.py:
##########
@@ -1195,10 +1377,26 @@ class ReportNotTriggeredErrorState(BaseReportState):
- Error
"""
- current_states = [ReportState.NOOP, ReportState.ERROR]
+ current_states = [ReportState.NOOP, ReportState.ERROR, ReportState.RETRYING]
initial = True
def next(self) -> None: # noqa: C901
+ # If retries from a previous crontab window are still in-flight and
+ # this is a new crontab trigger, skip — let the active retry chain
+ # finish. The stale-window check uses normalized (naive) datetimes
+ # so tz-aware vs naive mismatches don't cause false positives.
+ if (
+ self._report_schedule.last_state == ReportState.RETRYING
+ and self._is_retry_window_stale()
+ ):
+ logger.info(
+ "Skipping crontab execution for report %s — retries from a "
+ "previous window are still in-flight (execution %s)",
+ self._report_schedule.id,
+ self._execution_id,
+ )
+ return
Review Comment:
Skipping the crontab window while retries are active is intentional. The alternative (running both concurrently) causes duplicate emails and race conditions
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3681962061
##########
superset/reports/notifications/slack_mixin.py:
##########
@@ -47,7 +47,31 @@ def _message_template(
)
@staticmethod
- def _error_template(name: str, description: str, text: str) -> str:
+ def _error_template(
+ name: str,
+ description: str,
+ text: str,
+ retry_attempt: int | None = None,
+ retry_max_attempts: int | None = None,
+ ) -> str:
+ if retry_attempt is not None:
Review Comment:
**Suggestion:** Final-failure notifications pass `retry_max_attempts` but leave `retry_attempt` as `None`; this condition therefore selects the generic error template instead of identifying that all retries were exhausted. Email notifications already distinguish this case, so Slack recipients receive an ambiguous failure message. Handle the final-failure case explicitly when only `retry_max_attempts` is present. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slack final-failure messages omit retry exhaustion status.
- ⚠️ Slack recipients receive ambiguous error notifications.
- ⚠️ Affects both Slack and SlackV2 report delivery.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=72b3c2d51b4443f4b0af9ef657ec556d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=72b3c2d51b4443f4b0af9ef657ec556d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/slack_mixin.py
**Line:** 57:57
**Comment:**
*Logic Error: Final-failure notifications pass `retry_max_attempts` but leave `retry_attempt` as `None`; this condition therefore selects the generic error template instead of identifying that all retries were exhausted. Email notifications already distinguish this case, so Slack recipients receive an ambiguous failure message. Handle the final-failure case explicitly when only `retry_max_attempts` is present.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9bb2bc61b36fc61d84e75b582afaa3bee01563956130d4e7ed4e48315c0a5028&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9bb2bc61b36fc61d84e75b582afaa3bee01563956130d4e7ed4e48315c0a5028&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,188 @@ 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)
+
+ @staticmethod
+ def _normalize_dttm(dt: Optional[datetime]) -> Optional[datetime]:
+ """Strip timezone info so naive/aware datetimes can be compared."""
+ if dt is not None and dt.tzinfo is not None:
+ return dt.replace(tzinfo=None)
+ return dt
+
+ 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.
+
+ Both sides are normalized to naive datetimes because
+ retry_scheduled_dttm is a plain DateTime column (no timezone), while
+ _scheduled_dttm may be tz-aware depending on the Celery broker.
+ """
+ anchor = self._normalize_dttm(self._report_schedule.retry_scheduled_dttm)
+ current = self._normalize_dttm(self._scheduled_dttm)
+ return anchor is not None and anchor != current
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new count."""
+ self._report_schedule.retry_attempt += 1
+ # Store as naive — the column is a plain DateTime.
+ self._report_schedule.retry_scheduled_dttm = self._normalize_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(
+ (self._report_schedule.id, self._scheduled_dttm.isoformat()),
+ countdown=delay_seconds,
+ )
+
+ def send_retry_notification(
+ self, attempt: int, max_attempts: int, error_message: str
+ ) -> None:
+ """
+ Send a per-retry-attempt failure notification to the owners and/or recipients
+ selected via the retry_notify_owners / retry_notify_recipients flags.
+ """
+ recipients: list[ReportRecipients] = []
+
+ if self._report_schedule.retry_notify_owners:
+ recipients.extend(
+ [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=json.dumps({"target": s.user.email}),
+ )
+ for s in self._report_schedule.editors
+ if s.type == SubjectType.USER and s.user
+ ]
+ )
+
+ if self._report_schedule.retry_notify_recipients:
+ recipients.extend(self._report_schedule.recipients)
+
+ if not recipients:
+ return
+
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_attempt=attempt,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, recipients)
+
+ def send_final_failure_report(self, error_message: str) -> None:
+ """
+ Send the failed report notification to all configured recipients after
+ all retry attempts have been exhausted and send_failed_reports is enabled.
+ """
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ max_attempts: int = self._report_schedule.retry_max_attempts
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, self._report_schedule.recipients)
+
+ def _handle_retry_or_error(
+ self, error_message: str, original_exception: Exception
+ ) -> bool:
+ """
+ Shared retry-or-error logic. Returns True if a retry was scheduled
+ (caller should ``return`` without re-raising), or False if the caller
+ should fall through to its own error handling path.
+ """
+ retry_on_failure: bool = self._report_schedule.retry_on_failure
+ if not retry_on_failure:
+ return False
+
+ max_attempts: int = self._report_schedule.retry_max_attempts
+
+ # If a new crontab window has fired since the first failure, reset the
+ # retry counter so this window gets a fresh budget.
+ if self._is_retry_window_stale():
+ self._reset_retry_counter()
+
+ current_attempt = self._report_schedule.retry_attempt
+
+ # If this execution was itself a retry (current_attempt > 0),
+ # send the retry-failure notification *after* the attempt ran so
+ # the email reflects what happened, not what is about to be
+ # scheduled. ("You will receive an update after each retry.")
+ if current_attempt > 0:
+ try:
+ self.send_retry_notification(
+ current_attempt, max_attempts, error_message
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Failed to send retry notification for report schedule "
Review Comment:
**Suggestion:** The first failed execution has `current_attempt == 0`, so it skips `send_retry_notification`; notifications are only sent after retry executions fail. This violates the configured “notify on each failed attempt” behavior because owners or recipients receive no notification for the initial failure. Send the notification for the initial failed attempt as well, while distinguishing it from the retry number shown to users. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Owners miss notifications for initial delivery failures.
- ❌ Recipients miss configured first-attempt failure updates.
- ⚠️ Failure notification history is incomplete.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6aa8cdf30944b98b18ed7d3966c71ba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f6aa8cdf30944b98b18ed7d3966c71ba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1303:1310
**Comment:**
*Incomplete Implementation: The first failed execution has `current_attempt == 0`, so it skips `send_retry_notification`; notifications are only sent after retry executions fail. This violates the configured “notify on each failed attempt” behavior because owners or recipients receive no notification for the initial failure. Send the notification for the initial failed attempt as well, while distinguishing it from the retry number shown to users.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=d0750432cef38985540834114cec0d44e8bb2e1da75b8feadb53d04124a56dd3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=d0750432cef38985540834114cec0d44e8bb2e1da75b8feadb53d04124a56dd3&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,188 @@ 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)
+
+ @staticmethod
+ def _normalize_dttm(dt: Optional[datetime]) -> Optional[datetime]:
+ """Strip timezone info so naive/aware datetimes can be compared."""
+ if dt is not None and dt.tzinfo is not None:
+ return dt.replace(tzinfo=None)
+ return dt
+
+ 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.
+
+ Both sides are normalized to naive datetimes because
+ retry_scheduled_dttm is a plain DateTime column (no timezone), while
+ _scheduled_dttm may be tz-aware depending on the Celery broker.
+ """
+ anchor = self._normalize_dttm(self._report_schedule.retry_scheduled_dttm)
+ current = self._normalize_dttm(self._scheduled_dttm)
+ return anchor is not None and anchor != current
Review Comment:
**Suggestion:** Comparing exact datetimes after merely removing timezone metadata is not stable across Celery and database round trips. The database `DateTime` column can truncate microseconds, so a retry can load an anchor that differs from the original `scheduled_dttm`; every retry is then classified as a stale window, resetting the counter and potentially granting an unbounded retry sequence. Normalize to a precision-safe window identifier or compare timestamps with an appropriate tolerance. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MySQL retries can receive an unbounded retry sequence.
- ❌ Failed reports may repeatedly execute beyond configured limits.
- ⚠️ Retry state and execution logs become misleading.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3cf0350dff7041a4954c41b5f2f52aeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3cf0350dff7041a4954c41b5f2f52aeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 1191:1193
**Comment:**
*Logic Error: Comparing exact datetimes after merely removing timezone metadata is not stable across Celery and database round trips. The database `DateTime` column can truncate microseconds, so a retry can load an anchor that differs from the original `scheduled_dttm`; every retry is then classified as a stale window, resetting the counter and potentially granting an unbounded retry sequence. Normalize to a precision-safe window identifier or compare timestamps with an appropriate tolerance.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9b278a741d0d10424b97eb13a0cab9c866cda2facf5df81f980f3758dfe962c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=9b278a741d0d10424b97eb13a0cab9c866cda2facf5df81f980f3758dfe962c7&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-07-28_00-00_f3a8c1d2e9b7_add_report_retry_state_columns.py:
##########
@@ -0,0 +1,94 @@
+# 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: d3b9a1f6c204
+Create Date: 2026-07-28 00:00:00.000000
+
+"""
+
+import logging
+
+import sqlalchemy as sa
+from alembic import op
+
+from superset.migrations.shared.utils import get_table_column
+
+logger = logging.getLogger("alembic.env")
+
+# revision identifiers, used by Alembic.
+revision = "f3a8c1d2e9b7"
+down_revision = "d3b9a1f6c204"
+
+# Configuration columns (user-configurable)
+_CONFIG_COLUMNS = [
+ ("retry_on_failure", sa.Boolean(), False, "0"),
+ ("retry_max_attempts", sa.Integer(), False, "3"),
+ ("send_failed_reports", sa.Boolean(), False, "0"),
+ ("retry_notify_owners", sa.Boolean(), False, "1"),
+ ("retry_notify_recipients", sa.Boolean(), False, "0"),
+]
+
+# State columns (written by the execution engine)
+_STATE_COLUMNS = [
+ ("retry_attempt", sa.Integer(), False, "0"),
+ ("retry_scheduled_dttm", sa.DateTime(), True, None),
+]
+
+
+def upgrade() -> None:
+ """Add retry config and state columns to report_schedule."""
+ all_columns = _CONFIG_COLUMNS + _STATE_COLUMNS
+ missing = [
+ (name, col_type, nullable, default)
+ for name, col_type, nullable, default in all_columns
+ if get_table_column("report_schedule", name) is None
+ ]
+
+ if not missing:
+ logger.info("All retry columns already exist. Skipping migration.")
+ return
+
+ with op.batch_alter_table("report_schedule") as batch_op:
+ for name, col_type, nullable, default in missing:
+ batch_op.add_column(
+ sa.Column(
+ name,
+ col_type,
+ nullable=nullable,
+ server_default=default,
+ )
+ )
+
+
+def downgrade() -> None:
+ """Remove retry config and state columns from report_schedule."""
+ all_columns = _CONFIG_COLUMNS + _STATE_COLUMNS
+ present = [
+ name
+ for name, _, _, _ in all_columns
+ if get_table_column("report_schedule", name) is not None
+ ]
+
+ if not present:
+ logger.info("No retry columns found. Skipping downgrade.")
+ return
+
+ with op.batch_alter_table("report_schedule") as batch_op:
+ for name in present:
+ batch_op.drop_column(name)
Review Comment:
**Suggestion:** The downgrade drops every retry column that happens to exist, including columns that were already present before this migration or were created by a partial/manual repair. Because the upgrade intentionally tolerates pre-existing columns, the downgrade can destroy configuration and state data it does not own. Track which columns this revision actually added, or make downgrade ownership-aware. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Downgrade can destroy retry configuration data.
- ❌ Partial repairs lose state unexpectedly.
- ⚠️ Rollback behavior differs from upgrade ownership.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=033b1454d700403fb13273c695156310&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=033b1454d700403fb13273c695156310&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-07-28_00-00_f3a8c1d2e9b7_add_report_retry_state_columns.py
**Line:** 82:94
**Comment:**
*Possible Bug: The downgrade drops every retry column that happens to exist, including columns that were already present before this migration or were created by a partial/manual repair. Because the upgrade intentionally tolerates pre-existing columns, the downgrade can destroy configuration and state data it does not own. Track which columns this revision actually added, or make downgrade ownership-aware.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=e90d6e16ffea4e1e56b3d9404426216ecccc3eba3a440d87887a223f037205fc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42481&comment_hash=e90d6e16ffea4e1e56b3d9404426216ecccc3eba3a440d87887a223f037205fc&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(reports): add retry on failure support for alerts and reports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3690563015
##########
superset-frontend/src/features/alerts/components/AlertStatusIcon.tsx:
##########
@@ -92,6 +94,11 @@ export default function AlertStatusIcon({
lastStateConfig.label = t('Alert Triggered, In Grace Period');
lastStateConfig.status = AlertState.Grace;
break;
+ case AlertState.Retrying:
+ lastStateConfig.icon = Icons.Running;
+ lastStateConfig.label = t('Report retrying');
+ lastStateConfig.status = AlertState.Retrying;
+ break;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent conditional handling</b></div>
<div id="fix">
Removing the `isReportEnabled` conditional from the Retrying case creates inconsistent behavior compared to all other alert states (Success, Working, Error, Noop). When `isReportEnabled=false` (regular alerts), users will now see 'Report retrying' which is semantically incorrect. The label should remain conditional like the other states in this switch.
</div>
</div>
<small><i>Code Review Run #ce0668</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org