From 6b06d1efcbf7b11448aa0037ce4bd12844e54e2c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 9 Jul 2026 07:56:39 -0400 Subject: [PATCH] =?UTF-8?q?fix(site-call-audit):=20same-rank=20freshness?= =?UTF-8?q?=20tiebreaker=20=E2=80=94=20retrying=20calls=20no=20longer=20fr?= =?UTF-8?q?eeze=20RetryCount/LastError=20at=20first=20write?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Within an equal NON-terminal rank the newest UpdatedAtUtc wins, unfreezing the Attempted-phase RetryCount/LastError/HttpStatus. Terminal ranks (>=3) are excluded from the tiebreaker so terminal immutability is preserved (Delivered never overwrites Parked); equal stamps stay idempotent, lower rank stays a no-op. --- docs/requirements/Component-SiteCallAudit.md | 12 ++- .../Repositories/SiteCallAuditRepository.cs | 43 +++++++-- .../SiteCallAuditRepositoryTests.cs | 89 +++++++++++++++++-- 3 files changed, 126 insertions(+), 18 deletions(-) diff --git a/docs/requirements/Component-SiteCallAudit.md b/docs/requirements/Component-SiteCallAudit.md index 5ed06a60..26f2f263 100644 --- a/docs/requirements/Component-SiteCallAudit.md +++ b/docs/requirements/Component-SiteCallAudit.md @@ -73,9 +73,15 @@ mirror — never queried by scripts (`Tracking.Status()` is answered site-locall ## Ingest & Idempotency Telemetry ingestion is **insert-if-not-exists** keyed on `TrackedOperationId`, -then **upsert-on-newer-status**. The lifecycle is monotonic, so status only -advances and never regresses; at-least-once and out-of-order telemetry are -therefore harmless. +then **upsert-on-newer-status, with a newest-`UpdatedAtUtc` tiebreaker within +equal non-terminal rank**. The lifecycle is monotonic on status rank, so status +never regresses. Within an equal *non-terminal* rank (the `Attempted`/`Skipped` +retry phase), the packet with the newest `UpdatedAtUtc` wins — so a retrying +call's `RetryCount`/`LastError`/`HttpStatus` stay live instead of freezing at the +first `Attempted` write. Equal *terminal* rank stays immutable (a later +`Delivered` never overwrites an earlier `Parked`), equal stamps are an idempotent +no-op, and a lower rank is always a no-op — so at-least-once and out-of-order +telemetry remain harmless. From v1.x onward, the `CachedCallTelemetry` message additively carries the `AuditEvent` content alongside the existing operational fields. Central's diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs index c54b0b44..0b0d608d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs @@ -24,9 +24,17 @@ public class SiteCallAuditRepository : ISiteCallAuditRepository private const int SqlErrorUniqueIndexViolation = 2601; private const int SqlErrorPrimaryKeyViolation = 2627; - // Monotonic status ordering. Lower rank wins on tie (same-rank upserts are - // no-ops, including terminal-over-terminal): + // Monotonic status ordering: // Submitted < Forwarded < Attempted == Skipped < Delivered == Failed == Parked == Discarded. + // A higher incoming rank always wins. WITHIN an equal NON-terminal rank + // (Attempted/Skipped, rank 2 — and the transient Submitted/Forwarded ranks), + // the newest UpdatedAtUtc wins so a retrying call's live RetryCount/LastError + // no longer freezes at first-write (Task 11). Equal terminal ranks (rank 3) + // stay immutable — the freshness tiebreaker is deliberately scoped to + // rank < 3, so a later terminal NEVER flips an earlier one (Delivered cannot + // overwrite Parked). Still idempotent (equal stamps are inert) and still + // regression-proof (a lower rank is always a no-op). + private const int TerminalRank = 3; private static readonly Dictionary StatusRank = new(StringComparer.Ordinal) { ["Submitted"] = 0, @@ -95,10 +103,16 @@ VALUES idText); } - // Step 2: monotonic update. The CASE expression maps the stored Status - // string to the same rank table the caller uses; we only mutate if the - // incoming rank is strictly greater. Same-rank (including - // terminal-over-terminal) is a no-op — first-write-wins at each rank. + // Step 2: monotonic update with a same-rank freshness tiebreaker. The + // CASE expression maps the stored Status string to the same rank table + // the caller uses. We mutate when EITHER the incoming rank is strictly + // greater, OR the incoming rank equals the stored rank AND that rank is + // non-terminal (< TerminalRank) AND the incoming UpdatedAtUtc is strictly + // newer than the stored one — so a retrying call's Attempted-phase + // RetryCount/LastError/HttpStatus stay live instead of freezing at the + // first Attempted packet (Task 11). Terminal ranks are excluded from the + // tiebreaker, so a later terminal NEVER overwrites an earlier one; equal + // stamps are inert (idempotent replay) and a lower rank is always a no-op. // // SourceNode-stamping: SourceNode is updated via // COALESCE(@SourceNode, SourceNode). The operator returns @SourceNode @@ -125,7 +139,7 @@ SET Status = {siteCall.Status}, IngestedAtUtc = {siteCall.IngestedAtUtc}, SourceNode = COALESCE({siteCall.SourceNode}, SourceNode) WHERE TrackedOperationId = {idText} - AND {incomingRank} > (CASE Status + AND ( {incomingRank} > (CASE Status WHEN 'Submitted' THEN 0 WHEN 'Forwarded' THEN 1 WHEN 'Attempted' THEN 2 @@ -135,7 +149,20 @@ WHERE TrackedOperationId = {idText} WHEN 'Parked' THEN 3 WHEN 'Discarded' THEN 3 ELSE -1 - END);", + END) + OR ( {incomingRank} = (CASE Status + WHEN 'Submitted' THEN 0 + WHEN 'Forwarded' THEN 1 + WHEN 'Attempted' THEN 2 + WHEN 'Skipped' THEN 2 + WHEN 'Delivered' THEN 3 + WHEN 'Failed' THEN 3 + WHEN 'Parked' THEN 3 + WHEN 'Discarded' THEN 3 + ELSE -1 + END) + AND {incomingRank} < {TerminalRank} + AND UpdatedAtUtc < {siteCall.UpdatedAtUtc} ) );", ct); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/SiteCallAuditRepositoryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/SiteCallAuditRepositoryTests.cs index 2a3e07ea..47aa2799 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/SiteCallAuditRepositoryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/SiteCallAuditRepositoryTests.cs @@ -95,19 +95,23 @@ public class SiteCallAuditRepositoryTests : IClassFixture } [SkippableFact] - public async Task UpsertAsync_SameStatus_IsNoOp() + public async Task UpsertAsync_SameStatus_EqualUpdatedAt_IsNoOp() { Skip.IfNot(_fixture.Available, _fixture.SkipReason); var id = TrackedOperationId.New(); + var stamp = new DateTime(2026, 6, 1, 9, 0, 0, DateTimeKind.Utc); await using var context = CreateContext(); var repo = new SiteCallAuditRepository(context); - await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 1, lastError: "first")); + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 1, lastError: "first", updatedAtUtc: stamp)); var snapshot = await repo.GetAsync(id); - // Same rank (2) — repository must treat this as a no-op (no fields move). - await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 2, lastError: "second")); + // Same rank (2) AND an EQUAL UpdatedAtUtc — an idempotent replay of the + // same telemetry packet. The same-rank freshness tiebreaker only fires + // for a STRICTLY-newer stamp, so a true duplicate stays a no-op (no + // fields move). Task 11: newest-within-rank wins, equal stamps are inert. + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 2, lastError: "second", updatedAtUtc: stamp)); var afterDuplicate = await repo.GetAsync(id); Assert.NotNull(afterDuplicate); @@ -117,21 +121,92 @@ public class SiteCallAuditRepositoryTests : IClassFixture Assert.Equal(snapshot!.UpdatedAtUtc, afterDuplicate.UpdatedAtUtc); } + [SkippableFact] + public async Task UpsertAsync_SameRank_NewerUpdatedAt_RefreshesProgressFields() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + var id = TrackedOperationId.New(); + var t0 = new DateTime(2026, 6, 1, 10, 0, 0, DateTimeKind.Utc); + await using var context = CreateContext(); + var repo = new SiteCallAuditRepository(context); + + // Two Attempted packets (rank 2) from one retrying call; the second + // carries a strictly-newer UpdatedAtUtc, so its live RetryCount/LastError + // must land — the row no longer freezes at first-Attempted (Task 11). + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 1, lastError: "timeout #1", updatedAtUtc: t0)); + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 5, lastError: "timeout #5", updatedAtUtc: t0.AddMinutes(5))); + + var row = await repo.GetAsync(id); + Assert.NotNull(row); + Assert.Equal(5, row!.RetryCount); + Assert.Equal("timeout #5", row.LastError); + Assert.Equal(t0.AddMinutes(5), row.UpdatedAtUtc); + } + + [SkippableFact] + public async Task UpsertAsync_SameRank_OlderUpdatedAt_IsNoOp() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + var id = TrackedOperationId.New(); + var t0 = new DateTime(2026, 6, 1, 11, 0, 0, DateTimeKind.Utc); + await using var context = CreateContext(); + var repo = new SiteCallAuditRepository(context); + + // Land retry 5 (stamp t0+5), then an out-of-order retry-2 packet with an + // OLDER stamp (t0). Same rank, older UpdatedAtUtc → rejected; stays at 5. + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 5, lastError: "timeout #5", updatedAtUtc: t0.AddMinutes(5))); + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 2, lastError: "timeout #2", updatedAtUtc: t0)); + + var row = await repo.GetAsync(id); + Assert.NotNull(row); + Assert.Equal(5, row!.RetryCount); + Assert.Equal("timeout #5", row.LastError); + Assert.Equal(t0.AddMinutes(5), row.UpdatedAtUtc); + } + + [SkippableFact] + public async Task UpsertAsync_LowerRank_NewerUpdatedAt_IsStillNoOp() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + var id = TrackedOperationId.New(); + var t0 = new DateTime(2026, 6, 1, 12, 0, 0, DateTimeKind.Utc); + await using var context = CreateContext(); + var repo = new SiteCallAuditRepository(context); + + // Terminal Delivered (rank 3), then a late lower-rank Attempted (rank 2) + // with a NEWER stamp. Rank monotonicity dominates the freshness + // tiebreaker — the row stays Delivered (regression-proof). + await repo.UpsertAsync(NewRow(id, status: "Delivered", retryCount: 0, updatedAtUtc: t0, terminal: true, terminalAtUtc: t0)); + await repo.UpsertAsync(NewRow(id, status: "Attempted", retryCount: 9, lastError: "late", updatedAtUtc: t0.AddMinutes(5))); + + var row = await repo.GetAsync(id); + Assert.NotNull(row); + Assert.Equal("Delivered", row!.Status); + Assert.Equal(0, row.RetryCount); + } + [SkippableFact] public async Task UpsertAsync_TerminalOverTerminal_IsNoOp() { Skip.IfNot(_fixture.Available, _fixture.SkipReason); // Bundle B3 plan: terminal statuses share rank 3 and are mutually - // exclusive — Delivered cannot overwrite Parked. + // exclusive — Delivered cannot overwrite Parked. Task 11 note: the + // Delivered packet carries a STRICTLY-NEWER UpdatedAtUtc, yet the row + // stays Parked — the same-rank freshness tiebreaker is scoped to + // NON-terminal ranks (< 3), so terminal immutability is preserved. var id = TrackedOperationId.New(); + var t0 = new DateTime(2026, 6, 1, 13, 0, 0, DateTimeKind.Utc); await using var context = CreateContext(); var repo = new SiteCallAuditRepository(context); - await repo.UpsertAsync(NewRow(id, status: "Parked", retryCount: 3, lastError: "parked-reason", terminal: true)); + await repo.UpsertAsync(NewRow(id, status: "Parked", retryCount: 3, lastError: "parked-reason", updatedAtUtc: t0, terminal: true, terminalAtUtc: t0)); var afterPark = await repo.GetAsync(id); - await repo.UpsertAsync(NewRow(id, status: "Delivered", retryCount: 4, lastError: null, terminal: true)); + await repo.UpsertAsync(NewRow(id, status: "Delivered", retryCount: 4, lastError: null, updatedAtUtc: t0.AddMinutes(5), terminal: true, terminalAtUtc: t0.AddMinutes(5))); var afterDeliveredAttempt = await repo.GetAsync(id); Assert.NotNull(afterDeliveredAttempt);