mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-18 07:30:29 +00:00
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
This commit is contained in:
parent
c22ce9ebb2
commit
210fd55021
@ -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 {
|
||||
|
||||
@ -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 {
|
||||
|
||||
@ -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{}{})
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
@ -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.
|
||||
|
||||
@ -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))
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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())
|
||||
})
|
||||
})
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user