fix: make adapter disposal total (#587)

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.

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<number, DebugProtocol.Scope[]> = {
         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({

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<void> {
+  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 `"<context>: <message>"` and
  * never throwing.
@@ -37,13 +58,7 @@ export async function disposeAdapterQuietly(
   logger: ILogger,
   context: string
 ): Promise<void> {
-  try {
-    await adapter.dispose();
-  } catch (disposeError: unknown) {
-    try {
+  await disposeAdapterSafely(adapter, (disposeError) => {
       logger.warn(`${context}: ${getErrorMessage(disposeError)}`);
-    } catch {
-      // Deliberately empty — see above.
-    }
-  }
+  });
 }

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);
 

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,

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();
+  });
+});

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<string, Array<(...args: unknown[]) => 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', () => {