eschutho opened a new pull request, #42120: URL: https://github.com/apache/superset/pull/42120
### Decisions made that were not in the instructions - **Validation depth**: chose a cheap PNG/JPEG magic-byte header check (`\x89PNG\r\n\x1a\n` / `\xff\xd8\xff`), not a full image decode, per the instruction to keep this cheap. A full decode would add CPU cost to every cache read/write for a class of corruption (truncated/blank-but-headered files) that a header check won't catch — deemed out of scope for closing the specific "blank PDF from stale cache" report. - **Chart-screenshot endpoint**: no changes were needed to `superset/charts/api.py`. Both the dashboard and chart `screenshot()`/`thumbnail()`/`cache_*` handlers already funnel through the shared `BaseScreenshot.get_from_cache_key()` classmethod, so fixing validation there closes the read-side hole for both resource types mechanically, with no separate chart-specific patch. - **Where the read-side check lives**: added it to `get_from_cache_key()` rather than to `ScreenshotCachePayload.get_image()` or `__init__`. This scopes the change to the cache-retrieval choke point (what the endpoints actually call to decide "hit vs. miss") without changing the behavior of `get_image()`/`ScreenshotCachePayload` construction elsewhere (e.g. the Celery report/alert pipeline, which calls `get_screenshot()` directly and never touches this cache layer). ### SUMMARY Dashboard/chart screenshot and thumbnail caching (`ScreenshotCachePayload` in `superset/utils/screenshots.py`) had two gaps: 1. **No read-side validation.** `BaseScreenshot.get_from_cache_key()` returned whatever was in the cache as long as a status of `UPDATED` was recorded, even if the stored image was `None`/0-byte or otherwise not a real image. Since the cache key is digest-based, an unchanged dashboard/chart kept serving the same stale/blank entry indefinitely (e.g. a blank PDF download). 2. **Write-side validation was falsy-only.** A prior fix (#41097) set `ERROR` status when the screenshot task produced a falsy (`None`/`b""`) result, but a non-empty, non-image payload (e.g. truncated/corrupt bytes) still passed the `if image:` check and got cached with `UPDATED` status. This PR adds a shared, cheap validator (`validate_screenshot_image()` — checks non-empty + PNG/JPEG magic-byte header, no full decode) used on both paths: - **Read side**: `get_from_cache_key()` now rejects a cached payload that claims a successful screenshot (`status == UPDATED`) but fails validation, returning `None` — the same value callers already treat as a cache miss — and logs a `WARNING` with the cache key and the reason (`empty` vs `undecodable`). Because both the dashboard and chart `screenshot`/`thumbnail`/`cache_*` endpoints already call this same shared classmethod, this closes the hole for both resource types without touching `charts/api.py` or `dashboards/api.py`. - **Write side**: `BaseScreenshot.compute_and_cache()` now runs the same check on the freshly generated image before caching it as a success. If it fails, the payload is marked `ERROR` (consistent with #41097's approach) instead of `UPDATED`, and a `WARNING` with the cache key and reason is logged. **Explicitly not changed:** - Capture/webdriver logic (`superset/utils/screenshot_utils.py`, `superset/utils/webdriver.py`) — untouched, owned by concurrent PRs #42119 and #42118. - Cache backend, cache keys, and TTLs — unchanged. - Behavior for correctly-cached images — a valid, non-empty PNG/JPEG payload is served and cached exactly as before; this only changes behavior for payloads that were already broken (empty/corrupt). - No new config flags — the validation is unconditional, since serving a 0-byte image is never the intended behavior. ### TESTING INSTRUCTIONS Added unit tests in `tests/unit_tests/utils/test_screenshot_cache_fix.py` and updated fixtures in `tests/unit_tests/utils/screenshot_test.py` (existing tests used non-image placeholder bytes like `b"image_data"` as stand-ins for a "successful" screenshot; these now use a minimal valid PNG header so they still exercise the success path under the new validation): - Read side: a cached payload with a 0-byte image is served as a cache miss (`None`) with a `WARNING` logged; a payload with non-image garbage bytes is likewise treated as a cache miss; a valid PNG-header payload is served normally; a non-`UPDATED` (e.g. `PENDING`) payload is returned as-is. - Write side: `compute_and_cache()` with an empty or garbage-bytes screenshot result caches `ERROR` status (never `UPDATED`) and logs a `WARNING` including the cache key and reason. Run: ``` pytest tests/unit_tests/utils/test_screenshot_cache_fix.py tests/unit_tests/utils/screenshot_test.py -q ``` 48 passed. Also ran the broader `tests/unit_tests/utils/ -k screenshot` suite (76 passed); 3 unrelated pre-existing failures in `webdriver_test.py`/`test_screenshot_utils.py` reproduce identically on `master` (playwright-version mismatch in the test environment, unrelated to this change and in files this PR does not touch). `ruff check` passes on the changed files; `mypy` reports no errors in `superset/utils/screenshots.py`. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] 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 - [ ] Removes existing feature or API 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
