diff --git a/backend/src/services/disk.rs b/backend/src/services/disk.rs index 2afc197..9cf0bcf 100644 --- a/backend/src/services/disk.rs +++ b/backend/src/services/disk.rs @@ -7,6 +7,7 @@ //! barely moves. [`DiskCache`] refreshes it at most once per [`TTL`] and serves the //! rest from memory. +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock}; use std::time::{Duration, Instant}; @@ -20,52 +21,72 @@ pub struct DiskInfo { pub free: u64, } -/// Cheap-to-clone cache of the media filesystem's total/free bytes. Lives in -/// `AppState`. +/// Cheap-to-clone cache of a filesystem's total/free bytes, keyed by the path asked about. +/// Lives in `AppState`. +/// +/// ONE ENTRY PER PATH, not one entry total. This held a single slot carrying its own key, so a +/// lookup for a different path was a miss that OVERWROTE the previous reading — and the app asks +/// about two paths that are distinct mounts in production (`MEDIA_PATH=/media` and +/// `EXPORT_PATH=/exports`, docker-compose.yml). The upload gate and the per-user quota ask about +/// the media volume on every single photo; `host::get_event_status` asks about the exports volume +/// on every host-dashboard load. So while a host had the dashboard open the two evicted each +/// other and the cache hit rate collapsed to zero, putting an uncached +/// `sysinfo::Disks::new_with_refreshed_list()` — a synchronous scan of every mount, on the async +/// runtime — back on the busiest write path in the app. That is precisely the cost the comment in +/// `handlers::upload` says this cache exists to avoid, on the 2-vCPU box it says it matters on. +/// +/// The key space cannot grow: both paths come from `AppConfig`, never from request input, so this +/// map holds at most as many entries as there are configured volumes. #[derive(Clone)] pub struct DiskCache { - inner: Arc>>, + inner: Arc>>, } impl DiskCache { pub fn new() -> Self { Self { - inner: Arc::new(RwLock::new(None)), + inner: Arc::new(RwLock::new(HashMap::new())), } } - /// Drop the cached reading so the next `snapshot()` re-measures the filesystem. + /// Drop every cached reading so the next `snapshot()` re-measures the filesystem. /// /// Used by the e2e TRUNCATE endpoint. Truncating deletes every uploaded file, which materially /// changes free space — but the cached reading survives for up to the TTL, so the next test can /// compute a quota from the PREVIOUS test's disk. That matters now that the quota tests steer /// the per-user limit off `free_disk_bytes`: a stale reading makes the limit wrong and the test /// flaky, for reasons that have nothing to do with the code under test. + /// + /// Clears ALL paths, not just the media one: truncation moves free space on every volume the + /// app measures, and the export volume feeds the host dashboard's low-disk banner. pub fn invalidate(&self) { - *self.inner.write().unwrap() = None; + self.inner.write().unwrap().clear(); } - /// Cached `(total, free)` bytes for the filesystem that holds `media_path`. + /// Cached `(total, free)` bytes for the filesystem backing `path`. /// /// Returns `None` when the mount can't be resolved — callers MUST treat that as /// "unknown", never "zero free". (The quota path in particular fails *open* on /// `None`: enforcing a 0-byte limit would lock every user out of uploading.) - /// Cached free-space reading for `path`. /// - /// The cache is keyed BY PATH. It used to hold a single slot and ignore its argument on a hit, - /// so it would happily return the media volume's numbers for any other path within the TTL. - /// That was invisible only because every caller happened to pass `media_path` — the first - /// caller to ask about a different volume (e.g. the exports volume, which is a separate mount) - /// would have silently got the wrong filesystem's free space. + /// The cache is keyed BY PATH, and it RETAINS every path it has been asked about — see the + /// struct doc for why holding one slot made the cache stop caching as soon as the app asked + /// about its second volume. + /// + /// (The original single slot also carried its key, so it never returned the WRONG + /// filesystem's numbers. What it did instead was miss and re-measure on every alternation, + /// which is the quieter failure and the one that cost.) pub fn snapshot(&self, path: &Path) -> Option { - if let Some((cached_path, info, at)) = self.inner.read().unwrap().as_ref() - && cached_path == path + if let Some((info, at)) = self.inner.read().unwrap().get(path) && at.elapsed() < TTL { return Some(*info); } let info = read_disk_for_path(path)?; - *self.inner.write().unwrap() = Some((path.to_path_buf(), info, Instant::now())); + self.inner + .write() + .unwrap() + .insert(path.to_path_buf(), (info, Instant::now())); Some(info) } } @@ -129,7 +150,91 @@ fn select_disk(mounts: &[(String, u64, u64)], media_path: &str) -> Option