fix: improve mcp timeout handling #551
Merged
Conversation
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
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a configurable timeout for MCP calls, refactors context and cancellation handling in Mods, and modernizes type usage in the codebase.
- Adds
MCPTimeouttoConfigwith default and YAML/env support. - Refactors
Modsto use a sharedctxand a slice of cancel functions, and wiresMCPTimeoutinto all MCP contexts. - Parallelizes tool listing via
errgroup, updatesinterface{}toany, and replaces hardcoded timeouts.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mods.go | Changed cancelRequest to slice, added ctx field, and integrated MCPTimeout |
| mcp.go | Switched to iter.Seq2, added errgroup parallelism, and mutex for results |
| main.go | Passed CLI context into newMods, applied MCPTimeout to timeouts |
| config.go | Added MCPTimeout field and help text; updated YAML unmarshal signature |
| config_template.yml | Added default mcp-timeout entry |
Comments suppressed due to low confidence (2)
mods.go:66
- [nitpick] The field
cancelRequestis now a slice of cancel functions; renaming it tocancelRequestsorcancelFuncswould more clearly communicate its purpose.
cancelRequest []context.CancelFunc
mcp.go:118
- Using
context.Background()here bypasses the passed-inctx, ignoring timeouts and cancellations. Replace withclient.CallTool(ctx, request).
return "", fmt.Errorf("mcp: invalid tool name: %q", name)
|
1st is I could totally see that tripping people up! (Not me or anything 😂 ) |
bashbunni
reviewed
Jul 9, 2025
Co-authored-by: bashbunni <15822994+bashbunni@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request introduces a configurable timeout for MCP server calls, refactors context handling to improve cancellation and concurrency, and updates type usage for better code consistency. The most significant changes are grouped into three themes: MCP timeout configuration, context and concurrency improvements, and type updates.
MCP Timeout Configuration:
MCPTimeoutfield to theConfigstruct with a default value of 15 seconds. This timeout is configurable via YAML and environment variables (yaml:"mcp-timeout" env:"MCP_TIMEOUT") [1] [2] [3].helpmap to include a description for themcp-timeoutoption.MCPTimeoutvalue into context creation for MCP server calls, replacing hardcoded timeouts [1] [2] [3].Context and Concurrency Improvements:
Modsstruct to manage multiple cancel functions (cancelRequestchanged from a singlecontext.CancelFuncto a slice) and added a shared context field (ctx) for centralized context management [1] [2] [3].errgroup.Groupfor parallel execution and a mutex to safely update shared data.Type Updates:
interface{}type withanyin YAML unmarshaling and error handling for improved readability and modern Go practices [1] [2] [3] [4].These changes collectively enhance the configurability, robustness, and maintainability of the codebase.