From 4f47022f7f081de42a6e899ca5eb56eeb876fbed Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:42:23 -0500 Subject: [PATCH 01/24] feat: add plugin_id and plugin_playlist_id columns to playlist table --- ...260305051806_add_plugin_playlist_fields.go | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 db/migrations/20260305051806_add_plugin_playlist_fields.go diff --git a/db/migrations/20260305051806_add_plugin_playlist_fields.go b/db/migrations/20260305051806_add_plugin_playlist_fields.go new file mode 100644 index 000000000..518bab0ea --- /dev/null +++ b/db/migrations/20260305051806_add_plugin_playlist_fields.go @@ -0,0 +1,38 @@ +package migrations + +import ( + "context" + "database/sql" + + "github.com/pressly/goose/v3" +) + +func init() { + goose.AddMigrationContext(upAddPluginPlaylistFields, downAddPluginPlaylistFields) +} + +func upAddPluginPlaylistFields(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_id VARCHAR(255) DEFAULT '';`) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_playlist_id VARCHAR(255) DEFAULT '';`) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS idx_playlist_plugin ON playlist(plugin_id, plugin_playlist_id) WHERE plugin_id != '';`) + return err +} + +func downAddPluginPlaylistFields(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, `DROP INDEX IF EXISTS idx_playlist_plugin;`) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `ALTER TABLE playlist DROP COLUMN plugin_playlist_id;`) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `ALTER TABLE playlist DROP COLUMN plugin_id;`) + return err +} From 4600ed7c28d43bf5996d2198f8ce54c7f147b4fa Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:44:37 -0500 Subject: [PATCH 02/24] feat: add PluginID and PluginPlaylistID fields to Playlist model --- model/playlist.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/model/playlist.go b/model/playlist.go index e2f93993d..0bcf20ad0 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -30,12 +30,20 @@ type Playlist struct { // SmartPlaylist attributes Rules *criteria.Criteria `structs:"rules" json:"rules"` EvaluatedAt *time.Time `structs:"evaluated_at" json:"evaluatedAt"` + + // Plugin playlist attributes + PluginID string `structs:"plugin_id" json:"pluginId,omitempty"` + PluginPlaylistID string `structs:"plugin_playlist_id" json:"pluginPlaylistId,omitempty"` } func (pls Playlist) IsSmartPlaylist() bool { return pls.Rules != nil && pls.Rules.Expression != nil } +func (pls Playlist) IsPluginPlaylist() bool { + return pls.PluginID != "" +} + func (pls Playlist) MediaFiles() MediaFiles { if len(pls.Tracks) == 0 { return nil From bd0af7fd3840483b18b5b5c717b486080b4804cd Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:49:12 -0500 Subject: [PATCH 03/24] feat: add read-only guards for plugin-managed playlists Plugin playlists (PluginID != "") are now treated as read-only, like smart playlists. Track mutations (add/remove/reorder) and track replacement via Create are blocked with ErrNotAuthorized. --- core/playlists/playlists.go | 4 +-- core/playlists/playlists_test.go | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index a0086cd2d..cebb27b77 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -112,7 +112,7 @@ func (s *playlists) Create(ctx context.Context, playlistId string, name string, if err != nil { return err } - if pls.IsSmartPlaylist() { + if pls.IsSmartPlaylist() || pls.IsPluginPlaylist() { return model.ErrNotAuthorized } if !usr.IsAdmin && pls.OwnerID != usr.ID { @@ -218,7 +218,7 @@ func (s *playlists) checkTracksEditable(ctx context.Context, playlistID string) if err != nil { return nil, err } - if pls.IsSmartPlaylist() { + if pls.IsSmartPlaylist() || pls.IsPluginPlaylist() { return nil, model.ErrNotAuthorized } return pls, nil diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index 52d5c88d8..12ff44010 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -80,6 +80,8 @@ var _ = Describe("Playlists", func() { "pls-2": {ID: "pls-2", Name: "Other's", OwnerID: "other-user"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-plugin": {ID: "pls-plugin", Name: "Plugin Playlist", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix"}, } ps = playlists.NewPlaylists(ds, core.NewImageUploadService()) }) @@ -125,6 +127,12 @@ var _ = Describe("Playlists", func() { _, err := ps.Create(ctx, "pls-smart", "", []string{"song-1"}) Expect(err).To(MatchError(model.ErrNotAuthorized)) }) + + It("denies replacing tracks on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + _, err := ps.Create(ctx, "pls-plugin", "", []string{"song-1"}) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) }) Describe("Update", func() { @@ -137,6 +145,8 @@ var _ = Describe("Playlists", func() { "pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-plugin": {ID: "pls-plugin", Name: "Plugin Playlist", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix"}, } mockPlsRepo.TracksRepo = mockTracks ps = playlists.NewPlaylists(ds, core.NewImageUploadService()) @@ -182,6 +192,18 @@ var _ = Describe("Playlists", func() { Expect(err).To(MatchError(model.ErrNotAuthorized)) }) + It("denies adding tracks to a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.Update(ctx, "pls-plugin", nil, nil, nil, []string{"song-1"}, nil) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + + It("denies removing tracks from a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.Update(ctx, "pls-plugin", nil, nil, nil, nil, []int{0}) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + It("allows metadata updates on a smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) newName := "Updated Smart" @@ -199,6 +221,8 @@ var _ = Describe("Playlists", func() { "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-plugin": {ID: "pls-plugin", Name: "Plugin Playlist", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix"}, "pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"}, } mockPlsRepo.TracksRepo = mockTracks @@ -232,6 +256,12 @@ var _ = Describe("Playlists", func() { Expect(err).To(MatchError(model.ErrNotAuthorized)) }) + It("denies editing plugin playlists", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + _, err := ps.AddTracks(ctx, "pls-plugin", []string{"song-1"}) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + It("returns error when playlist not found", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) _, err := ps.AddTracks(ctx, "nonexistent", []string{"song-1"}) @@ -248,6 +278,8 @@ var _ = Describe("Playlists", func() { "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-plugin": {ID: "pls-plugin", Name: "Plugin Playlist", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix"}, } mockPlsRepo.TracksRepo = mockTracks ps = playlists.NewPlaylists(ds, core.NewImageUploadService()) @@ -266,6 +298,12 @@ var _ = Describe("Playlists", func() { Expect(err).To(MatchError(model.ErrNotAuthorized)) }) + It("denies on plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.RemoveTracks(ctx, "pls-plugin", []string{"track-1"}) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + It("denies non-owner", func() { ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false}) err := ps.RemoveTracks(ctx, "pls-1", []string{"track-1"}) @@ -282,6 +320,8 @@ var _ = Describe("Playlists", func() { "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-plugin": {ID: "pls-plugin", Name: "Plugin Playlist", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix"}, } mockPlsRepo.TracksRepo = mockTracks ps = playlists.NewPlaylists(ds, core.NewImageUploadService()) @@ -299,6 +339,12 @@ var _ = Describe("Playlists", func() { err := ps.ReorderTrack(ctx, "pls-smart", 1, 3) Expect(err).To(MatchError(model.ErrNotAuthorized)) }) + + It("denies on plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.ReorderTrack(ctx, "pls-plugin", 1, 3) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) }) Describe("SetImage", func() { From 520a3e9c55b752bff18e0c81ce5f863ac6847a55 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:50:00 -0500 Subject: [PATCH 04/24] feat: define PlaylistGenerator plugin capability interface --- plugins/capabilities/playlist_generator.go | 58 ++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 plugins/capabilities/playlist_generator.go diff --git a/plugins/capabilities/playlist_generator.go b/plugins/capabilities/playlist_generator.go new file mode 100644 index 000000000..68964c514 --- /dev/null +++ b/plugins/capabilities/playlist_generator.go @@ -0,0 +1,58 @@ +package capabilities + +// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// personalized recommendations). Plugins implementing this capability expose two +// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching +// the heavy payload (tracks, metadata). +// +//nd:capability name=playlistgenerator required=true +type PlaylistGenerator interface { + // GetPlaylists returns the list of playlists this plugin provides. + //nd:export name=nd_playlist_generator_get_playlists + GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + + // GetPlaylist returns the full data for a single playlist (tracks, metadata). + //nd:export name=nd_playlist_generator_get_playlist + GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) +} + +// GetPlaylistsRequest is the request for GetPlaylists. +type GetPlaylistsRequest struct{} + +// GetPlaylistsResponse is the response for GetPlaylists. +type GetPlaylistsResponse struct { + // Playlists is the list of playlists provided by this plugin. + Playlists []PlaylistInfo `json:"playlists"` + // RefreshInterval is the number of seconds until the next GetPlaylists call. + // 0 means never re-discover. + RefreshInterval int64 `json:"refreshInterval"` +} + +// PlaylistInfo identifies a plugin playlist and its target user. +type PlaylistInfo struct { + // ID is the plugin-scoped unique identifier for this playlist. + ID string `json:"id"` + // OwnerUserID is the Navidrome user ID that owns this playlist. + OwnerUserID string `json:"ownerUserId"` +} + +// GetPlaylistRequest is the request for GetPlaylist. +type GetPlaylistRequest struct { + // ID is the plugin-scoped playlist ID. + ID string `json:"id"` +} + +// GetPlaylistResponse is the response for GetPlaylist. +type GetPlaylistResponse struct { + // Name is the display name of the playlist. + Name string `json:"name"` + // Description is an optional description for the playlist. + Description string `json:"description,omitempty"` + // CoverArtURL is an optional external URL for the playlist cover art. + CoverArtURL string `json:"coverArtUrl,omitempty"` + // Tracks is the list of songs in the playlist, using SongRef for matching. + Tracks []SongRef `json:"tracks"` + // ValidUntil is a unix timestamp indicating when this playlist data expires. + // 0 means static (never refresh). + ValidUntil int64 `json:"validUntil"` +} From 002c7612bfa9acc60d0959a79bd9334941fa1e22 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:50:45 -0500 Subject: [PATCH 05/24] feat: register PlaylistGenerator capability with WASM function names --- plugins/playlist_generator.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 plugins/playlist_generator.go diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go new file mode 100644 index 000000000..3f8ef6b2b --- /dev/null +++ b/plugins/playlist_generator.go @@ -0,0 +1,16 @@ +package plugins + +const ( + CapabilityPlaylistGenerator Capability = "PlaylistGenerator" + + FuncPlaylistGeneratorGetPlaylists = "nd_playlist_generator_get_playlists" + FuncPlaylistGeneratorGetPlaylist = "nd_playlist_generator_get_playlist" +) + +func init() { + registerCapability( + CapabilityPlaylistGenerator, + FuncPlaylistGeneratorGetPlaylists, + FuncPlaylistGeneratorGetPlaylist, + ) +} From 22ae5bac54586b34b4ee913eac48868fd87ec9cb Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:52:56 -0500 Subject: [PATCH 06/24] feat: implement PlaylistGenerator orchestration logic Orchestrates plugin playlist lifecycle: discovery via GetPlaylists, data fetch via GetPlaylist, track matching via core/matcher, DB upsert, and timer-based refresh (ValidUntil per playlist, RefreshInterval for re-discovery). --- plugins/playlist_generator.go | 145 ++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index 3f8ef6b2b..eb6e212c9 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -1,5 +1,17 @@ package plugins +import ( + "context" + "fmt" + "time" + + "github.com/navidrome/navidrome/core/matcher" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" + "github.com/navidrome/navidrome/plugins/capabilities" +) + const ( CapabilityPlaylistGenerator Capability = "PlaylistGenerator" @@ -14,3 +26,136 @@ func init() { FuncPlaylistGeneratorGetPlaylist, ) } + +// playlistGeneratorOrchestrator manages playlist generation for a single plugin. +type playlistGeneratorOrchestrator struct { + pluginName string + plugin *plugin + ds model.DataStore + matcher *matcher.Matcher + refreshTimers map[string]*time.Timer // keyed by playlist DB ID + discoveryTimer *time.Timer +} + +func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.DataStore) *playlistGeneratorOrchestrator { + return &playlistGeneratorOrchestrator{ + pluginName: pluginName, + plugin: p, + ds: ds, + matcher: matcher.New(ds), + refreshTimers: make(map[string]*time.Timer), + } +} + +// discoverAndSync calls GetPlaylists, then GetPlaylist for each, matches tracks, and upserts. +func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { + resp, err := callPluginFunction[capabilities.GetPlaylistsRequest, capabilities.GetPlaylistsResponse]( + ctx, o.plugin, FuncPlaylistGeneratorGetPlaylists, capabilities.GetPlaylistsRequest{}, + ) + if err != nil { + log.Error(ctx, "Failed to call GetPlaylists", "plugin", o.pluginName, err) + return + } + + for _, info := range resp.Playlists { + dbID := id.NewHash(o.pluginName, info.ID, info.OwnerUserID) + o.syncPlaylist(ctx, info, dbID) + } + + // Schedule re-discovery if RefreshInterval > 0 + if resp.RefreshInterval > 0 { + o.scheduleDiscovery(ctx, time.Duration(resp.RefreshInterval)*time.Second) + } +} + +// syncPlaylist calls GetPlaylist, matches tracks, and upserts the playlist in the DB. +func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info capabilities.PlaylistInfo, dbID string) { + resp, err := callPluginFunction[capabilities.GetPlaylistRequest, capabilities.GetPlaylistResponse]( + ctx, o.plugin, FuncPlaylistGeneratorGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, + ) + if err != nil { + log.Error(ctx, "Failed to call GetPlaylist", "plugin", o.pluginName, "playlistID", info.ID, err) + return + } + + // Convert SongRef → agents.Song and match against library + songs := songRefsToAgentSongs(resp.Tracks) + matched, err := o.matcher.MatchSongsToLibrary(ctx, songs, len(songs)) + if err != nil { + log.Error(ctx, "Failed to match songs to library", "plugin", o.pluginName, "playlistID", info.ID, err) + return + } + + // Build playlist model + pls := &model.Playlist{ + ID: dbID, + Name: resp.Name, + Comment: resp.Description, + OwnerID: info.OwnerUserID, + Public: false, + ExternalImageURL: resp.CoverArtURL, + PluginID: o.pluginName, + PluginPlaylistID: info.ID, + } + + // Set tracks from matched media files + tracks := make(model.PlaylistTracks, len(matched)) + for i, mf := range matched { + tracks[i] = model.PlaylistTrack{ + ID: fmt.Sprintf("%d", i+1), + MediaFileID: mf.ID, + PlaylistID: dbID, + MediaFile: mf, + } + } + pls.Tracks = tracks + + // Upsert via repository + plsRepo := o.ds.Playlist(ctx) + if err := plsRepo.Put(pls); err != nil { + log.Error(ctx, "Failed to upsert plugin playlist", "plugin", o.pluginName, "playlistID", info.ID, err) + return + } + + log.Info(ctx, "Synced plugin playlist", "plugin", o.pluginName, "playlistID", info.ID, + "name", resp.Name, "tracks", len(matched), "owner", info.OwnerUserID) + + // Schedule refresh if ValidUntil > 0 + if resp.ValidUntil > 0 { + validUntil := time.Unix(resp.ValidUntil, 0) + delay := time.Until(validUntil) + if delay <= 0 { + delay = 1 * time.Second // Already expired, refresh soon + } + o.schedulePlaylistRefresh(ctx, info, dbID, delay) + } +} + +func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(_ context.Context, info capabilities.PlaylistInfo, dbID string, delay time.Duration) { + // Cancel existing timer if any + if timer, ok := o.refreshTimers[dbID]; ok { + timer.Stop() + } + o.refreshTimers[dbID] = time.AfterFunc(delay, func() { + o.syncPlaylist(context.Background(), info, dbID) + }) +} + +func (o *playlistGeneratorOrchestrator) scheduleDiscovery(_ context.Context, delay time.Duration) { + if o.discoveryTimer != nil { + o.discoveryTimer.Stop() + } + o.discoveryTimer = time.AfterFunc(delay, func() { + o.discoverAndSync(context.Background()) + }) +} + +// stop cancels all timers. +func (o *playlistGeneratorOrchestrator) stop() { + if o.discoveryTimer != nil { + o.discoveryTimer.Stop() + } + for _, timer := range o.refreshTimers { + timer.Stop() + } +} From 4908dae21fe85d470645f48320d42c28985e3cde Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 07:57:55 -0500 Subject: [PATCH 07/24] feat: integrate PlaylistGenerator orchestration into plugin manager lifecycle Start playlist generator orchestrators after loading enabled plugins. Stop them on manager shutdown and when individual plugins are unloaded. --- plugins/manager.go | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/plugins/manager.go b/plugins/manager.go index 0c7c91ed8..c1f15f66c 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -66,6 +66,9 @@ 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. @@ -73,10 +76,11 @@ 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), + ds: ds, + broker: broker, + metrics: m, + plugins: make(map[string]*plugin), + playlistGenerators: make(map[string]*playlistGeneratorOrchestrator), } }) } @@ -165,6 +169,9 @@ 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 { @@ -176,6 +183,21 @@ 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.RLock() + defer m.mu.RUnlock() + + for name, p := range m.plugins { + if !hasCapability(p.capabilities, CapabilityPlaylistGenerator) { + continue + } + orch := newPlaylistGeneratorOrchestrator(name, p, m.ds) + m.playlistGenerators[name] = orch + go orch.discoverAndSync(ctx) + } +} + // Stop shuts down the plugin manager and releases all resources. func (m *Manager) Stop() error { // Mark as stopped first to prevent new operations @@ -186,6 +208,12 @@ func (m *Manager) Stop() error { m.cancel() } + // Stop all playlist generator orchestrators + for name, orch := range m.playlistGenerators { + orch.stop() + delete(m.playlistGenerators, name) + } + // Stop file watcher m.stopWatcher() @@ -559,6 +587,12 @@ func (m *Manager) unloadPlugin(name string) error { delete(m.plugins, name) m.mu.Unlock() + // Stop playlist generator orchestrator if any + if orch, ok := m.playlistGenerators[name]; ok { + orch.stop() + delete(m.playlistGenerators, name) + } + // Run cleanup functions err := plugin.Close() if err != nil { From dedaf8e64a22ab873a0974635eaec180ec9fc910 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 08:02:31 -0500 Subject: [PATCH 08/24] feat: generate PDK code for PlaylistGenerator capability Generated via ndpgen: Go PDK wrappers, Rust PDK wrappers, and XTP schema for non-Go plugins. --- plugins/capabilities/playlist_generator.yaml | 120 +++++++++++++ .../go/playlistgenerator/playlistgenerator.go | 157 +++++++++++++++++ .../playlistgenerator_stub.go | 92 ++++++++++ .../pdk/rust/nd-pdk-capabilities/src/lib.rs | 1 + .../src/playlistgenerator.rs | 165 ++++++++++++++++++ 5 files changed, 535 insertions(+) create mode 100644 plugins/capabilities/playlist_generator.yaml create mode 100644 plugins/pdk/go/playlistgenerator/playlistgenerator.go create mode 100644 plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go create mode 100644 plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs diff --git a/plugins/capabilities/playlist_generator.yaml b/plugins/capabilities/playlist_generator.yaml new file mode 100644 index 000000000..d17252c89 --- /dev/null +++ b/plugins/capabilities/playlist_generator.yaml @@ -0,0 +1,120 @@ +version: v1-draft +exports: + nd_playlist_generator_get_playlists: + description: GetPlaylists returns the list of playlists this plugin provides. + input: + $ref: '#/components/schemas/GetPlaylistsRequest' + contentType: application/json + output: + $ref: '#/components/schemas/GetPlaylistsResponse' + contentType: application/json + nd_playlist_generator_get_playlist: + description: GetPlaylist returns the full data for a single playlist (tracks, metadata). + input: + $ref: '#/components/schemas/GetPlaylistRequest' + contentType: application/json + output: + $ref: '#/components/schemas/GetPlaylistResponse' + contentType: application/json +components: + schemas: + GetPlaylistRequest: + description: GetPlaylistRequest is the request for GetPlaylist. + properties: + id: + type: string + description: ID is the plugin-scoped playlist ID. + required: + - id + GetPlaylistResponse: + description: GetPlaylistResponse is the response for GetPlaylist. + properties: + name: + type: string + description: Name is the display name of the playlist. + description: + type: string + description: Description is an optional description for the playlist. + coverArtUrl: + type: string + description: CoverArtURL is an optional external URL for the playlist cover art. + tracks: + type: array + description: Tracks is the list of songs in the playlist, using SongRef for matching. + items: + $ref: '#/components/schemas/SongRef' + validUntil: + type: integer + format: int64 + description: |- + ValidUntil is a unix timestamp indicating when this playlist data expires. + 0 means static (never refresh). + required: + - name + - tracks + - validUntil + GetPlaylistsRequest: + description: GetPlaylistsRequest is the request for GetPlaylists. + properties: {} + GetPlaylistsResponse: + description: GetPlaylistsResponse is the response for GetPlaylists. + properties: + playlists: + type: array + description: Playlists is the list of playlists provided by this plugin. + items: + $ref: '#/components/schemas/PlaylistInfo' + refreshInterval: + type: integer + format: int64 + description: |- + RefreshInterval is the number of seconds until the next GetPlaylists call. + 0 means never re-discover. + required: + - playlists + - refreshInterval + PlaylistInfo: + description: PlaylistInfo identifies a plugin playlist and its target user. + properties: + id: + type: string + description: ID is the plugin-scoped unique identifier for this playlist. + ownerUserId: + type: string + description: OwnerUserID is the Navidrome user ID that owns this playlist. + required: + - id + - ownerUserId + SongRef: + description: SongRef is a reference to a song with metadata for matching. + properties: + id: + type: string + description: ID is the internal Navidrome mediafile ID (if known). + name: + type: string + description: Name is the song name. + mbid: + type: string + description: MBID is the MusicBrainz ID for the song. + isrc: + type: string + description: ISRC is the International Standard Recording Code for the song. + artist: + type: string + description: Artist is the artist name. + artistMbid: + type: string + description: ArtistMBID is the MusicBrainz artist ID. + album: + type: string + description: Album is the album name. + albumMbid: + type: string + description: AlbumMBID is the MusicBrainz release ID. + duration: + type: number + format: float + description: Duration is the song duration in seconds. + required: + - name diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator.go b/plugins/pdk/go/playlistgenerator/playlistgenerator.go new file mode 100644 index 000000000..0c8f40428 --- /dev/null +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator.go @@ -0,0 +1,157 @@ +// Code generated by ndpgen. DO NOT EDIT. +// +// This file contains export wrappers for the PlaylistGenerator capability. +// It is intended for use in Navidrome plugins built with TinyGo. +// +//go:build wasip1 + +package playlistgenerator + +import ( + "github.com/navidrome/navidrome/plugins/pdk/go/pdk" +) + +// GetPlaylistRequest is the request for GetPlaylist. +type GetPlaylistRequest struct { + // ID is the plugin-scoped playlist ID. + ID string `json:"id"` +} + +// GetPlaylistResponse is the response for GetPlaylist. +type GetPlaylistResponse struct { + // Name is the display name of the playlist. + Name string `json:"name"` + // Description is an optional description for the playlist. + Description string `json:"description,omitempty"` + // CoverArtURL is an optional external URL for the playlist cover art. + CoverArtURL string `json:"coverArtUrl,omitempty"` + // Tracks is the list of songs in the playlist, using SongRef for matching. + Tracks []SongRef `json:"tracks"` + // ValidUntil is a unix timestamp indicating when this playlist data expires. + // 0 means static (never refresh). + ValidUntil int64 `json:"validUntil"` +} + +// GetPlaylistsRequest is the request for GetPlaylists. +type GetPlaylistsRequest struct { +} + +// GetPlaylistsResponse is the response for GetPlaylists. +type GetPlaylistsResponse struct { + // Playlists is the list of playlists provided by this plugin. + Playlists []PlaylistInfo `json:"playlists"` + // RefreshInterval is the number of seconds until the next GetPlaylists call. + // 0 means never re-discover. + RefreshInterval int64 `json:"refreshInterval"` +} + +// PlaylistInfo identifies a plugin playlist and its target user. +type PlaylistInfo struct { + // ID is the plugin-scoped unique identifier for this playlist. + ID string `json:"id"` + // OwnerUserID is the Navidrome user ID that owns this playlist. + OwnerUserID string `json:"ownerUserId"` +} + +// SongRef is a reference to a song with metadata for matching. +type SongRef struct { + // ID is the internal Navidrome mediafile ID (if known). + ID string `json:"id,omitempty"` + // Name is the song name. + Name string `json:"name"` + // MBID is the MusicBrainz ID for the song. + MBID string `json:"mbid,omitempty"` + // ISRC is the International Standard Recording Code for the song. + ISRC string `json:"isrc,omitempty"` + // Artist is the artist name. + Artist string `json:"artist,omitempty"` + // ArtistMBID is the MusicBrainz artist ID. + ArtistMBID string `json:"artistMbid,omitempty"` + // Album is the album name. + Album string `json:"album,omitempty"` + // AlbumMBID is the MusicBrainz release ID. + AlbumMBID string `json:"albumMbid,omitempty"` + // Duration is the song duration in seconds. + Duration float32 `json:"duration,omitempty"` +} + +// PlaylistGenerator requires all methods to be implemented. +// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// personalized recommendations). Plugins implementing this capability expose two +// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching +// the heavy payload (tracks, metadata). +type PlaylistGenerator interface { + // GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. + GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). + GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) +} // Internal implementation holders +var ( + playlistsImpl func(GetPlaylistsRequest) (GetPlaylistsResponse, error) + playlistImpl func(GetPlaylistRequest) (GetPlaylistResponse, error) +) + +// Register registers a playlistgenerator implementation. +// All methods are required. +func Register(impl PlaylistGenerator) { + playlistsImpl = impl.GetPlaylists + playlistImpl = impl.GetPlaylist +} + +// NotImplementedCode is the standard return code for unimplemented functions. +// The host recognizes this and skips the plugin gracefully. +const NotImplementedCode int32 = -2 + +//go:wasmexport nd_playlist_generator_get_playlists +func _NdPlaylistGeneratorGetPlaylists() int32 { + if playlistsImpl == nil { + // Return standard code - host will skip this plugin gracefully + return NotImplementedCode + } + + var input GetPlaylistsRequest + if err := pdk.InputJSON(&input); err != nil { + pdk.SetError(err) + return -1 + } + + output, err := playlistsImpl(input) + if err != nil { + pdk.SetError(err) + return -1 + } + + if err := pdk.OutputJSON(output); err != nil { + pdk.SetError(err) + return -1 + } + + return 0 +} + +//go:wasmexport nd_playlist_generator_get_playlist +func _NdPlaylistGeneratorGetPlaylist() int32 { + if playlistImpl == nil { + // Return standard code - host will skip this plugin gracefully + return NotImplementedCode + } + + var input GetPlaylistRequest + if err := pdk.InputJSON(&input); err != nil { + pdk.SetError(err) + return -1 + } + + output, err := playlistImpl(input) + if err != nil { + pdk.SetError(err) + return -1 + } + + if err := pdk.OutputJSON(output); err != nil { + pdk.SetError(err) + return -1 + } + + return 0 +} diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go new file mode 100644 index 000000000..83dc9a61b --- /dev/null +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go @@ -0,0 +1,92 @@ +// Code generated by ndpgen. DO NOT EDIT. +// +// This file provides stub implementations for non-WASM platforms. +// It allows Go plugins to compile and run tests outside of WASM, +// but the actual functionality is only available in WASM builds. +// +//go:build !wasip1 + +package playlistgenerator + +// GetPlaylistRequest is the request for GetPlaylist. +type GetPlaylistRequest struct { + // ID is the plugin-scoped playlist ID. + ID string `json:"id"` +} + +// GetPlaylistResponse is the response for GetPlaylist. +type GetPlaylistResponse struct { + // Name is the display name of the playlist. + Name string `json:"name"` + // Description is an optional description for the playlist. + Description string `json:"description,omitempty"` + // CoverArtURL is an optional external URL for the playlist cover art. + CoverArtURL string `json:"coverArtUrl,omitempty"` + // Tracks is the list of songs in the playlist, using SongRef for matching. + Tracks []SongRef `json:"tracks"` + // ValidUntil is a unix timestamp indicating when this playlist data expires. + // 0 means static (never refresh). + ValidUntil int64 `json:"validUntil"` +} + +// GetPlaylistsRequest is the request for GetPlaylists. +type GetPlaylistsRequest struct { +} + +// GetPlaylistsResponse is the response for GetPlaylists. +type GetPlaylistsResponse struct { + // Playlists is the list of playlists provided by this plugin. + Playlists []PlaylistInfo `json:"playlists"` + // RefreshInterval is the number of seconds until the next GetPlaylists call. + // 0 means never re-discover. + RefreshInterval int64 `json:"refreshInterval"` +} + +// PlaylistInfo identifies a plugin playlist and its target user. +type PlaylistInfo struct { + // ID is the plugin-scoped unique identifier for this playlist. + ID string `json:"id"` + // OwnerUserID is the Navidrome user ID that owns this playlist. + OwnerUserID string `json:"ownerUserId"` +} + +// SongRef is a reference to a song with metadata for matching. +type SongRef struct { + // ID is the internal Navidrome mediafile ID (if known). + ID string `json:"id,omitempty"` + // Name is the song name. + Name string `json:"name"` + // MBID is the MusicBrainz ID for the song. + MBID string `json:"mbid,omitempty"` + // ISRC is the International Standard Recording Code for the song. + ISRC string `json:"isrc,omitempty"` + // Artist is the artist name. + Artist string `json:"artist,omitempty"` + // ArtistMBID is the MusicBrainz artist ID. + ArtistMBID string `json:"artistMbid,omitempty"` + // Album is the album name. + Album string `json:"album,omitempty"` + // AlbumMBID is the MusicBrainz release ID. + AlbumMBID string `json:"albumMbid,omitempty"` + // Duration is the song duration in seconds. + Duration float32 `json:"duration,omitempty"` +} + +// PlaylistGenerator requires all methods to be implemented. +// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// personalized recommendations). Plugins implementing this capability expose two +// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching +// the heavy payload (tracks, metadata). +type PlaylistGenerator interface { + // GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. + GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). + GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) +} + +// NotImplementedCode is the standard return code for unimplemented functions. +const NotImplementedCode int32 = -2 + +// Register is a no-op on non-WASM platforms. +// This stub allows code to compile outside of WASM. +func Register(_ PlaylistGenerator) {} diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs index 85375b525..f0f21b0e8 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs @@ -8,6 +8,7 @@ pub mod lifecycle; pub mod lyrics; pub mod metadata; +pub mod playlistgenerator; pub mod scheduler; pub mod scrobbler; pub mod taskworker; diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs new file mode 100644 index 000000000..296fe2854 --- /dev/null +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs @@ -0,0 +1,165 @@ +// Code generated by ndpgen. DO NOT EDIT. +// +// This file contains export wrappers for the PlaylistGenerator capability. +// It is intended for use in Navidrome plugins built with extism-pdk. + +use serde::{Deserialize, Serialize}; + +// Helper functions for skip_serializing_if with numeric types +#[allow(dead_code)] +fn is_zero_i32(value: &i32) -> bool { *value == 0 } +#[allow(dead_code)] +fn is_zero_u32(value: &u32) -> bool { *value == 0 } +#[allow(dead_code)] +fn is_zero_i64(value: &i64) -> bool { *value == 0 } +#[allow(dead_code)] +fn is_zero_u64(value: &u64) -> bool { *value == 0 } +#[allow(dead_code)] +fn is_zero_f32(value: &f32) -> bool { *value == 0.0 } +#[allow(dead_code)] +fn is_zero_f64(value: &f64) -> bool { *value == 0.0 } +/// GetPlaylistRequest is the request for GetPlaylist. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetPlaylistRequest { + /// ID is the plugin-scoped playlist ID. + #[serde(default)] + pub id: String, +} +/// GetPlaylistResponse is the response for GetPlaylist. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetPlaylistResponse { + /// Name is the display name of the playlist. + #[serde(default)] + pub name: String, + /// Description is an optional description for the playlist. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub description: String, + /// CoverArtURL is an optional external URL for the playlist cover art. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub cover_art_url: String, + /// Tracks is the list of songs in the playlist, using SongRef for matching. + #[serde(default)] + pub tracks: Vec, + /// ValidUntil is a unix timestamp indicating when this playlist data expires. + /// 0 means static (never refresh). + #[serde(default)] + pub valid_until: i64, +} +/// GetPlaylistsRequest is the request for GetPlaylists. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetPlaylistsRequest { +} +/// GetPlaylistsResponse is the response for GetPlaylists. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetPlaylistsResponse { + /// Playlists is the list of playlists provided by this plugin. + #[serde(default)] + pub playlists: Vec, + /// RefreshInterval is the number of seconds until the next GetPlaylists call. + /// 0 means never re-discover. + #[serde(default)] + pub refresh_interval: i64, +} +/// PlaylistInfo identifies a plugin playlist and its target user. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PlaylistInfo { + /// ID is the plugin-scoped unique identifier for this playlist. + #[serde(default)] + pub id: String, + /// OwnerUserID is the Navidrome user ID that owns this playlist. + #[serde(default)] + pub owner_user_id: String, +} +/// SongRef is a reference to a song with metadata for matching. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct SongRef { + /// ID is the internal Navidrome mediafile ID (if known). + #[serde(default, skip_serializing_if = "String::is_empty")] + pub id: String, + /// Name is the song name. + #[serde(default)] + pub name: String, + /// MBID is the MusicBrainz ID for the song. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub mbid: String, + /// ISRC is the International Standard Recording Code for the song. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub isrc: String, + /// Artist is the artist name. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub artist: String, + /// ArtistMBID is the MusicBrainz artist ID. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub artist_mbid: String, + /// Album is the album name. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub album: String, + /// AlbumMBID is the MusicBrainz release ID. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub album_mbid: String, + /// Duration is the song duration in seconds. + #[serde(default, skip_serializing_if = "is_zero_f32")] + pub duration: f32, +} + +/// Error represents an error from a capability method. +#[derive(Debug)] +pub struct Error { + pub message: String, +} + +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.message) + } +} + +impl std::error::Error for Error {} + +impl Error { + pub fn new(message: impl Into) -> Self { + Self { message: message.into() } + } +} + +/// PlaylistGenerator requires all methods to be implemented. +/// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +/// personalized recommendations). Plugins implementing this capability expose two +/// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching +/// the heavy payload (tracks, metadata). +pub trait PlaylistGenerator { + /// GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. + fn get_playlists(&self, req: GetPlaylistsRequest) -> Result; + /// GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). + fn get_playlist(&self, req: GetPlaylistRequest) -> Result; +} + +/// Register all exports for the PlaylistGenerator capability. +/// This macro generates the WASM export functions for all trait methods. +#[macro_export] +macro_rules! register_playlistgenerator { + ($plugin_type:ty) => { + #[extism_pdk::plugin_fn] + pub fn nd_playlist_generator_get_playlists( + req: extism_pdk::Json<$crate::playlistgenerator::GetPlaylistsRequest> + ) -> extism_pdk::FnResult> { + let plugin = <$plugin_type>::default(); + let result = $crate::playlistgenerator::PlaylistGenerator::get_playlists(&plugin, req.into_inner())?; + Ok(extism_pdk::Json(result)) + } + #[extism_pdk::plugin_fn] + pub fn nd_playlist_generator_get_playlist( + req: extism_pdk::Json<$crate::playlistgenerator::GetPlaylistRequest> + ) -> extism_pdk::FnResult> { + let plugin = <$plugin_type>::default(); + let result = $crate::playlistgenerator::PlaylistGenerator::get_playlist(&plugin, req.into_inner())?; + Ok(extism_pdk::Json(result)) + } + }; +} From 188584e3fb6182c20664b8037d7c10c55ab56675 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 08:12:12 -0500 Subject: [PATCH 09/24] test(plugins): add PlaylistGenerator integration tests with test WASM plugin Add a test playlist generator plugin and integration tests for the PlaylistGenerator capability. The test plugin exercises GetPlaylists and GetPlaylist WASM functions, and the tests verify orchestration flow including capability detection, playlist discovery/sync, deterministic ID generation, error handling, and graceful stop. Also initialize playlistGenerators map in the test helper to prevent nil map panics when loading plugins with PlaylistGenerator capability. --- plugins/playlist_generator_test.go | 138 ++++++++++++++++++ plugins/plugins_suite_test.go | 9 +- .../testdata/test-playlist-generator/go.mod | 16 ++ .../testdata/test-playlist-generator/go.sum | 14 ++ .../testdata/test-playlist-generator/main.go | 71 +++++++++ .../test-playlist-generator/manifest.json | 6 + 6 files changed, 250 insertions(+), 4 deletions(-) create mode 100644 plugins/playlist_generator_test.go create mode 100644 plugins/testdata/test-playlist-generator/go.mod create mode 100644 plugins/testdata/test-playlist-generator/go.sum create mode 100644 plugins/testdata/test-playlist-generator/main.go create mode 100644 plugins/testdata/test-playlist-generator/manifest.json diff --git a/plugins/playlist_generator_test.go b/plugins/playlist_generator_test.go new file mode 100644 index 000000000..5f474352b --- /dev/null +++ b/plugins/playlist_generator_test.go @@ -0,0 +1,138 @@ +//go:build !windows + +package plugins + +import ( + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("PlaylistGenerator", Ordered, func() { + var ( + pgManager *Manager + mockPlsRepo *tests.MockPlaylistRepo + ) + + BeforeAll(func() { + pgManager, _ = createTestManagerWithPlugins(nil, + "test-playlist-generator"+PackageExtension, + ) + + // Pre-initialize the mock playlist repo to avoid a race with the + // discoverAndSync goroutine that is launched during Start(). + mockDS := pgManager.ds.(*tests.MockDataStore) + if mockDS.MockedPlaylist == nil { + mockDS.MockedPlaylist = tests.CreateMockPlaylistRepo() + } + mockPlsRepo = mockDS.MockedPlaylist.(*tests.MockPlaylistRepo) + }) + + Describe("capability detection", func() { + It("detects the PlaylistGenerator capability", func() { + names := pgManager.PluginNames(string(CapabilityPlaylistGenerator)) + Expect(names).To(ContainElement("test-playlist-generator")) + }) + }) + + Describe("startPlaylistGenerators", func() { + It("creates an orchestrator for the plugin", func() { + Expect(pgManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + }) + + It("discovers and syncs playlists from the plugin", func() { + // The orchestrator runs discoverAndSync in a goroutine on Start(). + // Give it a moment to complete. + Eventually(func() int { + return len(mockPlsRepo.Data) + }).Should(BeNumerically(">=", 2)) + }) + + It("creates playlists with correct fields", func() { + // Check that playlists have the correct plugin fields + Eventually(func() bool { + for _, pls := range mockPlsRepo.Data { + if pls.PluginID == "test-playlist-generator" && pls.PluginPlaylistID == "daily-mix-1" { + return true + } + } + return false + }).Should(BeTrue()) + + // Find the daily-mix-1 playlist and verify its fields + var dailyMix1 *model.Playlist + for _, pls := range mockPlsRepo.Data { + if pls.PluginPlaylistID == "daily-mix-1" { + dailyMix1 = pls + break + } + } + Expect(dailyMix1).ToNot(BeNil()) + Expect(dailyMix1.Name).To(Equal("Daily Mix 1")) + Expect(dailyMix1.Comment).To(Equal("Your personalized daily mix")) + Expect(dailyMix1.ExternalImageURL).To(Equal("https://example.com/cover1.jpg")) + Expect(dailyMix1.OwnerID).To(Equal("user-1")) + Expect(dailyMix1.PluginID).To(Equal("test-playlist-generator")) + Expect(dailyMix1.PluginPlaylistID).To(Equal("daily-mix-1")) + Expect(dailyMix1.Public).To(BeFalse()) + }) + + It("generates deterministic playlist IDs", func() { + expectedID := id.NewHash("test-playlist-generator", "daily-mix-1", "user-1") + Eventually(func() bool { + _, exists := mockPlsRepo.Data[expectedID] + return exists + }).Should(BeTrue()) + }) + + It("creates distinct IDs for different playlists", func() { + id1 := id.NewHash("test-playlist-generator", "daily-mix-1", "user-1") + id2 := id.NewHash("test-playlist-generator", "daily-mix-2", "user-1") + Expect(id1).ToNot(Equal(id2)) + + Eventually(func() bool { + _, exists1 := mockPlsRepo.Data[id1] + _, exists2 := mockPlsRepo.Data[id2] + return exists1 && exists2 + }).Should(BeTrue()) + }) + }) + + Describe("GetPlaylists error handling", func() { + It("handles plugin errors gracefully", func() { + errManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ + "test-playlist-generator": {"error": "service unavailable"}, + }, "test-playlist-generator"+PackageExtension) + + // Should still have the orchestrator (error is logged, not fatal) + Expect(errManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + + // But no playlists created + errDS := errManager.ds.(*tests.MockDataStore) + if errDS.MockedPlaylist == nil { + errDS.MockedPlaylist = tests.CreateMockPlaylistRepo() + } + errPlsRepo := errDS.MockedPlaylist.(*tests.MockPlaylistRepo) + // The orchestrator was started but GetPlaylists returned error, + // so no playlists should be created + Consistently(func() int { + return len(errPlsRepo.Data) + }, "500ms").Should(Equal(0)) + }) + }) + + Describe("stop", func() { + It("stops the orchestrator when the manager stops", func() { + stopManager, _ := createTestManagerWithPlugins(nil, + "test-playlist-generator"+PackageExtension, + ) + Expect(stopManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + + err := stopManager.Stop() + Expect(err).ToNot(HaveOccurred()) + Expect(stopManager.playlistGenerators).To(BeEmpty()) + }) + }) +}) diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 1799ba3ce..62022fd95 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -137,10 +137,11 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s // Create and start manager manager := &Manager{ - plugins: make(map[string]*plugin), - ds: dataStore, - metrics: metrics, - subsonicRouter: http.NotFoundHandler(), // Stub router for tests + plugins: make(map[string]*plugin), + playlistGenerators: make(map[string]*playlistGeneratorOrchestrator), + ds: dataStore, + metrics: metrics, + subsonicRouter: http.NotFoundHandler(), // Stub router for tests } err = manager.Start(GinkgoT().Context()) Expect(err).ToNot(HaveOccurred()) diff --git a/plugins/testdata/test-playlist-generator/go.mod b/plugins/testdata/test-playlist-generator/go.mod new file mode 100644 index 000000000..7539672b5 --- /dev/null +++ b/plugins/testdata/test-playlist-generator/go.mod @@ -0,0 +1,16 @@ +module test-playlist-generator + +go 1.25 + +require github.com/navidrome/navidrome/plugins/pdk/go v0.0.0 + +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/extism/go-pdk v1.1.3 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/objx v0.5.2 // indirect + github.com/stretchr/testify v1.11.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) + +replace github.com/navidrome/navidrome/plugins/pdk/go => ../../pdk/go diff --git a/plugins/testdata/test-playlist-generator/go.sum b/plugins/testdata/test-playlist-generator/go.sum new file mode 100644 index 000000000..af880eb51 --- /dev/null +++ b/plugins/testdata/test-playlist-generator/go.sum @@ -0,0 +1,14 @@ +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/extism/go-pdk v1.1.3 h1:hfViMPWrqjN6u67cIYRALZTZLk/enSPpNKa+rZ9X2SQ= +github.com/extism/go-pdk v1.1.3/go.mod h1:Gz+LIU/YCKnKXhgge8yo5Yu1F/lbv7KtKFkiCSzW/P4= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/plugins/testdata/test-playlist-generator/main.go b/plugins/testdata/test-playlist-generator/main.go new file mode 100644 index 000000000..f4960a051 --- /dev/null +++ b/plugins/testdata/test-playlist-generator/main.go @@ -0,0 +1,71 @@ +// Test playlist generator plugin for Navidrome plugin system integration tests. +package main + +import ( + "fmt" + + "github.com/navidrome/navidrome/plugins/pdk/go/pdk" + pg "github.com/navidrome/navidrome/plugins/pdk/go/playlistgenerator" +) + +func init() { + pg.Register(&testPlaylistGenerator{}) +} + +type testPlaylistGenerator struct{} + +func (t *testPlaylistGenerator) GetPlaylists(_ pg.GetPlaylistsRequest) (pg.GetPlaylistsResponse, error) { + // Check for configured error + errMsg, hasErr := pdk.GetConfig("error") + if hasErr && errMsg != "" { + return pg.GetPlaylistsResponse{}, fmt.Errorf("%s", errMsg) + } + + // Get the owner user ID from config (defaults to "user-1") + ownerID := "user-1" + if id, ok := pdk.GetConfig("owner_id"); ok && id != "" { + ownerID = id + } + + return pg.GetPlaylistsResponse{ + Playlists: []pg.PlaylistInfo{ + {ID: "daily-mix-1", OwnerUserID: ownerID}, + {ID: "daily-mix-2", OwnerUserID: ownerID}, + }, + RefreshInterval: 0, // No re-discovery in tests + }, nil +} + +func (t *testPlaylistGenerator) GetPlaylist(req pg.GetPlaylistRequest) (pg.GetPlaylistResponse, error) { + // Check for configured error + errMsg, hasErr := pdk.GetConfig("get_playlist_error") + if hasErr && errMsg != "" { + return pg.GetPlaylistResponse{}, fmt.Errorf("%s", errMsg) + } + + switch req.ID { + case "daily-mix-1": + return pg.GetPlaylistResponse{ + Name: "Daily Mix 1", + Description: "Your personalized daily mix", + CoverArtURL: "https://example.com/cover1.jpg", + Tracks: []pg.SongRef{ + {Name: "Song A", Artist: "Artist One"}, + {Name: "Song B", Artist: "Artist Two"}, + }, + ValidUntil: 0, // Static, no refresh + }, nil + case "daily-mix-2": + return pg.GetPlaylistResponse{ + Name: "Daily Mix 2", + Tracks: []pg.SongRef{ + {Name: "Song C", Artist: "Artist Three"}, + }, + ValidUntil: 0, + }, nil + default: + return pg.GetPlaylistResponse{}, fmt.Errorf("unknown playlist: %s", req.ID) + } +} + +func main() {} diff --git a/plugins/testdata/test-playlist-generator/manifest.json b/plugins/testdata/test-playlist-generator/manifest.json new file mode 100644 index 000000000..6874d089f --- /dev/null +++ b/plugins/testdata/test-playlist-generator/manifest.json @@ -0,0 +1,6 @@ +{ + "name": "Test Playlist Generator", + "author": "Navidrome Test", + "version": "1.0.0", + "description": "A test playlist generator plugin for integration testing" +} From a8a336de33c69147ebe5cc22335840ca45a03903 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 09:01:33 -0500 Subject: [PATCH 10/24] fix: address race conditions and design issues in PlaylistGenerator Rename PlaylistInfo.OwnerUserID to OwnerUsername so plugins provide usernames instead of internal user IDs, with server-side resolution via FindByUsername. Fix race condition on playlistGenerators map by using write lock in startPlaylistGenerators and moving map access inside the lock in unloadPlugin/Stop. Add context and WaitGroup to the orchestrator for proper cancellation and goroutine tracking. Include owner_id in the plugin playlist unique index. Use SetTracks instead of direct assignment to refresh playlist duration/size/song count stats. --- ...260305051806_add_plugin_playlist_fields.go | 6 +-- plugins/capabilities/playlist_generator.go | 4 +- plugins/capabilities/playlist_generator.yaml | 6 +-- plugins/manager.go | 24 +++++++---- .../go/playlistgenerator/playlistgenerator.go | 4 +- .../playlistgenerator_stub.go | 4 +- .../src/playlistgenerator.rs | 4 +- plugins/playlist_generator.go | 41 +++++++++++++------ plugins/plugins_suite_test.go | 8 +++- .../testdata/test-playlist-generator/main.go | 12 +++--- 10 files changed, 71 insertions(+), 42 deletions(-) diff --git a/db/migrations/20260305051806_add_plugin_playlist_fields.go b/db/migrations/20260305051806_add_plugin_playlist_fields.go index 518bab0ea..9429e19b3 100644 --- a/db/migrations/20260305051806_add_plugin_playlist_fields.go +++ b/db/migrations/20260305051806_add_plugin_playlist_fields.go @@ -12,15 +12,15 @@ func init() { } func upAddPluginPlaylistFields(ctx context.Context, tx *sql.Tx) error { - _, err := tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_id VARCHAR(255) DEFAULT '';`) + _, err := tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_id VARCHAR(255) NOT NULL DEFAULT '';`) if err != nil { return err } - _, err = tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_playlist_id VARCHAR(255) DEFAULT '';`) + _, err = tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_playlist_id VARCHAR(255) NOT NULL DEFAULT '';`) if err != nil { return err } - _, err = tx.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS idx_playlist_plugin ON playlist(plugin_id, plugin_playlist_id) WHERE plugin_id != '';`) + _, err = tx.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS idx_playlist_plugin ON playlist(plugin_id, plugin_playlist_id, owner_id) WHERE plugin_id != '';`) return err } diff --git a/plugins/capabilities/playlist_generator.go b/plugins/capabilities/playlist_generator.go index 68964c514..5f7cb077c 100644 --- a/plugins/capabilities/playlist_generator.go +++ b/plugins/capabilities/playlist_generator.go @@ -32,8 +32,8 @@ type GetPlaylistsResponse struct { type PlaylistInfo struct { // ID is the plugin-scoped unique identifier for this playlist. ID string `json:"id"` - // OwnerUserID is the Navidrome user ID that owns this playlist. - OwnerUserID string `json:"ownerUserId"` + // OwnerUsername is the Navidrome username that owns this playlist. + OwnerUsername string `json:"ownerUsername"` } // GetPlaylistRequest is the request for GetPlaylist. diff --git a/plugins/capabilities/playlist_generator.yaml b/plugins/capabilities/playlist_generator.yaml index d17252c89..2b4f57a70 100644 --- a/plugins/capabilities/playlist_generator.yaml +++ b/plugins/capabilities/playlist_generator.yaml @@ -79,12 +79,12 @@ components: id: type: string description: ID is the plugin-scoped unique identifier for this playlist. - ownerUserId: + ownerUsername: type: string - description: OwnerUserID is the Navidrome user ID that owns this playlist. + description: OwnerUsername is the Navidrome username that owns this playlist. required: - id - - ownerUserId + - ownerUsername SongRef: description: SongRef is a reference to a song with metadata for matching. properties: diff --git a/plugins/manager.go b/plugins/manager.go index c1f15f66c..8cee839e2 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -185,16 +185,16 @@ func (m *Manager) Start(ctx context.Context) error { // startPlaylistGenerators starts orchestrators for all plugins with the PlaylistGenerator capability. func (m *Manager) startPlaylistGenerators(ctx context.Context) { - m.mu.RLock() - defer m.mu.RUnlock() + m.mu.Lock() + defer m.mu.Unlock() for name, p := range m.plugins { if !hasCapability(p.capabilities, CapabilityPlaylistGenerator) { continue } - orch := newPlaylistGeneratorOrchestrator(name, p, m.ds) + orch := newPlaylistGeneratorOrchestrator(name, p, m.ds, ctx) m.playlistGenerators[name] = orch - go orch.discoverAndSync(ctx) + orch.wg.Go(func() { orch.discoverAndSync(orch.ctx) }) } } @@ -209,9 +209,12 @@ func (m *Manager) Stop() error { } // Stop all playlist generator orchestrators - for name, orch := range m.playlistGenerators { + m.mu.Lock() + orchestrators := m.playlistGenerators + m.playlistGenerators = make(map[string]*playlistGeneratorOrchestrator) + m.mu.Unlock() + for _, orch := range orchestrators { orch.stop() - delete(m.playlistGenerators, name) } // Stop file watcher @@ -585,12 +588,15 @@ 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 if any - if orch, ok := m.playlistGenerators[name]; ok { + // Stop playlist generator orchestrator outside lock + if orch != nil { orch.stop() - delete(m.playlistGenerators, name) } // Run cleanup functions diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator.go b/plugins/pdk/go/playlistgenerator/playlistgenerator.go index 0c8f40428..438cff040 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator.go +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator.go @@ -49,8 +49,8 @@ type GetPlaylistsResponse struct { type PlaylistInfo struct { // ID is the plugin-scoped unique identifier for this playlist. ID string `json:"id"` - // OwnerUserID is the Navidrome user ID that owns this playlist. - OwnerUserID string `json:"ownerUserId"` + // OwnerUsername is the Navidrome username that owns this playlist. + OwnerUsername string `json:"ownerUsername"` } // SongRef is a reference to a song with metadata for matching. diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go index 83dc9a61b..b5f95e023 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go @@ -46,8 +46,8 @@ type GetPlaylistsResponse struct { type PlaylistInfo struct { // ID is the plugin-scoped unique identifier for this playlist. ID string `json:"id"` - // OwnerUserID is the Navidrome user ID that owns this playlist. - OwnerUserID string `json:"ownerUserId"` + // OwnerUsername is the Navidrome username that owns this playlist. + OwnerUsername string `json:"ownerUsername"` } // SongRef is a reference to a song with metadata for matching. diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs index 296fe2854..82aa4d3cd 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs @@ -71,9 +71,9 @@ pub struct PlaylistInfo { /// ID is the plugin-scoped unique identifier for this playlist. #[serde(default)] pub id: String, - /// OwnerUserID is the Navidrome user ID that owns this playlist. + /// OwnerUsername is the Navidrome username that owns this playlist. #[serde(default)] - pub owner_user_id: String, + pub owner_username: String, } /// SongRef is a reference to a song with metadata for matching. #[derive(Debug, Clone, Default, Serialize, Deserialize)] diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index eb6e212c9..3ae2a4ee4 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -3,6 +3,7 @@ package plugins import ( "context" "fmt" + "sync" "time" "github.com/navidrome/navidrome/core/matcher" @@ -33,16 +34,22 @@ type playlistGeneratorOrchestrator struct { plugin *plugin ds model.DataStore matcher *matcher.Matcher + ctx context.Context + cancel context.CancelFunc + wg sync.WaitGroup refreshTimers map[string]*time.Timer // keyed by playlist DB ID discoveryTimer *time.Timer } -func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.DataStore) *playlistGeneratorOrchestrator { +func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.DataStore, parentCtx context.Context) *playlistGeneratorOrchestrator { + ctx, cancel := context.WithCancel(parentCtx) return &playlistGeneratorOrchestrator{ pluginName: pluginName, plugin: p, ds: ds, matcher: matcher.New(ds), + ctx: ctx, + cancel: cancel, refreshTimers: make(map[string]*time.Timer), } } @@ -58,8 +65,16 @@ func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { } for _, info := range resp.Playlists { - dbID := id.NewHash(o.pluginName, info.ID, info.OwnerUserID) - o.syncPlaylist(ctx, info, dbID) + // Resolve username to user ID + user, err := o.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) + if err != nil { + log.Error(ctx, "Failed to resolve playlist owner", "plugin", o.pluginName, + "playlistID", info.ID, "username", info.OwnerUsername, err) + continue + } + ownerID := user.ID + dbID := id.NewHash(o.pluginName, info.ID, ownerID) + o.syncPlaylist(ctx, info, dbID, ownerID) } // Schedule re-discovery if RefreshInterval > 0 @@ -69,7 +84,7 @@ func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { } // syncPlaylist calls GetPlaylist, matches tracks, and upserts the playlist in the DB. -func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info capabilities.PlaylistInfo, dbID string) { +func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info capabilities.PlaylistInfo, dbID string, ownerID string) { resp, err := callPluginFunction[capabilities.GetPlaylistRequest, capabilities.GetPlaylistResponse]( ctx, o.plugin, FuncPlaylistGeneratorGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, ) @@ -91,7 +106,7 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c ID: dbID, Name: resp.Name, Comment: resp.Description, - OwnerID: info.OwnerUserID, + OwnerID: ownerID, Public: false, ExternalImageURL: resp.CoverArtURL, PluginID: o.pluginName, @@ -108,7 +123,7 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c MediaFile: mf, } } - pls.Tracks = tracks + pls.SetTracks(tracks) // Upsert via repository plsRepo := o.ds.Playlist(ctx) @@ -118,7 +133,7 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c } log.Info(ctx, "Synced plugin playlist", "plugin", o.pluginName, "playlistID", info.ID, - "name", resp.Name, "tracks", len(matched), "owner", info.OwnerUserID) + "name", resp.Name, "tracks", len(matched), "owner", ownerID) // Schedule refresh if ValidUntil > 0 if resp.ValidUntil > 0 { @@ -127,17 +142,17 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c if delay <= 0 { delay = 1 * time.Second // Already expired, refresh soon } - o.schedulePlaylistRefresh(ctx, info, dbID, delay) + o.schedulePlaylistRefresh(ctx, info, dbID, ownerID, delay) } } -func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(_ context.Context, info capabilities.PlaylistInfo, dbID string, delay time.Duration) { +func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(_ context.Context, info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { // Cancel existing timer if any if timer, ok := o.refreshTimers[dbID]; ok { timer.Stop() } o.refreshTimers[dbID] = time.AfterFunc(delay, func() { - o.syncPlaylist(context.Background(), info, dbID) + o.wg.Go(func() { o.syncPlaylist(o.ctx, info, dbID, ownerID) }) }) } @@ -146,16 +161,18 @@ func (o *playlistGeneratorOrchestrator) scheduleDiscovery(_ context.Context, del o.discoveryTimer.Stop() } o.discoveryTimer = time.AfterFunc(delay, func() { - o.discoverAndSync(context.Background()) + o.wg.Go(func() { o.discoverAndSync(o.ctx) }) }) } -// stop cancels all timers. +// stop cancels the context, stops all timers, and waits for in-flight goroutines. func (o *playlistGeneratorOrchestrator) stop() { + o.cancel() if o.discoveryTimer != nil { o.discoveryTimer.Stop() } for _, timer := range o.refreshTimers { timer.Stop() } + o.wg.Wait() } diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 62022fd95..4ab259173 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -133,7 +133,13 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s mockPluginRepo := tests.CreateMockPluginRepo() mockPluginRepo.Permitted = true mockPluginRepo.SetData(enabledPlugins) - dataStore := &tests.MockDataStore{MockedPlugin: mockPluginRepo} + + // Pre-seed a mock user repo with a default user so that + // PlaylistGenerator's discoverAndSync can resolve usernames. + mockUserRepo := tests.CreateMockUserRepo() + _ = mockUserRepo.Put(&model.User{ID: "user-1", UserName: "admin"}) + + dataStore := &tests.MockDataStore{MockedPlugin: mockPluginRepo, MockedUser: mockUserRepo} // Create and start manager manager := &Manager{ diff --git a/plugins/testdata/test-playlist-generator/main.go b/plugins/testdata/test-playlist-generator/main.go index f4960a051..830bc0845 100644 --- a/plugins/testdata/test-playlist-generator/main.go +++ b/plugins/testdata/test-playlist-generator/main.go @@ -21,16 +21,16 @@ func (t *testPlaylistGenerator) GetPlaylists(_ pg.GetPlaylistsRequest) (pg.GetPl return pg.GetPlaylistsResponse{}, fmt.Errorf("%s", errMsg) } - // Get the owner user ID from config (defaults to "user-1") - ownerID := "user-1" - if id, ok := pdk.GetConfig("owner_id"); ok && id != "" { - ownerID = id + // Get the owner username from config (defaults to "admin") + ownerUsername := "admin" + if u, ok := pdk.GetConfig("owner_username"); ok && u != "" { + ownerUsername = u } return pg.GetPlaylistsResponse{ Playlists: []pg.PlaylistInfo{ - {ID: "daily-mix-1", OwnerUserID: ownerID}, - {ID: "daily-mix-2", OwnerUserID: ownerID}, + {ID: "daily-mix-1", OwnerUsername: ownerUsername}, + {ID: "daily-mix-2", OwnerUsername: ownerUsername}, }, RefreshInterval: 0, // No re-discovery in tests }, nil From fde4517339cb6035b7d9682e02382df281199256 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 13:27:02 -0500 Subject: [PATCH 11/24] refactor(plugins): rename GetPlaylists, add NotFound error and RetryInterval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename GetPlaylists → GetAvailablePlaylists for clarity. Add PlaylistGeneratorErrorNotFound so plugins can signal a playlist is temporarily unavailable (orchestrator skips it but retries on next discovery). Add RetryInterval on GetAvailablePlaylistsResponse for automatic retry of transient GetPlaylist failures. --- plugins/capabilities/playlist_generator.go | 34 ++++++--- plugins/capabilities/playlist_generator.yaml | 55 +++++++------- .../go/playlistgenerator/playlistgenerator.go | 64 ++++++++++------- .../playlistgenerator_stub.go | 48 ++++++++----- .../src/playlistgenerator.rs | 58 ++++++++------- plugins/playlist_generator.go | 41 ++++++++--- plugins/playlist_generator_test.go | 71 ++++++++++++++++++- .../testdata/test-playlist-generator/main.go | 23 ++++-- 8 files changed, 279 insertions(+), 115 deletions(-) diff --git a/plugins/capabilities/playlist_generator.go b/plugins/capabilities/playlist_generator.go index 5f7cb077c..4d034a7c0 100644 --- a/plugins/capabilities/playlist_generator.go +++ b/plugins/capabilities/playlist_generator.go @@ -2,30 +2,33 @@ package capabilities // PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two -// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching -// the heavy payload (tracks, metadata). +// functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for +// fetching the heavy payload (tracks, metadata). // //nd:capability name=playlistgenerator required=true type PlaylistGenerator interface { - // GetPlaylists returns the list of playlists this plugin provides. - //nd:export name=nd_playlist_generator_get_playlists - GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + // GetAvailablePlaylists returns the list of playlists this plugin provides. + //nd:export name=nd_playlist_generator_get_available_playlists + GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist returns the full data for a single playlist (tracks, metadata). //nd:export name=nd_playlist_generator_get_playlist GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) } -// GetPlaylistsRequest is the request for GetPlaylists. -type GetPlaylistsRequest struct{} +// GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. +type GetAvailablePlaylistsRequest struct{} -// GetPlaylistsResponse is the response for GetPlaylists. -type GetPlaylistsResponse struct { +// GetAvailablePlaylistsResponse is the response for GetAvailablePlaylists. +type GetAvailablePlaylistsResponse struct { // Playlists is the list of playlists provided by this plugin. Playlists []PlaylistInfo `json:"playlists"` - // RefreshInterval is the number of seconds until the next GetPlaylists call. + // RefreshInterval is the number of seconds until the next GetAvailablePlaylists call. // 0 means never re-discover. RefreshInterval int64 `json:"refreshInterval"` + // RetryInterval is the number of seconds before retrying a failed GetPlaylist call. + // 0 means no automatic retry for transient errors. + RetryInterval int64 `json:"retryInterval"` } // PlaylistInfo identifies a plugin playlist and its target user. @@ -56,3 +59,14 @@ type GetPlaylistResponse struct { // 0 means static (never refresh). ValidUntil int64 `json:"validUntil"` } + +// PlaylistGeneratorError represents an error type for playlist generator operations. +type PlaylistGeneratorError string + +const ( + // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. + PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" +) + +// Error implements the error interface for PlaylistGeneratorError. +func (e PlaylistGeneratorError) Error() string { return string(e) } diff --git a/plugins/capabilities/playlist_generator.yaml b/plugins/capabilities/playlist_generator.yaml index 2b4f57a70..519387660 100644 --- a/plugins/capabilities/playlist_generator.yaml +++ b/plugins/capabilities/playlist_generator.yaml @@ -1,12 +1,12 @@ version: v1-draft exports: - nd_playlist_generator_get_playlists: - description: GetPlaylists returns the list of playlists this plugin provides. + nd_playlist_generator_get_available_playlists: + description: GetAvailablePlaylists returns the list of playlists this plugin provides. input: - $ref: '#/components/schemas/GetPlaylistsRequest' + $ref: '#/components/schemas/GetAvailablePlaylistsRequest' contentType: application/json output: - $ref: '#/components/schemas/GetPlaylistsResponse' + $ref: '#/components/schemas/GetAvailablePlaylistsResponse' contentType: application/json nd_playlist_generator_get_playlist: description: GetPlaylist returns the full data for a single playlist (tracks, metadata). @@ -18,6 +18,33 @@ exports: contentType: application/json components: schemas: + GetAvailablePlaylistsRequest: + description: GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. + properties: {} + GetAvailablePlaylistsResponse: + description: GetAvailablePlaylistsResponse is the response for GetAvailablePlaylists. + properties: + playlists: + type: array + description: Playlists is the list of playlists provided by this plugin. + items: + $ref: '#/components/schemas/PlaylistInfo' + refreshInterval: + type: integer + format: int64 + description: |- + RefreshInterval is the number of seconds until the next GetAvailablePlaylists call. + 0 means never re-discover. + retryInterval: + type: integer + format: int64 + description: |- + RetryInterval is the number of seconds before retrying a failed GetPlaylist call. + 0 means no automatic retry for transient errors. + required: + - playlists + - refreshInterval + - retryInterval GetPlaylistRequest: description: GetPlaylistRequest is the request for GetPlaylist. properties: @@ -53,26 +80,6 @@ components: - name - tracks - validUntil - GetPlaylistsRequest: - description: GetPlaylistsRequest is the request for GetPlaylists. - properties: {} - GetPlaylistsResponse: - description: GetPlaylistsResponse is the response for GetPlaylists. - properties: - playlists: - type: array - description: Playlists is the list of playlists provided by this plugin. - items: - $ref: '#/components/schemas/PlaylistInfo' - refreshInterval: - type: integer - format: int64 - description: |- - RefreshInterval is the number of seconds until the next GetPlaylists call. - 0 means never re-discover. - required: - - playlists - - refreshInterval PlaylistInfo: description: PlaylistInfo identifies a plugin playlist and its target user. properties: diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator.go b/plugins/pdk/go/playlistgenerator/playlistgenerator.go index 438cff040..15453c13f 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator.go +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator.go @@ -11,6 +11,33 @@ import ( "github.com/navidrome/navidrome/plugins/pdk/go/pdk" ) +// PlaylistGeneratorError represents an error type for playlist generator operations. +type PlaylistGeneratorError string + +const ( + // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. + PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" +) + +// Error implements the error interface for PlaylistGeneratorError. +func (e PlaylistGeneratorError) Error() string { return string(e) } + +// GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. +type GetAvailablePlaylistsRequest struct { +} + +// GetAvailablePlaylistsResponse is the response for GetAvailablePlaylists. +type GetAvailablePlaylistsResponse struct { + // Playlists is the list of playlists provided by this plugin. + Playlists []PlaylistInfo `json:"playlists"` + // RefreshInterval is the number of seconds until the next GetAvailablePlaylists call. + // 0 means never re-discover. + RefreshInterval int64 `json:"refreshInterval"` + // RetryInterval is the number of seconds before retrying a failed GetPlaylist call. + // 0 means no automatic retry for transient errors. + RetryInterval int64 `json:"retryInterval"` +} + // GetPlaylistRequest is the request for GetPlaylist. type GetPlaylistRequest struct { // ID is the plugin-scoped playlist ID. @@ -32,19 +59,6 @@ type GetPlaylistResponse struct { ValidUntil int64 `json:"validUntil"` } -// GetPlaylistsRequest is the request for GetPlaylists. -type GetPlaylistsRequest struct { -} - -// GetPlaylistsResponse is the response for GetPlaylists. -type GetPlaylistsResponse struct { - // Playlists is the list of playlists provided by this plugin. - Playlists []PlaylistInfo `json:"playlists"` - // RefreshInterval is the number of seconds until the next GetPlaylists call. - // 0 means never re-discover. - RefreshInterval int64 `json:"refreshInterval"` -} - // PlaylistInfo identifies a plugin playlist and its target user. type PlaylistInfo struct { // ID is the plugin-scoped unique identifier for this playlist. @@ -78,23 +92,23 @@ type SongRef struct { // PlaylistGenerator requires all methods to be implemented. // PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two -// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching -// the heavy payload (tracks, metadata). +// functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for +// fetching the heavy payload (tracks, metadata). type PlaylistGenerator interface { - // GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. - GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + // GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. + GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) } // Internal implementation holders var ( - playlistsImpl func(GetPlaylistsRequest) (GetPlaylistsResponse, error) - playlistImpl func(GetPlaylistRequest) (GetPlaylistResponse, error) + availablePlaylistsImpl func(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) + playlistImpl func(GetPlaylistRequest) (GetPlaylistResponse, error) ) // Register registers a playlistgenerator implementation. // All methods are required. func Register(impl PlaylistGenerator) { - playlistsImpl = impl.GetPlaylists + availablePlaylistsImpl = impl.GetAvailablePlaylists playlistImpl = impl.GetPlaylist } @@ -102,20 +116,20 @@ func Register(impl PlaylistGenerator) { // The host recognizes this and skips the plugin gracefully. const NotImplementedCode int32 = -2 -//go:wasmexport nd_playlist_generator_get_playlists -func _NdPlaylistGeneratorGetPlaylists() int32 { - if playlistsImpl == nil { +//go:wasmexport nd_playlist_generator_get_available_playlists +func _NdPlaylistGeneratorGetAvailablePlaylists() int32 { + if availablePlaylistsImpl == nil { // Return standard code - host will skip this plugin gracefully return NotImplementedCode } - var input GetPlaylistsRequest + var input GetAvailablePlaylistsRequest if err := pdk.InputJSON(&input); err != nil { pdk.SetError(err) return -1 } - output, err := playlistsImpl(input) + output, err := availablePlaylistsImpl(input) if err != nil { pdk.SetError(err) return -1 diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go index b5f95e023..348492a44 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go +++ b/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go @@ -8,6 +8,33 @@ package playlistgenerator +// PlaylistGeneratorError represents an error type for playlist generator operations. +type PlaylistGeneratorError string + +const ( + // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. + PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" +) + +// Error implements the error interface for PlaylistGeneratorError. +func (e PlaylistGeneratorError) Error() string { return string(e) } + +// GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. +type GetAvailablePlaylistsRequest struct { +} + +// GetAvailablePlaylistsResponse is the response for GetAvailablePlaylists. +type GetAvailablePlaylistsResponse struct { + // Playlists is the list of playlists provided by this plugin. + Playlists []PlaylistInfo `json:"playlists"` + // RefreshInterval is the number of seconds until the next GetAvailablePlaylists call. + // 0 means never re-discover. + RefreshInterval int64 `json:"refreshInterval"` + // RetryInterval is the number of seconds before retrying a failed GetPlaylist call. + // 0 means no automatic retry for transient errors. + RetryInterval int64 `json:"retryInterval"` +} + // GetPlaylistRequest is the request for GetPlaylist. type GetPlaylistRequest struct { // ID is the plugin-scoped playlist ID. @@ -29,19 +56,6 @@ type GetPlaylistResponse struct { ValidUntil int64 `json:"validUntil"` } -// GetPlaylistsRequest is the request for GetPlaylists. -type GetPlaylistsRequest struct { -} - -// GetPlaylistsResponse is the response for GetPlaylists. -type GetPlaylistsResponse struct { - // Playlists is the list of playlists provided by this plugin. - Playlists []PlaylistInfo `json:"playlists"` - // RefreshInterval is the number of seconds until the next GetPlaylists call. - // 0 means never re-discover. - RefreshInterval int64 `json:"refreshInterval"` -} - // PlaylistInfo identifies a plugin playlist and its target user. type PlaylistInfo struct { // ID is the plugin-scoped unique identifier for this playlist. @@ -75,11 +89,11 @@ type SongRef struct { // PlaylistGenerator requires all methods to be implemented. // PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two -// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching -// the heavy payload (tracks, metadata). +// functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for +// fetching the heavy payload (tracks, metadata). type PlaylistGenerator interface { - // GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. - GetPlaylists(GetPlaylistsRequest) (GetPlaylistsResponse, error) + // GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. + GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) } diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs index 82aa4d3cd..9e9d3e555 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs @@ -18,6 +18,31 @@ fn is_zero_u64(value: &u64) -> bool { *value == 0 } fn is_zero_f32(value: &f32) -> bool { *value == 0.0 } #[allow(dead_code)] fn is_zero_f64(value: &f64) -> bool { *value == 0.0 } +/// PlaylistGeneratorError represents an error type for playlist generator operations. +pub type PlaylistGeneratorError = &'static str; +/// PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. +pub const PLAYLIST_GENERATOR_ERROR_NOT_FOUND: PlaylistGeneratorError = "playlist_generator(not_found)"; +/// GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetAvailablePlaylistsRequest { +} +/// GetAvailablePlaylistsResponse is the response for GetAvailablePlaylists. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GetAvailablePlaylistsResponse { + /// Playlists is the list of playlists provided by this plugin. + #[serde(default)] + pub playlists: Vec, + /// RefreshInterval is the number of seconds until the next GetAvailablePlaylists call. + /// 0 means never re-discover. + #[serde(default)] + pub refresh_interval: i64, + /// RetryInterval is the number of seconds before retrying a failed GetPlaylist call. + /// 0 means no automatic retry for transient errors. + #[serde(default)] + pub retry_interval: i64, +} /// GetPlaylistRequest is the request for GetPlaylist. #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -47,23 +72,6 @@ pub struct GetPlaylistResponse { #[serde(default)] pub valid_until: i64, } -/// GetPlaylistsRequest is the request for GetPlaylists. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct GetPlaylistsRequest { -} -/// GetPlaylistsResponse is the response for GetPlaylists. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct GetPlaylistsResponse { - /// Playlists is the list of playlists provided by this plugin. - #[serde(default)] - pub playlists: Vec, - /// RefreshInterval is the number of seconds until the next GetPlaylists call. - /// 0 means never re-discover. - #[serde(default)] - pub refresh_interval: i64, -} /// PlaylistInfo identifies a plugin playlist and its target user. #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -131,11 +139,11 @@ impl Error { /// PlaylistGenerator requires all methods to be implemented. /// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", /// personalized recommendations). Plugins implementing this capability expose two -/// functions: GetPlaylists for lightweight discovery and GetPlaylist for fetching -/// the heavy payload (tracks, metadata). +/// functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for +/// fetching the heavy payload (tracks, metadata). pub trait PlaylistGenerator { - /// GetPlaylists - GetPlaylists returns the list of playlists this plugin provides. - fn get_playlists(&self, req: GetPlaylistsRequest) -> Result; + /// GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. + fn get_available_playlists(&self, req: GetAvailablePlaylistsRequest) -> Result; /// GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). fn get_playlist(&self, req: GetPlaylistRequest) -> Result; } @@ -146,11 +154,11 @@ pub trait PlaylistGenerator { macro_rules! register_playlistgenerator { ($plugin_type:ty) => { #[extism_pdk::plugin_fn] - pub fn nd_playlist_generator_get_playlists( - req: extism_pdk::Json<$crate::playlistgenerator::GetPlaylistsRequest> - ) -> extism_pdk::FnResult> { + pub fn nd_playlist_generator_get_available_playlists( + req: extism_pdk::Json<$crate::playlistgenerator::GetAvailablePlaylistsRequest> + ) -> extism_pdk::FnResult> { let plugin = <$plugin_type>::default(); - let result = $crate::playlistgenerator::PlaylistGenerator::get_playlists(&plugin, req.into_inner())?; + let result = $crate::playlistgenerator::PlaylistGenerator::get_available_playlists(&plugin, req.into_inner())?; Ok(extism_pdk::Json(result)) } #[extism_pdk::plugin_fn] diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index 3ae2a4ee4..687bc092b 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -3,6 +3,7 @@ package plugins import ( "context" "fmt" + "strings" "sync" "time" @@ -16,14 +17,14 @@ import ( const ( CapabilityPlaylistGenerator Capability = "PlaylistGenerator" - FuncPlaylistGeneratorGetPlaylists = "nd_playlist_generator_get_playlists" - FuncPlaylistGeneratorGetPlaylist = "nd_playlist_generator_get_playlist" + FuncPlaylistGeneratorGetAvailablePlaylists = "nd_playlist_generator_get_available_playlists" + FuncPlaylistGeneratorGetPlaylist = "nd_playlist_generator_get_playlist" ) func init() { registerCapability( CapabilityPlaylistGenerator, - FuncPlaylistGeneratorGetPlaylists, + FuncPlaylistGeneratorGetAvailablePlaylists, FuncPlaylistGeneratorGetPlaylist, ) } @@ -39,6 +40,7 @@ type playlistGeneratorOrchestrator struct { wg sync.WaitGroup refreshTimers map[string]*time.Timer // keyed by playlist DB ID discoveryTimer *time.Timer + retryInterval time.Duration // from last GetAvailablePlaylists response } func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.DataStore, parentCtx context.Context) *playlistGeneratorOrchestrator { @@ -54,16 +56,21 @@ func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.Dat } } -// discoverAndSync calls GetPlaylists, then GetPlaylist for each, matches tracks, and upserts. +// discoverAndSync calls GetAvailablePlaylists, then GetPlaylist for each, matches tracks, and upserts. func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { - resp, err := callPluginFunction[capabilities.GetPlaylistsRequest, capabilities.GetPlaylistsResponse]( - ctx, o.plugin, FuncPlaylistGeneratorGetPlaylists, capabilities.GetPlaylistsRequest{}, + resp, err := callPluginFunction[capabilities.GetAvailablePlaylistsRequest, capabilities.GetAvailablePlaylistsResponse]( + ctx, o.plugin, FuncPlaylistGeneratorGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, ) if err != nil { - log.Error(ctx, "Failed to call GetPlaylists", "plugin", o.pluginName, err) + log.Error(ctx, "Failed to call GetAvailablePlaylists", "plugin", o.pluginName, err) return } + // Store retry interval from response + if resp.RetryInterval > 0 { + o.retryInterval = time.Duration(resp.RetryInterval) * time.Second + } + for _, info := range resp.Playlists { // Resolve username to user ID user, err := o.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) @@ -89,7 +96,20 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c ctx, o.plugin, FuncPlaylistGeneratorGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, ) if err != nil { - log.Error(ctx, "Failed to call GetPlaylist", "plugin", o.pluginName, "playlistID", info.ID, err) + if isPlaylistNotFoundError(err) { + log.Info(ctx, "Playlist not found, skipping", "plugin", o.pluginName, "playlistID", info.ID) + // Stop any existing refresh timer for this playlist + if timer, ok := o.refreshTimers[dbID]; ok { + timer.Stop() + delete(o.refreshTimers, dbID) + } + return + } + log.Warn(ctx, "Failed to call GetPlaylist", "plugin", o.pluginName, "playlistID", info.ID, err) + // Schedule retry for transient errors if retryInterval is configured + if o.retryInterval > 0 { + o.schedulePlaylistRefresh(ctx, info, dbID, ownerID, o.retryInterval) + } return } @@ -165,6 +185,11 @@ func (o *playlistGeneratorOrchestrator) scheduleDiscovery(_ context.Context, del }) } +// isPlaylistNotFoundError checks if the error contains a NotFound sentinel from the plugin. +func isPlaylistNotFoundError(err error) bool { + return err != nil && strings.Contains(err.Error(), capabilities.PlaylistGeneratorErrorNotFound.Error()) +} + // stop cancels the context, stops all timers, and waits for in-flight goroutines. func (o *playlistGeneratorOrchestrator) stop() { o.cancel() diff --git a/plugins/playlist_generator_test.go b/plugins/playlist_generator_test.go index 5f474352b..d4b5c4f7c 100644 --- a/plugins/playlist_generator_test.go +++ b/plugins/playlist_generator_test.go @@ -3,8 +3,11 @@ package plugins import ( + "time" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" + "github.com/navidrome/navidrome/plugins/capabilities" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -100,7 +103,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }) }) - Describe("GetPlaylists error handling", func() { + Describe("GetAvailablePlaylists error handling", func() { It("handles plugin errors gracefully", func() { errManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ "test-playlist-generator": {"error": "service unavailable"}, @@ -115,7 +118,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { errDS.MockedPlaylist = tests.CreateMockPlaylistRepo() } errPlsRepo := errDS.MockedPlaylist.(*tests.MockPlaylistRepo) - // The orchestrator was started but GetPlaylists returned error, + // The orchestrator was started but GetAvailablePlaylists returned error, // so no playlists should be created Consistently(func() int { return len(errPlsRepo.Data) @@ -123,6 +126,70 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }) }) + Describe("GetPlaylist NotFound error", func() { + It("skips playlists when plugin returns NotFound", func() { + notFoundManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ + "test-playlist-generator": { + "get_playlist_error": "playlist temporarily unavailable", + "get_playlist_error_type": string(capabilities.PlaylistGeneratorErrorNotFound), + }, + }, "test-playlist-generator"+PackageExtension) + + // Should still have the orchestrator + Expect(notFoundManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + + // No playlists should be created (all returned NotFound) + notFoundDS := notFoundManager.ds.(*tests.MockDataStore) + if notFoundDS.MockedPlaylist == nil { + notFoundDS.MockedPlaylist = tests.CreateMockPlaylistRepo() + } + notFoundPlsRepo := notFoundDS.MockedPlaylist.(*tests.MockPlaylistRepo) + Consistently(func() int { + return len(notFoundPlsRepo.Data) + }, "500ms").Should(Equal(0)) + + // No refresh timers should be scheduled for NotFound playlists + orch := notFoundManager.playlistGenerators["test-playlist-generator"] + Eventually(func() int { + return len(orch.refreshTimers) + }).Should(Equal(0)) + }) + }) + + Describe("GetPlaylist transient error with RetryInterval", func() { + It("stores retryInterval and schedules retry on transient errors", func() { + retryManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ + "test-playlist-generator": { + "get_playlist_error": "temporary failure", + "retry_interval": "60", + }, + }, "test-playlist-generator"+PackageExtension) + + Expect(retryManager.playlistGenerators).To(HaveKey("test-playlist-generator")) + orch := retryManager.playlistGenerators["test-playlist-generator"] + + // retryInterval should be stored from the response + Eventually(func() time.Duration { + return orch.retryInterval + }).Should(Equal(60 * time.Second)) + + // No playlists should be created (GetPlaylist failed) + retryDS := retryManager.ds.(*tests.MockDataStore) + if retryDS.MockedPlaylist == nil { + retryDS.MockedPlaylist = tests.CreateMockPlaylistRepo() + } + retryPlsRepo := retryDS.MockedPlaylist.(*tests.MockPlaylistRepo) + Consistently(func() int { + return len(retryPlsRepo.Data) + }, "500ms").Should(Equal(0)) + + // Refresh timers should be scheduled for transient errors + Eventually(func() int { + return len(orch.refreshTimers) + }).Should(BeNumerically(">=", 1)) + }) + }) + Describe("stop", func() { It("stops the orchestrator when the manager stops", func() { stopManager, _ := createTestManagerWithPlugins(nil, diff --git a/plugins/testdata/test-playlist-generator/main.go b/plugins/testdata/test-playlist-generator/main.go index 830bc0845..f98168b5c 100644 --- a/plugins/testdata/test-playlist-generator/main.go +++ b/plugins/testdata/test-playlist-generator/main.go @@ -3,6 +3,7 @@ package main import ( "fmt" + "strconv" "github.com/navidrome/navidrome/plugins/pdk/go/pdk" pg "github.com/navidrome/navidrome/plugins/pdk/go/playlistgenerator" @@ -14,11 +15,11 @@ func init() { type testPlaylistGenerator struct{} -func (t *testPlaylistGenerator) GetPlaylists(_ pg.GetPlaylistsRequest) (pg.GetPlaylistsResponse, error) { +func (t *testPlaylistGenerator) GetAvailablePlaylists(_ pg.GetAvailablePlaylistsRequest) (pg.GetAvailablePlaylistsResponse, error) { // Check for configured error errMsg, hasErr := pdk.GetConfig("error") if hasErr && errMsg != "" { - return pg.GetPlaylistsResponse{}, fmt.Errorf("%s", errMsg) + return pg.GetAvailablePlaylistsResponse{}, fmt.Errorf("%s", errMsg) } // Get the owner username from config (defaults to "admin") @@ -27,19 +28,33 @@ func (t *testPlaylistGenerator) GetPlaylists(_ pg.GetPlaylistsRequest) (pg.GetPl ownerUsername = u } - return pg.GetPlaylistsResponse{ + resp := pg.GetAvailablePlaylistsResponse{ Playlists: []pg.PlaylistInfo{ {ID: "daily-mix-1", OwnerUsername: ownerUsername}, {ID: "daily-mix-2", OwnerUsername: ownerUsername}, }, RefreshInterval: 0, // No re-discovery in tests - }, nil + } + + // Support configurable retry interval + if ri, ok := pdk.GetConfig("retry_interval"); ok && ri != "" { + if v, err := strconv.ParseInt(ri, 10, 64); err == nil { + resp.RetryInterval = v + } + } + + return resp, nil } func (t *testPlaylistGenerator) GetPlaylist(req pg.GetPlaylistRequest) (pg.GetPlaylistResponse, error) { // Check for configured error errMsg, hasErr := pdk.GetConfig("get_playlist_error") if hasErr && errMsg != "" { + // Check if the error should be typed (e.g., NotFound) + errType, _ := pdk.GetConfig("get_playlist_error_type") + if errType == pg.PlaylistGeneratorErrorNotFound.Error() { + return pg.GetPlaylistResponse{}, fmt.Errorf("%w: %s", pg.PlaylistGeneratorErrorNotFound, errMsg) + } return pg.GetPlaylistResponse{}, fmt.Errorf("%s", errMsg) } From 9c516cf601ea552cc6a35ee8cc772adb2673c2d7 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 14:44:53 -0500 Subject: [PATCH 12/24] refactor(plugins): serialize PlaylistGenerator plugin calls via work queue Replace the timer-per-playlist + WaitGroup.Go model with a single worker goroutine processing work items from a buffered channel. This eliminates race conditions from parallel plugin calls (rate limiting risk), unsynchronized map/field access across goroutines, and overlapping discovery and refresh timers. The worker owns all mutable state (refreshTimers, discoveryTimer) exclusively, while retryInterval and refreshTimerCount use atomics for safe test observability. Also adds retry-on-failure for GetAvailablePlaylists (5 min delay) and makes MockPlaylistRepo thread-safe with sync.RWMutex. --- plugins/manager.go | 4 +- plugins/playlist_generator.go | 128 ++++++++++++++++++++++------- plugins/playlist_generator_test.go | 73 +++++----------- plugins/plugins_suite_test.go | 2 +- tests/mock_playlist_repo.go | 47 +++++++++++ 5 files changed, 167 insertions(+), 87 deletions(-) diff --git a/plugins/manager.go b/plugins/manager.go index 8cee839e2..4bf44928a 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -192,9 +192,9 @@ func (m *Manager) startPlaylistGenerators(ctx context.Context) { if !hasCapability(p.capabilities, CapabilityPlaylistGenerator) { continue } - orch := newPlaylistGeneratorOrchestrator(name, p, m.ds, ctx) + orch := newPlaylistGeneratorOrchestrator(ctx, name, p, m.ds) m.playlistGenerators[name] = orch - orch.wg.Go(func() { orch.discoverAndSync(orch.ctx) }) + go orch.run() } } diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index 687bc092b..c933f961a 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -4,7 +4,7 @@ import ( "context" "fmt" "strings" - "sync" + "sync/atomic" "time" "github.com/navidrome/navidrome/core/matcher" @@ -19,6 +19,12 @@ const ( FuncPlaylistGeneratorGetAvailablePlaylists = "nd_playlist_generator_get_available_playlists" FuncPlaylistGeneratorGetPlaylist = "nd_playlist_generator_get_playlist" + + // workChCapacity is the buffer size for the work channel. + workChCapacity = 16 + + // discoveryRetryDelay is how long to wait before retrying a failed GetAvailablePlaylists call. + discoveryRetryDelay = 5 * time.Minute ) func init() { @@ -29,21 +35,40 @@ func init() { ) } -// playlistGeneratorOrchestrator manages playlist generation for a single plugin. -type playlistGeneratorOrchestrator struct { - pluginName string - plugin *plugin - ds model.DataStore - matcher *matcher.Matcher - ctx context.Context - cancel context.CancelFunc - wg sync.WaitGroup - refreshTimers map[string]*time.Timer // keyed by playlist DB ID - discoveryTimer *time.Timer - retryInterval time.Duration // from last GetAvailablePlaylists response +type workType int + +const ( + workDiscover workType = iota // run discoverAndSync + workSync // run syncPlaylist for a single playlist +) + +type workItem struct { + typ workType + info capabilities.PlaylistInfo // only for workSync + dbID string // only for workSync + ownerID string // only for workSync } -func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.DataStore, parentCtx context.Context) *playlistGeneratorOrchestrator { +// playlistGeneratorOrchestrator manages playlist generation for a single plugin. +// All mutable state (refreshTimers, discoveryTimer) is owned exclusively by the +// worker goroutine — no synchronization needed. The retryInterval and +// refreshTimerCount fields use atomics so tests can observe them race-free. +type playlistGeneratorOrchestrator struct { + pluginName string + plugin *plugin + ds model.DataStore + matcher *matcher.Matcher + ctx context.Context + cancel context.CancelFunc + workCh chan workItem // serialized work queue + refreshTimers map[string]*time.Timer // keyed by playlist DB ID — worker-only + discoveryTimer *time.Timer // worker-only + retryInterval atomic.Int64 // nanoseconds; from last GetAvailablePlaylists response + refreshTimerCount atomic.Int32 // number of active refresh timers + done chan struct{} // closed when worker exits +} + +func newPlaylistGeneratorOrchestrator(parentCtx context.Context, pluginName string, p *plugin, ds model.DataStore) *playlistGeneratorOrchestrator { ctx, cancel := context.WithCancel(parentCtx) return &playlistGeneratorOrchestrator{ pluginName: pluginName, @@ -52,23 +77,51 @@ func newPlaylistGeneratorOrchestrator(pluginName string, p *plugin, ds model.Dat matcher: matcher.New(ds), ctx: ctx, cancel: cancel, + workCh: make(chan workItem, workChCapacity), refreshTimers: make(map[string]*time.Timer), + done: make(chan struct{}), + } +} + +// run is the single worker goroutine that processes all work items sequentially. +// It performs an initial discovery before entering the main loop. +func (o *playlistGeneratorOrchestrator) run() { + defer close(o.done) + + // Run initial discovery before entering the loop + o.discoverAndSync() + + for { + select { + case <-o.ctx.Done(): + o.stopAllTimers() + return + case item := <-o.workCh: + switch item.typ { + case workDiscover: + o.discoverAndSync() + case workSync: + o.syncPlaylist(item.info, item.dbID, item.ownerID) + } + } } } // discoverAndSync calls GetAvailablePlaylists, then GetPlaylist for each, matches tracks, and upserts. -func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { +func (o *playlistGeneratorOrchestrator) discoverAndSync() { + ctx := o.ctx resp, err := callPluginFunction[capabilities.GetAvailablePlaylistsRequest, capabilities.GetAvailablePlaylistsResponse]( ctx, o.plugin, FuncPlaylistGeneratorGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, ) if err != nil { - log.Error(ctx, "Failed to call GetAvailablePlaylists", "plugin", o.pluginName, err) + log.Error(ctx, "Failed to call GetAvailablePlaylists, retrying later", "plugin", o.pluginName, err) + o.scheduleDiscovery(discoveryRetryDelay) return } // Store retry interval from response if resp.RetryInterval > 0 { - o.retryInterval = time.Duration(resp.RetryInterval) * time.Second + o.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) } for _, info := range resp.Playlists { @@ -81,17 +134,18 @@ func (o *playlistGeneratorOrchestrator) discoverAndSync(ctx context.Context) { } ownerID := user.ID dbID := id.NewHash(o.pluginName, info.ID, ownerID) - o.syncPlaylist(ctx, info, dbID, ownerID) + o.syncPlaylist(info, dbID, ownerID) } // Schedule re-discovery if RefreshInterval > 0 if resp.RefreshInterval > 0 { - o.scheduleDiscovery(ctx, time.Duration(resp.RefreshInterval)*time.Second) + o.scheduleDiscovery(time.Duration(resp.RefreshInterval) * time.Second) } } // syncPlaylist calls GetPlaylist, matches tracks, and upserts the playlist in the DB. -func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info capabilities.PlaylistInfo, dbID string, ownerID string) { +func (o *playlistGeneratorOrchestrator) syncPlaylist(info capabilities.PlaylistInfo, dbID string, ownerID string) { + ctx := o.ctx resp, err := callPluginFunction[capabilities.GetPlaylistRequest, capabilities.GetPlaylistResponse]( ctx, o.plugin, FuncPlaylistGeneratorGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, ) @@ -102,13 +156,14 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c if timer, ok := o.refreshTimers[dbID]; ok { timer.Stop() delete(o.refreshTimers, dbID) + o.refreshTimerCount.Store(int32(len(o.refreshTimers))) } return } log.Warn(ctx, "Failed to call GetPlaylist", "plugin", o.pluginName, "playlistID", info.ID, err) // Schedule retry for transient errors if retryInterval is configured - if o.retryInterval > 0 { - o.schedulePlaylistRefresh(ctx, info, dbID, ownerID, o.retryInterval) + if ri := time.Duration(o.retryInterval.Load()); ri > 0 { + o.schedulePlaylistRefresh(info, dbID, ownerID, ri) } return } @@ -162,26 +217,33 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(ctx context.Context, info c if delay <= 0 { delay = 1 * time.Second // Already expired, refresh soon } - o.schedulePlaylistRefresh(ctx, info, dbID, ownerID, delay) + o.schedulePlaylistRefresh(info, dbID, ownerID, delay) } } -func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(_ context.Context, info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { +func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { // Cancel existing timer if any if timer, ok := o.refreshTimers[dbID]; ok { timer.Stop() } o.refreshTimers[dbID] = time.AfterFunc(delay, func() { - o.wg.Go(func() { o.syncPlaylist(o.ctx, info, dbID, ownerID) }) + select { + case o.workCh <- workItem{typ: workSync, info: info, dbID: dbID, ownerID: ownerID}: + case <-o.ctx.Done(): + } }) + o.refreshTimerCount.Store(int32(len(o.refreshTimers))) } -func (o *playlistGeneratorOrchestrator) scheduleDiscovery(_ context.Context, delay time.Duration) { +func (o *playlistGeneratorOrchestrator) scheduleDiscovery(delay time.Duration) { if o.discoveryTimer != nil { o.discoveryTimer.Stop() } o.discoveryTimer = time.AfterFunc(delay, func() { - o.wg.Go(func() { o.discoverAndSync(o.ctx) }) + select { + case o.workCh <- workItem{typ: workDiscover}: + case <-o.ctx.Done(): + } }) } @@ -190,14 +252,18 @@ func isPlaylistNotFoundError(err error) bool { return err != nil && strings.Contains(err.Error(), capabilities.PlaylistGeneratorErrorNotFound.Error()) } -// stop cancels the context, stops all timers, and waits for in-flight goroutines. -func (o *playlistGeneratorOrchestrator) stop() { - o.cancel() +// stopAllTimers stops the discovery timer and all refresh timers. +func (o *playlistGeneratorOrchestrator) stopAllTimers() { if o.discoveryTimer != nil { o.discoveryTimer.Stop() } for _, timer := range o.refreshTimers { timer.Stop() } - o.wg.Wait() +} + +// stop cancels the context and waits for the worker goroutine to finish. +func (o *playlistGeneratorOrchestrator) stop() { + o.cancel() + <-o.done } diff --git a/plugins/playlist_generator_test.go b/plugins/playlist_generator_test.go index d4b5c4f7c..dc4187952 100644 --- a/plugins/playlist_generator_test.go +++ b/plugins/playlist_generator_test.go @@ -5,7 +5,6 @@ package plugins import ( "time" - "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/plugins/capabilities" "github.com/navidrome/navidrome/tests" @@ -24,13 +23,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { "test-playlist-generator"+PackageExtension, ) - // Pre-initialize the mock playlist repo to avoid a race with the - // discoverAndSync goroutine that is launched during Start(). - mockDS := pgManager.ds.(*tests.MockDataStore) - if mockDS.MockedPlaylist == nil { - mockDS.MockedPlaylist = tests.CreateMockPlaylistRepo() - } - mockPlsRepo = mockDS.MockedPlaylist.(*tests.MockPlaylistRepo) + mockPlsRepo = pgManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) }) Describe("capability detection", func() { @@ -49,30 +42,16 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { // The orchestrator runs discoverAndSync in a goroutine on Start(). // Give it a moment to complete. Eventually(func() int { - return len(mockPlsRepo.Data) + return mockPlsRepo.Len() }).Should(BeNumerically(">=", 2)) }) It("creates playlists with correct fields", func() { - // Check that playlists have the correct plugin fields Eventually(func() bool { - for _, pls := range mockPlsRepo.Data { - if pls.PluginID == "test-playlist-generator" && pls.PluginPlaylistID == "daily-mix-1" { - return true - } - } - return false + return mockPlsRepo.FindByPluginPlaylistID("daily-mix-1") != nil }).Should(BeTrue()) - // Find the daily-mix-1 playlist and verify its fields - var dailyMix1 *model.Playlist - for _, pls := range mockPlsRepo.Data { - if pls.PluginPlaylistID == "daily-mix-1" { - dailyMix1 = pls - break - } - } - Expect(dailyMix1).ToNot(BeNil()) + dailyMix1 := mockPlsRepo.FindByPluginPlaylistID("daily-mix-1") Expect(dailyMix1.Name).To(Equal("Daily Mix 1")) Expect(dailyMix1.Comment).To(Equal("Your personalized daily mix")) Expect(dailyMix1.ExternalImageURL).To(Equal("https://example.com/cover1.jpg")) @@ -85,7 +64,7 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { It("generates deterministic playlist IDs", func() { expectedID := id.NewHash("test-playlist-generator", "daily-mix-1", "user-1") Eventually(func() bool { - _, exists := mockPlsRepo.Data[expectedID] + _, exists := mockPlsRepo.GetData(expectedID) return exists }).Should(BeTrue()) }) @@ -96,8 +75,8 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Expect(id1).ToNot(Equal(id2)) Eventually(func() bool { - _, exists1 := mockPlsRepo.Data[id1] - _, exists2 := mockPlsRepo.Data[id2] + _, exists1 := mockPlsRepo.GetData(id1) + _, exists2 := mockPlsRepo.GetData(id2) return exists1 && exists2 }).Should(BeTrue()) }) @@ -113,15 +92,11 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Expect(errManager.playlistGenerators).To(HaveKey("test-playlist-generator")) // But no playlists created - errDS := errManager.ds.(*tests.MockDataStore) - if errDS.MockedPlaylist == nil { - errDS.MockedPlaylist = tests.CreateMockPlaylistRepo() - } - errPlsRepo := errDS.MockedPlaylist.(*tests.MockPlaylistRepo) + errPlsRepo := errManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) // The orchestrator was started but GetAvailablePlaylists returned error, // so no playlists should be created Consistently(func() int { - return len(errPlsRepo.Data) + return errPlsRepo.Len() }, "500ms").Should(Equal(0)) }) }) @@ -139,20 +114,16 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Expect(notFoundManager.playlistGenerators).To(HaveKey("test-playlist-generator")) // No playlists should be created (all returned NotFound) - notFoundDS := notFoundManager.ds.(*tests.MockDataStore) - if notFoundDS.MockedPlaylist == nil { - notFoundDS.MockedPlaylist = tests.CreateMockPlaylistRepo() - } - notFoundPlsRepo := notFoundDS.MockedPlaylist.(*tests.MockPlaylistRepo) + notFoundPlsRepo := notFoundManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) Consistently(func() int { - return len(notFoundPlsRepo.Data) + return notFoundPlsRepo.Len() }, "500ms").Should(Equal(0)) // No refresh timers should be scheduled for NotFound playlists orch := notFoundManager.playlistGenerators["test-playlist-generator"] - Eventually(func() int { - return len(orch.refreshTimers) - }).Should(Equal(0)) + Eventually(func() int32 { + return orch.refreshTimerCount.Load() + }).Should(Equal(int32(0))) }) }) @@ -170,23 +141,19 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { // retryInterval should be stored from the response Eventually(func() time.Duration { - return orch.retryInterval + return time.Duration(orch.retryInterval.Load()) }).Should(Equal(60 * time.Second)) // No playlists should be created (GetPlaylist failed) - retryDS := retryManager.ds.(*tests.MockDataStore) - if retryDS.MockedPlaylist == nil { - retryDS.MockedPlaylist = tests.CreateMockPlaylistRepo() - } - retryPlsRepo := retryDS.MockedPlaylist.(*tests.MockPlaylistRepo) + retryPlsRepo := retryManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) Consistently(func() int { - return len(retryPlsRepo.Data) + return retryPlsRepo.Len() }, "500ms").Should(Equal(0)) // Refresh timers should be scheduled for transient errors - Eventually(func() int { - return len(orch.refreshTimers) - }).Should(BeNumerically(">=", 1)) + Eventually(func() int32 { + return orch.refreshTimerCount.Load() + }).Should(BeNumerically(">=", int32(1))) }) }) diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 4ab259173..1e53db664 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -139,7 +139,7 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s mockUserRepo := tests.CreateMockUserRepo() _ = mockUserRepo.Put(&model.User{ID: "user-1", UserName: "admin"}) - dataStore := &tests.MockDataStore{MockedPlugin: mockPluginRepo, MockedUser: mockUserRepo} + dataStore := &tests.MockDataStore{MockedPlugin: mockPluginRepo, MockedUser: mockUserRepo, MockedPlaylist: tests.CreateMockPlaylistRepo()} // Create and start manager manager := &Manager{ diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 9bdc52152..0fa3b0eae 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -2,6 +2,7 @@ package tests import ( "errors" + "sync" "github.com/deluan/rest" "github.com/navidrome/navidrome/model" @@ -17,6 +18,7 @@ func CreateMockPlaylistRepo() *MockPlaylistRepo { type MockPlaylistRepo struct { model.PlaylistRepository + mu sync.RWMutex Data map[string]*model.Playlist // keyed by ID PathMap map[string]*model.Playlist // keyed by path Last *model.Playlist @@ -26,10 +28,14 @@ type MockPlaylistRepo struct { } func (m *MockPlaylistRepo) SetError(err bool) { + m.mu.Lock() + defer m.mu.Unlock() m.Err = err } func (m *MockPlaylistRepo) Get(id string) (*model.Playlist, error) { + m.mu.RLock() + defer m.mu.RUnlock() if m.Err { return nil, errors.New("error") } @@ -46,6 +52,8 @@ func (m *MockPlaylistRepo) GetWithTracks(id string, _, _ bool) (*model.Playlist, } func (m *MockPlaylistRepo) Put(pls *model.Playlist) error { + m.mu.Lock() + defer m.mu.Unlock() if m.Err { return errors.New("error") } @@ -60,6 +68,8 @@ func (m *MockPlaylistRepo) Put(pls *model.Playlist) error { } func (m *MockPlaylistRepo) FindByPath(path string) (*model.Playlist, error) { + m.mu.RLock() + defer m.mu.RUnlock() if m.Err { return nil, errors.New("error") } @@ -72,6 +82,8 @@ func (m *MockPlaylistRepo) FindByPath(path string) (*model.Playlist, error) { } func (m *MockPlaylistRepo) Delete(id string) error { + m.mu.Lock() + defer m.mu.Unlock() if m.Err { return errors.New("error") } @@ -80,10 +92,14 @@ func (m *MockPlaylistRepo) Delete(id string) error { } func (m *MockPlaylistRepo) Tracks(_ string, _ bool) model.PlaylistTrackRepository { + m.mu.RLock() + defer m.mu.RUnlock() return m.TracksRepo } func (m *MockPlaylistRepo) Exists(id string) (bool, error) { + m.mu.RLock() + defer m.mu.RUnlock() if m.Err { return false, errors.New("error") } @@ -95,6 +111,8 @@ func (m *MockPlaylistRepo) Exists(id string) (bool, error) { } func (m *MockPlaylistRepo) Count(_ ...rest.QueryOptions) (int64, error) { + m.mu.RLock() + defer m.mu.RUnlock() if m.Err { return 0, errors.New("error") } @@ -102,10 +120,39 @@ func (m *MockPlaylistRepo) Count(_ ...rest.QueryOptions) (int64, error) { } func (m *MockPlaylistRepo) CountAll(_ ...model.QueryOptions) (int64, error) { + m.mu.RLock() + defer m.mu.RUnlock() if m.Err { return 0, errors.New("error") } return int64(len(m.Data)), nil } +// Len returns the number of playlists in the repo in a thread-safe manner. +func (m *MockPlaylistRepo) Len() int { + m.mu.RLock() + defer m.mu.RUnlock() + return len(m.Data) +} + +// GetData returns a playlist by ID in a thread-safe manner. +func (m *MockPlaylistRepo) GetData(id string) (*model.Playlist, bool) { + m.mu.RLock() + defer m.mu.RUnlock() + pls, ok := m.Data[id] + return pls, ok +} + +// FindByPluginPlaylistID returns the first playlist with the given plugin playlist ID. +func (m *MockPlaylistRepo) FindByPluginPlaylistID(pluginPlaylistID string) *model.Playlist { + m.mu.RLock() + defer m.mu.RUnlock() + for _, pls := range m.Data { + if pls.PluginPlaylistID == pluginPlaylistID { + return pls + } + } + return nil +} + var _ model.PlaylistRepository = (*MockPlaylistRepo)(nil) From 0127b8c607e8979ba1ec5c6bca1a95253bd09b7e Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 15:37:48 -0500 Subject: [PATCH 13/24] 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()) From 03a36c5bddc74726a7b8d8002ac6678784999342 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 16:03:06 -0500 Subject: [PATCH 14/24] refactor: inject matcher.Matcher via Wire instead of creating it inline Replaced inline matcher.New(ds) calls in external.provider and PlaylistGenerator orchestrator with proper dependency injection via Google Wire. Added matcher.New to the Wire provider set, updated NewProvider and GetManager signatures to accept *matcher.Matcher, and deleted the trivial provider_matching.go wrapper. This eliminates tight coupling where each caller knew how to construct a Matcher, following the same DI pattern used by other core services. Signed-off-by: Deluan --- cmd/wire_gen.go | 23 ++++++++++++----------- core/metrics/insights.go | 2 +- plugins/manager.go | 5 ++++- plugins/manager_loader.go | 2 +- plugins/playlist_generator.go | 4 ++-- plugins/plugins_suite_test.go | 2 ++ 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index b25b4c100..890f9c60b 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -71,9 +71,9 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) + agentsAgents := agents.GetAgents(dataStore, manager) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) @@ -93,9 +93,9 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) + agentsAgents := agents.GetAgents(dataStore, manager) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) transcodingCache := stream.GetTranscodingCache() @@ -122,9 +122,9 @@ func CreatePublicRouter() *public.Router { fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) + agentsAgents := agents.GetAgents(dataStore, manager) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) transcodingCache := stream.GetTranscodingCache() @@ -170,9 +170,9 @@ func CreateScanner(ctx context.Context) model.Scanner { fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) + agentsAgents := agents.GetAgents(dataStore, manager) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) @@ -189,9 +189,9 @@ func CreateScanWatcher(ctx context.Context) scanner.Watcher { fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) + agentsAgents := agents.GetAgents(dataStore, manager) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) @@ -214,7 +214,8 @@ func getPluginManager() *plugins.Manager { dataStore := persistence.New(sqlDB) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) + matcherMatcher := matcher.New(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics, matcherMatcher) return manager } diff --git a/core/metrics/insights.go b/core/metrics/insights.go index f069d3fb6..db835bb42 100644 --- a/core/metrics/insights.go +++ b/core/metrics/insights.go @@ -324,7 +324,7 @@ func (c *insightsCollector) hasSmartPlaylists(ctx context.Context) (bool, error) // collectPlugins collects information about installed plugins func (c *insightsCollector) collectPlugins(_ context.Context) map[string]insights.PluginInfo { // TODO Fix import/inject cycles - manager := plugins.GetManager(c.ds, events.GetBroker(), nil) + manager := plugins.GetManager(c.ds, events.GetBroker(), nil, nil) info := manager.GetPluginInfo() result := make(map[string]insights.PluginInfo, len(info)) diff --git a/plugins/manager.go b/plugins/manager.go index 0c7c91ed8..3e6cc75fe 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/core/lyrics" + "github.com/navidrome/navidrome/core/matcher" "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -66,16 +67,18 @@ type Manager struct { ds model.DataStore broker events.Broker metrics PluginMetricsRecorder + matcher *matcher.Matcher } // GetManager returns a singleton instance of the plugin manager. // The manager is not started automatically; call Start() to begin loading plugins. -func GetManager(ds model.DataStore, broker events.Broker, m PluginMetricsRecorder) *Manager { +func GetManager(ds model.DataStore, broker events.Broker, m PluginMetricsRecorder, mt *matcher.Matcher) *Manager { return singleton.GetInstance(func() *Manager { return &Manager{ ds: ds, broker: broker, metrics: m, + matcher: mt, plugins: make(map[string]*plugin), } }) diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index e394911f7..406c2f183 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -394,7 +394,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { // 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) + orch := newPlaylistGeneratorOrchestrator(m.ctx, p.ID, loadedPlugin, m.ds, m.matcher) loadedPlugin.closers = append(loadedPlugin.closers, orch) go orch.run() } diff --git a/plugins/playlist_generator.go b/plugins/playlist_generator.go index 64766bb3a..a2a684228 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_generator.go @@ -68,13 +68,13 @@ type playlistGeneratorOrchestrator struct { done chan struct{} // closed when worker exits } -func newPlaylistGeneratorOrchestrator(parentCtx context.Context, pluginName string, p *plugin, ds model.DataStore) *playlistGeneratorOrchestrator { +func newPlaylistGeneratorOrchestrator(parentCtx context.Context, pluginName string, p *plugin, ds model.DataStore, m *matcher.Matcher) *playlistGeneratorOrchestrator { ctx, cancel := context.WithCancel(parentCtx) return &playlistGeneratorOrchestrator{ pluginName: pluginName, plugin: p, ds: ds, - matcher: matcher.New(ds), + matcher: m, ctx: ctx, cancel: cancel, workCh: make(chan workItem, workChCapacity), diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 41adbc133..42bf64e5b 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/matcher" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" @@ -145,6 +146,7 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s manager := &Manager{ plugins: make(map[string]*plugin), ds: dataStore, + matcher: matcher.New(dataStore), metrics: metrics, subsonicRouter: http.NotFoundHandler(), // Stub router for tests } From 04aa10f988783db16280271a5dd9dce5941bc340 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 16:51:07 -0500 Subject: [PATCH 15/24] refactor(plugins): rename PlaylistGenerator to PlaylistProvider Rename the capability from PlaylistGenerator to PlaylistProvider and the internal orchestrator struct from playlistGeneratorOrchestrator to playlistSyncer. The new names better describe what the capability does (provides playlists) rather than how it works internally. All source files, test plugin, PDK packages (Go/Rust), YAML schemas, and exported WASM function names are updated accordingly. --- ...list_generator.go => playlist_provider.go} | 22 ++--- ..._generator.yaml => playlist_provider.yaml} | 4 +- plugins/manager_loader.go | 10 +-- .../playlistprovider.go} | 34 ++++---- .../playlistprovider_stub.go} | 22 ++--- .../pdk/rust/nd-pdk-capabilities/src/lib.rs | 2 +- ...aylistgenerator.rs => playlistprovider.rs} | 36 ++++----- ...list_generator.go => playlist_provider.go} | 40 +++++----- ...ator_test.go => playlist_provider_test.go} | 80 +++++++++---------- plugins/plugins_suite_test.go | 2 +- .../test-playlist-generator/manifest.json | 6 -- .../go.mod | 2 +- .../go.sum | 0 .../main.go | 34 ++++---- .../test-playlist-provider/manifest.json | 6 ++ 15 files changed, 150 insertions(+), 150 deletions(-) rename plugins/capabilities/{playlist_generator.go => playlist_provider.go} (76%) rename plugins/capabilities/{playlist_generator.yaml => playlist_provider.yaml} (98%) rename plugins/pdk/go/{playlistgenerator/playlistgenerator.go => playlistprovider/playlistprovider.go} (82%) rename plugins/pdk/go/{playlistgenerator/playlistgenerator_stub.go => playlistprovider/playlistprovider_stub.go} (84%) rename plugins/pdk/rust/nd-pdk-capabilities/src/{playlistgenerator.rs => playlistprovider.rs} (82%) rename plugins/{playlist_generator.go => playlist_provider.go} (83%) rename plugins/{playlist_generator_test.go => playlist_provider_test.go} (62%) delete mode 100644 plugins/testdata/test-playlist-generator/manifest.json rename plugins/testdata/{test-playlist-generator => test-playlist-provider}/go.mod (93%) rename plugins/testdata/{test-playlist-generator => test-playlist-provider}/go.sum (100%) rename plugins/testdata/{test-playlist-generator => test-playlist-provider}/main.go (59%) create mode 100644 plugins/testdata/test-playlist-provider/manifest.json diff --git a/plugins/capabilities/playlist_generator.go b/plugins/capabilities/playlist_provider.go similarity index 76% rename from plugins/capabilities/playlist_generator.go rename to plugins/capabilities/playlist_provider.go index 4d034a7c0..72ebaca7d 100644 --- a/plugins/capabilities/playlist_generator.go +++ b/plugins/capabilities/playlist_provider.go @@ -1,18 +1,18 @@ package capabilities -// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// PlaylistProvider provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two // functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for // fetching the heavy payload (tracks, metadata). // -//nd:capability name=playlistgenerator required=true -type PlaylistGenerator interface { +//nd:capability name=playlistprovider required=true +type PlaylistProvider interface { // GetAvailablePlaylists returns the list of playlists this plugin provides. - //nd:export name=nd_playlist_generator_get_available_playlists + //nd:export name=nd_playlist_provider_get_available_playlists GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist returns the full data for a single playlist (tracks, metadata). - //nd:export name=nd_playlist_generator_get_playlist + //nd:export name=nd_playlist_provider_get_playlist GetPlaylist(GetPlaylistRequest) (GetPlaylistResponse, error) } @@ -60,13 +60,13 @@ type GetPlaylistResponse struct { ValidUntil int64 `json:"validUntil"` } -// PlaylistGeneratorError represents an error type for playlist generator operations. -type PlaylistGeneratorError string +// PlaylistProviderError represents an error type for playlist provider operations. +type PlaylistProviderError string const ( - // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. - PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" + // PlaylistProviderErrorNotFound indicates a playlist is currently unavailable. + PlaylistProviderErrorNotFound PlaylistProviderError = "playlist_provider(not_found)" ) -// Error implements the error interface for PlaylistGeneratorError. -func (e PlaylistGeneratorError) Error() string { return string(e) } +// Error implements the error interface for PlaylistProviderError. +func (e PlaylistProviderError) Error() string { return string(e) } diff --git a/plugins/capabilities/playlist_generator.yaml b/plugins/capabilities/playlist_provider.yaml similarity index 98% rename from plugins/capabilities/playlist_generator.yaml rename to plugins/capabilities/playlist_provider.yaml index 519387660..66604a6bc 100644 --- a/plugins/capabilities/playlist_generator.yaml +++ b/plugins/capabilities/playlist_provider.yaml @@ -1,6 +1,6 @@ version: v1-draft exports: - nd_playlist_generator_get_available_playlists: + nd_playlist_provider_get_available_playlists: description: GetAvailablePlaylists returns the list of playlists this plugin provides. input: $ref: '#/components/schemas/GetAvailablePlaylistsRequest' @@ -8,7 +8,7 @@ exports: output: $ref: '#/components/schemas/GetAvailablePlaylistsResponse' contentType: application/json - nd_playlist_generator_get_playlist: + nd_playlist_provider_get_playlist: description: GetPlaylist returns the full data for a single playlist (tracks, metadata). input: $ref: '#/components/schemas/GetPlaylistRequest' diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 406c2f183..895b944fb 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -391,12 +391,12 @@ 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 + // Start PlaylistProvider syncer if capability is detected loadedPlugin := m.plugins[p.ID] - if hasCapability(loadedPlugin.capabilities, CapabilityPlaylistGenerator) { - orch := newPlaylistGeneratorOrchestrator(m.ctx, p.ID, loadedPlugin, m.ds, m.matcher) - loadedPlugin.closers = append(loadedPlugin.closers, orch) - go orch.run() + if hasCapability(loadedPlugin.capabilities, CapabilityPlaylistProvider) { + syncer := newPlaylistSyncer(m.ctx, p.ID, loadedPlugin, m.ds, m.matcher) + loadedPlugin.closers = append(loadedPlugin.closers, syncer) + go syncer.run() } return nil diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator.go b/plugins/pdk/go/playlistprovider/playlistprovider.go similarity index 82% rename from plugins/pdk/go/playlistgenerator/playlistgenerator.go rename to plugins/pdk/go/playlistprovider/playlistprovider.go index 15453c13f..42a99af00 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator.go +++ b/plugins/pdk/go/playlistprovider/playlistprovider.go @@ -1,26 +1,26 @@ // Code generated by ndpgen. DO NOT EDIT. // -// This file contains export wrappers for the PlaylistGenerator capability. +// This file contains export wrappers for the PlaylistProvider capability. // It is intended for use in Navidrome plugins built with TinyGo. // //go:build wasip1 -package playlistgenerator +package playlistprovider import ( "github.com/navidrome/navidrome/plugins/pdk/go/pdk" ) -// PlaylistGeneratorError represents an error type for playlist generator operations. -type PlaylistGeneratorError string +// PlaylistProviderError represents an error type for playlist provider operations. +type PlaylistProviderError string const ( - // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. - PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" + // PlaylistProviderErrorNotFound indicates a playlist is currently unavailable. + PlaylistProviderErrorNotFound PlaylistProviderError = "playlist_provider(not_found)" ) -// Error implements the error interface for PlaylistGeneratorError. -func (e PlaylistGeneratorError) Error() string { return string(e) } +// Error implements the error interface for PlaylistProviderError. +func (e PlaylistProviderError) Error() string { return string(e) } // GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. type GetAvailablePlaylistsRequest struct { @@ -89,12 +89,12 @@ type SongRef struct { Duration float32 `json:"duration,omitempty"` } -// PlaylistGenerator requires all methods to be implemented. -// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// PlaylistProvider requires all methods to be implemented. +// PlaylistProvider provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two // functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for // fetching the heavy payload (tracks, metadata). -type PlaylistGenerator interface { +type PlaylistProvider interface { // GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). @@ -105,9 +105,9 @@ var ( playlistImpl func(GetPlaylistRequest) (GetPlaylistResponse, error) ) -// Register registers a playlistgenerator implementation. +// Register registers a playlistprovider implementation. // All methods are required. -func Register(impl PlaylistGenerator) { +func Register(impl PlaylistProvider) { availablePlaylistsImpl = impl.GetAvailablePlaylists playlistImpl = impl.GetPlaylist } @@ -116,8 +116,8 @@ func Register(impl PlaylistGenerator) { // The host recognizes this and skips the plugin gracefully. const NotImplementedCode int32 = -2 -//go:wasmexport nd_playlist_generator_get_available_playlists -func _NdPlaylistGeneratorGetAvailablePlaylists() int32 { +//go:wasmexport nd_playlist_provider_get_available_playlists +func _NdPlaylistProviderGetAvailablePlaylists() int32 { if availablePlaylistsImpl == nil { // Return standard code - host will skip this plugin gracefully return NotImplementedCode @@ -143,8 +143,8 @@ func _NdPlaylistGeneratorGetAvailablePlaylists() int32 { return 0 } -//go:wasmexport nd_playlist_generator_get_playlist -func _NdPlaylistGeneratorGetPlaylist() int32 { +//go:wasmexport nd_playlist_provider_get_playlist +func _NdPlaylistProviderGetPlaylist() int32 { if playlistImpl == nil { // Return standard code - host will skip this plugin gracefully return NotImplementedCode diff --git a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go b/plugins/pdk/go/playlistprovider/playlistprovider_stub.go similarity index 84% rename from plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go rename to plugins/pdk/go/playlistprovider/playlistprovider_stub.go index 348492a44..9fdff8e48 100644 --- a/plugins/pdk/go/playlistgenerator/playlistgenerator_stub.go +++ b/plugins/pdk/go/playlistprovider/playlistprovider_stub.go @@ -6,18 +6,18 @@ // //go:build !wasip1 -package playlistgenerator +package playlistprovider -// PlaylistGeneratorError represents an error type for playlist generator operations. -type PlaylistGeneratorError string +// PlaylistProviderError represents an error type for playlist provider operations. +type PlaylistProviderError string const ( - // PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. - PlaylistGeneratorErrorNotFound PlaylistGeneratorError = "playlist_generator(not_found)" + // PlaylistProviderErrorNotFound indicates a playlist is currently unavailable. + PlaylistProviderErrorNotFound PlaylistProviderError = "playlist_provider(not_found)" ) -// Error implements the error interface for PlaylistGeneratorError. -func (e PlaylistGeneratorError) Error() string { return string(e) } +// Error implements the error interface for PlaylistProviderError. +func (e PlaylistProviderError) Error() string { return string(e) } // GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. type GetAvailablePlaylistsRequest struct { @@ -86,12 +86,12 @@ type SongRef struct { Duration float32 `json:"duration,omitempty"` } -// PlaylistGenerator requires all methods to be implemented. -// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +// PlaylistProvider requires all methods to be implemented. +// PlaylistProvider provides dynamically-generated playlists (e.g., "Daily Mix", // personalized recommendations). Plugins implementing this capability expose two // functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for // fetching the heavy payload (tracks, metadata). -type PlaylistGenerator interface { +type PlaylistProvider interface { // GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. GetAvailablePlaylists(GetAvailablePlaylistsRequest) (GetAvailablePlaylistsResponse, error) // GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). @@ -103,4 +103,4 @@ const NotImplementedCode int32 = -2 // Register is a no-op on non-WASM platforms. // This stub allows code to compile outside of WASM. -func Register(_ PlaylistGenerator) {} +func Register(_ PlaylistProvider) {} diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs index f0f21b0e8..43b3f428c 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/lib.rs @@ -8,7 +8,7 @@ pub mod lifecycle; pub mod lyrics; pub mod metadata; -pub mod playlistgenerator; +pub mod playlistprovider; pub mod scheduler; pub mod scrobbler; pub mod taskworker; diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistprovider.rs similarity index 82% rename from plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs rename to plugins/pdk/rust/nd-pdk-capabilities/src/playlistprovider.rs index 9e9d3e555..de5da4516 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/playlistgenerator.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/playlistprovider.rs @@ -1,6 +1,6 @@ // Code generated by ndpgen. DO NOT EDIT. // -// This file contains export wrappers for the PlaylistGenerator capability. +// This file contains export wrappers for the PlaylistProvider capability. // It is intended for use in Navidrome plugins built with extism-pdk. use serde::{Deserialize, Serialize}; @@ -18,10 +18,10 @@ fn is_zero_u64(value: &u64) -> bool { *value == 0 } fn is_zero_f32(value: &f32) -> bool { *value == 0.0 } #[allow(dead_code)] fn is_zero_f64(value: &f64) -> bool { *value == 0.0 } -/// PlaylistGeneratorError represents an error type for playlist generator operations. -pub type PlaylistGeneratorError = &'static str; -/// PlaylistGeneratorErrorNotFound indicates a playlist is currently unavailable. -pub const PLAYLIST_GENERATOR_ERROR_NOT_FOUND: PlaylistGeneratorError = "playlist_generator(not_found)"; +/// PlaylistProviderError represents an error type for playlist provider operations. +pub type PlaylistProviderError = &'static str; +/// PlaylistProviderErrorNotFound indicates a playlist is currently unavailable. +pub const PLAYLIST_PROVIDER_ERROR_NOT_FOUND: PlaylistProviderError = "playlist_provider(not_found)"; /// GetAvailablePlaylistsRequest is the request for GetAvailablePlaylists. #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -136,37 +136,37 @@ impl Error { } } -/// PlaylistGenerator requires all methods to be implemented. -/// PlaylistGenerator provides dynamically-generated playlists (e.g., "Daily Mix", +/// PlaylistProvider requires all methods to be implemented. +/// PlaylistProvider provides dynamically-generated playlists (e.g., "Daily Mix", /// personalized recommendations). Plugins implementing this capability expose two /// functions: GetAvailablePlaylists for lightweight discovery and GetPlaylist for /// fetching the heavy payload (tracks, metadata). -pub trait PlaylistGenerator { +pub trait PlaylistProvider { /// GetAvailablePlaylists - GetAvailablePlaylists returns the list of playlists this plugin provides. fn get_available_playlists(&self, req: GetAvailablePlaylistsRequest) -> Result; /// GetPlaylist - GetPlaylist returns the full data for a single playlist (tracks, metadata). fn get_playlist(&self, req: GetPlaylistRequest) -> Result; } -/// Register all exports for the PlaylistGenerator capability. +/// Register all exports for the PlaylistProvider capability. /// This macro generates the WASM export functions for all trait methods. #[macro_export] -macro_rules! register_playlistgenerator { +macro_rules! register_playlistprovider { ($plugin_type:ty) => { #[extism_pdk::plugin_fn] - pub fn nd_playlist_generator_get_available_playlists( - req: extism_pdk::Json<$crate::playlistgenerator::GetAvailablePlaylistsRequest> - ) -> extism_pdk::FnResult> { + pub fn nd_playlist_provider_get_available_playlists( + req: extism_pdk::Json<$crate::playlistprovider::GetAvailablePlaylistsRequest> + ) -> extism_pdk::FnResult> { let plugin = <$plugin_type>::default(); - let result = $crate::playlistgenerator::PlaylistGenerator::get_available_playlists(&plugin, req.into_inner())?; + let result = $crate::playlistprovider::PlaylistProvider::get_available_playlists(&plugin, req.into_inner())?; Ok(extism_pdk::Json(result)) } #[extism_pdk::plugin_fn] - pub fn nd_playlist_generator_get_playlist( - req: extism_pdk::Json<$crate::playlistgenerator::GetPlaylistRequest> - ) -> extism_pdk::FnResult> { + pub fn nd_playlist_provider_get_playlist( + req: extism_pdk::Json<$crate::playlistprovider::GetPlaylistRequest> + ) -> extism_pdk::FnResult> { let plugin = <$plugin_type>::default(); - let result = $crate::playlistgenerator::PlaylistGenerator::get_playlist(&plugin, req.into_inner())?; + let result = $crate::playlistprovider::PlaylistProvider::get_playlist(&plugin, req.into_inner())?; Ok(extism_pdk::Json(result)) } }; diff --git a/plugins/playlist_generator.go b/plugins/playlist_provider.go similarity index 83% rename from plugins/playlist_generator.go rename to plugins/playlist_provider.go index a2a684228..aa28990ea 100644 --- a/plugins/playlist_generator.go +++ b/plugins/playlist_provider.go @@ -15,10 +15,10 @@ import ( ) const ( - CapabilityPlaylistGenerator Capability = "PlaylistGenerator" + CapabilityPlaylistProvider Capability = "PlaylistProvider" - FuncPlaylistGeneratorGetAvailablePlaylists = "nd_playlist_generator_get_available_playlists" - FuncPlaylistGeneratorGetPlaylist = "nd_playlist_generator_get_playlist" + FuncPlaylistProviderGetAvailablePlaylists = "nd_playlist_provider_get_available_playlists" + FuncPlaylistProviderGetPlaylist = "nd_playlist_provider_get_playlist" // workChCapacity is the buffer size for the work channel. workChCapacity = 16 @@ -29,9 +29,9 @@ const ( func init() { registerCapability( - CapabilityPlaylistGenerator, - FuncPlaylistGeneratorGetAvailablePlaylists, - FuncPlaylistGeneratorGetPlaylist, + CapabilityPlaylistProvider, + FuncPlaylistProviderGetAvailablePlaylists, + FuncPlaylistProviderGetPlaylist, ) } @@ -49,11 +49,11 @@ type workItem struct { ownerID string // only for workSync } -// playlistGeneratorOrchestrator manages playlist generation for a single plugin. +// playlistSyncer manages playlist synchronization for a single plugin. // All mutable state (refreshTimers, discoveryTimer) is owned exclusively by the // worker goroutine — no synchronization needed. The retryInterval and // refreshTimerCount fields use atomics so tests can observe them race-free. -type playlistGeneratorOrchestrator struct { +type playlistSyncer struct { pluginName string plugin *plugin ds model.DataStore @@ -68,9 +68,9 @@ type playlistGeneratorOrchestrator struct { done chan struct{} // closed when worker exits } -func newPlaylistGeneratorOrchestrator(parentCtx context.Context, pluginName string, p *plugin, ds model.DataStore, m *matcher.Matcher) *playlistGeneratorOrchestrator { +func newPlaylistSyncer(parentCtx context.Context, pluginName string, p *plugin, ds model.DataStore, m *matcher.Matcher) *playlistSyncer { ctx, cancel := context.WithCancel(parentCtx) - return &playlistGeneratorOrchestrator{ + return &playlistSyncer{ pluginName: pluginName, plugin: p, ds: ds, @@ -85,7 +85,7 @@ func newPlaylistGeneratorOrchestrator(parentCtx context.Context, pluginName stri // run is the single worker goroutine that processes all work items sequentially. // It performs an initial discovery before entering the main loop. -func (o *playlistGeneratorOrchestrator) run() { +func (o *playlistSyncer) run() { defer close(o.done) // Run initial discovery before entering the loop @@ -108,10 +108,10 @@ func (o *playlistGeneratorOrchestrator) run() { } // discoverAndSync calls GetAvailablePlaylists, then GetPlaylist for each, matches tracks, and upserts. -func (o *playlistGeneratorOrchestrator) discoverAndSync() { +func (o *playlistSyncer) discoverAndSync() { ctx := o.ctx resp, err := callPluginFunction[capabilities.GetAvailablePlaylistsRequest, capabilities.GetAvailablePlaylistsResponse]( - ctx, o.plugin, FuncPlaylistGeneratorGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, + ctx, o.plugin, FuncPlaylistProviderGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, ) if err != nil { log.Error(ctx, "Failed to call GetAvailablePlaylists, retrying later", "plugin", o.pluginName, err) @@ -144,10 +144,10 @@ func (o *playlistGeneratorOrchestrator) discoverAndSync() { } // syncPlaylist calls GetPlaylist, matches tracks, and upserts the playlist in the DB. -func (o *playlistGeneratorOrchestrator) syncPlaylist(info capabilities.PlaylistInfo, dbID string, ownerID string) { +func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID string, ownerID string) { ctx := o.ctx resp, err := callPluginFunction[capabilities.GetPlaylistRequest, capabilities.GetPlaylistResponse]( - ctx, o.plugin, FuncPlaylistGeneratorGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, + ctx, o.plugin, FuncPlaylistProviderGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, ) if err != nil { if isPlaylistNotFoundError(err) { @@ -221,7 +221,7 @@ func (o *playlistGeneratorOrchestrator) syncPlaylist(info capabilities.PlaylistI } } -func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { +func (o *playlistSyncer) schedulePlaylistRefresh(info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { // Cancel existing timer if any if timer, ok := o.refreshTimers[dbID]; ok { timer.Stop() @@ -235,7 +235,7 @@ func (o *playlistGeneratorOrchestrator) schedulePlaylistRefresh(info capabilitie o.refreshTimerCount.Store(int32(len(o.refreshTimers))) } -func (o *playlistGeneratorOrchestrator) scheduleDiscovery(delay time.Duration) { +func (o *playlistSyncer) scheduleDiscovery(delay time.Duration) { if o.discoveryTimer != nil { o.discoveryTimer.Stop() } @@ -249,11 +249,11 @@ func (o *playlistGeneratorOrchestrator) scheduleDiscovery(delay time.Duration) { // isPlaylistNotFoundError checks if the error contains a NotFound sentinel from the plugin. func isPlaylistNotFoundError(err error) bool { - return err != nil && strings.Contains(err.Error(), capabilities.PlaylistGeneratorErrorNotFound.Error()) + return err != nil && strings.Contains(err.Error(), capabilities.PlaylistProviderErrorNotFound.Error()) } // stopAllTimers stops the discovery timer and all refresh timers. -func (o *playlistGeneratorOrchestrator) stopAllTimers() { +func (o *playlistSyncer) stopAllTimers() { if o.discoveryTimer != nil { o.discoveryTimer.Stop() } @@ -263,7 +263,7 @@ func (o *playlistGeneratorOrchestrator) stopAllTimers() { } // Close cancels the context and waits for the worker goroutine to finish. -func (o *playlistGeneratorOrchestrator) Close() error { +func (o *playlistSyncer) Close() error { o.cancel() <-o.done return nil diff --git a/plugins/playlist_generator_test.go b/plugins/playlist_provider_test.go similarity index 62% rename from plugins/playlist_generator_test.go rename to plugins/playlist_provider_test.go index 2086e0436..7d18e04e6 100644 --- a/plugins/playlist_generator_test.go +++ b/plugins/playlist_provider_test.go @@ -12,8 +12,8 @@ import ( . "github.com/onsi/gomega" ) -// findOrchestrator finds the playlistGeneratorOrchestrator in a plugin's closers. -func findOrchestrator(m *Manager, pluginName string) *playlistGeneratorOrchestrator { +// findSyncer finds the playlistSyncer in a plugin's closers. +func findSyncer(m *Manager, pluginName string) *playlistSyncer { m.mu.RLock() defer m.mu.RUnlock() p, ok := m.plugins[pluginName] @@ -21,14 +21,14 @@ func findOrchestrator(m *Manager, pluginName string) *playlistGeneratorOrchestra return nil } for _, c := range p.closers { - if orch, ok := c.(*playlistGeneratorOrchestrator); ok { - return orch + if syncer, ok := c.(*playlistSyncer); ok { + return syncer } } return nil } -var _ = Describe("PlaylistGenerator", Ordered, func() { +var _ = Describe("PlaylistProvider", Ordered, func() { var ( pgManager *Manager mockPlsRepo *tests.MockPlaylistRepo @@ -36,26 +36,26 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { BeforeAll(func() { pgManager, _ = createTestManagerWithPlugins(nil, - "test-playlist-generator"+PackageExtension, + "test-playlist-provider"+PackageExtension, ) mockPlsRepo = pgManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) }) Describe("capability detection", func() { - It("detects the PlaylistGenerator capability", func() { - names := pgManager.PluginNames(string(CapabilityPlaylistGenerator)) - Expect(names).To(ContainElement("test-playlist-generator")) + It("detects the PlaylistProvider capability", func() { + names := pgManager.PluginNames(string(CapabilityPlaylistProvider)) + Expect(names).To(ContainElement("test-playlist-provider")) }) }) - Describe("orchestrator lifecycle", func() { - It("creates an orchestrator for the plugin", func() { - Expect(findOrchestrator(pgManager, "test-playlist-generator")).ToNot(BeNil()) + Describe("syncer lifecycle", func() { + It("creates a syncer for the plugin", func() { + Expect(findSyncer(pgManager, "test-playlist-provider")).ToNot(BeNil()) }) It("discovers and syncs playlists from the plugin", func() { - // The orchestrator runs discoverAndSync in a goroutine on Start(). + // The syncer runs discoverAndSync in a goroutine on Start(). // Give it a moment to complete. Eventually(func() int { return mockPlsRepo.Len() @@ -72,13 +72,13 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Expect(dailyMix1.Comment).To(Equal("Your personalized daily mix")) Expect(dailyMix1.ExternalImageURL).To(Equal("https://example.com/cover1.jpg")) Expect(dailyMix1.OwnerID).To(Equal("user-1")) - Expect(dailyMix1.PluginID).To(Equal("test-playlist-generator")) + Expect(dailyMix1.PluginID).To(Equal("test-playlist-provider")) Expect(dailyMix1.PluginPlaylistID).To(Equal("daily-mix-1")) Expect(dailyMix1.Public).To(BeFalse()) }) It("generates deterministic playlist IDs", func() { - expectedID := id.NewHash("test-playlist-generator", "daily-mix-1", "user-1") + expectedID := id.NewHash("test-playlist-provider", "daily-mix-1", "user-1") Eventually(func() bool { _, exists := mockPlsRepo.GetData(expectedID) return exists @@ -86,8 +86,8 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }) It("creates distinct IDs for different playlists", func() { - id1 := id.NewHash("test-playlist-generator", "daily-mix-1", "user-1") - id2 := id.NewHash("test-playlist-generator", "daily-mix-2", "user-1") + id1 := id.NewHash("test-playlist-provider", "daily-mix-1", "user-1") + id2 := id.NewHash("test-playlist-provider", "daily-mix-2", "user-1") Expect(id1).ToNot(Equal(id2)) Eventually(func() bool { @@ -101,15 +101,15 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Describe("GetAvailablePlaylists error handling", func() { It("handles plugin errors gracefully", func() { errManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ - "test-playlist-generator": {"error": "service unavailable"}, - }, "test-playlist-generator"+PackageExtension) + "test-playlist-provider": {"error": "service unavailable"}, + }, "test-playlist-provider"+PackageExtension) - // Should still have the orchestrator (error is logged, not fatal) - Expect(findOrchestrator(errManager, "test-playlist-generator")).ToNot(BeNil()) + // Should still have the syncer (error is logged, not fatal) + Expect(findSyncer(errManager, "test-playlist-provider")).ToNot(BeNil()) // But no playlists created errPlsRepo := errManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) - // The orchestrator was started but GetAvailablePlaylists returned error, + // The syncer was started but GetAvailablePlaylists returned error, // so no playlists should be created Consistently(func() int { return errPlsRepo.Len() @@ -120,14 +120,14 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Describe("GetPlaylist NotFound error", func() { It("skips playlists when plugin returns NotFound", func() { notFoundManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ - "test-playlist-generator": { + "test-playlist-provider": { "get_playlist_error": "playlist temporarily unavailable", - "get_playlist_error_type": string(capabilities.PlaylistGeneratorErrorNotFound), + "get_playlist_error_type": string(capabilities.PlaylistProviderErrorNotFound), }, - }, "test-playlist-generator"+PackageExtension) + }, "test-playlist-provider"+PackageExtension) - // Should still have the orchestrator - Expect(findOrchestrator(notFoundManager, "test-playlist-generator")).ToNot(BeNil()) + // Should still have the syncer + Expect(findSyncer(notFoundManager, "test-playlist-provider")).ToNot(BeNil()) // No playlists should be created (all returned NotFound) notFoundPlsRepo := notFoundManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) @@ -136,9 +136,9 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { }, "500ms").Should(Equal(0)) // No refresh timers should be scheduled for NotFound playlists - orch := findOrchestrator(notFoundManager, "test-playlist-generator") + syncer := findSyncer(notFoundManager, "test-playlist-provider") Eventually(func() int32 { - return orch.refreshTimerCount.Load() + return syncer.refreshTimerCount.Load() }).Should(Equal(int32(0))) }) }) @@ -146,18 +146,18 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { Describe("GetPlaylist transient error with RetryInterval", func() { It("stores retryInterval and schedules retry on transient errors", func() { retryManager, _ := createTestManagerWithPlugins(map[string]map[string]string{ - "test-playlist-generator": { + "test-playlist-provider": { "get_playlist_error": "temporary failure", "retry_interval": "60", }, - }, "test-playlist-generator"+PackageExtension) + }, "test-playlist-provider"+PackageExtension) - orch := findOrchestrator(retryManager, "test-playlist-generator") - Expect(orch).ToNot(BeNil()) + syncer := findSyncer(retryManager, "test-playlist-provider") + Expect(syncer).ToNot(BeNil()) // retryInterval should be stored from the response Eventually(func() time.Duration { - return time.Duration(orch.retryInterval.Load()) + return time.Duration(syncer.retryInterval.Load()) }).Should(Equal(60 * time.Second)) // No playlists should be created (GetPlaylist failed) @@ -168,22 +168,22 @@ var _ = Describe("PlaylistGenerator", Ordered, func() { // Refresh timers should be scheduled for transient errors Eventually(func() int32 { - return orch.refreshTimerCount.Load() + return syncer.refreshTimerCount.Load() }).Should(BeNumerically(">=", int32(1))) }) }) Describe("stop", func() { - It("stops the orchestrator when the manager stops", func() { + It("stops the syncer when the manager stops", func() { stopManager, _ := createTestManagerWithPlugins(nil, - "test-playlist-generator"+PackageExtension, + "test-playlist-provider"+PackageExtension, ) - Expect(findOrchestrator(stopManager, "test-playlist-generator")).ToNot(BeNil()) + Expect(findSyncer(stopManager, "test-playlist-provider")).ToNot(BeNil()) err := stopManager.Stop() Expect(err).ToNot(HaveOccurred()) - // After Stop(), the plugin is unloaded so findOrchestrator returns nil - Expect(findOrchestrator(stopManager, "test-playlist-generator")).To(BeNil()) + // After Stop(), the plugin is unloaded so findSyncer returns nil + Expect(findSyncer(stopManager, "test-playlist-provider")).To(BeNil()) }) }) }) diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index 42bf64e5b..d284577d5 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -136,7 +136,7 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s mockPluginRepo.SetData(enabledPlugins) // Pre-seed a mock user repo with a default user so that - // PlaylistGenerator's discoverAndSync can resolve usernames. + // PlaylistProvider's discoverAndSync can resolve usernames. mockUserRepo := tests.CreateMockUserRepo() _ = mockUserRepo.Put(&model.User{ID: "user-1", UserName: "admin"}) diff --git a/plugins/testdata/test-playlist-generator/manifest.json b/plugins/testdata/test-playlist-generator/manifest.json deleted file mode 100644 index 6874d089f..000000000 --- a/plugins/testdata/test-playlist-generator/manifest.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "name": "Test Playlist Generator", - "author": "Navidrome Test", - "version": "1.0.0", - "description": "A test playlist generator plugin for integration testing" -} diff --git a/plugins/testdata/test-playlist-generator/go.mod b/plugins/testdata/test-playlist-provider/go.mod similarity index 93% rename from plugins/testdata/test-playlist-generator/go.mod rename to plugins/testdata/test-playlist-provider/go.mod index 7539672b5..b09064953 100644 --- a/plugins/testdata/test-playlist-generator/go.mod +++ b/plugins/testdata/test-playlist-provider/go.mod @@ -1,4 +1,4 @@ -module test-playlist-generator +module test-playlist-provider go 1.25 diff --git a/plugins/testdata/test-playlist-generator/go.sum b/plugins/testdata/test-playlist-provider/go.sum similarity index 100% rename from plugins/testdata/test-playlist-generator/go.sum rename to plugins/testdata/test-playlist-provider/go.sum diff --git a/plugins/testdata/test-playlist-generator/main.go b/plugins/testdata/test-playlist-provider/main.go similarity index 59% rename from plugins/testdata/test-playlist-generator/main.go rename to plugins/testdata/test-playlist-provider/main.go index f98168b5c..41def98aa 100644 --- a/plugins/testdata/test-playlist-generator/main.go +++ b/plugins/testdata/test-playlist-provider/main.go @@ -1,4 +1,4 @@ -// Test playlist generator plugin for Navidrome plugin system integration tests. +// Test playlist provider plugin for Navidrome plugin system integration tests. package main import ( @@ -6,20 +6,20 @@ import ( "strconv" "github.com/navidrome/navidrome/plugins/pdk/go/pdk" - pg "github.com/navidrome/navidrome/plugins/pdk/go/playlistgenerator" + pp "github.com/navidrome/navidrome/plugins/pdk/go/playlistprovider" ) func init() { - pg.Register(&testPlaylistGenerator{}) + pp.Register(&testPlaylistProvider{}) } -type testPlaylistGenerator struct{} +type testPlaylistProvider struct{} -func (t *testPlaylistGenerator) GetAvailablePlaylists(_ pg.GetAvailablePlaylistsRequest) (pg.GetAvailablePlaylistsResponse, error) { +func (t *testPlaylistProvider) GetAvailablePlaylists(_ pp.GetAvailablePlaylistsRequest) (pp.GetAvailablePlaylistsResponse, error) { // Check for configured error errMsg, hasErr := pdk.GetConfig("error") if hasErr && errMsg != "" { - return pg.GetAvailablePlaylistsResponse{}, fmt.Errorf("%s", errMsg) + return pp.GetAvailablePlaylistsResponse{}, fmt.Errorf("%s", errMsg) } // Get the owner username from config (defaults to "admin") @@ -28,8 +28,8 @@ func (t *testPlaylistGenerator) GetAvailablePlaylists(_ pg.GetAvailablePlaylists ownerUsername = u } - resp := pg.GetAvailablePlaylistsResponse{ - Playlists: []pg.PlaylistInfo{ + resp := pp.GetAvailablePlaylistsResponse{ + Playlists: []pp.PlaylistInfo{ {ID: "daily-mix-1", OwnerUsername: ownerUsername}, {ID: "daily-mix-2", OwnerUsername: ownerUsername}, }, @@ -46,40 +46,40 @@ func (t *testPlaylistGenerator) GetAvailablePlaylists(_ pg.GetAvailablePlaylists return resp, nil } -func (t *testPlaylistGenerator) GetPlaylist(req pg.GetPlaylistRequest) (pg.GetPlaylistResponse, error) { +func (t *testPlaylistProvider) GetPlaylist(req pp.GetPlaylistRequest) (pp.GetPlaylistResponse, error) { // Check for configured error errMsg, hasErr := pdk.GetConfig("get_playlist_error") if hasErr && errMsg != "" { // Check if the error should be typed (e.g., NotFound) errType, _ := pdk.GetConfig("get_playlist_error_type") - if errType == pg.PlaylistGeneratorErrorNotFound.Error() { - return pg.GetPlaylistResponse{}, fmt.Errorf("%w: %s", pg.PlaylistGeneratorErrorNotFound, errMsg) + if errType == pp.PlaylistProviderErrorNotFound.Error() { + return pp.GetPlaylistResponse{}, fmt.Errorf("%w: %s", pp.PlaylistProviderErrorNotFound, errMsg) } - return pg.GetPlaylistResponse{}, fmt.Errorf("%s", errMsg) + return pp.GetPlaylistResponse{}, fmt.Errorf("%s", errMsg) } switch req.ID { case "daily-mix-1": - return pg.GetPlaylistResponse{ + return pp.GetPlaylistResponse{ Name: "Daily Mix 1", Description: "Your personalized daily mix", CoverArtURL: "https://example.com/cover1.jpg", - Tracks: []pg.SongRef{ + Tracks: []pp.SongRef{ {Name: "Song A", Artist: "Artist One"}, {Name: "Song B", Artist: "Artist Two"}, }, ValidUntil: 0, // Static, no refresh }, nil case "daily-mix-2": - return pg.GetPlaylistResponse{ + return pp.GetPlaylistResponse{ Name: "Daily Mix 2", - Tracks: []pg.SongRef{ + Tracks: []pp.SongRef{ {Name: "Song C", Artist: "Artist Three"}, }, ValidUntil: 0, }, nil default: - return pg.GetPlaylistResponse{}, fmt.Errorf("unknown playlist: %s", req.ID) + return pp.GetPlaylistResponse{}, fmt.Errorf("unknown playlist: %s", req.ID) } } diff --git a/plugins/testdata/test-playlist-provider/manifest.json b/plugins/testdata/test-playlist-provider/manifest.json new file mode 100644 index 000000000..f429c71d6 --- /dev/null +++ b/plugins/testdata/test-playlist-provider/manifest.json @@ -0,0 +1,6 @@ +{ + "name": "Test Playlist Provider", + "author": "Navidrome Test", + "version": "1.0.0", + "description": "A test playlist provider plugin for integration testing" +} From f9af67d45e3f061767ba2f2a0ed2f5d8f7988eeb Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 17:08:29 -0500 Subject: [PATCH 16/24] refactor(plugins): streamline plugin loading and synchronization logic Signed-off-by: Deluan --- plugins/manager_loader.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 895b944fb..b2e4e520b 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -373,8 +373,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return fmt.Errorf("manifest validation: %w", err) } - m.mu.Lock() - m.plugins[p.ID] = &plugin{ + loadedPlugin := &plugin{ name: p.ID, path: p.Path, manifest: pkg.Manifest, @@ -386,13 +385,14 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { allUsers: p.AllUsers, libraries: newLibraryAccess(allowedLibraries, p.AllLibraries), } + m.mu.Lock() + m.plugins[p.ID] = loadedPlugin m.mu.Unlock() // Call plugin init function callPluginInit(ctx, m.plugins[p.ID]) // Start PlaylistProvider syncer if capability is detected - loadedPlugin := m.plugins[p.ID] if hasCapability(loadedPlugin.capabilities, CapabilityPlaylistProvider) { syncer := newPlaylistSyncer(m.ctx, p.ID, loadedPlugin, m.ds, m.matcher) loadedPlugin.closers = append(loadedPlugin.closers, syncer) From 657119fcb37a1293563ab4c6e11e8069e77cce28 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 17:11:31 -0500 Subject: [PATCH 17/24] refactor(plugins): increase work channel capacity for improved playlist processing Signed-off-by: Deluan --- plugins/playlist_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index aa28990ea..c1cb326aa 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -21,7 +21,7 @@ const ( FuncPlaylistProviderGetPlaylist = "nd_playlist_provider_get_playlist" // workChCapacity is the buffer size for the work channel. - workChCapacity = 16 + workChCapacity = 64 // discoveryRetryDelay is how long to wait before retrying a failed GetAvailablePlaylists call. discoveryRetryDelay = 5 * time.Minute From 9053a4ffe946073ee151c56aa78fb7c11176bbab Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 17:37:26 -0500 Subject: [PATCH 18/24] feat(plugins): require users permission for PlaylistProvider and validate owner PlaylistProvider capability now requires 'users' permission in the manifest (matching existing Scrobbler behavior) and validates that the resolved owner user ID is in the plugin's allowed users list before creating playlists. --- plugins/manifest.go | 7 +++ plugins/manifest_test.go | 43 +++++++++++++++++++ plugins/playlist_provider.go | 9 ++++ plugins/playlist_provider_test.go | 36 ++++++++++++++++ plugins/plugins_suite_test.go | 27 +++++++++++- .../test-playlist-provider/manifest.json | 7 ++- 6 files changed, 126 insertions(+), 3 deletions(-) diff --git a/plugins/manifest.go b/plugins/manifest.go index 7484718e3..bd04e6e52 100644 --- a/plugins/manifest.go +++ b/plugins/manifest.go @@ -65,6 +65,13 @@ func ValidateWithCapabilities(m *Manifest, capabilities []Capability) error { } } + // PlaylistProvider capability requires users permission + if hasCapability(capabilities, CapabilityPlaylistProvider) { + if m.Permissions == nil || m.Permissions.Users == nil { + return fmt.Errorf("playlist provider capability requires 'users' permission to be declared in manifest") + } + } + // Scheduler permission requires SchedulerCallback capability if m.Permissions != nil && m.Permissions.Scheduler != nil { if !hasCapability(capabilities, CapabilityScheduler) { diff --git a/plugins/manifest_test.go b/plugins/manifest_test.go index c45a480eb..723749cb3 100644 --- a/plugins/manifest_test.go +++ b/plugins/manifest_test.go @@ -463,5 +463,48 @@ var _ = Describe("Manifest", func() { err := ValidateWithCapabilities(m, []Capability{}) Expect(err).ToNot(HaveOccurred()) }) + + It("validates playlist provider capability with users permission", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + Permissions: &Permissions{ + Users: &UsersPermission{}, + }, + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).ToNot(HaveOccurred()) + }) + + It("returns error when playlist provider capability without users permission", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("playlist provider")) + Expect(err.Error()).To(ContainSubstring("users")) + }) + + It("returns error when playlist provider has permissions but no users", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + Permissions: &Permissions{ + Http: &HTTPPermission{}, + }, + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("playlist provider")) + Expect(err.Error()).To(ContainSubstring("users")) + }) }) }) diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index c1cb326aa..b26fc0629 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -3,6 +3,7 @@ package plugins import ( "context" "fmt" + "slices" "strings" "sync/atomic" "time" @@ -133,6 +134,14 @@ func (o *playlistSyncer) discoverAndSync() { continue } ownerID := user.ID + + // Validate that the plugin is permitted to create playlists for this user + if !o.plugin.allUsers && !slices.Contains(o.plugin.allowedUserIDs, ownerID) { + log.Error(ctx, "Plugin not permitted to create playlists for user", "plugin", o.pluginName, + "playlistID", info.ID, "username", info.OwnerUsername) + continue + } + dbID := id.NewHash(o.pluginName, info.ID, ownerID) o.syncPlaylist(info, dbID, ownerID) } diff --git a/plugins/playlist_provider_test.go b/plugins/playlist_provider_test.go index 7d18e04e6..a72ef357f 100644 --- a/plugins/playlist_provider_test.go +++ b/plugins/playlist_provider_test.go @@ -173,6 +173,42 @@ var _ = Describe("PlaylistProvider", Ordered, func() { }) }) + Describe("user permission validation", func() { + It("skips playlists for unauthorized users when AllUsers is false", func() { + // Create manager with restricted users — only "other-user" is allowed, + // but the plugin returns playlists for "admin" which resolves to "user-1" + restrictedManager, _ := createTestManagerWithPluginOverrides(nil, + map[string]pluginOverride{ + "test-playlist-provider": {AllUsers: false, Users: `["other-user"]`}, + }, + "test-playlist-provider"+PackageExtension, + ) + + // No playlists should be created because "user-1" is not in allowed users + restrictedPlsRepo := restrictedManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) + Consistently(func() int { + return restrictedPlsRepo.Len() + }, "500ms").Should(Equal(0)) + }) + + It("creates playlists for authorized users when AllUsers is false", func() { + // Create manager with restricted users — "user-1" is allowed, + // and the plugin returns playlists for "admin" which resolves to "user-1" + allowedManager, _ := createTestManagerWithPluginOverrides(nil, + map[string]pluginOverride{ + "test-playlist-provider": {AllUsers: false, Users: `["user-1"]`}, + }, + "test-playlist-provider"+PackageExtension, + ) + + // Playlists should be created because "user-1" is in allowed users + allowedPlsRepo := allowedManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) + Eventually(func() int { + return allowedPlsRepo.Len() + }).Should(BeNumerically(">=", 2)) + }) + }) + Describe("stop", func() { It("stops the syncer when the manager stops", func() { stopManager, _ := createTestManagerWithPlugins(nil, diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index d284577d5..7f7d95f05 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -82,10 +82,26 @@ func createTestManagerWithPlugins(pluginConfig map[string]map[string]string, plu return createTestManagerWithPluginsAndMetrics(pluginConfig, noopMetricsRecorder{}, plugins...) } +// pluginOverride allows tests to override model.Plugin fields for specific plugins. +type pluginOverride struct { + AllUsers bool + Users string // JSON array of user IDs, e.g. `["user-1"]` +} + +// createTestManagerWithPluginOverrides creates a new plugin Manager with the given plugin config, +// per-plugin overrides, and specified plugins. +func createTestManagerWithPluginOverrides(pluginConfig map[string]map[string]string, overrides map[string]pluginOverride, plugins ...string) (*Manager, string) { + return createTestManagerFull(pluginConfig, overrides, noopMetricsRecorder{}, plugins...) +} + // createTestManagerWithPluginsAndMetrics creates a new plugin Manager with the given plugin config, // metrics recorder, and specified plugins. It creates a temp directory, copies the specified plugins, and starts the manager. // Returns the manager and temp directory path. func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]string, metrics PluginMetricsRecorder, plugins ...string) (*Manager, string) { + return createTestManagerFull(pluginConfig, nil, metrics, plugins...) +} + +func createTestManagerFull(pluginConfig map[string]map[string]string, overrides map[string]pluginOverride, metrics PluginMetricsRecorder, plugins ...string) (*Manager, string) { // Create temp directory tmpDir, err := os.MkdirTemp("", "plugins-test-*") Expect(err).ToNot(HaveOccurred()) @@ -114,14 +130,21 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s configJSON = string(configBytes) } - enabledPlugins = append(enabledPlugins, model.Plugin{ + p := model.Plugin{ ID: pluginName, Path: destPath, SHA256: hashHex, Enabled: true, Config: configJSON, AllUsers: true, // Allow all users by default in tests - }) + } + if overrides != nil { + if o, ok := overrides[pluginName]; ok { + p.AllUsers = o.AllUsers + p.Users = o.Users + } + } + enabledPlugins = append(enabledPlugins, p) } // Setup config diff --git a/plugins/testdata/test-playlist-provider/manifest.json b/plugins/testdata/test-playlist-provider/manifest.json index f429c71d6..7efec1b2a 100644 --- a/plugins/testdata/test-playlist-provider/manifest.json +++ b/plugins/testdata/test-playlist-provider/manifest.json @@ -2,5 +2,10 @@ "name": "Test Playlist Provider", "author": "Navidrome Test", "version": "1.0.0", - "description": "A test playlist provider plugin for integration testing" + "description": "A test playlist provider plugin for integration testing", + "permissions": { + "users": { + "reason": "Required for playlist ownership" + } + } } From 73203eeef01fd90fd8cdc757b185f959317b1dfd Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 17:55:59 -0500 Subject: [PATCH 19/24] refactor: simplify and improve playlist provider and matcher code Eliminate redundant work and minor issues found during code review: - Replace manual PlaylistTrack construction in syncPlaylist with the existing Playlist.AddMediaFiles helper, removing duplicated logic - Pre-sanitize track fields once per artist batch in the matcher's fuzzy matching loop, avoiding redundant sanitization in both findBestMatch and computeSpecificityLevel on every iteration - Cache resolved usernames in discoverAndSync to avoid N+1 DB lookups when multiple playlists share the same owner - Use the local loadedPlugin variable instead of reading m.plugins[p.ID] after releasing the lock in loadPluginWithConfig - Fix misleading uint32 comparison (<=0 to ==0) in durationProximity - Update stale comment on checkTracksEditable to mention plugin playlists --- core/playlists/playlists.go | 2 +- plugins/manager_loader.go | 2 +- plugins/playlist_provider.go | 31 +++++++++++++------------------ 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index cebb27b77..ca1eb60a7 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -212,7 +212,7 @@ func (s *playlists) checkWritable(ctx context.Context, id string) (*model.Playli return pls, nil } -// checkTracksEditable verifies the user can modify tracks (ownership + not smart playlist). +// checkTracksEditable verifies the user can modify tracks (ownership + not smart/plugin playlist). func (s *playlists) checkTracksEditable(ctx context.Context, playlistID string) (*model.Playlist, error) { pls, err := s.checkWritable(ctx, playlistID) if err != nil { diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index b2e4e520b..150169a8f 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -390,7 +390,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { m.mu.Unlock() // Call plugin init function - callPluginInit(ctx, m.plugins[p.ID]) + callPluginInit(ctx, loadedPlugin) // Start PlaylistProvider syncer if capability is detected if hasCapability(loadedPlugin.capabilities, CapabilityPlaylistProvider) { diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index b26fc0629..7a36423fb 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -2,7 +2,6 @@ package plugins import ( "context" - "fmt" "slices" "strings" "sync/atomic" @@ -125,15 +124,20 @@ func (o *playlistSyncer) discoverAndSync() { o.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) } + resolvedUsers := map[string]string{} // username -> userID cache for _, info := range resp.Playlists { - // Resolve username to user ID - user, err := o.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) - if err != nil { - log.Error(ctx, "Failed to resolve playlist owner", "plugin", o.pluginName, - "playlistID", info.ID, "username", info.OwnerUsername, err) - continue + // Resolve username to user ID (cached) + ownerID, ok := resolvedUsers[info.OwnerUsername] + if !ok { + user, err := o.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) + if err != nil { + log.Error(ctx, "Failed to resolve playlist owner", "plugin", o.pluginName, + "playlistID", info.ID, "username", info.OwnerUsername, err) + continue + } + ownerID = user.ID + resolvedUsers[info.OwnerUsername] = ownerID } - ownerID := user.ID // Validate that the plugin is permitted to create playlists for this user if !o.plugin.allUsers && !slices.Contains(o.plugin.allowedUserIDs, ownerID) { @@ -198,16 +202,7 @@ func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin } // Set tracks from matched media files - tracks := make(model.PlaylistTracks, len(matched)) - for i, mf := range matched { - tracks[i] = model.PlaylistTrack{ - ID: fmt.Sprintf("%d", i+1), - MediaFileID: mf.ID, - PlaylistID: dbID, - MediaFile: mf, - } - } - pls.SetTracks(tracks) + pls.AddMediaFiles(matched) // Upsert via repository plsRepo := o.ds.Playlist(ctx) From 0a67142f74c6de55d3074319381988f580368585 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 18:03:27 -0500 Subject: [PATCH 20/24] refactor: update playlistSyncer methods to use consistent receiver naming Signed-off-by: Deluan --- plugins/playlist_provider.go | 114 +++++++++++++++++------------------ 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index 7a36423fb..ab220ba1f 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -85,43 +85,43 @@ func newPlaylistSyncer(parentCtx context.Context, pluginName string, p *plugin, // run is the single worker goroutine that processes all work items sequentially. // It performs an initial discovery before entering the main loop. -func (o *playlistSyncer) run() { - defer close(o.done) +func (p *playlistSyncer) run() { + defer close(p.done) // Run initial discovery before entering the loop - o.discoverAndSync() + p.discoverAndSync() for { select { - case <-o.ctx.Done(): - o.stopAllTimers() + case <-p.ctx.Done(): + p.stopAllTimers() return - case item := <-o.workCh: + case item := <-p.workCh: switch item.typ { case workDiscover: - o.discoverAndSync() + p.discoverAndSync() case workSync: - o.syncPlaylist(item.info, item.dbID, item.ownerID) + p.syncPlaylist(item.info, item.dbID, item.ownerID) } } } } // discoverAndSync calls GetAvailablePlaylists, then GetPlaylist for each, matches tracks, and upserts. -func (o *playlistSyncer) discoverAndSync() { - ctx := o.ctx +func (p *playlistSyncer) discoverAndSync() { + ctx := p.ctx resp, err := callPluginFunction[capabilities.GetAvailablePlaylistsRequest, capabilities.GetAvailablePlaylistsResponse]( - ctx, o.plugin, FuncPlaylistProviderGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, + ctx, p.plugin, FuncPlaylistProviderGetAvailablePlaylists, capabilities.GetAvailablePlaylistsRequest{}, ) if err != nil { - log.Error(ctx, "Failed to call GetAvailablePlaylists, retrying later", "plugin", o.pluginName, err) - o.scheduleDiscovery(discoveryRetryDelay) + log.Error(ctx, "Failed to call GetAvailablePlaylists, retrying later", "plugin", p.pluginName, err) + p.scheduleDiscovery(discoveryRetryDelay) return } // Store retry interval from response if resp.RetryInterval > 0 { - o.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) + p.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) } resolvedUsers := map[string]string{} // username -> userID cache @@ -129,9 +129,9 @@ func (o *playlistSyncer) discoverAndSync() { // Resolve username to user ID (cached) ownerID, ok := resolvedUsers[info.OwnerUsername] if !ok { - user, err := o.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) + user, err := p.ds.User(adminContext(ctx)).FindByUsername(info.OwnerUsername) if err != nil { - log.Error(ctx, "Failed to resolve playlist owner", "plugin", o.pluginName, + log.Error(ctx, "Failed to resolve playlist owner", "plugin", p.pluginName, "playlistID", info.ID, "username", info.OwnerUsername, err) continue } @@ -140,52 +140,52 @@ func (o *playlistSyncer) discoverAndSync() { } // Validate that the plugin is permitted to create playlists for this user - if !o.plugin.allUsers && !slices.Contains(o.plugin.allowedUserIDs, ownerID) { - log.Error(ctx, "Plugin not permitted to create playlists for user", "plugin", o.pluginName, + if !p.plugin.allUsers && !slices.Contains(p.plugin.allowedUserIDs, ownerID) { + log.Error(ctx, "Plugin not permitted to create playlists for user", "plugin", p.pluginName, "playlistID", info.ID, "username", info.OwnerUsername) continue } - dbID := id.NewHash(o.pluginName, info.ID, ownerID) - o.syncPlaylist(info, dbID, ownerID) + dbID := id.NewHash(p.pluginName, info.ID, ownerID) + p.syncPlaylist(info, dbID, ownerID) } // Schedule re-discovery if RefreshInterval > 0 if resp.RefreshInterval > 0 { - o.scheduleDiscovery(time.Duration(resp.RefreshInterval) * time.Second) + p.scheduleDiscovery(time.Duration(resp.RefreshInterval) * time.Second) } } // syncPlaylist calls GetPlaylist, matches tracks, and upserts the playlist in the DB. -func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID string, ownerID string) { - ctx := o.ctx +func (p *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID string, ownerID string) { + ctx := p.ctx resp, err := callPluginFunction[capabilities.GetPlaylistRequest, capabilities.GetPlaylistResponse]( - ctx, o.plugin, FuncPlaylistProviderGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, + ctx, p.plugin, FuncPlaylistProviderGetPlaylist, capabilities.GetPlaylistRequest{ID: info.ID}, ) if err != nil { if isPlaylistNotFoundError(err) { - log.Info(ctx, "Playlist not found, skipping", "plugin", o.pluginName, "playlistID", info.ID) + log.Info(ctx, "Playlist not found, skipping", "plugin", p.pluginName, "playlistID", info.ID) // Stop any existing refresh timer for this playlist - if timer, ok := o.refreshTimers[dbID]; ok { + if timer, ok := p.refreshTimers[dbID]; ok { timer.Stop() - delete(o.refreshTimers, dbID) - o.refreshTimerCount.Store(int32(len(o.refreshTimers))) + delete(p.refreshTimers, dbID) + p.refreshTimerCount.Store(int32(len(p.refreshTimers))) } return } - log.Warn(ctx, "Failed to call GetPlaylist", "plugin", o.pluginName, "playlistID", info.ID, err) + log.Warn(ctx, "Failed to call GetPlaylist", "plugin", p.pluginName, "playlistID", info.ID, err) // Schedule retry for transient errors if retryInterval is configured - if ri := time.Duration(o.retryInterval.Load()); ri > 0 { - o.schedulePlaylistRefresh(info, dbID, ownerID, ri) + if ri := time.Duration(p.retryInterval.Load()); ri > 0 { + p.schedulePlaylistRefresh(info, dbID, ownerID, ri) } return } // Convert SongRef → agents.Song and match against library songs := songRefsToAgentSongs(resp.Tracks) - matched, err := o.matcher.MatchSongsToLibrary(ctx, songs, len(songs)) + matched, err := p.matcher.MatchSongsToLibrary(ctx, songs, len(songs)) if err != nil { - log.Error(ctx, "Failed to match songs to library", "plugin", o.pluginName, "playlistID", info.ID, err) + log.Error(ctx, "Failed to match songs to library", "plugin", p.pluginName, "playlistID", info.ID, err) return } @@ -197,7 +197,7 @@ func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin OwnerID: ownerID, Public: false, ExternalImageURL: resp.CoverArtURL, - PluginID: o.pluginName, + PluginID: p.pluginName, PluginPlaylistID: info.ID, } @@ -205,13 +205,13 @@ func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin pls.AddMediaFiles(matched) // Upsert via repository - plsRepo := o.ds.Playlist(ctx) + plsRepo := p.ds.Playlist(ctx) if err := plsRepo.Put(pls); err != nil { - log.Error(ctx, "Failed to upsert plugin playlist", "plugin", o.pluginName, "playlistID", info.ID, err) + log.Error(ctx, "Failed to upsert plugin playlist", "plugin", p.pluginName, "playlistID", info.ID, err) return } - log.Info(ctx, "Synced plugin playlist", "plugin", o.pluginName, "playlistID", info.ID, + log.Info(ctx, "Synced plugin playlist", "plugin", p.pluginName, "playlistID", info.ID, "name", resp.Name, "tracks", len(matched), "owner", ownerID) // Schedule refresh if ValidUntil > 0 @@ -221,32 +221,32 @@ func (o *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin if delay <= 0 { delay = 1 * time.Second // Already expired, refresh soon } - o.schedulePlaylistRefresh(info, dbID, ownerID, delay) + p.schedulePlaylistRefresh(info, dbID, ownerID, delay) } } -func (o *playlistSyncer) schedulePlaylistRefresh(info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { +func (p *playlistSyncer) schedulePlaylistRefresh(info capabilities.PlaylistInfo, dbID string, ownerID string, delay time.Duration) { // Cancel existing timer if any - if timer, ok := o.refreshTimers[dbID]; ok { + if timer, ok := p.refreshTimers[dbID]; ok { timer.Stop() } - o.refreshTimers[dbID] = time.AfterFunc(delay, func() { + p.refreshTimers[dbID] = time.AfterFunc(delay, func() { select { - case o.workCh <- workItem{typ: workSync, info: info, dbID: dbID, ownerID: ownerID}: - case <-o.ctx.Done(): + case p.workCh <- workItem{typ: workSync, info: info, dbID: dbID, ownerID: ownerID}: + case <-p.ctx.Done(): } }) - o.refreshTimerCount.Store(int32(len(o.refreshTimers))) + p.refreshTimerCount.Store(int32(len(p.refreshTimers))) } -func (o *playlistSyncer) scheduleDiscovery(delay time.Duration) { - if o.discoveryTimer != nil { - o.discoveryTimer.Stop() +func (p *playlistSyncer) scheduleDiscovery(delay time.Duration) { + if p.discoveryTimer != nil { + p.discoveryTimer.Stop() } - o.discoveryTimer = time.AfterFunc(delay, func() { + p.discoveryTimer = time.AfterFunc(delay, func() { select { - case o.workCh <- workItem{typ: workDiscover}: - case <-o.ctx.Done(): + case p.workCh <- workItem{typ: workDiscover}: + case <-p.ctx.Done(): } }) } @@ -257,18 +257,18 @@ func isPlaylistNotFoundError(err error) bool { } // stopAllTimers stops the discovery timer and all refresh timers. -func (o *playlistSyncer) stopAllTimers() { - if o.discoveryTimer != nil { - o.discoveryTimer.Stop() +func (p *playlistSyncer) stopAllTimers() { + if p.discoveryTimer != nil { + p.discoveryTimer.Stop() } - for _, timer := range o.refreshTimers { + for _, timer := range p.refreshTimers { timer.Stop() } } // Close cancels the context and waits for the worker goroutine to finish. -func (o *playlistSyncer) Close() error { - o.cancel() - <-o.done +func (p *playlistSyncer) Close() error { + p.cancel() + <-p.done return nil } From a5fd18dc67517046d8e7c102f6c11156391b7829 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 18:45:45 -0500 Subject: [PATCH 21/24] feat: treat plugin playlists as read-only everywhere Enforce read-only restrictions for plugin playlists (PluginID != "") consistently across backend and UI, matching the existing smart playlist behavior. The Subsonic buildOSPlaylist now uses IsReadOnly() to mark both smart and plugin playlists as readonly. Backend Update and REST adapter block name/comment changes on plugin playlists while still allowing public toggle and cover art. The UI adds isPluginPlaylist helper to canChangeTracks, disables name/comment inputs in PlaylistEdit, disables auto-import toggle in PlaylistList, and adds a server-side readonly filter to exclude both smart and plugin playlists from the "add to playlist" dialog. --- core/playlists/playlists.go | 4 +++ core/playlists/playlists_test.go | 21 ++++++++++++ core/playlists/rest_adapter.go | 4 +++ core/playlists/rest_adapter_test.go | 36 +++++++++++++++++++++ model/playlist.go | 4 +++ persistence/playlist_repository.go | 12 +++++-- server/subsonic/playlists.go | 5 +-- server/subsonic/playlists_test.go | 35 ++++++++++++++++++++ ui/src/common/playlistUtils.js | 4 ++- ui/src/common/playlistUtils.test.js | 19 +++++++++++ ui/src/dialogs/AddToPlaylistDialog.test.jsx | 2 +- ui/src/dialogs/SelectPlaylistInput.jsx | 2 +- ui/src/dialogs/SelectPlaylistInput.test.jsx | 2 +- ui/src/playlist/PlaylistEdit.jsx | 9 ++++-- ui/src/playlist/PlaylistList.jsx | 3 +- 15 files changed, 151 insertions(+), 11 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index ca1eb60a7..4ba6e76fd 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -162,6 +162,10 @@ func (s *playlists) Update(ctx context.Context, playlistID string, if err != nil { return err } + // Plugin playlists allow public toggle and cover art, but block name/comment changes + if pls.IsPluginPlaylist() && (name != nil || comment != nil) { + return model.ErrNotAuthorized + } return s.ds.WithTxImmediate(func(tx model.DataStore) error { repo := tx.Playlist(ctx) diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index 12ff44010..86cac3c11 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -210,6 +210,27 @@ var _ = Describe("Playlists", func() { err := ps.Update(ctx, "pls-smart", &newName, nil, nil, nil, nil) Expect(err).ToNot(HaveOccurred()) }) + + It("denies name update on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + newName := "New Name" + err := ps.Update(ctx, "pls-plugin", &newName, nil, nil, nil, nil) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + + It("denies comment update on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + newComment := "New Comment" + err := ps.Update(ctx, "pls-plugin", nil, &newComment, nil, nil, nil) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + + It("allows public toggle on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + public := true + err := ps.Update(ctx, "pls-plugin", nil, nil, &public, nil, nil) + Expect(err).ToNot(HaveOccurred()) + }) }) Describe("AddTracks", func() { diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 3fecda0d5..33073e7da 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -93,6 +93,10 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity if !usr.IsAdmin && entity.OwnerID != "" && entity.OwnerID != current.OwnerID { return rest.ErrPermissionDenied } + // Plugin playlists allow public and ownership changes, but block name/comment + if current.IsPluginPlaylist() && (entity.Name != current.Name || entity.Comment != current.Comment) { + return rest.ErrPermissionDenied + } // Apply ownership change (admin only) if entity.OwnerID != "" { current.OwnerID = entity.OwnerID diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 097bc6310..05f4b3be2 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -102,6 +102,42 @@ var _ = Describe("REST Adapter", func() { Expect(err).ToNot(HaveOccurred()) }) + It("denies name change on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Changed Name", Comment: ""} + err := repo.Update("pls-plugin", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("denies comment change on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", Comment: "", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Plugin PL", Comment: "new comment"} + err := repo.Update("pls-plugin", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("allows public toggle on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Plugin PL", Public: true} + err := repo.Update("pls-plugin", pls) + Expect(err).ToNot(HaveOccurred()) + }) + It("allows admin to update any playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true}) repo = ps.NewRepository(ctx).(rest.Persistable) diff --git a/model/playlist.go b/model/playlist.go index 0bcf20ad0..d9db3f4d2 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -36,6 +36,10 @@ type Playlist struct { PluginPlaylistID string `structs:"plugin_playlist_id" json:"pluginPlaylistId,omitempty"` } +func (pls Playlist) IsReadOnly() bool { + return pls.IsSmartPlaylist() || pls.IsPluginPlaylist() +} + func (pls Playlist) IsSmartPlaylist() bool { return pls.Rules != nil && pls.Rules.Expression != nil } diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 8d1bbe0f8..de50d5c3d 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -52,8 +52,9 @@ func NewPlaylistRepository(ctx context.Context, db dbx.Builder) model.PlaylistRe r.ctx = ctx r.db = db r.registerModel(&model.Playlist{}, map[string]filterFunc{ - "q": playlistFilter, - "smart": smartPlaylistFilter, + "q": playlistFilter, + "smart": smartPlaylistFilter, + "readonly": readonlyPlaylistFilter, }) r.setSortMappings(map[string]string{ "owner_name": "owner_name", @@ -75,6 +76,13 @@ func smartPlaylistFilter(string, any) Sqlizer { } } +func readonlyPlaylistFilter(string, any) Sqlizer { + return And{ + smartPlaylistFilter("", nil), + Or{Eq{"plugin_id": ""}, Eq{"plugin_id": nil}}, + } +} + func (r *playlistRepository) userFilter() Sqlizer { user := loggedUser(r.ctx) if user.IsAdmin { diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index a8c3da68c..fc0b1cd91 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -165,10 +165,11 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso } pls := responses.OpenSubsonicPlaylist{} - if p.IsSmartPlaylist() { + if p.IsReadOnly() { pls.Readonly = true - if p.EvaluatedAt != nil { + // ValidUntil only applies to smart playlists + if p.IsSmartPlaylist() && p.EvaluatedAt != nil { pls.ValidUntil = P(p.EvaluatedAt.Add(conf.Server.SmartPlaylistRefreshDelay)) } } else { diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index 3f2a2068e..294f0f816 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -156,6 +156,41 @@ var _ = Describe("buildPlaylist", func() { }) }) + Describe("plugin playlist", func() { + BeforeEach(func() { + createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC) + updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) + + playlist = model.Playlist{ + ID: "pls-plugin", + Name: "Daily Mix", + Comment: "Generated by plugin", + OwnerName: "admin", + OwnerID: "1234", + Public: false, + SongCount: 5, + Duration: 300, + CreatedAt: createdAt, + UpdatedAt: updatedAt, + PluginID: "test-plugin", + PluginPlaylistID: "daily-mix", + } + }) + + It("marks plugin playlist as readonly", func() { + ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) + result := router.buildPlaylist(ctx, playlist) + Expect(result.Readonly).To(BeTrue()) + Expect(result.ValidUntil).To(BeNil()) + }) + + It("marks plugin playlist as readonly even for non-owner", func() { + ctx = request.WithUser(ctx, model.User{ID: "other-user", UserName: "other"}) + result := router.buildPlaylist(ctx, playlist) + Expect(result.Readonly).To(BeTrue()) + }) + }) + Describe("smart playlist", func() { evaluatedAt := time.Date(2023, 2, 20, 15, 45, 0, 0, time.UTC) validUntil := evaluatedAt.Add(5 * time.Second) diff --git a/ui/src/common/playlistUtils.js b/ui/src/common/playlistUtils.js index 74a01d47a..eef7c680c 100644 --- a/ui/src/common/playlistUtils.js +++ b/ui/src/common/playlistUtils.js @@ -11,5 +11,7 @@ export const isReadOnly = (ownerId) => { export const isSmartPlaylist = (pls) => !!pls.rules +export const isPluginPlaylist = (pls) => !!pls.pluginId + export const canChangeTracks = (pls) => - isWritable(pls.ownerId) && !isSmartPlaylist(pls) + isWritable(pls.ownerId) && !isSmartPlaylist(pls) && !isPluginPlaylist(pls) diff --git a/ui/src/common/playlistUtils.test.js b/ui/src/common/playlistUtils.test.js index 2c671ecf5..f40bfaea7 100644 --- a/ui/src/common/playlistUtils.test.js +++ b/ui/src/common/playlistUtils.test.js @@ -2,6 +2,7 @@ import { isWritable, isReadOnly, isSmartPlaylist, + isPluginPlaylist, canChangeTracks, } from './playlistUtils' @@ -56,6 +57,18 @@ describe('playlistUtils', () => { }) }) + describe('isPluginPlaylist', () => { + it('returns true if playlist has pluginId', () => { + const playlist = { pluginId: 'test-plugin' } + expect(isPluginPlaylist(playlist)).toBe(true) + }) + + it('returns false if playlist does not have pluginId', () => { + const playlist = {} + expect(isPluginPlaylist(playlist)).toBe(false) + }) + }) + describe('canChangeTracks', () => { it('returns true if user is the owner and playlist is not smart', () => { localStorage.setItem('userId', 'user1') @@ -74,5 +87,11 @@ describe('playlistUtils', () => { const playlist = { ownerId: 'user1', rules: [] } expect(canChangeTracks(playlist)).toBe(false) }) + + it('returns false if playlist is a plugin playlist', () => { + localStorage.setItem('userId', 'user1') + const playlist = { ownerId: 'user1', pluginId: 'test-plugin' } + expect(canChangeTracks(playlist)).toBe(false) + }) }) }) diff --git a/ui/src/dialogs/AddToPlaylistDialog.test.jsx b/ui/src/dialogs/AddToPlaylistDialog.test.jsx index 60d3cca0d..8735ad885 100644 --- a/ui/src/dialogs/AddToPlaylistDialog.test.jsx +++ b/ui/src/dialogs/AddToPlaylistDialog.test.jsx @@ -46,7 +46,7 @@ const createTestUtils = (mockDataProvider) => data: mockIndexedData, list: { cachedRequests: { - '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"smart":false}}': + '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"readonly":false}}': { ids: ['sample-id1', 'sample-id2'], total: 2, diff --git a/ui/src/dialogs/SelectPlaylistInput.jsx b/ui/src/dialogs/SelectPlaylistInput.jsx index 847107523..bc07ec0e4 100644 --- a/ui/src/dialogs/SelectPlaylistInput.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.jsx @@ -264,7 +264,7 @@ export const SelectPlaylistInput = ({ onChange }) => { 'playlist', { page: 1, perPage: -1 }, { field: 'name', order: 'ASC' }, - { smart: false }, + { readonly: false }, ) const options = diff --git a/ui/src/dialogs/SelectPlaylistInput.test.jsx b/ui/src/dialogs/SelectPlaylistInput.test.jsx index 4ffcdf0b6..44bb803ed 100644 --- a/ui/src/dialogs/SelectPlaylistInput.test.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.test.jsx @@ -53,7 +53,7 @@ const createTestComponent = ( data: indexedData, list: { cachedRequests: { - '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"smart":false}}': + '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"readonly":false}}': { ids: Object.keys(indexedData), total: Object.keys(indexedData).length, diff --git a/ui/src/playlist/PlaylistEdit.jsx b/ui/src/playlist/PlaylistEdit.jsx index f6882e366..181d23114 100644 --- a/ui/src/playlist/PlaylistEdit.jsx +++ b/ui/src/playlist/PlaylistEdit.jsx @@ -11,7 +11,7 @@ import { ReferenceInput, SelectInput, } from 'react-admin' -import { isWritable, Title } from '../common' +import { isWritable, isPluginPlaylist, Title } from '../common' const SyncFragment = ({ formData, variant, ...rest }) => { return ( @@ -33,12 +33,17 @@ const PlaylistEditForm = (props) => { const { permissions } = usePermissions() return ( - + { ) : null } From 9ddbcbf6b4c2918e893a81da96963ecfc18912dd Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 19:00:43 -0500 Subject: [PATCH 22/24] feat: persist and expose plugin playlist ValidUntil in Subsonic API Add a ValidUntil field to the Playlist model and persist it from the plugin's GetPlaylistResponse during sync. This allows clients to know when a plugin playlist's data will be refreshed. The value is exposed in the OpenSubsonic playlist response alongside the existing smart playlist ValidUntil calculation. The migration is consolidated into a single multi-statement ExecContext call. --- ...260305051806_add_plugin_playlist_fields.go | 30 ++++++++----------- model/playlist.go | 5 ++-- plugins/playlist_provider.go | 4 +++ plugins/playlist_provider_test.go | 1 + server/subsonic/playlists.go | 3 +- server/subsonic/playlists_test.go | 12 +++++++- 6 files changed, 33 insertions(+), 22 deletions(-) diff --git a/db/migrations/20260305051806_add_plugin_playlist_fields.go b/db/migrations/20260305051806_add_plugin_playlist_fields.go index 9429e19b3..cadc7ec8e 100644 --- a/db/migrations/20260305051806_add_plugin_playlist_fields.go +++ b/db/migrations/20260305051806_add_plugin_playlist_fields.go @@ -12,27 +12,21 @@ func init() { } func upAddPluginPlaylistFields(ctx context.Context, tx *sql.Tx) error { - _, err := tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_id VARCHAR(255) NOT NULL DEFAULT '';`) - if err != nil { - return err - } - _, err = tx.ExecContext(ctx, `ALTER TABLE playlist ADD COLUMN plugin_playlist_id VARCHAR(255) NOT NULL DEFAULT '';`) - if err != nil { - return err - } - _, err = tx.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS idx_playlist_plugin ON playlist(plugin_id, plugin_playlist_id, owner_id) WHERE plugin_id != '';`) + _, err := tx.ExecContext(ctx, ` +ALTER TABLE playlist ADD COLUMN plugin_id VARCHAR(255) NOT NULL DEFAULT ''; +ALTER TABLE playlist ADD COLUMN plugin_playlist_id VARCHAR(255) NOT NULL DEFAULT ''; +ALTER TABLE playlist ADD COLUMN valid_until DATETIME DEFAULT NULL; +CREATE UNIQUE INDEX IF NOT EXISTS idx_playlist_plugin ON playlist(plugin_id, plugin_playlist_id, owner_id) WHERE plugin_id != ''; +`) return err } func downAddPluginPlaylistFields(ctx context.Context, tx *sql.Tx) error { - _, err := tx.ExecContext(ctx, `DROP INDEX IF EXISTS idx_playlist_plugin;`) - if err != nil { - return err - } - _, err = tx.ExecContext(ctx, `ALTER TABLE playlist DROP COLUMN plugin_playlist_id;`) - if err != nil { - return err - } - _, err = tx.ExecContext(ctx, `ALTER TABLE playlist DROP COLUMN plugin_id;`) + _, err := tx.ExecContext(ctx, ` +DROP INDEX IF EXISTS idx_playlist_plugin; +ALTER TABLE playlist DROP COLUMN valid_until; +ALTER TABLE playlist DROP COLUMN plugin_playlist_id; +ALTER TABLE playlist DROP COLUMN plugin_id; +`) return err } diff --git a/model/playlist.go b/model/playlist.go index d9db3f4d2..7ed3e259b 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -32,8 +32,9 @@ type Playlist struct { EvaluatedAt *time.Time `structs:"evaluated_at" json:"evaluatedAt"` // Plugin playlist attributes - PluginID string `structs:"plugin_id" json:"pluginId,omitempty"` - PluginPlaylistID string `structs:"plugin_playlist_id" json:"pluginPlaylistId,omitempty"` + PluginID string `structs:"plugin_id" json:"pluginId,omitempty"` + PluginPlaylistID string `structs:"plugin_playlist_id" json:"pluginPlaylistId,omitempty"` + ValidUntil *time.Time `structs:"valid_until" json:"validUntil,omitempty"` } func (pls Playlist) IsReadOnly() bool { diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index ab220ba1f..a1e894873 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -200,6 +200,10 @@ func (p *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin PluginID: p.pluginName, PluginPlaylistID: info.ID, } + if resp.ValidUntil > 0 { + t := time.Unix(resp.ValidUntil, 0) + pls.ValidUntil = &t + } // Set tracks from matched media files pls.AddMediaFiles(matched) diff --git a/plugins/playlist_provider_test.go b/plugins/playlist_provider_test.go index a72ef357f..c11c7b6c9 100644 --- a/plugins/playlist_provider_test.go +++ b/plugins/playlist_provider_test.go @@ -75,6 +75,7 @@ var _ = Describe("PlaylistProvider", Ordered, func() { Expect(dailyMix1.PluginID).To(Equal("test-playlist-provider")) Expect(dailyMix1.PluginPlaylistID).To(Equal("daily-mix-1")) Expect(dailyMix1.Public).To(BeFalse()) + Expect(dailyMix1.ValidUntil).To(BeNil()) }) It("generates deterministic playlist IDs", func() { diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index fc0b1cd91..92576a66d 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -168,9 +168,10 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso if p.IsReadOnly() { pls.Readonly = true - // ValidUntil only applies to smart playlists if p.IsSmartPlaylist() && p.EvaluatedAt != nil { pls.ValidUntil = P(p.EvaluatedAt.Add(conf.Server.SmartPlaylistRefreshDelay)) + } else if p.IsPluginPlaylist() && p.ValidUntil != nil { + pls.ValidUntil = p.ValidUntil } } else { user, ok := request.UserFrom(ctx) diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index 294f0f816..a9fd29b40 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -177,13 +177,23 @@ var _ = Describe("buildPlaylist", func() { } }) - It("marks plugin playlist as readonly", func() { + It("marks plugin playlist as readonly with no ValidUntil when not set", func() { ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) result := router.buildPlaylist(ctx, playlist) Expect(result.Readonly).To(BeTrue()) Expect(result.ValidUntil).To(BeNil()) }) + It("exposes ValidUntil when set on the model", func() { + validUntil := time.Date(2023, 3, 1, 12, 0, 0, 0, time.UTC) + playlist.ValidUntil = &validUntil + + ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) + result := router.buildPlaylist(ctx, playlist) + Expect(result.Readonly).To(BeTrue()) + Expect(result.ValidUntil).To(Equal(&validUntil)) + }) + It("marks plugin playlist as readonly even for non-owner", func() { ctx = request.WithUser(ctx, model.User{ID: "other-user", UserName: "other"}) result := router.buildPlaylist(ctx, playlist) From ae8263671ac0d2b0662b2a640b3b8df62906d8f5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 20:29:38 -0500 Subject: [PATCH 23/24] fix: address PR review comments for playlist provider capability Fix timer lifecycle bugs in the playlist syncer: always store RetryInterval (including 0 to disable retries), cancel discovery timers when RefreshInterval becomes 0, and cancel stale refresh timers when ValidUntil becomes 0. Extract cancelRefreshTimer helper to deduplicate the timer cleanup pattern. Improve plugin playlist update restrictions in both the Subsonic and REST API paths to compare actual values instead of just checking pointer presence or field inclusion, so passing unchanged name/comment no longer triggers a false rejection. Signed-off-by: Deluan --- core/playlists/playlists.go | 6 ++++-- core/playlists/rest_adapter.go | 12 ++++++++++- core/playlists/rest_adapter_test.go | 6 +++--- plugins/playlist_provider.go | 31 ++++++++++++++++++----------- 4 files changed, 37 insertions(+), 18 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index 4ba6e76fd..4ee35eb8e 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -163,8 +163,10 @@ func (s *playlists) Update(ctx context.Context, playlistID string, return err } // Plugin playlists allow public toggle and cover art, but block name/comment changes - if pls.IsPluginPlaylist() && (name != nil || comment != nil) { - return model.ErrNotAuthorized + if pls.IsPluginPlaylist() { + if (name != nil && *name != pls.Name) || (comment != nil && *comment != pls.Comment) { + return model.ErrNotAuthorized + } } return s.ds.WithTxImmediate(func(tx model.DataStore) error { repo := tx.Playlist(ctx) diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 33073e7da..818d8a5cb 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -3,6 +3,7 @@ package playlists import ( "context" "errors" + "slices" "github.com/deluan/rest" "github.com/navidrome/navidrome/model" @@ -94,7 +95,16 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity return rest.ErrPermissionDenied } // Plugin playlists allow public and ownership changes, but block name/comment - if current.IsPluginPlaylist() && (entity.Name != current.Name || entity.Comment != current.Comment) { + if current.IsPluginPlaylist() && slices.ContainsFunc(cols, func(c string) bool { + switch c { + case "name": + return entity.Name != current.Name + case "comment": + return entity.Comment != current.Comment + default: + return false + } + }) { return rest.ErrPermissionDenied } // Apply ownership change (admin only) diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 05f4b3be2..825c53656 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -110,7 +110,7 @@ var _ = Describe("REST Adapter", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) repo = ps.NewRepository(ctx).(rest.Persistable) pls := &model.Playlist{Name: "Changed Name", Comment: ""} - err := repo.Update("pls-plugin", pls) + err := repo.Update("pls-plugin", pls, "name", "comment") Expect(err).To(Equal(rest.ErrPermissionDenied)) }) @@ -122,7 +122,7 @@ var _ = Describe("REST Adapter", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) repo = ps.NewRepository(ctx).(rest.Persistable) pls := &model.Playlist{Name: "Plugin PL", Comment: "new comment"} - err := repo.Update("pls-plugin", pls) + err := repo.Update("pls-plugin", pls, "name", "comment") Expect(err).To(Equal(rest.ErrPermissionDenied)) }) @@ -134,7 +134,7 @@ var _ = Describe("REST Adapter", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) repo = ps.NewRepository(ctx).(rest.Persistable) pls := &model.Playlist{Name: "Plugin PL", Public: true} - err := repo.Update("pls-plugin", pls) + err := repo.Update("pls-plugin", pls, "public") Expect(err).ToNot(HaveOccurred()) }) diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index a1e894873..9ab62a0da 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -119,10 +119,8 @@ func (p *playlistSyncer) discoverAndSync() { return } - // Store retry interval from response - if resp.RetryInterval > 0 { - p.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) - } + // Store retry interval from response (including 0, which disables retries) + p.retryInterval.Store(int64(time.Duration(resp.RetryInterval) * time.Second)) resolvedUsers := map[string]string{} // username -> userID cache for _, info := range resp.Playlists { @@ -150,9 +148,12 @@ func (p *playlistSyncer) discoverAndSync() { p.syncPlaylist(info, dbID, ownerID) } - // Schedule re-discovery if RefreshInterval > 0 + // Schedule re-discovery if RefreshInterval > 0, otherwise cancel any existing timer if resp.RefreshInterval > 0 { p.scheduleDiscovery(time.Duration(resp.RefreshInterval) * time.Second) + } else if p.discoveryTimer != nil { + p.discoveryTimer.Stop() + p.discoveryTimer = nil } } @@ -165,12 +166,7 @@ func (p *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin if err != nil { if isPlaylistNotFoundError(err) { log.Info(ctx, "Playlist not found, skipping", "plugin", p.pluginName, "playlistID", info.ID) - // Stop any existing refresh timer for this playlist - if timer, ok := p.refreshTimers[dbID]; ok { - timer.Stop() - delete(p.refreshTimers, dbID) - p.refreshTimerCount.Store(int32(len(p.refreshTimers))) - } + p.cancelRefreshTimer(dbID) return } log.Warn(ctx, "Failed to call GetPlaylist", "plugin", p.pluginName, "playlistID", info.ID, err) @@ -218,7 +214,7 @@ func (p *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin log.Info(ctx, "Synced plugin playlist", "plugin", p.pluginName, "playlistID", info.ID, "name", resp.Name, "tracks", len(matched), "owner", ownerID) - // Schedule refresh if ValidUntil > 0 + // Schedule refresh if ValidUntil > 0, otherwise cancel any stale timer if resp.ValidUntil > 0 { validUntil := time.Unix(resp.ValidUntil, 0) delay := time.Until(validUntil) @@ -226,6 +222,17 @@ func (p *playlistSyncer) syncPlaylist(info capabilities.PlaylistInfo, dbID strin delay = 1 * time.Second // Already expired, refresh soon } p.schedulePlaylistRefresh(info, dbID, ownerID, delay) + } else { + p.cancelRefreshTimer(dbID) + } +} + +// cancelRefreshTimer stops and removes the refresh timer for the given playlist DB ID, if any. +func (p *playlistSyncer) cancelRefreshTimer(dbID string) { + if timer, ok := p.refreshTimers[dbID]; ok { + timer.Stop() + delete(p.refreshTimers, dbID) + p.refreshTimerCount.Store(int32(len(p.refreshTimers))) } } From 439cff47ac3d5d32487a9f81b82a3f4a78756a76 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 21:00:31 -0500 Subject: [PATCH 24/24] feat: add server-managed fields for plugin playlists in rest adapter Signed-off-by: Deluan --- core/playlists/rest_adapter.go | 3 +++ core/playlists/rest_adapter_test.go | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 818d8a5cb..5e0042d04 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -69,6 +69,9 @@ func (s *playlists) savePlaylist(ctx context.Context, pls *model.Playlist) (stri pls.UploadedImage = "" // Managed by image upload endpoint pls.ExternalImageURL = "" // Managed by M3U import / plugins only pls.EvaluatedAt = nil // Server-managed + pls.PluginID = "" // Server-managed (plugin system) + pls.PluginPlaylistID = "" // Server-managed (plugin system) + pls.ValidUntil = nil // Server-managed (plugin system) err := s.ds.Playlist(ctx).Put(pls) if err != nil { return "", err diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 825c53656..832cdbda7 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -74,6 +74,9 @@ var _ = Describe("REST Adapter", func() { UploadedImage: "injected-image-path", ExternalImageURL: "http://evil.example.com/ssrf", EvaluatedAt: &now, + PluginID: "fake-plugin", + PluginPlaylistID: "fake-playlist-id", + ValidUntil: &now, } _, err := repo.Save(pls) Expect(err).ToNot(HaveOccurred()) @@ -90,6 +93,9 @@ var _ = Describe("REST Adapter", func() { Expect(saved.UploadedImage).To(BeEmpty()) Expect(saved.ExternalImageURL).To(BeEmpty()) Expect(saved.EvaluatedAt).To(BeNil()) + Expect(saved.PluginID).To(BeEmpty()) + Expect(saved.PluginPlaylistID).To(BeEmpty()) + Expect(saved.ValidUntil).To(BeNil()) }) })