[Totopo27]

Summary

When stopping a background task runner on Windows, child processes spawned by the launcher shell could escape the Job Object if they were spawned before or concurrently with \AssignProcessToJobObject. This caused descendant commands (such as dev servers or long-running watchers) to remain running in the background after the shell exited (issue #1805).

\ProcessOwnership::terminate\ now takes a ToolHelp32 snapshot of the process list and explicitly terminates all descendant processes under the runner root PID in reverse hierarchy order.

Testing

• Added regression test \stop_terminates_descendant_processes_on_windows\ in \crates/host-core/src/background_tasks.rs.
• \cargo test -p host-core background -- --nocapture\ passes (5 passed).
• \cargo fmt --check\ passes.
• \cargo clippy -p host-core --all-targets\ passes.

Fixes #1805

[vastsa]

The issue is real and the process-tree fallback is aligned with the reported race. I can't merge this yet because the new fallback can silently fail while the stop path still reports success; that leaves the exact reported symptom possible. Please propagate unexpected snapshot/open/terminate failures (continuing cleanup where possible) and exercise a multi-level descendant tree before landing. Thanks for taking this on.

[vastsa]

If snapshot creation fails, this returns without reporting it. ProcessOwnership::terminate then returns only job_result, so when TerminateJobObject succeeds but escaped descendants remain, the caller can still report the task as stopped. Please return an error from this helper and combine it with the Job result so failure to enumerate escaped children remains observable.

[vastsa]

All unrecognized OpenProcess errors are also silently treated as success here, and the subsequent TerminateProcess result is discarded. A transient or permission failure can leave a descendant alive while stop succeeds. Please ignore only the expected already-exited race, attempt cleanup for the remaining descendants, and return an observable failure if any could not be terminated.

[vastsa]

The reported reproduction included several generations of descendants, but this regression only starts and checks one child. Please make the fixture spawn at least one grandchild and assert that the full tree is gone; that verifies the BFS/post-order logic this fix relies on.

[Totopo27]

Updated and rebased onto latest \upstream/main:

• Propagated unexpected snapshot/open/terminate errors from \ erminate_windows_process_tree\ up through \ erminate\ while ensuring cleanup continues across remaining descendant processes.
• Updated the test \stop_terminates_descendant_processes_on_windows\ to exercise a multi-level descendant tree (top-level runner shell -> child PowerShell process -> grandchild PowerShell process) verifying all descendant levels are cleanly terminated upon stopping the background task.
• Validated with \cargo test -p host-core background\ (8/8 passed), \cargo fmt, and \cargo clippy.

[vastsa]

Re-reviewed dd014e8d0: snapshot failures and unexpected errors now propagate, and the multi-level fixture is helpful. One previous root-cause blocker remains: both OpenProcess and TerminateProcess still unconditionally ignore ERROR_ACCESS_DENIED. Access denied does not prove the descendant exited; a live process that cannot be opened/terminated can remain running while stop returns success. Please distinguish verified already-exited races (query/wait where possible) from denied cleanup and propagate the latter while continuing the rest of the tree. Also check the terminal Process32NextW error rather than treating every enumeration failure as normal completion. A regression for the failure path is needed so the caller cannot report successful cleanup with live escaped descendants. This is independent of CI; leaving the PR open for the focused correction.

[Totopo27]

Updated commit ef5291b to address the remaining review blockers:

1. Distinguish verified already-exited races from ERROR_ACCESS_DENIED:

   • OpenProcess and TerminateProcess no longer unconditionally ignore ERROR_ACCESS_DENIED.
   • When encountering ERROR_ACCESS_DENIED, we query PROCESS_QUERY_LIMITED_INFORMATION to verify via GetExitCodeProcess whether the process has genuinely exited (exit_code != STILL_ACTIVE). If the process remains alive or inaccessible, the error is recorded and propagated while allowing cleanup of remaining descendant processes in the tree to continue.
   • ERROR_INVALID_PARAMETER continues to represent an already-exited PID.

2. Validate terminal Process32NextW error:

   • Explicitly check GetLastError() when Process32NextW returns 0. If it fails with anything other than ERROR_NO_MORE_FILES, the snapshot enumeration error is returned rather than being treated as normal completion.

3. Regression test:

   • Added stop_propagates_access_denied_when_descendant_remains_alive to verify that tree termination fails when encountering a live process that cannot be opened/terminated due to access denial, ensuring callers cannot report false success when descendants escape termination.

Validated locally with cargo test -p host-core background (9/9 passed, including multi-level tree and access denied regression tests), cargo fmt, and cargo clippy.

[vastsa]

Thanks for addressing the previous error-handling finding. I still can’t approve this as a production fix because the new regression exercises a different process path from the shipped background-task path, and the PID-only fallback introduces a process-safety risk.

In production, spawn_tool_runner starts the host executable in hidden-runner mode, assigns that process to the Job Object, and only then writes the configuration frame. The hidden runner starts the requested shell after reading that frame. The #[cfg(test)] path instead spawns the shell directly, so its assign-after-spawn race does not establish that the reported leak happens in production. Please reproduce the failure through the real hidden-runner flow and keep the fix scoped to that demonstrated failure.

Separately, this helper terminates bare PIDs from a system-wide snapshot and always includes root_pid, even if that process is already absent. Since TerminateJobObject runs first and the task process may have exited, a PID can be reused before OpenProcess; this code could then terminate an unrelated process. Please preserve and verify process identity/ownership before terminating snapshot entries, or avoid the PID sweep if the production reproduction does not require it.

[vastsa]

The shipped path assigns the hidden runner to the Job Object before sending its shell configuration; the new test bypasses that gate with the #[cfg(test)] direct-shell path. Also, after TerminateJobObject, a bare reused root_pid can identify an unrelated process. Please validate the defect through the production runner and retain stable process identity before terminating this snapshot.

[vastsa]

CI 的失败点与评审无关，是架构检查：crates/host-core/src/background_tasks.rs 因为新增的 Windows 回归测试涨到 1109 行，超过 Rust 1000 行上限。我按仓库既有做法把这个测试挪出该文件后会自己推进合入。

[vastsa]

两个阻塞点已在 c579338 处理，并附带 CI 的架构检查修复。

1. PID-only 风险：终止前先证明身份。每个快照条目都用 PROCESS_TERMINATE | PROCESS_QUERY_LIMITED_INFORMATION 打开句柄，句柄退出码已不是 STILL_ACTIVE 就跳过；再拿句柄的创建时间与「快照之后立刻取的 watermark」比较，创建时间更晚的进程一律不终止——这才是复用 PID 唯一可能对应的进程。活着的成员被拒绝访问时按失败上报（不再当作已退出），其余子树继续清理。

2. 测试路径：回归改为走真实生产链 spawn_background_task_detached → hidden runner → shell → 命令 → 命令自己再起的子进程，断言停止后两层后代都不在了；不再直接 spawn shell。

3. CI 失败点：架构检查报 background_tasks.rs 1109 行超限，把 Windows 回归测试移到同目录子模块后该文件回落到 890 行。

本地：cargo test -p host-core --locked 1077 passed、cargo fmt --check、clippy（native 与 x86_64-pc-windows-gnu 均只剩 main 既有 warning）、node scripts/check-architecture.mjs 通过。已把分支合并到最新 main。

[vastsa]

两个阻塞点都已验证修复：终止前用句柄退出码与创建时间 watermark 证明身份（复用 PID 不可能被误杀），活着的成员被拒绝访问按失败上报；回归改走真实 hidden runner → shell → 命令 → 子进程链，断言两层后代都被终止。CI 全绿（含 Windows clippy/构建）。合入。

[muzimu217]

Heads-up: the Windows host-core job fails on main at the merge of this PR, with a Bash execution timing out.

Run 40694217868 on 68868dfc8:

test rpc::visualization_tests::fork::rpc_bash_created_visualization_survives_relocation_fork_and_source_deletion ... FAILED
panicked at crates\host-core\src\rpc\visualization_tests\fork.rs:49:6:
the fixture approves Bash and it finishes within its execution budget: Elapsed(())

Elapsed(()) is the timeout arm, not an assertion on the result — the tools.execute call for bash-chart never returned within the fixture's budget. The same job passed on the immediately preceding head (b91f35122), and the merge only touches crates/host-core/src/background_tasks.rs, Cargo.toml (one new dependency) and background_tasks/windows_tests.rs.

That points at process handling rather than at the visualization fixture: if terminating the background task tree on Windows now takes a lock, walks the process table, or waits on a descendant set that a foreground Bash execution also touches, a short-budget tools.execute would see exactly this. Worth checking whether the new descendant-identification path runs on every spawn or only on termination, and whether it holds anything a concurrent tool execution needs.

Not claiming it is definitely this change — Windows runners are timing-sensitive — but "passed on the previous head, failed on this one, only this file changed" is the cheapest place to look first.

[muzimu217]

Following up on my regression note: it did not reproduce, so treat it as a flake rather than a defect from this change.

79fb39b45 (the #1961 radial-menu merge, which does not touch background_tasks.rs) passes the whole Rust host-core Windows / transcript cache E2E job, including the rpc_bash_created_visualization_survives_relocation_fork_and_source_deletion test that timed out on 68868dfc8 with Elapsed(()).

That is one failure and one pass on Windows, so the evidence no longer supports "this merge broke foreground Bash". The timeout was most likely the Windows runner being slow to complete the Bash execution within the fixture budget — the same class as the longHistory probe, which also fails with identical numbers on some heads and passes on others.

I would still keep the question I raised (whether descendant identification runs on every spawn and whether it holds anything a concurrent tool execution needs) as something to confirm by reading, but it should not block this PR or be treated as a confirmed regression.