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 <noreply@anthropic.com>

superset/utils/screenshots.py

@@ -312,6 +312,8 @@ class BaseScreenshot:
         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())

superset/utils/webdriver.py

@@ -301,15 +301,6 @@ class WebDriverPlaywright(WebDriverProxy):
                 selenium_animation_wait = app.config[
                     "SCREENSHOT_SELENIUM_ANIMATION_WAIT"
                 ]
-                logger.debug(
-                    "Wait %i seconds for chart animation", selenium_animation_wait
-                )
-                page.wait_for_timeout(selenium_animation_wait * 1000)
-                logger.debug(
-                    "Taking a PNG screenshot of url %s as user %s",
-                    url,
-                    user.username if user else "None",
-                )
                 if app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]:
                     unexpected_errors = WebDriverPlaywright.find_unexpected_errors(page)
                     if unexpected_errors:
@@ -374,7 +365,7 @@ class WebDriverPlaywright(WebDriverProxy):
                             load_wait=self._screenshot_load_wait,
                             animation_wait=selenium_animation_wait,
                         )
-                        if img is None:
+                        if not img:
                             logger.warning(
                                 (
                                     "Tiled screenshot failed, "
@@ -384,14 +375,28 @@ class WebDriverPlaywright(WebDriverProxy):
                             img = WebDriverPlaywright._get_screenshot(
                                 page, element, element_name
                             )
+                        logger.debug(
+                            "Tiled screenshot result: %d bytes for url: %s",
+                            len(img) if img else 0,
+                            url,
+                        )
                     else:
+                        logger.debug(
+                            "Dashboard below tiling threshold "
+                            "(%s charts, %spx height); using standard screenshot "
+                            "for url: %s",
+                            chart_count,
+                            dashboard_height,
+                            url,
+                        )
                         # Standard screenshot captures the full element including
                         # below-the-fold content, so wait for all spinners globally.
                         try:
                             logger.debug(
-                                "Wait for loading element of charts to be gone"
-                                " at url: %s",
+                                "Waiting for all spinners to clear at url: %s "
+                                "(SCREENSHOT_LOAD_WAIT=%ss)",
                                 url,
+                                self._screenshot_load_wait,
                             )
                             page.wait_for_function(
                                 "() => document.querySelectorAll("
@@ -406,16 +411,40 @@ class WebDriverPlaywright(WebDriverProxy):
                                 self._screenshot_load_wait,
                             )
                             raise
+                        logger.debug("All spinners cleared for url: %s", url)
+                        if selenium_animation_wait > 0:
+                            logger.debug(
+                                "Wait %i seconds for chart animation",
+                                selenium_animation_wait,
+                            )
+                            page.wait_for_timeout(selenium_animation_wait * 1000)
+                        logger.debug(
+                            "Taking screenshot of url %s as user %s",
+                            url,
+                            user.username if user else "None",
+                        )
                         img = WebDriverPlaywright._get_screenshot(
                             page, element, element_name
                         )
+                        logger.debug(
+                            "Screenshot result: %d bytes for url: %s",
+                            len(img) if img else 0,
+                            url,
+                        )
                 else:
+                    logger.debug(
+                        "Tiled screenshots disabled; using standard screenshot "
+                        "for url: %s",
+                        url,
+                    )
                     # Standard screenshot captures the full element including
                     # below-the-fold content, so wait for all spinners globally.
                     try:
                         logger.debug(
-                            "Wait for loading element of charts to be gone at url: %s",
+                            "Waiting for all spinners to clear at url: %s "
+                            "(SCREENSHOT_LOAD_WAIT=%ss)",
                             url,
+                            self._screenshot_load_wait,
                         )
                         page.wait_for_function(
                             "() => document.querySelectorAll('.loading').length === 0",
@@ -429,9 +458,26 @@ class WebDriverPlaywright(WebDriverProxy):
                             self._screenshot_load_wait,
                         )
                         raise
+                    logger.debug("All spinners cleared for url: %s", url)
+                    if selenium_animation_wait > 0:
+                        logger.debug(
+                            "Wait %i seconds for chart animation",
+                            selenium_animation_wait,
+                        )
+                        page.wait_for_timeout(selenium_animation_wait * 1000)
+                    logger.debug(
+                        "Taking screenshot of url %s as user %s",
+                        url,
+                        user.username if user else "None",
+                    )
                     img = WebDriverPlaywright._get_screenshot(
                         page, element, element_name
                     )
+                    logger.debug(
+                        "Screenshot result: %d bytes for url: %s",
+                        len(img) if img else 0,
+                        url,
+                    )
 
             except PlaywrightTimeout:
                 raise

tests/unit_tests/utils/test_screenshot_cache_fix.py

@@ -55,7 +55,7 @@ class MockCache:
         self._cache.clear()
 
 
-@pytest.fixture
+@pytest.fixture()
 def mock_user():
     """Fixture to create a mock user."""
     user = MagicMock()
@@ -63,7 +63,7 @@ def mock_user():
     return user
 
 
-@pytest.fixture
+@pytest.fixture()
 def screenshot_obj():
     """Fixture to create a BaseScreenshot object."""
     url = "http://example.com"
@@ -149,6 +149,25 @@ 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, mock_user
+    ):
+        """Empty bytes from get_screenshot must set ERROR, not leave COMPUTING."""
+        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_no_intermediate_cache_during_computing(
         self, mocker: MockerFixture, screenshot_obj, mock_user
     ):
@@ -163,9 +182,9 @@ class TestCacheOnlyOnSuccess:
             cache_key = screenshot_obj.get_cache_key()
             cached_value = BaseScreenshot.cache.get(cache_key)
             # Cache should be empty during screenshot generation
-            assert cached_value is None, (
-                "Cache should not be saved during COMPUTING state"
-            )
+            assert (
+                cached_value is None
+            ), "Cache should not be saved during COMPUTING state"
             return b"image_data"
 
         mocker.patch(

tests/unit_tests/utils/webdriver_test.py

@@ -29,7 +29,7 @@ from superset.utils.webdriver import (
 )
 
 
-@pytest.fixture
+@pytest.fixture()
 def mock_app():
     """Mock Flask app with webdriver configuration."""
     app = MagicMock()
@@ -898,3 +898,229 @@ class TestWebDriverPlaywrightErrorHandling:
             "dashboard",
             "http://example.com",
         )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+    """Animation wait must run after the spinner wait, not before."""
+
+    _base_config = {
+        "WEBDRIVER_OPTION_ARGS": [],
+        "WEBDRIVER_WINDOW": {"pixel_density": 1},
+        "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+        "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+        "SCREENSHOT_SELENIUM_HEADSTART": 0,
+        "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+        "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+        "SCREENSHOT_LOCATE_WAIT": 10,
+        "SCREENSHOT_LOAD_WAIT": 30,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+    }
+
+    def _make_pw_mocks(self, mock_sync_playwright):
+        mock_playwright_instance = MagicMock()
+        mock_browser = MagicMock()
+        mock_context = MagicMock()
+        mock_page = MagicMock()
+        mock_element = MagicMock()
+
+        mock_sync_playwright.return_value.__enter__.return_value = (
+            mock_playwright_instance
+        )
+        mock_playwright_instance.chromium.launch.return_value = mock_browser
+        mock_browser.new_context.return_value = mock_context
+        mock_context.new_page.return_value = mock_page
+        mock_page.locator.return_value = mock_element
+        mock_element.screenshot.return_value = b"screenshot"
+        return mock_context, mock_page
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver.sync_playwright")
+    @patch("superset.utils.webdriver.app")
+    def test_animation_wait_after_spinner_wait_tiled_disabled(
+        self, mock_app, mock_sync_playwright
+    ):
+        """Non-tiled path: animation wait runs after spinner wait_for_function."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+        call_order: list[str] = []
+
+        def record_wait_for_function(*args, **kwargs):
+            call_order.append("spinner_wait")
+
+        def record_wait_for_timeout(ms):
+            if ms == 2 * 1000:
+                call_order.append("animation_wait")
+
+        mock_page.wait_for_function.side_effect = record_wait_for_function
+        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        assert "spinner_wait" in call_order
+        assert "animation_wait" in call_order
+        spinner_idx = call_order.index("spinner_wait")
+        anim_idx = call_order.index("animation_wait")
+        assert (
+            spinner_idx < anim_idx
+        ), "spinner wait must precede animation wait in non-tiled path"
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver.sync_playwright")
+    @patch("superset.utils.webdriver.app")
+    def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+        self, mock_app, mock_sync_playwright
+    ):
+        """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+        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)
+
+        # Small dashboard: 3 charts, 1000px height — below both thresholds
+        mock_page.evaluate.side_effect = [3, 1000]
+
+        call_order: list[str] = []
+
+        def record_wait_for_function(*args, **kwargs):
+            call_order.append("spinner_wait")
+
+        def record_wait_for_timeout(ms):
+            if ms == 2 * 1000:
+                call_order.append("animation_wait")
+
+        mock_page.wait_for_function.side_effect = record_wait_for_function
+        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        assert "spinner_wait" in call_order
+        assert "animation_wait" in call_order
+        assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+    @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_path_passes_animation_wait_per_tile_no_global_wait(
+        self, mock_app, mock_take_tiled, mock_sync_playwright
+    ):
+        """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
+        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)
+
+        # Large dashboard: 25 charts, 6000px height
+        mock_page.evaluate.side_effect = [25, 6000]
+        mock_take_tiled.return_value = b"tiled_screenshot"
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            result = WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "standalone", mock_user
+            )
+
+        assert result == b"tiled_screenshot"
+        mock_take_tiled.assert_called_once_with(
+            mock_page,
+            "standalone",
+            600,
+            load_wait=30,
+            animation_wait=2,
+        )
+        # The only wait_for_timeout call should be the 0ms headstart; no global
+        # animation wait should be issued (handled per-tile by take_tiled_screenshot)
+        animation_waits = [
+            call[0][0]
+            for call in mock_page.wait_for_timeout.call_args_list
+            if call[0][0] == 2 * 1000
+        ]
+        assert (
+            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")
+    def test_animation_wait_skipped_when_zero(self, mock_app, mock_sync_playwright):
+        """No extra wait_for_timeout call when SCREENSHOT_SELENIUM_ANIMATION_WAIT=0."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {
+            **self._base_config,
+            "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0,
+            "SCREENSHOT_TILED_ENABLED": False,
+        }
+
+        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        # Only headstart (0ms) should be called; no animation wait call
+        timeout_values = [
+            call[0][0] for call in mock_page.wait_for_timeout.call_args_list
+        ]
+        assert timeout_values == [
+            0
+        ], f"Expected only [0] (headstart), got {timeout_values}"