codeant-ai-for-open-source[bot] commented on code in PR #42253:
URL: https://github.com/apache/superset/pull/42253#discussion_r3650161890
##########
superset/utils/screenshot_utils.py:
##########
@@ -22,13 +22,71 @@
import time
from typing import TYPE_CHECKING
+from celery import current_task
from PIL import Image
logger = logging.getLogger(__name__)
# Time to wait after scrolling for content to settle and load (in milliseconds)
SCROLL_SETTLE_TIMEOUT_MS = 1000
+# Runtime task-budget policy shared with the approach introduced in #42118.
+# Celery exposes the effective per-task hard/soft limits only on the running
+# task, so a static Superset timeout cannot reliably stay below task-level
+# overrides. Reserve at most 20% (capped at five minutes) for browser cleanup,
+# cache error transition, and the remaining report pipeline.
+SCREENSHOT_TASK_BUDGET_MARGIN_FRACTION = 0.2
+SCREENSHOT_TASK_BUDGET_MAX_MARGIN_SECONDS = 300
+
+
+def resolve_screenshot_task_budget_seconds(
+ log_context: str | None = None,
+) -> float | None:
+ """
+ Return the safe screenshot budget derived from the active Celery task.
+
+ Celery exposes ``request.timelimit`` as ``(hard, soft)``. Prefer the soft
+ limit because cleanup must finish before Celery raises it, falling back to
+ the hard limit when no soft limit is configured. Outside Celery, or when
+ the metadata is absent or malformed, return ``None`` so callers preserve
+ their configured standalone timeout.
+ """
+ context_suffix = f" [{log_context}]" if log_context else ""
+ try:
+ if not current_task:
+ return None
+ timelimit = current_task.request.timelimit
+ if not isinstance(timelimit, (tuple, list)) or len(timelimit) != 2:
+ return None
+ hard_limit, soft_limit = timelimit
+ limit = soft_limit or hard_limit
Review Comment:
**Suggestion:** Celery provides `request.timelimit` in soft-limit,
hard-limit order, but this unpacking treats the first value as the hard limit
and the second as the soft limit. When both limits are configured, the code
therefore derives the budget from the hard limit and may continue past the soft
limit, allowing Celery to interrupt the screenshot before cleanup completes.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Report screenshot tasks can exceed their soft deadline.
- ⚠️ Celery may interrupt browser cleanup or cache error handling.
- ⚠️ Dashboard report generation becomes less reliable near task limits.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a report Celery task with distinct soft and hard limits through
the Celery
task configuration referenced by `superset/config.py:1629`.
2. Trigger dashboard screenshot generation through
`WebDriverPlaywright.get_screenshot()`
at `superset/utils/webdriver.py:409`, which calls
`resolve_screenshot_task_budget_seconds()` from
`superset/utils/webdriver.py:338`.
3. Celery exposes `current_task.request.timelimit` as `(soft_limit,
hard_limit)`, but
`superset/utils/screenshot_utils.py:61` assigns those values to `hard_limit`
and
`soft_limit` in the opposite order.
4. At `superset/utils/screenshot_utils.py:62-69`, the calculation selects
the actual hard
limit and reserves 20 percent, so the readiness wait can continue beyond the
actual soft
deadline and Celery can interrupt the task during later cleanup or error
handling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8992abcad83343088958a8d33cbcf91a&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=8992abcad83343088958a8d33cbcf91a&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:** 61:62
**Comment:**
*Logic Error: Celery provides `request.timelimit` in soft-limit,
hard-limit order, but this unpacking treats the first value as the hard limit
and the second as the soft limit. When both limits are configured, the code
therefore derives the budget from the hard limit and may continue past the soft
limit, allowing Celery to interrupt the screenshot before cleanup completes.
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%2F42253&comment_hash=b04f873266e2db53e6df1490e6d4453189a3a6fa8e1b5282cadd5f43a54a9ee9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42253&comment_hash=b04f873266e2db53e6df1490e6d4453189a3a6fa8e1b5282cadd5f43a54a9ee9&reaction=dislike'>👎</a>
##########
superset/utils/screenshot_utils.py:
##########
@@ -249,12 +362,12 @@ def take_tiled_screenshot(
tile_wait_start = time.monotonic()
try:
page.wait_for_function(
- _TILE_READY_CHECK_JS,
+ CHART_HOLDERS_READY_JS,
timeout=load_wait * 1000,
Review Comment:
**Suggestion:** The tiled path continues to use the full configured
`load_wait` without deriving a remaining budget from the active Celery task. A
task with a shorter soft or hard limit can therefore spend the entire static
timeout waiting for each tile and be terminated before screenshot cleanup and
error handling complete, unlike the non-tiled path. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Tiled dashboard reports can time out before completion.
- ⚠️ Browser resources may not be cleaned up before termination.
- ⚠️ Cache error transitions may not execute reliably.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a dashboard screenshot through
`WebDriverPlaywright.get_screenshot()` at
`superset/utils/webdriver.py:409` for a dashboard that exceeds the tiling
threshold and
therefore calls `take_tiled_screenshot()` at
`superset/utils/screenshot_utils.py:275`.
2. Use a Celery report task with a soft or hard time limit shorter than the
configured
screenshot wait; the active task metadata is available through
`current_task.request.timelimit` and is otherwise handled by
`resolve_screenshot_task_budget_seconds()` at
`superset/utils/screenshot_utils.py:42`.
3. For each tile, `take_tiled_screenshot()` at
`superset/utils/screenshot_utils.py:354-365` invokes
`page.wait_for_function()` with the
full `load_wait` value and does not call the task-budget resolver.
4. If multiple tiles each wait for slow chart readiness, the cumulative
waits can reach
the Celery deadline before `take_tiled_screenshot()` combines tiles or the
report caller
performs cleanup, allowing Celery to terminate the task.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=45e4cb1a87aa409f9a82e4c9b4a23c34&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=45e4cb1a87aa409f9a82e4c9b4a23c34&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:** 364:366
**Comment:**
*Possible Bug: The tiled path continues to use the full configured
`load_wait` without deriving a remaining budget from the active Celery task. A
task with a shorter soft or hard limit can therefore spend the entire static
timeout waiting for each tile and be terminated before screenshot cleanup and
error handling complete, unlike the non-tiled path.
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%2F42253&comment_hash=1f001f9fd83960b9a819af92dacfa7384948f6b07b4d044348f49436c3e72e92&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42253&comment_hash=1f001f9fd83960b9a819af92dacfa7384948f6b07b4d044348f49436c3e72e92&reaction=dislike'>👎</a>
##########
superset/utils/webdriver.py:
##########
@@ -491,27 +610,15 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
url,
)
# Standard screenshot captures the full element including
- # below-the-fold content, so wait for all spinners
globally.
- try:
- logger.debug(
- "Waiting for all spinners to clear at url: %s "
- "(SCREENSHOT_LOAD_WAIT=%ss)",
- url,
- self._screenshot_load_wait,
- )
- page.wait_for_function(
- "() =>
document.querySelectorAll('.loading').length === 0",
- timeout=self._screenshot_load_wait * 1000,
- )
- except PlaywrightTimeout:
- logger.warning(
- "Timed out waiting for charts to load at url %s "
- "(SCREENSHOT_LOAD_WAIT=%ss)",
- url,
- self._screenshot_load_wait,
- )
- raise
- logger.debug("All spinners cleared for url: %s", url)
+ # below-the-fold content, so wait for all viewport-visible
+ # chart holders to reach a terminal state.
+ WebDriverPlaywright._wait_for_charts_ready(
+ page,
+ url,
+ self._screenshot_load_wait,
+ log_context=log_context,
+ screenshot_started_at=screenshot_started_at,
Review Comment:
**Suggestion:** The task-budget calculation only limits the chart-readiness
wait. The subsequent animation delay and screenshot operation can consume the
remaining task budget, so the task may still hit its soft or hard deadline
during capture despite this method reporting a safe effective wait. [possible
bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Screenshot tasks can be interrupted during final capture.
- ⚠️ Animation waits consume time reserved for cleanup.
- ⚠️ Report generation may fail after charts become ready.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger standard screenshot generation through
`WebDriverPlaywright.get_screenshot()`
at `superset/utils/webdriver.py:409`; the method records
`screenshot_started_at` at line
416 and passes it to `_wait_for_charts_ready()` at lines 615-620.
2. `_wait_for_charts_ready()` at `superset/utils/webdriver.py:338-383`
limits only the
chart-readiness polling timeout to the remaining task budget.
3. After that method returns, the same standard branch waits for
`selenium_animation_wait`
at `superset/utils/webdriver.py:622-627` and then captures the image through
`_get_screenshot()` at `superset/utils/webdriver.py:283-287`.
4. If readiness completes close to the computed budget, the animation delay
and screenshot
operation can consume the remaining time, causing Celery to interrupt
capture or cleanup
even though the readiness wait itself respected the budget.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40765a6e9cf2429da4e1a199b6d43bb8&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=40765a6e9cf2429da4e1a199b6d43bb8&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:** 615:620
**Comment:**
*Possible Bug: The task-budget calculation only limits the
chart-readiness wait. The subsequent animation delay and screenshot operation
can consume the remaining task budget, so the task may still hit its soft or
hard deadline during capture despite this method reporting a safe effective
wait.
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%2F42253&comment_hash=3a3bc54391496d40e95294ba59312700f0c2536187b8110d03f9f589764659dc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42253&comment_hash=3a3bc54391496d40e95294ba59312700f0c2536187b8110d03f9f589764659dc&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]
