fix(test-servers): address review on the cancellation showcase
- `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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
85c5206895
commit
56d3c04643
3 changed files with 67 additions and 33 deletions
21
README.md
21
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
|
||||
|
|
|
|||
|
|
@ -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<typeof fetch>() },
|
||||
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<typeof fetch>()
|
||||
|
|
@ -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();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<boolean> {
|
||||
return new Promise<boolean>((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<boolean>((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);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue