Skip to content

fix: Prevent exec_shell timeout deadlock on Windows #2573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
idling11 wants to merge 2 commits into from
+29 −10
Closed

fix: Prevent exec_shell timeout deadlock on Windows #2573

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
942a778
fix: prevent exec_shell timeout deadlock on Windows by bounding reade…
idling11 Jun 2, 2026
73bc393
fix: apply recv_timeout to success path too, use // comment, document…
idling11 Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions crates/tui/src/tools/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,25 +882,40 @@ impl ShellManager {
let stdout_handle = child.stdout.take().context("Failed to capture stdout")?;
let stderr_handle = child.stderr.take().context("Failed to capture stderr")?;

// Spawn threads to read output
let stdout_thread = std::thread::spawn(move || {
// Spawn threads to read output, using channels so the main thread
// can apply a bounded join timeout on Windows where killed-process
// pipes may not close promptly (#2571).
let (stdout_tx, stdout_rx) = std::sync::mpsc::channel();
let (stderr_tx, stderr_rx) = std::sync::mpsc::channel();
let _stdout_thread = std::thread::spawn(move || {
let mut reader = stdout_handle;
let mut buf = Vec::new();
let _ = reader.read_to_end(&mut buf);
buf
stdout_tx.send(buf).ok();
});

let stderr_thread = std::thread::spawn(move || {
let _stderr_thread = std::thread::spawn(move || {
let mut reader = stderr_handle;
let mut buf = Vec::new();
let _ = reader.read_to_end(&mut buf);
buf
stderr_tx.send(buf).ok();
});

// Upper bound on how long we wait for reader threads after the
// process has been killed (or after the process exited but a
// grandchild still holds a pipe handle). Prevents a hung
// read_to_end from deadlocking the global tool_exec_lock.
// Two sequential recv_timeout calls cap worst-case extra latency
// at 2 × READER_JOIN_TIMEOUT (10 s).
const READER_JOIN_TIMEOUT: Duration = Duration::from_secs(5);

// Wait with timeout
if let Some(status) = child.wait_timeout(timeout)? {
let stdout = stdout_thread.join().unwrap_or_default();
let stderr = stderr_thread.join().unwrap_or_default();
let stdout = stdout_rx
.recv_timeout(READER_JOIN_TIMEOUT)
.unwrap_or_default();
let stderr = stderr_rx
.recv_timeout(READER_JOIN_TIMEOUT)
.unwrap_or_default();
Comment on lines 912 to +918
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Jun 2, 2026

Choose a reason for hiding this comment

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

P1 Silent output loss on the success path

When a process exits cleanly but a grandchild holds onto the pipe write-handle (common in shell scripts that launch background jobs), read_to_end in the reader thread never returns, recv_timeout expires after 5 s, and unwrap_or_default() silently substitutes an empty Vec<u8>. The caller receives ShellStatus::Completed with empty stdout/stderr — indistinguishable from a command that genuinely produced no output. Unlike the timeout branch (which sets ShellStatus::TimedOut), there is no field in ShellResult to signal that output was discarded, so the data loss is invisible to both the tool layer and the end user.

Fix in Codex Fix in Claude Code Fix in Cursor

let stdout_str = String::from_utf8_lossy(&stdout).to_string();
let stderr_str = String::from_utf8_lossy(&stderr).to_string();
let exit_code = status.code().unwrap_or(-1);
Expand Down Expand Up @@ -942,8 +957,12 @@ impl ShellManager {
#[cfg(not(unix))]
let _ = child.kill();
let status = child.wait().ok();
let stdout = stdout_thread.join().unwrap_or_default();
let stderr = stderr_thread.join().unwrap_or_default();
let stdout = stdout_rx
.recv_timeout(READER_JOIN_TIMEOUT)
.unwrap_or_default();
let stderr = stderr_rx
.recv_timeout(READER_JOIN_TIMEOUT)
.unwrap_or_default();
Comment thread
greptile-apps[bot] marked this conversation as resolved.
let stdout_str = String::from_utf8_lossy(&stdout).to_string();
let stderr_str = String::from_utf8_lossy(&stderr).to_string();
let (stdout, stdout_meta) = truncate_with_meta(&stdout_str);
Expand Down
Loading