Repository navigation
Terminal: write PTY input off the async runtime - #1006
Merged
Merged
Conversation
PtySession::write_input_by did a blocking write_all()+flush() on the PTY master fd inline on whatever thread called it. For MCP terminal_write that thread is a tokio runtime worker. portable_pty's writer fd is blocking, so a child that stops draining its stdin (a large paste, a canonical-mode line past the tty limit, a raw-mode TUI not reading input) makes the write block indefinitely. Observed on a live daemon: a terminal_write stalled the worker that owned tokio's multi-thread IO/time driver. A raw blocking syscall is invisible to tokio, so the driver was never handed off — every tokio::time timeout stopped firing and no IO was delivered, freezing the whole daemon (MCP tunnel, terminals, plugin, periodic sweeps) with all other workers parked and zero threads on kqueue. Recurred intermittently under concurrent terminal use. The reader was already handled correctly — reader_loop and wait_for_child run on dedicated std threads precisely because portable_pty's reads are blocking. The equally-blocking writer was the one left inline. This makes writes symmetric: a dedicated per-session writer thread owns the Box<dyn Write> and performs the blocking write_all; callers enqueue onto a bounded SyncSender and never block. A child that stalls its stdin now applies backpressure against the bounded queue (dropping input, with a warning) instead of wedging a runtime worker. The writer mutex is gone from the async path; the close-time EOF and the Windows DSR reply route through the same ordered channel.
lovon-spec
marked this pull request as ready for review
October 22, 2026 03:26
lovon-spec
enabled auto-merge
October 22, 2026 03:26
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PtySession::write_input_bydid a blockingwrite_all()+flush()on the PTY master fd inline on the calling thread. For the MCPterminal_writetool that thread is a tokio runtime worker. portable_pty's writer fd is blocking, so a child that stops draining its stdin (a large paste, a canonical-mode line past the tty limit, a raw-mode TUI not reading input) makes the write block indefinitely.Why it froze the whole daemon
Diagnosed on a live hung daemon (
sample(1)): aterminal_writestalled the worker that owned tokio's multi-thread IO/time driver. A raw blocking syscall is invisible to tokio, so the driver was never handed off — everytokio::timetimeout stopped firing, no IO was delivered, and the entire daemon (MCP tunnel, terminals, plugin, periodic sweeps) quiesced: all other workers parked on a condvar, zero threads on kqueue. Recurred intermittently under concurrent terminal use (several agents driving terminals through the tunnel).Full stack of the wedged worker:
The fix
The reader side was already correct —
reader_loop/wait_for_childrun on dedicatedstd::threads precisely because portable_pty's reads are blocking. The equally-blocking writer was the one left inline. This makes writes symmetric:Box<dyn Write>and performs the blockingwrite_all.SyncSender<Vec<u8>>and never block.writermutex is gone from the async path; the close-time EOF and the Windows DSR reply route through the same ordered channel.Notes
No new test: a faithful repro needs a real OS PTY and a child scheduled to not drain stdin — inherently timing/OS-dependent (the kind of widened-timeout test that's a smell here). The fix is non-blocking by construction and mirrors the existing reader design.
terminate()remains an unconditionalkill(), so the now-async close-time EOF changes nothing (it was already best-effort — the kill fired immediately after it in the old code too).