fix(upload): correct two errors in the keepsake headroom gate
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) <noreply@anthropic.com>
This commit is contained in:
@@ -462,13 +462,26 @@ pub async fn upload(
|
|||||||
// rationing space between guests; it was never meant to authorise running the disk to zero,
|
// 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
|
// 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.
|
// 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 media_after = state.media_total.get(&state.pool).await.saturating_add(size);
|
||||||
let keepsake_needs =
|
let keepsake_needs =
|
||||||
crate::services::export::required_free_bytes(media_after.max(0) as u64, 2) as i64;
|
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);
|
let required = keepsake_needs.saturating_add(DISK_RESERVE_BYTES);
|
||||||
if free_after < required {
|
if free < required {
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
free_bytes = free,
|
free_bytes = free,
|
||||||
upload_size = size,
|
upload_size = size,
|
||||||
@@ -1563,12 +1576,13 @@ mod tests {
|
|||||||
let step: i64 = 250 * 1024 * 1024;
|
let step: i64 = 250 * 1024 * 1024;
|
||||||
let mut last_accepted = 0i64;
|
let mut last_accepted = 0i64;
|
||||||
while media < USABLE {
|
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 media_after = media + step;
|
||||||
let free_after = free - step;
|
let free = USABLE - media_after;
|
||||||
let required =
|
let required =
|
||||||
crate::services::export::required_free_bytes(media_after as u64, 2) as i64 + reserve;
|
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 {
|
if gate_accepts {
|
||||||
// The export preflight must agree, using the SAME arithmetic it will run later.
|
// 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
|
crate::services::export::required_free_bytes(media_after as u64, 2) as i64
|
||||||
+ reserve;
|
+ reserve;
|
||||||
assert!(
|
assert!(
|
||||||
free_after >= preflight_needs,
|
free >= preflight_needs,
|
||||||
"gate accepted at media={media_after} but the preflight would refuse"
|
"gate accepted at media={media_after} but the preflight would refuse"
|
||||||
);
|
);
|
||||||
last_accepted = media_after;
|
last_accepted = media_after;
|
||||||
|
|||||||
Reference in New Issue
Block a user