diff --git a/code-reviews/Driver.Calculation.Contracts/findings.md b/code-reviews/Driver.Calculation.Contracts/findings.md index c17b915b..f5d14409 100644 --- a/code-reviews/Driver.Calculation.Contracts/findings.md +++ b/code-reviews/Driver.Calculation.Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -33,7 +33,7 @@ | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs:107-160` (`Inspect`) | -| Status | Open | +| Status | Resolved | **Description:** `Inspect` parses `reference` with `JsonDocument.Parse` once directly (to check the root kind and read `changeTriggered`/`timerIntervalMs` for the "present-but-discarded" warnings), and then — @@ -52,4 +52,24 @@ and `AdminUI`'s `CalculationTagConfigModel.cs`), so widening it safely is a smal a same-file one-liner, and the current cost is one extra JSON parse per calc tag at deploy time — not a runtime cost. -**Resolution:** _(Open — Low severity, record-only per review policy; deploy-time only, not a hot path.)_ +**Resolution (2026-08-10):** Fixed in `73230a42` — and it turned out to be a same-file change after all, +because widening the *public* signature was never necessary. The field-by-field read moved into a private +`TryReadRoot(JsonElement root, out CalculationTagConfigFields fields)`; `TryReadTagConfig(string, …)` keeps +its exact signature (still the entry point `Driver.Calculation`'s `CalculationTagDefinition.TryFromTagConfig` +and AdminUI's `CalculationTagConfigModel` consume) and now does nothing but parse, guard `JsonException`, and +delegate. `Inspect` calls `TryReadRoot(root, …)` over the root it has already parsed, so the blob is parsed +exactly once per gate pass while the scriptId rule stays owned by one authority — the "one authority" design +the finding names is preserved, not traded away. + +Equivalence is exact rather than argued: by the time `Inspect` reaches that call it has already returned early +for a blank blob, a non-`{`-prefixed blob, and a `JsonException`, and has already established +`root.ValueKind == Object` — precisely the three pre-checks `TryReadTagConfig` performs before its own +field read, over the same default-options parse of the same string. + +Pinned by a new `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs` +(17 cases: exact warning wording, the rejection-vs-advisory split and its ordering, the blank/non-object +pre-filter, and a falsification control that every advisory-drawing blob is still ACCEPTED by +`TryReadTagConfig` — the "never stricter than the runtime read" invariant). `Inspect` previously had no +coverage outside the DB-backed AdminUI parity corpus, so these were written against the **pre-refactor** +code and verified to pass there (production change stashed) before the refactor landed — a pin, not a +description of the new code. Behaviour is byte-identical; no accept/reject/advisory outcome changed. diff --git a/code-reviews/Driver.Calculation/findings.md b/code-reviews/Driver.Calculation/findings.md index 1b4c31a5..3269936f 100644 --- a/code-reviews/Driver.Calculation/findings.md +++ b/code-reviews/Driver.Calculation/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -15,7 +15,7 @@ |---|---|---| | 1 | Correctness & logic bugs | 3 findings (Driver.Calculation-001, -002, -003), all Resolved | | 2 | OtOpcUa conventions | No issues found. Galaxy/MXAccess-specific conventions do not apply to this pseudo-driver (no transport, no gateway). It correctly implements the capability-interface seams (IDriver, IDependencyConsumer, ISubscribable, IReadable, ISkippedTagReporter), .NET 10/AnyCPU, and authorization stays out of driver-level code. | -| 3 | Concurrency & thread safety | 1 finding (Driver.Calculation-005), Open (cross-module) | +| 3 | Concurrency & thread safety | 1 finding (Driver.Calculation-005), Resolved | | 4 | Error handling & resilience | Covered by Driver.Calculation-002 (script-evaluation faults never throw out of the driver; SkippedTagLedger tolerates a single bad tag without failing the whole rebuild). No further issues found. | | 5 | Security | No issues found. No secrets, no I/O, no SQL; the sandboxed Roslyn evaluator (shared Core.Scripting sandbox) is the security boundary and lives outside this module. | | 6 | Performance & resource management | Covered by Driver.Calculation-005 (blocking evaluation under a shared lock). CalculationTimerScheduler correctly bounds outstanding work to one tick per interval group (skip-if-in-flight) and CalculationEvaluator.ClearCompiledScripts() is invoked on every ReinitializeAsync, so compiled-script AssemblyLoadContexts do not accrete across script edits. No further issues found. | @@ -186,7 +186,7 @@ test — noted above, not silently dropped. | Severity | Medium | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:300-314` (`OnDependencyValue`), `:374-410` (`EvaluateTag`); cross-module: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs` | -| Status | Open | +| Status | Resolved | **Description:** `OnDependencyValue` evaluates every change-triggered dependent tag **inline, under `_lock`**, and `EvaluateTag`'s call into `CalculationEvaluator.Evaluate` ultimately runs @@ -222,4 +222,60 @@ asynchronously) so the actor thread returns immediately; or (c) tightening `RunT independent of the `RunTimeoutMs` config knob. Any of these should be scoped and reviewed together with the equivalent `VirtualTagActor` behaviour so the two mirrors do not diverge. -**Resolution:** _(Open — cross-module, not fixed as part of this review.)_ +**Resolution (2026-08-10):** Fixed in `235e06a8` — **inside this module, with no change to the +`IDependencyConsumer` contract and none to `Runtime`.** The finding scoped a fix as necessarily +cross-module (option (a), an async `OnDependencyValue`), but option (b) — the driver owning its own +dispatch — closes both halves without touching a shared interface, which is the smaller change and leaves +the mirrored `VirtualTagActor` behaviour free to be addressed separately. + +`OnDependencyValue` no longer evaluates anything. Under `_lock` it records the arrival and *prepares* each +change-triggered dependent — resolve the definition, apply the warm-up gate, snapshot that tag's declared +dependency values — into a `PendingEvaluation`, enqueues them, and returns; a single `Task.Run` **evaluation +pump** drains the queue in FIFO order. `_lock` is a short-held state lock again (dictionary work only, never +held across a script run); a new `_evalGate` carries the mutual exclusion `_lock` used to provide, and is +taken **only** by the two evaluation workers (the pump and `EvaluateTagFromTimer`), never by +`OnDependencyValue`, `ReadAsync` or `ReinitializeAsync` — which is what makes "no host thread can block on a +script" structural rather than incidental. Lock order is always `_evalGate` → `_lock`. `EvaluateTag` split +into `PrepareEvaluation` (under `_lock`) / `RunEvaluation` (script + apply, outside it) / +`MarkEvaluationSucceeded` + `MarkEvaluationFailed` (state decisions, under `_lock`, returning what to +publish); the script log and the `OnDataChange` fan-out now fire outside both locks. + +**The guarantee the fix rests on: at most one evaluation runs at a time.** The pump is single (`_pumpScheduled` +is set and cleared under `_lock`, and the flag and the queue are always checked together, so an enqueue either +starts a pump or is seen by one that has not yet exited), and `_evalGate` serializes it against timer ticks — +so per-tag *and* cross-tag evaluation ordering is exactly what it was when the change path evaluated inline +under the state lock. Publishes follow evaluation order because they are issued by the same serialized worker. + +Semantics deliberately preserved beyond ordering: the warm-up gate still runs BEFORE the evaluator +(Driver.Calculation-002 is not regressed — it moved into `PrepareEvaluation`, which is equivalent because +`_arrivedDeps` only ever grows and the arrival that completes the set queues its own evaluation); dedup, +once-per-transition Bad, and force-published recovery are unchanged. **The dependency snapshot is taken at +trigger time, not when the pump reaches the item** — without that, a burst of arrivals would collapse onto +whichever value happened to be current when the pump caught up, changing which intermediate values publish +(`Equal_result_is_deduped_and_a_changed_result_republishes` is the case that would have silently become +`[14.0]` instead of `[10.0, 14.0]`). The timer path keeps its own structure — a tick still evaluates on the +scheduler's pool thread under the scheduler's per-group in-flight guard, which is what bounds outstanding +timer work; routing ticks through the pump instead would have made `SkippedTickCount` dead and let ticks +queue without bound. `ReinitializeAsync`, `ShutdownAsync` and `Dispose` clear the queue (prepared items are +stale against a rebuilt tag table), and `RunEvaluation` re-looks-up the tag's state under `_lock` after the +run so a tag dropped mid-evaluation discards its result rather than publishing it. + +RED first, two tests, both failing against the pre-fix code: +`A_dependency_arrival_does_not_run_the_script_on_the_callers_thread` (the publish — which can only happen +after the script ran — landed on the caller's own thread id) and +`A_slow_script_blocks_neither_the_dependency_callback_nor_a_concurrent_read` (a 600 ms busy-wait script: +the callback took 1.21 s, and `ReadAsync`, which needs the same state lock, was stuck behind it). Both +carry a positive control asserting the value still computes and publishes, so neither can pass against a +driver that simply evaluates nothing. + +Known trade, documented rather than hidden: the pending queue is unbounded. Under sustained arrival +pressure the previous inline path applied backpressure by blocking the mux actor — which is the defect — +so the queue is the relief valve, and the tag-set is small and bounded by authored config. Five existing +tests that asserted on the line after `OnDependencyValue` now settle on a new internal `IsEvaluationIdle` +probe first (quiescence, not a sleep — the work is enqueued synchronously inside the callback, so idle +cannot be observed before it lands). Suites green: `Driver.Calculation.Tests` 53/53 (34 before this work), +`Runtime.Tests` 558 passed / 3 skipped. + +The mirrored `VirtualTagActor.OnDependencyChanged` blocking shape this finding also names is **unchanged** — +it lives in `Runtime`, outside this module, and is left as the separate scoped change the finding's +recommendation asks for.