From 50a231fdc1c104725531b5a1717ce55b167cd6a1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 2 May 2026 21:58:12 -0400 Subject: [PATCH] feat(tracks): RemoveTrack service with Lidarr-aware delete + cascade RemoveTrack handles both Lidarr-managed (delegates to existing lidarrquarantine.DeleteViaLidarr) and non-Lidarr (direct os.Remove) file deletion, then runs the cascade album-if-empty / artist-if-empty DB cleanup in a single transaction. Lidarr errors propagate as typed errors so the API layer can map them to the existing wire codes. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/tracks/service.go | 188 +++++++++++++++++ internal/tracks/service_test.go | 346 ++++++++++++++++++++++++++++++++ 2 files changed, 534 insertions(+) create mode 100644 internal/tracks/service.go create mode 100644 internal/tracks/service_test.go diff --git a/internal/tracks/service.go b/internal/tracks/service.go new file mode 100644 index 00000000..089b60e0 --- /dev/null +++ b/internal/tracks/service.go @@ -0,0 +1,188 @@ +// Package tracks owns the track-level admin actions exposed by the +// M7 #372 track-actions menu. Today that's RemoveTrack, which deletes +// a single track (Lidarr-managed via lidarrquarantine.DeleteViaLidarr, +// or local via os.Remove) and cascades the album-empty / artist-empty +// tidy-up so we don't leak orphan rows. +package tracks + +import ( + "context" + "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/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" +) + +// Public errors. Handlers map these to API codes. +var ( + ErrNotFound = errors.New("tracks: not found") + ErrLidarrUnreachable = errors.New("tracks: lidarr unreachable") + ErrLidarrUnauthorized = errors.New("tracks: lidarr unauthorized") + ErrLidarrServerError = errors.New("tracks: lidarr server error") +) + +// LidarrDeleter is the subset of *lidarrquarantine.Service the tracks +// service needs. Defined as an interface so tests can stub without +// spinning up a real Lidarr stub. +type LidarrDeleter interface { + DeleteViaLidarr(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, int, error) +} + +// Service owns RemoveTrack. lidarrDeleter may be nil — non-Lidarr-managed +// tracks (no mbid) still work fine; a Lidarr-managed track with a nil +// deleter falls back to direct os.Remove of the local file. +type Service struct { + pool *pgxpool.Pool + logger *slog.Logger + lidarrDeleter LidarrDeleter +} + +// NewService constructs a Service. logger may be nil (defaults to +// slog.Default). lidarrDeleter may be nil to disable the Lidarr-aware +// path entirely. +func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarrDeleter LidarrDeleter) *Service { + if logger == nil { + logger = slog.Default() + } + return &Service{pool: pool, logger: logger, lidarrDeleter: lidarrDeleter} +} + +// RemoveTrack deletes the track, runs the album-empty / artist-empty +// cascade, and returns the IDs of any rows deleted by the cascade so +// the API caller can invalidate caches. +// +// For Lidarr-managed tracks (mbid set on the track), Lidarr is told to +// delete the file first via lidarrquarantine.DeleteViaLidarr — this +// sets the import-list-exclusion flag so the next sync doesn't re-import +// the file. For local tracks (no mbid), os.Remove handles the file. +// +// Lidarr errors propagate as typed errors and the DB is left untouched +// so the operator can retry. A missing local file is tolerated — DB +// consistency takes priority over a noisy filesystem. +func (s *Service) RemoveTrack(ctx context.Context, trackID, adminID pgtype.UUID) (deletedAlbumID, deletedArtistID *pgtype.UUID, err error) { + q := dbq.New(s.pool) + + track, err := q.GetTrackByID(ctx, trackID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil, ErrNotFound + } + return nil, nil, fmt.Errorf("get track: %w", err) + } + + // Lidarr-managed → Lidarr deletes the album (and so the file) first. + // The lidarrquarantine path also deletes the local rows for every + // track on the album, so we pre-empt the cascade-tidy step here. If + // the lidarr deleter is nil, fall through to the local os.Remove path. + if track.Mbid != nil && *track.Mbid != "" && s.lidarrDeleter != nil { + _, _, lerr := s.lidarrDeleter.DeleteViaLidarr(ctx, trackID, adminID) + switch { + case lerr == nil: + // lidarrquarantine.DeleteViaLidarr already removed the tracks + // row(s) for the album. Cascade cleanup still needs to run + // against the album/artist IDs we captured before the call. + return s.cascadeAfterLidarr(ctx, q, track) + case errors.Is(lerr, lidarr.ErrUnreachable): + return nil, nil, ErrLidarrUnreachable + case errors.Is(lerr, lidarr.ErrAuthFailed): + return nil, nil, ErrLidarrUnauthorized + case errors.Is(lerr, lidarr.ErrServerError): + return nil, nil, ErrLidarrServerError + default: + return nil, nil, fmt.Errorf("lidarr delete: %w", lerr) + } + } + + // Local path: best-effort os.Remove, then DB cleanup in a tx. + 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. + } + } + + return s.deleteAndCascade(ctx, trackID) +} + +// deleteAndCascade runs DeleteTrack → DeleteAlbumIfEmpty → +// DeleteArtistIfEmpty in a single transaction. Returns the cascaded +// album / artist IDs (nil pointers when the cascade didn't fire because +// other tracks/albums still reference the parent). +func (s *Service) deleteAndCascade(ctx context.Context, trackID pgtype.UUID) (deletedAlbumID, deletedArtistID *pgtype.UUID, err error) { + tx, err := s.pool.Begin(ctx) + if err != nil { + return nil, nil, 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, 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 + default: + return nil, nil, fmt.Errorf("delete artist if empty: %w", arerr) + } + case errors.Is(aerr, pgx.ErrNoRows): + // album still has other tracks + default: + return nil, nil, fmt.Errorf("delete album if empty: %w", aerr) + } + + if err := tx.Commit(ctx); err != nil { + return nil, nil, fmt.Errorf("commit: %w", err) + } + return deletedAlbumID, deletedArtistID, nil +} + +// cascadeAfterLidarr runs the album-empty / artist-empty checks against +// the album/artist IDs from the track snapshot we took before delegating +// to lidarrquarantine. lidarrquarantine.DeleteViaLidarr already deleted +// the tracks row(s) for the entire album, so this is a tidy-up of the +// now-orphan album + artist parents. +func (s *Service) cascadeAfterLidarr(ctx context.Context, q *dbq.Queries, track dbq.Track) (deletedAlbumID, deletedArtistID *pgtype.UUID, err error) { + albumRow, aerr := q.DeleteAlbumIfEmpty(ctx, track.AlbumID) + switch { + case aerr == nil: + albumID := albumRow.ID + deletedAlbumID = &albumID + artistID, arerr := q.DeleteArtistIfEmpty(ctx, albumRow.ArtistID) + switch { + case arerr == nil: + id := artistID + deletedArtistID = &id + case errors.Is(arerr, pgx.ErrNoRows): + // artist still has other albums + default: + return nil, nil, fmt.Errorf("delete artist if empty: %w", arerr) + } + case errors.Is(aerr, pgx.ErrNoRows): + // album still has other tracks (unexpected after Lidarr delete, + // but tolerate — the operator may have re-imported between calls) + default: + return nil, nil, fmt.Errorf("delete album if empty: %w", aerr) + } + return deletedAlbumID, deletedArtistID, nil +} diff --git a/internal/tracks/service_test.go b/internal/tracks/service_test.go new file mode 100644 index 00000000..cafd7b56 --- /dev/null +++ b/internal/tracks/service_test.go @@ -0,0 +1,346 @@ +package tracks + +import ( + "context" + "errors" + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" +) + +// --- test seed helpers (inlined; mirrors lidarrquarantine patterns) ----- + +func newPool(t *testing.T) *pgxpool.Pool { + t.Helper() + if testing.Short() { + t.Skip("skipping integration test in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + return pool +} + +func seedAdmin(t *testing.T, pool *pgxpool.Pool, name string) dbq.User { + t.Helper() + u, err := dbq.New(pool).CreateUser(context.Background(), dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + name, PasswordHash: "x", + ApiToken: name + "-token", IsAdmin: true, + }) + if err != nil { + t.Fatalf("seed admin %s: %v", name, err) + } + return u +} + +// seedArtistAlbumTrack creates a (test-prefixed) artist + album + track +// row triple. mbid is set on both the album and the track when non-empty; +// empty mbid leaves both nil so the track exercises the local-delete path. +// filePath is the on-disk path stored on the track row. Returns the three +// rows. Uses unique names per call so multiple seeds in one test don't +// collide on UNIQUE (artist, name) etc. +func seedArtistAlbumTrack(t *testing.T, pool *pgxpool.Pool, prefix, mbid, filePath string) (dbq.Artist, dbq.Album, dbq.Track) { + t.Helper() + q := dbq.New(pool) + artist, err := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{ + Name: prefix + " Artist", SortName: prefix + " Artist", + }) + if err != nil { + t.Fatalf("artist: %v", err) + } + var albumMBIDPtr *string + if mbid != "" { + s := mbid + "-album" + albumMBIDPtr = &s + } + album, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{ + Title: prefix + " Album", SortTitle: prefix + " Album", + ArtistID: artist.ID, Mbid: albumMBIDPtr, + }) + if err != nil { + t.Fatalf("album: %v", err) + } + var trackMBIDPtr *string + if mbid != "" { + s := mbid + trackMBIDPtr = &s + } + track, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: prefix + " Track", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: filePath, FileSize: 1, FileFormat: "mp3", + Mbid: trackMBIDPtr, + }) + if err != nil { + t.Fatalf("track: %v", err) + } + return artist, album, track +} + +// --- fake LidarrDeleter ------------------------------------------------- + +type fakeLidarrDeleter struct { + calls []pgtype.UUID + err error +} + +func (f *fakeLidarrDeleter) DeleteViaLidarr(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, int, error) { + f.calls = append(f.calls, trackID) + if f.err != nil { + return dbq.LidarrQuarantineAction{}, 0, f.err + } + return dbq.LidarrQuarantineAction{}, 1, nil +} + +// --- tests -------------------------------------------------------------- + +func TestRemoveTrack_NotFound(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + var bogus pgtype.UUID + 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, nil, nil) + _, _, err := svc.RemoveTrack(context.Background(), bogus, admin.ID) + if !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} + +func TestRemoveTrack_NonLidarr_HappyPath(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + dir := t.TempDir() + path := filepath.Join(dir, "track.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + // mbid empty → local-delete path. + _, _, track := seedArtistAlbumTrack(t, pool, "Solo", "", path) + + svc := NewService(pool, nil, nil) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { + t.Errorf("file still present: %v", err) + } + q := dbq.New(pool) + if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { + t.Errorf("track row still exists") + } +} + +func TestRemoveTrack_NonLidarr_FileAlreadyMissing(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + // Path doesn't exist on disk; service should tolerate and clean up DB. + missing := filepath.Join(t.TempDir(), "does-not-exist.mp3") + _, _, track := seedArtistAlbumTrack(t, pool, "Ghost", "", missing) + + svc := NewService(pool, nil, nil) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + q := dbq.New(pool) + if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { + t.Errorf("track row still exists") + } +} + +func TestRemoveTrack_Lidarr_HappyPath(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + // mbid set → Lidarr path. Note: real lidarrquarantine.DeleteViaLidarr + // would have removed the tracks row already; the fake doesn't, so we + // use a track whose row will linger but cascadeAfterLidarr only acts + // on album/artist parents. The DB row staying behind for this test + // case is acceptable; the unit-of-work here is "DeleteViaLidarr was + // called, with the right trackID". + dir := t.TempDir() + path := filepath.Join(dir, "lidarr.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + _, _, track := seedArtistAlbumTrack(t, pool, "Lidarr", "track-mbid", path) + + fake := &fakeLidarrDeleter{} + svc := NewService(pool, nil, fake) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if len(fake.calls) != 1 { + t.Fatalf("DeleteViaLidarr call count = %d, want 1", len(fake.calls)) + } + if fake.calls[0] != track.ID { + t.Errorf("DeleteViaLidarr called with %v, want %v", fake.calls[0], track.ID) + } + // File should still exist — the fake doesn't actually delete it, + // and the local-os.Remove fallback is not exercised on the Lidarr path. + if _, err := os.Stat(path); err != nil { + t.Errorf("file unexpectedly missing on lidarr path: %v", err) + } +} + +func TestRemoveTrack_Lidarr_ErrorPropagation(t *testing.T) { + cases := []struct { + name string + lidarrE error + wantE error + }{ + {"unreachable", lidarr.ErrUnreachable, ErrLidarrUnreachable}, + {"unauthorized", lidarr.ErrAuthFailed, ErrLidarrUnauthorized}, + {"server", lidarr.ErrServerError, ErrLidarrServerError}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + dir := t.TempDir() + path := filepath.Join(dir, "lidarr.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + _, _, track := seedArtistAlbumTrack(t, pool, "LidErr", "track-mbid-"+tc.name, path) + + fake := &fakeLidarrDeleter{err: tc.lidarrE} + svc := NewService(pool, nil, fake) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + if !errors.Is(err, tc.wantE) { + t.Fatalf("err = %v, want %v", err, tc.wantE) + } + // DB row must remain — Lidarr error means no local mutation. + q := dbq.New(pool) + if _, gerr := q.GetTrackByID(context.Background(), track.ID); gerr != nil { + t.Errorf("track row missing after Lidarr error: %v", gerr) + } + }) + } +} + +func TestRemoveTrack_Cascade_AlbumEmpty(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + q := dbq.New(pool) + // Two-album artist: removing the sole track of album A leaves A + // orphaned, but the artist still has album B (so the artist cascade + // must NOT fire). + artist, err := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{ + Name: "Cascade Artist", SortName: "Cascade Artist", + }) + if err != nil { + t.Fatalf("artist: %v", err) + } + albumA, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{ + Title: "Album A", SortTitle: "Album A", ArtistID: artist.ID, + }) + if err != nil { + t.Fatalf("albumA: %v", err) + } + albumB, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{ + Title: "Album B", SortTitle: "Album B", ArtistID: artist.ID, + }) + if err != nil { + t.Fatalf("albumB: %v", err) + } + dir := t.TempDir() + pathA := filepath.Join(dir, "a.mp3") + pathB := filepath.Join(dir, "b.mp3") + _ = os.WriteFile(pathA, []byte("x"), 0o644) + _ = os.WriteFile(pathB, []byte("x"), 0o644) + trackA, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "TA", AlbumID: albumA.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: pathA, FileSize: 1, FileFormat: "mp3", + }) + if err != nil { + t.Fatalf("trackA: %v", err) + } + if _, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "TB", AlbumID: albumB.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: pathB, FileSize: 1, FileFormat: "mp3", + }); err != nil { + t.Fatalf("trackB: %v", err) + } + + svc := NewService(pool, nil, nil) + delAlbum, delArtist, err := svc.RemoveTrack(context.Background(), trackA.ID, admin.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if delAlbum == nil || *delAlbum != albumA.ID { + t.Errorf("deletedAlbumID = %v, want %v", delAlbum, albumA.ID) + } + if delArtist != nil { + t.Errorf("deletedArtistID = %v, want nil (artist still has album B)", delArtist) + } + // Album A should be gone; album B should still exist. + if _, err := q.GetAlbumByID(context.Background(), albumA.ID); err == nil { + t.Errorf("album A still exists after cascade") + } + if _, err := q.GetAlbumByID(context.Background(), albumB.ID); err != nil { + t.Errorf("album B unexpectedly removed: %v", err) + } + if _, err := q.GetArtistByID(context.Background(), artist.ID); err != nil { + t.Errorf("artist unexpectedly removed: %v", err) + } +} + +func TestRemoveTrack_Cascade_ArtistEmpty(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + dir := t.TempDir() + path := filepath.Join(dir, "lone.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + artist, album, track := seedArtistAlbumTrack(t, pool, "Lone", "", path) + + svc := NewService(pool, nil, nil) + delAlbum, delArtist, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if delAlbum == nil || *delAlbum != album.ID { + t.Errorf("deletedAlbumID = %v, want %v", delAlbum, album.ID) + } + if delArtist == nil || *delArtist != artist.ID { + t.Errorf("deletedArtistID = %v, want %v", delArtist, artist.ID) + } + q := dbq.New(pool) + if _, err := q.GetAlbumByID(context.Background(), album.ID); err == nil { + t.Errorf("album still exists after cascade") + } + if _, err := q.GetArtistByID(context.Background(), artist.ID); err == nil { + t.Errorf("artist still exists after cascade") + } +}