fix: tree-kill a cancelled video render on Windows (#4171) - #4267
Merged
Conversation
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.
…'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.
…p signal 0 a probe 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.
…efore stamping 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.
…hell 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spawnDetached()(server/lib/detachedSpawn.js) has no POSIXshfor its double-fork on Windows, so that branch fell back to a plainspawn()and returned the bareChildProcess. The cancel and watchdog paths inserver/services/videoGen/local.jsthen callproc.kill('SIGKILL'), which terminates only the python runner — whatever the runner spawned (the ffmpeg mux a CUDA video runtime shells out to, a model download) survived as an orphan still holding its output file and, mid-render, GPU memory.killProcessGroupcould not help:signalPid()implements it as a POSIX-pidsignal, so the two runtimes that set it and run on Windows got nothing from it.The win32 fallback now installs its own
killthat delegates tokillProcessTree(child, signal, { processGroup: true })(server/lib/bufferedSpawn.js) —taskkill /T /Fon Windows, the POSIX group signal elsewhere. That is a delegation to the existing shared helper, not a new mechanism.The POSIX
spawnDetachedpath is deliberately not rerouted through it:signalPidalso serves reattached/reaped runs by raw pid, whichkillProcessTree(ChildProcess-only) does not.killProcessTreeis handed an object that inherits from the child (sopidreads through and itsinstanceof ChildProcesstaskkill gate still holds) but carries Node's ownkill, so the helper's POSIX fall-through can never re-enter the override.child.killedis set by the override so the.killed-gated re-entrancy guards invideoGen/local.js(completion watchdog, idle-stall watchdog) engage on Windows too.taskkillterminates the tree out of band, so libuv records noexit_signaland the child would reportclose(1, null)where Node's ownkill()reportedclose(null, 'SIGKILL'). Callers classify on exactly that signal —isWatchdogSuccess()keeps a finished.mp4only whensignal === 'SIGKILL', anddescribeSignalDeath()reads it for the failure reason — so the handle re-stamps the requested signal onto its terminal events, matching both a native kill and the POSIX handle's decoded close. A concurrent clean exit (code 0) is left alone.The override keeps the rest of the
ChildProcess.kill()contract: it refuses to fire at a child that already reported a terminal code/signal (taskkillonly gets a pid, and Windows recycles pids, so a late escalation could tree-kill whatever inherited the number), treats signal0as an existence probe, decodes a numeric signal to its name before stamping, and delegates an unknown signal to Node so it still throwsERR_UNKNOWN_SIGNAL.Test plan
server/lib/detachedSpawn.test.js— new test asserts the win32 handle'skill('SIGKILL')callskillProcessTreeexactly once with{ processGroup: true }, that the target is still a realChildProcesscarrying the child's pid, and that it does not carry the override (no recursion). Verified it fails on the pre-fix source (expected "vi.fn()" to be called 1 times, but got 0 times).taskkill /T /Fproduces — and assertclosereports(null, 'SIGKILL'); and that a cleanexit 0racing a cancel is still reported as(0, null). Verified the signal test fails without the re-stamp (expected 1 to be null).killProcessTree, alongside the pre-existing reparent/kill/reattach/reap suite that covers survive-a-pm2-restart behavior.0probe, a numerickill(9)reported as'SIGKILL', and an unknown signal throwing rather than force-killing the tree. These tests are not platform-gated, so their child is driven bynode -e(no POSIX shell assumed).process.platform.cd server && npm test— 1339 files / 27772 tests pass. 56 files fail identically on a cleanmainin this environment (no local PostgreSQL:Pipeline series require PostgreSQL, plus 4 unrelatedhealth/updateExecutorassertions); none touchdetachedSpawn,bufferedSpawn, orvideoGen.server/lib/index.test.js(barrel + README catalog guard) passes with the updateddetachedSpawn.jsREADME row.Closes #4171