From e645d78a6e0838d12d573dd90693c5b4e737e311 Mon Sep 17 00:00:00 2001
From: MechaCat02 <fabian@diekaulbachs.de>
Date: Mon, 17 Aug 2026 17:54:11 +0200
Subject: [PATCH] fix(disk): the free-space cache stopped caching once a second
 volume was asked about
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`DiskCache` held ONE slot carrying its own key, so a lookup for a different path
was a miss that OVERWROTE the previous reading. The app asks about two paths that
are distinct mounts in production: `MEDIA_PATH=/media` and `EXPORT_PATH=/exports`.

  * the upload gate and the per-user quota ask about the media volume on EVERY
    photo (`handlers::upload`);
  * `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 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 `handlers::upload`'s own comment says this cache exists to
avoid, on the 2-vCPU box it says it matters on, and it degrades hardest exactly
when a host is watching the disk because uploads are failing.

Correctness was never affected — the slot carried its key, so it never returned
the WRONG filesystem's numbers. It missed and re-measured instead, which is the
quieter failure and the one that cost.

Now one entry per path. The key space cannot grow: both paths come from
`AppConfig`, never from request input. `invalidate` clears ALL volumes, since the
e2e TRUNCATE moves free space on every one of them and a survivor would let the
next test compute against the previous test's disk.

Three tests. The one that matters most is `a_stale_entry_is_refreshed_without_
deadlocking`: the hit check holds a READ guard and an expired entry falls through
to a WRITE guard on the same non-reentrant `RwLock`, so whether they overlap
depends on when the `if let` scrutinee's temporary is dropped — edition 2024
drops it before the fall-through, the 2021 rules did not. Too subtle to leave to
a reading of the edition, so it is pinned; it HANGS rather than fails if that
ever regresses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 backend/src/services/disk.rs | 139 ++++++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 17 deletions(-)

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<RwLock<Option<(PathBuf, DiskInfo, Instant)>>>,
+    inner: Arc<RwLock<HashMap<PathBuf, (DiskInfo, Instant)>>>,
 }
 
 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<DiskInfo> {
-        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<DiskIn
 
 #[cfg(test)]
 mod tests {
-    use super::select_disk;
+    use super::{DiskCache, select_disk};
+
+    /// THE REGRESSION: two paths must not evict each other.
+    ///
+    /// `MEDIA_PATH=/media` and `EXPORT_PATH=/exports` are separate mounts in production. With a
+    /// single-slot cache, every host-dashboard load (exports) threw away the reading the upload
+    /// gate and the per-user quota depend on (media), and the next upload threw the exports one
+    /// back — so during exactly the window a host is watching the disk, every photo paid for a
+    /// full `sysinfo` mount scan on the async runtime.
+    ///
+    /// Asserted on the map's occupancy rather than on timing, so it fails deterministically.
+    #[test]
+    fn asking_about_a_second_volume_does_not_evict_the_first() {
+        let cache = DiskCache::new();
+        // Two paths that exist on any host this runs on. They may well resolve to the SAME
+        // filesystem — irrelevant here: what is under test is that each keeps its own entry.
+        let media = std::path::Path::new("/");
+        let exports = std::env::temp_dir();
+
+        assert!(cache.snapshot(media).is_some(), "root must be resolvable");
+        assert!(
+            cache.snapshot(&exports).is_some(),
+            "the temp dir must be resolvable"
+        );
+
+        let held = cache.inner.read().unwrap();
+        assert!(
+            held.contains_key(media),
+            "the media reading was evicted by a lookup for another volume — the upload gate is \
+             back to an uncached mount scan per photo"
+        );
+        assert!(held.contains_key(exports.as_path()));
+    }
+
+    /// A STALE entry must be re-measured rather than deadlock the caller.
+    ///
+    /// This is the one path the test above does not reach, and the only one where the locking
+    /// could be wrong: the hit check holds a READ guard, and an expired entry then falls through
+    /// to `read_disk_for_path` and a WRITE guard on the same `RwLock` — which is not reentrant.
+    /// Whether the read guard is still alive at that point depends on when the `if let`
+    /// scrutinee's temporary is dropped (edition 2024 drops it before the fall-through; the 2021
+    /// rules did not). That is far too subtle to leave to a reading of the edition, so it is
+    /// pinned here: this test HANGS rather than fails if the guards ever overlap.
+    #[test]
+    fn a_stale_entry_is_refreshed_without_deadlocking() {
+        let cache = DiskCache::new();
+        let path = std::path::Path::new("/");
+        assert!(cache.snapshot(path).is_some());
+
+        // Back-date the entry past the TTL — the same trick `sse_tickets` uses to age an entry
+        // without sleeping through the window.
+        {
+            let mut map = cache.inner.write().unwrap();
+            let (info, _) = *map.get(path).expect("just inserted");
+            let stale = std::time::Instant::now()
+                .checked_sub(super::TTL + std::time::Duration::from_secs(1))
+                .expect("host uptime should exceed the disk TTL");
+            map.insert(path.to_path_buf(), (info, stale));
+        }
+
+        // Must take the miss path — read guard released, filesystem re-measured, write guard
+        // taken — and come back with a fresh reading.
+        assert!(
+            cache.snapshot(path).is_some(),
+            "a stale entry must be re-measured, not dropped"
+        );
+        assert!(
+            cache.inner.read().unwrap()[path].1.elapsed() < super::TTL,
+            "the refreshed entry must carry a NEW timestamp, or every later call re-measures too"
+        );
+    }
+
+    /// `invalidate` must clear EVERY volume: the e2e TRUNCATE changes free space on all of them,
+    /// and a surviving entry would let the next test compute against the previous one's disk.
+    #[test]
+    fn invalidate_clears_every_volume() {
+        let cache = DiskCache::new();
+        cache.snapshot(std::path::Path::new("/"));
+        cache.snapshot(&std::env::temp_dir());
+        cache.invalidate();
+        assert!(
+            cache.inner.read().unwrap().is_empty(),
+            "invalidate must not leave a volume behind"
+        );
+    }
 
     #[test]
     fn picks_longest_matching_mount() {