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")