feat/upload-hygiene #91
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/upload-hygiene"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Three deferred items from Phase 13 land as one coherent upload-hygiene phase:
360px-max-dimension preview at
{sha256}_preview.{ext}adjacent to the original. Theinline
<img>in the message bubble loads?size=preview; click-through opens theoriginal.
imagecrate IS the strip:decode drops EXIF / XMP / IPTC / PNG text chunks, and the encoders write none back. JPEG
re-encodes at q=92; PNG / WebP / GIF are lossless. Animated GIFs keep their animation
in the stored original via multi-frame re-encode and get a first-frame static preview.
file_uploadsrows wheremessage_id IS NULLandcreated_at < 24 hours ago. Dedup-aware: the on-disk file isremoved only when no other row references the same
storage_path. The sibling-count,file delete, and row delete all run inside one
BEGIN IMMEDIATESQLite transaction so aconcurrent upload cannot race the sweep into deleting a still-referenced file.
New surface
GET /api/files/:id?size=preview— content-type derived from the preview file'sextension, not the DB row, so a future format where preview-mime ≠ original-mime cannot
silently serve the wrong header.
POST /admin/uploads/regenerate-thumbnails— preview-only backfill. Originals are nottouched (pre-Phase-23 originals are content-addressed by their un-stripped bytes;
re-encoding would change their sha and break the DB → disk mapping; see Out of scope).
POST /admin/uploads/purge-orphans— runs the samerun_orphan_sweepas the hourlytick with threshold = 0. No parallel "skip the dedup check" path.
/admin/settings: on-disk MiB total, orphan row count, the twoaction buttons. One inline help line under Regenerate to prevent the
operator-clicks-twice failure mode.
Architecture
server/src/uploads/(pipeline, sweep, lift-from-routes helpers).image = "0.25"(narrow features:jpeg,png,gif,webp).Dev-dep
kamadak-exif = "0.5"for "EXIF actually stripped" assertions in tests.THUMBNAIL_CONCURRENCY = 4semaphore singleton, modeled onPUSH_FANOUT_CONCURRENCY.Caps memory pressure across uploads + regen.
.partialsibling + rename so a crash mid-write never leaves ahalf-written content-addressed file.
0012_uploads.sqlis exactly what thesweeper needs.
Operator notes
THRESHOLD_HOURSinspawn_orphan_sweeper).BEGIN IMMEDIATE+ sibling-count + filedelete + row delete sequence is documented at the module head as load-bearing under
concurrency. If anyone moves the sweep to a sidecar, the transaction stays correct
because of
BEGIN IMMEDIATE; document is there to prevent accidental relaxation.a decode failure on a sniffed-image upload rejects with 400 (re-encode IS the strip; if
we can't strip, we don't store).
Tests
25 tests across 4 binaries, all passing when run individually:
server/tests/uploads_pipeline.rs(6): JPEG-with-EXIF → stripped (verified viakamadak-exif), 1200x800 → 360px preview, 3-frame GIF preserves frames in original /
static in preview, decode failure, unsupported mime,
preview_from_path.server/tests/uploads_sweep.rs(6): centerpiece dedup+GC test (A linked + Borphan share storage_path → B's row gone, file preserved for A),
original-present-preview-absent (pins the "preview write failed, original committed"
recovery path), plus the four standard sweep invariants.
server/tests/routes_uploads.rs(+2):?size=previewreturns smaller decoded bytes;dedup upload heals a missing preview end-to-end through HTTP.
server/tests/admin_uploads.rs(4, new): purge happy path, regenerate happy path,anon → 303 to /login, non-admin → 403.
Drive-by fixture fixes (NOT phase 23 feature work)
Two small fixes to test infrastructure surfaced while wiring up the new tests. Both were
latent failures on
main:server/tests/routes_uploads.rschat-pool migration list stopped at0019;insert_message_quotedneeds thequote_idcolumn from0020. Added0020_quote_reply.sqland0021_enclave_invitations_enclave_idx.sql. Unblocks thepre-existing
send_message_with_attachment_renders_inline_imagefailure that washappening on
mainbefore any of this work.TINY_PNGbyte literal is rejected byimage::ImageReader::decodein image 0.25. Replaced with one generated by
PngEncoderat first use. Thebytes-equality assertion in the round-trip test is relaxed to semantic round-trip
(decode-and-check-dimensions) since Phase 23 re-encodes on upload.
Known pre-existing test failures (NOT introduced by this PR)
server/tests/email_digest_dispatch.rs(and a few others) fail to compile because theyconstruct
AppStatewithout thebgfield added in a later phase. This is unrelatedAppState-shape drift, surfaced when running the full test binary set. Reviewers running
cargo testwill see these failures; they exist onmainwithout this PR and should beaddressed separately as test debt.
Out of scope
preview-only by design: pre-Phase-23 originals are content-addressed by un-stripped
bytes; re-encoding now would change their sha and break the DB → disk mapping. A
separate operator action that re-hashes and migrates rows could fill this gap.
Test plan
exiftoolshows no GPS /camera fields.
tab); click opens the original.
animated original.
/admin/settings: Uploads panel shows MiB total + orphan count.Purge orphans now: count drops; rerun loads with?purged=Nflash.Regenerate thumbnails: delete a_previewfile via shell first, thenclick; file reappears.
🤖 Generated with Claude Code
Task 8 verification status
sweep.rs); committed as
b959eee. Remaining 22 clippy warnings are pre-existing in othercode.
missing bg field). All 25 new tests pass individually in their own binaries — verified
by running each test binary in isolation.
~32s, exceeding the recipe's hardcoded 30s health-check window. Verified manually that
the binary builds, server starts, and /login returns HTTP 200 with a .
included in the PR body for you to walk.
POST /api/upload now branches on the sniffed MIME after the existing `infer` allowlist check. Image MIMEs (jpeg/png/gif/webp) go through the new pipeline: acquire a permit from the THUMBNAIL_CONCURRENCY=4 semaphore, run `pipeline::process_image` on `tokio::task::spawn_blocking` (it is CPU- and sync-IO-bound), and on success write the stripped re-encoded original to `{sha256}.{ext}` and the preview to `{sha256}_preview.{ext}` via `write_atomic` (`.partial` sibling + rename). PDFs continue through the unchanged hash-the-temp-file + rename path. Decode failures map to a 400 with `"image could not be decoded"` (re-encode IS the strip; if we cannot strip, we do not store). Other pipeline errors map to 500. The sha256 is computed over the STRIPPED bytes so two users uploading the same photo with different camera-of-origin metadata dedup correctly. On a dedup hit the original is not rewritten; a missing preview is healed in-place so a prior failed preview write does not leave that row preview-less forever. The DB row's `mime_type` and `size_bytes` are taken from `ProcessedImage` (the pipeline output), not the inferred multipart MIME or the raw byte count. For static formats this is observably identical; surfacing the pipeline values keeps the contract clean and makes the source-of-truth obvious to a future reader. Three private helpers live in this file for now: `sha256_bytes`, `preview_storage_name`, `write_atomic`. `preview_storage_name` will lift to `crate::uploads` when Task 4 (the serve route) adds the second caller. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>GET /api/files/:id now reads an optional `?size=preview` query parameter. When set and the row is an image MIME, the handler resolves the preview path via `crate::uploads::preview_storage_name`, checks the preview file exists on disk, and serves it. Missing preview falls through to the original. The preview response's Content-Type is derived from `crate::uploads::mime_for_storage_path(&preview_name)` (extension → MIME mapping), NOT from the DB row's `mime_type`. For every supported format today the two are observably identical; this structural separation future-proofs against a format where preview-mime ≠ original-mime (e.g. HEIC → JPEG transcode), which would otherwise quietly serve the preview with the wrong header. If the mapping returns None for a supported-image preview (it should not for any path we ever write), a warn is logged and the row mime is used as a safety net. Content-Length for the preview is the on-disk size from the metadata stat we already perform for the existence check, so the response advertises the actual bytes being sent rather than the original's row.size_bytes. `preview_storage_name` was a private helper in routes/uploads.rs; lifted to crate::uploads now that the serve route is the second caller. `mime_for_storage_path` is new in the same module so the path/MIME helpers live together. Template: image attachments render `<img src="{{ a.url }}?size=preview">` inside the existing `<a href="{{ a.url }}">` link, so the inline render fetches the smaller preview while click-through opens the original. PDF/file branch unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>`uploads::sweep::run_orphan_sweep(state, threshold_hours)` selects rows from `file_uploads` where `message_id IS NULL` and `created_at < datetime('now', '-N hours')`, then processes each candidate inside its own SQLite transaction. Each per-row transaction opens with `BEGIN IMMEDIATE TRANSACTION`, which acquires SQLite's write lock immediately. The sibling-count (`SELECT COUNT(*) ... WHERE storage_path = ? AND id <> ?`), the filesystem delete of the original and preview files, and the row delete all run inside that critical section, in that order. The IMMEDIATE keyword is load-bearing: with deferred BEGIN, a concurrent upload could insert a sibling row between our COUNT and our DELETE and we would delete a still-referenced file. IMMEDIATE blocks that upload at its INSERT until we commit. The file delete happens BEFORE the row delete so a file-delete failure rolls back both for the next sweep to retry; today the scheduler is single-process, but the transaction makes a future sidecar correct under concurrency. `remove_if_exists` treats `NotFound` as success and propagates everything else. The missing-preview-but-present-original case (the Task 3 "preview write failed, original committed" recovery path) goes through this branch cleanly. `spawn_orphan_sweeper` in main.rs models on `spawn_digest_sender`: hourly `tokio::time::interval`, skip the immediate fire, log-and-continue on error, info-log only when something happened (`rows_deleted > 0 || errors > 0`) so idle deployments stay quiet. Threshold is 24 hours. DB helpers added to db/uploads.rs: `select_orphans_older_than` (batch read), `count_uploads_sharing_path` and `delete_upload_row` (both take `&mut SqliteConnection` so the sweeper can share its transaction handle). Plan amended in Task 7's test list with one bullet for the original-present-preview-absent case to pin the recovery path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>