From 32b2b2409915ffeffaa086e8a1343b9aa37f7433 Mon Sep 17 00:00:00 2001 From: pku-xht Date: Fri, 28 Aug 2026 20:30:11 +0800 Subject: [PATCH] fix(subprocess): close runner stdio handles --- .../tests/spawn-runner.spec.ts | 35 +++++++++++++ .../subprocess/win32-process/src/process.ts | 49 +++++++++++++++---- .../tests/ordinary-process.spec.ts | 47 +++++++++++++----- 3 files changed, 109 insertions(+), 22 deletions(-) diff --git a/packages/subprocess/subprocess-local/tests/spawn-runner.spec.ts b/packages/subprocess/subprocess-local/tests/spawn-runner.spec.ts index 9732cab128..32f2bdb90b 100644 --- a/packages/subprocess/subprocess-local/tests/spawn-runner.spec.ts +++ b/packages/subprocess/subprocess-local/tests/spawn-runner.spec.ts @@ -342,6 +342,41 @@ describe('runner launch inputs', () => { candidate === 'C:\\target\\tool.exe')).toBe('C:\\target\\tool.exe') expect(resolveWindowsExecutable('.\\missing', 'C:\\target', {}, () => false)) .toBe('C:\\target\\.\\missing') + + expect(resolveWindowsExecutable('tool', 'C:\\target', { + PATH: ';;C:\\bin', + }, candidate => candidate === 'C:\\bin\\tool.exe')).toBe('C:\\bin\\tool.exe') + expect(resolveWindowsExecutable('tool', 'C:\\target', { + PATH: '"";C:\\bin', + }, candidate => candidate === 'C:\\bin\\tool.exe')).toBe('C:\\bin\\tool.exe') + expect(resolveWindowsExecutable('tool', 'C:\\target', { + PATH: '"unterminated', + }, candidate => candidate === 'C:\\target\\unterminated\\tool.exe')) + .toBe('C:\\target\\unterminated\\tool.exe') + expect(resolveWindowsExecutable('\\\\server\\share\\tool', 'C:\\target', {}, candidate => + candidate === '\\\\server\\share\\tool.exe')).toBe('\\\\server\\share\\tool.exe') + expect(resolveWindowsExecutable('\\tools\\tool', 'C:\\target', {}, candidate => + candidate === 'C:\\tools\\tool.exe')).toBe('C:\\tools\\tool.exe') + expect(resolveWindowsExecutable('C:tools\\tool', 'C:\\target', {}, candidate => + candidate === 'C:\\target\\tools\\tool.exe')).toBe('C:\\target\\tools\\tool.exe') + + const noSearchEnvironment = { NoDefaultCurrentDirectoryInExePath: '1' } + expect(resolveWindowsExecutable('missing', 'C:\\target', {}, () => false, noSearchEnvironment)) + .toBe('C:\\target\\missing.exe') + expect(resolveWindowsExecutable('missing.cmd', 'C:\\target', {}, () => false, noSearchEnvironment)) + .toBe('C:\\target\\missing.cmd') + + const directory = mkdtempSync(join(tmpdir(), 'dsh-windows-resolver-')) + scratch.push(directory) + const executable = join(directory, 'direct.exe') + const directoryCandidate = join(directory, 'directory') + const missingExecutable = join(directory, 'missing.exe') + writeFileSync(executable, '') + mkdirSync(`${directoryCandidate}.com`) + writeFileSync(`${directoryCandidate}.exe`, '') + expect(resolveWindowsExecutable(executable, '', {})).toBe(executable) + expect(resolveWindowsExecutable(directoryCandidate, '', {})).toBe(`${directoryCandidate}.exe`) + expect(resolveWindowsExecutable(missingExecutable, '', {})).toBe(missingExecutable) }) }) diff --git a/packages/subprocess/win32-process/src/process.ts b/packages/subprocess/win32-process/src/process.ts index cbd7580316..ca1cedd028 100644 --- a/packages/subprocess/win32-process/src/process.ts +++ b/packages/subprocess/win32-process/src/process.ts @@ -1,6 +1,7 @@ /** Typed Win32 process operations over the shared binding table. */ import koffi from 'koffi' +import { closeSync } from 'node:fs' import * as abi from './abi.ts' import { allocProcessInfo, @@ -484,28 +485,56 @@ export function probeCurrentTokenJobSupport(api: Win32ProcessBindings): void { closeHandleChecked(api, job, 'current-token Job capability probe') } +interface MaterializedStdioStream { + readonly destroyed: boolean + destroy(): unknown + readonly _handle?: { close(): void } | null +} + /** * Release the runner's Node-owned standard streams after target creation. - * The target retains its inherited handle copies; destroying the runner's - * libuv owners permits parent pipe EOF to follow the target rather than the - * longer-lived runner. Raw `CloseHandle` is insufficient because Node may own - * a duplicated libuv handle that remains live until its stream is destroyed. - * @param streams - injectable current-process streams used by tests. + * The target retains its inherited handle copies. Node deliberately makes + * `process.stdout.destroy()` and `process.stderr.destroy()` leave their libuv + * handles open, so the runner must close both the descriptors and materialized + * output handles for parent pipe EOF to follow the target. + * @param streams - stdin, stdout, and stderr used by the current process. + * @param closeDescriptor - injectable descriptor close used by tests. */ export function closeCurrentProcessStandardStreams( - streams: ReadonlyArray<{ readonly destroyed: boolean; destroy(): unknown }> = [ + streams: readonly [MaterializedStdioStream, MaterializedStdioStream, MaterializedStdioStream] = [ process.stdin, process.stdout, process.stderr, ], + closeDescriptor: (fd: number) => void = closeSync, ): void { const failures: Error[] = [] - for (const stream of new Set(streams)) { - if (stream.destroyed) continue + const recordFailure = (error: unknown): void => { + failures.push(error instanceof Error ? error : new Error(String(error))) + } + const [stdin, ...outputs] = streams + const outputHandles = new Set(outputs.flatMap(stream => stream.destroyed || stream._handle == null + ? [] + : [stream._handle])) + if (!stdin.destroyed) { try { - stream.destroy() + stdin.destroy() } catch (error) { - failures.push(error instanceof Error ? error : new Error(String(error))) + recordFailure(error) + } + } + for (const fd of [0, 1, 2]) { + try { + closeDescriptor(fd) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EBADF') recordFailure(error) + } + } + for (const handle of outputHandles) { + try { + handle.close() + } catch (error) { + recordFailure(error) } } if (failures.length === 1) { diff --git a/packages/subprocess/win32-process/tests/ordinary-process.spec.ts b/packages/subprocess/win32-process/tests/ordinary-process.spec.ts index a9903ac648..76ea688f9f 100644 --- a/packages/subprocess/win32-process/tests/ordinary-process.spec.ts +++ b/packages/subprocess/win32-process/tests/ordinary-process.spec.ts @@ -213,27 +213,49 @@ describe('ordinary Job process operations', () => { expect(closeHandle).toHaveBeenCalledExactlyOnceWith(50n) }) - it('destroys each unique live runner standard stream', () => { - const alreadyClosed = { destroyed: true, destroy: vi.fn() } - const live = { destroyed: false, destroy: vi.fn() } + it('closes each runner descriptor and materialized output handle', () => { + const closeDescriptor = vi.fn() + const input = { destroyed: false, destroy: vi.fn() } + const sharedHandle = { close: vi.fn() } + const output = { destroyed: false, destroy: vi.fn(), _handle: sharedHandle } + const alreadyClosed = { destroyed: true, destroy: vi.fn(), _handle: { close: vi.fn() } } - expect(() => { closeCurrentProcessStandardStreams([alreadyClosed, live, live]) }).not.toThrow() + expect(() => { + closeCurrentProcessStandardStreams([input, output, output], closeDescriptor) + }).not.toThrow() + expect(input.destroy).toHaveBeenCalledOnce() + expect(output.destroy).not.toHaveBeenCalled() + expect(sharedHandle.close).toHaveBeenCalledOnce() + expect(closeDescriptor.mock.calls).toEqual([[0], [1], [2]]) + + expect(() => { + closeCurrentProcessStandardStreams([alreadyClosed, alreadyClosed, alreadyClosed], closeDescriptor) + }).not.toThrow() expect(alreadyClosed.destroy).not.toHaveBeenCalled() - expect(live.destroy).toHaveBeenCalledOnce() + expect(alreadyClosed._handle.close).not.toHaveBeenCalled() }) it('reports one or several runner standard-stream close failures', () => { - expect(() => { closeCurrentProcessStandardStreams([{ - destroyed: false, - destroy: () => { throw new Error('single close failure') }, - }]) }).toThrow('single close failure') + const closed = { destroyed: true, destroy: vi.fn() } + expect(() => { + closeCurrentProcessStandardStreams([ + { destroyed: false, destroy: () => { throw new Error('single close failure') } }, + closed, + closed, + ], vi.fn((fd: number) => { + if (fd === 0) throw Object.assign(new Error('already closed'), { code: 'EBADF' }) + })) + }).toThrow('single close failure') let failure: unknown try { closeCurrentProcessStandardStreams([ - { destroyed: false, destroy: () => { throw 'raw close failure' } }, - { destroyed: false, destroy: () => { throw new Error('second close failure') } }, - ]) + closed, + { destroyed: false, destroy: vi.fn(), _handle: { close: () => { throw 'raw close failure' } } }, + { destroyed: false, destroy: vi.fn(), _handle: { close: () => { throw new Error('second close failure') } } }, + ], (fd) => { + if (fd === 1) throw new Error('descriptor close failure') + }) } catch (error) { failure = error } @@ -245,6 +267,7 @@ describe('ordinary Job process operations', () => { expect(errors).toEqual(expect.arrayContaining([ expect.objectContaining({ message: 'raw close failure' }), expect.objectContaining({ message: 'second close failure' }), + expect.objectContaining({ message: 'descriptor close failure' }), ])) }) })