From c4caebe9b4b2f1299e4a02cd257bc94cc633b4b8 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Fri, 14 Aug 2026 23:12:06 -0400
Subject: [PATCH] fix(test): remove the unsynchronized audit-attempt assertion
 in the two dispatcher audit-safety tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both NotifyDispatcher_AuditWriter_Throws_DeliveryStillSucceeds and
NotificationDispatch_BrokenAuditWriter_StillTransitionsToDelivered read the
throwing writer's attempt counter with a bare Assert immediately after an
AwaitAssert on the Notifications row reaching Delivered. That assumes the audit
writes happen no later than the operational status write, which the dispatcher
deliberately does NOT guarantee: DeliverOneAsync persists the delivery state
first (NotificationOutboxActor.cs:657) and only then emits the Attempted
(:663) and terminal (:676) audit rows — audit is best-effort and must never
gate the user-facing action. Observing Delivered therefore establishes no
happens-before edge with the writer, and under a loaded full-solution parallel
run the continuation after the DB write can be scheduled after the poll that
saw Delivered, so the counter reads 0 and the test fails with "saw 0".

Reproduced deterministically by delaying only the post-update audit emission,
which yields both observed failure messages verbatim; with the fix in place the
same injected delay passes, and suppressing the emissions entirely still fails
both tests with the identical messages — the claims (delivery despite audit
failure, and attempts >= N) are unchanged in force, only the ordering
assumption is gone.

Test-only change; the update-then-audit ordering predates the remediation
(#23 M4) and is correct as written.
---
 .../AuditWriteFailureSafetyTests.cs           | 18 +++++++++++++++--
 .../NotifyDispatcherAuditTrailTests.cs        | 20 +++++++++++++++++--
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/AuditWriteFailureSafetyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/AuditWriteFailureSafetyTests.cs
index 34c94dd9..48214a9b 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/AuditWriteFailureSafetyTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/AuditWriteFailureSafetyTests.cs
@@ -330,8 +330,22 @@ public class AuditWriteFailureSafetyTests : TestKit, IClassFixture<MsSqlMigratio
             Assert.NotNull(row.DeliveredAt);
         }, TimeSpan.FromSeconds(15));
 
-        Assert.True(throwingWriter.Attempts >= 1,
-            $"Expected dispatcher to attempt audit write at least once; saw {throwingWriter.Attempts}.");
+        // AwaitAssert, not a bare Assert: the dispatcher persists the delivery
+        // state BEFORE emitting the audit rows (DeliverOneAsync updates the
+        // row, then emits Attempted, then the terminal), so seeing Delivered
+        // above orders nothing with respect to the audit write — on a loaded
+        // parallel run the post-write continuation can land after the poll that
+        // observed Delivered, producing a spurious "saw 0". The bounded wait
+        // keeps the assertion's force: the writer must actually be invoked
+        // within the timeout or the test fails exactly as before.
+        await AwaitAssertAsync(
+            () =>
+            {
+                Assert.True(throwingWriter.Attempts >= 1,
+                    $"Expected dispatcher to attempt audit write at least once; saw {throwingWriter.Attempts}.");
+                return Task.CompletedTask;
+            },
+            TimeSpan.FromSeconds(15));
     }
 
     // ---------------------------------------------------------------------
diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/NotifyDispatcherAuditTrailTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/NotifyDispatcherAuditTrailTests.cs
index 9135011d..7e498def 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/NotifyDispatcherAuditTrailTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/NotifyDispatcherAuditTrailTests.cs
@@ -324,8 +324,24 @@ public class NotifyDispatcherAuditTrailTests : TestKit, IClassFixture<MsSqlMigra
         // The writer was attempted (at least once for the Attempted row, plus
         // once for the Delivered terminal) — proves the dispatcher tried to
         // emit and absorbed the throws rather than aborting the action.
-        Assert.True(throwingWriter.AttemptCount >= 2,
-            $"Expected the dispatcher to attempt audit writes; saw {throwingWriter.AttemptCount}");
+        //
+        // AwaitAssert, not a bare Assert: the dispatcher deliberately persists
+        // the delivery state BEFORE emitting either audit row (DeliverOneAsync
+        // writes the row, then Attempted, then the terminal), so observing
+        // Delivered above establishes NO happens-before edge with the audit
+        // writes — under a loaded parallel run the continuation after the DB
+        // write can be scheduled after the poll that saw Delivered, yielding a
+        // spurious "saw 0". The bounded wait removes the ordering assumption
+        // without weakening the claim: the writer must genuinely be invoked at
+        // least twice inside the timeout or the test still fails.
+        await AwaitAssertAsync(
+            () =>
+            {
+                Assert.True(throwingWriter.AttemptCount >= 2,
+                    $"Expected the dispatcher to attempt audit writes; saw {throwingWriter.AttemptCount}");
+                return Task.CompletedTask;
+            },
+            TimeSpan.FromSeconds(15));
     }
 
     /// <summary>