Fix logs tool: context deadline exceeded without workflow_name filter#46554
Conversation
Three root-cause fixes: 1. Reduce BatchSizeForAllWorkflows from 250 to 100 (1 API call vs 3) 2. Propagate context to fetchWorkflowRunBatch so gh run list respects deadline 3. Apply 5-minute minimum timeout for no-workflow-filter requests 4. Remove static schema default for timeout to let runtime compute it per-request Closes #46538 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Fixes fleet-wide logs queries that time out while listing workflow runs.
Changes:
- Limits unfiltered batches to one GitHub API page.
- Propagates cancellation contexts into
gh run list. - Computes MCP timeouts dynamically and expands coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Computes workflow-aware timeouts. |
pkg/cli/mcp_tools_privileged_test.go |
Tests generated command arguments. |
pkg/cli/mcp_server_defaults_test.go |
Verifies timeout has no schema default. |
pkg/cli/logs_timeout_test.go |
Tests timeout scaling. |
pkg/cli/logs_orchestrator_download.go |
Propagates context and handles cancellation. |
pkg/cli/logs_models.go |
Reduces fleet-wide batch size. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
| // Without a workflow filter the GitHub API must scan all workflow runs, which | ||
| // is substantially slower for large repositories. Apply a higher minimum so | ||
| // the tool is less likely to exhaust the MCP gateway's per-tool timeout. | ||
| return max(defaultMCPLogsMinTimeoutMinutesAllWorkflows, base) |
| if errors.Is(err, context.DeadlineExceeded) { | ||
| timeoutReached = true | ||
| break | ||
| } | ||
| // Context cancelled (e.g. user signal or outer gateway timeout): propagate | ||
| // with partial results. We intentionally leave timeoutReached=false because | ||
| // this is an external cancellation, not the internal --timeout deadline firing. | ||
| // Callers can inspect the returned error to distinguish the two cases. | ||
| if errors.Is(err, context.Canceled) { | ||
| return processedRuns, timeoutReached, countLimitReached, err | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix logs tool timeout
The four-pronged fix is well-targeted and the changes are correct.
BatchSizeForAllWorkflows 250 → 100 — aligns with the GitHub API per-page cap (100), eliminating extra round-trips that caused timeouts.
Context propagation — threading ctx through to listWorkflowRunsWithPagination is the right fix. The soft-timeout / hard-cancel distinction (DeadlineExceeded breaks with partial results; Canceled propagates the error) is semantically correct and consistent with the existing caller.
Removing the static schema default for timeout — sound reasoning: the go-sdk fills in schema defaults before the handler runs, so a static value would always win over the runtime computation. Removing it lets args.Timeout == 0 trigger the runtime floor correctly.
5-minute floor for all-workflow queries — conservative and appropriate. Test coverage in logs_timeout_test.go and mcp_server_defaults_test.go is thorough.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.2 AIC · ⌖ 4.38 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 test functions)
Verdict✅ Passed. 0% implementation tests (threshold: 30%). All tests verify design contracts:
The test expansion is justified: new workflow_name parameter required comprehensive coverage of both named-workflow and all-workflow behavior paths. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (107 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
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 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.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — two issues found, requesting attention.
Key Themes
- Partial-result contract is unimplemented (see inline comment on
logs_orchestrator_download.go): the Canceled branch documents it returns partial results, but the caller discards them on any non-nil error. - Magic number in test (see inline comment on
logs_timeout_test.go): one case hardcodeswant: 7derived from ceil(250/40) — brittle if the constant changes.
Positive Highlights
- Root-cause analysis is clear and thorough.
- Removing the static timeout schema default is the right call; the inline comment is well-placed.
- Good test matrix: explicit, named-workflow, and all-workflow cases all covered.
- BatchSizeForAllWorkflows = 100 correctly matches the GitHub API per_page maximum.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36 AIC · ⌖ 4.62 AIC · ⊞ 6.7K
Comment /matt to run again
| // this is an external cancellation, not the internal --timeout deadline firing. | ||
| // Callers can inspect the returned error to distinguish the two cases. | ||
| if errors.Is(err, context.Canceled) { | ||
| return processedRuns, timeoutReached, countLimitReached, err |
There was a problem hiding this comment.
[/diagnosing-bugs] The comment says "propagate with partial results" but the caller discards them — the partial processedRuns are lost when this error is returned.
💡 Details
collectProcessedWorkflowRuns returns (processedRuns, timeoutReached, countLimitReached, err), but its caller in logs_orchestrator.go does:
processedRuns, timeoutReached, countLimitReached, err := collectProcessedWorkflowRuns(runtime, opts)
if err != nil {
return err // partial runs discarded here
}So on external cancellation (context.Canceled) the already-fetched runs are silently dropped. Either:
- Return
(nil, false, false, err)here (like the generic path below) and document that partial results are not surfaced forCanceled, OR - Thread partial-result surfacing through the caller so they are rendered before the error is returned.
The current code misleads future readers.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 7aef8a5. The context.Canceled branch now returns (nil, false, false, err) consistent with the generic error path. The misleading "propagate with partial results" comment has been removed. Additionally, listWorkflowRunsWithPagination now surfaces ctx.Err() directly when exec.CommandContext kills the subprocess, so errors.Is(err, context.Canceled) and errors.Is(err, context.DeadlineExceeded) in the caller work as expected.
| t.Errorf("effectiveMCPLogsToolTimeoutMinutes(%d, %d, %q) = %d, want %d", tt.requestedTimeout, tt.count, tt.workflowName, got, tt.want) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] The "very large count exceeds all-workflow minimum" test hardcodes want: 7 with a comment // ceil(250/40) = 7 — tying the test to implementation arithmetic rather than the exported constant.
💡 Suggestion
If mcpLogsRunsPerDefaultTimeoutMinute or the ceiling formula ever changes, this test will silently mismatch without the reader noticing. Consider:
want: effectiveMCPLogsToolTimeoutMinutes(0, 250, ""),or at minimum:
want: (250 + mcpLogsRunsPerDefaultTimeoutMinute - 1) / mcpLogsRunsPerDefaultTimeoutMinute,This keeps the test honest without coupling it to a magic number.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 7aef8a5. Replaced the hardcoded 7 with (250 + mcpLogsRunsPerDefaultTimeoutMinute - 1) / mcpLogsRunsPerDefaultTimeoutMinute so the expected value stays in sync with the constant automatically.
There was a problem hiding this comment.
Review: Fix logs tool context deadline / timeout
The three-part fix (batch size, context propagation, dynamic timeout floor) is well-reasoned and the test coverage is thorough. One non-blocking inconsistency worth addressing: the context.Canceled path does an early return where the context.DeadlineExceeded path uses break — see the inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 36 AIC · ⌖ 4.43 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_orchestrator_download.go:43
context.Canceled early return bypasses post-loop diagnostics: the timeout/iteration log statements after the loop are silently skipped, making it hard to diagnose why a run stopped.
<details>
<summary>💡 Suggested fix</summary>
The context.DeadlineExceeded branch uses break and falls through to post-loop logging. For consistency and debuggability, the Canceled path should do the same:
// Instead of early return, record the error and break
var cancelledErr error
if errors.Is…
</details>
|
@copilot run pr-finisher skill |
… misleading partial-results-for-Canceled; tie test want to constant Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
agenticworkflows logs --count 3without--workflow_namealways timed out (60s MCP gateway limit) due to three compounding issues in the no-workflow code path.Root causes & fixes
BatchSizeForAllWorkflows250 → 100 (logs_models.go)gh run list --limit 250pages as 3 sequential GitHub API calls (max 100/page). For repos with 250+ workflows, that's 45–60s+ just for the list step. Reducing to 100 means 1 call per batch.Context not propagated to
gh run list(logs_orchestrator_download.go)fetchWorkflowRunBatchwas calling the API withContext: nil(→context.Background()), so the subprocess had no deadline. Addedctx context.Contextparameter threaded fromruntime.activeCtx. Also added graceful handling:DeadlineExceededreturns partial results (treated as soft timeout),Canceledpropagates with partial results.Static
timeoutschema default bypassed per-request computation (mcp_tools_privileged.go)AddSchemaDefault(logsSchema, "timeout", 3)caused the go-sdk to fillargs.Timeout = 3before the handler ran, makingeffectiveMCPLogsToolTimeoutMinutesalways hit the "explicit timeout" branch. Removed the static default so the runtime computes the correct value from(requestedTimeout, count, workflowName).5-minute minimum for no-workflow queries
Added
defaultMCPLogsMinTimeoutMinutesAllWorkflows = 5.effectiveMCPLogsToolTimeoutMinutesnow acceptsworkflowName stringand applies this floor when no workflow is scoped, giving adequate headroom for fleet-wide queries.