fix: close what nine adversarial reviews found, most of it mine
Some checks failed
Checks / Backend — cargo test + clippy + fmt (push) Failing after 1m5s
Checks / Frontend — vitest + svelte-check (push) Failing after 5m55s
Checks / E2E — typecheck + lint (push) Failing after 49s
E2E / Playwright E2E (chromium + webkit) (push) Failing after 10m42s
E2E / Cross-UA smoke matrix (push) Failing after 7m57s
Audit / cargo audit (backend) (push) Failing after 11m12s
Audit / npm audit (frontend) (push) Successful in 44s
Some checks failed
Checks / Backend — cargo test + clippy + fmt (push) Failing after 1m5s
Checks / Frontend — vitest + svelte-check (push) Failing after 5m55s
Checks / E2E — typecheck + lint (push) Failing after 49s
E2E / Playwright E2E (chromium + webkit) (push) Failing after 10m42s
E2E / Cross-UA smoke matrix (push) Failing after 7m57s
Audit / cargo audit (backend) (push) Failing after 11m12s
Audit / npm audit (frontend) (push) Successful in 44s
Nine focused reviews (export state machine, upload path, auth/abuse, client queue, guest UI, database, deploy/ops, regression hunt, test honesty). Every finding below was re-verified against the code before being acted on; several plausible-sounding ones were checked and rejected. ## Data loss and denial of service **One request could OOM-kill the app container.** `client_upload_id` was read with `Field::text()` — axum builds its multipart reader with no SizeLimit, so the only bound was the route's 576 MiB body limit, then decoded into a second full String. `caption` and `hashtags` go through `read_text_field_bounded` for exactly this reason; this field arrived later and missed it. Any guest, one request, and every SSE stream drops and every in-flight temp file is stranded. **Nothing bounded concurrent upload bodies.** The headroom gate can only refuse to COMMIT — the body is already streamed to a temp file by the time it runs, and neither axum, the tower stack nor Caddy limits how many stream at once. ~100 guests tapping "upload all" after the ceremony puts 10-20 GB of .tmp on a 40 GB volume, invisible to the gate, eating the reserve that keeps Postgres able to write WAL. New `UploadAdmission` budgets bytes (not requests, so one video and two hundred photos coexist) via a permit that releases on drop, so every exit path returns it. **The export decode bypassed the memory permit the compression path takes.** Same class of work — decode + resize every image in the gallery — in a bare spawn_blocking. A release fired while the last photos were still compressing put both in the same 1 GiB cgroup; the OOM kill marks the export failed and `recover_exports` re-spawns it into the same conditions on the next boot. The permit is now process-wide in `imaging`, because the constraint it expresses is the container's memory, not one worker's. **`MediaTotalCache` cached its own failure as 0.** For the whole TTL the gate then saw an empty event and collapsed to the flat reserve — the behaviour the two-halves design replaced — with no log line. And the trigger correlates with the danger: with max_connections 10 the query fails exactly during a burst. Now falls back to the last good reading and says so. **V8's heap ceiling sat above the frontend container's entire budget** (measured: 259 MB inside a 256M limit), so GC could never intervene and the only backpressure was SIGKILL under an arrival burst. ## Guest-visible **The feed stopped being newest-first after the first reconcile.** It fetches whole 100-item server pages while `uploads` grows in 20s, so everything in the gap was absent from `present`, classified as new, and prepended — ~80 photos from earlier in the evening above the newest ones. It also stalled infinite scroll, since the cursor still pointed at item 20 and the observer only re-fires on a change. The union is now sorted on the server's own (created_at, id) key, which additionally places an SSE arrival correctly. **A stale `loadMoreError` outlived every refresh and filter change**, leaving a false error above a button that returns immediately on `!nextCursor`. **A failed derivative toasted "Ein Upload konnte nicht verarbeitet werden."** for a photo sitting right there on screen — the handler still assumed 1d9fb11's pre-fix behaviour (row deleted, quota refunded, card evicted), none of which is true any more. It was the last surviving route for the "your photo is gone" signal that fix set out to remove. ## Enforcement that existed only in comments `recover_name_rate_per_15min` is clamped at the point of use: the ordering `3 x ceiling <= PIN_LOCK_THRESHOLD` is the whole control against one source locking any guest whose name is on the feed, it was asserted in a comment, and `patch_config` accepted 1..100_000. The test pinned the default constant rather than the enforced bound; it now pins the bound. ## Tests that could not fail - The gate test asserted only its own premise (`500MB x 100 > 35GB`) and never touched the gate. It now checks both controls against the same state and requires them to disagree in the right direction. - `the_banner_always_fires_before_the_upload_gate_closes` reduced to `G < G + G/4` — true for any margin, including zero, so it could not detect the banner moving to exactly the gate. It now pins the gap. - `disk_is_low`'s `free < LOW_DISK_FLOOR_BYTES` clause was unreachable (warn_at is always >= 12.5 GB against a 10 GB floor). Two tests were named after it and neither could fail if it were deleted. Clause and constant removed. - The suspension test I added last commit hard-coded the credit cap instead of importing it, so changing STALL_TIMEOUT_MS would leave it passing against a system that no longer exists. Now imports MAX_SUSPEND_CREDIT_MS. ## Stale comments corrected The prune doc still argued at length for the pre-build ordering that1d9fb11reversed — a reader trusting it would reopen the blocker0506369fixed. DISK_RESERVE_BYTES claimed to equal the banner threshold that0506369deliberately offset by 25%. And host.rs kept its own duplicate 10 GB literal instead of importing the constant. 154/154 backend, 59/59 vitest, clippy clean, svelte-check 0 errors, eslint clean, both builds, compose + caddy validate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -522,6 +522,21 @@ async fn ensure_export_space_reclaiming(
|
||||
ensure_export_space(pool, event_id, export_path).await
|
||||
}
|
||||
|
||||
/// Take the process-wide heavy-image permit if this file is big enough to need it.
|
||||
///
|
||||
/// Mirrors the compression worker's gate exactly (a header probe, no pixels decoded), so the two
|
||||
/// producers of heavy image work agree on what "heavy" means and serialise against each other
|
||||
/// rather than each against itself.
|
||||
async fn heavy_permit_for(path: &Path) -> Option<tokio::sync::SemaphorePermit<'static>> {
|
||||
let estimate = crate::services::imaging::estimated_processing_peak_bytes(path, 2048);
|
||||
match estimate {
|
||||
Some(bytes) if bytes > crate::services::imaging::HEAVY_IMAGE_BYTES => {
|
||||
crate::services::imaging::HEAVY_IMAGE_PERMITS.acquire().await.ok()
|
||||
}
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
// ── ZIP export ───────────────────────────────────────────────────────────────
|
||||
|
||||
async fn run_zip_export(
|
||||
@@ -888,6 +903,12 @@ async fn run_html_export_inner(
|
||||
let thumb_path = media_tmp.join(&thumb);
|
||||
let thumb_path_clone = thumb_path.clone();
|
||||
|
||||
// Same process-wide memory permit the compression worker takes. Without it, a
|
||||
// release fired while the last phone photos were still compressing put an export
|
||||
// decode and a heavy compression job in the same 1 GiB cgroup — and the OOM kill
|
||||
// marks the export failed, which `recover_exports` then re-spawns into the same
|
||||
// conditions on the next boot.
|
||||
let _heavy = heavy_permit_for(&src).await;
|
||||
let thumb_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
||||
// `decode_oriented`, not `image::open`: the latter ignores the EXIF
|
||||
// orientation tag AND applies no decode limits. Using it here is why every
|
||||
@@ -922,6 +943,9 @@ async fn run_html_export_inner(
|
||||
let full_path = media_tmp.join(&full);
|
||||
let full_path_clone = full_path.clone();
|
||||
|
||||
// See the thumbnail above. This branch is the more expensive of the two: it
|
||||
// only runs for originals over 5 MB, i.e. exactly the giants.
|
||||
let _heavy = heavy_permit_for(&src).await;
|
||||
let compress_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
||||
// Same reason as the thumbnail above. This branch only runs for originals
|
||||
// over 5 MB, which is why the viewer's full image looked correct for small
|
||||
@@ -1315,16 +1339,19 @@ async fn protected_files(pool: &PgPool, event_id: Uuid) -> Vec<String> {
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Reclaim superseded FINAL archives BEFORE this generation starts writing its own.
|
||||
/// Reclaim superseded FINAL archives.
|
||||
///
|
||||
/// Peak disk usage used to be two full generations, because the only prune ran after the new archive
|
||||
/// was written, renamed and finalised. That ordering reads as durability ("don't delete the good
|
||||
/// keepsake before the replacement is safe") but it buys nothing: readiness is derived from
|
||||
/// `job.epoch = event.export_epoch AND status = 'done'`, so the moment `invalidate_and_arm` bumps
|
||||
/// the epoch the old archive is ALREADY unreachable — `GET /export/zip` 404s whether the file is on
|
||||
/// disk or not. Keeping it only reserves gigabytes for a download nobody can perform, and for
|
||||
/// `Affects::Both` (a takedown) it is content someone has explicitly asked to have removed. So a
|
||||
/// rebuild reclaims first and peaks at one generation.
|
||||
/// CALLED AFTER A SUCCESSFUL BUILD, not before one. This doc used to argue the opposite at
|
||||
/// length — that since readiness is derived from `job.epoch = event.export_epoch`, a superseded
|
||||
/// archive is already unreachable and keeping it "buys nothing". That reasoning is right about
|
||||
/// REACHABILITY and wrong about RECOVERABILITY: an epoch is a database value that can be rolled
|
||||
/// back, deleted bytes cannot. Pruning first meant any rebuild that then failed — ENOSPC, an OOM,
|
||||
/// a hung ffmpeg — left the event with NO archive at all, which is the one outcome the product
|
||||
/// exists to prevent, at the one moment nobody is watching.
|
||||
///
|
||||
/// The single exception is phase 2 of `ensure_export_space_reclaiming`, where the previous
|
||||
/// generation's bytes are the only way the rebuild can fit at all. Both call sites carry the full
|
||||
/// reasoning; do not "restore" a pre-build prune on the strength of this function's convenience.
|
||||
///
|
||||
/// Narrower than [`prune_stale_export_files`] on purpose: FINAL archives only. Those are inert — a
|
||||
/// superseded worker either already renamed its file (and will delete it itself when its guarded
|
||||
|
||||
Reference in New Issue
Block a user