Skip to content

Commit 4c5975d

Browse files
fix(client): complete response cache semantics
1 parent 5465223 commit 4c5975d

4 files changed

Lines changed: 101 additions & 21 deletions

File tree

crates/rmcp/src/service/client.rs

Lines changed: 79 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -285,22 +285,26 @@ const RESOURCE_TEMPLATE_LIST_CACHE_PREFIX: &str = "resources/templates/list:";
285285
const RESOURCE_READ_CACHE_PREFIX: &str = "resources/read:";
286286

287287
fn list_response_cache_key(prefix: &str, params: &Option<PaginatedRequestParams>) -> String {
288-
let cursor = params.as_ref().and_then(|params| params.cursor.as_deref());
289-
let cursor = serde_json::to_string(&cursor)
290-
.expect("serializing an optional pagination cursor cannot fail");
291-
format!("{prefix}{cursor}")
288+
let params = serde_json::to_string(params)
289+
.expect("serializing pagination request parameters cannot fail");
290+
format!("{prefix}{params}")
292291
}
293292

294293
fn resource_read_cache_key(params: &ReadResourceRequestParams) -> Option<String> {
295294
if params.input_responses.is_some() || params.request_state.is_some() {
296295
return None;
297296
}
298-
Some(resource_read_cache_key_for_uri(&params.uri))
297+
let serialized =
298+
serde_json::to_string(params).expect("serializing resource request parameters cannot fail");
299+
Some(format!(
300+
"{}{serialized}",
301+
resource_read_cache_prefix_for_uri(&params.uri)
302+
))
299303
}
300304

301-
fn resource_read_cache_key_for_uri(uri: &str) -> String {
305+
fn resource_read_cache_prefix_for_uri(uri: &str) -> String {
302306
let uri = serde_json::to_string(uri).expect("serializing a resource URI cannot fail");
303-
format!("{RESOURCE_READ_CACHE_PREFIX}{uri}")
307+
format!("{RESOURCE_READ_CACHE_PREFIX}{uri}:")
304308
}
305309

306310
fn request_uses_cursor(params: &Option<PaginatedRequestParams>) -> bool {
@@ -433,7 +437,7 @@ impl Peer<RoleClient> {
433437
}
434438

435439
pub(crate) async fn invalidate_resource_read_cache(&self, uri: &str) {
436-
self.invalidate_cached_response(&resource_read_cache_key_for_uri(uri))
440+
self.invalidate_cached_responses(&resource_read_cache_prefix_for_uri(uri))
437441
.await;
438442
}
439443

@@ -1292,6 +1296,27 @@ mod tests {
12921296
));
12931297
}
12941298

1299+
#[tokio::test]
1300+
async fn changing_ttl_policy_invalidates_existing_entries() {
1301+
let peer = disconnected_peer();
1302+
let key = list_response_cache_key(TOOL_LIST_CACHE_PREFIX, &None);
1303+
peer.cache_response(
1304+
key.clone(),
1305+
ServerResult::ListToolsResult(tools_result(Some(60_000), Some(CacheScope::Public))),
1306+
Some(60_000),
1307+
Some(CacheScope::Public),
1308+
)
1309+
.await;
1310+
assert!(peer.cached_response(&key).await.is_some());
1311+
1312+
peer.set_response_cache_config(
1313+
ClientCacheConfig::default().with_max_ttl(Duration::from_millis(1)),
1314+
)
1315+
.await;
1316+
1317+
assert!(peer.cached_response(&key).await.is_none());
1318+
}
1319+
12951320
#[tokio::test]
12961321
async fn private_entries_are_isolated_between_client_peers() {
12971322
let first = disconnected_peer();
@@ -1381,6 +1406,36 @@ mod tests {
13811406
);
13821407
}
13831408

1409+
#[test]
1410+
fn result_affecting_metadata_is_part_of_cache_keys() {
1411+
let mut first_meta = crate::model::Meta::new();
1412+
first_meta.insert("variant".into(), serde_json::json!("a"));
1413+
let mut second_meta = crate::model::Meta::new();
1414+
second_meta.insert("variant".into(), serde_json::json!("b"));
1415+
1416+
let first_page = Some(PaginatedRequestParams {
1417+
meta: Some(first_meta.clone()),
1418+
cursor: Some("page".into()),
1419+
});
1420+
let second_page = Some(PaginatedRequestParams {
1421+
meta: Some(second_meta.clone()),
1422+
cursor: Some("page".into()),
1423+
});
1424+
assert_ne!(
1425+
list_response_cache_key(TOOL_LIST_CACHE_PREFIX, &first_page),
1426+
list_response_cache_key(TOOL_LIST_CACHE_PREFIX, &second_page)
1427+
);
1428+
1429+
let first_resource =
1430+
ReadResourceRequestParams::new("file:///example").with_meta(first_meta);
1431+
let second_resource =
1432+
ReadResourceRequestParams::new("file:///example").with_meta(second_meta);
1433+
assert_ne!(
1434+
resource_read_cache_key(&first_resource),
1435+
resource_read_cache_key(&second_resource)
1436+
);
1437+
}
1438+
13841439
#[tokio::test]
13851440
async fn list_invalidation_discards_every_cached_page() {
13861441
let peer = disconnected_peer();
@@ -1408,11 +1463,17 @@ mod tests {
14081463
}
14091464

14101465
#[tokio::test]
1411-
async fn resource_update_invalidates_only_the_matching_uri() {
1466+
async fn resource_update_invalidates_every_metadata_variant_for_the_matching_uri() {
14121467
let peer = disconnected_peer();
1413-
let first_key = resource_read_cache_key_for_uri("file:///first");
1414-
let second_key = resource_read_cache_key_for_uri("file:///second");
1415-
for key in [&first_key, &second_key] {
1468+
let first_plain = ReadResourceRequestParams::new("file:///first");
1469+
let mut meta = crate::model::Meta::new();
1470+
meta.insert("variant".into(), serde_json::json!("a"));
1471+
let first_with_meta = ReadResourceRequestParams::new("file:///first").with_meta(meta);
1472+
let second = ReadResourceRequestParams::new("file:///second");
1473+
let first_plain_key = resource_read_cache_key(&first_plain).unwrap();
1474+
let first_meta_key = resource_read_cache_key(&first_with_meta).unwrap();
1475+
let second_key = resource_read_cache_key(&second).unwrap();
1476+
for key in [&first_plain_key, &first_meta_key, &second_key] {
14161477
peer.cache_response(
14171478
key.clone(),
14181479
ServerResult::ReadResourceResult(
@@ -1428,7 +1489,8 @@ mod tests {
14281489

14291490
peer.invalidate_resource_read_cache("file:///first").await;
14301491

1431-
assert!(peer.cached_response(&first_key).await.is_none());
1492+
assert!(peer.cached_response(&first_plain_key).await.is_none());
1493+
assert!(peer.cached_response(&first_meta_key).await.is_none());
14321494
assert!(peer.cached_response(&second_key).await.is_some());
14331495
}
14341496

@@ -1485,8 +1547,10 @@ mod tests {
14851547
let peer = disconnected_peer();
14861548
peer.set_response_cache_config(ClientCacheConfig::default().with_max_entries(1))
14871549
.await;
1488-
let first = resource_read_cache_key_for_uri("file:///first");
1489-
let second = resource_read_cache_key_for_uri("file:///second");
1550+
let first =
1551+
resource_read_cache_key(&ReadResourceRequestParams::new("file:///first")).unwrap();
1552+
let second =
1553+
resource_read_cache_key(&ReadResourceRequestParams::new("file:///second")).unwrap();
14901554
peer.cache_response(
14911555
first.clone(),
14921556
ServerResult::ReadResourceResult(ReadResourceResult::new(Vec::new())),

crates/rmcp/src/service/client/cache.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,11 +319,13 @@ impl Peer<RoleClient> {
319319
let mut cache = self.response_cache.write().await;
320320
let config_changed = cache.config != config;
321321
let partition_changed = cache.config.private_partition != config.private_partition;
322+
let ttl_policy_changed = cache.config.default_ttl != config.default_ttl
323+
|| cache.config.max_ttl != config.max_ttl;
322324
cache.config = config;
323325
if config_changed {
324326
cache.generation = cache.generation.wrapping_add(1);
325327
}
326-
if !cache.config.enabled {
328+
if !cache.config.enabled || ttl_policy_changed {
327329
cache.entries.clear();
328330
} else if partition_changed {
329331
cache

crates/rmcp/tests/test_tool_disable_notification.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use std::sync::{
99
use rmcp::{
1010
ClientHandler, RoleClient, RoleServer, ServerHandler, ServiceExt,
1111
handler::server::{router::tool::ToolRoute, tool::ToolCallContext},
12-
model::{CallToolResponse, CallToolResult, ServerCapabilities, ServerInfo, Tool},
12+
model::{CacheScope, CallToolResponse, CallToolResult, ServerCapabilities, ServerInfo, Tool},
1313
service::{MaybeSendFuture, NotificationContext},
1414
};
1515
use tokio::sync::{Notify, RwLock};
@@ -19,6 +19,7 @@ struct TestToolServer {
1919
router: Arc<RwLock<rmcp::handler::server::router::tool::ToolRouter<Self>>>,
2020
trigger_disable: Arc<Notify>,
2121
trigger_enable: Arc<Notify>,
22+
list_count: Arc<AtomicUsize>,
2223
}
2324

2425
impl TestToolServer {
@@ -36,6 +37,7 @@ impl TestToolServer {
3637
router: Arc::new(RwLock::new(tool_router)),
3738
trigger_disable: Arc::new(Notify::new()),
3839
trigger_enable: Arc::new(Notify::new()),
40+
list_count: Arc::new(AtomicUsize::new(0)),
3941
}
4042
}
4143
}
@@ -60,11 +62,14 @@ impl ServerHandler for TestToolServer {
6062
_request: Option<rmcp::model::PaginatedRequestParams>,
6163
_context: rmcp::service::RequestContext<RoleServer>,
6264
) -> Result<rmcp::model::ListToolsResult, rmcp::ErrorData> {
65+
self.list_count.fetch_add(1, Ordering::SeqCst);
6366
let router = self.router.read().await;
6467
Ok(rmcp::model::ListToolsResult {
6568
tools: router.list_all(),
6669
..Default::default()
67-
})
70+
}
71+
.with_ttl_ms(60_000)
72+
.with_cache_scope(CacheScope::Public))
6873
}
6974

7075
fn on_initialized(
@@ -128,6 +133,7 @@ async fn test_disable_enable_sends_tool_list_changed() {
128133
let server = TestToolServer::new();
129134
let trigger_disable = server.trigger_disable.clone();
130135
let trigger_enable = server.trigger_enable.clone();
136+
let list_count = server.list_count.clone();
131137

132138
let client = TestToolClient::new();
133139
let notification_count = client.notification_count.clone();
@@ -140,6 +146,11 @@ async fn test_disable_enable_sends_tool_list_changed() {
140146

141147
let tools = client_service.peer().list_tools(None).await.unwrap();
142148
assert_eq!(tools.tools.len(), 2);
149+
assert_eq!(list_count.load(Ordering::SeqCst), 1);
150+
151+
let cached_tools = client_service.peer().list_tools(None).await.unwrap();
152+
assert_eq!(cached_tools.tools.len(), 2);
153+
assert_eq!(list_count.load(Ordering::SeqCst), 1);
143154

144155
trigger_disable.notify_one();
145156
tokio::time::timeout(std::time::Duration::from_secs(5), client_notify.notified())
@@ -150,6 +161,7 @@ async fn test_disable_enable_sends_tool_list_changed() {
150161
let tools = client_service.peer().list_tools(None).await.unwrap();
151162
assert_eq!(tools.tools.len(), 1);
152163
assert_eq!(tools.tools[0].name, "tool_b");
164+
assert_eq!(list_count.load(Ordering::SeqCst), 2);
153165

154166
trigger_enable.notify_one();
155167
tokio::time::timeout(std::time::Duration::from_secs(5), client_notify.notified())
@@ -159,6 +171,7 @@ async fn test_disable_enable_sends_tool_list_changed() {
159171

160172
let tools = client_service.peer().list_tools(None).await.unwrap();
161173
assert_eq!(tools.tools.len(), 2);
174+
assert_eq!(list_count.load(Ordering::SeqCst), 3);
162175

163176
client_service.cancel().await.unwrap();
164177
server_handle.abort();

docs/CLIENT_CACHING.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,10 @@ Use `ClientCacheConfig::disabled()` to disable cache reads and writes, or
3434
`with_max_entries()` bounds the in-memory store; the default is 512 entries and
3535
a value of zero removes the limit.
3636

37-
Cache keys include the method and result-affecting parameters: the cursor for
38-
paginated list methods and the URI for resource reads. MRTR retries containing
39-
`inputResponses` or `requestState` are never cached. List-change notifications
37+
Cache keys include the method and all currently result-affecting parameters: the
38+
cursor and `_meta` for paginated list methods, and the URI plus `_meta` for resource
39+
reads. MRTR retries containing `inputResponses` or `requestState` are never cached.
40+
A response that omits `cacheScope` is treated as private rather than made shareable. List-change notifications
4041
invalidate every cached page for the corresponding method, while resource
4142
update notifications invalidate only the matching URI. When a cursor request
4243
fails, all cached pages for that list method are discarded so the next walk can

0 commit comments

Comments
 (0)