From 60e5b402288fba6c53bb8fbae2aa753d4b0e0731 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:40:15 -0400 Subject: [PATCH] fix(calculation): read cache carries real input status + timestamps --- .../plans/R3-02-stale-good-family-plan.md | 12 ++++ ...R3-02-stale-good-family-plan.md.tasks.json | 2 +- docs/drivers/Calculation.md | 37 +++++++++- .../CalculationDriver.cs | 8 +-- .../CalculationEvaluator.cs | 60 +++++++++++----- .../CalculationEvaluatorTests.cs | 69 +++++++++++++++++++ 6 files changed, 163 insertions(+), 25 deletions(-) diff --git a/archreview/plans/R3-02-stale-good-family-plan.md b/archreview/plans/R3-02-stale-good-family-plan.md index 4c973b2e..a5e2c0cb 100644 --- a/archreview/plans/R3-02-stale-good-family-plan.md +++ b/archreview/plans/R3-02-stale-good-family-plan.md @@ -758,3 +758,15 @@ test could only pass once the production change landed. - `TagState.LastPublishedBad` (two-state) became `HasPublished` + `LastPublishedStatus` (uint), which is what lets a Bad→different-Bad transition republish and what makes `ReadAsync` agree with the subscriber. The evaluator is still handed bare values here — T14 hands it the snapshots. + +### T14 — `CalculationEvaluator` snapshot overload + +- RED: `--filter "…CalculationEvaluatorTests"` ⇒ **build failure**, `CS1503: cannot convert from + 'Dictionary' to 'IReadOnlyDictionary'` (×3) — no snapshot + overload existed. +- GREEN: `Driver.Calculation.Tests` 64/64. +- `BuildReadCache` is **deleted**, not fixed: the snapshots now pass straight through as the read cache. + The value-only overload survives (tests use it) but is now the one that fabricates Good, and its + xmldoc says so and points at the snapshot overload. +- Docs: `docs/drivers/Calculation.md` gains an "Input quality" section (the policy table + the + behaviour change for null-tolerant scripts) and the dedup bullet now says **(value, status)**. diff --git a/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json b/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json index 4d4a5ce4..0d0ef866 100644 --- a/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json +++ b/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json @@ -102,7 +102,7 @@ { "id": "T14", "subject": "CalculationEvaluator snapshot overload: read cache carries real input status + timestamps; passthrough fast path on snapshots", - "status": "pending", + "status": "done", "blockedBy": [ "T13" ] diff --git a/docs/drivers/Calculation.md b/docs/drivers/Calculation.md index f198f6d4..e808467a 100644 --- a/docs/drivers/Calculation.md +++ b/docs/drivers/Calculation.md @@ -81,10 +81,43 @@ Two deploy-time gates run over the result: - **Nothing publishes until every declared dependency has arrived at least once.** Before that a read returns `BadWaitingForInitialData`. This is the same gate `VirtualTagActor` uses, and it exists so a calculation cannot publish a confident-looking value derived from defaults. -- **Equal results are deduped** — an evaluation producing the same value as last time does not - re-publish. +- **Equal results are deduped on (value, *status*)** — an evaluation producing the same value *and* + the same status as last time does not re-publish. The status half is load-bearing: a device that + comes back after an outage usually comes back holding the reading it had before, so a value-only + dedup would swallow the recovery and leave the node Bad forever. - **Timers are grouped by interval**, one timer per distinct interval, not one per tag. +## Input quality + +The contract is stated once, in +[`docs/DriverLifecycle.md` §"The quality contract"](../DriverLifecycle.md#the-quality-contract): a +value consumer never reports a quality better than its source's. A calc tag's inputs are other tags' +live values, so its output inherits their quality: + +| Worst input status | What the driver does | What the calc tag publishes | +|---|---|---| +| Good | evaluates | the computed value, `Good` | +| Uncertain (any sub-code) | **evaluates** — an Uncertain value is present and usable | the computed value, stamped with that Uncertain sub-code | +| Bad (any sub-code) | **holds** — the script does not run | the **last known value**, stamped with that input's Bad sub-code, once per change of status | +| a dependency never pushed | holds (the pre-existing warm-up gate) | nothing — the node stays `BadWaitingForInitialData` | + +Notes worth knowing before authoring against this: + +- **The sub-code propagates**, so the output says *why*: `BadNoCommunication` means go look at the + device feeding the input; `BadInternalError` means this tag's own script failed. +- **Holding on Bad is what removes the NRE storm.** A comms-lost input used to arrive as *Good, null*, + so `(double)ctx.GetTag("x").Value` threw on every trigger and filled `/script-log` with identical + "script threw" entries. Now the script simply is not run, and **no script-log entry is written** — an + input dying is not a script fault. +- ⚠️ **A script that relied on receiving `null` for a dead input no longer runs at all** while that + input is Bad. Rewrite such a script to assume its inputs are usable; the driver has already decided + they are. +- **`ctx.GetTag(path).StatusCode` and `.SourceTimestampUtc` are real.** The read cache used to stamp + every input `Good` at evaluation time; it now passes the input's own snapshot through. Since Bad + inputs never reach a script, what a script can observe is Good or Uncertain. +- **`ReadAsync` reports the same status the subscription carried**, so a polled read and a subscribed + client never disagree about the same node. + ## Error semantics An evaluator failure publishes **Bad quality carrying the last-known value** — the value is not diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs index a9704340..66e5b663 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs @@ -547,11 +547,9 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba return; } - // Projected to bare values for now — the evaluator's own read cache still fabricates Good, which - // the next task fixes by handing it the snapshots. Only reachable with USABLE inputs (the Bad - // hold above returned), so nothing Bad is being flattened here. - var depValues = pending.Dependencies.ToDictionary(kv => kv.Key, kv => kv.Value.Value, StringComparer.Ordinal); - var result = _evaluator.Evaluate(def.RawPath, def.ScriptSource, depValues); + // Snapshots, not bare values: the script's ctx.GetTag(path).StatusCode / .SourceTimestampUtc are + // then the input's real ones rather than a fabricated Good stamped at evaluation time (05/STAB-19). + var result = _evaluator.Evaluate(def.RawPath, def.ScriptSource, pending.Dependencies); string? failureReason = null; (object? Value, uint StatusCode)? toPublish; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs index b2077008..ad39bc4a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs @@ -75,7 +75,44 @@ public sealed class CalculationEvaluator : IScriptCacheOwner, IDisposable /// Identity of the calc tag (bound to the script log; in the live path equals the script id). /// The C# script source producing the tag's value. /// Read-only snapshot of dependency values keyed by RawPath. + /// + /// Value-only overload. Every dependency reaches the script stamped Good with an + /// evaluation-time timestamp, because bare values carry nothing better. Prefer + /// , which the + /// driver uses: it carries each input's real status and source timestamp, so + /// ctx.GetTag(path).StatusCode is honest (archreview 05/STAB-19). This overload remains for + /// callers that genuinely hold only values — chiefly tests. + /// public VirtualTagEvalResult Evaluate(string calcTagId, string scriptSource, IReadOnlyDictionary dependencies) + { + ArgumentNullException.ThrowIfNull(dependencies); + var nowUtc = DateTime.UtcNow; + var snapshots = new Dictionary(dependencies.Count, StringComparer.Ordinal); + foreach (var kv in dependencies) + snapshots[kv.Key] = new DataValueSnapshot(kv.Value, StatusCode: 0u, nowUtc, nowUtc); + + return Evaluate(calcTagId, scriptSource, snapshots); + } + + /// + /// Evaluate for the calc tag against + /// the dependency snapshots in — value, status code and + /// source timestamp per input. Never throws; script faults surface as + /// . + /// + /// + /// The snapshots pass through to the script's read cache rather than being re-wrapped as + /// Good (archreview 05/STAB-19). The old wrapping meant a script that inspected + /// ctx.GetTag(path).StatusCode was told Good for every input, including one whose driver had + /// stopped communicating — so a quality-aware script could not be written at all. The driver holds + /// evaluation entirely while any input is Bad, so what a script sees here is Good or Uncertain. + /// + /// Identity of the calc tag (bound to the script log). + /// The C# script source producing the tag's value. + /// Read-only snapshot of dependency values keyed by RawPath. + /// The evaluation outcome. + public VirtualTagEvalResult Evaluate( + string calcTagId, string scriptSource, IReadOnlyDictionary dependencies) { if (_disposed) return VirtualTagEvalResult.Failure("evaluator disposed"); if (string.IsNullOrWhiteSpace(scriptSource)) return VirtualTagEvalResult.Failure("empty expression"); @@ -86,12 +123,15 @@ public sealed class CalculationEvaluator : IScriptCacheOwner, IDisposable // returns null — Ok(null) here too. Near-misses fall through to the compiled path. if (PassthroughScript.TryMatch(scriptSource, out var passthroughRef)) { - return dependencies.TryGetValue(passthroughRef, out var ptValue) - ? VirtualTagEvalResult.Ok(ptValue) + // The VALUE only — the caller stamps the output with the worst input status, which for a + // single-dependency passthrough IS this input's status. Returning it here as well would put + // the same decision in two places. + return dependencies.TryGetValue(passthroughRef, out var ptSnapshot) + ? VirtualTagEvalResult.Ok(ptSnapshot.Value) : VirtualTagEvalResult.Ok(null); } - var readCache = BuildReadCache(dependencies); + var readCache = dependencies; // Per-evaluation script logger: bind both ScriptId and CalcTagId (via the VirtualTagId slot, which // the Script-log page attributes on) from the calc-tag id so each line is attributable. var scriptLog = _scriptRoot @@ -173,20 +213,6 @@ public sealed class CalculationEvaluator : IScriptCacheOwner, IDisposable /// public void ClearCompiledScripts() => _cache.Clear(); - private static IReadOnlyDictionary BuildReadCache( - IReadOnlyDictionary deps) - { - // VirtualTagContext.GetTag returns a DataValueSnapshot — wrap each raw dep value as Good-quality - // so the script's `(int)ctx.GetTag("a").Value` pattern works. Null values stay null. - var nowUtc = DateTime.UtcNow; - var cache = new Dictionary(StringComparer.Ordinal); - foreach (var kv in deps) - { - cache[kv.Key] = new DataValueSnapshot(kv.Value, StatusCode: 0u, SourceTimestampUtc: nowUtc, ServerTimestampUtc: nowUtc); - } - return cache; - } - /// Disposes the evaluator, draining + disposing every cached compile's collectible ALC. public void Dispose() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationEvaluatorTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationEvaluatorTests.cs index 3893e2bc..7669aa05 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationEvaluatorTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationEvaluatorTests.cs @@ -4,6 +4,7 @@ using Microsoft.Extensions.Logging.Abstractions; using Serilog; using Shouldly; using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Scripting; namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests; @@ -230,6 +231,74 @@ public sealed class CalculationEvaluatorTests return (int)countProp.GetValue(cache)!; } + // ── input quality reaches the script (archreview 05/STAB-19) ──────────────────────────── + + /// + /// A script reading ctx.GetTag(path).StatusCode sees the input's real status. + /// The read cache used to stamp every dependency Good, so a script that checked quality was lied + /// to — and, since the driver now holds on Bad, an Uncertain input is precisely the case where a + /// script can still make a qualified decision and needs the truth to do it. + /// + [Fact] + public void The_read_cache_carries_the_inputs_real_status_code() + { + const uint uncertain = 0x40000000u; + using var sut = new CalculationEvaluator(NullLogger.Instance, NoOpScriptRoot()); + var sourceStamp = new DateTime(2026, 8, 20, 9, 0, 0, DateTimeKind.Utc); + + var result = sut.Evaluate( + calcTagId: "calc-quality", + scriptSource: "return (long)ctx.GetTag(\"a\").StatusCode;", + dependencies: new Dictionary + { + ["a"] = new(7.0, uncertain, sourceStamp, sourceStamp), + }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe((long)uncertain); + } + + /// + /// The dependency's own source timestamp reaches the script too. The old cache stamped + /// evaluation time on every input, so a script comparing input ages compared its own clock with + /// itself. + /// + [Fact] + public void The_read_cache_carries_the_inputs_real_source_timestamp() + { + using var sut = new CalculationEvaluator(NullLogger.Instance, NoOpScriptRoot()); + var sourceStamp = new DateTime(2026, 8, 20, 9, 0, 0, DateTimeKind.Utc); + + var result = sut.Evaluate( + calcTagId: "calc-stamp", + scriptSource: "return ctx.GetTag(\"a\").SourceTimestampUtc;", + dependencies: new Dictionary + { + ["a"] = new(7.0, 0u, sourceStamp, DateTime.UtcNow), + }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe(sourceStamp); + } + + /// The passthrough fast path takes the same snapshot input and returns its value. + [Fact] + public void The_passthrough_fast_path_accepts_snapshots() + { + using var sut = new CalculationEvaluator(NullLogger.Instance, NoOpScriptRoot()); + + var result = sut.Evaluate( + calcTagId: "calc-mirror", + scriptSource: "return ctx.GetTag(\"a\").Value;", + dependencies: new Dictionary + { + ["a"] = new(42, 0u, DateTime.UtcNow, DateTime.UtcNow), + }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe(42); + } + /// Minimal that records emitted entries so the SetVirtualTag-drop /// test can assert the Debug "dropped" line was logged. private sealed class CapturingLogger : ILogger