codeant-ai-for-open-source[bot] commented on code in PR #42118:
URL: https://github.com/apache/superset/pull/42118#discussion_r3634941295
##########
superset/utils/screenshot_utils.py:
##########
@@ -217,40 +334,80 @@ def take_tiled_screenshot(
dashboard_top = element_info["top"]
logger.info(
- "Dashboard: %sx%spx at (%s, %s)",
+ "Dashboard: %sx%spx at (%s, %s)%s",
dashboard_width,
dashboard_height,
dashboard_left,
dashboard_top,
+ context_suffix,
)
# Calculate number of tiles needed
num_tiles = max(1, (dashboard_height + tile_height - 1) // tile_height)
- logger.info("Taking %s screenshot tiles", num_tiles)
+ logger.info("Taking %s screenshot tiles%s", num_tiles, context_suffix)
- screenshot_tiles = []
+ screenshot_tiles: list[bytes] = []
for i in range(num_tiles):
+ # Check the time budget before starting this tile's readiness wait.
+ # If it's already exhausted, we can no longer verify this (or any
+ # later) tile is actually ready to capture -- fail loudly instead
+ # of silently snapshotting a spinner or blank chart, or running
+ # past the Celery task time limit and getting SIGKILLed.
+ tile_start = time.monotonic()
+ elapsed = tile_start - start_time
+ remaining_budget = wait_budget_seconds - elapsed
+ if remaining_budget <= 0:
+ # A customer-side chart-loading issue (a slow/hung dashboard),
+ # not a Superset system fault, so this is a WARNING rather
+ # than an ERROR -- consistent with #38130/#38441, which
+ # deliberately downgraded screenshot timeout logs the same way.
+ logger.warning(
+ "Tiled screenshot time budget exhausted on tile %s/%s: "
+ "%s/%s tiles captured so far, %.1fs elapsed of a %.1fs "
+ "budget. Aborting instead of capturing remaining tiles "
+ "unchecked.%s",
+ i + 1,
+ num_tiles,
+ len(screenshot_tiles),
+ num_tiles,
+ elapsed,
+ wait_budget_seconds,
+ context_suffix,
+ )
+ raise TiledScreenshotBudgetExceededError(
+ f"Tiled screenshot budget of "
+ f"{wait_budget_seconds:.1f}s exhausted "
+ f"after {len(screenshot_tiles)}/{num_tiles} tiles"
+ )
+
# Calculate scroll position to show this tile's content
scroll_y = dashboard_top + (i * tile_height)
page.evaluate(f"window.scrollTo(0, {scroll_y})")
logger.debug(
- "Scrolled window to %s for tile %s/%s", scroll_y, i + 1,
num_tiles
+ "Scrolled window to %s for tile %s/%s%s",
+ scroll_y,
+ i + 1,
+ num_tiles,
+ context_suffix,
)
# Wait for scroll to settle and content to load
page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)
# Wait for every chart holder visible in the current viewport to
reach
- # a terminal state (rendered chart or error/empty state). Only
check
+ # a terminal state (rendered chart or error/empty state), capped at
+ # whatever remains of the total time budget so a slow dashboard
+ # degrades gracefully instead of exceeding it. Only check
# viewport-visible chart holders to avoid blocking on
virtualization
# placeholders rendered for off-screen charts. A holder that hasn't
# mounted anything yet does not satisfy this check -- unlike
checking
# for the absence of `.loading`, which passes vacuously in that
case.
+ tile_load_wait = min(load_wait, remaining_budget)
Review Comment:
**Suggestion:** The remaining budget is computed before the mandatory
scroll-settle delay, but then reused for `wait_for_function` without
recalculating. When the budget is nearly exhausted, this can overrun the
intended total budget by at least the settle delay (and potentially trigger
Celery time-limit kills again). Recompute `remaining_budget` after
`page.wait_for_timeout(...)` (or include settle wait in the cap) before
deriving the per-tile readiness timeout. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Tiled screenshot budget enforcement can overshoot configured budget.
- ⚠️ Long-running report captures may still approach Celery limits.
- ⚠️ Logging claims strict capping but implementation exceeds budget.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/utils/screenshot_utils.py`, note the tiled capture loop in
`take_tiled_screenshot()` starting around line 351, where `tile_start`,
`elapsed`, and
`remaining_budget` are computed (lines 356-359) and checked against zero
(line 360).
2. Still in the same function, observe that after the budget check, the code
scrolls and
then waits for scroll settle via
`page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` (lines
385-396), and only afterwards calls `page.wait_for_function(...)` using
`tile_load_wait =
min(load_wait, remaining_budget)` (line 405), where `remaining_budget` is
the value
computed before the settle wait.
3. Write a unit test or small harness that calls `take_tiled_screenshot()`
(function
defined starting around line 272 in this file) with:
- `_resolve_wait_budget_seconds()` (lines 80-133) patched to return a
small budget, for
example 2.0 seconds.
- `load_wait` set to a larger value, for example 60.
- A fake or real Playwright `Page` whose `wait_for_timeout`
implementation actually
sleeps for `SCROLL_SETTLE_TIMEOUT_MS` (1000 milliseconds), and whose
`wait_for_function` can also sleep for the requested timeout.
4. In that test, drive enough elapsed time before entering a given tile
iteration (for
example by patching `time.monotonic()` or by performing real sleeps) so
that, at the
budget check on line 356, `remaining_budget` is a small positive value (for
example 0.1
seconds). Then let the code run:
- The loop passes the `remaining_budget > 0` check and logs the warning
context.
- It calls `page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` (line 396),
consuming 1.0
additional seconds while still holding the stale `remaining_budget` value.
- It then computes `tile_load_wait = min(load_wait, remaining_budget)`
(line 405) using
that stale value and passes it to `page.wait_for_function`, so the total
elapsed time
since `start_time` exceeds the intended budget `wait_budget_seconds` by
at least the
scroll-settle duration, demonstrating that the total budget is not
actually capped by
`remaining_budget` as documented.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e0092aafcf2b41a89f990519fc5c554c&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=e0092aafcf2b41a89f990519fc5c554c&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:** 356:405
**Comment:**
*Logic Error: The remaining budget is computed before the mandatory
scroll-settle delay, but then reused for `wait_for_function` without
recalculating. When the budget is nearly exhausted, this can overrun the
intended total budget by at least the settle delay (and potentially trigger
Celery time-limit kills again). Recompute `remaining_budget` after
`page.wait_for_timeout(...)` (or include settle wait in the cap) before
deriving the per-tile readiness 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%2F42118&comment_hash=199ee2e887c1db464fe4efda295a6f824f32707ac82e1ea8f4002bb35fb58255&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=199ee2e887c1db464fe4efda295a6f824f32707ac82e1ea8f4002bb35fb58255&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]
