merge: Calculation review findings resolved (Driver.Calculation-005 pump split, Driver.Calculation.Contracts-001 single-parse Inspect)

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.

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,64 @@ 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 stack, inside the callback) 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. ⚠️ The first test's original form compared the publish's **thread id**
+against the caller's and was itself flaky (1 failure in 6 runs): the pump is free to reuse the pooled thread
+the test released while awaiting, so an id match means nothing. It asserts on a thread-local flag set for
+exactly the callback window instead — which is exact, and was re-falsified by making `SchedulePump` run the
+pump synchronously (red immediately, then reverted).
+
+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.