From ae5ecc6ac3b220b1a7272e9f32e4eef6db17d8b3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 3 Jan 2026 21:49:00 -0500 Subject: [PATCH] fix: implement dynamic loading for buffered scrobbler plugins Signed-off-by: Deluan --- core/scrobbler/buffered_scrobbler.go | 39 +++++++-- core/scrobbler/play_tracker.go | 19 +++-- core/scrobbler/play_tracker_test.go | 116 +++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 13 deletions(-) diff --git a/core/scrobbler/buffered_scrobbler.go b/core/scrobbler/buffered_scrobbler.go index 4f64a3c2b..be36e1f24 100644 --- a/core/scrobbler/buffered_scrobbler.go +++ b/core/scrobbler/buffered_scrobbler.go @@ -9,11 +9,27 @@ import ( "github.com/navidrome/navidrome/model" ) +// Loader is a function that loads a scrobbler by name. +// It returns the scrobbler and true if found, or nil and false if not available. +// This allows the buffered scrobbler to always get the current plugin instance. +type Loader func() (Scrobbler, bool) + +// newBufferedScrobbler creates a buffered scrobbler that wraps a static scrobbler instance. +// Use this for builtin scrobblers that don't change. func newBufferedScrobbler(ds model.DataStore, s Scrobbler, service string) *bufferedScrobbler { + return newBufferedScrobblerWithLoader(ds, service, func() (Scrobbler, bool) { + return s, true + }) +} + +// newBufferedScrobblerWithLoader creates a buffered scrobbler that dynamically loads +// the underlying scrobbler on each call. Use this for plugin scrobblers that may be +// reloaded (e.g., after configuration changes). +func newBufferedScrobblerWithLoader(ds model.DataStore, service string, loader Loader) *bufferedScrobbler { ctx, cancel := context.WithCancel(context.Background()) b := &bufferedScrobbler{ ds: ds, - wrapped: s, + loader: loader, service: service, wakeSignal: make(chan struct{}, 1), ctx: ctx, @@ -25,7 +41,7 @@ func newBufferedScrobbler(ds model.DataStore, s Scrobbler, service string) *buff type bufferedScrobbler struct { ds model.DataStore - wrapped Scrobbler + loader Loader service string wakeSignal chan struct{} ctx context.Context @@ -39,11 +55,19 @@ func (b *bufferedScrobbler) Stop() { } func (b *bufferedScrobbler) IsAuthorized(ctx context.Context, userId string) bool { - return b.wrapped.IsAuthorized(ctx, userId) + s, ok := b.loader() + if !ok { + return false + } + return s.IsAuthorized(ctx, userId) } func (b *bufferedScrobbler) NowPlaying(ctx context.Context, userId string, track *model.MediaFile, position int) error { - return b.wrapped.NowPlaying(ctx, userId, track, position) + s, ok := b.loader() + if !ok { + return errors.New("scrobbler not available") + } + return s.NowPlaying(ctx, userId, track, position) } func (b *bufferedScrobbler) Scrobble(ctx context.Context, userId string, s Scrobble) error { @@ -107,8 +131,13 @@ func (b *bufferedScrobbler) processUserQueue(ctx context.Context, userId string) if entry == nil { return true } + s, ok := b.loader() + if !ok { + log.Warn(ctx, "Scrobbler not available, will retry later", "scrobbler", b.service) + return false + } log.Debug(ctx, "Sending scrobble", "scrobbler", b.service, "track", entry.Title, "artist", entry.Artist) - err = b.wrapped.Scrobble(ctx, entry.UserID, Scrobble{ + err = s.Scrobble(ctx, entry.UserID, Scrobble{ MediaFile: entry.MediaFile, TimeStamp: entry.PlayTime, }) diff --git a/core/scrobbler/play_tracker.go b/core/scrobbler/play_tracker.go index 49c1dd87b..a40808007 100644 --- a/core/scrobbler/play_tracker.go +++ b/core/scrobbler/play_tracker.go @@ -116,7 +116,7 @@ func (p *playTracker) stopNowPlayingWorker() { <-p.workerDone // Wait for worker to finish } -// pluginNamesMatchScrobblers returns true if the set of pluginNames matches the keys in pluginScrobblers +// pluginNamesMatchScrobblers returns true if the set of pluginNames matches the keys in pluginScrobblers. func pluginNamesMatchScrobblers(pluginNames []string, scrobblers map[string]Scrobbler) bool { if len(pluginNames) != len(scrobblers) { return false @@ -129,7 +129,9 @@ func pluginNamesMatchScrobblers(pluginNames []string, scrobblers map[string]Scro return true } -// refreshPluginScrobblers updates the pluginScrobblers map to match the current set of plugin scrobblers +// refreshPluginScrobblers updates the pluginScrobblers map to match the current set of plugin scrobblers. +// The buffered scrobblers use a loader function to dynamically get the current plugin instance, +// so we only need to add/remove scrobblers when plugins are added/removed (not when reloaded). func (p *playTracker) refreshPluginScrobblers() { p.mu.Lock() defer p.mu.Unlock() @@ -148,15 +150,16 @@ func (p *playTracker) refreshPluginScrobblers() { // Build a set of current plugins for faster lookups current := make(map[string]struct{}, len(pluginNames)) - // Process additions - add new plugins + // Process additions - add new plugins with a loader that dynamically fetches the current instance for _, name := range pluginNames { current[name] = struct{}{} - // Only create a new scrobbler if it doesn't exist if _, exists := p.pluginScrobblers[name]; !exists { - s, ok := p.pluginLoader.LoadScrobbler(name) - if ok && s != nil { - p.pluginScrobblers[name] = newBufferedScrobbler(p.ds, s, name) - } + // Capture the name for the closure + pluginName := name + loader := p.pluginLoader + p.pluginScrobblers[name] = newBufferedScrobblerWithLoader(p.ds, name, func() (Scrobbler, bool) { + return loader.LoadScrobbler(pluginName) + }) } } diff --git a/core/scrobbler/play_tracker_test.go b/core/scrobbler/play_tracker_test.go index 839590e6b..f7edecdfd 100644 --- a/core/scrobbler/play_tracker_test.go +++ b/core/scrobbler/play_tracker_test.go @@ -432,6 +432,122 @@ var _ = Describe("PlayTracker", func() { Expect(pTracker.pluginScrobblers).NotTo(HaveKey("plugin1")) }) }) + + Describe("Plugin reload (config update) behavior", func() { + var mockPlugin *mockPluginLoader + var pTracker *playTracker + var originalScrobbler *fakeScrobbler + var reloadedScrobbler *fakeScrobbler + + BeforeEach(func() { + ctx = GinkgoT().Context() + ctx = request.WithUser(ctx, model.User{ID: "u-1"}) + ctx = request.WithPlayer(ctx, model.Player{ScrobbleEnabled: true}) + ds = &tests.MockDataStore{} + + // Setup initial plugin scrobbler + originalScrobbler = &fakeScrobbler{Authorized: true} + reloadedScrobbler = &fakeScrobbler{Authorized: true} + + mockPlugin = &mockPluginLoader{ + names: []string{"plugin1"}, + scrobblers: map[string]Scrobbler{"plugin1": originalScrobbler}, + } + + // Create tracker - this will create buffered scrobblers with loaders + pTracker = newPlayTracker(ds, events.GetBroker(), mockPlugin) + + // Trigger initial plugin registration + pTracker.refreshPluginScrobblers() + }) + + AfterEach(func() { + pTracker.stopNowPlayingWorker() + }) + + It("uses the new plugin instance after reload (simulating config update)", func() { + // First call should use the original scrobbler + scrobblers := pTracker.getActiveScrobblers() + pluginScr := scrobblers["plugin1"] + Expect(pluginScr).ToNot(BeNil()) + + err := pluginScr.NowPlaying(ctx, "u-1", &track, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(originalScrobbler.GetNowPlayingCalled()).To(BeTrue()) + Expect(reloadedScrobbler.GetNowPlayingCalled()).To(BeFalse()) + + // Simulate plugin reload (config update): replace the scrobbler in the loader + // This is what happens when UpdatePluginConfig is called - the plugin manager + // unloads the old plugin and loads a new instance + mockPlugin.mu.Lock() + mockPlugin.scrobblers["plugin1"] = reloadedScrobbler + mockPlugin.mu.Unlock() + + // Reset call tracking + originalScrobbler.nowPlayingCalled.Store(false) + + // Get scrobblers again - should still return the same buffered scrobbler + // but subsequent calls should use the new plugin instance via the loader + scrobblers = pTracker.getActiveScrobblers() + pluginScr = scrobblers["plugin1"] + + err = pluginScr.NowPlaying(ctx, "u-1", &track, 0) + Expect(err).ToNot(HaveOccurred()) + + // The new scrobbler should be called, not the old one + Expect(reloadedScrobbler.GetNowPlayingCalled()).To(BeTrue()) + Expect(originalScrobbler.GetNowPlayingCalled()).To(BeFalse()) + }) + + It("handles plugin becoming unavailable temporarily", func() { + // First verify plugin works + scrobblers := pTracker.getActiveScrobblers() + pluginScr := scrobblers["plugin1"] + + err := pluginScr.NowPlaying(ctx, "u-1", &track, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(originalScrobbler.GetNowPlayingCalled()).To(BeTrue()) + + // Simulate plugin becoming unavailable (e.g., during reload) + mockPlugin.mu.Lock() + delete(mockPlugin.scrobblers, "plugin1") + mockPlugin.mu.Unlock() + + originalScrobbler.nowPlayingCalled.Store(false) + + // NowPlaying should return error when plugin unavailable + err = pluginScr.NowPlaying(ctx, "u-1", &track, 0) + Expect(err).To(HaveOccurred()) + Expect(originalScrobbler.GetNowPlayingCalled()).To(BeFalse()) + + // Simulate plugin becoming available again + mockPlugin.mu.Lock() + mockPlugin.scrobblers["plugin1"] = reloadedScrobbler + mockPlugin.mu.Unlock() + + // Should work again with new instance + err = pluginScr.NowPlaying(ctx, "u-1", &track, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(reloadedScrobbler.GetNowPlayingCalled()).To(BeTrue()) + }) + + It("IsAuthorized uses the current plugin instance", func() { + scrobblers := pTracker.getActiveScrobblers() + pluginScr := scrobblers["plugin1"] + + // Original is authorized + Expect(pluginScr.IsAuthorized(ctx, "u-1")).To(BeTrue()) + + // Replace with unauthorized scrobbler + unauthorizedScrobbler := &fakeScrobbler{Authorized: false} + mockPlugin.mu.Lock() + mockPlugin.scrobblers["plugin1"] = unauthorizedScrobbler + mockPlugin.mu.Unlock() + + // Should reflect the new scrobbler's authorization status + Expect(pluginScr.IsAuthorized(ctx, "u-1")).To(BeFalse()) + }) + }) }) type fakeScrobbler struct {