Rework Cache to evict when untouched for 4 months

DavisVaughan · DavisVaughan · commit d3900b8d896a · 2026-07-08T12:54:13.000-04:00
With a secondary condition of a maximum of 1000 cache entries
diff --git a/crates/oak_cache/src/lib.rs b/crates/oak_cache/src/lib.rs
@@ -1,16 +1,21 @@
 //! A generic, content-agnostic on disk directory cache
 //!
-//! A [`Cache`] is rooted at a single subfolder of the shared cache directory and
-//! holds a fixed number of entries (an LRU `capacity`). Each entry is a directory
-//! keyed by a caller-supplied string and filled by a caller-supplied `populate`
-//! closure. The cache knows nothing about what lives inside an entry. The lock,
-//! completion-sentinel, last-access, and LRU-eviction machinery all live here so
+//! A [`Cache`] is rooted at a single subfolder of the shared cache directory. Each entry
+//! is a directory keyed by a caller-supplied string and filled by a caller-supplied
+//! `populate` closure. The cache knows nothing about what lives inside an entry. The
+//! lock, completion-sentinel, last-access, and eviction machinery all live here so
 //! callers don't reimplement it.
+//!
+//! There are two primary ways to evict an entry:
+//! - An entry untouched for [`DEFAULT_AGE`] is dropped.
+//! - When the total number of entries exceeds [`DEFAULT_CAPACITY`], the least recently
+//!   touched entries are dropped.
 
 mod file_lock;
 
 use std::path::Path;
 use std::path::PathBuf;
+use std::time::Duration;
 use std::time::SystemTime;
 
 use crate::file_lock::FileLock;
@@ -19,14 +24,23 @@ use crate::file_lock::Filesystem;
 /// Name of the root lock file and the per-key lock file.
 const LOCK_FILENAME: &str = ".lock";
 
+/// Default max age for a cache entry
+///
+/// Roughly 4 months in seconds, with 30 days per month
+const DEFAULT_AGE: Duration = Duration::from_secs(4 * 30 * 24 * 60 * 60);
+
+/// Default max number of cache entries
+const DEFAULT_CAPACITY: usize = 1000;
+
 /// Name of the completion sentinel, written last in each cache entry.
 ///
 /// An entry without it is a crashed partial and is removed by [`Cache::clean`].
 ///
-/// Its mtime doubles as the entry's last-access time. [`Cache::get`] bumps it on every
-/// retrieval (this is an atomic operation so we allow it even though we'll only hold a
-/// shared lock on this key's folder) and [`Cache::clean`] evicts the entries with the
-/// oldest mtime first.
+/// Its mtime doubles as the entry's last-access time. [`Cache::get`] bumps it
+/// on every retrieval (this is an atomic operation so we allow it even though
+/// we'll only hold a shared lock on this key's folder). [`Cache::clean`] uses
+/// the mtime both to evict entries older than [`DEFAULT_AGE`] and to evict the
+/// oldest first when the total number of entries exceeds [`DEFAULT_CAPACITY`].
 const COMPLETE_FILENAME: &str = ".complete";
 
 /// A generic on disk directory cache
@@ -83,19 +97,25 @@ impl Cache {
     /// Runs a best-effort [`Cache::clean`] under the exclusive root lock (skipped if
     /// another session holds the shared lock), then holds the shared root lock for the
     /// life of the returned `Cache` so handed-out paths stay valid.
-    pub fn open(root: &str, capacity: usize) -> anyhow::Result<Self> {
-        Self::open_in(cache_dir()?.join(root), capacity)
+    pub fn open(root: &str) -> anyhow::Result<Self> {
+        Self::open_in(cache_dir()?.join(root))
     }
 
     /// Like [`Cache::open`], but rooted at an explicit `root` rather than a subfolder of
     /// the shared cache directory. Only useful for testing against a temp directory.
-    pub fn open_in(root: PathBuf, capacity: usize) -> anyhow::Result<Self> {
+    pub fn open_in(root: PathBuf) -> anyhow::Result<Self> {
+        Self::open_with_options(root, DEFAULT_AGE, DEFAULT_CAPACITY)
+    }
+
+    /// Like [`Cache::open_in`], but with explicit eviction thresholds rather than
+    /// [`DEFAULT_AGE`] and [`DEFAULT_CAPACITY`]. Useful for tests.
+    fn open_with_options(root: PathBuf, age: Duration, capacity: usize) -> anyhow::Result<Self> {
         let root = Filesystem::new(root);
         root.create_dir()?;
 
         // Try to clean. Only possible if no other session holds the shared root lock.
         if let Some(root_lock) = root.try_open_rw_exclusive_create(LOCK_FILENAME)? {
-            if let Err(err) = clean(&root_lock, capacity) {
+            if let Err(err) = clean(&root_lock, age, capacity) {
                 log::warn!(
                     "Failed to clean cache at {root}: {err:?}",
                     root = root.display()
@@ -171,16 +191,14 @@ impl Cache {
     }
 }
 
-/// Removes crashed partials and trims the cache down to `capacity`
+/// Removes crashed partials, evicts entries older than `age`, then trims to `capacity`
 ///
 /// The caller must hold the root exclusive lock, which no one can take while a live
 /// session holds the shared lock, so eviction can never race a reader.
-///
-/// First removes any entry missing its `.complete` sentinel. Then, if the surviving
-/// entries exceed `capacity`, removes the least-recently-accessed ones until `capacity`
-/// remain.
-fn clean(root_lock: &FileLock, capacity: usize) -> anyhow::Result<()> {
+fn clean(root_lock: &FileLock, age: Duration, capacity: usize) -> anyhow::Result<()> {
+    let now = SystemTime::now();
     let root = root_lock.parent();
+
     let mut entries = Vec::new();
 
     for entry in std::fs::read_dir(root)? {
@@ -207,19 +225,34 @@ fn clean(root_lock: &FileLock, capacity: usize) -> anyhow::Result<()> {
             continue;
         }
 
-        entries.push((path, last_access(&complete)));
-    }
+        let accessed = last_access(&complete);
 
-    if entries.len() <= capacity {
-        return Ok(());
+        // Evict entries not accessed within `age`. Treat pathological future `accessed`
+        // times as stale.
+        let stale = now
+            .duration_since(accessed)
+            .map_or(true, |duration_since| duration_since > age);
+
+        if stale {
+            log::trace!("Evicting stale cache entry {}", path.display());
+            remove_dir_all_or_warn(&path);
+            continue;
+        }
+
+        entries.push((path, accessed));
     }
 
-    // Oldest first, then evict down to `capacity`
-    entries.sort_by_key(|(_, accessed)| *accessed);
-    let excess = entries.len() - capacity;
-    for (path, _) in entries.into_iter().take(excess) {
-        log::trace!("Evicting old cache entry {}", path.display());
-        remove_dir_all_or_warn(&path);
+    // Evict the oldest until `capacity` remain
+    if entries.len() > capacity {
+        entries.sort_by_key(|(_, accessed)| *accessed);
+        let excess = entries.len() - capacity;
+        for (path, _) in entries.into_iter().take(excess) {
+            log::trace!(
+                "Evicting cache entry exceeding cache capacity {}",
+                path.display()
+            );
+            remove_dir_all_or_warn(&path);
+        }
     }
 
     Ok(())
@@ -290,12 +323,14 @@ mod tests {
 
     use crate::Cache;
     use crate::COMPLETE_FILENAME;
+    use crate::DEFAULT_AGE;
+    use crate::DEFAULT_CAPACITY;
 
     /// Creates a cache rooted in a fresh temp dir, returning both so the temp dir stays
     /// alive for the test.
-    fn new(capacity: usize) -> (TempDir, Cache) {
+    fn new() -> (TempDir, Cache) {
         let dir = TempDir::new().unwrap();
-        let cache = Cache::open_in(dir.path().join("subfolder"), capacity).unwrap();
+        let cache = Cache::open_in(dir.path().join("subfolder")).unwrap();
         (dir, cache)
     }
 
@@ -326,13 +361,13 @@ mod tests {
 
     #[test]
     fn test_get_miss() {
-        let (_dir, cache) = new(10);
+        let (_dir, cache) = new();
         assert_eq!(cache.get("absent"), None);
     }
 
     #[test]
     fn test_insert_then_get_round_trip() {
-        let (_dir, cache) = new(10);
+        let (_dir, cache) = new();
 
         let inserted = cache.insert("key", write_file("hello")).unwrap().unwrap();
         let got = cache.get("key").unwrap();
@@ -345,7 +380,7 @@ mod tests {
 
     #[test]
     fn test_insert_unavailable_yields_no_entry() {
-        let (_dir, cache) = new(10);
+        let (_dir, cache) = new();
 
         let result = cache.insert("key", |_dir| Ok(false)).unwrap();
         assert_eq!(result, None);
@@ -359,15 +394,15 @@ mod tests {
 
         // Populate one good entry, then forge a crashed partial (no `.complete`).
         {
-            let cache = Cache::open_in(root.clone(), 10).unwrap();
+            let cache = Cache::open_in(root.clone()).unwrap();
             cache.insert("good", write_file("ok")).unwrap();
         }
         let partial = root.join("partial");
         std::fs::create_dir(&partial).unwrap();
         std::fs::write(partial.join("content.txt"), "junk").unwrap();
 
         // Reopening runs `clean`, which removes the partial but keeps the good entry.
-        let cache = Cache::open_in(root, 10).unwrap();
+        let cache = Cache::open_in(root).unwrap();
         assert!(!partial.exists());
         assert!(cache.get("good").is_some());
     }
@@ -378,7 +413,7 @@ mod tests {
         let root = dir.path().join("subfolder");
 
         {
-            let cache = Cache::open_in(root.clone(), 10).unwrap();
+            let cache = Cache::open_in(root.clone()).unwrap();
             cache.insert("good", write_file("ok")).unwrap();
         }
         // A stray file in the cache root that doesn't belong to any entry.
@@ -387,7 +422,7 @@ mod tests {
 
         // Reopening runs `clean`, which removes the stray file but keeps our root
         // `.lock` and the good entry.
-        let cache = Cache::open_in(root.clone(), 10).unwrap();
+        let cache = Cache::open_in(root.clone()).unwrap();
         assert!(!stray.exists());
         assert!(root.join(".lock").exists());
         assert!(cache.get("good").is_some());
@@ -399,27 +434,71 @@ mod tests {
         let root = dir.path().join("subfolder");
         let now = SystemTime::now();
 
-        // Insert four entries under a capacity that won't evict, then stamp their
-        // access times so the ordering is deterministic.
+        // Insert four entries, then stamp their access times so the ordering is
+        // deterministic
         {
-            let cache = Cache::open_in(root.clone(), 10).unwrap();
+            let cache = Cache::open_in(root.clone()).unwrap();
             for (index, key) in ["oldest", "older", "newer", "newest"].iter().enumerate() {
                 let entry = cache.insert(key, write_file(key)).unwrap().unwrap();
-                set_accessed(&entry, now + Duration::from_secs(index as u64));
+                set_accessed(&entry, now - Duration::from_secs(4 - index as u64));
             }
         }
 
         // Reopening with capacity 2 evicts the two least-recently-accessed.
-        let cache = Cache::open_in(root, 2).unwrap();
+        let cache = Cache::open_with_options(root, DEFAULT_AGE, 2).unwrap();
         assert_eq!(cache.get("oldest"), None);
         assert_eq!(cache.get("older"), None);
         assert!(cache.get("newer").is_some());
         assert!(cache.get("newest").is_some());
     }
 
+    #[test]
+    fn test_clean_evicts_stale_entries() {
+        let dir = TempDir::new().unwrap();
+        let root = dir.path().join("subfolder");
+        let now = SystemTime::now();
+
+        // A fresh entry and an entry last accessed an hour ago
+        {
+            let cache = Cache::open_in(root.clone()).unwrap();
+            let fresh = cache.insert("fresh", write_file("fresh")).unwrap().unwrap();
+            set_accessed(&fresh, now);
+            let stale = cache.insert("stale", write_file("stale")).unwrap().unwrap();
+            set_accessed(&stale, now - Duration::from_secs(3600));
+        }
+
+        // Reopening with a half-hour max age evicts the stale entry but keeps the fresh
+        // one
+        let cache =
+            Cache::open_with_options(root, Duration::from_secs(1800), DEFAULT_CAPACITY).unwrap();
+        assert!(cache.get("fresh").is_some());
+        assert_eq!(cache.get("stale"), None);
+    }
+
+    #[test]
+    fn test_clean_evicts_future_mtime() {
+        let dir = TempDir::new().unwrap();
+        let root = dir.path().join("subfolder");
+
+        // Forge an entry whose access time is in the future, as if the wall clock was
+        // reset. It has no meaningful age, so we treat it as broken.
+        {
+            let cache = Cache::open_in(root.clone()).unwrap();
+            let entry = cache
+                .insert("future", write_file("future"))
+                .unwrap()
+                .unwrap();
+            set_accessed(&entry, SystemTime::now() + Duration::from_secs(3600));
+        }
+
+        // Reopening evicts it
+        let cache = Cache::open_with_options(root, DEFAULT_AGE, DEFAULT_CAPACITY).unwrap();
+        assert_eq!(cache.get("future"), None);
+    }
+
     #[test]
     fn test_get_refreshes_accessed() {
-        let (_dir, cache) = new(10);
+        let (_dir, cache) = new();
 
         let entry = cache.insert("key", write_file("hello")).unwrap().unwrap();
 
@@ -434,7 +513,7 @@ mod tests {
 
     #[test]
     fn test_insert_is_complete_after_populate() {
-        let (_dir, cache) = new(10);
+        let (_dir, cache) = new();
         let entry = cache.insert("key", write_file("hello")).unwrap().unwrap();
         assert!(entry.join(COMPLETE_FILENAME).exists());
     }
diff --git a/crates/oak_source/src/lib.rs b/crates/oak_source/src/lib.rs
@@ -22,12 +22,6 @@ use oak_cache::Cache;
 /// Cache version
 const CACHE_VERSION: &str = "v1";
 
-/// LRU capacity for cached CRAN package source trees
-const CRAN_CAPACITY: usize = 200;
-
-/// LRU capacity for cached R version source trees, kept small because each is large
-const R_CAPACITY: usize = 5;
-
 /// Downloads and caches whole package / R version source trees
 ///
 /// Each cache holds its shared root lock for the life of this `SourceCache`, so any path
@@ -41,17 +35,17 @@ pub struct SourceCache {
 impl SourceCache {
     pub fn open() -> anyhow::Result<Self> {
         Ok(Self {
-            cran: Cache::open(&format!("source/{CACHE_VERSION}/cran"), CRAN_CAPACITY)?,
-            r: Cache::open(&format!("source/{CACHE_VERSION}/r"), R_CAPACITY)?,
+            cran: Cache::open(&format!("source/{CACHE_VERSION}/cran"))?,
+            r: Cache::open(&format!("source/{CACHE_VERSION}/r"))?,
         })
     }
 
     /// Like [`SourceCache::open`], but rooted at an explicit `root` rather than the
     /// shared cache directory. Only useful for testing against a temp directory.
     pub fn open_in(root: PathBuf) -> anyhow::Result<Self> {
         Ok(Self {
-            cran: Cache::open_in(root.join("cran"), CRAN_CAPACITY)?,
-            r: Cache::open_in(root.join("r"), R_CAPACITY)?,
+            cran: Cache::open_in(root.join("cran"))?,
+            r: Cache::open_in(root.join("r"))?,
         })
     }
 
diff --git a/crates/oak_srcref/src/lib.rs b/crates/oak_srcref/src/lib.rs
@@ -16,9 +16,6 @@ use sha2::Sha256;
 /// Cache version
 const CACHE_VERSION: &str = "v1";
 
-/// LRU capacity for the cache
-const CACHE_CAPACITY: usize = 200;
-
 /// Recovers and caches an installed package's sources from `srcref` metadata
 #[derive(Debug)]
 pub struct SrcrefCache {
@@ -31,7 +28,7 @@ impl SrcrefCache {
     pub fn open(r: PathBuf) -> anyhow::Result<Self> {
         Ok(Self {
             r,
-            cache: Cache::open(&format!("srcref/{CACHE_VERSION}"), CACHE_CAPACITY)?,
+            cache: Cache::open(&format!("srcref/{CACHE_VERSION}"))?,
         })
     }
 
@@ -41,7 +38,7 @@ impl SrcrefCache {
     pub fn open_in(root: PathBuf, r: PathBuf) -> anyhow::Result<Self> {
         Ok(Self {
             r,
-            cache: Cache::open_in(root, CACHE_CAPACITY)?,
+            cache: Cache::open_in(root)?,
         })
     }
 
