From 56d3c0464351f0fd070fc407de0d6e25b16d2465 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 01:11:30 -0400 Subject: [PATCH] fix(test-servers): address review on the cancellation showcase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `slow_task` sleeps through a helper that removes both listeners on whichever outcome wins and returns early on an already-aborted signal. `{ once: true }` detaches on the event firing, not on the waiter losing interest, so the old loop left one listener per second attached and a 60s run tripped MaxListenersExceededWarning. - Report the outcome on stderr. A successful cancel closes the stream the result would travel on, so the tool's return value is undeliverable by construction — the README now points at the server's terminal rather than at a result panel that cannot show it. - Drop a double cast the file's own next hunk shows is unnecessary, and a floating observer promise whose assertion was false anyway: `postSend` rethrows before awaiting the SSE wait, so the send rejects on abort whether or not that wait was cancelled. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- README.md | 21 +++++-- .../mcp/remote/remoteClientTransport.test.ts | 14 ++--- test-servers/src/test-server-fixtures.ts | 63 +++++++++++++------ 3 files changed, 66 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index ef428547..43733c2c 100644 --- a/README.md +++ b/README.md @@ -168,15 +168,24 @@ Each config below is a ready-made server for exercising one feature by hand. Loa #### Cancelling a call `cancellation-modern-http.json` serves `slow_task`, which reports progress once -a second for up to 60 seconds and returns early — saying how far it got — if it -is cancelled. Connect with **Protocol Era = Modern**. +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**. -Run `slow_task` from the Tools tab and click **Cancel** after a few seconds. The -progress must stop immediately, and the result must read `Cancelled after Ns`. -On the broken build the progress kept arriving until the tool completed all 60 -seconds, because the Inspector was sending the wrong cancellation signal +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 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 a415b55f..1aed2bea 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -579,7 +579,7 @@ describe("RemoteClientTransport", () => { it("advertises hasPerRequestStream only for streamable-http", () => { const forType = (c: MCPServerConfig) => new RemoteClientTransport( - { baseUrl, fetchFn: vi.fn() as unknown as typeof fetch }, + { baseUrl, fetchFn: vi.fn() }, c, ).hasPerRequestStream; @@ -640,7 +640,7 @@ describe("RemoteClientTransport", () => { await transport.close(); }); - it("aborting the requestSignal rejects the in-flight send and releases its response wait", async () => { + it("aborting the requestSignal rejects the in-flight send", async () => { let sse = createPushableSseStream(); const fetchFn = vi .fn() @@ -675,15 +675,13 @@ describe("RemoteClientTransport", () => { { jsonrpc: "2.0", id: 7, method: "tools/call" }, { requestSignal: controller.signal }, ); - const settled = vi.fn(); - void sent.then(settled, settled); 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/); - // The response wait registered for id 7 must have been cancelled too — - // otherwise this send would hang past the abort waiting for a JSON-RPC - // response the aborted request can never produce. - expect(settled).toHaveBeenCalledTimes(1); await transport.close(); }); diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index c8decf0d..83e058ca 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1219,6 +1219,35 @@ 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). * @@ -1229,10 +1258,14 @@ export function createCollectUrlElicitationTool(): ToolDefinition { * 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 reports which signal it actually received: cancelled through the stream, - * the progress stops immediately; cancelled by a `notifications/cancelled` the - * server is free to ignore, the progress keeps arriving until the tool - * completes on its own — the exact symptom reported. + * 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 { @@ -1255,22 +1288,14 @@ export function createSlowTaskTool(): ToolDefinition { ) => { 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++) { - const slept = await new Promise((resolve) => { - const timer = setTimeout(() => resolve(true), 1000); - extra?.signal?.addEventListener( - "abort", - () => { - clearTimeout(timer); - resolve(false); - }, - { once: true }, - ); - }); - if (!slept || extra?.signal?.aborted) { + if (!(await sleepUnlessAborted(1000, signal))) { // Cancelled. Return rather than keep working — the whole point of // the demo is that the work actually stops. - return toToolResult(`Cancelled after ${step - 1}s`); + const message = `[slow_task] cancelled after ${step - 1}s`; + console.error(message); + return toToolResult(message); } if (progressToken !== undefined) { await extra?.sendNotification?.({ @@ -1279,7 +1304,9 @@ export function createSlowTaskTool(): ToolDefinition { }); } } - return toToolResult(`Completed all ${total}s without being cancelled`); + const message = `[slow_task] completed all ${total}s without being cancelled`; + console.error(message); + return toToolResult(message); }, }; }