fix: tree-kill a cancelled video render on Windows (#4171)

.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

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 `<verb> 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. |

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<object>} ChildProcess-like handle (resolves once the PID is known)
  */
 export async function spawnDetached(bin, args = [], {
@@ -242,9 +244,72 @@ 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') {
-    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 } });
+    // `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.
+    // 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) => {
+      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') => {
+      // `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);
+      // 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, signalName, { processGroup: true });
+      return true;
+    };
+    return child;
   }
 
   const handle = new EventEmitter();