docs(13-07): summary — music index + music.* surface complete, 23/23 green
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
archipelago
co-authored by
Claude Fable 5
Claude Fable 5
parent
d3b0ed8a1d
commit
17da7a7233
+161
@@ -0,0 +1,161 @@
|
||||
---
|
||||
phase: 13-aiui-functional-conversational-node-control-and-content-surf
|
||||
plan: 07
|
||||
subsystem: music
|
||||
tags: [music-library, index, rpc, lofty, incremental-refresh, atomic-write, rust]
|
||||
|
||||
# Dependency graph
|
||||
requires:
|
||||
- phase: 13-04
|
||||
provides: "13-MUSIC-MODEL.md (D-13 decision), music/mod.rs entity types, music/tags.rs extract_tags with media-root confinement, lofty 0.24.0"
|
||||
provides:
|
||||
- "core/archipelago/src/music/index.rs — MusicIndex: reindex, refresh_incremental, load (NewerSchema refusal), save_atomic (temp+fsync+rename), group_albums/group_artists, ReindexState guard, ScanStats"
|
||||
- "core/archipelago/src/music/mod.rs — media_roots(Config) (filebrowser/Music + purchased-content), LibrarySnapshot"
|
||||
- "music.* RPC surface: music.list-albums, music.list-artists, music.list-tracks, music.status, music.reindex — one dispatcher.rs prefix arm"
|
||||
affects: [13-11, 13-12, song-grid, peer-audio]
|
||||
|
||||
# Tech tracking
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "Atomic index persistence: serialize to sibling temp + fsync + rename — concurrent readers see complete old or complete new, never partial"
|
||||
- "Forward-version refusal: load returns a distinct NewerSchema error and never overwrites the newer file; explicit reindex is the only rebuild path"
|
||||
- "Scan guard: AtomicBool + RAII drop-release; duplicate scans report already-running with last stats instead of queueing"
|
||||
- "RPC testability without RpcHandler: module-level dispatch fn parameterized on (state, data_dir, roots), thin RpcHandler method delegating"
|
||||
|
||||
key-files:
|
||||
created:
|
||||
- core/archipelago/src/music/index.rs
|
||||
- core/archipelago/src/api/rpc/music.rs
|
||||
modified:
|
||||
- core/archipelago/src/music/mod.rs
|
||||
- core/archipelago/src/api/rpc/dispatcher.rs
|
||||
- core/archipelago/src/api/rpc/mod.rs
|
||||
|
||||
key-decisions:
|
||||
- "media_roots = [data_dir/filebrowser/Music, data_dir/purchased-content] — both D-13 sources as local filesystem roots; per-root MusicSource assignment (purchased-content files map to Peer{onion} from their first path component)"
|
||||
- "music.reindex accepts optional incremental:true routing to refresh_incremental, so the stays-fresh truth has a production caller (default remains the on-demand full rebuild)"
|
||||
- "Albums ordered by (album_artist, album, min track year); tracks by (disc, track number, title); TrackId (source, path) is the final tiebreak everywhere"
|
||||
|
||||
patterns-established:
|
||||
- "T-13-42 mitigation shape: never write the index in place — save_atomic is the only writer"
|
||||
- "T-13-39 enforced twice: walker skips symlinks whose canonical target escapes the canonical root, and extract_tags re-checks confinement per file"
|
||||
|
||||
requirements-completed: [] # AIUI-03 spans multiple plans (13-11 SongGrid wiring still pending)
|
||||
|
||||
# Metrics
|
||||
duration: ~3h45m wall clock (dominated by four full non-incremental archipelago compiles on the shared 4-core box; one 10-minute foreground cargo run was killed by the harness timeout and rerun)
|
||||
completed: 2026-08-04
|
||||
status: complete
|
||||
---
|
||||
|
||||
# Phase 13 Plan 07: Music Library Index and music.* RPC Surface Summary
|
||||
|
||||
**Persisted, incrementally-refreshed music library over the D-13 model — atomic JSON index with schema-version refusal, symlink-confined scanning, derived album/artist grouping, and an authenticated five-method `music.*` surface behind a single dispatcher arm — 23/23 `music::` tests green.**
|
||||
|
||||
## Performance
|
||||
|
||||
- **Duration:** ~3h45m wall clock (~25 min of it authoring; the rest cargo compile time — four full `CARGO_INCREMENTAL=0` builds of the archipelago crate at ~10-14 min each on the live-node box)
|
||||
- **Started:** 2026-08-04T11:27Z
|
||||
- **Completed:** 2026-08-04T15:15Z
|
||||
- **Tasks:** 2 (both auto/TDD)
|
||||
- **Files modified:** 5
|
||||
|
||||
## Accomplishments
|
||||
|
||||
- `music/index.rs`: `reindex` walks the media roots, extracts tags per audio file via 13-04's `extract_tags`, and persists `data_dir/music/index.json`; `refresh_incremental` diffs `(path, mtime, size)` so unchanged files are never re-extracted, removes rows for vanished files (derived albums vanish with their last track), and preserves the lazily-backfilled `content_hash` column on unchanged rows.
|
||||
- `save_atomic` (temp sibling + fsync + rename) makes the concurrent-read truth hold: a reader sees the complete previous index or the complete new one, never a torn file — verified by a 100-writer/200-reader interleaving test.
|
||||
- `load` refuses `schema_version > MUSIC_SCHEMA_VERSION` with a distinct `NewerSchema` error and never overwrites the newer file; readers serve an empty library, and only an explicit reindex rebuilds (13-MUSIC-MODEL.md's downgrade contract, T-13-43).
|
||||
- Symlinks whose canonical target escapes the canonical media roots are skipped, not followed (T-13-39) — confinement enforced in the walker *and* re-checked per file inside `extract_tags`.
|
||||
- One comparator everywhere: albums by (album artist, album title, min year), tracks by (disc, track, title), with the `(source, path)` identity as final tiebreak — list ordering is stable across repeated calls.
|
||||
- `music.*` RPC surface: five methods behind exactly one `m if m.starts_with("music.")` dispatcher arm (the 13-01 `assistant.` pattern), placed adjacent to the `content.*` block; envelopes carry `total` + `scanned_at`; `list-tracks` clamps `limit` to [1,500] (default 100); `music.reindex` spawns and returns immediately, refusing to duplicate a running scan (T-13-41).
|
||||
- Nothing `music.*` in `UNAUTHENTICATED_METHODS` — the surface rides the existing session/CSRF/RBAC gate (T-13-40), asserted by `music_methods_require_session`.
|
||||
|
||||
## Task Commits
|
||||
|
||||
1. **Task 1: The index — scan, group, persist, stay fresh** — `49687f7e` (feat) — `music/index.rs` + `music/mod.rs` (`media_roots`, `LibrarySnapshot`); 9 tests, one per behavior bullet
|
||||
2. **Task 2: The music.* RPC surface** — `d3b0ed8a` (feat) — `api/rpc/music.rs`, single dispatcher arm, `mod music;` declaration; 7 tests
|
||||
|
||||
## Files Created/Modified
|
||||
|
||||
- `core/archipelago/src/music/index.rs` — `MusicIndex`, `IndexEntry`, `ScanStats`, `IndexError::NewerSchema`, `reindex`, `refresh_incremental`, `load`, `save_atomic`, `group_albums`, `group_artists`, `ReindexState`/`ReindexGuard`, `shared_state`, `INDEX_FILENAME`
|
||||
- `core/archipelago/src/music/mod.rs` — `pub mod index;`, `media_roots(&Config)`, `LibrarySnapshot`
|
||||
- `core/archipelago/src/api/rpc/music.rs` — `handle_music` prefix sub-dispatcher + `handle_music_list_albums` / `list_artists` / `list_tracks` / `status` / `reindex`
|
||||
- `core/archipelago/src/api/rpc/dispatcher.rs` — one `music.` prefix arm (the only registration point)
|
||||
- `core/archipelago/src/api/rpc/mod.rs` — `mod music;` declaration (alphabetical block)
|
||||
|
||||
## Decisions Made
|
||||
|
||||
- **Media roots**: `filebrowser/Music` (OwnLibrary — peer audio purchases are also auto-filed here by `content.*`'s paid-download path, so they enter the library with real filenames) plus `purchased-content` (Peer byte cache; files under it map to `MusicSource::Peer{onion}` from their first path component). Note: purchased-content stores files extensionless today, so that root yields no tracks until files there carry audio extensions — peer catalog surfacing in `SongGrid` is 13-11's job via the existing `content.*` discovery, per 13-MUSIC-MODEL.md ("reuses content.*'s existing peer-audio discovery").
|
||||
- **Grouping field names spot-checked against 13-MUSIC-MODEL.md** (plan acceptance): row key `TrackId{source, path}` (hybrid-identity) ✓; `content_hash` lazily-backfilled column preserved on unchanged rows, reset when bytes change ✓; albums derived at read time on `AlbumId{album_artist, album}`, artists on the `artist` tag — never persisted ✓; index at `data_dir/music/index.json`, pretty-printed JSON with top-level `schema_version` ✓; newer-version index treated as absent and never overwritten ✓.
|
||||
- `MusicIndex::empty()` populates `scanned_at` with "now", so even a never-scanned library serves a timestamp — empty arrays + timestamp, never null, never an error.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
### Auto-fixed / necessary additions
|
||||
|
||||
**1. [Rule 3 - Blocking] `mod music;` added to `api/rpc/mod.rs`**
|
||||
- **Found during:** Task 2
|
||||
- **Issue:** The plan's `files_modified` lists only `music.rs` + `dispatcher.rs`, but a new module must be declared in its parent.
|
||||
- **Fix:** One line in the alphabetical `mod` block (between `monitoring` and `names`).
|
||||
- **Commit:** `d3b0ed8a`
|
||||
|
||||
**2. [Rule 2 - Missing functionality] `music.reindex` gained an optional `incremental: true` param**
|
||||
- **Found during:** Task 2 (`cargo build` bin target flagged `refresh_incremental`/`ScanMode::Incremental` as dead code — the freshness machinery had no production caller, making the "stays fresh without a full rebuild" truth test-only)
|
||||
- **Fix:** `music.reindex {"incremental": true}` routes to `refresh_incremental`; default stays the on-demand full rebuild. Covered by `reindex_incremental_mode_refreshes_without_full_reextraction`. Also removed all three new dead-code warnings (bin builds with zero warnings from music files).
|
||||
- **Commit:** `d3b0ed8a`
|
||||
|
||||
**3. [Rule 1 - Bug] `anyhow!` format-string brace escape**
|
||||
- **Found during:** Task 2 first compile — `"expected { album, album_artist }"` parsed as format args.
|
||||
- **Fix:** escaped to `{{ album, album_artist }}`. Caught before any commit.
|
||||
|
||||
### Process notes
|
||||
|
||||
- **TDD gate:** tests were authored first (one per behavior bullet) but each task landed as a single green commit rather than separate RED `test(...)` + GREEN `feat(...)` commits — this repo's CLAUDE.md hard rule ("commit each feature the moment it works — it compiles and its targeted tests pass") takes precedence over the generic RED-commit convention, matching how 13-04 Task 3 executed. See TDD Gate Compliance below.
|
||||
- **Fixture reuse:** the compact FLAC byte-builder is duplicated into `index.rs` and `api/rpc/music.rs` test modules rather than refactoring `tags.rs`'s builders to be shared — `tags.rs` is outside this plan's file list. (An incidental `rustfmt` recursion into `tags.rs` was reverted to keep commits scoped.)
|
||||
|
||||
**Total deviations:** 2 necessary additions + 1 trivial compile fix. No scope changes.
|
||||
|
||||
## TDD Gate Compliance
|
||||
|
||||
- RED evidence exists as authored-first tests (16 new tests across the two tasks), but no standalone `test(...)` commits precede the `feat(...)` commits — CLAUDE.md's green-commit rule was applied deliberately (see Process notes). Gate sequence in git log is therefore `feat(49687f7e)` → `feat(d3b0ed8a)` with tests and implementation co-committed.
|
||||
|
||||
## Known Stubs
|
||||
|
||||
None — no placeholder text, no hardcoded empty values flowing to UI, no unwired data paths. (The `purchased-content` root legitimately yields zero tracks today because its files are extensionless content-ids; peer audio reaches `SongGrid` via 13-11's `content.*` path per the model doc, and audio purchases already land in `filebrowser/Music` with extensions.)
|
||||
|
||||
## Threat Flags
|
||||
|
||||
None — no security-relevant surface beyond the plan's threat model (T-13-39/40/41/42/43/44 all mitigated as specified; T-13-45 remains accepted/deferred to 13-12's `wrap_untrusted`, and nothing here places tag text in a model context).
|
||||
|
||||
## Issues Encountered
|
||||
|
||||
- **Foreground cargo timeout ceiling:** a full `CARGO_INCREMENTAL=0` test compile of the archipelago crate takes ~10-14 min on this box (live node sharing 4 cores), which exceeds the 10-minute foreground tool cap — one run was SIGTERM'd mid-compile and rerun. Workaround used thereafter: detached cargo writing to a log with a `.done` marker + foreground polling loops, keeping the session turn alive.
|
||||
|
||||
## Verification (plan-level)
|
||||
|
||||
- `CARGO_INCREMENTAL=0 cargo test --package archipelago music::` → **`ok. 23 passed; 0 failed`** (9 index + 7 tags + 7 rpc)
|
||||
- `CARGO_INCREMENTAL=0 cargo build --package archipelago` → exit 0 (`Finished dev profile`), zero warnings from music/rpc-music files
|
||||
- `grep -c 'starts_with("music.")' dispatcher.rs` → **1**
|
||||
- `grep -n 'music\.' middleware.rs` → no match (nothing unauthenticated)
|
||||
- `git ls-files core/archipelago | grep -ciE '\.(mp3|flac|m4a|ogg)$'` → 0 (no binary fixtures)
|
||||
- Index field names match `13-MUSIC-MODEL.md` (spot check recorded under Decisions Made)
|
||||
|
||||
## User Setup Required
|
||||
|
||||
None — no external service configuration required.
|
||||
|
||||
## Next Phase Readiness
|
||||
|
||||
- 13-11 can wire `SongGrid` to `music.list-albums`/`music.list-tracks` (envelopes follow `content.*`'s items+metadata shape: arrays + `total` + `scanned_at`) and trigger `music.reindex` (full or incremental).
|
||||
- No music tool was added to the assistant's curated registry — deliberately out of scope per the plan (track independence, D-13).
|
||||
- T-13-45 (peer tag text as untrusted model input) remains 13-12's `wrap_untrusted` responsibility.
|
||||
|
||||
## Self-Check: PASSED
|
||||
|
||||
- FOUND: `core/archipelago/src/music/index.rs`, `core/archipelago/src/api/rpc/music.rs`, `core/archipelago/src/music/mod.rs` (updated), `.planning/phases/13-.../13-07-SUMMARY.md`
|
||||
- FOUND commits: `49687f7e`, `d3b0ed8a`
|
||||
|
||||
---
|
||||
*Phase: 13-aiui-functional-conversational-node-control-and-content-surf*
|
||||
*Completed: 2026-08-04*
|
||||
Reference in New Issue
Block a user