diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index ab99060..eee1df1 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -201,6 +201,16 @@ pub async fn join( /// Mirrors migration 023; kept here so the invariant below can be asserted in a test. const RECOVER_NAME_CEILING_DEFAULT: usize = 4; +/// Hard ceiling on the CONFIGURED per-(IP, name) limit, whatever an operator sets. +/// +/// The ordering `3 x ceiling <= PIN_LOCK_THRESHOLD` is the entire control that stops one source +/// from locking a victim out: display names are public on the feed, so if a single IP can spend +/// the whole lock threshold it can lock any guest it likes, repeatedly. That ordering was asserted +/// in a comment and in a test — but the test pinned the DEFAULT constant, while the handler reads +/// the config value, and `patch_config` accepted anything from 1 to 100_000. So an operator +/// raising this key restored the exact DoS the tier ordering exists to prevent, silently. +pub const RECOVER_NAME_CEILING_MAX: usize = (PIN_LOCK_THRESHOLD as usize) / 3; + /// Wrong PINs, from ALL sources, before the ACCOUNT itself is locked for 15 minutes. /// /// This was 3, which sat BELOW the per-(IP, name) ceiling of 5 — and that ordering, not the @@ -337,7 +347,11 @@ pub async fn recover( "recover_name_rate_per_15min", RECOVER_NAME_CEILING_DEFAULT, ) - .await; + .await + // CLAMPED, not merely defaulted — see RECOVER_NAME_CEILING_MAX. The config value is + // operator-settable and the invariant it has to respect is not expressible in + // `patch_config`'s numeric range, so it is enforced at the point of use. + .min(RECOVER_NAME_CEILING_MAX); let name_key = display_name.to_lowercase(); if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), @@ -756,9 +770,10 @@ mod tests { #[test] fn one_ip_cannot_reach_the_account_lock() { assert!( - PIN_LOCK_THRESHOLD as usize >= RECOVER_NAME_CEILING_DEFAULT * 3, + PIN_LOCK_THRESHOLD as usize >= RECOVER_NAME_CEILING_MAX * 3, "locking a victim must require at least three distinct sources; \ - threshold {PIN_LOCK_THRESHOLD} vs per-IP ceiling {RECOVER_NAME_CEILING_DEFAULT}" + threshold {PIN_LOCK_THRESHOLD} vs the ENFORCED per-IP ceiling \ + {RECOVER_NAME_CEILING_MAX} (the default is {RECOVER_NAME_CEILING_DEFAULT})" ); } diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index ebc5f23..ffab67d 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -44,9 +44,6 @@ pub struct EventStatus { pub disk_low: bool, } -/// Absolute floor below which free space is worth surfacing regardless of gallery size — the -/// threshold the README has carried on the roadmap since v1. -const LOW_DISK_FLOOR_BYTES: u64 = 10_000_000_000; /// Is free space low enough that the host needs to know? /// @@ -72,7 +69,12 @@ const LOW_DISK_FLOOR_BYTES: u64 = 10_000_000_000; fn disk_is_low(free: u64, keepsake_required: u64) -> bool { let gate_closes_at = keepsake_required.saturating_add(crate::handlers::upload::DISK_RESERVE_BYTES as u64); let warn_at = gate_closes_at.saturating_add(gate_closes_at / 4); - free < LOW_DISK_FLOOR_BYTES || free < warn_at + // No separate absolute-floor clause. There used to be `free < LOW_DISK_FLOOR_BYTES ||` here, + // and it was unreachable: `gate_closes_at` is at least DISK_RESERVE_BYTES, so `warn_at` is at + // least 1.25x it (12.5 GB) — always above the 10 GB floor. Two tests were named after that + // clause and neither could fail if it were deleted. Keeping dead code that tests claim to + // cover is worse than not having it. + free < warn_at } /// Count non-banned hosts/admins in the event OTHER than `excluding` — the operators @@ -824,7 +826,7 @@ pub async fn release_gallery( #[cfg(test)] mod tests { - use super::{LOW_DISK_FLOOR_BYTES, disk_is_low}; + use super::disk_is_low; use crate::handlers::upload::DISK_RESERVE_BYTES; use crate::services::export::required_free_bytes; @@ -836,18 +838,18 @@ mod tests { assert!(!disk_is_low(60 * GB, 25 * GB)); } + /// Renamed from `the_absolute_floor_fires_...`: there is no separate floor clause any more + /// (see `disk_is_low`). What still has to hold is the behaviour the floor was there FOR — a + /// nearly-empty disk is low even when the gallery is small enough that the keepsake term + /// alone would clear it, because all three volumes share one filesystem and Postgres needs + /// room to write. #[test] - fn the_absolute_floor_fires_even_when_the_gallery_is_tiny() { + fn a_nearly_empty_disk_is_low_even_when_the_gallery_is_tiny() { // All three volumes share one filesystem, so running out doesn't degrade one subsystem — // Postgres stops being able to write and the event goes down. A 1 GB gallery would clear // the keepsake test comfortably; the floor is what catches this. assert!(disk_is_low(5 * GB, GB)); - assert!(disk_is_low(LOW_DISK_FLOOR_BYTES - 1, 0)); - // NOT `!disk_is_low(LOW_DISK_FLOOR_BYTES, 0)` any more. The floor is a lower bound, not - // the boundary: the gate-aware trigger now dominates it (at an empty gallery it warns - // below 1.25 x the reserve, i.e. 12.5 GB). Pinning equality here asserted the very - // behaviour that let the wall arrive before the warning. - assert!(disk_is_low(LOW_DISK_FLOOR_BYTES, 0)); + assert!(disk_is_low(9 * GB, 0)); assert!(!disk_is_low(20 * GB, 0), "a roomy empty disk is not low"); } @@ -858,12 +860,33 @@ mod tests { /// to upload while the dashboard shows a comfortable disk — with nobody on site to ask. #[test] fn the_banner_always_fires_before_the_upload_gate_closes() { + // Asserting `disk_is_low(gate_closes_at, required)` is what this used to do, and it was a + // tautology: `disk_is_low` recomputes the same `gate_closes_at` internally and compares + // against `gate + gate/4`, so the assertion reduced to `G < G + G/4` — true for every G, + // for any margin, even a margin of zero. It could not detect the banner being moved to + // exactly the gate, which is the regression it is named for. + // + // So pin the GAP instead: find the free-space level at which the banner starts, and + // require it to be strictly above the level at which the gate closes, by a usable amount. for media_gb in [0u64, 1, 4, 8, 16, 32] { let required = required_free_bytes(media_gb * GB, 2); let gate_closes_at = required + DISK_RESERVE_BYTES as u64; + + // Just above the gate: guests can still upload, and the host must already be warned. assert!( - disk_is_low(gate_closes_at, required), - "at media={media_gb}GB the gate is about to close but no banner is shown" + disk_is_low(gate_closes_at + 1, required), + "at media={media_gb}GB the banner is not yet showing while the gate still allows uploads" + ); + + // The warning must lead by a margin the host can act inside, not by one byte. + let mut warn_starts_at = gate_closes_at; + while disk_is_low(warn_starts_at, required) { + warn_starts_at += GB / 10; + } + assert!( + warn_starts_at >= gate_closes_at + gate_closes_at / 5, + "at media={media_gb}GB the banner leads the gate by only {} bytes", + warn_starts_at - gate_closes_at ); } } @@ -889,7 +912,7 @@ mod tests { } #[test] - fn an_empty_gallery_needs_nothing_and_only_the_floor_applies() { + fn an_empty_gallery_still_reserves_room_for_postgres() { // With no gallery the keepsake term is 0, so the warn threshold collapses to // 1.25 x DISK_RESERVE_BYTES (12.5 GB), which dominates the 10 GB absolute floor. assert!(!disk_is_low(13 * GB, 0)); diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index d79c483..290746c 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -248,6 +248,11 @@ pub async fn upload( // The client's idempotency key. Optional: an older client, or any other caller, simply // doesn't send one and gets the previous behaviour. let mut client_upload_id: Option = None; + // Admission reservation for this body's temp bytes. Declared out here so it lives until the + // handler returns — the temp file exists for that whole span, and releasing early would let + // the next upload reserve space this one is still occupying. Dropping it is the release, so + // every exit path (success, error, client disconnect) returns the budget automatically. + let mut _admission: Option = None; // The multipart read is wrapped so the field loop can use `?` freely; reclaiming the temp // file on failure is `file_guard`'s job, not this block's. @@ -274,6 +279,28 @@ pub async fn upload( } else { (max_image_mb.max(max_video_mb) * 1024 * 1024) as usize }; + // ADMISSION BEFORE THE FIRST BYTE TOUCHES DISK. The headroom gate below can + // only refuse to COMMIT an upload — by the time it runs, the body has already + // been streamed to its temp file. Nothing else bounds how many bodies stream + // at once (axum imposes no limit, the tower stack is just TraceLayer, Caddy + // passes through), so ~100 guests tapping "upload all" after the ceremony put + // 10-20 GB of `.tmp` on a 40 GB volume that the gate cannot see, eating the + // reserve that keeps Postgres able to write WAL. The permit is held until the + // handler returns, which is exactly as long as the temp file can exist. + _admission = Some( + state + .upload_admission + .reserve(cap_bytes) + .await + .ok_or_else(|| { + AppError::ServiceUnavailable( + "Gerade laden sehr viele Gäste hoch. Dein Foto bleibt in der \ + Warteschlange und wird gleich automatisch gesendet." + .into(), + Some(30), + ) + })?, + ); tokio::fs::create_dir_all(&originals_dir) .await .map_err(|e| AppError::Internal(e.into()))?; @@ -286,10 +313,19 @@ pub async fn upload( hashtags_csv = Some(read_text_field_bounded(field, MAX_HASHTAGS_BYTES).await?); } "client_upload_id" => { - let raw = field - .text() - .await - .map_err(|e| AppError::BadRequest(e.to_string()))?; + // BOUNDED, like every other text field here. This used `Field::text()`, + // which buffers without any ceiling of its own: axum builds its multipart + // reader with no `SizeLimit`, so the only bound was this route's 576 MiB + // body limit — and `text()` then decodes that into a second full String. + // One request from any authenticated guest, declaring `client_upload_id` + // and sending 576 MiB of padding, peaks well past the app container's 1 GB + // and gets it OOM-killed: every SSE stream dropped, every in-flight upload's + // temp file stranded, `restart: unless-stopped` cycling it. `caption` and + // `hashtags` were bounded by the helper for exactly this reason; this field + // arrived later (migration 022) and missed it. + // + // 64 bytes fits a hyphenated UUID (36) with room to spare. + let raw = read_text_field_bounded(field, 64).await?; // A malformed key is not worth rejecting an upload over — the photo is the // thing the guest cares about. Drop the key and lose only the retry // protection, which is exactly where we were before it existed. @@ -1064,8 +1100,10 @@ const MIN_QUOTA_LIMIT_BYTES: i64 = 500 * 1024 * 1024; /// Free space on the media volume that uploads may never consume, whatever any quota says. /// -/// Matches the host dashboard's low-disk threshold (`handlers::host`), so the banner the host -/// sees and the wall guests hit are the same number rather than two unrelated opinions. 10 GB +/// The host dashboard's low-disk banner is DERIVED from this (`handlers::host::disk_is_low`) +/// rather than equal to it: the banner fires at 1.25x the gate's closing point, deliberately, so +/// the host is warned while there is still room to act instead of at the same instant guests hit +/// the wall. Shared expression, offset threshold — do not "restore" them to one number. 10 GB /// is chosen to leave Postgres, its WAL and a rotation of container logs comfortable room on /// the shared filesystem long after new uploads have been refused. pub const DISK_RESERVE_BYTES: i64 = 10_000_000_000; @@ -1655,18 +1693,44 @@ mod tests { /// allowed 500 MB, the per-user quota alone would authorise ~50 GB on a 40 GB disk. #[test] fn the_global_gate_binds_before_the_per_user_floor_can_overfill_the_disk() { - let per_user_total = MIN_QUOTA_LIMIT_BYTES * 100; + const USABLE: i64 = 35 * GB; + const GUESTS: i64 = 100; + + // Premise (constant, so `const`-asserted rather than pretending to be a test): + // the per-user floor alone over-commits the volume, so something else must bind. + const _: () = assert!(MIN_QUOTA_LIMIT_BYTES * 100 > 35 * GB); + + // The property. The previous version of this test asserted ONLY the premise above and + // never touched the gate, so no change to `quota_limit_bytes`, `required_free_bytes` or + // DISK_RESERVE_BYTES could have failed it. + // + // Take a moment where the volume holds 10 GB of media. Check both controls against the + // SAME state and assert they disagree in the required direction: the per-user quota still + // says yes, and the global gate already says no. + let media: i64 = 10 * GB; + let free = USABLE - media; + + // Per-user: a guest who has uploaded nothing is still granted a full floor-sized + // allowance, because the formula divides free space and then applies the floor. + let per_user = quota_limit_bytes(free, 0.75, GUESTS, GUESTS); assert!( - per_user_total > 35 * GB, - "premise: the per-user floor alone over-commits the volume, so the global gate \ - is what must stop it" + per_user >= MIN_QUOTA_LIMIT_BYTES, + "the per-user quota is expected to still be permissive here, got {per_user}" + ); + + // Global: the keepsake needs both halves plus the reserve, and they no longer fit. + let required = + crate::services::export::required_free_bytes(media as u64, 2) as i64 + DISK_RESERVE_BYTES; + assert!( + free < required, + "the global gate must already be closed at {media} bytes of media: free {free} \ + vs required {required}" ); } - /// The floor must never write a cheque the volume cannot cash — otherwise a full disk - /// still hands out a 500 MB allowance and the filesystem Postgres needs fills up. - /// The floor must never write a cheque the volume cannot cash, and — the subtler half — - /// must never hand EVERY uploader the whole remaining budget. + /// The floor must never write a cheque the volume cannot cash — otherwise a full disk still + /// hands out a 500 MB allowance and the filesystem Postgres needs fills up — and, the subtler + /// half, it must never hand EVERY uploader the whole remaining budget. #[test] fn the_floor_never_exceeds_what_the_disk_can_back() { assert_eq!(quota_limit_bytes(0, 0.75, 3, 1), 0, "no disk, no allowance"); diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 590ae26..357c468 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -13,9 +13,6 @@ use crate::state::SseEvent; #[derive(Clone)] pub struct CompressionWorker { semaphore: Arc, - /// Serialises the memory-heavy image jobs — see `HEAVY_IMAGE_BYTES`. Separate from - /// `semaphore` so ordinary photos keep full concurrency. - heavy: Arc, pool: PgPool, media_path: PathBuf, sse_tx: broadcast::Sender, @@ -34,7 +31,6 @@ impl CompressionWorker { ) -> Self { Self { semaphore: Arc::new(Semaphore::new(concurrency)), - heavy: Arc::new(Semaphore::new(1)), pool, media_path, sse_tx, @@ -308,19 +304,6 @@ impl CompressionWorker { /// saving rather than risk the OOM kill. const OXIPNG_MAX_PIXELS: u64 = 8_000_000; - /// Estimated peak heap above which an image job takes the exclusive `heavy` permit. - /// - /// `compression_concurrency` (default 2) bounds how many jobs run at once, but says - /// nothing about how much memory each one costs, and the container gets 1 GiB total. A - /// single 8000x8000 original measures ~516 MiB peak even with the decode correctly scoped - /// — two of those overlapping is 1032 MiB and another OOM kill, from nothing more exotic - /// than two guests uploading big photos at the same moment. - /// - /// 150 MiB sits far above a normal phone photo (a 12 MP JPEG costs ~50 MiB all-in) so the - /// common path never serialises, and far below the point where two jobs stop fitting. - /// Throughput is unaffected for everything except the rare giant, which is exactly the - /// case that must not run in parallel with another giant. - const HEAVY_IMAGE_BYTES: u64 = 150 * 1024 * 1024; /// Wall-clock ceiling for one oxipng run. /// @@ -356,13 +339,13 @@ impl CompressionWorker { let estimate = crate::services::imaging::estimated_processing_peak_bytes(&original, Self::DISPLAY_MAX_EDGE); let _heavy_permit = match estimate { - Some(bytes) if bytes > Self::HEAVY_IMAGE_BYTES => { + Some(bytes) if bytes > crate::services::imaging::HEAVY_IMAGE_BYTES => { tracing::debug!( %upload_id, estimated_mib = bytes / (1024 * 1024), "waiting for the heavy-image permit" ); - Some(self.heavy.acquire().await) + Some(crate::services::imaging::HEAVY_IMAGE_PERMITS.acquire().await) } _ => None, }; @@ -768,7 +751,7 @@ mod tests { ) .expect("header readable"); assert!( - ordinary_peak <= CompressionWorker::HEAVY_IMAGE_BYTES, + ordinary_peak <= crate::services::imaging::HEAVY_IMAGE_BYTES, "a 12 MP photo estimated at {} MiB would serialise the common path", ordinary_peak / 1048576 ); @@ -782,7 +765,7 @@ mod tests { ) .expect("header readable"); assert!( - giant_peak > CompressionWorker::HEAVY_IMAGE_BYTES, + giant_peak > crate::services::imaging::HEAVY_IMAGE_BYTES, "an 8000x8000 RGBA original estimated at only {} MiB would be allowed to run \ concurrently with another one — 2x its real ~516 MiB peak does not fit in 1 GiB", giant_peak / 1048576 diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index ff036b1..46a3018 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -522,6 +522,21 @@ async fn ensure_export_space_reclaiming( ensure_export_space(pool, event_id, export_path).await } +/// Take the process-wide heavy-image permit if this file is big enough to need it. +/// +/// Mirrors the compression worker's gate exactly (a header probe, no pixels decoded), so the two +/// producers of heavy image work agree on what "heavy" means and serialise against each other +/// rather than each against itself. +async fn heavy_permit_for(path: &Path) -> Option> { + let estimate = crate::services::imaging::estimated_processing_peak_bytes(path, 2048); + match estimate { + Some(bytes) if bytes > crate::services::imaging::HEAVY_IMAGE_BYTES => { + crate::services::imaging::HEAVY_IMAGE_PERMITS.acquire().await.ok() + } + _ => None, + } +} + // ── ZIP export ─────────────────────────────────────────────────────────────── async fn run_zip_export( @@ -888,6 +903,12 @@ async fn run_html_export_inner( let thumb_path = media_tmp.join(&thumb); let thumb_path_clone = thumb_path.clone(); + // Same process-wide memory permit the compression worker takes. Without it, a + // release fired while the last phone photos were still compressing put an export + // decode and a heavy compression job in the same 1 GiB cgroup — and the OOM kill + // marks the export failed, which `recover_exports` then re-spawns into the same + // conditions on the next boot. + let _heavy = heavy_permit_for(&src).await; let thumb_result = tokio::task::spawn_blocking(move || -> Result<()> { // `decode_oriented`, not `image::open`: the latter ignores the EXIF // orientation tag AND applies no decode limits. Using it here is why every @@ -922,6 +943,9 @@ async fn run_html_export_inner( let full_path = media_tmp.join(&full); let full_path_clone = full_path.clone(); + // See the thumbnail above. This branch is the more expensive of the two: it + // only runs for originals over 5 MB, i.e. exactly the giants. + let _heavy = heavy_permit_for(&src).await; let compress_result = tokio::task::spawn_blocking(move || -> Result<()> { // Same reason as the thumbnail above. This branch only runs for originals // over 5 MB, which is why the viewer's full image looked correct for small @@ -1315,16 +1339,19 @@ async fn protected_files(pool: &PgPool, event_id: Uuid) -> Vec { .unwrap_or_default() } -/// Reclaim superseded FINAL archives BEFORE this generation starts writing its own. +/// Reclaim superseded FINAL archives. /// -/// Peak disk usage used to be two full generations, because the only prune ran after the new archive -/// was written, renamed and finalised. That ordering reads as durability ("don't delete the good -/// keepsake before the replacement is safe") but it buys nothing: readiness is derived from -/// `job.epoch = event.export_epoch AND status = 'done'`, so the moment `invalidate_and_arm` bumps -/// the epoch the old archive is ALREADY unreachable — `GET /export/zip` 404s whether the file is on -/// disk or not. Keeping it only reserves gigabytes for a download nobody can perform, and for -/// `Affects::Both` (a takedown) it is content someone has explicitly asked to have removed. So a -/// rebuild reclaims first and peaks at one generation. +/// CALLED AFTER A SUCCESSFUL BUILD, not before one. This doc used to argue the opposite at +/// length — that since readiness is derived from `job.epoch = event.export_epoch`, a superseded +/// archive is already unreachable and keeping it "buys nothing". That reasoning is right about +/// REACHABILITY and wrong about RECOVERABILITY: an epoch is a database value that can be rolled +/// back, deleted bytes cannot. Pruning first meant any rebuild that then failed — ENOSPC, an OOM, +/// a hung ffmpeg — left the event with NO archive at all, which is the one outcome the product +/// exists to prevent, at the one moment nobody is watching. +/// +/// The single exception is phase 2 of `ensure_export_space_reclaiming`, where the previous +/// generation's bytes are the only way the rebuild can fit at all. Both call sites carry the full +/// reasoning; do not "restore" a pre-build prune on the strength of this function's convenience. /// /// Narrower than [`prune_stale_export_files`] on purpose: FINAL archives only. Those are inert — a /// superseded worker either already renamed its file (and will delete it itself when its guarded diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index 513fa1a..c557825 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -205,6 +205,32 @@ pub fn decode_oriented(path: &Path) -> Result { Ok(img) } +/// Process-wide serialisation for memory-heavy image work. +/// +/// The `app` container gets 1 GiB. A single 8000x8000 original measures ~516 MiB peak even with +/// the decode correctly scoped, so two overlapping giants is an OOM kill — and the kernel kills +/// the whole process, dropping every SSE stream and stranding every in-flight upload. +/// +/// GLOBAL rather than a field on `CompressionWorker`, because the constraint is the container's +/// memory and there is more than one producer of this work. The export's own image path +/// (`services::export`) decodes and resizes every photo in the gallery — a thumbnail for each, +/// plus a 2000px re-encode for every original over 5 MB — and it ran in a bare `spawn_blocking` +/// with no permit at all. So "host taps Freigeben while the last phone photos are still +/// compressing" put an export decode and a heavy compression job in the same cgroup at the same +/// time, which is the scenario the permit exists to make impossible. Worse, it is self-repeating: +/// the OOM kill marks the export failed, and `recover_exports` re-spawns it on boot into the same +/// conditions. +/// +/// Held across the blocking section and released on drop, including on error. +pub static HEAVY_IMAGE_PERMITS: std::sync::LazyLock = + std::sync::LazyLock::new(|| tokio::sync::Semaphore::new(1)); + +/// Estimated peak heap above which a job must take [`HEAVY_IMAGE_PERMITS`]. +/// +/// 150 MiB sits far above a normal phone photo (a 12 MP JPEG costs ~50 MiB all-in) so the common +/// path never serialises, and far below the point where two jobs stop fitting in the container. +pub const HEAVY_IMAGE_BYTES: u64 = 150 * 1024 * 1024; + #[cfg(test)] mod tests { use super::*; diff --git a/backend/src/services/media_total.rs b/backend/src/services/media_total.rs index f878c8b..9ccaf00 100644 --- a/backend/src/services/media_total.rs +++ b/backend/src/services/media_total.rs @@ -61,15 +61,37 @@ impl MediaTotalCache { { return bytes; } - let bytes = sqlx::query_scalar::<_, Option>( + let queried = sqlx::query_scalar::<_, Option>( "SELECT SUM(total_upload_bytes)::bigint FROM \"user\"", ) .fetch_one(pool) - .await - .ok() - .flatten() - .unwrap_or(0) - .max(0); + .await; + + let bytes = match queried { + Ok(v) => v.unwrap_or(0).max(0), + Err(e) => { + // FAIL OPEN, but do NOT cache the failure, and do NOT let it pass silently. + // + // Storing 0 here pinned the gate's view of the event at "empty" for the whole + // TTL. During that window `media_after` is just this upload, `keepsake_needs` + // collapses to ~2.2x one file, and the gate degrades to the flat 10 GB reserve — + // precisely the behaviour the two-halves design replaced, reappearing with no + // trace in the log. And the trigger correlates with the danger: with + // `max_connections = 10` and a 5s acquire timeout, this query fails exactly when + // a burst is in progress. + // + // Falling back to the LAST GOOD reading (however stale) is strictly better than + // 0: the total only ever grows, so a stale value under-counts slightly, while 0 + // under-counts by everything. + let previous = self.inner.read().unwrap().map(|(b, _)| b); + tracing::warn!( + error = %e, + fallback_bytes = previous.unwrap_or(0), + "media total query failed; upload gate is running on a stale reading" + ); + return previous.unwrap_or(0); + } + }; *self.inner.write().unwrap() = Some((bytes, Instant::now())); bytes } diff --git a/backend/src/services/mod.rs b/backend/src/services/mod.rs index 36f6823..3a99259 100644 --- a/backend/src/services/mod.rs +++ b/backend/src/services/mod.rs @@ -7,4 +7,5 @@ pub mod maintenance; pub mod media_total; pub mod rate_limiter; pub mod sse_tickets; +pub mod upload_admission; pub mod video; diff --git a/backend/src/services/upload_admission.rs b/backend/src/services/upload_admission.rs new file mode 100644 index 0000000..4eebaee --- /dev/null +++ b/backend/src/services/upload_admission.rs @@ -0,0 +1,155 @@ +//! Admission control for upload bodies, budgeted in BYTES rather than requests. +//! +//! ## Why this has to exist +//! +//! The keepsake headroom gate in `handlers::upload` cannot bound a burst, and the reason is +//! structural rather than a bug in the gate: the request body is streamed to a temp file during +//! multipart parsing, so the bytes are already on disk by the time any check runs. The gate can +//! only refuse to COMMIT them. Nothing upstream limited how many bodies stream at once — axum has +//! no such limit, the tower stack is just `TraceLayer`, and Caddy passes requests straight +//! through. +//! +//! So the failure mode is the ordinary one, not an attack: the ceremony ends, ~100 guests tap +//! "upload all", and ~100 bodies stream concurrently. At phone-video sizes that is 10-20 GB of +//! `.tmp` files on a 40 GB volume, none of it visible to the gate, and `DISK_RESERVE_BYTES` — the +//! 10 GB standing between the party and Postgres losing the volume it writes WAL to — is consumed +//! by transient files. The `.tmp` sweeper only reclaims files idle for an hour, correctly, which +//! means nothing reclaims a burst on this timescale. +//! +//! ## Why bytes and not a request count +//! +//! A flat "N concurrent uploads" limit has to be sized for the worst case (a 500 MB video), which +//! makes it absurdly restrictive for the common case (a 3 MB photo). Budgeting bytes lets one +//! 500 MB video and two hundred photos coexist under the same ceiling, and it means the ceiling is +//! stated in the unit the disk actually cares about. +//! +//! The reservation is the streaming CAP, not the real size — the real size is unknowable until the +//! body has been read, which is far too late. Reserving the cap is deliberately pessimistic; that +//! pessimism is the safety margin. +//! +//! ## Why a permit and not a counter +//! +//! `OwnedSemaphorePermit` releases on drop. Every path out of the upload handler — success, error, +//! a client vanishing mid-body, a panic — therefore returns the reservation without any explicit +//! bookkeeping. A hand-rolled `AtomicI64` would need a decrement on each of those paths, and the +//! one that gets missed is the one that leaks the budget until restart. + +use std::sync::Arc; +use std::time::Duration; + +use tokio::sync::{OwnedSemaphorePermit, Semaphore}; + +/// Total transient upload bytes allowed on disk at once, in MiB. +/// +/// Sized against `DISK_RESERVE_BYTES` (10 GB): the reserve must survive a full burst with room to +/// spare, since Postgres is writing WAL to the same filesystem throughout. 4 GiB leaves ~6 GB of +/// the reserve untouched at the worst moment. +/// +/// It is NOT a throughput limit. On 2 vCPU the box cannot usefully ingest more than this at once +/// anyway — compression, ffmpeg, Postgres and TLS all contend for the same two cores — so the +/// budget mostly converts "everything is slow and the disk fills" into "a few uploads wait". +const BUDGET_MIB: u32 = 4096; + +/// How long an upload waits for room before being told to come back. +/// +/// Long enough to absorb the burst (a photo holds its reservation for well under a second), short +/// enough that a guest is not left staring at a spinner. On timeout the handler answers 503 with +/// `Retry-After`, which the client queue already treats as transient and retries with backoff. +const WAIT: Duration = Duration::from_secs(20); + +#[derive(Clone)] +pub struct UploadAdmission { + permits: Arc, +} + +impl UploadAdmission { + pub fn new() -> Self { + Self { + permits: Arc::new(Semaphore::new(BUDGET_MIB as usize)), + } + } + + /// Reserve room for a body capped at `cap_bytes`. The returned permit must be held for as long + /// as the temp file exists. + /// + /// `None` means the wait timed out and the caller should shed the request. + /// + /// A cap larger than the whole budget is clamped rather than refused. Otherwise an operator + /// raising `max_video_size_mb` above the budget would make `acquire_many` unsatisfiable and + /// every video upload would hang until timeout — a config change silently disabling video for + /// the event. Clamped, such an upload simply gets the whole budget to itself, which is the + /// honest interpretation of "one file may fill the machine". + pub async fn reserve(&self, cap_bytes: usize) -> Option { + let mib = cap_bytes.div_ceil(1024 * 1024).max(1); + let want = u32::try_from(mib).unwrap_or(BUDGET_MIB).min(BUDGET_MIB); + match tokio::time::timeout( + WAIT, + self.permits.clone().acquire_many_owned(want), + ) + .await + { + Ok(Ok(permit)) => Some(permit), + // The semaphore is never closed, so `Err` here is unreachable in practice; treat it + // the same as a timeout rather than panicking on the upload path. + Ok(Err(_)) => None, + Err(_) => { + tracing::warn!( + requested_mib = want, + "upload admission timed out; shedding to keep transient temp files bounded" + ); + None + } + } + } +} + +impl Default for UploadAdmission { + fn default() -> Self { + Self::new() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The budget must actually block once exhausted — otherwise this whole module is decoration. + #[tokio::test] + async fn a_full_budget_sheds_instead_of_admitting() { + let admission = UploadAdmission::new(); + let whole = admission + .reserve(BUDGET_MIB as usize * 1024 * 1024) + .await + .expect("first reservation takes the whole budget"); + + // Nothing left: a second reservation must not be granted. Raced against a short timeout so + // the test does not sit for the full WAIT. + let blocked = tokio::time::timeout( + Duration::from_millis(150), + admission.reserve(1024 * 1024), + ) + .await; + assert!(blocked.is_err(), "budget exhausted, yet a reservation was granted"); + + // ...and releasing the permit makes room again, so the budget is not a one-way latch. + drop(whole); + assert!( + admission.reserve(1024 * 1024).await.is_some(), + "budget did not recover after the permit was dropped" + ); + } + + /// A cap above the whole budget must be clamped, not left unsatisfiable. Unclamped, + /// `acquire_many` for more permits than exist never completes, so raising + /// `max_video_size_mb` past the budget would silently hang every video upload for 20s and + /// then shed it. + #[tokio::test] + async fn a_cap_larger_than_the_budget_is_clamped_rather_than_unsatisfiable() { + let admission = UploadAdmission::new(); + let oversized = (BUDGET_MIB as usize + 4096) * 1024 * 1024; + assert!( + admission.reserve(oversized).await.is_some(), + "an over-budget cap must still be admittable on an idle server" + ); + } +} diff --git a/backend/src/state.rs b/backend/src/state.rs index ebed452..7ee5a79 100644 --- a/backend/src/state.rs +++ b/backend/src/state.rs @@ -8,6 +8,7 @@ use crate::services::disk::DiskCache; use crate::services::media_total::MediaTotalCache; use crate::services::rate_limiter::RateLimiter; use crate::services::sse_tickets::SseTicketStore; +use crate::services::upload_admission::UploadAdmission; #[derive(Clone, Debug)] pub struct SseEvent { @@ -41,6 +42,10 @@ pub struct AppState { pub disk_cache: DiskCache, /// Cached sum of all media bytes, for the upload gate's keepsake-headroom check. pub media_total: MediaTotalCache, + /// Byte budget for upload bodies currently streaming to temp files. The headroom gate can + /// only refuse to COMMIT bytes that are already on disk; this is what bounds how many get + /// there at once. + pub upload_admission: UploadAdmission, } impl AppState { @@ -67,6 +72,7 @@ impl AppState { config_cache, disk_cache: DiskCache::new(), media_total: MediaTotalCache::new(), + upload_admission: UploadAdmission::new(), } } } diff --git a/docker-compose.yml b/docker-compose.yml index 7b9d325..891c839 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -136,6 +136,15 @@ services: # produces `https://` here and collapses the Caddyfile's site block below, so the stack # comes up with no TLS and no site and the only symptom is a browser error. ORIGIN: "https://${DOMAIN:?set DOMAIN in .env}" + # V8 sizes its old-space heap from the cgroup limit, but lands on ~101% of it (measured: + # heap_size_limit 259 MB inside a 256M container). So the JS heap ceiling sits ABOVE the + # container's entire budget — before base RSS (~60-90 MB), the C++ heap, or SSR response + # buffers, which are external memory V8 doesn't count at all. The practical effect is that + # V8 can never reach its own limit and run an emergency GC, so the only backpressure is a + # kernel SIGKILL: an arrival burst of ~100 guests SSR-rendering /join and /feed OOM-kills + # node, guests get the "Wir sind gleich zurück" page, it restarts, and the burst is still + # there. Setting the ceiling below the cgroup limit restores GC as the first line of defence. + NODE_OPTIONS: "--max-old-space-size=160" depends_on: - app expose: diff --git a/frontend/src/lib/upload-queue.test.ts b/frontend/src/lib/upload-queue.test.ts index 52fad20..a8540da 100644 --- a/frontend/src/lib/upload-queue.test.ts +++ b/frontend/src/lib/upload-queue.test.ts @@ -4,7 +4,8 @@ import { isReversibleLock, entryToQueueItem, shouldAbortForStall, - suspendedSinceLastTick + suspendedSinceLastTick, + MAX_SUSPEND_CREDIT_MS } from './upload-queue'; /** @@ -174,7 +175,7 @@ describe('suspendedSinceLastTick', () => { // Background tabs are throttled to ~1 tick/min WITHOUT the network stack pausing, so a // socket can be dead while ticks keep arriving. Unbounded crediting made this take ~18 // minutes, holding the queue's latch the whole time. - const CAP = 90_000; // MAX_SUSPEND_CREDIT_MS + const CAP = MAX_SUSPEND_CREDIT_MS; let lastProgressAt = 0; let lastTickAt = 0; let creditSpent = 0; @@ -211,7 +212,7 @@ describe('suspendedSinceLastTick', () => { * must go through here. */ function runTicks(lockMs: number, tickMs = 5_000, ticks = 3): boolean { - const CAP = 90_000; // MAX_SUSPEND_CREDIT_MS + const CAP = MAX_SUSPEND_CREDIT_MS; let lastProgressAt = 0; let lastTickAt = 0; let creditSpent = 0; diff --git a/frontend/src/lib/upload-queue.ts b/frontend/src/lib/upload-queue.ts index 127b3d3..3fa6550 100644 --- a/frontend/src/lib/upload-queue.ts +++ b/frontend/src/lib/upload-queue.ts @@ -130,7 +130,7 @@ const SUSPEND_TOLERANCE_MS = 2_000; * silent AND suspended spends it, and that is exactly the case where being wrong is cheap: one * re-send, bounded by MAX_AUTO_ATTEMPTS. */ -const MAX_SUSPEND_CREDIT_MS = STALL_TIMEOUT_MS; +export const MAX_SUSPEND_CREDIT_MS = STALL_TIMEOUT_MS; /** * Wall-clock the watchdog interval FAILED to cover because the page was suspended. diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index 6279539..d855ecb 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -347,22 +347,21 @@ /* ignore */ } }), - // A background transcode failed: the backend already cleaned up (refunded - // quota, removed the row) and an upload-deleted evicts the card. Only the - // uploader gets a toast — registered before upload-deleted so the card is - // still present to check ownership. - onSseEvent('upload-error', (data) => { - try { - const { upload_id } = JSON.parse(data) as { upload_id: string }; - const mine = uploads.find((u) => u.id === upload_id && u.user_id === myUserId); - if (mine) { - toast('Ein Upload konnte nicht verarbeitet werden.', 'error'); - void refreshQuota(); - } - } catch { - /* ignore */ - } - }), + // A background DERIVATIVE failed — the photo itself is fine. + // + // This used to read "the backend already cleaned up (refunded quota, removed the + // row)". None of that is true any more: 1d9fb11 stopped soft-deleting on a failed + // transcode precisely so a guest never loses a photo to a thumbnailing bug, the quota + // is deliberately NOT refunded (the bytes are still on disk), and `upload-deleted` is + // no longer emitted. So the row is present, the card is visible, and the feed falls + // back to serving the original. + // + // Which made this handler the last remaining way the old "your photo is gone" signal + // reached the guest — a red error toast about a photo sitting right there on screen, + // plus a pointless quota refetch. There is nothing here for the uploader to do and + // nothing for them to worry about, so say nothing. The host dashboard and the boot + // log still surface it to the people who can act on it. + onSseEvent('upload-error', () => {}), // A banned user's uploads were hidden — drop all their cards live. onSseEvent('user-hidden', (data) => { try { @@ -568,7 +567,34 @@ const byId = new Map(fetched.map((u) => [u.id, u])); uploads = uploads.map((u) => byId.get(u.id) ?? u); const fresh = fetched.filter((u) => !present.has(u.id)); - if (fresh.length) uploads = [...fresh, ...uploads]; + if (!fresh.length) return; + + // SORT the union — never blindly prepend. + // + // `fetched` is whole SERVER pages of 100, while `uploads` grows in pages of 20, so + // `uploads.length` is almost never a multiple of 100. Everything in the gap between what + // we hold and the next 100-boundary is absent from `present` and therefore looks "new" + // while being strictly OLDER than what we already show. Prepending it put ~80 photos from + // earlier in the evening above the newest ones — the gallery silently stopped being + // newest-first after the very first reconcile, which fires on any `upload-processed` + // event, i.e. constantly at a party. + // + // It also stalled infinite scroll: `nextCursor` still pointed at item 20, so the next + // `loadMore` re-fetched items already merged in, appended nothing, and the + // IntersectionObserver — which only re-fires on a change — never fired again. + // + // Sorting on the server's own ordering key fixes both, and additionally places an upload + // that arrived by SSE mid-fetch correctly rather than wherever the array splice left it. + const merged = [...fresh, ...uploads]; + merged.sort((a, b) => { + // (created_at DESC, id DESC) — mirrors the keyset cursor in backend/src/handlers/feed.rs. + // Ties on the timestamp are real: a burst of uploads can share a millisecond, and the + // id is what the server breaks them with. + if (a.created_at !== b.created_at) return a.created_at < b.created_at ? 1 : -1; + if (a.id === b.id) return 0; + return a.id < b.id ? 1 : -1; + }); + uploads = merged; } /** @@ -601,6 +627,12 @@ const res = await api.get(`/feed?${params}`); uploads = res.uploads; nextCursor = res.next_cursor; + // A fresh full load supersedes any earlier append failure. Without this the stale + // panel survived every refresh, filter change and pull-to-refresh — and once the new + // result set was short enough to have no next page, its "Erneut laden" button called a + // `loadMore` that returns immediately on `!nextCursor`. A false error message above a + // button that does nothing, for the rest of the evening. + loadMoreError = false; loadError = false; // A full refresh (pull-to-refresh, filter change) has resynced page 1, so the // "new posts" pill is no longer relevant. Cleared only on SUCCESS: clearing it up