diff --git a/clients/cli/README.md b/clients/cli/README.md index 2e1edcbe..175c8937 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -119,7 +119,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--log-level ` | Logging level for `logging/setLevel` (e.g. `debug`, `info`). | | `--metadata ` | General `_meta` entries (key=value); applied to all methods. The value is JSON-parsed when it parses, so `trace={"id":"abc"}` sends a real object and `n=3` a number; anything that is not valid JSON is sent as the literal string. Merged **over** the server's persisted `metadata` from `mcp.json`, which is sent on every request without this flag (#2093). | | `--tool-metadata ` | Tool-specific `_meta` entries for `tools/call`. Same JSON-parsed value handling as `--metadata`. | -| `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | +| `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level `connectionTimeout` for `--catalog`/`--config` runs — `30000` when the file sets none. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--strict` | With `--method tools/list`: report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit `6` if any is error-severity. Without it, a one-line count is printed instead. See [Schema portability](#schema-portability---strict). | | `--verify` | With `--method skills/list` or `--method skills/get`: run the SEP-2640 conformance, digest and frontmatter checks over the skills returned, emit one JSON report per skill on stdout, and exit `7` if any fails. See [Skill verification](#skill-verification---verify). | diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 59552a54..28b83ce8 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -133,6 +133,21 @@ describe("classifyError", () => { expect(classifyError(err).exitCode).toBe(EXIT_CODES.UNREACHABLE); }); + it("classifies InspectorClient's connect-time timeout as UNREACHABLE (#2320)", () => { + // The core's Promise.race rejection reads "Connection timed out after …", + // which the older `connect timed out` alternation never matched — so the + // one timeout the Inspector itself owns fell through to the generic exit + // code. Now that it fires by default (30 s), it has to classify with the + // rest of the network-layer failures. + const err = new Error( + "Connection timed out after 30000 ms. To accommodate a slower server, " + + "you may increase the timeout value in Server Settings.", + ); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); + expect(envelope.code).toBe("unreachable"); + }); + it("falls back to USAGE for an unrecognized Error", () => { expect(classifyError(new Error("something else")).exitCode).toBe( EXIT_CODES.USAGE, diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index d2ab086c..6bfb69b0 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -120,7 +120,7 @@ function statusOf(error: unknown): number | undefined { } const UNREACHABLE_PATTERN = - /ENOTFOUND|ECONNREFUSED|ECONNRESET|EAI_AGAIN|ETIMEDOUT|fetch failed|getaddrinfo|connect timed out|aborted/i; + /ENOTFOUND|ECONNREFUSED|ECONNRESET|EAI_AGAIN|ETIMEDOUT|fetch failed|getaddrinfo|connect(?:ion)? timed out|aborted/i; /** * Classify an arbitrary error into an exit code and envelope. Used both by the diff --git a/clients/web/src/test/core/mcp/node/servers.test.ts b/clients/web/src/test/core/mcp/node/servers.test.ts index a54aaa8d..e03242e6 100644 --- a/clients/web/src/test/core/mcp/node/servers.test.ts +++ b/clients/web/src/test/core/mcp/node/servers.test.ts @@ -18,6 +18,7 @@ import { selectServerEntry, type ResolvedServer, } from "@inspector/core/mcp/node/servers.js"; +import { DEFAULT_CONNECTION_TIMEOUT_MS } from "@inspector/core/mcp/types.js"; describe("headersToServerSettings", () => { it("returns undefined when no headers are given", () => { @@ -32,6 +33,10 @@ describe("headersToServerSettings", () => { ]); expect(settings?.metadata).toEqual({}); expect(settings?.roots).toEqual([]); + // The header-only shell is what the CLI/TUI hand the client when a file + // sets no timeout, so it must carry the product default — a regression + // back to 0 here would silently unbound every such connect (#2320). + expect(settings?.connectionTimeout).toBe(DEFAULT_CONNECTION_TIMEOUT_MS); }); }); diff --git a/clients/web/src/test/core/mcp/remote/progressToken.test.ts b/clients/web/src/test/core/mcp/remote/progressToken.test.ts new file mode 100644 index 00000000..dc00cfee --- /dev/null +++ b/clients/web/src/test/core/mcp/remote/progressToken.test.ts @@ -0,0 +1,115 @@ +/** + * Unit tests for the shared `progressTokenOf` guard (#2028) — the string/safe + * integer extractor both relay waits (browser `RemoteClientTransport` and Node + * `RemoteSession`) use to correlate a `notifications/progress` with the request + * whose timeout it should re-arm. Covers the protocol's accept set and, since + * the guard delegates to the SDK's `ProgressTokenSchema`, the fractional and + * past-`MAX_SAFE_INTEGER` values a bare `typeof` check would have let through. + */ +import { describe, it, expect } from "vitest"; +import { progressTokenOf } from "@inspector/core/mcp/remote/progressToken.js"; +import type { JSONRPCMessage } from "@modelcontextprotocol/client"; + +// Cast helper: these fixtures are deliberately off-spec to exercise the guard +// branches, which the typed JSONRPCMessage shape would otherwise forbid. +const msg = (m: unknown): JSONRPCMessage => m as JSONRPCMessage; + +describe("progressTokenOf (#2028)", () => { + it("returns a numeric progressToken from a progress notification", () => { + expect( + progressTokenOf({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: 42, progress: 1, total: 3 }, + }), + ).toBe(42); + }); + + it("returns a string progressToken", () => { + expect( + progressTokenOf({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: "abc", progress: 1 }, + }), + ).toBe("abc"); + }); + + it("returns undefined for a different notification method", () => { + expect( + progressTokenOf({ + jsonrpc: "2.0", + method: "notifications/message", + params: { level: "info", data: {} }, + }), + ).toBeUndefined(); + }); + + it("returns undefined for a JSON-RPC response (no method)", () => { + expect( + progressTokenOf({ jsonrpc: "2.0", id: 1, result: {} }), + ).toBeUndefined(); + }); + + it("returns undefined when a progress note carries no params", () => { + expect( + progressTokenOf( + msg({ jsonrpc: "2.0", method: "notifications/progress" }), + ), + ).toBeUndefined(); + }); + + it("returns undefined when params is null", () => { + expect( + progressTokenOf( + msg({ jsonrpc: "2.0", method: "notifications/progress", params: null }), + ), + ).toBeUndefined(); + }); + + it("returns undefined when the progressToken is neither string nor number", () => { + expect( + progressTokenOf( + msg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: { nested: true } }, + }), + ), + ).toBeUndefined(); + }); + + it("rejects a fractional numeric token (the protocol allows only integers)", () => { + expect( + progressTokenOf( + msg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: 1.5, progress: 1 }, + }), + ), + ).toBeUndefined(); + }); + + it("rejects an integer past MAX_SAFE_INTEGER", () => { + expect( + progressTokenOf( + msg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: Number.MAX_SAFE_INTEGER + 1, progress: 1 }, + }), + ), + ).toBeUndefined(); + }); + + it("accepts a safe integer at the boundary", () => { + expect( + progressTokenOf({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: Number.MAX_SAFE_INTEGER, progress: 1 }, + }), + ).toBe(Number.MAX_SAFE_INTEGER); + }); +}); diff --git a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts index 1aed2bea..d25eba92 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { RemoteClientTransport } from "@inspector/core/mcp/remote/remoteClientTransport.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -686,4 +686,125 @@ describe("RemoteClientTransport", () => { await transport.close(); }); }); + + describe("progress notifications keep the SSE response wait alive (#2028)", () => { + // A remote backend holds POST /api/mcp/send open until the JSON-RPC + // response, and the browser bounds that wait with `sseResponseTimeoutMs`. + // For a long tool that reports progress, that wait must re-arm on each + // `notifications/progress` — mirroring the SDK client's + // `resetTimeoutOnProgress` — or the web client alone times out a call that + // every other transport rides out. Log messages must NOT re-arm it. + function backendHoldingSend(): { + fetchFn: typeof fetch; + getSse: () => ReturnType; + getSentId: () => string | number | undefined; + } { + let sse!: ReturnType; + let sentId: string | number | undefined; + const fetchFn = vi + .fn() + .mockImplementation(async (input, init) => { + const url = String(input); + if (url.endsWith("/api/mcp/connect") && init?.method === "POST") { + return new Response(JSON.stringify({ sessionId: "s-progress" }), { + status: 200, + }); + } + if (url.includes("/api/mcp/events")) { + sse = createPushableSseStream(); + return sse.response; + } + if (url.endsWith("/api/mcp/send") && init?.method === "POST") { + const body = JSON.parse(String(init.body)) as { + message: { id?: string | number }; + }; + sentId = body.message.id; + // Backend accepted the request; the response arrives later over SSE. + return new Response(JSON.stringify({ ok: true }), { status: 200 }); + } + return new Response("not found", { status: 404 }); + }); + return { fetchFn, getSse: () => sse, getSentId: () => sentId }; + } + + // Fake timers so the deadline is driven deterministically rather than + // raced against a real `setTimeout` — a real-sleep test can let the timeout + // fire before a loaded runner processes the enqueued SSE frame (#1596: a + // race is fixed with fake timers, never with headroom). Each push is + // followed by a 0ms async advance to flush the SSE consumer's stream read + // so the frame is parsed before time moves. + const TIMEOUT_MS = 1000; + const flushSse = () => vi.advanceTimersByTimeAsync(0); + + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it("re-arms the wait on each progress note, so a call outliving the timeout still resolves", async () => { + const { fetchFn, getSse, getSentId } = backendHoldingSend(); + const transport = new RemoteClientTransport( + { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS }, + config, + ); + await transport.start(); + + const sent = transport.send({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + }); + await flushSse(); + + // Three progress notes, each after 900ms (< the 1000ms window): without + // the re-arm the wait dies at 1000ms, well before the third lands. + for (let i = 0; i < 3; i++) { + getSse().pushMessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: getSentId(), progress: i, total: 3 }, + }); + await flushSse(); + await vi.advanceTimersByTimeAsync(900); + } + getSse().pushMessage({ + jsonrpc: "2.0", + id: getSentId(), + result: { ok: true }, + }); + await flushSse(); + + await expect(sent).resolves.toBeUndefined(); + await transport.close(); + }); + + it("does not re-arm on notifications/message, so a log-only call still times out", async () => { + const { fetchFn, getSse, getSentId } = backendHoldingSend(); + const transport = new RemoteClientTransport( + { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS }, + config, + ); + await transport.start(); + + const sent = transport.send({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + }); + // Hold the expected rejection now (as the RemoteSession timeout tests do) + // so it is asserted rather than suppressed by an empty catch. + const rejection = expect(sent).rejects.toThrow(/Request timed out/); + await flushSse(); + + // A log message before the deadline must not extend it. + getSse().pushMessage({ + jsonrpc: "2.0", + method: "notifications/message", + params: { level: "info", data: { msg: "tick", token: getSentId() } }, + }); + await flushSse(); + await vi.advanceTimersByTimeAsync(TIMEOUT_MS); + + await rejection; + await transport.close(); + }); + }); }); diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 85553e18..f695b69d 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -21,6 +21,7 @@ import { serializeMcpConfig, storedFieldsToInspectorSettings, } from "@inspector/core/mcp/serverList.js"; +import { DEFAULT_CONNECTION_TIMEOUT_MS } from "@inspector/core/mcp/types.js"; import type { InspectorServerSettings, MCPServerConfig, @@ -258,7 +259,10 @@ describe("serverEntriesToMcpConfig", () => { url: "https://x.test/mcp", headers: { Authorization: "Bearer xyz" }, metadata: { tenant: "acme", limits: { rps: 10 } }, - connectionTimeout: 30000, + // Non-default on purpose: 30000 is DEFAULT_CONNECTION_TIMEOUT_MS, + // which the write side omits (as it does taskTtl's 60000), so it + // would not survive a byte-equal round-trip (#2320). + connectionTimeout: 45000, requestTimeout: 60000, oauth: { clientId: "client-abc", @@ -590,7 +594,8 @@ describe("serverEntriesToMcpConfig", () => { // Non-stdio server → empty env mirror in memory (for the form) env: [], metadata: {}, - connectionTimeout: 0, + // Absent connectionTimeout on disk → product default in memory (#2320) + connectionTimeout: 30000, requestTimeout: 0, // Absent taskTtl on disk → product default in memory (for the form) taskTtl: 60000, @@ -685,11 +690,45 @@ describe("serverEntriesToMcpConfig", () => { expect(stored?.headers).toEqual({ "X-Tenant": "acme" }); }); - it("omits zero-valued timeouts and empty oauth fields on serialize", () => { - // The form keeps numeric defaults at 0 and empty-string OAuth values. - // Round-tripping them onto disk would leave noisy `connectionTimeout: 0` - // / `oauth: {}` keys; suppress them so the diff stays minimal for - // entries the user never customized. + it("omits default-valued timeouts and empty oauth fields on serialize", () => { + // The form keeps the timeouts at their defaults (the 30 s product default + // for connectionTimeout, 0 = "SDK default" for requestTimeout) and + // empty-string OAuth values. Round-tripping them onto disk would leave + // noisy `connectionTimeout: 30000` / `oauth: {}` keys; suppress them so + // the diff stays minimal for entries the user never customized. + const entries: ServerEntry[] = [ + { + id: "alpha", + name: "alpha", + config: { type: "streamable-http", url: "https://x.test" }, + settings: { + headers: [], + env: [], + metadata: {}, + connectionTimeout: DEFAULT_CONNECTION_TIMEOUT_MS, + requestTimeout: 0, + taskTtl: 0, + maxFetchRequests: 1000, + roots: [], + }, + connection: { status: "disconnected" }, + }, + ]; + const stored = serverEntriesToMcpConfig(entries).mcpServers.alpha; + expect(stored).not.toHaveProperty("connectionTimeout"); + expect(stored).not.toHaveProperty("requestTimeout"); + expect(stored).not.toHaveProperty("taskTtl"); + expect(stored).not.toHaveProperty("oauth"); + expect(stored).not.toHaveProperty("headers"); + expect(stored).not.toHaveProperty("metadata"); + expect(stored).not.toHaveProperty("roots"); + }); + + it("persists an explicit connectionTimeout of 0 so the opt-out round-trips (#2320)", () => { + // 0 used to be the default and was suppressed on disk. Now that an absent + // field reads back as the 30 s product default, a suppressed 0 would + // silently turn "no timeout" into 30 s on the next load — so 0 is a real + // value here, written and read back as itself. const entries: ServerEntry[] = [ { id: "alpha", @@ -709,13 +748,11 @@ describe("serverEntriesToMcpConfig", () => { }, ]; const stored = serverEntriesToMcpConfig(entries).mcpServers.alpha; - expect(stored).not.toHaveProperty("connectionTimeout"); - expect(stored).not.toHaveProperty("requestTimeout"); - expect(stored).not.toHaveProperty("taskTtl"); - expect(stored).not.toHaveProperty("oauth"); - expect(stored).not.toHaveProperty("headers"); - expect(stored).not.toHaveProperty("metadata"); - expect(stored).not.toHaveProperty("roots"); + expect(stored?.connectionTimeout).toBe(0); + const [entry] = mcpConfigToServerEntries({ + mcpServers: { alpha: stored! }, + }); + expect(entry?.settings?.connectionTimeout).toBe(0); }); it("round-trips roots (uri + optional name) onto the top-level disk field", () => { diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 248f60cf..764480f5 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -516,7 +516,9 @@ describe("useServers", () => { headers: [{ key: "X-Keep", value: "yes" }], env: [], metadata: {}, - connectionTimeout: 0, + // Absent connectionTimeout on disk reads back as the 30 s product + // default for the form (#2320). + connectionTimeout: 30000, requestTimeout: 0, // Absent taskTtl on disk reads back as the product default for the form. taskTtl: 60000, diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 53c6642d..0d89558d 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -1,7 +1,12 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { resolve } from "node:path"; +import type { Transport } from "@modelcontextprotocol/client"; import * as z from "zod/v4"; -import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { + InspectorClient, + connectionTimeoutMessage, +} from "@inspector/core/mcp/inspectorClient.js"; +import { DEFAULT_CONNECTION_TIMEOUT_MS } from "@inspector/core/mcp/types.js"; import { MessageLogState, FetchRequestLogState, @@ -363,25 +368,40 @@ describe("InspectorClient", () => { messageLogState.destroy(); }); + /** + * A transport whose `start()` never resolves: the "server that accepts + * the TCP connection and never answers" from the #2320 measurement. + * Typed as the SDK's `Transport` rather than cast to it, so an interface + * change upstream is caught here instead of at runtime. + */ + function hangingTransport(): Transport { + return { + start: () => new Promise(() => {}), + send: async () => {}, + close: async () => {}, + }; + } + + /** A transport whose `start()` rejects with a recoverable 401. */ + function unauthorizedTransport(): Transport { + return { + start: async () => { + const err = new Error("Unauthorized") as Error & { status?: number }; + err.status = 401; + throw err; + }, + send: async () => {}, + close: async () => {}, + }; + } + it("rejects connect() with a timeout error when serverSettings.connectionTimeout fires", async () => { // Stub transport whose start() never resolves — simulates a slow / // unreachable upstream. InspectorClient.connect() should race against // serverSettings.connectionTimeout and reject with a descriptive error; // status should end up in "error", and the client should have // internally torn down the transport (next connect() must rebuild). - const hangingTransport = { - start: () => new Promise(() => {}), - send: async () => {}, - close: async () => {}, - onclose: undefined, - onerror: undefined, - onmessage: undefined, - sessionId: undefined, - }; - const fakeFactory = () => ({ - transport: - hangingTransport as unknown as import("@modelcontextprotocol/client").Transport, - }); + const fakeFactory = () => ({ transport: hangingTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:1/never" }, { @@ -412,24 +432,106 @@ describe("InspectorClient", () => { expect(client.getStatus()).toBe("error"); }); - it("holds status at connecting when connect fails with a recoverable 401", async () => { - const unauthorizedTransport = { - start: async () => { - const err = new Error("Unauthorized") as Error & { status?: number }; - err.status = 401; - throw err; + it("names the bound that fired and where to raise it in the timeout message (#2320)", async () => { + const fakeFactory = () => ({ transport: hangingTransport() }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:1/never" }, + { + environment: { transport: fakeFactory }, + serverSettings: { + headers: [], + env: [], + metadata: {}, + connectionTimeout: 50, + requestTimeout: 0, + taskTtl: 0, + maxFetchRequests: 1000, + roots: [], + }, }, - send: async () => {}, - close: async () => {}, - onclose: undefined, - onerror: undefined, - onmessage: undefined, - sessionId: undefined, - }; - const fakeFactory = () => ({ - transport: - unauthorizedTransport as unknown as import("@modelcontextprotocol/client").Transport, - }); + ); + await expect(client.connect()).rejects.toThrow( + connectionTimeoutMessage(50), + ); + // The sentence a user reads on the toast: the bound, then the remedy. + expect(connectionTimeoutMessage(50)).toBe( + "Connection timed out after 50 ms. To accommodate a slower server, " + + "you may increase the timeout value in Server Settings.", + ); + }); + + it("bounds connect() with DEFAULT_CONNECTION_TIMEOUT_MS when the settings carry no timeout (#2320)", async () => { + // No serverSettings at all — the shape every consumer that never opened + // Server Settings hands the client. Before #2320 that meant "no bound", + // and the only thing ending a hung handshake was the SDK's 60 s + // per-request timeout on `initialize`, reporting `Request timed out`. + // Fake timers so the 30 s default can fire without waiting it out. + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + try { + const fakeFactory = () => ({ transport: hangingTransport() }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:1/never" }, + { environment: { transport: fakeFactory } }, + ); + const pending = client.connect(); + // Hold the rejection so it cannot surface as unhandled while the clock + // is advanced; the assertion below re-awaits the same promise. + pending.catch(() => {}); + // One ms short of the default: still connecting, nothing has fired. + await vi.advanceTimersByTimeAsync(DEFAULT_CONNECTION_TIMEOUT_MS - 1); + expect(client.getStatus()).toBe("connecting"); + await vi.advanceTimersByTimeAsync(1); + await expect(pending).rejects.toThrow( + connectionTimeoutMessage(DEFAULT_CONNECTION_TIMEOUT_MS), + ); + expect(client.getStatus()).toBe("error"); + } finally { + vi.useRealTimers(); + } + }); + + it("leaves connect() unbounded when connectionTimeout is an explicit 0 (#2320)", async () => { + // 0 is the documented opt-out (`--connect-timeout 0`, a cleared field), + // so it must not be read as "absent" and replaced with the default. + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + try { + let settled = false; + const fakeFactory = () => ({ transport: hangingTransport() }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:1/never" }, + { + environment: { transport: fakeFactory }, + serverSettings: { + headers: [], + env: [], + metadata: {}, + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 0, + maxFetchRequests: 1000, + roots: [], + }, + }, + ); + const pending = client.connect().finally(() => { + settled = true; + }); + pending.catch(() => {}); + await vi.advanceTimersByTimeAsync(DEFAULT_CONNECTION_TIMEOUT_MS * 2); + expect(settled).toBe(false); + expect(client.getStatus()).toBe("connecting"); + // Tear down by hand so the hung attempt does not outlive the test; + // disconnect() rejects the pending connect, which `pending.catch` + // above already absorbs. + await client.disconnect(); + client = null; + } finally { + vi.useRealTimers(); + } + }); + + it("holds status at connecting when connect fails with a recoverable 401", async () => { + const fakeFactory = () => ({ transport: unauthorizedTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:8081/mcp" }, { environment: { transport: fakeFactory } }, @@ -438,6 +540,35 @@ describe("InspectorClient", () => { await expect(client.connect()).rejects.toMatchObject({ status: 401 }); expect(client.getStatus()).toBe("connecting"); }); + + it("holds status at connecting on a recoverable 401 even with a connect timeout armed (#2320)", async () => { + // The timeout teardown used to be gated on `connectionTimeout > 0`, so + // it ran for *every* failed connect once a timeout was set — including + // this 401, whose status must stay at "connecting" for the auth + // recovery. With a non-zero default that gate would have fired for + // every user, so the teardown is now keyed to the timer actually + // winning the race, and this is the case that proves it. + const fakeFactory = () => ({ transport: unauthorizedTransport() }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:8081/mcp" }, + { + environment: { transport: fakeFactory }, + serverSettings: { + headers: [], + env: [], + metadata: {}, + connectionTimeout: 5000, + requestTimeout: 0, + taskTtl: 0, + maxFetchRequests: 1000, + roots: [], + }, + }, + ); + + await expect(client.connect()).rejects.toMatchObject({ status: 401 }); + expect(client.getStatus()).toBe("connecting"); + }); }); describe("Message Tracking", () => { diff --git a/clients/web/src/test/integration/mcp/remote/remote-session.test.ts b/clients/web/src/test/integration/mcp/remote/remote-session.test.ts index d11aee37..61d55a1d 100644 --- a/clients/web/src/test/integration/mcp/remote/remote-session.test.ts +++ b/clients/web/src/test/integration/mcp/remote/remote-session.test.ts @@ -7,11 +7,16 @@ */ import { describe, it, expect, vi } from "vitest"; -import type { Transport } from "@modelcontextprotocol/client"; +import type { Transport, JSONRPCMessage } from "@modelcontextprotocol/client"; import { RemoteSession } from "@inspector/core/mcp/remote/node/remote-session.js"; import type { FetchRequestEntryBase } from "@inspector/core/mcp/types.js"; import { AuthChallengeError } from "@inspector/core/auth/challenge.js"; +// Stage a deliberately off-spec message as `unknown` and narrow with a single +// cast — the guard branches these fixtures exercise are ones the typed +// `JSONRPCMessage` shape forbids, and `unknown` is the safe top type to pass in. +const asMsg = (m: unknown): JSONRPCMessage => m as JSONRPCMessage; + function makeFetchEntry( overrides: Partial = {}, ): FetchRequestEntryBase { @@ -255,6 +260,76 @@ describe("RemoteSession", () => { await expect(wait).resolves.toBeUndefined(); }); + it("a matching progress notification re-arms the wait past its original deadline (#2028)", async () => { + vi.useFakeTimers(); + const session = new RemoteSession("s-progress-reset"); + const wait = session.waitForRequestResponse(5, 1000); + // Almost at the deadline, then a progress note for this request arrives. + await vi.advanceTimersByTimeAsync(900); + session.onMessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: 5, progress: 1, total: 10 }, + }); + // Past the *original* 1000ms deadline: without the re-arm this would have + // already rejected, and the resolve below would be a no-op on a dead wait. + await vi.advanceTimersByTimeAsync(900); + session.onMessage({ jsonrpc: "2.0", id: 5, result: {} }); + await expect(wait).resolves.toBeUndefined(); + vi.useRealTimers(); + }); + + it("a notifications/message does NOT re-arm the wait (log messages don't extend the deadline, #2028)", async () => { + vi.useFakeTimers(); + const session = new RemoteSession("s-message-no-reset"); + const wait = session.waitForRequestResponse(6, 1000); + const rejection = expect(wait).rejects.toThrow(/timed out/); + await vi.advanceTimersByTimeAsync(500); + session.onMessage({ + jsonrpc: "2.0", + method: "notifications/message", + params: { level: "info", data: { msg: "tick" } }, + }); + await vi.advanceTimersByTimeAsync(500); + await rejection; + vi.useRealTimers(); + }); + + it("noteRequestProgress is a no-op for an id with no pending wait", () => { + const session = new RemoteSession("s-progress-noop"); + expect(() => session.noteRequestProgress(999)).not.toThrow(); + }); + + it("a progress notification on a timeoutMs=0 wait leaves it timerless and it still resolves", async () => { + const session = new RemoteSession("s-progress-notimer"); + const wait = session.waitForRequestResponse(7, 0); + session.onMessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: 7, progress: 1 }, + }); + session.onMessage({ jsonrpc: "2.0", id: 7, result: {} }); + await expect(wait).resolves.toBeUndefined(); + }); + + it("a progress notification with a non-token progressToken re-arms nothing", async () => { + vi.useFakeTimers(); + const session = new RemoteSession("s-progress-bad-token"); + const wait = session.waitForRequestResponse(8, 1000); + const rejection = expect(wait).rejects.toThrow(/timed out/); + // A malformed token (object) must be ignored, not coerced to a key. + session.onMessage( + asMsg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: { bad: true }, progress: 1 }, + }), + ); + await vi.advanceTimersByTimeAsync(1000); + await rejection; + vi.useRealTimers(); + }); + it("endSend is a no-op when no send is active", () => { const session = new RemoteSession("s-endsend-noop"); expect(session.hasActiveSend()).toBe(false); diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index d0f9d407..a4fc1aa9 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -430,7 +430,9 @@ describe("/api/servers routes", () => { settings: { headers: [{ key: "Authorization", value: "Bearer xyz" }], metadata: { tenant: "acme", limits: { rps: 10 } }, - connectionTimeout: 30000, + // Non-default on purpose: 30000 is DEFAULT_CONNECTION_TIMEOUT_MS, + // which the write side omits from disk (#2320). + connectionTimeout: 45000, requestTimeout: 60000, oauthClientId: "client-abc", oauthScopes: "read:tools", @@ -444,7 +446,7 @@ describe("/api/servers routes", () => { expect(stored).not.toHaveProperty("settings"); expect(stored.headers).toEqual({ Authorization: "Bearer xyz" }); expect(stored.metadata).toEqual({ tenant: "acme", limits: { rps: 10 } }); - expect(stored.connectionTimeout).toBe(30000); + expect(stored.connectionTimeout).toBe(45000); expect(stored.requestTimeout).toBe(60000); expect(stored.oauth).toEqual({ clientId: "client-abc", @@ -596,9 +598,12 @@ describe("/api/servers routes", () => { expect(stored).not.toHaveProperty("settings"); expect(stored.headers).toEqual({ "X-Tenant": "acme" }); expect(stored.requestTimeout).toBe(45000); - // Zero/empty values are suppressed on disk to keep the diff minimal. + // Empty values are suppressed on disk to keep the diff minimal. expect(stored).not.toHaveProperty("metadata"); - expect(stored).not.toHaveProperty("connectionTimeout"); + // A connectionTimeout of 0 is the explicit "no timeout" opt-out, not the + // default (30 s) — so it must reach disk, or the next load would + // silently read it back as 30 s (#2320). + expect(stored.connectionTimeout).toBe(0); }); it("persists autoRefreshOnListChanged: true through the PUT write path", async () => { diff --git a/clients/web/src/utils/serverSettingsDefaults.test.ts b/clients/web/src/utils/serverSettingsDefaults.test.ts index 240b4608..ee653e7e 100644 --- a/clients/web/src/utils/serverSettingsDefaults.test.ts +++ b/clients/web/src/utils/serverSettingsDefaults.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + DEFAULT_CONNECTION_TIMEOUT_MS, DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS, } from "@inspector/core/mcp/types.js"; @@ -11,7 +12,7 @@ describe("EMPTY_SETTINGS", () => { headers: [], env: [], metadata: {}, - connectionTimeout: 0, + connectionTimeout: DEFAULT_CONNECTION_TIMEOUT_MS, requestTimeout: 0, taskTtl: DEFAULT_TASK_TTL_MS, autoRefreshOnListChanged: false, diff --git a/clients/web/src/utils/serverSettingsDefaults.ts b/clients/web/src/utils/serverSettingsDefaults.ts index 522b3d1e..72dbbf12 100644 --- a/clients/web/src/utils/serverSettingsDefaults.ts +++ b/clients/web/src/utils/serverSettingsDefaults.ts @@ -1,5 +1,6 @@ import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { + DEFAULT_CONNECTION_TIMEOUT_MS, DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS, } from "@inspector/core/mcp/types.js"; @@ -14,7 +15,7 @@ export const EMPTY_SETTINGS: InspectorServerSettings = { headers: [], env: [], metadata: {}, - connectionTimeout: 0, + connectionTimeout: DEFAULT_CONNECTION_TIMEOUT_MS, requestTimeout: 0, taskTtl: DEFAULT_TASK_TTL_MS, autoRefreshOnListChanged: false, diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 72bafec9..43b05f68 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -68,6 +68,7 @@ import type { CreateTransportOptions, ServerType, } from "./types.js"; +import { DEFAULT_CONNECTION_TIMEOUT_MS } from "./types.js"; import { MessageTrackingTransport, type MessageTrackingCallbacks, @@ -315,6 +316,20 @@ interface ReceiverTaskRecord { */ const MAX_URL_ELICITATION_RETRIES = 5; +/** + * The message `connect()` rejects with when the connect-time timeout fires. + * Names the bound that fired and, since the default is now a real value that + * a slow server on a slow link can legitimately hit, says where to raise it + * (#2320). All three clients surface this text as-is: the web client's + * "Failed to connect" toast, the TUI status line, and the CLI error envelope. + */ +export function connectionTimeoutMessage(timeoutMs: number): string { + return ( + `Connection timed out after ${timeoutMs} ms. To accommodate a slower ` + + `server, you may increase the timeout value in Server Settings.` + ); +} + /** * Error used to reject a pending sampling/elicitation request when the tool * call driving its MRTR round is aborted (e.g. the user hits Cancel). Its @@ -2321,11 +2336,28 @@ export class InspectorClient extends InspectorClientEventTarget { this.registerPeerRequestHandlers(); this.registerPeerNotificationHandlers(); - // Optional connect-time timeout from per-server settings. The MCP SDK - // has no connect-time timeout option, so we wrap the handshake in a + // Connect-time timeout from per-server settings, defaulting to + // `DEFAULT_CONNECTION_TIMEOUT_MS` when the settings carry none. The MCP + // SDK has no connect-time timeout option, so we wrap the handshake in a // Promise.race. On timeout, tear the transport down so the next // connect() starts clean and the upstream socket isn't left hanging. - const connectTimeoutMs = this.serverSettings?.connectionTimeout ?? 0; + // + // The default is ours on purpose (#2320): without it the only bound on + // a connect attempt was the SDK's per-request timeout on `initialize`, + // which covers nothing outside that request (`transport.start()` for + // one), fires after 60 s, and reports `Request timed out` — a sentence + // about a JSON-RPC request, not a connection. An explicit `0` is still + // "no timeout": that is what the CLI's `--connect-timeout 0` documents + // and what a user who cleared the field asked for. + const connectTimeoutMs = + this.serverSettings?.connectionTimeout ?? DEFAULT_CONNECTION_TIMEOUT_MS; + // Set only by the timer below, so the teardown in the catch runs for a + // timeout and nothing else. Gating it on `connectTimeoutMs > 0` instead + // would tear down *every* failed connect once the default is non-zero — + // including a recoverable 401, whose transport the auth recovery needs + // held open and whose status must stay at "connecting" (see the outer + // catch and `transportHasAuthProvider`). + let connectTimedOut = false; // Unwrap here — the earliest point — so an auth error the SDK's // era-negotiation probe buried in its cause chain is surfaced before // anything downstream inspects it: `withDirectAuthRecovery` (whose @@ -2369,15 +2401,10 @@ export class InspectorClient extends InspectorClientEventTarget { connectPromise.catch(() => {}); let timer: ReturnType | undefined; const timeoutPromise = new Promise((_, reject) => { - timer = setTimeout( - () => - reject( - new Error( - `Connection timed out after ${connectTimeoutMs} ms`, - ), - ), - connectTimeoutMs, - ); + timer = setTimeout(() => { + connectTimedOut = true; + reject(new Error(connectionTimeoutMessage(connectTimeoutMs))); + }, connectTimeoutMs); }); try { await Promise.race([connectPromise, timeoutPromise]); @@ -2392,7 +2419,7 @@ export class InspectorClient extends InspectorClientEventTarget { try { await this.invokeMcpClient(runConnect); } catch (err) { - if (connectTimeoutMs > 0) { + if (connectTimedOut) { await this.disconnect().catch(() => {}); } throw err; diff --git a/core/mcp/node/servers.ts b/core/mcp/node/servers.ts index 2f296c12..85f4bdfb 100644 --- a/core/mcp/node/servers.ts +++ b/core/mcp/node/servers.ts @@ -1,7 +1,11 @@ import type { SecretStore } from "../../auth/node/secret-store.js"; import { defaultSecretStore } from "../../auth/node/secret-store-selection.js"; import type { InspectorServerSettings, MCPServerConfig } from "../types.js"; -import { DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS } from "../types.js"; +import { + DEFAULT_CONNECTION_TIMEOUT_MS, + DEFAULT_MAX_FETCH_REQUESTS, + DEFAULT_TASK_TTL_MS, +} from "../types.js"; import { mcpConfigToServerEntries } from "../serverList.js"; import { applyOverrides, @@ -50,7 +54,7 @@ export function headersToServerSettings( headers: Object.entries(headers).map(([key, value]) => ({ key, value })), env: [], metadata: {}, - connectionTimeout: 0, + connectionTimeout: DEFAULT_CONNECTION_TIMEOUT_MS, requestTimeout: 0, taskTtl: DEFAULT_TASK_TTL_MS, maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, diff --git a/core/mcp/remote/node/remote-session.ts b/core/mcp/remote/node/remote-session.ts index 2dba31f6..5e5be4e1 100644 --- a/core/mcp/remote/node/remote-session.ts +++ b/core/mcp/remote/node/remote-session.ts @@ -10,6 +10,7 @@ import type { AuthChallenge } from "../../../auth/challenge.js"; import { AuthChallengeError } from "../../../auth/challenge.js"; import type { RemoteAuthProviderHandle } from "./tokenAuthProvider.js"; import type { RemoteAuthState } from "../types.js"; +import { progressTokenOf } from "../progressToken.js"; export interface SessionEvent { type: RemoteEvent["type"]; @@ -19,6 +20,14 @@ export interface SessionEvent { type RequestWait = { resolve: () => void; reject: (error: Error) => void; + /** + * Re-arm this wait's timeout for another full window. Called when a + * `notifications/progress` for this request arrives, mirroring the SDK + * client's `resetTimeoutOnProgress` so the relay's own deadline never fires + * before the client's progress-aware one (#2028). A no-op when the wait was + * created without a timeout. + */ + resetTimeout: () => void; }; export class RemoteSession { @@ -172,35 +181,57 @@ export class RemoteSession { timeoutMs = 60_000, ): Promise { return new Promise((resolve, reject) => { - const timer = - timeoutMs > 0 - ? setTimeout(() => { - this.requestWaits.delete(requestId); - reject( - new Error( - `MCP request ${String(requestId)} timed out after ${timeoutMs}ms`, - ), - ); - }, timeoutMs) - : undefined; + let timer: ReturnType | undefined; + const arm = () => { + if (timeoutMs <= 0) return; + timer = setTimeout(() => { + this.requestWaits.delete(requestId); + reject( + new Error( + `MCP request ${String(requestId)} timed out after ${timeoutMs}ms`, + ), + ); + }, timeoutMs); + }; + const clear = () => { + if (timer !== undefined) { + clearTimeout(timer); + timer = undefined; + } + }; + arm(); this.requestWaits.set(requestId, { resolve: () => { - if (timer !== undefined) { - clearTimeout(timer); - } + clear(); resolve(); }, reject: (error) => { - if (timer !== undefined) { - clearTimeout(timer); - } + clear(); reject(error); }, + // A progress notification for this request re-arms the full window, so + // a tool that keeps reporting progress never trips the relay's own + // deadline — the same guarantee the SDK client's `resetTimeoutOnProgress` + // gives on a direct transport (#2028). + resetTimeout: () => { + if (timer === undefined) return; + clear(); + arm(); + }, }); }); } + /** + * Re-arm a pending request's wait timeout. Invoked from {@link onMessage} when + * a `notifications/progress` carrying this request's `progressToken` arrives. + * A no-op when no wait is pending for the id. + */ + noteRequestProgress(requestId: string | number): void { + this.requestWaits.get(requestId)?.resetTimeout(); + } + cancelRequestWait(requestId: string | number): void { const wait = this.requestWaits.get(requestId); if (!wait) { @@ -265,6 +296,15 @@ export class RemoteSession { ) { this.settleRequestWait(message.id); } + // A `notifications/progress` carries the originating request's id in + // `params.progressToken` (the SDK stamps `progressToken: messageId`). Re-arm + // that request's wait so the relay's deadline tracks the client's + // progress-aware one instead of firing at a flat 60s (#2028). Log messages + // (`notifications/message`) deliberately do NOT reset it, matching the SDK. + const progressToken = progressTokenOf(message); + if (progressToken !== undefined) { + this.noteRequestProgress(progressToken); + } this.pushEvent({ type: "message", data: message }); } diff --git a/core/mcp/remote/progressToken.ts b/core/mcp/remote/progressToken.ts new file mode 100644 index 00000000..2ce37b1a --- /dev/null +++ b/core/mcp/remote/progressToken.ts @@ -0,0 +1,40 @@ +/** + * Shared helper for the remote relay's progress-aware request-response waits + * (#2028). Pure and browser-safe, so both the browser `RemoteClientTransport` + * and the Node `RemoteSession` import it rather than each carrying a copy. + */ + +import { ProgressTokenSchema } from "@modelcontextprotocol/core"; +import type { JSONRPCMessage } from "@modelcontextprotocol/client"; + +/** + * The token identifying the request a `notifications/progress` belongs to, or + * `undefined` for any other message. The SDK stamps `progressToken: messageId` + * on the outbound request, so a progress notification's `progressToken` is the + * id of the request whose wait should be re-armed. + * + * Validation is delegated to the SDK's own `ProgressTokenSchema` rather than a + * hand-rolled `typeof` check — the protocol allows only a string or a *safe* + * integer, and zod's `.int()` rejects a fractional or past-`MAX_SAFE_INTEGER` + * value that a bare `typeof token === "number"` would wave through (the same + * drift `isProgressToken` in `core/mcp/inspectorClient.ts` exists to avoid). + */ +export function progressTokenOf( + message: JSONRPCMessage, +): string | number | undefined { + if ( + "method" in message && + message.method === "notifications/progress" && + "params" in message && + typeof message.params === "object" && + message.params !== null + ) { + const parsed = ProgressTokenSchema.safeParse( + (message.params as { progressToken?: unknown }).progressToken, + ); + if (parsed.success) { + return parsed.data; + } + } + return undefined; +} diff --git a/core/mcp/remote/remoteClientTransport.ts b/core/mcp/remote/remoteClientTransport.ts index a5c918b9..8200b32a 100644 --- a/core/mcp/remote/remoteClientTransport.ts +++ b/core/mcp/remote/remoteClientTransport.ts @@ -32,6 +32,7 @@ import type { RemoteSendResponse, } from "./types.js"; import { oauthTokensToRemoteAuthState } from "./types.js"; +import { progressTokenOf } from "./progressToken.js"; export interface AuthRecoveryHandlers { handleAuthChallenge( @@ -89,6 +90,12 @@ const DEFAULT_SSE_RESPONSE_TIMEOUT_MS = 60_000; type SseResponseWait = { resolve: () => void; reject: (error: Error) => void; + /** + * Re-arm this wait's timeout for another full window when a + * `notifications/progress` for the request arrives, mirroring the SDK + * client's `resetTimeoutOnProgress` (#2028). + */ + resetTimeout: () => void; }; function requestIdForMessage( @@ -528,6 +535,13 @@ export class RemoteClientTransport implements Transport { if (parsed.type === "message") { const msg = parsed.data as JSONRPCMessage; this.settleSseResponseWait(msg); + // Keep the SSE wait alive while progress flows for a long call + // (#2028) — the settle above never matches a notification, so this + // is the only place a progress note touches the wait. + const progressToken = progressTokenOf(msg); + if (progressToken !== undefined) { + this.resetSseResponseWait(progressToken); + } this.onmessage?.(msg, undefined); } else if ( parsed.type === "fetch_request" && @@ -688,21 +702,25 @@ export class RemoteClientTransport implements Transport { private waitForSseResponse(requestId: string | number): Promise { return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - this.sseResponseWaits.delete(requestId); - // The SDK's own per-request timeout shape, not a plain `Error`: this - // wait can expire before the SDK's timer does (its budget is the - // relay's, not the request's), and the rejection travels up through - // `Client.request`, where `InspectorClient` annotates a timeout of - // exactly this shape with the connection's state (#2318). A plain - // error would pass that decorator untouched and reach the user as a - // bare timeout on the web client alone. - reject( - new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", { - timeout: this.sseResponseTimeoutMs, - }), - ); - }, this.sseResponseTimeoutMs); + let timer: ReturnType; + const arm = () => { + timer = setTimeout(() => { + this.sseResponseWaits.delete(requestId); + // The SDK's own per-request timeout shape, not a plain `Error`: + // this wait can expire before the SDK's timer does (its budget is + // the relay's, not the request's), and the rejection travels up + // through `Client.request`, where `InspectorClient` annotates a + // timeout of exactly this shape with the connection's state + // (#2318). A plain error would pass that decorator untouched and + // reach the user as a bare timeout on the web client alone. + reject( + new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", { + timeout: this.sseResponseTimeoutMs, + }), + ); + }, this.sseResponseTimeoutMs); + }; + arm(); this.sseResponseWaits.set(requestId, { resolve: () => { clearTimeout(timer); @@ -712,10 +730,22 @@ export class RemoteClientTransport implements Transport { clearTimeout(timer); reject(error); }, + // A progress notification for this request re-arms the full window so + // the browser-side deadline tracks the SDK client's progress-aware one + // rather than firing at a flat 60s (#2028). + resetTimeout: () => { + clearTimeout(timer); + arm(); + }, }); }); } + /** Re-arm a pending request's SSE wait timeout on a matching progress note. */ + private resetSseResponseWait(requestId: string | number): void { + this.sseResponseWaits.get(requestId)?.resetTimeout(); + } + private async postSend( message: JSONRPCMessage, options: TransportSendOptions | undefined, diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index 220253d5..04728a58 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -6,6 +6,7 @@ */ import { + DEFAULT_CONNECTION_TIMEOUT_MS, DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_MODERN_LOG_LEVEL, DEFAULT_PROTOCOL_ERA, @@ -490,8 +491,11 @@ export function oauthEndpointOverridesFromSettings( * * `headers` becomes a pair-array preserving the object's key insertion order; * `oauth.*` becomes the flat `oauthClientId` / `oauthClientSecret` / - * `oauthScopes` fields. Numeric timeouts default to 0 when absent — the form - * needs concrete values to render and 0 is the SDK's "no timeout" signal. + * `oauthScopes` fields. The numeric timeouts get concrete values when absent + * because the form needs them to render: `connectionTimeout` reads back as + * `DEFAULT_CONNECTION_TIMEOUT_MS` (30 s), while `requestTimeout` reads back as + * 0, the SDK's "use your own default" signal. An explicit `connectionTimeout` + * of 0 is preserved — it is the user's "no timeout" opt-out (#2320). * * `env` / `cwd` are SDK config fields (not Inspector-extension keys), but they * are mirrored into the settings here so the Server Settings modal can edit @@ -533,9 +537,13 @@ export function storedFieldsToInspectorSettings( headers: headersPairs, env: envRecordToPairs(stored.env), metadata: normalizeStoredMetadata(stored.metadata), - connectionTimeout: stored.connectionTimeout ?? 0, + // Concrete product default, like taskTtl below: an absent field reads back + // as 30 s so the form shows the bound that will actually apply. An explicit + // 0 is preserved as-is — it is the user's opt-out, not a sentinel (#2320). + connectionTimeout: + stored.connectionTimeout ?? DEFAULT_CONNECTION_TIMEOUT_MS, requestTimeout: stored.requestTimeout ?? 0, - // Unlike the timeouts (0 = "SDK default"), task TTL has a concrete product + // Unlike requestTimeout (0 = "SDK default"), task TTL has a concrete product // default so the form shows it and "Run as task" has a value to send. taskTtl: stored.taskTtl ?? DEFAULT_TASK_TTL_MS, autoRefreshOnListChanged: stored.autoRefreshOnListChanged ?? false, @@ -632,8 +640,10 @@ export function storedFieldsToInspectorSettings( * Splat the form-shape `InspectorServerSettings` back into the on-disk * Inspector-extension fields (object-form `headers`, nested `oauth`, etc.). * Empty-key rows are dropped — the form lets users leave new rows blank - * mid-edit and those shouldn't reach disk. Numeric timeouts at 0 are omitted - * so the file diff stays minimal for entries that never touched them. + * mid-edit and those shouldn't reach disk. Timeouts at their default are + * omitted so the file diff stays minimal for entries that never touched them: + * that is `DEFAULT_CONNECTION_TIMEOUT_MS` for `connectionTimeout` (an explicit + * 0 is a chosen opt-out and is written) and 0 for `requestTimeout`. * * Returns the field deltas to merge onto a `StoredMCPServer`; callers can * spread the result. @@ -659,7 +669,11 @@ export function inspectorSettingsToStoredFields( out.metadata = settings.metadata; } - if (settings.connectionTimeout > 0) { + // The product default is the omit-sentinel (as for taskTtl below), so a file + // that never named a timeout stays byte-stable. Anything else persists — + // including 0, which is now a non-default the user chose (no timeout) and + // would otherwise silently read back as 30 s on the next load (#2320). + if (settings.connectionTimeout !== DEFAULT_CONNECTION_TIMEOUT_MS) { out.connectionTimeout = settings.connectionTimeout; } if (settings.requestTimeout > 0) { @@ -714,10 +728,11 @@ export function inspectorSettingsToStoredFields( out.modernLogLevel = settings.modernLogLevel; } - // Persist only when it differs from the default. Unlike the timeouts, 0 is a - // meaningful value here (unlimited), so the omit-sentinel is the default - // itself rather than 0 — writing the default would inject the field into - // hand-edited files that never had it and break byte-stable round-trips. + // Persist only when it differs from the default. Like connectionTimeout and + // unlike requestTimeout, 0 is a meaningful value here (unlimited), so the + // omit-sentinel is the default itself rather than 0 — writing the default + // would inject the field into hand-edited files that never had it and break + // byte-stable round-trips. if (settings.maxFetchRequests !== DEFAULT_MAX_FETCH_REQUESTS) { out.maxFetchRequests = settings.maxFetchRequests; } diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 3edd8a7e..04386b0a 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -132,7 +132,11 @@ export type StoredMCPServer = MCPServerConfig & { * (#1629) */ modernLogLevel?: ModernLogLevel; - /** Inspector-specific connect-time timeout (ms). */ + /** + * Inspector-specific connect-time timeout (ms). Absent reads back as + * `DEFAULT_CONNECTION_TIMEOUT_MS`; an explicit `0` disables the timeout and + * is persisted so it round-trips (#2320). + */ connectionTimeout?: number; /** Inspector-specific request timeout (ms). */ requestTimeout?: number; @@ -665,6 +669,18 @@ export type OnInsufficientScopePolicy = "reauthorize" | "throw"; */ export const DEFAULT_TASK_TTL_MS = 60000; +/** + * Default connect-time timeout (ms) applied when a server has no explicit + * `connectionTimeout` — on disk, in the form, or on the client. The SDK has no + * connect-time timeout of its own; without this, the only thing bounding a + * connect attempt was the SDK's per-request timeout on `initialize`, which + * fires after 60 s with a message that describes a JSON-RPC request rather + * than a connection (#2320). `0` remains the explicit opt-out ("no timeout"), + * so a stored `connectionTimeout: 0` and the CLI's `--connect-timeout 0` keep + * their documented meaning; only an *absent* value resolves to this. + */ +export const DEFAULT_CONNECTION_TIMEOUT_MS = 30000; + /** * Default maximum number of HTTP fetch requests retained in the Network log * (per server). When exceeded, the oldest entries rotate out. A larger value @@ -803,6 +819,10 @@ export interface InspectorServerSettings { * empty/unset means "inherit". Only meaningful for stdio transports. */ cwd?: string; + /** + * Connect-time timeout (ms). Defaults to `DEFAULT_CONNECTION_TIMEOUT_MS` + * (30000) when the server has no explicit value; `0` means no timeout. + */ connectionTimeout: number; requestTimeout: number; /** TTL (ms) for tasks created via "Run as task". Defaults to 60000. */ diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index 293d6cd6..b938e53f 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -181,7 +181,8 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `modernLogLevel` | `"debug"` | Per-request log level stamped on modern connections, or `"off"`. Legacy connections ignore it | | `roots` | — | Roots advertised via the `roots` client capability; each is `{ uri, name? }` | | `metadata` | — | Default `_meta` payload merged into every outgoing request — a JSON object; values may be any JSON except for the reserved keys noted below. Sent by all three clients | -| `connectionTimeout` / `requestTimeout` | — | Timeouts in ms | +| `connectionTimeout` | `30000` | Connect-time timeout in ms (`DEFAULT_CONNECTION_TIMEOUT_MS`). `0` disables it; on timeout the error says where to raise it | +| `requestTimeout` | — | Per-request timeout in ms; absent or `0` leaves the SDK default | | `taskTtl` | `60000` | TTL in ms for tasks created via "Run as task" (`DEFAULT_TASK_TTL_MS`) | | `autoRefreshOnListChanged` | `false` | Refresh lists automatically on `*/list_changed` instead of only flagging the indicator | | `paginatedLists` | `false` | Fetch tools/resources/prompts one page at a time instead of auto-aggregating | diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 6576adb8..9c9bc27e 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -240,7 +240,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Shape**: - On disk (`StoredMCPServer` in `core/mcp/types.ts`): `headers` as `Record`, `metadata` as a pair-array (Inspector-only — no compat target), numeric timeouts, `oauth` as a nested `{ clientId?, clientSecret?, scopes? }` object. Every field optional; absent fields are omitted on disk so the file diff stays minimal. - - In memory + on the wire (`InspectorServerSettings` in `core/mcp/types.ts`): `headers` as a pair-array `{ key, value }[]`, flat `oauthClientId` / `oauthClientSecret` / `oauthScopes` fields, required `metadata` (pair-array) + required numeric `connectionTimeout` / `requestTimeout` (the form needs concrete values to render; 0 is the SDK's "no timeout" signal). + - In memory + on the wire (`InspectorServerSettings` in `core/mcp/types.ts`): `headers` as a pair-array `{ key, value }[]`, flat `oauthClientId` / `oauthClientSecret` / `oauthScopes` fields, required `metadata` (pair-array) + required numeric `connectionTimeout` / `requestTimeout` (the form needs concrete values to render). For `requestTimeout`, 0 is the SDK's "use your own default" signal. For `connectionTimeout`, an absent on-disk value reads back as `DEFAULT_CONNECTION_TIMEOUT_MS` (30 s, #2320) and an explicit 0 means "no timeout" and is persisted as such. - The bidirectional conversion lives in `core/mcp/serverList.ts` (`storedFieldsToInspectorSettings` / `inspectorSettingsToStoredFields`) and is invoked by `mcpConfigToServerEntries` / `serverEntriesToMcpConfig` and by the server route's `buildStoredEntry`. - **Where it takes effect**: - `headers` → wire headers on every SSE / streamable-http request (`core/mcp/node/transport.ts` consumes the pair-array via `InspectorServerSettings.headers`).