diff --git a/plugins/lyrics_adapter.go b/plugins/lyrics_adapter.go index 9e02115e7..2b41b7603 100644 --- a/plugins/lyrics_adapter.go +++ b/plugins/lyrics_adapter.go @@ -2,10 +2,13 @@ package plugins import ( "context" + "fmt" + "time" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/plugins/capabilities" + "golang.org/x/sync/singleflight" ) const CapabilityLyrics Capability = "Lyrics" @@ -18,6 +21,10 @@ const ( // lyrics for whole queues, and the resulting burst can rate-limit upstream providers. const maxConcurrentLyricsCalls = 2 +// lyricsPluginCallTimeout bounds work detached from one caller's request so a +// disconnected client cannot leave a shared plugin lookup running forever. +const lyricsPluginCallTimeout = time.Minute + func init() { registerCapability( CapabilityLyrics, @@ -33,11 +40,54 @@ func newLyricsPlugin(p *plugin) *LyricsPlugin { type LyricsPlugin struct { name string plugin *plugin + calls singleflight.Group } -// GetLyrics calls the plugin to fetch lyrics, then content-sniffs each response -// via model.ParseLyrics (TTML/SRT/YAML/LRC/plain). +// GetLyrics coalesces concurrent lookups for the same track. The shared call is +// detached from any one request so one disconnected client does not cancel it +// for the remaining callers. func (l *LyricsPlugin) GetLyrics(ctx context.Context, mf *model.MediaFile) (model.LyricList, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + + result := l.calls.DoChan(lyricsPluginCallKey(mf), func() (any, error) { + callCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), lyricsPluginCallTimeout) + defer cancel() + return l.getLyrics(callCtx, mf) + }) + + select { + case call := <-result: + if call.Err != nil { + return nil, call.Err + } + lyricsList, ok := call.Val.(model.LyricList) + if !ok { + return nil, fmt.Errorf("unexpected lyrics plugin result type %T", call.Val) + } + return lyricsList, nil + case <-ctx.Done(): + return nil, ctx.Err() + } +} + +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, + ) +} + +// 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) { select { case l.plugin.lyricsSem <- struct{}{}: defer func() { <-l.plugin.lyricsSem }() diff --git a/plugins/lyrics_adapter_test.go b/plugins/lyrics_adapter_test.go index d110665f5..5aba65adb 100644 --- a/plugins/lyrics_adapter_test.go +++ b/plugins/lyrics_adapter_test.go @@ -58,6 +58,106 @@ var _ = Describe("LyricsPlugin", Ordered, func() { Expect(result[0].Line[0].Value).To(ContainSubstring("Test Song")) }) + It("coalesces concurrent requests for the same track", 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 + } + }) + + type callResult struct { + lyrics model.LyricList + err error + } + start := make(chan struct{}) + results := make(chan callResult, 2) + track := &model.MediaFile{ID: "shared-track", Title: "Test Song", Artist: "Test Artist"} + for range 2 { + go func() { + <-start + lyrics, err := coalescingProvider.GetLyrics(GinkgoT().Context(), track) + results <- callResult{lyrics: lyrics, err: err} + }() + } + close(start) + + Consistently(results, "500ms").ShouldNot(Receive()) + <-sem + + for range 2 { + var result callResult + Eventually(results).Should(Receive(&result)) + Expect(result.err).ToNot(HaveOccurred()) + Expect(result.lyrics).To(HaveLen(1)) + } + + calls := metrics.getCalls() + Expect(calls).To(HaveLen(1)) + Expect(calls[0].method).To(Equal(FuncLyricsGetLyrics)) + }) + + It("keeps a shared request alive when one caller cancels", 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 + } + }) + + track := &model.MediaFile{ID: "shared-track", Title: "Test Song", Artist: "Test Artist"} + firstCtx, cancelFirst := context.WithCancel(GinkgoT().Context()) + firstDone := make(chan error, 1) + go func() { + _, err := coalescingProvider.GetLyrics(firstCtx, track) + firstDone <- err + }() + Consistently(firstDone, "100ms").ShouldNot(Receive()) + + secondDone := make(chan error, 1) + go func() { + _, err := coalescingProvider.GetLyrics(GinkgoT().Context(), track) + secondDone <- err + }() + Consistently(secondDone, "100ms").ShouldNot(Receive()) + + cancelFirst() + Eventually(firstDone).Should(Receive(MatchError(context.Canceled))) + Consistently(secondDone, "100ms").ShouldNot(Receive()) + + <-sem + Eventually(secondDone).Should(Receive(BeNil())) + Expect(metrics.getCalls()).To(HaveLen(1)) + }) + It("defaults language to 'xxx' when plugin does not provide one", func() { manager, _ := createTestManagerWithPlugins(map[string]map[string]string{ "test-lyrics": {"no_lang": "true"},