perf(tui): compile an MCP resource template once instead of per call (#6213 T7) - #6266
Conversation
…6213 T7) `resource_uri_matches_template` is an authorization check that runs per URI per advertised template, and it rebuilt the anchored pattern string and compiled a fresh `Regex` on every one of those calls — while the template itself is fixed by the server's listing. Split the pattern builder out as `resource_template_pattern` and add `compiled_resource_template`, which caches the compiled program by template string. `None` still means "matches nothing", so a template using an operator this RFC 6570 subset does not implement stays uncallable rather than over-matching. Part of #6213 (item T7). T5, T6 and T4 remain, as does T1's id-keying half. Verification: cargo check -p codewhale-tui --all-targets --all-features --locked (clean) cargo clippy -p codewhale-tui --all-targets --all-features --locked -- \ -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments \ -A clippy::unnecessary_map_or (clean) test result: ok. 275 passed; 0 failed; 1 ignored; 0 measured; 12531 filtered out (mcp::) test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 12807 filtered out (resource_uri_template_matching_is_anchored_and_fail_closed) Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 2m 57s —— View job Review of PR #6266 — compile MCP resource template once instead of per call
SummaryThe core refactor is correct: Finding (posted inline on
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn compiled_resource_template(template: &str) -> Option<Arc<regex::Regex>> { | ||
| static CACHE: std::sync::OnceLock< | ||
| std::sync::Mutex<HashMap<String, Option<Arc<regex::Regex>>>>, | ||
| > = std::sync::OnceLock::new(); | ||
| let cache = CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new())); | ||
| let mut cache = cache | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| cache | ||
| .entry(template.to_string()) | ||
| .or_insert_with(|| { | ||
| resource_template_pattern(template) | ||
| .and_then(|pattern| regex::Regex::new(&pattern).ok()) | ||
| .map(Arc::new) | ||
| }) | ||
| .clone() | ||
| } |
There was a problem hiding this comment.
CACHE here is a process-lifetime static keyed by the raw uriTemplate string, with no eviction and no size cap. template.uri_template (McpResourceTemplate::uri_template, populated in discover_resource_templates) is server-controlled content — a single discovery call is bounded to 4,096 items by McpCatalogBudget, but that budget resets every time a server (re)connects, and this cache never resets or shrinks. A misbehaving/malicious MCP server that varies its advertised templates across reconnects (or a long session that connects to many different servers) grows this map for the life of the process with no bound.
This crate already has the exact solved pattern for "small cache of things derived from untrusted/remote input" in crates/tui/src/tools/web/cache.rs: an lru::LruCache (already a workspace dependency, lru = "0.18" in Cargo.toml) behind a parking_lot::Mutex (already imported into this very file as RwLock), with a fixed entry cap. Reusing that shape here — a bounded LruCache<String, Option<Arc<Regex>>> — would cap worst-case memory instead of leaving it open-ended, and would also drop the std::sync::Mutex poisoning-recovery boilerplate (unwrap_or_else(|poisoned| poisoned.into_inner())) since parking_lot::Mutex doesn't poison.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 2 file patches, sha256:fd94cb076522c8ae43339682d6348c63494b11ce36e27c16937108df12a0132d.
Pass 1: This pass splits resource_uri_matches_template into resource_template_pattern (pattern builder, unchanged in what it accepts) and compiled_resource_template (a OnceLock<Mutex<HashMap<String, Option<Arc<Regex>>>>> cache), plus a new test. The authorization semantics are preserved: the early return false paths became ?/None, and a cached None still yields false, so the check remains anchored and fail-closed. The remaining concerns are with the new cache structure rather than with the matching decision: it is a process-global static with no eviction, and every call still allocates a String key and takes a global lock.
Findings
- [WARNING] Global template-regex cache is never evicted (unbounded memory retention) (
crates/tui/src/mcp.rs:1286)
compiled_resource_templateinserts every distinct template string into astaticHashMapthat is never pruned, keeping both the keyStringand either a compiledRegexor a negativeNone. The key is server-supplied (uriTemplatefromresources/templates/list), and listings can be re-fetched or a server reconnected with different templates, so a long-running TUI session retains one entry per template string ever observed — including templates belonging to servers that are no longer connected — for the life of the process. Triggering path: repeated listing refresh where the server returns varyinguriTemplatevalues, or a server that advertises many templates; the map grows monotonically and itsRegexvalues are retained forever. Impact: memory growth in a long-lived interactive process, with no bound and no way to release it. Smallest useful fix: bound or scope the cache (an LRU/capacity-limited structure, invalidation when a server's template listing is replaced/disconnected), instead of a process-global static keyed by arbitrary input. - [INFO] Every cache lookup allocates a String key and takes the global mutex (
crates/tui/src/mcp.rs:1294)
cache.entry(template.to_string())builds an ownedStringon every call, including hits, and does so while holding the process-wideMutex; first-time compilations of other templates also run insideor_insert_withwith the lock held, serialising them behind each other. This matters only because the change is explicitly a per-call hot-path optimisation: acache.get(template)fast path (theHashMap<String, _>key is reachable by&strthroughBorrow<str>) would avoid the per-call allocation, and compiling outside the lock would keep unrelated lookups from blocking. The impact is small relative to theRegex::newthat this PR removes, so this is informational, not a correctness defect.
Suggestions
crates/tui/src/mcp.rs:1294— Consider acache.get(template)fast path before theentry(...)insert so cache hits avoid allocating a freshStringkey, and consider bounding or invalidating the cache (per-server scope, capacity limit, or clear on listing refresh) so that template strings from servers that are gone are not retained for the process lifetime. Both require structural judgement, so no literal replacement is given.
Assessment
Pass 1: No correctness regression in the matching decision is visible in the diff: resource_template_pattern keeps the same literals, validation of variable names, operator handling and anchoring, the ? and None early exits are equivalent to the previous return false, and a cached None (unrepresentable or uncompilable template) still makes resource_uri_matches_template return false, so fail-closed behaviour is preserved. The new test's four assertions match that logic. The material findings are the never-evicted process-global cache (bounded input diversity needed for impact) and the per-call key allocation under a global lock, which partially offsets the stated goal. Unverified here: whether HashMap and Arc are in scope in both mcp.rs and mcp/tests.rs (the module context files were not supplied), and the claim of a clean cargo check/clippy run — no build, test or runtime execution was performed for this review. Also out of scope for this pass: the pattern builder itself, which is unchanged, still emits .+-based atoms for {+name} expansions, so any pathological backtracking surface there pre-dates this diff.
Advisory review by Codewhale (codewhale review --pr 6266 --post, head 6babc092d45a68852be16bba4cda1eeaefb2f41d). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// fixed by the server's listing, so compiling it on every call was pure | ||
| /// repetition. `None` still means "matches nothing" (#6213 T7). | ||
| fn compiled_resource_template(template: &str) -> Option<Arc<regex::Regex>> { | ||
| static CACHE: std::sync::OnceLock< |
There was a problem hiding this comment.
[WARNING] Global template-regex cache is never evicted (unbounded memory retention)
compiled_resource_template inserts every distinct template string into a static HashMap that is never pruned, keeping both the key String and either a compiled Regex or a negative None. The key is server-supplied (uriTemplate from resources/templates/list), and listings can be re-fetched or a server reconnected with different templates, so a long-running TUI session retains one entry per template string ever observed — including templates belonging to servers that are no longer connected — for the life of the process. Triggering path: repeated listing refresh where the server returns varying uriTemplate values, or a server that advertises many templates; the map grows monotonically and its Regex values are retained forever. Impact: memory growth in a long-lived interactive process, with no bound and no way to release it. Smallest useful fix: bound or scope the cache (an LRU/capacity-limited structure, invalidation when a server's template listing is replaced/disconnected), instead of a process-global static keyed by arbitrary input.
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| cache | ||
| .entry(template.to_string()) |
There was a problem hiding this comment.
[INFO] Every cache lookup allocates a String key and takes the global mutex
cache.entry(template.to_string()) builds an owned String on every call, including hits, and does so while holding the process-wide Mutex; first-time compilations of other templates also run inside or_insert_with with the lock held, serialising them behind each other. This matters only because the change is explicitly a per-call hot-path optimisation: a cache.get(template) fast path (the HashMap<String, _> key is reachable by &str through Borrow<str>) would avoid the per-call allocation, and compiling outside the lock would keep unrelated lookups from blocking. The impact is small relative to the Regex::new that this PR removes, so this is informational, not a correctness defect.
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| cache | ||
| .entry(template.to_string()) |
There was a problem hiding this comment.
Consider a cache.get(template) fast path before the entry(...) insert so cache hits avoid allocating a fresh String key, and consider bounding or invalidating the cache (per-server scope, capacity limit, or clear on listing refresh) so that template strings from servers that are gone are not retained for the process lifetime. Both require structural judgement, so no literal replacement is given.
Summary
Part of #6213 (item T7).
No-Issue: partial work on #6213 — the MCP template-cache item only; T4, T5 and T6 remain, so that issue must stay open.
What changed
resource_uri_matches_templateis an authorization check (crates/tui/src/mcp.rs:1303): it decides whether a URI may be read against a server's advertiseduriTemplate. It runs per URI per template, and each call rebuilt the anchored patternStringand compiled a freshRegex— while the template is fixed by the server's listing and the URI is the only thing that varies.Two functions now, with the split chosen so the fail-closed semantics stay obvious:
resource_template_pattern(template) -> Option<String>— the previous pattern builder, unchanged in what it accepts.Noneis still "this template cannot be expressed by our RFC 6570 subset".compiled_resource_template(template) -> Option<Arc<Regex>>— compiles that pattern once per template string and reuses it.The authorization decision is untouched: same pattern, same compile-or-refuse, same anchoring. A template using an operator we do not implement (
{?query}) still matches nothing rather than over-matching, and that is now pinned by a test.Verification
The new test pins all four behaviours the check depends on — literal anchor,
{id}not crossing/,{+path}crossing/, and fail-closed on an unimplemented operator and an unterminated expression — plus that the compile is reused (Arc::ptr_eq).Local only, macOS/aarch64. No hosted CI claim, and no benchmark: the win is argued from the removed compile, not measured.