Repository navigation
Add with_pty for ptys scoped to a callback - #34
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1987463b15
ℹ️ 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".
| extra_env?, | ||
| inherit_env?, | ||
| cwd?, | ||
| no_wait=true, |
There was a problem hiding this comment.
Close the ConPTY output before cancelling the child
On Windows versions before 11 24H2, this no_wait path can hang indefinitely when f returns while the child is still producing output. The cancellation branch in pty_win32.mbt synchronously calls ClosePseudoConsole before the output reader is closed, while the reader is only closed by the task-group defer; Microsoft documents that older ClosePseudoConsole waits and can deadlock if its output pipe is neither closed nor drained. Thus a callback that does not consume all output—the exact full-queue case this wrapper is intended to handle—can prevent with_pty from ever reaching its grace-period kill or returning. Close the output pipe before closing the HPCON, or keep it draining concurrently during teardown. Microsoft's ClosePseudoConsole documentation.
AGENTS.md reference: AGENTS.md:L27-L31
Useful? React with 👍 / 👎.
`spawn` keeps the pty open until its task group exits, so a long-lived group that spawns many ptys accumulates their file descriptors (50 finished ptys in one group held 50 descriptors until the group exited). `with_pty` runs a callback with a pty in a task group of its own and releases the pty when the callback is done. It spawns with no_wait=true: once the callback is done nobody reads the terminal, so a child that is still running goes through the usual cancellation cleanup instead of being waited for. It only calls `spawn`, so it lives in the shared pty.mbt and is type-checked on both platforms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The docs told callers who need tasks reading and writing the pty concurrently to open a task group inside the with_pty callback. That is one group more than needed: such a session already has a group with the pty's lifetime, so the pty should be spawned into it directly. Docs only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
178e4e6 to
d1389f5
Compare
… form with_pty is the form that cannot leak a pty into a long-lived group, so it is the one to show first. spawn moves to its own section, framed by when it is the right call: a task group with the pty's lifetime already exists. Both examples were run as a temporary test on macOS before committing. Docs only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be73de1b95
ℹ️ 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".
| extra_env?, | ||
| inherit_env?, | ||
| cwd?, | ||
| no_wait=true, |
There was a problem hiding this comment.
Handle the ConPTY normal-exit race before cancellation
On Windows before 11 24H2, if f leaves the output unread and a finite high-output child exits before this no_wait task is cancelled, pty_win32.mbt:193-195 takes the normal Some(status) branch and its defer calls ClosePseudoConsole while the full output pipe remains open. That synchronous call can hang the event-loop thread indefinitely, so with_pty never returns despite its cleanup guarantee. This is distinct from the earlier comment: the parent fixed the still-running cancellation branch, while the fresh evidence here is the already-exited normal branch. Teardown after f must close the output reader before the HPCON even when the waiter has already observed process exit.
AGENTS.md reference: AGENTS.md:L27-L31
Useful? React with 👍 / 👎.
The rest of the repo writes block callbacks as `f(args) <| x => { ... }`;
the README examples and the block-style with_pty call sites in the tests
now do too. One-line callbacks passed inside an assertion stay as plain
arguments.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
spawnties the pty to its task group: the terminal's file descriptors stay open until the group exits, even after the child has exited and been waited for. Measured on macOS, 50 finished ptys in one group held 50 descriptors until the group exited. A long-lived group (a terminal server, say) accumulates them without bound.Change
with_ptytakesspawn's arguments minusgroupandno_wait, runs the callback in a task group of its own, and releases the pty when the callback is done.no_wait=true. Once the callback is done nobody reads the terminal, so waiting for a child that is still running could block forever on a full pty queue; instead it goes through the existing cancellation cleanup (hang up, 5 s grace period, kill) andwith_ptyreturns once the child is reaped.spawn, so it lives in the sharedpty.mbtwith no#cfgand is type-checked on both platforms.spawnis unchanged and stays: it is whatwith_ptyis built on, and it is still the right call when the pty must outlive its creator or share an existing group's lifetime..mbtigains one line (with_pty); nothing else changes.Limitation, documented
A
Ptythat escapes the callback is closed, and a task still blocked reading it is not woken up (moonbitlang/async's close detaches the fd without waking waiters). The doc comment and README say to keep concurrent readers in a task group opened inside the callback.Tests
Unix (run locally on macOS, 26 passed, three runs):
pty.wait()inside the callback yields the exit codewith_ptyreturnsresizeraisesEBADF)spawnin one shared group climbs by 50)Windows (not run locally; verified by CI only):
The callback returning while the child is still running takes the win32 cancellation path. That path is covered since #35 by the flooding-child test on
spawn(no_wait=true), which is exactly the callwith_ptymakes, and #35 fixed the pre-24H2ClosePseudoConsolehang Codex flagged here. There is nowith_pty-specific test for it on Windows.Rebased onto main after #35.
🤖 Generated with Claude Code