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
+# ---------------------------------------------------------------------------
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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
+# ---------------------------------------------------------------------------
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]",
+ 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)
+
+
[email protected]("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="[email protected]", chart=chart, name="normal_report"
+ )
+ # Retrying report (fresh) — should be excluded
+ retrying = create_report_notification(
+ email_target="[email protected]",
+ 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="[email protected]",
+ 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: [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]
