diff --git a/code-reviews/Driver.Historian.Wonderware.Client/findings.md b/code-reviews/Driver.Historian.Wonderware.Client/findings.md index a5c8d88..fbdc2e3 100644 --- a/code-reviews/Driver.Historian.Wonderware.Client/findings.md +++ b/code-reviews/Driver.Historian.Wonderware.Client/findings.md @@ -64,7 +64,7 @@ and fail loudly. Add a test where the fake returns a partial/reordered sample se | Severity | Medium | | Category | Correctness & logic bugs | | Location | `WonderwareHistorianClient.cs:154-199`, `IAlarmHistorianSink.cs:66-74` | -| Status | Open | +| Status | Resolved | **Description:** `WriteBatchAsync` can never return `HistorianWriteOutcome.PermanentFail`. `HistorianWriteOutcome` defines three states (`Ack`, `RetryPlease`, `PermanentFail`) and @@ -83,7 +83,7 @@ sidecar and client per the Contracts.cs versioning rules, so unrecoverable event dead-lettered. Until then, document explicitly that this writer never produces `PermanentFail` and that poison events retry indefinitely. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — extending the wire contract (replacing `bool[] PerEventOk` with a per-event status enum) requires a coordinated change to the .NET 4.8 sidecar; instead, added a `` XML doc block on `WriteBatchAsync` explicitly stating that `PermanentFail` is never returned, that poison events retry indefinitely until the drain worker's own retry-count limit fires, and that the protocol extension is a tracked follow-up; also added inline `// NOTE` comments in both the success and catch paths. ### Driver.Historian.Wonderware.Client-003 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs index 3c8dd9b..3b5e1ef 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs @@ -194,6 +194,28 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist // ===== IAlarmHistorianWriter ===== + /// + /// Writes a batch of alarm events to the Wonderware historian via the sidecar. + /// + /// + /// + /// PermanentFail limitation (finding 002): this writer never returns + /// . The sidecar wire contract + /// () carries only a per-event + /// boolean (succeeded / did-not-succeed) and provides no unrecoverable vs. + /// transient distinction. A poison event that the historian SDK can never persist + /// (e.g. a permanently malformed row) will therefore retry indefinitely inside the + /// store-and-forward drain worker rather than being moved to the dead-letter table. + /// Extending the protocol to add a per-event status enum (Ack / Retry / Permanent) + /// requires a coordinated additive change to the .NET 4.8 sidecar and is tracked as + /// a follow-up. Until then, the drain worker's own retry-count limit is the + /// backstop against an infinite loop. + /// + /// + /// Transport or deserialization failures return + /// for every event in the batch; the drain worker's backoff controls recovery. + /// + /// public async Task> WriteBatchAsync( IReadOnlyList batch, CancellationToken cancellationToken) { @@ -223,6 +245,8 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist } // Per-event status: PerEventOk[i] = true → Ack; false → RetryPlease. + // NOTE: PermanentFail is never emitted — see for the wire-contract + // limitation and why poison events currently retry rather than dead-letter. var outcomes = new HistorianWriteOutcome[batch.Count]; for (var i = 0; i < batch.Count; i++) { @@ -234,7 +258,7 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist catch { // Transport / deserialization failure — every event is retry-please. The drain - // worker's backoff handles recovery. + // worker's backoff handles recovery. PermanentFail is never emitted (see ). var fail = new HistorianWriteOutcome[batch.Count]; Array.Fill(fail, HistorianWriteOutcome.RetryPlease); return fail;