fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'fix/no-retry-on-permanent-failure'

Browse Source
This commit is contained in:
fabi
2026-07-29 07:29:21 +02:00
parent 2bd54d7f0b fae12bd7ec
commit 674ea87bbd
4 changed files with 187 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
backend/src/handlers/upload.rs
View File

@@ -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.

10
backend/src/services/compression.rs
View File

@@ -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"

103
backend/src/services/imaging.rs
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

108
e2e/specs/02-upload/oversized-image.spec.ts
View File

@@ -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');
});
});