Skip to content

Recognize HTTP 429 and surface the server's Retry-After - #2737

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/feat/2442-rate-limit-detection
Oct 20, 2026
Merged

cliffhall merged 3 commits into
v2/mainfrom
v2/feat/2442-rate-limit-detection

Conversation

@cliffhall

@cliffhall cliffhall commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Closes #2442

What

A request that a server throttles now rejects with a message that says it is rate limiting, which request was throttled, and when the server asked to retry. Before, it showed only the SDK's generic error:

Rate limited by the server (HTTP 429) on tools/list. The server asked to retry after 30s. Error POSTing to endpoint: slow down

With no header, the message says The server sent no Retry-After header. A malformed header is quoted as-is rather than guessed at. The structured form (method, retryAfter, retryAfterMs) goes on error.data as AnnotatedRateLimitData, for any consumer that wants a countdown or JSON output.

How

  • core/mcp/rateLimit.ts (new, pure): parseRetryAfter handles delta-seconds and IMF-fixdate. Date.parse alone reads -5 as a year, so it is not trusted on its own. The module also has isRateLimitError and annotateRateLimit. The rebuilt error keeps the original's class (SdkHttpError), code and status, so any status or code check downstream sees the same thing it did before; the original error is kept as cause.
  • InspectorClient: one annotateRequestError applies both the Report outstanding-request and notification-stream state on a request timeout, not a bare "Request timed out" #2318 timeout annotation and this one. It serves the client.request funnel the timeout diagnostics already decorated (renamed annotateSdkRequestErrors) and the ext-tasks raw-wire channel that bypasses it (modern-era tool calls and tasks/* against a server advertising the tasks extension). So a request in any of the three clients gets it, with no per-call-site catch.
  • Where Retry-After comes from: the SDK's SdkHttpError keeps the status but drops every response header, so the header is read from the fetch tracker. A RateLimitLedger files each 429's header under the JSON-RPC id(s) in its tracked request body. trackSendFailure is the one point that knows which message a failed send was carrying, and it stamps that id's header onto the error. So two requests throttled at once each report their own server's Retry-After, whatever order the responses land in.
  • Web backend: in web mode the upstream fetch runs in the backend. The backend's RemoteSession keeps its own ledger, and the send route takes the header for the id it was sending and relays retryAfter on the typed HTTP error (extending Web client: protocol era "auto" does not fall back to legacy when a 2025-only server rejects the probe with HTTP 400 #2660's RemoteHttpErrorData). That way the browser doesn't depend on a fetch event arriving before the send's HTTP response, which travel over two different connections.

Screenshots

Web client, Tools screen, connected to a fixture server that answers tools/list with 429 Too Many Requests and Retry-After: 30. Same fixture and build steps on both sides; only the source differs (origin/v2/main vs this branch).

Before: the generic SDK error, with nothing to say this is throttling:

Before: "Couldn't load tools — Error POSTing to endpoint: Too many requests"

After: named as rate limiting, with the throttled request and the server's retry timing:

After: "Couldn't load tools — Rate limited by the server (HTTP 429) on tools/list. The server asked to retry after 30s. Error POSTing to endpoint: Too many requests"

The mid-word wrap (HTTP 42/9) is the error box's existing monospace wrapping, unchanged by this PR.

Scope

The issue asks for the 429 + Retry-After signal. Two things are deliberately left out:

  • 503 + Retry-After is not treated as rate limiting. 503 means "unavailable", which is exactly the case the issue wants told apart from throttling.
  • Heuristics like "a batch of failures" or "a latency cliff" are not added. They would be a diagnosis the Inspector can't verify from its own side. That's the same stance connectionDiagnostics.ts takes for timeouts.

Tests

  • src/test/core/mcp/rateLimit.test.ts: every branch of the pure module.
  • src/test/integration/mcp/inspectorClient-rate-limit.test.ts: a hand-rolled throttling server driven through the real Node transport (with a header, without one, a 503 left untouched) and through the real web backend + remote transport. Two more cases overlap two throttled requests (the first one's body is held back until the second has rejected) on both transports. They fail under the original "latest 429" behavior and pass with the id correlation. Two modern-era cases throttle tools/call on a tasks-extension server, which takes the raw wire; they fail with the raw-wire hook reverted to timeout-only.

npm run local:gate passes.

🤖 Generated with Claude Code

A throttled request surfaced as the SDK's bare "Error POSTing to endpoint",
indistinguishable from any other non-OK status. Annotate a 429 on the same
request funnel the #2318 timeout diagnostics use, so all three clients say
it is rate limiting, which request was throttled, and when the server asked
to retry.

The SDK's SdkHttpError drops response headers, so Retry-After comes from the
fetch tracker. On the web path the upstream fetch runs in the backend, which
relays the header on the typed send error rather than leaving the browser to
race a fetch event against the send response.

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 03:48

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

Concurrent requests can report another response’s retry timing in both direct and backend paths.

1 open issue
What changed in this PR

Adds shared HTTP 429 diagnostics so Inspector clients identify throttling and display the server’s retry timing.

Changes:

  • Parses Retry-After and annotates errors while preserving SDK error identity.
  • Relays retry metadata through the web backend.
  • Adds unit and integration coverage.
File Description
core/​mcp/​remote/​types.ts Adds retry metadata to remote errors.
core/​mcp/​remote/​remoteClientTransport.ts Restores relayed retry metadata.
core/​mcp/​remote/​node/​server.ts Includes retry headers in send errors.
core/​mcp/​remote/​node/​remote-session.ts Tracks upstream 429 responses.
core/​mcp/​rateLimit.ts Implements parsing and error annotation.
core/​mcp/​inspectorClient.ts Annotates throttled SDK requests.
clients/​web/​src/​test/​integration/​mcp/​inspectorClient-rate-limit.test.ts Exercises direct and remote transports.
clients/​web/​src/​test/​core/​mcp/​rateLimit.test.ts Tests parsing and annotation helpers.

Review effort: Balanced

Comment thread core/mcp/inspectorClient.ts Outdated
"The latest 429 since the request started" let a concurrent request's
response overwrite another's: request A, whose 429 body was still in flight,
reported request B's Retry-After. File each 429's header under the request
id(s) in its tracked request body, and stamp it onto the error at the point
that knows which message failed — trackSendFailure directly, the send route
on the web backend.

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, mirrored here because inline replies hide once the threads are outdated:

  • Correlate Retry-After with each upstream request (medium): fixed in ea35ae3. Retry-After is now matched by JSON-RPC id through a RateLimitLedger, on both the direct path (trackSendFailure) and the web backend (the send route). Overlap regressions were added for both transports and mutation-checked against the old behavior.

Also added before/after web screenshots to the PR body. npm run local:gate passes on ea35ae3. Requesting round 2.

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

Raw-wire operations bypass rate-limit annotation, and impossible calendar dates can produce incorrect retry timing.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Medium severity Annotate 429 errors on the raw-wire dispatch path

core/​mcp/​inspectorClient.ts:1124

This decorator only covers Client.request. Modern tool calls and task operations use dispatchTaskRequest → RawWireChannel.dispatch, whose send-rejection handler still calls the timeout-only host callback (inspectorClient.ts:539–540). A 429 on that path therefore retains the generic HTTP error and lacks method and retryAfterMs, even after its header is attached. Apply rate-limit annotation to the raw-wire rejection path too, preserving timeout handling, and add a modern-era 429 regression; the new integration cases all force legacy negotiation.

Medium severity Reject impossible dates instead of normalizing them

core/​mcp/​rateLimit.ts:138

Date.parse normalizes some impossible calendar dates instead of returning NaN. For example, Tue, 31 Nov 2026 12:00:00 GMT becomes 1 December, so this reports an invented retry delay rather than showing the malformed header verbatim. Reject dates that do not round-trip to the original IMF-fixdate, and add an impossible-day regression test.

1 resolved since last review

Review effort: Balanced

A modern-era tool call against a server advertising the tasks extension
goes through ext-tasks' raw-wire channel, whose host hook applied only the
timeout annotation, so its 429 kept the generic message. Both paths now
share one annotateRequestError, and the hook is renamed annotateError.

Date.parse rolls an impossible IMF-fixdate (31 Nov) over to a real one, so
a Retry-After is accepted only if it prints back exactly as sent.

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. Both findings were in the "previously missed" block, which has no inline thread, so they are answered here:

  • Annotate 429 errors on the raw-wire dispatch path (medium): fixed in 4c02f4b. InspectorClient.annotateRequestError now applies both the timeout and the rate-limit annotation, and serves both the SDK funnel and RawWireChannel (its host hook is renamed annotateTimeout → annotateError). Regression: modern-era tools/call against a server advertising io.modelcontextprotocol/tasks, which is what routes ext-tasks onto rawDispatch. Without that extension a modern tools/call still goes through client.request, and my first version of this test passed with the fix reverted for exactly that reason. Covered on both the direct and web-backend transports; both cases fail with the raw-wire hook reverted to timeout-only.
  • Reject impossible dates instead of normalizing them (medium): fixed in 4c02f4b. The parsed date must print back via toUTCString() as exactly the header sent, so Tue, 31 Nov 2026 … (rolled to 1 Dec) and a mismatched weekday are both reported verbatim as unparseable. Unit cases added for both.

npm run local:gate passes on 4c02f4b. Requesting round 3.

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 defects remain, and targeted regressions cover request correlation across direct and remote transports, including raw-wire tool calls.

No open issues 🎉

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review loop closed: round 3 was clean ("Approval recommended", no inline comments, no suppressed findings). Three rounds total: round 1's correlation finding and round 2's two findings were all in-scope defects, all fixed and covered by regression tests that fail with the fix reverted.

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] Proactively detect and surface server-side rate limiting

2 participants