From 210fd55021d4f8ba58a339a178e872c9a6aab73d Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 26 Sep 2026 19:43:51 -0400 Subject: [PATCH] feat(scanner): evaluate new and changed smart playlists at the end of a scan Smart playlists imported from .nsp files showed 0 tracks until their owner opened them, because tracks are only materialized on read. The scanner now queues every imported smart playlist that has no evaluation yet (new, or its file changed) and evaluates them as the last scan step, after GC and stats, so the rules see the final library state. Unchanged files keep their evaluation and are not re-run. The new PlaylistRepository.Evaluate runs the refresh as the playlist owner, ignoring the caller's visibility and the refresh delay, inside a transaction so readers never see an emptied playlist. Evaluating inside the scan also covers the default out-of-process scanner. Evaluation errors are logged and do not fail the scan; the playlist is still evaluated on its next read. Fixes #4539 --- model/playlist.go | 2 + persistence/share_repository.go | 16 +------- persistence/smart_playlist_repository.go | 18 ++++++++ persistence/smart_playlist_repository_test.go | 41 +++++++++++++++++++ persistence/sql_base_repository.go | 12 ++++++ scanner/phase_4_playlists.go | 4 ++ scanner/phase_4_playlists_test.go | 23 +++++++++++ scanner/scanner.go | 37 +++++++++++++++++ scanner/scanner_internal_test.go | 30 ++++++++++++++ tests/mock_playlist_repo.go | 9 ++++ 10 files changed, 177 insertions(+), 15 deletions(-) diff --git a/model/playlist.go b/model/playlist.go index d2ed97682..91fab1b43 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -221,6 +221,8 @@ type PlaylistRepository interface { FindByPath(ctx context.Context, path string) (*Playlist, error) Tracks(ctx context.Context, playlistId string, refreshSmartPlaylist bool) PlaylistTrackRepository GetPlaylists(ctx context.Context, mediaFileId string) (Playlists, error) + // Evaluate refreshes a smart playlist's tracks as its owner, ignoring visibility and the refresh delay. + Evaluate(ctx context.Context, id string) error } type PlaylistTrack struct { diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 6dd9c3d85..3a9ecfbad 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -10,7 +10,6 @@ import ( "github.com/deluan/rest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/pocketbase/dbx" ) @@ -78,7 +77,7 @@ func (r *shareRepository) loadMedia(ctx context.Context, share *model.Share) err return And{cond, Eq{"missing": false}} } // Load as the share owner so their library access is applied, whoever renders the share. - ownerCtx, err := r.ownerContext(ctx, share) + ownerCtx, err := r.ownerContext(ctx, share.UserID) if err != nil { return err } @@ -128,19 +127,6 @@ func (r *shareRepository) loadMedia(ctx context.Context, share *model.Share) err return nil } -// ownerContext returns a context scoped to the share owner, so repository -// queries apply the owner's library access when a public share is rendered. -func (r *shareRepository) ownerContext(ctx context.Context, share *model.Share) (context.Context, error) { - owner, err := NewUserRepository(r.db).Get(ctx, share.UserID) - if err != nil { - return nil, fmt.Errorf("loading share owner %q: %w", share.UserID, err) - } - if owner == nil { - return nil, fmt.Errorf("share owner %q not found", share.UserID) - } - return request.WithUser(ctx, *owner), nil -} - func sortByIdPosition(mfs model.MediaFiles, ids []string) model.MediaFiles { m := map[string]int{} for i, mf := range mfs { diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 871f53dd2..602533442 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "slices" "time" @@ -18,6 +19,23 @@ import ( // in its criteria. To optimize performance, it only refreshes when necessary based on the last evaluated time and // configured refresh delay. +func (r *playlistRepository) Evaluate(ctx context.Context, id string) error { + var res dbPlaylist + if err := r.queryOne(ctx, r.selectPlaylist(ctx).Where(Eq{"playlist.id": id}), &res); err != nil { + return err + } + pls := res.Playlist + ownerCtx, err := r.ownerContext(ctx, pls.OwnerID) + if err != nil { + return err + } + pls.EvaluatedAt = nil + if !r.refreshSmartPlaylist(ownerCtx, &pls) { + return fmt.Errorf("evaluating smart playlist %s", id) + } + return nil +} + // refreshSmartPlaylist evaluates the criteria of a smart playlist and updates its tracks accordingly. func (r *playlistRepository) refreshSmartPlaylist(ctx context.Context, pls *model.Playlist) bool { return r.refreshSmartPlaylistTree(ctx, pls, map[string]struct{}{}) diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 4d4c5edb0..5e8890e46 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -330,6 +330,47 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { }) }) + Describe("Evaluate", func() { + dayRules := func() *criteria.Criteria { + return &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + RefreshDelay: 24 * time.Hour, + } + } + + It("evaluates even when the refresh delay has not elapsed", func() { + evaluatedAt := time.Now().Add(-1 * time.Hour) + pls := model.Playlist{Name: "Evaluate Delay", OwnerID: "userid", Rules: dayRules(), EvaluatedAt: &evaluatedAt} + Expect(repo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(ctx, pls.ID) }) + + Expect(repo.Evaluate(ctx, pls.ID)).To(Succeed()) + + got, err := repo.Get(ctx, pls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(*got.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + Expect(got.SongCount).To(Equal(1)) + }) + + It("evaluates as the owner, even when the caller cannot see the playlist", func() { + pls := model.Playlist{Name: "Evaluate Owner", OwnerID: regularUser.ID, Rules: dayRules()} + Expect(repo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(ctx, pls.ID) }) + otherCtx := request.WithUser(log.NewContext(GinkgoT().Context()), thirdUser) + + Expect(repo.Evaluate(otherCtx, pls.ID)).To(Succeed()) + + got, err := repo.Get(ctx, pls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.EvaluatedAt).ToNot(BeNil()) + Expect(got.SongCount).To(Equal(1)) + }) + + It("returns ErrNotFound for an unknown playlist", func() { + Expect(repo.Evaluate(ctx, "nonexistent-id")).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("Playlist Track Sorting", func() { var testPlaylistID string diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index be88156d8..c5ab7cde1 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -58,6 +58,18 @@ func loggedUser(ctx context.Context) *model.User { } } +// ownerContext scopes ctx to the given user, so queries apply that user's library access and annotations. +func (r sqlRepository) ownerContext(ctx context.Context, userID string) (context.Context, error) { + owner, err := NewUserRepository(r.db).Get(ctx, userID) + if err != nil { + return nil, fmt.Errorf("loading owner %q: %w", userID, err) + } + if owner == nil { + return nil, fmt.Errorf("owner %q not found", userID) + } + return request.WithUser(ctx, *owner), nil +} + // ownerFilter returns the predicate restricting access to rows owned by the logged-in user, for // tables with a user_id column. It returns nil for admins and for headless/system contexts (invalid // user), meaning "no ownership restriction". Callers should skip the WHERE clause when it is nil. diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index bb67c1ba3..40747f3e3 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -144,6 +144,10 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. continue } if pls.IsSmartPlaylist() { + // A nil EvaluatedAt means the playlist is new or its file changed + if pls.ID != "" && pls.EvaluatedAt == nil { + p.scanState.queueSmartPlaylist(pls.ID) + } log.Debug("Scanner: Imported smart playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "elapsed", time.Since(started)) } else { log.Debug("Scanner: Imported playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks), "elapsed", time.Since(started)) diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 303af338f..d390f1017 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -6,12 +6,14 @@ import ( "os" "path/filepath" "sort" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -213,6 +215,27 @@ var _ = Describe("phasePlaylists", func() { ))) }) + It("queues only smart playlists that were never evaluated", func() { + libPath := GinkgoT().TempDir() + folder := &model.Folder{LibraryPath: libPath, Path: "path/to", Name: "folder"} + _ = os.MkdirAll(folder.AbsolutePath(), 0755) + for _, name := range []string{"new.nsp", "evaluated.nsp", "regular.m3u"} { + _ = os.WriteFile(filepath.Join(folder.AbsolutePath(), name), []byte{}, 0600) + } + + rules := &criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "Day"}}} + pls.On("ImportFromFolder", mock.Anything, folder, "new.nsp"). + Return(&model.Playlist{ID: "new", Rules: rules}, nil) + pls.On("ImportFromFolder", mock.Anything, folder, "evaluated.nsp"). + Return(&model.Playlist{ID: "evaluated", Rules: rules, EvaluatedAt: new(time.Now())}, nil) + pls.On("ImportFromFolder", mock.Anything, folder, "regular.m3u"). + Return(&model.Playlist{ID: "regular"}, nil) + + _, err := phase.processPlaylistsInFolder(folder) + Expect(err).ToNot(HaveOccurred()) + Expect(state.smartPlaylistsToEvaluate()).To(ConsistOf("new")) + }) + It("reports an error if there is an error reading files", func() { tests.SkipOnWindows("relies on Unix /etc filesystem") progress := make(chan *ProgressInfo) diff --git a/scanner/scanner.go b/scanner/scanner.go index cd2fe3c8d..02a66dff8 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -6,6 +6,7 @@ import ( "maps" "path/filepath" "slices" + "sync" "sync/atomic" "time" @@ -32,6 +33,21 @@ type scanState struct { libraries model.Libraries // Store libraries list for consistency across phases targets map[int][]string // Optional: map[libraryID][]folderPaths for selective scans totalLibraryCount int // Total number of libraries (unfiltered), for cross-library move detection + + smartPlaylistsMu sync.Mutex + smartPlaylists []string +} + +func (s *scanState) queueSmartPlaylist(id string) { + s.smartPlaylistsMu.Lock() + defer s.smartPlaylistsMu.Unlock() + s.smartPlaylists = append(s.smartPlaylists, id) +} + +func (s *scanState) smartPlaylistsToEvaluate() []string { + s.smartPlaylistsMu.Lock() + defer s.smartPlaylistsMu.Unlock() + return s.smartPlaylists } func (s *scanState) sendProgress(info *ProgressInfo) { @@ -187,6 +203,9 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] // Update last_scan_completed_at for all libraries s.runUpdateLibraries(ctx, &state), + + // Evaluate new/changed smart playlists last, so their rules see the final library state + s.runEvaluateSmartPlaylists(ctx, &state), ) if err != nil { log.Error(ctx, "Scanner: Finished with error", "duration", time.Since(startTime), err) @@ -332,6 +351,24 @@ func (s *scannerImpl) runRefreshStats(ctx context.Context, state *scanState) fun } } +// Failures are logged but never fail the scan: the playlist is still evaluated when it is next read. +func (s *scannerImpl) runEvaluateSmartPlaylists(ctx context.Context, state *scanState) func() error { + return func() error { + for _, id := range state.smartPlaylistsToEvaluate() { + start := time.Now() + err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Playlist().Evaluate(ctx, id) + }, "scanner: evaluate smart playlist") + if err != nil { + log.Warn(ctx, "Scanner: Could not evaluate smart playlist", "id", id, err) + continue + } + log.Debug(ctx, "Scanner: Evaluated smart playlist", "id", id, "elapsed", time.Since(start)) + } + return nil + } +} + func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) func() error { return func() error { start := time.Now() diff --git a/scanner/scanner_internal_test.go b/scanner/scanner_internal_test.go index 0778bd6ec..bd5fb0fb9 100644 --- a/scanner/scanner_internal_test.go +++ b/scanner/scanner_internal_test.go @@ -9,6 +9,7 @@ import ( "sync/atomic" ppl "github.com/google/go-pipeline/pkg/pipeline" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -135,3 +136,32 @@ var _ = Describe("runPhase", func() { Expect(counter.Load()).To(Equal(int64(3))) }) }) + +var _ = Describe("runEvaluateSmartPlaylists", func() { + var ctx context.Context + var plsRepo *tests.MockPlaylistRepo + var s *scannerImpl + var state *scanState + + BeforeEach(func() { + ctx = GinkgoT().Context() + plsRepo = tests.CreateMockPlaylistRepo() + s = &scannerImpl{ds: &tests.MockDataStore{MockedPlaylist: plsRepo}} + state = &scanState{} + }) + + It("evaluates every queued smart playlist", func() { + state.queueSmartPlaylist("p1") + state.queueSmartPlaylist("p2") + + Expect(s.runEvaluateSmartPlaylists(ctx, state)()).To(Succeed()) + Expect(plsRepo.Evaluated).To(Equal([]string{"p1", "p2"})) + }) + + It("does not fail the scan when an evaluation fails", func() { + plsRepo.SetError(true) + state.queueSmartPlaylist("p1") + + Expect(s.runEvaluateSmartPlaylists(ctx, state)()).To(Succeed()) + }) +}) diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 824e701f6..97cf7f351 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -30,6 +30,7 @@ type MockPlaylistRepo struct { Err bool TracksRepo model.PlaylistTrackRepository TracksRefreshed bool + Evaluated []string } func (m *MockPlaylistRepo) SetError(err bool) { @@ -186,4 +187,12 @@ func (m *MockPlaylistRepo) CountAll(_ context.Context, _ ...model.QueryOptions) return int64(len(m.Data)), nil } +func (m *MockPlaylistRepo) Evaluate(_ context.Context, id string) error { + if m.Err { + return errors.New("error") + } + m.Evaluated = append(m.Evaluated, id) + return nil +} + var _ model.PlaylistRepository = (*MockPlaylistRepo)(nil)