diff --git a/Dockerfile b/Dockerfile index 7aef6079..55b3690d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,8 +33,14 @@ 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 \ + && apt-get install -y --no-install-recommends ca-certificates ffmpeg libchromaprint-tools \ && rm -rf /var/lib/apt/lists/* RUN groupadd --system --gid 1000 minstrel \ diff --git a/README.md b/README.md index 4571d523..3f0ac051 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,12 @@ services: ports: ['4533:4533'] volumes: # Your music library. Point ./music at wherever your audio files - # live. Mounted read-only — Minstrel never writes to your library. - - ./music:/music:ro + # live. Writable, because Minstrel deletes a file when an admin asks + # it to (for example, quarantine's "Delete file"). It never moves, + # 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 @@ -47,7 +51,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 read-only music mount above (/music here). + # path of the music mount above (/music here). MINSTREL_LIBRARY_SCAN_PATHS: /music depends_on: [db] diff --git a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt index 4031a5fc..586c4ce5 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt @@ -15,10 +15,14 @@ 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 @@ -42,6 +46,12 @@ 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(), @@ -53,6 +63,7 @@ fun AdminQuarantineScreen( onBack = { navController.popBackStack() }, ) }, + snackbarHost = { SnackbarHost(snackbarHostState) }, ) { inner -> PullToRefreshScaffold( onRefresh = { viewModel.refresh().join() }, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt index 3e05b469..85e83b8e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt @@ -10,10 +10,13 @@ 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 @@ -34,6 +37,15 @@ class AdminQuarantineViewModel @Inject constructor( private val internal = MutableStateFlow(AdminQuarantineUiState.Loading) val uiState: StateFlow = 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(Channel.BUFFERED) + val transientMessages: Flow = transientMessagesChannel.receiveAsFlow() + init { refresh() viewModelScope.launch { @@ -86,8 +98,9 @@ class AdminQuarantineViewModel @Inject constructor( try { action(trackId) } catch ( - @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + @Suppress("TooGenericExceptionCaught") e: Throwable, ) { + transientMessagesChannel.trySend(ErrorCopy.fromThrowable(e)) refresh() } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt index 8456fad9..3f345851 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt @@ -37,18 +37,35 @@ object ErrorCopy { * as connection failures. */ fun fromThrowable(t: Throwable): String = when (t) { - is HttpException -> messageFor(codeFromHttp(t)) + is HttpException -> fromHttp(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 "unknown" - val code = runCatching { json.decodeFromString(raw).error?.code } - .getOrNull() - .orEmpty() - return code.ifEmpty { "unknown" } + ?: return Body() + return runCatching { json.decodeFromString(raw).error } + .getOrNull() ?: Body() } private val TABLE: Map = mapOf( @@ -99,6 +116,8 @@ 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.", diff --git a/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt new file mode 100644 index 00000000..c7b7d67a --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt @@ -0,0 +1,60 @@ +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(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")), + ) + } +} diff --git a/internal/api/admin_quarantine.go b/internal/api/admin_quarantine.go index 116f9048..bd372d3f 100644 --- a/internal/api/admin_quarantine.go +++ b/internal/api/admin_quarantine.go @@ -133,6 +133,13 @@ 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") diff --git a/internal/api/admin_quarantine_test.go b/internal/api/admin_quarantine_test.go index 59cd7b49..5318d5b3 100644 --- a/internal/api/admin_quarantine_test.go +++ b/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.lidarrQuarantine = lidarrquarantine.NewService(h.pool, cfg, clientFn, h.dataDir) } // flagDirect bypasses the HTTP handler to seed a quarantine row via the diff --git a/internal/api/admin_tracks.go b/internal/api/admin_tracks.go index 251d287b..7630fa27 100644 --- a/internal/api/admin_tracks.go +++ b/internal/api/admin_tracks.go @@ -23,15 +23,17 @@ type removeTrackResponse struct { // handleRemoveTrack implements DELETE /api/admin/tracks/{id}?unmonitor=true|false. // -// Admin-only (gated by auth.RequireAdmin on the /admin route group). Always -// deletes the file + DB row and runs the album/artist cascade tidy-up. When +// Admin-only (gated by auth.RequireAdmin on the /admin route group). Deletes the +// file, then the DB row, and runs the album/artist cascade tidy-up — and deletes +// 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 only error codes this handler emits are not_found, -// server_error, plus the auth codes the middleware emits upstream. +// wire error codes. The codes this handler emits are not_found, +// library_not_writable (409) and file_delete_failed when the file could not be +// 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) @@ -66,6 +68,11 @@ 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 diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index ae564b9c..171c0f8a 100644 --- a/internal/api/auth_test.go +++ b/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. diff --git a/internal/api/file_remove_error.go b/internal/api/file_remove_error.go new file mode 100644 index 00000000..3b2441c2 --- /dev/null +++ b/internal/api/file_remove_error.go @@ -0,0 +1,58 @@ +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...) +} diff --git a/internal/api/file_remove_error_test.go b/internal/api/file_remove_error_test.go new file mode 100644 index 00000000..1da062da --- /dev/null +++ b/internal/api/file_remove_error_test.go @@ -0,0 +1,96 @@ +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) + } + } +} diff --git a/internal/api/radio.go b/internal/api/radio.go index 218dadd8..d8558bb8 100644 --- a/internal/api/radio.go +++ b/internal/api/radio.go @@ -101,6 +101,10 @@ 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) diff --git a/internal/db/dbq/fingerprints.sql.go b/internal/db/dbq/fingerprints.sql.go new file mode 100644 index 00000000..f1b5e244 --- /dev/null +++ b/internal/db/dbq/fingerprints.sql.go @@ -0,0 +1,59 @@ +// 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 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 +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index d0d4a5f1..07ca6f75 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -667,6 +667,14 @@ 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 diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index f290ec35..bd798644 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/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, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) 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 random() + ORDER BY md5(gl.track_id::text || $12::text) 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, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) 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 random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT @@ -938,17 +938,18 @@ GROUP BY t.id, t.title, t.album_id, t.artist_id, t.duration_ms, t.file_path, ` type LoadRadioCandidatesV2Params struct { - UserID pgtype.UUID - ID pgtype.UUID - Column3 interface{} - Column4 []pgtype.UUID - Limit int32 - Limit_2 int32 - Limit_3 int32 - Limit_4 int32 - Limit_5 int32 - Limit_6 int32 - Limit_7 int32 + UserID pgtype.UUID + ID pgtype.UUID + Column3 interface{} + Column4 []pgtype.UUID + Limit int32 + Limit_2 int32 + Limit_3 int32 + Limit_4 int32 + Limit_5 int32 + Limit_6 int32 + Limit_7 int32 + Column12 string } type LoadRadioCandidatesV2Row struct { @@ -971,8 +972,22 @@ 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, @@ -987,6 +1002,7 @@ 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 diff --git a/internal/db/migrations/0058_track_fingerprints.down.sql b/internal/db/migrations/0058_track_fingerprints.down.sql new file mode 100644 index 00000000..c04a359c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.down.sql @@ -0,0 +1 @@ +DROP TABLE track_fingerprints; diff --git a/internal/db/migrations/0058_track_fingerprints.up.sql b/internal/db/migrations/0058_track_fingerprints.up.sql new file mode 100644 index 00000000..aa0f8c9c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.up.sql @@ -0,0 +1,38 @@ +-- 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; diff --git a/internal/db/queries/fingerprints.sql b/internal/db/queries/fingerprints.sql new file mode 100644 index 00000000..ef0276b5 --- /dev/null +++ b/internal/db/queries/fingerprints.sql @@ -0,0 +1,22 @@ +-- 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; diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index 20fbf7f7..7d9bfa87 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -45,7 +45,22 @@ 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 @@ -87,7 +102,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, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $6 ), tag_overlap AS ( @@ -115,7 +130,7 @@ likes_overlap AS ( WHERE t.id = gl.track_id AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags) ) - ORDER BY random() + ORDER BY md5(gl.track_id::text || $12::text) LIMIT $8 ), taste_overlap AS ( @@ -142,7 +157,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, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $11 ), random_fill AS ( @@ -158,7 +173,7 @@ random_fill AS ( UNION SELECT track_id FROM taste_overlap UNION SELECT track_id FROM coplay_artists ) - ORDER BY random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index c71b9aae..a99db739 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -87,6 +87,7 @@ var dataTables = []string{ // pristine Discover knobs rather than whatever a previous test tuned. "discover_tuning", "recommendation_tuning_audit", + "track_fingerprints", // M400 "tracks", "albums", "artists", diff --git a/internal/library/delete.go b/internal/library/delete.go index 81f5bd58..70d3ece5 100644 --- a/internal/library/delete.go +++ b/internal/library/delete.go @@ -5,12 +5,16 @@ 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" ) @@ -19,54 +23,159 @@ import ( // that has no row in tracks. var ErrTrackNotFound = errors.New("library: track not found") -// DeleteTrackFile removes a track file from disk and its row from the -// tracks table. Album and artist rows are left untouched. +// removeFile is os.Remove behind a variable so a test can make removal fail the +// way a read-only mount or a wrongly-owned directory does. A chmod-based test +// 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). // -// Steps: -// 1. Look up the track to get its file_path. -// 2. Remove the file from disk. fs.ErrNotExist is OK — already gone. -// 3. Delete the tracks row. +// Order is the whole contract. The file goes first, and if it cannot go — a +// read-only mount, a permission denial, an I/O error — nothing else happens and +// a *FileRemoveError comes back. Proceeding past that failure is how #3918 lost +// history: tracks CASCADEs to play_events, general_likes, contextual_likes, +// 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. // -// Order matters: file first, then DB. If the file delete fails (permission, -// I/O error), we leave the DB row alone so the admin can retry. +// A file that is already gone (fs.ErrNotExist) is not a failure; the row is +// removed as asked. // -// The reverse failure mode — file gone, DB row still present — IS reconciled -// now, and not by this function: the scan's reconcile pass stamps -// tracks.missing_since (#2523), every selection path filters on it, and a file -// that returns is un-marked or adopted at its new path (#2528). That is the -// 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). +// This is NOT the missing-file path. That lifecycle is deliberately +// non-destructive: reconcile stamps missing_since (#2523), selection paths +// filter on it, and a returning file is un-marked or adopted (#2528). This is the +// explicit, irreversible "remove this recording", never the way to tidy up a row +// whose file merely went away. // -// So this function is NOT the missing-file path. It is the explicit admin -// action "remove this recording from disk and from the library", and it is -// irreversible: tracks CASCADEs to play_events, general_likes_tracks, -// contextual_likes, track_tags and playback_errors. Reach for it when the -// operator means to destroy the record, never to tidy up a row whose file -// merely went away. -func DeleteTrackFile(ctx context.Context, pool *pgxpool.Pool, trackID pgtype.UUID) error { +// dataDir, when set, also clears the cached art of an artist the delete removed. +// logger may be nil. +func DeleteTrackFile( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, trackID pgtype.UUID, +) (DeletedTrack, error) { + if logger == nil { + logger = slog.Default() + } q := dbq.New(pool) track, err := q.GetTrackByID(ctx, trackID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return ErrTrackNotFound + return DeletedTrack{}, ErrTrackNotFound } - return fmt.Errorf("get track: %w", err) + return DeletedTrack{}, fmt.Errorf("get track: %w", err) } - if err := os.Remove(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("remove file: %w", err) + if err := removeFile(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return DeletedTrack{}, &FileRemoveError{ + Path: track.FilePath, UID: os.Getuid(), GID: os.Getgid(), Err: err, + } } - if _, err := pool.Exec(ctx, "DELETE FROM tracks WHERE id = $1", trackID); err != nil { - return fmt.Errorf("delete row: %w", err) + // The row and any album or artist it empties go together, so a failure + // partway cannot leave a deleted track with a ghost album behind it. + tx, err := pool.Begin(ctx) + if err != nil { + return DeletedTrack{}, fmt.Errorf("begin tx: %w", err) } - // Log the change after the delete succeeds. Best-effort: a Warn-level - // failure here would leave the cache index orphaned on offline clients - // until the next scan touches the surrounding album. + defer func() { _ = tx.Rollback(ctx) }() + tq := dbq.New(tx) + + 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 { - return fmt.Errorf("log change: %w", err) + logger.Warn("track delete: LogChange failed", "track_id", syncpkg.FormatUUID(trackID), "err", err) } - return nil + if out.ArtistID != nil && dataDir != "" { + 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 } diff --git a/internal/library/delete_test.go b/internal/library/delete_test.go index 9952bffd..76cf9ae2 100644 --- a/internal/library/delete_test.go +++ b/internal/library/delete_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "io" + "io/fs" "log/slog" "os" "path/filepath" + "syscall" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -64,6 +66,15 @@ 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) @@ -73,9 +84,18 @@ func TestDeleteTrackFile_HappyPath(t *testing.T) { if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil { t.Fatalf("write file: %v", err) } - track, album, _ := seedTrack(t, pool, path) + track, album, artist := 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) + } - if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil { + got, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID) + if err != nil { t.Fatalf("DeleteTrackFile: %v", err) } @@ -85,9 +105,99 @@ 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 row vanished: %v", err) + t.Errorf("album with a remaining track 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) + } + }) } } @@ -97,7 +207,7 @@ func TestDeleteTrackFile_FileAlreadyGoneSucceeds(t *testing.T) { track, _, _ := seedTrack(t, pool, "/no/such/file/anywhere.mp3") - if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil { + if _, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID); err != nil { t.Fatalf("DeleteTrackFile with missing file: %v", err) } if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { @@ -112,7 +222,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, bogus) + _, err := DeleteTrackFile(context.Background(), pool, nil, "", bogus) if !errors.Is(err, ErrTrackNotFound) { t.Errorf("err = %v, want ErrTrackNotFound", err) } diff --git a/internal/library/fingerprint.go b/internal/library/fingerprint.go new file mode 100644 index 00000000..59ac7cb7 --- /dev/null +++ b/internal/library/fingerprint.go @@ -0,0 +1,296 @@ +package library + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "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) +} + +// storeFingerprint records one attempt for a track whose bytes are new or have +// changed. It never fails the scan: a missing fingerprint only keeps a track +// out of duplicate detection, which is not worth dropping the track over. +func (s *Scanner) storeFingerprint( + ctx context.Context, q *dbq.Queries, trackID pgtype.UUID, path string, fp fingerprintResult, +) { + if fp.hashErr != nil { + s.logger.Warn("library scan: audio stream hash failed", "path", path, "err", fp.hashErr) + } + if fp.printErr != nil { + s.logger.Warn("library scan: chromaprint failed", "path", path, "err", fp.printErr) + } + if fp.inconclusive() { + // Any row this track holds describes its PREVIOUS bytes. Drop it and + // leave the track to the backfill, rather than stamping a failure that + // says nothing about this file. + if err := q.DeleteTrackFingerprint(ctx, trackID); err != nil { + s.logger.Warn("library scan: clearing stale fingerprint failed", "path", path, "err", err) + } + return + } + // 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 boot. 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 { + s.logger.Warn("library scan: storing fingerprint failed", "path", path, "err", err) + } +} + +// 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=" 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= +// FINGERPRINT=,,... +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") +} diff --git a/internal/library/fingerprint_scan_test.go b/internal/library/fingerprint_scan_test.go new file mode 100644 index 00000000..b2edb800 --- /dev/null +++ b/internal/library/fingerprint_scan_test.go @@ -0,0 +1,174 @@ +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) + } +} diff --git a/internal/library/fingerprint_test.go b/internal/library/fingerprint_test.go new file mode 100644 index 00000000..e4b5cee1 --- /dev/null +++ b/internal/library/fingerprint_test.go @@ -0,0 +1,194 @@ +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) + } + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 634c45a4..c23dbd94 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -75,10 +75,15 @@ 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} + return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint} } // Scan walks every configured root and upserts any audio file whose mtime is @@ -295,6 +300,25 @@ 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 @@ -359,6 +383,9 @@ 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 { + s.storeFingerprint(ctx, q, track.ID, path, fp) + } if knownTrack { stats.Updated++ diff --git a/internal/lidarrquarantine/service.go b/internal/lidarrquarantine/service.go index 3a402342..54a97abe 100644 --- a/internal/lidarrquarantine/service.go +++ b/internal/lidarrquarantine/service.go @@ -35,6 +35,9 @@ 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 @@ -42,11 +45,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) *Service { +func NewService(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client, dataDir string) *Service { if clientFn == nil { clientFn = func() *lidarr.Client { return nil } } - return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn} + return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn, dataDir: dataDir} } // Flag inserts or updates a quarantine row for the caller. Re-flagging @@ -245,10 +248,13 @@ 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, then -// (via FK ON DELETE CASCADE) clears all per-user quarantine rows for that -// track and writes an audit row. If the file deletion fails, the per-user -// rows stay so admin can retry. No partial state. +// DeleteFile removes the track file from disk and the tracks row (tidying away +// an album or artist that leaves empty), then — via FK ON DELETE CASCADE — +// clears every per-user quarantine row for that track and writes an audit row. +// +// 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) @@ -267,7 +273,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, trackID); err != nil { + if _, err := library.DeleteTrackFile(ctx, s.pool, nil, s.dataDir, trackID); err != nil { return dbq.LidarrQuarantineAction{}, fmt.Errorf("delete file: %w", err) } // tracks row is gone; ON DELETE CASCADE on lidarr_quarantine.track_id diff --git a/internal/lidarrquarantine/service_test.go b/internal/lidarrquarantine/service_test.go index cca8e26c..8129b01a 100644 --- a/internal/lidarrquarantine/service_test.go +++ b/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} diff --git a/internal/playlists/system.go b/internal/playlists/system.go index f0708793..420c0811 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -276,6 +276,17 @@ 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() @@ -629,6 +640,7 @@ func produceForYou( zeroVec, seeds, systemForYouSourceLimits(), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: for-you candidates load failed for seed; continuing", @@ -716,6 +728,7 @@ func produceSeedMixes( recommendation.ScaleForLibrary( recommendation.SongsLikeCandidateSourceLimits(), librarySize, ), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: seed candidates load failed; skipping", diff --git a/internal/playlists/you_might_like.go b/internal/playlists/you_might_like.go index 27ccaf58..2e4d4bc1 100644 --- a/internal/playlists/you_might_like.go +++ b/internal/playlists/you_might_like.go @@ -102,6 +102,7 @@ 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", diff --git a/internal/recommendation/candidates.go b/internal/recommendation/candidates.go index c2d2a4ac..ecd1d7e2 100644 --- a/internal/recommendation/candidates.go +++ b/internal/recommendation/candidates.go @@ -139,46 +139,41 @@ 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". // -// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint -// that shapes the numbers below, and it is not obvious from reading them. +// The seed-independent arms are trimmed hardest, because on this surface they +// are noise: `taste_overlap` (tracks by the user's top taste artists) and +// `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. // -// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()` -// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a -// STABLE set only while its LIMIT exceeds the rows eligible for it — at that -// point it returns all of them and the random order is irrelevant, because -// the caller sorts by id before scoring. Drop the limit below the eligible -// count and the arm starts returning a random SUBSET, which differs between -// two builds on the same day. +// THESE TRIMS WERE BLOCKED UNTIL #3889. `likes_overlap` and `random_fill` +// used to end in a bare `ORDER BY random()`, which made their output a stable +// SET only while the limit exceeded the eligible rows — so SHRINKING them +// changed pool membership between same-day rebuilds and broke daily +// determinism. Those arms now order by md5(id || seed), so a smaller limit +// takes a smaller but REPRODUCIBLE slice, and the trim is safe. // -// That is a real defect (#3889) rather than a quirk of this function, and it -// bit here: cutting RandomFill to 10 broke -// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller -// than the default limit and whose determinism was therefore accidental. -// Growing an arm is always safe; only shrinking one is. +// Reduced, never removed. Rule 131: the two seed-independent arms are the +// tier-3 floor, and zeroing them would leave a seed with thin ListenBrainz +// coverage producing a short mix or none at all. The weights (SimilarityWeight +// 4.0, everything seed-independent demoted) keep them ranked last, so they +// surface only when the closer tiers cannot fill the mix. // -// So the seed-independent arms are trimmed only where the ordering is -// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be -// cut, `random_fill` cannot. The reduction is consequently modest — and it -// matters less than it looks, because the WEIGHTS are what demote sim_score-0 -// candidates now. The pool change biases the draw; the songs_like profile is -// what actually keeps unrelated tracks out of the result. -// -// 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). +// likes_overlap is cut hardest of the tier-2 arms for a specific reason: its +// SQL assigns a FLAT 0.6 sim_score (recommendation.sql) rather than measuring +// anything. It is a collaborative signal wearing similarity's clothes, and a +// raised SimilarityWeight amplifies it — if real ListenBrainz scores commonly +// land below 0.6 it would outrank genuine matches. Halved pending the +// fill-rate measurement in #3879; the honest fix is to stop it claiming a +// similarity score it never computed. 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: 20, // tier 2 — NOT trimmed: unseeded random(), see above - TasteOverlap: 10, // tier 3 floor — halved; deterministic ordering, safe - RandomFill: 30, // tier 3 floor — NOT trimmed: unseeded random(), see above + LikesOverlap: 10, // tier 2, halved — flat 0.6 sim_score, see above + TasteOverlap: 10, // tier 3 floor — halved, not removed + RandomFill: 10, // tier 3 floor — cut hard, never to zero } } @@ -187,6 +182,10 @@ 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, @@ -196,6 +195,7 @@ func LoadCandidatesFromSimilarity( currentVector SessionVector, exclude []pgtype.UUID, limits CandidateSourceLimits, + orderSeed string, ) ([]Candidate, error) { if exclude == nil { exclude = []pgtype.UUID{} @@ -212,6 +212,12 @@ 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 diff --git a/internal/recommendation/candidates_v2_test.go b/internal/recommendation/candidates_v2_test.go index 0fa19b55..af654157 100644 --- a/internal/recommendation/candidates_v2_test.go +++ b/internal/recommendation/candidates_v2_test.go @@ -2,6 +2,10 @@ package recommendation import ( "context" + "fmt" + "reflect" + "sort" + "strings" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -50,7 +54,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -82,7 +86,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -106,7 +110,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -133,7 +137,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -155,7 +159,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -176,7 +180,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(), + []pgtype.UUID{excluded}, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -192,7 +196,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -222,7 +226,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -242,7 +246,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -295,7 +299,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, + ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -321,7 +325,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(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -331,3 +335,97 @@ 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)) + } +} diff --git a/internal/recommendation/songs_like_limits_test.go b/internal/recommendation/songs_like_limits_test.go index 568df1f5..8e8bce87 100644 --- a/internal/recommendation/songs_like_limits_test.go +++ b/internal/recommendation/songs_like_limits_test.go @@ -59,41 +59,3 @@ 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) - } - } -} diff --git a/internal/server/server.go b/internal/server/server.go index af4780c0..37b13da2 100644 --- a/internal/server/server.go +++ b/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) + lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn, s.DataDir) 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")) diff --git a/internal/tracks/service.go b/internal/tracks/service.go index 27194e48..7abb98ec 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -1,9 +1,9 @@ -// Package tracks owns the track-level admin actions exposed by the -// M7 #372 track-actions menu. Today that's RemoveTrack: the destructive -// part is always handled directly by Minstrel (os.Remove + DB delete + -// cascade); when the operator opts in via `unmonitor=true` the service -// also tells Lidarr to flip the track's monitored flag off so Lidarr -// doesn't search for a replacement. +// Package tracks owns the track-level admin actions behind DELETE +// /api/admin/tracks/{id}. Today that's RemoveTrack: the destructive part goes +// through library.DeleteTrackFile — the one path that deletes a track file — +// and when the operator opts in via `unmonitor=true` the service also tells +// Lidarr to flip the track's monitored flag off so Lidarr doesn't search for a +// replacement. // // History: an earlier shape (commit 50a231f, since rewritten) routed // Lidarr-managed tracks through lidarrquarantine.DeleteViaLidarr — but @@ -12,6 +12,9 @@ // 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 ( @@ -19,15 +22,14 @@ 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 @@ -69,10 +71,10 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitore return &Service{pool: pool, logger: logger, lidarr: lidarr, dataDir: dataDir} } -// RemoveTrack deletes the file from disk and the DB rows, runs the -// album-empty / artist-empty cascade tidy-up, and (when unmonitor is -// true and the track is Lidarr-managed) tells Lidarr to flip the track's -// monitored flag off so it won't search for a replacement. +// RemoveTrack deletes the track's file and then its row, tidies away an album +// or artist the delete empties, and (when unmonitor is true and the track is +// Lidarr-managed) tells Lidarr to flip the track's monitored flag off so it +// won't search for a replacement. // // Returns: // - deletedAlbumID: non-nil when removing the track left the album @@ -81,9 +83,10 @@ 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: only for failures *before* the destructive part completes. -// A failed os.Remove is logged and tolerated. A failed Lidarr -// unmonitor is reflected in the bool flag, not the error. +// - err: ErrNotFound, or a failure before anything was deleted. When the +// file cannot be removed it is a *library.FileRemoveError and NOTHING was +// deleted — see library.DeleteTrackFile for why that order is the contract +// (#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 @@ -104,10 +107,9 @@ func (s *Service) RemoveTrack( return nil, nil, false, fmt.Errorf("get track: %w", err) } - // Capture the album's mbid *before* the cascade-delete transaction. - // If removing this track empties the album, DeleteAlbumIfEmpty - // removes the row and a post-commit GetAlbumByID would return - // pgx.ErrNoRows — leaving the unmonitor walk with no album mbid. + // Capture the album's mbid *before* the delete. If removing this track + // empties the album, its row is gone afterwards and the unmonitor walk + // would have no album mbid to name. var albumMbid string if track.Mbid != nil && *track.Mbid != "" && unmonitor && s.lidarr != nil { alb, alerr := q.GetAlbumByID(ctx, track.AlbumID) @@ -118,64 +120,14 @@ func (s *Service) RemoveTrack( // "no albumMbid → can't unmonitor → flag failure." } - // Always: remove the file. Tolerate already-missing. - if track.FilePath != "" { - if rerr := os.Remove(track.FilePath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) { - s.logger.Warn("track delete: file remove failed", - "path", track.FilePath, "track_id", trackID, "err", rerr) - // Proceed: DB consistency is the priority. + deleted, err := library.DeleteTrackFile(ctx, s.pool, s.logger, s.dataDir, trackID) + if err != nil { + if errors.Is(err, library.ErrTrackNotFound) { + return nil, nil, false, ErrNotFound } - } - - // 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 { @@ -193,5 +145,5 @@ func (s *Service) RemoveTrack( } } - return deletedAlbumID, deletedArtistID, lidarrUnmonitorFailed, nil + return deleted.AlbumID, deleted.ArtistID, lidarrUnmonitorFailed, nil } diff --git a/web/src/lib/api/errors.test.ts b/web/src/lib/api/errors.test.ts index d12f1e0b..d728c1fc 100644 --- a/web/src/lib/api/errors.test.ts +++ b/web/src/lib/api/errors.test.ts @@ -49,3 +49,37 @@ 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 + ); + }); +}); diff --git a/web/src/lib/api/errors.ts b/web/src/lib/api/errors.ts index a7417811..1f09f55d 100644 --- a/web/src/lib/api/errors.ts +++ b/web/src/lib/api/errors.ts @@ -8,11 +8,25 @@ 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 = 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 { - return copyForCode(errCode(err)) ?? fallback; + const code = errCode(err); + 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; } diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index c18961fd..80b94860 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -40,6 +40,8 @@ "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.",