diff --git a/plugins/lyrics_adapter.go b/plugins/lyrics_adapter.go index 2b41b7603..5ad70be02 100644 --- a/plugins/lyrics_adapter.go +++ b/plugins/lyrics_adapter.go @@ -2,6 +2,7 @@ package plugins import ( "context" + "encoding/json" "fmt" "time" @@ -51,10 +52,18 @@ func (l *LyricsPlugin) GetLyrics(ctx context.Context, mf *model.MediaFile) (mode return nil, err } - result := l.calls.DoChan(lyricsPluginCallKey(mf), func() (any, error) { + req := capabilities.GetLyricsRequest{ + Track: mediaFileToTrackInfo(l.plugin, mf), + } + key, err := lyricsPluginCallKey(req) + if err != nil { + return nil, err + } + + result := l.calls.DoChan(key, func() (any, error) { callCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), lyricsPluginCallTimeout) defer cancel() - return l.getLyrics(callCtx, mf) + return l.getLyrics(callCtx, mf, req) }) select { @@ -72,31 +81,23 @@ func (l *LyricsPlugin) GetLyrics(ctx context.Context, mf *model.MediaFile) (mode } } -func lyricsPluginCallKey(mf *model.MediaFile) string { - return fmt.Sprintf( - "%s\x00%s\x00%d\x00%s\x00%s\x00%s\x00%.3f", - mf.ID, - mf.Path, - mf.UpdatedAt.UnixNano(), - mf.Title, - mf.Artist, - mf.Album, - mf.Duration, - ) +func lyricsPluginCallKey(req capabilities.GetLyricsRequest) (string, error) { + value, err := json.Marshal(req) + if err != nil { + return "", fmt.Errorf("encode lyrics plugin request key: %w", err) + } + return string(value), nil } // getLyrics calls the plugin, then content-sniffs each response via // model.ParseLyrics (TTML/SRT/YAML/LRC/plain). -func (l *LyricsPlugin) getLyrics(ctx context.Context, mf *model.MediaFile) (model.LyricList, error) { +func (l *LyricsPlugin) getLyrics(ctx context.Context, mf *model.MediaFile, req capabilities.GetLyricsRequest) (model.LyricList, error) { select { case l.plugin.lyricsSem <- struct{}{}: defer func() { <-l.plugin.lyricsSem }() case <-ctx.Done(): return nil, ctx.Err() } - req := capabilities.GetLyricsRequest{ - Track: mediaFileToTrackInfo(l.plugin, mf), - } resp, err := callPluginFunction[capabilities.GetLyricsRequest, capabilities.GetLyricsResponse]( ctx, l.plugin, FuncLyricsGetLyrics, req, ) diff --git a/plugins/lyrics_adapter_test.go b/plugins/lyrics_adapter_test.go index 5aba65adb..b366a387e 100644 --- a/plugins/lyrics_adapter_test.go +++ b/plugins/lyrics_adapter_test.go @@ -111,6 +111,54 @@ var _ = Describe("LyricsPlugin", Ordered, func() { Expect(calls[0].method).To(Equal(FuncLyricsGetLyrics)) }) + It("does not coalesce requests with different plugin metadata", func() { + metrics := &mockMetricsRecorder{} + manager, _ := createTestManagerWithPluginsAndMetrics( + nil, + metrics, + "test-lyrics"+PackageExtension, + ) + + p, ok := manager.LoadLyricsProvider("test-lyrics") + Expect(ok).To(BeTrue()) + coalescingProvider := p.(*LyricsPlugin) + + sem := coalescingProvider.plugin.lyricsSem + for range cap(sem) { + sem <- struct{}{} + } + DeferCleanup(func() { + for len(sem) > 0 { + <-sem + } + }) + + start := make(chan struct{}) + results := make(chan error, 2) + tracks := []*model.MediaFile{ + {ID: "same-id", Title: "Test Song", Artist: "Test Artist", TrackNumber: 1}, + {ID: "same-id", Title: "Test Song", Artist: "Test Artist", TrackNumber: 2}, + } + for _, track := range tracks { + go func() { + <-start + _, err := coalescingProvider.GetLyrics(GinkgoT().Context(), track) + results <- err + }() + } + close(start) + + Consistently(results, "500ms").ShouldNot(Receive()) + for range cap(sem) { + <-sem + } + + for range tracks { + Eventually(results).Should(Receive(BeNil())) + } + Expect(metrics.getCalls()).To(HaveLen(2)) + }) + It("keeps a shared request alive when one caller cancels", func() { metrics := &mockMetricsRecorder{} manager, _ := createTestManagerWithPluginsAndMetrics(