diff --git a/changelog.d/587.fixed.md b/changelog.d/587.fixed.md new file mode 100644 index 00000000..a84d5d13 --- /dev/null +++ b/changelog.d/587.fixed.md @@ -0,0 +1 @@ +Make adapter disposal total across registry unregister, dispose-all, and idle auto-dispose paths, including adapters whose `dispose()` throws synchronously. diff --git a/packages/shared/tests/unit/adapter-policy-js.spec.ts b/packages/shared/tests/unit/adapter-policy-js.spec.ts index 2648ed99..f79830dc 100644 --- a/packages/shared/tests/unit/adapter-policy-js.spec.ts +++ b/packages/shared/tests/unit/adapter-policy-js.spec.ts @@ -33,6 +33,12 @@ describe('JsDebugAdapterPolicy', () => { describe('normalizeStopReason', () => { const normalize = JsDebugAdapterPolicy.normalizeStopReason!; + const pauseContext = (pausePending: boolean) => ({ + pausePending, + ...(pausePending ? { pauseSource: 'user' as const } : {}), + lineBreakpointCount: 0, + functionBreakpointCount: 0 + }); // js-debug uses the same body for explicit pauses and genuine steps const jsDebugPauseBody = { reason: 'step', @@ -42,17 +48,17 @@ describe('JsDebugAdapterPolicy', () => { } as DebugProtocol.StoppedEvent['body']; it("maps a 'step' stop to 'pause' while a pause request is in flight", () => { - expect(normalize('step', jsDebugPauseBody, { pausePending: true })).toBe('pause'); + expect(normalize('step', jsDebugPauseBody, pauseContext(true))).toBe('pause'); }); it("leaves a genuine 'step' stop untouched when no pause is pending", () => { - expect(normalize('step', jsDebugPauseBody, { pausePending: false })).toBeUndefined(); + expect(normalize('step', jsDebugPauseBody, pauseContext(false))).toBeUndefined(); }); it('never touches other reasons, even while a pause is pending', () => { - expect(normalize('breakpoint', { reason: 'breakpoint' }, { pausePending: true })).toBeUndefined(); - expect(normalize('pause', { reason: 'pause' }, { pausePending: true })).toBeUndefined(); - expect(normalize('exception', { reason: 'exception' }, { pausePending: true })).toBeUndefined(); + expect(normalize('breakpoint', { reason: 'breakpoint' }, pauseContext(true))).toBeUndefined(); + expect(normalize('pause', { reason: 'pause' }, pauseContext(true))).toBeUndefined(); + expect(normalize('exception', { reason: 'exception' }, pauseContext(true))).toBeUndefined(); }); }); @@ -221,7 +227,7 @@ describe('JsDebugAdapterPolicy', () => { expect(JsDebugAdapterPolicy.extractLocalVariables!([frame], scopes, vars)).toEqual({ variables: [], scopeRefs: [] }); }); - it('never substitutes Global for a frame that exposes no Local scope', () => { + it('keeps Global explicit when a frame exposes no Local scope (#595)', () => { const scopes: Record = { 1: [ { name: 'Script', variablesReference: 100, expensive: false }, @@ -235,8 +241,10 @@ describe('JsDebugAdapterPolicy', () => { const result = JsDebugAdapterPolicy.extractLocalVariables!([frame], scopes, vars); - expect(result).toMatchObject({ variables: [], scopeRefs: [] }); - expect(result.note).toMatch(/get_scopes.*get_variables/); + expect(result.variables).toEqual([]); + expect(result.scopeRefs).toEqual([]); + expect(result.note).toContain('get_scopes'); + expect(result.note).toContain('get_variables'); }); it('merges a for-body Block scope ahead of the function locals (issue #558)', () => { @@ -597,7 +605,10 @@ describe('JsDebugAdapterPolicy', () => { { requestId: '3', dapCommand: 'configurationDone' }, { requestId: '4', dapCommand: 'threads' }, ]; - const ordered = JsDebugAdapterPolicy.processQueuedCommands!(commands); + const ordered = JsDebugAdapterPolicy.processQueuedCommands!( + commands, + JsDebugAdapterPolicy.createInitialState() + ); expect(ordered.map(c => c.dapCommand)).toEqual([ 'setBreakpoints', 'configurationDone', @@ -681,6 +692,7 @@ describe('JsDebugAdapterPolicy', () => { describe('getAdapterSpawnConfig', () => { it('returns spawn configuration when adapterCommand provided', () => { const spawn = JsDebugAdapterPolicy.getAdapterSpawnConfig!({ + executablePath: '/usr/bin/node', adapterCommand: { command: '/usr/bin/node', args: ['vsDebugServer.cjs', '5678', '127.0.0.1'], @@ -689,6 +701,7 @@ describe('JsDebugAdapterPolicy', () => { adapterHost: '127.0.0.1', adapterPort: 5678, logDir: '/tmp/session', + scriptPath: '/workspace/app.js' }); expect(spawn).toMatchObject({ diff --git a/src/adapters/adapter-disposal.ts b/src/adapters/adapter-disposal.ts index 0c60311a..72173f81 100644 --- a/src/adapters/adapter-disposal.ts +++ b/src/adapters/adapter-disposal.ts @@ -22,6 +22,27 @@ import type { IDebugAdapter, ILogger } from '@debugmcp/shared'; import { getErrorMessage } from '../errors/debug-errors.js'; +export type AdapterDisposalReporter = (error: unknown) => void; + +/** + * Dispose an adapter and report either a synchronous throw or an asynchronous + * rejection without ever allowing disposal or reporting to escape. + */ +export async function disposeAdapterSafely( + adapter: IDebugAdapter, + reporter: AdapterDisposalReporter +): Promise { + try { + await adapter.dispose(); + } catch (disposeError: unknown) { + try { + reporter(disposeError); + } catch { + // Disposal is a terminal cleanup path; a broken reporter cannot revive it. + } + } +} + /** * Dispose `adapter`, reporting any failure as `": "` and * never throwing. @@ -37,13 +58,7 @@ export async function disposeAdapterQuietly( logger: ILogger, context: string ): Promise { - try { - await adapter.dispose(); - } catch (disposeError: unknown) { - try { + await disposeAdapterSafely(adapter, (disposeError) => { logger.warn(`${context}: ${getErrorMessage(disposeError)}`); - } catch { - // Deliberately empty — see above. - } - } + }); } diff --git a/src/adapters/adapter-registry.ts b/src/adapters/adapter-registry.ts index 847173fa..55bec4f5 100644 --- a/src/adapters/adapter-registry.ts +++ b/src/adapters/adapter-registry.ts @@ -22,6 +22,8 @@ import type { AdapterMetadata as SharedAdapterMetadata, AdapterManifestEntry, Fa import { AdapterLoader } from './adapter-loader.js'; import type { AdapterMetadata } from './adapter-loader.js'; import { isContainerRuntime } from '../utils/container-path-utils.js'; +import { getErrorMessage } from '../errors/debug-errors.js'; +import { disposeAdapterSafely } from './adapter-disposal.js'; /** * Default registry configuration @@ -108,8 +110,8 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { const activeSet = this.activeAdapters.get(language); if (activeSet) { for (const adapter of activeSet) { - adapter.dispose().catch(err => { - this.emit('error', new Error(`Failed to dispose adapter: ${err.message}`)); + void disposeAdapterSafely(adapter, (error) => { + this.emit('error', new Error(`Failed to dispose adapter: ${getErrorMessage(error)}`)); }); this.clearDisposeTimer(adapter); } @@ -402,8 +404,8 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { for (const [language, activeSet] of this.activeAdapters) { for (const adapter of activeSet) { disposePromises.push( - adapter.dispose().catch(err => { - this.emit('error', new Error(`Failed to dispose adapter for ${language}: ${err.message}`)); + disposeAdapterSafely(adapter, (error) => { + this.emit('error', new Error(`Failed to dispose adapter for ${language}: ${getErrorMessage(error)}`)); }) ); } @@ -470,8 +472,8 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { this.clearDisposeTimer(adapter); // Start dispose timer const timer = setTimeout(() => { - adapter.dispose().catch(err => { - this.emit('error', new Error(`Auto-dispose failed: ${err.message}`)); + void disposeAdapterSafely(adapter, (error) => { + this.emit('error', new Error(`Auto-dispose failed: ${getErrorMessage(error)}`)); }); }, this.config.autoDisposeTimeout); diff --git a/tests/typecheck-baseline.json b/tests/typecheck-baseline.json index f1e9f8de..4eb6fd8d 100644 --- a/tests/typecheck-baseline.json +++ b/tests/typecheck-baseline.json @@ -20,7 +20,6 @@ "packages/adapter-rust/tests/rust-adapter.test.ts": 6, "packages/adapter-rust/tests/rust-debug-adapter.toolchain.test.ts": 26, "packages/shared/tests/unit/adapter-policy-cpp.test.ts": 3, - "packages/shared/tests/unit/adapter-policy-js.spec.ts": 7, "packages/shared/tests/unit/adapter-policy-rust.test.ts": 32, "tests/adapters/go/unit/go-debug-adapter.test.ts": 2, "tests/adapters/go/unit/go-utils.test.ts": 6, diff --git a/tests/unit/adapters/adapter-disposal.test.ts b/tests/unit/adapters/adapter-disposal.test.ts new file mode 100644 index 00000000..83fd0c12 --- /dev/null +++ b/tests/unit/adapters/adapter-disposal.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { IDebugAdapter } from '@debugmcp/shared'; +import { disposeAdapterSafely } from '../../../src/adapters/adapter-disposal.js'; + +describe('disposeAdapterSafely', () => { + it.each([ + ['synchronous throw', () => { throw new Error('sync dispose failure'); }], + ['asynchronous rejection', () => Promise.reject(new Error('async dispose failure'))] + ])('reports a %s and resolves', async (_label, dispose) => { + const reporter = vi.fn(); + const adapter = { dispose } as unknown as IDebugAdapter; + + await expect(disposeAdapterSafely(adapter, reporter)).resolves.toBeUndefined(); + + expect(reporter).toHaveBeenCalledWith(expect.any(Error)); + }); + + it('remains total when the reporter itself throws', async () => { + const adapter = { + dispose: () => { throw new Error('dispose failed'); } + } as unknown as IDebugAdapter; + + await expect( + disposeAdapterSafely(adapter, () => { throw new Error('reporter failed'); }) + ).resolves.toBeUndefined(); + }); +}); diff --git a/tests/unit/adapters/adapter-registry.test.ts b/tests/unit/adapters/adapter-registry.test.ts index 28164d0d..c7d85c8a 100644 --- a/tests/unit/adapters/adapter-registry.test.ts +++ b/tests/unit/adapters/adapter-registry.test.ts @@ -1,6 +1,21 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { AdapterRegistry, getAdapterRegistry, resetAdapterRegistry } from '../../../src/adapters/adapter-registry.js'; import { AdapterNotFoundError, DuplicateRegistrationError, FactoryValidationError } from '@debugmcp/shared'; +import { createProductionDependencies } from '../../../src/container/dependencies.js'; +import { createMockDependencies } from '../../test-utils/helpers/test-dependencies.js'; + +// AdapterRegistry.create() dynamically imports the production container. +// Keep this suite on inert dependencies so repeated registry construction +// never attaches real winston transports or accumulates process listeners. +vi.mock('../../../src/container/dependencies.js'); +vi.mock('../../../src/utils/logger.js', () => ({ + createLogger: vi.fn(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn() + })) +})); const createAdapterStub = () => { const eventHandlers = new Map void>>(); @@ -47,6 +62,15 @@ describe('AdapterRegistry', () => { beforeEach(() => { vi.restoreAllMocks(); vi.stubEnv('MCP_CONTAINER', undefined); + const dependencies = { + ...createMockDependencies(), + environment: { + get: vi.fn((key: string) => process.env[key]), + getAll: vi.fn(() => ({ ...process.env })), + getCurrentWorkingDirectory: vi.fn(() => process.cwd()) + } + }; + vi.mocked(createProductionDependencies).mockReturnValue(dependencies as any); }); it('registers and unregisters factories (with validation)', async () => { @@ -488,10 +512,12 @@ describe('AdapterRegistry', () => { }); describe('disposal error handling', () => { - it('unregister emits error when adapter disposal fails', async () => { + it('unregister reports a synchronous adapter disposal throw without escaping', async () => { const registry = new AdapterRegistry(); const adapterStub = createAdapterStub(); - adapterStub.dispose.mockRejectedValue(new Error('dispose failed')); + adapterStub.dispose.mockImplementation(() => { + throw new Error('dispose failed synchronously'); + }); const factory = createFactory({ createAdapter: vi.fn().mockReturnValue(adapterStub) @@ -514,16 +540,17 @@ describe('AdapterRegistry', () => { const result = registry.unregister('mock'); expect(result).toBe(true); - // Let the async disposal error propagate - await new Promise(resolve => setTimeout(resolve, 10)); - expect(errors.length).toBeGreaterThan(0); - expect(errors[0].message).toContain('dispose'); + await Promise.resolve(); + expect(errors).toHaveLength(1); + expect(errors[0].message).toContain('dispose failed synchronously'); }); - it('disposeAll resolves even when adapter disposal fails', async () => { + it('disposeAll resolves when adapter disposal throws synchronously', async () => { const registry = new AdapterRegistry(); const adapterStub = createAdapterStub(); - adapterStub.dispose.mockRejectedValue(new Error('dispose boom')); + adapterStub.dispose.mockImplementation(() => { + throw new Error('dispose boom synchronously'); + }); const factory = createFactory({ createAdapter: vi.fn().mockReturnValue(adapterStub) @@ -544,6 +571,39 @@ describe('AdapterRegistry', () => { await expect(registry.disposeAll()).resolves.toBeUndefined(); expect(registry.getActiveAdapterCount()).toBe(0); }); + + it('auto-dispose reports a synchronous adapter disposal throw without leaking from the timer', async () => { + vi.useFakeTimers(); + try { + const registry = new AdapterRegistry({ autoDispose: true, autoDisposeTimeout: 10 }); + const adapterStub = createAdapterStub(); + adapterStub.dispose.mockImplementation(() => { + throw new Error('idle dispose threw synchronously'); + }); + const factory = createFactory({ createAdapter: vi.fn().mockReturnValue(adapterStub) }); + const errors: Error[] = []; + registry.on('error', (error: Error) => errors.push(error)); + + await registry.register('mock', factory as any); + await registry.create('mock', { + sessionId: 's1', + adapterHost: '127.0.0.1', + adapterPort: 9000, + logDir: '/tmp', + scriptPath: '/tmp/app.js', + executablePath: '', + launchConfig: {} + }); + + adapterStub.emit('stateChanged', 'debugging', 'disconnected'); + await vi.advanceTimersByTimeAsync(10); + + expect(errors).toHaveLength(1); + expect(errors[0].message).toContain('idle dispose threw synchronously'); + } finally { + vi.useRealTimers(); + } + }); }); describe('singleton helpers', () => {