rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4391618317
##########
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
Review Comment:
I checked this against the earlier [force-creation
guard](https://github.com/apache/superset/pull/43523#discussion_r4277897326)
and [retained-refresh
behavior](https://github.com/apache/superset/pull/43523#discussion_r4129121321)
before changing code. This was a missed interaction: the creation guard was
intact, but a forced caller could bypass it by joining another request's
retained generation. I reproduced both Pending and Computing cases on the
previous head: the forced image URL returned 200 with the old capture.
Fixed in
[583929960f](https://github.com/apache/superset/commit/583929960f97acd3af0dfa8bd3d53817bd7c6502).
Forced callers reject retained Pending/Computing/Error generations at initial
lookup and all three race-dependent joins: locked reread, lost pointer
publication, and lock contention. They select an imageless successor, with
bounded retries if publication races. Ordinary refresh URLs keep serving their
retained image; forced callers can still share an imageless generation or a
completed replacement.
The new API cases verify the forced URL stays 404 until replacement bytes
exist, preserve the old URL, and cover each race and timeout. Targeted
mutations of nine guard behaviors fail their corresponding regressions,
including ordinary joins, completed-winner reuse, and imageless force-force
coalescing. Local validation: 147 unit tests, 59 API tests, all branch
pre-commit hooks, and mypy passed.
All 13 required CI checks are green on final commit
[`4e578bed19`](https://github.com/apache/superset/commit/4e578bed1919428c4c9f4e218cd3395e85de5d49);
no checks remain pending or failed.
##########
superset/dashboards/api.py:
##########
@@ -2478,11 +2478,27 @@ def get_generation(
),
)
+ def force_needs_new_generation(cache_payload: ScreenshotCachePayload)
-> bool:
+ """A forced download cannot join a generation carrying an older
capture."""
+ if not force or cache_payload.is_updated():
Review Comment:
Added in
[4e578bed19](https://github.com/apache/superset/commit/4e578bed1919428c4c9f4e218cd3395e85de5d49).
I verified this was missing regression coverage for the new helper; its
production `not force` guard was already correct.
The existing real-cache sequence now performs a force-less follow-up on the
same permalink for Pending, Computing, and fresh Error generations. Each case
asserts 200, the same cache key and status, readable retained bytes, and
exactly one enqueue. The subsequent forced replacement and force-force
coalescing assertions remain in the same test.
Removing only `not force` makes all three cases fail (202 instead of 200);
restoring it makes all three pass. Production code is unchanged by this
follow-up. Final local validation: 147 unit tests, 59 API tests, all branch
pre-commit hooks, and mypy passed.
All 13 required CI checks are green on final commit
[`4e578bed19`](https://github.com/apache/superset/commit/4e578bed1919428c4c9f4e218cd3395e85de5d49);
no checks remain pending or failed.
--
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]
