From 1a861029f0235cd4acd3529c28cd81651549da2f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 15 Oct 2026 06:35:27 +0000 Subject: [PATCH] fix(database): convert forge_instance_id to uuid before 0008 on PostgreSQL Migration 0008 failed with "operator does not exist: text = uuid" on PostgreSQL databases migrated through 0003. The 0003 stubs declare forge_instance_id without the relation that gives fresh databases a uuid column, so those databases have text columns that 0008 can neither compare with nor constrain to forge_instances.id. Convert them first. Fixes #920 --- database/sql/brownfield_test.go | 20 ++++++++++-- .../sql/migrations/0008_constraint_parity.go | 31 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/database/sql/brownfield_test.go b/database/sql/brownfield_test.go index 1338b4882..eb3478958 100644 --- a/database/sql/brownfield_test.go +++ b/database/sql/brownfield_test.go @@ -391,6 +391,8 @@ var parityConstraints = map[string]string{ "fk_forge_instances_jobs": "workflow_jobs", } +var forgeInstanceIDTables = []string{"forge_instance_events", "pools", "workflow_jobs"} + func requireConstraintCountPG(t *testing.T, conn *gorm.DB, constraint string, expected int64) { t.Helper() var count int64 @@ -407,7 +409,8 @@ func requireConstraintCountPG(t *testing.T, conn *gorm.DB, constraint string, ex // the constraints via ALTER TABLE. Databases born later have everything // already: for them 0008 must be a clean no-op. Constraints are dropped // manually to reconstruct the first cohort, since 0008 introduces no model -// changes of its own. +// changes of its own. The first cohort also went through the 0003 stubs, +// which created the forge_instance_id columns as text instead of uuid. func TestConstraintParityOnPostgres(t *testing.T) { if os.Getenv("GARM_TEST_POSTGRES_DSN") == "" { t.Skip("GARM_TEST_POSTGRES_DSN not set") @@ -428,8 +431,11 @@ func TestConstraintParityOnPostgres(t *testing.T) { for constraint, table := range parityConstraints { require.NoError(t, db.conn.Exec("ALTER TABLE "+table+" DROP CONSTRAINT "+constraint).Error) } + for _, table := range forgeInstanceIDTables { + require.NoError(t, db.conn.Exec("ALTER TABLE "+table+" ALTER COLUMN forge_instance_id TYPE text").Error) + } require.NoError(t, db.conn.Exec( - "INSERT INTO pools (id, created_at, updated_at, provider_name, runner_prefix, max_runners, min_idle_runners, runner_bootstrap_timeout, image, flavor, os_type, os_arch, enabled, generation, priority, proxy_id) VALUES ('99999999-9999-9999-9999-999999999999', now(), now(), 'test-provider', 'garm', 4, 0, 20, 'ubuntu:24.04', 'default', 'linux', 'amd64', true, 1, 0, 12345)").Error) + "INSERT INTO pools (id, created_at, updated_at, provider_name, runner_prefix, max_runners, min_idle_runners, runner_bootstrap_timeout, image, flavor, os_type, os_arch, enabled, generation, priority, proxy_id, forge_instance_id) VALUES ('99999999-9999-9999-9999-999999999999', now(), now(), 'test-provider', 'garm', 4, 0, 20, 'ubuntu:24.04', 'default', 'linux', 'amd64', true, 1, 0, 12345, '88888888-8888-8888-8888-888888888888')").Error) require.NoError(t, db.conn.Exec("DELETE FROM migrations WHERE id = '0008_constraint_parity'").Error) require.NoError(t, db.migrateDB(), "0008 failed on a PostgreSQL database missing the constraints") @@ -438,9 +444,19 @@ func TestConstraintParityOnPostgres(t *testing.T) { for constraint := range parityConstraints { requireConstraintCountPG(t, db.conn, constraint, 1) } + for _, table := range forgeInstanceIDTables { + var dataType string + require.NoError(t, db.conn.Raw( + "SELECT data_type FROM information_schema.columns WHERE table_schema = current_schema() AND table_name = ? AND column_name = 'forge_instance_id'", + table).Scan(&dataType).Error) + require.Equal(t, "uuid", dataType, "%s.forge_instance_id type mismatch", table) + } var orphanedProxyRefs int64 require.NoError(t, db.conn.Raw("SELECT count(*) FROM pools WHERE proxy_id IS NOT NULL").Scan(&orphanedProxyRefs).Error) require.Zero(t, orphanedProxyRefs, "orphaned proxy reference should have been cleaned up") + var orphanedForgeInstanceRefs int64 + require.NoError(t, db.conn.Raw("SELECT count(*) FROM pools WHERE forge_instance_id IS NOT NULL").Scan(&orphanedForgeInstanceRefs).Error) + require.Zero(t, orphanedForgeInstanceRefs, "orphaned forge instance reference should have been cleaned up") // Phase 2: a database that already has everything; 0008 must no-op. require.NoError(t, db.conn.Exec("DELETE FROM migrations WHERE id = '0008_constraint_parity'").Error) diff --git a/database/sql/migrations/0008_constraint_parity.go b/database/sql/migrations/0008_constraint_parity.go index f9a2a093e..f0c6c012e 100644 --- a/database/sql/migrations/0008_constraint_parity.go +++ b/database/sql/migrations/0008_constraint_parity.go @@ -116,6 +116,31 @@ type fkViolation struct { Fkid int64 `gorm:"column:fkid"` } +// convertForgeInstanceIDsToUUID gives the forge_instance_id columns the uuid +// type that fresh PostgreSQL databases have. The 0003 stubs declare them +// without the relation that the models in models.go derive the type from, +// so databases migrated through 0003 have text columns. Those can be +// neither compared with nor constrained to forge_instances.id. +func convertForgeInstanceIDsToUUID(tx *gorm.DB) error { + for _, table := range []string{"forge_instance_events", "pools", "workflow_jobs"} { + var dataType string + err := tx.Raw( + "SELECT data_type FROM information_schema.columns WHERE table_schema = current_schema() AND table_name = ? AND column_name = 'forge_instance_id'", + table).Scan(&dataType).Error + if err != nil { + return fmt.Errorf("inspecting %s.forge_instance_id: %w", table, err) + } + if dataType != "text" { + continue + } + if err := tx.Exec(fmt.Sprintf("ALTER TABLE %q ALTER COLUMN forge_instance_id TYPE uuid USING forge_instance_id::uuid", table)).Error; err != nil { + return fmt.Errorf("converting %s.forge_instance_id to uuid: %w", table, err) + } + slog.Info("converted column to uuid during migration", "table", table, "column", "forge_instance_id") + } + return nil +} + // resolveSQLiteOrphans applies each foreign key's own delete semantics to // rows that violate it. Old GARM versions ran with foreign key enforcement // disabled in places, so long lived databases can hold orphaned rows (for @@ -163,6 +188,12 @@ func init() { Register(&gormigrate.Migration{ ID: "0008_constraint_parity", Migrate: func(tx *gorm.DB) error { + if tx.Name() == "postgres" { + if err := convertForgeInstanceIDsToUUID(tx); err != nil { + return err + } + } + // Rows violating the constraints about to be added would fail // the ADD CONSTRAINT on PostgreSQL and the integrity check on // SQLite. Resolve them the way the constraints would have.