feat(doctor): add opt-in piped stdin for streaming fix execution - #921
Open
matt2e wants to merge 5 commits into
Open
feat(doctor): add opt-in piped stdin for streaming fix execution#921matt2e wants to merge 5 commits into
matt2e wants to merge 5 commits into
Conversation
Fix commands run through the streaming executor always inherited the host process's stdin. In a GUI host that stdin is never writable, so an interactive fix like `claude-agent-acp --cli auth login` — which prints an OAuth URL and then blocks reading the auth code — hangs forever (block/berd#99). Add an opt-in pipe the host can feed: - New public types `FixStdin` / `FixStdinWriter`: `FixStdin::pipe()` returns a cloneable line writer (`send_line`, which appends `\n` and flushes) plus the `FixStdin` to place in the options. Lines sent before spawn are buffered; dropping every writer delivers EOF. - `ExecuteFixOptions` gains `pub stdin: Option<FixStdin>` and a `with_stdin` builder; the existing `Debug`/`Clone`/`Default` derives are preserved via an `Arc<Mutex<Option<Receiver>>>` around the non-cloneable channel receiver. - The option threads through `execute_fix_streaming_with_env_options` → `run_command_streaming` → `run_command_streaming_blocking`, which only then sets `Stdio::piped()` on stdin and feeds the child from a detached writer thread. The thread is deliberately never joined — a writer outliving the child would park it in `rx.iter()` and hang the fix; it exits on channel close or on the post-exit EPIPE write error. - `stdin: None` keeps today's inherited-stdin behavior byte-for-byte, so terminal hosts with legitimately interactive fixes are untouched. Tests: cat echo round-trip (write + EOF + pre-spawn buffering), `read -r` prompt shape (the paste-an-auth-code flow), and no-hang-when-writer-outlives-child including post-exit `send_line` erroring instead of panicking. Existing streaming tests cover the default path. Verified with `cargo test` in crates/doctor (118 passed) plus `cargo fmt` and `cargo clippy --all-targets`. Signed-off-by: Matt Toohey <contact@matttoohey.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: d01aaa7818
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+1117
to
+1118
| std::thread::spawn(move || { | ||
| for line in stdin_rx.iter() { |
There was a problem hiding this comment.
When the subprocess exits while the caller retains a FixStdinWriter, this detached thread remains blocked in stdin_rx.iter() and therefore keeps the receiver open. A subsequent send_line after the execution future has completed returns Ok; only that queued write discovers EPIPE and eventually closes the channel, contrary to the public promise that sending after completion returns Err. It also leaves one parked OS thread per completed fix until the writer is dropped or another line is sent. Coordinate child completion with this worker so the receiver is closed as soon as the process exits.
Useful? React with 👍 / 👎.
`ExecuteFixOptions` and `FixStdin` are both `Clone`, so a host that caches its options struct and retries a failed login got a second child whose stdin was EOF'd at spawn, while `send_line` calls either vanished or landed in the *first* child's stdin. The retry hung or died with nothing in the output explaining why — block/berd#99 re-created by the mechanism meant to fix it. Claim the receiver before anything is launched and turn the already-consumed case into an error: - `run_command_streaming_blocking` hoists `take_receiver` to the top of the body, ahead of `build_shell_command`, and maps `None` to "FixStdin already consumed by a previous fix execution". Pre-spawn matters beyond tidiness: returning `Err` after `spawn` would drop the `Child`, and `std::process::Child`'s `Drop` neither kills nor reaps, so the doomed login subprocess would keep running and become a zombie. - `Stdio::piped()` now keys off the claimed receiver, and the post-spawn block collapses to the writer thread — no `match`, no `drop(child_stdin)` EOF arm. - `stdin: None` is untouched byte-for-byte; terminal hosts with legitimately interactive fixes keep inheriting stdin. A spawn failure now consumes the receiver even though no child got the pipe, so a retry with the same cached options reports the reuse rather than the underlying spawn error. That is strictly better than a hang, and a `restore_receiver` escape hatch would add a branch no test in this crate can exercise (the shell path isn't injectable). Docs for `FixStdin`, `ExecuteFixOptions::stdin`, `with_stdin`, and `take_receiver` are corrected to the single-use contract they now have. Tests: new reuse case asserts the second run errors naming the reuse and that nothing reaches `on_line`, which pins the pre-spawn ordering — `run_command_streaming` emits no preamble of its own, so any output would mean the child ran. Verified with `cargo test` in crates/doctor (119 passed) plus `cargo fmt`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri, which is workspace-excluded and so never built by crate-local commands. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`run_command_streaming_blocking` could park forever in two places, and the two were entangled: the detached stdin writer thread's only "am I done?" signal is the main loop finishing, and the main loop can't finish while the child is alive. Fixing either alone leaves the other in place. Reclaim the writer thread: - The thread polls `recv_timeout(STDIN_WRITER_POLL_INTERVAL)` (250 ms) against a shared `AtomicBool` instead of parking in `stdin_rx.iter()`, so a host that holds its `FixStdinWriter` and simply stops sending no longer leaks an OS thread and a `ChildStdin` fd per fix run. - A `FixFinishedFlag` drop guard sets that flag on every exit path — including the `wait` error return and a panic in `on_line` — so worst-case thread lifetime is "fix completion + one poll interval" regardless of caller discipline. Breaking out still drops `child_stdin`, delivering EOF. - Dropping the receiver on the way out makes `send_line` fail deterministically from then on, which lets the writer-outlives-child test replace its 100 x 10 ms polling loop with a fixed wait. Give the fix itself a deadline: - New `FixTimeout` (`Standard` / `After` / `Unbounded`) plus `DEFAULT_FIX_TIMEOUT = 600s`, added to `ExecuteFixOptions` with a `with_timeout` builder and threaded through `execute_fix_streaming_with_env_options` -> `run_command_streaming` -> `run_command_streaming_blocking` exactly as `stdin` was. An enum rather than `Option<Duration>` because `None` reads as both "use the default" and "no timeout"; `Unbounded` also keeps the plain `rx.recv()` path, sidestepping the `recv_timeout(Duration::MAX)` instant-overflow hazard. - 10 minutes clears a cold-cache `npm install -g` behind the corporate proxy and a human doing SSO in a browser. The 10s/15s probe timeouts in `command.rs` are wildly wrong for this path. An idle timeout would suit installs better but is precisely wrong for `auth login`, which prints its URL and then goes deliberately silent; `FixTimeout` can gain `Idle` later. - Enforcement replaces `rx.iter()` with `recv_timeout` against the deadline and `child.wait()` with `wait_timeout` — a process can close both pipes and keep running. On expiry: drain queued lines with `try_recv` so the last real output survives, emit one greppable notice through `on_line`, kill, reap, and return `Err`. The reader threads are deliberately *not* joined, because a descendant that escaped the process group can hold the inherited stdout open indefinitely (`command.rs`'s module docs; the escaped-descendant test). - `kill_child_process_group_or_child` is promoted to `pub(crate)`, and the child gets `process_group(0)` only when `stdin.is_some()`. Group-kill is available precisely when doctor owns the child's stdin and so the child can't touch the tty; setting it unconditionally would give a terminal host's tty-reading fix a SIGTTIN stop. `stdin: None` stays byte-for-byte unchanged. The in-crate `ExecuteFixOptions` literals switch to `..Default::default()` so the next option field is genuinely additive. Tests: `After(100ms)` vs `sleep 60` returns `Err` naming the timeout and the command in well under a second, with the notice line visible to `on_line`; the group kill takes a backgrounded grandchild with it (verified non-vacuous by disabling `process_group(0)`, which makes it fail); the timeout returns promptly even when a `setsid`-escaped descendant holds the pipes open; the writer thread has retired a few poll intervals after the fix returns; and a guard test pins `DEFAULT_FIX_TIMEOUT` at 600s, `FixTimeout::Standard` as the `ExecuteFixOptions` default, and the default at >= 30x probe scale. Verified with `cargo test` in crates/doctor (123 passed) plus `cargo fmt` and `cargo clippy --all-targets`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
… fields `apps/staged/src-tauri` is in the workspace `exclude` list, so `cargo check` under `crates/` never compiles it — while `staged-ci.yml` triggers on `crates/**`. Its exhaustive `ExecuteFixOptions` literal therefore turns every new doctor option into a CI-only breakage, twice now: `stdin`, then `timeout`. Switch it to `..Default::default()`. Staged's fixes are non-interactive, so the defaults are what it wanted anyway — inherited stdin and the standard 600s fix timeout, both far more than any install or login this runs needs. Verified with `cargo check` from apps/staged/src-tauri. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…livered
`send_line` was documented as returning `Err` "when the fix has already
finished (its stdin pipe is closed)" and could not honour it: it returned
`mpsc::Sender::send`'s result, which fails only once the receiver drops, and
the receiver lived in the detached writer thread. So the first post-exit
`send_line` — plus anything queued behind it — returned `Ok` and was then
discarded when the thread's write hit EPIPE. For berd#99 that is the
difference between an error and a hang: the login subprocess dies, the user
pastes the auth code a beat later, `send_line` says `Ok`, the code goes
nowhere, and a host treating `Ok` as delivery waits forever with nothing in
the log to explain it.
Replace the channel and the thread with a three-state pipe shared behind the
`Arc<Mutex<…>>` `FixStdin` already carried, written inline by the caller:
- `FixStdinState` is `Buffered { lines, eof, claimed }` -> `Live(ChildStdin)`
-> `Closed`. `send_line` queues on `Buffered`, writes through on `Live` and
reports the real `io::Error`, and fails fast on `Closed` — which is latched,
so a dead pipe is discovered once. `ChildStdin: Debug`, so the public
`Debug`/`Clone` derives on `FixStdin` and `ExecuteFixOptions` survive.
- EOF on last-writer-drop, previously free from the mpsc disconnect, comes
from a `Drop` on a `FixStdinWriterInner` behind an `Arc`, keeping
`FixStdinWriter: Clone`. The `Buffered { eof: true }` arm is load-bearing:
a host may queue a line, drop the writer, and only then start the fix, and
those lines must still be replayed before the pipe closes.
- The runner claims pre-spawn exactly as before (`claim` replaces
`take_receiver`), then `attach`es the child's stdin *after* the reader
threads start — the replay writes inline on the runner thread, so a queue
bigger than the pipe buffer would otherwise deadlock against a child whose
output nobody is draining.
- `FixStdinCloser` replaces `FixFinishedFlag`, closing the pipe on every exit
path (return, error, timeout, spawn failure, a panic in `on_line`). EPIPE
cannot carry this alone: a probe confirms that with the runner's
`zsh -l -c` shape a backgrounded grandchild inherits stdin and keeps the
read end open, so writes into a finished fix's pipe still succeed.
Two review comments dissolve rather than getting documented around: there is
no detached thread to leak an OS thread and a `ChildStdin` fd for, so
`STDIN_WRITER_POLL_INTERVAL` and its worst-case-lag caveat are both gone, and
the fd is now reclaimed at fix completion instead of one poll interval later.
Public API is unchanged — `FixStdin::pipe`, `send_line`, `ExecuteFixOptions::
stdin`/`with_stdin` keep their signatures, and `stdin: None` is byte-for-byte
untouched. One new semantic: `send_line` performs blocking I/O under a mutex
on the calling thread. For a one-line auth code against a ~64KB pipe buffer
that is instantaneous, but the doc now says it can block if the fix isn't
reading, and a host sending anything bulkier should keep it off its async
runtime. Docs also state the hazard that predates and survives this change:
a fix reading *to EOF* won't exit until every writer clone drops.
Tests: the writer-outlives-child test drops its 100x10ms poll loop and
asserts the *first* post-fix `send_line` is `Err` naming closed input; a new
grandchild test pins the explicit close (verified non-vacuous — disabling
`close()` fails it, while the no-grandchild case passes on EPIPE alone); the
prompt-style test now sends from inside `on_line`, on the fix's own thread in
response to the fix's own prompt, so its `Ok` is the delivery guarantee
rather than the pre-spawn queueing one; the `cat` round-trip test is
unchanged as the `Buffered { eof: true }` regression test, and was confirmed
to hang without that arm. Verified with `cargo test` in crates/doctor (124
passed) plus `cargo fmt`, `cargo clippy --all-targets`, and `cargo check` in
apps/staged/src-tauri.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
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
1 participant
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.
Summary
Adds opt-in piped stdin to the doctor crate's streaming fix execution, so a host can feed input (e.g. pasting an auth code into a login flow) to a running fix subprocess.
Changes
FixStdin::pipe()returns a connected(FixStdinWriter, FixStdin)pair. The caller keeps the cloneable writer and puts theFixStdininExecuteFixOptions::stdin(or uses.with_stdin(..)).FixStdinWriter::send_linequeues a line (trailing newline appended, pipe flushed) and returnsErronce the fix's stdin is closed. Lines sent before the child spawns are buffered and delivered on spawn; dropping every writer clone closes the child's stdin (EOF).stdin: Nonethe child keeps inheriting the host process's stdin, so interactive fixes in terminal hosts are unchanged.execute_fix_optionsfor the new field; its fixes are non-interactive, so they keep inherited stdin.Testing
Three new tests cover the round trip through
cat(including pre-spawn buffering and EOF-on-drop), the prompt-styleread -rshape fed while the fix runs, and the no-hang path when a writer outlives the child. Fulljust app staged ci(739 Rust + 669 frontend tests) and the crates fmt/lint/test suites pass.