Introduce idle-based eviction for the compiled bundle cache#213
Merged
Conversation
WalkthroughThis PR adds a new ChangesWitness Example Normalization
Bundle Cache Idle Eviction
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/dsperse/src/backend/jstprove.rs (1)
528-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify "only stale entries" are dropped.
The test name
evict_idle_drops_only_stale_entriesimplies mixed stale/fresh entries are exercised, but the body only checksevict_idleon an empty cache, which is a trivial0-result case already covered conceptually by the method'sretainno-op behavior. Consider inserting one entry with an already-elapsed synthetic timestamp (stale) and one freshly-inserted entry (viaload_bundle_cachedor direct insert withInstant::now()), then asserting the stale entry is evicted while the fresh one survives.✅ Suggested stronger test
#[test] fn evict_idle_drops_only_stale_entries() { let backend = JstproveBackend::default(); assert_eq!(backend.evict_idle(std::time::Duration::from_secs(0)), 0); + + let dummy = Arc::new(CompiledCircuit { + circuit: vec![1], + witness_solver: vec![], + metadata: None, + version: None, + }); + { + let mut cache = backend.bundle_cache.lock().unwrap(); + // Stale entry: touched far enough in the past to be older than ttl. + let stale_touch = std::time::Instant::now() - std::time::Duration::from_secs(120); + cache.insert(PathBuf::from("/tmp/stale"), (Arc::clone(&dummy), stale_touch)); + // Fresh entry: touched now. + cache.insert(PathBuf::from("/tmp/fresh"), (dummy, std::time::Instant::now())); + } + let evicted = backend.evict_idle(std::time::Duration::from_secs(60)); + assert_eq!(evicted, 1); + let cache = backend.bundle_cache.lock().unwrap(); + assert_eq!(cache.len(), 1); + assert!(cache.contains_key(Path::new("/tmp/fresh"))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dsperse/src/backend/jstprove.rs` around lines 528 - 532, The test named evict_idle_drops_only_stale_entries is too trivial and does not exercise mixed stale and fresh cache entries. Update the JstproveBackend test to set up one stale entry and one fresh entry, using load_bundle_cached or direct insertion with an Instant::now()-based timestamp, then call evict_idle and assert only the stale entry is removed while the fresh one remains. Keep the test focused on JstproveBackend::evict_idle and the cache state before and after eviction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dsperse/src/backend/jstprove.rs`:
- Around line 65-74: The evict_idle method on jstprove’s bundle cache is
implemented but never scheduled, so idle entries are never actually removed.
Wire JstProve::evict_idle into the existing runtime/background sweep path by
adding a periodic caller in the component that owns the cache and choosing a TTL
consistent with bundle lifetime, so cache cleanup runs automatically instead of
relying on manual invocation.
---
Nitpick comments:
In `@crates/dsperse/src/backend/jstprove.rs`:
- Around line 528-532: The test named evict_idle_drops_only_stale_entries is too
trivial and does not exercise mixed stale and fresh cache entries. Update the
JstproveBackend test to set up one stale entry and one fresh entry, using
load_bundle_cached or direct insertion with an Instant::now()-based timestamp,
then call evict_idle and assert only the stale entry is removed while the fresh
one remains. Keep the test focused on JstproveBackend::evict_idle and the cache
state before and after eviction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f798d44f-2ab0-452f-9d27-d1d4bee3cf4f
📒 Files selected for processing (3)
crates/dsperse/examples/witness_group.rscrates/dsperse/examples/witness_single.rscrates/dsperse/src/backend/jstprove.rs
Comment on lines
+65
to
+74
| pub fn evict_idle(&self, ttl: std::time::Duration) -> usize { | ||
| let mut cache = match self.bundle_cache.lock() { | ||
| Ok(cache) => cache, | ||
| Err(e) => e.into_inner(), | ||
| }; | ||
| let now = std::time::Instant::now(); | ||
| let before = cache.len(); | ||
| cache.retain(|_, (_, touched)| now.duration_since(*touched) < ttl); | ||
| before - cache.len() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search the repo for any caller that periodically invokes evict_idle.
rg -n -C3 '\bevict_idle\s*\(' --type=rustRepository: inference-labs-inc/dsperse
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files mentioning evict_idle ==\n'
rg -n -C 2 '\bevict_idle\s*\(' --type=rust || true
printf '\n== Files mentioning bundle_cache ==\n'
rg -n -C 2 '\bbundle_cache\b' crates/dsperse/src --type=rust || true
printf '\n== Files mentioning sweep/periodic/timer/spawn in dsperse src ==\n'
rg -n -C 2 '\b(sweep|periodic|timer|interval|spawn|timeout)\b' crates/dsperse/src --type=rust || trueRepository: inference-labs-inc/dsperse
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Rust files in crates/dsperse/src ==\n'
git ls-files 'crates/dsperse/src/**/*.rs' 'crates/dsperse/src/*.rs'
printf '\n== Search for cache eviction / sweep related symbols ==\n'
rg -n -C 2 '\b(evict|sweep|ttl|idle|cache)\b' crates/dsperse/src --type=rust || trueRepository: inference-labs-inc/dsperse
Length of output: 23693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search repo-wide for background-task patterns ==\n'
rg -n -C 2 '\b(tokio::spawn|spawn\(|interval\(|sleep\(|spawn_blocking|Recurring|cron|scheduler)\b' . --type=rust || true
printf '\n== Search repo-wide for eviction / sweep names ==\n'
rg -n -C 2 '\b(evict_idle|sweep|sweeper|cleanup|prune)\b' . --type=rust || trueRepository: inference-labs-inc/dsperse
Length of output: 4446
Wire evict_idle into a periodic sweep.
I couldn’t find any caller or scheduler using this method, so the bundle cache still has no idle eviction path and can keep growing unbounded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/dsperse/src/backend/jstprove.rs` around lines 65 - 74, The evict_idle
method on jstprove’s bundle cache is implemented but never scheduled, so idle
entries are never actually removed. Wire JstProve::evict_idle into the existing
runtime/background sweep path by adding a periodic caller in the component that
owns the cache and choosing a TTL consistent with bundle lifetime, so cache
cleanup runs automatically instead of relying on manual invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The compiled bundle cache retained every circuit ever loaded for the process lifetime, which on long-running verifiers grows toward the full circuit corpus and pins host memory. Measurement of production verification traffic shows bundle reuse is a burst phenomenon: 97 percent of reuse occurs within sixty seconds of the prior touch, driven by tiles of the same run verifying together, after which a bundle goes cold. Direct reads from the backing store reload the largest bundles in a fifth of a second and the median bundle in single-digit milliseconds, so retention beyond the burst window buys nothing.
Cache entries now carry a last-touched timestamp refreshed on hit, and an eviction method drops entries idle beyond a caller-supplied duration. Callers that sweep periodically retain the measured hit rate while steady-state cache size tracks the actively verifying working set instead of the corpus.
Summary by CodeRabbit
New Features
Bug Fixes
Tests