fix(hub): kill the whole child-process tree on Windows (#407)

Co-authored-by: zahid emre zorbaz <zahid.zorbaz@live.com>

Author: antfubot

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

@@ -1,5 +1,8 @@
 import type { DevframeTerminalSession } from '../../types/terminals'
 import type { DevframeHubContext } from '../context'
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
 import process from 'node:process'
 import { describe, expect, it, vi } from 'vitest'
 import { hasNative } from 'zigpty'
@@ -71,8 +74,8 @@ function createTerminalHost() {
   }
 }
 
-async function waitUntil(assertion: () => void): Promise<void> {
-  const deadline = Date.now() + 1000
+async function waitUntil(assertion: () => void, timeout = 1000): Promise<void> {
+  const deadline = Date.now() + timeout
   let lastError: unknown
   while (Date.now() < deadline) {
     try {
@@ -397,6 +400,104 @@ describe('devframeTerminalHost child-process status lifecycle', () => {
   })
 })
 
+// On Windows, tinyexec runs anything that isn't a `.exe`/`.com` (e.g. the
+// `node_modules/.bin/*.cmd` shims package managers generate) through
+// `cmd.exe /d /s /c`, so the pid the host holds belongs to `cmd.exe`, not to
+// the program the shim starts. Killing only that pid leaves the real process
+// orphaned (still holding its ports).
+describe.runIf(process.platform === 'win32')('devframeTerminalHost child-process tree on Windows', { timeout: 20_000 }, () => {
+  function isAlive(pid: number): boolean {
+    try {
+      process.kill(pid, 0)
+      return true
+    }
+    catch {
+      return false
+    }
+  }
+
+  async function startShimSession(host: DevframeTerminalsHost, id: string) {
+    const dir = mkdtempSync(join(tmpdir(), 'devframe-terminals-shim-'))
+    writeFileSync(join(dir, 'child.cjs'), 'console.log("pid:" + process.pid); setInterval(() => {}, 1000)\n')
+    const shim = join(dir, 'child.cmd')
+    writeFileSync(shim, `@"${NODE}" "%~dp0\\child.cjs" %*\r\n`)
+    const session = await host.startChildProcess({ command: shim, args: [], cwd: dir }, { id, title: id })
+    let pid = 0
+    await waitUntil(() => {
+      const match = session.buffer?.join('').match(/pid:(\d+)/)
+      expect(match).toBeTruthy()
+      pid = Number(match![1])
+    }, 10_000)
+    // The host holds the `cmd.exe` wrapper, not the node child.
+    expect(session.getChildProcess()?.pid).not.toBe(pid)
+    return { session, pid, cleanup: () => rmSync(dir, { recursive: true, force: true }) }
+  }
+
+  it('terminate() kills the process started by a .cmd shim', async () => {
+    const { host } = createTerminalHost()
+    const { session, pid, cleanup } = await startShimSession(host, 'shim-terminate')
+    try {
+      await session.terminate()
+      await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
+      expect(session.status).toBe('stopped')
+    }
+    finally {
+      if (isAlive(pid))
+        process.kill(pid)
+      cleanup()
+    }
+  })
+
+  it('terminate() reports a stopped, killed run rather than a crash', async () => {
+    const { host, sinks } = createTerminalHost()
+    const { session, pid, cleanup } = await startShimSession(host, 'shim-result')
+    const result = session.getResult()
+    try {
+      await session.terminate()
+      await waitUntil(() => expect(sinks.get('shim-result')?.closed).toBe(true))
+      const output = await result
+      expect(output.exitCode).toBeUndefined()
+      expect(result.killed).toBe(true)
+      expect(session.status).toBe('stopped')
+    }
+    finally {
+      if (isAlive(pid))
+        process.kill(pid)
+      cleanup()
+    }
+  })
+
+  it('restart() kills the previous run started by a .cmd shim', async () => {
+    const { host } = createTerminalHost()
+    const { session, pid, cleanup } = await startShimSession(host, 'shim-restart')
+    try {
+      await session.restart()
+      await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
+      expect(session.status).toBe('running')
+      await session.terminate()
+    }
+    finally {
+      if (isAlive(pid))
+        process.kill(pid)
+      cleanup()
+    }
+  })
+
+  it('cancelling the stream kills the process started by a .cmd shim', async () => {
+    const { host } = createTerminalHost()
+    const { session, pid, cleanup } = await startShimSession(host, 'shim-cancel')
+    try {
+      host.remove(session)
+      await waitUntil(() => expect(isAlive(pid)).toBe(false), 5000)
+    }
+    finally {
+      if (isAlive(pid))
+        process.kill(pid)
+      cleanup()
+    }
+  })
+})
+
 describe('devframeTerminalHost interactive PTY sessions', () => {
   itPty('inherits the parent process environment', async () => {
     expect.assertions(1)

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

@@ -35,6 +35,33 @@ const TERMINAL_BUFFER_LIMIT = 1000
 /** TERM handed to spawned PTYs; also used to reject fallback process labels. */
 const PTY_TERM_NAME = 'xterm-256color'
 
+/**
+ * Kill a `startChildProcess()` run together with everything it spawned.
+ *
+ * On Windows, tinyexec runs anything that isn't a `.exe`/`.com` - including
+ * the `node_modules/.bin/*.cmd` shims package managers generate - through
+ * `cmd.exe /d /s /c`, so the pid held here is the wrapper's. Killing a process
+ * on Windows doesn't reach its descendants, so `cp.kill()` alone would leave
+ * the real program running (and holding its ports). `taskkill /T /F` ends the
+ * whole tree; `cp.kill()` stays the fallback and the POSIX path.
+ */
+async function killProcessTree(cp: TinyExecResult): Promise<void> {
+  const child = cp.process
+  const pid = child?.pid
+  if (process.platform !== 'win32' || !child || pid === undefined || child.exitCode !== null || child.signalCode !== null) {
+    cp.kill()
+    return
+  }
+  const { exec } = await import('tinyexec')
+  try {
+    const { exitCode } = await exec('taskkill', ['/pid', String(pid), '/T', '/F'])
+    if (exitCode === 0)
+      return
+  }
+  catch {}
+  cp.kill()
+}
+
 export class DevframeTerminalsHost implements DevframeTerminalsHostType {
   public readonly sessions: DevframeTerminalsHostType['sessions'] = new Map()
   public readonly events: DevframeTerminalsHostType['events'] = createEventEmitter()
@@ -231,15 +258,26 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
     let cp: TinyExecResult | undefined
     let currentResult: DevframeChildProcessResult | undefined
     let runId = 0
+    // Runs stopped on purpose (terminate/restart/cancel). On Windows the tree
+    // kill ends them with exit code 1 rather than a signal, so this keeps them
+    // reported as a deliberate stop instead of a crash on every platform.
+    const hostKilled = new WeakSet<TinyExecResult>()
+    const killRun = (target: TinyExecResult | undefined): Promise<void> => {
+      if (!target)
+        return Promise.resolve()
+      hostKilled.add(target)
+      return killProcessTree(target)
+    }
 
     const stream = new ReadableStream<string>({
       start(_controller) {
         state.controller = _controller
       },
       cancel() {
-        cp?.kill()
+        const target = cp
         cp = undefined
         closeStream()
+        return killRun(target)
       },
     })
 
@@ -310,26 +348,27 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
         markStatus('error')
       })
       cp.process?.once('close', (code) => {
-        settle(code ?? undefined)
+        const killed = hostKilled.has(cp)
+        settle(killed ? undefined : code ?? undefined)
         if (currentRun !== runId)
           return
         closeStream()
         // A spawn/runtime error already settled the status; a non-zero exit
-        // code is a crash. A clean exit, or a signal kill (no numeric code,
-        // e.g. terminate()/restart()), is a deliberate/normal stop.
+        // code is a crash. A clean exit, or a kill by the host (terminate()/
+        // restart()), is a deliberate/normal stop.
         if (!runErrored)
-          markStatus(typeof code === 'number' && code !== 0 ? 'error' : 'stopped')
+          markStatus(!killed && typeof code === 'number' && code !== 0 ? 'error' : 'stopped')
       })
 
       currentResult = {
         get pid() {
           return cp.process?.pid
         },
         get exitCode() {
-          return cp.process?.exitCode ?? undefined
+          return hostKilled.has(cp) ? undefined : cp.process?.exitCode ?? undefined
         },
         get killed() {
-          return cp.process?.killed === true
+          return hostKilled.has(cp) || cp.process?.killed === true
         },
         kill: signal => cp.kill(signal),
         then: (onfulfilled, onrejected) => outputPromise.then(onfulfilled, onrejected),
@@ -343,13 +382,15 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType {
     const restart = async () => {
       if (state.streamClosed)
         throw diagnostics.DF8206({ id: terminal.id })
-      cp?.kill()
+      // Wait for the old tree to go away so the new run can reclaim its ports.
+      await killRun(cp)
       cp = createChildProcess()
       markStatus('running')
     }
     const terminate = async () => {
-      cp?.kill()
+      const target = cp
       cp = undefined
+      await killRun(target)
       closeStream()
       markStatus('stopped')
     }

packages/hub/src/types/terminals.ts

@@ -79,7 +79,7 @@ export interface DevframeChildProcessExecuteOptions {
  * The settled outcome of a {@link DevframeChildProcessTerminalSession} run:
  * stdout/stderr captured separately (unlike the session's merged display
  * `stream`), plus the process's exit code (`undefined` if it was killed by a
- * signal before exiting).
+ * signal, or by `terminate()`/`restart()`, before exiting).
  */
 export interface DevframeChildProcessOutput {
   stdout: string