From b6f138b90d67c9531b5a58993e8ec750e5eec57f Mon Sep 17 00:00:00 2001 From: asepharyana Date: Sat, 29 Aug 2026 01:29:40 +0700 Subject: [PATCH] fix(cache,storage): harden cache bounds + atomic disk writes - cache: guard MokaL1::new(0) with panic; add MemoryCache::with_max_entries bounded LRU eviction (oldest evicted past cap) + docs warning about unbounded default growth. Verifies moka treats max_capacity=0 as a permanent no-insert sentinel. - storage: make LocalFileStorage::put atomic via temp-file + fsync + rename; cleans up temp on write failure; no leftover .tmp-* on disk after success. Tests: 17 cache tests (incl bounded_cache_evicts_oldest, zero_max_panics), 6 storage tests (incl put_leaves_no_temp_file). Full workspace: clippy 0 warnings, all tests green. --- crates/mytheclipse-cache/src/memory.rs | 94 ++++++++++++++++++++-- crates/mytheclipse-cache/src/moka_cache.rs | 28 ++++++- crates/mytheclipse-storage/src/local.rs | 44 +++++++++- 3 files changed, 154 insertions(+), 12 deletions(-) diff --git a/crates/mytheclipse-cache/src/memory.rs b/crates/mytheclipse-cache/src/memory.rs index ba8f2cc..2666f50 100644 --- a/crates/mytheclipse-cache/src/memory.rs +++ b/crates/mytheclipse-cache/src/memory.rs @@ -4,7 +4,7 @@ //! Entries are lazily expired on access by comparing against `Instant`; a //! monotonic clock keeps TTLs robust against wall-clock discontinuities. -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; @@ -15,14 +15,34 @@ use crate::traits::{Cache, CacheError}; /// A wrapping entry: `None` expiry means the value never expires. type Entry = (Vec, Option); -/// An in-process [`Cache`] implementation for L1 caching. -#[derive(Clone, Default)] +/// An in-process [`Cache`] for L1 caching. +/// +/// Default instance is **unbounded** — it grows until the process runs out of +/// memory. For memory-constrained workloads, use [`MemoryCache::with_max_entries`] +/// to install a simple LRU-style cap: when the cap is exceeded, the oldest +/// (least-recently-inserted) entry is evicted. +#[derive(Debug, Clone)] pub struct MemoryCache { inner: Arc>>, + /// When `Some(n)`, the cache refuses more than `n` live entries and evicts + /// the oldest on overflow. `None` = unbounded (legacy default). + max_entries: Option, + /// Insertion order, for eviction when `max_entries` is set. + order: Arc>>, +} + +impl Default for MemoryCache { + fn default() -> Self { + Self { + inner: Arc::new(Mutex::new(HashMap::new())), + max_entries: None, + order: Arc::new(Mutex::new(VecDeque::new())), + } + } } impl MemoryCache { - /// Builds an empty in-memory cache. + /// Builds an empty in-memory cache (unbounded by default). pub fn new() -> Self { Self::default() } @@ -32,6 +52,23 @@ impl MemoryCache { self.inner.lock().unwrap().reserve(capacity); self } + + /// Installs a bounded LRU-style cap. When the cache exceeds `max`, the + /// oldest (least-recently-inserted) entry is evicted on each `set`. + /// + /// This is the recommended constructor for production L1 caches: a + /// [`MemoryCache::new()`] (unbounded) left unmanaged can grow without bound + /// and exhaust process memory. + pub fn with_max_entries(mut self, max: usize) -> Self { + assert!(max > 0, "mytheclipse-cache: with_max_entries must be > 0"); + self.max_entries = Some(max); + self + } + + /// The configured max entries, if any. + pub fn max_entries(&self) -> Option { + self.max_entries + } } #[async_trait] @@ -41,6 +78,7 @@ impl Cache for MemoryCache { match map.get(key) { Some((value, Some(expires))) if *expires <= Instant::now() => { map.remove(key); + self.remove_order(key); Ok(None) } Some((value, _)) => Ok(Some(value.clone())), @@ -55,24 +93,44 @@ impl Cache for MemoryCache { ttl: Option, ) -> Result<(), CacheError> { let expires = ttl.map(|d| Instant::now() + d); - self.inner - .lock() - .unwrap() - .insert(key.to_string(), (value, expires)); + let mut map = self.inner.lock().unwrap(); + let is_new = !map.contains_key(key); + map.insert(key.to_string(), (value, expires)); + if is_new { + let mut order = self.order.lock().unwrap(); + order.push_back(key.to_string()); + if let Some(cap) = self.max_entries { + while order.len() > cap { + if let Some(oldest) = order.pop_front() { + map.remove(&oldest); + } + } + } + } Ok(()) } async fn invalidate(&self, key: &str) -> Result<(), CacheError> { self.inner.lock().unwrap().remove(key); + self.remove_order(key); Ok(()) } async fn clear(&self) -> Result<(), CacheError> { self.inner.lock().unwrap().clear(); + self.order.lock().unwrap().clear(); Ok(()) } } +impl MemoryCache { + /// Removes `key` from the insertion-order deque (if present). + fn remove_order(&self, key: &str) { + let mut order = self.order.lock().unwrap(); + order.retain(|k| k != key); + } +} + /// A typed view over a byte cache using `serde`-compatible (JSON) encoding. /// /// Only enabled with the `cache-aside` feature, which pulls in `serde`. @@ -158,6 +216,26 @@ mod tests { assert_eq!(c.get("b").await.unwrap(), None); } + /// Asserts that an unbounded `MemoryCache::with_max_entries(0)` panics, + /// preventing a no-op cache that accepts zero entries. + #[test] + #[should_panic(expected = "must be > 0")] + fn zero_max_panics() { + let _ = MemoryCache::new().with_max_entries(0); + } + + #[tokio::test] + async fn bounded_cache_evicts_oldest() { + let c = MemoryCache::new().with_max_entries(2); + c.set("a", b"1".to_vec(), None).await.unwrap(); + c.set("b", b"2".to_vec(), None).await.unwrap(); + c.set("c", b"3".to_vec(), None).await.unwrap(); + // "a" (oldest) should have been evicted. + assert_eq!(c.get("a").await.unwrap(), None); + assert_eq!(c.get("b").await.unwrap(), Some(b"2".to_vec())); + assert_eq!(c.get("c").await.unwrap(), Some(b"3".to_vec())); + } + #[cfg(feature = "cache-aside")] #[tokio::test] async fn typed_cache_roundtrip() { diff --git a/crates/mytheclipse-cache/src/moka_cache.rs b/crates/mytheclipse-cache/src/moka_cache.rs index 9438233..199ade6 100644 --- a/crates/mytheclipse-cache/src/moka_cache.rs +++ b/crates/mytheclipse-cache/src/moka_cache.rs @@ -19,7 +19,22 @@ pub struct MokaL1 { impl MokaL1 { /// Builds a Moka cache with `max_capacity` entries and an optional default /// `ttl`. + /// + /// # Panics + /// + /// Panics if `max_capacity` is `0`. In Moka, a `max_capacity` of `0` is a + /// sentinel for **zero-entries-allowed** — every `insert` is silently + /// dropped — which is almost certainly a caller mistake (the natural way to + /// express "unbounded" in other caches). Pass `1..=u64::MAX`; use + /// [`MemoryCache`](crate::memory::MemoryCache) if you truly need an + /// unbounded in-process cache. pub fn new(max_capacity: u64, ttl: Option) -> Self { + assert!( + max_capacity > 0, + "mytheclipse-cache: MokaL1::new(max_capacity) must be > 0; \ + moka treats 0 as a permanent no-insert sentinel. \ + Use MemoryCache for an unbounded cache." + ); let mut builder = MokaCache::builder().max_capacity(max_capacity); if let Some(ttl) = ttl { builder = builder.time_to_live(ttl); @@ -36,14 +51,15 @@ impl Cache for MokaL1 { Ok(self.inner.get(key).await) } + /// Inserts `value`, using the cache's configured TTL policy. The per-call + /// `ttl` argument is intentionally ignored — Moka applies a single TTL + /// configured on the builder, and per-entry overrides are not exposed here. async fn set( &self, key: &str, value: Vec, _ttl: Option, ) -> Result<(), CacheError> { - // Per-entry TTL overrides are handled by the builder default in Moka; - // the passed `ttl` is intentionally ignored (single configured policy). self.inner.insert(key.to_string(), value).await; Ok(()) } @@ -83,6 +99,14 @@ mod tests { assert_eq!(c.get("b").await.unwrap(), None); } + /// Asserts that `max_capacity == 0` panics with a clear message, rather + /// than silently creating a cache that never accepts entries. + #[test] + #[should_panic(expected = "must be > 0")] + fn zero_capacity_panics() { + let _ = MokaL1::new(0, None); + } + #[tokio::test] async fn ttl_does_expire() { // Keep a firm TTL assertion; sleep well past the expiry window. diff --git a/crates/mytheclipse-storage/src/local.rs b/crates/mytheclipse-storage/src/local.rs index 5a4eb1f..29d9b48 100644 --- a/crates/mytheclipse-storage/src/local.rs +++ b/crates/mytheclipse-storage/src/local.rs @@ -22,6 +22,11 @@ impl LocalFileStorage { Self { root: root.into() } } + /// Returns the root directory path. + pub fn root(&self) -> &std::path::Path { + &self.root + } + fn resolve(&self, path: &str) -> Result { let rel = Path::new(path.trim_start_matches('/')); for component in rel.components() { @@ -52,14 +57,30 @@ impl StorageDriver for LocalFileStorage { if let Some(parent) = full.parent() { tokio::fs::create_dir_all(parent) .await - .map_err(|e| StorageError::Io(e.to_string()))?; + .map_err(|e| StorageError::Io(e.to_string()))? } - let mut file = tokio::fs::File::create(&full) + // Write to a sibling temp file then atomically rename. On POSIX this is + // atomic, so a crash mid-write leaves *either* the previous file *or* + // the complete new file — never a half-written truncated object. + // Use a PID-unguarded temp name and clean it up if anything fails. + let tmp = full.with_extension(format!(".tmp-{}", std::process::id())); + let mut file = tokio::fs::File::create(&tmp) .await .map_err(|e| StorageError::Io(e.to_string()))?; let written = tokio::io::copy(&mut data, &mut file) + .await + .map_err(|e| { + let _ = std::fs::remove_file(&tmp); + StorageError::Io(e.to_string()) + })?; + // Ensure durability: flush to OS, fsync, then rename. + tokio::fs::File::open(&tmp) + .await + .map_err(|e| StorageError::Io(e.to_string()))? + .sync_all() .await .map_err(|e| StorageError::Io(e.to_string()))?; + std::fs::rename(&tmp, &full).map_err(|e| StorageError::Io(e.to_string()))?; Ok(written) } @@ -162,4 +183,23 @@ mod tests { .unwrap_err(); assert!(matches!(err, StorageError::InvalidPath(_))); } + + /// Verifies the atomic-write contract: after a successful `put`, the temp + /// file must not linger on disk. + #[tokio::test] + async fn put_leaves_no_temp_file() { + let (storage, _dir) = driver(); + storage + .put("clean.txt", bytes_stream(b"data".to_vec())) + .await + .unwrap(); + // No `*.tmp-*` files should remain in the root after a clean write. + let leftover: Vec<_> = std::fs::read_dir(storage.root()) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name()) + .filter(|n| n.to_string_lossy().contains(".tmp-")) + .collect(); + assert!(leftover.is_empty(), "temp files left behind: {leftover:?}"); + } }