diff --git a/tests/tools/test_process_registry.py b/tests/tools/test_process_registry.py index a5fa3fed7dc8..8d235f0cd67f 100644 --- a/tests/tools/test_process_registry.py +++ b/tests/tools/test_process_registry.py @@ -787,6 +787,93 @@ def terminate(self): finally: registry._running.pop(s.id, None) + def test_windows_pty_kill_verifies_tree_before_pty_teardown_and_is_idempotent( + self, registry, monkeypatch + ): + """A successful Windows PTY kill verifies the owned tree first.""" + from tools import process_registry as pr + + events = [] + + class FakePty: + def terminate(self, force=False): + events.append(("pty_terminate", force)) + + s = _make_session(sid="proc_windows_pty", output="partial output") + s.pid = 4242 + s.host_start_time = 123456 + s._pty = FakePty() + s.notify_on_complete = True + registry._running[s.id] = s + + def verified_tree_kill(pid, expected_start): + events.append(("tree_kill", pid, expected_start)) + return True + + monkeypatch.setattr(pr, "_IS_WINDOWS", True) + monkeypatch.setattr(registry, "_terminate_host_pid", verified_tree_kill) + monkeypatch.setattr(registry, "_write_checkpoint", lambda: None) + + result = registry.kill_process(s.id) + again = registry.kill_process(s.id) + + assert result["status"] == "killed" + assert again["status"] == "already_exited" + assert events == [ + ("tree_kill", 4242, 123456), + ("pty_terminate", True), + ] + completion = registry.completion_queue.get_nowait() + assert completion["session_id"] == s.id + assert completion["completion_reason"] == "killed" + assert registry.completion_queue.empty() + + @pytest.mark.parametrize( + "tree_kill_outcome", + [ + False, + OSError("taskkill failed"), + OSError("taskkill timed out"), + OSError("owned process survived"), + ], + ids=["pid-mismatch", "taskkill-failure", "taskkill-timeout", "survivor"], + ) + def test_windows_pty_kill_never_claims_success_when_tree_cleanup_is_unverified( + self, registry, monkeypatch, tree_kill_outcome + ): + """Every unverified Windows tree-cleanup outcome fails closed.""" + from tools import process_registry as pr + + pty_terminate_calls = [] + + class FakePty: + def terminate(self, force=False): + pty_terminate_calls.append(force) + + s = _make_session(sid="proc_windows_pty_failure") + s.pid = 4343 + s.host_start_time = 654321 + s._pty = FakePty() + s.notify_on_complete = True + registry._running[s.id] = s + + def failed_tree_kill(_pid, _expected_start): + if isinstance(tree_kill_outcome, Exception): + raise tree_kill_outcome + return tree_kill_outcome + + monkeypatch.setattr(pr, "_IS_WINDOWS", True) + monkeypatch.setattr(registry, "_terminate_host_pid", failed_tree_kill) + + result = registry.kill_process(s.id) + + assert result["status"] == "error" + assert result["status"] != "killed" + assert pty_terminate_calls == [] + assert s.exited is False + assert s.id in registry._running + assert registry.completion_queue.empty() + # ========================================================================= # Tool handler @@ -967,6 +1054,162 @@ def fake_run(args, **kwargs): assert "/T" in captured["args"], "Tree flag required to reach descendants" assert "/F" in captured["args"], "Force flag required for headless Chromium" + def test_windows_taskkill_nonzero_is_an_explicit_failure(self, monkeypatch): + from tools import process_registry as pr + + monkeypatch.setattr(pr, "_IS_WINDOWS", True) + monkeypatch.setattr( + pr.ProcessRegistry, + "_host_pid_is_ours", + classmethod(lambda cls, pid, expected_start: True), + ) + monkeypatch.setattr( + pr.ProcessRegistry, + "_snapshot_windows_tree_identities", + classmethod(lambda cls, pid, expected_start: [(pid, expected_start)]), + ) + monkeypatch.setattr( + pr.subprocess, + "run", + lambda *args, **kwargs: MagicMock( + returncode=1, stdout="", stderr="ERROR: Access is denied." + ), + ) + + with pytest.raises(OSError, match="Access is denied"): + pr.ProcessRegistry._terminate_host_pid(12345, 67890) + + def test_windows_taskkill_timeout_is_an_explicit_failure(self, monkeypatch): + from tools import process_registry as pr + + monkeypatch.setattr(pr, "_IS_WINDOWS", True) + monkeypatch.setattr( + pr.ProcessRegistry, + "_host_pid_is_ours", + classmethod(lambda cls, pid, expected_start: True), + ) + monkeypatch.setattr( + pr.ProcessRegistry, + "_snapshot_windows_tree_identities", + classmethod(lambda cls, pid, expected_start: [(pid, expected_start)]), + ) + + def timeout(*args, **kwargs): + raise subprocess.TimeoutExpired(args[0], timeout=10) + + monkeypatch.setattr(pr.subprocess, "run", timeout) + + with pytest.raises(OSError, match="timed out"): + pr.ProcessRegistry._terminate_host_pid(12345, 67890) + + def test_windows_surviving_owned_pid_is_an_explicit_failure(self, monkeypatch): + from tools import process_registry as pr + + monkeypatch.setattr(pr, "_IS_WINDOWS", True) + monkeypatch.setattr( + pr.ProcessRegistry, + "_host_pid_is_ours", + classmethod(lambda cls, pid, expected_start: True), + ) + monkeypatch.setattr( + pr.ProcessRegistry, + "_snapshot_windows_tree_identities", + classmethod(lambda cls, pid, expected_start: [(pid, expected_start), (22222, 33333)]), + ) + monkeypatch.setattr( + pr.ProcessRegistry, + "_wait_for_host_identities_exit", + classmethod(lambda cls, identities, timeout: [(22222, 33333)]), + ) + monkeypatch.setattr( + pr.subprocess, + "run", + lambda *args, **kwargs: MagicMock(returncode=0, stdout="", stderr=""), + ) + + with pytest.raises(OSError, match="22222"): + pr.ProcessRegistry._terminate_host_pid(12345, 67890) + + +@pytest.mark.skipif(sys.platform != "win32", reason="native Windows PTY integration") +def test_windows_pty_kill_removes_owned_tree_and_preserves_unrelated_sentinel( + registry, tmp_path +): + """Provider-free E2E: real PTY wrapper, child, and grandchild all exit.""" + tree_script = tmp_path / "pty_tree.py" + receipt_path = tmp_path / "pty_tree.json" + tree_script.write_text( + "\n".join( + [ + "import json, os, subprocess, sys, time", + "from pathlib import Path", + "grandchild = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(120)'])", + "Path(sys.argv[1]).write_text(json.dumps({'child': os.getpid(), 'grandchild': grandchild.pid}), encoding='utf-8')", + "while True: time.sleep(0.2)", + ] + ), + encoding="utf-8", + ) + sentinel = _spawn_python_sleep(120) + sentinel_start = ProcessRegistry._safe_host_start_time(sentinel.pid) + session = None + owned = [] + + def cleanup_owned_processes(): + for pid, start_time in reversed(owned): + if not ProcessRegistry._host_pid_is_ours(pid, start_time): + continue + subprocess.run( + ["taskkill", "/PID", str(pid), "/T", "/F"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=10, + ) + + try: + command = f'"{sys.executable}" "{tree_script.as_posix()}" "{receipt_path.as_posix()}"' + session = registry.spawn_local(command, cwd=str(tmp_path), use_pty=True) + assert session._pty is not None, "test requires the real native Windows PTY path" + assert _wait_until(receipt_path.exists, timeout=20.0), "PTY child tree did not become ready" + + receipt = json.loads(receipt_path.read_text(encoding="utf-8")) + wrapper_pid = session.pid + child_pid = int(receipt["child"]) + grandchild_pid = int(receipt["grandchild"]) + named_application_pids = {wrapper_pid, child_pid, grandchild_pid} + assert len({*named_application_pids, sentinel.pid, os.getpid()}) == 5 + + owned = ProcessRegistry._snapshot_windows_tree_identities( + wrapper_pid, session.host_start_time + ) + snapshot_pids = {pid for pid, _ in owned} + assert named_application_pids <= snapshot_pids + assert sentinel.pid not in snapshot_pids + assert len(snapshot_pids) == len(owned) + assert all(start is not None for _, start in owned) + assert all(ProcessRegistry._host_pid_is_ours(pid, start) for pid, start in owned) + assert ProcessRegistry._host_pid_is_ours(sentinel.pid, sentinel_start) + + result = registry.kill_process(session.id) + + assert result["status"] == "killed", result + assert _wait_until( + lambda: all( + not ProcessRegistry._host_pid_is_ours(pid, start) for pid, start in owned + ), + timeout=10.0, + ), f"owned PTY tree survived: {owned}" + assert ProcessRegistry._host_pid_is_ours(sentinel.pid, sentinel_start) + assert registry.kill_process(session.id)["status"] == "already_exited" + finally: + cleanup_owned_processes() + if sentinel.poll() is None: + sentinel.kill() + sentinel.wait(timeout=10) + assert not ProcessRegistry._host_pid_is_ours(sentinel.pid, sentinel_start) + class TestTerminateHostPidPosix: """POSIX branch walks the tree via psutil and SIGTERMs children first.""" diff --git a/tools/process_registry.py b/tools/process_registry.py index 7daf74b2e336..b62b623fb2ff 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -74,6 +74,7 @@ WATCH_GLOBAL_MAX_PER_WINDOW = 15 WATCH_GLOBAL_WINDOW_SECONDS = 10 WATCH_GLOBAL_COOLDOWN_SECONDS = 30 +WINDOWS_TREE_EXIT_TIMEOUT_SECONDS = 5.0 def format_uptime_short(seconds: int) -> str: @@ -544,7 +545,68 @@ def _daemon_term_grace_seconds() -> float: return 2.0 @classmethod - def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> None: + def _snapshot_windows_tree_identities( + cls, pid: int, expected_start: Optional[int] + ) -> List[tuple[int, Optional[int]]]: + """Snapshot a live Windows tree before its parent can disappear. + + ``taskkill /T`` discovers descendants from parent links, so the + identities needed to verify cleanup must be captured first. Pairing + each PID with its start time prevents PID reuse from looking like an + owned survivor. + """ + identities: List[tuple[int, Optional[int]]] = [ + ( + pid, + expected_start + if expected_start is not None + else cls._safe_host_start_time(pid), + ) + ] + + import psutil + try: + descendants = psutil.Process(pid).children(recursive=True) + except psutil.NoSuchProcess: + return identities + except (psutil.AccessDenied, OSError) as exc: + raise OSError( + f"Could not snapshot owned Windows process tree for PID {pid}: {exc}" + ) from exc + + seen = {pid} + for proc in descendants: + child_pid = int(proc.pid) + if child_pid in seen: + continue + seen.add(child_pid) + identities.append((child_pid, cls._safe_host_start_time(child_pid))) + return identities + + @classmethod + def _wait_for_host_identities_exit( + cls, + identities: List[tuple[int, Optional[int]]], + timeout: float, + ) -> List[tuple[int, Optional[int]]]: + """Return owned identities still alive after a bounded deadline.""" + deadline = time.monotonic() + max(timeout, 0.0) + survivors = [ + identity + for identity in identities + if cls._host_pid_is_ours(*identity) + ] + while survivors and time.monotonic() < deadline: + time.sleep(0.05) + survivors = [ + identity + for identity in survivors + if cls._host_pid_is_ours(*identity) + ] + return survivors + + @classmethod + def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> bool: """Terminate a host-visible PID and its descendants. ``expected_start`` is the kernel start time captured when we spawned the @@ -581,10 +643,15 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> on Windows.) Headless Chromium has no GUI window, so the softer ``taskkill /T`` without ``/F`` won't reach it either. - ``psutil`` is a hard dependency (see ``pyproject.toml``); the - bare-``os.kill`` fallback covers OSError / PermissionError on - POSIX and a missing ``taskkill.exe`` on Windows (effectively - unreachable on real Windows installs, but cheap insurance). + ``psutil`` is a hard dependency (see ``pyproject.toml``). On Windows, + taskkill failure is explicit: falling back to ``os.kill`` would kill + only the wrapper and could not prove descendant cleanup. + + Returns ``False`` only when the root PID identity no longer matches; + successful verified termination returns ``True``. Windows taskkill, + snapshot, timeout, and survivor failures raise ``OSError`` so existing + best-effort cleanup callers can handle them while ``kill_process`` can + never report ``killed`` without real cleanup evidence. """ if expected_start is not None and not cls._host_pid_is_ours(pid, expected_start): # PID was recycled (start time changed) or is gone — never signal a @@ -594,10 +661,11 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> "Refusing to terminate host pid %d: start-time mismatch — " "PID was recycled onto an unrelated process.", pid, ) - return + return False if _IS_WINDOWS: + identities = cls._snapshot_windows_tree_identities(pid, expected_start) try: - subprocess.run( + result = subprocess.run( ["taskkill", "/PID", str(pid), "/T", "/F"], capture_output=True, text=True, encoding='utf-8', errors='replace', @@ -605,24 +673,39 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> creationflags=windows_hide_flags(), stdin=subprocess.DEVNULL, ) - except (FileNotFoundError, subprocess.TimeoutExpired, OSError): - try: - os.kill(pid, signal.SIGTERM) - except (OSError, ProcessLookupError, PermissionError): - pass - return + except subprocess.TimeoutExpired as exc: + raise OSError(f"taskkill timed out for PID {pid}") from exc + except (FileNotFoundError, OSError) as exc: + raise OSError(f"taskkill failed for PID {pid}: {exc}") from exc + + if result.returncode != 0: + details = (result.stderr or result.stdout or "").strip() + raise OSError(details or f"taskkill failed for PID {pid}") + + survivors = cls._wait_for_host_identities_exit( + identities, WINDOWS_TREE_EXIT_TIMEOUT_SECONDS + ) + if survivors: + survivor_pids = ", ".join( + str(survivor_pid) for survivor_pid, _ in survivors + ) + raise OSError( + "taskkill reported success but owned Windows PID(s) survived: " + f"{survivor_pids}" + ) + return True import psutil try: parent = psutil.Process(pid) except psutil.NoSuchProcess: - return + return True except (OSError, PermissionError): try: os.kill(pid, signal.SIGTERM) except (OSError, ProcessLookupError, PermissionError): pass - return + return True # Snapshot the whole tree (children before parent) and SIGTERM each. try: @@ -644,7 +727,7 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> # leak indefinitely. grace = cls._daemon_term_grace_seconds() if grace <= 0: - return + return True # Sleep out the grace window, then independently re-probe every target # and SIGKILL any survivor. We deliberately do NOT trust # ``psutil.wait_procs``'s gone/alive partition here: it reaps via @@ -670,6 +753,7 @@ def _terminate_host_pid(cls, pid: int, expected_start: Optional[int] = None) -> pass except (psutil.AccessDenied, OSError): pass + return True # ----- Spawn ----- @@ -1133,10 +1217,11 @@ def _pty_reader_loop(self, session: ProcessSession): pty.wait() except Exception as e: logger.debug("PTY wait timed out or failed: %s", e) - session.exited = True - if session.completion_reason != "killed": - session.exit_code = pty.exitstatus if hasattr(pty, 'exitstatus') else -1 - session.completion_reason = "exited" + with session._lock: + session.exited = True + if session.completion_reason != "killed": + session.exit_code = pty.exitstatus if hasattr(pty, 'exitstatus') else -1 + session.completion_reason = "exited" self._move_to_finished(session) def _move_to_finished(self, session: ProcessSession): @@ -1605,7 +1690,57 @@ def kill_process( # Kill via PTY, Popen (local), or env execute (non-local) try: if session._pty: - # PTY process -- terminate via ptyprocess + if _IS_WINDOWS: + # The PTY handle owns only the wrapper. Snapshot, identity- + # check, tree-kill, and verify the Windows process tree + # before touching that handle; otherwise parent/PPID evidence + # can disappear while descendants survive. + if not session.pid or session.host_start_time is None: + return { + "status": "error", + "error": ( + "Windows PTY process identity is unavailable; " + "refusing unverified tree cleanup" + ), + } + with session._lock: + tree_killed = self._terminate_host_pid( + session.pid, session.host_start_time + ) + if not tree_killed: + return { + "status": "error", + "error": ( + "Windows PTY process identity no longer matches; " + "tree cleanup was not attempted" + ), + } + try: + session._pty.terminate(force=True) + except Exception as exc: + # The verified tree is already gone. PTY implementations + # may reject terminate() after observing that exit; this + # is handle teardown noise, not a cleanup failure. + logger.debug("PTY handle teardown after tree kill failed: %s", exc) + + output = strip_ansi(session.output_buffer[-2000:]) + if consume_output: + self._completion_consumed.add(session_id) + session.exited = True + session.exit_code = -15 # SIGTERM-compatible tool contract + session.completion_reason = "killed" + session.termination_source = source + self._move_to_finished(session) + self._write_checkpoint() + return { + "status": "killed", + "session_id": session.id, + "completion_reason": session.completion_reason, + "termination_source": session.termination_source, + "output": output, + } + + # POSIX PTY process -- preserve ptyprocess termination behavior. try: session._pty.terminate(force=True) except Exception: