Skip to content

Improve Windows process management edge cases#19211

Merged
iceweasel-oai merged 3 commits intomainfrom
codex/windows-process-management-fixes
Apr 29, 2026
Merged

Improve Windows process management edge cases#19211
iceweasel-oai merged 3 commits intomainfrom
codex/windows-process-management-fixes

Conversation

@iceweasel-oai
Copy link
Copy Markdown
Collaborator

@iceweasel-oai iceweasel-oai commented Apr 23, 2026

Summary

Some improvements to Windows process-management issues from #15578

  • bound the elevated runner pipe-connect handshake instead of waiting forever on blocking pipe connects
  • terminate the spawned runner if that handshake fails, so timeout/error paths do not leave a stray codex-command-runner.exe
  • loop on partial WriteFile results when forwarding stdin in the elevated runner, so input is not silently truncated
  • fix the concrete HANDLE/SID cleanup paths in the runner setup code
  • keep draining driver-backed stdout/stderr after exit until the backend closes, instead of dropping the tail after a fixed 200ms grace period
  • reuse LocalSid for SID ownership and add more explanatory comments around the ownership/concurrency-sensitive code paths

Why

The original PR fixed a lot of Windows session plumbing, but there were still a few sharp process-lifecycle edges:

  • some elevated runner handshakes could block forever
  • the new timeout path could still orphan the spawned runner process
  • stdin forwarding still assumed a single WriteFile consumed the whole buffer
  • a few raw HANDLE/SID error paths still leaked
  • driver-backed output could still lose the last chunk of stdout/stderr on slower backends

Validation

  • cargo fmt -p codex-windows-sandbox -p codex-utils-pty
  • cargo test -p codex-utils-pty
  • cargo test -p codex-windows-sandbox finish_driver_spawn
  • cargo test -p codex-windows-sandbox runner_

Ran a local test matrix of unified-exec and shell_tool tests, all passing

@iceweasel-oai iceweasel-oai marked this pull request as ready for review April 24, 2026 20:18
@iceweasel-oai iceweasel-oai changed the title [codex] Fix Windows process management edge cases Improve Windows process management edge cases Apr 24, 2026
@iceweasel-oai iceweasel-oai requested a review from jif-oai April 24, 2026 20:37
@iceweasel-oai
Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

Codex Review: Didn't find any major issues. Keep it up!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Collaborator

@jif-oai jif-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startup ownership boundary still feels split: spawn_runner_transport owns the runner process only through pipe connect, but send_spawn_request / read_spawn_ready are still part of startup and can fail or time out after the process handle is closed

Comment thread codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs
@iceweasel-oai iceweasel-oai force-pushed the codex/windows-process-management-fixes branch from 094c306 to 8341f86 Compare April 28, 2026 20:27
@iceweasel-oai iceweasel-oai merged commit cecca5a into main Apr 29, 2026
25 checks passed
@iceweasel-oai iceweasel-oai deleted the codex/windows-process-management-fixes branch April 29, 2026 17:00
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants