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