From f627adc8e6c974e1876272258dcdb6891ff71da2 Mon Sep 17 00:00:00 2001 From: Gates Wang <9372086+SetagGnaw@users.noreply.github.com> Date: Fri, 24 Jul 2026 23:42:21 -0400 Subject: [PATCH 1/2] test(storage/sql): add failing regression tests for tx handling Add three regression tests for the SQL storage driver that capture bugs in the transaction handling of Create and Delete. They fail against the current code and pass once the driver is fixed: - TestSqlCreateCommitError / TestSqlDeleteCommitError: a commit-time failure is swallowed by `defer transaction.Commit()`, so the methods report success even though nothing was persisted. - TestSqlDeleteNotFoundReleasesTransaction: the not-found path returns without rolling back, leaking the pooled connection and an open server-side transaction. Signed-off-by: Gates Wang <9372086+SetagGnaw@users.noreply.github.com> --- pkg/storage/driver/sql_test.go | 156 +++++++++++++++++++++++++++++++++ 1 file changed, 156 insertions(+) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 044e9df7b..4b53830c0 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -14,6 +14,7 @@ limitations under the License. package driver import ( + "database/sql" "database/sql/driver" "errors" "fmt" @@ -561,3 +562,158 @@ func TestSqlCheckAppliedMigrations(t *testing.T) { assert.Equal(t, c.expectedResult, sqlDriver.checkAlreadyApplied(c.migrationsToApply), "Test case: %v, Expected: %v, Have: %v, Explanation: %v", i, c.expectedResult, !c.expectedResult, c.errorExplanation) } } + +// TestSqlCreateCommitError verifies that a commit-time failure is surfaced to +// the caller. A deferred, error-discarding Commit would report success even +// though nothing was persisted. +func TestSqlCreateCommitError(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("transaction commit failed")) + + err := sqlDriver.Create(key, rel) + require.Error(t, err, "expected Create to surface the commit error, got nil") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// TestSqlDeleteCommitError verifies that a commit-time failure during Delete is +// surfaced to the caller instead of being silently discarded. +func TestSqlDeleteCommitError(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("transaction commit failed")) + + _, err := sqlDriver.Delete(key) + require.Error(t, err, "expected Delete to surface the commit error, got nil") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// TestSqlDeleteNotFoundReleasesTransaction verifies that the not-found path +// rolls back the transaction rather than leaking its pooled connection. +func TestSqlDeleteNotFoundReleasesTransaction(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + + 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). + WillReturnError(sql.ErrNoRows) + mock.ExpectRollback() + + _, err := sqlDriver.Delete(key) + require.ErrorIs(t, err, ErrReleaseNotFound) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} From 11717c94c6a41d0530d5d3e6a551deaef3321893 Mon Sep 17 00:00:00 2001 From: Gates Wang <9372086+SetagGnaw@users.noreply.github.com> Date: Fri, 24 Jul 2026 23:42:22 -0400 Subject: [PATCH 2/2] fix(storage/sql): don't leak or silently commit transactions The SQL storage driver mishandled its explicit transactions in Create and Delete: - Delete's not-found and build-select-fail early returns returned without Rollback or Commit, permanently leaking the pooled connection and an open server-side transaction. - Both methods finished with `defer transaction.Commit()`, discarding the commit error. A commit-time failure (connection drop, serialization failure, disk full) left the release unpersisted while the method reported success. In Delete the deferred commit also fired on the post-delete error paths, committing a partial delete. Register a single `defer transaction.Rollback()` right after Beginx (a no-op once committed) so every early return releases the transaction, and commit explicitly at the success point, returning any commit error. Error paths after the delete now roll back, making Delete atomic. Covered by the regression tests added in the previous commit. Also convert the driver's direct Exec calls to ExecContext with context.Background(), matching the secrets and cfgmaps drivers. The new tests import database/sql, which activates noctx's database/sql checks for the package and flags these pre-existing call sites. Signed-off-by: Gates Wang <9372086+SetagGnaw@users.noreply.github.com> --- pkg/storage/driver/sql.go | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 85e6cbd3f..dc1a81cc7 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -17,6 +17,7 @@ limitations under the License. package driver import ( + "context" "fmt" "log/slog" "maps" @@ -486,6 +487,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). @@ -516,9 +518,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { return err } - if _, err := transaction.Exec(insertQuery, args...); err != nil { - defer transaction.Rollback() - + if _, err := transaction.ExecContext(context.Background(), insertQuery, args...); err != nil { selectQuery, args, buildErr := s.statementBuilder. Select(sqlReleaseTableKeyColumn). From(sqlReleaseTableName). @@ -558,18 +558,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() + if _, err := transaction.ExecContext(context.Background(), insertLabelsQuery, args...); err != nil { 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 release creation transaction", slog.Any("error", err)) + return fmt.Errorf("error committing transaction: %w", err) + } return nil } @@ -609,7 +611,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } - if _, err := s.db.Exec(query, args...); err != nil { + if _, err := s.db.ExecContext(context.Background(), query, args...); err != nil { s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } @@ -624,6 +626,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 +649,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). @@ -661,7 +662,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { return nil, err } - _, err = transaction.Exec(deleteQuery, args...) + _, err = transaction.ExecContext(context.Background(), deleteQuery, args...) if err != nil { s.Logger().Debug("failed perform delete query", slog.Any("error", err)) return release, err @@ -686,8 +687,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.ExecContext(context.Background(), deleteCustomLabelsQuery, args...); err != nil { + s.Logger().Debug("failed to delete release custom labels", slog.String("key", key), slog.Any("error", err)) + return release, err + } + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit release deletion transaction", slog.Any("error", err)) + return release, fmt.Errorf("error committing transaction: %w", err) + } + + return release, nil } // Get release custom labels from database