eschutho commented on PR #42118:
URL: https://github.com/apache/superset/pull/42118#issuecomment-5147790724
Went through the two open CodeAnt findings against the current (reworked)
revision:
**1. "Stale `remaining_budget` reused after the scroll-settle sleep" —
already fixed; the comment references the previous revision.** Its line anchors
point at code that no longer exists (e.g. `_resolve_wait_budget_seconds`,
removed when this PR was reduced to reuse #42427's shared budget helper). The
current revision recomputes elapsed/remaining *after*
`page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS)` and re-checks exhaustion
there before deriving `tile_load_wait` — see the "Recompute the remaining
budget after the scroll-settle sleep" block in `take_tiled_screenshot`, pinned
by `test_readiness_wait_uses_budget_recomputed_after_scroll_settle`. Resolving
as fixed.
**2. "No budget re-check between the animation wait and `page.screenshot()`"
— accurate as a control-flow observation, but intentional; declining the
suggested change.** Reasoning:
- The animation wait is capped at `min(animation_wait, remaining_budget)`,
so it can drain the budget to exactly zero but **cannot overshoot it** — and
the budget is `task limit − cleanup margin` (`min(300s, 20%)`), so even at full
consumption the entire margin remains for capture/combine/PDF/delivery, which
is precisely what the margin exists to cover. The finding's claim that the
function can return "despite exceeding the task limit" doesn't hold: no wait in
this path can run past the budget, let alone the limit.
- The tile captured after that wait is **fully readiness-verified** — its
capped readiness wait already completed within budget. `page.screenshot()` is a
synchronous snapshot, not a wait. The budget-exhaustion raise exists to prevent
unverifiable *future waits*, which doesn't apply here.
- The only behavioral effect of adding the suggested re-check would be on
the **last tile**: a complete, verified, within-budget capture would be
converted into a hard report failure at the finish line (mid-loop tiles are
already covered by the next iteration's top-of-loop check, which raises before
any work on the next tile). That trades a rare successful report for a failure
with no reduction in SIGKILL risk.
- This is the documented design in the code: the animation wait is "cosmetic
settling, not a readiness check, so we simply skip it (rather than raise) once
the budget runs out."
Also in the latest push: refreshed a comment in the budget-exhaustion
`except` clause that still described the pre-#42273 caller behavior ("callers
treat `return None` as fall back to a standard, unchecked screenshot") — since
#42273 merged, callers fail the capture loudly instead. No behavior change; CI
re-running on the amended head.
--
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]
