Don't wait for file-based MCPs we never attempt to start#10969
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR changes cloud-environment file-based MCP discovery so the agent runner waits only for servers that were actually auto-start requested, while still emitting metadata for all detected servers. It also reduces the readiness timeout and adds diagnostic logging around pending MCP startup state.
Concerns
No blocking correctness, security, or error-handling concerns found in the reviewed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let mut new_installations = Vec::new(); | ||
| for installation in installations { | ||
| let uuid = installation.uuid(); | ||
| let server_name = &installation.templatable_mcp_server().name; | ||
| if self.active_servers.contains_key(&uuid) { | ||
| log::info!( | ||
| "Skipping file-based MCP server '{server_name}' ({uuid}); already running" | ||
| ); | ||
| continue; | ||
| } | ||
| if self.spawned_servers.contains_key(&uuid) { | ||
| log::info!( | ||
| "Skipping file-based MCP server '{server_name}' ({uuid}); already starting" | ||
| ); | ||
| continue; | ||
| } | ||
| log::info!("Spawning file-based MCP server '{server_name}' ({uuid})"); | ||
| new_installations.push(installation.clone()); | ||
| } |
There was a problem hiding this comment.
This is a nice improvement! Thanks for making this more verbose.
vkodithala
left a comment
vkodithala
left a comment
There was a problem hiding this comment.
LGTM! Left a few comments, nothing blocking. Thanks for the fix!
| /// UUIDs that were actually auto-start requested while parsing each `(root, provider)`. | ||
| /// They are temporarily stored here and removed to emit FileBasedMCPManagerEvent::CloudEnvMcpScanComplete | ||
| pending_scan_auto_started_servers_by_root: | ||
| HashMap<PathBuf, HashMap<MCPProvider, HashSet<Uuid>>>, |
There was a problem hiding this comment.
nit: This is spaced weirdly. Should the HashMap declaration be inline?
There was a problem hiding this comment.
cargo fmt wants it separated like this :/
| log::info!( | ||
| "Auto-spawning file-based MCP server '{server_name}' ({installation_uuid})" | ||
| ); |
There was a problem hiding this comment.
nit: This might get noisy, especially for local logs. I think we already have sufficient signal from logs you included in driver.rs to enumerate which servers were auto-started, is there any functional reason to keep this? Or maybe just make this a log::debug?
There was a problem hiding this comment.
Are we autospawning servers that often? I thought it would be useful to know locally whenever mcp servers are being spawned, since we don't seem to have that now
…10969) ## Description In an oz run, we were timing out after waiting 60s for discovered file-based MCP servers to reach a terminal status. We intentionally do not start project-scoped file-based MCP servers for security reasons in [this PR](warpdotdev/warp-internal#24640) But the agent runner was still discovering these and waiting for them to reach a terminal status, even though we never attempt to start them Now `CloudEnvMcpScanComplete` contains all detected file-based mcp servers, but also notes which we will actually attempt to auto-start. We only wait for those. Did some refactoring to make clear we don't auto-start every file-based mcp server we detect. Also add some logging and reduce the timeout to 20s.
2 participants
Description
In an oz run, we were timing out after waiting 60s for discovered file-based MCP servers to reach a terminal status.
We intentionally do not start project-scoped file-based MCP servers for security reasons in this PR
But the agent runner was still discovering these and waiting for them to reach a terminal status, even though we never attempt to start them
Now
CloudEnvMcpScanCompletecontains all detected file-based mcp servers, but also notes which we will actually attempt to auto-start. We only wait for those. Did some refactoring to make clear we don't auto-start every file-based mcp server we detect.Also add some logging and reduce the timeout to 20s.