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
+# ---------------------------------------------------------------------------
+
+
[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()
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: [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]
