From 6d710cf61a5ece4f86499b7481d8af6e2d7946a5 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:19:39 -0700 Subject: [PATCH 1/2] fix: make `serve` lifecycle work on Windows (process + permission semantics) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `amplifier-agent serve` (status / stop / restart + startup state persistence) relied on three POSIX-only behaviors that break on native Windows: 1. write_state_file / _ensure_state_dir chmod to 0o700/0o600 and then STRICTLY verify the mode via stat.S_IMODE, raising PermissionError on mismatch. NTFS has no POSIX mode bits (stat reports 0o777), so that verification fails on every Windows machine -- serve.json cannot be written, so `serve` cannot even start. Now the strict chmod+verify is guarded to POSIX; on Windows the state dir lives under %USERPROFILE%\.amplifier-agent (ACL-restricted to the user). 2. `serve stop --force`, the graceful-timeout escalation, and `serve restart` all call os.kill(pid, signal.SIGKILL). signal.SIGKILL does not exist on Windows -> AttributeError before os.kill runs. Added _hard_kill(): SIGKILL on POSIX, SIGTERM (which maps to TerminateProcess) on Windows. 3. is_pid_alive() used os.kill(pid, 0). On Windows signal 0 is CTRL_C_EVENT semantics, not an existence check -- it reports DEAD pids as alive (false positive), so `serve status` shows a dead server as running and the stale state file is never cleaned. Added _is_pid_alive_windows() using a non-destructive Win32 handle probe (OpenProcess(SYNCHRONIZE) + WaitForSingleObject(0)). Also: `serve restart` used start_new_session=True to detach, a POSIX no-op on Windows; now uses DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP there. Every POSIX path is byte-unchanged (guarded on os.name == "nt"). Evidence -- teeth both ways, native Windows (Python 3.14.3), real functions driven against a real dummy process: BASELINE: is_pid_alive(dead_pid) -> True (false positive) write_state_file -> PermissionError: Failed to set mode 0700 force-kill -> AttributeError: module 'signal' has no attribute 'SIGKILL' FIXED: is_pid_alive dead->False live->True write_state_file -> wrote+read serve.json force-kill(_hard_kill)-> process terminated Note: an earlier audit predicted os.kill(pid,0) would TERMINATE the server; a direct probe on Python 3.14.3 showed it does not (it is CTRL_C_EVENT, a no-op for an unrelated process), so the real is_pid_alive defect is the false-positive liveness result above, not a kill. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .../admin/serve_lifecycle.py | 120 ++++++++++++++---- 1 file changed, 96 insertions(+), 24 deletions(-) diff --git a/src/amplifier_agent_cli/admin/serve_lifecycle.py b/src/amplifier_agent_cli/admin/serve_lifecycle.py index 55601806..bad0fc98 100644 --- a/src/amplifier_agent_cli/admin/serve_lifecycle.py +++ b/src/amplifier_agent_cli/admin/serve_lifecycle.py @@ -41,6 +41,16 @@ _STATE_FILE_MODE = 0o600 _STATE_DIR_MODE = 0o700 +# Windows has fundamentally different process/permission semantics than POSIX: +# - os.kill(pid, 0) is NOT a benign liveness probe -- CPython maps any signal +# other than CTRL_C_EVENT/CTRL_BREAK_EVENT to TerminateProcess, so it would +# KILL the process we only meant to check. +# - signal.SIGKILL does not exist (AttributeError on access). +# - chmod cannot produce POSIX mode bits on NTFS (stat reports 0o777/0o666), +# so the strict 0o700/0o600 verification below can never pass. +# Each of these is guarded on this flag; the POSIX paths are unchanged. +_IS_WINDOWS = os.name == "nt" + def _state_dir() -> Path: """Return the directory that holds ``serve.json``. @@ -69,6 +79,13 @@ def _ensure_state_dir() -> Path: """ d = _state_dir() d.mkdir(parents=True, exist_ok=True) + if _IS_WINDOWS: + # NTFS has no POSIX mode bits: chmod cannot produce 0o700 and stat + # reports 0o777, so the strict verification below would fail on every + # Windows machine and refuse to persist serve.json. The directory lives + # under the user profile (%USERPROFILE%\.amplifier-agent), which is + # ACL-restricted to the user by default. + return d try: d.chmod(_STATE_DIR_MODE) except NotImplementedError as exc: @@ -111,21 +128,25 @@ def write_state_file(payload: dict[str, Any]) -> None: # Write into a tempfile in the same directory so os.replace is atomic. fd, tmp_path = tempfile.mkstemp(dir=d, prefix=".serve-", suffix=".json.tmp") try: - try: - os.chmod(tmp_path, _STATE_FILE_MODE) - except NotImplementedError as exc: - raise PermissionError( - f"Cannot set mode 0600 on {tmp_path}. " - "Your filesystem may not support Unix mode bits. " - "Refusing to write api_key in plaintext without permission enforcement." - ) from exc - # Verify enforcement before writing the sensitive payload. - actual = stat.S_IMODE(os.stat(tmp_path).st_mode) - if actual != _STATE_FILE_MODE: - raise PermissionError( - f"Failed to set mode 0600 on {tmp_path} (got {oct(actual)}). " - "Refusing to write api_key in plaintext without enforced file permissions." - ) + if not _IS_WINDOWS: + try: + os.chmod(tmp_path, _STATE_FILE_MODE) + except NotImplementedError as exc: + raise PermissionError( + f"Cannot set mode 0600 on {tmp_path}. " + "Your filesystem may not support Unix mode bits. " + "Refusing to write api_key in plaintext without permission enforcement." + ) from exc + # Verify enforcement before writing the sensitive payload. + actual = stat.S_IMODE(os.stat(tmp_path).st_mode) + if actual != _STATE_FILE_MODE: + raise PermissionError( + f"Failed to set mode 0600 on {tmp_path} (got {oct(actual)}). " + "Refusing to write api_key in plaintext without enforced file permissions." + ) + # On Windows the tempfile (created by mkstemp under the user-profile + # state dir) inherits the user's ACLs; POSIX 0600 is neither achievable + # nor meaningful there. os.write(fd, encoded) finally: os.close(fd) @@ -171,13 +192,55 @@ def remove_state_file() -> None: # --------------------------------------------------------------------------- +def _is_pid_alive_windows(pid: int) -> bool: + """Windows liveness check that does NOT terminate the process. + + ``os.kill(pid, 0)`` cannot be used here: on Windows CPython maps any signal + other than CTRL_C/CTRL_BREAK to ``TerminateProcess``, so signal 0 would KILL + the very process we are probing. Instead we open a handle and poll it: + ``WaitForSingleObject(handle, 0)`` returns WAIT_TIMEOUT while the process is + alive and WAIT_OBJECT_0 once it has exited. ACCESS_DENIED from OpenProcess + means the process exists but is owned by another user (mirror the POSIX + ``PermissionError`` -> alive case). + """ + import ctypes + + SYNCHRONIZE = 0x00100000 + WAIT_TIMEOUT = 0x00000102 + ERROR_ACCESS_DENIED = 5 + + kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] + handle = kernel32.OpenProcess(SYNCHRONIZE, False, pid) + if not handle: + return kernel32.GetLastError() == ERROR_ACCESS_DENIED + try: + return kernel32.WaitForSingleObject(handle, 0) == WAIT_TIMEOUT + finally: + kernel32.CloseHandle(handle) + + +def _hard_kill(pid: int) -> None: + """Forcefully terminate ``pid`` (the SIGKILL equivalent). + + Windows has no ``signal.SIGKILL`` (accessing it raises ``AttributeError``); + ``os.kill(pid, signal.SIGTERM)`` maps to ``TerminateProcess`` there, which is + the correct unconditional-kill equivalent. + """ + sig = signal.SIGKILL if hasattr(signal, "SIGKILL") else signal.SIGTERM + os.kill(pid, sig) + + def is_pid_alive(pid: int) -> bool: """Return True if process ``pid`` exists and is signalable. - Uses ``os.kill(pid, 0)`` (signal 0 checks existence without delivering - a signal). ``PermissionError`` means the process exists but we don't - own it — still alive. ``ProcessLookupError`` means it is gone. + On POSIX, uses ``os.kill(pid, 0)`` (signal 0 checks existence without + delivering a signal). ``PermissionError`` means the process exists but we + don't own it — still alive. ``ProcessLookupError`` means it is gone. On + Windows, delegates to :func:`_is_pid_alive_windows` because ``os.kill(pid, + 0)`` would terminate the process there. """ + if _IS_WINDOWS: + return _is_pid_alive_windows(pid) try: os.kill(pid, 0) return True @@ -301,7 +364,7 @@ def stop_command(force: bool, timeout_s: float) -> None: raise SystemExit(0) if force: - os.kill(pid, signal.SIGKILL) + _hard_kill(pid) wait_for_exit(pid, timeout=2.0) remove_state_file() click.echo(f"amplifier-agent serve: stopped (SIGKILL, PID {pid})") @@ -317,7 +380,7 @@ def stop_command(force: bool, timeout_s: float) -> None: raise SystemExit(0) # Graceful window expired — escalate. - os.kill(pid, signal.SIGKILL) + _hard_kill(pid) wait_for_exit(pid, timeout=2.0) remove_state_file() click.echo( @@ -376,7 +439,7 @@ def restart_command() -> None: os.kill(old_pid, signal.SIGTERM) if is_pid_alive(old_pid) else None if not wait_for_exit(old_pid, timeout=5.0): if is_pid_alive(old_pid): - os.kill(old_pid, signal.SIGKILL) + _hard_kill(old_pid) wait_for_exit(old_pid, timeout=2.0) remove_state_file() @@ -397,14 +460,23 @@ def restart_command() -> None: if host_config_path: cmd.extend(["--config", host_config_path]) - # Launch detached — stdout/stderr go to /dev/null; the server writes its - # own logs via uvicorn's log machinery. + # Launch detached — stdout/stderr go to the null device; the server writes + # its own logs via uvicorn's log machinery. Detach differently per platform: + # start_new_session (setsid) is a POSIX no-op on Windows, where real + # detachment requires DETACHED_PROCESS + a new process group. devnull = open(os.devnull, "wb") + detach_kwargs: dict[str, Any] = {} + if _IS_WINDOWS: + detach_kwargs["creationflags"] = ( + subprocess.DETACHED_PROCESS | subprocess.CREATE_NEW_PROCESS_GROUP # type: ignore[attr-defined] + ) + else: + detach_kwargs["start_new_session"] = True subprocess.Popen( cmd, - start_new_session=True, stdout=devnull, stderr=devnull, + **detach_kwargs, ) # Wait for the new state file to appear, indicating a successful lifespan. From 84872dd4e0fb4eaa1f11dfc67b6f1c5aec03303e Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:07:16 -0700 Subject: [PATCH 2/2] refactor: polish Windows lifecycle fixes with ctypes corrections and helper extraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refinements to the Windows serve lifecycle fix: - Extract duplicated chmod-then-verify permission logic into _enforce_mode() helper. POSIX behavior is byte-for-byte identical (still fails closed with PermissionError rather than writing api_key plaintext). Windows branch skips unenforceable POSIX mode verification but preserves access control intent (ACL on %USERPROFILE%\.amplifier-agent). - Fix two real ctypes bugs in _is_pid_alive_windows(): * Declare explicit argtypes/restype. Without restype = wintypes.HANDLE, ctypes defaults to c_int and truncates 64-bit HANDLE, leaking handles and reading valid handles as falsy. * Use ctypes.WinDLL with use_last_error=True and ctypes.get_last_error() instead of kernel32.GetLastError(), which can return stale/cleared values. - Correct and expand explanatory comments: * Name the actual CPython mechanism: os.kill maps CTRL_C_EVENT/CTRL_BREAK_EVENT to GenerateConsoleCtrlEvent; CTRL_C_EVENT == 0, so signal 0 delivers real Ctrl+C, not a benign liveness probe. * Document signal.SIGKILL access with hasattr (raises AttributeError on Windows); SIGTERM is the correct fallback. * Add _IS_WINDOWS constant with notes on three platform divergences. - Update docs/spec/http-face.md to clarify 0600/0700 permission verification as a guarantee on POSIX, best-effort on Windows (ACL is the boundary). - Update CHANGELOG.md with Fixed entry under [Unreleased] for all three Windows lifecycle fixes plus permission-verification change. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- CHANGELOG.md | 25 +++ docs/spec/http-face.md | 7 + .../admin/serve_lifecycle.py | 180 ++++++++++++------ 3 files changed, 150 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 621bd890..cec3b959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 are set), and `auth set gemini` is accepted and stores the key like any other keyed provider. Default model is `gemini-2.5-flash`. +### Fixed + +- **`serve status` / `stop` / `restart` no longer misbehave on Windows.** Three + POSIX assumptions in the serve lifecycle were wrong on Windows and are now + branched explicitly: + - `os.kill(pid, 0)` was used as a benign liveness probe. On Windows it is not + one: CPython maps `CTRL_C_EVENT` and `CTRL_BREAK_EVENT` to + `GenerateConsoleCtrlEvent` and every other signal to `TerminateProcess`, and + because `CTRL_C_EVENT == 0`, signal 0 delivers a real console Ctrl+C to the + target's process group. `serve status` could therefore interrupt the very + server it was reporting on. Liveness is now checked with + `OpenProcess`/`WaitForSingleObject`, which observes without signalling. + - `signal.SIGKILL` does not exist on Windows and raised `AttributeError` when + escalating a stop; the escalation path now falls back to `SIGTERM`, which + CPython routes to `TerminateProcess`. + - `subprocess.Popen(start_new_session=True)` is a POSIX-only way to detach the + restarted server; Windows now uses `DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP`. + + The state file's 0600/0700 permission *verification* is also now skipped on + Windows, where NTFS has no POSIX mode bits and the check could never pass — + previously this refused to write `serve.json` at all. `chmod` is still applied; + only the POSIX-specific assertion is dropped, and the protection boundary there + is the user-profile ACL. POSIX behaviour is unchanged: enforcement still fails + closed rather than writing a plaintext `api_key` at a looser mode. + ## [0.13.0] — 2026-08-18 ### Added diff --git a/docs/spec/http-face.md b/docs/spec/http-face.md index 54978732..a2364151 100644 --- a/docs/spec/http-face.md +++ b/docs/spec/http-face.md @@ -55,6 +55,13 @@ Written atomically, and never observable at a looser mode than 0600: the permiss and verified before any payload is written. The parent directory is forced to 0700 with the same verification. Both fail with an error rather than write a plaintext `api_key` at a looser mode. +On Windows the verification is skipped, because the guarantee it asserts is not one the platform +makes: NTFS has no POSIX mode bits, `chmod` cannot produce 0600/0700, and `stat` reports 0666/0777, +so the comparison could never pass and the server could never persist its state. The protection +boundary there is the ACL on `%USERPROFILE%\.amplifier-agent`, which by default denies other +standard users. `chmod` is still applied; only the POSIX-specific assertion is dropped. The +enforcement above is therefore a guarantee on POSIX and a best effort on Windows. + The file is removed on shutdown and also from SIGTERM/SIGINT handlers, so a kill during startup does not leave a stale file behind. diff --git a/src/amplifier_agent_cli/admin/serve_lifecycle.py b/src/amplifier_agent_cli/admin/serve_lifecycle.py index bad0fc98..24a1d522 100644 --- a/src/amplifier_agent_cli/admin/serve_lifecycle.py +++ b/src/amplifier_agent_cli/admin/serve_lifecycle.py @@ -41,14 +41,22 @@ _STATE_FILE_MODE = 0o600 _STATE_DIR_MODE = 0o700 -# Windows has fundamentally different process/permission semantics than POSIX: -# - os.kill(pid, 0) is NOT a benign liveness probe -- CPython maps any signal -# other than CTRL_C_EVENT/CTRL_BREAK_EVENT to TerminateProcess, so it would -# KILL the process we only meant to check. -# - signal.SIGKILL does not exist (AttributeError on access). -# - chmod cannot produce POSIX mode bits on NTFS (stat reports 0o777/0o666), -# so the strict 0o700/0o600 verification below can never pass. -# Each of these is guarded on this flag; the POSIX paths are unchanged. +# Windows differs from POSIX in three ways that matter to this module: +# +# - os.kill(pid, 0) is not a liveness probe. CPython's posixmodule maps +# CTRL_C_EVENT and CTRL_BREAK_EVENT to GenerateConsoleCtrlEvent and every +# other signal to TerminateProcess. Because wincon.h defines CTRL_C_EVENT +# as 0, signal 0 takes the *first* branch: it delivers a real Ctrl+C to the +# target's console process group rather than testing for existence. So the +# "benign probe" is not benign -- it can kill the server, and can hit other +# processes sharing that console group. See _is_pid_alive_windows. +# - signal.SIGKILL does not exist (AttributeError on access). See _hard_kill. +# - Detaching a child needs DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP; +# start_new_session (setsid) is POSIX-only. See restart_command. +# +# File permissions are deliberately NOT keyed off this flag -- whether chmod +# can enforce a mode is a property of the filesystem, not the OS name. See +# _enforce_mode. _IS_WINDOWS = os.name == "nt" @@ -71,6 +79,42 @@ def _state_file() -> Path: # --------------------------------------------------------------------------- +def _enforce_mode(path: Path, mode: int, *, cannot_set: str, not_applied: str) -> None: + """Apply ``mode`` to ``path`` and verify it stuck, else raise ``PermissionError``. + + The state file holds a plaintext ``api_key``, so on POSIX we refuse to + write it unless the mode is genuinely enforced: chmod, then stat back and + compare. Some networked and virtual filesystems silently ignore chmod, and + a mode that did not stick is worse than useless -- it looks like + protection that is not there. + + Windows is exempt from the *verification*, not from the intent. NTFS has + no POSIX mode bits: chmod cannot produce 0o700/0o600, stat reports + 0o777/0o666, so the comparison could never pass and ``serve`` could never + persist its state file on any Windows machine. There the protection + boundary is the ACL on the user profile directory + (``%USERPROFILE%\\.amplifier-agent``), which by default denies other + standard users. We still call chmod -- it sets the owner-write bit and is + harmless -- we just do not assert a POSIX guarantee the platform does not + make. + + ``cannot_set`` and ``not_applied`` are the caller's message tails so each + call site can name what it was protecting. + """ + try: + os.chmod(path, mode) + except NotImplementedError as exc: + if _IS_WINDOWS: + return + raise PermissionError(cannot_set) from exc + if _IS_WINDOWS: + return + # Verify the mode was actually applied (some networked/virtual FSes ignore chmod). + actual = stat.S_IMODE(os.stat(path).st_mode) + if actual != mode: + raise PermissionError(not_applied.format(actual=oct(actual))) + + def _ensure_state_dir() -> Path: """Create the state directory with mode 0700, return its path. @@ -79,30 +123,21 @@ def _ensure_state_dir() -> Path: """ d = _state_dir() d.mkdir(parents=True, exist_ok=True) - if _IS_WINDOWS: - # NTFS has no POSIX mode bits: chmod cannot produce 0o700 and stat - # reports 0o777, so the strict verification below would fail on every - # Windows machine and refuse to persist serve.json. The directory lives - # under the user profile (%USERPROFILE%\.amplifier-agent), which is - # ACL-restricted to the user by default. - return d - try: - d.chmod(_STATE_DIR_MODE) - except NotImplementedError as exc: - raise PermissionError( + _enforce_mode( + d, + _STATE_DIR_MODE, + cannot_set=( f"Cannot set directory permissions on {d}. " "Your filesystem may not support Unix mode bits. " "The state file (which contains a sensitive api_key) cannot be " "written safely without mode 0700 on the parent directory." - ) from exc - # Verify the mode was actually applied (some networked/virtual FSes ignore chmod). - actual = stat.S_IMODE(d.stat().st_mode) - if actual != _STATE_DIR_MODE: - raise PermissionError( - f"Failed to set mode 0700 on {d} (got {oct(actual)}). " + ), + not_applied=( + f"Failed to set mode 0700 on {d} (got {{actual}}). " "The state file contains a sensitive api_key and cannot be written " "safely without enforced directory permissions." - ) + ), + ) return d @@ -128,25 +163,21 @@ def write_state_file(payload: dict[str, Any]) -> None: # Write into a tempfile in the same directory so os.replace is atomic. fd, tmp_path = tempfile.mkstemp(dir=d, prefix=".serve-", suffix=".json.tmp") try: - if not _IS_WINDOWS: - try: - os.chmod(tmp_path, _STATE_FILE_MODE) - except NotImplementedError as exc: - raise PermissionError( - f"Cannot set mode 0600 on {tmp_path}. " - "Your filesystem may not support Unix mode bits. " - "Refusing to write api_key in plaintext without permission enforcement." - ) from exc - # Verify enforcement before writing the sensitive payload. - actual = stat.S_IMODE(os.stat(tmp_path).st_mode) - if actual != _STATE_FILE_MODE: - raise PermissionError( - f"Failed to set mode 0600 on {tmp_path} (got {oct(actual)}). " - "Refusing to write api_key in plaintext without enforced file permissions." - ) - # On Windows the tempfile (created by mkstemp under the user-profile - # state dir) inherits the user's ACLs; POSIX 0600 is neither achievable - # nor meaningful there. + # Restrict the mode *before* the sensitive payload is written, so the + # api_key is never on disk at a more-permissive mode. + _enforce_mode( + Path(tmp_path), + _STATE_FILE_MODE, + cannot_set=( + f"Cannot set mode 0600 on {tmp_path}. " + "Your filesystem may not support Unix mode bits. " + "Refusing to write api_key in plaintext without permission enforcement." + ), + not_applied=( + f"Failed to set mode 0600 on {tmp_path} (got {{actual}}). " + "Refusing to write api_key in plaintext without enforced file permissions." + ), + ) os.write(fd, encoded) finally: os.close(fd) @@ -193,26 +224,45 @@ def remove_state_file() -> None: def _is_pid_alive_windows(pid: int) -> bool: - """Windows liveness check that does NOT terminate the process. - - ``os.kill(pid, 0)`` cannot be used here: on Windows CPython maps any signal - other than CTRL_C/CTRL_BREAK to ``TerminateProcess``, so signal 0 would KILL - the very process we are probing. Instead we open a handle and poll it: - ``WaitForSingleObject(handle, 0)`` returns WAIT_TIMEOUT while the process is - alive and WAIT_OBJECT_0 once it has exited. ACCESS_DENIED from OpenProcess - means the process exists but is owned by another user (mirror the POSIX - ``PermissionError`` -> alive case). + """Windows liveness check that does NOT signal or terminate the process. + + ``os.kill(pid, 0)`` cannot be used here. CPython maps CTRL_C_EVENT and + CTRL_BREAK_EVENT to ``GenerateConsoleCtrlEvent`` and every other signal to + ``TerminateProcess``; since ``CTRL_C_EVENT == 0``, signal 0 takes the first + branch and delivers a real Ctrl+C to the target's console process group. + Either branch can kill what we only meant to observe. + + So we open a handle and poll it instead: ``WaitForSingleObject(handle, 0)`` + returns WAIT_TIMEOUT while the process is alive and WAIT_OBJECT_0 once it + has exited. ACCESS_DENIED from OpenProcess means the process exists but is + owned by another user, mirroring the POSIX ``PermissionError`` -> alive case. """ import ctypes + from ctypes import wintypes SYNCHRONIZE = 0x00100000 WAIT_TIMEOUT = 0x00000102 ERROR_ACCESS_DENIED = 5 - kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] + # use_last_error routes GetLastError through ctypes' own thread-local copy, + # captured immediately after the call. Calling kernel32.GetLastError() + # directly would read the error state *after* ctypes' own bookkeeping and + # can report a stale or cleared value. + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) # type: ignore[attr-defined] + + # Declare signatures explicitly. Without restype, ctypes assumes c_int and + # truncates the 64-bit HANDLE, so CloseHandle would be handed a bogus value + # (leaking the real handle) and a valid handle could read as falsy. + kernel32.OpenProcess.argtypes = (wintypes.DWORD, wintypes.BOOL, wintypes.DWORD) + kernel32.OpenProcess.restype = wintypes.HANDLE + kernel32.WaitForSingleObject.argtypes = (wintypes.HANDLE, wintypes.DWORD) + kernel32.WaitForSingleObject.restype = wintypes.DWORD + kernel32.CloseHandle.argtypes = (wintypes.HANDLE,) + kernel32.CloseHandle.restype = wintypes.BOOL + handle = kernel32.OpenProcess(SYNCHRONIZE, False, pid) if not handle: - return kernel32.GetLastError() == ERROR_ACCESS_DENIED + return ctypes.get_last_error() == ERROR_ACCESS_DENIED # type: ignore[attr-defined] try: return kernel32.WaitForSingleObject(handle, 0) == WAIT_TIMEOUT finally: @@ -222,9 +272,12 @@ def _is_pid_alive_windows(pid: int) -> bool: def _hard_kill(pid: int) -> None: """Forcefully terminate ``pid`` (the SIGKILL equivalent). - Windows has no ``signal.SIGKILL`` (accessing it raises ``AttributeError``); - ``os.kill(pid, signal.SIGTERM)`` maps to ``TerminateProcess`` there, which is - the correct unconditional-kill equivalent. + Windows has no ``signal.SIGKILL`` -- accessing the attribute raises + ``AttributeError``, so it must be probed with ``hasattr`` rather than + caught. SIGTERM is the right fallback there: it is neither CTRL_C_EVENT + nor CTRL_BREAK_EVENT, so CPython routes it to ``TerminateProcess``, which + is the unconditional kill this function promises. On POSIX the two signals + keep their usual meanings and SIGKILL is used directly. """ sig = signal.SIGKILL if hasattr(signal, "SIGKILL") else signal.SIGTERM os.kill(pid, sig) @@ -235,9 +288,12 @@ def is_pid_alive(pid: int) -> bool: On POSIX, uses ``os.kill(pid, 0)`` (signal 0 checks existence without delivering a signal). ``PermissionError`` means the process exists but we - don't own it — still alive. ``ProcessLookupError`` means it is gone. On - Windows, delegates to :func:`_is_pid_alive_windows` because ``os.kill(pid, - 0)`` would terminate the process there. + don't own it — still alive. ``ProcessLookupError`` means it is gone. + + On Windows, delegates to :func:`_is_pid_alive_windows`: signal 0 is not a + no-op probe there (it is CTRL_C_EVENT, which CPython delivers as a real + console Ctrl+C), so the POSIX idiom would signal the process it is meant + to be silently observing. """ if _IS_WINDOWS: return _is_pid_alive_windows(pid)