diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b791882..8690f31 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -279,8 +279,8 @@ Key methods: - `Scan(ctx, path, snapshotID)` → Main scan operation Scan phases: -1. **Phase 0**: Detect deleted files from previous snapshots -2. **Phase 1**: Walk directory, identify files needing processing +1. **Phase 1**: Walk directory, identify files needing processing +2. **Phase 1b**: Count as deleted the files that the previous snapshot of the same name lists and the walk did not find; a file that is excluded, or inside an excluded directory, is not counted 3. **Phase 2**: Process files (chunk → pack → upload) #### SnapshotManager @@ -320,13 +320,13 @@ CreateSnapshot(opts) │ │ │ ├─► scanner.Scan(ctx, path, snapshotID) │ │ │ - │ │ ├─► Phase 0: detectDeletedFiles() - │ │ │ │ │ ├─► Phase 1: scanPhase() │ │ │ Walk directory │ │ │ Check file metadata changes │ │ │ Build list of files to process │ │ │ + │ │ ├─► Phase 1b: detectDeletedFiles() + │ │ │ │ │ └─► Phase 2: processPhase() │ │ For each file: │ │ chunker.ChunkReaderStreaming() diff --git a/internal/database/files.go b/internal/database/files.go index bce0a3b..9d64190 100644 --- a/internal/database/files.go +++ b/internal/database/files.go @@ -277,6 +277,50 @@ func (r *FileRepository) ListUnderPath( return files, rows.Err() } +// ListUnderPathInSnapshot returns the files that ListUnderPath returns +// for path and that the snapshot lists, in no particular order. +func (r *FileRepository) ListUnderPathInSnapshot( + ctx context.Context, path, snapshotID string, +) ([]*File, error) { + path = strings.TrimRight(path, "/") + dirPrefix := path + "/" + + query := ` + SELECT f.id, f.path, f.source_path, f.mtime, f.mtime_nsec, f.size, + f.mode, f.uid, f.gid, f.link_target + FROM files f + JOIN snapshot_files sf ON sf.file_id = f.id + WHERE sf.snapshot_id = ? + AND (f.path = ? OR substr(f.path, 1, length(?)) = ?) + ` + + rows, err := r.db.conn.QueryContext(ctx, query, + snapshotID, path, dirPrefix, dirPrefix) + if err != nil { + return nil, fmt.Errorf("querying files: %w", err) + } + + defer func() { + err := rows.Close() + if err != nil { + Fatalf("failed to close rows: %v", err) + } + }() + + var files []*File + + for rows.Next() { + file, err := r.scanFileRows(rows) + if err != nil { + return nil, fmt.Errorf("scanning file: %w", err) + } + + files = append(files, file) + } + + return files, rows.Err() +} + // ListIDsWithChunksNotInUploadedBlobs returns the IDs of the files whose // path starts with prefix and that list at least one chunk held by no // blob whose upload has completed (uploaded_ts set). A new snapshot diff --git a/internal/database/files_test.go b/internal/database/files_test.go index 8153766..343c482 100644 --- a/internal/database/files_test.go +++ b/internal/database/files_test.go @@ -212,6 +212,65 @@ func TestFileRepositoryListUnderPath(t *testing.T) { } } +// ListUnderPathInSnapshot leaves out files outside path and files the +// snapshot does not list, though another snapshot lists them. +func TestFileRepositoryListUnderPathInSnapshot(t *testing.T) { + t.Parallel() + + db, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + files := database.NewFileRepository(db) + snapshots := database.NewSnapshotRepository(db) + + listed := map[string][]string{ + "one": {"/src/a.txt", "/src2/c.txt"}, + "two": {"/src/b.txt"}, + } + + for snapshotID, paths := range listed { + err := snapshots.Create(ctx, nil, &database.Snapshot{ + ID: types.SnapshotID(snapshotID), + Hostname: "host", + StartedAt: time.Now(), + }) + if err != nil { + t.Fatalf("failed to create snapshot %s: %v", snapshotID, err) + } + + for _, path := range paths { + err := files.Create(ctx, nil, &database.File{ + Path: types.FilePath(path), + MTime: time.Now().Truncate(time.Second), + Mode: 0644, + }) + if err != nil { + t.Fatalf("failed to create %s: %v", path, err) + } + + err = snapshots.AddFile(ctx, nil, snapshotID, path) + if err != nil { + t.Fatalf("failed to add %s to %s: %v", path, snapshotID, err) + } + } + } + + found, err := files.ListUnderPathInSnapshot(ctx, "/src", "one") + if err != nil { + t.Fatalf("failed to list files: %v", err) + } + + got := make([]string, 0, len(found)) + for _, f := range found { + got = append(got, f.Path.String()) + } + + if !slices.Equal(got, []string{"/src/a.txt"}) { + t.Errorf("got %q, want only /src/a.txt", got) + } +} + func TestFileRepositorySymlink(t *testing.T) { t.Parallel() diff --git a/internal/snapshot/module.go b/internal/snapshot/module.go index fc0dfec..e9f6aab 100644 --- a/internal/snapshot/module.go +++ b/internal/snapshot/module.go @@ -16,6 +16,9 @@ type ScannerParams struct { Fs afero.Fs Exclude []string // Exclude patterns (combined global + snapshot-specific) SkipErrors bool // Skip file read errors (log loudly but continue) + // PreviousSnapshotID is the most recent completed snapshot of the + // same name, "" for none. + PreviousSnapshotID string } // Module exports backup functionality as an fx module. @@ -44,17 +47,18 @@ func provideScannerFactory( } return NewScanner(ScannerConfig{ - FS: params.Fs, - ChunkSize: cfg.ChunkSize.Int64(), - Repositories: repos, - Storage: storer, - MaxBlobSize: cfg.BlobSizeLimit.Int64(), - CompressionLevel: cfg.CompressionLevel, - AgeRecipients: cfg.AgeRecipients, - EnableProgress: params.EnableProgress, - UI: params.UI, - Exclude: excludes, - SkipErrors: params.SkipErrors, + FS: params.Fs, + ChunkSize: cfg.ChunkSize.Int64(), + Repositories: repos, + Storage: storer, + MaxBlobSize: cfg.BlobSizeLimit.Int64(), + CompressionLevel: cfg.CompressionLevel, + AgeRecipients: cfg.AgeRecipients, + EnableProgress: params.EnableProgress, + UI: params.UI, + Exclude: excludes, + SkipErrors: params.SkipErrors, + PreviousSnapshotID: params.PreviousSnapshotID, }) } } diff --git a/internal/snapshot/scanner.go b/internal/snapshot/scanner.go index dc98d4f..9f46cb4 100644 --- a/internal/snapshot/scanner.go +++ b/internal/snapshot/scanner.go @@ -58,6 +58,10 @@ type Scanner struct { compressionLevel int ageRecipient string snapshotID string // Current snapshot being processed + // previousSnapshotID is the most recent completed snapshot of the + // same name, "" for none. Only a file it lists can be counted as + // deleted. + previousSnapshotID string // currentSourcePath is the source directory being scanned, stored with // each file record. currentSourcePath string @@ -129,6 +133,9 @@ type ScannerConfig struct { // SkipErrors skips files that cannot be opened or read (log loudly but // continue); packer, database, encryption, and upload errors still abort. SkipErrors bool + // PreviousSnapshotID is the most recent completed snapshot of the + // same name, "" for none. + PreviousSnapshotID string } // ScanResult contains the results of a scan operation. Files and bytes @@ -204,6 +211,7 @@ func NewScanner(cfg ScannerConfig) *Scanner { compiledExclude: compiledExclude, progress: progress, skipErrors: cfg.SkipErrors, + previousSnapshotID: cfg.PreviousSnapshotID, ui: uiw, pendingChunkHashes: make(map[string]struct{}), } @@ -255,8 +263,9 @@ func (s *Scanner) Scan( filesToProcess := scanResult.FilesToProcess - // Phase 1b: Detect deleted files by comparing DB against scanned files - err = s.detectDeletedFilesFromMap(ctx, knownFiles, existingFiles, result) + // Phase 1b: Detect deleted files by comparing the previous snapshot + // of this name against scanned files + err = s.detectDeletedFiles(ctx, path, existingFiles, result) if err != nil { return nil, fmt.Errorf("detecting deleted files: %w", err) } @@ -436,9 +445,8 @@ func (s *Scanner) summarizeScanPhase( } // loadKnownFiles loads the known files at and beneath path from the -// database into a map for fast lookup. Every loaded file the scan does -// not find is counted as deleted. This avoids per-file database queries -// during the scan phase. +// database into a map for fast lookup. This avoids per-file database +// queries during the scan phase. func (s *Scanner) loadKnownFiles( ctx context.Context, path string, ) (map[string]*database.File, error) { @@ -1938,21 +1946,28 @@ func (s *Scanner) queueFileForBatchInsert( }) } -// detectDeletedFilesFromMap finds files that existed in previous snapshots -// but no longer exist. Uses pre-loaded maps to avoid any filesystem or -// database access. -func (s *Scanner) detectDeletedFilesFromMap( +// detectDeletedFiles counts as deleted each file at or beneath path that +// the previous snapshot of this name lists and the walk did not find. +// A file that is excluded, or inside an excluded directory, is not +// counted: the walk skips it whether or not it is still on disk. +func (s *Scanner) detectDeletedFiles( ctx context.Context, - knownFiles map[string]*database.File, + path string, existingFiles map[string]struct{}, result *ScanResult, ) error { - if len(knownFiles) == 0 { + if s.previousSnapshotID == "" { return nil } - // Check each known file against the enumerated set (no filesystem access needed) - for path, file := range knownFiles { + previousFiles, err := s.repos.Files.ListUnderPathInSnapshot( + ctx, path, s.previousSnapshotID) + if err != nil { + return fmt.Errorf("listing files of snapshot %s: %w", + s.previousSnapshotID, err) + } + + for _, file := range previousFiles { // Check context cancellation periodically select { case <-ctx.Done(): @@ -1960,13 +1975,16 @@ func (s *Scanner) detectDeletedFilesFromMap( default: } - // Check if the file exists in our enumerated set - if _, exists := existingFiles[path]; !exists { - // File has been deleted - result.FilesDeleted++ - result.BytesDeleted += file.Size - log.Debug("Detected deleted file", "path", path, "size", file.Size) + filePath := file.Path.String() + + _, exists := existingFiles[filePath] + if exists || s.excludedOrInExcludedDir(filePath, path) { + continue } + + result.FilesDeleted++ + result.BytesDeleted += file.Size + log.Debug("Detected deleted file", "path", filePath, "size", file.Size) } if result.FilesDeleted > 0 { @@ -1977,6 +1995,20 @@ func (s *Scanner) detectDeletedFilesFromMap( return nil } +// excludedOrInExcludedDir reports whether filePath, which is at or +// beneath rootPath, or any directory between rootPath and it, matches an +// exclude pattern. A pattern such as **/node_modules matches the +// directory but not the files inside it, which the walk skips too. +func (s *Scanner) excludedOrInExcludedDir(filePath, rootPath string) bool { + for p := filePath; len(p) > len(rootPath); p = filepath.Dir(p) { + if s.shouldExclude(p, rootPath) { + return true + } + } + + return false +} + // wrapPermissionError augments permission errors with platform-specific // remediation instructions. On macOS, TCC-protected directories (Calendars, // Reminders, Photos, etc.) return EPERM unless the running application has diff --git a/internal/vaultik/fault_injection_test.go b/internal/vaultik/fault_injection_test.go index 1595151..805cec3 100644 --- a/internal/vaultik/fault_injection_test.go +++ b/internal/vaultik/fault_injection_test.go @@ -661,17 +661,18 @@ func faultScannerFactory( ) snapshot.ScannerFactory { return func(params snapshot.ScannerParams) *snapshot.Scanner { return snapshot.NewScanner(snapshot.ScannerConfig{ - FS: params.Fs, - Storage: storer, - ChunkSize: faultChunkSize, - MaxBlobSize: faultMaxBlobSize, - CompressionLevel: cfg.CompressionLevel, - AgeRecipients: cfg.AgeRecipients, - Repositories: repos, - EnableProgress: params.EnableProgress, - UI: params.UI, - Exclude: params.Exclude, - SkipErrors: params.SkipErrors, + FS: params.Fs, + Storage: storer, + ChunkSize: faultChunkSize, + MaxBlobSize: faultMaxBlobSize, + CompressionLevel: cfg.CompressionLevel, + AgeRecipients: cfg.AgeRecipients, + Repositories: repos, + EnableProgress: params.EnableProgress, + UI: params.UI, + Exclude: params.Exclude, + SkipErrors: params.SkipErrors, + PreviousSnapshotID: params.PreviousSnapshotID, }) } } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 9b767df..69b21d9 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -240,12 +240,18 @@ func (v *Vaultik) createNamedSnapshot( return err } + previousSnapshotID, err := v.previousSnapshotID(snapName) + if err != nil { + return err + } + scanner := v.ScannerFactory(snapshot.ScannerParams{ - EnableProgress: !opts.Cron, - UI: v.UI, - Fs: v.Fs, - Exclude: v.Config.GetExcludes(snapName), - SkipErrors: opts.SkipErrors, + EnableProgress: !opts.Cron, + UI: v.UI, + Fs: v.Fs, + Exclude: v.Config.GetExcludes(snapName), + SkipErrors: opts.SkipErrors, + PreviousSnapshotID: previousSnapshotID, }) snapshotID, err := v.SnapshotManager.CreateSnapshotWithName( @@ -280,6 +286,24 @@ func (v *Vaultik) createNamedSnapshot( return nil } +// previousSnapshotID returns the ID of the most recent completed snapshot +// named snapName in the local index, or "" when there is none. +func (v *Vaultik) previousSnapshotID(snapName string) (string, error) { + snapshots, err := v.Repositories.Snapshots.ListRecent(v.ctx, listRecentLimit) + if err != nil { + return "", fmt.Errorf("listing snapshots: %w", err) + } + + for _, s := range snapshots { + if s.CompletedAt != nil && + parseSnapshotName(s.ID.String(), s.Hostname.String()) == snapName { + return s.ID.String(), nil + } + } + + return "", nil +} + // resolveSnapshotPaths resolves source directories to absolute paths // with symlink resolution. func (v *Vaultik) resolveSnapshotPaths(snapName string) ([]string, error) { diff --git a/internal/vaultik/snapshot_deleted_count_test.go b/internal/vaultik/snapshot_deleted_count_test.go new file mode 100644 index 0000000..d86f991 --- /dev/null +++ b/internal/vaultik/snapshot_deleted_count_test.go @@ -0,0 +1,196 @@ +package vaultik_test + +import ( + "bytes" + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/config" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/storage" + "sneak.berlin/go/vaultik/internal/ui" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// These tests cover https://git.eeqj.de/sneak/vaultik/issues/304: a +// backup counts a file as deleted only when the previous snapshot of +// the same name had it and it is gone, and never counts an excluded +// file. + +// deletedCountEnv is a backup setup over the source directory src whose +// user-facing output is kept in out. Each test configures its snapshots +// with setSnapshot. +type deletedCountEnv struct { + v *vaultik.Vaultik + out *bytes.Buffer + src string +} + +// newDeletedCountEnv writes files, keyed by path relative to src. +func newDeletedCountEnv(t *testing.T, files map[string][]byte) *deletedCountEnv { + t.Helper() + + ctx := context.Background() + fs := afero.NewOsFs() + tempDir := t.TempDir() + srcDir := filepath.Join(tempDir, "src") + + for rel, content := range files { + path := filepath.Join(srcDir, rel) + require.NoError(t, fs.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, afero.WriteFile(fs, path, content, 0o644)) + } + + cfg := faultTestConfig() + cfg.IndexPath = filepath.Join(tempDir, "index.sqlite") + cfg.Snapshots = map[string]config.SnapshotConfig{} + + store, err := storage.NewFileStorer(filepath.Join(tempDir, "remote")) + require.NoError(t, err) + + db, err := database.New(ctx, cfg.IndexPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + repos := database.NewRepositories(db) + out := &bytes.Buffer{} + v := newBackupVaultik(ctx, cfg, store, repos, db, fs) + v.UI = ui.NewWithColor(out, false) + + return &deletedCountEnv{v: v, out: out, src: srcDir} +} + +// setSnapshot configures the snapshot name to back up the path rel +// beneath src, with the given excludes. +func (e *deletedCountEnv) setSnapshot(name, rel string, exclude ...string) { + e.v.Config.Snapshots[name] = config.SnapshotConfig{ + Paths: []string{filepath.Join(e.src, rel)}, + Exclude: exclude, + } +} + +// backUp runs a backup of the named snapshot and returns its output. +func (e *deletedCountEnv) backUp(t *testing.T, name string) string { + t.Helper() + + e.out.Reset() + require.NoError(t, e.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{ + Snapshots: []string{name}, + })) + + return e.out.String() +} + +// A file this snapshot excludes is not counted as deleted: neither one +// that another snapshot backs up, nor one that the previous snapshot of +// this name had before the exclude was added. +func TestBackupDoesNotCountExcludedFileAsDeleted(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newDeletedCountEnv(t, map[string][]byte{ + "top.txt": []byte("top"), + "cache/tmp.bin": []byte("cache"), + "photos/a.jpg": []byte("photo a"), + "photos/b.jpg": []byte("photo b"), + "photos/c.jpg": []byte("photo c"), + }) + env.setSnapshot("photos", "photos") + env.setSnapshot("home", "", "/photos") + + env.backUp(t, "photos") + assert.Contains(t, env.backUp(t, "home"), filesLine(2, 2, 0)) + + env.setSnapshot("home", "", "/photos", "/cache") + assert.Contains(t, env.backUp(t, "home"), filesLine(1, 0, 1)) +} + +// A file inside a directory that an exclude added after a backup +// matches is not counted as deleted, though the pattern matches only the +// directory. +func TestBackupDoesNotCountFileInExcludedDirectoryAsDeleted(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newDeletedCountEnv(t, map[string][]byte{ + "top.txt": []byte("top"), + "app/node_modules/dep.js": []byte("dep"), + }) + env.setSnapshot("home", "") + + assert.Contains(t, env.backUp(t, "home"), filesLine(2, 2, 0)) + + env.setSnapshot("home", "", "**/node_modules") + assert.Contains(t, env.backUp(t, "home"), filesLine(1, 0, 1)) +} + +// A backup counts only files that the previous snapshot of its own name +// had, not one that a later snapshot of another name over the same path +// had and that is since removed. +func TestBackupIgnoresSnapshotOfAnotherName(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newDeletedCountEnv(t, map[string][]byte{ + "kept.txt": []byte("kept"), + }) + env.setSnapshot("home", "") + env.setSnapshot("other", "") + + env.backUp(t, "home") + + // Snapshot start times are kept in whole seconds: wait so that other's + // snapshot is strictly the most recent. + time.Sleep(time.Second) + + extra := filepath.Join(env.src, "extra.txt") + require.NoError(t, os.WriteFile(extra, []byte("extra"), 0o600)) + env.backUp(t, "other") + require.NoError(t, os.Remove(extra)) + + assert.Contains(t, env.backUp(t, "home"), filesLine(1, 0, 1)) +} + +// A file removed from disk is counted as deleted by the next backup of +// the snapshot that had it, and not by any later backup of that +// snapshot, nor by the first backup of another name over the same path. +func TestBackupCountsDeletedFileOnce(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + aContent := bytes.Repeat([]byte("a"), 1000) + bContent := bytes.Repeat([]byte("b"), 2000) + keptContent := bytes.Repeat([]byte("k"), 500) + + env := newDeletedCountEnv(t, map[string][]byte{ + "a.txt": aContent, + "b.txt": bContent, + "kept.txt": keptContent, + }) + env.setSnapshot("home", "") + env.setSnapshot("other", "") + + assert.Contains(t, env.backUp(t, "home"), filesLine(3, 3, 0)) + + require.NoError(t, os.Remove(filepath.Join(env.src, "a.txt"))) + require.NoError(t, os.Remove(filepath.Join(env.src, "b.txt"))) + + summary := env.backUp(t, "home") + assert.Contains(t, summary, + "Files: 1 examined, 0 backed up, 1 unchanged, 2 deleted.") + assert.Contains(t, summary, + fmt.Sprintf("Data: %s total (%s backed up), %s deleted.", + env.v.UI.Size(int64(len(keptContent))), env.v.UI.Size(0), + env.v.UI.Size(int64(len(aContent)+len(bContent))))) + + assert.Contains(t, env.backUp(t, "home"), filesLine(1, 0, 1)) + assert.Contains(t, env.backUp(t, "other"), filesLine(1, 0, 1)) +}