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`:

```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-<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 `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 `<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.