Make Rust CLI ownership crash-safe on Windows - #2458
Conversation
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>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unix graceful shutdown can signal a reused process-group ID after reaping the root.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
rust/src/lib.rs — Consume the tree before reaping the root. On Unix, terminate() does not disarm ProcessTree, so… |
What changed in this PR
Adds cross-platform lifecycle ownership for Rust SDK-spawned CLI process trees.
Changes:
- Uses Unix process groups and Windows Job Objects.
- Terminates descendants across stop, force-stop, drop, and startup failure.
- Adds real-process regression tests.
| File | Description |
|---|---|
rust/src/process_tree.rs |
Implements platform-specific process-tree containment. |
rust/src/lib.rs |
Integrates containment into client lifecycle and tests. |
rust/Cargo.toml |
Adds platform-specific system dependencies. |
rust/Cargo.lock |
Locks the new dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
|
Thanks for continuing to tighten this. The process primitive itself is better than #2448, especially on Windows where the root is created suspended, assigned to the Job Object, and only then resumed. However, I still don't see a real E2E proving the scenarios this PR is meant to address. #2448's real CLI validation showed the named cases did not hold up: shell tool children deliberately escape the CLI root process group, MCP servers already exit when stdio closes even without SDK process-tree termination, and subagents do not appear to be separate OS processes. Can we lock in what specific real-world cases this is intended to address, and figure out what sort of E2E test would accurately represent them? My guess is there is some real-world GitHub App scenario that would be affected but we can't tell what that is from this PR, and we would need to be able to explain to other SDK consumers in which cases it deals with cleaning up the child processes it creates. For tracking, I've moved this to draft — please mark as ready to review when appropriate. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Low-level cross-platform process containment requires final validation on native Windows and Unix environments.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
rust/src/lib.rs — Consume the tree before reaping the root. On Unix, terminate() does not disarm ProcessTree, so… View resolved comment |
|
I want to share an updated understanding of the problem here, since it changes what I think this PR should actually contain. github/app#2303, the issue this PR references, is Windows-only, and on inspection it's a crash-safety bug, not a descendant-containment problem: the leaked processes have zero descendants. The CLI host is already killed directly by this SDK's existing None of the four new tests exercise that property, though — they all go through the SDK's own graceful Separately, I'd like to reduce this PR to the Windows-only path unless there's a known real-world scenario where the Unix process-group containment helps on Linux/macOS, demonstrated with its own E2E test. Without that, the Unix side isn't fixing anything we have evidence is broken, and it reintroduces the shell/MCP/subagent rationale that #2448's own investigation showed doesn't hold up, along with real risk (e.g., the PGID-reuse double-terminate issue already flagged on this PR). |
|
@SteveSandersonMS makes sense. I'll update the PR based on your comments 👍 |
- 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>
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>
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>
gimenete
changed the title
|
Addressed in The unsupported Unix process-group and shell/MCP/subagent rationale and tests are removed. The retained Windows E2E starts a real CLI from a separate SDK-hosting fixture, terminates that host with |
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Re-review complete: scoped Windows crash-safety fix is correct and now covered by a real host-crash E2E.
Resolve the process-tree conflicts with #2458 (Windows crash-safe CLI ownership), which landed on main after this branch's last merge. Keep the PR's cross-platform whole-tree ownership (Unix process group + Windows Job Object, active terminate through stop/force_stop/Drop, RAII startup guard) and adopt main's Windows suspended-spawn + resume-after-assign that closes the Job Object assign race. On containment-setup failure the child is still resumed and teardown degrades to root-only rather than failing startup, preserving the PR's reviewed soft-fallback. - rust/Cargo.toml: union windows-sys features (add ToolHelp) and keep the unix libc dependency. - rust/src/process_tree.rs: cross-platform module; Windows configure sets CREATE_SUSPENDED, attach assigns the Job Object then resumes the initial thread; test-only resume_without_containment lets the root-only teardown tests run a child configure spawned suspended. - rust/src/lib.rs: thread extension_launch_provider through from_spawned_transport (main added it to from_transport); keep the guard-based spawn wiring and terminate routing. Validation on Windows (COPILOT_SKIP_CLI_DOWNLOAD=1): - cargo test --no-default-features --features test-support --lib: 230 passed, 0 failed (including all 8 process-lifecycle tests). - cargo +nightly-2026-04-14 fmt --all -- --config-path .rustfmt.nightly.toml --check: clean. - cargo clippy (unwrap_used, disallowed_macros, await_holding_invalid_type): clean on the changed code. - cargo doc -D warnings: clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, and resume them only after assignmentTerminateProcessand verifies the CLI exits through Job Object handle closureValidation
cargo +nightly-2026-04-14 fmt --all -- --config-path .rustfmt.nightly.toml --checkcargo clippy --all-features --all-targets -- -D warningscargo test --lib --all-featurescargo check --target x86_64-pc-windows-msvc --lib --all-featuresRelated: github/app#2303