You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by rb...@apache.org on 2026/09/29 13:08:48 UTC
(superset) 01/01: fix(thumbnails): keep dashboard last-good screenshot servable across a stale refresh
This is an automated email from the ASF dual-hosted git repository.
rebenitez1802 pushed a commit to branch rebenitez1802/screenshot-cache-stuck-fix
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 92b4ba0669abc65996c73e45331a549a8b4c9908
Author: Rafael Benitez <re...@gmail.com>
AuthorDate: Tue Sep 29 10:08:35 2026 -0300
fix(thumbnails): keep dashboard last-good screenshot servable across a stale refresh
Addresses review feedback on the dashboard on-demand screenshot endpoint.
- A force-less THUMBNAIL_UPDATED_CACHE_TTL staleness refresh used to publish a
fresh imageless PENDING successor generation and repoint the current pointer to
it before the render ran. If that render failed, the current generation landed
in ERROR with no image, so a caller following image_url saw 404 for the error
backoff instead of the previous, still-valid capture. The endpoint now carries
the prior generation's last-good image onto the successor (new
ScreenshotCachePayload.retain_image), so the read path keeps serving it while
the refresh renders and a failed render retains it (ERROR keeps the image). The
successor stays PENDING, so concurrent producers still coalesce and the worker
still recomputes it. Skipped for an explicit force and for a scope mismatch.
- test_cache_dashboard_screenshot_recomputes_stale_updated did not exercise the
staleness branch: without mocking get_current_api_generation_cache_key the
request had no prior generation, so the endpoint took the ordinary cache-miss
branch (also 202) and the mocked stale payload was never consulted -- the test
stayed green even with the check_updated_staleness wiring reverted. It now seeds
a resolvable generation pointer so it truly pins the behavior, plus a new
integration test for the retained-image path and a retain_image unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <no...@anthropic.com>
---
superset/dashboards/api.py | 23 ++++++
superset/utils/screenshots.py | 14 ++++
tests/integration_tests/dashboards/api_tests.py | 96 +++++++++++++++++++++-
.../unit_tests/utils/test_screenshot_cache_fix.py | 24 ++++++
4 files changed, 154 insertions(+), 3 deletions(-)
diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py
index a379db13244..a5ed1d80125 100644
--- a/superset/dashboards/api.py
+++ b/superset/dashboards/api.py
@@ -2123,6 +2123,29 @@ class DashboardRestApi(
)
cache_payload = ScreenshotCachePayload(scope=cache_scope)
cache_payload.pending()
+ # A force-less staleness refresh of a still-valid capture must
+ # not publish an imageless successor: carry the prior
+ # generation's last-good image onto this in-progress payload so
+ # the read path keeps serving it while the refresh renders, and
+ # a failed render retains it (ERROR keeps the image) instead of
+ # leaving image_url at 404 for the error backoff. The successor
+ # stays PENDING, so concurrent producers still coalesce and the
+ # worker still recomputes it. Skipped for an explicit force (the
+ # caller asked to discard the capture) and for a scope mismatch
+ # (must never serve another object's image).
+ if (
+ not force
+ and cached_payload is not None
+ and cached_payload.is_updated()
+ and cached_payload.get_scope() == cache_scope
+ and cached_payload.get_invalid_image_reason() is None
+ ):
+ try:
+ cache_payload.retain_image(
+ cached_payload.get_image().read()
+ )
+ except ScreenshotImageNotAvailableException:
+ pass
try:
screenshot_obj.store_cache_payload(
next_cache_key,
diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py
index 8197ec08c4b..e0774ec3935 100644
--- a/superset/utils/screenshots.py
+++ b/superset/utils/screenshots.py
@@ -189,6 +189,20 @@ class ScreenshotCachePayload:
self.status = StatusValues.UPDATED
self._image = image
+ def retain_image(self, image: bytes | None) -> None:
+ """Attach an already-rendered image without changing status or timestamp.
+
+ Unlike ``update`` (which marks the entry ``UPDATED``), this leaves the
+ status in progress. It is used when a staleness refresh publishes a
+ successor generation: carrying the prior generation's last-good image
+ keeps the read path serving it while the refresh renders, and a failed
+ render that transitions to ``ERROR`` retains it (``error()`` without
+ ``discard_image``) instead of leaving ``image_url`` at 404. Keeping the
+ successor ``PENDING`` means concurrent producers still coalesce and the
+ worker's ``should_trigger_task`` still recomputes it.
+ """
+ self._image = image
+
def error(self, *, discard_image: bool = False) -> None:
self.update_timestamp()
if discard_image:
diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py
index ee0bd8fd025..838cc96db2c 100644
--- a/tests/integration_tests/dashboards/api_tests.py
+++ b/tests/integration_tests/dashboards/api_tests.py
@@ -5474,16 +5474,34 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
@with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.cache_dashboard_screenshot")
+ @patch(
+ "superset.dashboards.api.DashboardScreenshot.set_current_api_generation_cache_key",
+ return_value=True,
+ )
+ @patch(
+ "superset.dashboards.api.DashboardScreenshot.get_current_api_generation_cache_key",
+ return_value="cache-key",
+ )
@patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
def test_cache_dashboard_screenshot_recomputes_stale_updated(
- self, mock_get_from_cache_key, mock_cache_task
+ self,
+ mock_get_from_cache_key,
+ mock_current_cache_key,
+ mock_set_current_cache_key,
+ mock_cache_task,
):
"""A force-less request whose cached UPDATED entry is older than
THUMBNAIL_UPDATED_CACHE_TTL -- but still valid and correctly scoped --
must reschedule the Celery task. This exercises the endpoint's
``check_updated_staleness=screenshot_obj.supports_updated_staleness``
wiring (True only for dashboards); dropping that argument makes the
- endpoint serve the stale entry (200) instead, failing this test."""
+ endpoint serve the stale entry (200) instead, failing this test.
+
+ ``get_current_api_generation_cache_key`` must be mocked to a resolvable
+ pointer: without it the request has no prior generation, so the endpoint
+ takes the ordinary first-render/cache-miss branch (also a 202) and the
+ stale-payload assertion below would pass even if the staleness wiring were
+ reverted."""
from datetime import datetime, timedelta
self.login(ADMIN_USERNAME)
@@ -5496,7 +5514,7 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
# A valid, correctly-scoped UPDATED entry, but 400s old against a 300s TTL.
stale_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
mock_get_from_cache_key.return_value = ScreenshotCachePayload(
- b"fake image data",
+ b"\x89PNG\r\n\x1a\nfake image data",
scope=f"dashboard:{dashboard.id}",
timestamp=stale_timestamp,
)
@@ -5506,6 +5524,78 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
assert cache_resp.status_code == 202
mock_cache_task.delay.assert_called()
+ @with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
+ @with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
+ @pytest.mark.usefixtures("create_dashboard_with_tag")
+ @patch("superset.dashboards.api.cache_dashboard_screenshot")
+ @patch("superset.dashboards.api.DashboardScreenshot.store_cache_payload")
+ @patch(
+ "superset.dashboards.api.DashboardScreenshot.set_current_api_generation_cache_key",
+ return_value=True,
+ )
+ @patch(
+ "superset.dashboards.api.DashboardScreenshot.get_current_api_generation_cache_key",
+ return_value="cache-key",
+ )
+ @patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
+ def test_cache_dashboard_screenshot_stale_refresh_retains_last_good_image(
+ self,
+ mock_get_from_cache_key,
+ mock_current_cache_key,
+ mock_set_current_cache_key,
+ mock_store_cache_payload,
+ mock_cache_task,
+ ):
+ """A force-less staleness refresh must not publish an imageless successor
+ generation. The new generation carries the prior valid capture, so the
+ read path keeps serving it while the refresh renders and a failed render
+ retains it (ERROR keeps the image) instead of 404ing ``image_url``. The
+ successor stays in progress (so concurrent producers coalesce) and the
+ worker's force-less ``should_trigger_task`` still recomputes it.
+
+ Reverting the carry-over leaves the successor imageless, so
+ ``get_image()`` below raises and the test fails."""
+ from datetime import datetime, timedelta
+
+ self.login(ADMIN_USERNAME)
+
+ dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "dash with tag")
+ .first()
+ )
+ retained_image = b"\x89PNG\r\n\x1a\nfake image data"
+ # A valid, correctly-scoped UPDATED entry, but 400s old against a 300s TTL.
+ stale_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ retained_image,
+ scope=f"dashboard:{dashboard.id}",
+ timestamp=stale_timestamp,
+ )
+
+ cache_resp = self._cache_screenshot(dashboard.id)
+
+ assert cache_resp.status_code == 202
+ mock_cache_task.delay.assert_called_once()
+
+ # The successor generation persisted for the worker carries the last-good
+ # image, stays in progress (so producers coalesce) and stays triggerable
+ # (so the worker still re-renders).
+ assert mock_store_cache_payload.called
+ successor = mock_store_cache_payload.call_args[0][1]
+ assert successor.get_invalid_image_reason() is None
+ # Load-bearing assertion: reverting the carry-over leaves the successor
+ # imageless, so get_image() raises here. The in-progress/triggerable
+ # assertions below also hold for an imageless PENDING successor, so they
+ # guard against retain_image wrongly flipping status -- not the carry.
+ assert successor.get_image().read() == retained_image
+ assert successor.is_in_progress()
+ assert successor.should_trigger_task(
+ force=False,
+ expected_scope=f"dashboard:{dashboard.id}",
+ check_updated_staleness=True,
+ ), "successor must remain triggerable so the worker re-renders"
+
@with_feature_flags(THUMBNAILS=True)
@with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
@pytest.mark.usefixtures("create_dashboard_with_tag")
diff --git a/tests/unit_tests/utils/test_screenshot_cache_fix.py b/tests/unit_tests/utils/test_screenshot_cache_fix.py
index a7e6fa97ae5..620a418a655 100644
--- a/tests/unit_tests/utils/test_screenshot_cache_fix.py
+++ b/tests/unit_tests/utils/test_screenshot_cache_fix.py
@@ -316,6 +316,30 @@ class TestShouldTriggerTask:
assert payload.should_trigger_task(force=False) is True
+ def test_retain_image_keeps_status_and_timestamp(self):
+ """retain_image attaches an image without changing status or timestamp.
+
+ This is what lets a staleness-refresh successor carry the prior last-good
+ image while staying PENDING: the unchanged fresh timestamp keeps
+ concurrent producers coalescing, and the still-PENDING status keeps the
+ worker's should_trigger_task recomputing it -- neither of which holds if
+ retain_image refreshed the timestamp or flipped the status to UPDATED.
+ """
+ timestamp = "2020-01-01T00:00:00"
+ payload = ScreenshotCachePayload(
+ status=StatusValues.PENDING, timestamp=timestamp
+ )
+
+ payload.retain_image(FAKE_PNG_BYTES)
+
+ # Image is now served, but the entry stays a fresh, triggerable PENDING.
+ assert payload.get_image().read() == FAKE_PNG_BYTES
+ assert payload.get_invalid_image_reason() is None
+ assert payload.status == StatusValues.PENDING
+ assert payload.is_in_progress() is True
+ assert payload.get_timestamp() == timestamp
+ assert payload.should_trigger_task(force=False) is True
+
@patch("superset.utils.screenshots.app")
def test_trigger_on_expired_error(self, mock_app):
"""Test that expired ERROR status triggers task."""