repository: Support transactional voting in `RemoveRepository()`

The `RemoveRepository()` RPC currently does not support transactional
voting. This is now implemented by doing a vote both previous to
removing the repository and after the repository either has been removed
or found to not exist.

Transactional behaviour is not yet enabled in Praefect and thus this new
logic isn't used. This will be handled in a subsequent commit.

Changelog: added

2 files changed, 97 insertions, 3 deletions

diff --git a/internal/gitaly/service/repository/remove.go b/internal/gitaly/service/repository/remove.go
index d40fca17a..aa0b2b123 100644
--- a/internal/gitaly/service/repository/remove.go
+++ b/internal/gitaly/service/repository/remove.go
@@ -2,11 +2,16 @@ package repository
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/tempdir"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -31,16 +36,51 @@ func (s *server) RemoveRepository(ctx context.Context, in *gitalypb.RemoveReposi
 	base := filepath.Base(path)
 	destDir := filepath.Join(tempDir, base+"+removed")
 
+	if err := s.voteOnAction(ctx, repo, preRemove); err != nil {
+		return nil, helper.ErrInternalf("vote on rename: %v", err)
+	}
+
 	if err := os.Rename(path, destDir); err != nil {
-		if os.IsNotExist(err) {
-			return &gitalypb.RemoveRepositoryResponse{}, nil
+		// Even in the case where the repository doesn't exist will we cast the same
+		// "post-remove" vote as in the case where the repository did exist, but we
+		// removed it just fine. The end result is the same in both cases anyway:
+		// the repository does not exist.
+		if !errors.Is(err, os.ErrNotExist) {
+			return nil, helper.ErrInternal(err)
 		}
-		return nil, helper.ErrInternal(err)
 	}
 
 	if err := os.RemoveAll(destDir); err != nil {
 		return nil, helper.ErrInternal(err)
 	}
 
+	if err := s.voteOnAction(ctx, repo, postRemove); err != nil {
+		return nil, helper.ErrInternalf("vote on finalizing: %v", err)
+	}
+
 	return &gitalypb.RemoveRepositoryResponse{}, nil
 }
+
+type removalStep int
+
+const (
+	preRemove = removalStep(iota)
+	postRemove
+)
+
+func (s *server) voteOnAction(ctx context.Context, repo *gitalypb.Repository, step removalStep) error {
+	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error {
+		var voteStep string
+		switch step {
+		case preRemove:
+			voteStep = "pre-remove"
+		case postRemove:
+			voteStep = "post-remove"
+		default:
+			return fmt.Errorf("invalid removal step: %d", step)
+		}
+
+		vote := fmt.Sprintf("%s %s", voteStep, repo.GetRelativePath())
+		return s.txManager.Vote(ctx, tx, voting.VoteFromData([]byte(vote)))
+	})
+}
diff --git a/internal/gitaly/service/repository/remove_test.go b/internal/gitaly/service/repository/remove_test.go
index d201787b5..d8289b4fb 100644
--- a/internal/gitaly/service/repository/remove_test.go
+++ b/internal/gitaly/service/repository/remove_test.go
@@ -1,10 +1,17 @@
 package repository
 
 import (
+	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -32,3 +39,50 @@ func TestRemoveRepositoryDoesNotExist(t *testing.T) {
 		Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "/does/not/exist"}})
 	require.NoError(t, err)
 }
+
+func TestRemoveRepositoryTransactional(t *testing.T) {
+	var votes []voting.Vote
+	txManager := transaction.MockManager{
+		VoteFn: func(ctx context.Context, tx txinfo.Transaction, vote voting.Vote) error {
+			votes = append(votes, vote)
+			return nil
+		},
+	}
+
+	cfg, repo, repoPath, client := setupRepositoryService(t, testserver.WithTransactionManager(&txManager))
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	ctx, err := txinfo.InjectTransaction(ctx, 1, "primary", true)
+	require.NoError(t, err)
+	ctx = helper.IncomingToOutgoing(ctx)
+
+	t.Run("with existing repository", func(t *testing.T) {
+		votes = []voting.Vote{}
+
+		_, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{Repository: repo})
+		require.NoError(t, err)
+		require.NoFileExists(t, repoPath)
+
+		require.Equal(t, []voting.Vote{
+			voting.VoteFromData([]byte(fmt.Sprintf("pre-remove %s", repo.GetRelativePath()))),
+			voting.VoteFromData([]byte(fmt.Sprintf("post-remove %s", repo.GetRelativePath()))),
+		}, votes)
+	})
+
+	t.Run("with nonexistent repository", func(t *testing.T) {
+		votes = []voting.Vote{}
+
+		_, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{
+			Repository: &gitalypb.Repository{
+				StorageName: cfg.Storages[0].Name, RelativePath: "/does/not/exist"},
+		},
+		)
+		require.NoError(t, err)
+
+		require.Equal(t, []voting.Vote{
+			voting.VoteFromData([]byte("pre-remove /does/not/exist")),
+			voting.VoteFromData([]byte("post-remove /does/not/exist")),
+		}, votes)
+	})
+}