Repository navigation
Close the ConPTY output pipe before the HPCON on cancellation - #35
Conversation
Before Windows 11 24H2, ClosePseudoConsole waits for the pseudoconsole to exit, and the pseudoconsole blocks on a full output pipe. The win32 cancellation cleanup closes the HPCON while the output pipe is still open and unread, so a child that keeps producing output can hang the teardown. This commit only adds the means to see that: - a windows-2022 job, which still has the waiting ClosePseudoConsole - timeout-minutes, because the hang blocks the event-loop thread and shows up as a job that never ends rather than a failed assertion - a test whose child floods a pty nobody reads, then cancels it The fix follows in the next commit, so this one is expected to time out on windows-2022. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The win32 cancellation cleanup called ClosePseudoConsole while our end of the output pipe was still open and unread. Before Windows 11 24H2 that call waits for the pseudoconsole to exit, and the pseudoconsole blocks on a full output pipe, so a child that keeps producing output could hang the teardown on the event-loop thread, before the grace-period kill. Close the output pipe first, as Microsoft's documentation requires. The group defer closing it a second time is harmless: RawFdStream::close is idempotent. The normal exit path is unchanged: the caller may still read up to EOF there, and EOF only arrives after ClosePseudoConsole. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c43c48954a
ℹ️ 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".
| // writing to it. Before Windows 11 24H2 a `ClosePseudoConsole` issued in | ||
| // that state never returns, and it runs on the event-loop thread, so no | ||
| // timeout can fire either: the failure mode of this test is the CI job's | ||
| // `timeout-minutes`, not an assertion. The bound covers the 5000ms grace |
There was a problem hiding this comment.
On the newly added windows-2022 CI leg, once this child fills the unread output pipe, cancellation still calls close_pseudo_console() in pty_win32.mbt before the group defer closes output_reader. As this comment notes, that blocks the event-loop thread on this Windows version, so the 15-second timeout cannot fire and the required test job instead fails at its 10-minute job timeout. Include the output-pipe-first cleanup change in this commit, or do not enable this known-failing test/runner yet.
Useful? React with 👍 / 👎.
When CreateProcessW fails, the cleanup closed both pipes and the HPCON and only then read GetLastError(). On the windows-2022 runner the bad-exe test then saw ERROR_ACCESS_DENIED instead of the CreateProcessW error; the closes are free to overwrite the thread's last-error value. Read it first, as the unix side already does with errno. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Raised by Codex on #34, and it predates that PR: the win32 cancellation cleanup calls
ClosePseudoConsolewhile our end of the output pipe is still open and nobody is reading it (the pipe is only closed later, by the task group's defer).Microsoft's documentation says the caller must either close the output pipe before
ClosePseudoConsoleor keep reading it until the call returns, and that before Windows 11 24H2 (build 26100) the call otherwise "may wait indefinitely". The call runs on the event-loop thread, so when it blocks nothing else runs: not the grace-period kill, not a timeout.with_pty(#34) makes this path the ordinary one — it is taken whenever the callback returns while the child is still running.Change
Three commits, pushed separately so each has its own CI run:
windows-2022job (older than 24H2, so it still has the waitingClosePseudoConsole),timeout-minutes: 10(jobs take about a minute; a deadlock hangs the test binary instead of failing it), and a test whose child floods a pty that nobody reads and is then cancelled.RawFdStream::closeis idempotent.docs/invariants.mdrecords the order and why.CreateProcessWfails, the cleanup closed both pipes and the HPCON and only then readGetLastError(). Onwindows-2022the closes overwrote it, and the existing bad-exe test sawERROR_ACCESS_DENIED. The error is now read first, as the unix side already does witherrno.CI evidence
windows-2022c43c489)Teststep hung for 9 minutes, cancelled bytimeout-minutes60119c1)bad1f84)windows-latestpasses at every commit, including the one without the fix, so it cannot catch a regression here; that is what thewindows-2022job is for.What is and is not verified
ClosePseudoConsole. A caller that does not drain the pty there can hit the same wait. Fixing that needsClosePseudoConsoleoff the event-loop thread and is out of scope here.🤖 Generated with Claude Code