fix: implement dynamic loading for buffered scrobbler plugins

Signed-off-by: Deluan <deluan@navidrome.org>

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

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

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 {