docs/Driver.TwinCAT.Cli.md

@@ -77,6 +77,14 @@ otopcua-twincat-cli read -n 192.168.1.40.1.1 -s "Recipe[3]" -t Real
 otopcua-twincat-cli read -n 192.168.1.40.1.1 -s GVL.sMessage -t WString
 ```
 
+ADS variable handles for `read` / `write` symbols are cached transparently
+inside the CLI's underlying `AdsTwinCATClient`. The first read of a symbol
+resolves a handle; repeats reuse the cached handle for smaller AMS payloads
+and skipped name resolution. The cache wipes on reconnect, on
+`DeviceSymbolVersionInvalid` (with a one-shot retry), and on CLI exit. See
+`docs/drivers/TwinCAT-Test-Fixture.md §Handle caching` for the full story
+including the staleness caveat after an online change.
+
 ### `write`
 
 ```powershell
@@ -99,3 +107,7 @@ otopcua-twincat-cli subscribe -n 192.168.1.40.1.1 -s GVL.Counter -t DInt -i 500
 
 The subscribe banner announces which mechanism is in play — "ADS notification"
 or "polling" — so it's obvious in screen-recorded bug reports.
+
+`--poll-only` polls go through the same cached-handle path as `read`, so
+repeated polls of the same symbol carry only a 4-byte handle on the wire
+rather than the full symbolic path.

docs/drivers/TwinCAT-Test-Fixture.md

@@ -154,6 +154,36 @@ The required fixture state (1000-DINT GVL + churn POU) is documented in
 `TwinCatProject/README.md §Performance scenarios`; XAE-form sources land at
 `TwinCatProject/PLC/GVLs/GVL_Perf.TcGVL` + `TwinCatProject/PLC/POUs/FB_PerfChurn.TcPOU`.
 
+### Handle caching (PR 2.2)
+
+Per-tag reads / writes route through an in-process ADS variable-handle cache.
+The first read of a symbol resolves a handle via `CreateVariableHandleAsync`;
+subsequent reads / writes of the same symbol issue against the cached handle.
+On the wire this trades a multi-byte symbolic path (`GVL_Perf.aTags[742]` =
+20+ bytes) for a 4-byte handle, and the device server skips name resolution
+on every subsequent op. Cache lifetime is process-scoped; entries are evicted
+on `AdsErrorCode.DeviceSymbolVersionInvalid` (with one retry against a fresh
+handle), wiped on reconnect (handles are per-AMS-session), and deleted
+best-effort on driver disposal.
+
+`TwinCATHandleCachePerfTests.Driver_handle_cache_avoids_repeat_symbol_resolution`
+asserts the contract on real XAR by reading 50 symbols twice and verifying
+the second pass issues zero new `CreateVariableHandleAsync` calls. It runs
+under the standard `[TwinCATFact]` gate (XAR reachable; no `TWINCAT_PERF`
+opt-in needed because 50 symbols is cheap).
+
+**Staleness caveat**: handles can go stale after a TwinCAT online change
+(POU edit + activate). Until PR 2.3 ships the proactive Symbol-Version
+invalidation listener, the safety net is twofold: (1) the
+`DeviceSymbolVersionInvalid` evict-and-retry path catches cases where the
+descriptor moves but the symbol survives, and (2) operators can call
+`ITwinCATClient.FlushOptionalCachesAsync` manually after a known online
+change to wipe the cache without forcing a full reconnect. The bulk
+Sum-read / Sum-write path remains on symbolic paths in PR 2.2 (the bulk
+path's per-call symbol resolution is already amortised across N tags;
+the perf delta vs. handle-batched bulk is marginal — tracked as a
+follow-up for the Phase-2 perf sweep).
+
 ## Follow-up candidates
 
 1. **XAR VM live-population** — scaffolding is in place (this PR); the

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs

@@ -31,6 +31,26 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     // than tracking liveness, matching the AbCip / Modbus / FOCAS pattern from #181.
     private readonly ConcurrentDictionary<string, SemaphoreSlim> _bitWriteLocks = new();
 
+    // PR 2.2 — handle cache. Per-tag read/write resolves a symbolic path to an ADS
+    // variable handle once, then issues every subsequent op against the handle. Smaller
+    // AMS payloads (4-byte handle vs N-byte path) + skips name resolution in the runtime.
+    // Lifetime is process-scoped: cleared on reconnect (EnsureConnected path), wiped on
+    // a Symbol-Version-Invalid retry, and disposed on Dispose. PR 2.3 will wire a
+    // proactive Symbol Version invalidation listener so stale handles after an online
+    // change get evicted before the next read fails — until then, operators can call
+    // FlushOptionalCachesAsync to wipe manually.
+    private readonly ConcurrentDictionary<string, uint> _handleCache = new();
+    private bool _wasConnected;
+    private readonly object _connectionStateGate = new();
+
+    // Test-only counter — number of CreateVariableHandleAsync calls actually issued
+    // (i.e. cache misses). Integration tests assert this stays at the unique-symbol
+    // count after a second pass over the same set.
+    internal int HandleCreateCount;
+
+    /// <summary>Test-only — current size of the handle cache.</summary>
+    internal int HandleCacheCount => _handleCache.Count;
+
     public AdsTwinCATClient()
     {
         _client.AdsNotificationEx += OnAdsNotificationEx;
@@ -43,7 +63,24 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         if (_client.IsConnected) return Task.CompletedTask;
         _client.Timeout = (int)Math.Max(1_000, timeout.TotalMilliseconds);
         var netId = AmsNetId.Parse(address.NetId);
+
+        // PR 2.2 — a fresh AMS session invalidates every cached handle (handle space is
+        // per-session in the ADS device server). Clear before reconnect so any read that
+        // raced with a transient drop never reuses a stale handle from the prior session.
+        // Note: the handles for the prior session are gone with that session — no need to
+        // call DeleteVariableHandleAsync, which would just fail with a transport error.
+        var wasConnected = false;
+        lock (_connectionStateGate)
+        {
+            wasConnected = _wasConnected;
+            _wasConnected = false;
+        }
+        if (wasConnected || !_handleCache.IsEmpty)
+            _handleCache.Clear();
+
         _client.Connect(netId, address.Port);
+
+        lock (_connectionStateGate) _wasConnected = _client.IsConnected;
         return Task.CompletedTask;
     }
 
@@ -59,13 +96,14 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
             var clrType = MapToClrType(type);
             var readType = IsWholeArray(arrayDimensions) ? clrType.MakeArrayType() : clrType;
 
-            var result = await _client.ReadValueAsync(symbolPath, readType, cancellationToken)
+            // PR 2.2 — handle-based read. EnsureHandleAsync resolves through the cache;
+            // SymbolVersionInvalid evicts + retries once with a fresh handle.
+            var (rawValue, errorCode) = await ReadByHandleWithRetryAsync(symbolPath, readType, cancellationToken)
                 .ConfigureAwait(false);
+            if (errorCode != AdsErrorCode.NoError)
+                return (null, TwinCATStatusMapper.MapAdsError((uint)errorCode));
 
-            if (result.ErrorCode != AdsErrorCode.NoError)
+            var value = rawValue;
-                return (null, TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode));
-
-            var value = result.Value;
             if (IsWholeArray(arrayDimensions))
             {
                 value = PostProcessArray(type, value);
@@ -84,6 +122,92 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         }
     }
 
+    /// <summary>
+    ///     Resolve <paramref name="symbolPath"/> to a cached ADS variable handle (or create one
+    ///     on first use) and dispatch a <see cref="AdsClient.ReadAnyAsync(uint, Type, CancellationToken)"/>.
+    ///     On <see cref="AdsErrorCode.DeviceSymbolVersionInvalid"/> evicts the cached handle
+    ///     + retries once with a freshly-created handle — covers the online-change race where
+    ///     the symbol survives but its descriptor moves.
+    /// </summary>
+    private async Task<(object? value, AdsErrorCode errorCode)> ReadByHandleWithRetryAsync(
+        string symbolPath, Type readType, CancellationToken cancellationToken)
+    {
+        var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        var result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid)
+        {
+            EvictHandle(symbolPath);
+            handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+            result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false);
+        }
+        return (result.Value, result.ErrorCode);
+    }
+
+    /// <summary>
+    ///     Mirror of <see cref="ReadByHandleWithRetryAsync"/> for writes. Returns the final
+    ///     <see cref="AdsErrorCode"/>; the caller maps that to an OPC UA status.
+    /// </summary>
+    private async Task<AdsErrorCode> WriteByHandleWithRetryAsync(
+        string symbolPath, object value, CancellationToken cancellationToken)
+    {
+        var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        var result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid)
+        {
+            EvictHandle(symbolPath);
+            handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+            result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false);
+        }
+        return result.ErrorCode;
+    }
+
+    /// <summary>
+    ///     Lookup-or-create the cached ADS handle for <paramref name="symbolPath"/>. The
+    ///     <see cref="ConcurrentDictionary{TKey, TValue}"/> guarantees publication safety,
+    ///     but two concurrent callers on a cold key may both call
+    ///     <see cref="AdsClient.CreateVariableHandleAsync(string, CancellationToken)"/>.
+    ///     The loser's handle leaks for the lifetime of the process — acceptable cost
+    ///     given how narrow the race window is, and matched by the libplctag / S7 driver
+    ///     handle-cache patterns.
+    /// </summary>
+    internal async ValueTask<uint> EnsureHandleAsync(string symbolPath, CancellationToken cancellationToken)
+    {
+        if (_handleCache.TryGetValue(symbolPath, out var existing))
+            return existing;
+
+        Interlocked.Increment(ref HandleCreateCount);
+        var result = await _client.CreateVariableHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode != AdsErrorCode.NoError)
+            throw new AdsErrorException(
+                $"CreateVariableHandleAsync failed for '{symbolPath}'", result.ErrorCode);
+
+        // GetOrAdd on a hit returns the winning handle; a loser-side DeleteVariableHandle here
+        // would race against an in-flight read using that handle elsewhere in this method, so
+        // we accept the small leak (one-time, per cold key) instead.
+        return _handleCache.GetOrAdd(symbolPath, result.Handle);
+    }
+
+    /// <summary>
+    ///     Evict a single cached handle. Best-effort delete on the wire — the runtime may
+    ///     already have invalidated the handle (Symbol-Version-Invalid path), so we swallow
+    ///     transport / ADS errors here.
+    /// </summary>
+    private void EvictHandle(string symbolPath)
+    {
+        if (!_handleCache.TryRemove(symbolPath, out var handle)) return;
+        try
+        {
+            // Fire-and-forget delete — the cache key is gone, the wire-side cleanup is
+            // strictly courtesy. If the device server is in a state where the handle is
+            // already dead, the delete will fail and we don't care.
+            _ = _client.DeleteVariableHandleAsync(handle, CancellationToken.None);
+        }
+        catch
+        {
+            // Best-effort.
+        }
+    }
+
     private static bool IsWholeArray(int[]? arrayDimensions) =>
         arrayDimensions is { Length: > 0 } && arrayDimensions.All(d => d > 0);
 
@@ -125,11 +249,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         try
         {
             var converted = ConvertForWrite(type, value);
-            var result = await _client.WriteValueAsync(symbolPath, converted, cancellationToken)
+            // PR 2.2 — handle-based write with SymbolVersionInvalid evict-and-retry.
+            var errorCode = await WriteByHandleWithRetryAsync(symbolPath, converted, cancellationToken)
                 .ConfigureAwait(false);
-            return result.ErrorCode == AdsErrorCode.NoError
+            return errorCode == AdsErrorCode.NoError
                 ? TwinCATStatusMapper.Good
-                : TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode);
+                : TwinCATStatusMapper.MapAdsError((uint)errorCode);
         }
         catch (AdsErrorException ex)
         {
@@ -159,19 +284,21 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         await rmwLock.WaitAsync(cancellationToken).ConfigureAwait(false);
         try
         {
-            var read = await _client.ReadValueAsync(parentPath, typeof(uint), cancellationToken)
+            // PR 2.2 — RMW round-trip flows through the same handle cache so that the
+            // parent word's resolved handle is reused on subsequent bit writes too.
+            var (rawCurrent, readErr) = await ReadByHandleWithRetryAsync(parentPath, typeof(uint), cancellationToken)
                 .ConfigureAwait(false);
-            if (read.ErrorCode != AdsErrorCode.NoError)
+            if (readErr != AdsErrorCode.NoError)
-                return TwinCATStatusMapper.MapAdsError((uint)read.ErrorCode);
+                return TwinCATStatusMapper.MapAdsError((uint)readErr);
 
-            var current = Convert.ToUInt32(read.Value ?? 0u);
+            var current = Convert.ToUInt32(rawCurrent ?? 0u);
             var updated = ApplyBit(current, bit, setBit);
 
-            var write = await _client.WriteValueAsync(parentPath, updated, cancellationToken)
+            var writeErr = await WriteByHandleWithRetryAsync(parentPath, updated, cancellationToken)
                 .ConfigureAwait(false);
-            return write.ErrorCode == AdsErrorCode.NoError
+            return writeErr == AdsErrorCode.NoError
                 ? TwinCATStatusMapper.Good
-                : TwinCATStatusMapper.MapAdsError((uint)write.ErrorCode);
+                : TwinCATStatusMapper.MapAdsError((uint)writeErr);
         }
         catch (AdsErrorException ex)
         {
@@ -354,6 +481,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     {
         if (reads.Count == 0) return Array.Empty<(object?, uint)>();
 
+        // PR 2.2 deviation: bulk path stays on symbolic Sum-command (SumInstancePathAnyTypeRead /
+        // SumWriteBySymbolPath). Beckhoff also exposes SumHandleRead / SumWriteByHandle, but
+        // wiring the cached handles into them changes the request layout substantially +
+        // would either need to reuse handles created on the per-tag path (tying lifetimes)
+        // or maintain a parallel handle batch — neither pulls weight against PR 2.1's already
+        // 10-20× win. Tracked as a follow-up for the Phase-2 perf sweep.
         // Build the (path, AnyTypeSpecifier) request envelope. SumInstancePathAnyTypeRead
         // batches all paths into a single ADS Sum-read round-trip (IndexGroup 0xF080 = read
         // multiple items by symbol name with ANY-type marshalling).
@@ -458,9 +591,53 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     {
         _client.AdsNotificationEx -= OnAdsNotificationEx;
         _notifications.Clear();
+
+        // PR 2.2 — release every cached handle on the wire as a good citizen. Best-effort
+        // and bounded to a short window so a hung router doesn't block process shutdown:
+        // each delete is fire-and-forget, errors swallowed. The session itself is about to
+        // tear down anyway, so the device server will reclaim everything regardless.
+        foreach (var kv in _handleCache)
+        {
+            try
+            {
+                _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None);
+            }
+            catch
+            {
+                // Per-entry failures are expected on a closing connection.
+            }
+        }
+        _handleCache.Clear();
+
         _client.Dispose();
     }
 
+    /// <summary>
+    ///     PR 2.2 — flush all process-scoped optional caches (handle cache today). A
+    ///     proactive Symbol Version invalidation listener arrives in PR 2.3 — until then,
+    ///     operators / 2.3-aware callers can wipe the cache manually after a known online
+    ///     change.
+    /// </summary>
+    public Task FlushOptionalCachesAsync()
+    {
+        // Best-effort delete on the wire — a held handle won't survive a redeploy anyway,
+        // but cleaning up matches the Dispose convention.
+        var snapshot = _handleCache.ToArray();
+        _handleCache.Clear();
+        foreach (var kv in snapshot)
+        {
+            try
+            {
+                _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None);
+            }
+            catch
+            {
+                // Best-effort.
+            }
+        }
+        return Task.CompletedTask;
+    }
+
     private sealed class NotificationRegistration(
         string symbolPath,
         TwinCATDataType type,

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs

@@ -114,6 +114,16 @@ public interface ITwinCATClient : IDisposable
     ///     decide whether to drill in via their own walker.
     /// </summary>
     IAsyncEnumerable<TwinCATDiscoveredSymbol> BrowseSymbolsAsync(CancellationToken cancellationToken);
+
+    /// <summary>
+    ///     PR 2.2 — wipe process-scoped optional caches (today: the ADS variable-handle
+    ///     cache backing per-tag reads / writes). Surfaces so operators + the future PR
+    ///     2.3 Symbol-Version invalidation listener can flush stale handles after a known
+    ///     online change without forcing a full reconnect. Safe to call mid-traffic — in-flight
+    ///     reads continue to use the handles they already hold; the next read for a symbol
+    ///     will re-resolve. Best-effort wire-side delete; failures are swallowed.
+    /// </summary>
+    Task FlushOptionalCachesAsync();
 }
 
 /// <summary>Opaque handle for a registered ADS notification. <see cref="IDisposable.Dispose"/> tears it down.</summary>

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj

@@ -26,6 +26,9 @@
 
     <ItemGroup>
         <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests"/>
+        <!-- PR 2.2 — integration tier needs visibility into AdsTwinCATClient + its
+             HandleCreateCount / HandleCacheCount counters to assert the live cache. -->
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests"/>
     </ItemGroup>
 
 </Project>

tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATHandleCachePerfTests.cs

@@ -0,0 +1,107 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests;
+
+/// <summary>
+///     PR 2.2 integration test — exercises the live <see cref="AdsTwinCATClient"/>
+///     handle cache against a real XAR runtime. Reads 50 distinct symbols twice and
+///     asserts the second pass issues zero new <c>CreateVariableHandleAsync</c> calls.
+/// </summary>
+/// <remarks>
+///     Hooks into <c>AdsTwinCATClient.HandleCreateCount</c> + <c>HandleCacheCount</c> via
+///     the <c>InternalsVisibleTo</c> bridge added in PR 2.1. The fixture's skip-reason is
+///     surfaced through <see cref="TwinCATFactAttribute"/> so the test stays green on a
+///     dev box without the XAR VM (and its expiring trial license).
+/// </remarks>
+[Collection("TwinCATXar")]
+[Trait("Category", "Integration")]
+[Trait("Simulator", "TwinCAT-XAR")]
+public sealed class TwinCATHandleCachePerfTests(TwinCATXarFixture sim)
+{
+    private const int TagCount = 50;
+
+    [TwinCATFact]
+    public async Task Driver_handle_cache_avoids_repeat_symbol_resolution()
+    {
+        if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason);
+
+        var deviceAddress = $"ads://{sim.TargetNetId}:{sim.AmsPort}";
+        var tags = new TwinCATTagDefinition[TagCount];
+        var refs = new string[TagCount];
+        for (var i = 0; i < TagCount; i++)
+        {
+            // GVL_Perf.aTags is 1-based per IEC 61131-3 ARRAY declaration. The 1000-element
+            // perf array is shared with TwinCATSumCommandPerfTests; this test only touches
+            // the first 50 indices so it stays cheap on every CI run.
+            var name = $"Perf{i + 1}";
+            refs[i] = name;
+            tags[i] = new TwinCATTagDefinition(
+                Name: name,
+                DeviceHostAddress: deviceAddress,
+                SymbolPath: $"GVL_Perf.aTags[{i + 1}]",
+                DataType: TwinCATDataType.DInt,
+                // ArrayDimensions = [1] forces the per-tag (handle-cached) path rather
+                // than the bulk Sum-read path, which still flows through symbolic paths
+                // by the PR 2.2 deviation note.
+                ArrayDimensions: [1]);
+        }
+
+        var options = new TwinCATDriverOptions
+        {
+            Devices = [new TwinCATDeviceOptions(deviceAddress, "XAR-VM")],
+            Tags = tags,
+            UseNativeNotifications = false,
+            Timeout = TimeSpan.FromSeconds(15),
+            Probe = new TwinCATProbeOptions { Enabled = false },
+        };
+
+        // Factory wrapper to capture the live AdsTwinCATClient and expose its internal
+        // counters back up to the test. Driver-side code only sees ITwinCATClient so this
+        // doesn't leak the implementation type out of the test.
+        var capture = new CapturingFactory();
+        await using var drv = new TwinCATDriver(options, "tc3-handle-cache", capture);
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+
+        // First pass: every symbol is a cache miss. After this pass HandleCreateCount
+        // should equal TagCount.
+        var firstResults = await drv.ReadAsync(refs, TestContext.Current.CancellationToken);
+        firstResults.Count.ShouldBe(TagCount);
+        firstResults.ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.Good);
+
+        capture.Client.ShouldNotBeNull("CapturingFactory should have produced exactly one AdsTwinCATClient");
+        var firstPassCreates = capture.Client!.HandleCreateCount;
+        capture.Client!.HandleCacheCount.ShouldBe(TagCount);
+        firstPassCreates.ShouldBe(TagCount);
+
+        // Second pass: every symbol is a cache hit. HandleCreateCount must not have moved.
+        var secondResults = await drv.ReadAsync(refs, TestContext.Current.CancellationToken);
+        secondResults.Count.ShouldBe(TagCount);
+        secondResults.ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.Good);
+
+        capture.Client!.HandleCreateCount.ShouldBe(
+            firstPassCreates,
+            $"Second pass over {TagCount} symbols should have created zero new handles, " +
+            $"but HandleCreateCount went {firstPassCreates} -> {capture.Client!.HandleCreateCount}.");
+        capture.Client!.HandleCacheCount.ShouldBe(TagCount);
+    }
+
+    /// <summary>
+    ///     Routes <see cref="ITwinCATClientFactory.Create"/> through the production
+    ///     <see cref="AdsTwinCATClientFactory"/> and snapshots the produced client so the
+    ///     test can read its internal handle-cache counters.
+    /// </summary>
+    private sealed class CapturingFactory : ITwinCATClientFactory
+    {
+        public AdsTwinCATClient? Client { get; private set; }
+
+        public ITwinCATClient Create()
+        {
+            var c = new AdsTwinCATClient();
+            Client ??= c; // first one wins — single-device test path.
+            return c;
+        }
+    }
+}

tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs

@@ -20,24 +20,108 @@ internal class FakeTwinCATClient : ITwinCATClient
     public List<(string symbol, TwinCATDataType type, int? bit, int[]? arrayDimensions)> ReadLog { get; } = new();
     public bool ProbeResult { get; set; } = true;
 
+    // ---- PR 2.2: handle-cache tracking ----
+    //
+    // The fake mirrors the production AdsTwinCATClient handle-cache state machine so the
+    // unit + integration tests can assert "second read of X reused the cached handle"
+    // without hitting a real ADS device. EnsureFakeHandle is called by every per-tag read /
+    // write path and increments HandleCreateInvocations only on a cache miss. Tests can
+    // arm SymbolVersionInvalidOnNextRead / Write to drive the evict-and-retry path.
+    public List<string> HandleCreateInvocations { get; } = new();
+    public List<uint> HandleDeleteInvocations { get; } = new();
+    public List<string> ReadByHandleInvocations { get; } = new();
+    public List<string> WriteByHandleInvocations { get; } = new();
+    public int FlushOptionalCachesCount { get; private set; }
+    /// <summary>Inject DeviceSymbolVersionInvalid into the next read of this symbol.</summary>
+    public HashSet<string> SymbolVersionInvalidOnNextRead { get; } = new(StringComparer.OrdinalIgnoreCase);
+    /// <summary>Inject DeviceSymbolVersionInvalid into the next write of this symbol.</summary>
+    public HashSet<string> SymbolVersionInvalidOnNextWrite { get; } = new(StringComparer.OrdinalIgnoreCase);
+
+    private readonly Dictionary<string, uint> _handleCache = new(StringComparer.OrdinalIgnoreCase);
+    private uint _nextHandle = 1;
+
+    private uint EnsureFakeHandle(string symbolPath)
+    {
+        if (_handleCache.TryGetValue(symbolPath, out var existing)) return existing;
+        HandleCreateInvocations.Add(symbolPath);
+        var handle = _nextHandle++;
+        _handleCache[symbolPath] = handle;
+        return handle;
+    }
+
+    private void EvictFakeHandle(string symbolPath)
+    {
+        if (_handleCache.Remove(symbolPath, out var handle))
+            HandleDeleteInvocations.Add(handle);
+    }
+
+    /// <summary>Test helper — current cached-handle count.</summary>
+    public int HandleCacheCount => _handleCache.Count;
+
+    /// <summary>Test helper — true when the symbol currently has a cached handle.</summary>
+    public bool HasCachedHandle(string symbolPath) => _handleCache.ContainsKey(symbolPath);
+
     public virtual Task ConnectAsync(TwinCATAmsAddress address, TimeSpan timeout, CancellationToken ct)
     {
         ConnectCount++;
         if (ThrowOnConnect) throw Exception ?? new InvalidOperationException();
+        // PR 2.2 — production wipes the handle cache on every (re)connect because handle
+        // identity is per-AMS-session. Mirror so tests of the reconnect flow see the same
+        // cache-clear semantics. Existing handles are dead with the prior session, no
+        // wire-side delete needed.
+        if (IsConnected) _handleCache.Clear();
         IsConnected = true;
         return Task.CompletedTask;
     }
 
+    /// <summary>Test helper — simulate a reconnect (ConnectAsync after the connection drops).</summary>
+    public void SimulateReconnect()
+    {
+        IsConnected = false;
+        _handleCache.Clear();
+        IsConnected = true;
+    }
+
     public virtual Task<(object? value, uint status)> ReadValueAsync(
         string symbolPath, TwinCATDataType type, int? bitIndex, int[]? arrayDimensions, CancellationToken ct)
     {
         if (ThrowOnRead) throw Exception ?? new InvalidOperationException();
         ReadLog.Add((symbolPath, type, bitIndex, arrayDimensions));
+
+        // PR 2.2 — mirror the production handle-cache state machine: resolve handle (cache
+        // miss → HandleCreateInvocations++), do read-by-handle, on injected
+        // SymbolVersionInvalid evict + retry once, then deliver the live value.
+        ReadOneByHandle(symbolPath);
+
         var status = ReadStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good;
         var value = Values.TryGetValue(symbolPath, out var v) ? v : null;
         return Task.FromResult((value, status));
     }
 
+    private void ReadOneByHandle(string symbolPath)
+    {
+        EnsureFakeHandle(symbolPath);
+        ReadByHandleInvocations.Add(symbolPath);
+        if (SymbolVersionInvalidOnNextRead.Remove(symbolPath))
+        {
+            EvictFakeHandle(symbolPath);
+            EnsureFakeHandle(symbolPath); // retry — fresh handle
+            ReadByHandleInvocations.Add(symbolPath);
+        }
+    }
+
+    private void WriteOneByHandle(string symbolPath)
+    {
+        EnsureFakeHandle(symbolPath);
+        WriteByHandleInvocations.Add(symbolPath);
+        if (SymbolVersionInvalidOnNextWrite.Remove(symbolPath))
+        {
+            EvictFakeHandle(symbolPath);
+            EnsureFakeHandle(symbolPath); // retry — fresh handle
+            WriteByHandleInvocations.Add(symbolPath);
+        }
+    }
+
     public virtual Task<uint> WriteValueAsync(
         string symbolPath, TwinCATDataType type, int? bitIndex, int[]? arrayDimensions, object? value, CancellationToken ct)
     {
@@ -51,6 +135,10 @@ internal class FakeTwinCATClient : ITwinCATClient
             var parentPath = AdsTwinCATClient.TryGetParentSymbolPath(symbolPath);
             if (parentPath is not null)
             {
+                // RMW touches the parent word twice (read + write); each goes through the
+                // handle cache, exactly mirroring the production path.
+                ReadOneByHandle(parentPath);
+                WriteOneByHandle(parentPath);
                 var current = Values.TryGetValue(parentPath, out var p) && p is not null
                     ? Convert.ToUInt32(p) : 0u;
                 Values[parentPath] = AdsTwinCATClient.ApplyBit(
@@ -59,6 +147,7 @@ internal class FakeTwinCATClient : ITwinCATClient
         }
         else
         {
+            WriteOneByHandle(symbolPath);
             Values[symbolPath] = value;
         }
 
@@ -66,6 +155,15 @@ internal class FakeTwinCATClient : ITwinCATClient
         return Task.FromResult(status);
     }
 
+    public virtual Task FlushOptionalCachesAsync()
+    {
+        FlushOptionalCachesCount++;
+        // Mirror production: emit a delete record per cached handle, then clear.
+        foreach (var kv in _handleCache) HandleDeleteInvocations.Add(kv.Value);
+        _handleCache.Clear();
+        return Task.CompletedTask;
+    }
+
     public virtual Task<bool> ProbeAsync(CancellationToken ct)
     {
         if (ThrowOnProbe) return Task.FromResult(false);
@@ -134,6 +232,10 @@ internal class FakeTwinCATClient : ITwinCATClient
     {
         DisposeCount++;
         IsConnected = false;
+        // PR 2.2 — production deletes cached handles on Dispose; mirror so tests can assert
+        // the fan-out delete count matches.
+        foreach (var kv in _handleCache) HandleDeleteInvocations.Add(kv.Value);
+        _handleCache.Clear();
     }
 
     // ---- notification fake ----

tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHandleCacheTests.cs

@@ -0,0 +1,231 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests;
+
+/// <summary>
+///     PR 2.2 — handle-based access with caching. Verifies the cache state machine
+///     mirrored on <see cref="FakeTwinCATClient"/>: cold-key resolves through
+///     <see cref="ITwinCATClient"/> -> CreateVariableHandle, warm-key reuses the cached
+///     handle, <see cref="TwinCAT.Ads.AdsErrorCode.DeviceSymbolVersionInvalid"/> evicts +
+///     retries, and Dispose / reconnect / FlushOptionalCachesAsync wipe the cache.
+/// </summary>
+/// <remarks>
+///     The fake's state machine is a high-fidelity mirror of <c>AdsTwinCATClient</c>:
+///     the production class is exercised through the same code paths in the integration
+///     tier (<c>TwinCATHandleCachePerfTests</c>) and through the fake at unit tier here —
+///     consistent with how the rest of this driver tests its <c>AdsClient</c> wrapper.
+/// </remarks>
+[Trait("Category", "Unit")]
+public sealed class TwinCATHandleCacheTests
+{
+    private const string DevA = "ads://5.23.91.23.1.1:851";
+
+    private static (TwinCATDriver drv, FakeTwinCATClientFactory factory) NewDriver(params TwinCATTagDefinition[] tags)
+    {
+        var factory = new FakeTwinCATClientFactory();
+        var hosts = tags.Select(t => t.DeviceHostAddress).Distinct().ToArray();
+        if (hosts.Length == 0) hosts = [DevA];
+        var drv = new TwinCATDriver(new TwinCATDriverOptions
+        {
+            Devices = [.. hosts.Select(h => new TwinCATDeviceOptions(h))],
+            Tags = tags,
+            Probe = new TwinCATProbeOptions { Enabled = false },
+        }, "drv-handle", factory);
+        return (drv, factory);
+    }
+
+    [Fact]
+    public async Task Same_symbol_read_twice_creates_single_handle()
+    {
+        // Force the per-tag path (whole-array reads + bit-extracted BOOL skip the bulk
+        // surface in PR 2.1; both still flow through the handle cache). Whole-array is
+        // the simplest single-symbol read for this assertion — see TwinCATSumCommandTests
+        // for the bulk-path's own handle-free path.
+        var fake = new FakeTwinCATClient { Values = { ["MAIN.Speed"] = 42 } };
+        await fake.ReadValueAsync("MAIN.Speed", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("MAIN.Speed", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+
+        fake.HandleCreateInvocations.ShouldBe(["MAIN.Speed"]);
+        fake.ReadByHandleInvocations.Count.ShouldBe(2);
+        fake.HandleCacheCount.ShouldBe(1);
+    }
+
+    [Fact]
+    public async Task Two_distinct_symbols_create_two_handles()
+    {
+        var fake = new FakeTwinCATClient
+        {
+            Values = { ["MAIN.A"] = 1, ["MAIN.B"] = 2 },
+        };
+        await fake.ReadValueAsync("MAIN.A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("MAIN.B", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("MAIN.A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+
+        fake.HandleCreateInvocations.ShouldBe(["MAIN.A", "MAIN.B"]);
+        fake.HandleCacheCount.ShouldBe(2);
+    }
+
+    [Fact]
+    public async Task SymbolVersionInvalid_evicts_and_retries_with_fresh_handle()
+    {
+        var fake = new FakeTwinCATClient { Values = { ["MAIN.Counter"] = 10 } };
+
+        // First read populates the cache.
+        await fake.ReadValueAsync("MAIN.Counter", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCreateInvocations.Count.ShouldBe(1);
+
+        // Arm SymbolVersionInvalid for the next read; the fake's state machine evicts
+        // the cached handle + retries once, exactly as AdsTwinCATClient does on the real wire.
+        fake.SymbolVersionInvalidOnNextRead.Add("MAIN.Counter");
+        await fake.ReadValueAsync("MAIN.Counter", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+
+        // Two creates total — original + retry — and one delete on eviction.
+        fake.HandleCreateInvocations.ShouldBe(["MAIN.Counter", "MAIN.Counter"]);
+        fake.HandleDeleteInvocations.Count.ShouldBe(1);
+        // After the retry the cache is repopulated.
+        fake.HandleCacheCount.ShouldBe(1);
+    }
+
+    [Fact]
+    public async Task SymbolVersionInvalid_on_write_evicts_and_retries()
+    {
+        var fake = new FakeTwinCATClient();
+        await fake.WriteValueAsync("MAIN.Setpoint", TwinCATDataType.DInt, null, null, 100, TestContext.Current.CancellationToken);
+        fake.HandleCreateInvocations.Count.ShouldBe(1);
+
+        fake.SymbolVersionInvalidOnNextWrite.Add("MAIN.Setpoint");
+        await fake.WriteValueAsync("MAIN.Setpoint", TwinCATDataType.DInt, null, null, 200, TestContext.Current.CancellationToken);
+
+        fake.HandleCreateInvocations.ShouldBe(["MAIN.Setpoint", "MAIN.Setpoint"]);
+        fake.HandleDeleteInvocations.Count.ShouldBe(1);
+        fake.HandleCacheCount.ShouldBe(1);
+    }
+
+    [Fact]
+    public async Task Dispose_deletes_all_cached_handles()
+    {
+        var fake = new FakeTwinCATClient
+        {
+            Values = { ["A"] = 1, ["B"] = 2, ["C"] = 3 },
+        };
+        await fake.ReadValueAsync("A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("B", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("C", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCacheCount.ShouldBe(3);
+
+        fake.Dispose();
+
+        fake.HandleCacheCount.ShouldBe(0);
+        fake.HandleDeleteInvocations.Count.ShouldBe(3);
+    }
+
+    [Fact]
+    public async Task FlushOptionalCachesAsync_clears_cache_then_recreates_handle()
+    {
+        var fake = new FakeTwinCATClient { Values = { ["X"] = 1 } };
+        await fake.ReadValueAsync("X", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCreateInvocations.Count.ShouldBe(1);
+        fake.HasCachedHandle("X").ShouldBeTrue();
+
+        await fake.FlushOptionalCachesAsync();
+
+        fake.FlushOptionalCachesCount.ShouldBe(1);
+        fake.HasCachedHandle("X").ShouldBeFalse();
+        fake.HandleDeleteInvocations.Count.ShouldBe(1);
+
+        // Subsequent read recreates.
+        await fake.ReadValueAsync("X", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCreateInvocations.Count.ShouldBe(2);
+    }
+
+    [Fact]
+    public async Task Reconnect_clears_handle_cache()
+    {
+        var fake = new FakeTwinCATClient { Values = { ["MAIN.Y"] = 9 } };
+        await fake.ConnectAsync(new TwinCATAmsAddress("5.23.91.23.1.1", 851), TimeSpan.FromSeconds(5),
+            TestContext.Current.CancellationToken);
+        await fake.ReadValueAsync("MAIN.Y", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCacheCount.ShouldBe(1);
+
+        // Simulate a connection drop + reconnect — handles from the prior session are dead.
+        fake.SimulateReconnect();
+
+        fake.HandleCacheCount.ShouldBe(0);
+        // Next read repopulates with a fresh handle.
+        await fake.ReadValueAsync("MAIN.Y", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken);
+        fake.HandleCreateInvocations.Count.ShouldBe(2);
+    }
+
+    [Fact]
+    public async Task Bulk_path_does_not_consume_handle_cache()
+    {
+        // PR 2.2 deviation note: bulk Sum-read / Sum-write stays on symbolic paths because
+        // the perf win over the per-tag handle path is marginal vs the diff cost. This test
+        // pins the contract — bulk does not create handles, the per-tag path does.
+        var (drv, factory) = NewDriver(
+            new TwinCATTagDefinition("X", DevA, "MAIN.X", TwinCATDataType.DInt),
+            new TwinCATTagDefinition("Y", DevA, "MAIN.Y", TwinCATDataType.DInt));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        factory.Customise = () => new FakeTwinCATClient
+        {
+            Values = { ["MAIN.X"] = 1, ["MAIN.Y"] = 2 },
+        };
+
+        await drv.ReadAsync(["X", "Y"], TestContext.Current.CancellationToken);
+        await drv.ReadAsync(["X", "Y"], TestContext.Current.CancellationToken);
+
+        var client = factory.Clients[0];
+        client.BulkReadInvocations.Count.ShouldBe(2);
+        client.HandleCreateInvocations.Count.ShouldBe(0);
+        client.HandleCacheCount.ShouldBe(0);
+    }
+
+    [Fact]
+    public async Task Per_tag_path_through_driver_uses_handle_cache()
+    {
+        // Whole-array reads route through the per-tag ReadValueAsync path — perfect for
+        // exercising the handle cache via the public driver surface.
+        var (drv, factory) = NewDriver(
+            new TwinCATTagDefinition("Recipe", DevA, "MAIN.Recipe", TwinCATDataType.DInt,
+                ArrayDimensions: [4]));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        factory.Customise = () => new FakeTwinCATClient
+        {
+            Values = { ["MAIN.Recipe"] = new int[] { 1, 2, 3, 4 } },
+        };
+
+        await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken);
+        await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken);
+        await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken);
+
+        var client = factory.Clients[0];
+        client.HandleCreateInvocations.ShouldBe(["MAIN.Recipe"]);
+        client.ReadByHandleInvocations.Count.ShouldBe(3);
+    }
+
+    [Fact]
+    public async Task Bit_RMW_routes_parent_word_through_handle_cache()
+    {
+        // BOOL-in-word writes do read + write of the parent UDINT — both hops should
+        // share the same cached handle for the parent. After three writes there should
+        // still be only one handle for "Flags".
+        var (drv, factory) = NewDriver(
+            new TwinCATTagDefinition("Bit3", DevA, "GVL.Flags.3", TwinCATDataType.Bool));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Flags"] = 0u } };
+
+        await drv.WriteAsync([new WriteRequest("Bit3", true)], TestContext.Current.CancellationToken);
+        await drv.WriteAsync([new WriteRequest("Bit3", false)], TestContext.Current.CancellationToken);
+        await drv.WriteAsync([new WriteRequest("Bit3", true)], TestContext.Current.CancellationToken);
+
+        var client = factory.Clients[0];
+        client.HandleCreateInvocations.ShouldBe(["GVL.Flags"]);
+        client.HandleCacheCount.ShouldBe(1);
+        // Three RMWs = three reads + three writes by handle on the parent.
+        client.ReadByHandleInvocations.Count(x => x == "GVL.Flags").ShouldBe(3);
+        client.WriteByHandleInvocations.Count(x => x == "GVL.Flags").ShouldBe(3);
+    }
+}