diff --git a/cmd/artwork.go b/cmd/artwork.go index 5cd4fc146..55aebd1bf 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -42,7 +42,7 @@ func init() { "stored trace of the last resolution; also initializes plugin agents, which may open "+ "external connections") artworkReprocessCmd.Flags().StringSliceVar(&artworkKinds, "kind", nil, - "kinds to reprocess ("+kindPrefixes(artwork.RecheckKinds)+"); repeatable") + "kinds to reprocess ("+kindPrefixes(artwork.ReprocessKinds)+"); repeatable") artworkReprocessCmd.Flags().StringSliceVar(&artworkSources, "source", nil, "only items currently resolved from these sources (e.g. folder, external:deezer, absent)") artworkReprocessCmd.Flags().BoolVar(&artworkAll, "all", false, "reprocess every kind") @@ -147,8 +147,8 @@ type sourceCount struct { } type absentCount struct { - kind model.Kind - model.ArtworkAbsentStat + kind model.Kind + count int64 } type statusReport struct { @@ -170,16 +170,6 @@ func queueTotal(stats []model.ArtworkQueueStat) int64 { return n } -func (r statusReport) backfillQueued() int64 { - var n int64 - for _, s := range r.queue { - if s.Priority == model.ArtworkPriorityBackfill { - n += s.Count - } - } - return n -} - func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error) { q := ds.ArtworkQueue(ctx) var rep statusReport @@ -188,8 +178,7 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error return rep, fmt.Errorf("breaking the artwork queue down by kind: %w", err) } - cutoff := time.Now().Add(-artwork.StaleAbsentAge) - for _, k := range artwork.RecheckKinds { + for _, k := range artwork.ReprocessKinds { sources, err := q.SourcesInUse(k) if err != nil { return rep, fmt.Errorf("listing the sources in use by %s artwork: %w", k, err) @@ -202,11 +191,11 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error } rep.sources = append(rep.sources, sourceCount{kind: k, source: s, count: n}) } - stat, err := q.CountAbsent(k, cutoff) + 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, ArtworkAbsentStat: stat}) + rep.absent = append(rep.absent, absentCount{kind: k, count: n}) } rep.current, rep.inputs = artwork.ConfigFingerprint(), artwork.FingerprintInputs() @@ -234,19 +223,18 @@ func formatStatus(rep statusReport) string { } fmt.Fprintln(w, "\nAbsent (resolved, no image found)") - fmt.Fprintln(w, " KIND\tABSENT\tDUE FOR RECHECK") + fmt.Fprintln(w, " KIND\tABSENT") for _, a := range rep.absent { - fmt.Fprintf(w, " %s\t%d\t%d\n", a.kind, a.Total, a.Stale) + fmt.Fprintf(w, " %s\t%d\n", a.kind, a.count) } - fmt.Fprintf(w, " (eligible once the last attempt is older than %gh; re-queued %d per kind per hour, oldest first)\n", - artwork.StaleAbsentAge.Hours(), artwork.StaleAbsentRecheckBatch) + fmt.Fprintln(w, " (never retried on their own, not even by a backfill; run 'artwork reprocess --source absent')") - fmt.Fprintln(w, "\nBackfill") - fmt.Fprintf(w, " State:\t%s\n", backfillState(rep)) + fmt.Fprintln(w, "\nConfig") + fmt.Fprintf(w, " State:\t%s\n", configState(rep)) fmt.Fprintf(w, " Stored fingerprint:\t%s\n", cmp.Or(rep.stored, "(none)")) fmt.Fprintf(w, " Current fingerprint:\t%s\n", rep.current) if len(rep.inputs) > 0 { - fmt.Fprintln(w, " Fingerprint inputs (changing any of these re-resolves the whole library):") + fmt.Fprintln(w, " Fingerprint inputs (changing any of these makes the stored artwork stale):") for _, in := range rep.inputs { fmt.Fprintf(w, " %s:\t%s\n", in.Name, in.Value) } @@ -256,18 +244,10 @@ func formatStatus(rep statusReport) string { return sb.String() } -// backfillState leads with the queued backlog: by the time anyone runs this, backfill has usually -// already stored the new fingerprint, and "up to date" would bury the flood it is still working through. -func backfillState(rep statusReport) string { - pending := "fingerprint changed — every artist, album, playlist and radio will be re-enqueued on the next startup" - if n := rep.backfillQueued(); n > 0 { - if rep.stored != rep.current { - return fmt.Sprintf("backfill running: %d items queued, and %s", n, pending) - } - return fmt.Sprintf("backfill running: %d items queued (fingerprint up to date)", n) - } +func configState(rep statusReport) string { if rep.stored != rep.current { - return pending + return "fingerprint changed — stored artwork keeps the old resolution; " + + "run 'artwork reprocess --all' to apply it" } return "up to date" } @@ -342,8 +322,10 @@ 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, confirmUnlessYes(artworkYes, os.Stdin, "re-resolve"), os.Stdout); err != nil { + artworkDryRun, full, confirmUnlessYes(artworkYes, os.Stdin, "re-resolve"), os.Stdout); err != nil { log.Fatal(ctx, err) } } @@ -351,13 +333,13 @@ func runReprocess(ctx context.Context) { func selectedKinds(kinds, sources []string, all bool) ([]model.Kind, error) { // A source filter on its own is already a complete selection, so it does not also need a kind. if all || (len(kinds) == 0 && len(sources) > 0) { - return artwork.RecheckKinds, nil + return artwork.ReprocessKinds, nil } if len(kinds) == 0 { return nil, fmt.Errorf("no selector given: pass --kind, --source or --all") } return parseAll(kinds, func(s string) (model.Kind, error) { - return parseArtworkKind(s, artwork.RecheckKinds) + return parseArtworkKind(s, artwork.ReprocessKinds) }) } @@ -447,7 +429,7 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error { return nil } var inUse []string - for _, k := range artwork.RecheckKinds { + for _, k := range artwork.ReprocessKinds { found, err := q.SourcesInUse(k) if err != nil { return fmt.Errorf("listing the sources in use by %s artwork: %w", k, err) @@ -472,7 +454,7 @@ 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 bool, confirm confirmFunc, out io.Writer) error { + imageAgents artwork.ImageAgentCount, dryRun, full bool, confirm confirmFunc, out io.Writer) error { q := ds.ArtworkQueue(ctx) if err := validateSources(q, sources); err != nil { return err @@ -519,6 +501,11 @@ 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 } @@ -546,7 +533,7 @@ func cancelSelection(kinds, priorities []string, all bool) ([]model.Kind, []int, if len(kinds) == 0 && len(priorities) == 0 { return nil, nil, fmt.Errorf("no selector given: pass --kind, --priority or --all") } - // RefreshableKinds, not RecheckKinds: media files are queued, so --kind must reach them. + // RefreshableKinds, not ReprocessKinds: media files are queued, so --kind must reach them. outKinds, err := parseAll(kinds, func(s string) (model.Kind, error) { return parseArtworkKind(s, artwork.RefreshableKinds) }) diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index a7220d2d5..8a3d92825 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -3,7 +3,6 @@ package cmd import ( "context" "errors" - "fmt" "io" "strings" "time" @@ -20,20 +19,20 @@ import ( var _ = Describe("parseArtworkKind", func() { It("accepts a supported kind", func() { - k, err := parseArtworkKind("ar", artwork.RecheckKinds) + k, err := parseArtworkKind("ar", artwork.ReprocessKinds) Expect(err).ToNot(HaveOccurred()) Expect(k).To(Equal(model.KindArtistArtwork)) }) It("rejects an unknown kind and lists the valid ones", func() { - _, err := parseArtworkKind("zz", artwork.RecheckKinds) + _, err := parseArtworkKind("zz", artwork.ReprocessKinds) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("ar")) Expect(err.Error()).To(ContainSubstring("al")) }) It("rejects a known kind the command does not accept", func() { - _, err := parseArtworkKind("mf", artwork.RecheckKinds) + _, err := parseArtworkKind("mf", artwork.ReprocessKinds) Expect(err).To(HaveOccurred()) }) @@ -442,13 +441,13 @@ var _ = Describe("artwork reprocess selection", func() { It("returns every kind for --all", func() { ks, err := selectedKinds(nil, nil, true) Expect(err).ToNot(HaveOccurred()) - Expect(ks).To(ConsistOf(artwork.RecheckKinds)) + Expect(ks).To(ConsistOf(artwork.ReprocessKinds)) }) It("returns every kind for a source filter given without a kind", func() { ks, err := selectedKinds(nil, []string{"folder"}, false) Expect(err).ToNot(HaveOccurred()) - Expect(ks).To(ConsistOf(artwork.RecheckKinds), "--source alone is already a complete selection") + Expect(ks).To(ConsistOf(artwork.ReprocessKinds), "--source alone is already a complete selection") }) It("returns only the named kinds", func() { @@ -557,6 +556,10 @@ 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 @@ -590,7 +593,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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, true, false, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("external:deezer")) Expect(out.String()).To(ContainSubstring("artist")) @@ -601,14 +604,27 @@ var _ = Describe("reprocessArtwork", func() { }) It("queues nothing when the operator declines", func() { - Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, decline, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, 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) { + 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(ds.Property(ctx).Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(expected())) + }, + 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), + ) + It("queues the matching items at recheck priority, leaving their artwork state alone", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(2))) queued, err := queue.Get(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) @@ -623,7 +639,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("targets the absent state", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(1))) _, err := queue.Get(model.KindArtistArtwork, "ar-2", model.ImageTypePrimary) @@ -634,7 +650,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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, 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")) @@ -645,7 +661,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, + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, false, false, func(io.Writer, int64, int64) bool { Fail("must not prompt when there is nothing to queue") return true @@ -656,14 +672,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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, true, false, 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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, imageAgents, true, false, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("External lookups: ~2 estimated")) }) @@ -673,14 +689,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, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, 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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, true, false, 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. @@ -694,7 +710,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, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, true, false, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("External lookups: none")) }) @@ -706,7 +722,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, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(1))) Expect(out.String()).To(ContainSubstring("External lookups: ~1 estimated")) @@ -718,7 +734,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, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(artwork.PlaylistGridSamples*3)), "one playlist samples 4 albums, each walking all 3 album agents") @@ -730,14 +746,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, capture, &out)).To(Succeed()) + nil, imageAgents, false, 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, accept, &out) + err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, imageAgents, true, false, accept, &out) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("externa:deezer")) @@ -751,18 +767,18 @@ var _ = Describe("reprocessArtwork", func() { put(model.KindArtistArtwork, "ar-2", "folder") Expect(reprocessArtwork(ctx, ds, kinds, repositorySources([]string{absentSource}), - imageAgents, false, accept, &out)).To(Succeed(), + imageAgents, false, 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, accept, &out)).ToNot(Succeed(), "a typo must still be rejected") + imageAgents, true, false, 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, decline, &out)).To(Succeed()) + imageAgents, false, 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") @@ -792,14 +808,14 @@ var _ = Describe("collectStatus", func() { ImageType: model.ImageTypePrimary, Source: source, Hash: hash, AttemptedAt: attempted})).To(Succeed()) } put(model.KindArtistArtwork, "ar-1", "external:deezer", "h1", time.Now()) - put(model.KindArtistArtwork, "ar-2", "", "", time.Now().Add(-artwork.StaleAbsentAge-time.Hour)) + put(model.KindArtistArtwork, "ar-2", "", "", time.Now().Add(-24*time.Hour)) put(model.KindArtistArtwork, "ar-3", "", "", time.Now()) put(model.KindAlbumArtwork, "al-1", "folder", "h2", time.Now()) Expect(queue.Enqueue(model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar-9", ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityBackfill})).To(Succeed()) }) - It("reports the queue, the source distribution and the absent ages", func() { + It("reports the queue, the source distribution and the absent totals", func() { rep, err := collectStatus(ctx, ds) Expect(err).ToNot(HaveOccurred()) @@ -810,8 +826,7 @@ var _ = Describe("collectStatus", func() { sourceCount{kind: model.KindArtistArtwork, source: "", count: 2}, sourceCount{kind: model.KindAlbumArtwork, source: "folder", count: 1}, )) - Expect(rep.absent).To(ContainElement(absentCount{kind: model.KindArtistArtwork, - ArtworkAbsentStat: model.ArtworkAbsentStat{Total: 2, Stale: 1}})) + Expect(rep.absent).To(ContainElement(absentCount{kind: model.KindArtistArtwork, count: 2})) }) It("compares the stored fingerprint against the current one", func() { @@ -845,7 +860,7 @@ var _ = Describe("formatStatus", func() { {kind: model.KindArtistArtwork, source: "", count: 2}, }, absent: []absentCount{ - {kind: model.KindArtistArtwork, ArtworkAbsentStat: model.ArtworkAbsentStat{Total: 2, Stale: 1}}, + {kind: model.KindArtistArtwork, count: 2}, }, inputs: []artwork.FingerprintInput{{Name: "Agents", Value: "deezer,lastfm"}}, stored: "abc123", @@ -878,50 +893,33 @@ var _ = Describe("formatStatus", func() { Expect(sources).To(MatchRegexp(`artist\s+absent\s+2`)) }) - It("prints the absent total and how many are due for recheck", func() { + It("prints the absent total", func() { absent := block(formatStatus(rep), "Absent (resolved, no image found)") - Expect(absent).To(MatchRegexp(`artist\s+2\s+1`)) + Expect(absent).To(MatchRegexp(`artist\s+2`)) }) - It("states the recheck window and the drip rate the absent counts are bucketed against", func() { - Expect(formatStatus(rep)).To(ContainSubstring(fmt.Sprintf("%gh", artwork.StaleAbsentAge.Hours()))) - Expect(formatStatus(rep)).To(ContainSubstring("100 per kind per hour")) + It("says absent states are never retried on their own, and names the command that does", func() { + Expect(formatStatus(rep)).To(ContainSubstring("never retried on their own")) + Expect(formatStatus(rep)).To(ContainSubstring("artwork reprocess --source absent")) }) - It("leads with the queued backlog, which is the finding, not with the fingerprint verdict", func() { - out := block(formatStatus(rep), "Backfill") - Expect(out).To(MatchRegexp(`State:\s+backfill running: 2 items queued`), - "an operator scanning for trouble must not read 'up to date' while 2 items churn") - Expect(out).To(ContainSubstring("fingerprint up to date")) - }) - - It("keeps the re-enqueue warning while a backfill is already running", func() { - rep.stored = "older" - - out := block(formatStatus(rep), "Backfill") - Expect(out).To(MatchRegexp(`State:\s+backfill running: 2 items queued`)) - Expect(out).To(ContainSubstring("re-enqueued"), - "the stored fingerprint is still stale, so a second full re-enqueue is pending on top of this one") - }) - - It("reports up to date only once the backfill has drained", func() { - rep.queue = []model.ArtworkQueueStat{{ItemKind: "al", Priority: model.ArtworkPriorityScan, Count: 1}} - - Expect(block(formatStatus(rep), "Backfill")).To(MatchRegexp(`State:\s+up to date`)) + It("reports a matching fingerprint as up to date, whatever else is queued", func() { + Expect(block(formatStatus(rep), "Config")).To(MatchRegexp(`State:\s+up to date`)) }) It("echoes the config inputs a fingerprint change would have come from", func() { - out := block(formatStatus(rep), "Backfill") + out := block(formatStatus(rep), "Config") Expect(out).To(MatchRegexp(`Agents:\s+deezer,lastfm`)) Expect(out).To(ContainSubstring("abc123"), "the fingerprint values themselves must be printed") }) - It("reports a changed fingerprint as a pending re-resolve of everything", func() { + It("reports a changed fingerprint as stale artwork, and names the command that applies it", func() { rep.stored = "older" rep.queue = nil out := formatStatus(rep) Expect(out).To(ContainSubstring("fingerprint changed")) + Expect(out).To(ContainSubstring("artwork reprocess --all")) Expect(out).ToNot(ContainSubstring("up to date")) }) @@ -1015,7 +1013,7 @@ var _ = Describe("needsImageAgents", func() { It("is false once the chains no longer reach an agent", func() { conf.Server.CoverArtPriority = "cover.*" conf.Server.ArtistArtPriority = "artist.*" - Expect(needsImageAgents(artwork.RecheckKinds)).To(BeFalse()) + Expect(needsImageAgents(artwork.ReprocessKinds)).To(BeFalse()) }) }) diff --git a/cmd/root.go b/cmd/root.go index ff1641bd3..a7876401b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -357,21 +357,18 @@ func startArtworkWorker(ctx context.Context, worker *artwork.Worker) func() erro } } -// scheduleArtworkHousekeeping runs the startup fingerprint backfill and registers the -// recurring stale-absent recheck and prune jobs. +// scheduleArtworkHousekeeping registers the recurring missing-state and prune jobs, and +// reports an artwork config change without acting on it. func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) func() error { return func() error { schedulerInstance := scheduler.GetInstance() - if _, err := schedulerInstance.Add(consts.ArtworkStaleAbsentRecheckSchedule, func() { - if err := worker.EnqueueStaleAbsentAll(ctx); err != nil { - log.Error(ctx, "Error enqueueing stale artwork rechecks", err) - } + if _, err := schedulerInstance.Add(consts.ArtworkEnqueueMissingSchedule, func() { if err := worker.EnqueueMissingAll(ctx); err != nil { log.Error(ctx, "Error enqueueing missing artwork rechecks", err) } }); err != nil { - log.Error(ctx, "Error scheduling artwork stale-absent recheck", err) + log.Error(ctx, "Error scheduling artwork missing-state recheck", err) } if _, err := schedulerInstance.Add(consts.ArtworkPruneSchedule, func() { @@ -388,23 +385,8 @@ func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) fu log.Error(ctx, "Error enqueueing missing artwork rechecks", err) } - backfilled, err := worker.Backfill(ctx) - if err != nil { - log.Error(ctx, "Error running artwork backfill", err) - return nil - } - if !backfilled { - return nil - } - log.Info(ctx, "Artwork backfill enqueued, scheduling a follow-up prune") - timer := time.NewTimer(consts.ArtworkPostBackfillPruneDelay) - defer timer.Stop() - select { - case <-timer.C: - if err := worker.RunPrune(ctx); err != nil { - log.Error(ctx, "Error running post-backfill artwork prune", err) - } - case <-ctx.Done(): + if err := worker.CheckConfig(ctx); err != nil { + log.Error(ctx, "Error checking the artwork config fingerprint", err) } return nil } diff --git a/consts/consts.go b/consts/consts.go index 2934cd968..0f950ef10 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -24,8 +24,8 @@ const ( LastDBAnalyzeAttemptAtKey = "LastDBAnalyzeAttemptAt" DBAnalyzePendingKey = "DBAnalyzePending" DBAnalyzeFailureCountKey = "DBAnalyzeFailureCount" - // ArtConfFingerprintPropertyKey is the model.PropertyRepository key Backfill compares against - // to detect artwork-affecting config changes across restarts. + // ArtConfFingerprintPropertyKey is the model.PropertyRepository key the artwork config check + // compares against to detect artwork-affecting config changes across restarts. ArtConfFingerprintPropertyKey = "ArtConfFingerprint" UIAuthorizationHeader = "X-ND-Authorization" @@ -39,9 +39,8 @@ const ( DBAnalyzeCheckSchedule = "@every 30m" DBAnalyzeMaxAge = 24 * time.Hour - ArtworkStaleAbsentRecheckSchedule = "@every 1h" - ArtworkPruneSchedule = "@daily" - ArtworkPostBackfillPruneDelay = 10 * time.Minute + ArtworkEnqueueMissingSchedule = "@every 1h" + ArtworkPruneSchedule = "@daily" // DefaultEncryptionKey This is the encryption key used if none is specified in the `PasswordEncryptionKey` option // Never ever change this! Or it will break all Navidrome installations that don't set the config option diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index e8458a0f9..663d06d25 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -118,10 +118,6 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa } } -// requestRecheckAge throttles view-triggered rechecks so reopening a genuinely-absent page can't -// hammer external services; below StaleAbsentAge to catch younger absences. -const requestRecheckAge = time.Hour - func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) { ia, err := s.ds.Artwork(ctx).GetItemArtwork(artID.Kind, artID.ID, model.ImageTypePrimary) switch { @@ -130,10 +126,7 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i case err != nil: return nil, err case ia.Hash == "": - // Inserts an immediately-eligible recheck for a settled absent row. - if time.Since(ia.AttemptedAt) > requestRecheckAge { - s.enqueue(ctx, artID, model.ArtworkPriorityBump) - } + // Settled absent: only an explicit reprocess or refresh retries it. return nil, ErrUnavailable default: return s.serveHash(ctx, artID, ia, size, square) diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 7de5475d6..3750805f2 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -204,28 +204,19 @@ var _ = Describe("Artwork", func() { Expect(err).To(MatchError(ErrUnavailable)) }) - It("does not re-enqueue a recently-attempted absent state", func() { - Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ - ItemKind: "al", ItemID: "al4", AttemptedAt: time.Now(), - })).To(Succeed()) + DescribeTable("never re-enqueues an absent state on view, however old", + func(id string, attemptedAt time.Time) { + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: "al", ItemID: id, AttemptedAt: attemptedAt, + })).To(Succeed()) - _, err := svc.Get(ctx, model.MustParseArtworkID("al-al4"), 0, false) - Expect(err).To(MatchError(ErrUnavailable)) - Expect(queueRepo.Data).To(BeEmpty()) - }) - - It("promotes a stale absent state at Bump priority on view", func() { - Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ - ItemKind: "al", ItemID: "al4b", AttemptedAt: time.Now().Add(-2 * requestRecheckAge), - })).To(Succeed()) - - _, err := svc.Get(ctx, model.MustParseArtworkID("al-al4b"), 0, false) - Expect(err).To(MatchError(ErrUnavailable)) - Expect(queueRepo.Data[primaryKey("al", "al4b")].Priority).To(Equal(model.ArtworkPriorityBump)) - ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al4b", model.ImageTypePrimary) - Expect(err).ToNot(HaveOccurred()) - Expect(ia.Hash).To(BeEmpty()) - }) + _, err := svc.Get(ctx, model.MustParseArtworkID("al-"+id), 0, false) + Expect(err).To(MatchError(ErrUnavailable)) + Expect(queueRepo.Data).To(BeEmpty()) + }, + Entry("just attempted", "al4", time.Now()), + Entry("attempted a year ago", "al4b", time.Now().Add(-365*24*time.Hour)), + ) }) Describe("provisional read-through", func() { diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index a3330d7cf..cb714ac43 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -6,26 +6,18 @@ import ( "slices" "strconv" "strings" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/slice" "github.com/zeebo/xxh3" ) -// StaleAbsentAge is how long an absent state is trusted before a recheck retries it. -const StaleAbsentAge = 30 * 24 * time.Hour - -// StaleAbsentRecheckBatch caps how many absent states each hourly tick re-queues per kind, -// oldest first, so external agents see a flat drip instead of a daily burst. -const StaleAbsentRecheckBatch = 100 - -// RecheckKinds omits media files: they resolve embedded-only, at scan or on view. -var RecheckKinds = []model.Kind{ +// ReprocessKinds omits media files: they resolve embedded-only, at scan or on view. Artists lead +// so bulk enqueues give the most external-dependent kind a queue headstart. +var ReprocessKinds = []model.Kind{ model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, } @@ -34,14 +26,15 @@ var RecheckKinds = []model.Kind{ func KeepsState(kind model.Kind) bool { return kind != model.KindDiscArtwork } // RefreshableKinds is every kind Refresh can clear and re-queue, so it holds exactly the kinds -// KeepsState admits. Media files are absent from RecheckKinds but belong here: the worker -// resolves them, it just never revisits them on its own. -var RefreshableKinds = append(slices.Clone(RecheckKinds), model.KindMediaFileArtwork) +// KeepsState admits. Media files are absent from ReprocessKinds but belong here: the worker +// resolves them, it just never enumerates them in bulk. +var RefreshableKinds = append(slices.Clone(ReprocessKinds), model.KindMediaFileArtwork) -// hasRecheckPath reports whether a periodic job will revisit this kind, making an absent settle recoverable. -func hasRecheckPath(prefix string) bool { +// 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. +func settlesAbsentOnGiveUp(prefix string) bool { kind, ok := model.ParseKind(prefix) - return ok && slices.Contains(RecheckKinds, kind) + return ok && slices.Contains(ReprocessKinds, kind) } // artworkEpoch invalidates all resolution state when bumped; bump it whenever resolution semantics change. @@ -72,93 +65,36 @@ func ConfigFingerprint() string { return fmt.Sprintf("%016x", xxh3.Hash([]byte(raw))) } -// backfillSummary is what a backfill enqueued. MaxExternalLookups is an upper estimate for one -// attempt per item, not a bound: a local hit ends the walk, and a retry asks the agents again. -type backfillSummary struct { - Ran bool - PerKind map[string]int64 - Items int64 - MaxExternalLookups int64 -} - -// backfill enqueues artwork resolution for every entity when the config fingerprint changed. -func backfill(ctx context.Context, ds model.DataStore, agentCount func() ImageAgentCount) (backfillSummary, error) { - start := time.Now() - ctx = auth.WithAdminUser(ctx, ds) +// CheckConfigFingerprint 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 { current := ConfigFingerprint() - props := ds.Property(ctx) - stored, err := props.DefaultGet(consts.ArtConfFingerprintPropertyKey, "") + stored, err := ds.Property(ctx).DefaultGet(consts.ArtConfFingerprintPropertyKey, "") if err != nil { - return backfillSummary{}, err + return err } - if stored == current { - return backfillSummary{}, nil - } - - // Artists first: few entities, most external-dependent, so they get a queue headstart. - kinds := []struct { - kind model.Kind - fetch func() ([]string, error) - }{ - {model.KindArtistArtwork, func() ([]string, error) { return ds.Artist(ctx).GetAllIDs() }}, - {model.KindAlbumArtwork, func() ([]string, error) { return ds.Album(ctx).GetAllIDs() }}, - {model.KindPlaylistArtwork, func() ([]string, error) { return ds.Playlist(ctx).GetAllIDs() }}, - {model.KindRadioArtwork, func() ([]string, error) { return ds.Radio(ctx).GetAllIDs() }}, - } - // Counted here, not by the caller: building the agent list constructs every enabled agent, and - // an unchanged fingerprint returns above without ever needing the number. - agents := agentCount() - summary := backfillSummary{Ran: true, PerKind: map[string]int64{}} - for _, k := range kinds { - ids, err := k.fetch() - if err != nil { - return backfillSummary{}, err - } - if err := enqueueBackfillKind(ctx, ds, k.kind, ids); err != nil { - return backfillSummary{}, err - } - n := int64(len(ids)) - summary.PerKind[k.kind.Prefix()] = n - summary.Items += n - summary.MaxExternalLookups += n * ExternalLookupsPerItem(k.kind, agents) - } - - if err := props.Put(consts.ArtConfFingerprintPropertyKey, current); err != nil { - return backfillSummary{}, err - } - log.Info(ctx, "Artwork: Config fingerprint changed, backfill enqueued", "items", summary.Items, - "byKind", summary.PerKind, "maxExternalLookups", summary.MaxExternalLookups, - "elapsed", time.Since(start)) - return summary, nil -} - -func enqueueBackfillKind(ctx context.Context, ds model.DataStore, kind model.Kind, ids []string) error { - if len(ids) == 0 { - return nil - } - items := slice.Map(ids, func(id string) model.ArtworkQueueItem { - return model.ArtworkQueueItem{ - ItemKind: kind.Prefix(), ItemID: id, ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityBackfill, - } - }) - return ds.ArtworkQueue(ctx).Enqueue(items...) -} - -func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error { - cutoff := time.Now().Add(-StaleAbsentAge) - queue := ds.ArtworkQueue(ctx) - for _, kind := range RecheckKinds { - if _, err := queue.EnqueueStaleAbsent(kind, cutoff, StaleAbsentRecheckBatch); err != nil { - return err - } + switch stored { + case current: + case "": + // A library with no stored fingerprint has nothing resolved under an older one to warn about. + return MarkConfigApplied(ctx, ds) + default: + log.Warn(ctx, "Artwork: Config changed since the last full reprocess. Stored artwork keeps "+ + "the old resolution; run 'navidrome artwork reprocess --all' to apply the change", + "stored", stored, "current", current, "inputs", FingerprintInputs()) } return nil } +// MarkConfigApplied records the current fingerprint as the one the library is resolved under. +func MarkConfigApplied(ctx context.Context, ds model.DataStore) error { + return ds.Property(ctx).Put(consts.ArtConfFingerprintPropertyKey, ConfigFingerprint()) +} + // enqueueMissingAll is the safety net for entities a scan never enqueued (added between scans, or scanner off). func enqueueMissingAll(ctx context.Context, ds model.DataStore) error { queue := ds.ArtworkQueue(ctx) - for _, kind := range RecheckKinds { + for _, kind := range ReprocessKinds { if _, err := queue.EnqueueAllMissing(kind, model.ArtworkPriorityRecheck); err != nil { return err } diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go index c9809203a..594613dba 100644 --- a/core/artwork/housekeeping_test.go +++ b/core/artwork/housekeeping_test.go @@ -2,7 +2,6 @@ package artwork import ( "context" - "fmt" "slices" "time" @@ -39,20 +38,16 @@ func adminUserRepo() *tests.MockedUserRepo { return repo } -func noAgents() ImageAgentCount { return ImageAgentCount{} } - -// orderTrackingQueueRepo records the item kind of each Enqueue call, so tests can -// assert phase ordering (artists-first) that same-priority timestamps can't guarantee. +// 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 { *tests.MockArtworkQueueRepo callKinds []string } -func (o *orderTrackingQueueRepo) Enqueue(items ...model.ArtworkQueueItem) error { - if len(items) > 0 { - o.callKinds = append(o.callKinds, items[0].ItemKind) - } - return o.MockArtworkQueueRepo.Enqueue(items...) +func (o *orderTrackingQueueRepo) EnqueueAllMissing(kind model.Kind, priority int) (int64, error) { + o.callKinds = append(o.callKinds, kind.Prefix()) + return o.MockArtworkQueueRepo.EnqueueAllMissing(kind, priority) } var _ = Describe("RefreshableKinds", func() { @@ -89,24 +84,6 @@ var _ = Describe("Housekeeping", func() { ds = &tests.MockDataStore{MockedArtworkQueue: queueRepo, MockedProperty: propRepo} }) - seedEntities := func() { - artistRepo := tests.CreateMockArtistRepo() - artistRepo.SetData(model.Artists{{ID: "ar1"}, {ID: "ar2"}}) - ds.MockedArtist = artistRepo - - albumRepo := tests.CreateMockAlbumRepo() - albumRepo.SetData(model.Albums{{ID: "al1"}}) - ds.MockedAlbum = albumRepo - - playlistRepo := tests.CreateMockPlaylistRepo() - playlistRepo.SetData(model.Playlists{{ID: "pl1"}}) - ds.MockedPlaylist = playlistRepo - - radioRepo := tests.CreateMockedRadioRepo() - radioRepo.All = model.Radios{{ID: "ra1"}} - ds.MockedRadio = radioRepo - } - Describe("Fingerprint", func() { It("changes when a fingerprint-affecting config value changes", func() { f1 := ConfigFingerprint() @@ -161,141 +138,43 @@ var _ = Describe("Housekeeping", func() { }) }) - Describe("Backfill", func() { - It("enqueues nothing and returns false when the stored fingerprint matches", func() { - seedEntities() - Expect(propRepo.Put(consts.ArtConfFingerprintPropertyKey, ConfigFingerprint())).To(Succeed()) + Describe("CheckConfigFingerprint", func() { + It("records the current fingerprint when none was ever stored", func() { + Expect(CheckConfigFingerprint(ctx, ds)).To(Succeed()) - counted := false - s, err := backfill(ctx, ds, func() ImageAgentCount { - counted = true - return ImageAgentCount{Artist: 3, Album: 2} - }) - Expect(err).ToNot(HaveOccurred()) - Expect(s).To(Equal(backfillSummary{})) - Expect(counted).To(BeFalse(), "building the agent list constructs every agent; an unchanged fingerprint must not pay for it") - - count, err := queueRepo.Count() - Expect(err).ToNot(HaveOccurred()) - Expect(count).To(BeZero()) + Expect(propRepo.Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(ConfigFingerprint())) + Expect(queueRepo.Count()).To(BeZero()) }) - It("runs the backfill when no fingerprint was ever stored", func() { - seedEntities() - - s, err := backfill(ctx, ds, noAgents) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Ran).To(BeTrue()) - - count, err := queueRepo.Count() - Expect(err).ToNot(HaveOccurred()) - Expect(count).To(Equal(int64(5))) // 2 artists + 1 album + 1 playlist + 1 radio - - stored, err := propRepo.Get(consts.ArtConfFingerprintPropertyKey) - Expect(err).ToNot(HaveOccurred()) - Expect(stored).To(Equal(ConfigFingerprint())) - }) - - It("enqueues a private playlist by resolving it under an admin context", func() { - ds.MockedUser = adminUserRepo() - vds := &visibilityPlaylistDS{ - MockDataStore: ds, - private: model.Playlist{ID: "plPrivate", OwnerID: "admin"}, - tracks: &tests.MockPlaylistTrackRepo{}, - } - - s, err := backfill(ctx, vds, noAgents) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Ran).To(BeTrue()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "pl", "plPrivate")).ToNot(BeNil()) - }) - - It("enqueues artists before albums/playlists/radios, all at Backfill priority", func() { - seedEntities() + It("leaves a stale fingerprint stored, so the warning survives a restart", func() { Expect(propRepo.Put(consts.ArtConfFingerprintPropertyKey, "stale-fingerprint")).To(Succeed()) - s, err := backfill(ctx, ds, noAgents) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Ran).To(BeTrue()) + Expect(CheckConfigFingerprint(ctx, ds)).To(Succeed()) - Expect(queueRepo.callKinds).ToNot(BeEmpty()) - firstOther := slices.IndexFunc(queueRepo.callKinds, func(k string) bool { return k != "ar" }) - Expect(firstOther).ToNot(Equal(0), "artists must be the first Enqueue call") - if firstOther >= 0 { - Expect(queueRepo.callKinds[firstOther:]).ToNot(ContainElement("ar"), - "no artist Enqueue may follow another kind") - } - - for _, it := range queueRepo.Data { - Expect(it.Priority).To(Equal(model.ArtworkPriorityBackfill)) - Expect(it.ItemKind).To(BeElementOf("ar", "al", "pl", "ra")) - } + Expect(propRepo.Get(consts.ArtConfFingerprintPropertyKey)).To(Equal("stale-fingerprint")) }) - It("reports what it enqueued, per kind and as an external-lookup ceiling", func() { - conf.Server.ArtistArtPriority = "artist.*, external" - conf.Server.CoverArtPriority = "cover.*, external" - conf.Server.EnableM3UExternalAlbumArt = false - seedEntities() - - s, err := backfill(ctx, ds, func() ImageAgentCount { return ImageAgentCount{Artist: 3, Album: 2} }) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Ran).To(BeTrue()) - - Expect(s.PerKind).To(Equal(map[string]int64{"ar": 2, "al": 1, "pl": 1, "ra": 1})) - Expect(s.Items).To(Equal(int64(5))) - // 2 artists x 3 agents, 1 album x 2, 1 playlist grid x 2, and radios never fetch. - Expect(s.MaxExternalLookups).To(Equal(int64(6 + 2 + PlaylistGridSamples*2))) - }) + 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("EnqueueStaleAbsentAll", func() { - var artRepo *tests.MockArtworkRepo + Describe("MarkConfigApplied", func() { + It("overwrites a stale fingerprint with the current one", func() { + Expect(propRepo.Put(consts.ArtConfFingerprintPropertyKey, "stale-fingerprint")).To(Succeed()) - BeforeEach(func() { - artRepo = tests.CreateMockArtworkRepo() - ds.MockedArtwork = artRepo - queueRepo.ItemArtworkSource = artRepo - }) + Expect(MarkConfigApplied(ctx, ds)).To(Succeed()) - It("enqueues only absent entries older than the recheck window, across all kinds", func() { - old := time.Now().Add(-StaleAbsentAge - time.Hour) - recent := time.Now().Add(-StaleAbsentAge + time.Hour) - - artRepo.ItemData["ar-stale"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old} - artRepo.ItemData["al-stale"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old} - artRepo.ItemData["pl-stale"] = model.ItemArtwork{ItemKind: "pl", ItemID: "pl1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old} - artRepo.ItemData["ra-stale"] = model.ItemArtwork{ItemKind: "ra", ItemID: "ra1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old} - artRepo.ItemData["ar-recent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar2", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: recent} - artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al2", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: old} - - err := enqueueStaleAbsentAll(ctx, ds) - Expect(err).ToNot(HaveOccurred()) - - Expect(queueRepo.Data).To(HaveLen(4)) - for _, it := range queueRepo.Data { - Expect(it.Priority).To(Equal(model.ArtworkPriorityRecheck)) - } - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ar", "ar1")).ToNot(BeNil()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al1")).ToNot(BeNil()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "pl", "pl1")).ToNot(BeNil()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ra", "ra1")).ToNot(BeNil()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ar", "ar2")).To(BeNil()) - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al2")).To(BeNil()) - }) - - It("caps each tick at the recheck batch, oldest attempts first", func() { - for i := range StaleAbsentRecheckBatch + 1 { - id := fmt.Sprintf("ar%d", i) - artRepo.ItemData[id] = model.ItemArtwork{ItemKind: "ar", ItemID: id, ImageType: model.ImageTypePrimary, - Hash: "", AttemptedAt: time.Now().Add(-StaleAbsentAge - time.Duration(i+1)*time.Minute)} - } - - Expect(enqueueStaleAbsentAll(ctx, ds)).To(Succeed()) - - Expect(queueRepo.Data).To(HaveLen(StaleAbsentRecheckBatch)) - // ar0 has the newest attempted_at of the cohort, so it is the one left out. - Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ar", "ar0")).To(BeNil()) + Expect(propRepo.Get(consts.ArtConfFingerprintPropertyKey)).To(Equal(ConfigFingerprint())) }) }) @@ -330,6 +209,18 @@ var _ = Describe("Housekeeping", func() { Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al1")).To(BeNil()) Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ar", "ar1")).To(BeNil()) }) + + It("enqueues artists before every other kind", func() { + Expect(enqueueMissingAll(ctx, ds)).To(Succeed()) + + Expect(queueRepo.callKinds).ToNot(BeEmpty()) + firstOther := slices.IndexFunc(queueRepo.callKinds, func(k string) bool { return k != "ar" }) + Expect(firstOther).ToNot(Equal(0), "artists must be the first enqueue call") + if firstOther >= 0 { + Expect(queueRepo.callKinds[firstOther:]).ToNot(ContainElement("ar"), + "no artist enqueue may follow another kind") + } + }) }) }) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index bea478aa5..120d6ac31 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -23,8 +23,8 @@ import ( const ( workerPollInterval = 5 * time.Second backoffBase = 5 * time.Second - // giveUpAfter bounds the retry budget from enqueue; past it the item falls to the - // periodic stale-absent recheck. + // giveUpAfter bounds the retry budget from enqueue; past it the item settles and only an + // explicit reprocess retries it. giveUpAfter = 12 * time.Hour ) @@ -133,17 +133,9 @@ func (w *Worker) RunPrune(ctx context.Context) error { return prune(ctx, w.proc.ds, w.proc.store) } -// Backfill enqueues every entity for re-resolution when the artwork config fingerprint changed, -// artists first. It reports whether the backfill ran. -func (w *Worker) Backfill(ctx context.Context) (bool, error) { - s, err := backfill(ctx, w.proc.ds, func() ImageAgentCount { return NewImageAgentCount(w.agents) }) - return s.Ran, err -} - -// EnqueueStaleAbsentAll requeues known-absent entries older than StaleAbsentAge, at most -// StaleAbsentRecheckBatch per kind, oldest first. -func (w *Worker) EnqueueStaleAbsentAll(ctx context.Context) error { - return enqueueStaleAbsentAll(ctx, w.proc.ds) +// 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) } // EnqueueMissingAll requeues entities with no artwork state row: the safety net for anything @@ -265,10 +257,9 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc "budgetLeft", time.Until(item.EnqueuedAt.Add(giveUpAfter))) break } - // Absent is only recoverable where a periodic recheck revisits it, so other kinds keep - // no row; art already being served is kept, as exhaustion means unreachable, not removed. + // Art already being served is kept: exhaustion means unreachable, not removed. settled := "kept previous state" - if out == outcomeFailed && hasRecheckPath(item.ItemKind) && !w.hasResolvedArtwork(ctx, item) { + if out == outcomeFailed && settlesAbsentOnGiveUp(item.ItemKind) && !w.hasResolvedArtwork(ctx, item) { writeAbsent(ctx, w.proc.ds.Artwork(ctx), item) settled = "recorded absent" } diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index b0ef665fc..bac46f12b 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -441,9 +441,9 @@ var _ = Describe("Worker", func() { Expect(ia.Hash).To(Equal("cafebabe"), "recording the failure must not disturb the served art") }) - // Media files are excluded from RecheckKinds, so an absent row here would never be - // revisited: a transient read error would look permanent. - It("does not settle absent on exhaustion for a kind with no recheck path", func() { + // Only a view enqueues a media file, and an absent row is exactly what stops a view from + // doing so: a transient read error would look permanent. + It("does not settle absent on exhaustion for a media file", func() { conf.Server.EnableMediaFileCoverArt = true ds.MockedMediaFile = tests.CreateMockMediaFileRepo() ds.MockedMediaFile.(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ diff --git a/model/artwork.go b/model/artwork.go index 3c0df209b..578bdf144 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -88,11 +88,12 @@ func (i ItemArtworkInfo) Image() ItemImage { } type ArtworkQueueItem struct { - ItemKind string `structs:"item_kind"` - ItemID string `structs:"item_id"` - ImageType string `structs:"image_type"` - Priority int `structs:"priority"` - Attempts int `structs:"attempts"` + ItemKind string `structs:"item_kind"` + ItemID string `structs:"item_id"` + ImageType string `structs:"image_type"` + Priority int `structs:"priority"` + Attempts int `structs:"attempts"` + // RetryAt is the earliest time the drain may take this row, not when it will run. RetryAt time.Time `structs:"retry_at"` EnqueuedAt time.Time `structs:"enqueued_at"` // Trace is why the last attempt failed. Only Get reads it; the drain projects it away. @@ -101,7 +102,8 @@ type ArtworkQueueItem struct { // Queue priorities: higher drains first. const ( - ArtworkPriorityRecheck = 0 + ArtworkPriorityRecheck = 0 + // ArtworkPriorityBackfill is never enqueued; the CLI keeps it to name and cancel rows still on it. ArtworkPriorityBackfill = 10 ArtworkPriorityScan = 50 ArtworkPriorityBump = 100 @@ -134,9 +136,6 @@ type ArtworkQueueRepository interface { // EnqueuePreservingBackoff upserts like Enqueue but preserves an existing row's retry_at, so a // request-triggered read-through never resets a failed resolution's backoff. EnqueuePreservingBackoff(items ...ArtworkQueueItem) error - // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff, oldest - // first; limit caps the selection, so already-queued rows use up budget (backpressure when the drain stalls). - EnqueueStaleAbsent(kind Kind, attemptedBefore time.Time, limit int) (int64, error) // EnqueueAllMissing inserts queue rows for all entities with no item_artwork row, at the given priority. EnqueueAllMissing(kind Kind, priority int) (int64, error) // EnqueueIfMissing inserts only for items with no item_artwork row yet. @@ -161,9 +160,8 @@ 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 the absent states of a kind, and how many are past the given cutoff, - // eligible for EnqueueStaleAbsent (which drains them limit rows per call). - CountAbsent(kind Kind, attemptedBefore time.Time) (ArtworkAbsentStat, 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. @@ -175,8 +173,3 @@ type ArtworkQueueStat struct { Priority int Count int64 } - -type ArtworkAbsentStat struct { - Total int64 - Stale int64 -} diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index 1c0077fc7..6ea5b5fa3 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -56,14 +56,6 @@ func (r *artworkQueueRepository) EnqueuePreservingBackoff(items ...model.Artwork priority = MAX(priority, excluded.priority)`, items) } -func (r *artworkQueueRepository) EnqueueStaleAbsent(kind model.Kind, attemptedBefore time.Time, limit int) (int64, error) { - now := time.Now() - return r.insertIfNotQueued("", `SELECT item_kind, item_id, image_type, ?, 0, ?, ? - FROM `+itemArtworkTable+` WHERE item_kind = ? AND hash = '' AND attempted_at < ? - ORDER BY attempted_at LIMIT ?`, - model.ArtworkPriorityRecheck, now, now, kind.Prefix(), attemptedBefore, limit) -} - func (r *artworkQueueRepository) EnqueueAllMissing(kind model.Kind, priority int) (int64, error) { entityTable, ok := artworkOwnerTables[kind] if !ok { @@ -231,13 +223,11 @@ func (r *artworkQueueRepository) Count() (int64, error) { return res.Count, err } -// CountAbsent matches EnqueueStaleAbsent on hash, so the stale count is the pool a recheck drains from. -func (r *artworkQueueRepository) CountAbsent(kind model.Kind, attemptedBefore time.Time) (model.ArtworkAbsentStat, error) { - var res model.ArtworkAbsentStat - err := r.queryOne(Select("count(*) as total"). - Column(Expr("coalesce(sum(attempted_at < ?), 0) as stale", attemptedBefore)). - From(itemArtworkTable).Where(Eq{"item_kind": kind.Prefix(), "hash": ""}), &res) - return res, 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 1673a5b0e..830de4e5e 100644 --- a/persistence/artwork_queue_repository_test.go +++ b/persistence/artwork_queue_repository_test.go @@ -237,41 +237,6 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(ids).To(ConsistOf(albumSgtPeppers.ID, artistKraftwerk.ID, plsBest.ID, radioWithHomePage.ID, songDayInALife.ID)) }) - It("enqueues stale absent states for recheck", func() { - awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) - old := time.Now().Add(-48 * time.Hour) - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) - - n, err := repo.EnqueueStaleAbsent(model.KindArtistArtwork, time.Now().Add(-24*time.Hour), 100) - Expect(err).ToNot(HaveOccurred()) - Expect(n).To(Equal(int64(1))) - - items, err := repo.DequeueBatch(10) - Expect(err).ToNot(HaveOccurred()) - Expect(items).To(HaveLen(1)) - Expect(items[0].ItemID).To(Equal("stale1")) - Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) - }) - - It("enqueues only the oldest stale absent states up to the limit", func() { - awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) - now := time.Now() - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "oldest", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: now.Add(-72 * time.Hour)})).To(Succeed()) - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "older", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: now.Add(-60 * time.Hour)})).To(Succeed()) - Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "old", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: now.Add(-48 * time.Hour)})).To(Succeed()) - - n, err := repo.EnqueueStaleAbsent(model.KindArtistArtwork, now.Add(-24*time.Hour), 2) - Expect(err).ToNot(HaveOccurred()) - Expect(n).To(Equal(int64(2))) - - items, err := repo.DequeueBatch(10) - Expect(err).ToNot(HaveOccurred()) - ids := slice.Map(items, func(it model.ArtworkQueueItem) string { return it.ItemID }) - Expect(ids).To(ConsistOf("oldest", "older")) - }) - It("enqueues entities that have no item_artwork row at all", func() { awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumSgtPeppers.ID, ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: time.Now()})).To(Succeed()) @@ -439,7 +404,7 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(repo.CountQueued(nil, nil)).To(BeEmpty()) }) - It("counts absent states and how many are due for recheck", func() { + It("counts absent states", func() { awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) old := time.Now().Add(-48 * time.Hour) for _, ia := range []model.ItemArtwork{ @@ -451,12 +416,11 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(awRepo.PutItemArtwork(&ia)).To(Succeed()) } - Expect(repo.CountAbsent(model.KindArtistArtwork, time.Now().Add(-24*time.Hour))). - To(Equal(model.ArtworkAbsentStat{Total: 2, Stale: 1})) + 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, time.Now())).To(Equal(model.ArtworkAbsentStat{})) + Expect(repo.CountAbsent(model.KindRadioArtwork)).To(Equal(int64(0))) }) }) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index 51ddf4b61..2e954053c 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -16,7 +16,7 @@ type MockArtworkQueueRepo struct { mu sync.Mutex Data map[string]model.ArtworkQueueItem // keyed by iaKey(kind, id, imageType) Err error - // ItemArtworkSource, when set, backs EnqueueStaleAbsent with real item_artwork state. + // ItemArtworkSource, when set, backs the set-difference insert with real item_artwork state. ItemArtworkSource *MockArtworkRepo // ExistingIDs is keyed by item_kind; a nil per-kind map means PurgeDangling keeps that kind. ExistingIDs map[string]map[string]bool @@ -227,23 +227,19 @@ func (m *MockArtworkQueueRepo) CountQueued(kinds []model.Kind, priorities []int) } // CountAbsent mirrors the SQL predicate: an absent state is one with no hash. -func (m *MockArtworkQueueRepo) CountAbsent(kind model.Kind, attemptedBefore time.Time) (model.ArtworkAbsentStat, error) { +func (m *MockArtworkQueueRepo) CountAbsent(kind model.Kind) (int64, error) { m.mu.Lock() defer m.mu.Unlock() - var res model.ArtworkAbsentStat if m.Err != nil || m.ItemArtworkSource == nil { - return res, m.Err + return 0, m.Err } + var total int64 for _, ia := range m.ItemArtworkSource.ItemData { - if ia.ItemKind != kind.Prefix() || ia.Hash != "" { - continue - } - res.Total++ - if ia.AttemptedAt.Before(attemptedBefore) { - res.Stale++ + if ia.ItemKind == kind.Prefix() && ia.Hash == "" { + total++ } } - return res, nil + return total, nil } func (m *MockArtworkQueueRepo) EnqueuePreservingBackoff(items ...model.ArtworkQueueItem) error { @@ -272,41 +268,6 @@ func (m *MockArtworkQueueRepo) EnqueuePreservingBackoff(items ...model.ArtworkQu return nil } -func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind model.Kind, attemptedBefore time.Time, limit int) (int64, error) { - m.mu.Lock() - defer m.mu.Unlock() - if m.Err != nil || m.ItemArtworkSource == nil { - return 0, m.Err - } - var stale []model.ItemArtwork - for _, ia := range m.ItemArtworkSource.ItemData { - if ia.ItemKind == kind.Prefix() && ia.Hash == "" && ia.AttemptedAt.Before(attemptedBefore) { - stale = append(stale, ia) - } - } - slices.SortFunc(stale, func(a, b model.ItemArtwork) int { return a.AttemptedAt.Compare(b.AttemptedAt) }) - // The limit caps the selection, like the SQL's LIMIT before ON CONFLICT: queued rows use up budget. - stale = stale[:min(limit, len(stale))] - now := time.Now() - var inserted int64 - for _, ia := range stale { - k := iaKey(ia.ItemKind, ia.ItemID, ia.ImageType) - if _, ok := m.Data[k]; ok { // DO NOTHING: never touch existing queue rows - continue - } - m.Data[k] = model.ArtworkQueueItem{ - ItemKind: ia.ItemKind, - ItemID: ia.ItemID, - ImageType: ia.ImageType, - Priority: model.ArtworkPriorityRecheck, - RetryAt: now, - EnqueuedAt: now, - } - inserted++ - } - return inserted, nil -} - // matchingSource mirrors the SQL filter: no sources means every source, "" the absent state. func (m *MockArtworkQueueRepo) matchingSource(kind model.Kind, sources []string) []model.ItemArtwork { if m.ItemArtworkSource == nil { @@ -366,7 +327,7 @@ func (m *MockArtworkQueueRepo) EnqueueBySource(kind model.Kind, sources []string return inserted, nil } -// EnqueueMissing mirrors the SQL set-difference insert: ExistingIDs[kind] minus ItemArtworkSource. +// EnqueueAllMissing mirrors the SQL set-difference insert: ExistingIDs[kind] minus ItemArtworkSource. func (m *MockArtworkQueueRepo) EnqueueAllMissing(kind model.Kind, priority int) (int64, error) { m.mu.Lock() defer m.mu.Unlock()