diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 1dd9e8ae..983d36e9 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -311,6 +311,29 @@ fn terminate_windows_job(job: Option<&WindowsJob>, child: &mut Child) -> std::io child.kill() } +#[cfg(windows)] +fn terminate_and_close_windows_job(windows_job: Option) { + if let Some(job) = windows_job.as_ref() + && let Err(err) = job.terminate() + { + tracing::warn!( + ?err, + "failed to terminate Windows shell job before closing job handle" + ); + } + drop(windows_job); +} + +#[cfg(windows)] +fn terminate_child_and_close_windows_job( + windows_job: Option, + child: &mut Child, +) -> std::io::Result<()> { + let result = terminate_windows_job(windows_job.as_ref(), child); + drop(windows_job); + result +} + #[cfg(windows)] fn attach_windows_job(child: &Child, command: &str) -> Option { match WindowsJob::attach_to_child(child) { @@ -485,16 +508,7 @@ impl BackgroundShell { let _ = kill_child_process_group(proc); } #[cfg(windows)] - if let Some(job) = self.windows_job.as_ref() { - let _ = job.terminate(); - } - #[cfg(windows)] - { - // Close the job handle before joining reader threads so - // JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE can still release inherited - // pipe handles if explicit termination failed. - self.windows_job = None; - } + terminate_and_close_windows_job(self.windows_job.take()); if let Some(handle) = self.stdout_thread.take() { let _ = handle.join(); } @@ -1039,9 +1053,7 @@ impl ShellManager { // Wait with timeout if let Some(status) = child.wait_timeout(timeout)? { #[cfg(windows)] - if let Some(job) = windows_job.as_ref() { - let _ = job.terminate(); - } + terminate_and_close_windows_job(windows_job); let stdout = stdout_thread.join().unwrap_or_default(); let stderr = stderr_thread.join().unwrap_or_default(); let stdout_str = String::from_utf8_lossy(&stdout).to_string(); @@ -1083,7 +1095,7 @@ impl ShellManager { #[cfg(unix)] let _ = kill_child_process_group(&mut child); #[cfg(windows)] - let _ = terminate_windows_job(windows_job.as_ref(), &mut child); + let _ = terminate_child_and_close_windows_job(windows_job, &mut child); #[cfg(all(not(unix), not(windows)))] let _ = child.kill(); let status = child.wait().ok(); @@ -1175,9 +1187,7 @@ impl ShellManager { if let Some(status) = child.wait_timeout(timeout)? { #[cfg(windows)] - if let Some(job) = windows_job.as_ref() { - let _ = job.terminate(); - } + terminate_and_close_windows_job(windows_job); Ok(ShellResult { task_id: None, status: if status.success() { @@ -1207,7 +1217,7 @@ impl ShellManager { #[cfg(unix)] let _ = kill_child_process_group(&mut child); #[cfg(windows)] - let _ = terminate_windows_job(windows_job.as_ref(), &mut child); + let _ = terminate_child_and_close_windows_job(windows_job, &mut child); #[cfg(all(not(unix), not(windows)))] let _ = child.kill(); let status = child.wait().ok(); diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index c52a5d06..f24923c1 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -1005,7 +1005,8 @@ fn windows_job_terminate_denied_falls_back_to_child_kill() { "limited job handle should not allow TerminateJobObject" ); - terminate_windows_job(Some(&limited_job), &mut child).expect("fallback child kill"); + terminate_child_and_close_windows_job(Some(limited_job), &mut child) + .expect("fallback child kill"); let status = child .wait_timeout(std::time::Duration::from_secs(3)) @@ -1016,6 +1017,54 @@ fn windows_job_terminate_denied_falls_back_to_child_kill() { ); } +#[cfg(windows)] +#[test] +fn windows_job_close_releases_foreground_reader_threads_when_terminate_denied() { + let mut child = Command::new("ping") + .args(["127.0.0.1", "-n", "8"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn ping"); + + let job = WindowsJob::attach_to_child(&child).expect("attach job"); + let limited_job = duplicate_job_without_terminate_access(job); + assert!( + limited_job.terminate().is_err(), + "limited job handle should not allow TerminateJobObject" + ); + + let stdout_handle = child.stdout.take().expect("stdout pipe"); + let stderr_handle = child.stderr.take().expect("stderr pipe"); + 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 + }); + 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 + }); + + let started = std::time::Instant::now(); + terminate_and_close_windows_job(Some(limited_job)); + let _ = stdout_thread.join().unwrap_or_default(); + let _ = stderr_thread.join().unwrap_or_default(); + let status = child + .wait_timeout(std::time::Duration::from_secs(3)) + .expect("wait after kill-on-close"); + + assert!( + started.elapsed() < std::time::Duration::from_secs(4), + "reader joins waited for natural descendant exit instead of kill-on-close" + ); + assert!(status.is_some(), "kill-on-close should terminate child"); +} + #[cfg(windows)] #[test] fn windows_job_kill_on_close_releases_reader_threads_when_terminate_denied() {