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 55601806..24a1d522 100644 --- a/src/amplifier_agent_cli/admin/serve_lifecycle.py +++ b/src/amplifier_agent_cli/admin/serve_lifecycle.py @@ -41,6 +41,24 @@ _STATE_FILE_MODE = 0o600 _STATE_DIR_MODE = 0o700 +# 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" + def _state_dir() -> Path: """Return the directory that holds ``serve.json``. @@ -61,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. @@ -69,23 +123,21 @@ def _ensure_state_dir() -> Path: """ d = _state_dir() d.mkdir(parents=True, exist_ok=True) - 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 @@ -111,21 +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: - try: - os.chmod(tmp_path, _STATE_FILE_MODE) - except NotImplementedError as exc: - raise PermissionError( + # 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." - ) 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)}). " + ), + 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) @@ -171,13 +223,80 @@ def remove_state_file() -> None: # --------------------------------------------------------------------------- +def _is_pid_alive_windows(pid: int) -> bool: + """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 + + # 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 ctypes.get_last_error() == ERROR_ACCESS_DENIED # type: ignore[attr-defined] + 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 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) + + 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`: 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) try: os.kill(pid, 0) return True @@ -301,7 +420,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 +436,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 +495,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 +516,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.