Re: [PR] fix(thumbnails): refresh stale dashboard screenshot cache entries [superset]

["rebenitez1802 (via GitHub)" <gi...@apache.org>]

rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4380811087


##########
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:
   Addressed in [970488566f](https://github.com/apache/superset/commit/970488566feb8e1fb3baad440ae9e64ec6c80772). Added four producer cases using a real cache and the returned `image_url`: valid wrong-scope and missing-scope predecessors, a correctly scoped UPDATED predecessor without an image, and corrupt bytes. Each asserts 202, a correctly scoped imageless Pending successor, 404 from its image URL, an unchanged predecessor, and one force-less enqueue.
   
   I also mutation-checked the guards: removing the scope conjunct makes both scope cases fail (200 instead of 404), and removing the missing-image fallback makes the scoped imageless case fail (500 instead of 202).
   
   The broader review also caught a related retention gap: a rejected replacement cache write could discard the previous valid image. The same commit preserves it when the backend accepts the retained-image Error payload, with a minimal imageless fallback when that also fails. Six worker regressions cover retention, fallback rejection, wrong/missing scope, and invalid/absent prior images. Final local validation: 147 unit tests, 48 API tests, all branch pre-commit hooks, and mypy passed.



##########
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:
   Updated in [970488566f](https://github.com/apache/superset/commit/970488566feb8e1fb3baad440ae9e64ec6c80772). The config comment names the dashboard on-demand endpoint and dashboard `compute-thumbnails` path, explicitly excludes chart screenshots and card-list endpoints, and states that this is a refresh threshold rather than image expiry. It also explains retained-image availability during refresh/error backoff and that `0`/`None` disables this check without disabling backend expiry or eviction. The seven-day value is described as a matching default, not a live link to `CACHE_DEFAULT_TIMEOUT`.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org