From 08668b0c65461684627bd9d0cb2895663f635b1e Mon Sep 17 00:00:00 2001 From: JF Date: Mon, 13 Jul 2026 20:38:28 +0300 Subject: [PATCH] test: fix fork-worker process-listener leaks behind windows CI flake (#159) A vitest fork worker on windows-latest occasionally died before flushing its results, failing a fully green suite with "[vitest-pool]: Worker exited unexpectedly". Attribution via summary counts: the dying file was tests/unit/proxy/dap-proxy-core.test.ts, whose setupGlobalErrorHandlers describe attached four REAL exit-bearing listeners to the worker process (call-through process.on spy) and never removed them. - dap-proxy-core.test.ts: capture handlers via mockImplementation instead of attaching; file-wide process.exit mock net (start() arms a real 10s exit(1) timeout); drop inline exitSpy.mockRestore() calls so the net survives until restoreAllMocks. - dap-proxy-dependencies.test.ts: settle the async shutdownFn().finally(() => process.exit(1)) chain before restoring the exit spy (its intercepted exit(1) appears in the failed run's log); assert the exit. - stdio-command.test.ts: capture process.on without attaching; drain the 60s keepAlive interval via the captured SIGTERM handler; fake stdin everywhere. - vitest.setup.ts: per-fork process-listener leak guard - baselines rawListeners for 8 critical events, removes + reports leaks after each test with test/file attribution; LEAK_GUARD_STRICT=1 makes it fail. - CI attribution: json reporter on CI (survives fork death) and test-results.json in the windows-test-diagnostics artifact. - Delete dead tests/unit/proxy/proxy-manager-test-setup.ts. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 1 + tests/unit/cli/stdio-command.test.ts | 46 +++++++--- tests/unit/proxy/dap-proxy-core.test.ts | 75 ++++++++-------- .../unit/proxy/dap-proxy-dependencies.test.ts | 10 ++- tests/unit/proxy/proxy-manager-test-setup.ts | 54 ------------ tests/vitest.setup.ts | 86 ++++++++++++++++++- vitest.config.ts | 9 +- 7 files changed, 171 insertions(+), 110 deletions(-) delete mode 100644 tests/unit/proxy/proxy-manager-test-setup.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d7808543..a3a85b2e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,6 +103,7 @@ jobs: dist/**/*.log logs/tests/**/*.log logs/tests/adapters/failures/**/*.json + test-results.json if-no-files-found: warn - name: Upload coverage reports diff --git a/tests/unit/cli/stdio-command.test.ts b/tests/unit/cli/stdio-command.test.ts index 83d07e9a..5dc2569e 100644 --- a/tests/unit/cli/stdio-command.test.ts +++ b/tests/unit/cli/stdio-command.test.ts @@ -11,6 +11,17 @@ describe('STDIO Command Handler', () => { let mockServerFactory: ReturnType; let mockExitProcess: ReturnType; let mockServer: DebugMcpServer; + const originalProcessOn = process.on; + let capturedProcessListeners: Map; + + function makeFakeStdin() { + const stdin = new EventEmitter() as unknown as NodeJS.ReadStream & { + resume: ReturnType; + emit: (event: string, ...args: unknown[]) => boolean; + }; + (stdin as unknown as { resume: unknown }).resume = vi.fn(); + return stdin; + } beforeEach(() => { // Create mock logger @@ -36,6 +47,26 @@ describe('STDIO Command Handler', () => { // Create mock exit function mockExitProcess = vi.fn(); + + // Capture process listeners WITHOUT attaching them (issue #159): a + // successful handleStdioCommand registers real SIGTERM/SIGINT/exit + // listeners that would otherwise leak into the vitest fork worker. + capturedProcessListeners = new Map(); + process.on = vi.fn(function (this: NodeJS.Process, event: string, listener: (...args: unknown[]) => void) { + const list = capturedProcessListeners.get(event) ?? []; + list.push(listener); + capturedProcessListeners.set(event, list); + return this; + }) as unknown as typeof process.on; + }); + + afterEach(() => { + process.on = originalProcessOn; + // Fire captured SIGTERM handlers once: each closes over the 60s keepAlive + // interval and clears it; the exit goes to the mocked exitProcess. + for (const listener of capturedProcessListeners.get('SIGTERM') ?? []) { + listener(); + } }); it('should start server successfully in stdio mode', async () => { @@ -47,7 +78,8 @@ describe('STDIO Command Handler', () => { await handleStdioCommand(options, { logger: mockLogger, serverFactory: mockServerFactory, - exitProcess: mockExitProcess + exitProcess: mockExitProcess, + stdin: makeFakeStdin() }); // Verify log level was set @@ -79,7 +111,8 @@ describe('STDIO Command Handler', () => { await handleStdioCommand(options, { logger: mockLogger, serverFactory: mockServerFactory, - exitProcess: mockExitProcess + exitProcess: mockExitProcess, + stdin: makeFakeStdin() }); // Verify log level was not changed @@ -154,15 +187,6 @@ describe('STDIO Command Handler', () => { vi.unstubAllEnvs(); }); - function makeFakeStdin() { - const stdin = new EventEmitter() as unknown as NodeJS.ReadStream & { - resume: ReturnType; - emit: (event: string, ...args: unknown[]) => boolean; - }; - (stdin as unknown as { resume: unknown }).resume = vi.fn(); - return stdin; - } - it('exits 0 when stdin ends in host mode (MCP client disconnected)', async () => { const stdin = makeFakeStdin(); diff --git a/tests/unit/proxy/dap-proxy-core.test.ts b/tests/unit/proxy/dap-proxy-core.test.ts index 098f7cb7..5ee16828 100644 --- a/tests/unit/proxy/dap-proxy-core.test.ts +++ b/tests/unit/proxy/dap-proxy-core.test.ts @@ -43,6 +43,19 @@ function createMockLogger(): ILogger { }; } +/* ------------------------------------------------------------------ */ +/* Safety net (issue #159) */ +/* ------------------------------------------------------------------ */ + +// No code path in this file may hard-exit the fork worker: ProxyRunner.start() +// arms a real 10s init timeout that calls process.exit(1). Describes that +// assert on exit re-spy process.exit and get this same mock instance back. +// Restoration is centralized in tests/vitest.setup.ts (restoreAllMocks), which +// runs after file-local afterEach hooks have awaited runner.stop(). +beforeEach(() => { + vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); +}); + /* ------------------------------------------------------------------ */ /* ProxyRunner */ /* ------------------------------------------------------------------ */ @@ -430,7 +443,6 @@ describe('ProxyRunner IPC communication', () => { await new Promise(r => setTimeout(r, 10)); expect(exitSpy).toHaveBeenCalledWith(0); - exitSpy.mockRestore(); }); it('error handler logs IPC channel error', async () => { @@ -505,8 +517,6 @@ describe('ProxyRunner stdin communication', () => { ); expect(shutdownSpy).toHaveBeenCalled(); expect(exitSpy).toHaveBeenCalledWith(0); - - exitSpy.mockRestore(); }); it('stop() closing the stdin interface does not exit the process', async () => { @@ -523,8 +533,6 @@ describe('ProxyRunner stdin communication', () => { expect(logger.warn).not.toHaveBeenCalledWith( expect.stringContaining('stdin closed') ); - - exitSpy.mockRestore(); }); }); @@ -581,8 +589,6 @@ describe('ProxyRunner heartbeat and init timeout', () => { expect(fakeSend).toHaveBeenCalledWith( expect.objectContaining({ type: 'ipc-heartbeat-tick', counter: 2 }) ); - - exitSpy.mockRestore(); }); it('shuts down and exits(1) when heartbeat tick send fails', async () => { @@ -611,8 +617,6 @@ describe('ProxyRunner heartbeat and init timeout', () => { exitSpy.mockClear(); await vi.advanceTimersByTimeAsync(15000); expect(exitSpy).not.toHaveBeenCalled(); - - exitSpy.mockRestore(); }); it('init timeout fires after 10 seconds', async () => { @@ -627,8 +631,6 @@ describe('ProxyRunner heartbeat and init timeout', () => { expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining('No initialization received') ); - - exitSpy.mockRestore(); }); it('init timeout does not fire before 10 seconds', async () => { @@ -640,8 +642,6 @@ describe('ProxyRunner heartbeat and init timeout', () => { vi.advanceTimersByTime(9999); expect(exitSpy).not.toHaveBeenCalled(); - - exitSpy.mockRestore(); }); }); @@ -656,18 +656,41 @@ describe('ProxyRunner.setupGlobalErrorHandlers', () => { const originalSend = process.send; let processOnSpy: ReturnType; let exitSpy: ReturnType; + let capturedHandlers: Record; + const guardedEvents = ['uncaughtException', 'unhandledRejection', 'SIGTERM', 'SIGINT'] as const; + let listenerBaseline: Map; beforeEach(() => { deps = createMockDependencies(); logger = createMockLogger(); delete (process as any).send; - processOnSpy = vi.spyOn(process, 'on'); + listenerBaseline = new Map( + guardedEvents.map((event) => [event, process.rawListeners(event) as Function[]]) + ); + // Capture handlers WITHOUT attaching them: a real uncaughtException / + // SIGTERM listener that outlives the test calls process.exit() and can + // hard-kill the vitest fork worker (issue #159). + capturedHandlers = {}; + processOnSpy = vi.spyOn(process, 'on').mockImplementation(function (this: any, event: string, fn: any) { + capturedHandlers[event] = fn; + return this; + }); exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); }); afterEach(async () => { processOnSpy.mockRestore(); exitSpy.mockRestore(); + // Belt-and-braces: remove anything that still reached the real process. + for (const event of guardedEvents) { + const evt: string = event; + const baseline = listenerBaseline.get(evt) ?? []; + for (const listener of process.rawListeners(evt)) { + if (!baseline.includes(listener as Function)) { + process.removeListener(evt, listener as (...args: unknown[]) => void); + } + } + } if (originalSend) { process.send = originalSend; } else { @@ -690,12 +713,6 @@ describe('ProxyRunner.setupGlobalErrorHandlers', () => { }); it('uncaughtException handler sends error and calls shutdown', async () => { - const capturedHandlers: Record = {}; - processOnSpy.mockImplementation(function (this: any, event: string, fn: any) { - capturedHandlers[event] = fn; - return this; - }); - runner = new ProxyRunner(deps, logger); const shutdownFn = vi.fn().mockResolvedValue(undefined); const getSessionId = vi.fn().mockReturnValue('sess-1'); @@ -713,12 +730,6 @@ describe('ProxyRunner.setupGlobalErrorHandlers', () => { }); it('unhandledRejection handler sends error but does not exit', () => { - const capturedHandlers: Record = {}; - processOnSpy.mockImplementation(function (this: any, event: string, fn: any) { - capturedHandlers[event] = fn; - return this; - }); - runner = new ProxyRunner(deps, logger); const shutdownFn = vi.fn().mockResolvedValue(undefined); const getSessionId = vi.fn().mockReturnValue(null); @@ -734,12 +745,6 @@ describe('ProxyRunner.setupGlobalErrorHandlers', () => { }); it('SIGTERM handler shuts down and exits with 0', async () => { - const capturedHandlers: Record = {}; - processOnSpy.mockImplementation(function (this: any, event: string, fn: any) { - capturedHandlers[event] = fn; - return this; - }); - runner = new ProxyRunner(deps, logger); const shutdownFn = vi.fn().mockResolvedValue(undefined); @@ -753,12 +758,6 @@ describe('ProxyRunner.setupGlobalErrorHandlers', () => { }); it('SIGINT handler shuts down and exits with 0', async () => { - const capturedHandlers: Record = {}; - processOnSpy.mockImplementation(function (this: any, event: string, fn: any) { - capturedHandlers[event] = fn; - return this; - }); - runner = new ProxyRunner(deps, logger); const shutdownFn = vi.fn().mockResolvedValue(undefined); diff --git a/tests/unit/proxy/dap-proxy-dependencies.test.ts b/tests/unit/proxy/dap-proxy-dependencies.test.ts index 6b774e57..f8ad3ae9 100644 --- a/tests/unit/proxy/dap-proxy-dependencies.test.ts +++ b/tests/unit/proxy/dap-proxy-dependencies.test.ts @@ -181,7 +181,7 @@ describe('setupGlobalErrorHandlers', () => { exitSpy.mockRestore(); }); - it('unhandledRejection handler sends error message', () => { + it('unhandledRejection handler sends error, shuts down, and exits(1)', async () => { const capturedHandlers: Record = {}; const spy = vi.spyOn(process, 'on').mockImplementation(function (this: any, event: string, fn: any) { capturedHandlers[event] = fn; @@ -204,6 +204,14 @@ describe('setupGlobalErrorHandlers', () => { expect.objectContaining({ type: 'error', sessionId: 'unknown' }) ); + // The handler schedules shutdownFn().finally(() => process.exit(1)) as a + // microtask chain. Settle it BEFORE restoring the exit spy — restoring + // first lets a REAL exit(1) fire between tests, which can hard-kill the + // vitest fork worker (issue #159). + await new Promise(r => setTimeout(r, 0)); + expect(shutdownFn).toHaveBeenCalled(); + expect(exitSpy).toHaveBeenCalledWith(1); + spy.mockRestore(); exitSpy.mockRestore(); }); diff --git a/tests/unit/proxy/proxy-manager-test-setup.ts b/tests/unit/proxy/proxy-manager-test-setup.ts deleted file mode 100644 index cf4696e0..00000000 --- a/tests/unit/proxy/proxy-manager-test-setup.ts +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Test setup for ProxyManager tests - * Handles expected unhandled rejections from timeout tests - */ - -let unhandledRejectionHandler: ((reason: any, promise: Promise) => void) | null = null; -let originalListeners: Function[] = []; - -/** - * Setup handler for expected unhandled rejections in timeout tests - */ -export function setupTimeoutTestHandler() { - // Capture ALL existing listeners before removing them - originalListeners = process.rawListeners('unhandledRejection') as Function[]; - - unhandledRejectionHandler = (reason: any, promise: Promise) => { - // Check if this is an expected timeout rejection from our tests - const message = reason?.message || ''; - const isExpectedTimeout = - message.includes('Proxy initialization timeout') || - message.includes('Timeout waiting for DAP response'); - - if (!isExpectedTimeout) { - // If it's not an expected timeout, re-throw or log - console.error('Unexpected unhandled rejection:', reason); - // Delegate to original listeners - for (const listener of originalListeners) { - listener(reason, promise); - } - } - // Otherwise, silently ignore as it's expected in our timeout tests - }; - - // Remove existing handlers and add ours - process.removeAllListeners('unhandledRejection'); - process.on('unhandledRejection', unhandledRejectionHandler); -} - -/** - * Cleanup handler after tests - */ -export function cleanupTimeoutTestHandler() { - if (unhandledRejectionHandler) { - process.removeListener('unhandledRejection', unhandledRejectionHandler); - unhandledRejectionHandler = null; - } - - // Restore ALL original listeners - process.removeAllListeners('unhandledRejection'); - for (const listener of originalListeners) { - process.on('unhandledRejection', listener as any); - } - originalListeners = []; -} diff --git a/tests/vitest.setup.ts b/tests/vitest.setup.ts index e9a72b8c..63fd450b 100644 --- a/tests/vitest.setup.ts +++ b/tests/vitest.setup.ts @@ -6,15 +6,34 @@ * - Set up global mocks * - Initialize test environments */ -import { vi, beforeAll, afterEach, afterAll } from 'vitest'; +import { vi, beforeAll, afterEach, afterAll, expect } from 'vitest'; import { portManager } from './test-utils/helpers/port-manager.js'; +/** Best-effort path of the test file currently running (empty outside a test). */ +function currentTestPath(): string { + try { + return expect.getState().testPath ?? ''; + } catch { + return ''; + } +} + // Surface unhandled rejections/exceptions during tests with concise messages process.on('unhandledRejection', (reason) => { - console.error('[Test] UnhandledRejection:', reason instanceof Error ? reason.message : reason); + const at = currentTestPath(); + console.error( + '[Test] UnhandledRejection:', + reason instanceof Error ? reason.message : reason, + at ? `(file: ${at})` : '' + ); }); process.on('uncaughtException', (err) => { - console.error('[Test] UncaughtException:', err instanceof Error ? err.message : err); + const at = currentTestPath(); + console.error( + '[Test] UncaughtException:', + err instanceof Error ? err.message : err, + at ? `(file: ${at})` : '' + ); }); // Ensure console silencing is disabled in unit tests unless explicitly set @@ -47,6 +66,67 @@ beforeAll(() => { portManager.reset(); }); +// --- process-listener leak guard (issue #159) ------------------------------- +// Tests that attach real listeners to the fork worker's `process` and never +// remove them can hard-kill the worker: a leaked uncaughtException handler +// whose mocks were reset throws inside the handler (fatal), and leaked +// handlers around exit-bearing production paths call the real process.exit(). +// Vitest then reports "[vitest-pool]: Worker exited unexpectedly" with the +// file's results lost — a red CI run over a fully green suite. +// +// 'warning' is deliberately not guarded: src/index.ts installs a module-level +// noop 'warning' listener on import, which would trip the guard benignly. +const GUARDED_PROCESS_EVENTS = [ + 'uncaughtException', + 'unhandledRejection', + 'SIGTERM', + 'SIGINT', + 'message', + 'disconnect', + 'error', + 'exit' +] as const; + +type ProcessListener = (...args: unknown[]) => void; + +// Baseline includes vitest/tinypool worker plumbing (installed before setup +// files run) and this file's diagnostic handlers (registered above). +const processListenerBaseline = new Map>( + GUARDED_PROCESS_EVENTS.map((event) => [ + event, + new Set(process.rawListeners(event) as ProcessListener[]) + ]) +); + +// Registered BEFORE the mock-reset afterEach below: vitest's sequence.hooks +// defaults to 'stack' (LIFO), so this hook runs LAST — after file-local +// afterEach cleanup AND after restoreAllMocks (any process.on spies are gone). +afterEach(() => { + const leaked: string[] = []; + for (const event of GUARDED_PROCESS_EVENTS) { + const evt: string = event; + const baseline = processListenerBaseline.get(evt) ?? new Set(); + for (const listener of process.rawListeners(evt) as ProcessListener[]) { + if (!baseline.has(listener)) { + process.removeListener(evt, listener); + leaked.push(evt); + } + } + } + if (leaked.length > 0) { + const state = expect.getState(); + console.error( + `[process-listener-leak] Removed leaked '${leaked.join("', '")}' listener(s) left by ` + + `"${state.currentTestName ?? 'unknown test'}" (${state.testPath ?? 'unknown file'}). ` + + `Leaked process listeners can hard-kill the vitest fork worker (issue #159): ` + + `capture handlers with mockImplementation instead of attaching them, or remove them in afterEach.` + ); + if (process.env.LEAK_GUARD_STRICT) { + throw new Error(`[process-listener-leak] leaked listener(s): ${leaked.join(', ')}`); + } + } +}); + // Reset test states after each test afterEach(() => { vi.resetAllMocks(); diff --git a/vitest.config.ts b/vitest.config.ts index 2dc4aa37..2f0357c6 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -53,7 +53,8 @@ function onConsoleLog(log: string, type: 'stdout' | 'stderr'): boolean | void { '[Discovery Test]', '[Workflow Test]', '[Test Server]', - '[env-utils]' + '[env-utils]', + '[process-listener-leak]' ]; if (importantPatterns.some(pattern => log.includes(pattern))) { return true; @@ -183,8 +184,10 @@ export default defineConfig({ // Seed is unpinned (defaults to Date.now()); reproduce a failure with // `vitest run --project --sequence.seed= [--sequence.shuffle.tests]`. sequence: { shuffle: { files: true, tests: false } }, - // Reporter configuration - reporters: process.env.CI ? ['dot'] : ['default'], + // Reporter configuration. On CI, 'json' (written from the main process, so + // it survives a fork-worker death) records which file never reported — + // attribution the dot reporter cannot give for pool errors (issue #159). + reporters: process.env.CI ? ['dot', 'json'] : ['default'], outputFile: { json: './test-results.json' },