From 78230e974779f1cc7651230cc3403dcc33e1e553 Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Fri, 7 Aug 2026 12:49:30 +0200 Subject: [PATCH] fix(migration): resume the settings reshape after a partial run on mysql MySQL implicitly commits around every ALTER TABLE, so the transaction wrapping the reshape is real only on sqlite and postgres. On MySQL an interrupted run could stop between the two legacy column drops, and the rerun keyed its already-migrated check on the cluster column alone -- leaving the subdomain column behind forever. Key the entry check on either legacy column remaining, run the backfill and its loud-failure check only while the cluster column exists (they provably completed before any drop), and skip drops for columns already gone. No data-loss window existed in any state: the backfill commits before the first drop on every engine. Co-Authored-By: Claude Fable 5 --- .../migration/migration_agentnetwork.go | 67 ++++++++++++------- management/server/migration/migration_test.go | 40 +++++++++++ 2 files changed, 81 insertions(+), 26 deletions(-) diff --git a/management/server/migration/migration_agentnetwork.go b/management/server/migration/migration_agentnetwork.go index 3581d0165..c6cda56c2 100644 --- a/management/server/migration/migration_agentnetwork.go +++ b/management/server/migration/migration_agentnetwork.go @@ -33,6 +33,15 @@ func (agentNetworkSettingsMigration) TableName() string { return "agent_network_ // bootstrap always wrote both, so such a row indicates corruption and the // migration fails loudly rather than leaving an empty domain to collide with // the unique index confusingly. +// +// The transaction is real only on sqlite and postgres, where DDL is +// transactional. MySQL implicitly commits around every ALTER TABLE, so there +// each step stands alone; what makes an interrupted run resumable on MySQL is +// that every step is guarded by the schema state it changes — the entry check +// fires while either legacy column remains, the adds skip existing columns, +// the backfill and its loud-failure check run only while the legacy cluster +// column exists (they provably completed before any drop), and each drop +// skips what is already gone. func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error { model := &agentNetworkSettingsMigration{} migrator := db.Migrator() @@ -40,7 +49,8 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error if !migrator.HasTable(model) { return nil } - if !migrator.HasColumn(model, "cluster") { + hasCluster := migrator.HasColumn(model, "cluster") + if !hasCluster && !migrator.HasColumn(model, "subdomain") { // Fresh schema or already migrated — nothing to reshape. return nil } @@ -55,27 +65,33 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error } } - concat := "subdomain || '.' || cluster" - if tx.Name() == "mysql" { - concat = "CONCAT(subdomain, '.', cluster)" - } - res := tx.Exec(fmt.Sprintf( - "UPDATE agent_network_settings SET domain = %s, proxy_address = cluster WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", - concat, - )) - if res.Error != nil { - return fmt.Errorf("backfill agent_network_settings domain: %w", res.Error) - } + if hasCluster { + concat := "subdomain || '.' || cluster" + if tx.Name() == "mysql" { + concat = "CONCAT(subdomain, '.', cluster)" + } + res := tx.Exec(fmt.Sprintf( + "UPDATE agent_network_settings SET domain = %s, proxy_address = cluster WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", + concat, + )) + if res.Error != nil { + return fmt.Errorf("backfill agent_network_settings domain: %w", res.Error) + } - var unmigratable int64 - if err := tx.Model(model).Where("domain IS NULL OR domain = ''").Count(&unmigratable).Error; err != nil { - return fmt.Errorf("count unmigratable agent_network_settings rows: %w", err) - } - if unmigratable > 0 { - return fmt.Errorf( - "%d agent_network_settings row(s) have no cluster/subdomain to derive an endpoint from; resolve them manually before upgrading", - unmigratable, - ) + var unmigratable int64 + if err := tx.Model(model).Where("domain IS NULL OR domain = ''").Count(&unmigratable).Error; err != nil { + return fmt.Errorf("count unmigratable agent_network_settings rows: %w", err) + } + if unmigratable > 0 { + return fmt.Errorf( + "%d agent_network_settings row(s) have no cluster/subdomain to derive an endpoint from; resolve them manually before upgrading", + unmigratable, + ) + } + + if res.RowsAffected > 0 { + log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) + } } if txMigrator.HasIndex(model, "idx_agent_network_settings_cluster_subdomain") { @@ -84,14 +100,13 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error } } for _, field := range []string{"Cluster", "Subdomain"} { - if err := txMigrator.DropColumn(model, field); err != nil { - return fmt.Errorf("drop legacy agent_network_settings column %s: %w", field, err) + if txMigrator.HasColumn(model, field) { + if err := txMigrator.DropColumn(model, field); err != nil { + return fmt.Errorf("drop legacy agent_network_settings column %s: %w", field, err) + } } } - if res.RowsAffected > 0 { - log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) - } return nil }) } diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 6298f1614..868332fdf 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -818,3 +818,43 @@ func TestMigrateAgentNetworkSettingsToDomain_FailsOnUnmigratableRow(t *testing.T require.Error(t, err, "a row with no identity to derive an endpoint from must fail the migration") assert.Contains(t, err.Error(), "resolve them manually", "the error must tell the operator what to do") } + +// partialAgentNetworkSettings models the one non-atomic state a MySQL run can +// be interrupted in: DDL auto-commits there, so a crash between the two legacy +// column drops leaves subdomain behind while cluster (and the completed +// backfill) are already committed. +type partialAgentNetworkSettings struct { + AccountID string `gorm:"primaryKey"` + Subdomain string + Domain string `gorm:"type:varchar(255)"` + ProxyAddress string `gorm:"type:varchar(255)"` +} + +func (partialAgentNetworkSettings) TableName() string { return "agent_network_settings" } + +// TestMigrateAgentNetworkSettingsToDomain_ResumesAfterPartialDrop pins MySQL +// resumability: a rerun over the interrupted state must remove the leftover +// subdomain column without re-running the backfill (the cluster column that +// feeds it is gone) and without touching the migrated values. +func TestMigrateAgentNetworkSettingsToDomain_ResumesAfterPartialDrop(t *testing.T) { + ctx := context.Background() + db := setupDatabase(t) + require.NoError(t, db.Migrator().DropTable(&partialAgentNetworkSettings{})) + require.NoError(t, db.AutoMigrate(&partialAgentNetworkSettings{})) + require.NoError(t, db.Create(&partialAgentNetworkSettings{ + AccountID: "acct-1", Subdomain: "violet", + Domain: "violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", + }).Error) + + require.NoError(t, migration.MigrateAgentNetworkSettingsToDomain(ctx, db), + "a rerun over a partially-dropped schema must resume, not error") + + migrator := db.Migrator() + assert.False(t, migrator.HasColumn(&partialAgentNetworkSettings{}, "subdomain"), + "the leftover legacy column must be dropped on resume") + + var row agentNetworkTypes.Settings + require.NoError(t, db.First(&row, "account_id = ?", "acct-1").Error) + assert.Equal(t, "violet.eu.proxy.netbird.io", row.Domain, "migrated values must be untouched") + assert.Equal(t, "eu.proxy.netbird.io", row.ProxyAddress, "migrated values must be untouched") +}