From 951cd4fff888928e3e51f70bbcf30de132c9c0d8 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:13:36 +0100 Subject: [PATCH 1/2] Fix two aged-database migration bugs behind #2748's Lite startup failure Root-caused both errors from the reported v47->v56 upgrade log against real DuckDB (1.5.5, matching this repo's pin), not just the error text: - v48 (server_properties NOT NULL relaxation): fails on any database that completed a prior startup, because DuckDbSchemaGenerator's default-case index (idx_server_properties_time) already exists and DuckDB's ALTER COLUMN dependency check refuses a table with ANY index on it, even one naming none of the altered columns. Confirmed empirically that a plain archive VIEW does NOT trigger this - only the index does; an earlier attempt at this fix targeted the view and was wrong. Fix: drop the index before the ALTERs, let the schema loop's unconditional index-creation step (already runs right after migrations) recreate it. - v53 (config_database_state_expected new columns): fails on any database old enough to predate the table's introduction (#2166/ #2203), because the table has no numbered CREATE migration of its own - it only exists via Schema.GetAllTableStatements()'s unconditional CREATE TABLE IF NOT EXISTS, which does not run until after RunMigrationsAsync returns. v54's migration already does this correctly (CREATE-then-ALTER); v53 never did. Fix: call the same idempotent CreateDatabaseStateExpectedTable DDL before the ALTERs, matching v54's established pattern. Both fixes validated against a standalone DuckDB.NET 1.5.5 repro reproducing the exact logged error text before the fix and confirming success after. Added AgedDatabaseMigrationTests.cs seeding both real preconditions (an aged NOT NULL server_properties + its real default index; a v47 database missing config_database_state_expected entirely) against the actual DuckDbInitializer.InitializeAsync path. Neither bug is confirmed as the actual "app does not start" failure - v53's failure self-heals moments later regardless (the unconditional CREATE TABLE IF NOT EXISTS runs right after migrations either way), and v48's collector writes happen on a fire-and-forget background task that would not surface as a startup-blocking exception. Both are real, independently worth fixing, and match the two errors the user's log actually shows - but the log cuts off right after migrations complete, before whatever actually blocked startup. Commenting on the issue to ask for the rest of the log rather than closing it. Part of #2748. Co-Authored-By: Claude Sonnet 5 --- Lite.Tests/AgedDatabaseMigrationTests.cs | 134 +++++++++++++++++++++++ Lite/Database/DuckDbInitializer.cs | 27 +++++ 2 files changed, 161 insertions(+) create mode 100644 Lite.Tests/AgedDatabaseMigrationTests.cs 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..e04176129 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,6 +1403,14 @@ 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"); } From 0ebf8e880499028e26bd10664200583616c61c64 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:24:28 +0100 Subject: [PATCH 2/2] Log the v53 migration's catch instead of swallowing it silently Review catch on #2750: the CREATE TABLE IF NOT EXISTS just added means "table doesn't exist yet" can no longer be why this catch fires - a catch here now means something else went wrong. Log it like every sibling migration block does, rather than repeat the exact silently-swallowed-failure shape this PR exists to fix. Co-Authored-By: Claude Sonnet 5 --- Lite/Database/DuckDbInitializer.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Lite/Database/DuckDbInitializer.cs b/Lite/Database/DuckDbInitializer.cs index e04176129..999d1c02d 100644 --- a/Lite/Database/DuckDbInitializer.cs +++ b/Lite/Database/DuckDbInitializer.cs @@ -1414,9 +1414,13 @@ before that later step ever creates it. CreateDatabaseStateExpectedTable is itse 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); } }