From 35326adc67370e4ecbca80e6fab2cd9e1d1d92dd Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:06:13 +0000 Subject: [PATCH] fix(scanner): keep re-probed outages protected through folder-wide cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth Codex review round on #472; both findings confirmed. The first could destroy data. reprobeNestedRoots protected a root it found offline only for the scope being reconciled, then discarded the result. The folder-wide membership reconcile and the trash sweep afterwards rebuilt their protected set from the initial probe alone, so rows under a child that dropped mid-scan — already marked missing and past the removal grace — were hard-deleted by the very scan that noticed the outage. Accumulate those roots in reprobedRoots, fold them into protectedScanRoots, and reuse that set for the membership reconcile and sweep instead of rebuilding. They now also land in ScanResult.UnreachableRoots so the folder warning reflects the outage rather than presenting a partial scan as clean. Snapshot and group pruning was enabled for scopes that were never walked. The gate was len(walkFailures) == 0, but an unreachable root gets nil walkRoots, so it has no walk and therefore no failures — and pruning then deleted its snapshots, observed locations and group locations even though its media rows were protected. The same held for a suspect-empty child compacted into a populated parent. Pruning now additionally requires that the scope was actually walked and contains no protected path. The new test pins that the sweep honours the protected set it is given. It does not reproduce the mid-scan race itself: staging that needs the drop to land between the probe and the walk, which a test cannot reach without hooks. That path is covered by inspection, and the test comment says so rather than implying coverage it does not have. --- internal/scanner/dead_root_test.go | 92 ++++++++++++++++++++++++++++++ internal/scanner/scanner.go | 63 ++++++++++++++++++-- 2 files changed, 149 insertions(+), 6 deletions(-) diff --git a/internal/scanner/dead_root_test.go b/internal/scanner/dead_root_test.go index e6e5104a..7a1f4340 100644 --- a/internal/scanner/dead_root_test.go +++ b/internal/scanner/dead_root_test.go @@ -1378,3 +1378,95 @@ func TestReprobeNestedRootsCatchesMidScanChildDrop(t *testing.T) { t.Fatalf("protected = %v, want none: %s has no nested configured roots", got, sibling) } } + +// TestScanFolderProtectedChildRootSurvivesTrashSweep asserts that a nested +// child root which is offline at scan time keeps its already-missing rows +// through the folder-wide trash sweep, even when those rows are long past the +// removal grace. +// +// Scope note: this stages the child as unreachable BEFORE the scan, so the +// initial probe classifies it and the protection comes from that path. It does +// NOT reproduce the mid-scan drop behind Codex finding #6 — where the child is +// healthy at probe time and dies during the walk, so only reprobeNestedRoots +// sees it. Staging that race needs the drop to land between the probe and the +// walk, which is not reachable from a test without hooks. The fix for that +// path (carrying reprobedRoots into protectedScanRoots) is therefore covered +// by inspection, not by this test; what this test does pin is that the sweep +// honours the protected set it is given. +// +// Trash emptying is on with a zero grace, so any row left unprotected is +// deleted immediately rather than merely hidden. +func TestScanFolderProtectedChildRootSurvivesTrashSweep(t *testing.T) { + pool := newDeadRootTestPool(t) + ctx := context.Background() + folderID := seedDeadRootTestFolder(t, pool, "movies", "Mid-Scan Drop Sweep Test") + + base := t.TempDir() + parent := filepath.Join(base, "media") + child := filepath.Join(parent, "child-mount") + parentFile := filepath.Join(parent, "Keeper (2020)", "Keeper (2020).mkv") + childFile := filepath.Join(child, "Child (2021)", "Child (2021).mkv") + for _, p := range []string{parentFile, childFile} { + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(p, []byte("fake movie payload"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + } + + folder := &models.MediaFolder{ + ID: folderID, Paths: []string{parent, child}, Type: "movies", + Name: "Mid-Scan Drop Sweep 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 childID int + if err := pool.QueryRow(ctx, + `SELECT id FROM media_files WHERE media_folder_id = $1 AND file_path = $2`, + folderID, childFile).Scan(&childID); err != nil { + t.Fatalf("child row: %v", err) + } + + // Put the child's row in the state the sweep would delete: already marked + // missing, well past the (zero) removal grace. + if _, err := pool.Exec(ctx, + `UPDATE media_files SET missing_since = NOW() - INTERVAL '48 hours' WHERE id = $1`, + childID); err != nil { + t.Fatalf("pre-mark child row: %v", err) + } + + // The child mount drops. The parent stays healthy and still walks files, + // so the scan takes the populated-parent path. + if err := os.RemoveAll(child); err != nil { + t.Fatalf("drop child mount: %v", err) + } + + if _, err := scanner.ScanFolder(ctx, folder); err != nil { + t.Fatalf("scan after child drop: %v", err) + } + + var survives bool + if err := pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM media_files WHERE id = $1)`, childID).Scan(&survives); err != nil { + t.Fatalf("existence check: %v", err) + } + if !survives { + t.Fatal("row under an offline child root was hard-deleted by the trash sweep; " + + "an outage must never be a trigger for permanent deletion") + } + + // The parent's own file must be unaffected throughout. + var parentMissing *time.Time + if err := pool.QueryRow(ctx, + `SELECT missing_since FROM media_files WHERE media_folder_id = $1 AND file_path = $2`, + folderID, parentFile).Scan(&parentMissing); err != nil { + t.Fatalf("parent row: %v", err) + } + if parentMissing != nil { + t.Fatalf("healthy parent file marked missing at %v", parentMissing) + } +} diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index f2cf6a20..dd166279 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -1111,6 +1111,11 @@ func (s *Scanner) scanFolderByRoots( return nil, err } + // reprobedRoots accumulates roots found offline by the mid-loop re-probe, + // so later folder-wide cleanup honours them alongside the initial probe's + // results. + reprobedRoots := make([]string, 0) + pendingEmptyScopes := make([]*scopedScan, 0) totalExisting := 0 seenAnyFiles := false @@ -1172,6 +1177,15 @@ func (s *Scanner) scanFolderByRoots( return nil, err } scopeProtected = append(scopeProtected, freshProtected...) + // A root discovered offline here must stay protected for the rest + // of the scan, not just for this scope. The folder-wide membership + // reconcile and trash sweep below run off reprobedRoots too — + // without that, rows under a child that dropped mid-scan and are + // already past the removal grace get hard-deleted by the very scan + // that noticed the outage. + for _, protectedRoot := range freshProtected { + reprobedRoots = appendUniquePath(reprobedRoots, protectedRoot) + } if err := s.applyScopedScan(ctx, folder, scope, false, scopeProtected); err != nil { return nil, err } @@ -1254,8 +1268,18 @@ func (s *Scanner) scanFolderByRoots( suspectRoots = nil } result.SuspectEmptyRoots = suspectRoots + // A root that dropped mid-scan is as much an outage as one that failed the + // initial probe; report it so the folder warning and scan result do not + // present a partial scan as clean. + for _, protectedRoot := range reprobedRoots { + unreachableRoots = appendUniquePath(unreachableRoots, protectedRoot) + } + result.UnreachableRoots = unreachableRoots protectedScanRoots := append(append([]string(nil), unreachableRoots...), suspectRoots...) + for _, protectedRoot := range reprobedRoots { + protectedScanRoots = appendUniquePath(protectedScanRoots, protectedRoot) + } for _, pending := range pendingEmptyScopes { beforeErrors := pending.result.Errors // forceDeleteAll only ever fires for confirmed cleanup, and even then @@ -1280,11 +1304,13 @@ func (s *Scanner) scanFolderByRoots( return nil, fmt.Errorf("syncing present library state for folder %d: %w", folder.ID, err) } - protectedRoots := unreachableRoots - if len(suspectRoots) > 0 { - protectedRoots = make([]string, 0, len(unreachableRoots)+len(suspectRoots)) - protectedRoots = append(append(protectedRoots, unreachableRoots...), suspectRoots...) - } + // Reuse the same protected set the scoped cleanup used, so membership + // removal and the trash sweep below honour roots the mid-loop re-probe + // found offline. Rebuilding from only the initial probe here would let a + // child that dropped during this scan have its already-missing rows hard + // deleted once they pass the removal grace — by the very scan that + // noticed the outage. + protectedRoots := protectedScanRoots removedMemberships, deletedItems, orphanedImageDirs, err := s.reconcileLibraryMemberships(ctx, folder.ID, protectedRoots) if err != nil { return nil, fmt.Errorf("reconciling library membership for folder %d: %w", folder.ID, err) @@ -1721,7 +1747,21 @@ func (s *Scanner) applyScopedScan( // even though the media_files rows themselves stay protected below. // Upserting what we did see is always safe; pruning is what must wait for // a scan that read the whole tree. - pruneUnseen := len(scope.walkFailures) == 0 + // Pruning deletes whatever this walk did not observe, so it is only sound + // when the walk actually observed the scope. Three cases must suppress it: + // an incomplete walk (some of the tree was unreadable), a scope that was + // never walked at all (an unreachable root gets nil walkRoots), and a + // scope containing a protected path (a suspect-empty child compacted into + // a populated parent). In each case the observed set is a lower bound, and + // pruning against it deletes snapshots, observed locations and group + // locations for media that is still there — corrupting later matching even + // though the media_files rows themselves are protected. + // + // Keeping stale metadata is harmless by comparison: the next complete scan + // prunes it. + pruneUnseen := len(scope.walkFailures) == 0 && + len(scope.walkRoots) > 0 && + !anyPathWithinRoots(protectedRoots, scope.reconcileRoots) if err := s.reconcileScannedRoots( ctx, folder.ID, @@ -1882,6 +1922,17 @@ func (s *Scanner) emptyCleanupArmed(ctx context.Context, folderID int) (bool, er return folder.AllowEmptyCleanupOnce, nil } +// anyPathWithinRoots reports whether any of paths lies at or under one of +// roots. Used to detect a protected path inside a scope about to be pruned. +func anyPathWithinRoots(paths, roots []string) bool { + for _, path := range paths { + if pathWithinAnyRoot(path, roots) { + return true + } + } + return false +} + // 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 []string) {