From a8a336de33c69147ebe5cc22335840ca45a03903 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 09:01:33 -0500 Subject: [PATCH] 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