Fix screenshot timing out when the target page is not in the foreground - #55
Open
goeric wants to merge 1 commit into
Open
Fix screenshot timing out when the target page is not in the foreground#55goeric wants to merge 1 commit into
goeric wants to merge 1 commit into
Conversation
"rodney screenshot" fails with "context deadline exceeded" whenever the
page it targets is not the browser's foreground one:
rodney open <heavy page>
rodney screenshot a.png # works
rodney newpage <other url>
rodney page 1 # back to the heavy page
rodney screenshot b.png # context deadline exceeded
rodney's "active page" is only an index in its own state file. The
browser is never told about it, so after "rodney page N" the foreground
target is still whichever page was opened last. DOM and JS commands do
not care -- they read a hidden target perfectly well, which is why url,
js and text all keep reporting the switched-to page correctly -- but a
backgrounded target has no compositor producing frames, and
Page.captureScreenshot waits for one that never arrives.
Page weight is a red herring: a heavy page captures in 0.23s when it is
in the foreground, while a backgrounded example.com times out. What
decides it is foreground, not size. Element capture is affected too, and
worse: rod's stability wait ignores the page timeout, so screenshot-el
on a hidden target hangs for over eight minutes rather than 30 seconds.
cmdPage now switches the browser as well as the index, and both capture
commands raise their target first, so a state file that has drifted out
of sync with the browser cannot wedge a capture.
Not a regression from the crash fixes: it reproduces identically on the
pre-crash-fix binary with --single-process and old headless.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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
rodney screenshotfails withcontext deadline exceededwhenever the page ittargets is not the browser's foreground target:
Root cause
rodney's "active page" is only an index in its own state file (
active_page).The browser is never told about it, so after
rodney page Nthe foregroundtarget is still whichever page was opened last.
cmdPagewrote the index andnothing else —
Activate/bringToFrontappeared nowhere in the codebase.DOM and JS commands don't care: they read a hidden target perfectly well, which
is why
url,jsandtextall keep reporting the switched-to page correctly.But a backgrounded target has no compositor producing frames, so
Page.captureScreenshotwaits for one that never arrives.Page size is a red herring. Measured: a heavy page captures in 0.23s when
it is in the foreground, while a backgrounded
example.comtimes out after 30s.What decides it is foreground, not weight — the reported "heavy page" correlation
was a confound, because the heavy page happened to be the backgrounded one.
screenshot-elis affected too, and worse: rod's element stability wait ignoresthe page timeout, so a hidden target hangs for over eight minutes rather than
30 seconds.
pdfis unaffected —printToPDFre-renders and needs no frame.Fix
cmdPageswitches the browser as well as the index, so the page a caller justselected is the one being rendered.
cmdScreenshotandcmdScreenshotElraise their target before capturing, so astate file that has drifted out of sync with the browser (page opened by
window.open, foreground page closed) cannot wedge a capture.Not a regression from the crash fixes
It reproduces identically on the pre-crash-fix binary with
--single-processandold headless, so it is pre-existing and unrelated to #54 or to extension mode.
Verification
screenshot-el, backgroundedThree regression tests added. Verified they genuinely catch the bug: with
bringToFrontneutered they fail in 51s total (0.08s/20.06s/30.07s);with the fix they pass in 3.4s. The element test bounds its own wait so a
regression fails that one test rather than panicking the suite ten minutes later,
which is what an unbounded version did.
go build,go vetand the fullgo test ./...suite pass.🤖 Generated with Claude Code