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/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/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..9f14ae37 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) } @@ -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) @@ -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) 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.",