diff --git a/apps/backend/internal/orchestrator/event_handlers_test.go b/apps/backend/internal/orchestrator/event_handlers_test.go index 42af6868f5..dac74c0161 100644 --- a/apps/backend/internal/orchestrator/event_handlers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_test.go @@ -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 } diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 3e2b59c3d1..3d00cbd9b7 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -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 diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 7d009c1954..557490950c 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -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 { diff --git a/apps/backend/internal/orchestrator/task_operations_test.go b/apps/backend/internal/orchestrator/task_operations_test.go index eae3b71401..6d4730787b 100644 --- a/apps/backend/internal/orchestrator/task_operations_test.go +++ b/apps/backend/internal/orchestrator/task_operations_test.go @@ -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) { diff --git a/apps/web/components/task/chat/chat-input-toolbar.test.tsx b/apps/web/components/task/chat/chat-input-toolbar.test.tsx new file mode 100644 index 0000000000..0a9a0d1c61 --- /dev/null +++ b/apps/web/components/task/chat/chat-input-toolbar.test.tsx @@ -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() { + let resolve!: (v: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +function renderToolbar(onCancel: () => void | Promise) { + return render( + {}} + 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(); + 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(); + 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); + }); +}); diff --git a/apps/web/components/task/chat/chat-input-toolbar.tsx b/apps/web/components/task/chat/chat-input-toolbar.tsx index a0bb99dd28..37472f16ea 100644 --- a/apps/web/components/task/chat/chat-input-toolbar.tsx +++ b/apps/web/components/task/chat/chat-input-toolbar.tsx @@ -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; 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; 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 (
{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" > - + {isCancelling ? ( + + ) : ( + + )} - Cancel agent + {isCancelling ? "Cancelling..." : "Cancel agent"} )} {showSendButton && (