fix(siteeventlog)+fix(host): purge startup yield + auto_vacuum reality (arch-review 2026-08-19 P4-5 + P4-6)
P4-5: EventLogPurgeService.ExecuteAsync now yields (Task.Yield()) before running the initial RunPurge(), making "the purge never runs synchronously on the caller's thread" a property of the method itself rather than an artifact of .NET 8+'s BackgroundService scheduling (verified empirically not to reproduce the literal "StartAsync blocks" symptom on this repo's net10.0 target — a 1.5s synchronous Thread.Sleep in ExecuteAsync still lets StartAsync return in ~2ms). Proven instead via a reflection-invoked ExecuteAsync test that bypasses BackgroundService's Task.Run wrapper, which is timing-independent. Also: a throwing isActive predicate now fails CLOSED (logged as inactive, at Warning) instead of silently claiming "active" — the predicate only affects the tick's log line since P4-4, but a throw must not masquerade as a successful check. P4-6: PRAGMA auto_vacuum = INCREMENTAL only takes effect on a table-less database, and LocalDbSchema.EnsureCreated (inside SqliteLocalDb's constructor) always creates LocalDb's own bookkeeping tables before SiteEventLogSchema.Apply ever gets a connection — so the pragma there had never once taken effect, in any real call path. Real fix per plan decision 2: SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile pre-creates a genuinely fresh LocalDb file with the pragma set, called from SiteServiceRegistration.Configure BEFORE AddZbLocalDb's lazy singleton ever constructs SqliteLocalDb — no LocalDb API change needed. Only helps NEW site databases; an existing file (every node on the docker rig today) is left untouched rather than VACUUM'd, per the plan's explicit rejection of an unconditional VACUUM against a live ~1 GB file. The now-dead pragma call in SiteEventLogSchema.Apply is removed and its comment corrected. Tests proven to fail against the unfixed code (git-diff-and-restore, not stash — this repo shares one stash ref across all worktrees): ExecuteAsync_YieldsBeforeRunningTheInitial- PurgeSynchronously and RunPurge_ActiveNodeCheckThrows_TickLogsAsInactive_NotActive fail without the EventLogPurgeService fix; SiteLocalDbAutoVacuumTests fails to compile without EnsureAutoVacuumOnFreshFile. SiteEventLogging.Tests 112/112 and Host.Tests 526/526 green with the fix applied.
This commit is contained in:
Joseph Doherty
@@ -1,3 +1,4 @@
|
||||
using Microsoft.Data.Sqlite;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using ZB.MOM.WW.LocalDb;
|
||||
using ZB.MOM.WW.ScadaBridge.SiteEventLogging;
|
||||
@@ -234,4 +235,69 @@ public static class SiteLocalDbSetup
|
||||
return !string.IsNullOrWhiteSpace(section["PeerAddress"])
|
||||
|| !string.IsNullOrWhiteSpace(section["ApiKey"]);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Sets <c>auto_vacuum = INCREMENTAL</c> on the consolidated LocalDb file — arch-review
|
||||
/// 2026-08-19 P4-6 — but ONLY when that file does not exist yet.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>The defect this closes.</b> SQLite only honors a change to <c>auto_vacuum</c> while
|
||||
/// a database is still completely table-less; once one table exists the pragma is a
|
||||
/// silent no-op. <c>SqliteLocalDb</c>'s constructor runs <c>LocalDbSchema.EnsureCreated</c>
|
||||
/// on its own master connection before <see cref="OnReady"/> — or anything else — ever
|
||||
/// gets a connection to the file, so the pragma <see cref="SiteEventLogSchema.Apply"/>
|
||||
/// used to issue had never once taken effect, in any real call path in this repo: by the
|
||||
/// time it ran, LocalDb's own bookkeeping tables already existed.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The fix, and why it needs no LocalDb API change.</b> <c>AddZbLocalDb</c> registers
|
||||
/// <see cref="ILocalDb"/> as a LAZY singleton — nothing touches the file until it is
|
||||
/// first resolved. This method is called from <c>SiteServiceRegistration.Configure</c>,
|
||||
/// synchronously, BEFORE <c>services.AddZbLocalDb(...)</c> is even reached in that method
|
||||
/// — i.e. well before the host is built and anything could resolve <see cref="ILocalDb"/>
|
||||
/// — so it is free to open its own short-lived connection to the not-yet-existing file and
|
||||
/// set the pragma there. Empirically verified (not merely inferred from the pragma's
|
||||
/// documented "unless a subsequent operation modifies the database" caveat): opening a
|
||||
/// connection to a fresh path and executing this single PRAGMA is sufficient by itself —
|
||||
/// Microsoft.Data.Sqlite persists it to the file header on close, with no throwaway
|
||||
/// CREATE/DROP TABLE and no VACUUM required. A LATER session — <c>SqliteLocalDb</c>'s own
|
||||
/// constructor, opening the SAME path — sees the flag already baked in and every table it
|
||||
/// (and everyone downstream) subsequently creates lives under incremental-vacuum mode.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Only helps FRESH files — say so honestly, always.</b> A file that already exists
|
||||
/// already has tables, so this method deliberately does nothing to it: retrofitting
|
||||
/// auto_vacuum onto a populated database needs SQLite's own <c>VACUUM</c> rebuild, which
|
||||
/// plan decision 2 (2026-08-19) explicitly rejects running unconditionally against a live
|
||||
/// ~1 GB file on every boot. Consequence: every node on the docker rig today, and any
|
||||
/// site database created before this fix shipped, stays permanently
|
||||
/// <c>auto_vacuum=NONE</c> — <c>PRAGMA incremental_vacuum</c>, the per-trim call in
|
||||
/// <see cref="ZB.MOM.WW.ScadaBridge.SiteEventLogging.EventLogPurgeService.PurgeByStorageCap"/>,
|
||||
/// remains a harmless no-op there until an operator opts into a one-time offline VACUUM.
|
||||
/// Only a NEWLY created site database benefits, from the moment this ships onward.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="config">Configuration, read for <c>LocalDb:Path</c> only.</param>
|
||||
internal static void EnsureAutoVacuumOnFreshFile(IConfiguration config)
|
||||
{
|
||||
var path = config["LocalDb:Path"];
|
||||
if (string.IsNullOrWhiteSpace(path))
|
||||
return; // AddZbLocalDb's own options validation reports a missing path loudly; nothing to pre-create here.
|
||||
|
||||
if (File.Exists(path))
|
||||
return; // Already has tables (or is at least a pre-existing file) — the pragma cannot help it now.
|
||||
|
||||
var directory = Path.GetDirectoryName(path);
|
||||
if (!string.IsNullOrEmpty(directory))
|
||||
Directory.CreateDirectory(directory);
|
||||
|
||||
var connectionString = new SqliteConnectionStringBuilder { DataSource = path }.ToString();
|
||||
using var connection = new SqliteConnection(connectionString);
|
||||
connection.Open();
|
||||
|
||||
using var pragmaCmd = connection.CreateCommand();
|
||||
pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL";
|
||||
pragmaCmd.ExecuteNonQuery();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -91,6 +91,16 @@ public static class SiteServiceRegistration
|
||||
// failure, and the default site path is the relative "./data/site-localdb.db".
|
||||
// The shim is gone; SiteLocalDbDirectoryTests still pins the outcome here, since
|
||||
// what this host needs is the guarantee, not any particular owner of it.
|
||||
//
|
||||
// arch-review 2026-08-19 P4-6: MUST run before AddZbLocalDb — its singleton
|
||||
// factory is lazy, but the ordering still has to be enforced here rather than
|
||||
// relied upon, because it is what makes "PRAGMA auto_vacuum = INCREMENTAL" take
|
||||
// effect at all. SQLite only honors that pragma on a table-less database, and
|
||||
// AddZbLocalDb's own SqliteLocalDb constructor creates LocalDb's bookkeeping
|
||||
// tables the moment it is first resolved — see
|
||||
// SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile for the full mechanism, and note
|
||||
// it only helps a file that does not exist yet.
|
||||
SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config);
|
||||
services.AddZbLocalDb(config, db => SiteLocalDbSetup.OnReady(db, config));
|
||||
|
||||
// The replication engine, likewise unconditional but INERT by default: with no
|
||||
|
||||
@@ -164,6 +164,19 @@ public class EventLogPurgeService : BackgroundService
|
||||
_options.RetentionDays, _options.MaxStorageMb, _options.PurgeInterval,
|
||||
_options.MaxPurgedRowsPerTick > 0 ? _options.MaxPurgedRowsPerTick.ToString() : "unlimited");
|
||||
|
||||
// arch-review 2026-08-19 P4-5: yield BEFORE the initial (potentially ~17s
|
||||
// ceiling-limited, ~285s unpaced worst case) RunPurge() rather than calling it
|
||||
// as the first synchronous statement. .NET 8+'s BackgroundService already invokes
|
||||
// ExecuteAsync via Task.Run, so a synchronous prelude here does not actually block
|
||||
// IHostedService.StartAsync on this repo's net10.0 target (verified empirically —
|
||||
// it did not reproduce). This Task.Yield() is still the correct fix rather than a
|
||||
// no-op: it makes the "purge never blocks the calling thread" contract a property of
|
||||
// THIS method, not an artifact of the host's scheduling internals — reflection-invoking
|
||||
// ExecuteAsync directly (as EventLogPurgeServiceTests does, bypassing BackgroundService
|
||||
// entirely) would otherwise still run the initial purge fully synchronously, and any
|
||||
// future caller that awaits ExecuteAsync directly (not through StartAsync) would block.
|
||||
await Task.Yield();
|
||||
|
||||
// Run an initial purge on startup
|
||||
var ceilingHit = RunPurge();
|
||||
|
||||
@@ -213,9 +226,18 @@ public class EventLogPurgeService : BackgroundService
|
||||
}
|
||||
catch (Exception checkEx)
|
||||
{
|
||||
_logger.LogDebug(checkEx,
|
||||
"Active-node check threw during purge tick; treating as active for logging purposes");
|
||||
isActive = true;
|
||||
// arch-review 2026-08-19 P4-5: fail CLOSED, not "active". This predicate no
|
||||
// longer gates any work (P4-4 — both nodes purge their own file every tick
|
||||
// regardless), so a wrong value here cannot skip a purge; but silently
|
||||
// reporting "active" for a check that just threw is dishonest, and an
|
||||
// operator correlating this line against the real active-node state would be
|
||||
// misled into thinking the check succeeded. Warning (not Debug) because the
|
||||
// check itself is broken and is worth noticing on its own.
|
||||
_logger.LogWarning(checkEx,
|
||||
"Active-node check threw during purge tick; logging this tick as inactive " +
|
||||
"rather than claiming an active status the check could not confirm. The purge " +
|
||||
"itself is unaffected — every node purges its own file regardless of this flag");
|
||||
isActive = false;
|
||||
}
|
||||
|
||||
_logger.LogDebug("Running event log purge tick (active={IsActive})", isActive);
|
||||
@@ -513,9 +535,16 @@ public class EventLogPurgeService : BackgroundService
|
||||
""";
|
||||
var rows = cmd.ExecuteNonQuery();
|
||||
|
||||
// Reclaim free pages so page_count/freelist measurement reflects the
|
||||
// delete. Effective because auto_vacuum = INCREMENTAL is set at schema
|
||||
// creation; harmless otherwise.
|
||||
// Reclaim free pages so page_count/freelist measurement reflects the delete.
|
||||
// arch-review 2026-08-19 P4-6: this is effective ONLY on a consolidated
|
||||
// LocalDb file that was FRESH when SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile
|
||||
// ran (Host/SiteLocalDbSetup.cs) — auto_vacuum can only be enabled before any
|
||||
// table exists, and SqliteLocalDb's constructor creates LocalDb's own
|
||||
// bookkeeping tables before this component ever gets a connection. Every file
|
||||
// that predates that fix — including every node on the docker rig today —
|
||||
// stays permanently auto_vacuum=NONE, and this PRAGMA is a harmless, silent
|
||||
// no-op on it (retrofitting one requires an offline VACUUM, deliberately not
|
||||
// attempted here against a live ~1 GB file — plan decision 2, 2026-08-19).
|
||||
using var vacuumCmd = connection.CreateCommand();
|
||||
vacuumCmd.CommandText = "PRAGMA incremental_vacuum";
|
||||
vacuumCmd.ExecuteNonQuery();
|
||||
|
||||
@@ -36,15 +36,24 @@ public static class SiteEventLogSchema
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(connection);
|
||||
|
||||
// auto_vacuum must be set before any table is created for it to take effect
|
||||
// on a fresh database. With INCREMENTAL mode, PRAGMA incremental_vacuum can
|
||||
// later reclaim free pages so the storage-cap purge can shrink the file.
|
||||
using (var pragmaCmd = connection.CreateCommand())
|
||||
{
|
||||
pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL";
|
||||
pragmaCmd.ExecuteNonQuery();
|
||||
}
|
||||
|
||||
// arch-review 2026-08-19 P4-6: this method used to also set
|
||||
// "PRAGMA auto_vacuum = INCREMENTAL" here, on the theory that setting it right
|
||||
// before CREATE TABLE would make it take effect on a fresh database. That was
|
||||
// wrong in every real call path: SQLite only honors an auto_vacuum change on a
|
||||
// database that has NO tables yet, and by the time this method runs — via the
|
||||
// Host's AddZbLocalDb onReady callback (SiteLocalDbSetup.OnReady) OR via
|
||||
// SiteEventLogger's own InitializeSchema fallback for directly-constructed
|
||||
// instances — the consolidated file already holds LocalDb's own bookkeeping
|
||||
// tables (SqliteLocalDb's constructor runs LocalDbSchema.EnsureCreated on its
|
||||
// master connection before either caller ever gets a connection). So the pragma
|
||||
// here had never once taken effect; it was a silent, permanent no-op. The real
|
||||
// fix now lives in Host/SiteLocalDbSetup.cs
|
||||
// (EnsureAutoVacuumOnFreshFile), which pre-creates a genuinely fresh LocalDb file
|
||||
// with the pragma set BEFORE AddZbLocalDb's lazy singleton ever constructs
|
||||
// SqliteLocalDb — and, being a Host-layer, one-time, pre-file-existence step, it
|
||||
// helps ONLY brand-new site databases; every file created before this fix shipped
|
||||
// stays auto_vacuum=NONE forever (see the PurgeByStorageCap remarks in
|
||||
// EventLogPurgeService for the operational consequence).
|
||||
using var cmd = connection.CreateCommand();
|
||||
cmd.CommandText = """
|
||||
CREATE TABLE IF NOT EXISTS site_events (
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
using Microsoft.Data.Sqlite;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ZB.MOM.WW.LocalDb;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Arch-review 2026-08-19 P4-6 — <c>PRAGMA auto_vacuum = INCREMENTAL</c> only takes effect on
|
||||
/// a table-less database, and <c>SqliteLocalDb</c>'s constructor creates LocalDb's own
|
||||
/// bookkeeping tables before anything else ever gets a connection to the consolidated site
|
||||
/// file. <see cref="SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile"/> is the fix: it pre-creates
|
||||
/// a genuinely fresh file with the pragma set BEFORE <c>AddZbLocalDb</c>'s lazy singleton ever
|
||||
/// constructs <c>SqliteLocalDb</c>.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// These tests exercise <see cref="SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile"/> directly
|
||||
/// (it is <c>internal</c>, visible here via <c>InternalsVisibleTo</c>) rather than through the
|
||||
/// full <see cref="SiteServiceRegistration"/> composition root — the mechanism under test is a
|
||||
/// pure ordering/pragma concern that does not depend on the rest of the site graph, and
|
||||
/// <see cref="SiteLocalDbWiringTests"/> already proves the composition root wires
|
||||
/// <c>SiteLocalDbSetup</c> in at all.
|
||||
/// </remarks>
|
||||
public class SiteLocalDbAutoVacuumTests
|
||||
{
|
||||
[Fact]
|
||||
public void FreshFile_ReportsIncrementalAutoVacuum()
|
||||
{
|
||||
// The plan's own acceptance test: a fresh file reports PRAGMA auto_vacuum = 2
|
||||
// (INCREMENTAL) after the pre-create step runs.
|
||||
var path = TempDbPath();
|
||||
try
|
||||
{
|
||||
var config = ConfigFor(path);
|
||||
|
||||
SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config);
|
||||
|
||||
Assert.Equal(2L, ReadAutoVacuum(path));
|
||||
}
|
||||
finally
|
||||
{
|
||||
Cleanup(path);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void FreshFile_AutoVacuumSurvives_LocalDbCreatingItsOwnBookkeepingTablesAfterward()
|
||||
{
|
||||
// The real end-to-end claim: the pre-create step has to run BEFORE AddZbLocalDb ever
|
||||
// constructs SqliteLocalDb (which creates LocalDb's own tables first, unconditionally,
|
||||
// in its constructor) for the ordering fix to mean anything. This proves the flag
|
||||
// baked in by EnsureAutoVacuumOnFreshFile survives that later table creation — the
|
||||
// exact sequence SiteServiceRegistration.Configure now runs in production.
|
||||
var path = TempDbPath();
|
||||
try
|
||||
{
|
||||
var config = ConfigFor(path);
|
||||
|
||||
SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config);
|
||||
|
||||
var services = new ServiceCollection();
|
||||
services.AddZbLocalDb(config, db => SiteLocalDbSetup.OnReady(db, config));
|
||||
using var provider = services.BuildServiceProvider();
|
||||
using var scope = provider.CreateScope();
|
||||
|
||||
// Resolving ILocalDb is what triggers SqliteLocalDb's constructor (LocalDbSchema
|
||||
// .EnsureCreated) and then OnReady (SiteEventLogSchema.Apply, etc.) — i.e. every
|
||||
// real table this file will ever hold. This config carries no
|
||||
// LocalDb:Replication section, so OnReady takes the deregistration branch and
|
||||
// db.ReplicatedTables stays empty — check the raw file instead to confirm the
|
||||
// table genuinely exists.
|
||||
var db = scope.ServiceProvider.GetRequiredService<ILocalDb>();
|
||||
Assert.NotNull(db);
|
||||
Assert.Contains("site_events", TableNames(path));
|
||||
|
||||
Assert.Equal(2L, ReadAutoVacuum(path));
|
||||
}
|
||||
finally
|
||||
{
|
||||
SqliteConnection.ClearAllPools();
|
||||
Cleanup(path);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ExistingFile_IsLeftAlone_AutoVacuumStaysNone()
|
||||
{
|
||||
// Honesty requirement (plan decision 2): the fix only helps a file that does not exist
|
||||
// yet. A file that already has tables — every node on the docker rig today, and any
|
||||
// site database created before this fix shipped — is untouched, and stays
|
||||
// auto_vacuum = NONE forever short of an offline VACUUM this task deliberately does
|
||||
// not attempt.
|
||||
var path = TempDbPath();
|
||||
try
|
||||
{
|
||||
// Pre-populate the file exactly like a real (pre-fix) LocalDb file: at least one
|
||||
// table exists before EnsureAutoVacuumOnFreshFile ever runs.
|
||||
using (var seed = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString()))
|
||||
{
|
||||
seed.Open();
|
||||
using var cmd = seed.CreateCommand();
|
||||
cmd.CommandText = "CREATE TABLE legacy_marker (id INTEGER PRIMARY KEY)";
|
||||
cmd.ExecuteNonQuery();
|
||||
}
|
||||
|
||||
Assert.Equal(0L, ReadAutoVacuum(path)); // NONE, the SQLite default.
|
||||
|
||||
var config = ConfigFor(path);
|
||||
SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config);
|
||||
|
||||
// Still NONE — the method must not attempt VACUUM against a populated file.
|
||||
Assert.Equal(0L, ReadAutoVacuum(path));
|
||||
}
|
||||
finally
|
||||
{
|
||||
SqliteConnection.ClearAllPools();
|
||||
Cleanup(path);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MissingLocalDbPath_DoesNotThrow()
|
||||
{
|
||||
// AddZbLocalDb's own options validation is the authoritative place a missing path is
|
||||
// reported; this pre-create step must defer to it rather than throwing its own,
|
||||
// differently-worded error first.
|
||||
var config = new ConfigurationBuilder().AddInMemoryCollection(
|
||||
new Dictionary<string, string?>()).Build();
|
||||
|
||||
var ex = Record.Exception(() => SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile(config));
|
||||
|
||||
Assert.Null(ex);
|
||||
}
|
||||
|
||||
private static string TempDbPath() =>
|
||||
Path.Combine(Path.GetTempPath(), $"scadabridge_autovacuum_{Guid.NewGuid():N}.db");
|
||||
|
||||
private static IConfiguration ConfigFor(string path) =>
|
||||
new ConfigurationBuilder()
|
||||
.AddInMemoryCollection(new Dictionary<string, string?> { ["LocalDb:Path"] = path })
|
||||
.Build();
|
||||
|
||||
private static long ReadAutoVacuum(string path)
|
||||
{
|
||||
using var connection = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString());
|
||||
connection.Open();
|
||||
using var cmd = connection.CreateCommand();
|
||||
cmd.CommandText = "PRAGMA auto_vacuum";
|
||||
return Convert.ToInt64(cmd.ExecuteScalar());
|
||||
}
|
||||
|
||||
private static HashSet<string> TableNames(string path)
|
||||
{
|
||||
using var connection = new SqliteConnection(new SqliteConnectionStringBuilder { DataSource = path }.ToString());
|
||||
connection.Open();
|
||||
using var cmd = connection.CreateCommand();
|
||||
cmd.CommandText = "SELECT name FROM sqlite_master WHERE type = 'table'";
|
||||
using var reader = cmd.ExecuteReader();
|
||||
var names = new HashSet<string>(StringComparer.Ordinal);
|
||||
while (reader.Read()) names.Add(reader.GetString(0));
|
||||
return names;
|
||||
}
|
||||
|
||||
private static void Cleanup(string path)
|
||||
{
|
||||
foreach (var suffix in new[] { "", "-wal", "-shm" })
|
||||
{
|
||||
try { File.Delete(path + suffix); } catch { /* best effort */ }
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,3 +1,4 @@
|
||||
using System.Reflection;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Microsoft.Extensions.Options;
|
||||
@@ -355,6 +356,92 @@ public class EventLogPurgeServiceTests : IDisposable
|
||||
await purge.StopAsync(CancellationToken.None);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ExecuteAsync_YieldsBeforeRunningTheInitialPurgeSynchronously()
|
||||
{
|
||||
// arch-review 2026-08-19 P4-5 — timing-independent companion to
|
||||
// StartAsync_DoesNotBlock_OnTheInitialPurge above.
|
||||
//
|
||||
// That test times BackgroundService.StartAsync as a whole and passes even on the
|
||||
// UNFIXED code, because .NET 8+ already invokes ExecuteAsync via Task.Run — the
|
||||
// synchronous prelude runs on a pool thread, not the caller's, so StartAsync itself
|
||||
// never blocks regardless of whether ExecuteAsync yields first. Verified empirically
|
||||
// against this repo's actual net10.0 target (a BackgroundService whose ExecuteAsync
|
||||
// does Thread.Sleep(1500) before its first await still returns from StartAsync in
|
||||
// ~2 ms). So P4-5's literal "StartAsync blocks" symptom does not reproduce here, and
|
||||
// a wall-clock test cannot prove the fix.
|
||||
//
|
||||
// What CAN be proven without any timing is the ExecuteAsync-level contract itself:
|
||||
// invoking the protected ExecuteAsync directly via reflection — bypassing
|
||||
// BackgroundService's Task.Run wrapper entirely — still runs it synchronously, on
|
||||
// the calling thread, up through its first genuinely-suspending await (that is what
|
||||
// "async Task method" means; MethodInfo.Invoke does not change it). On the unfixed
|
||||
// code ExecuteAsync's first statement is RunPurge() with no await before it, so
|
||||
// Invoke would not return until the initial purge had fully run — deleting the
|
||||
// expired row before this method's Assert ever executes. With Task.Yield() as the
|
||||
// very first statement, Invoke returns as soon as MoveNext() hits the (always
|
||||
// IsCompleted == false) yield point, before RunPurge has been called at all.
|
||||
InsertEventWithTimestamp(DateTimeOffset.UtcNow.AddDays(-31));
|
||||
Assert.Equal(1, GetEventCount());
|
||||
|
||||
var purge = CreatePurgeService();
|
||||
var executeAsync = typeof(EventLogPurgeService).GetMethod(
|
||||
"ExecuteAsync", BindingFlags.NonPublic | BindingFlags.Instance);
|
||||
Assert.NotNull(executeAsync);
|
||||
|
||||
using var cts = new CancellationTokenSource();
|
||||
var task = (Task)executeAsync!.Invoke(purge, [cts.Token])!;
|
||||
|
||||
// Reflection Invoke has already returned control here — a synchronous
|
||||
// RunPurge() ahead of any await would have already deleted the row by now.
|
||||
Assert.Equal(1, GetEventCount());
|
||||
|
||||
// The initial purge still runs, just after control has been yielded back.
|
||||
var deadline = DateTime.UtcNow.AddSeconds(5);
|
||||
while (GetEventCount() != 0 && DateTime.UtcNow < deadline)
|
||||
{
|
||||
await Task.Delay(25);
|
||||
}
|
||||
Assert.Equal(0, GetEventCount());
|
||||
|
||||
await cts.CancelAsync();
|
||||
try
|
||||
{
|
||||
await task;
|
||||
}
|
||||
catch (OperationCanceledException)
|
||||
{
|
||||
// Expected: the loop's Task.Delay observes the cancellation.
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void RunPurge_ActiveNodeCheckThrows_TickLogsAsInactive_NotActive()
|
||||
{
|
||||
// arch-review 2026-08-19 P4-5: a throwing isActive predicate must fail CLOSED
|
||||
// (logged as inactive) rather than silently claiming "active". The predicate no
|
||||
// longer gates any purge work (P4-4 — every node purges its own file every tick
|
||||
// regardless), so a wrong value here cannot skip work; but an operator correlating
|
||||
// this line against the real active-node state must not be misled into thinking the
|
||||
// check actually succeeded and reported active.
|
||||
var logger = new CapturingLogger();
|
||||
var purge = CreatePurgeService(
|
||||
isActiveNode: () => throw new InvalidOperationException("boom"),
|
||||
logger: logger);
|
||||
|
||||
purge.RunPurge();
|
||||
|
||||
var warnLine = Assert.Single(
|
||||
logger.Entries,
|
||||
e => e.Message.Contains("Active-node check threw", StringComparison.Ordinal));
|
||||
Assert.Equal(LogLevel.Warning, warnLine.Level);
|
||||
|
||||
var tickLine = Assert.Single(
|
||||
logger.Entries,
|
||||
e => e.Message.Contains("Running event log purge tick", StringComparison.Ordinal));
|
||||
Assert.Contains("active=False", tickLine.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user