codeant-ai-for-open-source[bot] commented on code in PR #42624:
URL: https://github.com/apache/superset/pull/42624#discussion_r3694356602
##########
superset/utils/webdriver.py:
##########
@@ -1051,6 +1452,12 @@ def get_screenshot( # noqa: C901
unexpected_errors,
)
+ if report_execution_context:
+ phase_timeout(
+ "screenshot_capture",
+ None,
+ report_execution_context.post_capture_reserve_seconds,
+ )
img = element.screenshot_as_png
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> For Selenium screenshot capture, use a phase-timeout gate before starting
the synchronous screenshot call; do not require additional thread/signal
wrappers to bound the WebDriver call, as Celery soft limits and
command-boundary retry handle captures that block past the deadline.
**Applied to:**
- `superset/utils/webdriver.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the report delivery gate for reserving only cleanup capacity;
delivery_reserve_seconds is the budget intended for delivery itself, while the
gate protects later cleanup phases and should not double-count delivery
capacity.
**Applied to:**
- `superset/commands/report/execute.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag flooring sub-viable report working timeouts to the minimum
execution budget; this is intentional to allow capture, delivery, and cleanup
to complete, with a runtime warning documenting the discrepancy.
**Applied to:**
- `superset/utils/report_execution.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Treat synchronous CPU-bound PDF generation as cooperative only at
operation boundaries; do not require internal periodic deadline checks when the
Celery soft-limit handler and command boundary provide terminal-state
persistence.
**Applied to:**
- `superset/commands/report/execute.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
