fix(browser): tolerate ChromeDriver exit races during teardown

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())

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)

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)

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()