fix: detach audit/audit-diff subprocess contexts from MCP gateway 60s deadline

docs/adr/49061-detach-audit-subprocess-context-from-mcp-gateway-deadline.md

@@ -0,0 +1,44 @@
+# ADR-49061: Detach Audit/Audit-Diff Subprocess Contexts from MCP Gateway Deadline
+
+**Date**: 2026-07-30
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+The MCP gateway imposes a hard per-tool RPC deadline (typically 60 seconds) on every inbound request context. The `audit` and `audit-diff` MCP tools passed this context directly to `exec.CommandContext`, which binds the subprocess lifetime to the gateway's deadline. As a result, every `audit` and `audit-diff` call failed with `context deadline exceeded` at exactly ~60 seconds, regardless of whether the actual workload could complete in time. The `logs` tool had already been fixed with the correct detach-and-own-timeout pattern; `audit` and `audit-diff` were inconsistently missing it.
+
+### Decision
+
+We will detach the `audit` and `audit-diff` subprocess contexts from the MCP gateway's RPC deadline by applying `context.WithoutCancel(ctx)` before creating a tool-owned `context.WithTimeout`. Context values (e.g. trace IDs) are preserved. A goroutine watcher selectively forwards only `context.Canceled` (explicit client disconnect) to the subprocess context — it never forwards `context.DeadlineExceeded` from the gateway, ensuring the subprocess can run for its full allotted time. This is identical to the pattern already used for the `logs` tool.
+
+### Alternatives Considered
+
+#### Alternative 1: Increase the MCP Gateway's RPC Deadline
+
+Raise the gateway's per-tool timeout to 5+ minutes for all tools. This would prevent the 60 s kill but would require changes to gateway infrastructure/configuration outside this codebase. It also applies a blanket increase to all tools, including fast ones, where a 60 s deadline is a reasonable guard against runaway subprocesses.
+
+#### Alternative 2: Redesign Tools for Incremental/Paginated Output
+
+Break audit operations into smaller, faster incremental requests that each complete within the 60 s window. This would be a deeper protocol redesign with higher implementation complexity and would require changes on both the tool and client sides. It does not address the root cause of the subprocess-context coupling.
+
+### Consequences
+
+#### Positive
+- `audit` and `audit-diff` tools no longer fail with `context deadline exceeded` after 60 seconds; legitimate long-running audits can complete within their 5-minute subprocess timeout.
+- Client disconnects (explicit `context.Canceled`) still propagate promptly to clean up subprocesses, preventing orphaned processes.
+- The pattern is now consistent across all three long-running privileged tools (`logs`, `audit`, `audit-diff`).
+
+#### Negative
+- A subprocess that has been detached from the gateway context can continue running for up to 5 minutes even if the gateway drops the connection for a reason other than explicit client cancellation (e.g. gateway restart, proxy timeout). Only `context.Canceled` is forwarded; `context.DeadlineExceeded` is intentionally suppressed.
+- The 5-minute timeout constants (`defaultMCPAuditTimeoutMinutes`, `defaultMCPAuditDiffTimeoutMinutes`) are both set to the same value; if operational experience shows `audit-diff` needs more headroom than `audit` (it downloads artifacts for multiple runs), both constants must be updated independently.
+
+#### Neutral
+- Two goroutine watchers are introduced (one per tool), which is the same pattern already in use for `logs`. Each goroutine is bounded in lifetime by the subprocess context.
+- Regression tests modeled on the existing `TestLogsToolSubprocessContextIgnoresGatewayDeadline` are added for both new tools.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/mcp_tools_privileged.go

@@ -32,6 +32,16 @@ const (
 	// single long-running request could otherwise block all callers for an arbitrarily
 	// long time.
 	maxMCPLogsSubprocessTimeoutMinutes = 60
+
+	// defaultMCPAuditTimeoutMinutes is the default subprocess timeout for the audit
+	// tool.  Auditing a single run typically takes 5–30 s, but large runs with many
+	// artifact sets can take longer.  5 minutes gives ample headroom while still
+	// bounding the subprocess lifetime.
+	defaultMCPAuditTimeoutMinutes = 5
+	// defaultMCPAuditDiffTimeoutMinutes is the default subprocess timeout for the
+	// audit-diff tool.  5 minutes gives ample headroom for the artifact-download
+	// and diff steps while still bounding the subprocess lifetime.
+	defaultMCPAuditDiffTimeoutMinutes = 5
 )
 
 // appendRepoFlagFromEnv appends "--repo <owner/repo>" to args when GITHUB_REPOSITORY
@@ -47,6 +57,43 @@ func appendRepoFlagFromEnv(args []string) []string {
 	return args
 }
 
+// newMCPSubprocessContext creates a subprocess context that is detached from the
+// MCP gateway's per-request deadline.  The gateway imposes a short RPC deadline
+// (typically 60 s) on the request context; passing that context directly to
+// exec.CommandContext would kill long-running subprocesses prematurely.
+//
+// The returned context is rooted at context.Background() (values preserved,
+// gateway deadline stripped) and carries only the caller-specified timeout.
+// A goroutine is started to forward explicit client cancellations
+// (context.Canceled) to the subprocess while ignoring DeadlineExceeded from
+// the gateway.
+//
+// toolName is used solely in the panic-recovery log message.
+func newMCPSubprocessContext(ctx context.Context, timeout time.Duration, toolName string) (context.Context, context.CancelFunc) {
+	subCtx, subCancel := context.WithTimeout(
+		context.WithoutCancel(ctx),
+		timeout,
+	)
+	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				mcpLog.Printf("Panic in MCP %s context-watcher goroutine (recovered): %v", toolName, r)
+			}
+		}()
+		// Only forward explicit cancellations (context.Canceled); do NOT propagate
+		// context.DeadlineExceeded from the MCP gateway — that would kill the subprocess
+		// at the gateway's 60 s RPC deadline and defeat the purpose of this fix.
+		select {
+		case <-ctx.Done():
+			if ctx.Err() == context.Canceled {
+				subCancel() // propagate client disconnect to subprocess
+			}
+		case <-subCtx.Done(): // subprocess timed out or caller already cancelled
+		}
+	}()
+	return subCtx, subCancel
+}
+
 // logsArgs holds the input parameters for the logs tool.
 type logsArgs struct {
 	WorkflowName      string   `json:"workflow_name,omitempty" jsonschema:"Name of the workflow to download logs for (empty for all)"`
@@ -486,11 +533,15 @@ Multi-run diff returns JSON describing changes between the base and each compari
 
 		notifyProgress(ctx, req, 0, 100, "Downloading audit artifacts...")
 
+		// Detach from the gateway's per-tool RPC deadline; see newMCPSubprocessContext.
+		subCtx, subCancel := newMCPSubprocessContext(ctx, time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute, "audit")
+		defer subCancel()
+
 		// Execute the CLI command.
 		// Use separate stdout/stderr capture instead of CombinedOutput because:
 		// - Stdout contains JSON output (--json flag)
 		// - Stderr contains console messages and debug logs that shouldn't be mixed with JSON
-		stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...)
+		stdout, err := runMCPExecOutput(subCtx, execCmd, cmdArgs...)
 
 		// The audit command outputs JSON to stdout when --json flag is used.
 		// If the command fails, we need to provide detailed error information.
@@ -619,7 +670,11 @@ Returns JSON describing the differences between the base run and each comparison
 
 		notifyProgress(ctx, req, 0, 100, "Downloading artifacts for diff...")
 
-		stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...)
+		// Detach from the gateway's per-tool RPC deadline; see newMCPSubprocessContext.
+		subCtx, subCancel := newMCPSubprocessContext(ctx, time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute, "audit-diff")
+		defer subCancel()
+
+		stdout, err := runMCPExecOutput(subCtx, execCmd, cmdArgs...)
 		outputStr := string(stdout)
 
 		if err != nil {

pkg/cli/mcp_tools_privileged_test.go

@@ -983,3 +983,65 @@ func TestLogsToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) {
 		"subprocess context deadline (%v) should be ≥ %d minutes from call start (%v) regardless of the 2s gateway deadline; got %v from start",
 		capturedDeadline, requestedTimeoutMinutes, before, capturedDeadline.Sub(before))
 }
+
+// TestAuditToolSubprocessContextIgnoresGatewayDeadline verifies that the audit
+// tool creates a subprocess context rooted at context.Background() so the
+// subprocess deadline is independent of the MCP gateway's 60 s per-request
+// deadline. This is the regression test for the bug where a 60 s gateway
+// deadline caused context deadline exceeded on every audit call.
+//
+// The test calls newMCPSubprocessContext directly with a deadline-bearing
+// context so it is not affected by the MCP in-memory transport's context
+// isolation, which strips deadlines from server handler contexts.
+func TestAuditToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) {
+	// Simulate the MCP gateway's short per-tool RPC deadline (2 s) by passing a
+	// deadline-bearing context directly to the detachment helper — the same
+	// context the handler receives in production.
+	gatewayCtx, gatewayCancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer gatewayCancel()
+
+	before := time.Now()
+
+	subCtx, subCancel := newMCPSubprocessContext(gatewayCtx, time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute, "audit")
+	defer subCancel()
+
+	deadline, ok := subCtx.Deadline()
+	require.True(t, ok, "subprocess context should have a deadline")
+
+	// The subprocess deadline must be at least defaultMCPAuditTimeoutMinutes out,
+	// not bounded by the 2-second gateway context.  This assertion would fail if
+	// newMCPSubprocessContext were changed to context.WithTimeout(ctx, ...) without
+	// the context.WithoutCancel detachment step.
+	expectedMinDeadline := before.Add(time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute - 5*time.Second)
+	assert.True(t, deadline.After(expectedMinDeadline),
+		"subprocess context deadline (%v) should be ≥ %d minutes from call start (%v) regardless of the 2s gateway deadline; got %v from start",
+		deadline, defaultMCPAuditTimeoutMinutes, before, deadline.Sub(before))
+}
+
+// TestAuditDiffToolSubprocessContextIgnoresGatewayDeadline verifies that the
+// audit-diff tool creates a subprocess context rooted at context.Background()
+// so the subprocess deadline is independent of the MCP gateway's 60 s
+// per-request deadline.
+//
+// The test calls newMCPSubprocessContext directly with a deadline-bearing
+// context so it is not affected by the MCP in-memory transport's context
+// isolation, which strips deadlines from server handler contexts.
+func TestAuditDiffToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) {
+	// Simulate the MCP gateway's short per-tool RPC deadline (2 s).
+	gatewayCtx, gatewayCancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer gatewayCancel()
+
+	before := time.Now()
+
+	subCtx, subCancel := newMCPSubprocessContext(gatewayCtx, time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute, "audit-diff")
+	defer subCancel()
+
+	deadline, ok := subCtx.Deadline()
+	require.True(t, ok, "subprocess context should have a deadline")
+
+	// The subprocess deadline must be at least defaultMCPAuditDiffTimeoutMinutes out.
+	expectedMinDeadline := before.Add(time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute - 5*time.Second)
+	assert.True(t, deadline.After(expectedMinDeadline),
+		"subprocess context deadline (%v) should be ≥ %d minutes from call start (%v) regardless of the 2s gateway deadline; got %v from start",
+		deadline, defaultMCPAuditDiffTimeoutMinutes, before, deadline.Sub(before))
+}