From ce0a2261fca565d5433c3ba59694e600d76f8770 Mon Sep 17 00:00:00 2001 From: Mazen Essam Date: Thu, 23 Jul 2026 23:03:00 +0000 Subject: [PATCH] fix(storage): return SQL transaction commit errors in Create and Delete The SQL driver ignored errors from the deferred transaction.Commit() in Create and Delete, reporting success even when the commit failed. Delete also registered the deferred commit before its DELETE statements ran, so error paths after that point committed partial work while returning an error, and a few early returns left the transaction open. Defer a rollback right after starting the transaction (a no-op once the commit succeeds) and commit explicitly as the last step, returning the commit error. Add sqlmock regression tests for commit failures in both functions and for rollback when deleting labels fails. Signed-off-by: Mazen Essam --- pkg/storage/driver/sql.go | 27 +++-- pkg/storage/driver/sql_test.go | 179 +++++++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+), 9 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 85e6cbd3f..acb580f86 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -486,6 +486,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return fmt.Errorf("error beginning transaction: %w", err) } + defer transaction.Rollback() insertQuery, args, err := s.statementBuilder. Insert(sqlReleaseTableName). @@ -517,8 +518,6 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } if _, err := transaction.Exec(insertQuery, args...); err != nil { - defer transaction.Rollback() - selectQuery, args, buildErr := s.statementBuilder. Select(sqlReleaseTableKeyColumn). From(sqlReleaseTableName). @@ -558,18 +557,20 @@ func (s *SQL) Create(key string, rel release.Releaser) error { ).ToSql() if err != nil { - defer transaction.Rollback() s.Logger().Debug("failed to build insert query", slog.Any("error", err)) return err } if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { - defer transaction.Rollback() s.Logger().Debug("failed to write Labels", slog.Any("error", err)) return err } } - defer transaction.Commit() + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit SQL transaction", slog.Any("error", err)) + return fmt.Errorf("error committing transaction: %w", err) + } return nil } @@ -624,6 +625,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return nil, fmt.Errorf("error beginning transaction: %w", err) } + defer transaction.Rollback() selectQuery, args, err := s.statementBuilder. Select(sqlReleaseTableBodyColumn). @@ -646,10 +648,8 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { release, err := decodeRelease(record.Body) if err != nil { s.Logger().Debug("failed to decode release", slog.String("key", key), slog.Any("error", err)) - transaction.Rollback() return nil, err } - defer transaction.Commit() deleteQuery, args, err := s.statementBuilder. Delete(sqlReleaseTableName). @@ -686,8 +686,17 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err)) return nil, err } - _, err = transaction.Exec(deleteCustomLabelsQuery, args...) - return release, err + if _, err = transaction.Exec(deleteCustomLabelsQuery, args...); err != nil { + s.Logger().Debug("failed to perform delete Labels query", slog.Any("error", err)) + return release, err + } + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit SQL transaction", slog.Any("error", err)) + return release, fmt.Errorf("error committing transaction: %w", err) + } + + return release, nil } // Get release custom labels from database diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index adcc780aa..6283ab506 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -288,6 +288,59 @@ func TestSqlCreateAlreadyExists(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +func TestSqlCreateCommitFailure(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + query := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s,%s,%s,%s,%s,%s) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableTypeColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableCreatedAtColumn, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()). + WillReturnResult(sqlmock.NewResult(1, 1)) + + labelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(labelsQuery)). + WithArgs(key, rel.Namespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit().WillReturnError(errors.New("commit failed")) + + err := sqlDriver.Create(key, rel) + require.ErrorContains(t, err, "commit failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSqlUpdate(t *testing.T) { vers := 1 name := "smug-pigeon" @@ -498,6 +551,132 @@ func TestSqlDelete(t *testing.T) { assert.Truef(t, reflect.DeepEqual(rel, deletedRelease), "Expected release {%v}, got {%v}", rel, deletedRelease) } +func TestSqlDeleteCommitFailure(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + + body, _ := encodeRelease(rel) + + sqlDriver, mock := newTestFixtureSQL(t) + + selectQuery := fmt.Sprintf( + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectBegin() + mock. + ExpectQuery(regexp.QuoteMeta(selectQuery)). + WithArgs(key, namespace). + WillReturnRows( + mock.NewRows([]string{ + sqlReleaseTableBodyColumn, + }).AddRow( + body, + ), + ).RowsWillBeClosed() + + deleteQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock. + ExpectExec(regexp.QuoteMeta(deleteQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mockGetReleaseCustomLabels(mock, key, namespace, rel.Labels) + + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.ExpectCommit().WillReturnError(errors.New("commit failed")) + + _, err := sqlDriver.Delete(key) + require.ErrorContains(t, err, "commit failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +func TestSqlDeleteLabelsFailureRollsBack(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + + body, _ := encodeRelease(rel) + + sqlDriver, mock := newTestFixtureSQL(t) + + selectQuery := fmt.Sprintf( + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectBegin() + mock. + ExpectQuery(regexp.QuoteMeta(selectQuery)). + WithArgs(key, namespace). + WillReturnRows( + mock.NewRows([]string{ + sqlReleaseTableBodyColumn, + }).AddRow( + body, + ), + ).RowsWillBeClosed() + + deleteQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock. + ExpectExec(regexp.QuoteMeta(deleteQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mockGetReleaseCustomLabels(mock, key, namespace, rel.Labels) + + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnError(errors.New("labels delete failed")) + + mock.ExpectRollback() + + _, err := sqlDriver.Delete(key) + require.ErrorContains(t, err, "labels delete failed") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func mockGetReleaseCustomLabels(mock sqlmock.Sqlmock, key string, namespace string, labels map[string]string) { query := fmt.Sprintf( regexp.QuoteMeta("SELECT %s, %s FROM %s WHERE %s = $1 AND %s = $2"),