fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill - #42118
Conversation
dosubot
Bot
added
the
alert-reports
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # under that ceiling for the rest of the pipeline that runs after tiling | ||
| # completes: combining tiles into one image, building the PDF, and | ||
| # uploading/delivering the notification. | ||
| TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440 # 1740s limit - 300s margin |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to this new module-level constant (for example, annotate it as an integer) to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is a new module-level constant introduced without a type annotation, and the custom rule requires type hints for relevant variables that can be annotated. An int annotation would satisfy the rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 54:54
**Comment:**
*Custom Rule: Add an explicit type annotation to this new module-level constant (for example, annotate it as an integer) to satisfy the type-hint requirement for relevant variables.
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|
|
||
| class TestTileWaitBudget: | ||
| @pytest.fixture | ||
| def mock_page(self): |
There was a problem hiding this comment.
Suggestion: Add explicit parameter and return type annotations to this fixture method signature. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new fixture method omits both parameter and return type annotations, which violates the Python type-hints rule for newly added code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 405:405
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to this fixture method signature.
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| page.screenshot.return_value = b"fake_screenshot_data" | ||
| return page | ||
|
|
||
| def test_per_tile_wait_shrinks_as_budget_depletes(self, mock_page, monkeypatch): |
There was a problem hiding this comment.
Suggestion: Add type annotations for all parameters and an explicit return type on this new test method. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The added test method has unannotated parameters and no return type annotation, so it matches the type-hints violation rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 419:419
**Comment:**
*Custom Rule: Add type annotations for all parameters and an explicit return type on this new test method.
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| assert timeouts == [100 * 1000, 50 * 1000, 10 * 1000] | ||
| assert timeouts == sorted(timeouts, reverse=True) | ||
|
|
||
| def test_budget_exhausted_raises_and_stops_capturing(self, mock_page, monkeypatch): |
There was a problem hiding this comment.
Suggestion: Add type hints to this method signature, including fixture argument types and a return type. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This newly added test method is missing type annotations on its parameters and return type, which is a real violation of the type-hints rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 445:445
**Comment:**
*Custom Rule: Add type hints to this method signature, including fixture argument types and a return type.
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| assert error_args[3] == 1000 | ||
| assert error_args[4] == 1000 | ||
|
|
||
| def test_fast_dashboard_matches_default_behavior(self, mock_page): |
There was a problem hiding this comment.
Suggestion: Add parameter and return type annotations to this test method declaration. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This new test method omits type hints for its argument and return value, so the suggestion correctly identifies a type-hints violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_screenshot_utils.py
**Line:** 481:481
**Comment:**
*Custom Rule: Add parameter and return type annotations to this test method declaration.
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
| # under that ceiling for the rest of the pipeline that runs after tiling | ||
| # completes: combining tiles into one image, building the PDF, and | ||
| # uploading/delivering the notification. | ||
| TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440 # 1740s limit - 300s margin |
There was a problem hiding this comment.
Suggestion: The hardcoded 1440-second budget is not aligned with per-schedule Celery time_limit/soft_time_limit overrides, which can be configured lower via working_timeout. In those deployments this function can still run longer than the actual worker limit and get killed before failing cleanly. Derive the budget from the active report/task timeout when available (or enforce a minimum of configured task limits), and only fall back to a static default when no runtime limit is accessible. [possible bug]
Severity Level: Critical 🚨
❌ Report screenshot tasks can be killed mid-tiling.
⚠️ Budgeted waits ignore per-schedule Celery time limits.Steps of Reproduction ✅
1. Configure a report schedule with a lower `working_timeout` via the `working_timeout`
field exposed in the Reports API (`superset/reports/api.py:148, 206`), e.g. 600 seconds.
2. Celery beat uses that `working_timeout` to set per-schedule `time_limit` and
`soft_time_limit` in `scheduler()` at `superset/tasks/scheduler.py:23-35`, so the worker
enforces a shorter execution window than the global 1740s default.
3. When the schedule fires, `reports.execute` (`superset/tasks/scheduler.py:38-56`) runs
`AsyncExecuteReportScheduleCommand.run()` (`superset/commands/report/execute.py:22-77`),
which calls `_get_screenshots()` (`superset/commands/report/execute.py:520-79`) and
ultimately `BaseScreenshot.get_screenshot()` (`superset/utils/screenshots.py:53-62`) using
`WebDriverPlaywright` and `take_tiled_screenshot()` for large dashboards.
4. `take_tiled_screenshot()` applies the fixed `TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS
= 1440` (`superset/utils/screenshot_utils.py:54, 61-83, 93-121`); on slow, many-tile
dashboards this allows up to ~1440s of tiling before `TiledScreenshotBudgetExceededError`,
but the Celery worker may hit its lower per-schedule `time_limit`/`soft_time_limit` first,
yielding `SoftTimeLimitExceeded` or a hard kill instead of the intended clean budget
exhaustion failure.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 54:54
**Comment:**
*Possible Bug: The hardcoded 1440-second budget is not aligned with per-schedule Celery `time_limit`/`soft_time_limit` overrides, which can be configured lower via `working_timeout`. In those deployments this function can still run longer than the actual worker limit and get killed before failing cleanly. Derive the budget from the active report/task timeout when available (or enforce a minimum of configured task limits), and only fall back to a static default when no runtime limit is accessible.
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| # dashboard degrades gracefully instead of exceeding it. | ||
| # Only check viewport-visible spinners to avoid blocking on | ||
| # virtualization placeholders rendered for off-screen charts. | ||
| tile_load_wait = min(load_wait, remaining_budget) |
There was a problem hiding this comment.
Suggestion: The spinner timeout is derived from a stale remaining-budget value that was computed before the mandatory scroll-settle sleep. This lets each tile wait up to one extra settle interval beyond the declared global budget, so total runtime can still exceed the intended cap. Recompute the remaining budget immediately before wait_for_function (after settle wait) and cap spinner wait with that refreshed value. [logic error]
Severity Level: Major ⚠️
⚠️ Tiled screenshots may exceed configured global wait budget.
⚠️ Celery task margin eroded on heavily tiled dashboards.Steps of Reproduction ✅
1. Schedule a dashboard report so Celery runs `reports.execute` at
`superset/tasks/scheduler.py:38-56`, which invokes
`AsyncExecuteReportScheduleCommand.run()` at `superset/tasks/scheduler.py:131-135`.
2. Inside `AsyncExecuteReportScheduleCommand._get_screenshots()`
(`superset/commands/report/execute.py:520-79`), a `DashboardScreenshot` or
`ChartScreenshot` is constructed and `BaseScreenshot.get_screenshot()`
(`superset/utils/screenshots.py:53-62`) is called.
3. `BaseScreenshot.driver()` returns `WebDriverPlaywright` when enabled
(`superset/utils/screenshots.py:37-41`), whose `get_screenshot()` calls
`take_tiled_screenshot()` (`superset/utils/webdriver.py:40-51, 213-219, 399-49`) for large
dashboards.
4. In `take_tiled_screenshot()` (`superset/utils/screenshot_utils.py:61-121`),
`remaining_budget` is computed before `page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` at
line ~93, then `tile_load_wait = min(load_wait, remaining_budget)` at line ~99 uses that
stale value; this lets each tile spend `SCROLL_SETTLE_TIMEOUT_MS` extra per loop beyond
the nominal `TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS`, so total wall-clock time can
exceed the configured budget before `TiledScreenshotBudgetExceededError` is raised.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 218:218
**Comment:**
*Logic Error: The spinner timeout is derived from a stale remaining-budget value that was computed before the mandatory scroll-settle sleep. This lets each tile wait up to one extra settle interval beyond the declared global budget, so total runtime can still exceed the intended cap. Recompute the remaining budget immediately before `wait_for_function` (after settle wait) and cap spinner wait with that refreshed 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
…'s log_context Renamed the execution_id parameter/suffix mechanism added in the previous commit to log_context / " [context]", matching the mechanism used by the concurrent fix-tile-wait-budget PR (#42118) for the same call chain (BaseReportState._get_screenshots -> BaseScreenshot.get_screenshot -> WebDriverProxy subclasses -> take_tiled_screenshot). Both PRs touch take_tiled_screenshot()'s signature, so using the same parameter name and suffix format keeps the eventual rebase mechanical instead of producing a semantic conflict and two different correlation-id formats in the logs. The call site in execute.py now passes log_context=f"execution_id=..." (rather than a bare execution_id kwarg), matching #42118's convention of a self-describing context string (it uses cache_key=... for thumbnails). Only the correlation-id plumbing changed -- the per-chart unready-state diagnostics (chart id + waiting_on_database/spinner_mounted/nothing_mounted) added in the previous commit are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #75d1aaActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks @eschutho, this looks like a solid fix. Pre-commit's failing since The rest of the failures (changes, frontend-build, playwright, etc.) look like the change-detector step hit a GitHub 503, so that's probably just a flaky rerun rather than anything in the diff. I just opened a PR for this separately so it retries. Codeant's logic-error thread on the stale |
573b8a4 to
d64e368
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42118 +/- ##
==========================================
- Coverage 65.44% 65.43% -0.02%
==========================================
Files 2810 2810
Lines 159362 159391 +29
Branches 36374 36377 +3
==========================================
- Hits 104302 104301 -1
- Misses 53018 53047 +29
- Partials 2042 2043 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # 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) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #1c7538
Actionable Suggestions - 1
-
tests/unit_tests/utils/test_screenshot_utils.py - 1
- Exception uses string literal directly · Line 815-819
Additional Suggestions - 2
-
superset/mcp_service/screenshot/pooled_screenshot.py - 1
-
Unused parameter dead code · Line 52-57The `log_context` parameter is accepted but never used — it is not forwarded to `_get_screenshot_internal()` (line 71-73) nor any inner driver call. Accepting a parameter with no functional use is dead code that creates API surface noise.
-
-
superset/utils/screenshots.py - 1
-
Missing parameter docstring · Line 192-197The `log_context` parameter on `driver()` lacks documentation. Compare to `WebDriverProxy.get_screenshot()` which documents it as: `:param log_context: Optional identifier (e.g. report execution id, or a cache key for thumbnails) included in log lines for tracing.`
-
Review Details
-
Files reviewed - 6 · Commit Range:
8db513a..d64e368- superset/mcp_service/screenshot/pooled_screenshot.py
- superset/utils/screenshot_utils.py
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/test_screenshot_utils.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| @property | ||
| def request(self): | ||
| raise RuntimeError("boom") | ||
|
|
||
| with patch("superset.utils.screenshot_utils.current_task", _BrokenTask()): |
There was a problem hiding this comment.
Exception raised with string literal directly instead of assigning to a variable first.
Code suggestion
Check the AI-generated fix before applying
| @property | |
| def request(self): | |
| raise RuntimeError("boom") | |
| with patch("superset.utils.screenshot_utils.current_task", _BrokenTask()): | |
| def request(self): | |
| error_msg = "boom" | |
| raise RuntimeError(error_msg) | |
| with patch("superset.utils.screenshot_utils.current_task", _BrokenTask()): |
Code Review Run #1c7538
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #0f1bedActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the review, @rusackas — went through the bot threads:
Pushed in bcb5957. CI failures I see now ( |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #608ba4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
bcb5957 to
ebf735c
Compare
| animation_wait_elapsed = 0.0 | ||
| if animation_wait > 0: | ||
| page.wait_for_timeout(animation_wait * 1000) | ||
| elapsed = time.monotonic() - start_time | ||
| remaining_budget = wait_budget_seconds - elapsed | ||
| tile_animation_wait = max(0, min(animation_wait, remaining_budget)) | ||
| if tile_animation_wait > 0: | ||
| animation_wait_start = time.monotonic() | ||
| page.wait_for_timeout(tile_animation_wait * 1000) | ||
| animation_wait_elapsed = time.monotonic() - animation_wait_start |
There was a problem hiding this comment.
Suggestion: After the capped animation wait completes, the code proceeds directly to page.screenshot() without checking the remaining budget. If the animation wait consumes the final available time, the tile is still captured after the hard budget has expired, and if this is the last tile the function returns successfully despite exceeding the task limit. Recheck the budget before capture and raise TiledScreenshotBudgetExceededError when it is exhausted. [incorrect control flow]
Severity Level: Major ⚠️
- ❌ Tiled report capture can exceed its safe task budget.
- ⚠️ Celery cleanup time can be consumed by screenshot capture.
- ⚠️ Last tiles may succeed after budget exhaustion.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 482:490
**Comment:**
*Incorrect Control Flow: After the capped animation wait completes, the code proceeds directly to `page.screenshot()` without checking the remaining budget. If the animation wait consumes the final available time, the tile is still captured after the hard budget has expired, and if this is the last tile the function returns successfully despite exceeding the task limit. Recheck the budget before capture and raise `TiledScreenshotBudgetExceededError` when it is exhausted.
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
ebf735c to
dc3672b
Compare
Code Review Agent Run #165fdbActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
… hitting Celery kill Cumulative per-tile readiness waits in take_tiled_screenshot() have no bound tied to the running Celery task's time limit: each tile's wait_for_function runs at the full configured load_wait, so a slow dashboard with N tiles can wait up to N * load_wait and get SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the report failing cleanly and notifying owners. Reworked from this PR's original revision to sit on top of the merged #42253/#42427 readiness work and reuse its runtime budget helper rather than shipping a second one: - Derive one wall-clock budget for the whole tiled operation from resolve_screenshot_task_budget_seconds() (the #42427 helper the non-tiled path already uses). When it returns None (no Celery task context, e.g. synchronous thumbnail generation), fall back to a fixed total ceiling (TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440s) instead of "keep the configured timeout" -- unlike the non-tiled path's single wait, per-tile waits accumulate, so the operation still needs one bounded total. - Cap each tile's readiness-wait timeout at the remaining budget, recomputed after the mandatory scroll-settle sleep (which itself consumes wall-clock time) so a tile can't overrun by up to one settle interval. Cap or skip the cosmetic per-tile animation wait the same way. - Raise TiledScreenshotBudgetExceededError the moment the budget is exhausted -- before any further tile is captured -- and exempt it from the function's return-None fallback so callers fail the report loudly instead of receiving a partial/unchecked screenshot. No budget floor: a budget already exhausted by setup raises before the first tile, matching the non-tiled path's raise-before-capture semantics. - Make the new error a subclass of ScreenshotTaskBudgetExceededError (moved to screenshot_utils.py, re-exported from webdriver.py) so callers can catch the whole budget-error family with one type. - Enrich the per-tile timeout WARNING with budget context (waited vs requested load_wait, elapsed vs total budget, tiles captured) and add a per-tile DEBUG timing breakdown so slow dashboards can be profiled from logs alone. Co-Authored-By: Claude <noreply@anthropic.com>
dc3672b to
df77884
Compare
|
Went through the two open CodeAnt findings against the current (reworked) revision: 1. "Stale 2. "No budget re-check between the animation wait and
Also in the latest push: refreshed a comment in the budget-exhaustion |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at df77884. Full scan of the tiled budget path, both CRUX behaviors traced to boundaries, and all 10 open bot threads adjudicated on merit. No net-new issues above NIT; CI shows no failures (several jobs still pending). Not approving solely because 10 reviewer threads remain unresolved — all adjudicated below as non-blocking (nits / false-positives / already-addressed at HEAD).
Budget math + None/falsy-zero degrade — correct. wait_budget_seconds = resolve_screenshot_task_budget_seconds(...), then if wait_budget_seconds is None: wait_budget_seconds = float(TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS). Per tile: remaining_budget = wait_budget_seconds - (time.monotonic() - start_time), tile_load_wait = min(load_wait, remaining_budget).
- in-task: derived from
request.timelimit(soft preferred, else hard) minusmin(300, limit*0.2)margin. - no-task / None: degrades to the fixed 1440s ceiling — not 0, not unbounded. The
is Nonecheck (notif not budget) correctly avoids conflating a legitimate 0 budget with unset. - exhausted (
<= 0):_raise_if_budget_exhausteduses strict> 0, so a 0/negative remaining raises immediately on the pre-tile check. - tiny-limit / margin≈budget:
budget = max(0.0, limit - margin)floors at 0, which then raises cleanly on the first check.
Cleanup margin before the hard kill — correct. The budget subtracts the same 300s (or 20%) margin the runtime derivation reserves, so the clean raise fires well before Celery's hard limit, leaving room for combine + PDF + delivery. The margin, not a bare == hard_limit, is what makes the SIGKILL avoidable.
Fail-loud — correct, no swallowing. except TiledScreenshotBudgetExceededError: raise is ordered before the generic except Exception: return None, so exhaustion is never degraded to an anonymous None. At the caller (webdriver.py:554) the error is a RuntimeError subclass — neither PlaywrightTimeout nor PlaywrightError — so it propagates past the except clauses at 658–663 and out through finally: context.close() to fail the capture loudly.
Non-tiled / #42253 non-regression — correct. webdriver.py is a pure refactor: the ScreenshotTaskBudgetExceededError class moved into screenshot_utils.py and is re-imported. The non-tiled readiness raise (webdriver.py:368) and its #42427 degrade guard are untouched; the tiled error subclasses the shared base so it fails the report the same way.
Tests — non-vacuous. Reverting the raise guard fails exactly the three raise-on-exhaustion tests (Rule 26 holds). Coverage includes raise-on-exhaustion, None-degrade to fixed ceiling, budget recomputed after scroll-settle, in-Celery derivation (real 120→96s margin math, not a mock echo), and within-budget success. The _FakeClock + real resolve_screenshot_task_budget_seconds (patching only current_task/monotonic) asserts against a real computed budget. seconds→ms (* 1000) is consistent throughout.
Open threads (10, all unresolved):
- Threads 1–5 (type-annotation nits on the constant + test methods): cosmetic and consistent with the file's existing unannotated constants/tests. Non-blocking.
- Thread 6 (1440 vs per-schedule
time_limit): false positive — the budget is derived fromcurrent_task.request.timelimit, which reflects per-invocation overrides; 1440 is only the no-task fallback. - Threads 7 & 8 (stale budget before scroll-settle): already addressed at HEAD —
remaining_budgetis recomputed afterwait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)(lines 415–418) and the readiness cap uses the refreshed value (line 428), with a dedicated regression test. These reference pre-fix commits. - Thread 9 (f-string in
raise): stylistic, non-issue. - Thread 10 (capture after animation wait without re-check): false positive — the animation wait is capped at
remaining_budget, so it ends at the budget rather than past it, and the reserved 300s margin covers the in-flight tile capture + combine + delivery; the tile's readiness was already verified before the wait.
Recommend resolving the above threads; no code change is required for any of them.
| wait_budget_seconds = resolve_screenshot_task_budget_seconds(log_context) | ||
| if wait_budget_seconds is None: | ||
| wait_budget_seconds = float(TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS) | ||
| start_time = time.monotonic() |
There was a problem hiding this comment.
🟡 Non-blocking (owner's call) — tiled budget clock resets instead of accounting for pre-capture time.
wait_budget_seconds is derived from the Celery task limit, but elapsed is measured from this local start_time, so page.goto (bounded 60s), the 3s SELENIUM_HEADSTART, the 30s element.wait_for, and dimension probing all run before the clock starts — the tiled path effectively gets a fresh full budget. The non-tiled _wait_for_charts_ready avoids this by threading screenshot_started_at from the top of get_screenshot and subtracting already-elapsed time; the tiled call site doesn't pass it.
Not a correctness bug: in the common case the 20%/300s margin (and the soft→hard gap) absorbs the difference, and a soft-limit overrun is still caught cleanly as SoftTimeLimitExceeded rather than the SIGKILL this PR targets. It's just a looser guarantee than the non-tiled path — worth a conscious decision.
If you want both paths on one clock, it's 3 coordinated one-liners (NameError until the other two land):
- add
screenshot_started_at: float | None = Noneto thetake_tiled_screenshotsignature; - the change below;
- pass
screenshot_started_at=screenshot_started_atfrom thetake_tiled_screenshot(...)call inwebdriver.py.
| start_time = time.monotonic() | |
| start_time = ( | |
| screenshot_started_at if screenshot_started_at is not None else time.monotonic() | |
| ) |
There was a problem hiding this comment.
Good catch, and agreed it deserved both-paths-on-one-clock: implemented exactly as suggested (all three coordinated changes) in follow-up #42661, with tests pinning that pre-capture elapsed time now reduces the first tile's capped wait, and that the omitted-anchor default is unchanged.
…eenshot capture logs The screenshot capture code is reached by two call paths that identify their runs differently: scheduled reports carry an execution_id (already threaded end-to-end via log_context since #42253), while thumbnails and direct PDF/screenshot downloads are identified by their cache_key -- which was never passed down. BaseScreenshot.compute_and_cache had the cache_key in hand and get_screenshot already accepted a log_context parameter, but the two were never connected, so every capture-layer log line produced by a thumbnail or direct-download run is anonymous: there is no way to join "trying to generate screenshot" / webdriver navigation / readiness / capture-result log lines to the cached digest they were computing. This threads the existing optional log_context through the remaining capture-layer log lines, and populates it on the thumbnail path: - screenshots.py: compute_and_cache passes log_context=f"cache_key={cache_key}" into get_screenshot and resize_image; the thumbnail lifecycle log lines (generate/fail/resize/ cache-updated) now include the cache_key; driver() accepts log_context for its Playwright-unavailable fallback notice. - webdriver.py: the non-tiled Playwright log lines (navigation, headstart, element/chart-container waits, screenshot result), the entire WebDriverSelenium.get_screenshot path, and find_unexpected_errors (both engines) now append the context suffix. - screenshot_utils.py: the non-budget tiled log lines (dimensions, tile count, scroll, capture, skip, combine) and combine_screenshot_tiles gain the same suffix. Log-line/plumbing only -- no behavior change. Split out of #42118 per its scope reduction to tiled-path budgeting; the readiness log lines added by Co-Authored-By: Claude <noreply@anthropic.com>
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
left a comment otherwise, LGTM
|
Bito Automatic Review Skipped – PR Already Merged |
4 participants
SUMMARY
Cumulative per-tile readiness waits in
take_tiled_screenshot()have no bound tied to the running Celery task's own time limit: each tile'swait_for_functionruns at the full configuredload_wait(SCREENSHOT_LOAD_WAIT, default 60s, commonly raised to 600s), so a slow dashboard with N tiles can wait up to N ×load_waitand get SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the report failing cleanly and notifying owners. #42253/#42427 bounded the non-tiled readiness wait this way; the tiled loop remained uncapped.This PR has been reworked from its original revision to sit on top of the merged #42253/#42427 readiness work and is now scoped to tiled-path budgeting only:
resolve_screenshot_task_budget_seconds) instead of shipping a second derivation — one budget policy, both paths. The tiled operation derives a single wall-clock budget from the running Celery task's own soft/hard limit at the start of the capture.Nonefallback differs from the non-tiled path, deliberately. Outside Celery (e.g. synchronous thumbnail generation) the helper returnsNone, which the non-tiled path treats as "keep the configured timeout" — correct for its single bounded wait. The tiled path instead falls back to a fixed total ceiling (TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440s), because per-tile waits accumulate: with N tiles, "keep the configured timeout" would allow N ×load_waittotal. Sized against the longest report-task hard limit observed in production (1740s) minus the same 300s cleanup margin the runtime derivation reserves.TiledScreenshotBudgetExceededError— before any further tile is captured — and is exempted from the function'sreturn Nonefallback, so callers fail the report loudly instead of receiving a partial/unchecked screenshot. No budget floor: a budget already exhausted by setup raises before the first tile, matching the non-tiled path's raise-before-capture semantics (the original revision's 30s floor is dropped for consistency).ScreenshotTaskBudgetExceededError(base class moved toscreenshot_utils.py, re-exported fromwebdriver.pyfor compatibility) so callers can catch the whole budget-error family with one type.load_wait, elapsed vs. total budget, tiles captured so far), and a per-tile DEBUG line logs the readiness-wait/animation-wait timing breakdown so a slow dashboard can be profiled from logs alone. Log levels follow the fix(screenshots): downgrade screenshot timeout logs from ERROR to WARNING #38130/chore(playwright): Using warning for timeouts #38441 precedent (WARNING for chart-loading slowness, ERROR reserved for system faults).Dropped from the original revision (superseded or split out):
_resolve_wait_budget_seconds, margin/fraction constants) — fix(reports): make #42253 screenshot readiness production-safe #42427 landed the shared helper; the tiled path now reuses it, per the reconciliation note in fix(reports): make #42253 screenshot readiness production-safe #42427's own description.log_contexttracing retrofit acrossscreenshots.py/Selenium/find_unexpected_errors— orthogonal to budgeting; will be proposed as a separate small PR.PooledBaseScreenshot.get_screenshotsignature fix — landed separately as fix(mcp): align pooled screenshot override signature #42384.Relationship to #42624: that PR centralizes a full report-level deadline and credits this PR's budget design; this PR is the minimal, targeted tiled-path protection that composes with the merged readiness work today and rebases cleanly under #42624 whenever it lands.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; backend screenshot orchestration and diagnostics only.
TESTING INSTRUCTIONS
New coverage (
TestTileWaitBudget): per-tile timeout shrinks as the budget depletes; the readiness wait is capped with the budget recomputed after the scroll-settle sleep; exhaustion raisesTiledScreenshotBudgetExceededError(WARNING not ERROR) and stops capturing/combining; exhaustion before the first tile raises without capture (no floor); outside Celery the fixed total fallback caps the first tile's wait; inside Celery the #42427-derived budget caps it; fast dashboards under budget behave exactly as before; the error subclassesScreenshotTaskBudgetExceededError; per-tile timing DEBUG lines carry the log context.ADDITIONAL INFORMATION