fix(library): a track delete that cannot remove its file deletes nothing — #3918

Two delete paths had opposite failure policies. tracks.RemoveTrack
logged a failed os.Remove and deleted the row anyway, which CASCADEs
likes, plays, playlist memberships and tags, while the file survived
for the next scan to re-import as a stranger. library.DeleteTrackFile
stopped correctly but reported it as a bare 500 nobody could read.

One path now: library.DeleteTrackFile removes the file first and, on
anything but ErrNotExist, returns *FileRemoveError with nothing
deleted. Only then does it delete the row and tidy an emptied album
and artist in one transaction, log the sync change and clear orphaned
artist art. RemoveTrack calls it, which also fixes RemoveTrack never
logging a sync change. Quarantine Delete file now tidies emptied
albums and artists too.

Both endpoints answer an unwritable library (EROFS, EACCES, EPERM) with
409 library_not_writable. The message names the directory (removal
writes to the parent), the uid:gid the server runs as, and that
nothing was deleted. Other remove errors are 500 file_delete_failed
with the path.

The reachable surface is quarantine Delete file, which failed
silently: no copy for the code on either client, and Android swallowed
the exception so the row just reappeared. Web and Android now have
copy for both codes and append the server message for exactly those
two. Android's quarantine screen shows it in a snackbar.

DELETE /api/admin/tracks/{id} has had no client since f7278f24, which
kept it on purpose for a safer admin surface, so its history loss was
latent. Fixed rather than removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH

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
 }

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)
 	}