diff --git a/README.md b/README.md index ccf9d6002..43733c2ce 100644 --- a/README.md +++ b/README.md @@ -163,6 +163,51 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | | `tasks-{legacy,modern}-http.json` | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | +| `cancellation-modern-http.json` | Cancelling a call by closing its response stream | [#2140](https://github.com/modelcontextprotocol/inspector/issues/2140) | + +#### Cancelling a call + +`cancellation-modern-http.json` serves `slow_task`, which reports progress once +a second for up to 60 seconds and stops early if it is cancelled, printing how +far it got to **the server's terminal**. Connect with **Protocol Era = Modern**. + +Watch the terminal you started the server in, run `slow_task` from the Tools +tab, and click **Cancel** after a few seconds. The progress must stop +immediately, the Inspector must report the call cancelled, and the server must +print `[slow_task] cancelled after Ns`. On the broken build the progress kept +arriving until the tool completed all 60 seconds and the server printed +`completed all 60s without being cancelled`, because the Inspector was sending +the wrong cancellation signal +([#2140](https://github.com/modelcontextprotocol/inspector/issues/2140)). + +The server's terminal is the place to watch, not the Inspector's result panel: +cancellation closes the very stream the tool's result would travel on, so on a +successful cancel the tool's return value is undeliverable by construction and +the Inspector shows a cancelled call rather than a result. Which is the point — +what has to stop is the *work*, and only the server can report that. + +The 2026-07-28 spec makes this +[transport-specific](https://modelcontextprotocol.io/specification/2026-07-28/basic/patterns/cancellation#transport-specific-cancellation): +for Streamable HTTP, **closing the request's SSE response stream is the +cancellation signal**, and a `notifications/cancelled` is "neither required nor +expected"; stdio, which has no per-request stream to close, keeps the +notification. A spec-compliant server therefore answers the notification `202 +Accepted` and drops it — which is precisely what the reporter observed, with the +task running on to completion while the Inspector reported it cancelled. + +The SDK already implements that fork, off `transport.hasPerRequestStream`. Every +Inspector connection is wrapped in `MessageTrackingTransport` (it feeds the +Protocol and Network tabs), which did not forward the flag — so the SDK saw +`undefined` and took the stdio branch on **every** client, CLI and TUI included. +The web client needed two more links in the chain: its browser-side transport +answers for the real upstream one that lives on the Node backend, and the abort +has to survive the `POST /api/mcp/send` hop to reach it. + +Watch it on the wire in the **Protocol** tab: cancelling now emits no +`notifications/cancelled` frame at all, and the `tools/call` entry ends as an +aborted request rather than a completed one. Switch the same server to +**Legacy** and the notification comes back — that era has no per-stream +mechanism, so it is still the correct signal there. #### MCP Apps diff --git a/clients/web/src/test/core/mcp/messageTrackingTransport.test.ts b/clients/web/src/test/core/mcp/messageTrackingTransport.test.ts index 7d1782d0d..5f7204a2c 100644 --- a/clients/web/src/test/core/mcp/messageTrackingTransport.test.ts +++ b/clients/web/src/test/core/mcp/messageTrackingTransport.test.ts @@ -12,6 +12,8 @@ class FakeTransport implements Transport { sessionId?: string; // Optional on the SDK Transport interface; stdio omits it, HTTP defines it. setProtocolVersion?: (version: string) => void; + // Optional too: Streamable HTTP advertises it, stdio and SSE say nothing. + hasPerRequestStream?: boolean; async start(): Promise {} async send(message: JSONRPCMessage): Promise { this.sent.push(message); @@ -272,4 +274,19 @@ describe("MessageTrackingTransport.setProtocolVersion", () => { expect(() => tracked.setProtocolVersion("2025-06-18")).not.toThrow(); expect(tracked.protocolVersion).toBe("2025-06-18"); }); + + // #2140: the SDK reads `hasPerRequestStream` off the transport it was handed + // — always this wrapper — to choose between aborting a request's own stream + // and POSTing `notifications/cancelled`. A wrapper that answers for the base + // transport instead of forwarding its answer breaks Cancel on every client. + it("forwards the base transport's per-request-stream capability", () => { + const { tracked, base } = makeTracked(); + expect(tracked.hasPerRequestStream).toBeUndefined(); + + base.hasPerRequestStream = true; + expect(tracked.hasPerRequestStream).toBe(true); + + base.hasPerRequestStream = false; + expect(tracked.hasPerRequestStream).toBe(false); + }); }); diff --git a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts index c2ddcc2a0..1aed2bea0 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -569,4 +569,121 @@ describe("RemoteClientTransport", () => { await transport.close(); }); + + // #2140: the Cancel button's wire signal is chosen by the SDK from the + // transport's `hasPerRequestStream`. The real upstream transport lives on the + // backend, so this transport answers for it — and must answer per server + // type, since only Streamable HTTP gives a request a stream of its own to + // close. + describe("per-request-stream cancellation (#2140)", () => { + it("advertises hasPerRequestStream only for streamable-http", () => { + const forType = (c: MCPServerConfig) => + new RemoteClientTransport( + { baseUrl, fetchFn: vi.fn() }, + c, + ).hasPerRequestStream; + + expect( + forType({ type: "streamable-http", url: "http://server.example/mcp" }), + ).toBe(true); + // stdio and SSE multiplex every request over one shared channel, so + // aborting anything would cancel the whole session rather than the call. + // The SDK must keep sending `notifications/cancelled` for them. + expect(forType(config)).toBe(false); + expect(forType({ type: "sse", url: "http://server.example/sse" })).toBe( + false, + ); + }); + + it("applies the SDK's requestSignal to the browser-to-backend send fetch", async () => { + const seenInits: (RequestInit | undefined)[] = []; + let sse = createPushableSseStream(); + const fetchFn = vi + .fn() + .mockImplementation(async (input, init) => { + const url = String(input); + if (url.endsWith("/api/mcp/connect")) { + return new Response(JSON.stringify({ sessionId: "abc" }), { + status: 200, + }); + } + if (url.includes("/api/mcp/events")) { + sse = createPushableSseStream(); + return sse.response; + } + if (url.endsWith("/api/mcp/send")) { + seenInits.push(init); + return new Response(JSON.stringify({ ok: true }), { status: 200 }); + } + return new Response("not found", { status: 404 }); + }); + + const transport = new RemoteClientTransport({ baseUrl, fetchFn }, config); + await transport.start(); + + const controller = new AbortController(); + await transport.send( + { jsonrpc: "2.0", method: "notifications/initialized" }, + { requestSignal: controller.signal }, + ); + expect(seenInits[0]?.signal).toBe(controller.signal); + + // No signal supplied (stdio/SSE, where the SDK never creates one) must + // leave the fetch unaborted rather than passing `undefined` through as a + // deliberate value. + await transport.send({ + jsonrpc: "2.0", + method: "notifications/roots/list_changed", + }); + expect(seenInits[1]).not.toHaveProperty("signal"); + + await transport.close(); + }); + + it("aborting the requestSignal rejects the in-flight send", async () => { + let sse = createPushableSseStream(); + const fetchFn = vi + .fn() + .mockImplementation(async (input, init) => { + const url = String(input); + if (url.endsWith("/api/mcp/connect")) { + return new Response(JSON.stringify({ sessionId: "abc" }), { + status: 200, + }); + } + if (url.includes("/api/mcp/events")) { + sse = createPushableSseStream(); + return sse.response; + } + if (url.endsWith("/api/mcp/send")) { + // The backend holds this route open for the whole call, so model it + // as a fetch that only settles when the caller aborts. + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new DOMException("Aborted", "AbortError")), + ); + }); + } + return new Response("not found", { status: 404 }); + }); + + const transport = new RemoteClientTransport({ baseUrl, fetchFn }, config); + await transport.start(); + + const controller = new AbortController(); + const sent = transport.send( + { jsonrpc: "2.0", id: 7, method: "tools/call" }, + { requestSignal: controller.signal }, + ); + + controller.abort(); + // The backend never answers an aborted send, so the rejection has to come + // from the fetch itself. `postSend` already cancels the SSE response wait + // it registered for this id on that path (its catch does, unchanged by + // #2140) — so the send surfaces the abort rather than the wait's timeout. + await expect(sent).rejects.toThrow(/Aborted/); + + await transport.close(); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts index 06b932d5c..83e15acbf 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts @@ -20,6 +20,7 @@ import { createMrtrEdgeCaseTool, loadConfig, resolveConfig, + type ToolDefinition, } from "@modelcontextprotocol/inspector-test-server"; import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server"; import type { @@ -437,6 +438,146 @@ describe("modern-era negotiation (2026-07-28)", () => { expect(result.result!.isError).toBeFalsy(); }); + // #2140: on the 2026-07-28 era, closing the request's own response stream IS + // the cancellation signal for Streamable HTTP; `notifications/cancelled` is + // the stdio mechanism and the spec says it is neither required nor expected + // here. The SDK implements that fork off `transport.hasPerRequestStream` — + // but every Inspector connection is wrapped in `MessageTrackingTransport`, + // which was not forwarding it, so all three clients POSTed the notification + // and a spec-compliant server acknowledged it `202` and kept running. + // + // This asserts at the far end, on the server's own request signal: nothing on + // the client side distinguishes a cancel that reached the server from one + // that was dropped, which is exactly how this shipped. + it("cancels by aborting the request's stream, which the server observes", async () => { + let sawAbort!: (value: boolean) => void; + const aborted = new Promise((resolve) => { + sawAbort = resolve; + }); + let started!: () => void; + const running = new Promise((resolve) => { + started = resolve; + }); + + const slowServer = createTestServerHttp({ + serverInfo: createTestServerInfo("cancel-era-test", "1.0.0"), + tools: [ + { + name: "slow_task", + description: "Runs until the client cancels it", + handler: async (_params, _context, extra) => { + extra?.signal?.addEventListener("abort", () => sawAbort(true), { + once: true, + }); + started(); + // Never settles on its own, so a regression is a timeout rather + // than a pass. + return new Promise(() => {}); + }, + }, + ], + modern: {}, + }); + await slowServer.start(); + server = slowServer; + + const connected = await connectWithEra(slowServer.url, "modern"); + const { tools } = await connected.listTools(); + const tool = tools.find((t) => t.name === "slow_task"); + + // Hold the rejection immediately so it is never seen as unhandled while we + // wait on the server side. + const settled = connected.callTool(tool!, {}).catch((err: unknown) => err); + await running; + expect(connected.cancelToolCall()).toBe(true); + + await expect(settled).resolves.toBeInstanceOf(ToolCallCancelledError); + await expect(aborted).resolves.toBe(true); + }, 30_000); + + // The test above builds its own never-returning tool, which is the sharpest + // way to assert on the server's abort signal but says nothing about the + // artifacts a human actually uses. This one drives the documented showcase + // through the SAME path they would — the checked-in config, resolved through + // the preset registry — so a misspelt preset name, a config naming a dead + // preset, or a regression in the `slow_task` handler's own abort loop fails + // here rather than only when someone runs the repro by hand. + // + // It observes the handler's own return value, not the client's view. Counting + // progress notifications would be a false negative: the SDK drops a cancelled + // request's progress handler locally, so the client stops seeing ticks the + // moment it cancels whether or not the server ever stopped working — the very + // silence this bug hid behind. + it("stops the showcase tool's work on cancel (slow_task, via the showcase config)", async () => { + const config = loadConfig( + join(repoRoot, "test-servers/configs/cancellation-modern-http.json"), + ); + expect(config.tools).toContainEqual({ preset: "slow_task" }); + + const resolved = resolveConfig(config); + expect(resolved.tools?.map((tool) => tool.name)).toEqual([ + "slow_task", + "echo", + ]); + + // Wrap the resolved preset rather than replacing it, so what runs is the + // real registered handler and only its outcome is observed. `tools` is a + // union with the task-tool shape, whose handler has a different signature, + // so narrow to the plain one this preset actually is. + const preset = resolved.tools!.find( + (tool): tool is ToolDefinition => tool.name === "slow_task", + )!; + let reportOutcome!: (text: string) => void; + const outcome = new Promise((resolve) => { + reportOutcome = resolve; + }); + let firstTick!: () => void; + const running = new Promise((resolve) => { + firstTick = resolve; + }); + + const started = createTestServerHttp({ + ...resolved, + tools: resolved.tools!.map((tool) => + tool.name === "slow_task" + ? { + ...preset, + handler: async (params, context, extra) => { + extra?.signal?.addEventListener("abort", () => firstTick(), { + once: true, + }); + const result = await preset.handler(params, context, extra); + reportOutcome(JSON.stringify(result)); + return result; + }, + } + : tool, + ), + }); + await started.start(); + server = started; + const connected = await connectWithEra(started.url, "modern"); + + const { tools } = await connected.listTools(); + const tool = tools.find((t) => t.name === "slow_task"); + expect(tool).toBeDefined(); + + // Hold the rejection immediately so it is never seen as unhandled. + const settled = connected + .callTool(tool!, { seconds: 30 }) + .catch((err: unknown) => err); + + // Let it get past its first tick, then cancel. + await new Promise((resolve) => setTimeout(resolve, 1500)); + expect(connected.cancelToolCall()).toBe(true); + await expect(settled).resolves.toBeInstanceOf(ToolCallCancelledError); + await running; + + // The handler must return promptly, saying it was cancelled — not run on to + // its 30th second, which is what it did before the fix. + await expect(outcome).resolves.toContain("cancelled after"); + }, 30_000); + it("cancels an in-flight MRTR call while its embedded request is pending", async () => { const started = await startMrtrServer(createMrtrTool()); const connected = await connectWithEra(started.url, "modern"); diff --git a/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts b/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts new file mode 100644 index 000000000..612b4cbfc --- /dev/null +++ b/clients/web/src/test/integration/mcp/remote/cancel-per-request-stream.test.ts @@ -0,0 +1,199 @@ +/** + * #2140 — the Cancel button must send the transport's cancellation signal. + * + * On a 2026-07-28 Streamable HTTP connection the spec makes *closing the + * request's SSE response stream* the cancellation signal, and says a + * `notifications/cancelled` is neither required nor expected. The SDK + * implements that fork itself: `Protocol.request` aborts a per-request + * `AbortController` — handed to the transport as + * `TransportSendOptions.requestSignal` — when the connection is modern **and** + * the transport advertises `hasPerRequestStream`; otherwise it POSTs the + * notification. + * + * The web client lost it at the proxy boundary. The real upstream transport + * lives on the Node backend, and the browser's `RemoteClientTransport` neither + * advertised the flag nor applied a `requestSignal` to its + * `POST /api/mcp/send` — so every web cancel took the stdio branch, the server + * answered the notification `202 Accepted` and dropped it, and the tool kept + * running to completion. Only a manual disconnect actually cancelled anything. + * + * This drives the whole chain the browser drives — InspectorClient -> + * RemoteClientTransport -> the Hono backend -> a real upstream Streamable HTTP + * transport -> a modern test server — and asserts at the far end, on the + * server's own request abort signal. Nothing shallower reaches it: each of the + * three hops can drop the signal on its own, and a unit test of any one of them + * passes while the chain stays broken. + */ + +import { describe, it, expect, afterEach } from "vitest"; +import { serve } from "@hono/node-server"; +import type { ServerType } from "@hono/node-server"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createRemoteTransport } from "@inspector/core/mcp/remote/createRemoteTransport.js"; +import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; +import { ToolCallCancelledError } from "@inspector/core/mcp/toolCallCancelledError.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; +import { + createTestServerHttp, + createTestServerInfo, + type TestServerHttp, + type ToolDefinition, +} from "@modelcontextprotocol/inspector-test-server"; + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void; + const promise = new Promise((r) => { + resolve = r; + }); + return { promise, resolve }; +} + +/** + * A tool that never returns on its own. It reports when it started, and whether + * the server-side request signal — which the SDK aborts when the client closes + * this request's response stream — ever fired. + */ +function createSlowTool(): { + tool: ToolDefinition; + started: Promise; + aborted: Promise; +} { + const started = deferred(); + const aborted = deferred(); + const tool: ToolDefinition = { + name: "slow_task", + description: "Runs until the client cancels it", + handler: async (_params, _context, extra) => { + extra?.signal?.addEventListener("abort", () => aborted.resolve(true), { + once: true, + }); + started.resolve(); + // Never settles: this call ends by cancellation or not at all, so a + // regression shows up as the test's own timeout rather than as a pass. + return new Promise(() => {}); + }, + }; + return { tool, started: started.promise, aborted: aborted.promise }; +} + +async function startRemoteBackend(): Promise<{ + baseUrl: string; + server: ServerType; + authToken: string; +}> { + const { app, authToken } = createRemoteApp({ + initialConfig: { defaultEnvironment: {} }, + }); + return new Promise((resolve, reject) => { + const server = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + const port = + info && typeof info === "object" && "port" in info + ? (info as { port: number }).port + : 0; + resolve({ baseUrl: `http://127.0.0.1:${port}`, server, authToken }); + }, + ); + server.on("error", reject); + }); +} + +describe("web-client cancellation over a per-request stream (#2140)", () => { + let client: InspectorClient | null = null; + let backend: ServerType | null = null; + let upstream: TestServerHttp | null = null; + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // Ignore disconnect errors. + } + client = null; + } + if (backend) { + const server = backend; + backend = null; + await new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); + } + if (upstream) { + try { + await upstream.stop(); + } catch { + // Ignore stop errors. + } + upstream = null; + } + }); + + it("aborts the upstream request stream, which the server observes as cancellation", async () => { + const slow = createSlowTool(); + upstream = createTestServerHttp({ + serverInfo: createTestServerInfo("cancel-test", "1.0.0"), + tools: [slow.tool], + modern: {}, + }); + await upstream.start(); + + const { baseUrl, server, authToken } = await startRemoteBackend(); + backend = server; + + const connected = new InspectorClient( + { type: "streamable-http", url: upstream.url }, + { + environment: { + transport: createRemoteTransport({ baseUrl, authToken }), + }, + versionNegotiation: eraToVersionNegotiation("modern"), + }, + ); + await connected.connect(); + client = connected; + expect(connected.getProtocolEra()).toBe("modern"); + + const { tools } = await connected.listTools(); + const tool = tools.find((t) => t.name === "slow_task"); + expect(tool).toBeDefined(); + + // Hold the rejection from the first turn so the runner never sees it as + // unhandled while we wait on the server side. + const settled = connected.callTool(tool!, {}).catch((err: unknown) => err); + + await slow.started; + expect(connected.cancelToolCall()).toBe(true); + + // The user-facing contract is unchanged: a deliberate cancel still surfaces + // as ToolCallCancelledError, whichever wire signal carried it. + await expect(settled).resolves.toBeInstanceOf(ToolCallCancelledError); + + // The assertion that matters. Before the fix the server saw only a + // `notifications/cancelled` it was free to ignore (and did), its request + // signal never fired, and this promise never resolved. + await expect(slow.aborted).resolves.toBe(true); + }, 30_000); + + it("advertises the per-request stream only for streamable-http", () => { + // The flag is what the SDK forks on, and the factory is where a server's + // configured type reaches the transport. stdio and SSE multiplex every + // request over one shared channel: there is no per-request stream to close, + // and aborting anything would take the whole session down — so they must + // keep the `notifications/cancelled` mechanism. + const create = createRemoteTransport({ baseUrl: "http://unused.example" }); + expect( + create({ type: "stdio", command: "echo", args: [] }, {}).transport + .hasPerRequestStream, + ).toBe(false); + expect( + create({ type: "sse", url: "http://unused.example/sse" }, {}).transport + .hasPerRequestStream, + ).toBe(false); + expect( + create({ type: "streamable-http", url: "http://unused.example/mcp" }, {}) + .transport.hasPerRequestStream, + ).toBe(true); + }); +}); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 2f115beb4..45c91f6f7 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -648,9 +648,12 @@ export class InspectorClient extends InspectorClientEventTarget { >(); private rawWireRequestCounter = 0; // Abort controller for the in-flight ordinary (non-task) tool call. Aborting - // it makes the SDK send a `notifications/cancelled` for that request (the MCP - // cancellation flow) and reject the pending call, which `callTool` surfaces as - // a `ToolCallCancelledError`. Undefined when no ordinary call is in flight. + // it hands the SDK the MCP cancellation flow for that request and rejects the + // pending call, which `callTool` surfaces as a `ToolCallCancelledError`. Which + // signal reaches the server is the transport's business, not ours: a + // per-request-stream transport on a 2026-era connection has that request's + // stream aborted, and everything else gets `notifications/cancelled` (#2140). + // Undefined when no ordinary call is in flight. // Task-augmented calls have a server-side task and are cancelled via // `cancelRequestorTask` instead, so they don't use this (#1458). private activeToolCallAbortController?: AbortController; @@ -1169,8 +1172,10 @@ export class InspectorClient extends InspectorClientEventTarget { if (this.requestTimeout !== undefined) { opts.timeout = this.requestTimeout; } - // When provided, aborting this signal makes the SDK send a - // `notifications/cancelled` for the request and reject it (#1458). + // When provided, aborting this signal cancels the request and rejects it + // (#1458). The SDK picks the wire signal from the transport: an aborted + // per-request SSE stream on a 2026-era Streamable HTTP connection, and + // `notifications/cancelled` everywhere else (#2140). if (signal) { opts.signal = signal; } @@ -2976,10 +2981,13 @@ export class InspectorClient extends InspectorClientEventTarget { /** * Cancel the in-flight ordinary (non-task) tool call started by - * {@link callTool}. Aborting its request makes the SDK send a - * `notifications/cancelled` to the server (the MCP cancellation flow) and - * reject the pending call, which `callTool` surfaces as a - * {@link ToolCallCancelledError}. + * {@link callTool}. Aborting its request runs the MCP cancellation flow and + * rejects the pending call, which `callTool` surfaces as a + * {@link ToolCallCancelledError}. The SDK chooses the wire signal from the + * transport: on a 2026-era Streamable HTTP connection it aborts that + * request's own SSE response stream — the spec's cancellation signal — + * rather than sending `notifications/cancelled`, which is the stdio + * mechanism and remains in use there (#2140). * * Task-augmented calls have a server-side task and are cancelled via * {@link cancelRequestorTask} instead — this is a no-op for them (and whenever diff --git a/core/mcp/messageTrackingTransport.ts b/core/mcp/messageTrackingTransport.ts index 32b17169d..3402e244b 100644 --- a/core/mcp/messageTrackingTransport.ts +++ b/core/mcp/messageTrackingTransport.ts @@ -203,6 +203,30 @@ export class MessageTrackingTransport implements Transport { return this.baseTransport.sessionId; } + /** + * Forward the base transport's per-request-stream capability (#2140). + * + * The SDK's `Protocol.request` reads this off the transport it was handed — + * which is always this wrapper — to decide how a cancelled request is + * signalled: abort that request's own stream (the 2026-07-28 signal for + * Streamable HTTP) or POST a `notifications/cancelled` (the stdio one). + * + * Not forwarding it made the wrapper *answer* the question, `undefined`, for + * every client. So the Cancel button POSTed a notification even on a modern + * Streamable HTTP connection, where a spec-compliant server acknowledges it + * `202` and drops it — the tool kept running and only a disconnect really + * cancelled anything. That hit the CLI and TUI as much as the web client: + * their real `StreamableHTTPClientTransport` advertises the flag correctly + * and this wrapper hid it. + * + * A base transport that says nothing stays `undefined` rather than becoming + * `false`, since the SDK's check is `=== true` either way and inventing a + * value here would misreport what the transport claimed. + */ + get hasPerRequestStream(): boolean | undefined { + return this.baseTransport.hasPerRequestStream; + } + // Implemented as a concrete method (rather than delegating the base // transport's optional `setProtocolVersion`) so the SDK Client always // invokes it after the initialize handshake — including for stdio, whose diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 33d57e6d3..45185e412 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1022,6 +1022,36 @@ export function createRemoteApp( // Auth errors may reject the wait via onerror while send() also throws. void responseWait.catch(() => {}); } + + // #2140: this route is held open for the whole call, so the browser + // aborting its `POST /api/mcp/send` mid-flight *is* the client's + // cancellation signal on a per-request-stream upstream. Forward it as + // `requestSignal` so the upstream StreamableHTTP transport closes that + // request's SSE response stream, which is what the 2026-07-28 spec makes + // the cancellation signal. A transport with no per-request stream (stdio, + // SSE) ignores the option, and a *cancel* never reaches it as an abort + // anyway: `RemoteClientTransport` withholds `hasPerRequestStream` there, so + // the SDK keeps sending `notifications/cancelled`. Forwarding the + // disconnect unconditionally is still right — a browser that went away + // mid-call is not waiting for the answer whatever the transport is. + const upstreamAbort = new AbortController(); + const clientSignal = c.req.raw.signal; + const onClientDisconnect = () => { + upstreamAbort.abort(clientSignal.reason); + // The response will never arrive now; release the wait so this handler + // unwinds instead of sitting on a promise nothing can settle. + if (requestId !== undefined) { + session.cancelRequestWait(requestId); + } + }; + if (clientSignal.aborted) { + onClientDisconnect(); + } else { + clientSignal.addEventListener("abort", onClientDisconnect, { + once: true, + }); + } + try { // SEP-2243: apply the client's mirrored headers to the upstream request, // restricted to the `Mcp-Param-` prefix so a client can't inject other @@ -1031,6 +1061,7 @@ export function createRemoteApp( await session.transport.send(message, { relatedRequestId: relatedRequestId as string | number | undefined, ...(paramHeaders && { headers: paramHeaders }), + requestSignal: upstreamAbort.signal, }); if (responseWait) { await responseWait; @@ -1051,6 +1082,7 @@ export function createRemoteApp( const msg = err instanceof Error ? err.message : String(err); return c.json({ ok: false, kind: "transport_error", error: msg }); } finally { + clientSignal.removeEventListener("abort", onClientDisconnect); session.endSend(); } }); diff --git a/core/mcp/remote/remoteClientTransport.ts b/core/mcp/remote/remoteClientTransport.ts index 5ddbe6129..9cd22460d 100644 --- a/core/mcp/remote/remoteClientTransport.ts +++ b/core/mcp/remote/remoteClientTransport.ts @@ -261,6 +261,25 @@ export class RemoteClientTransport implements Transport { private readonly options: RemoteTransportOptions; private readonly config: import("../types.js").MCPServerConfig; + /** + * Whether the upstream connection gives each request its own response stream + * (#2140). The SDK's `Protocol.request` reads this: on a 2026-era connection + * a transport advertising it has its per-request stream **aborted** as the + * spec's cancellation signal, and no `notifications/cancelled` is sent; a + * transport that does not gets the stdio mechanism instead. + * + * The real upstream transport lives on the backend, so this transport has to + * answer for it. Streamable HTTP opens one POST — and one SSE response + * stream — per request, so it qualifies; stdio and SSE multiplex every + * request over one shared channel and must keep sending the notification. + * + * Advertising it is only half the fix: `requestSend` applies the SDK's + * `requestSignal` to the browser-to-backend fetch, and `/api/mcp/send` + * forwards that disconnect to the upstream `transport.send`, which is what + * actually closes the stream the server is watching. + */ + readonly hasPerRequestStream: boolean; + /** * Intentionally returns undefined. The MCP Client checks transport.sessionId to detect * reconnects and skip initialize. Our _sessionId is the remote server's session ID, not @@ -312,6 +331,7 @@ export class RemoteClientTransport implements Transport { ) { this.options = options; this.config = config; + this.hasPerRequestStream = config.type === "streamable-http"; } setAuthRecovery(handlers: AuthRecoveryHandlers | undefined): void { @@ -757,10 +777,17 @@ export class RemoteClientTransport implements Transport { }), }; + // #2140: aborting this fetch is the cancellation signal for a + // per-request-stream upstream. The backend holds `/api/mcp/send` open for + // the whole call (it awaits the JSON-RPC response), so the disconnect + // reaches it mid-flight and it aborts the upstream request's SSE stream. + // For stdio/SSE `hasPerRequestStream` is false, so the SDK never supplies + // a `requestSignal` and this is undefined. const res = await this.fetchFn(`${this.baseUrl}/api/mcp/send`, { method: "POST", headers: this.headers, body: JSON.stringify(body), + ...(options?.requestSignal && { signal: options.requestSignal }), }); if (!res.ok) { diff --git a/specification/v2_ux.md b/specification/v2_ux.md index 675e94722..5ac0c2dd4 100644 --- a/specification/v2_ux.md +++ b/specification/v2_ux.md @@ -390,7 +390,11 @@ When a tool execution triggers sampling or elicitation requests, they appear inl - Step description if provided - Elapsed time display - Execute button with loading state -- **Cancel button** sends `notifications/cancelled` +- **Cancel button** sends the transport-appropriate cancellation signal (#2140): + on a 2026-07-28 Streamable HTTP connection it aborts the request's own SSE + response stream, which the spec makes the cancellation signal; on stdio (and + on any pre-2026 connection, where the per-stream mechanism does not exist) it + sends `notifications/cancelled` - **Inline Client Request Queue** - When tool triggers sampling/elicitation: - Pending requests shown inline (not as separate modal) - Queue counter shows total pending requests diff --git a/specification/v2_ux_features.md b/specification/v2_ux_features.md index 17e65bd53..d100af8c2 100644 --- a/specification/v2_ux_features.md +++ b/specification/v2_ux_features.md @@ -110,7 +110,11 @@ When a tool execution triggers sampling or elicitation requests, they appear inl - Step description if provided - Elapsed time display - Execute button with loading state -- **Cancel button** sends `notifications/cancelled` +- **Cancel button** sends the transport-appropriate cancellation signal (#2140): + on a 2026-07-28 Streamable HTTP connection it aborts the request's own SSE + response stream, which the spec makes the cancellation signal; on stdio (and + on any pre-2026 connection, where the per-stream mechanism does not exist) it + sends `notifications/cancelled` - **Inline Client Request Queue** - When tool triggers sampling/elicitation: - Pending requests shown inline (not as separate modal) - Queue counter shows total pending requests diff --git a/test-servers/configs/cancellation-modern-http.json b/test-servers/configs/cancellation-modern-http.json new file mode 100644 index 000000000..b74f33632 --- /dev/null +++ b/test-servers/configs/cancellation-modern-http.json @@ -0,0 +1,12 @@ +{ + "serverInfo": { + "name": "cancellation-showcase", + "version": "1.0.0" + }, + "tools": [{ "preset": "slow_task" }, { "preset": "echo" }], + "transport": { + "type": "streamable-http", + "port": 6606, + "modern": true + } +} diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index f3f4bdfcb..26fa8b9a3 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -33,6 +33,7 @@ import { createCollectUrlElicitationTool, createUrlElicitationFormTool, createSendNotificationTool, + createSlowTaskTool, createGetAnnotatedMessageTool, createGetTempTool, createGetTempExtraTool, @@ -165,6 +166,8 @@ function resolveToolPreset( return createUrlElicitationFormTool(); case "send_notification": return createSendNotificationTool(); + case "slow_task": + return createSlowTaskTool(); case "get_annotated_message": return createGetAnnotatedMessageTool(); case "get_temp": diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 0eeccffc8..83e058ca6 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1219,6 +1219,98 @@ export function createCollectUrlElicitationTool(): ToolDefinition { }; } +/** + * Sleep `ms`, resolving `false` the moment `signal` aborts instead. + * + * Both listeners are removed on whichever outcome wins. A caller that sleeps in + * a loop would otherwise leave one attached per iteration — `{ once: true }` + * detaches a listener when the event *fires*, not when the waiter loses + * interest — and enough of them trip Node's `MaxListenersExceededWarning`. + */ +function sleepUnlessAborted( + ms: number, + signal?: AbortSignal, +): Promise { + return new Promise((resolve) => { + if (signal?.aborted) { + resolve(false); + return; + } + const onAbort = (): void => { + clearTimeout(timer); + resolve(false); + }; + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(true); + }, ms); + signal?.addEventListener("abort", onAbort, { once: true }); + }); +} + +/** + * Create a "slow_task" tool that runs until the client cancels it (#2140). + * + * It emits a progress notification every second up to `seconds` (default 60) + * and completes only if it is never cancelled, so there is a long, visible + * window in which to click Cancel. + * + * The point is what it does on the way out. On the 2026-07-28 era a client + * cancels a Streamable HTTP request by closing that request's response stream, + * and the SDK surfaces the disconnect to this handler as `extra.signal`. So the + * tool stops the moment the stream closes; cancelled by a + * `notifications/cancelled` the server is free to ignore, it keeps emitting + * progress until it completes on its own — the exact symptom reported. + * + * Both outcomes are written to **stderr**, because that is the only channel + * they can reach. A cancellation closes the very stream this handler's result + * would travel on, so its return value is undeliverable by construction: the + * server's terminal is where you watch this, not the Inspector's result panel. + */ +export function createSlowTaskTool(): ToolDefinition { + return { + name: "slow_task", + description: + "Run for up to `seconds`, reporting progress each second, until cancelled", + inputSchema: { + seconds: z + .number() + .int() + .min(1) + .max(600) + .optional() + .describe("How long to run before completing on its own (default 60)"), + }, + handler: async ( + params: Record, + _context?: TestServerContext, + extra?: HandlerExtra, + ) => { + const total = typeof params.seconds === "number" ? params.seconds : 60; + const progressToken = extra?._meta?.progressToken; + const signal = extra?.signal; + for (let step = 1; step <= total; step++) { + if (!(await sleepUnlessAborted(1000, signal))) { + // Cancelled. Return rather than keep working — the whole point of + // the demo is that the work actually stops. + const message = `[slow_task] cancelled after ${step - 1}s`; + console.error(message); + return toToolResult(message); + } + if (progressToken !== undefined) { + await extra?.sendNotification?.({ + method: "notifications/progress", + params: { progressToken, progress: step, total }, + }); + } + } + const message = `[slow_task] completed all ${total}s without being cancelled`; + console.error(message); + return toToolResult(message); + }, + }; +} + /** * Create a "send_notification" tool that sends a notification message from the server */ diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index 20f9baadd..f8a965c3b 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -38,9 +38,21 @@ import { * SDK v2's {@link WebStandardStreamableHTTPServerTransport} speaks the Fetch API * (`Request`/`Response`) rather than Node `req`/`res`. The JSON body is passed * to `handleRequest` via `parsedBody` (Express already parsed it), so the Web - * request carries only method, URL, and headers. + * request carries only method, URL, headers — and a `signal`. + * + * The signal is load-bearing, not decoration (#2140). On the 2026-07-28 era a + * client cancels a request by closing that request's response stream, and the + * SDK server surfaces the disconnect to a tool handler as `ctx.mcpReq.signal` + * — reading it off this Request. Building one without a signal therefore makes + * every fixture server deaf to cancellation, which is indistinguishable from a + * client that never sent the signal: a test asserting that a cancel reaches the + * server passes only if this is wired. + * + * `res`'s `close` is the disconnect, but only when the response had not already + * finished — Express emits it on every completed response too, and aborting + * then would cancel handlers that had already succeeded. */ -function toWebRequest(req: Request): globalThis.Request { +function toWebRequest(req: Request, res: Response): globalThis.Request { const headers = new Headers(); for (const [key, value] of Object.entries(req.headers)) { if (Array.isArray(value)) { @@ -50,7 +62,17 @@ function toWebRequest(req: Request): globalThis.Request { } } const url = `http://localhost${req.originalUrl || req.url}`; - return new globalThis.Request(url, { method: req.method, headers }); + const controller = new AbortController(); + res.on("close", () => { + if (!res.writableEnded) { + controller.abort(); + } + }); + return new globalThis.Request(url, { + method: req.method, + headers, + signal: controller.signal, + }); } /** @@ -436,7 +458,7 @@ export class TestServerHttp { } this.currentRequestHeaders = extractHeaders(req); try { - const webResponse = await handler.fetch(toWebRequest(req), { + const webResponse = await handler.fetch(toWebRequest(req, res), { parsedBody: req.body, }); await writeWebResponse(res, webResponse); @@ -540,9 +562,12 @@ export class TestServerHttp { } try { - const webResponse = await transport.handleRequest(toWebRequest(req), { - parsedBody: req.body, - }); + const webResponse = await transport.handleRequest( + toWebRequest(req, res), + { + parsedBody: req.body, + }, + ); await writeWebResponse(res, webResponse); } catch (error) { // If response already sent (e.g., by OAuth middleware), don't send another @@ -577,7 +602,7 @@ export class TestServerHttp { try { const webResponse = await newTransport.handleRequest( - toWebRequest(req), + toWebRequest(req, res), { parsedBody: req.body }, ); await writeWebResponse(res, webResponse); @@ -615,7 +640,9 @@ export class TestServerHttp { // Let the transport handle the GET request this.currentRequestHeaders = extractHeaders(req); try { - const webResponse = await transport.handleRequest(toWebRequest(req)); + const webResponse = await transport.handleRequest( + toWebRequest(req, res), + ); await writeWebResponse(res, webResponse); } catch (error) { if (!res.headersSent) {