muzick/REVIEW-2026-07-30.md

muzick — engineering review, 2026-07-30

Scope: full-project review (backend, workers, frontend, schema, deployment, docs) against live runtime state. Every finding marked confirmed was verified directly against the code, the live database, or a throwaway container — not inferred.

Live context at time of review: containers up 8 days, 3,962 tracks, 13,910 claims, 843 beliefs, 68 plays in the preceding 7 days (last: 2026-07-29). Actively used.

---

verdict

A genuinely good system that is quietly broken in ways its own dashboards cannot show. The architecture is sound and proportionate to the problem. The failures are not design failures — roughly six of the most interesting subsystems are dead or silently degrading in production, and nothing surfaces that.

Repair. Do not rewrite.

what the project is now

A single-user, actively-used self-hosted music player with an ambitious belief/claim recommendation engine. 17.2k LOC across three deployable units. Reachable only from LAN/VPN — /etc/nginx/sites-available/muzick.kvmx.ru has allow 10.42.0.0/24; allow 192.168.1.0/24; deny all and listens on LAN/VPN interfaces only, not 0.0.0.0.

what it should become

The same system, with its ambitious half either working or removed, and with a fresh-volume rebuild that actually works. At present it is a recommendation engine whose learning loop is partly disconnected, with no way to notice.

classification

fragile + misaligned, plus overbuilt in one specific place (System D diversity budgets, discovery walk).

Not stale, not abandoned, not better replaced.

---

first assessment

purpose	self-hosted music player + recommendation engine over a local library
intended users	single user (owner)
actual users	single user, actively — 68 plays in 7 days
critical workflows	browse/play library; Rolling Vibe session; enrichment; quarantine
current state	live and serving, with several subsystems silently inert
known failures	see findings 1–10
maintenance burden	moderate; concentrated in 3 oversized files and absent verification
technical constraints	/music is a read-only bind; worker needs SOCKS5 for egress; Typesense pinned 0.25.1
personal constraints	homelab, single operator, LAN/VPN-only exposure
what still works well	playback, library browsing, scanning, enrichment writes, migration registry, cron registration
what has become obsolete	discovery graph walk, System D budgets, workers shadow schema, dead frontend components

---

main findings

1. The project cannot be rebuilt from scratch — confirmed

Problem. A fresh Postgres volume produces a permanently empty library.

Evidence. Booted a throwaway postgres:16-alpine with the real backend/src/db/schema.sql and ran the scanner's exact insert (workers/src/scanner.service.ts:241 — INSERT INTO artists (name) VALUES ($1)):

ERROR: null value in column "canonical_name" of relation "artists"
       violates not-null constraint

schema.sql:50 declares canonical_name TEXT NOT NULL with no default. The live database has that column nullable — the volume predates the constraint. That is the only reason anything currently works.

Impact. Highest severity. Every artist insert fails on a fresh volume, and processFile swallows the error per-file (scanner.service.ts:199), so a scan reports success with 0 tracks detected. There is no disaster recovery, and docker-compose up -d — the documented setup path in the README — silently yields an empty library.

Action. Repair: insert canonical_name in the scanner, or give the column a default. Then prove it with a scratch-volume rebuild.

2. Every admin action in the UI returns 403 — confirmed

Problem. The entire admin surface of the SPA has been non-functional since auth landed.

Evidence. Confirmed at three layers:

• frontend/nginx.conf.template:16 injects only Authorization: Bearer ${MUZICK_API_KEY}
• docker-compose.yml passes only MUZICK_API_KEY to the frontend container (verified inside it: API=set ADMIN=)
• backend/src/app.ts:108 requires token === adminKey for /api/admin/*
• the two keys differ (35 vs 41 chars); frontend/src/services/api.ts is 5 lines and sets no headers at all

Impact. All 10 admin call sites are dead: the 477-line Jobs page (polling 403s every 3s/5s forever, rendering a blank Overview with no error state) and every Settings library action — Scan, Reindex, Reprocess artists, Re-enrich, Duplicates merge. Introduced by commits 5ed8d9e / 3bc9f2d without updating the frontend path.

Action. Repair: inject the admin key for location /api/admin/ in the nginx template. Honest context — the outer proxy already forges credentials for anything reaching location /api, so behind a LAN-only proxy this key split buys nothing while costing the whole admin surface.

3. Belief decay has never once run — confirmed

Problem. The temporal dimension of the recommendation engine is entirely inert.

Evidence. backend/src/services/db.service.ts:1788 builds a CTE with SELECT profile, CASE profile ... and no FROM clause. Postgres rejects it: column "profile" does not exist (SQLSTATE 42703), thrown hourly for 8+ days.

Rewritten as WITH halflives(profile, halflife_sec) AS (VALUES ...) and run against the live DB inside a transaction: UPDATE 843, then ROLLBACK. No data was changed.

Impact. obsession (14-day half-life) and contextual (7-day) never fade, so old fixations stay maximally weighted forever. Three of six spec'd profiles — discovery, contextual, forgotten — have zero rows.

Action. Repair (fix verified). Warning: the first successful run applies ~23 days of accrued decay at once, cutting obsession beliefs to ~0.32×. That is correct behaviour, but it will visibly change recommendations.

4. Claim de-duplication is broken and is actively corrupting scores — confirmed

Problem. Re-enrichment inserts duplicate claims instead of reinforcing them, inflating fusion weights.

Evidence. schema.sql:343 declares UNIQUE (..., source, user_id). Postgres treats NULLs as distinct, and every objective claim has user_id IS NULL — so ON CONFLICT DO UPDATE / DO NOTHING (db.service.ts:1425, workers/src/mb-spine-writer.ts:54) never fires. Live data:

dup_groups: 236 | excess_rows: 2110 | worst single claim: 86 copies

claim_fusion is SUM(trust * confidence * recency), so one edge can carry 86× its intended weight. ~15% of the 13,910 claims are re-enrichment duplicates.

Impact. The recommendation graph is measurably skewed toward whatever was re-enriched most — the most likely cause of repetitive recommendations. Grows with every re-enrich. Almost certainly the root cause behind commit d497588 ("claim_fusion MV duplicate-key failure").

Action. Repair: NULLS NOT DISTINCT (PG15+; running 16) or store the zero-UUID, plus a one-time dedup migration. Dedup must run before the constraint is added.

5. SQL injection via column names — confirmed

Problem. Request-body keys are interpolated into SQL as column identifiers.

Evidence. db.service.ts:1237:

const setClause = fields.map((f, i) => `"${f}" = $${i + 2}`).join(', ');

fields = Object.keys(data), and data is request.body as any (library.routes.ts:176). No allowlist. Same shape in updateArtist / updateAlbum.

Impact. A crafted body key closes the quoted identifier and injects into the SET list; also plain mass-assignment of path, state, play_count. Mitigated in practice only by the LAN/VPN-only proxy — which supplies the auth token automatically, so any device on the LAN can reach it from a browser.

Action. Repair: allowlist columns.

6. The dislike lifecycle is architecturally impossible as specified — confirmed

Problem. The spec's terminal state cannot be reached in this deployment.

Evidence. docs/architecture/02-invariants-and-risks.md §C makes hard file deletion the final step. /music is mounted :ro in both services. workers/src/cleanup.service.ts:72 calls unlink() → EROFS → continue. Live state: 37 tracks stuck WARNED since 2026-07-03 (27 days), 296 EROFS errors in the last 24h, and feedback contains zero deleted_permanent rows.

It fails closed — no DB row is deleted — which is the good outcome. But it will never progress, and it is by far the loudest log source.

Related, independent of the mount: db.service.ts:909 inserts the deleted_permanent audit row and then deletes the track, but feedback.track_id is ON DELETE CASCADE (verified: confdeltype = c) — so that audit row destroys itself.

Action. Requires a decision — see open decisions below. Fix the cascade regardless.

7. Prev and repeat-all are permanently broken — confirmed

Problem. next() destroys the queue history it needs.

Evidence. frontend/src/store/usePlaybackStore.ts:102 — next() does queue.slice(idx + 1), so the new track is always queue[0]. prev() requires idx > 0 (:123) → always resolves null. repeat: 'all' jumps to queue[0], which is the current last track.

Impact. After any auto-advance, Previous does nothing, ever. At the end of an album, repeat-all loops the final track instead of restarting.

Action. Repair: track a currentIndex instead of trimming. One change fixes both. Separately, the uncommitted usePlaybackStore.ts change in the working tree is a correct fix for a real end-of-queue auto-resume loop — commit it.

8. MusicBrainz rate limiting is not enforced — confirmed

Problem. The 1 req/s throttle is a TOCTOU race and does not hold.

Evidence. workers/src/integrations/http.ts:153-161 reads lastRequestAt, then await delay(...), then writes — no mutex, no queue, with concurrency: 10. Ten jobs read the same timestamp, sleep the same duration, and fire in the same tick.

Impact. Up to ~10 req/s against MusicBrainz's 1 req/s policy, risking an IP block. And because musicbrainz.client.ts:280 catches HttpError and returns null, a rate-limited MusicBrainz is indistinguishable from "no data for your library" while every job reports success.

Action. Repair: serialize throttle per host with a promise chain.

9. Expensive work computed and discarded — confirmed

Problem. Two spec'd subsystems cost real time and produce nothing.

Evidence.

• session-director.service.ts:242-270 getBudgets runs 7+ aggregate queries per replan. rankCandidates (:677) never reads its budgets or state parameters, and detectAntiLoop ignores its first three. System D budget enforcement is unimplemented.
• discovery.service.ts:86 writes source: 'graph_exploration', which is absent from source_trust (verified — only 7 keys: curated, mb, cover_art_archive, discogs, lastfm, listener_behavior, tag). POST /api/discovery/walk therefore always 500s on an FK violation, after having already committed an orphan candidate row.

Action. Delete, or finish. The discovery walk has never worked.

10. Verification is largely theatre — confirmed

Problem. Nothing would have caught findings 1, 3, or 4.

Evidence.

• Frontend npm run typecheck cannot run — frontend/node_modules/typescript is a partial install (no package.json, dangling .bin/tsc symlink).
• .github/workflows/typecheck.yml matrixes only [backend, workers] and has no test job — the 33 backend tests never run in CI.
• Those 33 tests pass in 440ms with zero database. All three files mock { query: vi.fn() } and largely assert that a substring appears in a SQL template. Several cannot fail: expect(results).toBeDefined(), and assertions wrapped in if (results.length > 0).
• frontend/Dockerfile uses npm install, not npm ci, so the lockfile is ignored. Backend node_modules has @fastify/cors@11.3.0 installed against a ^9.0.1 declaration — local and built images do not agree.

Every confirmed bug above lives in untested code.

---

secondary findings

• Integrity sweep has no sanity guard. workers/src/integrity.service.ts:188 marks every unreachable track MISSING with no percentage threshold and no check that MUSIC_DIR is mounted. If /mnt/hdd1 is unmounted when the 03:00 sweep runs, the entire library is flipped to MISSING with no automatic path back. It also only ever checks an arbitrary 5,000 tracks (:74 — LIMIT with no ORDER BY, no pagination).
• Worker shares one pg connection across concurrency: 10. workers/src/index.ts:24 creates a single Client; cleanup.service.ts:82 issues bare BEGIN/COMMIT/ROLLBACK on it. Another job's query can be enrolled in — and discarded by — that transaction. This is the exact hazard backend/src/app.ts:35-40 documents as its reason for using a Pool.
• No BullMQ retries anywhere. Zero attempts/backoff in either package (default is 1 attempt). Combined with jobId: meta-<trackId> and removeOnFail: {age: 86400}, re-enqueueing a failed track within 24h is a silent no-op.
• Image proxy SSRF. backend/src/routes/images.routes.ts:79 validates redirect hop 1, then re-fetches without redirect: 'manual' — following up to 20 further hops unvalidated. Directly contradicts the comment above it. Any open redirect on an allowlisted host (commons.wikimedia.org, *.musicbrainz.org) becomes a full SSRF primitive.
• split-collab-artists.ts cascade-deletes tracks. :125-136 leaves colliding albums on the doomed artist, then deletes it; albums.artist_id and tracks.album_id are both ON DELETE CASCADE. Do not run as written. dedup-artists-albums.ts is the correct implementation.
• Play recording only happens inside a Vibe session. historyService.recordPlay / recordSkip / feedback have no callers in the frontend (verified by grep — only historyService.list() is used). The sole write path is v2.routes.ts:137 on a Vibe completed. Live: sum(play_count) = 1445 = exactly the play_history row count, and only 535/3,962 tracks (13.5%) have any plays. Browsing/album playback contributes no signal, yet Home's "Most Played" and Vibe's seed list both sort by play_count.
• No error listener on the audio element (AudioEngine.tsx:89-93), and play().catch(() => {}) swallows failures. A 404 stream leaves isPlaying: true, the scrubber at 0:00, and no toast — playback hangs silently.
• Nine bare usePlaybackStore() calls with no selector re-render on every state change; setPosition fires ~4×/s, so 50 TrackRows re-render four times a second during playback.
• workers' own ensureSchema() (enrichment.service.ts:78-156) is 79 lines of shadow schema that recreates a non-unique idx_artists_mbid on every boot — an index the backend migration 20260709_artists_mbid_unique deliberately dropped.
• runMigrations has no advisory lock (db.service.ts:573). Concurrent boots can both run a pending migration; several bodies are not concurrency-safe.
• No keyboard control of playback at all — no Space, no arrow seek, no next/prev. TrackRow.tsx:59 is a <div onClick> with no tabIndex/role/onKeyDown, so no list in the app is playable by keyboard.

---

corrected claims

Recorded so these are not acted on at the wrong priority.

• dedup-albums grouping by title alone (admin.routes.ts:41, duplicated in enrichment.service.ts:1264 — omits artist_id from the GROUP BY) is real in code, but blast radius today is zero: no two albums in the library share a lowercase title. A landmine that fires on the next reprocess_artists if one appears — fix it, but it is not active data loss.
• dislikes.grace_hours being NULL is a non-issue — the column has DEFAULT 48.
• Mixed timestamp / timestamptz (19 naive vs 12 aware; tracks has both) is latent, not active — DB and backend are both Etc/UTC. Refactor-later.
• "Tracks missing from disk" — an initial check stat'd container paths from the host and was wrong. Re-run inside the container: 60/60 present. Invariant A holds.
• normalize_artist truncation is real (AC/DC → AC, Felix Mendelssohn → Feli) but affects only 2 live rows and produces 0 collisions. Low priority.

---

keep

The three-process split; Postgres as source of truth; the migration registry (schema_migrations is recorded, ordered, individually atomic, append-only by convention — genuinely well done); upsertJobScheduler cron registration (correctly idempotent across restarts); graceful worker shutdown; the backend's Pool-not-Client reasoning; the Ethos design system; Artwork's gradient fallback; useDislikeTrack (the one flow with full success/error/undo feedback); the docs/architecture/ spec set — it is the reason these bugs are identifiable as bugs.

remove

• discovery.service.walkGraphForDiscovery — never worked
• System D budget computation — or wire it in
• frontend/src/components/Inspector.tsx — 169 lines, unreachable (setInspector is only ever called by closeInspector)
• frontend/src/components/PanelHeader.tsx — 36 lines, zero importers, documenting a refactor that was never applied
• frontend/nginx.conf — dead, unreferenced by the Dockerfile, and the insecure variant
• ~20 dead frontend service exports
• workers' ensureSchema() — shadow schema
• the stale muzick.service note in AGENTS.md and CLAUDE.md — verified: no such systemd unit exists

repair now, in order

1. canonical_name — restores rebuildability (finding 1)
2. nginx admin-key injection — restores the entire admin UI (finding 2)
3. decayBeliefs CTE — fix verified (finding 3)
4. claim dedup + NULLS NOT DISTINCT (finding 4)
5. column allowlist in the three update* methods (finding 5)
6. currentIndex in the playback store; commit the pending fix (finding 7)
7. serialize throttle per host (finding 8)
8. integrity-sweep abort threshold (secondary)
9. worker Client → Pool (secondary)

refactor later

Decompose enrichment.service.ts (1,311 lines, six unrelated concerns); move the 328-line inline MIGRATIONS array out of db.service.ts; add selectors to the nine bare usePlaybackStore() call sites; unify Genres.tsx / Discover.tsx (~250 near-duplicate lines); normalize timestamp types; advisory lock around runMigrations.

rewrite only if

Nothing here justifies a rewrite. Every finding is a local repair in structurally sound code.

The one thing worth reconsidering from first principles is System A/B (claims → fusion → beliefs) — but only after findings 3 and 4 are fixed, because that subsystem has never once been observed running correctly. Judge the design on working behaviour, not on the current state.

verification plan

• docker run a scratch Postgres with schema.sql, run a real scan, assert count(tracks) > 0 — turns finding 1 into a permanent regression test
• add frontend to the CI matrix; add a npm test job; reinstall frontend node_modules; switch the frontend Dockerfile to npm ci
• backend integration tests against a real container for withTransaction, runMigrations, recordPlay, and the auth hook
• post-fix assertions:
  • claims duplicate groups → 0
  • max(last_decayed_at) advances hourly
  • zero EROFS errors in 24h
  • Jobs page renders stats

open decisions

The dislike lifecycle. The spec says delete the file; the mount is read-only. Pick one:

• (a) Drop hard deletion. Terminal state becomes HIDDEN; amend invariant §C; delete finalizeDeleted. Safe, honest, and the library stays immutable. Recommended — the library is a read-only bind for a reason.
• (b) Make deletion real. Requires an rw mount. cleanup.service.ts currently unlinks before its DB transaction and has no dry-run and no per-sweep cap; that ordering must be fixed first.

Either way, clear the 37 stuck WARNED rows — they generate ~300 errors/day.

uncertainties

• No HTTP endpoint could be exercised: the review sandbox blocked egress (identical 503s that never reached the backend, confirmed absent from its logs). The auth and SSRF findings are established from configuration and code, not from a live request. The SSRF one is unambiguous in code.
• docker-compose.yml sets no resource limits, though docs/architecture/02-invariants-and-risks.md §B names cgroup limits as the mitigation for Essentia starving the API. Unquantified in practice.
• Whether the discovery / contextual profiles are empty because decay never ran or because nothing ever wrote them is not yet separable; re-evaluate after finding 3 is fixed.

---

second-opinion review, 2026-07-30 (Claude)

Independently spot-checked the load-bearing findings against the code. Verdict endorsed: repair, do not rewrite.

Verified directly:

• Finding 1 — schema.sql:50 declares canonical_name TEXT NOT NULL with no default; scanner.service.ts:241 inserts only (name). Confirmed.
• Finding 3 — the decayBeliefs CTE is WITH halflives AS (SELECT profile, CASE ...) with no FROM clause; Postgres rejects it on every hourly run. Confirmed; the VALUES-based rewrite is the right fix.
• Finding 4 — UNIQUE (..., source, user_id) with nullable user_id means ON CONFLICT never fires for objective claims. Confirmed; NULLS NOT DISTINCT (PG16) is correct, and dedup must indeed precede the constraint.
• Finding 5 — updateTrack interpolates Object.keys(body) into the SET clause. Confirmed.
• Finding 7 — next() does queue.slice(idx + 1), so the current track is always index 0; prev()'s idx > 0 guard can never pass after an auto-advance, and repeat-all restarts from the track that just ended. Confirmed. The uncommitted end-of-queue fix in the working tree is correct and independent of this bug.
• Finding 2 — nginx.conf.template injects only Bearer ${MUZICK_API_KEY} for all of /api, no admin-key location block. Confirmed at the config layer.

Judgment calls also endorsed: the repair ordering (rebuildability → admin UI → learning loop), the "corrected claims" discipline, and option (a) for the dislike lifecycle — the :ro mount is intentional.

Two additions:

1. Finding 3 rollout: the first successful decay run applies ~23 days of accrued decay at once (obsession beliefs → ~0.32×). Correct behaviour, but recommendations will shift visibly — expected, not a regression.
2. Finding 4 dedup: the one-time migration should collapse each duplicate group keeping MAX(last_reinforced_at) (and ideally max confidence), not merely delete extra rows, or reinforcement recency is lost.