feat: bound connect attempts with a 30 s default connectionTimeout (#2320)
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4h8Xa6Ed9zwtv5ojX35MT Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
153ee7dd15
commit
e641edeeab
14 changed files with 343 additions and 38 deletions
|
|
@ -119,7 +119,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en
|
|||
| `--log-level <level>` | Logging level for `logging/setLevel` (e.g. `debug`, `info`). |
|
||||
| `--metadata <key=value>` | 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 <key=value>` | Tool-specific `_meta` entries for `tools/call`. Same JSON-parsed value handling as `--metadata`. |
|
||||
| `--connect-timeout <ms>` | 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 <ms>` | 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 <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). |
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<void>(() => {}),
|
||||
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<void>(() => {}),
|
||||
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<void>(() => {}),
|
||||
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", () => {
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<typeof setTimeout> | undefined;
|
||||
const timeoutPromise = new Promise<never>((_, 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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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. */
|
||||
|
|
|
|||
|
|
@ -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 |
|
||||
|
|
|
|||
Loading…
Reference in a new issue