codeant-ai-for-open-source[bot] commented on code in PR #42624:
URL: https://github.com/apache/superset/pull/42624#discussion_r3694363953
##########
superset/utils/webdriver.py:
##########
@@ -380,11 +440,15 @@ def _wait_for_charts_ready(
elapsed,
context_suffix,
)
- readiness_predicate = (
- CHART_CONTAINER_READY_JS
- if element_name == "chart-container"
- else CHART_HOLDERS_READY_JS
- )
+ if element_name == "chart-container":
+ readiness_predicate = CHART_CONTAINER_READY_JS
+ elif report_execution_context:
+ readiness_predicate = REPORT_CHART_HOLDERS_READY_JS
Review Comment:
**Suggestion:** The report readiness predicate only verifies chart holders
intersecting the current viewport, but the subsequent standard screenshot
captures the entire dashboard element, including below-the-fold content. As a
result, lower charts can still be unmounted, loading, or blank when the
full-page screenshot is delivered. Either ensure the full dashboard is
progressively scrolled and each viewport is validated, or use the tiled capture
path for reports that require full-dashboard readiness. [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scheduled dashboard reports can omit lower charts.
- ⚠️ Standard captures may deliver partially rendered dashboards.
- ⚠️ Virtualized dashboard content is not validated before delivery.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8082733238c40b68a4e3fee1f00206d&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=f8082733238c40b68a4e3fee1f00206d&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/webdriver.py
**Line:** 445:446
**Comment:**
*Incorrect Condition Logic: The report readiness predicate only
verifies chart holders intersecting the current viewport, but the subsequent
standard screenshot captures the entire dashboard element, including
below-the-fold content. As a result, lower charts can still be unmounted,
loading, or blank when the full-page screenshot is delivered. Either ensure the
full dashboard is progressively scrolled and each viewport is validated, or use
the tiled capture path for reports that require full-dashboard readiness.
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=916be032ad47ed19e4a47e4ada32f01be96658b42ac74c6b33c85b3a4f6c2be4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=916be032ad47ed19e4a47e4ada32f01be96658b42ac74c6b33c85b3a4f6c2be4&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
+ )
+ ended_at = datetime.now(timezone.utc).replace(tzinfo=None)
+ working_log.state = ReportState.ERROR
+ working_log.error_message = error_message
+ working_log.end_dttm = ended_at
+ if owns_schedule_state:
+ report_schedule.last_state = ReportState.ERROR
+ report_schedule.last_eval_dttm = ended_at
+
+ db.session.commit() # pylint: disable=consider-using-transaction
Review Comment:
**Suggestion:** Race condition: the latest WORKING log and
`report_schedule.last_state` are read without a row lock or conditional update,
then the schedule is committed later. A newer execution can become WORKING
after this check but before the commit, allowing an older worker's retry to set
the schedule to ERROR and overwrite the newer execution's state. Lock the
schedule/latest log row or make the terminal update conditional on the
execution UUID in the same transaction. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Newer report execution can lose WORKING schedule state.
- ⚠️ Stale recovery may run against the wrong execution.
- ⚠️ Concurrent report scheduling can produce inconsistent audit state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ca21702eecba438ea61a7494eb6b0b41&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=ca21702eecba438ea61a7494eb6b0b41&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:** 196:222
**Comment:**
*Race Condition: Race condition: the latest WORKING log and
`report_schedule.last_state` are read without a row lock or conditional update,
then the schedule is committed later. A newer execution can become WORKING
after this check but before the commit, allowing an older worker's retry to set
the schedule to ERROR and overwrite the newer execution's state. Lock the
schedule/latest log row or make the terminal update conditional on the
execution UUID in the same transaction.
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=7cbc96d670648a79dc775c8ef1b18ea7d79eb8e7184434a7c5d6d9ab537407ae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=7cbc96d670648a79dc775c8ef1b18ea7d79eb8e7184434a7c5d6d9ab537407ae&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -1472,11 +1808,62 @@ def __init__(self, task_id: str, model_id: int,
scheduled_dttm: datetime):
self._execution_id = UUID(task_id)
def run(self) -> None:
+ monotonic_started_at = time.monotonic()
+ report_execution_context: ReportExecutionContext | None = None
+ owns_report_working_state = False
try:
self.validate()
if not self._model:
raise ReportScheduleExecuteUnexpectedError()
+ if self._model.type == ReportScheduleType.REPORT:
+ # 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.last_state != ReportState.WORKING
+ )
+ total_seconds = resolve_report_execution_budget_seconds(
+ app.config,
+ working_timeout=self._model.working_timeout,
+ )
Review Comment:
**Suggestion:** The resolved budget no longer honors the schedule's
configured `working_timeout` when that value is below the reserve floor. For
example, a schedule with the valid configured timeout of one second is assigned
at least the summed reserves plus 30 seconds, and the Celery limits and
deadline therefore permit execution well beyond the owner's timeout. Either
reject such configurations or preserve the per-schedule timeout as the
effective cap instead of flooring it above that value. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ REPORT schedules can exceed configured execution limits.
- ⚠️ Celery workers remain occupied beyond owner expectations.
- ⚠️ Slow reports delay subsequent scheduled executions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5531f7b016544bd18bbc91815f50853e&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=5531f7b016544bd18bbc91815f50853e&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:** 1827:1830
**Comment:**
*Api Mismatch: The resolved budget no longer honors the schedule's
configured `working_timeout` when that value is below the reserve floor. For
example, a schedule with the valid configured timeout of one second is assigned
at least the summed reserves plus 30 seconds, and the Celery limits and
deadline therefore permit execution well beyond the owner's timeout. Either
reject such configurations or preserve the per-schedule timeout as the
effective cap instead of flooring it above that value.
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=27618c732b9e81a2a424c4d0e361d38d88897304e3cbdc4310af8bb7ca180002&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=27618c732b9e81a2a424c4d0e361d38d88897304e3cbdc4310af8bb7ca180002&reaction=dislike'>👎</a>
##########
superset/tasks/scheduler.py:
##########
@@ -98,19 +100,14 @@ def scheduler(self: Task) -> None: # pylint:
disable=unused-argument
triggered_at, active_schedule.crontab, active_schedule.timezone
):
logger.info("Scheduling alert %s eta: %s", active_schedule.name,
schedule)
- async_options = {"eta": schedule}
- if (
- active_schedule.working_timeout is not None
- and current_app.config["ALERT_REPORTS_WORKING_TIME_OUT_KILL"]
- ):
- async_options["time_limit"] = (
- active_schedule.working_timeout
- + current_app.config["ALERT_REPORTS_WORKING_TIME_OUT_LAG"]
- )
- async_options["soft_time_limit"] = (
- active_schedule.working_timeout
- +
current_app.config["ALERT_REPORTS_WORKING_SOFT_TIME_OUT_LAG"]
- )
+ async_options = {
+ "eta": schedule,
+ **get_report_task_timeout_options(
+ is_report=active_schedule.type ==
ReportScheduleType.REPORT,
+ working_timeout=active_schedule.working_timeout,
+ config=current_app.config,
+ ),
+ }
execute.apply_async((active_schedule.id,), **async_options)
Review Comment:
**Suggestion:** Passing every report's `working_timeout` through
`get_report_task_timeout_options` can produce a Celery timeout longer than the
schedule's configured cap: the helper floors values below the
reserve-plus-working minimum instead of preserving the requested cap. For
example, a report configured with a 10-second `working_timeout` receives a
210-second soft limit, so stalled executions can run well past the
user-configured timeout. The report timeout calculation must never exceed the
schedule's `working_timeout`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Stalled scheduled reports exceed configured execution limits.
- ⚠️ Celery workers remain occupied beyond schedule settings.
- ⚠️ Manual report execution shares the same timeout mismatch.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d4905d65b9474c8c9109c9bdcdf343f2&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=d4905d65b9474c8c9109c9bdcdf343f2&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/tasks/scheduler.py
**Line:** 103:111
**Comment:**
*Logic Error: Passing every report's `working_timeout` through
`get_report_task_timeout_options` can produce a Celery timeout longer than the
schedule's configured cap: the helper floors values below the
reserve-plus-working minimum instead of preserving the requested cap. For
example, a report configured with a 10-second `working_timeout` receives a
210-second soft limit, so stalled executions can run well past the
user-configured timeout. The report timeout calculation must never exceed the
schedule's `working_timeout`.
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=5a2cc074ab8dbde2f3dd8abcd28c076e4d7e9e5e9be758925833a7f2fcb8ffd9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=5a2cc074ab8dbde2f3dd8abcd28c076e4d7e9e5e9be758925833a7f2fcb8ffd9&reaction=dislike'>👎</a>
##########
superset/utils/screenshot_utils.py:
##########
@@ -406,16 +515,12 @@ def _raise_if_budget_exhausted(elapsed: float,
remaining_budget: float) -> None:
# Wait for scroll to settle and content to load
page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)
- # Recompute the remaining budget after the scroll-settle sleep --
- # which itself consumes real wall-clock time -- rather than
- # reusing the value from before it, so the readiness-check
- # timeout below is capped against a fresh number instead of a
- # stale one that would let each tile overrun the budget by up
- # to one settle interval.
- tile_wait_start = time.monotonic()
- elapsed = tile_wait_start - start_time
- remaining_budget = wait_budget_seconds - elapsed
- _raise_if_budget_exhausted(elapsed, remaining_budget)
+ # Re-check after the scroll-settle sleep -- which itself consumes
+ # real wall-clock time -- so the readiness-check timeout below is
+ # derived from a fresh remaining value instead of a stale one
+ # that would let each tile overrun the budget by up to one settle
+ # interval (_timeout_seconds also recomputes at call time).
+ _raise_if_budget_exhausted()
Review Comment:
**Suggestion:** The budget check occurs only after the fixed
`page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` sleep, so a report with less
than one second remaining can exceed its monotonic deadline before this check
runs. Cap the scroll-settle wait using the remaining deadline, or check and
fail before sleeping when the remaining time is shorter than the settle
interval. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Near-deadline tiled reports exceed their authoritative execution budget.
- ⚠️ Up to one second of capture time consumes reserved phases.
- ⚠️ Large dashboards repeat this delay once per tile.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=928479e1d5194910a9cf9e7a3797de86&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=928479e1d5194910a9cf9e7a3797de86&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/screenshot_utils.py
**Line:** 523:523
**Comment:**
*Possible Bug: The budget check occurs only after the fixed
`page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` sleep, so a report with less
than one second remaining can exceed its monotonic deadline before this check
runs. Cap the scroll-settle wait using the remaining deadline, or check and
fail before sleeping when the remaining time is shorter than the settle
interval.
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=f2ce74d46643ee277fe523b5db32c7906d1044a06d33126f59c86974e425776e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42624&comment_hash=f2ce74d46643ee277fe523b5db32c7906d1044a06d33126f59c86974e425776e&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]
