diff --git a/internal/scanner/dead_root_test.go b/internal/scanner/dead_root_test.go index 0eec5284..2694fba7 100644 --- a/internal/scanner/dead_root_test.go +++ b/internal/scanner/dead_root_test.go @@ -520,8 +520,17 @@ func TestScanFolderNestedSuspectEmptyChildRootProtection(t *testing.T) { ).Scan(&gotID, &missing); err != nil { t.Fatalf("child row after scan 2 (was it deleted?): %v", err) } - if gotID != childID || missing == nil { - t.Fatalf("child row id/missing = %d/%v, want %d/non-nil", gotID, missing, childID) + // The child mountpoint survived but its contents vanished with the mount, + // which is indistinguishable from an intentional emptying. The safe + // reading is to leave the rows visible: hiding them on a guess turns a + // dropped mount into a library outage, whereas an intentional emptying is + // confirmed explicitly through the cleanup allowance. + if gotID != childID { + t.Fatalf("child row id = %d, want %d", gotID, childID) + } + if missing != nil { + t.Fatalf("child row marked missing at %v; a suspect-empty child mount must not be "+ + "hidden just because its parent root scanned cleanly", missing) } } @@ -1099,3 +1108,127 @@ func TestScanFolderFlappingRootNeverHidesPresentFiles(t *testing.T) { t.Fatal("a genuinely deleted file under a reachable root survived; real deletions must still be detected") } } + +// TestScanFolderFirstScanAfterMountDropsProtectsLiveRows covers Codex review +// finding #2 on PR #472: suspect-empty detection used to require a root whose +// rows were ALL already missing, which meant it could only recognise a lost +// mount one scan too late. +// +// Here the mount drops leaving a reachable but empty mountpoint, and the rows +// are still live because nothing has marked them yet — the state on the very +// first scan after a real mount failure. The root must be classified suspect +// and its rows protected on that first scan, not after they have been hidden. +func TestScanFolderFirstScanAfterMountDropsProtectsLiveRows(t *testing.T) { + pool := newDeadRootTestPool(t) + ctx := context.Background() + folderID := seedDeadRootTestFolder(t, pool, "movies", "First Outage Scan Test") + + base := t.TempDir() + live := filepath.Join(base, "live") + dropped := filepath.Join(base, "dropped") + liveFile := filepath.Join(live, "Alpha (2020)", "Alpha (2020).mkv") + droppedFile := filepath.Join(dropped, "Beta (2021)", "Beta (2021).mkv") + + write := func(path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, []byte("fake movie payload"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + } + write(liveFile) + write(droppedFile) + + folder := &models.MediaFolder{ + ID: folderID, Paths: []string{live, dropped}, Type: "movies", + Name: "First Outage Scan Test", Enabled: true, + } + scanner := NewScanner(NewFileRepository(pool), "", nil, 2, true, 0) + + if _, err := scanner.ScanFolder(ctx, folder); err != nil { + t.Fatalf("baseline scan: %v", err) + } + var missing *time.Time + if err := pool.QueryRow(ctx, + `SELECT missing_since FROM media_files WHERE media_folder_id = $1 AND file_path = $2`, + folderID, droppedFile).Scan(&missing); err != nil { + t.Fatalf("baseline row: %v", err) + } + if missing != nil { + t.Fatalf("baseline: row already missing at %v", missing) + } + + // The mount drops: contents vanish, the mountpoint directory remains and + // still probes reachable. The rows under it are all still live. + if err := os.RemoveAll(filepath.Join(dropped, "Beta (2021)")); err != nil { + t.Fatalf("empty the dropped root: %v", err) + } + + result, err := scanner.ScanFolder(ctx, folder) + if err != nil { + t.Fatalf("first outage scan: %v", err) + } + if len(result.SuspectEmptyRoots) != 1 || result.SuspectEmptyRoots[0] != dropped { + t.Fatalf("SuspectEmptyRoots = %v, want [%s] on the FIRST scan after the drop", + result.SuspectEmptyRoots, dropped) + } + if err := pool.QueryRow(ctx, + `SELECT missing_since FROM media_files WHERE media_folder_id = $1 AND file_path = $2`, + folderID, droppedFile).Scan(&missing); err != nil { + t.Fatalf("row after outage scan (hard-deleted?): %v", err) + } + if missing != nil { + t.Fatalf("first scan after the mount dropped hid the row at %v; suspect-empty "+ + "protection must engage before the rows are marked, not after", missing) + } +} + +// TestCollectLogicalFilePathsReportsUnreadableEntries covers Codex review +// finding #3 on PR #472: the video walk swallowed per-entry read failures and +// reported no signal, so a mount dying partway through traversal produced a +// short file list indistinguishable from a large deletion. +func TestCollectLogicalFilePathsReportsUnreadableEntries(t *testing.T) { + t.Parallel() + + if os.Geteuid() == 0 { + t.Skip("running as root: permission bits do not deny access") + } + + root := t.TempDir() + readable := filepath.Join(root, "Alpha (2020)") + if err := os.MkdirAll(readable, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(readable, "Alpha (2020).mkv"), []byte("x"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + // A subtree the walk cannot read stands in for the portion of a tree that + // becomes unreachable when a mount dies mid-traversal. + blocked := filepath.Join(root, "Beta (2021)") + if err := os.MkdirAll(blocked, 0o755); err != nil { + t.Fatalf("mkdir blocked: %v", err) + } + if err := os.WriteFile(filepath.Join(blocked, "Beta (2021).mkv"), []byte("x"), 0o644); err != nil { + t.Fatalf("write blocked: %v", err) + } + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(blocked, 0o755) }) + + files, walkFailures, err := collectLogicalFilePaths(context.Background(), []string{root}, "movies") + if err != nil { + t.Fatalf("collectLogicalFilePaths: %v", err) + } + if walkFailures == 0 { + t.Fatal("walk reported 0 failures despite an unreadable subtree; a partial listing " + + "would then be treated as an authoritative inventory") + } + // The readable file is still found — one bad subtree must not abort the walk. + if len(files) != 1 { + t.Fatalf("files = %v, want just the readable one", files) + } +} diff --git a/internal/scanner/file_repo.go b/internal/scanner/file_repo.go index cc46f0ab..264e05ec 100644 --- a/internal/scanner/file_repo.go +++ b/internal/scanner/file_repo.go @@ -2682,6 +2682,58 @@ func (r *FileRepository) DeleteMissingByFolder(ctx context.Context, folderID int return int(tag.RowsAffected()), nil } +// ListRootsWithCatalogedFiles returns the subset of roots (in input order) +// that still have any media_files rows at or under them in the folder, +// whether those rows are present or already marked missing. +// +// This is the proactive counterpart to ListRootsWithOnlyMissingFiles. That +// query requires a root to have NO live rows left, which means it can only +// recognise a lost mount after a scan has already marked its files missing — +// i.e. after the damage is done. For deciding whether to mark in the first +// place, the question is simply "does the catalog believe anything lives +// here", because an empty-but-reachable directory that still owns cataloged +// files is the signature of a dropped mount exposing its bare mountpoint. +// +// A genuinely emptied root also matches, which is intended: emptying a root +// is confirmed through the operator's one-time cleanup allowance rather than +// inferred from a single scan. +func (r *FileRepository) ListRootsWithCatalogedFiles(ctx context.Context, folderID int, roots []string) ([]string, error) { + if len(roots) == 0 { + return nil, nil + } + patterns := make([]string, len(roots)) + for i, root := range roots { + patterns[i] = pathscope.PrefixLike(root) + } + rows, err := r.pool.Query(ctx, ` + SELECT r.root + FROM unnest($2::text[], $3::text[]) WITH ORDINALITY AS r(root, pattern, ord) + WHERE EXISTS ( + SELECT 1 FROM media_files mf + WHERE mf.media_folder_id = $1 + AND (mf.file_path = r.root OR mf.file_path LIKE r.pattern ESCAPE '\') + ) + ORDER BY r.ord + `, folderID, roots, patterns) + if err != nil { + return nil, fmt.Errorf("querying roots with cataloged files: %w", err) + } + defer rows.Close() + + occupied := make([]string, 0) + for rows.Next() { + var root string + if err := rows.Scan(&root); err != nil { + return nil, fmt.Errorf("scanning root with cataloged files: %w", err) + } + occupied = append(occupied, root) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("iterating roots with cataloged files: %w", err) + } + return occupied, nil +} + // ListRootsWithOnlyMissingFiles returns the subset of roots (in input order) // that still have media_files rows at or under them in the folder but none // that are present (missing_since IS NULL). A reachable root in this state is diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 26bfca4c..7ab31251 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -621,27 +621,38 @@ func walkLogicalTree( return nil } -func collectLogicalFilePaths(ctx context.Context, walkRoots []string, libraryType string) ([]string, error) { +// collectLogicalFilePaths walks the given roots and returns the media files +// found, plus how many entries the walk could not read or resolve. +// +// walkLogicalTree deliberately swallows per-entry Lstat/ReadDir failures so one +// bad file cannot abort a scan of a million. That makes the returned list +// non-authoritative when failures occurred: a mount that dies partway through +// traversal yields a short list that looks exactly like a large deletion. +// Callers must treat a non-zero failure count as "this scope's picture is +// incomplete" and skip missing reconciliation for it, the same way the ebook +// scanner does with ebookRootScan.failed. +func collectLogicalFilePaths(ctx context.Context, walkRoots []string, libraryType string) ([]string, int, error) { filePaths := make([]string, 0) visitedPhysicalDirs := make(map[string]struct{}) mode := walkModeFor(libraryType) + walkFailures := 0 for _, rootPath := range walkRoots { if ctx != nil { if err := ctx.Err(); err != nil { - return nil, err + return nil, 0, err } } cleanRoot := filepath.Clean(rootPath) if cleanRoot == "" || cleanRoot == "." { continue } - if err := walkLogicalTree(ctx, cleanRoot, cleanRoot, mode, visitedPhysicalDirs, &filePaths, nil); err != nil { - return nil, err + if err := walkLogicalTree(ctx, cleanRoot, cleanRoot, mode, visitedPhysicalDirs, &filePaths, &walkFailures); err != nil { + return nil, 0, err } } - return filePaths, nil + return filePaths, walkFailures, nil } func (s *Scanner) scanPaths( @@ -693,7 +704,7 @@ func (s *Scanner) scanPaths( Message: "Discovering media files", CurrentScope: firstScope(reconcileRoots), }) - filePaths, walkErr := collectLogicalFilePaths(ctx, walkRoots, folder.Type) + filePaths, walkFailures, walkErr := collectLogicalFilePaths(ctx, walkRoots, folder.Type) if walkErr != nil { return nil, fmt.Errorf("walking media roots: %w", walkErr) } @@ -702,6 +713,9 @@ func (s *Scanner) scanPaths( "scope", firstScope(reconcileRoots), "files", len(filePaths), ) + if walkFailures > 0 { + logIncompleteWalk(ctx, folder.ID, reconcileRoots, walkFailures) + } reportProgress(ctx, ProgressUpdate{ Phase: "processing", Message: "Processing discovered files", @@ -899,31 +913,14 @@ func (s *Scanner) scanPaths( if err != nil { return nil, err } - - // Mark files that were in the DB but not found on disk as missing. - now := time.Now().UTC() - for _, existing := range existingFiles { - if seenPaths[existing.FilePath] { - continue - } - if pathWithinAnyRoot(existing.FilePath, protectedRoots) { - result.MissingSkippedProtected++ - continue - } - // Only mark as missing if not already marked. - if existing.MissingSince == nil { - if err := s.fileRepo.MarkMissing(ctx, existing.ID, now); err != nil { - slog.ErrorContext(ctx, "scanner: failed to mark file missing", "component", "scanner", - "path", existing.FilePath, - "error", err, - ) - result.Errors++ - continue - } - } - result.Missing++ + // An incomplete walk makes this scope's file list a lower bound rather + // than an inventory; reconciling against it would hide whatever the walk + // failed to read. + if walkFailures > 0 { + protectedRoots = append(append([]string(nil), protectedRoots...), reconcileRoots...) } - logProtectedMissingSkips(ctx, folder.ID, result.MissingSkippedProtected, protectedRoots) + + s.markMissingExcludingProtected(ctx, folder.ID, existingFiles, seenPaths, protectedRoots, result) // Empty trash: delete files marked as missing for longer than the removal // grace for this folder. Safe because the empty-root guard (above) returns @@ -1028,6 +1025,10 @@ type scopedScan struct { rootInference rootInferenceResult groupInference groupInferenceResult result *ScanResult + // walkIncomplete is set when the walk could not read part of this scope's + // tree. The file list is then a lower bound, not an inventory, so the + // scope must be excluded from missing reconciliation. + walkIncomplete bool } func (s *Scanner) scanFolderByRoots( @@ -1044,12 +1045,12 @@ func (s *Scanner) scanFolderByRoots( roots := compactScanRoots(configuredRoots) // An unreachable root (dead drive, lost mount) is temporarily offline, not - // removed from the library: its files are still marked missing below so - // clients stop seeing them, but nothing under it may be hard-deleted while - // it stays unreachable — trash emptying and item purging are guarded - // against these roots further down. Probe every CONFIGURED path, not the - // compacted traversal roots: a nested child mount is dropped by compaction - // but can die independently of its reachable parent. + // removed from the library: its files are left untouched below — not + // marked missing, not trashed, not purged — for as long as it stays + // unreachable. Marking would be enough to hide them, since every catalog + // read filters on missing_since IS NULL. Probe every CONFIGURED path, not + // the compacted traversal roots: a nested child mount is dropped by + // compaction but can die independently of its reachable parent. configuredProbes := rootcheck.ProbeManyWithTimeout(ctx, configuredRoots, rootcheck.DefaultProbeTimeout) unreachableRoots := make([]string, 0) suspectRoots := make([]string, 0) @@ -1091,7 +1092,9 @@ func (s *Scanner) scanFolderByRoots( scopeWalkRoots := []string{root} if unreachableSet[root] { // Skip walking a dead root — it would yield nothing anyway. Its - // scope still reconciles below so existing rows are marked missing. + // scope still reconciles below (roots, groups, memberships), but + // its existing rows are protected from missing-marking rather + // than swept up by the empty walk. scopeWalkRoots = nil } scope, err := s.scanScope(ctx, folder, scopeWalkRoots, []string{root}) @@ -1108,11 +1111,19 @@ func (s *Scanner) scanFolderByRoots( if len(scope.filePaths) > 0 { seenAnyFiles = true beforeErrors := scope.result.Errors - // Pass the unreachable roots even though this scope walked files: - // a nested child mount can be dead under a live parent, and its - // rows fall inside this scope's existing files. Without them the - // child's catalog gets marked missing on the parent's success. - if err := s.applyScopedScan(ctx, folder, scope, false, unreachableRoots); err != nil { + // Pass both protected sets even though this scope walked files: a + // nested child mount compacted into this root can be dead, or can + // be a suspect-empty mountpoint, and either way its rows fall + // inside this scope's existing files. Without them the child's + // catalog gets marked missing on the parent's success. + // + // suspectRoots is read before the confirmed-cleanup allowance is + // consumed below, so a nested child is protected from marking + // here regardless. Deleting such a root's catalog stays an + // explicit operator action rather than a side effect of its + // parent scanning cleanly. + scopeProtected := append(append([]string(nil), unreachableRoots...), suspectRoots...) + if err := s.applyScopedScan(ctx, folder, scope, false, scopeProtected); err != nil { return nil, err } mergeCleanupResult(result, scope.result, beforeErrors) @@ -1379,7 +1390,13 @@ func (s *Scanner) suspectEmptyRoots(ctx context.Context, folderID int, configure if len(emptyRoots) == 0 { return nil, nil } - suspect, err := s.fileRepo.ListRootsWithOnlyMissingFiles(ctx, folderID, emptyRoots) + // Any cataloged row under an empty-but-reachable root makes it suspect, + // not just a root whose rows are already all missing. Requiring the + // latter made this protection reactive: on the first scan after a mount + // dropped, the rows are still live, the root is not classified suspect, + // and the scan marks everything missing — the exact outage this guards + // against, recognised only in time to protect the wreckage. + suspect, err := s.fileRepo.ListRootsWithCatalogedFiles(ctx, folderID, emptyRoots) if err != nil { return nil, fmt.Errorf("listing suspect-empty roots for folder %d: %w", folderID, err) } @@ -1462,7 +1479,7 @@ func (s *Scanner) scanScope( return nil, fmt.Errorf("loading item statuses for folder %d path %q: %w", folder.ID, reconcileRoots[0], err) } - filePaths, walkErr := collectLogicalFilePaths(ctx, walkRoots, folder.Type) + filePaths, walkFailures, walkErr := collectLogicalFilePaths(ctx, walkRoots, folder.Type) if walkErr != nil { return nil, fmt.Errorf("walking media roots for %q: %w", reconcileRoots[0], walkErr) } @@ -1471,6 +1488,9 @@ func (s *Scanner) scanScope( "scope", firstScope(reconcileRoots), "files", len(filePaths), ) + if walkFailures > 0 { + logIncompleteWalk(ctx, folder.ID, reconcileRoots, walkFailures) + } reportProgress(ctx, ProgressUpdate{ Phase: "processing", Message: "Processing discovered files", @@ -1622,6 +1642,7 @@ func (s *Scanner) scanScope( rootInference: rootInference, groupInference: groupInference, result: result, + walkIncomplete: walkFailures > 0, }, nil } @@ -1653,32 +1674,14 @@ func (s *Scanner) applyScopedScan( return fmt.Errorf("reconciling scanned groups for scope %q: %w", scope.reconcileRoots[0], err) } - now := time.Now().UTC() - for _, existing := range scope.existingFiles { - if scope.seenPaths[existing.FilePath] { - continue - } - // A file under an offline root was not found because the storage did - // not answer. Marking it missing hides it from every catalog read, so - // a transient mount fault would take working titles out of the library - // until the next successful scan. - if pathWithinAnyRoot(existing.FilePath, protectedRoots) { - scope.result.MissingSkippedProtected++ - continue - } - if existing.MissingSince == nil { - if err := s.fileRepo.MarkMissing(ctx, existing.ID, now); err != nil { - slog.ErrorContext(ctx, "scanner: failed to mark file missing", "component", "scanner", - "path", existing.FilePath, - "error", err, - ) - scope.result.Errors++ - continue - } - } - scope.result.Missing++ + // A scope whose walk could not read part of its tree saw an incomplete + // picture of what is on disk. Treating that as authoritative would mark + // everything below the unread portion missing, which is the transient + // storage failure this protection exists for — so protect the whole scope. + if scope.walkIncomplete { + protectedRoots = append(append([]string(nil), protectedRoots...), scope.reconcileRoots...) } - logProtectedMissingSkips(ctx, folder.ID, scope.result.MissingSkippedProtected, protectedRoots) + s.markMissingExcludingProtected(ctx, folder.ID, scope.existingFiles, scope.seenPaths, protectedRoots, scope.result) staleFileIDs := collectStaleRemovedPathFileIDs(scope.existingFiles, scope.seenPaths, scope.reconcileRoots) if forceDeleteAll && len(scope.filePaths) == 0 { @@ -1712,6 +1715,60 @@ func mergeScanResult(dst *ScanResult, src *ScanResult) { dst.EmptyRootGuarded = dst.EmptyRootGuarded || src.EmptyRootGuarded } +// markMissingExcludingProtected marks every cataloged file the walk did not +// see as missing, except those under a protected root. +// +// Protection is the whole point: catalog reads filter on +// missing_since IS NULL, so marking a file is user-visible deletion. A root +// that is unreachable, is a suspect-empty mountpoint, or whose walk came back +// incomplete tells us nothing about whether its files exist, and hiding them +// on that basis turns a storage blip into a library outage. +// +// Shared by the folder scan and the scoped scan so the two cannot drift. +func (s *Scanner) markMissingExcludingProtected( + ctx context.Context, + folderID int, + existingFiles []*scanStateFile, + seenPaths map[string]bool, + protectedRoots []string, + result *ScanResult, +) { + now := time.Now().UTC() + for _, existing := range existingFiles { + if seenPaths[existing.FilePath] { + continue + } + if pathWithinAnyRoot(existing.FilePath, protectedRoots) { + result.MissingSkippedProtected++ + continue + } + // Only mark as missing if not already marked. + if existing.MissingSince == nil { + if err := s.fileRepo.MarkMissing(ctx, existing.ID, now); err != nil { + slog.ErrorContext(ctx, "scanner: failed to mark file missing", "component", "scanner", + "path", existing.FilePath, + "error", err, + ) + result.Errors++ + continue + } + } + result.Missing++ + } + logProtectedMissingSkips(ctx, folderID, result.MissingSkippedProtected, protectedRoots) +} + +// logIncompleteWalk reports that a scope's traversal could not read part of +// its tree, so its file list is a lower bound rather than an inventory. +func logIncompleteWalk(ctx context.Context, folderID int, reconcileRoots []string, walkFailures int) { + slog.WarnContext(ctx, "scanner: root walk incomplete; scope excluded from missing-file reconciliation", + "component", "scanner", + "folder_id", folderID, + "scope", firstScope(reconcileRoots), + "walk_failures", walkFailures, + ) +} + // logProtectedMissingSkips reports files a scan declined to mark missing // because their root was offline. This is the signal an operator needs to tell // "my library shrank" from "my mount dropped": without it the scan looks diff --git a/internal/scanner/scanner_test.go b/internal/scanner/scanner_test.go index 745ae29f..0d08d0f7 100644 --- a/internal/scanner/scanner_test.go +++ b/internal/scanner/scanner_test.go @@ -46,10 +46,13 @@ func TestCollectLogicalFilePaths_PreservesLogicalSymlinkRootPaths(t *testing.T) t.Skipf("symlinks not supported on this platform: %v", err) } - files, err := collectLogicalFilePaths(context.Background(), []string{logicalRoot}, "series") + files, walkFailures, err := collectLogicalFilePaths(context.Background(), []string{logicalRoot}, "series") if err != nil { t.Fatalf("collect logical paths: %v", err) } + if walkFailures != 0 { + t.Fatalf("walkFailures = %d, want 0 for a fully readable tree", walkFailures) + } want := filepath.Join(logicalRoot, "Season 1", "Episode 01.mkv") if len(files) != 1 { @@ -82,10 +85,13 @@ func TestCollectLogicalFilePaths_DedupesSharedPhysicalDirsAndCycles(t *testing.T t.Skipf("symlinks not supported on this platform: %v", err) } - files, err := collectLogicalFilePaths(context.Background(), []string{physicalRoot, aliasRoot}, "movie") + files, walkFailures, err := collectLogicalFilePaths(context.Background(), []string{physicalRoot, aliasRoot}, "movie") if err != nil { t.Fatalf("collect logical paths: %v", err) } + if walkFailures != 0 { + t.Fatalf("walkFailures = %d, want 0 for a fully readable tree", walkFailures) + } sort.Strings(files) want := []string{filepath.Join(physicalRoot, "Movie.mkv")}