fix(decomposedfs): [OCISDEV-817] remove stale trash symlink on partial delete rollback

changelog/unreleased/bugfix-decomposedfs-stale-trash-symlink-on-delete-failure.md

@@ -0,0 +1,13 @@
+Bugfix: Remove stale trash symlink when decomposedfs Delete fails mid-operation
+
+`Tree.Delete` creates a trash symlink before renaming the node. If any
+subsequent step failed (`os.Rename`, `MetadataBackend().Rename`, or
+`os.Remove` of the parent-dir entry) the symlink was not removed during
+rollback. The dangling symlink permanently blocked future delete attempts
+on the same node with a `file exists` error on the next `os.Symlink` call.
+
+All three failure paths now remove the trash symlink as part of rollback.
+The third path additionally reverts the node rename and metadata rename in
+reverse order.
+
+https://github.com/owncloud/reva/pull/723

pkg/storage/utils/decomposedfs/tree/tree.go

@@ -507,14 +507,13 @@ func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) {
 	trashPath := nodePath + node.TrashIDDelimiter + deletionTime
 	err = os.Rename(nodePath, trashPath)
 	if err != nil {
-		// To roll back changes
-		// TODO remove symlink
-		// Roll back changes
+		_ = os.Remove(trashLink)
 		_ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true)
 		return
 	}
 	err = t.lookup.MetadataBackend().Rename(nodePath, trashPath)
 	if err != nil {
+		_ = os.Remove(trashLink)
 		_ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true)
 		_ = os.Rename(trashPath, nodePath)
 		return
@@ -525,10 +524,9 @@ func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) {
 
 	// finally remove the entry from the parent dir
 	if err = os.Remove(path); err != nil {
-		// To roll back changes
-		// TODO revert the rename
-		// TODO remove symlink
-		// Roll back changes
+		_ = t.lookup.MetadataBackend().Rename(trashPath, nodePath)
+		_ = os.Rename(trashPath, nodePath)
+		_ = os.Remove(trashLink)
 		_ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true)
 		return
 	}

pkg/storage/utils/decomposedfs/tree/tree_test.go

@@ -19,23 +19,33 @@
 package tree_test
 
 import (
+	"errors"
 	"os"
 	"path"
 	"path/filepath"
 
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	"github.com/google/uuid"
+	"github.com/rs/zerolog"
+	"github.com/owncloud/reva/v2/pkg/storage/fs/posix/timemanager"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/lookup"
+	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node"
 	helpers "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/testhelpers"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree"
+	"github.com/owncloud/reva/v2/pkg/store"
 	"github.com/stretchr/testify/mock"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
+// renameFailBackend wraps a metadata.Backend and injects a failure on Rename (OCISDEV-817)
+type renameFailBackend struct{ metadata.Backend }
+
+func (b renameFailBackend) Rename(_, _ string) error { return errors.New("injected rename error") }
+
 var _ = Describe("Tree", func() {
 	var (
 		env *helpers.TestEnv
@@ -135,6 +145,63 @@ var _ = Describe("Tree", func() {
 					env.Blobstore.AssertNotCalled(GinkgoT(), "Delete", mock.AnythingOfType("*node.Node"))
 				})
 			})
+
+			// OCISDEV-817: trash symlink must be removed when MetadataBackend.Rename fails mid-delete.
+			// This is the exact path observed in the SE-1149 production incident.
+			Context("when MetadataBackend.Rename fails", func() {
+				It("rolls back the trash symlink", func() {
+					log := &zerolog.Logger{}
+					lu := lookup.New(renameFailBackend{env.Lookup.MetadataBackend()}, env.Options, &timemanager.Manager{})
+					failTree := tree.New(lu, env.Blobstore, env.Options, store.Create(), log)
+
+					file, err := lu.NodeFromResource(env.Ctx, &provider.Reference{
+						ResourceId: env.SpaceRootRes,
+						Path:       "dir1/file1",
+					})
+					Expect(err).ToNot(HaveOccurred())
+
+					Expect(failTree.Delete(env.Ctx, file)).To(HaveOccurred())
+
+					trashLink := path.Join(env.Root, "spaces", lookup.Pathify(file.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(file.ID, 4, 2))
+					_, statErr := os.Lstat(trashLink)
+					Expect(os.IsNotExist(statErr)).To(BeTrue())
+				})
+			})
+
+			// OCISDEV-817: trash symlink must be removed when os.Rename fails mid-delete.
+			// chmod 0500 on the node's parent dir blocks rename but allows xattr writes (tested on macOS + Linux).
+			Context("when os.Rename fails", func() {
+				It("rolls back the trash symlink", func() {
+					nodeDir := filepath.Dir(n.InternalPath())
+					Expect(os.Chmod(nodeDir, 0o500)).To(Succeed())
+					defer os.Chmod(nodeDir, 0o700)
+
+					Expect(t.Delete(env.Ctx, n)).To(HaveOccurred())
+
+					trashLink := path.Join(env.Root, "spaces", lookup.Pathify(n.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(n.ID, 4, 2))
+					_, statErr := os.Lstat(trashLink)
+					Expect(os.IsNotExist(statErr)).To(BeTrue())
+				})
+			})
+
+			// OCISDEV-817: trash symlink and node rename must be rolled back when removing
+			// the parent-dir entry fails (path #3). chmod 0500 on n.ParentPath() blocks
+			// os.Remove(path) while all prior steps succeed.
+			Context("when os.Remove of parent entry fails", func() {
+				It("rolls back the trash symlink and node rename", func() {
+					Expect(os.Chmod(n.ParentPath(), 0o500)).To(Succeed())
+					defer os.Chmod(n.ParentPath(), 0o700)
+
+					Expect(t.Delete(env.Ctx, n)).To(HaveOccurred())
+
+					trashLink := path.Join(env.Root, "spaces", lookup.Pathify(n.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(n.ID, 4, 2))
+					_, statErr := os.Lstat(trashLink)
+					Expect(os.IsNotExist(statErr)).To(BeTrue())
+
+					_, err := os.Stat(n.InternalPath())
+					Expect(err).ToNot(HaveOccurred())
+				})
+			})
 		})
 
 		Context("that was deleted", func() {