From 23e2f485dd9287b3902d0dc012d67c3a618a402c Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 8 Aug 2026 22:10:40 +0200 Subject: [PATCH] fix(upload): correct two errors in the keepsake headroom gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found reviewing my own change rather than by a test, which is the point. DOUBLE-SUBTRACTION. The gate computed `free - size`, but the body is streamed to its temp file during multipart parsing — far above the gate — so the free-space reading already excludes those bytes. Subtracting again refused uploads a full file-size early; with max_video_size_mb at 500 that is half a gigabyte of phantom pressure. `media_total` genuinely does need `+ size` (its row is not committed yet), which is what made the asymmetry easy to miss. BLOCKING SCAN ON THE HOTTEST PATH. It called `disk::free_bytes`, whose doc comment says it deliberately bypasses DiskCache — but that rationale is the export preflight's: a rare, high-stakes decision where a sibling worker can move free space by tens of GB inside the TTL. Per upload it means sysinfo re-scanning every mount, synchronously, on the async runtime, on a 2 vCPU box with two worker threads. Now uses the cached snapshot, the same 15s staleness the quota check immediately below already accepts for the same question. Co-Authored-By: Claude Opus 5 (1M context) --- backend/src/handlers/upload.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 8eab212..284026a 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -462,13 +462,26 @@ pub async fn upload( // rationing space between guests; it was never meant to authorise running the disk to zero, // and an operator flipping it at 23:00 to unblock a guest should not silently disarm the // last thing standing between the party and a dead database. - if let Some(free) = crate::services::disk::free_bytes(&state.config.media_path) { + // `disk_cache`, not the uncached `disk::free_bytes`. That function deliberately bypasses the + // cache for the EXPORT PREFLIGHT, where a sibling worker can move free space by tens of GB + // inside the TTL and a stale reading would authorise the write that fills the disk. This is + // the opposite situation: the busiest write path in the app, on a 2 vCPU box, where an + // uncached read means `sysinfo::Disks::new_with_refreshed_list()` — a synchronous scan of + // every mount, on the async runtime — for every single photo. The quota check immediately + // below already accepts the same 15s staleness for the same question. + if let Some(disk) = state.disk_cache.snapshot(&state.config.media_path) { + // NOTE: `free` already excludes this upload. The body was streamed to its temp file + // during multipart parsing, well above, so the bytes are on disk before this runs — + // subtracting `size` here again would refuse a full file-size early. + // + // `media_total` is the opposite: it is a sum of committed DB rows, and this upload's row + // does not exist yet, so the prospective total does need `+ size`. + let free = disk.free as i64; let media_after = state.media_total.get(&state.pool).await.saturating_add(size); let keepsake_needs = crate::services::export::required_free_bytes(media_after.max(0) as u64, 2) as i64; - let free_after = (free as i64).saturating_sub(size); let required = keepsake_needs.saturating_add(DISK_RESERVE_BYTES); - if free_after < required { + if free < required { tracing::error!( free_bytes = free, upload_size = size, @@ -1563,12 +1576,13 @@ mod tests { let step: i64 = 250 * 1024 * 1024; let mut last_accepted = 0i64; while media < USABLE { - let free = USABLE - media; + // `free` already excludes the bytes just streamed to the temp file, matching the + // handler: the gate compares live free space against what the keepsake will need. let media_after = media + step; - let free_after = free - step; + let free = USABLE - media_after; let required = crate::services::export::required_free_bytes(media_after as u64, 2) as i64 + reserve; - let gate_accepts = free_after >= required; + let gate_accepts = free >= required; if gate_accepts { // The export preflight must agree, using the SAME arithmetic it will run later. @@ -1576,7 +1590,7 @@ mod tests { crate::services::export::required_free_bytes(media_after as u64, 2) as i64 + reserve; assert!( - free_after >= preflight_needs, + free >= preflight_needs, "gate accepted at media={media_after} but the preflight would refuse" ); last_accepted = media_after;