# 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`: ```ts 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-` 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 `TrackRow`s 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 `
` 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.