feat(artwork): synchronous resize pipeline + variant endpoint (phase 1.h.3) - #30
Merged
Conversation
…1.h.3)
Every successful upload now runs through `artwork_pipeline::generate_variants`
to produce two JPEG q85 resizes (thumb ≤ 128px, preview ≤ 480px),
hashed independently and stored alongside the original in
`metadata_artwork_variant` (parent → variant via `ON DELETE CASCADE`
so a future GC of the parent reclaims both in one transaction).
The pipeline applies a long-edge clamp before invoking
`DynamicImage::thumbnail` — that helper happily upscales when the
source is smaller than the bounding box, which is the wrong
semantic for cover art. A 64 × 48 source now passes through at
native size rather than ballooning to 128 × 96 fake pixels.
A new `GET /api/v1/artwork/{hash}/{variant}` route serves the
resized bytes with the same cache + ETag posture as the parent.
The bare `GET /api/v1/artwork/{hash}` also resolves variant hashes
through a fallback lookup on `metadata_artwork_variant.hash`, so a
client that persisted only the variant hash never needs to know
the parent.
Upload response gains a `variants[]` array (alphabetically ordered:
preview, thumb) so a client never needs a second round-trip to
discover what's available. Idempotent re-uploads echo the cached
set without re-running the resize.
Pipeline failures map to 400 (decode rejected the input) or 500
(our JPEG encoder bailed on otherwise valid input). The MIME header
is still validated up-front, so the decoder only sees bytes that
claim to be JPEG / PNG / WebP.
Tests: 8 new unit tests on the pipeline (variant kind round-trip,
clamp behaviour, distinct hashes per variant, no upscale of small
sources) + 5 new integration tests (variant endpoint round-trip,
unknown variant 400, missing parent 404, idempotent variant echo,
bare GET fallback on variant hash). 34/34 lib tests pass.
Sync execution is the right move for 1.h.3 — a JPEG resize is on
the order of tens of milliseconds for the 4 MiB ceiling we enforce
upstream, well within a typical upload's latency budget. 1.i.1 will
move the pipeline behind an apalis job so the upload responds
immediately and the variants stream in over the sync channel.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLe PR ajoute un pipeline synchrone générant deux variantes d'image (thumb, preview) à l'upload, stocke leurs métadonnées idempotemment en base, étend les endpoints GET pour servir variantes (par parent/variant ou par hash), et couvre le tout par tests unitaires et E2E. ChangesPipeline de génération de variantes redimensionnées
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/artwork.rs (1)
218-231:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGérer les échecs partiels de variantes pour éviter un cache “bloqué” incomplet.
Si une écriture échoue après insertion du parent (Line 275) mais avant la fin de toutes les variantes (Line 279+), un retry tombe sur le fast-path (Line 218) et renvoie un set partiel sans réparation. Résultat: état incohérent durable pour ce hash.
Recommandation: rendre l’écriture DB atomique (transaction parent + variantes) et/ou ajouter une logique de backfill quand
fetch_variants_for_parentne retourne pas l’ensemble attendu.As per coding guidelines:
Artwork POST /api/v1/artworkdoit rester idempotent et réutiliser un état stocké cohérent pour les réuploads.Also applies to: 279-300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/artwork.rs` around lines 218 - 231, The code can return a partial variant set when a parent row is inserted but some variant writes fail; fix by making the DB write atomic or by backfilling on read: wrap the parent+all variant inserts in a single transaction (the upload handler that currently writes parent then variants) so either all variants are persisted or none, and/or change the read fast-path (the code calling crate::db::artwork::fetch_meta and crate::db::artwork::fetch_variants_for_parent) to detect a missing/incomplete variant set and trigger a repair/backfill (re-run the failed variant inserts or roll back the parent) before returning the stored row. Reference the upload POST handler and the DB operations used here (fetch_meta, fetch_variants_for_parent and the parent/variant insert routines) and ensure idempotency and consistent stored state for reuploads.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/artwork.rs`:
- Around line 283-320: La réponse inclut les variantes dans l'ordre du pipeline
(non déterministe) — stabilise l'ordre en triant les variantes avant d'itérer :
trier le tableau `variants` (ou une copie) par `variant.kind.as_str()` en ordre
alphabétique puis effectuer le stockage, l'insertion DB
(`crate::db::artwork::insert_variant_if_absent`) et la construction de
`VariantResponse`; alternative acceptable : construire `variant_responses` puis
trier `variant_responses` par son champ `variant` avant de l'inclure dans
`UploadResponse` pour garantir un ordre stable (`preview` avant `thumb`).
---
Outside diff comments:
In `@src/api/artwork.rs`:
- Around line 218-231: The code can return a partial variant set when a parent
row is inserted but some variant writes fail; fix by making the DB write atomic
or by backfilling on read: wrap the parent+all variant inserts in a single
transaction (the upload handler that currently writes parent then variants) so
either all variants are persisted or none, and/or change the read fast-path (the
code calling crate::db::artwork::fetch_meta and
crate::db::artwork::fetch_variants_for_parent) to detect a missing/incomplete
variant set and trigger a repair/backfill (re-run the failed variant inserts or
roll back the parent) before returning the stored row. Reference the upload POST
handler and the DB operations used here (fetch_meta, fetch_variants_for_parent
and the parent/variant insert routines) and ensure idempotency and consistent
stored state for reuploads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c10b994-8c77-437d-a80e-e026c110ceb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (8)
CLAUDE.mdCargo.tomlmigrations/20260607000000_metadata_artwork_variant.sqlsrc/api/artwork.rssrc/artwork_pipeline.rssrc/db.rssrc/lib.rstests/artwork.rs
…ache Two issues raised by CodeRabbit + the CI failure on the alphabetical ordering assertion: 1. The upload handler returned variants in the pipeline's iteration order, which happened to match the alphabetical assertion locally but not on the CI runner. Sort `variants_response` by `variant.cmp(&other.variant)` on both code paths (fresh upload AND idempotent re-upload) so the wire order is stable and clients can pin `variants[0]` without reading the source of truth. 2. The idempotent fast path returned whatever was in `metadata_artwork_variant`, even if a previous upload had landed the parent row + parent bytes but lost a variant write to a backend / DB hiccup. The cache would then advertise a partial set forever. Added a self-heal pass: when the variant count is below `EXPECTED_VARIANT_COUNT`, re-run the pipeline against the freshly received body and write only the missing variants. The inserts go through `ON CONFLICT (parent_hash, variant) DO NOTHING` so a concurrent repair collapses cleanly, and the response re-fetches the post-repair set so it reflects the canonical row state. Test coverage: `re_upload_self_heals_a_missing_variant` deletes one variant row out from under the cache, re-uploads the same bytes, and asserts the response carries both variants AND the table is back to the full set. The two CI-failing assertions (`upload_then_public_get_round_trips`, `second_upload_of_same_bytes_is_idempotent`) now hold because the handler sorts before responding. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai deux corrections en un round : 1. Wire order stable (issue + CI fail) — l'assertion alphabetical sur 2. Self-heal sur idempotent path (outside-diff) — addressed. Quand Test Push: d8534d6. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the current state of the artwork handler and the test file on the PR branch
echo "=== src/api/artwork.rs ==="
cat src/api/artwork.rs
echo ""
echo "=== tests/artwork.rs (new test) ==="
grep -n "self_heal\|EXPECTED_VARIANT_COUNT\|sort\|repair\|re_upload\|fetch_variants_for_parent" tests/artwork.rs | head -80
echo ""
echo "=== src/artwork_pipeline.rs (EXPECTED_VARIANT_COUNT) ==="
grep -n "EXPECTED_VARIANT_COUNT\|pub const\|VariantKind\|fn generate_variants" src/artwork_pipeline.rs | head -30Length of output: 29275
Fix 1 — Sort stable ✅
Fix 2 — Self-heal ✅
Un point à surveiller pour la suite : 🧠 Learnings used |
4 tasks
InstaZDLL
added a commit
that referenced
this pull request
Jun 6, 2026
CodeRabbit flagged a `usize → i64` cast in run_once that could wrap for a hostile `WAVEFLOW_ARTWORK_SCANNER_BATCH_SIZE` value above `i64::MAX`. The hot path passes the value straight to a SQL `LIMIT` binding (sqlx serialises as i64), so a wrap would either error inside Postgres or skew the query semantics. Clamp at boot via `.min(i64::MAX as usize)` so the hot loop stays a plain `as i64` cast. Defaulted value (50) is unaffected; only a deliberately-set astronomical env value triggers the clamp. Skipping the second finding (rename migration to today's date, 2026-06-06): PR #30 already merged `20260607000000_metadata_artwork_variant.sql`, so renaming this file's prefix to `20260606xxxxxx` would put it BEFORE the previously-applied migration in sqlx's ordering. The "future date" pattern is already established in the repo (the 20260607 itself); enforcing today's wall-clock here would only be cosmetic and re-checksum a row that nobody's applied yet anyway. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sprint 1 / Phase 1.h.3 — every upload now produces two resize variants (
thumb≤ 128px,preview≤ 480px, JPEG q85) alongside the byte-perfect original. New public endpointGET /api/v1/artwork/{hash}/{variant}serves the resized bytes; the bareGET /api/v1/artwork/{hash}also resolves variant hashes via a fallback lookup so a client that persisted only the variant hash can hit the same route.Highlights
DynamicImage::thumbnailwould happily upscale a 64×48 source to 128×96 fake pixels — the pipeline checkssource.long_edge() ≤ targetand passes the image through at native size when no shrink is needed.metadata_artwork_variant) withFK ON DELETE CASCADEso a future GC sweep onmetadata_artworkreclaims variants in the same transaction. PK on(parent_hash, variant); secondary index on the variant's ownhashfor the bare-GET fallback.UploadResponse.variants[]alphabetically ordered (preview,thumb) so a client never needs a second round-trip to discover what's available.ON CONFLICT (parent_hash, variant) DO NOTHING) — two concurrent uploads of the same parent collapse to one variant row.EmptySource/ZeroDimension/Decode→ 400,Encode→ 500.Wire contract
All GETs ship
Cache-Control: public, max-age=31536000, immutable+ ETag ="<hash>".Sync vs async
Resize runs synchronously on the upload thread in 1.h.3 — a JPEG resize is on the order of tens of milliseconds for the 4 MiB upstream cap, well within a normal upload's latency budget. 1.i.1 will move it behind an apalis Postgres job so the POST responds immediately and the variants stream in over the sync channel.
Test plan
cargo fmt --all --check✅cargo clippy --all-targets --all-features -- -D warnings✅cargo test --lib(34/34 pass; 8 new pipeline unit tests + the existing 26 stay green)cargo testintegration suite — needs reachable Postgres; CI will run it (5 new integration tests: variant round-trip, unknown variant 400, missing parent 404, idempotent variant echo, bare GET fallback)Files
src/artwork_pipeline.rs(resize + JPEG q85 encode + BLAKE3)migrations/20260607000000_metadata_artwork_variant.sqlimage = { default-features = false, features = ["jpeg", "png", "webp"] }Config/AppStateshape unchanged.Follow-up
apalisPostgres-backed job queue, move pipeline async. Per the post-1.g sprint plan (validated 2026-06-05).Summary by CodeRabbit
New Features
Database
Tests
Chores