From 6d4ac1506ff7f435418f79de1fc4cd7b097d0e60 Mon Sep 17 00:00:00 2001 From: ranokay Date: Tue, 25 Aug 2026 20:53:01 +0300 Subject: [PATCH] fix(plugins): cancel abandoned lyrics lookups --- plugins/lyrics_adapter.go | 102 ++++++++++++++++++++++++++++----- plugins/lyrics_adapter_test.go | 30 ++++++++++ plugins/manager_plugin.go | 7 +-- 3 files changed, 120 insertions(+), 19 deletions(-) diff --git a/plugins/lyrics_adapter.go b/plugins/lyrics_adapter.go index 20479c333..76a0fb885 100644 --- a/plugins/lyrics_adapter.go +++ b/plugins/lyrics_adapter.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "sync" "time" "github.com/navidrome/navidrome/log" @@ -25,6 +26,86 @@ const maxConcurrentLyricsCalls = 2 // disconnected client cannot leave a shared plugin lookup running forever. const lyricsPluginCallTimeout = time.Minute +// lyricsCallGroup coalesces lookups while keeping their lifetime tied to the +// callers that are still waiting. One caller may leave without interrupting the +// others, but the shared work is cancelled once the last waiter is gone. +type lyricsCallGroup struct { + mu sync.Mutex + calls map[string]*lyricsCall +} + +type lyricsCall struct { + group *lyricsCallGroup + key string + ctx context.Context + cancel context.CancelFunc + done chan struct{} + lyrics model.LyricList + err error + waiters int + finished bool +} + +func (g *lyricsCallGroup) join( + parent context.Context, + key string, + lookup func(context.Context) (model.LyricList, error), +) *lyricsCall { + g.mu.Lock() + if call := g.calls[key]; call != nil { + call.waiters++ + g.mu.Unlock() + return call + } + + ctx, cancel := context.WithTimeout(context.WithoutCancel(parent), lyricsPluginCallTimeout) + call := &lyricsCall{ + group: g, + key: key, + ctx: ctx, + cancel: cancel, + done: make(chan struct{}), + waiters: 1, + } + if g.calls == nil { + g.calls = make(map[string]*lyricsCall) + } + g.calls[key] = call + g.mu.Unlock() + + go call.run(lookup) + return call +} + +func (c *lyricsCall) run(lookup func(context.Context) (model.LyricList, error)) { + lyrics, err := lookup(c.ctx) + + c.group.mu.Lock() + c.lyrics = lyrics + c.err = err + c.finished = true + if c.group.calls[c.key] == c { + delete(c.group.calls, c.key) + } + close(c.done) + c.group.mu.Unlock() + c.cancel() +} + +func (c *lyricsCall) release() { + c.group.mu.Lock() + c.waiters-- + shouldCancel := c.waiters == 0 && !c.finished + if shouldCancel && c.group.calls[c.key] == c { + delete(c.group.calls, c.key) + } + c.group.mu.Unlock() + + if shouldCancel { + c.cancel() + } +} + func init() { registerCapability( CapabilityLyrics, @@ -42,9 +123,8 @@ type LyricsPlugin struct { plugin *plugin } -// 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. +// GetLyrics coalesces concurrent lookups for the same track. The shared call +// survives individual disconnections while another caller is still waiting. func (l *LyricsPlugin) GetLyrics(ctx context.Context, mf *model.MediaFile) (model.LyricList, error) { if err := ctx.Err(); err != nil { return nil, err @@ -58,22 +138,14 @@ func (l *LyricsPlugin) GetLyrics(ctx context.Context, mf *model.MediaFile) (mode return nil, err } - result := l.plugin.lyricsCalls.DoChan(key, func() (any, error) { - callCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), lyricsPluginCallTimeout) - defer cancel() + call := l.plugin.lyricsCalls.join(ctx, key, func(callCtx context.Context) (model.LyricList, error) { return l.getLyrics(callCtx, mf, req) }) + defer call.release() 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 <-call.done: + return call.lyrics, call.err case <-ctx.Done(): return nil, ctx.Err() } diff --git a/plugins/lyrics_adapter_test.go b/plugins/lyrics_adapter_test.go index dc273a2a1..ac8a5b158 100644 --- a/plugins/lyrics_adapter_test.go +++ b/plugins/lyrics_adapter_test.go @@ -210,6 +210,36 @@ var _ = Describe("LyricsPlugin", Ordered, func() { Expect(metrics.getCalls()).To(HaveLen(1)) }) + It("cancels a shared request after its last caller leaves", func() { + var group lyricsCallGroup + firstCtx, cancelFirst := context.WithCancel(GinkgoT().Context()) + secondCtx, cancelSecond := context.WithCancel(GinkgoT().Context()) + started := make(chan struct{}) + stopped := make(chan error, 1) + + firstCall := group.join(firstCtx, "shared", func(ctx context.Context) (model.LyricList, error) { + close(started) + <-ctx.Done() + stopped <- ctx.Err() + return nil, ctx.Err() + }) + Eventually(started).Should(BeClosed()) + + secondCall := group.join(secondCtx, "shared", func(context.Context) (model.LyricList, error) { + Fail("started a second lookup for the same key") + return nil, nil + }) + Expect(secondCall).To(BeIdenticalTo(firstCall)) + + cancelFirst() + firstCall.release() + Consistently(stopped, "100ms").ShouldNot(Receive()) + + cancelSecond() + secondCall.release() + Eventually(stopped).Should(Receive(MatchError(context.Canceled))) + }) + It("defaults language to 'xxx' when plugin does not provide one", func() { manager, _ := createTestManagerWithPlugins(map[string]map[string]string{ "test-lyrics": {"no_lang": "true"}, diff --git a/plugins/manager_plugin.go b/plugins/manager_plugin.go index 394b4c881..cda2cd1f5 100644 --- a/plugins/manager_plugin.go +++ b/plugins/manager_plugin.go @@ -10,7 +10,6 @@ import ( extism "github.com/extism/go-sdk" "github.com/navidrome/navidrome/model" "github.com/tetratelabs/wazero" - "golang.org/x/sync/singleflight" ) // plugin represents a loaded plugin @@ -25,9 +24,9 @@ type plugin struct { allowedUserIDs []string // User IDs this plugin can access (from DB configuration) allUsers bool // If true, plugin can access all users libraries libraryAccess - lyricsSem chan struct{} // Caps concurrent lyrics calls (see LyricsPlugin.GetLyrics) - lyricsCalls singleflight.Group // Shared by the transient LyricsPlugin adapters - fsConfig wazero.FSConfig // Sandboxed library mounts, nil if no filesystem permission + lyricsSem chan struct{} // Caps concurrent lyrics calls (see LyricsPlugin.GetLyrics) + lyricsCalls lyricsCallGroup // Shared by the transient LyricsPlugin adapters + fsConfig wazero.FSConfig // Sandboxed library mounts, nil if no filesystem permission } // instanceConfig is used by every call site, so all instances get the sandboxed mounts.