Joseph Doherty
a836e0d983
Code review of Driver.Calculation found two real defects, both TDD-fixed:
- CalculationDriver's ctor tried to reuse its ILogger<CalculationDriver> as the
evaluator's ILogger<CalculationEvaluator> via an `is` check that can never
succeed (unrelated closed generics), so CalculationEvaluator always ran with
a NullLogger regardless of what the driver was constructed with. Fixed by
having CalculationEvaluator accept the non-generic ILogger instead.
- EvaluateTag called the evaluator BEFORE checking whether every declared
dependency had arrived, so a multi-dependency change-triggered calc tag
evaluated its script against a partial snapshot on every warm-up tick.
VirtualTagContext.GetTag returns a Bad snapshot (.Value == null) for a
missing dep, so the idiomatic `(double)ctx.GetTag("x").Value` cast throws
NullReferenceException — caught and logged as a spurious "script threw"
script-log Warning on ordinary deploy/restart warm-up, for the standard
multi-input calc-tag shape. Fixed by gating evaluation itself on
"all deps arrived", not just the publish.
See code-reviews/Driver.Calculation/findings.md for full detail (5 findings:
001-004 Resolved, 005 Open/cross-module).
16 KiB
Code Review — Driver.Calculation
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation |
| Reviewer | Claude (automated review agent) |
| Review date | 2026-08-10 |
| Commit reviewed | b990adae |
| Status | Reviewed |
| Open findings | 1 |
Checklist coverage
| # | Category | Result |
|---|---|---|
| 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) |
| 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. |
| 7 | Design-document adherence | No issues found. The class-level XML docs cite the "mini-design" sections directly and the implementation matches them (change-gate, once-per-transition Bad, force-published recovery, GetHealth() per Gitea #517, ISkippedTagReporter per Gitea #541). |
| 8 | Code organization & conventions | 1 finding (Driver.Calculation-003, also an organization smell: three near-duplicate JsonSerializerOptions definitions across the module). Otherwise clean: one construction routine (RebuildTagTable) shared by the ctor and ReinitializeAsync so the two paths cannot drift. |
| 9 | Testing coverage | 1 finding (Driver.Calculation-004), Resolved (tests added). CalculationTimerScheduler.SkippedTickCount remains untested — see note under -004. |
| 10 | Documentation & comments | No issues found. XML docs are extensive, accurate, and were kept in sync with the fixes below. |
Findings
Driver.Calculation-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:103-106 (pre-fix), CalculationEvaluator.cs:47,56-59 (pre-fix) |
| Status | Resolved |
Description: CalculationDriver's constructor built the evaluator's logger as
_logger is ILogger<CalculationEvaluator> el ? el : NullLogger<CalculationEvaluator>.Instance, where
_logger is typed ILogger<CalculationDriver>. ILogger<CalculationDriver> and
ILogger<CalculationEvaluator> are unrelated closed generic interfaces (no inheritance relationship
between CalculationDriver and CalculationEvaluator), so the concrete logger instance the host injects
(a Logger<CalculationDriver> from ILoggerFactory.CreateLogger<CalculationDriver>(), or
NullLogger<CalculationDriver>.Instance) never actually implements ILogger<CalculationEvaluator>. The
is check was therefore always false, and every CalculationEvaluator silently ran with
NullLogger<CalculationEvaluator>.Instance regardless of what logger the driver itself was constructed
with — swallowing the evaluator's host-side diagnostics (Roslyn compile failures, sandbox violations,
unexpected execution throws, the single-tag SetVirtualTag-drop Debug line) with no way to route them to
the real host log sink. (The actual script-failure reason still reached the operator via the
script-logs DPS topic through CalculationDriver.PublishScriptLog, so this was a silent loss of
redundant host-side diagnostics rather than a loss of the primary operator signal — hence Medium, not
High.)
Recommendation: Have CalculationEvaluator accept the non-generic ILogger (every ILogger<T>
already implements it) instead of the closed generic ILogger<CalculationEvaluator>, and pass the
driver's own _logger straight through.
Resolution: Fixed 2026-08-10 (branch review/calculation-session). Changed
CalculationEvaluator's constructor parameter/field from ILogger<CalculationEvaluator> to ILogger,
and CalculationDriver's constructor now does _evaluator = new CalculationEvaluator(_logger, _scriptRoot, _options.RunTimeout); with no type gymnastics. Regression test
CalculationDriverTests.Evaluator_compile_diagnostics_flow_through_to_the_drivers_own_logger (a
capturing ILogger<CalculationDriver> passed into the driver, a deliberately broken timer-triggered
script, asserting the capturing logger receives the "Roslyn compile failed" Warning) fails against the
pre-fix code and passes after.
Driver.Calculation-002
| Field | Value |
|---|---|
| Severity | High |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:374-410 (pre-fix EvaluateTag) |
| Status | Resolved |
Description: EvaluateTag computed the change-gate flag (allArrived) but only used it to decide
whether to publish the result — it called _evaluator.Evaluate(...) before checking allArrived,
against a dependency snapshot that only contains the deps that HAVE arrived. VirtualTagContext.GetTag
(shared with VirtualTag scripts) returns a Bad snapshot with .Value == null for a key not present in
the read cache. The idiomatic calc-tag script shape used throughout this module's own tests and comments —
return (double)ctx.GetTag("src/a").Value + (double)ctx.GetTag("src/b").Value; — casts that null to a
non-nullable value type, which throws NullReferenceException at the unboxing conversion. That exception
was caught by CalculationEvaluator.Evaluate's generic catch (Exception ex) and reported as a
Failure("script threw: Object reference not set to an instance of an object."), which
OnEvaluationFailed then unconditionally logged to the script-logs topic as a Warning — on every
partial-dependency arrival, for any multi-dependency change-triggered calc tag whose declared deps
don't all arrive on the same tick. This is not a corner case: it fires on ordinary warm-up (every deploy
and every driver restart) for the standard multi-input calc-tag shape, producing a false-positive
"script threw" warning that looks exactly like a real script bug even though nothing is wrong, plus a
wasted Roslyn/compiled-cache invocation per partial arrival.
Recommendation: Move the allArrived check before the call to _evaluator.Evaluate so the script
never runs against a snapshot it wasn't declared against, matching the class's own documented intent
("publish nothing until every declared dep has arrived at least once").
Resolution: Fixed 2026-08-10 (branch review/calculation-session). EvaluateTag now returns before
building the dependency snapshot / calling the evaluator when !def.DependencyRefs.All(_arrivedDeps.Contains).
OnEvaluationFailed's now-always-true allArrived parameter was removed (the caller only reaches it once
every dep has arrived). Regression test
CalculationDriverTests.No_evaluation_or_script_log_while_a_declared_dep_is_still_missing reproduces the
exact scenario from No_publish_until_every_declared_dep_has_arrived but also asserts logs.ShouldBeEmpty();
it fails with the captured "script threw: Object reference not set..." entry against the pre-fix code
and passes after. The pre-existing No_publish_until_every_declared_dep_has_arrived and
Failure_publishes_Bad_once_per_transition_emits_script_log_and_recovers tests are unaffected (neither
exercises a multi-dep tag mid-warm-up while asserting on logs).
Driver.Calculation-003
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs / Code organization & conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs:16-21 (pre-fix) |
| Status | Resolved |
Description: The module defines its config-parsing JsonSerializerOptions in three places —
CalculationDriverFactoryExtensions.JsonOptions (spawn), CalculationDriver.ReinitJsonOptions (in-place
ApplyDelta), and CalculationDriverProbe.Opts (Test-Connect) — and the first two both set
ReadCommentHandling = JsonCommentHandling.Skip and AllowTrailingCommas = true while the probe's did
not. A config blob the real spawn/reinit path tolerates (a trailing comma or a // comment, plausible for
a hand-edited config used during troubleshooting) could therefore fail Test-Connect with "Config JSON is
invalid" even though deploying the identical JSON would succeed — the probe was stricter than the path
it exists to stand in for, the opposite of its documented contract ("Never throws; returns Ok with
negligible latency" / parse-only parity with the real path).
Recommendation: Align CalculationDriverProbe.Opts with the other two option sets (or extract a
single shared JsonSerializerOptions constant the three call sites reuse, to remove the duplication risk
entirely for future changes).
Resolution: Fixed 2026-08-10 (branch review/calculation-session). Added
ReadCommentHandling = JsonCommentHandling.Skip and AllowTrailingCommas = true to Opts, with a
comment naming the two option sets it must stay in parity with. New test file
CalculationDriverProbeTests.cs (previously nonexistent — see Driver.Calculation-004) pins this with
Tolerates_trailing_commas_like_the_real_spawn_and_reinit_paths and
Tolerates_comments_like_the_real_spawn_and_reinit_paths, both of which fail against the pre-fix Opts
and pass after.
Driver.Calculation-004
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Testing coverage |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs, CalculationDriver.cs (ISkippedTagReporter/GetSkippedTags()), CalculationTimerScheduler.cs (SkippedTickCount) |
| Status | Resolved |
Description: Two behaviours in this module had zero test coverage before this review:
CalculationDriverProbehad no test file at all — the Test-Connect surface for this driver type was completely unverified (and, per Driver.Calculation-003, was in fact wrong).CalculationDriver's implementation ofISkippedTagReporter/GetSkippedTags()(the Gitea #541 authored-vs-mapped-tag visibility feature — the exact class of bug that let a driver silently ignore 20 of 22 tags while still reporting Healthy) had no test verifying a raw tag with noscriptIdis actually reported skipped, thatAuthoredCount/SkippedCount/Sampleare correct, or that the report is rebuilt (not stale) afterReinitializeAsync.
CalculationTimerScheduler.SkippedTickCount (the in-flight-tick-skip counter) remains untested — left
open rather than force-fitted, since reliably forcing a same-group timer overlap without a sleep-based
flaky test would need either an injectable clock/scheduler or an artificially slow evaluator hook, neither
of which exists yet in this module.
Recommendation: Add a dedicated CalculationDriverProbeTests.cs; add GetSkippedTags() coverage to
CalculationDriverTests.cs. For SkippedTickCount, a follow-up could add a test-only evaluator delay hook
if the counter ever needs to be pinned by a test rather than only exercised implicitly.
Resolution: Fixed 2026-08-10 (branch review/calculation-session). Added
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverProbeTests.cs (6 tests,
including the two parity regressions from -003). Added to CalculationDriverTests.cs:
GetSkippedTags_reports_a_raw_tag_with_no_scriptId_alongside_the_authored_total,
GetSkippedTags_has_no_skips_when_every_authored_tag_maps, and
GetSkippedTags_updates_after_reinitialize. SkippedTickCount is intentionally left without a dedicated
test — noted above, not silently dropped.
Driver.Calculation-005
| Field | Value |
|---|---|
| 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 |
Description: OnDependencyValue evaluates every change-triggered dependent tag inline, under
_lock, and EvaluateTag's call into CalculationEvaluator.Evaluate ultimately runs
TimedScriptEvaluator<...>.RunAsync(context).GetAwaiter().GetResult() — a synchronous blocking wait bounded
by RunTimeout (default 2 s, configurable via RunTimeoutMs, with no enforced ceiling). OnDependencyValue
is invoked directly and synchronously from DependencyConsumerMuxAdapter.OnDependencyValueChanged
(src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs:66-70), an Akka
ReceiveActor's message handler — i.e. on the actor's dispatcher thread. A calc script that runs close to
its timeout budget therefore blocks that thread (and, transitively, that specific driver's dependency-value
mailbox) for up to RunTimeout on every dependency arrival, and — because TimedScriptEvaluator offloads
the actual run via Task.Run while the caller blocks synchronously waiting on it — occupies a pool thread
for the duration rather than yielding it, which is a known thread-pool-starvation risk under Akka's
fork-join dispatcher if enough calc drivers are evaluating slow scripts concurrently. The in-line comment
("the compile cache makes this a bounded method invocation — the same 'fast enough to run inline' contract
as the VT evaluator") is not actually enforced by anything faster than RunTimeout itself — the existence
of the timeout mechanism is evidence a script CAN run slow, not evidence it never will.
This exact pattern (_evaluator.Evaluate(...) called unconditionally, inline, from an Akka actor's
synchronous message handler, with the identical GetAwaiter().GetResult() blocking shape) is also present
in VirtualTagActor.OnDependencyChanged (src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs:179-199),
which CalculationDriver's own docs explicitly say this module mirrors — so this is a systemic,
pre-existing trade-off in the scripting engine, not something introduced by the Calculation driver alone.
Recorded here because the blocking call itself lives in this module's code (CalculationDriver.cs), even
though a full fix spans Core.Abstractions (the synchronous IDependencyConsumer.OnDependencyValue
contract) and Runtime (the adapter actor) — both out of this review's module scope.
Recommendation: Not a fix for this review (cross-module: the synchronous IDependencyConsumer contract
lives in Core.Abstractions, and the actor calling it lives in Runtime). If this risk is judged worth
closing, options include: (a) giving IDependencyConsumer.OnDependencyValue an async signature so the actor
can PipeTo the result instead of blocking its own thread; (b) having CalculationDriver dispatch evaluation
onto a dedicated bounded scheduler (e.g. Task.Run from OnDependencyValue itself, publishing the result
asynchronously) so the actor thread returns immediately; or (c) tightening RunTimeout's effective ceiling
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.)