From 72728a5d45a3bdfdc05810f611bbbbdff235c398 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:24:45 -0400 Subject: [PATCH] fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `EvictRuntime` helper that removes + disposes a stale `ConcurrentDictionary` entry. Call it from `ReadSingleAsync`, `ReadGroupAsync`, and `WriteAsync` on non-zero libplctag status and transport exceptions so the next call for the same tag re-creates a fresh handle — mirroring the probe loop's recreate-on-failure pattern. Value-conversion exceptions (NotSupportedException, FormatException, InvalidCastException, OverflowException) are not transport faults and do not evict the handle. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.AbCip/findings.md | 4 +- .../AbCipDriver.cs | 42 +++++++++++++++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index 387bd05..2263f11 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -168,13 +168,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `AbCipDriver.cs:621-648`, `AbCipDriver.cs:346-391` | -| Status | Open | +| Status | Resolved | **Description:** Once `EnsureTagRuntimeAsync` successfully creates and initializes a `LibplctagTagRuntime`, that runtime is cached for the lifetime of the device and never re-created on failure. If the underlying native tag enters a permanently-bad state (connection dropped, controller rebooted, tag handle invalidated by a PLC program download), every subsequent `ReadAsync`/`WriteAsync` reuses the same dead handle and returns errors forever. The probe loop does tear down and recreate its runtime after a failure, but the read/write path has no equivalent recovery; only a full `ReinitializeAsync` (itself broken, see Driver.AbCip-001) clears the cache. The normal data path should self-heal from a transient handle fault without operator-driven reinitialize. **Recommendation:** On a non-zero libplctag status or transport exception in `ReadSingleAsync`/`ReadGroupAsync`/`WriteAsync`, evict the offending runtime from `device.Runtimes` (and dispose it) so the next call re-creates and re-initializes it. Mirror the probe loop recreate-on-failure behavior. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `EvictRuntime(device, tagName)` helper that calls `ConcurrentDictionary.TryRemove` + disposes the evicted instance; called from `ReadSingleAsync`, `ReadGroupAsync`, and `WriteAsync` on both non-zero libplctag status and transport exceptions (type/value-conversion exceptions are not transport faults and do not evict). The next read/write for the affected tag re-runs `EnsureTagRuntimeAsync`, which creates and initializes a fresh handle, mirroring the probe loop's recreate-on-failure behaviour. ### Driver.AbCip-011 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 7ad86c3..764ed56 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -448,6 +448,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var status = runtime.GetStatus(); if (status != 0) { + // Evict the stale handle so the next call re-creates it (Driver.AbCip-010). + // A non-zero status can mean the controller dropped the connection or the tag + // handle became permanently invalid (e.g. after a PLC download). Evicting + // mirrors the probe loop's recreate-on-failure behaviour. + EvictRuntime(device, def.Name); results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.MapLibplctagStatus(status), null, now); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, @@ -467,6 +472,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { + // Transport exception — evict so the next read creates a fresh handle. + EvictRuntime(device, def.Name); results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadCommunicationError, null, now); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); @@ -499,6 +506,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var status = runtime.GetStatus(); if (status != 0) { + EvictRuntime(device, parent.Name); // Driver.AbCip-010 var mapped = AbCipStatusMapper.MapLibplctagStatus(status); StampGroupStatus(group, results, now, mapped); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, @@ -519,6 +527,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { + EvictRuntime(device, parent.Name); // Driver.AbCip-010 StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); } @@ -589,10 +598,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); var status = runtime.GetStatus(); - results[i] = new WriteResult(status == 0 - ? AbCipStatusMapper.Good - : AbCipStatusMapper.MapLibplctagStatus(status)); - if (status == 0) _health = new DriverHealth(DriverState.Healthy, now, null); + if (status != 0) + { + EvictRuntime(device, def.Name); // Driver.AbCip-010 + results[i] = new WriteResult(AbCipStatusMapper.MapLibplctagStatus(status)); + } + else + { + results[i] = new WriteResult(AbCipStatusMapper.Good); + _health = new DriverHealth(DriverState.Healthy, now, null); + } } catch (OperationCanceledException) { @@ -600,11 +615,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (NotSupportedException nse) { + // Type/protocol error — not a transport fault; don't evict the handle. results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); } catch (FormatException fe) { + // Value conversion error — not a transport fault; don't evict. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message); } @@ -620,6 +637,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { + // Transport / wire error — evict so the next write creates a fresh handle. + EvictRuntime(device, def.Name); // Driver.AbCip-010 results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); } @@ -738,6 +757,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, return device.Runtimes[def.Name]; } + /// + /// Evict the runtime for from the device's cache and dispose + /// it so the next read/write call re-creates and re-initializes a fresh handle. + /// Called from , , and + /// after a non-zero libplctag status or transport exception — + /// mirroring the probe loop's recreate-on-failure behaviour (Driver.AbCip-010). + /// + private static void EvictRuntime(DeviceState device, string tagName) + { + if (device.Runtimes.TryRemove(tagName, out var stale)) + { + try { stale.Dispose(); } catch { } + } + } + public DriverHealth GetHealth() => _health; ///