diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index b8183b5d3..095e4b79e 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -95,4 +95,4 @@ objc2 = "0.6.3" objc2-foundation = { version = "0.3.2", default-features = false, features = ["std", "NSArray", "NSDictionary", "NSError", "NSObject", "NSString", "NSURL"] } [target.'cfg(target_os = "windows")'.dependencies] -windows = { version = "0.60", features = ["Win32_Foundation", "Win32_System_Console", "Win32_UI_WindowsAndMessaging", "Win32_System_Diagnostics_Debug", "Win32_System_Threading"] } +windows = { version = "0.60", features = ["Win32_Foundation", "Win32_Security", "Win32_System_Console", "Win32_System_Diagnostics_Debug", "Win32_System_JobObjects", "Win32_System_Threading", "Win32_UI_WindowsAndMessaging"] } diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 2cfae1929..983d36e91 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -21,6 +21,18 @@ use wait_timeout::ChildExt; #[cfg(unix)] use std::os::unix::process::CommandExt; +#[cfg(windows)] +use std::os::windows::io::AsRawHandle; +#[cfg(windows)] +use windows::Win32::Foundation::{CloseHandle, HANDLE}; +#[cfg(windows)] +use windows::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation, + SetInformationJobObject, TerminateJobObject, +}; +#[cfg(windows)] +use windows::core::PCWSTR; use portable_pty::{CommandBuilder, PtySize, native_pty_system}; @@ -223,6 +235,120 @@ fn install_parent_death_signal(_cmd: &mut Command) { // leak children on those platforms — tracked as a follow-up. } +#[cfg(windows)] +#[derive(Debug)] +struct WindowsJob { + handle: HANDLE, +} + +#[cfg(windows)] +// SAFETY: Windows job handles are process-wide kernel handles. Moving the +// wrapper between threads does not invalidate the handle, and access is +// externally synchronized by ShellManager's mutex. +unsafe impl Send for WindowsJob {} +#[cfg(windows)] +// SAFETY: The wrapper exposes only terminate/drop operations around a kernel +// handle; concurrent use is guarded by ShellManager. +unsafe impl Sync for WindowsJob {} + +#[cfg(windows)] +impl WindowsJob { + fn attach_to_child(child: &Child) -> std::io::Result { + let handle = unsafe { CreateJobObjectW(None, PCWSTR::null()).map_err(windows_io_error)? }; + let job = Self { handle }; + + let mut limits = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default(); + limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + + unsafe { + SetInformationJobObject( + job.handle, + JobObjectExtendedLimitInformation, + &limits as *const _ as *const core::ffi::c_void, + std::mem::size_of::() as u32, + ) + .map_err(windows_io_error)?; + + let process_handle = HANDLE(child.as_raw_handle() as *mut core::ffi::c_void); + AssignProcessToJobObject(job.handle, process_handle).map_err(windows_io_error)?; + } + + Ok(job) + } + + fn terminate(&self) -> std::io::Result<()> { + unsafe { TerminateJobObject(self.handle, 1).map_err(windows_io_error) } + } +} + +#[cfg(windows)] +impl Drop for WindowsJob { + fn drop(&mut self) { + unsafe { + let _ = CloseHandle(self.handle); + } + } +} + +#[cfg(windows)] +fn windows_io_error(error: windows::core::Error) -> std::io::Error { + std::io::Error::other(error) +} + +#[cfg(windows)] +fn terminate_windows_job(job: Option<&WindowsJob>, child: &mut Child) -> std::io::Result<()> { + if let Some(job) = job { + match job.terminate() { + Ok(()) => return Ok(()), + Err(error) => { + tracing::warn!( + ?error, + "failed to terminate Windows job object; falling back to immediate child kill" + ); + } + } + } + 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) { + Ok(job) => Some(job), + Err(error) => { + tracing::warn!( + ?error, + command, + "failed to attach Windows shell process to job object; descendant cleanup degraded" + ); + None + } + } +} + #[derive(Clone, Copy, Debug)] struct ShellExitStatus { code: Option, @@ -333,6 +459,8 @@ pub struct BackgroundShell { stderr_cursor: usize, stdin: Option, child: Option, + #[cfg(windows)] + windows_job: Option, stdout_thread: Option>, stderr_thread: Option>, } @@ -379,6 +507,8 @@ impl BackgroundShell { if let Some(ShellChild::Process(ref mut proc)) = self.child { let _ = kill_child_process_group(proc); } + #[cfg(windows)] + terminate_and_close_windows_job(self.windows_job.take()); if let Some(handle) = self.stdout_thread.take() { let _ = handle.join(); } @@ -470,8 +600,22 @@ impl BackgroundShell { /// Kill the process fn kill(&mut self) -> Result<()> { if let Some(ref mut child) = self.child { - child.kill().context("Failed to kill process")?; - let _ = child.wait(); + if let ShellChild::Process(proc) = child { + #[cfg(windows)] + { + terminate_windows_job(self.windows_job.as_ref(), proc) + .context("Failed to kill process tree")?; + let _ = proc.wait(); + } + #[cfg(not(windows))] + { + proc.kill().context("Failed to kill process")?; + let _ = proc.wait(); + } + } else { + child.kill().context("Failed to kill process")?; + let _ = child.wait(); + } } self.status = ShellStatus::Killed; self.collect_output(); @@ -553,6 +697,13 @@ impl Drop for BackgroundShell { if self.status == ShellStatus::Running && let Some(ref mut child) = self.child { + #[cfg(windows)] + if let ShellChild::Process(proc) = child { + let _ = terminate_windows_job(self.windows_job.as_ref(), proc); + } else { + let _ = child.kill(); + } + #[cfg(not(windows))] let _ = child.kill(); let _ = child.wait(); } @@ -869,6 +1020,8 @@ impl ShellManager { let mut child = cmd .spawn() .with_context(|| format!("Failed to execute: {original_command}"))?; + #[cfg(windows)] + let windows_job = attach_windows_job(&child, original_command); if let Some(input) = stdin_data && let Some(mut stdin) = child.stdin.take() @@ -899,6 +1052,8 @@ impl ShellManager { // Wait with timeout if let Some(status) = child.wait_timeout(timeout)? { + #[cfg(windows)] + 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(); @@ -939,7 +1094,9 @@ impl ShellManager { // Timeout - kill the process #[cfg(unix)] let _ = kill_child_process_group(&mut child); - #[cfg(not(unix))] + #[cfg(windows)] + 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(); let stdout = stdout_thread.join().unwrap_or_default(); @@ -1025,8 +1182,12 @@ impl ShellManager { let mut child = cmd .spawn() .with_context(|| format!("Failed to execute: {original_command}"))?; + #[cfg(windows)] + let windows_job = attach_windows_job(&child, original_command); if let Some(status) = child.wait_timeout(timeout)? { + #[cfg(windows)] + terminate_and_close_windows_job(windows_job); Ok(ShellResult { task_id: None, status: if status.success() { @@ -1055,7 +1216,9 @@ impl ShellManager { } else { #[cfg(unix)] let _ = kill_child_process_group(&mut child); - #[cfg(not(unix))] + #[cfg(windows)] + 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(); @@ -1108,6 +1271,9 @@ impl ShellManager { Some(Arc::new(Mutex::new(Vec::new()))) }; + #[cfg(windows)] + let mut windows_job = None; + let (child, stdin, stdout_thread, stderr_thread) = if tty { let pty_system = native_pty_system(); let pair = pty_system @@ -1165,6 +1331,10 @@ impl ShellManager { let mut child = cmd .spawn() .with_context(|| format!("Failed to spawn background: {original_command}"))?; + #[cfg(windows)] + { + windows_job = attach_windows_job(&child, original_command); + } let stdout_handle = child.stdout.take().context("Failed to capture stdout")?; let stderr_handle = child.stderr.take().context("Failed to capture stderr")?; @@ -1201,6 +1371,8 @@ impl ShellManager { stderr_cursor: 0, stdin, child: Some(child), + #[cfg(windows)] + windows_job, stdout_thread, stderr_thread, }; diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 9bfb9d7de..f24923c1d 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -4,6 +4,11 @@ use crate::tools::spec::ToolContext; use serde_json::{Value, json}; use tempfile::tempdir; +#[cfg(windows)] +use windows::Win32::Foundation::{DUPLICATE_HANDLE_OPTIONS, DuplicateHandle, HANDLE}; +#[cfg(windows)] +use windows::Win32::System::Threading::GetCurrentProcess; + // `env_lock` exists only to serialize Unix-only env-mutating tests. // Windows builds gate that test out, so the helper would be dead code // under `-Dwarnings` if the import + helper were unconditional. @@ -16,6 +21,33 @@ fn env_lock() -> &'static Mutex<()> { LOCK.get_or_init(|| Mutex::new(())) } +#[cfg(windows)] +const JOB_OBJECT_QUERY_ACCESS: u32 = 0x0004; + +#[cfg(windows)] +fn duplicate_job_without_terminate_access(job: WindowsJob) -> WindowsJob { + let process = unsafe { GetCurrentProcess() }; + let mut limited_handle = HANDLE::default(); + + unsafe { + DuplicateHandle( + process, + job.handle, + process, + &mut limited_handle, + JOB_OBJECT_QUERY_ACCESS, + false, + DUPLICATE_HANDLE_OPTIONS(0), + ) + .expect("duplicate job handle without terminate access"); + } + + drop(job); + WindowsJob { + handle: limited_handle, + } +} + fn echo_command(message: &str) -> String { format!("echo {message}") } @@ -922,6 +954,159 @@ fn test_orphaned_subprocess_does_not_block_collect_output() { assert_eq!(done.status, ShellStatus::Completed); } +// Windows equivalent of the orphaned pipe-handle regression. `cmd /c start /b` +// launches a descendant process that inherits stdout/stderr and outlives the +// shell. Job-object cleanup must terminate that descendant before reader-thread +// joins, otherwise get_output() blocks until ping exits. +#[cfg(windows)] +#[test] +fn background_collection_does_not_block_on_detached_descendant_pipe() { + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + + let result = manager + .execute( + r#"cmd /c start "" /b ping 127.0.0.1 -n 4"#, + None, + 5000, + true, + ) + .expect("execute"); + let task_id = result.task_id.expect("task id"); + + let started = std::time::Instant::now(); + let done = manager + .get_output(&task_id, true, 3000) + .expect("get_output must complete, not hang"); + + assert!( + started.elapsed() < std::time::Duration::from_secs(6), + "get_output blocked on descendant pipe handles" + ); + assert_eq!(done.status, ShellStatus::Completed); +} + +#[cfg(windows)] +#[test] +fn windows_job_terminate_denied_falls_back_to_child_kill() { + let mut child = Command::new("ping") + .args(["127.0.0.1", "-n", "20"]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .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" + ); + + 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)) + .expect("wait after fallback kill"); + assert!( + status.is_some(), + "fallback child kill should terminate child" + ); +} + +#[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() { + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + + let result = manager + .execute( + r#"cmd /c start "" /b ping 127.0.0.1 -n 8"#, + None, + 5000, + true, + ) + .expect("execute"); + let task_id = result.task_id.expect("task id"); + + { + let shell = manager + .processes + .get_mut(&task_id) + .expect("background shell"); + let job = shell.windows_job.take().expect("windows job attached"); + let limited_job = duplicate_job_without_terminate_access(job); + assert!( + limited_job.terminate().is_err(), + "limited job handle should not allow TerminateJobObject" + ); + shell.windows_job = Some(limited_job); + } + + let started = std::time::Instant::now(); + let done = manager + .get_output(&task_id, true, 3000) + .expect("get_output must complete via kill-on-close fallback"); + + assert!( + started.elapsed() < std::time::Duration::from_secs(4), + "get_output waited for natural descendant exit instead of kill-on-close" + ); + assert_eq!(done.status, ShellStatus::Completed); +} + #[test] fn test_list_jobs_cleans_up_completed_old_processes() { let tmp = tempdir().expect("tempdir");