Fix recurring 60s timeout in logs MCP tool degrading audit datasets#45714
Conversation
- Add engine.mcp.tool-timeout: 10m to safe-output-health.md so the MCP gateway allows enough time for the logs subprocess to complete - Make the logs MCP tool handler context-deadline-aware: compute a bounded internal --timeout that leaves a 30s buffer for the subprocess to write continuation data before the MCP gateway context expires - Improve error message when subprocess is killed by context deadline exceeded to guide users to set engine.mcp.tool-timeout - Fix documentation: timeout parameter is in minutes, not seconds - Add tests for effectiveMCPLogsToolTimeoutMinutesWithDeadline Closes #45510 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…tion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| - `after_run_id`, `before_run_id` (optional): Pagination by run ID | ||
| - `artifacts` (array of strings, optional): Artifact sets to download. Valid values: `all`, `activation`, `agent`, `detection`, `experiment`, `firewall`, `github-api`, `mcp`, `usage`. Defaults to `usage`. | ||
| - `timeout` (optional): Max seconds to download (default: 50) | ||
| - `timeout` (optional): Max minutes to download (default: auto-scales with count in the MCP server, rounded up in 40-run increments; e.g. 1 minute up to 40 runs, 2 minutes for 41-80, 3 minutes for 81-120). For large date-range fetches set this explicitly (e.g. `5`) and also configure `engine.mcp.tool-timeout: 10m` in the workflow frontmatter so the MCP gateway allows enough time. |
There was a problem hiding this comment.
Updated in 0026d77: changed the explicit timeout example from 5 to 10 in the logs reference entry so it lines up with the engine.mcp.tool-timeout: 10m guidance.
There was a problem hiding this comment.
Pull request overview
Attempts to prevent MCP log downloads from being interrupted before continuation data is written.
Changes:
- Adds deadline-aware subprocess timeout calculation and timeout guidance.
- Configures the health workflow with a 10-minute gateway timeout.
- Corrects timeout documentation and adds unit tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Adds deadline-aware log timeout handling. |
pkg/cli/logs_timeout_test.go |
Tests deadline-based timeout calculations. |
docs/src/content/docs/reference/gh-aw-as-mcp-server.md |
Documents timeout units and scaling. |
.github/workflows/safe-output-health.md |
Sets the MCP tool timeout to 10 minutes. |
.github/workflows/safe-output-health.lock.yml |
Recompiles the workflow configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Medium
| deadline, ok := ctx.Deadline() | ||
| if !ok { | ||
| return baseTimeout | ||
| } |
| if errors.Is(ctx.Err(), context.DeadlineExceeded) || | ||
| strings.Contains(err.Error(), "context deadline exceeded") { |
| mainMsg += "\n\nThe logs tool exceeded the MCP gateway tool timeout. " + | ||
| "Add 'engine.mcp.tool-timeout: 10m' (or higher) to your workflow frontmatter to allow more time." |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 32 + Urgency 20 + Quality 10 Rationale: Fixes recurring 60s timeout that degrades audit dataset quality on daily workflow runs. Moderate risk (5 files, adds timeout test). CI
|
|
Run: https://github.com/github/gh-aw/actions/runs/29420425558
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in 952f8b6. I addressed the unresolved review findings by removing the deadline-based timeout capping and deadline-exceeded hint path in |
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Run: https://github.com/github/gh-aw/actions/runs/29426851914
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed on Current status: unresolved review threads are all outdated against older commits, and the latest PR workflow runs are |
|
@copilot please run the
|
Ran pr-finisher on the up-to-date branch at |
…ix-60s-timeout Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran pr-finisher triage on the current head ( I reran local checks:
CI on this branch head remains |
|
🎉 This pull request is included in a new release. Release: |
The
logsMCP tool was hitting the MCP gateway's default 60s tool-execution deadline mid-download on daily audit runs (safe-output-health,api-consumption), killing thegh aw logssubprocess via context cancellation before it could write continuation data — leaving reports on partial datasets.Changes
Immediate fix:
safe-output-health.mdengine: claudeto setengine.mcp.tool-timeout: 10m, matchingapi-consumption-report.md(already fixed). Raises the gateway per-tool deadline from 60s to 10m.Systemic fix: deadline-aware subprocess timeout (
mcp_tools_privileged.go)effectiveMCPLogsToolTimeoutMinutesWithDeadline(ctx, requestedTimeout, count)that caps the subprocess--timeouttofloor((remaining − 30s buffer) / 1min)when the context carries a deadline shorter than the auto-scaled value. The 30s buffer gives the subprocess time to write continuation data before the gateway context fires.mcpLogsDeadlineBufferSeconds = 30:truncate((2m−30s)/1m) = 1)context.DeadlineExceeded, directing users to setengine.mcp.tool-timeout: 10m.Documentation fix:
gh-aw-as-mcp-server.mdtimeoutparameter was documented as "Max seconds"; corrected to minutes with auto-scaling description.Tests
TestEffectiveMCPLogsToolTimeoutMinutesWithDeadlinecovering: no deadline, generous deadline (no cap), tight deadlines triggering caps, sub-buffer deadline (no cap), and explicit timeout capping.