fix: detach audit/audit-diff subprocess contexts from MCP gateway 60s deadline - #49061
Conversation
… 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 #49058 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Detaches audit subprocesses from the MCP gateway deadline while retaining bounded execution.
Changes:
- Adds five-minute audit and audit-diff subprocess timeouts.
- Forwards explicit cancellation while ignoring gateway deadline expiry.
- Adds regression tests for subprocess deadlines.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Adds detached, bounded subprocess contexts. |
pkg/cli/mcp_tools_privileged_test.go |
Adds audit deadline regression tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/mcp_tools_privileged_test.go:1053
- As above, the deadline passed to
ClientSession.CallToolis client-side and is not inherited by the server handler across the JSON-RPC transport; this immediate command therefore never tests detachment from a deadline-bearing handler context. The test would pass withcontext.WithTimeout(ctx, ...), so it does not guard the regression it names. Test the context-construction logic with a context that directly has the short deadline, or inject that deadline server-side and use a blocking subprocess.
// 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{
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| // 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 |
| // 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{ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly mirrors the existing logs tool pattern — context.WithoutCancel + context.WithTimeout with a goroutine watcher that forwards only context.Canceled, not DeadlineExceeded. Tests are analogous to the existing regression test.
One non-blocking note: the comment on defaultMCPAuditDiffTimeoutMinutes says it "needs more headroom than a single-run audit" but the value is the same as defaultMCPAuditTimeoutMinutes (both 5). The comment is slightly misleading.
Overall the change is correct, well-tested, and safe to merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.9 AIC · ⌖ 6.01 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (2 tests)
Quality DetailsPattern Consistency: Both tests follow the established pattern from Regression Coverage: These tests directly verify the bug fix by:
Design Invariants Enforced:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: The fix correctly detaches subprocess contexts from the MCP gateway's RPC deadline using the proven
context.WithoutCancel + context.WithTimeoutpattern already established by thelogstool. - Regression tests present: Both new tests follow the same structure as
TestLogsToolSubprocessContextIgnoresGatewayDeadline, ensuring the 60s failure mode cannot regress silently. - One minor doc/value inconsistency:
defaultMCPAuditDiffTimeoutMinutesis documented as needing more headroom thanauditbut is set to the same value — see inline comment.
Positive Highlights
- ✅ Goroutine watcher correctly forwards only
context.Canceled, nevercontext.DeadlineExceeded - ✅ Panic recovery in goroutines matches the existing
logspattern - ✅ Tests use a simulated 2 s gateway deadline to validate independence from the real 60 s deadline
- ✅ Clear inline comments explain why each context transform is needed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.9 AIC · ⌖ 4.93 AIC · ⊞ 7K
Comment /matt to run again
| // 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 |
There was a problem hiding this comment.
[/diagnosing-bugs] The comment says audit-diff needs more headroom than audit, but both constants are 5. Either align the comment or raise the value.
💡 Suggested fix
Either update the comment to remove the "more headroom" claim, or raise the constant to reflect the intent:
// audit-diff downloads artifacts for all referenced runs before computing the diff,
// so it needs more headroom than a single-run audit.
defaultMCPAuditDiffTimeoutMinutes = 10Mismatched comments and values erode trust in constants over time.
@copilot please address this.
… from MCP gateway deadline
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (153 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Verdict: fix is correct but the underlying timeout scaling is incomplete
The core fix (detaching subprocess context from the gateway RPC deadline via WithoutCancel+WithTimeout, forwarding only client cancellations) correctly mirrors the proven logs tool pattern and is well tested with regression tests for both audit and audit-diff.
💡 Themes
- Flat timeout vs. scaling by request size (high): unlike
logs, which scales its timeout with the number of runs/workflows requested,audit/audit-diffuse a flat 5-minute constant even though both accept multiple run IDs. Large multi-run audits/diffs can still hitcontext deadline exceeded, just at 300s instead of 60s. - Duplicated boilerplate (low): the WithoutCancel/WithTimeout + watcher-goroutine pattern is now copy-pasted three times (logs, audit, audit-diff); worth extracting to a shared helper to avoid future divergence.
Neither issue is a blocking correctness bug in this diff, so filing as COMMENT rather than REQUEST_CHANGES, but the timeout-scaling gap should be addressed promptly since it can reproduce the original bug report under load.
🔎 Code quality review by PR Code Quality Reviewer · auto · 81.1 AIC · ⌖ 4.72 AIC · ⊞ 7.8K
Comment /review to run again
| // 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 |
There was a problem hiding this comment.
Fixed 5-minute timeout regardless of how many runs are audited/diffed — this can reintroduce the same premature-timeout bug the PR is fixing, just at a different threshold.
💡 Details
audit accepts run_ids_or_urls (multiple runs) and audit-diff accepts compare_run_ids (multiple comparison runs), but both use a flat defaultMCPAuditTimeoutMinutes = 5 / defaultMCPAuditDiffTimeoutMinutes = 5 constant. The sibling logs tool in this same file already solves this by scaling its timeout with effectiveMCPLogsToolTimeoutMinutes(...) based on request size. Auditing/diffing many runs — each requiring its own artifact download — can legitimately exceed 5 minutes, causing the exact context deadline exceeded symptom this PR is fixing, just moved from 60s to 300s.
Suggested fix: scale the timeout by len(runItems) / len(args.CompareRunIDs) similar to the logs tool per-count scaling, or expose a configurable timeout parameter like logs does.
| // 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, |
There was a problem hiding this comment.
The context-detach-and-cancellation-watcher block is now copy-pasted verbatim across three tool handlers (logs, audit, audit-diff).
💡 Details
Each of the three call sites duplicates the same ~15 lines: context.WithTimeout(context.WithoutCancel(ctx), ...) plus a panic-recovering goroutine that forwards only context.Canceled. Any future correction to this cancellation-forwarding semantics (e.g. a subtle bug in the Canceled-only forwarding, or a need to also propagate a signal shutdown) must now be manually replicated in three places, and it is easy for the copies to silently diverge over time.
Suggested fix: extract a helper, e.g. func withDetachedTimeout(ctx context.Context, timeout time.Duration, logPrefix string) (context.Context, context.CancelFunc), that encapsulates the WithoutCancel/WithTimeout + watcher goroutine, and call it from all three tools.
|
@copilot please run the Reviewer follow-up:
Run: https://github.com/github/gh-aw/actions/runs/30520508562
|
Triage Summary
Fixes reliable 60s timeout failure in audit/audit-diff MCP tools by detaching subprocess context from gateway deadline. Not draft.
|
…t 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>
Addressed in the latest commit. Here's what changed: Comment fix (lines 41–44): Removed the incorrect claim that Test fix (lines 1009–1013 and 1049): The reviewer was right — To fix this properly, I extracted the detachment logic into a gatewayCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
subCtx, subCancel := newMCPSubprocessContext(gatewayCtx, 5*time.Minute, "audit")
deadline, _ := subCtx.Deadline()
// asserts deadline is ~5 minutes from now, NOT 2 secondsI verified the regression guard works: temporarily removing ADR: The draft ADR (docs/adr/49061-detach-audit-subprocess-context-from-mcp-gateway-deadline.md) was already committed in an earlier commit. |
|
🎉 This pull request is included in a new release. Release: |
The
auditandaudit-diffMCP tools reliably failed after exactly 60 seconds withcontext deadline exceededbecause they passed the gateway's request context directly toexec.CommandContext, binding the subprocess lifetime to the gateway's per-tool RPC deadline. Thelogstool had already been fixed with the correct pattern;auditandaudit-diffwere missing it.Changes
mcp_tools_privileged.godefaultMCPAuditTimeoutMinutes = 5anddefaultMCPAuditDiffTimeoutMinutes = 5constantscontext.WithoutCancel + context.WithTimeoutto bothauditandaudit-diffsubprocess execution — identical to the existinglogsfixcontext.Canceled), never the gateway'scontext.DeadlineExceededmcp_tools_privileged_test.goTestAuditToolSubprocessContextIgnoresGatewayDeadlineandTestAuditDiffToolSubprocessContextIgnoresGatewayDeadline— analogous to the existingTestLogsToolSubprocessContextIgnoresGatewayDeadlineregression test