feat(artwork): background self-heal scanner (phase 1.i.1) - #31
Conversation
The 1.h.3 upload handler heals partial variant caches on the re-upload path, but only when a client happens to re-POST the same bytes. A partial cache nobody re-uploads stays partial forever and the public read endpoints keep advertising an incomplete `variants[]` to every reader. Spawn a tokio task at boot — same shape as the sync compaction loop — that periodically scans `metadata_artwork` for parents whose variant row count is below `EXPECTED_VARIANT_COUNT`, fetches the source bytes from object_store, re-runs the resize pipeline, and inserts only the variants still missing. The repair writes go through `ON CONFLICT (parent_hash, variant) DO NOTHING` so a concurrent upload-side repair (or a peer scanner in a multi- replica deploy) collapses cleanly. Configurable via env: `WAVEFLOW_ARTWORK_SCANNER_DISABLED` to opt out entirely, plus `_INTERVAL_SECS` (default 300, floor 1) and `_BATCH_SIZE` (default 50, floor 1) for the cadence + batch knobs. Floors are applied at boot so a misconfigured 0 doesn't busy-loop the worker. The scanner is only resolved when an artwork backend is also configured — a scanner with no storage would have nothing to do. We deliberately picked a tokio polling loop over `apalis` for 1.i.1: the workload is "periodic catch-up", not "queue-driven retries with priorities", and the surrounding infra already has a compaction loop to generalise from. The post-1.g sprint plan explicitly lists this as one of the valid candidates (`apalis`, `tokio-cron-scheduler`, or rolling-our-own). `apalis` lands when a job type genuinely needs persistent queues + priorities (e.g. RFC-004 community moderation). Tests: - `run_once_repairs_a_partial_cache` seeds a parent row + parent bytes + one variant, calls `run_once`, asserts both variants present afterwards with the missing one inserted. - `run_once_is_a_noop_when_cache_is_complete` confirms a clean cycle reports 0 repairs. - `run_once_skips_parents_with_missing_bytes` asserts a parent with a phantom hash (no bytes in storage) doesn't abort the cycle, leaving the broken row for the next pass. 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 (1)
📝 WalkthroughWalkthroughCette PR ajoute un scanner Tokio de fond configurable via variables d'environnement qui, périodiquement, liste les parents d'artwork partiels, récupère leurs bytes, relance le pipeline de génération de variantes et insère idempotent les variantes manquantes. ChangesScanner d'artwork auto-réparant
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/db.rs`:
- Around line 655-656: La requête de sélection "ORDER BY a.created_at ASC LIMIT
..." fait revenir sans cesse les mêmes parents irrécupérables en tête et peut
provoquer la famine; changez la stratégie dans src/db.rs et la logique de
run_once (src/artwork_jobs.rs) pour ajouter et utiliser une métrique de
retry/backoff persistée (par ex. colonnes retry_count et next_retry_at ou
last_failed_at) : à l'échec incrémentez retry_count et mettez next_retry_at =
now() + backoff(retry_count) dans la transaction d'échec, et modifiez la requête
de sélection pour trier par next_retry_at ASC (ou COALESCE(next_retry_at,
a.created_at)) au lieu de created_at seul afin d'éviter de resservir
indéfiniment les mêmes échecs; ainsi run_once lira parents dont next_retry_at
est atteint et les parents irrécupérables restent pénalisés jusqu'à expiration
du backoff.
In `@tests/artwork_scanner.rs`:
- Around line 37-154: Add an extra test that mirrors the partial-cache happy
path but starts with a parent row and zero metadata_artwork_variant rows while
the parent bytes are present in storage: create a tempdir-backed ArtworkStorage
via ArtworkStorage::local, write the source bytes (use synth_jpeg and
blake3::hash to compute parent_hash), insert the metadata_artwork row, ensure no
variant rows exist for parent_hash, call artwork_jobs::run_once(&pool, &storage,
50) and assert it returns 1 (or 2 if the scanner reports both variants) and that
both 'thumb' and 'preview' rows now exist in metadata_artwork_variant; reference
the existing tests run_once_repairs_a_partial_cache and
run_once_skips_parents_with_missing_bytes for structure and reuse the same query
patterns against metadata_artwork and metadata_artwork_variant to validate pre-
and post-conditions.
🪄 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: dd4cea07-49a1-412e-81c8-996f28a149f3
📒 Files selected for processing (9)
.env.exampleCLAUDE.mdsrc/artwork_jobs.rssrc/config.rssrc/db.rssrc/lib.rssrc/main.rstests/artwork_scanner.rstests/support.rs
Three updates in one round: 1. Starvation guard on the scanner (CR finding). Without it, an irrecoverable parent (e.g. one whose source bytes were lost from object_store) sat at the head of `ORDER BY created_at ASC` every cycle and dominated the batch. Add a `last_repair_failure_at` column on `metadata_artwork`, stamp it on every failed repair, and order the scanner query by `last_repair_failure_at NULLS FIRST, created_at ASC` with a 1-hour backoff filter. Untouched parents always lead; freshly-failed ones recede until the cooldown expires. Permanently broken rows stop blocking the queue. 2. New integration test `run_once_repairs_parent_with_no_variants` for the "parent landed but zero variants made it" edge case (CR finding). Mirrors the existing single-variant test structure; asserts the scanner regenerates the full set from scratch. 3. Extended `run_once_skips_parents_with_missing_bytes` to assert the backoff stamp lands on the failed row AND that a follow-up `list_partial_parents` call doesn't re-surface the same hash inside the 1-hour window — the property the starvation guard is supposed to deliver. Bumps that fit cleanly without an API audit: - uuid 1.23.1 → 1.23.2 (patch, error message tweaks) - tokio-tungstenite 0.27 → 0.29 (dev-dep, sync WS tests only) Skipped (production-critical, deserve dedicated dependabot PRs): - rand 0.8 → 0.10 (Alphanumeric::sample_string API may have moved) - hmac 0.12 → 0.13 (stream_token signing) - sha2 0.10 → 0.11 (stream_token + sha-256 hashing) Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
@coderabbitai trois corrections pushées (c1e9666) : 1. Starvation guard (finding 1) — addressed via migration
Cooldown 1h. À chaque échec de repair, le scanner appelle 2. Zero-variant test (finding 2) — 3. Backoff coverage — étendu Bumps appliqués pendant que j'y étais :
Skipped pour PRs dedicated : |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look at the new migration file
fd '20260608000000' --exec cat {}
# Look at the updated db.rs artwork module
rg -n 'list_partial_parents|mark_repair_failure|last_repair_failure_at|backoff' src/db.rs -A5 -B2
# Look at artwork_jobs.rs for backoff/mark_repair_failure usage
rg -n 'mark_repair_failure|backoff|last_repair_failure_at|run_once|repair_one' src/artwork_jobs.rs -A5 -B2
# Look at the test file
cat tests/artwork_scanner.rsLength of output: 16966 🏁 Script executed: #!/bin/bash
# Verify REPAIR_BACKOFF constant and any other new constants
rg -n 'REPAIR_BACKOFF|EXPECTED_VARIANT_COUNT|DEFAULT_SCAN_INTERVAL|DEFAULT_BATCH_SIZE' src/artwork_jobs.rs -A2
# Check the full artwork_jobs.rs for any subtle issues
cat src/artwork_jobs.rsLength of output: 11014
Finding 1 — Starvation guard ✅
Finding 2 — Zero-variant test ✅ Le test Finding 3 — Backoff coverage ✅ La vérification à deux temps dans
Bumps de dépendances : Tout est bon, le PR est prêt à merger. [approve] ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@migrations/20260608000000_artwork_repair_backoff.sql`:
- Around line 18-28: Change the new column last_repair_failure_at from
TIMESTAMPTZ to BIGINT (epoch-millis) and recreate the index
idx_metadata_artwork_repair_pending to target that BIGINT column (keeping the
WHERE last_repair_failure_at IS NOT NULL predicate); then update the Rust code
paths that consume/produce this value (list_partial_parents and
mark_repair_failure) to pass/expect epoch-millis BIGINT (not a timestamptz) so
the DB schema and the waveflow-core SQLite migrations remain parity.
🪄 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: 76aa6290-55d1-4f2c-ab7a-a6dc7b97369d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (5)
Cargo.tomlmigrations/20260608000000_artwork_repair_backoff.sqlsrc/artwork_jobs.rssrc/db.rstests/artwork_scanner.rs
CodeRabbit flagged the column type violates the repo's documented schema convention. CLAUDE.md says: "epoch-millis BIGINT for timestamps" + "Postgres tables must mirror shapes in waveflow-core's SQLite migrations for repository trait parity" — TIMESTAMPTZ breaks both. Switch the new column to BIGINT and propagate the epoch-millis contract through the API: - Migration: `last_repair_failure_at BIGINT`, partial index unchanged. - `db::artwork::list_partial_parents` takes `backoff_cutoff_ms: i64` instead of `chrono::DateTime<Utc>`. - `db::artwork::mark_repair_failure` takes `now_ms: i64` instead of using SQL `NOW()` — caller mints the timestamp, which also keeps the scanner clock and the DB clock decoupled for a future distributed deploy. - `artwork_jobs::run_once` mints `chrono::Utc::now().timestamp_millis()` once per cycle and passes it down. - Integration test reads the stamp as `Option<i64>` and computes the cutoff via `timestamp_millis() - 3_600_000`. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
@coderabbitai bien vu, finding valide — le contrat CLAUDE.md est explicite : "epoch-millis BIGINT for timestamps" + "Postgres tables must mirror shapes in waveflow-core's SQLite migrations". Switched (la migration n'était pas mergée donc je l'ai modifiée in-place) :
Push : d99919f. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify the migration change
fd '20260608000000' --exec cat {}
# Check db.rs for updated signatures
rg -n 'list_partial_parents|mark_repair_failure|backoff_cutoff|now_ms|last_repair_failure_at' src/db.rs -A8 -B2
# Check artwork_jobs.rs for how run_once mints the timestamp
rg -n 'timestamp_millis|backoff_cutoff|mark_repair_failure|run_once|REPAIR_BACKOFF' src/artwork_jobs.rs -A5 -B2
# Check test assertions
rg -n 'last_repair_failure_at|backoff_cutoff|timestamp_millis|now_ms|Option<i64>' tests/artwork_scanner.rs -A5 -B2Length of output: 9240
Rien à corriger. PR prêt à merger. [approve] ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@migrations/20260608000000_artwork_repair_backoff.sql`:
- Around line 18-25: The migration file
migrations/20260608000000_artwork_repair_backoff.sql has a future timestamp
(2026-06-08) — rename the file so its leading timestamp uses the current date
(e.g. 20260606) to avoid reserving a future migration order; keep the rest of
the file (ALTER TABLE metadata_artwork ADD COLUMN last_repair_failure_at BIGINT)
unchanged, ensure the new filename is unique among migrations, and perform a git
mv/commit so references and history stay consistent.
In `@src/artwork_jobs.rs`:
- Around line 141-145: The scanner breaks if batch_size > i64::MAX because
run_once casts a usize batch_size to i64 and passes it to list_partial_parents
(used as SQL LIMIT); clamp or validate WAVEFLOW_ARTWORK_SCANNER_BATCH_SIZE to
i64::MAX when reading the config (or perform a checked conversion in run_once)
so the value never wraps negative; update the code around run_once where it
calls list_partial_parents (and/or the config reader that produces batch_size)
to use i64::try_from or clamp_to(i64::MAX) and return/log a clear error if the
configured batch is > i64::MAX.
🪄 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: 56207b6f-864a-4039-b50c-250dd9aa6ce1
📒 Files selected for processing (4)
migrations/20260608000000_artwork_repair_backoff.sqlsrc/artwork_jobs.rssrc/db.rstests/artwork_scanner.rs
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>
|
@coderabbitai deux findings vérifiés : 1. Valide. .min(i64::MAX as usize)Le hot path 2. Migration filename "future date" → skipped (brief reason) La file s'appelle |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify the batch_size clamp in Config::from_env
rg -n 'i64::MAX|batch_size' src/config.rs -A3 -B3Length of output: 1421
Finding 1 — Finding 2 — Migration filename ✅ (skipped, justification valide) PR prêt à merger. [approve] 🧠 Learnings used |
Summary
Sprint 1 / Phase 1.i.1 — closes Sprint 1 on artwork. Spawns a tokio task at boot that periodically scans
metadata_artworkfor parents whose variant cache is incomplete (a partial-write incident left a parent row + parent bytes but lost a variant insert), pulls the source bytes from object_store, re-runs the resize pipeline, and inserts only the variants still missing.Why not apalis
The post-1.g sprint plan explicitly listed three candidates: `apalis` (Postgres queue + retries), `tokio-cron-scheduler` (simple cron), or rolling-our-own with `tokio::spawn` + the existing compaction-loop pattern.
The workload here is periodic catch-up, not queue-driven retries with priorities — the right primitive is a tokio interval loop. The surrounding infrastructure already has a sync compaction loop to generalise from, so the new module sits cleanly next to it.
`apalis` lands when a job type genuinely needs persistent queues + priorities + retries — RFC-004 community moderation (vote-based, needs durable queue) is the natural first user.
Wire surface
New env vars, opt-out by default once the artwork backend is set:
Highlights
Test plan
What's not in scope
Sprint 1 ✅ COMPLETE
This closes the artwork track for Sprint 1:
Next: Sprint 2 — playlist_track materialization (Phase 1.j) + OAuth providers + RFC-004 draft.
Summary by CodeRabbit
Nouvelles fonctionnalités
Configuration
Documentation
Base de données
Tests