diff --git a/internal/lidarr/client.go b/internal/lidarr/client.go index f57ccbca..83f2f10c 100644 --- a/internal/lidarr/client.go +++ b/internal/lidarr/client.go @@ -83,12 +83,25 @@ func readBodySnippet(r io.Reader) string { // status codes to typed errors; the caller is responsible for closing the // returned body. func (c *Client) post(ctx context.Context, path string, body []byte) (*http.Response, error) { + return c.bodyRequest(ctx, http.MethodPost, path, body) +} + +// put is the shared PUT helper. Body must be marshaled JSON. Mirrors post() — +// same status-code mapping, same caller-closes-body contract. +func (c *Client) put(ctx context.Context, path string, body []byte) (*http.Response, error) { + return c.bodyRequest(ctx, http.MethodPut, path, body) +} + +// bodyRequest is the shared core for POST and PUT. Lidarr's monitor toggle +// is PUT-shaped, so factoring it out keeps the post/put helpers from +// duplicating the status-code mapping. +func (c *Client) bodyRequest(ctx context.Context, method, path string, body []byte) (*http.Response, error) { u, err := url.Parse(c.BaseURL) if err != nil { return nil, fmt.Errorf("%w: %v", ErrUnreachable, err) } u.Path = strings.TrimRight(u.Path, "/") + path - req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, method, u.String(), bytes.NewReader(body)) if err != nil { return nil, err } diff --git a/internal/lidarr/client_test.go b/internal/lidarr/client_test.go index 5f56f0bf..6ece5c5f 100644 --- a/internal/lidarr/client_test.go +++ b/internal/lidarr/client_test.go @@ -605,3 +605,179 @@ func TestPickPosterImage_SkipsNonPoster(t *testing.T) { t.Errorf("got %q, want empty (no poster)", got) } } + +// --- UnmonitorTrack --- + +// unmonitorMux is the three-endpoint stub UnmonitorTrack walks. The flags it +// captures are inspected by the individual tests. +type unmonitorMux struct { + t *testing.T + wantAlbumMbid string + albumStatus int // override status for album lookup; 0 → 200 with body + tracksStatus int // override status for tracks list; 0 → 200 with body + putStatus int // override status for PUT /track/monitor; 0 → 200 + albumBody []byte // when albumStatus is 0 + tracksBody []byte // when tracksStatus is 0 + putBody map[string]any + putHit bool +} + +func (m *unmonitorMux) ServeHTTP(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/album": + if got := r.URL.Query().Get("foreignAlbumId"); got != m.wantAlbumMbid { + m.t.Errorf("album lookup foreignAlbumId = %q, want %q", got, m.wantAlbumMbid) + } + if m.albumStatus != 0 { + w.WriteHeader(m.albumStatus) + return + } + _, _ = w.Write(m.albumBody) + case "/api/v1/track": + if got := r.URL.Query().Get("albumId"); got != "42" { + m.t.Errorf("track list albumId = %q, want 42", got) + } + if m.tracksStatus != 0 { + w.WriteHeader(m.tracksStatus) + return + } + _, _ = w.Write(m.tracksBody) + case "/api/v1/track/monitor": + if r.Method != http.MethodPut { + m.t.Errorf("monitor method = %q, want PUT", r.Method) + } + m.putHit = true + if m.putStatus != 0 { + w.WriteHeader(m.putStatus) + return + } + _ = json.NewDecoder(r.Body).Decode(&m.putBody) + w.WriteHeader(http.StatusAccepted) + default: + m.t.Errorf("unexpected path: %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } +} + +func TestUnmonitorTrack_HappyPath(t *testing.T) { + mux := &unmonitorMux{ + t: t, + wantAlbumMbid: "album-mbid", + albumBody: []byte(`[{"id":42,"foreignAlbumId":"album-mbid","title":"X","artistId":7}]`), + tracksBody: []byte(`[{"id":1001,"foreignTrackId":"other-mbid"},{"id":1002,"foreignTrackId":"track-mbid"}]`), + } + srv := httptest.NewServer(mux) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + + if err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid"); err != nil { + t.Fatalf("UnmonitorTrack: %v", err) + } + if !mux.putHit { + t.Fatal("PUT /api/v1/track/monitor was not called") + } + if mux.putBody["monitored"] != false { + t.Errorf("PUT body monitored = %v, want false", mux.putBody["monitored"]) + } + ids, ok := mux.putBody["trackIds"].([]any) + if !ok || len(ids) != 1 { + t.Fatalf("PUT body trackIds = %v, want [1002]", mux.putBody["trackIds"]) + } + if ids[0] != float64(1002) { + t.Errorf("PUT trackIds[0] = %v, want 1002 (the matching foreignTrackId)", ids[0]) + } +} + +func TestUnmonitorTrack_EmptyMbid(t *testing.T) { + c := &Client{BaseURL: "http://127.0.0.1:1", APIKey: "k", HTTP: &http.Client{}} + if err := c.UnmonitorTrack(context.Background(), "", "album-mbid"); err == nil { + t.Error("UnmonitorTrack with empty trackMbid: want error, got nil") + } + if err := c.UnmonitorTrack(context.Background(), "track-mbid", ""); err == nil { + t.Error("UnmonitorTrack with empty albumMbid: want error, got nil") + } +} + +func TestUnmonitorTrack_AlbumNotInLidarr(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/album" { + t.Errorf("unexpected path %q", r.URL.Path) + } + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestUnmonitorTrack_TrackNotInAlbum(t *testing.T) { + mux := &unmonitorMux{ + t: t, + wantAlbumMbid: "album-mbid", + albumBody: []byte(`[{"id":42,"foreignAlbumId":"album-mbid","title":"X","artistId":7}]`), + // No track with foreignTrackId="track-mbid" + tracksBody: []byte(`[{"id":1001,"foreignTrackId":"other-mbid"}]`), + } + srv := httptest.NewServer(mux) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } + if mux.putHit { + t.Error("PUT was called even though track wasn't found") + } +} + +func TestUnmonitorTrack_ServerError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrServerError) { + t.Fatalf("err = %v, want ErrServerError", err) + } +} + +func TestUnmonitorTrack_AuthFailed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrAuthFailed) { + t.Fatalf("err = %v, want ErrAuthFailed", err) + } +} + +func TestUnmonitorTrack_Unreachable(t *testing.T) { + c := &Client{BaseURL: "http://127.0.0.1:1", APIKey: "k", HTTP: &http.Client{}} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrUnreachable) { + t.Fatalf("err = %v, want ErrUnreachable", err) + } +} + +func TestUnmonitorTrack_PutServerError(t *testing.T) { + mux := &unmonitorMux{ + t: t, + wantAlbumMbid: "album-mbid", + albumBody: []byte(`[{"id":42,"foreignAlbumId":"album-mbid","title":"X","artistId":7}]`), + tracksBody: []byte(`[{"id":1002,"foreignTrackId":"track-mbid"}]`), + putStatus: http.StatusInternalServerError, + } + srv := httptest.NewServer(mux) + defer srv.Close() + c := &Client{BaseURL: srv.URL, APIKey: "k", HTTP: srv.Client()} + err := c.UnmonitorTrack(context.Background(), "track-mbid", "album-mbid") + if !errors.Is(err, ErrServerError) { + t.Fatalf("err = %v, want ErrServerError on PUT 5xx", err) + } +} diff --git a/internal/lidarr/unmonitor.go b/internal/lidarr/unmonitor.go new file mode 100644 index 00000000..81d541c7 --- /dev/null +++ b/internal/lidarr/unmonitor.go @@ -0,0 +1,91 @@ +package lidarr + +import ( + "context" + "encoding/json" + "fmt" + "net/url" + "strconv" +) + +// UnmonitorTrack flips Lidarr's `monitored` flag to false on the track +// matching trackMbid (a MusicBrainz id, aka Lidarr's `foreignTrackId`). +// Used by the M7 #372 admin "Remove from library" flow when the operator +// requests `?unmonitor=true` so Lidarr's monitor doesn't search for a +// replacement after we delete the local file + DB rows. +// +// Lidarr's monitor API takes its own internal numeric id, not the mbid, +// so this is a three-step walk: +// +// 1. GET /api/v1/album?foreignAlbumId={albumMbid} — find the album's +// internal Lidarr id. albumMbid is the parent album's mbid; the +// caller (tracks service) passes it because looking it up from the +// trackMbid alone would mean either a dedicated Lidarr endpoint +// that doesn't exist or scanning the entire library. +// 2. GET /api/v1/track?albumId={lidarr_album_id} — list the album's +// tracks, each with its internal id and foreignTrackId. +// 3. PUT /api/v1/track/monitor — body `{trackIds:[id], monitored:false}`. +// +// Returns ErrNotFound when the album or track isn't in Lidarr's library +// (already removed, or the track was never imported). Network / auth / +// 5xx errors propagate as typed sentinel errors so the tracks service +// can decide whether to surface a `lidarr_unmonitor_failed` flag. +func (c *Client) UnmonitorTrack(ctx context.Context, trackMbid, albumMbid string) error { + if trackMbid == "" { + return fmt.Errorf("lidarr: empty track mbid") + } + if albumMbid == "" { + return fmt.Errorf("lidarr: empty album mbid") + } + + // Step 1: resolve album mbid → Lidarr album id. + album, err := c.LookupAlbumByMBID(ctx, albumMbid) + if err != nil { + return err + } + if album.ID == 0 { + return ErrNotFound + } + + // Step 2: list tracks under that album, find the one whose + // foreignTrackId matches. + q := url.Values{"albumId": []string{strconv.Itoa(album.ID)}} + resp, err := c.get(ctx, "/api/v1/track", q) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + + var tracks []struct { + ID int `json:"id"` + ForeignTrackID string `json:"foreignTrackId"` + } + if err := json.NewDecoder(resp.Body).Decode(&tracks); err != nil { + return fmt.Errorf("%w: decode tracks: %v", ErrInvalidPayload, err) + } + var lidarrTrackID int + for _, t := range tracks { + if t.ForeignTrackID == trackMbid { + lidarrTrackID = t.ID + break + } + } + if lidarrTrackID == 0 { + return ErrNotFound + } + + // Step 3: PUT the monitor toggle. + body, err := json.Marshal(map[string]any{ + "trackIds": []int{lidarrTrackID}, + "monitored": false, + }) + if err != nil { + return fmt.Errorf("%w: marshal: %v", ErrInvalidPayload, err) + } + putResp, err := c.put(ctx, "/api/v1/track/monitor", body) + if err != nil { + return err + } + _ = putResp.Body.Close() + return nil +} diff --git a/internal/tracks/service.go b/internal/tracks/service.go index 089b60e0..5500a594 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -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 } diff --git a/internal/tracks/service_test.go b/internal/tracks/service_test.go index cafd7b56..8bb1d468 100644 --- a/internal/tracks/service_test.go +++ b/internal/tracks/service_test.go @@ -15,7 +15,6 @@ import ( "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) ----- @@ -53,12 +52,12 @@ func seedAdmin(t *testing.T, pool *pgxpool.Pool, name string) dbq.User { 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. +// seedArtistAlbumTrack creates an artist + album + track triple. mbid is +// set on both the album and the track when non-empty (the album mbid is +// derived from the track mbid suffix); empty leaves both nil so the +// track exercises the local-only path. filePath is the on-disk path +// stored on the track row. Names are unique per call so multiple seeds +// in one test don't collide on UNIQUE constraints. func seedArtistAlbumTrack(t *testing.T, pool *pgxpool.Pool, prefix, mbid, filePath string) (dbq.Artist, dbq.Album, dbq.Track) { t.Helper() q := dbq.New(pool) @@ -96,19 +95,21 @@ func seedArtistAlbumTrack(t *testing.T, pool *pgxpool.Pool, prefix, mbid, filePa return artist, album, track } -// --- fake LidarrDeleter ------------------------------------------------- +// --- fake LidarrUnmonitorer -------------------------------------------- -type fakeLidarrDeleter struct { - calls []pgtype.UUID - err error +// fakeLidarrUnmonitorer captures call args + a configurable error so +// tests can assert "called with mbid X" and "RemoveTrack tolerated this +// Lidarr error." +type fakeLidarrUnmonitorer struct { + calls []string // captured trackMbid per call + albumMbid string // last albumMbid received + 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 +func (f *fakeLidarrUnmonitorer) UnmonitorTrack(ctx context.Context, trackMbid, albumMbid string) error { + f.calls = append(f.calls, trackMbid) + f.albumMbid = albumMbid + return f.err } // --- tests -------------------------------------------------------------- @@ -122,13 +123,13 @@ func TestRemoveTrack_NotFound(t *testing.T) { bogus.Valid = true svc := NewService(pool, nil, nil) - _, _, err := svc.RemoveTrack(context.Background(), bogus, admin.ID) + _, _, _, err := svc.RemoveTrack(context.Background(), bogus, admin.ID, false) if !errors.Is(err, ErrNotFound) { t.Errorf("err = %v, want ErrNotFound", err) } } -func TestRemoveTrack_NonLidarr_HappyPath(t *testing.T) { +func TestRemoveTrack_LocalFile_HappyPath(t *testing.T) { pool := newPool(t) admin := seedAdmin(t, pool, "alice") @@ -137,14 +138,18 @@ func TestRemoveTrack_NonLidarr_HappyPath(t *testing.T) { if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { t.Fatal(err) } - // mbid empty → local-delete path. + // mbid empty → no Lidarr branch even with unmonitor=true. _, _, track := seedArtistAlbumTrack(t, pool, "Solo", "", path) - svc := NewService(pool, nil, nil) - _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + fake := &fakeLidarrUnmonitorer{} + svc := NewService(pool, nil, fake) + _, _, lidarrFailed, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, false) if err != nil { t.Fatalf("RemoveTrack: %v", err) } + if lidarrFailed { + t.Error("lidarrUnmonitorFailed = true; want false on local-file happy path") + } if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { t.Errorf("file still present: %v", err) } @@ -152,18 +157,21 @@ func TestRemoveTrack_NonLidarr_HappyPath(t *testing.T) { if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { t.Errorf("track row still exists") } + if len(fake.calls) != 0 { + t.Errorf("Lidarr called %d times; want 0 (no mbid, unmonitor=false)", len(fake.calls)) + } } -func TestRemoveTrack_NonLidarr_FileAlreadyMissing(t *testing.T) { +func TestRemoveTrack_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. + // Path doesn't exist on disk; service must 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) + _, _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, false) if err != nil { t.Fatalf("RemoveTrack: %v", err) } @@ -173,79 +181,6 @@ func TestRemoveTrack_NonLidarr_FileAlreadyMissing(t *testing.T) { } } -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") @@ -292,7 +227,7 @@ func TestRemoveTrack_Cascade_AlbumEmpty(t *testing.T) { } svc := NewService(pool, nil, nil) - delAlbum, delArtist, err := svc.RemoveTrack(context.Background(), trackA.ID, admin.ID) + delAlbum, delArtist, _, err := svc.RemoveTrack(context.Background(), trackA.ID, admin.ID, false) if err != nil { t.Fatalf("RemoveTrack: %v", err) } @@ -302,7 +237,6 @@ func TestRemoveTrack_Cascade_AlbumEmpty(t *testing.T) { 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") } @@ -326,7 +260,7 @@ func TestRemoveTrack_Cascade_ArtistEmpty(t *testing.T) { artist, album, track := seedArtistAlbumTrack(t, pool, "Lone", "", path) svc := NewService(pool, nil, nil) - delAlbum, delArtist, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID) + delAlbum, delArtist, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, false) if err != nil { t.Fatalf("RemoveTrack: %v", err) } @@ -344,3 +278,174 @@ func TestRemoveTrack_Cascade_ArtistEmpty(t *testing.T) { t.Errorf("artist still exists after cascade") } } + +// unmonitor=true AND mbid set → Lidarr called once; lidarrUnmonitorFailed=false. +// Uses two albums on the same artist so the album's row survives the +// cascade — that lets us assert the album mbid (captured pre-tx) is +// passed through correctly. +func TestRemoveTrack_Unmonitor_HappyPath(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + q := dbq.New(pool) + artist, err := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{ + Name: "Lidarr Artist", SortName: "Lidarr Artist", + }) + if err != nil { + t.Fatalf("artist: %v", err) + } + albumMbid := "album-mbid-xyz" + album, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{ + Title: "Lidarr Album", SortTitle: "Lidarr Album", + ArtistID: artist.ID, Mbid: &albumMbid, + }) + if err != nil { + t.Fatalf("album: %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) + trackMbid := "track-mbid-abc" + track, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "T1", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: pathA, FileSize: 1, FileFormat: "mp3", + Mbid: &trackMbid, + }) + if err != nil { + t.Fatalf("track1: %v", err) + } + // Sibling track keeps the album alive after the delete. + if _, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "T2", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: pathB, FileSize: 1, FileFormat: "mp3", + }); err != nil { + t.Fatalf("track2: %v", err) + } + + fake := &fakeLidarrUnmonitorer{} + svc := NewService(pool, nil, fake) + _, _, lidarrFailed, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, true) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if lidarrFailed { + t.Error("lidarrUnmonitorFailed = true; want false on happy path") + } + if len(fake.calls) != 1 { + t.Fatalf("Lidarr call count = %d, want 1", len(fake.calls)) + } + if fake.calls[0] != trackMbid { + t.Errorf("Lidarr called with mbid %q, want %q", fake.calls[0], trackMbid) + } + if fake.albumMbid != albumMbid { + t.Errorf("Lidarr called with albumMbid %q, want %q", fake.albumMbid, albumMbid) + } +} + +// unmonitor=true AND Lidarr fails → file + DB still deleted, flag set, +// service returns nil error. +func TestRemoveTrack_Unmonitor_LidarrErrorIsNonFatal(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + q := dbq.New(pool) + artist, err := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{ + Name: "ErrFatal Artist", SortName: "ErrFatal Artist", + }) + if err != nil { + t.Fatalf("artist: %v", err) + } + albumMbid := "err-album-mbid" + album, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{ + Title: "ErrFatal Album", SortTitle: "ErrFatal Album", + ArtistID: artist.ID, Mbid: &albumMbid, + }) + if err != nil { + t.Fatalf("album: %v", err) + } + dir := t.TempDir() + path := filepath.Join(dir, "f.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + trackMbid := "err-track-mbid" + track, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "T", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: path, FileSize: 1, FileFormat: "mp3", + Mbid: &trackMbid, + }) + if err != nil { + t.Fatalf("track: %v", err) + } + + fake := &fakeLidarrUnmonitorer{err: errors.New("simulated lidarr 5xx")} + svc := NewService(pool, nil, fake) + _, _, lidarrFailed, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, true) + if err != nil { + t.Fatalf("RemoveTrack: %v (want nil — Lidarr failure is non-fatal)", err) + } + if !lidarrFailed { + t.Error("lidarrUnmonitorFailed = false; want true after Lidarr error") + } + // File + DB still gone. + if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { + t.Errorf("file still present after Lidarr error: %v", err) + } + if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { + t.Errorf("track row still present after Lidarr error") + } +} + +// unmonitor=true AND mbid empty (non-Lidarr track) → no Lidarr call. +func TestRemoveTrack_Unmonitor_NoMbidSkipsLidarr(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + + dir := t.TempDir() + path := filepath.Join(dir, "local.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + _, _, track := seedArtistAlbumTrack(t, pool, "Local", "", path) + + fake := &fakeLidarrUnmonitorer{} + svc := NewService(pool, nil, fake) + _, _, lidarrFailed, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, true) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if lidarrFailed { + t.Error("lidarrUnmonitorFailed = true; want false (no mbid)") + } + if len(fake.calls) != 0 { + t.Errorf("Lidarr call count = %d, want 0 (track has no mbid)", len(fake.calls)) + } +} + +// unmonitor=false AND mbid set → no Lidarr call. +func TestRemoveTrack_NoUnmonitor_SkipsLidarr(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, "NoMon", "track-mbid", path) + + fake := &fakeLidarrUnmonitorer{} + svc := NewService(pool, nil, fake) + _, _, lidarrFailed, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID, false) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if lidarrFailed { + t.Error("lidarrUnmonitorFailed = true; want false (unmonitor=false)") + } + if len(fake.calls) != 0 { + t.Errorf("Lidarr call count = %d, want 0 (unmonitor=false)", len(fake.calls)) + } +}