Give remote-CDP actionability probes the full remaining action budget

python/rustwright/sync_api.py

@@ -4607,6 +4607,19 @@ def _sleep_until_next_poll(deadline: float, interval: float = 0.02) -> None:
         time.sleep(min(interval, remaining))
 
 
+def _actionability_probe_timeout(remaining_ms: float, timeout_disabled: bool = False) -> float:
+    # Budget a single actionability/state probe with the full remaining action time rather
+    # than a small fixed cap. A cap shorter than one remote-CDP round trip made every probe
+    # time out and abort the action while the outer deadline was still far away (observed on
+    # click/select_option over connect_over_cdp attach). Polling cadence is driven by
+    # _sleep_until_next_poll, not this bound, so widening it does not busy-loop. For an
+    # infinite wait (timeout<=0) keep a bounded cadence so owner-availability and locator
+    # handlers are still re-checked periodically.
+    if timeout_disabled:
+        return min(remaining_ms, 1_000.0)
+    return max(remaining_ms, 1.0)
+
+
 def _handle_event_wait_timeout(owner: Any, event: str, timeout_display: str, deadline: Optional[float]) -> None:
     if owner is not None:
         _raise_if_owner_unavailable(owner, use_close_reason=True)
@@ -21785,16 +21798,31 @@ def _wait_for_single(
         while True:
             _raise_if_owner_unavailable(self._page, method=method)
             remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0)
-            command_timeout = min(1_000.0 if timeout_disabled else max(timeout_ms, 1.0), remaining_ms, 1_000.0)
+            command_timeout = _actionability_probe_timeout(remaining_ms, timeout_disabled)
             self._page._run_locator_handlers(deadline)
-            last_info = self._target_state(
-                command_timeout,
-                scroll=scroll or state in {"actionable", "scrollable"},
-                stable=requires_stable,
-                receives_events=state == "actionable",
-                stable_position_required=stable_position_required,
-                action_position=action_position,
-            )
+            try:
+                last_info = self._target_state(
+                    command_timeout,
+                    scroll=scroll or state in {"actionable", "scrollable"},
+                    stable=requires_stable,
+                    receives_events=state == "actionable",
+                    stable_position_required=stable_position_required,
+                    action_position=action_position,
+                )
+            except TimeoutError:
+                # A single actionability probe that exceeds its own budget is transient,
+                # not fatal: over a high-latency remote-CDP transport one probe's round
+                # trips can outlast a small fixed cap. Surface owner unavailability
+                # immediately, otherwise keep polling until the outer deadline instead of
+                # aborting the whole action (matches Playwright and the fill/select-apply
+                # loops). Doing this before the deadline check means a page/context/browser
+                # that closed mid-probe raises the precise owner error rather than a
+                # generic timeout when the deadline coincides.
+                _raise_if_owner_unavailable(self._page, method=method)
+                if time.monotonic() >= deadline:
+                    break
+                _sleep_until_next_poll(deadline)
+                continue
             if self._strict and not self._explicit_index:
                 self._raise_frame_strict_violation(last_info.get("frame_strict_violation"))
             count = int(last_info.get("count") or 0)
@@ -21857,9 +21885,22 @@ def _wait_for_fill_ready(self, action: str, *, timeout: Optional[float] = None)
         while True:
             _raise_if_owner_unavailable(self._page, method=method)
             remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0)
-            command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0)
+            command_timeout = _actionability_probe_timeout(remaining_ms)
             self._page._run_locator_handlers(deadline)
-            last_info = self._target_state(command_timeout)
+            try:
+                last_info = self._target_state(command_timeout)
+            except TimeoutError:
+                # A single fill-readiness probe timing out is transient over a slow
+                # remote-CDP transport: surface owner unavailability immediately, otherwise
+                # keep polling until the outer deadline (matches the fill/select-apply
+                # loops). Checking the owner before the deadline break means a closed
+                # page/context/browser raises the precise owner error rather than a generic
+                # timeout when the deadline coincides.
+                _raise_if_owner_unavailable(self._page, method=method)
+                if time.monotonic() >= deadline:
+                    break
+                _sleep_until_next_poll(deadline)
+                continue
             if self._strict and not self._explicit_index:
                 self._raise_frame_strict_violation(last_info.get("frame_strict_violation"))
             count = int(last_info.get("count") or 0)
@@ -23614,10 +23655,22 @@ def _fill(self, value: str, *, action: str, timeout: Optional[float] = None, for
         last_info: dict[str, Any] = {}
         while True:
             remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0)
-            command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0)
+            command_timeout = _actionability_probe_timeout(remaining_ms)
             self._page._run_locator_handlers(deadline)
             try:
                 result = self._eval(fill_script, command_timeout)
+            except TimeoutError:
+                # A single fill-apply probe timing out is transient over a slow remote-CDP
+                # transport: one probe's CDP round trips can outlast the per-probe budget.
+                # Surface owner unavailability immediately, otherwise keep polling until the
+                # outer deadline instead of aborting the whole action (matches Playwright and
+                # the sibling wait/select loops). TimeoutError subclasses Error, so this must
+                # precede the generic Error handler below.
+                _raise_if_owner_unavailable(self._page, method=_locator_method_for_action(action))
+                if time.monotonic() >= deadline:
+                    break
+                _sleep_until_next_poll(deadline)
+                continue
             except Error as exc:
                 if "No element matches locator" not in str(exc):
                     raise
@@ -24397,10 +24450,13 @@ def _select_option_impl(
         deadline = started + max(timeout_ms, 0.0) / 1000
         while True:
             remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0)
-            command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0)
+            command_timeout = _actionability_probe_timeout(remaining_ms)
             try:
                 result = self._eval(select_script, command_timeout, method=method)
             except TimeoutError:
+                # A transient select-apply probe timeout is retried until the outer deadline
+                # (see the fill-apply loop), but owner unavailability is surfaced immediately.
+                _raise_if_owner_unavailable(self._page, method=method)
                 if time.monotonic() >= deadline:
                     break
                 _sleep_until_next_poll(deadline)

src/lib.rs

@@ -9046,12 +9046,18 @@ return true;
     #[pyo3(signature = (locator_json, index, body, timeout_ms=None))]
     fn locator_eval(
         &self,
+        py: Python<'_>,
         locator_json: &str,
         index: usize,
         body: &str,
         timeout_ms: Option<f64>,
     ) -> PyResult<String> {
-        self.evaluate_locator(locator_json, index, body, timeout_ms)
+        // Release the GIL for the duration of the blocking CDP round-trip, matching
+        // `click` and `locator_eval_handle`. `evaluate_locator` already detaches inside
+        // `block_on`, but keeping the detach explicit at the binding layer keeps the
+        // three locator bindings consistent and guards against a future refactor that
+        // adds GIL-holding work before `block_on` or changes the transport.
+        py.detach(|| self.evaluate_locator(locator_json, index, body, timeout_ms))
             .map_err(py_err)
     }
 

tests/test_rustwright_sync_api.py

@@ -10177,6 +10177,167 @@ def test_locator_click_waits_for_delayed_element(page):
     assert page.evaluate("document.body.dataset.clicked") == "yes"
 
 
+def test_wait_for_single_retries_transient_actionability_probe_timeout(page, monkeypatch):
+    # A single actionability probe can exceed its own budget over a high-latency remote-CDP
+    # transport (connect_over_cdp attach): its several CDP round trips outlast the per-probe
+    # budget even though the outer action deadline is still far off. The wait loop must treat
+    # that probe TimeoutError as transient and keep polling, not abort the whole action.
+    from rustwright.sync_api import Locator
+
+    page.set_content("<button id='go' onclick=\"document.body.dataset.clicked='yes'\">go</button>")
+
+    real_target_state = Locator._target_state
+    probe = {"calls": 0, "timed_out": 0}
+
+    def flaky_target_state(self, timeout=None, **kwargs):
+        probe["calls"] += 1
+        # Time out only the first probe regardless of how large its budget is, so the retry
+        # path itself is exercised rather than merely the enlarged per-probe budget.
+        if probe["timed_out"] == 0:
+            probe["timed_out"] += 1
+            raise TimeoutError(f"timed out after {int(timeout)} ms")
+        return real_target_state(self, timeout, **kwargs)
+
+    monkeypatch.setattr(Locator, "_target_state", flaky_target_state)
+
+    page.click("#go", timeout=30_000)
+
+    assert page.evaluate("document.body.dataset.clicked") == "yes"
+    assert probe["timed_out"] == 1
+    assert probe["calls"] >= 2  # retried after the transient probe timeout
+
+
+def test_fill_apply_loop_retries_transient_probe_timeout(page, monkeypatch):
+    # The fill-apply loop performs the actual fill via _eval. A first probe whose CDP round
+    # trips outlast the per-probe budget raises TimeoutError; the loop must retry until the
+    # outer deadline instead of re-raising that transient timeout as a fatal error.
+    from rustwright.sync_api import Locator
+
+    page.set_content("<input id='name'>")
+
+    real_eval = Locator._eval
+    probe = {"calls": 0, "timed_out": 0}
+
+    def flaky_eval(self, body, timeout=None, *, method=None):
+        if "nonFillableInputTypes" in body:
+            probe["calls"] += 1
+            if probe["timed_out"] == 0:
+                probe["timed_out"] += 1
+                raise TimeoutError(f"timed out after {int(timeout)} ms")
+        return real_eval(self, body, timeout, method=method)
+
+    monkeypatch.setattr(Locator, "_eval", flaky_eval)
+
+    page.fill("#name", "Ada", timeout=30_000)
+
+    assert page.locator("#name").input_value() == "Ada"
+    assert probe["timed_out"] == 1
+    assert probe["calls"] >= 2  # retried after the transient probe timeout
+
+
+def test_select_apply_loop_retries_transient_probe_timeout(page, monkeypatch):
+    # The select-apply loop performs the final selection via _eval. A first transient probe
+    # timeout must be retried until the outer deadline rather than aborting select_option.
+    from rustwright.sync_api import Locator
+
+    page.set_content(
+        "<select id='sel'><option value='a'>A</option><option value='b'>B</option></select>"
+    )
+
+    real_eval = Locator._eval
+    probe = {"calls": 0, "timed_out": 0}
+
+    def flaky_eval(self, body, timeout=None, *, method=None):
+        if "foundValues" in body:
+            probe["calls"] += 1
+            if probe["timed_out"] == 0:
+                probe["timed_out"] += 1
+                raise TimeoutError(f"timed out after {int(timeout)} ms")
+        return real_eval(self, body, timeout, method=method)
+
+    monkeypatch.setattr(Locator, "_eval", flaky_eval)
+
+    assert page.select_option("#sel", "b", timeout=30_000) == ["b"]
+    assert probe["timed_out"] == 1
+    assert probe["calls"] >= 2  # retried after the transient probe timeout
+
+
+def test_fill_apply_loop_surfaces_outer_deadline_on_persistent_probe_timeout(page, monkeypatch):
+    # When every fill-apply probe keeps timing out, the loop must still end at the outer
+    # deadline with its own editable-timeout message (not the raw per-probe timeout) and
+    # without busy-looping past the deadline.
+    from rustwright.sync_api import Locator
+
+    page.set_content("<input id='name'>")
+
+    real_eval = Locator._eval
+
+    def always_timeout_eval(self, body, timeout=None, *, method=None):
+        if "nonFillableInputTypes" in body:
+            raise TimeoutError(f"timed out after {int(timeout)} ms")
+        return real_eval(self, body, timeout, method=method)
+
+    monkeypatch.setattr(Locator, "_eval", always_timeout_eval)
+
+    started = time.monotonic()
+    with pytest.raises(TimeoutError, match="editable"):
+        page.fill("#name", "Ada", timeout=300)
+    elapsed = time.monotonic() - started
+
+    assert elapsed < 5  # honored the ~300ms outer deadline; no runaway busy-loop
+
+
+def test_fill_apply_loop_propagates_owner_crash_on_probe_timeout(page, monkeypatch):
+    # A probe timeout must not mask owner unavailability: if the page crashes while a fill
+    # probe is timing out, the loop surfaces the crash immediately instead of polling until
+    # the outer deadline.
+    from rustwright.sync_api import Locator
+
+    page.set_content("<input id='name'>")
+
+    real_eval = Locator._eval
+
+    def crashing_probe_eval(self, body, timeout=None, *, method=None):
+        if "nonFillableInputTypes" in body:
+            self._page._crashed = True
+            raise TimeoutError(f"timed out after {int(timeout)} ms")
+        return real_eval(self, body, timeout, method=method)
+
+    monkeypatch.setattr(Locator, "_eval", crashing_probe_eval)
+
+    try:
+        with pytest.raises(Error, match="Page crashed"):
+            page.fill("#name", "Ada", timeout=30_000)
+    finally:
+        page._crashed = False
+
+
+def test_wait_for_single_propagates_owner_crash_when_probe_times_out_at_deadline(page, monkeypatch):
+    # Owner unavailability must win over a probe timeout even when the timeout coincides
+    # with the outer deadline. The top-of-loop owner check already covers the retry path,
+    # but a probe that both crashes the owner and outlasts the deadline would otherwise
+    # take the deadline-break path and raise a generic actionability timeout. The except
+    # block now surfaces the crash first, matching the fill/select-apply loops.
+    from rustwright.sync_api import Locator
+
+    page.set_content("<button id='go'>go</button>")
+
+    def crashing_probe(self, timeout=None, **kwargs):
+        # Crash the owner and sleep past the tiny outer deadline before timing out, so the
+        # loop reaches the deadline break rather than re-polling on the next iteration.
+        self._page._crashed = True
+        time.sleep(0.05)
+        raise TimeoutError(f"timed out after {int(timeout)} ms")
+
+    monkeypatch.setattr(Locator, "_target_state", crashing_probe)
+
+    try:
+        with pytest.raises(Error, match="Page crashed"):
+            page.click("#go", timeout=10)
+    finally:
+        page._crashed = False
+
+
 def test_locator_fill_waits_for_delayed_editable(page):
     page.set_content(
         """
@@ -23962,6 +24123,62 @@ async def run() -> None:
     asyncio.run(run())
 
 
+def test_locator_eval_does_not_monopolize_gil_while_blocking(page):
+    # Invariant guard for the PR #95 GIL finding: while native ``locator_eval`` blocks
+    # on a slow in-page evaluate, it must not hold the Python GIL, so other Python
+    # threads keep running. This holds because ``locator_eval`` runs the CDP round-trip
+    # under ``py.detach`` (matching ``click`` / ``locator_eval_handle``); ``block_on``
+    # also detaches internally, so the property predates and survives the binding-layer
+    # change -- this test locks it in against future refactors that could hold the GIL.
+    #
+    # The probe is a pure-Python busy-increment thread (no I/O -- ``time.sleep`` would
+    # release the GIL regardless). If ``locator_eval`` ever held the GIL for the whole
+    # blocking window, the busy thread would be starved to ~0 iterations, because native
+    # Rust code never reaches a bytecode boundary where CPython could rotate threads.
+    # Absolute iteration counts are very noisy (10-20x run-to-run), so the assertion
+    # only checks "not frozen" with a wide margin rather than a tight rate.
+    page.set_content("<div id='target'>hi</div>")
+
+    stop = threading.Event()
+    progress = {"count": 0}
+
+    def spin() -> None:
+        while not stop.is_set():
+            progress["count"] += 1
+
+    worker = threading.Thread(target=spin, daemon=True)
+    worker.start()
+    try:
+        # Let the busy thread reach steady state before measuring.
+        time.sleep(0.05)
+        before = progress["count"]
+        started = time.monotonic()
+        # ``_eval`` routes straight through the native ``locator_eval`` binding. The JS
+        # body busy-waits ~750ms so the native call blocks for a measurable window.
+        page.locator("#target")._eval(
+            "const end = Date.now() + 750; while (Date.now() < end) {} return true;"
+        )
+        elapsed = time.monotonic() - started
+        advanced = progress["count"] - before
+    finally:
+        stop.set()
+        worker.join(timeout=2)
+
+    # Confirm the in-page busy loop actually blocked the native call (~0.75s), so the
+    # measurement window is real rather than a fast return.
+    assert elapsed >= 0.4, (
+        f"native locator_eval returned too fast to prove blocking: {elapsed:.3f}s"
+    )
+    # A frozen GIL would starve the busy thread to ~0 iterations. A released GIL lets it
+    # accumulate hundreds of thousands to millions; 50k is far above 0 yet far below the
+    # smallest observed released count, so it discriminates "frozen" from "released"
+    # without depending on the machine's exact throughput.
+    assert advanced > 50_000, (
+        f"busy thread only advanced {advanced} iterations during a {elapsed:.3f}s "
+        "native locator_eval; the GIL appears to be held for the blocking call"
+    )
+
+
 def test_connect_over_cdp_cancelled_creations_cleanup_after_last_browser_owner_drops():
     async def run() -> None:
         from playwright.async_api import async_playwright