hook: Stop transactions when post-receive or update hooks fail

The pre-receive, post-receive and update hooks all have logic which will
only run on the primary Gitaly node if transactions are in use. This
includes the access checks for the pre-receive hook, but also the
custom hooks logic implemented for all hooks. Executing custom hooks
multiple times for a single change may break current assumptions about
how they work in a single-node setup, so we shouldn't execute them
multiple times in a Praefect setup either.

As only the primary will execute this logic, secondary nodes in a
transaction wouldn't know about any potential failures. We have thus
implmented a mechanism to gracefully stop transactions in 0d2ea9ff5
(transactions: Allow graceful stop of transactions, 2020-09-25). But
right now, this logic is only executed for the pre-receive hook.
Considering that primary-only failures can also happen for the
post-receive and update hooks, this doesn't make a lot of sense.

Let's fix this by stopping transactions on failure of the primary for
the post-receive and update hooks, too. While at it, this also unifies
error messages when executing custom hooks.

4 files changed, 137 insertions, 2 deletions

diff --git a/changelogs/unreleased/pks-hooks-stop-transactions.yml b/changelogs/unreleased/pks-hooks-stop-transactions.yml
new file mode 100644
index 000000000..6cbf5acd6
--- /dev/null
+++ b/changelogs/unreleased/pks-hooks-stop-transactions.yml
@@ -0,0 +1,5 @@
+---
+title: 'hook: Stop transactions when post-receive and update hooks fail'
+merge_request: 3094
+author:
+type: fixed
diff --git a/internal/gitaly/hook/postreceive.go b/internal/gitaly/hook/postreceive.go
index 2b4d1e74c..f42e1db91 100644
--- a/internal/gitaly/hook/postreceive.go
+++ b/internal/gitaly/hook/postreceive.go
@@ -128,6 +128,9 @@ func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb.
 
 	if isPrimary(payload) {
 		if err := m.postReceiveHook(ctx, payload, repo, pushOptions, env, changes, stdout, stderr); err != nil {
+			// If the post-receive hook declines the push, then we need to stop any
+			// secondaries voting on the transaction.
+			m.stopTransaction(ctx, payload)
 			return err
 		}
 	}
@@ -186,7 +189,7 @@ func (m *GitLabHookManager) postReceiveHook(ctx context.Context, payload git.Hoo
 		stdout,
 		stderr,
 	); err != nil {
-		return err
+		return fmt.Errorf("executing custom hooks: %w", err)
 	}
 
 	return nil
diff --git a/internal/gitaly/hook/transactions_test.go b/internal/gitaly/hook/transactions_test.go
new file mode 100644
index 000000000..256e70a7e
--- /dev/null
+++ b/internal/gitaly/hook/transactions_test.go
@@ -0,0 +1,123 @@
+package hook
+
+import (
+	"context"
+	"errors"
+	"io/ioutil"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+type mockTransactionManager struct {
+	stop func(context.Context, metadata.Transaction, metadata.PraefectServer) error
+}
+
+func (m *mockTransactionManager) Vote(context.Context, metadata.Transaction, metadata.PraefectServer, []byte) error {
+	return nil
+}
+
+func (m *mockTransactionManager) Stop(ctx context.Context, tx metadata.Transaction, praefect metadata.PraefectServer) error {
+	return m.stop(ctx, tx, praefect)
+}
+
+func TestHookManager_stopCalled(t *testing.T) {
+	expectedTx := metadata.Transaction{
+		ID: 1234, Node: "primary", Primary: true,
+	}
+
+	expectedPraefect := metadata.PraefectServer{
+		SocketPath: "socket",
+		Token:      "foo",
+	}
+
+	repo, repoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	var mockTxMgr mockTransactionManager
+	hookManager := NewManager(config.NewLocator(config.Config), &mockTxMgr, GitlabAPIStub, config.Config)
+
+	hooksPayload, err := git.NewHooksPayload(
+		config.Config,
+		repo,
+		&expectedTx,
+		&expectedPraefect,
+		&git.ReceiveHooksPayload{
+			UserID:   "1234",
+			Username: "user",
+			Protocol: "web",
+		},
+	).Env()
+	require.NoError(t, err)
+
+	ctx, cleanup := testhelper.Context()
+	defer cleanup()
+
+	for _, hook := range []string{"pre-receive", "update", "post-receive"} {
+		cleanup = testhelper.WriteCustomHook(t, repoPath, hook, []byte("#!/bin/sh\nexit 1\n"))
+		defer cleanup()
+	}
+
+	preReceiveFunc := func(t *testing.T) error {
+		return hookManager.PreReceiveHook(ctx, repo, nil, []string{hooksPayload}, strings.NewReader("changes"), ioutil.Discard, ioutil.Discard)
+	}
+	updateFunc := func(t *testing.T) error {
+		return hookManager.UpdateHook(ctx, repo, "ref", git.ZeroOID.String(), git.ZeroOID.String(), []string{hooksPayload}, ioutil.Discard, ioutil.Discard)
+	}
+	postReceiveFunc := func(t *testing.T) error {
+		return hookManager.PostReceiveHook(ctx, repo, nil, []string{hooksPayload}, strings.NewReader("changes"), ioutil.Discard, ioutil.Discard)
+	}
+
+	for _, tc := range []struct {
+		desc     string
+		hookFunc func(*testing.T) error
+		stopErr  error
+	}{
+		{
+			desc:     "pre-receive gets successfully stopped",
+			hookFunc: preReceiveFunc,
+		},
+		{
+			desc:     "pre-receive with stop error does not clobber real error",
+			hookFunc: preReceiveFunc,
+			stopErr:  errors.New("stop error"),
+		},
+		{
+			desc:     "post-receive gets successfully stopped",
+			hookFunc: postReceiveFunc,
+		},
+		{
+			desc:     "post-receive with stop error does not clobber real error",
+			hookFunc: postReceiveFunc,
+			stopErr:  errors.New("stop error"),
+		},
+		{
+			desc:     "update gets successfully stopped",
+			hookFunc: updateFunc,
+		},
+		{
+			desc:     "update with stop error does not clobber real error",
+			hookFunc: updateFunc,
+			stopErr:  errors.New("stop error"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			wasInvoked := false
+			mockTxMgr.stop = func(ctx context.Context, tx metadata.Transaction, praefect metadata.PraefectServer) error {
+				require.Equal(t, expectedTx, tx)
+				require.Equal(t, expectedPraefect, praefect)
+				wasInvoked = true
+				return tc.stopErr
+			}
+
+			err := tc.hookFunc(t)
+			require.Equal(t, "executing custom hooks: exit status 1", err.Error())
+			require.True(t, wasInvoked, "expected stop to have been invoked")
+		})
+	}
+}
diff --git a/internal/gitaly/hook/update.go b/internal/gitaly/hook/update.go
index 9e9705d67..500573505 100644
--- a/internal/gitaly/hook/update.go
+++ b/internal/gitaly/hook/update.go
@@ -2,6 +2,7 @@ package hook
 
 import (
 	"context"
+	"fmt"
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -17,6 +18,9 @@ func (m *GitLabHookManager) UpdateHook(ctx context.Context, repo *gitalypb.Repos
 
 	if isPrimary(payload) {
 		if err := m.updateHook(ctx, payload, repo, ref, oldValue, newValue, env, stdout, stderr); err != nil {
+			// If the update hook declines the push, then we need
+			// to stop any secondaries voting on the transaction.
+			m.stopTransaction(ctx, payload)
 			return err
 		}
 	}
@@ -56,7 +60,7 @@ func (m *GitLabHookManager) updateHook(ctx context.Context, payload git.HooksPay
 		stdout,
 		stderr,
 	); err != nil {
-		return err
+		return fmt.Errorf("executing custom hooks: %w", err)
 	}
 
 	return nil