diff --git a/server/pkg/agent/claude.go b/server/pkg/agent/claude.go index 80a341a681..a94ca1679a 100644 --- a/server/pkg/agent/claude.go +++ b/server/pkg/agent/claude.go @@ -12,9 +12,23 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" + "syscall" "time" ) +// claudeTerminateGraceNanos optionally overrides, in nanoseconds, how long a +// cancelled claude process group is given to exit after SIGTERM before it is +// SIGKILLed. Set via atomic store in tests; zero keeps the default. +var claudeTerminateGraceNanos atomic.Int64 + +func claudeTerminateGrace() time.Duration { + if n := claudeTerminateGraceNanos.Load(); n > 0 { + return time.Duration(n) + } + return 5 * time.Second +} + // claudeBackend implements Backend by spawning the Claude Code CLI // with --output-format stream-json. type claudeBackend struct { @@ -59,6 +73,21 @@ func (b *claudeBackend) Execute(ctx context.Context, prompt string, opts ExecOpt cmd := exec.CommandContext(runCtx, execPath, args...) hideAgentWindow(cmd) + // Run claude in its own process group so cancellation can reach the whole + // tree — the claude CLI plus the MCP servers and tool subprocesses it + // spawns — not just the direct child. The default CommandContext behaviour + // SIGKILLs only the leader, which orphans those descendants; on a resumed + // stream-json session with no wall-clock timeout they then keep running and + // burning model budget long after the task was cancelled, and under + // --max-concurrent-tasks 1 starve every queued task (#5918). This mirrors + // the fix already made for codex (#4520) and opencode (#4533). + configureProcessGroup(cmd) + // Take over context cancellation: the default would SIGKILL only the leader + // the instant runCtx is done. We instead drive a graceful group-wide + // SIGTERM→SIGKILL from the cancellation goroutine below and close stdout + // only after the tree has been signalled. Returning nil keeps os/exec from + // racing us with its own kill; WaitDelay remains the hard backstop. + cmd.Cancel = func() error { return nil } b.cfg.Logger.Info("agent command", "exec", execPath, "args", args) cmd.WaitDelay = 10 * time.Second if opts.Cwd != "" { @@ -104,6 +133,10 @@ func (b *claudeBackend) Execute(ctx context.Context, prompt string, opts ExecOpt msgCh := make(chan Message, 256) resCh := make(chan Result, 1) + // procDone closes once cmd.Wait() returns, letting the cancellation handler + // skip a process that already exited and avoid signalling a dead/reused pid. + procDone := make(chan struct{}) + // writeClaudeInput runs in its own goroutine so it cannot deadlock // against the stdout reader. With --verbose --output-format stream-json // the CLI emits a startup banner before reading its first stdin frame; @@ -148,10 +181,35 @@ func (b *claudeBackend) Execute(ctx context.Context, prompt string, opts ExecOpt assistantEventCount := 0 toolUseCount := 0 - // Close stdout when the context is cancelled so scanner.Scan() unblocks. + // On cancellation / timeout, terminate claude (and every MCP server and + // tool subprocess it spawned) BEFORE unblocking the scanner. EOF stdin + // to nudge a clean exit, then SIGTERM the whole process group, give it a + // grace period, and SIGKILL the group if any member is still alive. + // SIGKILL is uncatchable, so once delivered no group member can write + // again — only then is it safe to close the stdout read end as a + // last-resort unblock for a scanner a wedged descendant still keeps + // open. WaitDelay is the final backstop (#5918). go func() { - <-runCtx.Done() + select { + case <-procDone: + return // finished on its own; nothing to terminate + case <-runCtx.Done(): + } closeStdin() + if cmd.Process != nil { + signalProcessGroup(cmd.Process, syscall.SIGTERM) + // Escalate to a group SIGKILL unless the WHOLE process group has + // exited within the grace window. This must key off the process + // group, not procDone: procDone only means cmd.Wait() returned + // for the leader, so a SIGTERM-ignoring descendant that does not + // hold claude's stdout would let the leader exit, close procDone, + // and skip the SIGKILL — leaking exactly the orphan this fix + // targets. waitProcessGroupGone returns as soon as the group is + // empty, so the graceful case adds no latency. + if !waitProcessGroupGone(cmd.Process, claudeTerminateGrace()) { + signalProcessGroup(cmd.Process, syscall.SIGKILL) + } + } _ = stdout.Close() }() @@ -223,8 +281,9 @@ func (b *claudeBackend) Execute(ctx context.Context, prompt string, opts ExecOpt closeStdin() - // Wait for process exit + // Wait for process exit, then release the cancellation handler. exitErr := cmd.Wait() + close(procDone) duration := time.Since(startTime) // writeDone is buffered (cap 1) and the writer always sends — by the // time cmd has exited, the prompt write has either succeeded, hit a diff --git a/server/pkg/agent/claude_cancel_unix_test.go b/server/pkg/agent/claude_cancel_unix_test.go new file mode 100644 index 0000000000..2ea076de3d --- /dev/null +++ b/server/pkg/agent/claude_cancel_unix_test.go @@ -0,0 +1,146 @@ +//go:build unix + +package agent + +import ( + "context" + "log/slog" + "path/filepath" + "testing" + "time" +) + +// claudeCancelFakeScript returns a POSIX-sh script that impersonates a +// long-running `claude` in stream-json mode: it spawns a background grandchild +// (standing in for an MCP server or tool subprocess), records both its own +// (process-group-leader) pid and the grandchild pid, emits a stream-json system +// init line, then streams stdout in a tight loop forever. This is the shape +// that orphaned a descendant for 64+ minutes in #5918 when the daemon only +// killed the leader. When ignoreTerm is true the whole group ignores SIGTERM, +// forcing the SIGKILL escalation path. +func claudeCancelFakeScript(ignoreTerm bool) string { + trap := "trap 'exit 0' TERM\n" + if ignoreTerm { + trap = "trap '' TERM\n" + } + return "#!/bin/sh\n" + trap + + `# Background grandchild so the test can assert the *whole* group is +# terminated on cancellation, not just the direct child. +( sleep 300 ) & +child=$! +if [ -n "$CLAUDE_PID_FILE" ]; then + printf '%s %s\n' "$$" "$child" > "$CLAUDE_PID_FILE" +fi +printf '{"type":"system","session_id":"ses_fake"}\n' +while true; do + printf '{"type":"system","session_id":"ses_fake"}\n' + sleep 0.1 +done +` +} + +// claudeMixedSignalFakeScript returns a fake `claude` whose leader RESPECTS +// SIGTERM (so it exits and cmd.Wait() returns the instant the group is +// signalled) while a background grandchild IGNORES SIGTERM and detaches its +// stdio, so it holds neither the leader alive nor claude's stdout pipe. This is +// the mixed case that leaks when escalation keys off the leader's exit +// (procDone) instead of the whole process group: the leader is reaped, procDone +// closes, the SIGKILL is skipped, and the SIGTERM-resistant descendant survives +// — the exact #5918 orphan. +func claudeMixedSignalFakeScript() string { + return "#!/bin/sh\n" + "trap 'exit 0' TERM\n" + + `# Grandchild ignores TERM and redirects its stdio away from the pipe so +# it does not keep claude's stdout open after the leader exits. +( trap '' TERM; sleep 300 ) /dev/null 2>&1 & +child=$! +if [ -n "$CLAUDE_PID_FILE" ]; then + printf '%s %s\n' "$$" "$child" > "$CLAUDE_PID_FILE" +fi +printf '{"type":"system","session_id":"ses_fake"}\n' +while true; do + printf '{"type":"system","session_id":"ses_fake"}\n' + sleep 0.1 +done +` +} + +// TestClaudeCancellationTerminatesProcessGroupGraceful verifies that cancelling +// a run terminates a SIGTERM-respecting claude and its whole process group, +// returns an "aborted" result without hanging, and leaves no orphaned +// descendant. +func TestClaudeCancellationTerminatesProcessGroupGraceful(t *testing.T) { + runClaudeCancellationTest(t, claudeCancelFakeScript(false)) +} + +// TestClaudeCancellationEscalatesToSIGKILL verifies the worst case from #5918: +// claude (and the descendants it spawned) ignore SIGTERM and keep running. +// Cancellation must escalate to a group SIGKILL, still return promptly, and +// still reap the whole group — without deadlocking on the stdout scanner or +// closing the pipe under a live writer. +func TestClaudeCancellationEscalatesToSIGKILL(t *testing.T) { + claudeTerminateGraceNanos.Store(int64(300 * time.Millisecond)) + t.Cleanup(func() { claudeTerminateGraceNanos.Store(0) }) + runClaudeCancellationTest(t, claudeCancelFakeScript(true)) +} + +// TestClaudeCancellationEscalatesWhenDescendantIgnoresTERM is the mixed-signal +// regression for #5918: a SIGTERM-respecting leader plus a SIGTERM-ignoring, +// stdio-detached descendant. Cancellation must still reap the descendant, which +// only holds when the SIGKILL escalation is gated on the whole process group +// (not the leader's exit). This fails against the leader-keyed escalation. +func TestClaudeCancellationEscalatesWhenDescendantIgnoresTERM(t *testing.T) { + claudeTerminateGraceNanos.Store(int64(300 * time.Millisecond)) + t.Cleanup(func() { claudeTerminateGraceNanos.Store(0) }) + runClaudeCancellationTest(t, claudeMixedSignalFakeScript()) +} + +func runClaudeCancellationTest(t *testing.T, script string) { + t.Helper() + + tempDir := t.TempDir() + pidFile := filepath.Join(tempDir, "pids") + fakePath := filepath.Join(tempDir, "claude") + writeTestExecutable(t, fakePath, []byte(script)) + + backend, err := New("claude", Config{ + ExecutablePath: fakePath, + Logger: slog.Default(), + Env: map[string]string{"CLAUDE_PID_FILE": pidFile}, + }) + if err != nil { + t.Fatalf("new claude backend: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{Cwd: tempDir}) + if err != nil { + t.Fatalf("execute: %v", err) + } + + // Drain streamed messages so the reader never blocks on a full channel. + go func() { + for range session.Messages { + } + }() + + pids := waitForPids(t, pidFile) + + cancel() // user cancels the task + + select { + case res := <-session.Result: + if res.Status != "aborted" { + t.Errorf("status = %q, want aborted", res.Status) + } + case <-time.After(10 * time.Second): + t.Fatal("Execute did not return after cancellation (possible scanner deadlock or unkilled process)") + } + + // The leader and the grandchild must both be gone — cancellation reaped the + // whole group, leaving no orphan spinning. + for _, pid := range pids { + waitProcessGone(t, pid) + } +}