fitzee commented on code in PR #44336:
URL: https://github.com/apache/superset/pull/44336#discussion_r4060269532
##########
superset/reports/notifications/email.py:
##########
@@ -148,12 +148,8 @@ def _get_smtp_domain() -> str:
return parseaddr(current_app.config["SMTP_MAIL_FROM"])[1].split("@")[1]
def _error_template(self, text: str) -> str:
- # The error text is derived from exception messages that can embed
- # data-controlled content (e.g. crafted table/column names in a DB
- # error). Strip all HTML before interpolating it into the email body,
- # matching the sanitization applied to the normal content path.
- # pylint: disable=no-member
- safe_text = nh3.clean(text, tags=set(), attributes={})
+ # Diagnostics remain in execution history, not outbound notifications.
+ safe_text = __("Contact the report owner for error details.")
Review Comment:
Fixed in ba4ae23ba3a37f74ad7b09ecba5b46c182982565. `send_error` marks its
editor-only email explicitly, and that path retains `nh3`-sanitized
diagnostics. Configured-recipient and retry/final-failure notifications remain
redacted. Tests verify both HTML sanitization and that diagnostic emails go
only to editors.
##########
superset/commands/report/execute.py:
##########
@@ -142,26 +149,8 @@ def resolve_executor_user(model: ReportSchedule) ->
tuple["User", str]:
def _should_build_execution_context(model: ReportSchedule) -> bool:
- """
- Whether an execution should run under a :class:`ReportExecutionContext`.
-
- Reports always do — their behavior is unchanged. Alerts join them only when
- they deliver a rendered PNG/PDF screenshot to recipients, which happens
when
- ``ALERTS_ATTACH_REPORTS`` is enabled. Delivered screenshots must fail
closed:
- the context selects the fail-closed readiness predicate and disables
- partial-tile fallback, so a blank or incomplete capture raises instead of
- being delivered.
-
- CSV/text alerts, alerts without the attach flag, the non-delivered
- query-context capture, and UI thumbnails are deliberately excluded and keep
- their lenient capture contract.
- """
- if model.type == ReportScheduleType.REPORT:
- return True
- return model.report_format in (
- ReportDataFormat.PNG,
- ReportDataFormat.PDF,
- ) and feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+ """Give every scheduled report and alert a shared deadline and ownership
context."""
+ return model.type in (ReportScheduleType.REPORT, ReportScheduleType.ALERT)
Review Comment:
Fixed in ba4ae23ba3a37f74ad7b09ecba5b46c182982565. Ownership context no
longer selects the capture policy by itself. CSV/XLSX/TEXT alerts pass no
report capture context to the query-context bootstrap screenshot, preserving
their prior contract. Rendered PNG/PDF alerts with `ALERTS_ATTACH_REPORTS`
still use fail-closed capture. Added coverage for the actual screenshot call
and the format/flag matrix.
##########
superset/commands/report/execute.py:
##########
@@ -2159,34 +2333,51 @@ class AsyncExecuteReportScheduleCommand(BaseCommand):
- On Alerts uses related Command AlertCommand and sends configured
notifications
"""
- def __init__(self, task_id: str, model_id: int, scheduled_dttm: datetime):
+ def __init__(
+ self,
+ task_id: str,
+ model_id: int,
+ scheduled_dttm: datetime,
+ *,
+ is_retry: bool = False,
+ expected_owner: str | None = None,
+ ):
self._model_id = model_id
self._model: Optional[ReportSchedule] = None
self._scheduled_dttm = scheduled_dttm
self._execution_id = UUID(task_id)
+ self._is_retry = is_retry
+ self._expected_owner = expected_owner
- def run(self) -> None:
+ def run(self) -> None: # noqa: C901
monotonic_started_at = time.monotonic()
report_execution_context: ReportExecutionContext | None = None
- owns_report_working_state = False
+ owns_working_state = False
try:
self.validate()
if not self._model:
raise ReportScheduleExecuteUnexpectedError()
- # Reports always run under an execution context; alerts join them
- # only when they deliver a rendered screenshot, so a blank/partial
- # capture fails closed instead of being delivered. Ownership and
- # terminal-error persistence remain report-only recovery semantics.
+ if self._is_retry and (
+ not
feature_flag_manager.is_feature_enabled("ALERT_REPORTS_RETRY")
+ or not self._model.retry_on_failure
+ or self._model.last_state != ReportState.RETRYING
+ or normalize_window(self._model.retry_scheduled_dttm)
+ != normalize_window(self._scheduled_dttm)
+ ):
+ logger.info(
+ "report_retry_discarded report_schedule_id=%s
execution_id=%s",
+ self._model_id,
+ self._execution_id,
+ )
+ return
Review Comment:
Fixed in ba4ae23ba3a37f74ad7b09ecba5b46c182982565. A disabled queued retry
performs an owner/window/state-guarded update to ERROR and clears its retry
counter/anchor. The execution owner/window remain as replay fences. If the flag
is still enabled, the UPDATE also rechecks that the schedule opt-in is
disabled. SQLite/PostgreSQL tests cover old owners, mismatched windows,
re-enabled schedules, and admission of the next cron window.
##########
superset/commands/report/execute.py:
##########
@@ -2262,11 +2454,44 @@ def run(self) -> None:
self._execution_id,
report_execution_context,
).get_dashboard_urls()
+ execution_claim = None
+ if self._model.last_state != ReportState.WORKING:
+ execution_claim = claim_execution(
+ db.session,
+ self._model.id,
+ str(self._execution_id),
+ self._scheduled_dttm,
+ is_retry=self._is_retry,
+ expected_owner=self._expected_owner,
+
retries_enabled=feature_flag_manager.is_feature_enabled(
+ "ALERT_REPORTS_RETRY"
+ ),
+ stale_retry_seconds=(
+ app.config.get(
+ "ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS", 3600
+ )
+ + resolve_report_execution_budget_seconds(
Review Comment:
Fixed in ba4ae23ba3a37f74ad7b09ecba5b46c182982565. Claim admission uses
`ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS` without adding the execution budget.
This matches the recovery threshold. Tests exercise admission immediately
before and after the one-hour boundary on SQLite and PostgreSQL.
##########
superset/commands/report/execute.py:
##########
@@ -2159,34 +2333,51 @@ class AsyncExecuteReportScheduleCommand(BaseCommand):
- On Alerts uses related Command AlertCommand and sends configured
notifications
"""
- def __init__(self, task_id: str, model_id: int, scheduled_dttm: datetime):
+ def __init__(
+ self,
+ task_id: str,
+ model_id: int,
+ scheduled_dttm: datetime,
+ *,
+ is_retry: bool = False,
+ expected_owner: str | None = None,
+ ):
self._model_id = model_id
self._model: Optional[ReportSchedule] = None
self._scheduled_dttm = scheduled_dttm
self._execution_id = UUID(task_id)
+ self._is_retry = is_retry
+ self._expected_owner = expected_owner
- def run(self) -> None:
+ def run(self) -> None: # noqa: C901
monotonic_started_at = time.monotonic()
report_execution_context: ReportExecutionContext | None = None
- owns_report_working_state = False
+ owns_working_state = False
try:
self.validate()
if not self._model:
raise ReportScheduleExecuteUnexpectedError()
- # Reports always run under an execution context; alerts join them
- # only when they deliver a rendered screenshot, so a blank/partial
- # capture fails closed instead of being delivered. Ownership and
- # terminal-error persistence remain report-only recovery semantics.
+ if self._is_retry and (
+ not
feature_flag_manager.is_feature_enabled("ALERT_REPORTS_RETRY")
+ or not self._model.retry_on_failure
+ or self._model.last_state != ReportState.RETRYING
+ or normalize_window(self._model.retry_scheduled_dttm)
+ != normalize_window(self._scheduled_dttm)
+ ):
+ logger.info(
+ "report_retry_discarded report_schedule_id=%s
execution_id=%s",
+ self._model_id,
+ self._execution_id,
+ )
+ return
+
+ # All scheduled executions share ownership and retry fencing.
if _should_build_execution_context(self._model):
# An invocation that enters on WORKING is a duplicate or stale
# recovery, not the owner that created the active row. Its
state
# handler may terminalize a stale execution, but the command
# boundary must never infer ownership from a replayed UUID.
- owns_report_working_state = (
- self._model.type == ReportScheduleType.REPORT
- and self._model.last_state != ReportState.WORKING
- )
total_seconds = resolve_report_execution_budget_seconds(
Review Comment:
Fixed in ba4ae23ba3a37f74ad7b09ecba5b46c182982565. Alert execution uses its
existing Celery soft limit (working timeout plus lag), or no overall
application deadline when that limit is disabled/absent. It does not inherit
the report-only global budget or phase reserves. Rendered attachments use a
separate finite browser budget capped by remaining alert time, so an unlimited
alert cannot pass infinity to Playwright. That phase shares sticky rejection
state with the parent execution. Tests cover two-hour alerts, absent/disabled
limits, and rejection propagation.
--
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]
