eschutho opened a new pull request, #42118:
URL: https://github.com/apache/superset/pull/42118
### Decisions made that were not in the instructions
- **Safety margin: 300s.** Budget = 1740s (observed Celery hard
`task_time_limit` for report execution) minus a 300s margin for the pipeline
steps that run after tiling: combining tiles, building the PDF, and
uploading/delivering the notification. Resulting budget constant:
`TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440`.
- **Config-derivation approach: fixed constant, not runtime-derived.**
Investigated whether the actual Celery time limit is reachable from
`take_tiled_screenshot()`. It is not reliably reachable:
`superset/tasks/scheduler.py` only sets `time_limit`/`soft_time_limit` per
report schedule via `apply_async(...)`, and only when that schedule defines
`working_timeout` (gated by `ALERT_REPORTS_WORKING_TIME_OUT_KILL`) — there's no
static Superset config key that always reflects the limit actually enforced at
runtime. This function can also run outside a Celery task entirely (e.g.
synchronous thumbnail generation), where there is no task context to introspect
at all. Given that, I used a conservative module-level constant with a comment
documenting the 1740s constraint it must stay under, per the task instructions'
fallback guidance.
- **Animation wait exhaustion doesn't raise.** When the budget runs out
mid-tile for the *spinner* wait, we abort (raise) because that's the actual
readiness check. When it runs out for the trailing *animation* wait (cosmetic
settling after the spinner already cleared), we just skip that wait rather than
raising, since the tile has already been verified ready.
### SUMMARY
`take_tiled_screenshot()` in `superset/utils/screenshot_utils.py` waited up
to `load_wait` (production: `SCREENSHOT_LOAD_WAIT`, up to 600s) per tile for
loading spinners to clear, with no cap on cumulative wait across all tiles. On
dashboards with many tiles, `load_wait × num_tiles` could exceed the Celery
`task_time_limit`, so the worker was SIGKILLed mid-capture instead of the
report failing cleanly — observed in production on 2026-07-13 as `Tiled
screenshot failed: SoftTimeLimitExceeded()`.
This PR:
- Tracks wall-clock elapsed time across the whole tiled-screenshot operation
via `time.monotonic()`.
- Caps each tile's spinner wait (and trailing animation wait) at whatever
remains of a fixed `TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440` (1740s
Celery limit − 300s margin) budget, so a slow dashboard degrades gracefully
instead of accumulating unbounded waits.
- If the budget is exhausted before a tile's readiness can even be checked,
logs an `ERROR` with tiles completed/remaining, elapsed time, and budget, then
raises a new `TiledScreenshotBudgetExceededError` — a hard failure, not a
silent `None`/partial result. This propagates up through
`WebDriverPlaywright.get_screenshot` → `ChartScreenshot.get_screenshot` →
`AsyncExecuteReportScheduleCommand._get_screenshots`, which already wraps it in
`ReportScheduleScreenshotFailedError`, so the report errors and owners are
notified instead of receiving a blank/partial report.
### NOT changed (intentionally)
- The per-tile readiness predicate itself (the `.loading` viewport-spinner
check) and its timeout/warn-and-continue semantics on an individual
`PlaywrightTimeout` are unchanged — that's owned by the concurrent
`fix-tile-readiness-check` PR. These two PRs touch adjacent lines in the same
loop; whichever merges second will need to rebase/resolve.
- No changes to `superset/utils/webdriver.py`'s existing
fallback-to-standard-screenshot behavior when `take_tiled_screenshot()` returns
`None` for non-budget errors (that path is untouched).
### TESTING INSTRUCTIONS
- New unit tests in `tests/unit_tests/utils/test_screenshot_utils.py` (class
`TestTileWaitBudget`):
- `test_per_tile_wait_shrinks_as_budget_depletes` — mocks `time.monotonic`
to simulate elapsing time and asserts `wait_for_function` timeouts shrink
tile-over-tile as the budget depletes.
- `test_budget_exhausted_raises_and_stops_capturing` — simulates the
budget hitting zero mid-loop; asserts `TiledScreenshotBudgetExceededError` is
raised, no further tiles are captured or combined, and an `ERROR` is logged
with the tiles-captured/total/elapsed/budget values.
- `test_fast_dashboard_matches_default_behavior` — a fast (real-time) run
stays well under budget and behaves identically to before (full
`load_wait`/`animation_wait` per tile).
- Ran the full existing `tests/unit_tests/utils/test_screenshot_utils.py`
suite: 23 passed, 1 pre-existing failure
(`test_per_tile_spinner_timeout_logs_warning_and_continues`) confirmed
unrelated — it fails identically on `master` in this environment due to a local
Playwright version mismatch (`PlaywrightTimeout()` requiring a `message` arg),
not this change.
- Ran `ruff check`, `ruff format --check`, and `mypy` on the changed files —
clean aside from 3 pre-existing mypy errors in this file that are unchanged
from `master`.
### 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
--
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]
