diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 7a2879e..3e11a81 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -10,6 +10,7 @@ use uuid::Uuid; use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::services::config; +use crate::services::sse_tickets::TicketKind; use crate::state::AppState; // ── DTOs ───────────────────────────────────────────────────────────────────── @@ -132,6 +133,16 @@ pub async fn patch_config( ("social_rate_per_min", true, 1.0, 100_000.0), ("quota_tolerance", false, 0.0, 1.0), ("estimated_guest_count", true, 1.0, 1_000_000.0), + // The three limiters migration 025 introduced. All are READ at runtime + // (`upload.rs` for the edit limiter, `auth/handlers.rs` for the other two) and 025 + // INSERTs all of them into `config`, so `GET /admin/config` listed them while + // `PATCH /admin/config` answered "Unbekannter Konfigurationsschlüssel" — the same + // dead-key defect the comment under BOOL_KEYS says was fixed for the two login + // toggles. These are precisely the knobs an operator reaches for while abuse is + // happening, which is the one moment a restart to change them is unaffordable. + ("upload_edit_rate_per_min", true, 1.0, 100_000.0), + ("recover_name_rate_per_15min", true, 1.0, 100_000.0), + ("pin_reset_ip_rate_per_min", true, 1.0, 100_000.0), ]; const BOOL_KEYS: &[&str] = &[ "rate_limits_enabled", @@ -145,6 +156,9 @@ pub async fn patch_config( "admin_login_rate_enabled", "recover_rate_enabled", "social_rate_enabled", + // Read by `upload::edit_upload`, inserted by migration 025, and until now unreachable + // from this endpoint — see the note in NUMERIC_SPECS. + "upload_edit_rate_enabled", "quota_enabled", "storage_quota_enabled", "upload_count_quota_enabled", @@ -397,7 +411,21 @@ pub async fn export_ticket( enforce_export_rate(&state, auth.user_id).await?; - let ticket = state.sse_tickets.issue(auth.token_hash); + // `issue` returns None when the ticket store is at capacity. Unwrapping it into the JSON body + // serialized `{"ticket": null}` with a 200 — so `api.post` resolved happily, the page toasted + // success, the iframe navigated to `?ticket=null`, and one of the guest's three DAILY + // downloads had already been charged above. That is precisely the phantom-success failure + // this endpoint's pre-validation was added to eliminate, arriving through the other door. + // 503 + Retry-After, matching how `sse::issue_ticket` answers the identical condition. + let ticket = state + .sse_tickets + .issue(auth.token_hash, TicketKind::Download) + .ok_or_else(|| { + AppError::ServiceUnavailable( + "Server ist gerade ausgelastet. Bitte versuch es in einem Moment erneut.".into(), + Some(30), + ) + })?; Ok(Json(serde_json::json!({ "ticket": ticket }))) } @@ -407,7 +435,7 @@ pub async fn export_ticket( async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result { let token_hash = state .sse_tickets - .consume(ticket) + .consume(ticket, TicketKind::Download) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) .await diff --git a/backend/src/handlers/sse.rs b/backend/src/handlers/sse.rs index 4e9de30..2a9347a 100644 --- a/backend/src/handlers/sse.rs +++ b/backend/src/handlers/sse.rs @@ -13,6 +13,7 @@ use tokio_stream::wrappers::errors::BroadcastStreamRecvError; use crate::auth::middleware::AuthUser; use crate::error::AppError; use crate::models::session::Session; +use crate::services::sse_tickets::TicketKind; use crate::state::AppState; #[derive(Deserialize)] @@ -52,7 +53,7 @@ pub async fn issue_ticket( )); } - let ticket = state.sse_tickets.issue(auth.token_hash).ok_or_else(|| { + let ticket = state.sse_tickets.issue(auth.token_hash, TicketKind::Sse).ok_or_else(|| { AppError::ServiceUnavailable( "Server ist gerade ausgelastet. Live-Updates folgen in Kürze.".into(), Some(30), @@ -75,7 +76,7 @@ pub async fn stream( ) -> Result>>, AppError> { let token_hash = state .sse_tickets - .consume(&q.ticket) + .consume(&q.ticket, TicketKind::Sse) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; // NOTE: this authenticates via ticket→session, not the `AuthUser` extractor. The diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 0bc8733..d79c483 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -774,13 +774,37 @@ pub async fn edit_upload( // REGEN_DEBOUNCE bounds the rate of that, not the total work, so the keepsake could be kept // permanently un-downloadable. // - // Residual, deliberately not fixed: re-sending an IDENTICAL hashtag list still counts as a - // change. Comparing would need another query, and unlike `PATCH {}` it is not a free loop. + // The hashtag half of that guard was wrong, and the comment here used to defend it: it said + // re-sending an identical list "is not a free loop". It is exactly a free loop. `PATCH + // {"hashtags": []}` carries no photo, no bytes and no client-side cost, yet it made + // `normalized_tags` `Some`, sailed past the caption-only check, and bumped `export_epoch` on + // every request — retiring the HTML keepsake instantly. REGEN_DEBOUNCE (20s) throttles when a + // rebuild may START, not the epoch bump, so at the 30/min this endpoint allows no rebuild ever + // gets a quiet window to finish in and `GET /export/html` 404s for the whole event. The ZIP is + // carried forward, so this denied exactly half the product. + // + // So compare properly. One indexed lookup against `upload_hashtag` is cheap next to the + // full-gallery rebuild a false positive arms. let caption_changed = match (&body.caption, &upload.caption) { (Some(new), existing) => Some(new.as_str()) != existing.as_deref(), (None, _) => false, }; - if !caption_changed && normalized_tags.is_none() { + let tags_changed = match &normalized_tags { + None => false, + Some(incoming) => { + // Compare on the same normalised form `upsert` keys on, so "#Party", "party" and + // " #PARTY " are all the same tag and none of them counts as an edit. + let mut want: Vec = incoming + .iter() + .map(|t| t.trim().trim_start_matches('#').to_lowercase()) + .collect(); + want.sort(); + want.dedup(); + let have = Hashtag::normalized_for_upload(&state.pool, upload_id).await?; + want != have + } + }; + if !caption_changed && !tags_changed { return Ok(StatusCode::OK); } diff --git a/backend/src/models/hashtag.rs b/backend/src/models/hashtag.rs index e9dde24..234feb6 100644 --- a/backend/src/models/hashtag.rs +++ b/backend/src/models/hashtag.rs @@ -63,6 +63,33 @@ impl Hashtag { .await?; Ok(()) } + + /// The upload's current tags, lowercased and sorted — the comparable form. + /// + /// Exists so `edit_upload` can tell a real hashtag change from a re-send of the same list. + /// Without it, `PATCH {"hashtags": []}` in a loop retired the HTML keepsake on every request + /// (readiness is derived from `event.export_epoch`), and every armed rebuild was superseded + /// before the debounce let it start — so the viewer 404'd for the rest of the event at zero + /// cost to the client. One indexed lookup on `upload_hashtag(upload_id)` is a fair price for + /// closing that. + pub async fn normalized_for_upload<'e, E>( + executor: E, + upload_id: Uuid, + ) -> Result, sqlx::Error> + where + E: sqlx::PgExecutor<'e>, + { + let rows: Vec<(String,)> = sqlx::query_as( + "SELECT lower(h.tag) FROM hashtag h + JOIN upload_hashtag uh ON uh.hashtag_id = h.id + WHERE uh.upload_id = $1 + ORDER BY lower(h.tag)", + ) + .bind(upload_id) + .fetch_all(executor) + .await?; + Ok(rows.into_iter().map(|(t,)| t).collect()) + } } /// Extract `#hashtags` from text (caption or body). Tags are restricted to diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index 15e9e44..ff036b1 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -691,7 +691,7 @@ async fn run_zip_export_inner( // Note what ISN'T here any more: the ready-flag flip. Readiness is derived from // (released AND job.epoch = event.epoch AND status = 'done'), so writing `done` at a live epoch // IS the publish, atomically. A worker at a dead epoch simply writes a row nobody can see. - if !finalize_job(pool, event_id, "zip", epoch, &format!("exports/{out_name}")).await { + if !finalize_job(pool, event_id, "zip", epoch, &format!("exports/{out_name}")).await? { let _ = tokio::fs::remove_file(&out_path).await; tracing::info!( "ZIP export for event {event_id} superseded (epoch {epoch} retired); discarded" @@ -1129,7 +1129,7 @@ async fn run_html_export_inner( epoch, &format!("exports/{out_name}"), ) - .await + .await? { let _ = tokio::fs::remove_file(&out_path).await; tracing::info!( @@ -1250,14 +1250,27 @@ async fn claim_job(pool: &PgPool, event_id: Uuid, export_type: &str, epoch: i64) /// AND status = 'done')`, so a single row write makes the keepsake downloadable — there is no /// second flag to flip and therefore no window between "done" and "visible". A worker whose epoch /// was retired matches nothing here and must discard its output. +/// +/// ERRORS ARE DISTINGUISHED FROM A LOST RACE, for the same reason `claim_job` distinguishes them — +/// and the consequence here is strictly worse. This used to end `.unwrap_or(false)`, collapsing a +/// pool timeout into "we were superseded". At that point the archive is already built, fsynced and +/// renamed into place, so the caller went on to DELETE the finished multi-GB file and return the +/// `Superseded` sentinel — which `abandon_if_superseded` swallows into `Ok(())`, so +/// `spawn_export_jobs` never called `mark_failed` either. The row stayed `running` at 99% at the +/// LIVE epoch, which the host dashboard renders as "Wird erstellt (99 %)" with the download +/// disabled, forever: no sweep re-examines `running` rows, and `recover_exports` runs only at boot. +/// +/// A pool timeout is not exotic here. `max_connections` is 10, `acquire_timeout` 5s, and this fires +/// at the end of a full-gallery export while ~100 guests are uploading. Returning `Err` instead +/// leaves the finished archive on disk and lets the caller's `mark_failed` record a real reason. async fn finalize_job( pool: &PgPool, event_id: Uuid, export_type: &str, epoch: i64, file_path: &str, -) -> bool { - sqlx::query( +) -> Result { + let r = sqlx::query( "UPDATE export_job SET status = 'done', progress_pct = 100, file_path = $3, completed_at = NOW() WHERE event_id = $1 AND type = $2::export_type @@ -1269,8 +1282,8 @@ async fn finalize_job( .bind(epoch) .execute(pool) .await - .map(|r| r.rows_affected() > 0) - .unwrap_or(false) + .context("finalizing export job")?; + Ok(r.rows_affected() > 0) } /// Parse the trailing generation number out of `` (e.g. diff --git a/backend/src/services/sse_tickets.rs b/backend/src/services/sse_tickets.rs index fd921c9..0e42bfc 100644 --- a/backend/src/services/sse_tickets.rs +++ b/backend/src/services/sse_tickets.rs @@ -23,6 +23,26 @@ const MAX_TICKETS: usize = 4096; /// EventSources concurrently; 4 absorbs that without letting a reconnect loop accumulate. const MAX_TICKETS_PER_SESSION: usize = 4; +/// What a ticket may be redeemed for. +/// +/// The store began life serving only SSE and stayed untyped when the export download started +/// reusing it, which silently made the two interchangeable. That is not a theoretical mixing +/// concern: `POST /stream/ticket` is rate-limited at 60/min per user and charges nothing, while +/// `POST /export/ticket` charges one of three PER-DAY downloads. An untyped ticket let any guest +/// mint at the cheap endpoint and redeem at the expensive one, so the daily export limit was +/// bypassable ~60×/minute — each redemption streaming the whole multi-GB keepsake, `no-store`, +/// off the same filesystem Postgres writes WAL to. +/// +/// `consume` therefore requires the kind to MATCH. A ticket is only ever valid for the thing it +/// was minted for. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum TicketKind { + /// Opens the SSE stream (`GET /stream`). Cheap, high volume. + Sse, + /// Downloads an export archive (`GET /export/{zip,html}`). Expensive, rate-limited per day. + Download, +} + #[derive(Clone)] pub struct SseTicketStore { inner: Arc>>, @@ -32,6 +52,7 @@ pub struct SseTicketStore { struct Entry { token_hash: String, issued_at: Instant, + kind: TicketKind, } impl SseTicketStore { @@ -55,7 +76,7 @@ impl SseTicketStore { /// Three bounds, because `issue` had none: no size cap, no per-caller cap, and no rate /// limit on the endpoint, while `prune` ran only hourly against a 30-second TTL. So any /// authenticated session could loop the endpoint and grow the map for an hour. - pub fn issue(&self, token_hash: String) -> Option { + pub fn issue(&self, token_hash: String, kind: TicketKind) -> Option { let ticket = random_ticket(); let mut map = self.inner.lock().unwrap(); @@ -95,20 +116,34 @@ impl SseTicketStore { Entry { token_hash, issued_at: Instant::now(), + kind, }, ); Some(ticket) } - /// Consume a ticket. Returns `Some(token_hash)` if the ticket exists and is - /// not expired. Single-use: the ticket is removed regardless of whether it - /// was still fresh, so a replay can't slip through after expiry. - pub fn consume(&self, ticket: &str) -> Option { + /// Consume a ticket minted for `kind`. Returns `Some(token_hash)` if the ticket exists, is + /// not expired, and was minted for this purpose. Single-use: the ticket is removed regardless + /// of whether it was still fresh, so a replay can't slip through after expiry. + /// + /// A ticket of the WRONG kind is also removed. It was a valid ticket the caller legitimately + /// held, so this is not punitive — but leaving it would let a redemption loop probe the store + /// without ever spending anything, and the client has no legitimate reason to present a + /// ticket at the wrong endpoint. + pub fn consume(&self, ticket: &str, kind: TicketKind) -> Option { let mut map = self.inner.lock().unwrap(); let entry = map.remove(ticket)?; if entry.issued_at.elapsed() > TTL { return None; } + if entry.kind != kind { + tracing::warn!( + expected = ?kind, + found = ?entry.kind, + "ticket presented at the wrong endpoint; rejected" + ); + return None; + } Some(entry.token_hash) } @@ -135,17 +170,54 @@ mod tests { /// `issue` now returns `Option`; in every test below the store is far from capacity, so an /// `expect` here documents that refusing is exceptional rather than routine. fn issue(store: &SseTicketStore, hash: &str) -> String { - store.issue(hash.into()).expect("store has capacity") + store + .issue(hash.into(), TicketKind::Sse) + .expect("store has capacity") + } + + /// The store is shared by two endpoints with wildly different costs: `/stream/ticket` is + /// 60/min per user and free, `/export/ticket` charges one of three PER-DAY downloads. While + /// entries were untyped, a ticket minted at the cheap endpoint opened the expensive one — so + /// the daily export limit could be bypassed ~60×/minute, each redemption streaming the whole + /// multi-GB keepsake off the disk Postgres writes WAL to. + /// + /// Asserted in BOTH directions so this cannot be "fixed" by a check that only guards one. + #[test] + fn a_ticket_is_only_valid_for_the_purpose_it_was_minted_for() { + let store = SseTicketStore::new(); + + let sse = store.issue("h".into(), TicketKind::Sse).unwrap(); + assert_eq!( + store.consume(&sse, TicketKind::Download), + None, + "an SSE ticket must not open the export download" + ); + + let dl = store.issue("h".into(), TicketKind::Download).unwrap(); + assert_eq!( + store.consume(&dl, TicketKind::Sse), + None, + "a download ticket must not open the SSE stream" + ); + + // And the matching cases still work, so the guard is not simply rejecting everything. + let sse = store.issue("h".into(), TicketKind::Sse).unwrap(); + assert_eq!(store.consume(&sse, TicketKind::Sse).as_deref(), Some("h")); + let dl = store.issue("h".into(), TicketKind::Download).unwrap(); + assert_eq!( + store.consume(&dl, TicketKind::Download).as_deref(), + Some("h") + ); } #[test] fn issue_then_consume_returns_the_hash_exactly_once() { let store = SseTicketStore::new(); let ticket = issue(&store, "hash-1"); - assert_eq!(store.consume(&ticket).as_deref(), Some("hash-1")); + assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("hash-1")); // Single-use: a replay of the same ticket is rejected. assert_eq!( - store.consume(&ticket), + store.consume(&ticket, TicketKind::Sse), None, "a consumed ticket must not be reusable" ); @@ -154,7 +226,7 @@ mod tests { #[test] fn unknown_ticket_consumes_to_none() { let store = SseTicketStore::new(); - assert_eq!(store.consume("never-issued"), None); + assert_eq!(store.consume("never-issued", TicketKind::Sse), None); } #[test] @@ -172,7 +244,7 @@ mod tests { let store = SseTicketStore::new(); let ticket = issue(&store, "h"); store.prune(); // not expired → kept - assert_eq!(store.consume(&ticket).as_deref(), Some("h")); + assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("h")); } /// Build an entry that is already past the TTL. @@ -180,6 +252,7 @@ mod tests { store.inner.lock().unwrap().insert( key.to_string(), Entry { + kind: TicketKind::Sse, token_hash: token_hash.into(), issued_at: Instant::now() .checked_sub(TTL + Duration::from_secs(1)) @@ -193,7 +266,7 @@ mod tests { let store = SseTicketStore::new(); insert_stale(&store, "stale-ticket", "h"); assert_eq!( - store.consume("stale-ticket"), + store.consume("stale-ticket", TicketKind::Sse), None, "an expired ticket must not authenticate" ); @@ -236,7 +309,7 @@ mod tests { "the newest ticket is the one the caller is about to use" ); assert_eq!( - store.consume(&stranger).as_deref(), + store.consume(&stranger, TicketKind::Sse).as_deref(), Some("other-session"), "another session's ticket must survive — evicting it would let one client deny \ SSE to the venue" @@ -254,6 +327,7 @@ mod tests { map.insert( format!("filler-{i}"), Entry { + kind: TicketKind::Sse, token_hash: format!("session-{i}"), issued_at: Instant::now(), }, @@ -261,7 +335,7 @@ mod tests { } } assert_eq!( - store.issue("newcomer".into()), + store.issue("newcomer".into(), TicketKind::Sse), None, "a full store must refuse, so the caller can answer 503" ); diff --git a/frontend/src/lib/upload-queue.test.ts b/frontend/src/lib/upload-queue.test.ts index ff6c11d..52fad20 100644 --- a/frontend/src/lib/upload-queue.test.ts +++ b/frontend/src/lib/upload-queue.test.ts @@ -203,14 +203,47 @@ describe('suspendedSinceLastTick', () => { expect(suspendedSinceLastTick(now + 60_000, now, 5_000)).toBe(0); }); - it('a suspension longer than the stall ceiling does not abort a healthy upload', () => { - // The bug, end to end: 3 minutes suspended, interval resumes, no bytes since. - const lastProgressAt = now - 180_000; - const credited = Math.min( - now, - lastProgressAt + suspendedSinceLastTick(now - 185_000, now, 5_000) - ); - expect(shouldAbortForStall(credited, now, false)).toBe(false); + /** + * Replays the production watchdog tick faithfully — including the MAX_SUSPEND_CREDIT_MS clamp. + * + * The previous version of the test below omitted that clamp, so it asserted a property the + * shipped code does not have and could not fail. Anything checking the suspension behaviour + * must go through here. + */ + function runTicks(lockMs: number, tickMs = 5_000, ticks = 3): boolean { + const CAP = 90_000; // MAX_SUSPEND_CREDIT_MS + let lastProgressAt = 0; + let lastTickAt = 0; + let creditSpent = 0; + let clock = lockMs; // first tick lands when the page resumes + for (let i = 0; i < ticks; i++) { + const credit = Math.min( + suspendedSinceLastTick(lastTickAt, clock, tickMs), + Math.max(0, CAP - creditSpent) + ); + creditSpent += credit; + lastProgressAt = Math.min(clock, lastProgressAt + credit); + lastTickAt = clock; + if (shouldAbortForStall(lastProgressAt, clock, false)) return true; + clock += tickMs; + } + return false; + } + + it('a pocket-length screen lock does not abort a healthy upload', () => { + // 60s locked, then the interval resumes: fully credited, nothing aborted. + expect(runTicks(60_000)).toBe(false); + }); + + it('a suspension beyond the credit cap DOES abort — the cap is the deliberate bound', () => { + // 3 minutes locked. The cap forgives 90s, so the first tick after resume survives and the + // next one aborts. This is intended: after a lock that long the socket is almost certainly + // reaped (iOS does so without firing `error`), and re-sending beats hanging on `xhr.timeout` + // for 5-60 minutes while the queue's latch is held. + // + // It is asserted rather than merely tolerated because the cost lands on the retry budget — + // see RETRY_BUDGET_WINDOW_MS, which is what keeps this from parking the photo for good. + expect(runTicks(180_000)).toBe(true); }); it('but a socket still silent 91s AFTER resume is aborted, never left to xhr.timeout', () => { diff --git a/frontend/src/lib/upload-queue.ts b/frontend/src/lib/upload-queue.ts index cfbac77..127b3d3 100644 --- a/frontend/src/lib/upload-queue.ts +++ b/frontend/src/lib/upload-queue.ts @@ -49,6 +49,20 @@ const MAX_QUEUE_ITEMS = 100; */ const MAX_AUTO_ATTEMPTS = 5; +/** + * Quiet time after which an item's automatic-retry budget refills. + * + * The cap above is a rate limiter, and a rate limiter needs a window or it is a lifetime quota. + * Five attempts on a 5/10/20/40s ladder is ~75 seconds, so ANY outage longer than that — a venue + * AP brownout, a captive portal re-arming, an `app` container restart — permanently parked every + * in-flight photo behind a per-row button three taps deep that no guest will find. + * + * 10 minutes is chosen against the thing being protected: the concern is a hot loop re-sending a + * 200 MB video over a shared uplink, and one re-send per item per 10 minutes is not that. It is + * also comfortably longer than every outage the queue can ride out on its own. + */ +const RETRY_BUDGET_WINDOW_MS = 10 * 60_000; + /** Exponential backoff between automatic attempts: 5s, 10s, 20s, 40s, … capped below. */ const RETRY_BASE_DELAY_MS = 5_000; const MAX_RETRY_DELAY_MS = 5 * 60_000; @@ -186,6 +200,21 @@ interface QueueEntry { attempts?: number; /** Earliest ms timestamp at which an automatic resume may re-send this item. */ nextAttemptAt?: number; + /** When the most recent attempt failed. Lets the budget refill after a quiet spell. */ + lastFailureAt?: number; + /** + * The guest stopped this transfer themselves (the ✕ on an in-flight row). + * + * `requeueRetriable` requeues any blob-bearing `error` item that is under budget and past its + * backoff — and a cancel deliberately charges NO attempt and sets NO backoff, so without this + * flag it matched on both counts and the upload restarted from byte zero within ~120s (an + * `online` event, or the SSE backstop's `feed-delta` poll). It then restarted forever, because + * a path that never charges an attempt can never exhaust the budget that would stop it. The row + * said "Abgebrochen. Tippe auf „Erneut“." the whole time, on a shared venue uplink. + * + * Cleared by `retryItem` — an explicit tap is the guest changing their mind. + */ + cancelled?: boolean; blob?: Blob; } @@ -273,10 +302,29 @@ async function requeueRetriable(options: { resetAttempts?: boolean } = {}): Prom let soonest: number | null = null; for (const entry of all) { if (entry.userId !== myUserId || entry.status !== 'error' || !entry.blob) continue; + // A cancel is the guest's decision, not a transient failure — never undo it automatically, + // not even on `resetAttempts` (the host reopening the event says nothing about whether + // this guest still wants this photo sent). Only `retryItem` clears it. + if (entry.cancelled) continue; if (options.resetAttempts) { entry.attempts = 0; entry.nextAttemptAt = undefined; } + // The budget is a RATE, not a lifetime allowance. + // + // Five attempts with a 5/10/20/40s ladder is ~75 seconds of failure end to end. A venue + // AP brownout, a re-armed captive portal or a backend restart lasting two minutes — with + // `navigator.onLine` still true the whole time, so none of it takes the offline path — + // therefore exhausted every automatic attempt and parked the item until the guest went + // FAB → sheet → "Warteschlange" → per-row "Erneut". Nobody does that; the photo simply + // never arrives. Refilling after a quiet spell keeps the bound that matters (no hot + // retry loop against a server that is genuinely down) while letting the evening recover + // from a blip on its own. + const lastFailureAt = entry.lastFailureAt ?? 0; + if (now - lastFailureAt > RETRY_BUDGET_WINDOW_MS) { + entry.attempts = 0; + entry.nextAttemptAt = undefined; + } if ((entry.attempts ?? 0) >= MAX_AUTO_ATTEMPTS) continue; if (entry.nextAttemptAt && entry.nextAttemptAt > now) { // Still cooling down — remember the earliest deadline so the sweep below can @@ -333,6 +381,7 @@ function scheduleRetrySweep(delayMs: number): void { function chargeAttempt(entry: QueueEntry): boolean { const attempts = (entry.attempts ?? 0) + 1; entry.attempts = attempts; + entry.lastFailureAt = Date.now(); if (attempts >= MAX_AUTO_ATTEMPTS) { entry.nextAttemptAt = undefined; return true; @@ -761,6 +810,9 @@ export async function retryItem(id: string): Promise { // so a guest who watched their photo fail five times can still get it sent right now. entry.attempts = 0; entry.nextAttemptAt = undefined; + // And it is the one thing that un-cancels: tapping "Erneut" on a row the guest stopped + // themselves is them changing their mind. + entry.cancelled = false; await storePut(entry); queueItems.update((items) => @@ -1179,7 +1231,10 @@ async function uploadItem(id: string): Promise { if (removedUploads.delete(id)) throw e; // The guest's own ✕. Park it retryable with the blob intact and spend no retry // budget — they asked for the transfer to stop, not for the photo to be dropped. + // `cancelled` is what keeps the automatic path from immediately undoing that; see the + // field's docstring. Without it the ✕ was purely cosmetic. entry.status = 'error'; + entry.cancelled = true; entry.error = 'Abgebrochen. Tippe auf „Erneut“.'; await storePut(entry); updateItemStatus(id, 'error', entry.error);