rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4443263579
##########
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:
You're right, fixed in 73bc6639c72084b9c4381e40f254fef9151b953d. The pointer
is written at publication with the backend timeout, before the worker stamps
the capture. Under the defaults it lapsed before the capture aged past the TTL,
so a force-less request published an imageless generation.
The pointer's lifetime is the backend timeout plus
`THUMBNAIL_UPDATED_CACHE_TTL` plus `THUMBNAIL_COMPUTING_CACHE_TTL`. The added
part is capped, and the lifetime is unchanged when the backend timeout is 0 or
the TTL is disabled. I went past the `TTL + computing TTL` floor on purpose.
For a capture that finished at the end of its lease, `max(timeout, TTL +
computing TTL)` expires at the exact second the capture turns stale, which
leaves no window.
On which deployments get the retained-image refresh: the backend must still
hold the capture once it's stale. That covers backends without TTL eviction,
such as S3, for about one backend timeout after the capture turns stale. It
also covers a thumbnail `CACHE_DEFAULT_TIMEOUT` of 0 or above the TTL. On an
evicting backend such as Redis with the defaults, the 7-day timeout drops the
capture the moment it turns stale, so the refresh renders without an image, as
before. The cache docs, `UPDATING.md` and the config comment say this.
A new API test uses a non-evicting cache with the default 7d timeout and 7d
TTL. It asserts that a force-less POST at t0+8d carries the original bytes.
Reverting the pointer change makes it 404.
##########
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:
Agreed. That rationale dates from the first opt-out commit, and this PR has
since removed both parts of it. Fixed in
d7efd15192d9eebb4a7a990552cc419a874f78ea.
These now give the actual reason:
- the docstrings here, near line 437, and in
`test_supports_updated_staleness_flags`;
- the `supports_updated_staleness` comment.
Charts are excluded by scope. The chart endpoint has no producer lock or
generation pointer, so a refresh would rewrite the entry behind its `image_url`
in place; at default sizes, the card list serves that same entry. The worker
reads the flag per class, so opting in would also make `compute-thumbnails`
without `--force` re-render every stale chart.
--
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]
