geido commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r4013595416
##########
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:
Good catch — fixed in a55d5eb89c. Successor generations now get a UUID while
holding the producer lock, so an expired pointer cannot reuse and overwrite an
older artifact. The split-TTL regression proves the old image stays
downloadable under a distinct key.
##########
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:
Good catch — fixed in a55d5eb89c. Strict API/UI capture now validates every
contentful region after tile combination and fails instead of caching if any
region turned blank. There is an API regression for the path without report
context.
##########
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:
Good catch — fixed in a55d5eb89c. Strict retries now share one monotonic
deadline, so each attempt cannot reset the full wait. The regression verifies
the shrinking timeout and that no third capture starts after the budget is
exhausted.
##########
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:
I checked this against DashboardGrid and the browser matrix. The grid and
selected children mount in the same render; later work happens inside existing
holders. Requiring a holder would break valid empty and chart-free tabs, so the
hydrated grid plus stable dwell is intentional.
##########
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:
I checked this path and kept it as-is. Once the pointed payload is missing
or corrupt, there is no truthful state left to return, so a locked caller
starts a replacement generation. The producer lock coalesces races, and a late
old worker cannot move the pointer backward.
--
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]
