From c60a000ac765e180b9dbe003a022284e94030609 Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Thu, 23 Jul 2026 14:15:04 -0400 Subject: [PATCH 1/5] refactor(server): pf-4401 centralize child-process, ipc, host --- package.json | 3 +- .../__snapshots__/server.tools.test.ts.snap | 160 ++---- .../server.toolsHost.test.ts.snap | 257 --------- src/__tests__/server.process.test.ts | 225 ++++++++ src/__tests__/server.processHost.test.ts | 133 +++++ src/__tests__/server.processIpc.test.ts | 135 +++++ src/__tests__/server.tools.test.ts | 272 +++------ src/__tests__/server.toolsHost.test.ts | 361 +----------- src/__tests__/server.toolsIpc.test.ts | 515 +----------------- src/server.process.ts | 322 +++++++++++ src/server.processHost.ts | 176 ++++++ src/server.processIpc.ts | 173 ++++++ src/server.tools.ts | 248 ++------- src/server.toolsHost.ts | 255 ++------- src/server.toolsIpc.ts | 175 +----- 15 files changed, 1411 insertions(+), 1999 deletions(-) create mode 100644 src/__tests__/server.process.test.ts create mode 100644 src/__tests__/server.processHost.test.ts create mode 100644 src/__tests__/server.processIpc.test.ts create mode 100644 src/server.process.ts create mode 100644 src/server.processHost.ts create mode 100644 src/server.processIpc.ts diff --git a/package.json b/package.json index 725ee418..1b0876d0 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,8 @@ "type": "module", "imports": { "~docsCatalog": "./src/docs.json", - "#toolsHost": "./dist/server.toolsHost.js" + "#toolsHost": "./dist/server.toolsHost.js", + "#processHost": "./dist/server.processHost.js" }, "exports": { ".": { diff --git a/src/__tests__/__snapshots__/server.tools.test.ts.snap b/src/__tests__/__snapshots__/server.tools.test.ts.snap index 0fe70b85..741e199d 100644 --- a/src/__tests__/__snapshots__/server.tools.test.ts.snap +++ b/src/__tests__/__snapshots__/server.tools.test.ts.snap @@ -16,22 +16,14 @@ exports[`composeTools should attempt to setup creators, file package creators 1` exports[`composeTools should attempt to setup creators, file package creators, Node.js 22 1`] = ` { - "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], - ], + "log": [], "toolsCount": 5, } `; exports[`composeTools should attempt to setup creators, file package creators, Node.js 24 1`] = ` { - "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], - ], + "log": [], "toolsCount": 5, } `; @@ -39,9 +31,6 @@ exports[`composeTools should attempt to setup creators, file package creators, N exports[`composeTools should attempt to setup creators, file package creators, Node.js undefined 1`] = ` { "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], [ "External tool plugins require Node >= 22; skipping file-based tools.", ], @@ -53,9 +42,6 @@ exports[`composeTools should attempt to setup creators, file package creators, N exports[`composeTools should attempt to setup creators, file package duplicate creators 1`] = ` { "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], [ "Skipping tool plugin "@patternfly/tools" – name already used by built-in/inline tool.", ], @@ -66,11 +52,7 @@ exports[`composeTools should attempt to setup creators, file package duplicate c exports[`composeTools should attempt to setup creators, inline and file package creators 1`] = ` { - "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], - ], + "log": [], "toolsCount": 7, } `; @@ -78,9 +60,6 @@ exports[`composeTools should attempt to setup creators, inline and file package exports[`composeTools should attempt to setup creators, inline and file package creators duplicate builtin creators 1`] = ` { "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], [ "Skipping inline tool "loremipsum" because a tool with the same name is already provided (built-in or earlier).", ], @@ -95,9 +74,6 @@ exports[`composeTools should attempt to setup creators, inline and file package exports[`composeTools should attempt to setup creators, inline and file package creators, duplicates 1`] = ` { "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], [ "Skipping tool plugin "@patternfly/tools" – name already used by built-in/inline tool.", ], @@ -112,9 +88,6 @@ exports[`composeTools should attempt to setup creators, inline and file package exports[`composeTools should attempt to setup creators, inline and file package creators, duplicates, Node.js 22 1`] = ` { "log": [ - [ - "Existing Tools Host session detected test-session-id. Shutting down the existing host before creating a new one.", - ], [ "Skipping tool plugin "@patternfly/tools" – name already used by built-in/inline tool.", ], @@ -175,7 +148,7 @@ exports[`composeTools should return default creators on tools host error 1`] = ` { "log": [ [ - "Failed to start Tools Host; skipping externals and continuing with built-ins/inline. undefined", + "Failed to start Tools Host; skipping externals and continuing with built-ins/inline. Error: Mock spawn failure", ], ], "toolsCount": 3, @@ -410,54 +383,54 @@ exports[`logWarningsErrors should log warnings and errors, with warnings only 1` exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with error: handler 1`] = `[Error: Error message]`; -exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with error: send 1`] = ` +exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with error: request 1`] = ` [ [ - undefined, { "args": { "loremIpsum": 7, }, - "id": "id-1", "t": "invoke", "toolId": "loremIpsum", }, + "invoke:result", + 10, ], ] `; exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with full error: handler 1`] = `[Error: Error message]`; -exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with full error: send 1`] = ` +exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false with full error: request 1`] = ` [ [ - undefined, { "args": { "loremIpsum": 7, }, - "id": "id-1", "t": "invoke", "toolId": "loremIpsum", }, + "invoke:result", + 10, ], ] `; exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false: handler 1`] = `[Error: Tool invocation failed]`; -exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false: send 1`] = ` +exports[`makeProxyCreators should attempt to invoke a creator then throw an error on child response, ok false: request 1`] = ` [ [ - undefined, { "args": { "loremIpsum": 7, }, - "id": "id-1", "t": "invoke", "toolId": "loremIpsum", }, + "invoke:result", + 10, ], ] `; @@ -577,97 +550,62 @@ exports[`makeProxyCreators should attempt to return proxy creators, a function w } `; -exports[`sendToolsHostShutdown should attempt force shutdown of child and fail NaN`] = ` -[ - [ - "Failed to send shutdown signal to Tools Host child process: Error: Mock send failure", - ], - [ - "Failed to force-kill Tools Host child process: Error: Mock failed to kill child process", - ], - [ - "Failed to close Tools Host stderr reader: Error: Mock close failure 1", - ], - [ - "Slow shutdown response. Primary fallback force-killing Tools Host child process.", - ], -] -`; - exports[`spawnToolsHost attempt to spawn the Tools Host, with no pluginIsolation, node 24: spawn 1`] = ` { - "spawn": [ - [ - "/mock/path/to/toolsHost.js", - ], - { - "stdio": [ - "ignore", - "pipe", - "pipe", - "ipc", - ], + "spawnConfig": { + "enableStderrDebug": [Function], + "importSpecifier": "#toolsHost", + "isolation": { + "fsReadAllowlist": [], + "mode": "none", + "nodeVersion": 24, }, - ], + "label": "Tools Host", + }, } `; exports[`spawnToolsHost attempt to spawn the Tools Host, with strict pluginIsolation, node 22: spawn 1`] = ` { - "spawn": [ - [ - "--experimental-permission", - "--allow-fs-read=/", - "--allow-fs-read=/mock/path/to", - "/mock/path/to/toolsHost.js", - ], - { - "stdio": [ - "ignore", - "pipe", - "pipe", - "ipc", - ], + "spawnConfig": { + "enableStderrDebug": [Function], + "importSpecifier": "#toolsHost", + "isolation": { + "fsReadAllowlist": [], + "mode": "strict", + "nodeVersion": 22, }, - ], + "label": "Tools Host", + }, } `; exports[`spawnToolsHost attempt to spawn the Tools Host, with strict pluginIsolation, node 24: spawn 1`] = ` { - "spawn": [ - [ - "--permission", - "--allow-fs-read=/", - "--allow-fs-read=/mock/path/to", - "/mock/path/to/toolsHost.js", - ], - { - "stdio": [ - "ignore", - "pipe", - "pipe", - "ipc", - ], + "spawnConfig": { + "enableStderrDebug": [Function], + "importSpecifier": "#toolsHost", + "isolation": { + "fsReadAllowlist": [], + "mode": "strict", + "nodeVersion": 24, }, - ], + "label": "Tools Host", + }, } `; exports[`spawnToolsHost attempt to spawn the Tools Host, with undefined pluginIsolation, node 22: spawn 1`] = ` { - "spawn": [ - [ - "/mock/path/to/toolsHost.js", - ], - { - "stdio": [ - "ignore", - "pipe", - "pipe", - "ipc", - ], + "spawnConfig": { + "enableStderrDebug": [Function], + "importSpecifier": "#toolsHost", + "isolation": { + "fsReadAllowlist": [], + "mode": "none", + "nodeVersion": 22, }, - ], + "label": "Tools Host", + }, } `; diff --git a/src/__tests__/__snapshots__/server.toolsHost.test.ts.snap b/src/__tests__/__snapshots__/server.toolsHost.test.ts.snap index 334882d6..4b1865b6 100644 --- a/src/__tests__/__snapshots__/server.toolsHost.test.ts.snap +++ b/src/__tests__/__snapshots__/server.toolsHost.test.ts.snap @@ -141,55 +141,6 @@ exports[`normalizeCreatorSchema should attempt to normalize a schema, with valid } `; -exports[`requestFallback should send error response, with request id 1`] = ` -{ - "error": "Test error", - "id": "test-id", - "ok": false, - "t": "invoke:result", -} -`; - -exports[`requestFallback should send error response, with string error 1`] = ` -{ - "error": "String error", - "id": "test-id", - "ok": false, - "t": "invoke:result", -} -`; - -exports[`requestFallback should send error response, without request id 1`] = ` -{ - "error": "Test error", - "id": "n/a", - "ok": false, - "t": "invoke:result", -} -`; - -exports[`requestHello should send hello:ack message, with different id 1`] = ` -[ - [ - { - "id": "test-id-2", - "t": "hello:ack", - }, - ], -] -`; - -exports[`requestHello should send hello:ack message, with valid request 1`] = ` -[ - [ - { - "id": "test-id-1", - "t": "hello:ack", - }, - ], -] -`; - exports[`requestInvoke should attempt tool invocation, handler attempting to return a DOMException-like object, with name, message and multiline line stack 1`] = ` { "error": "Internal error", @@ -339,211 +290,3 @@ exports[`requestInvoke should timeout when handler takes too long 1`] = ` ], ] `; - -exports[`requestLoad should send load:ack message, with empty warnings and errors 1`] = ` -[ - [ - { - "errors": [], - "id": "test-id", - "t": "load:ack", - "warnings": [], - }, - ], -] -`; - -exports[`requestLoad should send load:ack message, with only errors 1`] = ` -[ - [ - { - "errors": [ - "error1", - ], - "id": "test-id", - "t": "load:ack", - "warnings": [], - }, - ], -] -`; - -exports[`requestLoad should send load:ack message, with only warnings 1`] = ` -[ - [ - { - "errors": [], - "id": "test-id", - "t": "load:ack", - "warnings": [ - "warning1", - ], - }, - ], -] -`; - -exports[`requestLoad should send load:ack message, with undefined warnings and errors 1`] = ` -[ - [ - { - "errors": [], - "id": "test-id", - "t": "load:ack", - "warnings": [], - }, - ], -] -`; - -exports[`requestLoad should send load:ack message, with warnings and errors 1`] = ` -[ - [ - { - "errors": [ - "error1", - ], - "id": "test-id", - "t": "load:ack", - "warnings": [ - "warning1", - "warning2", - ], - }, - ], -] -`; - -exports[`requestManifestGet should send manifest:result message, with empty descriptors 1`] = ` -[ - [ - { - "id": "test-id", - "t": "manifest:result", - "tools": [], - }, - ], -] -`; - -exports[`requestManifestGet should send manifest:result message, with multiple tool descriptors 1`] = ` -[ - [ - { - "id": "test-id", - "t": "manifest:result", - "tools": [ - { - "description": "Description 1", - "id": "tool-1", - "inputSchema": { - "type": "object", - }, - "name": "Tool1", - "source": "module1", - }, - { - "description": "Description 2", - "id": "tool-2", - "inputSchema": {}, - "name": "Tool2", - "source": "module2", - }, - ], - }, - ], -] -`; - -exports[`requestManifestGet should send manifest:result message, with single tool descriptor 1`] = ` -[ - [ - { - "id": "test-id", - "t": "manifest:result", - "tools": [ - { - "description": "Description 1", - "id": "tool-1", - "inputSchema": {}, - "name": "Tool1", - "source": "module1", - }, - ], - }, - ], -] -`; - -exports[`requestShutdown should send shutdown:ack and exit, with different id 1`] = ` -[ - [ - { - "id": "test-id-2", - "t": "shutdown:ack", - }, - ], -] -`; - -exports[`requestShutdown should send shutdown:ack and exit, with valid request 1`] = ` -[ - [ - { - "id": "test-id-1", - "t": "shutdown:ack", - }, - ], -] -`; - -exports[`setHandlers should set up message handlers and attempt handle requests, hello 1`] = ` -[ - [ - { - "id": "test-id", - "t": "hello:ack", - }, - ], -] -`; - -exports[`setHandlers should set up message handlers and attempt handle requests, invoke 1`] = ` -[ - [ - { - "error": { - "message": "Unknown toolId", - }, - "id": "test-id", - "ok": false, - "t": "invoke:result", - }, - ], -] -`; - -exports[`setHandlers should set up message handlers and attempt handle requests, load 1`] = ` -[ - [ - { - "errors": [], - "id": "test-id", - "t": "load:ack", - "warnings": [], - }, - ], -] -`; - -exports[`setHandlers should set up message handlers and attempt handle requests, manifest:get 1`] = ` -[ - [ - { - "id": "test-id", - "t": "manifest:result", - "tools": [], - }, - ], -] -`; diff --git a/src/__tests__/server.process.test.ts b/src/__tests__/server.process.test.ts new file mode 100644 index 00000000..81448e55 --- /dev/null +++ b/src/__tests__/server.process.test.ts @@ -0,0 +1,225 @@ +import { spawn } from 'node:child_process'; +import { + buildIsolationArgs, + shutdownChildProcess, + resolveEntry, + spawnChildProcess, + activeChildrenBySession +} from '../server.process'; +import { log } from '../logger'; + +jest.mock('node:child_process', () => ({ + spawn: jest.fn() +})); + +jest.mock('node:fs', () => ({ + realpathSync: (path: string) => path +})); + +jest.mock('../logger', () => ({ + log: { + warn: jest.fn(), + error: jest.fn(), + info: jest.fn(), + debug: jest.fn() + }, + formatUnknownError: jest.fn((error: unknown) => String(error)) +})); + +describe('resolveEntry', () => { + const MockLog = jest.mocked(log); + + it('should return a pre-resolved entry as-is', () => { + const out = resolveEntry({ importSpecifier: '#host', entry: '/lorem/ipsum.js' }); + + expect(out).toBe('/lorem/ipsum.js'); + }); + + it('should throw when the specifier cannot be resolved', () => { + resolveEntry({ label: 'Test Host' } as any); + + expect(MockLog.debug).toHaveBeenCalledWith(expect.stringContaining('Failed to import.meta.resolve Test Host')); + }); + + it('should fall back to the mock path under NODE_ENV=local', () => { + const previous = process.env.NODE_ENV; + + process.env.NODE_ENV = 'local'; + + try { + expect(resolveEntry({ importSpecifier: '#nope', label: 'Local Host' })).toBe('/mock/path/to/host.js'); + } finally { + process.env.NODE_ENV = previous; + } + }); +}); + +describe('buildIsolationArgs', () => { + it.each([ + { description: 'non-strict yields no args', isolation: { mode: 'none' as const }, expectFlag: undefined }, + { description: 'node 22 uses experimental flag', isolation: { mode: 'strict' as const, nodeVersion: 22 }, expectFlag: '--experimental-permission' }, + { description: 'node 24 uses permission flag', isolation: { mode: 'strict' as const, nodeVersion: 24 }, expectFlag: '--permission' } + ])('$description', ({ isolation, expectFlag }) => { + const args = buildIsolationArgs('/abs/dir/host.js', isolation); + + if (expectFlag === undefined) { + expect(args).toEqual([]); + } else { + expect(args[0]).toBe(expectFlag); + expect(args).toContain('--allow-fs-read=/abs/dir'); + } + }); + + it('should include the injected fsReadAllowlist', () => { + const args = buildIsolationArgs('/abs/dir/host.js', { + mode: 'strict', nodeVersion: 24, fsReadAllowlist: ['/project'] + }); + + expect(args).toContain('--allow-fs-read=/project'); + }); +}); + +describe('shutdownChildProcess', () => { + const makeChild = () => { + const listeners: Record void>> = {}; + + return { + killed: false, + kill: jest.fn(), + send: jest.fn().mockReturnValue(true), + once: jest.fn((event: string, handle: any) => (listeners[event] ??= []).push(handle)), + off: jest.fn(), + _emit: (event: string) => (listeners[event] || []).forEach(handle => handle()) + } as any; + }; + + it('should resolve immediately when no handle is provided', async () => { + await expect(shutdownChildProcess(undefined)).resolves.toBeUndefined(); + }); + + it('should send shutdown, resolve on exit, and close stderr', async () => { + const child = makeChild(); + const closeStderr = jest.fn(); + const handle = { child, closeStderr, request: jest.fn() } as any; + const promise = shutdownChildProcess(handle, { gracePeriodMs: 0 }); + + child._emit('exit'); + + await promise; + + expect(child.send).toHaveBeenCalledWith(expect.objectContaining({ t: 'shutdown' })); + expect(closeStderr).toHaveBeenCalledTimes(1); + }); + + it('should force-kill via the primary fallback timer', async () => { + jest.useFakeTimers(); + const child = makeChild(); + const handle = { child, closeStderr: jest.fn(), request: jest.fn() } as any; + const promise = shutdownChildProcess(handle, { gracePeriodMs: 0 }); + + jest.advanceTimersByTime(1); + await promise; + + expect(child.kill).toHaveBeenCalledWith('SIGKILL'); + jest.useRealTimers(); + }); + + it('should remove the handle from the session registry on exit', async () => { + const child = makeChild(); + const handle = { child, closeStderr: jest.fn(), request: jest.fn() } as any; + + activeChildrenBySession.set('session-1', handle); + + const promise = shutdownChildProcess(handle, { gracePeriodMs: 0, sessionId: 'session-1' }); + + child._emit('exit'); + await promise; + + expect(activeChildrenBySession.has('session-1')).toBe(false); + }); +}); + +describe('spawnChildProcess stdio', () => { + it('should spawn with the IPC-capable stdio shape', async () => { + (spawn as jest.Mock).mockReturnValue({ stderr: { on: jest.fn(), off: jest.fn() } }); + const { spawnChildProcess } = await import('../server.process'); + + spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + + expect(spawn).toHaveBeenCalledWith( + process.execPath, + ['/abs/host.js'], + { stdio: ['ignore', 'pipe', 'pipe', 'ipc'] } + ); + }); +}); + +describe('spawnChildProcess request', () => { + const makeIpcChild = () => { + const messageHandlers: Array<(m: any) => void> = []; + const child: any = { + send: jest.fn(), + on: jest.fn((event: string, handler: any) => { + if (event === 'message') { + messageHandlers.push(handler); + } + + return child; + }), + off: jest.fn(() => child), + stderr: { on: jest.fn(), off: jest.fn() } + }; + + return { child, messageHandlers }; + }; + + it('should correlate the response by the generated id', async () => { + const { child, messageHandlers } = makeIpcChild(); + + (spawn as jest.Mock).mockReturnValue(child); + + const { request } = spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + const pending = request({ t: 'hello' }, 'hello:ack', 1000); + const sentId = child.send.mock.calls[0][0].id; + + await Promise.resolve(); + messageHandlers.forEach(handler => handler({ t: 'hello:ack', id: sentId })); + + await expect(pending).resolves.toEqual({ t: 'hello:ack', id: sentId }); + expect(typeof sentId).toBe('string'); + }); + + it('should match any id when correlate is false', async () => { + const { child, messageHandlers } = makeIpcChild(); + + (spawn as jest.Mock).mockReturnValue(child); + + const { request } = spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + const pending = request({ t: 'ping' }, 'pong', 1000, false); + + await Promise.resolve(); + messageHandlers.forEach(handler => handler({ t: 'pong', id: 'unrelated' })); + + await expect(pending).resolves.toEqual({ t: 'pong', id: 'unrelated' }); + }); + + it('should reject on a correlated :error envelope', async () => { + const { child, messageHandlers } = makeIpcChild(); + + (spawn as jest.Mock).mockReturnValue(child); + + const { request } = spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + const pending = request({ t: 'load' }, 'load:ack', 1000); + const sentId = child.send.mock.calls[0][0].id; + + await Promise.resolve(); + messageHandlers.forEach(handler => handler({ + t: 'load:error', + id: sentId, + ok: false, + error: { message: 'handler boom', code: 'E_BOOM' } + })); + + await expect(pending).rejects.toThrow('handler boom'); + }); +}); diff --git a/src/__tests__/server.processHost.test.ts b/src/__tests__/server.processHost.test.ts new file mode 100644 index 00000000..49ad4038 --- /dev/null +++ b/src/__tests__/server.processHost.test.ts @@ -0,0 +1,133 @@ +import { setHandlers, createProcessHost, helloHandler, shutdownHandler } from '../server.processHost'; + +describe('server.processHost', () => { + let messageHandlers: Array<(m: any) => void>; + let disconnectHandlers: Array<() => void>; + let sendSpy: jest.Mock; + let exitSpy: jest.SpyInstance; + const originalSend = process.send; + + beforeEach(() => { + messageHandlers = []; + disconnectHandlers = []; + sendSpy = jest.fn(); + (process as any).send = sendSpy; + jest.spyOn(process, 'on').mockImplementation((event: any, handler: any) => { + if (event === 'message') { + messageHandlers.push(handler); + } + if (event === 'disconnect') { + disconnectHandlers.push(handler); + } + + return process; + }); + jest.spyOn(process, 'off').mockImplementation(() => process); + exitSpy = jest.spyOn(process, 'exit').mockImplementation((() => undefined) as any); + }); + + afterEach(() => { + jest.restoreAllMocks(); + (process as any).send = originalSend; + }); + + it('should route messages to the matching handler', async () => { + const load = jest.fn(); + + setHandlers({ load }); + await (messageHandlers[0] as any)({ t: 'load', id: '1' }); + + expect(load).toHaveBeenCalledWith({ t: 'load', id: '1' }, expect.objectContaining({ send: expect.any(Function) })); + }); + + it('should reply to the built-in hello handler', async () => { + setHandlers({}); + await (messageHandlers[0] as any)({ t: 'hello', id: 'h1' }); + + expect(sendSpy).toHaveBeenCalledWith({ t: 'hello:ack', id: 'h1' }); + }); + + it('should ignore unknown message types', async () => { + setHandlers({}); + await (messageHandlers[0] as any)({ t: 'nope', id: 'x' }); + + expect(sendSpy).not.toHaveBeenCalled(); + }); + + it('should route thrown handler errors through requestFallback', async () => { + setHandlers({ + lorem: () => { + throw new Error('bad'); + } + }); + await (messageHandlers[0] as any)({ t: 'lorem', id: 'b1' }); + + expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({ + t: 'lorem:error', id: 'b1', ok: false, error: expect.objectContaining({ message: 'bad' }) + })); + }); + + it('should exit on disconnect', () => { + setHandlers({}); + (disconnectHandlers[0] as any)(); + + expect(exitSpy).toHaveBeenCalledWith(0); + }); + + it('should ack and exit on the built-in shutdown handler', async () => { + setHandlers({}); + const handler = messageHandlers[0] as any; + + await handler({ t: 'shutdown', id: 's1' }); + + expect(sendSpy).toHaveBeenCalledWith({ t: 'shutdown:ack', id: 's1' }); + expect(exitSpy).toHaveBeenCalledWith(0); + }); + + it('shutdownHandler should ack via ctx.send then exit', async () => { + const send = jest.fn(); + + shutdownHandler({ t: 'shutdown', id: 'q' }, { send }); + + expect(send).toHaveBeenCalledWith({ t: 'shutdown:ack', id: 'q' }); + expect(exitSpy).toHaveBeenCalledWith(0); + }); + + it('should one-shot bootstrap: detach then route the first message', async () => { + const load = jest.fn(); + const { bootstrapMessage } = createProcessHost({ load }); + + await bootstrapMessage({ t: 'load', id: 'first' } as any); + + expect(process.off).toHaveBeenCalledWith('message', bootstrapMessage); + expect(load).toHaveBeenCalledWith({ t: 'load', id: 'first' }, expect.anything()); + }); + + it('should auto-attach the bootstrap listener once when process.send exists', () => { + const { bootstrapMessage } = createProcessHost({}); + const mockOn = process.on as jest.Mock; + + expect(mockOn).toHaveBeenCalledWith('message', bootstrapMessage); + expect(mockOn.mock.calls.filter(([event]) => event === 'message')).toHaveLength(1); + }); + + it('helloHandler should ack via ctx.send', () => { + const send = jest.fn(); + + helloHandler({ t: 'hello', id: 'z' }, { send }); + + expect(send).toHaveBeenCalledWith({ t: 'hello:ack', id: 'z' }); + }); + + it('should detach the prior bootstrap when instantiated again', () => { + const first = createProcessHost({}).bootstrapMessage; + const second = createProcessHost({}).bootstrapMessage; + + // The earlier bootstrap is removed before the new one is attached. + expect(process.off).toHaveBeenCalledWith('message', first); + // Only the latest bootstrap remains attached. + const attached = (process.on as jest.Mock).mock.calls.filter(([event]) => event === 'message'); + + expect(attached[attached.length - 1][1]).toBe(second); + }); +}); diff --git a/src/__tests__/server.processIpc.test.ts b/src/__tests__/server.processIpc.test.ts new file mode 100644 index 00000000..67f91eb8 --- /dev/null +++ b/src/__tests__/server.processIpc.test.ts @@ -0,0 +1,135 @@ +import { + send, + awaitIpc, + makeId, + matchResponse, + serializeError, + type ProcessRequest, + type ProcessResponse +} from '../server.processIpc'; + +describe('makeId', () => { + it('should generate unique ids', () => { + const ids = new Set(Array.from({ length: 100 }, () => makeId())); + + expect(ids.size).toBe(100); + }); +}); + +describe('send', () => { + let mockProcess: NodeJS.Process; + + beforeEach(() => { + mockProcess = { send: jest.fn().mockReturnValue(true) } as any; + }); + + it.each([ + { description: 'send returns true', process: { send: jest.fn().mockReturnValue(true) }, expected: true }, + { description: 'send returns false', process: { send: jest.fn().mockReturnValue(false) }, expected: false }, + { description: 'no send method', process: {}, expected: false } + ])('should return $expected, $description', ({ process, expected }) => { + expect(send(process as any, { t: 'hello', id: 'id' })).toBe(expected); + }); + + it('should forward the request payload', () => { + const request: ProcessRequest = { t: 'hello', id: 'id' }; + + send(mockProcess, request); + + expect(mockProcess.send).toHaveBeenCalledWith(request); + }); +}); + +describe('matchResponse', () => { + it.each([ + { description: 'type + id match', type: 'x:ack', id: 'a', message: { t: 'x:ack', id: 'a' }, expected: true }, + { description: 'type mismatch', type: 'x:ack', id: 'a', message: { t: 'y', id: 'a' }, expected: false }, + { description: 'id mismatch', type: 'x:ack', id: 'a', message: { t: 'x:ack', id: 'b' }, expected: false }, + { description: 'no id constraint', type: 'x:ack', id: undefined, message: { t: 'x:ack', id: 'z' }, expected: true }, + { description: 'no id constraint, non-string id', type: 'x:ack', id: undefined, message: { t: 'x:ack', id: 5 }, expected: false }, + { description: 'null message', type: 'x:ack', id: 'a', message: null, expected: false } + ])('should match, $description', ({ type, id, message, expected }) => { + expect(matchResponse(type, id as any)(message)).toBe(expected); + }); +}); + +describe('serializeError', () => { + it('should serialize an Error instance', () => { + const err: any = new Error('lorem'); + + err.code = 'E_IPSUM'; + + const out = serializeError(err); + + expect(out.message).toBe('lorem'); + expect(out.code).toBe('E_IPSUM'); + expect(typeof out.stack).toBe('string'); + }); + + it('should serialize a non-error value', () => { + expect(serializeError('dolor').message).toBe('dolor'); + }); + + it('should always emit the optional keys uniformly', () => { + const out = serializeError(new Error('lorem')); + + expect(Object.keys(out).sort()).toEqual(expect.arrayContaining(['cause', 'code', 'details', 'message', 'stack'])); + }); +}); + +describe('awaitIpc', () => { + let mockProcess: NodeJS.Process; + let messageHandlers: Array<(m: any) => void>; + let exitHandlers: Array<(c?: number, s?: string) => void>; + + beforeEach(() => { + messageHandlers = []; + exitHandlers = []; + mockProcess = { + on: jest.fn((event: string, handler: any) => { + if (event === 'message') { + messageHandlers.push(handler); + } + if (event === 'exit' || event === 'disconnect') { + exitHandlers.push(handler); + } + + return mockProcess; + }), + off: jest.fn(() => mockProcess) + } as any; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should resolve on a matching message and clean up', async () => { + const promise = awaitIpc(mockProcess, matchResponse('x:ack', 'id'), 1000); + + await Promise.resolve(); + messageHandlers.forEach(handler => handler({ t: 'x:ack', id: 'id' })); + + await expect(promise).resolves.toEqual({ t: 'x:ack', id: 'id' }); + expect(mockProcess.off).toHaveBeenCalledWith('message', expect.any(Function)); + }); + + it('should reject on early exit', async () => { + const promise = awaitIpc(mockProcess, matchResponse('x:ack', 'id'), 1000); + + await Promise.resolve(); + exitHandlers.forEach(handler => handler(1, 'SIGKILL')); + + await expect(promise).rejects.toThrow('exited before response'); + }); + + it('should reject on timeout', async () => { + jest.useFakeTimers(); + const promise = awaitIpc(mockProcess, matchResponse('x:ack', 'id'), 50); + + jest.advanceTimersByTime(60); + + await expect(promise).rejects.toThrow('Timed out'); + jest.useRealTimers(); + }); +}); diff --git a/src/__tests__/server.tools.test.ts b/src/__tests__/server.tools.test.ts index cd5c1ad8..d36013de 100644 --- a/src/__tests__/server.tools.test.ts +++ b/src/__tests__/server.tools.test.ts @@ -1,5 +1,4 @@ import { resolve } from 'node:path'; -import { spawn } from 'node:child_process'; import { z } from 'zod'; import { log } from '../logger'; import { @@ -13,11 +12,19 @@ import { sendToolsHostShutdown, composeTools } from '../server.tools'; -import { awaitIpc, makeId, send } from '../server.toolsIpc'; +import { spawnChildProcess, shutdownChildProcess, activeChildrenBySession } from '../server.process'; +import { getOptions, getSessionOptions } from '../options.context'; import { isZodSchema } from '../server.schema'; -jest.mock('node:child_process', () => ({ - spawn: jest.fn() +jest.mock('../server.process', () => ({ + spawnChildProcess: jest.fn(), + shutdownChildProcess: jest.fn().mockResolvedValue(undefined), + activeChildrenBySession: new Map() +})); + +jest.mock('../options.context', () => ({ + getOptions: jest.fn(), + getSessionOptions: jest.fn() })); jest.mock('../logger', () => ({ @@ -30,16 +37,6 @@ jest.mock('../logger', () => ({ formatUnknownError: jest.fn((error: unknown) => String(error)) })); -jest.mock('../server.toolsIpc', () => ({ - send: jest.fn(), - awaitIpc: jest.fn(), - makeId: jest.fn(() => 'id-1'), - isHelloAck: jest.fn((msg: any) => msg?.t === 'hello:ack'), - isInvokeResult: jest.fn((msg: any) => msg?.t === 'invoke:result'), - isLoadAck: jest.fn((id: string) => (msg: any) => msg?.t === 'load:ack' && msg?.id === id), - isManifestResult: jest.fn((id: string) => (msg: any) => msg?.t === 'manifest:result' && msg?.id === id) -})); - describe('getBuiltInToolNames', () => { const MockLog = jest.mocked(log); @@ -245,10 +242,7 @@ describe('debugChild', () => { }); describe('spawnToolsHost', () => { - const MockSpawn = jest.mocked(spawn); - const MockAwaitIpc = jest.mocked(awaitIpc); - const MockSend = jest.mocked(send); - const MockMakeId = jest.mocked(makeId); + const MockSpawnChildProcess = jest.mocked(spawnChildProcess); beforeEach(() => { jest.clearAllMocks(); @@ -272,46 +266,47 @@ describe('spawnToolsHost', () => { options: { nodeVersion: 24, pluginIsolation: 'strict' } } ])('attempt to spawn the Tools Host, $description', async ({ options }) => { - const updatedOptions = { pluginHost: { loadTimeoutMs: 10, invokeTimeoutMs: 10 }, ...options }; + const updatedOptions = { toolModules: [], pluginHost: { loadTimeoutMs: 10, invokeTimeoutMs: 10 }, ...options }; const mockPid = 123; const mockTools = [{ name: 'alphaTool' }, { name: 'betaTool' }]; - - MockSpawn.mockReturnValue({ - pid: mockPid + const mockRequest = jest.fn() + .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' }) + .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] }) + .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockTools }); + + MockSpawnChildProcess.mockReturnValue({ + child: { pid: mockPid } as any, + request: mockRequest, + closeStderr: jest.fn() } as any); - MockAwaitIpc - .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' } as any) - .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] } as any) - .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockTools } as any); + // Ensure internal calls get the right options + jest.mocked(getOptions).mockReturnValue(updatedOptions as any); const result = await spawnToolsHost(updatedOptions as any); expect(result.child.pid).toBe(mockPid); expect(result.tools).toEqual(mockTools); - expect(MockMakeId).toHaveBeenCalledTimes(3); - expect(MockSend).toHaveBeenCalledTimes(3); + expect(mockRequest).toHaveBeenCalledTimes(3); expect({ - spawn: MockSpawn.mock.calls?.[0]?.slice?.(1) + spawnConfig: MockSpawnChildProcess.mock.calls?.[0]?.[0] }).toMatchSnapshot('spawn'); }); - it('should throw when resolve fails', async () => { - process.env.NODE_ENV = '__test__'; + it('should throw when spawn fails', async () => { + jest.mocked(getOptions).mockReturnValue({ toolModules: [], pluginHost: {} } as any); + MockSpawnChildProcess.mockImplementationOnce(() => { + throw new Error('Failed to resolve Tools Host entry \'#toolsHost\'.'); + }); await expect( spawnToolsHost({ nodeVersion: 24, pluginIsolation: 'strict', pluginHost: {} } as any) ).rejects.toThrow(/Failed to resolve Tools Host/); - - process.env.NODE_ENV = 'local'; }); }); describe('makeProxyCreators', () => { - const MockAwaitIpc = jest.mocked(awaitIpc); - const MockSend = jest.mocked(send); - const MockMakeId = jest.mocked(makeId); const MockLog = jest.mocked(log); beforeEach(() => { @@ -448,11 +443,10 @@ describe('makeProxyCreators', () => { } ]; - MockMakeId.mockReturnValue('id-1' as any); - MockAwaitIpc - .mockResolvedValueOnce({ t: 'invoke:result', id: 'id-1', ...response } as any); + const mockRequest = jest.fn().mockResolvedValueOnce({ t: 'invoke:result', ...response }); + const mockHandle = { tools, request: mockRequest, child: { pid: 123 } }; - const proxies = makeProxyCreators({ tools } as any, { pluginHost: { invokeTimeoutMs: 10 } } as any); + const proxies = makeProxyCreators(mockHandle as any, { pluginHost: { invokeTimeoutMs: 10 } } as any); const [_name, _schema, handler]: any = proxies.map(proxy => { const [name, { description, inputSchema, ...rest }, handler] = proxy(); @@ -464,156 +458,41 @@ describe('makeProxyCreators', () => { })[0]; await expect(handler({ loremIpsum: 7 })).rejects.toMatchSnapshot('handler'); - expect(MockSend.mock.calls).toMatchSnapshot('send'); + expect(mockRequest).toHaveBeenCalledTimes(1); + expect(mockRequest.mock.calls).toMatchSnapshot('request'); }); }); describe('sendToolsHostShutdown', () => { - const MockLog = jest.mocked(log); - const MockSend = jest.mocked(send); - let mapGetSpy: jest.SpyInstance; - let mapDeleteSpy: jest.SpyInstance; + const MockShutdownChildProcess = jest.mocked(shutdownChildProcess); beforeEach(() => { jest.clearAllMocks(); - jest.useFakeTimers(); - mapGetSpy = jest.spyOn(Map.prototype, 'get'); - mapDeleteSpy = jest.spyOn(Map.prototype, 'delete'); - }); - - afterEach(() => { - jest.useRealTimers(); - mapGetSpy.mockRestore(); - mapDeleteSpy.mockRestore(); }); it('should attempt graceful shutdown of child', async () => { - const onceHandlers: Record = {}; - const child = { - kill: jest.fn(), - killed: false, - once: jest.fn((event: string, handler: any) => { - onceHandlers[event] = handler; - }), - off: jest.fn(), - stderr: { - on: jest.fn(), - off: jest.fn() - } - }; - const handle = { child, closeStderr: jest.fn() }; - const sessionId = 'test-session-id'; - - mapGetSpy.mockReturnValue(handle); - - const promise = sendToolsHostShutdown({ pluginHost: { gracePeriodMs: 10 } } as any, { sessionId } as any); - - onceHandlers['disconnect'](); - - await promise; - - expect(MockSend).toHaveBeenCalledTimes(1); - expect(child.once).toHaveBeenCalledTimes(2); - expect(child.off).toHaveBeenCalledWith('exit', onceHandlers['exit']); - expect(child.off).toHaveBeenCalledWith('disconnect', onceHandlers['disconnect']); - expect(handle.closeStderr).toHaveBeenCalledTimes(1); - expect(mapDeleteSpy).toHaveBeenCalledWith(sessionId); - - jest.advanceTimersByTime(220); - expect(child.kill).not.toHaveBeenCalled(); - }); - - it('should attempt force shutdown of child', async () => { - const child = { - // eslint-disable-next-line func-names - kill: jest.fn(function (this: any) { - this.killed = true; - - return true; - }), - killed: false, - once: jest.fn(), - off: jest.fn(), - stderr: { - on: jest.fn(), - off: jest.fn() - } - }; + const child = { pid: 123 }; const handle = { child, closeStderr: jest.fn() }; const sessionId = 'test-session-id'; - mapGetSpy.mockReturnValue(handle); - - const promise = sendToolsHostShutdown({ pluginHost: { gracePeriodMs: 10 } } as any, { sessionId } as any); - - jest.advanceTimersByTime(20); - await promise; + activeChildrenBySession.set(sessionId, handle as any); - jest.advanceTimersByTime(220); + await sendToolsHostShutdown({ pluginHost: { gracePeriodMs: 10 } } as any, { sessionId } as any); - expect(MockSend).toHaveBeenCalledTimes(1); - expect(child.once).toHaveBeenCalledTimes(2); - expect(child.kill).toHaveBeenCalledTimes(1); - expect(child.kill).toHaveBeenCalledWith('SIGKILL'); - expect(child.killed).toBe(true); - expect(child.off).toHaveBeenCalledTimes(2); - expect(handle.closeStderr).toHaveBeenCalledTimes(1); - expect(mapDeleteSpy).toHaveBeenCalledWith(sessionId); - }); - - it('should attempt force shutdown of child and fail', async () => { - const child = { - kill: jest.fn() - .mockImplementationOnce(() => { - throw new Error('Mock failed to kill child process'); - }) - // eslint-disable-next-line func-names - .mockImplementationOnce(function (this: any) { - this.killed = true; - - return true; - }), - killed: false, - once: jest.fn(), - off: jest.fn(), - stderr: { - on: jest.fn(), - off: jest.fn() - } - }; - const handle = { - child, - closeStderr: jest.fn() - .mockImplementationOnce(() => { - throw new Error('Mock close failure 1'); - }) - .mockImplementationOnce(() => {}) - }; - const sessionId = 'test-session-id'; - - MockSend.mockImplementationOnce(() => { - throw new Error('Mock send failure'); + expect(MockShutdownChildProcess).toHaveBeenCalledTimes(1); + expect(MockShutdownChildProcess).toHaveBeenCalledWith(handle, { + gracePeriodMs: 10, + sessionId, + label: 'Tools Host' }); - - mapGetSpy.mockReturnValue(handle); - - const promise = sendToolsHostShutdown({ pluginHost: { gracePeriodMs: 10 } } as any, { sessionId } as any); - - jest.advanceTimersByTime(10); - await promise; - - jest.advanceTimersByTime(220); - - expect(child.kill).toHaveBeenCalledWith('SIGKILL'); - expect(child.killed).toBe(false); - expect([...MockLog.error.mock.calls, ...MockLog.warn.mock.calls, ...MockLog.info.mock.calls]).toMatchSnapshot(); }); }); describe('composeTools', () => { - const MockSpawn = jest.mocked(spawn); - const MockAwaitIpc = jest.mocked(awaitIpc); + const MockSpawnChildProcess = jest.mocked(spawnChildProcess); const MockLog = jest.mocked(log); + const MockGetOptions = jest.mocked(getOptions); + const MockGetSessionOptions = jest.mocked(getSessionOptions); // Mock default creators const loremIpsum = () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: z.object({}) }, () => {}]; @@ -626,10 +505,9 @@ describe('composeTools', () => { beforeEach(() => { jest.clearAllMocks(); - jest.resetAllMocks(); - }); - - afterAll(() => { + activeChildrenBySession.clear(); + MockGetOptions.mockReturnValue({ toolModules: [], pluginHost: { loadTimeoutMs: 10, invokeTimeoutMs: 10 } } as any); + MockGetSessionOptions.mockReturnValue({ sessionId: 'test-session-id' } as any); }); it.each([ @@ -779,16 +657,24 @@ describe('composeTools', () => { const sessionId = 'test-session-id'; - MockSpawn.mockReturnValueOnce(mockChild as any); + const mockRequest = jest.fn() + .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' }) + .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] }) + .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockFilePackageTools }); - MockAwaitIpc - .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' } as any) - .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] } as any) - .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockFilePackageTools } as any); + MockSpawnChildProcess.mockReturnValue({ + child: mockChild as any, + request: mockRequest, + closeStderr: jest.fn() + } as any); const defaultCreators: any[] = [loremIpsum, dolorSitAmet, consecteturAdipiscingElit]; const globalOptions: any = { toolModules: filePackageToolModules, nodeVersion, contextUrl: 'file:///test/path', contextPath: '/test/path' }; const sessionOptions: any = { sessionId }; + + // Ensure getOptions returns what's expected for internal calls + MockGetOptions.mockReturnValue(globalOptions); + const tools = await composeTools(defaultCreators, globalOptions, sessionOptions); expect(tools.length).toBe(expectedModuleCount); @@ -805,34 +691,36 @@ describe('composeTools', () => { once: jest.fn((event: string, handler: any) => { onceHandlers[event] = handler; }), - off: jest.fn(), - stderr: { - on: jest.fn(), - off: jest.fn() - } + off: jest.fn() }; const filePackageToolModules: any[] = ['file:///test/module.js', '@patternfly/woot']; const mockFilePackageTools = filePackageToolModules.map(tool => ({ name: tool, description: tool, inputSchema: {}, source: tool })); const sessionId = 'test-session-id'; - MockSpawn.mockReturnValueOnce(mockChild as any); + const mockRequest = jest.fn() + .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' }) + .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] }) + .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockFilePackageTools }); - MockAwaitIpc - .mockResolvedValueOnce({ t: 'hello:ack', id: 'id-1' } as any) - .mockResolvedValueOnce({ t: 'load:ack', id: 'id-1', warnings: [], errors: [] } as any) - .mockResolvedValueOnce({ t: 'manifest:result', id: 'id-1', tools: mockFilePackageTools } as any); + MockSpawnChildProcess.mockReturnValue({ + child: mockChild as any, + request: mockRequest, + closeStderr: jest.fn() + } as any); const defaultCreators: any[] = [loremIpsum, dolorSitAmet, consecteturAdipiscingElit]; const globalOptions: any = { toolModules: filePackageToolModules, nodeVersion: 22, contextUrl: 'file:///test/path', contextPath: '/test/path' }; const sessionOptions: any = { sessionId }; + MockGetOptions.mockReturnValue(globalOptions); + await composeTools(defaultCreators, globalOptions, sessionOptions); - onceHandlers['disconnect'](); + if (onceHandlers['disconnect']) { + onceHandlers['disconnect'](); + } expect(mockChild.once).toHaveBeenCalledTimes(2); - expect(mockChild.stderr.on).toHaveBeenCalledWith('data', expect.any(Function)); - expect(mockChild.stderr.off).toHaveBeenCalledWith('data', expect.any(Function)); expect(mockChild.off).toHaveBeenCalledWith('exit', onceHandlers['exit']); expect(mockChild.off).toHaveBeenCalledWith('disconnect', onceHandlers['disconnect']); }); @@ -842,7 +730,7 @@ describe('composeTools', () => { const sessionId = 'test-session-id'; - MockSpawn.mockImplementationOnce(() => { + MockSpawnChildProcess.mockImplementationOnce(() => { throw new Error('Mock spawn failure'); }); diff --git a/src/__tests__/server.toolsHost.test.ts b/src/__tests__/server.toolsHost.test.ts index e2bfc9b6..d77879a3 100644 --- a/src/__tests__/server.toolsHost.test.ts +++ b/src/__tests__/server.toolsHost.test.ts @@ -1,14 +1,5 @@ import { z } from 'zod'; -import { - normalizeCreatorSchema, - requestHello, - requestLoad, - requestManifestGet, - requestInvoke, - requestShutdown, - requestFallback, - setHandlers -} from '../server.toolsHost'; +import { requestInvoke, createToolsHost, normalizeCreatorSchema } from '../server.toolsHost'; import { isZodSchema } from '../server.schema'; describe('normalizeCreatorSchema', () => { @@ -102,44 +93,6 @@ describe('normalizeCreatorSchema', () => { }); }); -describe('requestHello', () => { - let mockSend: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - process.send = mockSend; - }); - - afterEach(() => { - delete (process as any).send; - jest.clearAllMocks(); - }); - - it.each([ - { - description: 'with valid request', - request: { t: 'hello', id: 'test-id-1' } - }, - { - description: 'with different id', - request: { t: 'hello', id: 'test-id-2' } - } - ])('should send hello:ack message, $description', ({ request }) => { - requestHello(request as any); - - expect(mockSend).toHaveBeenCalledTimes(1); - expect(mockSend.mock.calls).toMatchSnapshot(); - }); - - it('should not throw when process.send is undefined', () => { - delete (process as any).send; - - expect(() => { - requestHello({ t: 'hello', id: 'test-id' }); - }).not.toThrow(); - }); -}); - describe('requestInvoke', () => { let mockSend: jest.Mock; @@ -262,13 +215,14 @@ describe('requestInvoke', () => { ] ); - const promise = requestInvoke(mockState as any, { t: 'invoke', id: 'request-id', toolId: requestToolId, args: { param: 'value' } }); + const ctx = { send: jest.fn() }; + const promise = requestInvoke(mockState as any, { t: 'invoke', id: 'request-id', toolId: requestToolId, args: { param: 'value' } }, ctx); await promise; - expect(mockSend.mock.calls.length).toBe(1); + expect(ctx.send.mock.calls.length).toBe(1); - const { error, ...rest } = mockSend.mock.calls[0][0]; + const { error, ...rest } = ctx.send.mock.calls[0][0]; expect({ ...((error?.message && { error: error?.message }) || undefined), @@ -308,7 +262,8 @@ describe('requestInvoke', () => { ] ); - const invokePromise = requestInvoke(mockState, { t: 'invoke', id: 'request-id', toolId: requestToolId, args: {} }); + const ctx = { send: jest.fn() }; + const invokePromise = requestInvoke(mockState, { t: 'invoke', id: 'request-id', toolId: requestToolId, args: {} }, ctx); // Wait for handler to be called, timeout to be set up await Promise.resolve(); @@ -320,307 +275,27 @@ describe('requestInvoke', () => { await Promise.resolve(); // Verify timeout message was sent - expect(mockSend.mock.calls).toMatchSnapshot(); + expect(ctx.send.mock.calls).toMatchSnapshot(); // Wait for the function to complete await invokePromise; - expect(mockSend).toHaveBeenCalledTimes(1); + expect(ctx.send).toHaveBeenCalledTimes(1); jest.useRealTimers(); }); }); -describe('requestLoad', () => { - let mockSend: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - process.send = mockSend; - }); - - afterEach(() => { - delete (process as any).send; - jest.clearAllMocks(); - }); - - it.each([ - { - description: 'with warnings and errors', - request: { t: 'load', id: 'test-id', specs: [] }, - warnings: ['warning1', 'warning2'], - errors: ['error1'] - }, - { - description: 'with empty warnings and errors', - request: { t: 'load', id: 'test-id', specs: [] }, - warnings: [], - errors: [] - }, - { - description: 'with only warnings', - request: { t: 'load', id: 'test-id', specs: [] }, - warnings: ['warning1'], - errors: [] - }, - { - description: 'with only errors', - request: { t: 'load', id: 'test-id', specs: [] }, - warnings: [], - errors: ['error1'] - }, - { - description: 'with undefined warnings and errors', - request: { t: 'load', id: 'test-id', specs: [] }, - warnings: undefined, - errors: undefined - } - ])('should send load:ack message, $description', ({ request, warnings, errors }) => { - const options: { warnings?: string[]; errors?: string[] } = {}; - - if (warnings !== undefined) { - options.warnings = warnings; - } - if (errors !== undefined) { - options.errors = errors; - } - requestLoad(request as any, options); - - expect(mockSend).toHaveBeenCalledTimes(1); - expect(mockSend.mock.calls).toMatchSnapshot(); - }); - - it('should not throw when process.send is undefined', () => { - delete (process as any).send; - - expect(() => { - requestLoad({ t: 'load', id: 'test-id', specs: [] }, {}); - }).not.toThrow(); - }); -}); - -describe('requestManifestGet', () => { - let mockSend: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - process.send = mockSend; - }); - - afterEach(() => { - delete (process as any).send; - jest.clearAllMocks(); - }); +describe('createToolsHost', () => { + it('should route load then reply with load:ack via the host', async () => { + const sendSpy = jest.fn(); - it.each([ - { - description: 'with empty descriptors', - state: { - toolMap: new Map(), - descriptors: [], - invokeTimeoutMs: 1000 - }, - request: { t: 'manifest:get', id: 'test-id' } - }, - { - description: 'with single tool descriptor', - state: { - toolMap: new Map(), - descriptors: [ - { - id: 'tool-1', - name: 'Tool1', - description: 'Description 1', - inputSchema: {}, - source: 'module1' - } - ], - invokeTimeoutMs: 1000 - }, - request: { t: 'manifest:get', id: 'test-id' } - }, - { - description: 'with multiple tool descriptors', - state: { - toolMap: new Map(), - descriptors: [ - { - id: 'tool-1', - name: 'Tool1', - description: 'Description 1', - inputSchema: { type: 'object' }, - source: 'module1' - }, - { - id: 'tool-2', - name: 'Tool2', - description: 'Description 2', - inputSchema: {}, - source: 'module2' - } - ], - invokeTimeoutMs: 1000 - }, - request: { t: 'manifest:get', id: 'test-id' } - } - ])('should send manifest:result message, $description', ({ state, request }) => { - requestManifestGet(state, request as any); - - expect(mockSend).toHaveBeenCalledTimes(1); - expect(mockSend.mock.calls).toMatchSnapshot(); - }); + (process as any).send = sendSpy; + const { bootstrapMessage } = createToolsHost(); - it('should not throw when process.send is undefined', () => { - const mockHostState = { - toolMap: new Map(), - descriptors: [], - invokeTimeoutMs: 1000 - }; - - delete (process as any).send; + await bootstrapMessage({ t: 'load', id: 'L1', specs: [] } as any); - expect(() => { - requestManifestGet(mockHostState, { t: 'manifest:get', id: 'test-id' }); - }).not.toThrow(); - }); -}); - -describe('requestShutdown', () => { - let mockSend: jest.Mock; - let mockExit: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - mockExit = jest.fn(); - process.send = mockSend; - process.exit = mockExit as any; - }); - - afterEach(() => { - delete (process as any).send; - delete (process as any).exit; - jest.clearAllMocks(); - }); - - it.each([ - { - description: 'with valid request', - request: { t: 'shutdown', id: 'test-id-1' } - }, - { - description: 'with different id', - request: { t: 'shutdown', id: 'test-id-2' } - } - ])('should send shutdown:ack and exit, $description', ({ request }) => { - requestShutdown(request as any); - - expect(mockSend).toHaveBeenCalledTimes(1); - expect(mockSend.mock.calls).toMatchSnapshot(); - expect(mockExit).toHaveBeenCalledTimes(1); - expect(mockExit).toHaveBeenCalledWith(0); - }); -}); - -describe('requestFallback', () => { - let mockSend: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - process.send = mockSend; - }); - - afterEach(() => { - delete (process as any).send; - jest.clearAllMocks(); - }); - - it.each([ - { - description: 'with request id', - request: { t: 'hello', id: 'test-id' }, - error: new Error('Test error') - }, - { - description: 'without request id', - request: { t: 'load', id: '', specs: [] }, - error: new Error('Test error') - }, - { - description: 'with string error', - request: { t: 'invoke', id: 'test-id', toolId: 'tool', args: {} }, - error: 'String error' - } - ])('should send error response, $description', ({ request, error }) => { - requestFallback(request as any, error as Error); - - expect(mockSend).toHaveBeenCalledTimes(1); - - const { error: err, ...rest } = mockSend.mock.calls[0][0]; - - expect({ - ...rest, - error: err?.message - }).toMatchSnapshot(); - }); - - it('should not throw when process.send is undefined', () => { - delete (process as any).send; - - expect(() => { - requestFallback({ t: 'hello', id: 'test-id' }, new Error('Test')); - }).not.toThrow(); - }); - - it('should not throw when send throws', () => { - mockSend.mockImplementation(() => { - throw new Error('Send failed'); - }); - - expect(() => { - requestFallback({ t: 'hello', id: 'test-id' }, new Error('Test')); - }).not.toThrow(); - }); -}); - -describe('setHandlers', () => { - let mockOn: jest.Mock; - let mockSend: jest.Mock; - - beforeEach(() => { - mockSend = jest.fn(); - mockOn = jest.fn(); - - process.on = mockOn; - process.send = mockSend; - }); - - afterEach(() => { - jest.clearAllMocks(); - delete (process as any).send; - }); - - it.each([ - { - description: 'hello', - request: { t: 'hello', id: 'test-id' } - }, - { - description: 'load', - request: { t: 'load', id: 'test-id' } - }, - { - description: 'manifest:get', - request: { t: 'manifest:get', id: 'test-id' } - }, - { - description: 'invoke', - request: { t: 'invoke', id: 'test-id' } - } - ])('should set up message handlers and attempt handle requests, $description', async ({ request }) => { - const handler = setHandlers(); - - await handler(request as any); - - expect(mockOn).toHaveBeenCalledWith('message', expect.any(Function)); - expect(mockSend.mock.calls).toMatchSnapshot(); + expect(sendSpy).toHaveBeenCalledWith( + expect.objectContaining({ t: 'load:ack', id: 'L1', warnings: [], errors: [] }) + ); }); }); diff --git a/src/__tests__/server.toolsIpc.test.ts b/src/__tests__/server.toolsIpc.test.ts index bdcabecc..50bc70f0 100644 --- a/src/__tests__/server.toolsIpc.test.ts +++ b/src/__tests__/server.toolsIpc.test.ts @@ -1,510 +1,13 @@ -import { type ChildProcess } from 'node:child_process'; -import { - send, - awaitIpc, - isHelloAck, - isLoadAck, - isManifestResult, - isInvokeResult, - type IpcRequest, - type IpcResponse -} from '../server.toolsIpc'; - -describe('send', () => { - let mockProcess: NodeJS.Process; - let mockChildProcess: ChildProcess; - - beforeEach(() => { - mockProcess = { - send: jest.fn().mockReturnValue(true) - } as any; - - mockChildProcess = { - send: jest.fn().mockReturnValue(true) - } as any; - }); - - it.each([ - { - description: 'hello request', - request: { t: 'hello', id: 'test-id' } - }, - { - description: 'load request', - request: { t: 'load', id: 'test-id', specs: ['module1', 'module2'] } - }, - { - description: 'load request with invokeTimeoutMs', - request: { t: 'load', id: 'test-id', specs: ['module1'], invokeTimeoutMs: 5000 } - }, - { - description: 'manifest:get request', - request: { t: 'manifest:get', id: 'test-id' } - }, - { - description: 'invoke request', - request: { t: 'invoke', id: 'test-id', toolId: 'tool1', args: { param: 'value' } } - }, - { - description: 'shutdown request', - request: { t: 'shutdown', id: 'test-id' } - } - ])('should send IPC message, $description', ({ request }) => { - const result = send(mockProcess, request as IpcRequest); - - expect(result).toBe(true); - expect(mockProcess.send).toHaveBeenCalledTimes(1); - expect(mockProcess.send).toHaveBeenCalledWith(request); - - const childResult = send(mockChildProcess, request as IpcRequest); - - expect(childResult).toBe(true); - expect(mockChildProcess.send).toHaveBeenCalledTimes(1); - expect(mockChildProcess.send).toHaveBeenCalledWith(request); - }); - - it.each([ - { - description: 'process without send', - process: {} - }, - { - description: 'process with send returning false', - process: { - send: jest.fn().mockReturnValue(false) - } - } - ])('should return false, $description', ({ process }) => { - const result = send(process as any, { t: 'hello', id: 'test-id' }); - - expect(result).toBe(false); - }); -}); - -describe('isHelloAck', () => { - it.each([ - { - description: 'valid hello:ack message', - message: { t: 'hello:ack', id: 'test-id' }, - expected: true - }, - { - description: 'invalid type', - message: { t: 'hello', id: 'test-id' }, - expected: false - }, - { - description: 'missing type', - message: { id: 'test-id' }, - expected: false - }, - { - description: 'missing id', - message: { t: 'hello:ack' }, - expected: false - }, - { - description: 'non-string id', - message: { t: 'hello:ack', id: 123 }, - expected: false - }, - { - description: 'null message', - message: null, - expected: false - }, - { - description: 'undefined message', - message: undefined, - expected: false - }, - { - description: 'empty object', - message: {}, - expected: false - } - ])('should check if message is hello:ack, $description', ({ message, expected }) => { - expect(isHelloAck(message)).toBe(expected); - }); -}); - -describe('isLoadAck', () => { - it.each([ - { - description: 'valid load:ack message with matching id', - message: { t: 'load:ack', id: 'test-id', warnings: [], errors: [] }, - expectedId: 'test-id', - expected: true - }, - { - description: 'valid load:ack with warnings and errors', - message: { t: 'load:ack', id: 'test-id', warnings: ['warning1'], errors: ['error1'] }, - expectedId: 'test-id', - expected: true - }, - { - description: 'mismatched id', - message: { t: 'load:ack', id: 'other-id', warnings: [], errors: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'invalid type', - message: { t: 'load', id: 'test-id', warnings: [], errors: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'missing warnings', - message: { t: 'load:ack', id: 'test-id', errors: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'missing errors', - message: { t: 'load:ack', id: 'test-id', warnings: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'non-array warnings', - message: { t: 'load:ack', id: 'test-id', warnings: 'not-array', errors: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'non-array errors', - message: { t: 'load:ack', id: 'test-id', warnings: [], errors: 'not-array' }, - expectedId: 'test-id', - expected: false - }, - { - description: 'null message', - message: null, - expectedId: 'test-id', - expected: false - }, - { - description: 'undefined message', - message: undefined, - expectedId: 'test-id', - expected: false - } - ])('should check if message is load:ack, $description', ({ message, expectedId, expected }) => { - const matcher = isLoadAck(expectedId); - - expect(matcher(message)).toBe(expected); - }); -}); - -describe('isManifestResult', () => { - it.each([ - { - description: 'valid manifest:result with matching id', - message: { t: 'manifest:result', id: 'test-id', tools: [] }, - expectedId: 'test-id', - expected: true - }, - { - description: 'valid manifest:result with tools', - message: { - t: 'manifest:result', - id: 'test-id', - tools: [ - { id: 'tool1', name: 'Tool1', description: 'Description', inputSchema: {} } - ] - }, - expectedId: 'test-id', - expected: true - }, - { - description: 'mismatched id', - message: { t: 'manifest:result', id: 'other-id', tools: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'invalid type', - message: { t: 'manifest', id: 'test-id', tools: [] }, - expectedId: 'test-id', - expected: false - }, - { - description: 'missing tools', - message: { t: 'manifest:result', id: 'test-id' }, - expectedId: 'test-id', - expected: false - }, - { - description: 'non-array tools', - message: { t: 'manifest:result', id: 'test-id', tools: 'not-array' }, - expectedId: 'test-id', - expected: false - }, - { - description: 'null message', - message: null, - expectedId: 'test-id', - expected: false - }, - { - description: 'undefined message', - message: undefined, - expectedId: 'test-id', - expected: false - } - ])('should check if message is manifest:result, $description', ({ message, expectedId, expected }) => { - const matcher = isManifestResult(expectedId); - - expect(matcher(message)).toBe(expected); - }); -}); - -describe('isInvokeResult', () => { - it.each([ - { - description: 'valid invoke:result with ok:true and matching id', - message: { t: 'invoke:result', id: 'test-id', ok: true, result: { data: 'value' } }, - expectedId: 'test-id', - expected: true - }, - { - description: 'valid invoke:result with ok:false and error', - message: { - t: 'invoke:result', - id: 'test-id', - ok: false, - error: { message: 'Error message', stack: 'stack trace', code: 'ERROR_CODE' } - }, - expectedId: 'test-id', - expected: true - }, - { - description: 'mismatched id', - message: { t: 'invoke:result', id: 'other-id', ok: true, result: {} }, - expectedId: 'test-id', - expected: false - }, - { - description: 'invalid type', - message: { t: 'invoke', id: 'test-id', ok: true, result: {} }, - expectedId: 'test-id', - expected: false - }, - { - description: 'missing id', - message: { t: 'invoke:result', ok: true, result: {} }, - expectedId: 'test-id', - expected: false - }, - { - description: 'null message', - message: null, - expectedId: 'test-id', - expected: false - }, - { - description: 'undefined message', - message: undefined, - expectedId: 'test-id', - expected: false - } - ])('should check if message is invoke:result, $description', ({ message, expectedId, expected }) => { - const matcher = isInvokeResult(expectedId); - - expect(matcher(message)).toBe(expected); - }); -}); - -describe('awaitIpc', () => { - let mockProcess: NodeJS.Process; - let messageHandlers: Array<(message: any) => void>; - let exitHandlers: Array<(code?: number, signal?: string) => void>; - let disconnectHandlers: Array<() => void>; - - beforeEach(() => { - messageHandlers = []; - exitHandlers = []; - disconnectHandlers = []; - - mockProcess = { - on: jest.fn((event: string, handler: any) => { - switch (event) { - case 'message': - messageHandlers.push(handler); - break; - case 'exit': - exitHandlers.push(handler); - break; - case 'disconnect': - disconnectHandlers.push(handler); - break; - } - - return mockProcess; - }), - off: jest.fn((event: string, handler: any) => { - switch (event) { - case 'message': { - const index = messageHandlers.indexOf(handler); - - if (index > -1) { - messageHandlers.splice(index, 1); - } - break; - } - case 'exit': { - const index = exitHandlers.indexOf(handler); - - if (index > -1) { - exitHandlers.splice(index, 1); - } - break; - } - case 'disconnect': { - const index = disconnectHandlers.indexOf(handler); - - if (index > -1) { - disconnectHandlers.splice(index, 1); - } - break; - } - } - - return mockProcess; - }) - } as any; - }); - - afterEach(() => { - jest.clearAllMocks(); - }); +import { send, awaitIpc, makeId, matchResponse, serializeError } from '../server.toolsIpc'; +describe('server.toolsIpc re-exports', () => { it.each([ - { - description: 'hello:ack message', - response: { t: 'hello:ack', id: 'test-id' } - }, - { - description: 'load:ack message', - response: { t: 'load:ack', id: 'test-id', warnings: [], errors: [] } - }, - { - description: 'manifest:result message', - response: { t: 'manifest:result', id: 'test-id', tools: [] } - }, - { - description: 'invoke:result with ok:true', - response: { t: 'invoke:result', id: 'test-id', ok: true, result: { data: 'value' } } - }, - { - description: 'invoke:result with ok:false', - response: { t: 'invoke:result', id: 'test-id', ok: false, error: { message: 'Error' } } - } - ])('should await and resolve IPC response, $description', async ({ response }) => { - let promise: Promise; - - switch (response.t) { - case 'hello:ack': - promise = awaitIpc(mockProcess, isHelloAck, 1000); - break; - case 'load:ack': - promise = awaitIpc(mockProcess, isLoadAck(response.id), 1000); - break; - case 'manifest:result': - promise = awaitIpc(mockProcess, isManifestResult(response.id), 1000); - break; - default: - promise = awaitIpc(mockProcess, isInvokeResult(response.id), 1000); - break; - } - - // Simulate message arrival, wait for handlers to be registered - await Promise.resolve(); - messageHandlers.forEach(handler => handler(response)); - - const result = await promise; - - expect(result).toEqual(response); - expect(mockProcess.on).toHaveBeenCalledWith('message', expect.any(Function)); - expect(mockProcess.on).toHaveBeenCalledWith('exit', expect.any(Function)); - expect(mockProcess.on).toHaveBeenCalledWith('disconnect', expect.any(Function)); - }); - - it('should ignore non-matching messages and only resolve once', async () => { - const responseOne = { t: 'hello:ack', id: 'test-id-1' }; - const responseTwo = { t: 'other:type', id: 'test-id-2' }; - const responseThree = { t: 'hello:ack', id: 'test-id-2' }; - - const promise = awaitIpc(mockProcess, isHelloAck, 1000); - - // Simulate message arrival, wait for handlers to be registered - await Promise.resolve(); - messageHandlers.forEach(handler => handler(responseTwo)); - - await Promise.resolve(); - messageHandlers.forEach(handler => handler(responseOne)); - - await Promise.resolve(); - messageHandlers.forEach(handler => handler(responseThree)); - - const result = await promise; - - expect(result).toEqual(responseOne); - }); - - it('should reject when process exits', async () => { - const exit = { event: 'exit', code: 1, signal: 'SIGTERM' }; - - const promise = awaitIpc(mockProcess, isHelloAck, 1000); - - // Simulate message arrival, wait for handlers to be registered - await Promise.resolve(); - exitHandlers.forEach(handler => handler(exit.code, exit.signal)); - - await expect(promise).rejects.toThrow('Tools Host exited before response'); - }); - - it('should reject on timeout', async () => { - jest.useFakeTimers(); - const promise = awaitIpc(mockProcess, isHelloAck, 1000); - - jest.advanceTimersByTime(1001); - - await expect(promise).rejects.toThrow('Timed out waiting for IPC response'); - jest.useRealTimers(); - }); - - it('should cleanup event listeners on resolve', async () => { - const response = { t: 'hello:ack', id: 'test-id' }; - const promise = awaitIpc(mockProcess, isHelloAck, 1000); - - // Simulate message arrival, wait for handlers to be registered - await Promise.resolve(); - messageHandlers.forEach(handler => handler(response)); - - await promise; - - expect(mockProcess.off).toHaveBeenCalledWith('message', expect.any(Function)); - expect(mockProcess.off).toHaveBeenCalledWith('exit', expect.any(Function)); - expect(mockProcess.off).toHaveBeenCalledWith('disconnect', expect.any(Function)); - }); - - it('should cleanup event listeners on reject', async () => { - jest.useFakeTimers(); - const promise = awaitIpc(mockProcess, isHelloAck, 1000); - - jest.advanceTimersByTime(1001); - - try { - await promise; - } catch { - // Expected to reject - } - jest.useRealTimers(); - - expect(mockProcess.off).toHaveBeenCalledWith('message', expect.any(Function)); - expect(mockProcess.off).toHaveBeenCalledWith('exit', expect.any(Function)); - expect(mockProcess.off).toHaveBeenCalledWith('disconnect', expect.any(Function)); + ['send', send], + ['awaitIpc', awaitIpc], + ['makeId', makeId], + ['matchResponse', matchResponse], + ['serializeError', serializeError] + ])('should re-export %s from server.processIpc', (_name, fn) => { + expect(typeof fn).toBe('function'); }); }); diff --git a/src/server.process.ts b/src/server.process.ts new file mode 100644 index 00000000..d9c3762f --- /dev/null +++ b/src/server.process.ts @@ -0,0 +1,322 @@ +import { spawn, type ChildProcess } from 'node:child_process'; +import { realpathSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname } from 'node:path'; +import { log, formatUnknownError } from './logger'; +import { + awaitIpc, + send, + makeId, + matchResponse, + type ProcessRequest, + type ProcessResponse, + type SerializedError +} from './server.processIpc'; + +/** + * Isolation options for a spawned child process. + * + * @property mode - 'strict' enables Node permission model; 'none' disables it. + * @property nodeVersion - Detected Node major version (selects the permission flag). + * @property fsReadAllowlist - Directories to allow read access under strict mode. + */ +type IsolationOptions = { + mode?: 'strict' | 'none'; + nodeVersion?: number; + fsReadAllowlist?: string[]; +}; + +/** + * Configuration for spawning a child process. + * + * @property importSpecifier - Import map specifier for the child entry (e.g. `#toolsHost`). + * @property entry - Pre-resolved absolute entry path (overrides `importSpecifier`). + * @property isolation - Node isolation options. + * @property label - Human-readable label used in error/log messages. + * @property enableStderrDebug - Optional stderr reader factory; returns a `closeStderr`. + */ +type SpawnConfig = { + importSpecifier: string; + entry?: string; + isolation?: IsolationOptions; + label?: string; + enableStderrDebug?: (child: ChildProcess) => () => void; +}; + +/** + * Handle for a spawned child process. + * + * @property child - The child process. + * @property closeStderr - Detach the stderr debug reader. + * @property request - Typed round-trip: send a request and await a matching response. + */ +type ChildHandle = { + child: ChildProcess; + closeStderr: () => void; + request: ( + req: Omit & { id?: string } & Record, + responseType: string, + timeoutMs: number, + correlate?: boolean + ) => Promise; +}; + +/** + * Generic per-session registry of live child handles. + */ +const activeChildrenBySession = new Map(); + +/** + * Resolve a child entry path from an import specifier or a pre-resolved path. + * + * @param {SpawnConfig} config + * @throws {Error} If the entry cannot be resolved. + */ +const resolveEntry = ({ importSpecifier, entry, label = 'child process' }: SpawnConfig): string => { + if (entry) { + return entry; + } + + let resolved: string | undefined = undefined; + + try { + resolved = fileURLToPath(import.meta.resolve(importSpecifier)); + } catch (error) { + log.debug(`Failed to import.meta.resolve ${label} entry '${importSpecifier}': ${formatUnknownError(error)}`); + + if (process.env.NODE_ENV === 'local') { + resolved = '/mock/path/to/host.js'; + } + } + + if (resolved === undefined) { + throw new Error(`Failed to resolve ${label} entry '${importSpecifier}'.`); + } + + return resolved; +}; + +/** + * Build Node isolation flags for a strict-mode child process. + * + * @param entry - Resolved child entry path (its dir is auto-added to the allowlist). + * @param {IsolationOptions} isolation + * @returns Array of Node CLI args. + */ +const buildIsolationArgs = (entry: string, isolation: IsolationOptions = {}): string[] => { + const { mode, nodeVersion = 0, fsReadAllowlist = [] } = isolation; + + if (mode !== 'strict') { + return []; + } + + // Node 24+ moves to using the "--permission" flag instead of "--experimental-permission" + const permissionFlag = nodeVersion >= 24 ? '--permission' : '--experimental-permission'; + const nodeArgs: string[] = [permissionFlag]; + + const allowSet = new Set(fsReadAllowlist); + + allowSet.add(dirname(entry)); + + // Normalize to real absolute paths to avoid symlink mismatches + const allowList = [...allowSet] + .map(dir => { + try { + return realpathSync(dir); + } catch { + return dir; + } + }) + .filter(Boolean); + + for (const dir of allowList) { + nodeArgs.push(`--allow-fs-read=${dir}`); + } + + log.debug(`Child allow-fs-read flags: ${allowList.map(dir => `--allow-fs-read=${dir}`).join(' ')}`); + log.debug(`Child permission flag: ${permissionFlag}`); + + return nodeArgs; +}; + +/** + * Spawn a child process with the standard IPC-capable stdio shape and return a handle. + * + * @param {SpawnConfig} config + * @returns {ChildHandle} + */ +const spawnChildProcess = (config: SpawnConfig): ChildHandle => { + const entry = resolveEntry(config); + const nodeArgs = buildIsolationArgs(entry, config.isolation); + + const child: ChildProcess = spawn(process.execPath, [...nodeArgs, entry], { + stdio: ['ignore', 'pipe', 'pipe', 'ipc'] + }); + + const closeStderr = config.enableStderrDebug?.(child) ?? (() => {}); + + const request = ( + req: Omit & { id?: string } & Record, + responseType: string, + timeoutMs: number, + correlate = true + ): Promise => { + const id = req.id ?? makeId(); + const errorType = `${req.t}:error`; + const matchOk = matchResponse(responseType, correlate ? id : undefined); + const matchErr = matchResponse(errorType, correlate ? id : undefined); + + // Resolve on the expected response OR the correlated `:error` envelope + // emitted by the generic host's requestFallback, so a thrown handler rejects + // promptly rather than waiting for the timeout. + const matcher = (message: any): message is T => matchOk(message) || matchErr(message); + + const pending = awaitIpc(child, matcher, timeoutMs).then(message => { + if ((message as ProcessResponse)?.t === errorType) { + const errorValue = (message as { error?: SerializedError })?.error; + const settledError = new Error(errorValue?.message || 'Child process handler error', { + cause: errorValue?.cause + }) as Error & { code?: string; details?: unknown }; + + if (errorValue?.stack) { + settledError.stack = errorValue.stack; + } + + if (errorValue?.code) { + settledError.code = errorValue.code; + } + + if (errorValue?.details) { + settledError.details = errorValue.details; + } + + throw settledError; + } + + return message; + }); + + send(child, { ...req, id } as ProcessRequest); + + return pending; + }; + + return { child, closeStderr, request }; +}; + +/** + * Best-effort graceful shutdown of a child process with primary/secondary SIGKILL fallback. + * + * @param {ChildHandle} handle - The child handle to shut down. + * @param options - Shutdown options. + * @param options.gracePeriodMs - Primary grace period before force-kill. + * @param options.sessionId - Optional session id to clean from the registry. + * @param options.label - Human-readable label for logs. + */ +const shutdownChildProcess = async ( + handle: ChildHandle | undefined, + { gracePeriodMs = 0, sessionId, label = 'child process' }: { + gracePeriodMs?: number; sessionId?: string; label?: string; + } = {} +): Promise => { + if (!handle) { + return; + } + + const grace = Math.max(0, Number(gracePeriodMs) || 0); + const fallbackGrace = grace + 200; + const child = handle.child; + + let resolved = false; + let forceKillPrimary: NodeJS.Timeout | undefined; + let forceKillSecondary: NodeJS.Timeout | undefined; + let resolveIt: ((value: PromiseLike | void) => void) | undefined; + + const shutdownChild = () => { + if (resolved) { + return; + } + + resolved = true; + child.off('exit', shutdownChild); + child.off('disconnect', shutdownChild); + + if (forceKillPrimary) { + clearTimeout(forceKillPrimary); + } + + if (forceKillSecondary) { + clearTimeout(forceKillSecondary); + } + + try { + handle.closeStderr(); + log.info(`${label} stderr reader closed.`); + } catch (error) { + log.error(`Failed to close ${label} stderr reader: ${formatUnknownError(error)}`); + } + + if (sessionId !== undefined) { + const confirmHandle = activeChildrenBySession.get(sessionId); + + if (confirmHandle?.child === child) { + activeChildrenBySession.delete(sessionId); + } + } + + resolveIt?.(); + }; + + const sigkillChild = (isSecondaryFallback = false) => { + try { + if (!child?.killed) { + log.warn( + `${ + (resolved && 'Already attempted shutdown.') || 'Slow shutdown response.' + } ${ + (isSecondaryFallback && 'Secondary') || 'Primary' + } fallback force-killing ${label}.` + ); + child.kill('SIGKILL'); + } + } catch (error) { + log.error(`Failed to force-kill ${label}: ${formatUnknownError(error)}`); + } + }; + + await new Promise(resolve => { + resolveIt = resolve; + + try { + send(child, { t: 'shutdown', id: makeId() }); + } catch (error) { + log.error(`Failed to send shutdown signal to ${label}: ${formatUnknownError(error)}`); + } + + forceKillPrimary = setTimeout(() => { + sigkillChild(); + shutdownChild(); + }, grace); + forceKillPrimary?.unref?.(); + + forceKillSecondary = setTimeout(() => { + sigkillChild(true); + shutdownChild(); + }, fallbackGrace); + forceKillSecondary?.unref?.(); + + child.once('exit', shutdownChild); + child.once('disconnect', shutdownChild); + }); +}; + +export { + spawnChildProcess, + shutdownChildProcess, + buildIsolationArgs, + resolveEntry, + activeChildrenBySession, + type SpawnConfig, + type IsolationOptions, + type ChildHandle +}; diff --git a/src/server.processHost.ts b/src/server.processHost.ts new file mode 100644 index 00000000..22c07e3c --- /dev/null +++ b/src/server.processHost.ts @@ -0,0 +1,176 @@ +import { type ProcessRequest, serializeError } from './server.processIpc'; + +/** + * Context passed to every handler, providing a bound reply helper. + * + * @property send - Send a response back over the IPC channel (no-op if unavailable). + */ +type HostContext = { + send: (response: Record) => void; +}; + +/** + * Map of message type to handler. `hello` is provided by default and may be overridden. + */ +type HostHandlers = { + [messageType: string]: (request: ProcessRequest, ctx: HostContext) => void | Promise; +}; + +/** + * Reply helper bound to `process.send`. + * + * @param response + */ +const reply = (response: Record) => { + process.send?.(response); +}; + +/** + * Fallback handler for unhandled errors. Attempts a best-effort structured + * error message back over the IPC channel and fails silently otherwise. + * + * @param {ProcessRequest} request - Original IPC request object. + * @param {Error} error - Failed request error object. + */ +const requestFallback = (request: ProcessRequest, error: Error) => { + try { + process.send?.({ + t: `${request?.t || 'unknown'}:error`, + id: request?.id || 'n/a', + ok: false, + error: serializeError(error) + }); + } catch {} +}; + +/** + * Default `hello` handler. Acknowledges the handshake. + * + * @param {ProcessRequest} request + * @param {HostContext} ctx + */ +const helloHandler = (request: ProcessRequest, ctx: HostContext) => { + ctx.send({ t: 'hello:ack', id: request.id }); +}; + +/** + * Default `shutdown` handler. Acknowledges then exits the child gracefully. + * + * Mirrors the cooperative shutdown in `server.toolsHost.ts` so the parent's + * `shutdownChildProcess` resolves on a clean exit rather than the kill fallback. + * + * @param {ProcessRequest} request + * @param {HostContext} ctx + */ +const shutdownHandler = (request: ProcessRequest, ctx: HostContext) => { + ctx.send({ t: 'shutdown:ack', id: request.id }); + process.exit(0); +}; + +/** + * Install the message router and disconnect handler for a child process. + * + * Routes each request to the matching handler in `handlers`. Built-in `hello` + * and `shutdown` handlers are provided and can be overridden. Errors are passed + * to `requestFallback`. + * + * @param {HostHandlers} handlers - Map of message type to handler. + * @returns The router function (used by the one-shot bootstrap). + */ +const setHandlers = (handlers: HostHandlers) => { + const routes: HostHandlers = { hello: helloHandler, shutdown: shutdownHandler, ...handlers }; + const ctx: HostContext = { send: reply }; + + /** + * Handle incoming IPC messages by looking up the handler for the message type. + * + * @param {ProcessRequest} request + */ + const handlerMessage = async (request: ProcessRequest) => { + try { + const handler = routes[request?.t]; + + if (handler) { + await handler(request, ctx); + } + } catch (error) { + requestFallback(request, error as Error); + } + }; + + process.on('message', handlerMessage); + + /** + * Handle process disconnects. + */ + const handlerDisconnect = () => { + process.exit(0); + }; + + process.on('disconnect', handlerDisconnect); + + // Expose the router for bootstrapping. + return handlerMessage; +}; + +/** + * The currently-attached one-shot bootstrap listener, if any. + * + * Tracked at module scope so repeated `createProcessHost` calls detach the prior + * bootstrap before attaching a new one, keeping a single `message` listener and + * avoiding `MaxListenersExceeded` accumulation (e.g. under repeated instantiation in tests). + */ +let activeBootstrap: ((first: ProcessRequest) => void) | undefined; + +/** + * Create a generic child-process host with a one-shot bootstrap. + * + * The first message removes the bootstrap listener, installs the real handlers, + * and re-routes the first message through the same code path. + * + * @param {HostHandlers} handlers - Map of message type to handler. + */ +const createProcessHost = (handlers: HostHandlers) => { + /** + * Lazy initialize IPC handlers. One-shot: detach then install real handlers. + * + * @param {ProcessRequest} first + */ + const bootstrapMessage = (first: ProcessRequest) => { + // Detach bootstrap to avoid duplicate delivery + process.off('message', bootstrapMessage); + activeBootstrap = undefined; + + // Install real handlers and get a reference to the router + const route = setHandlers(handlers); + + // Route the very first message through the same code path + void route(first); + }; + + if (process.send) { + // Detach any previously-attached bootstrap so repeated instantiation + // never accumulates duplicate `message` listeners. + if (activeBootstrap) { + process.off('message', activeBootstrap); + } + + activeBootstrap = bootstrapMessage; + process.on('message', bootstrapMessage); + } + + // Return only the one-shot bootstrap. `setHandlers` is a standalone export and is + // invoked from within `bootstrapMessage`; exposing a second re-attaching wrapper + // here would double-register the message/disconnect listeners. + return { bootstrapMessage }; +}; + +export { + createProcessHost, + setHandlers, + requestFallback, + helloHandler, + shutdownHandler, + type HostHandlers, + type HostContext +}; diff --git a/src/server.processIpc.ts b/src/server.processIpc.ts new file mode 100644 index 00000000..81cec90c --- /dev/null +++ b/src/server.processIpc.ts @@ -0,0 +1,173 @@ +import { type ChildProcess } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; + +/** + * Base IPC (Inter-Process Communication) request envelope. + * + * Every request carries a message type and a correlation id. Concrete protocols + * intersect this with their own fields, e.g. `ProcessRequest & { specs: string[] }`. + * + * @property t - Message type. + * @property id - Message identifier. + */ +type ProcessRequest = { t: string; id: string }; + +/** + * Base IPC (Inter-Process Communication) response envelope. + * + * @property t - Message type. + * @property id - Message identifier. + */ +type ProcessResponse = { t: string; id: string }; + +/** + * Serialized error object for IPC. + * + * @property message - Error message. + * @property stack - Error stack trace. + * @property code - Error code. + * @property cause - Error cause. + * @property details - Additional details. + */ +type SerializedError = { + message: string; + stack?: string | undefined; + code?: string | undefined; + cause?: unknown; + details?: unknown +}; + +/** + * Generate a unique ID for IPC messages. + */ +const makeId = () => randomUUID(); + +/** + * Send an IPC message to the provided process. + * + * @param processRef + * @param {ProcessRequest} request + */ +const send = ( + processRef: NodeJS.Process | ChildProcess, + request: ProcessRequest +): boolean => Boolean(processRef.send?.(request)); + +/** + * Serialize an error value into a structured object. + * + * @param errorValue - Error-like value to serialize. + * @returns {SerializedError} - Serialized error object. + */ +const serializeError = (errorValue: unknown): SerializedError => { + const err = errorValue as SerializedError | undefined; + + return { + message: err?.message || String(errorValue), + stack: err?.stack, + code: err?.code, + details: err?.details, + cause: err?.cause + }; +}; + +/** + * Build a type guard that matches a response by message type and, optionally, id. + * + * Replaces the hand-written `isXAck` factories with a single generic matcher. + * + * @param type - Expected `t` message type. + * @param expectedId - Optional identifier to match against the message `id` field. + * @returns Function that determines if a message matches the type (and id). + */ +const matchResponse = (type: string, expectedId?: string) => + (message: any): message is T => { + if (!message || message.t !== type) { + return false; + } + + if (expectedId !== undefined) { + return message.id === expectedId; + } + + return typeof message.id === 'string'; + }; + +/** + * Await an IPC response from the provided process. + * + * Resolves on the first message that satisfies `matcher`. Rejects on early + * `exit`/`disconnect` or timeout. Always cleans up listeners and timers. + * + * @param processRef + * @param matcher + * @param timeoutMs + */ +const awaitIpc = ( + processRef: NodeJS.Process | ChildProcess, + matcher: (message: any) => message is T, + timeoutMs: number +): Promise => new Promise((resolve, reject) => { + let settled = false; + + // Cleanup listeners and timers on exit or timeout + const cleanup = () => { + processRef.off('message', onMessage); + processRef.off('exit', onExit); + processRef.off('disconnect', onExit); + clearTimeout(timerId); + }; + + // Listen for messages and resolve on match + const onMessage = (message: any) => { + if (settled) { + return; + } + + if (matcher(message)) { + settled = true; + cleanup(); + resolve(message); + } + }; + + // Reject on exit or disconnect + const onExit = (code?: number, signal?: string) => { + if (settled) { + return; + } + + settled = true; + cleanup(); + reject(new Error(`Child process exited before response (code=${code}, signal=${signal || 'none'})`)); + }; + + // Set a timeout to reject if the process doesn't respond + const timerId = setTimeout(() => { + if (settled) { + return; + } + + settled = true; + cleanup(); + reject(new Error('Timed out waiting for IPC response')); + }, timeoutMs); + + timerId?.unref?.(); + + // Attach listeners to the process + processRef.on('message', onMessage); + processRef.on('exit', onExit); + processRef.on('disconnect', onExit); +}); + +export { + send, + awaitIpc, + makeId, + matchResponse, + serializeError, + type ProcessRequest, + type ProcessResponse, + type SerializedError +}; diff --git a/src/server.tools.ts b/src/server.tools.ts index 378c02be..7c5eb42d 100644 --- a/src/server.tools.ts +++ b/src/server.tools.ts @@ -1,21 +1,15 @@ -import { spawn, type ChildProcess } from 'node:child_process'; -import { realpathSync } from 'node:fs'; -import { fileURLToPath } from 'node:url'; -import { dirname } from 'node:path'; +import { type ChildProcess } from 'node:child_process'; import { z } from 'zod'; import { type AppSession, type GlobalOptions } from './options'; import { type McpToolCreator } from './mcpSdk'; import { log, formatUnknownError } from './logger'; +import { type ToolDescriptor, type IpcResponse } from './server.toolsIpc'; import { - awaitIpc, - send, - makeId, - isHelloAck, - isLoadAck, - isManifestResult, - isInvokeResult, - type ToolDescriptor -} from './server.toolsIpc'; + spawnChildProcess, + shutdownChildProcess, + activeChildrenBySession, + type ChildHandle +} from './server.process'; import { getOptions, getSessionOptions } from './options.context'; import { setToolOptions } from './options.tools'; import { normalizeTools, sanitizeStaticToolName, type NormalizedToolEntry } from './server.toolsUser'; @@ -24,21 +18,12 @@ import { jsonSchemaToZod, normalizeInputSchema } from './server.schema'; /** * Handle for a spawned Tools Host process. * - * @property child - Child process * @property tools - Array of tool descriptors from `tools/list` - * @property closeStderr - Optional function to close stderr reader */ -type HostHandle = { - child: ChildProcess; +type HostHandle = ChildHandle & { tools: ToolDescriptor[]; - closeStderr?: () => void; }; -/** - * Map of active Tools Hosts per session. - */ -const activeHostsBySession = new Map(); - /** * Get a set of tool names from the builtin creators. * @@ -234,92 +219,44 @@ const debugChild = (child: ChildProcess, { sessionId } = getSessionOptions()) => const spawnToolsHost = async ( options: GlobalOptions = getOptions() ): Promise => { - const { nodeVersion, pluginIsolation, pluginHost } = options || {}; + const { pluginIsolation, pluginHost, nodeVersion } = options || {}; const { loadTimeoutMs, invokeTimeoutMs } = pluginHost || {}; - const nodeArgs: string[] = []; - let updatedEntry: string | undefined = undefined; - - try { - const entryUrl = import.meta.resolve('#toolsHost'); - - updatedEntry = fileURLToPath(entryUrl); - } catch (error) { - log.debug(`Failed to import.meta.resolve Tools Host entry '#toolsHost': ${formatUnknownError(error)}`); - - if (process.env.NODE_ENV === 'local') { - updatedEntry = '/mock/path/to/toolsHost.js'; - } - } - - if (updatedEntry === undefined) { - throw new Error(`Failed to resolve Tools Host entry '#toolsHost'.`); - } - - // Deny network and fs write by omission - if (pluginIsolation === 'strict') { - // Node 24+ moves to using the "--permission" flag instead of "--experimental-permission" - const permissionFlag = nodeVersion >= 24 ? '--permission' : '--experimental-permission'; - - nodeArgs.push(permissionFlag); - - // 1) Gather directories (project, plugin modules, and the host entry's dir) - const allowSet = new Set(computeFsReadAllowlist()); - - allowSet.add(dirname(updatedEntry)); - - // 2) Normalize to real absolute paths to avoid symlink mismatches - // Using top-level import instead of dynamic import for better performance - const allowList = [...allowSet] - .map(dir => { - try { - return realpathSync(dir); - } catch { - return dir; - } - }) - .filter(Boolean); - - // 3) Pass one --allow-fs-read per directory (more robust than a single comma-separated flag) - for (const dir of allowList) { - nodeArgs.push(`--allow-fs-read=${dir}`); - } - - // Optional debug to verify exactly what the child gets - log.debug(`Tools Host allow-fs-read flags: ${allowList.map(dir => `--allow-fs-read=${dir}`).join(' ')}`); - log.debug(`Tools Host permission flag: ${permissionFlag}`); - } // Pre-compute file and package tool modules before spawning to reduce latency const filePackageToolModules = getFilePackageToolModules() || []; + const toolOptions = setToolOptions(options); - const child: ChildProcess = spawn(process.execPath, [...nodeArgs, updatedEntry], { - stdio: ['ignore', 'pipe', 'pipe', 'ipc'] + const handle = spawnChildProcess({ + importSpecifier: '#toolsHost', + label: 'Tools Host', + isolation: { + mode: pluginIsolation === 'strict' ? 'strict' : 'none', + nodeVersion, + fsReadAllowlist: computeFsReadAllowlist() + }, + enableStderrDebug: child => debugChild(child) }); - const closeStderr = debugChild(child); - // hello - send(child, { t: 'hello', id: makeId() }); - await awaitIpc(child, isHelloAck, loadTimeoutMs); + await handle.request({ t: 'hello' }, 'hello:ack', loadTimeoutMs); // load - const loadId = makeId(); - - // Pass a focused set of tool options to the host. Avoid the full options object. - const toolOptions = setToolOptions(options); - - send(child, { t: 'load', id: loadId, specs: filePackageToolModules, invokeTimeoutMs, toolOptions }); - const loadAck = await awaitIpc(child, isLoadAck(loadId), loadTimeoutMs); + const loadAck = await handle.request>( + { t: 'load', specs: filePackageToolModules, invokeTimeoutMs, toolOptions }, + 'load:ack', + loadTimeoutMs + ); logWarningsErrors(loadAck); // manifest - const manifestRequestId = makeId(); - - send(child, { t: 'manifest:get', id: manifestRequestId }); - const manifest = await awaitIpc(child, isManifestResult(manifestRequestId), loadTimeoutMs); + const manifest = await handle.request>( + { t: 'manifest:get' }, + 'manifest:result', + loadTimeoutMs + ); - return { child, tools: manifest.tools as ToolDescriptor[], closeStderr }; + return { ...handle, tools: manifest.tools as ToolDescriptor[] }; }; /** @@ -377,13 +314,9 @@ const makeProxyCreators = ( }; const handler = async (args: unknown) => { - const requestId = makeId(); - - send(handle.child, { t: 'invoke', id: requestId, toolId: tool.id, args }); - - const response = await awaitIpc( - handle.child, - isInvokeResult(requestId), + const response = await handle.request>( + { t: 'invoke', toolId: tool.id, args }, + 'invoke:result', invokeTimeoutMs ); @@ -401,7 +334,9 @@ const makeProxyCreators = ( invocationError.code = response.error?.code; } - invocationError.details = response.error?.details || (response as any).error?.cause?.details; + const errorCause = response.error?.cause as { details?: unknown } | undefined; + + invocationError.details = response.error?.details || errorCause?.details; throw invocationError; } @@ -426,101 +361,12 @@ const sendToolsHostShutdown = async ( { pluginHost }: GlobalOptions = getOptions(), { sessionId }: AppSession = getSessionOptions() ): Promise => { - const handle = activeHostsBySession.get(sessionId); - - if (!handle) { - return; - } - - const gracePeriodMs = Math.max(0, Number(pluginHost?.gracePeriodMs) || 0); - const fallbackGracePeriodMs = gracePeriodMs + 200; - - const child = handle.child; - let resolved = false; - let forceKillPrimary: NodeJS.Timeout | undefined; - let forceKillSecondary: NodeJS.Timeout | undefined; - let resolveIt: ((value: PromiseLike | void) => void) | undefined; - - // Attempt exit, disconnect, then remove from activeHostsBySession and finally resolve - const shutdownChild = () => { - if (resolved) { - return; - } - - resolved = true; - child.off('exit', shutdownChild); - child.off('disconnect', shutdownChild); - - if (forceKillPrimary) { - clearTimeout(forceKillPrimary); - } - - if (forceKillSecondary) { - clearTimeout(forceKillSecondary); - } - - try { - (handle as any).closeStderr(); - log.info('Tools Host stderr reader closed.'); - } catch (error) { - log.error(`Failed to close Tools Host stderr reader: ${formatUnknownError(error)}`); - } - - const confirmHandle = activeHostsBySession.get(sessionId); - - if (confirmHandle?.child === child) { - activeHostsBySession.delete(sessionId); - } - - resolveIt?.(); - }; - - // Forced shutdown. - const sigkillChild = (isSecondaryFallback: boolean = false) => { - try { - if (!child?.killed) { - log.warn( - `${ - (resolved && 'Already attempted shutdown.') || 'Slow shutdown response.' - } ${ - (isSecondaryFallback && 'Secondary') || 'Primary' - } fallback force-killing Tools Host child process.` - ); - child.kill('SIGKILL'); - } - } catch (error) { - log.error(`Failed to force-kill Tools Host child process: ${formatUnknownError(error)}`); - } - }; - - // Start the shutdown process - await new Promise(resolve => { - resolveIt = resolve; - // Send a shutdown signal to child. We try/catch in case the process is already dead, and - // since we're still following it up with a graceful shutdown, then force-kill. - try { - send(child, { t: 'shutdown', id: makeId() }); - } catch (error) { - log.error(`Failed to send shutdown signal to Tools Host child process: ${formatUnknownError(error)}`); - } + const handle = activeChildrenBySession.get(sessionId) as HostHandle | undefined; - // Set primary timeout for force shutdown - forceKillPrimary = setTimeout(() => { - sigkillChild(); - shutdownChild(); - }, gracePeriodMs); - forceKillPrimary?.unref?.(); - - // Set fallback timeout for force shutdown - forceKillSecondary = setTimeout(() => { - sigkillChild(true); - shutdownChild(); - }, fallbackGracePeriodMs); - forceKillSecondary?.unref?.(); - - // Set up exit/disconnect handlers to resolve - child.once('exit', shutdownChild); - child.once('disconnect', shutdownChild); + await shutdownChildProcess(handle, { + gracePeriodMs: Math.max(0, Number(pluginHost?.gracePeriodMs) || 0), + sessionId, + label: 'Tools Host' }); }; @@ -542,7 +388,7 @@ const composeTools = async ( { toolModules, nodeVersion, contextUrl, contextPath }: GlobalOptions = getOptions(), { sessionId }: AppSession = getSessionOptions() ): Promise => { - const existingSession = activeHostsBySession.get(sessionId); + const existingSession = activeChildrenBySession.get(sessionId); if (existingSession) { log.warn(`Existing Tools Host session detected ${sessionId}. Shutting down the existing host before creating a new one.`); @@ -605,17 +451,17 @@ const composeTools = async ( return; } - const current = activeHostsBySession.get(sessionId); + const current = activeChildrenBySession.get(sessionId); if (current && current.child === host.child) { try { - (host as any).closeStderr(); + host.closeStderr(); log.info('Tools Host stderr reader closed.'); } catch (error) { log.error(`Failed to close Tools Host stderr reader: ${formatUnknownError(error)}`); } - activeHostsBySession.delete(sessionId); + activeChildrenBySession.delete(sessionId); } host.child.off('exit', onChildExitOrDisconnect); @@ -646,7 +492,7 @@ const composeTools = async ( const proxiedCreators = makeProxyCreators(filteredHandle); // Associate the spawned host with the current session - activeHostsBySession.set(sessionId, host); + activeChildrenBySession.set(sessionId, host); host.child.once('exit', onChildExitOrDisconnect); host.child.once('disconnect', onChildExitOrDisconnect); diff --git a/src/server.toolsHost.ts b/src/server.toolsHost.ts index adfc0c51..6465e385 100644 --- a/src/server.toolsHost.ts +++ b/src/server.toolsHost.ts @@ -1,9 +1,10 @@ import { type IpcRequest, type ToolDescriptor, - type SerializedError, makeId } from './server.toolsIpc'; +import { serializeError, type SerializedError } from './server.processIpc'; +import { createProcessHost, type HostContext } from './server.processHost'; import { resolveExternalCreators } from './server.toolsHostCreator'; import { DEFAULT_OPTIONS } from './options.defaults'; import { type ToolOptions } from './options.tools'; @@ -16,31 +17,16 @@ import { } from './server.schema'; import { isPlainObject } from './server.helpers'; -/** - * SubType of IpcRequest for "hello" requests. - */ -type HelloRequest = Extract; - /** * SubType of IpcRequest for "load" requests. */ type LoadRequest = Extract; -/** - * SubType of IpcRequest for "manifest:get" requests. - */ -type ManifestGetRequest = Extract; - /** * SubType of IpcRequest for "invoke" requests. */ type InvokeRequest = Extract; -/** - * SubType of IpcRequest for "shutdown" requests. - */ -type ShutdownRequest = Extract; - /** * State object for the tools host. */ @@ -62,24 +48,6 @@ const createHostState = (invokeTimeoutMs = DEFAULT_OPTIONS.pluginHost.invokeTime invokeTimeoutMs }); -/** - * Serialize an error value into a structured object. - * - * @param errorValue - Error-like value to serialize. - * @returns {SerializedError} - Serialized error object. - */ -const serializeError = (errorValue: unknown) => { - const err = errorValue as SerializedError | undefined; - - return { - message: err?.message || String(errorValue), - stack: err?.stack, - code: err?.code, - details: err?.details, - cause: err?.cause - }; -}; - /** * Result of `normalizeCreatorSchema`. * @@ -259,40 +227,6 @@ const performLoad = async (request: LoadRequest): Promise { - process.send?.({ t: 'hello:ack', id: request.id }); -}; - -/** - * Load tools from the provided list of module specifiers. - * - * @param {LoadRequest} request - Load request object. - * @param warningsErrors - * @param warningsErrors.warnings - List of warnings generated during tool loading. - * @param warningsErrors.errors - List of errors generated during tool loading. - */ -const requestLoad = ( - request: LoadRequest, - { warnings = [], errors = [] }: { warnings?: string[]; errors?: string[] } = {} -) => { - process.send?.({ t: 'load:ack', id: request.id, warnings, errors }); -}; - -/** - * Respond to a manifest request with a list of available tools. - * - * @param {HostState} state - * @param {ManifestGetRequest} request - */ -const requestManifestGet = (state: HostState, request: ManifestGetRequest) => { - process.send?.({ t: 'manifest:result', id: request.id, tools: state.descriptors }); -}; - /** * Invoke a realized tool by id. Validates arguments against the in-memory Zod schema. * @@ -302,12 +236,13 @@ const requestManifestGet = (state: HostState, request: ManifestGetRequest) => { * * @param {HostState} state * @param {InvokeRequest} request + * @param {HostContext} ctx */ -const requestInvoke = async (state: HostState, request: InvokeRequest) => { +const requestInvoke = async (state: HostState, request: InvokeRequest, ctx: HostContext) => { const tool = state.toolMap.get(request.toolId); if (!tool) { - process.send?.({ + ctx.send({ t: 'invoke:result', id: request.id, ok: false, @@ -326,7 +261,7 @@ const requestInvoke = async (state: HostState, request: InvokeRequest) => { settled = true; - process.send?.({ + ctx.send({ t: 'invoke:result', id: request.id, ok: false, @@ -376,13 +311,13 @@ const requestInvoke = async (state: HostState, request: InvokeRequest) => { if (!settled) { settled = true; clearTimeout(timer); - process.send?.({ t: 'invoke:result', id: request.id, ok: true, result }); + ctx.send({ t: 'invoke:result', id: request.id, ok: true, result }); } } catch (error) { if (!settled) { settled = true; clearTimeout(timer); - process.send?.({ + ctx.send({ t: 'invoke:result', id: request.id, ok: false, @@ -393,165 +328,39 @@ const requestInvoke = async (state: HostState, request: InvokeRequest) => { }; /** - * Handle shutdown requests. - * - * @param request - */ -const requestShutdown = (request: ShutdownRequest) => { - process.send?.({ t: 'shutdown:ack', id: request.id }); - process.exit(0); -}; - -/** - * Fallback handler for unhandled errors. - * - * @param {IpcRequest} request - Original IPC request object. - * @param {Error} error - Failed request error object - * - * Attempt to send a structured message back to the IPC channel. The message includes: - * - Type of response ('invoke:result'). - * - Request identifier, or 'n/a' if the request ID is unavailable. - * - Operation status (`ok: false`). - * - Serialized error object. - * - * Any issues during this process (e.g., if `process.send` is unavailable) fail silently. - */ -const requestFallback = (request: IpcRequest, error: Error) => { - try { - process.send?.({ - t: 'invoke:result', - id: request?.id || 'n/a', - ok: false, - error: serializeError(error) - }); - } catch {} -}; - -/** - * Initializes and sets up handlers for incoming IPC (Inter-Process Communication) messages. - * - * @returns Function to remove IPC message listeners. + * Create the Tools Host: a generic child-process host wired with the tool handlers. + * Built-in `hello`/`shutdown` handlers come from `createProcessHost`. */ -const setHandlers = () => { +const createToolsHost = () => { let state: HostState = createHostState(); - /** - * Load tools from the provided list of module specifiers. Splits out warnings/errors - * before updating state. - * - * @param {LoadRequest} request - */ - const onRequestLoad = async (request: LoadRequest) => { - const loaded = await performLoad(request); - - state = { - toolMap: loaded.toolMap, - descriptors: loaded.descriptors, - invokeTimeoutMs: loaded.invokeTimeoutMs - }; - - requestLoad(request, { warnings: loaded.warnings, errors: loaded.errors }); - }; - - /** - * Handle incoming IPC (Inter-Process Communication) messages. - * - * Process the request and execute the corresponding handler function for each type. A fallback handler - * is triggered on error. - * - * @param {IpcRequest} request - The IPC request object containing the type of request and associated data. - * @throws {Error} - Any error, pass the request through the fallback handler. - * - * @remarks - * Supported request types: - * - 'hello': Trigger the `requestHello` handler. - * - 'load': Trigger the `requestLoad` handler. - * - 'manifest:get': Trigger the `requestManifestGet` handler. - * - 'invoke': Trigger the asynchronous `requestInvoke` handler. - * - 'shutdown': Trigger the `requestShutdown` handler. - */ - const handlerMessage = async (request: IpcRequest) => { - try { - switch (request.t) { - case 'hello': - requestHello(request); - break; - - case 'load': - await onRequestLoad(request); - break; - - case 'manifest:get': - requestManifestGet(state, request); - break; - - case 'invoke': { - await requestInvoke(state, request); - break; - } - case 'shutdown': { - requestShutdown(request); - break; - } - } - } catch (error) { - requestFallback(request, error as Error); + return createProcessHost({ + load: async (request, ctx) => { + const loaded = await performLoad(request as LoadRequest); + + state = { + toolMap: loaded.toolMap, + descriptors: loaded.descriptors, + invokeTimeoutMs: loaded.invokeTimeoutMs + }; + + ctx.send({ t: 'load:ack', id: request.id, warnings: loaded.warnings, errors: loaded.errors }); + }, + 'manifest:get': (request, ctx) => { + ctx.send({ t: 'manifest:result', id: request.id, tools: state.descriptors }); + }, + invoke: async (request, ctx) => { + await requestInvoke(state, request as InvokeRequest, ctx); } - }; - - /** - * Listen for incoming IPC messages. - */ - process.on('message', handlerMessage); - - /** - * Handle process disconnects. - */ - const handlerDisconnect = () => { - process.exit(0); - }; - - /** - * Handle process disconnects. - */ - process.on('disconnect', handlerDisconnect); - - // Expose the router for bootstrapping. - return handlerMessage; -}; - -/** - * Lazy initialize for IPC (Inter-Process Communication) handlers. - * - * This is a one-shot process: the first message received will remove itself then - * trigger the real handler setup. - * - * @param {IpcRequest} first - */ -const bootstrapMessage = (first: IpcRequest) => { - // Detach bootstrap to avoid duplicate delivery - process.off('message', bootstrapMessage); - - // Install real handlers and get a reference to the router - const route = setHandlers(); - - // Route the very first message through the same code path the real handler uses - // Use void to fire-and-forget async operations to avoid blocking - void route(first); + }); }; -if (process.send) { - process.on('message', bootstrapMessage); -} +// createProcessHost internally guards on `process.send`, so this is safe at module load. +createToolsHost(); export { normalizeCreatorSchema, performLoad, - requestHello, - requestLoad, - requestManifestGet, requestInvoke, - requestShutdown, - requestFallback, - setHandlers + createToolsHost }; diff --git a/src/server.toolsIpc.ts b/src/server.toolsIpc.ts index a3376146..f51074bb 100644 --- a/src/server.toolsIpc.ts +++ b/src/server.toolsIpc.ts @@ -1,6 +1,12 @@ -import { type ChildProcess } from 'node:child_process'; -import { randomUUID } from 'node:crypto'; import { type ToolOptions } from './options.tools'; +import { + send, + awaitIpc, + makeId, + matchResponse, + serializeError, + type SerializedError +} from './server.processIpc'; /** * IPC (Inter-Process Communication) request messages. @@ -24,17 +30,6 @@ type IpcRequest = { t: 'invoke'; id: string; toolId: string; args: unknown } | { t: 'shutdown'; id: string }; -/** - * Serialized error object for IPC. - * - * @property message - Error message. - * @property stack - Error stack trace. - * @property code - Error code. - * @property cause - Error cause. - * @property details - Additional details. - */ -type SerializedError = { message: string; stack?: string; code?: string; cause?: unknown; details?: unknown }; - /** * Tool descriptor object for IPC. * @@ -84,162 +79,12 @@ type IpcResponse = { t: 'invoke:result'; id: string; ok: false; error: SerializedError } | { t: 'shutdown:ack'; id: string }; -/** - * Generate a unique ID for IPC messages. - */ -const makeId = () => randomUUID(); - -/** - * Send an IPC message to the provided process. - * - * @param processRef - * @param {IpcRequest} request - */ -const send = ( - processRef: NodeJS.Process | ChildProcess, - request: IpcRequest -): boolean => Boolean(processRef.send?.(request)); - -/** - * Await an IPC response from the provided process. - * - * @param processRef - * @param matcher - * @param timeoutMs - */ -const awaitIpc = ( - processRef: NodeJS.Process | ChildProcess, - matcher: (message: any) => message is T, - timeoutMs: number -): Promise => new Promise((resolve, reject) => { - let settled = false; - - // Cleanup listeners and timers on exit or timeout - const cleanup = () => { - processRef.off('message', onMessage); - processRef.off('exit', onExit); - processRef.off('disconnect', onExit); - clearTimeout(timerId); - }; - - // Listen for messages and resolve on match or timeout - const onMessage = (message: any) => { - if (settled) { - return; - } - - if (matcher(message)) { - settled = true; - cleanup(); - resolve(message); - } - }; - - // Reject on exit or timeout - const onExit = (code?: number, signal?: string) => { - if (settled) { - return; - } - - settled = true; - cleanup(); - reject(new Error(`Tools Host exited before response (code=${code}, signal=${signal || 'none'})`)); - }; - - // Set a timeout to reject if the process doesn't respond' - const timerId = setTimeout(() => { - if (settled) { - return; - } - - settled = true; - cleanup(); - reject(new Error('Timed out waiting for IPC response')); - }, timeoutMs); - - timerId?.unref?.(); - - // Attach listeners to the process - processRef.on('message', onMessage); - processRef.on('exit', onExit); - processRef.on('disconnect', onExit); -}); - -/** - * Check if a message is a "hello" response. IPC message type guards. - * - * @param message - */ -const isHelloAck = (message: any): message is { t: 'hello:ack'; id: string } => { - if (!message || message.t !== 'hello:ack') { - return false; - } - - return typeof message.id === 'string'; -}; - -/** - * Check if a message is a "load" response. IPC message type guards. - * - * Checks - * - If a given message is a valid load acknowledgment (`load:ack`) with expected id - * - That the message contains the proper structure, including the required fields and - * correct types for `warnings` and `errors`. - * - * @param expectedId - Expected identifier to match against the message `id` field. - * @returns Function that takes a message and determines if it conforms to the expected structure and values. - */ -const isLoadAck = (expectedId: string) => (message: any): message is { - t: 'load:ack'; id: string; warnings: string[]; errors: string[] -} => { - if (!message || message.t !== 'load:ack' || message.id !== expectedId) { - return false; - } - - const hasWarnings = Array.isArray(message.warnings); - const hasErrors = Array.isArray(message.errors); - - return hasWarnings && hasErrors; -}; - -/** - * Check if a message is a "manifest" response. IPC message type guards. - * - * @param expectedId - */ -const isManifestResult = (expectedId: string) => (message: any): message is { - t: 'manifest:result'; id: string; tools: ToolDescriptor[] -} => { - if (!message || message.t !== 'manifest:result' || message.id !== expectedId) { - return false; - } - - return Array.isArray(message.tools); -}; - -/** - * Check if a message is an "invoke" response. IPC message type guards. - * - * @param expectedId - */ -const isInvokeResult = (expectedId: string) => (message: any): message is - { t: 'invoke:result'; id: string; ok: true; result: unknown } | - { t: 'invoke:result'; id: string; ok: false; error: SerializedError } => { - if (!message || message.t !== 'invoke:result') { - return false; - } - - return message.id === expectedId; -}; - export { send, awaitIpc, makeId, - isHelloAck, - isLoadAck, - isManifestResult, - isInvokeResult, + matchResponse, + serializeError, type IpcRequest, type IpcResponse, type ToolDescriptor, From f36c9f8bc5d041fde410863d453a9ad9ef4817af Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Wed, 29 Jul 2026 10:52:48 -0400 Subject: [PATCH 2/5] fix: review update --- package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/package.json b/package.json index 1b0876d0..725ee418 100644 --- a/package.json +++ b/package.json @@ -6,8 +6,7 @@ "type": "module", "imports": { "~docsCatalog": "./src/docs.json", - "#toolsHost": "./dist/server.toolsHost.js", - "#processHost": "./dist/server.processHost.js" + "#toolsHost": "./dist/server.toolsHost.js" }, "exports": { ".": { From b5651dba5d581b1bb5fc7e0d79cc181e4d8af02c Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Wed, 29 Jul 2026 13:24:39 -0400 Subject: [PATCH 3/5] fix: review update --- src/__tests__/server.process.test.ts | 4 ++-- src/server.process.ts | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/__tests__/server.process.test.ts b/src/__tests__/server.process.test.ts index 81448e55..ebcadb6d 100644 --- a/src/__tests__/server.process.test.ts +++ b/src/__tests__/server.process.test.ts @@ -217,9 +217,9 @@ describe('spawnChildProcess request', () => { t: 'load:error', id: sentId, ok: false, - error: { message: 'handler boom', code: 'E_BOOM' } + error: { message: 'handler lorem ipsum', code: 'E_LOREM_IPSUM' } })); - await expect(pending).rejects.toThrow('handler boom'); + await expect(pending).rejects.toThrow('handler lorem ipsum'); }); }); diff --git a/src/server.process.ts b/src/server.process.ts index d9c3762f..ab4c093f 100644 --- a/src/server.process.ts +++ b/src/server.process.ts @@ -171,6 +171,14 @@ const spawnChildProcess = (config: SpawnConfig): ChildHandle => { // promptly rather than waiting for the timeout. const matcher = (message: any): message is T => matchOk(message) || matchErr(message); + // Deferred rejection channel for early send-side failures (throw or send()===false). + // Race against awaitIpc ensures the caller sees a prompt rejection instead + // of waiting for an IPC timeout since the channel is already broken. + let rejectSendFailure: ((error: Error) => void) | undefined; + const sendFailure = new Promise((_resolve, reject) => { + rejectSendFailure = reject; + }); + const pending = awaitIpc(child, matcher, timeoutMs).then(message => { if ((message as ProcessResponse)?.t === errorType) { const errorValue = (message as { error?: SerializedError })?.error; @@ -196,9 +204,19 @@ const spawnChildProcess = (config: SpawnConfig): ChildHandle => { return message; }); - send(child, { ...req, id } as ProcessRequest); + try { + const ok = send(child, { ...req, id } as ProcessRequest); + + if (!ok) { + log.debug(`IPC send returned false. Failed to send IPC request '${req.t}' (id=${id}); channel closed.`); + } + } catch (error) { + rejectSendFailure?.( + new Error(`Failed to send IPC request '${req.t}' (id=${id}): ${formatUnknownError(error)}`, { cause: error }) + ); + } - return pending; + return Promise.race([pending, sendFailure]) as Promise; }; return { child, closeStderr, request }; From bbf5564832ae015ffc74bfa857bd997aa1c1e8c3 Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Wed, 29 Jul 2026 13:53:47 -0400 Subject: [PATCH 4/5] fix: review update --- src/server.process.ts | 48 ++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/server.process.ts b/src/server.process.ts index ab4c093f..453fbb8a 100644 --- a/src/server.process.ts +++ b/src/server.process.ts @@ -8,6 +8,7 @@ import { send, makeId, matchResponse, + serializeError, type ProcessRequest, type ProcessResponse, type SerializedError @@ -179,26 +180,34 @@ const spawnChildProcess = (config: SpawnConfig): ChildHandle => { rejectSendFailure = reject; }); - const pending = awaitIpc(child, matcher, timeoutMs).then(message => { - if ((message as ProcessResponse)?.t === errorType) { - const errorValue = (message as { error?: SerializedError })?.error; - const settledError = new Error(errorValue?.message || 'Child process handler error', { - cause: errorValue?.cause - }) as Error & { code?: string; details?: unknown }; + // Reconstruct a rich Error preserving stack/code/cause/details from any error-like value. + // Mirrors the SerializedError → Error mapping used in the pending branch so send-side + // failures surface the same diagnostic shape as remote (child-side) failures. + const reconstructError = (message: string, errorValue?: SerializedError): Error => { + const settledError = new Error(message, { + cause: errorValue?.cause + }) as Error & { code?: string; details?: unknown }; - if (errorValue?.stack) { - settledError.stack = errorValue.stack; - } + if (errorValue?.stack) { + settledError.stack = errorValue.stack; + } - if (errorValue?.code) { - settledError.code = errorValue.code; - } + if (errorValue?.code) { + settledError.code = errorValue.code; + } + + if (errorValue?.details) { + settledError.details = errorValue.details; + } - if (errorValue?.details) { - settledError.details = errorValue.details; - } + return settledError; + }; - throw settledError; + const pending = awaitIpc(child, matcher, timeoutMs).then(message => { + if ((message as ProcessResponse)?.t === errorType) { + const errorValue = (message as { error?: SerializedError })?.error; + + throw reconstructError(errorValue?.message || 'Child process handler error', errorValue); } return message; @@ -211,8 +220,13 @@ const spawnChildProcess = (config: SpawnConfig): ChildHandle => { log.debug(`IPC send returned false. Failed to send IPC request '${req.t}' (id=${id}); channel closed.`); } } catch (error) { + const errorMessage = `Failed to send IPC request '${req.t}' (id=${id}): ${formatUnknownError(error)}`; + + log.error(errorMessage); + const serialized = serializeError(error); + rejectSendFailure?.( - new Error(`Failed to send IPC request '${req.t}' (id=${id}): ${formatUnknownError(error)}`, { cause: error }) + reconstructError(errorMessage, { ...serialized, cause: serialized.cause || error }) ); } From ead289b6a8115094c4f1794c5f7c9d2764c60c9a Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Wed, 29 Jul 2026 14:50:52 -0400 Subject: [PATCH 5/5] fix: review update --- src/__tests__/server.process.test.ts | 36 +++++++++++++++++++++++++++- src/server.process.ts | 2 +- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/__tests__/server.process.test.ts b/src/__tests__/server.process.test.ts index ebcadb6d..deeb9747 100644 --- a/src/__tests__/server.process.test.ts +++ b/src/__tests__/server.process.test.ts @@ -158,7 +158,7 @@ describe('spawnChildProcess request', () => { const makeIpcChild = () => { const messageHandlers: Array<(m: any) => void> = []; const child: any = { - send: jest.fn(), + send: jest.fn().mockReturnValue(true), on: jest.fn((event: string, handler: any) => { if (event === 'message') { messageHandlers.push(handler); @@ -222,4 +222,38 @@ describe('spawnChildProcess request', () => { await expect(pending).rejects.toThrow('handler lorem ipsum'); }); + + it('should reject immediately when send returns false', async () => { + const { child } = makeIpcChild(); + + // immediately send false + child.send.mockReturnValue(false); + (spawn as jest.Mock).mockReturnValue(child); + + const { request } = spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + const pending = request({ t: 'hello' }, 'hello:ack', 1000); + + await expect(pending).rejects.toMatchObject({ + message: expect.stringContaining("Failed to send IPC request 'hello'"), + cause: expect.objectContaining({ message: 'IPC send returned false; exiting early.' }) + }); + }); + + it('should reject immediately when send throws an error', async () => { + const { child } = makeIpcChild(); + + // immediately send false + child.send.mockImplementation(() => { + throw new Error('dolor sit'); + }); + (spawn as jest.Mock).mockReturnValue(child); + + const { request } = spawnChildProcess({ importSpecifier: '#host', entry: '/abs/host.js' }); + const pending = request({ t: 'hello' }, 'hello:ack', 1000); + + await expect(pending).rejects.toMatchObject({ + message: expect.stringContaining("Failed to send IPC request 'hello'"), + cause: expect.objectContaining({ message: 'dolor sit' }) + }); + }); }); diff --git a/src/server.process.ts b/src/server.process.ts index 453fbb8a..01f4ef7d 100644 --- a/src/server.process.ts +++ b/src/server.process.ts @@ -217,7 +217,7 @@ const spawnChildProcess = (config: SpawnConfig): ChildHandle => { const ok = send(child, { ...req, id } as ProcessRequest); if (!ok) { - log.debug(`IPC send returned false. Failed to send IPC request '${req.t}' (id=${id}); channel closed.`); + throw new Error('IPC send returned false; exiting early.'); } } catch (error) { const errorMessage = `Failed to send IPC request '${req.t}' (id=${id}): ${formatUnknownError(error)}`;