sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4380138683
##########
superset/dashboards/api.py:
##########
@@ -2556,6 +2560,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:
The carry-over guard here (`get_scope() == cache_scope`, plus the
`ScreenshotImageNotAvailableException` fallback below) is what keeps another
object's bytes off this dashboard's authorized `image_url`, but no producer
test reaches either branch. If a refactor drops the scope conjunct, a
force-less refresh of a predecessor whose valid image is scoped to a different
dashboard or has no scope would publish a PENDING successor carrying that
image, and every current test (matching scope, force,
wrong-scope-on-lock-timeout) stays green. The same holds for a correctly scoped
`UPDATED` payload with no image: `get_image()` raises, and removing the
`except` turns the request into a 500. Could we seed a mismatched-scope
predecessor and assert the successor is imageless with a 404 `image_url`, and
seed an imageless scoped predecessor and assert a 202 with an imageless PENDING
successor?
##########
superset/config.py:
##########
@@ -1414,6 +1414,13 @@ def sync_theme_logo_href(
# How long to treat a COMPUTING cache entry as an active lease before
considering
# the worker stuck. Should exceed the task soft_time_limit (300 s) by a
margin.
THUMBNAIL_COMPUTING_CACHE_TTL = int(timedelta(seconds=360).total_seconds())
+# How long a successfully-rendered (UPDATED) thumbnail stays servable before it
+# is recomputed. Cache backends without TTL eviction (e.g. S3) never expire
+# entries on their own, so without this a once-rendered thumbnail is served
+# forever even if it was a valid-but-blank capture. Defaults to match the
+# THUMBNAIL_CACHE_CONFIG CACHE_DEFAULT_TIMEOUT (7 days) so freshness matches
what
+# TTL-evicting backends already enforce. Set to 0 or None to disable (opt out).
+THUMBNAIL_UPDATED_CACHE_TTL = int(timedelta(days=7).total_seconds())
Review Comment:
This comment says an UPDATED thumbnail stays servable only until this TTL,
but the setting only triggers a refresh on the dashboard on-demand endpoint and
`compute-thumbnails`; chart and card-list endpoints ignore it, and a retained
image stays servable past it through render and error backoff. An operator
reading config.py could set it expecting a global freshness bound on all
thumbnails. Could the comment name those affected paths the way `cache.mdx`
does?
--
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]
