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/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 003f8422..0d89558d 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -1,5 +1,6 @@ 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, @@ -367,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" }, { @@ -417,19 +433,7 @@ describe("InspectorClient", () => { }); 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, - }); + const fakeFactory = () => ({ transport: hangingTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:1/never" }, { @@ -464,19 +468,7 @@ describe("InspectorClient", () => { // 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, - }); + const fakeFactory = () => ({ transport: hangingTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:1/never" }, { environment: { transport: fakeFactory } }, @@ -504,19 +496,7 @@ describe("InspectorClient", () => { 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, - }); + const fakeFactory = () => ({ transport: hangingTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:1/never" }, { @@ -551,23 +531,7 @@ describe("InspectorClient", () => { }); 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; - }, - send: async () => {}, - close: async () => {}, - onclose: undefined, - onerror: undefined, - onmessage: undefined, - sessionId: undefined, - }; - const fakeFactory = () => ({ - transport: - unauthorizedTransport as unknown as import("@modelcontextprotocol/client").Transport, - }); + const fakeFactory = () => ({ transport: unauthorizedTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:8081/mcp" }, { environment: { transport: fakeFactory } }, @@ -584,23 +548,7 @@ describe("InspectorClient", () => { // 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, - }); + const fakeFactory = () => ({ transport: unauthorizedTransport() }); client = new InspectorClient( { type: "streamable-http", url: "http://localhost:8081/mcp" }, { diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index fa5074ef..04728a58 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -491,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 @@ -637,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. @@ -723,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/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`).