smarthttp: Add finalizing vote in PostReceivePackjc-finalizing-vote-post-receive-pack

When all updates are rejected, there will be no votes cast. This leads
to an unnecessary replication job. In this case, we want to cast a last
but empty vote. This is the same as how SSHReceivePack works.

Changelog: changed

2 files changed, 83 insertions, 2 deletions

diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index 7e9f5776f..1653594bc 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -1,10 +1,14 @@
 package smarthttp
 
 import (
+	"errors"
+
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v14/streamio"
 	"google.golang.org/grpc/codes"
@@ -67,6 +71,27 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err)
 	}
 
+	// In cases where all reference updates are rejected by git-receive-pack(1), we would end up
+	// with no transactional votes at all. This would lead to scheduling
+	// replication jobs, which wouldn't accomplish anything since no refs
+	// were updated.
+	// To prevent replication jobs from being unnecessarily created, do a
+	// final vote which concludes this RPC to ensure there's always at least
+	// one vote. In case there was diverging behaviour in git-receive-pack(1)
+	// which led to a different outcome across voters, then this final vote
+	// would fail because the sequence of votes would be different.
+	if err := transaction.VoteOnContext(ctx, s.txManager, voting.Vote{}, voting.Committed); err != nil {
+		// When the pre-receive hook failed, git-receive-pack(1) exits with code 0.
+		// It's arguable whether this is the expected behavior, but anyhow it means
+		// cmd.Wait() did not error out. On the other hand, the gitaly-hooks command did
+		// stop the transaction upon failure. So this final vote fails.
+		// To avoid this error being presented to the end user, ignore it when the
+		// transaction was stopped.
+		if !errors.Is(err, transaction.ErrTransactionStopped) {
+			return status.Errorf(codes.Aborted, "final transactional vote: %v", err)
+		}
+	}
+
 	return nil
 }
 
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 608cfd57f..e7349ac80 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -3,6 +3,7 @@ package smarthttp
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -17,6 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	gitalyhook "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitlab"
@@ -789,7 +791,7 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) {
 
 		expectedResponse := "0049\x01000eunpack ok\n0019ok refs/heads/master\n0019ok refs/heads/branch\n00000000"
 		require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response)
-		require.Equal(t, 4, refTransactionServer.called)
+		require.Equal(t, 5, refTransactionServer.called)
 	})
 
 	t.Run("delete", func(t *testing.T) {
@@ -817,6 +819,60 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) {
 
 		expectedResponse := "0033\x01000eunpack ok\n001cok refs/heads/delete-me\n00000000"
 		require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response)
-		require.Equal(t, 2, refTransactionServer.called)
+		require.Equal(t, 3, refTransactionServer.called)
 	})
 }
+
+func TestPostReceive_allRejected(t *testing.T) {
+	t.Parallel()
+
+	cfg := testcfg.Build(t)
+
+	testcfg.BuildGitalyHooks(t, cfg)
+
+	refTransactionServer := &testTransactionServer{}
+
+	hookManager := gitalyhook.NewMockManager(
+		t,
+		func(
+			t *testing.T,
+			ctx context.Context,
+			repo *gitalypb.Repository,
+			pushOptions, env []string,
+			stdin io.Reader, stdout, stderr io.Writer) error {
+			return errors.New("not allowed")
+		},
+		gitalyhook.NopPostReceive,
+		gitalyhook.NopUpdate,
+		gitalyhook.NopReferenceTransaction,
+	)
+	addr := testserver.RunGitalyServer(t, cfg, nil, func(srv *grpc.Server, deps *service.Dependencies) {
+		gitalypb.RegisterSmartHTTPServiceServer(srv, NewServer(
+			deps.GetLocator(),
+			deps.GetGitCmdFactory(),
+			deps.GetTxManager(),
+			deps.GetDiskCache(),
+		))
+		gitalypb.RegisterHookServiceServer(srv, hook.NewServer(deps.GetHookManager(), deps.GetGitCmdFactory(), deps.GetPackObjectsCache()))
+	}, testserver.WithDisablePraefect(), testserver.WithHookManager(hookManager))
+
+	ctx, err := txinfo.InjectTransaction(testhelper.Context(t), 1234, "primary", true)
+	require.NoError(t, err)
+	ctx = metadata.IncomingToOutgoing(ctx)
+
+	client := newMuxedSmartHTTPClient(t, ctx, addr, cfg.Auth.Token, func() backchannel.Server {
+		srv := grpc.NewServer()
+		gitalypb.RegisterRefTransactionServer(srv, refTransactionServer)
+		return srv
+	})
+
+	stream, err := client.PostReceivePack(ctx)
+	require.NoError(t, err)
+
+	repo, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+
+	request := &gitalypb.PostReceivePackRequest{Repository: repo, GlId: "key-1234", GlRepository: "some_repo"}
+	doPush(t, stream, request, newTestPush(t, cfg, nil).body)
+
+	require.Equal(t, 1, refTransactionServer.called)
+}