Give remote-CDP actionability probes the full remaining action budget#96
Open
LawyZheng wants to merge 3 commits into
Open
Give remote-CDP actionability probes the full remaining action budget#96LawyZheng wants to merge 3 commits into
LawyZheng wants to merge 3 commits into
Conversation
Locator actionability/state polling loops capped each individual CDP probe at a fixed 1000 ms and, in the actionability paths, did not catch a probe timeout. A single probe issues several CDP round trips; over a high-latency remote transport (e.g. connect_over_cdp attach) those can exceed 1000 ms, so every probe timed out and the raw TimeoutError aborted the whole click/select_option/fill after ~0.3-0.4 s even though the outer 30 s action deadline was still far off. Budget each probe with the full remaining action time via a new _actionability_probe_timeout helper, and treat a per-probe TimeoutError as a transient retry until the real deadline (matching Playwright). Applied uniformly to the four sibling loops: _wait_for_single, _wait_for_fill_ready, the fill-apply loop, and the select-apply loop. Polling cadence is still driven by _sleep_until_next_poll, so widening the per-probe bound does not busy-loop. For an infinite wait the probe keeps a bounded 1000 ms cadence so owner-availability and locator handlers are still re-checked periodically. Add regression test test_actionability_probe_tolerates_slow_remote_cdp_round_trips: a probe modeled to need more than the old 1000 ms cap fails click()/select_option() under the former fixed cap and passes once each probe receives the full budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The fill-apply loop caught only the generic Error and re-raised a per-probe TimeoutError, so a single actionability/state probe whose CDP round trips outlast the per-probe budget over a high-latency remote transport aborted the whole fill/type even though the outer action deadline was still far off. This was inconsistent with the sibling wait loops (_wait_for_single, _wait_for_fill_ready) and the select-apply loop, which already treat a probe TimeoutError as transient. Catch TimeoutError before the generic Error handler in the fill-apply loop: surface owner unavailability immediately, otherwise keep polling with the existing _sleep_until_next_poll cadence until the outer deadline, then fall through to the loop's normal editable-timeout error. TimeoutError subclasses Error, so the new handler is ordered ahead of it. Add the same owner-availability check to the select-apply loop's existing TimeoutError handler so all four sibling loops preserve close/crash/owner-unavailable propagation uniformly. No timeout is inflated and cadence is unchanged, so this does not busy-loop. Replace the regression coverage: the prior test never triggered the retry path (it only asserted the enlarged per-probe budget). Add call-count-driven tests that force a first-probe TimeoutError on the real affected loops -- fill-apply retry, select-apply retry, _wait_for_single retry -- plus fill-apply outer-deadline behavior and owner-crash propagation on a probe timeout. Reword the test comment to drop the raw benchmark-like interaction ratio. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The native `locator_eval` binding called `evaluate_locator` directly while its siblings `click` and `locator_eval_handle` wrap the same call in `py.detach`. Functionally the GIL was already released one layer deeper (`evaluate_locator` -> `block_on` -> `run_blocking_detached` -> `run_detached` runs the blocking CDP round-trip under `Python::try_attach(|py| py.detach(..))`), so `locator_eval` never actually monopolized the GIL. But the binding-layer inconsistency is worth closing: make the detach explicit so all three locator bindings match and a future refactor that adds GIL-holding work before `block_on` (or changes the transport) cannot silently regress concurrency. Add an invariant guard test proving a blocked native `locator_eval` does not freeze other Python threads (a pure-Python busy thread keeps advancing while the native call waits ~750ms). Also align `_wait_for_single`/`_wait_for_fill_ready` with the fill/select-apply loops: surface owner unavailability inside `except TimeoutError` before the deadline break, so a page/context/browser that closes exactly as a probe times out at the deadline raises the precise owner error instead of a generic actionability timeout. Covered by a focused RED->GREEN test on the live `_wait_for_single` path (`_wait_for_fill_ready` is currently unreferenced, so the mirrored edit is consistency-only). No transport or outer/probe timeout semantics changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LawyZheng
marked this pull request as ready for review
July 18, 2026 06:11
suchintan
reviewed
Jul 21, 2026
| time.sleep(min(interval, remaining)) | ||
|
|
||
|
|
||
| def _actionability_probe_timeout(remaining_ms: float, timeout_disabled: bool = False) -> float: |
Member
There was a problem hiding this comment.
This kind of code should go in the rust library if possible. The python lib should just be a shim
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.
Problem
On the
connect_over_cdp()remote-attach path,click(),select_option(), andfill()could time out prematurely — aborting after roughly 0.3–0.4 s even though thecaller's action deadline (default 30 s) was still far off.
The Locator actionability/state polling loops capped each individual CDP probe at a fixed
1000 ms. A single probe issues several CDP round trips; over a high-latency remote
transport those round trips can exceed 1000 ms, so every probe hit its per-probe cap and
raised
TimeoutError. The_wait_for_singleand_wait_for_fill_readyloops did notcatch that error at all; the fill-apply loop caught only the generic
Errorand re-raisedthe
TimeoutError(sinceTimeoutErrorsubclassesError). Either way the probe timeoutpropagated out and aborted the whole action long before the real deadline.
Fix
via the
_actionability_probe_timeouthelper.TimeoutErroras transient — keep polling until the outer actiondeadline rather than aborting (matching Playwright's behavior).
_wait_for_single,_wait_for_fill_ready,the fill-apply loop, and the select-apply loop. The fill-apply loop now catches
TimeoutErrorbefore the genericErrorhandler; the select-apply loop alreadycaught it.
immediately on a probe timeout instead of masking it: the two wait loops re-check the
owner at the top of each iteration, and the fill/select-apply loops call
_raise_if_owner_unavailablein theirTimeoutErrorhandlers.Polling cadence is still driven by
_sleep_until_next_poll, so widening the per-probebound does not busy-loop. For an infinite wait (
timeout<=0) the probe keeps a bounded1000 ms cadence so owner-availability and locator handlers are still re-checked
periodically.
GIL-release + owner-consistency hardening
locator_evalbinding now releases the GIL explicitly viapy.detach, matching itssiblings
clickandlocator_eval_handle. The GIL was already released for theblocking CDP round-trip one layer deeper —
evaluate_locator→block_on→run_blocking_detached→run_detachedwraps the work inPython::try_attach(|py| py.detach(..))— solocator_evaldid not actuallymonopolize the GIL in practice (verified: a pure-Python busy thread advances ~9M
iterations while a ~0.75 s native
locator_evalblocks). The change closes thebinding-layer inconsistency and guards against future refactors. No probe cap was added.
test_locator_eval_does_not_monopolize_gil_while_blockinglocks in theGIL-release invariant.
_wait_for_singleand_wait_for_fill_readynowalso call
_raise_if_owner_unavailableinside theirexcept TimeoutErrorblock(before the deadline break), so a close that coincides with a probe timeout at the outer
deadline raises the precise owner error instead of a generic actionability timeout —
matching the fill/select-apply loops.
This change is scoped to the Python actionability/state loops plus a one-line native GIL
detach in
src/lib.rs. No transport or outer/probe timeout semantics changed, no timeoutinflation of the outer deadline, no vendor-specific behavior, no retries that bypass
semantics, and no fallback engine.
Tests
Call-count-driven regressions that force a first-probe
TimeoutErroron the realaffected loop paths (rather than only asserting the enlarged per-probe budget):
test_fill_apply_loop_retries_transient_probe_timeout— the fill-apply_evalprobetimes out once, then the fill completes.
test_select_apply_loop_retries_transient_probe_timeout— the select-apply_evalprobe times out once, then
select_option()completes.test_wait_for_single_retries_transient_actionability_probe_timeout— the actionability_target_stateprobe times out once, thenclick()completes.test_fill_apply_loop_surfaces_outer_deadline_on_persistent_probe_timeout— a probethat always times out ends at the outer deadline with the loop's own editable-timeout
message, bounded (no runaway busy-loop).
test_fill_apply_loop_propagates_owner_crash_on_probe_timeout— a page crash during aprobe timeout raises
Page crashedimmediately instead of polling to the deadline.test_wait_for_single_propagates_owner_crash_when_probe_times_out_at_deadline— ownerunavailability wins over a probe timeout even when it coincides with the outer deadline.
test_locator_eval_does_not_monopolize_gil_while_blocking— GIL-release invariant guard.Validation status
Local browser-backed execution runs against the Playwright
chrome-headless-shellbuild(the full Chromium build does not expose its CDP endpoint on this host); under that build
the actionability/GIL regressions plus the surrounding fill/select/click suite pass. Rust
cargo check/cargo fmt/cargo test --libare green. External high-latency remote-transport validation has completed successfully underthe driver-default operation timeout, covering the full fill/select/click interaction
sequence with clean teardown. The change is ready for review.