From 3fb654920902cc8b09641dff10654e12d7242473 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:59:53 -0400 Subject: [PATCH] fix(review): resolve deferred code-review findings; S7.Cli verdict, browser CP release, MemberLeft/Exited/Downed, Sql Detach hoist (#625) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- code-reviews/Cluster/findings.md | 5 +- code-reviews/Commons/findings.md | 4 +- code-reviews/Driver.Galaxy/findings.md | 16 +-- .../Driver.OpcUaClient.Browser/findings.md | 4 +- .../Driver.OpcUaClient.Contracts/findings.md | 4 +- code-reviews/Driver.S7.Cli/findings.md | 4 +- code-reviews/Driver.S7.Contracts/findings.md | 4 +- code-reviews/OpcUaServer/findings.md | 11 +- code-reviews/README.md | 18 +-- code-reviews/Runtime/findings.md | 4 +- docs/drivers/S7.md | 6 +- docs/v2/driver-specs.md | 2 +- .../ClusterRoleInfo.cs | 26 +++- .../Commands/ProbeCommand.cs | 31 +++++ .../OpcUaClientBrowseSession.cs | 30 ++++- .../AbandonedTask.cs | 21 ++++ .../ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs | 19 +-- .../SqlPollReader.cs | 16 +-- .../HistoryPaging.cs | 8 +- .../ClusterRoleInfoMemberEventTests.cs | 100 ++++++++++++++++ .../ProbeCommandTests.cs | 66 +++++++++++ ...OpcUaClientBrowseSessionPaginationTests.cs | 112 ++++++++++++++++++ 22 files changed, 423 insertions(+), 88 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/AbandonedTask.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoMemberEventTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ProbeCommandTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionPaginationTests.cs diff --git a/code-reviews/Cluster/findings.md b/code-reviews/Cluster/findings.md index b15d8c7b..3b9418fe 100644 --- a/code-reviews/Cluster/findings.md +++ b/code-reviews/Cluster/findings.md @@ -60,7 +60,7 @@ Cross-cluster regression coverage lives in `Host.IntegrationTests`. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs:110-133` | -| Status | Deferred | +| Status | Resolved | **Description:** `HandleMemberEvent` only adds members on `MemberUp` and removes on `MemberRemoved`. `MemberLeft`, `MemberExited`, `MemberWeaklyUp`, and `MemberDowned` (all @@ -73,8 +73,7 @@ data. **Recommendation:** Add cases for `MemberLeft`, `MemberExited`, and `MemberDowned` that remove the member from `_membersByRole` immediately. -**Resolution:** Deferred — `MembersWithRole` has no production callers today; stale window is -bounded and self-corrects on `MemberRemoved`. Revisit when a caller is added. +**Resolution:** Resolved 2026-10-08 (#625). `MembersWithRole` gained a production caller (the historization scope-peer check in `Runtime/ServiceCollectionExtensions.cs`), so the stale window mattered. `HandleMemberEvent` now removes a member on `MemberLeft`, `MemberExited` and `MemberDowned` as well as `MemberRemoved` (shared `RemoveMember`). Pinned by `ClusterRoleInfoMemberEventTests` (Up add is the positive control; each departing transition empties the role). --- diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index b8996779..b0290e37 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -59,7 +59,7 @@ apply", referencing the old `Phase7*` naming that was renamed to `AddressSpace*` | Severity | Low | | Category | Code organization & conventions | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` | -| Status | Deferred | +| Status | Won't Fix | **Description:** `RestartDriver`, `ReconnectDriver`, and `TestDriverConnect` (plus their result records) carry `Guid CorrelationId` parameters using the raw `System.Guid` type, while every @@ -73,7 +73,7 @@ predate the `CorrelationId` struct. cross-cutting public-API change that touches callers in at least AdminUI, Runtime, and Cluster projects; defer to a coordinated cleanup pass. -**Resolution:** Deferred — cross-cutting public-API change requiring coordinated update across at least three other modules (AdminUI, Runtime, Cluster). Safe to defer. +**Resolution:** Won't Fix (2026-10-08, #625) — accepted divergence. The three messages (`RestartDriver`, `ReconnectDriver`, `TestDriverConnect`) are admin-command round-trips whose `Guid` is generated and matched at the sender; migrating to the `CorrelationId` wrapper is a public-API change across AdminUI, Runtime and Cluster for no behavioural gain. Revisit only if one of those senders is being rewritten anyway. --- diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index 55669027..59f68110 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -381,25 +381,13 @@ corrected package set. | Severity | Low | | Category | Design-document adherence | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/` (no source change), gateway proto contract | -| Status | Deferred | +| Status | Won't Fix | **Description:** The vendored `MxGateway.Contracts.dll` only carries the OLD `MxGateway.Contracts.Proto[.Galaxy]` namespace (PE-namespace dump confirms — `MxGateway.Client`, `MxGateway.Contracts`, `MxGateway.Contracts.Proto`, `MxGateway.Contracts.Proto.Galaxy` only). The sibling `mxaccessgw` repo's live `Protos/mxaccess_gateway.proto`, `mxaccess_worker.proto`, and `galaxy_repository.proto` files now generate into `ZB.MOM.WW.MxGateway.Contracts.Proto.*`. The proto wire format itself can still evolve (new RPCs, renamed fields, removed fields) and the driver has no contract-version handshake (a repo-wide search for `ContractVersion|ProtocolVersion|ApiVersion|WireVersion` in the driver returns nothing) — so a gateway service that evolves its proto past what the vendored client knows will fail silently at runtime: gRPC `UNIMPLEMENTED` for a renamed RPC, default-value reads for a removed scalar field, or worse, a wire-tag collision if a field number is reused. The risk surface grew with vendoring: previously the `ProjectReference` would have hard-failed at build time if the proto changed shape; now the driver builds green against a frozen contract that may not match the running gateway. **Recommendation:** (a) Add a single `Ping`/`GetVersion` RPC call at gateway-session open, comparing the gateway's reported contract version against a string baked into `libs/README.md` (or a `GatewayContractVersion` const) and refusing the session on mismatch with a clear log. (b) Document in `libs/README.md` the exact mxaccessgw commit SHA (and proto-file SHA-256s) the vendored DLLs were built from, so a parity-rig operator can grep the live gateway for the matching commit. (c) Add a soak/parity test that asserts the live gateway's proto descriptor still matches what the vendored DLL expects — fail loud rather than degrade. -**Resolution:** Deferred 2026-05-23 — the recommendation's part (b) -(record the mxaccessgw source-commit SHA in `libs/README.md`) is satisfied -by the Driver.Galaxy-015 resolution, which records both DLLs were built -from mxaccessgw commit `dd7ca1634e2d2b8a866c81f0009bf87ee9427750`. Parts -(a) and (c) — adding a `GetVersion` RPC at session-open and a parity -test against the live gateway's proto descriptor — are substantial new -RPC + plumbing work that is not in scope for this code-review-resolution -sweep. The risk surface is bounded because either of the two unwinding -paths in `libs/README.md` (sibling repo restores `MxGateway.Client.csproj`, -or this driver migrates to the new namespace) will move the codebase -past the vendoring + close this concern naturally. Re-open if neither -unwinding path is taken within the next quarter and the live gateway -service does evolve its proto under the driver. +**Resolution:** Won't Fix (2026-10-08, #625) — part (b) is done (Driver.Galaxy-015 records the mxaccessgw commit SHA); parts (a)/(c) need a new `GetVersion` RPC on the gateway and a live-gateway parity gate, which belongs to the mxaccessgw contract owner, not this driver. Contract drift surfaces as gRPC `UNIMPLEMENTED` at first use. ### Driver.Galaxy-018 diff --git a/code-reviews/Driver.OpcUaClient.Browser/findings.md b/code-reviews/Driver.OpcUaClient.Browser/findings.md index d7740991..32495b9f 100644 --- a/code-reviews/Driver.OpcUaClient.Browser/findings.md +++ b/code-reviews/Driver.OpcUaClient.Browser/findings.md @@ -61,7 +61,7 @@ Although the OpcUaClient browser never issues a server round-trip in `Attributes | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:112-125` | -| Status | Deferred | +| Status | Resolved | **Description:** If the caller's `CancellationToken` fires during the `BrowseNextAsync` pagination loop, `OperationCanceledException` propagates out of the loop and any in-progress server-side continuation point is never released via `BrowseNext(releaseContinuationPoints: true)`. The server holds the cursor until the session is closed. For the transient AdminUI picker session this is low-risk (the session is short-lived and `DisposeAsync` → `CloseAsync` eventually cleans it up server-side), but it represents a server-side resource leak during cancellation. @@ -69,7 +69,7 @@ The identical pattern exists in the runtime driver (`OpcUaClientDriver.cs`), mak **Recommendation:** Catch `OperationCanceledException` in the pagination loop, issue a fire-and-forget `BrowseNext(releaseContinuationPoints: true, continuationPoints: [cp])` before re-throwing, and apply the same fix to `Driver.OpcUaClient`. -**Resolution:** Deferred — awaiting a cross-cutting fix that also updates `Driver.OpcUaClient`. Short session lifetime of the AdminUI picker limits practical impact. +**Resolution:** Resolved 2026-10-08 (#625). The runtime driver was fixed first (Driver.OpcUaClient-016); `OpcUaClientBrowseSession.BrowseOneLevelAsync` now catches `OperationCanceledException` in its `BrowseNextAsync` loop and issues a best-effort `releaseContinuationPoints: true` call on `CancellationToken.None` before rethrowing. Pinned by `OpcUaClientBrowseSessionPaginationTests` (mock `ISession`; an uncancelled pagination is the negative control). --- diff --git a/code-reviews/Driver.OpcUaClient.Contracts/findings.md b/code-reviews/Driver.OpcUaClient.Contracts/findings.md index 59589688..505a28f1 100644 --- a/code-reviews/Driver.OpcUaClient.Contracts/findings.md +++ b/code-reviews/Driver.OpcUaClient.Contracts/findings.md @@ -50,13 +50,13 @@ | Severity | Low | | Category | OtOpcUa conventions / Design-document adherence | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts.csproj:9` | -| Status | Deferred | +| Status | Resolved | **Description:** The Contracts project carries a direct `PackageReference` to `OPCFoundation.NetStandard.Opc.Ua.Client` solely to support `NamespaceMap`. This makes the Contracts project not a lightweight DTO/enum assembly: every consumer (AdminUI serializers, future test helpers, config-only tooling) that references Contracts for `OpcUaClientDriverOptions` and its enums also transitively receives the full OPC UA client SDK. Both `OpcUaClientDriver` and `OpcUaClientDriverBrowser` already reference the SDK directly and could host `NamespaceMap` without any new dependency. Moving `NamespaceMap` to the runtime driver (or to a dedicated shared helper project referenced by both driver and browser) would restore the Contracts project to its intended SDK-free role. **Recommendation:** Move `NamespaceMap` out of Contracts into `ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient` (or an internal shared helper). Remove the `OPCFoundation.NetStandard.Opc.Ua.Client` reference from the Contracts `.csproj`. `NamespaceMap` is not part of the public wire contract; it is an implementation detail of the driver's namespace-stability mechanism. -**Resolution:** Deferred 2026-06-20. Investigated the relocation during the open-findings resolution pass. `NamespaceMap` is consumed by the runtime driver (`Driver.OpcUaClient`) AND the Browser (`Driver.OpcUaClient.Browser`), but the Browser references **only** `Driver.OpcUaClient.Contracts` — it has no ProjectReference to the runtime driver. So the recommended "move it into `Driver.OpcUaClient`" would break the Browser's compile. The only relocation that keeps both consumers building without the Browser taking a runtime-driver dependency is a brand-new dedicated shared-helper project referenced by both — a structural change (new `.csproj`, solution-file edit, two ProjectReference rewrites) disproportionate to a Low architectural-cleanliness finding whose only cost is the OPC UA SDK being transitively visible to Contracts consumers (AdminUI already references the SDK directly via the driver anyway). Left Deferred until an OpcUaClient project-layout refactor is undertaken for an independent reason; revisit then. +**Resolution:** Resolved (verified 2026-10-08, #625). `NamespaceMap` has since moved to the dedicated `Driver.OpcUaClient.Session` leaf project (Gitea #512), referenced by both the runtime driver and the Browser; the Contracts project no longer references the OPC UA client SDK (its only reference is `Core.Abstractions`). This is the shared-helper-project shape the 2026-06-20 deferral said was the only workable one. --- diff --git a/code-reviews/Driver.S7.Cli/findings.md b/code-reviews/Driver.S7.Cli/findings.md index 3a1691a9..1b6b2fa0 100644 --- a/code-reviews/Driver.S7.Cli/findings.md +++ b/code-reviews/Driver.S7.Cli/findings.md @@ -292,10 +292,10 @@ the S7 copy explains why the event handler writes via `console.Output` synchrono | Severity | Low | | Category | Design-document adherence | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs` | -| Status | Deferred | +| Status | Resolved | **Description:** The Modbus CLI `ProbeCommand` was enhanced (Driver.Modbus.Cli-006) with a `ComputeVerdict` method that derives a single `OK / DEGRADED / FAIL` headline from both the driver state and the probe-read snapshot StatusCode. This prevents the contradictory output `Health: Healthy` when the probe snapshot carries a Bad quality code. The S7 `ProbeCommand` has the same contradiction risk — it prints `Health: {health.State}` without cross-checking the snapshot quality. **Recommendation:** Port the `ComputeVerdict` logic from the Modbus CLI to the S7 CLI: derive a verdict from `health.State` + `snapshot[0].StatusCode` and print it as the headline `Verdict:` line, keeping `Health:` for the raw driver state. -**Resolution:** Deferred — enhancement rather than bug; requires mirroring the Modbus CLI's `ComputeVerdict` pattern which is a design change not a correctness fix. No test regression risk from deferring. +**Resolution:** Resolved 2026-10-08 (#625). Ported `ComputeVerdict` to `S7.Cli`'s `ProbeCommand`; the probe now prints a `Verdict:` headline (OK / DEGRADED / FAIL) from driver state + snapshot StatusCode, keeping `Health:` as the raw driver state. Covered by `ProbeCommandTests` in `Driver.S7.Cli.Tests`. diff --git a/code-reviews/Driver.S7.Contracts/findings.md b/code-reviews/Driver.S7.Contracts/findings.md index e84e3a0b..df8b90b4 100644 --- a/code-reviews/Driver.S7.Contracts/findings.md +++ b/code-reviews/Driver.S7.Contracts/findings.md @@ -74,10 +74,10 @@ a category produced nothing rather than leaving it blank. | Severity | Low | | Category | Design-document adherence | | Location | `docs/v2/driver-specs.md:369` | -| Status | Deferred | +| Status | Resolved | **Description:** The `CpuType` table in `docs/v2/driver-specs.md` (§5, "Connection Settings") lists only four values: `S7300`, `S7400`, `S71200`, `S71500`. The `S7CpuType` enum in this module defines seven values — `S7200 = 0`, `Logo0BA8 = 1`, `S7200Smart = 2`, `S7300 = 10`, `S7400 = 20`, `S71200 = 30`, `S71500 = 40` — all mapped through `S7CpuTypeMap.ToS7Net` in the driver. `docs/drivers/S7.md` mentions S7-200 / S7-200 Smart / LOGO! 0BA8 in prose but gives no enum string names or rack/slot conventions. An operator targeting a LOGO! 0BA8 has no documentation reference for the `CpuType` JSON string to use. **Recommendation:** Add `S7200`, `S7200Smart`, `Logo0BA8` rows to the `driver-specs.md` CpuType table (or note limited S7comm support for these legacy families) and add their enum names to `docs/drivers/S7.md`'s rack/slot table. -**Resolution:** Deferred — the fix is in `docs/v2/driver-specs.md` and `docs/drivers/S7.md`, outside this module's source files. No code impact. Tracked as a documentation-only follow-up. +**Resolution:** Resolved 2026-10-08 (#625). `docs/v2/driver-specs.md` CpuType row and `docs/drivers/S7.md` rack/slot section now list `S7200`, `S7200Smart` and `Logo0BA8` with their rack/slot behaviour and the limited-support caveat. diff --git a/code-reviews/OpcUaServer/findings.md b/code-reviews/OpcUaServer/findings.md index d6924b49..4c379453 100644 --- a/code-reviews/OpcUaServer/findings.md +++ b/code-reviews/OpcUaServer/findings.md @@ -162,7 +162,7 @@ out of scope; option (a) here removes the silent-truncation defect for the commo | Severity | Low | | Category | Correctness & logic bugs | | Location | `OtOpcUaNodeManager.cs:1978` (`ServeRawPaged`), `HistoryPaging.cs` (whole), `HistoryPaging.cs:213` (`SliceTieCluster` `next <= endUtc`) | -| Status | Deferred | +| Status | Won't Fix | **Description:** The Raw paging chain treats `endUtc` as an **inclusive** upper bound throughout — the `HistoryContinuationState`/`HistoryPaging` XML docs all say "the original (inclusive) end of @@ -186,14 +186,7 @@ the inclusive/exclusive question requires confirming the Wonderware backend's ac semantics (cross-module / infra), and changing a comparison without that confirmation risks the opposite off-by-one. -**Resolution:** Deferred — 2026-06-20: infra-gated. Resolving the `endUtc` inclusive-vs-exclusive -disagreement requires confirming the actual Wonderware historian backend's boundary semantics, which is -hardware/infra-gated and not reachable from this macOS dev host. The impact is benign and bounded — because -the backend is the authority on which samples exist (a sample at exactly `endUtc` never appears in an -exclusive-end read), the disagreement only ever yields ONE extra empty resume page (`[endUtc, endUtc)` → -GoodNoData, no continuation point) rather than any duplicated or dropped data. Changing the -`SliceTieCluster` comparison / paging XML docs without confirming the live backend boundary risks -introducing the opposite off-by-one, so no code is changed here pending that live confirmation. +**Resolution:** Won't Fix (2026-10-08, #625) — accepted by design. The paging chain treating `endUtc` as inclusive is the conservative side of the exclusive backend contract: worst case is one extra empty resume page, never a dropped or duplicated sample, and the live backend boundary cannot be confirmed from a dev host. The XML doc on `HistoryPaging.SliceTieCluster` now records the ruling. The backward-read (`StartTime == MinValue`) caveat is unchanged: the backend is assumed to return chronological results. ### OpcUaServer-004 diff --git a/code-reviews/README.md b/code-reviews/README.md index 8158cc61..d5bd66ae 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -321,7 +321,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | OpcUaServer-001 | Medium | Resolved | Correctness & logic bugs | `AddressSpacePlan.cs:56` (`AddressSpacePlan.IsEmpty`), `AddressSpacePlan.cs:80` (`AddressSpacePlanner.Compute`) | | OpcUaServer-002 | Medium | Resolved | Correctness & logic bugs | `OtOpcUaNodeManager.cs:1748` (`HistoryReadEvents`), `OtOpcUaNodeManager.cs:1814` (`ClampToInt`) | | OpcUaServer-007 | Medium | Resolved | Correctness & logic bugs | `OtOpcUaNodeManager.cs:674` (`MaterialiseAlarmCondition` → `alarm.OnTimedUnshelve`) | -| Runtime-001 | Medium | Deferred | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs:311-318`; cross-module `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpacePlan.cs:56-61` + `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs:55` | +| Runtime-001 | Medium | Resolved | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs:311-318`; cross-module `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpacePlan.cs:56-61` + `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs:55` | | Runtime-002 | Medium | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs:148-169, 782-828` | | Server-003 | Medium | Resolved | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Phase7/RingBufferHistoryWriter.cs:96-119` | | Server-005 | Medium | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Alarms/AlarmConditionService.cs:166`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:303-311` | @@ -371,12 +371,12 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Client.UI-011 | Low | Resolved | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` | | Client.UI-012 | Low | Resolved | Testing coverage | `ViewModels/AlarmsViewModel.cs:60-93` | | Client.UI-013 | Low | Resolved | Correctness & logic bugs | `ViewModels/AlarmsViewModel.cs:71-82` | -| Cluster-002 | Low | Deferred | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs:110-133` | +| Cluster-002 | Low | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs:110-133` | | Cluster-003 | Low | Resolved | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/HoconLoader.cs:12-16` | | Cluster-004 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/Resources/akka.conf:4,7` | | Cluster-005 | Low | Resolved | Testing coverage | `tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ServiceLevelCalculatorTests.cs` | | Commons-001 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs:79` | -| Commons-002 | Low | Deferred | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` | +| Commons-002 | Low | Won't Fix | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` | | Commons-003 | Low | Resolved | Testing coverage | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`, `DeferredServiceLevelPublisher.cs` | | Commons-004 | Low | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs` | | Configuration-004 | Low | Resolved | OtOpcUa conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs:8`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:417` | @@ -498,7 +498,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.Galaxy-010 | Low | Resolved | Security | `GalaxyDriver.cs:311-341` | | Driver.Galaxy-012 | Low | Resolved | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` | | Driver.Galaxy-013 | Low | Resolved | Design-document adherence | `GalaxyDriver.cs:14-27`, `GalaxyDriver.cs:374-382`, `Config/GalaxyDriverOptions.cs:84-86` | -| Driver.Galaxy-017 | Low | Deferred | Design-document adherence | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/` (no source change), gateway proto contract | +| Driver.Galaxy-017 | Low | Won't Fix | Design-document adherence | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/` (no source change), gateway proto contract | | Driver.Galaxy-018 | Low | Resolved | Documentation & comments | `libs/README.md:32-37`, `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:40-47` | | Driver.Galaxy-019 | Low | Resolved | Error handling & resilience | `Runtime/GatewayGalaxySubscriber.cs:89-100` | | Driver.Galaxy.Browser-003 | Low | Resolved | Code organization & conventions | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` | @@ -572,8 +572,8 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.OpcUaClient-014 | Low | Resolved | Performance & resource management | `OpcUaClientDriver.cs:1138`, `:1314` | | Driver.OpcUaClient-016 | Low | Resolved | Performance & resource management / Error handling & resilience | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs:934-948` | | Driver.OpcUaClient.Browser-001 | Low | Resolved | Correctness & logic bugs | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:66` | -| Driver.OpcUaClient.Browser-002 | Low | Deferred | Performance & resource management | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:112-125` | -| Driver.OpcUaClient.Contracts-002 | Low | Deferred | OtOpcUa conventions / Design-document adherence | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts.csproj:9` | +| Driver.OpcUaClient.Browser-002 | Low | Resolved | Performance & resource management | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:112-125` | +| Driver.OpcUaClient.Contracts-002 | Low | Resolved | OtOpcUa conventions / Design-document adherence | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts.csproj:9` | | Driver.OpcUaClient.Contracts-003 | Low | Resolved | Code organization & conventions | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs:131-138`, `:174-176` | | Driver.S7-003 | Low | Resolved | Correctness & logic bugs | `S7Driver.cs:172`, `S7Driver.cs:255` | | Driver.S7-005 | Low | Resolved | OtOpcUa conventions | `S7Driver.cs:33`, `S7Driver.cs:433` | @@ -588,10 +588,10 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.S7.Cli-009 | Low | Resolved | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:61`, `Commands/ReadCommand.cs`, `Commands/WriteCommand.cs` | | Driver.S7.Cli-010 | Low | Resolved | Code organization & conventions | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs` | | Driver.S7.Cli-011 | Low | Resolved | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:61-67` | -| Driver.S7.Cli-012 | Low | Deferred | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs` | +| Driver.S7.Cli-012 | Low | Resolved | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs` | | Driver.S7.Contracts-001 | Low | Resolved | Documentation & comments | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs:107` | | Driver.S7.Contracts-002 | Low | Resolved | Correctness & logic bugs | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs:38` | -| Driver.S7.Contracts-003 | Low | Deferred | Design-document adherence | `docs/v2/driver-specs.md:369` | +| Driver.S7.Contracts-003 | Low | Resolved | Design-document adherence | `docs/v2/driver-specs.md:369` | | Driver.Sql-004 | Low | Resolved | Error handling & resilience | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:651` | | Driver.Sql-005 | Low | Resolved | Correctness & logic bugs | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:641` | | Driver.Sql-006 | Low | Resolved | Correctness & logic bugs | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:599` | @@ -625,7 +625,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.TwinCAT.Contracts-002 | Low | Resolved | Documentation & comments | `TwinCATDataType.cs:33` (`Structure` member) | | Host-003 | Low | Resolved | Design-document adherence | `docs/ServiceHosting.md` (section "Per-role configuration overlays") | | Host-004 | Low | Resolved | Documentation & comments | `src/Server/ZB.MOM.WW.OtOpcUa.Host/Observability/ObservabilityExtensions.cs:41–42` | -| OpcUaServer-003 | Low | Deferred | Correctness & logic bugs | `OtOpcUaNodeManager.cs:1978` (`ServeRawPaged`), `HistoryPaging.cs` (whole), `HistoryPaging.cs:213` (`SliceTieCluster` `next <= endUtc`) | +| OpcUaServer-003 | Low | Won't Fix | Correctness & logic bugs | `OtOpcUaNodeManager.cs:1978` (`ServeRawPaged`), `HistoryPaging.cs` (whole), `HistoryPaging.cs:213` (`SliceTieCluster` `next <= endUtc`) | | OpcUaServer-004 | Low | Resolved | Error handling & resilience | `OtOpcUaNodeManager.cs:1597` (`ResolveParentFolder`), and every public sink mutator that calls it (`EnsureFolder` 1278, `EnsureVariable` 1335, `MaterialiseAlarmCondition` 597, plus `WriteValue`/`WriteAlarmCondition` `CreateVariable`) | | OpcUaServer-005 | Low | Won't Fix | Testing coverage | `OtOpcUaNodeManager.cs:2049` (`ServeRawPaged` tie-cluster stall path), `OtOpcUaNodeManager.cs:2068` (absurd-burst backstop) | | OpcUaServer-006 | Low | Resolved | Documentation & comments | `OtOpcUaNodeManager.cs:11-30` (class XML doc), `OpcUaApplicationHost.cs:88-93` / `OpcUaApplicationHost.cs:421-423` (F13/F13c follow-up notes) | diff --git a/code-reviews/Runtime/findings.md b/code-reviews/Runtime/findings.md index cba01606..9247c563 100644 --- a/code-reviews/Runtime/findings.md +++ b/code-reviews/Runtime/findings.md @@ -62,7 +62,7 @@ alarms (`ScriptedAlarmHostActor.cs`, `EfAlarmConditionStateStore.cs`, | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs:311-318`; cross-module `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpacePlan.cs:56-61` + `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs:55` | -| Status | Deferred | +| Status | Resolved | **Description:** This is the Runtime side of the orchestrator-handed cross-module item **OpcUaServer-001**: a UNS Area/Line *rename* (display-name-only edit, no Equipment/Tag/Driver/Alarm change) produces an @@ -90,7 +90,7 @@ is necessary but not sufficient. `HandleRebuild` runs `MaterialiseHierarchy` for a non-empty UNS-only plan (it already does, since the short-circuit only fires for a truly-empty plan). No Runtime-only change can close it safely. -**Resolution:** _(deferred — needs OpcUaServer `AddressSpacePlan` rename signal + Commons `EnsureFolder`/`RenameFolder` rename semantics; both outside the Runtime edit boundary. Tracked with OpcUaServer-001.)_ +**Resolution:** Resolved (verified 2026-10-08, #625) — closed by OpcUaServer-001: `AddressSpacePlanner` now diffs `UnsAreas`/`UnsLines` renames (`DiffRenames`), so a display-name-only rename is no longer `IsEmpty`, and the sink renames the existing folder in place. The surgical-deploys work did not change this path. ### Runtime-002 diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 492f2faa..90edc95e 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -57,7 +57,11 @@ operates off a **static, per-site tag list**, not live symbol discovery. pick the family that matches the PLC exactly. `Rack` is almost always `0` (relevant only for distributed S7-400 racks). `Slot` conventions per family: S7-300 = slot 2, S7-400 = slot 2 or 3, S7-1200 / S7-1500 = slot 0 (onboard PN). -A wrong slot causes a connection refusal during the handshake. See +The `CpuType` JSON strings are `S7300`, `S7400`, `S71200`, `S71500` and the legacy +families `S7200` (S7-200), `S7200Smart` (S7-200 Smart) and `Logo0BA8` (LOGO! 0BA8). +For the three legacy families S7.Net uses fixed TSAPs, so `Rack` / `Slot` are not +consulted — leave them `0` / `0`; support is limited S7comm and unverified against +hardware. A wrong slot causes a connection refusal during the handshake. See `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs` for the per-field defaults. diff --git a/docs/v2/driver-specs.md b/docs/v2/driver-specs.md index d7d679a1..a56789d8 100644 --- a/docs/v2/driver-specs.md +++ b/docs/v2/driver-specs.md @@ -366,7 +366,7 @@ Pure managed .NET, MIT license. No native dependencies. | `Host` | string | — | PLC IP address | | `Rack` | int | 0 | Hardware rack number | | `Slot` | int | 0 | CPU slot (S7-300=2, S7-1200/1500=0) | -| `CpuType` | enum | S71500 | `S7300`, `S7400`, `S71200`, `S71500` | +| `CpuType` | enum | S71500 | `S7300`, `S7400`, `S71200`, `S71500`; legacy families `S7200`, `S7200Smart`, `Logo0BA8` (S7.Net uses fixed TSAPs for these, so `Rack`/`Slot` are not consulted — leave `0`/`0`; limited S7comm support, unverified against hardware) | | `TimeoutMs` | int | 5000 | Read/write timeout | | `MaxConcurrentConnections` | int | 1 | Connections to this PLC (usually 1) | diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs index 7dfd2fdd..9cf82799 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs @@ -158,15 +158,35 @@ public sealed class ClusterRoleInfo : IClusterRoleInfo, IDisposable set.Add(up.Member); } break; + // Cluster-002: a leaving / exiting / downed member is no longer a usable member, so + // drop it as soon as that transition lands rather than waiting for MemberRemoved + // (several seconds later). MembersWithRole now has a production caller (the + // historization scope-peer check), which must not defer to a departing partner. + // Member equality is by UniqueAddress, so the status change does not matter here. + case ClusterEvent.MemberLeft left: + RemoveMember(left.Member); + break; + case ClusterEvent.MemberExited exited: + RemoveMember(exited.Member); + break; + case ClusterEvent.MemberDowned downed: + RemoveMember(downed.Member); + break; case ClusterEvent.MemberRemoved removed: - foreach (var role in removed.Member.Roles) - if (_membersByRole.TryGetValue(role, out var set)) - set.Remove(removed.Member); + RemoveMember(removed.Member); break; } } } + // Caller holds _lock. + private void RemoveMember(Member member) + { + foreach (var role in member.Roles) + if (_membersByRole.TryGetValue(role, out var set)) + set.Remove(member); + } + /// Handles a role leader change event. /// The role leader changed event from the cluster. internal void HandleRoleLeaderChanged(ClusterEvent.RoleLeaderChanged evt) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs index edc0ab24..ccc2379b 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs @@ -1,5 +1,6 @@ using CliFx.Attributes; using CliFx.Infrastructure; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Commands; @@ -51,8 +52,14 @@ public sealed class ProbeCommand : S7CommandBase var snapshot = await driver.ReadAsync(["__probe"], ct); var health = driver.GetHealth(); + // Derive the headline verdict from BOTH the driver state AND the probe-read + // StatusCode so the operator never sees `Health: Healthy` over a Bad snapshot + // line (Driver.S7.Cli-012; mirrors Modbus.Cli's ComputeVerdict). + var verdict = ComputeVerdict(health.State, snapshot[0].StatusCode); + await console.Output.WriteLineAsync($"Host: {Host}:{Port}"); await console.Output.WriteLineAsync($"CPU: {CpuType} rack={Rack} slot={Slot}"); + await console.Output.WriteLineAsync($"Verdict: {verdict}"); await console.Output.WriteLineAsync($"Health: {health.State}"); if (health.LastError is { } err) await console.Output.WriteLineAsync($"Last error: {err}"); @@ -77,4 +84,28 @@ public sealed class ProbeCommand : S7CommandBase await console.Output.WriteLineAsync($"Last error: {err}"); } } + + /// + /// Combine the driver-side with the probe snapshot's OPC UA + /// StatusCode into a single headline verdict: FAIL when the driver is not + /// Healthy or the snapshot is Bad, DEGRADED when Healthy but Uncertain, else OK. + /// + /// The driver's health state. + /// The OPC UA status code from the probe read. + /// A string describing the verdict. + public static string ComputeVerdict(DriverState state, uint statusCode) + { + // Top 2 bits encode the quality class: 00 Good, 01 Uncertain, 10/11 Bad. + var qualityClass = statusCode & 0xC0000000u; + var snapshotGood = qualityClass == 0x00000000u; + var snapshotUncertain = qualityClass == 0x40000000u; + + if (state != DriverState.Healthy || !snapshotGood && !snapshotUncertain) + return $"FAIL (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + + if (snapshotUncertain) + return $"DEGRADED (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + + return $"OK (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs index 68a670b3..9df89463 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs @@ -104,11 +104,31 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession var cp = result.ContinuationPoint; while (cp is { Length: > 0 }) { - var next = await _session.BrowseNextAsync( - requestHeader: null, - releaseContinuationPoints: false, - continuationPoints: [cp], - ct: ct).ConfigureAwait(false); + BrowseNextResponse next; + try + { + next = await _session.BrowseNextAsync( + requestHeader: null, + releaseContinuationPoints: false, + continuationPoints: [cp], + ct: ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // Caller cancelled mid-pagination (Driver.OpcUaClient.Browser-002): release the + // server-side cursor on a fresh token (the original is already cancelled). + // Best-effort — mirrors OpcUaClientDriver's pagination loop. + try + { + await _session.BrowseNextAsync( + requestHeader: null, + releaseContinuationPoints: true, + continuationPoints: [cp], + ct: CancellationToken.None).ConfigureAwait(false); + } + catch { /* best-effort — server may have already cleaned up */ } + throw; + } if (next.Results.Count == 0) break; var nextResult = next.Results[0]; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/AbandonedTask.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/AbandonedTask.cs new file mode 100644 index 00000000..7185fd35 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/AbandonedTask.cs @@ -0,0 +1,21 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; + +/// +/// Shared helper for work the driver stops waiting on (a timed-out liveness probe or poll group). +/// +internal static class AbandonedTask +{ + /// + /// Observes an abandoned task's eventual fault so it never surfaces as an unobserved task + /// exception. The task still owns — and disposes — its own connection, and still releases any + /// concurrency slot when it finally completes; that is what keeps a frozen database from + /// accumulating connections. + /// + /// The task being abandoned. + internal static void Detach(Task work) + => _ = work.ContinueWith( + static t => _ = t.Exception, + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs index 402ba588..56b7ba60 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -1048,7 +1048,7 @@ public sealed class SqlDriver /// Both the linked CTS deadline and an outer wait are used, because /// each covers a case the other does not: the CTS handles a provider that does honour /// cancellation, and the outer wait handles one that does not. An abandoned attempt keeps ownership of - /// its own connection and disposes it; observes its eventual fault. + /// its own connection and disposes it; observes its eventual fault. /// /// The work's result type. /// The operation to bound. Receives the deadline-linked token. @@ -1089,13 +1089,13 @@ public sealed class SqlDriver } catch (TimeoutException) { - Detach(running); + AbandonedTask.Detach(running); throw new TimeoutException($"{what} within {(int)budget.TotalMilliseconds} ms."); } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { // Our deadline fired and the provider DID honour the linked token. - Detach(running); + AbandonedTask.Detach(running); throw new TimeoutException($"{what} within {(int)budget.TotalMilliseconds} ms."); } } @@ -1123,19 +1123,6 @@ public sealed class SqlDriver } } - /// - /// Observes an abandoned liveness attempt's eventual fault so it never surfaces as an unobserved - /// task exception. The task still owns — and disposes — its own connection. - /// TODO(M2): a verbatim duplicate of SqlPollReader.Detach; both live in Driver.Sql and - /// could hoist to one internal helper. Left in place here to keep this change off the reader. - /// - private static void Detach(Task work) - => _ = work.ContinueWith( - static t => _ = t.Exception, - CancellationToken.None, - TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, - TaskScheduler.Default); - // ---- endpoint description ---- /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs index 8e17407b..2169fc2f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs @@ -329,7 +329,7 @@ public sealed class SqlPollReader // Either the caller pulled the plug, or our deadline fired and the provider DID honour the // linked token. Both leave the work running with nobody to observe its fault. if (!cancellationToken.IsCancellationRequested) return TimedOut(plan, work); - Detach(work); + AbandonedTask.Detach(work); throw; } } @@ -352,22 +352,10 @@ public sealed class SqlPollReader plan.Model, plan.Members[0].Table, plan.Members.Count, (int)_operationTimeout.TotalMilliseconds); - Detach(work); + AbandonedTask.Detach(work); return GroupResult.TimedOut; } - /// - /// Observes an abandoned group's eventual fault so it never surfaces as an unobserved task - /// exception. The task still owns its connection and still releases the concurrency slot when it - /// finally completes — that is what keeps a frozen database from accumulating connections. - /// - private static void Detach(Task work) - => _ = work.ContinueWith( - static t => _ = t.Exception, - CancellationToken.None, - TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, - TaskScheduler.Default); - /// /// Opens a connection, executes the plan's single command, and materialises the result set. /// The rows are read into memory before the reader is disposed — the slice-back needs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs index ba26dc5f..effb0e99 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs @@ -191,7 +191,13 @@ internal static class HistoryPaging /// How many ties at were already emitted on prior pages. /// The client's per-page cap (NumValuesPerNode); must be > 0 to make progress. /// The single SourceTimestamp every tie in the cluster shares. - /// The (inclusive) upper bound of the read window; unchanged across pages. + /// The (inclusive) upper bound of the read window; unchanged across pages. + /// OpcUaServer-003, accepted by design: the backend contract + /// (IHistoryProvider.ReadRawAsync) treats endUtc as EXCLUSIVE, so the + /// next <= endUtc test here is deliberately the conservative side — at worst one extra + /// empty resume page ([endUtc, endUtc) → GoodNoData, no continuation point), never a + /// dropped or duplicated sample. Tightening to < would be correct only if the live + /// backend's boundary is confirmed exclusive; it is not worth the opposite off-by-one risk. /// The index into the cluster the emitted slice starts at (clamped to the count). /// How many ties the emitted slice contains (may be 0 on a stale-skip self-heal). /// The next page's inclusive start: while the diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoMemberEventTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoMemberEventTests.cs new file mode 100644 index 00000000..024b182c --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoMemberEventTests.cs @@ -0,0 +1,100 @@ +using System.Collections.Immutable; +using System.Net; +using System.Net.Sockets; +using Akka.Actor; +using Akka.Cluster; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// Cluster-002: must drop a member the moment it +/// is leaving / exiting / downed, not several seconds later at MemberRemoved. The cluster +/// events are fed straight into HandleMemberEvent, so no gossip timing is involved. +/// +public sealed class ClusterRoleInfoMemberEventTests +{ + private const string SystemName = "clusterroleinfo-member-event-test"; + + private static int FreePort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + // Member's factory is internal to Akka.Cluster; reflection is the only way to mint one without + // forming a real cluster (the whole point here is to avoid gossip timing). + private static Member NewMember(int port, MemberStatus status) + { + // Pick the 5-argument overload: (UniqueAddress, upNumber, status, roles, appVersion). + var create = typeof(Member) + .GetMethods(System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic + | System.Reflection.BindingFlags.Public) + .Single(m => m.Name == "Create" && m.GetParameters().Length == 5); + var versionType = create.GetParameters()[4].ParameterType; + var args = new object?[] + { + new UniqueAddress(new Address("akka.tcp", SystemName, "127.0.0.1", port), 1), + 1, status, ImmutableHashSet.Create("driver"), + versionType.IsValueType ? Activator.CreateInstance(versionType) : null, + }; + return (Member)create.Invoke(null, args)!; + } + + private static async Task WithInfoAsync(Func body) + { + var port = FreePort(); + var options = new AkkaClusterOptions + { + SystemName = SystemName, Hostname = "127.0.0.1", PublicHostname = "127.0.0.1", + Port = port, Roles = new[] { "driver" }, SeedNodes = Array.Empty(), + }; + var system = ActorSystem.Create(SystemName, Akka.Configuration.ConfigurationFactory.ParseString( + "akka.actor.provider = cluster\nakka.remote.dot-netty.tcp.hostname = 127.0.0.1\n" + + $"akka.remote.dot-netty.tcp.port = {port}")); + try + { + using var info = new ClusterRoleInfo(system, Options.Create(options), + NullLogger.Instance); + await body(info); + } + finally + { + await system.Terminate(); + } + } + + /// Each departing transition removes the member at once; the Up add is the positive control. + [Theory] + [InlineData("left")] + [InlineData("exited")] + [InlineData("downed")] + [InlineData("removed")] + public async Task Departing_transition_removes_the_member_immediately(string transition) + { + await WithInfoAsync(info => + { + var up = NewMember(11111, MemberStatus.Up); + info.HandleMemberEvent(new ClusterEvent.MemberUp(up)); + info.MembersWithRole("driver").Count.ShouldBe(1, "positive control: the Up member is listed"); + + ClusterEvent.IMemberEvent evt = transition switch + { + "left" => new ClusterEvent.MemberLeft(NewMember(11111, MemberStatus.Leaving)), + "exited" => new ClusterEvent.MemberExited(NewMember(11111, MemberStatus.Exiting)), + "downed" => new ClusterEvent.MemberDowned(NewMember(11111, MemberStatus.Down)), + _ => new ClusterEvent.MemberRemoved(NewMember(11111, MemberStatus.Removed), MemberStatus.Exiting), + }; + info.HandleMemberEvent(evt); + + info.MembersWithRole("driver").ShouldBeEmpty(); + return Task.CompletedTask; + }); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ProbeCommandTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ProbeCommandTests.cs new file mode 100644 index 00000000..c91853f3 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ProbeCommandTests.cs @@ -0,0 +1,66 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Covers — the headline-string helper that +/// combines the driver-side with the probe snapshot's OPC UA +/// so the operator never sees the previous +/// contradictory pair (`Health: Healthy` above a `Status: Bad...` snapshot line — see +/// Driver.S7.Cli-012). +/// +[Trait("Category", "Unit")] +public sealed class ProbeCommandTests +{ + /// Verifies that ComputeVerdict returns OK when driver state is Healthy and status is Good. + [Fact] + public void ComputeVerdict_returns_OK_when_state_is_Healthy_and_status_is_Good() + { + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x00000000u); + + verdict.ShouldContain("OK"); + } + + /// Verifies that ComputeVerdict returns FAIL when driver state is not Healthy. + /// The driver state to test. + [Theory] + [InlineData(DriverState.Faulted)] + [InlineData(DriverState.Reconnecting)] + [InlineData(DriverState.Unknown)] + public void ComputeVerdict_returns_FAIL_when_driver_state_is_not_Healthy(DriverState state) + { + var verdict = ProbeCommand.ComputeVerdict(state, statusCode: 0x00000000u); + + verdict.ShouldContain("FAIL"); + } + + /// Verifies that ComputeVerdict returns FAIL when snapshot status is Bad even if driver is Healthy. + /// The OPC UA status code to test. + [Theory] + [InlineData(0x80050000u)] // BadCommunicationError + [InlineData(0x800A0000u)] // BadTimeout + [InlineData(0x80740000u)] // BadTypeMismatch + [InlineData(0x80000000u)] // generic Bad + public void ComputeVerdict_returns_FAIL_when_snapshot_status_is_Bad_even_if_driver_state_Healthy(uint statusCode) + { + // Driver.S7.Cli-012: a successful InitializeAsync sets DriverState.Healthy, but + // the probe read may still fail (snapshot.StatusCode != Good). Previously the + // headline reported Healthy while the snapshot line below showed Bad. The verdict + // must reflect the actual probe-read outcome. + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode); + + verdict.ShouldContain("FAIL"); + } + + /// Verifies that ComputeVerdict returns DEGRADED for uncertain status with healthy driver. + [Fact] + public void ComputeVerdict_returns_DEGRADED_for_uncertain_status_with_healthy_driver() + { + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x40000000u); + + verdict.ShouldContain("DEGRADED"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionPaginationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionPaginationTests.cs new file mode 100644 index 00000000..59b5fba9 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionPaginationTests.cs @@ -0,0 +1,112 @@ +using Moq; +using Opc.Ua; +using Opc.Ua.Client; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests; + +/// +/// Driver.OpcUaClient.Browser-002: when the caller cancels during the BrowseNextAsync +/// pagination loop, the server-side continuation point must be released +/// (releaseContinuationPoints: true) before the cancellation propagates. Mirrors the +/// runtime driver's OpcUaClientContinuationPointReleaseTests; no live server needed. +/// +[Trait("Category", "Unit")] +public sealed class OpcUaClientBrowseSessionPaginationTests +{ + private static readonly byte[] FakeContinuationPoint = [0x01, 0x02, 0x03]; + + private static Mock BuildSession(bool cancelNext) + { + var mock = new Mock(MockBehavior.Loose); + mock.SetupGet(s => s.MessageContext).Returns(new ServiceMessageContext()); + mock.SetupGet(s => s.NamespaceUris).Returns(new NamespaceTable()); + + var firstRef = new ReferenceDescription + { + NodeId = new ExpandedNodeId(new NodeId(1000u)), + BrowseName = new QualifiedName("TestObject"), + DisplayName = new LocalizedText("TestObject"), + NodeClass = NodeClass.Object, + }; + mock.Setup(s => s.BrowseAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny())) + .ReturnsAsync(new BrowseResponse + { + ResponseHeader = new ResponseHeader(), + Results = new BrowseResultCollection + { + new BrowseResult + { + ContinuationPoint = FakeContinuationPoint, + References = new ReferenceDescriptionCollection { firstRef }, + }, + }, + DiagnosticInfos = new DiagnosticInfoCollection(), + }); + + var emptyPage = new BrowseNextResponse + { + ResponseHeader = new ResponseHeader(), + Results = new BrowseResultCollection + { + new BrowseResult + { + ContinuationPoint = Array.Empty(), + References = new ReferenceDescriptionCollection(), + }, + }, + DiagnosticInfos = new DiagnosticInfoCollection(), + }; + + var pageSetup = mock.Setup(s => s.BrowseNextAsync( + It.IsAny(), It.Is(r => !r), + It.IsAny(), It.IsAny())); + if (cancelNext) pageSetup.ThrowsAsync(new OperationCanceledException("cancelled mid-page")); + else pageSetup.ReturnsAsync(emptyPage); + + mock.Setup(s => s.BrowseNextAsync( + It.IsAny(), It.Is(r => r), + It.IsAny(), It.IsAny())) + .ReturnsAsync(emptyPage); + return mock; + } + + private static OpcUaClientBrowseSession NewSession(Mock mock) + => new(mock.Object, NamespaceMap.FromTable(new NamespaceTable()), ObjectIds.ObjectsFolder); + + /// Cancellation mid-pagination releases the continuation point exactly once. + [Fact] + public async Task RootAsync_releases_continuation_point_when_cancelled_mid_pagination() + { + var mock = BuildSession(cancelNext: true); + await using var session = NewSession(mock); + + await Should.ThrowAsync(async () => + await session.RootAsync(TestContext.Current.CancellationToken)); + + mock.Verify(s => s.BrowseNextAsync( + It.IsAny(), It.Is(r => r), + It.IsAny(), It.IsAny()), Times.Once); + } + + /// Positive control: an uncancelled pagination never issues a release call. + [Fact] + public async Task RootAsync_does_not_release_when_pagination_completes() + { + var mock = BuildSession(cancelNext: false); + await using var session = NewSession(mock); + + var nodes = await session.RootAsync(TestContext.Current.CancellationToken); + + nodes.Count.ShouldBe(1); + mock.Verify(s => s.BrowseNextAsync( + It.IsAny(), It.Is(r => !r), + It.IsAny(), It.IsAny()), Times.Once); + mock.Verify(s => s.BrowseNextAsync( + It.IsAny(), It.Is(r => r), + It.IsAny(), It.IsAny()), Times.Never); + } +}