diff --git a/backend/migrations/031_takedown_holds_idempotency_key.down.sql b/backend/migrations/031_takedown_holds_idempotency_key.down.sql new file mode 100644 index 0000000..ab7aa08 --- /dev/null +++ b/backend/migrations/031_takedown_holds_idempotency_key.down.sql @@ -0,0 +1,10 @@ +-- Restore migration 026's predicate, then drop the column it depended on. +-- +-- Note the same pairing caveat 026's own down carries: this is only valid alongside a code +-- rollback. `Upload::create` sends an ON CONFLICT predicate that must match the live index, so +-- running this down against the current binary makes every keyed upload a runtime 500. +DROP INDEX IF EXISTS upload_client_upload_id_key; +CREATE UNIQUE INDEX upload_client_upload_id_key ON upload (client_upload_id) + WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL; + +ALTER TABLE upload DROP COLUMN IF EXISTS taken_down_by_host; diff --git a/backend/migrations/031_takedown_holds_idempotency_key.up.sql b/backend/migrations/031_takedown_holds_idempotency_key.up.sql new file mode 100644 index 0000000..a630179 --- /dev/null +++ b/backend/migrations/031_takedown_holds_idempotency_key.up.sql @@ -0,0 +1,28 @@ +-- Keep a client upload key CLAIMED when the deletion was a host takedown. +-- +-- Migration 026 narrowed `upload_client_upload_id_key` to live rows so that a guest who deletes +-- their own photo and whose queue later retries gets a fresh upload instead of a permanent 409. +-- That rationale reasoned only about the GUEST deleting. `deleted_at` is also set by +-- `host_delete_upload`, and for that case the same rule undoes a moderation decision: +-- +-- 1. Guest uploads. The row commits and the photo appears in the feed, but the response is lost +-- on the way back (the flaky-wifi case this whole feature exists for), so the phone keeps the +-- queue item. +-- 2. The host sees the photo and takes it down. `deleted_at` is stamped, the keepsake epoch is +-- bumped, and the archive is rebuilt without it. +-- 3. Ten minutes later the phone reconnects and retries. The key is no longer claimed, the +-- INSERT succeeds, and the photo is BACK — in the feed, in the next keepsake, under a NEW +-- uuid that matches nothing in the host's moderation history, with nothing logged to say a +-- takedown was undone. +-- +-- So the key stays claimed for a host takedown and is released only for a guest's own delete. The +-- retry then resolves to the duplicate path and is refused, which is the correct answer: the photo +-- was deliberately removed, and re-sending the bytes must not bring it back. +ALTER TABLE upload ADD COLUMN taken_down_by_host BOOLEAN NOT NULL DEFAULT FALSE; + +-- KEEP THE PREDICATE IN LOCKSTEP WITH `Upload::create`'s ON CONFLICT clause (models/upload.rs). +-- A drift between the two is not a compile error here — queries are checked at runtime — it is a +-- 500 on every upload that carries a key, i.e. on exactly the retries this index exists to serve. +DROP INDEX IF EXISTS upload_client_upload_id_key; +CREATE UNIQUE INDEX upload_client_upload_id_key ON upload (client_upload_id) + WHERE client_upload_id IS NOT NULL AND (deleted_at IS NULL OR taken_down_by_host); diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index 17dae13..0dcba32 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -705,7 +705,9 @@ pub async fn host_delete_upload( // invalidation didn't, the taken-down photo would stay downloadable forever and nothing would // notice (the keepsake still looks complete, and the host can no longer find the upload to retry). let mut tx = state.pool.begin().await?; - let deleted = Upload::soft_delete_in_event(&mut tx, upload_id, auth.event_id).await?; + // `by_host: true` — the takedown holds the uploader's idempotency key so a late retry from + // their queue cannot resurrect the photo. See migration 031. + let deleted = Upload::soft_delete_in_event(&mut tx, upload_id, auth.event_id, true).await?; if !deleted { return Err(AppError::NotFound("Upload nicht gefunden.".into())); } diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index afee353..3034753 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -770,11 +770,28 @@ pub async fn upload( .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()) - })?; + // No live row behind the key. Two very different causes, and the guest deserves to be + // told which: either the winning row vanished in the intervening milliseconds, or the + // key is still held by a photo the HOST took down (migration 031), in which case the + // refusal is the whole point and re-sending will never work. + let existing = match existing { + Some(e) => e, + None => { + let taken_down = match client_upload_id { + Some(cid) => { + Upload::taken_down_by_client_upload_id(&state.pool, auth.user_id, cid) + .await + .unwrap_or(false) + } + None => false, + }; + return Err(AppError::Conflict(if taken_down { + "Dieses Foto wurde von den Gastgebern entfernt.".into() + } else { + "Dieser Upload wurde bereits verarbeitet.".to_string() + })); + } + }; tracing::info!( upload_id = %existing.id, "concurrent duplicate upload resolved; replaying the stored row" @@ -978,7 +995,9 @@ pub async fn delete_upload( // Atomic with the keepsake invalidation: a guest removing their own photo must have it removed // from the downloadable archive too, and a half-applied delete would leave it there forever. let mut tx = state.pool.begin().await?; - Upload::soft_delete_in_event(&mut tx, upload_id, auth.event_id).await?; + // `by_host: false` — the guest deleted their own photo, so the idempotency key is released and + // a later retry of the same queue item uploads afresh rather than 409ing forever. + Upload::soft_delete_in_event(&mut tx, upload_id, auth.event_id, false).await?; let regen = crate::services::export::invalidate_and_arm( &mut tx, &state.config.event_slug, diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index bdd22ae..bc96503 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -78,18 +78,20 @@ impl Upload { // The conflict target repeats the index's `WHERE` clause because it is a partial index; // without it Postgres cannot prove which index to use and rejects the statement. // - // KEEP THIS IN LOCKSTEP WITH `upload_client_upload_id_key` (migration 026). The predicate - // here must match the index's, or the arbiter cannot be inferred and every upload that - // carries a `client_upload_id` fails as a runtime 500 — queries in this codebase are not - // compile-time checked, so nothing catches a drift between the two at build time. + // KEEP THIS IN LOCKSTEP WITH `upload_client_upload_id_key` (migrations 026 and 031). The + // predicate here must match the index's, or the arbiter cannot be inferred and every + // upload that carries a `client_upload_id` fails as a runtime 500 — queries in this + // codebase are not compile-time checked, so nothing catches a drift at build time. // - // `AND deleted_at IS NULL` is what makes a retry-after-delete work instead of 409ing - // forever: the key is claimed only while a LIVE row holds it, which is what - // `find_by_client_upload_id` below has always assumed. + // `deleted_at IS NULL` is what makes a retry-after-delete work instead of 409ing forever: + // the key is claimed only while a LIVE row holds it, which is what + // `find_by_client_upload_id` below has always assumed. `OR taken_down_by_host` carves the + // moderation case back out — see migration 031: releasing the key for a HOST takedown let + // a late retry resurrect a photo the host had deliberately removed. sqlx::query_as::<_, Self>( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption, client_upload_id) VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL DO NOTHING + ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND (deleted_at IS NULL OR taken_down_by_host) DO NOTHING RETURNING *", ) .bind(event_id) @@ -124,6 +126,29 @@ impl Upload { .await } + /// Was this key claimed by a row the HOST took down? + /// + /// Only used to answer a refused retry honestly. Without it the guest's queue shows + /// "Dieser Upload wurde bereits verarbeitet." for a photo that was in fact removed by the + /// hosts — technically true, actively misleading, and it invites them to try again. + pub async fn taken_down_by_client_upload_id( + pool: &sqlx::PgPool, + user_id: Uuid, + client_upload_id: Uuid, + ) -> Result { + sqlx::query_scalar::<_, bool>( + "SELECT EXISTS ( + SELECT 1 FROM upload + WHERE client_upload_id = $1 AND user_id = $2 + AND deleted_at IS NOT NULL AND taken_down_by_host + )", + ) + .bind(client_upload_id) + .bind(user_id) + .fetch_one(pool) + .await + } + /// Lean lookup for the public media aliases (`get_original`/`get_preview`/ /// `get_thumbnail`): returns ONLY the file paths + mime for a visible upload — /// excluding soft-deleted rows, hidden owners (`uploads_hidden`), and banned owners @@ -289,20 +314,27 @@ impl Upload { /// dropped handler future, a failed second tx), the taken-down photo stays in the downloadable /// archive forever, and recovery can't tell — the keepsake still looks complete at the current /// epoch, and the host can no longer even find the upload to retry. + /// + /// `by_host` records WHO removed it, which decides whether the row keeps holding its + /// idempotency key — see migration 031. A host takedown holds it, so a late retry from the + /// uploader's queue cannot bring the photo back; a guest deleting their own photo releases it, + /// so their next upload of the same queue item succeeds. pub async fn soft_delete_in_event( conn: &mut sqlx::PgConnection, id: Uuid, event_id: Uuid, + by_host: bool, ) -> Result { let tx = conn; let row: Option<(Uuid, i64)> = sqlx::query_as( "UPDATE upload - SET deleted_at = NOW() + SET deleted_at = NOW(), taken_down_by_host = $3 WHERE id = $1 AND event_id = $2 AND deleted_at IS NULL RETURNING user_id, original_size_bytes", ) .bind(id) .bind(event_id) + .bind(by_host) .fetch_optional(&mut *tx) .await?; let deleted = if let Some((user_id, bytes)) = row { diff --git a/backend/tests/upload_idempotency.rs b/backend/tests/upload_idempotency.rs index 6f5ff52..90c7a3c 100644 --- a/backend/tests/upload_idempotency.rs +++ b/backend/tests/upload_idempotency.rs @@ -26,7 +26,7 @@ async fn create_upload( let row: Option<(Uuid,)> = sqlx::query_as( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption, client_upload_id) VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL DO NOTHING + ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND (deleted_at IS NULL OR taken_down_by_host) DO NOTHING RETURNING id", ) .bind(event_id) @@ -161,7 +161,10 @@ async fn a_deleted_upload_is_not_replayed(pool: PgPool) { let id = create_upload(&pool, event_id, user_id, "originals/a.jpg", Some(key)) .await .expect("first insert"); - sqlx::query("UPDATE upload SET deleted_at = NOW() WHERE id = $1") + // `taken_down_by_host = FALSE` — the GUEST deleted their own photo. See migration 031 and the + // sibling test below: the two cases must behave differently, and this is the one that frees + // the key. + sqlx::query("UPDATE upload SET deleted_at = NOW(), taken_down_by_host = FALSE WHERE id = $1") .bind(id) .execute(&pool) .await @@ -198,3 +201,68 @@ async fn a_deleted_upload_is_not_replayed(pool: PgPool) { "the live row is the one the replay lookup must now find" ); } + +/// The mirror of the test above, and the case migration 026's rationale did not consider. +/// +/// `deleted_at` is set by the guest deleting their own photo AND by `host_delete_upload`. Freeing +/// the idempotency key on both meant a takedown could be silently undone: the guest's response was +/// lost, so their queue still holds the item; the host removes the photo (bumping the keepsake +/// epoch and rebuilding the archive without it); the phone reconnects ten minutes later and +/// retries; the key is free, the INSERT succeeds, and the photo is back in the feed and in the next +/// keepsake — under a NEW uuid that matches nothing in the host's moderation history, with nothing +/// logged to say a takedown was reversed. The host has to find and delete it a second time. +/// +/// Migration 031 keeps the key claimed for a host takedown, so the retry resolves to the duplicate +/// path and is refused. Refusing is the correct answer here: the photo was deliberately removed. +#[sqlx::test] +async fn a_host_takedown_is_not_undone_by_a_late_retry(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user_id = seed_user(&pool, event_id, "Übermütiger Uwe").await; + let key = Uuid::new_v4(); + + let id = create_upload(&pool, event_id, user_id, "originals/a.jpg", Some(key)) + .await + .expect("first insert"); + + // SRC: `models/upload.rs::Upload::soft_delete_in_event` with `by_host = true`. + sqlx::query("UPDATE upload SET deleted_at = NOW(), taken_down_by_host = TRUE WHERE id = $1") + .bind(id) + .execute(&pool) + .await + .expect("host takedown"); + + let retried = create_upload(&pool, event_id, user_id, "originals/a.jpg", Some(key)).await; + assert_eq!( + retried, None, + "a retry after a HOST takedown must be suppressed — otherwise the phone silently \ + reinstates a photo the hosts removed" + ); + + let live: i64 = sqlx::query_scalar( + "SELECT count(*) FROM upload WHERE client_upload_id = $1 AND deleted_at IS NULL", + ) + .bind(key) + .fetch_one(&pool) + .await + .expect("count"); + assert_eq!(live, 0, "the taken-down photo must stay gone"); + + // And the handler must be able to tell the guest WHY, rather than "already processed". + // SRC: `models/upload.rs::Upload::taken_down_by_client_upload_id`. + let was_taken_down: bool = sqlx::query_scalar( + "SELECT EXISTS ( + SELECT 1 FROM upload + WHERE client_upload_id = $1 AND user_id = $2 + AND deleted_at IS NOT NULL AND taken_down_by_host + )", + ) + .bind(key) + .bind(user_id) + .fetch_one(&pool) + .await + .expect("takedown lookup"); + assert!( + was_taken_down, + "the refusal must be attributable to a takedown so the queue can say so" + ); +}