From 7d96d942ccdb9ff53d2d9a4d77a06cc520ad3c60 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:17:20 +0000 Subject: [PATCH 1/4] Initial plan From 896682865731f024b15903a675f9147e3419874c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:32:13 +0000 Subject: [PATCH 2/4] fix: detach audit and audit-diff subprocess contexts from MCP gateway 60s deadline The MCP gateway imposes a per-tool RPC deadline of ~60 seconds. The logs tool already had a fix: using context.WithoutCancel to detach the subprocess from the gateway deadline, then applying only an explicit subprocess timeout. The audit and audit-diff tools were missing this fix, causing every call to fail with "context deadline exceeded" at exactly 60 seconds. Apply the same pattern to both tools: - Add defaultMCPAuditTimeoutMinutes (5m) and defaultMCPAuditDiffTimeoutMinutes (5m) constants - Detach subprocess contexts from gateway deadline via context.WithoutCancel - Forward only explicit client disconnects (context.Canceled), not deadline exceeded - Add regression tests TestAuditToolSubprocessContextIgnoresGatewayDeadline and TestAuditDiffToolSubprocessContextIgnoresGatewayDeadline Fixes github/gh-aw#49058 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_tools_privileged.go | 72 +++++++++++++++++++++++- pkg/cli/mcp_tools_privileged_test.go | 83 ++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 2 deletions(-) diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index bdbde22430b..b6a05427b08 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. It downloads artifacts for all referenced runs before + // computing the diff, so it needs more headroom than a single-run audit. + defaultMCPAuditDiffTimeoutMinutes = 5 ) // appendRepoFlagFromEnv appends "--repo " to args when GITHUB_REPOSITORY @@ -486,11 +496,44 @@ Multi-run diff returns JSON describing changes between the base and each compari notifyProgress(ctx, req, 0, 100, "Downloading audit artifacts...") + // The MCP gateway imposes a per-tool RPC deadline (typically 60 s) on the + // request context. exec.CommandContext ties the subprocess lifetime to that + // context, so the subprocess would be killed after 60 s even for legitimate + // long-running audits (large runs with many artifact sets can take longer). + // + // Fix: detach from the gateway deadline by stripping cancellation/deadline + // from ctx via context.WithoutCancel, then applying only the subprocess + // timeout. Context values (e.g. trace IDs) are preserved. We still forward + // any explicit cancellations from the MCP request context (e.g. client + // disconnect) so the subprocess is cleaned up promptly when the caller goes away. + subCtx, subCancel := context.WithTimeout( + context.WithoutCancel(ctx), + time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute, + ) + defer subCancel() + go func() { + defer func() { + if r := recover(); r != nil { + mcpLog.Printf("Panic in MCP audit context-watcher goroutine (recovered): %v", 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(): + } + }() + // 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 +662,32 @@ 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...) + // The MCP gateway imposes a per-tool RPC deadline (typically 60 s) on the + // request context. Detach from that deadline so the subprocess can run for + // its full allotted time without being killed at the gateway boundary. + subCtx, subCancel := context.WithTimeout( + context.WithoutCancel(ctx), + time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute, + ) + defer subCancel() + go func() { + defer func() { + if r := recover(); r != nil { + mcpLog.Printf("Panic in MCP audit-diff context-watcher goroutine (recovered): %v", r) + } + }() + // Only forward explicit cancellations (context.Canceled); do NOT propagate + // context.DeadlineExceeded from the MCP gateway. + select { + case <-ctx.Done(): + if ctx.Err() == context.Canceled { + subCancel() + } + case <-subCtx.Done(): + } + }() + + 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..ae22ff413ac 100644 --- a/pkg/cli/mcp_tools_privileged_test.go +++ b/pkg/cli/mcp_tools_privileged_test.go @@ -983,3 +983,86 @@ 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. +func TestAuditToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) { + var capturedDeadline time.Time + var capturedHasDeadline bool + + mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { + capturedDeadline, capturedHasDeadline = ctx.Deadline() + return exec.CommandContext(ctx, "sh", "-c", `printf '%s' "$1"`, "sh", `{"overview":{"run_id":"1234567890"}}`) + } + + server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) + err := registerAuditTool(server, mockExecCmd, "", false) + require.NoError(t, err, "registerAuditTool should succeed") + + session := connectInMemory(t, server) + + before := time.Now() + + // Simulate the MCP gateway's short per-tool RPC deadline (2 s). + gatewayCtx, gatewayCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer gatewayCancel() + + _, _ = session.CallTool(gatewayCtx, &mcp.CallToolParams{ + Name: "audit", + Arguments: map[string]any{"run_id": "1234567890"}, + }) + + require.True(t, capturedHasDeadline, "subprocess context should have a deadline") + + // The subprocess deadline must be at least defaultMCPAuditTimeoutMinutes out, + // not bounded by the 2-second gateway context. + expectedMinDeadline := before.Add(time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute - 5*time.Second) + assert.True(t, capturedDeadline.After(expectedMinDeadline), + "subprocess context deadline (%v) should be ≥ %d minutes from call start (%v) regardless of the 2s gateway deadline; got %v from start", + capturedDeadline, defaultMCPAuditTimeoutMinutes, before, capturedDeadline.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. +func TestAuditDiffToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) { + var capturedDeadline time.Time + var capturedHasDeadline bool + + mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { + capturedDeadline, capturedHasDeadline = ctx.Deadline() + return exec.CommandContext(ctx, "sh", "-c", `printf '%s' "$1"`, "sh", `[{"base_run_id":100,"compare_run_id":200}]`) + } + + server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) + err := registerAuditDiffTool(server, mockExecCmd, "", false) + require.NoError(t, err, "registerAuditDiffTool should succeed") + + session := connectInMemory(t, server) + + before := time.Now() + + // Simulate the MCP gateway's short per-tool RPC deadline (2 s). + gatewayCtx, gatewayCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer gatewayCancel() + + _, _ = session.CallTool(gatewayCtx, &mcp.CallToolParams{ + Name: "audit-diff", + Arguments: map[string]any{ + "base_run_id": "100", + "compare_run_ids": []string{"200"}, + }, + }) + + require.True(t, capturedHasDeadline, "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, capturedDeadline.After(expectedMinDeadline), + "subprocess context deadline (%v) should be ≥ %d minutes from call start (%v) regardless of the 2s gateway deadline; got %v from start", + capturedDeadline, defaultMCPAuditDiffTimeoutMinutes, before, capturedDeadline.Sub(before)) +} From fb6ab4956405c19a8b3bdcb8b3f35e5d8a5e8ee8 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 06:42:53 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-49061 for detaching audit subprocess context from MCP gateway deadline --- ...ocess-context-from-mcp-gateway-deadline.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/49061-detach-audit-subprocess-context-from-mcp-gateway-deadline.md 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.* From 2fd9d5209320e051ba6028ecdadc577747243c5c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:17:56 +0000 Subject: [PATCH 4/4] fix: extract newMCPSubprocessContext helper and fix subprocess context tests - Fix misleading comment on defaultMCPAuditDiffTimeoutMinutes (claimed more headroom than single-run audit; both values are equal at 5 min) - Extract newMCPSubprocessContext helper encapsulating the context.WithoutCancel + context.WithTimeout pattern with the cancellation-forwarding goroutine - Refactor audit and audit-diff handlers to use the helper (DRY) - Rewrite TestAuditToolSubprocessContextIgnoresGatewayDeadline and TestAuditDiffToolSubprocessContextIgnoresGatewayDeadline to call newMCPSubprocessContext directly with a deadline-bearing context, so the test actually exercises the detachment logic (the previous implementation went through session.CallTool which does not propagate the client's context deadline to the server handler) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/mcp_tools_privileged.go | 99 ++++++++++++---------------- pkg/cli/mcp_tools_privileged_test.go | 81 +++++++++-------------- 2 files changed, 73 insertions(+), 107 deletions(-) diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index b6a05427b08..fe4570a6923 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/pkg/cli/mcp_tools_privileged.go @@ -39,8 +39,8 @@ const ( // bounding the subprocess lifetime. defaultMCPAuditTimeoutMinutes = 5 // defaultMCPAuditDiffTimeoutMinutes is the default subprocess timeout for the - // audit-diff tool. It downloads artifacts for all referenced runs before - // computing the diff, so it needs more headroom than a single-run audit. + // audit-diff tool. 5 minutes gives ample headroom for the artifact-download + // and diff steps while still bounding the subprocess lifetime. defaultMCPAuditDiffTimeoutMinutes = 5 ) @@ -57,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)"` @@ -496,38 +533,9 @@ Multi-run diff returns JSON describing changes between the base and each compari notifyProgress(ctx, req, 0, 100, "Downloading audit artifacts...") - // The MCP gateway imposes a per-tool RPC deadline (typically 60 s) on the - // request context. exec.CommandContext ties the subprocess lifetime to that - // context, so the subprocess would be killed after 60 s even for legitimate - // long-running audits (large runs with many artifact sets can take longer). - // - // Fix: detach from the gateway deadline by stripping cancellation/deadline - // from ctx via context.WithoutCancel, then applying only the subprocess - // timeout. Context values (e.g. trace IDs) are preserved. We still forward - // any explicit cancellations from the MCP request context (e.g. client - // disconnect) so the subprocess is cleaned up promptly when the caller goes away. - subCtx, subCancel := context.WithTimeout( - context.WithoutCancel(ctx), - time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute, - ) + // Detach from the gateway's per-tool RPC deadline; see newMCPSubprocessContext. + subCtx, subCancel := newMCPSubprocessContext(ctx, time.Duration(defaultMCPAuditTimeoutMinutes)*time.Minute, "audit") defer subCancel() - go func() { - defer func() { - if r := recover(); r != nil { - mcpLog.Printf("Panic in MCP audit context-watcher goroutine (recovered): %v", 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(): - } - }() // Execute the CLI command. // Use separate stdout/stderr capture instead of CombinedOutput because: @@ -662,30 +670,9 @@ Returns JSON describing the differences between the base run and each comparison notifyProgress(ctx, req, 0, 100, "Downloading artifacts for diff...") - // The MCP gateway imposes a per-tool RPC deadline (typically 60 s) on the - // request context. Detach from that deadline so the subprocess can run for - // its full allotted time without being killed at the gateway boundary. - subCtx, subCancel := context.WithTimeout( - context.WithoutCancel(ctx), - time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute, - ) + // Detach from the gateway's per-tool RPC deadline; see newMCPSubprocessContext. + subCtx, subCancel := newMCPSubprocessContext(ctx, time.Duration(defaultMCPAuditDiffTimeoutMinutes)*time.Minute, "audit-diff") defer subCancel() - go func() { - defer func() { - if r := recover(); r != nil { - mcpLog.Printf("Panic in MCP audit-diff context-watcher goroutine (recovered): %v", r) - } - }() - // Only forward explicit cancellations (context.Canceled); do NOT propagate - // context.DeadlineExceeded from the MCP gateway. - select { - case <-ctx.Done(): - if ctx.Err() == context.Canceled { - subCancel() - } - case <-subCtx.Done(): - } - }() stdout, err := runMCPExecOutput(subCtx, execCmd, cmdArgs...) outputStr := string(stdout) diff --git a/pkg/cli/mcp_tools_privileged_test.go b/pkg/cli/mcp_tools_privileged_test.go index ae22ff413ac..e2024bed7ac 100644 --- a/pkg/cli/mcp_tools_privileged_test.go +++ b/pkg/cli/mcp_tools_privileged_test.go @@ -989,80 +989,59 @@ func TestLogsToolSubprocessContextIgnoresGatewayDeadline(t *testing.T) { // 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) { - var capturedDeadline time.Time - var capturedHasDeadline bool - - mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { - capturedDeadline, capturedHasDeadline = ctx.Deadline() - return exec.CommandContext(ctx, "sh", "-c", `printf '%s' "$1"`, "sh", `{"overview":{"run_id":"1234567890"}}`) - } - - server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) - err := registerAuditTool(server, mockExecCmd, "", false) - require.NoError(t, err, "registerAuditTool should succeed") - - session := connectInMemory(t, server) - - before := time.Now() - - // Simulate the MCP gateway's short per-tool RPC deadline (2 s). + // 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() - _, _ = session.CallTool(gatewayCtx, &mcp.CallToolParams{ - Name: "audit", - Arguments: map[string]any{"run_id": "1234567890"}, - }) + before := time.Now() - require.True(t, capturedHasDeadline, "subprocess context should have a deadline") + 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. + // 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, capturedDeadline.After(expectedMinDeadline), + 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", - capturedDeadline, defaultMCPAuditTimeoutMinutes, before, capturedDeadline.Sub(before)) + 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) { - var capturedDeadline time.Time - var capturedHasDeadline bool - - mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { - capturedDeadline, capturedHasDeadline = ctx.Deadline() - return exec.CommandContext(ctx, "sh", "-c", `printf '%s' "$1"`, "sh", `[{"base_run_id":100,"compare_run_id":200}]`) - } - - server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) - err := registerAuditDiffTool(server, mockExecCmd, "", false) - require.NoError(t, err, "registerAuditDiffTool should succeed") - - session := connectInMemory(t, server) - - before := time.Now() - // Simulate the MCP gateway's short per-tool RPC deadline (2 s). gatewayCtx, gatewayCancel := context.WithTimeout(context.Background(), 2*time.Second) defer gatewayCancel() - _, _ = session.CallTool(gatewayCtx, &mcp.CallToolParams{ - Name: "audit-diff", - Arguments: map[string]any{ - "base_run_id": "100", - "compare_run_ids": []string{"200"}, - }, - }) + before := time.Now() - require.True(t, capturedHasDeadline, "subprocess context should have a deadline") + 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, capturedDeadline.After(expectedMinDeadline), + 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", - capturedDeadline, defaultMCPAuditDiffTimeoutMinutes, before, capturedDeadline.Sub(before)) + deadline, defaultMCPAuditDiffTimeoutMinutes, before, deadline.Sub(before)) }