From 8badec2d62bb4d17ee08436b8f9f95e676fe8e9d Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 30 Aug 2026 20:03:10 -0400 Subject: [PATCH] refactor(artwork): drop what the removed retry paths left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting the absent-retry jobs and the startup backfill orphaned more than the code that went with them. `Worker.agents` lost its only reader when Backfill went. `RadioRepository.GetAllIDs` existed solely for that backfill's bulk enumeration, as its own comment said. `CountAbsent` became a second query for a number `artwork status` already had: only two paths write item_artwork, and only the absent one leaves `source` empty, so `hash = ''` and `source = ''` select the same rows. The absent count now comes from the source breakdown collected in the same loop, four queries cheaper. Two defects surfaced while pulling on that thread. `reprocess --all` returned on an empty match set before recording the fingerprint, so the command the startup warning names could not silence it. And the "did this run apply the config" flag was derived in the CLI from its own globals, which made `--kind ar --kind al --kind pl --kind rd` — work identical to `--all` — leave the fingerprint stale. It is now derived inside reprocessArtwork from the kinds and sources that drive the queries, so a filter added later has to pass through it. settlesAbsentOnGiveUp decided a worker persistence policy by testing membership in a CLI-facing kind list. It now states the policy directly. Its rationale was wrong too: media files already get an absent row on the definitive path, and the reason to skip them on give-up is that a track with no row still falls back to disc or album art. CheckConfigFingerprint wrote to the database on one of three branches, so it is ReconcileConfigFingerprint now. --- cmd/artwork.go | 45 ++++++++------- cmd/artwork_test.go | 58 ++++++++++---------- cmd/root.go | 2 +- core/artwork/housekeeping.go | 10 ++-- core/artwork/housekeeping_test.go | 48 ++-------------- core/artwork/worker.go | 8 +-- core/artwork/worker_test.go | 24 ++++++++ model/artwork.go | 5 +- model/radio.go | 1 - persistence/artwork_queue_repository.go | 7 --- persistence/artwork_queue_repository_test.go | 18 ------ persistence/radio_repository.go | 8 --- persistence/radio_repository_test.go | 12 ---- tests/mock_artwork_queue_repo.go | 16 ------ tests/mock_radio_repository.go | 9 --- 15 files changed, 95 insertions(+), 176 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index 55aebd1bf..2fb696ca4 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -113,7 +113,7 @@ var artworkCancelCmd = &cobra.Command{ "Work already picked up is not interrupted, and an item with no artwork yet can be\n" + "queued again by the hourly re-check. The selection is applied again when you confirm,\n" + "so anything queued after the preview is cancelled too. Use it to call off a bulk\n" + - "backfill, not to stop the worker.", + "reprocess, not to stop the worker.", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { runCancel(cmd.Context()) @@ -122,7 +122,7 @@ var artworkCancelCmd = &cobra.Command{ var artworkStatusCmd = &cobra.Command{ Use: "status", - Short: "Report the artwork queue, where artwork resolves from, and the backfill state", + Short: "Report the artwork queue, where artwork resolves from, and the config state", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { runStatus(cmd.Context()) @@ -190,12 +190,11 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error return rep, fmt.Errorf("counting %s artwork resolved from %s: %w", k, displaySource(s), err) } rep.sources = append(rep.sources, sourceCount{kind: k, source: s, count: n}) + // An absent state is exactly a row with no source, so it needs no second query. + if s == "" { + rep.absent = append(rep.absent, absentCount{kind: k, count: n}) + } } - n, err := q.CountAbsent(k) - if err != nil { - return rep, fmt.Errorf("counting absent %s artwork: %w", k, err) - } - rep.absent = append(rep.absent, absentCount{kind: k, count: n}) } rep.current, rep.inputs = artwork.ConfigFingerprint(), artwork.FingerprintInputs() @@ -227,7 +226,7 @@ func formatStatus(rep statusReport) string { for _, a := range rep.absent { fmt.Fprintf(w, " %s\t%d\n", a.kind, a.count) } - fmt.Fprintln(w, " (never retried on their own, not even by a backfill; run 'artwork reprocess --source absent')") + fmt.Fprintln(w, " (never retried on their own; run 'artwork reprocess --source absent')") fmt.Fprintln(w, "\nConfig") fmt.Fprintf(w, " State:\t%s\n", configState(rep)) @@ -277,8 +276,8 @@ type artworkPriority struct { var knownPriorities = []artworkPriority{ {"bump", model.ArtworkPriorityBump}, {"scan", model.ArtworkPriorityScan}, - {"backfill", model.ArtworkPriorityBackfill}, {"recheck", model.ArtworkPriorityRecheck}, + {"backfill", model.ArtworkPriorityBackfill}, } // priorityName falls back to the number: a row written by a newer version still has to print. @@ -322,10 +321,8 @@ func runReprocess(ctx context.Context) { imageAgents = artwork.NewImageAgentCount(agents.GetAgents(ds, mgr)) } - // Only a whole-library, unfiltered run leaves nothing resolved under the old config. - full := artworkAll && len(artworkSources) == 0 if err := reprocessArtwork(ctx, ds, kinds, repositorySources(artworkSources), imageAgents, - artworkDryRun, full, confirmUnlessYes(artworkYes, os.Stdin, "re-resolve"), os.Stdout); err != nil { + artworkDryRun, confirmUnlessYes(artworkYes, os.Stdin, "re-resolve"), os.Stdout); err != nil { log.Fatal(ctx, err) } } @@ -454,12 +451,24 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error { // reprocessArtwork previews from CountBySource — rows matched — then reports what EnqueueBySource // actually inserted; the two differ because an already-queued row is left untouched. func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kind, sources []string, - imageAgents artwork.ImageAgentCount, dryRun, full bool, confirm confirmFunc, out io.Writer) error { + imageAgents artwork.ImageAgentCount, dryRun bool, confirm confirmFunc, out io.Writer) error { q := ds.ArtworkQueue(ctx) if err := validateSources(q, sources); err != nil { return err } + // Derived from what actually drives the queries, so a filter added to this signature cannot + // silently keep stamping the fingerprint for a partial run. + markApplied := func() error { + if len(sources) > 0 || len(kinds) < len(artwork.ReprocessKinds) { + return nil + } + if err := artwork.MarkConfigApplied(ctx, ds); err != nil { + return fmt.Errorf("recording the applied artwork config: %w", err) + } + return nil + } + matched := make([]int64, len(kinds)) var total, external int64 for i, k := range kinds { @@ -478,8 +487,9 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin fmt.Fprintln(out, "\nDry run: nothing was queued.") return nil case total == 0: + // An empty match set still leaves nothing resolved under the old config. fmt.Fprintln(out, "Nothing was queued.") - return nil + return markApplied() case !confirm(out, total, external): fmt.Fprintln(out, "Aborted: nothing was queued.") return nil @@ -501,12 +511,7 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin if skipped := total - queued; skipped > 0 { fmt.Fprintf(out, "Already queued, left unchanged: %d (priority and retry backoff untouched).\n", skipped) } - if full { - if err := artwork.MarkConfigApplied(ctx, ds); err != nil { - return fmt.Errorf("recording the applied artwork config: %w", err) - } - } - return nil + return markApplied() } func runCancel(ctx context.Context) { diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 8a3d92825..6a3ade04a 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -556,10 +556,6 @@ var _ = Describe("confirmUnlessYes", func() { }) }) -// unchangedFingerprint is the seeded value, deferred like the real one: an Entry argument is -// evaluated at tree-construction time, before the suite sets the config the fingerprint hashes. -func unchangedFingerprint() string { return "stale-fingerprint" } - var _ = Describe("reprocessArtwork", func() { var ds *tests.MockDataStore var art *tests.MockArtworkRepo @@ -593,7 +589,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("previews the per-kind breakdown and queues nothing on a dry run", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, true, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("external:deezer")) Expect(out.String()).To(ContainSubstring("artist")) @@ -604,27 +600,33 @@ var _ = Describe("reprocessArtwork", func() { }) It("queues nothing when the operator declines", func() { - Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, false, decline, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, decline, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Aborted")) Expect(queue.Count()).To(BeZero()) }) DescribeTable("records the applied config only for a run that leaves nothing on the old one", - func(sources []string, dryRun, full bool, expected func() string) { + func(selected []model.Kind, sources []string, dryRun, applied bool) { Expect(ds.Property(ctx).Put(consts.ArtConfFingerprintPropertyKey, "stale-fingerprint")).To(Succeed()) - Expect(reprocessArtwork(ctx, ds, kinds, sources, imageAgents, dryRun, full, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, selected, sources, imageAgents, dryRun, accept, &out)).To(Succeed()) - Expect(ds.Property(ctx).Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(expected())) + want := "stale-fingerprint" + if applied { + want = artwork.ConfigFingerprint() + } + Expect(ds.Property(ctx).Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(want)) }, - Entry("--all, unfiltered", nil, false, true, artwork.ConfigFingerprint), - Entry("filtered by source", []string{"external:deezer"}, false, false, unchangedFingerprint), - Entry("a dry run applies nothing", nil, true, true, unchangedFingerprint), + Entry("every kind, unfiltered", artwork.ReprocessKinds, nil, false, true), + Entry("every kind, but nothing matched", artwork.ReprocessKinds, []string{}, false, true), + Entry("filtered by source", artwork.ReprocessKinds, []string{"external:deezer"}, false, false), + Entry("a subset of kinds", []model.Kind{model.KindAlbumArtwork}, nil, false, false), + Entry("a dry run applies nothing", artwork.ReprocessKinds, nil, true, false), ) It("queues the matching items at recheck priority, leaving their artwork state alone", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(2))) queued, err := queue.Get(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) @@ -639,7 +641,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("targets the absent state", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(1))) _, err := queue.Get(model.KindArtistArtwork, "ar-2", model.ImageTypePrimary) @@ -650,7 +652,7 @@ var _ = Describe("reprocessArtwork", func() { Expect(queue.Enqueue(model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar-1", ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityBump})).To(Succeed()) - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Queued 1 of 2 matched items")) Expect(out.String()).To(ContainSubstring("Already queued, left unchanged: 1")) @@ -661,7 +663,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("stops at a selection that matches nothing instead of prompting", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, false, false, + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, false, func(io.Writer, int64, int64) bool { Fail("must not prompt when there is nothing to queue") return true @@ -672,14 +674,14 @@ var _ = Describe("reprocessArtwork", func() { }) It("reports an empty selection as a dry run when one was asked for", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, true, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Nothing matches")) Expect(out.String()).To(ContainSubstring("Dry run")) }) It("shows the external estimate on a dry run, which never reaches the prompt", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, imageAgents, true, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("External lookups: ~2 estimated")) }) @@ -689,14 +691,14 @@ var _ = Describe("reprocessArtwork", func() { var external int64 capture := func(_ io.Writer, _, e int64) bool { external = e; return false } - Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, false, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(2*2+2*3)), "2 artists at 2 agents plus 2 albums at 3 agents") Expect(out.String()).To(ContainSubstring("External lookups: ~10 estimated")) }) It("names the estimate's blind spots instead of claiming a bound it cannot hold", func() { - Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, true, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, true, accept, &out)).To(Succeed()) // The count includes plugin agents once they load, so the caveat is about a failed load, // not about plugins being invisible to the CLI. @@ -710,7 +712,7 @@ var _ = Describe("reprocessArtwork", func() { conf.Server.CoverArtPriority = "cover.*" put(model.KindPlaylistArtwork, "pl-1", "playlist") - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, true, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("External lookups: none")) }) @@ -722,7 +724,7 @@ var _ = Describe("reprocessArtwork", func() { var external int64 capture := func(_ io.Writer, _, e int64) bool { external = e; return false } - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, false, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(1))) Expect(out.String()).To(ContainSubstring("External lookups: ~1 estimated")) @@ -734,7 +736,7 @@ var _ = Describe("reprocessArtwork", func() { var external int64 capture := func(_ io.Writer, _, e int64) bool { external = e; return false } - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, false, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(artwork.PlaylistGridSamples*3)), "one playlist samples 4 albums, each walking all 3 album agents") @@ -746,14 +748,14 @@ var _ = Describe("reprocessArtwork", func() { capture := func(_ io.Writer, t, e int64) bool { total, external = t, e; return false } Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork, model.KindRadioArtwork}, - nil, imageAgents, false, false, capture, &out)).To(Succeed()) + nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(total).To(Equal(int64(3))) Expect(external).To(Equal(int64(2)), "radio artwork never reaches an external agent") }) It("rejects an unknown source and names the ones in use", func() { - err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, imageAgents, true, false, accept, &out) + err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, imageAgents, true, accept, &out) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("externa:deezer")) @@ -767,18 +769,18 @@ var _ = Describe("reprocessArtwork", func() { put(model.KindArtistArtwork, "ar-2", "folder") Expect(reprocessArtwork(ctx, ds, kinds, repositorySources([]string{absentSource}), - imageAgents, false, false, accept, &out)).To(Succeed(), + imageAgents, false, accept, &out)).To(Succeed(), "a reserved source must stay valid once the library has none of it") Expect(out.String()).To(ContainSubstring("Nothing matches")) Expect(queue.Count()).To(BeZero()) Expect(reprocessArtwork(ctx, ds, kinds, repositorySources([]string{"absnt"}), - imageAgents, true, false, accept, &out)).ToNot(Succeed(), "a typo must still be rejected") + imageAgents, true, accept, &out)).ToNot(Succeed(), "a typo must still be rejected") }) It("accepts a source another kind uses, letting the empty selection report itself", func() { Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindArtistArtwork}, []string{"folder"}, - imageAgents, false, false, decline, &out)).To(Succeed()) + imageAgents, false, decline, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Nothing matches"), "a well-formed filter must not be reported as a typo because of the kinds selected") diff --git a/cmd/root.go b/cmd/root.go index a7876401b..a44eb743b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -385,7 +385,7 @@ func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) fu log.Error(ctx, "Error enqueueing missing artwork rechecks", err) } - if err := worker.CheckConfig(ctx); err != nil { + if err := worker.ReconcileConfig(ctx); err != nil { log.Error(ctx, "Error checking the artwork config fingerprint", err) } return nil diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index cb714ac43..2e07fb296 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -31,10 +31,10 @@ func KeepsState(kind model.Kind) bool { return kind != model.KindDiscArtwork } var RefreshableKinds = append(slices.Clone(ReprocessKinds), model.KindMediaFileArtwork) // settlesAbsentOnGiveUp reports whether an exhausted retry budget records an absent state. Media -// files are excluded: only a view enqueues them, and an absent row is what stops a view from doing so. +// files are excluded: a track with no row still falls back to its disc or album art. func settlesAbsentOnGiveUp(prefix string) bool { kind, ok := model.ParseKind(prefix) - return ok && slices.Contains(ReprocessKinds, kind) + return ok && KeepsState(kind) && kind != model.KindMediaFileArtwork } // artworkEpoch invalidates all resolution state when bumped; bump it whenever resolution semantics change. @@ -65,9 +65,9 @@ func ConfigFingerprint() string { return fmt.Sprintf("%016x", xxh3.Hash([]byte(raw))) } -// CheckConfigFingerprint warns when the artwork config changed since the library was last +// ReconcileConfigFingerprint warns when the artwork config changed since the library was last // resolved under it. Nothing re-resolves on its own; applying a change is an explicit reprocess. -func CheckConfigFingerprint(ctx context.Context, ds model.DataStore) error { +func ReconcileConfigFingerprint(ctx context.Context, ds model.DataStore) error { current := ConfigFingerprint() stored, err := ds.Property(ctx).DefaultGet(consts.ArtConfFingerprintPropertyKey, "") if err != nil { @@ -76,7 +76,7 @@ func CheckConfigFingerprint(ctx context.Context, ds model.DataStore) error { switch stored { case current: case "": - // A library with no stored fingerprint has nothing resolved under an older one to warn about. + // An unset fingerprint counts as current; the alternative warns every upgrading install once. return MarkConfigApplied(ctx, ds) default: log.Warn(ctx, "Artwork: Config changed since the last full reprocess. Stored artwork keeps "+ diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go index 594613dba..9a108a0a9 100644 --- a/core/artwork/housekeeping_test.go +++ b/core/artwork/housekeeping_test.go @@ -3,41 +3,16 @@ package artwork import ( "context" "slices" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -// visibilityPlaylistDS models playlist_repository's userFilter: a private playlist is only -// visible when the ctx carries an admin, so headless work must wrap ctx with one first. -type visibilityPlaylistDS struct { - *tests.MockDataStore - private model.Playlist - tracks model.PlaylistTrackRepository -} - -func (v *visibilityPlaylistDS) Playlist(ctx context.Context) model.PlaylistRepository { - repo := tests.CreateMockPlaylistRepo() - repo.TracksRepo = v.tracks - if u, ok := request.UserFrom(ctx); ok && u.IsAdmin { - repo.SetData(model.Playlists{v.private}) - } - return repo -} - -func adminUserRepo() *tests.MockedUserRepo { - repo := tests.CreateMockUserRepo() - Expect(repo.Put(&model.User{ID: "admin", UserName: "admin", IsAdmin: true})).To(Succeed()) - return repo -} - // orderTrackingQueueRepo records the kind of each bulk insert, so tests can assert // phase ordering (artists-first) that same-priority timestamps can't guarantee. type orderTrackingQueueRepo struct { @@ -138,9 +113,9 @@ var _ = Describe("Housekeeping", func() { }) }) - Describe("CheckConfigFingerprint", func() { + Describe("ReconcileConfigFingerprint", func() { It("records the current fingerprint when none was ever stored", func() { - Expect(CheckConfigFingerprint(ctx, ds)).To(Succeed()) + Expect(ReconcileConfigFingerprint(ctx, ds)).To(Succeed()) Expect(propRepo.Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(ConfigFingerprint())) Expect(queueRepo.Count()).To(BeZero()) @@ -149,23 +124,10 @@ var _ = Describe("Housekeeping", func() { It("leaves a stale fingerprint stored, so the warning survives a restart", func() { Expect(propRepo.Put(consts.ArtConfFingerprintPropertyKey, "stale-fingerprint")).To(Succeed()) - Expect(CheckConfigFingerprint(ctx, ds)).To(Succeed()) + Expect(ReconcileConfigFingerprint(ctx, ds)).To(Succeed()) Expect(propRepo.Get(consts.ArtConfFingerprintPropertyKey)).To(Equal("stale-fingerprint")) }) - - DescribeTable("never enqueues anything, whatever the stored fingerprint", - func(stored func() string) { - if v := stored(); v != "" { - Expect(propRepo.Put(consts.ArtConfFingerprintPropertyKey, v)).To(Succeed()) - } - Expect(CheckConfigFingerprint(ctx, ds)).To(Succeed()) - Expect(queueRepo.Count()).To(BeZero()) - }, - Entry("never stored", func() string { return "" }), - Entry("changed", func() string { return "stale-fingerprint" }), - Entry("unchanged", ConfigFingerprint), - ) }) Describe("MarkConfigApplied", func() { @@ -194,8 +156,8 @@ var _ = Describe("Housekeeping", func() { }) It("enqueues only entities that have no item_artwork row, across all kinds", func() { - artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: time.Now()} - artRepo.ItemData["ar-absent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()} + artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "somehash"} + artRepo.ItemData["ar-absent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: ""} err := enqueueMissingAll(ctx, ds) Expect(err).ToNot(HaveOccurred()) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 120d6ac31..bb09be55e 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -40,7 +40,6 @@ type drainPool struct { // independently, and pruneMu serializes prune against the store-write window. type Worker struct { proc *processor - agents *agents.Agents cache cache.FileCache ffmpeg ffmpeg.FFmpeg broker events.Broker @@ -55,7 +54,6 @@ type Worker struct { func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg ffmpeg.FFmpeg, broker events.Broker, imgCache cache.FileCache) *Worker { w := &Worker{ proc: &processor{ds: ds, store: store}, - agents: ag, cache: imgCache, ffmpeg: ffmpeg, broker: broker, @@ -133,9 +131,9 @@ func (w *Worker) RunPrune(ctx context.Context) error { return prune(ctx, w.proc.ds, w.proc.store) } -// CheckConfig warns when the artwork config changed since the library was last resolved under it. -func (w *Worker) CheckConfig(ctx context.Context) error { - return CheckConfigFingerprint(ctx, w.proc.ds) +// ReconcileConfig records the artwork config fingerprint, or warns when it changed. +func (w *Worker) ReconcileConfig(ctx context.Context) error { + return ReconcileConfigFingerprint(ctx, w.proc.ds) } // EnqueueMissingAll requeues entities with no artwork state row: the safety net for anything diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index bac46f12b..ebb8de251 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -15,6 +15,7 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/tests" "github.com/navidrome/navidrome/utils/cache" @@ -115,6 +116,29 @@ func findQueued(q *tests.MockArtworkQueueRepo, kind, id string) *model.ArtworkQu return nil } +// visibilityPlaylistDS models playlist_repository's userFilter: a private playlist is only +// visible when the ctx carries an admin, so headless work must wrap ctx with one first. +type visibilityPlaylistDS struct { + *tests.MockDataStore + private model.Playlist + tracks model.PlaylistTrackRepository +} + +func (v *visibilityPlaylistDS) Playlist(ctx context.Context) model.PlaylistRepository { + repo := tests.CreateMockPlaylistRepo() + repo.TracksRepo = v.tracks + if u, ok := request.UserFrom(ctx); ok && u.IsAdmin { + repo.SetData(model.Playlists{v.private}) + } + return repo +} + +func adminUserRepo() *tests.MockedUserRepo { + repo := tests.CreateMockUserRepo() + Expect(repo.Put(&model.User{ID: "admin", UserName: "admin", IsAdmin: true})).To(Succeed()) + return repo +} + var _ = Describe("Worker", func() { var ( ctx context.Context diff --git a/model/artwork.go b/model/artwork.go index 578bdf144..d4413ec9c 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -103,7 +103,8 @@ type ArtworkQueueItem struct { // Queue priorities: higher drains first. const ( ArtworkPriorityRecheck = 0 - // ArtworkPriorityBackfill is never enqueued; the CLI keeps it to name and cancel rows still on it. + // ArtworkPriorityBackfill sits between the hourly sweep and scan-driven work. Nothing enqueues + // it today; it stays named so a row still carrying it can be reported and cancelled. ArtworkPriorityBackfill = 10 ArtworkPriorityScan = 50 ArtworkPriorityBump = 100 @@ -160,8 +161,6 @@ type ArtworkQueueRepository interface { // CountQueued reports the pending rows matching the kinds and priorities, grouped by both; // an empty filter means every one. CountQueued(kinds []Kind, priorities []int) ([]ArtworkQueueStat, error) - // CountAbsent reports how many states of a kind resolved to no image. - CountAbsent(kind Kind) (int64, error) // PurgeDangling removes queue rows whose entity no longer exists. PurgeDangling() (int64, error) // PurgeQueued removes pending rows matching the kinds and priorities; an empty filter means every one. diff --git a/model/radio.go b/model/radio.go index 466ff48b0..013a24beb 100644 --- a/model/radio.go +++ b/model/radio.go @@ -35,6 +35,5 @@ type RadioRepository interface { Exists(id string) (bool, error) Get(id string) (*Radio, error) GetAll(options ...QueryOptions) (Radios, error) - GetAllIDs(options ...QueryOptions) ([]string, error) Put(u *Radio, colsToUpdate ...string) error } diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index 6ea5b5fa3..3ad368772 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -223,11 +223,4 @@ func (r *artworkQueueRepository) Count() (int64, error) { return res.Count, err } -func (r *artworkQueueRepository) CountAbsent(kind model.Kind) (int64, error) { - var res struct{ Count int64 } - err := r.queryOne(Select("count(*) as count").From(itemArtworkTable). - Where(Eq{"item_kind": kind.Prefix(), "hash": ""}), &res) - return res.Count, err -} - var _ model.ArtworkQueueRepository = (*artworkQueueRepository)(nil) diff --git a/persistence/artwork_queue_repository_test.go b/persistence/artwork_queue_repository_test.go index 830de4e5e..a4a1ea76f 100644 --- a/persistence/artwork_queue_repository_test.go +++ b/persistence/artwork_queue_repository_test.go @@ -404,24 +404,6 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(repo.CountQueued(nil, nil)).To(BeEmpty()) }) - It("counts absent states", func() { - awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) - old := time.Now().Add(-48 * time.Hour) - for _, ia := range []model.ItemArtwork{ - {ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}, - {ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()}, - {ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old}, - {ItemKind: "al", ItemID: "stale2", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}, - } { - Expect(awRepo.PutItemArtwork(&ia)).To(Succeed()) - } - - Expect(repo.CountAbsent(model.KindArtistArtwork)).To(Equal(int64(2))) - }) - - It("reports a kind with no absent state as zero, not as an error", func() { - Expect(repo.CountAbsent(model.KindRadioArtwork)).To(Equal(int64(0))) - }) }) Describe("PurgeQueued", func() { diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index b73487e40..816faf134 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -79,14 +79,6 @@ func (r *radioRepository) hydrateArtwork(radios model.Radios) { func(rd *model.Radio) (string, *model.ItemImage) { return rd.ID, &rd.ItemImage }) } -// GetAllIDs returns just the radio IDs. Used by bulk enumeration (artwork backfill). -func (r *radioRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) { - sel := r.newSelect(options...).Columns("id") - ids := []string{} - err := r.queryAllSlice(sel, &ids) - return ids, err -} - func (r *radioRepository) Put(radio *model.Radio, colsToUpdate ...string) error { if !r.isPermitted() { return rest.ErrPermissionDenied diff --git a/persistence/radio_repository_test.go b/persistence/radio_repository_test.go index e2564455d..65b800548 100644 --- a/persistence/radio_repository_test.go +++ b/persistence/radio_repository_test.go @@ -7,7 +7,6 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -79,17 +78,6 @@ var _ = Describe("RadioRepository", func() { }) }) - Describe("GetAllIDs", func() { - It("returns the same id set as GetAll", func() { - want, err := repo.GetAll() - Expect(err).To(BeNil()) - Expect(want).ToNot(BeEmpty()) - ids, err := repo.GetAllIDs() - Expect(err).To(BeNil()) - Expect(ids).To(ConsistOf(slice.Map(want, func(r model.Radio) string { return r.ID }))) - }) - }) - Describe("Put", func() { It("successfully updates item", func() { err := repo.Put(&model.Radio{ diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index 2e954053c..a1b7b3e3d 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -226,22 +226,6 @@ func (m *MockArtworkQueueRepo) CountQueued(kinds []model.Kind, priorities []int) return res, nil } -// CountAbsent mirrors the SQL predicate: an absent state is one with no hash. -func (m *MockArtworkQueueRepo) CountAbsent(kind model.Kind) (int64, error) { - m.mu.Lock() - defer m.mu.Unlock() - if m.Err != nil || m.ItemArtworkSource == nil { - return 0, m.Err - } - var total int64 - for _, ia := range m.ItemArtworkSource.ItemData { - if ia.ItemKind == kind.Prefix() && ia.Hash == "" { - total++ - } - } - return total, nil -} - func (m *MockArtworkQueueRepo) EnqueuePreservingBackoff(items ...model.ArtworkQueueItem) error { m.mu.Lock() defer m.mu.Unlock() diff --git a/tests/mock_radio_repository.go b/tests/mock_radio_repository.go index 2baeadc5c..20f81ec45 100644 --- a/tests/mock_radio_repository.go +++ b/tests/mock_radio_repository.go @@ -5,7 +5,6 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" - "github.com/navidrome/navidrome/utils/slice" ) type MockedRadioRepo struct { @@ -74,14 +73,6 @@ func (m *MockedRadioRepo) GetAll(qo ...model.QueryOptions) (model.Radios, error) return m.All, nil } -func (m *MockedRadioRepo) GetAllIDs(qo ...model.QueryOptions) ([]string, error) { - all, err := m.GetAll(qo...) - if err != nil { - return nil, err - } - return slice.Map(all, func(r model.Radio) string { return r.ID }), nil -} - func (m *MockedRadioRepo) Put(radio *model.Radio, _ ...string) error { if m.Err { return errors.New("error")