Skip to content

Commit 8a6ea61

Browse files
zahidzorbazclaude
authored andcommitted
fix(hub): kill the whole child-process tree on Windows
On Windows, tinyexec runs `.cmd` shims (e.g. `node_modules/.bin/vitest.cmd`) through `cmd.exe`, so `terminate()`, `restart()` and stream cancel only killed the wrapper and left the real program running and holding its ports. Kill the tree with `taskkill /T /F` there, wait for it before `restart()` spawns the next run, and keep reporting host-initiated kills as `stopped` with an `undefined` exit code on every platform. Fixes #402 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 18fa60e commit 8a6ea61

3 files changed

Lines changed: 161 additions & 12 deletions

File tree

‎packages/hub/src/node/__tests__/host-terminals.test.ts‎

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import type { DevframeTerminalSession } from '../../types/terminals'
22
import type { DevframeHubContext } from '../context'
3+
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
4+
import { tmpdir } from 'node:os'
5+
import { join } from 'node:path'
36
import process from 'node:process'
47
import { describe, expect, it, vi } from 'vitest'
58
import { hasNative } from 'zigpty'
@@ -71,8 +74,8 @@ function createTerminalHost() {
7174
}
7275
}
7376

74-
async function waitUntil(assertion: () => void): Promise<void> {
75-
const deadline = Date.now() + 1000
77+
async function waitUntil(assertion: () => void, timeout = 1000): Promise<void> {
78+
const deadline = Date.now() + timeout
7679
let lastError: unknown
7780
while (Date.now() < deadline) {
7881
try {
@@ -397,6 +400,104 @@ describe('devframeTerminalHost child-process status lifecycle', () => {
397400
})
398401
})
399402

403+
// On Windows, tinyexec runs anything that isn't a `.exe`/`.com` (e.g. the
404+
// `node_modules/.bin/*.cmd` shims package managers generate) through
405+
// `cmd.exe /d /s /c`, so the pid the host holds belongs to `cmd.exe`, not to
406+
// the program the shim starts. Killing only that pid leaves the real process
407+
// orphaned (still holding its ports).
408+
describe.runIf(process.platform === 'win32')('devframeTerminalHost child-process tree on Windows', { timeout: 20_000 }, () => {
409+
function isAlive(pid: number): boolean {
410+
try {
411+
process.kill(pid, 0)
412+
return true
413+
}
414+
catch {
415+
return false
416+
}
417+
}
418+
419+
async function startShimSession(host: DevframeTerminalsHost, id: string) {
420+
const dir = mkdtempSync(join(tmpdir(), 'devframe-terminals-shim-'))
421+
writeFileSync(join(dir, 'child.cjs'), 'console.log("pid:" + process.pid); setInterval(() => {}, 1000)\n')
422+
const shim = join(dir, 'child.cmd')
423+
writeFileSync(shim, `@"${NODE}" "%~dp0\\child.cjs" %*\r\n`)
424+
const session = await host.startChildProcess({ command: shim, args: [], cwd: dir }, { id, title: id })
425+
let pid = 0
426+
await waitUntil(() => {
427+
const match = session.buffer?.join('').match(/pid:(\d+)/)
428+
expect(match).toBeTruthy()
429+
pid = Number(match![1])
430+
}, 10_000)
431+
// The host holds the `cmd.exe` wrapper, not the node child.
432+
expect(session.getChildProcess()?.pid).not.toBe(pid)
433+
return { session, pid, cleanup: () => rmSync(dir, { recursive: true, force: true }) }
434+
}
435+
436+
it('terminate() kills the process started by a .cmd shim', async () => {
437+
const { host } = createTerminalHost()
438+
const { session, pid, cleanup } = await startShimSession(host, 'shim-terminate')
439+
try {
440+
await session.terminate()
441+
await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
442+
expect(session.status).toBe('stopped')
443+
}
444+
finally {
445+
if (isAlive(pid))
446+
process.kill(pid)
447+
cleanup()
448+
}
449+
})
450+
451+
it('terminate() reports a stopped, killed run rather than a crash', async () => {
452+
const { host, sinks } = createTerminalHost()
453+
const { session, pid, cleanup } = await startShimSession(host, 'shim-result')
454+
const result = session.getResult()
455+
try {
456+
await session.terminate()
457+
await waitUntil(() => expect(sinks.get('shim-result')?.closed).toBe(true))
458+
const output = await result
459+
expect(output.exitCode).toBeUndefined()
460+
expect(result.killed).toBe(true)
461+
expect(session.status).toBe('stopped')
462+
}
463+
finally {
464+
if (isAlive(pid))
465+
process.kill(pid)
466+
cleanup()
467+
}
468+
})
469+
470+
it('restart() kills the previous run started by a .cmd shim', async () => {
471+
const { host } = createTerminalHost()
472+
const { session, pid, cleanup } = await startShimSession(host, 'shim-restart')
473+
try {
474+
await session.restart()
475+
await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
476+
expect(session.status).toBe('running')
477+
await session.terminate()
478+
}
479+
finally {
480+
if (isAlive(pid))
481+
process.kill(pid)
482+
cleanup()
483+
}
484+
})
485+
486+
it('cancelling the stream kills the process started by a .cmd shim', async () => {
487+
const { host } = createTerminalHost()
488+
const { session, pid, cleanup } = await startShimSession(host, 'shim-cancel')
489+
try {
490+
host.remove(session)
491+
await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
492+
}
493+
finally {
494+
if (isAlive(pid))
495+
process.kill(pid)
496+
cleanup()
497+
}
498+
})
499+
})
500+
400501
describe('devframeTerminalHost interactive PTY sessions', () => {
401502
itPty('inherits the parent process environment', async () => {
402503
expect.assertions(1)

‎packages/hub/src/node/host-terminals.ts‎

Lines changed: 57 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type {
1616
DevframeTerminalsHost as DevframeTerminalsHostType,
1717
} from '../types/terminals'
1818
import type { DevframeHubContext } from './context'
19+
import { spawn } from 'node:child_process'
1920
import process from 'node:process'
2021
import { createEventEmitter } from 'devframe/utils/events'
2122
import { HUB_EVENTS } from '../events'
@@ -35,6 +36,39 @@ const TERMINAL_BUFFER_LIMIT = 1000
3536
/** TERM handed to spawned PTYs; also used to reject fallback process labels. */
3637
const PTY_TERM_NAME = 'xterm-256color'
3738

39+
/**
40+
* Kill a `startChildProcess()` run together with everything it spawned.
41+
*
42+
* On Windows, tinyexec runs anything that isn't a `.exe`/`.com` - including
43+
* the `node_modules/.bin/*.cmd` shims package managers generate - through
44+
* `cmd.exe /d /s /c`, so the pid held here is the wrapper's. Killing a process
45+
* on Windows doesn't reach its descendants, so `cp.kill()` alone would leave
46+
* the real program running (and holding its ports). `taskkill /T /F` ends the
47+
* whole tree; `cp.kill()` stays the fallback and the POSIX path.
48+
*/
49+
function killProcessTree(cp: TinyExecResult): Promise<void> {
50+
const child = cp.process
51+
const pid = child?.pid
52+
if (process.platform !== 'win32' || !child || pid === undefined || child.exitCode !== null || child.signalCode !== null) {
53+
cp.kill()
54+
return Promise.resolve()
55+
}
56+
return new Promise((resolve) => {
57+
let finished = false
58+
const finish = (ok: boolean) => {
59+
if (finished)
60+
return
61+
finished = true
62+
if (!ok)
63+
cp.kill()
64+
resolve()
65+
}
66+
const killer = spawn('taskkill', ['/pid', String(pid), '/T', '/F'], { stdio: 'ignore', windowsHide: true })
67+
killer.once('error', () => finish(false))
68+
killer.once('exit', code => finish(code === 0))
69+
})
70+
}
71+
3872
export class DevframeTerminalsHost implements DevframeTerminalsHostType {
3973
public readonly sessions: DevframeTerminalsHostType['sessions'] = new Map()
4074
public readonly events: DevframeTerminalsHostType['events'] = createEventEmitter()
@@ -231,15 +265,26 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
231265
let cp: TinyExecResult | undefined
232266
let currentResult: DevframeChildProcessResult | undefined
233267
let runId = 0
268+
// Runs stopped on purpose (terminate/restart/cancel). On Windows the tree
269+
// kill ends them with exit code 1 rather than a signal, so this keeps them
270+
// reported as a deliberate stop instead of a crash on every platform.
271+
const hostKilled = new WeakSet<TinyExecResult>()
272+
const killRun = (target: TinyExecResult | undefined): Promise<void> => {
273+
if (!target)
274+
return Promise.resolve()
275+
hostKilled.add(target)
276+
return killProcessTree(target)
277+
}
234278

235279
const stream = new ReadableStream<string>({
236280
start(_controller) {
237281
state.controller = _controller
238282
},
239283
cancel() {
240-
cp?.kill()
284+
const target = cp
241285
cp = undefined
242286
closeStream()
287+
return killRun(target)
243288
},
244289
})
245290

@@ -310,26 +355,27 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
310355
markStatus('error')
311356
})
312357
cp.process?.once('close', (code) => {
313-
settle(code ?? undefined)
358+
const killed = hostKilled.has(cp)
359+
settle(killed ? undefined : code ?? undefined)
314360
if (currentRun !== runId)
315361
return
316362
closeStream()
317363
// A spawn/runtime error already settled the status; a non-zero exit
318-
// code is a crash. A clean exit, or a signal kill (no numeric code,
319-
// e.g. terminate()/restart()), is a deliberate/normal stop.
364+
// code is a crash. A clean exit, or a kill by the host (terminate()/
365+
// restart()), is a deliberate/normal stop.
320366
if (!runErrored)
321-
markStatus(typeof code === 'number' && code !== 0 ? 'error' : 'stopped')
367+
markStatus(!killed && typeof code === 'number' && code !== 0 ? 'error' : 'stopped')
322368
})
323369

324370
currentResult = {
325371
get pid() {
326372
return cp.process?.pid
327373
},
328374
get exitCode() {
329-
return cp.process?.exitCode ?? undefined
375+
return hostKilled.has(cp) ? undefined : cp.process?.exitCode ?? undefined
330376
},
331377
get killed() {
332-
return cp.process?.killed === true
378+
return hostKilled.has(cp) || cp.process?.killed === true
333379
},
334380
kill: signal => cp.kill(signal),
335381
then: (onfulfilled, onrejected) => outputPromise.then(onfulfilled, onrejected),
@@ -343,13 +389,15 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
343389
const restart = async () => {
344390
if (state.streamClosed)
345391
throw diagnostics.DF8206({ id: terminal.id })
346-
cp?.kill()
392+
// Wait for the old tree to go away so the new run can reclaim its ports.
393+
await killRun(cp)
347394
cp = createChildProcess()
348395
markStatus('running')
349396
}
350397
const terminate = async () => {
351-
cp?.kill()
398+
const target = cp
352399
cp = undefined
400+
await killRun(target)
353401
closeStream()
354402
markStatus('stopped')
355403
}

‎packages/hub/src/types/terminals.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export interface DevframeChildProcessExecuteOptions {
7979
* The settled outcome of a {@link DevframeChildProcessTerminalSession} run:
8080
* stdout/stderr captured separately (unlike the session's merged display
8181
* `stream`), plus the process's exit code (`undefined` if it was killed by a
82-
* signal before exiting).
82+
* signal, or by `terminate()`/`restart()`, before exiting).
8383
*/
8484
export interface DevframeChildProcessOutput {
8585
stdout: string

0 commit comments

Comments
 (0)