Missing files: detect them, stop offering them, and follow them when they move #121

2026-08-06T20:40:16-04:00

bvandeusen commented

2026-08-06 20:40:16 -04:00

Closes #2523 and #2528. Two commits, server-only, carries migration 0055.

CI green on 24d33042 (run 3477): sqlc verify, go vet, golangci-lint, go test (short, race), and the integration lane — which applies real migrations and includes an end-to-end rename test.

Follows on from #2499 (v2026.08.06), which fixed genre extraction but left the operator still seeing welded genres. The reason turned out to be a different bug entirely.

Nothing ever noticed a deleted file

The scanner walks the filesystem, so a row whose file is gone was never visited: not scanned, not errored, not counted. classifyEvent ignores fsnotify removals by design, and the safety-net scan is the same walk, so it only ever covered additions. Rows accumulated permanently.

Confirmed on the operator's library — a completed scan reported skipped=24185 errored=0 while the MBID backfill (which opens files by DB path rather than walking) logged ~40 no such file or directory across three reorganised albums. Those same rows kept their pre-#2499 welded genre, since the version-stamped tag re-read can only reach files the walk visits. That's how this surfaced.

The harm isn't cosmetic: tracks is the candidate universe for recommendation.sql / discover.sql / system_mixes.sql, and nothing filtered on file existence, so a mix could spend a slot on a track that cannot stream.

Marks, never deletes

A missing file is a claim about the filesystem, and the filesystem lies transiently — an unmounted volume, a network blip, a container that started before its media mount attached. Every sweep in internal/gc resolves a truth inside the database and is safe to run blind. This one isn't, so no deletion happens.

Three guards refuse to act on ambiguous evidence:

every scan root must resolve to a non-empty directory (a bare mount point passes os.Stat, so emptiness is checked separately)
the walk must have seen at least one file
one reconcile may newly mark at most 25% of the library

Clearing a mark runs unconditionally, outside the cap — otherwise a library that tripped it once could never recover after the mount returned.

There's a second reason not to delete, which decided the design: taste_profile.sql reaches artist and genre through play_events → tracks, so a ghost row's plays keep contributing. Deleting CASCADEs play_events and general_likes_tracks away, which is the operation that would actually skew the taste profile. Keeping marked rows is the history-preserving choice.

Excluded from selection, not from history

Filtered: all 13 track-emitting queries (radio ×2, system mixes ×5, discover ×4, most-played ×2), the 6 play-history seed picks, and the genre browse axis.

Deliberately not filtered:

ListPlaylistTracks — shared by system and user playlists; hiding a track the user added themselves would be wrong. System playlists shed orphans on their next daily rebuild.
History and the taste profile — those record the past.
Year browse — an album is still a real release even if tracks are gone.

Following a move (#2528)

Track identity was file_path, so a file returning under a new name looked like a deletion plus an unrelated new track: the old row kept the like and every play event while a fresh zero-history row appeared. A liked song read as unliked, its play count reset, and Rediscover could offer it as a discovery. Renumbering an album was enough — which is exactly what happened to the operator's Minutes to Midnight.

Adoption re-points the existing row's file_path and clears its mark; the normal UpsertTrack then conflicts on file_path and updates that row, so the track id survives and likes, plays and playlist memberships come with it. Clients see an upsert rather than a delete-and-create, so no cache churn.

Matching is recording MBID first (survives a re-encode), then file_size + duration_ms for untagged files — both required non-zero, since duration_ms is 0 when ffprobe failed and matching 0 against 0 would pair unrelated broken files. Only marked-missing rows are eligible: a row whose file exists elsewhere is a duplicate, and re-pointing it would corrupt the copy that still exists. Ambiguous matches insert fresh rather than adopting arbitrarily — a fork is recoverable, a wrong merge isn't.

Why Scan is now three phases

The obvious implementation fails in the case that matters most. Adoption can only claim a row that is already marked missing, and reconcile ran at the end of the scan — so a rename performed while the server was down surfaced the deletion and the addition in the same scan: the new path inserted first, and the fork became permanent. It would only have worked for renames made while the server was running.

So enumeration is now separate from processing: walk (paths only, no tag reads or probes) → reconcile → process in walk order. Cheap, since WalkDir already stats every entry.

Known limitation

When reconcile refuses — absent root, or a reorganisation exceeding the 25% cap — adoption can't fire and renamed files fork as before. That's the pre-#2528 behaviour rather than a new failure, but it means a mass reorganisation of a small library still forks. The scan warning names that consequence explicitly.

Tests

26 new unit cases against narrowed interfaces (trackReconciler, trackAdopter, both with compile-time assertions that *dbq.Queries still satisfies them), covering every guard and every ambiguity path. Plus TestScanner_AdoptsMovedFile_Integration, which writes a real file with a TXXX:MusicBrainz Track Id frame, renames it on disk, re-scans, and asserts the track id is unchanged, the mark is cleared, the old path has no row, and no row was added.

🤖 Generated with Claude Code

https://claude.ai/code/session_01N6vZoJ4Se5YyaqdtGVkap5

Closes #2523 and #2528. Two commits, server-only, **carries migration 0055**. CI green on `24d33042` (run 3477): sqlc verify, `go vet`, golangci-lint, `go test (short, race)`, and the integration lane — which applies real migrations and includes an end-to-end rename test. Follows on from #2499 (`v2026.08.06`), which fixed genre extraction but left the operator still seeing welded genres. The reason turned out to be a different bug entirely. ## Nothing ever noticed a deleted file The scanner walks the filesystem, so a row whose file is gone was never visited: not scanned, not errored, not counted. `classifyEvent` ignores fsnotify removals by design, and the safety-net scan is the same walk, so it only ever covered additions. Rows accumulated permanently. Confirmed on the operator's library — a completed scan reported `skipped=24185 errored=0` while the MBID backfill (which opens files by DB path rather than walking) logged ~40 `no such file or directory` across three reorganised albums. Those same rows kept their pre-#2499 welded genre, since the version-stamped tag re-read can only reach files the walk visits. That's how this surfaced. The harm isn't cosmetic: `tracks` is the candidate universe for `recommendation.sql` / `discover.sql` / `system_mixes.sql`, and nothing filtered on file existence, so a mix could spend a slot on a track that cannot stream. ## Marks, never deletes A missing file is a claim about the filesystem, and the filesystem lies transiently — an unmounted volume, a network blip, a container that started before its media mount attached. Every sweep in `internal/gc` resolves a truth *inside* the database and is safe to run blind. This one isn't, so no deletion happens. Three guards refuse to act on ambiguous evidence: - every scan root must resolve to a **non-empty** directory (a bare mount point passes `os.Stat`, so emptiness is checked separately) - the walk must have seen at least one file - one reconcile may newly mark at most **25%** of the library Clearing a mark runs unconditionally, outside the cap — otherwise a library that tripped it once could never recover after the mount returned. There's a second reason not to delete, which decided the design: `taste_profile.sql` reaches artist and genre through `play_events → tracks`, so a ghost row's plays keep contributing. Deleting CASCADEs `play_events` and `general_likes_tracks` away, which is the operation that would actually skew the taste profile. Keeping marked rows is the history-preserving choice. ## Excluded from selection, not from history Filtered: all 13 track-emitting queries (radio ×2, system mixes ×5, discover ×4, most-played ×2), the 6 play-history seed picks, and the genre browse axis. Deliberately **not** filtered: - `ListPlaylistTracks` — shared by system *and* user playlists; hiding a track the user added themselves would be wrong. System playlists shed orphans on their next daily rebuild. - History and the taste profile — those record the past. - Year browse — an album is still a real release even if tracks are gone. ## Following a move (#2528) Track identity was `file_path`, so a file returning under a new name looked like a deletion plus an unrelated new track: the old row kept the like and every play event while a fresh zero-history row appeared. A liked song read as unliked, its play count reset, and Rediscover could offer it as a discovery. Renumbering an album was enough — which is exactly what happened to the operator's *Minutes to Midnight*. Adoption re-points the existing row's `file_path` and clears its mark; the normal `UpsertTrack` then conflicts on `file_path` and updates **that** row, so the track id survives and likes, plays and playlist memberships come with it. Clients see an upsert rather than a delete-and-create, so no cache churn. Matching is recording MBID first (survives a re-encode), then `file_size` + `duration_ms` for untagged files — both required non-zero, since `duration_ms` is 0 when ffprobe failed and matching 0 against 0 would pair unrelated broken files. Only marked-missing rows are eligible: a row whose file exists elsewhere is a **duplicate**, and re-pointing it would corrupt the copy that still exists. Ambiguous matches insert fresh rather than adopting arbitrarily — a fork is recoverable, a wrong merge isn't. ### Why Scan is now three phases The obvious implementation fails in the case that matters most. Adoption can only claim a row that is *already* marked missing, and reconcile ran at the **end** of the scan — so a rename performed while the server was down surfaced the deletion and the addition in the same scan: the new path inserted first, and the fork became permanent. It would only have worked for renames made while the server was running. So enumeration is now separate from processing: **walk (paths only, no tag reads or probes) → reconcile → process in walk order.** Cheap, since `WalkDir` already stats every entry. ## Known limitation When reconcile refuses — absent root, or a reorganisation exceeding the 25% cap — adoption can't fire and renamed files fork as before. That's the pre-#2528 behaviour rather than a new failure, but it means a mass reorganisation of a *small* library still forks. The scan warning names that consequence explicitly. ## Tests 26 new unit cases against narrowed interfaces (`trackReconciler`, `trackAdopter`, both with compile-time assertions that `*dbq.Queries` still satisfies them), covering every guard and every ambiguity path. Plus `TestScanner_AdoptsMovedFile_Integration`, which writes a real file with a `TXXX:MusicBrainz Track Id` frame, renames it on disk, re-scans, and asserts the track id is unchanged, the mark is cleared, the old path has no row, and no row was added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01N6vZoJ4Se5YyaqdtGVkap5

bvandeusen added 2 commits 2026-08-06 20:40:16 -04:00

feat(library): detect missing files and stop offering them — #2523

test-go / test (push) Successful in 1m0s

Details

test-go / integration (push) Successful in 5m10s

Details

f6d1cf24f0

Nothing in Minstrel ever noticed a deleted file. The walk only visits
paths that exist, so a row whose file was gone was never scanned, never
errored, never counted — permanently invisible. classifyEvent ignores
fsnotify removals by design, and the safety-net scan is the same walk, so
it covers additions only. Rows accumulated forever.

Found on the operator's library: a completed scan reported
skipped=24185 errored=0 while the MBID backfill (which opens files by DB
path rather than walking) logged ~40 "no such file or directory" across
three reorganised albums. Those rows also kept their pre-#2499 welded
genre, which is how this surfaced — the version-stamped tag re-read can
only reach files the walk visits.

The harm is not cosmetic. tracks is the candidate universe for
recommendation.sql / discover.sql / system_mixes.sql and nothing filtered
on file existence, so a mix could spend a slot on a track that cannot
stream.

Marks rather than deletes. A missing file is a claim about the filesystem
and the filesystem lies transiently — an unmounted volume, a network
blip, a container that started before its media mount attached. Every
sweep in internal/gc resolves a truth INSIDE the database and is safe to
run blind; this one is not, so no deletion happens here. Three guards
refuse to act on ambiguous evidence: every scan root must resolve to a
non-empty directory, the walk must have seen at least one file, and one
reconcile may newly mark at most 25% of the library. Clearing a mark is
never the dangerous direction, so it runs unconditionally — otherwise a
library that tripped the cap could never recover once the mount returned.

Only a full Scan reconciles. The walk's set of seen paths is the
evidence, and ScanFiles has no basis for concluding anything about files
it did not look at.

Excludes marked tracks from all 13 track-emitting queries (radio x2,
system mixes x5, discover x4, most-played x2), the 6 play-history seed
picks, and the genre browse axis. Deliberately NOT filtered: the shared
ListPlaylistTracks read path, because it also serves user-curated
playlists where hiding a track the user added would be wrong — system
playlists shed orphans on their next daily rebuild instead. History and
the taste profile also keep them: those record the past, and a track you
played 200 times still says something about your taste.

Reconcile tallies land in scan_runs so a disappearance is visible rather
than discovered when a mix comes up short.

feat(library): adopt moved files instead of forking their history — #2528

test-go / test (push) Successful in 52s

Details

test-go / integration (push) Successful in 5m0s

Details

24d330424f

Track identity was file_path, so a file that came back renamed or in a
different directory looked like a deletion plus an unrelated new track:
the old row kept the like and every play_event while a fresh zero-history
row appeared, and nothing connected them. A liked song read as unliked, its
play count reset, and Rediscover could offer it as a discovery — silently.
Renumbering an album was enough, which is what happened to the operator's
copy of Minutes to Midnight.

Adoption re-points the existing row's file_path at the new location and
clears its missing mark. The normal UpsertTrack then conflicts on file_path
and updates THAT row, so the track id survives and likes, plays and
playlist memberships travel with it — and clients see an update rather than
a delete-and-create, so no cache churn either.

Matching is MBID first (identifies the recording, so it survives a
re-encode), then file_size + duration_ms for untagged files. Both
fingerprint components must be non-zero: duration_ms is 0 when ffprobe
failed, and matching 0 against 0 would pair up unrelated broken files.
Only rows already marked missing are eligible — a row whose file is present
elsewhere is a duplicate, not a move, and re-pointing it would corrupt the
copy that still exists. An ambiguous match inserts fresh rather than
adopting one arbitrarily: a fork is recoverable later, a wrong merge isn't.

Scan is now three phases, and the order is the point. Adoption can only
claim a row that is ALREADY marked missing, but reconcile previously ran
after processing — so a rename performed while the server was down surfaced
the deletion and the addition in the same scan, the new path inserted first,
and the fork became permanent. Enumeration is therefore separated from
processing so reconcile can run between them: walk (paths only, no tag
reads or probes) -> reconcile -> process in walk order.

Consequence worth knowing: when reconcile refuses (an absent root, or a
reorganisation exceeding the 25% mark cap) adoption cannot fire and renamed
files fork as before. That's the pre-#2528 behaviour rather than a new
failure, and the warning now names it.

The old outer walk-error branch was unreachable — the callback always
returned nil, so WalkDir never surfaced an error — and verifyRootsPresent is
the real protection, so enumerate counts walk errors instead of pretending
to abort on them.

bvandeusen merged commit a99f855e98 into main

2026-08-06 20:40:39 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/minstrel#121