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; + } +}