fix(reports): fail loudly instead of falling back to unguarded screenshot when tiled capture fails - #42273
Conversation
dosubot
Bot
added
the
alert-reports
Code Review Agent Run #483014Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42273 +/- ##
=======================================
Coverage 65.44% 65.44%
=======================================
Files 2810 2810
Lines 159362 159364 +2
Branches 36372 36372
=======================================
+ Hits 104292 104294 +2
Misses 53027 53027
Partials 2043 2043
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:
|
Code Review Agent Run #0a1c12Actionable 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 |
|
Thanks, this certainly beats silently shipping a blank or spinner screenshot. Does a report on a retry-enabled schedule get another attempt after this raises, or does one flaky tiled capture now fail the whole run? |
08573c7 to
ff6b3f3
Compare
|
Rebased onto current master (single clean commit, replacing the previous merge-commit history; the separate ruff-format style commit is folded in). Per review guidance, verified the fail-loud change still composes cleanly with the newly merged readiness flow (#42253/#42427):
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if not img: | ||
| # _get_screenshot() has no wait/readiness logic at | ||
| # all, so falling back to it here would risk | ||
| # silently delivering a screenshot of spinners or | ||
| # a blank dashboard. Fail the report loudly | ||
| # instead of guessing at a "safer" fallback. | ||
| logger.warning( | ||
| ( | ||
| "Tiled screenshot failed, " | ||
| "falling back to standard screenshot" | ||
| ) | ||
| "Tiled screenshot failed for url %s and no " | ||
| "safe fallback exists; failing the report", | ||
| url, | ||
| ) | ||
| img = WebDriverPlaywright._get_screenshot( | ||
| page, element, element_name | ||
| raise PlaywrightTimeout( | ||
| f"Tiled screenshot failed for url {url}" | ||
| ) |
There was a problem hiding this comment.
Suggestion: The tiled helper returns None for non-timeout failures such as element lookup, JavaScript evaluation, screenshot capture, or tile-combination errors, but this branch converts every such failure into PlaywrightTimeout. That misclassifies system failures as readiness timeouts and discards the exception from the caller's control flow. Preserve the original exception (or introduce a dedicated tiled-capture failure exception and handle it explicitly) instead of synthesizing a timeout for all falsy results. [api mismatch]
Severity Level: Major ⚠️
- ❌ System capture errors receive timeout status 408.
- ⚠️ Original exception details are not propagated.
- ⚠️ Report failure classification becomes operationally misleading.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 565:578
**Comment:**
*Api Mismatch: The tiled helper returns `None` for non-timeout failures such as element lookup, JavaScript evaluation, screenshot capture, or tile-combination errors, but this branch converts every such failure into `PlaywrightTimeout`. That misclassifies system failures as readiness timeouts and discards the exception from the caller's control flow. Preserve the original exception (or introduce a dedicated tiled-capture failure exception and handle it explicitly) instead of synthesizing a timeout for all falsy results.
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
|
The flagged issue is valid. The current implementation in Here is a concise implementation to replace the generic timeout: # Define a specific exception for tiled capture failures
class TiledCaptureError(Exception):
pass
# In superset/utils/webdriver.py, replace the raise statement:
if not img:
logger.warning("Tiled screenshot failed for url %s", url)
raise TiledCaptureError(f"Tiled screenshot failed for url {url}")I have checked the PR comments and there are no other pending review comments to address. Would you like me to assist with any other part of this PR? superset/utils/webdriver.py |
…shot when tiled capture fails The tiled screenshot path fell back to WebDriverPlaywright._get_screenshot(), a raw capture with no wait/readiness logic, whenever take_tiled_screenshot() returned a falsy result. That risked silently delivering a screenshot of spinners or a blank dashboard. Replace the fallback with a WARNING log and a raised PlaywrightTimeout, matching the existing failure pattern used elsewhere in this method, so the report fails cleanly instead. Co-Authored-By: Claude <noreply@anthropic.com>
ff6b3f3 to
c66515e
Compare
|
Ran an adversarial self-review on the rebase before requesting human review. Full result: no blockers — verified the raise sits only in the tiled branch (the other two Two review nits applied in the amended commit:
One pre-existing adjacent issue deliberately left out of scope: 713/713 unit tests in |
Good question — traced it through the execution path. Short answer: the current run fails, but the schedule's next cron occurrence runs completely fresh, and nothing about this raise changes existing retry semantics — because there were none for captures to begin with. Details, with the relevant code:
So the trade this PR makes is precisely: before, one flaky tiled capture silently delivered an unguarded (often blank/spinner) screenshot and recorded the run as SUCCESS; after, that run records ERROR with an owner notification, and the next scheduled occurrence retries fresh. If we want an in-run capture retry on top of that, it's a reasonable follow-up, but it's orthogonal to removing the unsafe fallback — and worth noting #42624 explicitly considered and deferred in-run retry ("retrying delivery is not safe without provider idempotency"; a capture-only retry wouldn't have that problem, but belongs in its own PR). |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
The change itself is correct and I could not break it.
What I verified against the head blobs and the consumers:
- The raise is reachable and propagates cleanly.
raise PlaywrightTimeout(...)sits inside thetry:atwebdriver.py:473, whoseexcept PlaywrightTimeout: raiseat :661 re-raises it, andfinally: context.close()at :667 still runs — no leaked browser context. - Both consumers already handle it.
_get_screenshots()(commands/report/execute.py:620) catches it underexcept Exceptionand converts it toReportScheduleScreenshotFailedError, i.e. the same clean report failure as the other timeout paths.compute_and_cache()(utils/screenshots.py:317) catches it and callscache_payload.error(), so the thumbnail lands inERRORand is retried afterTHUMBNAIL_ERROR_CACHE_TTL. The inline comment's "(report error, thumbnail cache ERROR)" is accurate. - It does not reopen the bug #41097 fixed. That PR's other half — the
elif cache_payload.status != StatusValues.ERROR: cache_payload.error()branch incompute_and_cache()— is untouched, and the exception path reacheserror()before it, so the payload still leavesCOMPUTINGand the indefinite re-trigger loop stays closed. - The
b""case is a real path, not a hypothetical.take_tiled_screenshot()skips every tile when the element measuresscrollHeight == 0at tile time, leaving an empty list, andcombine_screenshot_tiles([])returnsb"". Previously that produced an unguarded capture; the second test covers it. len(img)at :582 is safe — theraisenarrowsimgtobytes.- Both tests exercise the real
get_screenshot()and fail if the production change is reverted (no raise, sopytest.raisesfails). Neither reachespage.screenshot/element.screenshot, and the log assertion pins the new message. - Security scan floor (error-message leakage, JWT, SQL/RLS, committed secrets) is clean on both changed files.
On the standing bot thread about PlaywrightTimeout misclassifying non-timeout failures: partly fair on naming, but two of its stated consequences do not hold in this repo. PlaywrightTimeout is not mapped to HTTP 408 or any status anywhere — it appears only in webdriver.py, screenshot_utils.py, and tests — and both consumers catch it under a broad except Exception, so the exception type changes no downstream classification today. The original error is also not discarded: take_tiled_screenshot() already logs it via logger.exception before returning None. What is left is a naming/type-accuracy preference, not a behavior change. Your call whether a dedicated exception is worth it.
Two non-blocking notes — one inline, one below.
Residual partial-capture fallback one frame down in combine_screenshot_tiles()
Not this PR — a follow-up question, since you've explicitly scoped screenshot_utils.py out.
combine_screenshot_tiles() still substitutes a degraded capture on failure: with two or more tiles, a PIL error logs and returns screenshot_tiles[0] (screenshot_utils.py:248-251). For a 6000px dashboard tiled at 2000px that ships the top third as if it were the whole dashboard. The value is truthy, so the new guard here accepts it and the report succeeds with a truncated image — the same user-visible outcome you're closing off one frame up.
It's deliberate and pinned by test_combine_tiles_handles_pil_error, so: is tile-1-as-fallback still the behavior you want now that the caller fails loudly on falsy results, or should it return None and let this guard fire?
| # _get_screenshot() has no wait/readiness logic at | ||
| # all, so falling back to it here would risk | ||
| # silently delivering a screenshot of spinners or | ||
| # a blank dashboard. Fail the capture loudly | ||
| # (report error, thumbnail cache ERROR) instead of | ||
| # guessing at a "safer" fallback. |
There was a problem hiding this comment.
This branch has now flipped twice. #41080 removed the fallback for exactly the reason the new comment gives (blank capture silently reaching report recipients), and #41097 re-added it eleven days later while fixing the b""/COMPUTING retry loop — its description presents the fallback as the intended behavior and doesn't mention #41080.
The comment explains why there is no fallback, but not why the reason it came back no longer applies. Someone arriving from #41097 has nothing telling them not to restore it. Worth pinning the history and the reason the retry loop stays closed: compute_and_cache() catches the exception and calls cache_payload.error(), so the payload leaves COMPUTING exactly as it did when the fallback produced a falsy image.
| # _get_screenshot() has no wait/readiness logic at | |
| # all, so falling back to it here would risk | |
| # silently delivering a screenshot of spinners or | |
| # a blank dashboard. Fail the capture loudly | |
| # (report error, thumbnail cache ERROR) instead of | |
| # guessing at a "safer" fallback. | |
| # _get_screenshot() has no wait/readiness logic at | |
| # all, so falling back to it here would risk | |
| # silently delivering a screenshot of spinners or | |
| # a blank dashboard. Fail the capture loudly | |
| # (report error, thumbnail cache ERROR) instead of | |
| # guessing at a "safer" fallback. | |
| # History: removed in #41080, re-added in #41097 to | |
| # stop a COMPUTING-status retry loop; raising keeps | |
| # that loop closed via compute_and_cache()'s ERROR | |
| # transition. Re-read both before restoring it. |
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
WebDriverPlaywright.get_screenshot()fell back to_get_screenshot()— a rawpage.screenshot()/element.screenshot()call with zero wait/readiness logic — whenever the tiled capture (take_tiled_screenshot()) returned a falsy result (Noneorb""). Unlike the tiled path itself or the non-tiled/standard branch in this same method, this fallback never waited for spinners to clear or charts to render, so it could silently deliver a screenshot of loading spinners or a blank dashboard in a report/thumbnail.This PR removes that fallback. When tiled capture fails for any reason, the method now logs a
WARNING(a capture/rendering issue, not a system fault — matching the level already used for the other capture-failure paths in this method) and raisesPlaywrightTimeout, which propagates to a clean report failure the same way the other timeout paths in this method already do.BEFORE/AFTER
Before:
After:
TESTING INSTRUCTIONS
tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightFallback::test_tiled_screenshot_failure_raises_without_fallback—take_tiled_screenshot()returnsNone→ assertsPlaywrightTimeoutis raised,page.screenshot/element.screenshotare never called, and theWARNINGlog fires with the expected message.tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightAnimationWaitOrder::test_tiled_empty_bytes_raises_without_fallback— same assertions for theb""(falsy-but-not-None) case.pytest tests/unit_tests/utils/webdriver_test.py tests/unit_tests/utils/test_screenshot_utils.py— 59 passed, 1 pre-existing failure unrelated to this change (test_get_screenshot_handles_playwright_timeoutfails onmastertoo, due to aplaywrightpackage version mismatch with the test's no-argPlaywrightTimeout()construction).pre-commit run --files superset/utils/webdriver.py tests/unit_tests/utils/webdriver_test.py— all hooks pass (ruff, ruff-format, mypy, pylint).ADDITIONAL INFORMATION
Not changed (out of scope for this PR):
take_tiled_screenshot()itself inscreenshot_utils.py, and the tiling-routing decision (chart-count/height threshold) inwebdriver.py.WebDriverPlaywright.get_screenshot().WebDriverSelenium).