diff --git a/code-reviews/Driver.OpcUaClient.Browser/findings.md b/code-reviews/Driver.OpcUaClient.Browser/findings.md new file mode 100644 index 00000000..ea4d9d9e --- /dev/null +++ b/code-reviews/Driver.OpcUaClient.Browser/findings.md @@ -0,0 +1,72 @@ +# Code Review — Driver.OpcUaClient.Browser + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 0 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Finding 001 (LastUsedUtc not updated in AttributesAsync) | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found (disposal race is accepted pattern, same as GalaxyBrowseSession; TTL reaper evicts only idle sessions) | +| 4 | Error handling & resilience | No issues found (BrowseRoot parse failure is caught and session cleaned up; continuation-point cancel-leak is cross-cutting — see 002) | +| 5 | Security | No issues found (separate PKI store correct; AutoAcceptCertificates not forwarded to browse store; no credential logging) | +| 6 | Performance & resource management | Finding 002 (continuation-point server-side leak on cancellation) | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found (unit tests cover pre-connect validation; live tests correctly gated on RequiresOpcPlc) | +| 10 | Documentation & comments | No issues found | + +## Findings + + + +### Driver.OpcUaClient.Browser-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:66` | +| Status | Resolved | + +**Description:** `AttributesAsync` returns `Task.FromResult(Array.Empty())` without updating `LastUsedUtc`. The `IBrowseSession` interface contract (at `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/IBrowseSession.cs:14-15`) states that `LastUsedUtc` is "Refreshed on `RootAsync`, `ExpandAsync`, and `AttributesAsync`". If the AdminUI calls `AttributesAsync` exclusively for 2+ minutes without an intermediate `RootAsync` or `ExpandAsync` (e.g. in a polymorphic picker context), the `BrowseSessionReaper` may evict an active session early. + +Although the OpcUaClient browser never issues a server round-trip in `AttributesAsync` (it always returns empty), the timestamp must be refreshed to honour the contract and keep the session alive while the user is active. + +**Recommendation:** Set `LastUsedUtc = DateTime.UtcNow` inside `AttributesAsync` before returning, mirroring the pattern in `BrowseOneLevelAsync`. + +**Resolution:** Fixed 2026-06-19 (SHA pending commit) — added `LastUsedUtc = DateTime.UtcNow` to `AttributesAsync` body; regression test `AttributesAsync_updates_LastUsedUtc` added to the unit test project. + +--- + +### Driver.OpcUaClient.Browser-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs:112-125` | +| Status | Deferred | + +**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. + +The identical pattern exists in the runtime driver (`OpcUaClientDriver.cs`), making this a cross-cutting concern rather than a module-local defect. + +**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. 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 ef9dacaa..c4493eba 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 @@ -60,11 +60,16 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession } /// The OPC UA picker treats variables as terminal leaves and does not surface - /// a per-attribute side-panel, so this always returns empty. + /// a per-attribute side-panel, so this always returns empty. + /// is still refreshed to honour the contract and prevent + /// the reaper from evicting an active session that only receives attribute calls. /// Ignored. /// Ignored. public Task> AttributesAsync(string nodeId, CancellationToken cancellationToken) - => Task.FromResult>(Array.Empty()); + { + LastUsedUtc = DateTime.UtcNow; + return Task.FromResult>(Array.Empty()); + } /// Issue a single-level Browse (plus continuation-point follow-ups) under the /// given parent node. is not thread-safe, so all calls diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.csproj index a95ccef8..83eccb18 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.csproj @@ -14,4 +14,7 @@ + + + diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs index c61cbd47..969ce158 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs @@ -1,5 +1,9 @@ +using Moq; +using Opc.Ua; +using Opc.Ua.Client; using Shouldly; using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser; namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests; @@ -49,4 +53,39 @@ public sealed class OpcUaClientDriverBrowserTests () => _sut.OpenAsync(json, TestContext.Current.CancellationToken)); ex.Message.ShouldContain("Certificate"); } + + // ---- Driver.OpcUaClient.Browser-001: AttributesAsync must refresh LastUsedUtc ---- + + /// + /// must be updated on every call + /// including , which the + /// uses for idle eviction. Before the fix + /// the method returned immediately without touching the property, causing a + /// session that only received AttributesAsync calls to be evicted early. + /// + [Fact] + public async Task AttributesAsync_updates_LastUsedUtc() + { + var ct = TestContext.Current.CancellationToken; + + // Arrange: build a minimal OpcUaClientBrowseSession without a live server. + // AttributesAsync does not call ISession — MockBehavior.Loose is safe. + var mockSession = new Mock(MockBehavior.Loose); + var nsMap = NamespaceMap.FromTable(new NamespaceTable()); + var sut = new OpcUaClientBrowseSession(mockSession.Object, nsMap, ObjectIds.ObjectsFolder); + + var before = sut.LastUsedUtc; + + // Introduce a small delay so clock advances at least one tick. + await Task.Delay(5, ct); + + // Act + var attrs = await sut.AttributesAsync("nsu=http://opcfoundation.org/UA/;i=85", ct); + + // Assert: LastUsedUtc must have been refreshed, and the result must be empty + // (OPC UA picker treats variables as leaves, no attribute side-panel). + attrs.ShouldBeEmpty(); + sut.LastUsedUtc.ShouldBeGreaterThan(before, + "AttributesAsync must refresh LastUsedUtc to satisfy the IBrowseSession contract"); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests.csproj b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests.csproj index c86f3bdf..84c25286 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests.csproj +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests.csproj @@ -12,6 +12,7 @@ + all