diff --git a/clients/mcpdo/README.md b/clients/mcpdo/README.md index 8e27ce311..7712f2e22 100644 --- a/clients/mcpdo/README.md +++ b/clients/mcpdo/README.md @@ -248,11 +248,32 @@ mcpdo @prod tools/call search query:=mcp --load --load-requests 500 --load-rate | `--load-duration ` | Stop starting calls after this many seconds; in-flight calls finish. Default `10` without `--load-requests`. | | `--load-requests ` | Stop after starting this many calls. With `--load-duration` too, whichever comes first ends the run. | | `--load-rate ` | Start at most this many calls per second, spaced evenly (default: unlimited). | +| `--load-connections ` | Spread the calls over this many independent sessions (HTTP/SSE only; at most `--load-concurrency`; default `1`). | +| `--load-max-error-rate ` | Exit `10` (`load_error_rate_exceeded`) when the share of failed calls is above this value (`0` to `1`). | +| `--load-max-p99 ` | Exit `10` (`load_p99_exceeded`) when the p99 latency is above this many milliseconds, or no call completed. | Each `--load-*` flag requires `--load`, and `--load` cannot be combined with `--app-info`. `--format json` prints the summary object itself, with no -`{ result }` envelope, as for every other mcpdo command. The exit code is `0` -whenever the run completes, however many calls failed — read `errorRate`. +`{ result }` envelope, as for every other mcpdo command. Without a threshold +flag the exit code is `0` whenever the run completes, however many calls +failed — read `errorRate`. + +**Thresholds** ([#2800](https://github.com/modelcontextprotocol/inspector/issues/2800)) +make the exit code carry the verdict, as in the one-shot CLI. They are judged +by the `mcpdo` process after the summary has been printed, so a breached run +still shows its numbers; the breach then follows as the usual error envelope, +with the same `code` and exit code `10` the CLI uses. + +**Multiple sessions** (`--load-connections`, #2800) open the extra sessions +inside the daemon, before the clock starts, from the connection's stored server +config and settings. They use stored credentials only — an extra never starts a +sign-in, so connect the connection first — and a session that fails to open is +counted in the summary's `connections` block, not as a call error. The +connection's own live client is always the first session. The extras belong to +the run: they never appear in `connections/list`, they are closed when the run +ends (or the daemon stops), and Ctrl-C cancels their in-flight calls too. Over +stdio each session would spawn its own server process, so more than one is +refused there. How a run behaves on a shared daemon connection: diff --git a/clients/mcpdo/__tests__/daemon-load.test.ts b/clients/mcpdo/__tests__/daemon-load.test.ts index ade7f3534..41b385d3d 100644 --- a/clients/mcpdo/__tests__/daemon-load.test.ts +++ b/clients/mcpdo/__tests__/daemon-load.test.ts @@ -2,7 +2,13 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import { + createEchoTool, + createTestServerHttp, + createTestServerInfo, +} from "@modelcontextprotocol/inspector-test-server"; import { DaemonServer } from "../src/daemon/server.js"; +import { ConnectionRegistry } from "../src/daemon/connections.js"; import type { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { InspectorClientEventTarget } from "@inspector/core/mcp/inspectorClientEventTarget.js"; import type { ConnectionStatus } from "@inspector/core/mcp/types.js"; @@ -11,6 +17,7 @@ import type { LoadSummary, } from "@inspector/core/cli/handlers/load-test.js"; import type { RpcParams } from "../src/daemon/protocol.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; /** * `tools/call --load` inside the daemon (#2744): the `rpc` op hands the run to @@ -24,7 +31,10 @@ type LoadImpl = ( client: unknown, args: Record, options: LoadOptions, - run: { signal?: AbortSignal }, + run: { + signal?: AbortSignal; + openConnection?: () => Promise; + }, ) => Promise; const loadMock = vi.hoisted(() => { @@ -47,6 +57,36 @@ vi.mock( }), ); +/** + * Observes the daemon's OAuth storage handling (#2800) without changing it: + * the real reset runs behind a spy, and the real class is counted. + */ +const oauthStorageMock = vi.hoisted(() => ({ + constructed: 0, + reset: vi.fn(), +})); +vi.mock("@inspector/core/auth/node/storage-node.js", async (importOriginal) => { + const actual = + await importOriginal< + typeof import("@inspector/core/auth/node/storage-node.js") + >(); + oauthStorageMock.reset.mockImplementation(actual.resetNodeOAuthStorageCache); + return { ...actual, resetNodeOAuthStorageCache: oauthStorageMock.reset }; +}); +vi.mock("@inspector/core/auth/node/index.js", async (importOriginal) => { + const actual = + await importOriginal(); + class CountedNodeOAuthStorage extends actual.NodeOAuthStorage { + constructor( + ...args: ConstructorParameters + ) { + super(...args); + oauthStorageMock.constructed++; + } + } + return { ...actual, NodeOAuthStorage: CountedNodeOAuthStorage }; +}); + const runMethodMock = vi.hoisted(() => ({ calls: 0 })); vi.mock("@inspector/core/cli/handlers/run-method.js", () => ({ runMethod: async () => { @@ -82,7 +122,7 @@ class FakeLoadClient implements Pick< InspectorClient, - "getStatus" | "cancelToolCall" | "setAmbientRequestSignal" + "getStatus" | "cancelToolCall" | "setAmbientRequestSignal" | "disconnect" > { ambient: AbortSignal | undefined; @@ -105,6 +145,8 @@ class FakeLoadClient this.ambient = undefined; }; } + + async disconnect(): Promise {} } function fakeClient(status: ConnectionStatus = "connected") { @@ -260,4 +302,224 @@ describe("daemon rpc with --load", () => { }); expect(runMethodMock.calls).toBe(1); }); + + describe("--load-connections", () => { + const MULTI: LoadOptions = { concurrency: 2, connections: 2, requests: 4 }; + + /** Point the connection at a server config and stub the extra dial. */ + function useConnection(serverConfig: MCPServerConfig) { + const extra = fakeClient(); + // Only `name` and `serverConfig` are read on this path. + vi.spyOn(server.registry, "connectionFor").mockReturnValue({ + name: "srv", + serverConfig, + } as ReturnType); + const open = vi + .spyOn(server.registry, "openLoadClient") + .mockResolvedValue(extra.client); + const release = vi.spyOn(server.registry, "releaseLoadClient"); + return { extra, open, release }; + } + + it("opens extras through the registry with the run's signal, and releases them", async () => { + const { extra, open, release } = useConnection({ + type: "streamable-http", + url: "http://localhost:1/mcp", + }); + let opened: InspectorClient | undefined; + loadMock.impl = async (_client, _args, options, run) => { + expect(options).toEqual(MULTI); + opened = await run.openConnection!(); + // The caller hanging up aborts the extra's in-flight calls too. + expect(extra.ambient()).toBe(run.signal); + return SUMMARY; + }; + const response = await rpc({ load: MULTI }); + expect(response.ok).toBe(true); + expect(opened).toBe(extra.client); + expect(open).toHaveBeenCalledWith( + { + serverConfig: { + type: "streamable-http", + url: "http://localhost:1/mcp", + }, + }, + fake.client, + expect.any(AbortSignal), + ); + expect(release).toHaveBeenCalledWith(extra.client); + expect(extra.ambient()).toBeUndefined(); + }); + + it("hands the run's signal to the dial, so a stalled handshake cancels", async () => { + const { open } = useConnection({ + type: "streamable-http", + url: "http://localhost:1/mcp", + }); + let runSignal: AbortSignal | undefined; + loadMock.impl = async (_client, _args, _options, run) => { + runSignal = run.signal; + await run.openConnection!(); + return SUMMARY; + }; + expect((await rpc({ load: MULTI })).ok).toBe(true); + expect(open.mock.calls[0]![2]).toBe(runSignal); + }); + + it("dials the server the primary was resolved on, even if the default moves while queued", async () => { + const { open } = useConnection({ + type: "streamable-http", + url: "http://a.example/mcp", + }); + // A first run holds the connection's queue until its signal aborts. + const firstStarted = runUntilAborted(); + const blocker = new AbortController(); + const first = rpc({ load: LOAD }, blocker.signal); + await firstStarted; + // The second run resolves its connection now, then waits in the queue. + const second = rpc({ load: MULTI }); + await vi.waitFor(() => + expect(server.registry.liveClientFor).toHaveBeenCalledTimes(2), + ); + // While it waits, the default connection changes to another server. + vi.mocked(server.registry.connectionFor).mockReturnValue({ + name: "other", + serverConfig: { type: "streamable-http", url: "http://b.example/mcp" }, + } as ReturnType); + loadMock.impl = async (_client, _args, _options, run) => { + await run.openConnection!(); + return SUMMARY; + }; + blocker.abort(); + await first; + expect((await second).ok).toBe(true); + expect(open).toHaveBeenCalledWith( + { + serverConfig: { + type: "streamable-http", + url: "http://a.example/mcp", + }, + }, + fake.client, + expect.any(AbortSignal), + ); + }); + + it("gives a single-session run no way to open more", async () => { + const { open } = useConnection({ + type: "streamable-http", + url: "http://localhost:1/mcp", + }); + loadMock.impl = async (_client, _args, _options, run) => { + expect(run.openConnection).toBeUndefined(); + return SUMMARY; + }; + expect((await rpc({ load: { ...MULTI, connections: 1 } })).ok).toBe(true); + expect(open).not.toHaveBeenCalled(); + }); + + it("refuses more than one session over stdio before the run starts", async () => { + const { open } = useConnection({ type: "stdio", command: "node" }); + loadMock.impl = async () => SUMMARY; + const response = await rpc({ load: MULTI }); + expect(response.ok).toBe(false); + expect(!response.ok && response.error.code).toBe("load_flag_conflict"); + expect(open).not.toHaveBeenCalled(); + // Nothing was set up, so nothing is left behind on the primary. + expect(fake.ambient()).toBeUndefined(); + }); + }); +}); + +/** + * The registry side of `--load-connections` (#2800), against a real HTTP + * server: an extra session is dialed from the connection's stored config, + * stays out of the connection list, and is closed by a daemon shutdown that + * lands mid-run. + */ +describe("ConnectionRegistry load sessions", () => { + let httpServer: ReturnType; + let registry: ConnectionRegistry; + + beforeEach(async () => { + httpServer = createTestServerHttp({ + serverInfo: createTestServerInfo("load-registry", "1.0.0"), + tools: [createEchoTool()], + }); + await httpServer.start(); + registry = new ConnectionRegistry(0); + await registry.connect({ + name: "remote", + serverConfig: { type: "streamable-http", url: httpServer.url }, + serverIdentity: httpServer.url, + }); + }); + + afterEach(async () => { + await registry.disconnectAll(); + await httpServer.stop(); + }); + + const target = () => ({ + serverConfig: { type: "streamable-http" as const, url: httpServer.url }, + }); + const primary = () => registry.resolve("remote", true).client; + + it("dials a separate, unlisted session that shutdown closes", async () => { + const extra = await registry.openLoadClient(target(), primary()); + expect(extra).not.toBe(primary()); + expect(extra.getStatus()).toBe("connected"); + expect(registry.list().map((c) => c.name)).toEqual(["remote"]); + expect(registry.connectionCount()).toBe(1); + + const disconnect = vi.spyOn(extra, "disconnect"); + await registry.disconnectAll(); + expect(disconnect).toHaveBeenCalledOnce(); + }); + + it("shares the primary's OAuth storage instead of a fresh token snapshot", async () => { + oauthStorageMock.reset.mockClear(); + const constructedBefore = oauthStorageMock.constructed; + await registry.openLoadClient(target(), primary()); + // A reset would orphan the primary's in-memory tokens, and a new storage + // would hold its own copy: either way a refresh-token rotation in one + // session would leave the others with an invalidated token. + expect(oauthStorageMock.reset).not.toHaveBeenCalled(); + expect(oauthStorageMock.constructed).toBe(constructedBefore); + }); + + it("leaves a released session to its run", async () => { + const extra = await registry.openLoadClient(target(), primary()); + await extra.disconnect(); + registry.releaseLoadClient(extra); + const disconnect = vi.spyOn(extra, "disconnect"); + await registry.disconnectAll(); + expect(disconnect).not.toHaveBeenCalled(); + }); + + it("closes a session whose dial finished after shutdown began", async () => { + const opening = registry.openLoadClient(target(), primary()); + const outcome = opening.catch((err: unknown) => err); + await registry.disconnectAll(); + expect(await outcome).toMatchObject({ + envelope: { code: "daemon_stopping" }, + }); + }); + + it("refuses to open a session once the daemon is stopping", async () => { + const client = primary(); + await registry.disconnectAll(); + await expect( + registry.openLoadClient(target(), client), + ).rejects.toMatchObject({ + envelope: { code: "daemon_stopping" }, + }); + }); + + it("cancels a dial when the run's signal aborts", async () => { + const stop = new AbortController(); + const opening = registry.openLoadClient(target(), primary(), stop.signal); + stop.abort(); + await expect(opening).rejects.toThrow(); + }); }); diff --git a/clients/mcpdo/__tests__/load.test.ts b/clients/mcpdo/__tests__/load.test.ts index 0b5465189..20eeaa5af 100644 --- a/clients/mcpdo/__tests__/load.test.ts +++ b/clients/mcpdo/__tests__/load.test.ts @@ -2,9 +2,15 @@ import { describe, it, expect, afterEach } from "vitest"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import { + createEchoTool, + createTestServerHttp, + createTestServerInfo, +} from "@modelcontextprotocol/inspector-test-server"; import { runMcp } from "./helpers/mcp-runner.js"; import { createSampleTestConfig, + createTestConfig, deleteConfigFile, } from "../../cli/__tests__/helpers/fixtures.js"; import { expectCliSuccess } from "../../cli/__tests__/helpers/assertions.js"; @@ -17,11 +23,13 @@ import { DEFAULT_LOAD_DURATION_MS, type LoadSummary, } from "@inspector/core/cli/handlers/load-test.js"; +import { EXIT_CODES } from "@inspector/core/cli/error-handler.js"; import { PLAIN } from "@inspector/core/cli/style.js"; /** * `mcpdo tools/call --load` (#2744): the flag rules, the summary rendering, - * and one end-to-end run through an auto-spawned daemon. + * and end-to-end runs through an auto-spawned daemon — including the + * thresholds and multiple sessions brought over from the one-shot CLI (#2800). */ const SUMMARY: LoadSummary = { @@ -46,31 +54,66 @@ describe("resolveLoadFlags", () => { expect(resolveLoadFlags({ toolName: "echo" })).toBeUndefined(); }); - it("applies the engine's defaults", () => { + it("applies the engine's defaults, with no thresholds", () => { expect(resolveLoadFlags({ load: true, toolName: "echo" })).toEqual({ - concurrency: DEFAULT_LOAD_CONCURRENCY, - durationMs: DEFAULT_LOAD_DURATION_MS, + options: { + concurrency: DEFAULT_LOAD_CONCURRENCY, + durationMs: DEFAULT_LOAD_DURATION_MS, + }, }); }); - it("passes every bound through", () => { + it("passes every bound and threshold through", () => { expect( resolveLoadFlags({ load: true, toolName: "echo", loadConcurrency: 3, + loadConnections: 2, loadDuration: 2, loadRequests: 9, loadRate: 5, + loadMaxErrorRate: 0, + loadMaxP99: 250, + }), + ).toEqual({ + options: { + concurrency: 3, + connections: 2, + durationMs: 2000, + requests: 9, + rate: 5, + }, + thresholds: { maxErrorRate: 0, maxP99Ms: 250 }, + }); + }); + + it("refuses more connections than workers as a usage error", () => { + expect(() => + resolveLoadFlags({ + load: true, + toolName: "echo", + loadConcurrency: 2, + loadConnections: 3, }), - ).toEqual({ concurrency: 3, durationMs: 2000, requests: 9, rate: 5 }); + ).toThrow( + expect.objectContaining({ + exitCode: EXIT_CODES.USAGE, + message: + "--load-connections (3) cannot exceed --load-concurrency (2); every connection needs at least one worker.", + envelope: expect.objectContaining({ code: "load_flag_conflict" }), + }), + ); }); it.each([ [{ loadConcurrency: 2 }, "--load-concurrency requires --load."], + [{ loadConnections: 2 }, "--load-connections requires --load."], [{ loadDuration: 1 }, "--load-duration requires --load."], [{ loadRequests: 1 }, "--load-requests requires --load."], [{ loadRate: 1 }, "--load-rate requires --load."], + [{ loadMaxErrorRate: 0.1 }, "--load-max-error-rate requires --load."], + [{ loadMaxP99: 100 }, "--load-max-p99 requires --load."], [ { load: true, appInfo: true }, "--load cannot be combined with --app-info.", @@ -121,6 +164,45 @@ describe("formatLoadSummaryHuman", () => { expect(text).toContain("Error rate: 0.00%"); expect(text).toContain("Latency: no calls completed"); expect(text).not.toContain("Top errors"); + expect(text).not.toContain("Sessions:"); + }); + + it("renders the sessions of a multi-connection run, failures included", () => { + const text = formatLoadSummaryHuman( + { + ...SUMMARY, + errorMessages: [], + connections: { + requested: 3, + opened: 2, + failed: 1, + connectMs: { min: 4, mean: 4, p50: 4, p90: 4, p99: 4, max: 4 }, + errorMessages: [{ message: "ECONNREFUSED", count: 1 }], + }, + }, + PLAIN, + ); + expect(text).toContain( + " Sessions: 2 of 3 opened, 1 failed (connect p50 4 / max 4 ms)\n 1× ECONNREFUSED", + ); + }); + + it("omits connect times when no extra session opened", () => { + const text = formatLoadSummaryHuman( + { + ...SUMMARY, + connections: { + requested: 2, + opened: 1, + failed: 1, + connectMs: null, + errorMessages: [], + }, + }, + PLAIN, + ); + expect(text).toContain(" Sessions: 1 of 2 opened, 1 failed"); + expect(text).not.toContain("connect p50"); }); }); @@ -264,6 +346,136 @@ describe("mcpdo tools/call --load", { timeout: DAEMON_TEST_MS }, () => { expect(result.stderr).toContain("Tool 'no_such_tool' not found on server."); }); + async function connectStdio(e: Record): Promise { + configPath = createSampleTestConfig(); + expectCliSuccess( + await runMcp(["connect", "test-stdio", "--config", configPath], { + env: e, + timeout: DAEMON_COMMAND_MS, + }), + ); + } + + it("prints the summary, then fails a breached threshold with the CLI's code", async () => { + const e = env(); + await connectStdio(e); + // A real call never completes in a microsecond, so this p99 always fails. + const breached = await runMcp( + [ + "tools/call", + "echo", + "message:=hi", + "--load", + "--load-requests", + "4", + "--load-max-p99", + "0.001", + "--format", + "json", + ], + { env: e, timeout: DAEMON_COMMAND_MS }, + ); + expect(breached.exitCode).toBe(EXIT_CODES.LOAD_THRESHOLD_EXCEEDED); + expect(JSON.parse(breached.stdout)).toMatchObject({ requests: 4, ok: 4 }); + expect(breached.stderr).toContain("load_p99_exceeded"); + + // A threshold the run meets leaves the exit code alone. + expectCliSuccess( + await runMcp( + [ + "tools/call", + "echo", + "message:=hi", + "--load", + "--load-requests", + "4", + "--load-max-error-rate", + "0", + ], + { env: e, timeout: DAEMON_COMMAND_MS }, + ), + ); + }); + + it("refuses more than one session over stdio", async () => { + const e = env(); + await connectStdio(e); + const result = await runMcp( + [ + "tools/call", + "echo", + "--load", + "--load-requests", + "2", + "--load-connections", + "2", + ], + { env: e, timeout: DAEMON_COMMAND_MS }, + ); + expect(result.exitCode).toBe(EXIT_CODES.USAGE); + expect(result.stderr).toContain( + "--load-connections above 1 requires an HTTP or SSE connection", + ); + }); + + it("spreads a run over extra sessions that never become connections", async () => { + const server = createTestServerHttp({ + serverInfo: createTestServerInfo("load-sessions", "1.0.0"), + tools: [createEchoTool()], + }); + try { + await server.start(); + configPath = createTestConfig({ + mcpServers: { remote: { type: "streamable-http", url: server.url } }, + }); + const e = env(); + expectCliSuccess( + await runMcp(["connect", "remote", "--config", configPath], { + env: e, + timeout: DAEMON_COMMAND_MS, + }), + ); + const loaded = await runMcp( + [ + "tools/call", + "echo", + "message:=hi", + "--load", + "--load-requests", + "9", + "--load-concurrency", + "3", + "--load-connections", + "3", + "--format", + "json", + ], + { env: e, timeout: DAEMON_COMMAND_MS }, + ); + expectCliSuccess(loaded); + expect(JSON.parse(loaded.stdout)).toMatchObject({ + requests: 9, + ok: 9, + errors: 0, + connections: { requested: 3, opened: 3, failed: 0 }, + }); + + // The extras belonged to the run: only the connection itself is listed. + const listed = await runMcp(["connections/list", "--format", "json"], { + env: e, + timeout: DAEMON_COMMAND_MS, + }); + expectCliSuccess(listed); + expect( + ( + JSON.parse(listed.stdout) as { connections: { name: string }[] } + ).connections.map((c) => c.name), + ).toEqual(["remote"]); + } finally { + await server.stop(); + } + }); + it.each([ [ ["tools/call", "echo", "--load-requests", "2"], @@ -274,6 +486,18 @@ describe("mcpdo tools/call --load", { timeout: DAEMON_TEST_MS }, () => { ["tools/call", "echo", "--load", "--load-concurrency", "0"], "--load-concurrency must be a positive integer.", ], + [ + ["tools/call", "echo", "--load-max-p99", "10"], + "--load-max-p99 requires --load.", + ], + [ + ["tools/call", "echo", "--load", "--load-max-error-rate", "5"], + "--load-max-error-rate must be a number from 0 to 1.", + ], + [ + ["tools/call", "echo", "--load", "--load-connections", "0"], + "--load-connections must be a positive integer.", + ], ])("rejects %j before reaching a daemon", async (args, message) => { const result = await runMcp(args, { env: env() }); expect(result.exitCode).not.toBe(0); diff --git a/clients/mcpdo/src/connection/dispatch.ts b/clients/mcpdo/src/connection/dispatch.ts index 1246f02c1..add91f612 100644 --- a/clients/mcpdo/src/connection/dispatch.ts +++ b/clients/mcpdo/src/connection/dispatch.ts @@ -5,7 +5,8 @@ import type { MethodArgs, } from "@inspector/core/cli/handlers/method-types.js"; import type { OutputFormat } from "@inspector/core/cli/handlers/format-output.js"; -import type { LoadOptions } from "@inspector/core/cli/handlers/load-test.js"; +import { checkLoadThresholds } from "@inspector/core/cli/handlers/load-thresholds.js"; +import type { LoadFlags } from "./load-flags.js"; import { writeConnectionOutput } from "./format-connection.js"; import { styleFromOpts, type Style } from "@inspector/core/cli/style.js"; import { promptElicitation } from "./elicitation-prompt.js"; @@ -25,8 +26,11 @@ export type ConnectionDispatchOpts = { plain?: boolean; connection?: string; requireExplicit: boolean; - /** `tools/call --load` (#2744): run the load engine daemon-side. */ - load?: LoadOptions; + /** + * `tools/call --load` (#2744): `options` run the load engine daemon-side; + * `thresholds` (#2800) grade its summary here, after it is printed. + */ + load?: LoadFlags; }; /** @@ -48,7 +52,7 @@ export async function dispatchConnectionRpc( method, name: stripAt(opts.connection), requireExplicit: opts.requireExplicit, - ...(opts.load !== undefined && { load: opts.load }), + ...(opts.load !== undefined && { load: opts.load.options }), }; const { socketPath } = await ensureDaemon(); @@ -139,6 +143,12 @@ export async function dispatchConnectionRpc( methodArgs.toolName, outcome, ); + // After the summary is out, so a breached run still shows its numbers — + // the one-shot CLI's order (#2745). The breach is a CliExitCodeError, so it + // leaves as mcpdo's error envelope with the CLI's code and exit code. + if (outcome.kind === "load") { + checkLoadThresholds(outcome.summary, opts.load?.thresholds); + } } /** diff --git a/clients/mcpdo/src/connection/format-human.ts b/clients/mcpdo/src/connection/format-human.ts index 28650d35f..e1e6df99e 100644 --- a/clients/mcpdo/src/connection/format-human.ts +++ b/clients/mcpdo/src/connection/format-human.ts @@ -1016,6 +1016,22 @@ export function formatLoadSummaryHuman( lines.push(` ${count}× ${message}`); } } + // `--load-connections` above 1 (#2800). Connect failures are not call + // errors, so they are listed under this line rather than "Top errors". + const c = summary.connections; + if (c !== undefined) { + const failed = `${c.failed} failed`; + const connect = + c.connectMs === null + ? "" + : ` (connect p50 ${c.connectMs.p50} / max ${c.connectMs.max} ms)`; + lines.push( + ` Sessions: ${c.opened} of ${c.requested} opened, ${c.failed > 0 ? style.red(failed) : failed}${connect}`, + ); + for (const { message, count } of c.errorMessages) { + lines.push(` ${count}× ${message}`); + } + } return lines.join("\n"); } diff --git a/clients/mcpdo/src/connection/load-flags.ts b/clients/mcpdo/src/connection/load-flags.ts index 79c4b24d2..c9ad7cacd 100644 --- a/clients/mcpdo/src/connection/load-flags.ts +++ b/clients/mcpdo/src/connection/load-flags.ts @@ -6,32 +6,54 @@ import { resolveLoadOptions, type LoadOptions, } from "@inspector/core/cli/handlers/load-test.js"; +import { + resolveLoadThresholds, + type LoadThresholds, +} from "@inspector/core/cli/handlers/load-thresholds.js"; + +/** What a `tools/call --load` invocation asked for. */ +export type LoadFlags = { + /** Bounds the run; sent to the daemon, which runs the engine. */ + options: LoadOptions; + /** + * Grades the finished summary (#2800). Front-end only: the daemon never + * sees it, since it judges a summary rather than bounding the run. + */ + thresholds?: LoadThresholds; +}; /** * `tools/call --load` (#2744): turn mcpdo's flag values into the shared - * engine's {@link LoadOptions}, refusing the same combinations the one-shot - * CLI refuses (#2439). Each `--load-*` bound is meaningless without `--load`, - * and `--app-info` shapes a single result where a load run prints a summary, - * so both are usage errors rather than silently ignored. Value parsing itself - * (positive integers, positive numbers) is done by commander through the - * engine's own parsers, so the two CLIs reject the same inputs. + * engine's {@link LoadOptions} and the optional {@link LoadThresholds}, + * refusing the same combinations the one-shot CLI refuses (#2439, #2745, + * #2746). Each `--load-*` flag is meaningless without `--load`, and + * `--app-info` shapes a single result where a load run prints a summary, so + * both are usage errors rather than silently ignored. Value parsing itself + * (positive integers, positive numbers, fractions) is done by commander + * through the engine's own parsers, so the two CLIs reject the same inputs. * * Returns `undefined` when `--load` is not set: an ordinary `tools/call`. */ export function resolveLoadFlags(flags: { load?: boolean; loadConcurrency?: number; + loadConnections?: number; loadDuration?: number; loadRequests?: number; loadRate?: number; + loadMaxErrorRate?: number; + loadMaxP99?: number; appInfo?: boolean; toolName?: string; -}): LoadOptions | undefined { +}): LoadFlags | undefined { const bounds = [ ["--load-concurrency", flags.loadConcurrency], + ["--load-connections", flags.loadConnections], ["--load-duration", flags.loadDuration], ["--load-requests", flags.loadRequests], ["--load-rate", flags.loadRate], + ["--load-max-error-rate", flags.loadMaxErrorRate], + ["--load-max-p99", flags.loadMaxP99], ] as const; if (flags.load !== true) { for (const [flag, value] of bounds) { @@ -49,12 +71,26 @@ export function resolveLoadFlags(flags: { if (!flags.toolName) { throw usage("--load requires a tool name.", "missing_tool_name"); } - return resolveLoadOptions({ - concurrency: flags.loadConcurrency, - durationSec: flags.loadDuration, - requests: flags.loadRequests, - rate: flags.loadRate, + let options: LoadOptions; + try { + options = resolveLoadOptions({ + concurrency: flags.loadConcurrency, + connections: flags.loadConnections, + durationSec: flags.loadDuration, + requests: flags.loadRequests, + rate: flags.loadRate, + }); + } catch (err) { + // `--load-connections` above `--load-concurrency`: the engine's own + // message, as a usage error rather than a generic failure. It only ever + // throws an `Error`. + throw usage((err as Error).message); + } + const thresholds = resolveLoadThresholds({ + maxErrorRate: flags.loadMaxErrorRate, + maxP99Ms: flags.loadMaxP99, }); + return { options, ...(thresholds && { thresholds }) }; } function usage(message: string, code = "load_flag_conflict"): CliExitCodeError { diff --git a/clients/mcpdo/src/connection/mcp.ts b/clients/mcpdo/src/connection/mcp.ts index b802d2144..e9bee4df0 100644 --- a/clients/mcpdo/src/connection/mcp.ts +++ b/clients/mcpdo/src/connection/mcp.ts @@ -58,9 +58,12 @@ import { import { positiveIntegerParser, positiveNumberParser, - type LoadOptions, } from "@inspector/core/cli/handlers/load-test.js"; -import { resolveLoadFlags } from "./load-flags.js"; +import { + fractionParser, + maxP99Parser, +} from "@inspector/core/cli/handlers/load-thresholds.js"; +import { resolveLoadFlags, type LoadFlags } from "./load-flags.js"; import { authorizeInFrontend } from "./authorize.js"; import { AUTH_HELPER_COMMAND, @@ -1360,6 +1363,11 @@ function registerRpcCommands(program: CommandType): void { "With --load: most calls in flight at once (positive integer; default 10).", positiveIntegerParser("--load-concurrency"), ) + .option( + "--load-connections ", + "With --load: spread the calls over this many independent sessions to the connection's HTTP or SSE server, opened before timing starts with stored credentials only (positive integer, at most --load-concurrency; default 1).", + positiveIntegerParser("--load-connections"), + ) .option( "--load-duration ", "With --load: stop starting calls after this many seconds; in-flight calls still finish (default 10 when --load-requests is not set).", @@ -1374,6 +1382,18 @@ function registerRpcCommands(program: CommandType): void { "--load-rate ", "With --load: start at most this many calls per second, spread evenly (positive number; default unlimited).", positiveNumberParser("--load-rate"), + ) + // Thresholds (#2800): judged here, after the summary is printed, + // exactly as the one-shot CLI does (#2745). + .option( + "--load-max-error-rate ", + "With --load: exit 10 (load_error_rate_exceeded) when the share of failed calls is above this value, after printing the summary (0 to 1; 0 fails on any failed call).", + fractionParser("--load-max-error-rate"), + ) + .option( + "--load-max-p99 ", + "With --load: exit 10 (load_p99_exceeded) when the p99 latency is above this many milliseconds, or when no call completed, after printing the summary (positive number).", + maxP99Parser, ); cmd.action( async ( @@ -1651,7 +1671,7 @@ async function runRpc( program: CommandType, method: string, methodArgs: MethodArgs, - load?: LoadOptions, + load?: LoadFlags, ): Promise { const opts = program.opts(); await dispatchConnectionRpc(method, methodArgs, { diff --git a/clients/mcpdo/src/daemon/connections.ts b/clients/mcpdo/src/daemon/connections.ts index 77809afc7..cefd4c1f8 100644 --- a/clients/mcpdo/src/daemon/connections.ts +++ b/clients/mcpdo/src/daemon/connections.ts @@ -115,6 +115,16 @@ export class ConnectionRegistry { */ private closed = false; + /** + * Extra `--load-connections` sessions (#2800) currently open. They belong + * to the load run that opened them, which closes them when it ends, and are + * never entered in {@link connections}: `connections/list` does not show + * them and idle accounting does not count them (a run only happens on a + * registered connection, which already holds the idle timer off). They are + * tracked here only so that a daemon shutdown mid-run closes them too. + */ + private readonly loadClients = new Set(); + private async withNameLock( name: string, fn: () => Promise, @@ -431,14 +441,18 @@ export class ConnectionRegistry { serverSettings?: InspectorServerSettings; }, signal?: AbortSignal, + shareOAuthWith?: InspectorClient, ): Promise { // Front-end authorize / auth/clear write oauth.json in another process. - // Drop the daemon's cached store so this dial re-reads disk. - resetNodeOAuthStorageCache(); + // Drop the daemon's cached store so this dial re-reads disk — except for + // a session that shares a live client's storage (see openLoadClient), + // which must see that client's tokens, not a fresh snapshot of the disk. + if (!shareOAuthWith) resetNodeOAuthStorageCache(); const client = await createConnectionClient( params.serverConfig, params.serverSettings, + shareOAuthWith && clientOAuthStorage.get(shareOAuthWith), ); try { @@ -464,6 +478,46 @@ export class ConnectionRegistry { return client; } + /** + * Open one extra session for a `--load-connections` run (#2800): a fresh + * client built from a connection's stored server config and settings + * (captured by the caller when it resolved the primary), dialed the way a + * revive dials — stored credentials only, so an extra never starts an + * interactive sign-in mid-run. Aborting `signal` cancels a dial still in + * progress. + * + * The extra shares `primary`'s OAuth storage rather than a fresh one. That + * storage is load-once and memory-authoritative, so separate instances + * would each hold their own token snapshot: a refresh-token rotation in + * one session would leave its siblings, and the primary after the run, + * holding a token the authorization server has already invalidated. The caller owns the returned client and must hand it to + * {@link releaseLoadClient} once it has disconnected it. A failed dial + * leaves nothing behind. + */ + async openLoadClient( + target: { + serverConfig: MCPServerConfig; + serverSettings?: InspectorServerSettings; + }, + primary: InspectorClient, + signal?: AbortSignal, + ): Promise { + this.assertOpen(); + const client = await this.dial(target, signal, primary); + if (this.closed) { + // Shutdown ran while dialing, past the point where it closes these. + await safeDisconnect(client); + this.assertOpen(); + } + this.loadClients.add(client); + return client; + } + + /** Stop tracking an extra session its load run has closed. */ + releaseLoadClient(client: InspectorClient): void { + this.loadClients.delete(client); + } + use(name: string): ConnectionInfo { const connection = this.resolve(name, true); this.touch(connection.name); @@ -638,6 +692,11 @@ export class ConnectionRegistry { // rest. } } + // A load run still in flight would close these in its own `finally`, but + // the daemon may exit first; close them now. Disconnecting twice is safe. + const extras = [...this.loadClients]; + this.loadClients.clear(); + await Promise.all(extras.map((client) => safeDisconnect(client))); this.clearIdleTimer(); } @@ -814,9 +873,17 @@ export async function getLiveConnectionAuthInfo(connection: { } } +/** + * The OAuth storage each daemon-built client was given, so a load run's extra + * sessions can share their primary's (see `openLoadClient`). `InspectorClient` + * exposes no accessor for it. + */ +const clientOAuthStorage = new WeakMap(); + async function createConnectionClient( serverConfig: MCPServerConfig, serverSettings: InspectorServerSettings | undefined, + oauthStorage?: NodeOAuthStorage, ): Promise { const environment: InspectorClientEnvironment = { transport: createTransportNode, @@ -839,7 +906,7 @@ async function createConnectionClient( redirectUrlProvider.redirectUrl = formatRunnerOAuthRedirectUrl(callbackUrlConfig); environment.oauth = { - storage: new NodeOAuthStorage(), + storage: oauthStorage ?? new NodeOAuthStorage(), navigation: new ConsoleNavigation(), redirectUrlProvider, }; @@ -852,7 +919,7 @@ async function createConnectionClient( {}, ); - return new InspectorClient(serverConfig, { + const client = new InspectorClient(serverConfig, { environment, clientIdentity: { name: CONNECTION_CLIENT_NAME, @@ -883,6 +950,10 @@ async function createConnectionClient( }), ...clientAuthOptions, }); + if (environment.oauth?.storage instanceof NodeOAuthStorage) { + clientOAuthStorage.set(client, environment.oauth.storage); + } + return client; } async function safeDisconnect(client: InspectorClient): Promise { diff --git a/clients/mcpdo/src/daemon/server.ts b/clients/mcpdo/src/daemon/server.ts index ea866861c..29fd73d35 100644 --- a/clients/mcpdo/src/daemon/server.ts +++ b/clients/mcpdo/src/daemon/server.ts @@ -23,6 +23,7 @@ import { type HandleOutcome, } from "./ipc-glue.js"; import { wireElicitationBridge } from "./elicitation-bridge.js"; +import { isOAuthCapableServerConfig } from "@inspector/core/client/runner.js"; import { ElicitationParkRegistry, ParkingElicitationChannel, @@ -87,6 +88,15 @@ const autoCancelElicitationChannel: ElicitationChannel = { }, }; +/** + * Opens one extra `--load-connections` session (#2800) sharing `primary`'s + * OAuth storage; aborting `signal` cancels the dial. + */ +type LoadClientOpener = ( + primary: InspectorClient, + signal: AbortSignal, +) => Promise; + export type DaemonServerOptions = { dir?: string; idleMs?: number; @@ -617,6 +627,14 @@ export class DaemonServer { const connectionName = park ? this.registry.connectionFor(params.name, params.requireExplicit).name : undefined; + // Resolved in the same synchronous step as the primary client below + // (`liveClientFor` resolves before its first await), so the extra + // sessions dial the server the primary is on even if the default + // connection changes while this run waits in the queue (#2800). + const openLoadClient = + params.load !== undefined + ? this.loadConnectionOpener(params, params.load) + : undefined; const client = await this.registry.liveClientFor( params.name, params.requireExplicit, @@ -639,6 +657,7 @@ export class DaemonServer { requestId, params, params.load, + openLoadClient, signal, ); } @@ -721,6 +740,7 @@ export class DaemonServer { requestId: string, params: RpcParams, load: LoadOptions, + openLoadClient: LoadClientOpener | undefined, signal?: AbortSignal, ): Promise { const methodArgs = stripConnectionFields(params); @@ -755,12 +775,41 @@ export class DaemonServer { client.addEventListener("statusChange", onStatus); // In-flight calls abort with the run, not only new ones (#1783). const clearAmbientSignal = client.setAmbientRequestSignal(stop.signal); + // Extra sessions this run opened (#2800). The engine disconnects each in + // its own `finally`; this releases what the daemon set up around them. + const extras: { client: InspectorClient; release: () => void }[] = []; try { const summary = await runToolLoad(client, methodArgs, load, { signal: stop.signal, + ...(openLoadClient && { + openConnection: async () => { + // The run's signal reaches the dial too, so a stalled handshake + // does not hold the queue past Ctrl-C or a dropped primary. + const extra = await openLoadClient(client, stop.signal); + // The same treatment as the primary: the caller hanging up + // aborts the extras' in-flight calls too, and their elicitations + // are cancelled rather than left pending with nobody to answer. + const clearExtraSignal = extra.setAmbientRequestSignal(stop.signal); + const unwireExtra = wireElicitationBridge( + extra, + autoCancelElicitationChannel, + requestId, + ); + extras.push({ + client: extra, + release: () => { + unwireExtra(); + clearExtraSignal(); + this.registry.releaseLoadClient(extra); + }, + }); + return extra; + }, + }), }); return { kind: "load", summary }; } finally { + for (const extra of extras) extra.release(); clearAmbientSignal(); client.removeEventListener("statusChange", onStatus); signal?.removeEventListener("abort", onCallerGone); @@ -768,6 +817,42 @@ export class DaemonServer { } } + /** + * The extra-session opener for a `--load-connections` run (#2800), bound + * to the connection's server config as resolved now, or `undefined` for a + * single-session run. Checked before the run is queued: over stdio every + * session would spawn its own server process, testing N isolated servers + * rather than one server under N sessions, so it is refused exactly as the + * one-shot CLI refuses it (#2746). + */ + private loadConnectionOpener( + params: RpcParams, + load: LoadOptions, + ): LoadClientOpener | undefined { + if (load.connections === undefined || load.connections <= 1) { + return undefined; + } + const connection = this.registry.connectionFor( + params.name, + params.requireExplicit, + ); + if (!isOAuthCapableServerConfig(connection.serverConfig)) { + throw new CliExitCodeError( + EXIT_CODES.USAGE, + "--load-connections above 1 requires an HTTP or SSE connection; over stdio each session would spawn its own server process.", + { code: "load_flag_conflict" }, + ); + } + const target = { + serverConfig: connection.serverConfig, + ...(connection.serverSettings && { + serverSettings: connection.serverSettings, + }), + }; + return (primary, signal) => + this.registry.openLoadClient(target, primary, signal); + } + /** * A connection with a parked call must not accept new rpcs: the bridge * routes elicitations to its oldest subscriber, so a second in-flight call