sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4403230841
##########
superset/dashboards/api.py:
##########
@@ -2556,6 +2573,32 @@ def should_enqueue(cache_payload:
ScreenshotCachePayload) -> bool:
)
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. Any
valid,
+ # scope-matching retained image qualifies -- not only
UPDATED --
+ # so the image also survives a retry that rotates to a
fresh
+ # generation after a prior render failed (whose
predecessor is
+ # ERROR, still carrying the last-good bytes). 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.get_scope() == cache_scope
Review Comment:
With the defaults (`THUMBNAIL_UPDATED_CACHE_TTL` and the thumbnail cache's
default timeout are both seven days), the request-key pointer expires before
the `UPDATED` payload it points to can age past the TTL: the pointer is written
at publish time with the backend's default timeout, while the image timestamp
is written later by the worker. A force-less request therefore never reaches
this carry for a stale capture; once the pointer lapses it takes the no-pointer
branch, publishes an imageless generation, and `image_url` 404s while the
render runs, which is the gap this block is meant to close. Should the pointer
be written with a lifetime of at least the updated TTL plus the computing TTL,
or is the retained-image refresh only intended for deployments whose cache
timeout exceeds the TTL?
##########
tests/unit_tests/utils/test_screenshot_cache_fix.py:
##########
@@ -569,6 +1182,181 @@ def test_stale_computing_triggers_retry(
assert cached_value["status"] == "Updated"
assert cached_value["image"] is not None
+ @pytest.mark.parametrize("explicit_cache_key", [None, "thumbnail-key"])
+ @patch("superset.utils.screenshots.app")
+ def test_stale_updated_unpinned_capture_triggers_recompute(
+ self,
+ mock_app: MagicMock,
+ mocker: MockerFixture,
+ mock_user: MagicMock,
+ explicit_cache_key: str | None,
+ ) -> None:
+ """Direct and CLI thumbnail captures refresh stale dashboard images.
+
+ CLI tasks supply a resolved cache key without requiring complete
capture;
+ that key does not identify an immutable API generation.
+ """
+ # Both keys are required: THUMBNAIL_UPDATED_CACHE_TTL drives the new
+ # staleness clause, and THUMBNAIL_COMPUTING_CACHE_TTL is read as the
+ # DistributedLock ttl_seconds even with the lock mocked.
+ mock_app.config = {
+ "THUMBNAIL_UPDATED_CACHE_TTL": 300,
+ "THUMBNAIL_COMPUTING_CACHE_TTL": 360,
+ }
+ mocker.patch(DISTRIBUTED_LOCK_PATH)
+ BaseScreenshot.cache = MockCache()
+
+ screenshot_obj = DashboardScreenshot("http://example.com";,
"sample_digest")
+ screenshot_obj.cache_scope = "dashboard:1"
+
+ # Seed a stale (400s old) but valid UPDATED entry with a *matching*
+ # scope so the pre-existing scope-mismatch clause doesn't fire first.
+ old_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
+ stale_payload = ScreenshotCachePayload(
+ image=FAKE_PNG_BYTES, timestamp=old_timestamp, scope="dashboard:1"
+ )
+ cache_key = explicit_cache_key or screenshot_obj.get_cache_key()
+ BaseScreenshot.cache.set(cache_key, stale_payload.to_dict())
+ seeded_image = BaseScreenshot.cache.get(cache_key)["image"]
+
+ new_image = b"\x89PNG\r\n\x1a\n" + b"freshly-rendered-body"
+ mocker.patch(BASE_SCREENSHOT_PATH + ".get_screenshot",
return_value=new_image)
+ mocker.patch(BASE_SCREENSHOT_PATH + ".resize_image",
return_value=new_image)
+
+ # Stale UPDATED with a matching scope and staleness enabled triggers a
+ # recompute.
+ assert (
+ stale_payload.should_trigger_task(
+ expected_scope="dashboard:1", check_updated_staleness=True
+ )
+ is True
+ )
+
+ screenshot_obj.compute_and_cache(
+ user=mock_user, force=False, cache_key=explicit_cache_key
+ )
+
+ cached_value = BaseScreenshot.cache.get(cache_key)
+ assert cached_value is not None
+ assert cached_value["status"] == "Updated"
+ assert cached_value["timestamp"] != old_timestamp
+ assert cached_value["image"] is not None
+ assert cached_value["image"] != seeded_image
+
+ @pytest.mark.parametrize("status", [StatusValues.UPDATED,
StatusValues.PENDING])
+ @patch("superset.utils.screenshots.app")
+ def test_api_generation_renders_pending_but_preserves_completed_capture(
+ self,
+ mock_app: MagicMock,
+ mocker: MockerFixture,
+ monkeypatch: pytest.MonkeyPatch,
+ status: StatusValues,
+ ) -> None:
+ """A delayed duplicate cannot replace a completed API generation's
image."""
+ mock_app.config = {
+ "THUMBNAIL_UPDATED_CACHE_TTL": 300,
+ "THUMBNAIL_COMPUTING_CACHE_TTL": 360,
+ }
+ mocker.patch(DISTRIBUTED_LOCK_PATH)
+ cache = MockCache()
+ monkeypatch.setattr(BaseScreenshot, "cache", cache)
+ screenshot_obj = DashboardScreenshot(
+ "http://example.com";, "digest", require_complete_capture=True
+ )
+ screenshot_obj.cache_scope = "dashboard:1"
+ payload = ScreenshotCachePayload(
+ status=status,
+ timestamp=(datetime.now() - timedelta(seconds=400)).isoformat(),
+ scope="dashboard:1",
+ )
+ payload.retain_image(FAKE_PNG_BYTES)
+ seeded = payload.to_dict()
+ cache.set("generation-key", seeded)
+ new_image = b"\x89PNG\r\n\x1a\nnew capture"
+ get_screenshot = mocker.patch(
+ BASE_SCREENSHOT_PATH + ".get_screenshot", return_value=new_image
+ )
+
+ screenshot_obj.compute_and_cache(
+ force=False,
+ cache_key="generation-key",
+ window_size=(800, 600),
+ thumb_size=(800, 600),
+ )
+
+ if status == StatusValues.UPDATED:
+ get_screenshot.assert_not_called()
+ assert cache.get("generation-key") == seeded
+ else:
+ get_screenshot.assert_called_once()
+ result =
ScreenshotCachePayload.from_dict(cache.get("generation-key"))
+ assert result.is_updated()
+ assert result.get_image().read() == new_image
+
+ @patch("superset.utils.screenshots.app")
+ def test_stale_updated_not_recomputed_for_chart(
+ self,
+ mock_app: MagicMock,
+ mocker: MockerFixture,
+ mock_user: MagicMock,
+ ) -> None:
+ """
+ Regression test for the finding: a ChartScreenshot
+ (supports_updated_staleness=False) must NOT recompute a stale-but-valid
+ UPDATED entry on a force-less request. The chart on-demand path
pre-wipes
Review Comment:
This docstring says the chart on-demand path pre-wipes the cache to PENDING
and serves only `UPDATED`, but this PR drops that pre-wipe for force-less
requests and `screenshot` now serves any valid retained image regardless of
status (`charts/api.py`). The same rationale is repeated near line 437. Could
these be reworded to state the actual reason charts opt out, so a later change
to the freshness behavior isn't guided by a constraint that no longer exists?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
