From ae8263671ac0d2b0662b2a640b3b8df62906d8f5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 20:29:38 -0500 Subject: [PATCH] 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))) } }