Recommendation relevance, the rollback unit, and a version that names what shipped (#131)

Dockerfile

@@ -33,14 +33,8 @@ RUN go build -trimpath \
     -o /out/minstrel ./cmd/minstrel
 
 FROM debian:bookworm-slim
-# ffmpeg: duration probes and the exact-tier audio hash (a SHA-256 of the
-# encoded audio packets, so no decode). libchromaprint-tools: fpcalc, the
-# acoustic fingerprint that tells the same recording at two bitrates apart
-# from two different recordings (M400). Both are baked in at build time so a
-# deployed instance never fetches either (rule 164); fpcalc is shelled out
-# rather than bound because CGO_ENABLED=0 above rules out cgo.
 RUN apt-get update \
-    && apt-get install -y --no-install-recommends ca-certificates ffmpeg libchromaprint-tools \
+    && apt-get install -y --no-install-recommends ca-certificates ffmpeg \
     && rm -rf /var/lib/apt/lists/*
 
 RUN groupadd --system --gid 1000 minstrel \

README.md

@@ -37,12 +37,8 @@ services:
     ports: ['4533:4533']
     volumes:
       # Your music library. Point ./music at wherever your audio files
-      # live. Writable, because Minstrel deletes a file when an admin asks
+      # live. Mounted read-only — Minstrel never writes to your library.
-      # it to (for example, quarantine's "Delete file"). It never moves,
+      - ./music:/music:ro
-      # renames or retags anything. The container runs as uid 1000, so that
-      # user needs write access to the folders. Mount it :ro to forbid even
-      # deletes: those actions then refuse, say why, and delete nothing.
-      - ./music:/music
       # Generated data: playlist cover collages, artist art, caches.
       # The path must match MINSTREL_STORAGE_DATA_DIR, which the image
       # sets to /app/data — keep this mount on /app/data or your cache
@@ -51,7 +47,7 @@ services:
     environment:
       MINSTREL_DATABASE_URL: postgres://minstrel:minstrel@db:5432/minstrel?sslmode=disable
       # Colon-separated library roots to scan; must match the container
-      # path of the music mount above (/music here).
+      # path of the read-only music mount above (/music here).
       MINSTREL_LIBRARY_SCAN_PATHS: /music
     depends_on: [db]
 

android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt

@@ -15,14 +15,10 @@ import androidx.compose.material3.HorizontalDivider
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.OutlinedButton
 import androidx.compose.material3.Scaffold
-import androidx.compose.material3.SnackbarHost
-import androidx.compose.material3.SnackbarHostState
 import androidx.compose.material3.Text
 import androidx.compose.material3.TextButton
 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
-import androidx.compose.runtime.remember
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.text.style.TextOverflow
@@ -46,12 +42,6 @@ fun AdminQuarantineScreen(
     viewModel: AdminQuarantineViewModel = hiltViewModel(),
 ) {
     val state by viewModel.uiState.collectAsStateWithLifecycle()
-    val snackbarHostState = remember { SnackbarHostState() }
-    LaunchedEffect(Unit) {
-        viewModel.transientMessages.collect { msg ->
-            snackbarHostState.showSnackbar(msg)
-        }
-    }
     Scaffold(
         contentWindowInsets = ShellContentWindowInsets,
         modifier = Modifier.fillMaxSize(),
@@ -63,7 +53,6 @@ fun AdminQuarantineScreen(
                 onBack = { navController.popBackStack() },
             )
         },
-        snackbarHost = { SnackbarHost(snackbarHostState) },
     ) { inner ->
         PullToRefreshScaffold(
             onRefresh = { viewModel.refresh().join() },

android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt

@@ -10,13 +10,10 @@ import com.fabledsword.minstrel.events.EventsStream
 import com.fabledsword.minstrel.models.AdminQuarantineItemRef
 import dagger.hilt.android.lifecycle.HiltViewModel
 import kotlinx.coroutines.Job
-import kotlinx.coroutines.channels.Channel
-import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.filter
-import kotlinx.coroutines.flow.receiveAsFlow
 import kotlinx.coroutines.launch
 import javax.inject.Inject
 
@@ -37,15 +34,6 @@ class AdminQuarantineViewModel @Inject constructor(
     private val internal = MutableStateFlow<AdminQuarantineUiState>(AdminQuarantineUiState.Loading)
     val uiState: StateFlow<AdminQuarantineUiState> = internal.asStateFlow()
 
-    /**
-     * One-shot messages for the screen's snackbar. A failed action has to say
-     * why: the row quietly reappearing reads as a glitch, and for a Delete
-     * file refused by a read-only library it hides the one thing the
-     * operator can fix (#3918).
-     */
-    private val transientMessagesChannel = Channel<String>(Channel.BUFFERED)
-    val transientMessages: Flow<String> = transientMessagesChannel.receiveAsFlow()
-
     init {
         refresh()
         viewModelScope.launch {
@@ -98,9 +86,8 @@ class AdminQuarantineViewModel @Inject constructor(
             try {
                 action(trackId)
             } catch (
-                @Suppress("TooGenericExceptionCaught") e: Throwable,
+                @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable,
             ) {
-                transientMessagesChannel.trySend(ErrorCopy.fromThrowable(e))
                 refresh()
             }
         }

android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt

@@ -37,35 +37,18 @@ object ErrorCopy {
      * as connection failures.
      */
     fun fromThrowable(t: Throwable): String = when (t) {
-        is HttpException -> fromHttp(t)
+        is HttpException -> messageFor(codeFromHttp(t))
         is IOException -> messageFor("connection_refused")
         else -> TABLE.getValue("unknown")
     }
 
-    /**
+    private fun codeFromHttp(e: HttpException): String {
-     * Codes whose server message carries specifics the operator needs in
-     * order to act — which directory, which uid — that fixed copy cannot say.
-     * For these the message follows the copy (#3918). Kept to a named set on
-     * purpose: most server messages are internal detail. Mirrors web's
-     * errors.ts.
-     */
-    private val DETAIL_CODES = setOf("library_not_writable", "file_delete_failed")
-
-    private fun fromHttp(e: HttpException): String {
-        val body = bodyFromHttp(e)
-        val copy = messageFor(body.code.ifEmpty { "unknown" })
-        return if (body.code in DETAIL_CODES && body.message.isNotBlank()) {
-            "$copy ${body.message}"
-        } else {
-            copy
-        }
-    }
-
-    private fun bodyFromHttp(e: HttpException): Body {
         val raw = runCatching { e.response()?.errorBody()?.string() }.getOrNull()
-            ?: return Body()
+            ?: return "unknown"
-        return runCatching { json.decodeFromString<Envelope>(raw).error }
+        val code = runCatching { json.decodeFromString<Envelope>(raw).error?.code }
-            .getOrNull() ?: Body()
+            .getOrNull()
+            .orEmpty()
+        return code.ifEmpty { "unknown" }
     }
 
     private val TABLE: Map<String, String> = mapOf(
@@ -116,8 +99,6 @@ object ErrorCopy {
         "request_not_pending" to "This request is no longer pending.",
         "request_not_found" to "That request no longer exists.",
         "track_not_found" to "That track no longer exists.",
-        "library_not_writable" to "The music library isn't writable by the server.",
-        "file_delete_failed" to "The file couldn't be deleted.",
         "album_not_found" to "That album no longer exists.",
         "artist_not_found" to "That artist no longer exists.",
         "playlist_not_found" to "That playlist no longer exists.",

android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt

@@ -1,60 +0,0 @@
-package com.fabledsword.minstrel.api
-
-import okhttp3.MediaType.Companion.toMediaType
-import okhttp3.ResponseBody.Companion.toResponseBody
-import org.junit.jupiter.api.Assertions.assertEquals
-import org.junit.jupiter.api.Test
-import retrofit2.HttpException
-import retrofit2.Response
-import java.io.IOException
-
-class ErrorCopyTest {
-    private fun httpError(status: Int, body: String): HttpException =
-        HttpException(
-            Response.error<Unit>(status, body.toResponseBody("application/json".toMediaType())),
-        )
-
-    @Test
-    fun libraryNotWritableAppendsTheServerDetail() {
-        val detail = "Minstrel runs as uid 1000, gid 1000 and cannot delete from /music/A " +
-            "(read-only file system). The library mount must be writable by that user. " +
-            "Nothing was deleted."
-        val e = httpError(409, """{"error":{"code":"library_not_writable","message":"$detail"}}""")
-
-        assertEquals(
-            "${ErrorCopy.messageFor("library_not_writable")} $detail",
-            ErrorCopy.fromThrowable(e),
-        )
-    }
-
-    @Test
-    fun detailCodeWithoutAMessageShowsTheCopyAlone() {
-        val e = httpError(409, """{"error":{"code":"library_not_writable","message":""}}""")
-
-        assertEquals(ErrorCopy.messageFor("library_not_writable"), ErrorCopy.fromThrowable(e))
-    }
-
-    // Server messages are usually internal detail; appending them for every
-    // code would leak driver errors into snackbars. This pins the scope.
-    @Test
-    fun otherCodesNeverCarryTheServerMessage() {
-        val e = httpError(404, """{"error":{"code":"track_not_found","message":"pgx: no rows"}}""")
-
-        assertEquals(ErrorCopy.messageFor("track_not_found"), ErrorCopy.fromThrowable(e))
-    }
-
-    @Test
-    fun anUnparseableBodyFallsBackToUnknown() {
-        val e = httpError(500, "not json")
-
-        assertEquals(ErrorCopy.messageFor("unknown"), ErrorCopy.fromThrowable(e))
-    }
-
-    @Test
-    fun transportFailureMapsToConnectionRefused() {
-        assertEquals(
-            ErrorCopy.messageFor("connection_refused"),
-            ErrorCopy.fromThrowable(IOException("refused")),
-        )
-    }
-}

cmd/minstrel/main.go

@@ -214,12 +214,6 @@ func run() error {
 	// SQL, no external calls; empty on single-user servers.
 	go coplay.NewWorker(pool, logger.With("component", "coplay")).Run(ctx)
 
-	// Fingerprint backfill (M400 #3908): fingerprints the tracks the scan never
-	// will — everything imported before fingerprinting existed, and rows derived
-	// by an older method. A worker of its own rather than a scan stage; see
-	// internal/library/fingerprint_backfill.go for why.
-	go library.NewFingerprintBackfillWorker(pool, logger.With("component", "fingerprint_backfill")).Run(ctx)
-
 	// Start the tag-enrichment worker (#1490). Reconciles the compiled-in
 	// tag providers with tag_provider_settings, bumps the sources version if
 	// the provider set changed (re-opening settled rows), then drains tracks

internal/api/admin_coverage.go

@@ -5,7 +5,6 @@ import (
 
 	"git.fabledsword.com/bvandeusen/minstrel/internal/apierror"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
-	"git.fabledsword.com/bvandeusen/minstrel/internal/library"
 )
 
 // coverageRollupResp is the wire shape for GET /api/admin/library/coverage.
@@ -37,31 +36,3 @@ func (h *handlers) handleGetLibraryCoverage(w http.ResponseWriter, r *http.Reque
 		PendingNoMbid: row.PendingNoMbid,
 	})
 }
-
-// fingerprintCoverageResp is the wire shape for GET /api/admin/library/fingerprints.
-// fingerprinted + rejected + pending = total. Missing tracks are not counted:
-// there is no file to fingerprint.
-type fingerprintCoverageResp struct {
-	Total         int64 `json:"total"`
-	Fingerprinted int64 `json:"fingerprinted"`
-	Rejected      int64 `json:"rejected"`
-	Pending       int64 `json:"pending"`
-}
-
-// handleGetFingerprintCoverage implements GET /api/admin/library/fingerprints:
-// how far the fingerprint backfill (#3908) has got. The backfill is its own
-// worker spanning many passes, with no scan run to attach a tally to, so its
-// progress is read live here. Always 200; zeros on an empty library.
-func (h *handlers) handleGetFingerprintCoverage(w http.ResponseWriter, r *http.Request) {
-	row, err := library.FingerprintCoverage(r.Context(), h.pool)
-	if err != nil {
-		writeErrWithLog(w, h.logger, "admin: get fingerprint coverage", apierror.InternalMsg("lookup failed", err))
-		return
-	}
-	writeJSON(w, http.StatusOK, fingerprintCoverageResp{
-		Total:         row.Total,
-		Fingerprinted: row.Fingerprinted,
-		Rejected:      row.Rejected,
-		Pending:       row.Pending,
-	})
-}

internal/api/admin_quarantine.go

@@ -133,13 +133,6 @@ func (h *handlers) handleDeleteQuarantineFile(w http.ResponseWriter, r *http.Req
 	}
 	action, err := h.lidarrQuarantine.DeleteFile(r.Context(), id, admin.ID)
 	if err != nil {
-		// Written in the enveloped shape, not writeAdminJSONErr's bare code: the
-		// message is the part that tells the operator which directory and uid.
-		if apiErr, ok := fileRemoveAPIError(err); ok {
-			logFileRemoveFailure(h.logger, apiErr, "track_id", uuidToString(id))
-			writeErr(w, apiErr)
-			return
-		}
 		switch {
 		case errors.Is(err, lidarrquarantine.ErrTrackNotFound):
 			writeAdminJSONErr(w, http.StatusNotFound, "track_not_found")

internal/api/admin_quarantine_test.go

@@ -69,7 +69,7 @@ func installQuarantineClientFn(t *testing.T, h *handlers) {
 		}
 		return lidarr.NewClient(c.BaseURL, c.APIKey)
 	}
-	h.lidarrQuarantine = lidarrquarantine.NewService(h.pool, cfg, clientFn, h.dataDir)
+	h.lidarrQuarantine = lidarrquarantine.NewService(h.pool, cfg, clientFn)
 }
 
 // flagDirect bypasses the HTTP handler to seed a quarantine row via the

internal/api/admin_tracks.go

@@ -23,17 +23,15 @@ type removeTrackResponse struct {
 
 // handleRemoveTrack implements DELETE /api/admin/tracks/{id}?unmonitor=true|false.
 //
-// Admin-only (gated by auth.RequireAdmin on the /admin route group). Deletes the
+// Admin-only (gated by auth.RequireAdmin on the /admin route group). Always
-// file, then the DB row, and runs the album/artist cascade tidy-up — and deletes
+// deletes the file + DB row and runs the album/artist cascade tidy-up. When
-// nothing at all when the file cannot be removed (#3918). When
 // unmonitor=true and the track has an mbid, also calls Lidarr.UnmonitorTrack
 // — failure there is non-fatal (the destructive part already completed) and
 // surfaces as `lidarr_unmonitor_failed: true` in the success envelope.
 //
 // Per spec §5, Lidarr-side errors during the unmonitor step do NOT map to
-// wire error codes. The codes this handler emits are not_found,
+// wire error codes; the only error codes this handler emits are not_found,
-// library_not_writable (409) and file_delete_failed when the file could not be
+// server_error, plus the auth codes the middleware emits upstream.
-// removed, server_error, plus the auth codes the middleware emits upstream.
 func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) {
 	idStr := chi.URLParam(r, "id")
 	trackID, ok := parseUUID(idStr)
@@ -68,11 +66,6 @@ func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) {
 			writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "track not found"})
 			return
 		}
-		if apiErr, ok := fileRemoveAPIError(err); ok {
-			logFileRemoveFailure(h.logger, apiErr, "track_id", idStr)
-			writeErr(w, apiErr)
-			return
-		}
 		h.logger.Error("api: remove track failed", "err", err, "track_id", idStr)
 		writeErr(w, apierror.InternalMsg("remove failed", err))
 		return

internal/api/api.go

@@ -215,7 +215,6 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
 				admin.Get("/library/missing", h.handleListMissingTracks)
 
 				admin.Get("/library/coverage", h.handleGetLibraryCoverage)
-				admin.Get("/library/fingerprints", h.handleGetFingerprintCoverage)
 
 				admin.Get("/invites", h.handleListInvites)
 				admin.Post("/invites", h.handleCreateInvite)

internal/api/auth_test.go

@@ -65,7 +65,7 @@ func testHandlers(t *testing.T) (*handlers, *pgxpool.Pool) {
 	}
 	lidarrCfg := lidarrconfig.New(pool)
 	lidarrReqs := lidarrrequests.NewService(pool, lidarrCfg, nil, nil)
-	lidarrQuar := lidarrquarantine.NewService(pool, lidarrCfg, nil, "")
+	lidarrQuar := lidarrquarantine.NewService(pool, lidarrCfg, nil)
 	// tracks.Service has no Lidarr unmonitorer in tests by default; the
 	// admin-tracks tests below override h.tracks via installTracksLidarrStub
 	// when they need a stubbed Lidarr.

internal/api/file_remove_error.go

@@ -1,58 +0,0 @@
-package api
-
-import (
-	"errors"
-	"fmt"
-	"log/slog"
-	"net/http"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/apierror"
-	"git.fabledsword.com/bvandeusen/minstrel/internal/library"
-)
-
-// fileRemoveAPIError answers a delete that could not reach the track's file
-// (#3918). Both delete endpoints use it, so the operator gets the same
-// explanation from the admin remove-track action and from quarantine's Delete
-// file.
-//
-// The unwritable case is a 409 rather than a 500 because nothing is broken: the
-// request conflicts with how the library is mounted, and the fix is the
-// operator's. The message names the directory — removal writes to the parent,
-// not the file — and the uid/gid the process runs as, which is the half of a
-// permission problem invisible from the host. Every case says nothing was
-// deleted, because that is exactly what the operator will be worried about.
-func fileRemoveAPIError(err error) (*apierror.Error, bool) {
-	var fre *library.FileRemoveError
-	if !errors.As(err, &fre) {
-		return nil, false
-	}
-	if fre.NotWritable() {
-		return &apierror.Error{
-			Status: http.StatusConflict,
-			Code:   "library_not_writable",
-			Message: fmt.Sprintf(
-				"Minstrel runs as uid %d, gid %d and cannot delete from %s (%s). "+
-					"The library mount must be writable by that user. Nothing was deleted.",
-				fre.UID, fre.GID, fre.Dir(), fre.Reason()),
-			Cause: err,
-		}, true
-	}
-	return &apierror.Error{
-		Status:  http.StatusInternalServerError,
-		Code:    "file_delete_failed",
-		Message: fmt.Sprintf("Could not delete %s (%s). Nothing was deleted.", fre.Path, fre.Reason()),
-		Cause:   err,
-	}, true
-}
-
-// logFileRemoveFailure records a delete that could not reach its file. An
-// unwritable library is an environment fact the operator can fix, so it is a
-// Warn; anything else is a real fault.
-func logFileRemoveFailure(logger *slog.Logger, apiErr *apierror.Error, attrs ...any) {
-	attrs = append(attrs, "code", apiErr.Code, "err", apiErr.Cause)
-	if apiErr.Status == http.StatusConflict {
-		logger.Warn("api: track file could not be deleted", attrs...)
-		return
-	}
-	logger.Error("api: track file could not be deleted", attrs...)
-}

internal/api/file_remove_error_test.go

@@ -1,96 +0,0 @@
-package api
-
-import (
-	"errors"
-	"fmt"
-	"io/fs"
-	"net/http"
-	"strings"
-	"syscall"
-	"testing"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/library"
-)
-
-const removeTestPath = "/music/Moe Shop/WWW (2020)/01 - WWW.mp3"
-
-// removeFailure builds the error a delete service returns when the file would
-// not go, wrapped the way lidarrquarantine.DeleteFile and tracks.RemoveTrack
-// wrap it — the mapping has to see through that.
-func removeFailure(errno syscall.Errno) error {
-	return fmt.Errorf("delete file: %w", &library.FileRemoveError{
-		Path: removeTestPath, UID: 1000, GID: 1000,
-		Err: &fs.PathError{Op: "remove", Path: removeTestPath, Err: errno},
-	})
-}
-
-func TestFileRemoveAPIError(t *testing.T) {
-	cases := []struct {
-		name       string
-		errno      syscall.Errno
-		wantStatus int
-		wantCode   string
-		wantIn     []string
-	}{
-		{
-			name: "read-only mount", errno: syscall.EROFS,
-			wantStatus: http.StatusConflict, wantCode: "library_not_writable",
-			wantIn: []string{"uid 1000, gid 1000", "/music/Moe Shop/WWW (2020)", "read-only file system", "Nothing was deleted"},
-		},
-		{
-			name: "permission denied", errno: syscall.EACCES,
-			wantStatus: http.StatusConflict, wantCode: "library_not_writable",
-			wantIn: []string{"permission denied", "Nothing was deleted"},
-		},
-		{
-			name: "operation not permitted", errno: syscall.EPERM,
-			wantStatus: http.StatusConflict, wantCode: "library_not_writable",
-			wantIn: []string{"operation not permitted"},
-		},
-		{
-			name: "i/o error", errno: syscall.EIO,
-			wantStatus: http.StatusInternalServerError, wantCode: "file_delete_failed",
-			wantIn: []string{removeTestPath, "input/output error", "Nothing was deleted"},
-		},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			apiErr, ok := fileRemoveAPIError(removeFailure(tc.errno))
-			if !ok {
-				t.Fatal("a wrapped *library.FileRemoveError was not recognised")
-			}
-			if apiErr.Status != tc.wantStatus || apiErr.Code != tc.wantCode {
-				t.Fatalf("got %d %s, want %d %s", apiErr.Status, apiErr.Code, tc.wantStatus, tc.wantCode)
-			}
-			for _, want := range tc.wantIn {
-				if !strings.Contains(apiErr.Message, want) {
-					t.Errorf("message %q lacks %q", apiErr.Message, want)
-				}
-			}
-		})
-	}
-}
-
-// The unwritable answer must name the DIRECTORY. Removal needs write access to
-// the parent, so a message naming the file would send the operator to fix the
-// wrong permissions. The directory is a prefix of the file path, which is why a
-// plain "contains the directory" check could never catch that regression.
-func TestFileRemoveAPIError_NotWritableNamesTheDirectoryNotTheFile(t *testing.T) {
-	apiErr, _ := fileRemoveAPIError(removeFailure(syscall.EROFS))
-	if strings.Contains(apiErr.Message, "01 - WWW.mp3") {
-		t.Fatalf("message names the file rather than its directory: %q", apiErr.Message)
-	}
-}
-
-func TestFileRemoveAPIError_IgnoresOtherErrors(t *testing.T) {
-	for name, err := range map[string]error{
-		"nil":         nil,
-		"plain error": errors.New("delete track: connection reset"),
-		"path error":  &fs.PathError{Op: "remove", Path: removeTestPath, Err: syscall.EROFS},
-		"not found":   library.ErrTrackNotFound,
-	} {
-		if _, ok := fileRemoveAPIError(err); ok {
-			t.Errorf("%s: mapped as a file-remove failure", name)
-		}
-	}
-}

internal/api/radio.go

@@ -101,10 +101,6 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) {
 	candidates, err := recommendation.LoadCandidatesFromSimilarity(
 		r.Context(), q, user.ID, seedID,
 		h.recCfg.RecentlyPlayedHours, currentVec, exclude, limits,
-		// A fresh seed per request (#3889): radio is a new session each time
-		// and SHOULD draw differently. The system mixes are the surfaces that
-		// promise repeatability; this is not one of them.
-		strconv.FormatInt(time.Now().UnixNano(), 36),
 	)
 	if err != nil {
 		h.logger.Warn("api: radio: similarity-pool failed; falling back to whole-library", "err", err)

internal/db/dbq/fingerprints.sql.go

@@ -1,148 +0,0 @@
-// Code generated by sqlc. DO NOT EDIT.
-// versions:
-//   sqlc v1.31.1
-// source: fingerprints.sql
-
-package dbq
-
-import (
-	"context"
-
-	"github.com/jackc/pgx/v5/pgtype"
-)
-
-const deleteTrackFingerprint = `-- name: DeleteTrackFingerprint :exec
-DELETE FROM track_fingerprints WHERE track_id = $1
-`
-
-// A file changed but could not be fingerprinted, for a reason unrelated to the
-// file. The stored row describes the OLD bytes, so it goes and the backfill
-// re-derives it — nothing may keep trusting a stale identity.
-func (q *Queries) DeleteTrackFingerprint(ctx context.Context, trackID pgtype.UUID) error {
-	_, err := q.db.Exec(ctx, deleteTrackFingerprint, trackID)
-	return err
-}
-
-const getFingerprintCoverage = `-- name: GetFingerprintCoverage :one
-SELECT count(*)::bigint AS total,
-       count(*) FILTER (
-           WHERE f.fingerprint_version >= $1
-             AND f.audio_stream_sha256 IS NOT NULL AND f.chromaprint IS NOT NULL
-       )::bigint AS fingerprinted,
-       count(*) FILTER (
-           WHERE f.fingerprint_version >= $1
-             AND (f.audio_stream_sha256 IS NULL OR f.chromaprint IS NULL)
-       )::bigint AS rejected,
-       count(*) FILTER (
-           WHERE f.track_id IS NULL OR f.fingerprint_version < $1
-       )::bigint AS pending
-  FROM tracks t
-  LEFT JOIN track_fingerprints f ON f.track_id = t.id
- WHERE t.missing_since IS NULL
-`
-
-type GetFingerprintCoverageRow struct {
-	Total         int64
-	Fingerprinted int64
-	Rejected      int64
-	Pending       int64
-}
-
-// The admin gauge for the backfill. fingerprinted + rejected + pending = total.
-// rejected is a row at the current version with a NULL half: a tool ran and
-// refused the file, which is settled rather than waiting. Missing tracks are
-// excluded, or the gauge could never reach the end.
-func (q *Queries) GetFingerprintCoverage(ctx context.Context, currentVersion int16) (GetFingerprintCoverageRow, error) {
-	row := q.db.QueryRow(ctx, getFingerprintCoverage, currentVersion)
-	var i GetFingerprintCoverageRow
-	err := row.Scan(
-		&i.Total,
-		&i.Fingerprinted,
-		&i.Rejected,
-		&i.Pending,
-	)
-	return i, err
-}
-
-const listTracksNeedingFingerprint = `-- name: ListTracksNeedingFingerprint :many
-SELECT t.id, t.file_path
-  FROM tracks t
-  LEFT JOIN track_fingerprints f ON f.track_id = t.id
- WHERE t.missing_since IS NULL
-   AND (f.track_id IS NULL OR f.fingerprint_version < $1)
-   AND t.id > $2
- ORDER BY t.id
- LIMIT $3
-`
-
-type ListTracksNeedingFingerprintParams struct {
-	CurrentVersion int16
-	AfterID        pgtype.UUID
-	BatchLimit     int32
-}
-
-type ListTracksNeedingFingerprintRow struct {
-	ID       pgtype.UUID
-	FilePath string
-}
-
-// The backfill's work queue (#3908): tracks with no fingerprint, or one derived
-// by an older method. Keyset-paged on id so a pass visits each track at most
-// once. That cursor is load-bearing: an inconclusive attempt writes no row, so
-// without it a file that keeps timing out would be listed again straight away
-// and retried in a tight loop. Missing tracks are skipped — there is no file to
-// read.
-func (q *Queries) ListTracksNeedingFingerprint(ctx context.Context, arg ListTracksNeedingFingerprintParams) ([]ListTracksNeedingFingerprintRow, error) {
-	rows, err := q.db.Query(ctx, listTracksNeedingFingerprint, arg.CurrentVersion, arg.AfterID, arg.BatchLimit)
-	if err != nil {
-		return nil, err
-	}
-	defer rows.Close()
-	var items []ListTracksNeedingFingerprintRow
-	for rows.Next() {
-		var i ListTracksNeedingFingerprintRow
-		if err := rows.Scan(&i.ID, &i.FilePath); err != nil {
-			return nil, err
-		}
-		items = append(items, i)
-	}
-	if err := rows.Err(); err != nil {
-		return nil, err
-	}
-	return items, nil
-}
-
-const upsertTrackFingerprint = `-- name: UpsertTrackFingerprint :exec
-INSERT INTO track_fingerprints (
-    track_id, audio_stream_sha256, chromaprint, fingerprint_version
-) VALUES (
-    $1, $2, $3,
-    $4
-)
-ON CONFLICT (track_id) DO UPDATE SET
-    audio_stream_sha256 = EXCLUDED.audio_stream_sha256,
-    chromaprint         = EXCLUDED.chromaprint,
-    fingerprint_version = EXCLUDED.fingerprint_version,
-    computed_at         = now()
-`
-
-type UpsertTrackFingerprintParams struct {
-	TrackID            pgtype.UUID
-	AudioStreamSha256  []byte
-	Chromaprint        []int32
-	FingerprintVersion int16
-}
-
-// Written whenever a track's fingerprint is derived: by the scan when a file is
-// new or its bytes changed, and by the backfill (#3908) for rows derived by an
-// older method. Replaces the row wholesale — a fingerprint of the old bytes has
-// no standing once the file has changed.
-func (q *Queries) UpsertTrackFingerprint(ctx context.Context, arg UpsertTrackFingerprintParams) error {
-	_, err := q.db.Exec(ctx, upsertTrackFingerprint,
-		arg.TrackID,
-		arg.AudioStreamSha256,
-		arg.Chromaprint,
-		arg.FingerprintVersion,
-	)
-	return err
-}

internal/db/dbq/models.go

@@ -667,14 +667,6 @@ type Track struct {
 	MissingSince      pgtype.Timestamptz
 }
 
-type TrackFingerprint struct {
-	TrackID            pgtype.UUID
-	AudioStreamSha256  []byte
-	Chromaprint        []int32
-	FingerprintVersion int16
-	ComputedAt         pgtype.Timestamptz
-}
-
 type TrackSimilarity struct {
 	TrackAID  pgtype.UUID
 	TrackBID  pgtype.UUID

internal/db/dbq/recommendation.sql.go

@@ -829,7 +829,7 @@ similar_artists AS (
     JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id
     WHERE asim.source = 'listenbrainz'
       AND t.id NOT IN (SELECT id FROM excluded_ids)
-    ORDER BY asim.score DESC, md5(t.id::text || $12::text)
+    ORDER BY asim.score DESC, random()
     LIMIT $6
 ),
 tag_overlap AS (
@@ -857,7 +857,7 @@ likes_overlap AS (
           WHERE t.id = gl.track_id
             AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags)
       )
-    ORDER BY md5(gl.track_id::text || $12::text)
+    ORDER BY random()
     LIMIT $8
 ),
 taste_overlap AS (
@@ -884,7 +884,7 @@ coplay_artists AS (
     WHERE asim.source = 'user_cooccurrence'
       AND t.id NOT IN (SELECT id FROM excluded_ids)
       AND t.id <> $2
-    ORDER BY asim.score DESC, md5(t.id::text || $12::text)
+    ORDER BY asim.score DESC, random()
     LIMIT $11
 ),
 random_fill AS (
@@ -900,7 +900,7 @@ random_fill AS (
           UNION SELECT track_id FROM taste_overlap
           UNION SELECT track_id FROM coplay_artists
       )
-    ORDER BY md5(t.id::text || $12::text)
+    ORDER BY random()
     LIMIT $9
 )
 SELECT
@@ -949,7 +949,6 @@ type LoadRadioCandidatesV2Params struct {
 	Limit_5 int32
 	Limit_6 int32
 	Limit_7 int32
-	Column12 string
 }
 
 type LoadRadioCandidatesV2Row struct {
@@ -972,22 +971,8 @@ type LoadRadioCandidatesV2Row struct {
 //	enter the pool even when the similarity/random arms miss them; scored
 //	in Go via TasteMatch, so sim_score here is 0 pool-inclusion),
 //	$11 coplay_artists K (#1533 — tracks by artists co-played across the
-//	instance with the seed's artist; source='user_cooccurrence'),
+//	instance with the seed's artist; source='user_cooccurrence').
-//	$12 order_seed (text) — see below.
 //
-// $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned
-// a stable set only while their LIMIT exceeded the rows eligible for them: at
-// that point they returned all of them and the order stopped mattering,
-// because the caller sorts by track id before scoring. Below that threshold
-// they returned a random SUBSET, and two builds on the same day drew
-// different ones — so "daily determinism" held by accident, and only for
-// libraries smaller than the limits.
-//
-// md5(id || seed) keeps the intent — an arbitrary spread that changes when
-// the seed does — while making it reproducible for a given seed. The CALLER
-// decides what that means: system mixes pass a per-(user, day) string and get
-// the determinism they promise; radio passes a fresh value per request and
-// keeps varying, which is what a radio should do.
 // Returns same shape as LoadRadioCandidates plus similarity_score column.
 func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandidatesV2Params) ([]LoadRadioCandidatesV2Row, error) {
 	rows, err := q.db.Query(ctx, loadRadioCandidatesV2,
@@ -1002,7 +987,6 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid
 		arg.Limit_5,
 		arg.Limit_6,
 		arg.Limit_7,
-		arg.Column12,
 	)
 	if err != nil {
 		return nil, err

internal/db/migrations/0058_track_fingerprints.down.sql

@@ -1 +0,0 @@
-DROP TABLE track_fingerprints;

internal/db/migrations/0058_track_fingerprints.up.sql

@@ -1,38 +0,0 @@
--- 0058_track_fingerprints.up.sql — an acoustic identity per track (Scribe
--- milestone #400: #3905, #3906).
---
--- A table of its own rather than columns on tracks, for the hot path's sake:
--- tracks is read with SELECT * by eight queries, among them ListTracksByAlbum,
--- SearchTracks and GetTracksByIDs — album pages, search, the Subsonic surface.
--- A ~4 KB chromaprint column on tracks would be de-TOASTed on every one of
--- those reads to carry a value only the duplicate sweep ever looks at.
---
--- What a row means, which the backfill depends on:
---   no row                          never fingerprinted
---   fingerprint_version < current   derived by an older method; re-derive it
---   fingerprint_version = current   attempted; a NULL value means that tool
---                                   failed on this file, and it is not retried
---                                   until the file changes
--- A failure that says nothing about the file — a timeout, a cancelled scan, a
--- missing binary — writes no row at all, so the backfill tries again.
-CREATE TABLE track_fingerprints (
-    -- CASCADE is right here, unlike for the likes and play history M400's
-    -- merge has to carry across: a fingerprint describes one file's bytes and
-    -- means nothing once that file's row is gone.
-    track_id            uuid        PRIMARY KEY REFERENCES tracks (id) ON DELETE CASCADE,
-    -- SHA-256 of the ENCODED audio packets (ffmpeg -c:a copy -f hash), not of
-    -- decoded samples. internal/library/fingerprint.go says why.
-    audio_stream_sha256 bytea
-        CHECK (audio_stream_sha256 IS NULL OR octet_length(audio_stream_sha256) = 32),
-    -- fpcalc -raw -signed: the same 32 bits per item, stored signed because
-    -- integer is.
-    chromaprint         integer[],
-    fingerprint_version smallint    NOT NULL,
-    computed_at         timestamptz NOT NULL DEFAULT now()
-);
-
--- The exact duplicate tier is an equality match on this column. Partial
--- because a NULL is never looked up — it only means the hash was not taken.
-CREATE INDEX track_fingerprints_audio_stream_sha256
-    ON track_fingerprints (audio_stream_sha256)
-    WHERE audio_stream_sha256 IS NOT NULL;

internal/db/queries/fingerprints.sql

@@ -1,59 +0,0 @@
--- name: UpsertTrackFingerprint :exec
--- Written whenever a track's fingerprint is derived: by the scan when a file is
--- new or its bytes changed, and by the backfill (#3908) for rows derived by an
--- older method. Replaces the row wholesale — a fingerprint of the old bytes has
--- no standing once the file has changed.
-INSERT INTO track_fingerprints (
-    track_id, audio_stream_sha256, chromaprint, fingerprint_version
-) VALUES (
-    sqlc.arg(track_id), sqlc.narg(audio_stream_sha256), sqlc.narg(chromaprint),
-    sqlc.arg(fingerprint_version)
-)
-ON CONFLICT (track_id) DO UPDATE SET
-    audio_stream_sha256 = EXCLUDED.audio_stream_sha256,
-    chromaprint         = EXCLUDED.chromaprint,
-    fingerprint_version = EXCLUDED.fingerprint_version,
-    computed_at         = now();
-
--- name: DeleteTrackFingerprint :exec
--- A file changed but could not be fingerprinted, for a reason unrelated to the
--- file. The stored row describes the OLD bytes, so it goes and the backfill
--- re-derives it — nothing may keep trusting a stale identity.
-DELETE FROM track_fingerprints WHERE track_id = $1;
-
--- name: ListTracksNeedingFingerprint :many
--- The backfill's work queue (#3908): tracks with no fingerprint, or one derived
--- by an older method. Keyset-paged on id so a pass visits each track at most
--- once. That cursor is load-bearing: an inconclusive attempt writes no row, so
--- without it a file that keeps timing out would be listed again straight away
--- and retried in a tight loop. Missing tracks are skipped — there is no file to
--- read.
-SELECT t.id, t.file_path
-  FROM tracks t
-  LEFT JOIN track_fingerprints f ON f.track_id = t.id
- WHERE t.missing_since IS NULL
-   AND (f.track_id IS NULL OR f.fingerprint_version < sqlc.arg(current_version))
-   AND t.id > sqlc.arg(after_id)
- ORDER BY t.id
- LIMIT sqlc.arg(batch_limit);
-
--- name: GetFingerprintCoverage :one
--- The admin gauge for the backfill. fingerprinted + rejected + pending = total.
--- rejected is a row at the current version with a NULL half: a tool ran and
--- refused the file, which is settled rather than waiting. Missing tracks are
--- excluded, or the gauge could never reach the end.
-SELECT count(*)::bigint AS total,
-       count(*) FILTER (
-           WHERE f.fingerprint_version >= sqlc.arg(current_version)
-             AND f.audio_stream_sha256 IS NOT NULL AND f.chromaprint IS NOT NULL
-       )::bigint AS fingerprinted,
-       count(*) FILTER (
-           WHERE f.fingerprint_version >= sqlc.arg(current_version)
-             AND (f.audio_stream_sha256 IS NULL OR f.chromaprint IS NULL)
-       )::bigint AS rejected,
-       count(*) FILTER (
-           WHERE f.track_id IS NULL OR f.fingerprint_version < sqlc.arg(current_version)
-       )::bigint AS pending
-  FROM tracks t
-  LEFT JOIN track_fingerprints f ON f.track_id = t.id
- WHERE t.missing_since IS NULL;

internal/db/queries/recommendation.sql

@@ -45,22 +45,7 @@ WHERE t.id <> $2
 --   enter the pool even when the similarity/random arms miss them; scored
 --   in Go via TasteMatch, so sim_score here is 0 pool-inclusion),
 --   $11 coplay_artists K (#1533 — tracks by artists co-played across the
---   instance with the seed's artist; source='user_cooccurrence'),
+--   instance with the seed's artist; source='user_cooccurrence').
---   $12 order_seed (text) — see below.
---
--- $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned
--- a stable set only while their LIMIT exceeded the rows eligible for them: at
--- that point they returned all of them and the order stopped mattering,
--- because the caller sorts by track id before scoring. Below that threshold
--- they returned a random SUBSET, and two builds on the same day drew
--- different ones — so "daily determinism" held by accident, and only for
--- libraries smaller than the limits.
---
--- md5(id || seed) keeps the intent — an arbitrary spread that changes when
--- the seed does — while making it reproducible for a given seed. The CALLER
--- decides what that means: system mixes pass a per-(user, day) string and get
--- the determinism they promise; radio passes a fresh value per request and
--- keeps varying, which is what a radio should do.
 -- Returns same shape as LoadRadioCandidates plus similarity_score column.
 
 WITH
@@ -102,7 +87,7 @@ similar_artists AS (
     JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id
     WHERE asim.source = 'listenbrainz'
       AND t.id NOT IN (SELECT id FROM excluded_ids)
-    ORDER BY asim.score DESC, md5(t.id::text || $12::text)
+    ORDER BY asim.score DESC, random()
     LIMIT $6
 ),
 tag_overlap AS (
@@ -130,7 +115,7 @@ likes_overlap AS (
           WHERE t.id = gl.track_id
             AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags)
       )
-    ORDER BY md5(gl.track_id::text || $12::text)
+    ORDER BY random()
     LIMIT $8
 ),
 taste_overlap AS (
@@ -157,7 +142,7 @@ coplay_artists AS (
     WHERE asim.source = 'user_cooccurrence'
       AND t.id NOT IN (SELECT id FROM excluded_ids)
       AND t.id <> $2
-    ORDER BY asim.score DESC, md5(t.id::text || $12::text)
+    ORDER BY asim.score DESC, random()
     LIMIT $11
 ),
 random_fill AS (
@@ -173,7 +158,7 @@ random_fill AS (
           UNION SELECT track_id FROM taste_overlap
           UNION SELECT track_id FROM coplay_artists
       )
-    ORDER BY md5(t.id::text || $12::text)
+    ORDER BY random()
     LIMIT $9
 )
 SELECT

internal/dbtest/reset.go

@@ -87,7 +87,6 @@ var dataTables = []string{
 	// pristine Discover knobs rather than whatever a previous test tuned.
 	"discover_tuning",
 	"recommendation_tuning_audit",
-	"track_fingerprints", // M400
 	"tracks",
 	"albums",
 	"artists",

internal/library/delete.go

@@ -5,16 +5,12 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
-	"log/slog"
 	"os"
-	"path/filepath"
-	"syscall"
 
 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgtype"
 	"github.com/jackc/pgx/v5/pgxpool"
 
-	"git.fabledsword.com/bvandeusen/minstrel/internal/coverart"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
 	syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync"
 )
@@ -23,159 +19,54 @@ import (
 // that has no row in tracks.
 var ErrTrackNotFound = errors.New("library: track not found")
 
-// removeFile is os.Remove behind a variable so a test can make removal fail the
+// DeleteTrackFile removes a track file from disk and its row from the
-// way a read-only mount or a wrongly-owned directory does. A chmod-based test
+// tracks table. Album and artist rows are left untouched.
-// cannot stand in for that: root ignores permission bits, so in a CI container
-// running as root it would pass without ever exercising the failure.
-var removeFile = os.Remove
-
-// FileRemoveError reports that a track's file exists but could not be removed.
-// When DeleteTrackFile returns one, NOTHING was deleted: the row, its likes, its
-// play history and its playlist memberships are all intact.
-type FileRemoveError struct {
-	Path string
-	// UID and GID are the identity the server process runs as — the half of a
-	// permission problem the operator cannot see from the host side.
-	UID, GID int
-	Err      error
-}
-
-func (e *FileRemoveError) Error() string { return fmt.Sprintf("remove track file: %v", e.Err) }
-
-func (e *FileRemoveError) Unwrap() error { return e.Err }
-
-// Dir is the directory removal needs write access to. Unlinking a file writes to
-// its PARENT, so a world-writable file inside a read-only directory still cannot
-// be removed — naming the file's own permissions would send the operator to the
-// wrong place.
-func (e *FileRemoveError) Dir() string { return filepath.Dir(e.Path) }
-
-// NotWritable reports whether the library is unwritable for this process — a
-// read-only mount or a permission denial — rather than an I/O fault. It is the
-// case the operator can fix, so callers answer it differently.
-func (e *FileRemoveError) NotWritable() bool {
-	return errors.Is(e.Err, fs.ErrPermission) || errors.Is(e.Err, syscall.EROFS)
-}
-
-// Reason is the underlying cause without the path os.Remove already wrapped
-// around it, for messages that name the directory themselves.
-func (e *FileRemoveError) Reason() string {
-	var pathErr *fs.PathError
-	if errors.As(e.Err, &pathErr) {
-		return pathErr.Err.Error()
-	}
-	return e.Err.Error()
-}
-
-// DeletedTrack reports what a delete tidied away beyond the track itself.
-type DeletedTrack struct {
-	// AlbumID is set when the track was its album's last, so the album went too.
-	AlbumID *pgtype.UUID
-	// ArtistID is set when that album was its artist's last, so the artist went too.
-	ArtistID *pgtype.UUID
-}
-
-// DeleteTrackFile removes a track's file from disk and then its row, tidying
-// away an album or artist the delete leaves empty. It is the ONLY path that
-// deletes a track file: the admin remove-track endpoint and quarantine's Delete
-// file both come through here (#3918).
 //
-// Order is the whole contract. The file goes first, and if it cannot go — a
+// Steps:
-// read-only mount, a permission denial, an I/O error — nothing else happens and
+//  1. Look up the track to get its file_path.
-// a *FileRemoveError comes back. Proceeding past that failure is how #3918 lost
+//  2. Remove the file from disk. fs.ErrNotExist is OK — already gone.
-// history: tracks CASCADEs to play_events, general_likes, contextual_likes,
+//  3. Delete the tracks row.
-// playlist_tracks, track_tags and playback_errors, so the row and everything
-// hanging off it were destroyed while the file survived, and the next scan
-// re-imported it as a brand-new track with none of it.
 //
-// A file that is already gone (fs.ErrNotExist) is not a failure; the row is
+// Order matters: file first, then DB. If the file delete fails (permission,
-// removed as asked.
+// I/O error), we leave the DB row alone so the admin can retry.
 //
-// This is NOT the missing-file path. That lifecycle is deliberately
+// The reverse failure mode — file gone, DB row still present — IS reconciled
-// non-destructive: reconcile stamps missing_since (#2523), selection paths
+// now, and not by this function: the scan's reconcile pass stamps
-// filter on it, and a returning file is un-marked or adopted (#2528). This is the
+// tracks.missing_since (#2523), every selection path filters on it, and a file
-// explicit, irreversible "remove this recording", never the way to tidy up a row
+// that returns is un-marked or adopted at its new path (#2528). That is the
-// whose file merely went away.
+// normal life of a vanished file and it is deliberately non-destructive: the
+// row, its play history and its likes survive, because a missing file is a
+// track Minstrel still knows about (#2527).
 //
-// dataDir, when set, also clears the cached art of an artist the delete removed.
+// So this function is NOT the missing-file path. It is the explicit admin
-// logger may be nil.
+// action "remove this recording from disk and from the library", and it is
-func DeleteTrackFile(
+// irreversible: tracks CASCADEs to play_events, general_likes_tracks,
-	ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, trackID pgtype.UUID,
+// contextual_likes, track_tags and playback_errors. Reach for it when the
-) (DeletedTrack, error) {
+// operator means to destroy the record, never to tidy up a row whose file
-	if logger == nil {
+// merely went away.
-		logger = slog.Default()
+func DeleteTrackFile(ctx context.Context, pool *pgxpool.Pool, trackID pgtype.UUID) error {
-	}
 	q := dbq.New(pool)
 	track, err := q.GetTrackByID(ctx, trackID)
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
-			return DeletedTrack{}, ErrTrackNotFound
+			return ErrTrackNotFound
 		}
-		return DeletedTrack{}, fmt.Errorf("get track: %w", err)
+		return fmt.Errorf("get track: %w", err)
 	}
 
-	if err := removeFile(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) {
+	if err := os.Remove(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) {
-		return DeletedTrack{}, &FileRemoveError{
+		return fmt.Errorf("remove file: %w", err)
-			Path: track.FilePath, UID: os.Getuid(), GID: os.Getgid(), Err: err,
-		}
 	}
 
-	// The row and any album or artist it empties go together, so a failure
+	if _, err := pool.Exec(ctx, "DELETE FROM tracks WHERE id = $1", trackID); err != nil {
-	// partway cannot leave a deleted track with a ghost album behind it.
+		return fmt.Errorf("delete row: %w", err)
-	tx, err := pool.Begin(ctx)
-	if err != nil {
-		return DeletedTrack{}, fmt.Errorf("begin tx: %w", err)
 	}
-	defer func() { _ = tx.Rollback(ctx) }()
+	// Log the change after the delete succeeds. Best-effort: a Warn-level
-	tq := dbq.New(tx)
+	// failure here would leave the cache index orphaned on offline clients
-
+	// until the next scan touches the surrounding album.
-	deleted, err := tq.DeleteTrack(ctx, trackID)
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			// Removed by someone else between the lookup and here.
-			return DeletedTrack{}, ErrTrackNotFound
-		}
-		return DeletedTrack{}, fmt.Errorf("delete track: %w", err)
-	}
-
-	var out DeletedTrack
-	album, err := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID)
-	switch {
-	case err == nil:
-		albumID := album.ID
-		out.AlbumID = &albumID
-		artistID, aerr := tq.DeleteArtistIfEmpty(ctx, album.ArtistID)
-		switch {
-		case aerr == nil:
-			out.ArtistID = &artistID
-		case errors.Is(aerr, pgx.ErrNoRows):
-			// The artist still has other albums or stray tracks.
-		default:
-			return DeletedTrack{}, fmt.Errorf("delete artist if empty: %w", aerr)
-		}
-	case errors.Is(err, pgx.ErrNoRows):
-		// The album still has other tracks.
-	default:
-		return DeletedTrack{}, fmt.Errorf("delete album if empty: %w", err)
-	}
-
-	if err := tx.Commit(ctx); err != nil {
-		return DeletedTrack{}, fmt.Errorf("commit: %w", err)
-	}
-
-	// Both of these run after the delete has committed, so neither may fail
-	// it: the recording is gone either way. An unlogged change leaves the track
-	// in offline clients' caches until the next scan touches its album; a
-	// leftover art directory is only disk.
 	if err := syncpkg.LogChange(ctx, pool, syncpkg.EntityTrack,
 		syncpkg.FormatUUID(trackID), syncpkg.OpDelete); err != nil {
-		logger.Warn("track delete: LogChange failed", "track_id", syncpkg.FormatUUID(trackID), "err", err)
+		return fmt.Errorf("log change: %w", err)
 	}
-	if out.ArtistID != nil && dataDir != "" {
+	return nil
-		if err := coverart.CleanupArtistArt(dataDir, *out.ArtistID); err != nil {
-			logger.Warn("track delete: artist-art cleanup failed",
-				"artist_id", syncpkg.FormatUUID(*out.ArtistID), "err", err)
-		}
-	}
-	return out, nil
 }

internal/library/delete_test.go

@@ -4,11 +4,9 @@ import (
 	"context"
 	"errors"
 	"io"
-	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
-	"syscall"
 	"testing"
 
 	"github.com/jackc/pgx/v5/pgtype"
@@ -66,15 +64,6 @@ func seedTrack(t *testing.T, pool *pgxpool.Pool, filePath string) (dbq.Track, db
 	return track, album, artist
 }
 
-// stubRemoveFile makes file removal fail (or succeed) on demand for one test.
-// See removeFile for why this is a seam rather than a chmod.
-func stubRemoveFile(t *testing.T, fn func(string) error) {
-	t.Helper()
-	orig := removeFile
-	removeFile = fn
-	t.Cleanup(func() { removeFile = orig })
-}
-
 func TestDeleteTrackFile_HappyPath(t *testing.T) {
 	pool := newPool(t)
 	q := dbq.New(pool)
@@ -84,18 +73,9 @@ func TestDeleteTrackFile_HappyPath(t *testing.T) {
 	if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil {
 		t.Fatalf("write file: %v", err)
 	}
-	track, album, artist := seedTrack(t, pool, path)
+	track, album, _ := seedTrack(t, pool, path)
-	// A sibling keeps the album non-empty, so this case pins that the tidy-up
-	// only removes an album the delete actually emptied.
-	if _, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{
-		Title: "Sibling", AlbumID: album.ID, ArtistID: artist.ID,
-		DurationMs: 1000, FilePath: filepath.Join(dir, "sibling.mp3"), FileSize: 100, FileFormat: "mp3",
-	}); err != nil {
-		t.Fatalf("sibling: %v", err)
-	}
 
-	got, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID)
+	if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil {
-	if err != nil {
 		t.Fatalf("DeleteTrackFile: %v", err)
 	}
 
@@ -105,99 +85,9 @@ func TestDeleteTrackFile_HappyPath(t *testing.T) {
 	if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil {
 		t.Errorf("track row still exists")
 	}
+	// Album row preserved (other tracks may reference it).
 	if _, err := q.GetAlbumByID(context.Background(), album.ID); err != nil {
-		t.Errorf("album with a remaining track vanished: %v", err)
+		t.Errorf("album row vanished: %v", err)
-	}
-	if got.AlbumID != nil || got.ArtistID != nil {
-		t.Errorf("reported tidy-up %+v for an album that still has a track", got)
-	}
-}
-
-func TestDeleteTrackFile_EmptiedAlbumAndArtistGoToo(t *testing.T) {
-	pool := newPool(t)
-	q := dbq.New(pool)
-
-	path := filepath.Join(t.TempDir(), "lone.mp3")
-	if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil {
-		t.Fatalf("write file: %v", err)
-	}
-	track, album, artist := seedTrack(t, pool, path)
-
-	got, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID)
-	if err != nil {
-		t.Fatalf("DeleteTrackFile: %v", err)
-	}
-	if got.AlbumID == nil || *got.AlbumID != album.ID {
-		t.Errorf("AlbumID = %v, want %v", got.AlbumID, album.ID)
-	}
-	if got.ArtistID == nil || *got.ArtistID != artist.ID {
-		t.Errorf("ArtistID = %v, want %v", got.ArtistID, artist.ID)
-	}
-	if _, err := q.GetAlbumByID(context.Background(), album.ID); err == nil {
-		t.Errorf("emptied album row still exists")
-	}
-	if _, err := q.GetArtistByID(context.Background(), artist.ID); err == nil {
-		t.Errorf("emptied artist row still exists")
-	}
-}
-
-// The #3918 proof. A file that cannot be removed must leave EVERYTHING in place:
-// the row is what carries likes, plays and playlist memberships, and the file
-// surviving means the next scan would re-import it as a stranger.
-func TestDeleteTrackFile_UnremovableFileDeletesNothing(t *testing.T) {
-	cases := []struct {
-		name        string
-		errno       syscall.Errno
-		notWritable bool
-	}{
-		{"read-only mount", syscall.EROFS, true},
-		{"permission denied", syscall.EACCES, true},
-		{"operation not permitted", syscall.EPERM, true},
-		{"i/o error", syscall.EIO, false},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			pool := newPool(t)
-			q := dbq.New(pool)
-
-			dir := t.TempDir()
-			path := filepath.Join(dir, "track.mp3")
-			if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil {
-				t.Fatalf("write file: %v", err)
-			}
-			track, album, _ := seedTrack(t, pool, path)
-			stubRemoveFile(t, func(name string) error {
-				return &fs.PathError{Op: "remove", Path: name, Err: tc.errno}
-			})
-
-			_, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID)
-
-			var fre *FileRemoveError
-			if !errors.As(err, &fre) {
-				t.Fatalf("err = %v, want a *FileRemoveError", err)
-			}
-			if fre.NotWritable() != tc.notWritable {
-				t.Errorf("NotWritable = %v, want %v", fre.NotWritable(), tc.notWritable)
-			}
-			if fre.Dir() != dir {
-				t.Errorf("Dir = %q, want the parent directory %q", fre.Dir(), dir)
-			}
-			if fre.Reason() != tc.errno.Error() {
-				t.Errorf("Reason = %q, want %q", fre.Reason(), tc.errno.Error())
-			}
-			if fre.UID != os.Getuid() || fre.GID != os.Getgid() {
-				t.Errorf("identity = %d:%d, want this process's %d:%d", fre.UID, fre.GID, os.Getuid(), os.Getgid())
-			}
-			if _, err := q.GetTrackByID(context.Background(), track.ID); err != nil {
-				t.Errorf("track row was deleted although its file was not: %v", err)
-			}
-			if _, err := q.GetAlbumByID(context.Background(), album.ID); err != nil {
-				t.Errorf("album row was deleted although the track's file was not: %v", err)
-			}
-			if _, err := os.Stat(path); err != nil {
-				t.Errorf("file gone although removal was refused: %v", err)
-			}
-		})
 	}
 }
 
@@ -207,7 +97,7 @@ func TestDeleteTrackFile_FileAlreadyGoneSucceeds(t *testing.T) {
 
 	track, _, _ := seedTrack(t, pool, "/no/such/file/anywhere.mp3")
 
-	if _, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID); err != nil {
+	if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil {
 		t.Fatalf("DeleteTrackFile with missing file: %v", err)
 	}
 	if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil {
@@ -222,7 +112,7 @@ func TestDeleteTrackFile_NotFoundReturnsErr(t *testing.T) {
 	bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}
 	bogus.Valid = true
 
-	_, err := DeleteTrackFile(context.Background(), pool, nil, "", bogus)
+	err := DeleteTrackFile(context.Background(), pool, bogus)
 	if !errors.Is(err, ErrTrackNotFound) {
 		t.Errorf("err = %v, want ErrTrackNotFound", err)
 	}

internal/library/fingerprint.go

@@ -1,314 +0,0 @@
-package library
-
-import (
-	"bytes"
-	"context"
-	"crypto/sha256"
-	"encoding/hex"
-	"errors"
-	"fmt"
-	"log/slog"
-	"os/exec"
-	"strconv"
-	"strings"
-	"time"
-
-	"github.com/jackc/pgx/v5/pgtype"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
-)
-
-// Acoustic identity (M400).
-//
-// Two values per track, because they answer different questions:
-//
-//	audio_stream_sha256  a SHA-256 of the ENCODED audio packets. Equal means the
-//	                     same audio bytes, whatever the tags or container around
-//	                     them say. No threshold and no false positives — this is
-//	                     what catches two copies of one MP3 that differ only in
-//	                     their ID3 (#3885).
-//
-//	chromaprint          fpcalc's raw fingerprint. Close means the same
-//	                     recording, even at another bitrate or in another codec
-//	                     — the case an exact hash cannot see.
-//
-// Both shell out, in the shape probeDurationMs already set: a deadline on every
-// call, and a failure that leaves the value unset rather than failing the file.
-// A track with no fingerprint is never a duplicate candidate; it is still a
-// track.
-
-// fingerprintTimeout bounds one ffmpeg hash or fpcalc call. Longer than
-// probeTimeout because both read the audio rather than a header: the hash reads
-// every packet and fpcalc decodes up to its -length. 60s leaves room for a large
-// lossless file on a slow network mount; a call needing more is a stall, not a
-// big file.
-const fingerprintTimeout = 60 * time.Second
-
-// fingerprintWaitDelay bounds how long Output may keep waiting on the tool's
-// pipes after the deadline has killed it. Without it, a child that left a
-// descendant holding stdout open would block the scan past its own timeout.
-const fingerprintWaitDelay = 5 * time.Second
-
-// fingerprintVersion stamps how a track_fingerprints row was derived. Bump it
-// whenever the derivation changes — the hash arguments, fpcalc's flags or its
-// length — and the backfill re-derives every row below it. Fingerprints taken
-// by two methods are not comparable, and nothing else would reveal that the
-// library held a mix.
-const fingerprintVersion int16 = 1
-
-// errFingerprintTimeout marks a tool that ran out of time. Distinct from a
-// failed exit because a stall is a fact about the mount, not about the file.
-var errFingerprintTimeout = errors.New("fingerprint tool timed out")
-
-// defaultChromaprintLengthSec is how many seconds of audio fpcalc fingerprints.
-// 120 is fpcalc's own default. Fingerprints taken at different lengths are not
-// comparable, so changing this has to re-derive every stored one.
-const defaultChromaprintLengthSec = 120
-
-// fpcalcStderrTail caps how much of a failing tool's stderr reaches the log.
-const fpcalcStderrTail = 512
-
-// streamHashArgs hashes the encoded audio packets, never decoded samples.
-//
-// -c:a copy is the point, not an optimisation. A decoded hash of a lossy file
-// depends on the decoder's float maths and sample conversion, which can move
-// between ffmpeg releases — so an image upgrade could silently change every
-// stored hash, and yesterday's duplicate would stop matching today's copy.
-// Packet bytes do not move. It is also far cheaper: demux only, no decode.
-//
-// -map 0:a keeps embedded cover art (an attached-picture video stream) out of
-// the hash, so two copies of one recording carrying different art still match.
-func streamHashArgs(path string) []string {
-	return []string{
-		"-v", "error",
-		"-i", path,
-		"-map", "0:a",
-		"-c:a", "copy",
-		"-f", "hash", "-hash", "sha256",
-		"-",
-	}
-}
-
-// fpcalcArgs asks for the raw fingerprint as SIGNED integers.
-//
-// -raw because the matcher compares items bit by bit, which the compressed form
-// cannot do without being unpacked first. -signed because the column is Postgres
-// integer[], which is signed: fpcalc's default prints uint32, and half of those
-// values do not fit. Signed output is the same 32 bits with no reinterpretation
-// step left to get wrong.
-func fpcalcArgs(path string, lengthSec int) []string {
-	return []string{
-		"-raw", "-signed",
-		"-length", strconv.Itoa(lengthSec),
-		path,
-	}
-}
-
-// fingerprintResult is one attempt at both halves of a track's identity. They
-// fail independently: a file ffmpeg can demux may still defeat fpcalc.
-type fingerprintResult struct {
-	streamSHA256 []byte
-	chromaprint  []int32
-	hashErr      error
-	printErr     error
-}
-
-// computeFingerprint derives both halves for the file at path.
-func computeFingerprint(ctx context.Context, path string) fingerprintResult {
-	var r fingerprintResult
-	r.streamSHA256, r.hashErr = computeAudioStreamSHA256(ctx, path)
-	r.chromaprint, r.printErr = computeChromaprint(ctx, path, defaultChromaprintLengthSec)
-	return r
-}
-
-// inconclusive reports whether either half failed for a reason that says
-// nothing about the file. Such a result must never be stored: stamped at the
-// current version it would read as "tried, and this file cannot be
-// fingerprinted", and the backfill would never try it again.
-func (r fingerprintResult) inconclusive() bool {
-	return isInconclusive(r.hashErr) || isInconclusive(r.printErr)
-}
-
-// isInconclusive names the failures that are not a verdict on the file: a
-// stall, a cancelled scan, and a tool that is not installed. The last matters
-// outside the image — a dev binary run without fpcalc on PATH must not stamp
-// every track in the library as unfingerprintable.
-func isInconclusive(err error) bool {
-	return errors.Is(err, errFingerprintTimeout) ||
-		errors.Is(err, context.Canceled) ||
-		errors.Is(err, context.DeadlineExceeded) ||
-		errors.Is(err, exec.ErrNotFound)
-}
-
-// fingerprintFile runs the scanner's fingerprinter. A Scanner built without New
-// gets the real tools rather than a nil-func panic halfway through a scan.
-func (s *Scanner) fingerprintFile(ctx context.Context, path string) fingerprintResult {
-	if s.fingerprint == nil {
-		return computeFingerprint(ctx, path)
-	}
-	return s.fingerprint(ctx, path)
-}
-
-// fingerprintOutcome is what storeFingerprint did with one attempt.
-type fingerprintOutcome int
-
-const (
-	outcomeFingerprinted fingerprintOutcome = iota // both halves stored
-	outcomeRejected                                // stored with a NULL half: a verdict
-	outcomeInconclusive                            // nothing stored; worth trying again
-	outcomeStoreFailed                             // the write itself failed
-)
-
-// storeFingerprint records one attempt, for the scan (new or changed bytes) and
-// the backfill (#3908) alike, so there is one rule for what gets written. It
-// never fails its caller: a missing fingerprint only keeps a track out of
-// duplicate detection, which is not worth dropping a scan or a pass over.
-func storeFingerprint(
-	ctx context.Context, q *dbq.Queries, logger *slog.Logger,
-	trackID pgtype.UUID, path string, fp fingerprintResult,
-) fingerprintOutcome {
-	if fp.hashErr != nil {
-		logger.Warn("fingerprint: audio stream hash failed", "path", path, "err", fp.hashErr)
-	}
-	if fp.printErr != nil {
-		logger.Warn("fingerprint: chromaprint failed", "path", path, "err", fp.printErr)
-	}
-	if fp.inconclusive() {
-		// Any row this track holds describes bytes we could not confirm — the
-		// previous bytes for the scan, an older derivation for the backfill.
-		// Drop it rather than stamp a failure that says nothing about the file.
-		if err := q.DeleteTrackFingerprint(ctx, trackID); err != nil {
-			logger.Warn("fingerprint: clearing stale fingerprint failed", "path", path, "err", err)
-		}
-		return outcomeInconclusive
-	}
-	// A NULL half here is a verdict — the tool ran and rejected this file — and
-	// is stamped at the current version so the backfill does not retry it on
-	// every pass. It is retried when the file changes.
-	if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{
-		TrackID:            trackID,
-		AudioStreamSha256:  fp.streamSHA256,
-		Chromaprint:        fp.chromaprint,
-		FingerprintVersion: fingerprintVersion,
-	}); err != nil {
-		logger.Warn("fingerprint: storing fingerprint failed", "path", path, "err", err)
-		return outcomeStoreFailed
-	}
-	if fp.hashErr != nil || fp.printErr != nil {
-		return outcomeRejected
-	}
-	return outcomeFingerprinted
-}
-
-// computeAudioStreamSHA256 returns the SHA-256 of the file's encoded audio.
-func computeAudioStreamSHA256(ctx context.Context, path string) ([]byte, error) {
-	out, err := runFingerprintTool(ctx, "ffmpeg", streamHashArgs(path))
-	if err != nil {
-		return nil, err
-	}
-	return parseStreamHash(out)
-}
-
-// computeChromaprint returns the raw acoustic fingerprint of the first
-// lengthSec seconds of the file.
-func computeChromaprint(ctx context.Context, path string, lengthSec int) ([]int32, error) {
-	out, err := runFingerprintTool(ctx, "fpcalc", fpcalcArgs(path, lengthSec))
-	if err != nil {
-		return nil, err
-	}
-	return parseFpcalcRaw(out)
-}
-
-// runFingerprintTool runs one tool under fingerprintTimeout.
-//
-// Any non-zero exit is an error, and that deliberately includes fpcalc's exit 3:
-// "reading failed, but here is a fingerprint of what I got". A partial
-// fingerprint of a damaged file is not that file's identity. Stored, it would
-// score against a healthy copy over whatever prefix survived, and could group
-// or fail to group either way. Absent is better than wrong.
-func runFingerprintTool(ctx context.Context, name string, args []string) ([]byte, error) {
-	runCtx, cancel := context.WithTimeout(ctx, fingerprintTimeout)
-	defer cancel()
-
-	cmd := exec.CommandContext(runCtx, name, args...)
-	cmd.WaitDelay = fingerprintWaitDelay
-	out, err := cmd.Output()
-	if err == nil {
-		return out, nil
-	}
-	// The caller gave up (a cancelled scan). Report that rather than the
-	// signal-killed exit it caused, so it is never mistaken for a verdict on
-	// the file.
-	if ctx.Err() != nil {
-		return nil, fmt.Errorf("%s: %w", name, ctx.Err())
-	}
-	// Named separately so a stall reads as a stall, not as a crash.
-	if errors.Is(runCtx.Err(), context.DeadlineExceeded) {
-		return nil, fmt.Errorf("%s: no result within %s: %w", name, fingerprintTimeout, errFingerprintTimeout)
-	}
-	var exitErr *exec.ExitError
-	if errors.As(err, &exitErr) {
-		return nil, fmt.Errorf("%s exited %d: %s", name, exitErr.ExitCode(), stderrTail(exitErr.Stderr))
-	}
-	return nil, fmt.Errorf("%s: %w", name, err)
-}
-
-// stderrTail keeps the END of a failing tool's stderr. ffmpeg and fpcalc print
-// the actual reason last, after any banner or per-frame warnings, so a cap that
-// kept the head would log the noise and drop the cause.
-func stderrTail(stderr []byte) []byte {
-	stderr = bytes.TrimSpace(stderr)
-	if len(stderr) > fpcalcStderrTail {
-		stderr = stderr[len(stderr)-fpcalcStderrTail:]
-	}
-	return stderr
-}
-
-// parseStreamHash reads the ffmpeg hash muxer's "SHA256=<hex>" line.
-func parseStreamHash(out []byte) ([]byte, error) {
-	for _, line := range strings.Split(string(out), "\n") {
-		hexed, ok := strings.CutPrefix(strings.TrimSpace(line), "SHA256=")
-		if !ok {
-			continue
-		}
-		sum, err := hex.DecodeString(hexed)
-		if err != nil {
-			return nil, fmt.Errorf("stream hash %q: %w", hexed, err)
-		}
-		if len(sum) != sha256.Size {
-			return nil, fmt.Errorf("stream hash is %d bytes, want %d", len(sum), sha256.Size)
-		}
-		return sum, nil
-	}
-	return nil, errors.New("ffmpeg printed no SHA256= line")
-}
-
-// parseFpcalcRaw reads fpcalc's text output:
-//
-//	DURATION=<seconds>
-//	FINGERPRINT=<int32>,<int32>,...
-func parseFpcalcRaw(out []byte) ([]int32, error) {
-	for _, line := range strings.Split(string(out), "\n") {
-		list, ok := strings.CutPrefix(strings.TrimSpace(line), "FINGERPRINT=")
-		if !ok {
-			continue
-		}
-		if list == "" {
-			return nil, errors.New("fpcalc returned an empty fingerprint")
-		}
-		items := strings.Split(list, ",")
-		fp := make([]int32, len(items))
-		for i, item := range items {
-			// ParseInt at 32 bits, not ParseUint: a value past int32 means the
-			// output was unsigned — -signed went missing from the invocation —
-			// and nothing downstream would reinterpret it. Refuse it here.
-			v, err := strconv.ParseInt(item, 10, 32)
-			if err != nil {
-				return nil, fmt.Errorf("fingerprint item %d %q: %w", i, item, err)
-			}
-			fp[i] = int32(v)
-		}
-		return fp, nil
-	}
-	return nil, errors.New("fpcalc printed no FINGERPRINT= line")
-}

internal/library/fingerprint_backfill.go

@@ -1,196 +0,0 @@
-package library
-
-import (
-	"context"
-	"fmt"
-	"log/slog"
-	"sync"
-	"time"
-
-	"github.com/jackc/pgx/v5/pgtype"
-	"github.com/jackc/pgx/v5/pgxpool"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
-)
-
-// Fingerprint backfill (M400 #3908).
-//
-// The scan fingerprints only bytes it has not seen (see scanFile), so every track
-// imported before fingerprinting existed — and every row derived by an older
-// fingerprintVersion — needs a pass of its own. That pass is this worker.
-//
-// Its own worker rather than a stage in RunScan, for two reasons, both about
-// time:
-//   - RunScan runs at boot and then every safetyNetScanInterval (12h), and an
-//     in-flight scan older than StuckScanThreshold (1h) is reaped and a second
-//     one started beside it. A stage would have to stop well inside the hour — a
-//     few hundred decodes — so a 50k-track library would take about a month.
-//   - A long stage holds the scan run in flight, and a manual rescan answers 409
-//     for as long as it runs.
-//
-// Progress is read live (FingerprintCoverage, the admin gauge) rather than from a
-// scan_runs tally: the work spans many passes with no single run to attach to.
-
-// fingerprintBackfillTick is how often the worker looks for work. Once the
-// library has caught up, a tick is one indexed query; mostly the hour bounds how
-// long a file that timed out on a slow mount waits before it is tried again.
-const fingerprintBackfillTick = time.Hour
-
-// fingerprintBackfillBatch is how many tracks one query hands the worker. Small,
-// so tracks the scan adds mid-pass are not stuck behind one enormous page.
-const fingerprintBackfillBatch = 50
-
-// fingerprintBackfillConcurrency is how many files are decoded at once. Two is
-// deliberately low: fpcalc and the stream hash compete with playback transcoding
-// for CPU and with streaming for the mount, and a backfill that makes playback
-// stutter is worse than one that takes longer. Operator-tunable in #3913.
-const fingerprintBackfillConcurrency = 2
-
-// BackfillFingerprintsResult tallies one pass.
-type BackfillFingerprintsResult struct {
-	Processed     int
-	Fingerprinted int // both halves stored
-	Rejected      int // stored with a NULL half: a tool refused the file (settled)
-	Inconclusive  int // nothing stored; tried again on a later pass
-}
-
-func (r *BackfillFingerprintsResult) add(o fingerprintOutcome) {
-	r.Processed++
-	switch o {
-	case outcomeFingerprinted:
-		r.Fingerprinted++
-	case outcomeRejected:
-		r.Rejected++
-	default:
-		r.Inconclusive++
-	}
-}
-
-// FingerprintBackfillWorker fingerprints the tracks the scan never will.
-type FingerprintBackfillWorker struct {
-	pool        *pgxpool.Pool
-	logger      *slog.Logger
-	tick        time.Duration
-	batch       int32
-	concurrency int
-	// fingerprint is a field for the same reason as Scanner.fingerprint: an
-	// integration test pins which tracks a pass touches, not what the tools print.
-	fingerprint func(ctx context.Context, path string) fingerprintResult
-}
-
-// NewFingerprintBackfillWorker builds a worker with the production cadence.
-func NewFingerprintBackfillWorker(pool *pgxpool.Pool, logger *slog.Logger) *FingerprintBackfillWorker {
-	return &FingerprintBackfillWorker{
-		pool:        pool,
-		logger:      logger,
-		tick:        fingerprintBackfillTick,
-		batch:       fingerprintBackfillBatch,
-		concurrency: fingerprintBackfillConcurrency,
-		fingerprint: computeFingerprint,
-	}
-}
-
-// Run blocks until ctx is cancelled: one pass at start, so a fresh deploy does
-// not sit idle for an hour, then one per tick.
-func (w *FingerprintBackfillWorker) Run(ctx context.Context) {
-	w.runOnce(ctx)
-	t := time.NewTicker(w.tick)
-	defer t.Stop()
-	for {
-		select {
-		case <-ctx.Done():
-			return
-		case <-t.C:
-			w.runOnce(ctx)
-		}
-	}
-}
-
-// runOnce contains a pass so that nothing it does — an error, a panic — can stop
-// the next tick from firing (rule 157).
-func (w *FingerprintBackfillWorker) runOnce(ctx context.Context) {
-	defer func() {
-		if r := recover(); r != nil {
-			w.logger.Error("fingerprint backfill: pass panicked", "panic", r)
-		}
-	}()
-	res, err := w.pass(ctx)
-	if err != nil && ctx.Err() == nil {
-		w.logger.Warn("fingerprint backfill: pass failed", "err", err, "processed", res.Processed)
-	}
-	if res.Processed > 0 {
-		w.logger.Info("fingerprint backfill: pass complete",
-			"processed", res.Processed, "fingerprinted", res.Fingerprinted,
-			"rejected", res.Rejected, "inconclusive", res.Inconclusive)
-	}
-}
-
-// pass walks every track needing a fingerprint once, keyset-paged on id. The
-// cursor is what lets a pass end: an inconclusive attempt writes no row, so a
-// file that keeps timing out would otherwise be listed again immediately and
-// retried forever within the pass.
-func (w *FingerprintBackfillWorker) pass(ctx context.Context) (BackfillFingerprintsResult, error) {
-	q := dbq.New(w.pool)
-	var (
-		res BackfillFingerprintsResult
-		mu  sync.Mutex
-	)
-	// The all-zero uuid sorts before every real id. Valid must be true: a NULL
-	// cursor would make "id > NULL" match nothing and every pass a silent no-op.
-	after := pgtype.UUID{Valid: true}
-	for {
-		if err := ctx.Err(); err != nil {
-			return res, err
-		}
-		rows, err := q.ListTracksNeedingFingerprint(ctx, dbq.ListTracksNeedingFingerprintParams{
-			CurrentVersion: fingerprintVersion,
-			AfterID:        after,
-			BatchLimit:     w.batch,
-		})
-		if err != nil {
-			return res, fmt.Errorf("list tracks needing fingerprint: %w", err)
-		}
-		if len(rows) == 0 {
-			return res, nil
-		}
-
-		sem := make(chan struct{}, w.concurrency)
-		var wg sync.WaitGroup
-		for _, row := range rows {
-			if ctx.Err() != nil {
-				break
-			}
-			sem <- struct{}{}
-			wg.Add(1)
-			go func(trackID pgtype.UUID, path string) {
-				defer wg.Done()
-				defer func() { <-sem }()
-				defer func() {
-					if r := recover(); r != nil {
-						w.logger.Error("fingerprint backfill: track panicked", "path", path, "panic", r)
-					}
-				}()
-				outcome := storeFingerprint(ctx, q, w.logger, trackID, path, w.fingerprintFile(ctx, path))
-				mu.Lock()
-				res.add(outcome)
-				mu.Unlock()
-			}(row.ID, row.FilePath)
-		}
-		wg.Wait()
-		after = rows[len(rows)-1].ID
-	}
-}
-
-func (w *FingerprintBackfillWorker) fingerprintFile(ctx context.Context, path string) fingerprintResult {
-	if w.fingerprint == nil {
-		return computeFingerprint(ctx, path)
-	}
-	return w.fingerprint(ctx, path)
-}
-
-// FingerprintCoverage reports how much of the library carries a current
-// fingerprint, for the admin gauge. It lives here, beside the backfill, so the
-// version it counts against is the one the backfill writes.
-func FingerprintCoverage(ctx context.Context, pool *pgxpool.Pool) (dbq.GetFingerprintCoverageRow, error) {
-	return dbq.New(pool).GetFingerprintCoverage(ctx, fingerprintVersion)
-}

internal/library/fingerprint_backfill_test.go

@@ -1,156 +0,0 @@
-package library
-
-import (
-	"bytes"
-	"context"
-	"errors"
-	"fmt"
-	"io"
-	"log/slog"
-	"path/filepath"
-	"sync"
-	"testing"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
-)
-
-// TestFingerprintBackfill_Integration pins which tracks a pass touches, that a
-// pass ends, and that the coverage gauge counts what the pass wrote.
-func TestFingerprintBackfill_Integration(t *testing.T) {
-	pool := newPool(t)
-	ctx := context.Background()
-	q := dbq.New(pool)
-	dir := t.TempDir()
-
-	_, album, artist := seedTrack(t, pool, filepath.Join(dir, "unfingerprinted.mp3"))
-	addTrack := func(name string) dbq.Track {
-		t.Helper()
-		tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{
-			Title: name, AlbumID: album.ID, ArtistID: artist.ID,
-			DurationMs: 1000, FilePath: filepath.Join(dir, name+".mp3"), FileSize: 100, FileFormat: "mp3",
-		})
-		if err != nil {
-			t.Fatalf("track %s: %v", name, err)
-		}
-		return tr
-	}
-	current := addTrack("current")
-	stale := addTrack("stale")
-	missing := addTrack("missing")
-
-	sum := bytes.Repeat([]byte{0xCD}, 32)
-	for _, seed := range []struct {
-		track   dbq.Track
-		version int16
-	}{
-		{current, fingerprintVersion},
-		{stale, fingerprintVersion - 1},
-	} {
-		if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{
-			TrackID: seed.track.ID, AudioStreamSha256: sum, Chromaprint: []int32{1},
-			FingerprintVersion: seed.version,
-		}); err != nil {
-			t.Fatalf("seed fingerprint: %v", err)
-		}
-	}
-	if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() WHERE id = $1", missing.ID); err != nil {
-		t.Fatalf("mark missing: %v", err)
-	}
-
-	var mu sync.Mutex
-	calls := map[string]int{}
-	w := NewFingerprintBackfillWorker(pool, slog.New(slog.NewTextHandler(io.Discard, nil)))
-	// A batch of one forces the keyset cursor across several queries in a pass.
-	w.batch = 1
-	w.fingerprint = func(_ context.Context, path string) fingerprintResult {
-		name := filepath.Base(path)
-		mu.Lock()
-		calls[name]++
-		mu.Unlock()
-		switch name {
-		case "stall.mp3":
-			return fingerprintResult{streamSHA256: sum, printErr: fmt.Errorf("fpcalc: %w", errFingerprintTimeout)}
-		case "rejected.mp3":
-			return fingerprintResult{hashErr: errors.New("ffmpeg exited 1"), printErr: errors.New("fpcalc exited 2")}
-		default:
-			return fingerprintResult{streamSHA256: sum, chromaprint: []int32{7, -7}}
-		}
-	}
-	callCount := func(name string) int {
-		mu.Lock()
-		defer mu.Unlock()
-		return calls[name]
-	}
-
-	// 1. Only the track with no row and the stale one are fingerprinted — never
-	// the current one, never the missing one.
-	res, err := w.pass(ctx)
-	if err != nil {
-		t.Fatalf("first pass: %v", err)
-	}
-	if res.Processed != 2 || res.Fingerprinted != 2 {
-		t.Fatalf("first pass = %+v, want 2 processed, 2 fingerprinted", res)
-	}
-	for name, want := range map[string]int{
-		"unfingerprinted.mp3": 1, "stale.mp3": 1, "current.mp3": 0, "missing.mp3": 0,
-	} {
-		if got := callCount(name); got != want {
-			t.Errorf("%s fingerprinted %d times, want %d", name, got, want)
-		}
-	}
-
-	// 2. A pass after a complete one is a no-op. A backfill that redoes its work
-	// every hour is the expensive way this could be wrong.
-	res, err = w.pass(ctx)
-	if err != nil {
-		t.Fatalf("second pass: %v", err)
-	}
-	if res.Processed != 0 {
-		t.Fatalf("second pass processed %d tracks, want 0", res.Processed)
-	}
-
-	// 3. An inconclusive file is tried exactly once and the pass ENDS. Without the
-	// keyset cursor it would be re-listed immediately and this call would never
-	// return.
-	addTrack("stall")
-	addTrack("rejected")
-	res, err = w.pass(ctx)
-	if err != nil {
-		t.Fatalf("third pass: %v", err)
-	}
-	if res.Processed != 2 || res.Inconclusive != 1 || res.Rejected != 1 {
-		t.Fatalf("third pass = %+v, want 2 processed, 1 inconclusive, 1 rejected", res)
-	}
-	if got := callCount("stall.mp3"); got != 1 {
-		t.Fatalf("stalling file tried %d times in one pass, want exactly 1", got)
-	}
-
-	// 4. The gauge counts what the passes wrote, and its buckets add up.
-	cov, err := FingerprintCoverage(ctx, pool)
-	if err != nil {
-		t.Fatalf("coverage: %v", err)
-	}
-	// Five present tracks: unfingerprinted, current, stale, stall, rejected.
-	// The missing track is not counted.
-	if cov.Total != 5 || cov.Fingerprinted != 3 || cov.Rejected != 1 || cov.Pending != 1 {
-		t.Errorf("coverage = %+v, want total 5, fingerprinted 3, rejected 1, pending 1", cov)
-	}
-	if cov.Fingerprinted+cov.Rejected+cov.Pending != cov.Total {
-		t.Errorf("coverage buckets %+v do not sum to the total", cov)
-	}
-}
-
-func TestBackfillFingerprintsResult_Add(t *testing.T) {
-	var r BackfillFingerprintsResult
-	for _, o := range []fingerprintOutcome{
-		outcomeFingerprinted, outcomeFingerprinted, outcomeRejected, outcomeInconclusive, outcomeStoreFailed,
-	} {
-		r.add(o)
-	}
-	// A failed write stored nothing, so like an inconclusive attempt it is
-	// tried again next pass — and counts as such.
-	want := BackfillFingerprintsResult{Processed: 5, Fingerprinted: 2, Rejected: 1, Inconclusive: 2}
-	if r != want {
-		t.Errorf("tally = %+v, want %+v", r, want)
-	}
-}

internal/library/fingerprint_scan_test.go

@@ -1,174 +0,0 @@
-package library
-
-import (
-	"bytes"
-	"context"
-	"errors"
-	"fmt"
-	"io"
-	"log/slog"
-	"os"
-	"path/filepath"
-	"slices"
-	"testing"
-	"time"
-
-	"github.com/jackc/pgx/v5"
-	"github.com/jackc/pgx/v5/pgxpool"
-
-	"git.fabledsword.com/bvandeusen/minstrel/internal/db"
-)
-
-// TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration pins WHEN the scan
-// fingerprints. The cost of getting it wrong is asymmetric and invisible: a
-// scan that re-fingerprints unchanged files still produces correct rows, just
-// by decoding the entire library on every tag-repair pass.
-//
-// The fingerprinter is stubbed. CI has no real audio, and the tools' output is
-// covered by the parser tests; this covers the scan's decisions.
-func TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping scanner integration in -short mode")
-	}
-	dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL")
-	if dsn == "" {
-		t.Skip("MINSTREL_TEST_DATABASE_URL not set")
-	}
-	ctx := context.Background()
-	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
-
-	if err := db.Migrate(dsn, logger); err != nil {
-		t.Fatalf("migrate: %v", err)
-	}
-	pool, err := pgxpool.New(ctx, dsn)
-	if err != nil {
-		t.Fatalf("pool: %v", err)
-	}
-	t.Cleanup(pool.Close)
-	if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil {
-		t.Fatalf("truncate: %v", err)
-	}
-
-	root := t.TempDir()
-	a := filepath.Join(root, "artist/album/01.mp3")
-	b := filepath.Join(root, "artist/album/02.mp3")
-	writeTestMP3(t, a, map[string]string{"TIT2": "One", "TPE1": "Artist", "TALB": "Album", "TRCK": "1"})
-	writeTestMP3(t, b, map[string]string{"TIT2": "Two", "TPE1": "Artist", "TALB": "Album", "TRCK": "2"})
-
-	sum := bytes.Repeat([]byte{0xAB}, 32)
-	chroma := []int32{7, -7, 2147483647}
-	result := fingerprintResult{streamSHA256: sum, chromaprint: chroma}
-	calls := map[string]int{}
-
-	scanner := New(pool, logger, []string{root})
-	scanner.fingerprint = func(_ context.Context, path string) fingerprintResult {
-		calls[path]++
-		return result
-	}
-
-	scan := func(step string) Stats {
-		t.Helper()
-		st, err := scanner.Scan(ctx, nil)
-		if err != nil {
-			t.Fatalf("%s: scan: %v", step, err)
-		}
-		return st
-	}
-	type row struct {
-		sha     []byte
-		chroma  []int32
-		version int16
-	}
-	stored := func(path string) (row, bool) {
-		t.Helper()
-		var r row
-		err := pool.QueryRow(ctx, `
-			SELECT f.audio_stream_sha256, f.chromaprint, f.fingerprint_version
-			  FROM track_fingerprints f JOIN tracks t ON t.id = f.track_id
-			 WHERE t.file_path = $1`, path).Scan(&r.sha, &r.chroma, &r.version)
-		if errors.Is(err, pgx.ErrNoRows) {
-			return row{}, false
-		}
-		if err != nil {
-			t.Fatalf("read fingerprint for %s: %v", path, err)
-		}
-		return r, true
-	}
-	// A later step moves mtime forward past the row's updated_at, which is
-	// what the scan reads as "these bytes changed".
-	touch := func(path string, ahead time.Duration) {
-		t.Helper()
-		when := time.Now().Add(ahead)
-		if err := os.Chtimes(path, when, when); err != nil {
-			t.Fatalf("chtimes %s: %v", path, err)
-		}
-	}
-
-	// 1. New files are fingerprinted, and stored at the current version.
-	scan("first scan")
-	if calls[a] != 1 || calls[b] != 1 {
-		t.Fatalf("first scan fingerprint calls = %v, want one per file", calls)
-	}
-	got, ok := stored(a)
-	if !ok {
-		t.Fatal("first scan stored no fingerprint")
-	}
-	if !bytes.Equal(got.sha, sum) || !slices.Equal(got.chroma, chroma) || got.version != fingerprintVersion {
-		t.Fatalf("stored %+v, want sha %x chromaprint %v version %d", got, sum, chroma, fingerprintVersion)
-	}
-
-	// 2. A tag-repair pass re-reads every unchanged file and must not
-	// fingerprint any of them again.
-	//
-	// The Updated count is what makes this able to fail. Without it, a scan
-	// that simply SKIPPED both files would also leave the call counts at one,
-	// and the assertion would pass without the re-read path ever running.
-	if _, err := pool.Exec(ctx, "UPDATE tracks SET duration_ms = 1000, tag_read_version = 0"); err != nil {
-		t.Fatalf("force tag re-read: %v", err)
-	}
-	if st := scan("tag-repair scan"); st.Updated != 2 || st.Skipped != 0 {
-		t.Fatalf("tag-repair scan stats = %+v, want both files re-read (Updated=2 Skipped=0)", st)
-	}
-	if calls[a] != 1 || calls[b] != 1 {
-		t.Fatalf("tag-repair scan re-fingerprinted unchanged files: calls = %v", calls)
-	}
-	if _, ok := stored(a); !ok {
-		t.Fatal("tag-repair scan dropped a stored fingerprint")
-	}
-
-	// 3. Bytes that changed are fingerprinted again, and only those.
-	touch(a, time.Hour)
-	scan("changed-file scan")
-	if calls[a] != 2 || calls[b] != 1 {
-		t.Fatalf("changed-file scan calls = %v, want a=2 b=1", calls)
-	}
-
-	// 4. A changed file whose attempt is inconclusive loses its old row: that
-	// row describes the previous bytes, and a stall says nothing about the new
-	// ones.
-	result = fingerprintResult{streamSHA256: sum, printErr: fmt.Errorf("fpcalc: %w", errFingerprintTimeout)}
-	touch(a, 2*time.Hour)
-	scan("inconclusive scan")
-	if _, ok := stored(a); ok {
-		t.Fatal("inconclusive attempt left the previous bytes' fingerprint in place")
-	}
-	if _, ok := stored(b); !ok {
-		t.Fatal("inconclusive attempt on one file removed another file's fingerprint")
-	}
-
-	// 5. A file the tools reject gets a row at the current version with both
-	// halves NULL — a verdict, so the backfill does not retry it every boot.
-	result = fingerprintResult{
-		hashErr:  errors.New("ffmpeg exited 1"),
-		printErr: errors.New("fpcalc exited 2"),
-	}
-	touch(a, 3*time.Hour)
-	scan("rejected scan")
-	got, ok = stored(a)
-	if !ok {
-		t.Fatal("a file the tools rejected got no row, so the backfill would retry it forever")
-	}
-	if got.sha != nil || got.chroma != nil || got.version != fingerprintVersion {
-		t.Fatalf("rejected file stored %+v, want both halves NULL at version %d", got, fingerprintVersion)
-	}
-}

internal/library/fingerprint_test.go

@@ -1,194 +0,0 @@
-package library
-
-import (
-	"bytes"
-	"context"
-	"errors"
-	"fmt"
-	"os/exec"
-	"slices"
-	"strings"
-	"testing"
-)
-
-func TestParseFpcalcRaw(t *testing.T) {
-	cases := []struct {
-		name    string
-		out     string
-		want    []int32
-		wantErr string
-	}{
-		{
-			name: "signed output with negatives",
-			out:  "DURATION=213\nFINGERPRINT=-1453821711,17,0,2147483647,-2147483648\n",
-			want: []int32{-1453821711, 17, 0, 2147483647, -2147483648},
-		},
-		{
-			name: "fingerprint line need not come second",
-			out:  "FINGERPRINT=5,6\nDURATION=1\n",
-			want: []int32{5, 6},
-		},
-		{
-			// fpcalc's default is uint32. This value only appears when -signed
-			// is missing, and storing it would need a reinterpretation nothing
-			// performs.
-			name:    "unsigned output is refused",
-			out:     "DURATION=213\nFINGERPRINT=2841145585,17\n",
-			wantErr: "item 0",
-		},
-		{name: "empty fingerprint", out: "DURATION=0\nFINGERPRINT=\n", wantErr: "empty fingerprint"},
-		{name: "no fingerprint line", out: "DURATION=213\n", wantErr: "no FINGERPRINT= line"},
-		{name: "non-numeric item", out: "FINGERPRINT=1,x,3\n", wantErr: "item 1"},
-		{name: "trailing comma", out: "FINGERPRINT=1,2,\n", wantErr: "item 2"},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			got, err := parseFpcalcRaw([]byte(tc.out))
-			if tc.wantErr != "" {
-				if err == nil || !strings.Contains(err.Error(), tc.wantErr) {
-					t.Fatalf("err = %v, want one containing %q", err, tc.wantErr)
-				}
-				return
-			}
-			if err != nil {
-				t.Fatalf("unexpected err: %v", err)
-			}
-			if !slices.Equal(got, tc.want) {
-				t.Fatalf("got %v, want %v", got, tc.want)
-			}
-		})
-	}
-}
-
-func TestParseStreamHash(t *testing.T) {
-	// The real value ffmpeg printed for both files of the #3885 pair.
-	const www = "24e2daa3b4a534ff1a8d1a76f67810205869daf89f728d83a16625da4d28a18e"
-
-	got, err := parseStreamHash([]byte("SHA256=" + www + "\n"))
-	if err != nil {
-		t.Fatalf("unexpected err: %v", err)
-	}
-	if len(got) != 32 || got[0] != 0x24 || got[31] != 0x8e {
-		t.Fatalf("decoded %x, want %s", got, www)
-	}
-
-	for name, out := range map[string]string{
-		"no hash line":  "",
-		"other hash":    "MD5=" + www[:32] + "\n",
-		"not hex":       "SHA256=" + strings.Repeat("zz", 32) + "\n",
-		"short digest":  "SHA256=" + www[:62] + "\n",
-		"odd hex chars": "SHA256=" + www[:63] + "\n",
-	} {
-		if _, err := parseStreamHash([]byte(out)); err == nil {
-			t.Errorf("%s: parsed %q without error", name, out)
-		}
-	}
-}
-
-// followedBy reports whether flag appears in args immediately followed by value.
-func followedBy(args []string, flag, value string) bool {
-	for i := 0; i+1 < len(args); i++ {
-		if args[i] == flag && args[i+1] == value {
-			return true
-		}
-	}
-	return false
-}
-
-// The exact tier's stored hashes must stay comparable across ffmpeg upgrades,
-// which only holds while the packets are copied rather than decoded. A decoded
-// hash still matches within one ffmpeg build, so nothing else would notice the
-// change until an image upgrade silently broke every stored value.
-func TestStreamHashArgs_HashPacketsNotSamples(t *testing.T) {
-	args := streamHashArgs("/music/a.mp3")
-	for _, pair := range [][2]string{
-		{"-c:a", "copy"}, // no decode
-		{"-map", "0:a"},  // audio only: cover art stays out of the hash
-		{"-f", "hash"},   // the hash muxer, not a file
-		{"-hash", "sha256"},
-		{"-i", "/music/a.mp3"},
-	} {
-		if !followedBy(args, pair[0], pair[1]) {
-			t.Errorf("streamHashArgs lacks %s %s: %v", pair[0], pair[1], args)
-		}
-	}
-}
-
-func TestFpcalcArgs_RequestSignedRawOutput(t *testing.T) {
-	args := fpcalcArgs("/music/a.flac", 90)
-	for _, flag := range []string{"-raw", "-signed"} {
-		if !slices.Contains(args, flag) {
-			t.Errorf("fpcalcArgs lacks %s: %v", flag, args)
-		}
-	}
-	if !followedBy(args, "-length", "90") {
-		t.Errorf("fpcalcArgs does not pass the requested length: %v", args)
-	}
-	// fpcalc takes the file as its trailing positional argument.
-	if args[len(args)-1] != "/music/a.flac" {
-		t.Errorf("path is not last: %v", args)
-	}
-}
-
-func TestStderrTail_KeepsTheCauseNotTheBanner(t *testing.T) {
-	banner := bytes.Repeat([]byte("warning: skipping frame\n"), fpcalcStderrTail)
-	got := stderrTail(append(banner, []byte("ERROR: could not decode\n")...))
-	if len(got) != fpcalcStderrTail {
-		t.Fatalf("tail is %d bytes, want the %d-byte cap", len(got), fpcalcStderrTail)
-	}
-	if !bytes.HasSuffix(got, []byte("ERROR: could not decode")) {
-		t.Fatalf("tail dropped the final line: ...%q", got[len(got)-40:])
-	}
-
-	if got := stderrTail([]byte("  short  \n")); string(got) != "short" {
-		t.Fatalf("short stderr = %q, want it trimmed and whole", got)
-	}
-}
-
-// A stored failure is permanent until the file changes, so the classification
-// decides whether a track is ever retried. Every inconclusive case here would,
-// if misfiled as a verdict, silently exclude that track from duplicate
-// detection for good.
-func TestIsInconclusive(t *testing.T) {
-	notInstalled := fmt.Errorf("fpcalc: %w", &exec.Error{Name: "fpcalc", Err: exec.ErrNotFound})
-	cases := []struct {
-		name string
-		err  error
-		want bool
-	}{
-		{"timeout", fmt.Errorf("fpcalc: no result: %w", errFingerprintTimeout), true},
-		{"scan cancelled", fmt.Errorf("ffmpeg: %w", context.Canceled), true},
-		{"caller deadline", fmt.Errorf("ffmpeg: %w", context.DeadlineExceeded), true},
-		{"tool not installed", notInstalled, true},
-		{"tool rejected the file", errors.New("fpcalc exited 2: could not decode"), false},
-		{"unparseable output", errors.New("fpcalc printed no FINGERPRINT= line"), false},
-		{"success", nil, false},
-	}
-	for _, tc := range cases {
-		if got := isInconclusive(tc.err); got != tc.want {
-			t.Errorf("%s: isInconclusive = %v, want %v", tc.name, got, tc.want)
-		}
-	}
-}
-
-// Either half being inconclusive taints the whole result: storing the half that
-// succeeded would stamp the row at the current version with the other half
-// NULL, and that NULL would then read as a verdict.
-func TestFingerprintResult_InconclusiveIfEitherHalfIs(t *testing.T) {
-	stall := fmt.Errorf("fpcalc: %w", errFingerprintTimeout)
-	rejected := errors.New("fpcalc exited 2")
-	for name, tc := range map[string]struct {
-		r    fingerprintResult
-		want bool
-	}{
-		"both succeeded":            {fingerprintResult{streamSHA256: []byte{1}, chromaprint: []int32{1}}, false},
-		"hash ok, print stalled":    {fingerprintResult{streamSHA256: []byte{1}, printErr: stall}, true},
-		"hash stalled, print ok":    {fingerprintResult{hashErr: stall, chromaprint: []int32{1}}, true},
-		"hash ok, print rejected":   {fingerprintResult{streamSHA256: []byte{1}, printErr: rejected}, false},
-		"both rejected by the file": {fingerprintResult{hashErr: rejected, printErr: rejected}, false},
-	} {
-		if got := tc.r.inconclusive(); got != tc.want {
-			t.Errorf("%s: inconclusive = %v, want %v", name, got, tc.want)
-		}
-	}
-}

internal/library/scanner.go

@@ -75,15 +75,10 @@ type Scanner struct {
 	pool   *pgxpool.Pool
 	logger *slog.Logger
 	paths  []string
-	// fingerprint derives a file's acoustic identity (M400). A field so an
-	// integration test can substitute a deterministic one: CI has no real audio
-	// to fingerprint, and what the test pins is WHEN the scan fingerprints, not
-	// what the tools print. Call it through fingerprintFile.
-	fingerprint func(ctx context.Context, path string) fingerprintResult
 }
 
 func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner {
-	return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint}
+	return &Scanner{pool: pool, logger: logger, paths: paths}
 }
 
 // Scan walks every configured root and upserts any audio file whose mtime is
@@ -300,25 +295,6 @@ func (s *Scanner) scanFile(
 		durationMs = probed
 	}
 
-	// Fingerprint only bytes this row has not seen: a new path, or a file whose
-	// mtime moved past the row's. An unchanged file re-read for a tag repair
-	// keeps its stored fingerprint, for the same reason it keeps its duration
-	// above — a tagReadVersion bump must stay bound by tag reads, not become a
-	// decode of the whole library.
-	//
-	// An unchanged file with NO fingerprint yet is the backfill's job (#3908),
-	// deliberately not the scan's. Folding it into the skip check would make
-	// the first scan after an upgrade re-decode every track and push a sync
-	// change to every client for each one.
-	//
-	// Computed before move adoption so adoption can match on the audio hash
-	// (#3914); stored after the upsert, once the row id is known.
-	var fp fingerprintResult
-	fingerprinted := !unchanged
-	if fingerprinted {
-		fp = s.fingerprintFile(ctx, path)
-	}
-
 	// A path we've never seen might not be a new track — it might be one that
 	// moved or was renamed (#2528). Adopting re-points the existing row at this
 	// path and clears its missing mark, so the UpsertTrack below conflicts on
@@ -383,9 +359,6 @@ func (s *Scanner) scanFile(
 		// touches this track will re-emit the change.
 		s.logger.Warn("library scan: LogChange track upsert failed", "track_id", track.ID, "err", err)
 	}
-	if fingerprinted {
-		storeFingerprint(ctx, q, s.logger, track.ID, path, fp)
-	}
 
 	if knownTrack {
 		stats.Updated++

internal/lidarrquarantine/service.go

@@ -35,9 +35,6 @@ var (
 // config changes in lidarrconfig take effect immediately. clientFn returns
 // nil when Lidarr is disabled.
 type Service struct {
-	// dataDir lets a Delete file that empties an artist clear that artist's
-	// cached art, the same as the admin remove-track path.
-	dataDir   string
 	pool      *pgxpool.Pool
 	lidarrCfg *lidarrconfig.Service
 	clientFn  func() *lidarr.Client
@@ -45,11 +42,11 @@ type Service struct {
 
 // NewService constructs a Service. Pass nil for clientFn to disable the
 // Lidarr-using methods (DeleteViaLidarr will return ErrLidarrDisabled).
-func NewService(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client, dataDir string) *Service {
+func NewService(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client) *Service {
 	if clientFn == nil {
 		clientFn = func() *lidarr.Client { return nil }
 	}
-	return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn, dataDir: dataDir}
+	return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn}
 }
 
 // Flag inserts or updates a quarantine row for the caller. Re-flagging
@@ -248,13 +245,10 @@ func (s *Service) snapshot(ctx context.Context, q *dbq.Queries, track dbq.Track)
 	}, nil
 }
 
-// DeleteFile removes the track file from disk and the tracks row (tidying away
+// DeleteFile removes the track file from disk and the tracks row, then
-// an album or artist that leaves empty), then — via FK ON DELETE CASCADE —
+// (via FK ON DELETE CASCADE) clears all per-user quarantine rows for that
-// clears every per-user quarantine row for that track and writes an audit row.
+// track and writes an audit row. If the file deletion fails, the per-user
-//
+// rows stay so admin can retry. No partial state.
-// If the file cannot be removed, nothing is deleted and the per-user rows stay
-// so the admin can retry: the error wraps a *library.FileRemoveError, which the
-// handler turns into an answer naming the cause (#3918). No partial state.
 func (s *Service) DeleteFile(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, error) {
 	q := dbq.New(s.pool)
 	track, err := q.GetTrackByID(ctx, trackID)
@@ -273,7 +267,7 @@ func (s *Service) DeleteFile(ctx context.Context, trackID, adminID pgtype.UUID)
 	if err != nil {
 		return dbq.LidarrQuarantineAction{}, fmt.Errorf("count: %w", err)
 	}
-	if _, err := library.DeleteTrackFile(ctx, s.pool, nil, s.dataDir, trackID); err != nil {
+	if err := library.DeleteTrackFile(ctx, s.pool, trackID); err != nil {
 		return dbq.LidarrQuarantineAction{}, fmt.Errorf("delete file: %w", err)
 	}
 	// tracks row is gone; ON DELETE CASCADE on lidarr_quarantine.track_id

internal/lidarrquarantine/service_test.go

@@ -87,7 +87,7 @@ func TestFlag_HappyPath(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "Bad Track", "abc")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	row, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "crackly")
 	if err != nil {
 		t.Fatalf("Flag: %v", err)
@@ -105,7 +105,7 @@ func TestFlag_UpsertOnSecondFlag(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "first"); err != nil {
 		t.Fatalf("first flag: %v", err)
 	}
@@ -129,7 +129,7 @@ func TestFlag_NonexistentTrackReturnsErrTrackNotFound(t *testing.T) {
 	bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}
 	bogus.Valid = true
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	_, err := svc.Flag(context.Background(), user.ID, bogus, "bad_rip", "")
 	if !errors.Is(err, ErrTrackNotFound) {
 		t.Errorf("err = %v, want ErrTrackNotFound", err)
@@ -141,7 +141,7 @@ func TestFlag_BadReasonRejected(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	_, err := svc.Flag(context.Background(), user.ID, track.ID, "garbage", "")
 	if !errors.Is(err, ErrBadReason) {
 		t.Errorf("err = %v, want ErrBadReason", err)
@@ -153,7 +153,7 @@ func TestUnflag_DeletesRow(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", ""); err != nil {
 		t.Fatalf("Flag: %v", err)
 	}
@@ -171,7 +171,7 @@ func TestListMine_OrderedNewestFirst(t *testing.T) {
 	t1, _, _ := seedTrack(t, pool, "T1", "x")
 	t2, _, _ := seedTrack(t, pool, "T2", "y")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), user.ID, t1.ID, "bad_rip", ""); err != nil {
 		t.Fatalf("Flag t1: %v", err)
 	}
@@ -199,7 +199,7 @@ func TestListAdminQueue_AggregatesByTrackWithReasonCounts(t *testing.T) {
 	carol := seedUser(t, pool, "carol")
 	track, _, _ := seedTrack(t, pool, "Hot Mess", "abc")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), alice.ID, track.ID, "bad_rip", ""); err != nil {
 		t.Fatalf("alice flag: %v", err)
 	}
@@ -235,7 +235,7 @@ func TestResolve_ClearsRowsAndWritesAudit(t *testing.T) {
 	bob := seedUser(t, pool, "bob")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), alice.ID, track.ID, "bad_rip", ""); err != nil {
 		t.Fatalf("alice flag: %v", err)
 	}
@@ -267,7 +267,7 @@ func TestResolve_NoExistingRowsStillWritesAudit(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	// No flags applied — resolve a track with zero quarantine rows.
 	audit, err := svc.Resolve(context.Background(), track.ID, user.ID)
 	if err != nil {
@@ -291,7 +291,7 @@ func TestResolve_TrackNotFound(t *testing.T) {
 	bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}
 	bogus.Valid = true
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	_, err := svc.Resolve(context.Background(), bogus, user.ID)
 	if !errors.Is(err, ErrTrackNotFound) {
 		t.Errorf("err = %v, want ErrTrackNotFound", err)
@@ -315,7 +315,7 @@ func TestDeleteFile_RemovesFileAndAuditsAffected(t *testing.T) {
 		DurationMs: 1000, FilePath: path, FileSize: 1, FileFormat: "mp3",
 	})
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", ""); err != nil {
 		t.Fatalf("Flag: %v", err)
 	}
@@ -366,7 +366,7 @@ func TestDeleteViaLidarr_FullCascade(t *testing.T) {
 		t.Fatalf("save config: %v", err)
 	}
 	clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") }
-	svc := NewService(pool, cfg, clientFn, "")
+	svc := NewService(pool, cfg, clientFn)
 
 	q := dbq.New(pool)
 	artist, _ := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{Name: "A", SortName: "A"})
@@ -424,7 +424,7 @@ func TestDeleteViaLidarr_LidarrDisabled(t *testing.T) {
 	user := seedUser(t, pool, "alice")
 	track, _, _ := seedTrack(t, pool, "T", "x")
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	_, _, err := svc.DeleteViaLidarr(context.Background(), track.ID, user.ID)
 	if !errors.Is(err, ErrLidarrDisabled) {
 		t.Errorf("err = %v, want ErrLidarrDisabled", err)
@@ -456,7 +456,7 @@ func TestDeleteViaLidarr_AlbumMBIDMissing(t *testing.T) {
 	cfg := lidarrconfig.New(pool)
 	_ = cfg.Save(context.Background(), lidarrconfig.Config{Enabled: true, BaseURL: stub.URL, APIKey: "k"})
 	clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") }
-	svc := NewService(pool, cfg, clientFn, "")
+	svc := NewService(pool, cfg, clientFn)
 	_, _ = svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "")
 
 	_, _, err := svc.DeleteViaLidarr(context.Background(), track.ID, user.ID)
@@ -481,7 +481,7 @@ func TestDeleteViaLidarr_LidarrAlbumNotFound(t *testing.T) {
 	cfg := lidarrconfig.New(pool)
 	_ = cfg.Save(context.Background(), lidarrconfig.Config{Enabled: true, BaseURL: stub.URL, APIKey: "k"})
 	clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") }
-	svc := NewService(pool, cfg, clientFn, "")
+	svc := NewService(pool, cfg, clientFn)
 
 	track, _, _ := seedTrack(t, pool, "T", "x")
 	_, _ = svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "")
@@ -499,7 +499,7 @@ func TestDeleteFile_TrackNotFound(t *testing.T) {
 	bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}
 	bogus.Valid = true
 
-	svc := NewService(pool, lidarrconfig.New(pool), nil, "")
+	svc := NewService(pool, lidarrconfig.New(pool), nil)
 	_, err := svc.DeleteFile(context.Background(), bogus, user.ID)
 	if !errors.Is(err, ErrTrackNotFound) {
 		t.Errorf("err = %v, want ErrTrackNotFound", err)
@@ -521,7 +521,7 @@ func TestDeleteViaLidarr_TrackNotFound(t *testing.T) {
 		t.Fatalf("save config: %v", err)
 	}
 	clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") }
-	svc := NewService(pool, cfg, clientFn, "")
+	svc := NewService(pool, cfg, clientFn)
 
 	var bogus pgtype.UUID
 	bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}

internal/playlists/system.go

@@ -276,17 +276,6 @@ func SetTasteConfig(c taste.Config) {
 	systemTasteConfig = c
 }
 
-// dailyOrderSeed is the value the randomised candidate arms order by (#3889).
-//
-// Per (user, day) so a same-day rebuild draws the SAME set — which is what
-// TestBuildSystemPlaylists_DailyNonceDeterminism asserts and what those arms
-// only ever achieved by accident before, when their limits happened to exceed
-// the eligible rows. It changes on the day boundary, so the mixes still move
-// daily.
-func dailyOrderSeed(userID pgtype.UUID, dateStr string) string {
-	return uuidStringPL(userID) + ":" + dateStr
-}
-
 func currentSongsLikeWeights() recommendation.ScoringWeights {
 	systemTuningMu.RLock()
 	defer systemTuningMu.RUnlock()
@@ -640,7 +629,6 @@ func produceForYou(
 			zeroVec,
 			seeds,
 			systemForYouSourceLimits(),
-			dailyOrderSeed(userID, dateStr),
 		)
 		if cerr != nil {
 			logger.Warn("system playlist: for-you candidates load failed for seed; continuing",
@@ -728,7 +716,6 @@ func produceSeedMixes(
 			recommendation.ScaleForLibrary(
 				recommendation.SongsLikeCandidateSourceLimits(), librarySize,
 			),
-			dailyOrderSeed(userID, dateStr),
 		)
 		if cerr != nil {
 			logger.Warn("system playlist: seed candidates load failed; skipping",

internal/playlists/you_might_like.go

@@ -102,7 +102,6 @@ func buildYouMightLike(
 	cands, err := recommendation.LoadCandidatesFromSimilarity(
 		ctx, q, userID, seed, 1, zeroVec,
 		[]pgtype.UUID{seed}, ymlLimits,
-		dailyOrderSeed(userID, dateStr),
 	)
 	if err != nil {
 		logger.Warn("you-might-like: candidate load failed; skipping",

internal/recommendation/candidates.go

@@ -139,41 +139,46 @@ func DefaultCandidateSourceLimits() CandidateSourceLimits {
 // would produce a short mix or none at all, and "no playlist" is a worse
 // answer than "a few tracks further from the seed than we would like".
 //
-// The seed-independent arms are trimmed hardest, because on this surface they
+// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint
-// are noise: `taste_overlap` (tracks by the user's top taste artists) and
+// that shapes the numbers below, and it is not obvious from reading them.
-// `random_fill` (any track not already in the pool) both carry
-// `0.0::float8 AS sim_score`, so nearly a third of the default pool had no
-// relationship to the seed at all.
 //
-// THESE TRIMS WERE BLOCKED UNTIL #3889. `likes_overlap` and `random_fill`
+// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()`
-// used to end in a bare `ORDER BY random()`, which made their output a stable
+// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a
-// SET only while the limit exceeded the eligible rows — so SHRINKING them
+// STABLE set only while its LIMIT exceeds the rows eligible for it — at that
-// changed pool membership between same-day rebuilds and broke daily
+// point it returns all of them and the random order is irrelevant, because
-// determinism. Those arms now order by md5(id || seed), so a smaller limit
+// the caller sorts by id before scoring. Drop the limit below the eligible
-// takes a smaller but REPRODUCIBLE slice, and the trim is safe.
+// count and the arm starts returning a random SUBSET, which differs between
+// two builds on the same day.
 //
-// Reduced, never removed. Rule 131: the two seed-independent arms are the
+// That is a real defect (#3889) rather than a quirk of this function, and it
-// tier-3 floor, and zeroing them would leave a seed with thin ListenBrainz
+// bit here: cutting RandomFill to 10 broke
-// coverage producing a short mix or none at all. The weights (SimilarityWeight
+// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller
-// 4.0, everything seed-independent demoted) keep them ranked last, so they
+// than the default limit and whose determinism was therefore accidental.
-// surface only when the closer tiers cannot fill the mix.
+// Growing an arm is always safe; only shrinking one is.
 //
-// likes_overlap is cut hardest of the tier-2 arms for a specific reason: its
+// So the seed-independent arms are trimmed only where the ordering is
-// SQL assigns a FLAT 0.6 sim_score (recommendation.sql) rather than measuring
+// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be
-// anything. It is a collaborative signal wearing similarity's clothes, and a
+// cut, `random_fill` cannot. The reduction is consequently modest — and it
-// raised SimilarityWeight amplifies it — if real ListenBrainz scores commonly
+// matters less than it looks, because the WEIGHTS are what demote sim_score-0
-// land below 0.6 it would outrank genuine matches. Halved pending the
+// candidates now. The pool change biases the draw; the songs_like profile is
-// fill-rate measurement in #3879; the honest fix is to stop it claiming a
+// what actually keeps unrelated tracks out of the result.
-// similarity score it never computed.
+//
+// One arm is left alone that arguably should not be: `likes_overlap` assigns
+// a FLAT 0.6 sim_score (recommendation.sql:108) rather than measuring
+// anything — a collaborative signal wearing similarity's clothes, which a
+// raised SimilarityWeight amplifies. If real ListenBrainz scores commonly
+// land below 0.6 it will outrank genuine matches. It cannot be trimmed here
+// without the determinism fix landing first; the honest repair is to stop it
+// claiming a similarity score it never computed (#3879).
 func SongsLikeCandidateSourceLimits() CandidateSourceLimits {
 	return CandidateSourceLimits{
 		LBSimilar:     60, // tier 1 — doubled; the only arm that measures the seed
 		SimilarArtist: 40, // tier 2 — raised; growing is always safe
 		TagOverlap:    20, // tier 2
 		UserCoplay:    20, // tier 2
-		LikesOverlap:  10, // tier 2, halved — flat 0.6 sim_score, see above
+		LikesOverlap:  20, // tier 2 — NOT trimmed: unseeded random(), see above
-		TasteOverlap:  10, // tier 3 floor — halved, not removed
+		TasteOverlap:  10, // tier 3 floor — halved; deterministic ordering, safe
-		RandomFill:    10, // tier 3 floor — cut hard, never to zero
+		RandomFill:    30, // tier 3 floor — NOT trimmed: unseeded random(), see above
 	}
 }
 
@@ -182,10 +187,6 @@ func SongsLikeCandidateSourceLimits() CandidateSourceLimits {
 // likes-overlap / random fill) + dedup-by-max sim_score. Returns
 // []Candidate (same shape as LoadCandidates) so Shuffle is unchanged.
 //
-// orderSeed decides whether the randomised arms repeat their draw — see
-// Column12 below and #3889. Pass a stable per-(user, day) value where the
-// selection must be reproducible, and a varying one where it should not be.
-//
 // Caller (radio handler) falls back to LoadCandidates on error.
 func LoadCandidatesFromSimilarity(
 	ctx context.Context,
@@ -195,7 +196,6 @@ func LoadCandidatesFromSimilarity(
 	currentVector SessionVector,
 	exclude []pgtype.UUID,
 	limits CandidateSourceLimits,
-	orderSeed string,
 ) ([]Candidate, error) {
 	if exclude == nil {
 		exclude = []pgtype.UUID{}
@@ -212,12 +212,6 @@ func LoadCandidatesFromSimilarity(
 		Limit_5: int32(limits.RandomFill),
 		Limit_6: int32(limits.TasteOverlap),
 		Limit_7: int32(limits.UserCoplay),
-		// #3889. Four arms used to end in a bare ORDER BY random(), which made
-		// their output a stable SET only while the limit exceeded the eligible
-		// rows. They now order by md5(id || this), so the caller decides
-		// whether the draw repeats: a per-(user, day) seed for the system
-		// mixes that promise daily determinism, a fresh one per radio request.
-		Column12: orderSeed,
 	})
 	if err != nil {
 		return nil, err

internal/recommendation/candidates_v2_test.go

@@ -2,10 +2,6 @@ package recommendation
 
 import (
 	"context"
-	"fmt"
-	"reflect"
-	"sort"
-	"strings"
 	"testing"
 
 	"github.com/jackc/pgx/v5/pgtype"
@@ -54,7 +50,7 @@ func TestLoadCandidatesFromSimilarity_LBSimilarSourceContributes(t *testing.T) {
 	target := f.tracks[1]
 	helperLBSimilarity(t, f, seed.ID, target.ID, 0.85)
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -86,7 +82,7 @@ func TestLoadCandidatesFromSimilarity_SimilarArtistTracksContribute(t *testing.T
 	})
 	helperArtistSimilarity(t, f, seed.ArtistID, otherArtist.ID, 0.8)
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -110,7 +106,7 @@ func TestLoadCandidatesFromSimilarity_TagOverlapContributes(t *testing.T) {
 	helperSetTrackGenre(t, f, seed.ID, "Rock; Pop")
 	helperSetTrackGenre(t, f, target.ID, "Rock")
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -137,7 +133,7 @@ func TestLoadCandidatesFromSimilarity_LikesOverlapContributes(t *testing.T) {
 		t.Fatalf("like: %v", err)
 	}
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -159,7 +155,7 @@ func TestLoadCandidatesFromSimilarity_RandomFillReturnsTracks(t *testing.T) {
 	f := newFixture(t, 10) // 10 tracks; no similarity data
 	seed := f.tracks[0]
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -180,7 +176,7 @@ func TestLoadCandidatesFromSimilarity_ExcludeListRespected(t *testing.T) {
 	excluded := f.tracks[1].ID
 	got, err := LoadCandidatesFromSimilarity(
 		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true},
-		[]pgtype.UUID{excluded}, defaultLimits(), "test-seed",
+		[]pgtype.UUID{excluded}, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -196,7 +192,7 @@ func TestLoadCandidatesFromSimilarity_SeedAlwaysExcluded(t *testing.T) {
 	f := newFixture(t, 5)
 	seed := f.tracks[0]
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -226,7 +222,7 @@ func TestLoadCandidatesFromSimilarity_RecentlyPlayedExcluded(t *testing.T) {
 		t.Fatalf("play_event: %v", err)
 	}
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -246,7 +242,7 @@ func TestLoadCandidatesFromSimilarity_DedupTakesMaxScore(t *testing.T) {
 	helperSetTrackGenre(t, f, target.ID, "Rock")      // jaccard 1/1 = 1.0 from tag-overlap
 	helperLBSimilarity(t, f, seed.ID, target.ID, 0.5) // weaker LB signal
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -299,7 +295,7 @@ func TestLoadCandidatesFromSimilarity_TasteOverlapArm(t *testing.T) {
 	// Only the taste_overlap arm is enabled.
 	limits := CandidateSourceLimits{TasteOverlap: 10}
 	got, err := LoadCandidatesFromSimilarity(
-		ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, "test-seed",
+		ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits,
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -325,7 +321,7 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) {
 	f := newFixture(t, 1) // just the seed
 	seed := f.tracks[0]
 	got, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed",
+		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(),
 	)
 	if err != nil {
 		t.Fatalf("load: %v", err)
@@ -335,97 +331,3 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) {
 		t.Errorf("got %d candidates from seed-only library, want 0", len(got))
 	}
 }
-
-// The randomised arms must draw REPRODUCIBLY for a given seed (#3889).
-//
-// Four arms used to end in a bare `ORDER BY random()`. That returned a stable
-// set only while the arm's LIMIT exceeded the rows eligible for it — at that
-// point it returned all of them and the order stopped mattering, because the
-// caller sorts by track id before scoring. Below that threshold it returned a
-// random SUBSET, so two calls drew different candidates.
-//
-// It therefore held by ACCIDENT, and only for libraries smaller than the
-// limits. Any real library is larger, so same-day rebuilds had been drawing
-// different mixes since the arm was written — invisible, because a mix that
-// changes after a refresh looks like a feature.
-//
-// Limits deliberately smaller than the fixture, because that is the only
-// regime where the bug existed at all: with limits above the eligible count
-// the old code passes this too.
-func TestLoadCandidatesFromSimilarity_SameSeedDrawsTheSameSet(t *testing.T) {
-	f := newFixture(t, 12)
-	seed := f.tracks[0]
-
-	tight := CandidateSourceLimits{
-		LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2,
-		LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2,
-	}
-	ids := func(cs []Candidate) []string {
-		out := make([]string, 0, len(cs))
-		for _, c := range cs {
-			out = append(out, fmt.Sprintf("%x", c.Track.ID.Bytes))
-		}
-		sort.Strings(out) // membership, not order — order is settled downstream
-		return out
-	}
-
-	first, err := LoadCandidatesFromSimilarity(
-		context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one",
-	)
-	if err != nil {
-		t.Fatalf("load: %v", err)
-	}
-	if len(first) == 0 {
-		t.Fatal("no candidates, so this test asserts nothing")
-	}
-
-	for i := 0; i < 3; i++ {
-		again, err := LoadCandidatesFromSimilarity(
-			context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one",
-		)
-		if err != nil {
-			t.Fatalf("load %d: %v", i, err)
-		}
-		if !reflect.DeepEqual(ids(first), ids(again)) {
-			t.Fatalf("same seed drew a different set on call %d:\n  first %v\n  again %v",
-				i, ids(first), ids(again))
-		}
-	}
-}
-
-// ...and a different seed is free to draw differently, or the ordering would
-// be fixed rather than seeded and every day would serve the same mix.
-//
-// Asserted as "not pinned to one answer" rather than "always differs": with a
-// small fixture two seeds can legitimately collide, so requiring a difference
-// on any single pair would be flaky. Several seeds producing exactly one
-// distinct set is the real regression — that is what a constant ORDER BY
-// looks like.
-func TestLoadCandidatesFromSimilarity_DifferentSeedsCanDrawDifferently(t *testing.T) {
-	f := newFixture(t, 12)
-	seed := f.tracks[0]
-	tight := CandidateSourceLimits{
-		LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2,
-		LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2,
-	}
-
-	seen := map[string]bool{}
-	for _, orderSeed := range []string{"a", "b", "c", "d", "e", "f"} {
-		cs, err := LoadCandidatesFromSimilarity(
-			context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, orderSeed,
-		)
-		if err != nil {
-			t.Fatalf("load %q: %v", orderSeed, err)
-		}
-		ids := make([]string, 0, len(cs))
-		for _, c := range cs {
-			ids = append(ids, fmt.Sprintf("%x", c.Track.ID.Bytes))
-		}
-		sort.Strings(ids)
-		seen[strings.Join(ids, ",")] = true
-	}
-	if len(seen) < 2 {
-		t.Errorf("six different seeds produced %d distinct set(s); the ordering is not "+
-			"varying with the seed at all", len(seen))
-	}
-}

internal/recommendation/songs_like_limits_test.go

@@ -59,3 +59,41 @@ func TestSongsLikeLimits_KeepThePoolRoughlyTheSameSize(t *testing.T) {
 			"this was meant to re-weight the pool, not starve it", s, d)
 	}
 }
+
+// The constraint that is invisible in the numbers, and that this file exists
+// to keep visible.
+//
+// `likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no
+// daily seed (recommendation.sql:118, :161). Such an arm returns a stable set
+// only while its LIMIT exceeds the eligible rows; below that it returns a
+// random SUBSET that differs between two builds on the same day, and the
+// daily-determinism promise quietly stops holding.
+//
+// This is not hypothetical — it is how this change first failed CI. Cutting
+// RandomFill to 10 broke TestBuildSystemPlaylists_DailyNonceDeterminism,
+// whose library is smaller than the default limit and whose determinism was
+// therefore an accident of the limit exceeding the library.
+//
+// Growing these arms is always safe. Only shrinking is, and the fix that
+// would make shrinking safe is a seeded ordering (#3889), not a smaller
+// number here.
+func TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms(t *testing.T) {
+	d := DefaultCandidateSourceLimits()
+	s := SongsLikeCandidateSourceLimits()
+
+	for _, tc := range []struct {
+		arm             string
+		songsLike, dflt int
+	}{
+		{"RandomFill", s.RandomFill, d.RandomFill},
+		{"LikesOverlap", s.LikesOverlap, d.LikesOverlap},
+	} {
+		if tc.songsLike < tc.dflt {
+			t.Errorf("%s cut from %d to %d. That arm is ordered by unseeded random(), "+
+				"so a smaller limit makes pool membership vary between same-day "+
+				"rebuilds — it breaks daily determinism rather than merely narrowing "+
+				"the mix. Fix the ordering (#3889) before trimming this.",
+				tc.arm, tc.dflt, tc.songsLike)
+		}
+	}
+}

internal/server/server.go

@@ -160,7 +160,7 @@ func (s *Server) Router() http.Handler {
 		if raErr != nil {
 			s.Logger.Warn("reacquisition settings unavailable; serving defaults", "err", raErr)
 		}
-		lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn, s.DataDir)
+		lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn)
 		tracksSvc := tracks.NewService(s.Pool, s.Logger, lidarrUnmonitorAdapter{fn: lidarrClientFn}, s.DataDir)
 		playlistsSvc := playlists.NewService(s.Pool, s.Logger, s.DataDir)
 		smtpSender := mailer.NewSMTPSender(s.Pool, s.Logger.With("component", "mailer"))

internal/tracks/service.go

@@ -1,9 +1,9 @@
-// Package tracks owns the track-level admin actions behind DELETE
+// Package tracks owns the track-level admin actions exposed by the
-// /api/admin/tracks/{id}. Today that's RemoveTrack: the destructive part goes
+// M7 #372 track-actions menu. Today that's RemoveTrack: the destructive
-// through library.DeleteTrackFile — the one path that deletes a track file —
+// part is always handled directly by Minstrel (os.Remove + DB delete +
-// and when the operator opts in via `unmonitor=true` the service also tells
+// cascade); when the operator opts in via `unmonitor=true` the service
-// Lidarr to flip the track's monitored flag off so Lidarr doesn't search for a
+// also tells Lidarr to flip the track's monitored flag off so Lidarr
-// replacement.
+// doesn't search for a replacement.
 //
 // History: an earlier shape (commit 50a231f, since rewritten) routed
 // Lidarr-managed tracks through lidarrquarantine.DeleteViaLidarr — but
@@ -12,9 +12,6 @@
 // drop sibling tracks the operator didn't ask to remove. The current
 // shape per spec revision 723eee9 is "always direct delete; opt-in
 // Lidarr unmonitor for replacement-suppression."
-//
-// The web track-kebab entry that called this was removed in f7278f24; the
-// endpoint was kept deliberately for a safer admin surface to rebind.
 package tracks
 
 import (
@@ -22,14 +19,15 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"os"
 
 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgtype"
 	"github.com/jackc/pgx/v5/pgxpool"
 
 	"git.fabledsword.com/bvandeusen/minstrel/internal/apierror"
+	"git.fabledsword.com/bvandeusen/minstrel/internal/coverart"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
-	"git.fabledsword.com/bvandeusen/minstrel/internal/library"
 )
 
 // ErrNotFound is returned when the track id doesn't resolve. Aliased
@@ -71,10 +69,10 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitore
 	return &Service{pool: pool, logger: logger, lidarr: lidarr, dataDir: dataDir}
 }
 
-// RemoveTrack deletes the track's file and then its row, tidies away an album
+// RemoveTrack deletes the file from disk and the DB rows, runs the
-// or artist the delete empties, and (when unmonitor is true and the track is
+// album-empty / artist-empty cascade tidy-up, and (when unmonitor is
-// Lidarr-managed) tells Lidarr to flip the track's monitored flag off so it
+// true and the track is Lidarr-managed) tells Lidarr to flip the track's
-// won't search for a replacement.
+// monitored flag off so it won't search for a replacement.
 //
 // Returns:
 //   - deletedAlbumID: non-nil when removing the track left the album
@@ -83,10 +81,9 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitore
 //     empty (only set if deletedAlbumID is also set).
 //   - lidarrUnmonitorFailed: true when the operator requested unmonitor
 //     and the Lidarr call failed; the file + DB delete still succeeded.
-//   - err: ErrNotFound, or a failure before anything was deleted. When the
+//   - err: only for failures *before* the destructive part completes.
-//     file cannot be removed it is a *library.FileRemoveError and NOTHING was
+//     A failed os.Remove is logged and tolerated. A failed Lidarr
-//     deleted — see library.DeleteTrackFile for why that order is the contract
+//     unmonitor is reflected in the bool flag, not the error.
-//     (#3918). A failed Lidarr unmonitor is reflected in the bool, not here.
 //
 // adminID is currently unused — the cascade audit-log line that would
 // reference it isn't wired in this slice. It's threaded through the
@@ -107,9 +104,10 @@ func (s *Service) RemoveTrack(
 		return nil, nil, false, fmt.Errorf("get track: %w", err)
 	}
 
-	// Capture the album's mbid *before* the delete. If removing this track
+	// Capture the album's mbid *before* the cascade-delete transaction.
-	// empties the album, its row is gone afterwards and the unmonitor walk
+	// If removing this track empties the album, DeleteAlbumIfEmpty
-	// would have no album mbid to name.
+	// removes the row and a post-commit GetAlbumByID would return
+	// pgx.ErrNoRows — leaving the unmonitor walk with no album mbid.
 	var albumMbid string
 	if track.Mbid != nil && *track.Mbid != "" && unmonitor && s.lidarr != nil {
 		alb, alerr := q.GetAlbumByID(ctx, track.AlbumID)
@@ -120,14 +118,64 @@ func (s *Service) RemoveTrack(
 		// "no albumMbid → can't unmonitor → flag failure."
 	}
 
-	deleted, err := library.DeleteTrackFile(ctx, s.pool, s.logger, s.dataDir, trackID)
+	// Always: remove the file. Tolerate already-missing.
-	if err != nil {
+	if track.FilePath != "" {
-		if errors.Is(err, library.ErrTrackNotFound) {
+		if rerr := os.Remove(track.FilePath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) {
-			return nil, nil, false, ErrNotFound
+			s.logger.Warn("track delete: file remove failed",
+				"path", track.FilePath, "track_id", trackID, "err", rerr)
+			// Proceed: DB consistency is the priority.
 		}
+	}
+
+	// DB cleanup in a transaction so a midway failure leaves things consistent.
+	tx, err := s.pool.Begin(ctx)
+	if err != nil {
+		return nil, nil, false, fmt.Errorf("begin tx: %w", err)
+	}
+	defer func() { _ = tx.Rollback(ctx) }()
+
+	tq := dbq.New(tx)
+
+	deleted, err := tq.DeleteTrack(ctx, trackID)
+	if err != nil {
 		return nil, nil, false, fmt.Errorf("delete track: %w", err)
 	}
 
+	albumRow, aerr := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID)
+	switch {
+	case aerr == nil:
+		albumID := albumRow.ID
+		deletedAlbumID = &albumID
+		artistID, arerr := tq.DeleteArtistIfEmpty(ctx, albumRow.ArtistID)
+		switch {
+		case arerr == nil:
+			id := artistID
+			deletedArtistID = &id
+		case errors.Is(arerr, pgx.ErrNoRows):
+			// Artist still has other albums or stray tracks. OK.
+		default:
+			return nil, nil, false, fmt.Errorf("delete artist if empty: %w", arerr)
+		}
+	case errors.Is(aerr, pgx.ErrNoRows):
+		// Album still has other tracks. OK.
+	default:
+		return nil, nil, false, fmt.Errorf("delete album if empty: %w", aerr)
+	}
+
+	if err := tx.Commit(ctx); err != nil {
+		return nil, nil, false, fmt.Errorf("commit: %w", err)
+	}
+
+	// Cleanup artist-art filesystem cache if the delete cascade orphaned
+	// an artist. Non-fatal — the destructive part is done; we just want
+	// to keep the dataDir tidy.
+	if deletedArtistID != nil && s.dataDir != "" {
+		if err := coverart.CleanupArtistArt(s.dataDir, *deletedArtistID); err != nil {
+			s.logger.Warn("track delete: artist-art cleanup failed",
+				"artist_id", *deletedArtistID, "err", err)
+		}
+	}
+
 	// Lidarr unmonitor — non-fatal. The destructive part is done; any
 	// failure here is informational so the operator can retry manually.
 	if unmonitor && track.Mbid != nil && *track.Mbid != "" && s.lidarr != nil {
@@ -145,5 +193,5 @@ func (s *Service) RemoveTrack(
 		}
 	}
 
-	return deleted.AlbumID, deleted.ArtistID, lidarrUnmonitorFailed, nil
+	return deletedAlbumID, deletedArtistID, lidarrUnmonitorFailed, nil
 }

web/src/lib/api/admin.fingerprints.test.ts

@@ -1,32 +0,0 @@
-import { describe, it, expect, vi, beforeEach } from 'vitest';
-import { getFingerprintCoverage, type FingerprintCoverage } from './admin';
-
-vi.mock('./client', () => ({
-  api: { get: vi.fn(), post: vi.fn() }
-}));
-
-import { api } from './client';
-
-describe('admin fingerprint coverage API', () => {
-  beforeEach(() => vi.clearAllMocks());
-
-  it('getFingerprintCoverage GETs the correct path', async () => {
-    const sample: FingerprintCoverage = {
-      total: 18026,
-      fingerprinted: 9400,
-      rejected: 12,
-      pending: 8614
-    };
-    (api.get as unknown as ReturnType<typeof vi.fn>).mockResolvedValueOnce(sample);
-    const got = await getFingerprintCoverage();
-    expect(api.get).toHaveBeenCalledWith('/api/admin/library/fingerprints');
-    expect(got).toEqual(sample);
-  });
-
-  it('buckets sum to the total', async () => {
-    const sample: FingerprintCoverage = { total: 10, fingerprinted: 6, rejected: 1, pending: 3 };
-    (api.get as unknown as ReturnType<typeof vi.fn>).mockResolvedValueOnce(sample);
-    const got = await getFingerprintCoverage();
-    expect(got.fingerprinted + got.rejected + got.pending).toBe(got.total);
-  });
-});

web/src/lib/api/admin.ts

@@ -313,30 +313,6 @@ export function createCoverageQuery() {
   });
 }
 
-// Fingerprint backfill (#3908) --------------------------------------------
-
-export type FingerprintCoverage = {
-  total: number;
-  fingerprinted: number;
-  rejected: number;
-  pending: number;
-};
-
-export async function getFingerprintCoverage(): Promise<FingerprintCoverage> {
-  return api.get<FingerprintCoverage>('/api/admin/library/fingerprints');
-}
-
-// Polled far less often than the cover gauge: the backfill decodes files two at
-// a time, so the count moves by a few tracks a minute and a 3s poll is noise.
-export function createFingerprintCoverageQuery() {
-  return createQuery({
-    queryKey: qk.fingerprintCoverage(),
-    queryFn: getFingerprintCoverage,
-    staleTime: 30_000,
-    refetchInterval: 30_000
-  });
-}
-
 // Cover-art providers ------------------------------------------------------
 
 export type CoverProviderCapability = 'album_cover' | 'artist_thumb' | 'artist_fanart';

web/src/lib/api/errors.test.ts

@@ -49,37 +49,3 @@ describe('errMessage', () => {
     expect(result.length).toBeGreaterThan(0);
   });
 });
-
-describe('errMessage detail codes (#3918)', () => {
-  const detail =
-    'Minstrel runs as uid 1000, gid 1000 and cannot delete from /music/A (read-only file system). ' +
-    'The library mount must be writable by that user. Nothing was deleted.';
-
-  test('library_not_writable appends the server message to the copy', () => {
-    expect(errMessage({ code: 'library_not_writable', message: detail })).toBe(
-      `${ERROR_COPY.library_not_writable} ${detail}`
-    );
-  });
-
-  test('file_delete_failed appends the server message to the copy', () => {
-    const msg = 'Could not delete /music/A/01.mp3 (input/output error). Nothing was deleted.';
-    expect(errMessage({ code: 'file_delete_failed', message: msg })).toBe(
-      `${ERROR_COPY.file_delete_failed} ${msg}`
-    );
-  });
-
-  test('a detail code with no message shows the copy alone', () => {
-    expect(errMessage({ code: 'library_not_writable' })).toBe(ERROR_COPY.library_not_writable);
-    expect(errMessage({ code: 'library_not_writable', message: '  ' })).toBe(
-      ERROR_COPY.library_not_writable
-    );
-  });
-
-  // Server messages are usually internal detail. Appending them for every code
-  // would leak things like driver errors into toasts; this pins the scope.
-  test('other codes never carry the server message', () => {
-    expect(errMessage({ code: 'track_not_found', message: 'pgx: no rows in result set' })).toBe(
-      ERROR_COPY.track_not_found
-    );
-  });
-});

web/src/lib/api/errors.ts

@@ -8,25 +8,11 @@ export function errCode(err: unknown): string {
   return (err as { code?: string })?.code ?? 'unknown';
 }
 
-/**
- * Codes whose server message carries specifics the operator needs in order to
- * act — which directory, which uid — that fixed copy cannot say. For these the
- * message follows the copy (#3918). Kept to a named list on purpose: most
- * server messages are internal detail and must never reach a toast. Mirrored
- * in Android's ErrorCopy.
- */
-const DETAIL_CODES: ReadonlySet<string> = new Set(['library_not_writable', 'file_delete_failed']);
-
 /**
  * Returns user-facing copy for an unknown error value. Looks up the
  * error's code in the error-copy map; falls back to the supplied
  * fallback (default: "Something went wrong.") when the code is unknown.
- * For a DETAIL_CODES code, the server's message is appended.
  */
 export function errMessage(err: unknown, fallback = 'Something went wrong.'): string {
-  const code = errCode(err);
+  return copyForCode(errCode(err)) ?? fallback;
-  const copy = copyForCode(code) ?? fallback;
-  if (!DETAIL_CODES.has(code)) return copy;
-  const detail = (err as { message?: unknown })?.message;
-  return typeof detail === 'string' && detail.trim() !== '' ? `${copy} ${detail}` : copy;
 }

web/src/lib/api/queries.ts

@@ -47,7 +47,6 @@ export const qk = {
     ['adminPlaybackErrors', { resolved: resolved ?? false }] as const,
   scanStatus:             ()              => ['scanStatus'] as const,
   coverage:               ()              => ['coverage'] as const,
-  fingerprintCoverage:    ()              => ['fingerprintCoverage'] as const,
   coverProviders:         ()              => ['coverProviders'] as const,
   tagProviders:           ()              => ['tagProviders'] as const,
   adminUsers:             ()              => ['adminUsers'] as const,

web/src/lib/styles/error-copy.json

@@ -40,8 +40,6 @@
   "request_not_pending": "This request is no longer pending.",
   "request_not_found": "That request no longer exists.",
   "track_not_found": "That track no longer exists.",
-  "library_not_writable": "The music library isn't writable by the server.",
-  "file_delete_failed": "The file couldn't be deleted.",
   "album_not_found": "That album no longer exists.",
   "artist_not_found": "That artist no longer exists.",
   "playlist_not_found": "That playlist no longer exists.",

web/src/routes/admin/+page.svelte

@@ -10,7 +10,6 @@
     createAdminQuarantineQuery,
     createScanStatusQuery,
     createCoverageQuery,
-    createFingerprintCoverageQuery,
     approveRequest,
     rejectRequest,
     resolveQuarantine,
@@ -178,11 +177,6 @@
   const coverageQ = $derived($coverageStore);
   const coverage = $derived(coverageQ.data);
 
-  // ---- Fingerprint backfill gauge (#3908) ----
-  const fingerprintStore = $derived(createFingerprintCoverageQuery());
-  const fingerprintQ = $derived($fingerprintStore);
-  const fingerprints = $derived(fingerprintQ.data);
-
   let triggering = $state(false);
   let triggerResult = $state<string | null>(null);
 
@@ -434,28 +428,6 @@
     {#if triggerResult}
       <p class="mt-2 text-sm">{triggerResult}</p>
     {/if}
-
-    <!-- Fingerprint backfill (#3908). A worker of its own rather than a scan
-         stage, so its progress is read live here, not from the run above. -->
-    {#if fingerprints && fingerprints.total > 0}
-      <div class="mt-3 flex flex-wrap items-center gap-3 text-sm">
-        <span class="text-xs font-medium uppercase tracking-wide text-text-muted">Fingerprints</span>
-        <span>{fingerprints.fingerprinted.toLocaleString()} of {fingerprints.total.toLocaleString()} tracks</span>
-        {#if fingerprints.pending > 0}
-          <span class="text-text-muted">·</span>
-          <span>{fingerprints.pending.toLocaleString()} pending</span>
-        {/if}
-        {#if fingerprints.rejected > 0}
-          <span class="text-text-muted">·</span>
-          <span
-            class="cursor-help"
-            title="The fingerprint tools could not read these files. Each is tried again when its file changes."
-          >
-            {fingerprints.rejected.toLocaleString()} unreadable
-          </span>
-        {/if}
-      </div>
-    {/if}
   </section>
 
   <!-- Cover art bulk refetch -->

web/src/routes/admin/admin.test.ts

@@ -35,12 +35,6 @@ vi.mock('$lib/api/admin', async () => {
         isPending: false,
         isError: false
       }),
-    createFingerprintCoverageQuery: () =>
-      readable({
-        data: undefined,
-        isPending: false,
-        isError: false
-      }),
     approveRequest: vi.fn().mockResolvedValue({}),
     rejectRequest: vi.fn().mockResolvedValue({}),
     resolveQuarantine: vi.fn().mockResolvedValue({}),