fix(driver-focas): resolve Medium code-review finding (Driver.FOCAS-003)

Throw InvalidOperationException at InitializeAsync when a tag's
DeviceHostAddress does not match any entry in the Devices list, naming
both the tag and the unresolved host.  Previously the missing-device
check was guarded by a TryGetValue so a typo silently bypassed
capability-matrix validation and deferred the error to per-read
BadNodeIdUnknown — the opposite of the documented "fail at load" goal.

Also resolves findings 004, 005, and 006 in the same file:
- 004: DiscoverAsync now unconditionally emits ViewOnly for all user
  tags; the Writable config field no longer influences security class
  because the wire backend always returns BadNotWritable.
- 005: All _health reads use Volatile.Read and all writes use
  Volatile.Write so concurrent readers observe a consistent reference
  and read-modify-write sequences capture a stable snapshot.
- 006: EnsureConnectedAsync disposes and nulls any existing
  non-connected client before creating a fresh one, preventing
  ObjectDisposedException loops after a HandleRecycle race or teardown.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 09:25:57 -04:00
parent 6bb971c040
commit 5b5e9ad83b
2 changed files with 46 additions and 21 deletions

View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 10 | | Open findings | 9 |
## Checklist coverage ## Checklist coverage
@@ -95,7 +95,7 @@ or op-mode read to be `IsOk` before declaring the capability present.
| Severity | Medium | | Severity | Medium |
| Category | Correctness & logic bugs | | Category | Correctness & logic bugs |
| Location | `FocasDriver.cs:71-79` | | Location | `FocasDriver.cs:71-79` |
| Status | Open | | Status | Resolved |
**Description:** In `InitializeAsync`, capability-matrix validation only runs when **Description:** In `InitializeAsync`, capability-matrix validation only runs when
`_devices.TryGetValue(tag.DeviceHostAddress, out var device)` succeeds. A tag whose `_devices.TryGetValue(tag.DeviceHostAddress, out var device)` succeeds. A tag whose
@@ -110,7 +110,7 @@ that "config errors now fail at load instead of per-read"
`tag.DeviceHostAddress`, throw an `InvalidOperationException` naming the tag and the `tag.DeviceHostAddress`, throw an `InvalidOperationException` naming the tag and the
unresolved device host so the operator fixes the typo at startup. unresolved device host so the operator fixes the typo at startup.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-22 — `InitializeAsync` now throws `InvalidOperationException` naming the tag and the unresolved device when `_devices` does not contain `tag.DeviceHostAddress`, preventing silent skip-and-defer to per-read `BadNodeIdUnknown`.
### Driver.FOCAS-004 ### Driver.FOCAS-004

View File

@@ -25,6 +25,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, FocasTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, FocasTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
private FocasAlarmProjection? _alarmProjection; private FocasAlarmProjection? _alarmProjection;
// _health is read/written from multiple threads (ReadAsync, WriteAsync, ProbeLoopAsync).
// Volatile.Read/Write ensures every thread sees the latest reference without a lock — the
// record is immutable so there is no torn-read risk on the object itself.
private DriverHealth _health = new(DriverState.Unknown, null, null); private DriverHealth _health = new(DriverState.Unknown, null, null);
public event EventHandler<DataChangeEventArgs>? OnDataChange; public event EventHandler<DataChangeEventArgs>? OnDataChange;
@@ -49,7 +52,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken)
{ {
_health = new DriverHealth(DriverState.Initializing, null, null); Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null));
try try
{ {
foreach (var device in _options.Devices) foreach (var device in _options.Devices)
@@ -69,8 +72,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
$"FOCAS tag '{tag.Name}' has invalid Address '{tag.Address}'. " + $"FOCAS tag '{tag.Name}' has invalid Address '{tag.Address}'. " +
$"Expected forms: R100, R100.3, PARAM:1815/0, MACRO:500."); $"Expected forms: R100, R100.3, PARAM:1815/0, MACRO:500.");
if (_devices.TryGetValue(tag.DeviceHostAddress, out var device) if (!_devices.TryGetValue(tag.DeviceHostAddress, out var device))
&& FocasCapabilityMatrix.Validate(device.Options.Series, parsed) is { } reason) throw new InvalidOperationException(
$"FOCAS tag '{tag.Name}' references device '{tag.DeviceHostAddress}' " +
$"which is not in the Devices list. Check for a typo (e.g. wrong port or hostname).");
if (FocasCapabilityMatrix.Validate(device.Options.Series, parsed) is { } reason)
{ {
throw new InvalidOperationException( throw new InvalidOperationException(
$"FOCAS tag '{tag.Name}' ({tag.Address}) rejected by capability matrix: {reason}"); $"FOCAS tag '{tag.Name}' ({tag.Address}) rejected by capability matrix: {reason}");
@@ -111,11 +117,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
} }
} }
_health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null));
} }
catch (Exception ex) catch (Exception ex)
{ {
_health = new DriverHealth(DriverState.Faulted, null, ex.Message); Volatile.Write(ref _health, new DriverHealth(DriverState.Faulted, null, ex.Message));
throw; throw;
} }
return Task.CompletedTask; return Task.CompletedTask;
@@ -150,10 +156,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
} }
_devices.Clear(); _devices.Clear();
_tagsByName.Clear(); _tagsByName.Clear();
_health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null));
} }
public DriverHealth GetHealth() => _health; public DriverHealth GetHealth() => Volatile.Read(ref _health);
public long GetMemoryFootprint() => 0; public long GetMemoryFootprint() => 0;
public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) => Task.CompletedTask; public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) => Task.CompletedTask;
@@ -206,16 +212,18 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
results[i] = new DataValueSnapshot(value, status, now, now); results[i] = new DataValueSnapshot(value, status, now, now);
if (status == FocasStatusMapper.Good) if (status == FocasStatusMapper.Good)
_health = new DriverHealth(DriverState.Healthy, now, null); Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null));
else else
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded,
$"FOCAS status 0x{status:X8} reading {reference}"); Volatile.Read(ref _health).LastSuccessfulRead,
$"FOCAS status 0x{status:X8} reading {reference}"));
} }
catch (OperationCanceledException) { throw; } catch (OperationCanceledException) { throw; }
catch (Exception ex) catch (Exception ex)
{ {
results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now); results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded,
Volatile.Read(ref _health).LastSuccessfulRead, ex.Message));
} }
} }
@@ -261,7 +269,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
catch (NotSupportedException nse) catch (NotSupportedException nse)
{ {
results[i] = new WriteResult(FocasStatusMapper.BadNotSupported); results[i] = new WriteResult(FocasStatusMapper.BadNotSupported);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded,
Volatile.Read(ref _health).LastSuccessfulRead, nse.Message));
} }
catch (Exception ex) when (ex is FormatException or InvalidCastException) catch (Exception ex) when (ex is FormatException or InvalidCastException)
{ {
@@ -274,7 +283,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
catch (Exception ex) catch (Exception ex)
{ {
results[i] = new WriteResult(FocasStatusMapper.BadCommunicationError); results[i] = new WriteResult(FocasStatusMapper.BadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded,
Volatile.Read(ref _health).LastSuccessfulRead, ex.Message));
} }
} }
@@ -369,17 +379,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase)); string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase));
foreach (var tag in tagsForDevice) foreach (var tag in tagsForDevice)
{ {
// The wire backend is read-only by design (WriteAsync returns BadNotWritable
// for every address) so all FOCAS tags are advertised as ViewOnly regardless
// of the Writable config field. Advertising Operate would mislead OPC UA
// clients and the DriverNodeManager ACL layer into granting write permission
// on nodes that can never be written.
deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo( deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo(
FullName: tag.Name, FullName: tag.Name,
DriverDataType: tag.DataType.ToDriverDataType(), DriverDataType: tag.DataType.ToDriverDataType(),
IsArray: false, IsArray: false,
ArrayDim: null, ArrayDim: null,
SecurityClass: tag.Writable SecurityClass: SecurityClassification.ViewOnly,
? SecurityClassification.Operate
: SecurityClassification.ViewOnly,
IsHistorized: false, IsHistorized: false,
IsAlarm: false, IsAlarm: false,
WriteIdempotent: tag.WriteIdempotent)); WriteIdempotent: false));
} }
} }
return Task.CompletedTask; return Task.CompletedTask;
@@ -862,7 +875,19 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
private async Task<IFocasClient> EnsureConnectedAsync(DeviceState device, CancellationToken ct) private async Task<IFocasClient> EnsureConnectedAsync(DeviceState device, CancellationToken ct)
{ {
if (device.Client is { IsConnected: true } c) return c; if (device.Client is { IsConnected: true } c) return c;
device.Client ??= _clientFactory.Create();
// Discard the existing client (if any) before creating a new one. A client that is
// non-null but not connected may have been disposed by a HandleRecycle race or a prior
// teardown — retrying ConnectAsync on a disposed FocasWireClient hits ThrowIfDisposed and
// returns a permanent BadCommunicationError with no recovery. Replacing it unconditionally
// ensures EnsureConnectedAsync always works with a fresh, non-disposed instance.
if (device.Client is not null)
{
device.Client.Dispose();
device.Client = null;
}
device.Client = _clientFactory.Create();
try try
{ {
await device.Client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct).ConfigureAwait(false); await device.Client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct).ConfigureAwait(false);