diff --git a/changelog/unreleased/issue-5063 b/changelog/unreleased/issue-5063 new file mode 100644 index 000000000..65aa379e4 --- /dev/null +++ b/changelog/unreleased/issue-5063 @@ -0,0 +1,11 @@ +Bugfix: Correctly `backup` extended metadata when using VSS on Windows + +On Windows, when creating a backup using the `--use-fs-snapshot` option, +then the extended metadata was not read from the filesystem snapshot. This +could result in errors when files have been removed in the meantime. + +This issue has been resolved. + +https://github.com/restic/restic/issues/5063 +https://github.com/restic/restic/pull/5097 +https://github.com/restic/restic/pull/5099 diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 562108a33..c7c0bcc50 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -95,6 +95,7 @@ type BackupOptions struct { } var backupOptions BackupOptions +var backupFSTestHook func(fs fs.FS) fs.FS // ErrInvalidSourceData is used to report an incomplete backup var ErrInvalidSourceData = errors.New("at least one source file could not be read") @@ -598,6 +599,10 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter targets = []string{filename} } + if backupFSTestHook != nil { + targetFS = backupFSTestHook(targetFS) + } + wg, wgCtx := errgroup.WithContext(ctx) cancelCtx, cancel := context.WithCancel(wgCtx) defer cancel() diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index 5e00b84b0..5926fdd54 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "testing" + "time" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/restic" @@ -111,6 +112,63 @@ func TestBackupWithRelativePath(t *testing.T) { rtest.Assert(t, latestSn.Parent != nil && latestSn.Parent.Equal(firstSnapshotID), "second snapshot selected unexpected parent %v instead of %v", latestSn.Parent, firstSnapshotID) } +type vssDeleteOriginalFS struct { + fs.FS + testdata string + hasRemoved bool +} + +func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) { + if !f.hasRemoved { + // call Lstat to trigger snapshot creation + _, _ = f.FS.Lstat(name) + // nuke testdata + var err error + for i := 0; i < 3; i++ { + // The CI sometimes runs into "The process cannot access the file because it is being used by another process" errors + // thus try a few times to remove the data + err = os.RemoveAll(f.testdata) + if err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if err != nil { + return nil, err + } + f.hasRemoved = true + } + return f.FS.Lstat(name) +} + +func TestBackupVSS(t *testing.T) { + if runtime.GOOS != "windows" || fs.HasSufficientPrivilegesForVSS() != nil { + t.Skip("vss fs test can only be run on windows with admin privileges") + } + + env, cleanup := withTestEnvironment(t) + defer cleanup() + + testSetupBackupData(t, env) + opts := BackupOptions{UseFsSnapshot: true} + + var testFS *vssDeleteOriginalFS + backupFSTestHook = func(fs fs.FS) fs.FS { + testFS = &vssDeleteOriginalFS{ + FS: fs, + testdata: env.testdata, + } + return testFS + } + defer func() { + backupFSTestHook = nil + }() + + testRunBackup(t, filepath.Dir(env.testdata), []string{"testdata"}, opts, env.gopts) + testListSnapshots(t, env.gopts, 1) + rtest.Equals(t, true, testFS.hasRemoved, "testdata was not removed") +} + func TestBackupParentSelection(t *testing.T) { env, cleanup := withTestEnvironment(t) defer cleanup() diff --git a/cmd/restic/integration_helpers_test.go b/cmd/restic/integration_helpers_test.go index 978deab3d..8ae3bb78a 100644 --- a/cmd/restic/integration_helpers_test.go +++ b/cmd/restic/integration_helpers_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "sync" "testing" @@ -168,6 +169,16 @@ type testEnvironment struct { gopts GlobalOptions } +type logOutputter struct { + t testing.TB +} + +func (l *logOutputter) Write(p []byte) (n int, err error) { + l.t.Helper() + l.t.Log(strings.TrimSuffix(string(p), "\n")) + return len(p), nil +} + // withTestEnvironment creates a test environment and returns a cleanup // function which removes it. func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { @@ -200,8 +211,11 @@ func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { Quiet: true, CacheDir: env.cache, password: rtest.TestPassword, - stdout: os.Stdout, - stderr: os.Stderr, + // stdout and stderr are written to by Warnf etc. That is the written data + // usually consists of one or multiple lines and therefore can be handled well + // by t.Log. + stdout: &logOutputter{t}, + stderr: &logOutputter{t}, extended: make(options.Options), // replace this hook with "nil" if listing a filetype more than once is necessary diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 839320816..03b3b9986 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -248,7 +248,8 @@ func (arch *Archiver) trackItem(item string, previous, current *restic.Node, s I // nodeFromFileInfo returns the restic node from an os.FileInfo. func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - node, err := restic.NodeFromFileInfo(filename, fi, ignoreXattrListError) + mappedFilename := arch.FS.MapFilename(filename) + node, err := restic.NodeFromFileInfo(mappedFilename, fi, ignoreXattrListError) if !arch.WithAtime { node.AccessTime = node.ModTime } diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index d10334301..70666506d 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -156,7 +156,7 @@ func (s *FileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat debug.Log("%v", snPath) - node, err := s.NodeFromFileInfo(snPath, f.Name(), fi, false) + node, err := s.NodeFromFileInfo(snPath, target, fi, false) if err != nil { _ = f.Close() completeError(err) diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 48c40dc90..06dbae9a0 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -18,6 +18,12 @@ func (fs Local) VolumeName(path string) string { return filepath.VolumeName(path) } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs Local) MapFilename(filename string) string { + return filename +} + // Open opens a file for reading. func (fs Local) Open(name string) (File, error) { f, err := os.Open(fixpath(name)) diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index 718dfc46d..db6c95155 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -145,6 +145,12 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { return os.Lstat(fs.snapshotPath(name)) } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs *LocalVss) MapFilename(filename string) string { + return fs.snapshotPath(filename) +} + // isMountPointIncluded is true if given mountpoint included by user. func (fs *LocalVss) isMountPointIncluded(mountPoint string) bool { if fs.excludeVolumes == nil { diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 47af74245..57864c87b 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -39,6 +39,12 @@ func (fs *Reader) VolumeName(_ string) string { return "" } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs *Reader) MapFilename(filename string) string { + return filename +} + // Open opens a file for reading. func (fs *Reader) Open(name string) (f File, err error) { switch name { @@ -223,7 +229,7 @@ func (r *readerFile) Close() error { var _ File = &readerFile{} // fakeFile implements all File methods, but only returns errors for anything -// except Stat() and Name(). +// except Stat() type fakeFile struct { name string os.FileInfo @@ -260,10 +266,6 @@ func (f fakeFile) Stat() (os.FileInfo, error) { return f.FileInfo, nil } -func (f fakeFile) Name() string { - return f.name -} - // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile. type fakeDir struct { entries []os.FileInfo diff --git a/internal/fs/interface.go b/internal/fs/interface.go index b26c56944..147773e2d 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -11,6 +11,7 @@ type FS interface { OpenFile(name string, flag int, perm os.FileMode) (File, error) Stat(name string) (os.FileInfo, error) Lstat(name string) (os.FileInfo, error) + MapFilename(filename string) string Join(elem ...string) string Separator() string @@ -33,5 +34,4 @@ type File interface { Readdir(int) ([]os.FileInfo, error) Seek(int64, int) (int64, error) Stat() (os.FileInfo, error) - Name() string }