Merge branch 'smh-vote-on-committed' into 'master'

Vote when reference transaction has been committed

See merge request gitlab-org/gitaly!3514

10 files changed, 45 insertions, 22 deletions

diff --git a/internal/gitaly/hook/referencetransaction.go b/internal/gitaly/hook/referencetransaction.go
index 1028ab367..e2e23834b 100644
--- a/internal/gitaly/hook/referencetransaction.go
+++ b/internal/gitaly/hook/referencetransaction.go
@@ -29,10 +29,13 @@ func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state
 		return fmt.Errorf("reading stdin from request: %w", err)
 	}
 
-	// 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 {
+	// We're voting in prepared state as this is the only stage in Git's reference transaction
+	// which allows us to abort the transaction. We're also voting in committed state to tell
+	// Praefect we've actually persisted the changes. This is necessary as some RPCs fail return
+	// errors in the response body rather than as an error code. Praefect can't tell if these RPCs
+	// have failed. Voting on committed ensure Praefect sees either a missing vote or that the RPC did
+	// commit the changes.
+	if state != ReferenceTransactionPrepared && state != ReferenceTransactionCommitted {
 		return nil
 	}
 
diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go
index 4be1ee951..909c0b974 100644
--- a/internal/gitaly/service/hook/reference_transaction_test.go
+++ b/internal/gitaly/service/hook/reference_transaction_test.go
@@ -81,10 +81,11 @@ func TestReferenceTransactionHook(t *testing.T) {
 			expectedCode: codes.OK,
 		},
 		{
-			desc:         "hook does not trigger transaction with committed state",
-			stdin:        []byte("foobar"),
-			state:        gitalypb.ReferenceTransactionHookRequest_COMMITTED,
-			expectedCode: codes.OK,
+			desc:              "hook triggers transaction with committed state",
+			stdin:             []byte("foobar"),
+			state:             gitalypb.ReferenceTransactionHookRequest_COMMITTED,
+			expectedCode:      codes.OK,
+			expectedReftxHash: []byte("foobar"),
 		},
 		{
 			desc:              "hook fails with failed vote",
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 0b8a7c6f6..76c2394c6 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -207,7 +207,7 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 			response, err := client.UserCreateBranch(ctx, request)
 			require.NoError(t, err)
 			require.Empty(t, response.PreReceiveError)
-			require.Equal(t, 1, transactionServer.called)
+			require.Equal(t, 2, transactionServer.called)
 		})
 	}
 }
@@ -547,7 +547,7 @@ func TestUserDeleteBranch_transaction(t *testing.T) {
 		User:       gittest.TestUser,
 	})
 	require.NoError(t, err)
-	require.Equal(t, 1, transactionServer.called)
+	require.Equal(t, 2, transactionServer.called)
 }
 
 func TestFailedUserDeleteBranchDueToValidation(t *testing.T) {
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index d34d55946..7e48da3d4 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -128,14 +128,14 @@ func testUserRebaseConfirmableTransactionFeatured(t *testing.T, ctx context.Cont
 			desc:                 "primary votes and executes hook",
 			withTransaction:      true,
 			primary:              true,
-			expectedVotes:        1,
+			expectedVotes:        2,
 			expectPreReceiveHook: true,
 		},
 		{
 			desc:                 "secondary votes but does not execute hook",
 			withTransaction:      true,
 			primary:              false,
-			expectedVotes:        1,
+			expectedVotes:        2,
 			expectPreReceiveHook: false,
 		},
 	} {
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index af4f79f5f..a218ad75b 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -340,6 +340,8 @@ func TestUserCreateTagWithTransaction(t *testing.T) {
 		},
 	} {
 		t.Run(testCase.desc, func(t *testing.T) {
+			*transactionServer = testTransactionServer{}
+
 			if err := os.Remove(hooksOutputPath); err != nil {
 				require.True(t, os.IsNotExist(err), "error when cleaning up work area: %v", err)
 			}
@@ -395,7 +397,7 @@ func TestUserCreateTagWithTransaction(t *testing.T) {
 				require.NoFileExists(t, hooksOutputPath)
 			}
 
-			require.Equal(t, 1, transactionServer.called)
+			require.Equal(t, 2, transactionServer.called)
 			transactionServer.called = 0
 		})
 	}
diff --git a/internal/gitaly/service/operations/update_with_hooks.go b/internal/gitaly/service/operations/update_with_hooks.go
index b8c8a2935..0cd9ce09c 100644
--- a/internal/gitaly/service/operations/update_with_hooks.go
+++ b/internal/gitaly/service/operations/update_with_hooks.go
@@ -121,6 +121,10 @@ func (s *Server) updateReferenceWithHooks(
 		return updateRefError{reference: reference.String()}
 	}
 
+	if err := s.hookManager.ReferenceTransactionHook(ctx, hook.ReferenceTransactionCommitted, env, strings.NewReader(changes)); err != nil {
+		return preReceiveError{message: err.Error()}
+	}
+
 	if err := s.hookManager.PostReceiveHook(ctx, repo, pushOptions, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
 		msg := hookErrorMessage(stdout.String(), stderr.String(), err)
 		return preReceiveError{message: msg}
diff --git a/internal/gitaly/service/operations/update_with_hooks_test.go b/internal/gitaly/service/operations/update_with_hooks_test.go
index b93438e93..8cd59d1af 100644
--- a/internal/gitaly/service/operations/update_with_hooks_test.go
+++ b/internal/gitaly/service/operations/update_with_hooks_test.go
@@ -145,6 +145,7 @@ func TestUpdateReferenceWithHooks(t *testing.T) {
 		payload,
 	}
 
+	referenceTransactionCalls := 0
 	testCases := []struct {
 		desc                 string
 		preReceive           func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error
@@ -183,7 +184,15 @@ func TestUpdateReferenceWithHooks(t *testing.T) {
 				changes, err := ioutil.ReadAll(stdin)
 				require.NoError(t, err)
 				require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ZeroOID.String()), string(changes))
-				require.Equal(t, state, hook.ReferenceTransactionPrepared)
+
+				require.Less(t, referenceTransactionCalls, 2)
+				if referenceTransactionCalls == 0 {
+					require.Equal(t, state, hook.ReferenceTransactionPrepared)
+				} else {
+					require.Equal(t, state, hook.ReferenceTransactionCommitted)
+				}
+				referenceTransactionCalls++
+
 				require.Equal(t, env, expectedEnv)
 				return nil
 			},
@@ -255,6 +264,7 @@ func TestUpdateReferenceWithHooks(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
+		referenceTransactionCalls = 0
 		t.Run(tc.desc, func(t *testing.T) {
 			hookManager := &mockHookManager{
 				t:                    t,
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 7743ddfa6..68e7ae483 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -634,7 +634,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, 2, refTransactionServer.called)
+		require.Equal(t, 4, refTransactionServer.called)
 	})
 
 	t.Run("delete", func(t *testing.T) {
@@ -662,6 +662,6 @@ 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, 1, refTransactionServer.called)
+		require.Equal(t, 2, refTransactionServer.called)
 	})
 }
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index a64315c25..fdfd48c4a 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -316,7 +316,7 @@ func TestReceivePackTransactional(t *testing.T) {
 			expectedRefs: map[string]string{
 				"refs/heads/master": masterOID,
 			},
-			expectedVotes: 2,
+			expectedVotes: 3,
 		},
 		{
 			desc:          "update",
@@ -331,7 +331,7 @@ func TestReceivePackTransactional(t *testing.T) {
 			expectedRefs: map[string]string{
 				"refs/heads/master": masterParentOID,
 			},
-			expectedVotes: 2,
+			expectedVotes: 3,
 		},
 		{
 			desc:          "creation",
@@ -346,7 +346,7 @@ func TestReceivePackTransactional(t *testing.T) {
 			expectedRefs: map[string]string{
 				"refs/heads/other": masterOID,
 			},
-			expectedVotes: 2,
+			expectedVotes: 3,
 		},
 		{
 			desc: "deletion",
@@ -360,7 +360,7 @@ func TestReceivePackTransactional(t *testing.T) {
 			expectedRefs: map[string]string{
 				"refs/heads/other": git.ZeroOID.String(),
 			},
-			expectedVotes: 2,
+			expectedVotes: 3,
 		},
 		{
 			desc:          "multiple commands",
@@ -381,7 +381,7 @@ func TestReceivePackTransactional(t *testing.T) {
 				"refs/heads/a": masterOID,
 				"refs/heads/b": masterOID,
 			},
-			expectedVotes: 3,
+			expectedVotes: 5,
 		},
 		{
 			desc:          "refused recreation of branch",
@@ -416,7 +416,7 @@ func TestReceivePackTransactional(t *testing.T) {
 			expectedRefs: map[string]string{
 				"refs/heads/a": masterOID,
 			},
-			expectedVotes: 2,
+			expectedVotes: 3,
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/ruby/lib/gitlab/git/hooks_service.rb b/ruby/lib/gitlab/git/hooks_service.rb
index 67ac4c3fb..baccaa5c0 100644
--- a/ruby/lib/gitlab/git/hooks_service.rb
+++ b/ruby/lib/gitlab/git/hooks_service.rb
@@ -20,6 +20,9 @@ module Gitlab
         end
 
         yield(self).tap do
+          status, message = run_hook('reference-transaction')
+          Gitlab::GitLogger.error("reference-transaction committed hook: #{message}") unless status
+
           status, message = run_hook('post-receive')
 
           Gitlab::GitLogger.error("post-receive hook: #{message}") unless status