diff --git a/code-reviews/Driver.MTConnect/findings.md b/code-reviews/Driver.MTConnect/findings.md index 25f6a9a4..c4fd0af5 100644 --- a/code-reviews/Driver.MTConnect/findings.md +++ b/code-reviews/Driver.MTConnect/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Complete | -| Open findings | 5 | +| Open findings | 1 | ## Checklist coverage @@ -161,7 +161,7 @@ modules. The stale premises this defect invalidated inside these two modules are | 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 @@ -185,8 +185,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 @@ -195,7 +219,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 @@ -216,7 +240,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 @@ -225,7 +259,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 @@ -243,7 +277,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` 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 @@ -252,7 +299,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 @@ -265,7 +312,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