From 281eb3bec7af54b7b3a3d63758dedb62f9c3158c Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 29 Jul 2026 19:38:26 +0200 Subject: [PATCH] fix(export): refuse an export that cannot fit, and stop peaking at two generations Nothing in export.rs ever asked whether the keepsake would fit. Both archives write their media `Compression::Stored`, so each is essentially a byte-for-byte second copy of the originals -- Gallery.zip always, and Memories.zip for every video and every image at or under 5 MB. On the documented CX33 (80 GB, all three volumes on one filesystem) the upload quota's fixed point leaves ~40 GB free, and a release spawns BOTH halves concurrently against it. The failure is not "the export failed", it is "the deliverable is stuck": 1. ENOSPC lands partway through a multi-GB write. 2. The epoch has already moved, so the job row is `failed` at the CURRENT generation and readiness (epoch = event.export_epoch AND status = 'done') is false -- GET /export/zip 404s. 3. The last good archive sits on disk, unreferenced and unreachable. 4. POST /host/export/rebuild, the only escape, re-arms the same doomed write. Three changes. Reclaim before building. `prune_stale_export_files` ran only after the new archive was written, renamed and finalised. That reads as durability but buys nothing: the moment `invalidate_and_arm` bumps the epoch the old archive is ALREADY unreachable, so keeping it reserves gigabytes for a download nobody can perform -- and for a takedown it is content someone explicitly asked to have removed. Peak usage is now one generation. Narrower than the post-finalize prune on purpose: final archives only, never a `.tmp` or a `viewer_tmp_` dir, since a superseded worker can still be streaming into those and at build START is far more likely to be alive. Preflight the space. SUM(original_size_bytes) over exactly `query_uploads`' visibility filter, +10% for ZIP overhead, multiplied by the number of armed jobs -- without that multiplier each of the two concurrent halves independently sees "it fits" and together they don't. Runs AFTER claim_job, not before as reported: bailing before the claim leaves the row `pending` with no worker and no error, the spinner-forever state `mark_failed`'s status guard exists to prevent. Fails open when the mount can't be read, exactly as the upload quota does. Show the host the reason. /export/status returned {status, progress_pct} and nothing else, so the host dashboard could only render "fehlgeschlagen" next to the retry button. The message was written to the row and surfaced solely in the ADMIN job list -- a different screen, possibly a different person. It now travels with the status, and only on a failure, so a message left on a since-succeeded row can't appear beside a green "ist bereit". Tests: 10 unit (the u128 clamp caught a real bug in the first draft -- saturating_mul then /100 turns an overflow into a number ~100x too small, the one direction that authorises the write being guarded against; the carried-forward archive must survive its own older epoch in the filename), 4 DB-backed (the estimate is asserted against the row set the archive actually contains, not against a restatement of the WHERE clause, so the two queries cannot drift), 3 e2e over the four-hop plumbing. Co-Authored-By: Claude Opus 5 --- backend/src/handlers/admin.rs | 27 +- backend/src/services/disk.rs | 11 + backend/src/services/export.rs | 333 +++++++++++++++++- backend/tests/common/mod.rs | 82 +++++ backend/tests/export_preflight.rs | 154 ++++++++ e2e/fixtures/db.ts | 11 +- .../06-export/failure-reason-visible.spec.ts | 95 +++++ frontend/src/lib/export-status-store.ts | 2 + frontend/src/routes/host/+page.svelte | 13 + 9 files changed, 705 insertions(+), 23 deletions(-) create mode 100644 backend/tests/export_preflight.rs create mode 100644 e2e/specs/06-export/failure-reason-visible.spec.ts diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index cb8fb96..c31a95d 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -456,8 +456,13 @@ pub async fn export_status( // worker superseded mid-run) is meaningless — surfacing its frozen `running`/77% would show a // progress bar that never moves for a keepsake nobody is building. It reads as "locked" (no // current job), which is exactly what it is. - let jobs: Vec<(String, String, i16)> = sqlx::query_as( - "SELECT j.type::text, j.status::text, j.progress_pct + // `error_message` is carried here, not just on the admin dashboard's job list. The host is the + // one who releases the keepsake and the one who owns the "Erneut versuchen" button, but this + // endpoint used to hand them a bare `failed` — so a fully actionable reason (notably the disk + // preflight's "needs X GB, Y GB free") was written to the row and then shown to nobody who + // could act on it. An admin-only diagnostic is not a diagnostic for the person on the spot. + let jobs: Vec<(String, String, i16, Option)> = sqlx::query_as( + "SELECT j.type::text, j.status::text, j.progress_pct, j.error_message FROM export_job j JOIN event e ON e.id = j.event_id WHERE e.id = $1 AND j.epoch = e.export_epoch", @@ -468,9 +473,21 @@ pub async fn export_status( let job_status = |type_name: &str| { jobs.iter() - .find(|(t, _, _)| t == type_name) - .map(|(_, status, pct)| serde_json::json!({ "status": status, "progress_pct": pct })) - .unwrap_or_else(|| serde_json::json!({ "status": "locked", "progress_pct": 0 })) + .find(|(t, _, _, _)| t == type_name) + .map(|(_, status, pct, err)| { + serde_json::json!({ + "status": status, + "progress_pct": pct, + // Only on a failure. A stale message left on a row that has since been re-armed + // would otherwise show an error next to a running progress bar. + "error_message": if status == "failed" { err.clone() } else { None }, + }) + }) + .unwrap_or_else(|| { + serde_json::json!({ + "status": "locked", "progress_pct": 0, "error_message": null, + }) + }) }; Ok(Json(serde_json::json!({ diff --git a/backend/src/services/disk.rs b/backend/src/services/disk.rs index 171ef4c..2afc197 100644 --- a/backend/src/services/disk.rs +++ b/backend/src/services/disk.rs @@ -76,6 +76,17 @@ impl Default for DiskCache { } } +/// UNCACHED free-space reading for the filesystem backing `path`. +/// +/// Deliberately bypasses [`DiskCache`]. The cache exists for the quota poll, where a 15s-stale +/// number is fine because it is only ever advisory. The export preflight is the opposite case: it +/// decides whether to start writing a multi-GB archive, and the sibling export worker running +/// concurrently can move free space by tens of gigabytes well inside the TTL. A stale reading there +/// would authorise exactly the write that fills the disk. +pub fn free_bytes(path: &Path) -> Option { + read_disk_for_path(path).map(|d| d.free) +} + /// Resolve the filesystem backing `media_path` and read its total/free bytes. /// /// Snapshots the mount table via `sysinfo`, then delegates the selection to the pure diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index 3abea0f..51099a5 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -476,6 +476,15 @@ async fn run_zip_export( return Ok(()); } + // Reclaim BEFORE measuring: the superseded archive is already unreachable, and the space it + // holds is very often exactly the space this rebuild needs. + prune_superseded_archives(pool, export_path, "Gallery", event_id, epoch).await; + // AFTER the claim, not before. A preflight that bailed before claiming would leave the row + // `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?; + // 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 // here so a failing export can't leak them (which is what fills the disk in the first place). @@ -658,6 +667,11 @@ async fn run_html_export( return Ok(()); } + // See run_zip_export: reclaim the superseded generation first, then refuse at the door rather + // than ENOSPC mid-write. + prune_superseded_archives(pool, export_path, "Memories", event_id, epoch).await; + ensure_export_space(pool, event_id, export_path).await?; + let res = run_html_export_inner( epoch, event_id, @@ -1161,6 +1175,182 @@ fn parse_gen_seq(name: &str, prefix: &str, suffix: &str) -> Option { .ok() } +/// Filenames a live (current-epoch, `done`) job row still points at — OFF LIMITS to every prune, +/// regardless of the epoch encoded in the name. +/// +/// A ViewerOnly regeneration carries the finished ZIP forward by re-stamping its row to the new +/// epoch WITHOUT renaming the file, so `Gallery...zip` is still the served archive and +/// deleting it by filename-epoch would 404 the download. +async fn protected_files(pool: &PgPool, event_id: Uuid) -> Vec { + sqlx::query_scalar::<_, String>( + "SELECT split_part(j.file_path, '/', -1) FROM export_job j + JOIN event e ON e.id = j.event_id + WHERE j.event_id = $1 AND j.epoch = e.export_epoch + AND j.status = 'done' AND j.file_path IS NOT NULL", + ) + .bind(event_id) + .fetch_all(pool) + .await + .unwrap_or_default() +} + +/// Reclaim superseded FINAL archives BEFORE this generation starts writing its own. +/// +/// 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. +/// +/// 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 +/// `finalize_job` fails) or never will. `.tmp` files and `viewer_tmp_` staging dirs are NOT touched +/// here: a superseded worker can still be streaming into those, and at build START it is far more +/// likely to be alive than at finalize time. +async fn prune_superseded_archives( + pool: &PgPool, + exports_dir: &Path, + prefix: &str, + event_id: Uuid, + keep_seq: i64, +) { + let protected = protected_files(pool, event_id).await; + let final_prefix = format!("{prefix}.{event_id}."); + let mut rd = match tokio::fs::read_dir(exports_dir).await { + Ok(rd) => rd, + Err(_) => return, + }; + let mut reclaimed = 0u64; + while let Ok(Some(entry)) = rd.next_entry().await { + let name = entry.file_name(); + let name = name.to_string_lossy(); + if !is_superseded_archive(&name, &final_prefix, keep_seq, &protected) { + continue; + } + let len = entry.metadata().await.map(|m| m.len()).unwrap_or(0); + if tokio::fs::remove_file(entry.path()).await.is_ok() { + reclaimed += len; + } + } + if reclaimed > 0 { + tracing::info!( + "reclaimed {reclaimed} bytes of superseded {prefix} archives before rebuilding \ + event {event_id} @ epoch {keep_seq}" + ); + } +} + +/// Is `name` a FINAL archive of a strictly-older generation that no live row points at? +/// +/// Pure so the two dangerous cases can be pinned without a filesystem: the carried-forward archive +/// (protected despite an older epoch in its name) and the in-flight `.tmp` (never matched at all). +fn is_superseded_archive( + name: &str, + final_prefix: &str, + keep_seq: i64, + protected: &[String], +) -> bool { + if protected.iter().any(|p| p == name) { + return false; + } + parse_gen_seq(name, final_prefix, ".zip").is_some_and(|n| n < keep_seq) +} + +/// Bytes the media in this event's keepsake will occupy, as an UPPER BOUND per archive. +/// +/// Both archives write their media entries `Compression::Stored`, so an archive is essentially a +/// byte-for-byte second copy of the originals: `Gallery.zip` always, and `Memories.zip` for every +/// video ([`MediaSource::Original`]) and every image at or under 5 MB. Images over 5 MB are +/// downscaled to 2000px first, which only makes this estimate more conservative — the direction we +/// want, since being wrong low means ENOSPC halfway through. +/// +/// Matches [`query_uploads`]' visibility filter exactly, so hidden/banned uploads aren't counted. +async fn estimate_export_bytes(pool: &PgPool, event_id: Uuid) -> Result { + let (bytes,): (i64,) = sqlx::query_as( + "SELECT COALESCE(SUM(u.original_size_bytes), 0)::bigint + FROM upload u + JOIN \"user\" usr ON usr.id = u.user_id + WHERE u.event_id = $1 AND u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE", + ) + .bind(event_id) + .fetch_one(pool) + .await + .context("estimating the export size")?; + Ok(bytes.max(0) as u64) +} + +/// Headroom multiplier over the raw media sum: ZIP central directory, per-entry headers, the +/// embedded viewer, and the HTML export's temp staging. +const EXPORT_SIZE_OVERHEAD_PCT: u64 = 110; + +/// Bytes this export must have available, given the raw media sum and how many jobs are competing. +/// +/// `armed` is the multiplier that keeps two concurrent workers honest. `spawn_export_jobs` starts +/// the ZIP and the HTML halves at the same instant and both are gallery-sized, so a worker that +/// reserved only for itself would see "it fits", its sibling would independently see the same, and +/// together they would not fit — which is precisely the ENOSPC this preflight exists to prevent. +/// Computed in `u128` and clamped, NOT with `saturating_mul`: saturating first and then dividing by +/// 100 quietly turns an overflow into a number ~100x too small, which is the one direction that +/// matters here — an under-estimate authorises the very write the preflight exists to refuse. +fn required_free_bytes(media_bytes: u64, armed: i64) -> u64 { + let needed = media_bytes as u128 * EXPORT_SIZE_OVERHEAD_PCT as u128 / 100 + * armed.max(1).min(i64::from(u32::MAX)) as u128; + needed.min(u64::MAX as u128) as u64 +} + +/// Refuse to start an export that cannot fit, with a reason the host can act on. +/// +/// Without this the failure mode is ENOSPC halfway through a multi-GB write, and the wreckage +/// outlives it: the epoch has already moved, so the job row is `failed` at the CURRENT generation +/// and `GET /export/zip` 404s, while the last good archive sits on disk unreferenced. The host's +/// only escape (`POST /host/export/rebuild`) needs the very space that isn't there. Failing at the +/// door instead leaves the disk untouched and puts a number in front of the operator. +/// +/// Both halves are spawned concurrently and both are gallery-sized, so a worker must reserve for +/// its live sibling too — otherwise each independently sees "it fits", and together they don't. +async fn ensure_export_space(pool: &PgPool, event_id: Uuid, export_path: &Path) -> Result<()> { + let media_bytes = estimate_export_bytes(pool, event_id).await?; + + // Every job armed at any epoch for this event that hasn't finished is competing for this disk. + let (armed,): (i64,) = sqlx::query_as( + "SELECT COUNT(*) FROM export_job + WHERE event_id = $1 AND status IN ('pending', 'running')", + ) + .bind(event_id) + .fetch_one(pool) + .await + .context("counting armed export jobs")?; + let needed = required_free_bytes(media_bytes, armed); + + // `None` = the mount couldn't be resolved. Fail OPEN, exactly as the upload quota does: refusing + // to build the keepsake because we can't read a number would be a worse failure than trying. + let Some(free) = crate::services::disk::free_bytes(export_path) else { + tracing::warn!("export preflight: disk snapshot unavailable; proceeding without the check"); + return Ok(()); + }; + + if free < needed { + let gb = |b: u64| b as f64 / 1_000_000_000.0; + tracing::error!( + needed, + free, + armed, + "export preflight: not enough free space to build the keepsake for event {event_id}" + ); + anyhow::bail!( + "Nicht genug Speicherplatz für das Keepsake: benötigt ca. {:.1} GB, frei sind {:.1} GB. \ + Bitte Speicher freigeben und das Keepsake anschließend neu erstellen.", + gb(needed), + gb(free) + ); + } + Ok(()) +} + /// Best-effort removal of stale per-generation export artifacts for one export type. Deletes /// ONLY strictly-older generations (`n < keep_seq`) — never `keep_seq`'s own current file, /// and never a NEWER generation that a concurrent re-release may already be producing (that @@ -1177,20 +1367,7 @@ async fn prune_stale_export_files( event_id: Uuid, keep_seq: i64, ) { - // Files still referenced by a live (current-epoch) job row are OFF LIMITS regardless of the - // epoch in their name. A ViewerOnly regeneration carries the finished ZIP forward by re-stamping - // its row to the new epoch WITHOUT renaming the file — so `Gallery...zip` is still - // the served archive, and deleting it by filename-epoch would 404 the download. - let protected: Vec = sqlx::query_scalar::<_, String>( - "SELECT split_part(j.file_path, '/', -1) FROM export_job j - JOIN event e ON e.id = j.event_id - WHERE j.event_id = $1 AND j.epoch = e.export_epoch - AND j.status = 'done' AND j.file_path IS NOT NULL", - ) - .bind(event_id) - .fetch_all(pool) - .await - .unwrap_or_default(); + let protected = protected_files(pool, event_id).await; // EVERY shape is event-scoped. All events share one exports volume, so a name keyed only by // generation would let event A's prune delete event B's live keepsake (and let two events @@ -1504,3 +1681,131 @@ So geht's:\n\ Alles ist lokal auf deinem Gerät gespeichert.\n\ \n\ Viel Freude mit den Erinnerungen!\n"; + +// ── Tests ──────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + const EVT: &str = "11111111-1111-1111-1111-111111111111"; + + fn gallery_prefix() -> String { + format!("Gallery.{EVT}.") + } + + #[test] + fn a_strictly_older_archive_is_reclaimed() { + // The whole point: at the start of a rebuild at epoch 5, generation 4's archive is dead + // weight — readiness is derived from `epoch = event.export_epoch`, so it is already + // unreachable — and its bytes are very often exactly the bytes the rebuild needs. + assert!(is_superseded_archive( + &format!("Gallery.{EVT}.4.zip"), + &gallery_prefix(), + 5, + &[] + )); + } + + #[test] + fn our_own_and_newer_generations_are_never_touched() { + // `keep_seq` is OUR generation; a NEWER one belongs to a re-release that has already + // superseded us, and deleting it would let a lagging worker nuke a live keepsake. + for seq in [5, 6] { + assert!( + !is_superseded_archive( + &format!("Gallery.{EVT}.{seq}.zip"), + &gallery_prefix(), + 5, + &[] + ), + "generation {seq} must survive a prune keeping 5" + ); + } + } + + #[test] + fn a_carried_forward_archive_survives_despite_an_older_epoch_in_its_name() { + // THE dangerous case. A ViewerOnly regeneration (a moderated comment) re-stamps the + // finished ZIP's row to the new epoch WITHOUT renaming the file, so the SERVED archive + // legitimately carries an older generation number. Pruning it by filename-epoch would 404 + // the photo download to change nothing in it. The protected set is what stops that, and + // moving the prune to build-start makes this case reachable far more often. + let carried = format!("Gallery.{EVT}.4.zip"); + assert!(!is_superseded_archive( + &carried, + &gallery_prefix(), + 5, + std::slice::from_ref(&carried) + )); + } + + #[test] + fn temps_and_staging_dirs_are_out_of_scope_for_the_early_prune() { + // A superseded worker may still be streaming into these, and at build START it is much + // more likely to be alive than at finalize time. Only inert FINAL archives are reclaimed + // here; `prune_stale_export_files` still handles the rest after we win. + for name in [ + format!("Gallery.{EVT}.4.tmp"), + format!("viewer_tmp_{EVT}_4"), + format!("Memories.{EVT}.4.zip"), + ] { + assert!( + !is_superseded_archive(&name, &gallery_prefix(), 5, &[]), + "{name} must not be reclaimed by the pre-build prune" + ); + } + } + + #[test] + fn another_events_archive_is_never_reclaimed() { + // All events share one exports volume, so the prefix carries the event id. + let other = "22222222-2222-2222-2222-222222222222"; + assert!(!is_superseded_archive( + &format!("Gallery.{other}.4.zip"), + &gallery_prefix(), + 5, + &[] + )); + } + + #[test] + fn unrelated_files_are_left_alone() { + for name in ["Gallery.zip", "notes.txt", "Gallery..4.zip"] { + assert!(!is_superseded_archive(name, &gallery_prefix(), 5, &[])); + } + } + + #[test] + fn a_lone_armed_job_reserves_for_one_archive() { + // A ViewerOnly regeneration re-arms only the HTML half — reserving for two would refuse + // a rebuild that fits perfectly well. + assert_eq!(required_free_bytes(1_000, 1), 1_100); + } + + #[test] + fn two_concurrent_halves_reserve_for_both() { + // The bug this exists to prevent: each worker independently sees "it fits", and together + // they don't. Both halves are gallery-sized, so the reservation must be for the pair. + assert_eq!(required_free_bytes(1_000, 2), 2_200); + } + + #[test] + fn a_zero_count_still_reserves_for_one() { + // Defensive: a racing status transition must never yield a zero requirement, which would + // wave through an export of any size onto a full disk. + assert_eq!(required_free_bytes(1_000, 0), 1_100); + } + + #[test] + fn an_empty_gallery_needs_nothing() { + assert_eq!(required_free_bytes(0, 2), 0); + } + + #[test] + fn a_pathological_size_saturates_instead_of_wrapping() { + // u64 overflow would wrap to a TINY requirement and authorise the exact write we're + // guarding against — the failure mode must be "refuse", never "wrap and allow". + assert_eq!(required_free_bytes(u64::MAX, 2), u64::MAX); + } +} diff --git a/backend/tests/common/mod.rs b/backend/tests/common/mod.rs index c0a0a6b..9420b23 100644 --- a/backend/tests/common/mod.rs +++ b/backend/tests/common/mod.rs @@ -255,3 +255,85 @@ pub async fn downloadable(pool: &PgPool, event_id: Uuid, export_type: &str) -> O .expect("downloadable") .flatten() } + +/// Insert an upload of `size` bytes, optionally already soft-deleted. +pub async fn seed_upload( + pool: &PgPool, + event_id: Uuid, + user_id: Uuid, + size: i64, + deleted: bool, +) -> Uuid { + sqlx::query_scalar( + "INSERT INTO upload (event_id, user_id, original_path, mime_type, + original_size_bytes, deleted_at) + VALUES ($1, $2, 'originals/x.jpg', 'image/jpeg', $3, + CASE WHEN $4 THEN NOW() ELSE NULL END) + RETURNING id", + ) + .bind(event_id) + .bind(user_id) + .bind(size) + .bind(deleted) + .fetch_one(pool) + .await + .expect("seed upload") +} + +/// Flip the moderation flags a ban sets. +pub async fn set_user_moderation(pool: &PgPool, user_id: Uuid, banned: bool, hidden: bool) { + sqlx::query("UPDATE \"user\" SET is_banned = $2, uploads_hidden = $3 WHERE id = $1") + .bind(user_id) + .bind(banned) + .bind(hidden) + .execute(pool) + .await + .expect("set moderation"); +} + +/// SRC: `services/export.rs::query_uploads` — the visibility filter, verbatim, projected down to +/// `(id, original_size_bytes)`. This is the row set that ACTUALLY lands in the archives. +pub async fn export_visible_uploads(pool: &PgPool, event_id: Uuid) -> Vec<(Uuid, i64)> { + sqlx::query_as( + "SELECT u.id, u.original_size_bytes + FROM upload u + JOIN \"user\" usr ON usr.id = u.user_id + WHERE u.event_id = $1 AND u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE + GROUP BY u.id, usr.display_name + ORDER BY u.created_at ASC", + ) + .bind(event_id) + .fetch_all(pool) + .await + .expect("export_visible_uploads") +} + +/// SRC: `services/export.rs::estimate_export_bytes` — verbatim. +pub async fn estimate_export_bytes(pool: &PgPool, event_id: Uuid) -> i64 { + let (bytes,): (i64,) = sqlx::query_as( + "SELECT COALESCE(SUM(u.original_size_bytes), 0)::bigint + FROM upload u + JOIN \"user\" usr ON usr.id = u.user_id + WHERE u.event_id = $1 AND u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE", + ) + .bind(event_id) + .fetch_one(pool) + .await + .expect("estimate_export_bytes"); + bytes +} + +/// SRC: `services/export.rs::ensure_export_space` — the armed-job count, verbatim. +pub async fn armed_job_count(pool: &PgPool, event_id: Uuid) -> i64 { + let (n,): (i64,) = sqlx::query_as( + "SELECT COUNT(*) FROM export_job + WHERE event_id = $1 AND status IN ('pending', 'running')", + ) + .bind(event_id) + .fetch_one(pool) + .await + .expect("armed_job_count"); + n +} diff --git a/backend/tests/export_preflight.rs b/backend/tests/export_preflight.rs new file mode 100644 index 0000000..4f556fc --- /dev/null +++ b/backend/tests/export_preflight.rs @@ -0,0 +1,154 @@ +//! DB-backed tests for the export disk preflight. +//! +//! The keepsake used to be built with NO free-space check at all, and the failure that produced was +//! not "the export failed" but "the deliverable is stuck and the escape hatch needs the space that +//! isn't there": +//! +//! 1. A takedown bumps the epoch and re-arms both halves. +//! 2. The ZIP hits ENOSPC partway through a multi-GB write. +//! 3. The job row is now `failed` at the CURRENT epoch, so readiness +//! (`epoch = event.export_epoch AND status = 'done'`) is false and `GET /export/zip` 404s — +//! while the last good archive sits on disk, unreferenced and unreachable. +//! 4. `POST /host/export/rebuild` re-arms the same doomed write. +//! +//! Two changes close it: reclaim the superseded generation BEFORE building (so peak usage is one +//! generation, not two) and refuse up front with a number the host can act on. +//! +//! What these tests pin is the ESTIMATE — the part that decides. The arithmetic on top of it lives +//! in `services/export.rs`'s unit tests; the filesystem selection lives in `is_superseded_archive`. +//! The risk here is drift: if `query_uploads` ever gains or loses a visibility predicate and +//! `estimate_export_bytes` doesn't, the preflight silently sizes the wrong gallery. So rather than +//! restating the filter, these assert the estimate against the row set the archive actually +//! contains. + +mod common; + +use common::*; +use sqlx::PgPool; + +/// The estimate must equal the sum over EXACTLY the rows `query_uploads` returns — computed from +/// that row set, not from a restatement of its WHERE clause. +/// +/// PREVENTS: the two queries drifting apart. An estimate that counts rows the archive skips is +/// merely pessimistic; one that MISSES rows the archive writes under-reserves, which is the whole +/// failure being guarded against. +#[sqlx::test] +async fn the_estimate_sums_exactly_the_rows_the_archive_will_contain(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + + let visible = seed_user(&pool, event_id, "Anna").await; + let banned = seed_user(&pool, event_id, "Ben").await; + let hidden = seed_user(&pool, event_id, "Cara").await; + + seed_upload(&pool, event_id, visible, 1_000, false).await; + seed_upload(&pool, event_id, visible, 2_500, false).await; + // Each of these is excluded from the archive by a DIFFERENT predicate. + seed_upload(&pool, event_id, visible, 9_000, true).await; // soft-deleted + seed_upload(&pool, event_id, banned, 9_000, false).await; // uploader banned + seed_upload(&pool, event_id, hidden, 9_000, false).await; // uploads hidden + + set_user_moderation(&pool, banned, true, true).await; + set_user_moderation(&pool, hidden, false, true).await; + + let rows = export_visible_uploads(&pool, event_id).await; + let expected: i64 = rows.iter().map(|(_, bytes)| bytes).sum(); + + assert_eq!(rows.len(), 2, "only Anna's two live uploads are archived"); + assert_eq!( + estimate_export_bytes(&pool, event_id).await, + expected, + "the preflight must size the gallery the export will actually write" + ); + assert_eq!(expected, 3_500); +} + +/// An event with nothing to archive estimates zero rather than NULL. +/// +/// PREVENTS: `SUM()` over no rows returning NULL and the decode blowing up — which would abort the +/// export with a type error instead of building an (entirely legitimate) empty keepsake. +#[sqlx::test] +async fn an_empty_gallery_estimates_zero_not_null(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + assert_eq!(estimate_export_bytes(&pool, event_id).await, 0); + + // And with a user who has uploaded nothing. + seed_user(&pool, event_id, "Anna").await; + assert_eq!(estimate_export_bytes(&pool, event_id).await, 0); +} + +/// A release arms both halves, so the preflight sees a count of 2 and reserves for the pair. +/// +/// PREVENTS: the concurrency under-reservation. `spawn_export_jobs` starts the ZIP and HTML workers +/// at the same instant, and BOTH are gallery-sized (`Memories.zip` streams the original for every +/// video and every image at or under 5 MB, all `Compression::Stored`). A worker reserving only for +/// itself would see "it fits", its sibling would independently see the same, and together they +/// would ENOSPC — which is why `required_free_bytes` multiplies by this count. +#[sqlx::test] +async fn a_release_arms_both_halves_so_the_preflight_reserves_for_two(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user = seed_user(&pool, event_id, "Anna").await; + seed_upload(&pool, event_id, user, 1_000, false).await; + + assert_eq!( + armed_job_count(&pool, event_id).await, + 0, + "nothing is armed before the release" + ); + + let epoch = release_gallery(&pool, "wedding").await.expect("released"); + assert_eq!( + armed_job_count(&pool, event_id).await, + 2, + "a release arms zip AND html — both compete for the same disk" + ); + + // A worker that has claimed its half is still competing; `running` must keep counting. + assert!(claim_job(&pool, event_id, "zip", epoch).await); + assert_eq!( + armed_job_count(&pool, event_id).await, + 2, + "claiming moves pending -> running, which must not drop out of the reservation" + ); + + // Only a FINISHED half stops competing. + assert!(finalize_job(&pool, event_id, "zip", epoch, "exports/Gallery.zip").await); + assert_eq!( + armed_job_count(&pool, event_id).await, + 1, + "a done half no longer needs space reserved for it" + ); +} + +/// A ViewerOnly regeneration re-arms only the HTML half, so the preflight reserves for one. +/// +/// PREVENTS: over-reservation refusing a rebuild that fits perfectly well. Moderating a comment +/// carries the finished ZIP forward untouched; demanding room for a second copy of it would fail +/// the one operation that needs no new gallery-sized write at all. +#[sqlx::test] +async fn a_viewer_only_regeneration_reserves_for_one_half(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user = seed_user(&pool, event_id, "Anna").await; + seed_upload(&pool, event_id, user, 1_000, false).await; + + let epoch = release_gallery(&pool, "wedding").await.expect("released"); + for t in ["zip", "html"] { + assert!(claim_job(&pool, event_id, t, epoch).await); + assert!(finalize_job(&pool, event_id, t, epoch, &format!("exports/{t}")).await); + } + assert_eq!(armed_job_count(&pool, event_id).await, 0); + + // A moderated comment: bump the epoch, carry the ZIP forward, re-arm only the viewer. + let (_, _, next) = bump_epoch(&pool, "wedding").await.expect("bumped"); + assert!( + carry_zip_forward(&pool, event_id, next).await, + "the finished ZIP is re-stamped, not rebuilt" + ); + let mut conn = pool.acquire().await.expect("acquire"); + enqueue_types_at_epoch(&mut conn, event_id, next, &["html"]).await; + + assert_eq!( + armed_job_count(&pool, event_id).await, + 1, + "only the viewer is being rebuilt, so only one archive's worth of space is needed" + ); +} diff --git a/e2e/fixtures/db.ts b/e2e/fixtures/db.ts index cdbe6e2..7db267d 100644 --- a/e2e/fixtures/db.ts +++ b/e2e/fixtures/db.ts @@ -142,7 +142,8 @@ export const db = { async fakeExportJob( eventSlug: string, type: 'zip' | 'html', - status: 'pending' | 'running' | 'done' + status: 'pending' | 'running' | 'done' | 'failed', + errorMessage: string | null = null ) { await withClient(async (c) => { const ev = await c.query<{ id: string; export_epoch: string }>( @@ -151,11 +152,12 @@ export const db = { ); if (ev.rows.length === 0) throw new Error(`No event with slug ${eventSlug}`); await c.query( - `INSERT INTO export_job (event_id, type, status, progress_pct, completed_at, epoch) - VALUES ($1, $2::export_type, $3::export_status, $4, $5, $6) + `INSERT INTO export_job (event_id, type, status, progress_pct, completed_at, epoch, + error_message) + VALUES ($1, $2::export_type, $3::export_status, $4, $5, $6, $7) ON CONFLICT (event_id, type) DO UPDATE SET status = EXCLUDED.status, progress_pct = EXCLUDED.progress_pct, - epoch = EXCLUDED.epoch`, + epoch = EXCLUDED.epoch, error_message = EXCLUDED.error_message`, [ ev.rows[0].id, type, @@ -163,6 +165,7 @@ export const db = { status === 'done' ? 100 : 0, status === 'done' ? new Date() : null, ev.rows[0].export_epoch, + errorMessage, ] ); }); diff --git a/e2e/specs/06-export/failure-reason-visible.spec.ts b/e2e/specs/06-export/failure-reason-visible.spec.ts new file mode 100644 index 0000000..72c8535 --- /dev/null +++ b/e2e/specs/06-export/failure-reason-visible.spec.ts @@ -0,0 +1,95 @@ +/** + * Regression guard — when the keepsake fails to build, the HOST must be told why. + * + * `/export/status` reported `{status, progress_pct}` and nothing else, so the host dashboard could + * only ever render "Keepsake-Erstellung fehlgeschlagen." next to an "Erneut versuchen" button. The + * reason WAS being written — `mark_failed` stores it on the job row — but it surfaced solely in the + * admin dashboard's job list. The host is the person who releases the gallery, owns the retry + * button, and is standing at the venue; the admin may be someone else entirely, or the same person + * without the password to hand. + * + * That matters most for the failure this shipped alongside: the export disk preflight. Its message + * names the two numbers that decide what to do ("benötigt ca. X GB, frei sind Y GB"), and without + * it "Erneut versuchen" fails identically, forever, with no hint that the answer is free some space. + * + * These drive the real UI and the real endpoint — the plumbing is four hops (SQL → handler JSON → + * store type → Svelte branch) and any one of them dropping the field restores the silent version. + */ +import { test, expect } from '../../fixtures/test'; +import { BASE } from '../../helpers/env'; + +const SLUG = 'e2e-test-event'; +const DISK_REASON = + 'Nicht genug Speicherplatz für das Keepsake: benötigt ca. 42.0 GB, frei sind 3.0 GB. ' + + 'Bitte Speicher freigeben und das Keepsake anschließend neu erstellen.'; + +test.describe('Export — a failed keepsake explains itself to the host', () => { + test('the failure reason reaches /export/status', async ({ host, db }) => { + await db.setExportReleased(SLUG, true); + await db.fakeExportJob(SLUG, 'zip', 'failed', DISK_REASON); + await db.fakeExportJob(SLUG, 'html', 'failed', DISK_REASON); + + const res = await fetch(`${BASE}/api/v1/export/status`, { + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + expect(res.status).toBe(200); + const body = (await res.json()) as { + zip: { status: string; error_message: string | null }; + html: { status: string; error_message: string | null }; + }; + + expect(body.zip.status).toBe('failed'); + expect( + body.zip.error_message, + 'the reason must travel with the status, not live only in the admin job list' + ).toBe(DISK_REASON); + expect(body.html.error_message).toBe(DISK_REASON); + }); + + test('a succeeding export carries no stale reason', async ({ host, db }) => { + // The mirror that keeps the above honest: a handler that returned `error_message` + // unconditionally would pass the first test while showing an error next to a green + // "Keepsake ist bereit." Rows keep their last message until they are re-armed, so this is a + // real state, not a hypothetical one. + await db.setExportReleased(SLUG, true); + await db.fakeExportJob(SLUG, 'zip', 'failed', DISK_REASON); + await db.fakeExportJob(SLUG, 'zip', 'done', DISK_REASON); + + const res = await fetch(`${BASE}/api/v1/export/status`, { + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + const body = (await res.json()) as { zip: { status: string; error_message: string | null } }; + expect(body.zip.status).toBe('done'); + expect( + body.zip.error_message, + 'a message left on a row that has since succeeded must not be shown' + ).toBeNull(); + }); + + test('the host dashboard renders the reason under the failure', async ({ + page, + host, + signIn, + db, + }) => { + await db.setExportReleased(SLUG, true); + await db.fakeExportJob(SLUG, 'zip', 'failed', DISK_REASON); + await db.fakeExportJob(SLUG, 'html', 'failed', DISK_REASON); + + await signIn(page, host); + await page.goto('/host'); + + await expect(page.getByText(/Keepsake-Erstellung fehlgeschlagen/i)).toBeVisible({ + timeout: 15_000, + }); + // The actionable half — the numbers, not just the verdict. + await expect( + page.getByText(/Nicht genug Speicherplatz/i), + 'the host must see WHY, next to the only button they have' + ).toBeVisible(); + await expect(page.getByText(/3\.0 GB/)).toBeVisible(); + + // And the retry button is still mounted — it is deliberately outside the status branches. + await expect(page.getByTestId('export-rebuild')).toBeVisible(); + }); +}); diff --git a/frontend/src/lib/export-status-store.ts b/frontend/src/lib/export-status-store.ts index d9e3d2d..0d9965b 100644 --- a/frontend/src/lib/export-status-store.ts +++ b/frontend/src/lib/export-status-store.ts @@ -11,6 +11,8 @@ import { getToken, onClearAuth } from './auth'; export interface ExportJob { status: 'locked' | 'pending' | 'running' | 'done' | 'failed'; progress_pct: number; + /** Only populated when `status === 'failed'` — the reason, phrased for the host. */ + error_message: string | null; } export interface ExportStatusSnapshot { diff --git a/frontend/src/routes/host/+page.svelte b/frontend/src/routes/host/+page.svelte index 8212c5f..f485927 100644 --- a/frontend/src/routes/host/+page.svelte +++ b/frontend/src/routes/host/+page.svelte @@ -40,6 +40,7 @@ interface ExportJob { status: string; progress_pct: number; + error_message: string | null; } interface ExportStatusDto { released: boolean; @@ -71,6 +72,11 @@ let exportProgress = $derived( Math.min(exportInfo?.zip?.progress_pct ?? 0, exportInfo?.html?.progress_pct ?? 0) ); + // Either half can carry the reason, and a disk failure usually fails both with the same text — + // so take the first one present rather than rendering it twice. + let exportError = $derived( + exportInfo?.zip?.error_message ?? exportInfo?.html?.error_message ?? null + ); // SSE unsubscribers, torn down on destroy. let sseOff: Array<() => void> = []; @@ -686,6 +692,13 @@

{:else}

Keepsake-Erstellung fehlgeschlagen.

+ + {#if exportError} +

{exportError}

+ {/if} {/if}