From 6896802d75b24aef271fbfd05b0b02f32fc30b33 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 1 Feb 2026 13:17:41 +0100 Subject: [PATCH] feat(process-management): implemented native process tree management with platform-specific implementations - Added native process tree killing and listing functions via new `ps` module in pi-natives crate with platform-specific implementations (Linux /proc, macOS libproc, Windows CreateToolhelp32Snapshot). - Refactored process termination logic to prioritize native process tree killing when available, improving reliability of process cleanup across platforms. - Removed detached process spawning across codebase (bash-executor, shell-session, kernel, lsp client, markitdown conversion) in favor of native process tree management. - Simplified ptree module by removing managed children tracking and postmortem cleanup hooks, delegating process lifecycle management to native implementations. --- Cargo.lock | 1 + crates/pi-natives/Cargo.toml | 3 + crates/pi-natives/src/lib.rs | 1 + crates/pi-natives/src/ps.rs | 223 ++++++++++++++++++ packages/ai/test/context-overflow.test.ts | 1 - packages/ai/test/stream.test.ts | 1 - .../coding-agent/src/exec/bash-executor.ts | 1 - .../coding-agent/src/exec/shell-session.ts | 1 - .../src/ipy/gateway-coordinator.ts | 6 +- packages/coding-agent/src/ipy/kernel.ts | 1 - packages/coding-agent/src/lsp/client.ts | 1 - packages/coding-agent/src/tools/read.ts | 1 - .../coding-agent/src/web/scrapers/utils.ts | 1 - packages/natives/CHANGELOG.md | 3 + packages/natives/src/index.ts | 33 +++ packages/natives/src/native.ts | 4 + packages/utils/src/index.ts | 1 + packages/utils/src/procmgr.ts | 54 +++-- packages/utils/src/ptree.ts | 28 +-- 19 files changed, 309 insertions(+), 56 deletions(-) create mode 100644 crates/pi-natives/src/ps.rs diff --git a/Cargo.lock b/Cargo.lock index f7b790a93..932271090 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -842,6 +842,7 @@ dependencies = [ "html-to-markdown-rs", "ignore", "image", + "libc", "napi", "napi-build", "napi-derive", diff --git a/crates/pi-natives/Cargo.toml b/crates/pi-natives/Cargo.toml index ccb3bdbe3..d3cc910ae 100644 --- a/crates/pi-natives/Cargo.toml +++ b/crates/pi-natives/Cargo.toml @@ -38,5 +38,8 @@ syntect = { version = "5.3", default-features = false, features = [ html-to-markdown-rs = { version = "2.24", default-features = false } phf = { version = "0.11", features = ["macros"] } +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [build-dependencies] napi-build = "2" diff --git a/crates/pi-natives/src/lib.rs b/crates/pi-natives/src/lib.rs index 3e6b83fc1..5a1a2c7ce 100644 --- a/crates/pi-natives/src/lib.rs +++ b/crates/pi-natives/src/lib.rs @@ -26,4 +26,5 @@ pub mod highlight; pub mod html; pub mod image; pub mod keys; +pub mod ps; pub mod text; diff --git a/crates/pi-natives/src/ps.rs b/crates/pi-natives/src/ps.rs new file mode 100644 index 000000000..677967a1e --- /dev/null +++ b/crates/pi-natives/src/ps.rs @@ -0,0 +1,223 @@ +//! Cross-platform process tree management. +//! +//! Provides efficient process tree enumeration and termination without requiring +//! processes to be spawned with `detached: true`. +//! +//! # Platform Implementation +//! - **Linux**: Reads `/proc/{pid}/children` recursively +//! - **macOS**: Uses `libproc` (`proc_listchildpids`) +//! - **Windows**: Uses `CreateToolhelp32Snapshot` to build parent-child relationships +//! +//! # Example +//! ```ignore +//! use pi_natives::ps::kill_tree; +//! +//! // Kill process 1234 and all its descendants +//! let killed = kill_tree(1234, 9); // SIGKILL +//! ``` + +use napi_derive::napi; + +#[cfg(target_os = "linux")] +mod platform { + use std::fs; + + /// Recursively collect all descendant PIDs by reading /proc/{pid}/children. + pub fn collect_descendants(pid: i32, pids: &mut Vec) { + let children_path = format!("/proc/{pid}/task/{pid}/children"); + let Ok(content) = fs::read_to_string(&children_path) else { + return; + }; + + for part in content.split_whitespace() { + if let Ok(child_pid) = part.parse::() { + pids.push(child_pid); + collect_descendants(child_pid, pids); + } + } + } + + /// Kill a process with the given signal. + pub fn kill_pid(pid: i32, signal: i32) -> bool { + // SAFETY: libc::kill is safe to call with any pid/signal combination + unsafe { libc::kill(pid, signal) == 0 } + } +} + +#[cfg(target_os = "macos")] +mod platform { + use std::ptr; + + #[link(name = "proc", kind = "dylib")] + extern "C" { + fn proc_listchildpids(ppid: i32, buffer: *mut i32, buffersize: i32) -> i32; + } + + /// Recursively collect all descendant PIDs using libproc. + pub fn collect_descendants(pid: i32, pids: &mut Vec) { + // First call to get count + let count = unsafe { proc_listchildpids(pid, ptr::null_mut(), 0) }; + if count <= 0 { + return; + } + + let mut buffer = vec![0i32; count as usize]; + let actual = unsafe { + proc_listchildpids( + pid, + buffer.as_mut_ptr(), + (buffer.len() * size_of::()) as i32, + ) + }; + + if actual <= 0 { + return; + } + + let child_count = actual as usize / size_of::(); + for &child_pid in &buffer[..child_count] { + if child_pid > 0 { + pids.push(child_pid); + collect_descendants(child_pid, pids); + } + } + } + + /// Kill a process with the given signal. + pub fn kill_pid(pid: i32, signal: i32) -> bool { + // SAFETY: libc::kill is safe to call with any pid/signal combination + unsafe { libc::kill(pid, signal) == 0 } + } +} + +#[cfg(target_os = "windows")] +mod platform { + use std::collections::HashMap; + use std::mem; + use std::ptr; + + #[repr(C)] + #[allow(non_snake_case)] + struct PROCESSENTRY32W { + dwSize: u32, + cntUsage: u32, + th32ProcessID: u32, + th32DefaultHeapID: usize, + th32ModuleID: u32, + cntThreads: u32, + th32ParentProcessID: u32, + pcPriClassBase: i32, + dwFlags: u32, + szExeFile: [u16; 260], + } + + type HANDLE = *mut std::ffi::c_void; + const INVALID_HANDLE_VALUE: HANDLE = -1isize as HANDLE; + const TH32CS_SNAPPROCESS: u32 = 0x00000002; + const PROCESS_TERMINATE: u32 = 0x0001; + + #[link(name = "kernel32")] + extern "system" { + fn CreateToolhelp32Snapshot(dwFlags: u32, th32ProcessID: u32) -> HANDLE; + fn Process32FirstW(hSnapshot: HANDLE, lppe: *mut PROCESSENTRY32W) -> i32; + fn Process32NextW(hSnapshot: HANDLE, lppe: *mut PROCESSENTRY32W) -> i32; + fn CloseHandle(hObject: HANDLE) -> i32; + fn OpenProcess(dwDesiredAccess: u32, bInheritHandle: i32, dwProcessId: u32) -> HANDLE; + fn TerminateProcess(hProcess: HANDLE, uExitCode: u32) -> i32; + } + + /// Build a map of parent_pid -> [child_pids] for all processes. + fn build_process_tree() -> HashMap> { + let mut tree: HashMap> = HashMap::new(); + + unsafe { + let snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); + if snapshot == INVALID_HANDLE_VALUE { + return tree; + } + + let mut entry: PROCESSENTRY32W = mem::zeroed(); + entry.dwSize = mem::size_of::() as u32; + + if Process32FirstW(snapshot, &mut entry) != 0 { + loop { + tree.entry(entry.th32ParentProcessID) + .or_default() + .push(entry.th32ProcessID); + + if Process32NextW(snapshot, &mut entry) == 0 { + break; + } + } + } + + CloseHandle(snapshot); + } + + tree + } + + /// Recursively collect all descendant PIDs. + pub fn collect_descendants(pid: i32, pids: &mut Vec) { + let tree = build_process_tree(); + collect_descendants_from_tree(pid as u32, &tree, pids); + } + + fn collect_descendants_from_tree(pid: u32, tree: &HashMap>, pids: &mut Vec) { + if let Some(children) = tree.get(&pid) { + for &child_pid in children { + pids.push(child_pid as i32); + collect_descendants_from_tree(child_pid, tree, pids); + } + } + } + + /// Kill a process (signal is ignored on Windows, always terminates). + pub fn kill_pid(pid: i32, _signal: i32) -> bool { + unsafe { + let handle = OpenProcess(PROCESS_TERMINATE, 0, pid as u32); + if handle.is_null() || handle == INVALID_HANDLE_VALUE { + return false; + } + let result = TerminateProcess(handle, 1); + CloseHandle(handle); + result != 0 + } + } +} + +/// Kill a process tree (the process and all its descendants). +/// +/// Kills children first (bottom-up) to prevent orphan re-parenting issues. +/// Returns the number of processes successfully killed. +#[napi] +pub fn kill_tree(pid: i32, signal: i32) -> u32 { + let mut descendants = Vec::new(); + platform::collect_descendants(pid, &mut descendants); + + let mut killed = 0u32; + + // Kill children first (deepest first by reversing the DFS order) + for &child_pid in descendants.iter().rev() { + if platform::kill_pid(child_pid, signal) { + killed += 1; + } + } + + // Kill the root process last + if platform::kill_pid(pid, signal) { + killed += 1; + } + + killed +} + +/// List all descendant PIDs of a process. +/// +/// Returns an empty array if the process has no children or doesn't exist. +#[napi] +pub fn list_descendants(pid: i32) -> Vec { + let mut descendants = Vec::new(); + platform::collect_descendants(pid, &mut descendants); + descendants +} diff --git a/packages/ai/test/context-overflow.test.ts b/packages/ai/test/context-overflow.test.ts index 91d16b1e5..bff206a85 100644 --- a/packages/ai/test/context-overflow.test.ts +++ b/packages/ai/test/context-overflow.test.ts @@ -476,7 +476,6 @@ describe("Context overflow error handling", () => { // Start ollama server ollamaProcess = spawn("ollama", ["serve"], { - detached: false, stdio: "ignore", }); diff --git a/packages/ai/test/stream.test.ts b/packages/ai/test/stream.test.ts index ca6dde232..eeba6d634 100644 --- a/packages/ai/test/stream.test.ts +++ b/packages/ai/test/stream.test.ts @@ -1379,7 +1379,6 @@ describe("Generate E2E Tests", () => { // Start ollama server ollamaProcess = spawn("ollama", ["serve"], { - detached: false, stdio: "ignore", }); diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index 1cfb7fcb4..1b4457823 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -105,7 +105,6 @@ async function executeBashOnce( env: finalEnv, signal: options?.signal, timeout: options?.timeout, - detached: true, }); // Pump streams - errors during abort/timeout are expected diff --git a/packages/coding-agent/src/exec/shell-session.ts b/packages/coding-agent/src/exec/shell-session.ts index c5525638a..f3037ae21 100644 --- a/packages/coding-agent/src/exec/shell-session.ts +++ b/packages/coding-agent/src/exec/shell-session.ts @@ -254,7 +254,6 @@ class ShellSession { this.#child = ptree.spawn([this.config.shell, ...args], { stdin: "pipe", env: this.config.env, - detached: !IS_WINDOWS, }); if (this.#child.proc.exitCode !== null) { diff --git a/packages/coding-agent/src/ipy/gateway-coordinator.ts b/packages/coding-agent/src/ipy/gateway-coordinator.ts index 86f4b8fdb..104321a1d 100644 --- a/packages/coding-agent/src/ipy/gateway-coordinator.ts +++ b/packages/coding-agent/src/ipy/gateway-coordinator.ts @@ -265,8 +265,8 @@ async function startGatewayProcess( stdin: "ignore", stdout: "pipe", stderr: "pipe", - detached: true, windowsHide: true, + detached: true, env: kernelEnv, }, ); @@ -296,13 +296,13 @@ async function startGatewayProcess( await Bun.sleep(100); } - await procmgr.terminate({ target: gatewayProcess, group: true }); + gatewayProcess.kill(); throw new Error("Gateway startup timeout"); } async function killGateway(pid: number, context: string): Promise { try { - await procmgr.terminate({ target: pid, group: true }); + await procmgr.terminate({ target: pid }); } catch (err) { logger.warn("Failed to kill shared gateway process", { error: err instanceof Error ? err.message : String(err), diff --git a/packages/coding-agent/src/ipy/kernel.ts b/packages/coding-agent/src/ipy/kernel.ts index c38f50132..30786658f 100644 --- a/packages/coding-agent/src/ipy/kernel.ts +++ b/packages/coding-agent/src/ipy/kernel.ts @@ -490,7 +490,6 @@ export class PythonKernel { { cwd: options.cwd, env: kernelEnv, - detached: true, }, ); diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index 0f22ee3f2..60f79df5f 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -411,7 +411,6 @@ export async function getOrCreateClient(config: ServerConfig, cwd: string, initT const proc = ptree.spawn([command, ...args], { cwd, - detached: true, stdin: "pipe", env: env ? { ...process.env, ...env } : undefined, }); diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index c39eabef1..65045f064 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -497,7 +497,6 @@ async function convertWithMarkitdown( allowNonZero: true, allowAbort: true, stderr: "buffer", - detached: true, }); if (result.exitError?.aborted) { diff --git a/packages/coding-agent/src/web/scrapers/utils.ts b/packages/coding-agent/src/web/scrapers/utils.ts index 01e9b3241..604c4fe68 100644 --- a/packages/coding-agent/src/web/scrapers/utils.ts +++ b/packages/coding-agent/src/web/scrapers/utils.ts @@ -54,7 +54,6 @@ export async function convertWithMarkitdown( signal, allowNonZero: true, stderr: "full", - detached: true, }); if (!result.ok) { return { diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 30ad47b29..3cb3f3fdf 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -1,8 +1,11 @@ # Changelog ## [Unreleased] + ### Added +- Exported `killTree` function to kill a process and all its descendants using platform-native APIs +- Exported `listDescendants` function to list all descendant PIDs of a process - Added `dev:native` npm script to build debug native binaries with `--dev` flag - Added `OMP_DEV` environment variable support for loading and debugging development native builds - Exported keyboard parsing and matching functions: `parseKey`, `parseKittySequence`, `matchesLegacySequence`, and `matchesKey` for terminal input handling diff --git a/packages/natives/src/index.ts b/packages/natives/src/index.ts index f41733a8b..d16e21b07 100644 --- a/packages/natives/src/index.ts +++ b/packages/natives/src/index.ts @@ -3,9 +3,12 @@ */ import * as path from "node:path"; +import { setNativeKillTree } from "@oh-my-pi/pi-utils"; import type { FindMatch, FindOptions, FindResult } from "./find/types"; import { native } from "./native"; +setNativeKillTree(native.killTree); + // ============================================================================= // Grep (ripgrep-based regex search) // ============================================================================= @@ -115,3 +118,33 @@ export { type HtmlToMarkdownOptions, htmlToMarkdown, } from "./html/index"; + +// ============================================================================= +// Process management +// ============================================================================= + +/** + * Kill a process and all its descendants. + * + * Uses platform-native APIs for efficiency: + * - Linux: /proc/{pid}/children + * - macOS: libproc (proc_listchildpids) + * - Windows: CreateToolhelp32Snapshot + * + * @param pid - Process ID to kill + * @param signal - Signal number (e.g., 9 for SIGKILL). Ignored on Windows. + * @returns Number of processes successfully killed + */ +export function killTree(pid: number, signal: number): number { + return native.killTree(pid, signal); +} + +/** + * List all descendant PIDs of a process. + * + * @param pid - Process ID to query + * @returns Array of descendant PIDs (children, grandchildren, etc.) + */ +export function listDescendants(pid: number): number[] { + return native.listDescendants(pid); +} diff --git a/packages/natives/src/native.ts b/packages/natives/src/native.ts index d5e796f00..65ead615b 100644 --- a/packages/natives/src/native.ts +++ b/packages/natives/src/native.ts @@ -90,6 +90,8 @@ export interface NativeBindings { matchesLegacySequence(data: string, keyName: string): boolean; parseKittySequence(data: string): ParsedKittyResult | null; matchesKey(data: string, keyId: string, kittyProtocolActive: boolean): boolean; + killTree(pid: number, signal: number): number; + listDescendants(pid: number): number[]; } const require = createRequire(import.meta.url); @@ -178,6 +180,8 @@ function validateNative(bindings: NativeBindings, source: string): void { checkFn("parseKittySequence"); checkFn("matchesKey"); checkFn("visibleWidth"); + checkFn("killTree"); + checkFn("listDescendants"); if (missing.length) { throw new Error( diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 39bd3e1f5..cafac90f2 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -5,6 +5,7 @@ export * from "./glob"; export * as logger from "./logger"; export * as postmortem from "./postmortem"; export * as procmgr from "./procmgr"; +export { setNativeKillTree } from "./procmgr"; export * as ptree from "./ptree"; export { AbortError, ChildProcess, Exception, NonZeroExitError } from "./ptree"; export * from "./stream"; diff --git a/packages/utils/src/procmgr.ts b/packages/utils/src/procmgr.ts index 00662fd54..2782ce27d 100644 --- a/packages/utils/src/procmgr.ts +++ b/packages/utils/src/procmgr.ts @@ -184,13 +184,32 @@ export function getShellConfig(customShellPath?: string): ShellConfig { return cachedShellConfig; } +/** + * Function signature for native process tree killing. + * Returns the number of processes killed. + */ +export type KillTreeFn = (pid: number, signal: number) => number; + +/** + * Global native kill tree function, injected by pi-natives when loaded. + * Falls back to platform-specific behavior if not set. + */ +export let nativeKillTree: KillTreeFn | undefined; + +/** + * Set the native kill tree function. Called by pi-natives on load. + */ +export function setNativeKillTree(fn: KillTreeFn): void { + nativeKillTree = fn; +} + /** * Options for terminating a process and all its descendants. */ export interface TerminateOptions { /** The process to terminate */ target: Subprocess | number; - /** Whether to terminate the process group (Windows only) */ + /** Whether to terminate the process tree (all descendants) */ group?: boolean; /** Timeout in milliseconds */ timeout?: number; @@ -293,31 +312,22 @@ export async function terminate(options: TerminateOptions): Promise { } } catch {} - if (group) { - try { - if (IS_WINDOWS) { - const taskkill = Bun.spawn({ - cmd: ["taskkill", "/F", "/T", "/PID", pid.toString()], - stdin: "ignore", - stdout: "ignore", - stderr: "ignore", - timeout: 5000, - windowsHide: true, - }); - void taskkill.exited.catch(() => {}); - taskkill.unref(); - } else { + if (nativeKillTree) { + nativeKillTree(pid, 9); + } else { + if (group && !IS_WINDOWS) { + try { process.kill(-pid, "SIGKILL"); + } catch {} + } + try { + if (typeof target === "number") { + process.kill(target, "SIGKILL"); + } else { + target.kill("SIGKILL"); } } catch {} } - try { - if (typeof target === "number") { - process.kill(target, "SIGKILL"); - } else { - target.kill("SIGKILL"); - } - } catch {} return await Promise.race([Bun.sleep(timeout).then(() => false), exitPromise]); } finally { diff --git a/packages/utils/src/ptree.ts b/packages/utils/src/ptree.ts index defb2904c..ee9664a51 100644 --- a/packages/utils/src/ptree.ts +++ b/packages/utils/src/ptree.ts @@ -10,11 +10,8 @@ */ import type { Spawn, Subprocess } from "bun"; -import { postmortem } from "."; import { terminate } from "./procmgr"; -const managedChildren = new Set(); - /** A Bun subprocess with stdout/stderr always piped (stdin may vary). */ type PipedSubprocess = Subprocess; @@ -98,15 +95,9 @@ async function pump( * - Unix: negative PID signals the process group */ async function killChild(child: ChildProcess) { - await terminate({ target: child.proc, group: child.isProcessGroup }); + await terminate({ target: child.proc }); } -postmortem.register("managed-children", async () => { - const children = Array.from(managedChildren); - managedChildren.clear(); - await Promise.all(children.map(killChild)); -}); - /** * Options for waiting for process exit and capturing output. */ @@ -205,10 +196,7 @@ export class ChildProcess { #stderrDone: Promise; #exited: Promise; - constructor( - public readonly proc: PipedSubprocess, - public readonly isProcessGroup: boolean, - ) { + constructor(public readonly proc: PipedSubprocess) { const { promise: stderrDone, resolve: resolveStderrDone } = Promise.withResolvers(); this.#stderrDone = stderrDone; @@ -245,8 +233,6 @@ export class ChildProcess { const { promise, resolve, reject } = Promise.withResolvers(); this.#exited = promise; - if (this.proc.exitCode === null) managedChildren.add(this); - // Normalize Bun's exited promise into our "exitReason / exitedCleanly" model. proc.exited .catch(() => null) @@ -279,9 +265,6 @@ export class ChildProcess { this.#exitReason = ex; reject(ex); - }) - .finally(() => { - managedChildren.delete(this); }); } @@ -426,7 +409,7 @@ export class ChildProcess { */ type ChildSpawnOptions = Omit< Spawn.SpawnOptions, - "stdout" | "stderr" + "stdout" | "stderr" | "detached" > & { /** AbortSignal to cancel the process */ signal?: AbortSignal; @@ -441,16 +424,15 @@ type ChildSpawnOptions = Omit< * @returns A ChildProcess instance. */ export function spawn(cmd: string[], options?: ChildSpawnOptions): ChildProcess { - const { detached = false, timeout = -1, signal, ...rest } = options ?? {}; + const { timeout = -1, signal, ...rest } = options ?? {}; const child = Bun.spawn(cmd, { stdin: "ignore", stdout: "pipe", stderr: "pipe", - detached, windowsHide: true, ...rest, }); - const cproc = new ChildProcess(child, detached); + const cproc = new ChildProcess(child); if (signal) cproc.attachSignal(signal); if (timeout > 0) cproc.attachTimeout(timeout); return cproc;