fix(scanner): import playlists skipped when no admin existed yet (#5609)

* fix(scanner): import playlists skipped when no admin existed yet (#5499)

On a fresh install the first scan runs before any admin user exists, so the
scanner's playlist phase skips all playlists (playlists are owned by the
first admin). Nothing re-imported them afterwards because folder selection
is gated on updated_at > last_scan_at, which nothing bumps.

The playlist phase now:
- resolves the admin at phase time (FindFirstAdmin) instead of trusting the
  context snapshot taken at scan start, so a long admin-less scan still
  imports playlists in its own phase if an admin was created meanwhile;
- records a persisted PlaylistsImportPending flag when no admin exists yet;
- when that flag is set, imports ALL playlist folders via a new
  GetAllWithPlaylists (bypassing the timestamp gate) and clears the flag.

Playlists are recovered by the next scan that runs with an admin, with no
dependency on scan duration and no changes to the auth/server layers.

* fix(scanner): surface datastore errors in playlist import deferral (#5499)

Address review feedback:
- distinguish model.ErrNotFound (no admin yet -> defer) from real datastore
  errors when resolving the admin, so DB failures are propagated, not swallowed;
- propagate the error if the pending-import flag can't be persisted, so a scan
  doesn't complete as successful without recording the recovery;
- surface read errors when checking the pending flag.

Also name the no-admin condition for readability.

* fix(scanner): simplify admin existence check in playlist import

Signed-off-by: Deluan <deluan@navidrome.org>

* fix(scanner): streamline folder access in playlist import logic

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão 2026-06-14 13:39:16 -04:00 committed by GitHub
parent f3887df334
commit 2c90685bc2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 226 additions and 24 deletions

View file

@ -14,6 +14,9 @@ const (
DefaultDbPath = "navidrome.db?cache=shared&_busy_timeout=15000&_journal_mode=WAL&_foreign_keys=on&synchronous=normal" DefaultDbPath = "navidrome.db?cache=shared&_busy_timeout=15000&_journal_mode=WAL&_foreign_keys=on&synchronous=normal"
InitialSetupFlagKey = "InitialSetup" InitialSetupFlagKey = "InitialSetup"
FullScanAfterMigrationFlagKey = "FullScanAfterMigration" FullScanAfterMigrationFlagKey = "FullScanAfterMigration"
// PlaylistsImportPendingFlagKey marks that playlist import was deferred because
// no admin user existed yet; the next scan with an admin imports them.
PlaylistsImportPendingFlagKey = "PlaylistsImportPending"
LastScanErrorKey = "LastScanError" LastScanErrorKey = "LastScanError"
LastScanTypeKey = "LastScanType" LastScanTypeKey = "LastScanType"
LastScanStartTimeKey = "LastScanStartTime" LastScanStartTimeKey = "LastScanStartTime"

View file

@ -93,4 +93,7 @@ type FolderRepository interface {
Put(*Folder) error Put(*Folder) error
MarkMissing(missing bool, ids ...string) error MarkMissing(missing bool, ids ...string) error
GetTouchedWithPlaylists() (FolderCursor, error) GetTouchedWithPlaylists() (FolderCursor, error)
// GetAllWithPlaylists returns all non-missing folders with playlists, ignoring
// the scan-timestamp gate used by GetTouchedWithPlaylists.
GetAllWithPlaylists() (FolderCursor, error)
} }

View file

@ -250,6 +250,18 @@ func (r folderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error)
return wrapFolderCursor(cursor), nil return wrapFolderCursor(cursor), nil
} }
func (r folderRepository) GetAllWithPlaylists() (model.FolderCursor, error) {
query := r.selectFolder().Where(And{
Eq{"missing": false},
Gt{"num_playlists": 0},
})
cursor, err := queryWithStableResults[dbFolder](r.sqlRepository, query)
if err != nil {
return nil, err
}
return wrapFolderCursor(cursor), nil
}
func wrapFolderCursor(cursor iter.Seq2[dbFolder, error]) model.FolderCursor { func wrapFolderCursor(cursor iter.Seq2[dbFolder, error]) model.FolderCursor {
return func(yield func(model.Folder, error) bool) { return func(yield func(model.Folder, error) bool) {
for f, err := range cursor { for f, err := range cursor {

View file

@ -317,4 +317,36 @@ var _ = Describe("FolderRepository", func() {
Expect(folders[0].ID).To(Equal("f1")) Expect(folders[0].ID).To(Equal("f1"))
}) })
}) })
Describe("GetAllWithPlaylists", func() {
It("returns all non-missing folders with playlists, ignoring the scan-timestamp gate", func() {
withPls := model.NewFolder(testLib, "TestAllPls/WithPls")
withPls.NumPlaylists = 2
noPls := model.NewFolder(testLib, "TestAllPls/NoPls")
noPls.NumPlaylists = 0
missingWithPls := model.NewFolder(testLib, "TestAllPls/Missing")
missingWithPls.NumPlaylists = 1
missingWithPls.Missing = true
Expect(repo.Put(withPls)).To(Succeed())
Expect(repo.Put(noPls)).To(Succeed())
Expect(repo.Put(missingWithPls)).To(Succeed())
// Force the folder's updated_at to the past so GetTouchedWithPlaylists
// (which gates on updated_at > last_scan_at) would NOT return it.
_, err := conn.NewQuery("UPDATE folder SET updated_at = {:t} WHERE id = {:id}").
Bind(dbx.Params{"t": "2000-01-01 00:00:00", "id": withPls.ID}).Execute()
Expect(err).ToNot(HaveOccurred())
var ids []string
cursor, err := repo.GetAllWithPlaylists()
Expect(err).ToNot(HaveOccurred())
for f, err := range cursor {
Expect(err).ToNot(HaveOccurred())
ids = append(ids, f.ID)
}
Expect(ids).To(ConsistOf(withPls.ID)) // only the non-missing folder with playlists
})
})
}) })

View file

@ -2,6 +2,7 @@ package scanner
import ( import (
"context" "context"
"errors"
"fmt" "fmt"
"os" "os"
"strings" "strings"
@ -10,6 +11,7 @@ import (
ppl "github.com/google/go-pipeline/pkg/pipeline" ppl "github.com/google/go-pipeline/pkg/pipeline"
"github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/artwork"
"github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/playlists"
"github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/log"
@ -18,12 +20,13 @@ import (
) )
type phasePlaylists struct { type phasePlaylists struct {
ctx context.Context ctx context.Context
scanState *scanState scanState *scanState
ds model.DataStore ds model.DataStore
pls playlists.Playlists pls playlists.Playlists
cw artwork.CacheWarmer cw artwork.CacheWarmer
refreshed atomic.Uint32 refreshed atomic.Uint32
pendingImport bool
} }
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) *phasePlaylists {
@ -49,22 +52,41 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error {
log.Info(p.ctx, "Playlists will not be imported, AutoImportPlaylists is set to false") log.Info(p.ctx, "Playlists will not be imported, AutoImportPlaylists is set to false")
return nil return nil
} }
u, _ := request.UserFrom(p.ctx)
if !u.IsAdmin || u.ID == "" { // Resolve the admin at phase time (the producer runs late in the scan), so an
log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet, "+ // admin created while the scan was in progress is picked up. Assigned once,
"Please create an admin user first, and then update the playlists for them to be imported") // before any put() below, so the channel send synchronizes it with the stages.
return nil admin, err := p.ds.User(p.ctx).FindFirstAdmin()
if err != nil && !errors.Is(err, model.ErrNotFound) {
return fmt.Errorf("finding admin user: %w", err)
}
noAdmin := admin == nil || admin.ID == ""
if noAdmin {
return p.deferImport()
}
p.ctx = request.WithUser(p.ctx, *admin)
// When recovering a deferred import, scan all playlist folders, not just touched ones.
pending, err := p.importPending()
if err != nil {
return fmt.Errorf("checking pending playlist import: %w", err)
}
p.pendingImport = pending
var cursor model.FolderCursor
if p.pendingImport {
cursor, err = p.ds.Folder(p.ctx).GetAllWithPlaylists()
} else {
cursor, err = p.ds.Folder(p.ctx).GetTouchedWithPlaylists()
}
if err != nil {
return fmt.Errorf("loading folders with playlists: %w", err)
} }
count := 0 count := 0
cursor, err := p.ds.Folder(p.ctx).GetTouchedWithPlaylists() log.Debug(p.ctx, "Scanner: Checking playlists that may need refresh", "pendingImport", p.pendingImport)
if err != nil {
return fmt.Errorf("loading touched folders: %w", err)
}
log.Debug(p.ctx, "Scanner: Checking playlists that may need refresh")
for folder, err := range cursor { for folder, err := range cursor {
if err != nil { if err != nil {
return fmt.Errorf("loading touched folder: %w", err) return fmt.Errorf("loading folder with playlists: %w", err)
} }
count++ count++
put(&folder) put(&folder)
@ -78,6 +100,23 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error {
return nil return nil
} }
// deferImport records the pending-import flag so a later scan with an admin can
// import the playlists, and returns an error if the flag can't be persisted (so
// the scan does not complete as successful without recording the recovery).
func (p *phasePlaylists) deferImport() error {
if err := p.ds.Property(p.ctx).Put(consts.PlaylistsImportPendingFlagKey, "1"); err != nil {
return fmt.Errorf("recording pending playlist import: %w", err)
}
log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet. "+
"They will be imported automatically once an admin user is created.")
return nil
}
func (p *phasePlaylists) importPending() (bool, error) {
v, err := p.ds.Property(p.ctx).DefaultGet(consts.PlaylistsImportPendingFlagKey, "0")
return v == "1", err
}
func (p *phasePlaylists) stages() []ppl.Stage[*model.Folder] { func (p *phasePlaylists) stages() []ppl.Stage[*model.Folder] {
return []ppl.Stage[*model.Folder]{ return []ppl.Stage[*model.Folder]{
ppl.NewStage(p.processPlaylistsInFolder, ppl.Name("process playlists in folder"), ppl.Concurrency(3)), ppl.NewStage(p.processPlaylistsInFolder, ppl.Name("process playlists in folder"), ppl.Concurrency(3)),
@ -123,6 +162,11 @@ func (p *phasePlaylists) finalize(err error) error {
} else { } else {
p.scanState.changesDetected.Store(true) p.scanState.changesDetected.Store(true)
} }
if p.pendingImport && err == nil {
if derr := p.ds.Property(p.ctx).Delete(consts.PlaylistsImportPendingFlagKey); derr != nil {
log.Warn(p.ctx, "Scanner: Could not clear pending playlist-import flag", derr)
}
}
logF(p.ctx, "Scanner: Finished refreshing playlists", "refreshed", refreshed, err) logF(p.ctx, "Scanner: Finished refreshing playlists", "refreshed", refreshed, err)
return err return err
} }

View file

@ -9,10 +9,10 @@ import (
"github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/artwork"
"github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/playlists"
"github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
"github.com/navidrome/navidrome/tests" "github.com/navidrome/navidrome/tests"
. "github.com/onsi/ginkgo/v2" . "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega" . "github.com/onsi/gomega"
@ -30,14 +30,22 @@ var _ = Describe("phasePlaylists", func() {
cw artwork.CacheWarmer cw artwork.CacheWarmer
) )
var userRepo *tests.MockedUserRepo
var propRepo *tests.MockedPropertyRepo
BeforeEach(func() { BeforeEach(func() {
DeferCleanup(configtest.SetupConfig()) DeferCleanup(configtest.SetupConfig())
conf.Server.AutoImportPlaylists = true conf.Server.AutoImportPlaylists = true
ctx = context.Background() ctx = context.Background()
ctx = request.WithUser(ctx, model.User{ID: "123", IsAdmin: true})
folderRepo = &mockFolderRepository{} folderRepo = &mockFolderRepository{}
userRepo = tests.CreateMockUserRepo()
// An admin user exists by default, so playlist import proceeds.
Expect(userRepo.Put(&model.User{ID: "123", UserName: "admin", IsAdmin: true})).To(Succeed())
propRepo = &tests.MockedPropertyRepo{}
ds = &tests.MockDataStore{ ds = &tests.MockDataStore{
MockedFolder: folderRepo, MockedFolder: folderRepo,
MockedUser: userRepo,
MockedProperty: propRepo,
} }
pls = &mockPlaylists{} pls = &mockPlaylists{}
cw = artwork.NoopCacheWarmer() cw = artwork.NoopCacheWarmer()
@ -84,6 +92,81 @@ var _ = Describe("phasePlaylists", func() {
Expect(called).To(BeFalse()) Expect(called).To(BeFalse())
Expect(err).To(MatchError(ContainSubstring("error loading folders"))) Expect(err).To(MatchError(ContainSubstring("error loading folders")))
}) })
It("sets the pending flag and imports nothing when no admin user exists", func() {
// Remove the admin user; produce resolves the admin at phase time.
userRepo.Data = map[string]*model.User{}
folderRepo.SetData(map[*model.Folder]error{
{Path: "/path/to/folder1"}: nil,
})
called := false
err := phase.produce(func(folder *model.Folder) { called = true })
Expect(err).ToNot(HaveOccurred())
Expect(called).To(BeFalse())
v, _ := propRepo.Get(consts.PlaylistsImportPendingFlagKey)
Expect(v).To(Equal("1"))
})
It("returns an error (not a silent defer) on a datastore failure resolving the admin", func() {
userRepo.Error = errors.New("db is locked")
err := phase.produce(func(folder *model.Folder) {})
Expect(err).To(MatchError(ContainSubstring("finding admin user")))
// Must NOT have set the pending flag on a real error.
_, getErr := propRepo.Get(consts.PlaylistsImportPendingFlagKey)
Expect(getErr).To(HaveOccurred())
})
It("returns an error when the pending flag cannot be persisted", func() {
userRepo.Data = map[string]*model.User{} // no admin -> defer path
propRepo.Error = errors.New("property table unavailable")
err := phase.produce(func(folder *model.Folder) {})
Expect(err).To(MatchError(ContainSubstring("recording pending playlist import")))
})
It("imports all playlist folders when the pending flag is set", func() {
Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed())
folderRepo.SetAllData(map[*model.Folder]error{
{Path: "/path/to/folder1"}: nil,
{Path: "/path/to/folder2"}: nil,
})
// Touched set is empty: proves selection used GetAllWithPlaylists.
folderRepo.SetData(map[*model.Folder]error{})
var produced []*model.Folder
err := phase.produce(func(folder *model.Folder) { produced = append(produced, folder) })
Expect(err).ToNot(HaveOccurred())
Expect(produced).To(HaveLen(2))
Expect(phase.pendingImport).To(BeTrue())
})
})
Describe("finalize", func() {
It("clears the pending flag after a successful pending import", func() {
Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed())
phase.pendingImport = true
Expect(phase.finalize(nil)).To(Succeed())
_, err := propRepo.Get(consts.PlaylistsImportPendingFlagKey)
Expect(err).To(HaveOccurred()) // deleted
})
It("keeps the pending flag when the import failed", func() {
Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed())
phase.pendingImport = true
Expect(phase.finalize(errors.New("boom"))).To(HaveOccurred())
v, _ := propRepo.Get(consts.PlaylistsImportPendingFlagKey)
Expect(v).To(Equal("1"))
})
}) })
Describe("processPlaylistsInFolder", func() { Describe("processPlaylistsInFolder", func() {
@ -141,12 +224,13 @@ func (p *mockPlaylists) ImportFromFolder(ctx context.Context, folder *model.Fold
type mockFolderRepository struct { type mockFolderRepository struct {
model.FolderRepository model.FolderRepository
data map[*model.Folder]error data map[*model.Folder]error
allData map[*model.Folder]error
} }
func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error) { func cursorFromData(data map[*model.Folder]error) model.FolderCursor {
return func(yield func(model.Folder, error) bool) { return func(yield func(model.Folder, error) bool) {
for folder, err := range f.data { for folder, err := range data {
if err != nil { if err != nil {
if !yield(model.Folder{}, err) { if !yield(model.Folder{}, err) {
return return
@ -157,9 +241,21 @@ func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, er
return return
} }
} }
}, nil }
}
func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error) {
return cursorFromData(f.data), nil
}
func (f *mockFolderRepository) GetAllWithPlaylists() (model.FolderCursor, error) {
return cursorFromData(f.allData), nil
} }
func (f *mockFolderRepository) SetData(m map[*model.Folder]error) { func (f *mockFolderRepository) SetData(m map[*model.Folder]error) {
f.data = m f.data = m
} }
func (f *mockFolderRepository) SetAllData(m map[*model.Folder]error) {
f.allData = m
}

View file

@ -57,6 +57,18 @@ func (u *MockedUserRepo) FindByUsernameWithPassword(username string) (*model.Use
return u.FindByUsername(username) return u.FindByUsername(username)
} }
func (u *MockedUserRepo) FindFirstAdmin() (*model.User, error) {
if u.Error != nil {
return nil, u.Error
}
for _, usr := range u.Data {
if usr.IsAdmin {
return usr, nil
}
}
return nil, model.ErrNotFound
}
func (u *MockedUserRepo) Get(id string) (*model.User, error) { func (u *MockedUserRepo) Get(id string) (*model.User, error) {
if u.Error != nil { if u.Error != nil {
return nil, u.Error return nil, u.Error