From 2d7b716834b4ff1936c25f1fc2896175b995e646 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 16 Dec 2025 06:38:50 -0500 Subject: [PATCH 1/5] fix(scanner): remove stale role associations when artist role changes. Fix #4242 Signed-off-by: Deluan --- persistence/album_repository_test.go | 64 ++++++++++++++++++++++++++++ persistence/sql_participations.go | 6 ++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index a062b4398..284d4dc5e 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -512,6 +512,70 @@ var _ = Describe("AlbumRepository", func() { // Clean up the test album created for this test _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) }) + + It("removes stale role associations when artist role changes", func() { + // Regression test for issue #4242: Composers displayed in albumartist list + // This happens when an artist's role changes (e.g., was both albumartist and composer, + // now only composer) and the old role association isn't properly removed. + + // Create an artist that will have changing roles + artist := &model.Artist{ + ID: "role-change-artist-1", + Name: "Role Change Artist", + OrderArtistName: "role change artist", + } + err := createArtistWithLibrary(artistRepo, artist, 1) + Expect(err).ToNot(HaveOccurred()) + + // Create album with artist as both albumartist and composer + album := &model.Album{ + LibraryID: 1, + ID: "test-album-role-change", + Name: "Test Album Role Change", + AlbumArtistID: "role-change-artist-1", + AlbumArtist: "Role Change Artist", + Participants: model.Participants{ + model.RoleAlbumArtist: { + {Artist: model.Artist{ID: "role-change-artist-1", Name: "Role Change Artist"}}, + }, + model.RoleComposer: { + {Artist: model.Artist{ID: "role-change-artist-1", Name: "Role Change Artist"}}, + }, + }, + } + + err = albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify initial state: artist has both albumartist and composer roles + expected := []albumArtistRecord{ + {ArtistID: "role-change-artist-1", Role: "albumartist", SubRole: ""}, + {ArtistID: "role-change-artist-1", Role: "composer", SubRole: ""}, + } + verifyAlbumArtists(album.ID, expected) + + // Now update album so artist is ONLY a composer (remove albumartist role) + album.Participants = model.Participants{ + model.RoleComposer: { + {Artist: model.Artist{ID: "role-change-artist-1", Name: "Role Change Artist"}}, + }, + } + + err = albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify that the albumartist role was removed - only composer should remain + // This is the key test: before the fix, the albumartist role would remain + // causing composers to appear in the albumartist filter + expectedAfter := []albumArtistRecord{ + {ArtistID: "role-change-artist-1", Role: "composer", SubRole: ""}, + } + verifyAlbumArtists(album.ID, expectedAfter) + + // Clean up + _, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artist.ID})) + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) + }) }) }) diff --git a/persistence/sql_participations.go b/persistence/sql_participations.go index d88eca45e..38b0203fa 100644 --- a/persistence/sql_participations.go +++ b/persistence/sql_participations.go @@ -51,8 +51,10 @@ func unmarshalParticipants(data string) (model.Participants, error) { } func (r sqlRepository) updateParticipants(itemID string, participants model.Participants) error { - ids := participants.AllIDs() - sqd := Delete(r.tableName + "_artists").Where(And{Eq{r.tableName + "_id": itemID}, NotEq{"artist_id": ids}}) + // Delete all existing participant entries for this item. + // This ensures stale role associations are removed when an artist's role changes + // (e.g., an artist was both albumartist and composer, but is now only composer). + sqd := Delete(r.tableName + "_artists").Where(Eq{r.tableName + "_id": itemID}) _, err := r.executeSQL(sqd) if err != nil { return err From 017676c457092570042ad1a3f877a243f6530c0c Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 16 Dec 2025 06:43:02 -0500 Subject: [PATCH 2/5] fix(ui): export all missing files instead of first 1000 Fixes #4721 --- ui/src/missing/MissingListActions.jsx | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/ui/src/missing/MissingListActions.jsx b/ui/src/missing/MissingListActions.jsx index 4bbf77115..fc5c4f7e3 100644 --- a/ui/src/missing/MissingListActions.jsx +++ b/ui/src/missing/MissingListActions.jsx @@ -1,12 +1,15 @@ import React from 'react' -import { TopToolbar, ExportButton } from 'react-admin' +import { TopToolbar, ExportButton, useListContext } from 'react-admin' import DeleteMissingFilesButton from './DeleteMissingFilesButton.jsx' -const MissingListActions = (props) => ( - - - - -) +const MissingListActions = (props) => { + const { total } = useListContext() + return ( + + + + + ) +} export default MissingListActions From cde5992c4632c455dd2bd7bfa658e585e258ae28 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 16 Dec 2025 11:37:13 -0500 Subject: [PATCH 3/5] fix(scanner): execute GetFolderUpdateInfo in batches to avoid "Expression tree is too large (maximum depth 1000)" Signed-off-by: Deluan --- persistence/folder_repository.go | 95 ++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 29 deletions(-) diff --git a/persistence/folder_repository.go b/persistence/folder_repository.go index a586746a0..f80cbde65 100644 --- a/persistence/folder_repository.go +++ b/persistence/folder_repository.go @@ -95,45 +95,82 @@ func (r folderRepository) CountAll(opt ...model.QueryOptions) (int64, error) { } func (r folderRepository) GetFolderUpdateInfo(lib model.Library, targetPaths ...string) (map[string]model.FolderUpdateInfo, error) { + // If no specific paths, return all folders in the library + if len(targetPaths) == 0 { + return r.getFolderUpdateInfoAll(lib) + } + + // Check if any path is root (return all folders) + for _, targetPath := range targetPaths { + if targetPath == "" || targetPath == "." { + return r.getFolderUpdateInfoAll(lib) + } + } + + // Process paths in batches to avoid SQLite's expression tree depth limit (max 1000). + // Each path generates ~3 conditions, so batch size of 100 keeps us well under the limit. + const batchSize = 100 + result := make(map[string]model.FolderUpdateInfo) + + for batch := range slices.Chunk(targetPaths, batchSize) { + batchResult, err := r.getFolderUpdateInfoBatch(lib, batch) + if err != nil { + return nil, err + } + for id, info := range batchResult { + result[id] = info + } + } + + return result, nil +} + +// getFolderUpdateInfoAll returns update info for all non-missing folders in the library +func (r folderRepository) getFolderUpdateInfoAll(lib model.Library) (map[string]model.FolderUpdateInfo, error) { + where := And{ + Eq{"library_id": lib.ID}, + Eq{"missing": false}, + } + return r.queryFolderUpdateInfo(where) +} + +// getFolderUpdateInfoBatch returns update info for a batch of target paths and their descendants +func (r folderRepository) getFolderUpdateInfoBatch(lib model.Library, targetPaths []string) (map[string]model.FolderUpdateInfo, error) { where := And{ Eq{"library_id": lib.ID}, Eq{"missing": false}, } - // If specific paths are requested, include those folders and all their descendants - if len(targetPaths) > 0 { - // Collect folder IDs for exact target folders and path conditions for descendants - folderIDs := make([]string, 0, len(targetPaths)) - pathConditions := make(Or, 0, len(targetPaths)*2) + // Collect folder IDs for exact target folders and path conditions for descendants + folderIDs := make([]string, 0, len(targetPaths)) + pathConditions := make(Or, 0, len(targetPaths)*2) - for _, targetPath := range targetPaths { - if targetPath == "" || targetPath == "." { - // Root path - include everything in this library - pathConditions = Or{} - folderIDs = nil - break - } - // Clean the path to normalize it. Paths stored in the folder table do not have leading/trailing slashes. - cleanPath := strings.TrimPrefix(targetPath, string(os.PathSeparator)) - cleanPath = filepath.Clean(cleanPath) + for _, targetPath := range targetPaths { + // Clean the path to normalize it. Paths stored in the folder table do not have leading/trailing slashes. + cleanPath := strings.TrimPrefix(targetPath, string(os.PathSeparator)) + cleanPath = filepath.Clean(cleanPath) - // Include the target folder itself by ID - folderIDs = append(folderIDs, model.FolderID(lib, cleanPath)) + // Include the target folder itself by ID + folderIDs = append(folderIDs, model.FolderID(lib, cleanPath)) - // Include all descendants: folders whose path field equals or starts with the target path - // Note: Folder.Path is the directory path, so children have path = targetPath - pathConditions = append(pathConditions, Eq{"path": cleanPath}) - pathConditions = append(pathConditions, Like{"path": cleanPath + "/%"}) - } - - // Combine conditions: exact folder IDs OR descendant path patterns - if len(folderIDs) > 0 { - where = append(where, Or{Eq{"id": folderIDs}, pathConditions}) - } else if len(pathConditions) > 0 { - where = append(where, pathConditions) - } + // Include all descendants: folders whose path field equals or starts with the target path + // Note: Folder.Path is the directory path, so children have path = targetPath + pathConditions = append(pathConditions, Eq{"path": cleanPath}) + pathConditions = append(pathConditions, Like{"path": cleanPath + "/%"}) } + // Combine conditions: exact folder IDs OR descendant path patterns + if len(folderIDs) > 0 { + where = append(where, Or{Eq{"id": folderIDs}, pathConditions}) + } else if len(pathConditions) > 0 { + where = append(where, pathConditions) + } + + return r.queryFolderUpdateInfo(where) +} + +// queryFolderUpdateInfo executes the query and returns the result map +func (r folderRepository) queryFolderUpdateInfo(where And) (map[string]model.FolderUpdateInfo, error) { sq := r.newSelect().Columns("id", "updated_at", "hash").Where(where) var res []struct { ID string From 8c80be56daf87958edef2f94796b1c113d39ae14 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 16 Dec 2025 12:16:00 -0500 Subject: [PATCH 4/5] fix(scanner): ensure FullScanInProgress reflects current scan request during interrupted scans Signed-off-by: Deluan --- scanner/phase_1_folders.go | 6 ++ scanner/scanner_test.go | 149 +++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 329029951..b493a94d4 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -76,6 +76,12 @@ func newScanJob(ctx context.Context, ds model.DataStore, cw artwork.CacheWarmer, log.Error(ctx, "Error getting fs for library", "library", lib.Name, "path", lib.Path, err) return nil, fmt.Errorf("getting fs for library: %w", err) } + + // Ensure FullScanInProgress reflects the current scan request. + // This is important when resuming an interrupted quick scan as a full scan: + // the DB may have FullScanInProgress=false, but we need it true for isOutdated() to work correctly. + lib.FullScanInProgress = lib.FullScanInProgress || fullScan + return &scanJob{ lib: lib, fs: fsys, diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 873065aa3..351255ae8 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -675,6 +675,155 @@ var _ = Describe("Scanner", Ordered, func() { }) }) + Describe("Interrupted scan resumption", func() { + var fsys storagetest.FakeFS + var help func(...map[string]any) *fstest.MapFile + + BeforeEach(func() { + help = template(_t{"albumartist": "The Beatles", "album": "Help!", "year": 1965}) + fsys = createFS(fstest.MapFS{ + "The Beatles/Help!/01 - Help!.mp3": help(track(1, "Help!")), + "The Beatles/Help!/02 - The Night Before.mp3": help(track(2, "The Night Before")), + }) + }) + + simulateInterruptedScan := func(fullScan bool) { + // Call ScanBegin to properly set LastScanStartedAt and FullScanInProgress + // This simulates what would happen if a scan was interrupted (ScanBegin called but ScanEnd not) + Expect(ds.Library(ctx).ScanBegin(lib.ID, fullScan)).To(Succeed()) + + // Verify the update was persisted + reloaded, err := ds.Library(ctx).Get(lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(reloaded.LastScanStartedAt).ToNot(BeZero()) + Expect(reloaded.FullScanInProgress).To(Equal(fullScan)) + } + + Context("when a quick scan is interrupted and resumed with a full scan request", func() { + BeforeEach(func() { + // First, complete a full scan to populate the database + Expect(runScanner(ctx, true)).To(Succeed()) + + // Verify files were imported + mfs, err := ds.MediaFile(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(2)) + + // Now simulate an interrupted quick scan + // (LastScanStartedAt is set, FullScanInProgress is false) + simulateInterruptedScan(false) + }) + + It("should rescan all folders when resumed as full scan", func() { + // Update a tag without changing the folder hash by preserving the original modtime. + // In a quick scan, this wouldn't be detected because the folder hash hasn't changed. + // But in a full scan, all files should be re-read regardless of hash. + origModTime := fsys.MapFS["The Beatles/Help!/01 - Help!.mp3"].ModTime + fsys.UpdateTags("The Beatles/Help!/01 - Help!.mp3", _t{"comment": "updated comment"}, origModTime) + + // Resume with a full scan - this should process all folders + // even though folder hashes haven't changed + Expect(runScanner(ctx, true)).To(Succeed()) + + // Verify the comment was updated (which means the folder was processed and file re-imported) + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"title": "Help!"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + Expect(mfs[0].Comment).To(Equal("updated comment")) + }) + }) + + Context("when a full scan is interrupted and resumed with a quick scan request", func() { + BeforeEach(func() { + // First, complete a full scan to populate the database + Expect(runScanner(ctx, true)).To(Succeed()) + + // Verify files were imported + mfs, err := ds.MediaFile(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(2)) + + // Now simulate an interrupted full scan + // (LastScanStartedAt is set, FullScanInProgress is true) + simulateInterruptedScan(true) + }) + + It("should continue as full scan even when quick scan is requested", func() { + // Update a tag without changing the folder hash by preserving the original modtime. + origModTime := fsys.MapFS["The Beatles/Help!/01 - Help!.mp3"].ModTime + fsys.UpdateTags("The Beatles/Help!/01 - Help!.mp3", _t{"comment": "full scan comment"}, origModTime) + + // Request a quick scan - but because a full scan was in progress, + // it should continue as a full scan + Expect(runScanner(ctx, false)).To(Succeed()) + + // Verify the comment was updated (folder was processed despite unchanged hash) + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"title": "Help!"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + Expect(mfs[0].Comment).To(Equal("full scan comment")) + }) + }) + + Context("when no scan was in progress", func() { + BeforeEach(func() { + // First, complete a full scan to populate the database + Expect(runScanner(ctx, true)).To(Succeed()) + + // Verify files were imported + mfs, err := ds.MediaFile(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(2)) + + // Library should have LastScanStartedAt cleared after successful scan + updatedLib, err := ds.Library(ctx).Get(lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(updatedLib.LastScanStartedAt).To(BeZero()) + Expect(updatedLib.FullScanInProgress).To(BeFalse()) + }) + + It("should respect the full scan flag for new scans", func() { + // Update a tag without changing the folder hash by preserving the original modtime. + origModTime := fsys.MapFS["The Beatles/Help!/01 - Help!.mp3"].ModTime + fsys.UpdateTags("The Beatles/Help!/01 - Help!.mp3", _t{"comment": "new full scan"}, origModTime) + + // Start a new full scan + Expect(runScanner(ctx, true)).To(Succeed()) + + // Verify the comment was updated + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"title": "Help!"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + Expect(mfs[0].Comment).To(Equal("new full scan")) + }) + + It("should not rescan unchanged folders during quick scan", func() { + // Update a tag without changing the folder hash by preserving the original modtime. + // This simulates editing tags in a file (e.g., with a tag editor) without modifying its timestamp. + // In a quick scan, this should NOT be detected because the folder hash remains unchanged. + origModTime := fsys.MapFS["The Beatles/Help!/01 - Help!.mp3"].ModTime + fsys.UpdateTags("The Beatles/Help!/01 - Help!.mp3", _t{"comment": "should not appear"}, origModTime) + + // Do a quick scan - unchanged folders should be skipped + Expect(runScanner(ctx, false)).To(Succeed()) + + // Verify the comment was NOT updated (folder was skipped) + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"title": "Help!"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + Expect(mfs[0].Comment).To(BeEmpty()) + }) + }) + }) + Describe("RefreshStats", func() { var refreshStatsCalls []bool var fsys storagetest.FakeFS From 9ed309ac8133c4b92b91ebb12c0aaa94e874496c Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 16 Dec 2025 16:08:32 -0500 Subject: [PATCH 5/5] feat(scanner): implement file-based target passing for large target lists Signed-off-by: Deluan --- cmd/scan.go | 46 ++++++++++- cmd/scan_test.go | 89 ++++++++++++++++++++++ scanner/external.go | 74 +++++++++++++++++- scanner/external_test.go | 160 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 363 insertions(+), 6 deletions(-) create mode 100644 cmd/scan_test.go create mode 100644 scanner/external_test.go diff --git a/cmd/scan.go b/cmd/scan.go index e587b8931..daf58b29c 100644 --- a/cmd/scan.go +++ b/cmd/scan.go @@ -1,9 +1,12 @@ package cmd import ( + "bufio" "context" "encoding/gob" + "fmt" "os" + "strings" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/db" @@ -19,12 +22,14 @@ var ( fullScan bool subprocess bool targets []string + targetFile string ) func init() { scanCmd.Flags().BoolVarP(&fullScan, "full", "f", false, "check all subfolders, ignoring timestamps") scanCmd.Flags().BoolVarP(&subprocess, "subprocess", "", false, "run as subprocess (internal use)") scanCmd.Flags().StringArrayVarP(&targets, "target", "t", []string{}, "list of libraryID:folderPath pairs, can be repeated (e.g., \"-t 1:Music/Rock -t 1:Music/Jazz -t 2:Classical\")") + scanCmd.Flags().StringVar(&targetFile, "target-file", "", "path to file containing targets (one libraryID:folderPath per line)") rootCmd.AddCommand(scanCmd) } @@ -71,10 +76,17 @@ func runScanner(ctx context.Context) { ds := persistence.New(sqlDB) pls := core.NewPlaylists(ds) - // Parse targets if provided + // Parse targets from command line or file var scanTargets []model.ScanTarget - if len(targets) > 0 { - var err error + var err error + + if targetFile != "" { + scanTargets, err = readTargetsFromFile(targetFile) + if err != nil { + log.Fatal(ctx, "Failed to read targets from file", err) + } + log.Info(ctx, "Scanning specific folders from file", "numTargets", len(scanTargets)) + } else if len(targets) > 0 { scanTargets, err = model.ParseTargets(targets) if err != nil { log.Fatal(ctx, "Failed to parse targets", err) @@ -94,3 +106,31 @@ func runScanner(ctx context.Context) { trackScanInteractively(ctx, progress) } } + +// readTargetsFromFile reads scan targets from a file, one per line. +// Each line should be in the format "libraryID:folderPath". +// Empty lines and lines starting with # are ignored. +func readTargetsFromFile(filePath string) ([]model.ScanTarget, error) { + file, err := os.Open(filePath) + if err != nil { + return nil, fmt.Errorf("failed to open target file: %w", err) + } + defer file.Close() + + var targetStrings []string + scanner := bufio.NewScanner(file) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + // Skip empty lines and comments + if line == "" { + continue + } + targetStrings = append(targetStrings, line) + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("failed to read target file: %w", err) + } + + return model.ParseTargets(targetStrings) +} diff --git a/cmd/scan_test.go b/cmd/scan_test.go new file mode 100644 index 000000000..beeecca19 --- /dev/null +++ b/cmd/scan_test.go @@ -0,0 +1,89 @@ +package cmd + +import ( + "os" + "path/filepath" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("readTargetsFromFile", func() { + var tempDir string + + BeforeEach(func() { + var err error + tempDir, err = os.MkdirTemp("", "navidrome-test-") + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + os.RemoveAll(tempDir) + }) + + It("reads valid targets from file", func() { + filePath := filepath.Join(tempDir, "targets.txt") + content := "1:Music/Rock\n2:Music/Jazz\n3:Classical\n" + err := os.WriteFile(filePath, []byte(content), 0600) + Expect(err).ToNot(HaveOccurred()) + + targets, err := readTargetsFromFile(filePath) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(HaveLen(3)) + Expect(targets[0]).To(Equal(model.ScanTarget{LibraryID: 1, FolderPath: "Music/Rock"})) + Expect(targets[1]).To(Equal(model.ScanTarget{LibraryID: 2, FolderPath: "Music/Jazz"})) + Expect(targets[2]).To(Equal(model.ScanTarget{LibraryID: 3, FolderPath: "Classical"})) + }) + + It("skips empty lines", func() { + filePath := filepath.Join(tempDir, "targets.txt") + content := "1:Music/Rock\n\n2:Music/Jazz\n\n" + err := os.WriteFile(filePath, []byte(content), 0600) + Expect(err).ToNot(HaveOccurred()) + + targets, err := readTargetsFromFile(filePath) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(HaveLen(2)) + }) + + It("trims whitespace", func() { + filePath := filepath.Join(tempDir, "targets.txt") + content := " 1:Music/Rock \n\t2:Music/Jazz\t\n" + err := os.WriteFile(filePath, []byte(content), 0600) + Expect(err).ToNot(HaveOccurred()) + + targets, err := readTargetsFromFile(filePath) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(HaveLen(2)) + Expect(targets[0].FolderPath).To(Equal("Music/Rock")) + Expect(targets[1].FolderPath).To(Equal("Music/Jazz")) + }) + + It("returns error for non-existent file", func() { + _, err := readTargetsFromFile("/nonexistent/file.txt") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to open target file")) + }) + + It("returns error for invalid target format", func() { + filePath := filepath.Join(tempDir, "targets.txt") + content := "invalid-format\n" + err := os.WriteFile(filePath, []byte(content), 0600) + Expect(err).ToNot(HaveOccurred()) + + _, err = readTargetsFromFile(filePath) + Expect(err).To(HaveOccurred()) + }) + + It("handles mixed valid and empty lines", func() { + filePath := filepath.Join(tempDir, "targets.txt") + content := "\n1:Music/Rock\n\n\n2:Music/Jazz\n\n" + err := os.WriteFile(filePath, []byte(content), 0600) + Expect(err).ToNot(HaveOccurred()) + + targets, err := readTargetsFromFile(filePath) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(HaveLen(2)) + }) +}) diff --git a/scanner/external.go b/scanner/external.go index f5a117e48..75ee2bead 100644 --- a/scanner/external.go +++ b/scanner/external.go @@ -14,6 +14,12 @@ import ( "github.com/navidrome/navidrome/model" ) +const ( + // argLengthThreshold is the threshold for switching from command-line args to file-based target passing. + // Set conservatively at 24KB to support Windows (~32KB limit) with margin for env vars. + argLengthThreshold = 24 * 1024 +) + // scannerExternal is a scanner that runs an external process to do the scanning. It is used to avoid // memory leaks or retention in the main process, as the scanner can consume a lot of memory. The // external process will be spawned with the same executable as the current process, and will run @@ -45,10 +51,14 @@ func (s *scannerExternal) scan(ctx context.Context, fullScan bool, targets []mod // Add targets if provided if len(targets) > 0 { - for _, target := range targets { - args = append(args, "-t", target.String()) + targetArgs, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + if err != nil { + progress <- &ProgressInfo{Error: err.Error()} + return } - log.Debug(ctx, "Spawning external scanner process with targets", "fullScan", fullScan, "path", exe, "targets", targets) + defer cleanup() + log.Debug(ctx, "Spawning external scanner process with target file", "fullScan", fullScan, "path", exe, "numTargets", len(targets)) + args = append(args, targetArgs...) } else { log.Debug(ctx, "Spawning external scanner process", "fullScan", fullScan, "path", exe) } @@ -98,4 +108,62 @@ func (s *scannerExternal) wait(cmd *exec.Cmd, out *io.PipeWriter) { _ = out.Close() } +// targetArguments builds command-line arguments for the given scan targets. +// If the estimated argument length exceeds a threshold, it writes the targets to a temp file +// and returns the --target-file argument instead. +// Returns the arguments, a cleanup function to remove any temp file created, and an error if any. +func targetArguments(ctx context.Context, targets []model.ScanTarget, lengthThreshold int) ([]string, func(), error) { + var args []string + + // Estimate argument length to decide whether to use file-based approach + argLength := estimateArgLength(targets) + + if argLength > lengthThreshold { + // Write targets to temp file and pass via --target-file + targetFile, err := writeTargetsToFile(targets) + if err != nil { + return nil, nil, fmt.Errorf("failed to write targets to file: %w", err) + } + args = append(args, "--target-file", targetFile) + return args, func() { + os.Remove(targetFile) // Clean up temp file + }, nil + } + + // Use command-line arguments for small target lists + for _, target := range targets { + args = append(args, "-t", target.String()) + } + return args, func() {}, nil +} + +// estimateArgLength estimates the total length of command-line arguments for the given targets. +func estimateArgLength(targets []model.ScanTarget) int { + length := 0 + for _, target := range targets { + // Each target adds: "-t " + target string + space + length += 3 + len(target.String()) + 1 + } + return length +} + +// writeTargetsToFile writes the targets to a temporary file, one per line. +// Returns the path to the temp file, which the caller should clean up. +func writeTargetsToFile(targets []model.ScanTarget) (string, error) { + tmpFile, err := os.CreateTemp("", "navidrome-scan-targets-*.txt") + if err != nil { + return "", fmt.Errorf("failed to create temp file: %w", err) + } + defer tmpFile.Close() + + for _, target := range targets { + if _, err := fmt.Fprintln(tmpFile, target.String()); err != nil { + os.Remove(tmpFile.Name()) + return "", fmt.Errorf("failed to write to temp file: %w", err) + } + } + + return tmpFile.Name(), nil +} + var _ scanner = (*scannerExternal)(nil) diff --git a/scanner/external_test.go b/scanner/external_test.go new file mode 100644 index 000000000..55f103f4d --- /dev/null +++ b/scanner/external_test.go @@ -0,0 +1,160 @@ +package scanner + +import ( + "context" + "os" + "strings" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("targetArguments", func() { + var ctx context.Context + + BeforeEach(func() { + ctx = GinkgoT().Context() + }) + + Context("with small target list", func() { + It("returns command-line arguments for single target", func() { + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music/Rock"}, + } + + args, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(Equal([]string{"-t", "1:Music/Rock"})) + }) + + It("returns command-line arguments for multiple targets", func() { + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music/Rock"}, + {LibraryID: 2, FolderPath: "Music/Jazz"}, + {LibraryID: 3, FolderPath: "Classical"}, + } + + args, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(Equal([]string{ + "-t", "1:Music/Rock", + "-t", "2:Music/Jazz", + "-t", "3:Classical", + })) + }) + + It("handles targets with special characters", func() { + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music/Rock & Roll"}, + {LibraryID: 2, FolderPath: "Music/Jazz (Modern)"}, + } + + args, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(Equal([]string{ + "-t", "1:Music/Rock & Roll", + "-t", "2:Music/Jazz (Modern)", + })) + }) + }) + + Context("with large target list exceeding threshold", func() { + It("returns --target-file argument when exceeding threshold", func() { + // Create enough targets to exceed the threshold + var targets []model.ScanTarget + for i := 1; i <= 600; i++ { + targets = append(targets, model.ScanTarget{ + LibraryID: 1, + FolderPath: "Music/VeryLongFolderPathToSimulateRealScenario/SubFolder", + }) + } + + args, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(HaveLen(2)) + Expect(args[0]).To(Equal("--target-file")) + + // Verify the file exists and has correct format + filePath := args[1] + Expect(filePath).To(ContainSubstring("navidrome-scan-targets-")) + Expect(filePath).To(HaveSuffix(".txt")) + + // Verify file actually exists + _, err = os.Stat(filePath) + Expect(err).ToNot(HaveOccurred()) + }) + + It("creates temp file with correct format", func() { + // Use custom threshold to easily exceed it + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music/Rock"}, + {LibraryID: 2, FolderPath: "Music/Jazz"}, + {LibraryID: 3, FolderPath: "Classical"}, + } + + // Set threshold very low to force file usage + args, cleanup, err := targetArguments(ctx, targets, 10) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args[0]).To(Equal("--target-file")) + + // Verify file exists with correct format + filePath := args[1] + Expect(filePath).To(ContainSubstring("navidrome-scan-targets-")) + Expect(filePath).To(HaveSuffix(".txt")) + + // Verify file content + content, err := os.ReadFile(filePath) + Expect(err).ToNot(HaveOccurred()) + lines := strings.Split(strings.TrimSpace(string(content)), "\n") + Expect(lines).To(HaveLen(3)) + Expect(lines[0]).To(Equal("1:Music/Rock")) + Expect(lines[1]).To(Equal("2:Music/Jazz")) + Expect(lines[2]).To(Equal("3:Classical")) + }) + }) + + Context("edge cases", func() { + It("handles empty target list", func() { + var targets []model.ScanTarget + + args, cleanup, err := targetArguments(ctx, targets, argLengthThreshold) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(BeEmpty()) + }) + + It("uses command-line args when exactly at threshold", func() { + // Create targets that are exactly at threshold + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music"}, + } + + // Estimate length should be 11 bytes + estimatedLength := estimateArgLength(targets) + + args, cleanup, err := targetArguments(ctx, targets, estimatedLength) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args).To(Equal([]string{"-t", "1:Music"})) + }) + + It("uses file when one byte over threshold", func() { + targets := []model.ScanTarget{ + {LibraryID: 1, FolderPath: "Music"}, + } + + // Set threshold just below the estimated length + estimatedLength := estimateArgLength(targets) + args, cleanup, err := targetArguments(ctx, targets, estimatedLength-1) + Expect(err).ToNot(HaveOccurred()) + defer cleanup() + Expect(args[0]).To(Equal("--target-file")) + }) + }) +})