diff --git a/packages/hub/src/node/__tests__/host-terminals.test.ts b/packages/hub/src/node/__tests__/host-terminals.test.ts index be481fec..e14b965b 100644 --- a/packages/hub/src/node/__tests__/host-terminals.test.ts +++ b/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 { - const deadline = Date.now() + 1000 +async function waitUntil(assertion: () => void, timeout = 1000): Promise { + 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) diff --git a/packages/hub/src/node/host-terminals.ts b/packages/hub/src/node/host-terminals.ts index e98729ef..58b25ba5 100644 --- a/packages/hub/src/node/host-terminals.ts +++ b/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 { + 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() + const killRun = (target: TinyExecResult | undefined): Promise => { + if (!target) + return Promise.resolve() + hostKilled.add(target) + return killProcessTree(target) + } const stream = new ReadableStream({ start(_controller) { state.controller = _controller }, cancel() { - cp?.kill() + const target = cp cp = undefined closeStream() + return killRun(target) }, }) @@ -310,15 +348,16 @@ 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 = { @@ -326,10 +365,10 @@ export class DevframeTerminalsHost implements DevframeTerminalsHostType { 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') } diff --git a/packages/hub/src/types/terminals.ts b/packages/hub/src/types/terminals.ts index 0f1fcdd8..feced0e4 100644 --- a/packages/hub/src/types/terminals.ts +++ b/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