diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 68c9423db673..94c8f8686aa6 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -251,7 +251,36 @@ def _run_one_file( bound a pathologically slow or hung file as a whole. """ cmd = [sys.executable, "-m", "pytest", str(file), *pytest_args] - + + # Isolate each pytest child from the runner's console / process group. + # + # POSIX: start_new_session=True → os.setsid() in the child, placing it + # at the head of its own process group so _kill_tree can SIGKILL the + # group atomically. + # + # Windows: start_new_session only maps to CREATE_NEW_PROCESS_GROUP in + # CPython 3.12+ — on 3.11 it is silently ignored, so every child + # shared the runner's console process group. One os.kill(pid, 0) + # liveness probe anywhere in the run — which on Windows routes through + # GenerateConsoleCtrlEvent (bpo-14484) — then broadcast + # KeyboardInterrupt to every concurrent child AND the runner itself. + # Pass the creationflags explicitly, on every Python version: + # CREATE_NEW_PROCESS_GROUP — child is its own ctrl-event group root, + # so console ctrl events can't fan out across children; + # CREATE_NO_WINDOW — child gets its own invisible console, fully + # insulating it from ctrl-event broadcasts on the runner's console + # (and suppressing per-child conhost window flashes). + # _kill_tree handles the Windows kill path via taskkill /F /T. + if sys.platform == "win32": + isolation_kwargs: Dict[str, object] = { + "creationflags": ( + subprocess.CREATE_NEW_PROCESS_GROUP + | subprocess.CREATE_NO_WINDOW + ), + } + else: + isolation_kwargs = {"start_new_session": True} + subproc_start = time.monotonic() # launch the pytest process proc = subprocess.Popen( @@ -262,11 +291,7 @@ def _run_one_file( text=True, # skipping writing bytecode because we're running a bunch of parallel python processes on the same code env={**os.environ, 'PYTHONDONTWRITEBYTECODE': '1'}, - # POSIX: place the child at the head of its own process group so - # _kill_tree can SIGKILL the group atomically. - # Windows: this maps to CREATE_NEW_PROCESS_GROUP in CPython 3.12+; - # _kill_tree handles the Windows path via taskkill /F /T. - start_new_session=True, + **isolation_kwargs, ) # Capture the pgid NOW, before the leader can exit and be reaped. Once diff --git a/tests/test_run_tests_parallel.py b/tests/test_run_tests_parallel.py index 3cba46fab00d..63b7fd81b911 100644 --- a/tests/test_run_tests_parallel.py +++ b/tests/test_run_tests_parallel.py @@ -277,3 +277,66 @@ def test_positional_path_not_treated_as_flag(tmp_path: Path) -> None: # Discovery found the probe file (2 tests), proving the positional path # was consumed as a root, not forwarded to pytest as a bad flag. assert "test_flagprobe.py" in proc.stdout, proc.stdout + + +def _load_runner_module(): + """Import scripts/run_tests_parallel.py as a module (it's not a package).""" + import importlib.util + + runner = Path(__file__).resolve().parent.parent / "scripts" / "run_tests_parallel.py" + spec = importlib.util.spec_from_file_location("_run_tests_parallel_under_test", runner) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def test_children_spawn_in_their_own_process_group(monkeypatch, tmp_path: Path) -> None: + """Every pytest child must be isolated from the runner's process group. + + POSIX: ``start_new_session=True`` (os.setsid) so ``_kill_tree`` can + SIGKILL the group atomically. + + Windows: ``start_new_session`` is silently IGNORED before CPython 3.12, + so the runner must pass ``CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW`` + creationflags explicitly. Without them every child shares the runner's + console process group, and a single ``os.kill(pid, 0)`` liveness probe + anywhere in the run — which on Windows routes through + GenerateConsoleCtrlEvent (bpo-14484) — broadcasts KeyboardInterrupt to + every concurrent child AND the runner itself (observed: the runner died + at 100% completion with ~200 collateral KeyboardInterrupt failures). + """ + mod = _load_runner_module() + captured: dict = {} + + class _FakeProc: + pid = 99999 + returncode = 0 + + def communicate(self, timeout=None): + return ("1 passed", None) + + def poll(self): + return 0 + + def kill(self): + pass + + def fake_popen(cmd, **kwargs): + captured.update(kwargs) + return _FakeProc() + + monkeypatch.setattr(mod.subprocess, "Popen", fake_popen) + # _kill_tree shells out to taskkill on Windows — neuter it so the fake + # pid can't hit a real process. + monkeypatch.setattr(mod, "_kill_tree", lambda proc, pgid=None: None) + + probe = tmp_path / "test_probe.py" + probe.write_text("def test_ok():\n assert True\n") + mod._run_one_file(probe, [], tmp_path, 30.0) + + if sys.platform == "win32": + flags = captured.get("creationflags", 0) + assert flags & subprocess.CREATE_NEW_PROCESS_GROUP, captured + assert flags & subprocess.CREATE_NO_WINDOW, captured + else: + assert captured.get("start_new_session") is True, captured diff --git a/tests/tools/test_zombie_process_cleanup.py b/tests/tools/test_zombie_process_cleanup.py index a8b745f541a9..0def6508989a 100644 --- a/tests/tools/test_zombie_process_cleanup.py +++ b/tests/tools/test_zombie_process_cleanup.py @@ -5,12 +5,11 @@ gateway deployments. """ -import os -import signal import subprocess import sys import threading +import psutil def _spawn_sleep(seconds: float = 60) -> subprocess.Popen: @@ -21,12 +20,15 @@ def _spawn_sleep(seconds: float = 60) -> subprocess.Popen: def _pid_alive(pid: int) -> bool: - """Return True if a process with the given PID is still running.""" - try: - os.kill(pid, 0) - return True - except (ProcessLookupError, PermissionError): - return False + """Return True if a process with the given PID is still running. + + Never probe with ``os.kill(pid, 0)`` here: on Windows that is NOT a + no-op — it routes through GenerateConsoleCtrlEvent (bpo-14484) and + broadcasts Ctrl+C across the shared console, killing every concurrent + pytest child AND the parallel-runner process. psutil probes via + OpenProcess/GetExitCodeProcess on Windows (no signals involved). + """ + return psutil.pid_exists(pid) class TestZombieReproduction: @@ -36,13 +38,14 @@ def test_orphaned_processes_survive_without_cleanup(self): """REPRODUCTION: processes spawned directly survive if no one kills them — this models the gap that causes zombie accumulation when the gateway drops agent references without calling close().""" - pids = [] + procs = [] try: for _ in range(3): proc = _spawn_sleep(60) - pids.append(proc.pid) + procs.append(proc) + pids = [p.pid for p in procs] for pid in pids: assert _pid_alive(pid), f"PID {pid} should be alive after spawn" @@ -56,10 +59,14 @@ def test_orphaned_processes_survive_without_cleanup(self): f"expected it to survive (demonstrating the bug)" ) finally: - for pid in pids: + # Popen.kill() (TerminateProcess on Windows, SIGKILL on POSIX) + # instead of os.kill(pid, signal.SIGKILL): signal.SIGKILL does + # not exist on Windows, so the old cleanup raised AttributeError. + for p in procs: try: - os.kill(pid, signal.SIGKILL) - except (ProcessLookupError, PermissionError): + p.kill() + p.wait(timeout=5) + except Exception: pass def test_explicit_terminate_reaps_processes(self):