From 3220d6239a52b0667500cdf4c213c60e24df689f Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Sat, 13 Jun 2026 22:23:02 -0400 Subject: [PATCH] fix(web): send a cancellation notification when cancelling an ordinary tool call (#1458) (#1463) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): send a cancellation notification when cancelling an ordinary tool call (#1458) The Tool detail panel's Cancel button was a silent no-op for ordinary (non-task) tool calls: App.onCancelToolCall found no captured taskId and did nothing, leaving the button looking actionable while the call ran on. Wire request cancellation through the MCP cancellation flow. callTool now tracks the in-flight call with an AbortController whose signal is threaded into the SDK request options; aborting it makes the SDK send a notifications/cancelled for that request and reject the pending call. cancelToolCall() performs the abort. callTool surfaces the cancellation as a ToolCallCancelledError (without recording a failed call), which App clears as a clean cancel plus a short confirmation toast. Task-augmented calls are unchanged: they have a server-side task and are still cancelled via cancelRequestorTask (#1457). Closes #1458 Co-Authored-By: Claude Opus 4.8 (1M context) * fix(web): address review — distinguish disconnect from cancel; tidy callTool params Review follow-ups on #1463: - Disconnect no longer surfaces as "Tool call cancelled". The catch in callToolWithRetries now throws ToolCallCancelledError only when the abort reason matches the deliberate user-cancel reason (TOOL_CALL_CANCELLED_REASON); a disconnect aborts the same controller with a different reason and falls through to the normal error path, so no misleading toast. - Note the activeToolCallAbortController slot is shared (last-writer-wins) across all callTool callers. - Fold callToolWithRetries/attemptToolCall's long positional parameter list into a single ToolCallRequest object; move the URL-elicitation loop state into callToolWithRetries. Adds an integration test that a disconnect mid-call rejects as an ordinary error (not ToolCallCancelledError), using a slow send_progress call over HTTP. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- clients/web/src/App.test.tsx | 35 +++- clients/web/src/App.tsx | 26 ++- .../integration/mcp/inspectorClient.test.ts | 85 +++++++++ core/mcp/inspectorClient.ts | 163 +++++++++++++++--- core/mcp/toolCallCancelledError.ts | 26 +++ 5 files changed, 309 insertions(+), 26 deletions(-) create mode 100644 core/mcp/toolCallCancelledError.ts diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 9a96ce70..ccd6ae56 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { ErrorCode, McpError } from "@modelcontextprotocol/sdk/types.js"; import { UrlElicitationLoopError } from "@inspector/core/mcp/urlElicitation.js"; +import { ToolCallCancelledError } from "@inspector/core/mcp/toolCallCancelledError.js"; import { renderWithMantine, screen, @@ -51,6 +52,7 @@ vi.mock("@inspector/core/mcp/index.js", () => { .fn() .mockResolvedValue({ success: true, result: { acts: [] } }); cancelRequestorTask = vi.fn().mockResolvedValue(undefined); + cancelToolCall = vi.fn().mockReturnValue(true); getPrompt = vi.fn().mockResolvedValue({ result: { messages: [] } }); readResource = vi .fn() @@ -810,7 +812,7 @@ describe("App task wiring", () => { expect(client.cancelRequestorTask).toHaveBeenCalledTimes(1); }); - it("does not cancel a task when an ordinary tool call is cancelled", async () => { + it("aborts the request (not a task) when an ordinary tool call is cancelled", async () => { const user = userEvent.setup(); renderWithMantine(); await user.click(screen.getByText("connect")); @@ -818,6 +820,7 @@ describe("App task wiring", () => { const client = clientInstances[0] as unknown as { cancelRequestorTask: ReturnType; + cancelToolCall: ReturnType; }; // A stale task id from an earlier task call... @@ -828,7 +831,8 @@ describe("App task wiring", () => { }), ); }); - // ...is cleared when a new ordinary call starts, so its Cancel is a no-op. + // ...is cleared when a new ordinary call starts, so Cancel routes to the + // request-abort path (notifications/cancelled), not the task API (#1458). await user.click(screen.getByText("call")); await waitFor(() => expect(screen.getByTestId("tool-status")).toHaveTextContent("ok"), @@ -836,9 +840,36 @@ describe("App task wiring", () => { await user.click(screen.getByText("cancel-tool-call")); + expect(client.cancelToolCall).toHaveBeenCalledTimes(1); expect(client.cancelRequestorTask).not.toHaveBeenCalled(); }); + it("clears the executing state and toasts when a cancelled call rejects", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + // The aborted call rejects with ToolCallCancelledError (the SDK already sent + // notifications/cancelled). App treats that as a clean cancel, not a failure. + ( + clientInstances[0] as unknown as { + callTool: ReturnType; + } + ).callTool.mockRejectedValueOnce(new ToolCallCancelledError("get_acts")); + + await user.click(screen.getByText("call")); + + // The result panel returns to idle (no error state)... + await waitFor(() => + expect(screen.getByTestId("tool-status")).toHaveTextContent("none"), + ); + // ...and a confirmation toast acknowledges the cancellation. + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ title: "Tool call cancelled" }), + ); + }); + it("shows a URL-elicitation toast when a tool call fails with a no-list -32042", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 21571276..3489f6fe 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -27,6 +27,7 @@ import { getUrlElicitationsFromError, UrlElicitationLoopError, } from "@inspector/core/mcp/urlElicitation.js"; +import { ToolCallCancelledError } from "@inspector/core/mcp/toolCallCancelledError.js"; import type { TypedEventGeneric } from "@inspector/core/mcp/typedEventTarget.js"; import type { InspectorServerSettings, @@ -1481,6 +1482,20 @@ function App() { error: invocation.error, }); } catch (err) { + // The user cancelled the in-flight call (Cancel button → cancelToolCall). + // The cancellation notification was already sent to the server, so just + // clear the executing state — surfacing it as an error would read as a + // failure rather than the deliberate cancel it was (#1458). + if (err instanceof ToolCallCancelledError) { + setToolCallState(undefined); + notifications.show({ + title: "Tool call cancelled", + message: "A cancellation request was sent to the server.", + color: "gray", + autoClose: 3000, + }); + return; + } // The server kept asking for a URL the user already completed this call, // so callTool aborted to avoid an endless re-prompt loop. Surface that // explicitly rather than as a generic failure. @@ -1754,9 +1769,12 @@ function App() { // Cancel the in-flight tool call. A task-augmented call (run-as-task) has a // server-side task, so cancel that via the tasks API (#1455) — the cancelled // status then flows back through the managed task store and toasts, the same - // as cancelling from the Tasks screen. An ordinary call has no task (and no - // request-abort channel yet), so there is nothing to cancel server-side. + // as cancelling from the Tasks screen. An ordinary call has no task, so abort + // its request: the SDK sends a `notifications/cancelled` to the server (the + // MCP cancellation flow) and the pending call rejects with a + // ToolCallCancelledError that `onCallTool` clears as a cancellation (#1458). const onCancelToolCall = useCallback(() => { + if (!inspectorClient) return; const taskId = activeToolCallTaskIdRef.current; if (taskId) { // Clear the ref before the call resolves so a rapid second Cancel click @@ -1764,8 +1782,10 @@ function App() { // spurious "Failed to cancel task" toast). activeToolCallTaskIdRef.current = undefined; void onCancelTask(taskId); + return; } - }, [onCancelTask]); + inspectorClient.cancelToolCall(); + }, [inspectorClient, onCancelTask]); const onClearCompletedTasks = useCallback(() => { clearCompletedTasks(); diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 5a078242..7993000d 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -13,6 +13,7 @@ import { ManagedToolsState, } from "@inspector/core/mcp/state/index.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { ToolCallCancelledError } from "@inspector/core/mcp/toolCallCancelledError.js"; import { SamplingCreateMessage } from "@inspector/core/mcp/samplingCreateMessage.js"; import { ElicitationCreateMessage } from "@inspector/core/mcp/elicitationCreateMessage.js"; import { @@ -29,6 +30,7 @@ import { createCollectUrlElicitationTool, createUrlElicitationFormTool, createSendNotificationTool, + createSendProgressTool, createListRootsTool, createArgsPrompt, createNumberedTools, @@ -950,6 +952,89 @@ describe("InspectorClient", () => { } }); + it("cancelToolCall() is a no-op (returns false) when no call is in flight", () => { + expect(client!.cancelToolCall()).toBe(false); + }); + + it("ToolCallCancelledError carries the tool name when known, and reads generically without one", () => { + expect(new ToolCallCancelledError("echo").message).toContain('"echo"'); + expect(new ToolCallCancelledError().message).toBe( + "Tool call was cancelled.", + ); + }); + + it("cancelToolCall() aborts the in-flight call: sends notifications/cancelled, rejects with ToolCallCancelledError, and records no failed call", async () => { + const tool = await getTool(client!, "echo"); + + const messages: MessageEntry[] = []; + client!.addEventListener("message", (event) => { + messages.push(event.detail); + }); + let failedCallCount = 0; + client!.addEventListener("toolCallResultChange", (event) => { + if (!event.detail.success) failedCallCount++; + }); + + // Start the call, then cancel synchronously — before the response can be + // processed — so the abort always wins the race regardless of tool speed. + const promise = client!.callTool(tool, { message: "hello world" }); + expect(client!.cancelToolCall()).toBe(true); + + await expect(promise).rejects.toBeInstanceOf(ToolCallCancelledError); + + // The MCP cancellation flow: a notifications/cancelled reaches the server. + const cancelled = messages.find( + (m) => + m.direction === "notification" && + "method" in m.message && + m.message.method === "notifications/cancelled", + ); + expect(cancelled).toBeDefined(); + + // Cancelling is intentional, so it is not recorded as a failed tool call. + expect(failedCallCount).toBe(0); + + // The controller was cleared, so a second cancel is a no-op. + expect(client!.cancelToolCall()).toBe(false); + }); + + it("a disconnect mid-call does not surface as a ToolCallCancelledError", async () => { + // disconnect() aborts the same in-flight controller, but with a different + // reason than cancelToolCall(). The call must reject as an ordinary error, + // not a user-cancel — so App doesn't show a misleading "cancelled" toast. + // Use a genuinely slow tool (over HTTP) so the call is still in flight when + // we disconnect; echo would complete before teardown. + await client!.disconnect(); + server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createSendProgressTool()], + }); + await server.start(); + client = new InspectorClient( + { type: "streamable-http", url: server.url }, + { + environment: { transport: createTransportNode }, + clientIdentity: { name: "test", version: "1.0.0" }, + }, + ); + await client.connect(); + const tool = await getTool(client, "send_progress"); + + // 20 units * 200ms ≈ 4s — comfortably still running when we disconnect. + const promise = client.callTool(tool, { units: 20, delayMs: 200 }); + await new Promise((resolve) => setTimeout(resolve, 150)); + await client.disconnect(); + + let caught: unknown; + try { + await promise; + } catch (err) { + caught = err; + } + expect(caught).toBeDefined(); + expect(caught).not.toBeInstanceOf(ToolCallCancelledError); + }); + it("should paginate tools when maxPageSize is set", async () => { // Disconnect and create a new server with pagination await client!.disconnect(); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index c543b1b1..d1e0017c 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -117,6 +117,7 @@ import { getUrlElicitationsFromError, UrlElicitationLoopError, } from "./urlElicitation.js"; +import { ToolCallCancelledError } from "./toolCallCancelledError.js"; import type { AuthGuidedState, OAuthStep } from "../auth/types.js"; import type { OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js"; import type pino from "pino"; @@ -141,6 +142,29 @@ interface ReceiverTaskRecord { */ const MAX_URL_ELICITATION_RETRIES = 5; +/** + * The abort reason used by `cancelToolCall()`. It rides along on the + * `notifications/cancelled` sent to the server and lets `callToolWithRetries` + * tell a deliberate user cancel apart from other aborts of the same controller + * (e.g. a disconnect, which aborts with a different reason and should surface as + * an ordinary error, not a "Tool call cancelled" — #1458). + */ +const TOOL_CALL_CANCELLED_REASON = "Tool call cancelled by user"; + +/** + * The descriptor for a single tools/call, threaded through the retry loop and + * each attempt. Bundled into one object so `callToolWithRetries`/`attemptToolCall` + * don't take a long, transposition-prone positional parameter list. + */ +interface ToolCallRequest { + tool: Tool; + args: Record; + generalMetadata?: Record; + toolSpecificMetadata?: Record; + taskOptions?: { ttl?: number }; + options?: { skipOutputValidation?: boolean }; +} + /** * InspectorClient wraps an MCP Client and provides: * - Message tracking and storage @@ -199,6 +223,13 @@ export class InspectorClient extends InspectorClientEventTarget { // instead, so it lands in the right state immediately (#1455). Cleared on // disconnect. private cancelledTaskIds: Set = new Set(); + // 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. + // Task-augmented calls have a server-side task and are cancelled via + // `cancelRequestorTask` instead, so they don't use this (#1458). + private activeToolCallAbortController?: AbortController; // Receiver tasks (server-initiated: server sends createMessage/elicit with params.task, server polls us) private receiverTasks: boolean; private receiverTaskTtlMs: number | (() => number); @@ -441,13 +472,21 @@ export class InspectorClient extends InspectorClientEventTarget { return { ...(defaults ?? {}), ...(callMetadata ?? {}) }; } - private getRequestOptions(progressToken?: ProgressToken): RequestOptions { + private getRequestOptions( + progressToken?: ProgressToken, + signal?: AbortSignal, + ): RequestOptions { const opts: RequestOptions = { resetTimeoutOnProgress: this.resetTimeoutOnProgress, }; 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). + if (signal) { + opts.signal = signal; + } if (this.progress) { const token = progressToken; const onprogress: ProgressCallback = (progress: Progress) => { @@ -1049,6 +1088,10 @@ export class InspectorClient extends InspectorClientEventTarget { // Clear resource subscriptions on disconnect this.subscribedResources.clear(); this.cancelledTaskIds.clear(); + // Abort any in-flight ordinary tool call so its promise settles instead of + // hanging past teardown; drop the controller reference either way. + this.activeToolCallAbortController?.abort("Disconnected"); + this.activeToolCallAbortController = undefined; // Clear receiver tasks: stop TTL timers and drop records for (const record of this.receiverTaskRecords.values()) { if (record.cleanupTimeoutId != null) { @@ -1204,6 +1247,34 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("taskCancelled", { taskId }); } + /** + * 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}. + * + * Task-augmented calls have a server-side task and are cancelled via + * {@link cancelRequestorTask} instead — this is a no-op for them (and whenever + * no ordinary call is in flight). + * + * @returns `true` if a call was in flight to cancel, `false` otherwise. + */ + cancelToolCall(): boolean { + const controller = this.activeToolCallAbortController; + if (!controller) { + return false; + } + // Drop the reference up front so a rapid second cancel is a clean no-op and + // can't re-abort a call that's already terminating. + this.activeToolCallAbortController = undefined; + // The reason string rides along on the `notifications/cancelled` the SDK + // sends to the server (and lets the call's catch distinguish this deliberate + // cancel from other aborts of the same controller, e.g. a disconnect). + controller.abort(TOOL_CALL_CANCELLED_REASON); + return true; + } + /** * List all requestor tasks with optional pagination (tasks we created on the server) * @param cursor Optional pagination cursor @@ -1406,28 +1477,71 @@ export class InspectorClient extends InspectorClientEventTarget { ); } - // Retry loop for the URL-elicitation error path: a `-32042` + const request: ToolCallRequest = { + tool, + args, + generalMetadata, + toolSpecificMetadata, + taskOptions, + options, + }; + // Track this call so `cancelToolCall()` can abort it. Aborting makes the SDK + // send a `notifications/cancelled` to the server (the MCP cancellation flow) + // and reject the pending request, which we surface as a + // `ToolCallCancelledError`. This single slot is shared by *every* `callTool` + // caller (last-writer-wins), so `cancelToolCall()` targets the most recently + // started ordinary call — fine today since the Cancel button only surfaces + // for the single Tools-screen call. Cleared in `finally`, but only if still + // ours so a later overlapping call's controller isn't clobbered (#1458). + const abortController = new AbortController(); + this.activeToolCallAbortController = abortController; + try { + return await this.callToolWithRetries(request, abortController); + } finally { + if (this.activeToolCallAbortController === abortController) { + this.activeToolCallAbortController = undefined; + } + } + } + + /** + * The URL-elicitation retry loop for {@link callTool}, factored out so the + * caller can wrap it in the abort-controller lifecycle (`try`/`finally`). On a + * user cancellation (the call's abort signal fired with the cancel reason) the + * SDK has already sent `notifications/cancelled`, so we throw a + * {@link ToolCallCancelledError} without recording a failed call — the cancel + * was intentional, not a failure. + */ + private async callToolWithRetries( + request: ToolCallRequest, + abortController: AbortController, + ): Promise { + const { tool, args, generalMetadata, toolSpecificMetadata } = request; + // Retry-loop state for the URL-elicitation error path: a `-32042` // (UrlElicitationRequired) response means the server needs the user to // complete one or more URL elicitations before the call can succeed. We // surface them, wait for completion, then re-issue the same call. The // counter bounds a server that keeps returning `-32042` so we can't spin - // forever (each accepted round is one attempt). + // forever (each accepted round is one attempt). `presentedUrls` guards the + // loop: a retry that re-requests a URL we already handled can't progress, so + // we abort with a UrlElicitationLoopError rather than re-prompt. let urlElicitationAttempt = 0; - // URLs already presented in this call. If a retry's error re-requests one, - // completing it again can't make progress (the server isn't advancing), so - // we abort with a UrlElicitationLoopError rather than re-prompt the user. const presentedUrls = new Set(); while (true) { try { - return await this.attemptToolCall( - tool, - args, - generalMetadata, - toolSpecificMetadata, - taskOptions, - options, - ); + return await this.attemptToolCall(request, abortController.signal); } catch (error) { + // The controller was aborted. A deliberate `cancelToolCall()` (matched + // by reason) means the SDK already sent `notifications/cancelled` — so + // surface a clean cancellation, not a generic failure, and don't record + // it in history. Any other abort (e.g. a disconnect, which aborts with a + // different reason) falls through to the normal error path (#1458). + if ( + abortController.signal.aborted && + abortController.signal.reason === TOOL_CALL_CANCELLED_REASON + ) { + throw new ToolCallCancelledError(tool.name); + } const urlElicitations = getUrlElicitationsFromError(error); if ( urlElicitations && @@ -1507,13 +1621,17 @@ export class InspectorClient extends InspectorClientEventTarget { * retry loop owns the elicitation handling and failure bookkeeping. */ private async attemptToolCall( - tool: Tool, - args: Record, - generalMetadata?: Record, - toolSpecificMetadata?: Record, - taskOptions?: { ttl?: number }, - options?: { skipOutputValidation?: boolean }, + request: ToolCallRequest, + signal?: AbortSignal, ): Promise { + const { + tool, + args, + generalMetadata, + toolSpecificMetadata, + taskOptions, + options, + } = request; const client = this.client; if (!client) { throw new Error("Client is not connected"); @@ -1562,7 +1680,10 @@ export class InspectorClient extends InspectorClientEventTarget { // returned (and that legacy hosts render fine). For those passthrough // calls go through request() directly, which skips that host-side // validation. Regular Tools-screen calls keep validating. - const requestOptions = this.getRequestOptions(metadata?.progressToken); + const requestOptions = this.getRequestOptions( + metadata?.progressToken, + signal, + ); // Both branches yield a CallToolResult: request() parsed it with // CallToolResultSchema above, callTool() returns the same shape — so the // `as CallToolResult` casts below are safe. diff --git a/core/mcp/toolCallCancelledError.ts b/core/mcp/toolCallCancelledError.ts new file mode 100644 index 00000000..85a46f7f --- /dev/null +++ b/core/mcp/toolCallCancelledError.ts @@ -0,0 +1,26 @@ +/** + * Thrown by `InspectorClient.callTool` when the in-flight ordinary (non-task) + * tool call is cancelled via `cancelToolCall()`. By the time this is thrown the + * SDK has already sent the `notifications/cancelled` to the server (the MCP + * cancellation flow) and rejected the underlying request; `callTool` converts + * that rejection into this error so the web layer can clear the executing state + * as a clean cancellation instead of surfacing a generic failure — and so the + * cancelled call is *not* recorded as a failed call in request history. + * + * Task-augmented calls have a server-side task and are cancelled via + * `cancelRequestorTask()` instead, so they never produce this error. + */ +export class ToolCallCancelledError extends Error { + /** The name of the tool whose call was cancelled, when known. */ + readonly toolName?: string; + + constructor(toolName?: string) { + super( + toolName + ? `Tool call "${toolName}" was cancelled.` + : "Tool call was cancelled.", + ); + this.name = "ToolCallCancelledError"; + this.toolName = toolName; + } +}