Add configurable MCP_TIMEOUT (default 600s) across all agents#520
Add configurable MCP_TIMEOUT (default 600s) across all agents#520davidhadas wants to merge 1 commit into
Conversation
davidhadas
force-pushed
the
align-mcp-timeout
branch
4 times, most recently
from
61f43eb to
7652bc1
Compare
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
The PR goal is sound — HITL flows need configurable MCP timeouts and MCP_TIMEOUT is a clean approach. However the branch has a bad rebase that introduces duplicate field definitions in two config files, plus a typo. See inline comments.
Areas reviewed: Python (config classes, agent setup)
Commits: 1, signed-off ✓
CI: all passing
| LLM_API_KEY: str = "dummy" | ||
| MCP_URLS: str = "http://localhost:8000/mcp" | ||
| MCP_TRANSPORT: str = "streamable_http" | ||
| MCP_TIMEOUT: int = 600 |
There was a problem hiding this comment.
nit — env var not read
This uses a bare MCP_TIMEOUT: int = 600 without os.getenv("MCP_TIMEOUT", ...), so setting the MCP_TIMEOUT env var won't take effect for this agent. All other agents read from the env var.
Consider: MCP_TIMEOUT: int = int(os.getenv("MCP_TIMEOUT", "600")) or use Field(default=int(os.getenv("MCP_TIMEOUT", "600")), ...).
There was a problem hiding this comment.
Since we are at class Configuration(BaseSettings) - extending BaseSettings from pydantic-settings, the library automatically reads field values from the environment by field name. MCP_TIMEOUT: int = 600 will pick up MCP_TIMEOUT from the environment at instantiation time without needing an explicit os.getenv() call.
| url=mcp_url, | ||
| timeout=30, | ||
| sse_read_timeout=300, | ||
| timeout=self.settings.MCP_TIMEOUT, |
There was a problem hiding this comment.
suggestion — Single timeout for two different semantics
Previously: timeout=30 (connection/HTTP timeout) and sse_read_timeout=300 (how long to wait for the next SSE event). Both are now MCP_TIMEOUT (600s).
The HITL use case justifies a long sse_read_timeout, but a 10-minute connection timeout means a down MCP server won't be detected quickly. Consider keeping connection timeout shorter (e.g. 30s default) or adding a separate MCP_CONNECT_TIMEOUT setting.
There was a problem hiding this comment.
We currently use long polling, the client sends a standard HTTP request, and the server purposefully holds that request open—delaying its response—until it actually has new data to send back.
If we set timeout=30, the client will only wait a maximum of 30 seconds for the server to send an HTTP response.
| async with ( | ||
| streamablehttp_client( | ||
| url=settings.MCP_URL, | ||
| timeout=settings.MCP_TIMEOUT, |
There was a problem hiding this comment.
suggestion — Same concern as simple_generalist: both timeout and sse_read_timeout are set to MCP_TIMEOUT. The SSE read timeout should be long for HITL, but the connection timeout probably shouldn't be 10 minutes.
Replace hardcoded MCP connection timeouts with a configurable MCP_TIMEOUT environment variable (default 10 minutes) in git_issue_agent, simple_generalist, slack_researcher, and generic_agent. Signed-off-by: David Hadas <david.hadas@gmail.com>
davidhadas
force-pushed
the
align-mcp-timeout
branch
from
7652bc1 to
96761e4
Compare
3 participants
Replace hardcoded MCP connection timeouts with a configurable MCP_TIMEOUT environment variable (default 10 minutes) in git_issue_agent, simple_generalist, slack_researcher, and generic_agent.
Context - HITL requires longer timeouts to not get to a timeout while waiting for user consent.
kagenti/kagenti#1435
Fixes # #519