Skip to content

feat(cli): --load mode for load-testing a tool - #2743

Merged
cliffhall merged 4 commits into
v2/mainfrom
v2/feat/2439-cli-load-test
Oct 20, 2026
Merged

cliffhall merged 4 commits into
v2/mainfrom
v2/feat/2439-cli-load-test

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2439

What

A CLI load mode: --method tools/call --tool-name <name> --load calls one tool repeatedly and concurrently over the connection the CLI already builds (transport, headers, OAuth, catalog settings), then prints one latency / error-rate summary in place of the single result.

mcp-inspector --cli <server> --method tools/call --tool-name search \
  --tool-arg query=mcp --load --load-concurrency 20 --load-duration 30
Flag Meaning
--load Enable load mode (tools/call only)
--load-concurrency <n> Most calls in flight (default 10)
--load-duration <sec> Stop starting calls after this long; in-flight calls finish (default 10s when --load-requests is unset)
--load-requests <n> Stop after starting this many; with a duration too, whichever comes first
--load-rate <n> Start at most n calls/sec, spaced evenly

Real output against the stdio test server (echo, 200 calls, concurrency 20):

{
  "tool": "echo", "concurrency": 20, "requestLimit": 200,
  "requests": 200, "ok": 200, "toolErrors": 0, "errors": 0, "errorRate": 0,
  "elapsedMs": 8.74, "throughput": 22887.99,
  "latencyMs": { "min": 0.4, "mean": 0.76, "p50": 0.62, "p90": 1.38, "p99": 2.35, "max": 2.39 },
  "errorMessages": []
}

Design notes

  • toolErrors vs errors. isError: true results (the tool ran and failed) are counted apart from calls that threw (protocol errors, timeouts, a dropped connection). Thrown messages are grouped by frequency. The first 10 distinct ones are kept and the rest fold into (other), so a server that puts a request id in every message cannot grow the tally once per call.
  • errorRate is rounded to four places, not two, so 23 failures in 41k calls doesn't read as 0.
  • The tool is resolved once, before the clock starts. A missing tool exits 5 (tool_not_found), like a plain tools/call, instead of reporting a 100% error rate. A completed run exits 0 whatever its error rate, because the summary is the result. Threshold-based exit codes for CI would be a natural follow-up, but they are not part of this PR.
  • One connection. All calls share one session, so this measures concurrency on a session, not many sessions (README says so). A multi-connection mode is a possible follow-up.
  • Validation runs before the short-circuit returns, matching --strict/--verify. --load is rejected with any method but tools/call, and with --app-info/--output/--output-format. Each --load-* flag is rejected without --load.
  • mcpdo: the engine (runLoad, runToolLoad, the flag parsers and resolveLoadOptions) lives in core/cli/handlers/load-test.ts, the surface the CLI shares with mcpdo, so mcpdo can adopt it later without a second copy. Only the one-shot CLI exposes it in this PR, as requested.

Tests

clients/cli/__tests__/load-test.test.ts (30 tests):

  • Engine unit tests on an injected fake clock: the concurrency cap, the request cap, deadline behavior, even rate spacing, no sleeping past the window, outcome classification, the error-kind cap, four-place rounding, and an empty run.
  • In-process CLI runs against real test servers:
    • stdio echo
    • an HTTP tool that fails on every other call (toolErrors)
    • the crashable server (errors after the transport dies)
    • a taskSupport: "required" tool, which goes through the task path
    • every rejection message

Coverage: load-test.ts 100 / 98.6 / 100 / 100. npm run local:gate is green.

No UI change, so no screenshots.

🤖 Generated with Claude Code

`--load` drives one tool with concurrent, sustained tools/call traffic over
the connection the CLI already sets up (transport, headers, OAuth), then
prints a latency / error-rate summary in place of the single result.

Bounds: --load-concurrency (default 10), --load-duration (default 10s when
--load-requests is unset), --load-requests, and --load-rate (even spacing).
The summary separates isError results (toolErrors) from calls that threw
(errors), groups thrown messages (first ten kinds, rest as "(other)"), and
reports nearest-rank latency percentiles and throughput.

The engine lives in core/cli/handlers/load-test.ts so mcpdo can reuse it
later; only the one-shot CLI exposes it for now.

Closes #2439

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Oct 20, 2026
@cliffhall cliffhall linked an issue Oct 20, 2026 that may be closed by this pull request
2 tasks done
@cliffhall
cliffhall requested a balanced review from Copilot October 20, 2026 05:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Scheduling bounds, tool-level authentication recovery and short-circuit validation have unresolved correctness gaps.

4 open issues
What changed in this PR

Adds one-shot CLI load testing over an existing MCP connection, with a reusable engine in shared core.

Changes:

  • Adds concurrency, duration, request-count and rate controls.
  • Reports latency, throughput and error summaries.
  • Documents usage and adds engine and CLI tests.
File Description
core/​cli/​handlers/​load-test.ts Implements load scheduling and summaries.
clients/​cli/​src/​cli.ts Adds flags, validation and execution wiring.
clients/​cli/​README.md Documents load mode and its limits.
clients/​cli/​__tests__/​load-test.test.ts Tests scheduling, outcomes and CLI integration.

Review effort: Balanced

Comment thread clients/cli/src/cli.ts
Comment thread clients/cli/src/cli.ts
Comment thread core/cli/handlers/load-test.ts Outdated
Comment thread core/cli/handlers/load-test.ts Outdated
…r a wait

The rate limiter scheduled slot i at start + i/rate, so a stall (one slow
call at concurrency 1) left every missed slot owed and they all started at
once afterwards. It now keys off the last slot actually handed out, so a
missed slot is dropped and starts stay at least 1/rate apart.

The deadline is also checked again after the rate wait, since a timer that
wakes late under load would otherwise start a call past --load-duration.

--load is now rejected with --list-stored-auth and --print-handoff, which
return before the load runs. The README notes that a load run does not
prompt for OAuth step-up.

The engine tests' fake clock gains a timer queue so concurrent sleepers
wake in order, as real timers do.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1, answered in 9649799 (also replied in each thread):

  • Auth recovery at the invocation boundary: declined as out of scope. Step-up in the middle of a concurrent run is its own design question. Failures are already counted and their message shows in errorMessages, and the README now gives the workaround (run one plain tools/call first).
  • Reject with short-circuit modes: fixed. --load now refuses --list-stored-auth and --print-handoff.
  • Catch-up bursts: fixed. Missed rate slots are dropped, not owed; tests cover a slow call and concurrent workers.
  • Recheck the deadline after a wake: fixed. A regression test uses a late-firing clock.

Both scheduling fixes were mutation-checked: reverting either one fails only its own test. npm run local:gate is green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Delayed concurrent timers can start calls together, violating the configured rate limit.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Concurrent delayed timers can violate load-rate spacing

core/​cli/​handlers/​load-test.ts:241

The single-worker catch-up case is fixed, but concurrent timers that wake late can still violate --load-rate. With concurrency 3, rate 5, three requests and a 2s window, delaying the queued timers until 1000ms produces starts at [0, 1000, 1000], rather than keeping them at least 200ms apart. Workers reserve slots before sleeping and only recheck the deadline afterward, so overdue calls start together. Recheck and claim the shared next-start time after each wake, as below, and add a regression for multiple delayed timers within the window.

4 resolved since last review

Review effort: Balanced

Workers reserved their slot before sleeping, so when the event loop stalled
and several timers fired late together, every overdue worker started at once
and --load-rate's spacing collapsed. A slot is now claimed only by a worker
that is awake at its time, and each wake re-checks the bounds; the rest go
back to waiting.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 2, answered in bc6db3e (the finding was in the "previously missed" block, so it has no thread to reply in):

  • Concurrent delayed timers can violate --load-rate spacing: fixed. Workers no longer reserve a slot before sleeping. A slot is claimed only by a worker that is awake at its time, and every wake re-checks the bounds, so when several timers fire late together the first one to run takes the slot and the rest go back to waiting. The claim is still synchronous, so no two workers can take the same slot. Your scenario is the regression test: concurrency 3, rate 5, three requests, a 2s window, and a clock whose timers can't fire before 1000ms. It now starts at [0, 1000, 1200]. Restoring reserve-before-sleep fails only that test.

npm run local:gate is green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Verified deadline enforcement and timer-scaling issues undermine load scheduling and measurement accuracy.

2 open issues

Review effort: Balanced

Comment thread core/cli/handlers/load-test.ts Outdated
Comment thread core/cli/handlers/load-test.ts
Every idle worker armed a timer for the same next slot and all but one
re-armed on waking: 9,900 timers for 100 calls at concurrency 100. Workers
now queue on a FIFO turnstile and only the head waits on the clock, which
brings that to 99.

A claim also reset the schedule from the wake time, so ordinary timer
lateness accumulated and the run fell short of the rate (176 of 200 calls in
a 1s run at 200/s). A wake that is late by less than one interval now keeps
the slot grid; only a slot missed entirely is dropped, so there is still no
catch-up burst after a stall. The README notes the ~1ms timer ceiling that
limits rates in the high hundreds per second.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 3, answered in 4e13034 (also replied in each thread):

  • One timer per slot: fixed. A FIFO turnstile means only the head of the queue waits on the clock: 99 timers for 100 calls at concurrency 100, down from 9,900. Mutation-checked.
  • Average rate under timer lateness: fixed too. I found this while measuring the change above. Small lateness no longer pushes the schedule back, and a slot missed entirely is still dropped. 50/s and 200/s now hit their targets exactly, and the README documents the ~1ms timer ceiling.
  • Recheck the deadline after the claim: declined. The scenario needs a tool call that blocks the event loop for 100ms, and callTool blocks for microseconds.

npm run local:gate is green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues remain; prior concerns are fixed or documented as intentional limitations.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review loop closed: round 4 was clean on all three channels ("Approval recommended", no open, missed or suppressed findings, no inline comments). Across four rounds there were seven findings: five fixed, plus one more found while measuring a fix, and two declined as out of scope (in-run OAuth step-up, and a deadline recheck that only matters for a tool call that blocks the event loop synchronously).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Discussion] Built-in load/concurrency test mode for stress-testing MCP servers

2 participants