fix(upload): stop a late retry from undoing a host takedown

Migration 026 freed the idempotency key as soon as deleted_at was set, so a
retry after a delete uploads afresh instead of 409ing forever. That rationale
only considered the GUEST deleting. deleted_at is also set by
host_delete_upload, and there the same rule reverses a moderation decision:

  1. Guest uploads; the row commits and the photo appears, but the response
     is lost on the way back — the flaky-wifi case the key exists for — so
     the phone keeps the queue item.
  2. The host takes the photo down. Epoch bumped, keepsake rebuilt without it.
  3. 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.

Migration 031 keeps the key claimed for a host takedown and releases it only
for a guest's own delete, so the retry resolves to the duplicate path and is
refused. The refusal now says why ("von den Gastgebern entfernt") rather than
"already processed", which invites another try.

The index predicate and the ON CONFLICT arbiter are changed in lockstep;
these queries are not compile-checked, so a drift between them is a 500 on
exactly the retries the index exists to serve. Verified against a real
Postgres: live retry suppressed, host takedown holds the key, guest delete
releases it. The integration test's copy of the insert is updated too — it is
verbatim by design, and a stale copy would have kept passing.
This commit is contained in:
fabi
2026-08-12 09:14:51 +02:00
parent 1b3ca46f8a
commit 9b38d31f97
6 changed files with 177 additions and 18 deletions

View File

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

View File

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

View File

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

View File

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

View File

@@ -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<bool, sqlx::Error> {
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<bool, sqlx::Error> {
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 {

View File

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