Merge branch 'smh-deletion-unique-path' into 'master'

Remove repository even if there is a failed deletion

Closes #5422

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6043

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

2 files changed, 48 insertions, 8 deletions

diff --git a/internal/gitaly/repoutil/remove.go b/internal/gitaly/repoutil/remove.go
index de3ca7822..0ce2cd8c9 100644
--- a/internal/gitaly/repoutil/remove.go
+++ b/internal/gitaly/repoutil/remove.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
@@ -23,6 +24,16 @@ func Remove(
 	txManager transaction.Manager,
 	repository storage.Repository,
 ) error {
+	return remove(ctx, locator, txManager, repository, os.RemoveAll)
+}
+
+func remove(
+	ctx context.Context,
+	locator storage.Locator,
+	txManager transaction.Manager,
+	repository storage.Repository,
+	removeAll func(string) error,
+) error {
 	path, err := locator.GetRepoPath(repository, storage.WithRepositoryVerificationSkipped())
 	if err != nil {
 		return structerr.NewInternal("%w", err)
@@ -37,9 +48,6 @@ func Remove(
 		return structerr.NewInternal("%w", err)
 	}
 
-	base := filepath.Base(path)
-	destDir := filepath.Join(tempDir, base+"+removed")
-
 	// Check whether the repository exists. If not, then there is nothing we can
 	// remove. Historically, we didn't return an error in this case, which was just
 	// plain bad RPC design: callers should be able to act on this, and if they don't
@@ -77,10 +85,21 @@ func Remove(
 		return structerr.NewInternal("vote on rename: %w", err)
 	}
 
+	destDir, err := os.MkdirTemp(tempDir, filepath.Base(path)+"+removed-*")
+	if err != nil {
+		return fmt.Errorf("mkdir temp: %w", err)
+	}
+
+	defer func() {
+		if err := removeAll(destDir); err != nil {
+			ctxlogrus.Extract(ctx).WithError(err).Error("failed removing repository from temporary directory")
+		}
+	}()
+
 	// We move the repository into our temporary directory first before we start to
 	// delete it. This is done such that we don't leave behind a partially-removed and
 	// thus likely corrupt repository.
-	if err := os.Rename(path, destDir); err != nil {
+	if err := os.Rename(path, filepath.Join(destDir, "repo")); err != nil {
 		return structerr.NewInternal("staging repository for removal: %w", err)
 	}
 
@@ -88,10 +107,6 @@ func Remove(
 		return fmt.Errorf("sync removal: %w", err)
 	}
 
-	if err := os.RemoveAll(destDir); err != nil {
-		return structerr.NewInternal("removing repository: %w", err)
-	}
-
 	if err := voteOnAction(ctx, txManager, repository, voting.Committed); err != nil {
 		return structerr.NewInternal("vote on finalizing: %w", err)
 	}
diff --git a/internal/gitaly/repoutil/remove_test.go b/internal/gitaly/repoutil/remove_test.go
index 0cbc158f1..19a61ab82 100644
--- a/internal/gitaly/repoutil/remove_test.go
+++ b/internal/gitaly/repoutil/remove_test.go
@@ -58,6 +58,31 @@ func TestRemove(t *testing.T) {
 			},
 			expectedErr: structerr.NewFailedPrecondition("repository is already locked"),
 		},
+		{
+			desc: "unfinished deletion doesn't fail subsequent deletions",
+			createRepo: func(tb testing.TB, ctx context.Context, cfg config.Cfg) (*gitalypb.Repository, string) {
+				repo, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+					SkipCreationViaService: true,
+				})
+
+				require.NoError(t,
+					remove(
+						ctx,
+						config.NewLocator(cfg),
+						transaction.NewTrackingManager(),
+						repo,
+						// Pass a no-op removeAll to leave the temporary directory in place. Previous unfinished
+						// deletion for the same relative path should not cause any conflicts.
+						func(string) error { return nil },
+					),
+				)
+
+				return gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+					SkipCreationViaService: true,
+					RelativePath:           repo.RelativePath,
+				})
+			},
+		},
 	} {
 		tc := tc