You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/07/16 22:15:10 UTC
(superset) 01/01: fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch fix-tile-wait-budget
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 4fca5099dd1288f58e7e5fa5ebcc70f0263c4cbb
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Thu Jul 16 22:14:49 2026 +0000
fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill
Cumulative per-tile spinner/animation waits in take_tiled_screenshot() could
exceed the Celery task_time_limit on dashboards with many tiles, causing the
worker to be SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the
report failing cleanly. Track elapsed wall time across the operation, cap
each tile's wait at what remains of a fixed budget, and raise before the
budget runs out so the report errors and notifies owners rather than
silently degrading or crashing.
Co-Authored-By: Claude <no...@anthropic.com>
---
superset/utils/screenshot_utils.py | 87 ++++++++++++++++++--
tests/unit_tests/utils/test_screenshot_utils.py | 104 ++++++++++++++++++++++++
2 files changed, 185 insertions(+), 6 deletions(-)
diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py
index ccd2c80aa37..65cd68e2be6 100644
--- a/superset/utils/screenshot_utils.py
+++ b/superset/utils/screenshot_utils.py
@@ -19,6 +19,7 @@ from __future__ import annotations
import io
import logging
+import time
from typing import TYPE_CHECKING
from PIL import Image
@@ -28,6 +29,35 @@ logger = logging.getLogger(__name__)
# Time to wait after scrolling for content to settle and load (in milliseconds)
SCROLL_SETTLE_TIMEOUT_MS = 1000
+# Total wall-clock budget, in seconds, for the entire tiled-screenshot
+# operation (element lookup plus all per-tile spinner/animation waits
+# combined). Each tile's wait is capped at whatever remains of this budget
+# so a slow dashboard degrades gracefully instead of running past the
+# Celery task time limit and getting SIGKILLed mid-capture.
+#
+# This is a conservative fixed constant rather than a value derived from
+# Celery's configured task time limit at runtime, because that limit isn't
+# reliably reachable from here: superset/tasks/scheduler.py sets it
+# per-report-schedule via `apply_async(time_limit=..., soft_time_limit=...)`
+# only when a schedule defines `working_timeout`
+# (ALERT_REPORTS_WORKING_TIME_OUT_KILL); there is no static config value
+# that always reflects the limit actually enforced, and this function can
+# also run outside of a Celery task entirely (e.g. synchronous thumbnail
+# generation).
+#
+# Production has observed a Celery hard task_time_limit of 1740s (29 min)
+# for report execution (2026-07-13 incident: a tiled screenshot was killed
+# mid-capture with SoftTimeLimitExceeded). This budget leaves a 300s margin
+# under that ceiling for the rest of the pipeline that runs after tiling
+# completes: combining tiles into one image, building the PDF, and
+# uploading/delivering the notification.
+TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440 # 1740s limit - 300s margin
+
+
+class TiledScreenshotBudgetExceededError(RuntimeError):
+ """Raised when the tiled-screenshot time budget runs out mid-capture."""
+
+
try:
from playwright.sync_api import TimeoutError as PlaywrightTimeout
except ImportError:
@@ -103,7 +133,14 @@ def take_tiled_screenshot(
Returns:
Combined screenshot bytes or None if failed
+
+ Raises:
+ TiledScreenshotBudgetExceededError: If the total time budget for the
+ tiled-screenshot operation runs out before every tile has been
+ verifiably captured. Callers must treat this as a hard failure
+ rather than fall back to an unchecked/partial screenshot.
"""
+ start_time = time.monotonic()
try:
# Get the target element
element = page.locator(f".{element_name}")
@@ -138,9 +175,32 @@ def take_tiled_screenshot(
num_tiles = max(1, (dashboard_height + tile_height - 1) // tile_height)
logger.info("Taking %s screenshot tiles", num_tiles)
- screenshot_tiles = []
+ screenshot_tiles: list[bytes] = []
for i in range(num_tiles):
+ # Check the time budget before starting this tile's readiness wait.
+ # If it's already exhausted, we can no longer verify this (or any
+ # later) tile is actually ready to capture -- fail loudly instead
+ # of silently snapshotting a spinner or blank chart, or running
+ # past the Celery task time limit and getting SIGKILLed.
+ elapsed = time.monotonic() - start_time
+ remaining_budget = TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS - elapsed
+ if remaining_budget <= 0:
+ logger.error(
+ "Tiled screenshot time budget exhausted: %s/%s tiles captured, "
+ "%.1fs elapsed against a %ss budget. Aborting instead of "
+ "capturing remaining tiles unchecked.",
+ len(screenshot_tiles),
+ num_tiles,
+ elapsed,
+ TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS,
+ )
+ raise TiledScreenshotBudgetExceededError(
+ f"Tiled screenshot budget of "
+ f"{TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS}s exhausted "
+ f"after {len(screenshot_tiles)}/{num_tiles} tiles"
+ )
+
# Calculate scroll position to show this tile's content
scroll_y = dashboard_top + (i * tile_height)
@@ -150,9 +210,12 @@ def take_tiled_screenshot(
)
# Wait for scroll to settle and content to load
page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)
- # Wait for any loading spinners visible in the current viewport to clear.
+ # Wait for any loading spinners visible in the current viewport to clear,
+ # capped at whatever remains of the total time budget so a slow
+ # dashboard degrades gracefully instead of exceeding it.
# Only check viewport-visible spinners to avoid blocking on
# virtualization placeholders rendered for off-screen charts.
+ tile_load_wait = min(load_wait, remaining_budget)
try:
page.wait_for_function(
"""() => {
@@ -165,7 +228,7 @@ def take_tiled_screenshot(
}
return true;
}""",
- timeout=load_wait * 1000,
+ timeout=tile_load_wait * 1000,
)
except PlaywrightTimeout:
logger.warning(
@@ -173,14 +236,21 @@ def take_tiled_screenshot(
"(load_wait=%ss)",
i + 1,
num_tiles,
- load_wait,
+ tile_load_wait,
)
# Wait for chart animations (e.g. ECharts) to finish after spinner clears.
# The global animation wait before tiling only covers the first tile;
- # subsequent tiles need their own wait after data loads.
+ # subsequent tiles need their own wait after data loads. Capped at
+ # whatever remains of the budget; unlike the spinner wait above this
+ # is cosmetic settling, not a readiness check, so we simply skip it
+ # (rather than raise) once the budget runs out.
if animation_wait > 0:
- page.wait_for_timeout(animation_wait * 1000)
+ elapsed = time.monotonic() - start_time
+ remaining_budget = TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS - elapsed
+ tile_animation_wait = max(0, min(animation_wait, remaining_budget))
+ if tile_animation_wait > 0:
+ page.wait_for_timeout(tile_animation_wait * 1000)
# Calculate what portion of the element we want to capture for this tile
tile_start_in_element = i * tile_height
@@ -229,6 +299,11 @@ def take_tiled_screenshot(
return combined_screenshot
+ except TiledScreenshotBudgetExceededError:
+ # Budget exhaustion must fail cleanly, not be swallowed into a
+ # `return None` (which upstream callers treat as "fall back to a
+ # standard, unchecked screenshot").
+ raise
except Exception as e:
logger.exception("Tiled screenshot failed: %s", e)
return None
diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py
index bf264a54b8d..7ae6630c938 100644
--- a/tests/unit_tests/utils/test_screenshot_utils.py
+++ b/tests/unit_tests/utils/test_screenshot_utils.py
@@ -25,6 +25,7 @@ from superset.utils.screenshot_utils import (
combine_screenshot_tiles,
SCROLL_SETTLE_TIMEOUT_MS,
take_tiled_screenshot,
+ TiledScreenshotBudgetExceededError,
)
@@ -397,3 +398,106 @@ class TestTakeTiledScreenshot:
sig = inspect.signature(take_tiled_screenshot)
assert sig.parameters["animation_wait"].default == 0
+
+
+class TestTileWaitBudget:
+ @pytest.fixture
+ def mock_page(self):
+ """Create a mock Playwright page object for a 3-tile (5000px) dashboard."""
+ page = MagicMock()
+ element = MagicMock()
+ page.locator.return_value = element
+ page.evaluate.return_value = {
+ "height": 5000,
+ "top": 100,
+ "left": 50,
+ "width": 800,
+ }
+ page.screenshot.return_value = b"fake_screenshot_data"
+ return page
+
+ def test_per_tile_wait_shrinks_as_budget_depletes(self, mock_page, monkeypatch):
+ """Each tile's spinner-wait timeout is capped at the remaining budget."""
+ monkeypatch.setattr(
+ "superset.utils.screenshot_utils.TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS", # noqa: E501
+ 1000,
+ )
+ # monotonic() is called once for start_time, then once per tile to
+ # compute elapsed/remaining budget before that tile's spinner wait.
+ monotonic_values = iter([0, 0, 950, 990])
+ with patch(
+ "superset.utils.screenshot_utils.time.monotonic",
+ side_effect=lambda: next(monotonic_values),
+ ):
+ with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ result = take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=100
+ )
+
+ assert result is not None
+ timeouts = [
+ call[1]["timeout"] for call in mock_page.wait_for_function.call_args_list
+ ]
+ # remaining budget: 1000, 50, 10 seconds -> capped timeouts shrink
+ assert timeouts == [100 * 1000, 50 * 1000, 10 * 1000]
+ assert timeouts == sorted(timeouts, reverse=True)
+
+ def test_budget_exhausted_raises_and_stops_capturing(self, mock_page, monkeypatch):
+ """Exhausting the budget aborts cleanly instead of capturing unchecked."""
+ monkeypatch.setattr(
+ "superset.utils.screenshot_utils.TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS", # noqa: E501
+ 1000,
+ )
+ # start_time=0, tile 0 check: elapsed=0 (proceeds and captures),
+ # tile 1 check: elapsed=1000 -> remaining=0 -> raise before capturing.
+ monotonic_values = iter([0, 0, 1000])
+ with patch(
+ "superset.utils.screenshot_utils.time.monotonic",
+ side_effect=lambda: next(monotonic_values),
+ ):
+ with patch(
+ "superset.utils.screenshot_utils.combine_screenshot_tiles"
+ ) as mock_combine:
+ with patch("superset.utils.screenshot_utils.logger") as mock_logger:
+ with pytest.raises(TiledScreenshotBudgetExceededError):
+ take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=100
+ )
+
+ # Only the first tile was captured before the budget ran out.
+ assert mock_page.screenshot.call_count == 1
+ # Tiles were never combined -- the function raised before that point.
+ mock_combine.assert_not_called()
+
+ mock_logger.error.assert_called_once()
+ error_args = mock_logger.error.call_args[0]
+ assert "budget exhausted" in error_args[0]
+ # tiles captured, tiles total, elapsed seconds, budget seconds
+ assert error_args[1] == 1
+ assert error_args[2] == 3
+ assert error_args[3] == 1000
+ assert error_args[4] == 1000
+
+ def test_fast_dashboard_matches_default_behavior(self, mock_page):
+ """Well under budget, waits are not capped and behavior is unchanged."""
+ with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ result = take_tiled_screenshot(
+ mock_page,
+ "dashboard",
+ tile_height=2000,
+ load_wait=30,
+ animation_wait=5,
+ )
+
+ assert result is not None
+ assert mock_page.screenshot.call_count == 3
+
+ for call in mock_page.wait_for_function.call_args_list:
+ assert call[1]["timeout"] == 30 * 1000
+
+ animation_calls = [
+ call
+ for call in mock_page.wait_for_timeout.call_args_list
+ if call[0][0] == 5 * 1000
+ ]
+ assert len(animation_calls) == 3