Skip to content

fix(reports): fail loudly instead of falling back to unguarded screenshot when tiled capture fails #42273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eschutho merged 1 commit into from
Jul 31, 2026
+47 −25
Merged

fix(reports): fail loudly instead of falling back to unguarded screenshot when tiled capture fails #42273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions superset/utils/webdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,18 +563,23 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n
log_context=log_context,
)
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 capture loudly
# (report error, thumbnail cache ERROR) instead of
# guessing at a "safer" fallback.
Comment on lines +566 to +571

@aminghadersohi aminghadersohi Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# _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.

logger.warning(
(
"Tiled screenshot failed, "
"falling back to standard screenshot"
)
"Tiled screenshot failed for url %s and no "
"safe fallback exists; failing the capture",
url,
)
img = WebDriverPlaywright._get_screenshot(
page, element, element_name
raise PlaywrightTimeout(
f"Tiled screenshot failed for url {url}"
)
Comment on lines 565 to 579

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

logger.debug(
"Tiled screenshot result: %d bytes for url: %s",
len(img) if img else 0,
len(img),
url,
)
else:
Expand Down
53 changes: 35 additions & 18 deletions tests/unit_tests/utils/webdriver_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,10 +930,13 @@ def evaluate_side_effect(script):
@patch("superset.utils.webdriver._browser_manager")
@patch("superset.utils.webdriver.logger")
@patch("superset.utils.webdriver.take_tiled_screenshot")
def test_tiled_screenshot_failure_falls_back_to_standard_screenshot(
def test_tiled_screenshot_failure_raises_without_fallback(
self, mock_take_tiled, mock_logger, mock_browser_manager
) -> None:
"""When take_tiled_screenshot returns None, fall back to standard screenshot."""
"""When take_tiled_screenshot returns None, fail loudly instead of
falling back to an unguarded standard screenshot."""
from superset.utils.webdriver import PlaywrightTimeout

mock_user = MagicMock()
mock_user.username = "test_user"

Expand All @@ -947,7 +950,8 @@ def test_tiled_screenshot_failure_falls_back_to_standard_screenshot(
mock_context.new_page.return_value = mock_page
mock_page.locator.return_value = mock_element
mock_element.wait_for.return_value = None
# page.screenshot is used by _get_screenshot for the "standalone" element
# page.screenshot is used by _get_screenshot for the "standalone" element;
# it must never be reached by the failure path under test.
mock_page.screenshot.return_value = b"fallback_screenshot"

def evaluate_side_effect(script):
Expand Down Expand Up @@ -983,14 +987,20 @@ def evaluate_side_effect(script):
mock_auth.return_value = mock_context

driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
"http://example.com", "standalone", mock_user
)
# match= keeps this assertion meaningful even when playwright
# is not installed and PlaywrightTimeout aliases bare Exception.
with pytest.raises(
PlaywrightTimeout, match="Tiled screenshot failed for url"
):
driver.get_screenshot("http://example.com", "standalone", mock_user)

assert result == b"fallback_screenshot"
mock_take_tiled.assert_called_once()
mock_page.screenshot.assert_not_called()
mock_element.screenshot.assert_not_called()
mock_logger.warning.assert_any_call(
("Tiled screenshot failed, falling back to standard screenshot"),
"Tiled screenshot failed for url %s and no safe fallback "
"exists; failing the capture",
"http://example.com",
)


Expand Down Expand Up @@ -1514,10 +1524,13 @@ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
@patch("superset.utils.webdriver._browser_manager")
@patch("superset.utils.webdriver.take_tiled_screenshot")
@patch("superset.utils.webdriver.app")
def test_tiled_fallback_triggered_on_empty_bytes(
def test_tiled_empty_bytes_raises_without_fallback(
self, mock_app, mock_take_tiled, mock_browser_manager
):
"""Tiled fallback fires when take_tiled_screenshot returns b"" (not None)."""
"""Tiled failure raises when take_tiled_screenshot returns b"" (not None),
instead of silently falling through to an unguarded raw capture."""
from superset.utils.webdriver import PlaywrightTimeout

mock_user = MagicMock()
mock_user.username = "test_user"
mock_app.config = {
Expand All @@ -1532,20 +1545,24 @@ def test_tiled_fallback_triggered_on_empty_bytes(
mock_page.evaluate.side_effect = [25, 6000]
# Empty bytes — falsy but not None; was silently passed through before the fix
mock_take_tiled.return_value = b""
# _get_screenshot("standalone") calls page.screenshot(full_page=True);
# configure that return value so we can assert the fallback was reached
# _get_screenshot("standalone") calls page.screenshot(full_page=True); it
# must never be reached by the failure path under test.
mock_page.screenshot.return_value = b"fallback"

with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
result = WebDriverPlaywright("chrome").get_screenshot(
"http://example.com", "standalone", mock_user
)
# match= keeps this assertion meaningful even when playwright
# is not installed and PlaywrightTimeout aliases bare Exception.
with pytest.raises(
PlaywrightTimeout, match="Tiled screenshot failed for url"
):
WebDriverPlaywright("chrome").get_screenshot(
"http://example.com", "standalone", mock_user
)

assert result == b"fallback"
# Tiled path was taken (take_tiled_screenshot was called)
mock_take_tiled.assert_called_once()
# Standard screenshot was called as fallback (full_page=True for "standalone")
mock_page.screenshot.assert_called_with(full_page=True)
# Standard screenshot must never be called as a fallback
mock_page.screenshot.assert_not_called()

@patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
@patch("superset.utils.webdriver._browser_manager")
Expand Down
Loading