From 6e768a838775985a705ceb55d7b61c6470c160fe Mon Sep 17 00:00:00 2001 From: Eric P Date: Tue, 13 Sep 2022 19:20:04 +0200 Subject: [PATCH] lib/versioner: Fix cleaning behaviour (fixes #7988) (#8537) The cleaning logic in util.go was used by Simple and Trashcan but only really suited Trashcan since it works based on mtimes which Simple does not use. The cleaning logic in util.go was moved to trashcan.go. Staggered and Simple seemed to be able to benefit from the same base so util.go now has the base for those two with an added parameter which takes a function so it can still handle versioner-specific logic to decide which files to clean up. Simple now also correctly cleans files based on their time-stamp in the title together with a specific maximum amount to keep. The Archive function in Simple.go was changed to get rid of duplicated code. Additionally the trashcan testcase which was used by Trashcan as well as Simple was moved from versioner_test.go to trashcan_test.go to keep it clean, there was no need to keep it in a separate test file --- lib/versioner/simple.go | 50 +++++++++++++----- lib/versioner/staggered.go | 76 +--------------------------- lib/versioner/trashcan.go | 46 ++++++++++++++++- lib/versioner/trashcan_test.go | 71 ++++++++++++++++++++++++++ lib/versioner/util.go | 56 +++++++++++++------- lib/versioner/versioner_test.go | 90 --------------------------------- 6 files changed, 194 insertions(+), 195 deletions(-) delete mode 100644 lib/versioner/versioner_test.go 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") - } - }) - } -}