The reprocess_artists case was 208 of index.ts's 515 lines — 40% of the file
and most of what this PR changed in it, buried inside a switch. index.ts is
now 313 lines and reads as what it is: wiring, the job switch, cron
registration, shutdown.
The move also collapses a real duplication. The artist merge and the
normalized_name dedup pass ran the same five statements in the same order
against different id pairs, so the withTransaction change had to be made
twice, identically. Both now call one mergeArtistInto(client, keepId,
loserId), which takes a Queryable so the caller owns the transaction, and the
ON DELETE CASCADE hazard is documented once instead of twice.
Verified as behaviour-preserving: the statement sequence diffs identical
against the previous commit, and workers typecheck is clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dislike lifecycle promised WARNED -> HIDDEN -> deleted, but nothing ever
removed a file: cleanup.service logged its intent behind
MUZICK_ALLOW_HARD_DELETE and returned, and the two backend delete paths
(hardDeleteTrack, permanentlyDeleteTrack) disagreed about what deletion
meant. The review recommended dropping hard deletion and making HIDDEN
terminal; the owner chose to make deletion real instead.
- cleanup.service performs a true unlink() — no trash directory — for
tracks that have been HIDDEN for a 7-day grace period, then settles the
row. This is the single unlink() call site in the system.
- permanentlyDeleteTrack is the one delete path; hardDeleteTrack is gone.
- a deleted_permanent audit row records what was removed, and
migration 20260730_hard_delete_audit_trail backs it.
MUZICK_ALLOW_HARD_DELETE remains OFF: the docker-compose entry is commented
out, there is no enabling default in code, and the worker's /music bind is
the only writable one. Deletion stays dry-run until the owner opts in
deliberately.
REVIEW-2026-07-30.md open decision: dislike lifecycle.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sweep stats every track path and marks unreadable files missing, with no
check that /music is mounted. An unmounted or misbehaving bind would fail
every stat and mark the entire library missing in one pass; the ratio is only
recoverable by a full rescan.
Three guards, cheapest first:
- liveness: probe a sample of existing track paths before doing anything;
abort if none are readable
- ratio: abort mid-sweep if the missing fraction crosses a threshold,
leaving already-marked rows alone rather than rolling back a partial pass
- progress: keyset pagination over id with the cursor persisted in
integrity_sweep_state, so a sweep aborted or restarted mid-run resumes
instead of re-walking from the top and re-marking
The repair-corrupted-metadata script shares the same failure mode and gets
the same abort path.
REVIEW-2026-07-30.md secondary finding: integrity sweep has no mount check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
kami