Fix cache hit rate by making MCP tools order deterministic (#2611)
Fixes https://github.com/openai/codex/issues/2610 This PR sorts the tools in `get_openai_tools` by name to ensure a consistent MCP tool order. Currently, MCP servers are stored in a HashMap, which does not guarantee ordering. As a result, the tool order changes across turns, effectively breaking prompt caching in multi-turn sessions. An alternative solution would be to replace the HashMap with an ordered structure, but that would require a much larger code change. Given that it is unrealistic to have so many MCP tools that sorting would cause performance issues, this lightweight fix is chosen instead. By ensuring deterministic tool order, this change should significantly improve cache hit rates and prevent users from hitting usage limits too quickly. (For reference, my own sessions last week reached the limit unusually fast, with cache hit rates falling below 1%.) ## Result After this fix, sessions with MCP servers now show caching behavior almost identical to sessions without MCP servers. Without MCP | With MCP :-------------------------:|:-------------------------: <img width="1368" height="1634" alt="image" src="https://github.com/user-attachments/assets/26edab45-7be8-4d6a-b471-558016615fc8" /> | <img width="1356" height="1632" alt="image" src="https://github.com/user-attachments/assets/5f3634e0-3888-420b-9aaf-deefd9397b40" />
This commit is contained in:
@@ -531,7 +531,12 @@ pub(crate) fn get_openai_tools(
|
||||
}
|
||||
|
||||
if let Some(mcp_tools) = mcp_tools {
|
||||
for (name, tool) in mcp_tools {
|
||||
// Ensure deterministic ordering to maximize prompt cache hits.
|
||||
// HashMap iteration order is non-deterministic, so sort by fully-qualified tool name.
|
||||
let mut entries: Vec<(String, mcp_types::Tool)> = mcp_tools.into_iter().collect();
|
||||
entries.sort_by(|a, b| a.0.cmp(&b.0));
|
||||
|
||||
for (name, tool) in entries.into_iter() {
|
||||
match mcp_tool_to_openai_tool(name.clone(), tool.clone()) {
|
||||
Ok(converted_tool) => tools.push(OpenAiTool::Function(converted_tool)),
|
||||
Err(e) => {
|
||||
@@ -710,6 +715,80 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_get_openai_tools_mcp_tools_sorted_by_name() {
|
||||
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");
|
||||
let config = ToolsConfig::new(
|
||||
&model_family,
|
||||
AskForApproval::Never,
|
||||
SandboxPolicy::ReadOnly,
|
||||
false,
|
||||
false,
|
||||
/*use_experimental_streamable_shell_tool*/ false,
|
||||
);
|
||||
|
||||
// Intentionally construct a map with keys that would sort alphabetically.
|
||||
let tools_map: HashMap<String, mcp_types::Tool> = HashMap::from([
|
||||
(
|
||||
"test_server/do".to_string(),
|
||||
mcp_types::Tool {
|
||||
name: "a".to_string(),
|
||||
input_schema: ToolInputSchema {
|
||||
properties: Some(serde_json::json!({})),
|
||||
required: None,
|
||||
r#type: "object".to_string(),
|
||||
},
|
||||
output_schema: None,
|
||||
title: None,
|
||||
annotations: None,
|
||||
description: Some("a".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"test_server/something".to_string(),
|
||||
mcp_types::Tool {
|
||||
name: "b".to_string(),
|
||||
input_schema: ToolInputSchema {
|
||||
properties: Some(serde_json::json!({})),
|
||||
required: None,
|
||||
r#type: "object".to_string(),
|
||||
},
|
||||
output_schema: None,
|
||||
title: None,
|
||||
annotations: None,
|
||||
description: Some("b".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"test_server/cool".to_string(),
|
||||
mcp_types::Tool {
|
||||
name: "c".to_string(),
|
||||
input_schema: ToolInputSchema {
|
||||
properties: Some(serde_json::json!({})),
|
||||
required: None,
|
||||
r#type: "object".to_string(),
|
||||
},
|
||||
output_schema: None,
|
||||
title: None,
|
||||
annotations: None,
|
||||
description: Some("c".to_string()),
|
||||
},
|
||||
),
|
||||
]);
|
||||
|
||||
let tools = get_openai_tools(&config, Some(tools_map));
|
||||
// Expect shell first, followed by MCP tools sorted by fully-qualified name.
|
||||
assert_eq_tool_names(
|
||||
&tools,
|
||||
&[
|
||||
"shell",
|
||||
"test_server/cool",
|
||||
"test_server/do",
|
||||
"test_server/something",
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_mcp_tool_property_missing_type_defaults_to_string() {
|
||||
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");
|
||||
|
||||
Reference in New Issue
Block a user