fix(compression): reclaim failed originals instead of leaking them
Round 1 stopped the compression worker deleting an upload's original on failure — a transient ENOSPC or a codec panic must never destroy the only copy of a photo a guest cannot retake. But it left `Upload::soft_delete`'s quota refund in place, so the bytes stayed on disk while the uploader was charged nothing for them. That is worse than it first looks. The row is soft-deleted, so the file is invisible and unowned; a guest hitting a reproducible codec failure can accumulate orphans indefinitely at zero personal cost. And `active_uploaders` counts only users with non-deleted uploads, so dropping out of that count RAISES everyone's per-user ceiling — the leak loosens the very quota meant to contain it. Keep the refund: the uploader didn't cause the failure and shouldn't silently lose quota to it. Bound the leak instead, with an hourly sweep alongside the existing session cleanup in `spawn_periodic_tasks`, reclaiming failed originals older than 14 days — comfortably longer than any single event, so an operator investigating a failed upload still has the file. The selection predicate is the entire safety argument, so it is deliberately narrow: `compression_status = 'failed'` AND soft-deleted AND past the window AND `original_path <> ''`. That is exactly the state the give-up path leaves behind, and it cannot reach a live upload, an owner-deleted one, or a failure still inside its recovery window. `original_path` is cleared after a successful reclaim, which makes the sweep idempotent — otherwise a row whose file is already gone is re-selected on every tick forever. The row itself is kept as the audit trail. Tests reproduce the selection verbatim (same pattern as upload_concurrency) and assert it against five near-misses that must survive, both sides of the retention boundary, and the idempotence property. Also fixes two comments in export.rs still claiming "the compression worker hard-deletes an original when its transcode fails" — no longer true, and the defensive handling they justify is now justified by this sweep and by ordinary deletes instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
fabi
207
backend/tests/failed_original_sweep.rs
Normal file
207
backend/tests/failed_original_sweep.rs
Normal file
@@ -0,0 +1,207 @@
|
||||
//! DB-backed tests for the failed-original sweep (`services/maintenance.rs`).
|
||||
//!
|
||||
//! Context: the compression worker deliberately no longer deletes an upload's original when
|
||||
//! its transcode fails — a transient ENOSPC or a codec panic must never destroy the only
|
||||
//! copy of a photo a guest cannot retake. But the row is soft-deleted and the uploader's
|
||||
//! quota IS refunded, so those bytes become invisible, unowned and free. A guest hitting a
|
||||
//! reproducible codec failure could accumulate orphans at no personal cost, and since
|
||||
//! `active_uploaders` counts only users with non-deleted uploads, dropping out of that count
|
||||
//! actually RAISES everyone's per-user ceiling while the disk fills.
|
||||
//!
|
||||
//! The sweep reclaims them after a retention window. Its selection predicate is the whole
|
||||
//! safety argument — it must reach the give-up path's leftovers and nothing else — so that
|
||||
//! is what these tests pin, following the same "reproduce the SQL verbatim" pattern as
|
||||
//! `upload_concurrency.rs`.
|
||||
//!
|
||||
//! `#[sqlx::test]` gives each test a fresh database with the real migrations applied.
|
||||
|
||||
mod common;
|
||||
|
||||
use common::*;
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
/// SRC: `services/maintenance.rs::cleanup_failed_originals` — the selection, verbatim.
|
||||
async fn sweep_selects(pool: &PgPool, retention_days: i64) -> Vec<Uuid> {
|
||||
sqlx::query_as::<_, (Uuid, String)>(
|
||||
"SELECT id, original_path FROM upload
|
||||
WHERE compression_status = 'failed'
|
||||
AND deleted_at IS NOT NULL
|
||||
AND deleted_at < NOW() - ($1 || ' days')::interval
|
||||
AND original_path <> ''",
|
||||
)
|
||||
.bind(retention_days.to_string())
|
||||
.fetch_all(pool)
|
||||
.await
|
||||
.expect("sweep query")
|
||||
.into_iter()
|
||||
.map(|(id, _)| id)
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
async fn seed_upload(
|
||||
pool: &PgPool,
|
||||
event_id: Uuid,
|
||||
user_id: Uuid,
|
||||
status: &str,
|
||||
deleted_days_ago: Option<i64>,
|
||||
original_path: &str,
|
||||
) -> Uuid {
|
||||
let id: Uuid = sqlx::query_scalar(
|
||||
"INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes,
|
||||
compression_status, deleted_at)
|
||||
VALUES ($1, $2, $3, 'image/jpeg', 1000, $4,
|
||||
CASE WHEN $5::bigint IS NULL THEN NULL
|
||||
ELSE NOW() - ($5::text || ' days')::interval END)
|
||||
RETURNING id",
|
||||
)
|
||||
.bind(event_id)
|
||||
.bind(user_id)
|
||||
.bind(original_path)
|
||||
.bind(status)
|
||||
.bind(deleted_days_ago)
|
||||
.fetch_one(pool)
|
||||
.await
|
||||
.expect("seed upload");
|
||||
id
|
||||
}
|
||||
|
||||
#[sqlx::test]
|
||||
async fn sweeps_only_long_failed_soft_deleted_uploads(pool: PgPool) {
|
||||
let event_id = seed_event(&pool, "sweep-event").await;
|
||||
let user_id = seed_user(&pool, event_id, "Sweeper").await;
|
||||
|
||||
// The one and only thing the sweep may touch: the exact state the compression worker's
|
||||
// give-up path leaves behind, aged past the window.
|
||||
let target = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
Some(30),
|
||||
"originals/e/target.jpg",
|
||||
)
|
||||
.await;
|
||||
|
||||
// Everything below is a near-miss that must survive.
|
||||
// A healthy live upload — the catastrophic case if the predicate were ever loosened.
|
||||
let live = seed_upload(&pool, event_id, user_id, "done", None, "originals/e/live.jpg").await;
|
||||
// Failed but still inside the retention window: the recovery window is the entire point
|
||||
// of keeping the file, so reclaiming it early would defeat the fix it protects.
|
||||
let recent = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
Some(1),
|
||||
"originals/e/recent.jpg",
|
||||
)
|
||||
.await;
|
||||
// Failed but NOT soft-deleted — not the give-up path; something else set this status.
|
||||
let failed_live = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
None,
|
||||
"originals/e/failed-live.jpg",
|
||||
)
|
||||
.await;
|
||||
// Soft-deleted by the OWNER, compression fine. Its file is already gone; this row must
|
||||
// never be re-processed.
|
||||
let owner_deleted = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"done",
|
||||
Some(30),
|
||||
"originals/e/owner.jpg",
|
||||
)
|
||||
.await;
|
||||
// Already swept: `original_path` cleared. Re-selecting it every hour would log a
|
||||
// phantom reclaim forever.
|
||||
let already_swept = seed_upload(&pool, event_id, user_id, "failed", Some(30), "").await;
|
||||
|
||||
let selected = sweep_selects(&pool, 14).await;
|
||||
|
||||
assert_eq!(
|
||||
selected,
|
||||
vec![target],
|
||||
"the sweep must select exactly the aged give-up-path leftovers"
|
||||
);
|
||||
for (id, what) in [
|
||||
(live, "a live upload"),
|
||||
(recent, "a failure still inside the retention window"),
|
||||
(failed_live, "a failed but not soft-deleted upload"),
|
||||
(owner_deleted, "an owner-deleted upload"),
|
||||
(already_swept, "an already-swept row"),
|
||||
] {
|
||||
assert!(!selected.contains(&id), "the sweep must not touch {what}");
|
||||
}
|
||||
}
|
||||
|
||||
#[sqlx::test]
|
||||
async fn retention_window_is_honoured_at_the_boundary(pool: PgPool) {
|
||||
let event_id = seed_event(&pool, "sweep-boundary").await;
|
||||
let user_id = seed_user(&pool, event_id, "Boundary").await;
|
||||
|
||||
let inside = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
Some(13),
|
||||
"originals/e/inside.jpg",
|
||||
)
|
||||
.await;
|
||||
let outside = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
Some(15),
|
||||
"originals/e/outside.jpg",
|
||||
)
|
||||
.await;
|
||||
|
||||
let selected = sweep_selects(&pool, 14).await;
|
||||
assert!(
|
||||
selected.contains(&outside),
|
||||
"15 days old must be past a 14-day window"
|
||||
);
|
||||
assert!(
|
||||
!selected.contains(&inside),
|
||||
"13 days old must still be retained"
|
||||
);
|
||||
}
|
||||
|
||||
#[sqlx::test]
|
||||
async fn clearing_original_path_makes_the_sweep_idempotent(pool: PgPool) {
|
||||
// The sweep clears `original_path` after reclaiming the file. Without that, a row whose
|
||||
// file is already gone is re-selected on every hourly tick forever.
|
||||
let event_id = seed_event(&pool, "sweep-idempotent").await;
|
||||
let user_id = seed_user(&pool, event_id, "Idem").await;
|
||||
let id = seed_upload(
|
||||
&pool,
|
||||
event_id,
|
||||
user_id,
|
||||
"failed",
|
||||
Some(30),
|
||||
"originals/e/once.jpg",
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(sweep_selects(&pool, 14).await, vec![id]);
|
||||
|
||||
sqlx::query("UPDATE upload SET original_path = '' WHERE id = $1")
|
||||
.bind(id)
|
||||
.execute(&pool)
|
||||
.await
|
||||
.expect("clear path");
|
||||
|
||||
assert!(
|
||||
sweep_selects(&pool, 14).await.is_empty(),
|
||||
"a swept row must not come back"
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user