From 759ab26b19f4d9a44bd5e47110cb6029153182f2 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Mar 2026 19:59:10 -0400 Subject: [PATCH 1/5] feat(playlists): add Evaluate method to PlaylistRepository Refactor refreshSmartPlaylist into guard checks + core evaluation logic (doRefreshSmartPlaylist). Add public Evaluate method that bypasses ownership/delay guards for system-level evaluation (e.g., background processing after scanner import). Part of #4539 --- model/playlist.go | 1 + persistence/playlist_repository.go | 24 ++++++++++-- persistence/playlist_repository_test.go | 51 +++++++++++++++++++++++++ tests/mock_playlist_repo.go | 7 ++++ 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/model/playlist.go b/model/playlist.go index e2f93993d..ff120c736 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -131,6 +131,7 @@ type PlaylistRepository interface { Delete(id string) error Tracks(playlistId string, refreshSmartPlaylist bool) PlaylistTrackRepository GetPlaylists(mediaFileId string) (Playlists, error) + Evaluate(id string) error } type PlaylistTrack struct { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 8d1bbe0f8..1aed46e92 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -215,6 +215,10 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return false } + return r.doRefreshSmartPlaylist(pls, usr.ID) +} + +func (r *playlistRepository) doRefreshSmartPlaylist(pls *model.Playlist, userID string) bool { log.Debug(r.ctx, "Refreshing smart playlist", "playlist", pls.Name, "id", pls.ID) start := time.Now() @@ -244,11 +248,11 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { From("media_file").LeftJoin("annotation on ("+ "annotation.item_id = media_file.id"+ " AND annotation.item_type = 'media_file'"+ - " AND annotation.user_id = ?)", usr.ID) + " AND annotation.user_id = ?)", userID) // Conditionally join album/artist annotation tables only when referenced by criteria or sort requiredJoins := rules.RequiredJoins() - sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, usr.ID) + sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, userID) // Only include media files from libraries the user has access to sq = r.applyLibraryFilter(sq, "media_file") @@ -261,8 +265,8 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { LeftJoin("annotation on ("+ "annotation.item_id = media_file.id"+ " AND annotation.item_type = 'media_file'"+ - " AND annotation.user_id = ?)", usr.ID) - countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, usr.ID) + " AND annotation.user_id = ?)", userID) + countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, userID) countSq = r.applyLibraryFilter(countSq, "media_file") countSq = countSq.Where(rules) @@ -310,6 +314,18 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return true } +func (r *playlistRepository) Evaluate(id string) error { + pls, err := r.Get(id) + if err != nil { + return err + } + if !pls.IsSmartPlaylist() { + return nil + } + r.doRefreshSmartPlaylist(pls, pls.OwnerID) + return nil +} + func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, joins criteria.JoinType, userID string) SelectBuilder { if joins.Has(criteria.JoinAlbumAnnotation) { sq = sq.LeftJoin("annotation AS album_annotation ON ("+ diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index c091cb32b..0619904c5 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -254,6 +254,57 @@ var _ = Describe("PlaylistRepository", func() { }) }) + Describe("Evaluate", func() { + var testPlaylistID string + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + AfterEach(func() { + if testPlaylistID != "" { + _ = repo.Delete(testPlaylistID) + testPlaylistID = "" + } + }) + + It("evaluates a smart playlist and sets EvaluatedAt and SongCount", func() { + rules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Contains{"title": "Day"}, + }, + } + newPls := model.Playlist{Name: "Evaluate Test", OwnerID: "userid", Rules: rules} + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + Expect(repo.Evaluate(newPls.ID)).To(Succeed()) + + saved, err := repo.Get(newPls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.EvaluatedAt).ToNot(BeNil()) + Expect(*saved.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + Expect(saved.SongCount).To(BeNumerically(">", 0)) + }) + + It("is a no-op for non-smart playlists", func() { + newPls := model.Playlist{Name: "Regular Playlist", OwnerID: "userid"} + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + Expect(repo.Evaluate(newPls.ID)).To(Succeed()) + + saved, err := repo.Get(newPls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.EvaluatedAt).To(BeNil()) + }) + + It("returns ErrNotFound for a non-existent playlist ID", func() { + err := repo.Evaluate("nonexistent-id") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("Playlist Track Sorting", func() { var testPlaylistID string diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 9bdc52152..9717e49d2 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -108,4 +108,11 @@ func (m *MockPlaylistRepo) CountAll(_ ...model.QueryOptions) (int64, error) { return int64(len(m.Data)), nil } +func (m *MockPlaylistRepo) Evaluate(_ string) error { + if m.Err { + return errors.New("error") + } + return nil +} + var _ model.PlaylistRepository = (*MockPlaylistRepo)(nil) From 7ce0d3e79fefd45e4113745d240e2ae3e4d6eab6 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Mar 2026 20:00:49 -0400 Subject: [PATCH 2/5] feat(playlists): add SmartPlaylistEvaluator for background evaluation Implements the CacheWarmer pattern: a background goroutine processes batched playlist IDs, evaluating smart playlist criteria and populating tracks asynchronously. Includes a noop implementation for CLI/test use. Part of #4539 --- core/playlists/evaluator.go | 107 +++++++++++++++++++++++++++++++ core/playlists/evaluator_test.go | 17 +++++ 2 files changed, 124 insertions(+) create mode 100644 core/playlists/evaluator.go create mode 100644 core/playlists/evaluator_test.go diff --git a/core/playlists/evaluator.go b/core/playlists/evaluator.go new file mode 100644 index 000000000..0b237c936 --- /dev/null +++ b/core/playlists/evaluator.go @@ -0,0 +1,107 @@ +package playlists + +import ( + "context" + "maps" + "slices" + "sync" + "time" + + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +// SmartPlaylistEvaluator evaluates smart playlists in the background. +// Call Enqueue to queue a playlist for evaluation. The evaluation happens +// asynchronously in a background goroutine. +type SmartPlaylistEvaluator interface { + Enqueue(playlistID string) +} + +func NewSmartPlaylistEvaluator(ds model.DataStore) SmartPlaylistEvaluator { + e := &smartPlaylistEvaluator{ + ds: ds, + buffer: make(map[string]struct{}), + wakeSignal: make(chan struct{}, 1), + } + go e.run() + return e +} + +type smartPlaylistEvaluator struct { + ds model.DataStore + buffer map[string]struct{} + mutex sync.Mutex + wakeSignal chan struct{} +} + +func (e *smartPlaylistEvaluator) Enqueue(playlistID string) { + e.mutex.Lock() + defer e.mutex.Unlock() + e.buffer[playlistID] = struct{}{} + e.sendWakeSignal() +} + +func (e *smartPlaylistEvaluator) sendWakeSignal() { + select { + case e.wakeSignal <- struct{}{}: + default: + } +} + +func (e *smartPlaylistEvaluator) run() { + for { + e.waitSignal(10 * time.Second) + + e.mutex.Lock() + if len(e.buffer) == 0 { + e.mutex.Unlock() + continue + } + + batch := slices.Collect(maps.Keys(e.buffer)) + e.buffer = make(map[string]struct{}) + e.mutex.Unlock() + + e.processBatch(batch) + } +} + +func (e *smartPlaylistEvaluator) waitSignal(timeout time.Duration) { + select { + case <-time.After(timeout): + case <-e.wakeSignal: + } +} + +func (e *smartPlaylistEvaluator) processBatch(batch []string) { + log.Debug("Evaluating smart playlists in background", "count", len(batch)) + for _, id := range batch { + e.doEvaluate(id) + } +} + +func (e *smartPlaylistEvaluator) doEvaluate(id string) { + // Use admin context so userFilter() returns all playlists. + // Evaluate() internally uses pls.OwnerID for annotation JOINs. + ctx := auth.WithAdminUser(context.TODO(), e.ds) + + start := time.Now() + err := e.ds.Playlist(ctx).Evaluate(id) + if err != nil { + log.Error("Error evaluating smart playlist in background", "id", id, err) + return + } + log.Debug("Background smart playlist evaluation complete", "id", id, "elapsed", time.Since(start)) +} + +// NoopSmartPlaylistEvaluator returns an evaluator that does nothing. +// Used in CLI scan and test contexts. +func NoopSmartPlaylistEvaluator() SmartPlaylistEvaluator { + return &noopSmartPlaylistEvaluator{} +} + +type noopSmartPlaylistEvaluator struct{} + +func (n *noopSmartPlaylistEvaluator) Enqueue(string) {} diff --git a/core/playlists/evaluator_test.go b/core/playlists/evaluator_test.go new file mode 100644 index 000000000..8aad7c814 --- /dev/null +++ b/core/playlists/evaluator_test.go @@ -0,0 +1,17 @@ +package playlists_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/navidrome/navidrome/core/playlists" +) + +var _ = Describe("SmartPlaylistEvaluator", func() { + Describe("NoopSmartPlaylistEvaluator", func() { + It("does not panic when enqueuing", func() { + noop := playlists.NoopSmartPlaylistEvaluator() + Expect(func() { noop.Enqueue("some-id") }).ToNot(Panic()) + }) + }) +}) From af0a6615b293cad1526e53144aaa32df06801b89 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Mar 2026 20:06:28 -0400 Subject: [PATCH 3/5] feat(scanner): evaluate smart playlists in background after import Wire SmartPlaylistEvaluator into the scanner pipeline. After importing a .nsp file, the scanner enqueues the playlist for background evaluation. The evaluator processes the queue asynchronously, populating tracks and updating song_count/duration/size without blocking the scan. Fixes #4539 --- cmd/wire_gen.go | 12 ++++++++---- core/wire_providers.go | 1 + scanner/controller.go | 8 +++++--- scanner/controller_test.go | 2 +- scanner/phase_4_playlists.go | 6 +++++- scanner/phase_4_playlists_test.go | 2 +- scanner/scanner.go | 3 ++- scanner/scanner_benchmark_test.go | 2 +- scanner/scanner_multilibrary_test.go | 2 +- scanner/scanner_selective_test.go | 2 +- scanner/scanner_test.go | 2 +- server/e2e/e2e_suite_test.go | 4 ++-- server/e2e/subsonic_multilibrary_test.go | 2 +- 13 files changed, 30 insertions(+), 18 deletions(-) diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 5b9fd648f..94ecec945 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -75,7 +75,8 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { provider := external.NewProvider(dataStore, agentsAgents) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + smartPlaylistEvaluator := playlists.NewSmartPlaylistEvaluator(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, smartPlaylistEvaluator, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) user := core.NewUser(dataStore, manager) @@ -103,7 +104,8 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + smartPlaylistEvaluator := playlists.NewSmartPlaylistEvaluator(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, smartPlaylistEvaluator, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) lyricsLyrics := lyrics.NewLyrics(manager) @@ -173,7 +175,8 @@ func CreateScanner(ctx context.Context) model.Scanner { cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + smartPlaylistEvaluator := playlists.NewSmartPlaylistEvaluator(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, smartPlaylistEvaluator, metricsMetrics) return modelScanner } @@ -191,7 +194,8 @@ func CreateScanWatcher(ctx context.Context) scanner.Watcher { cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + smartPlaylistEvaluator := playlists.NewSmartPlaylistEvaluator(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, smartPlaylistEvaluator, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) return watcher } diff --git a/core/wire_providers.go b/core/wire_providers.go index 276d9556a..14443de6a 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -20,6 +20,7 @@ var Set = wire.NewSet( NewPlayers, NewShare, playlists.NewPlaylists, + playlists.NewSmartPlaylistEvaluator, NewLibrary, NewUser, NewMaintenance, diff --git a/scanner/controller.go b/scanner/controller.go index 94248ffd0..69b020f6f 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -27,13 +27,14 @@ var ( ) func New(rootCtx context.Context, ds model.DataStore, cw artwork.CacheWarmer, broker events.Broker, - pls playlists.Playlists, m metrics.Metrics) model.Scanner { + pls playlists.Playlists, spe playlists.SmartPlaylistEvaluator, m metrics.Metrics) model.Scanner { c := &controller{ rootCtx: rootCtx, ds: ds, cw: cw, broker: broker, pls: pls, + spe: spe, metrics: m, devExternalScanner: conf.Server.DevExternalScanner, } @@ -47,7 +48,7 @@ func (s *controller) getScanner() scanner { if s.devExternalScanner { return &scannerExternal{} } - return &scannerImpl{ds: s.ds, cw: s.cw, pls: s.pls} + return &scannerImpl{ds: s.ds, cw: s.cw, pls: s.pls, spe: s.spe} } // CallScan starts an in-process scan of specific library/folder pairs. @@ -64,7 +65,7 @@ func CallScan(ctx context.Context, ds model.DataStore, pls playlists.Playlists, progress := make(chan *ProgressInfo, 100) go func() { defer close(progress) - scanner := &scannerImpl{ds: ds, cw: artwork.NoopCacheWarmer(), pls: pls} + scanner := &scannerImpl{ds: ds, cw: artwork.NoopCacheWarmer(), pls: pls, spe: playlists.NoopSmartPlaylistEvaluator()} scanner.scanFolders(ctx, fullScan, targets, progress) }() return progress, nil @@ -99,6 +100,7 @@ type controller struct { broker events.Broker metrics metrics.Metrics pls playlists.Playlists + spe playlists.SmartPlaylistEvaluator limiter *rate.Sometimes devExternalScanner bool count atomic.Uint32 diff --git a/scanner/controller_test.go b/scanner/controller_test.go index d60d432b4..ad71dc278 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -32,7 +32,7 @@ var _ = Describe("Controller", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())} ds.MockedProperty = &tests.MockedPropertyRepo{} - ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) }) It("includes last scan error", func() { diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index ab5f77ae0..e3d5f16d8 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -23,16 +23,19 @@ type phasePlaylists struct { ds model.DataStore pls playlists.Playlists cw artwork.CacheWarmer + spe playlists.SmartPlaylistEvaluator refreshed atomic.Uint32 } -func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls playlists.Playlists, cw artwork.CacheWarmer) *phasePlaylists { +func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, + pls playlists.Playlists, cw artwork.CacheWarmer, spe playlists.SmartPlaylistEvaluator) *phasePlaylists { return &phasePlaylists{ ctx: ctx, scanState: scanState, ds: ds, pls: pls, cw: cw, + spe: spe, } } @@ -105,6 +108,7 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. continue } if pls.IsSmartPlaylist() { + p.spe.Enqueue(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 0b50d39cb..923771c48 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -42,7 +42,7 @@ var _ = Describe("phasePlaylists", func() { pls = &mockPlaylists{} cw = artwork.NoopCacheWarmer() state = &scanState{} - phase = createPhasePlaylists(ctx, state, ds, pls, cw) + phase = createPhasePlaylists(ctx, state, ds, pls, cw, playlists.NoopSmartPlaylistEvaluator()) }) Describe("description", func() { diff --git a/scanner/scanner.go b/scanner/scanner.go index 871b0c696..b7675afc8 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -24,6 +24,7 @@ type scannerImpl struct { ds model.DataStore cw artwork.CacheWarmer pls playlists.Playlists + spe playlists.SmartPlaylistEvaluator } // scanState holds the state of an in-progress scan, to be passed to the various phases @@ -148,7 +149,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] runPhase[*model.Album](ctx, 3, createPhaseRefreshAlbums(ctx, &state, s.ds)), // Phase 4: Import/update playlists - runPhase[*model.Folder](ctx, 4, createPhasePlaylists(ctx, &state, s.ds, s.pls, s.cw)), + runPhase[*model.Folder](ctx, 4, createPhasePlaylists(ctx, &state, s.ds, s.pls, s.cw, s.spe)), ), // Final Steps (cannot be parallelized): diff --git a/scanner/scanner_benchmark_test.go b/scanner/scanner_benchmark_test.go index 8f0dcd340..60b7ace13 100644 --- a/scanner/scanner_benchmark_test.go +++ b/scanner/scanner_benchmark_test.go @@ -41,7 +41,7 @@ func BenchmarkScan(b *testing.B) { ds := persistence.New(db.Db()) conf.Server.DevExternalScanner = false s := scanner.New(context.Background(), ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) fs := storagetest.FakeFS{} storagetest.Register("fake", &fs) diff --git a/scanner/scanner_multilibrary_test.go b/scanner/scanner_multilibrary_test.go index 856015239..ca7ed1e6c 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -78,7 +78,7 @@ var _ = Describe("Scanner - Multi-Library", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) // Create two test libraries (let DB auto-assign IDs) lib1 = model.Library{Name: "Rock Collection", Path: "rock:///music"} diff --git a/scanner/scanner_selective_test.go b/scanner/scanner_selective_test.go index 594b74e38..1b97b3376 100644 --- a/scanner/scanner_selective_test.go +++ b/scanner/scanner_selective_test.go @@ -64,7 +64,7 @@ var _ = Describe("ScanFolders", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} Expect(ds.Library(ctx).Put(&lib)).To(Succeed()) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 922d21e62..24b545e44 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -85,7 +85,7 @@ var _ = Describe("Scanner", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} Expect(ds.Library(ctx).Put(&lib)).To(Succeed()) diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 262a5ed36..91981a9de 100644 --- a/server/e2e/e2e_suite_test.go +++ b/server/e2e/e2e_suite_test.go @@ -453,7 +453,7 @@ var _ = BeforeSuite(func() { buildTestFS() s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(initDS, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(initDS, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, true) Expect(err).ToNot(HaveOccurred()) @@ -490,7 +490,7 @@ func setupTestDB() { streamerSpy = &spyStreamer{} decider := stream.NewTranscodeDecider(ds, noopFFmpeg{}) s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) router = subsonic.New( ds, noopArtwork{}, diff --git a/server/e2e/subsonic_multilibrary_test.go b/server/e2e/subsonic_multilibrary_test.go index a837da124..88f4d4cd3 100644 --- a/server/e2e/subsonic_multilibrary_test.go +++ b/server/e2e/subsonic_multilibrary_test.go @@ -54,7 +54,7 @@ var _ = Describe("Multi-Library Support", Ordered, func() { // Run incremental scan to import lib2 content (lib1 files unchanged → skipped) s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds, core.NewImageUploadService()), playlists.NoopSmartPlaylistEvaluator(), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, false) Expect(err).ToNot(HaveOccurred()) From 03b813a2f635f9c0f691802cbf0a3537d4cd3e4f Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Mar 2026 20:15:19 -0400 Subject: [PATCH 4/5] fix(playlists): simplify Evaluate to reuse refreshSmartPlaylist directly Instead of refactoring refreshSmartPlaylist into guard+core methods, Evaluate simply resets EvaluatedAt and delegates to refreshSmartPlaylist. This avoids unnecessary refactoring while still bypassing the delay guard. Also returns an error if evaluation fails. --- persistence/playlist_repository.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 1aed46e92..45f63cdac 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -215,10 +215,6 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return false } - return r.doRefreshSmartPlaylist(pls, usr.ID) -} - -func (r *playlistRepository) doRefreshSmartPlaylist(pls *model.Playlist, userID string) bool { log.Debug(r.ctx, "Refreshing smart playlist", "playlist", pls.Name, "id", pls.ID) start := time.Now() @@ -248,11 +244,11 @@ func (r *playlistRepository) doRefreshSmartPlaylist(pls *model.Playlist, userID From("media_file").LeftJoin("annotation on ("+ "annotation.item_id = media_file.id"+ " AND annotation.item_type = 'media_file'"+ - " AND annotation.user_id = ?)", userID) + " AND annotation.user_id = ?)", usr.ID) // Conditionally join album/artist annotation tables only when referenced by criteria or sort requiredJoins := rules.RequiredJoins() - sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, userID) + sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, usr.ID) // Only include media files from libraries the user has access to sq = r.applyLibraryFilter(sq, "media_file") @@ -265,8 +261,8 @@ func (r *playlistRepository) doRefreshSmartPlaylist(pls *model.Playlist, userID LeftJoin("annotation on ("+ "annotation.item_id = media_file.id"+ " AND annotation.item_type = 'media_file'"+ - " AND annotation.user_id = ?)", userID) - countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, userID) + " AND annotation.user_id = ?)", usr.ID) + countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, usr.ID) countSq = r.applyLibraryFilter(countSq, "media_file") countSq = countSq.Where(rules) @@ -322,7 +318,11 @@ func (r *playlistRepository) Evaluate(id string) error { if !pls.IsSmartPlaylist() { return nil } - r.doRefreshSmartPlaylist(pls, pls.OwnerID) + // Reset EvaluatedAt so refreshSmartPlaylist won't skip due to delay check + pls.EvaluatedAt = nil + if !r.refreshSmartPlaylist(pls) { + return fmt.Errorf("failed to evaluate smart playlist %s", id) + } return nil } From 359ea3c61cda4781f91dc48e882c15170996f417 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Mar 2026 20:25:31 -0400 Subject: [PATCH 5/5] refactor(playlists): improve evaluator efficiency and logging - Use time.NewTimer instead of time.After to avoid timer leaks - Create admin context once per batch instead of per playlist - Pass context to log calls for consistent request-scoped fields --- core/playlists/evaluator.go | 26 ++++++++++++-------------- scanner/phase_4_playlists.go | 2 +- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/core/playlists/evaluator.go b/core/playlists/evaluator.go index 0b237c936..d0d3463ca 100644 --- a/core/playlists/evaluator.go +++ b/core/playlists/evaluator.go @@ -69,31 +69,29 @@ func (e *smartPlaylistEvaluator) run() { } func (e *smartPlaylistEvaluator) waitSignal(timeout time.Duration) { + timer := time.NewTimer(timeout) + defer timer.Stop() select { - case <-time.After(timeout): + case <-timer.C: case <-e.wakeSignal: } } func (e *smartPlaylistEvaluator) processBatch(batch []string) { - log.Debug("Evaluating smart playlists in background", "count", len(batch)) - for _, id := range batch { - e.doEvaluate(id) - } -} - -func (e *smartPlaylistEvaluator) doEvaluate(id string) { // Use admin context so userFilter() returns all playlists. // Evaluate() internally uses pls.OwnerID for annotation JOINs. ctx := auth.WithAdminUser(context.TODO(), e.ds) - start := time.Now() - err := e.ds.Playlist(ctx).Evaluate(id) - if err != nil { - log.Error("Error evaluating smart playlist in background", "id", id, err) - return + log.Debug(ctx, "Evaluating smart playlists in background", "count", len(batch)) + for _, id := range batch { + start := time.Now() + err := e.ds.Playlist(ctx).Evaluate(id) + if err != nil { + log.Error(ctx, "Error evaluating smart playlist in background", "id", id, err) + continue + } + log.Debug(ctx, "Smart playlist evaluation complete", "id", id, "elapsed", time.Since(start)) } - log.Debug("Background smart playlist evaluation complete", "id", id, "elapsed", time.Since(start)) } // NoopSmartPlaylistEvaluator returns an evaluator that does nothing. diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index e3d5f16d8..3c8c6aedc 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -109,7 +109,7 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. } if pls.IsSmartPlaylist() { p.spe.Enqueue(pls.ID) - log.Debug("Scanner: Imported smart playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "elapsed", time.Since(started)) + log.Debug(p.ctx, "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)) }