sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4021929182
##########
superset/utils/screenshots.py:
##########
@@ -111,20 +111,28 @@ def __init__(
scope: str | None = None,
):
self._image = image
- self._timestamp = timestamp or datetime.now().isoformat()
+ self._timestamp = timestamp or datetime.now(timezone.utc).isoformat()
Review Comment:
Writing offset-aware timestamps here breaks mixed-version reads: pre-change
pods subtract naive `datetime.now()` from `datetime.fromisoformat(...)`, so a
new worker’s `COMPUTING` or `ERROR` entry raises `TypeError` on old web pods; a
failed first render or rollback can leave S3-backed thumbnails stuck at 500.
Could the serialized format remain backward-compatible for one release, or be
versioned before requiring aware timestamps?
##########
superset/charts/api.py:
##########
@@ -1276,16 +1279,22 @@ def screenshot(self, pk: int, digest: str) ->
WerkzeugResponse:
# serve its image under a different, merely-accessible `pk`.
if cache_payload.get_scope() != f"chart:{chart.id}":
return self.response_404()
- if cache_payload.status == StatusValues.UPDATED:
- try:
- image = cache_payload.get_image()
- except ScreenshotImageNotAvailableException:
- return self.response_404()
- return Response(
- FileWrapper(image),
- mimetype="image/png",
- direct_passthrough=True,
- )
+ # Serve whenever a valid image is present instead of gating on
+ # status == UPDATED. A failed forced refresh leaves the entry in an
+ # ERROR/COMPUTING backoff while still carrying the retained
last-good
+ # image; requiring UPDATED here would 404 that image for up to a
day.
+ # get_from_cache_key already rejects an invalid UPDATED image, and
a
Review Comment:
`get_from_cache_key()` only validates images whose status is `UPDATED`;
after the status-preservation change, retained `ERROR` or `COMPUTING` images
skip the check, so a pre-validation or corrupted cache entry can be served as
`image/png` instead of becoming a miss. Could we validate every retained image
regardless of status and add a non-`UPDATED` invalid-image case?
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -4259,6 +4259,92 @@ def
test_cache_dashboard_screenshot_dashboard_not_found(self):
response = self._cache_screenshot(non_existent_id)
assert response.status_code == 404
+ @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.get_from_cache_key")
+ def test_cache_dashboard_screenshot_recomputes_stale_updated(
+ self, mock_get_from_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."""
+ from datetime import datetime, timedelta
+
+ self.login(ADMIN_USERNAME)
+
+ dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "dash with tag")
+ .first()
+ )
+ # A valid, correctly-scoped UPDATED entry, but 400s old against a 300s
TTL.
+ stale_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
Review Comment:
This “400s old” fixture is interpreted as UTC by `_age_seconds()`, so on a
UTC-ahead developer machine it becomes hours in the future and the test gets
200 with no queued task instead of 202. Could this use
`datetime.now(timezone.utc)` like the adjacent fixtures?
##########
superset/utils/screenshots.py:
##########
@@ -183,21 +191,66 @@ def get_invalid_image_reason(self) -> str | None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp, or None if the stored
+ timestamp is unusable -- a corrupt string (ValueError) or a legacy
+ tz-aware value that cannot be subtracted from naive now() (TypeError).
+ Callers treat None as 'past any TTL' so the entry self-heals."""
+ try:
+ return (
+ datetime.now() - datetime.fromisoformat(self.get_timestamp())
+ ).total_seconds()
+ except (ValueError, TypeError):
+ logger.warning(
+ "Unusable screenshot cache timestamp %r; "
+ "treating entry as expired/stale",
+ self.get_timestamp(),
+ )
+ return None
+
def is_error_cache_ttl_expired(self) -> bool:
- error_cache_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ # strict '>' (an entry exactly at the TTL is still fresh). An unusable
+ # timestamp (age is None) is treated as expired so the entry
self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() > error_cache_ttl
+ age_seconds is None or age_seconds >
app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ )
def is_computing_stale(self) -> bool:
"""Check if a COMPUTING status is stale (task likely failed or
stuck)."""
- computing_ttl = app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ # '>=' (unlike the strict '>' of the ERROR/UPDATED helpers). An
unusable
+ # timestamp (age is None) is treated as stale so the entry self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() >= computing_ttl
+ age_seconds is None
+ or age_seconds >= app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ )
+
+ def is_updated_stale(self) -> bool:
+ """Whether a successfully-rendered (UPDATED) entry is old enough to be
+ recomputed. Returns False when the TTL is unset/0 (no-op unless an
operator
+ opts in). A timestamp we cannot use -- a corrupt string (ValueError)
or a
+ legacy tz-aware string that parses but cannot be subtracted from naive
+ now() (TypeError) -- is logged and treated as stale so it self-heals
rather
+ than being served forever."""
+ # `.get` (not `[]` like the sibling ERROR/COMPUTING helpers) on
purpose:
+ # a deployment whose config predates this key should silently disable
the
+ # feature, not raise KeyError. Checked first so a disabled feature
never
+ # parses/logs an unusable timestamp.
+ updated_ttl = app.config.get("THUMBNAIL_UPDATED_CACHE_TTL")
+ if not updated_ttl: # None or 0 => disabled
+ return False
+ # strict '>' (an image exactly at the TTL is still fresh), matching
+ # is_error_cache_ttl_expired -- not the '>=' of is_computing_stale. An
+ # unusable timestamp (age is None) is treated as stale so it
self-heals.
+ age_seconds = self._age_seconds()
+ return age_seconds is None or age_seconds > updated_ttl
Review Comment:
Assuming every legacy naive timestamp is UTC breaks upgrades on UTC-ahead
hosts: a recent `COMPUTING` entry becomes several hours in the future, so the
360s lease remains fresh and thumbnails can 404 for the timezone offset. Could
we preserve the old local-time interpretation for naive entries, or bound the
negative-age tolerance?
##########
superset/utils/screenshots.py:
##########
@@ -117,14 +117,22 @@ def __init__(
@classmethod
def from_dict(cls, payload: ScreenshotCachePayloadType) ->
ScreenshotCachePayload:
- return cls(
+ instance = cls(
image=base64.b64decode(payload["image"]) if payload["image"] else
None,
status=StatusValues(payload["status"]),
timestamp=payload["timestamp"],
# `.get` rather than `payload["scope"]`: entries cached before this
# field existed won't have the key.
scope=payload.get("scope"),
)
+ # `__init__` infers UPDATED whenever an image is present -- convenient
for
+ # the `ScreenshotCachePayload(image=bytes)` and legacy
bytes-reconstruction
+ # paths, but wrong when rehydrating a persisted entry: an ERROR or
COMPUTING
+ # entry keeps its previous image, and re-inferring UPDATED here would
mask it
+ # as fresh and bypass the shorter ERROR/COMPUTING recovery TTLs.
Restore the
+ # persisted status explicitly.
+ instance.status = StatusValues(payload["status"])
Review Comment:
Serving the retained image covers the fresh ERROR backoff, but once the TTL
expires this path pre-writes an empty PENDING payload before retrying; if the
render fails again, the last-good image is gone and `image_url` stays 404.
Could we preserve the retained payload across the queued retry and cover the
force-fail → retry-fail sequence?
--
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]
