diff --git a/archreview/plans/R2-09-driver-fleet-batch-plan.md.tasks.json b/archreview/plans/R2-09-driver-fleet-batch-plan.md.tasks.json index fb0fa1d6..b1f16985 100644 --- a/archreview/plans/R2-09-driver-fleet-batch-plan.md.tasks.json +++ b/archreview/plans/R2-09-driver-fleet-batch-plan.md.tasks.json @@ -42,7 +42,7 @@ { "id": "B2.3", "subject": "B2: wrap ReadSingleAsync/ReadGroupAsync/WriteAsync critical sections in the per-runtime lock; AbCip suite green", - "status": "pending", + "status": "completed", "blockedBy": [ "B2.2" ] 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 3a9612a6..f95d344d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -542,37 +542,49 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, try { var runtime = await EnsureTagRuntimeAsync(device, def, ct).ConfigureAwait(false); - await runtime.ReadAsync(ct).ConfigureAwait(false); - var status = runtime.GetStatus(); - if (status != 0) + // Serialise Read → GetStatus → Decode against the shared runtime — the cached libplctag + // Tag handle is not concurrency-safe and GetStatus returns the LAST op's status (05/STAB-4). + var opLock = device.GetRuntimeLock(def.Name); + await opLock.WaitAsync(ct).ConfigureAwait(false); + try { - // Evict the stale handle so the next call re-creates it. - // 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, - $"libplctag status {status} reading {reference}"); - _logger.LogWarning( - "AbCip read returned non-zero libplctag status {LibplctagStatus} for tag {Tag} on device {Device}; " + - "evicting cached runtime so next call re-creates it", - status, reference, def.DeviceHostAddress); - return; - } + await runtime.ReadAsync(ct).ConfigureAwait(false); - var tagPath = AbCipTagPath.TryParse(def.TagPath); - var bitIndex = tagPath?.BitIndex; - // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an - // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array - // (ElementCount 1). Scalar tags keep the single-value path. - var value = IsArrayTag(def) - ? runtime.DecodeArray(def.DataType, Math.Max(1, def.ElementCount)) - : runtime.DecodeValue(def.DataType, bitIndex); - results[fb.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); - _health = new DriverHealth(DriverState.Healthy, now, null); + var status = runtime.GetStatus(); + if (status != 0) + { + // Evict the stale handle so the next call re-creates it. + // 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, + $"libplctag status {status} reading {reference}"); + _logger.LogWarning( + "AbCip read returned non-zero libplctag status {LibplctagStatus} for tag {Tag} on device {Device}; " + + "evicting cached runtime so next call re-creates it", + status, reference, def.DeviceHostAddress); + return; + } + + var tagPath = AbCipTagPath.TryParse(def.TagPath); + var bitIndex = tagPath?.BitIndex; + // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an + // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array + // (ElementCount 1). Scalar tags keep the single-value path. + var value = IsArrayTag(def) + ? runtime.DecodeArray(def.DataType, Math.Max(1, def.ElementCount)) + : runtime.DecodeValue(def.DataType, bitIndex); + results[fb.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + _health = new DriverHealth(DriverState.Healthy, now, null); + } + finally + { + opLock.Release(); + } } catch (OperationCanceledException) { @@ -612,29 +624,41 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, try { var runtime = await EnsureTagRuntimeAsync(device, parent, ct).ConfigureAwait(false); - await runtime.ReadAsync(ct).ConfigureAwait(false); - var status = runtime.GetStatus(); - if (status != 0) + // Serialise the whole-UDT Read → GetStatus → per-member DecodeValueAt against the shared + // parent runtime — the members decode from that one handle's buffer (05/STAB-4). + var opLock = device.GetRuntimeLock(parent.Name); + await opLock.WaitAsync(ct).ConfigureAwait(false); + try { - EvictRuntime(device, parent.Name); - var mapped = AbCipStatusMapper.MapLibplctagStatus(status); - StampGroupStatus(group, results, now, mapped); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, - $"libplctag status {status} reading UDT {group.ParentName}"); - _logger.LogWarning( - "AbCip whole-UDT read returned non-zero libplctag status {LibplctagStatus} for parent {Parent} " + - "on device {Device}; {MemberCount} member values stamped with mapped status", - status, group.ParentName, parent.DeviceHostAddress, group.Members.Count); - return; - } + await runtime.ReadAsync(ct).ConfigureAwait(false); - foreach (var member in group.Members) - { - var value = runtime.DecodeValueAt(member.Definition.DataType, member.Offset, bitIndex: null); - results[member.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + var status = runtime.GetStatus(); + if (status != 0) + { + EvictRuntime(device, parent.Name); + var mapped = AbCipStatusMapper.MapLibplctagStatus(status); + StampGroupStatus(group, results, now, mapped); + _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, + $"libplctag status {status} reading UDT {group.ParentName}"); + _logger.LogWarning( + "AbCip whole-UDT read returned non-zero libplctag status {LibplctagStatus} for parent {Parent} " + + "on device {Device}; {MemberCount} member values stamped with mapped status", + status, group.ParentName, parent.DeviceHostAddress, group.Members.Count); + return; + } + + foreach (var member in group.Members) + { + var value = runtime.DecodeValueAt(member.Definition.DataType, member.Offset, bitIndex: null); + results[member.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + } + _health = new DriverHealth(DriverState.Healthy, now, null); + } + finally + { + opLock.Release(); } - _health = new DriverHealth(DriverState.Healthy, now, null); } catch (OperationCanceledException) { @@ -706,10 +730,23 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } var runtime = await EnsureTagRuntimeAsync(device, def, cancellationToken).ConfigureAwait(false); - runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); - await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); - var status = runtime.GetStatus(); + // Serialise Encode → Write → GetStatus against the shared runtime — the same cached + // Tag handle may be in use by a concurrent ReadAsync or poll loop (05/STAB-4). + var opLock = device.GetRuntimeLock(def.Name); + await opLock.WaitAsync(cancellationToken).ConfigureAwait(false); + int status; + try + { + runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); + await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); + status = runtime.GetStatus(); + } + finally + { + opLock.Release(); + } + if (status != 0) { EvictRuntime(device, def.Name);