bug: Windows PTY backend inverts TerminateProcess success/failure handling

Open 💬 3 comments Opened Mar 8, 2026 by William17738

Summary

do_kill() in the Windows PTY backend treats TerminateProcess success as failure and failure as success. The Win32 API returns non-zero on success and zero on failure, but the code has the condition inverted.

Root cause

File: codex-rs/utils/pty/src/win/mod.rs, lines 67-76

fn do_kill(&mut self) -> IoResult<()> {
    let proc = self.proc.lock().unwrap().try_clone().unwrap();
    let res = unsafe { TerminateProcess(proc.as_raw_handle() as _, 1) };
    let err = IoError::last_os_error();
    if res != 0 {    // TerminateProcess returns non-zero on SUCCESS
        Err(err)     // ← but this branch treats it as failure
    } else {         // returns zero on FAILURE
        Ok(())       // ← and this branch treats it as success
    }
}

Cross-reference: the pipe backend gets it right

File: codex-rs/utils/pty/src/pipe.rs, lines 55-74

The pipe backend calls the same TerminateProcess API but with the correct condition:

let ret = unsafe { TerminateProcess(handle, 1) };
if ret == 0 {
    // ret == 0 means failure → return error
    return Err(io::Error::last_os_error());
}
// ret != 0 means success → Ok

This internal inconsistency within the same crate confirms the PTY backend has the logic inverted.

Impact

  1. Successful kills return Err: callers believe termination failed when it succeeded.
  2. Failed kills return Ok: callers believe termination succeeded when it didn't → orphaned processes.
  3. The bug is masked: ChildKiller::kill() at line 80-83 calls self.do_kill().ok() and always returns Ok(()), swallowing the inverted error silently.

This can explain reports of processes not being properly cleaned up on Windows, leading to resource leaks and potential interference with subsequent operations.

Suggested fix

Swap the condition to match the pipe backend and the Win32 API contract:

fn do_kill(&mut self) -> IoResult<()> {
    let proc = self.proc.lock().unwrap().try_clone().unwrap();
    let res = unsafe { TerminateProcess(proc.as_raw_handle() as _, 1) };
    if res == 0 {
        Err(IoError::last_os_error())
    } else {
        Ok(())
    }
}

I have a fix ready and can submit a PR if invited.

View original on GitHub ↗

This issue has 3 comments on GitHub. Read the full discussion on GitHub ↗