From 6c584de435922d842080dab5256512ada691e6de Mon Sep 17 00:00:00 2001 From: yoggydev <280342032+yoggydev@users.noreply.github.com> Date: Mon, 24 Aug 2026 06:30:08 +0900 Subject: [PATCH] test(cli): drive the Windows pid probe through an injected kernel32 --- .../client/cli/commands/autoroute/process.py | 62 +++++++++---- .../client/cli/autoroute/test_process.py | 91 +++++++++++++++++++ 2 files changed, 135 insertions(+), 18 deletions(-) diff --git a/litellm/proxy/client/cli/commands/autoroute/process.py b/litellm/proxy/client/cli/commands/autoroute/process.py index 3c9d32ce4fb..898cbfcd66d 100644 --- a/litellm/proxy/client/cli/commands/autoroute/process.py +++ b/litellm/proxy/client/cli/commands/autoroute/process.py @@ -8,6 +8,7 @@ import subprocess import sys import threading import time +from collections.abc import Callable from dataclasses import dataclass from pathlib import Path from typing import Final @@ -143,19 +144,17 @@ _STILL_ACTIVE: Final = 259 _ERROR_ACCESS_DENIED: Final = 5 -def _windows_pid_exists(pid: int) -> bool: - """Liveness check for Windows that does not signal the process. +@dataclass(frozen=True, slots=True) +class _WindowsProcessApi: + """The kernel32 surface ``_windows_pid_exists`` needs, injected so the probe is testable off Windows.""" - Opens a query-only handle and asks for the exit code. ``OpenProcess`` - failing with ``ERROR_ACCESS_DENIED`` means the pid exists but is not ours - to open, which is the case the ``PermissionError`` arm covers on POSIX. + open_query_handle: Callable[[int], int] + exit_code: Callable[[int], int | None] + close_handle: Callable[[int], None] + last_error: Callable[[], int] - Caveat kept deliberately: a process whose real exit code is 259 reads as - alive, because ``GetExitCodeProcess`` reports ``STILL_ACTIVE`` (259) for a - running process and cannot distinguish the two. That is the standard - trade-off for this API and it is strictly better than the previous - behaviour, which killed the process it was asked about. - """ + +def _load_windows_process_api() -> _WindowsProcessApi: # pragma: no cover - kernel32 is Windows-only; CI is Linux import ctypes from ctypes import wintypes @@ -167,16 +166,43 @@ def _windows_pid_exists(pid: int) -> bool: kernel32.CloseHandle.argtypes = (wintypes.HANDLE,) kernel32.CloseHandle.restype = wintypes.BOOL - handle: Final = kernel32.OpenProcess(_PROCESS_QUERY_LIMITED_INFORMATION, False, pid) + def exit_code(handle: int) -> int | None: + code = wintypes.DWORD() + if not kernel32.GetExitCodeProcess(handle, ctypes.byref(code)): + return None + return code.value + + return _WindowsProcessApi( + open_query_handle=lambda pid: kernel32.OpenProcess(_PROCESS_QUERY_LIMITED_INFORMATION, False, pid), + exit_code=exit_code, + close_handle=kernel32.CloseHandle, + last_error=ctypes.get_last_error, + ) + + +def _windows_pid_exists(pid: int, api: _WindowsProcessApi | None = None) -> bool: + """Liveness check for Windows that does not signal the process. + + Opens a query-only handle and asks for the exit code. ``OpenProcess`` + failing with ``ERROR_ACCESS_DENIED`` means the pid exists but is not ours + to open, which is the case the ``PermissionError`` arm covers on POSIX. + An unreadable exit code is reported as alive rather than guessed away. + + Caveat kept deliberately: a process whose real exit code is 259 reads as + alive, because ``GetExitCodeProcess`` reports ``STILL_ACTIVE`` (259) for a + running process and cannot distinguish the two. That is the standard + trade-off for this API and it is strictly better than the previous + behaviour, which killed the process it was asked about. + """ + resolved: Final = api if api is not None else _load_windows_process_api() + handle: Final = resolved.open_query_handle(pid) if not handle: - return ctypes.get_last_error() == _ERROR_ACCESS_DENIED + return resolved.last_error() == _ERROR_ACCESS_DENIED try: - exit_code: Final = wintypes.DWORD() - if not kernel32.GetExitCodeProcess(handle, ctypes.byref(exit_code)): - return True - return exit_code.value == _STILL_ACTIVE + code: Final = resolved.exit_code(handle) + return code is None or code == _STILL_ACTIVE finally: - kernel32.CloseHandle(handle) + resolved.close_handle(handle) def is_running(pid: int) -> bool: diff --git a/tests/test_litellm/proxy/client/cli/autoroute/test_process.py b/tests/test_litellm/proxy/client/cli/autoroute/test_process.py index b0f1ba96636..a2b0b608024 100644 --- a/tests/test_litellm/proxy/client/cli/autoroute/test_process.py +++ b/tests/test_litellm/proxy/client/cli/autoroute/test_process.py @@ -14,6 +14,8 @@ from litellm.proxy.client.cli.commands.autoroute.process import ( PidRecord, ProcessLaunchError, UpError, + _windows_pid_exists, + _WindowsProcessApi, clear_pid_record, is_port_available, is_running, @@ -44,6 +46,18 @@ def _signals_sent(fake_kill: Mock) -> tuple[int, ...]: return tuple(call.args[1] for call in fake_kill.call_args_list) +def _windows_api(handle: int, exit_code: int | None = None, last_error: int = 0) -> tuple[_WindowsProcessApi, Mock]: + """A kernel32 stand-in, plus the Mock that recorded ``CloseHandle``.""" + close_handle: Final = Mock() + api: Final = _WindowsProcessApi( + open_query_handle=Mock(return_value=handle), + exit_code=Mock(return_value=exit_code), + close_handle=close_handle, + last_error=Mock(return_value=last_error), + ) + return api, close_handle + + class TestIsPortAvailable: def test_true_for_a_free_port(self): with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: @@ -185,6 +199,83 @@ class TestIsRunningDoesNotSignal: assert _signals_sent(fake_kill) == (), "os.kill reached on win32; signal 0 is CTRL_C_EVENT there" +class TestWindowsPidExists: + """The Windows probe, driven through an injected kernel32 so it runs on any platform. + + ``OpenProcess`` / ``GetExitCodeProcess`` only exist on Windows, so CI could + never execute this decision logic before. The four calls the probe needs + now arrive as a ``_WindowsProcessApi``; only the loader that builds the real + one out of ``ctypes`` stays Windows-only. + """ + + _ERROR_ACCESS_DENIED: Final = 5 + _ERROR_INVALID_PARAMETER: Final = 87 + _STILL_ACTIVE: Final = 259 + + def test_access_denied_on_open_means_the_pid_exists(self): + """A pid we may not open is still a live pid; POSIX answers the same way via PermissionError.""" + api, close_handle = _windows_api(handle=0, last_error=self._ERROR_ACCESS_DENIED) + + assert _windows_pid_exists(4321, api) is True + close_handle.assert_not_called() + + def test_any_other_open_failure_means_the_pid_is_gone(self): + api, close_handle = _windows_api(handle=0, last_error=self._ERROR_INVALID_PARAMETER) + + assert _windows_pid_exists(4321, api) is False + close_handle.assert_not_called() + + def test_still_active_means_running(self): + api, close_handle = _windows_api(handle=99, exit_code=self._STILL_ACTIVE) + + assert _windows_pid_exists(4321, api) is True + close_handle.assert_called_once_with(99) + + def test_a_real_exit_code_means_the_process_finished(self): + api, close_handle = _windows_api(handle=99, exit_code=0) + + assert _windows_pid_exists(4321, api) is False + close_handle.assert_called_once_with(99) + + def test_an_unreadable_exit_code_is_reported_as_running(self): + """``GetExitCodeProcess`` failing is not evidence the process died, so do not claim it did.""" + api, close_handle = _windows_api(handle=99, exit_code=None) + + assert _windows_pid_exists(4321, api) is True + close_handle.assert_called_once_with(99) + + def test_the_handle_is_released_even_when_the_query_raises(self): + """A leaked query handle would keep the exited process object alive for the life of the CLI.""" + close_handle: Final = Mock() + api: Final = _WindowsProcessApi( + open_query_handle=Mock(return_value=99), + exit_code=Mock(side_effect=OSError(22, "Invalid argument")), + close_handle=close_handle, + last_error=Mock(return_value=0), + ) + + with pytest.raises(OSError): + _windows_pid_exists(4321, api) + + close_handle.assert_called_once_with(99) + + def test_the_probe_asks_about_the_pid_it_was_given(self): + api, _ = _windows_api(handle=99, exit_code=self._STILL_ACTIVE) + + _windows_pid_exists(4321, api) + + api.open_query_handle.assert_called_once_with(4321) + + def test_the_probe_never_signals_the_process(self, monkeypatch): + """The bug this replaces: ``os.kill(pid, 0)`` is ``CTRL_C_EVENT`` on Windows.""" + fake_kill: Final = Mock() + monkeypatch.setattr(process_module.os, "kill", fake_kill) + api, _ = _windows_api(handle=99, exit_code=self._STILL_ACTIVE) + + assert _windows_pid_exists(4321, api) is True + assert _signals_sent(fake_kill) == () + + class TestTerminateHardKillSignal: def test_escalation_resolves_a_signal_when_sigkill_is_absent(self, monkeypatch): """``signal.SIGKILL`` does not exist on Windows.