fix(dashboard): make screenshot completion truthful

[geido]

SUMMARY

Make API/UI dashboard screenshot completion truthful: Updated is persisted only with a successfully captured artifact after the selected dashboard state reaches a stable terminal render state.

• Publish a fresh Pending generation before enqueueing, preserve prior artifact URLs, and coalesce concurrent forced requests.
• Opt only API/UI dashboard exports into the existing report-grade readiness, stability, virtualization, and blank-image checks; scheduled reports and ordinary thumbnails retain their existing behavior.
• Persist Error for incomplete captures and setup/cache failures without overwriting output completed by another worker.
• Preserve the existing request fields, Celery task signature, status values, and 202 producer / 200 observer convention.
• Let the dashboard download hook consume the additive server task timeout so it does not abandon a valid long-running export.
• Change no visualization-plugin code and add no plugin readiness interface.

The signal covers Superset's chart-holder lifecycle plus the existing ECharts/AG Grid paint signals and blank-image validation. It does not claim a new completion contract for arbitrary third-party asynchronous canvas/WebGL plugins.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

On exact master, a below-fold chart backed by a 45-second query and a 30-second screenshot wait was cached as Updated; GET returned an 11,337-byte PNG containing a loading spinner. With this change, the same fixture transitions Pending → Computing → Error and GET returns 404 in both standard and tiled modes.

A browser UI export from a non-first top-level tab completed as Updated and downloaded the selected chart.

TESTING INSTRUCTIONS

Automated and local verification on final commit 06b78ab259:

• The four new compatibility/deadline regressions failed against the prior implementation, then passed after the fix.
• 220 focused backend unit tests, 20 screenshot API integration tests, and 6 download-hook tests pass.
• Staged pre-commit gates pass (MyPy, Ruff, formatting, and all other applicable hooks); the exact Pylint hook was run in the repository image because the host pre-commit environment did not contain the pylint executable, and rated the branch 10/10.
• A production-style Docker image with Superset, PostgreSQL, Redis, Celery, and Chromium passed standard and tiled success/failure flows: valid PNG/PDF for non-first/nested/chart-free tabs, and Error plus artifact GET 404 for a chart exceeding SCREENSHOT_LOAD_WAIT.
• The broader visualization matrix passed for Word Cloud, DeckGL single/multi, Point Cluster, Cartodiagram, and multi-tile capture; the final guard-only commit changes no readiness predicate or plugin code.
• Eight simultaneous forced POSTs coalesced to one producer and seven observers; forced refresh kept the prior artifact downloadable while the new generation ran.

Manual verification:

1. Enable THUMBNAILS and ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS, configure the screenshot cache/Celery, and start Superset plus a worker.
2. POST /api/v1/dashboard/<id>/cache_dashboard_screenshot/ and record cache_key, dashboard_url, and image_url.
3. Extract the permalink key from the last path segment of dashboard_url, then poll the same POST endpoint with {"permalinkKey": "<key>"} until task_status is terminal.
4. Confirm successful work becomes Updated and the returned PNG/PDF URL is HTTP 200.
5. Delay a selected-tab chart beyond SCREENSHOT_LOAD_WAIT; confirm terminal Error and artifact GET 404.
6. Force-refresh a successful request; confirm the new generation is Pending while the prior generation URL remains downloadable.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags: THUMBNAILS, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API: additive task status/timeout response metadata; no endpoint, request field, or status-code removal
• Removes existing feature or API

[codecov]

Codecov Report

❌ Patch coverage is 73.89937% with 83 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.27%. Comparing base (0049f81) to head (6eff102).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/utils/screenshots.py	63.24%	36 Missing and 7 partials ⚠️
superset/dashboards/api.py	68.00%	22 Missing and 2 partials ⚠️
superset/utils/screenshot_utils.py	73.07%	5 Missing and 2 partials ⚠️
superset/utils/webdriver.py	92.30%	2 Missing and 2 partials ⚠️
superset/tasks/thumbnails.py	81.25%	1 Missing and 2 partials ⚠️
...ntend/src/dashboard/hooks/useDownloadScreenshot.ts	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44144      +/-   ##
==========================================
- Coverage   80.28%   80.27%   -0.01%     
==========================================
  Files        2928     2928              
  Lines      174116   174372     +256     
  Branches    40402    40446      +44     
==========================================
+ Hits       139782   139971     +189     
- Misses      31677    31730      +53     
- Partials     2657     2671      +14     

Flag	Coverage Δ
javascript	75.68% <93.54%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[msyavuz]

Core fix looks right: persisting Pending before enqueue and coalescing repeat/force POSTs closes both the spinner-as-Updated case and the per-poll task storm (every poll on a cache miss enqueued a new task on master). Two asks: fix the openapi drift check, and consider splitting the report/deck.gl readiness changes into their own PR so the API state-machine fix can land and be backported alone. Note nothing here stops an Updated entry from being evicted between the POST and a later GET, so 404-after-Updated can still happen on eviction-heavy caches.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	4be12fc
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6aaa7ef7f862670009d08907
😎 Deploy Preview	https://deploy-preview-44144--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[msyavuz]

Prior threads check out against the head: default readiness predicate is logic-equivalent to master and the report dwell path is unchanged, locks and the setup-failure fallback are ordered correctly, markers match the backend selectors, openapi matches the schemas. pending() still nulls a good artifact on force, but since the UI now GETs only after Updated that's no longer observable.

One recurring gap in the new code: every readiness marker only has loading | rendered, with no terminal failed value. So a map/style/tile/registry failure doesn't produce a prompt Error, it burns the full readiness budget and then errors. The four inline comments below are instances of that; a failed marker value the backend selector treats as terminal would fix all of them.

[bito-code-review]

Code Review Agent Run #6d771c

Actionable Suggestions - 0

Additional Suggestions - 5

• superset/utils/webdriver.py - 1

  • Readiness gate always fires · Line 355-356
    `page.evaluate(capture_readiness_predicate)` passes an arrow-function *expression* string (e.g. `() => {...}`). Playwright evaluates it as an expression, yielding a function object that cannot be serialized, so it returns `undefined`. `not bool(undefined)` is always `True`, so this gate always discards the capture and raises `ScreenshotCaptureReadinessChangedError` when `require_complete_capture` is set. Wrap in an IIFE: `page.evaluate(f"({capture_readiness_predicate})()")`.
• superset/dashboards/api.py - 1

  • Blocking lock poll loop · Line 2153-2153
    The `while True` loop blocks the request worker for up to `SCREENSHOT_API_LOCK_WAIT_SECONDS` (1s) via `time.sleep` while polling `DistributedLock`. Under concurrent screenshot requests this can exhaust synchronous gunicorn workers. Consider returning 202/503 immediately on `LockAlreadyHeldException` and letting clients poll the status endpoint instead of sleeping in the handler.
• superset/utils/screenshots.py - 2

  • Cache error now 500 · Line 356-361
    `get_from_cache_key` now raises `ScreenshotCacheError` on cache read failure instead of returning None. The producer path (`get_current_generation`, api.py:2021) handles this with a 503, but the `thumbnail` trigger path (api.py:2335-2336) is unwrapped, so a transient cache backend error now surfaces as a generic 500 via `@safe` instead of the previous graceful async fallback (202). Consider handling it consistently.
  • Inconsistent write check · Line 497-500
    When `require_complete_capture` is False, the write uses raw `self.cache.set(...)` (line 500) without the `stored is False` failure check that `store_cache_payload` applies. This leaves the non-complete-capture path (charts) unable to detect a rejected write, diverging from the new guarded path.
• superset/utils/screenshot_utils.py - 1

  • Fail-closed gate skips reports · Line 1361-1361
    This fail-closed check is gated on `require_complete_capture`, which is `False` for report execution (`execute.py` builds `DashboardScreenshot` without it), even though reports set `strict_capture=True` (line 851). So report tiles captured during a render transition are never discarded here. Consider gating on `strict_capture` to match the pre-capture stability intent.

Review Details

• Files reviewed - 11 · Commit Range: 7eaaa95..25ec992

  • superset/dashboards/api.py
  • superset/tasks/thumbnails.py
  • superset/utils/screenshot_utils.py
  • superset/utils/screenshots.py
  • superset/utils/webdriver.py
  • tests/integration_tests/dashboards/api_tests.py
  • tests/unit_tests/tasks/test_thumbnails.py
  • tests/unit_tests/utils/screenshot_test.py
  • tests/unit_tests/utils/test_screenshot_cache_fix.py
  • tests/unit_tests/utils/test_screenshot_utils.py
  • tests/unit_tests/utils/webdriver_test.py
• Files skipped - 1

  • docs/static/resources/openapi.json - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[EnxDev]

I traced the generation lifecycle and capture changes at 25ec992 and found one cache-expiry case worth fixing below. I reproduced it with the PR's cache helpers and a controlled SimpleCache clock. The focused pytest run couldn't start because the configured SQLite test database was unavailable, so I haven't validated the full suite locally.

[EnxDev]

Thanks for fixing the generation-key reuse. I checked the UUID change and the expiry regression passes. This pass found one more case in the worker's cache-error handling, reproduced below. Focused validation: 204 tests passed; 10 failed because the isolated SQLite test database has no key_value table, so those lock-dependent checks remain unverified.

[EnxDev]

Thanks for the follow-up fixes. I checked the strict worker reads and their no-write regressions, the generation-key fix, the producer-lock response handling, and the capture/polling budget changes. I didn't find any additional actionable issues in this pass.

Validation on 4be12fc: 207 focused backend tests passed, including both cache-read failure regressions, and all 6 useDownloadScreenshot tests passed with the repository's Node 24 version. I excluded the 10 lock-dependent backend tests that need the missing test-database setup; I haven't run the API integration suite or a live browser export locally. Leaving this as a comment review given those validation limits.