fix(media): stop a poster-frame failure from deleting the guest's video
Reproduced live, by accident, while smoke-testing on a machine with no ffmpeg: the clip uploaded fine, returned 201, and roughly six seconds later had `deleted_at` set and was gone from the feed. The `Ok(None)` "this clip yields no frame" case was already handled — that fix landed when sub-second clips were being destroyed. But the `?` on the call itself still routed every OTHER failure into the same give-up path, which soft-deletes: ffmpeg missing from the image, ffmpeg hanging on a truncated `.mov` and tripping the timeout, an ENOSPC on `thumbnails/`, or a DB blip in `set_thumbnail_path`. None of those says anything about the video, and `get_original` serves the file byte-for-byte, so a post that merely lacks a poster is fully watchable. No failure in the video branch may fail the upload. iPhone `.mov` is exactly the input most likely to trip it, and a wedding clip is not retakeable. ENOSPC gets its own classifier. It was the one failure the retry loop actively made worse: a disk does not drain during six seconds of backoff, so all three attempts failed identically while holding a compression permit that photos were queued behind — and the give-up path then refunded the quota and soft-deleted the row while deliberately KEEPING the original. That freed nothing, removed the photo seconds after a 201, and handed the guest the allowance to upload it again into the same full disk. Now: no retry, no refund, no delete. The row stays live and the photo is served from its original, and `backfill_stale_derivatives` regenerates the derivatives on the next start once there is room. `is_storage_full_error` has to look inside `ImageError::IoError` as well as at bare io errors, because `image` wraps rather than sources it and a plain chain walk would miss every derivative-write failure. FFMPEG_TIMEOUT drops 120s -> 45s. It was never a budget for honest work — a poster 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 the ceiling on how long a pathological input holds a permit that guests' photos are waiting behind, so it should be as tight as it can be without cutting off real work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -88,7 +88,8 @@ impl CompressionWorker {
|
|||||||
Ok(v) => break Ok(v),
|
Ok(v) => break Ok(v),
|
||||||
Err(e)
|
Err(e)
|
||||||
if attempt < Self::MAX_PROCESS_ATTEMPTS
|
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!(
|
tracing::warn!(
|
||||||
error = ?e, %upload_id, attempt,
|
error = ?e, %upload_id, attempt,
|
||||||
@@ -113,6 +114,34 @@ impl CompressionWorker {
|
|||||||
data: serde_json::json!({ "upload_id": upload_id }).to_string(),
|
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) => {
|
Err(e) => {
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
"compression failed for upload {upload_id} after {attempt} attempt(s): {e:#}"
|
"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
|
// 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
|
// 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.
|
// turned into data loss, on exactly the mis-tap/Live-Photo clips guests produce most.
|
||||||
match self.generate_video_thumbnail(upload_id, &original).await? {
|
// Handling only the `Ok(None)` arm was not enough: the `?` on the call itself still
|
||||||
Some(thumb_rel) => {
|
// routed every OTHER poster failure into the give-up path. `extract_poster_frame`
|
||||||
Upload::set_thumbnail_path(&self.pool, upload_id, &thumb_rel).await?;
|
// returns `Err` when ffmpeg is missing from the image, when it hangs on a truncated
|
||||||
tracing::info!("thumbnail generated for upload {upload_id}");
|
// `.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!(
|
tracing::warn!(
|
||||||
%upload_id,
|
%upload_id,
|
||||||
"no poster frame could be extracted; the video keeps its own tile"
|
"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"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -39,8 +39,10 @@ fn decode_limits() -> image::Limits {
|
|||||||
///
|
///
|
||||||
/// Deliberately narrow. Only the `ImageError` variants that are a property of the *input*
|
/// 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
|
/// 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
|
/// attempts. `IoError` is excluded on purpose — EMFILE under load, or a momentarily
|
||||||
/// EMFILE under load, is exactly the transient case the retry exists for.
|
/// 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 {
|
pub fn is_permanent_image_error(err: &anyhow::Error) -> bool {
|
||||||
err.chain().any(|cause| {
|
err.chain().any(|cause| {
|
||||||
matches!(
|
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::<std::io::Error>().is_some_and(is_full)
|
||||||
|
|| matches!(
|
||||||
|
cause.downcast_ref::<image::ImageError>(),
|
||||||
|
Some(image::ImageError::IoError(io)) if is_full(io)
|
||||||
|
)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
/// Build a decoder for `path` with the budget enforced, WITHOUT reading any pixels.
|
/// 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
|
/// Single source of truth for "may this image be decoded at all": both the upload
|
||||||
@@ -189,9 +219,10 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_plain_io_error_is_not_permanent() {
|
fn a_plain_io_error_is_not_permanent() {
|
||||||
// The mirror that keeps the classifier honest. ENOSPC while writing a derivative, or
|
// The mirror that keeps the classifier honest. EMFILE under load, or a momentary
|
||||||
// EMFILE under load, is exactly what the retry exists for — misclassifying those as
|
// 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.
|
// 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"))
|
let err = decode_oriented(Path::new("/nonexistent/definitely-not-here.jpg"))
|
||||||
.map(|img| (img.width(), img.height()))
|
.map(|img| (img.width(), img.height()))
|
||||||
.expect_err("a missing file must error");
|
.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]
|
#[test]
|
||||||
fn admission_rejects_only_the_over_budget_case() {
|
fn admission_rejects_only_the_over_budget_case() {
|
||||||
// Admission and processing must agree about SIZE — a photo accepted at the door and
|
// Admission and processing must agree about SIZE — a photo accepted at the door and
|
||||||
|
|||||||
@@ -26,7 +26,12 @@ use anyhow::{Context, Result};
|
|||||||
/// the semaphore permit and the pool eventually deadlocks; in the export worker it strands the job
|
/// 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
|
/// at `running` so the keepsake never completes. `export.rs` had NO timeout at all before this
|
||||||
/// module — sharing the spawn fixes that too.
|
/// 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.
|
/// 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 {
|
mod tests {
|
||||||
use super::*;
|
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.
|
/// The order is the whole fix. `-ss` must precede `-i`, and 0 must be tried after 1 s.
|
||||||
#[test]
|
#[test]
|
||||||
fn the_fallback_seek_exists_and_comes_last() {
|
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.
|
/// poster", never fail the upload. `Err` is reserved for a hang or a spawn failure.
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn a_missing_source_yields_no_frame_rather_than_an_error() {
|
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()));
|
let dir = std::env::temp_dir().join(format!("es-video-{}", std::process::id()));
|
||||||
std::fs::create_dir_all(&dir).unwrap();
|
std::fs::create_dir_all(&dir).unwrap();
|
||||||
let dest = dir.join("out.jpg");
|
let dest = dir.join("out.jpg");
|
||||||
|
|||||||
Reference in New Issue
Block a user