diff --git a/core/artwork/trace.go b/core/artwork/trace.go index b46e09bfe..122ad65ba 100644 --- a/core/artwork/trace.go +++ b/core/artwork/trace.go @@ -2,6 +2,8 @@ package artwork import ( "context" + "errors" + "io" "slices" "sync" ) @@ -55,3 +57,33 @@ func traceFrom(ctx context.Context) *chainTrace { t, _ := ctx.Value(traceCtxKey{}).(*chainTrace) return t } + +var errOfflineSkipped = errors.New("artwork: external lookup skipped (offline)") + +// tracingGate records each external agent's outcome without changing what the gate returns. +func tracingGate(t *chainTrace, inner gateFunc) gateFunc { + return func(name string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) { + r, path, err := inner(name, f) + candidate := "external:" + name + switch { + case r != nil: + t.add(traceStep{Candidate: candidate, Outcome: outcomeHit, Detail: path}) + case errors.Is(err, errBreakerOpen): + t.add(traceStep{Candidate: candidate, Outcome: outcomeSkipped, Detail: "circuit breaker open"}) + case isTransientExternal(err): + t.add(traceStep{Candidate: candidate, Outcome: outcomeError, Detail: err.Error()}) + default: + t.add(traceStep{Candidate: candidate, Outcome: outcomeMiss}) + } + return r, path, err + } +} + +// offlineGate reports which agents would be asked without asking them, so a diagnostic +// command cannot add load to a provider that is already rate-limiting us. +func offlineGate(t *chainTrace) gateFunc { + return func(name string, _ func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) { + t.add(traceStep{Candidate: "external:" + name, Outcome: outcomeWouldTry}) + return nil, "", errOfflineSkipped + } +} diff --git a/core/artwork/trace_test.go b/core/artwork/trace_test.go index b772df71d..b60c1414c 100644 --- a/core/artwork/trace_test.go +++ b/core/artwork/trace_test.go @@ -2,9 +2,12 @@ package artwork import ( "context" + "errors" + "io" "os" "path/filepath" "runtime" + "strings" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -102,6 +105,75 @@ var _ = Describe("chainState tracing", func() { }) }) +var _ = Describe("external gate tracing", func() { + hit := func() (io.ReadCloser, string, error) { + return io.NopCloser(strings.NewReader("x")), "http://img", nil + } + miss := func() (io.ReadCloser, string, error) { return nil, "", agents.ErrNotFound } + boom := func() (io.ReadCloser, string, error) { return nil, "", errors.New("returned status 429") } + + It("records a hit with the image path", func() { + t := &chainTrace{} + g := tracingGate(t, passthroughGate) + + r, _, err := g("deezer", hit) + + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + Expect(t.Steps()).To(Equal([]traceStep{ + {Candidate: "external:deezer", Outcome: outcomeHit, Detail: "http://img"}, + })) + }) + + It("records a miss for a not-found", func() { + t := &chainTrace{} + _, _, _ = tracingGate(t, passthroughGate)("deezer", miss) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeMiss)) + }) + + It("records a miss for a model not-found", func() { + t := &chainTrace{} + notFound := func() (io.ReadCloser, string, error) { return nil, "", model.ErrNotFound } + _, _, _ = tracingGate(t, passthroughGate)("deezer", notFound) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeMiss), + "both not-found flavours are definitive answers, not faults") + }) + + It("records an error with its reason", func() { + t := &chainTrace{} + _, _, _ = tracingGate(t, passthroughGate)("apple-music", boom) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeError)) + Expect(t.Steps()[0].Detail).To(ContainSubstring("429")) + }) + + It("records skipped when the breaker is open", func() { + t := &chainTrace{} + open := func(string, func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) { + return nil, "", errBreakerOpen + } + _, _, _ = tracingGate(t, open)("apple-music", hit) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeSkipped)) + Expect(t.Steps()[0].Detail).To(ContainSubstring("circuit breaker")) + }) + + It("never calls the agent in offline mode", func() { + t := &chainTrace{} + called := false + counting := func() (io.ReadCloser, string, error) { + called = true + return hit() + } + + _, _, err := offlineGate(t)("deezer", counting) + + Expect(called).To(BeFalse(), "offline mode must not perform external requests") + Expect(err).To(MatchError(errOfflineSkipped)) + Expect(t.Steps()).To(Equal([]traceStep{ + {Candidate: "external:deezer", Outcome: outcomeWouldTry}, + })) + }) +}) + var _ = Describe("resolveAlbum tracing", func() { var ( ctx context.Context