diff --git a/core/agents/interfaces.go b/core/agents/interfaces.go index d5f4a6580..7fc5de361 100644 --- a/core/agents/interfaces.go +++ b/core/agents/interfaces.go @@ -53,6 +53,8 @@ func (s Song) Equals(other Song) bool { } var ( + // ErrNotFound means the provider answered and had nothing. Return the underlying error + // for a fault instead, or callers that back off on faults will treat it as definitive. ErrNotFound = errors.New("not found") ) diff --git a/core/artwork/gate.go b/core/artwork/gate.go index a64c11a20..ff77f733d 100644 --- a/core/artwork/gate.go +++ b/core/artwork/gate.go @@ -1,6 +1,7 @@ package artwork import ( + "context" "errors" "io" "sync" @@ -101,10 +102,13 @@ func (b *breaker) allow() bool { } func (b *breaker) record(name string, err error) { + // A cancelled run says nothing about the provider, so it neither counts nor clears. + if errors.Is(err, context.Canceled) { + return + } b.mu.Lock() defer b.mu.Unlock() - // A not-found is a definitive answer, not a fault; keep in sync with isTransientExternal. - if err == nil || errors.Is(err, model.ErrNotFound) || errors.Is(err, agents.ErrNotFound) { + if !isTransientExternal(err) { if b.failures >= breakerThreshold { log.Info("Artwork: Circuit breaker closed for agent", "agent", name) } diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 5b6e42885..9fad4b503 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -539,6 +539,43 @@ var _ = Describe("Worker", func() { Expect(calls).To(Equal(5), "the breaker should have re-closed after the success") }) + It("does not open the breaker when the run is cancelled", func() { + cancelled := func() (io.ReadCloser, string, error) { return nil, "", context.Canceled } + for range breakerThreshold + 3 { + _, _, err := w.gate("A", cancelled) + Expect(err).To(MatchError(context.Canceled), "a cancellation passes through, never errBreakerOpen") + } + + var calls int + counting := func() (io.ReadCloser, string, error) { + calls++ + return nil, "", errors.New("boom") + } + _, _, _ = w.gate("A", counting) + Expect(calls).To(Equal(1), "the breaker stayed closed, so the step still runs") + }) + + It("ignores a cancellation mid-run, neither counting nor clearing the failures", func() { + failing := func() (io.ReadCloser, string, error) { return nil, "", errors.New("boom") } + cancelled := func() (io.ReadCloser, string, error) { return nil, "", context.Canceled } + for range breakerThreshold - 1 { + _, _, _ = w.gate("A", failing) + } + _, _, _ = w.gate("A", cancelled) + + var calls int + counting := func() (io.ReadCloser, string, error) { + calls++ + return nil, "", errors.New("boom") + } + _, _, _ = w.gate("A", counting) + Expect(calls).To(Equal(1), "the cancellation must not have counted as the final failure") + + _, _, err := w.gate("A", counting) + Expect(err).To(MatchError(errBreakerOpen), "the cancellation must not have cleared the earlier failures") + Expect(calls).To(Equal(1), "an open breaker must not call the external step") + }) + It("does not open the breaker on a run of agent not-found misses", func() { // agents.ErrNotFound is a definitive miss, not a fault: artless items must not // trip the breaker, or they would loop in retry instead of settling absent. diff --git a/plugins/metadata_agent.go b/plugins/metadata_agent.go index e5d6d43fc..607926438 100644 --- a/plugins/metadata_agent.go +++ b/plugins/metadata_agent.go @@ -50,6 +50,15 @@ func newMetadataAgent(p *plugin) *MetadataAgent { return &MetadataAgent{name: p.name, plugin: p} } +// agentErr keeps a plugin fault distinguishable from a definitive miss: a method the plugin +// simply does not implement has answered, so it must not count against a caller's back-off. +func agentErr(err error) error { + if errors.Is(err, errNotImplemented) || errors.Is(err, errFunctionNotFound) { + return errors.Join(agents.ErrNotFound, err) + } + return err +} + // MetadataAgent is an adapter that wraps an Extism plugin and implements // the agents interfaces for metadata retrieval. type MetadataAgent struct { @@ -69,7 +78,7 @@ func (a *MetadataAgent) GetArtistMBID(ctx context.Context, id string, name strin input := capabilities.ArtistMBIDRequest{ID: id, Name: name} result, err := callPluginFunction[capabilities.ArtistMBIDRequest, *capabilities.ArtistMBIDResponse](ctx, a.plugin, FuncGetArtistMBID, input) if err != nil { - return "", errors.Join(agents.ErrNotFound, err) + return "", agentErr(err) } if result == nil || result.MBID == "" { @@ -84,7 +93,7 @@ func (a *MetadataAgent) GetArtistURL(ctx context.Context, id, name, mbid string) input := capabilities.ArtistRequest{ID: id, Name: name, MBID: mbid} result, err := callPluginFunction[capabilities.ArtistRequest, *capabilities.ArtistURLResponse](ctx, a.plugin, FuncGetArtistURL, input) if err != nil { - return "", errors.Join(agents.ErrNotFound, err) + return "", agentErr(err) } if result == nil || result.URL == "" { return "", agents.ErrNotFound @@ -97,7 +106,7 @@ func (a *MetadataAgent) GetArtistBiography(ctx context.Context, id, name, mbid s input := capabilities.ArtistRequest{ID: id, Name: name, MBID: mbid} result, err := callPluginFunction[capabilities.ArtistRequest, *capabilities.ArtistBiographyResponse](ctx, a.plugin, FuncGetArtistBiography, input) if err != nil { - return "", errors.Join(agents.ErrNotFound, err) + return "", agentErr(err) } if result == nil || result.Biography == "" { @@ -112,7 +121,7 @@ func (a *MetadataAgent) GetSimilarArtists(ctx context.Context, id, name, mbid st input := capabilities.SimilarArtistsRequest{ID: id, Name: name, MBID: mbid, Limit: int32(limit)} result, err := callPluginFunction[capabilities.SimilarArtistsRequest, *capabilities.SimilarArtistsResponse](ctx, a.plugin, FuncGetSimilarArtists, input) if err != nil { - return nil, errors.Join(agents.ErrNotFound, err) + return nil, agentErr(err) } if result == nil || len(result.Artists) == 0 { @@ -132,7 +141,7 @@ func (a *MetadataAgent) GetArtistImages(ctx context.Context, id, name, mbid stri input := capabilities.ArtistRequest{ID: id, Name: name, MBID: mbid} result, err := callPluginFunction[capabilities.ArtistRequest, *capabilities.ArtistImagesResponse](ctx, a.plugin, FuncGetArtistImages, input) if err != nil { - return nil, errors.Join(agents.ErrNotFound, err) + return nil, agentErr(err) } if result == nil || len(result.Images) == 0 { @@ -152,7 +161,7 @@ func (a *MetadataAgent) GetArtistTopSongs(ctx context.Context, id, artistName, m input := capabilities.TopSongsRequest{ID: id, Name: artistName, MBID: mbid, Count: int32(count)} result, err := callPluginFunction[capabilities.TopSongsRequest, *capabilities.TopSongsResponse](ctx, a.plugin, FuncGetArtistTopSongs, input) if err != nil { - return nil, errors.Join(agents.ErrNotFound, err) + return nil, agentErr(err) } if result == nil || len(result.Songs) == 0 { @@ -167,7 +176,7 @@ func (a *MetadataAgent) GetAlbumInfo(ctx context.Context, name, artist, mbid str input := capabilities.AlbumRequest{Name: name, Artist: artist, MBID: mbid} result, err := callPluginFunction[capabilities.AlbumRequest, *capabilities.AlbumInfoResponse](ctx, a.plugin, FuncGetAlbumInfo, input) if err != nil { - return nil, errors.Join(agents.ErrNotFound, err) + return nil, agentErr(err) } if result == nil { @@ -187,7 +196,7 @@ func (a *MetadataAgent) GetAlbumImages(ctx context.Context, name, artist, mbid s input := capabilities.AlbumRequest{Name: name, Artist: artist, MBID: mbid} result, err := callPluginFunction[capabilities.AlbumRequest, *capabilities.AlbumImagesResponse](ctx, a.plugin, FuncGetAlbumImages, input) if err != nil { - return nil, errors.Join(agents.ErrNotFound, err) + return nil, agentErr(err) } if result == nil || len(result.Images) == 0 { @@ -205,7 +214,7 @@ func (a *MetadataAgent) GetAlbumImages(ctx context.Context, name, artist, mbid s func callSimilarSongsPluginFunction[T any](ctx context.Context, plugin *plugin, funcName string, input T) ([]agents.Song, error) { result, err := callPluginFunction[T, *capabilities.SimilarSongsResponse](ctx, plugin, funcName, input) if err != nil { - return nil, err + return nil, agentErr(err) } if result == nil || len(result.Songs) == 0 { return nil, agents.ErrNotFound diff --git a/plugins/metadata_agent_test.go b/plugins/metadata_agent_test.go index a14db5d34..2dc67d41c 100644 --- a/plugins/metadata_agent_test.go +++ b/plugins/metadata_agent_test.go @@ -3,12 +3,36 @@ package plugins import ( + "errors" + "fmt" + "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/plugins/capabilities" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) +// The partial-metadata-agent fixture registers through the Go PDK, which exports every method +// and answers -2, so only errNotImplemented reaches agentErr through a real plugin. A plugin +// that omits the export entirely yields errFunctionNotFound, covered here directly. +var _ = Describe("agentErr", func() { + DescribeTable("classifies a plugin error as a miss or a fault", + func(err error, wantMiss bool) { + got := agentErr(err) + Expect(errors.Is(got, agents.ErrNotFound)).To(Equal(wantMiss)) + Expect(got).To(MatchError(err), "the underlying reason must survive for diagnostics") + }, + Entry("an unimplemented method is a miss", + fmt.Errorf("%w: %s", errNotImplemented, FuncGetArtistImages), true), + Entry("a missing export is a miss", + fmt.Errorf("%w: %s", errFunctionNotFound, FuncGetArtistImages), true), + Entry("a call failure is a fault", + fmt.Errorf("plugin call failed: %w", errors.New("returned status 429")), false), + Entry("a non-zero exit is a fault", + errors.New("plugin call exited with code 1"), false), + ) +}) + var _ = Describe("MetadataAgent", Ordered, func() { var agent agents.Interface @@ -166,82 +190,57 @@ var _ = Describe("MetadataAgent error handling", Ordered, func() { Expect(ok).To(BeTrue()) }) - It("returns error from GetArtistMBID", func() { - retriever := errorAgent.(agents.ArtistMBIDRetriever) - _, err := retriever.GetArtistMBID(GinkgoT().Context(), "artist-1", "Test") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetArtistURL", func() { - retriever := errorAgent.(agents.ArtistURLRetriever) - _, err := retriever.GetArtistURL(GinkgoT().Context(), "artist-1", "Test", "mbid") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetArtistBiography", func() { - retriever := errorAgent.(agents.ArtistBiographyRetriever) - _, err := retriever.GetArtistBiography(GinkgoT().Context(), "artist-1", "Test", "mbid") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetArtistImages", func() { - retriever := errorAgent.(agents.ArtistImageRetriever) - _, err := retriever.GetArtistImages(GinkgoT().Context(), "artist-1", "Test", "mbid") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetSimilarArtists", func() { - retriever := errorAgent.(agents.ArtistSimilarRetriever) - _, err := retriever.GetSimilarArtists(GinkgoT().Context(), "artist-1", "Test", "mbid", 5) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetArtistTopSongs", func() { - retriever := errorAgent.(agents.ArtistTopSongsRetriever) - _, err := retriever.GetArtistTopSongs(GinkgoT().Context(), "artist-1", "Test", "mbid", 5) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetAlbumInfo", func() { - retriever := errorAgent.(agents.AlbumInfoRetriever) - _, err := retriever.GetAlbumInfo(GinkgoT().Context(), "Album", "Artist", "mbid") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetAlbumImages", func() { - retriever := errorAgent.(agents.AlbumImageRetriever) - _, err := retriever.GetAlbumImages(GinkgoT().Context(), "Album", "Artist", "mbid") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetSimilarSongsByTrack", func() { - retriever := errorAgent.(agents.SimilarSongsByTrackRetriever) - _, err := retriever.GetSimilarSongsByTrack(GinkgoT().Context(), "track-1", "Test", "Artist", "mbid", 5) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetSimilarSongsByAlbum", func() { - retriever := errorAgent.(agents.SimilarSongsByAlbumRetriever) - _, err := retriever.GetSimilarSongsByAlbum(GinkgoT().Context(), "album-1", "Album", "Artist", "mbid", 5) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) - - It("returns error from GetSimilarSongsByArtist", func() { - retriever := errorAgent.(agents.SimilarSongsByArtistRetriever) - _, err := retriever.GetSimilarSongsByArtist(GinkgoT().Context(), "artist-1", "Artist", "mbid", 5) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("simulated plugin error")) - }) + DescribeTable("surfaces the plugin failure, not a definitive not-found", + func(call func() error) { + err := call() + Expect(err).To(MatchError(ContainSubstring("simulated plugin error"))) + Expect(err).ToNot(MatchError(agents.ErrNotFound)) + }, + Entry("GetArtistMBID", func() error { + _, err := errorAgent.(agents.ArtistMBIDRetriever).GetArtistMBID(GinkgoT().Context(), "artist-1", "Test") + return err + }), + Entry("GetArtistURL", func() error { + _, err := errorAgent.(agents.ArtistURLRetriever).GetArtistURL(GinkgoT().Context(), "artist-1", "Test", "mbid") + return err + }), + Entry("GetArtistBiography", func() error { + _, err := errorAgent.(agents.ArtistBiographyRetriever).GetArtistBiography(GinkgoT().Context(), "artist-1", "Test", "mbid") + return err + }), + Entry("GetSimilarArtists", func() error { + _, err := errorAgent.(agents.ArtistSimilarRetriever).GetSimilarArtists(GinkgoT().Context(), "artist-1", "Test", "mbid", 5) + return err + }), + Entry("GetArtistImages", func() error { + _, err := errorAgent.(agents.ArtistImageRetriever).GetArtistImages(GinkgoT().Context(), "artist-1", "Test", "mbid") + return err + }), + Entry("GetArtistTopSongs", func() error { + _, err := errorAgent.(agents.ArtistTopSongsRetriever).GetArtistTopSongs(GinkgoT().Context(), "artist-1", "Test", "mbid", 5) + return err + }), + Entry("GetAlbumInfo", func() error { + _, err := errorAgent.(agents.AlbumInfoRetriever).GetAlbumInfo(GinkgoT().Context(), "Album", "Artist", "mbid") + return err + }), + Entry("GetAlbumImages", func() error { + _, err := errorAgent.(agents.AlbumImageRetriever).GetAlbumImages(GinkgoT().Context(), "Album", "Artist", "mbid") + return err + }), + Entry("GetSimilarSongsByTrack", func() error { + _, err := errorAgent.(agents.SimilarSongsByTrackRetriever).GetSimilarSongsByTrack(GinkgoT().Context(), "track-1", "Test", "Artist", "mbid", 5) + return err + }), + Entry("GetSimilarSongsByAlbum", func() error { + _, err := errorAgent.(agents.SimilarSongsByAlbumRetriever).GetSimilarSongsByAlbum(GinkgoT().Context(), "album-1", "Album", "Artist", "mbid", 5) + return err + }), + Entry("GetSimilarSongsByArtist", func() error { + _, err := errorAgent.(agents.SimilarSongsByArtistRetriever).GetSimilarSongsByArtist(GinkgoT().Context(), "artist-1", "Artist", "mbid", 5) + return err + }), + ) }) var _ = Describe("MetadataAgent partial implementation", Ordered, func() { @@ -268,68 +267,55 @@ var _ = Describe("MetadataAgent partial implementation", Ordered, func() { Expect(bio).To(Equal("Partial agent biography for Test Artist")) }) - It("returns ErrNotFound for unimplemented method (GetArtistMBID)", func() { - retriever := partialAgent.(agents.ArtistMBIDRetriever) - _, err := retriever.GetArtistMBID(GinkgoT().Context(), "artist-1", "Test Artist") - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetArtistURL)", func() { - retriever := partialAgent.(agents.ArtistURLRetriever) - _, err := retriever.GetArtistURL(GinkgoT().Context(), "artist-1", "Test Artist", "mbid") - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetArtistImages)", func() { - retriever := partialAgent.(agents.ArtistImageRetriever) - _, err := retriever.GetArtistImages(GinkgoT().Context(), "artist-1", "Test Artist", "mbid") - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetSimilarArtists)", func() { - retriever := partialAgent.(agents.ArtistSimilarRetriever) - _, err := retriever.GetSimilarArtists(GinkgoT().Context(), "artist-1", "Test Artist", "mbid", 5) - Expect(err).To(MatchError(errNotImplemented)) - - }) - - It("returns ErrNotFound for unimplemented method (GetArtistTopSongs)", func() { - retriever := partialAgent.(agents.ArtistTopSongsRetriever) - _, err := retriever.GetArtistTopSongs(GinkgoT().Context(), "artist-1", "Test Artist", "mbid", 5) - Expect(err).To(MatchError(errNotImplemented)) - - }) - - It("returns ErrNotFound for unimplemented method (GetAlbumInfo)", func() { - retriever := partialAgent.(agents.AlbumInfoRetriever) - _, err := retriever.GetAlbumInfo(GinkgoT().Context(), "Album", "Artist", "mbid") - Expect(err).To(MatchError(errNotImplemented)) - - }) - - It("returns ErrNotFound for unimplemented method (GetAlbumImages)", func() { - retriever := partialAgent.(agents.AlbumImageRetriever) - _, err := retriever.GetAlbumImages(GinkgoT().Context(), "Album", "Artist", "mbid") - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetSimilarSongsByTrack)", func() { - retriever := partialAgent.(agents.SimilarSongsByTrackRetriever) - _, err := retriever.GetSimilarSongsByTrack(GinkgoT().Context(), "track-1", "Test", "Artist", "mbid", 5) - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetSimilarSongsByAlbum)", func() { - retriever := partialAgent.(agents.SimilarSongsByAlbumRetriever) - _, err := retriever.GetSimilarSongsByAlbum(GinkgoT().Context(), "album-1", "Album", "Artist", "mbid", 5) - Expect(err).To(MatchError(errNotImplemented)) - }) - - It("returns ErrNotFound for unimplemented method (GetSimilarSongsByArtist)", func() { - retriever := partialAgent.(agents.SimilarSongsByArtistRetriever) - _, err := retriever.GetSimilarSongsByArtist(GinkgoT().Context(), "artist-1", "Artist", "mbid", 5) - Expect(err).To(MatchError(errNotImplemented)) - }) + // An unimplemented optional method is a definitive miss. Reported as a fault it would + // count against the artwork circuit breaker and keep the item in the retry queue. + DescribeTable("reports an unimplemented method as a definitive not-found", + func(call func() error) { + err := call() + Expect(err).To(MatchError(errNotImplemented)) + Expect(err).To(MatchError(agents.ErrNotFound)) + }, + Entry("GetArtistMBID", func() error { + _, err := partialAgent.(agents.ArtistMBIDRetriever).GetArtistMBID(GinkgoT().Context(), "artist-1", "Test Artist") + return err + }), + Entry("GetArtistURL", func() error { + _, err := partialAgent.(agents.ArtistURLRetriever).GetArtistURL(GinkgoT().Context(), "artist-1", "Test Artist", "mbid") + return err + }), + Entry("GetArtistImages", func() error { + _, err := partialAgent.(agents.ArtistImageRetriever).GetArtistImages(GinkgoT().Context(), "artist-1", "Test Artist", "mbid") + return err + }), + Entry("GetSimilarArtists", func() error { + _, err := partialAgent.(agents.ArtistSimilarRetriever).GetSimilarArtists(GinkgoT().Context(), "artist-1", "Test Artist", "mbid", 5) + return err + }), + Entry("GetArtistTopSongs", func() error { + _, err := partialAgent.(agents.ArtistTopSongsRetriever).GetArtistTopSongs(GinkgoT().Context(), "artist-1", "Test Artist", "mbid", 5) + return err + }), + Entry("GetAlbumInfo", func() error { + _, err := partialAgent.(agents.AlbumInfoRetriever).GetAlbumInfo(GinkgoT().Context(), "Album", "Artist", "mbid") + return err + }), + Entry("GetAlbumImages", func() error { + _, err := partialAgent.(agents.AlbumImageRetriever).GetAlbumImages(GinkgoT().Context(), "Album", "Artist", "mbid") + return err + }), + Entry("GetSimilarSongsByTrack", func() error { + _, err := partialAgent.(agents.SimilarSongsByTrackRetriever).GetSimilarSongsByTrack(GinkgoT().Context(), "track-1", "Test", "Artist", "mbid", 5) + return err + }), + Entry("GetSimilarSongsByAlbum", func() error { + _, err := partialAgent.(agents.SimilarSongsByAlbumRetriever).GetSimilarSongsByAlbum(GinkgoT().Context(), "album-1", "Album", "Artist", "mbid", 5) + return err + }), + Entry("GetSimilarSongsByArtist", func() error { + _, err := partialAgent.(agents.SimilarSongsByArtistRetriever).GetSimilarSongsByArtist(GinkgoT().Context(), "artist-1", "Artist", "mbid", 5) + return err + }), + ) }) var _ = Describe("songRefToAgentSong multi-artist", func() {