fix(auditlog)+fix(siteeventlog): complete audit write tasks only after commit; run event-log retention on standby (arch-review 2026-08-19 P4-2 + P4-4)

P4-2: SqliteAuditWriter.FlushBatch completed each pending row's Task
(TrySetResult) inside the per-row insert loop, before transaction.Commit()
ran. A later failure in the same batch (another row's insert throwing, or
the commit itself failing) rolled the whole transaction back, but the
earlier rows' Tasks had already completed successfully -- the caller and
FallbackAuditWriter (whose ring-buffer fallback exists precisely for this
outage) both saw false success while the rows were actually gone from the
database; the outer catch's TrySetException became a silent no-op against
an already-completed TaskCompletionSource. Every row's Completion is now
deferred and only resolved (TrySetResult for the whole batch, or
TrySetException for the whole batch) after transaction.Commit() succeeds
or the transaction rolls back, mirroring SiteEventLogger.FlushBatch.

P4-4: EventLogPurgeService gated the daily site_events purge on the
active-node check, early-exiting on the standby, on the stale premise that
"the standby receives no writes". DataConnectionActor runs on both site
nodes and writes site_events regardless of active/standby status (register
rows 89/90/97), so an unreplicated standby (the rig's site-b/site-c
posture) grew its site_events file forever with no retention and no cap.
The gate is removed; every node now purges its own file every tick. Safe
on a replicated pair because the retention/cap deletes are CDC-captured
and converge under LWW regardless of which node issues them; required on
an unreplicated pair because each node's file is independent. The
active-node check is retained only to annotate the purge tick's log line.

Tests: new SqliteAuditWriterCommitFailureTests forces a genuine
non-swallowed mid-batch INSERT failure via a real SQLite trigger + custom
scalar function on the writer's own connection (reflection seam), proving
(a) a row whose own insert succeeded still faults when a later row in the
same batch fails, and (b) FallbackAuditWriter rings every faulted row and
increments the failure counter for each -- both assertions fail against
the pre-fix code (proof-of-failure verified by temporarily reverting the
fix). EventLogPurgeServiceTests' standby case is inverted to assert the
purge now runs (not skips) on a standby node.
This commit is contained in:
Joseph Doherty
2026-08-19 14:44:37 -04:00
parent 26ab6f8f09
commit 6c11362d9f
8 changed files with 290 additions and 67 deletions
@@ -484,7 +484,16 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
{
eventCmd.ExecuteNonQuery();
fwdCmd.ExecuteNonQuery();
pending.Completion.TrySetResult();
// Do NOT complete pending.Completion here (arch-review 2026-08-19
// P4-2). A row's INSERTs succeeding is not durable until the
// transaction actually commits below — completing the caller's Task
// this early means a later commit failure (SQLITE_FULL, IOERR, or a
// later row in this same batch throwing) reports false success: the
// row is rolled back out of the database but the caller — and
// FallbackAuditWriter, whose ring-buffer fallback exists precisely
// for this outage — already saw it as delivered. Every row in this
// batch is completed with TrySetResult() together, AFTER
// transaction.Commit() succeeds, mirroring SiteEventLogger.FlushBatch.
}
catch (SqliteException ex) when (ex.SqliteErrorCode == SqliteErrorConstraint)
{
@@ -492,21 +501,35 @@ public class SqliteAuditWriter : IAuditWriter, ISiteAuditQueue, IAsyncDisposable
// audit_event PRIMARY KEY throws before the sidecar insert
// runs, so neither table gains a second row. Treat as
// success: the lifecycle event is durably recorded under
// the first writer's payload.
// the first writer's payload. Still deferred to after Commit()
// below — the surrounding transaction (this row's canonical/sidecar
// insert never ran, but earlier rows in the batch did) must still
// commit for that to be true.
_logger.LogDebug(ex,
"Duplicate EventId {EventId} swallowed by SqliteAuditWriter",
evt.EventId);
pending.Completion.TrySetResult();
}
}
transaction.Commit();
_flushCountForTests++;
// Only NOW — after the whole batch is durably committed — tell every
// caller (and FallbackAuditWriter) that their row landed.
foreach (var pending in batch)
{
pending.Completion.TrySetResult();
}
}
catch (Exception ex)
{
transaction.Rollback();
_logger.LogError(ex, "SqliteAuditWriter batch insert failed; faulting {Count} pending events", batch.Count);
// Fault EVERY row in the batch, including any whose own INSERTs succeeded
// above — none of that is true anymore, since the whole transaction (this
// row's insert included) just rolled back. No TrySetResult has run yet for
// any row in this batch (see above), so every TrySetException here is a
// real transition, not a no-op racing an already-completed Task.
foreach (var pending in batch)
{
pending.Completion.TrySetException(ex);
@@ -210,20 +210,23 @@ public static class SiteServiceRegistration
return new AkkaClusterNodeProvider(akkaService, siteRole);
});
// The EventLogPurgeService runs on every
// site host node but consults this optional gate each tick and early-exits on
// the standby. Register it to delegate to IClusterNodeProvider.SelfIsPrimary
// (the canonical "this node is the oldest Up member (singleton host)" check) so purge
// runs ONLY on the active node — no duplicated cluster logic. Non-clustered test hosts that
// never call SiteServiceRegistration leave it unregistered, so the purge defaults
// to always-run (the pre-fix behaviour, preserved).
// The EventLogPurgeService runs on every site host node and, as of arch-review
// 2026-08-19 P4-4, purges its own site_events file on EVERY node — the old
// early-exit-on-standby gate this delegate drove is gone (DataConnectionActor
// writes site_events on both site nodes, so a standby that never purged would grow
// unbounded on an unreplicated site). Register it to delegate to
// IClusterNodeProvider.SelfIsPrimary (the canonical "this node is the oldest Up
// member (singleton host)" check) purely so the purge tick's log line can say
// whether this node is active — no duplicated cluster logic, no gating. Non-clustered
// test hosts that never call SiteServiceRegistration leave it unregistered, so the
// purge just logs every tick as active.
services.AddSingleton<SiteEventLogActiveNodeCheck>(sp =>
{
var nodeProvider = sp.GetRequiredService<IClusterNodeProvider>();
return () => nodeProvider.SelfIsPrimary;
});
// Wording-only companion to the gate above: when a purge on a replication-enabled node
// Wording-only companion to the delegate above: when a purge on a replication-enabled node
// actually deletes rows, it appends an operator note explaining the LocalDbOplogBacklog
// spike that follows. site_events is a replicated table and a retention/cap DELETE is
// CDC-captured like any other write (LocalDb Phase 2 — no purge-exemption path), so the
@@ -8,17 +8,16 @@ using ZB.MOM.WW.ScadaBridge.Commons.Observability;
namespace ZB.MOM.WW.ScadaBridge.SiteEventLogging;
/// <summary>
/// Predicate the <see cref="EventLogPurgeService"/>
/// consults at the top of every purge tick to decide whether THIS node should
/// run the daily purge. The design states "a daily background job runs on the
/// active node and deletes all events older than 30 days"; the standby's local
/// SQLite receives no writes, so purging there is harmless but unnecessary —
/// and silently doing it anyway diverges from the design.
/// Predicate the <see cref="EventLogPurgeService"/> consults at the top of every purge
/// tick. As of arch-review 2026-08-19 P4-4 it no longer GATES the purge — DataConnectionActor
/// writes <c>site_events</c> on both site nodes (register rows 89/90/97), so a standby that
/// skipped its own purge would grow its file unboundedly on an unreplicated site. Every node
/// now purges its own file every tick; this predicate is consulted only to annotate the tick's
/// log line with whether this node is the active one.
///
/// Registration is the Host's responsibility (it knows the cluster topology);
/// when no implementation is registered the purge service defaults to "always
/// active" so non-clustered hosts and unit tests are unaffected — backward
/// compatible with the prior "run on every host" behaviour.
/// when no implementation is registered the purge service treats every tick as active for
/// logging purposes — backward compatible with hosts that never register this delegate.
/// </summary>
/// <returns><c>true</c> if this node is the active site member and should run the purge; <c>false</c> to skip.</returns>
public delegate bool SiteEventLogActiveNodeCheck();
@@ -116,11 +115,11 @@ public class EventLogPurgeService : BackgroundService
/// <param name="options">Site event log options (retention days, storage cap, purge interval).</param>
/// <param name="logger">Logger instance.</param>
/// <param name="isActiveNode">
/// Optional active-node check. When <c>null</c>, the
/// service runs the purge on every tick (preserves the pre-fix behaviour
/// for non-clustered hosts and existing tests). When supplied — e.g. by
/// the Host on a site node — each tick early-exits on the standby so the
/// daily purge runs only on the active node, matching the design.
/// Optional active-node check, consulted only to annotate the purge tick's log line
/// (arch-review 2026-08-19 P4-4 removed the early-exit this used to drive — every node
/// purges its own <c>site_events</c> file every tick, since DataConnectionActor writes
/// on both site nodes and a standby that skipped its own purge would grow unbounded on
/// an unreplicated site). When <c>null</c> every tick logs as active.
/// </param>
/// <param name="isReplicationConfigured">
/// Optional LocalDb-replication check. When <c>null</c> — non-clustered hosts,
@@ -193,13 +192,20 @@ public class EventLogPurgeService : BackgroundService
{
try
{
// Gate every tick on the active-node check.
// The standby's local SQLite receives no writes, so purging there
// is harmless but unnecessary; the design (Component-SiteEventLogging
// → Storage) explicitly states the purge runs on the active node.
// Defensive try/catch: a transient cluster-state read failure must
// not stop the purge loop — fall back to running the purge (the
// pre-fix behaviour was "always run", which is harmless on standby).
// arch-review 2026-08-19 P4-4: purge no longer gates on the active-node check.
// DataConnectionActor writes site_events on BOTH site nodes (register rows
// 89/90/97), so the old "standby receives no writes" premise this gate was
// built on is stale — a standby that never purges its own file grows
// site_events forever (no retention, no cap) on an UNREPLICATED site (the rig's
// site-b/site-c posture). On a REPLICATED pair this was masked because the
// retention/cap DELETEs are CDC-captured and converge under LWW regardless of
// which node issues them (same reasoning as the existing purge-on-replicated
// design — see the replication-backlog note below and CLAUDE.md's LocalDb
// bullet); it is REQUIRED on an unreplicated pair because each node's file is
// independent. Both nodes now run retention + cap trim against their own file
// every tick. isActive is retained only to annotate the log line below (and
// defensively defaults to true if the check itself throws) — it no longer
// skips any work.
bool isActive;
try
{
@@ -208,15 +214,11 @@ public class EventLogPurgeService : BackgroundService
catch (Exception checkEx)
{
_logger.LogDebug(checkEx,
"Active-node check threw during purge tick; running purge to be safe");
"Active-node check threw during purge tick; treating as active for logging purposes");
isActive = true;
}
if (!isActive)
{
_logger.LogDebug("Skipping event log purge tick — this node is not the active site member");
return false;
}
_logger.LogDebug("Running event log purge tick (active={IsActive})", isActive);
var budget = new PurgeBudget(_options.MaxPurgedRowsPerTick);
PurgeByRetention(budget);
@@ -23,13 +23,14 @@ public static class ServiceCollectionExtensions
services.AddSingleton<ISiteEventLogger>(sp => sp.GetRequiredService<SiteEventLogger>());
services.AddSingleton<IEventLogQueryService, EventLogQueryService>();
// The purge service still registers on every host
// node, but it consults an optional SiteEventLogActiveNodeCheck on each
// tick and early-exits on the standby. The Host registers the real
// active-node check on site nodes; tests and non-clustered hosts leave
// it unregistered, and the purge defaults to "always run" (the
// pre-fix behaviour). Building the service via a factory so the
// optional delegate flows from DI rather than the constructor default.
// The purge service registers on every host node and purges its own
// site_events file on every tick, on every node (arch-review 2026-08-19 P4-4 —
// the standby writes site_events too, so it can no longer skip its own purge).
// It still consults an optional SiteEventLogActiveNodeCheck, but only to annotate
// the tick's log line, not to gate it. The Host registers the real active-node
// check on site nodes; tests and non-clustered hosts leave it unregistered, and
// the log line just reports every tick as active. Building the service via a
// factory so the optional delegate flows from DI rather than the constructor default.
//
// SiteEventLogReplicationCheck flows the same way and is likewise Host-supplied:
// it only selects the wording of the purge log line (whether to add the