From f8cf65404ab7be3072ebceff002c377f33cf6c68 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 16 Jun 2026 00:31:23 +0000 Subject: [PATCH] fix(screenshots): catch empty-bytes tiled result and set ERROR on falsy image Two bugs that could cause blank PDFs to re-trigger indefinitely: 1. webdriver.py: tiled fallback used `if img is None:` which let b"" (returned by combine_screenshot_tiles on an empty tile list) pass through silently. Changed to `if not img:` to catch both cases. 2. screenshots.py: compute_and_cache had no else branch after `if image: cache_payload.update(image)`. When get_screenshot returned None or b"" without raising, status stayed at COMPUTING instead of ERROR, causing is_computing_stale() to re-trigger the task indefinitely after THUMBNAIL_ERROR_CACHE_TTL. Added `else: cache_payload.error()` so failed screenshots always transition to ERROR and use the controlled TTL back-off. Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/screenshots.py | 3 ++ superset/utils/webdriver.py | 14 +++++--- .../utils/test_screenshot_cache_fix.py | 20 +++++++++++ tests/unit_tests/utils/webdriver_test.py | 34 +++++++++++++++++++ 4 files changed, 66 insertions(+), 5 deletions(-) diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py index be965a5aee3..b7b12ad4dba 100644 --- a/superset/utils/screenshots.py +++ b/superset/utils/screenshots.py @@ -325,11 +325,14 @@ class BaseScreenshot: cache_payload.error() image = None + # Cache the result (success or error) to avoid immediate retries if image: with event_logger.log_context( f"screenshot.cache.{self.thumbnail_type}" ): cache_payload.update(image) + else: + cache_payload.error() logger.info("Caching thumbnail: %s", cache_key) self.cache.set(cache_key, cache_payload.to_dict()) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index bd612428cbd..505223fd2ae 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -406,11 +406,15 @@ class WebDriverPlaywright(WebDriverProxy): load_wait=self._screenshot_load_wait, animation_wait=selenium_animation_wait, ) - if img is None: - logger.error( - "Tiled screenshot failed at url %s; " - "not falling back to avoid sending a blank PDF", - url, + if not img: + logger.warning( + ( + "Tiled screenshot failed, " + "falling back to standard screenshot" + ) + ) + img = WebDriverPlaywright._get_screenshot( + page, element, element_name ) logger.debug( "Tiled screenshot result: %d bytes for url: %s", diff --git a/tests/unit_tests/utils/test_screenshot_cache_fix.py b/tests/unit_tests/utils/test_screenshot_cache_fix.py index cf62a01d348..20585a4e629 100644 --- a/tests/unit_tests/utils/test_screenshot_cache_fix.py +++ b/tests/unit_tests/utils/test_screenshot_cache_fix.py @@ -155,6 +155,26 @@ class TestCacheOnlyOnSuccess: assert cached_value["status"] == "Updated" assert cached_value["image"] is not None + def test_cache_error_status_when_screenshot_returns_empty_bytes( + self, mocker: MockerFixture, screenshot_obj: BaseScreenshot, mock_user: MagicMock + ) -> None: + """Empty bytes from get_screenshot must set ERROR, not leave COMPUTING.""" + mocker.patch(DISTRIBUTED_LOCK_PATH) + mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None) + mocker.patch( + BASE_SCREENSHOT_PATH + ".get_screenshot", + return_value=b"", + ) + BaseScreenshot.cache = MockCache() + + screenshot_obj.compute_and_cache(user=mock_user, force=True) + + cache_key = screenshot_obj.get_cache_key() + cached_value = BaseScreenshot.cache.get(cache_key) + assert cached_value is not None + assert cached_value["status"] == "Error" + assert cached_value.get("image") is None + def test_computing_status_written_to_cache_early( self, mocker: MockerFixture, diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index 35403f510a7..52ce3f706ac 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -1139,6 +1139,40 @@ class TestWebDriverPlaywrightAnimationWaitOrder: animation_waits == [] ), "No global 2s animation wait_for_timeout should fire on the tiled path" + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) + @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver.take_tiled_screenshot") + @patch("superset.utils.webdriver.app") + def test_tiled_fallback_triggered_on_empty_bytes( + self, mock_app, mock_take_tiled, mock_sync_playwright + ): + """Tiled fallback fires when take_tiled_screenshot returns b"" (not None).""" + mock_user = MagicMock() + mock_user.username = "test_user" + mock_app.config = { + **self._base_config, + "SCREENSHOT_TILED_ENABLED": True, + "SCREENSHOT_TILED_CHART_THRESHOLD": 20, + "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000, + "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600, + } + + mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright) + 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"" + + with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): + with patch.object( + WebDriverPlaywright, "_get_screenshot", return_value=b"fallback" + ) as mock_fallback: + result = WebDriverPlaywright("chrome").get_screenshot( + "http://example.com", "standalone", mock_user + ) + + assert result == b"fallback" + mock_fallback.assert_called_once() + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) @patch("superset.utils.webdriver.sync_playwright") @patch("superset.utils.webdriver.app")