From d68d03e88f49a939a0c99daff631dc0fdeb41a9c Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 24 Oct 2026 14:49:13 +0800 Subject: [PATCH] test(mcp): consolidate fixture connection and cleanup ownership Route application and provider-environment harnesses through the shared MCP connection owner. Join accepted handshakes during shutdown, retain each failed close for retry, preserve primary and independent cleanup failures, and release provider files only after their session closes. Surface BinarySession close Result errors in production and artifact fixture teardown. Add real SDK and descriptor-boundary regressions for startup races, concurrent cleanup, mixed failures and recovery. Use a provider-free plist for descriptor ownership so installed deep engines cannot change the test outcome. --- .../applicationMcpHarnessLifecycle.test.ts | 188 +++++++++ .../boundary/mcp/mcpFixtureLifecycle.test.ts | 367 ++++++++++++++++++ ...reverseDebuggerMcpHarnessLifecycle.test.ts | 131 +++++++ .../mcp/sessionFixtureCleanup.test.ts | 120 ++++++ tests/fixtures/applicationMcpHarness.ts | 79 ++-- tests/support/mcp/artifactSession.ts | 3 +- tests/support/mcp/mcpFixture.ts | 131 +++++-- tests/support/mcp/productionSession.ts | 3 +- .../support/reverseDebuggerMcpTestHelpers.ts | 69 +++- 9 files changed, 1019 insertions(+), 72 deletions(-) create mode 100644 tests/boundary/mcp/applicationMcpHarnessLifecycle.test.ts create mode 100644 tests/boundary/mcp/mcpFixtureLifecycle.test.ts create mode 100644 tests/boundary/mcp/reverseDebuggerMcpHarnessLifecycle.test.ts create mode 100644 tests/boundary/mcp/sessionFixtureCleanup.test.ts diff --git a/tests/boundary/mcp/applicationMcpHarnessLifecycle.test.ts b/tests/boundary/mcp/applicationMcpHarnessLifecycle.test.ts new file mode 100644 index 0000000000..0003844932 --- /dev/null +++ b/tests/boundary/mcp/applicationMcpHarnessLifecycle.test.ts @@ -0,0 +1,188 @@ +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { expect, it, vi } from "vitest"; + +import type { AnalysisClient } from "../../../src/application/AnalysisProvider.js"; +import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; +import { err, ok } from "../../../src/domain/result.js"; +import { + createApplicationMcpHarness, + type ApplicationMcpHarness, +} from "../../fixtures/applicationMcpHarness.js"; +import { + createCacheProvider, + createDeferred, + createTestBinarySession, +} from "../../fixtures/binarySession.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const failureLeaves = (reason: unknown): unknown[] => + reason instanceof AggregateError + ? reason.errors.flatMap(failureLeaves) + : [reason]; + +const acquireProvider = async ( + close: AnalysisClient["close"], + serverOptions?: NonNullable< + Parameters[0] + >["serverOptions"], +) => { + const directory = await createTestTempDirectory("rea-mcp-harness-close-"); + const path = join(directory, "target.hop"); + await writeFile(path, "fixture database bytes"); + const operations: string[] = []; + const provider = createCacheProvider(operations); + const createClient = vi.fn(provider.createClient); + provider.createClient = (...args) => ({ ...createClient(...args), close }); + const harness = await createApplicationMcpHarness({ + serverOptions, + session: createTestBinarySession(provider), + }); + expect( + ( + await harness.client.callTool({ + name: "open_binary", + arguments: { path }, + }) + ).isError, + ).not.toBe(true); + expect( + ( + await harness.client.callTool({ + name: "address_name", + arguments: { address: "0x1000", document: "fixture" }, + }) + ).isError, + ).not.toBe(true); + expect(operations).toEqual(["address_name"]); + expect(createClient).toHaveBeenCalledTimes(1); + return { harness, createClient }; +}; + +it("reports a failed session close, joins concurrent attempts and retries the retained provider", async () => { + const failure = new ProviderCleanupError("fixture", ["binary-owner"], { + reason: "controlled binary close denial", + }); + const entered = createDeferred(); + const release = createDeferred(); + let allowClose = false; + const closeProvider = vi.fn(async () => { + entered.resolve(); + await release.promise; + if (!allowClose) return err(failure); + failure.resolveCleanup(); + return ok(null); + }); + let harness: ApplicationMcpHarness | undefined; + try { + const acquired = await acquireProvider(closeProvider); + harness = acquired.harness; + const closing = harness.close(); + const outcome = closing.catch((reason: unknown) => reason); + await entered.promise; + const concurrent = harness.close(); + const concurrentOutcome = concurrent.catch((reason: unknown) => reason); + expect(closeProvider).toHaveBeenCalledTimes(1); + release.resolve(); + const reason: unknown = await outcome; + expect(reason).toBeInstanceOf(AggregateError); + if (!(reason instanceof AggregateError)) + throw new Error("Harness ignored the session close failure"); + expect(reason.errors).toContain(failure); + expect(concurrent).toBe(closing); + expect(await concurrentOutcome).toBe(reason); + expect(failure.cleanupIncomplete).toBe(true); + expect(await harness.session.status()).toMatchObject({ + cleanup_pending: true, + }); + + allowClose = true; + await harness.close(); + expect(acquired.createClient).toHaveBeenCalledTimes(1); + expect(closeProvider).toHaveBeenCalledTimes(2); + expect(failure.cleanupIncomplete).toBe(false); + expect(await harness.session.status()).toMatchObject({ + cleanup_pending: false, + }); + await harness.close(); + expect(closeProvider).toHaveBeenCalledTimes(2); + } finally { + allowClose = true; + release.resolve(); + await harness?.close(); + } +}); + +it("preserves independent MCP provider and session close failures and releases each owner once", async () => { + const binaryFailure = new ProviderCleanupError("fixture", ["binary-owner"], { + reason: "controlled binary close denial", + }); + const adbFailure = new Error("controlled ADB close denial"); + let allowBinaryClose = false; + let allowAdbClose = false; + const closeBinary = vi.fn(async () => { + if (!allowBinaryClose) return err(binaryFailure); + binaryFailure.resolveCleanup(); + return ok(null); + }); + const closeAdb = vi.fn(async () => { + if (!allowAdbClose) throw adbFailure; + }); + let harness: ApplicationMcpHarness | undefined; + try { + const acquired = await acquireProvider(closeBinary, { + adbDeviceAnalysis: { + inspectAvailability: async () => ({ + status: "available", + code: null, + reason: null, + diagnostics: {}, + }), + execute: async () => { + throw new Error("This fixture must not execute ADB operations"); + }, + close: closeAdb, + }, + }); + harness = acquired.harness; + const reason: unknown = await harness + .close() + .catch((cause: unknown) => cause); + expect(reason).toBeInstanceOf(AggregateError); + if (!(reason instanceof AggregateError)) + throw new Error("Harness lost independent cleanup failures"); + expect(failureLeaves(reason)).toEqual( + expect.arrayContaining([binaryFailure, adbFailure]), + ); + expect(failureLeaves(reason)).toHaveLength(2); + expect(closeBinary).toHaveBeenCalledTimes(1); + expect(closeAdb).toHaveBeenCalled(); + expect(await harness.session.status()).toMatchObject({ + cleanup_pending: true, + }); + + allowAdbClose = true; + await expect(harness.close()).rejects.toMatchObject({ + errors: [binaryFailure], + }); + const completedAdbCloses = closeAdb.mock.calls.length; + expect(closeBinary).toHaveBeenCalledTimes(2); + allowBinaryClose = true; + await harness.close(); + expect(acquired.createClient).toHaveBeenCalledTimes(1); + expect(closeBinary).toHaveBeenCalledTimes(3); + expect(closeAdb).toHaveBeenCalledTimes(completedAdbCloses); + expect(binaryFailure.cleanupIncomplete).toBe(false); + expect(await harness.session.status()).toMatchObject({ + cleanup_pending: false, + }); + await harness.close(); + expect(closeBinary).toHaveBeenCalledTimes(3); + expect(closeAdb).toHaveBeenCalledTimes(completedAdbCloses); + } finally { + allowBinaryClose = true; + allowAdbClose = true; + await harness?.close(); + } +}); diff --git a/tests/boundary/mcp/mcpFixtureLifecycle.test.ts b/tests/boundary/mcp/mcpFixtureLifecycle.test.ts new file mode 100644 index 0000000000..4f8becd064 --- /dev/null +++ b/tests/boundary/mcp/mcpFixtureLifecycle.test.ts @@ -0,0 +1,367 @@ +import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import { McpServer } from "@modelcontextprotocol/server"; +import { expect, it, vi } from "vitest"; + +import { createDeferred } from "../../fixtures/binarySession.js"; +import { createTestMcp } from "../../support/mcp/mcpFixture.js"; + +/** Keep real SDK objects recoverable even when a red ownership assertion fails. */ +const createSdkFixture = () => { + const mcp = createTestMcp(); + const clientClose = Client.prototype.close; + const clientConnects = vi.spyOn(Client.prototype, "connect"); + const clientCloses = vi.spyOn(Client.prototype, "close"); + const serverCloses: Array<() => Promise> = []; + return { + mcp, + clientCloses, + server(name: string) { + const server = new McpServer({ name, version: "1" }); + serverCloses.push(server.close.bind(server)); + return server; + }, + async dispose() { + const clients = new Set([ + ...clientConnects.mock.contexts, + ...clientCloses.mock.contexts, + ]); + clientConnects.mockRestore(); + clientCloses.mockRestore(); + const outcomes = await Promise.allSettled([ + ...[...clients].map((client) => clientClose.call(client)), + ...serverCloses.map((close) => close()), + ]); + const failures = outcomes.flatMap((outcome) => + outcome.status === "rejected" ? [outcome.reason as unknown] : [], + ); + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) + throw new AggregateError(failures, "SDK fixture cleanup failed"); + }, + }; +}; + +it("joins an accepted late handshake and refuses to publish it after shutdown", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("late-handshake"); + const connected = createDeferred(); + const release = createDeferred(); + const connect = server.connect.bind(server); + server.connect = async (transport) => { + await connect(transport); + connected.resolve(); + await release.promise; + }; + const serverClose = vi.spyOn(server, "close"); + const opening = fixture.mcp.connect(server).then( + (client) => ({ ok: true as const, client }), + (reason: unknown) => ({ ok: false as const, reason }), + ); + let closeSettled = false; + let closing: Promise | undefined; + try { + await connected.promise; + closing = fixture.mcp.close().then(() => { + closeSettled = true; + }); + // The producer is held, so a shutdown with no pending acquisition would + // already settle through these promise continuations. + await Promise.resolve(); + await Promise.resolve(); + expect(closeSettled).toBe(false); + release.resolve(); + expect(await opening).toMatchObject({ ok: false }); + await closing; + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + } finally { + release.resolve(); + await opening; + await closing?.catch(() => undefined); + await fixture.dispose(); + } +}); + +it("registers acquisition before a producer can reenter shutdown and attach its transport late", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("reentrant-shutdown"); + const entered = createDeferred(); + const release = createDeferred(); + const connect = server.connect.bind(server); + const close = server.close.bind(server); + const lifecycle: string[] = []; + let closing: Promise | undefined; + let closeSettled = false; + server.connect = async (transport) => { + closing = fixture.mcp.close().then(() => { + closeSettled = true; + }); + entered.resolve(); + await release.promise; + await connect(transport); + lifecycle.push("attached"); + }; + const serverClose = vi.spyOn(server, "close").mockImplementation(async () => { + lifecycle.push("closed"); + await close(); + }); + const opening = fixture.mcp.connect(server).then( + (client) => ({ ok: true as const, client }), + (reason: unknown) => ({ ok: false as const, reason }), + ); + try { + await entered.promise; + await Promise.resolve(); + await Promise.resolve(); + expect(closeSettled).toBe(false); + expect(serverClose).not.toHaveBeenCalled(); + expect(fixture.clientCloses).not.toHaveBeenCalled(); + release.resolve(); + const outcome = await opening; + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("Shutdown published a late MCP client"); + expect(outcome.reason).toMatchObject({ + message: expect.stringMatching(/closed/iu), + }); + await closing; + expect(closeSettled).toBe(true); + expect(lifecycle).toEqual(["attached", "closed"]); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + } finally { + release.resolve(); + await opening; + await closing?.catch(() => undefined); + await fixture.dispose(); + } +}); + +it("shares one concurrent close attempt and does not reclose released SDK objects", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("concurrent-close"); + const entered = createDeferred(); + const release = createDeferred(); + let first: Promise | undefined; + let second: Promise | undefined; + try { + const client = await fixture.mcp.connect(server); + await expect(client.ping()).resolves.toEqual({}); + const close = server.close.bind(server); + const serverClose = vi + .spyOn(server, "close") + .mockImplementation(async () => { + entered.resolve(); + await release.promise; + await close(); + }); + first = fixture.mcp.close(); + await entered.promise; + second = fixture.mcp.close(); + await Promise.resolve(); + await Promise.resolve(); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + release.resolve(); + await Promise.all([first, second]); + await fixture.mcp.close(); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + } finally { + release.resolve(); + await Promise.allSettled([first, second]); + await fixture.dispose(); + } +}); + +it("cancels a real initialize handshake before awaiting its held server handler", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("stalled-initialize"); + const entered = createDeferred(); + const release = createDeferred(); + server.server.setRequestHandler("initialize", async () => { + entered.resolve(); + await release.promise; + throw new Error("Held initialize handler released during test cleanup"); + }); + let openingSettled = false; + const opening = fixture.mcp.connect(server).then( + (client) => { + openingSettled = true; + return { ok: true as const, client }; + }, + (reason: unknown) => { + openingSettled = true; + return { ok: false as const, reason }; + }, + ); + const serverClose = vi.spyOn(server, "close"); + try { + await entered.promise; + await fixture.mcp.close(); + expect(openingSettled).toBe(true); + expect(await opening).toMatchObject({ ok: false }); + expect(serverClose).toHaveBeenCalledTimes(1); + } finally { + release.resolve(); + await opening; + await fixture.dispose(); + } +}); + +it("keeps a failed handshake as the exact error when its SDK cleanup succeeds", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("failed-handshake"); + const startup = new Error("controlled server handshake failure"); + const connect = server.connect.bind(server); + server.connect = async (transport) => { + await connect(transport); + throw startup; + }; + const serverClose = vi.spyOn(server, "close"); + try { + await expect(fixture.mcp.connect(server)).rejects.toBe(startup); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + await fixture.mcp.close(); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + } finally { + await fixture.dispose(); + } +}); + +it.each([new Error("controlled server close failure"), undefined])( + "retains failed handshake cleanup and retries its exact owner: %s", + async (cleanup) => { + const fixture = createSdkFixture(); + const server = fixture.server("failed-handshake-cleanup"); + const startup = new Error("controlled server handshake failure"); + const connect = server.connect.bind(server); + const close = server.close.bind(server); + let denied = true; + server.connect = async (transport) => { + await connect(transport); + throw startup; + }; + const serverClose = vi + .spyOn(server, "close") + .mockImplementation(async () => { + if (denied) throw cleanup; + await close(); + }); + try { + const error: unknown = await fixture.mcp + .connect(server) + .catch((reason: unknown) => reason); + expect(error).toBeInstanceOf(AggregateError); + if (!(error instanceof AggregateError)) + throw new Error("Expected startup and retained cleanup failures"); + expect(error.cause).toBe(startup); + expect(error.errors).toEqual(expect.arrayContaining([startup, cleanup])); + expect(error.errors).toHaveLength(2); + expect(serverClose).toHaveBeenCalledTimes(1); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + await expect(fixture.mcp.close()).rejects.toBe(cleanup); + expect(serverClose).toHaveBeenCalledTimes(2); + denied = false; + await fixture.mcp.close(); + expect(serverClose).toHaveBeenCalledTimes(3); + expect(fixture.clientCloses).toHaveBeenCalledTimes(1); + await fixture.mcp.close(); + expect(serverClose).toHaveBeenCalledTimes(3); + } finally { + denied = false; + await fixture.dispose(); + } + }, +); + +it("attempts every SDK owner, reports all close failures and retries only unresolved resources", async () => { + const fixture = createSdkFixture(); + const firstServer = fixture.server("mixed-close-first"); + const secondServer = fixture.server("mixed-close-second"); + const clientFailure = new Error("controlled client close failure"); + const serverFailure = new Error("controlled server close failure"); + let denyClient = true; + let denyServer = true; + try { + const firstClient = await fixture.mcp.connect(firstServer); + const secondClient = await fixture.mcp.connect(secondServer); + await expect(firstClient.ping()).resolves.toEqual({}); + await expect(secondClient.ping()).resolves.toEqual({}); + const closeClient = firstClient.close.bind(firstClient); + const closeServer = firstServer.close.bind(firstServer); + const firstClientClose = vi.fn(async () => { + if (denyClient) throw clientFailure; + await closeClient(); + }); + firstClient.close = firstClientClose; + const firstServerClose = vi + .spyOn(firstServer, "close") + .mockImplementation(async () => { + if (denyServer) throw serverFailure; + await closeServer(); + }); + const closeSecondClient = secondClient.close.bind(secondClient); + const secondClientClose = vi.fn(closeSecondClient); + secondClient.close = secondClientClose; + const secondServerClose = vi.spyOn(secondServer, "close"); + const error: unknown = await fixture.mcp + .close() + .catch((reason: unknown) => reason); + expect(error).toBeInstanceOf(AggregateError); + if (!(error instanceof AggregateError)) + throw new Error("Expected both independent SDK close failures"); + expect(error.errors).toEqual( + expect.arrayContaining([clientFailure, serverFailure]), + ); + expect(error.errors).toHaveLength(2); + for (const closed of [ + firstClientClose, + firstServerClose, + secondClientClose, + secondServerClose, + ]) + expect(closed).toHaveBeenCalledTimes(1); + + denyClient = false; + await expect(fixture.mcp.close()).rejects.toBe(serverFailure); + expect(firstClientClose).toHaveBeenCalledTimes(2); + expect(firstServerClose).toHaveBeenCalledTimes(2); + expect(secondClientClose).toHaveBeenCalledTimes(1); + expect(secondServerClose).toHaveBeenCalledTimes(1); + + denyServer = false; + await fixture.mcp.close(); + await fixture.mcp.close(); + expect(firstClientClose).toHaveBeenCalledTimes(2); + expect(firstServerClose).toHaveBeenCalledTimes(3); + expect(secondClientClose).toHaveBeenCalledTimes(1); + expect(secondServerClose).toHaveBeenCalledTimes(1); + } finally { + denyClient = false; + denyServer = false; + await fixture.dispose(); + } +}); + +it("rejects new connections after shutdown before acquiring a pair and leaves the unadmitted server caller-owned", async () => { + const fixture = createSdkFixture(); + const server = fixture.server("unadmitted-server"); + const pair = vi.spyOn(InMemoryTransport, "createLinkedPair"); + const serverConnect = vi.spyOn(server, "connect"); + const serverClose = vi.spyOn(server, "close"); + try { + await fixture.mcp.close(); + await expect(fixture.mcp.connect(server)).rejects.toThrow(/closed/iu); + expect(pair).not.toHaveBeenCalled(); + expect(serverConnect).not.toHaveBeenCalled(); + expect(serverClose).not.toHaveBeenCalled(); + expect(fixture.clientCloses).not.toHaveBeenCalled(); + await server.close(); + expect(serverClose).toHaveBeenCalledTimes(1); + } finally { + pair.mockRestore(); + await fixture.dispose(); + } +}); diff --git a/tests/boundary/mcp/reverseDebuggerMcpHarnessLifecycle.test.ts b/tests/boundary/mcp/reverseDebuggerMcpHarnessLifecycle.test.ts new file mode 100644 index 0000000000..d2e27bb976 --- /dev/null +++ b/tests/boundary/mcp/reverseDebuggerMcpHarnessLifecycle.test.ts @@ -0,0 +1,131 @@ +import { access, mkdtemp, readFile, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { McpServer } from "@modelcontextprotocol/server"; +import { expect, it, onTestFinished, vi } from "vitest"; + +import { createProviderEnvironmentMcpHarness } from "../../support/reverseDebuggerMcpTestHelpers.js"; + +const providerSource = `#!${process.execPath}\nprocess.stdout.write("selected provider\\n");\n`; +const inputSource = "selected input bytes\n"; + +const selectedProviderFiles = async () => { + const directory = await mkdtemp(join(tmpdir(), "rea-provider-owner-")); + const provider = join(directory, "selected-rizin"); + const input = join(directory, "fixture.bin"); + await Promise.all([ + writeFile(provider, providerSource, { mode: 0o700 }), + writeFile(input, inputSource), + ]); + return { directory, provider, input }; +}; + +const providerOptions = (provider: string, close: () => Promise) => ({ + environment: { REA_RIZIN_COMMAND: provider }, + adbDeviceAnalysis: { + inspectAvailability: async () => ({ + status: "available" as const, + code: null, + reason: null, + diagnostics: {}, + }), + execute: async () => { + throw new Error("This fixture must not execute ADB operations"); + }, + close, + }, +}); + +const failureLeaves = (reason: unknown): unknown[] => + reason instanceof AggregateError + ? reason.errors.flatMap(failureLeaves) + : [reason]; + +it("retains selected provider files after failed close and removes them after owner recovery", async () => { + const files = await selectedProviderFiles(); + const closeFailure = new Error("controlled provider close denial"); + let allowClose = false; + const closeProvider = vi.fn(async () => { + if (!allowClose) throw closeFailure; + }); + let harness: + | Awaited> + | undefined; + try { + harness = await createProviderEnvironmentMcpHarness({ + directory: files.directory, + clientName: "provider-owner-close", + serverOptions: providerOptions(files.provider, closeProvider), + }); + await expect(harness.client.ping()).resolves.toEqual({}); + const reason: unknown = await harness + .close() + .catch((cause: unknown) => cause); + expect(failureLeaves(reason)).toContain(closeFailure); + expect(await readFile(files.provider, "utf8")).toBe(providerSource); + expect(await readFile(files.input, "utf8")).toBe(inputSource); + expect(closeProvider).toHaveBeenCalled(); + + allowClose = true; + await harness.close(); + await expect(access(files.directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + const completedCloses = closeProvider.mock.calls.length; + await harness.close(); + expect(closeProvider).toHaveBeenCalledTimes(completedCloses); + } finally { + allowClose = true; + await harness?.close(); + } +}); + +it("retains selected files when attached MCP startup and provider cleanup both fail before publication", async () => { + const files = await selectedProviderFiles(); + const startupFailure = new Error("controlled attached MCP startup failure"); + const closeFailure = new Error("controlled startup cleanup denial"); + let allowClose = false; + const closeProvider = vi.fn(async () => { + if (!allowClose) throw closeFailure; + }); + const connect = McpServer.prototype.connect; + const startup = vi + .spyOn(McpServer.prototype, "connect") + .mockImplementationOnce(async function (this: McpServer, ...args) { + await connect.apply(this, args); + throw startupFailure; + }); + let harness: + | Awaited> + | undefined; + onTestFinished(async () => { + await expect(access(files.directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + try { + let reason: unknown; + try { + harness = await createProviderEnvironmentMcpHarness({ + directory: files.directory, + clientName: "provider-owner-startup", + serverOptions: providerOptions(files.provider, closeProvider), + }); + } catch (cause) { + reason = cause; + } + expect(harness).toBeUndefined(); + expect(startup).toHaveBeenCalledTimes(1); + expect(failureLeaves(reason)).toEqual( + expect.arrayContaining([startupFailure, closeFailure]), + ); + expect(closeProvider).toHaveBeenCalled(); + expect(await readFile(files.provider, "utf8")).toBe(providerSource); + expect(await readFile(files.input, "utf8")).toBe(inputSource); + } finally { + allowClose = true; + startup.mockRestore(); + await harness?.close(); + } +}); diff --git a/tests/boundary/mcp/sessionFixtureCleanup.test.ts b/tests/boundary/mcp/sessionFixtureCleanup.test.ts new file mode 100644 index 0000000000..6315ef6715 --- /dev/null +++ b/tests/boundary/mcp/sessionFixtureCleanup.test.ts @@ -0,0 +1,120 @@ +import { stat, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { expect, vi, type OnTestFinishedHandler } from "vitest"; + +import { closeVerifierResources } from "../../../scripts/lib/verifier-cleanup.mjs"; +import { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; +import { OwnedFileHandle } from "../../../src/filesystem/OwnedFileHandle.js"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { openArtifactTarget } from "../../support/mcp/artifactSession.js"; +import { mcpTest } from "../../support/mcp/mcpFixture.js"; +import { openProductionTarget } from "../../support/mcp/productionSession.js"; + +mcpTest.for([ + { name: "production", open: openProductionTarget }, + { name: "artifact", open: openArtifactTarget }, +])( + "$name session fixture reports failed teardown and retries its exact descriptor owner", + async ({ open }, { mcp, task }) => { + const directory = await createTestTempDirectory( + "rea-session-fixture-cleanup-", + ); + // Descriptor ownership needs real target bytes, without selecting an + // installed deep-analysis engine whose availability varies by host. + const path = join(directory, "target.plist"); + await writeFile(path, ''); + const identity = await stat(path); + let cleanup: OnTestFinishedHandler | undefined; + let retained: OwnedFileHandle | undefined; + let denied = true; + let deniedAttempts = 0; + let restoreClose = () => {}; + try { + const client = await open( + mcp, + (handler) => { + cleanup = handler; + }, + path, + ); + if (cleanup === undefined) + throw new Error("Session fixture did not register teardown"); + const nativeClose = OwnedFileHandle.prototype.close; + const closeFailure = new Error("controlled fixture target close denial"); + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + if (retained === undefined && this.handle.fd >= 0) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) + retained = this; + } + if (this === retained && denied) { + deniedAttempts += 1; + throw closeFailure; + } + await nativeClose.call(this); + }); + restoreClose = () => close.mockRestore(); + + // A successful admission has already closed its read descriptor. Reopen + // the same target to leave a real failed close owned by this session. + const failedOpen = parseMcpToolError( + await client.callTool({ name: "open_binary", arguments: { path } }), + ); + expect(failedOpen.error).toMatchObject({ + code: "cleanup_incomplete", + details: { + resources: [path], + cleanup_reason: expect.stringContaining(closeFailure.message), + }, + }); + if (retained === undefined) + throw new Error( + "Target close denial did not retain a descriptor owner", + ); + const owner = retained; + const descriptor = owner.handle.fd; + expect(descriptor).toBeGreaterThanOrEqual(0); + expect(await owner.handle.stat()).toMatchObject({ + dev: identity.dev, + ino: identity.ino, + }); + + const beforeFirstCleanup = deniedAttempts; + await expect(cleanup(task.context)).rejects.toBeInstanceOf(AnalysisError); + expect(deniedAttempts).toBeGreaterThan(beforeFirstCleanup); + expect(retained).toBe(owner); + expect(owner.handle.fd).toBe(descriptor); + + const beforeRetry = deniedAttempts; + await expect(cleanup(task.context)).rejects.toMatchObject({ + cleanupIncomplete: true, + cleanupResources: [path], + diagnostics: { reason: expect.stringContaining(closeFailure.message) }, + }); + expect(deniedAttempts).toBeGreaterThan(beforeRetry); + expect(retained).toBe(owner); + expect(owner.handle.fd).toBe(descriptor); + + denied = false; + restoreClose(); + await cleanup(task.context); + expect(owner.handle.fd).toBe(-1); + await expect(owner.handle.stat()).rejects.toMatchObject({ + code: "EBADF", + }); + } finally { + denied = false; + restoreClose(); + await closeVerifierResources([ + async () => { + await cleanup?.(task.context); + }, + () => mcp.close(), + ]); + } + }, +); diff --git a/tests/fixtures/applicationMcpHarness.ts b/tests/fixtures/applicationMcpHarness.ts index 95196e6218..03ef8320df 100644 --- a/tests/fixtures/applicationMcpHarness.ts +++ b/tests/fixtures/applicationMcpHarness.ts @@ -1,10 +1,16 @@ -import { ok as resultOk } from "../../src/domain/result.js"; -import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import type { Client } from "@modelcontextprotocol/client"; +import { onTestFinished } from "vitest"; +import { closeVerifierResources } from "../../scripts/lib/verifier-cleanup.mjs"; +import { ok as resultOk } from "../../src/domain/result.js"; import { createTestBinarySession } from "./binarySession.js"; import type { BinarySession } from "../../src/application/binary/BinarySession.js"; import { observed } from "./analysisExecution.js"; import { createServer } from "../../src/server/createServer.js"; +import { + createTestMcp, + type McpClientIdentity, +} from "../support/mcp/mcpFixture.js"; /** Owned MCP SDK session shared by application workflow scenarios. */ export interface ApplicationMcpHarness { @@ -18,6 +24,8 @@ export async function createApplicationMcpHarness( options: { readonly session?: BinarySession; readonly serverOptions?: Parameters[1]; + readonly clientIdentity?: McpClientIdentity; + readonly registerCleanup?: (close: ApplicationMcpHarness["close"]) => void; } = {}, ): Promise { const session = @@ -26,25 +34,52 @@ export async function createApplicationMcpHarness( execute: () => Promise.resolve(observed(null)), close: () => Promise.resolve(resultOk(null)), })); - const server = createServer( - { kind: "session", session }, - options.serverOptions, - ); - const client = new Client({ - name: "application-workflow-test", - version: "1", - }); - const [clientTransport, serverTransport] = - InMemoryTransport.createLinkedPair(); - - await server.connect(serverTransport); - await client.connect(clientTransport); - - return { - client, - session, - close: async () => { - await Promise.all([client.close(), server.close(), session.close()]); - }, + const mcp = createTestMcp(); + let sessionClosed = false; + let closing: Promise | undefined; + const close = (): Promise => { + if (closing !== undefined) return closing; + const attempt = closeVerifierResources([ + () => mcp.close(), + async () => { + if (sessionClosed) return; + const result = await session.close(); + if (!result.ok) throw result.error; + sessionClosed = true; + }, + ]); + closing = attempt; + void attempt.catch(() => { + if (closing === attempt) closing = undefined; + }); + return attempt; }; + onTestFinished(close); + + try { + options.registerCleanup?.(close); + const server = createServer( + { kind: "session", session }, + options.serverOptions, + ); + const client = await mcp.connect( + server, + options.clientIdentity ?? { + name: "application-workflow-test", + version: "1", + }, + ); + return { client, session, close }; + } catch (cause) { + try { + await close(); + } catch (cleanup) { + throw new AggregateError( + [cause, cleanup], + "Application MCP harness setup and cleanup failed", + { cause }, + ); + } + throw cause; + } } diff --git a/tests/support/mcp/artifactSession.ts b/tests/support/mcp/artifactSession.ts index 53ca015602..ddd8f46118 100644 --- a/tests/support/mcp/artifactSession.ts +++ b/tests/support/mcp/artifactSession.ts @@ -13,7 +13,8 @@ const openSessionTarget = async ( ): Promise => { const session = await createArtifactBinarySession(process.env); onTestFinished(async () => { - await session.close(); + const result = await session.close(); + if (!result.ok) throw result.error; }); const client = await mcp.connect(createServer({ kind: "session", session })); const opened = await client.callTool({ diff --git a/tests/support/mcp/mcpFixture.ts b/tests/support/mcp/mcpFixture.ts index 2eef34a963..340918eec0 100644 --- a/tests/support/mcp/mcpFixture.ts +++ b/tests/support/mcp/mcpFixture.ts @@ -10,6 +10,7 @@ export interface McpClientIdentity { /** Registry for production MCP servers and SDK clients owned by one test. */ export interface TestMcp { + /** Transfer server ownership on admission; a closed registry leaves it with the caller. */ connect(server: McpServer, identity?: McpClientIdentity): Promise; close(): Promise; } @@ -19,41 +20,117 @@ const DEFAULT_CLIENT_IDENTITY: McpClientIdentity = { version: "1.0.0", }; -/** Create an MCP connection registry for a fixture composition root. */ +interface McpConnectionOwner { + readonly client: Client; + readonly server: McpServer; + readonly handshake: AbortController; + clientClosed: boolean; + serverClosed: boolean; +} + +/** Each SDK object stays owned until its own close resolves successfully. */ +const closeConnection = async ( + connection: McpConnectionOwner, +): Promise => { + const results = await Promise.allSettled([ + (async () => { + if (connection.clientClosed) return; + await connection.client.close(); + connection.clientClosed = true; + })(), + (async () => { + if (connection.serverClosed) return; + await connection.server.close(); + connection.serverClosed = true; + })(), + ]); + return results.flatMap((result) => + result.status === "rejected" ? [result.reason as unknown] : [], + ); +}; + +const registryClosed = (): Error => new Error("Test MCP registry is closed"); + +/** Own accepted acquisitions, SDK resources, and failed cleanup through shutdown. */ export const createTestMcp = (): TestMcp => { - const connections: { readonly client: Client; readonly server: McpServer }[] = - []; + const connections = new Set(); + const pending = new Set>(); + let closed = false; + let closing: Promise | undefined; + const release = async ( + connection: McpConnectionOwner, + ): Promise => { + const failures = await closeConnection(connection); + if (connection.clientClosed && connection.serverClosed) + connections.delete(connection); + return failures; + }; return { - connect: async (server, identity = DEFAULT_CLIENT_IDENTITY) => { + connect: (server, identity = DEFAULT_CLIENT_IDENTITY) => { + if (closed) return Promise.reject(registryClosed()); const client = new Client(identity); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); - try { - await server.connect(serverTransport); - await client.connect(clientTransport); - } catch (cause) { - await Promise.allSettled([client.close(), server.close()]); - throw cause; - } - connections.push({ client, server }); - return client; - }, - close: async () => { - const results = await Promise.allSettled( - [...connections].reverse().map(async ({ client, server }) => { - try { - await client.close(); - } finally { - await server.close(); + const connection: McpConnectionOwner = { + client, + server, + handshake: new AbortController(), + clientClosed: false, + serverClosed: false, + }; + connections.add(connection); + const opening = Promise.resolve().then(async () => { + try { + if (closed) throw connection.handshake.signal.reason; + await server.connect(serverTransport); + if (closed) throw connection.handshake.signal.reason; + await client.connect(clientTransport, { + signal: connection.handshake.signal, + }); + if (closed) throw connection.handshake.signal.reason; + return client; + } catch (cause) { + // Shutdown joins this acquisition, then owns its cleanup. Otherwise + // failed startup releases immediately without losing failed owners. + if (!closed) { + const failures = await release(connection); + if (failures.length > 0) + throw new AggregateError( + [cause, ...failures], + "MCP startup and cleanup failed", + { cause }, + ); } - }), + throw cause; + } + }); + pending.add(opening); + void opening.then( + () => pending.delete(opening), + () => pending.delete(opening), ); - const failure = results.find( - (result): result is PromiseRejectedResult => - result.status === "rejected", - ); - if (failure !== undefined) throw failure.reason; + return opening; + }, + close: () => { + closed = true; + if (closing !== undefined) return closing; + for (const connection of connections) + connection.handshake.abort(registryClosed()); + const attempt = (async () => { + await Promise.allSettled(pending); + const failures = ( + await Promise.all([...connections].reverse().map(release)) + ).flat(); + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) + throw new AggregateError(failures, "Test MCP cleanup failed"); + })(); + closing = attempt; + void attempt.catch(() => { + if (closing === attempt) closing = undefined; + }); + return attempt; }, }; }; diff --git a/tests/support/mcp/productionSession.ts b/tests/support/mcp/productionSession.ts index 38ce87e093..ea09130b76 100644 --- a/tests/support/mcp/productionSession.ts +++ b/tests/support/mcp/productionSession.ts @@ -21,7 +21,8 @@ export const openProductionTarget = async ( process.env, ); onTestFinished(async () => { - await session.close(); + const result = await session.close(); + if (!result.ok) throw result.error; }); const client = await mcp.connect(createServer({ kind: "session", session })); const opened = await client.callTool({ diff --git a/tests/support/reverseDebuggerMcpTestHelpers.ts b/tests/support/reverseDebuggerMcpTestHelpers.ts index 5b7762325d..5ba79c869f 100644 --- a/tests/support/reverseDebuggerMcpTestHelpers.ts +++ b/tests/support/reverseDebuggerMcpTestHelpers.ts @@ -1,12 +1,16 @@ import { readFile, rm } from "node:fs/promises"; -import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import type { Client } from "@modelcontextprotocol/client"; import { Ajv2020 } from "ajv/dist/2020.js"; import { expect, onTestFinished } from "vitest"; import { z } from "zod"; import { toolContract } from "../../src/contracts/toolContracts.js"; -import { createServer } from "../../src/server/createServer.js"; +import type { createServer } from "../../src/server/createServer.js"; +import { + createApplicationMcpHarness, + type ApplicationMcpHarness, +} from "../fixtures/applicationMcpHarness.js"; import { createTestBinarySession } from "../fixtures/binarySession.js"; export const waitForStartedFile = async (path: string): Promise => { @@ -47,24 +51,47 @@ export const createProviderEnvironmentMcpHarness = async (options: { readonly clientName: string; readonly serverOptions: NonNullable[1]>; }) => { - const session = createTestBinarySession(() => { - throw new Error("No binary analysis provider should start"); - }); - const server = createServer( - { kind: "session", session }, - options.serverOptions, - ); - const client = new Client({ name: options.clientName, version: "1" }); - onTestFinished(async () => { - await client.close(); - await server.close(); - await session.close(); - await rm(options.directory, { recursive: true, force: true }); - }); + let closeHarness: ApplicationMcpHarness["close"] | undefined; + let directoryOwned = true; + let closing: Promise | undefined; + const close = (): Promise => { + if (closing !== undefined) return closing; + const attempt = (async () => { + await closeHarness?.(); + if (!directoryOwned) return; + await rm(options.directory, { recursive: true, force: true }); + directoryOwned = false; + })(); + closing = attempt; + void attempt.catch(() => { + if (closing === attempt) closing = undefined; + }); + return attempt; + }; + onTestFinished(close); - const [clientTransport, serverTransport] = - InMemoryTransport.createLinkedPair(); - await server.connect(serverTransport); - await client.connect(clientTransport); - return { client }; + try { + const harness = await createApplicationMcpHarness({ + session: createTestBinarySession(() => { + throw new Error("No binary analysis provider should start"); + }), + serverOptions: options.serverOptions, + clientIdentity: { name: options.clientName, version: "1" }, + registerCleanup: (close) => { + closeHarness = close; + }, + }); + return { client: harness.client, close }; + } catch (cause) { + try { + await close(); + } catch (cleanup) { + throw new AggregateError( + [cause, cleanup], + "Provider environment MCP harness setup and cleanup failed", + { cause }, + ); + } + throw cause; + } };