From 05063694d2b1f5cce6419e60ff20669ceed7bbb5 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 8 Aug 2026 22:33:43 +0200 Subject: [PATCH] fix: close eight regressions the audit pass found, five of them mine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two adversarial reviews over 61119be, 1d9fb11 and eb0e405. The merge itself came back clean — client_upload_id end to end, TempFileGuard's arm/retarget/disarm, the supervised sweep wiring and v_feed's column parity were all verified sound. What follows is what my own three commits broke. BLOCKER — a post-release rebuild was permanently impossible, and it 404'd the keepsake 1d9fb11 deferred prune_superseded_archives to run only on success, so a failed rebuild could no longer destroy the last good archive. It did not follow that through: ensure_export_space runs BEFORE the prune, so at rebuild time the previous generation is still on disk and counted against free. That halves the gallery a rebuild can survive (~4.6 GB) relative to what the upload gate accepts (~7.8 GB) — and it self-locks, because invalidate_and_arm bumps the epoch on COMMIT, which 404s both download routes immediately, while the only code that could free the space now runs only after a success that can never happen. A guest deleting their own photo is enough to trigger it. Recovery needed `docker exec rm`. Now two-phase: try to build while preserving the old generation; if that genuinely does not fit, reclaim it and try once more. Strictly better than both the original ordering and my change — the old archive is sacrificed only when it is the only way to get a new one. BLOCKER — the deferred prune could delete the last archive when a worker LOST the race run_*_export_inner returned Ok(()) on the superseded/discard path, so `res.is_ok()` fired the prune with the worker's own RETIRED epoch as keep_seq. At that moment the winning generation is still `pending` with no file, so protected_files is empty and the last good archive was deleted with no replacement. Exactly the invariant deferring the prune was meant to establish. Returns Err(Superseded) now, which abandon_if_superseded already swallows for the caller. BLOCKER — the low-disk banner could never fire before the wall eb0e405's gate refuses at `free < keepsake + DISK_RESERVE`, while disk_is_low warned at `free < keepsake`. The two differ by the whole reserve, so the wall always came first: every guest blocked from uploading while the host dashboard showed ~27 GB free and no banner, with nobody on site. disk_is_low now shares the gate's expression plus a 25% margin, and a test asserts the banner fires at the gate threshold across the whole gallery-size range. BLOCKER — I raised the unauthenticated bcrypt ceiling 24x on a 2 vCPU box 1d9fb11 moved admin_login's tight bucket after verify_password (correct — that is what stops a guest locking the operator out) but replaced the incidental 5/min bound on bcrypt with 120/min and nothing global. bcrypt is on spawn_blocking, but tokio's blocking pool is 512 threads, so "off the runtime" is not "bounded": enough concurrent verifies preempt both async workers and uploads, feed and SSE stall. Three unauthenticated endpoints reach bcrypt and every guest shares one NAT IP, so per-IP limits bound nothing globally. Adds a process-wide semaphore of `cores - 1` around both verify and hash, and drops the ceiling to 30. Also correcting my own claim: "a correct password is never throttled" was wrong. The failure bucket cannot block it, but the CPU ceiling still can. The code comment said so; the commit message did not. BLOCKER — migration 025 could crash-loop the app on boot Its UPDATE derives `Name (8hex)` with no guard against idx_user_event_name_ci. A guest who had already joined as exactly that string makes the migration fail, which propagates out of create_pool, exits main, and `restart: unless-stopped` turns it into a permanent loop — a worse version of the lockout the migration exists to clean up. Now skips colliding rows (create_admin_user already falls back to Admin-<8hex>, so the cleanup is convenience, not load-bearing). Also `role = 'guest'` rather than `<> 'admin'`, which was renaming legitimately promoted hosts named "Host". DEGRADATION — the watchdog's suspension credit was unbounded Background tabs are throttled to ~1 tick/min WITHOUT the network stack pausing, and the tick gap cannot tell that from a freeze. Crediting every late tick grew the observed silence by only one interval per real minute, so a dead socket took ~18 minutes to detect while holding the queue's processing latch. Credit is now capped at one stall window and REFILLS on real progress: an upload that is moving survives any number of screen locks, while one that is silent and suspended is detected within ~3 minutes. DEGRADATION — the 4xx log line was an unauthenticated log-injection vector validate_display_name allowed newlines, several 4xx messages interpolate the name, and %message wrote it unescaped. Two unauthenticated /join requests could forge arbitrary lines in the only forensic record an unattended event has. Fixed at both ends: control characters rejected at the door, and `detail = ?message` escapes on the way out (which also stops colliding with tracing's reserved `message` field). 401/404 drop to DEBUG — they carry no operator signal and were the cheapest lines for a scanner to use to roll the 30 MB log window in minutes. DEGRADATION — the quota floor was inverted exactly where it mattered `computed.max(MIN.min(budget))`: `budget` is the whole disk's share, so below 500 MiB the "floor" became the entire remaining budget and EVERY uploader was authorised all of it — 400 MB free, 3 uploaders, 300 MB each. A test pinned that as correct under the name `the_floor_never_exceeds_what_the_disk_can_back`. Both fixed. Also replaces the headline gate test, which asserted its own precondition inside an `if` on that precondition and could not fail. It now pins what actually binds the gate to the preflight — that required_free_bytes charges for both halves — plus the ceiling band. Verified: 151/151 backend tests against a live Postgres, clippy clean, 58/58 vitest, svelte-check 0 errors, eslint clean, both builds, caddy validate, and the migration collision reproduced against Postgres 16 before and after. Co-Authored-By: Claude Opus 5 (1M context) --- .../025_reserved_names_and_pin_decay.up.sql | 26 ++++- backend/src/auth/handlers.rs | 77 +++++++++++-- backend/src/error.rs | 24 +++- backend/src/handlers/host.rs | 57 +++++++++- backend/src/handlers/upload.rs | 106 +++++++++++------- backend/src/services/export.rs | 60 +++++++++- frontend/src/lib/upload-queue.test.ts | 28 +++++ frontend/src/lib/upload-queue.ts | 31 ++++- 8 files changed, 350 insertions(+), 59 deletions(-) diff --git a/backend/migrations/025_reserved_names_and_pin_decay.up.sql b/backend/migrations/025_reserved_names_and_pin_decay.up.sql index 7a3e926..9db09b7 100644 --- a/backend/migrations/025_reserved_names_and_pin_decay.up.sql +++ b/backend/migrations/025_reserved_names_and_pin_decay.up.sql @@ -13,10 +13,32 @@ -- -- RENAMED, NEVER DELETED: the guest keeps their uploads, their PIN and their session. Only -- non-admin rows are touched — a real admin row named "Admin" is the expected state. +-- Two guards that are not optional, because this statement runs INSIDE the migration +-- transaction on boot and a failure here exits the process — `restart: unless-stopped` then +-- turns it into a crash loop with no in-app recovery. That is a strictly worse version of the +-- lockout this migration exists to clean up after. +-- +-- * role = 'guest', not role <> 'admin'. The enum also has 'host' (001), and hosts are +-- promoted from guests at runtime — so <> 'admin' renamed a legitimately promoted staff +-- member whose name happens to be "Host". +-- * NOT EXISTS. The target name is derived, not unique: `idx_user_event_name_ci` (007) is a +-- UNIQUE index on (event_id, lower(display_name)), and nothing stopped a second guest from +-- having already joined as exactly "Admin (a1b2c3d4)" — the old code had no reserved-name +-- guard and the join response hands each guest their own id. Rare, but the cost of losing +-- that bet is the whole event. +-- +-- A row that collides is simply left alone: `create_admin_user` already handles a name clash by +-- falling back to `Admin-<8hex>`, and `admin_login` no longer resolves by name at all, so this +-- cleanup is convenience rather than load-bearing. UPDATE "user" u SET display_name = u.display_name || ' (' || left(u.id::text, 8) || ')' - WHERE u.role <> 'admin' - AND lower(u.display_name) IN ('admin', 'administrator', 'host', 'eventsnap'); + WHERE u.role = 'guest' + AND lower(u.display_name) IN ('admin', 'administrator', 'host', 'eventsnap') + AND NOT EXISTS ( + SELECT 1 FROM "user" x + WHERE x.event_id = u.event_id + AND lower(x.display_name) = lower(u.display_name || ' (' || left(u.id::text, 8) || ')') + ); -- 2. PIN LOCKOUT DECAY. -- diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index a537686..ab99060 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -48,9 +48,22 @@ fn validate_display_name(raw: &str) -> Result<&str, AppError> { "Name muss zwischen 1 und 50 Zeichen lang sein.".into(), )); } - // Postgres rejects 0x00 in TEXT columns with a 500. Catch it here so callers see a clean - // 400 instead of an internal error. - if name.contains('\0') { + // No control characters. NUL is the hard requirement — Postgres rejects 0x00 in TEXT with a + // 500, so catching it here turns an internal error into a clean 400 — but the rest matter + // too, and for reasons beyond tidiness: + // + // * Newlines make the name a LOG INJECTION vector. Several 4xx messages interpolate it + // ("Der Name \"X\" ist bereits vergeben.") and those are logged; a name carrying a + // newline plus a plausible timestamp prefix lets two unauthenticated requests forge + // entries in the only forensic record an unattended event has. `error.rs` escapes on the + // way out as well — this is the other half, and the half that keeps the forged text out + // of the database and out of the feed byline in the first place. + // * A bare CR or a bidi override renders as a name that is not what was typed, in the feed, + // the host dashboard's moderation list and the keepsake. + // + // Deliberately NOT a whitelist: guests have accents, emoji and non-Latin scripts in their + // names, and rejecting those would be worse than the problem. + if name.chars().any(|c| c.is_control()) { return Err(AppError::BadRequest( "Name enthält ungültige Zeichen.".into(), )); @@ -237,7 +250,32 @@ fn dummy_pin_hash() -> &'static str { /// flood of `/recover` or `/admin/login` attempts stalls every other request on the box, /// including the feed. Offloading moves that cost to the blocking pool, which is sized for /// exactly this and whose saturation degrades logins rather than the whole app. +/// Process-wide ceiling on CONCURRENT bcrypt work. +/// +/// bcrypt is deliberately expensive — ~250 ms of a core at cost 12, and this deployment's own +/// runbook generates the admin hash at a higher cost than that. Every call is correctly on +/// `spawn_blocking`, but tokio's blocking pool defaults to 512 threads, so "off the async +/// runtime" is not the same as "bounded": enough concurrent hashes will preempt both async +/// worker threads a 2 vCPU box gets, and uploads, feed and SSE stall behind them. +/// +/// Three unauthenticated endpoints reach bcrypt — `/join` (hash), `/recover` (verify, including +/// a deliberate throwaway verify for unknown names) and `/admin/login` (verify) — each with only +/// a per-IP bucket in front, and at a venue every guest shares one public IP. A per-IP limit +/// therefore bounds nothing globally. +/// +/// `cores - 1` leaves a core for actually serving requests. Excess callers WAIT on the permit +/// rather than burning CPU, so a flood degrades to latency instead of an outage. +static BCRYPT_PERMITS: std::sync::LazyLock = + std::sync::LazyLock::new(|| { + let cores = std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(2); + tokio::sync::Semaphore::new(cores.saturating_sub(1).max(1)) + }); + async fn verify_password(candidate: String, hash: String) -> bool { + // `acquire()` only fails if the semaphore is closed, which never happens here. + let _permit = BCRYPT_PERMITS.acquire().await; tokio::task::spawn_blocking(move || bcrypt::verify(&candidate, &hash).unwrap_or(false)) .await .unwrap_or(false) @@ -246,6 +284,9 @@ async fn verify_password(candidate: String, hash: String) -> bool { /// Hash a secret on the blocking pool. Same reasoning as [`verify_password`] — and this one /// runs on the busiest auth path there is, since every guest who joins gets a PIN hashed. pub async fn hash_password(secret: String, cost: u32) -> Result { + // Same global ceiling as `verify_password` — `/join` hashes a PIN for every guest, and 100 + // guests scanning the QR at once is the arrival burst this box has to survive. + let _permit = BCRYPT_PERMITS.acquire().await; tokio::task::spawn_blocking(move || bcrypt::hash(&secret, cost)) .await .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))? @@ -411,11 +452,15 @@ pub struct AdminLoginResponse { /// Requests per minute per IP that may reach `verify_password` at all. /// -/// Not a security control — the failure bucket below is. This exists solely so an unauthenticated -/// endpoint cannot burn the box's CPU on cost-12 bcrypt (~250 ms each) at line rate. Set far above -/// anything a person typing a password can produce, because on venue NAT every guest shares the -/// operator's IP and this ceiling, unlike the failure bucket, can still refuse a correct password. -const ADMIN_LOGIN_CPU_CEILING: usize = 120; +/// Not a security control — the failure bucket below is. It bounds how deep a queue can form on +/// `BCRYPT_PERMITS`, which is what actually caps the CPU cost. +/// +/// Still far above anything a person typing a password produces, but note the honest limitation: +/// unlike the failure bucket, this ceiling CAN refuse a correct password, and on venue NAT every +/// guest shares the operator's IP. It is a smaller number than it first was for exactly that +/// reason — the earlier 120 was chosen when this was the only bound on bcrypt, which made it both +/// too weak to cap CPU and too coarse to be safe for the operator. +const ADMIN_LOGIN_CPU_CEILING: usize = 30; pub async fn admin_login( State(state): State, @@ -686,6 +731,22 @@ pub async fn request_pin_reset( mod tests { use super::*; + /// Control characters are rejected at the door. Newlines in particular: several 4xx messages + /// interpolate the display name and those are logged, so a name carrying a newline plus a + /// plausible prefix would let two unauthenticated requests forge lines in the event's only + /// forensic record. `error.rs` escapes on output too; this keeps it out of the database and + /// the feed byline in the first place. + #[test] + fn a_display_name_may_not_carry_control_characters() { + for bad in ["Anna\nERROR forged", "Anna\rX", "Anna\u{0}X", "A\u{7}B"] { + assert!(validate_display_name(bad).is_err(), "{bad:?} must be rejected"); + } + // Real guests have accents, emoji and non-Latin names — never reject those. + for good in ["Anna", "Zo\u{eb}", "Jos\u{e9}", "\u{5c71}\u{7530}", "Anna \u{1f389}"] { + assert!(validate_display_name(good).is_ok(), "{good:?} must be allowed"); + } + } + /// THE defect, stated as arithmetic: the account-lock threshold sat BELOW the per-(IP, name) /// attempt ceiling, so a single IP could exhaust it and lock any guest whose display name is /// visible on the feed — every 15 minutes, indefinitely. The tier meant to protect a guest diff --git a/backend/src/error.rs b/backend/src/error.rs index 9da0c09..eebe314 100644 --- a/backend/src/error.rs +++ b/backend/src/error.rs @@ -94,8 +94,30 @@ impl IntoResponse for AppError { // no path, method or user id to attach. Status + code + message is what can honestly be // reported from here, and it is enough to see the SHAPE of a bad evening. Raising // `tower_http` to DEBUG instead was considered and rejected — see the note in main.rs. + // + // `detail = ?message`, NOT `%message`. Two reasons, both learned the hard way: + // + // * `message` is tracing's own reserved field for an event's format literal, so `%message` + // printed unlabelled and would collide under a JSON layer. + // * Debug formatting QUOTES AND ESCAPES the string. `validate_display_name` allows + // newlines (it rejects only NUL and length), and several 4xx messages interpolate the + // guest's chosen name — `Der Name "X" ist bereits vergeben.` So with Display + // formatting, two unauthenticated `/join` requests could forge arbitrary lines in the + // only forensic record an unattended event has: pick a name containing a newline and a + // plausible log prefix, then trigger the 409. Escaping closes that. + // + // 401 and 404 are logged at DEBUG rather than WARN. They carry no operator signal (an + // expired session, a mistyped URL) and they are the cheapest lines for a scanner to + // generate — at ~260 bytes each against the 30 MB the json-file driver retains + // (docker-compose.yml), a sustained flood could otherwise roll the whole window in + // minutes and destroy the post-event forensics this logging exists to provide. if status.is_client_error() { - tracing::warn!(status = status.as_u16(), code, %message, "request rejected"); + let noisy = status == StatusCode::UNAUTHORIZED || status == StatusCode::NOT_FOUND; + if noisy { + tracing::debug!(status = status.as_u16(), code, detail = ?message, "request rejected"); + } else { + tracing::warn!(status = status.as_u16(), code, detail = ?message, "request rejected"); + } } let mut body = serde_json::json!({ diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index d8e6aea..ebc5f23 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -59,8 +59,20 @@ const LOW_DISK_FLOOR_BYTES: u64 = 10_000_000_000; /// gallery-sized archives, and the only moment a host can do anything about that is BEFORE they /// release. Warning at "you could not build the keepsake right now" turns a post-event dead end /// into a decision someone can still make. +/// +/// IT MUST FIRE BEFORE THE UPLOAD GATE CLOSES, and that is why the reserve and the margin are +/// here. The gate in `handlers::upload` refuses at `free < keepsake_required + DISK_RESERVE_BYTES`; +/// warning at `free < keepsake_required` alone meant the two differed by the whole reserve, so +/// the wall was always hit FIRST. Every guest would be blocked from uploading while this +/// dashboard showed a comfortable disk and no banner at all — on the shipped 40 GB box, uploads +/// stopping with ~27 GB free and nothing on screen to explain it, with no operator present. +/// +/// The 25% margin makes it a warning rather than an obituary: the host sees it while there is +/// still room to act (delete a few large videos, which refunds immediately and reopens the gate). fn disk_is_low(free: u64, keepsake_required: u64) -> bool { - free < LOW_DISK_FLOOR_BYTES || free < keepsake_required + 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 } /// Count non-banned hosts/admins in the event OTHER than `excluding` — the operators @@ -813,12 +825,15 @@ pub async fn release_gallery( #[cfg(test)] mod tests { use super::{LOW_DISK_FLOOR_BYTES, disk_is_low}; + use crate::handlers::upload::DISK_RESERVE_BYTES; + use crate::services::export::required_free_bytes; const GB: u64 = 1_000_000_000; #[test] fn a_healthy_disk_with_room_for_the_keepsake_is_not_low() { - assert!(!disk_is_low(40 * GB, 25 * GB)); + // Room for the keepsake AND the reserve the upload gate holds back, with margin. + assert!(!disk_is_low(60 * GB, 25 * GB)); } #[test] @@ -828,7 +843,29 @@ mod tests { // 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)); - assert!(!disk_is_low(LOW_DISK_FLOOR_BYTES, 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(20 * GB, 0), "a roomy empty disk is not low"); + } + + /// THE PROPERTY THIS EXISTS FOR: the host must be warned BEFORE guests are blocked. + /// + /// `handlers::upload` refuses at `free < keepsake_required + DISK_RESERVE_BYTES`. If the + /// banner fires only at or below that, the host's first signal is 100 guests being unable + /// 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() { + 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; + assert!( + disk_is_low(gate_closes_at, required), + "at media={media_gb}GB the gate is about to close but no banner is shown" + ); + } } #[test] @@ -841,13 +878,21 @@ mod tests { #[test] fn the_keepsake_trigger_is_exact_at_the_boundary() { - assert!(!disk_is_low(66 * GB, 66 * GB), "exactly enough is enough"); - assert!(disk_is_low(66 * GB - 1, 66 * GB)); + // The boundary is the UPLOAD GATE's threshold plus a 25% margin, not the bare keepsake + // size — see `disk_is_low`. Warning at the bare size fired only after the gate had + // already blocked every guest. + let required = 20 * GB; + let gate = required + DISK_RESERVE_BYTES as u64; + let warn_at = gate + gate / 4; + assert!(!disk_is_low(warn_at, required), "exactly enough is enough"); + assert!(disk_is_low(warn_at - 1, required)); } #[test] fn an_empty_gallery_needs_nothing_and_only_the_floor_applies() { - assert!(!disk_is_low(11 * GB, 0)); + // 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)); assert!(disk_is_low(9 * GB, 0)); } } diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 284026a..0bc8733 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -1064,8 +1064,18 @@ fn quota_limit_bytes(free_disk: i64, tolerance: f64, active_uploaders: i64, expe // The floor may never exceed what the disk can actually back. Raising a ceiling the volume // cannot honour would hand out an allowance on a full disk — turning the quota from a // usability floor into a way to finish filling the filesystem that Postgres writes WAL to. - let backed_floor = MIN_QUOTA_LIMIT_BYTES.min(budget as i64); - computed.max(backed_floor) + // Only raise to the floor when the disk can back a floor-sized allowance for real. + // + // The earlier form was `computed.max(MIN.min(budget))`, which inverts exactly where it + // matters: `budget` is the WHOLE disk's share, not one user's, so once budget < 500 MiB the + // "floor" became the entire remaining budget and every uploader was authorised all of it — + // 400 MB free with 3 uploaders promised 300 MB each. Below the floor, fall through to the + // divided value, which is the only number that still shares the space out. + if budget < MIN_QUOTA_LIMIT_BYTES as f64 { + computed + } else { + computed.max(MIN_QUOTA_LIMIT_BYTES) + } } /// Computes the per-user storage quota using @@ -1560,50 +1570,60 @@ mod tests { /// THE INVARIANT THE UPLOAD GATE EXISTS FOR: if an upload is accepted, the keepsake must /// still be buildable afterwards. /// - /// The gate and `ensure_export_space` answer the same question at different times, from the - /// same `required_free_bytes`. If they ever drift, the failure is silent and terminal — every - /// upload succeeds and the archive can never be built, discovered only when the host taps - /// release and there is nobody left to fix it. This pins the two together. + /// An earlier version of this test computed the gate's threshold and the preflight's + /// threshold with the SAME expression and then asserted one against the other inside an + /// `if` on that expression — a tautology that could not fail and would not have noticed a + /// term being added to `ensure_export_space`. What actually binds the two together is that + /// both call `required_free_bytes`, so what is worth pinning is the SHAPE of that function + /// and the ceiling it produces on the real volume. /// - /// Models the real box: 40 GB volume, ~5 GB consumed by OS, images and Postgres. + /// Models the shipped box: 40 GB, ~5 GB consumed by OS, images and Postgres. #[test] - fn an_accepted_upload_always_leaves_room_to_build_the_keepsake() { + fn the_gate_ceiling_keeps_both_keepsake_halves_and_the_reserve_affordable() { const USABLE: i64 = 35 * GB; let reserve = DISK_RESERVE_BYTES; - // Walk the gallery upward in 250 MB steps and assert the two agree at every point. - let mut media: i64 = 0; - let step: i64 = 250 * 1024 * 1024; - let mut last_accepted = 0i64; + // Walk the gallery upward and find the last size the gate would accept. + let mut ceiling = 0i64; + let step = 250 * 1024 * 1024; + let mut media = 0i64; while media < USABLE { - // `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 = USABLE - media_after; - let required = - crate::services::export::required_free_bytes(media_after as u64, 2) as i64 + reserve; - let gate_accepts = free >= required; - - if gate_accepts { - // The export preflight must agree, using the SAME arithmetic it will run later. - let preflight_needs = - crate::services::export::required_free_bytes(media_after as u64, 2) as i64 - + reserve; - assert!( - free >= preflight_needs, - "gate accepted at media={media_after} but the preflight would refuse" - ); - last_accepted = media_after; + media += step; + let free = USABLE - media; + if free >= crate::services::export::required_free_bytes(media as u64, 2) as i64 + reserve + { + ceiling = media; } - media = media_after; } - // Sanity-check the ceiling is where the arithmetic says: 35 = 2.2·M + 10 ⇒ M ≈ 7.8 GB. - // Pinned loosely (6–9 GB) so a deliberate change to the overhead multiplier or the - // reserve fails this test loudly rather than silently moving the cliff. + // At the ceiling, BOTH archives and the reserve must genuinely fit in what is left. + let free_at_ceiling = USABLE - ceiling; + let both_halves = crate::services::export::required_free_bytes(ceiling as u64, 2) as i64; assert!( - (6 * GB..=9 * GB).contains(&last_accepted), - "expected the gallery ceiling near 7.8 GB on a 35 GB volume, got {last_accepted} bytes" + free_at_ceiling >= both_halves + reserve, + "at the ceiling the keepsake ({both_halves}) + reserve ({reserve}) must fit in \ + {free_at_ceiling}" + ); + + // And one byte more must NOT fit — i.e. the ceiling is where the gate actually closes, + // not somewhere short of it. + let over = ceiling + step; + let free_over = USABLE - over; + assert!( + free_over < crate::services::export::required_free_bytes(over as u64, 2) as i64 + reserve, + "the gate should already be closed one step past the ceiling" + ); + + // Independently: `required_free_bytes` must charge for TWO gallery-sized archives. + // If someone changes `armed` or the overhead, this is the line that notices. + let one = crate::services::export::required_free_bytes(ceiling as u64, 1) as i64; + assert_eq!(both_halves, one * 2, "a release arms both halves"); + + // Pinned loosely so a deliberate change to the overhead or the reserve fails loudly + // rather than silently moving the cliff. + assert!( + (6 * GB..=9 * GB).contains(&ceiling), + "expected a gallery ceiling near 7.8 GiB on a 35 GB volume, got {ceiling} bytes" ); } @@ -1621,13 +1641,23 @@ mod tests { /// 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. #[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"); - // 400 MB free * 0.75 = 300 MB — below the floor, so the disk wins. + + // 400 MB free x 0.75 = 300 MB of budget, below the 500 MiB floor. The floor must NOT + // apply: with 3 uploaders the answer is the divided share, not the whole budget. let tight = quota_limit_bytes(400 * 1024 * 1024, 0.75, 3, 1); - assert_eq!(tight, 300 * 1024 * 1024); + assert_eq!(tight, 100 * 1024 * 1024, "a scarce budget is still divided"); assert!(tight < MIN_QUOTA_LIMIT_BYTES); + + // The promise across all uploaders must stay inside the budget. + assert!( + tight * 3 <= (400.0 * 1024.0 * 1024.0 * 0.75) as i64, + "the sum of per-user allowances must not exceed the disk's share" + ); } #[test] diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index cfa4536..a8c2eea 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -503,7 +503,49 @@ async fn run_zip_export( // `pending` with no worker and no error — the spinner-forever state `mark_failed`'s status // guard was widened to prevent. Failing here goes through the caller's `mark_failed`, so the // host gets the reason. - ensure_export_space(pool, event_id, export_path).await?; + // Two-phase, and the order is the whole point. + // + // Deferring the prune (so a failed rebuild can never leave the event with no archive at + // all) has a cost the first version of this did not follow through on: at rebuild time the + // previous generation is still on disk and still counted against free space, so the + // preflight demands room for BOTH. That halves the gallery size a rebuild can survive + // relative to the size the upload gate allows — and every path that bumps the epoch (a + // guest deleting their own photo, a caption edit, a ban, "Neu erzeugen") retires the + // current keepsake IMMEDIATELY on commit. So above that threshold the download 404s, the + // rebuild is refused, and the only thing that could free the space is the prune that now + // only runs on success. Permanently stuck, unreachable from any handler. + // + // So: try to build while preserving the old generation. If that genuinely does not fit, + // the old generation is the one thing we can reclaim — sacrifice it and try once more. A + // keepsake that exists beats one we preserved but can never replace. + if ensure_export_space(pool, event_id, export_path).await.is_err() { + tracing::warn!( + "not enough room to rebuild alongside the previous keepsake; reclaiming it first" + ); + prune_superseded_archives(pool, export_path, "Gallery", event_id, epoch).await; + // Two-phase, and the order is the whole point. + // + // Deferring the prune (so a failed rebuild can never leave the event with no archive at + // all) has a cost the first version of this did not follow through on: at rebuild time the + // previous generation is still on disk and still counted against free space, so the + // preflight demands room for BOTH. That halves the gallery size a rebuild can survive + // relative to the size the upload gate allows — and every path that bumps the epoch (a + // guest deleting their own photo, a caption edit, a ban, "Neu erzeugen") retires the + // current keepsake IMMEDIATELY on commit. So above that threshold the download 404s, the + // rebuild is refused, and the only thing that could free the space is the prune that now + // only runs on success. Permanently stuck, unreachable from any handler. + // + // So: try to build while preserving the old generation. If that genuinely does not fit, + // the old generation is the one thing we can reclaim — sacrifice it and try once more. A + // keepsake that exists beats one we preserved but can never replace. + if ensure_export_space(pool, event_id, export_path).await.is_err() { + tracing::warn!( + "not enough room to rebuild alongside the previous keepsake; reclaiming it first" + ); + prune_superseded_archives(pool, export_path, "Memories", event_id, epoch).await; + ensure_export_space(pool, event_id, export_path).await?; + } + } // On error, mark THIS generation failed — a no-op if we've since been superseded (the // caller in `spawn_export_jobs` does it, epoch-guarded). Temp artifacts are cleaned up @@ -657,7 +699,13 @@ async fn run_zip_export_inner( tracing::info!( "ZIP export for event {event_id} superseded (epoch {epoch} retired); discarded" ); - return Ok(()); + // Err, NOT Ok. `abandon_if_superseded` swallows this sentinel for the caller, so the + // outcome is unchanged — but `run_*_export`'s deferred prune keys off `res.is_ok()`, + // and a worker that LOST the epoch race reporting success made it reclaim generations + // older than its own RETIRED epoch. At that moment the winning generation is still + // `pending` with no file, so nothing protected the last good archive and it was deleted + // with no replacement on disk — the precise outcome deferring the prune exists to stop. + return Err(Superseded.into()); } prune_stale_export_files(pool, &exports_dir, "Gallery", event_id, epoch).await; @@ -1090,7 +1138,13 @@ async fn run_html_export_inner( tracing::info!( "HTML export for event {event_id} superseded (epoch {epoch} retired); discarded" ); - return Ok(()); + // Err, NOT Ok. `abandon_if_superseded` swallows this sentinel for the caller, so the + // outcome is unchanged — but `run_*_export`'s deferred prune keys off `res.is_ok()`, + // and a worker that LOST the epoch race reporting success made it reclaim generations + // older than its own RETIRED epoch. At that moment the winning generation is still + // `pending` with no file, so nothing protected the last good archive and it was deleted + // with no replacement on disk — the precise outcome deferring the prune exists to stop. + return Err(Superseded.into()); } prune_stale_export_files(pool, &exports_dir, "Memories", event_id, epoch).await; diff --git a/frontend/src/lib/upload-queue.test.ts b/frontend/src/lib/upload-queue.test.ts index 7107f80..ff6c11d 100644 --- a/frontend/src/lib/upload-queue.test.ts +++ b/frontend/src/lib/upload-queue.test.ts @@ -166,6 +166,34 @@ describe('suspendedSinceLastTick', () => { expect(suspendedSinceLastTick(now - 6_900, now, 5_000)).toBe(0); }); + /** + * The bound that matters: however the page is throttled or frozen, a genuinely dead socket + * must be detected in a time a guest would tolerate — not left to `xhr.timeout` (5-60 min). + */ + it('detects a dead socket in bounded time even under 1-tick-per-minute throttling', () => { + // 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 + let lastProgressAt = 0; + let lastTickAt = 0; + let creditSpent = 0; + let clock = 0; + let ticks = 0; + while (!shouldAbortForStall(lastProgressAt, clock, false) && ticks < 100) { + clock += 60_000; // throttled tick + const credit = Math.min( + suspendedSinceLastTick(lastTickAt, clock, 5_000), + Math.max(0, CAP - creditSpent) + ); + creditSpent += credit; + lastProgressAt = Math.min(clock, lastProgressAt + credit); + lastTickAt = clock; + ticks += 1; + } + expect(clock).toBeLessThanOrEqual(240_000); + }); + it('credits the whole frozen window when the interval did not run', () => { // Screen locked ~2 minutes: a 5s interval arriving 130s late. expect(suspendedSinceLastTick(now - 130_000, now, 5_000)).toBe(125_000); diff --git a/frontend/src/lib/upload-queue.ts b/frontend/src/lib/upload-queue.ts index 951bb05..cfbac77 100644 --- a/frontend/src/lib/upload-queue.ts +++ b/frontend/src/lib/upload-queue.ts @@ -100,6 +100,24 @@ export function shouldAbortForStall( */ const SUSPEND_TOLERANCE_MS = 2_000; +/** + * Total suspension credit a single silent stretch may ever be granted. + * + * A tick gap cannot distinguish "the page was frozen" from "the page is being throttled to one + * tick per minute while the network stack keeps running" — and in the second case the socket can + * be genuinely dead. Crediting every late tick meant the observed silence grew by only one + * interval per real minute, so a dead upload took ~18 minutes to detect while holding the + * queue's `processing` latch and blocking every other photo the guest had queued. + * + * Capping the total at one stall window bounds detection at roughly 2x the ceiling (~3 min) + * whatever the throttling pattern, while still absorbing the pocket-length freezes this exists + * for. Crucially the budget RESETS on real progress (see the `progress` handler), so a long + * upload that is actually moving survives any number of screen locks — only an upload that is + * 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; + /** * Wall-clock the watchdog interval FAILED to cover because the page was suspended. * @@ -945,6 +963,7 @@ async function uploadItem(id: string): Promise { // that without also punishing a healthy slow link. let lastProgressAt = Date.now(); let lastTickAt = Date.now(); + let creditSpent = 0; let bodySent = false; let stalled = false; const stallTimer = setInterval(() => { @@ -958,7 +977,15 @@ async function uploadItem(id: string): Promise { // a period in which it could not observe anything is not evidence of silence. // Clamped to `now` so a progress event delivered right at resume cannot push the // timestamp into the future. - lastProgressAt = Math.min(now, lastProgressAt + suspendedSinceLastTick(lastTickAt, now)); + // Bounded: see MAX_SUSPEND_CREDIT_MS. Without the cap, sustained background + // throttling credited back all but one interval on every tick, so a dead socket + // took ~18 minutes to notice while the queue's latch stayed held. + const credit = Math.min( + suspendedSinceLastTick(lastTickAt, now), + Math.max(0, MAX_SUSPEND_CREDIT_MS - creditSpent) + ); + creditSpent += credit; + lastProgressAt = Math.min(now, lastProgressAt + credit); lastTickAt = now; if (!shouldAbortForStall(lastProgressAt, now, bodySent)) return; stalled = true; @@ -974,6 +1001,8 @@ async function uploadItem(id: string): Promise { xhr.upload.addEventListener('progress', (e) => { lastProgressAt = Date.now(); + // Bytes moved: this stretch of silence is over, so the suspension budget refills. + creditSpent = 0; if (e.lengthComputable) { const pct = Math.round((e.loaded / e.total) * 100); queueItems.update((items) =>