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: [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]
