From 0127b8c607e8979ba1ec5c6bca1a95253bd09b7e Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 15:37:48 -0500 Subject: [PATCH] refactor(plugins): move PlaylistGenerator orchestrator into plugin closers Move the PlaylistGenerator orchestrator from a separate map on the Manager into each plugin's closers list, giving it the same lifecycle as other host services (Scheduler, TaskQueue, WebSocket). The orchestrator now implements io.Closer and is created during plugin load, so it starts/stops automatically with the plugin. This removes the playlistGenerators map, the startPlaylistGenerators() method, and special-case cleanup code from both Stop() and unloadPlugin(), simplifying reload and unload paths. --- plugins/manager.go | 48 +++--------------------------- plugins/manager_loader.go | 8 +++++ plugins/playlist_generator.go | 5 ++-- plugins/playlist_generator_test.go | 35 ++++++++++++++++------ plugins/plugins_suite_test.go | 9 +++--- 5 files changed, 45 insertions(+), 60 deletions(-) diff --git a/plugins/manager.go b/plugins/manager.go index 4bf44928a..0c7c91ed8 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -66,9 +66,6 @@ type Manager struct { ds model.DataStore broker events.Broker metrics PluginMetricsRecorder - - // Playlist generator orchestrators (keyed by plugin name) - playlistGenerators map[string]*playlistGeneratorOrchestrator } // GetManager returns a singleton instance of the plugin manager. @@ -76,11 +73,10 @@ type Manager struct { func GetManager(ds model.DataStore, broker events.Broker, m PluginMetricsRecorder) *Manager { return singleton.GetInstance(func() *Manager { return &Manager{ - ds: ds, - broker: broker, - metrics: m, - plugins: make(map[string]*plugin), - playlistGenerators: make(map[string]*playlistGeneratorOrchestrator), + ds: ds, + broker: broker, + metrics: m, + plugins: make(map[string]*plugin), } }) } @@ -169,9 +165,6 @@ func (m *Manager) Start(ctx context.Context) error { return fmt.Errorf("loading enabled plugins: %w", err) } - // Start playlist generators for plugins with the PlaylistGenerator capability - m.startPlaylistGenerators(ctx) - // Start file watcher if auto-reload is enabled if conf.Server.Plugins.AutoReload { if err := m.startWatcher(); err != nil { @@ -183,21 +176,6 @@ func (m *Manager) Start(ctx context.Context) error { return nil } -// startPlaylistGenerators starts orchestrators for all plugins with the PlaylistGenerator capability. -func (m *Manager) startPlaylistGenerators(ctx context.Context) { - m.mu.Lock() - defer m.mu.Unlock() - - for name, p := range m.plugins { - if !hasCapability(p.capabilities, CapabilityPlaylistGenerator) { - continue - } - orch := newPlaylistGeneratorOrchestrator(ctx, name, p, m.ds) - m.playlistGenerators[name] = orch - go orch.run() - } -} - // Stop shuts down the plugin manager and releases all resources. func (m *Manager) Stop() error { // Mark as stopped first to prevent new operations @@ -208,15 +186,6 @@ func (m *Manager) Stop() error { m.cancel() } - // Stop all playlist generator orchestrators - m.mu.Lock() - orchestrators := m.playlistGenerators - m.playlistGenerators = make(map[string]*playlistGeneratorOrchestrator) - m.mu.Unlock() - for _, orch := range orchestrators { - orch.stop() - } - // Stop file watcher m.stopWatcher() @@ -588,17 +557,8 @@ func (m *Manager) unloadPlugin(name string) error { return fmt.Errorf("plugin %q not found", name) } delete(m.plugins, name) - - // Extract playlist generator orchestrator under lock - orch := m.playlistGenerators[name] - delete(m.playlistGenerators, name) m.mu.Unlock() - // Stop playlist generator orchestrator outside lock - if orch != nil { - orch.stop() - } - // Run cleanup functions err := plugin.Close() if err != nil { diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index ccda9e4cb..e394911f7 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -391,6 +391,14 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { // Call plugin init function callPluginInit(ctx, m.plugins[p.ID]) + // Start PlaylistGenerator orchestrator if capability is detected + loadedPlugin := m.plugins[p.ID] + if hasCapability(loadedPlugin.capabilities, CapabilityPlaylistGenerator) { + orch := newPlaylistGeneratorOrchestrator(m.ctx, p.ID, loadedPlugin, m.ds) + loadedPlugin.closers = append(loadedPlugin.closers, orch) + go orch.run() + } + return nil } diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index c933f961a..64766bb3a 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -262,8 +262,9 @@ func (o *playlistGeneratorOrchestrator) stopAllTimers() { } } -// stop cancels the context and waits for the worker goroutine to finish. -func (o *playlistGeneratorOrchestrator) stop() { +// Close cancels the context and waits for the worker goroutine to finish. +func (o *playlistGeneratorOrchestrator) Close() error { o.cancel() <-o.done + return nil } diff --git a/plugins/playlist_generator_test.go b/plugins/playlist_generator_test.go index dc4187952..2086e0436 100644 --- a/plugins/playlist_generator_test.go +++ b/plugins/playlist_generator_test.go @@ -12,6 +12,22 @@ import ( . "github.com/onsi/gomega" ) +// findOrchestrator finds the playlistGeneratorOrchestrator in a plugin's closers. +func findOrchestrator(m *Manager, pluginName string) *playlistGeneratorOrchestrator { + m.mu.RLock() + defer m.mu.RUnlock() + p, ok := m.plugins[pluginName] + if !ok { + return nil + } + for _, c := range p.closers { + if orch, ok := c.(*playlistGeneratorOrchestrator); ok { + return orch + } + } + return nil +} + var _ = Describe("PlaylistGenerator", Ordered, func() { var ( pgManager *Manager @@ -33,9 +49,9 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }) }) - Describe("startPlaylistGenerators", func() { + Describe("orchestrator lifecycle", func() { It("creates an orchestrator for the plugin", func() { - Expect(pgManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + Expect(findOrchestrator(pgManager, "test-playlist-generator")).ToNot(BeNil()) }) It("discovers and syncs playlists from the plugin", func() { @@ -89,7 +105,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }, "test-playlist-generator"+PackageExtension) // Should still have the orchestrator (error is logged, not fatal) - Expect(errManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + Expect(findOrchestrator(errManager, "test-playlist-generator")).ToNot(BeNil()) // But no playlists created errPlsRepo := errManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) @@ -111,7 +127,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }, "test-playlist-generator"+PackageExtension) // Should still have the orchestrator - Expect(notFoundManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + Expect(findOrchestrator(notFoundManager, "test-playlist-generator")).ToNot(BeNil()) // No playlists should be created (all returned NotFound) notFoundPlsRepo := notFoundManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) @@ -120,7 +136,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }, "500ms").Should(Equal(0)) // No refresh timers should be scheduled for NotFound playlists - orch := notFoundManager.playlistGenerators["test-playlist-generator"] + orch := findOrchestrator(notFoundManager, "test-playlist-generator") Eventually(func() int32 { return orch.refreshTimerCount.Load() }).Should(Equal(int32(0))) @@ -136,8 +152,8 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }, }, "test-playlist-generator"+PackageExtension) - Expect(retryManager.playlistGenerators).To(HaveKey("test-playlist-generator")) - orch := retryManager.playlistGenerators["test-playlist-generator"] + orch := findOrchestrator(retryManager, "test-playlist-generator") + Expect(orch).ToNot(BeNil()) // retryInterval should be stored from the response Eventually(func() time.Duration { @@ -162,11 +178,12 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { stopManager, _ := createTestManagerWithPlugins(nil, "test-playlist-generator"+PackageExtension, ) - Expect(stopManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + Expect(findOrchestrator(stopManager, "test-playlist-generator")).ToNot(BeNil()) err := stopManager.Stop() Expect(err).ToNot(HaveOccurred()) - Expect(stopManager.playlistGenerators).To(BeEmpty()) + // After Stop(), the plugin is unloaded so findOrchestrator returns nil + Expect(findOrchestrator(stopManager, "test-playlist-generator")).To(BeNil()) }) }) }) diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 1e53db664..41adbc133 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -143,11 +143,10 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s // Create and start manager manager := &Manager{ - plugins: make(map[string]*plugin), - playlistGenerators: make(map[string]*playlistGeneratorOrchestrator), - ds: dataStore, - metrics: metrics, - subsonicRouter: http.NotFoundHandler(), // Stub router for tests + plugins: make(map[string]*plugin), + ds: dataStore, + metrics: metrics, + subsonicRouter: http.NotFoundHandler(), // Stub router for tests } err = manager.Start(GinkgoT().Context()) Expect(err).ToNot(HaveOccurred())