diff --git a/.memory/run-command-process-cleanup.md b/.memory/run-command-process-cleanup.md new file mode 100644 index 000000000..3d8d14848 --- /dev/null +++ b/.memory/run-command-process-cleanup.md @@ -0,0 +1,25 @@ +# run_command process-group cleanup, issue #424 + +Unix `run_command` owns its detached process group until requested termination finishes. Shell `close` settles the command only after the SIGTERM-to-SIGKILL cleanup promise completes or the group is confirmed absent. A closing shell alone does not cancel escalation: a redirected background descendant can remain in the group. Successful commands take no cleanup delay, and abort, timeout, output-limit, and spawn-error result formatting stays the same. + +The existing coding-tools suite now runs a Node worker that ignores SIGTERM and writes a heartbeat. It covers early shell closure, a resistant foreground process, timeout, and output overflow. The cancellation case fails against `11b58367` because the heartbeat continues after the command settles. The test has fallback PID cleanup and skips Windows, whose process-tree behavior is outside this issue. No Remote, UI, native-client, or onboarding contract changed; no plan status changed. + +Local verification on macOS Node 26.10.0: focused process regression, root type-check, focused ESLint, and `npm run test:cli` (99 tests) pass. The full coding-tools suite is 58/59 because an unrelated native-glob comparison fails identically on unchanged production source; its one targeted rerun also fails. Linux verification was unavailable because the configured Docker daemon is stopped. + +## Latest-main integration, 2026-10-16 + +Merged `origin/main` at `ee218d1f` into PR #494 with merge commit `555fdfd26`. The sole conflict was `CodingToolOptions`: retain this PR's test deadline seam and main's native `fileMutatorBinary` together. `package.json` matches main exactly, so its test scripts are preserved. The merged coding-tools file runs both the native edit/write helper and the process-group cleanup. + +After building the native file mutator, eight focused `run_command`/`edit_file` cases passed. Full coding-tools passed 66/67; only the pre-existing native-glob symlink comparison failed. Root and CLI type checks, focused ESLint, and the full bundled CLI suite (99/99) passed. Hosted exact-head checks are pending. + +Main advanced through the 0.57.0 release while these checks ran. Merged `origin/main` at `515de673` as `7452b181` without conflicts; it changed release metadata and the plan index only. With refreshed root dependencies, the native helper build, eight focused process/edit cases, root and CLI type checks, focused ESLint, and bundled CLI suite (99/99) all passed again. The root `package.json` still matches main exactly. Hosted validation is pending on the pushed integration head. + +Merged `origin/main` at `716636cf9` as `a1cdee738` without conflicts. This brings in the Bot Files owner-authority fix (#509) and simulator coordinate assertion fix (#508). `run_command` source and tests, `package.json`, the Remote contract revision (32), native clients, and protocol fixtures are unchanged by this merge. The CLI Bot Files constructor now receives its owner audience while `run_command` remains in the pinned coding-tools path. + +Post-merge local checks passed: eight focused process/edit tests; 162 Remote Bot Files/router/service tests (one skip); root and CLI type checks; scoped ESLint; native helper and desktop builds; Android `AidenBotContractTest`; and iOS `AidenRemoteClientTests` on an iPhone 18 Pro simulator. The CLI bundled suite passed 97/99 on its first resource-contended run, with two long-running Bot/codemode cases failing; both passed on one isolated rerun. The focused simulator stream E2E case timed out before its simulator assertion while a model response was still generating, then passed on one rerun. Treat those first-run failures as flakes, not clean first passes. Hosted exact-head validation remains pending. + +Merged `origin/main` at `264b537a4` as `f8f420cac` without conflicts. This includes #510 (Bot Web Search), #511 (keyboard focus/input modality), and #512 (Bot message attachments, Remote revision 33). The root `package.json` matches main exactly; revision 33 matches the shared fixture. Relative to current main, this PR still changes only the process cleanup and its test, plus its memory/papercut notes. + +On that merge, focused process/edit tests passed 8/8; Remote Bot Files/session/router/protocol tests passed 192/192; Bot authority/application and input-modality tests passed 84/84. Root, CLI, and E2E TypeScript checks, scoped ESLint, CLI bundle build, desktop build, Android `AidenBotContractTest`, and iOS `AidenBotSessionTests` plus `AidenRemoteClientTests` on an iPhone 18 Pro simulator passed. Hosted exact-head CI and reviews are still required before merge. + +Merged `origin/main` at `5661bae0e` as `9e694dfba` without conflicts. Main's #496 changes only `device-service.ts`, `local-device-host.ts`, and their lifecycle tests. The agent-device shim that can be placed on `run_command`'s PATH still uses its existing service path; this PR's process cleanup and tests remain the sole production/test delta against main. `package.json` and the Remote revision 33 fixture match main exactly. Local Device Hub service/host tests passed 80/80, focused process/edit tests 8/8, root and CLI type checks, scoped ESLint, desktop build, and `git diff --check` passed. New exact-head hosted CI and reviews remain required. diff --git a/.papercuts/run-command-process-cleanup.md b/.papercuts/run-command-process-cleanup.md new file mode 100644 index 000000000..3f763dc09 --- /dev/null +++ b/.papercuts/run-command-process-cleanup.md @@ -0,0 +1,13 @@ +# run_command cleanup workflow + +- The managed worktree started without `node_modules`, so the focused regression could not run until `npm ci --ignore-scripts` populated root dependencies. The install completed in 7 seconds. +- Node 26.10.0's native `glob` follows the fixture symlink for `**/*`, while the existing `foreground glob agrees with native glob` test expects it not to. The full coding-tools run failed 58/59; one targeted rerun and an unchanged-production baseline both failed the same way. This is separate from process cleanup. +- Linux verification was unavailable locally: `docker` is installed, but its configured OrbStack daemon socket is not running. +- The 2026-10-16 main merge conflicted only where #494's test deadline option and main's native file-mutator option were added to the same interface. Keeping both resolved it; the root test script matched main without manual edits. +- `.memory/` and `.papercuts/` are ignored even for previously committed files; a path-specific `git add` rejected their updates. Stage tracked updates with `git add -u` instead. +- Main advanced to the 0.57.0 release while the first merged head was being validated. A second ordinary merge and focused rerun were needed before publishing the final exact head. +- The Android Gradle wrapper lives in `android/`, so an initial repository-root `./gradlew` invocation failed before any build work. The focused test was rerun from `android/`. +- Running bundled CLI and native iOS/Android builds together saturated the host: the CLI Bot parity and codemode tests took roughly 16 minutes and failed with authority revocation/SIGTERM (exit 143). Both passed on one isolated rerun; the Bot case still took 13 minutes. This appears timing-sensitive and is not evidence that the tests are healthy under load. +- The simulator stream E2E case first timed out waiting for a saved assistant reply while generation remained active, before reaching the #508 coordinate assertion. It passed on one rerun (14.6 seconds); keep the first failure visible as a flake. +- Main advanced again after the `716636cf9` integration was published; #510, #511, and #512 required another clean merge and revision-33 contract verification before the PR could receive final exact-head checks. +- Main advanced once more with #496 after the revision-33 head was pushed, requiring another merge and local Device Hub lifecycle rerun before exact-head validation. diff --git a/main/services/coding-tools.test.ts b/main/services/coding-tools.test.ts index cf4d11ffe..87c4978cd 100644 --- a/main/services/coding-tools.test.ts +++ b/main/services/coding-tools.test.ts @@ -1601,6 +1601,73 @@ test("run_command cancellation kills the shell process group", async () => { } }); +test("run_command waits for resistant process-group cleanup after cancellation, timeout, and output overflow", async () => { + if (process.platform === "win32") return; + for (const trigger of ["cancel", "timeout", "output", "foreground"] as const) { + const root = await fs.mkdtemp(path.join(os.tmpdir(), `aiden-command-${trigger}-`)); + const controller = new AbortController(); + let running: Promise | undefined; + try { + await fs.writeFile(path.join(root, "worker.cjs"), ` +const fs = require("node:fs"); +process.on("SIGTERM", () => {}); +fs.writeFileSync("ready", String(process.pid)); +setInterval(() => fs.appendFileSync("heartbeat", "x"), 25); +`); + if (trigger === "output") { + await fs.writeFile(path.join(root, "output.cjs"), 'process.stdout.write("x".repeat(11 * 1024 * 1024));'); + } + const command = trigger === "foreground" + ? "node worker.cjs" + : trigger === "output" + ? "node worker.cjs >/dev/null 2>&1 & while [ ! -f ready ]; do sleep 0.01; done; node output.cjs; wait" + : "node worker.cjs >/dev/null 2>&1 & wait"; + const runCommand = buildCodingTools(root, undefined, { + commandTimeoutMs: trigger === "timeout" ? 2_000 : undefined, + }).find((tool) => tool.name === "run_command")!; + running = runCommand.execute(trigger, { command }, controller.signal); + + let pid = 0; + const readyDeadline = Date.now() + 5_000; + while (Date.now() < readyDeadline) { + pid = Number(await fs.readFile(path.join(root, "ready"), "utf8").catch(() => "0")); + const heartbeat = await fs.readFile(path.join(root, "heartbeat")).catch(() => Buffer.alloc(0)); + if (pid > 0 && heartbeat.length >= 2) break; + await new Promise((resolve) => setTimeout(resolve, 25)); + } + assert.ok(pid > 0, `${trigger}: worker did not become ready`); + if (trigger === "cancel" || trigger === "foreground") { + controller.abort(new Error(`${trigger} cancellation`)); + await assert.rejects(running, new RegExp(`${trigger} cancellation`, "u")); + } else { + const result = await running as Awaited>; + assert.equal(result.isError, true); + assert.match( + result.content[0].type === "text" ? result.content[0].text : "", + trigger === "timeout" ? /^Command timed out\./u : /^Command exceeded the output limit\./u, + ); + } + + const before = (await fs.readFile(path.join(root, "heartbeat"))).length; + await new Promise((resolve) => setTimeout(resolve, 250)); + const after = (await fs.readFile(path.join(root, "heartbeat"))).length; + assert.equal(after, before, `${trigger}: descendant kept writing after the command settled`); + const status = await execFileAsync("ps", ["-o", "stat=", "-p", String(pid)]) + .then(({ stdout }) => stdout.trim()) + .catch(() => ""); + assert.ok(!status || status.startsWith("Z"), `${trigger}: descendant is still executing (${status})`); + } finally { + controller.abort(new Error("fixture cleanup")); + await running?.catch(() => {}); + const pid = Number(await fs.readFile(path.join(root, "ready"), "utf8").catch(() => "0")); + if (pid > 0) { + try { process.kill(pid, "SIGKILL"); } catch { /* Already exited. */ } + } + await fs.rm(root, { recursive: true, force: true }); + } + } +}); + test("agent commands find user CLIs from a macOS GUI launch PATH", async () => { const home = await fs.mkdtemp(path.join(os.tmpdir(), "aiden-agent-path-")); try { diff --git a/main/services/coding-tools.ts b/main/services/coding-tools.ts index e60c954b9..092a3bd3e 100644 --- a/main/services/coding-tools.ts +++ b/main/services/coding-tools.ts @@ -1943,6 +1943,8 @@ function makeParentGrep(workspace: WorkspaceRootGuard): AgentTool { */ export interface CodingToolOptions { pathPrefix?: string | (() => string | null | undefined); + /** A shorter deadline for process-lifecycle tests; production uses the fixed limit. */ + commandTimeoutMs?: number; /** Trusted native helper from the application installation, if packaged outside Electron. */ fileMutatorBinary?: string; } @@ -2005,6 +2007,18 @@ function makeRunCommand(workspace: WorkspaceRootGuard, options: CodingToolOption let settled = false; let terminationRequested = false; let forceKill: ReturnType | undefined; + let finishTermination: (() => void) | undefined; + let terminationCleanup: Promise | undefined; + + const processGroupExists = () => { + if (process.platform === "win32" || !child.pid) return false; + try { + process.kill(-child.pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code !== "ESRCH"; + } + }; const signalProcess = (processSignal: NodeJS.Signals) => { if (process.platform !== "win32" && child.pid) { @@ -2021,10 +2035,17 @@ function makeRunCommand(workspace: WorkspaceRootGuard, options: CodingToolOption if (terminationRequested) return; terminationRequested = true; signalProcess("SIGTERM"); - forceKill = setTimeout(() => { - if (!settled) signalProcess("SIGKILL"); - }, 1_000); - forceKill.unref?.(); + if (!child.pid) return; + terminationCleanup = new Promise((resolve) => { + finishTermination = resolve; + forceKill = setTimeout(() => { + // The shell can close before its descendants leave the process group. + // Keep this timer owned by the group, independent of shell settlement. + signalProcess("SIGKILL"); + finishTermination?.(); + finishTermination = undefined; + }, 1_000); + }); }; const abort = () => { aborted = true; @@ -2046,30 +2067,39 @@ function makeRunCommand(workspace: WorkspaceRootGuard, options: CodingToolOption if (settled) return; settled = true; clearTimeout(timeout); - clearTimeout(forceKill); signal?.removeEventListener("abort", abort); - reject(error); + void (async () => { + await terminationCleanup; + reject(error); + })(); }); child.once("close", (exitCode) => { if (settled) return; settled = true; clearTimeout(timeout); - clearTimeout(forceKill); signal?.removeEventListener("abort", abort); - resolve({ - stdout: Buffer.concat(stdout).toString("utf-8"), - stderr: Buffer.concat(stderr).toString("utf-8"), - exitCode, - timedOut, - outputLimitExceeded, - aborted, - }); + if (finishTermination && !processGroupExists()) { + clearTimeout(forceKill); + finishTermination(); + finishTermination = undefined; + } + void (async () => { + await terminationCleanup; + resolve({ + stdout: Buffer.concat(stdout).toString("utf-8"), + stderr: Buffer.concat(stderr).toString("utf-8"), + exitCode, + timedOut, + outputLimitExceeded, + aborted, + }); + })(); }); signal?.addEventListener("abort", abort, { once: true }); const timeout = setTimeout(() => { timedOut = true; terminate(); - }, COMMAND_TIMEOUT_MS); + }, options.commandTimeoutMs ?? COMMAND_TIMEOUT_MS); timeout.unref?.(); }); if (result.aborted) {