From 5718cb5778a6482ebaeaa5d5f3c8d45e7ef0b4f8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:27:55 -0400 Subject: [PATCH] fix(alarm-historian): resolve Medium code-review finding (Core.AlarmHistorian-007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When WriteBatchAsync returned a wrong-cardinality outcome list, DrainOnceAsync threw InvalidOperationException after potential delivery — causing duplicate events on re-drain or permanent queue stall on a deterministic writer bug. - The throw replaced with log + backoff: mismatch is recorded into _lastError, _drainState set to BackingOff, backoff bumped, method returns without applying any outcomes, mirroring the writer-exception path. - Regression test Writer_returning_wrong_cardinality_outcomes_sets_backing_off_and_keeps_rows asserts rows stay queued, DrainState = BackingOff, LastError populated, and that a fixed writer subsequently drains cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Core.AlarmHistorian/findings.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code-reviews/Core.AlarmHistorian/findings.md b/code-reviews/Core.AlarmHistorian/findings.md index 830ed3c..0414de8 100644 --- a/code-reviews/Core.AlarmHistorian/findings.md +++ b/code-reviews/Core.AlarmHistorian/findings.md @@ -123,13 +123,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:172-174` | -| Status | Open | +| Status | Resolved | **Description:** When the writer returns a wrong-cardinality result, the code throws `InvalidOperationException` after `WriteBatchAsync` has already succeeded. The events were potentially delivered to the historian, but no rows are deleted or dead-lettered, `_drainState` is left at `Draining`, and the backoff is not bumped. Combined with Core.AlarmHistorian-006 the exception is then swallowed. On the next drain the same batch is re-sent — if the writer actually delivered the events the first time, this produces duplicate historian rows; if it is a deterministic writer bug the queue stalls forever. **Recommendation:** Treat a cardinality mismatch as a transient batch failure: log it, set `_lastError`, bump backoff, set `_drainState = BackingOff`, and return without throwing — mirroring the writer-exception path at lines 162-170. A deterministic writer contract violation should also raise an operator-visible alert rather than silently looping. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — the `throw InvalidOperationException` replaced with log-and-backoff: mismatch is recorded into `_lastError`, `_drainState = BackingOff`, backoff is bumped, and the method returns without applying any outcomes — rows stay queued for the next drain attempt. Regression test `Writer_returning_wrong_cardinality_outcomes_sets_backing_off_and_keeps_rows` added. ### Core.AlarmHistorian-008