Repository navigation
fix(serve): answer tools/list from cache during background refresh - #124
Conversation
There was a problem hiding this comment.
Nice catch, @ozkanerbatuhan. Blocking tools/list on rediscovery right after a restart throws away the whole point of the persistent cache, and has_backends_without_tools keeps the partial-cache case going through discovery, which is exactly the line I'd want. The test reproducing the window is great, 5.4 s down to under a second is a real win.
One mechanical thing: main grew an extra field on protocol::Tool since you branched, so the new test no longer compiles on top of main (E0063: missing field extra, checked locally merging this branch into current main). Could you rebase and add it? After that I'm happy to merge.
| max_idle_timeout: None, | ||
| }, | ||
| ); | ||
| let tool = Tool { |
There was a problem hiding this comment.
Tool has an extra: ExtraFields field on main now, so this initializer fails to compile after a rebase. Something like this should do it:
let tool = Tool {
name: "lookup".to_string(),
description: Some("Lookup".to_string()),
input_schema: None,
annotations: None,
extra: Default::default(),
};d752243 to
f659fbf
Compare
After startup the proxy loads tools from the persistent cache, then the background refresh calls `reset_cache_loaded_for_refresh`, which removes every cache-loaded backend from `discovered_backends`. A `tools/list` arriving in that window saw `has_undiscovered_backends() == true` and blocked on `discover_pending_backends` until every backend had been spawned and listed - exactly the wait the cache exists to avoid. With 9 backends that is 5-12 s per proxy restart, long enough for a client's tools fetch to time out. `tools/list` now asks `has_backends_without_tools()`: like `has_undiscovered_backends`, but a backend whose tools are already registered from the cache (Disconnected with non-empty `cached_tools`) does not force a blocking discovery. Backends with no cache entry still do, so the partial-cache case keeps working (`tools_list_discovers_backends_missing_ from_a_partial_cache` unchanged and passing). The refresh itself still runs in the background and updates the list. Measured (HTTP mode, 9 stdio backends): tools/list right after a restart went from 5.4 s to 0.1-0.7 s.
f659fbf to
b3e487c
Compare
There was a problem hiding this comment.
Rebased and the extra field is in, thanks @ozkanerbatuhan. I ran it on top of main together with #122 and the full suite passes locally (1139), CI is green too. Going from a 5s first tools/list after a restart to under a second is the kind of fix people feel without knowing why, and the test pins exactly the window that was hurting. Merging.
Part of #121 (item 3); affects all platforms.
After startup,
reset_cache_loaded_for_refreshdrops cache-loaded backends fromdiscovered_backends, so atools/listin that window blocked on discovering every backend.tools/listnow useshas_backends_without_tools(), which ignores backends whose tools are already registered from the cache; backends with no cache entry still trigger discovery (the existing partial-cache test is unchanged and passes).New test
test_tools_list_serves_cache_during_background_refreshfails without the fix and passes with it. Measured with 9 stdio backends over HTTP: firsttools/listafter a restart 5.4 s → 0.1–0.7 s.🤖 Generated with Claude Code