diff --git a/Lite.Tests/AgedDatabaseMigrationTests.cs b/Lite.Tests/AgedDatabaseMigrationTests.cs new file mode 100644 index 000000000..c106aaf82 --- /dev/null +++ b/Lite.Tests/AgedDatabaseMigrationTests.cs @@ -0,0 +1,134 @@ +using System; +using System.IO; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using PerformanceMonitorLite.Database; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #2748: a real user's database, aged past v47, failed both the v48 and v53 migrations on upgrade +/// to v3.6.0.0. Both errors were logged as "non-fatal" and the migration chain nominally completed to +/// v56, but the app then failed to start — the two swallowed failures left the database missing +/// state later code assumes is there. These tests seed the exact aged-database preconditions that +/// triggered each failure and assert the upgrade both succeeds AND leaves the database actually +/// correct, not just quietly incomplete. +/// +public class AgedDatabaseMigrationTests : IDisposable +{ + private readonly string _tempDir; + private readonly string _dbPath; + + public AgedDatabaseMigrationTests() + { + _tempDir = Path.Combine(Path.GetTempPath(), "LiteTests_" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(_tempDir); + _dbPath = Path.Combine(_tempDir, "test.duckdb"); + } + + public void Dispose() + { + try + { + if (Directory.Exists(_tempDir)) + Directory.Delete(_tempDir, recursive: true); + } + catch + { + /* Best-effort cleanup */ + } + } + + /// + /// v48 drops NOT NULL from three server_properties columns. On any database that has completed a + /// prior startup, Schema.GetAllIndexStatements() already created idx_server_properties_time — a + /// real, persisted index on (server_id, collection_time) — and DuckDB's ALTER COLUMN dependency + /// check refuses to touch a table with ANY index on it, even one naming none of the altered columns + /// (confirmed empirically: a plain SELECT * archive view does NOT trigger this, only the index + /// does). Seeds that exact precondition and asserts the upgrade both completes AND the column is + /// actually nullable afterward, not merely that it didn't throw. + /// + [Fact] + public async Task UpgradeFromV47_DropsServerPropertiesNotNull_EvenWithAPreExistingIndex() + { + using (var seed = new DuckDBConnection($"Data Source={_dbPath}")) + { + await seed.OpenAsync(); + await ExecAsync(seed, "CREATE TABLE schema_version (version INTEGER NOT NULL)"); + await ExecAsync(seed, "INSERT INTO schema_version VALUES (47)"); + await ExecAsync(seed, @"CREATE TABLE server_properties ( + server_id INTEGER NOT NULL, + collection_time TIMESTAMP NOT NULL, + cpu_count INTEGER NOT NULL, + hyperthread_ratio INTEGER NOT NULL, + physical_memory_mb BIGINT NOT NULL + )"); + await ExecAsync(seed, "INSERT INTO server_properties VALUES (1, current_timestamp, 4, 1, 16384)"); + /* The real dependent object: DuckDbSchemaGenerator.CreateIndex's default case for any + collector table, including server_properties, is exactly this index/column shape. */ + await ExecAsync(seed, "CREATE INDEX idx_server_properties_time ON server_properties(server_id, collection_time)"); + } + + var initializer = new DuckDbInitializer(_dbPath); + await initializer.InitializeAsync(); + + using var verify = new DuckDBConnection($"Data Source={_dbPath}"); + await verify.OpenAsync(); + + /* The real assertion: a permission-free collector row (NULL hardware columns) must actually be + insertable now. Before the fix, the dependency error silently left the NOT NULL constraint in + place, so this insert would throw — the exact failure mode #2748's v48 fix exists to prevent. */ + await ExecAsync(verify, "INSERT INTO server_properties VALUES (2, current_timestamp, NULL, NULL, NULL)"); + + using var countCmd = verify.CreateCommand(); + countCmd.CommandText = "SELECT COUNT(*) FROM server_properties WHERE server_id = 2 AND cpu_count IS NULL"; + Assert.Equal(1L, Convert.ToInt64(await countCmd.ExecuteScalarAsync())); + } + + /// + /// v53 adds two columns to config_database_state_expected via ALTER TABLE. That table was never + /// given its own numbered migration — it only exists because Schema.GetAllTableStatements() + /// unconditionally creates it, which does not run until AFTER migrations. A database old enough to + /// predate the table (introduced by #2166/#2203, well after v47) hits the ALTER before the table + /// exists at all. Seeds a v47 database with NO config_database_state_expected table, and asserts + /// the upgrade both completes AND the table exists afterward with both new columns present and + /// actually writable. + /// + [Fact] + public async Task UpgradeFromV47_CreatesConfigDatabaseStateExpected_WhenItPredatesTheTable() + { + using (var seed = new DuckDBConnection($"Data Source={_dbPath}")) + { + await seed.OpenAsync(); + await ExecAsync(seed, "CREATE TABLE schema_version (version INTEGER NOT NULL)"); + await ExecAsync(seed, "INSERT INTO schema_version VALUES (47)"); + /* Deliberately absent: config_database_state_expected. #2748's real-world database was old + enough that this table had never been created — that is the entire bug. */ + } + + var initializer = new DuckDbInitializer(_dbPath); + await initializer.InitializeAsync(); + + using var verify = new DuckDBConnection($"Data Source={_dbPath}"); + await verify.OpenAsync(); + + /* The real assertion: the columns v53 exists to add must actually be writable afterward — this + is what DuckDbAlertHistoryStore's UPDATE (the database-state alert's edge-trigger memory) + depends on, and what #2748's user's app crashed trying to do. */ + await ExecAsync(verify, + "INSERT INTO config_database_state_expected (server_id, database_name, expected_state, last_alerted_state, last_alerted_at) " + + "VALUES (1, 'TestDb', 'ONLINE', 'ONLINE', current_timestamp)"); + + using var countCmd = verify.CreateCommand(); + countCmd.CommandText = "SELECT COUNT(*) FROM config_database_state_expected WHERE server_id = 1 AND database_name = 'TestDb'"; + Assert.Equal(1L, Convert.ToInt64(await countCmd.ExecuteScalarAsync())); + } + + private static async Task ExecAsync(DuckDBConnection connection, string sql) + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = sql; + await cmd.ExecuteNonQueryAsync(); + } +} diff --git a/Lite/Database/DuckDbInitializer.cs b/Lite/Database/DuckDbInitializer.cs index 304854329..999d1c02d 100644 --- a/Lite/Database/DuckDbInitializer.cs +++ b/Lite/Database/DuckDbInitializer.cs @@ -1289,6 +1289,25 @@ database has to have the constraint dropped. New databases get it from the generator. Column types and ordinals are unchanged, so the positional appender and old parquet are unaffected. */ _logger?.LogInformation("Running migration to v48: server_properties hardware columns become nullable"); + + /* #2748: on any database that has ever completed a prior startup, DuckDbSchemaGenerator.CreateIndex's + default case already created idx_server_properties_time ON server_properties(server_id, + collection_time) — a real catalog object persisted in the .duckdb file, surviving a restart. + DuckDB's ALTER COLUMN dependency check refuses to touch a table with ANY index on it, even one + that names none of the altered columns — confirmed empirically, not merely by reading the error + text: "Dependency Error: Cannot alter entry ... because there are entries that depend on it." + (An archive view on the table does NOT trigger this — only the index does.) Drop the index + first; Schema.GetAllIndexStatements()'s loop (called unconditionally right after migrations, + inside this same InitializeAsync) recreates it, so nothing is left dangling. */ + try + { + await ExecuteNonQueryAsync(connection, "DROP INDEX IF EXISTS idx_server_properties_time"); + } + catch (Exception ex) + { + _logger?.LogWarning("Migration to v48 could not drop idx_server_properties_time ahead of the ALTERs (non-fatal, the ALTERs below may still fail): {Error}", ex.Message); + } + foreach (var column in new[] { "cpu_count", "hyperthread_ratio", "physical_memory_mb" }) { try @@ -1384,12 +1403,24 @@ CreateArchiveViewsAsync via ArchivableTables. */ _logger?.LogInformation("Running migration to v53: adding the alerted-state memory to config_database_state_expected"); try { + /* #2748: config_database_state_expected itself was never given its own numbered migration — + it only exists because Schema.GetAllTableStatements() unconditionally CREATE TABLE IF NOT + EXISTS-es it, which does not run until AFTER RunMigrationsAsync returns. A database old + enough to predate the table (upgrading through v53 for the first time) hits this ALTER + before that later step ever creates it. CreateDatabaseStateExpectedTable is itself + idempotent, so calling it here is a no-op for anyone who already has the table (from a + prior run) and a correct fresh create — new columns included — for anyone who does not. */ + await ExecuteNonQueryAsync(connection, Schema.CreateDatabaseStateExpectedTable); await ExecuteNonQueryAsync(connection, "ALTER TABLE config_database_state_expected ADD COLUMN IF NOT EXISTS last_alerted_state VARCHAR"); await ExecuteNonQueryAsync(connection, "ALTER TABLE config_database_state_expected ADD COLUMN IF NOT EXISTS last_alerted_at TIMESTAMP"); } - catch + catch (Exception ex) { - /* Table doesn't exist yet — will be created with the full schema below */ + /* The CREATE above means "table doesn't exist yet" can no longer be the cause — a catch here + now means something else went wrong. Log it rather than swallow it silently, matching every + sibling migration block; this whole PR exists because a silently-swallowed failure here is + exactly what left #2748's database unfixed for two releases. */ + _logger?.LogWarning("Migration to v53 encountered an error (non-fatal): {Error}", ex.Message); } }