Skip to content

fix: make serve lifecycle work on Windows (process + permission semantics) - #132

Merged
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/windows-serve-lifecycle
Aug 19, 2026
Merged

fix: make serve lifecycle work on Windows (process + permission semantics)#132
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/windows-serve-lifecycle

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

amplifier-agent serve (status / stop / restart + startup state persistence) relied on three POSIX-only behaviors that break on native Windows. Every POSIX path is behaviorally unchanged.

1. State file can't be written → serve can't start. 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/0o666), so that verification fails on every Windows machine — serve.json cannot be written at all. The strict verify is now POSIX-only; on Windows the protection boundary is the ACL on %USERPROFILE%\.amplifier-agent, which by default denies other standard users. chmod is still called there — only the POSIX-specific assertion is dropped.

2. serve stop --force crashes. The force path, the graceful-timeout escalation, and serve restart all called os.kill(pid, signal.SIGKILL). signal.SIGKILL does not exist on Windows — accessing the attribute raises AttributeError before os.kill ever runs. Added _hard_kill(): SIGKILL on POSIX, SIGTERM on Windows (CPython routes it to TerminateProcess, the correct unconditional-kill equivalent).

3. is_pid_alive() was unsafe and wrong on Windows. It used os.kill(pid, 0). CPython's posixmodule.c maps CTRL_C_EVENT/CTRL_BREAK_EVENT to GenerateConsoleCtrlEvent and every other signal to TerminateProcess. Because wincon.h defines CTRL_C_EVENT == 0, signal 0 takes the first branch: it delivers a real console Ctrl+C to the target's process group instead of testing for existence. Replaced with _is_pid_alive_windows() — a non-destructive OpenProcess(SYNCHRONIZE) + WaitForSingleObject(handle, 0) probe that observes without signalling.

Also: serve restart used start_new_session=True to detach — POSIX-only; Windows now uses DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP.

Correction to an earlier claim in this PR

An earlier revision of this description stated that signal 0 on Windows is "CTRL_C_EVENT, a no-op for an unrelated process," and downgraded the is_pid_alive defect to a mere false-positive liveness result. That conclusion was wrong, and it understated the bug.

Signal 0 does map to CTRL_C_EVENT, but CPython does not treat it as a no-op — it calls GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid), which delivers a genuine Ctrl+C to every process sharing that console process group. It is benign only when the target happens not to share your console. When serve was started from the same terminal — the common case for a local dev server — serve status could interrupt the very server it was reporting on, and could hit unrelated processes in that group as collateral.

So the defect is not just a bad reading; it is a status command with a side effect on the thing it observes. The handle-probe replacement is what makes the observation actually passive.

What changed since the original commit

The original fix (6d710cf, Brian Krabach (@bkrabach)) is preserved as its own commit. A follow-up commit (84872dd) applies these refinements:

  • Two real ctypes bugs fixed in _is_pid_alive_windows():
    • Explicit argtypes/restype declared. Without restype = wintypes.HANDLE, ctypes defaults to c_int and truncates the 64-bit HANDLECloseHandle would then be handed a bogus value (leaking the real handle), and a valid handle could read as falsy.
    • ctypes.WinDLL(..., use_last_error=True) + ctypes.get_last_error() instead of kernel32.GetLastError(). The direct call reads the error state after ctypes' own bookkeeping and can return a stale or cleared value, which would misclassify an ACCESS_DENIED (process alive, not ours) as "dead."
  • Duplicated permission logic consolidated into a single _enforce_mode() helper, shared by _ensure_state_dir and write_state_file. POSIX behavior is byte-for-byte identical.
  • Comments corrected to name the actual mechanism rather than the symptom — the previous comment on is_pid_alive asserted signal 0 would "terminate" the process, which is the wrong branch of posixmodule.c. Also documented that signal.SIGKILL must be probed with hasattr (accessing it raises, so it cannot be caught).
  • docs/spec/http-face.md updated. This is a spec-first repo and the permission contract is stated there; it now says the 0600/0700 enforcement is a guarantee on POSIX and a best effort on Windows, and why.
  • CHANGELOG.md entry added under [Unreleased].

Verification

ruff check, ruff format --check, and pyright src/ are clean.

Behavioral verification against the real lifecycle functions on macOS (POSIX), driving real subprocesses:

POSIX enforcement still fails closed:
  dir  0700 unenforceable -> PermissionError, names 0700 + actual, no api_key in message
  file 0600 unenforceable -> PermissionError, names 0600 + actual, no api_key in message
  chmod raising NotImplementedError -> PermissionError (fails closed)
Windows branch (flag forced on):
  writes successfully despite unenforceable mode; tolerates NotImplementedError
Liveness:
  self -> True   running child -> True   reaped child -> False   pid 1 (root-owned) -> True
  probing a live process 5x leaves it running   <- the property the Win32 probe exists to preserve
Kill / wait:
  _hard_kill terminates target (rc=-9)   wait_for_exit True on exit, False on timeout
  timeout poll leaves the process running
State file:
  0600 file / 0700 dir, round-trip intact, schema_version stamped, no leftover tempfiles

Windows-native evidence from the original commit (Python 3.14.3, 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

Known limits

  • No Windows CI leg. This repo's CI is ubuntu-latest only, so nothing guards these paths against future regression. The Windows evidence above is one machine, one Python (3.14.3). Worth a follow-up; out of scope here.
  • The ctypes corrections are reasoned from the Win32 and ctypes contracts and are not yet re-exercised on native Windows. They are strictly-safer declarations (correct handle width, correct error source) — the failure modes they remove are the undeclared ones.
  • POSIX paths are unchanged and verified above, so POSIX behavior cannot regress.

…antics)

`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>
…helper extraction

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>
@sadlilas
Salil Das (sadlilas) force-pushed the fix/windows-serve-lifecycle branch from 6d29984 to 84872dd Compare August 18, 2026 23:07
@sadlilas
Salil Das (sadlilas) merged commit 8c82773 into main Aug 19, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants