review(Driver.OpcUaClient.Browser): AttributesAsync updates LastUsedUtc

Review at HEAD 7286d320. -001: AttributesAsync now updates LastUsedUtc (IBrowseSession contract) + test (InternalsVisibleTo+Moq added). -002 (continuation-point cancel leak) deferred cross-cutting w/ runtime Driver.OpcUaClient.
2026-06-19 10:52:23 -04:00
parent 960d76ffcb
commit 2fe8e587dd
5 changed files with 122 additions and 2 deletions
@@ -0,0 +1,72 @@
+# Code Review — Driver.OpcUaClient.Browser
+
+<!-- Per-module findings file. See ../../REVIEW-PROCESS.md for the full process.
+     The base README.md is generated from these files by regen-readme.py — do not edit README.md by hand. -->
+
+| 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
+
+<!-- One ### entry per finding. IDs are Driver.OpcUaClient.Browser-NNN, sequential within the module,
+     never reused. Findings are never deleted — close them by changing Status and completing Resolution. -->
+
+### 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<AttributeInfo>())` 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.
@@ -60,11 +60,16 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession
    }

    /// <summary>The OPC UA picker treats variables as terminal leaves and does not surface
-    /// a per-attribute side-panel, so this always returns empty.</summary>
+    /// a per-attribute side-panel, so this always returns empty. <see cref="LastUsedUtc"/>
+    /// is still refreshed to honour the <see cref="IBrowseSession"/> contract and prevent
+    /// the reaper from evicting an active session that only receives attribute calls.</summary>
    /// <param name="nodeId">Ignored.</param>
    /// <param name="cancellationToken">Ignored.</param>
    public Task<IReadOnlyList<AttributeInfo>> AttributesAsync(string nodeId, CancellationToken cancellationToken)
-        => Task.FromResult<IReadOnlyList<AttributeInfo>>(Array.Empty<AttributeInfo>());
+    {
+        LastUsedUtc = DateTime.UtcNow;
+        return Task.FromResult<IReadOnlyList<AttributeInfo>>(Array.Empty<AttributeInfo>());
+    }

    /// <summary>Issue a single-level Browse (plus continuation-point follow-ups) under the
    /// given parent node. <see cref="Session.BrowseAsync"/> is not thread-safe, so all calls
@@ -14,4 +14,7 @@
        <PackageReference Include="OPCFoundation.NetStandard.Opc.Ua.Configuration" />
        <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
    </ItemGroup>
+    <ItemGroup>
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests"/>
+    </ItemGroup>
 </Project>
@@ -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 ----
+
+    /// <summary>
+    ///     <see cref="IBrowseSession.LastUsedUtc"/> must be updated on every call
+    ///     including <see cref="OpcUaClientBrowseSession.AttributesAsync"/>, which the
+    ///     <see cref="BrowseSessionReaper"/> uses for idle eviction. Before the fix
+    ///     the method returned immediately without touching the property, causing a
+    ///     session that only received <c>AttributesAsync</c> calls to be evicted early.
+    /// </summary>
+    [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<ISession>(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");
+    }
 }
@@ -12,6 +12,7 @@
    <ItemGroup>
        <PackageReference Include="xunit.v3"/>
        <PackageReference Include="Shouldly"/>
+        <PackageReference Include="Moq"/>
        <PackageReference Include="Microsoft.NET.Test.Sdk"/>
        <PackageReference Include="xunit.runner.visualstudio">
            <PrivateAssets>all</PrivateAssets>