diff --git a/scripts/ci/run_mv3_compatibility.py b/scripts/ci/run_mv3_compatibility.py index ecda65b32..bbf157f53 100755 --- a/scripts/ci/run_mv3_compatibility.py +++ b/scripts/ci/run_mv3_compatibility.py @@ -640,6 +640,29 @@ def _wait_for_linux_process_teardown( time.sleep(min(0.05, remaining_seconds)) +def _terminate_chromedriver_process(driver: subprocess.Popen[str]) -> None: + """Terminate and reap ChromeDriver under one deadline despite normal exit races.""" + + deadline = time.monotonic() + PROCESS_EXIT_TIMEOUT_SECONDS + try: + driver.terminate() + except ProcessLookupError: + driver.wait(timeout=max(0.0, deadline - time.monotonic())) + return + + try: + driver.wait(timeout=max(0.0, deadline - time.monotonic())) + return + except subprocess.TimeoutExpired: + pass + + try: + driver.kill() + except ProcessLookupError: + pass + driver.wait(timeout=max(0.0, deadline - time.monotonic())) + + def _sample_linux_process_rss_bytes(process_id: int) -> int: """Read one attributed Linux process RSS through a bounded ``/proc`` status file.""" @@ -944,12 +967,7 @@ def _run_browser_pass( _webdriver_path(session_id, ""), {}, ) - driver.terminate() - try: - driver.wait(timeout=5) - except subprocess.TimeoutExpired: - driver.kill() - driver.wait(timeout=5) + _terminate_chromedriver_process(driver) def _run_restart_trial( @@ -1286,12 +1304,7 @@ def _run_agent_task_browser_pass( _webdriver_path(session_id, ""), {}, ) - driver.terminate() - try: - driver.wait(timeout=5) - except subprocess.TimeoutExpired: - driver.kill() - driver.wait(timeout=5) + _terminate_chromedriver_process(driver) if browser_process_id is None or browser_process_start_time_ticks is None: raise RuntimeError("Agent Task browser process identity was not captured") @@ -1648,12 +1661,7 @@ def _run_agent_task_forced_close_browser_pass( _webdriver_path(session_id, ""), {}, ) - driver.terminate() - try: - driver.wait(timeout=5) - except subprocess.TimeoutExpired: - driver.kill() - driver.wait(timeout=5) + _terminate_chromedriver_process(driver) if browser_process_id is None or browser_process_start_time_ticks is None: raise RuntimeError("Agent Task forced-close browser process identity was not captured") @@ -2049,4 +2057,4 @@ def main() -> int: if __name__ == "__main__": - raise SystemExit(main()) + raise SystemExit(main()) \ No newline at end of file diff --git a/tests/test_agent_task_forced_close_process_termination_contract.py b/tests/test_agent_task_forced_close_process_termination_contract.py index 2ea30608f..d060fa624 100644 --- a/tests/test_agent_task_forced_close_process_termination_contract.py +++ b/tests/test_agent_task_forced_close_process_termination_contract.py @@ -83,7 +83,7 @@ def test_forced_close_browser_failure_is_returned_after_teardown_waits(self) -> with self.subTest(expected=expected): self.assertIn(expected, browser_pass) - shutdown = browser_pass.index("driver.wait(timeout=5)") + shutdown = browser_pass.index("_terminate_chromedriver_process(driver)") teardown_wait = browser_pass.index("_wait_for_linux_process_teardown(") failure_return = browser_pass.index("if browser_failure_type is not None:") self.assertLess(shutdown, teardown_wait) diff --git a/tests/test_agent_task_shared_teardown_deadline.py b/tests/test_agent_task_shared_teardown_deadline.py index e7304e222..077d2ebfa 100644 --- a/tests/test_agent_task_shared_teardown_deadline.py +++ b/tests/test_agent_task_shared_teardown_deadline.py @@ -24,7 +24,7 @@ def test_browser_pass_uses_only_the_combined_teardown_waiter(self) -> None: self.assertNotIn("_wait_for_linux_process_identity_exit(", browser_pass) self.assertNotIn("_wait_for_linux_process_identity_set_exit(", browser_pass) - shutdown = browser_pass.index("driver.wait(timeout=5)") + shutdown = browser_pass.index("_terminate_chromedriver_process(driver)") teardown_wait = browser_pass.index("_wait_for_linux_process_teardown(") failure_return = browser_pass.index("if browser_failure_type is not None:") self.assertLess(shutdown, teardown_wait) diff --git a/tests/test_chromedriver_exit_race_cleanup.py b/tests/test_chromedriver_exit_race_cleanup.py new file mode 100644 index 000000000..1caedafa6 --- /dev/null +++ b/tests/test_chromedriver_exit_race_cleanup.py @@ -0,0 +1,150 @@ +"""Behavioral contracts for race-safe bounded ChromeDriver process cleanup.""" + +from __future__ import annotations + +import pathlib +import runpy +import subprocess +import unittest +from typing import Any + +ROOT = pathlib.Path(__file__).resolve().parents[1] +RUNNER = ROOT / "scripts" / "ci" / "run_mv3_compatibility.py" + + +def _runner_symbol(name: str) -> Any: + """Load one private runner symbol without invoking the compatibility entry point.""" + + return runpy.run_path(str(RUNNER))[name] + + +class _ExitBeforeTerminateDriver: + """Model a child that exits immediately before the TERM syscall boundary.""" + + def __init__(self) -> None: + self.wait_timeouts: list[float] = [] + self.kill_calls = 0 + + def terminate(self) -> None: + """Report ESRCH because the child has already exited.""" + + raise ProcessLookupError("ChromeDriver already exited") + + def wait(self, *, timeout: float) -> int: + """Reap the already exited child.""" + + self.wait_timeouts.append(timeout) + return 0 + + def kill(self) -> None: + """Record any unnecessary escalation.""" + + self.kill_calls += 1 + + +class _ExitBeforeKillDriver: + """Model a child that exits after TERM wait expiry but before KILL.""" + + def __init__(self) -> None: + self.wait_timeouts: list[float] = [] + self.kill_calls = 0 + + def terminate(self) -> None: + """Accept the initial TERM request.""" + + def wait(self, *, timeout: float) -> int: + """Timeout once, then reap the child after the KILL race.""" + + self.wait_timeouts.append(timeout) + if len(self.wait_timeouts) == 1: + raise subprocess.TimeoutExpired("chromedriver", timeout) + return 0 + + def kill(self) -> None: + """Report ESRCH because the child exited before escalation.""" + + self.kill_calls += 1 + raise ProcessLookupError("ChromeDriver exited before KILL") + + +class _PermissionDeniedDriver: + """Model an unexpected signaling failure that must not be normalized.""" + + def terminate(self) -> None: + """Reject the signal for a reason other than child exit.""" + + raise PermissionError("TERM denied") + + def wait(self, *, timeout: float) -> int: + """Fail if cleanup incorrectly continues after permission denial.""" + + raise AssertionError(f"unexpected wait({timeout})") + + def kill(self) -> None: + """Fail if cleanup incorrectly escalates after permission denial.""" + + raise AssertionError("unexpected KILL") + + +class ChromeDriverExitRaceCleanupTests(unittest.TestCase): + """Keep normal child-exit races harmless without suppressing real failures.""" + + def test_exit_before_terminate_is_reaped_without_kill(self) -> None: + """An ESRCH from TERM is normal exit evidence, not teardown failure.""" + + cleanup = _runner_symbol("_terminate_chromedriver_process") + driver = _ExitBeforeTerminateDriver() + + cleanup(driver) + + self.assertEqual(driver.kill_calls, 0) + self.assertEqual(len(driver.wait_timeouts), 1) + self.assertGreaterEqual(driver.wait_timeouts[0], 0.0) + self.assertLessEqual(driver.wait_timeouts[0], 5.0) + + def test_exit_before_kill_is_reaped_without_leaking_process_lookup(self) -> None: + """An ESRCH from KILL must still finish the bounded reap path.""" + + cleanup = _runner_symbol("_terminate_chromedriver_process") + driver = _ExitBeforeKillDriver() + + cleanup(driver) + + self.assertEqual(driver.kill_calls, 1) + self.assertEqual(len(driver.wait_timeouts), 2) + self.assertGreaterEqual(driver.wait_timeouts[1], 0.0) + self.assertLessEqual(driver.wait_timeouts[0], 5.0) + self.assertLessEqual(driver.wait_timeouts[1], driver.wait_timeouts[0]) + + def test_unexpected_signal_error_propagates(self) -> None: + """Permission failures must remain visible rather than being broadly suppressed.""" + + cleanup = _runner_symbol("_terminate_chromedriver_process") + + with self.assertRaisesRegex(PermissionError, "TERM denied"): + cleanup(_PermissionDeniedDriver()) + + def test_all_browser_lanes_use_the_race_safe_cleanup_boundary(self) -> None: + """Every pinned browser lane must share the same ChromeDriver cleanup contract.""" + + runner = RUNNER.read_text(encoding="utf-8") + boundaries = ( + ("def _run_browser_pass(", "\ndef _run_restart_trial("), + ("def _run_agent_task_browser_pass(", "\ndef _run_agent_task_trial("), + ( + "def _run_agent_task_forced_close_browser_pass(", + "\ndef _run_agent_task_forced_close_trial(", + ), + ) + for start_marker, end_marker in boundaries: + with self.subTest(start_marker=start_marker): + start = runner.index(start_marker) + end = runner.index(end_marker, start) + browser_lane = runner[start:end] + self.assertIn("_terminate_chromedriver_process(driver)", browser_lane) + self.assertNotIn("driver.terminate()", browser_lane) + self.assertNotIn("driver.kill()", browser_lane) + + +if __name__ == "__main__": + unittest.main()