fix: dedupe cancel-turn clicks to stop "turn cancelled" cascade (#784)

* fix: dedupe cancel-turn clicks to stop "turn cancelled" cascade The cancel button had no in-flight feedback, so impatient users clicked it repeatedly while the agent was tearing down a slow turn (e.g. a Claude Monitor tool). Each click reached the backend and emitted its own "Turn cancelled by user" message, with the second-and-later calls also lazily creating phantom turns to host those messages. Fix on both sides: SubmitButton now tracks isCancelling locally and disables itself with a spinner while the request is in flight; the orchestrator deduplicates concurrent CancelAgent calls per session via a sync.Map guard so duplicates that slip through return immediately. * fix: address PR review feedback - Test: replace sleep-based polling in cancel-dedupe test with channel synchronization (cancelAgentEntered) per project convention. - UI: switch the cancel re-entrancy guard from state to a ref so isCancelling no longer thrashes the useCallback dep array. --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
2026-05-02 22:49:19 +01:00
parent d8a9bb79d5
commit 58fb9ffb0f
6 changed files with 241 additions and 8 deletions
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"

@@ -144,6 +145,15 @@ type mockAgentManager struct {

 	// Optional override for GetExecutionIDForSession
 	getExecutionIDForSessionFunc func(context.Context, string) (string, error)
+
+	// CancelAgent tracking. cancelAgentCalls counts every invocation. If
+	// cancelAgentBlock is non-nil, CancelAgent blocks on it before returning;
+	// if cancelAgentEntered is non-nil, CancelAgent does a non-blocking send
+	// on it on entry. Together they let tests stage concurrent cancel calls
+	// without sleep-based polling.
+	cancelAgentCalls   atomic.Int32
+	cancelAgentBlock   chan struct{}
+	cancelAgentEntered chan struct{}
 }

 type stopAgentCall struct {
@@ -201,7 +211,19 @@ func (m *mockAgentManager) PromptAgent(_ context.Context, executionID string, pr
 	}
 	return &executor.PromptResult{}, nil
 }
-func (m *mockAgentManager) CancelAgent(_ context.Context, _ string) error { return nil }
+func (m *mockAgentManager) CancelAgent(_ context.Context, _ string) error {
+	m.cancelAgentCalls.Add(1)
+	if m.cancelAgentEntered != nil {
+		select {
+		case m.cancelAgentEntered <- struct{}{}:
+		default:
+		}
+	}
+	if m.cancelAgentBlock != nil {
+		<-m.cancelAgentBlock
+	}
+	return nil
+}
 func (m *mockAgentManager) RespondToPermissionBySessionID(_ context.Context, _, _, _ string, _ bool) error {
 	return nil
 }
@@ -262,6 +262,15 @@ type Service struct {
 	// activity before triggering fallback resume.
 	clarificationWatchdogTimeout time.Duration

+	// cancelInFlight tracks sessionIDs whose CancelAgent call is currently in
+	// progress. Used to deduplicate impatient retries from the UI: while the
+	// first cancel is still propagating through the agent (which can take several
+	// seconds when a long-running tool like Claude's Monitor is being torn down),
+	// the user often clicks the button repeatedly. Without this guard each click
+	// would create another "Turn cancelled by user" message and lazily start a
+	// phantom turn just to host it.
+	cancelInFlight sync.Map
+
 	// Service state
 	mu        sync.RWMutex
 	running   bool
@@ -1832,6 +1832,20 @@ func (s *Service) RespondToPermission(ctx context.Context, sessionID, pendingID,
 func (s *Service) CancelAgent(ctx context.Context, sessionID string) error {
 	s.logger.Debug("cancelling agent turn", zap.String("session_id", sessionID))

+	// Deduplicate concurrent retries. The UI's cancel button has no in-flight
+	// disable, so impatient users click it multiple times while the agent is
+	// still tearing down the turn (e.g. unwinding a Claude Monitor tool can take
+	// several seconds). Without this guard each click produces a duplicate
+	// "Turn cancelled by user" message and races on turn cleanup — the second
+	// call's getActiveTurnID lazily starts a phantom turn after the first call
+	// already closed the real one.
+	if _, busy := s.cancelInFlight.LoadOrStore(sessionID, struct{}{}); busy {
+		s.logger.Debug("cancel already in flight; skipping duplicate",
+			zap.String("session_id", sessionID))
+		return nil
+	}
+	defer s.cancelInFlight.Delete(sessionID)
+
 	// Fetch session for state updates and message creation
 	session, err := s.repo.GetTaskSession(ctx, sessionID)
 	if err != nil {
@@ -305,6 +305,66 @@ func TestPromptTask_ResetInProgressReturnsSentinelError(t *testing.T) {
 	}
 }

+// --- CancelAgent ---
+
+// TestCancelAgent_DeduplicatesConcurrentCalls covers the impatient-user case:
+// the UI's cancel button has no in-flight disable, so users click it multiple
+// times while the agent is still tearing down a slow turn (e.g. a Claude
+// Monitor tool). Without dedupe each click reaches the lifecycle layer and
+// emits its own "Turn cancelled by user" message; phantom turns are also
+// lazily started to host those messages. We assert that only one cancel makes
+// it through to agentManager.CancelAgent while one is already in flight.
+func TestCancelAgent_DeduplicatesConcurrentCalls(t *testing.T) {
+	repo := setupTestRepo(t)
+	agentMgr := &mockAgentManager{
+		isAgentRunning:     true,
+		cancelAgentBlock:   make(chan struct{}),
+		cancelAgentEntered: make(chan struct{}, 1),
+	}
+	svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr)
+
+	seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateRunning)
+
+	// First call goes async and parks inside agentManager.CancelAgent.
+	firstDone := make(chan error, 1)
+	go func() {
+		firstDone <- svc.CancelAgent(context.Background(), "session1")
+	}()
+
+	// Wait for the first call to actually enter agentManager.CancelAgent so
+	// the dedupe guard is set before the duplicate calls fire. Channel sync
+	// (over sleep-based polling) is the project convention for tests that
+	// don't depend on real subprocess timing.
+	<-agentMgr.cancelAgentEntered
+
+	// Fire several duplicates while the first is still parked. Each must be
+	// short-circuited by the dedupe guard and return immediately.
+	const duplicates = 5
+	for i := 0; i < duplicates; i++ {
+		if err := svc.CancelAgent(context.Background(), "session1"); err != nil {
+			t.Fatalf("duplicate cancel %d returned error: %v", i, err)
+		}
+	}
+	if got := agentMgr.cancelAgentCalls.Load(); got != 1 {
+		t.Fatalf("expected exactly 1 agentManager.CancelAgent call while first is in flight, got %d", got)
+	}
+
+	// Release the first call. After it returns, the guard clears and a fresh
+	// cancel is allowed through.
+	close(agentMgr.cancelAgentBlock)
+	if err := <-firstDone; err != nil {
+		t.Fatalf("first CancelAgent returned error: %v", err)
+	}
+
+	agentMgr.cancelAgentBlock = nil // unblock subsequent calls
+	if err := svc.CancelAgent(context.Background(), "session1"); err != nil {
+		t.Fatalf("post-release CancelAgent returned error: %v", err)
+	}
+	if got := agentMgr.cancelAgentCalls.Load(); got != 2 {
+		t.Fatalf("expected 2 agentManager.CancelAgent calls after release, got %d", got)
+	}
+}
+
 // --- StartCreatedSession ---

 func TestStartCreatedSession_WrongTask(t *testing.T) {
@@ -0,0 +1,97 @@
+import { describe, it, expect, vi, afterEach } from "vitest";
+import { render, screen, fireEvent, act, cleanup } from "@testing-library/react";
+
+afterEach(() => {
+  cleanup();
+});
+
+vi.mock("@kandev/ui/tooltip", () => ({
+  Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+  TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+  TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+  TooltipProvider: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+}));
+
+import { ChatInputToolbar } from "./chat-input-toolbar";
+
+function deferred<T>() {
+  let resolve!: (v: T) => void;
+  const promise = new Promise<T>((res) => {
+    resolve = res;
+  });
+  return { promise, resolve };
+}
+
+function renderToolbar(onCancel: () => void | Promise<void>) {
+  return render(
+    <ChatInputToolbar
+      planModeEnabled={false}
+      onPlanModeChange={() => {}}
+      sessionId="s1"
+      taskId="t1"
+      taskDescription=""
+      isAgentBusy
+      isDisabled={false}
+      isSending={false}
+      onCancel={onCancel}
+      onSubmit={() => {}}
+      minimalToolbar
+    />,
+  );
+}
+
+// The cancel button must disable itself while a cancel request is in flight.
+// Without this guard, an impatient user clicking it repeatedly while the agent
+// tears down a long-running tool (Claude Monitor, etc.) sends N cancel requests
+// to the backend, each producing a duplicate "Turn cancelled by user" message.
+describe("ChatInputToolbar cancel button", () => {
+  it("disables itself and blocks duplicate clicks while cancel is in flight", async () => {
+    const { promise, resolve } = deferred<void>();
+    const onCancel = vi.fn(() => promise);
+
+    renderToolbar(onCancel);
+
+    const button = screen.getByTestId("cancel-agent-button") as HTMLButtonElement;
+    expect(button.disabled).toBe(false);
+
+    fireEvent.click(button);
+    // Click is processed synchronously; React then flushes the setState that
+    // marks the button disabled. We assert post-flush.
+    await act(async () => {});
+    expect(button.disabled).toBe(true);
+    expect(onCancel).toHaveBeenCalledTimes(1);
+
+    // Subsequent clicks while the promise is pending must not call onCancel.
+    fireEvent.click(button);
+    fireEvent.click(button);
+    fireEvent.click(button);
+    expect(onCancel).toHaveBeenCalledTimes(1);
+
+    // Once the in-flight cancel resolves, the button re-enables for retry
+    // (rare, but possible if the first cancel returned an error).
+    await act(async () => {
+      resolve();
+      await promise;
+    });
+    expect(button.disabled).toBe(false);
+  });
+
+  it("re-enables the button if onCancel rejects", async () => {
+    const { promise, resolve } = deferred<void>();
+    const onCancel = vi.fn(() => promise.then(() => Promise.reject(new Error("network"))));
+
+    renderToolbar(onCancel);
+    const button = screen.getByTestId("cancel-agent-button") as HTMLButtonElement;
+
+    fireEvent.click(button);
+    await act(async () => {});
+    expect(button.disabled).toBe(true);
+
+    await act(async () => {
+      resolve();
+      // Allow the rejected promise to settle inside the click handler.
+      await new Promise((r) => setTimeout(r, 0));
+    });
+    expect(button.disabled).toBe(false);
+  });
+});
@@ -1,6 +1,6 @@
 "use client";

-import { memo, useRef, useState, type ReactNode } from "react";
+import { memo, useCallback, useRef, useState, type ReactNode } from "react";
 import {
  IconArrowUp,
  IconChevronsLeft,
@@ -46,7 +46,7 @@ export type ChatInputToolbarProps = {
  hasContent?: boolean;
  isDisabled: boolean;
  isSending: boolean;
-  onCancel: () => void;
+  onCancel: () => void | Promise<void>;
  onSubmit: () => void;
  submitKey?: "enter" | "cmd_enter";
  contextCount?: number;
@@ -79,7 +79,7 @@ type SubmitButtonProps = {
  isDisabled: boolean;
  isSending: boolean;
  planModeEnabled: boolean;
-  onCancel: () => void;
+  onCancel: () => void | Promise<void>;
  onSubmit: () => void;
  submitShortcut: (typeof SHORTCUTS)[keyof typeof SHORTCUTS];
 };
@@ -103,6 +103,32 @@ function SubmitButton({
  // When agent is busy and there's nothing to send, show only the cancel button.
  // When there's content to queue, show both cancel and send buttons side-by-side.
  const showSendButton = !isAgentBusy || hasContent;
+  // Track cancel-in-flight locally so impatient retries are blocked at the
+  // button itself: cancelling a long-running tool (e.g. Claude Monitor) can
+  // take several seconds, and without this the user clicks repeatedly and
+  // each click hits the backend.
+  const [isCancelling, setIsCancelling] = useState(false);
+  // Re-entrancy guard via ref: `disabled={isCancelling}` already blocks DOM
+  // clicks, but the ref makes the guard effective for any programmatic caller
+  // and keeps `isCancelling` out of the useCallback deps so the handler
+  // identity is stable across the false→true→false flips.
+  const cancellingRef = useRef(false);
+  const handleCancelClick = useCallback(async () => {
+    if (cancellingRef.current) return;
+    cancellingRef.current = true;
+    setIsCancelling(true);
+    try {
+      await onCancel();
+    } catch (error) {
+      // Parent handlers normally swallow errors, but a rejected onCancel
+      // must not propagate as an unhandled rejection — and the button must
+      // re-enable so the user can retry.
+      console.error("Failed to cancel agent turn:", error);
+    } finally {
+      cancellingRef.current = false;
+      setIsCancelling(false);
+    }
+  }, [onCancel]);
  return (
    <div className="flex items-center gap-1">
      {isAgentBusy && (
@@ -112,14 +138,19 @@ function SubmitButton({
              type="button"
              variant="secondary"
              size="icon"
-              className="h-7 w-7 rounded-full cursor-pointer bg-destructive/10 text-destructive hover:bg-destructive/20"
-              onClick={onCancel}
+              className="h-7 w-7 rounded-full cursor-pointer bg-destructive/10 text-destructive hover:bg-destructive/20 disabled:cursor-not-allowed disabled:opacity-70"
+              onClick={handleCancelClick}
+              disabled={isCancelling}
              data-testid="cancel-agent-button"
            >
-              <IconPlayerPauseFilled className="h-3.5 w-3.5" />
+              {isCancelling ? (
+                <GridSpinner className="text-destructive" />
+              ) : (
+                <IconPlayerPauseFilled className="h-3.5 w-3.5" />
+              )}
            </Button>
          </TooltipTrigger>
-          <TooltipContent>Cancel agent</TooltipContent>
+          <TooltipContent>{isCancelling ? "Cancelling..." : "Cancel agent"}</TooltipContent>
        </Tooltip>
      )}
      {showSendButton && (