feat(track-sync): apply pipeline for the track entity (phase 4.d.0.2)

[InstaZDLL]

Summary

Second PR of the 4.d.0 sprint chain — closes the Phase 1.g.0 deferred TODO that left track out of the apply pipeline (see 20260604000000_apply_pipeline.sql:60-67). The desktop can now sync tracks + their album / artist linkage through `/api/v1/sync/ops`; the apply path materialises the 4.d.0.1 entity tables in one atomic transaction per op.

Wire shape

• `entity: "track"`, `entity_id` is the file_path — the per-library natural identity (`UNIQUE (library_id, file_path)`). Wire-shape divergence from the Phase 1.g.0 note (which suggested file_hash) is deliberate: the desktop's tag-editor rewrites embedded metadata frames on save, so file_hash changes while file_path doesn't — keying on file_hash would break the upsert. file_hash rides as a payload field for the liked_track / rating joins.
• `payload.library_canonical_id` is the tenant scope.
• INSERT payload packs the full track metadata + the album / artist plumbing: `album_title?`, `album_artist_name?` (None → compilation), `is_compilation?`, `artists?: [String, ...]` (the desktop's `";"`-split list — position derives from array index).

Handler chain

1. Resolve library_id from `library_canonical_id` (Skipped if not yet materialised).
2. Upsert every contributor artist on `(library_id, name)`.
3. Resolve album_artist_id by name (dedups against contributors).
4. Upsert album on the 4.d.0.1 natural key `(library_id, canonical_title, album_artist_id)` — NULLS NOT DISTINCT collapses the compilation case to one row.
5. Upsert track on `(library_id, file_path)`. Every scalar overwrites on conflict — a tag-edit re-emit lands as an in-place UPDATE.
6. DELETE-then-INSERT the multi-artist links via a single UNNEST INSERT (avoids N+1 round-trips).

DELETE is keyed on `(library_id, file_path)` and cascades into `track_artist`. SET ops are intentionally Unknown — the desktop re-emits a full INSERT on tag edit and the upsert merges.

CR pre-push findings applied

• H1 (blocker): tag-edit re-emit would 500 because `ON CONFLICT (library_id, file_hash)` falls through to INSERT and trips the pre-existing `(library_id, file_path)` UNIQUE. Fixed by swapping the upsert key to file_path (the existing natural key) and making entity_id the file_path. The originally-planned migration `20260609120000_track_sync.sql` is dropped — no schema change needed.
• H2: empty-string `album_title` / `album_artist_name` rejected at the apply boundary as InvalidPayload rather than tripping the CHECK constraints (which would 500). New `reject_empty` helper.
• M1: added `track_insert_cross_tenant_library_canonical_skips` — pins the per-profile scope on `lookup_library_id`.
• M2: added `track_insert_set_op_is_unknown` to lock the Unknown contract for SET ops.
• M3: `replace_track_artists` uses a single UNNEST INSERT instead of N+1.
• L3: doc-comment about tag-edit hashing rewritten to match reality.

Skipped: M4 / M5 (perf optimisations to measure first), L1 (acceptable at realistic artist counts), L2 (matches existing playlist / library behaviour), L4 / L5 (documentation polish).

Test plan

12 new tests in `tests/apply.rs`:

• `track_insert_creates_track_album_and_artists` — full insert, verify track + album + artist + track_artist rows.
• `track_insert_multi_artist_preserves_order` — three artists at positions 0/1/2.
• `track_insert_compilation_with_null_album_artist` — compilation case.
• `track_insert_replay_is_idempotent_and_dedups_album_artist` — counts (1,1,1,1) post-replay.
• `track_insert_without_library_canonical_is_rejected` — push fails, no row leak.
• `track_insert_with_unknown_library_skips_but_logs` — Skipped, push 200.
• `track_insert_then_replay_after_library_lands_materialises` — three-step proof.
• `track_delete_removes_row_and_cascades_track_artist` — delete + cascade.
• `track_re_emit_after_tag_edit_updates_in_place` — H1 regression guard.
• `track_insert_rejects_empty_album_title` — H2 regression guard.
• `track_insert_set_op_is_unknown` — M2 contract pin.
• `track_insert_cross_tenant_library_canonical_skips` — M1 regression guard.

`cargo check --all-targets` + `cargo fmt --all --check` clean locally. Tests run on CI Postgres.

Summary by CodeRabbit

• New Features

  • Synchronisation des pistes avec materialisation/maj en place (mises à jour idempotentes), gestion automatique des métadonnées (album, artistes, contributeurs) et prise en charge multi-artistes/compilations.

• Bug Fixes / Comportements

  • Rejet des payloads invalides (ex. titre d’album vide), opérations "set" reconnues mais non matérialisées, opérations "Skipped" quand la bibliothèque référencée n’existe pas encore.

• Documentation

  • Nouvelle section de guidance détaillant le flux de synchronisation des pistes.

• Tests

  • Suite d’intégration couvrant insert/delete, replay, déduplication, multi-tenant et cas de compilation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4d121b1e-9aaf-442a-9bd0-668a4bbccfba

📥 Commits

Reviewing files that changed from the base of the PR and between be9bc74 and 8418029.

📒 Files selected for processing (2)

• src/apply.rs
• tests/apply.rs

---

📝 Walkthrough

Walkthrough

Synchronisation des entités track via apply operations: identification par (library_id, file_path), file_hash comme payload de déduplication inter-appareils, validation de métadonnées album/artistes, upserts séquentiels de contributeurs/album/track, remplacement des liens multi-artistes, isolation multi-tenant par library_canonical_id, et comportement Skipped pour librairies non encore matérialisées.

Changes

Track Synchronization

Layer / File(s)	Summary
DB types and contracts CLAUDE.md, src/db.rs	Documentation Phase 4.d.0.2 et structures publiques ArtistLinkInput et TrackInput<'a> mappant métadonnées track et liens multi-artistes pour l'upsert.
DB persistence functions src/db.rs	Fonctions transactionnelles upsert_artist, upsert_album (fusion year/is_compilation), upsert_track (clé library_id + file_path), replace_track_artists (DELETE + UNNEST vectorisé), delete_track, et lookup_library_id pour résolution cross-tenant.
Apply routing and track handler src/apply.rs	Routage de l'entité "track" via résolution profile_canonical_id, dispatch vers track::apply, parsing/validation du payload (champs requis, rejet des chaînes vides pour album fields), Skipped si library non matérialisée, séquence d'upserts (artistes → album → track) puis remplacement des liens, delete par (library_id,file_path), set → Unknown.
Integration test suite tests/apply.rs	Helpers (materialise_library, track_insert_payload) et scénarios couvrant insertion/album/artistes/positions, multi-artistes ordre, compilation (album_artist NULL + is_compilation), idempotence, rejets (library_canonical_id manquant, album_title vide), Skipped avec re-matérialisation sur replay, suppression avec cascade track_artist, re-émission update-in-place, set ops Unknown, et isolation multi-tenant.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• InstaZDLL/waveflow-server#34: Dépendances schéma / contraintes album/artist/track_artist introduites en phase 4.d.0.1 utilisées par db::track_sync.
• InstaZDLL/waveflow-server#26: Établit la pipeline apply et la résolution profile_canonical_id réutilisées pour ce handler track.

Poem

🎵 Tracks now sync, file paths align,
Artists ordered, albums sign,
Skipped waits till libraries shine,
Replays mend when rows combine,
Small payloads, big harmony.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Le titre décrit précisément la fonctionnalité principale ajoutée : le pipeline apply pour l'entité track (phase 4.d.0.2), ce qui correspond directement aux changements du PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/4-d-0-2-track-sync-apply

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 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/apply.rs`:
- Around line 1130-1161: The function artists_from_payload currently returns
duplicates which later cause PK conflicts in
upsert_artist/replace_track_artists; change artists_from_payload to deduplicate
artists while preserving their first-seen order (e.g., iterate arr, convert
entries to strings as currently done, but track seen names with a HashSet and
only push the first occurrence into out) and return that deduplicated
Vec<String>; alternatively, if you prefer strict validation, detect duplicates
and return ApplyError::InvalidPayload noting duplicate artist entries instead of
deduplicating—implement the chosen behavior inside artists_from_payload so
upsert_artist and replace_track_artists no longer receive duplicate artist_ids.

In `@src/db.rs`:
- Around line 818-833: L'UPSERT ON CONFLICT dans la requête SQL ne met pas à
jour la colonne added_at sur conflit, donc les re-emit n'actualisent pas le tri
"Recently added"; modify the ON CONFLICT ... DO UPDATE clause in the SQL string
found in src/db.rs (the UPSERT that currently sets title, file_hash, ...,
album_id) to also include added_at = EXCLUDED.added_at so that added_at is
overwritten on conflict (ensure the same SQL string/variable that contains the
RETURNING id is updated).

In `@tests/apply.rs`:
- Around line 1175-1216: The test
track_insert_with_unknown_library_skips_but_logs currently only asserts the
track row was not materialised; also assert the operation was persisted in the
durable log (sync_op) so replay can later succeed: capture the op UUID before
calling push, then after push query the sync_op table for a row with that op id
(or matching target = 'track' and file_path) and assert it exists (and
optionally has a status/mark indicating Skipped). Update the test to store the
generated Uuid, run a SELECT against sync_op to fetch/count that entry, and
assert the result to ensure the op was logged.

🪄 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: 09f212d0-7525-4ad8-b723-97b154928606

📥 Commits

Reviewing files that changed from the base of the PR and between 02a371f and be9bc74.

📒 Files selected for processing (4)

• CLAUDE.md
• src/apply.rs
• src/db.rs
• tests/apply.rs

[InstaZDLL]

@coderabbitai CI fix + round 1 findings pushed (commit 8418029).

CI failure fixed: track_insert_creates_track_album_and_artists panic'd on a (i64,) decode against an INT8 scalar — query_scalar::<_, (i64,)> was wrong (the column is an int, not a record). Swapped to query_scalar::<_, i64>.

Findings applied:

1. Artist dedup ✅ Real bug. Without dedup, a desktop that ships ["A", "A"] (e.g., a tag with a duplicated entry after the ";" split) would have upsert_artist return the same id twice → replace_track_artists trips the (track_id, artist_id) PK on the second INSERT. Fixed: HashSet-based first-seen-order dedup inside artists_from_payload. New regression test track_insert_dedups_duplicate_artists pins the contract (duplicates dropped silently, first-seen wins).

2. sync_op durability assertion ✅ Added. The Skipped-path test now also SELECTs sync_op WHERE user_id + operation_id + entity = 'track' + entity_id = file_path and asserts COUNT = 1. A future refactor that drops the op on the floor breaks this before it breaks the Skipped-then-replay contract.

Finding skipped:

3. added_at = EXCLUDED.added_at on UPSERT conflict ❌ Skip with rationale. The column's documented semantic is "Epoch milliseconds for the original library import. Drives the Recently added sort" (20260530000003_track.sql:47-50). A tag-edit re-emit doesn't change WHEN the file first landed — overwriting would bump the row to "just added" on every re-emit, which is the opposite of the column's purpose. The current behaviour (preserve on conflict) is intentional.

`cargo check --all-targets` + `cargo fmt --all --check` clean locally.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check artists_from_payload dedup implementation
rg -n "artists_from_payload" --type rust -A 20

# Check upsert_track ON CONFLICT clause for added_at
rg -n "added_at" src/db.rs -A 2 -B 2

# Check the new regression test
rg -n "track_insert_dedups_duplicate_artists" tests/apply.rs -A 40

# Check sync_op durability assertion in Skipped-path test
rg -n "sync_op" tests/apply.rs -A 5 -B 2

Length of output: 9445

---

🏁 Script executed:

#!/bin/bash
# Check full ON CONFLICT clause for upsert_track
rg -n "ON CONFLICT" src/db.rs -A 30 -B 2

# Also check the position field handling in artists_from_payload after dedup
rg -n "artists_from_payload" src/apply.rs -A 40 | tail -30

Length of output: 19672

---

---

@InstaZDLL Fixes vérifiés sur commit 8418029 — tout est correct.

CI fix ✅ query_scalar::<_, i64> — bon type, le decode (i64,) était bien la cause.

Finding 1 — Artist dedup ✅ Implémentation solide : seen.insert() retourne false sur les doublons, la position est ré-indexée depuis le Vec dédupliqué (comportement correct), et en bonus les entrées vides dans le tableau artists sont aussi rejetées (lignes 1161–1167). Test bien ciblé avec ["Daft Punk", "Daft Punk", "Pharrell Williams", "Daft Punk"].

Finding 2 — sync_op durability ✅ L'assertion est précise — WHERE user_id + operation_id + entity + entity_id plutôt qu'un COUNT générique. Un refactor qui drop l'op le cassera immédiatement.

Finding 3 — added_at preserve-on-conflict ✅ Confirmé dans le code : added_at est absent du DO UPDATE SET (lignes 819–832 de src/db.rs). Tous les scalaires de métadonnées sont écrasés sauf added_at. La sémantique "date d'import originel" est préservée correctement.

Une observation mineure : updated_at est aussi absent du DO UPDATE SET. Sur un re-emit de tag-edit, la colonne reste à sa valeur d'insertion initiale. Si un client utilise updated_at pour détecter des changements locaux, il ne verra rien. Est-ce intentionnel (timestamp d'insertion seulement) ou un oubli ?

🐇 ✅

---

🧠 Learnings used

Learnt from: CR
Repo: InstaZDLL/waveflow-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-07T22:55:22.615Z
Learning: Applies to {migrations/**/*.sql,src/api/**/*.rs,src/db.rs} : Playlist tracks: Apply pipeline writes `playlist + field: "tracks"` ops into `playlist_track` table with `(playlist_id, track_id)` PK. Store snapshot columns (`snapshot_title`, `snapshot_artist`, `snapshot_duration_ms`) for cross-device display. Use `INSERT … ON CONFLICT DO NOTHING` for race-safe concurrent operations.

---

Learnt from: CR
Repo: InstaZDLL/waveflow-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-07T22:55:22.615Z
Learning: Applies to {migrations/**/*.sql,src/api/**/*.rs,src/db.rs} : Sync operations: Use append-only `sync_op` log with `BIGSERIAL id`. Enforce per-`(user, device)` UNIQUEs on `operation_id` and `lamport_ts`. Use `ON CONFLICT operation_id DO NOTHING` for idempotent replay and 409 + `stored_max` on lamport regression.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 0
File: :0-0
Timestamp: 2026-06-07T22:50:38.169Z
Learning: Dans waveflow-server (migrations/20260608120000_album_artist.sql + tests/album_artist.rs), la contrainte cross-library est enforced au niveau schéma via des FK composites : chaque lien entité-à-entité porte `library_id` dans LES DEUX colonnes de la FK, avec `UNIQUE (id, library_id)` sur la table parente comme cible. Détails :
- `track`, `artist`, `album` ont tous `UNIQUE (id, library_id)`.
- `album.album_artist_id` → `FOREIGN KEY (album_artist_id, library_id) REFERENCES artist (id, library_id) ON DELETE SET NULL (album_artist_id)` (PG15+ column-level SET NULL).
- `track.album_id` → `FOREIGN KEY (album_id, library_id) REFERENCES album (id, library_id) ON DELETE SET NULL (album_id)`.
- `track_artist` porte une colonne dénormalisée `library_id BIGINT NOT NULL` + deux FK composites vers `track(id, library_id)` et `artist(id, library_id)` ON DELETE CASCADE — la même colonne `library_id` partagée rend impossible l'insertion d'une paire inter-bibliothèque.
- Cible PG17 (même gate que `UNIQUE NULLS NOT DISTINCT`). Le apply pipeline (4.d.0.2) dérive `track_artist.library_id` de `track.library_id` à l'upsert.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 0
File: :0-0
Timestamp: 2026-06-06T19:46:50.757Z
Learning: Dans `src/artwork_jobs.rs` et la migration `20260608000000_artwork_repair_backoff.sql` (waveflow-server), la colonne `metadata_artwork.last_repair_failure_at` est `BIGINT` epoch-millis (pas `TIMESTAMPTZ`), conforme au contrat CLAUDE.md "epoch-millis BIGINT for timestamps" + compatibilité SQLite mirror. `list_partial_parents` prend `backoff_cutoff_ms: i64`. `mark_repair_failure` prend `now_ms: i64` (caller-minted, clock DB découpée). `run_once` mint `chrono::Utc::now().timestamp_millis()` une fois par cycle et utilise `saturating_sub` pour le cutoff. `REPAIR_BACKOFF = 3600s` → 3_600_000 ms.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 3
File: .github/workflows/ci.yml:26-27
Timestamp: 2026-05-30T00:39:05.874Z
Learning: In the repository `InstaZDLL/waveflow-server`, the maintainer intentionally uses the floating tag `postgres:17` (not pinned by SHA256 digest) for the GitHub Actions CI service container. The rationale: it's an ephemeral test container, the test surface (SELECT 1 / sqlx integration tests) doesn't change between minor Postgres versions, it auto-receives security patches, and digest pinning creates unjustified maintenance overhead for a non-production test infrastructure. Do not flag this as an issue in future reviews.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 16
File: src/db.rs:144-146
Timestamp: 2026-05-31T00:40:18.480Z
Learning: Dans `src/db.rs` (waveflow-server, Rust/sqlx/Postgres), la fonction `find_or_provision_by_external_id` utilise un pattern "read-first, write-on-miss" plutôt qu'un CTE `INSERT ... ON CONFLICT DO NOTHING RETURNING id UNION ALL SELECT id`. Raison : sous READ COMMITTED, les deux branches d'un CTE partagent le même snapshot statement — si une tx concurrente insère la même ligne entre l'acquisition du snapshot et notre INSERT, DO NOTHING se déclenche sans RETURNING, et le SELECT du UNION ALL ne voit pas encore la row commitée → 0 lignes → erreur 500. Le pattern retenu (SELECT optionnel → return si trouvé; sinon INSERT ON CONFLICT DO UPDATE SET external_id = EXCLUDED.external_id RETURNING id) évite ce cas en garantissant que RETURNING se déclenche toujours sur le miss path, même en cas de race concurrente. Référence : Erwin Brandstetter sur la race condition du pattern CTE UPSERT.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 6
File: src/api/profiles.rs:254-273
Timestamp: 2026-05-30T03:28:04.286Z
Learning: In `src/api/profiles.rs`, the non-atomic rename_then_read race (rename_for_user followed by get_for_user) is a known issue intentionally deferred. The proper fix is to add `UPDATE ... RETURNING *` to `rename_for_user` in waveflow-core, batched with the same refactor for `insert_for_user` and `touch_last_used`, rather than a piecemeal server-side workaround.

---

Learnt from: InstaZDLL
Repo: InstaZDLL/waveflow-server PR: 18
File: tests/stream.rs:355-363
Timestamp: 2026-05-31T12:09:16.191Z
Learning: In `src/api/stream.rs` (Rust, Axum), the `resolve_path` function strips leading `/` and `\` characters from the `file_path` stored in the DB via `trim_start_matches(['/', '\\'])` before joining with `music_root`. This means that binding an absolute path (e.g. `/tmp/xxx/secret.txt`) as `file_path` in tests does NOT exercise the `starts_with(music_root)` prefix-check guard — the absolute path silently becomes a relative segment joined under `music_root`, which then doesn't exist, causing a canonicalize failure (wrong 404 code path). To correctly test the prefix-check boundary, use a relative `../<sibling_dir>/file` path that canonicalizes to an existing file outside `music_root`.

---

Learnt from: CR
Repo: InstaZDLL/waveflow-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-07T22:55:22.615Z
Learning: Applies to migrations/**/*.sql : Tenancy enforcement at schema level for album/artist/track: Use composite FKs carrying `library_id` in both columns with parent's `UNIQUE (id, library_id)` as target. Set up `ON DELETE SET NULL` for artist → album links and album → track links to preserve rows on parent deletion.