Skip to content

bug: Windows PTY backend inverts TerminateProcess success/failure handling #13945

@William17738

Description

@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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    agentIssues related to the core agent loopbugSomething isn't workingwindows-osIssues related to Codex on Windows systems

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions