-
Notifications
You must be signed in to change notification settings - Fork 9.9k
bug: Windows PTY backend inverts TerminateProcess success/failure handling #13945
Description
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 → OkThis internal inconsistency within the same crate confirms the PTY backend has the logic inverted.
Impact
- Successful kills return
Err: callers believe termination failed when it succeeded. - Failed kills return
Ok: callers believe termination succeeded when it didn't → orphaned processes. - The bug is masked:
ChildKiller::kill()at line 80-83 callsself.do_kill().ok()and always returnsOk(()), 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.