From c78bb529bc2c68e7b242f7f7ea430c3807dcc7d0 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Thu, 12 Nov 2026 00:55:19 +0100 Subject: [PATCH] fix(fuse): bound memory held by thumbnail decodes Every image upload decodes the whole file to build its thumbnail, and nothing limited how many decodes ran at once. With a queue of large photos all 16 drain workers decoded together; 100-megapixel JPEGs held about 9 GiB, swapped the machine, and the watchdog killed the daemon on every restart. Each decode now takes a share of a process-wide 512 MiB budget, sized from the encoded length and the header's pixel count. Phone photos still decode side by side; a photo larger than the budget waits and runs alone. Records B280. --- crates/pdfs-fuse/src/photos.rs | 181 ++++++++++++++++++++++++++++++++- docs/BUGS.md | 34 ++++++- 2 files changed, 210 insertions(+), 5 deletions(-) diff --git a/crates/pdfs-fuse/src/photos.rs b/crates/pdfs-fuse/src/photos.rs index 9a1e73a..d53198f 100644 --- a/crates/pdfs-fuse/src/photos.rs +++ b/crates/pdfs-fuse/src/photos.rs @@ -72,6 +72,105 @@ const UPLOAD_THUMB_QUALITIES: [u8; 4] = [65, 50, 35, 20]; /// Largest still image read whole to render the thumbnail its upload carries. const UPLOAD_IMAGE_MAX_BYTES: u64 = 256 * 1024 * 1024; +/// How much memory every thumbnail decode in the daemon may hold at once: +/// the encoded file plus its decoded pixels. +/// +/// Without a limit, each of the 16 drain workers decoded its own full-size +/// photo. A batch of 100-megapixel JPEGs then held about 9 GiB, which swapped the +/// machine until the watchdog killed the daemon, on every restart +/// (`docs/BUGS.md` B280). With this budget, a phone photo still decodes beside +/// several others, and a 100-megapixel one decodes alone. +const DECODE_BUDGET_BYTES: u64 = 512 * 1024 * 1024; + +/// The process-wide [`DecodeBudget`] every thumbnail decode draws from. +static DECODE_BUDGET: DecodeBudget = DecodeBudget::new(DECODE_BUDGET_BYTES); + +/// A counting gate over bytes: [`acquire`](Self::acquire) waits until the cost +/// fits beside what is already held. +struct DecodeBudget { + limit: u64, + used: std::sync::Mutex, + freed: std::sync::Condvar, +} + +/// A share of a [`DecodeBudget`], given back on drop. +struct DecodePermit<'a> { + budget: &'a DecodeBudget, + cost: u64, +} + +impl DecodeBudget { + const fn new(limit: u64) -> Self { + Self { + limit, + used: std::sync::Mutex::new(0), + freed: std::sync::Condvar::new(), + } + } + + /// Wait until `cost` bytes fit, and hold them. A cost above the limit is + /// cut down to the limit, so the largest image waits for an idle budget + /// and then runs alone, instead of waiting forever. + fn acquire(&self, cost: u64) -> DecodePermit<'_> { + let cost = cost.clamp(1, self.limit); + let mut used = self + .used + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + while *used + cost > self.limit { + used = self + .freed + .wait(used) + .unwrap_or_else(std::sync::PoisonError::into_inner); + } + *used += cost; + DecodePermit { budget: self, cost } + } +} + +impl Drop for DecodePermit<'_> { + fn drop(&mut self) { + let mut used = self + .budget + .used + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + *used -= self.cost; + self.budget.freed.notify_all(); + } +} + +/// What decoding an image of `encoded_len` bytes and `dimensions` costs in +/// memory: the encoded bytes, the decoded pixels at up to 4 bytes each, and a +/// second copy of the pixels when [`decode_upright`] rotates them. When the +/// header gives no dimensions, assume the image is compressed 16 to 1. +fn decode_cost(encoded_len: u64, dimensions: Option<(u32, u32)>) -> u64 { + let pixels = match dimensions { + Some((width, height)) => u64::from(width) * u64::from(height) * 4 * 2, + None => encoded_len.saturating_mul(16), + }; + encoded_len.saturating_add(pixels) +} + +/// The pixel dimensions in the header of an encoded image, without decoding it. +fn dimensions_of(bytes: &[u8]) -> Option<(u32, u32)> { + image::ImageReader::new(std::io::Cursor::new(bytes)) + .with_guessed_format() + .ok()? + .into_dimensions() + .ok() +} + +/// [`dimensions_of`] for an image file. Reads only the header. +fn dimensions_of_file(path: &Path) -> Option<(u32, u32)> { + image::ImageReader::open(path) + .ok()? + .with_guessed_format() + .ok()? + .into_dimensions() + .ok() +} + /// The aspect ratio (w/h) of an encoded image, read from its header alone — no /// pixels are decoded. `None` when the format is unknown or the header is torn. fn ratio_of(bytes: &[u8]) -> Option { @@ -209,7 +308,14 @@ fn thumbnail_build_may_start(status: &ThumbnailBuildStatus, path: &str) -> CoreR /// formats return an undecodable verdict. /// /// CPU-bound (a 20 MP JPEG is real work), so callers run it on the blocking pool. +/// Waits for its share of [`DECODE_BUDGET`] first. fn scale_thumbnail(bytes: &[u8], name: &str, staging_dir: &Path) -> ScaleAttempt { + let _permit = DECODE_BUDGET.acquire(decode_cost(bytes.len() as u64, dimensions_of(bytes))); + scale_thumbnail_unbudgeted(bytes, name, staging_dir) +} + +/// [`scale_thumbnail`] for a caller that already holds its [`DecodePermit`]. +fn scale_thumbnail_unbudgeted(bytes: &[u8], name: &str, staging_dir: &Path) -> ScaleAttempt { scale_thumbnail_with_exiftool( bytes, name, @@ -544,11 +650,20 @@ fn upload_thumbnails_with( } video_file_thumbnail(path, ffmpeg, FFMPEG_TIMEOUT) } else if is_thumbnail_image_name(name) { - let fits = std::fs::metadata(path).is_ok_and(|meta| meta.len() <= UPLOAD_IMAGE_MAX_BYTES); - let Some(bytes) = fits.then(|| std::fs::read(path).ok()).flatten() else { + let Some(len) = std::fs::metadata(path) + .ok() + .map(|meta| meta.len()) + .filter(|len| *len <= UPLOAD_IMAGE_MAX_BYTES) + else { return Vec::new(); }; - scale_thumbnail(&bytes, name, staging_dir) + // Taken before the read, so the encoded bytes count against the + // budget too. + let _permit = DECODE_BUDGET.acquire(decode_cost(len, dimensions_of_file(path))); + let Ok(bytes) = std::fs::read(path) else { + return Vec::new(); + }; + scale_thumbnail_unbudgeted(&bytes, name, staging_dir) } else { return Vec::new(); }; @@ -2116,6 +2231,66 @@ impl Core { } } +#[cfg(test)] +mod decode_budget_tests { + use super::{DecodeBudget, decode_cost}; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::time::Duration; + + /// A decode that does not fit beside the ones running waits for them, + /// instead of every drain worker decoding at once (`docs/BUGS.md` B280). + #[test] + fn a_decode_over_the_budget_waits_for_the_running_one() { + let budget = Arc::new(DecodeBudget::new(100)); + let first = budget.acquire(60); + let started = Arc::new(AtomicBool::new(false)); + let waiter = { + let (budget, started) = (budget.clone(), started.clone()); + std::thread::spawn(move || { + let _second = budget.acquire(60); + started.store(true, Ordering::SeqCst); + }) + }; + std::thread::sleep(Duration::from_millis(100)); + assert!( + !started.load(Ordering::SeqCst), + "second decode ran over budget" + ); + drop(first); + waiter.join().expect("waiter"); + assert!(started.load(Ordering::SeqCst)); + } + + /// Decodes that fit together run together. + #[test] + fn small_decodes_share_the_budget() { + let budget = DecodeBudget::new(100); + let _a = budget.acquire(30); + let _b = budget.acquire(30); + let _c = budget.acquire(30); + } + + /// An image bigger than the whole budget still decodes, alone, rather than + /// waiting forever. + #[test] + fn an_image_over_the_whole_budget_still_decodes() { + let budget = DecodeBudget::new(100); + drop(budget.acquire(10_000)); + let _again = budget.acquire(100); + } + + /// The cost of the 102-megapixel JPEG that set off B280 fills the real + /// budget, so it never decodes beside another one. + #[test] + fn a_hundred_megapixel_photo_costs_the_whole_budget() { + let cost = decode_cost(97_266_141, Some((11_656, 8_742))); + assert!(cost >= super::DECODE_BUDGET_BYTES); + let phone = decode_cost(4_000_000, Some((4_000, 3_000))); + assert!(phone * 4 < super::DECODE_BUDGET_BYTES); + } +} + #[cfg(test)] mod thumb_tests { use super::{ diff --git a/docs/BUGS.md b/docs/BUGS.md index d8ed131..9b2155e 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -109,6 +109,7 @@ a live account since. | Entry | Title | |---|---| +| [B280](#b280--uploading-a-batch-of-large-photos-swaps-the-machine-until-the-daemon-is-killed) | Uploading a batch of large photos swaps the machine until the daemon is killed | | [B279](#b279--after-a-watchdog-kill-the-mount-never-comes-back) | After a watchdog kill the mount never comes back | | [B278](#b278--sync-list-and-devices-list-show-the-last-sync-as-epoch-seconds) | `sync list` and `devices list` show the last sync as epoch seconds | | [B277](#b277--my-files-mounted-over-a-folder-with-files-in-it-and-hid-them) | My files mounted over a folder with files in it, and hid them | @@ -382,6 +383,35 @@ a live account since. Newest first. +### B280 — Uploading a batch of large photos swaps the machine until the daemon is killed + +**Status:** Fixed, not verified live +**Found:** 2026-11-12, after B279. 868 new pictures (15.6 GiB) were copied into +`~/ProtonDrive/Pictures`. Each restart reached 1.7 GiB resident and 9.8 GiB of swap within 40 s. +The 16 `pdfs-drain-*` threads were all busy, cgroup memory pressure (`full avg10`) was 77 %, and +the system's 15 GiB of swap was full. The watchdog killed the daemon, and the next start did the +same. The largest picture was a 97 MB JPEG of 11656×8742 pixels. +**Where:** `upload_thumbnails_with` and `scale_thumbnail` in `crates/pdfs-fuse/src/photos.rs` + +**Repro:** Queue more large photos than there are drain workers, for example by copying a folder +of 100-megapixel JPEGs into the mount, and start the daemon. + +**Cause.** Every upload of an image builds its thumbnail by decoding the whole file. Nothing +limited how many of these decodes ran at once, so all 16 drain workers did it together. One +102-megapixel JPEG holds about 300 MB of pixels, and twice that when it has to be turned upright. +Sixteen of them do not fit in memory, and `MemoryHigh=2G` throttled the daemon into swap until it +stopped answering. + +**Fix.** Each thumbnail decode first takes a share of a process-wide 512 MiB budget. The share is +the encoded size plus 8 bytes per pixel, read from the image header. A 12-megapixel phone photo +still decodes beside four others. A 100-megapixel photo waits until no other decode runs, and +then runs alone. The upload path takes its share before it reads the file. + +**Test:** `decode_budget_tests` in `crates/pdfs-fuse/src/photos.rs`. On 2026-11-12 the same queue +on the fixed build peaked at 1.3 GiB resident and 90 MB of swap, with memory pressure near 0 %. + +--- + ### B279 — After a watchdog kill the mount never comes back **Status:** Fixed, not verified live @@ -415,8 +445,8 @@ supervisor first gets an answer from the control socket, it sends the unit's own The supervisor also probes and pings on its first tick, not one period later. A startup that hangs for good is still aborted, only later. -What is left: why the daemon stopped answering during the copy. Its swap use points at memory -growth during a large batch of new files (compare [B91](#b91--the-daemon-hung-overnight-and-nothing-could-say-what-it-was-waiting-on)). +The daemon stopped answering during the copy because of +[B280](#b280--uploading-a-batch-of-large-photos-swaps-the-machine-until-the-daemon-is-killed). **Test:** `first_watchdog_ping_is_due_at_once` in `crates/pdfs-fuse/src/supervisor.rs`. The stale-mount path and the watchdog handshake need a live systemd unit to check.