codeant-ai-for-open-source[bot] commented on code in PR #42118: URL: https://github.com/apache/superset/pull/42118#discussion_r3599324397
########## superset/utils/screenshot_utils.py: ########## @@ -28,6 +29,35 @@ # Time to wait after scrolling for content to settle and load (in milliseconds) SCROLL_SETTLE_TIMEOUT_MS = 1000 +# Total wall-clock budget, in seconds, for the entire tiled-screenshot +# operation (element lookup plus all per-tile spinner/animation waits +# combined). Each tile's wait is capped at whatever remains of this budget +# so a slow dashboard degrades gracefully instead of running past the +# Celery task time limit and getting SIGKILLed mid-capture. +# +# This is a conservative fixed constant rather than a value derived from +# Celery's configured task time limit at runtime, because that limit isn't +# reliably reachable from here: superset/tasks/scheduler.py sets it +# per-report-schedule via `apply_async(time_limit=..., soft_time_limit=...)` +# only when a schedule defines `working_timeout` +# (ALERT_REPORTS_WORKING_TIME_OUT_KILL); there is no static config value +# that always reflects the limit actually enforced, and this function can +# also run outside of a Celery task entirely (e.g. synchronous thumbnail +# generation). +# +# Production has observed a Celery hard task_time_limit of 1740s (29 min) +# for report execution (2026-07-13 incident: a tiled screenshot was killed +# mid-capture with SoftTimeLimitExceeded). This budget leaves a 300s margin +# under that ceiling for the rest of the pipeline that runs after tiling +# completes: combining tiles into one image, building the PDF, and +# uploading/delivering the notification. +TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440 # 1740s limit - 300s margin Review Comment: **Suggestion:** The hardcoded 1440-second budget is not aligned with per-schedule Celery `time_limit`/`soft_time_limit` overrides, which can be configured lower via `working_timeout`. In those deployments this function can still run longer than the actual worker limit and get killed before failing cleanly. Derive the budget from the active report/task timeout when available (or enforce a minimum of configured task limits), and only fall back to a static default when no runtime limit is accessible. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx ❌ Report screenshot tasks can be killed mid-tiling. ⚠️ Budgeted waits ignore per-schedule Celery time limits. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a report schedule with a lower `working_timeout` via the `working_timeout` field exposed in the Reports API (`superset/reports/api.py:148, 206`), e.g. 600 seconds. 2. Celery beat uses that `working_timeout` to set per-schedule `time_limit` and `soft_time_limit` in `scheduler()` at `superset/tasks/scheduler.py:23-35`, so the worker enforces a shorter execution window than the global 1740s default. 3. When the schedule fires, `reports.execute` (`superset/tasks/scheduler.py:38-56`) runs `AsyncExecuteReportScheduleCommand.run()` (`superset/commands/report/execute.py:22-77`), which calls `_get_screenshots()` (`superset/commands/report/execute.py:520-79`) and ultimately `BaseScreenshot.get_screenshot()` (`superset/utils/screenshots.py:53-62`) using `WebDriverPlaywright` and `take_tiled_screenshot()` for large dashboards. 4. `take_tiled_screenshot()` applies the fixed `TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440` (`superset/utils/screenshot_utils.py:54, 61-83, 93-121`); on slow, many-tile dashboards this allows up to ~1440s of tiling before `TiledScreenshotBudgetExceededError`, but the Celery worker may hit its lower per-schedule `time_limit`/`soft_time_limit` first, yielding `SoftTimeLimitExceeded` or a hard kill instead of the intended clean budget exhaustion failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed6d8ebe609b419da381394a77026170&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ed6d8ebe609b419da381394a77026170&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/screenshot_utils.py **Line:** 54:54 **Comment:** *Possible Bug: The hardcoded 1440-second budget is not aligned with per-schedule Celery `time_limit`/`soft_time_limit` overrides, which can be configured lower via `working_timeout`. In those deployments this function can still run longer than the actual worker limit and get killed before failing cleanly. Derive the budget from the active report/task timeout when available (or enforce a minimum of configured task limits), and only fall back to a static default when no runtime limit is accessible. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=5b9c742566caae0ed56f4248ef3e0ea5c585ee40b22d48ca08660042c8578a1b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=5b9c742566caae0ed56f4248ef3e0ea5c585ee40b22d48ca08660042c8578a1b&reaction=dislike'>👎</a> ########## superset/utils/screenshot_utils.py: ########## @@ -150,9 +210,12 @@ def take_tiled_screenshot( ) # Wait for scroll to settle and content to load page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS) - # Wait for any loading spinners visible in the current viewport to clear. + # Wait for any loading spinners visible in the current viewport to clear, + # capped at whatever remains of the total time budget so a slow + # dashboard degrades gracefully instead of exceeding it. # Only check viewport-visible spinners to avoid blocking on # virtualization placeholders rendered for off-screen charts. + tile_load_wait = min(load_wait, remaining_budget) Review Comment: **Suggestion:** The spinner timeout is derived from a stale remaining-budget value that was computed before the mandatory scroll-settle sleep. This lets each tile wait up to one extra settle interval beyond the declared global budget, so total runtime can still exceed the intended cap. Recompute the remaining budget immediately before `wait_for_function` (after settle wait) and cap spinner wait with that refreshed value. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Tiled screenshots may exceed configured global wait budget. ⚠️ Celery task margin eroded on heavily tiled dashboards. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Schedule a dashboard report so Celery runs `reports.execute` at `superset/tasks/scheduler.py:38-56`, which invokes `AsyncExecuteReportScheduleCommand.run()` at `superset/tasks/scheduler.py:131-135`. 2. Inside `AsyncExecuteReportScheduleCommand._get_screenshots()` (`superset/commands/report/execute.py:520-79`), a `DashboardScreenshot` or `ChartScreenshot` is constructed and `BaseScreenshot.get_screenshot()` (`superset/utils/screenshots.py:53-62`) is called. 3. `BaseScreenshot.driver()` returns `WebDriverPlaywright` when enabled (`superset/utils/screenshots.py:37-41`), whose `get_screenshot()` calls `take_tiled_screenshot()` (`superset/utils/webdriver.py:40-51, 213-219, 399-49`) for large dashboards. 4. In `take_tiled_screenshot()` (`superset/utils/screenshot_utils.py:61-121`), `remaining_budget` is computed before `page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` at line ~93, then `tile_load_wait = min(load_wait, remaining_budget)` at line ~99 uses that stale value; this lets each tile spend `SCROLL_SETTLE_TIMEOUT_MS` extra per loop beyond the nominal `TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS`, so total wall-clock time can exceed the configured budget before `TiledScreenshotBudgetExceededError` is raised. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0c858d2b97344bc5be87dfe5c5b082b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0c858d2b97344bc5be87dfe5c5b082b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/screenshot_utils.py **Line:** 218:218 **Comment:** *Logic Error: The spinner timeout is derived from a stale remaining-budget value that was computed before the mandatory scroll-settle sleep. This lets each tile wait up to one extra settle interval beyond the declared global budget, so total runtime can still exceed the intended cap. Recompute the remaining budget immediately before `wait_for_function` (after settle wait) and cap spinner wait with that refreshed value. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=7100d668f092031d049f284f9a9973ecd220d0cff9ff6b45c5ee616121ea3bc7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42118&comment_hash=7100d668f092031d049f284f9a9973ecd220d0cff9ff6b45c5ee616121ea3bc7&reaction=dislike'>👎</a> -- 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]
