From 00dd959b82bb6675e01f54ade1180e345c826fee Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:03:22 +0800 Subject: [PATCH] fix(mcp): derive shared provider cleanup from loaded ports Separate load-failure metadata from the typed optional observation ports and derive the process cleanup inventory from those ports. This includes the JavaScript runtime owner and removes the separate four-provider list. Preserve all load failures in server injection and keep shared providers caller-owned across individual server closure. Retain failed close hooks for retry while removing peers whose cleanup completed. Validated with 19 focused lifecycle/isolation checks and a final rerun of the new boundary regression, check, docs and exact test discovery. The runner test uses real idle providers with controlled SDK/shutdown handles and a close rejection; active Inspector resource consequences, native Linux/Windows and end-to-end stdio shutdown remain unverified. Fixes #5222 --- src/main/transport.ts | 15 +- .../mcp/optionalProviderShutdown.test.ts | 134 ++++++++++++++++++ 2 files changed, 140 insertions(+), 9 deletions(-) create mode 100644 tests/boundary/mcp/optionalProviderShutdown.test.ts diff --git a/src/main/transport.ts b/src/main/transport.ts index ecc1c8e6a1..c26e51a77d 100644 --- a/src/main/transport.ts +++ b/src/main/transport.ts @@ -51,9 +51,9 @@ export const startMcpTransport = async ( "Optional MCP providers could not load; affected tools remain unavailable", ); } - for (const failure of Object.values( - optionalProviders.optionalProviderLoadFailures ?? {}, - )) { + const { optionalProviderLoadFailures, ...observationProviders } = + optionalProviders; + for (const failure of Object.values(optionalProviderLoadFailures ?? {})) { serverLogger.warn( { providerId: failure.providerId, error: failure.reason }, "Optional MCP adapter could not load; its peers remain available", @@ -81,12 +81,9 @@ export const startMcpTransport = async ( // Optional providers are shared by every server instance the SDK creates, // including a discarded discovery probe, so only process shutdown closes them. const optionalProvidersToClose = new Set( - [ - optionalProviders.browserObservation, - optionalProviders.browserScenarioCapture, - optionalProviders.electronObservation, - optionalProviders.electronActiveObservation, - ].filter((provider) => provider !== undefined), + Object.values(observationProviders).filter( + (provider) => provider !== undefined, + ), ); let optionalProviderClose: Promise | undefined; const closeOptionalProviders = async (): Promise => { diff --git a/tests/boundary/mcp/optionalProviderShutdown.test.ts b/tests/boundary/mcp/optionalProviderShutdown.test.ts new file mode 100644 index 0000000000..142af125f0 --- /dev/null +++ b/tests/boundary/mcp/optionalProviderShutdown.test.ts @@ -0,0 +1,134 @@ +import { afterAll, expect, it, vi } from "vitest"; +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { resolve } from "node:path"; + +import { createTestBinarySession } from "../../fixtures/binarySession.js"; +import { createBrowserObservationProvider } from "../../../src/composition/browserObservation.js"; +import { createJavaScriptRuntimeObservationProvider } from "../../../src/composition/javascriptRuntimeObservation.js"; +import { run } from "../../../src/main.js"; +import { MCP_SHUTDOWN_FAILED } from "../../../src/main/messages.js"; +import { createServer } from "../../../src/server/createServer.js"; + +mkdirSync(".cache", { recursive: true }); +const root = mkdtempSync(resolve(".cache/optional-provider-shutdown-")); +const environment = { + HOME: `${root}/home`, + XDG_CONFIG_HOME: `${root}/config`, + XDG_CACHE_HOME: `${root}/cache`, + REA_LOG_LEVEL: "silent", +}; +for (const path of [ + environment.HOME, + environment.XDG_CONFIG_HOME, + environment.XDG_CACHE_HOME, +]) + mkdirSync(path); +afterAll(() => rmSync(root, { recursive: true, force: true })); + +it("retries failed process-owned optional cleanup without reclosing completed peers", async () => { + const runtime = createJavaScriptRuntimeObservationProvider(); + const browser = createBrowserObservationProvider(); + const closeRuntime = runtime.close?.bind(runtime); + const runtimeClose = vi.spyOn(runtime, "close"); + const browserClose = vi.spyOn(browser, "close"); + if (closeRuntime === undefined) throw new Error("Runtime close hook missing"); + runtimeClose.mockImplementation(async () => { + if (runtimeClose.mock.calls.length === 1) + throw new Error("controlled runtime cleanup failure"); + await closeRuntime(); + }); + + const callerOwnedSession = createTestBinarySession(() => { + throw new Error("No analysis target is allowed in this lifecycle test"); + }); + const callerOwnedServer = createServer( + { kind: "session", session: callerOwnedSession }, + { + environment, + javascriptRuntimeObservation: runtime, + optionalObservationProviderOwner: "caller", + }, + ); + try { + await callerOwnedServer.close(); + expect(runtimeClose).not.toHaveBeenCalled(); + } finally { + const closed = await callerOwnedSession.close(); + if (!closed.ok) throw closed.error; + } + + let shutdown: (() => void) | undefined; + let servers = 0; + let transportCloses = 0; + let unregisterCalls = 0; + const stderr: string[] = []; + const exitCodes: number[] = []; + const optionalProviderLoadFailures = { + browserScenarioCapture: { + providerId: "playwright-browser-scenario", + reason: "controlled optional provider load failure", + }, + }; + + try { + const result = await run({ + env: environment, + loadOptionalProviders: async () => ({ + browserObservation: browser, + javascriptRuntimeObservation: runtime, + optionalProviderLoadFailures, + }), + createServer: (source, options) => { + expect(options?.optionalObservationProviderOwner).toBe("caller"); + expect(options?.javascriptRuntimeObservation).toBe(runtime); + expect(options?.optionalProviderLoadFailures).toBe( + optionalProviderLoadFailures, + ); + servers += 1; + return createServer(source, options); + }, + serve: (factory) => { + const server = factory({ era: "legacy" }); + return { + close: async () => { + transportCloses += 1; + await (await server).close(); + }, + }; + }, + registerShutdown: (handler) => { + shutdown = handler; + return () => { + unregisterCalls += 1; + }; + }, + writeStderr: (text) => stderr.push(text), + setExitCode: (code) => exitCodes.push(code), + }); + expect(result).toBe(0); + if (shutdown === undefined) + throw new Error("Shutdown hook was not installed"); + + shutdown(); + await vi.waitFor(() => expect(runtimeClose).toHaveBeenCalledTimes(1)); + await vi.waitFor(() => expect(exitCodes).toEqual([1])); + expect(browserClose).toHaveBeenCalledTimes(1); + expect(transportCloses).toBe(1); + expect(servers).toBe(1); + expect(unregisterCalls).toBe(0); + + shutdown(); + await vi.waitFor(() => expect(unregisterCalls).toBe(1)); + + expect(runtimeClose).toHaveBeenCalledTimes(2); + expect(browserClose).toHaveBeenCalledTimes(1); + expect(transportCloses).toBe(1); + expect(exitCodes).toEqual([1]); + expect(stderr).toContain(`${MCP_SHUTDOWN_FAILED}\n`); + } finally { + runtimeClose.mockRestore(); + browserClose.mockRestore(); + await runtime.close?.(); + await browser.close?.(); + } +});