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>
pull/32471/head
Gates Wang 2 months ago
parent f627adc8e6
commit 11717c94c6

@ -17,6 +17,7 @@ limitations under the License.
package driver package driver
import ( import (
"context"
"fmt" "fmt"
"log/slog" "log/slog"
"maps" "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)) s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err))
return fmt.Errorf("error beginning transaction: %w", err) return fmt.Errorf("error beginning transaction: %w", err)
} }
defer transaction.Rollback()
insertQuery, args, err := s.statementBuilder. insertQuery, args, err := s.statementBuilder.
Insert(sqlReleaseTableName). Insert(sqlReleaseTableName).
@ -516,9 +518,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error {
return err return err
} }
if _, err := transaction.Exec(insertQuery, args...); err != nil { if _, err := transaction.ExecContext(context.Background(), insertQuery, args...); err != nil {
defer transaction.Rollback()
selectQuery, args, buildErr := s.statementBuilder. selectQuery, args, buildErr := s.statementBuilder.
Select(sqlReleaseTableKeyColumn). Select(sqlReleaseTableKeyColumn).
From(sqlReleaseTableName). From(sqlReleaseTableName).
@ -558,18 +558,20 @@ func (s *SQL) Create(key string, rel release.Releaser) error {
).ToSql() ).ToSql()
if err != nil { if err != nil {
defer transaction.Rollback()
s.Logger().Debug("failed to build insert query", slog.Any("error", err)) s.Logger().Debug("failed to build insert query", slog.Any("error", err))
return err return err
} }
if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { if _, err := transaction.ExecContext(context.Background(), insertLabelsQuery, args...); err != nil {
defer transaction.Rollback()
s.Logger().Debug("failed to write Labels", slog.Any("error", err)) s.Logger().Debug("failed to write Labels", slog.Any("error", err))
return 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 return nil
} }
@ -609,7 +611,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error {
return err 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)) s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err))
return 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)) s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err))
return nil, fmt.Errorf("error beginning transaction: %w", err) return nil, fmt.Errorf("error beginning transaction: %w", err)
} }
defer transaction.Rollback()
selectQuery, args, err := s.statementBuilder. selectQuery, args, err := s.statementBuilder.
Select(sqlReleaseTableBodyColumn). Select(sqlReleaseTableBodyColumn).
@ -646,10 +649,8 @@ func (s *SQL) Delete(key string) (release.Releaser, error) {
release, err := decodeRelease(record.Body) release, err := decodeRelease(record.Body)
if err != nil { if err != nil {
s.Logger().Debug("failed to decode release", slog.String("key", key), slog.Any("error", err)) s.Logger().Debug("failed to decode release", slog.String("key", key), slog.Any("error", err))
transaction.Rollback()
return nil, err return nil, err
} }
defer transaction.Commit()
deleteQuery, args, err := s.statementBuilder. deleteQuery, args, err := s.statementBuilder.
Delete(sqlReleaseTableName). Delete(sqlReleaseTableName).
@ -661,7 +662,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) {
return nil, err return nil, err
} }
_, err = transaction.Exec(deleteQuery, args...) _, err = transaction.ExecContext(context.Background(), deleteQuery, args...)
if err != nil { if err != nil {
s.Logger().Debug("failed perform delete query", slog.Any("error", err)) s.Logger().Debug("failed perform delete query", slog.Any("error", err))
return release, err return release, err
@ -686,10 +687,19 @@ func (s *SQL) Delete(key string) (release.Releaser, error) {
s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err)) s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err))
return nil, err return nil, err
} }
_, err = transaction.Exec(deleteCustomLabelsQuery, args...) 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 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 // Get release custom labels from database
func (s *SQL) getReleaseCustomLabels(key string, _ string) (map[string]string, error) { func (s *SQL) getReleaseCustomLabels(key string, _ string) (map[string]string, error) {
query, args, err := s.statementBuilder. query, args, err := s.statementBuilder.

Loading…
Cancel
Save