Scale MCP logs timeout for larger fetch windows #42295
+140
−38
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,25 +109,65 @@ func TestTimeoutLogic(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestEffectiveMCPLogsToolTimeoutMinutes verifies that the MCP logs tool | ||
| // scales its implicit timeout with larger fetch windows while preserving | ||
| // explicit user-provided timeouts. | ||
| func TestEffectiveMCPLogsToolTimeoutMinutes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| requestedTimeout int | ||
| count int | ||
| want int | ||
| }{ | ||
| { | ||
| name: "explicit timeout is preserved", | ||
| requestedTimeout: 5, | ||
| count: 100, | ||
| want: 5, | ||
| }, | ||
| { | ||
| name: "small fetch window keeps one minute default", | ||
| requestedTimeout: 0, | ||
| count: 40, | ||
| want: 1, | ||
| }, | ||
| { | ||
| name: "fetch window above forty runs gets two minutes", | ||
| requestedTimeout: 0, | ||
| count: 41, | ||
| want: 2, | ||
| }, | ||
| { | ||
| name: "eighty run fetch window stays in two minute tier", | ||
| requestedTimeout: 0, | ||
| count: 80, | ||
| want: 2, | ||
| }, | ||
| { | ||
| name: "eighty one run fetch window enters three minute tier", | ||
| requestedTimeout: 0, | ||
| count: 81, | ||
| want: 3, | ||
| }, | ||
| { | ||
| name: "default hundred run window gets three minutes", | ||
| requestedTimeout: 0, | ||
| count: 100, | ||
| want: 3, | ||
| }, | ||
| { | ||
| name: "unspecified count falls back to default window size", | ||
| requestedTimeout: 0, | ||
| count: 0, | ||
| want: 3, | ||
| }, | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The tier 1→2 boundary (count=40/41) is tested but the tier 2→3 boundary (count=80/81) is not. A future off-by-one in the ceiling-division formula would only be caught at the tested boundaries. 💡 Two additional cases to add before line 152{
name: "top of tier 2 keeps two minutes",
requestedTimeout: 0,
count: 80,
want: 2,
},
{
name: "bottom of tier 3 gets three minutes",
requestedTimeout: 0,
count: 81,
want: 3,
},These pin both sides of the second tier boundary, matching the depth of coverage already present for the first boundary (40/41). @copilot please address this. |
||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := effectiveMCPLogsToolTimeoutMinutes(tt.requestedTimeout, tt.count); got != tt.want { | ||
| t.Errorf("effectiveMCPLogsToolTimeoutMinutes(%d, %d) = %d, want %d", tt.requestedTimeout, tt.count, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,12 @@ import ( | |
| "github.com/modelcontextprotocol/go-sdk/mcp" | ||
| ) | ||
|
|
||
| const ( | ||
| defaultMCPLogsToolCount = 100 | ||
| defaultMCPLogsTimeoutMinutes = 1 | ||
| mcpLogsRunsPerDefaultTimeoutMinute = 40 | ||
| ) | ||
|
|
||
| // appendRepoFlagFromEnv appends "--repo <owner/repo>" to args when GITHUB_REPOSITORY | ||
| // is set in the environment. This allows gh CLI subcommands to identify the repository | ||
| // without falling back to git-based detection, which fails in sandboxed environments | ||
|
|
@@ -41,11 +47,34 @@ type logsArgs struct { | |
| Branch string `json:"branch,omitempty" jsonschema:"Filter runs by branch name"` | ||
| AfterRunID int64 `json:"after_run_id,omitempty" jsonschema:"Filter runs with database ID after this value (exclusive)"` | ||
| BeforeRunID int64 `json:"before_run_id,omitempty" jsonschema:"Filter runs with database ID before this value (exclusive)"` | ||
| Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spend downloading logs (default: auto-scales with count in the MCP server, rounded up in 40-run increments; e.g. 1 minute up to 40, 2 minutes for 41-80, 3 minutes for 81-120, and so on)"` | ||
| MaxTokens int `json:"max_tokens,omitempty" jsonschema:"Deprecated: accepted for backward compatibility but ignored. Output is always written to a file."` | ||
| Artifacts []string `json:"artifacts,omitempty" jsonschema:"Artifact sets to download (default: usage). Valid sets: all, activation, agent, detection, experiment, firewall, github-api, mcp, usage"` | ||
| } | ||
|
|
||
| func defaultMCPLogsToolTimeoutMinutesForCount(count int) int { | ||
| count = effectiveMCPLogsToolCount(count) | ||
|
|
||
| // Round up in 40-run increments so requests slightly above the current | ||
| // 60-second threshold automatically get an extra minute of budget. | ||
| return max(defaultMCPLogsTimeoutMinutes, (count+mcpLogsRunsPerDefaultTimeoutMinute-1)/mcpLogsRunsPerDefaultTimeoutMinute) | ||
| } | ||
|
|
||
| func effectiveMCPLogsToolCount(count int) int { | ||
| if count > 0 { | ||
| return count | ||
| } | ||
| return defaultMCPLogsToolCount | ||
| } | ||
|
|
||
| func effectiveMCPLogsToolTimeoutMinutes(requestedTimeout, count int) int { | ||
| if requestedTimeout > 0 { | ||
| return requestedTimeout | ||
| } | ||
|
|
||
| return defaultMCPLogsToolTimeoutMinutesForCount(count) | ||
| } | ||
|
|
||
| // The logs tool requires write+ access and checks actor permissions. | ||
| // Returns an error if schema generation fails. | ||
| func registerLogsTool(server *mcp.Server, execCmd execCmdFunc, actor string, validateActor bool) error { | ||
|
|
@@ -56,10 +85,12 @@ func registerLogsTool(server *mcp.Server, execCmd execCmdFunc, actor string, val | |
| return err | ||
| } | ||
| // Add elicitation defaults for common parameters | ||
| if err := AddSchemaDefault(logsSchema, "count", defaultMCPLogsToolCount); err != nil { | ||
| mcpLog.Printf("Failed to add default for count: %v", err) | ||
| } | ||
| // Schema default corresponds to defaultMCPLogsToolCount; runtime timeout | ||
| // scales with the effective count used for the request. | ||
| if err := AddSchemaDefault(logsSchema, "timeout", defaultMCPLogsToolTimeoutMinutesForCount(defaultMCPLogsToolCount)); err != nil { | ||
|
Contributor
There was a problem hiding this comment. The schema default for Consider adding a small comment here like: // Schema default corresponds to defaultMCPLogsToolCount; actual runtime value
// scales down for smaller count values — see defaultMCPLogsToolTimeoutMinutesForCount.@copilot please address this. |
||
| mcpLog.Printf("Failed to add default for timeout: %v", err) | ||
| } | ||
| if err := AddSchemaDefault(logsSchema, "max_tokens", 12000); err != nil { | ||
|
|
@@ -141,9 +172,8 @@ from where the previous request stopped due to timeout.`, | |
| if args.WorkflowName != "" { | ||
| cmdArgs = append(cmdArgs, args.WorkflowName) | ||
| } | ||
| effectiveCount := effectiveMCPLogsToolCount(args.Count) | ||
| cmdArgs = append(cmdArgs, "-c", strconv.Itoa(effectiveCount)) | ||
| if args.StartDate != "" { | ||
| cmdArgs = append(cmdArgs, "--start-date", args.StartDate) | ||
| } | ||
|
|
@@ -179,19 +209,17 @@ from where the previous request stopped due to timeout.`, | |
|
|
||
| cmdArgs = appendRepoFlagFromEnv(cmdArgs) | ||
|
|
||
| // Scale the implicit MCP timeout with the requested fetch window so | ||
| // larger fleet-wide requests do not hit the 60s server deadline by default. | ||
| timeoutValue := effectiveMCPLogsToolTimeoutMinutes(args.Timeout, effectiveCount) | ||
| cmdArgs = append(cmdArgs, "--timeout", strconv.Itoa(timeoutValue)) | ||
|
Comment on lines
210
to
215
|
||
|
|
||
| // Always use --json mode in MCP server | ||
| cmdArgs = append(cmdArgs, "--json") | ||
|
|
||
| // Log the command being executed for debugging | ||
| mcpLog.Printf("Executing logs tool: workflow=%s, requested_count=%d, effective_count=%d, firewall=%v, no_firewall=%v, filtered_integrity=%v, timeout=%d, command_args=%v", | ||
| args.WorkflowName, args.Count, effectiveCount, args.Firewall, args.NoFirewall, args.FilteredIntegrity, timeoutValue, cmdArgs) | ||
|
|
||
| notifyProgress(ctx, req, 0, 100, "Downloading workflow logs...") | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite covers the
40 → 41boundary (1 → 2 min) but misses the80 → 81boundary (2 → 3 min). Consider adding test cases forcount=80(expect 2 min) andcount=81(expect 3 min) to fully exercise both tier transitions and guard against accidental formula changes.@copilot please address this.