From f5e938b3e62932b400ed6031d0b1e31870bcede5 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 02:37:08 +0000 Subject: [PATCH] Count a file as deleted only if the previous snapshot of its name had it (closes #304) A backup counted as deleted every file in the local index under the source path that the walk did not find. That took in files the snapshot excludes but another snapshot backs up, and files removed long ago, which every later backup, and the first backup of another name over the same path, reported as deleted again. The backup now looks up the most recent completed snapshot of the same name and counts only the files it lists that the walk did not find and that are not excluded. The snapshots row stores no deleted figure; only the backup's output changes. Model: opus-5-5 --- ARCHITECTURE.md | 8 +- TODO.md | 12 ++ internal/database/files.go | 44 ++++++ internal/database/files_test.go | 59 +++++++ internal/snapshot/module.go | 26 ++-- internal/snapshot/scanner.go | 56 ++++--- internal/vaultik/fault_injection_test.go | 23 +-- internal/vaultik/snapshot.go | 34 +++- .../vaultik/snapshot_deleted_count_test.go | 146 ++++++++++++++++++ 9 files changed, 358 insertions(+), 50 deletions(-) create mode 100644 internal/vaultik/snapshot_deleted_count_test.go diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b791882..9a010bb 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; an excluded file 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/TODO.md b/TODO.md index 61fdbc6..a8539c5 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,18 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-13: Counted a file as deleted only when the previous snapshot + of the same name had it + ([issue #304](https://git.eeqj.de/sneak/vaultik/issues/304)). The + deleted figure of a backup counted every file in the local index under + the source path that the walk did not find. That took in files the + snapshot excludes but another snapshot backs up, and files deleted + long ago, which every later backup, and the first backup of another + name over the same path, reported as deleted again. A file is now + counted only when the previous completed snapshot of the same name + lists it and the walk did not find it, and an excluded file is never + counted. + - 2026-10-12: Bound the local index to the destination an `s3.*` config names ([issue #298](https://git.eeqj.de/sneak/vaultik/issues/298)). Only `storage_url` was checked, so after `s3.bucket` or `s3.prefix` 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 c810278..0fb4a4e 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) { @@ -1933,21 +1941,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. +// An excluded file 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(): @@ -1955,13 +1970,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.shouldExclude(filePath, path) { + continue } + + result.FilesDeleted++ + result.BytesDeleted += file.Size + log.Debug("Detected deleted file", "path", filePath, "size", file.Size) } if result.FilesDeleted > 0 { 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 97ecfdb..f80212a 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -213,12 +213,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( @@ -253,6 +259,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..2eddbfd --- /dev/null +++ b/internal/vaultik/snapshot_deleted_count_test.go @@ -0,0 +1,146 @@ +package vaultik_test + +import ( + "bytes" + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "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 sets the snapshots in +// v.Config.Snapshots. +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") + + 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} +} + +// 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.v.Config.Snapshots = map[string]config.SnapshotConfig{ + "photos": {Paths: []string{filepath.Join(env.src, "photos")}}, + "home": {Paths: []string{env.src}, Exclude: []string{"/photos"}}, + } + + env.backUp(t, "photos") + assert.Contains(t, env.backUp(t, "home"), filesLine(2, 2, 0)) + + env.v.Config.Snapshots["home"] = config.SnapshotConfig{ + Paths: []string{env.src}, + Exclude: []string{"/photos", "/cache"}, + } + 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.v.Config.Snapshots = map[string]config.SnapshotConfig{ + "home": {Paths: []string{env.src}}, + "other": {Paths: []string{env.src}}, + } + + 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)) +}