diff --git a/cmd/artwork.go b/cmd/artwork.go index 72291460c..c3bef0b07 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -44,7 +44,8 @@ func init() { artworkReprocessCmd.Flags().StringSliceVar(&artworkKinds, "kind", nil, "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)") + "only items currently resolved from these sources (e.g. folder, external:deezer, absent, "+ + "or failed for the absent ones that gave up)") artworkReprocessCmd.Flags().BoolVar(&artworkAll, "all", false, "reprocess every kind") artworkReprocessCmd.Flags().BoolVar(&artworkDryRun, "dry-run", false, "report what would be queued and exit without queueing") @@ -194,9 +195,9 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error 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 == "" { - failed, err := q.CountAbsentAfterFailure(k) + failed, err := q.CountBySource(k, []string{model.ArtworkSourceFailed}) if err != nil { - return rep, fmt.Errorf("counting absent %s artwork that gave up: %w", k, err) + return rep, fmt.Errorf("counting failed %s artwork: %w", k, err) } rep.absent = append(rep.absent, absentCount{kind: k, count: n, afterFailure: failed}) } @@ -228,12 +229,13 @@ func formatStatus(rep statusReport) string { } fmt.Fprintln(w, "\nAbsent (resolved, no image found)") - fmt.Fprintln(w, " KIND\tABSENT\tGAVE UP") + fmt.Fprintln(w, " KIND\tABSENT\tFAILED") for _, a := range rep.absent { fmt.Fprintf(w, " %s\t%d\t%d\n", a.kind, a.count, a.afterFailure) } fmt.Fprintln(w, " (never retried on their own; run 'artwork reprocess --source absent')") - fmt.Fprintln(w, " (gave up: the retry budget ran out rather than a source answering; most likely to resolve)") + fmt.Fprintln(w, " (failed: the retry budget ran out rather than a source answering; retry just those\n"+ + " with 'artwork reprocess --source failed')") fmt.Fprintln(w, "\nConfig") fmt.Fprintf(w, " State:\t%s\n", configState(rep)) @@ -347,19 +349,31 @@ func selectedKinds(kinds, sources []string, all bool) ([]model.Kind, error) { }) } -// absentSource is how the stored empty source — resolved, no image — is spelled on the CLI. -const absentSource = "absent" +// absentSource is how the stored empty source — resolved, no image — is spelled on the CLI, and +// failedSource the subset of it that gave up rather than being answered. +const ( + absentSource = "absent" + failedSource = "failed" +) func repositorySources(sources []string) []string { return slice.Map(sources, func(s string) string { - if s == absentSource { + switch s { + case absentSource: return "" + case failedSource: + return model.ArtworkSourceFailed } return s }) } -func displaySource(s string) string { return cmp.Or(s, absentSource) } +func displaySource(s string) string { + if s == model.ArtworkSourceFailed { + return failedSource + } + return cmp.Or(s, absentSource) +} type confirmFunc func(out io.Writer, total, external int64) bool @@ -442,7 +456,8 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error { } var unknown []string for _, s := range sources { - if s != "" && !slices.Contains(inUse, s) { // the reserved absent source is valid even when nothing is absent + // The reserved absent and failed sources are valid even when nothing currently matches them. + if s != "" && s != model.ArtworkSourceFailed && !slices.Contains(inUse, s) { unknown = append(unknown, displaySource(s)) } } diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 4bee2f203..19918e482 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -511,6 +511,11 @@ var _ = Describe("repositorySources", func() { Expect(repositorySources([]string{"absent", "folder"})).To(Equal([]string{"", "folder"})) }) + It("maps the failed name onto the pseudo-source, and back for display", func() { + Expect(repositorySources([]string{failedSource})).To(Equal([]string{model.ArtworkSourceFailed})) + Expect(displaySource(model.ArtworkSourceFailed)).To(Equal(failedSource)) + }) + It("keeps an empty selection empty, meaning every source", func() { Expect(repositorySources(nil)).To(BeEmpty()) }) @@ -898,7 +903,7 @@ var _ = Describe("formatStatus", func() { It("splits the absent total by how it was reached", func() { out := block(formatStatus(rep), "Absent (resolved, no image found)") Expect(out).To(MatchRegexp(`artist\s+2\s+1`), "2 absent, of which 1 gave up") - Expect(formatStatus(rep)).To(ContainSubstring("the retry budget ran out")) + Expect(formatStatus(rep)).To(ContainSubstring("artwork reprocess --source failed")) }) It("says absent states are never retried on their own, and names the command that does", func() { diff --git a/model/artwork.go b/model/artwork.go index c6bfadf1d..8856a35fa 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -18,6 +18,10 @@ type Artwork struct { const ImageTypePrimary = "primary" +// ArtworkSourceFailed is a pseudo-source selecting absent states that exhausted the retry budget +// rather than being answered. The "!" keeps it from colliding with a stored source value. +const ArtworkSourceFailed = "!failed" + // ItemImage is per-entity artwork state hydrated at query time; never persisted. type ItemImage struct { ImageHash string `structs:"-" json:"imageHash,omitempty"` @@ -142,13 +146,11 @@ type ArtworkQueueRepository interface { // EnqueueIfMissing inserts only for items with no item_artwork row yet. EnqueueIfMissing(items ...ArtworkQueueItem) error // CountBySource reports how many items of a kind currently resolve from the given sources. - // An empty sources slice means every source; "" matches absent state. + // An empty sources slice means every source; "" matches absent state, and the pseudo-source + // ArtworkSourceFailed matches the absent states that gave up. CountBySource(kind Kind, sources []string) (int64, error) // SourcesInUse lists the distinct sources items of a kind currently resolve from, "" included. SourcesInUse(kind Kind) ([]string, error) - // CountAbsentAfterFailure reports absent states a kind recorded because the retry budget ran - // out, not because every source answered. Those are the ones a reprocess is likely to resolve. - CountAbsentAfterFailure(kind Kind) (int64, error) // EnqueueBySource inserts queue rows for items of a kind whose current source matches. // It does not clear existing artwork state: the current image stays until it is replaced. EnqueueBySource(kind Kind, sources []string, priority int) (int64, error) diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index 2889bddaa..88b6f6f80 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -102,13 +102,23 @@ func (r *artworkQueueRepository) insertIfNotQueued(with, sql string, args ...any ` (`+strings.Join(enqueueColumns, ", ")+`) `+sql+skipIfQueued, args...)) } -// artworkSourceFilter selects item_artwork rows of a kind; no sources means every source, "" the absent state. +// artworkSourceFilter selects item_artwork rows of a kind; no sources means every source, "" the +// absent state, and ArtworkSourceFailed the absent states that gave up. Several are a union, so +// asking for both absent and failed is just absent. func artworkSourceFilter(kind model.Kind, sources []string) Sqlizer { f := And{Eq{"item_kind": kind.Prefix()}} - if len(sources) > 0 { - f = append(f, Eq{"source": sources}) + if len(sources) == 0 { + return f } - return f + stored := slices.DeleteFunc(slices.Clone(sources), func(s string) bool { return s == model.ArtworkSourceFailed }) + var match Or + if len(stored) > 0 { + match = append(match, Eq{"source": stored}) + } + if len(stored) != len(sources) { + match = append(match, And{Eq{"hash": ""}, NotEq{"last_failure": ""}}) + } + return append(f, match) } func (r *artworkQueueRepository) CountBySource(kind model.Kind, sources []string) (int64, error) { @@ -223,11 +233,4 @@ func (r *artworkQueueRepository) Count() (int64, error) { return res.Count, err } -func (r *artworkQueueRepository) CountAbsentAfterFailure(kind model.Kind) (int64, error) { - var res struct{ Count int64 } - err := r.queryOne(Select("count(*) as count").From(itemArtworkTable). - Where(And{Eq{"item_kind": kind.Prefix(), "hash": ""}, NotEq{"last_failure": ""}}), &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 2b936eca2..0481d4193 100644 --- a/persistence/artwork_queue_repository_test.go +++ b/persistence/artwork_queue_repository_test.go @@ -400,7 +400,7 @@ var _ = Describe("ArtworkQueueRepository", func() { )) }) - It("counts only the absent states that gave up, not those a source answered", func() { + It("selects only the absent states that gave up, not those a source answered", func() { awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) for _, ia := range []model.ItemArtwork{ {ItemKind: "ar", ItemID: "gaveup", ImageType: model.ImageTypePrimary, LastFailure: "[]"}, @@ -410,17 +410,33 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(awRepo.PutItemArtwork(&ia)).To(Succeed()) } - Expect(repo.CountAbsentAfterFailure(model.KindArtistArtwork)).To(Equal(int64(1)), + Expect(repo.CountBySource(model.KindArtistArtwork, []string{model.ArtworkSourceFailed})).To(Equal(int64(1)), "an item still serving art is not absent, however its last attempt went") // A later success rewrites the row, clearing the record. Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "gaveup", ImageType: model.ImageTypePrimary, Hash: "hZ"})).To(Succeed()) - Expect(repo.CountAbsentAfterFailure(model.KindArtistArtwork)).To(Equal(int64(0))) + Expect(repo.CountBySource(model.KindArtistArtwork, []string{model.ArtworkSourceFailed})).To(Equal(int64(0))) }) - It("reports a kind that never gave up as zero", func() { - Expect(repo.CountAbsentAfterFailure(model.KindRadioArtwork)).To(Equal(int64(0))) + It("unions the failed pseudo-source with a real one, so absent plus failed is just absent", func() { + awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) + for _, ia := range []model.ItemArtwork{ + {ItemKind: "ar", ItemID: "gaveup", ImageType: model.ImageTypePrimary, LastFailure: "[]"}, + {ItemKind: "ar", ItemID: "toldno", ImageType: model.ImageTypePrimary}, + {ItemKind: "ar", ItemID: "folder", ImageType: model.ImageTypePrimary, Hash: "hX", Source: "folder"}, + } { + Expect(awRepo.PutItemArtwork(&ia)).To(Succeed()) + } + failedAndAbsent := []string{model.ArtworkSourceFailed, ""} + Expect(repo.CountBySource(model.KindArtistArtwork, failedAndAbsent)).To(Equal(int64(2)), + "failed is a subset of absent, so asking for both is asking for absent") + Expect(repo.CountBySource(model.KindArtistArtwork, []string{model.ArtworkSourceFailed, "folder"})). + To(Equal(int64(2)), "a pseudo-source and a stored source combine as a union") + }) + + It("reports a kind with nothing failed as zero", func() { + Expect(repo.CountBySource(model.KindRadioArtwork, []string{model.ArtworkSourceFailed})).To(Equal(int64(0))) }) It("reports an empty queue as no rows", func() { diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index f0e09d4cf..c482e2150 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -252,14 +252,24 @@ func (m *MockArtworkQueueRepo) EnqueuePreservingBackoff(items ...model.ArtworkQu return nil } -// matchingSource mirrors the SQL filter: no sources means every source, "" the absent state. +// matchingSource mirrors the SQL filter: no sources means every source, "" the absent state, and +// ArtworkSourceFailed the absent states that gave up. func (m *MockArtworkQueueRepo) matchingSource(kind model.Kind, sources []string) []model.ItemArtwork { if m.ItemArtworkSource == nil { return nil } + matches := func(ia model.ItemArtwork) bool { + if len(sources) == 0 { + return true + } + if slices.Contains(sources, ia.Source) { + return true + } + return slices.Contains(sources, model.ArtworkSourceFailed) && ia.Hash == "" && ia.LastFailure != "" + } var res []model.ItemArtwork for _, ia := range m.ItemArtworkSource.ItemData { - if ia.ItemKind == kind.Prefix() && (len(sources) == 0 || slices.Contains(sources, ia.Source)) { + if ia.ItemKind == kind.Prefix() && matches(ia) { res = append(res, ia) } } @@ -275,21 +285,6 @@ func (m *MockArtworkQueueRepo) CountBySource(kind model.Kind, sources []string) return int64(len(m.matchingSource(kind, sources))), nil } -func (m *MockArtworkQueueRepo) CountAbsentAfterFailure(kind model.Kind) (int64, error) { - m.mu.Lock() - defer m.mu.Unlock() - if m.Err != nil || m.ItemArtworkSource == nil { - return 0, m.Err - } - var n int64 - for _, ia := range m.ItemArtworkSource.ItemData { - if ia.ItemKind == kind.Prefix() && ia.Hash == "" && ia.LastFailure != "" { - n++ - } - } - return n, nil -} - func (m *MockArtworkQueueRepo) SourcesInUse(kind model.Kind) ([]string, error) { m.mu.Lock() defer m.mu.Unlock()