diff --git a/Darling/Darling.Tests/QsCaptureModeRouteKnobToastRungTests.cs b/Darling/Darling.Tests/QsCaptureModeRouteKnobToastRungTests.cs index 47b949343b..abe0e65673 100644 --- a/Darling/Darling.Tests/QsCaptureModeRouteKnobToastRungTests.cs +++ b/Darling/Darling.Tests/QsCaptureModeRouteKnobToastRungTests.cs @@ -391,9 +391,20 @@ being the ceiling. */ Assert.True(int.Parse(versionMatch.Groups[1].Value) >= 64); var start = initializer.IndexOf("if (fromVersion < 64)", StringComparison.Ordinal); Assert.True(start >= 0, "DuckDbInitializer has no v64 block"); - var block = initializer[start..]; - Assert.Contains("(\"query_store_health\", \"query_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); - Assert.Contains("(\"query_store_health\", \"wait_stats_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); + /* #4727: the two columns are entries of ONE versioned list, DuckDbInitializer.AddedColumns (version first), + and the v64 step adds its own version's entries through the shared AddMissingColumnsAsync. The step is cut + at the next "if (fromVersion <" so a call in a later step cannot satisfy the pin. */ + var end = initializer.IndexOf("if (fromVersion <", start + 1, StringComparison.Ordinal); + Assert.True(end > start, "DuckDbInitializer's v64 block has no later step to end at"); + var block = initializer[start..end]; + var listStart = initializer.IndexOf("internal static readonly (int Version, string Table, string Column, string Type)[] AddedColumns =", StringComparison.Ordinal); + Assert.True(listStart >= 0, "DuckDbInitializer no longer declares AddedColumns in the pinned shape"); + var listEnd = initializer.IndexOf("};", listStart, StringComparison.Ordinal); + Assert.True(listEnd > listStart, "DuckDbInitializer's AddedColumns list has no closing brace to end at"); + var list = initializer[listStart..listEnd]; + Assert.Contains("(64, \"query_store_health\", \"query_capture_mode\", \"VARCHAR\")", list, StringComparison.Ordinal); + Assert.Contains("(64, \"query_store_health\", \"wait_stats_capture_mode\", \"VARCHAR\")", list, StringComparison.Ordinal); + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(64))", block, StringComparison.Ordinal); Assert.Contains("Running migration to v64", block, StringComparison.Ordinal); Assert.Contains("twinning Darling's V137", block, StringComparison.Ordinal); diff --git a/Darling/Darling.Tests/TimeHonestyRungTests.cs b/Darling/Darling.Tests/TimeHonestyRungTests.cs index 708255164a..95b344fdfc 100644 --- a/Darling/Darling.Tests/TimeHonestyRungTests.cs +++ b/Darling/Darling.Tests/TimeHonestyRungTests.cs @@ -205,9 +205,23 @@ public void TheLiteTwinIsSchemaV63_AndCarriesBothColumns() var declaration = System.Text.RegularExpressions.Regex.Match(lite, @"internal const int CurrentSchemaVersion = (\d+);"); Assert.True(declaration.Success, "DuckDbInitializer no longer declares CurrentSchemaVersion in the pinned shape"); Assert.True(int.Parse(declaration.Groups[1].Value, CultureInfo.InvariantCulture) >= 63, "Lite's schema version fell below the v63 twin"); - var block = lite[lite.IndexOf("if (fromVersion < 63)", StringComparison.Ordinal)..]; - Assert.Contains("(\"cpu_utilization_stats\", \"sample_time_utc\", \"TIMESTAMP\")", block, StringComparison.Ordinal); - Assert.Contains("(\"server_properties\", \"time_zone_id\", \"VARCHAR\")", block, StringComparison.Ordinal); + /* #4727: Lite's added columns are entries of ONE versioned list, DuckDbInitializer.AddedColumns (version + first), and each migration step adds its own version's entries through the shared AddMissingColumnsAsync. + So the two v63 columns are pinned as entries of that list, and the v63 step, cut at the next + "if (fromVersion <" so a call in a later step cannot satisfy the pin, as the one that adds version 63's. */ + var listStart = lite.IndexOf("internal static readonly (int Version, string Table, string Column, string Type)[] AddedColumns =", StringComparison.Ordinal); + Assert.True(listStart >= 0, "DuckDbInitializer no longer declares AddedColumns in the pinned shape"); + var listEnd = lite.IndexOf("};", listStart, StringComparison.Ordinal); + Assert.True(listEnd > listStart, "DuckDbInitializer's AddedColumns list has no closing brace to end at"); + var list = lite[listStart..listEnd]; + Assert.Contains("(63, \"cpu_utilization_stats\", \"sample_time_utc\", \"TIMESTAMP\")", list, StringComparison.Ordinal); + Assert.Contains("(63, \"server_properties\", \"time_zone_id\", \"VARCHAR\")", list, StringComparison.Ordinal); + var start = lite.IndexOf("if (fromVersion < 63)", StringComparison.Ordinal); + Assert.True(start >= 0, "DuckDbInitializer has no v63 block"); + var end = lite.IndexOf("if (fromVersion <", start + 1, StringComparison.Ordinal); + Assert.True(end > start, "DuckDbInitializer's v63 block has no later step to end at"); + var block = lite[start..end]; + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(63))", block, StringComparison.Ordinal); Assert.Contains("twinning Darling's V134", block, StringComparison.Ordinal); } diff --git a/Lite.Tests/CollectionCycleResumeTests.cs b/Lite.Tests/CollectionCycleResumeTests.cs new file mode 100644 index 0000000000..ba0fd2b2a5 --- /dev/null +++ b/Lite.Tests/CollectionCycleResumeTests.cs @@ -0,0 +1,129 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using PerformanceMonitorLite.Models; +using PerformanceMonitorLite.Services; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #4728: after the computer sleeps and resumes, the collection loop's delay returns long after the slot it waited +/// for. Handing the collectors that stale slot ran every collector that was due then, and ran it again at the next +/// slot a minute later. The wait returns the latest grid slot at or before now instead, so each due collector runs +/// once. The simulation drives the production wait () +/// with a fake clock and a fake delay, and 's mark-and-due for the collectors, the way +/// RemoteCollectorService marks a run with the cycle's slot. +/// +public sealed class CollectionCycleResumeTests : IDisposable +{ + private static readonly DateTime T0 = new(2026, 9, 29, 10, 0, 0, DateTimeKind.Utc); + private static readonly TimeSpan Interval = TimeSpan.FromMinutes(1); + private const string ServerId = "resume-server"; + private const string EveryMinute = "every_minute"; + private const string EveryFive = "every_five"; + private const string EveryFifteen = "every_fifteen"; + + private readonly string _configDir; + + public CollectionCycleResumeTests() + { + _configDir = Path.Combine(Path.GetTempPath(), "CollectionCycleResumeTests_" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(_configDir); + } + + public void Dispose() + { + try { if (Directory.Exists(_configDir)) Directory.Delete(_configDir, recursive: true); } + catch { /* best-effort cleanup */ } + } + + private sealed record Run(string Collector, DateTime Slot, DateTime WallClock); + + /// + /// Runs loop passes from slot T0. The delay after pass + /// comes back late, the way a resume from sleep does (-1 for no sleep). + /// + private async Task> SimulateAsync(int iterations, int sleepAfterPass, TimeSpan sleep) + { + var schedule = new ScheduleManager(_configDir); + schedule.SetScheduleForServer(ServerId, new List + { + new() { Name = EveryMinute, Enabled = true, FrequencyMinutes = 1 }, + new() { Name = EveryFive, Enabled = true, FrequencyMinutes = 5 }, + new() { Name = EveryFifteen, Enabled = true, FrequencyMinutes = 15 }, + }); + + var now = T0; + var cycleStart = T0; + var runs = new List(); + for (var pass = 0; pass < iterations; pass++) + { + foreach (var due in schedule.GetDueCollectorsForServer(ServerId, cycleStart)) + { + runs.Add(new Run(due.Name, cycleStart, now)); + schedule.MarkCollectorRunForServer(ServerId, due.Name, cycleStart); + } + + var thisPass = pass; + cycleStart = await CollectionBackgroundService.WaitForNextCycleAsync( + cycleStart, + Interval, + () => now, + (wait, _) => + { + now += wait; + if (thisPass == sleepAfterPass) + now += sleep; + return Task.CompletedTask; + }, + CancellationToken.None); + } + + return runs; + } + + /// + /// A 47-minute sleep begins while the loop waits for the slot at which the 5-minute collector becomes due. Each + /// due collector runs once after the resume: the 5-minute one is not run again a minute later (the old shape ran + /// it at the stale slot and again at the next one, 60 s apart), and the resume slot is on the grid. + /// + [Fact] + public async Task AfterASleepAndResume_EachDueCollectorRunsOnce_NotTwiceAMinuteApart() + { + var sleep = TimeSpan.FromMinutes(47); + var runs = await SimulateAsync(iterations: 12, sleepAfterPass: 4, sleep); + + var resumeWall = T0 + TimeSpan.FromMinutes(5) + sleep; + var afterResume = runs.Where(r => r.WallClock >= resumeWall).ToList(); + + /* The slot handed to the collectors is the latest grid slot at or before now, not the one from before the sleep. */ + Assert.All(afterResume, r => Assert.Equal(r.WallClock, r.Slot)); + + var five = afterResume.Where(r => r.Collector == EveryFive).ToList(); + Assert.Equal(new[] { resumeWall, resumeWall + TimeSpan.FromMinutes(5) }, five.Select(r => r.WallClock).ToArray()); + + var fifteen = afterResume.Where(r => r.Collector == EveryFifteen).ToList(); + Assert.Equal(new[] { resumeWall }, fifteen.Select(r => r.WallClock).ToArray()); + + /* A 1-minute collector still runs at the resume slot and at the next one. */ + var minute = afterResume.Where(r => r.Collector == EveryMinute).Select(r => r.Slot).ToList(); + Assert.Contains(resumeWall, minute); + Assert.Contains(resumeWall + Interval, minute); + } + + /// Without a sleep the grid is unchanged: each collector runs at its own cadence, at exact slots. + [Fact] + public async Task WithoutASleep_EachCollectorRunsAtItsOwnCadenceOnTheGrid() + { + var runs = await SimulateAsync(iterations: 16, sleepAfterPass: -1, TimeSpan.Zero); + + Assert.Equal(16, runs.Count(r => r.Collector == EveryMinute)); + Assert.Equal(new[] { 0, 5, 10, 15 }, runs.Where(r => r.Collector == EveryFive).Select(r => (int)(r.Slot - T0).TotalMinutes).ToArray()); + Assert.Equal(new[] { 0, 15 }, runs.Where(r => r.Collector == EveryFifteen).Select(r => (int)(r.Slot - T0).TotalMinutes).ToArray()); + Assert.All(runs, r => Assert.Equal(r.WallClock, r.Slot)); + } +} diff --git a/Lite.Tests/DeltaFamilyIntervalColumnTests.cs b/Lite.Tests/DeltaFamilyIntervalColumnTests.cs index c83dfcc487..599a8dc80a 100644 --- a/Lite.Tests/DeltaFamilyIntervalColumnTests.cs +++ b/Lite.Tests/DeltaFamilyIntervalColumnTests.cs @@ -184,12 +184,16 @@ public void TheLiteMigration_AddsTheColumnToAllFour_AtSchemaVersion60() var source = RepoSource.Read("Lite", "Database", "DuckDbInitializer.cs"); Assert.Contains("if (fromVersion < 60)", source, StringComparison.Ordinal); - Assert.Contains( - "foreach (var table in new[] { \"wait_stats\", \"file_io_stats\", \"latch_stats\", \"spinlock_stats\" })", - source, StringComparison.Ordinal); - Assert.Contains( - "$\"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS sample_interval_seconds INTEGER\"", - source, StringComparison.Ordinal); + + /* #4727: the entries moved out of the step into DuckDbInitializer.AddedColumns; the step adds its own + version's entries through the shared AddMissingColumnsAsync, one idempotent ADD COLUMN per entry. */ + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(60))", source, StringComparison.Ordinal); + foreach (var table in new[] { "wait_stats", "file_io_stats", "latch_stats", "spinlock_stats" }) + { + Assert.Contains($"(60, \"{table}\", \"sample_interval_seconds\", \"INTEGER\")", source, StringComparison.Ordinal); + } + + Assert.Contains("ADD COLUMN IF NOT EXISTS {column} {type}", source, StringComparison.Ordinal); } /// @@ -207,18 +211,21 @@ public void TheLiteMigration_CompletesTheIntervalAndStoresTheOffsets_AtSchemaVer var source = RepoSource.Read("Lite", "Database", "DuckDbInitializer.cs"); var block = source[source.IndexOf("if (fromVersion < 61)", StringComparison.Ordinal)..]; - foreach (var pair in new[] + /* #4727: the four entries live in DuckDbInitializer.AddedColumns (INTEGER each); the step adds its own + version's entries through the shared AddMissingColumnsAsync. */ + foreach (var entry in new[] { - "(\"procedure_stats\", \"sample_interval_seconds\")", - "(\"memory_grant_stats\", \"sample_interval_seconds\")", - "(\"query_stats\", \"statement_start_offset\")", - "(\"query_stats\", \"statement_end_offset\")", + "(61, \"procedure_stats\", \"sample_interval_seconds\", \"INTEGER\")", + "(61, \"memory_grant_stats\", \"sample_interval_seconds\", \"INTEGER\")", + "(61, \"query_stats\", \"statement_start_offset\", \"INTEGER\")", + "(61, \"query_stats\", \"statement_end_offset\", \"INTEGER\")", }) { - Assert.Contains(pair, block, StringComparison.Ordinal); + Assert.Contains(entry, block, StringComparison.Ordinal); } - Assert.Contains("$\"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS {column} INTEGER\"", block, StringComparison.Ordinal); + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(61))", block, StringComparison.Ordinal); + Assert.Contains("ADD COLUMN IF NOT EXISTS {column} {type}", block, StringComparison.Ordinal); /* The offsets' semantics, in the block, in these words: the pair a future reader second-guesses. */ Assert.Contains("BYTES", block, StringComparison.Ordinal); diff --git a/Lite.Tests/NewerSchemaFileGuardTests.cs b/Lite.Tests/NewerSchemaFileGuardTests.cs new file mode 100644 index 0000000000..1660296931 --- /dev/null +++ b/Lite.Tests/NewerSchemaFileGuardTests.cs @@ -0,0 +1,97 @@ +using System; +using System.IO; +using System.Security.Cryptography; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using PerformanceMonitorLite.Database; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #4727: a data file stamped with a schema version newer than the app's is refused, with a message naming both +/// versions, and nothing is written to it. A read-only open would not be enough: the collectors would still fail. +/// +public class NewerSchemaFileGuardTests : IDisposable +{ + private readonly string _tempDir; + private readonly string _dbPath; + + public NewerSchemaFileGuardTests() + { + _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 */ + } + } + + [Fact] + public async Task AFileStampedOneVersionAboveTheApp_IsRefused_AndNothingIsWrittenToIt() + { + var newer = DuckDbInitializer.CurrentSchemaVersion + 1; + + 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 ({newer})"); + } + + var bytesBefore = SHA256.HashData(File.ReadAllBytes(_dbPath)); + var writtenBefore = File.GetLastWriteTimeUtc(_dbPath); + + var initializer = new DuckDbInitializer(_dbPath); + var refusal = await Assert.ThrowsAsync(() => initializer.InitializeAsync()); + + Assert.Equal(newer, refusal.FileVersion); + Assert.Equal(DuckDbInitializer.CurrentSchemaVersion, refusal.AppVersion); + Assert.Contains($"schema version {newer}", refusal.Message, StringComparison.Ordinal); + Assert.Contains($"schema version {DuckDbInitializer.CurrentSchemaVersion}", refusal.Message, StringComparison.Ordinal); + + Assert.Equal(bytesBefore, SHA256.HashData(File.ReadAllBytes(_dbPath))); + Assert.Equal(writtenBefore, File.GetLastWriteTimeUtc(_dbPath)); + Assert.False(File.Exists(_dbPath + ".wal"), "a refused open must not leave a write-ahead log behind"); + + /* And no table was created: the stamp table is still the only thing in the file. */ + using var verify = new DuckDBConnection($"Data Source={_dbPath};ACCESS_MODE=READ_ONLY"); + await verify.OpenAsync(); + using var count = verify.CreateCommand(); + count.CommandText = "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema = 'main'"; + Assert.Equal(1L, Convert.ToInt64(await count.ExecuteScalarAsync())); + } + + [Fact] + public async Task AFileStampedAtTheAppsOwnVersion_StillOpens() + { + var first = new DuckDbInitializer(_dbPath); + await first.InitializeAsync(); + + var second = new DuckDbInitializer(_dbPath); + await second.InitializeAsync(); + + using var verify = new DuckDBConnection($"Data Source={_dbPath}"); + await verify.OpenAsync(); + using var version = verify.CreateCommand(); + version.CommandText = "SELECT MAX(version) FROM schema_version"; + Assert.Equal((long)DuckDbInitializer.CurrentSchemaVersion, Convert.ToInt64(await version.ExecuteScalarAsync())); + } + + private static async Task ExecAsync(DuckDBConnection connection, string sql) + { + using var command = connection.CreateCommand(); + command.CommandText = sql; + await command.ExecuteNonQueryAsync(); + } +} diff --git a/Lite.Tests/PerfmonCounterTypeTests.cs b/Lite.Tests/PerfmonCounterTypeTests.cs index 150f9e57c1..869f230a4c 100644 --- a/Lite.Tests/PerfmonCounterTypeTests.cs +++ b/Lite.Tests/PerfmonCounterTypeTests.cs @@ -128,7 +128,11 @@ public void TheLiteMigration_StoresTheCounterType_AtSchemaVersion62() Assert.True(start >= 0, "DuckDbInitializer has no v62 block"); var block = source[start..]; - Assert.Contains("\"ALTER TABLE perfmon_stats ADD COLUMN IF NOT EXISTS cntr_type INTEGER\"", block, StringComparison.Ordinal); + /* #4727: the entry lives in DuckDbInitializer.AddedColumns; the step adds it through the shared + AddMissingColumnsAsync, one idempotent ADD COLUMN IF NOT EXISTS. */ + Assert.Contains("(62, \"perfmon_stats\", \"cntr_type\", \"INTEGER\")", block, StringComparison.Ordinal); + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(62))", block, StringComparison.Ordinal); + Assert.Contains("ADD COLUMN IF NOT EXISTS {column} {type}", block, StringComparison.Ordinal); Assert.Contains("Running migration to v62", block, StringComparison.Ordinal); foreach (var phrase in new[] { "COUNTS", "LEVELS", "falling level", "counter reset", "NULL delta and NULL interval", "name-suffix proxy", "Appended at the end", "Nothing to backfill", "REQUIRED on this side" }) { diff --git a/Lite.Tests/QsCaptureModeRungTests.cs b/Lite.Tests/QsCaptureModeRungTests.cs index 8fe66123da..d30c9d625e 100644 --- a/Lite.Tests/QsCaptureModeRungTests.cs +++ b/Lite.Tests/QsCaptureModeRungTests.cs @@ -61,8 +61,11 @@ public void TheLiteMigration_StoresBothCaptureModes_AtSchemaVersion64() Assert.True(start >= 0, "DuckDbInitializer has no v64 block"); var block = source[start..]; - Assert.Contains("(\"query_store_health\", \"query_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); - Assert.Contains("(\"query_store_health\", \"wait_stats_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); + /* #4727: the entries live in DuckDbInitializer.AddedColumns; the step adds its own version's entries through + the shared AddMissingColumnsAsync. */ + Assert.Contains("(64, \"query_store_health\", \"query_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); + Assert.Contains("(64, \"query_store_health\", \"wait_stats_capture_mode\", \"VARCHAR\")", block, StringComparison.Ordinal); + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(64))", block, StringComparison.Ordinal); Assert.Contains("ADD COLUMN IF NOT EXISTS {column} {type}", block, StringComparison.Ordinal); Assert.Contains("Running migration to v64", block, StringComparison.Ordinal); foreach (var phrase in new[] diff --git a/Lite.Tests/SchemaColumnHealingTests.cs b/Lite.Tests/SchemaColumnHealingTests.cs new file mode 100644 index 0000000000..6f61172550 --- /dev/null +++ b/Lite.Tests/SchemaColumnHealingTests.cs @@ -0,0 +1,199 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using Microsoft.Extensions.Logging; +using PerformanceMonitorLite.Database; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #4727 item 2: the columns that schema versions 60 to 65 add are re-applied on every start of an existing +/// data file, so a file where one failed to apply (or that was stamped without it) gets it back instead of +/// failing every batch for that table. The stamp is not held back: the heal does not depend on it. +/// +public sealed class SchemaColumnHealingTests : IDisposable +{ + /// The 15 columns versions 60 to 65 add, spelled out here on purpose so a trimmed production list fails. + private static readonly (string Table, string Column, string Type)[] NewerColumns = + { + ("wait_stats", "sample_interval_seconds", "INTEGER"), + ("file_io_stats", "sample_interval_seconds", "INTEGER"), + ("latch_stats", "sample_interval_seconds", "INTEGER"), + ("spinlock_stats", "sample_interval_seconds", "INTEGER"), + ("procedure_stats", "sample_interval_seconds", "INTEGER"), + ("memory_grant_stats", "sample_interval_seconds", "INTEGER"), + ("query_stats", "statement_start_offset", "INTEGER"), + ("query_stats", "statement_end_offset", "INTEGER"), + ("perfmon_stats", "cntr_type", "INTEGER"), + ("cpu_utilization_stats", "sample_time_utc", "TIMESTAMP"), + ("server_properties", "time_zone_id", "VARCHAR"), + ("query_store_health", "query_capture_mode", "VARCHAR"), + ("query_store_health", "wait_stats_capture_mode", "VARCHAR"), + ("ag_replica_states", "group_id", "VARCHAR"), + ("ag_database_replica_states", "group_id", "VARCHAR"), + }; + + private readonly string _tempDir; + private readonly string _dbPath; + + public SchemaColumnHealingTests() + { + _tempDir = Path.Combine(Path.GetTempPath(), "SchemaColumnHealingTests_" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(_tempDir); + _dbPath = Path.Combine(_tempDir, "lite.duckdb"); + } + + public void Dispose() + { + try { if (Directory.Exists(_tempDir)) Directory.Delete(_tempDir, recursive: true); } + catch { /* best-effort cleanup */ } + } + + [Fact] + public async Task AFileStampedCurrent_ButMissingTheNewerColumns_GetsEveryOneBackOnTheNextStart() + { + using (var first = new DuckDbInitializer(_dbPath)) + { + await first.InitializeAsync(); + } + + using (var conn = new DuckDBConnection($"Data Source={_dbPath}")) + { + await conn.OpenAsync(); + + /* DuckDB refuses DROP COLUMN on a table that has an index (a Dependency Error); a view over the table + does not block it. So only the indexes go, and they are put straight back with the columns gone: + the file then looks the way a real one does when a column add failed, with its indexes and its v_ + views all standing and the stamp at the current version. */ + foreach (var table in NewerColumns.Select(c => c.Table).Distinct()) + { + foreach (var index in await ListAsync(conn, $"SELECT index_name FROM duckdb_indexes() WHERE table_name = '{table}'")) + await ExecAsync(conn, $"DROP INDEX IF EXISTS {index}"); + foreach (var (_, column, _) in NewerColumns.Where(c => c.Table == table)) + await ExecAsync(conn, $"ALTER TABLE {table} DROP COLUMN {column}"); + } + + foreach (var statement in Schema.GetAllIndexStatements()) + await ExecAsync(conn, statement); + + /* The arrange step holds: every column gone, every index and view back, the stamp current. */ + foreach (var (table, column, _) in NewerColumns) + { + Assert.Equal(0L, await CountAsync(conn, $"SELECT COUNT(*) FROM information_schema.columns WHERE table_name = '{table}' AND column_name = '{column}'")); + Assert.True(await CountAsync(conn, $"SELECT COUNT(*) FROM duckdb_indexes() WHERE table_name = '{table}'") >= 1, $"index on {table}"); + Assert.Equal(1L, await CountAsync(conn, $"SELECT COUNT(*) FROM duckdb_views() WHERE NOT internal AND view_name = 'v_{table}'")); + } + + Assert.Equal((long)DuckDbInitializer.CurrentSchemaVersion, await CountAsync(conn, "SELECT MAX(version) FROM schema_version")); + } + + using (var second = new DuckDbInitializer(_dbPath)) + { + await second.InitializeAsync(); + } + + using (var conn = new DuckDBConnection($"Data Source={_dbPath}")) + { + await conn.OpenAsync(); + foreach (var (table, column, type) in NewerColumns) + { + var types = await ListAsync(conn, $"SELECT data_type FROM information_schema.columns WHERE table_name = '{table}' AND column_name = '{column}'"); + Assert.True(types.Count == 1, $"{table}.{column} is back"); + Assert.Equal(type, types[0]); + + /* The passthrough view reads the restored column too (Lite rebuilds every v_ view on start). */ + Assert.Contains(column, await ListAsync(conn, $"SELECT column_name FROM (DESCRIBE SELECT * FROM v_{table})")); + } + + Assert.Equal((long)DuckDbInitializer.CurrentSchemaVersion, await CountAsync(conn, "SELECT MAX(version) FROM schema_version")); + } + } + + /// + /// A column add that still fails logs ONE Error naming the table and column, and the loop moves on to the + /// next entry, so the start continues and the next start retries it. A type DuckDB does not know makes the + /// ALTER itself fail on a table that exists. + /// + [Fact] + public async Task AColumnAddThatFails_LogsOneErrorNamingTheColumn_AndTheNextEntryIsStillAdded() + { + var log = new CapturingLogger(); + using var initializer = new DuckDbInitializer(_dbPath, log); + await initializer.InitializeAsync(); + log.Entries.Clear(); + + using var conn = new DuckDBConnection($"Data Source={_dbPath}"); + await conn.OpenAsync(); + + await initializer.AddMissingColumnsAsync(conn, new[] + { + (0, "wait_stats", "broken_column", "NOT_A_TYPE"), + (0, "wait_stats", "healthy_column", "INTEGER"), + }); + + var error = Assert.Single(log.Entries, e => e.Level == LogLevel.Error); + Assert.Contains("wait_stats.broken_column", error.Message, StringComparison.Ordinal); + Assert.Equal(0L, await CountAsync(conn, "SELECT COUNT(*) FROM information_schema.columns WHERE table_name = 'wait_stats' AND column_name = 'broken_column'")); + Assert.Equal(1L, await CountAsync(conn, "SELECT COUNT(*) FROM information_schema.columns WHERE table_name = 'wait_stats' AND column_name = 'healthy_column'")); + } + + /// + /// A file older than a table has no such table when a migration step runs (the table statements after the + /// steps create it, with the column). That is not a failure and must not read as one in the log. + /// + [Fact] + public async Task ATableThatDoesNotExistYet_IsSkippedWithoutAWarningOrAnError() + { + var log = new CapturingLogger(); + using var initializer = new DuckDbInitializer(_dbPath, log); + await initializer.InitializeAsync(); + log.Entries.Clear(); + + using var conn = new DuckDBConnection($"Data Source={_dbPath}"); + await conn.OpenAsync(); + + await initializer.AddMissingColumnsAsync(conn, new[] { (0, "table_created_later", "some_column", "INTEGER") }); + + Assert.DoesNotContain(log.Entries, e => e.Level >= LogLevel.Warning); + } + + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = new(); + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => Entries.Add((logLevel, formatter(state, exception))); + } + + private static async Task ExecAsync(DuckDBConnection conn, string sql) + { + using var cmd = conn.CreateCommand(); + cmd.CommandText = sql; + await cmd.ExecuteNonQueryAsync(); + } + + private static async Task CountAsync(DuckDBConnection conn, string sql) + { + using var cmd = conn.CreateCommand(); + cmd.CommandText = sql; + return Convert.ToInt64(await cmd.ExecuteScalarAsync()); + } + + private static async Task> ListAsync(DuckDBConnection conn, string sql) + { + var rows = new List(); + using var cmd = conn.CreateCommand(); + cmd.CommandText = sql; + using var reader = await cmd.ExecuteReaderAsync(); + while (await reader.ReadAsync()) rows.Add(reader.GetValue(0)?.ToString() ?? ""); + return rows; + } +} diff --git a/Lite.Tests/TimeHonestyRungTests.cs b/Lite.Tests/TimeHonestyRungTests.cs index 93a4999de7..80de5540d1 100644 --- a/Lite.Tests/TimeHonestyRungTests.cs +++ b/Lite.Tests/TimeHonestyRungTests.cs @@ -59,8 +59,11 @@ public void TheLiteMigration_StoresBothColumns_AtSchemaVersion63() Assert.True(start >= 0, "DuckDbInitializer has no v63 block"); var block = source[start..]; - Assert.Contains("(\"cpu_utilization_stats\", \"sample_time_utc\", \"TIMESTAMP\")", block, StringComparison.Ordinal); - Assert.Contains("(\"server_properties\", \"time_zone_id\", \"VARCHAR\")", block, StringComparison.Ordinal); + /* #4727: the entries live in DuckDbInitializer.AddedColumns; the step adds its own version's entries through + the shared AddMissingColumnsAsync. */ + Assert.Contains("(63, \"cpu_utilization_stats\", \"sample_time_utc\", \"TIMESTAMP\")", block, StringComparison.Ordinal); + Assert.Contains("(63, \"server_properties\", \"time_zone_id\", \"VARCHAR\")", block, StringComparison.Ordinal); + Assert.Contains("AddMissingColumnsAsync(connection, AddedColumnsForVersion(63))", block, StringComparison.Ordinal); Assert.Contains("ADD COLUMN IF NOT EXISTS {column} {type}", block, StringComparison.Ordinal); Assert.Contains("Running migration to v63", block, StringComparison.Ordinal); foreach (var phrase in new[] diff --git a/Lite/Database/DuckDbInitializer.cs b/Lite/Database/DuckDbInitializer.cs index 3c4af710e5..36866539a2 100644 --- a/Lite/Database/DuckDbInitializer.cs +++ b/Lite/Database/DuckDbInitializer.cs @@ -781,11 +781,22 @@ private async Task InitializeCoreAsync() using (connection) { + /* #4727: read the stamp BEFORE anything is created, and refuse a file stamped newer than this build + (an older Lite on the shared data root after a rollback). Versions 60 to 65 added columns to 13 + tables, so carrying on would fail every batch for them with only a log line to show for it. A + missing schema_version table reads as version 0 here, so a fresh file still takes the create path. */ + var existingVersion = await GetSchemaVersionAsync(connection); + if (existingVersion > CurrentSchemaVersion) + { + _logger?.LogError( + "Refusing to open {Path}: schema v{FileVersion} is newer than this build's v{AppVersion}", + _databasePath, existingVersion, CurrentSchemaVersion); + throw new SchemaVersionTooNewException(_databasePath, existingVersion, CurrentSchemaVersion); + } + await ExecuteNonQueryAsync(connection, "CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL)"); - var existingVersion = await GetSchemaVersionAsync(connection); - /* On a fresh/reset database (v0), skip migrations entirely — they DROP tables expecting CREATE TABLE to follow, which is destructive on a blank DB. Just create tables with the current schema and stamp the version. */ @@ -805,6 +816,18 @@ Just create tables with the current schema and stamp the version. */ await ExecuteNonQueryAsync(connection, indexStatement); } + /* #4727: re-apply the columns versions 60 to 65 added on EVERY start of an existing file, after the + table and index statements. A step whose ALTER failed (a transient fault during the one start that + ran it) is stamped done all the same, and a file can also carry the stamp without the column; without + this the table's batches fail on every start from then on. The stamp is not held back: the pass + below heals a stamped file, and a column that still cannot be added logs an Error and lets the + start continue, so the next start retries it. A fresh file gets the columns from the table + statements above. */ + if (existingVersion > 0) + { + await AddMissingColumnsAsync(connection, AddedColumns); + } + if (existingVersion < CurrentSchemaVersion) { await SetSchemaVersionAsync(connection, CurrentSchemaVersion); @@ -2071,18 +2094,7 @@ interval is the ONLY thing that tells those apart. These four collectors discard Non-fatal per table, matching v59's posture. */ _logger?.LogInformation("Running migration to v60: the four naked delta families gain sample_interval_seconds"); - foreach (var table in new[] { "wait_stats", "file_io_stats", "latch_stats", "spinlock_stats" }) - { - try - { - await ExecuteNonQueryAsync(connection, - $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS sample_interval_seconds INTEGER"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v60 on {Table} encountered an error (non-fatal): {Error}", table, ex.Message); - } - } + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(60)); } if (fromVersion < 61) @@ -2127,24 +2139,7 @@ key nothing will ever present. (CreateArchiveViewsAsync, called after this). Non-fatal per statement, matching v59/v60. */ _logger?.LogInformation("Running migration to v61: every delta family stores its interval, and query_stats stores the statement offsets its delta key is made of"); - foreach (var (table, column) in new[] - { - ("procedure_stats", "sample_interval_seconds"), - ("memory_grant_stats", "sample_interval_seconds"), - ("query_stats", "statement_start_offset"), - ("query_stats", "statement_end_offset"), - }) - { - try - { - await ExecuteNonQueryAsync(connection, - $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS {column} INTEGER"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v61 on {Table}.{Column} encountered an error (non-fatal): {Error}", table, column, ex.Message); - } - } + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(61)); } if (fromVersion < 62) @@ -2178,15 +2173,7 @@ rebuilds every v_ view on start (CreateArchiveViewsAsync, called after this). No matching v59/v60/v61. */ _logger?.LogInformation("Running migration to v62: perfmon_stats stores each counter's type, so gauges are no longer differenced"); - try - { - await ExecuteNonQueryAsync(connection, - "ALTER TABLE perfmon_stats ADD COLUMN IF NOT EXISTS cntr_type INTEGER"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v62 on perfmon_stats.cntr_type encountered an error (non-fatal): {Error}", ex.Message); - } + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(62)); } if (fromVersion < 63) @@ -2240,22 +2227,7 @@ parquet are unaffected (v_ views UNION ALL BY NAME the parquet with union_by_nam (CreateArchiveViewsAsync, called after this). Non-fatal per statement, matching v59–v62. */ _logger?.LogInformation("Running migration to v63: cpu_utilization_stats stores each sample's UTC instant beside the server-local one, and server_properties stores the engine's time-zone id beside its offset"); - foreach (var (table, column, type) in new[] - { - ("cpu_utilization_stats", "sample_time_utc", "TIMESTAMP"), - ("server_properties", "time_zone_id", "VARCHAR"), - }) - { - try - { - await ExecuteNonQueryAsync(connection, - $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS {column} {type}"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v63 on {Table}.{Column} encountered an error (non-fatal): {Error}", table, column, ex.Message); - } - } + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(63)); } if (fromVersion < 64) @@ -2293,22 +2265,7 @@ recover a capture mode from the other nine options. Query Store clutter view (#3797) is the consumer. */ _logger?.LogInformation("Running migration to v64: query_store_health stores each database's Query Store capture modes, so plan churn can be told apart from configuration"); - foreach (var (table, column, type) in new[] - { - ("query_store_health", "query_capture_mode", "VARCHAR"), - ("query_store_health", "wait_stats_capture_mode", "VARCHAR"), - }) - { - try - { - await ExecuteNonQueryAsync(connection, - $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS {column} {type}"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v64 on {Table}.{Column} encountered an error (non-fatal): {Error}", table, column, ex.Message); - } - } + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(64)); } if (fromVersion < 65) @@ -2337,21 +2294,96 @@ an existing database and are idempotent. Neither AG table has ever had a v_ pass per statement, matching v59–v64. */ _logger?.LogInformation("Running migration to v65: ag_replica_states and ag_database_replica_states store each Availability Group's engine-assigned id, closing a replica-name-overlap gap in the distinct-group count"); - foreach (var table in new[] + await AddMissingColumnsAsync(connection, AddedColumnsForVersion(65)); + } + } + + /// + /// #4727: the 15 columns that schema versions 60 to 65 add to existing tables, in ONE list. Each migration + /// step adds its own version's entries (), and every start of an existing + /// file runs the whole list once more after the table and index statements, so a column whose add failed + /// during the one start that ran its step, or a file stamped without it, gets it back on the next start. + /// A fresh file gets all of them from the table statements. A later version that adds a column to an + /// existing table adds its entries here. + /// + internal static readonly (int Version, string Table, string Column, string Type)[] AddedColumns = + { + (60, "wait_stats", "sample_interval_seconds", "INTEGER"), + (60, "file_io_stats", "sample_interval_seconds", "INTEGER"), + (60, "latch_stats", "sample_interval_seconds", "INTEGER"), + (60, "spinlock_stats", "sample_interval_seconds", "INTEGER"), + (61, "procedure_stats", "sample_interval_seconds", "INTEGER"), + (61, "memory_grant_stats", "sample_interval_seconds", "INTEGER"), + (61, "query_stats", "statement_start_offset", "INTEGER"), + (61, "query_stats", "statement_end_offset", "INTEGER"), + (62, "perfmon_stats", "cntr_type", "INTEGER"), + (63, "cpu_utilization_stats", "sample_time_utc", "TIMESTAMP"), + (63, "server_properties", "time_zone_id", "VARCHAR"), + (64, "query_store_health", "query_capture_mode", "VARCHAR"), + (64, "query_store_health", "wait_stats_capture_mode", "VARCHAR"), + (65, "ag_replica_states", "group_id", "VARCHAR"), + (65, "ag_database_replica_states", "group_id", "VARCHAR"), + }; + + internal static IEnumerable<(int Version, string Table, string Column, string Type)> AddedColumnsForVersion(int version) => + AddedColumns.Where(c => c.Version == version); + + /// + /// #4727: adds each listed column its table lacks, one idempotent ADD COLUMN IF NOT EXISTS per + /// missing column. The migration steps for versions 60 to 65 call it over their own entries, and + /// calls it over on every start of an existing + /// file. One read finds which tables and columns exist, so a start where nothing is missing runs no ALTER, + /// and a table that does not exist yet (a file older than the table: the table statements that follow + /// create it with the column) is skipped rather than reported as a failure. ADD COLUMN goes through on a + /// table that has an index or a v_ view over it (checked against the bundled DuckDB); only DROP COLUMN and + /// ALTER COLUMN hit the Dependency Error, so no index has to be dropped first. A column that still cannot be + /// added logs one Error naming the table and column and the loop moves on, so the start continues and the + /// next start retries it. + /// + internal async Task AddMissingColumnsAsync(DuckDBConnection connection, IEnumerable<(int Version, string Table, string Column, string Type)> columns) + { + var wanted = columns.ToList(); + if (wanted.Count == 0) + return; + + var tablesPresent = new HashSet(StringComparer.OrdinalIgnoreCase); + var columnsPresent = new HashSet(StringComparer.OrdinalIgnoreCase); + try + { + using var probe = connection.CreateCommand(); + probe.CommandText = "SELECT table_name, column_name FROM information_schema.columns WHERE table_schema = 'main' AND table_name IN (" + + string.Join(", ", wanted.Select(c => $"'{c.Table}'").Distinct()) + ")"; + using var reader = await probe.ExecuteReaderAsync(); + while (await reader.ReadAsync()) + { + tablesPresent.Add(reader.GetString(0)); + columnsPresent.Add(reader.GetString(0) + "." + reader.GetString(1)); + } + } + catch (Exception ex) + { + _logger?.LogError("Could not read which columns exist, so the check for missing schema columns was skipped; the next start retries it: {Error}", ex.Message); + return; + } + + foreach (var (version, table, column, type) in wanted) + { + if (!tablesPresent.Contains(table) || columnsPresent.Contains(table + "." + column)) + continue; + + try { - "ag_replica_states", - "ag_database_replica_states", - }) + /* Not ExecuteNonQueryAsync: that helper logs its own Error with the whole statement before it + rethrows, and a failed add must log exactly ONE Error, the one below that names the column. */ + using var alter = connection.CreateCommand(); + alter.CommandText = $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS {column} {type}"; + await alter.ExecuteNonQueryAsync(); + _logger?.LogInformation("Added missing column {Table}.{Column} {Type} (schema v{Version})", table, column, type, version); + } + catch (Exception ex) { - try - { - await ExecuteNonQueryAsync(connection, - $"ALTER TABLE {table} ADD COLUMN IF NOT EXISTS group_id VARCHAR"); - } - catch (Exception ex) - { - _logger?.LogWarning("Migration to v65 on {Table}.group_id encountered an error (non-fatal): {Error}", table, ex.Message); - } + _logger?.LogError("Adding column {Table}.{Column} {Type} (schema v{Version}) failed; the start continues and the next start retries it: {Error}", + table, column, type, version, ex.Message); } } } diff --git a/Lite/Database/SchemaVersionTooNewException.cs b/Lite/Database/SchemaVersionTooNewException.cs new file mode 100644 index 0000000000..bb32981bbc --- /dev/null +++ b/Lite/Database/SchemaVersionTooNewException.cs @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; + +namespace PerformanceMonitorLite.Database; + +/// +/// #4727: the data file is stamped with a schema version newer than this build understands (an older Lite pointed +/// at the shared data root after a rollback, or an older portable copy). Schema versions 60 to 65 added columns to +/// 13 tables, so an older build that carried on would fail every batch for those tables with only a log line to +/// show for it. throws this before it creates or alters anything, +/// and the startup handler in MainWindow shows to the user. +/// +public sealed class SchemaVersionTooNewException : InvalidOperationException +{ + public SchemaVersionTooNewException(string databasePath, int fileVersion, int appVersion) + : base( + $"The data file {databasePath} was written by a newer version of Performance Monitor Lite. " + + $"It is at schema version {fileVersion}, and this version only understands up to schema version {appVersion}. " + + "The file has not been changed. Update Performance Monitor Lite to the newer version, " + + "or start this one against a different data folder.") + { + FileVersion = fileVersion; + AppVersion = appVersion; + } + + /// The schema version the data file is stamped with. + public int FileVersion { get; } + + /// The newest schema version this build understands. + public int AppVersion { get; } +} diff --git a/Lite/Services/CollectionBackgroundService.cs b/Lite/Services/CollectionBackgroundService.cs index 4a27a6c08b..e56f7dfd7b 100644 --- a/Lite/Services/CollectionBackgroundService.cs +++ b/Lite/Services/CollectionBackgroundService.cs @@ -245,14 +245,8 @@ property reads — negligible overhead at 1-minute cadence. */ try { - var nextCycle = CollectorCadence.NextDue(cycleStart, DateTime.UtcNow, CollectionInterval); - var wait = nextCycle - DateTime.UtcNow; - if (wait > TimeSpan.Zero) - { - await Task.Delay(wait, stoppingToken); - } - - cycleStart = nextCycle; + cycleStart = await WaitForNextCycleAsync(cycleStart, CollectionInterval, () => DateTime.UtcNow, + (wait, token) => Task.Delay(wait, token), stoppingToken); } catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) { @@ -263,6 +257,36 @@ property reads — negligible overhead at 1-minute cadence. */ _logger?.LogInformation("Collection background service stopped"); } + /// + /// #4728: waits for the next slot on the collector grid and returns the logical start of the cycle to run. + /// The wait is normally one interval, and becomes the slot it waited for. After + /// the computer sleeps and resumes, though, the delay returns long after that slot, and handing the collectors + /// a slot from before the sleep would run every collector that was due then and run it again at the next slot + /// a minute later. When the delay returns a whole interval or more past its slot, the cycle starts at the + /// latest grid slot at or before now instead (the same arithmetic as Darling's ServedSlot): a slot missed + /// during a stall is skipped, not replayed (). Static, with the clock + /// and the delay passed in, so a test drives this path with a fake clock and a fake delay. + /// + internal static async Task WaitForNextCycleAsync( + DateTime cycleStart, + TimeSpan interval, + Func utcNow, + Func delay, + CancellationToken cancellationToken) + { + var nextCycle = CollectorCadence.NextDue(cycleStart, utcNow(), interval); + var wait = nextCycle - utcNow(); + if (wait > TimeSpan.Zero) + { + await delay(wait, cancellationToken); + } + + var now = utcNow(); + return now >= nextCycle + interval + ? CollectorCadence.NextDue(nextCycle, now, interval) - interval + : nextCycle; + } + /// #2058: fills the Query Store history the live path never takes — the 60-minute /// first-contact tail and clamp-bounded outage holes — newest-first, strictly behind the live /// path's floor, never past the resolved query_store retention. See