Skip to content

Commit 76f6b6d

Browse files
gimeneteCopilotSteveSandersonMS
authored
Make Rust CLI ownership crash-safe on Windows (#2458)
* 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> * 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> * 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> * 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> * 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> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
1 parent 6b0252c commit 76f6b6d

6 files changed

Lines changed: 471 additions & 25 deletions

File tree

‎rust/Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎rust/Cargo.toml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,12 @@ tokio-tungstenite = { version = "0.24", default-features = false, features = ["c
7070

7171
[target.'cfg(windows)'.dependencies]
7272
zip = { version = "2", default-features = false, features = ["deflate"], optional = true }
73+
windows-sys = { version = "0.61", default-features = false, features = [
74+
"Win32_Foundation",
75+
"Win32_System_Diagnostics_ToolHelp",
76+
"Win32_System_JobObjects",
77+
"Win32_System_Threading",
78+
] }
7379

7480
[dev-dependencies]
7581
rusqlite = { version = "0.35", features = ["bundled"] }
@@ -105,6 +111,13 @@ required-features = ["test-support"]
105111
test = false
106112
bench = false
107113

114+
[[bin]]
115+
name = "copilot-host-crash-fixture"
116+
path = "tests/fixtures/host_crash_fixture.rs"
117+
required-features = ["test-support"]
118+
test = false
119+
bench = false
120+
108121
[build-dependencies]
109122
base64 = "0.22"
110123
dirs = "5"

‎rust/src/lib.rs‎

Lines changed: 55 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ pub mod hooks;
3333
mod jsonrpc;
3434
/// Permission-policy helpers that produce a [`handler::PermissionHandler`].
3535
pub mod permission;
36+
mod process_tree;
3637
/// BYOK bearer-token provider callbacks.
3738
pub mod provider_token;
3839
mod provider_token_dispatch;
@@ -1066,6 +1067,7 @@ impl std::fmt::Debug for Client {
10661067

10671068
struct ClientInner {
10681069
child: parking_lot::Mutex<Option<Child>>,
1070+
process_tree: parking_lot::Mutex<Option<process_tree::ProcessTree>>,
10691071
#[cfg(feature = "bundled-in-process")]
10701072
/// In-process FFI runtime host, set only for [`Transport::InProcess`].
10711073
/// Closing it tears down the native runtime connection.
@@ -1302,6 +1304,7 @@ impl Client {
13021304
reader,
13031305
writer,
13041306
None,
1307+
None,
13051308
working_directory,
13061309
options.on_list_models,
13071310
extension_launch_provider.clone(),
@@ -1317,7 +1320,7 @@ impl Client {
13171320
port,
13181321
connection_token: _,
13191322
} => {
1320-
let (mut child, actual_port, spawn_elapsed, port_wait_elapsed) =
1323+
let (mut child, tree, actual_port, spawn_elapsed, port_wait_elapsed) =
13211324
Self::spawn_tcp(&program, &options, &working_directory, port).await?;
13221325
timings.process_spawn_ms = Some(StartupTimings::millis(spawn_elapsed));
13231326
timings.port_wait_ms = Some(StartupTimings::millis(port_wait_elapsed));
@@ -1334,6 +1337,7 @@ impl Client {
13341337
reader,
13351338
writer,
13361339
Some(child),
1340+
tree,
13371341
working_directory,
13381342
options.on_list_models,
13391343
extension_launch_provider.clone(),
@@ -1346,7 +1350,7 @@ impl Client {
13461350
)?
13471351
}
13481352
Transport::Stdio => {
1349-
let (mut child, spawn_elapsed) =
1353+
let (mut child, tree, spawn_elapsed) =
13501354
Self::spawn_stdio(&program, &options, &working_directory)?;
13511355
timings.process_spawn_ms = Some(StartupTimings::millis(spawn_elapsed));
13521356
let stdin = child.stdin.take().expect("stdin is piped");
@@ -1356,6 +1360,7 @@ impl Client {
13561360
stdout,
13571361
stdin,
13581362
Some(child),
1363+
tree,
13591364
working_directory,
13601365
options.on_list_models,
13611366
extension_launch_provider.clone(),
@@ -1422,6 +1427,7 @@ impl Client {
14221427
reader,
14231428
writer,
14241429
None,
1430+
None,
14251431
working_directory,
14261432
options.on_list_models,
14271433
extension_launch_provider.clone(),
@@ -1563,6 +1569,7 @@ impl Client {
15631569
reader,
15641570
writer,
15651571
None,
1572+
None,
15661573
cwd,
15671574
None,
15681575
None,
@@ -1589,6 +1596,7 @@ impl Client {
15891596
reader,
15901597
writer,
15911598
None,
1599+
None,
15921600
cwd,
15931601
None,
15941602
Some(provider),
@@ -1619,6 +1627,7 @@ impl Client {
16191627
reader,
16201628
writer,
16211629
None,
1630+
None,
16221631
cwd,
16231632
None,
16241633
None,
@@ -1645,6 +1654,7 @@ impl Client {
16451654
reader,
16461655
writer,
16471656
None,
1657+
None,
16481658
cwd,
16491659
None,
16501660
None,
@@ -1671,6 +1681,7 @@ impl Client {
16711681
reader,
16721682
writer,
16731683
None,
1684+
None,
16741685
cwd,
16751686
None,
16761687
None,
@@ -1698,6 +1709,7 @@ impl Client {
16981709
reader: impl AsyncRead + Unpin + Send + 'static,
16991710
writer: impl AsyncWrite + Unpin + Send + 'static,
17001711
child: Option<Child>,
1712+
process_tree: Option<process_tree::ProcessTree>,
17011713
cwd: PathBuf,
17021714
on_list_models: Option<Arc<dyn ListModelsHandler>>,
17031715
extension_launch_provider: Option<
@@ -1732,6 +1744,7 @@ impl Client {
17321744
let client = Self {
17331745
inner: Arc::new(ClientInner {
17341746
child: parking_lot::Mutex::new(child),
1747+
process_tree: parking_lot::Mutex::new(process_tree),
17351748
#[cfg(feature = "bundled-in-process")]
17361749
ffi_host: parking_lot::Mutex::new(None),
17371750
rpc,
@@ -1870,13 +1883,6 @@ impl Client {
18701883
.stdout(Stdio::piped())
18711884
.stderr(Stdio::piped());
18721885

1873-
#[cfg(windows)]
1874-
{
1875-
use std::os::windows::process::CommandExt;
1876-
const CREATE_NO_WINDOW: u32 = 0x08000000;
1877-
command.as_std_mut().creation_flags(CREATE_NO_WINDOW);
1878-
}
1879-
18801886
command
18811887
}
18821888

@@ -1933,7 +1939,7 @@ impl Client {
19331939
program: &Path,
19341940
options: &ClientOptions,
19351941
working_directory: &Path,
1936-
) -> Result<(Child, Duration)> {
1942+
) -> Result<(Child, Option<process_tree::ProcessTree>, Duration)> {
19371943
info!(cwd = ?working_directory, program = %program.display(), "spawning copilot CLI (stdio)");
19381944
let mut command = Self::build_command(program, options, working_directory);
19391945
command
@@ -1945,21 +1951,27 @@ impl Client {
19451951
.args(&options.extra_args)
19461952
.stdin(Stdio::piped());
19471953
let spawn_start = Instant::now();
1948-
let child = command.spawn()?;
1954+
let (child, tree) = process_tree::spawn(&mut command)?;
19491955
let spawn_elapsed = spawn_start.elapsed();
19501956
debug!(
19511957
elapsed_ms = spawn_elapsed.as_millis(),
19521958
"Client::spawn_stdio subprocess spawned"
19531959
);
1954-
Ok((child, spawn_elapsed))
1960+
Ok((child, tree, spawn_elapsed))
19551961
}
19561962

19571963
async fn spawn_tcp(
19581964
program: &Path,
19591965
options: &ClientOptions,
19601966
working_directory: &Path,
19611967
port: u16,
1962-
) -> Result<(Child, u16, Duration, Duration)> {
1968+
) -> Result<(
1969+
Child,
1970+
Option<process_tree::ProcessTree>,
1971+
u16,
1972+
Duration,
1973+
Duration,
1974+
)> {
19631975
info!(cwd = ?working_directory, program = %program.display(), port = %port, "spawning copilot CLI (tcp)");
19641976
let mut command = Self::build_command(program, options, working_directory);
19651977
command
@@ -1971,7 +1983,7 @@ impl Client {
19711983
.args(&options.extra_args)
19721984
.stdin(Stdio::null());
19731985
let spawn_start = Instant::now();
1974-
let mut child = command.spawn()?;
1986+
let (mut child, tree) = process_tree::spawn(&mut command)?;
19751987
let spawn_elapsed = spawn_start.elapsed();
19761988
debug!(
19771989
elapsed_ms = spawn_elapsed.as_millis(),
@@ -2018,7 +2030,7 @@ impl Client {
20182030
"Client::spawn_tcp TCP port wait complete"
20192031
);
20202032
info!(port = %actual_port, "CLI server listening");
2021-
Ok((child, actual_port, spawn_elapsed, port_wait_elapsed))
2033+
Ok((child, tree, actual_port, spawn_elapsed, port_wait_elapsed))
20222034
}
20232035

20242036
fn drain_stderr(child: &mut Child) {
@@ -2561,11 +2573,12 @@ impl Client {
25612573
/// Cooperatively shut down the client and the CLI child process.
25622574
///
25632575
/// Walks every still-registered session and sends `session.destroy`
2564-
/// for each one, asks SDK-owned runtimes to shut down, then kills the
2565-
/// CLI child. Errors from per-session destroys, runtime shutdown, and
2566-
/// the final child-kill are collected into
2567-
/// [`StopErrors`] rather than short-circuiting on the first failure
2568-
/// — so callers see the full picture of teardown.
2576+
/// for each one, asks SDK-owned runtimes to shut down, terminates the
2577+
/// Windows-owned CLI Job Object when present, and reaps the root process.
2578+
/// Errors from per-session destroys, runtime shutdown, and final process
2579+
/// termination are collected into [`StopErrors`] rather than
2580+
/// short-circuiting on the first failure — so callers see the full picture
2581+
/// of teardown.
25692582
///
25702583
/// If you have already called [`Session::disconnect`] on every
25712584
/// session this client created, the per-session destroy step is a
@@ -2654,8 +2667,14 @@ impl Client {
26542667
}
26552668

26562669
let child = self.inner.child.lock().take();
2670+
let process_tree = self.inner.process_tree.lock().take();
26572671
*self.inner.state.lock() = ConnectionState::Disconnected;
26582672
*self.inner.models_cache.lock() = Arc::new(tokio::sync::OnceCell::new());
2673+
if let Some(process_tree) = process_tree
2674+
&& let Err(error) = process_tree.terminate()
2675+
{
2676+
errors.push(error.into());
2677+
}
26592678
if let Some(mut child) = child {
26602679
match child.try_wait() {
26612680
Ok(Some(_status)) => {}
@@ -2696,10 +2715,9 @@ impl Client {
26962715
///
26972716
/// Synchronous fallback when [`stop`](Self::stop) is unsuitable — for
26982717
/// example when the awaiting tokio runtime is shutting down or the
2699-
/// process is wedged on I/O. Sends a kill signal without awaiting
2700-
/// reaper completion and immediately drops all per-session router
2701-
/// state so dependent tasks observe a closed channel rather than a
2702-
/// hang.
2718+
/// process is wedged on I/O. Terminates the Windows-owned CLI Job Object
2719+
/// when present and immediately drops all per-session router state so
2720+
/// dependent tasks observe a closed channel rather than a hang.
27032721
///
27042722
/// # Cancel safety
27052723
///
@@ -2725,6 +2743,11 @@ impl Client {
27252743
let pid = self.pid();
27262744
info!(pid = ?pid, "force-stopping CLI process");
27272745
self.inner.extension_launch_provider.clear();
2746+
if let Some(process_tree) = self.inner.process_tree.lock().take()
2747+
&& let Err(error) = process_tree.terminate()
2748+
{
2749+
error!(pid = ?pid, %error, "failed to terminate CLI process tree");
2750+
}
27282751
if let Some(mut child) = self.inner.child.lock().take()
27292752
&& let Err(e) = child.start_kill()
27302753
{
@@ -2786,8 +2809,13 @@ impl Client {
27862809

27872810
impl Drop for ClientInner {
27882811
fn drop(&mut self) {
2812+
let pid = self.child.lock().as_ref().and_then(Child::id);
2813+
if let Some(process_tree) = self.process_tree.lock().take()
2814+
&& let Err(error) = process_tree.terminate()
2815+
{
2816+
error!(pid = ?pid, %error, "failed to terminate CLI process tree on drop");
2817+
}
27892818
if let Some(ref mut child) = *self.child.lock() {
2790-
let pid = child.id();
27912819
if let Err(e) = child.start_kill() {
27922820
error!(pid = ?pid, error = %e, "failed to kill CLI process on drop");
27932821
} else {
@@ -3447,6 +3475,7 @@ mod tests {
34473475
client_read,
34483476
client_write,
34493477
Some(child),
3478+
None,
34503479
temp.path().to_path_buf(),
34513480
None,
34523481
None,
@@ -3536,6 +3565,7 @@ mod tests {
35363565
Client {
35373566
inner: Arc::new(ClientInner {
35383567
child: parking_lot::Mutex::new(None),
3568+
process_tree: parking_lot::Mutex::new(None),
35393569
#[cfg(feature = "bundled-in-process")]
35403570
ffi_host: parking_lot::Mutex::new(None),
35413571
rpc: {

0 commit comments

Comments
 (0)