fix(data-connection-layer): resolve DataConnectionLayer-014..017 — real logger for OPC UA client, initial-connect failover, accurate subscribe response, per-tag write-batch results

code-reviews/DataConnectionLayer/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-17 |
 | Reviewer | claude-agent |
 | Commit reviewed | `39d737e` |
-| Open findings | 4 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -688,7 +688,7 @@ guard), and it passes against the atomic fix.
 |--|--|
 | Severity | High |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.DataConnectionLayer/Adapters/RealOpcUaClient.cs:325`, `src/ScadaLink.DataConnectionLayer/Adapters/RealOpcUaClient.cs:35-39,79-83` |
 
 **Description**
@@ -724,7 +724,18 @@ the default is `false`.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). `RealOpcUaClientFactory` gained an
+`ILoggerFactory` constructor parameter and `Create()` now passes
+`_loggerFactory.CreateLogger<RealOpcUaClient>()` into every `RealOpcUaClient` it builds;
+`DataConnectionFactory` (which already holds an `ILoggerFactory`) now constructs
+`RealOpcUaClientFactory(globalOptions, _loggerFactory)`, so the DCL-012 auto-accept-cert
+warning reaches a real logger in production instead of being discarded by `NullLogger`.
+The parameterless / single-arg factory constructors still default to `NullLoggerFactory`
+so existing callers are unaffected. Regression tests
+`DCL014_RealOpcUaClientFactory_CreatesClientWithRealLogger` and
+`DCL014_DataConnectionFactory_ThreadsLoggerToRealOpcUaClient` fail against the pre-fix
+code (the first fails to compile — no logger ctor; the second observes a `NullLogger`)
+and pass after.
 
 ### DataConnectionLayer-015 — Initial-connect failures never trigger failover; an unreachable primary at startup never tries the backup
 
@@ -732,7 +743,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.DataConnectionLayer/Actors/DataConnectionActor.cs:404-417`, `src/ScadaLink.DataConnectionLayer/Actors/DataConnectionActor.cs:419-493` |
 
 **Description**
@@ -769,7 +780,16 @@ backup".
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). The endpoint-switch failover logic was extracted
+from `HandleReconnectResult` into a shared `CountFailureAndMaybeFailover` helper, and
+`HandleConnectResult` (the initial-connect handler in the `Connecting` state) now
+increments `_consecutiveFailures` and calls that helper on failure — so a primary that
+is unreachable at startup fails over to the configured backup after
+`FailoverRetryCount` attempts instead of retrying the primary forever. Single-endpoint
+connections (no backup) still retry indefinitely. Regression test
+`DCL015_PrimaryDownAtStartup_FailsOverToBackup` fails against the pre-fix code (the
+backup adapter is never created) and passes after; `DCL015_SingleEndpointDownAtStartup_RetriesIndefinitely_NoFailover`
+guards the no-backup path.
 
 ### DataConnectionLayer-016 — `HandleSubscribeCompleted` reports `SubscribeTagsResponse` success even on a connection-level subscribe failure
 
@@ -777,7 +797,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.DataConnectionLayer/Actors/DataConnectionActor.cs:606,666-672`, `src/ScadaLink.DataConnectionLayer/Actors/DataConnectionActor.cs:232-240` |
 
 **Description**
@@ -814,7 +834,16 @@ Instance Actor can reflect partial success. Add a test asserting the response is
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). `HandleSubscribeCompleted` now replies
+`SubscribeTagsResponse(success: false, error: "connection unavailable — will
+re-subscribe on reconnect")` when `connectionLevelFailure` is true — matching the
+actor's own assessment as it drives into `Reconnecting` — instead of unconditionally
+replying `success: true`. Genuine tag-resolution failures still reply `success: true`
+(they are a runtime quality concern tracked via `_unresolvedTags`, with a `Bad`-quality
+`TagValueUpdate` already pushed), so that case is not regressed. Regression test
+`DCL016_ConnectionLevelSubscribeFailure_RepliesWithUnsuccessfulResponse` fails against
+the pre-fix code (it returned `Success: true`) and passes after;
+`DCL016_GenuineResolutionFailure_StillRepliesSuccess` guards the resolution-failure path.
 
 ### DataConnectionLayer-017 — `WriteBatchAsync` aborts the whole batch on a mid-batch disconnect
 
@@ -822,7 +851,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.DataConnectionLayer/Adapters/OpcUaDataConnection.cs:229-237`, `src/ScadaLink.DataConnectionLayer/Adapters/OpcUaDataConnection.cs:218-227` |
 
 **Description**
@@ -856,4 +885,14 @@ complete, consistent result map.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). `WriteBatchAsync` now wraps each per-tag
+`WriteAsync` call in a try/catch — mirroring the DCL-007 fix for `ReadBatchAsync` — so a
+mid-batch fault (the `InvalidOperationException` from `EnsureConnected` on a dropped
+connection) is recorded as a failed `WriteResult(false, ex.Message)` for that tag and
+the batch returns a complete result map; `OperationCanceledException` is still
+propagated so a cancelled batch aborts as a whole. Callers (including
+`WriteBatchAndWaitAsync`) now get a consistent per-tag result shape instead of an
+unhandled exception. Regression test
+`DCL017_WriteBatch_ReturnsPerTagResults_WhenConnectionDropsMidBatch` fails against the
+pre-fix code (the batch throws, no map returned) and passes after;
+`DCL017_WriteBatch_CancellationAbortsWholeBatch` guards that cancellation still aborts.