fix(smartplaylists): stop infinite recursion on playlists that reference each other

Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.

persistence/smart_playlist_repository.go

@@ -17,6 +17,17 @@ import (
 
 // refreshSmartPlaylist evaluates the criteria of a smart playlist and updates its tracks accordingly.
 func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool {
+	return r.refreshSmartPlaylistTree(pls, map[string]struct{}{})
+}
+
+// The visited set stops playlists that reference each other from recursing forever.
+func (r *playlistRepository) refreshSmartPlaylistTree(pls *model.Playlist, visited map[string]struct{}) bool {
+	if _, seen := visited[pls.ID]; seen {
+		log.Trace(r.ctx, "Skipping already visited smart playlist", "playlist", pls.Name, "id", pls.ID)
+		return false
+	}
+	visited[pls.ID] = struct{}{}
+
 	usr := loggedUser(r.ctx)
 	if !r.shouldRefreshSmartPlaylist(pls, usr) {
 		return false
@@ -34,7 +45,7 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool {
 	normalisedPls := pls.WithNormalizeChildPaths()
 	rulesSQL := newSmartPlaylistCriteria(*normalisedPls.Rules, withSmartPlaylistOwner(*usr))
 
-	if !r.refreshChildPlaylists(&normalisedPls, rulesSQL) {
+	if !r.refreshChildPlaylists(&normalisedPls, rulesSQL, visited) {
 		return false
 	}
 
@@ -91,7 +102,7 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr
 
 // refreshChildPlaylists handles refreshing any child playlists that are referenced in the smart playlist criteria.
 // Returns false if child playlists could not be loaded (DB error), signaling the parent refresh should abort.
-func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool {
+func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria, visited map[string]struct{}) bool {
 	childPlaylistIds := rulesSQL.ChildPlaylistIds()
 	childPlaylistPaths := rulesSQL.ChildPlaylistPaths()
 	if len(childPlaylistIds) == 0 && len(childPlaylistPaths) == 0 {
@@ -122,7 +133,7 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL
 		if childPlaylists[i].Path != "" {
 			found[norm.NFC.String(childPlaylists[i].Path)] = struct{}{}
 		}
-		r.refreshSmartPlaylist(&childPlaylists[i])
+		r.refreshSmartPlaylistTree(&childPlaylists[i], visited)
 	}
 	for _, id := range childPlaylistIds {
 		if _, ok := found[id]; !ok {

persistence/smart_playlist_repository_test.go

@@ -174,6 +174,28 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() {
 				})
 			})
 
+			It("does not recurse forever when two smart playlists reference each other", func() {
+				conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
+
+				plsA := model.Playlist{Name: "Cycle A", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{
+					Expression: criteria.All{criteria.Contains{"title": "Day"}},
+				}}
+				Expect(repo.Put(&plsA)).To(Succeed())
+				DeferCleanup(func() { _ = repo.Delete(plsA.ID) })
+
+				plsB := model.Playlist{Name: "Cycle B", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{
+					Expression: criteria.All{criteria.InPlaylist{"id": plsA.ID}},
+				}}
+				Expect(repo.Put(&plsB)).To(Succeed())
+				DeferCleanup(func() { _ = repo.Delete(plsB.ID) })
+
+				plsA.Rules = &criteria.Criteria{Expression: criteria.All{criteria.InPlaylist{"id": plsB.ID}}}
+				Expect(repo.Put(&plsA)).To(Succeed())
+
+				_, err := repo.GetWithTracks(plsA.ID, true, false)
+				Expect(err).ToNot(HaveOccurred())
+			})
+
 			It("does not treat an empty path as a reference to every playlist without a path", func() {
 				conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second