diff --git a/src/mcpDoctor.ts b/src/mcpDoctor.ts index 6b1652bc38..60cd3a9b72 100644 --- a/src/mcpDoctor.ts +++ b/src/mcpDoctor.ts @@ -1,5 +1,4 @@ import { Client, SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; -import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { Help } from "incur"; import { performance } from "node:perf_hooks"; import { resolve } from "node:path"; @@ -37,6 +36,7 @@ import { mcpDoctorDeadlineRecovery, } from "./mcpStartupPolicy.js"; import { MCP_DOCTOR_ALIAS, MCP_DOCTOR_OPTIONS } from "./mcpDoctorOptions.js"; +import { OwnedMcpStdioTransport } from "./process/OwnedMcpStdioTransport.js"; const OUTPUT_FORMATS = CLI_DOCUMENT_OUTPUT_FORMATS; type OutputFormat = (typeof OUTPUT_FORMATS)[number]; @@ -120,14 +120,15 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { Math.max(1, Math.ceil(deadline - performance.now())), ); const stderr = captureStderr(); - const transport = new OwnedStdioClientTransport({ - command: options.command, - args: [...options.args], - cwd: options.cwd, - env: targetFreeEnvironment(options.environment), - stderr: "pipe", - }); - transport.stderr?.on("data", stderr.append); + const transport = new DoctorStdioTransport( + { + command: options.command, + arguments: [...options.args], + ...(options.cwd === undefined ? {} : { cwd: options.cwd }), + env: targetFreeEnvironment(options.environment), + }, + stderr.append, + ); const client = new Client({ name: "rea-mcp-doctor", version: "1" }); let result: ProductionMcpDoctorResult; let cleanupFailure: { readonly cause: unknown } | undefined; @@ -195,6 +196,8 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { }, ], }; + // The owned transport bounds what it retains; this reporter keeps the exact + // prefix semantics the report promises, including a withheld partial character. return { ...result, diagnostics: stderr.value() }; }; @@ -213,13 +216,24 @@ type ProductionMcpDoctorResult = readonly checks: readonly McpDoctorCheck[]; }; -/** Join SDK-triggered and caller-triggered close calls for the same child. */ -class OwnedStdioClientTransport extends StdioClientTransport { - #closePromise: Promise | undefined; - - override close(): Promise { - this.#closePromise ??= super.close(); - return this.#closePromise; +/** The doctor's bounded stdio session, owning the process family it starts. */ +class DoctorStdioTransport extends OwnedMcpStdioTransport { + constructor( + options: ConstructorParameters[0], + onStderr: (chunk: unknown) => void, + ) { + super(options, { + provider: "mcp-doctor", + label: "MCP doctor", + maxFrameBytes: 8 * 1024 * 1024, + maxOperationBytes: 32 * 1024 * 1024, + // Retain only stderr, with the byte budget the report promises. + maxDiagnosticBytes: { + stdout: 0, + stderr: MCP_STARTUP_POLICY.doctorStderrBytes, + }, + onStderr, + }); } } diff --git a/src/process/OwnedMcpStdioTransport.ts b/src/process/OwnedMcpStdioTransport.ts index 66800a609b..0e66a90d20 100644 --- a/src/process/OwnedMcpStdioTransport.ts +++ b/src/process/OwnedMcpStdioTransport.ts @@ -8,6 +8,7 @@ import { randomUUID } from "node:crypto"; import { finished } from "node:stream/promises"; import type { OwnedProviderProcessSpawnOptions, + ProcessDiagnosticBudget, SpawnedOwnedProviderProcess, } from "./ProviderProcess.js"; import type { AnalysisCapturedOutput } from "../domain/analysisErrorBase.js"; @@ -62,7 +63,9 @@ export class OwnedMcpStdioTransport implements Transport { readonly maxFrameBytes: number; readonly maxOperationBytes: number; readonly gracefulInputCloseMs?: number; - readonly maxDiagnosticBytes?: number; + readonly maxDiagnosticBytes?: ProcessDiagnosticBudget; + /** Observe the owned child's stderr from acquisition onward, including when it closes early. */ + readonly onStderr?: (chunk: unknown) => void; readonly notifyOnExit?: boolean; }, readonly launcher: OwnedMcpLauncher = spawnOwnedProviderProcess, @@ -113,6 +116,8 @@ export class OwnedMcpStdioTransport implements Transport { spawned.process.stdout.on("data", this.#read); spawned.process.stdout.on("error", this.#error); spawned.process.stderr?.on("data", this.#countStderr); + if (this.settings.onStderr !== undefined) + spawned.process.stderr?.on("data", this.settings.onStderr); spawned.process.on("error", this.#error); spawned.process.once("exit", this.#exit); spawned.process.once("close", this.#outputClosed); @@ -194,6 +199,8 @@ export class OwnedMcpStdioTransport implements Transport { process?.stdout?.off("data", this.#read); process?.stdout?.off("error", this.#error); process?.stderr?.off("data", this.#countStderr); + if (this.settings.onStderr !== undefined) + process?.stderr?.off("data", this.settings.onStderr); process?.off("error", this.#error); process?.off("exit", this.#exit); process?.off("close", this.#outputClosed); diff --git a/tests/boundary/mcp/mcpDoctor.test.ts b/tests/boundary/mcp/mcpDoctor.test.ts index 7ef05b066a..5b4299e58c 100644 --- a/tests/boundary/mcp/mcpDoctor.test.ts +++ b/tests/boundary/mcp/mcpDoctor.test.ts @@ -770,6 +770,42 @@ describe("production MCP doctor child lifecycle", () => { expect(result.checks[0]?.detail).not.toContain("REA_MCP_STARTUP"); await expect(waitForExit(pid)).resolves.toBeUndefined(); }); + + it("stops the owned process family when a descendant outlives the launcher", async () => { + const root = await createTestTempDirectory("rea-mcp-doctor-descendant-"); + const pidPath = join(root, "pids.json"); + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + const pending = runProductionMcpDoctor({ + command: process.execPath, + args: [resolve("tests/fixtures/mcpDoctorForkDescendant.mjs")], + cwd: process.cwd(), + environment: { + ...process.env, + REA_MCP_DOCTOR_PID_PATH: pidPath, + REA_MCP_DOCTOR_DEADLINE_MS: "60000", + }, + }); + const pids = await vi.waitFor( + async () => { + const value = JSON.parse(await readFile(pidPath, "utf8")) as { + launcher: number; + descendant: number; + }; + expect(Number.isSafeInteger(value.descendant)).toBe(true); + return value; + }, + { timeout: 10_000 }, + ); + await vi.advanceTimersByTimeAsync(60_000); + const result = await vi.waitFor(() => pending, { timeout: 10_000 }); + vi.useRealTimers(); + // The launcher exits at once, so only owned cleanup can stop the descendant; + // a joined SDK close would leave it running and report no cleanup failure. + expect(result.healthy).toBe(false); + expect(result.checks.map(({ name }) => name)).toEqual(["transport"]); + expectDoctorDeadline(result.checks[0]?.detail, 60_000); + await expect(waitForExit(pids.descendant)).resolves.toBeUndefined(); + }); }); describe("production MCP doctor request-flow deadline", () => { diff --git a/tests/fixtures/mcpDoctorForkDescendant.mjs b/tests/fixtures/mcpDoctorForkDescendant.mjs new file mode 100644 index 0000000000..2a5bbf3505 --- /dev/null +++ b/tests/fixtures/mcpDoctorForkDescendant.mjs @@ -0,0 +1,14 @@ +import { spawn } from "node:child_process"; +import { writeFile } from "node:fs/promises"; + +// Fork an inherited-stdio descendant, publish both PIDs, then exit so the +// doctor sees stdin EOF and a dead launcher while the descendant keeps running. +const descendant = spawn(process.execPath, ["-e", "setInterval(() => {}, 1_000)"], { + stdio: "inherit", +}); +if (process.env.REA_MCP_DOCTOR_PID_PATH !== undefined) + await writeFile( + process.env.REA_MCP_DOCTOR_PID_PATH, + JSON.stringify({ launcher: process.pid, descendant: descendant.pid }), + ); +process.exit(0);