diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 55f6830..d578769 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -233,6 +233,28 @@ pub async fn upload( ))); } + // Images only: refuse anything the compression worker could never decode, reading just + // the header. Without this the upload is accepted with a 201 and then silently + // soft-deleted minutes later when the worker gives up — the guest sees the photo + // vanish with, at best, a vague "could not be processed". Rejecting here gives them a + // reason at the door that they can act on, and it uses the SAME budget the worker + // enforces, so admission and processing cannot disagree. + if mime.starts_with("image/") + && let Err(e) = crate::services::imaging::probe_decodable(&temp_abs) + { + let mp = crate::services::imaging::megapixels(&temp_abs); + tracing::info!( + error = ?e, %mime, megapixels = ?mp, + "rejecting an image that exceeds the decode budget at admission" + ); + let _ = tokio::fs::remove_file(&temp_abs).await; + let detail = mp.map_or(String::new(), |mp| format!(" (ca. {mp:.0} Megapixel)")); + return Err(AppError::BadRequest(format!( + "Bild hat zu viele Bildpunkte{detail} und kann nicht verarbeitet werden. \ + Bitte verkleinere es und lade es erneut hoch." + ))); + } + // Per-user storage quota — dynamic formula based on available disk space and the // number of active uploaders. Gated by master + per-area toggles so the admin can // disable it on trusted instances. diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 0ad11d7..eba9f43 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -74,6 +74,11 @@ impl CompressionWorker { // an ENOSPC spike while several guests upload at once, a momentary DB-pool // exhaustion, a panic inside the image codec — and the give-up path is // user-visible data loss, so it is worth a few seconds to avoid entering it. + // + // But only for failures that CAN clear. An image that exceeds the decode budget, + // is corrupt, or is in an unsupported format fails identically on every attempt, + // so retrying it just burns 2s + 4s of backoff and writes three near-identical + // warnings before reaching the same conclusion. Give up on those immediately. let mut attempt = 1u32; let outcome = loop { match worker @@ -81,7 +86,10 @@ impl CompressionWorker { .await { Ok(v) => break Ok(v), - Err(e) if attempt < Self::MAX_PROCESS_ATTEMPTS => { + Err(e) + if attempt < Self::MAX_PROCESS_ATTEMPTS + && !crate::services::imaging::is_permanent_image_error(&e) => + { tracing::warn!( error = ?e, %upload_id, attempt, "compression attempt failed; retrying" diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index 6854c12..dc78b47 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -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 { +/// 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::(), + 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 { 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 { 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 { + 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 { + 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 diff --git a/e2e/specs/02-upload/oversized-image.spec.ts b/e2e/specs/02-upload/oversized-image.spec.ts index 3e4654a..678ed57 100644 --- a/e2e/specs/02-upload/oversized-image.spec.ts +++ b/e2e/specs/02-upload/oversized-image.spec.ts @@ -1,24 +1,26 @@ /** - * Regression guard — an image that would blow the decode budget must be refused, not - * allocated, and the container must survive it. + * Regression guard — an image that would blow the decode budget must be refused at the + * door, with a reason the guest can act on, and must never allocate. * - * The compression worker sets `max_alloc = 256 MiB`, but that budget was inert: reading the - * EXIF orientation tag requires `ImageReader::into_decoder()`, which skips the - * `limits.reserve(decoder.total_bytes())` that `decode()` performs, and nothing else enforces - * it (the JPEG decoder's `set_limits` only checks support and dimensions). So the only real - * bound was the 12000px per-axis cap — leaving 12000x12000 decodable at 412 MiB, and two - * concurrent decodes at 824 MiB against a 1 GiB container. + * Two defects met here. * - * That mattered acutely because bumping DERIVATIVES_REV makes the first boot after a deploy - * re-decode the whole gallery two at a time: an OOM kill there restarts the container, which - * re-runs the backfill — a boot loop. + * 1. The budget was inert. `max_alloc = 256 MiB` was set, but reading the EXIF orientation + * tag requires `ImageReader::into_decoder()`, which skips the + * `limits.reserve(decoder.total_bytes())` that `decode()` performs — and nothing else + * enforces it (the JPEG decoder's `set_limits` only checks support and dimensions). The + * only real bound was the 12000px per-axis cap, leaving two concurrent decodes at + * 824 MiB against a 1 GiB container. This suite could not have caught it either, because + * the e2e app container had NO memory limit while production is capped at 1 GiB; that cap + * is now mirrored in docker-compose.test.yml so these assertions mean something. * - * This suite could never have caught it, because until now the e2e app container had NO - * memory limit at all while production is capped at 1 GiB. The cap is mirrored in - * docker-compose.test.yml so this test means something. + * 2. Even with the budget restored, the upload was ACCEPTED with a 201 and then silently + * soft-deleted minutes later when the worker gave up — the photo simply vanished, with at + * best a vague "could not be processed". Admission now runs the same budget check against + * the header, so the guest is told immediately and told why. * * Fixture: 11000x9000 = 99 MP, 568 KiB on disk. Deliberately UNDER the per-axis cap, so the - * axis check cannot be what rejects it — 283 MiB decoded against a 256 MiB budget. + * axis check cannot be what rejects it — 283 MiB decoded against a 256 MiB budget. A fixture + * at 13000px would pass this test against a build with no budget at all. */ import { test, expect } from '../../fixtures/test'; import { uploadRaw } from '../../helpers/upload-client'; @@ -30,35 +32,29 @@ const HUGE = join(process.cwd(), 'fixtures', 'media', 'huge-99mp.jpg'); const SAMPLE = join(process.cwd(), 'fixtures', 'media', 'sample.jpg'); test.describe('Upload — an oversized image is refused, not allocated', () => { - test('a 99 MP upload fails compression gracefully and the backend stays up', async ({ - guest, - db, - }) => { - test.setTimeout(90_000); + test('a 99 MP upload is rejected at admission with a readable reason', async ({ guest, db }) => { + test.setTimeout(60_000); const g = await guest('BombThrower'); + const before = await db.countUploadsForUser(g.userId); - // The upload itself is accepted — 568 KiB is well within the body cap. The rejection - // happens in the compression worker, where the decode budget lives. const res = await uploadRaw(g.jwt, readFileSync(HUGE), { filename: 'huge.jpg', contentType: 'image/jpeg', caption: 'zu gross', }); - expect(res.status, 'a 568 KiB file is a legitimate upload').toBe(201); - const { id } = (await res.json()) as { id: string }; - // It must land in 'failed', not 'done' — and must get there, rather than the container - // dying mid-decode and leaving it stuck in 'processing' forever. - await expect - .poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] }) - .toBe('failed'); + // 4xx, not 201-then-vanish. The queue classifies this as terminal, so the guest gets the + // message rather than watching the photo disappear. + expect(res.status, 'an undecodable image must be refused at the door').toBe(400); + const body = (await res.json()) as { message?: string }; + expect(body.message ?? '', 'the reason must be actionable, not generic').toMatch(/bildpunkte/i); + expect(body.message ?? '', 'and should name the size so it is obvious why').toMatch(/99/); - // The whole point: the process is still alive. An OOM kill would have taken the backend - // down here, and Docker would have restarted it. - const health = await fetch(`${BASE}/health`); - expect(health.status, 'the backend must have survived the oversized decode').toBe(200); + // Nothing was stored — no row to soft-delete later, no orphaned file to sweep. + expect(await db.countUploadsForUser(g.userId)).toBe(before); - // And it is still doing useful work afterwards — not wedged or restarting. + // The backend never allocated: it is still alive and still doing useful work. + expect((await fetch(`${BASE}/health`)).status).toBe(200); const ok = await uploadRaw(g.jwt, readFileSync(SAMPLE), { filename: 'after.jpg', contentType: 'image/jpeg', @@ -68,27 +64,37 @@ test.describe('Upload — an oversized image is refused, not allocated', () => { await expect.poll(() => db.compressionStatus(after.id), { timeout: 30_000 }).toBe('done'); }); - test('two oversized uploads at once still leave the container alive', async ({ guest, db }) => { - // The concurrent case is the one that actually OOM'd: `compression_concurrency` is 2, so - // two decodes overlap. Under the old behaviour this pair peaked near the container cap. - test.setTimeout(90_000); + test('a burst of oversized uploads leaves the container alive', async ({ guest }) => { + // The concurrent case is the one that OOM'd: `compression_concurrency` is 2, so decodes + // overlapped. Four at once is comfortably past that, and must still cost only header + // reads. + test.setTimeout(60_000); const g = await guest('BombThrower2'); const bytes = readFileSync(HUGE); - const [a, b] = await Promise.all([ - uploadRaw(g.jwt, bytes, { filename: 'huge-a.jpg', contentType: 'image/jpeg' }), - uploadRaw(g.jwt, bytes, { filename: 'huge-b.jpg', contentType: 'image/jpeg' }), - ]); - expect([a.status, b.status]).toEqual([201, 201]); - const ids = [((await a.json()) as { id: string }).id, ((await b.json()) as { id: string }).id]; + const results = await Promise.all( + Array.from({ length: 4 }, (_, i) => + uploadRaw(g.jwt, bytes, { filename: `huge-${i}.jpg`, contentType: 'image/jpeg' }) + ) + ); + expect(results.map((r) => r.status)).toEqual([400, 400, 400, 400]); - for (const id of ids) { - await expect - .poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] }) - .toBe('failed'); - } + expect( + (await fetch(`${BASE}/health`)).status, + 'concurrent oversized uploads must not kill the backend' + ).toBe(200); + }); - const health = await fetch(`${BASE}/health`); - expect(health.status, 'two concurrent oversized decodes must not kill the backend').toBe(200); + test('an ordinary photo is unaffected by the admission check', async ({ guest, db }) => { + // The mirror that keeps the check honest: a budget that rejected everything would pass + // both tests above. + const g = await guest('NormalShooter'); + const res = await uploadRaw(g.jwt, readFileSync(SAMPLE), { + filename: 'normal.jpg', + contentType: 'image/jpeg', + }); + expect(res.status).toBe(201); + const { id } = (await res.json()) as { id: string }; + await expect.poll(() => db.compressionStatus(id), { timeout: 30_000 }).toBe('done'); }); });