fix(driver-ablegacy): resolve Low code-review findings (Driver.AbLegacy-005,011,013)

- Driver.AbLegacy-005: optional ILogger<AbLegacyDriver> ctor parameter,
  logged init failure / probe transitions / first non-zero libplctag
  status per device.
- Driver.AbLegacy-011: Dispose() runs the synchronous teardown directly
  instead of bridging via DisposeAsync().AsTask().GetAwaiter().GetResult()
  to remove the documented sync-over-async deadlock pattern.
- Driver.AbLegacy-013: documented the ResolveHost three-tier fallback
  chain in XML and pointed DiscoverAsync's IsArray=false comment at the
  Modbus ArrayCount pattern for the eventual multi-element follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 07:45:31 -04:00
parent 77b8686199
commit f7e3e9885e
5 changed files with 404 additions and 12 deletions

View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 3 |
| Open findings | 0 |
## Checklist coverage
@@ -141,7 +141,7 @@ decode the full 16-bit word and test bit 0.
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `AbLegacyDriver.cs` (whole file) |
| Status | Open |
| Status | Resolved |
**Description:** The driver uses no `ILogger`/Serilog at all. Probe-loop failures,
runtime initialisation failures, libplctag non-zero statuses, and read/write
@@ -155,7 +155,16 @@ string that the next read or write immediately clobbers.
log probe transitions, runtime-init failures, and the first occurrence of a non-zero
libplctag status per device.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `AbLegacyDriver` now accepts an optional
`ILogger<AbLegacyDriver>` (falls back to `NullLogger`), mirroring the Modbus / S7 /
Galaxy driver pattern. `InitializeAsync` catch-path logs the init failure at Error
level; `TransitionDeviceState` logs every probe transition (Warning on downgrade to
Stopped, Information on recovery); `ReadAsync` logs the first non-zero libplctag
status per device at Warning level via a re-armable `DeviceState.FirstNonZeroStatusLogged`
latch so a permanently-bad PLC doesn't flood the rolling file. `AbLegacyDriverFactoryExtensions.Register`
gains an optional `ILoggerFactory` parameter so the Server bootstrap can wire DI
logging when it chooses; the legacy single-arg `CreateInstance` overload stays for
back-compat. Regression coverage in `AbLegacyLoggerInjectionTests`.
### Driver.AbLegacy-006
@@ -293,7 +302,7 @@ into a real PCCC-STS path or delete it as dead code. The same defect exists in
| Severity | Low |
| Category | Performance & resource management |
| Location | `AbLegacyDriver.cs:440` |
| Status | Open |
| Status | Resolved |
**Description:** `Dispose()` is implemented as
`DisposeAsync().AsTask().GetAwaiter().GetResult()` - sync-over-async. `ShutdownAsync`
@@ -306,7 +315,16 @@ single-threaded synchronization context.
must exist, perform the synchronous teardown directly (cancel CTSs, dispose runtimes)
rather than blocking on the async path.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `Dispose()` now performs the synchronous
teardown directly (cancel probe CTSs, dispose runtimes, clear maps) rather than
wrapping `DisposeAsync().AsTask().GetAwaiter().GetResult()`. The poll engine's
`DisposeAsync` is drained with `.ConfigureAwait(false).GetAwaiter().GetResult()` so a
captured single-threaded `SynchronizationContext` can never be the resumption target —
the classic sync-over-async deadlock is structurally ruled out. Regression test
`Dispose_under_single_threaded_sync_context_does_not_deadlock` drives the path
through a cooperative single-threaded `SynchronizationContext` with a 2s pump timeout;
`Dispose_runs_teardown_without_blocking_on_async_wait` and `Dispose_is_idempotent`
cover the cleanup invariants.
### Driver.AbLegacy-012
@@ -345,7 +363,7 @@ unused fields and the doc comments that imply they are load-bearing.
| Severity | Low |
| Category | Code organization & conventions |
| Location | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` |
| Status | Open |
| Status | Resolved |
**Description:** Two minor organisational issues:
1. `ResolveHost` returns `_options.Devices.FirstOrDefault()?.HostAddress ??
@@ -362,4 +380,17 @@ unused fields and the doc comments that imply they are load-bearing.
document why falling back to the instance id is acceptable. For (2), record the
array-addressing gap as a tracked follow-up.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 —
(1) `ResolveHost` carries a new XML-doc block that documents the three-step fallback
chain (known tag → first device → `DriverInstanceId`) and explicitly cites the
`IPerCallHostResolver` contract which requires implementations to return the driver's
default-host string rather than throw on an unknown reference. The instance-id
fallback is therefore the documented single-host behaviour, not a leaky fake. Three
regression tests in `AbLegacyDisposeAndResolveHostTests` pin each branch of the chain
(`ResolveHost_known_reference_returns_tag_device`,
`ResolveHost_unknown_reference_with_devices_returns_first_device`,
`ResolveHost_unknown_reference_no_devices_returns_driver_instance_id`).
(2) `DiscoverAsync` now carries an inline tracked-follow-up comment that calls out
the PCCC-file-as-array gap, notes the consistency with the PR-staged scope in
`docs/v2/driver-specs.md`, and points to the Modbus `ArrayCount` flow as the pattern
to mirror when multi-element addressing lands.