mirror of
https://github.com/morluto/rea.git
synced 2026-11-14 10:43:30 +00:00
fix(mcp-doctor): stop the owned process family on every exit path
runProductionMcpDoctor wrapped the SDK StdioClientTransport and only joined its close calls. That transport clears its direct-child reference, ends stdin and signals only that child, so a server whose launcher exited after forking an inherited-stdio descendant left the descendant running while the doctor reported nothing but the transport deadline. Run the doctor on OwnedMcpStdioTransport, which spawns the child in a dedicated POSIX process group or Windows job and verifies group and token-owned descendant cleanup before close returns. Feed stderr through a sink on acquisition so diagnostics survive an early child exit and still honor the 64 KiB retained prefix and full byte count. Covered by a fixture that forks a descendant, exits, and is timed out: the regression asserts the descendant is gone after the doctor returns.
This commit is contained in:
1 parent
3f7d0d2d21
commit
d427fc05c3
4 files changed
+88
-17
No files matched your search
+30
-16
@@ -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<void> | undefined;
|
||||
|
||||
override close(): Promise<void> {
|
||||
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<typeof OwnedMcpStdioTransport>[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,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
+14
@@ -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);
|
||||
Reference in new issue
Block a user