diff --git a/scripts/ci/run_mv3_compatibility.py b/scripts/ci/run_mv3_compatibility.py index a0d60f39..59dab636 100644 --- a/scripts/ci/run_mv3_compatibility.py +++ b/scripts/ci/run_mv3_compatibility.py @@ -17,6 +17,7 @@ import os import pathlib import queue +import signal import string import subprocess import tempfile @@ -38,6 +39,8 @@ REQUEST_TIMEOUT_SECONDS = 5.0 STARTUP_TIMEOUT_SECONDS = 20.0 FIXTURE_TIMEOUT_SECONDS = 20.0 +PROCESS_GROUP_EXIT_TIMEOUT_SECONDS = 5.0 +PROCESS_GROUP_EXIT_POLL_SECONDS = 0.05 MAX_WEBDRIVER_RESPONSE_BYTES = 1_048_576 MAX_CHROMEDRIVER_STARTUP_LINE_BYTES = 512 CHROMEDRIVER_BOUND_PORT_PREFIX = "ChromeDriver was started successfully on port " @@ -396,8 +399,87 @@ def _exercise_real_click(driver_port: int, session_id: str) -> str: return str(text) +def _wait_for_process_group_exit(process_group_id: int) -> Exception | None: + """Wait a bounded interval until one isolated process group no longer exists.""" + + deadline = time.monotonic() + PROCESS_GROUP_EXIT_TIMEOUT_SECONDS + while True: + try: + os.killpg(process_group_id, 0) + except ProcessLookupError: + return None + except OSError as error: + return error + if time.monotonic() >= deadline: + return RuntimeError( + "ChromeDriver process group remained alive after bounded teardown" + ) + time.sleep(PROCESS_GROUP_EXIT_POLL_SECONDS) + + +def _kill_and_reap_process_group( + driver: subprocess.Popen[bytes], process_group_id: int +) -> Exception | None: + """Force one surviving isolated process group down and verify bounded disappearance.""" + + try: + os.killpg(process_group_id, signal.SIGKILL) + except ProcessLookupError: + pass + except OSError as kill_error: + return kill_error + try: + driver.wait(timeout=5) + except (OSError, subprocess.TimeoutExpired) as wait_error: + return wait_error + return _wait_for_process_group_exit(process_group_id) + + def _teardown_driver_process(driver: subprocess.Popen[bytes]) -> Exception | None: - """Best-effort reap ChromeDriver while preserving unrecovered process failures.""" + """Reap ChromeDriver and, for real Popen instances, its isolated process group. + + Production ChromeDriver launches expose a positive `pid` and run in a fresh + process session. Teardown signals the group with SIGTERM, reaps the leader, + verifies whether descendants still occupy the group, and applies bounded + SIGKILL recovery before reporting success. A pid-less test double retains the + older bounded leader-only path so cleanup-failure contracts can isolate + session semantics without sending operating-system signals. + """ + + driver_pid = getattr(driver, "pid", None) + if isinstance(driver_pid, int) and driver_pid > 0: + try: + os.killpg(driver_pid, signal.SIGTERM) + except ProcessLookupError: + try: + driver.wait(timeout=5) + except (OSError, subprocess.TimeoutExpired) as wait_error: + return wait_error + return None + except OSError as terminate_error: + fallback_error = _kill_and_reap_process_group(driver, driver_pid) + if fallback_error is not None: + terminate_error.add_note( + "bounded ChromeDriver process-group kill fallback also failed: " + f"{type(fallback_error).__name__}" + ) + return terminate_error + return None + + try: + driver.wait(timeout=5) + except subprocess.TimeoutExpired: + return _kill_and_reap_process_group(driver, driver_pid) + except OSError as wait_error: + return wait_error + + try: + os.killpg(driver_pid, 0) + except ProcessLookupError: + return None + except OSError as probe_error: + return probe_error + return _kill_and_reap_process_group(driver, driver_pid) try: driver.terminate() @@ -474,6 +556,7 @@ def _start_chromedriver( [str(chromedriver_bin), "--port=0", "--allowed-ips=127.0.0.1"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + start_new_session=True, ) if driver.stdout is None: teardown_error = _teardown_driver_process(driver) @@ -882,4 +965,4 @@ def main() -> int: if __name__ == "__main__": - raise SystemExit(main()) \ No newline at end of file + raise SystemExit(main()) diff --git a/tests/test_mv3_ephemeral_profile_contract.py b/tests/test_mv3_ephemeral_profile_contract.py new file mode 100644 index 00000000..84601375 --- /dev/null +++ b/tests/test_mv3_ephemeral_profile_contract.py @@ -0,0 +1,131 @@ +"""Regression contract for bounded ephemeral Chromium profile lifecycle.""" + +from __future__ import annotations + +import pathlib +import runpy +import signal +import tempfile +import unittest +import unittest.mock + +ROOT = pathlib.Path(__file__).resolve().parents[1] +RUNNER = ROOT / "scripts" / "ci" / "run_mv3_compatibility.py" + + +class ManifestV3EphemeralProfileContractTests(unittest.TestCase): + """Prove each Chromium trial creates, reuses, and deletes one isolated profile.""" + + def test_restart_trial_uses_empty_profile_then_deletes_it(self) -> None: + """A trial must start empty, reuse only its own profile, then remove it.""" + + namespace = runpy.run_path(str(RUNNER), run_name="mv3_ephemeral_profile_contract") + run_restart_trial = namespace["_run_restart_trial"] + globals_ = run_restart_trial.__globals__ + observed_profiles: list[pathlib.Path] = [] + call_count = 0 + + def fake_browser_pass( + _chrome_bin: pathlib.Path, + _chromedriver_bin: pathlib.Path, + _fixture_url: str, + profile_dir: str, + expected_storage_persistence: str, + ) -> dict[str, object]: + nonlocal call_count + profile_path = pathlib.Path(profile_dir) + if call_count == 0: + self.assertTrue(profile_path.is_dir()) + self.assertEqual(list(profile_path.iterdir()), []) + profile_path.joinpath("profile-created-by-browser").write_text( + "fixture", encoding="utf-8" + ) + else: + self.assertEqual(profile_path, observed_profiles[0]) + self.assertTrue( + profile_path.joinpath("profile-created-by-browser").is_file() + ) + observed_profiles.append(profile_path) + call_count += 1 + return { + "browser_version": namespace["PINNED_CHROME_VERSION"], + "worker_start_count": call_count, + "storage_persistence": expected_storage_persistence, + "surfaces": {"fixture": True}, + } + + with unittest.mock.patch.dict( + globals_, {"_run_browser_pass": fake_browser_pass} + ): + result = run_restart_trial( + pathlib.Path("/unused/chrome"), + pathlib.Path("/unused/chromedriver"), + "http://127.0.0.1/fixture", + 1, + ) + + self.assertEqual(len(observed_profiles), 2) + self.assertEqual(observed_profiles[0], observed_profiles[1]) + self.assertFalse(observed_profiles[0].exists()) + self.assertNotIn(str(observed_profiles[0]), repr(result)) + self.assertIs(result.get("passed"), True) + + def test_browser_pass_owns_and_terminates_the_chromedriver_process_group(self) -> None: + """Failure cleanup must signal the isolated driver group, not only its leader.""" + + source = RUNNER.read_text(encoding="utf-8") + self.assertIn("start_new_session=True", source) + + namespace = runpy.run_path(str(RUNNER), run_name="mv3_process_group_contract") + run_browser_pass = namespace["_run_browser_pass"] + globals_ = run_browser_pass.__globals__ + driver = unittest.mock.Mock() + driver.pid = 4242 + driver.wait.return_value = 0 + + def fake_kill_process_group(process_group_id: int, process_signal: int) -> None: + self.assertEqual(process_group_id, driver.pid) + if process_signal == signal.SIGTERM: + return + if process_signal == 0: + raise ProcessLookupError + raise AssertionError(f"unexpected process-group signal: {process_signal}") + + with tempfile.TemporaryDirectory(prefix="originweave-mv3-cleanup-") as profile_dir: + with ( + unittest.mock.patch.dict( + globals_, + { + "_start_chromedriver": lambda _binary: (driver, 43123), + "_wait_for_driver": unittest.mock.Mock( + side_effect=RuntimeError("controlled startup failure") + ), + }, + ), + unittest.mock.patch.object( + globals_["os"], "killpg", side_effect=fake_kill_process_group + ) as kill_process_group, + ): + with self.assertRaisesRegex(RuntimeError, "controlled startup failure"): + run_browser_pass( + pathlib.Path("/unused/chrome"), + pathlib.Path("/unused/chromedriver"), + "http://127.0.0.1/fixture", + profile_dir, + "initialized", + ) + + self.assertEqual( + kill_process_group.call_args_list, + [ + unittest.mock.call(driver.pid, signal.SIGTERM), + unittest.mock.call(driver.pid, 0), + ], + ) + driver.wait.assert_called_once_with(timeout=5) + driver.terminate.assert_not_called() + driver.kill.assert_not_called() + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mv3_process_group_cleanup_contract.py b/tests/test_mv3_process_group_cleanup_contract.py new file mode 100644 index 00000000..11060329 --- /dev/null +++ b/tests/test_mv3_process_group_cleanup_contract.py @@ -0,0 +1,181 @@ +"""Regression contracts for bounded process-group cleanup.""" + +from __future__ import annotations + +import os +import pathlib +import runpy +import signal +import subprocess +import sys +import tempfile +import time +import unittest +import unittest.mock + +ROOT = pathlib.Path(__file__).resolve().parents[1] +RUNNER = ROOT / "scripts" / "ci" / "run_mv3_compatibility.py" + + +class ManifestV3ProcessGroupCleanupContractTests(unittest.TestCase): + """Prove cleanup causality and descendant process-group termination.""" + + @staticmethod + def _surfaces() -> dict[str, str]: + """Return one fully passing controlled compatibility surface set.""" + + return { + "workerStartCount": "1", + "storagePersistence": "initialized", + "workerReply": "pong", + "content": "ready", + "storage": "ready", + "dnr": "blocked", + "tabs": "ready", + "windows": "ready", + "scripting": "ready", + "scriptingExecuted": "ready", + "commands": "ready", + "sidePanel": "ready", + "bookmarks": "ready", + "history": "ready", + "downloads": "ready", + } + + def test_session_cleanup_error_survives_process_group_signal_failures(self) -> None: + """Process-group teardown failure stays secondary to reviewed session cleanup.""" + + namespace = runpy.run_path(str(RUNNER), run_name="mv3_group_cleanup_contract") + run_browser_pass = namespace["_run_browser_pass"] + globals_ = run_browser_pass.__globals__ + driver = unittest.mock.Mock() + driver.pid = 4242 + session_error = RuntimeError("session delete failed") + + def fake_json_request( + _driver_port: int, + method: str, + path: str, + _payload=None, + *, + timeout: float = 5.0, + ): + if timeout <= 0: + raise AssertionError("timeout must remain positive") + if method == "POST" and path == "/session": + return { + "value": { + "sessionId": "session-1", + "capabilities": { + "browserVersion": namespace["PINNED_CHROME_VERSION"] + }, + } + } + if method == "POST" and path.endswith("/url"): + return {"value": None} + if method == "DELETE" and path.endswith("/session/session-1"): + raise session_error + raise AssertionError(f"unexpected WebDriver request: {method} {path}") + + with tempfile.TemporaryDirectory(prefix="originweave-group-cleanup-") as profile_dir: + with ( + unittest.mock.patch.object( + globals_["os"], + "killpg", + side_effect=PermissionError("process-group signal denied"), + ) as kill_process_group, + unittest.mock.patch.dict( + globals_, + { + "_start_chromedriver": lambda _binary: (driver, 43123), + "_wait_for_driver": lambda _port: None, + "_json_request": fake_json_request, + "_wait_for_extension_evidence": ( + lambda _port, _session, _expected: self._surfaces() + ), + "_exercise_real_click": lambda _port, _session: "clicked", + }, + ), + ): + with self.assertRaises(namespace["WebDriverSessionCleanupError"]) as raised: + run_browser_pass( + pathlib.Path("/controlled/chrome"), + pathlib.Path("/controlled/chromedriver"), + "http://127.0.0.1:8080/page.html", + profile_dir, + "initialized", + ) + + self.assertIs(raised.exception.__cause__, session_error) + self.assertEqual( + kill_process_group.call_args_list, + [ + unittest.mock.call(driver.pid, signal.SIGTERM), + unittest.mock.call(driver.pid, signal.SIGKILL), + ], + ) + self.assertIn( + "ChromeDriver process teardown also failed: PermissionError", + getattr(raised.exception, "__notes__", []), + ) + + @unittest.skipUnless(os.name == "posix" and hasattr(os, "killpg"), "requires POSIX process groups") + def test_teardown_reaps_descendant_that_ignores_sigterm_after_leader_exits(self) -> None: + """A fast-exiting leader cannot make a SIGTERM-resistant descendant look reaped.""" + + namespace = runpy.run_path(str(RUNNER), run_name="mv3_real_group_cleanup_contract") + teardown_driver_process = namespace["_teardown_driver_process"] + child_program = ( + "import signal,time\n" + "signal.signal(signal.SIGTERM, signal.SIG_IGN)\n" + "print('ready', flush=True)\n" + "time.sleep(60)\n" + ) + leader_program = ( + "import subprocess,sys,time\n" + f"child = subprocess.Popen([sys.executable, '-c', {child_program!r}], " + "stdout=subprocess.PIPE, text=True)\n" + "assert child.stdout is not None\n" + "assert child.stdout.readline().strip() == 'ready'\n" + "print(child.pid, flush=True)\n" + "time.sleep(60)\n" + ) + driver = subprocess.Popen( + [sys.executable, "-c", leader_program], + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + text=True, + start_new_session=True, + ) + self.assertIsNotNone(driver.stdout) + assert driver.stdout is not None + child_pid = int(driver.stdout.readline().strip()) + driver.stdout.close() + self.assertGreater(child_pid, 0) + process_group_id = driver.pid + + try: + self.assertIsNone(teardown_driver_process(driver)) + deadline = time.monotonic() + 1.0 + while time.monotonic() < deadline: + try: + os.killpg(process_group_id, 0) + except ProcessLookupError: + break + time.sleep(0.05) + else: + self.fail("process-group teardown left a SIGTERM-resistant descendant alive") + finally: + try: + os.killpg(process_group_id, signal.SIGKILL) + except ProcessLookupError: + pass + try: + driver.wait(timeout=5) + except subprocess.TimeoutExpired: + driver.kill() + driver.wait(timeout=5) + + +if __name__ == "__main__": + unittest.main()