Merge branch 'smh-error-dbl-commit-rollbak' into 'master'

Error when double committing or rollbacking a transaction

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5841

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

2 files changed, 99 insertions, 1 deletions

diff --git a/internal/gitaly/transaction_manager.go b/internal/gitaly/transaction_manager.go
index 1cae27d8a..7d7a22da8 100644
--- a/internal/gitaly/transaction_manager.go
+++ b/internal/gitaly/transaction_manager.go
@@ -36,6 +36,12 @@ var (
 	ErrRepositoryNotFound = structerr.NewNotFound("repository not found")
 	// ErrTransactionProcessingStopped is returned when the TransactionManager stops processing transactions.
 	ErrTransactionProcessingStopped = errors.New("transaction processing stopped")
+	// ErrTransactionAlreadyCommitted is returned when attempting to rollback or commit a transaction that
+	// already had commit called on it.
+	ErrTransactionAlreadyCommitted = errors.New("transaction already committed")
+	// ErrTransactionAlreadyRollbacked is returned when attempting to rollback or commit a transaction that
+	// already had rollback called on it.
+	ErrTransactionAlreadyRollbacked = errors.New("transaction already rollbacked")
 	// errInitializationFailed is returned when the TransactionManager failed to initialize successfully.
 	errInitializationFailed = errors.New("initializing transaction processing failed")
 	// errNotDirectory is returned when the repository's path doesn't point to a directory
@@ -122,8 +128,22 @@ type Snapshot struct {
 	CustomHookPath string
 }
 
+type transactionState int
+
+const (
+	// transactionStateOpen indicates the transaction is open, and hasn't been committed or rolled back yet.
+	transactionStateOpen = transactionState(iota)
+	// transactionStateRollback indicates the transaction has been rolled back.
+	transactionStateRollback
+	// transactionStateCommit indicates the transaction has already been committed.
+	transactionStateCommit
+)
+
 // Transaction is a unit-of-work that contains reference changes to perform on the repository.
 type Transaction struct {
+	// state records whether the transaction is still open. Transaction is open until either Commit()
+	// or Rollback() is called on it.
+	state transactionState
 	// commit commits the Transaction through the TransactionManager.
 	commit func(context.Context, *Transaction) error
 	// finalize decrements the active transaction count on the partition in the PartitionManager. This is
@@ -268,9 +288,27 @@ func (mgr *TransactionManager) Begin(ctx context.Context) (_ *Transaction, retur
 	}
 }
 
+func (txn *Transaction) updateState(newState transactionState) error {
+	switch txn.state {
+	case transactionStateOpen:
+		txn.state = newState
+		return nil
+	case transactionStateRollback:
+		return ErrTransactionAlreadyRollbacked
+	case transactionStateCommit:
+		return ErrTransactionAlreadyCommitted
+	default:
+		return fmt.Errorf("unknown transaction state: %q", txn.state)
+	}
+}
+
 // Commit performs the changes. If no error is returned, the transaction was successful and the changes
 // have been performed. If an error was returned, the transaction may or may not be persisted.
 func (txn *Transaction) Commit(ctx context.Context) (returnedErr error) {
+	if err := txn.updateState(transactionStateCommit); err != nil {
+		return err
+	}
+
 	defer func() {
 		txn.finalize()
 
@@ -284,6 +322,10 @@ func (txn *Transaction) Commit(ctx context.Context) (returnedErr error) {
 
 // Rollback releases resources associated with the transaction without performing any changes.
 func (txn *Transaction) Rollback() error {
+	if err := txn.updateState(transactionStateRollback); err != nil {
+		return err
+	}
+
 	defer txn.finalize()
 	return txn.finishUnadmitted()
 }
diff --git a/internal/gitaly/transaction_manager_test.go b/internal/gitaly/transaction_manager_test.go
index 7a12cb982..7d6d1bd27 100644
--- a/internal/gitaly/transaction_manager_test.go
+++ b/internal/gitaly/transaction_manager_test.go
@@ -338,6 +338,8 @@ func TestTransactionManager(t *testing.T) {
 	type Rollback struct {
 		// TransactionID identifies the transaction to rollback.
 		TransactionID int
+		// ExpectedError is the error that is expected to be returned when rolling back the transaction.
+		ExpectedError error
 	}
 
 	// Prune prunes all unreferenced objects from the repository.
@@ -2843,6 +2845,60 @@ func TestTransactionManager(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "transaction rollbacked after already being rollbacked",
+			steps: steps{
+				StartManager{},
+				Begin{},
+				Rollback{},
+				Rollback{
+					ExpectedError: ErrTransactionAlreadyRollbacked,
+				},
+			},
+		},
+		{
+			desc: "transaction rollbacked after already being committed",
+			steps: steps{
+				StartManager{},
+				Begin{},
+				Commit{},
+				Rollback{
+					ExpectedError: ErrTransactionAlreadyCommitted,
+				},
+			},
+			expectedState: StateAssertion{
+				Database: DatabaseState{
+					string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(),
+				},
+			},
+		},
+		{
+			desc: "transaction committed after already being committed",
+			steps: steps{
+				StartManager{},
+				Begin{},
+				Commit{},
+				Commit{
+					ExpectedError: ErrTransactionAlreadyCommitted,
+				},
+			},
+			expectedState: StateAssertion{
+				Database: DatabaseState{
+					string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(),
+				},
+			},
+		},
+		{
+			desc: "transaction committed after already being rollbacked",
+			steps: steps{
+				StartManager{},
+				Begin{},
+				Rollback{},
+				Commit{
+					ExpectedError: ErrTransactionAlreadyRollbacked,
+				},
+			},
+		},
 	}
 
 	type invalidReferenceTestCase struct {
@@ -3167,7 +3223,7 @@ func TestTransactionManager(t *testing.T) {
 					<-transaction.admitted
 				case Rollback:
 					require.Contains(t, openTransactions, step.TransactionID, "test error: transaction rollbacked before beginning it")
-					require.NoError(t, openTransactions[step.TransactionID].Rollback())
+					require.Equal(t, step.ExpectedError, openTransactions[step.TransactionID].Rollback())
 				case Prune:
 					// Repack all objects into a single pack and remove all other packs to remove all
 					// unreachable objects from the packs.