feat(mcp): revalidate cached local-path indexes on query#78
Conversation
Port semble#211: an MCP session-cache entry for a local path is now rechecked against its live source fingerprint on each `get`, so an entry is rebuilt once its files change instead of serving stale results for the lifetime of the server. Git URLs (URL+ref keyed) are never revalidated. - index.rs: extract `source_fingerprint(source, content)` (the same content-hash oracle load_or_build_index already used) as a pub helper; load_or_build_index now calls it too (no behavior change). - mcp.rs: IndexCache entries carry the build-time fingerprint plus a `revalidate_after` cooldown (build duration x MIN_REVALIDATE_FACTOR=3), so a slow-to-build repo isn't re-walked on every query. On get, past the cooldown, a changed fingerprint evicts the entry -> rebuild. The LoadOrBuild seam gains a `fingerprint` method (None for git URLs). - Tests: cache_revalidates_stale_local_path (cooldown skip + stale rebuild + fresh-match reuse) and cache_git_url_not_revalidated. Refs #74
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a cache revalidation mechanism for local source trees using content-hash fingerprints and a build-time-scaled cooldown period to prevent frequent disk I/O. The review feedback correctly identifies a performance issue: when a revalidation check succeeds (the fingerprint matches), the cooldown timer is not reset, causing subsequent queries to repeatedly perform fingerprint checks. The reviewer suggests adding a revalidate_cooldown field to CacheEntry and updating the revalidate_after timestamp upon successful revalidation.
| struct CacheEntry { | ||
| index: Arc<CspIndex>, | ||
| /// Source fingerprint captured at build time; `None` for git URLs (never | ||
| /// revalidated). A live fingerprint that differs means the entry is stale. | ||
| fingerprint: Option<String>, | ||
| /// Staleness re-checks for this entry are skipped until this instant, so a | ||
| /// slow-to-build repo isn't re-walked on every query. | ||
| revalidate_after: Instant, | ||
| } |
There was a problem hiding this comment.
[HIGH] CacheEntry에 쿨다운 기간(revalidate_cooldown) 필드 추가 필요
Problem: 캐시 항목 재검증 시 지문이 일치하더라도 쿨다운 타이머가 재설정되지 않는 문제를 해결하기 위해, 각 캐시 항목 생성 시 계산된 쿨다운 기간을 저장할 필드가 필요합니다.
Rationale: 쿨다운 기간을 구조체에 저장해 두면, 재검증 성공 시 매번 빌드 시간을 다시 측정할 필요 없이 즉시 다음 재검증 시점을 갱신할 수 있습니다.
Suggestion: CacheEntry 구조체에 revalidate_cooldown: std::time::Duration 필드를 추가합니다.
| struct CacheEntry { | |
| index: Arc<CspIndex>, | |
| /// Source fingerprint captured at build time; `None` for git URLs (never | |
| /// revalidated). A live fingerprint that differs means the entry is stale. | |
| fingerprint: Option<String>, | |
| /// Staleness re-checks for this entry are skipped until this instant, so a | |
| /// slow-to-build repo isn't re-walked on every query. | |
| revalidate_after: Instant, | |
| } | |
| struct CacheEntry { | |
| index: Arc<CspIndex>, | |
| /// Source fingerprint captured at build time; `None` for git URLs (never | |
| /// revalidated). A live fingerprint that differs means the entry is stale. | |
| fingerprint: Option<String>, | |
| /// Staleness re-checks for this entry are skipped until this instant, so a | |
| /// slow-to-build repo isn't re-walked on every query. | |
| revalidate_after: Instant, | |
| /// Cooldown duration to apply when resetting the revalidation timer. | |
| revalidate_cooldown: std::time::Duration, | |
| } |
| // Past the cooldown, re-check a local entry against the live fingerprint. | ||
| let stored_fingerprint = match self.tasks.get(&key) { | ||
| Some(entry) | ||
| if entry.fingerprint.is_some() && Instant::now() >= entry.revalidate_after => | ||
| { | ||
| Some(entry.fingerprint.clone()) | ||
| } | ||
| _ => None, | ||
| }; | ||
| if let Some(stored) = stored_fingerprint { | ||
| if self.seam.fingerprint(source, &self.content) != stored { | ||
| self.tasks.shift_remove(&key); | ||
| } | ||
| } | ||
|
|
||
| if let Some(entry) = self.tasks.shift_remove(&key) { | ||
| // Fresh (or git) → serve; touch for LRU (re-insert at the recent end). | ||
| let index = entry.index.clone(); | ||
| self.tasks.insert(key, entry); | ||
| return Ok(index); | ||
| } | ||
|
|
||
| // LRU eviction: drop the oldest entry when full. | ||
| if self.tasks.len() >= CACHE_MAX_SIZE { | ||
| self.tasks.shift_remove_index(0); | ||
| } | ||
|
|
||
| let start = Instant::now(); | ||
| let index = Arc::new(self.seam.load_or_build(source, &self.content, git_ref)?); | ||
| self.tasks.insert(key, index.clone()); | ||
| let build_elapsed = start.elapsed(); | ||
| let fingerprint = self.seam.fingerprint(source, &self.content); | ||
| self.tasks.insert( | ||
| key, | ||
| CacheEntry { | ||
| index: index.clone(), | ||
| fingerprint, | ||
| revalidate_after: Instant::now() + build_elapsed * MIN_REVALIDATE_FACTOR, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
[HIGH] 캐시 재검증 성공 시 쿨다운 타이머 갱신 및 신규 항목 생성 시 쿨다운 저장
Problem: 현재 구현에서는 로컬 경로 캐시의 지문(fingerprint) 재검증이 성공(지문 일치)하더라도 revalidate_after 시간이 업데이트되지 않습니다. 이로 인해 최초 쿨다운 만료 이후에는 매 쿼리마다 디스크 I/O와 지문 계산이 강제되어 심각한 성능 저하가 발생합니다. 또한 신규 캐시 항목 생성 시 쿨다운 기간을 함께 저장해야 합니다.
Rationale: 쿨다운 메커니즘이 정상적으로 작동하려면 재검증 성공 시 다음 검증 시점(revalidate_after)을 현재 시간 기준으로 다시 뒤로 미뤄야(갱신해야) 합니다.
Suggestion: 지문이 일치하는 경우 revalidate_after를 Instant::now() + entry.revalidate_cooldown으로 갱신하고, 신규 캐시 생성 시 revalidate_cooldown을 함께 설정하도록 수정합니다.
// Past the cooldown, re-check a local entry against the live fingerprint.
let stored_fingerprint = match self.tasks.get(&key) {
Some(entry)
if entry.fingerprint.is_some() && Instant::now() >= entry.revalidate_after =>
{
Some(entry.fingerprint.clone())
}
_ => None,
};
if let Some(stored) = stored_fingerprint {
if self.seam.fingerprint(source, &self.content) != stored {
self.tasks.shift_remove(&key);
} else if let Some(entry) = self.tasks.get_mut(&key) {
entry.revalidate_after = Instant::now() + entry.revalidate_cooldown;
}
}
if let Some(entry) = self.tasks.shift_remove(&key) {
// Fresh (or git) → serve; touch for LRU (re-insert at the recent end).
let index = entry.index.clone();
self.tasks.insert(key, entry);
return Ok(index);
}
// LRU eviction: drop the oldest entry when full.
if self.tasks.len() >= CACHE_MAX_SIZE {
self.tasks.shift_remove_index(0);
}
let start = Instant::now();
let index = Arc::new(self.seam.load_or_build(source, &self.content, git_ref)?);
let build_elapsed = start.elapsed();
let fingerprint = self.seam.fingerprint(source, &self.content);
let cooldown = build_elapsed * MIN_REVALIDATE_FACTOR;
self.tasks.insert(
key,
CacheEntry {
index: index.clone(),
fingerprint,
revalidate_after: Instant::now() + cooldown,
revalidate_cooldown: cooldown,
},
);![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/csp/src/mcp.rs">
<violation number="1" location="crates/csp/src/mcp.rs:150">
P2: Unchanged local repositories are re-walked on every query after the first cooldown expiry, defeating the cooldown and adding synchronous latency under the cache mutex. Refresh `revalidate_after` after a matching fingerprint check (retain the cooldown duration in the entry) so the next check is deferred too.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as MCP Client
participant Server as MCP Server
participant Cache as IndexCache
participant Seam as LoadOrBuild (DiskLoadOrBuild)
participant FP as source_fingerprint()
participant FS as File System
Client->>Server: search/find_related(source, query)
Server->>Cache: get(source, git_ref)
Cache->>Cache: computeKey(source, git_ref)
Cache->>Cache: look up entry in index map
alt entry exists AND local path (fingerprint is Some) AND cooldown elapsed
Cache->>Seam: fingerprint(source, content)
Seam->>FP: source_fingerprint(source, content)
FP->>FS: collect_source_files() + compute_content_hash()
FS-->>FP: hash value
FP-->>Seam: Some(hash)
Seam-->>Cache: fingerprint value
Cache->>Cache: compare with stored fingerprint
alt mismatch → stale
Cache->>Cache: shift_remove entry (evict)
else match
Note over Cache: entry stays valid
end
else
Note over Cache: skip revalidation (cooldown active or git URL)
end
Cache->>Cache: shift_remove key (if not already evicted)
alt entry still present
Cache->>Cache: re-insert for LRU touch
Cache-->>Server: Arc<CspIndex>
else entry evicted or missing
Note over Cache: LRU eviction if at capacity
Cache->>Seam: load_or_build(source, content, git_ref)
Seam->>Seam: build index (walk filesystem / clone repo)
Seam-->>Cache: CspIndex
Note over Cache: record build_duration = now - start
Cache->>Seam: fingerprint(source, content) again
Seam-->>Cache: live fingerprint (None for git)
Cache->>Cache: create CacheEntry(fingerprint, revalidate_after = now + build_duration × 3)
Cache->>Cache: insert into cache
Cache-->>Server: Arc<CspIndex>
end
Server-->>Client: search results
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| _ => None, | ||
| }; | ||
| if let Some(stored) = stored_fingerprint { | ||
| if self.seam.fingerprint(source, &self.content) != stored { |
There was a problem hiding this comment.
P2: Unchanged local repositories are re-walked on every query after the first cooldown expiry, defeating the cooldown and adding synchronous latency under the cache mutex. Refresh revalidate_after after a matching fingerprint check (retain the cooldown duration in the entry) so the next check is deferred too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/csp/src/mcp.rs, line 150:
<comment>Unchanged local repositories are re-walked on every query after the first cooldown expiry, defeating the cooldown and adding synchronous latency under the cache mutex. Refresh `revalidate_after` after a matching fingerprint check (retain the cooldown duration in the entry) so the next check is deferred too.</comment>
<file context>
@@ -100,22 +130,52 @@ impl<S: LoadOrBuild> IndexCache<S> {
+ _ => None,
+ };
+ if let Some(stored) = stored_fingerprint {
+ if self.seam.fingerprint(source, &self.content) != stored {
+ self.tasks.shift_remove(&key);
+ }
</file context>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds content-hash-based revalidation to the MCP session's
Confidence Score: 5/5The cache revalidation logic is correct for all covered cases; the only rough edge is a test that relies on timing coincidence rather than an explicit future deadline. The production logic is sound: git URLs are never revalidated, local paths are evicted on fingerprint mismatch after the cooldown, and failed builds remain uncached. The single finding is confined to a test assertion whose within-cooldown premise is not enforced by the test setup. crates/csp/src/mcp.rs — the cache_revalidates_stale_local_path test needs an explicit future revalidate_after to make the within-cooldown assertion deterministic. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["get(source, git_ref)"] --> B["compute_key"]
B --> C{"Entry in tasks?"}
C -- No --> H["LRU eviction if full"]
C -- Yes --> D{"fingerprint.is_some() AND now >= revalidate_after?"}
D -- No --> G["serve from cache (LRU touch)"]
D -- Yes --> E["compute live fingerprint"]
E --> F{"live == stored?"}
F -- Yes --> G
F -- No --> I["evict (shift_remove)"]
I --> H
H --> J["seam.load_or_build()"]
J -- Error --> K["return Err"]
J -- Ok --> L["seam.fingerprint(source)"]
L --> M["insert CacheEntry {index, fingerprint, revalidate_after}"]
M --> N["return Ok(index)"]
G --> N
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["get(source, git_ref)"] --> B["compute_key"]
B --> C{"Entry in tasks?"}
C -- No --> H["LRU eviction if full"]
C -- Yes --> D{"fingerprint.is_some() AND now >= revalidate_after?"}
D -- No --> G["serve from cache (LRU touch)"]
D -- Yes --> E["compute live fingerprint"]
E --> F{"live == stored?"}
F -- Yes --> G
F -- No --> I["evict (shift_remove)"]
I --> H
H --> J["seam.load_or_build()"]
J -- Error --> K["return Err"]
J -- Ok --> L["seam.fingerprint(source)"]
L --> M["insert CacheEntry {index, fingerprint, revalidate_after}"]
M --> N["return Ok(index)"]
G --> N
Reviews (2): Last reviewed commit: "chore: merge main into amondnet/mcp-reva..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Some(stored) = stored_fingerprint { | ||
| if self.seam.fingerprint(source, &self.content) != stored { | ||
| self.tasks.shift_remove(&key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Once revalidate_after has elapsed and the fingerprint comparison passes (entry is not evicted), the field is left pointing to the same expired Instant. Every subsequent call to get satisfies Instant::now() >= entry.revalidate_after immediately, so source_fingerprint (and the full tree walk it triggers) runs on every query from that point on — the stated goal of "don't re-walk sooner than 3× the build duration" is only honoured for the very first check window. To restore the throttle after a successful validation, revalidate_after must be advanced when the fingerprints agree. To do that, CacheEntry needs to also carry the original build_duration (e.g. build_duration: std::time::Duration), which can then be used as entry.revalidate_after = Instant::now() + entry.build_duration * MIN_REVALIDATE_FACTOR in the matching arm.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/csp/src/mcp.rs
Line: 149-153
Comment:
**Cooldown never resets after a successful match**
Once `revalidate_after` has elapsed and the fingerprint comparison passes (entry is _not_ evicted), the field is left pointing to the same expired `Instant`. Every subsequent call to `get` satisfies `Instant::now() >= entry.revalidate_after` immediately, so `source_fingerprint` (and the full tree walk it triggers) runs on every query from that point on — the stated goal of "don't re-walk sooner than 3× the build duration" is only honoured for the very first check window. To restore the throttle after a successful validation, `revalidate_after` must be advanced when the fingerprints agree. To do that, `CacheEntry` needs to also carry the original `build_duration` (e.g. `build_duration: std::time::Duration`), which can then be used as `entry.revalidate_after = Instant::now() + entry.build_duration * MIN_REVALIDATE_FACTOR` in the matching arm.
How can I resolve this? If you propose a fix, please make it concise.
|
1 participant
Closes #74. Ports upstream semble#211 (
41c36f7).Problem
The MCP session cache (
IndexCache) served a local path's index for the lifetime of the server without ever rechecking it. If the repo's files changed mid-session,search/find_relatedreturned stale results.Change
index.rs— extractsource_fingerprint(source, content)(the same content-hash oracleload_or_build_indexalready used to validate the on-disk cache) as apubhelper.load_or_build_indexnow calls it too — no behavior change, just deduplication.mcp.rs— eachIndexCacheentry now carries the fingerprint captured at build time plus arevalidate_aftercooldown (build_duration × MIN_REVALIDATE_FACTOR, factor = 3, mirroring upstream's_MIN_REVALIDATE_FACTOR). Onget, once the cooldown has elapsed, the live fingerprint is recomputed; a mismatch evicts the entry so it rebuilds. Git URLs reportNoneand are never revalidated. The cooldown keeps a slow-to-build repo from being re-walked on every query.LoadOrBuildseam gains afingerprint()method.Why the fingerprint (not upstream's mtime+file-set)
Upstream
get_validated_cachere-stats every file against a stored write-time and compares file sets. csp already uses a content hash of the source tree as its cache-validity oracle (stronger and simpler), so revalidation reuses that same signal rather than introducing an mtime path.Verification
cargo fmt --all✅ ·cargo clippy --all-targets --all-features -- -D warnings✅ (clean)cargo test --workspace✅ — 259 lib + 20 CLI tests pass, incl. new:cache_revalidates_stale_local_path— within-cooldown reuse despite drift, then stale rebuild past the cooldown, then fresh-match reusecache_git_url_not_revalidated— git URLs served from cache even when the (local-only) fingerprint changescsp searchstill returns results after theload_or_build_indexrefactor.Notes
tokio::sync::Mutex; the fingerprint recompute is blocking like the existing build path already is (offloading to a worker is a pre-existing, separate concern noted in CLAUDE.md).Summary by cubic
Revalidates cached local-path MCP indexes on query using a content-hash fingerprint with a build-time-scaled cooldown, so results refresh after file changes without re-walking on every query. Git URLs are never revalidated.
New Features
IndexCachestores a build-time fingerprint andrevalidate_after; past the cooldown, a changed live fingerprint evicts and rebuilds.MIN_REVALIDATE_FACTOR(3).LoadOrBuildaddsfingerprint(); git URLs returnNoneand are not revalidated.Refactors
source_fingerprint(source, content)and reused it inload_or_build_index(no behavior change).Written for commit 94ebf5c. Summary will update on new commits.