From bf72f0076013ff24dd1087648ea19aa4ab41ce79 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 7 Oct 2026 13:33:39 -0400 Subject: [PATCH 1/2] fix(persistence): keep song pages correct past the large-offset threshold Past DevOffsetOptimize (50,000 by default) GetAll and the legacy LIKE search exclude earlier rows with a "rowid NOT IN (subquery)". The subquery selected only the rowid, so its ORDER BY could not use the outer column names and SQLite matched them against every joined table. Sorting songs by id, path, updatedAt or comment failed with "ambiguous column name", so /api/song returned 500 for those sorts. Album legacy search failed the same way on name. The subquery and the outer query also ordered tied rows differently, so sorts with many equal values (starred, rating, playCount, ...) repeated some songs and skipped others once past the threshold, without errors. The subquery now keeps the outer projection, so ORDER BY resolves the same way in both queries. Paginated queries get a rowid tie-breaker, so the order is total and every page agrees on where ties go. Every index ends with the rowid, so index-served sorts stay index-served. createdAt and updatedAt now sort by (date, id) to match their (date, id) indexes; with the date alone, the rowid tie-breaker would need a temp B-tree. Fixes #6285 --- persistence/mediafile_repository.go | 3 +- persistence/sql_base_repository.go | 27 +++-- persistence/sql_base_repository_test.go | 134 ++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 10 deletions(-) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index b9a5c3374..d9d417312 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -96,7 +96,8 @@ func NewMediaFileRepository(db dbx.Builder) model.MediaFileRepository { "album_artist": "order_album_artist_name, order_album_name, release_date, disc_number, track_number", "album": "order_album_name, album_id, disc_number, track_number, order_artist_name, " + naturalSort("media_file.title"), "random": "random", - "created_at": "media_file.created_at", + "created_at": "media_file.created_at, media_file.id", + "updated_at": "media_file.updated_at, media_file.id", "recently_added": mediaFileRecentlyAddedSort(), "starred_at": "starred, starred_at", "rated_at": "rating, rated_at", diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 03cc6a01b..f0ac36536 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -409,9 +409,7 @@ func wrapCursor[D, T any](cursor iter.Seq2[D, error], toModel func(D) *T) iter.S // queryWithStableResults is a helper function to execute a query and return an iterator that will yield its results // from a cursor, guaranteeing that the results will be stable, even if the underlying data changes. func queryWithStableResults[T any](ctx context.Context, r sqlRepository, sq SelectBuilder, options ...model.QueryOptions) (iter.Seq2[T, error], error) { - if len(options) > 0 && options[0].Offset > 0 { - sq = r.optimizePagination(sq, options[0]) - } + sq = r.paginate(sq, options...) query, args, err := r.toSQL(sq) if err != nil { return nil, err @@ -439,9 +437,7 @@ func queryWithStableResults[T any](ctx context.Context, r sqlRepository, sq Sele } func (r sqlRepository) queryAll(ctx context.Context, sq SelectBuilder, response any, options ...model.QueryOptions) error { - if len(options) > 0 && options[0].Offset > 0 { - sq = r.optimizePagination(sq, options[0]) - } + sq = r.paginate(sq, options...) query, args, err := r.toSQL(sq) if err != nil { return err @@ -472,15 +468,28 @@ func (r sqlRepository) queryAllSlice(ctx context.Context, sq SelectBuilder, resp return err } +// paginate adds a rowid tie-breaker, so the order is total and rows with equal sort values land on the +// same page every time. Indexes already end with the rowid, so index-served sorts stay index-served. +func (r sqlRepository) paginate(sq SelectBuilder, options ...model.QueryOptions) SelectBuilder { + if len(options) == 0 || (options[0].Max == 0 && options[0].Offset == 0) { + return sq + } + order := "asc" + if strings.EqualFold(strings.TrimSpace(options[0].Order), "desc") { + order = "desc" + } + return r.optimizePagination(sq.OrderBy(r.tableName+".rowid "+order), options[0]) +} + // optimizePagination uses a less inefficient pagination, by not using OFFSET. // See https://gist.github.com/ssokolow/262503 func (r sqlRepository) optimizePagination(sq SelectBuilder, options model.QueryOptions) SelectBuilder { if options.Offset > conf.Server.DevOffsetOptimize { sq = sq.RemoveOffset() - rowidSq := sq.RemoveColumns().Columns(r.tableName + ".rowid") - rowidSq = rowidSq.Limit(uint64(options.Offset)) + // Keep the outer projection, so ORDER BY names resolve to the same columns in both queries. + rowidSq := sq.Column(r.tableName + ".rowid as _paginated_rowid").Limit(uint64(options.Offset)) rowidSql, args, _ := rowidSq.ToSql() - sq = sq.Where(r.tableName+".rowid not in ("+rowidSql+")", args...) + sq = sq.Where(r.tableName+".rowid not in (SELECT _paginated_rowid FROM ("+rowidSql+"))", args...) } return sq } diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 4b42e7f1d..ef5b2462a 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -2,8 +2,12 @@ package persistence import ( "context" + "slices" "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/hasher" @@ -421,3 +425,133 @@ var _ = Describe("sqlRepository", func() { }) }) }) + +var _ = Describe("paginated queries", func() { + var ctx context.Context + var mr model.MediaFileRepository + + BeforeEach(func() { + ctx = request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + DeferCleanup(configtest.SetupConfig()) + mr = NewMediaFileRepository(GetDBXBuilder()) + }) + + Describe("SQL", func() { + var r sqlRepository + BeforeEach(func() { + r = sqlRepository{tableName: "media_file", sortMappings: map[string]string{"name": "title"}} + conf.Server.DevOffsetOptimize = 10 + }) + + DescribeTable("adds a rowid tie-breaker in the requested direction", + func(options model.QueryOptions, expectedOrder string) { + sql, _, err := r.paginate(squirrel.Select("*").From("media_file"), options).ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(HaveSuffix(expectedOrder)) + }, + Entry("no sort", model.QueryOptions{Max: 5}, "ORDER BY media_file.rowid asc"), + Entry("desc", model.QueryOptions{Sort: "name", Order: "desc", Max: 5}, "ORDER BY media_file.rowid desc"), + ) + + It("leaves unpaginated queries alone", func() { + sql, _, _ := r.paginate(squirrel.Select("*").From("media_file"), model.QueryOptions{Sort: "name"}).ToSql() + Expect(sql).To(Equal("SELECT * FROM media_file")) + }) + + It("uses the rowid subquery only past the threshold, keeping the outer projection", func() { + sq := squirrel.Select("media_file.*", "library.path as library_path").From("media_file").OrderBy("id asc") + atThreshold, _, _ := r.paginate(sq.Limit(5).Offset(10), model.QueryOptions{Max: 5, Offset: 10}).ToSql() + Expect(atThreshold).ToNot(ContainSubstring("not in")) + + past, _, _ := r.paginate(sq.Limit(5).Offset(11), model.QueryOptions{Max: 5, Offset: 11}).ToSql() + Expect(past).To(ContainSubstring("media_file.rowid not in (SELECT _paginated_rowid FROM (" + + "SELECT media_file.*, library.path as library_path, media_file.rowid as _paginated_rowid FROM media_file")) + Expect(past).ToNot(ContainSubstring("OFFSET")) + }) + }) + + // walk pages through the repository, crossing the optimizer threshold + walk := func(ctx context.Context, sort, order string, pageSize int) []string { + var ids []string + for offset := 0; ; offset += pageSize { + page, err := mr.GetAll(ctx, model.QueryOptions{Sort: sort, Order: order, Offset: offset, Max: pageSize, Seed: "seed"}) + Expect(err).ToNot(HaveOccurred(), "sort=%s order=%s offset=%d", sort, order, offset) + for _, m := range page { + ids = append(ids, m.ID) + } + if len(page) < pageSize { + return ids + } + } + } + + DescribeTable("returns every song exactly once, in the same order as a single page", + func(sort string) { + conf.Server.DevOffsetOptimize = 2 + for _, order := range []string{"asc", "desc"} { + expected := walk(ctx, sort, order, 1000) + Expect(len(expected)).To(BeNumerically(">=", len(testSongs))) + Expect(slices.Compact(slices.Sorted(slices.Values(expected)))).To(HaveLen(len(expected))) + // Page size 2 hits offset == threshold (plain OFFSET), size 3 hits offset > threshold first. + for _, pageSize := range []int{2, 3} { + Expect(walk(ctx, sort, order, pageSize)).To(Equal(expected), "sort=%s order=%s page=%d", sort, order, pageSize) + } + } + }, + Entry("id", "id"), + Entry("path", "path"), + Entry("updatedAt", "updatedAt"), + Entry("comment", "comment"), + Entry("title", "title"), + Entry("createdAt", "createdAt"), + Entry("starred (ties)", "starred"), + Entry("playCount (ties)", "playCount"), + Entry("rating (ties)", "rating"), + Entry("album", "album"), + Entry("seeded random", "random"), + ) + + It("keeps the library filter on every page", func() { + _, otherLib, restrictedUser := restrictedFixture("paginate") + hidden := model.MediaFile{ID: "paginate-hidden", Title: "Hidden", LibraryID: otherLib.ID, Path: p("other/hidden.mp3")} + Expect(mr.Put(ctx, &hidden)).To(Succeed()) + DeferCleanup(func() { _ = mr.Delete(ctx, hidden.ID) }) + conf.Server.DevOffsetOptimize = 2 + + Expect(walk(ctx, "id", "asc", 1000)).To(ContainElement(hidden.ID)) + restrictedCtx := request.WithUser(ctx, restrictedUser) + expected := walk(restrictedCtx, "id", "asc", 1000) + Expect(expected).ToNot(BeEmpty()) + Expect(expected).ToNot(ContainElement(hidden.ID)) + Expect(walk(restrictedCtx, "id", "asc", 3)).To(Equal(expected)) + }) + + Describe("legacy search", func() { + BeforeEach(func() { + conf.Server.Search.Backend = "legacy" + conf.Server.DevOffsetOptimize = 0 + }) + + It("pages albums whose name is shared with the joined library", func() { + alr := NewAlbumRepository(GetDBXBuilder()) + all, err := alr.Search(ctx, "abbey", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(all).To(HaveLen(2)) + second, err := alr.Search(ctx, "abbey", model.QueryOptions{Max: 1, Offset: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(second).To(HaveLen(1)) + Expect(second[0].ID).To(Equal(all[1].ID)) + }) + + It("pages artists", func() { + arr := NewArtistRepository(GetDBXBuilder()) + all, err := arr.Search(ctx, "the", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(all)).To(BeNumerically(">=", 2)) + second, err := arr.Search(ctx, "the", model.QueryOptions{Max: 1, Offset: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(second).To(HaveLen(1)) + Expect(second[0].ID).To(Equal(all[1].ID)) + }) + }) +}) From b78c963931563f23379e4762667a7b067fab864e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 8 Oct 2026 14:34:42 -0700 Subject: [PATCH 2/2] fix(scanner): respect FollowSymlinks in watcher-triggered scans (#6293) * fix(scanner): respect FollowSymlinks in watcher-triggered scans With FollowSymlinks disabled, creating a folder symlink inside the library made the watcher schedule a selective scan with the link itself as the target. walkDirTree only checked FollowSymlinks for child entries, so it walked the link and imported its files as duplicates (or, for links that point outside the library, files that should never be scanned). walkDirTree now skips any target folder whose path, or any parent folder, is a symlink when FollowSymlinks is disabled. The skipped target stays in lastUpdates, so rows previously imported through it are marked missing, matching what a full scan does. localFS now implements fs.ReadLinkFS so fs.Lstat can see symlinks instead of following them. Fixes #6292 * test(scanner): run the #6292 symlinked target tests on Windows Remove the SkipOnWindows guard from the symlinked target folder tests, so the go-windows CI job covers the FollowSymlinks fix for selective scans. --- core/storage/local/local.go | 11 +++++++ scanner/walk_dir_tree.go | 19 +++++++++++ scanner/walk_dir_tree_test.go | 62 ++++++++++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 8 deletions(-) diff --git a/core/storage/local/local.go b/core/storage/local/local.go index 686838565..e2ce00a1b 100644 --- a/core/storage/local/local.go +++ b/core/storage/local/local.go @@ -76,6 +76,17 @@ func (lfs *localFS) ResolveSymlink(name string) (string, error) { return filepath.EvalSymlinks(filepath.Join(lfs.root, filepath.FromSlash(name))) } +// ReadLink and Lstat implement fs.ReadLinkFS, so callers can detect symlinks without following them. +var _ fs.ReadLinkFS = (*localFS)(nil) + +func (lfs *localFS) ReadLink(name string) (string, error) { + return fs.ReadLink(lfs.FS, name) +} + +func (lfs *localFS) Lstat(name string) (fs.FileInfo, error) { + return fs.Lstat(lfs.FS, name) +} + func (lfs *localFS) ReadTags(path ...string) (map[string]metadata.Info, error) { res, err := lfs.extractor.Parse(path...) if err != nil { diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 1864a6a44..d39cad5f6 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -43,6 +43,13 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- continue } + // A full walk never descends into symlinked folders when following is disabled, so a + // target reached through one (e.g. a watcher event for a new link) is skipped too. + if !conf.Server.Scanner.FollowSymlinks && isSymlinkedPath(job.fs, folderPath) { + log.Debug(ctx, "Scanner: Skipping symlinked target folder, following is disabled", "path", folderPath) + continue + } + // Create checker and push patterns from root to this folder checker := newIgnoreChecker(job.fs) err = checker.PushAllParents(ctx, folderPath) @@ -225,6 +232,18 @@ func isDirOrSymlinkToDir(fsys fs.FS, baseDir string, dirEnt fs.DirEntry) (bool, return fileInfo.IsDir(), nil } +// isSymlinkedPath returns true if folderPath, or any of its parent folders, is a symbolic link. +// It needs fsys to implement fs.ReadLinkFS, otherwise links are followed and never detected. +func isSymlinkedPath(fsys fs.FS, folderPath string) bool { + for p := path.Clean(folderPath); p != "." && p != "/"; p = path.Dir(p) { + info, err := fs.Lstat(fsys, p) + if err == nil && info.Mode()&fs.ModeSymlink != 0 { + return true + } + } + return false +} + const maxSymlinkHops = 40 // resolveEntryName returns the name to classify the entry by, and whether to diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 43939e5c2..0c2aba3e0 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -4,8 +4,10 @@ import ( "context" "fmt" "io/fs" + "maps" "os" "path/filepath" + "slices" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -260,6 +262,44 @@ var _ = Describe("walk_dir_tree", func() { // Folders not in targets should remain in lastUpdates Expect(job.lastUpdates).To(HaveKey(model.FolderID(job.lib, "OtherArtist/Album3"))) }) + + // #6292: a watcher event for a new folder symlink makes the link itself a scan target + Context("symlinked target folders (production local storage FS)", func() { + BeforeEach(func() { + libRoot := GinkgoT().TempDir() + Expect(os.MkdirAll(filepath.Join(libRoot, "Mozart", "Album1"), 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(libRoot, "Mozart", "Album1", "track.mp3"), []byte("AUDIO"), 0600)).To(Succeed()) + Expect(os.Symlink("Mozart", filepath.Join(libRoot, "Wolfgang Amadeus Mozart"))).To(Succeed()) + job = &scanJob{fs: newLocalMusicFS(libRoot), lib: model.Library{Path: libRoot}} + }) + + walkTargets := func(targets ...string) map[string]*folderEntry { + results, err := walkDirTree(ctx, job, targets...) + Expect(err).ToNot(HaveOccurred()) + folders := map[string]*folderEntry{} + for folder := range results { + folders[folder.path] = folder + } + return folders + } + + DescribeTable("with FollowSymlinks disabled", + func(target string, expected ...string) { + conf.Server.Scanner.FollowSymlinks = false + Expect(slices.Collect(maps.Keys(walkTargets(target)))).To(ConsistOf(expected)) + }, + Entry("skips a target that is a symlink", "Wolfgang Amadeus Mozart"), + Entry("skips a target under a symlinked folder", "Wolfgang Amadeus Mozart/Album1"), + Entry("walks a regular target", "Mozart", "Mozart", "Mozart/Album1"), + ) + + It("walks a symlinked target when FollowSymlinks is enabled", func() { + conf.Server.Scanner.FollowSymlinks = true + folders := walkTargets("Wolfgang Amadeus Mozart") + Expect(folders).To(HaveKey("Wolfgang Amadeus Mozart/Album1")) + Expect(folders["Wolfgang Amadeus Mozart/Album1"].audioFiles).To(HaveKey("track.mp3")) + }) + }) }) }) @@ -433,8 +473,8 @@ var _ = Describe("walk_dir_tree", func() { }) // Regression for #5752: the production localFS must resolve file symlinks. - // It wraps os.DirFS behind the fs.FS interface, so fs.ReadLink-based - // resolution is not available and full OS-level resolution is required. + // fs.ReadLink-based resolution can't follow targets outside the library + // root, so full OS-level resolution is required. Context("production local storage FS", func() { var libRoot string var musicFS storage.MusicFS @@ -460,12 +500,7 @@ var _ = Describe("walk_dir_tree", func() { Expect(os.Symlink(filepath.Join(pool, "mid.wav"), filepath.Join(libRoot, "evil.wav"))).To(Succeed()) Expect(os.Symlink(filepath.Join(pool, "missing.mp3"), filepath.Join(libRoot, "broken.mp3"))).To(Succeed()) - u, err := storage.LocalPathToURL(libRoot) - Expect(err).ToNot(HaveOccurred()) - s, err := storage.For(u.String()) - Expect(err).ToNot(HaveOccurred()) - musicFS, err = s.FS() - Expect(err).ToNot(HaveOccurred()) + musicFS = newLocalMusicFS(libRoot) }) walkRoot := func() *folderEntry { @@ -700,6 +735,17 @@ func getDirEntry(baseDir, name string) os.DirEntry { panic(fmt.Sprintf("Could not find %s in %s", name, baseDir)) } +// newLocalMusicFS returns the production local storage MusicFS rooted at libRoot +func newLocalMusicFS(libRoot string) storage.MusicFS { + u, err := storage.LocalPathToURL(libRoot) + Expect(err).ToNot(HaveOccurred()) + s, err := storage.For(u.String()) + Expect(err).ToNot(HaveOccurred()) + musicFS, err := s.FS() + Expect(err).ToNot(HaveOccurred()) + return musicFS +} + // mockMusicFS is a mock implementation of the MusicFS interface that supports symlinks type mockMusicFS struct { storage.MusicFS