transactions: Correctly handle cancellation of empty transactions

If cancelling a transaction which doesn't have any subtransactons, we
currently loose track of the fact that the transaction has been
cancelled as we only use subtransaction information to determine the
transaction's state. It's thus fine to vote on an cancelled, but empty
transaction, which is simply wrong.

Fix this by explicitly keeping track of the cancellation state, refusing
to create any new subtransactions if the transaction is in such a state.

3 files changed, 53 insertions, 0 deletions

diff --git a/changelogs/unreleased/pks-tx-empty-transaction-cancellation.yml b/changelogs/unreleased/pks-tx-empty-transaction-cancellation.yml
new file mode 100644
index 000000000..25817e926
--- /dev/null
+++ b/changelogs/unreleased/pks-tx-empty-transaction-cancellation.yml
@@ -0,0 +1,5 @@
+---
+title: 'transactions: Correctly handle cancellation of empty transactions'
+merge_request: 2594
+author:
+type: fixed
diff --git a/internal/praefect/transactions/transaction.go b/internal/praefect/transactions/transaction.go
index c98331053..37f4b841c 100644
--- a/internal/praefect/transactions/transaction.go
+++ b/internal/praefect/transactions/transaction.go
@@ -21,6 +21,13 @@ var (
 	ErrSubtransactionFailed = errors.New("subtransaction has failed")
 )
 
+type transactionState int
+
+const (
+	transactionOpen = transactionState(iota)
+	transactionCanceled
+)
+
 // Voter is a participant in a given transaction that may cast a vote.
 type Voter struct {
 	// Name of the voter, usually Gitaly's storage name.
@@ -44,6 +51,7 @@ type Transaction struct {
 	voters    []Voter
 
 	lock            sync.Mutex
+	state           transactionState
 	subtransactions []*subtransaction
 }
 
@@ -79,6 +87,7 @@ func newTransaction(id uint64, voters []Voter, threshold uint) (*Transaction, er
 		id:        id,
 		threshold: threshold,
 		voters:    voters,
+		state:     transactionOpen,
 	}, nil
 }
 
@@ -89,6 +98,8 @@ func (t *Transaction) cancel() {
 	for _, subtransaction := range t.subtransactions {
 		subtransaction.cancel()
 	}
+
+	t.state = transactionCanceled
 }
 
 // ID returns the identifier used to uniquely identify a transaction.
@@ -145,6 +156,15 @@ func (t *Transaction) getOrCreateSubtransaction(node string) (*subtransaction, e
 	t.lock.Lock()
 	defer t.lock.Unlock()
 
+	switch t.state {
+	case transactionOpen:
+		// expected state, nothing to do
+	case transactionCanceled:
+		return nil, ErrTransactionCanceled
+	default:
+		return nil, errors.New("invalid transaction state")
+	}
+
 	for _, subtransaction := range t.subtransactions {
 		result, err := subtransaction.getResult(node)
 		if err != nil {
diff --git a/internal/praefect/transactions/transaction_test.go b/internal/praefect/transactions/transaction_test.go
new file mode 100644
index 000000000..6a9ff1e53
--- /dev/null
+++ b/internal/praefect/transactions/transaction_test.go
@@ -0,0 +1,28 @@
+package transactions
+
+import (
+	"crypto/sha1"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+func TestTransactionCancellationWithEmptyTransaction(t *testing.T) {
+	ctx, cleanup := testhelper.Context()
+	defer cleanup()
+
+	tx, err := newTransaction(1, []Voter{
+		{Name: "voter", Votes: 1},
+	}, 1)
+	require.NoError(t, err)
+
+	hash := sha1.Sum([]byte{})
+
+	tx.cancel()
+
+	// When canceling a transaction, no more votes may happen.
+	err = tx.vote(ctx, "voter", hash[:])
+	require.Error(t, err)
+	require.Equal(t, err, ErrTransactionCanceled)
+}