fix(rust): prevent orphaned CLI processes

[lukehoban]

Summary

• break the lifecycle-dispatcher ownership cycle that kept ClientInner and its CLI child alive after the last Client was dropped
• enable Tokio kill_on_drop for SDK-spawned CLI commands so startup failures cannot strand a child before ownership reaches ClientInner
• add focused process-level regression tests for both cleanup paths

Fixes github/app#2303.

Validation

• dropping_last_client_kills_spawned_cli fails against the original lifecycle cycle with child survived after its owner was dropped, then passes with the fix
• cargo test --no-default-features --lib dropping_last_client_kills_spawned_cli
• cargo test --no-default-features --lib spawned_child_is_killed_when_dropped

Generated by Copilot

[Copilot]

Pull request overview

Adds cross-platform process-tree ownership and teardown for Rust SDK-spawned CLI processes.

Changes:

• Uses Unix process groups and Windows Job Objects.
• Integrates tree termination into startup, stop, force-stop, and drop paths.
• Adds lifecycle and process-tree tests.

Show a summary per file

File	Description
rust/src/process_tree.rs	Implements process-tree lifecycle management.
rust/src/lib.rs	Integrates managed children into the client lifecycle.
rust/src/errors.rs	Updates teardown error documentation.
rust/Cargo.toml	Adds platform-specific dependencies.
rust/Cargo.lock	Locks the added dependencies.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 3
• Review effort level: Balanced

[stephentoub]

Spent time on this against github/app#2303. I think the fix is already in this diff — it's just not the headline change.

spawn_lifecycle_dispatcher is a reference cycle (lib.rs:1525). The task moves Arc<ClientInner> in and loops on notif_rx.recv(), whose only exit is RecvError::Closed — which requires all senders dropped. The sender is inner.notification_tx, owned by the ClientInner the task holds. The Arc never releases, ClientInner::drop never runs, the child is never killed. That's the orphan. The issue's numbers corroborate: 2,729 reaping idle pooled CLI process vs 252 stopping CLI process — most teardowns never reach stop() and rely entirely on Drop. Since spawn_lifecycle_dispatcher is called from from_transport (lib.rs:1512), the cycle exists from the moment the Client does, so it also strands children on every post-handshake startup failure.

kill_on_drop(true) is a good independent catch. Between spawn_tcp returning and from_transport taking ownership — the TcpStream::connect at lib.rs:1189 and the port-scan wait — the child has no owner that kills it, so a startup failure there leaks it today. The cycle fix doesn't cover that window.

Could we land this PR as just those two changes plus their tests? That fixes #2303 on its own.

On the process-tree work — I don't think it's wrong, but it's solving a different failure mode and I'd like to separate it. The issue reports "61 orphaned hosts with zero descendants," so descendant containment isn't what's broken here. The genuinely unique thing it buys is Windows KILL_ON_JOB_CLOSE: kernel-enforced cleanup when the app dies abnormally, which no amount of Drop/stop() correctness can provide. That's worth having. But:

• The Unix half doesn't provide it. Process groups only die if teardown code runs — the same precondition the cycle fix already satisfies. No protection against host SIGKILL, and any descendant calling setsid() escapes. Worth not describing both as equivalent "SDK-owned process trees."
• It should be all six SDKs, not just Rust. .NET already does Kill(entireProcessTree: true); Node, Python, Go and Java are direct-child only. Landing this in Rust alone makes five of six diverge in a repo that otherwise holds parity.
• The Windows guarantee is available for much less code. PROC_THREAD_ATTRIBUTE_JOB_LIST via STARTUPINFOEX (Win10+) has the kernel place the process in the job atomically at creation. That removes CREATE_SUSPENDED, ResumeThread, the per-spawn system-wide CreateToolhelp32Snapshot, and the assign race it exists to close.
• As written it can turn a leak into an outage. Combined with "preserve startup failures instead of falling back," a transient CreateToolhelp32Snapshot ERROR_BAD_LENGTH (documented, retryable) or a failed AssignProcessToJobObject under a non-breakaway parent job becomes a hard CLI start failure. The attribute-list approach makes that tradeoff disappear.

A few things worth carrying into that PR whenever it happens: the reaper thread in ManagedChild::drop loops with no deadline while the force_stop doc says "finite"; terminate()/wait_for_tree_exit() both .expect() on a tree that wait_for_tree_exit itself takes on success; the Unix terminate() can killpg a recycled pgid; reap_adopted's raw waitpid(-pgid) bypasses Tokio's sole child owner and can make wait() return ECHILD; and github-app/crates/copilot-sdk/Cargo.toml has no libc/windows-sys, so a verbatim sync breaks the app build.

Last thing, and I think it's the highest-leverage one: should the runtime just exit on its own? The logs show [shutdown] Shutdown complete — the clean, non-timeout branch — then serve finished, and the process still survives. On HEAD that path runs shutdown.ts:307-308 → cleanExit → process.exit(code), and runtime-bin/src/main.rs:64 exits unconditionally if copilot_runtime_provider_main() returns. So everything reports success and the process stays up anyway. runExitInterceptor is the one uncapped await before the exit call, and process.exit() here is running inside an embedded Node (dlopen'd runtime.node) rather than the node binary — worth checking whether it actually terminates in that host. Given cleanExit exists as the #2276 0xc0000409 teardown mitigation and #2303 is filed as related, crashing-on-teardown and never-finishing-teardown may be one bug. A watchdog that hard-exits shortly after the serve loop reports finished would fix this for all six SDKs, for non-SDK consumers, and in the exact state #2303 describes where the parent is already gone — which is the one case no SDK-side fix can reach. (Reading HEAD; the report is 1.0.71, so this may already differ.)

Generated by Copilot