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>
329 lines
15 KiB
Rust
329 lines
15 KiB
Rust
//! Shared image decoding.
|
||
//!
|
||
//! Exists so there is exactly ONE way to turn a file on disk into a `DynamicImage` in this
|
||
//! codebase. Two properties have to hold everywhere an image is decoded, and both were
|
||
//! previously re-derived per call site — which is how they drifted apart:
|
||
//!
|
||
//! - **EXIF orientation must be applied.** Phones do not rotate sensor data; they record how
|
||
//! the camera was held in a tag and store the pixels as shot. `image::open` and
|
||
//! `ImageReader::decode` both hand back the raw pixels and ignore that tag, and re-encoding
|
||
//! to JPEG writes no EXIF, so the derivative is permanently sideways while the untouched
|
||
//! original still renders upright. The compression worker was fixed; the export worker was
|
||
//! not, so every portrait photo came out sideways in the keepsake's HTML viewer.
|
||
//! - **Decode limits must be set.** The upload body cap bounds the file on disk, but a small
|
||
//! file can decode to enormous dimensions (a ~1 MB image expanding to 50k×50k px), OOM-ing
|
||
//! the box. `image::open` applies NO limits at all, so the export path was also decoding
|
||
//! arbitrary user-supplied images unbounded.
|
||
|
||
use anyhow::{Context, Result};
|
||
use image::{DynamicImage, ImageDecoder};
|
||
use std::path::Path;
|
||
|
||
/// Bounds for any decode of user-supplied image data. The per-axis cap covers any real phone
|
||
/// photo; `max_alloc` bounds the decoded buffer — but only because `decode_oriented` reserves
|
||
/// against it explicitly, see there.
|
||
///
|
||
/// Sized against the deployment: the app container is capped at 1 GiB and the compression
|
||
/// worker runs `compression_concurrency` decodes at once (default 2), so 256 MiB per decode
|
||
/// leaves headroom for the resize buffers and the runtime.
|
||
fn decode_limits() -> image::Limits {
|
||
let mut limits = image::Limits::default();
|
||
limits.max_image_width = Some(12_000);
|
||
limits.max_image_height = Some(12_000);
|
||
limits.max_alloc = Some(256 * 1024 * 1024);
|
||
limits
|
||
}
|
||
|
||
/// True when re-running the exact same work on the exact same bytes cannot possibly
|
||
/// succeed, so retrying only burns wall-clock and log noise.
|
||
///
|
||
/// 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 — 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!(
|
||
cause.downcast_ref::<image::ImageError>(),
|
||
Some(
|
||
image::ImageError::Limits(_)
|
||
| image::ImageError::Unsupported(_)
|
||
| image::ImageError::Decoding(_)
|
||
)
|
||
)
|
||
})
|
||
}
|
||
|
||
/// 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.
|
||
///
|
||
/// Single source of truth for "may this image be decoded at all": both the upload
|
||
/// admission check and the compression worker go through here, so they cannot disagree
|
||
/// about what is acceptable.
|
||
fn decoder_within_budget(path: &Path) -> Result<impl image::ImageDecoder> {
|
||
let mut reader = image::ImageReader::open(path)
|
||
.context("failed to open image")?
|
||
.with_guessed_format()
|
||
.context("failed to read image header")?;
|
||
let mut limits = decode_limits();
|
||
reader.limits(limits.clone());
|
||
|
||
// We need `into_decoder` rather than `decode()` to read the EXIF orientation tag before
|
||
// the pixels are consumed. But the two are NOT equivalent on safety: `decode()` performs
|
||
//
|
||
// limits.reserve(decoder.total_bytes())?;
|
||
//
|
||
// between building the decoder and reading the image, and `into_decoder()` skips it (the
|
||
// crate's own FIXME concedes `from_decoder` doesn't compensate). Nothing else enforces
|
||
// `max_alloc` — the JPEG decoder's `set_limits` only checks support and dimensions — so
|
||
// without the line below the budget is inert and the ONLY bound is the per-axis cap. That
|
||
// leaves 12000x12000 decodable at 412 MiB, and two concurrent at 824 MiB against a 1 GiB
|
||
// container. Re-add it, exactly as `decode()` does.
|
||
let mut decoder = reader.into_decoder().context("failed to decode image")?;
|
||
limits
|
||
.reserve(decoder.total_bytes())
|
||
.context("image too large to decode within the memory budget")?;
|
||
decoder
|
||
.set_limits(limits)
|
||
.context("image too large to decode within the memory budget")?;
|
||
Ok(decoder)
|
||
}
|
||
|
||
/// Megapixels an image would decode to, or `None` if its header can't be read. Used only
|
||
/// to put a concrete number in the message the guest sees.
|
||
pub fn megapixels(path: &Path) -> Option<f64> {
|
||
let reader = image::ImageReader::open(path)
|
||
.ok()?
|
||
.with_guessed_format()
|
||
.ok()?;
|
||
let (w, h) = reader.into_dimensions().ok()?;
|
||
Some(f64::from(w) * f64::from(h) / 1_000_000.0)
|
||
}
|
||
|
||
/// True when an image cannot be decoded specifically because it would exceed the memory
|
||
/// budget — read from the header, no pixels touched.
|
||
///
|
||
/// Called at upload admission so a guest who sends a 100 MP photo is told at the door, with
|
||
/// a reason they can act on, instead of the upload being accepted with a 201 and then
|
||
/// silently soft-deleted minutes later when the worker gives up on it.
|
||
///
|
||
/// Deliberately narrow: ONLY the budget. A corrupt, truncated or unsupported file also
|
||
/// fails to build a decoder, but rejecting those here would change a contract the
|
||
/// adversarial suite pins on purpose — acceptance follows the magic bytes, and a payload
|
||
/// with a valid JPEG header is accepted regardless of what follows it. Those go to the
|
||
/// compression worker as before, which handles them gracefully and (since the retry
|
||
/// classifier) no longer burns backoff on them.
|
||
pub fn exceeds_decode_budget(path: &Path) -> bool {
|
||
match decoder_within_budget(path) {
|
||
Ok(_) => false,
|
||
Err(e) => e.chain().any(|cause| {
|
||
matches!(
|
||
cause.downcast_ref::<image::ImageError>(),
|
||
Some(image::ImageError::Limits(_))
|
||
)
|
||
}),
|
||
}
|
||
}
|
||
|
||
/// Decode an image from disk with decompression-bomb limits applied and its EXIF
|
||
/// orientation baked into the pixels.
|
||
///
|
||
/// Blocking — call inside `spawn_blocking`.
|
||
pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
|
||
let mut decoder = decoder_within_budget(path)?;
|
||
|
||
// Cheap, and it happens BEFORE any pixels are read: an oversized image costs a header
|
||
// parse, not an allocation.
|
||
let orientation = decoder
|
||
.orientation()
|
||
.unwrap_or(image::metadata::Orientation::NoTransforms);
|
||
let mut img = DynamicImage::from_decoder(decoder).context("failed to decode image")?;
|
||
img.apply_orientation(orientation);
|
||
Ok(img)
|
||
}
|
||
|
||
#[cfg(test)]
|
||
mod tests {
|
||
use super::*;
|
||
|
||
/// Shared with the e2e suite rather than duplicating 568 KiB of binary: the same file
|
||
/// drives `02-upload/oversized-image` so both layers assert on one artefact.
|
||
const HUGE: &str = concat!(
|
||
env!("CARGO_MANIFEST_DIR"),
|
||
"/../e2e/fixtures/media/huge-99mp.jpg"
|
||
);
|
||
|
||
#[test]
|
||
fn rejects_an_image_that_would_blow_the_allocation_budget() {
|
||
// 11000x9000 = 99 MP. Deliberately UNDER the 12000px per-axis cap, so the axis check
|
||
// cannot reject it — the allocation budget is the only thing that can, which is
|
||
// exactly what makes this a regression test rather than a restatement of the axis cap.
|
||
// 283 MiB decoded as RGB8 against a 256 MiB budget, from 568 KiB on disk.
|
||
//
|
||
// This failed before the guard was restored: `ImageReader::decode` performs
|
||
// `limits.reserve(decoder.total_bytes())`, and `into_decoder()` — which we need for
|
||
// the EXIF tag — skips it, so `max_alloc` was inert and this decoded happily.
|
||
// Map the Ok arm to its dimensions first: on failure `expect_err` Debug-prints the
|
||
// value, and Debug on a DynamicImage dumps every pixel — 283 MiB of output.
|
||
let err = decode_oriented(Path::new(HUGE))
|
||
.map(|img| (img.width(), img.height()))
|
||
.expect_err("a 99 MP image must be refused, not allocated");
|
||
let msg = format!("{err:#}");
|
||
assert!(
|
||
msg.to_lowercase().contains("limit") || msg.to_lowercase().contains("memory"),
|
||
"expected a limits error, got: {msg}"
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn an_oversized_image_is_a_permanent_failure() {
|
||
// The retry loop must not burn 2s + 4s of backoff on this: the file will not shrink
|
||
// between attempts, so all three attempts reach the identical conclusion.
|
||
let err = decode_oriented(Path::new(HUGE))
|
||
.map(|img| (img.width(), img.height()))
|
||
.expect_err("fixture must exceed the budget");
|
||
assert!(
|
||
is_permanent_image_error(&err),
|
||
"a Limits error can never succeed on retry: {err:#}"
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn a_plain_io_error_is_not_permanent() {
|
||
// 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");
|
||
assert!(
|
||
!is_permanent_image_error(&err),
|
||
"an IO error must stay retryable: {err:#}"
|
||
);
|
||
}
|
||
|
||
#[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
|
||
// then rejected by the worker for being too big is the failure this pair prevents.
|
||
assert!(
|
||
exceeds_decode_budget(Path::new(HUGE)),
|
||
"admission must reject what the decoder rejects for size"
|
||
);
|
||
let ordinary = concat!(
|
||
env!("CARGO_MANIFEST_DIR"),
|
||
"/../e2e/fixtures/media/portrait-exif6.jpg"
|
||
);
|
||
assert!(
|
||
!exceeds_decode_budget(Path::new(ordinary)),
|
||
"admission must accept an ordinary photo"
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn admission_does_not_reject_a_merely_undecodable_file() {
|
||
// The narrowing that keeps the adversarial contract intact: a payload with valid
|
||
// JPEG magic bytes and nothing behind them cannot be decoded, but acceptance follows
|
||
// the magic bytes by design (07-adversarial/file-upload-attacks). It is the worker's
|
||
// job to fail it, not admission's — admission is only the resource guard.
|
||
let dir = std::env::temp_dir().join("eventsnap-imaging-test");
|
||
std::fs::create_dir_all(&dir).expect("tmp dir");
|
||
let stub = dir.join("magic-only.jpg");
|
||
let mut bytes = vec![0u8; 1024];
|
||
bytes[..3].copy_from_slice(&[0xFF, 0xD8, 0xFF]);
|
||
std::fs::write(&stub, &bytes).expect("write stub");
|
||
|
||
assert!(
|
||
!exceeds_decode_budget(&stub),
|
||
"a corrupt file is not an over-budget file"
|
||
);
|
||
assert!(
|
||
decode_oriented(&stub)
|
||
.map(|i| (i.width(), i.height()))
|
||
.is_err(),
|
||
"...but it must still fail in the worker"
|
||
);
|
||
let _ = std::fs::remove_file(&stub);
|
||
}
|
||
|
||
#[test]
|
||
fn still_decodes_an_ordinary_photo_and_applies_orientation() {
|
||
// The guard must not have become a blanket refusal. This fixture is 40x20 stored with
|
||
// EXIF Orientation=6, so a correct decode returns it rotated to 20x40 portrait.
|
||
let path = concat!(
|
||
env!("CARGO_MANIFEST_DIR"),
|
||
"/../e2e/fixtures/media/portrait-exif6.jpg"
|
||
);
|
||
let img = decode_oriented(Path::new(path)).expect("an ordinary photo must decode");
|
||
assert_eq!(
|
||
(img.width(), img.height()),
|
||
(20, 40),
|
||
"EXIF orientation must still be applied after restoring the guard"
|
||
);
|
||
}
|
||
}
|