diff --git a/lib/versioner/simple.go b/lib/versioner/simple.go index e4865116a..1d173ba7f 100644 --- a/lib/versioner/simple.go +++ b/lib/versioner/simple.go @@ -8,6 +8,7 @@ package versioner import ( "context" + "sort" "strconv" "time" @@ -57,17 +58,7 @@ func (v simple) Archive(filePath string) error { return err } - // Versions are sorted by timestamp in the file name, oldest first. - versions := findAllVersions(v.versionsFs, filePath) - if len(versions) > v.keep { - for _, toRemove := range versions[:len(versions)-v.keep] { - l.Debugln("cleaning out", toRemove) - err = v.versionsFs.Remove(toRemove) - if err != nil { - l.Warnln("removing old version:", err) - } - } - } + cleanVersions(v.versionsFs, findAllVersions(v.versionsFs, filePath), v.toRemove) return nil } @@ -81,5 +72,40 @@ func (v simple) Restore(filepath string, versionTime time.Time) error { } func (v simple) Clean(ctx context.Context) error { - return cleanByDay(ctx, v.versionsFs, v.cleanoutDays) + return clean(ctx, v.versionsFs, v.toRemove) +} + +func (v simple) toRemove(versions []string, now time.Time) []string { + var remove []string + + // The list of versions may or may not be properly sorted. + sort.Strings(versions) + + // If the amount of elements exceeds the limit: the oldest elements are to be removed. + if len(versions) > v.keep { + remove = versions[:len(versions)-v.keep] + versions = versions[len(versions)-v.keep:] + } + + // If cleanoutDays is not a positive value then don't remove based on age. + if v.cleanoutDays <= 0 { + return remove + } + + maxAge := time.Duration(v.cleanoutDays) * 24 * time.Hour + + // For the rest of the versions, elements which are too old are to be removed + for _, version := range versions { + versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local) + if err != nil { + l.Debugf("Versioner: file name %q is invalid: %v", version, err) + continue + } + + if now.Sub(versionTime) > maxAge { + remove = append(remove, version) + } + } + + return remove } diff --git a/lib/versioner/staggered.go b/lib/versioner/staggered.go index 2a341590e..6f844b195 100644 --- a/lib/versioner/staggered.go +++ b/lib/versioner/staggered.go @@ -60,79 +60,7 @@ func newStaggered(cfg config.FolderConfiguration) Versioner { } func (v *staggered) Clean(ctx context.Context) error { - l.Debugln("Versioner clean: Cleaning", v.versionsFs) - - if _, err := v.versionsFs.Stat("."); fs.IsNotExist(err) { - // There is no need to clean a nonexistent dir. - return nil - } - - versionsPerFile := make(map[string][]string) - dirTracker := make(emptyDirTracker) - - walkFn := func(path string, f fs.FileInfo, err error) error { - if err != nil { - return err - } - select { - case <-ctx.Done(): - return ctx.Err() - default: - } - - if f.IsDir() && !f.IsSymlink() { - dirTracker.addDir(path) - return nil - } - - // Regular file, or possibly a symlink. - dirTracker.addFile(path) - - name, _ := UntagFilename(path) - if name == "" { - return nil - } - - versionsPerFile[name] = append(versionsPerFile[name], path) - - return nil - } - - if err := v.versionsFs.Walk(".", walkFn); err != nil { - l.Warnln("Versioner: error scanning versions dir", err) - return err - } - - for _, versionList := range versionsPerFile { - select { - case <-ctx.Done(): - return ctx.Err() - default: - } - v.expire(versionList) - } - - dirTracker.deleteEmptyDirs(v.versionsFs) - - l.Debugln("Cleaner: Finished cleaning", v.versionsFs) - return nil -} - -func (v *staggered) expire(versions []string) { - l.Debugln("Versioner: Expiring versions", versions) - for _, file := range v.toRemove(versions, time.Now()) { - if fi, err := v.versionsFs.Lstat(file); err != nil { - l.Warnln("versioner:", err) - continue - } else if fi.IsDir() { - l.Infof("non-file %q is named like a file version", file) - continue - } - - if err := v.versionsFs.Remove(file); err != nil { - l.Warnf("Versioner: can't remove %q: %v", file, err) - } - } + return clean(ctx, v.versionsFs, v.toRemove) } func (v *staggered) toRemove(versions []string, now time.Time) []string { @@ -192,7 +120,7 @@ func (v *staggered) Archive(filePath string) error { return err } - v.expire(findAllVersions(v.versionsFs, filePath)) + cleanVersions(v.versionsFs, findAllVersions(v.versionsFs, filePath), v.toRemove) return nil } diff --git a/lib/versioner/trashcan.go b/lib/versioner/trashcan.go index a72d7b42e..e300de92a 100644 --- a/lib/versioner/trashcan.go +++ b/lib/versioner/trashcan.go @@ -56,7 +56,51 @@ func (t *trashcan) String() string { } func (t *trashcan) Clean(ctx context.Context) error { - return cleanByDay(ctx, t.versionsFs, t.cleanoutDays) + if t.cleanoutDays <= 0 { + return nil + } + + if _, err := t.versionsFs.Lstat("."); fs.IsNotExist(err) { + return nil + } + + cutoff := time.Now().Add(time.Duration(-24*t.cleanoutDays) * time.Hour) + dirTracker := make(emptyDirTracker) + + walkFn := func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + + if info.IsDir() && !info.IsSymlink() { + dirTracker.addDir(path) + return nil + } + + if info.ModTime().Before(cutoff) { + // The file is too old; remove it. + err = t.versionsFs.Remove(path) + } else { + // Keep this file, and remember it so we don't unnecessarily try + // to remove this directory. + dirTracker.addFile(path) + } + return err + } + + if err := t.versionsFs.Walk(".", walkFn); err != nil { + return err + } + + dirTracker.deleteEmptyDirs(t.versionsFs) + + return nil } func (t *trashcan) GetVersions() (map[string][]FileVersion, error) { diff --git a/lib/versioner/trashcan_test.go b/lib/versioner/trashcan_test.go index 35bad47ce..348d2ac41 100644 --- a/lib/versioner/trashcan_test.go +++ b/lib/versioner/trashcan_test.go @@ -7,7 +7,11 @@ package versioner import ( + "context" + "fmt" "io" + "os" + "path/filepath" "testing" "time" @@ -120,3 +124,70 @@ func writeFile(t *testing.T, filesystem fs.Filesystem, name, content string) { t.Fatal(n, len(content), err) } } + +func TestTrashcanCleanOut(t *testing.T) { + testDir := t.TempDir() + + cfg := config.FolderConfiguration{ + FilesystemType: fs.FilesystemTypeBasic, + Path: testDir, + Versioning: config.VersioningConfiguration{ + Params: map[string]string{ + "cleanoutDays": "7", + }, + }, + } + + fs := cfg.Filesystem(nil) + + v := newTrashcan(cfg) + + var testcases = map[string]bool{ + ".stversions/file1": false, + ".stversions/file2": true, + ".stversions/keep1/file1": false, + ".stversions/keep1/file2": false, + ".stversions/keep2/file1": false, + ".stversions/keep2/file2": true, + ".stversions/keep3/keepsubdir/file1": false, + ".stversions/remove/file1": true, + ".stversions/remove/file2": true, + ".stversions/remove/removesubdir/file1": true, + } + + t.Run(fmt.Sprintf("trashcan versioner trashcan clean up"), func(t *testing.T) { + oldTime := time.Now().Add(-8 * 24 * time.Hour) + for file, shouldRemove := range testcases { + fs.MkdirAll(filepath.Dir(file), 0777) + + writeFile(t, fs, file, "some content") + + if shouldRemove { + if err := fs.Chtimes(file, oldTime, oldTime); err != nil { + t.Fatal(err) + } + } + } + + if err := v.Clean(context.Background()); err != nil { + t.Fatal(err) + } + + for file, shouldRemove := range testcases { + _, err := fs.Lstat(file) + if shouldRemove && !os.IsNotExist(err) { + t.Error(file, "should have been removed") + } else if !shouldRemove && err != nil { + t.Error(file, "should not have been removed") + } + } + + if _, err := fs.Lstat(".stversions/keep3"); os.IsNotExist(err) { + t.Error("directory with non empty subdirs should not be removed") + } + + if _, err := fs.Lstat(".stversions/remove"); !os.IsNotExist(err) { + t.Error("empty directory should have been removed") + } + }) +} diff --git a/lib/versioner/util.go b/lib/versioner/util.go index 07e76f14e..04809a2a4 100644 --- a/lib/versioner/util.go +++ b/lib/versioner/util.go @@ -287,50 +287,70 @@ func findAllVersions(fs fs.Filesystem, filePath string) []string { return versions } -func cleanByDay(ctx context.Context, versionsFs fs.Filesystem, cleanoutDays int) error { - if cleanoutDays <= 0 { +func clean(ctx context.Context, versionsFs fs.Filesystem, toRemove func([]string, time.Time) []string) error { + l.Debugln("Versioner clean: Cleaning", versionsFs) + + if _, err := versionsFs.Stat("."); fs.IsNotExist(err) { + // There is no need to clean a nonexistent dir. return nil } - if _, err := versionsFs.Lstat("."); fs.IsNotExist(err) { - return nil - } - - cutoff := time.Now().Add(time.Duration(-24*cleanoutDays) * time.Hour) + versionsPerFile := make(map[string][]string) dirTracker := make(emptyDirTracker) - walkFn := func(path string, info fs.FileInfo, err error) error { + walkFn := func(path string, f fs.FileInfo, err error) error { if err != nil { return err } - select { case <-ctx.Done(): return ctx.Err() default: } - if info.IsDir() && !info.IsSymlink() { + if f.IsDir() && !f.IsSymlink() { dirTracker.addDir(path) return nil } - if info.ModTime().Before(cutoff) { - // The file is too old; remove it. - err = versionsFs.Remove(path) - } else { - // Keep this file, and remember it so we don't unnecessarily try - // to remove this directory. - dirTracker.addFile(path) + // Regular file, or possibly a symlink. + dirTracker.addFile(path) + + name, _ := UntagFilename(path) + if name == "" { + return nil } - return err + + versionsPerFile[name] = append(versionsPerFile[name], path) + + return nil } if err := versionsFs.Walk(".", walkFn); err != nil { + l.Warnln("Versioner: error scanning versions dir", err) return err } + for _, versionList := range versionsPerFile { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + cleanVersions(versionsFs, versionList, toRemove) + } + dirTracker.deleteEmptyDirs(versionsFs) + l.Debugln("Cleaner: Finished cleaning", versionsFs) return nil } + +func cleanVersions(versionsFs fs.Filesystem, versions []string, toRemove func([]string, time.Time) []string) { + l.Debugln("Versioner: Expiring versions", versions) + for _, file := range toRemove(versions, time.Now()) { + if err := versionsFs.Remove(file); err != nil { + l.Warnf("Versioner: can't remove %q: %v", file, err) + } + } +} diff --git a/lib/versioner/versioner_test.go b/lib/versioner/versioner_test.go deleted file mode 100644 index 4ba73c36c..000000000 --- a/lib/versioner/versioner_test.go +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright (C) 2020 The Syncthing Authors. -// -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -package versioner - -import ( - "context" - "fmt" - "os" - "path/filepath" - "testing" - "time" - - "github.com/syncthing/syncthing/lib/config" - "github.com/syncthing/syncthing/lib/fs" -) - -func TestVersionerCleanOut(t *testing.T) { - cfg := config.FolderConfiguration{ - FilesystemType: fs.FilesystemTypeBasic, - Path: "testdata", - Versioning: config.VersioningConfiguration{ - Params: map[string]string{ - "cleanoutDays": "7", - }, - }, - } - - testCasesVersioner := map[string]Versioner{ - "simple": newSimple(cfg), - "trashcan": newTrashcan(cfg), - } - - var testcases = map[string]bool{ - "testdata/.stversions/file1": false, - "testdata/.stversions/file2": true, - "testdata/.stversions/keep1/file1": false, - "testdata/.stversions/keep1/file2": false, - "testdata/.stversions/keep2/file1": false, - "testdata/.stversions/keep2/file2": true, - "testdata/.stversions/keep3/keepsubdir/file1": false, - "testdata/.stversions/remove/file1": true, - "testdata/.stversions/remove/file2": true, - "testdata/.stversions/remove/removesubdir/file1": true, - } - - for versionerType, versioner := range testCasesVersioner { - t.Run(fmt.Sprintf("%v versioner trashcan clean up", versionerType), func(t *testing.T) { - os.RemoveAll("testdata") - defer os.RemoveAll("testdata") - - oldTime := time.Now().Add(-8 * 24 * time.Hour) - for file, shouldRemove := range testcases { - os.MkdirAll(filepath.Dir(file), 0777) - if err := os.WriteFile(file, []byte("data"), 0644); err != nil { - t.Fatal(err) - } - if shouldRemove { - if err := os.Chtimes(file, oldTime, oldTime); err != nil { - t.Fatal(err) - } - } - } - - if err := versioner.Clean(context.Background()); err != nil { - t.Fatal(err) - } - - for file, shouldRemove := range testcases { - _, err := os.Lstat(file) - if shouldRemove && !os.IsNotExist(err) { - t.Error(file, "should have been removed") - } else if !shouldRemove && err != nil { - t.Error(file, "should not have been removed") - } - } - - if _, err := os.Lstat("testdata/.stversions/keep3"); os.IsNotExist(err) { - t.Error("directory with non empty subdirs should not be removed") - } - - if _, err := os.Lstat("testdata/.stversions/remove"); !os.IsNotExist(err) { - t.Error("empty directory should have been removed") - } - }) - } -}