The worker ran every job through a single pg Client while BullMQ was
configured with concurrency: 10. A Client is one connection with one
protocol stream and no queueing: ten concurrent jobs interleave on it, and
any BEGIN/COMMIT is shared by all of them, so an unrelated job's failure can
roll back another's work and a rollback can discard a third's committed
intent.
Switched to a Pool, added a small withTransaction(pool, fn) helper that
takes a dedicated connection per transaction, and threaded a Queryable
interface through the services so they accept either a pool or a pooled
client. Both reprocess_artists merge blocks — the artist merge and the
duplicate-album merge — now run inside withTransaction; previously a failure
partway through left artists merged and their tracks unmoved.
integrity.service and cleanup.service get only the constructor type change
here so this commit compiles; their own fixes follow in the next two
commits. cleanup.service's BEGIN/COMMIT-on-a-Pool is therefore still wrong
at this commit and is replaced wholesale by the hard-delete commit.
REVIEW-2026-07-30.md finding 4 (and the concurrency note in finding 3).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
throttle() read lastRequestAt, awaited delay(), then wrote it back — a
TOCTOU race with no mutex or queue, under concurrency: 10. Ten jobs read the
same timestamp, slept the same duration and fired in the same tick, giving
up to ~10 req/s against MusicBrainz's 1 req/s policy and risking an IP
block.
Replaced the lastRequestAt map with a per-host { lastRequestAt, tail }
limiter; each call links onto that host's promise chain, so the
read-sleep-write critical section is serialized and N concurrent callers
space out by minIntervalMs. Chain rejections are swallowed so one failure
cannot poison the queue. Per-host rather than global, so other integrations
are not starved by MusicBrainz.
Measured: 5 concurrent same-host calls at 200ms -> 802ms (previously all in
one tick); 3 distinct hosts at 1000ms -> 0ms, confirming no cross-host
starvation.
musicbrainz.client caught HttpError and returned null at all 7 catch sites,
making a rate-limited MusicBrainz indistinguishable from "no data for your
library" while every job reported success. A shared logMbFailure() now logs
429 (and 503 whose body mentions a rate limit) at error, stating results are
INCOMPLETE. The error model is otherwise unchanged.
REVIEW-2026-07-30.md finding 8.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
next() did `queue.slice(idx + 1)`, so the current track was always
queue[0]. prev()'s `idx > 0` guard could therefore never pass after an
auto-advance — Previous did nothing, ever — and repeat: 'all' jumped to
queue[0], which is the track that just finished, looping the last track of
an album instead of restarting it.
Replaced with a currentIndex cursor; the queue is no longer trimmed behind
the playhead. The old slice did serve a purpose — bounding Vibe-prefetch
growth — so that is preserved as a MAX_HISTORY = 50 cap that drops the
oldest entries and re-bases the index, rather than dropped outright.
setQueue/playTrack/setCurrentTrack recompute the cursor, next()/prev() fall
back to findIndex if it drifts, and shuffle now picks by index so the
cursor stays valid.
Consumer audit: NowPlayingPanel and Vibe.tsx already derived position via
findIndex and needed no change. TrackRow.handlePlay did
`setQueue(queue.slice(index))`, which re-broke prev at the point of click
even with the store fixed; it now passes the intact queue.
This commit also includes a pre-existing uncommitted fix from the working
tree (not authored by Claude): the end-of-queue auto-resume loop, which
stops playback at the end of the queue instead of restarting. It is correct
and independent of the cursor bug, and is preserved verbatim here.
REVIEW-2026-07-30.md finding 7.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The entire admin surface of the SPA had been dead since auth landed
(5ed8d9e / 3bc9f2d). nginx.conf.template injected only
`Authorization: Bearer ${MUZICK_API_KEY}` for all of /api, docker-compose
passed only MUZICK_API_KEY to the frontend container, and app.ts requires
token === adminKey for /api/admin/*. The two keys differ, and
services/api.ts sets no headers of its own.
All 10 admin call sites were affected: the Jobs page polled 403s every
3s/5s forever and rendered a blank Overview with no error state, and every
Settings library action (Scan, Reindex, Reprocess artists, Re-enrich,
Duplicates merge) silently failed.
Three changes, each necessary:
- a `location /api/admin/` block injecting the admin key
- the Dockerfile envsubst list widened to include MUZICK_ADMIN_KEY,
without which the new variable substitutes to empty and the header
becomes a bare "Bearer"
- MUZICK_ADMIN_KEY passed to the frontend service in docker-compose
nginx selects the longest matching prefix regardless of block order;
verified empirically in a throwaway nginx:stable-alpine running the real
envsubst output against a stub that echoes $http_authorization:
/api/admin/queue-stats -> Bearer ADMINKEY456
/api/admin/duplicates/merge -> Bearer ADMINKEY456
/api/tracks -> Bearer APIKEY123
/api/health -> Bearer APIKEY123
All 10 call sites use /admin/... under the axios /api baseURL and none
request bare /api/admin without a trailing slash.
Also gives the Jobs page an error state: a banner that names a 401/403 as a
missing or wrong admin key, a Retry button, "Loading queue stats..." in
place of a blank Overview, and refetchInterval returning false once the
query has errored so it stops hammering a failing endpoint.
Deletes frontend/nginx.conf — unreferenced by the Dockerfile (confirmed by
grep) and the insecure variant of the template.
Worth noting and not addressed here: the outer LAN-only proxy already
forges credentials for everything reaching /api, so this key split buys no
real security while having cost the whole admin surface. Collapsing to one
key would be simpler.
REVIEW-2026-07-30.md finding 2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
db.service inserted the 'deleted_permanent' feedback row and then deleted
the track, but feedback.track_id was ON DELETE CASCADE (verified on the
live DB: confdeltype = 'c'), so the audit row deleted itself. feedback
contains zero deleted_permanent rows.
feedback is an audit log and must outlive its subject: the FK becomes
ON DELETE SET NULL. track_id was already nullable, and nothing in backend/
or workers/ SELECTs from feedback — the only other reference is
mergeTracks()'s UPDATE feedback SET track_id, which re-points to the
survivor — so no caller assumed non-null.
Migration 20260730_feedback_track_id_set_null drops the constraint by
matching confdeltype rather than by name, since the live schema has
drifted. Verified on a scratch PG16: confdeltype flips 'c' -> 'n' and a
deleted_permanent row survives its track's deletion.
Correct under either resolution of the dislike-lifecycle decision, so it
lands independently of it.
REVIEW-2026-07-30.md finding 6 (cascade only).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
updateTrack/updateArtist/updateAlbum built their SET clause from
Object.keys(data) where data is `request.body as any`, interpolating
request-supplied keys straight into SQL as quoted identifiers:
fields.map((f, i) => `"${f}" = $${i + 2}`)
A crafted body key closes the quoted identifier and injects into the SET
list. Mitigated in practice only by the LAN/VPN-only proxy — which supplies
the auth token automatically, so any device on the LAN could reach it from
a browser.
Adds per-table UPDATABLE_COLUMNS plus an allowedFields() helper, applied in
all three methods. The allowlist lives in the service layer rather than the
routes so it covers every caller.
Unknown keys are dropped rather than rejected: the three routes do no error
mapping, so a throw surfaces as a bare 500, and the pre-existing "No fields
to update" error still fires for a payload rejected in its entirety.
Also closes plain mass-assignment. Excluded: path/hash/mtime
(scanner-owned; path is the only link to the read-only bind),
state/quarantined_at/deleted_at (dislike lifecycle and integrity sweep),
play_count/skip_count/dislike_count/last_played_at (learning signal —
forgeable counters poison the engine), and identity/generated columns.
The only callers are the three HTTP PUTs; the frontend's update* service
exports are dead code, so nothing relied on writing an excluded column.
REVIEW-2026-07-30.md finding 5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
claims declared UNIQUE (..., source, user_id). Every objective claim has
user_id IS NULL, and under default NULLS DISTINCT semantics Postgres treats
those rows as unique, so the ON CONFLICT DO UPDATE / DO NOTHING clauses in
db.service and mb-spine-writer never fired. Re-enrichment inserted a fresh
duplicate every run instead of reinforcing.
Live data: 236 duplicate groups, 2110 excess rows, worst single claim 86
copies, ~15% of 13,910 claims. claim_fusion is SUM(trust * confidence *
recency), so one edge could carry 86x its intended weight — the likely
cause of repetitive recommendations, and almost certainly the root of
d497588 (claim_fusion MV duplicate-key failure).
Migration 20260730_claims_dedup_nulls_not_distinct, two phases in one
transaction. Dedup MUST precede the constraint or adding it fails.
Phase 1 collapses each group into its most recently reinforced row,
carrying forward MAX(last_reinforced_at), MAX(evidence_at) and
MAX(confidence) — reinforcement recency would otherwise be lost by simply
deleting extras. The MAX(...) OVER grp and ROW_NUMBER() OVER ordered
windows are deliberately separate: an ORDER BY inside the window makes the
default frame UNBOUNDED PRECEDING TO CURRENT ROW, which turns MAX() into a
running maximum and would silently keep the wrong confidence.
Phase 2 drops the old constraint by matching its definition rather than its
name, because the live DB has drifted and its autogenerated name is
truncated at 63 characters.
Verified on a scratch PG16 seeded with the old schema plus a 3-row
duplicate group, a distinct-source singleton and a real-user_id row:
UPDATE 3 / DELETE 2, keeper retained the group max of each field from three
different rows, re-run is a no-op, and a subsequent ON CONFLICT DO UPDATE
with user_id = NULL fires correctly.
REVIEW-2026-07-30.md finding 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CTE was `WITH halflives AS (SELECT profile, CASE profile ...)` with no
FROM clause. Postgres rejects it with 42703 (column "profile" does not
exist) on every hourly invocation, so the temporal dimension of the
recommendation engine had never executed once — obsession (14d half-life)
and contextual (7d) never faded.
Rewritten as `WITH halflives(profile, halflife_sec) AS (VALUES ...)`,
half-lives preserved exactly.
One deliberate semantic change: the broken CASE had an `ELSE 30 * 86400`
fallback, so an unrecognised profile would have decayed on a 30-day
half-life. The VALUES join leaves unknown profiles undecayed instead.
Today that is a no-op (only `forgotten`, already excluded by the WHERE),
but a future profile added without a half-life will now conspicuously not
decay rather than quietly decaying at an arbitrary rate.
Verified against a scratch PG16 with one belief per profile aged exactly
one half-life: UPDATE 2, obsession and contextual halved, forgotten and a
fresh longterm untouched. Against the live DB (in a rolled-back
transaction) the fix reports UPDATE 843.
NOTE ON ROLLOUT: the first successful run applies ~23 days of accrued
decay at once, cutting obsession beliefs to ~0.32x. That is correct
behaviour, but recommendations will shift visibly. Expected, not a
regression.
REVIEW-2026-07-30.md finding 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three separate insert paths made a fresh Postgres volume unusable. The live
database only works because its volume predates the constraints.
- scanner.service.resolveOrCreateArtist inserted only (name), but
schema.sql declares canonical_name NOT NULL with no default. Every
artist insert failed, and processFile swallows per-file errors, so a
scan reported success with 0 tracks and a permanently empty library.
- enrichment.service inserted explicitly into artists.normalized_name,
which is GENERATED ALWAYS AS (normalize_artist(name)) STORED:
"cannot insert a non-DEFAULT value into column" (428C9). All
enrichment artist creation failed on a fresh volume.
- db.service.createArtist omitted canonical_name, same failure.
canonical_name holds the raw tag name, not normalize_artist() output,
which truncates on `/` and a standalone `x` ("AC/DC" -> "AC"). That is the
convention createLocalArtist already used. The truncation bug in
artists.name is pre-existing and deliberately left untouched here.
Verified on a scratch postgres:16-alpine with the real schema: the old
statement reproduces the NOT NULL violation, the new path yields
artists/albums/tracks/track_artists rows.
REVIEW-2026-07-30.md finding 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The MV SELECTed COALESCE(user_id, zero-uuid) but GROUPed BY raw user_id,
so a global enrichment claim (user_id NULL) and a default-user behavior
claim (user_id = zero-uuid) for the same edge produced two rows that
collide on idx_claim_fusion_pk, breaking REFRESH ... CONCURRENTLY.
Exposed by the #219 listener_behavior same_scene_as/alias_of writes.
New migration rebuilds the MV grouping by the COALESCE'd user_id so the
two fuse into one row.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-user SET NX PX lock with Lua CAS release around the
getActivePlan->mutate->setActivePlan span so concurrent prefetch +
feedback requests serialize instead of losing one side's write.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
both generators ran N sequential queries per artist/album.
rewritten with ROW_NUMBER() OVER (PARTITION BY ...) to get per-group
limits in a single round-trip, preserving existing semantics:
- comfortGenerator: up to 2 tracks per artist (was 20 queries → 1)
- deepDiveGenerator: up to 5 tracks per album (was 20 queries → 1)
kami