Give Screen.open enough budget to retry a slow page load - #291
Draft
evnchn wants to merge 1 commit into
Draft
Conversation
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.
Drafted by Claude Code on evnchn's behalf.
Fork staging PR — for review before anything goes upstream.
Motivation
Screen.open()has a retry loop, but it can never retry the failure it most needs to: a slow page load.Two numbers live in different files and are silently coupled the wrong way round:
screen_plugin.py—driver_.set_page_load_timeout(4)screen.py—def open(self, path: str, timeout: float = 3.0)A single
selenium.get()may block for the full 4 s page-load timeout. By the time theexceptbranch checksif time.time() > deadline, the 3 s budget has already expired — soopen()re-raises on the first attempt. The loop below it is dead code for this failure mode:This is not hypothetical — it is the mechanism behind two separate merge-queue flakes:
tests/test_add_html.py::test_add_head_htmltests/test_alternate_ui_frameworks.py::test_element_plusBoth died with
timeout: Timed out receiving message from renderer, each after exactly one attempt.Implementation
Make the coupling explicit and give the retry loop room to actually run:
and in
screen_plugin.py,driver_.set_page_load_timeout(Screen.PAGE_LOAD_TIMEOUT)so the two can no longer drift apart silently. The docstring states the invariant: the budget must stay abovePAGE_LOAD_TIMEOUT, or there is no room to retry.A page that is genuinely unreachable now takes ~12 s to fail instead of ~4 s. That cost is paid only by failing tests.
Verification
Instrumented
selenium.get()to count attempts, with CDP network latency forcing every navigation past the page-load timeout:selenium.get()attempts beforeopen()gave upNo regressions: full suite on macOS / py3.14 —
9 failed, 1003 passed, 3 skipped, 2 xfailed. The 9 are the pre-existing macOS failures (test_dark_mode[*]oklchassertions,test_module_access_does_not_import_others), identical to a run on unmodifiedmain.Gates: pre-commit all Passed · mypy clean over 245 source files · pylint 10.00/10.
Relationship to the other flake fixes
This is the shared root cause underneath the renderer-timeout family. It is independent of, and complementary to, the CDN fix: removing the external fetch stops
test_element_plusfrom needing >4 s, while this gives every test a second chance when a page load runs long for any reason.It is also why
test_add_head_html— a test with no network dependency at all — failed once in CI: being first in the suite, it pays the cold-browser cost with no retry to absorb it.Progress
open()keeps its signature; only the default timeout grows.