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

feat(upload): make uploads idempotent so a lost response cannot duplicate a photo

Browse Source 
The ordinary mobile failure, not an exotic one: the server receives the body,
validates it, commits the row — and the response is lost on the way back because the
guest walked out of range or the AP dropped the connection. The client sees a network
error with the blob still in hand and re-sends it, both when the guest taps "Erneut"
and automatically when the queue requeues on reconnect. Every attempt minted a fresh
`Uuid::new_v4()` server-side, so the same photo landed in the gallery two or three
times and was charged against the guest's storage quota each time.

The client already has a stable per-queue-item UUID, so it costs nothing to send.
Migration 022 adds `client_upload_id` with a partial unique index — partial so the
NULLs of every pre-022 upload, and of any caller that doesn't send one, keep working
untouched.

Two paths, because there are two races:

- Sequential retry: a lookup before the transaction finds the stored row, deletes the
  re-sent bytes and replays the original response as 200. The body has necessarily
  already been streamed, since the key arrives as a multipart field — re-sending is the
  client's cost and is already paid by the time we see it. What must be prevented is a
  second ROW.
- Concurrent retry: two attempts in flight at once. `ON CONFLICT DO NOTHING` returns no
  row to the loser, which abandons its transaction (quota increment included) and
  replays the winner. Letting the unique index raise instead would only surface after
  the transaction had aborted, as an opaque error the caller would have to string-match.

The replay reads live state rather than assuming a fresh row: a reconnect can be
minutes later, by which time the derivatives may exist and the photo may have been
liked. Every read there fails soft — the upload is already safely stored, so a sparser
response is fine and failing the request is not.

Verified live: the same photo sent three times returns 201, 200, 200 with one id, one
row, and the quota charged exactly once.

Also in this file: the two image-header probes at admission now run on `spawn_blocking`.
Both open the file and run the codec's header parse synchronously, and `#[tokio::main]`
gives two worker threads on a 2-vCPU box — so every upload stalled half the runtime's
request-serving capacity. Everything else that blocks here (image encode, bcrypt) was
already offloaded; this was the one that wasn't.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Fabian Hamm (Privat)
2026-08-03 18:35:05 +02:00
parent faea555967
commit 1d0df3ebf6
5 changed files with 431 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -111,6 +111,9 @@ pub async fn upload(
let mut streamed: Option<(i64, Vec<u8>)> = None; // (size, head bytes for sniffing)
let mut caption: Option<String> = None;
let mut hashtags_csv: Option<String> = None;
// The client's idempotency key. Optional: an older client, or any other caller, simply
// doesn't send one and gets the previous behaviour.
let mut client_upload_id: Option<Uuid> = None;
// Wrap the multipart read so any error after the temp file is created still cleans
// it up (a mid-stream parse failure must not leave a stray `.tmp` on disk).
@@ -158,6 +161,16 @@ pub async fn upload(
.map_err(|e| AppError::BadRequest(e.to_string()))?,
);
}
"client_upload_id" => {
let raw = field
.text()
.await
.map_err(|e| AppError::BadRequest(e.to_string()))?;
// A malformed key is not worth rejecting an upload over — the photo is the
// thing the guest cares about. Drop the key and lose only the retry
// protection, which is exactly where we were before it existed.
client_upload_id = Uuid::parse_str(raw.trim()).ok();
}
_ => {}
}
}
@@ -170,6 +183,29 @@ pub async fn upload(
return Err(e);
}
// Idempotency, fast path: this key already has a live upload, so the previous attempt DID
// succeed and only its response was lost. Replay that response instead of storing the photo
// a second time and charging the guest's quota twice.
//
// The body has necessarily already been streamed to disk — the key arrives as a multipart
// field, so it cannot be known before the body is read. Re-sending the bytes is the client's
// cost and it has already been paid by the time we get here; what has to be prevented is a
// second ROW, and that is what this does. The concurrent case (two retries in flight at once)
// is caught by the unique index inside the transaction below.
if let Some(cid) = client_upload_id
&& let Some(existing) = Upload::find_by_client_upload_id(&state.pool, auth.user_id, cid)
.await
.map_err(AppError::from)?
{
let _ = tokio::fs::remove_file(&temp_abs).await;
tracing::info!(
client_upload_id = %cid, upload_id = %existing.id,
"duplicate upload suppressed; replaying the original response"
);
let dto = replay_upload_dto(&state, &existing, &user.display_name).await;
return Ok((StatusCode::OK, Json(dto)));
}
// From here on the temp file may exist; every validation failure removes it before
// returning so a rejected upload never leaves bytes behind.
let (size, head) = match streamed {
@@ -239,18 +275,42 @@ pub async fn upload(
// 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/") && crate::services::imaging::exceeds_decode_budget(&temp_abs) {
let mp = crate::services::imaging::megapixels(&temp_abs);
tracing::info!(
%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."
)));
//
// Both probes open the file and run the codec's header parse — synchronous filesystem and
// CPU work. They ran inline on the async task, which on this 2-vCPU box means tokio has
// exactly two worker threads and every upload stalled half the runtime's request-serving
// capacity. Everything else in the app that blocks (image encode, bcrypt) is already on the
// blocking pool; this was the one that wasn't.
if mime.starts_with("image/") {
let probe_path = temp_abs.clone();
let probe = tokio::task::spawn_blocking(move || {
let over = crate::services::imaging::exceeds_decode_budget(&probe_path);
// Only pay for the second header read when it will actually be shown to the guest.
let mp = over
.then(|| crate::services::imaging::megapixels(&probe_path))
.flatten();
(over, mp)
})
.await;
// A join error is the blocking pool panicking or shutting down. That says nothing about
// the image, so admit it and let the compression worker be the judge rather than
// rejecting a photo for an infrastructure reason.
let (over_budget, mp) = probe.unwrap_or_else(|e| {
tracing::warn!(error = ?e, "decode-budget probe failed to run; admitting the upload");
(false, None)
});
if over_budget {
tracing::info!(
%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
@@ -273,7 +333,13 @@ pub async fn upload(
if prospective_total > limit {
let _ = tokio::fs::remove_file(&temp_abs).await;
return Err(AppError::QuotaExceeded(
"Du hast dein Upload-Limit für dieses Event erreicht.".into(),
// Name the remedy, because the guest cannot see the number. Every quota
// display is staff-gated by design, so a guest hitting this had no idea what
// the limit was, how close they were, or what to do — and the one sentence
// that tells them ("delete older posts") lived inside the staff-only block.
"Du hast dein Upload-Limit für dieses Event erreicht. Lösche ältere eigene \
Beiträge, um wieder Platz zu schaffen."
.into(),
));
}
}
@@ -364,10 +430,20 @@ pub async fn upload(
};
if inc.rows_affected() == 0 {
return Err(AppError::QuotaExceeded(
"Du hast dein Upload-Limit für dieses Event erreicht.".into(),
// Name the remedy, because the guest cannot see the number. Every quota
// display is staff-gated by design, so a guest hitting this had no idea what
// the limit was, how close they were, or what to do — and the one sentence
// that tells them ("delete older posts") lived inside the staff-only block.
"Du hast dein Upload-Limit für dieses Event erreicht. Lösche ältere eigene \
Beiträge, um wieder Platz zu schaffen."
.into(),
));
}
let upload = Upload::create(
// `None` means a concurrent request already stored this key. The transaction — quota
// increment included — is abandoned by returning here, and the caller replays the winning
// row. This is the narrow race the fast path above cannot see: two retries of the same
// photo in flight at the same moment.
let Some(upload) = Upload::create(
&mut *tx,
auth.event_id,
auth.user_id,
@@ -375,8 +451,12 @@ pub async fn upload(
&mime,
size,
caption.as_deref(),
client_upload_id,
)
.await?;
.await?
else {
return Err(AppError::Conflict(DUPLICATE_UPLOAD_MARKER.into()));
};
for tag in &tags {
let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?;
Hashtag::link_to_upload(&mut *tx, upload.id, h.id).await?;
@@ -390,6 +470,28 @@ pub async fn upload(
// row will ever reference it, so remove it now rather than orphan bytes on disk.
let upload = match tx_result {
Ok(u) => u,
// The concurrent duplicate resolved inside the transaction. The winner's row is committed;
// answer with it so both retries of the same photo get the same successful reply.
Err(AppError::Conflict(ref marker)) if marker == DUPLICATE_UPLOAD_MARKER => {
let _ = tokio::fs::remove_file(&absolute_path).await;
let existing = match client_upload_id {
Some(cid) => Upload::find_by_client_upload_id(&state.pool, auth.user_id, cid)
.await
.map_err(AppError::from)?,
None => None,
};
// If the winning row has vanished between the conflict and this lookup (deleted in
// the intervening milliseconds), there is nothing to replay — report the conflict.
let existing = existing.ok_or_else(|| {
AppError::Conflict("Dieser Upload wurde bereits verarbeitet.".into())
})?;
tracing::info!(
upload_id = %existing.id,
"concurrent duplicate upload resolved; replaying the stored row"
);
let dto = replay_upload_dto(&state, &existing, &user.display_name).await;
return Ok((StatusCode::OK, Json(dto)));
}
Err(e) => {
let _ = tokio::fs::remove_file(&absolute_path).await;
return Err(e);
@@ -466,7 +568,15 @@ pub async fn edit_upload(
}
if let Some(ref hashtags) = body.hashtags {
Hashtag::unlink_all_from_upload(&mut *tx, upload_id).await?;
for tag in hashtags {
// Sort + dedup before upserting, exactly as the upload path does. `Hashtag::upsert`
// takes row locks, so two transactions touching the same two tags in OPPOSITE order
// deadlock; Postgres aborts one after ~1s and the guest gets a 500. Here the order is
// whatever the client sent, so it is genuinely attacker-free but genuinely unordered.
// Sort on the NORMALISED form — that is the key `upsert` actually locks on.
let mut tags: Vec<&String> = hashtags.iter().collect();
tags.sort_by_key(|t| t.trim().trim_start_matches('#').to_lowercase());
tags.dedup_by_key(|t| t.trim().trim_start_matches('#').to_lowercase());
for tag in tags {
let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?;
Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?;
}
@@ -592,6 +702,69 @@ async fn stream_field_to_file(
Ok((total as i64, head))
}
/// Sentinel for the duplicate detected INSIDE the commit transaction. It never reaches a client:
/// the caller intercepts this exact `Conflict` and answers with the stored row. A marker rather
/// than a new `AppError` variant because the condition is local to this one handler and returning
/// early is the only way to abandon the transaction from inside the async block.
const DUPLICATE_UPLOAD_MARKER: &str = "__duplicate_client_upload_id__";
/// Rebuild the response for an upload that already exists, so a retry is answered exactly as the
/// original was.
///
/// Reads the live state rather than assuming a fresh row: by the time a retry arrives — a
/// reconnect can be minutes later — the derivatives may have been generated and the photo may
/// already have been liked, and a response claiming otherwise would be wrong in a way the client
/// has no way to detect.
///
/// Every read here fails soft. This is the success path of an upload that is already safely
/// stored; degrading to a sparser response is fine, failing the request is not.
async fn replay_upload_dto(state: &AppState, upload: &Upload, uploader_name: &str) -> UploadDto {
let hashtags: Vec<String> = sqlx::query_scalar(
"SELECT h.tag FROM upload_hashtag uh
JOIN hashtag h ON h.id = uh.hashtag_id
WHERE uh.upload_id = $1
ORDER BY h.tag",
)
.bind(upload.id)
.fetch_all(&state.pool)
.await
.unwrap_or_default();
let counts: Option<(i64, i64, bool)> = sqlx::query_as(
"SELECT v.like_count, v.comment_count,
EXISTS (SELECT 1 FROM \"like\" l WHERE l.upload_id = v.id AND l.user_id = $2)
FROM v_feed v WHERE v.id = $1",
)
.bind(upload.id)
.bind(upload.user_id)
.fetch_optional(&state.pool)
.await
.ok()
.flatten();
let (like_count, comment_count, liked_by_me) = counts.unwrap_or((0, 0, false));
UploadDto {
id: upload.id,
user_id: upload.user_id,
uploader_name: uploader_name.to_string(),
preview_url: upload
.preview_path
.as_ref()
.map(|_| format!("/api/v1/upload/{}/preview", upload.id)),
thumbnail_url: upload
.thumbnail_path
.as_ref()
.map(|_| format!("/api/v1/upload/{}/thumbnail", upload.id)),
mime_type: upload.mime_type.clone(),
caption: upload.caption.clone(),
hashtags,
like_count,
comment_count,
liked_by_me,
created_at: upload.created_at,
}
}
/// Drain a multipart body so the HTTP connection stays clean when returning an early error.
/// Without draining, the client may still be sending the body after we've sent our response,
/// which can corrupt the keep-alive connection for subsequent requests.