geido commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r4013595422
##########
superset/dashboards/api.py:
##########
@@ -1965,44 +1983,183 @@ def cache_dashboard_screenshot(self, pk: int,
**kwargs: Any) -> WerkzeugResponse
dashboard_url = get_url_path("Superset.dashboard_permalink",
key=permalink_key)
screenshot_obj = DashboardScreenshot(dashboard_url, dashboard.digest)
- cache_key = screenshot_obj.get_cache_key(window_size, thumb_size,
permalink_key)
- image_url = get_url_path(
- "DashboardRestApi.screenshot", pk=dashboard.id, digest=cache_key
- )
- cache_payload = (
- screenshot_obj.get_from_cache_key(cache_key) or
ScreenshotCachePayload()
+ cache_scope = f"dashboard:{dashboard.id}"
+ request_cache_key = screenshot_obj.get_api_request_cache_key(
+ window_size,
+ thumb_size,
+ permalink_key,
+ cache_scope,
)
- def build_response(status_code: int) -> WerkzeugResponse:
+ def build_response(
+ status_code: int,
+ cache_key: str,
+ cache_payload: ScreenshotCachePayload,
+ ) -> WerkzeugResponse:
return self.response(
status_code,
cache_key=cache_key,
dashboard_url=dashboard_url,
- image_url=image_url,
+ image_url=get_url_path(
+ "DashboardRestApi.screenshot",
+ pk=dashboard.id,
+ digest=cache_key,
+ ),
task_updated_at=cache_payload.get_timestamp(),
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(
- force, expected_scope=f"dashboard:{dashboard.id}"
- ):
- logger.info("Triggering screenshot ASYNC")
- cache_dashboard_screenshot.delay(
- username=get_current_user(),
- guest_token=(
- g.user.guest_token
- if get_current_user() and isinstance(g.user, GuestUser)
- else None
- ),
- dashboard_id=dashboard.id,
- dashboard_url=dashboard_url,
- thumb_size=thumb_size,
- window_size=window_size,
- cache_key=cache_key,
- force=force,
+ def get_current_generation() -> tuple[
+ str | None, ScreenshotCachePayload | None
+ ]:
+ cache_key = screenshot_obj.get_current_api_generation_cache_key(
+ request_cache_key,
+ cache_scope,
)
- return build_response(202)
- return build_response(200)
+ return (
+ cache_key,
+ screenshot_obj.get_from_cache_key(cache_key) if cache_key else
None,
+ )
+
+ try:
+ observed_cache_key, _ = get_current_generation()
+ except ScreenshotCacheError:
+ logger.exception("Screenshot cache read failed: %s",
request_cache_key)
+ return self.response(
+ 503,
+ message=gettext("Screenshot cache is unavailable"),
+ )
+
+ lock_deadline = time.monotonic() + SCREENSHOT_API_LOCK_WAIT_SECONDS
+ while True:
+ try:
+ with DistributedLock(
+ namespace=SCREENSHOT_API_LOCK_NAMESPACE,
+ request_cache_key=request_cache_key,
+ ):
+ try:
+ cache_key, cached_payload = get_current_generation()
+ except ScreenshotCacheError:
+ logger.exception(
+ "Screenshot cache read failed: %s",
+ request_cache_key,
+ )
+ return self.response(
+ 503,
+ message=gettext("Screenshot cache is unavailable"),
+ )
+
+ cache_payload = cached_payload or ScreenshotCachePayload(
+ scope=cache_scope
+ )
+ if cached_payload is not None and (
+ cache_key != observed_cache_key
+ or not cache_payload.should_enqueue_task(
+ force,
+ expected_scope=cache_scope,
+ )
+ ):
+ assert cache_key is not None
+ return build_response(200, cache_key, cache_payload)
Review Comment:
Checked and intentionally unchanged in a55d5eb89c. If the pointer exists but
its payload was evicted/corrupted, there is no truthful status left to observe;
advancing to a new generation is the recovery path, while the request-scoped
producer lock coalesces concurrent recovery callers. A late old worker can only
write its orphan generation and cannot move the pointer back. Keeping the
pointer would instead leave the API permanently stuck on an unavailable
artifact.
##########
superset/utils/screenshot_utils.py:
##########
@@ -488,6 +492,21 @@ def _unready_chart_holders_js_body(*, viewport_only: bool)
-> str:
f"() => {{ {UNREADY_ALL_CHART_HOLDERS_JS_BODY} "
"return holders.length > 0 && unready.length === 0; }"
)
+# API/UI exports capture the selected tab state from a permalink. A selected
+# tab may intentionally contain no charts, so layout hydration is the non-
+# vacuous mount signal while holder readiness applies to every chart actually
+# rendered by that state.
+DASHBOARD_LAYOUT_READY_JS = "() => document.querySelector('.dashboard-grid')
!== null"
+DASHBOARD_CHART_HOLDERS_READY_JS = (
+ "() => { if (document.querySelector('.dashboard-grid') === null) "
+ f"return false; {UNREADY_CHART_HOLDERS_JS_BODY} "
+ "return unready.length === 0; }"
Review Comment:
Checked and intentionally unchanged in a55d5eb89c. and its selected layout
children are emitted by the same DashboardGrid render; asynchronous chart work
happens inside an already-mounted holder. Requiring holders.length > 0 would
reject valid empty dashboards and chart-free selected tabs. The strict
predicate still needs a hydrated grid, applies a 500ms stable dwell, and the
rebuilt Docker matrix proves empty, chart-free, in-grid non-first, nested
non-first, and top-level non-first states.
##########
superset/utils/screenshots.py:
##########
@@ -532,3 +689,96 @@ def get_cache_key(
"permalink_key": permalink_key,
}
return hash_from_dict(args)
+
+ def get_api_request_cache_key(
+ self,
+ window_size: bool | WindowSize | None,
+ thumb_size: bool | WindowSize | None,
+ permalink_key: str,
+ scope: str,
+ ) -> str:
+ """Return the stable pointer key for one API screenshot request
state."""
+
+ return hash_from_dict(
+ {
+ "type": "dashboard_screenshot_api_request",
+ "version": 1,
+ "legacy_cache_key": self.get_cache_key(
+ window_size,
+ thumb_size,
+ permalink_key,
+ ),
+ "scope": scope,
+ }
+ )
+
+ @staticmethod
+ def get_next_api_generation_cache_key(
+ request_cache_key: str,
+ previous_cache_key: str | None,
+ ) -> str:
+ """Return a deterministic successor so racing producers coalesce."""
+
+ return hash_from_dict(
+ {
+ "type": "dashboard_screenshot_api_generation",
+ "request_cache_key": request_cache_key,
+ "previous_cache_key": previous_cache_key,
Review Comment:
Fixed in a55d5eb89c. Each successor now includes a UUID allocated while the
producer lock is held, so an expired request pointer cannot recreate an older
generation key. The new exact-cache regression advances the pointer and image
TTLs independently, expires only the pointer, requests a replacement, and
verifies the old Updated image remains downloadable under a distinct key.
##########
superset/utils/webdriver.py:
##########
@@ -255,35 +269,52 @@ def _get_screenshot(
return element.screenshot(**timeout_kwargs)
@staticmethod
- def _get_validated_screenshot(
+ def _get_validated_screenshot( # noqa: C901
page: Page,
element: Locator,
element_name: str,
log_context: str | None,
report_execution_context: ReportExecutionContext | None,
+ *,
+ validate_rendered_content: bool = False,
+ require_complete_capture: bool = False,
+ load_wait_seconds: float = 60.0,
) -> bytes:
- """Capture a standard screenshot and reject blank report output."""
+ """Capture a standard screenshot and reject incomplete rendered
output."""
context_suffix = f" [{log_context}]" if log_context else ""
for attempt in range(1, TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS + 1):
- if report_execution_context:
- stable_timeout =
report_execution_context.deadline.timeout_seconds(
- "capture_readiness_stability",
- reserve_seconds=(
- report_execution_context.readiness_reserve_seconds
- ),
- )
- stable_predicate = (
- STABLE_CHART_CONTAINER_READY_JS
- if element_name == "chart-container"
- else STABLE_REPORT_ALL_CHART_HOLDERS_READY_JS
+ if report_execution_context or require_complete_capture:
+ stable_timeout = (
+ report_execution_context.deadline.timeout_seconds(
+ "capture_readiness_stability",
+ reserve_seconds=(
+ report_execution_context.readiness_reserve_seconds
+ ),
+ )
+ if report_execution_context
+ else load_wait_seconds
)
+ if element_name == "chart-container":
+ capture_readiness_predicate = CHART_CONTAINER_READY_JS
+ stable_predicate = STABLE_CHART_CONTAINER_READY_JS
+ elif require_complete_capture:
+ capture_readiness_predicate =
DASHBOARD_ALL_CHART_HOLDERS_READY_JS
+ stable_predicate =
STABLE_DASHBOARD_ALL_CHART_HOLDERS_READY_JS
+ else:
+ capture_readiness_predicate =
REPORT_ALL_CHART_HOLDERS_READY_JS
+ stable_predicate = STABLE_REPORT_ALL_CHART_HOLDERS_READY_JS
try:
waited_for_stability = wait_for_stable_readiness(
page,
stable_predicate,
stable_timeout,
)
Review Comment:
Fixed in a55d5eb89c. Strict standard retries now share one monotonic capture
deadline instead of receiving a fresh load_wait_seconds budget each time. The
deterministic regression proves the second stability timeout shrinks from
3000ms to 1000ms and the third capture is never attempted once the shared
budget is exhausted.
##########
superset/utils/screenshot_utils.py:
##########
@@ -1466,11 +1550,11 @@ def _raise_if_budget_exhausted() -> None:
logger.info("Combining screenshot tiles...%s", context_suffix)
combined_screenshot = combine_screenshot_tiles(
screenshot_tiles,
- allow_partial_fallback=report_execution_context is None,
+ allow_partial_fallback=not strict_capture,
log_context=log_context,
)
- if report_execution_context and contentful_tiles_captured:
+ if strict_capture and contentful_tiles_captured:
Review Comment:
Fixed in a55d5eb89c. The rebase incorporated upstream #44191's
per-contentful-region validation after tile combination and generalized it to
strict API/UI capture: if any previously validated region becomes blank,
capture raises ScreenshotBlankCaptureError and the generation ends Error. An
API-specific regression covers the no-report-context path.
--
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]
