rebenitez1802 commented on code in PR #42118:
URL: https://github.com/apache/superset/pull/42118#discussion_r3693643801
##########
superset/utils/screenshot_utils.py:
##########
@@ -286,6 +315,15 @@ def take_tiled_screenshot(
# match `except PlaywrightTimeout` and incorrectly propagate instead of
# degrading to `None` like every other unexpected error in this function.
readiness_timeout = False
+ # Cap the whole tiled operation against the running Celery task's own
+ # time limit, using the same runtime derivation as the non-tiled
+ # readiness wait (#42253/#42427). Unlike that path, a None budget does
+ # not mean "keep the configured timeout": per-tile waits accumulate, so
+ # the operation falls back to a fixed total ceiling instead.
+ wait_budget_seconds = resolve_screenshot_task_budget_seconds(log_context)
+ if wait_budget_seconds is None:
+ wait_budget_seconds = float(TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS)
+ start_time = time.monotonic()
Review Comment:
**🟡 Non-blocking (owner's call) — tiled budget clock resets instead of
accounting for pre-capture time.**
`wait_budget_seconds` is derived from the Celery task limit, but elapsed is
measured from this local `start_time`, so `page.goto` (bounded 60s), the 3s
`SELENIUM_HEADSTART`, the 30s `element.wait_for`, and dimension probing all run
*before* the clock starts — the tiled path effectively gets a fresh full
budget. The non-tiled `_wait_for_charts_ready` avoids this by threading
`screenshot_started_at` from the top of `get_screenshot` and subtracting
already-elapsed time; the tiled call site doesn't pass it.
Not a correctness bug: in the common case the 20%/300s margin (and the
soft→hard gap) absorbs the difference, and a soft-limit overrun is still caught
cleanly as `SoftTimeLimitExceeded` rather than the SIGKILL this PR targets.
It's just a looser guarantee than the non-tiled path — worth a conscious
decision.
If you want both paths on one clock, it's 3 coordinated one-liners (⚠️
applying the suggestion below *alone* will `NameError` until the other two
land):
1. add `screenshot_started_at: float | None = None` to the
`take_tiled_screenshot` signature;
2. the change below;
3. pass `screenshot_started_at=screenshot_started_at` from the
`take_tiled_screenshot(...)` call in `webdriver.py`.
```suggestion
start_time = (
screenshot_started_at if screenshot_started_at is not None else
time.monotonic()
)
```
--
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]
