Skip to content

Give remote-CDP actionability probes the full remaining action budget#96

Open
LawyZheng wants to merge 3 commits into
mainfrom
remote-cdp-actionability-probe-budget
Open

Give remote-CDP actionability probes the full remaining action budget#96
LawyZheng wants to merge 3 commits into
mainfrom
remote-cdp-actionability-probe-budget

Conversation

@LawyZheng

@LawyZheng LawyZheng commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Problem

On the connect_over_cdp() remote-attach path, click(), select_option(), and
fill() could time out prematurely — aborting after roughly 0.3–0.4 s even though the
caller'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_single and _wait_for_fill_ready loops did not
catch that error at all; the fill-apply loop caught only the generic Error and re-raised
the TimeoutError (since TimeoutError subclasses Error). Either way the probe timeout
propagated out and aborted the whole action long before the real deadline.

Fix

  • Budget each probe with the full remaining action time instead of a small fixed cap,
    via the _actionability_probe_timeout helper.
  • Treat a per-probe TimeoutError as transient — keep polling until the outer action
    deadline rather than aborting (matching Playwright's behavior).
  • Applied uniformly to the four sibling loops: _wait_for_single, _wait_for_fill_ready,
    the fill-apply loop, and the select-apply loop. The fill-apply loop now catches
    TimeoutError before the generic Error handler; the select-apply loop already
    caught it.
  • All four loops surface owner unavailability (page/context/browser closed or crashed)
    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_unavailable in their TimeoutError handlers.

Polling cadence is still driven by _sleep_until_next_poll, so widening the per-probe
bound does not busy-loop. For an infinite wait (timeout<=0) the probe keeps a bounded
1000 ms cadence so owner-availability and locator handlers are still re-checked
periodically.

GIL-release + owner-consistency hardening

  • locator_eval binding now releases the GIL explicitly via py.detach, matching its
    siblings click and locator_eval_handle. The GIL was already released for the
    blocking CDP round-trip one layer deeper — evaluate_locatorblock_on
    run_blocking_detachedrun_detached wraps the work in
    Python::try_attach(|py| py.detach(..)) — so locator_eval did not actually
    monopolize the GIL in practice (verified: a pure-Python busy thread advances ~9M
    iterations while a ~0.75 s native locator_eval blocks). The change closes the
    binding-layer inconsistency and guards against future refactors. No probe cap was added.
  • Guard test test_locator_eval_does_not_monopolize_gil_while_blocking locks in the
    GIL-release invariant.
  • Owner-unavailability refinement: _wait_for_single and _wait_for_fill_ready now
    also call _raise_if_owner_unavailable inside their except TimeoutError block
    (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 timeout
inflation 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 TimeoutError on the real
affected loop paths (rather than only asserting the enlarged per-probe budget):

  • test_fill_apply_loop_retries_transient_probe_timeout — the fill-apply _eval probe
    times out once, then the fill completes.
  • test_select_apply_loop_retries_transient_probe_timeout — the select-apply _eval
    probe times out once, then select_option() completes.
  • test_wait_for_single_retries_transient_actionability_probe_timeout — the actionability
    _target_state probe times out once, then click() completes.
  • test_fill_apply_loop_surfaces_outer_deadline_on_persistent_probe_timeout — a probe
    that 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 a
    probe timeout raises Page crashed immediately instead of polling to the deadline.
  • test_wait_for_single_propagates_owner_crash_when_probe_times_out_at_deadline — owner
    unavailability 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-shell build
(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 --lib are green. External high-latency remote-transport validation has completed successfully under
the driver-default operation timeout, covering the full fill/select/click interaction
sequence with clean teardown. The change is ready for review.

LawyZheng and others added 3 commits July 18, 2026 10:18
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>
time.sleep(min(interval, remaining))


def _actionability_probe_timeout(remaining_ms: float, timeout_disabled: bool = False) -> float:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of code should go in the rust library if possible. The python lib should just be a shim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants