fix(load): partial summary on Ctrl+C, circuit breaker, success-only latency, dropped rate slots

clients/cli/README.md

@@ -162,7 +162,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en
 | `--load-requests <n>`         | With `--load`: stop after starting this many calls. With `--load-duration` too, whichever is reached first ends the run. |
 | `--load-rate <n>`             | With `--load`: start at most this many calls per second across all workers, spaced evenly (default: unlimited). |
 | `--load-max-error-rate <fraction>` | With `--load`: after printing the summary, exit `10` (`load_error_rate_exceeded`) when the share of failed calls is above this value. `0` to `1`; `0` fails on any failed call. See [Thresholds for CI](#thresholds-for-ci). |
-| `--load-max-p99 <ms>`         | With `--load`: after printing the summary, exit `10` (`load_p99_exceeded`) when the p99 latency is above this many milliseconds, or when no call completed. Positive number. |
+| `--load-max-p99 <ms>`         | With `--load`: after printing the summary, exit `10` (`load_p99_exceeded`) when the p99 latency of the successful calls is above this many milliseconds, or when no call succeeded. Positive number. |
 | `--relogin`                   | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits.                                                                                                                  |
 | `--no-revoke`                 | With `--relogin`, skip the [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) revocation request that would otherwise end the grant at the authorization server when the local state is deleted. The per-server `oauth.revokeOnClear` setting is the persistent form of the same opt-out; either one is enough to skip it. See [Revoking on `--relogin`](#revoking-on---relogin). |
 | `--stored-auth-only`          | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login.                                                                                                                                        |
@@ -331,8 +331,11 @@ is printed on stdout (wrapped in `{ "result": … }` with `--format json`):
   "errorRate": 0.0006,
   "elapsedMs": 30012.4,
   "throughput": 1373.12,
-  "latencyMs": { "min": 2.1, "mean": 14.5, "p50": 12.9, "p90": 22.4, "p99": 61.3, "max": 30004.8 },
-  "errorMessages": [{ "message": "Request timed out", "count": 23 }]
+  "okThroughput": 1372.35,
+  "latencyMs": { "min": 2.1, "mean": 14.5, "p50": 12.9, "p90": 22.4, "p99": 61.3, "max": 212.8 },
+  "errorLatencyMs": { "min": 30001.2, "mean": 30003.1, "p50": 30002.9, "p90": 30004.1, "p99": 30004.8, "max": 30004.8 },
+  "errorMessages": [{ "message": "Request timed out", "count": 23 }],
+  "toolErrorMessages": []
 }
 ```
 
@@ -342,20 +345,68 @@ is printed on stdout (wrapped in `{ "result": … }` with `--format json`):
   together over `requests`, rounded to four places.
 - **`errorMessages`** groups the thrown messages by frequency. The first ten
   distinct messages are kept; any further ones are counted under `(other)`.
+  **`toolErrorMessages`** does the same for the `isError` results, using the
+  first text block of each, trimmed and cut to 200 characters (`(no text
+  content)` when a result has none), so a run that is all `rate limited`
+  says so ([#2836](https://github.com/modelcontextprotocol/inspector/issues/2836)).
 - **Latency** is per call, client-side, in milliseconds, with nearest-rank
-  percentiles. It includes everything the Inspector does around the request,
-  which is negligible next to a network round trip but not next to a
+  percentiles. **`latencyMs` covers the successful (`ok`) calls only**, and
+  **`errorLatencyMs`** the failed ones (tool errors and thrown alike), so a
+  failure that comes back instantly never makes a struggling server look
+  faster; either is `null` when it has no calls. `throughput` counts every
+  completed call per second, **`okThroughput`** only the successful ones.
+  Latency includes everything the Inspector does around the request, which
+  is negligible next to a network round trip but not next to a
   sub-millisecond stdio echo.
 - **Bounds.** `--load-duration` stops *starting* calls; anything in flight is
   allowed to finish, so `elapsedMs` can exceed the window by up to one call.
+  A run that reaches its `--load-duration` is measured to the end of that
+  window at least, so a `--load-rate 1` run over five seconds reports a
+  throughput of 1/s rather than the 1.25/s its last start would suggest
+  ([#2837](https://github.com/modelcontextprotocol/inspector/issues/2837)).
   With `--load-rate`, starts follow an even `1/rate` grid; a timer that wakes
   a little late keeps the grid, so the average rate holds. A slot missed
   entirely — every worker busy, or a stalled event loop — is dropped rather
   than made up later, so a stall is never followed by a catch-up burst. A
   slot that falls past the window is never started. Node's timers resolve to
   about a millisecond, so a rate in the high hundreds per second or above is
   reached only approximately (1000/s measured ~700 against a local stdio
-  server); compare `requests` against what you asked for.
+  server).
+- **`plannedStarts`, `droppedSlots`, `achievedRate`** appear only with
+  `--load-rate` ([#2837](https://github.com/modelcontextprotocol/inspector/issues/2837)).
+  `plannedStarts` is what the `1/rate` grid called for over the window the run
+  was starting calls in (the whole `--load-duration` for a run that reached
+  it, or up to the last start of a run that `--load-requests` ended);
+  `droppedSlots` is how many of those were
+  never started because every worker was busy; `achievedRate` is the calls
+  started per second over that window. Dropping slots keeps a stall from
+  turning into a burst, but it also makes the run closed-loop: a server too
+  slow for the target rate reports healthy latency while most of the planned
+  load never ran. So when fewer than 90% of `plannedStarts` were started, a
+  warning naming the shortfall is printed on stderr after the summary
+  (dropped by `--quiet`). Raise `--load-concurrency` or lower the rate.
+- **The run stops early when the server is clearly gone**
+  ([#2835](https://github.com/modelcontextprotocol/inspector/issues/2835)).
+  A refused connection fails each call in microseconds, so a server that dies
+  mid-run would otherwise turn the rest of the window into thousands of
+  instant failures. Once 50 calls in a row have thrown — or calls have done
+  nothing but throw for a second, at least 5 of them — no further call
+  starts. Only thrown calls count; a tool error is the server answering, and
+  any other outcome resets the streak. The summary covers what completed and
+  carries **`stoppedEarly`**: `{ "reason": "consecutive_errors", "detail":
+  "…" }`, with a warning on stderr. The exit code is unchanged — `0` unless a
+  threshold is breached.
+- **Ctrl+C or SIGTERM ends the run, not the summary**
+  ([#2834](https://github.com/modelcontextprotocol/inspector/issues/2834)).
+  The first signal stops new calls from starting and lets the in-flight ones
+  finish; the summary of what completed is then printed with
+  `"stoppedEarly": { "reason": "interrupted", "detail": "received SIGINT" }`,
+  and the CLI exits `130` (`interrupted`) without checking any threshold. A
+  second Ctrl+C exits `130` immediately, for in-flight calls that will not
+  settle. A repeat of the same signal within half a second of the first is
+  taken as a re-raise rather than a second press — a dependency of the CLI
+  (`when-exit`) re-sends the signal to the process after its own cleanup — so
+  the press that exits at once is one made after that.
 - **`connections`** appears only with `--load-connections` above `1` (see
   below), so a default run's summary is unchanged.
 
@@ -411,7 +462,10 @@ the run with exit `5` (`tool_not_found`) instead of reporting a 100% error
 rate. Otherwise a completed run exits `0` whatever its error rate — the summary
 is the result — unless a threshold below is set. `--load` is rejected with any other method and with `--app-info`,
 `--output`, `--output-format`, `--list-stored-auth` and `--print-handoff`, and each `--load-*` flag is
-rejected without `--load`.
+rejected without `--load`. Each of those refusals — and `--load-connections`
+above `--load-concurrency`, or above `1` over stdio — exits `1` with the
+envelope `code` `load_flag_conflict`, the code `mcpdo` uses for the same
+mistakes.
 
 ##### Thresholds for CI
 
@@ -434,8 +488,10 @@ mcp-inspector --cli <server> --method tools/call --tool-name search \
   share of failed calls (`toolErrors` plus `errors`, over `requests`), not the
   four-place `errorRate`, so `--load-max-error-rate 0` fails even one failure
   that rounds away in the summary.
-- With `--load-max-p99`, a run in which no call completed has no p99 to
-  compare, and fails rather than passing a latency check it never measured.
+- `--load-max-p99` judges the **successful** calls (`latencyMs`), so a run
+  in which half the calls fail instantly cannot pass on the strength of its
+  failures. A run in which no call succeeded has no p99 to compare, and fails
+  rather than passing a latency check it never measured.
 - With no threshold set, nothing changes: a completed run exits `0`.
 
 #### Schema portability (`--strict`)
@@ -720,6 +776,7 @@ prose from stderr:
 | `9`  | `--verify --require-digests` found a skill that advertised no digests (`skills_unverifiable` — `resources: "dynamic"`). Only produced under `--require-digests`; without it such a skill is reported `unverifiable` and the run exits `0`. |
 | `10` | A `--load` run breached a threshold (`load_error_rate_exceeded` for `--load-max-error-rate`, `load_p99_exceeded` for `--load-max-p99`). The summary is still printed on stdout first. See [Thresholds for CI](#thresholds-for-ci). |
 | `11` | The server rate limited the request (`rate_limited` — HTTP 429). The envelope carries the server's retry timing; see below. |
+| `130` | A `--load` run was interrupted by Ctrl+C or SIGTERM (`interrupted`). The partial summary is printed on stdout first. See [Load testing a tool](#load-testing-a-tool---load). |
 
 On any non-zero exit the CLI also writes a single JSON line to **stderr** — the
 `ErrorEnvelope`:

clients/cli/__tests__/helpers/load-clock.ts

@@ -0,0 +1,50 @@
+import type { LoadClock } from "@inspector/core/cli/handlers/load-test.js";
+
+/**
+ * A clock that only moves when told to, with a timer queue: each `sleep`
+ * wakes at an absolute time, and timers fire one at a time in wake order,
+ * each on its own macrotask so the woken worker runs (up to its next await)
+ * before the clock moves again — as real timers behave. A rate-limited run
+ * therefore completes instantly while keeping the schedule it would in real
+ * time.
+ */
+export function fakeClock(
+  options: { lateUntil?: number; jitter?: number } = {},
+): LoadClock & { advance: (ms: number) => void; sleeps: () => number } {
+  let t = 1000;
+  // A stalled event loop: no timer fires before this many ms into the run.
+  const earliestWake = 1000 + (options.lateUntil ?? 0);
+  const timers: { wakeAt: number; resolve: () => void }[] = [];
+  let pumping = false;
+  let sleepCount = 0;
+  const pump = (): void => {
+    timers.sort((a, b) => a.wakeAt - b.wakeAt);
+    const next = timers.shift();
+    if (!next) {
+      pumping = false;
+      return;
+    }
+    t = Math.max(t, next.wakeAt);
+    next.resolve();
+    setImmediate(pump);
+  };
+  return {
+    now: () => t,
+    sleep: (ms) =>
+      new Promise<void>((resolve) => {
+        sleepCount++;
+        timers.push({
+          wakeAt: Math.max(t + ms + (options.jitter ?? 0), earliestWake),
+          resolve,
+        });
+        if (!pumping) {
+          pumping = true;
+          setImmediate(pump);
+        }
+      }),
+    advance: (ms) => {
+      t += ms;
+    },
+    sleeps: () => sleepCount,
+  };
+}

clients/cli/__tests__/load-interrupt-e2e.test.ts

@@ -0,0 +1,173 @@
+import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { spawn, type ChildProcess } from "node:child_process";
+import { resolve, dirname } from "node:path";
+import { fileURLToPath } from "node:url";
+import {
+  createTestServerHttp,
+  createTestServerInfo,
+} from "@modelcontextprotocol/inspector-test-server";
+import { EXIT_CODES } from "@inspector/core/cli/error-handler.js";
+import { REPEAT_GRACE_MS } from "../src/handlers/load-interrupt.js";
+
+const here = dirname(fileURLToPath(import.meta.url));
+const BIN = resolve(here, "../build/index.js");
+
+/**
+ * Ctrl+C / SIGTERM delivered as a REAL signal to the BUILT CLI (#2834).
+ *
+ * The in-process suite (`load-summary.test.ts`) calls the CLI's own listener
+ * directly, which is exactly what let the first fix ship broken: `when-exit`
+ * (via `atomically`) also listens, and re-raises the signal with
+ * `process.kill(process.pid, …)` after its cleanups. That copy only exists
+ * when the kernel delivers a signal to a real process carrying the real
+ * dependency graph, so only a spawned binary can see it.
+ *
+ * Budgets: the child is killed after `CHILD_TIMEOUT_MS`, inside the test's own
+ * `E2E_SPAWN_MS`, so a hang names the CLI rather than timing out the test
+ * generically (the same arrangement as `e2e.test.ts`, #2323).
+ */
+const CHILD_TIMEOUT_MS = 15_000;
+const E2E_SPAWN_MS = 25_000;
+
+type Run = {
+  child: ChildProcess;
+  done: Promise<{ code: number | null; stdout: string; stderr: string }>;
+  stderr: () => string;
+};
+
+function spawnLoad(url: string, tool: string): Run {
+  const child = spawn(
+    "node",
+    [
+      BIN,
+      url,
+      "--transport",
+      "http",
+      "--method",
+      "tools/call",
+      "--tool-name",
+      tool,
+      "--load",
+      "--load-duration",
+      "60",
+      "--load-concurrency",
+      "2",
+      "--format",
+      "json",
+    ],
+    { stdio: ["ignore", "pipe", "pipe"] },
+  );
+  let stdout = "";
+  let stderr = "";
+  child.stdout!.on("data", (d) => (stdout += d.toString()));
+  child.stderr!.on("data", (d) => (stderr += d.toString()));
+  const done = new Promise<{
+    code: number | null;
+    stdout: string;
+    stderr: string;
+  }>((resolveDone, reject) => {
+    const timer = setTimeout(() => {
+      child.kill("SIGKILL");
+      reject(new Error(`built CLI did not exit within ${CHILD_TIMEOUT_MS}ms`));
+    }, CHILD_TIMEOUT_MS);
+    child.on("error", (err) => {
+      clearTimeout(timer);
+      reject(err);
+    });
+    child.on("close", (code) => {
+      clearTimeout(timer);
+      resolveDone({ code, stdout, stderr });
+    });
+  });
+  return { child, done, stderr: () => stderr };
+}
+
+/** Resolve once `check` holds, polling every 50ms. */
+async function until(check: () => boolean): Promise<void> {
+  while (!check()) await new Promise((r) => setTimeout(r, 50));
+}
+
+describe("--load under a real signal (built CLI)", () => {
+  let calls = 0;
+  let release: (() => void)[] = [];
+  const makeServer = () =>
+    createTestServerHttp({
+      serverInfo: createTestServerInfo("load-signal", "1.0.0"),
+      tools: [
+        {
+          name: "slow",
+          description: "Answers after 50ms",
+          inputSchema: {},
+          handler: async () => {
+            calls++;
+            await new Promise((r) => setTimeout(r, 50));
+            return { content: [{ type: "text", text: "ok" }] };
+          },
+        },
+        {
+          name: "hang",
+          description: "Answers only when the test lets it",
+          inputSchema: {},
+          handler: async () => {
+            calls++;
+            await new Promise<void>((r) => release.push(r));
+            return { content: [{ type: "text", text: "late" }] };
+          },
+        },
+      ],
+    });
+
+  let server: ReturnType<typeof makeServer>;
+
+  beforeEach(async () => {
+    calls = 0;
+    release = [];
+    server = makeServer();
+    await server.start();
+  });
+
+  afterEach(async () => {
+    for (const r of release) r();
+    await server.stop();
+  });
+
+  it.each(["SIGINT", "SIGTERM"] as const)(
+    "prints the partial summary and exits 130 on one %s",
+    async (signal) => {
+      const run = spawnLoad(server.url, "slow");
+      await until(() => calls >= 10);
+      run.child.kill(signal);
+      const { code, stdout, stderr } = await run.done;
+      expect(code).toBe(EXIT_CODES.INTERRUPTED);
+      const { result } = JSON.parse(stdout.trim().split("\n")[0]!);
+      expect(result.stoppedEarly).toEqual({
+        reason: "interrupted",
+        detail: `received ${signal}`,
+      });
+      expect(result.ok).toBeGreaterThan(0);
+      expect(stderr).toContain(`Interrupted (${signal})`);
+      expect(JSON.parse(stderr.trim().split("\n").pop()!).error.code).toBe(
+        "interrupted",
+      );
+    },
+    E2E_SPAWN_MS,
+  );
+
+  it(
+    "exits 130 at once on a second Ctrl+C while calls will not settle",
+    async () => {
+      const run = spawnLoad(server.url, "hang");
+      await until(() => calls >= 2);
+      run.child.kill("SIGINT");
+      await until(() => run.stderr().includes("Interrupted (SIGINT)"));
+      // A person's second press, past the window that absorbs a re-raise.
+      await new Promise((r) => setTimeout(r, REPEAT_GRACE_MS + 200));
+      run.child.kill("SIGINT");
+      const { code, stdout } = await run.done;
+      expect(code).toBe(EXIT_CODES.INTERRUPTED);
+      // The in-flight calls never settled, so there was no summary to print.
+      expect(stdout).toBe("");
+    },
+    E2E_SPAWN_MS,
+  );
+});