From 439dee52d352c204cf6ee1f610faf13192027207 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:21:02 -0400 Subject: [PATCH] fix(kpi): classify the failover fold-race pass loss as a self-healing Information event (plan R2-04 T9) --- docs/requirements/Component-KpiHistory.md | 2 +- .../Repositories/KpiHistoryRepository.cs | 29 ++++- .../Repositories/KpiHistoryRepositoryTests.cs | 100 ++++++++++++++++++ 3 files changed, 128 insertions(+), 3 deletions(-) diff --git a/docs/requirements/Component-KpiHistory.md b/docs/requirements/Component-KpiHistory.md index 604cb65a..11f3b15b 100644 --- a/docs/requirements/Component-KpiHistory.md +++ b/docs/requirements/Component-KpiHistory.md @@ -88,7 +88,7 @@ A timer fires every `SampleInterval` (default 60s; an immediate first tick prime **Best-effort, per-source isolation.** Each source call and the write are individually guarded. A throwing source is logged and its samples skipped for that tick; it never aborts the tick, the other sources, or the source component itself. This is the same `IEnumerable<>`-of-adapters decoupling pattern used by `INotificationDeliveryAdapter`. -**Hourly rollup tick.** A third timer (`kpi-rollup`) fires every `RollupInterval` (default 1h). On each tick the recorder opens a per-tick DI scope and calls `IKpiHistoryRepository.FoldHourlyRollupsAsync` over the window `[TruncateToHour(now) − RollupLookbackHours, TruncateToHour(now))` — an **exclusive** upper bound at the current hour start, so the in-progress hour is never folded. The fold groups raw `KpiSample` rows by `(series, hour)`, applies the per-metric aggregation intent (sum for rate metrics, last-value for gauges — see Query), and **idempotently upserts** each `(Source, Metric, Scope, ScopeKey, HourStartUtc)` row. Re-folding the trailing lookback window (default 3h) rather than only the last hour is the self-heal for a **singleton-failover-missed tick**: because the upsert overwrites the aggregate in place, re-running the same window produces identical values (no double-count), so any complete hour a missed tick skipped is refilled on the next fold. A `_rollupInFlight` guard (mirroring `_sampleInFlight`) skips a tick while a prior fold is still running so two idempotent upserts never race on overlapping rows. +**Hourly rollup tick.** A third timer (`kpi-rollup`) fires every `RollupInterval` (default 1h). On each tick the recorder opens a per-tick DI scope and calls `IKpiHistoryRepository.FoldHourlyRollupsAsync` over the window `[TruncateToHour(now) − RollupLookbackHours, TruncateToHour(now))` — an **exclusive** upper bound at the current hour start, so the in-progress hour is never folded. The fold groups raw `KpiSample` rows by `(series, hour)`, applies the per-metric aggregation intent (sum for rate metrics, last-value for gauges — see Query), and **idempotently upserts** each `(Source, Metric, Scope, ScopeKey, HourStartUtc)` row. Re-folding the trailing lookback window (default 3h) rather than only the last hour is the self-heal for a **singleton-failover-missed tick**: because the upsert overwrites the aggregate in place, re-running the same window produces identical values (no double-count), so any complete hour a missed tick skipped is refilled on the next fold. A `_rollupInFlight` guard (mirroring `_sampleInFlight`) skips a tick while a prior fold is still running so two idempotent upserts never race on overlapping rows. During a **failover overlap** — the old singleton incarnation's in-flight fold and the new node's first fold both `Add`-ing the same `(series, hour)` row — the loser of the unique `IX_KpiRollupHourly_Series` upsert race discards its **whole fold pass** (a single `SaveChanges`), logged at Information and repaired by the next idempotent fold; when reading fold logs after a failover, the failure grain is the **pass, not the row** (arch-review 04 round 2, R5). **One-shot backfill.** So the 30 d / 90 d charts are not blank until enough wall-clock passes, the recorder runs a backfill on start (`kpi-rollup-backfill` timer) that folds the retention window, reusing the same `FoldHourlyRollupsAsync` path. The `FoldHourlyRollupsAsync` repository call materializes its whole window in memory, so the backfill folds the window in **≤24 h slices, oldest-first** — one bounded slice per fold — rather than handing the fold the full 90-day window at once (which at fleet volume is tens of millions of rows in one tracked pass). Between slices it lowers its in-flight guard so periodic folds **interleave** instead of stalling behind the historical pass; a periodic fold and a backfill slice strictly alternate through the single `_rollupInFlight`/`_backfillInFlight` gate pair, so the two idempotent upserts never race on overlapping rows. On a **failover restart** the plan first consults the `IKpiHistoryRepository.GetLatestRollupHourAsync` watermark: if the newest folded hour is already within `RollupLookbackHours` of now (the common case) the whole historical pass is **skipped** — the periodic fold's lookback covers the tail — otherwise the window is **shrunk to the un-rolled tail** `[watermark, TruncateToHour(now))` (re-folding the watermark hour itself as an idempotent safety margin) instead of the full `RetentionDays` floor. A `_backfillDone` latch makes the whole pass at-most-once per actor lifetime, and it defers (re-arms) while a periodic fold is in flight. Being an idempotent upsert, it is safe (idempotent) to re-run on a later failover restart; the watermark fast-path makes the failover re-run cheap as well. diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs index 50bb4223..b1298cfb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs @@ -1,4 +1,6 @@ using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi; @@ -13,14 +15,22 @@ namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; public sealed class KpiHistoryRepository : IKpiHistoryRepository { private readonly ScadaBridgeDbContext _context; + private readonly ILogger _logger; /// /// Initializes a new instance of the class. /// /// The EF Core database context. - public KpiHistoryRepository(ScadaBridgeDbContext context) + /// + /// Optional logger; defaults to (mirrors + /// SiteCallAuditRepository — MS.DI resolves automatically, + /// so no registration churn). Used to classify the self-healing failover fold-race. + /// + public KpiHistoryRepository( + ScadaBridgeDbContext context, ILogger? logger = null) { _context = context ?? throw new ArgumentNullException(nameof(context)); + _logger = logger ?? NullLogger.Instance; } /// @@ -170,7 +180,22 @@ public sealed class KpiHistoryRepository : IKpiHistoryRepository } } - await _context.SaveChangesAsync(cancellationToken); + try + { + await _context.SaveChangesAsync(cancellationToken); + } + catch (DbUpdateException ex) + { + // Failover-overlap race: the old singleton incarnation's in-flight fold and the + // new node's first fold can both Add the same (series, hour) row; the unique + // IX_KpiRollupHourly_Series then faults the loser's ENTIRE SaveChanges — the + // failure grain is the PASS, not the row (arch-review 04 round 2, R5). The fold + // only ever writes KpiRollupHourly rows, so any save fault here is a lost upsert + // race or a transient the next idempotent re-fold repairs identically; classify + // at Information instead of surfacing a scary error for a self-healing no-op. + _logger.LogInformation(ex, + "KPI rollup fold lost a failover-overlap upsert race; pass discarded, next fold self-heals."); + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/KpiHistoryRepositoryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/KpiHistoryRepositoryTests.cs index 3a815797..195b36b5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/KpiHistoryRepositoryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/KpiHistoryRepositoryTests.cs @@ -429,10 +429,110 @@ public class KpiHistoryRepositoryTests Assert.Equal(Base.AddHours(3), await repo.GetLatestRollupHourAsync()); } + // ---- Task 9: failover fold-race failure grain is the pass, not the row (R5) ---- + + [Fact] + public async Task Fold_LosingFailoverUpsertRace_DoesNotThrow_AndNextFoldSelfHeals() + { + // A single shared in-memory SQLite connection so a second context (the interceptor's + // side-write, and the "next tick" fold) sees the same database. + await using var connection = new Microsoft.Data.Sqlite.SqliteConnection("DataSource=:memory:"); + await connection.OpenAsync(); + await using (var schema = ContextOn(connection)) + { + await schema.Database.EnsureCreatedAsync(); + } + + // The old singleton incarnation's in-flight fold wins the (series, hour) upsert race: + // the interceptor inserts the conflicting rollup row on the same connection just before + // THIS fold's SaveChanges, so the unique IX_KpiRollupHourly_Series faults the loser's save. + // A non-null ScopeKey (site-scoped series) is used deliberately: SQLite treats NULLs as + // distinct in a UNIQUE index, so only a non-null key deterministically forces the conflict. + var interceptor = new InsertConflictingRollupOnFirstSave(connection, () => new KpiRollupHourly + { + Source = "NotificationOutbox", Metric = "queueDepth", Scope = "Site", ScopeKey = "plant-a", + HourStartUtc = Base, Value = -1, MinValue = -1, MaxValue = -1, SampleCount = 1, + }); + + await using var ctx = ContextOn(connection, interceptor); + var repo = new KpiHistoryRepository(ctx); + await repo.RecordSamplesAsync(new[] + { + Sample("NotificationOutbox", "queueDepth", "Site", "plant-a", 5, Base.AddMinutes(10)), + }); + + // Losing the race must NOT propagate DbUpdateException — the failure grain is the pass. + var ex = await Record.ExceptionAsync(() => repo.FoldHourlyRollupsAsync(Base, Base.AddHours(1))); + Assert.Null(ex); + + // Next tick's fold (a fresh scope/context over the same DB) self-heals: it finds the + // conflicting row and idempotently upserts it in place to the correct folded value. + await using var ctx2 = ContextOn(connection); + var repo2 = new KpiHistoryRepository(ctx2); + await repo2.FoldHourlyRollupsAsync(Base, Base.AddHours(1)); + + var series = await repo2.GetHourlySeriesAsync( + "NotificationOutbox", "queueDepth", "Site", "plant-a", Base, Base.AddHours(1)); + Assert.Single(series); + Assert.Equal(5, series[0].Value); // the fold's own value overwrote the side-inserted -1 + } + // Local mirror of the repo's private hour-truncation, for cutoff assertions. private static DateTime TruncateHour(DateTime utc) => new(utc.Year, utc.Month, utc.Day, utc.Hour, 0, 0, DateTimeKind.Utc); + /// Builds a SQLite test context bound to an externally-owned open connection + /// (so multiple contexts share one in-memory database), with optional interceptors. + private static ScadaBridgeDbContext ContextOn(DbConnection connection, params IInterceptor[] interceptors) + { + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)) + .AddInterceptors(interceptors) + .Options; + return new SqliteTestDbContext(options); + } + + /// + /// SaveChanges interceptor that, exactly once and only when a fold is about to insert a + /// , inserts a conflicting (series, hour) rollup row through a + /// second context on the same connection — deterministically reproducing the failover-overlap + /// upsert race the losing fold must survive (arch-review 04 round 2, R5). + /// + private sealed class InsertConflictingRollupOnFirstSave : Microsoft.EntityFrameworkCore.Diagnostics.SaveChangesInterceptor + { + private readonly DbConnection _connection; + private readonly Func _rowFactory; + private bool _fired; + + public InsertConflictingRollupOnFirstSave(DbConnection connection, Func rowFactory) + { + _connection = connection; + _rowFactory = rowFactory; + } + + public override async ValueTask> SavingChangesAsync( + DbContextEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (!_fired && eventData.Context is not null && + eventData.Context.ChangeTracker.Entries() + .Any(e => e.State == EntityState.Added)) + { + _fired = true; + var options = new DbContextOptionsBuilder() + .UseSqlite(_connection) + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)) + .Options; + await using var side = new SqliteTestDbContext(options); + side.KpiRollupHourly.Add(_rowFactory()); + await side.SaveChangesAsync(cancellationToken); + } + + return await base.SavingChangesAsync(eventData, result, cancellationToken); + } + } + /// /// EF command interceptor that counts the DELETE statements actually issued /// to the store, so a test can prove the purge is sliced into multiple