fix(upload): refuse undecodable images at the door, and stop retrying them

Two halves of the same complaint: an oversized photo was accepted with a 201 and
then silently soft-deleted minutes later, after the worker had burned six seconds
of backoff re-reaching a conclusion it could not change.

Admission. The compression budget now runs at upload time, against the header
only, so a guest is told immediately and told why:

  "Bild hat zu viele Bildpunkte (ca. 99 Megapixel) und kann nicht verarbeitet
   werden. Bitte verkleinere es und lade es erneut hoch."

instead of watching the photo vanish behind a vague "could not be processed" —
which arrived only if they happened to still be on the feed with that card
loaded. Nothing is stored, so there is no row to soft-delete and no orphan for
the sweep to reclaim.

Admission and the worker share ONE function (`decoder_within_budget`), so they
cannot drift apart and start disagreeing about what is acceptable — a photo
accepted at the door and rejected by the worker would be worse than either
behaviour alone. The worker keeps its own check: the backfill decodes files that
predate this check, and defence in depth is the whole reason the budget exists.

Retries. The loop retried every failure, including ones that are a property of
the input. An image over the budget, a corrupt file, an unsupported format: each
fails identically on all three attempts, so the only effect was 2s + 4s of sleep
and three near-identical warnings before the same outcome. `is_permanent_image_error`
classifies the `ImageError` variants that cannot change between attempts — Limits,
Unsupported, Decoding — and the loop gives up on those at once. `IoError` is
deliberately excluded: an ENOSPC while writing a derivative is exactly the
transient case the retry exists for, and misclassifying it would turn a blip back
into the data loss round 1 fixed. Measured: retry log lines went from 3 per
oversized upload to 0.

Tests: unit tests for both sides of the classifier (a Limits error is permanent, a
missing file is not) and for admission agreeing with the decoder on accept AND
reject. The e2e spec is rewritten for the new contract — 400 with an actionable
message, nothing stored, backend alive after a burst of four — plus a mirror
asserting an ordinary photo still uploads and processes, since a budget that
rejected everything would satisfy the other two.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
fabi
2026-07-29 07:29:21 +02:00
parent 2bd54d7f0b
commit fae12bd7ec
4 changed files with 187 additions and 56 deletions

View File

@@ -34,11 +34,32 @@ fn decode_limits() -> image::Limits {
limits
}
/// Decode an image from disk with decompression-bomb limits applied and its EXIF
/// orientation baked into the pixels.
/// 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.
///
/// Blocking — call inside `spawn_blocking`.
pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
/// 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.
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(_)
)
)
})
}
/// 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()
@@ -64,6 +85,35 @@ pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
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)
}
/// Reject an image the compression worker could never process, reading only its header.
///
/// Called at upload admission so the guest 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.
pub fn probe_decodable(path: &Path) -> Result<()> {
decoder_within_budget(path).map(|_| ())
}
/// 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.
@@ -108,6 +158,51 @@ mod tests {
);
}
#[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. ENOSPC while writing a derivative, or
// EMFILE under load, is exactly what the retry exists for — misclassifying those as
// permanent would turn a transient blip back into the data loss round 1 fixed.
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 probe_agrees_with_the_decoder_on_both_sides() {
// Admission and processing must never disagree — a photo accepted at the door and
// then rejected by the worker is the exact failure this pair exists to prevent.
assert!(
probe_decodable(Path::new(HUGE)).is_err(),
"probe must reject what the decoder rejects"
);
let ordinary = concat!(
env!("CARGO_MANIFEST_DIR"),
"/../e2e/fixtures/media/portrait-exif6.jpg"
);
assert!(
probe_decodable(Path::new(ordinary)).is_ok(),
"probe must accept what the decoder accepts"
);
}
#[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