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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
