diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 529bdc1..24af3d1 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -88,7 +88,8 @@ impl CompressionWorker { Ok(v) => break Ok(v), Err(e) if attempt < Self::MAX_PROCESS_ATTEMPTS - && !crate::services::imaging::is_permanent_image_error(&e) => + && !crate::services::imaging::is_permanent_image_error(&e) + && !crate::services::imaging::is_storage_full_error(&e) => { tracing::warn!( error = ?e, %upload_id, attempt, @@ -113,6 +114,34 @@ impl CompressionWorker { data: serde_json::json!({ "upload_id": upload_id }).to_string(), }); } + Err(e) if crate::services::imaging::is_storage_full_error(&e) => { + // Out of disk. Keep the row AND the original — the opposite of the branch + // below, and for the same reason it retains the file: nothing here is the + // guest's fault and nothing about the photo is wrong. + // + // Soft-deleting on ENOSPC was strictly harmful. It refunded the quota while + // keeping the bytes, so it freed nothing, removed the photo from the feed + // seconds after a `201 Created`, and handed the guest the allowance to + // upload it again into the same full disk. Leaving the row live costs + // nothing instead: every client already falls back to the original when + // `preview_url` and `thumbnail_url` are NULL, so the photo stays visible — + // just uncompressed — and `backfill_stale_derivatives` regenerates the + // derivatives on the next start, once there is room for them. + tracing::error!( + %upload_id, + "compression failed: the media filesystem is out of space. The upload is \ + kept and served from its original; free disk space and restart to \ + regenerate derivatives: {e:#}" + ); + let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; + // Not an "error" event: nothing was lost and there is nothing for the guest + // to act on. Clients treat this purely as "refetch me", which is what makes + // the card appear with its original as the image source. + let _ = worker.sse_tx.send(SseEvent { + event_type: "upload-processed".to_string(), + data: serde_json::json!({ "upload_id": upload_id }).to_string(), + }); + } Err(e) => { tracing::error!( "compression failed for upload {upload_id} after {attempt} attempt(s): {e:#}" @@ -180,17 +209,39 @@ impl CompressionWorker { // this non-fatal would have been far worse than the bug. Every clip of a second or less // would fail compression, exhaust its retries and be soft-deleted — a cosmetic defect // turned into data loss, on exactly the mis-tap/Live-Photo clips guests produce most. - match self.generate_video_thumbnail(upload_id, &original).await? { - Some(thumb_rel) => { - Upload::set_thumbnail_path(&self.pool, upload_id, &thumb_rel).await?; - tracing::info!("thumbnail generated for upload {upload_id}"); + // Handling only the `Ok(None)` arm was not enough: the `?` on the call itself still + // routed every OTHER poster failure into the give-up path. `extract_poster_frame` + // returns `Err` when ffmpeg is missing from the image, when it hangs on a truncated + // `.mov` and trips FFMPEG_TIMEOUT, or when `thumbnails/` can't be created — and + // `set_thumbnail_path` returns `Err` on any DB blip. None of those say anything about + // the video itself, yet each one destroyed it. Confirmed live: on a box with no ffmpeg + // the spawn error propagated, exhausted all three attempts and soft-deleted the clip. + // + // Nothing about a video post depends on the poster — `get_original` serves the file + // byte-for-byte and the tile falls back to the video element — so no failure in this + // branch may fail the upload. + match self.generate_video_thumbnail(upload_id, &original).await { + Ok(Some(thumb_rel)) => { + match Upload::set_thumbnail_path(&self.pool, upload_id, &thumb_rel).await { + Ok(()) => tracing::info!("thumbnail generated for upload {upload_id}"), + Err(e) => tracing::warn!( + error = ?e, %upload_id, + "poster extracted but could not be recorded; the video keeps its own tile" + ), + } } - None => { + Ok(None) => { tracing::warn!( %upload_id, "no poster frame could be extracted; the video keeps its own tile" ); } + Err(e) => { + tracing::warn!( + error = ?e, %upload_id, + "poster extraction failed; the video keeps its own tile" + ); + } } } diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index 0266a5c..3b29c3a 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -39,8 +39,10 @@ fn decode_limits() -> image::Limits { /// /// Deliberately narrow. Only the `ImageError` variants that are a property of the *input* /// count: the file will not shrink, gain codec support, or un-corrupt itself between -/// attempts. `IoError` is excluded on purpose — an ENOSPC while writing a derivative, or -/// EMFILE under load, is exactly the transient case the retry exists for. +/// attempts. `IoError` is excluded on purpose — EMFILE under load, or a momentarily +/// unreadable file, is exactly the transient case the retry exists for. A FULL disk is the +/// one io error that must not be retried either, but for a different reason and with a +/// different remedy; see [`is_storage_full_error`]. pub fn is_permanent_image_error(err: &anyhow::Error) -> bool { err.chain().any(|cause| { matches!( @@ -54,6 +56,34 @@ pub fn is_permanent_image_error(err: &anyhow::Error) -> bool { }) } +/// True when the failure is the media filesystem being out of space. +/// +/// Deliberately separate from [`is_permanent_image_error`], which is about the *input*. ENOSPC +/// is about the *host*, and it is the one failure the retry loop actively makes worse: a disk +/// does not drain during six seconds of backoff, so all three attempts fail identically while +/// holding a compression permit that photos are queued behind. +/// +/// The give-up path it fed was worse still. It refunded the guest's quota and soft-deleted the +/// row while deliberately RETAINING the original — so the bytes stayed on the full disk, the +/// photo vanished from the feed seconds after a `201 Created`, and the guest was handed back +/// the quota to upload it again into the same full disk. Each round shrank free space further. +pub fn is_storage_full_error(err: &anyhow::Error) -> bool { + fn is_full(io: &std::io::Error) -> bool { + // `StorageFull` is the portable classification; the raw ENOSPC catches the paths where + // the OS error was never mapped to a named kind. + io.kind() == std::io::ErrorKind::StorageFull || io.raw_os_error() == Some(28) + } + err.chain().any(|cause| { + // `image` wraps the io error in its own variant rather than exposing it as a source, + // so the plain downcast alone would miss every derivative-write failure. + cause.downcast_ref::().is_some_and(is_full) + || matches!( + cause.downcast_ref::(), + Some(image::ImageError::IoError(io)) if is_full(io) + ) + }) +} + /// Build a decoder for `path` with the budget enforced, WITHOUT reading any pixels. /// /// Single source of truth for "may this image be decoded at all": both the upload @@ -189,9 +219,10 @@ mod tests { #[test] fn a_plain_io_error_is_not_permanent() { - // The mirror that keeps the classifier honest. ENOSPC while writing a derivative, or - // EMFILE under load, is exactly what the retry exists for — misclassifying those as + // The mirror that keeps the classifier honest. EMFILE under load, or a momentary + // unreadable file, is exactly what the retry exists for — misclassifying those as // permanent would turn a transient blip back into the data loss round 1 fixed. + // (A FULL disk is its own case now; see the storage-full tests below.) let err = decode_oriented(Path::new("/nonexistent/definitely-not-here.jpg")) .map(|img| (img.width(), img.height())) .expect_err("a missing file must error"); @@ -201,6 +232,40 @@ mod tests { ); } + #[test] + fn a_full_disk_is_recognised_through_both_wrappers() { + // The two shapes ENOSPC actually arrives in. A bare io::Error is what `tokio::fs` and + // `std::fs` produce; the `image` crate wraps its own in `ImageError::IoError`, which is + // NOT reachable via `source()` — so a chain walk that only downcast to io::Error would + // miss every derivative-write failure, i.e. the exact case this classifier exists for. + let bare = anyhow::Error::from(std::io::Error::from(std::io::ErrorKind::StorageFull)) + .context("failed to write the preview"); + assert!(is_storage_full_error(&bare), "bare io::Error: {bare:#}"); + + let wrapped = anyhow::Error::from(image::ImageError::IoError(std::io::Error::from( + std::io::ErrorKind::StorageFull, + ))) + .context("failed to save the display derivative"); + assert!( + is_storage_full_error(&wrapped), + "ImageError::IoError: {wrapped:#}" + ); + } + + #[test] + fn an_ordinary_io_error_is_not_a_full_disk() { + // Keeps the classifier from swallowing the general case: only ENOSPC may skip the retry + // and take the keep-the-row branch. Anything else must still be retried and, if it keeps + // failing, soft-deleted as before. + let missing = decode_oriented(Path::new("/nonexistent/definitely-not-here.jpg")) + .map(|img| (img.width(), img.height())) + .expect_err("a missing file must error"); + assert!( + !is_storage_full_error(&missing), + "a missing file is not a full disk: {missing:#}" + ); + } + #[test] fn admission_rejects_only_the_over_budget_case() { // Admission and processing must agree about SIZE — a photo accepted at the door and diff --git a/backend/src/services/video.rs b/backend/src/services/video.rs index d22ee22..968121e 100644 --- a/backend/src/services/video.rs +++ b/backend/src/services/video.rs @@ -26,7 +26,12 @@ use anyhow::{Context, Result}; /// the semaphore permit and the pool eventually deadlocks; in the export worker it strands the job /// at `running` so the keepsake never completes. `export.rs` had NO timeout at all before this /// module — sharing the spawn fixes that too. -const FFMPEG_TIMEOUT: Duration = Duration::from_secs(120); +/// 45s, not the 120s this started at. The timeout is not a budget for honest work — a poster +/// frame from a phone clip takes well under a second, and `-ss` before `-i` means even a 500 MB +/// file seeks rather than scans. It is purely the ceiling on how long a pathological input may +/// hold a compression permit that guests' photos are queued behind, so it should be as tight as +/// it can be without ever cutting off real work. +const FFMPEG_TIMEOUT: Duration = Duration::from_secs(45); /// Seek positions to try, in order. /// @@ -107,6 +112,22 @@ async fn run_ffmpeg(src: &Path, dest: &Path, width: u32, seek: &str) -> Result<( mod tests { use super::*; + /// Is there a usable `ffmpeg` on PATH? + /// + /// The poster-frame path shells out, and `extract_poster_frame` documents `Err` as meaning + /// "a hang or a SPAWN failure" — which is exactly what a missing binary produces. So on a + /// machine without ffmpeg the test below stops exercising the case it names (missing INPUT) + /// and instead reports a code defect that isn't there. The runtime image installs ffmpeg + /// (see backend/Dockerfile), so this only ever skips on a bare developer machine. + fn ffmpeg_available() -> bool { + std::process::Command::new("ffmpeg") + .arg("-version") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .is_ok() + } + /// The order is the whole fix. `-ss` must precede `-i`, and 0 must be tried after 1 s. #[test] fn the_fallback_seek_exists_and_comes_last() { @@ -121,6 +142,15 @@ mod tests { /// poster", never fail the upload. `Err` is reserved for a hang or a spawn failure. #[tokio::test] async fn a_missing_source_yields_no_frame_rather_than_an_error() { + if !ffmpeg_available() { + eprintln!( + "SKIP a_missing_source_yields_no_frame_rather_than_an_error: no ffmpeg on PATH. \ + A missing binary is a spawn failure, which this function returns Err for by \ + design, so the missing-INPUT case cannot be exercised here. Install ffmpeg to \ + run it (the runtime image already has it)." + ); + return; + } let dir = std::env::temp_dir().join(format!("es-video-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let dest = dir.join("out.jpg");