From c361cbcc0f13e9794cfe93752927d25534386631 Mon Sep 17 00:00:00 2001 From: Alberto Gimeno Date: Tue, 1 Sep 2026 17:46:57 +0200 Subject: [PATCH 1/5] Fix CLI process tree lifecycle Contain SDK-spawned CLI descendants in Unix process groups and Windows Job Objects, and terminate the complete owned tree during shutdown and failure cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- rust/Cargo.lock | 2 + rust/Cargo.toml | 9 ++ rust/src/lib.rs | 246 +++++++++++++++++++++++++++++++++---- rust/src/process_tree.rs | 253 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 487 insertions(+), 23 deletions(-) create mode 100644 rust/src/process_tree.rs diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 8de6797989..b4445c0b63 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -434,6 +434,7 @@ dependencies = [ "getrandom 0.2.17", "http", "indexmap", + "libc", "libloading", "native-tls", "parking_lot", @@ -454,6 +455,7 @@ dependencies = [ "tracing", "ureq", "uuid", + "windows-sys 0.61.2", "zip", ] diff --git a/rust/Cargo.toml b/rust/Cargo.toml index a28190cd1f..08664aefaf 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -70,6 +70,15 @@ tokio-tungstenite = { version = "0.24", default-features = false, features = ["c [target.'cfg(windows)'.dependencies] zip = { version = "2", default-features = false, features = ["deflate"], optional = true } +windows-sys = { version = "0.61", default-features = false, features = [ + "Win32_Foundation", + "Win32_System_Diagnostics_ToolHelp", + "Win32_System_JobObjects", + "Win32_System_Threading", +] } + +[target.'cfg(unix)'.dependencies] +libc = "0.2" [dev-dependencies] rusqlite = { version = "0.35", features = ["bundled"] } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 1c20fd1837..7875bb02df 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -33,6 +33,7 @@ pub mod hooks; mod jsonrpc; /// Permission-policy helpers that produce a [`handler::PermissionHandler`]. pub mod permission; +mod process_tree; /// BYOK bearer-token provider callbacks. pub mod provider_token; mod provider_token_dispatch; @@ -1066,6 +1067,7 @@ impl std::fmt::Debug for Client { struct ClientInner { child: parking_lot::Mutex>, + process_tree: parking_lot::Mutex>, #[cfg(feature = "bundled-in-process")] /// In-process FFI runtime host, set only for [`Transport::InProcess`]. /// Closing it tears down the native runtime connection. @@ -1302,6 +1304,7 @@ impl Client { reader, writer, None, + None, working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1317,7 +1320,7 @@ impl Client { port, connection_token: _, } => { - let (mut child, actual_port, spawn_elapsed, port_wait_elapsed) = + let (mut child, tree, actual_port, spawn_elapsed, port_wait_elapsed) = Self::spawn_tcp(&program, &options, &working_directory, port).await?; timings.process_spawn_ms = Some(StartupTimings::millis(spawn_elapsed)); timings.port_wait_ms = Some(StartupTimings::millis(port_wait_elapsed)); @@ -1334,6 +1337,7 @@ impl Client { reader, writer, Some(child), + Some(tree), working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1346,7 +1350,7 @@ impl Client { )? } Transport::Stdio => { - let (mut child, spawn_elapsed) = + let (mut child, tree, spawn_elapsed) = Self::spawn_stdio(&program, &options, &working_directory)?; timings.process_spawn_ms = Some(StartupTimings::millis(spawn_elapsed)); let stdin = child.stdin.take().expect("stdin is piped"); @@ -1356,6 +1360,7 @@ impl Client { stdout, stdin, Some(child), + Some(tree), working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1422,6 +1427,7 @@ impl Client { reader, writer, None, + None, working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1563,6 +1569,7 @@ impl Client { reader, writer, None, + None, cwd, None, None, @@ -1589,6 +1596,7 @@ impl Client { reader, writer, None, + None, cwd, None, Some(provider), @@ -1619,6 +1627,7 @@ impl Client { reader, writer, None, + None, cwd, None, None, @@ -1645,6 +1654,7 @@ impl Client { reader, writer, None, + None, cwd, None, None, @@ -1671,6 +1681,7 @@ impl Client { reader, writer, None, + None, cwd, None, None, @@ -1698,6 +1709,7 @@ impl Client { reader: impl AsyncRead + Unpin + Send + 'static, writer: impl AsyncWrite + Unpin + Send + 'static, child: Option, + process_tree: Option, cwd: PathBuf, on_list_models: Option>, extension_launch_provider: Option< @@ -1732,6 +1744,7 @@ impl Client { let client = Self { inner: Arc::new(ClientInner { child: parking_lot::Mutex::new(child), + process_tree: parking_lot::Mutex::new(process_tree), #[cfg(feature = "bundled-in-process")] ffi_host: parking_lot::Mutex::new(None), rpc, @@ -1870,13 +1883,6 @@ impl Client { .stdout(Stdio::piped()) .stderr(Stdio::piped()); - #[cfg(windows)] - { - use std::os::windows::process::CommandExt; - const CREATE_NO_WINDOW: u32 = 0x08000000; - command.as_std_mut().creation_flags(CREATE_NO_WINDOW); - } - command } @@ -1933,7 +1939,7 @@ impl Client { program: &Path, options: &ClientOptions, working_directory: &Path, - ) -> Result<(Child, Duration)> { + ) -> Result<(Child, process_tree::ProcessTree, Duration)> { info!(cwd = ?working_directory, program = %program.display(), "spawning copilot CLI (stdio)"); let mut command = Self::build_command(program, options, working_directory); command @@ -1945,13 +1951,13 @@ impl Client { .args(&options.extra_args) .stdin(Stdio::piped()); let spawn_start = Instant::now(); - let child = command.spawn()?; + let (child, tree) = process_tree::spawn(&mut command)?; let spawn_elapsed = spawn_start.elapsed(); debug!( elapsed_ms = spawn_elapsed.as_millis(), "Client::spawn_stdio subprocess spawned" ); - Ok((child, spawn_elapsed)) + Ok((child, tree, spawn_elapsed)) } async fn spawn_tcp( @@ -1959,7 +1965,7 @@ impl Client { options: &ClientOptions, working_directory: &Path, port: u16, - ) -> Result<(Child, u16, Duration, Duration)> { + ) -> Result<(Child, process_tree::ProcessTree, u16, Duration, Duration)> { info!(cwd = ?working_directory, program = %program.display(), port = %port, "spawning copilot CLI (tcp)"); let mut command = Self::build_command(program, options, working_directory); command @@ -1971,7 +1977,7 @@ impl Client { .args(&options.extra_args) .stdin(Stdio::null()); let spawn_start = Instant::now(); - let mut child = command.spawn()?; + let (mut child, tree) = process_tree::spawn(&mut command)?; let spawn_elapsed = spawn_start.elapsed(); debug!( elapsed_ms = spawn_elapsed.as_millis(), @@ -2018,7 +2024,7 @@ impl Client { "Client::spawn_tcp TCP port wait complete" ); info!(port = %actual_port, "CLI server listening"); - Ok((child, actual_port, spawn_elapsed, port_wait_elapsed)) + Ok((child, tree, actual_port, spawn_elapsed, port_wait_elapsed)) } fn drain_stderr(child: &mut Child) { @@ -2561,9 +2567,9 @@ impl Client { /// Cooperatively shut down the client and the CLI child process. /// /// Walks every still-registered session and sends `session.destroy` - /// for each one, asks SDK-owned runtimes to shut down, then kills the - /// CLI child. Errors from per-session destroys, runtime shutdown, and - /// the final child-kill are collected into + /// for each one, asks SDK-owned runtimes to shut down, then terminates + /// the owned CLI process tree and reaps its root. Errors from per-session + /// destroys, runtime shutdown, and final process termination are collected into /// [`StopErrors`] rather than short-circuiting on the first failure /// — so callers see the full picture of teardown. /// @@ -2654,8 +2660,14 @@ impl Client { } let child = self.inner.child.lock().take(); + let process_tree = self.inner.process_tree.lock().take(); *self.inner.state.lock() = ConnectionState::Disconnected; *self.inner.models_cache.lock() = Arc::new(tokio::sync::OnceCell::new()); + if let Some(process_tree) = &process_tree + && let Err(error) = process_tree.terminate() + { + errors.push(error.into()); + } if let Some(mut child) = child { match child.try_wait() { Ok(Some(_status)) => {} @@ -2696,10 +2708,9 @@ impl Client { /// /// Synchronous fallback when [`stop`](Self::stop) is unsuitable — for /// example when the awaiting tokio runtime is shutting down or the - /// process is wedged on I/O. Sends a kill signal without awaiting - /// reaper completion and immediately drops all per-session router - /// state so dependent tasks observe a closed channel rather than a - /// hang. + /// process is wedged on I/O. Terminates the owned CLI process tree and + /// immediately drops all per-session router state so dependent tasks + /// observe a closed channel rather than a hang. /// /// # Cancel safety /// @@ -2725,6 +2736,11 @@ impl Client { let pid = self.pid(); info!(pid = ?pid, "force-stopping CLI process"); self.inner.extension_launch_provider.clear(); + if let Some(process_tree) = self.inner.process_tree.lock().take() + && let Err(error) = process_tree.terminate() + { + error!(pid = ?pid, %error, "failed to terminate CLI process tree"); + } if let Some(mut child) = self.inner.child.lock().take() && let Err(e) = child.start_kill() { @@ -2786,8 +2802,13 @@ impl Client { impl Drop for ClientInner { fn drop(&mut self) { + let pid = self.child.lock().as_ref().and_then(Child::id); + if let Some(process_tree) = self.process_tree.lock().take() + && let Err(error) = process_tree.terminate() + { + error!(pid = ?pid, %error, "failed to terminate CLI process tree on drop"); + } if let Some(ref mut child) = *self.child.lock() { - let pid = child.id(); if let Err(e) = child.start_kill() { error!(pid = ?pid, error = %e, "failed to kill CLI process on drop"); } else { @@ -3447,6 +3468,7 @@ mod tests { client_read, client_write, Some(child), + None, temp.path().to_path_buf(), None, None, @@ -3465,6 +3487,183 @@ mod tests { assert_test_child_killed(&survived).await; } + #[cfg(any(unix, windows))] + #[tokio::test] + async fn stop_terminates_spawned_descendants() { + let (client, root_pid, descendant_pid) = client_with_process_tree().await; + + let _ = client.stop().await; + + wait_for_process_exit(root_pid, "root survived Client::stop").await; + wait_for_process_exit(descendant_pid, "descendant survived Client::stop").await; + } + + #[cfg(any(unix, windows))] + #[tokio::test] + async fn force_stop_terminates_spawned_descendants() { + let (client, root_pid, descendant_pid) = client_with_process_tree().await; + + client.force_stop(); + + wait_for_process_exit(root_pid, "root survived Client::force_stop").await; + wait_for_process_exit(descendant_pid, "descendant survived Client::force_stop").await; + } + + #[cfg(any(unix, windows))] + #[tokio::test] + async fn drop_terminates_spawned_descendants() { + let (client, root_pid, descendant_pid) = client_with_process_tree().await; + + drop(client); + + wait_for_process_exit(root_pid, "root survived Client drop").await; + wait_for_process_exit(descendant_pid, "descendant survived Client drop").await; + } + + #[cfg(any(unix, windows))] + #[tokio::test] + async fn failed_start_terminates_spawned_descendants() { + let temp = tempfile::tempdir().unwrap(); + let descendant_pid_path = temp.path().join("descendant-pid"); + let ready_path = temp.path().join("ready"); + let options = failed_start_options(temp.path(), &descendant_pid_path, &ready_path); + + Client::start(options).await.unwrap_err(); + + wait_for_test_child(&ready_path).await; + let descendant_pid = read_test_pid(&descendant_pid_path); + wait_for_process_exit(descendant_pid, "descendant survived failed Client::start").await; + } + + #[cfg(any(unix, windows))] + async fn client_with_process_tree() -> (Client, u32, u32) { + let temp = tempfile::tempdir().unwrap(); + let descendant_pid_path = temp.path().join("descendant-pid"); + let ready_path = temp.path().join("ready"); + let mut command = process_tree_test_command(temp.path(), &descendant_pid_path, &ready_path); + let (child, process_tree) = process_tree::spawn(&mut command).unwrap(); + let root_pid = child.id().unwrap(); + wait_for_test_child(&ready_path).await; + let descendant_pid = read_test_pid(&descendant_pid_path); + assert!(process_tree::process_alive(root_pid)); + assert!(process_tree::process_alive(descendant_pid)); + + let (client_write, server_read) = tokio::io::duplex(64); + let (server_write, client_read) = tokio::io::duplex(64); + drop(server_read); + drop(server_write); + let client = Client::from_transport( + client_read, + client_write, + Some(child), + Some(process_tree), + temp.keep(), + None, + None, + false, + false, + None, + None, + None, + ClientMode::default(), + ) + .unwrap(); + (client, root_pid, descendant_pid) + } + + #[cfg(any(unix, windows))] + fn failed_start_options(temp: &Path, descendant_pid: &Path, ready: &Path) -> ClientOptions { + #[cfg(unix)] + let (program, prefix_args) = ( + PathBuf::from("sh"), + vec![ + "-c".to_string(), + "sleep 120 >/dev/null 2>&1 & echo $! > \"$DESCENDANT_PID\"; \ + printf ready > \"$READY\"" + .to_string(), + ], + ); + #[cfg(windows)] + let (program, prefix_args) = ( + PathBuf::from("powershell.exe"), + vec![ + "-NoLogo".to_string(), + "-NoProfile".to_string(), + "-NonInteractive".to_string(), + "-Command".to_string(), + "$child = Start-Process powershell.exe -ArgumentList @( \ + '-NoLogo','-NoProfile','-NonInteractive','-Command', \ + 'Start-Sleep -Seconds 120') -PassThru; \ + Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id; \ + Set-Content -LiteralPath $env:READY ready" + .to_string(), + ], + ); + + ClientOptions::default() + .with_program(CliProgram::Path(program)) + .with_cwd(temp) + .with_prefix_args(prefix_args) + .with_env([ + ("DESCENDANT_PID", descendant_pid.as_os_str()), + ("READY", ready.as_os_str()), + ]) + } + + #[cfg(any(unix, windows))] + fn process_tree_test_command(temp: &Path, descendant_pid: &Path, ready: &Path) -> Command { + #[cfg(unix)] + let mut command = { + let mut command = + Client::build_command(Path::new("sh"), &ClientOptions::default(), temp); + command.args([ + "-c", + "sleep 120 >/dev/null 2>&1 & echo $! > \"$DESCENDANT_PID\"; \ + printf ready > \"$READY\"; wait", + ]); + command + }; + #[cfg(windows)] + let mut command = { + let mut command = + Client::build_command(Path::new("powershell.exe"), &ClientOptions::default(), temp); + command.args([ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-Command", + "$child = Start-Process powershell.exe -ArgumentList @( \ + '-NoLogo','-NoProfile','-NonInteractive','-Command', \ + 'Start-Sleep -Seconds 120') -PassThru; \ + Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id; \ + Set-Content -LiteralPath $env:READY ready; $child.WaitForExit()", + ]); + command + }; + command + .env("DESCENDANT_PID", descendant_pid) + .env("READY", ready); + command + } + + #[cfg(any(unix, windows))] + fn read_test_pid(path: &Path) -> u32 { + std::fs::read_to_string(path) + .unwrap() + .trim() + .parse() + .unwrap() + } + + #[cfg(any(unix, windows))] + async fn wait_for_process_exit(pid: u32, message: &str) { + let deadline = tokio::time::Instant::now() + Duration::from_secs(15); + while process_tree::process_alive(pid) { + assert!(tokio::time::Instant::now() < deadline, "{message}"); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + #[cfg(any(unix, windows))] #[tokio::test] async fn spawned_child_is_killed_when_dropped() { @@ -3536,6 +3735,7 @@ mod tests { Client { inner: Arc::new(ClientInner { child: parking_lot::Mutex::new(None), + process_tree: parking_lot::Mutex::new(None), #[cfg(feature = "bundled-in-process")] ffi_host: parking_lot::Mutex::new(None), rpc: { diff --git a/rust/src/process_tree.rs b/rust/src/process_tree.rs new file mode 100644 index 0000000000..afb748353e --- /dev/null +++ b/rust/src/process_tree.rs @@ -0,0 +1,253 @@ +//! Cross-platform ownership of a spawned CLI process tree. +//! +//! Windows Job Objects retain every nested descendant. Unix process groups +//! cover descendants that inherit the CLI's group; a process that explicitly +//! creates a new session or process group is outside that platform primitive. + +use std::io; + +use tokio::process::{Child, Command}; + +/// Spawns `command` inside an OS primitive that contains the root process and +/// every descendant it creates. +pub(crate) fn spawn(command: &mut Command) -> io::Result<(Child, ProcessTree)> { + platform::spawn(command).map(|(child, tree)| (child, ProcessTree(tree))) +} + +/// Owns the OS containment primitive for one SDK-spawned CLI. +pub(crate) struct ProcessTree(platform::Tree); + +impl ProcessTree { + /// Terminates every process still contained in the tree. + pub(crate) fn terminate(&self) -> io::Result<()> { + self.0.terminate() + } +} + +impl Drop for ProcessTree { + fn drop(&mut self) { + let _ = self.terminate(); + } +} + +#[cfg(test)] +pub(crate) fn process_alive(pid: u32) -> bool { + platform::process_alive(pid) +} + +#[cfg(unix)] +mod platform { + use std::io; + + use tokio::process::{Child, Command}; + + pub(super) struct Tree { + pgid: i32, + } + + pub(super) fn spawn(command: &mut Command) -> io::Result<(Child, Tree)> { + // The group is established between fork and exec, before the child + // can create descendants. + command.process_group(0); + let child = command.spawn()?; + let pid = child.id().ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "CLI exited before its process group could be recorded", + ) + })?; + Ok((child, Tree { pgid: pid as i32 })) + } + + impl Tree { + pub(super) fn terminate(&self) -> io::Result<()> { + // Callers must terminate before reaping the root. Its unreaped pid + // keeps this process-group id from being reused for an unrelated + // group between ownership lookup and signaling. + // SAFETY: `killpg` takes an integer process-group identifier and + // does not dereference memory. + if unsafe { libc::killpg(self.pgid, libc::SIGKILL) } == 0 { + return Ok(()); + } + match io::Error::last_os_error() { + error if error.raw_os_error() == Some(libc::ESRCH) => Ok(()), + error => Err(error), + } + } + } + + #[cfg(test)] + pub(super) fn process_alive(pid: u32) -> bool { + // SAFETY: signal 0 probes process existence without changing it. + unsafe { libc::kill(pid as i32, 0) == 0 } + } +} + +#[cfg(windows)] +mod platform { + use std::io; + use std::mem::size_of; + use std::os::windows::process::CommandExt; + use std::ptr; + + use tokio::process::{Child, Command}; + use windows_sys::Win32::Foundation::{CloseHandle, HANDLE, INVALID_HANDLE_VALUE}; + use windows_sys::Win32::System::Diagnostics::ToolHelp::{ + CreateToolhelp32Snapshot, TH32CS_SNAPTHREAD, THREADENTRY32, Thread32First, Thread32Next, + }; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation, + SetInformationJobObject, TerminateJobObject, + }; + use windows_sys::Win32::System::Threading::{ + CREATE_NO_WINDOW, CREATE_SUSPENDED, OpenThread, ResumeThread, THREAD_SUSPEND_RESUME, + }; + + struct OwnedHandle(HANDLE); + + // SAFETY: Win32 handles may be used and closed from any thread. + unsafe impl Send for OwnedHandle {} + unsafe impl Sync for OwnedHandle {} + + impl Drop for OwnedHandle { + fn drop(&mut self) { + // SAFETY: this value uniquely owns a valid handle. + unsafe { + CloseHandle(self.0); + } + } + } + + pub(super) struct Tree { + job: OwnedHandle, + } + + pub(super) fn spawn(command: &mut Command) -> io::Result<(Child, Tree)> { + // Suspension closes the post-spawn assignment race: the root cannot + // create descendants until it belongs to the Job Object. + command + .as_std_mut() + .creation_flags(CREATE_NO_WINDOW | CREATE_SUSPENDED); + let mut child = command.spawn()?; + let result = attach_and_resume(&child); + match result { + Ok(tree) => Ok((child, tree)), + Err(error) => { + let _ = child.start_kill(); + Err(error) + } + } + } + + fn attach_and_resume(child: &Child) -> io::Result { + // SAFETY: null security attributes and name create a private, + // non-inheritable Job Object. + let raw_job = unsafe { CreateJobObjectW(ptr::null(), ptr::null()) }; + if raw_job.is_null() { + return Err(io::Error::last_os_error()); + } + let job = OwnedHandle(raw_job); + + let mut limits = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default(); + limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + // SAFETY: `limits` has the layout required by the selected info class. + if unsafe { + SetInformationJobObject( + job.0, + JobObjectExtendedLimitInformation, + ptr::from_ref(&limits).cast(), + size_of::() as u32, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + + let process = child.raw_handle().ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "CLI exited before Job Object assignment", + ) + })?; + // SAFETY: both handles are valid and the child is still suspended. + if unsafe { AssignProcessToJobObject(job.0, process.cast()) } == 0 { + return Err(io::Error::last_os_error()); + } + + resume_initial_thread(child.id().ok_or_else(|| { + io::Error::new(io::ErrorKind::NotFound, "CLI exited before thread resume") + })?)?; + Ok(Tree { job }) + } + + fn resume_initial_thread(pid: u32) -> io::Result<()> { + // SAFETY: the returned snapshot handle is owned and closed below. + let snapshot = unsafe { CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0) }; + if snapshot == INVALID_HANDLE_VALUE { + return Err(io::Error::last_os_error()); + } + let snapshot = OwnedHandle(snapshot); + let mut entry = THREADENTRY32 { + dwSize: size_of::() as u32, + ..Default::default() + }; + + // SAFETY: `entry` has the documented size and remains live throughout + // enumeration. + let mut found = unsafe { Thread32First(snapshot.0, &mut entry) } != 0; + while found { + if entry.th32OwnerProcessID == pid { + // SAFETY: the thread id came from the live system snapshot. + let raw_thread = + unsafe { OpenThread(THREAD_SUSPEND_RESUME, 0, entry.th32ThreadID) }; + if raw_thread.is_null() { + return Err(io::Error::last_os_error()); + } + let thread = OwnedHandle(raw_thread); + // SAFETY: this is the root's suspended initial thread. + if unsafe { ResumeThread(thread.0) } == u32::MAX { + return Err(io::Error::last_os_error()); + } + return Ok(()); + } + // SAFETY: same valid snapshot and initialized entry as above. + found = unsafe { Thread32Next(snapshot.0, &mut entry) } != 0; + } + + Err(io::Error::new( + io::ErrorKind::NotFound, + "CLI initial thread was not found", + )) + } + + impl Tree { + pub(super) fn terminate(&self) -> io::Result<()> { + // SAFETY: the handle is a live Job Object owned by this value. + if unsafe { TerminateJobObject(self.job.0, 1) } != 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } + } + } + + #[cfg(test)] + pub(super) fn process_alive(pid: u32) -> bool { + use windows_sys::Win32::Foundation::WAIT_TIMEOUT; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, + }; + + // SAFETY: the process handle is closed before returning. + unsafe { + let process = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); + if process.is_null() { + return false; + } + let alive = WaitForSingleObject(process, 0) == WAIT_TIMEOUT; + CloseHandle(process); + alive + } + } +} From 84b6457dadbef766ed550bd558d1ea433af43f68 Mon Sep 17 00:00:00 2001 From: Alberto Gimeno Date: Tue, 1 Sep 2026 18:46:11 +0200 Subject: [PATCH 2/5] Disarm process tree before root reap Consume process-tree ownership during termination to prevent a second Unix signal after PID reuse, and make the Windows failed-start fixture execute through a PowerShell script. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- rust/src/lib.rs | 34 +++++++++++++++++++--------------- rust/src/process_tree.rs | 15 ++++++++------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 7875bb02df..8b9a26c3de 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -2663,7 +2663,7 @@ impl Client { let process_tree = self.inner.process_tree.lock().take(); *self.inner.state.lock() = ConnectionState::Disconnected; *self.inner.models_cache.lock() = Arc::new(tokio::sync::OnceCell::new()); - if let Some(process_tree) = &process_tree + if let Some(process_tree) = process_tree && let Err(error) = process_tree.terminate() { errors.push(error.into()); @@ -3584,21 +3584,25 @@ mod tests { ], ); #[cfg(windows)] - let (program, prefix_args) = ( - PathBuf::from("powershell.exe"), - vec![ - "-NoLogo".to_string(), - "-NoProfile".to_string(), - "-NonInteractive".to_string(), - "-Command".to_string(), + let (program, prefix_args) = (PathBuf::from("powershell.exe"), { + let script = temp.join("failed-start.ps1"); + std::fs::write( + &script, "$child = Start-Process powershell.exe -ArgumentList @( \ - '-NoLogo','-NoProfile','-NonInteractive','-Command', \ - 'Start-Sleep -Seconds 120') -PassThru; \ - Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id; \ - Set-Content -LiteralPath $env:READY ready" - .to_string(), - ], - ); + '-NoLogo','-NoProfile','-NonInteractive','-Command', \ + 'Start-Sleep -Seconds 120') -PassThru\n\ + Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id\n\ + Set-Content -LiteralPath $env:READY ready\n", + ) + .unwrap(); + vec![ + "-NoLogo".into(), + "-NoProfile".into(), + "-NonInteractive".into(), + "-File".into(), + script.into_os_string(), + ] + }); ClientOptions::default() .with_program(CliProgram::Path(program)) diff --git a/rust/src/process_tree.rs b/rust/src/process_tree.rs index afb748353e..f965f685e1 100644 --- a/rust/src/process_tree.rs +++ b/rust/src/process_tree.rs @@ -11,22 +11,24 @@ use tokio::process::{Child, Command}; /// Spawns `command` inside an OS primitive that contains the root process and /// every descendant it creates. pub(crate) fn spawn(command: &mut Command) -> io::Result<(Child, ProcessTree)> { - platform::spawn(command).map(|(child, tree)| (child, ProcessTree(tree))) + platform::spawn(command).map(|(child, tree)| (child, ProcessTree(Some(tree)))) } /// Owns the OS containment primitive for one SDK-spawned CLI. -pub(crate) struct ProcessTree(platform::Tree); +pub(crate) struct ProcessTree(Option); impl ProcessTree { /// Terminates every process still contained in the tree. - pub(crate) fn terminate(&self) -> io::Result<()> { - self.0.terminate() + pub(crate) fn terminate(mut self) -> io::Result<()> { + self.0.take().expect("process tree is armed").terminate() } } impl Drop for ProcessTree { fn drop(&mut self) { - let _ = self.terminate(); + if let Some(tree) = self.0.take() { + let _ = tree.terminate(); + } } } @@ -85,10 +87,9 @@ mod platform { #[cfg(windows)] mod platform { - use std::io; use std::mem::size_of; use std::os::windows::process::CommandExt; - use std::ptr; + use std::{io, ptr}; use tokio::process::{Child, Command}; use windows_sys::Win32::Foundation::{CloseHandle, HANDLE, INVALID_HANDLE_VALUE}; From 610bdc3bc84f8388b0821fcd27334eb1143772cf Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 1 Sep 2026 17:54:20 +0000 Subject: [PATCH 3/5] Add real-CLI E2E coverage for descendant and host-crash scenarios - Add stop_terminates_real_cli_wrapper_descendants: a Unix E2E test that launches the real bundled CLI through a shell wrapper (via ClientOptions::prefix_args) that backgrounds a descendant process, and verifies Client::stop() kills that inherited descendant. - Add abrupt_host_termination_still_kills_cli_via_job_object: a Windows E2E test representing the actual github/app#2303 scenario -- an SDK-embedding host process being killed/crashing abruptly, so none of its own cleanup code (Client::stop/force_stop/Drop) ever runs. It spawns a new copilot-host-crash-fixture helper binary that starts a real CLI client and then never calls any SDK teardown code, terminates that helper abruptly, and asserts the CLI still dies via the Job Object's kill-on-close semantics. Manually reproducing the same abrupt-kill scenario on Linux showed the CLI already exits on its own within ~200ms once the OS closes the dead host's end of the stdio pipe, with none of this crate's code involved -- a pre-existing, code-free safety net specific to stdio-piped processes on Unix. That's further evidence Unix process-group containment isn't needed to address #2303's failure mode, which is Windows-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- rust/Cargo.toml | 7 + rust/tests/e2e/client_lifecycle.rs | 284 +++++++++++++++++++++- rust/tests/fixtures/host_crash_fixture.rs | 60 +++++ 3 files changed, 350 insertions(+), 1 deletion(-) create mode 100644 rust/tests/fixtures/host_crash_fixture.rs diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 08664aefaf..7e5c78dd0b 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -114,6 +114,13 @@ required-features = ["test-support"] test = false bench = false +[[bin]] +name = "copilot-host-crash-fixture" +path = "tests/fixtures/host_crash_fixture.rs" +required-features = ["test-support"] +test = false +bench = false + [build-dependencies] base64 = "0.22" dirs = "5" diff --git a/rust/tests/e2e/client_lifecycle.rs b/rust/tests/e2e/client_lifecycle.rs index 75646b4860..b318e1b914 100644 --- a/rust/tests/e2e/client_lifecycle.rs +++ b/rust/tests/e2e/client_lifecycle.rs @@ -1,4 +1,6 @@ -use github_copilot_sdk::SessionLifecycleEventType; +#[cfg(unix)] +use github_copilot_sdk::Transport; +use github_copilot_sdk::{CliProgram, SessionLifecycleEventType}; use serde_json::json; use super::support::{wait_for_lifecycle_event, with_e2e_context}; @@ -135,6 +137,239 @@ async fn dispose_disconnects_client_and_disposes_rpc_surface_drop() { .await; } +#[cfg(unix)] +#[tokio::test] +async fn stop_terminates_real_cli_wrapper_descendants() { + with_e2e_context( + "client_lifecycle", + "stop_terminates_real_cli_wrapper_descendants", + |ctx| { + Box::pin(async move { + if super::support::skip_inprocess( + "process-tree ownership only applies to SDK-spawned child-process transports", + ) { + return; + } + + let descendant_pid_path = ctx.work_dir().join("wrapper-descendant.pid"); + let mut options = ctx.client_options().with_transport(Transport::Stdio); + let original_program = match &options.program { + CliProgram::Path(path) => path.clone(), + CliProgram::Resolve => { + panic!("E2E client options should resolve to an explicit CLI path") + } + }; + let mut wrapper_args = vec![ + "-c".into(), + "sleep 120 >/dev/null 2>&1 & echo $! > \"$SDK_DESCENDANT_PID\"; exec \"$@\"" + .into(), + "sdk-cli-wrapper".into(), + original_program.into_os_string(), + ]; + wrapper_args.extend(options.prefix_args); + options.program = CliProgram::Path(std::path::PathBuf::from("sh")); + options.prefix_args = wrapper_args; + options.env.push(( + "SDK_DESCENDANT_PID".into(), + descendant_pid_path.clone().into(), + )); + + let client = github_copilot_sdk::Client::start(options) + .await + .expect("start wrapped real CLI"); + let session = client + .create_session(ctx.approve_all_session_config()) + .await + .expect("create session through wrapped real CLI"); + session.disconnect().await.expect("disconnect session"); + + let descendant_pid = wait_for_pid_file(&descendant_pid_path).await; + assert!( + process_alive(descendant_pid), + "wrapper descendant should be alive before client stop" + ); + + client.stop().await.expect("stop wrapped real CLI"); + let exited = wait_for_process_exit(descendant_pid).await; + if !exited { + kill_process(descendant_pid); + } + assert!(exited, "real CLI wrapper descendant survived Client::stop"); + }) + }, + ) + .await; +} + +// This test represents github/app#2303: an SDK-embedding host process +// (there, the "Agency" process) is killed or crashes abruptly, without ever +// running any of its own cleanup code — so `Client::stop`/`force_stop`/`Drop` +// never execute. On Windows, the CLI is contained in a Job Object with +// `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so the OS itself terminates the CLI +// when the last handle to the job closes, which happens automatically when +// the owning process exits for any reason, including an abrupt, uncatchable +// termination. This test spawns a separate helper process that starts a real +// CLI client and then never calls any SDK cleanup code, terminates that +// helper process abruptly (`TerminateProcess` via `Child::kill`, which runs +// none of the helper's own code), and asserts the CLI still dies. +// +// Unix process groups have no equivalent auto-kill-on-owner-death guarantee +// (a `SIGKILL`ed owner leaves `Drop` un-run and `killpg` never called), so +// this test is Windows-only; it validates the property this PR's Windows +// implementation specifically targets. +// +// Manually reproducing this same abrupt-kill scenario on Linux with +// `Transport::Stdio` (a host process started, spawned a real CLI, then was +// `SIGKILL`ed with no cleanup code running) showed the CLI still exiting on +// its own within ~200ms, driven entirely by stdin EOF once the OS closed the +// dead host's end of the pipe — with none of this crate's code involved. +// That is a real, pre-existing, code-free safety net specific to +// stdio-piped processes on Unix; it's further evidence Unix process-group +// containment isn't needed to fix #2303's failure mode. +#[cfg(windows)] +#[tokio::test] +async fn abrupt_host_termination_still_kills_cli_via_job_object() { + with_e2e_context( + "client_lifecycle", + "abrupt_host_termination_still_kills_cli_via_job_object", + |ctx| { + Box::pin(async move { + let options = ctx.client_options(); + let program = match &options.program { + CliProgram::Path(path) => path + .to_str() + .expect("CLI program path is valid UTF-8") + .to_owned(), + CliProgram::Resolve => { + panic!("E2E client options should resolve to an explicit CLI path") + } + }; + let prefix_args: Vec = options + .prefix_args + .iter() + .map(|arg| arg.to_str().expect("prefix arg is valid UTF-8").to_owned()) + .collect(); + let env_pairs: Vec<(String, String)> = options + .env + .iter() + .map(|(k, v)| { + ( + k.to_str().expect("env key is valid UTF-8").to_owned(), + v.to_str().expect("env value is valid UTF-8").to_owned(), + ) + }) + .collect(); + let cwd = options + .working_directory + .to_str() + .expect("cwd is valid UTF-8") + .to_owned(); + let pid_file = ctx.work_dir().join("host-crash-fixture-cli.pid"); + + let mut host = + std::process::Command::new(env!("CARGO_BIN_EXE_copilot-host-crash-fixture")) + .env("HOST_CRASH_FIXTURE_PROGRAM", &program) + .env( + "HOST_CRASH_FIXTURE_PREFIX_ARGS_JSON", + serde_json::to_string(&prefix_args).expect("serialize prefix args"), + ) + .env("HOST_CRASH_FIXTURE_CWD", &cwd) + .env( + "HOST_CRASH_FIXTURE_ENV_JSON", + serde_json::to_string(&env_pairs).expect("serialize env pairs"), + ) + .env("HOST_CRASH_FIXTURE_PID_FILE", &pid_file) + .spawn() + .expect("spawn host-crash fixture process"); + + let cli_pid = wait_for_pid_file_windows(&pid_file).await; + assert!( + process_alive_windows(cli_pid), + "CLI should be alive before its host process is terminated" + ); + + // Abruptly terminate the fixture process itself — the + // Windows analogue of the Agency process dying in #2303. + // `Child::kill` maps to `TerminateProcess`, which runs none + // of the target process's own code (no `Drop`, no `main` + // unwind). + host.kill().expect("terminate host-crash fixture process"); + host.wait().expect("reap host-crash fixture process"); + + let cli_exited = wait_for_process_exit_windows(cli_pid).await; + if !cli_exited { + kill_process_windows(cli_pid); + } + assert!( + cli_exited, + "CLI survived its abruptly terminated host process; Job Object \ + kill-on-close did not terminate it" + ); + }) + }, + ) + .await; +} + +#[cfg(windows)] +async fn wait_for_pid_file_windows(path: &std::path::Path) -> u32 { + super::support::wait_for_condition("host-crash fixture CLI pid file", || async { + path.exists() + }) + .await; + std::fs::read_to_string(path) + .expect("read host-crash fixture CLI pid") + .trim() + .parse() + .expect("parse host-crash fixture CLI pid") +} + +#[cfg(windows)] +async fn wait_for_process_exit_windows(pid: u32) -> bool { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); + while process_alive_windows(pid) { + if std::time::Instant::now() >= deadline { + return false; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + true +} + +#[cfg(windows)] +fn process_alive_windows(pid: u32) -> bool { + use windows_sys::Win32::Foundation::{CloseHandle, WAIT_TIMEOUT}; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, + }; + + // SAFETY: the process handle is closed before returning. + unsafe { + let process = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); + if process.is_null() { + return false; + } + let alive = WaitForSingleObject(process, 0) == WAIT_TIMEOUT; + CloseHandle(process); + alive + } +} + +#[cfg(windows)] +fn kill_process_windows(pid: u32) { + use windows_sys::Win32::Foundation::CloseHandle; + use windows_sys::Win32::System::Threading::{OpenProcess, PROCESS_TERMINATE, TerminateProcess}; + + // SAFETY: the pid came from this test's controlled fixture-spawned CLI. + unsafe { + let process = OpenProcess(PROCESS_TERMINATE, 0, pid); + if !process.is_null() { + TerminateProcess(process, 1); + CloseHandle(process); + } + } +} + #[tokio::test] async fn should_receive_session_updated_lifecycle_event_for_non_ephemeral_activity() { with_e2e_context( @@ -178,6 +413,53 @@ async fn should_receive_session_updated_lifecycle_event_for_non_ephemeral_activi .await; } +#[cfg(unix)] +async fn wait_for_pid_file(path: &std::path::Path) -> u32 { + super::support::wait_for_condition("wrapper descendant pid file", || async { path.exists() }) + .await; + std::fs::read_to_string(path) + .expect("read wrapper descendant pid") + .trim() + .parse() + .expect("parse wrapper descendant pid") +} + +#[cfg(unix)] +async fn wait_for_process_exit(pid: u32) -> bool { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); + while process_alive(pid) { + if std::time::Instant::now() >= deadline { + return false; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + true +} + +#[cfg(unix)] +fn process_alive(pid: u32) -> bool { + #[cfg(target_os = "linux")] + if let Ok(stat) = std::fs::read_to_string(format!("/proc/{pid}/stat")) + && stat + .rsplit_once(") ") + .and_then(|(_, fields)| fields.chars().next()) + .is_some_and(|state| matches!(state, 'Z' | 'X')) + { + return false; + } + + // SAFETY: signal 0 probes process existence without modifying it. + unsafe { libc::kill(pid as i32, 0) == 0 } +} + +#[cfg(unix)] +fn kill_process(pid: u32) { + // SAFETY: the pid came from this test's controlled descendant process. + unsafe { + libc::kill(pid as i32, libc::SIGKILL); + } +} + #[tokio::test] async fn should_receive_session_deleted_lifecycle_event_when_deleted() { with_e2e_context( diff --git a/rust/tests/fixtures/host_crash_fixture.rs b/rust/tests/fixtures/host_crash_fixture.rs new file mode 100644 index 0000000000..c688cf3e87 --- /dev/null +++ b/rust/tests/fixtures/host_crash_fixture.rs @@ -0,0 +1,60 @@ +//! Test-only binary that hosts a single [`github_copilot_sdk::Client`] and then +//! blocks forever, so an external test can terminate *this* process abruptly +//! (simulating an SDK-embedding app process crashing) without ever running any +//! of this process's own cleanup code (`Client::stop`, `force_stop`, or +//! `Drop`). +//! +//! Configuration is passed entirely through environment variables so the +//! caller doesn't need this crate's non-`pub` types: +//! - `HOST_CRASH_FIXTURE_PROGRAM`: CLI program path. +//! - `HOST_CRASH_FIXTURE_PREFIX_ARGS_JSON`: JSON array of prefix args. +//! - `HOST_CRASH_FIXTURE_CWD`: working directory for the spawned CLI. +//! - `HOST_CRASH_FIXTURE_ENV_JSON`: JSON array of `[key, value]` pairs to set +//! on the spawned CLI's environment. +//! - `HOST_CRASH_FIXTURE_PID_FILE`: path this process writes the CLI child's +//! OS process id to, once the client finishes starting. + +use std::path::PathBuf; + +use github_copilot_sdk::{CliProgram, Client, ClientOptions, Transport}; + +#[tokio::main(flavor = "current_thread")] +async fn main() { + let program = std::env::var("HOST_CRASH_FIXTURE_PROGRAM").expect("HOST_CRASH_FIXTURE_PROGRAM"); + let prefix_args: Vec = serde_json::from_str( + &std::env::var("HOST_CRASH_FIXTURE_PREFIX_ARGS_JSON") + .expect("HOST_CRASH_FIXTURE_PREFIX_ARGS_JSON"), + ) + .expect("parse HOST_CRASH_FIXTURE_PREFIX_ARGS_JSON"); + let cwd = std::env::var("HOST_CRASH_FIXTURE_CWD").expect("HOST_CRASH_FIXTURE_CWD"); + let env_pairs: Vec<(String, String)> = serde_json::from_str( + &std::env::var("HOST_CRASH_FIXTURE_ENV_JSON").expect("HOST_CRASH_FIXTURE_ENV_JSON"), + ) + .expect("parse HOST_CRASH_FIXTURE_ENV_JSON"); + let pid_file = PathBuf::from( + std::env::var("HOST_CRASH_FIXTURE_PID_FILE").expect("HOST_CRASH_FIXTURE_PID_FILE"), + ); + + let options = ClientOptions::new() + .with_program(CliProgram::Path(PathBuf::from(program))) + .with_prefix_args(prefix_args) + .with_cwd(PathBuf::from(cwd)) + .with_env(env_pairs) + .with_use_logged_in_user(false) + .with_transport(Transport::Stdio); + + let client = Client::start(options).await.expect("start CLI client"); + let pid = client.pid().expect("client reports spawned CLI pid"); + std::fs::write(&pid_file, pid.to_string()).expect("write pid file"); + + // Deliberately leak the client so nothing in this process — including its + // `Drop` impls — ever runs cleanup code. The external test process + // terminates this process abruptly (e.g. `TerminateProcess` on Windows) + // to simulate an SDK-embedding host crashing, and asserts that the CLI + // still dies via the OS containment primitive alone. + std::mem::forget(client); + + loop { + std::thread::sleep(std::time::Duration::from_secs(3600)); + } +} From 4080d88e57fb0fd70b9576124c63634fdf6c4849 Mon Sep 17 00:00:00 2001 From: Alberto Gimeno Date: Tue, 1 Sep 2026 20:09:07 +0200 Subject: [PATCH 4/5] fix(rust): narrow process ownership to Windows crash safety Use a kill-on-close Job Object only on Windows and test that abruptly terminating the SDK host tears down its owned CLI process. Preserve direct-child behavior on other platforms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- rust/Cargo.lock | 1 - rust/Cargo.toml | 3 - rust/src/lib.rs | 212 ++++----------------------------------- rust/src/process_tree.rs | 160 +++++++++++++++++------------ 4 files changed, 114 insertions(+), 262 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index b4445c0b63..b91eebd06c 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -434,7 +434,6 @@ dependencies = [ "getrandom 0.2.17", "http", "indexmap", - "libc", "libloading", "native-tls", "parking_lot", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 7e5c78dd0b..c7a704fd50 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -77,9 +77,6 @@ windows-sys = { version = "0.61", default-features = false, features = [ "Win32_System_Threading", ] } -[target.'cfg(unix)'.dependencies] -libc = "0.2" - [dev-dependencies] rusqlite = { version = "0.35", features = ["bundled"] } schemars = "1" diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 8b9a26c3de..32d44ddc0f 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1337,7 +1337,7 @@ impl Client { reader, writer, Some(child), - Some(tree), + tree, working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1360,7 +1360,7 @@ impl Client { stdout, stdin, Some(child), - Some(tree), + tree, working_directory, options.on_list_models, extension_launch_provider.clone(), @@ -1939,7 +1939,7 @@ impl Client { program: &Path, options: &ClientOptions, working_directory: &Path, - ) -> Result<(Child, process_tree::ProcessTree, Duration)> { + ) -> Result<(Child, Option, Duration)> { info!(cwd = ?working_directory, program = %program.display(), "spawning copilot CLI (stdio)"); let mut command = Self::build_command(program, options, working_directory); command @@ -1965,7 +1965,13 @@ impl Client { options: &ClientOptions, working_directory: &Path, port: u16, - ) -> Result<(Child, process_tree::ProcessTree, u16, Duration, Duration)> { + ) -> Result<( + Child, + Option, + u16, + Duration, + Duration, + )> { info!(cwd = ?working_directory, program = %program.display(), port = %port, "spawning copilot CLI (tcp)"); let mut command = Self::build_command(program, options, working_directory); command @@ -2567,11 +2573,12 @@ impl Client { /// Cooperatively shut down the client and the CLI child process. /// /// Walks every still-registered session and sends `session.destroy` - /// for each one, asks SDK-owned runtimes to shut down, then terminates - /// the owned CLI process tree and reaps its root. Errors from per-session - /// destroys, runtime shutdown, and final process termination are collected into - /// [`StopErrors`] rather than short-circuiting on the first failure - /// — so callers see the full picture of teardown. + /// for each one, asks SDK-owned runtimes to shut down, terminates the + /// Windows-owned CLI Job Object when present, and reaps the root process. + /// Errors from per-session destroys, runtime shutdown, and final process + /// termination are collected into [`StopErrors`] rather than + /// short-circuiting on the first failure — so callers see the full picture + /// of teardown. /// /// If you have already called [`Session::disconnect`] on every /// session this client created, the per-session destroy step is a @@ -2708,9 +2715,9 @@ impl Client { /// /// Synchronous fallback when [`stop`](Self::stop) is unsuitable — for /// example when the awaiting tokio runtime is shutting down or the - /// process is wedged on I/O. Terminates the owned CLI process tree and - /// immediately drops all per-session router state so dependent tasks - /// observe a closed channel rather than a hang. + /// process is wedged on I/O. Terminates the Windows-owned CLI Job Object + /// when present and immediately drops all per-session router state so + /// dependent tasks observe a closed channel rather than a hang. /// /// # Cancel safety /// @@ -3487,187 +3494,6 @@ mod tests { assert_test_child_killed(&survived).await; } - #[cfg(any(unix, windows))] - #[tokio::test] - async fn stop_terminates_spawned_descendants() { - let (client, root_pid, descendant_pid) = client_with_process_tree().await; - - let _ = client.stop().await; - - wait_for_process_exit(root_pid, "root survived Client::stop").await; - wait_for_process_exit(descendant_pid, "descendant survived Client::stop").await; - } - - #[cfg(any(unix, windows))] - #[tokio::test] - async fn force_stop_terminates_spawned_descendants() { - let (client, root_pid, descendant_pid) = client_with_process_tree().await; - - client.force_stop(); - - wait_for_process_exit(root_pid, "root survived Client::force_stop").await; - wait_for_process_exit(descendant_pid, "descendant survived Client::force_stop").await; - } - - #[cfg(any(unix, windows))] - #[tokio::test] - async fn drop_terminates_spawned_descendants() { - let (client, root_pid, descendant_pid) = client_with_process_tree().await; - - drop(client); - - wait_for_process_exit(root_pid, "root survived Client drop").await; - wait_for_process_exit(descendant_pid, "descendant survived Client drop").await; - } - - #[cfg(any(unix, windows))] - #[tokio::test] - async fn failed_start_terminates_spawned_descendants() { - let temp = tempfile::tempdir().unwrap(); - let descendant_pid_path = temp.path().join("descendant-pid"); - let ready_path = temp.path().join("ready"); - let options = failed_start_options(temp.path(), &descendant_pid_path, &ready_path); - - Client::start(options).await.unwrap_err(); - - wait_for_test_child(&ready_path).await; - let descendant_pid = read_test_pid(&descendant_pid_path); - wait_for_process_exit(descendant_pid, "descendant survived failed Client::start").await; - } - - #[cfg(any(unix, windows))] - async fn client_with_process_tree() -> (Client, u32, u32) { - let temp = tempfile::tempdir().unwrap(); - let descendant_pid_path = temp.path().join("descendant-pid"); - let ready_path = temp.path().join("ready"); - let mut command = process_tree_test_command(temp.path(), &descendant_pid_path, &ready_path); - let (child, process_tree) = process_tree::spawn(&mut command).unwrap(); - let root_pid = child.id().unwrap(); - wait_for_test_child(&ready_path).await; - let descendant_pid = read_test_pid(&descendant_pid_path); - assert!(process_tree::process_alive(root_pid)); - assert!(process_tree::process_alive(descendant_pid)); - - let (client_write, server_read) = tokio::io::duplex(64); - let (server_write, client_read) = tokio::io::duplex(64); - drop(server_read); - drop(server_write); - let client = Client::from_transport( - client_read, - client_write, - Some(child), - Some(process_tree), - temp.keep(), - None, - None, - false, - false, - None, - None, - None, - ClientMode::default(), - ) - .unwrap(); - (client, root_pid, descendant_pid) - } - - #[cfg(any(unix, windows))] - fn failed_start_options(temp: &Path, descendant_pid: &Path, ready: &Path) -> ClientOptions { - #[cfg(unix)] - let (program, prefix_args) = ( - PathBuf::from("sh"), - vec![ - "-c".to_string(), - "sleep 120 >/dev/null 2>&1 & echo $! > \"$DESCENDANT_PID\"; \ - printf ready > \"$READY\"" - .to_string(), - ], - ); - #[cfg(windows)] - let (program, prefix_args) = (PathBuf::from("powershell.exe"), { - let script = temp.join("failed-start.ps1"); - std::fs::write( - &script, - "$child = Start-Process powershell.exe -ArgumentList @( \ - '-NoLogo','-NoProfile','-NonInteractive','-Command', \ - 'Start-Sleep -Seconds 120') -PassThru\n\ - Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id\n\ - Set-Content -LiteralPath $env:READY ready\n", - ) - .unwrap(); - vec![ - "-NoLogo".into(), - "-NoProfile".into(), - "-NonInteractive".into(), - "-File".into(), - script.into_os_string(), - ] - }); - - ClientOptions::default() - .with_program(CliProgram::Path(program)) - .with_cwd(temp) - .with_prefix_args(prefix_args) - .with_env([ - ("DESCENDANT_PID", descendant_pid.as_os_str()), - ("READY", ready.as_os_str()), - ]) - } - - #[cfg(any(unix, windows))] - fn process_tree_test_command(temp: &Path, descendant_pid: &Path, ready: &Path) -> Command { - #[cfg(unix)] - let mut command = { - let mut command = - Client::build_command(Path::new("sh"), &ClientOptions::default(), temp); - command.args([ - "-c", - "sleep 120 >/dev/null 2>&1 & echo $! > \"$DESCENDANT_PID\"; \ - printf ready > \"$READY\"; wait", - ]); - command - }; - #[cfg(windows)] - let mut command = { - let mut command = - Client::build_command(Path::new("powershell.exe"), &ClientOptions::default(), temp); - command.args([ - "-NoLogo", - "-NoProfile", - "-NonInteractive", - "-Command", - "$child = Start-Process powershell.exe -ArgumentList @( \ - '-NoLogo','-NoProfile','-NonInteractive','-Command', \ - 'Start-Sleep -Seconds 120') -PassThru; \ - Set-Content -LiteralPath $env:DESCENDANT_PID $child.Id; \ - Set-Content -LiteralPath $env:READY ready; $child.WaitForExit()", - ]); - command - }; - command - .env("DESCENDANT_PID", descendant_pid) - .env("READY", ready); - command - } - - #[cfg(any(unix, windows))] - fn read_test_pid(path: &Path) -> u32 { - std::fs::read_to_string(path) - .unwrap() - .trim() - .parse() - .unwrap() - } - - #[cfg(any(unix, windows))] - async fn wait_for_process_exit(pid: u32, message: &str) { - let deadline = tokio::time::Instant::now() + Duration::from_secs(15); - while process_tree::process_alive(pid) { - assert!(tokio::time::Instant::now() < deadline, "{message}"); - tokio::time::sleep(Duration::from_millis(10)).await; - } - } - #[cfg(any(unix, windows))] #[tokio::test] async fn spawned_child_is_killed_when_dropped() { diff --git a/rust/src/process_tree.rs b/rust/src/process_tree.rs index f965f685e1..242092d6b6 100644 --- a/rust/src/process_tree.rs +++ b/rust/src/process_tree.rs @@ -1,24 +1,28 @@ -//! Cross-platform ownership of a spawned CLI process tree. +//! Windows crash-safe ownership of an SDK-spawned CLI process. //! -//! Windows Job Objects retain every nested descendant. Unix process groups -//! cover descendants that inherit the CLI's group; a process that explicitly -//! creates a new session or process group is outside that platform primitive. +//! A Job Object with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` lets Windows +//! terminate the CLI when the SDK-hosting process exits abruptly, even when +//! Rust cleanup code never runs. Other platforms retain Tokio's direct-child +//! ownership because no equivalent product failure has been demonstrated. use std::io; use tokio::process::{Child, Command}; -/// Spawns `command` inside an OS primitive that contains the root process and -/// every descendant it creates. -pub(crate) fn spawn(command: &mut Command) -> io::Result<(Child, ProcessTree)> { - platform::spawn(command).map(|(child, tree)| (child, ProcessTree(Some(tree)))) +pub(crate) fn spawn(command: &mut Command) -> io::Result<(Child, Option)> { + #[cfg(windows)] + { + platform::spawn(command).map(|(child, tree)| (child, Some(ProcessTree(Some(tree))))) + } + #[cfg(not(windows))] + { + command.spawn().map(|child| (child, None)) + } } -/// Owns the OS containment primitive for one SDK-spawned CLI. pub(crate) struct ProcessTree(Option); impl ProcessTree { - /// Terminates every process still contained in the tree. pub(crate) fn terminate(mut self) -> io::Result<()> { self.0.take().expect("process tree is armed").terminate() } @@ -32,57 +36,15 @@ impl Drop for ProcessTree { } } -#[cfg(test)] -pub(crate) fn process_alive(pid: u32) -> bool { - platform::process_alive(pid) -} - -#[cfg(unix)] +#[cfg(not(windows))] mod platform { - use std::io; - - use tokio::process::{Child, Command}; - - pub(super) struct Tree { - pgid: i32, - } - - pub(super) fn spawn(command: &mut Command) -> io::Result<(Child, Tree)> { - // The group is established between fork and exec, before the child - // can create descendants. - command.process_group(0); - let child = command.spawn()?; - let pid = child.id().ok_or_else(|| { - io::Error::new( - io::ErrorKind::NotFound, - "CLI exited before its process group could be recorded", - ) - })?; - Ok((child, Tree { pgid: pid as i32 })) - } + pub(super) struct Tree; impl Tree { - pub(super) fn terminate(&self) -> io::Result<()> { - // Callers must terminate before reaping the root. Its unreaped pid - // keeps this process-group id from being reused for an unrelated - // group between ownership lookup and signaling. - // SAFETY: `killpg` takes an integer process-group identifier and - // does not dereference memory. - if unsafe { libc::killpg(self.pgid, libc::SIGKILL) } == 0 { - return Ok(()); - } - match io::Error::last_os_error() { - error if error.raw_os_error() == Some(libc::ESRCH) => Ok(()), - error => Err(error), - } + pub(super) fn terminate(&self) -> std::io::Result<()> { + unreachable!("process-tree ownership is Windows-only") } } - - #[cfg(test)] - pub(super) fn process_alive(pid: u32) -> bool { - // SAFETY: signal 0 probes process existence without changing it. - unsafe { libc::kill(pid as i32, 0) == 0 } - } } #[cfg(windows)] @@ -125,14 +87,12 @@ mod platform { } pub(super) fn spawn(command: &mut Command) -> io::Result<(Child, Tree)> { - // Suspension closes the post-spawn assignment race: the root cannot - // create descendants until it belongs to the Job Object. + // The root cannot run or create descendants before Job assignment. command .as_std_mut() .creation_flags(CREATE_NO_WINDOW | CREATE_SUSPENDED); let mut child = command.spawn()?; - let result = attach_and_resume(&child); - match result { + match attach_and_resume(&child) { Ok(tree) => Ok((child, tree)), Err(error) => { let _ = child.start_kill(); @@ -232,14 +192,84 @@ mod platform { } } } +} + +#[cfg(all(test, windows))] +mod tests { + use std::path::Path; + use std::time::Duration; + + use tokio::process::Command; + use windows_sys::Win32::Foundation::{CloseHandle, WAIT_TIMEOUT}; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, + }; + + use super::spawn; + + const HELPER_FILTER: &str = "process_tree::tests::sdk_host_helper_entrypoint"; - #[cfg(test)] - pub(super) fn process_alive(pid: u32) -> bool { - use windows_sys::Win32::Foundation::WAIT_TIMEOUT; - use windows_sys::Win32::System::Threading::{ - OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, + #[tokio::test] + async fn sdk_host_helper_entrypoint() { + let Ok(cli_pid_path) = std::env::var("PROCESS_TREE_CLI_PID_PATH") else { + return; }; + let mut command = Command::new("powershell.exe"); + command.args([ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-Command", + "Start-Sleep -Seconds 120", + ]); + let (child, _tree) = spawn(&mut command).expect("spawn owned CLI process"); + std::fs::write(cli_pid_path, child.id().expect("CLI pid").to_string()) + .expect("record CLI pid"); + std::future::pending::<()>().await; + } + + #[tokio::test] + async fn job_kills_cli_when_sdk_host_is_terminated() { + let temp = tempfile::tempdir().expect("create temp directory"); + let cli_pid_path = temp.path().join("cli.pid"); + let mut host = Command::new(std::env::current_exe().expect("current test binary")); + host.args([HELPER_FILTER, "--exact", "--nocapture"]) + .env("PROCESS_TREE_CLI_PID_PATH", &cli_pid_path); + let mut host = host.spawn().expect("spawn SDK host"); + let cli_pid = wait_for_pid(&cli_pid_path).await; + assert!(process_alive(cli_pid), "CLI must be alive before host exit"); + + host.kill().await.expect("terminate SDK host abruptly"); + + wait_for_process_exit(cli_pid).await; + } + + async fn wait_for_pid(path: &Path) -> u32 { + let deadline = tokio::time::Instant::now() + Duration::from_secs(15); + loop { + if let Ok(value) = std::fs::read_to_string(path) { + return value.trim().parse().expect("parse CLI pid"); + } + assert!( + tokio::time::Instant::now() < deadline, + "SDK host did not record its CLI pid" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + async fn wait_for_process_exit(pid: u32) { + let deadline = tokio::time::Instant::now() + Duration::from_secs(15); + while process_alive(pid) { + assert!( + tokio::time::Instant::now() < deadline, + "CLI survived abrupt SDK host termination" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + fn process_alive(pid: u32) -> bool { // SAFETY: the process handle is closed before returning. unsafe { let process = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); From 7576fdea4334015927ecf984de4a1693accfd4d6 Mon Sep 17 00:00:00 2001 From: Alberto Gimeno Date: Tue, 1 Sep 2026 20:21:35 +0200 Subject: [PATCH 5/5] test(rust): focus process ownership coverage on Windows Keep the real-CLI abrupt-host E2E and remove unsupported Unix descendant coverage and the redundant synthetic crash test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- rust/src/process_tree.rs | 89 ----------------- rust/tests/e2e/client_lifecycle.rs | 151 ++--------------------------- 2 files changed, 8 insertions(+), 232 deletions(-) diff --git a/rust/src/process_tree.rs b/rust/src/process_tree.rs index 242092d6b6..8dc7a451c5 100644 --- a/rust/src/process_tree.rs +++ b/rust/src/process_tree.rs @@ -193,92 +193,3 @@ mod platform { } } } - -#[cfg(all(test, windows))] -mod tests { - use std::path::Path; - use std::time::Duration; - - use tokio::process::Command; - use windows_sys::Win32::Foundation::{CloseHandle, WAIT_TIMEOUT}; - use windows_sys::Win32::System::Threading::{ - OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, - }; - - use super::spawn; - - const HELPER_FILTER: &str = "process_tree::tests::sdk_host_helper_entrypoint"; - - #[tokio::test] - async fn sdk_host_helper_entrypoint() { - let Ok(cli_pid_path) = std::env::var("PROCESS_TREE_CLI_PID_PATH") else { - return; - }; - let mut command = Command::new("powershell.exe"); - command.args([ - "-NoLogo", - "-NoProfile", - "-NonInteractive", - "-Command", - "Start-Sleep -Seconds 120", - ]); - let (child, _tree) = spawn(&mut command).expect("spawn owned CLI process"); - std::fs::write(cli_pid_path, child.id().expect("CLI pid").to_string()) - .expect("record CLI pid"); - std::future::pending::<()>().await; - } - - #[tokio::test] - async fn job_kills_cli_when_sdk_host_is_terminated() { - let temp = tempfile::tempdir().expect("create temp directory"); - let cli_pid_path = temp.path().join("cli.pid"); - let mut host = Command::new(std::env::current_exe().expect("current test binary")); - host.args([HELPER_FILTER, "--exact", "--nocapture"]) - .env("PROCESS_TREE_CLI_PID_PATH", &cli_pid_path); - let mut host = host.spawn().expect("spawn SDK host"); - let cli_pid = wait_for_pid(&cli_pid_path).await; - assert!(process_alive(cli_pid), "CLI must be alive before host exit"); - - host.kill().await.expect("terminate SDK host abruptly"); - - wait_for_process_exit(cli_pid).await; - } - - async fn wait_for_pid(path: &Path) -> u32 { - let deadline = tokio::time::Instant::now() + Duration::from_secs(15); - loop { - if let Ok(value) = std::fs::read_to_string(path) { - return value.trim().parse().expect("parse CLI pid"); - } - assert!( - tokio::time::Instant::now() < deadline, - "SDK host did not record its CLI pid" - ); - tokio::time::sleep(Duration::from_millis(10)).await; - } - } - - async fn wait_for_process_exit(pid: u32) { - let deadline = tokio::time::Instant::now() + Duration::from_secs(15); - while process_alive(pid) { - assert!( - tokio::time::Instant::now() < deadline, - "CLI survived abrupt SDK host termination" - ); - tokio::time::sleep(Duration::from_millis(10)).await; - } - } - - fn process_alive(pid: u32) -> bool { - // SAFETY: the process handle is closed before returning. - unsafe { - let process = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); - if process.is_null() { - return false; - } - let alive = WaitForSingleObject(process, 0) == WAIT_TIMEOUT; - CloseHandle(process); - alive - } - } -} diff --git a/rust/tests/e2e/client_lifecycle.rs b/rust/tests/e2e/client_lifecycle.rs index b318e1b914..92bfa6ff6d 100644 --- a/rust/tests/e2e/client_lifecycle.rs +++ b/rust/tests/e2e/client_lifecycle.rs @@ -1,6 +1,6 @@ -#[cfg(unix)] -use github_copilot_sdk::Transport; -use github_copilot_sdk::{CliProgram, SessionLifecycleEventType}; +#[cfg(windows)] +use github_copilot_sdk::CliProgram; +use github_copilot_sdk::SessionLifecycleEventType; use serde_json::json; use super::support::{wait_for_lifecycle_event, with_e2e_context}; @@ -137,95 +137,10 @@ async fn dispose_disconnects_client_and_disposes_rpc_surface_drop() { .await; } -#[cfg(unix)] -#[tokio::test] -async fn stop_terminates_real_cli_wrapper_descendants() { - with_e2e_context( - "client_lifecycle", - "stop_terminates_real_cli_wrapper_descendants", - |ctx| { - Box::pin(async move { - if super::support::skip_inprocess( - "process-tree ownership only applies to SDK-spawned child-process transports", - ) { - return; - } - - let descendant_pid_path = ctx.work_dir().join("wrapper-descendant.pid"); - let mut options = ctx.client_options().with_transport(Transport::Stdio); - let original_program = match &options.program { - CliProgram::Path(path) => path.clone(), - CliProgram::Resolve => { - panic!("E2E client options should resolve to an explicit CLI path") - } - }; - let mut wrapper_args = vec![ - "-c".into(), - "sleep 120 >/dev/null 2>&1 & echo $! > \"$SDK_DESCENDANT_PID\"; exec \"$@\"" - .into(), - "sdk-cli-wrapper".into(), - original_program.into_os_string(), - ]; - wrapper_args.extend(options.prefix_args); - options.program = CliProgram::Path(std::path::PathBuf::from("sh")); - options.prefix_args = wrapper_args; - options.env.push(( - "SDK_DESCENDANT_PID".into(), - descendant_pid_path.clone().into(), - )); - - let client = github_copilot_sdk::Client::start(options) - .await - .expect("start wrapped real CLI"); - let session = client - .create_session(ctx.approve_all_session_config()) - .await - .expect("create session through wrapped real CLI"); - session.disconnect().await.expect("disconnect session"); - - let descendant_pid = wait_for_pid_file(&descendant_pid_path).await; - assert!( - process_alive(descendant_pid), - "wrapper descendant should be alive before client stop" - ); - - client.stop().await.expect("stop wrapped real CLI"); - let exited = wait_for_process_exit(descendant_pid).await; - if !exited { - kill_process(descendant_pid); - } - assert!(exited, "real CLI wrapper descendant survived Client::stop"); - }) - }, - ) - .await; -} - -// This test represents github/app#2303: an SDK-embedding host process -// (there, the "Agency" process) is killed or crashes abruptly, without ever -// running any of its own cleanup code — so `Client::stop`/`force_stop`/`Drop` -// never execute. On Windows, the CLI is contained in a Job Object with -// `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so the OS itself terminates the CLI -// when the last handle to the job closes, which happens automatically when -// the owning process exits for any reason, including an abrupt, uncatchable -// termination. This test spawns a separate helper process that starts a real -// CLI client and then never calls any SDK cleanup code, terminates that -// helper process abruptly (`TerminateProcess` via `Child::kill`, which runs -// none of the helper's own code), and asserts the CLI still dies. -// -// Unix process groups have no equivalent auto-kill-on-owner-death guarantee -// (a `SIGKILL`ed owner leaves `Drop` un-run and `killpg` never called), so -// this test is Windows-only; it validates the property this PR's Windows -// implementation specifically targets. -// -// Manually reproducing this same abrupt-kill scenario on Linux with -// `Transport::Stdio` (a host process started, spawned a real CLI, then was -// `SIGKILL`ed with no cleanup code running) showed the CLI still exiting on -// its own within ~200ms, driven entirely by stdin EOF once the OS closed the -// dead host's end of the pipe — with none of this crate's code involved. -// That is a real, pre-existing, code-free safety net specific to -// stdio-piped processes on Unix; it's further evidence Unix process-group -// containment isn't needed to fix #2303's failure mode. +// This test represents github/app#2303: the SDK-hosting GitHub Copilot app +// process exits abruptly, so Client cleanup never runs. The helper starts a +// real CLI client, is terminated through `TerminateProcess`, and relies only +// on Job Object kill-on-close behavior to terminate the CLI. #[cfg(windows)] #[tokio::test] async fn abrupt_host_termination_still_kills_cli_via_job_object() { @@ -288,11 +203,8 @@ async fn abrupt_host_termination_still_kills_cli_via_job_object() { "CLI should be alive before its host process is terminated" ); - // Abruptly terminate the fixture process itself — the - // Windows analogue of the Agency process dying in #2303. // `Child::kill` maps to `TerminateProcess`, which runs none - // of the target process's own code (no `Drop`, no `main` - // unwind). + // of the target process's cleanup code. host.kill().expect("terminate host-crash fixture process"); host.wait().expect("reap host-crash fixture process"); @@ -413,53 +325,6 @@ async fn should_receive_session_updated_lifecycle_event_for_non_ephemeral_activi .await; } -#[cfg(unix)] -async fn wait_for_pid_file(path: &std::path::Path) -> u32 { - super::support::wait_for_condition("wrapper descendant pid file", || async { path.exists() }) - .await; - std::fs::read_to_string(path) - .expect("read wrapper descendant pid") - .trim() - .parse() - .expect("parse wrapper descendant pid") -} - -#[cfg(unix)] -async fn wait_for_process_exit(pid: u32) -> bool { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); - while process_alive(pid) { - if std::time::Instant::now() >= deadline { - return false; - } - tokio::time::sleep(std::time::Duration::from_millis(25)).await; - } - true -} - -#[cfg(unix)] -fn process_alive(pid: u32) -> bool { - #[cfg(target_os = "linux")] - if let Ok(stat) = std::fs::read_to_string(format!("/proc/{pid}/stat")) - && stat - .rsplit_once(") ") - .and_then(|(_, fields)| fields.chars().next()) - .is_some_and(|state| matches!(state, 'Z' | 'X')) - { - return false; - } - - // SAFETY: signal 0 probes process existence without modifying it. - unsafe { libc::kill(pid as i32, 0) == 0 } -} - -#[cfg(unix)] -fn kill_process(pid: u32) { - // SAFETY: the pid came from this test's controlled descendant process. - unsafe { - libc::kill(pid as i32, libc::SIGKILL); - } -} - #[tokio::test] async fn should_receive_session_deleted_lifecycle_event_when_deleted() { with_e2e_context(