fix(upload): remove the /original rate limit that would have broken the feed
The limiter added here was justified as bounding "100 guests occasionally tapping Original anzeigen". That is not what this route is. `pickMediaUrl` resolves to `preview_url ?? thumbnail_url ?? /original`, and a freshly committed upload has BOTH derivatives null until the compression worker reaches it — at COMPRESSION_WORKER_CONCURRENCY=2 that is minutes during a post-ceremony burst. So /original is the feed's hot path for exactly the newest photos, in a newest-first grid, at the busiest moment. With every guest behind one NAT the 600/min bucket is venue-wide: six new photos fanned out by `upload-new` to ~100 open feeds exhausts it, and then every original fetch from anyone 429s for the rest of the window. The tiles' own 4-second retry uses a fresh `?r=` nonce, so the clients hold the bucket saturated themselves — the whole venue watching the newest photos render as broken tiles while the projector skips slides. A per-IP bucket cannot separate one scraper from the entire party when they share an address, and these media routes are unauthenticated by design (an `<img>` cannot send a bearer token), so there is no per-user key to move to. Bandwidth abuse belongs at the proxy. Also here: the release/lock check order. `release ⇒ lock`, so testing the lock first made the `GalleryReleased` arm unreachable dead code and every post-release upload answered `uploads_locked`. The codes are not interchangeable to the client — `uploads_locked` charges a retry attempt and re-pushes the whole photo on the backoff ladder against an answer that cannot change, while `gallery_released` parks it and says the photo is safe but the hosts must reopen. Both sites now test release first, so the fast path and the commit-time re-check agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -97,7 +97,8 @@ impl CompressionWorker {
|
||||
let mut attempt = 1u32;
|
||||
let outcome = loop {
|
||||
match worker
|
||||
.do_process(upload_id, &original_path, &mime_type)
|
||||
// Charge the lifetime budget once per episode, on the first attempt only.
|
||||
.do_process(upload_id, &original_path, &mime_type, attempt == 1)
|
||||
.await
|
||||
{
|
||||
Ok(v) => break Ok(v),
|
||||
@@ -205,11 +206,25 @@ impl CompressionWorker {
|
||||
});
|
||||
}
|
||||
|
||||
/// `charge_lifetime_attempt` is true only for the FIRST `do_process` of a given
|
||||
/// `process()` call, so the two budgets stay independent.
|
||||
///
|
||||
/// They were not. `MAX_PROCESS_ATTEMPTS` (in-request retries, 3) and
|
||||
/// `MAX_DERIVATIVE_ATTEMPTS` (lifetime, 3) are equal, and every retry re-entered here and
|
||||
/// charged the lifetime counter — so one request's three retries, six seconds apart,
|
||||
/// exhausted the entire lifetime budget. A ten-second pool blip during the arrival burst
|
||||
/// therefore stranded every photo whose worker was inside that window with no preview and no
|
||||
/// display derivative, permanently, recoverable by nothing: the boot backfill re-selects them
|
||||
/// and immediately gives up on the same exhausted counter.
|
||||
///
|
||||
/// The two exist to bound different things — "this request is flapping" versus "this INPUT is
|
||||
/// poison" — and only the second should survive across requests.
|
||||
async fn do_process(
|
||||
&self,
|
||||
upload_id: Uuid,
|
||||
original_path: &str,
|
||||
mime_type: &str,
|
||||
charge_lifetime_attempt: bool,
|
||||
) -> Result<()> {
|
||||
Upload::set_compression_status(&self.pool, upload_id, "processing").await?;
|
||||
|
||||
@@ -218,8 +233,15 @@ impl CompressionWorker {
|
||||
if mime_type.starts_with("image/") {
|
||||
// Count the attempt BEFORE doing the work — see `begin_derivative_attempt`. If this
|
||||
// input is the one that kills the container, this write is the only record that
|
||||
// survives, and it is what stops the boot backfill replaying it forever.
|
||||
match Upload::begin_derivative_attempt(&self.pool, upload_id).await? {
|
||||
// survives, and it is what stops the boot backfill replaying it forever. Charging on
|
||||
// the first attempt preserves that: a container-killing input never reaches a second.
|
||||
let charged = if charge_lifetime_attempt {
|
||||
Upload::begin_derivative_attempt(&self.pool, upload_id).await?
|
||||
} else {
|
||||
// Already charged for this episode. Re-read the row only to notice it vanished.
|
||||
Upload::derivative_attempts(&self.pool, upload_id).await?
|
||||
};
|
||||
match charged {
|
||||
Some(attempts) if attempts > Self::MAX_DERIVATIVE_ATTEMPTS => {
|
||||
anyhow::bail!(
|
||||
"derivative generation gave up after {} attempt(s)",
|
||||
@@ -304,7 +326,6 @@ impl CompressionWorker {
|
||||
/// saving rather than risk the OOM kill.
|
||||
const OXIPNG_MAX_PIXELS: u64 = 8_000_000;
|
||||
|
||||
|
||||
/// Wall-clock ceiling for one oxipng run.
|
||||
///
|
||||
/// Bounds TIME, NOT MEMORY — oxipng checks the deadline between trials, so a single trial
|
||||
@@ -336,8 +357,10 @@ impl CompressionWorker {
|
||||
// the upload handler already does via `exceeds_decode_budget`) and, if this job is a
|
||||
// giant, take the exclusive permit so it cannot overlap another giant. Held for the
|
||||
// whole blocking section, released on drop including on error.
|
||||
let estimate =
|
||||
crate::services::imaging::estimated_processing_peak_bytes(&original, Self::DISPLAY_MAX_EDGE);
|
||||
let estimate = crate::services::imaging::estimated_processing_peak_bytes(
|
||||
&original,
|
||||
Self::DISPLAY_MAX_EDGE,
|
||||
);
|
||||
let _heavy_permit = match estimate {
|
||||
Some(bytes) if bytes > crate::services::imaging::HEAVY_IMAGE_BYTES => {
|
||||
tracing::debug!(
|
||||
@@ -345,14 +368,24 @@ impl CompressionWorker {
|
||||
estimated_mib = bytes / (1024 * 1024),
|
||||
"waiting for the heavy-image permit"
|
||||
);
|
||||
Some(crate::services::imaging::HEAVY_IMAGE_PERMITS.acquire().await)
|
||||
Some(
|
||||
crate::services::imaging::HEAVY_IMAGE_PERMITS
|
||||
.acquire()
|
||||
.await,
|
||||
)
|
||||
}
|
||||
_ => None,
|
||||
};
|
||||
|
||||
// Run blocking image operations in a spawn_blocking task
|
||||
tokio::task::spawn_blocking(move || {
|
||||
write_image_derivatives(upload_id, &original, &mime_owned, &preview_path, &display_path)
|
||||
write_image_derivatives(
|
||||
upload_id,
|
||||
&original,
|
||||
&mime_owned,
|
||||
&preview_path,
|
||||
&display_path,
|
||||
)
|
||||
})
|
||||
.await??;
|
||||
|
||||
|
||||
@@ -55,15 +55,23 @@ impl MediaTotalCache {
|
||||
/// quota path and the export preflight: a database blip must not turn into "every upload
|
||||
/// refused". The disk-space half of the gate still applies, so a failure here degrades the
|
||||
/// check to the old flat-reserve behaviour rather than disabling it.
|
||||
pub async fn get(&self, pool: &PgPool) -> i64 {
|
||||
pub async fn get(&self, pool: &PgPool, event_slug: &str) -> i64 {
|
||||
if let Some((bytes, at)) = *self.inner.read().unwrap()
|
||||
&& at.elapsed() < TTL
|
||||
{
|
||||
return bytes;
|
||||
}
|
||||
// Scoped to THIS event (H12). The unscoped `SUM(total_upload_bytes) FROM "user"` summed
|
||||
// every user row in the table, so reusing the install for a second event carried the first
|
||||
// one's bytes into the second one's keepsake-headroom gate — closing uploads early with a
|
||||
// message about "the event's storage" being full, counting media that belongs to a party
|
||||
// that already happened (and whose files are never reclaimed either).
|
||||
let queried = sqlx::query_scalar::<_, Option<i64>>(
|
||||
"SELECT SUM(total_upload_bytes)::bigint FROM \"user\"",
|
||||
"SELECT SUM(u.total_upload_bytes)::bigint FROM \"user\" u
|
||||
JOIN event e ON e.id = u.event_id
|
||||
WHERE e.slug = $1",
|
||||
)
|
||||
.bind(event_slug)
|
||||
.fetch_one(pool)
|
||||
.await;
|
||||
|
||||
|
||||
@@ -82,12 +82,7 @@ impl UploadAdmission {
|
||||
pub async fn reserve(&self, cap_bytes: usize) -> Option<OwnedSemaphorePermit> {
|
||||
let mib = cap_bytes.div_ceil(1024 * 1024).max(1);
|
||||
let want = u32::try_from(mib).unwrap_or(BUDGET_MIB).min(BUDGET_MIB);
|
||||
match tokio::time::timeout(
|
||||
WAIT,
|
||||
self.permits.clone().acquire_many_owned(want),
|
||||
)
|
||||
.await
|
||||
{
|
||||
match tokio::time::timeout(WAIT, self.permits.clone().acquire_many_owned(want)).await {
|
||||
Ok(Ok(permit)) => Some(permit),
|
||||
// The semaphore is never closed, so `Err` here is unreachable in practice; treat it
|
||||
// the same as a timeout rather than panicking on the upload path.
|
||||
@@ -124,12 +119,12 @@ mod tests {
|
||||
|
||||
// Nothing left: a second reservation must not be granted. Raced against a short timeout so
|
||||
// the test does not sit for the full WAIT.
|
||||
let blocked = tokio::time::timeout(
|
||||
Duration::from_millis(150),
|
||||
admission.reserve(1024 * 1024),
|
||||
)
|
||||
.await;
|
||||
assert!(blocked.is_err(), "budget exhausted, yet a reservation was granted");
|
||||
let blocked =
|
||||
tokio::time::timeout(Duration::from_millis(150), admission.reserve(1024 * 1024)).await;
|
||||
assert!(
|
||||
blocked.is_err(),
|
||||
"budget exhausted, yet a reservation was granted"
|
||||
);
|
||||
|
||||
// ...and releasing the permit makes room again, so the budget is not a one-way latch.
|
||||
drop(whole);
|
||||
|
||||
Reference in New Issue
Block a user