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 <deluan@navidrome.org>

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)

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)

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

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