From 34713fcd472e99de921014945e574ccda5418652 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:00:58 +0000 Subject: [PATCH 1/5] fix: tree-kill a cancelled video render on Windows (#4171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spawnDetached's win32 fallback returned a bare ChildProcess, so the cancel and watchdog paths in videoGen/local.js killed only the python runner — whatever it spawned (the ffmpeg mux, a model download) survived as an orphan holding the output file and GPU memory. killProcessGroup couldn't help: it is implemented as a POSIX `-pid` signal. The win32 handle now gets its own kill that delegates to killProcessTree (taskkill /T /F). The POSIX path is untouched — signalPid also serves reattached/reaped runs by raw pid, which killProcessTree does not. --- .changelog/next/fixed-issue-4171.md | 1 + server/lib/README.md | 2 +- server/lib/detachedSpawn.js | 24 +++++++++++++- server/lib/detachedSpawn.test.js | 51 +++++++++++++++++++++++++++-- 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 .changelog/next/fixed-issue-4171.md diff --git a/.changelog/next/fixed-issue-4171.md b/.changelog/next/fixed-issue-4171.md new file mode 100644 index 0000000000..fa8072b5c4 --- /dev/null +++ b/.changelog/next/fixed-issue-4171.md @@ -0,0 +1 @@ +- Cancelling a video render on Windows now tree-kills the runner, so ffmpeg/download children die with it instead of orphaning and holding GPU memory diff --git a/server/lib/README.md b/server/lib/README.md index 964d00ce4f..339a86e706 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -191,7 +191,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `commandExists.js` | `commandExists(cmd, args = ['--version'], { timeoutMs = 5_000 })` — does running `cmd args` succeed? A capability probe (`execFile`-based), not a PATH lookup like `processEnv.js`'s `whichFirst`. Consolidates the two previously-private copies in `localLlm.js`/`ollamaManager.js`; callers probing a heavier CLI (e.g. `codeReview.js`'s reviewer-binary probe) pass a longer `timeoutMs`. | | `spawnCwd.js` | `resolveSpawnCwd(workspacePath, fallbackRoot, label)` — resolves and **logs** the working directory a run/agent spawns into (expanding `~`), and throws when a workspace was requested but is missing / not a directory. Behind `services/runner.js#resolveRunCwd`, which turns that throw into a normal failed-run record for the two spawning runners. Stops a bad app `repoPath` from silently spawning in the PortOS checkout (#3180). `withSpawnCwdEnv(env, cwd)` — returns a copy of `env` with `PWD` pinned to `cwd` (dropping stale case-variant keys), because `spawn({ cwd })` doesn't rewrite the inherited `PWD` and OpenCode resolves its project root as `process.env.PWD ?? process.cwd()` (#3193). Apply it at every spawn that names its own cwd — the shared wrappers (`bufferedSpawn`, `spawnDetached`) already do, so their callers inherit it. `spawnCwd.test.js` discovers cwd-passing spawns across `server/` and fails on any that neither pins nor is listed exempt. | `commandSecurity.js` | Allowlist of safe shell commands + `validatePm2Command(args)` (rejects daemon-wide `pm2 kill`/`startup`/`unstartup` and ` all`). `validateCommand` runs the pm2 check for `pm2` base commands. Mirrored by the `agentGuard/` PATH shim for agentic paths. | -| `detachedSpawn.js` | `spawnDetached(bin, args, {controlDir,env,cwd,killProcessGroup?})` → ChildProcess-like handle for a long media job that SURVIVES `pm2 restart portos-server`. A pure-`sh` double-fork reparents the job to init (escaping pm2's PPID-based TreeKill — `detached:true` alone doesn't, since it only changes the process group); the server tails on-disk log files for `stdout`/`stderr`/`close`. Group-kill mode persists a marker so cancel, reattach, and orphan reaping terminate a group-leader wrapper plus every runtime child together. Used by loraTraining + videoGen. Also exports `reattachDetached(controlDir)` / `isReattachable(controlDir)` to RE-ATTACH a survivor after a restart (boot re-attach, #1332) and `reapDetached` to checkpoint-kill one when re-attach isn't possible. | +| `detachedSpawn.js` | `spawnDetached(bin, args, {controlDir,env,cwd,killProcessGroup?})` → ChildProcess-like handle for a long media job that SURVIVES `pm2 restart portos-server`. A pure-`sh` double-fork reparents the job to init (escaping pm2's PPID-based TreeKill — `detached:true` alone doesn't, since it only changes the process group); the server tails on-disk log files for `stdout`/`stderr`/`close`. Group-kill mode persists a marker so cancel, reattach, and orphan reaping terminate a group-leader wrapper plus every runtime child together. Windows has no double-fork (plain-spawn fallback), so its handle's `kill` delegates to `killProcessTree` (`taskkill /T /F`) — a cancel there takes the runner's children with it. Used by loraTraining + videoGen. Also exports `reattachDetached(controlDir)` / `isReattachable(controlDir)` to RE-ATTACH a survivor after a restart (boot re-attach, #1332) and `reapDetached` to checkpoint-kill one when re-attach isn't possible. | | `hostShutdown.js` | Tells "PortOS was restarted out from under a running agent" apart from "the agent failed" (#3202). `markHostShuttingDown()` / `isHostShuttingDown()` are the in-process latch the SIGTERM/SIGINT handler sets first thing; `shouldAbandonForHostShutdown({sentinelPresent,terminatedByUser,paused})` keeps every spawn path on the same preserve-vs-finalize policy. `writeHostShutdownMarker({agentIds,signal})` / `readHostShutdownMarker()` / `clearHostShutdownMarker()` persist that verdict to `data/cos/host-shutdown.json` so the NEXT boot's orphan sweep can requeue those agents as *interrupted* — no orphan-retry charge, no 30-minute cooldown. All non-throwing: a missing marker degrades to the ordinary orphan path. | | `execGit.js` | `execGit` utility imported by `git.js` + worktree manager. | | `ffmpeg.js` | Shared ffmpeg helpers (videoGen + videoTimeline). Includes `probeFrameCount(videoPath)` (metadata `nb_frames`, falling back to a real `-count_frames` pass — expensive, so call it once per file) and `trimVideoFromFrame(videoPath, outPath, {startFrame, fps})` — a frame-EXACT head cut via the `trim` filter (an `-ss` seek can drift a frame, which reads as a stutter at a stitch seam), keeping audio in sync with `atrim` when the clip has a soundtrack and taking `-an` when it doesn't. Re-encodes by necessity, so a later concat must re-encode too; `outPath` may equal the input (temp-file + rename install). Used by the chained-render context window — see `videoContinuity.js`. `buildTrimConcatArgs({inputs, outPath, width, height, fps, withAudio})` builds the argv for the other half of that job: a concat that drops leading frames from some of its inputs inside a `filter_complex` graph, so the cuts ride along with the timeline encode instead of costing one pre-encode per clip (pass `withAudio` only when EVERY input has an audio stream — check with `hasAudioStream`). `H264_ENCODE_ARGS` / `AAC_ENCODE_ARGS` are the shared encode profile: clips produced by different paths here get concatenated together, so a mismatch shows up as one segment graded differently from its neighbours — spread these rather than re-typing the flags. Every re-encode here also pins BT.709: `BT709_CONTAINER_ARGS` (the `colr` atom, always emitted) plus `bt709TagFilter()` → the `BT709_TAG_FILTER` `setparams=…` string, or `null` on an ffmpeg without that filter (`supportsSetparamsFilter()` probes `-filters` once per process; `null` = not probed, and a probe that couldn't run stays uncached). Both halves are required — from ffmpeg 8 the encoder reads color properties off the FRAMES, silently overriding the container flags, so a flags-only output decodes washed-out. `buildTrimConcatArgs` takes the filter as its `colorTagFilter` option rather than probing, to stay pure. | diff --git a/server/lib/detachedSpawn.js b/server/lib/detachedSpawn.js index f0bf0c9d36..43a347276d 100644 --- a/server/lib/detachedSpawn.js +++ b/server/lib/detachedSpawn.js @@ -48,6 +48,7 @@ import { EventEmitter } from 'events'; import { constants as osConstants } from 'os'; import { join } from 'path'; import { open, readFile, writeFile, rm, stat, readdir } from 'fs/promises'; +import { killProcessTree } from './bufferedSpawn.js'; import { ensureDir, sleep } from './fileUtils.js'; import { withSpawnCwdEnv } from './spawnCwd.js'; @@ -230,6 +231,7 @@ function createLogTailer(handle, { controlDir, pollMs, cleanup }) { * @param {boolean} [opts.killProcessGroup] - signal `-pid` on cancel/reap so a * group-leader wrapper and every runtime child terminate together. The job is * responsible for establishing its own process group before spawning children. + * POSIX-only — the win32 fallback's `kill` always tree-kills, group or not. * @returns {Promise} ChildProcess-like handle (resolves once the PID is known) */ export async function spawnDetached(bin, args = [], { @@ -244,7 +246,27 @@ export async function spawnDetached(bin, args = [], { // exitCode / signalCode), so callers are unaffected. Surviving a pm2 restart // is a POSIX-only guarantee; Windows keeps its prior spawn semantics. if (process.platform === 'win32') { - return spawn(bin, args, { env: withSpawnCwdEnv(env ?? process.env, cwd), cwd, stdio: ['ignore', 'pipe', 'pipe'] }); + const child = spawn(bin, args, { env: withSpawnCwdEnv(env ?? process.env, cwd), cwd, stdio: ['ignore', 'pipe', 'pipe'] }); + // A bare `child.kill()` terminates ONLY the runner. Windows has no process + // group for the POSIX `-pid` trick `killProcessGroup` relies on, so whatever + // the runner spawned (the ffmpeg mux a CUDA video runtime shells out to, a + // model download) survives as an orphan still holding the output file and + // GPU memory. Delegate to the shared tree-killer instead — `taskkill /T /F` + // on Windows, the POSIX group signal elsewhere (#4171). Note `taskkill /T /F` + // ignores the requested signal and force-kills, so a SIGTERM cancel is not + // graceful here; that is killProcessTree's documented Windows contract. + const nativeKill = child.kill.bind(child); + // What we hand killProcessTree: inherits from `child` (so `pid` reads + // through and `instanceof ChildProcess` still holds — the taskkill branch is + // gated on it) but exposes Node's own kill, so killProcessTree's POSIX + // fall-through can never re-enter the override below. + const treeKillTarget = Object.create(child, { kill: { value: nativeKill } }); + child.kill = (signal = 'SIGTERM') => { + child.killed = true; + killProcessTree(treeKillTarget, signal, { processGroup: true }); + return true; + }; + return child; } const handle = new EventEmitter(); diff --git a/server/lib/detachedSpawn.test.js b/server/lib/detachedSpawn.test.js index e9ea9ad35f..ea368f1876 100644 --- a/server/lib/detachedSpawn.test.js +++ b/server/lib/detachedSpawn.test.js @@ -1,12 +1,21 @@ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect, afterEach, vi } from 'vitest'; import { mkdir, mkdtemp, rm, stat, writeFile } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; -import { execFile } from 'child_process'; +import { ChildProcess, execFile } from 'child_process'; import { promisify } from 'util'; import { pinPlatform } from './testHelper.js'; +import { killProcessTree } from './bufferedSpawn.js'; import { spawnDetached, reapDetached, reapAndCleanDetachedDirs, reattachDetached, isReattachable, isDetachedRunning } from './detachedSpawn.js'; +// Only the win32 fallback's kill() reaches killProcessTree, so stubbing it is +// inert for every POSIX test here — and it lets the win32 test assert the +// delegation on a platform where `taskkill` doesn't exist. +vi.mock('./bufferedSpawn.js', async (importOriginal) => ({ + ...(await importOriginal()), + killProcessTree: vi.fn(), +})); + const execFileAsync = promisify(execFile); // spawnDetached's POSIX `sh` double-fork — and with it the whole control-dir @@ -219,6 +228,44 @@ describe('spawnDetached', () => { } }); + it('win32 fallback kill() tree-kills so the runner\'s children die with it', async () => { + const restorePlatform = pinPlatform('win32'); + try { + killProcessTree.mockClear(); + const controlDir = await tmpControlDir(); + const handle = await spawnDetached('sh', ['-c', 'sleep 30'], { controlDir }); + const closed = onClose(handle); + expect(handle.kill('SIGKILL')).toBe(true); + expect(handle.killed).toBe(true); + expect(killProcessTree).toHaveBeenCalledTimes(1); + const [target, signal, opts] = killProcessTree.mock.calls[0]; + expect(signal).toBe('SIGKILL'); + expect(opts).toEqual({ processGroup: true }); + // The target must still be a real ChildProcess — killProcessTree's + // `taskkill /T /F` branch is gated on `instanceof ChildProcess` — and must + // carry Node's own kill, not the override, so its POSIX fall-through + // can't recurse back into it. + expect(target).toBeInstanceOf(ChildProcess); + expect(target.pid).toBe(handle.pid); + expect(target.kill).not.toBe(handle.kill); + // killProcessTree is stubbed, so nothing actually died — reap the sleeper. + target.kill('SIGKILL'); + await closed; + } finally { + restorePlatform(); + } + }); + + it.runIf(IS_POSIX)('leaves the POSIX path on its own pid/group signalling (no tree-kill)', async () => { + killProcessTree.mockClear(); + const controlDir = await tmpControlDir(); + const handle = await spawnDetached('sh', ['-c', 'sleep 30'], { controlDir, pollMs: 25 }); + await new Promise((r) => setTimeout(r, 50)); + expect(handle.kill('SIGKILL')).toBe(true); + expect(killProcessTree).not.toHaveBeenCalled(); + await onClose(handle); + }); + describe('reapDetached', () => { it.runIf(IS_POSIX)('SIGTERMs a surviving orphan and reports it reaped', async () => { const controlDir = await tmpControlDir(); From 6a04a18a0ba1ed0bbe48d3372acb1720f344b702 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:10:44 +0000 Subject: [PATCH 2/5] address review (claude): re-stamp the kill signal on the win32 handle's terminal events taskkill terminates the tree out of band, so libuv records no exit_signal and the child reports close(1, null) where Node's own kill reported close(null, 'SIGKILL'). videoGen's isWatchdogSuccess keeps a finished .mp4 only when signal === 'SIGKILL', so a completion/idle-stall kill on Windows would have discarded the render as 'Exit code 1'. A concurrent clean exit (code 0) is left alone. --- server/lib/detachedSpawn.js | 22 ++++++++++- server/lib/detachedSpawn.test.js | 64 ++++++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/server/lib/detachedSpawn.js b/server/lib/detachedSpawn.js index 43a347276d..128c5ae269 100644 --- a/server/lib/detachedSpawn.js +++ b/server/lib/detachedSpawn.js @@ -244,7 +244,8 @@ export async function spawnDetached(bin, args = [], { // back to a normal child process: a real ChildProcess already satisfies the // handle contract (pid / stdout / stderr / on('close',code,signal) / kill / // exitCode / signalCode), so callers are unaffected. Surviving a pm2 restart - // is a POSIX-only guarantee; Windows keeps its prior spawn semantics. + // is a POSIX-only guarantee; Windows keeps its prior spawn semantics apart + // from `kill`, which is replaced with a tree-kill below. if (process.platform === 'win32') { const child = spawn(bin, args, { env: withSpawnCwdEnv(env ?? process.env, cwd), cwd, stdio: ['ignore', 'pipe', 'pipe'] }); // A bare `child.kill()` terminates ONLY the runner. Windows has no process @@ -261,7 +262,26 @@ export async function spawnDetached(bin, args = [], { // gated on it) but exposes Node's own kill, so killProcessTree's POSIX // fall-through can never re-enter the override below. const treeKillTarget = Object.create(child, { kill: { value: nativeKill } }); + // `taskkill` terminates the tree OUT OF BAND, so libuv never records an + // exit_signal and the child reports `close(1, null)` where Node's own + // `kill()` reported `close(null, 'SIGKILL')`. Callers classify on exactly + // that signal — videoGen's watchdog-success test keeps a finished .mp4 only + // when `signal === 'SIGKILL'`, and `describeSignalDeath` reads it for the + // failure reason — so re-stamp the signal we asked for onto the terminal + // events, matching both a native kill and the POSIX handle's decoded close. + // A concurrent clean exit (code 0) is left alone: the job really did finish. + let killSignal = null; + const nativeEmit = child.emit.bind(child); + child.emit = (event, ...rest) => { + if (killSignal && (event === 'close' || event === 'exit') && rest[1] == null && rest[0] !== 0) { + child.exitCode = null; + child.signalCode = killSignal; + return nativeEmit(event, null, killSignal); + } + return nativeEmit(event, ...rest); + }; child.kill = (signal = 'SIGTERM') => { + killSignal = signal; child.killed = true; killProcessTree(treeKillTarget, signal, { processGroup: true }); return true; diff --git a/server/lib/detachedSpawn.test.js b/server/lib/detachedSpawn.test.js index ea368f1876..2b4a65b9f9 100644 --- a/server/lib/detachedSpawn.test.js +++ b/server/lib/detachedSpawn.test.js @@ -228,12 +228,25 @@ describe('spawnDetached', () => { } }); + // A child that runs until a marker file appears, then exits with a code and + // NO signal — the shape `taskkill /T /F` produces on Windows, where the kill + // happens out of band so libuv records no exit_signal. + const spawnWin32Fallback = async () => { + const controlDir = await tmpControlDir(); + const marker = join(controlDir, 'go'); + const handle = await spawnDetached( + 'sh', + ['-c', 'while [ ! -f "$1" ]; do sleep 0.05; done; exit 1', 'sh', marker], + { controlDir } + ); + return { handle, terminate: () => writeFile(marker, '1') }; + }; + it('win32 fallback kill() tree-kills so the runner\'s children die with it', async () => { const restorePlatform = pinPlatform('win32'); try { killProcessTree.mockClear(); - const controlDir = await tmpControlDir(); - const handle = await spawnDetached('sh', ['-c', 'sleep 30'], { controlDir }); + const { handle, terminate } = await spawnWin32Fallback(); const closed = onClose(handle); expect(handle.kill('SIGKILL')).toBe(true); expect(handle.killed).toBe(true); @@ -248,14 +261,57 @@ describe('spawnDetached', () => { expect(target).toBeInstanceOf(ChildProcess); expect(target.pid).toBe(handle.pid); expect(target.kill).not.toBe(handle.kill); - // killProcessTree is stubbed, so nothing actually died — reap the sleeper. - target.kill('SIGKILL'); + await terminate(); await closed; } finally { restorePlatform(); } }); + it('win32 fallback reports the requested signal on close (taskkill kills out of band)', async () => { + const restorePlatform = pinPlatform('win32'); + try { + killProcessTree.mockClear(); + const { handle, terminate } = await spawnWin32Fallback(); + const closed = onClose(handle); + handle.kill('SIGKILL'); + // The stubbed tree-kill didn't terminate anything; let the child exit the + // way a taskkill'd one does — a plain non-zero code, no signal. + await terminate(); + const { code, signal } = await closed; + // Without the re-stamp this is (1, null) and videoGen discards a finished + // render as "Exit code 1" instead of honoring the watchdog kill. + expect(code).toBeNull(); + expect(signal).toBe('SIGKILL'); + expect(handle.exitCode).toBeNull(); + expect(handle.signalCode).toBe('SIGKILL'); + } finally { + restorePlatform(); + } + }); + + it('win32 fallback leaves a clean exit alone when a cancel races completion', async () => { + const restorePlatform = pinPlatform('win32'); + try { + killProcessTree.mockClear(); + const controlDir = await tmpControlDir(); + const marker = join(controlDir, 'go'); + const handle = await spawnDetached( + 'sh', + ['-c', 'while [ ! -f "$1" ]; do sleep 0.05; done; exit 0', 'sh', marker], + { controlDir } + ); + const closed = onClose(handle); + handle.kill('SIGTERM'); + await writeFile(marker, '1'); + const { code, signal } = await closed; + expect(code).toBe(0); + expect(signal).toBeNull(); + } finally { + restorePlatform(); + } + }); + it.runIf(IS_POSIX)('leaves the POSIX path on its own pid/group signalling (no tree-kill)', async () => { killProcessTree.mockClear(); const controlDir = await tmpControlDir(); From ffa42fc89b5e9f9f0eecc48ef0e5de6007f735ad Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:35:41 +0000 Subject: [PATCH 3/5] address review (codex): never taskkill an exited win32 child, and keep signal 0 a probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit taskkill only gets a pid, and Windows recycles pids — a late escalation against an already-exited child could tree-kill whatever inherited the number. Refuse the kill once the child reports a terminal code/signal, and delegate signal 0 (an existence probe) to Node's own kill instead of force-killing the tree. --- server/lib/detachedSpawn.js | 15 ++++++++++++++ server/lib/detachedSpawn.test.js | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/server/lib/detachedSpawn.js b/server/lib/detachedSpawn.js index 128c5ae269..0dd9f511f4 100644 --- a/server/lib/detachedSpawn.js +++ b/server/lib/detachedSpawn.js @@ -270,6 +270,12 @@ export async function spawnDetached(bin, args = [], { // failure reason — so re-stamp the signal we asked for onto the terminal // events, matching both a native kill and the POSIX handle's decoded close. // A concurrent clean exit (code 0) is left alone: the job really did finish. + // The stamp records the signal we ASKED for without waiting to confirm the + // tree died from it — exactly what Node's own `kill()` does (libuv stamps + // exit_signal at request time), and the guard above already refuses to fire + // at an exited child, so the only ambiguity left is a nonzero exit racing + // our kill by microseconds. Both readings mean "cancelled", so no async + // taskkill-completion plumbing is warranted here. let killSignal = null; const nativeEmit = child.emit.bind(child); child.emit = (event, ...rest) => { @@ -281,6 +287,15 @@ export async function spawnDetached(bin, args = [], { return nativeEmit(event, ...rest); }; child.kill = (signal = 'SIGTERM') => { + // `taskkill /T /F` is destructive and Windows recycles PIDs freely, so it + // must never fire at a child that already exited — a late escalation + // (killWithEscalation's 8s SIGKILL) would tree-kill whatever inherited the + // number. Node's own kill is safe there because it holds a process HANDLE, + // not a pid; taskkill only gets the pid. + if (!child.pid || child.exitCode !== null || child.signalCode !== null) return false; + // Signal 0 is an existence PROBE, not a kill — hand it to Node so callers + // keep that meaning instead of force-killing the tree. + if (signal === 0 || signal === '0') return nativeKill(signal); killSignal = signal; child.killed = true; killProcessTree(treeKillTarget, signal, { processGroup: true }); diff --git a/server/lib/detachedSpawn.test.js b/server/lib/detachedSpawn.test.js index 2b4a65b9f9..b85d76e016 100644 --- a/server/lib/detachedSpawn.test.js +++ b/server/lib/detachedSpawn.test.js @@ -312,6 +312,40 @@ describe('spawnDetached', () => { } }); + it('win32 fallback refuses to tree-kill a child that already exited', async () => { + const restorePlatform = pinPlatform('win32'); + try { + const { handle, terminate } = await spawnWin32Fallback(); + const closed = onClose(handle); + await terminate(); + await closed; + // Windows recycles PIDs, so a late escalation must not taskkill whatever + // inherited the number. + killProcessTree.mockClear(); + expect(handle.kill('SIGKILL')).toBe(false); + expect(killProcessTree).not.toHaveBeenCalled(); + } finally { + restorePlatform(); + } + }); + + it('win32 fallback treats signal 0 as an existence probe, not a kill', async () => { + const restorePlatform = pinPlatform('win32'); + try { + killProcessTree.mockClear(); + const { handle, terminate } = await spawnWin32Fallback(); + const closed = onClose(handle); + // Node's own kill(0) answers the probe (and sets `killed`, as it always + // has); what matters is that no taskkill went out. + expect(handle.kill(0)).toBe(true); + expect(killProcessTree).not.toHaveBeenCalled(); + await terminate(); + await closed; + } finally { + restorePlatform(); + } + }); + it.runIf(IS_POSIX)('leaves the POSIX path on its own pid/group signalling (no tree-kill)', async () => { killProcessTree.mockClear(); const controlDir = await tmpControlDir(); From a32f629f8e92b039ed459adeda3d4a7381eff0ee Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:49:03 +0000 Subject: [PATCH 4/5] address review (codex): normalize a numeric kill signal to its name before stamping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kill() accepts a signal number, but ChildProcess reports signal NAMES on close — stamping a raw 9 would break every `signal === 'SIGKILL'` comparison downstream. Decode through the module's existing SIGNAL_BY_NUMBER table; an unrecognized number stamps nothing. --- server/lib/detachedSpawn.js | 7 ++++++- server/lib/detachedSpawn.test.js | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/server/lib/detachedSpawn.js b/server/lib/detachedSpawn.js index 0dd9f511f4..56f8c4cbcf 100644 --- a/server/lib/detachedSpawn.js +++ b/server/lib/detachedSpawn.js @@ -296,7 +296,12 @@ export async function spawnDetached(bin, args = [], { // Signal 0 is an existence PROBE, not a kill — hand it to Node so callers // keep that meaning instead of force-killing the tree. if (signal === 0 || signal === '0') return nativeKill(signal); - killSignal = signal; + // `kill()` also accepts a signal NUMBER, but ChildProcess reports signal + // NAMES on close — stamping the raw number would break every + // `signal === 'SIGKILL'` comparison. Reuse the same inverted table the + // POSIX handle decodes `wait` statuses with; an unrecognized number + // stamps nothing and leaves Node's own reporting alone. + killSignal = typeof signal === 'number' ? (SIGNAL_BY_NUMBER[signal] ?? null) : signal; child.killed = true; killProcessTree(treeKillTarget, signal, { processGroup: true }); return true; diff --git a/server/lib/detachedSpawn.test.js b/server/lib/detachedSpawn.test.js index b85d76e016..0467e10123 100644 --- a/server/lib/detachedSpawn.test.js +++ b/server/lib/detachedSpawn.test.js @@ -312,6 +312,24 @@ describe('spawnDetached', () => { } }); + it('win32 fallback stamps a numeric signal as its NAME on close', async () => { + const restorePlatform = pinPlatform('win32'); + try { + const { handle, terminate } = await spawnWin32Fallback(); + const closed = onClose(handle); + // kill() accepts a number; ChildProcess reports names, so a raw 9 would + // break every `signal === 'SIGKILL'` comparison downstream. + handle.kill(9); + await terminate(); + const { code, signal } = await closed; + expect(code).toBeNull(); + expect(signal).toBe('SIGKILL'); + expect(handle.signalCode).toBe('SIGKILL'); + } finally { + restorePlatform(); + } + }); + it('win32 fallback refuses to tree-kill a child that already exited', async () => { const restorePlatform = pinPlatform('win32'); try { From 3e86fded10ff5c1ac6c425008eae455e047f46f4 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:54:19 +0000 Subject: [PATCH 5/5] address review (codex): validate the kill signal and drop the POSIX shell from the win32 tests - reject an unknown signal through Node's own kill (ERR_UNKNOWN_SIGNAL) instead of silently force-killing the whole tree - the win32 tests are not IS_POSIX-gated, so drive their child with node -e rather than sh -c, which a real Windows checkout may not have - attach the close listener before killing in the POSIX no-tree-kill test --- server/lib/detachedSpawn.js | 17 +++++++------ server/lib/detachedSpawn.test.js | 43 ++++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/server/lib/detachedSpawn.js b/server/lib/detachedSpawn.js index 56f8c4cbcf..3b52c5a974 100644 --- a/server/lib/detachedSpawn.js +++ b/server/lib/detachedSpawn.js @@ -296,14 +296,17 @@ export async function spawnDetached(bin, args = [], { // Signal 0 is an existence PROBE, not a kill — hand it to Node so callers // keep that meaning instead of force-killing the tree. if (signal === 0 || signal === '0') return nativeKill(signal); - // `kill()` also accepts a signal NUMBER, but ChildProcess reports signal - // NAMES on close — stamping the raw number would break every - // `signal === 'SIGKILL'` comparison. Reuse the same inverted table the - // POSIX handle decodes `wait` statuses with; an unrecognized number - // stamps nothing and leaves Node's own reporting alone. - killSignal = typeof signal === 'number' ? (SIGNAL_BY_NUMBER[signal] ?? null) : signal; + // Decode and validate the signal exactly as ChildProcess.kill() does: + // `kill()` also accepts a NUMBER while `close` reports NAMES (stamping a + // raw 9 would break every `signal === 'SIGKILL'` comparison), and an + // unknown signal must still throw ERR_UNKNOWN_SIGNAL rather than silently + // force-killing the tree. SIGNAL_BY_NUMBER is the same inverted table the + // POSIX handle decodes `wait` statuses with. + const signalName = typeof signal === 'number' ? SIGNAL_BY_NUMBER[signal] : signal; + if (!signalName || !(signalName in osConstants.signals)) return nativeKill(signal); + killSignal = signalName; child.killed = true; - killProcessTree(treeKillTarget, signal, { processGroup: true }); + killProcessTree(treeKillTarget, signalName, { processGroup: true }); return true; }; return child; diff --git a/server/lib/detachedSpawn.test.js b/server/lib/detachedSpawn.test.js index 0467e10123..d8b8f7d837 100644 --- a/server/lib/detachedSpawn.test.js +++ b/server/lib/detachedSpawn.test.js @@ -230,13 +230,16 @@ describe('spawnDetached', () => { // A child that runs until a marker file appears, then exits with a code and // NO signal — the shape `taskkill /T /F` produces on Windows, where the kill - // happens out of band so libuv records no exit_signal. - const spawnWin32Fallback = async () => { + // happens out of band so libuv records no exit_signal. Driven by `node -e` + // rather than `sh -c` because these tests are NOT gated on IS_POSIX: they + // pin the platform and must run on a real Windows checkout, which has no + // guaranteed POSIX shell. + const spawnWin32Fallback = async (exitCode = 1) => { const controlDir = await tmpControlDir(); const marker = join(controlDir, 'go'); const handle = await spawnDetached( - 'sh', - ['-c', 'while [ ! -f "$1" ]; do sleep 0.05; done; exit 1', 'sh', marker], + process.execPath, + ['-e', `const {existsSync}=require('fs');const t=setInterval(()=>{if(existsSync(process.argv[1])){clearInterval(t);process.exit(${exitCode});}},25);`, marker], { controlDir } ); return { handle, terminate: () => writeFile(marker, '1') }; @@ -294,16 +297,10 @@ describe('spawnDetached', () => { const restorePlatform = pinPlatform('win32'); try { killProcessTree.mockClear(); - const controlDir = await tmpControlDir(); - const marker = join(controlDir, 'go'); - const handle = await spawnDetached( - 'sh', - ['-c', 'while [ ! -f "$1" ]; do sleep 0.05; done; exit 0', 'sh', marker], - { controlDir } - ); + const { handle, terminate } = await spawnWin32Fallback(0); const closed = onClose(handle); handle.kill('SIGTERM'); - await writeFile(marker, '1'); + await terminate(); const { code, signal } = await closed; expect(code).toBe(0); expect(signal).toBeNull(); @@ -364,14 +361,34 @@ describe('spawnDetached', () => { } }); + it('win32 fallback rejects an unknown signal instead of force-killing the tree', async () => { + const restorePlatform = pinPlatform('win32'); + try { + killProcessTree.mockClear(); + const { handle, terminate } = await spawnWin32Fallback(); + const closed = onClose(handle); + // ChildProcess.kill() throws ERR_UNKNOWN_SIGNAL on a typo'd name; the + // override must not turn that into a silent whole-tree force-kill. + expect(() => handle.kill('SIGKLL')).toThrow(); + expect(killProcessTree).not.toHaveBeenCalled(); + await terminate(); + await closed; + } finally { + restorePlatform(); + } + }); + it.runIf(IS_POSIX)('leaves the POSIX path on its own pid/group signalling (no tree-kill)', async () => { killProcessTree.mockClear(); const controlDir = await tmpControlDir(); const handle = await spawnDetached('sh', ['-c', 'sleep 30'], { controlDir, pollMs: 25 }); + // Attach the close listener BEFORE killing — the tail loop can fire 'close' + // as soon as the signal lands. + const closed = onClose(handle); await new Promise((r) => setTimeout(r, 50)); expect(handle.kill('SIGKILL')).toBe(true); expect(killProcessTree).not.toHaveBeenCalled(); - await onClose(handle); + await closed; }); describe('reapDetached', () => {