From 784c5ecc1618dddfcc837258ff44fd99b1dab4e7 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Fri, 2 Oct 2026 02:21:44 -0700 Subject: [PATCH] fix: purge revisions with whole-second timestamps (#13036) Fixes #11167 Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../bugfix-purge-whole-second-revisions.md | 13 ++ ocis/pkg/revisions/purge_test.go | 120 ++++++++++ ocis/pkg/revisions/revisions.go | 8 +- ocis/pkg/revisions/revisions_test.go | 208 ++++++++++++++++++ 4 files changed, 348 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/bugfix-purge-whole-second-revisions.md diff --git a/changelog/unreleased/bugfix-purge-whole-second-revisions.md b/changelog/unreleased/bugfix-purge-whole-second-revisions.md new file mode 100644 index 00000000000..91a7368172a --- /dev/null +++ b/changelog/unreleased/bugfix-purge-whole-second-revisions.md @@ -0,0 +1,13 @@ +Bugfix: Purge revisions stored with whole-second timestamps + +The revisions purge command reported a clean storage when revisions were still +present. Revision filenames are the previous mtime formatted with +time.RFC3339Nano, which omits fractional seconds on a whole second. Names such +as `.REV.1601-01-01T00:00:00Z` never matched the purge pattern, so those +revisions stayed on disk. + +The matcher now treats the decimal point and fractional digits as optional. +Whole-second revisions and their `.mpk` and `.mlock` companions are discovered +and purged along with revisions that still carry a fractional timestamp. + +https://github.com/owncloud/ocis/issues/11167 diff --git a/ocis/pkg/revisions/purge_test.go b/ocis/pkg/revisions/purge_test.go index 050ca8be213..96b836ccde8 100644 --- a/ocis/pkg/revisions/purge_test.go +++ b/ocis/pkg/revisions/purge_test.go @@ -3,7 +3,9 @@ package revisions import ( "os" "path/filepath" + "sort" "testing" + "time" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/shamaton/msgpack/v2" @@ -53,3 +55,121 @@ func TestPurgeRevisionsDeletesBlobWithSpaceID(t *testing.T) { require.Equal(t, blobID, bs.deleted[0].BlobID) require.Equal(t, spaceID, bs.deleted[0].SpaceID, "SpaceID must be passed to the blobstore, otherwise the blob is orphaned") } + +// TestPurgeRevisionsMixedWholeSecondAndFractional purges a decomposed tree that +// mixes whole-second and fractional revision triplets. Each triplet is the +// revision node plus its .mpk and .mlock companion: three files, one blob, and +// one revision. Dry-run keeps every file and does not call the blobstore. +func TestPurgeRevisionsMixedWholeSecondAndFractional(t *testing.T) { + for _, d := range revisionDiscoverers() { + t.Run(d.name, func(t *testing.T) { + for _, dryRun := range []bool{true, false} { + name := "delete" + if dryRun { + name = "dry-run" + } + t.Run(name, func(t *testing.T) { + tree := newDecomposedRevisionTree(t) + bs := &recordingBlobstore{} + + files, blobs, revs := PurgeRevisions(d.find(tree.root), bs, dryRun, false) + + require.Equal(t, tree.revisions*3, files, "files") + require.Equal(t, tree.revisions, blobs, "blobs") + require.Equal(t, tree.revisions, revs, "revisions") + requirePresent(t, tree.preservedPaths) + + if dryRun { + require.Empty(t, bs.deleted) + requirePresent(t, tree.revisionPaths) + return + } + + require.Equal(t, tree.blobs, deletedBlobs(bs)) + requireAbsent(t, tree.revisionPaths) + }) + } + }) + } +} + +// TestPurgeRevisionsRetainsInvalidWholeSecondMessagePack keeps a whole-second +// .mpk whose metadata is not MessagePack. A sibling whole-second triplet is +// purged in the same call, which shows the retained file was recognized as a +// revision and then left in place by the existing read error path. That path +// also skips the blob delete. +func TestPurgeRevisionsRetainsInvalidWholeSecondMessagePack(t *testing.T) { + root := t.TempDir() + nodePath := decomposedNodePath(root, revisionFixtureSpaceID, revisionFixtureNodeID) + whole := time.Date(2024, 5, 22, 7, 32, 53, 0, time.UTC).UTC().Format(time.RFC3339Nano) + ancient := time.Date(1601, 1, 1, 0, 0, 0, 0, time.UTC).UTC().Format(time.RFC3339Nano) + require.Equal(t, "2024-05-22T07:32:53Z", whole) + require.Equal(t, "1601-01-01T00:00:00Z", ancient) + + validBase := nodePath + ".REV." + whole + badMPK := nodePath + ".REV." + ancient + ".mpk" + currentMPK := nodePath + ".mpk" + + writeFixtureFile(t, nodePath, nil) + writeFixtureFile(t, currentMPK, revisionBlobMPK(t, "blob-current")) + writeFixtureFile(t, validBase, nil) + writeFixtureFile(t, validBase+".mpk", revisionBlobMPK(t, "blob-valid")) + writeFixtureFile(t, validBase+".mlock", nil) + writeFixtureFile(t, badMPK, []byte("not-msgpack")) + + nodes := make(chan string, 6) + for _, p := range []string{ + badMPK, + validBase, + validBase + ".mpk", + validBase + ".mlock", + nodePath, + currentMPK, + } { + nodes <- p + } + close(nodes) + + bs := &recordingBlobstore{} + files, blobs, revs := PurgeRevisions(nodes, bs, false, false) + + require.Equal(t, 3, files, "files") + require.Equal(t, 1, blobs, "blobs") + require.Equal(t, 1, revs, "revisions") + require.Equal(t, []purgedBlob{{ + spaceID: revisionFixtureSpaceID, + blobID: "blob-valid", + }}, deletedBlobs(bs)) + requireAbsent(t, []string{validBase, validBase + ".mpk", validBase + ".mlock"}) + requirePresent(t, []string{badMPK, nodePath, currentMPK}) +} + +func deletedBlobs(bs *recordingBlobstore) []purgedBlob { + got := make([]purgedBlob, 0, len(bs.deleted)) + for _, n := range bs.deleted { + got = append(got, purgedBlob{spaceID: n.SpaceID, blobID: n.BlobID}) + } + sort.Slice(got, func(i, j int) bool { + if got[i].blobID != got[j].blobID { + return got[i].blobID < got[j].blobID + } + return got[i].spaceID < got[j].spaceID + }) + return got +} + +func requirePresent(t *testing.T, paths []string) { + t.Helper() + for _, p := range paths { + _, err := os.Stat(p) + require.NoError(t, err, p) + } +} + +func requireAbsent(t *testing.T, paths []string) { + t.Helper() + for _, p := range paths { + _, err := os.Stat(p) + require.True(t, os.IsNotExist(err), "%s still present: %v", p, err) + } +} diff --git a/ocis/pkg/revisions/revisions.go b/ocis/pkg/revisions/revisions.go index 519f18ec91e..f09531de290 100644 --- a/ocis/pkg/revisions/revisions.go +++ b/ocis/pkg/revisions/revisions.go @@ -18,7 +18,13 @@ var ( // 9113a718-8285-4b32-9042-f930f1a58ac2.REV.2024-05-22T07:32:53.89969726Z // 9113a718-8285-4b32-9042-f930f1a58ac2.REV.2024-05-22T07:32:53.89969726Z.mpk // 9113a718-8285-4b32-9042-f930f1a58ac2.REV.2024-05-22T07:32:53.89969726Z.mlock - _versionRegex = regexp.MustCompile(`\.REV\.[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\.[0-9]+Z*`) + // 9113a718-8285-4b32-9042-f930f1a58ac2.REV.2024-05-22T07:32:53Z + // 9113a718-8285-4b32-9042-f930f1a58ac2.REV.1601-01-01T00:00:00Z + // + // Fractional seconds are optional because decomposedfs formats the revision + // id with time.RFC3339Nano, which drops them when the mtime is a whole + // second. The pattern stays unanchored so .mpk and .mlock companions match. + _versionRegex = regexp.MustCompile(`\.REV\.[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z*`) ) // DelBlobstore is the interface for a blobstore that can delete blobs. diff --git a/ocis/pkg/revisions/revisions_test.go b/ocis/pkg/revisions/revisions_test.go index ab63f95d598..7354a1ac22a 100644 --- a/ocis/pkg/revisions/revisions_test.go +++ b/ocis/pkg/revisions/revisions_test.go @@ -5,11 +5,14 @@ import ( "io/fs" "os" "path/filepath" + "sort" "strconv" "testing" + "time" "github.com/google/uuid" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/lookup" + "github.com/shamaton/msgpack/v2" "github.com/test-go/testify/require" ) @@ -168,3 +171,208 @@ func initialize(numNodes int, numRevisions int) string { } return base } + +// revisionTimestampCase is a timestamp decomposedfs can write as a revision name. +// The stamp comes from time.Time.UTC().Format(time.RFC3339Nano), which omits the +// fractional part when the mtime falls on a whole second. +type revisionTimestampCase struct { + name string + at time.Time + want string + blobID string +} + +func revisionTimestampCases() []revisionTimestampCase { + return []revisionTimestampCase{ + { + name: "contemporary whole second", + at: time.Date(2024, 5, 22, 7, 32, 53, 0, time.UTC), + want: "2024-05-22T07:32:53Z", + blobID: "blob-contemporary", + }, + { + name: "year 1601 whole second", + at: time.Date(1601, 1, 1, 0, 0, 0, 0, time.UTC), + want: "1601-01-01T00:00:00Z", + blobID: "blob-1601", + }, + { + name: "one fractional digit", + at: time.Date(2024, 6, 1, 12, 0, 0, 100000000, time.UTC), + want: "2024-06-01T12:00:00.1Z", + blobID: "blob-frac-1", + }, + { + name: "nine fractional digits", + at: time.Date(2024, 6, 1, 12, 0, 1, 123456789, time.UTC), + want: "2024-06-01T12:00:01.123456789Z", + blobID: "blob-frac-9", + }, + } +} + +func TestRevisionTimestampsFollowWriterContract(t *testing.T) { + for _, c := range revisionTimestampCases() { + t.Run(c.name, func(t *testing.T) { + require.Equal(t, c.want, c.at.UTC().Format(time.RFC3339Nano)) + }) + } +} + +const ( + revisionFixtureSpaceID = "8f638374-6ea8-4f0d-80c4-66d9b49830a5" + revisionFixtureNodeID = "aa111111-1111-4111-8111-111111111111" + revisionFixtureOtherID = "bb222222-2222-4222-8222-222222222222" +) + +// purgedBlob is the SpaceID and BlobID pair passed to the blobstore. +type purgedBlob struct { + spaceID string + blobID string +} + +// decomposedRevisionTree is a temporary decomposedfs tree: +// +// /spaces///nodes///// +// /spaces///nodes/////.REV. +// +// plus the .mpk and .mlock companion of every revision. +type decomposedRevisionTree struct { + root string + revisions int + revisionPaths []string + preservedPaths []string + blobs []purgedBlob +} + +type revisionDiscoverer struct { + name string + find func(root string) <-chan string +} + +// revisionDiscoverers matches the mechanisms revisions purge can run, plus Walk. +func revisionDiscoverers() []revisionDiscoverer { + return []revisionDiscoverer{ + { + name: "glob", + find: func(root string) <-chan string { + return Glob(filepath.Join(root, "spaces", "*", "*", "nodes", "*", "*", "*", "*", "*")) + }, + }, + { + name: "globWorkers", + find: func(root string) <-chan string { + return GlobWorkers(filepath.Join(root, "spaces", "*", "*", "nodes"), "/*", "/*/*/*/*") + }, + }, + { + name: "walk", + find: func(root string) <-chan string { + return Walk(root) + }, + }, + { + name: "list", + find: func(root string) <-chan string { + return List(filepath.Join(root, "spaces"), 10) + }, + }, + } +} + +func decomposedNodePath(root, spaceID, nodeID string) string { + return filepath.Join(root, "spaces", lookup.Pathify(spaceID, 1, 2), "nodes", lookup.Pathify(nodeID, 4, 2)) +} + +func revisionBlobMPK(t *testing.T, blobID string) []byte { + t.Helper() + b, err := msgpack.Marshal(map[string][]byte{"user.ocis.blobid": []byte(blobID)}) + require.NoError(t, err) + return b +} + +func writeFixtureFile(t *testing.T, path string, content []byte) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, content, 0o644)) +} + +// newDecomposedRevisionTree builds one space with a current node, an unrelated +// node, ordinary files, and one revision triplet per writer-contract timestamp. +func newDecomposedRevisionTree(t *testing.T) decomposedRevisionTree { + t.Helper() + + root := t.TempDir() + nodePath := decomposedNodePath(root, revisionFixtureSpaceID, revisionFixtureNodeID) + otherPath := decomposedNodePath(root, revisionFixtureSpaceID, revisionFixtureOtherID) + + writeFixtureFile(t, nodePath, nil) + writeFixtureFile(t, nodePath+".mpk", revisionBlobMPK(t, "blob-current")) + writeFixtureFile(t, otherPath, nil) + + notes := filepath.Join(filepath.Dir(nodePath), "notes.txt") + writeFixtureFile(t, notes, []byte("unrelated")) + trashStamp := time.Date(2024, 5, 22, 7, 32, 53, 0, time.UTC).UTC().Format(time.RFC3339Nano) + trash := nodePath + ".T." + trashStamp + writeFixtureFile(t, trash, nil) + readme := filepath.Join(root, "README.txt") + writeFixtureFile(t, readme, []byte("unrelated")) + ignored := filepath.Join(root, "spaces", "ignored.txt") + writeFixtureFile(t, ignored, []byte("unrelated")) + + tree := decomposedRevisionTree{ + root: root, + preservedPaths: []string{ + nodePath, + nodePath + ".mpk", + otherPath, + notes, + trash, + readme, + ignored, + }, + } + + for _, c := range revisionTimestampCases() { + stamp := c.at.UTC().Format(time.RFC3339Nano) + base := nodePath + ".REV." + stamp + writeFixtureFile(t, base, nil) + writeFixtureFile(t, base+".mpk", revisionBlobMPK(t, c.blobID)) + writeFixtureFile(t, base+".mlock", nil) + + tree.revisions++ + tree.revisionPaths = append(tree.revisionPaths, base, base+".mpk", base+".mlock") + tree.blobs = append(tree.blobs, purgedBlob{spaceID: revisionFixtureSpaceID, blobID: c.blobID}) + } + + sort.Strings(tree.revisionPaths) + sort.Slice(tree.blobs, func(i, j int) bool { + if tree.blobs[i].blobID != tree.blobs[j].blobID { + return tree.blobs[i].blobID < tree.blobs[j].blobID + } + return tree.blobs[i].spaceID < tree.blobs[j].spaceID + }) + return tree +} + +// drainRevisionPaths reads a discovery channel to completion. GlobWorkers and +// List send from several goroutines, so callers compare the sorted result. +func drainRevisionPaths(ch <-chan string) []string { + paths := make([]string, 0) + for p := range ch { + paths = append(paths, p) + } + sort.Strings(paths) + return paths +} + +func TestDiscoverRevisionsWholeSecondAndFractional(t *testing.T) { + tree := newDecomposedRevisionTree(t) + + for _, d := range revisionDiscoverers() { + t.Run(d.name, func(t *testing.T) { + got := drainRevisionPaths(d.find(tree.root)) + require.Equal(t, tree.revisionPaths, got) + }) + } +}