fix(tracks): rewrite RemoveTrack — direct os.Remove + optional Lidarr unmonitor
Replaces commit 50a231f's wrong-shape Lidarr-routed delete. The previous
design called lidarrquarantine.DeleteViaLidarr for Lidarr-managed tracks,
which deletes the entire **album** in Lidarr (Lidarr is album-granular,
no per-track delete API) — silently dropping sibling tracks the operator
didn't ask to remove.
New shape per spec revision 723eee9:
- Always: os.Remove + DB delete + cascade through Minstrel.
- When unmonitor=true AND track has mbid: call new Lidarr.UnmonitorTrack
primitive so Lidarr won't replace. Failure is non-fatal — file is
already gone, DB consistent; the lidarrUnmonitorFailed bool flows to
the wire response so the UI can surface a follow-up "unmonitor
manually" toast.
Service signature changes from `(trackID, adminID) → (delAlbum, delArtist, err)`
to `(trackID, adminID, unmonitor) → (delAlbum, delArtist, lidarrFailed, err)`.
Adds Lidarr.UnmonitorTrack with full three-step API walk:
1. GET /api/v1/album?foreignAlbumId={mbid} → Lidarr's internal album id
2. GET /api/v1/track?albumId={id} → match track by foreignTrackId
3. PUT /api/v1/track/monitor with {trackIds:[id], monitored:false}
Refactors post() into a shared bodyRequest() so put() reuses the same
status-code → typed-error mapping. The album mbid is captured *before*
the cascade-delete transaction — DeleteAlbumIfEmpty may remove the
album row, after which a post-commit GetAlbumByID returns ErrNoRows
and the unmonitor walk would have nothing to walk against.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+96
-104
@@ -1,8 +1,17 @@
|
||||
// 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.
|
||||
// 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.
|
||||
//
|
||||
// History: an earlier shape (commit 50a231f, since rewritten) routed
|
||||
// Lidarr-managed tracks through lidarrquarantine.DeleteViaLidarr — but
|
||||
// that primitive deletes the entire **album** in Lidarr (Lidarr is
|
||||
// album-granular and has no per-track delete API), which would silently
|
||||
// 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."
|
||||
package tracks
|
||||
|
||||
import (
|
||||
@@ -17,90 +26,92 @@ import (
|
||||
"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")
|
||||
)
|
||||
// ErrNotFound is returned when the track id doesn't resolve. Handler
|
||||
// maps to 404 `not_found`.
|
||||
var ErrNotFound = errors.New("tracks: not found")
|
||||
|
||||
// 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)
|
||||
// LidarrUnmonitorer is the subset of *lidarr.Client RemoveTrack uses.
|
||||
// Defined as an interface so tests can stub without spinning up a Lidarr
|
||||
// httptest server. UnmonitorTrack failures are non-fatal at the service
|
||||
// layer — the file + DB are already gone — so any error returned here
|
||||
// surfaces as a `lidarr_unmonitor_failed` flag in the response, not as
|
||||
// a hard error.
|
||||
type LidarrUnmonitorer interface {
|
||||
UnmonitorTrack(ctx context.Context, trackMbid, albumMbid string) 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.
|
||||
// Service owns RemoveTrack. lidarr may be nil — when it is, the
|
||||
// unmonitor branch is skipped entirely (with `lidarrUnmonitorFailed`
|
||||
// remaining false) regardless of the unmonitor query param. This is
|
||||
// the right fallback when Lidarr isn't configured: file + DB delete
|
||||
// still happen.
|
||||
type Service struct {
|
||||
pool *pgxpool.Pool
|
||||
logger *slog.Logger
|
||||
lidarrDeleter LidarrDeleter
|
||||
pool *pgxpool.Pool
|
||||
logger *slog.Logger
|
||||
lidarr LidarrUnmonitorer
|
||||
}
|
||||
|
||||
// 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 {
|
||||
// slog.Default). lidarr may be nil to disable the unmonitor branch.
|
||||
func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitorer) *Service {
|
||||
if logger == nil {
|
||||
logger = slog.Default()
|
||||
}
|
||||
return &Service{pool: pool, logger: logger, lidarrDeleter: lidarrDeleter}
|
||||
return &Service{pool: pool, logger: logger, lidarr: lidarr}
|
||||
}
|
||||
|
||||
// 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.
|
||||
// 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.
|
||||
//
|
||||
// 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.
|
||||
// Returns:
|
||||
// - deletedAlbumID: non-nil when removing the track left the album
|
||||
// empty and the album row was deleted.
|
||||
// - deletedArtistID: non-nil when both album AND artist were left
|
||||
// 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.
|
||||
//
|
||||
// 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) {
|
||||
// adminID is currently unused — the cascade audit-log line that would
|
||||
// reference it isn't wired in this slice. It's threaded through the
|
||||
// signature so the upcoming admin_tracks handler doesn't have to
|
||||
// re-plumb when audit logging lands.
|
||||
func (s *Service) RemoveTrack(
|
||||
ctx context.Context,
|
||||
trackID, adminID pgtype.UUID,
|
||||
unmonitor bool,
|
||||
) (deletedAlbumID *pgtype.UUID, deletedArtistID *pgtype.UUID, lidarrUnmonitorFailed bool, 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, false, ErrNotFound
|
||||
}
|
||||
return nil, nil, fmt.Errorf("get track: %w", err)
|
||||
return nil, nil, false, 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)
|
||||
// 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.
|
||||
var albumMbid string
|
||||
if track.Mbid != nil && *track.Mbid != "" && unmonitor && s.lidarr != nil {
|
||||
alb, alerr := q.GetAlbumByID(ctx, track.AlbumID)
|
||||
if alerr == nil && alb.Mbid != nil {
|
||||
albumMbid = *alb.Mbid
|
||||
}
|
||||
// Lookup failure is tolerated — handled below as
|
||||
// "no albumMbid → can't unmonitor → flag failure."
|
||||
}
|
||||
|
||||
// Local path: best-effort os.Remove, then DB cleanup in a tx.
|
||||
// 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",
|
||||
@@ -109,17 +120,10 @@ func (s *Service) RemoveTrack(ctx context.Context, trackID, adminID pgtype.UUID)
|
||||
}
|
||||
}
|
||||
|
||||
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) {
|
||||
// DB cleanup in a transaction so a midway failure leaves things consistent.
|
||||
tx, err := s.pool.Begin(ctx)
|
||||
if err != nil {
|
||||
return nil, nil, fmt.Errorf("begin tx: %w", err)
|
||||
return nil, nil, false, fmt.Errorf("begin tx: %w", err)
|
||||
}
|
||||
defer func() { _ = tx.Rollback(ctx) }()
|
||||
|
||||
@@ -127,7 +131,7 @@ func (s *Service) deleteAndCascade(ctx context.Context, trackID pgtype.UUID) (de
|
||||
|
||||
deleted, err := tq.DeleteTrack(ctx, trackID)
|
||||
if err != nil {
|
||||
return nil, nil, fmt.Errorf("delete track: %w", err)
|
||||
return nil, nil, false, fmt.Errorf("delete track: %w", err)
|
||||
}
|
||||
|
||||
albumRow, aerr := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID)
|
||||
@@ -141,48 +145,36 @@ func (s *Service) deleteAndCascade(ctx context.Context, trackID pgtype.UUID) (de
|
||||
id := artistID
|
||||
deletedArtistID = &id
|
||||
case errors.Is(arerr, pgx.ErrNoRows):
|
||||
// artist still has other albums or stray tracks
|
||||
// Artist still has other albums or stray tracks. OK.
|
||||
default:
|
||||
return nil, nil, fmt.Errorf("delete artist if empty: %w", arerr)
|
||||
return nil, nil, false, fmt.Errorf("delete artist if empty: %w", arerr)
|
||||
}
|
||||
case errors.Is(aerr, pgx.ErrNoRows):
|
||||
// album still has other tracks
|
||||
// Album still has other tracks. OK.
|
||||
default:
|
||||
return nil, nil, fmt.Errorf("delete album if empty: %w", aerr)
|
||||
return nil, nil, false, fmt.Errorf("delete album if empty: %w", aerr)
|
||||
}
|
||||
|
||||
if err := tx.Commit(ctx); err != nil {
|
||||
return nil, nil, fmt.Errorf("commit: %w", err)
|
||||
return nil, nil, false, 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)
|
||||
// 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 {
|
||||
if albumMbid == "" {
|
||||
// Couldn't capture the album mbid (album row had nil mbid
|
||||
// or was missing somehow). The Lidarr walk needs it; mark
|
||||
// failure rather than calling with an empty string.
|
||||
s.logger.Warn("track delete: lidarr unmonitor skipped — no album mbid",
|
||||
"track_id", trackID, "track_mbid", *track.Mbid)
|
||||
lidarrUnmonitorFailed = true
|
||||
} else if uerr := s.lidarr.UnmonitorTrack(ctx, *track.Mbid, albumMbid); uerr != nil {
|
||||
s.logger.Warn("track delete: lidarr unmonitor failed",
|
||||
"track_id", trackID, "track_mbid", *track.Mbid, "err", uerr)
|
||||
lidarrUnmonitorFailed = true
|
||||
}
|
||||
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
|
||||
|
||||
return deletedAlbumID, deletedArtistID, lidarrUnmonitorFailed, nil
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user