fix(beholder): guard #fetchImages against scrollHeight runaway and detached frame races#881
Merged
Merged
Conversation
Return `{ scrolled, scrollHeight }` instead of `void` so callers can detect
pages whose post-load scrollHeight exceeds `maxScrollHeight` and abandon the
device preset. Existing callers ignore the return value, so the type widening
is non-breaking. Without `maxScrollHeight` the scroll runs unbounded as
before.
Motivated by a Puppeteer race surfaced in beholder's image extraction:
`scrollAllOver` on a pathological responsive page can run longer than the
@retryable timeout, and `Promise.race` does not cancel `fn()`, leaving
background `page.evaluate` calls colliding with the next retry attempt.
…tached frame races Extend the @retryable timeout from 5 to 20 minutes and skip any device preset whose post-load scrollHeight exceeds 1,000,000 px. The previous 5-minute budget was shorter than the natural scroll time for responsive pages that expand to ~300k px at 320 px viewport, so the retry's `Promise.race` would fire its `RetryTimeoutError` while `scrollAllOver` kept running in the background. The next retry then collided with those pending `page.evaluate` calls on the same Puppeteer page, surfacing as "Attempted to use detached Frame" or "Session closed". The `maxScrollHeight` ceiling backs up the longer timeout: even 20 minutes cannot absorb every pathological layout, and abandoning a device preset is preferable to risking the same retry/background collision again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@d-zero/beholderの#fetchImagesが responsive レイアウトのページで📷 mobile-small: skipped — Attempted to use detached Frame ...を発生させていた問題を修正scrollAllOverが 320 px viewport で 5 分超走り、@retryableの 5 分 timeout がPromise.raceで発火してもfn()を kill しないため、 background のpage.evaluateと次の retry が同じ page で衝突 (Promise.raceの loser-uncancelled 仕様)@retryabletimeout を 5 → 20 分、scrollHeight > 1,000,000 pxの device 全体スキップ (maxScrollHeightガード) を追加Changes
@d-zero/puppeteer-page-scanbeforePageScanのOptionsにmaxScrollHeight?: numberを追加Promise<void>→Promise<{ scrolled: boolean; scrollHeight: number }>に拡張print/a11y-check-core/replicator/puppeteer-screenshot) は戻り値を破棄しているため非破壊document.body.scrollHeightを測定し、超過したら scrollAllOver をスキップしてscrolled: falseを返す@d-zero/beholder#fetchImagesの@retryabletimeout を 5 分 → 20 分に拡張MAX_SCROLL_HEIGHT = 1,000,000を導入し、beforePageScanにmaxScrollHeightとして渡すscrolled: falseを受け取ったら専用メッセージで emit +continueで device 全体をスキップRoot cause analysis
実機調査 (Chrome DevTools MCP) で確認:
document.body.scrollHeight(mobile-small 320 px)@retryabletimeout5 分 timeout が確実に先発火 →
Promise.raceは loser を kill しない → 旧 scroll のpage.evaluateが background で走り続ける → 新 retry が同じ page で setViewport + reload を実行 → main frame navigation と衝突してAttempted to use detached FrameまたはSession closed。frame ID が毎回異なる事実、
Emulation.setTouchEmulationEnabledで session closed が出る事実、 mobile-small でだけ顕在化する事実すべてがこのメカニズムで一貫して説明できる (元々の「iframe detach race」仮説は実機で iframe 0 個を確認したため棄却)。Trade-offs
retry.tsのPromise.race→AbortSignalベース) は全 caller に波及するため別タスクに deferredTest plan
@d-zero/puppeteer-page-scanにmaxScrollHeightガードの単体テスト 6 件追加 (境界 / 超過 / 未指定 /0指定 /page.evaluatereject 伝播)yarn test(775 tests pass)yarn lint(cspell pass)yarn build(27 projects pass)Attempted to use detached Frameが出ないことを確認 (merge 後)🤖 Generated with Claude Code