diff --git a/clients/web/src/test/core/extension/tasks/rawWireChannel.test.ts b/clients/web/src/test/core/extension/tasks/rawWireChannel.test.ts index eb2300cd4..af9a83169 100644 --- a/clients/web/src/test/core/extension/tasks/rawWireChannel.test.ts +++ b/clients/web/src/test/core/extension/tasks/rawWireChannel.test.ts @@ -23,7 +23,7 @@ function setup(overrides: Partial = {}) { transport: () => transport, defaultTimeoutMs: () => 1_000, resetTimeoutOnProgress: () => true, - annotateTimeout: (error) => error, + annotateError: (error) => error, ...overrides, }); return { channel, sent, transport }; @@ -100,8 +100,8 @@ describe("RawWireChannel", () => { it("times out with the annotated timeout as its cause, re-armed by progress", async () => { vi.useFakeTimers(); - const annotateTimeout = vi.fn((error: unknown) => error); - const { channel, sent } = setup({ annotateTimeout }); + const annotateError = vi.fn((error: unknown) => error); + const { channel, sent } = setup({ annotateError }); const promise = channel.dispatch({ method: "tools/call", params: { _meta: { progressToken: "p" } }, @@ -121,10 +121,7 @@ describe("RawWireChannel", () => { error instanceof DispatchError && (error.cause as { code?: unknown }).code !== undefined, ); - expect(annotateTimeout).toHaveBeenCalledWith( - expect.anything(), - "tools/call", - ); + expect(annotateError).toHaveBeenCalledWith(expect.anything(), "tools/call"); // stdio-style transport: the timeout reaches the server as a cancel. expect(sent[1]).toMatchObject({ method: "notifications/cancelled" }); }); @@ -166,8 +163,8 @@ describe("RawWireChannel", () => { it("annotates a send failure and rejects everything on rejectAll", async () => { const boom = new Error("closed"); - const annotateTimeout = vi.fn(() => boom); - const failing = setup({ annotateTimeout }); + const annotateError = vi.fn(() => boom); + const failing = setup({ annotateError }); failing.transport.send.mockRejectedValueOnce("raw failure"); await expect( failing.channel.dispatch({ method: "tasks/get" }), diff --git a/clients/web/src/test/core/mcp/rateLimit.test.ts b/clients/web/src/test/core/mcp/rateLimit.test.ts new file mode 100644 index 000000000..8e3b628b9 --- /dev/null +++ b/clients/web/src/test/core/mcp/rateLimit.test.ts @@ -0,0 +1,285 @@ +import { describe, it, expect } from "vitest"; +import { + SdkError, + SdkErrorCode, + SdkHttpError, +} from "@modelcontextprotocol/client"; +import { + annotateRateLimit, + attachRetryAfter, + isRateLimitError, + MAX_UNCLAIMED_RATE_LIMITS, + parseRetryAfter, + RateLimitLedger, + requestIdsOf, + type AnnotatedRateLimitData, +} from "@inspector/core/mcp/rateLimit.js"; +import type { FetchRequestEntryBase } from "@inspector/core/mcp/types.js"; + +const NOW = Date.parse("2026-10-19T12:00:00Z"); + +function entry( + overrides: Partial = {}, +): FetchRequestEntryBase { + return { + id: "fetch-1", + timestamp: new Date(NOW), + method: "POST", + url: "http://localhost/mcp", + requestHeaders: {}, + requestBody: JSON.stringify({ + jsonrpc: "2.0", + id: 7, + method: "tools/list", + }), + responseStatus: 429, + responseHeaders: { "Retry-After": "30" }, + ...overrides, + }; +} + +function throttled(data: Record = {}): SdkHttpError { + return new SdkHttpError( + SdkErrorCode.ClientHttpNotImplemented, + "Error POSTing to endpoint: slow down", + { status: 429, statusText: "Too Many Requests", ...data }, + ); +} + +describe("requestIdsOf", () => { + it("reads a single request's id, string or number", () => { + expect( + requestIdsOf('{"jsonrpc":"2.0","id":7,"method":"tools/list"}'), + ).toEqual([7]); + expect(requestIdsOf('{"jsonrpc":"2.0","id":"a","method":"x"}')).toEqual([ + "a", + ]); + }); + + it("reads every request in a batch, skipping notifications and junk", () => { + expect( + requestIdsOf( + JSON.stringify([ + { jsonrpc: "2.0", id: 1, method: "a" }, + { jsonrpc: "2.0", method: "notifications/x" }, + { jsonrpc: "2.0", id: 2, result: {} }, + { jsonrpc: "2.0", id: null, method: "b" }, + null, + { jsonrpc: "2.0", id: 3, method: "c" }, + ]), + ), + ).toEqual([1, 3]); + }); + + it("finds nothing in a missing or non-JSON body", () => { + expect(requestIdsOf(undefined)).toEqual([]); + expect(requestIdsOf("not json")).toEqual([]); + expect(requestIdsOf("42")).toEqual([]); + }); +}); + +describe("RateLimitLedger", () => { + it("files a 429's header, whatever its casing, under its request id, once", () => { + const ledger = new RateLimitLedger(); + ledger.record(entry()); + expect(ledger.take(7)).toBe("30"); + expect(ledger.take(7)).toBeUndefined(); + }); + + it("keeps each overlapping request's own header", () => { + const ledger = new RateLimitLedger(); + ledger.record(entry()); + ledger.record( + entry({ + requestBody: JSON.stringify({ jsonrpc: "2.0", id: 8, method: "x" }), + responseHeaders: { "retry-after": "5" }, + }), + ); + expect(ledger.take(7)).toBe("30"); + expect(ledger.take(8)).toBe("5"); + }); + + it("files a 429 with no header as having none", () => { + const ledger = new RateLimitLedger(); + ledger.record(entry({ responseHeaders: {} })); + expect(ledger.take(7)).toBeUndefined(); + }); + + it("ignores anything that is not a 429", () => { + const ledger = new RateLimitLedger(); + ledger.record(entry({ responseStatus: 503 })); + ledger.record(entry({ responseStatus: undefined })); + expect(ledger.take(7)).toBeUndefined(); + }); + + it("drops the oldest unclaimed 429 past its cap", () => { + const ledger = new RateLimitLedger(); + for (let id = 0; id <= MAX_UNCLAIMED_RATE_LIMITS; id++) { + ledger.record( + entry({ + requestBody: JSON.stringify({ jsonrpc: "2.0", id, method: "x" }), + }), + ); + } + expect(ledger.take(0)).toBeUndefined(); + expect(ledger.take(1)).toBe("30"); + expect(ledger.take(MAX_UNCLAIMED_RATE_LIMITS)).toBe("30"); + }); + + it("forgets everything on clear", () => { + const ledger = new RateLimitLedger(); + ledger.record(entry()); + ledger.clear(); + expect(ledger.take(7)).toBeUndefined(); + }); +}); + +describe("attachRetryAfter", () => { + it("stamps the header onto a 429's data", () => { + const err = throttled(); + attachRetryAfter(err, "30"); + expect(err.data.retryAfter).toBe("30"); + }); + + it("keeps a header already relayed on the error", () => { + const err = throttled({ retryAfter: "5" }); + attachRetryAfter(err, "30"); + expect(err.data.retryAfter).toBe("5"); + }); + + it("leaves a non-429, or a missing header, alone", () => { + const unavailable = throttled({ status: 503 }); + attachRetryAfter(unavailable, "30"); + expect(unavailable.data.retryAfter).toBeUndefined(); + const err = throttled(); + attachRetryAfter(err, undefined); + expect(err.data).not.toHaveProperty("retryAfter"); + expect(() => attachRetryAfter(undefined, "30")).not.toThrow(); + }); +}); + +describe("parseRetryAfter", () => { + it("reads delta-seconds", () => { + expect(parseRetryAfter("30", NOW)).toBe(30_000); + expect(parseRetryAfter(" 0 ", NOW)).toBe(0); + }); + + it("reads an HTTP-date as the delay from now", () => { + expect(parseRetryAfter("Mon, 19 Oct 2026 12:01:30 GMT", NOW)).toBe(90_000); + }); + + it("clamps a date already past to zero", () => { + expect(parseRetryAfter("Mon, 19 Oct 2026 11:00:00 GMT", NOW)).toBe(0); + }); + + it("treats a missing or malformed value as absent", () => { + expect(parseRetryAfter(undefined, NOW)).toBeUndefined(); + expect(parseRetryAfter("soon", NOW)).toBeUndefined(); + // `Date.parse` alone reads these as dates. + expect(parseRetryAfter("-5", NOW)).toBeUndefined(); + expect(parseRetryAfter("1.5", NOW)).toBeUndefined(); + expect(parseRetryAfter("2026-10-19T12:01:30Z", NOW)).toBeUndefined(); + // The right shape, but no real date: `Date.parse` rolls an impossible + // day over (31 Nov → 1 Dec) and ignores a weekday that does not match. + expect(parseRetryAfter("Tue, 31 Nov 2026 12:00:00 GMT", NOW)).toBe( + undefined, + ); + expect(parseRetryAfter("Tue, 19 Oct 2026 12:01:30 GMT", NOW)).toBe( + undefined, + ); + expect(parseRetryAfter("Mon, 99 Foo 2026 12:00:00 GMT", NOW)).toBe( + undefined, + ); + }); +}); + +describe("isRateLimitError", () => { + it("matches only an SdkHttpError with status 429", () => { + expect(isRateLimitError(throttled())).toBe(true); + expect(isRateLimitError(throttled({ status: 503 }))).toBe(false); + expect( + isRateLimitError(new SdkError(SdkErrorCode.RequestTimeout, "timed out")), + ).toBe(false); + expect( + isRateLimitError(Object.assign(new Error("x"), { status: 429 })), + ).toBe(false); + expect(isRateLimitError(undefined)).toBe(false); + }); +}); + +describe("annotateRateLimit", () => { + it("names the method and the retry delay, keeping class, code, status and cause", () => { + const original = throttled({ retryAfter: "30" }); + const annotated = annotateRateLimit(original, "tools/call", NOW); + + expect(SdkHttpError.isInstance(annotated)).toBe(true); + const error = annotated as SdkHttpError; + expect(error.message).toBe( + "Rate limited by the server (HTTP 429) on tools/call. " + + "The server asked to retry after 30s. " + + "Error POSTing to endpoint: slow down", + ); + expect(error.code).toBe(SdkErrorCode.ClientHttpNotImplemented); + expect(error.status).toBe(429); + expect(error.statusText).toBe("Too Many Requests"); + expect(error.cause).toBe(original); + const data = error.data as AnnotatedRateLimitData; + expect(data.method).toBe("tools/call"); + expect(data.retryAfter).toBe("30"); + expect(data.retryAfterMs).toBe(30_000); + }); + + it("ignores a retryAfter on the data that is not a string", () => { + const annotated = annotateRateLimit( + throttled({ retryAfter: 5 }), + "tools/list", + NOW, + ) as SdkHttpError; + expect(annotated.message).toContain( + "The server sent no Retry-After header.", + ); + }); + + it("says when the server sent no Retry-After", () => { + const annotated = annotateRateLimit( + throttled(), + "tools/list", + NOW, + ) as SdkHttpError; + expect(annotated.message).toContain( + "The server sent no Retry-After header.", + ); + const data = annotated.data as AnnotatedRateLimitData; + expect(data.retryAfter).toBeUndefined(); + expect(data.retryAfterMs).toBeUndefined(); + }); + + it("reports an unparseable Retry-After verbatim rather than guessing", () => { + const annotated = annotateRateLimit( + throttled({ retryAfter: "soon" }), + "tools/list", + NOW, + ) as SdkHttpError; + expect(annotated.message).toContain( + "The server sent an unparseable Retry-After (soon).", + ); + const data = annotated.data as AnnotatedRateLimitData; + expect(data.retryAfter).toBe("soon"); + expect(data.retryAfterMs).toBeUndefined(); + }); + + it("defaults the clock to now", () => { + const annotated = annotateRateLimit( + throttled({ retryAfter: "1" }), + "tools/list", + ) as SdkHttpError; + expect(annotated.message).toContain("retry after 1s"); + }); + + it("returns anything that is not a 429 untouched", () => { + const unavailable = throttled({ status: 503 }); + expect(annotateRateLimit(unavailable, "tools/list", NOW)).toBe(unavailable); + const timeout = new SdkError(SdkErrorCode.RequestTimeout, "timed out"); + expect(annotateRateLimit(timeout, "tools/list", NOW)).toBe(timeout); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-rate-limit.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-rate-limit.test.ts new file mode 100644 index 000000000..ff21cd261 --- /dev/null +++ b/clients/web/src/test/integration/mcp/inspectorClient-rate-limit.test.ts @@ -0,0 +1,424 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { + createServer, + type IncomingMessage, + type Server, + type ServerResponse, +} from "node:http"; +import { serve, type ServerType } from "@hono/node-server"; +import { SdkErrorCode, SdkHttpError } from "@modelcontextprotocol/client"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { createRemoteTransport } from "@inspector/core/mcp/remote/createRemoteTransport.js"; +import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; +import { + eraToVersionNegotiation, + type CreateTransport, +} from "@inspector/core/mcp/types.js"; +import type { AnnotatedRateLimitData } from "@inspector/core/mcp/rateLimit.js"; + +/** + * Rate-limit recognition against a real connection (#2442). + * + * A server that answers `initialize` and then throttles `tools/list` with a + * `429`. The SDK's `SdkHttpError` keeps the status but drops the response + * headers, so the `Retry-After` in the message can only have come from the + * fetch tracker — directly on a Node transport, and relayed on the send error + * by the web backend on the remote one. Both are driven end to end here. + * + * Hand-rolled rather than a composable test server because the SDK's own + * server cannot be configured to answer one method with a 429, let alone hold + * one 429's body back while another overtakes it. + */ + +interface ToolsListReply { + status: number; + headers?: Record; + body: string; +} + +interface ThrottlingServer { + url: string; + setToolsListReply: (reply: ToolsListReply) => void; + /** + * The methods of the overlapping pair (`prompts/list`, `resources/list`), in + * the order they arrived. The first is answered `429` + `Retry-After: 30` + * with its body held back; the second `429` + `Retry-After: 5` at once. + */ + overlapArrivals: () => string[]; + /** Finish the held first response of the overlapping pair. */ + releaseFirstOverlap: () => void; + stop: () => Promise; +} + +const OVERLAPPING = new Set(["prompts/list", "resources/list"]); + +const THROTTLED: ToolsListReply = { + status: 429, + headers: { "retry-after": "30" }, + body: "slow down", +}; + +function startThrottlingServer(): Promise { + let toolsListReply = THROTTLED; + const overlapArrivals: string[] = []; + let releaseFirstOverlap = () => {}; + + const handler = async (req: IncomingMessage, res: ServerResponse) => { + // No standalone notification stream; nothing here needs one. + if (req.method === "GET") { + res.writeHead(405).end(); + return; + } + if (req.method === "DELETE") { + res.writeHead(200).end(); + return; + } + const chunks: Buffer[] = []; + for await (const chunk of req) chunks.push(chunk as Buffer); + const body = JSON.parse(Buffer.concat(chunks).toString()) as { + id?: unknown; + method: string; + }; + if (body.id === undefined) { + res.writeHead(202).end(); + return; + } + if (body.method === "tools/call") { + res.writeHead(THROTTLED.status, { + "content-type": "text/plain", + ...THROTTLED.headers, + }); + res.end(THROTTLED.body); + return; + } + if (body.method === "tools/list") { + res.writeHead(toolsListReply.status, { + "content-type": "text/plain", + ...toolsListReply.headers, + }); + res.end(toolsListReply.body); + return; + } + if (OVERLAPPING.has(body.method)) { + overlapArrivals.push(body.method); + const first = overlapArrivals.length === 1; + res.writeHead(429, { + "content-type": "text/plain", + "retry-after": first ? "30" : "5", + }); + if (first) { + // Headers out now, so the 429 is tracked; the SDK's error waits on + // the body, which only the test releases. + res.flushHeaders(); + releaseFirstOverlap = () => res.end("first"); + } else { + res.end("second"); + } + return; + } + if (body.method === "server/discover") { + // Only a modern-era connect sends this; the legacy-pinned cases never do. + res.writeHead(200, { "content-type": "application/json" }); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + id: body.id, + result: { + resultType: "complete", + ttlMs: 0, + cacheScope: "public", + supportedVersions: ["2026-07-28"], + // The tasks extension is what routes a modern `tools/call` onto + // ext-tasks' raw-wire channel instead of the SDK's `request`. + capabilities: { + tools: {}, + extensions: { "io.modelcontextprotocol/tasks": {} }, + }, + serverInfo: { name: "throttling-server", version: "1.0.0" }, + }, + }), + ); + return; + } + const result = + body.method === "initialize" + ? { + protocolVersion: "2025-06-18", + capabilities: { tools: {}, prompts: {}, resources: {} }, + serverInfo: { name: "throttling-server", version: "1.0.0" }, + } + : {}; + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ jsonrpc: "2.0", id: body.id, result })); + }; + + const server: Server = createServer((req, res) => { + void handler(req, res); + }); + + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + /* v8 ignore next -- listen() on a fresh server always yields an AddressInfo */ + const port = typeof address === "object" && address ? address.port : 0; + resolve({ + url: `http://127.0.0.1:${port}/mcp`, + setToolsListReply: (reply) => { + toolsListReply = reply; + }, + overlapArrivals: () => [...overlapArrivals], + releaseFirstOverlap: () => releaseFirstOverlap(), + stop: () => + new Promise((done) => { + server.closeAllConnections(); + server.close(() => done()); + }), + }); + }); + }); +} + +/** The web backend on an ephemeral port, as the browser's remote transport sees it. */ +function startRemoteBackend(): Promise<{ + server: ServerType; + transport: CreateTransport; +}> { + const { app, authToken } = createRemoteApp({ + initialConfig: { defaultEnvironment: {} }, + }); + return new Promise((resolve, reject) => { + const server = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + resolve({ + server, + transport: createRemoteTransport({ + baseUrl: `http://127.0.0.1:${info.port}`, + authToken, + }), + }); + }, + ); + server.on("error", reject); + }); +} + +/** Poll until `predicate` holds; the failure names what never became true. */ +async function waitUntil( + what: string, + predicate: () => boolean, + timeoutMs = 5000, +): Promise { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + if (Date.now() > deadline) { + throw new Error(`timed out waiting for ${what}`); + } + await new Promise((r) => setTimeout(r, 10)); + } +} + +async function rejectionOf(promise: Promise): Promise { + try { + await promise; + } catch (err) { + return err; + } + throw new Error("expected the request to reject"); +} + +describe("InspectorClient rate-limit recognition (#2442)", () => { + let client: InspectorClient | null = null; + let upstream: ThrottlingServer | null = null; + let backend: ServerType | null = null; + + async function connectTo( + url: string, + transport: CreateTransport = createTransportNode, + era: "legacy" | "modern" = "legacy", + ): Promise { + client = new InspectorClient( + { type: "streamable-http", url }, + { + environment: { transport }, + // Pinned, so the connect is exactly `initialize` (legacy) or + // `server/discover` (modern) and nothing else. + versionNegotiation: eraToVersionNegotiation(era), + }, + ); + await client.connect(); + return client; + } + + afterEach(async () => { + try { + await client?.disconnect(); + } catch { + // Teardown only — the assertions already ran. + } + client = null; + await upstream?.stop(); + upstream = null; + if (backend) { + const closing = backend; + await new Promise((done) => closing.close(() => done())); + backend = null; + } + }); + + function expectThrottled(caught: unknown): AnnotatedRateLimitData { + expect(SdkHttpError.isInstance(caught)).toBe(true); + const error = caught as SdkHttpError; + // Same class, code and status as the SDK's own error: only the message + // and the data grew. + expect(error.code).toBe(SdkErrorCode.ClientHttpNotImplemented); + expect(error.status).toBe(429); + return error.data as AnnotatedRateLimitData; + } + + it("names the throttled request and the server's Retry-After", async () => { + upstream = await startThrottlingServer(); + const c = await connectTo(upstream.url); + + const caught = await rejectionOf(c.listTools()); + + const data = expectThrottled(caught); + expect((caught as Error).message).toBe( + "Rate limited by the server (HTTP 429) on tools/list. " + + "The server asked to retry after 30s. " + + "Error POSTing to endpoint: slow down", + ); + expect(data.method).toBe("tools/list"); + expect(data.retryAfter).toBe("30"); + expect(data.retryAfterMs).toBe(30_000); + expect(SdkHttpError.isInstance((caught as Error).cause)).toBe(true); + }); + + it("says so when the server sent no Retry-After", async () => { + upstream = await startThrottlingServer(); + upstream.setToolsListReply({ status: 429, body: "slow down" }); + const c = await connectTo(upstream.url); + + const caught = await rejectionOf(c.listTools()); + + const data = expectThrottled(caught); + expect((caught as Error).message).toContain( + "The server sent no Retry-After header.", + ); + expect(data.retryAfter).toBeUndefined(); + }); + + it("leaves a non-429 failure exactly as the SDK threw it", async () => { + upstream = await startThrottlingServer(); + upstream.setToolsListReply({ + status: 503, + headers: { "retry-after": "30" }, + body: "down for maintenance", + }); + const c = await connectTo(upstream.url); + + const caught = await rejectionOf(c.listTools()); + + expect(SdkHttpError.isInstance(caught)).toBe(true); + expect((caught as SdkHttpError).status).toBe(503); + expect((caught as Error).message).toBe( + "Error POSTing to endpoint: down for maintenance", + ); + }); + + it("relays the Retry-After through the web backend", async () => { + upstream = await startThrottlingServer(); + const started = await startRemoteBackend(); + backend = started.server; + const c = await connectTo(upstream.url, started.transport); + + const caught = await rejectionOf(c.listTools()); + + const data = expectThrottled(caught); + expect((caught as Error).message).toMatch( + /^Rate limited by the server \(HTTP 429\) on tools\/list\. The server asked to retry after 30s\. /, + ); + expect(data.retryAfter).toBe("30"); + }); + + /** + * Two requests throttled at once, the first one's response overtaken by + * the second's. Each must report its own response's `Retry-After`; picking + * "the latest 429" would hand the first request the second's 5s. + */ + async function expectOverlapCorrelated(c: InspectorClient): Promise { + const first = rejectionOf(c.listPrompts()); + await waitUntil( + "the first request to arrive", + () => upstream!.overlapArrivals().length === 1, + ); + const second = await rejectionOf(c.listResources()); + upstream!.releaseFirstOverlap(); + const firstCaught = await first; + + expect(upstream!.overlapArrivals()).toEqual([ + "prompts/list", + "resources/list", + ]); + expect(expectThrottled(firstCaught).retryAfter).toBe("30"); + expect((firstCaught as Error).message).toContain( + "on prompts/list. The server asked to retry after 30s.", + ); + expect(expectThrottled(second).retryAfter).toBe("5"); + expect((second as Error).message).toContain( + "on resources/list. The server asked to retry after 5s.", + ); + } + + it("gives each of two overlapping 429s its own Retry-After", async () => { + upstream = await startThrottlingServer(); + await expectOverlapCorrelated(await connectTo(upstream.url)); + }); + + it("gives each of two overlapping 429s its own Retry-After through the web backend", async () => { + upstream = await startThrottlingServer(); + const started = await startRemoteBackend(); + backend = started.server; + await expectOverlapCorrelated( + await connectTo(upstream.url, started.transport), + ); + }); + + /** + * Against a modern server advertising the tasks extension, a tool call + * bypasses the SDK's `Client.request` for the ext-tasks raw-wire channel, + * so this is the case that proves the annotation is applied on that path + * too, not only on the SDK funnel. (`tools/list` is no use here: it takes + * the SDK funnel in both eras, and so does `tools/call` without the + * extension.) + */ + async function expectModernThrottled(c: InspectorClient): Promise { + expect(c.getProtocolEra()).toBe("modern"); + const caught = await rejectionOf( + c.callTool({ name: "echo", inputSchema: { type: "object" } }, {}), + ); + const data = expectThrottled(caught); + expect((caught as Error).message).toMatch( + /^Rate limited by the server \(HTTP 429\) on tools\/call\. The server asked to retry after 30s\. /, + ); + expect(data.method).toBe("tools/call"); + expect(data.retryAfterMs).toBe(30_000); + } + + it("annotates a modern-era 429, which takes the raw-wire path", async () => { + upstream = await startThrottlingServer(); + await expectModernThrottled( + await connectTo(upstream.url, createTransportNode, "modern"), + ); + }); + + it("annotates a modern-era 429 through the web backend", async () => { + upstream = await startThrottlingServer(); + const started = await startRemoteBackend(); + backend = started.server; + await expectModernThrottled( + await connectTo(upstream.url, started.transport, "modern"), + ); + }); +}); diff --git a/core/extension/tasks/rawWireChannel.ts b/core/extension/tasks/rawWireChannel.ts index 455062c5f..e2c401715 100644 --- a/core/extension/tasks/rawWireChannel.ts +++ b/core/extension/tasks/rawWireChannel.ts @@ -47,9 +47,9 @@ export interface RawWireChannelHost { defaultTimeoutMs(): number; /** Whether progress on a request's token re-arms its timeout. */ resetTimeoutOnProgress(): boolean; - /** Decorate a timeout the way an SDK request's timeout is (#2318); any - * other error is returned untouched. */ - annotateTimeout(error: unknown, method: string): unknown; + /** Decorate a failed request's error the way an SDK request's is — a + * timeout (#2318), a `429` (#2442); any other error is returned untouched. */ + annotateError(error: unknown, method: string): unknown; } interface PendingRawRequest { @@ -182,7 +182,7 @@ export class RawWireChannel { `Raw MCP request "${message.method}" timed out after ${timeoutMs} ms`, false, { - cause: this.host.annotateTimeout( + cause: this.host.annotateError( new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", { timeout: timeoutMs, }), @@ -232,10 +232,10 @@ export class RawWireChannel { pending.cleanup(); // The browser's remote transport awaits the response inside `send`, // so its relay wait can expire here first, as the SDK's timeout - // shape; annotate it exactly as the local timer above does (a - // non-timeout error passes through untouched). + // shape; annotate it exactly as the local timer above does. A `429` + // fails the send here too, and is annotated the same way (#2442). reject( - this.host.annotateTimeout( + this.host.annotateError( error instanceof Error ? error : new Error(String(error)), message.method, ), diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 818c4f60e..83dfc14ee 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -296,6 +296,11 @@ import { type NotificationStreamState, type OutstandingRequest, } from "./connectionDiagnostics.js"; +import { + annotateRateLimit, + attachRetryAfter, + RateLimitLedger, +} from "./rateLimit.js"; import { OAuthManager, type OAuthManagerConfig } from "./oauthManager.js"; import { RemoteClientTransport } from "./remote/remoteClientTransport.js"; @@ -531,8 +536,7 @@ export class InspectorClient extends InspectorClientEventTarget { transport: () => this.transport, defaultTimeoutMs: () => this.requestTimeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC, resetTimeoutOnProgress: () => this.resetTimeoutOnProgress, - annotateTimeout: (error, method) => - annotateRequestTimeout(error, method, this.getConnectionDiagnostics()), + annotateError: (error, method) => this.annotateRequestError(error, method), }); private readonly dispatchTaskRequest: RawClientDispatch = ( request, @@ -555,6 +559,10 @@ export class InspectorClient extends InspectorClientEventTarget { // `GET` on Streamable HTTP). Both per-session, reset with the maps above. private lastResponse: LastResponse | undefined; private notificationStream: TrackedNotificationStream | undefined; + // Each tracked `429`'s `Retry-After`, by the JSON-RPC id that drew it, until + // that id's failed send claims it (#2442). Per-session, reset with the + // diagnostics above. + private readonly rateLimits = new RateLimitLedger(); /** True when the cached transport was built with an OAuth authProvider attached. */ private transportHasAuthProvider = false; /** Dedupes concurrent ambient auth challenges (reason + scopes). */ @@ -1071,7 +1079,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.clientInfo, Object.keys(clientOptions).length > 0 ? clientOptions : undefined, ); - this.annotateSdkRequestTimeouts(this.client); + this.annotateSdkRequestErrors(this.client); if (this.tasksCapabilityAdvertised) { this.bindReceiverTasks(); } @@ -1080,7 +1088,8 @@ export class InspectorClient extends InspectorClientEventTarget { /** * Decorate the SDK Client's `request` so a per-request timeout rejects with * the connection's state at that moment instead of a bare `Request timed - * out` (#2318). + * out` (#2318), and a `429` rejects as rate limiting with the server's + * `Retry-After` instead of a bare `Error POSTing to endpoint` (#2442). * * `Protocol.request` is the one funnel every SDK verb goes through — * `listTools`, `callTool`, `readResource`, the handshake's `initialize`, @@ -1098,7 +1107,7 @@ export class InspectorClient extends InspectorClientEventTarget { * implementation signature serves both at runtime; the single cast on the * way back is what the overloads cost. */ - private annotateSdkRequestTimeouts(client: Client): void { + private annotateSdkRequestErrors(client: Client): void { type SdkRequest = Client["request"]; const sdkRequest: SdkRequest = client.request.bind(client); const decorated = async ( @@ -1107,16 +1116,26 @@ export class InspectorClient extends InspectorClientEventTarget { try { return await sdkRequest(...args); } catch (err) { - throw annotateRequestTimeout( - err, - args[0].method, - this.getConnectionDiagnostics(), - ); + throw this.annotateRequestError(err, args[0].method); } }; client.request = decorated as SdkRequest; } + /** + * The one place a failed request's error is decorated, shared by the SDK + * funnel above and the raw-wire channel that bypasses it (modern-era tool + * calls and task operations), so a caller sees the same error whichever + * path its request took: a timeout with the connection's state (#2318), a + * `429` named as rate limiting (#2442), anything else untouched. + */ + private annotateRequestError(err: unknown, method: string): unknown { + return annotateRateLimit( + annotateRequestTimeout(err, method, this.getConnectionDiagnostics()), + method, + ); + } + /** * @param base - the composed OAuth-path fetch, deadline innermost (#2319). * @@ -1178,6 +1197,10 @@ export class InspectorClient extends InspectorClientEventTarget { // saw as unanswered, in every timeout message and Connection Info // row, until the session resets (#2318). if (isRequestTimeoutError(error)) return; + // The SDK's 429 error carries no headers; this is the one point that + // knows which request it failed, so it is where that request's own + // `Retry-After` goes onto it (#2442). + attachRetryAfter(error, this.rateLimits.take(message.id)); if (this.outstandingRequests.delete(message.id)) { this.dispatchConnectionDiagnosticsChange(); } @@ -1673,6 +1696,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.taskProgress.clear(); this.lastResponse = undefined; this.notificationStream = undefined; + this.rateLimits.clear(); this.dispatchConnectionDiagnosticsChange(); // Per-session for the same reason: both name entries of the PREVIOUS // server's list. Cleared here as well as in `disconnect()` because the @@ -1813,6 +1837,7 @@ export class InspectorClient extends InspectorClientEventTarget { onFetchRequest: (entry: FetchRequestEntryBase) => { this.dispatchFetchRequest({ ...entry, category: "transport" }); this.noteTransportStream(entry); + this.rateLimits.record(entry); }, onFetchResponseBody: (id: string, body: string) => { this.dispatchFetchRequestBodyUpdate(id, body); diff --git a/core/mcp/rateLimit.ts b/core/mcp/rateLimit.ts new file mode 100644 index 000000000..6696e79e4 --- /dev/null +++ b/core/mcp/rateLimit.ts @@ -0,0 +1,230 @@ +/** + * Rate-limit recognition: the one place that turns a server's `HTTP 429` into + * a message saying the server is throttling, and when it says to retry + * (#2442). + * + * Before this, a throttled request surfaced as the SDK's + * `Error POSTing to endpoint: ` — the same shape as every other non-OK + * status, so telling "the server is down" from "the server is pacing me" was + * left to the reader. Both facts needed to say more were already in-process: + * the error carries the status, and the fetch tracker recorded the response's + * `Retry-After` header. + * + * The header has to come from the tracker because the SDK's `SdkHttpError` + * keeps the status and the body but none of the response headers. It is + * matched to its error by JSON-RPC id, not by time: a {@link RateLimitLedger} + * files each 429's header under the request id(s) in the tracked request body, + * and the code that sees the failed send — which knows the message it was + * sending — takes that id's header and stamps it onto the error's data as + * `retryAfter` ({@link attachRetryAfter}). Two requests throttled at once + * therefore each report their own server's word, whatever order their + * responses land in. + * + * Where the upstream fetch runs in the web backend, the backend does the + * stamping from its own ledger and relays `retryAfter` on the send error (see + * `RemoteHttpErrorData`), so the browser does not depend on a fetch event + * racing the send's HTTP response across two connections. Either way, by the + * time {@link annotateRateLimit} sees the error, the header is on it. + * + * Deliberately narrow: only a 429 is recognized. A 503 also carries + * `Retry-After` but means "unavailable", which is exactly the case this exists + * to tell apart from throttling. + */ + +import { + SdkHttpError, + type SdkHttpErrorData, +} from "@modelcontextprotocol/client"; +import { findHeader } from "./fetchTracking.js"; +import { formatDuration } from "./connectionDiagnostics.js"; +import type { FetchRequestEntryBase } from "./types.js"; + +/** `429 Too Many Requests` (RFC 6585). */ +export const HTTP_TOO_MANY_REQUESTS = 429; + +/** + * The JSON-RPC request ids in a tracked request body: one for a single + * request, several for a batch, none for a notification, a response, or a + * body that is not JSON. The tracker records the body redacted, but redaction + * masks only sensitive string fields, so `id` survives it. + */ +export function requestIdsOf(body: string | undefined): (string | number)[] { + if (body === undefined) return []; + let parsed: unknown; + try { + parsed = JSON.parse(body); + } catch { + return []; + } + const messages: unknown[] = Array.isArray(parsed) ? parsed : [parsed]; + const ids: (string | number)[] = []; + for (const message of messages) { + if (typeof message !== "object" || message === null) continue; + if (!("method" in message) || !("id" in message)) continue; + const { id } = message; + if (typeof id === "string" || typeof id === "number") ids.push(id); + } + return ids; +} + +/** + * How many unclaimed 429s a ledger holds before it drops the oldest. A 429 is + * normally claimed within the same tick its send fails; one that never is (a + * send aborted mid-flight is not reported as a failure) would otherwise stay + * for the rest of the session. + */ +export const MAX_UNCLAIMED_RATE_LIMITS = 100; + +/** + * Each tracked 429's `Retry-After`, filed under the JSON-RPC id of the request + * that drew it, until the failed send for that id claims it. + */ +export class RateLimitLedger { + // `undefined` is a real value here: a 429 that carried no header. + private readonly byRequestId = new Map(); + + /** File a tracked fetch, if it was a 429. Anything else is ignored. */ + record(entry: FetchRequestEntryBase): void { + if (entry.responseStatus !== HTTP_TOO_MANY_REQUESTS) return; + const retryAfter = findHeader(entry.responseHeaders, "retry-after"); + for (const id of requestIdsOf(entry.requestBody)) { + this.byRequestId.delete(id); + this.byRequestId.set(id, retryAfter); + if (this.byRequestId.size > MAX_UNCLAIMED_RATE_LIMITS) { + const [oldest] = this.byRequestId.keys(); + /* v8 ignore next -- size > 0 was just checked, so a first key exists */ + if (oldest !== undefined) this.byRequestId.delete(oldest); + } + } + } + + /** The `Retry-After` filed for `id`, removed as it is read. */ + take(id: string | number): string | undefined { + const retryAfter = this.byRequestId.get(id); + this.byRequestId.delete(id); + return retryAfter; + } + + clear(): void { + this.byRequestId.clear(); + } +} + +/** + * The IMF-fixdate form of an HTTP-date (`Sun, 06 Nov 1994 08:49:37 GMT`), + * which RFC 9110 §5.6.7 requires senders to use. Matched before `Date.parse` + * because `Date.parse` is lenient past usefulness: it reads `-5` as the year + * 5 BC, which would report a malformed header as "retry now". + */ +const IMF_FIXDATE = + /^[A-Z][a-z]{2}, \d{2} [A-Z][a-z]{2} \d{4} \d{2}:\d{2}:\d{2} GMT$/; + +/** + * A `Retry-After` value as a delay in ms from `now` (RFC 9110 §10.2.3): either + * a non-negative integer count of seconds, or an HTTP-date. A date already in + * the past is a delay of `0`. Anything else is `undefined` — a malformed header + * is reported verbatim rather than guessed at. + */ +export function parseRetryAfter( + value: string | undefined, + now: number, +): number | undefined { + if (value === undefined) return undefined; + const trimmed = value.trim(); + if (/^\d+$/.test(trimmed)) return Number(trimmed) * 1000; + if (!IMF_FIXDATE.test(trimmed)) return undefined; + const at = Date.parse(trimmed); + // A well-formed shape can still name no real date. `Mon, 99 Foo 2026 …` is + // `NaN`, but `Tue, 31 Nov 2026 …` silently rolls over to 1 December, so the + // parse is accepted only if it prints back as exactly what was sent — + // `toUTCString` emits IMF-fixdate, so a real date always does. + if (Number.isNaN(at) || new Date(at).toUTCString() !== trimmed) { + return undefined; + } + return Math.max(0, at - now); +} + +/** Whether `err` is the SDK's HTTP error for a `429 Too Many Requests`. */ +export function isRateLimitError(err: unknown): err is SdkHttpError { + return SdkHttpError.isInstance(err) && err.status === HTTP_TOO_MANY_REQUESTS; +} + +/** + * The data carried by an annotated rate-limit error: the SDK's own HTTP data, + * plus the method that was throttled and the retry timing when the server + * gave one. Exposed so a consumer that wants the structured form (a countdown, + * a CLI's JSON output) can read it off `error.data` instead of parsing the + * message. + */ +export interface AnnotatedRateLimitData extends SdkHttpErrorData { + method: string; + /** The raw `Retry-After` header, when the server sent one. */ + retryAfter?: string; + /** The parsed delay, when `retryAfter` was well-formed. */ + retryAfterMs?: number; +} + +function retryAfterOn(err: SdkHttpError): string | undefined { + const value = err.data.retryAfter; + return typeof value === "string" ? value : undefined; +} + +/** + * Stamp a 429's own `Retry-After` onto its error's data, where + * {@link annotateRateLimit} reads it. A header already there wins: on the + * browser's remote transport the backend relayed it from the upstream + * response itself, which nothing on this side can know better. Anything that + * is not a 429, or a missing header, leaves the error untouched. + */ +export function attachRetryAfter( + err: unknown, + retryAfter: string | undefined, +): void { + if (!isRateLimitError(err) || retryAfter === undefined) return; + if (retryAfterOn(err) !== undefined) return; + err.data.retryAfter = retryAfter; +} + +function describeRetry( + retryAfter: string | undefined, + retryAfterMs: number | undefined, +): string { + if (retryAfterMs !== undefined) { + return `The server asked to retry after ${formatDuration(retryAfterMs)}.`; + } + if (retryAfter !== undefined) { + return `The server sent an unparseable Retry-After (${retryAfter}).`; + } + return "The server sent no Retry-After header."; +} + +/** + * Rebuild a 429 `SdkHttpError` with a message that names it as rate limiting, + * says which request was throttled, and gives the retry timing when the server + * sent one. + * + * The header is read off the error's own data, where {@link attachRetryAfter} + * (or the backend relay) put it. + * + * Anything that is not a 429 is returned untouched, so this is safe to apply + * blindly on the rejection path of every request. The result keeps the + * original's code and HTTP data, so a status or code check downstream sees + * what it saw before; the original is kept as `cause`. + */ +export function annotateRateLimit( + err: unknown, + method: string, + now: number = Date.now(), +): unknown { + if (!isRateLimitError(err)) return err; + const header = retryAfterOn(err); + const retryAfterMs = parseRetryAfter(header, now); + const data: AnnotatedRateLimitData = { + ...err.data, + method, + ...(header !== undefined && { retryAfter: header }), + ...(retryAfterMs !== undefined && { retryAfterMs }), + }; + const message = `Rate limited by the server (HTTP 429) on ${method}. ${describeRetry(header, retryAfterMs)} ${err.message}`; + return new SdkHttpError(err.code, message, data, { cause: err }); +} diff --git a/core/mcp/remote/node/remote-session.ts b/core/mcp/remote/node/remote-session.ts index cb0b16f89..e81675d60 100644 --- a/core/mcp/remote/node/remote-session.ts +++ b/core/mcp/remote/node/remote-session.ts @@ -11,6 +11,7 @@ import { AuthChallengeError } from "../../../auth/challenge.js"; import type { RemoteAuthProviderHandle } from "./tokenAuthProvider.js"; import type { RemoteAuthState } from "../types.js"; import { progressTokenOf, waitForProgressToken } from "../progressToken.js"; +import { RateLimitLedger } from "../../rateLimit.js"; export interface SessionEvent { type: RemoteEvent["type"]; @@ -47,6 +48,8 @@ export class RemoteSession { private readonly requestWaits = new Map(); private authProviderHandle: RemoteAuthProviderHandle | null = null; private appliedProtocolVersion: string | undefined; + /** Each upstream `429`'s `Retry-After`, by the JSON-RPC id that drew it (#2442). */ + private readonly rateLimits = new RateLimitLedger(); /** A protocol version is a dated token (`2025-11-25`, `2026-07-28`, …). */ private static readonly PROTOCOL_VERSION_PATTERN = /^[A-Za-z0-9._-]{1,64}$/; @@ -310,6 +313,7 @@ export class RemoteSession { } onFetchRequest(entry: FetchRequestEntryBase): void { + this.rateLimits.record(entry); this.pushEvent({ type: "fetch_request", data: { @@ -322,6 +326,15 @@ export class RemoteSession { }); } + /** + * The `Retry-After` of the upstream `429` that answered request `id` — read + * by the send route, so the header rides back on the send's own error rather + * than on a fetch event racing it (#2442). + */ + takeRetryAfter(id: string | number): string | undefined { + return this.rateLimits.take(id); + } + onFetchResponseBody(id: string, responseBody: string): void { this.pushEvent({ type: "fetch_request_body_update", diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 0c677fb36..576672445 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -30,6 +30,7 @@ import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; import { redactUrlQuery } from "../../fetchTracking.js"; +import { isRateLimitError } from "../../rateLimit.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1115,6 +1116,15 @@ export function createRemoteApp( // browser rebuilds an `SdkHttpError` the era probe can classify (#2660). if (err instanceof SdkHttpError) { const text = err.data.text; + // The 429's own header, by this send's JSON-RPC id (#2442). Read off + // the message rather than `requestId`, which is unset for a request + // the route deliberately does not wait on (`subscriptions/listen`). + const sentId = "id" in message ? message.id : undefined; + const retryAfter = + isRateLimitError(err) && + (typeof sentId === "string" || typeof sentId === "number") + ? session.takeRetryAfter(sentId) + : undefined; return c.json({ ok: false, kind: "transport_error", @@ -1126,6 +1136,7 @@ export function createRemoteApp( statusText: err.statusText, }), ...(typeof text === "string" && { text }), + ...(retryAfter !== undefined && { retryAfter }), }, }); } diff --git a/core/mcp/remote/remoteClientTransport.ts b/core/mcp/remote/remoteClientTransport.ts index 9f9a5462f..6339ebbda 100644 --- a/core/mcp/remote/remoteClientTransport.ts +++ b/core/mcp/remote/remoteClientTransport.ts @@ -182,6 +182,7 @@ function toSdkHttpError( status: http.status, ...(http.statusText !== undefined && { statusText: http.statusText }), ...(http.text !== undefined && { text: http.text }), + ...(http.retryAfter !== undefined && { retryAfter: http.retryAfter }), }); } diff --git a/core/mcp/remote/types.ts b/core/mcp/remote/types.ts index 006d16d04..6caa918d4 100644 --- a/core/mcp/remote/types.ts +++ b/core/mcp/remote/types.ts @@ -118,6 +118,11 @@ export interface RemoteHttpErrorData { statusText?: string; /** The upstream response body, which the probe parses for a JSON-RPC error. */ text?: string; + /** + * The upstream `Retry-After` header on a `429`, which the SDK's error does + * not carry; read off the backend's own tracked fetch (#2442). + */ + retryAfter?: string; } export type RemoteSendResponse =