fix (db): Improve error handling for commit errors. (#2728)

We were not capturing the errors which happen on commit.  We've changed
to a multierror so we should get the commit error and any subsequent
rollback errors in the same txn.

internal/db/read_writer.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/boundary/internal/oplog/store"
 	"github.com/hashicorp/go-dbw"
 	wrapping "github.com/hashicorp/go-kms-wrapping/v2"
+	"github.com/hashicorp/go-multierror"
 )
 
 const (
@@ -438,11 +439,16 @@ func (rw *Db) DoTx(ctx context.Context, retries uint, backOff Backoff, handler T
 			return info, errors.Wrap(ctx, err, op, errors.WithoutEvent())
 		}
 
-		if err := beginTx.Commit(ctx); err != nil {
+		var txnErr *multierror.Error
+		if commitErr := beginTx.Commit(ctx); commitErr != nil {
+			txnErr = multierror.Append(txnErr, errors.Wrap(ctx, commitErr, op, errors.WithMsg("commit error")))
+			// unsure if rolling back is required or possible, but including
+			// this attempt to rollback on a commit error just in case it's
+			// possible.
 			if err := beginTx.Rollback(ctx); err != nil {
-				return info, errors.Wrap(ctx, err, op)
+				return info, multierror.Append(txnErr, errors.Wrap(ctx, err, op, errors.WithMsg("rollback error")))
 			}
-			return info, errors.Wrap(ctx, err, op)
+			return info, txnErr
 		}
 		return info, nil // it all worked!!!
 	}

internal/db/read_writer_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/DATA-DOG/go-sqlmock"
 	"github.com/hashicorp/boundary/internal/db/db_test"
 	"github.com/hashicorp/boundary/internal/db/timestamp"
 	"github.com/hashicorp/boundary/internal/errors"
@@ -1323,6 +1324,35 @@ func TestDb_DoTx(t *testing.T) {
 		require.NoError(err)
 		assert.Equal(foundUser.Name, user.Name)
 	})
+	t.Run("commit-and-rollback-errors", func(t *testing.T) {
+		assert, require := assert.New(t), require.New(t)
+		mockConn, mock := TestSetupWithMock(t)
+		mockConn.Debug(true)
+		rw := New(mockConn)
+		user, err := db_test.NewTestUser()
+		require.NoError(err)
+		user.Id = 1
+
+		mock.ExpectBegin()
+		mock.ExpectExec(`UPDATE`).WillReturnResult(sqlmock.NewResult(1, 1))
+		mock.ExpectQuery(`SELECT`).WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(1))
+		mock.ExpectCommit().WillReturnError(fmt.Errorf("commit-err"))
+		mock.ExpectRollback().WillReturnError(fmt.Errorf("rollback-err"))
+		_, err = rw.DoTx(context.Background(), 10, ExpBackoff{}, func(r Reader, w Writer) error {
+			user.Name = "friendly-name"
+			rowsUpdated, err := w.Update(context.Background(), user, []string{"Name"}, nil)
+			if err != nil {
+				return err
+			}
+			if rowsUpdated != 1 {
+				return fmt.Errorf("error in number of rows updated %d", rowsUpdated)
+			}
+			return nil
+		})
+		require.Error(err)
+		assert.Contains(err.Error(), "commit error")
+		assert.Contains(err.Error(), "rollback error")
+	})
 }
 
 func TestDb_Delete(t *testing.T) {