fix: add optional timeout to McpClient::send_request() (#852)
We now impose a 10s timeout on the initial `tools/list` request to an MCP server. We do not apply a timeout for other types of requests yet, but we should start enforcing those, as well.
This commit is contained in:
@@ -5,6 +5,7 @@ use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Context;
|
||||
use async_channel::Receiver;
|
||||
@@ -396,9 +397,10 @@ impl Session {
|
||||
server: &str,
|
||||
tool: &str,
|
||||
arguments: Option<serde_json::Value>,
|
||||
timeout: Option<Duration>,
|
||||
) -> anyhow::Result<mcp_types::CallToolResult> {
|
||||
self.mcp_connection_manager
|
||||
.call_tool(server, tool, arguments)
|
||||
.call_tool(server, tool, arguments, timeout)
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -1194,7 +1196,12 @@ async fn handle_function_call(
|
||||
_ => {
|
||||
match try_parse_fully_qualified_tool_name(&name) {
|
||||
Some((server, tool_name)) => {
|
||||
handle_mcp_tool_call(sess, &sub_id, call_id, server, tool_name, arguments).await
|
||||
// TODO(mbolin): Determine appropriate timeout for tool call.
|
||||
let timeout = None;
|
||||
handle_mcp_tool_call(
|
||||
sess, &sub_id, call_id, server, tool_name, arguments, timeout,
|
||||
)
|
||||
.await
|
||||
}
|
||||
None => {
|
||||
// Unknown function: reply with structured failure so the model can adapt.
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
//! `"<server><MCP_TOOL_NAME_DELIMITER><tool>"` as the key.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
@@ -25,6 +26,9 @@ use crate::mcp_server_config::McpServerConfig;
|
||||
/// choose a delimiter from this character set.
|
||||
const MCP_TOOL_NAME_DELIMITER: &str = "__OAI_CODEX_MCP__";
|
||||
|
||||
/// Timeout for the `tools/list` request.
|
||||
const LIST_TOOLS_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
fn fully_qualified_tool_name(server: &str, tool: &str) -> String {
|
||||
format!("{server}{MCP_TOOL_NAME_DELIMITER}{tool}")
|
||||
}
|
||||
@@ -104,6 +108,7 @@ impl McpConnectionManager {
|
||||
server: &str,
|
||||
tool: &str,
|
||||
arguments: Option<serde_json::Value>,
|
||||
timeout: Option<Duration>,
|
||||
) -> Result<mcp_types::CallToolResult> {
|
||||
let client = self
|
||||
.clients
|
||||
@@ -112,7 +117,7 @@ impl McpConnectionManager {
|
||||
.clone();
|
||||
|
||||
client
|
||||
.call_tool(tool.to_string(), arguments)
|
||||
.call_tool(tool.to_string(), arguments, timeout)
|
||||
.await
|
||||
.with_context(|| format!("tool call failed for `{server}/{tool}`"))
|
||||
}
|
||||
@@ -132,7 +137,9 @@ pub async fn list_all_tools(
|
||||
let server_name_cloned = server_name.clone();
|
||||
let client_clone = client.clone();
|
||||
join_set.spawn(async move {
|
||||
let res = client_clone.list_tools(None).await;
|
||||
let res = client_clone
|
||||
.list_tools(None, Some(LIST_TOOLS_TIMEOUT))
|
||||
.await;
|
||||
(server_name_cloned, res)
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
use std::time::Duration;
|
||||
|
||||
use tracing::error;
|
||||
|
||||
use crate::codex::Session;
|
||||
@@ -15,6 +17,7 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
server: String,
|
||||
tool_name: String,
|
||||
arguments: String,
|
||||
timeout: Option<Duration>,
|
||||
) -> ResponseInputItem {
|
||||
// Parse the `arguments` as JSON. An empty string is OK, but invalid JSON
|
||||
// is not.
|
||||
@@ -45,25 +48,27 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
notify_mcp_tool_call_event(sess, sub_id, tool_call_begin_event).await;
|
||||
|
||||
// Perform the tool call.
|
||||
let (tool_call_end_event, tool_call_err) =
|
||||
match sess.call_tool(&server, &tool_name, arguments_value).await {
|
||||
Ok(result) => (
|
||||
EventMsg::McpToolCallEnd {
|
||||
call_id,
|
||||
success: !result.is_error.unwrap_or(false),
|
||||
result: Some(result),
|
||||
},
|
||||
None,
|
||||
),
|
||||
Err(e) => (
|
||||
EventMsg::McpToolCallEnd {
|
||||
call_id,
|
||||
success: false,
|
||||
result: None,
|
||||
},
|
||||
Some(e),
|
||||
),
|
||||
};
|
||||
let (tool_call_end_event, tool_call_err) = match sess
|
||||
.call_tool(&server, &tool_name, arguments_value, timeout)
|
||||
.await
|
||||
{
|
||||
Ok(result) => (
|
||||
EventMsg::McpToolCallEnd {
|
||||
call_id,
|
||||
success: !result.is_error.unwrap_or(false),
|
||||
result: Some(result),
|
||||
},
|
||||
None,
|
||||
),
|
||||
Err(e) => (
|
||||
EventMsg::McpToolCallEnd {
|
||||
call_id,
|
||||
success: false,
|
||||
result: None,
|
||||
},
|
||||
Some(e),
|
||||
),
|
||||
};
|
||||
|
||||
notify_mcp_tool_call_event(sess, sub_id, tool_call_end_event.clone()).await;
|
||||
let EventMsg::McpToolCallEnd {
|
||||
|
||||
Reference in New Issue
Block a user