From 2ae63f4aa2a2af7b0ed605582aef7c9025297e13 Mon Sep 17 00:00:00 2001 From: julius Date: Wed, 4 Mar 2026 04:54:58 -0800 Subject: [PATCH] Fix shell: true side effects on Windows (orphaned processes, swallowed ENOENT) Add tree-kill via taskkill /T /F in processRunner and codexAppServerManager so timeout enforcement and session stop properly terminate the entire process tree instead of just the cmd.exe wrapper. Detect Windows command not found (exit 9009) in normalizeExitError. Pre-validate editor commands with isCommandAvailable in open.ts before spawning. Fix ProviderHealth test import and dev-runner test path assertion for Windows. Made-with: Cursor --- apps/server/src/codexAppServerManager.ts | 21 +++++++- apps/server/src/open.ts | 52 +++++++++++-------- apps/server/src/processRunner.ts | 35 +++++++++++-- .../provider/Layers/ProviderHealth.test.ts | 3 +- scripts/dev-runner.test.ts | 3 +- 5 files changed, 84 insertions(+), 30 deletions(-) diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts index b197a07fd447a54fd553ad089a4715c1114cd840..7b19c7be5ce8c19daeef4063c7672dbb038e3a37 100644 --- a/apps/server/src/codexAppServerManager.ts +++ b/apps/server/src/codexAppServerManager.ts @@ -1,4 +1,4 @@ -import { type ChildProcessWithoutNullStreams, spawn } from "node:child_process"; +import { type ChildProcessWithoutNullStreams, spawn, spawnSync } from "node:child_process"; import { randomUUID } from "node:crypto"; import { EventEmitter } from "node:events"; import readline from "node:readline"; @@ -104,6 +104,23 @@ const RECOVERABLE_THREAD_RESUME_ERROR_SNIPPETS = [ "does not exist", ]; +/** + * On Windows with `shell: true`, `child.kill()` only terminates the `cmd.exe` + * wrapper, leaving the actual command running. Use `taskkill /T` to kill the + * entire process tree instead. + */ +function killChildTree(child: ChildProcessWithoutNullStreams): void { + if (process.platform === "win32" && child.pid !== undefined) { + try { + spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); + return; + } catch { + // fallback to direct kill + } + } + child.kill(); +} + export function normalizeCodexModelSlug( model: string | undefined | null, preferredId?: string, @@ -476,7 +493,7 @@ export class CodexAppServerManager extends EventEmitter - Effect.callback((resume) => { - let child; - try { - child = spawn(launch.command, [...launch.args], { - detached: true, - stdio: "ignore", - shell: process.platform === "win32", - }); - } catch (error) { - return resume( - Effect.fail(new OpenError({ message: "failed to spawn detached process", cause: error })), + Effect.gen(function* () { + if (!isCommandAvailable(launch.command)) { + return yield* new OpenError({ message: `Editor command not found: ${launch.command}` }); + } + + yield* Effect.callback((resume) => { + let child; + try { + child = spawn(launch.command, [...launch.args], { + detached: true, + stdio: "ignore", + shell: process.platform === "win32", + }); + } catch (error) { + return resume( + Effect.fail( + new OpenError({ message: "failed to spawn detached process", cause: error }), + ), + ); + } + + const handleSpawn = () => { + child.unref(); + resume(Effect.void); + }; + + child.once("spawn", handleSpawn); + child.once("error", (cause) => + resume(Effect.fail(new OpenError({ message: "failed to spawn detached process", cause }))), ); - } - - const handleSpawn = () => { - child.unref(); - resume(Effect.void); - }; - - child.once("spawn", handleSpawn); - child.once("error", (cause) => - resume(Effect.fail(new OpenError({ message: "failed to spawn detached process", cause }))), - ); + }); }); const make = Effect.gen(function* () { diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts index 75c939617f160135759ca10f6a1752eda00b441a..cbf4b76063fdab636a160f58d611a50c3b138a3c 100644 --- a/apps/server/src/processRunner.ts +++ b/apps/server/src/processRunner.ts @@ -1,4 +1,4 @@ -import { spawn } from "node:child_process"; +import { type ChildProcess as ChildProcessHandle, spawn, spawnSync } from "node:child_process"; export interface ProcessRunOptions { cwd?: string | undefined; @@ -37,11 +37,21 @@ function normalizeSpawnError(command: string, args: readonly string[], error: un return new Error(`Failed to run ${commandLabel(command, args)}: ${error.message}`); } +function isWindowsCommandNotFound(code: number | null, stderr: string): boolean { + if (process.platform !== "win32") return false; + if (code === 9009) return true; + return /is not recognized as an internal or external command/i.test(stderr); +} + function normalizeExitError( command: string, args: readonly string[], result: ProcessRunResult, ): Error { + if (isWindowsCommandNotFound(result.code, result.stderr)) { + return new Error(`Command not found: ${command}`); + } + const reason = result.timedOut ? "timed out" : `failed (code=${result.code ?? "null"}, signal=${result.signal ?? "null"})`; @@ -70,6 +80,23 @@ function normalizeBufferError( const DEFAULT_MAX_BUFFER_BYTES = 8 * 1024 * 1024; +/** + * On Windows with `shell: true`, `child.kill()` only terminates the `cmd.exe` + * wrapper, leaving the actual command running. Use `taskkill /T` to kill the + * entire process tree instead. + */ +function killChild(child: ChildProcessHandle, signal: NodeJS.Signals = "SIGTERM"): void { + if (process.platform === "win32" && child.pid !== undefined) { + try { + spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); + return; + } catch { + // fallback to direct kill + } + } + child.kill(signal); +} + function appendChunkWithinLimit( target: string, currentBytes: number, @@ -127,9 +154,9 @@ export async function runProcess( const timeoutTimer = setTimeout(() => { timedOut = true; - child.kill("SIGTERM"); + killChild(child, "SIGTERM"); forceKillTimer = setTimeout(() => { - child.kill("SIGKILL"); + killChild(child, "SIGKILL"); }, 1_000); }, timeoutMs); @@ -144,7 +171,7 @@ export async function runProcess( }; const fail = (error: Error): void => { - child.kill("SIGTERM"); + killChild(child, "SIGTERM"); finalize(() => { reject(error); }); diff --git a/apps/server/src/provider/Layers/ProviderHealth.test.ts b/apps/server/src/provider/Layers/ProviderHealth.test.ts index 2c190179b03121e88b340f25309213df71a59ef4..58eac64da0a1b8a1f8d4d8c8595b44a965383394 100644 --- a/apps/server/src/provider/Layers/ProviderHealth.test.ts +++ b/apps/server/src/provider/Layers/ProviderHealth.test.ts @@ -1,4 +1,5 @@ -import { assert, it } from "@effect/vitest"; +import assert from "node:assert/strict"; +import { it } from "@effect/vitest"; import { Effect, Layer, Sink, Stream } from "effect"; import * as PlatformError from "effect/PlatformError"; import { ChildProcessSpawner } from "effect/unstable/process"; diff --git a/scripts/dev-runner.test.ts b/scripts/dev-runner.test.ts index ee8094970d1b7ffb8ed8f63890a49fa91d631484..61809a40f3260038c2d6820d2722b7e2af73e5d6 100644 --- a/scripts/dev-runner.test.ts +++ b/scripts/dev-runner.test.ts @@ -1,4 +1,5 @@ import * as NodeServices from "@effect/platform-node/NodeServices"; +import { resolve } from "node:path"; import { assert, describe, it } from "@effect/vitest"; import { Effect } from "effect"; @@ -86,7 +87,7 @@ it.layer(NodeServices.layer)("dev-runner", (it) => { devUrl: new URL("http://localhost:7331"), }); - assert.equal(env.T3CODE_STATE_DIR, "/tmp/override-state"); + assert.equal(env.T3CODE_STATE_DIR, resolve("/tmp/override-state")); assert.equal(env.T3CODE_PORT, "4222"); assert.equal(env.VITE_WS_URL, "ws://localhost:4222"); assert.equal(env.T3CODE_NO_BROWSER, "1"); -- 2.54.0