perf(plugins): coalesce lyrics requests

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 }()

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"},