* fix(web): cancel the underlying task when cancelling a task tool call (#1455) The ToolDetailPanel Cancel button fired onCancel, but App never supplied an onCancelToolCall handler, so clicking Cancel did nothing — a tool running as a task kept running on the server. App now captures the in-flight task-augmented call's taskId from the `toolCallTaskUpdated` event callToolStream dispatches, and the new onCancelToolCall handler cancels that task via the existing tasks API (reusing onCancelTask). The id is reset at the start of every call, so an ordinary (non-task) call's Cancel stays a no-op (no task to cancel, no request-abort channel yet) — preserving prior behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): show a toast when a task is cancelled Cancellation goes out as a `taskCancelled` event (not a status notification), so a running task's open 'Task <status>' toast would otherwise hang with no confirmation. Subscribe to `taskCancelled` and replace that toast (or show a fresh one) with a short auto-closing 'Task cancelled' toast, pruning the dead progress entry. Covers both cancel paths — the Tasks screen and the Tool detail panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): report a cancelled task as cancelled, not failed When a task is cancelled, the in-flight callToolStream rejects with a generic -32603 error, whose error path reported the task as "failed" — so the Tasks screen flashed FAILED until a manual refresh fetched the server's true "cancelled" state. InspectorClient now records cancelled task ids (set in cancelRequestorTask before the await, since cancelling unblocks the stream) and the stream's error path labels the terminal task "cancelled" with a matching status message. Cleared on disconnect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: address review nits on task-cancel wiring - Prune the consumed taskId from cancelledTaskIds in the stream error path so the set can't grow across a long session (disconnect-clear remains the backstop for cancels whose task completed before the cancel landed). - Clear activeToolCallTaskIdRef before the cancel resolves so a rapid second Cancel click doesn't re-cancel the terminating task (which would surface a spurious 'Failed to cancel task' toast). Add a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
8d4363d0ea
commit
6bb49e55ba
4 changed files with 284 additions and 5 deletions
|
|
@ -263,6 +263,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
onReadResource: (uri: string) => void;
|
||||
onSetLogLevel: (level: string) => void;
|
||||
onCancelTask: (taskId: string) => void;
|
||||
onCancelToolCall: () => void;
|
||||
onClearCompletedTasks: () => void;
|
||||
onRefreshTasks: () => void;
|
||||
onServerSettings: (id: string) => void;
|
||||
|
|
@ -357,6 +358,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
call-as-task
|
||||
</button>
|
||||
<button onClick={() => props.onCancelTask("task-1")}>cancel-task</button>
|
||||
<button onClick={() => props.onCancelToolCall()}>cancel-tool-call</button>
|
||||
<button onClick={() => props.onClearCompletedTasks()}>
|
||||
clear-completed
|
||||
</button>
|
||||
|
|
@ -756,6 +758,87 @@ describe("App task wiring", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("cancels the underlying task when a task-augmented tool call is cancelled", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
const client = clientInstances[0] as unknown as {
|
||||
cancelRequestorTask: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
||||
// callToolStream surfaces the created task's id via `toolCallTaskUpdated`
|
||||
// mid-call; App stashes it so Cancel knows which task to cancel (#1455).
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("toolCallTaskUpdated", {
|
||||
detail: { taskId: "task-42", task: { taskId: "task-42" } },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
await user.click(screen.getByText("cancel-tool-call"));
|
||||
|
||||
expect(client.cancelRequestorTask).toHaveBeenCalledWith("task-42");
|
||||
});
|
||||
|
||||
it("does not re-cancel on a rapid second Cancel click", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
const client = clientInstances[0] as unknown as {
|
||||
cancelRequestorTask: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("toolCallTaskUpdated", {
|
||||
detail: { taskId: "task-42", task: { taskId: "task-42" } },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
// Two clicks before the call resolves must cancel only once — the second
|
||||
// finds the ref already cleared, avoiding a spurious cancel of a terminal
|
||||
// task.
|
||||
await user.click(screen.getByText("cancel-tool-call"));
|
||||
await user.click(screen.getByText("cancel-tool-call"));
|
||||
|
||||
expect(client.cancelRequestorTask).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("does not cancel a task when an ordinary tool call is cancelled", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
const client = clientInstances[0] as unknown as {
|
||||
cancelRequestorTask: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
||||
// A stale task id from an earlier task call...
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("toolCallTaskUpdated", {
|
||||
detail: { taskId: "old-task", task: { taskId: "old-task" } },
|
||||
}),
|
||||
);
|
||||
});
|
||||
// ...is cleared when a new ordinary call starts, so its Cancel is a no-op.
|
||||
await user.click(screen.getByText("call"));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("tool-status")).toHaveTextContent("ok"),
|
||||
);
|
||||
|
||||
await user.click(screen.getByText("cancel-tool-call"));
|
||||
|
||||
expect(client.cancelRequestorTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("shows a URL-elicitation toast when a tool call fails with a no-list -32042", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
|
|
@ -941,6 +1024,57 @@ describe("App task wiring", () => {
|
|||
),
|
||||
);
|
||||
});
|
||||
|
||||
it("shows a 'Task cancelled' toast when a task is cancelled", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
// No live status toast for this task — cancellation shows a fresh one.
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("taskCancelled", { detail: { taskId: "task-1" } }),
|
||||
);
|
||||
});
|
||||
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ title: "Task cancelled", color: "gray" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("converts a running task's live toast into the cancellation toast", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
// A running task has an open "Task working" toast...
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("taskStatusChange", {
|
||||
detail: {
|
||||
taskId: "task-1",
|
||||
task: { status: "working", statusMessage: "Interpreting" },
|
||||
},
|
||||
}),
|
||||
);
|
||||
});
|
||||
const liveId = notificationsMock.show.mock.calls[0][0].id;
|
||||
notificationsMock.show.mockClear();
|
||||
|
||||
// ...which the cancel replaces in place (update), not a stacked toast.
|
||||
act(() => {
|
||||
clientInstances[0].dispatchEvent(
|
||||
new CustomEvent("taskCancelled", { detail: { taskId: "task-1" } }),
|
||||
);
|
||||
});
|
||||
|
||||
expect(notificationsMock.show).not.toHaveBeenCalled();
|
||||
expect(notificationsMock.update).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ id: liveId, title: "Task cancelled" }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// Live-apply roots on settings-dialog close: the App diffs the final draft
|
||||
|
|
|
|||
|
|
@ -347,6 +347,10 @@ function formatErrorDetails(err: unknown): string {
|
|||
// progress stops (i.e. the call finished or went quiet).
|
||||
const PROGRESS_TOAST_AUTOCLOSE_MS = 5000;
|
||||
|
||||
// A task cancellation is a one-shot confirmation (unlike the live status toast,
|
||||
// which stays open while the task runs), so it auto-dismisses after a moment.
|
||||
const TASK_CANCELLED_TOAST_AUTOCLOSE_MS = 5000;
|
||||
|
||||
// Stable toast id for a progress stream. Notifications keyed by this id are
|
||||
// replaced (not stacked) so a chatty server updates one toast per stream
|
||||
// rather than flooding the corner. The injected `progressToken` correlates a
|
||||
|
|
@ -585,6 +589,15 @@ function App() {
|
|||
// `notifications/tasks/status` tick replaces the existing toast.
|
||||
const taskToastIdsRef = useRef<Set<string>>(new Set());
|
||||
|
||||
// The taskId of the in-flight task-augmented tool call, captured from the
|
||||
// `toolCallTaskUpdated` event `callToolStream` dispatches. Lets the Tool
|
||||
// detail panel's Cancel button cancel the underlying task on the server
|
||||
// (#1455) instead of no-op'ing. Reset at the start of every call, so an
|
||||
// ordinary (non-task) call leaves it undefined and its Cancel doesn't fire a
|
||||
// stray task cancellation. A ref (not state) because it's only read at the
|
||||
// moment Cancel is clicked and must not trigger re-renders.
|
||||
const activeToolCallTaskIdRef = useRef<string | undefined>(undefined);
|
||||
|
||||
// Per-task progress, keyed by taskId. Sourced from the core `requestorTaskProgress`
|
||||
// event (emitted by callToolStream, which owns the taskId), fed to the Tasks
|
||||
// screen so `TaskCard`'s progress bar renders for active tasks. Pruned on
|
||||
|
|
@ -805,6 +818,30 @@ function App() {
|
|||
};
|
||||
}, [inspectorClient]);
|
||||
|
||||
// Capture the in-flight task-augmented tool call's taskId so the detail
|
||||
// panel's Cancel button can cancel the task on the server (#1455). The id
|
||||
// only becomes known mid-call, when `callToolStream` dispatches
|
||||
// `toolCallTaskUpdated`, so we stash the latest into the ref the cancel
|
||||
// handler reads. `onCallTool` clears it at the start of each call.
|
||||
useEffect(() => {
|
||||
if (!inspectorClient) return;
|
||||
const onToolCallTaskUpdated = (
|
||||
event: TypedEventGeneric<InspectorClientEventMap, "toolCallTaskUpdated">,
|
||||
) => {
|
||||
activeToolCallTaskIdRef.current = event.detail.taskId;
|
||||
};
|
||||
inspectorClient.addEventListener(
|
||||
"toolCallTaskUpdated",
|
||||
onToolCallTaskUpdated,
|
||||
);
|
||||
return () => {
|
||||
inspectorClient.removeEventListener(
|
||||
"toolCallTaskUpdated",
|
||||
onToolCallTaskUpdated,
|
||||
);
|
||||
};
|
||||
}, [inspectorClient]);
|
||||
|
||||
// Surface live task status as per-task toasts — the v2 replacement for v1/v1.5's
|
||||
// inline "Task status: … Polling…" line under the Tool Result (#1422, consistent
|
||||
// with #1414). Subscribes to `taskStatusChange` (server `notifications/tasks/status`)
|
||||
|
|
@ -862,11 +899,47 @@ function App() {
|
|||
) => {
|
||||
handleTaskUpdate(event.detail.taskId, event.detail.task);
|
||||
};
|
||||
// A cancel goes out as `taskCancelled` (dispatched by cancelRequestorTask),
|
||||
// not as a status notification, so it would otherwise leave the running
|
||||
// task's live "Task <status>" toast hanging with no confirmation. Replace
|
||||
// that toast (or show a fresh one) with a short "Task cancelled" toast, and
|
||||
// prune the now-dead progress entry. Covers both cancel paths — the Tasks
|
||||
// screen and the Tool detail panel — since both route through
|
||||
// cancelRequestorTask (#1455).
|
||||
const onTaskCancelled = (
|
||||
event: TypedEventGeneric<InspectorClientEventMap, "taskCancelled">,
|
||||
) => {
|
||||
const { taskId } = event.detail;
|
||||
setProgressByTaskId((prev) => {
|
||||
if (!(taskId in prev)) return prev;
|
||||
const next = { ...prev };
|
||||
delete next[taskId];
|
||||
return next;
|
||||
});
|
||||
const id = taskToastId(taskId);
|
||||
const toast = {
|
||||
id,
|
||||
title: "Task cancelled",
|
||||
message: "The task was cancelled.",
|
||||
color: taskToastColor("cancelled"),
|
||||
autoClose: TASK_CANCELLED_TOAST_AUTOCLOSE_MS,
|
||||
};
|
||||
if (liveToastIds.has(id)) {
|
||||
// Convert the open status toast into the auto-closing confirmation; drop
|
||||
// it from the live set so a trailing "cancelled" status tick (if the
|
||||
// server sends one) doesn't re-show it.
|
||||
notifications.update(toast);
|
||||
liveToastIds.delete(id);
|
||||
} else {
|
||||
notifications.show(toast);
|
||||
}
|
||||
};
|
||||
inspectorClient.addEventListener("taskStatusChange", onTaskStatusChange);
|
||||
inspectorClient.addEventListener(
|
||||
"requestorTaskUpdated",
|
||||
onRequestorTaskUpdated,
|
||||
);
|
||||
inspectorClient.addEventListener("taskCancelled", onTaskCancelled);
|
||||
return () => {
|
||||
inspectorClient.removeEventListener(
|
||||
"taskStatusChange",
|
||||
|
|
@ -876,6 +949,7 @@ function App() {
|
|||
"requestorTaskUpdated",
|
||||
onRequestorTaskUpdated,
|
||||
);
|
||||
inspectorClient.removeEventListener("taskCancelled", onTaskCancelled);
|
||||
// Hide any still-visible task toasts on client swap so they don't linger
|
||||
// into the next session, then drop the bookkeeping (mirrors the progress-
|
||||
// toast teardown above).
|
||||
|
|
@ -1378,6 +1452,10 @@ function App() {
|
|||
const asTask =
|
||||
serverSupportsTaskToolCalls &&
|
||||
(runAsTask || tool.execution?.taskSupport === "required");
|
||||
// Drop any prior call's task id before starting; a task-augmented call
|
||||
// repopulates it via the `toolCallTaskUpdated` listener below, an ordinary
|
||||
// call leaves it cleared (#1455).
|
||||
activeToolCallTaskIdRef.current = undefined;
|
||||
setToolCallState({ status: "pending" });
|
||||
try {
|
||||
// ToolsScreen types the args as `Record<string, unknown>` (it accepts
|
||||
|
|
@ -1673,6 +1751,22 @@ function App() {
|
|||
[inspectorClient],
|
||||
);
|
||||
|
||||
// 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.
|
||||
const onCancelToolCall = useCallback(() => {
|
||||
const taskId = activeToolCallTaskIdRef.current;
|
||||
if (taskId) {
|
||||
// Clear the ref before the call resolves so a rapid second Cancel click
|
||||
// doesn't re-cancel the now-terminating task (which would surface a
|
||||
// spurious "Failed to cancel task" toast).
|
||||
activeToolCallTaskIdRef.current = undefined;
|
||||
void onCancelTask(taskId);
|
||||
}
|
||||
}, [onCancelTask]);
|
||||
|
||||
const onClearCompletedTasks = useCallback(() => {
|
||||
clearCompletedTasks();
|
||||
}, [clearCompletedTasks]);
|
||||
|
|
@ -2184,6 +2278,7 @@ function App() {
|
|||
onCallTool={(name, args, runAsTask) => {
|
||||
void onCallTool(name, args, runAsTask);
|
||||
}}
|
||||
onCancelToolCall={onCancelToolCall}
|
||||
onClearToolResult={onClearToolResult}
|
||||
onReadResourceContents={onReadResourceContents}
|
||||
onRefreshTools={onRefreshTools}
|
||||
|
|
|
|||
|
|
@ -3956,6 +3956,21 @@ describe("InspectorClient", () => {
|
|||
const taskId = taskCreatedDetail.taskId;
|
||||
expect(taskId).toBeDefined();
|
||||
|
||||
// Collect the optimistic task updates the stream emits. The cancellation
|
||||
// ends the stream with a generic error; the terminal update must report
|
||||
// "cancelled" (not "failed") so the UI lands on the true state without
|
||||
// waiting for a refresh (#1455).
|
||||
const taskUpdates: TaskWithOptionalCreatedAt[] = [];
|
||||
const onRequestorTaskUpdated = (
|
||||
e: CustomEvent<{ taskId: string; task: TaskWithOptionalCreatedAt }>,
|
||||
) => {
|
||||
taskUpdates.push(e.detail.task);
|
||||
};
|
||||
client!.addEventListener(
|
||||
"requestorTaskUpdated",
|
||||
onRequestorTaskUpdated as EventListener,
|
||||
);
|
||||
|
||||
const cancelledPromise = waitForEvent<{ taskId: string }>(
|
||||
client,
|
||||
"taskCancelled",
|
||||
|
|
@ -3967,6 +3982,10 @@ describe("InspectorClient", () => {
|
|||
cancelledPromise,
|
||||
taskPromise,
|
||||
]);
|
||||
client!.removeEventListener(
|
||||
"requestorTaskUpdated",
|
||||
onRequestorTaskUpdated as EventListener,
|
||||
);
|
||||
expect(cancelledResult.status).toBe("fulfilled");
|
||||
const cancelledDetail = (
|
||||
cancelledResult as PromiseFulfilledResult<{ taskId: string }>
|
||||
|
|
@ -3974,6 +3993,12 @@ describe("InspectorClient", () => {
|
|||
expect(cancelledDetail.taskId).toBe(taskId);
|
||||
expect(taskResult.status).toBe("rejected");
|
||||
|
||||
// The terminal optimistic update is "cancelled", never "failed".
|
||||
const terminalUpdate = taskUpdates.find((t) =>
|
||||
["completed", "failed", "cancelled"].includes(t.status),
|
||||
);
|
||||
expect(terminalUpdate?.status).toBe("cancelled");
|
||||
|
||||
const task = await client!.getRequestorTask(taskId);
|
||||
expect(task.status).toBe("cancelled");
|
||||
});
|
||||
|
|
|
|||
|
|
@ -191,6 +191,14 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
};
|
||||
// Resource subscriptions
|
||||
private subscribedResources: Set<string> = new Set();
|
||||
// Task ids the user explicitly cancelled. A cancel makes the in-flight
|
||||
// `callToolStream` reject with a generic -32603 error, which the stream's
|
||||
// error path would otherwise report as a *failed* task — flashing "failed"
|
||||
// in the UI until a refresh fetches the server's true "cancelled" state.
|
||||
// Recording the id lets that path label the terminal task "cancelled"
|
||||
// instead, so it lands in the right state immediately (#1455). Cleared on
|
||||
// disconnect.
|
||||
private cancelledTaskIds: Set<string> = new Set();
|
||||
// Receiver tasks (server-initiated: server sends createMessage/elicit with params.task, server polls us)
|
||||
private receiverTasks: boolean;
|
||||
private receiverTaskTtlMs: number | (() => number);
|
||||
|
|
@ -1040,6 +1048,7 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
this.pendingElicitations = [];
|
||||
// Clear resource subscriptions on disconnect
|
||||
this.subscribedResources.clear();
|
||||
this.cancelledTaskIds.clear();
|
||||
// Clear receiver tasks: stop TTL timers and drop records
|
||||
for (const record of this.receiverTaskRecords.values()) {
|
||||
if (record.cleanupTimeoutId != null) {
|
||||
|
|
@ -1182,6 +1191,10 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
if (!this.client) {
|
||||
throw new Error("Client is not connected");
|
||||
}
|
||||
// Mark before awaiting: cancelling unblocks the in-flight callToolStream,
|
||||
// whose error message may arrive before this resolves — the stream's error
|
||||
// path reads this set to label the task "cancelled" rather than "failed".
|
||||
this.cancelledTaskIds.add(taskId);
|
||||
await this.client.experimental.tasks.cancelTask(
|
||||
taskId,
|
||||
this.getRequestOptions(),
|
||||
|
|
@ -1826,21 +1839,33 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
message.error.message || "Task execution failed";
|
||||
error = new Error(errorMessage);
|
||||
if (taskId) {
|
||||
const failedTask: TaskWithOptionalCreatedAt = {
|
||||
// A user-cancelled task surfaces here as a generic error; report
|
||||
// it as "cancelled" (not "failed") so the UI lands on the true
|
||||
// terminal state immediately, matching what a refresh would show
|
||||
// (#1455).
|
||||
const cancelled = this.cancelledTaskIds.has(taskId);
|
||||
// Consume the marker — task ids are single-use, so this keeps the
|
||||
// set from growing across a long session of cancellations (the
|
||||
// disconnect-clear stays the backstop for cancels whose task
|
||||
// completed before the cancel landed and never hit this path).
|
||||
this.cancelledTaskIds.delete(taskId);
|
||||
const terminalTask: TaskWithOptionalCreatedAt = {
|
||||
taskId,
|
||||
ttl: null,
|
||||
status: "failed",
|
||||
statusMessage: errorMessage,
|
||||
status: cancelled ? "cancelled" : "failed",
|
||||
statusMessage: cancelled
|
||||
? "Client cancelled task execution."
|
||||
: errorMessage,
|
||||
lastUpdatedAt: new Date().toISOString(),
|
||||
};
|
||||
this.dispatchTypedEvent("toolCallTaskUpdated", {
|
||||
taskId,
|
||||
task: failedTask,
|
||||
task: terminalTask,
|
||||
error: message.error,
|
||||
});
|
||||
this.dispatchTypedEvent("requestorTaskUpdated", {
|
||||
taskId,
|
||||
task: failedTask,
|
||||
task: terminalTask,
|
||||
error: message.error,
|
||||
});
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue