fitzee commented on PR #42624: URL: https://github.com/apache/superset/pull/42624#issuecomment-5138139118
## Griffen review (Claude + Codex dual pass) **Verdict: Needs changes** — full agreement between both reviewers on verdict; noting this is already in draft with an explicit "do not merge" from the author, so treat the below as pre-merge punch list rather than a blocker on the current state. ### Door classification: Mixed Code-level revert is clean (no schema/migration). But two behaviors have one-way-door side effects during the live window: customer notifications missed on Celery soft-timeout can't be un-missed after the fact, and the stale-row recovery race (below) can write incorrect terminal state into the execution-log audit trail that a later code revert doesn't repair. ### HIGH — Celery soft-timeout now suppresses all customer notification For every REPORT-type schedule (dashboard screenshots **and** CSV/Excel/data exports) that hits the Celery soft time limit, the recipient now gets zero notification — `test_soft_timeout_csv` confirms `email_mock.assert_not_called()`. Previously a timing-out CSV report sent an error email. The tradeoff (skip the notification round-trip so the hard-limit grace window is spent on guaranteed terminal-state persistence) is reasonable engineering, but the customer-facing silent-failure implication isn't disclosed as a behavior change, and there's no compensating signal (metric/alert on `terminal_reason=celery_soft_timeout`) for ops to notice it happening. Suggest: add an observable signal at minimum; consider a durable outbox/idempotency-keyed notification task decoupled from the terminal-state write if in-band notification is wanted back. ### MEDIUM — stale-WORKING-row recovery can race the *original* worker, not just itself `ReportWorkingState.next()` mutates a stale `WORKING` execution-log row to `ERROR` (a different row than the current invocation's own) when the last-working uuid differs from the current execution_id. This assumes the original worker is actually dead. That's only guaranteed under Celery's `prefork` pool (hard `time_limit` → SIGKILL of the child process). Under `solo`/`eventlet`/`gevent` pools, a stuck synchronous Playwright/Selenium call can't be preempted by Celery's soft/hard limits at all — so the original worker can still be alive and later commit `SUCCESS` to that same row while a subsequent recovery invocation concurrently writes `ERROR` to it. That's a lost-update on the audit trail, not the harmless idempotent duplication the current tests (which mock the DAO lookup) exercise. Suggest a conditional write (`UPDATE ... WHERE uuid=? AND state=WORKING`, checking affected-row count) or `SELECT ... FOR UPDATE` before promoting the stale row, plus a test that has the "lost" w orker still alive and racing recovery. ### MEDIUM — budget/reserve config validated lazily, not at boot `ReportExecutionContext.__post_init__` (and `get_report_task_timeout_options()`) reject invalid reserve/budget combinations, but only when a REPORT schedule actually executes — not at app startup. A misconfigured deploy (e.g. bumping `ALERT_REPORTS_EXECUTION_DELIVERY_RESERVE_SECONDS` without checking the sum) fails every scheduled dashboard report until an operator notices the error-log pileup. Suggest validating this invariant at config-load/app-boot time. ### MEDIUM — coverage gap on exactly the branches that decide over/under-aggressive termination Codecov: `superset/utils/webdriver.py` patch coverage 9.64% (96 lines missing), `superset/utils/screenshot_utils.py` 8.10% (68 lines missing). The `ReportExecutionDeadline`/`ReportExecutionContext` arithmetic itself is well unit-tested in isolation, but most of the Selenium-path `report_execution_context` wiring (phase_timeout → `WebDriverWait`, `set_page_load_timeout`, animation-wait clamping) is untested. This is the class of code where a seconds/ms conversion or reserve-ordering bug would only show up as production reports dying too early or hanging too long. Suggest at least one Selenium-path integration test analogous to the Playwright tiled-path tests that already exist. ### MEDIUM (non-blocking) — undisclosed tiling-decision change In `WebDriverPlaywright.get_screenshot`, `use_tiled` changed from `(chart_count >= chart_threshold or dashboard_height > height_threshold) and dashboard_height > tile_height` to `chart_count >= chart_threshold or (dashboard_height > height_threshold and dashboard_height > tile_height)` — dropping the `dashboard_height > tile_height` guard from the chart-count branch, for all captures (reports and thumbnails). Not mentioned in the PR description. Likely safe (degrades to a single tile), but unverified by any test targeting exactly this case — worth a regression test and a one-line callout in the description since it's a behavior change riding along in a reliability fix. ### LOW `_get_pdf()`'s budget check is a before/after tripwire, not an enforced timeout — `build_pdf_from_screenshots` itself isn't preemptible, so an unusually slow PDF build can still overrun the budget before the after-check catches it. Acceptable given no easy synchronous timeout primitive here, just worth naming as a known gap. ### What's sound The core architecture — one shared monotonic deadline instead of independent screenshot/Celery/working_timeout constants — is the right fix, backed by real staging evidence (dashboard 10, 52 charts, 300s+ captures) rather than a guessed constant. `ReportExecutionDeadline`/`ReportExecutionContext` are clean and well-tested in isolation. The self-authored "Adversarial scope/recovery review" section correctly pre-empted the Celery signal-handler DB-safety and duplicate-delivery-on-replay hazards. ALERT-type schedules are correctly left untouched throughout, and `combine_screenshot_tiles(allow_partial_fallback=False)` for reports correctly refuses to ever deliver a truncated image. --- *Reviewed via Claude (Griffen) + Codex dual pass, synthesized. Full internal writeup on file.* -- 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]
