diff --git a/pkg/infrastructure/database/repository.go b/pkg/infrastructure/database/repository.go index a3c8355..ef22560 100644 --- a/pkg/infrastructure/database/repository.go +++ b/pkg/infrastructure/database/repository.go @@ -53,11 +53,14 @@ func (r *Repository) ImageExists(name string) (bool, error) { } // InsertImage inserts an image and all its related data in a transaction. -func (r *Repository) InsertImage(img *domain.ImageRecord) error { +// err is a named result so the deferred rollback always sees failures from +// every return path (including short-declaration shadows in nested scopes). +func (r *Repository) InsertImage(img *domain.ImageRecord) (err error) { tx, err := r.db.Begin() if err != nil { return fmt.Errorf("beginning transaction: %w", err) } + // Defer is registered only after Begin succeeds, so tx is always non-nil here. defer func() { if err != nil { if rbErr := tx.Rollback(); rbErr != nil { @@ -118,10 +121,11 @@ func (r *Repository) InsertImage(img *domain.ImageRecord) error { return fmt.Errorf("getting image id: %w", err) } - // Clear old related data on upsert + // Clear old related data on upsert. + // Assign to the named err (not :=) so deferred rollback observes failures. relTables := []string{"languages", "vulnerabilities", "package_managers", "capabilities", "system_packages", "security_findings"} for _, t := range relTables { - if _, err := tx.Exec(fmt.Sprintf("DELETE FROM %s WHERE image_id = ?", t), imageID); err != nil { + if _, err = tx.Exec(fmt.Sprintf("DELETE FROM %s WHERE image_id = ?", t), imageID); err != nil { return fmt.Errorf("clearing %s: %w", t, err) } } @@ -183,8 +187,7 @@ func (r *Repository) InsertImage(img *domain.ImageRecord) error { } } - err = tx.Commit() - if err != nil { + if err = tx.Commit(); err != nil { return fmt.Errorf("committing transaction: %w", err) } diff --git a/pkg/infrastructure/database/repository_test.go b/pkg/infrastructure/database/repository_test.go index 5e5f3d2..7afa79d 100644 --- a/pkg/infrastructure/database/repository_test.go +++ b/pkg/infrastructure/database/repository_test.go @@ -25,6 +25,7 @@ package database import ( "database/sql" "fmt" + "path/filepath" "testing" "github.com/microsoft/sbi/pkg/domain" @@ -395,3 +396,60 @@ func TestQueryLanguages_BaseSortsLast(t *testing.T) { assert.Equal(t, "python", languages[1]) assert.Equal(t, "base", languages[2]) } + +// TestInsertImage_RollbackOnClearFailure ensures a failure while clearing +// related tables rolls back the whole upsert and releases the connection. +// +// This guards against the classic Go bug where `if _, err := tx.Exec(...)` +// shadows the outer err used by deferred rollback, leaving the transaction +// open (and, with MaxOpenConns(1), blocking all further DB use). +func TestInsertImage_RollbackOnClearFailure(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "rollback.db") + db, err := sql.Open("sqlite", dbPath) + require.NoError(t, err) + defer func() { _ = db.Close() }() + + // Match production OpenDB: a single connection so a leaked tx is observable. + db.SetMaxOpenConns(1) + _, err = db.Exec("PRAGMA foreign_keys=ON") + require.NoError(t, err) + require.NoError(t, CreateTables(db)) + + repo := NewRepository(db) + img := &domain.ImageRecord{ + Name: "test:1.0", + Registry: "r", + Repository: "repo", + Tag: "1.0", + TotalVulnerabilities: 5, + Languages: []domain.Language{ + {Language: "python", Version: "3.12"}, + }, + } + require.NoError(t, repo.InsertImage(img)) + + // Force the related-table clear path to fail on the next upsert. + _, err = db.Exec("DROP TABLE languages") + require.NoError(t, err) + + img.TotalVulnerabilities = 99 + err = repo.InsertImage(img) + require.Error(t, err) + assert.Contains(t, err.Error(), "clearing languages") + + // Upsert must not have been committed. + var total int + err = db.QueryRow(`SELECT total_vulnerabilities FROM images WHERE name = ?`, img.Name).Scan(&total) + require.NoError(t, err) + assert.Equal(t, 5, total, "failed clear must roll back the image upsert") + + // Connection must be released so further writes can proceed. + require.NoError(t, CreateTables(db)) // recreates languages (IF NOT EXISTS) + img.TotalVulnerabilities = 7 + img.Languages = []domain.Language{{Language: "python", Version: "3.12.1"}} + require.NoError(t, repo.InsertImage(img)) + + err = db.QueryRow(`SELECT total_vulnerabilities FROM images WHERE name = ?`, img.Name).Scan(&total) + require.NoError(t, err) + assert.Equal(t, 7, total) +}