codeant-ai-for-open-source[bot] commented on code in PR #42624:
URL: https://github.com/apache/superset/pull/42624#discussion_r3694277001
##########
superset/commands/report/execute.py:
##########
@@ -639,7 +856,20 @@ def _get_pdf(self) -> bytes:
:raises: ReportSchedulePdfFailedError
"""
screenshots = self._get_screenshots()
+ reserve_seconds = (
+ self._report_execution_context.post_capture_reserve_seconds
+ if self._report_execution_context
+ else 0.0
+ )
+ self._phase_timeout(
+ "pdf_generation",
+ reserve_seconds=reserve_seconds,
+ )
pdf = build_pdf_from_screenshots(screenshots)
+ self._phase_timeout(
+ "pdf_generation",
+ reserve_seconds=reserve_seconds,
+ )
Review Comment:
**Suggestion:** The deadline is checked only before and after the
synchronous `build_pdf_from_screenshots` call, so PDF conversion itself has no
cooperative timeout. A large or pathological screenshot set can consume the
remaining budget and cleanup reserve before the second check runs, allowing the
Celery hard limit to terminate the task before terminal state persistence. PDF
generation needs a bounded operation or periodic deadline checks that preserve
the cleanup window. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Large PDF reports can exceed Celery hard limits.
- ⚠️ Terminal ERROR persistence may not execute.
- ⚠️ Timed-out executions can remain WORKING until recovery.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=907ef921497446f6a5e563c125e61bfb&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=907ef921497446f6a5e563c125e61bfb&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:** 864:872
**Comment:**
*Possible Bug: The deadline is checked only before and after the
synchronous `build_pdf_from_screenshots` call, so PDF conversion itself has no
cooperative timeout. A large or pathological screenshot set can consume the
remaining budget and cleanup reserve before the second check runs, allowing the
Celery hard limit to terminate the task before terminal state persistence. PDF
generation needs a bounded operation or periodic deadline checks that preserve
the cleanup window.
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%2F42624&comment_hash=118eec2383619acfd93a1642c9cb9505bb6f746422cd5b1f4aa9018142f02689&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=118eec2383619acfd93a1642c9cb9505bb6f746422cd5b1f4aa9018142f02689&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -123,6 +131,137 @@ def resolve_executor_user(model: ReportSchedule) ->
tuple["User", str]:
return user, username
+def log_report_delivery_phase(
+ report_context: ReportExecutionContext | None,
+ recipient_type: ReportRecipientType | None,
+ phase: str,
+ *,
+ enforce_budget: bool,
+) -> None:
+ """Enforce and log a notification phase when executing a report."""
+
+ if report_context is None:
+ return
+ deadline = report_context.deadline
+ if enforce_budget:
+ deadline.timeout_seconds(
+ "notification_delivery",
+ reserve_seconds=report_context.cleanup_reserve_seconds,
+ )
+ logger.info(
+ "report_delivery_%s %s recipient_type=%s elapsed_seconds=%.2f "
+ "remaining_seconds=%.2f",
+ phase,
+ report_context.log_context,
+ recipient_type,
+ deadline.elapsed_seconds,
+ deadline.remaining_seconds,
+ )
+
+
+def persist_owned_report_execution_terminal_error(
+ report_schedule_id: int,
+ execution_id: UUID,
+ error_message: str,
+ terminal_reason: str,
+ report_context: ReportExecutionContext | None = None,
+) -> bool:
+ """
+ Terminalize this command's WORKING row from its application-owned boundary.
+
+ Report states normally persist their terminal result before re-raising. If
+ that first write loses its transaction or database connection, the command
+ boundary is the last safe in-process retry: it still has Flask application
+ context and knows the execution UUID it owns. A compare against the latest
+ active WORKING row prevents an old worker from changing the schedule state
+ after a newer execution has started.
+ """
+
+ try:
+ # The state-machine transaction has already rolled back on its way to
+ # this boundary. Roll back again so a failed terminal flush cannot
leave
+ # the scoped session unusable for the retry.
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ working_log = (
+ db.session.query(ReportExecutionLog)
+ .filter(
+ ReportExecutionLog.report_schedule_id == report_schedule_id,
+ ReportExecutionLog.uuid == execution_id,
+ ReportExecutionLog.state == ReportState.WORKING,
+ ReportExecutionLog.error_message.is_(None),
+ )
+ .first()
+ )
+ if working_log is None:
+ return False
+
+ latest_working_log = (
+ db.session.query(ReportExecutionLog)
+ .filter(
+ ReportExecutionLog.report_schedule_id == report_schedule_id,
+ ReportExecutionLog.state == ReportState.WORKING,
+ ReportExecutionLog.error_message.is_(None),
+ )
+ .order_by(ReportExecutionLog.end_dttm.desc())
+ .first()
+ )
+ report_schedule = working_log.report_schedule
+ owns_schedule_state = (
+ report_schedule.last_state == ReportState.WORKING
+ and latest_working_log is not None
+ and latest_working_log.uuid == execution_id
+ )
Review Comment:
**Suggestion:** The ownership check is a non-atomic read-modify-write: after
`latest_working_log` and `report_schedule.last_state` are read, a newer
execution can start and establish a different active WORKING row before this
commit. The stale retry can then overwrite `report_schedule.last_state` with
ERROR, incorrectly aborting the newer execution. Perform the ownership check
and terminal update under a row lock or use a compare-and-swap update that
verifies the active execution UUID. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Newer scheduled execution can be marked ERROR by stale worker.
- ⚠️ Schedule remains blocked or requires timeout recovery.
- ⚠️ Execution history can report an incorrect terminal owner.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab3a153463244fd694e677b1553ef646&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=ab3a153463244fd694e677b1553ef646&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:** 198:213
**Comment:**
*Race Condition: The ownership check is a non-atomic read-modify-write:
after `latest_working_log` and `report_schedule.last_state` are read, a newer
execution can start and establish a different active WORKING row before this
commit. The stale retry can then overwrite `report_schedule.last_state` with
ERROR, incorrectly aborting the newer execution. Perform the ownership check
and terminal update under a row lock or use a compare-and-swap update that
verifies the active execution UUID.
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%2F42624&comment_hash=7be1a60497c625d36fa3ab1da4244e925991927bc1199c9b0db5fdea8848309d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=7be1a60497c625d36fa3ab1da4244e925991927bc1199c9b0db5fdea8848309d&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1048,10 +1314,17 @@ def _send(
:raises: CommandException
"""
notification_errors: list[SupersetError] = []
+ report_context = getattr(self, "_report_execution_context", None)
for recipient in recipients:
notification = create_notification(recipient, notification_content)
try:
try:
+ log_report_delivery_phase(
+ report_context,
+ getattr(recipient, "type", None),
+ "start",
+ enforce_budget=True,
+ )
Review Comment:
**Suggestion:** The delivery gate reserves only `cleanup_reserve_seconds`;
it does not reserve `delivery_reserve_seconds`, and `notification.send()`
receives no deadline-derived timeout. A slow email, Slack, or webhook delivery
can therefore consume the delivery and cleanup windows, leaving insufficient
time to persist the terminal report state despite the shared budget. Reserve
both delivery and cleanup capacity and pass a bounded timeout to notification
implementations where supported. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Slow notification can exhaust terminal cleanup time.
- ⚠️ Report ERROR or SUCCESS state may not persist.
- ⚠️ Sequential recipients amplify delivery overrun risk.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a9133d9f6a3347e28dfb60ed9afbc26a&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=a9133d9f6a3347e28dfb60ed9afbc26a&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:** 1322:1327
**Comment:**
*Possible Bug: The delivery gate reserves only
`cleanup_reserve_seconds`; it does not reserve `delivery_reserve_seconds`, and
`notification.send()` receives no deadline-derived timeout. A slow email,
Slack, or webhook delivery can therefore consume the delivery and cleanup
windows, leaving insufficient time to persist the terminal report state despite
the shared budget. Reserve both delivery and cleanup capacity and pass a
bounded timeout to notification implementations where supported.
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%2F42624&comment_hash=16e154d84cd83c5d4151fd7e0e35aa26e483fe58a96f7f48016f58eae70cba21&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=16e154d84cd83c5d4151fd7e0e35aa26e483fe58a96f7f48016f58eae70cba21&reaction=dislike'>👎</a>
##########
superset/utils/report_execution.py:
##########
@@ -0,0 +1,269 @@
+# 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.
+
+"""Shared deadline and logging context for scheduled report execution."""
+
+from __future__ import annotations
+
+import logging
+import time
+from collections.abc import Callable, Mapping
+from dataclasses import dataclass, field
+from typing import Any
+from uuid import UUID
+
+logger = logging.getLogger(__name__)
+
+# Minimum working allowance kept above the summed phase reserves when a
+# per-schedule working_timeout would otherwise squeeze the effective budget
+# below what the execution context can represent. A floored budget still
+# fails fast (budget-exceeded on the first phase) rather than erroring while
+# constructing the deadline.
+MIN_REPORT_EXECUTION_WORK_SECONDS = 30.0
+
+
+def validate_report_execution_config(config: Mapping[str, Any]) -> None:
+ """Validate the scheduled-report budget invariant during application
startup."""
+
+ budget = float(config["ALERT_REPORTS_EXECUTION_BUDGET_SECONDS"])
+ reserves = (
+ float(config["ALERT_REPORTS_EXECUTION_CAPTURE_RESERVE_SECONDS"]),
+ float(config["ALERT_REPORTS_EXECUTION_DELIVERY_RESERVE_SECONDS"]),
+ float(config["ALERT_REPORTS_EXECUTION_CLEANUP_RESERVE_SECONDS"]),
+ )
+ hard_timeout_grace = float(
+ config["ALERT_REPORTS_EXECUTION_HARD_TIMEOUT_GRACE_SECONDS"]
+ )
+
+ if budget <= 0:
+ raise ValueError("Report execution budget must be greater than zero")
+ if any(reserve < 0 for reserve in reserves):
+ raise ValueError("Report execution phase reserves cannot be negative")
+ if sum(reserves) >= budget:
+ raise ValueError(
+ "Report execution phase reserves must total less than the
execution budget"
+ )
+ if hard_timeout_grace < 0:
+ raise ValueError("Report execution hard-timeout grace cannot be
negative")
+
+
+def resolve_report_execution_budget_seconds(
+ config: Mapping[str, Any],
+ working_timeout: int | None = None,
+) -> float:
+ """Return the effective execution budget for one REPORT schedule.
+
+ The per-schedule ``working_timeout`` keeps its historical, user-facing
+ meaning ("kill my report after N seconds"): when it is lower than the
+ global ``ALERT_REPORTS_EXECUTION_BUDGET_SECONDS`` it caps the budget, so
+ introducing the global deadline does not silently grant a schedule more
+ time than its owner configured. The result is floored at the summed
+ phase reserves plus a minimal working allowance so the execution context
+ remains constructible; a floored budget fails cleanly on its first phase
+ check instead of raising at setup.
+ """
+ budget = float(config["ALERT_REPORTS_EXECUTION_BUDGET_SECONDS"])
+ if working_timeout is not None:
+ budget = min(budget, float(working_timeout))
+ reserves_total = (
+ float(config["ALERT_REPORTS_EXECUTION_CAPTURE_RESERVE_SECONDS"])
+ + float(config["ALERT_REPORTS_EXECUTION_DELIVERY_RESERVE_SECONDS"])
+ + float(config["ALERT_REPORTS_EXECUTION_CLEANUP_RESERVE_SECONDS"])
+ )
+ if budget < (min_viable := reserves_total +
MIN_REPORT_EXECUTION_WORK_SECONDS):
+ logger.warning(
+ "Report working_timeout=%s is below the minimum viable execution "
+ "budget (%.0fs phase reserves + %.0fs working allowance); "
+ "flooring the effective budget at %.0fs.",
+ working_timeout,
+ reserves_total,
+ MIN_REPORT_EXECUTION_WORK_SECONDS,
+ min_viable,
+ )
+ return min_viable
Review Comment:
**Suggestion:** When a schedule's `working_timeout` is smaller than the
phase reserves, this floors the effective budget above the user-configured
limit. The resulting value is used for Celery's soft and hard limits and
stale-working detection, so a schedule configured to stop after a short
interval can continue for `reserves_total + 30` seconds instead of honoring its
configured timeout. Preserve the configured cap and fail the execution cleanly
when it cannot accommodate the reserves. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Report schedules can exceed their configured working timeout.
- ⚠️ Celery soft and hard limits no longer honor short schedules.
- ⚠️ Stale WORKING recovery is delayed beyond user configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d4b6a9d81bd74520827033141e44c5e7&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=d4b6a9d81bd74520827033141e44c5e7&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/utils/report_execution.py
**Line:** 86:96
**Comment:**
*Logic Error: When a schedule's `working_timeout` is smaller than the
phase reserves, this floors the effective budget above the user-configured
limit. The resulting value is used for Celery's soft and hard limits and
stale-working detection, so a schedule configured to stop after a short
interval can continue for `reserves_total + 30` seconds instead of honoring its
configured timeout. Preserve the configured cap and fail the execution cleanly
when it cannot accommodate the reserves.
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%2F42624&comment_hash=1c9e0abdd8fbff115142ee6e73dc778ad727043a2724aaf50eba80ade789b19b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=1c9e0abdd8fbff115142ee6e73dc778ad727043a2724aaf50eba80ade789b19b&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]
