diff --git a/docs/adr/49061-detach-audit-subprocess-context-from-mcp-gateway-deadline.md b/docs/adr/49061-detach-audit-subprocess-context-from-mcp-gateway-deadline.md new file mode 100644 index 00000000000..3179d90616b --- /dev/null +++ b/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.* diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index bdbde22430b..fe4570a6923 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/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 " 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 { diff --git a/pkg/cli/mcp_tools_privileged_test.go b/pkg/cli/mcp_tools_privileged_test.go index daaafdd58dc..e2024bed7ac 100644 --- a/pkg/cli/mcp_tools_privileged_test.go +++ b/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)) +}