Merge origin/v2/main into v2/feat/2318-timeout-diagnostics
Resolves the relay-wait conflict with #2028: the wait keeps its progress-driven re-arm and expires with the SDK's timeout shape. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APKSYHGxXBUpnR7ErFiLJn Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
commit
1b46b9564b
10 changed files with 796 additions and 111 deletions
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
115
clients/web/src/test/core/mcp/remote/progressToken.test.ts
Normal file
115
clients/web/src/test/core/mcp/remote/progressToken.test.ts
Normal file
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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<typeof createPushableSseStream>;
|
||||
getSentId: () => string | number | undefined;
|
||||
} {
|
||||
let sse!: ReturnType<typeof createPushableSseStream>;
|
||||
let sentId: string | number | undefined;
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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,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<void>(() => {}),
|
||||
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<void>(() => {}),
|
||||
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", () => {
|
||||
|
|
|
|||
|
|
@ -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> = {},
|
||||
): 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);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue