diff --git a/docs/orchestration-lifecycle.md b/docs/orchestration-lifecycle.md index adb6ba63b..c63856db0 100644 --- a/docs/orchestration-lifecycle.md +++ b/docs/orchestration-lifecycle.md @@ -274,6 +274,55 @@ this run mode; ran in the foreground.`. `STEP_DISABLE_SUBAGENT_BACKGROUND=1` or `[subagent] background = false` does the same everywhere, with a matching notice. +## Background commands and process ownership + +`run_command` with `run_in_background: true` starts a detached process tree for +long-lived work such as a dev server. The tree is registered in the process +registry under the calling session, with `kind: "bash"`. + +Kind decides whether a tree counts as unfinished work. `subagent` and +`workflow` are the session's own work, so they count. `bash` is a side effect +the user inspects afterwards: a session with a running dev server is still a +finished session, and its row does not read as busy. + +Ownership decides when the tree stops: + +| Event | Background command | +| --- | --- | +| Switching which session is viewed | keeps running | +| Forking a session | keeps running; the new session starts with none of them | +| Reloading settings or extensions | keeps running | +| The owning session is deleted, replaced, or resumed away | stopped | +| The host exits | stopped | + +Stopping goes through three layers, from narrowest to widest: + +1. `releaseOwnerGracefully(ownerId)` sends `SIGTERM` to the tree's process + group, then `SIGKILL` after `PROCESS_TREE_TERM_GRACE_MS` (3s). The owning + session calls it from its `session_shutdown` handler. A command the user + interrupted uses the shorter `INTERRUPTED_COMMAND_GRACE_MS` (500ms) instead: + an Escape or an elapsed deadline must look stopped immediately, and the call + still waits for the tree to exit. +2. `releaseAllProcessesSync()` runs from a `process.exit` listener registered + inside the registry module. Every host inherits it, including hosts that do + not write their own shutdown path. +3. A detached parent-death supervisor holds one IPC lease per armed group. + When the channel closes — including when the host is `SIGKILL`ed — the + supervisor signals the group. No in-process handler can cover that case. + Every command arms it, foreground or background: both are spawned detached, + so both would otherwise outlive a killed host. The supervisor is one process + per host, so a foreground command costs a lease, not a process. + +The registry also decides what the model and the UI can see. `bash_list` returns +the calling session's own commands with pid, status, and log path; `bash_kill` +stops one or all of them and refuses pids owned by another session. A fork +therefore starts with an empty list even though the parent's commands keep +running. + +Log files survive their command: they are the only record of what a dev server +printed before it was stopped, so the registry keeps the `logPath` and does not +delete it. + ## Cron scheduling and delivery `cron_create` accepts numeric five-field local-time cron expressions. Fields diff --git a/packages/coding-agent/src/core/process-registry.ts b/packages/coding-agent/src/core/process-registry.ts new file mode 100644 index 000000000..13896b633 --- /dev/null +++ b/packages/coding-agent/src/core/process-registry.ts @@ -0,0 +1,201 @@ +/** + * Process ownership registry for detached child process trees. + * + * A coding session starts process trees it cannot wait on: subagent RPC children, background + * `run_in_background` commands, and the shells that `run_command` creates. Each of them must be + * reclaimed at a different moment, and the moment depends on *why* the process exists: + * + * - `subagent` and `workflow` are the session's own work. They count as unfinished work, and they + * die with the session that started them. + * - `bash` is a side effect the user inspects (a dev server, a watcher). It must survive a session + * switch or a fork, so it is *not* unfinished work, but it still dies when its session is + * deleted or the host exits. + * + * Three reclamation layers exist, from narrowest to widest: + * + * 1. `releaseOwner(ownerId)` — one session's trees. Used when a session is torn down. + * 2. `releaseAll()` — every tree this process owns. Registered on `process.exit`, so a host that + * never wrote its own shutdown path still reclaims children. + * 3. The parent-death guard in `./parent-death-guard.ts` — an external supervisor for `SIGKILL`, + * which no in-process handler can survive. + * + * Watch out: `trackDetachedChildPid` used to be a bare `Set` in `utils/shell.ts`. Anything + * that still uses it keeps working through the compatibility wrappers exported there, but new code + * must register here so ownership and kind are known. + */ + +import { killProcessTree, PROCESS_TREE_TERM_GRACE_MS, terminateProcessTreeGracefully } from "../utils/process-tree.ts"; + +/** Why a process tree exists. Determines whether it counts as unfinished session work. */ +export type ProcessKind = "bash" | "subagent" | "workflow"; + +/** One registered process tree. */ +export interface ProcessRegistration { + /** Process-group leader pid on POSIX; the wrapper's pid on Windows. */ + pid: number; + kind: ProcessKind; + /** Session that owns the tree; `HOST_OWNER` for trees outside any session. */ + ownerId: string; + /** Human-readable label, bounded by the caller. */ + label: string; + /** Working directory the command ran in, when known. */ + cwd?: string; + /** Log file capturing output, for background commands. */ + logPath?: string; + startedAt: number; + /** True once a reclamation has been requested for this tree. */ + releasing?: boolean; +} + +/** Owner id for trees a host starts outside a session. */ +export const HOST_OWNER = "\0host"; + +/** Grace window between the tree's SIGTERM and its SIGKILL. */ +export const PROCESS_TERM_GRACE_MS = PROCESS_TREE_TERM_GRACE_MS; + +/** Kinds that count as unfinished session work. `bash` deliberately does not. */ +const UNFINISHED_WORK_KINDS: ReadonlySet = new Set(["subagent", "workflow"]); + +/** Whether a kind represents work the session is still performing. */ +export function isUnfinishedWork(kind: ProcessKind): boolean { + return UNFINISHED_WORK_KINDS.has(kind); +} + +const registrations = new Map(); +const listeners = new Set<(entries: readonly ProcessRegistration[]) => void>(); + +function notify(): void { + if (listeners.size === 0) return; + const snapshot = [...registrations.values()]; + for (const listener of listeners) { + try { + listener(snapshot); + } catch { + // An observer must never break process reclamation. + } + } +} + +/** Subscribe to registry changes; returns the unsubscribe function. */ +export function subscribeToProcessRegistry(listener: (entries: readonly ProcessRegistration[]) => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +/** Every registered tree, newest last. */ +export function listProcesses(ownerId?: string): ProcessRegistration[] { + const all = [...registrations.values()]; + return ownerId === undefined ? all : all.filter((entry) => entry.ownerId === ownerId); +} + +/** Trees for one owner that count as unfinished work. */ +export function listUnfinishedWork(ownerId: string): ProcessRegistration[] { + return listProcesses(ownerId).filter((entry) => isUnfinishedWork(entry.kind) && !entry.releasing); +} + +/** Register one process tree. Re-registering the same pid replaces the previous record. */ +export function registerProcess(input: Omit & { startedAt?: number }): void { + if (!Number.isSafeInteger(input.pid) || input.pid <= 0) return; + registrations.set(input.pid, { ...input, startedAt: input.startedAt ?? Date.now() }); + notify(); +} + +/** Drop one tree's record without signalling it (the caller already reaped it). */ +export function forgetProcess(pid: number): void { + if (registrations.delete(pid)) notify(); +} + +/** + * Drop a tree's record without signalling it, but only when the record still describes what the + * caller registered. A pid can be reused after exit, so a late `exit` event from an old child must + * not remove a newer registration. + */ +export function forgetProcessIf(pid: number, matches: (entry: ProcessRegistration) => boolean): void { + const entry = registrations.get(pid); + if (entry && matches(entry) && registrations.delete(pid)) notify(); +} + +/** + * Kill one owner's trees immediately and forget them. + * + * Synchronous and unbounded: it exists for `process.exit`, where there is no event loop turn left to + * await an escalation. Callers that can wait must use {@link releaseOwnerGracefully} instead, so a + * shell's `trap ... TERM` handler still runs. + */ +export function releaseOwnerProcessesSync(ownerId: string): number { + let released = 0; + for (const [pid, entry] of [...registrations]) { + if (entry.ownerId !== ownerId) continue; + killProcessTree(pid); + registrations.delete(pid); + released += 1; + } + if (released > 0) notify(); + return released; +} + +/** Kill every registered tree immediately and forget them. For host exit. */ +export function releaseAllProcessesSync(): number { + let released = 0; + for (const [pid] of [...registrations]) { + killProcessTree(pid); + registrations.delete(pid); + released += 1; + } + if (released > 0) notify(); + return released; +} + +/** + * Terminate one owner's trees and wait for the grace window. + * + * **Kind is the caller's decision.** This function releases every registration the owner has, + * whatever its kind, unless `only` narrows it. That is deliberate for session teardown, where all + * of the session's work should stop. It is wrong for a narrower caller: `bash_kill` promises to + * stop background commands, so it must pass the pids it means and leave the owner's subagent trees + * alone. Any new caller that means "one kind" has to say so explicitly: + * + * ```ts + * const pids = listProcesses(ownerId).filter((e) => e.kind === "bash").map((e) => e.pid); + * await releaseOwnerGracefully(ownerId, undefined, new Set(pids)); + * ``` + * + * `only` narrows pids, this function does not widen them back. + * + * Marks each tree released first, so a concurrent {@link releaseAllProcessesSync} on `process.exit` + * does not signal the same group twice. Returns after the escalation is *sent*, not after every pid + * is observed dead: a tree that ignores `SIGKILL` is not something this process can fix. + */ +export async function releaseOwnerGracefully( + ownerId: string, + graceMs: number = PROCESS_TERM_GRACE_MS, + only?: ReadonlySet, +): Promise { + const pending: Array> = []; + for (const entry of registrations.values()) { + if (entry.ownerId !== ownerId || entry.releasing) continue; + if (only !== undefined && !only.has(entry.pid)) continue; + entry.releasing = true; + // `keepAlive`: the caller awaits `settled`, so the grace timer must keep the loop alive long + // enough to fire the escalation. An unref'd timer would let Node exit mid-wait. + pending.push(terminateProcessTreeGracefully(entry.pid, graceMs, { keepAlive: true }).settled); + } + if (pending.length === 0) return 0; + notify(); + await Promise.all(pending); + for (const [pid, entry] of [...registrations]) { + if (entry.ownerId !== ownerId || !entry.releasing) continue; + if (only !== undefined && !only.has(pid)) continue; + registrations.delete(pid); + } + notify(); + return pending.length; +} + +// A host that never wrote its own shutdown path still reclaims its children. `prependListener` +// runs this before any other exit handler, including ones that throw. +process.prependListener("exit", () => { + releaseAllProcessesSync(); +}); diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index dbea9327f..138d324f0 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -7,12 +7,14 @@ import { keyHint } from "../../render/keybinding-hints.ts"; import { truncateToVisualLines } from "../../render/visual-truncate.ts"; import { theme } from "../../theme/theme.ts"; import { waitForChildProcess } from "../../utils/child-process.ts"; +import { armParentDeathGuard } from "../../utils/parent-death-guard.ts"; import { getShellConfig, getShellEnv, - killProcessTree, + INTERRUPTED_COMMAND_GRACE_MS, type ShellConfig, spawnShellChild, + terminateProcessTreeGracefully, trackDetachedChildPid, untrackDetachedChildPid, } from "../../utils/shell.ts"; @@ -137,10 +139,31 @@ export function createLocalShellOperations( stderr: "pipe", }); if (child.pid) trackDetachedChildPid(child.pid); + // The child is spawned detached (its own process group), so it survives a host that dies + // without running its handlers. Arm the same guard a background command uses: one shared + // supervisor per host, so a foreground command costs a lease, not a process. Without this + // a `SIGKILL`ed host leaves the command — and everything it started — running. + const guard = child.pid ? armParentDeathGuard(child.pid, INTERRUPTED_COMMAND_GRACE_MS) : undefined; + void guard?.ready.catch(() => {}); let timedOut = false; let timeoutHandle: NodeJS.Timeout | undefined; + // Stop and timeout send SIGTERM to the process group and escalate to SIGKILL after a + // grace window, so a shell `trap ... TERM` handler gets to run its own cleanup. The + // escalation is cancelled once the child actually exits; a late SIGKILL must never land + // on a reused pid. Re-arming (timeout then abort) replaces the previous escalation + // instead of leaking it. + // + // A foreground command uses a short window: the user pressed Escape, or the deadline + // already elapsed, so anything longer reads as a hang. The call still waits for the tree + // to exit, which is where a cleanup handler's time actually goes. + let cancelEscalation: (() => void) | undefined; + const stopGracefully = () => { + if (!child.pid) return; + cancelEscalation?.(); + cancelEscalation = terminateProcessTreeGracefully(child.pid, INTERRUPTED_COMMAND_GRACE_MS); + }; const onAbort = () => { - if (child.pid) killProcessTree(child.pid); + stopGracefully(); }; try { @@ -148,13 +171,13 @@ export function createLocalShellOperations( if (timeoutMs !== undefined) { timeoutHandle = setTimeout(() => { timedOut = true; - if (child.pid) killProcessTree(child.pid); + stopGracefully(); }, timeoutMs); } // Stream stdout and stderr. child.stdout?.on("data", onData); child.stderr?.on("data", onData); - // Handle abort signal by killing the entire process tree. + // Handle abort signal by stopping the entire process tree. if (signal) { if (signal.aborted) onAbort(); else signal.addEventListener("abort", onAbort, { once: true }); @@ -170,6 +193,11 @@ export function createLocalShellOperations( ...(timeout === undefined ? {} : { timeoutSeconds: timeout }), }; } finally { + // Disarm a pending escalation first: the process is gone, and its pid may be reused. + cancelEscalation?.(); + // Release the guard lease too. A command that exited on its own must not be reported + // as a parent death, and the lease must not outlive the pid it names. + void guard?.disarm(); if (child.pid) untrackDetachedChildPid(child.pid); if (timeoutHandle) clearTimeout(timeoutHandle); if (signal) signal.removeEventListener("abort", onAbort); diff --git a/packages/coding-agent/src/features/step.ts b/packages/coding-agent/src/features/step.ts index f73c1d03f..c947bb0d6 100644 --- a/packages/coding-agent/src/features/step.ts +++ b/packages/coding-agent/src/features/step.ts @@ -8,6 +8,7 @@ import type { ExtensionUIContext, InlineExtension, } from "../core/extensions/types.ts"; +import { releaseOwnerGracefully } from "../core/process-registry.ts"; import { BUILTIN_SLASH_COMMANDS } from "../core/slash-commands.ts"; import type { FeedbackIdentity } from "../step/feedback/types.ts"; import { STEP_INIT_PROMPT } from "../step/init-prompt.ts"; @@ -346,6 +347,11 @@ export function createStepExtension(options: StepExtensionOptions = {}): Extensi // A "this session" Full Access grant must not carry into the next // session; only the persisted acknowledgment does. fullAccessAcknowledged = false; + // Background commands belong to the session that started them. A session switch or a + // fork never reaches this handler (it keeps the same session object and the old one + // stays open), so reaching here means the session is really going away: release exactly + // its own trees, and leave every other session's dev servers running. + releaseSessionProcesses(ctx); if (nativeRetryOverridden && nativeRetryPreference !== undefined && ctx.setAutoRetryEnabled) { try { ctx.setAutoRetryEnabled(nativeRetryPreference); @@ -522,6 +528,32 @@ export function createStepExtension(options: StepExtensionOptions = {}): Extensi * persisted preset as `initialPreset` would otherwise outrank env aliases in * Pi's resolver, so only inject it when no policy override is present. */ +/** + * Release the background process trees one session owns. + * + * Called from `session_shutdown`, which fires only when the session is really going away: `/new`, + * resume, a deleted session, and quit. A session switch or a fork reuses the same session object + * and never reaches this handler, so a dev server the user is watching keeps running while they + * look at another conversation. That is the intended difference between "not visible" and "closed". + * + * Failure is reported, not thrown: a session that cannot stop one tree must still finish shutting + * down, and the host-exit sweep is the backstop. + */ +function releaseSessionProcesses(ctx: ExtensionContext): void { + let ownerId: string; + try { + ownerId = ctx.sessionManager.getSessionId(); + } catch { + // A context from an already-replaced session cannot name its owner; the host sweep covers it. + return; + } + if (!ownerId) return; + void releaseOwnerGracefully(ownerId).catch(() => { + // The registry already marked the trees releasing; a failed wait must not surface as an + // unhandled rejection during teardown. + }); +} + function resolvePermissionOptions(options: StepExtensionOptions): StepPermissionControllerOptions | undefined { const explicit: StepPermissionControllerOptions = { ...options.permission, diff --git a/packages/coding-agent/src/features/subagent/rpc-adapter.ts b/packages/coding-agent/src/features/subagent/rpc-adapter.ts index 7bbdff943..f3caa17a6 100644 --- a/packages/coding-agent/src/features/subagent/rpc-adapter.ts +++ b/packages/coding-agent/src/features/subagent/rpc-adapter.ts @@ -14,8 +14,8 @@ import { rmSync } from "node:fs"; import { mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { forgetProcess, HOST_OWNER, registerProcess } from "../../core/process-registry.ts"; import { waitForChildProcess } from "../../utils/child-process.ts"; -import { trackDetachedChildPid, untrackDetachedChildPid } from "../../utils/shell.ts"; import { CHILD_MARKER, cloneUsage, @@ -261,10 +261,19 @@ export async function createSubagentRpcSession( shell: false, stdio: ["pipe", "pipe", "pipe"], }); - // Registered with the host's process-exit sweep, so a signal or emergency - // exit that skips session shutdown still kills the child. + // Registered as `kind: "subagent"` under the parent session, so the parent's teardown reclaims + // it and so it counts as unfinished work: a running subagent means the session is still doing + // something, unlike a background dev server the user may be inspecting. const childPid = child.pid; - if (childPid) trackDetachedChildPid(childPid); + if (childPid) { + registerProcess({ + pid: childPid, + kind: "subagent", + ownerId: input.parentSessionId ?? HOST_OWNER, + label: `subagent ${sessionId}`, + cwd: input.cwd, + }); + } // `close` can be held open by inherited descendant pipes after OS exit. // Reuse the bounded-idle stdio drain used by our other child processes. const exitStatus = waitForChildProcess(child); @@ -491,7 +500,7 @@ export async function createSubagentRpcSession( child.once("error", (error) => { processError = error.message; childExited = true; - if (childPid) untrackDetachedChildPid(childPid); + if (childPid) forgetProcess(childPid); dropFromRegistry(); clearIdleWatchdog(); clearWallClock(); @@ -505,7 +514,7 @@ export async function createSubagentRpcSession( }); const finishExitedChild = (code: number | null): void => { childExited = true; - if (childPid) untrackDetachedChildPid(childPid); + if (childPid) forgetProcess(childPid); dropFromRegistry(); clearIdleWatchdog(); clearWallClock(); diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 51134e1f3..a58ac7421 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -287,6 +287,21 @@ export type { ResolvedResource, } from "./core/package-manager.ts"; export { DefaultPackageManager } from "./core/package-manager.ts"; +export { + forgetProcess, + HOST_OWNER, + isUnfinishedWork, + listProcesses, + listUnfinishedWork, + PROCESS_TERM_GRACE_MS, + type ProcessKind, + type ProcessRegistration, + registerProcess, + releaseAllProcessesSync, + releaseOwnerGracefully, + releaseOwnerProcessesSync, + subscribeToProcessRegistry, +} from "./core/process-registry.ts"; export type { AppMode } from "./core/project-trust.ts"; export type { ResourceCollision, @@ -898,6 +913,7 @@ export { } from "./utils/image-resize.ts"; export { detectSupportedImageMimeTypeFromFile } from "./utils/mime.ts"; export { openBrowser } from "./utils/open-browser.ts"; +export { armParentDeathGuard, type ParentDeathGuard } from "./utils/parent-death-guard.ts"; export { canonicalizePath, getCwdRelativePath, isLocalPath, resolvePath } from "./utils/paths.ts"; export { getPiUserAgent } from "./utils/pi-user-agent.ts"; // Shell utilities diff --git a/packages/coding-agent/src/step/tool-profile.ts b/packages/coding-agent/src/step/tool-profile.ts index 22d8f5eca..313f7a2a8 100644 --- a/packages/coding-agent/src/step/tool-profile.ts +++ b/packages/coding-agent/src/step/tool-profile.ts @@ -23,6 +23,13 @@ import type { ToolRenderContext, ToolRenderResultOptions, } from "../core/extensions/types.ts"; +import { + forgetProcessIf, + HOST_OWNER, + listProcesses, + registerProcess, + releaseOwnerGracefully, +} from "../core/process-registry.ts"; import { type BashExecutionOutcome, type BashToolDetails, ShellToolError } from "../core/tools/bash.ts"; import { commandOutcomeNotice, @@ -54,15 +61,9 @@ import { pathExists, resolveReadPathAsync, resolveToCwd } from "../core/tools/pa import type { ReadToolOptions } from "../core/tools/read.ts"; import type { WriteToolOptions } from "../core/tools/write.ts"; import { detectSupportedImageMimeTypeFromFile } from "../utils/mime.ts"; +import { armParentDeathGuard, type ParentDeathGuard } from "../utils/parent-death-guard.ts"; import { resolvePath } from "../utils/paths.ts"; -import { - getShellConfig, - getShellEnv, - killProcessTree, - spawnShellChild, - trackDetachedChildPid, - untrackDetachedChildPid, -} from "../utils/shell.ts"; +import { getShellConfig, getShellEnv, killProcessTree, spawnShellChild } from "../utils/shell.ts"; import { resolveStepAgentDir } from "./environment.ts"; import { createSearchWebTool, type SearchWebToolOptions } from "./search-web-tool.ts"; import { readThenRunCommand } from "./then-run.ts"; @@ -76,6 +77,8 @@ const STEP_TOOL_NAMES = [ "write_file", "edit_file", "run_command", + "bash_list", + "bash_kill", "find_tools", ] as const; @@ -157,7 +160,7 @@ const runCommandSchema = Type.Object({ run_in_background: Type.Optional( Type.Boolean({ description: - "Start the command detached and return immediately with its pid and a log file capturing stdout+stderr. Use for long-running processes such as dev servers: read the log to confirm readiness, then stop it (and the processes it spawned) with the kill command returned in the result. The process is terminated when the session exits; timeout_ms and max_output_chars do not apply.", + "Start the command detached and return immediately with its pid and a log file capturing stdout+stderr. Use for long-running processes such as dev servers: read the log to confirm readiness, then stop it (and the processes it spawned) with bash_kill or the kill command returned in the result. It keeps running while the user views another conversation and stops when this session is deleted or the app exits; timeout_ms and max_output_chars do not apply.", }), ), }); @@ -240,6 +243,23 @@ function getToolCwd(ctx: ExtensionContext | undefined, fallback: string): string return getContextValue(ctx, "cwd") ?? fallback; } +/** + * The session a tool call belongs to. + * + * Background process trees are owned per session, so a session that is deleted reclaims exactly its + * own trees and no one else's. A host that runs a tool with no session (an SDK embedder, a test) + * still gets an owner, so its trees are reclaimed on host exit. + */ +function getOwnerId(ctx: ExtensionContext | undefined): string { + if (!ctx) return HOST_OWNER; + try { + return ctx.sessionManager.getSessionId() || HOST_OWNER; + } catch { + // A context from a replaced session throws on read; its trees belong to the host. + return HOST_OWNER; + } +} + function getContextOutputLimit(ctx: ExtensionContext | undefined): number | undefined { const limit = getContextValue(ctx, "commandOutputLimit"); return typeof limit === "number" && Number.isFinite(limit) ? limit : undefined; @@ -1510,14 +1530,17 @@ function createFindToolsDefinition( } /** - * Detached run for long-lived processes (dev servers, watchers). The child - * outlives the tool call but not the session: its pid stays in the detached - * registry until it exits, and session shutdown kills whatever is left. + * Detached run for long-lived processes (dev servers, watchers). The child outlives the tool call, + * but not its session: it is registered as `kind: "bash"` under the owning session, so the session's + * teardown reclaims it while a session switch or fork leaves it running. + * + * `kind: "bash"` is deliberate. A background command is a side effect the user inspects, not work + * the session is still performing, so it must not make an otherwise finished session read as busy. */ async function startBackgroundCommand( command: string, commandCwd: string, - options: { shellPath?: string; agentDir?: string }, + options: { shellPath?: string; agentDir?: string; ownerId: string }, ): Promise { try { await fsStat(commandCwd); @@ -1564,9 +1587,24 @@ async function startBackgroundCommand( child.once("error", (error) => reject(error)); }); const pid = child.pid; + let guard: ParentDeathGuard | undefined; if (pid !== undefined) { - trackDetachedChildPid(pid); - child.once("exit", () => untrackDetachedChildPid(pid)); + registerProcess({ + pid, + kind: "bash", + ownerId: options.ownerId, + label: commandLabel(command), + cwd: commandCwd, + logPath, + }); + child.once("exit", () => { + void guard?.disarm(); + forgetProcessIf(pid, (entry) => entry.kind === "bash" && entry.ownerId === options.ownerId); + }); + // The guard is what reclaims this tree if the host dies without running its own shutdown, + // including SIGKILL. It is best-effort: an unavailable supervisor leaves the other layers. + guard = armParentDeathGuard(pid); + void guard?.ready.catch(() => {}); } child.unref(); // Stop the whole process tree, not just the wrapper shell. The child is spawned @@ -1596,10 +1634,16 @@ async function startBackgroundCommand( ].join("\n"), }, ], - details: { background: true, pid: pid ?? null, logPath }, + details: { background: true, kind: "bash", pid: pid ?? null, logPath, command: commandLabel(command) }, }; } +/** First line of a command, bounded: the label identifies a row, it is not the command itself. */ +function commandLabel(command: string): string { + const firstLine = command.trim().split("\n")[0]?.trim() ?? ""; + return firstLine.length > 120 ? `${firstLine.slice(0, 117)}...` : firstLine; +} + /** Create Step-facing definitions backed by Pi's native tools. */ export function createStepToolProfile(cwd: string, options: StepToolProfileOptions = {}): AnyToolDefinition[] { // A Step profile is also used by embedded hosts that do not launch @@ -1793,6 +1837,9 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio return startBackgroundCommand(args.command, commandCwd, { ...(nativeBashOptions.shellPath ? { shellPath: nativeBashOptions.shellPath } : {}), agentDir, + // A background command belongs to the session that asked for it, but it is not + // that session's work: a switch or fork must leave it running. + ownerId: getOwnerId(ctx), }); } const nativeArgs = mapRunCommandArgs(args, ctx); @@ -1836,11 +1883,146 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio writeFile, editFile, runCommand, + createBashListDefinition(getOwnerId), + createBashKillDefinition(getOwnerId), ]; const findTools = createFindToolsDefinition(profileWithoutFindTools); return [...profileWithoutFindTools, findTools]; } +/** Details for `bash_list`: the session's background commands, in registry order. */ +interface BashListDetails { + background: true; + kind: "bash"; + commands: Array<{ + pid: number; + label: string; + status: "running" | "stopping"; + cwd?: string; + logPath?: string; + }>; +} + +/** Details for `bash_kill`: how many trees the release covered. */ +interface BashKillDetails { + background: true; + kind: "bash"; + stopped: number; + pid?: number; +} + +const bashListSchema = Type.Object({}); +const bashKillSchema = Type.Object({ + pid: Type.Optional(Type.Integer({ minimum: 1, description: "Pid from bash_list." })), + all: Type.Optional(Type.Boolean({ description: "Stop every background command in this session." })), +}); + +type BashKillInput = Static; + +/** + * `bash_list`: the background commands this session started. + * + * The model otherwise has to remember pids from earlier tool results, and a session that was + * resumed or compacted has lost them. Listing from the registry makes the state authoritative + * rather than derived from transcript text. + */ +function createBashListDefinition( + ownerOf: (ctx: ExtensionContext | undefined) => string, +): ToolDefinition { + return { + name: "bash_list", + label: "bash_list", + description: + "List the background commands this session started, with pid, status, and log path. Background commands keep running across a session switch and stop when the session is deleted or the app exits.", + promptSnippet: "List this session's background commands", + parameters: bashListSchema, + execute: async (_toolCallId, _args, _signal, _onUpdate, ctx) => { + const ownerId = ownerOf(ctx); + const entries = listProcesses(ownerId).filter((entry) => entry.kind === "bash"); + if (entries.length === 0) { + return { content: [{ type: "text" as const, text: "(no background commands)" }], details: undefined }; + } + const text = entries + .map((entry) => { + const parts = [ + `pid ${entry.pid}`, + entry.releasing ? "stopping" : "running", + entry.label, + entry.logPath ? `log: ${entry.logPath}` : undefined, + ].filter((part): part is string => part !== undefined); + return parts.join(" · "); + }) + .join("\n"); + return { + content: [{ type: "text" as const, text }], + details: { + background: true, + kind: "bash" as const, + commands: entries.map((entry) => ({ + pid: entry.pid, + label: entry.label, + status: entry.releasing ? ("stopping" as const) : ("running" as const), + ...(entry.cwd ? { cwd: entry.cwd } : {}), + ...(entry.logPath ? { logPath: entry.logPath } : {}), + })), + }, + }; + }, + }; +} + +/** + * `bash_kill`: stop background commands this session started. + * + * Scoped to the caller's own session on purpose. A subagent or a fork must not be able to kill a + * dev server another session is watching. + */ +function createBashKillDefinition( + ownerOf: (ctx: ExtensionContext | undefined) => string, +): ToolDefinition { + return { + name: "bash_kill", + label: "bash_kill", + description: + "Stop background commands this session started. Accepts one pid or `all`. Sends SIGTERM, then SIGKILL after a short grace period. Only the caller's own session's commands can be stopped.", + promptSnippet: "Stop a background command started by this session", + parameters: bashKillSchema, + execute: async (_toolCallId, args: BashKillInput, _signal, _onUpdate, ctx) => { + const ownerId = ownerOf(ctx); + const owned = listProcesses(ownerId).filter((entry) => entry.kind === "bash"); + const killAll = args.all === true; + if (!killAll && args.pid === undefined) { + throw new Error("Pass `pid` or `all: true`."); + } + if (args.pid !== undefined && !owned.some((entry) => entry.pid === args.pid)) { + throw new Error(`No background command with pid ${args.pid} in this session.`); + } + if (killAll) { + // Narrow to this session's background commands. The registry's owner release is + // kind-agnostic by design, so passing the whole owner would also stop this session's + // subagent and workflow trees — which the description does not promise and which the + // user did not ask for. + const count = await releaseOwnerGracefully(ownerId, undefined, new Set(owned.map((entry) => entry.pid))); + return { + content: [ + { + type: "text" as const, + text: count === 0 ? "(no background commands)" : `Stopped ${count} background command(s).`, + }, + ], + details: { background: true, kind: "bash" as const, stopped: count }, + }; + } + const target = args.pid as number; + await releaseOwnerGracefully(ownerId, undefined, new Set([target])); + return { + content: [{ type: "text" as const, text: `Stopped background command ${target}.` }], + details: { background: true, kind: "bash" as const, stopped: 1, pid: target }, + }; + }, + }; +} + /** True for the Step model-facing names. */ export function isStepToolName(name: string): name is StepToolName { return (STEP_TOOL_NAMES as readonly string[]).includes(name); diff --git a/packages/coding-agent/src/utils/parent-death-guard.ts b/packages/coding-agent/src/utils/parent-death-guard.ts new file mode 100644 index 000000000..9e4769747 --- /dev/null +++ b/packages/coding-agent/src/utils/parent-death-guard.ts @@ -0,0 +1,216 @@ +/** + * Parent-death guard for detached process trees. + * + * `SIGKILL` cannot be caught, so no in-process handler can reclaim a shell that outlived its host. + * An external supervisor can: this module spawns one detached Node process per host, and that + * supervisor's IPC channel doubles as a liveness lease. + * + * The mechanism relies on one Node guarantee: an IPC channel's `disconnect` event fires when the + * other end disappears for *any* reason, including `SIGKILL` of the parent. The supervisor arms a + * lease per process group; when the channel closes it signals every armed group, and disarms when + * the owner reports a normal exit. + * + * One supervisor serves the whole host: leases are keyed by a random id, so a stale `disarm` from a + * previous generation cannot cancel a live lease, and a pid that was recycled after its owner + * exited is never signalled. + * + * Why the supervisor exists at all, rather than just `process.on("exit")`: `exit` handlers do not + * run on `SIGKILL`, and a crashed Electron main process is exactly the case that left a dev server + * running for days. + */ + +import { type ChildProcess, spawn } from "node:child_process"; +import { randomUUID } from "node:crypto"; + +/** A live lease. `disarm` must be called once the guarded tree exited on its own. */ +export interface ParentDeathGuard { + /** Resolves when the supervisor has armed the lease and is watching. */ + ready: Promise; + /** Release the lease so a normal exit is not reported as a parent death. */ + disarm: () => Promise; +} + +/** + * Source of the detached supervisor. Kept as a string because it must run in a separate Node + * process with no module resolution: it is passed to `--eval`. + */ +const SUPERVISOR_SOURCE = ` +import { spawn } from "node:child_process"; + +const targets = new Map(); +let armed = true; +// Keeps the supervisor alive while it holds leases. Cleared before every exit. +const keepAlive = setInterval(() => {}, 1 << 30); + +const signalGroup = (targetPid, signal) => { + try { process.kill(-targetPid, signal); } + catch { + try { process.kill(targetPid, signal); } catch {} + } +}; + +const taskkill = (targetPid, force) => { + const root = process.env.SystemRoot || process.env.windir; + const executable = root ? win32.join(root, "System32", "taskkill.exe") : "taskkill.exe"; + try { + const child = spawn(executable, [...(force ? ["/F"] : []), "/T", "/PID", String(targetPid)], { + stdio: "ignore", + windowsHide: true, + }); + const fallbackKill = () => { + try { process.kill(targetPid, "SIGKILL"); } catch {} + }; + child.once("error", fallbackKill); + child.unref(); + } catch { + try { process.kill(targetPid, "SIGKILL"); } catch {} + } +}; + +const terminate = () => { + if (!armed) return; + armed = false; + if (targets.size === 0) { + clearInterval(keepAlive); + process.exit(0); + } + let maxGraceMs = 0; + for (const { pid: targetPid, graceMs } of targets.values()) { + maxGraceMs = Math.max(maxGraceMs, graceMs); + if (process.platform === "win32") taskkill(targetPid, false); + else signalGroup(targetPid, "SIGTERM"); + } + setTimeout(() => { + for (const { pid: targetPid } of targets.values()) { + if (process.platform === "win32") taskkill(targetPid, true); + else signalGroup(targetPid, "SIGKILL"); + } + clearInterval(keepAlive); + process.exit(0); + }, maxGraceMs); +}; + +// The channel closing means the host is gone, however it died. This is the whole point. +process.once("disconnect", terminate); +process.once("SIGTERM", terminate); +process.on("message", (message) => { + if (!message || typeof message !== "object") return; + const pid = Number(message.pid); + const leaseId = typeof message.leaseId === "string" ? message.leaseId : ""; + if (!leaseId || !Number.isFinite(pid) || pid <= 0) return; + if (message.type === "arm") { + targets.set(leaseId, { pid, graceMs: Math.max(0, Number(message.graceMs) || 0) }); + process.send?.({ type: "armed", leaseId, pid }); + return; + } + if (message.type !== "disarm") return; + targets.delete(leaseId); + process.send?.({ type: "disarmed", leaseId, pid }); +}); +`; + +let supervisor: ChildProcess | undefined; + +function isUsable(candidate: ChildProcess | undefined): candidate is ChildProcess { + return candidate !== undefined && candidate.connected === true; +} + +function spawnSupervisor(): ChildProcess | undefined { + let child: ChildProcess; + try { + child = spawn(process.execPath, ["--input-type=module", "--eval", SUPERVISOR_SOURCE], { + detached: true, + stdio: ["ignore", "ignore", "ignore", "ipc"], + windowsHide: true, + env: { + // The supervisor only needs enough environment to find `taskkill` on Windows and to + // run Node as Node when this host is Electron. Do not inherit credentials. + ...(process.env.SystemRoot ? { SystemRoot: process.env.SystemRoot } : {}), + ...(process.env.windir ? { windir: process.env.windir } : {}), + ...(process.versions.electron ? { ELECTRON_RUN_AS_NODE: "1" } : {}), + }, + }); + } catch { + return undefined; + } + const forget = () => { + if (supervisor === child) supervisor = undefined; + }; + child.once("error", forget); + child.once("exit", forget); + child.unref(); + child.channel?.unref?.(); + supervisor = child; + return child; +} + +function waitForMessage( + child: ChildProcess, + type: "armed" | "disarmed", + pid: number, + leaseId: string, + send: { type: "arm"; graceMs: number } | { type: "disarm" }, +): Promise { + return new Promise((resolve, reject) => { + let timer: NodeJS.Timeout; + const finish = (error?: Error) => { + clearTimeout(timer); + child.off("message", onMessage); + child.off("exit", onExit); + if (error) reject(error); + else resolve(); + }; + const onMessage = (received: unknown) => { + if (!received || typeof received !== "object") return; + const message = received as { type?: unknown; leaseId?: unknown; pid?: unknown }; + if (message.type !== type || message.leaseId !== leaseId || message.pid !== pid) return; + finish(); + }; + const onExit = () => finish(new Error("The parent-death supervisor exited before acknowledging")); + // A supervisor that never answers must not block command output: the command already runs + // unguarded in that case, which is the same state as before this module existed. The timer + // deliberately is *not* unref'd: a caller awaiting `ready` has nothing else pending, so an + // unref'd timer would let Node exit mid-wait and the promise would never settle. + timer = setTimeout(() => { + finish(new Error("The parent-death supervisor did not acknowledge in time")); + }, 2_000); + child.on("message", onMessage); + child.once("exit", onExit); + child.send( + { type: send.type, leaseId, pid, ...(send.type === "arm" ? { graceMs: send.graceMs } : {}) }, + (error) => { + if (error) finish(error); + }, + ); + }); +} + +/** + * Arm the parent-death guard for one process group. + * + * Returns `undefined` when the supervisor cannot be started or the pid is unusable; callers then + * fall back to whatever reclamation they already had. This never throws: a guard failing must not + * fail the command the user asked for. + */ +export function armParentDeathGuard(pid: number, graceMs = 3_000): ParentDeathGuard | undefined { + if (!Number.isSafeInteger(pid) || pid <= 0) return undefined; + const leaseId = randomUUID(); + const guardian = isUsable(supervisor) ? supervisor : spawnSupervisor(); + if (!guardian) return undefined; + const ready = waitForMessage(guardian, "armed", pid, leaseId, { type: "arm", graceMs }); + let disarmed = false; + return { + ready, + disarm: async () => { + if (disarmed) return; + disarmed = true; + // A lease that never armed has nothing to release; do not surface that as an error. + const armed = await ready.then( + () => true, + () => false, + ); + if (!armed || !guardian.connected) return; + await waitForMessage(guardian, "disarmed", pid, leaseId, { type: "disarm" }).catch(() => undefined); + }, + }; +} diff --git a/packages/coding-agent/src/utils/process-tree.ts b/packages/coding-agent/src/utils/process-tree.ts new file mode 100644 index 000000000..eccd68cd1 --- /dev/null +++ b/packages/coding-agent/src/utils/process-tree.ts @@ -0,0 +1,204 @@ +/** + * Cross-platform process-tree signalling. + * + * Split out of `utils/shell.ts` so the process registry can use it without importing the shell + * configuration layer, which would create a cycle (`shell` → `registry` → `shell`). + * + * Two termination strengths exist and callers must pick deliberately: + * + * - {@link terminateProcessTreeGracefully} sends `SIGTERM` to the group, waits, then escalates to + * `SIGKILL`. Use it for user-initiated stop and timeout, where a shell's `trap ... TERM` cleanup + * is part of the command's contract. + * - {@link killProcessTree} sends `SIGKILL` immediately. Use it only when waiting is not an option: + * the host is exiting, or a stream already broke. + */ + +import { type ChildProcess, spawn } from "node:child_process"; +import { readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** + * Long enough for a shell `trap ... TERM` handler to run its own cleanup. + * + * Used where waiting is acceptable: releasing a session's trees during teardown, where nothing + * interactive is blocked on it. + */ +export const PROCESS_TREE_TERM_GRACE_MS = 3_000; + +/** + * Grace for a command the user just interrupted or that hit its deadline. + * + * Shorter than {@link PROCESS_TREE_TERM_GRACE_MS} because a foreground command must look stopped + * immediately: the user pressed Escape, or the deadline already elapsed. A long window here reads as + * a hang, and the call is still held open until the tree actually exits. + */ +export const INTERRUPTED_COMMAND_GRACE_MS = 500; + +type LinuxProcessIdentity = { pid: number; ppid: number; startTime: string }; + +function readLinuxProcessIdentity(pid: number): LinuxProcessIdentity | undefined { + try { + const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); + const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" "); + const startTime = fields[19]; + return startTime ? { pid, ppid: Number(fields[1]), startTime } : undefined; + } catch { + return undefined; + } +} + +/** Snapshot descendants before killing their parent makes them untraceable. */ +function getLinuxDescendants(pid: number): LinuxProcessIdentity[] { + const children = new Map(); + try { + // /proc//task//children is absent on some Linux kernels. + // The stat parent field is available without that optional interface. + for (const entry of readdirSync("/proc")) { + const childPid = Number(entry); + if (!Number.isSafeInteger(childPid) || childPid <= 0) continue; + const child = readLinuxProcessIdentity(childPid); + if (!child) continue; + const siblings = children.get(child.ppid) ?? []; + siblings.push(child); + children.set(child.ppid, siblings); + } + } catch { + return []; + } + const descendants: LinuxProcessIdentity[] = []; + const pending = [pid]; + const visited = new Set(pending); + for (let index = 0; index < pending.length; index++) { + for (const child of children.get(pending[index]) ?? []) { + if (visited.has(child.pid)) continue; + visited.add(child.pid); + descendants.push(child); + pending.push(child.pid); + } + } + return descendants; +} + +/** + * Snapshot of the Linux descendants observed for one tree, taken before the first signal. + * + * Re-reading `/proc` at escalation time would find nothing: by then the parent is dead and its + * children have been reparented, so their `ppid` no longer points at the tree. The identity + * snapshot is what makes the second signal able to reach a descendant that ignored the first. + */ +type TreeSnapshot = LinuxProcessIdentity[]; + +function snapshotTree(pid: number): TreeSnapshot { + return process.platform === "linux" ? getLinuxDescendants(pid) : []; +} + +/** + * Signal one process group plus the descendants observed in `snapshot`. + * + * A command can put a child in its own session or process group (`setsid`, `nohup ... &`), which + * `kill(-pid)` does not reach — those are what the snapshot covers. The identity check keeps a + * recycled pid from being signalled. + * + * Never throws: a process that exited between the snapshot and the signal is not a failure. + */ +function signalTree(pid: number, signal: NodeJS.Signals, snapshot?: TreeSnapshot): void { + const descendants = snapshot ?? snapshotTree(pid); + try { + process.kill(-pid, signal); + } catch { + try { + process.kill(pid, signal); + } catch { + // The process already exited. + } + } + for (const descendant of [...descendants].reverse()) { + if (readLinuxProcessIdentity(descendant.pid)?.startTime !== descendant.startTime) continue; + try { + process.kill(descendant.pid, signal); + } catch { + // The descendant already exited or was killed with the parent group. + } + } +} + +function spawnWindowsTaskkill(pid: number, force: boolean): void { + try { + const child: ChildProcess = spawn( + join(process.env.SystemRoot ?? "C:\\Windows", "System32", "taskkill.exe"), + [...(force ? ["/F"] : []), "/T", "/PID", String(pid)], + { stdio: "ignore", detached: true, windowsHide: true }, + ); + // A failed spawn emits "error" asynchronously; consume it to avoid crashing Node. + child.once("error", () => {}); + } catch { + // Ignore errors if taskkill fails. + } +} + +/** + * Terminate a process tree, giving it a chance to clean up. + * + * `keepAlive` controls whether the grace timer keeps the process alive: + * + * - `false` (default) is for fire-and-forget stops. The timer is unref'd so a 3-second grace window + * cannot hold a quitting host open. + * - `true` is for callers that `await` {@link ProcessTreeTermination.settled}. An unref'd timer + * resolves nothing: with the await as the only pending work, Node drains the loop and exits + * instead of firing it. Such a caller must opt in. + * + * Returns a canceller that disarms the pending escalation. Call it once the process actually exited: + * a late `SIGKILL` must never land on a reused pid or process group. + */ +export function terminateProcessTreeGracefully( + pid: number, + graceMs: number = PROCESS_TREE_TERM_GRACE_MS, + options: { keepAlive?: boolean } = {}, +): (() => void) & { settled: Promise } { + let finish = (): void => {}; + const settled = new Promise((resolve) => { + let done = false; + let timer: NodeJS.Timeout | undefined; + finish = () => { + if (done) return; + done = true; + if (timer) clearTimeout(timer); + resolve(); + }; + if (process.platform === "win32") { + spawnWindowsTaskkill(pid, false); + timer = setTimeout(() => { + spawnWindowsTaskkill(pid, true); + finish(); + }, graceMs); + } else { + // One snapshot, used by both signals: after the SIGTERM the parent is usually gone, and a + // descendant that ignored it has already been reparented out of the tree. + const snapshot = snapshotTree(pid); + signalTree(pid, "SIGTERM", snapshot); + timer = setTimeout(() => { + signalTree(pid, "SIGKILL", snapshot); + finish(); + }, graceMs); + } + // `unref` only where nobody awaits: see the doc comment. + if (options.keepAlive !== true) timer.unref?.(); + }); + return Object.assign(() => finish(), { settled }); +} + +/** + * Kill a process tree immediately, without a grace window. + * + * Use only where waiting is impossible: host exit, or a stream that already failed. A shell + * `trap ... TERM` never runs on this path. + */ +export function killProcessTree(pid: number): void { + if (process.platform === "win32") { + // Use the trusted System32 executable so cleanup does not depend on PATH. + spawnWindowsTaskkill(pid, true); + return; + } + // `signalTree` already covers descendants that moved to their own group or session. + signalTree(pid, "SIGKILL"); +} diff --git a/packages/coding-agent/src/utils/shell.ts b/packages/coding-agent/src/utils/shell.ts index 43b72e10f..3087f5e76 100644 --- a/packages/coding-agent/src/utils/shell.ts +++ b/packages/coding-agent/src/utils/shell.ts @@ -1,9 +1,20 @@ -import { existsSync, readdirSync, readFileSync, realpathSync, statSync } from "node:fs"; +import { existsSync, realpathSync, statSync } from "node:fs"; import { delimiter, join, win32 } from "node:path"; import { type ChildProcess, spawn, spawnSync } from "child_process"; import { getBinDir } from "../config.ts"; +import { forgetProcess, HOST_OWNER, registerProcess, releaseAllProcessesSync } from "../core/process-registry.ts"; import { isStepStorageContext, resolveStepAgentDir } from "../step/environment.ts"; +// Process-tree signalling and ownership used to live in this module. They moved to their own +// modules so the registry can signal trees without importing shell configuration; these +// re-exports keep the existing call sites and the package entry point working. +export { + INTERRUPTED_COMMAND_GRACE_MS, + killProcessTree, + PROCESS_TREE_TERM_GRACE_MS, + terminateProcessTreeGracefully, +} from "./process-tree.ts"; + export interface ShellConfig { shell: string; args: string[]; @@ -259,112 +270,26 @@ export function sanitizeBinaryOutput(str: string): string { /** * Detached child processes must be tracked so they can be killed on parent * shutdown signals (SIGHUP/SIGTERM). + * + * These three functions are the pre-registry API: they know a pid but neither its owner nor its + * kind. They forward to the process registry, registering under `HOST_OWNER` with `kind: "bash"`, + * which is what every legacy call site meant. New code must call `registerProcess()` with the + * owning session and the real kind, so `listUnfinishedWork()` can tell a subagent from a dev server. */ -const trackedDetachedChildPids = new Set(); - export function trackDetachedChildPid(pid: number): void { - trackedDetachedChildPids.add(pid); + registerProcess({ pid, kind: "bash", ownerId: HOST_OWNER, label: "detached child" }); } export function untrackDetachedChildPid(pid: number): void { - trackedDetachedChildPids.delete(pid); -} - -export function killTrackedDetachedChildren(): void { - for (const pid of trackedDetachedChildPids) { - killProcessTree(pid); - } - trackedDetachedChildPids.clear(); -} - -type LinuxProcessIdentity = { pid: number; ppid: number; startTime: string }; - -function readLinuxProcessIdentity(pid: number): LinuxProcessIdentity | undefined { - try { - const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); - const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" "); - const startTime = fields[19]; - return startTime ? { pid, ppid: Number(fields[1]), startTime } : undefined; - } catch { - return undefined; - } -} - -/** Snapshot descendants before killing their parent makes them untraceable. */ -function getLinuxDescendants(pid: number): LinuxProcessIdentity[] { - const children = new Map(); - try { - // /proc//task//children is absent on some Linux kernels. - // The stat parent field is available without that optional interface. - for (const entry of readdirSync("/proc")) { - const childPid = Number(entry); - if (!Number.isSafeInteger(childPid) || childPid <= 0) continue; - const child = readLinuxProcessIdentity(childPid); - if (!child) continue; - const siblings = children.get(child.ppid) ?? []; - siblings.push(child); - children.set(child.ppid, siblings); - } - } catch { - return []; - } - const descendants: LinuxProcessIdentity[] = []; - const pending = [pid]; - const visited = new Set(pending); - for (let index = 0; index < pending.length; index++) { - for (const child of children.get(pending[index]) ?? []) { - if (visited.has(child.pid)) continue; - visited.add(child.pid); - descendants.push(child); - pending.push(child.pid); - } - } - return descendants; + forgetProcess(pid); } /** - * Kill a process and all its children (cross-platform) + * Kill everything this process registered, immediately. + * + * Kept for the modes that already call it on their own shutdown path. The registry also sweeps on + * `process.exit`, so a host that never calls this still reclaims its children. */ -export function killProcessTree(pid: number): void { - if (process.platform === "win32") { - // Use the trusted System32 executable so cleanup does not depend on PATH. - try { - const child = spawn( - join(process.env.SystemRoot ?? "C:\\Windows", "System32", "taskkill.exe"), - ["/F", "/T", "/PID", String(pid)], - { - stdio: "ignore", - detached: true, - windowsHide: true, - }, - ); - // A failed spawn emits "error" asynchronously; consume it to avoid crashing Node. - child.once("error", () => {}); - } catch { - // Ignore errors if taskkill fails. - } - } else { - const descendants = process.platform === "linux" ? getLinuxDescendants(pid) : []; - // Use SIGKILL on Unix/Linux/Mac - try { - process.kill(-pid, "SIGKILL"); - } catch { - // Fallback to killing just the child if process group kill fails - try { - process.kill(pid, "SIGKILL"); - } catch { - // Process already dead - } - } - // A detached child can have a different group/session. Target only the - // identities observed in this command's subtree, never all host processes. - for (const descendant of descendants.reverse()) { - if (readLinuxProcessIdentity(descendant.pid)?.startTime !== descendant.startTime) continue; - try { - process.kill(descendant.pid, "SIGKILL"); - } catch { - // The descendant already exited or was killed with the parent group. - } - } - } +export function killTrackedDetachedChildren(): void { + releaseAllProcessesSync(); } diff --git a/packages/coding-agent/test/background-command-lifecycle.test.ts b/packages/coding-agent/test/background-command-lifecycle.test.ts new file mode 100644 index 000000000..20cebc5f4 --- /dev/null +++ b/packages/coding-agent/test/background-command-lifecycle.test.ts @@ -0,0 +1,247 @@ +import { execFileSync, spawn } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; +import type { ExtensionContext } from "../src/core/extensions/types.ts"; +import { + forgetProcess, + HOST_OWNER, + listProcesses, + registerProcess, + releaseOwnerGracefully, +} from "../src/core/process-registry.ts"; +import { createStepToolProfile } from "../src/step/tool-profile.ts"; + +const workspaces: string[] = []; +const owners: string[] = []; + +function workspace(): string { + const dir = mkdtempSync(path.join(tmpdir(), "step-bg-command-")); + workspaces.push(dir); + return dir; +} + +function alive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + try { + const state = execFileSync("ps", ["-o", "state=", "-p", String(pid)], { encoding: "utf8" }).trim(); + return !state.startsWith("Z"); + } catch { + return false; + } +} + +/** Minimal extension context: the tools only read cwd and the session id. */ +/** + * A long-lived process that is not a background command, for tests that need a second kind in the + * same session. Registered by the caller; `afterEach` reaps it through `releaseOwnerGracefully`. + */ +async function ordinaryProcess(): Promise { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1 << 30)"], { + stdio: "ignore", + detached: true, + }); + child.unref(); + if (child.pid === undefined) throw new Error("child did not start"); + return child.pid; +} + +function contextFor(sessionId: string, cwd: string): ExtensionContext { + return { cwd, sessionManager: { getSessionId: () => sessionId } } as unknown as ExtensionContext; +} + +function tool(tools: ReturnType, name: string) { + const found = tools.find((candidate) => candidate.name === name); + if (!found) throw new Error(`tool ${name} not registered`); + return found; +} + +async function startBackground(cwd: string, sessionId: string, command: string) { + const tools = createStepToolProfile(cwd); + const result = await tool(tools, "run_command").execute( + "call-1", + { command, run_in_background: true }, + undefined, + undefined, + contextFor(sessionId, cwd), + ); + const logPath = (result as { details?: { logPath?: string } }).details?.logPath; + if (logPath) logs.add(logPath); + return result as { + content: Array<{ type: string; text?: string }>; + details?: { background?: boolean; kind?: string; pid?: number | null; logPath?: string }; + }; +} + +/** Log files this suite created, removed with the workspaces so a run leaves no trace. */ +const logs = new Set(); + +afterEach(async () => { + // Owner release is kind-agnostic on purpose, so this reaps every kind a test registered. + for (const owner of owners.splice(0)) await releaseOwnerGracefully(owner, 200).catch(() => undefined); + for (const dir of workspaces.splice(0)) rmSync(dir, { recursive: true, force: true }); + // A log file is a deliberate product artifact (it is how a stopped dev server stays inspectable), + // but this suite's own files are not the user's. Remove only the ones it created. + for (const log of logs) rmSync(log, { force: true }); + logs.clear(); +}); + +describe("run_in_background lifecycle", () => { + test("registers the command as bash work owned by the calling session", async () => { + const cwd = workspace(); + const sessionId = `session-${Math.random().toString(16).slice(2)}`; + owners.push(sessionId); + + const result = await startBackground(cwd, sessionId, "sleep 300"); + const pid = result.details?.pid; + expect(typeof pid).toBe("number"); + + // The tool result names the kind, so a host can tell a dev server from a subagent without + // re-deriving it from prose. + expect(result.details?.kind).toBe("bash"); + expect(result.details?.background).toBe(true); + expect(typeof result.details?.logPath).toBe("string"); + + const registered = listProcesses(sessionId); + expect(registered).toHaveLength(1); + expect(registered[0]?.kind).toBe("bash"); + expect(registered[0]?.pid).toBe(pid); + expect(registered[0]?.logPath).toBe(result.details?.logPath); + // A background command is a side effect, not unfinished work. + expect(registered[0]?.kind).not.toBe("subagent"); + }, 20_000); + + test("the command dies when its session is released, and another session's keeps running", async () => { + const cwdA = workspace(); + const cwdB = workspace(); + const sessionA = `session-a-${Math.random().toString(16).slice(2)}`; + const sessionB = `session-b-${Math.random().toString(16).slice(2)}`; + owners.push(sessionA, sessionB); + + const a = await startBackground(cwdA, sessionA, "sleep 300"); + const b = await startBackground(cwdB, sessionB, "sleep 300"); + const pidA = a.details?.pid as number; + const pidB = b.details?.pid as number; + expect(alive(pidA)).toBe(true); + expect(alive(pidB)).toBe(true); + + await releaseOwnerGracefully(sessionA, 500); + + expect(alive(pidA)).toBe(false); + expect(alive(pidB)).toBe(true); + expect(listProcesses(sessionA)).toEqual([]); + expect(listProcesses(sessionB)).toHaveLength(1); + }, 30_000); + + test("bash_list reports this session's commands only, and bash_kill stops one", async () => { + const cwd = workspace(); + const session = `session-${Math.random().toString(16).slice(2)}`; + const other = `session-${Math.random().toString(16).slice(2)}`; + owners.push(session, other); + + const started = await startBackground(cwd, session, "sleep 300"); + const pid = started.details?.pid as number; + const tools = createStepToolProfile(cwd); + const ctx = contextFor(session, cwd); + + const listed = (await tool(tools, "bash_list").execute("call-2", {}, undefined, undefined, ctx)) as { + content: Array<{ text?: string }>; + details?: { commands?: Array<{ pid: number; status: string }> }; + }; + expect(listed.details?.commands?.map((entry) => entry.pid)).toEqual([pid]); + expect(listed.content[0]?.text).toContain(String(pid)); + + const killed = (await tool(tools, "bash_kill").execute("call-3", { pid }, undefined, undefined, ctx)) as { + details?: { stopped?: number }; + }; + expect(killed.details?.stopped).toBe(1); + expect(alive(pid)).toBe(false); + expect(listProcesses(session)).toEqual([]); + }, 30_000); + + test("bash_kill with `all` stops only background commands, never the session's subagent trees", async () => { + const cwd = workspace(); + const session = `session-${Math.random().toString(16).slice(2)}`; + owners.push(session); + + const started = await startBackground(cwd, session, "sleep 300"); + const bashPid = started.details?.pid as number; + // A subagent tree the same session owns. `bash_kill` speaks only for background commands, so + // `all` must not reach it: the description promises commands, and a subagent is not one. + const subagentPid = await ordinaryProcess(); + registerProcess({ pid: subagentPid, kind: "subagent", ownerId: session, label: "subagent x" }); + + const tools = createStepToolProfile(cwd); + const result = (await tool(tools, "bash_kill").execute( + "call-all", + { all: true }, + undefined, + undefined, + contextFor(session, cwd), + )) as { details?: { stopped?: number } }; + + expect(result.details?.stopped).toBe(1); + expect(alive(bashPid)).toBe(false); + expect(alive(subagentPid)).toBe(true); + expect(listProcesses(session).map((entry) => entry.kind)).toEqual(["subagent"]); + }, 30_000); + + test("bash_kill refuses a pid that belongs to another session", async () => { + const cwdA = workspace(); + const cwdB = workspace(); + const sessionA = `session-a-${Math.random().toString(16).slice(2)}`; + const sessionB = `session-b-${Math.random().toString(16).slice(2)}`; + owners.push(sessionA, sessionB); + + const b = await startBackground(cwdB, sessionB, "sleep 300"); + const pidB = b.details?.pid as number; + + const tools = createStepToolProfile(cwdA); + await expect( + tool(tools, "bash_kill").execute("call-4", { pid: pidB }, undefined, undefined, contextFor(sessionA, cwdA)), + ).rejects.toThrow(/No background command/); + + // The other session's command is untouched. + expect(alive(pidB)).toBe(true); + }, 30_000); + + test("a host-owned command (no session) is still registered and reclaimable", async () => { + const cwd = workspace(); + const result = await startBackground(cwd, HOST_OWNER, "sleep 300"); + const pid = result.details?.pid as number; + expect(listProcesses(HOST_OWNER).some((entry) => entry.pid === pid)).toBe(true); + await releaseOwnerGracefully(HOST_OWNER, 300); + expect(alive(pid)).toBe(false); + forgetProcess(pid); + }, 20_000); + + test("the log file records the command's output", async () => { + const cwd = workspace(); + const session = `session-${Math.random().toString(16).slice(2)}`; + owners.push(session); + + const result = await startBackground(cwd, session, "echo step-background-probe"); + const logPath = result.details?.logPath as string; + const pid = result.details?.pid as number; + + // Wait for the write to land; the command exits on its own. + const deadline = Date.now() + 10_000; + let text = ""; + while (Date.now() < deadline) { + try { + text = readFileSync(logPath, "utf8"); + } catch { + text = ""; + } + if (text.includes("step-background-probe")) break; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + expect(text).toContain("step-background-probe"); + forgetProcess(pid); + }, 20_000); +}); diff --git a/packages/coding-agent/test/foreground-command-guard.test.ts b/packages/coding-agent/test/foreground-command-guard.test.ts new file mode 100644 index 000000000..e217d3d3a --- /dev/null +++ b/packages/coding-agent/test/foreground-command-guard.test.ts @@ -0,0 +1,100 @@ +import { execFileSync, spawn } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; + +const workspaces: string[] = []; + +function workspace(): string { + const dir = mkdtempSync(path.join(tmpdir(), "step-foreground-guard-")); + workspaces.push(dir); + return dir; +} + +function alive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + try { + const state = execFileSync("ps", ["-o", "state=", "-p", String(pid)], { encoding: "utf8" }).trim(); + return !state.startsWith("Z"); + } catch { + return false; + } +} + +async function waitFor(check: () => boolean, timeoutMs = 20_000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (check()) return; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw new Error("condition not met before the deadline"); +} + +afterEach(() => { + for (const dir of workspaces.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +/** + * A host script that runs one foreground command through the real shell operations, reports the + * command's child pid, then idles until it is killed. + * + * The point is what happens *after* the host dies: a foreground command is spawned detached, so + * nothing in-process can reclaim it once the host is gone. + */ +function foregroundHostScript(bashModulePath: string, readyPath: string): string { + // The command writes its own pid, then idles. It runs in the shell's own process group, so + // nothing in-process can reclaim it once the host is gone. + const childScript = `const fs=require("node:fs");fs.writeFileSync(${JSON.stringify(readyPath)},String(process.pid));setInterval(()=>{},1<<30)`; + return [ + 'import { writeFileSync } from "node:fs";', + `import { createLocalBashOperations } from ${JSON.stringify(bashModulePath)};`, + "const operations = createLocalBashOperations();", + `const command = ${JSON.stringify(`${process.execPath} -e ${JSON.stringify(childScript)}`)};`, + "const running = operations.exec(command, process.cwd(), { onData: () => {} });", + "void running;", + "setInterval(() => {}, 1 << 30);", + "void writeFileSync;", + ].join("\n"); +} + +describe("foreground command parent-death guard", () => { + test("a foreground command dies when the host is SIGKILLed", async () => { + const dir = workspace(); + const ready = path.join(dir, "ready"); + const hostFile = path.join(dir, "host.mts"); + writeFileSync( + hostFile, + foregroundHostScript(path.resolve(__dirname, "../src/core/tools/bash.ts"), ready), + "utf8", + ); + + const host = spawn(process.execPath, ["--import", "tsx", hostFile], { + stdio: "ignore", + env: process.env, + }); + if (host.pid === undefined) throw new Error("host did not start"); + + try { + await waitFor(() => existsSync(ready), 30_000); + const commandPid = Number.parseInt(readFileSync(ready, "utf8").trim(), 10); + expect(alive(commandPid)).toBe(true); + + // No shutdown handler runs on SIGKILL: the guard is the only thing that can reclaim it. + process.kill(host.pid, "SIGKILL"); + + await waitFor(() => !alive(commandPid), 20_000); + expect(alive(commandPid)).toBe(false); + } finally { + try { + process.kill(host.pid, "SIGKILL"); + } catch { + // Already gone. + } + } + }, 60_000); +}); diff --git a/packages/coding-agent/test/parent-death-guard.test.ts b/packages/coding-agent/test/parent-death-guard.test.ts new file mode 100644 index 000000000..06f47b145 --- /dev/null +++ b/packages/coding-agent/test/parent-death-guard.test.ts @@ -0,0 +1,144 @@ +import { execFileSync, spawn } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; + +const workspaces: string[] = []; +const spawned: number[] = []; + +function workspace(): string { + const dir = mkdtempSync(path.join(tmpdir(), "step-parent-guard-")); + workspaces.push(dir); + return dir; +} + +function alive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + try { + const state = execFileSync("ps", ["-o", "state=", "-p", String(pid)], { encoding: "utf8" }).trim(); + return !state.startsWith("Z"); + } catch { + return false; + } +} + +async function waitFor(check: () => boolean, timeoutMs = 15_000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (check()) return; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw new Error("condition not met before the deadline"); +} + +afterEach(() => { + // Kill the pid itself as well as its group: the host is spawned without `detached`, so its pid + // is not a process-group id and `kill(-pid)` would miss it — leaving a host running for as long + // as the test run, which is exactly the leak this suite exists to prevent. + for (const pid of spawned.splice(0)) { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // Not a group leader, or already gone. + } + try { + process.kill(pid, "SIGKILL"); + } catch { + // Already gone. + } + } + for (const dir of workspaces.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +/** + * A host script that arms the guard for a long-lived child and then reports the child's pid. + * + * The child ignores `SIGTERM`, so a passing test can only be the supervisor's `SIGKILL` + * escalation: a `SIGTERM`-only cleanup would leave it alive and fail the assertion. + * + * The host deliberately never disarms and never shuts down: the test kills it with SIGKILL, which is + * the case no in-process handler can cover. + */ +function guardedHostScript(guardModulePath: string, readyPath: string): string { + return [ + 'import { spawn } from "node:child_process";', + 'import { writeFileSync } from "node:fs";', + `import { armParentDeathGuard } from ${JSON.stringify(guardModulePath)};`, + 'const child = spawn(process.execPath, ["-e", "process.on(\\"SIGTERM\\", () => {}); setInterval(() => {}, 1 << 30)"], { stdio: "ignore", detached: true });', + "child.unref();", + "const guard = armParentDeathGuard(child.pid, 300);", + "if (!guard) throw new Error('guard unavailable');", + "await guard.ready;", + `writeFileSync(${JSON.stringify(readyPath)}, String(child.pid));`, + "setInterval(() => {}, 1 << 30);", + ].join("\n"); +} + +describe("parent-death guard", () => { + test("reclaims an armed child when the host is SIGKILLed", async () => { + const dir = workspace(); + const ready = path.join(dir, "ready"); + const hostScript = guardedHostScript(path.resolve(__dirname, "../src/utils/parent-death-guard.ts"), ready); + const hostFile = path.join(dir, "host.mts"); + writeFileSync(hostFile, hostScript, "utf8"); + + // Run the host with `node` directly, not through `tsx`: a tsx wrapper would be the process to + // kill, and the guard's lease belongs to the process that awaits `ready` in the script. + const host = spawn(process.execPath, ["--import", "tsx", hostFile], { + stdio: "ignore", + env: process.env, + }); + if (host.pid === undefined) throw new Error("host did not start"); + spawned.push(host.pid); + + await waitFor(() => existsSync(ready), 30_000); + const childPid = Number.parseInt(readFileSync(ready, "utf8").trim(), 10); + spawned.push(childPid); + // The guard must be armed for a live process: otherwise this test would pass by the child + // simply failing to start, and prove nothing about reclamation. + expect(alive(childPid)).toBe(true); + + // The whole point: no shutdown handler runs on SIGKILL. + process.kill(host.pid, "SIGKILL"); + + await waitFor(() => !alive(childPid), 20_000); + expect(alive(childPid)).toBe(false); + }, 60_000); + + test("a disarm keeps a normally exited child from being reported", async () => { + const dir = workspace(); + const childScript = path.join(dir, "child.mjs"); + const childReady = path.join(dir, "child-ready"); + const armed = path.join(dir, "armed"); + writeFileSync( + childScript, + [ + 'import { writeFileSync } from "node:fs";', + `import { armParentDeathGuard } from ${JSON.stringify( + path.resolve(__dirname, "../src/utils/parent-death-guard.ts"), + )};`, + "const guard = armParentDeathGuard(process.pid, 300);", + "if (!guard) throw new Error('guard unavailable');", + "await guard.ready;", + `writeFileSync(${JSON.stringify(childReady)}, "ready");`, + `writeFileSync(${JSON.stringify(armed)}, "armed");`, + // Disarm, then exit normally: the supervisor must not signal this pid. + "await guard.disarm();", + "process.exit(0);", + ].join("\n"), + "utf8", + ); + const run = spawn(process.execPath, ["--import", "tsx", childScript], { + stdio: "ignore", + env: process.env, + }); + const code = await new Promise((resolve) => run.once("exit", resolve)); + expect(code).toBe(0); + expect(alive(run.pid as number)).toBe(false); + }, 30_000); +}); diff --git a/packages/coding-agent/test/process-registry.test.ts b/packages/coding-agent/test/process-registry.test.ts new file mode 100644 index 000000000..c8dca9e77 --- /dev/null +++ b/packages/coding-agent/test/process-registry.test.ts @@ -0,0 +1,175 @@ +import { execFileSync, spawn } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; +import { + forgetProcess, + HOST_OWNER, + isUnfinishedWork, + listProcesses, + listUnfinishedWork, + registerProcess, + releaseAllProcessesSync, + releaseOwnerGracefully, + releaseOwnerProcessesSync, +} from "../src/core/process-registry.ts"; + +const workspaces: string[] = []; +const spawned: number[] = []; + +function workspace(): string { + const dir = mkdtempSync(path.join(tmpdir(), "step-process-registry-")); + workspaces.push(dir); + return dir; +} + +/** Wait until a file exists, so a signal is never sent before the child installed its handler. */ +async function waitForFile(file: string, timeoutMs = 10_000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (existsSync(file)) return; + await new Promise((resolve) => setTimeout(resolve, 10)); + } + throw new Error(`timed out waiting for ${file}`); +} + +/** A process that records SIGTERM and ignores it, so only the SIGKILL escalation can stop it. */ +async function stubborn(): Promise<{ pid: number; marker: string }> { + const dir = workspace(); + const marker = path.join(dir, "termed"); + const ready = path.join(dir, "ready"); + const script = [ + 'const fs = require("node:fs");', + `process.on("SIGTERM", () => fs.writeFileSync(${JSON.stringify(marker)}, "term"));`, + `fs.writeFileSync(${JSON.stringify(ready)}, "ready");`, + "setInterval(() => {}, 1 << 30);", + ].join(""); + const child = spawn(process.execPath, ["-e", script], { stdio: "ignore", detached: true }); + child.unref(); + if (child.pid === undefined) throw new Error("child did not start"); + spawned.push(child.pid); + await waitForFile(ready); + return { pid: child.pid, marker }; +} + +/** A plain long-lived process, ready to be signalled. */ +async function ordinary(): Promise { + const dir = workspace(); + const ready = path.join(dir, "ready"); + const script = [ + 'const fs = require("node:fs");', + `fs.writeFileSync(${JSON.stringify(ready)}, "ready");`, + "setInterval(() => {}, 1 << 30);", + ].join(""); + const child = spawn(process.execPath, ["-e", script], { stdio: "ignore", detached: true }); + child.unref(); + if (child.pid === undefined) throw new Error("child did not start"); + spawned.push(child.pid); + await waitForFile(ready); + return child.pid; +} + +/** + * Whether a pid is a running process. + * + * `kill(pid, 0)` alone is not enough: a child that this process has not reaped yet is a zombie, + * which still accepts signals. A zombie is dead for the purpose of these tests. + */ +function alive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + try { + const state = execFileSync("ps", ["-o", "state=", "-p", String(pid)], { encoding: "utf8" }).trim(); + return !state.startsWith("Z"); + } catch { + return false; + } +} + +afterEach(() => { + releaseAllProcessesSync(); + for (const pid of spawned.splice(0)) { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // Already gone. + } + } + // Anything a test forgot to stop is not the next test's problem. + for (const dir of workspaces.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe("process registry", () => { + test("bash does not count as unfinished work, subagent and workflow do", () => { + expect(isUnfinishedWork("bash")).toBe(false); + expect(isUnfinishedWork("subagent")).toBe(true); + expect(isUnfinishedWork("workflow")).toBe(true); + }); + + test("lists trees per owner and separates unfinished work from side effects", async () => { + const bashPid = await ordinary(); + const agentPid = await ordinary(); + registerProcess({ pid: bashPid, kind: "bash", ownerId: "session-a", label: "npm run dev" }); + registerProcess({ pid: agentPid, kind: "subagent", ownerId: "session-a", label: "subagent x" }); + registerProcess({ pid: await ordinary(), kind: "bash", ownerId: "session-b", label: "other session" }); + + expect( + listProcesses("session-a") + .map((entry) => entry.kind) + .sort(), + ).toEqual(["bash", "subagent"]); + expect(listUnfinishedWork("session-a").map((entry) => entry.kind)).toEqual(["subagent"]); + }); + + test("releases one owner's trees without touching another session's", async () => { + const mine = await ordinary(); + const theirs = await ordinary(); + registerProcess({ pid: mine, kind: "bash", ownerId: "session-a", label: "dev server" }); + registerProcess({ pid: theirs, kind: "bash", ownerId: "session-b", label: "other dev server" }); + + await releaseOwnerGracefully("session-a", 200); + + expect(listProcesses("session-a")).toEqual([]); + expect(listProcesses("session-b")).toHaveLength(1); + expect(alive(theirs)).toBe(true); + }); + + test("graceful release lets a SIGTERM handler run before the escalation", async () => { + const { pid, marker } = await stubborn(); + registerProcess({ pid, kind: "bash", ownerId: "session-a", label: "trap test" }); + + await releaseOwnerGracefully("session-a", 1_500); + + // The child wrote its marker on SIGTERM, then the escalation stopped it. + expect(existsSync(marker)).toBe(true); + expect(readFileSync(marker, "utf8")).toBe("term"); + expect(alive(pid)).toBe(false); + }, 20_000); + + test("sync release stops trees immediately and forgets them", async () => { + const pid = await ordinary(); + registerProcess({ pid, kind: "bash", ownerId: HOST_OWNER, label: "host child" }); + + const released = releaseOwnerProcessesSync(HOST_OWNER); + + expect(released).toBe(1); + expect(listProcesses(HOST_OWNER)).toEqual([]); + }); + + test("a re-registered pid replaces the previous record", async () => { + const pid = await ordinary(); + registerProcess({ pid, kind: "bash", ownerId: "session-a", label: "first" }); + registerProcess({ pid, kind: "subagent", ownerId: "session-b", label: "second" }); + + expect(listProcesses()).toHaveLength(1); + expect(listProcesses("session-b")[0]?.kind).toBe("subagent"); + }); + + test("forgetting an unknown pid is a no-op", () => { + expect(() => forgetProcess(999_999_999)).not.toThrow(); + }); +}); diff --git a/packages/coding-agent/test/step-tool-profile.test.ts b/packages/coding-agent/test/step-tool-profile.test.ts index 298b7ae01..b4815f549 100644 --- a/packages/coding-agent/test/step-tool-profile.test.ts +++ b/packages/coding-agent/test/step-tool-profile.test.ts @@ -76,6 +76,16 @@ describe("Step tool profile", () => { "Run a non-interactive shell command from the initial working directory by default. Use for tests, builds, formatters, git, and project scripts; prefer dedicated file/search tools for reading and searching.", promptSnippet: "Run a non-interactive shell command", }, + bash_list: { + description: + "List the background commands this session started, with pid, status, and log path. Background commands keep running across a session switch and stop when the session is deleted or the app exits.", + promptSnippet: "List this session's background commands", + }, + bash_kill: { + description: + "Stop background commands this session started. Accepts one pid or `all`. Sends SIGTERM, then SIGKILL after a short grace period. Only the caller's own session's commands can be stopped.", + promptSnippet: "Stop a background command started by this session", + }, find_tools: { description: "Search registered tools by natural-language intent, tool name, description, and parameter names.", diff --git a/packages/coding-agent/test/subagent-child-env.test.ts b/packages/coding-agent/test/subagent-child-env.test.ts index a01a956e4..d99513bf1 100644 --- a/packages/coding-agent/test/subagent-child-env.test.ts +++ b/packages/coding-agent/test/subagent-child-env.test.ts @@ -177,7 +177,12 @@ test("a child that acks the prompt then goes silent settles instead of stranding const result = await session.runTurn(runInput({ turnIdleTimeoutMs: 400 })); expect(result.stopReason).toBe("error"); expect(result.errorMessage).toMatch(/produced no output/); - session.stop(); + // `stop` only asks the child to leave (it closes stdin). The fake child intentionally ignores + // that, so a test that must not leak a process has to terminate it, exactly as a real host + // does when a lane cannot settle on its own. + session.terminate(); + await session.exited; + liveSubagentSessions.clear(); } finally { process.argv[1] = originalArgv1; } @@ -203,7 +208,10 @@ test("streaming output keeps resetting the idle budget", async () => { const result = await session.runTurn(runInput({ turnIdleTimeoutMs: 400 })); expect(result.stopReason).toBeUndefined(); expect(result.exitCode).toBe(0); - session.stop(); + // Same reason as the idle test: the fake child ignores stdin EOF, so terminate it explicitly. + session.terminate(); + await session.exited; + liveSubagentSessions.clear(); } finally { process.argv[1] = originalArgv1; }