sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4197818948
##########
superset/utils/screenshots.py:
##########
@@ -525,6 +658,7 @@ def compute_and_cache( # pylint:
disable=too-many-arguments # noqa: C901
force=force,
expected_scope=self.cache_scope,
retry_fresh_error=retry_fresh_error,
+ check_updated_staleness=self.supports_updated_staleness,
Review Comment:
Passing the staleness opt-in here also applies to
`cache_dashboard_screenshot` tasks that target a unique API generation key,
which the endpoint deliberately enqueues with `force=False` so duplicate
deliveries never recompute a completed result. If a worker stores the capture
and the broker redelivers the task after `THUMBNAIL_UPDATED_CACHE_TTL`
(late-ack setups), the entry is now stale, so this re-renders and overwrites
the already-completed generation in place. Previously issued `image_url`s for
that generation then change content without a new generation being published.
Should freshness only be evaluated when selecting the generation in the API,
leaving the worker to treat a completed pinned generation as done?
##########
superset/utils/screenshots.py:
##########
@@ -191,32 +221,103 @@ def get_status(self) -> str:
return self.status.value
def get_invalid_image_reason(self) -> str | None:
- """Reason this payload's image should not be served/cached, or None if
- it passes validation (or it isn't claiming a successful screenshot)."""
- if self.status != StatusValues.UPDATED:
+ # Validate whenever an image is present, regardless of status: retained
+ # ERROR/COMPUTING entries still carry their previous image and are now
+ # served by the read paths, so a corrupt/blank retained image must be
+ # rejected (treated as a cache miss) too.
+ if self._image is None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp.
+
+ Timestamps are stored NAIVE (``datetime.now().isoformat()``), so the
age
+ is computed against a naive ``datetime.now()``. Keeping the naive
format
+ means old pods reading entries written by a newer pod (and vice versa)
+ during a rolling deploy never hit a naive-minus-aware ``TypeError``.
+
+ Returns None when the stored timestamp is unusable:
+
+ - A corrupt string ``datetime.fromisoformat`` cannot parse
+ (``ValueError``) or a legacy tz-aware string that raises
``TypeError``
+ when subtracted from naive now(). Callers treat None as 'past any
TTL',
+ so the entry self-heals.
+ - A timestamp implausibly far in the future (age below
+ ``-_FUTURE_TIMESTAMP_TOLERANCE_SECONDS``), also treated as unusable
so
+ it self-heals.
+
+ A small negative age (a future timestamp within tolerance, from clock
+ skew between the worker that wrote the entry and the web host reading
it)
+ is returned as-is rather than coerced to None. Callers apply
+ ``age is None or age >/>= TTL``, so a small negative age reads as NOT
+ stale/expired: the entry is treated as fresh. This is what lets the web
+ tier and the worker converge -- if a future timestamp were treated as
+ stale, the web would re-enqueue at 202 while the worker saw its own
+ timestamp as fresh and skipped, looping forever."""
+ try:
+ age = (
+ 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
+ if age < -_FUTURE_TIMESTAMP_TOLERANCE_SECONDS:
Review Comment:
A timestamp more than 300s ahead now returns `None`, which
`is_error_cache_ttl_expired`/`is_in_progress_stale` read as expired, whereas
before a negative age was always fresh. When the writer's clock is more than
five minutes ahead (web and worker containers with different `TZ`, since these
timestamps are naive local time, or a DST fall-back on one host), a fresh ERROR
written by the worker looks expired, so every card-list `thumbnail` load
re-enqueues a render for each failing chart/dashboard, and fresh COMPUTING
entries stop coalescing. Could the far-future branch avoid affecting the paths
that don't opt in, or is skew beyond 300s intentionally out of scope?
##########
superset/charts/api.py:
##########
@@ -1208,9 +1207,24 @@ def build_response(status_code: int) -> WerkzeugResponse:
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(force,
expected_scope=f"chart:{chart.id}"):
+ if cache_payload.should_trigger_task(
+ force,
+ expected_scope=f"chart:{chart.id}",
+ check_updated_staleness=screenshot_obj.supports_updated_staleness,
+ ):
logger.info("Triggering screenshot ASYNC")
- screenshot_obj.cache.set(cache_key,
ScreenshotCachePayload().to_dict())
+ # Do not pre-write the cache entry here (mirroring the dashboard
+ # on-demand endpoint). The worker's compute_and_cache re-reads this
+ # same cache key and re-runs should_trigger_task(force=..., ...); a
+ # force-less request (`force` is None when omitted) that pre-wrote
a
+ # fresh COMPUTING entry would make that re-check see a non-stale
+ # COMPUTING entry, skip the render, and the screenshot would never
be
+ # computed -- churning COMPUTING every
THUMBNAIL_COMPUTING_CACHE_TTL
+ # forever. Leaving the entry untouched also preserves any retained
+ # last-good image: the read path keeps serving it while the refresh
+ # runs, and the worker flips the entry to COMPUTING without
+ # discarding the image (and back to ERROR, still retaining it, if
the
+ # render fails again).
cache_chart_thumbnail.delay(
Review Comment:
With the pre-write removed, a `force=true` request now leaves the old
`UPDATED` entry in place, and the screenshot reader serves a retained image in
any status. A client following 202 → poll `image_url` therefore gets 200 with
the previous image while the task is queued or computing, and keeps getting it
if the render fails; before this change the forced request cleared the entry,
so `image_url` stayed 404 until the new capture existed. The dashboard endpoint
explicitly drops the old image on force. Should an explicit force still discard
the capture here (keeping it only for a failed retry), so callers can tell the
refresh finished?
--
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]
