transactions: Only vote when reftx hook is in prepared state

For each Git reference transaction, the reference-transaction hook is
going to be invoked two times: once when it moves into prepared state,
meaning all references have now been locked. And then a second time if
the transaction was either committed or aborted.

The hook service is getting executed for both of these state
transitions, and as a result we perform two transactional votes. While
it's not bad in and off itself, it doesn't make a lot of sense as we can
only influence Git's transaction in the "prepared" state anyway. As a
result, the second vote is essentially useless to us without anything
like a three-phase commit and only incurs another roundtrip to Praefect.

This commit thus changes the logic to only perform a transactional vote
in case we're moving into "prepared" state.

4 files changed, 36 insertions, 2 deletions

diff --git a/changelogs/unreleased/pks-reftx-dup-votes.yml b/changelogs/unreleased/pks-reftx-dup-votes.yml
new file mode 100644
index 000000000..c4cffd264
--- /dev/null
+++ b/changelogs/unreleased/pks-reftx-dup-votes.yml
@@ -0,0 +1,5 @@
+---
+title: 'transactions: Only vote when reftx hook is in prepared state'
+merge_request: 2571
+author:
+type: changed
diff --git a/internal/gitaly/hook/referencetransaction.go b/internal/gitaly/hook/referencetransaction.go
index 7128a2e11..0b899d21c 100644
--- a/internal/gitaly/hook/referencetransaction.go
+++ b/internal/gitaly/hook/referencetransaction.go
@@ -10,6 +10,13 @@ import (
 )
 
 func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state ReferenceTransactionState, env []string, stdin io.Reader) error {
+	// We're only voting in prepared state as this is the only stage in
+	// Git's reference transaction which allows us to abort the
+	// transaction.
+	if state != ReferenceTransactionPrepared {
+		return nil
+	}
+
 	changes, err := ioutil.ReadAll(stdin)
 	if err != nil {
 		return helper.ErrInternalf("reading stdin from request: %w", err)
diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go
index 67ce79d8d..c7052f4fc 100644
--- a/internal/gitaly/service/hook/reference_transaction_test.go
+++ b/internal/gitaly/service/hook/reference_transaction_test.go
@@ -50,18 +50,39 @@ func TestReferenceTransactionHook(t *testing.T) {
 	testCases := []struct {
 		desc              string
 		stdin             []byte
+		state             gitalypb.ReferenceTransactionHookRequest_State
 		voteResponse      gitalypb.VoteTransactionResponse_TransactionState
 		expectedCode      codes.Code
 		expectedReftxHash []byte
 	}{
 		{
-			desc:              "hook triggers transaction",
+			desc:              "hook triggers transaction with default state",
 			stdin:             []byte("foobar"),
 			voteResponse:      gitalypb.VoteTransactionResponse_COMMIT,
 			expectedCode:      codes.OK,
 			expectedReftxHash: []byte("foobar"),
 		},
 		{
+			desc:              "hook triggers transaction with explicit prepared state",
+			stdin:             []byte("foobar"),
+			state:             gitalypb.ReferenceTransactionHookRequest_PREPARED,
+			voteResponse:      gitalypb.VoteTransactionResponse_COMMIT,
+			expectedCode:      codes.OK,
+			expectedReftxHash: []byte("foobar"),
+		},
+		{
+			desc:         "hook does not trigger transaction with aborted state",
+			stdin:        []byte("foobar"),
+			state:        gitalypb.ReferenceTransactionHookRequest_ABORTED,
+			expectedCode: codes.OK,
+		},
+		{
+			desc:         "hook does not trigger transaction with committed state",
+			stdin:        []byte("foobar"),
+			state:        gitalypb.ReferenceTransactionHookRequest_COMMITTED,
+			expectedCode: codes.OK,
+		},
+		{
 			desc:              "hook fails with failed vote",
 			stdin:             []byte("foobar"),
 			voteResponse:      gitalypb.VoteTransactionResponse_ABORT,
@@ -130,6 +151,7 @@ func TestReferenceTransactionHook(t *testing.T) {
 			require.NoError(t, err)
 			require.NoError(t, stream.Send(&gitalypb.ReferenceTransactionHookRequest{
 				Repository:           testRepo,
+				State:                tc.state,
 				EnvironmentVariables: environment,
 			}))
 			require.NoError(t, stream.Send(&gitalypb.ReferenceTransactionHookRequest{
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index eb3dbbfee..0f5793844 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -648,7 +648,7 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) {
 			if features.IsDisabled(featureflag.ReferenceTransactionHook) || !supported {
 				require.Equal(t, 0, refTransactionServer.called)
 			} else {
-				require.Equal(t, 4, refTransactionServer.called)
+				require.Equal(t, 2, refTransactionServer.called)
 			}
 		})
 	}