From e641edeeab374291d7915f027fceaac5098c4787 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 13 Sep 2026 20:54:23 -0400 Subject: [PATCH] feat: bound connect attempts with a 30 s default connectionTimeout (#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connectionTimeout defaulted to 0, which InspectorClient.connect() read as "no bound at all" — so out of the box nothing the Inspector owns bounded a connect attempt, and the only thing ending a hung handshake was the SDK's 60 s per-request timeout on initialize, reporting "Request timed out". - Add DEFAULT_CONNECTION_TIMEOUT_MS (30000) to core/mcp/types.ts. An absent value resolves to it on the client, on disk read, in the web form draft, and in the CLI's header-only settings shell. An explicit 0 keeps its documented meaning (no timeout; --connect-timeout 0) and is now persisted so the opt-out round-trips; the default is the omit-sentinel, as for taskTtl. - Key the post-timeout transport teardown to the timer actually firing, not to connectionTimeout > 0 — otherwise a non-zero default would tear down every failed connect, including a recoverable 401 whose transport the auth recovery holds open. - The timeout message names the bound and where to raise it: "Connection timed out after N ms. To accommodate a slower server, you may increase the timeout value in Server Settings." - The CLI's UNREACHABLE classifier now matches that message. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4h8Xa6Ed9zwtv5ojX35MT Signed-off-by: cliffhall --- clients/cli/README.md | 2 +- clients/cli/__tests__/error-handler.test.ts | 15 ++ clients/cli/src/error-handler.ts | 2 +- .../web/src/test/core/mcp/serverList.test.ts | 65 ++++-- .../src/test/core/react/useServers.test.tsx | 4 +- .../integration/mcp/inspectorClient.test.ts | 185 +++++++++++++++++- .../mcp/remote/servers-route.test.ts | 13 +- .../src/utils/serverSettingsDefaults.test.ts | 3 +- .../web/src/utils/serverSettingsDefaults.ts | 3 +- core/mcp/inspectorClient.ts | 53 +++-- core/mcp/node/servers.ts | 8 +- core/mcp/serverList.ts | 15 +- core/mcp/types.ts | 22 ++- docs/mcp-server-configuration.md | 3 +- 14 files changed, 349 insertions(+), 44 deletions(-) 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/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..003f8422 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -1,7 +1,11 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { resolve } from "node:path"; 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, @@ -412,6 +416,140 @@ describe("InspectorClient", () => { expect(client.getStatus()).toBe("error"); }); + it("names the bound that fired and where to raise it in the timeout message (#2320)", async () => { + 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, + }); + 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: [], + }, + }, + ); + 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 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, + }); + 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 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, + }); + 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 unauthorizedTransport = { start: async () => { @@ -438,6 +576,51 @@ 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 unauthorizedTransport = { + start: async () => { + const err = new Error("Unauthorized") as Error & { status?: number }; + err.status = 401; + throw err; + }, + send: async () => {}, + close: async () => {}, + onclose: undefined, + onerror: undefined, + onmessage: undefined, + sessionId: undefined, + }; + const fakeFactory = () => ({ + transport: + unauthorizedTransport as unknown as import("@modelcontextprotocol/client").Transport, + }); + 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/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 91cc7ff8..c6fc0c64 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -67,6 +67,7 @@ import type { CreateTransportOptions, ServerType, } from "./types.js"; +import { DEFAULT_CONNECTION_TIMEOUT_MS } from "./types.js"; import { MessageTrackingTransport, type MessageTrackingCallbacks, @@ -293,6 +294,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 @@ -2220,11 +2235,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 @@ -2268,15 +2300,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]); @@ -2291,7 +2318,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/serverList.ts b/core/mcp/serverList.ts index 220253d5..fa5074ef 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, @@ -533,9 +534,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, @@ -659,7 +664,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) { diff --git a/core/mcp/types.ts b/core/mcp/types.ts index d55bb95f..87dc7dbf 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; @@ -643,6 +647,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 @@ -781,6 +797,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 |