merge: MTConnect review findings resolved (Driver.MTConnect-004 write-back authoring, -005/-006/-007)
# Conflicts: # code-reviews/Driver.MTConnect/findings.md
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-08-10 |
|
||||
| Commit reviewed | `b990adae` |
|
||||
| Status | Complete |
|
||||
| Open findings | 4 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -168,7 +168,7 @@ defect left inside these two modules are corrected under `Driver.MTConnect-008`
|
||||
| Severity | Medium |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor:1` · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:146` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `IWritable` shipped for this driver on 2026-08-05 (`b29623cf`) behind
|
||||
`Write.Enabled`, and `docs/drivers/MTConnect.md` §"Two independent gates" tells the operator to set
|
||||
@@ -192,8 +192,32 @@ edit through the form. The capability is unreachable rather than fragile.
|
||||
comment. Until then the driver-side gate is correct and fails closed, which is why this is Medium
|
||||
rather than High.
|
||||
|
||||
**Resolution:** _(open — the fix belongs in `ZB.MOM.WW.OtOpcUa.AdminUI`, outside this review's
|
||||
modules.)_
|
||||
**Resolution (2026-08-10):** Fixed `298ad04f`. `MTConnectDriverForm` gained a **Write-back**
|
||||
section — a switch plus form text stating that a write is observation injection, that the Agent's own
|
||||
`AllowPut` is the independent second gate, and that arming it does not by itself make any node
|
||||
writable over OPC UA (`Tag.AccessLevel` still composes `CurrentWrite`; CONDITION items never are).
|
||||
`Write` was added to the form's mirrored `MTConnectConfigDto` and to `MTConnectFormModel`, the false
|
||||
"MTConnect is read-only" header comment is gone, and `docs/drivers/MTConnect.md` §"Two independent
|
||||
gates" now names the section instead of implying a config-DB edit.
|
||||
|
||||
⚠️ **The block is written in BOTH states, unlike the timing knobs.** Those are omitted when unusable
|
||||
so `ParseOptions` substitutes the driver's own default; an omitted `write` block would instead leave a
|
||||
previously-authored `"enabled": true` standing — the switch would appear to turn writing off while the
|
||||
deployed driver went on accepting writes. Pinned by
|
||||
`Turning_write_back_off_clears_a_previously_authored_true`, the falsification control for that
|
||||
decision. The finding's own mitigation (unknown top-level keys survive an edit) is likewise not
|
||||
sufficient on its own: `FromJson` had to LEARN the block, or the modal would show the switch off over
|
||||
a config that has writing on — `Write_back_round_trips_and_a_hand_authored_block_loads_into_the_form`.
|
||||
|
||||
This project has no bUnit, so the seam is unit-tested from both ends rather than through the page:
|
||||
`MTConnectDriverFormModelTests` asserts the exact bytes `BuildConfigJson` emits, and
|
||||
`MTConnectFactoryTests.The_document_the_AdminUI_form_emits_arms_write_back` parses that same document
|
||||
through `MTConnectDriver.ParseOptions` and asserts `MTConnectDriverOptions.Write.Enabled` is true
|
||||
(then false for the switched-off variant). The literal is duplicated because a driver test project
|
||||
cannot reference the AdminUI; `The_write_enabled_document_is_the_literal_the_driver_test_parses` pins
|
||||
the two copies together so a rename fails on the authoring side, naming the literal to update.
|
||||
**Residual: live page verification is still a rig step** — the razor markup itself (the switch's
|
||||
binding and `EmitAsync` wiring) is not covered by any test here.
|
||||
|
||||
### Driver.MTConnect-005
|
||||
|
||||
@@ -202,7 +226,7 @@ modules.)_
|
||||
| Severity | Low |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:2655` · `:689` · `:797` · `:1425` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `_options` is the one piece of shared driver state published by a **plain** field
|
||||
write (`_options = options;` at the `StartCoreAsync` commit point) and read lock-free from other
|
||||
@@ -223,7 +247,17 @@ three siblings, or fold it into the `AgentSession` / `TagBinding` snapshot so th
|
||||
consistent unit" rule this file already argues for covers it too. Record-only for now: the change
|
||||
touches ~10 read sites and the exposure does not justify unreviewed churn.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution (2026-08-10):** Fixed `2d7dffcc`. Both writes are `Volatile.Write` (the constructor's as
|
||||
well as the commit point's) and all thirteen reads are `Volatile.Read`; the field gained the remark
|
||||
its three siblings already carried. Two sites took a local snapshot rather than repeating the read —
|
||||
the `_streamUnsupported` refusal, which named the Agent twice, and `GetOrFetchProbeModelAsync`.
|
||||
|
||||
**A source scan is the pin, deliberately.** The defect is a missing fence, and no test can force the
|
||||
interleaving that exposes it on x86/arm64 in bounded time; what actually regresses is a NEW read site
|
||||
written without the fence. `MTConnectOptionsPublicationTests` therefore fails on any unguarded touch
|
||||
of `_options` in `MTConnectDriver.cs` — the same shape as `LibplctagGatewayConsumptionGuardTests`, and
|
||||
red before the change with all 15 sites listed by line. It carries a floor count so the scan cannot go
|
||||
blind if the field is renamed.
|
||||
|
||||
### Driver.MTConnect-006
|
||||
|
||||
@@ -232,7 +266,7 @@ touches ~10 read sites and the exposure does not justify unreviewed churn.
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:914` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `TryFormatValue` refuses `null` and `Array`, then renders everything else with
|
||||
`Convert.ToString(value, InvariantCulture)`. Its own remark gives the right reason for the array
|
||||
@@ -250,7 +284,20 @@ MTConnect tag is a scalar Boolean/numeric/String — hence Low.
|
||||
that happens to have it: `if (value is null or Array or not IConvertible) return false;`. `string`,
|
||||
`bool`, every numeric and `DateTime` are `IConvertible`, so the accepted set is unchanged.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution (2026-08-10):** Fixed `2d7dffcc` exactly as recommended (`Array` kept in the pattern as
|
||||
documentation of the shape this was first noticed on, though it is subsumed). Red first:
|
||||
`A_non_convertible_composite_value_is_refused_as_a_type_mismatch` wrote a `List<double>` and got
|
||||
`Good` back, having injected the list's own type name into the Agent as the observation value.
|
||||
The falsification control is the load-bearing half — `An_IConvertible_scalar_still_writes` plus
|
||||
`A_DateTime_still_writes_as_its_invariant_string` fail any over-correction expressed as "is this a
|
||||
primitive I recognise", which would start refusing real writes.
|
||||
|
||||
⚠️ **Worth knowing: the fix stranded the vehicle of an existing test.**
|
||||
`Cancellation_mid_batch_leaves_the_slots_already_written_with_their_real_outcomes` injected its
|
||||
cancellation from a value's own `ToString`, and such a value is not `IConvertible` — so it was refused
|
||||
before it was ever rendered and the cancellation never happened. The injection point moved to the
|
||||
Agent call (`CannedAgentClient.AfterWrite`), which is the seam that test is asserting around anyway;
|
||||
its determinism property is unchanged.
|
||||
|
||||
### Driver.MTConnect-007
|
||||
|
||||
@@ -259,7 +306,7 @@ that happens to have it: `if (value is null or Array or not IConvertible) return
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs:85` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `MTConnectAgentClient.Dispose()` is a plain check-then-set on a non-volatile
|
||||
`_disposed` field, so two concurrent disposals can both pass the guard. The second then calls
|
||||
@@ -272,7 +319,16 @@ Low rather than Medium.
|
||||
**Recommendation:** Guard with `Interlocked.Exchange(ref _disposed, 1) != 0` over an `int` field, or
|
||||
state on `Dispose` that it is not thread-safe and is called only under the driver's lifecycle lock.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution (2026-08-10):** Fixed `2d7dffcc` — `_disposed` is an `int` claimed with
|
||||
`Interlocked.Exchange`, the four request seams read it with `Volatile.Read`, and `Dispose` now states
|
||||
that it is idempotent under a race. Pinned by `Concurrent_disposals_are_idempotent`.
|
||||
|
||||
**Its budget is part of the fix.** The window is a handful of instructions and the loser only throws
|
||||
if it reaches `Cancel()` after the winner has finished disposing two `HttpClient`s, so a small
|
||||
race proves nothing: four racers over 200 rounds passed against the DEFECTIVE code. Eight racers on a
|
||||
barrier over 3000 rounds failed at round 690, which is the red this resolution rests on. Once the
|
||||
claim is atomic no schedule can fail it, so the test cannot flake in either direction — but do not
|
||||
shrink the budget.
|
||||
|
||||
### Driver.MTConnect-008
|
||||
|
||||
|
||||
Reference in New Issue
Block a user