diff --git a/CLAUDE.md b/CLAUDE.md index 429f3c83..d6631a52 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -297,7 +297,32 @@ non-`Healthy` state **and** its `LastSuccessfulRead` has been frozen for 90 s. T system — it is a local `GetHealth()` read, and only reaches the network once a driver has already stopped reading. -See `docs/DriverLifecycle.md` §"The comms-loss watchdog". +- **What comms loss now does downstream — one contract, six seams (archreview R3-02).** "A value consumer + never reports a quality better than its source's" is stated once in `docs/DriverLifecycle.md` §"The + quality contract" and implemented by `Core.Abstractions.InputQuality` (`WorstOf`/`IsUsable`). Three + things changed with it, all client-visible: + - ⚠️ **Value nodes of a comms-lost driver now go Bad.** `DriverHostActor.FanCommsLossToValueNodes` + stamps `BadNoCommunication` on the raw NodeId **and every referencing UNS NodeId**, **value retained**, + via the new status-only sink primitive `IOpcUaAddressSpaceSink.WriteStatuses` (the host holds no value + cache, so only the node can keep the value). #481 had wired comms loss to the mux and to Part 9 + conditions and stopped one seam short of the external data plane. A consumer that treats Bad as "drop + the tag" (ScadaBridge) will see more Bads — that is the fix working. + - **Nothing Good is written on reconnect** — recovery is the driver's own re-publish (the poll engine's + `forceRaise`, MQTT's new initial push). Instead the host **audits**: refs still latched 30 s after a + reconnect log a Warning and increment `otopcua.driver.comms_latched_refs`. Non-zero on that counter + means a driver reconnects without re-publishing. + - **Every dedup on the recovery path is keyed on (value, status), not value.** A device that comes back + usually comes back holding the same reading, so a value-only dedup swallows the recovery and the node + stays Bad forever — the v3-gate trap, now closed in the Calculation driver, the virtual-tag actor and + (via a connection-loss stale) MQTT's plain-mode suppression. **Any new consumer of driver values must + repeat this**; a regression test for it must replay the IDENTICAL value or it proves nothing. + - Consequences per seam: MQTT/Sparkplug stale on connection loss and push cached snapshots on every + (re)subscribe (`docs/drivers/Mqtt.md`); the Calculation driver and virtual tags **hold** while any + input is Bad and inherit the worst input's sub-code, evaluate through Uncertain, and hand scripts the + input's real `StatusCode`/`SourceTimestampUtc` (`docs/drivers/Calculation.md`, `docs/ScriptEditor.md`) + — so a script that relied on receiving `null` for a dead input no longer runs at all while it is Bad. + +See `docs/DriverLifecycle.md` §"The comms-loss watchdog" and §"The quality contract". ## mxaccessgw diff --git a/archreview/plans/R3-02-stale-good-family-plan.md b/archreview/plans/R3-02-stale-good-family-plan.md index 686d5020..9eca0c0f 100644 --- a/archreview/plans/R3-02-stale-good-family-plan.md +++ b/archreview/plans/R3-02-stale-good-family-plan.md @@ -580,3 +580,331 @@ T17 anytime; T18 after all; T19 last ``` Overall effort: **~2 dev-days code+unit, plus the rig session**. The three HIGH-RISK data-plane slices (T3 MQTT stale, T8 value-node Bad, T13/T16 hold-on-Bad) each land behind their full suites; the two recovery contract tests (T10, T16's identical-value case) must each be seen to FAIL once against a reverted dedup before merge — that is the whole reason this family re-appeared. + +--- + +## Execution notes + +Branch `fix/r3-02-stale-good-family` (worktree `.claude/worktrees/agent-a696a39981023fa0b`), off master +`72329eab`. Executed in plan block order T1, B (T2–T5), C (T6–T10), D (T12–T14), E (T15–T16), T17, T18. +**T11 and T19 are deliberately NOT in this branch** — T11 waits for the sibling R3-03 branch to merge +(it is the only edit that would touch `DriverInstanceActor` lifecycle alongside R3-03's restart work), +and T19 is the operator rig gate. + +Red-before observations are recorded per task below: what was run, and the failure text that proves the +test could only pass once the production change landed. + +### T1 — `InputQuality.WorstOf` + +- RED: `dotnet test tests/Core/…Core.Abstractions.Tests --filter "FullyQualifiedName~InputQualityTests"` + ⇒ **build failure**, 8 × `error CS0103: The name 'InputQuality' does not exist in the current context`. +- GREEN: 13 passed. `Core.ScriptedAlarms.Tests` 94/94 green with `WorstInputStatus` re-pointed (the + never-pushed/null skips stay in the alarm engine — they are alarm policy, not the shared rule). + +### T2 — MQTT plain-mode initial push on `SubscribeAsync` + +- RED: `--filter "FullyQualifiedName~MqttSubscriptionManagerRecoveryTests"` ⇒ **4 failed**, each on + `raised.Count` (0 observed, 1 expected) / `raised.Select(r => r.Reference)` — `SubscribeAsync` raised + nothing at all for an already-observed ref. +- GREEN: 77 `MqttSubscriptionManager*` tests pass, incl. the suppression control (an identical message + after the push is still swallowed). Full `Driver.Mqtt.Tests`: 790/790. + +### T3 — `MqttConnection.ConnectionLost` + plain-mode stale + +- RED: `--filter "…MqttSubscriptionManagerRecoveryTests|…ConnectionLost"` ⇒ **build failure**, + `CS1061: 'MqttSubscriptionManager' does not contain a definition for 'OnConnectionLost'` (×5) and + `CS1061: 'MqttConnection' does not contain a definition for 'ConnectionLost'` (×3). +- GREEN: 11/11 (8 manager recovery + 3 connection edge). Full `Driver.Mqtt.Tests`: 797/797. +- The event fires inside the existing `State == Connected` guard in `OnDisconnectedAsync`, which is + what makes "not on an initial-connect failure" and "not on dispose" true by construction rather + than by a second condition — `DisposeAsync` sets `Disposed` before tearing the client down. + +### T4 — Sparkplug ingestor: initial push + connection-loss stale + +- RED: `--filter "…SparkplugIngestorRecoveryTests"` ⇒ **build failure**, `CS1061: 'SparkplugIngestor' + does not contain a definition for 'OnConnectionLost'` (×2). +- GREEN: 6/6. Full `Driver.Mqtt.Tests`: 803/803. +- ⚠️ **Deviation from the plan's test rationale (recorded, not silently accepted):** the plan asks for + the identical-DBIRTH-replay assertion as this seam's vacuity guard. It is present, but **it is not a + falsifier here** — Sparkplug ingestion has no unchanged-value suppression at all (births/data + publish directly, never through `Deliver`), so that replay was already raised Good before the + change. That is now asserted explicitly by an added control, + `An_identical_birth_replay_is_raised_even_without_a_preceding_stale`, and the suite header says so. + The load-bearing assertion on this seam is the **stale** itself. +- The stale reports `BadNoCommunication` while a DDEATH keeps `BadCommunicationError`: the death is the + source announcing its own offline state, the stale is our transport losing the broker (and matches + what the host's S15 fan-out writes). The existing const's "no driver emits BadNoCommunication at all" + remark was corrected rather than left to rot. + +### T5 — MQTT driver-level end-to-end recovery + docs + +- The test is green on first run by construction (T2–T4 already landed), so the plan's TDD bar was met + the other way: **the wiring was deliberately broken and the test seen to fail.** Deleting + `connection.ConnectionLost += OnConnectionLost;` from `MqttSubscriptionManager.AttachTo` ⇒ + `ABrokerDrop_MarksCachedRefsBadNoCommunication_AndTheIdenticalReplayRecovers` **FAILED** with + `Timed out waiting for the broker drop to publish a BadNoCommunication snapshot` (15 s). Restored, + green. That is the point of this test: the manager-level suites drive `OnConnectionLost()` directly + and stay green against an unforwarded seam. +- The outage edge is a real socket drop through MiniBroker → MQTTnet `DisconnectedAsync`; ingest is fed + through `Subscriptions.HandleMessage` because MiniBroker answers CONNECT/PINGREQ only and cannot + PUBLISH. +- Docs: `docs/drivers/Mqtt.md` §"Quality on connection loss and re-subscribe" (links the contract in + `DriverLifecycle.md`, records the two distinct "not communicating" codes and the operator-visible + behaviour change). + +### T6 — `IOpcUaAddressSpaceSink.WriteStatuses` (interface default + node manager + both sinks) + +- RED (the guard, first): adding the member to the interface and running + `--filter "…DeferredSinkForwardingReflectionTests"` ⇒ **`Every_forwarding_interface_method_reaches_the_inner_sink` + FAILED**. Note the failure surfaced as `System.ArgumentNullException : Value cannot be null. (Parameter + 'statuses')` rather than the guard's "did NOT forward" message — the reflection harness passes `null` + for an `IReadOnlyList<>` parameter, so the *interface default* ran on the wrapper and threw. Red for + the right reason (the member was not forwarded); green once `DeferredAddressSpaceSink.WriteStatuses` + forwards. This is the memory'd "Deferred sink doesn't forward the new member" trap, and here the + default is not merely slower — it writes a **null value**, erasing the very value the annotation exists + to keep. +- RED (node manager): `--filter "…OtOpcUaNodeManagerWriteStatusesTests"` ⇒ **build failure**, `CS1061: + 'OtOpcUaNodeManager' does not contain a definition for 'WriteStatuses'` (×5). +- GREEN: 5/5 new; full `OpcUaServer.Tests` 495 passed / 1 skipped; `Commons.Tests` 348/348. +- An unmaterialised NodeId is **skipped and not counted as a failure** (Debug tally via the SDK static + trace — the manager has no `ILogger`): the caller's per-driver index racing a surgical remove is an + expected shape, not a fault. + +### T7 — `OpcUaPublishActor.AttributeStatusBatchUpdate` + +- RED: `--filter "…OpcUaPublishActorStatusBatchTests"` ⇒ **build failure**, `CS0426: The type name + 'AttributeStatusBatchUpdate' does not exist in the type 'OpcUaPublishActor'` (×3). +- GREEN: 58 `OpcUaPublishActor*` tests pass (3 new). +- Metered `OpcUaSinkWrite kind:"status"` so a status annotation is separable from a value write on the + same counter; the recording fake asserts **zero** value writes, since a handler falling through to the + value path would null every node. + +### T8 — `DriverHostActor.FanCommsLossToValueNodes` + +- RED: `--filter "…DriverHostActorCommsLossValueNodesTests"` ⇒ **2 failed**, both + `Timeout 00:00:15 while waiting for a message of type …AttributeStatusBatchUpdate` (the disconnect + produced no value-node write at all — the finding, reproduced). +- GREEN: 10 passed across the new suite + `DriverHostActorCommsLossTests` (mux half) + + `DriverHostActorBatchFanOutTests`, all unchanged. +- **Deviation (sequencing, not design):** the plan's snippet calls `TrackCommsLatchSet(...)` from inside + this method, but that helper is T9's — calling it here would not compile. The call is added in T9, + where the latch and its audit timer land together. R3-03 boundary respected: this is a new method plus + one branch in `OnDriverConnectivityChanged`; `HandleRestartDriver` and the watchdog are untouched. + +### T9 — recovery audit (`_commsLatched` + timer + `otopcua.driver.comms_latched_refs`) + +- RED: `--filter "…DriverHostActorCommsLossValueNodesTests"` ⇒ **build failure**, `CS1739: The best + overload for 'Props' does not have a parameter named 'commsRecoveryAuditDelay'`. +- GREEN: 6/6 in the suite; 20 passed across it + `DriverHostActorCommsLossTests` + + `DriverHostActorBatchFanOutTests` + `DriverHostActorPrimaryGateTests`. +- **Falsification of the control** (the plan's discipline, applied to the audit rather than to a dedup + key): deleting the prune loop in `ForwardBatchToMux` ⇒ + `A_driver_that_republishes_every_ref_trips_no_audit` **FAILED** with `Received 1 message too many. + Expected 0 messages but received 1 that matched filter [Warning when Message contains "still + BadNoCommunication"]`. Restored, green. So the "no audit fires" assertion is falsifiable and the audit + is not merely dormant. +- Meter assertions filter on a per-test **driver tag**, not on the instrument name alone (#503: a + `MeterListener` sees every measurement in the process). +- The latch counts **RawPaths**, so the raw node and its UNS mirrors report as one ref — the operator is + told how many tags did not come back, not how many nodes. + +### T10 — S15 end-to-end recovery contract + silent-resubscribe control + +- RED: `--filter "…DriverHostCommsLossRecoveryTests"` ⇒ **2 failed**, `Timeout 00:00:15 while waiting for + a message of type …AttributeValueBatchUpdate` (before `AttributeStatusBatchUpdate` even existed in the + first draft, and again while the fake's instance id disagreed — see below). +- GREEN: 2/2 (33 s wall clock — see the reconnect-interval note). `Runtime.Tests` `~Drivers` filter: + 333 passed / 3 skipped. +- **Two harness facts worth keeping** (both cost a red run to find): + - `DriverInstanceActor` takes its instance id from **`driver.DriverInstanceId`**, not from the spec the + host spawned it with. A fake returning a different id publishes under a key the host's ref map does + not contain, and every value is silently dropped as unmapped. The fake now takes the id. + - The fake's push-on-subscribe fires on the FIRST subscribe too, so the pre-outage value must come from + that push rather than from a separate seeding call — otherwise the probe holds an extra batch. +- **Deviation:** the plan sketched driving the outage with the #527 watchdog (`commsLossTimeout` 200 ms). + The host spawns its children with the **default** `reconnectInterval`/`healthPollInterval` and exposes + no seam for either, so the outage is driven by the production `ReconnectDriver` (operator Force + Reconnect) command instead — same `Reconnecting` entry, same `ConnectivityChanged(false)`, and it does + not touch `HandleRestartDriver` (R3-03's). The watchdog's own trip condition stays covered by + `DriverInstanceActorCommsLossWatchdogTests`. The 10 s `DefaultReconnectInterval` is why these two tests + take ~15 s each. +- The identical-value rule is honoured: the driver pushes **5.0 Good** before and after the outage, so the + recovery assertion cannot be satisfied by a value change. The control (`InitialPush = null`) asserts + `ExpectNoMsg` — no Good ever arrives — and that the audit fires with exactly 1 latched ref. + +### T12 — `DependencyConsumerMuxAdapter` forwards the real status code + +- RED: `--filter "…DependencyConsumerMuxAdapterTests"` ⇒ **2 failed**, both + `Shouldly.ShouldAssertException : consumer.Received[…].StatusCode should be 2150694912u` (0x80310000) + — the consumer received `0` (Good) for a `BadNoCommunication` input, on both the single and the batch + path. That is the finding, reproduced at the seam. +- GREEN: 11 passed (`DependencyConsumerMuxAdapterTests` + `DependencyMuxActorTests`). +- The false "quality is stripped at the mux boundary" comment is deleted and replaced with why it was + false (theme 6: the contract moved at #478/#519 and the comment outlived the fact). + +### T13 — `CalculationDriver` input quality (hold on Bad, propagate worst, status-keyed dedup) + +- RED: `--filter "…CalculationDriverInputQualityTests"` ⇒ **5 failed**: `Snapshot.StatusCode should be + 2150694912u` (BadNoCommunication) on the Bad-input, cold-start and passthrough cases; `should be + 1073741824u` (Uncertain) on the Uncertain case; `published.Count should be 1` on the sub-code-change + case. That is the laundering, reproduced from three directions. +- GREEN: whole `Driver.Calculation.Tests` project 61/61 — the 9 new plus every pre-existing case + unchanged (the Good-only paths are byte-identical). +- **Vacuity check performed** (the plan's requirement): reverting the dedup key to value-only + (`if (st.HasValue && Equals(st.LastValue, value)) return null;`) ⇒ + `An_identical_value_is_republished_when_recovering_from_a_bad_input` **FAILED** with + `published.Count should be 1` — i.e. the recovery was swallowed, exactly the "Bad forever" trap. + Restored, green. +- `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)**. + +### T15 — `IVirtualTagEvaluator` snapshot overload + Roslyn read-cache pass-through + +- RED: `--filter "…RoslynVirtualTagEvaluatorTests"` ⇒ **build failure**, `CS1503: cannot convert from + 'Dictionary' to 'IReadOnlyDictionary'` (×3). +- GREEN: 24/24 in that suite; full solution build clean (0 errors), so **all five existing + `IVirtualTagEvaluator` fakes compile untouched** — which is the point of default-implementing the + overload. A test pins that fallback explicitly (`ValueOnlyEvaluator`). +- `RoslynVirtualTagEvaluator.BuildReadCache` is **deleted**; the snapshots are the read cache. The + value-only overload now projects up into the snapshot one, so there is a single evaluation body. + +### T16 — `VirtualTagActor` honours input quality + +- RED: `--filter "…VirtualTagActorInputQualityTests"` ⇒ **7 failed**, incl. `bad.StatusCode should be + 2150694912u`, `result.StatusCode should be 1073741824u` (Uncertain), `reassert.StatusCode should be + 2150694912u`, and `Expected no messages … instead we received EvaluationResult { … StatusCode = 0 }` + for the cold-start case — i.e. a Bad input produced a fresh Good result. +- GREEN: 58 passed across `VirtualTag*` (the 7 new + the R2-03 transition suites + the host bridge's + new historize-parity case), all pre-existing cases unchanged. +- **Vacuity check performed** (the plan requires this one explicitly): reverting the dedup key to + value-only ⇒ `An_identical_value_is_republished_when_recovering_from_a_bad_input` **FAILED** with + `Timeout 00:00:15 while waiting for a message of type …EvaluationResult` — the recovery was swallowed + entirely. Restored, green. +- ⚠️ **Deviation from the plan's design, deliberate and load-bearing.** The plan says the worst-of-input + walk should treat a **never-pushed** dependency as `BadWaitingForInitialData` and hold. Implemented + literally, that HOLDS evaluation during ordinary warm-up — but this actor (unlike the Calculation + driver) has always evaluated on partial arrival and published the partial result, and two existing + tests assert exactly that (`VirtualTagActorTests.Evaluator_result_flows_to_parent_as_EvaluationResult` + and `DependencyMuxActorTests.VirtualTagActor_PreStart_registers_deps_…_end_to_end`, which both went red + under the literal reading). The worst-of walk therefore covers **arrived** deps only — the same skip + `ScriptedAlarmEngine.WorstInputStatus` has always made — and cold start stays governed by R2-03's + existing `AllDependenciesArrived` gate on the publish tails. The plan's own risk note ("every Good-path + assertion in the existing suites stays byte-identical") is what this preserves. +- `_lastPublishedBad` became `_hasPublished` + `_lastPublishedStatus`, which is also what makes + `ReassertValue` re-emit `BadNoCommunication` after a deploy instead of guessing Good. +- Docs: `docs/ScriptEditor.md` §"Input quality: what a script sees, and when it runs at all". + +### T17 — Docs: the contract stated once + +- `docs/DriverLifecycle.md` gains §"The quality contract" (the contract quoted verbatim + a table of the + six seams that implement it + the consumer-visible warning) and a new sub-section under the comms-loss + fan-out covering the value-node fan, `WriteStatuses`, and the recovery audit. +- Pointers added, each linking back rather than restating: `docs/Raw.md`, `docs/Uns.md`, + `docs/ScriptEditor.md` (T16), `docs/drivers/Calculation.md` (T14), `docs/drivers/Mqtt.md` (T5). +- `CLAUDE.md`: one bullet under the comms-loss watchdog section — value nodes go Bad, nothing Good on + reconnect (+ the audit meter), and the **(value, status)** dedup rule any future consumer must repeat. + +### Merge of master (R3-03 + R3-05 + R3-01) — before T11 + +`git merge master` at `d79d51dd`. Two conflicts, both **additive-vs-additive** on parameters R3-03 and +this plan each appended last: + +- `DriverHostActor` — `reprovisionBackoff` (R3-03) and `commsRecoveryAuditDelay` (T9) both claimed the + final ctor/Props slot, plus adjacent field blocks. Resolved by keeping BOTH in master-then-mine order + across all three positional sites (Props signature, `Props.Create` call, ctor), per the file's own + "new parameters go LAST in all three places" warning. +- `V3ArtifactSeeder` — R3-03 had independently added the same `enabled` + `driverFactory` seams T10 + needed (`driversEnabled`). **Master's naming won**; mine was deleted and the T10 call site moved to + `enabled: true` + named `commsRecoveryAuditDelay:`. `Spawn`/`SpawnHostAndApply` carry all three + optional knobs. +- One non-conflicting mis-merge caught by eye: git interleaved the new + `DriverCommsLatchedRefs` counter *between* `PrimaryGateDenied`'s xmldoc and its declaration in + `OtOpcUaTelemetry`. Re-ordered. +- Post-merge: solution builds clean, `Runtime.Tests` **670 passed / 3 skipped**. + +### T11 — `DriverInstanceActor.PostStop` announces comms loss + +- RED: `--filter "…Stopping_a_connected_child_announces…"` ⇒ **FAILED**, `Expected a message of type + …ConnectivityChanged, but received {…}` — stopping announced nothing at all. +- GREEN: `DriverInstanceActor*` 67/67; full `Runtime.Tests` **672 passed / 3 skipped**. +- **Deviation (narrowing, deliberate):** the Tell is **gated on the child having reached `Connected`** + (`_connectedSinceUtc != default`, the field R3-03 added for the watchdog grace) rather than + unconditional as the plan sketched. A never-connected child's nodes are still + `BadWaitingForInitialData`, which is more precise than "communication lost"; overwriting it would claim + an outage that never happened. Pinned by a control test + (`Stopping_a_never_connected_child_announces_nothing`). +- With R3-03 merged the recovery leg holds: `PostStop` → host latches Bad → R3-03's `SpawnChild` always + sends the desired set → the fresh child subscribes → the driver's initial push clears the latch. + +### T18 — full-suite sweep + bookkeeping + +- **Full-solution unit sweep green** (`dotnet test ZB.MOM.WW.OtOpcUa.slnx`): every project `Failed: 0`. + Notable per-project counts after this branch: `Commons.Tests` 348, `OpcUaServer.Tests` 498/1 skipped, + `Driver.Mqtt.Tests` 803, `Driver.Calculation.Tests` 64, `Core.ScriptedAlarms.Tests` 94, + `Core.Abstractions.Tests` (incl. the new `InputQualityTests`) green, `Runtime.Tests` 672/3 skipped. + Docker-gated integration suites skip cleanly as usual (Modbus 34 skipped, S7 3, AbCip 3, Galaxy 8, + Historian.Gateway 6). +- **`Runtime.Tests` × 5** for the #500 flake profile: **four runs 672 passed / 3 skipped**; one run + reported **1 failure** whose test name was lost to an output filter on that invocation. Not reproduced + in four subsequent full runs, and every suite this plan touched was run green individually many times. + Recorded rather than hidden — if it recurs, capture it with + `dotnet test … --logger "console;verbosity=normal"` and check it against the #500 list. +- Bookkeeping: `archreview/plans/STATUS.md` gains an R3-02 section (what landed per task, the + client-visible behaviour changes, the suite state, and the fact that **T19 the live gate is not run**). +- **Deviation:** the plan also asks for edits to "the four source reports' prior-finding tables". Those + tables are the round-3 review record written at `12b0035a`, and the sibling round-3 branches (R3-01, + R3-03, R3-04) likewise left them untouched, tracking progress in STATUS.md + the plan register instead. + Followed that precedent rather than diverging mid-round. + +--- + +## Outstanding + +- **T19 — the live gate (merge-blocking).** Not run: it needs the docker-dev rig plus the shared Modbus + fixture, and it is the only evidence that the identical-value recovery works end to end on a real + device. Recipe unchanged in §Live gate; the constant HR[5] vs auto-incrementing HR[100] pairing is what + makes it non-vacuous. + +--- + +## Review outcome (verdict MERGE, two Low items applied) + +Both were **real** — each is a place where a polled/observed surface disagreed with what the contract +says, i.e. the same class of bug this plan exists to close, one hop further out. Applied TDD, one commit. + +1. **`CalculationDriver.ReadOne` guarded on `!st.HasValue`** — so a tag whose only publish was an + inputs-Bad report (no value of its own) read back `BadWaitingForInitialData` while its subscriber had + been told `BadNoCommunication`. The guard is now `!st.HasPublished`, which is the state that actually + means "nothing has ever been said about this tag". + - RED: new `ReadAsync_after_an_inputs_Bad_with_no_prior_value_reports_the_bad_status` ⇒ + `Shouldly.ShouldAssertException : read[0].StatusCode should be 2150694912u`. + - The pre-existing `An_unevaluated_tag_still_reads_BadWaitingForInitialData` is the control for the + other side of the same guard and stayed green. +2. **`VirtualTagActor.OnInputsBad` metered `VirtualTagEval{outcome=inputs-bad}` above the + `AllDependenciesArrived()` return** — so ordinary cold start produced an "inputs bad" measurement per + trigger. The `Add` moved below **both** guards (readiness and transition-once), so the counter now + measures tags that actually report. + - RED: new `The_inputs_bad_counter_does_not_tick_during_warm_up` ⇒ + `Shouldly.ShouldAssertException : meter.Count("inputs-bad") should be 0`. + - The absence half carries a **positive control in the same test** (completing the dep set ⇒ exactly + one measurement), so it cannot pass for want of a working `MeterListener`. + +**Suites after the fixes:** `Driver.Calculation.Tests` **65/65**; `Runtime.Tests` `~VirtualTag` +**59/59**; `Core.ScriptedAlarms.Tests` **94/94**. + +**STATUS.md corrected:** the anonymous Runtime.Tests flake is +`DriverHostActorNativeAlarmAckRoutingTests.RouteNativeAlarmAck_routes_to_driver_AcknowledgeAsync_with_principal` +— untouched by this branch, known #500 profile, 3× green in isolation per the reviewer. 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 20d2dce8..60e9a6c5 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 @@ -5,19 +5,19 @@ { "id": "T1", "subject": "InputQuality.WorstOf shared helper (Core.Abstractions) + truth-table tests; re-point ScriptedAlarmEngine.WorstInputStatus", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T2", "subject": "MQTT plain-mode initial push of cached snapshots on SubscribeAsync — push-model forceRaise (05/STAB-18 shape b)", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T3", "subject": "MqttConnection.ConnectionLost event + plain-mode stale observed refs to BadNoCommunication (value retained) so the identical replay recovers (05/STAB-18 shape a)", - "status": "pending", + "status": "done", "blockedBy": [ "T2" ] @@ -25,7 +25,7 @@ { "id": "T4", "subject": "SparkplugIngestor: initial push on subscribe + connection-loss stale; rewrite the 'values are deliberately not staled' remark", - "status": "pending", + "status": "done", "blockedBy": [ "T3" ] @@ -33,7 +33,7 @@ { "id": "T5", "subject": "MQTT driver-level end-to-end recovery test (broker drop -> Bad -> identical retained replay -> Good) + docs/drivers/Mqtt.md section", - "status": "pending", + "status": "done", "blockedBy": [ "T4" ] @@ -41,13 +41,13 @@ { "id": "T6", "subject": "IOpcUaAddressSpaceSink.WriteStatuses (StatusWrite, default impl) + OtOpcUaNodeManager status-only batch retaining value + SdkAddressSpaceSink/DeferredAddressSpaceSink forwards (03/S15)", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T7", "subject": "OpcUaPublishActor.AttributeStatusBatchUpdate handler -> sink.WriteStatuses (metered, partial-failure Warning)", - "status": "pending", + "status": "done", "blockedBy": [ "T6" ] @@ -55,7 +55,7 @@ { "id": "T8", "subject": "DriverHostActor.FanCommsLossToValueNodes: one BadNoCommunication status batch over raw + UNS nodes on ConnectivityChanged(false); nothing on reconnect (new method — R3-03 boundary)", - "status": "pending", + "status": "done", "blockedBy": [ "T7" ] @@ -63,7 +63,7 @@ { "id": "T9", "subject": "Recovery audit: _commsLatched per driver, pruned by routed values, CommsRecoveryAudit timer + otopcua.driver.comms_latched_refs meter (dedup-trap tripwire)", - "status": "pending", + "status": "done", "blockedBy": [ "T8" ] @@ -71,7 +71,7 @@ { "id": "T10", "subject": "S15 end-to-end recovery contract test: real DriverInstanceActor + scripted fake with IDENTICAL-value initial push; silent-resubscribe falsification control", - "status": "pending", + "status": "done", "blockedBy": [ "T9" ] @@ -79,7 +79,7 @@ { "id": "T11", "subject": "DriverInstanceActor.PostStop Tells ConnectivityChanged(false) so operator restart / respawn latches nodes Bad (land after R3-03 merges)", - "status": "pending", + "status": "done", "blockedBy": [ "T8" ] @@ -87,13 +87,13 @@ { "id": "T12", "subject": "DependencyConsumerMuxAdapter forwards the real StatusCode; delete the false 'quality is stripped' comment (05/STAB-19)", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T13", "subject": "CalculationDriver input quality: snapshots per dep, hold on Bad, publish worst-of-input once per status change, status-keyed dedup bypass on recovery, ReadOne parity", - "status": "pending", + "status": "done", "blockedBy": [ "T1", "T12" @@ -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" ] @@ -110,13 +110,13 @@ { "id": "T15", "subject": "IVirtualTagEvaluator snapshot overload (default-implemented) + RoslynVirtualTagEvaluator.BuildReadCache pass-through (02/S14)", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T16", "subject": "VirtualTagActor honours input quality: snapshot map, hold on Bad, worst-of status, (value,status) dedup key, reassert carries status; host-bridge historize parity test", - "status": "pending", + "status": "done", "blockedBy": [ "T1", "T15" @@ -125,13 +125,13 @@ { "id": "T17", "subject": "Docs: the quality contract stated once (DriverLifecycle.md) + Raw/Uns/ScriptEditor/Calculation pointers + CLAUDE.md bullet", - "status": "pending", + "status": "done", "blockedBy": [] }, { "id": "T18", "subject": "Full-suite sweep (Runtime.Tests 3x for the #500 flake profile) + STATUS.md / source-report bookkeeping", - "status": "pending", + "status": "done", "blockedBy": [ "T5", "T10", diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 9944f1fb..eb3019d7 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -499,3 +499,41 @@ and the 12-item action list are in `00-OVERALL.md`. **Round-3 plans: NOT yet written.** Next step is one plan per action-list item (R3-01…), house format, as round 2 did. + +--- + +## R3-02 — the stale-Good family (branch `fix/r3-02-stale-good-family`) + +Plan: [`R3-02-stale-good-family-plan.md`](R3-02-stale-good-family-plan.md) (19 tasks; per-task red-before +evidence in that file's **Execution notes**). Closes OVERALL action item #2 — **03/S15**, **05/STAB-19**, +**05/STAB-18**, **02/S14** — by stating the quality contract once and pinning it per seam. + +**Landed (T1–T18, one commit each; branched off `72329eab`, merged master `d79d51dd` mid-flight):** + +| Task | What | +|---|---| +| T1 | `Core.Abstractions.InputQuality` (`WorstOf`/`IsUsable`) — the one worst-of-input rule; `ScriptedAlarmEngine.WorstInputStatus` re-pointed at it (its never-pushed skip stays alarm-local) | +| T2–T5 | **MQTT (05/STAB-18)**: initial push of cached snapshots on every (re)subscribe (plain + Sparkplug); `MqttConnection.ConnectionLost` → stale observed refs to `BadNoCommunication`, value retained; end-to-end driver test through a real socket drop; `docs/drivers/Mqtt.md` | +| T6–T10 | **03/S15**: `IOpcUaAddressSpaceSink.WriteStatuses` (status-only, value retained) + node manager + both sinks; `OpcUaPublishActor.AttributeStatusBatchUpdate`; `DriverHostActor.FanCommsLossToValueNodes`; the recovery audit (`_commsLatched` + `otopcua.driver.comms_latched_refs`); the identical-value recovery contract test + silent-resubscribe control | +| T11 | `DriverInstanceActor.PostStop` announces comms loss (gated on having reached Connected), so an operator Restart / supervision respawn latches Bad — recovery rides R3-03's re-provision | +| T12–T14 | **05/STAB-19**: mux adapter forwards the real status code; Calculation holds on Bad, propagates the worst input status, status-keyed dedup; evaluator read cache carries real status + timestamps | +| T15–T16 | **02/S14**: `IVirtualTagEvaluator` snapshot overload (default-implemented) + Roslyn pass-through; `VirtualTagActor` holds on Bad, inherits the worst input status, `(value, status)` dedup, reassert carries the status | +| T17 | The contract stated once in `docs/DriverLifecycle.md` §"The quality contract"; pointers from `Raw.md`, `Uns.md`, `ScriptEditor.md`, `drivers/Calculation.md`, `drivers/Mqtt.md`; one `CLAUDE.md` bullet | +| T18 | Full unit sweep — every project green; `Runtime.Tests` run 5× (see flake note below) | + +**Client-visible behaviour changes** (called out in the docs task): value nodes of a comms-lost driver +now go **Bad with the last value retained**; MQTT tags go Bad on a broker drop rather than at the 90 s +watchdog; calc tags and virtual tags **hold** while an input is Bad and report that input's sub-code, so +a script that relied on receiving `null` for a dead input no longer runs while it is Bad. + +**Test-suite state at `62c5bc3a`:** full-solution unit sweep green. `Runtime.Tests` 672 passed / 3 +skipped on four separate runs; one further run showed a single failure — +`DriverHostActorNativeAlarmAckRoutingTests.RouteNativeAlarmAck_routes_to_driver_AcknowledgeAsync_with_principal` +(named by the reviewer, who saw it once and then 3× green in isolation). It is untouched by this branch +and matches the known #500 flake profile (~13 %/run); not reproduced in four subsequent full runs. No +pre-existing red on the base commit. + +**NOT done here — T19, the live gate**, which is the merge-blocking rig session (docker-dev + the Modbus +`standard` fixture: constant HR[5] proves the force-republish, HR[100] is the positive control; plus the +deadband falsification leg, the operator-restart leg, and the MQTT retained-replay leg). Recipe in the +plan's §Live gate. diff --git a/docs/DriverLifecycle.md b/docs/DriverLifecycle.md index a157a5d7..ce77f088 100644 --- a/docs/DriverLifecycle.md +++ b/docs/DriverLifecycle.md @@ -306,6 +306,37 @@ So a driver whose `InitializeAsync` disposes anything on its failure path must b `MTConnectDriver` does today, because it is the only one that tears down inside a `catch`. The shutdown paths deliberately keep the single trailing write. +### The quality contract + +> **A value consumer never reports a quality better than its source's.** A node derived from a driver +> value — the raw node, every UNS mirror, a Calculation output, a virtual tag, a scripted condition's +> quality — carries, at worst, the worst status among its inputs: a Bad input makes the derived node Bad +> **with that input's sub-code and the last-known value**; an Uncertain input makes it Uncertain; a source +> that has stopped communicating is a Bad input (`BadNoCommunication`) for everything downstream of it, +> **including the external OPC UA data plane**. Recovery is a *publish*, never an assumption: the first +> post-recovery value must land even when it equals the value held before the outage, so every dedup on +> the path — driver caches, the node's own status, a consumer's last-published memory — must be keyed on +> **(value, status)**, not value alone. + +This is one rule with one implementation of its comparison, +`ZB.MOM.WW.OtOpcUa.Core.Abstractions.InputQuality` (`WorstOf` / `IsUsable`), used by every derived-value +seam. It was written down because the same lie had been fixed four times in four places and re-opened in +a fifth (archreview round 3, theme "the stale-Good family"). The seams that implement it: + +| Seam | What it does | Where | +|---|---|---| +| Driver comms loss → **value nodes** | one status-only batch stamps `BadNoCommunication` on the driver's raw NodeIds and every referencing UNS NodeId, **value retained** | `DriverHostActor.FanCommsLossToValueNodes` (below) | +| Driver comms loss → dependency mux | synthetic Bad per ref, so scripts and alarms see the source die | `DriverHostActor.FanCommsLossToDependencyMux` | +| Driver comms loss → Part 9 conditions | condition quality annotated out of band | `DriverHostActor.OnDriverConnectivityChanged` | +| MQTT / Sparkplug | connection loss stales observed refs; every (re)subscribe pushes the cached snapshot | [`docs/drivers/Mqtt.md`](drivers/Mqtt.md#quality-on-connection-loss-and-re-subscribe) | +| Calculation driver | holds on Bad, propagates the worst input status | [`docs/drivers/Calculation.md`](drivers/Calculation.md#input-quality) | +| Virtual tags | holds on Bad, inherits the worst input status, reassert carries it | [`docs/ScriptEditor.md`](ScriptEditor.md#input-quality-what-a-script-sees-and-when-it-runs-at-all) | +| Scripted alarms | worst-of-input quality annotated out of band; Bad inputs hold the condition | [`docs/ScriptedAlarms.md`](ScriptedAlarms.md) | + +⚠️ **Consumer-visible:** value nodes of a comms-lost driver now go **Bad**, where they used to serve +their last value at Good. A consumer that treats a Bad value as "drop the tag" (ScadaBridge's data +connection layer, say) will see more Bads than before — that is the fix working, not a regression. + ### The comms-loss watchdog (Gitea #527) — `GetHealth()` is now load-bearing `GetHealth()` used to be **reporting-only**: `DriverInstanceActor.PublishHealthSnapshot` read it every 30 s @@ -391,8 +422,8 @@ on it still gets its initialise attempt. #### The fan-out that watchdog triggers: indexed per driver, and one batch `DisconnectObserved` raises `ConnectivityChanged`, and `DriverHostActor` answers it by pushing -`BadNoCommunication` at every ref the flapping driver owns — the value fan to the dependency mux, and the -alarm fan to the publish actor's condition quality. Both used to answer "which refs are this driver's?" +`BadNoCommunication` at every ref the flapping driver owns — **three** fans: the value nodes (the +external OPC UA surface), the dependency mux, and the publish actor's condition quality. Both used to answer "which refs are this driver's?" by **scanning the node's entire ref map and filtering** — the maps are keyed by the composite `(DriverInstanceId, RawPath)`, so one driver's flap walked all ~50k keys on a large node to find its own few thousand. Each now has a secondary index, `DriverInstanceId → [(RawPath, node set)]` @@ -418,6 +449,37 @@ its last one — everything on the subscription path was already wrapped in `Att `DependencyMuxActor` still registers the per-value receive, but in production it is dead code; it stays for tests and direct senders. +#### The third fan: the value nodes (archreview 03/S15) + +The two fans above reach *internal* consumers and *conditions*. Until archreview round 3 the driver's raw +value variables and their UNS mirrors — what an OPC UA client actually subscribes to — got nothing, so a +client saw the last value at **Good** quality for the whole outage. That was invisible on Modbus (which +degrades per tag within one poll interval on its own) and total on every driver whose failure shape is +"the reader stops producing": a coalesced-read throw, a dropped push session, or simply a driver whose +last act was a Good publish. + +`FanCommsLossToValueNodes` closes it with **one status-only batch** — a new sink primitive, +`IOpcUaAddressSpaceSink.WriteStatuses` → `OtOpcUaNodeManager.WriteStatuses` — that changes each node's +StatusCode and Timestamp and **leaves its Value alone**. Status-only is not an optimisation: the host +holds no value cache (v3's single-source design deliberately removed the independent buffer), so only the +node itself can retain the last known value, and a Bad node still carrying its last reading is the OPC UA +Part 8 convention a SCADA consumer expects. Two details: + +- It is **ungated by redundancy role**, like the condition annotation: a Secondary keeps its address space + honest for failover. +- An unmaterialised NodeId is **skipped, never created** — a status carries nothing to create a node from, + and the per-driver index can legitimately name a node a surgical remove has just dropped. + +**Nothing is written on reconnect.** Publishing Good there would assert a quality the host has not +observed. Recovery is the driver's own re-publish: for a poll driver the watchdog's `DetachSubscription` +forces a fresh `PollGroupEngine` subscription whose initial poll runs `forceRaise: true`; for MQTT it is +the initial push added by 05/STAB-18. What the host does instead is **audit** it — `CommsRecoveryAudit` +fires `DefaultCommsRecoveryAuditDelay` (30 s) after the driver reconnects and reports any ref still +latched Bad, as a Warning naming the driver plus the `otopcua.driver.comms_latched_refs` counter (tagged +`driver`, `driver_type`). That counter going non-zero means a driver reconnected without re-publishing — +a push driver missing its initial push, or a value-only dedup swallowing an unchanged value. It is a +tripwire, not a repair: nothing downstream depends on it. + ### The connect path feeds the circuit breaker (R2-10 T10 closure) `IDriver.InitializeAsync` is routed through the capability invoker as diff --git a/docs/Raw.md b/docs/Raw.md index 144ba49f..022451bc 100644 --- a/docs/Raw.md +++ b/docs/Raw.md @@ -123,6 +123,17 @@ the full design. Highlights: rejects an A→B→A oscillation loop naming the members. Compile is not hard-gated (VirtualTag parity); a runtime compile/throw/timeout lands as Bad quality + a `script-logs` entry. +## Quality: a raw tag's node when its driver stops communicating + +A raw tag's node — and every UNS mirror of it — goes **`BadNoCommunication` with its last value +retained** when the owning driver is declared comms-lost, and clears when the driver re-publishes. It is +never cleared by the server on reconnect: recovery is a value, never an assumption. The rule and the +seams that implement it are stated once in +[`docs/DriverLifecycle.md` §"The quality contract"](DriverLifecycle.md#the-quality-contract). + +Calc tags authored here inherit the same rule from their inputs — see +[`docs/drivers/Calculation.md` §"Input quality"](drivers/Calculation.md#input-quality). + ## Retirement note The routed `/clusters/{id}/drivers` driver-authoring flow (`DriverTypePicker`, diff --git a/docs/ScriptEditor.md b/docs/ScriptEditor.md index 1241db9d..88f7c54c 100644 --- a/docs/ScriptEditor.md +++ b/docs/ScriptEditor.md @@ -399,6 +399,29 @@ reference map identically, so their plans stay byte-parity. --- +## Input quality: what a script sees, and when it runs at all + +The rule is stated once, in +[`docs/DriverLifecycle.md` §"The quality contract"](DriverLifecycle.md#the-quality-contract), and it +applies identically to virtual-tag scripts and to `Calculation` driver scripts — they share +`VirtualTagContext` and this editor, so one policy keeps this page true for every `ctx.GetTag` call +site. + +- **`ctx.GetTag(path).StatusCode` and `.SourceTimestampUtc` are real.** The read cache used to stamp + every dependency `Good` with an evaluation-time timestamp, so a quality-aware script could not be + written at all — it was told Good even for an input whose driver had stopped communicating. +- **A script is NOT run while any of its declared inputs is Bad.** The tag instead reports that + input's own status (`BadNoCommunication`, `BadWaitingForInitialData`, …) carrying its last known + value. So the `(double)ctx.GetTag("x").Value` idiom is safe against a dead input: the script simply + does not run, rather than throwing on every trigger and filling `/script-log`. +- **An Uncertain input IS passed to the script**, and the output inherits that Uncertain sub-code. An + Uncertain value is present and a script can make a qualified decision from it; that is the alarm + engine's long-standing rule, now shared. +- **The output never reports better than its worst input.** A Good result over an Uncertain input is + published Uncertain; nothing is published Good while an input is Bad. +- ⚠️ **If you wrote a script that deliberately handled `null` for a dead input, it no longer runs + while that input is Bad.** Assume your inputs are usable — the runtime has already decided they are. + ## Known Limitations / Follow-ups - **Tag-path completion shows resolvable keys only.** Surfacing the UNS browse diff --git a/docs/Uns.md b/docs/Uns.md index 7ccc5238..89651195 100644 --- a/docs/Uns.md +++ b/docs/Uns.md @@ -476,6 +476,17 @@ not — clients pick those up on their next browse. And a **degraded** non-rebui (some node failed to materialise) deliberately does not record its composition as applied, so the next deploy re-diffs and repairs it. +## Quality on a UNS node + +A UNS reference variable mirrors its backing raw tag exactly — value, status and timestamps — so when +that tag's driver stops communicating the mirror goes **`BadNoCommunication` with its last value +retained**, in the same fan-out message as the raw node, and clears only when the driver re-publishes. +A VirtualTag on the same equipment follows the same rule from its own inputs: it holds while any input +is Bad and reports that input's sub-code. The contract, and every seam that implements it, is stated +once in [`docs/DriverLifecycle.md` §"The quality contract"](DriverLifecycle.md#the-quality-contract); +what a script sees is in +[`docs/ScriptEditor.md`](ScriptEditor.md#input-quality-what-a-script-sees-and-when-it-runs-at-all). + ## See also - [Configuration.md](Configuration.md) — the underlying config entities. 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/docs/drivers/Mqtt.md b/docs/drivers/Mqtt.md index 0876c55a..d13bbf16 100644 --- a/docs/drivers/Mqtt.md +++ b/docs/drivers/Mqtt.md @@ -415,6 +415,41 @@ one session, and carrying it across made the host's re-subscribe resolve an empt SUBSCRIBE at all, and leave the driver Connected/Healthy and permanently deaf. Sparkplug's `SparkplugIngestor.AttachTo` resets its own session state for the same reason. +## Quality on connection loss and re-subscribe + +The quality contract this implements is stated once, in +[`docs/DriverLifecycle.md` §"The quality contract"](../DriverLifecycle.md#the-quality-contract). +Two edge behaviours make MQTT — a push-model driver — honour it. A poll driver gets both free from +`PollGroupEngine` (a fresh subscription's first poll runs with `forceRaise: true`); MQTT had neither, +which is archreview 05/STAB-18. + +- **Connection loss stales, keeping the value.** On the `Connected → Reconnecting` edge + `MqttConnection.ConnectionLost` fires, and both ingest paths mark every **already-observed** + reference `BadNoCommunication` with its **last value retained** and publish the change. A + never-observed reference (still `BadWaitingForInitialData`) and an already-Bad one are left alone. + The event does **not** fire for a failed *initial* connect or during dispose — neither is an outage. +- **Every (re)subscribe pushes the cached snapshots.** `SubscribeAsync` republishes each observed + reference's cached snapshot onto the new handle before any broker traffic. This is what refills a + node an address-space rebuild reseeded `BadWaitingForInitialData` on a quiet topic — a rebuild + triggers no retained replay and, in Sparkplug, no rebirth. + +**Why the stale is what makes recovery work.** The plain path suppresses a message whose value *and* +status equal the driver-owned cache, and that cache survives reconnects and redeploys. So the retained +message a broker replays on reconnect — routinely the identical payload — was suppressed, and every +node the host had latched Bad stayed Bad until the plant value happened to change. Staling the status +first makes the identical replay a change. (A regression test for this must replay the **identical** +payload; a different value defeats the suppression on its own and proves nothing.) + +**Two "not communicating" codes, deliberately.** `BadNoCommunication` (0x80310000) means *the link is +down* — the connection-loss stale, and what the host's comms-loss fan-out writes onto the value nodes. +`BadCommunicationError` (0x80050000) keeps its narrower Sparkplug meaning: the **source announced its +own death** (NDEATH/DDEATH), or a birth voided the scope feeding the tag. A refused SUBSCRIBE filter +also degrades to `BadCommunicationError`. + +**Operator-visible change:** an MQTT tag now goes Bad when the broker session drops, rather than +holding its last value at Good until the host's 90 s comms-loss watchdog notices — which, since +MQTTnet usually reconnects well inside that window, in practice meant never. + ## Write-through `IWritable` landed with [#508](https://gitea.dohertylan.com/dohertj2/lmxopcua/issues/508). Before it, diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Engines/IVirtualTagEvaluator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Engines/IVirtualTagEvaluator.cs index 7864246c..0dc8825a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Engines/IVirtualTagEvaluator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Engines/IVirtualTagEvaluator.cs @@ -1,3 +1,5 @@ +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + namespace ZB.MOM.WW.OtOpcUa.Commons.Engines; /// @@ -18,6 +20,34 @@ public interface IVirtualTagEvaluator /// Read-only dictionary of variable names to values for expression evaluation. /// Result containing success flag, evaluated value, and optional failure reason. VirtualTagEvalResult Evaluate(string virtualTagId, string expression, IReadOnlyDictionary dependencies); + + /// + /// Evaluate against dependency snapshots — value, status + /// code and source timestamp per input — so a script's ctx.GetTag(path).StatusCode reports + /// what the source actually said (archreview 02/S14). + /// + /// + /// Default-implemented on purpose. It projects to values and calls the older overload, so + /// every existing implementation — the Roslyn evaluator aside, that is the null evaluator and the + /// test fakes — compiles and behaves unchanged; an implementation that can do better (the Roslyn + /// one) overrides it. Making it abstract would have been a breaking change for a fix whose whole + /// point is that it changes nothing on the Good path. + /// The default necessarily discards the status codes it was given, which is exactly the + /// laundering this overload exists to end — so a consumer that cares must call an implementation + /// that overrides it. + /// + /// The unique identifier of the virtual tag being evaluated. + /// The expression string to evaluate. + /// Read-only dictionary of dependency snapshots, keyed by tag path. + /// Result containing success flag, evaluated value, and optional failure reason. + VirtualTagEvalResult Evaluate( + string virtualTagId, string expression, IReadOnlyDictionary dependencies) + { + ArgumentNullException.ThrowIfNull(dependencies); + var values = new Dictionary(dependencies.Count, StringComparer.Ordinal); + foreach (var kv in dependencies) values[kv.Key] = kv.Value?.Value; + return Evaluate(virtualTagId, expression, values); + } } /// Result of one virtual-tag expression eval. Stash a Reason on every Failure so diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs index ba3a0441..6dc7c9c3 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs @@ -104,6 +104,15 @@ public static class OtOpcUaTelemetry Meter.CreateCounter("otopcua.redundancy.service_level_change", unit: "{change}", description: "OPC UA Server.ServiceLevel transitions emitted by the redundancy state."); + /// Archreview 03/S15 — tag refs a driver left BadNoCommunication after it reconnected: + /// the host marked them Bad on the outage, and the driver never re-published them. Non-zero means a + /// driver that reconnects without a full re-publish (a push driver missing its initial push, or a + /// value-only dedup swallowing an unchanged value), which leaves those nodes Bad indefinitely. + /// Tagged driver + driver_type. + public static readonly Counter DriverCommsLatchedRefs = + Meter.CreateCounter("otopcua.driver.comms_latched_refs", unit: "{ref}", + description: "Refs still BadNoCommunication after their driver reconnected (driver, driver_type)."); + /// Inbound operations denied by the Primary data-plane gate (archreview 03/S4) — /// site=write|ack|alarm-emit, reason=secondary|detached|role-unknown. public static readonly Counter PrimaryGateDenied = diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs index 5c917dc0..b7f477f0 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs @@ -32,6 +32,14 @@ public sealed class DeferredAddressSpaceSink : IOpcUaAddressSpaceSink, ISurgical // (actors inject THIS wrapper, not the inner SdkAddressSpaceSink) — same trap as WireAlarmNotifiers above. public void WriteValues(IReadOnlyList values) => _inner.WriteValues(values); + /// + // Forward the status-only batch (03/S15). Unforwarded, the interface's DEFAULT runs here instead — + // and that default writes a NULL value, so a comms-loss annotation would erase the last known value + // of every node on the driver rather than retaining it, on every driver-role host (actors inject + // THIS wrapper, not the inner SdkAddressSpaceSink). Same trap as WriteValues / WireAlarmNotifiers + // above, except this one degrades to wrong data rather than to slow. + public void WriteStatuses(IReadOnlyList statuses) => _inner.WriteStatuses(statuses); + /// public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc, AddressSpaceRealm realm) => _inner.WriteAlarmCondition(alarmNodeId, state, sourceTimestampUtc, realm); diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs index 2e240a6d..6cb319bd 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs @@ -42,6 +42,31 @@ public interface IOpcUaAddressSpaceSink } } + /// Batched status-only write: each node keeps its CURRENT value and only its + /// StatusCode + SourceTimestamp change. This is the comms-loss annotation for value variables + /// (archreview 03/S15) — a driver that has stopped communicating leaves the node holding its last + /// known value (the Part 8 convention SCADA clients expect) under a + /// status, rather than nulling it. + /// A sink that HOLDS the node's value must retain it — the SDK-backed sink does, because the + /// node itself is where the value lives. This DEFAULT implementation cannot: the host holds no value + /// cache (v3's single-source design), so it degrades to with a null value, + /// which is the honest statement for a value-less sink and keeps every fake compiling unchanged. + /// Same per-item isolation contract as . An unknown or + /// unmaterialised NodeId is a no-op, never a create: a status carries nothing to create a node + /// from, and the caller's index can legitimately name a node a surgical remove has just dropped. + /// The status writes to apply, in order. + /// A batching implementation throws this when some — but not + /// necessarily all — items failed; every other item has been applied. + void WriteStatuses(IReadOnlyList statuses) + { + ArgumentNullException.ThrowIfNull(statuses); + for (var i = 0; i < statuses.Count; i++) + { + var s = statuses[i]; + WriteValue(s.NodeId, null, s.StatusCode, s.SourceTimestampUtc, s.Realm); + } + } + /// Write an alarm-condition's full Part 9 state. When a real condition node has been /// materialised for via , /// this projects the whole @@ -190,6 +215,16 @@ public interface IOpcUaAddressSpaceSink public readonly record struct ValueWrite( string NodeId, object? Value, uint StatusCode, DateTime SourceTimestampUtc, AddressSpaceRealm Realm); +/// One status-only write within a batch: the +/// node's current VALUE is retained and only + +/// change. +/// The OPC UA node id of the variable, within . +/// The full status code, sub-code included, passed to the SDK verbatim (#519). +/// The source timestamp in UTC. +/// Which of the two v3 namespaces lives in. +public readonly record struct StatusWrite( + string NodeId, uint StatusCode, DateTime SourceTimestampUtc, AddressSpaceRealm Realm); + /// Aggregate outcome of a partially-failed batch: /// every non-failing item HAS been applied; this carries what the caller needs to log and meter honestly. /// A batched sink must NOT abandon the tail of a batch when one item throws: the poll engine diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs new file mode 100644 index 00000000..df51b4b8 --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs @@ -0,0 +1,74 @@ +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +/// +/// The one worst-of-input rule every derived-value seam uses — scripted alarms, virtual tags and +/// the Calculation driver. +/// +/// The contract it implements (archreview R3-02, "the stale-Good family"): a value +/// consumer never reports a quality better than its source's. A node derived from driver values +/// carries, at worst, the worst status among its inputs — a Bad input makes the derived node Bad +/// with that input's sub-code, an Uncertain input makes it Uncertain, and a source that has +/// stopped communicating is a Bad input for everything downstream of it. +/// +/// Why it lives here. Core.Abstractions is the one assembly all three seams already +/// reference (Commons, Runtime and the driver assemblies all sit above it), and +/// — the shape the rule reads — is declared beside it. Stating +/// the rule three times is how the three seams drifted apart in the first place. +/// +/// Severity, not equality. Comparison is on the OPC UA Part 4 severity bits (the top +/// two: 00 Good, 01 Uncertain, 10/11 Bad); the sub-code below them is +/// carried through verbatim because the sub-code is the diagnostic +/// (BadNoCommunication = go look at the device, BadWaitingForInitialData = it never +/// arrived, BadInternalError = the script). +/// +public static class InputQuality +{ + /// + /// No value has ever been received for this input — the status a fresh node is materialised + /// with, and what reports for a dependency slot that holds no snapshot + /// at all. Spelled here rather than taken from Commons.OpcUa.OpcUaStatus because + /// Core.Abstractions sits below Commons and deliberately references nothing. + /// + public const uint BadWaitingForInitialData = 0x80320000u; + + /// + /// The worst status among : the highest Part 4 severity, carrying + /// that input's own sub-code. A entry is a dependency that has never + /// been pushed and reports — reporting Good for it is + /// the laundering this helper exists to stop. An empty set is Good: there is nothing to be + /// worse than. + /// + /// The dependency snapshots, in a stable order. + /// The worst status code observed. + /// + /// Among equal severity the first entry seen wins, so a re-evaluation over the same + /// inputs reports the same sub-code instead of oscillating between two equally-bad codes. + /// + public static uint WorstOf(IEnumerable inputs) + { + ArgumentNullException.ThrowIfNull(inputs); + + var worst = 0u; + var worstSeverity = 0u; + foreach (var input in inputs) + { + var status = input?.StatusCode ?? BadWaitingForInitialData; + var severity = status >> 30; + if (severity <= worstSeverity) continue; + worstSeverity = severity; + worst = status; + } + + return worst; + } + + /// + /// True when a consumer may evaluate against a value carrying . + /// Good and Uncertain are usable — an Uncertain value is present and a script can make a + /// qualified decision from it; Bad is not, and a consumer holds instead (the alarm engine's + /// long-standing policy, generalised). + /// + /// The status code to classify. + /// for Good or Uncertain severity. + public static bool IsUsable(uint statusCode) => (statusCode >> 30) < 2; +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs index 2acbb9cd..f8c8e07d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -919,26 +919,18 @@ public sealed class ScriptedAlarmEngine : IDisposable /// the case the skip was hiding was the case that matters most. The upstream source now stamps a /// never-pushed path and nothing else does, so the two are tellable /// apart. A null snapshot is still skipped: that is a cache the refill did not populate, not an - /// observation. + /// observation. + /// The comparison itself lives in (archreview R3-02) + /// — the same rule the virtual-tag and Calculation seams now use, so "worst input wins, carrying its + /// sub-code, first-seen on a severity tie" is stated once. What stays here is the alarm-specific + /// filter: this engine's two skips are deliberately NOT part of the shared rule, because the + /// other two seams treat a never-pushed input as Bad rather than as absent. private static uint WorstInputStatus(IReadOnlyDictionary cache) { - uint worst = 0u; - var worstSeverity = 0u; - foreach (var kv in cache) - { - // No entry at all — nothing was observed, so there is nothing to report on. - if (kv.Value is null) continue; - // Observed as "no data has ever arrived" — the cold-start placeholder, not a fault. - if (kv.Value.StatusCode == StatusNeverPushed) continue; - var status = kv.Value.StatusCode; - var severity = status >> 30; - if (severity > worstSeverity) - { - worstSeverity = severity; - worst = status; - } - } - return worst; + // No entry at all — nothing was observed, so there is nothing to report on. + // Observed as "no data has ever arrived" — the cold-start placeholder, not a fault. + var observed = cache.Values.Where(v => v is not null && v.StatusCode != StatusNeverPushed); + return InputQuality.WorstOf(observed); } /// Update the tracked worst-quality bucket for an alarm; return true iff the 3-state bucket 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 2363b539..99ee773b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs @@ -82,7 +82,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba // SHORT-HELD state lock — it is taken for dictionary work only and is NEVER held across a script run // (Driver.Calculation-005). private readonly object _lock = new(); - private readonly Dictionary _depValues = new(StringComparer.Ordinal); + // Dependency SNAPSHOTS, not bare values (archreview 05/STAB-19): the status code is what tells a + // comms-lost input apart from a genuine Good null, and storing values only is what let this driver + // compute a Good output from a dead source. + private readonly Dictionary _depValues = new(StringComparer.Ordinal); private readonly HashSet _arrivedDeps = new(StringComparer.Ordinal); private readonly Dictionary _stateByRawPath = new(StringComparer.Ordinal); @@ -332,7 +335,7 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba if (!_running) return; lock (_lock) { - _depValues[rawPath] = value; + _depValues[rawPath] = new DataValueSnapshot(value, statusCode, timestampUtc, timestampUtc); _arrivedDeps.Add(rawPath); if (!_changeDependents.TryGetValue(rawPath, out var dependents) || dependents.Count == 0) return; @@ -391,9 +394,16 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba var now = DateTime.UtcNow; lock (_lock) { - if (!_stateByRawPath.TryGetValue(reference, out var st) || !st.HasValue) + // Gated on HAS PUBLISHED, not on "has a value": a tag whose only publish was an inputs-Bad + // report has no value of its own, and answering BadWaitingForInitialData for it would tell a + // polled reader "never observed" while the subscriber on the same node had been told + // BadNoCommunication. Something HAS been observed about such a tag — that its input is dead. + if (!_stateByRawPath.TryGetValue(reference, out var st) || !st.HasPublished) return new DataValueSnapshot(null, StatusBadWaitingForInitialData, null, now); - var status = st.LastPublishedBad ? StatusBadInternalError : StatusGood; + // Exactly what the subscriber was last told — including a propagated input sub-code. Deriving + // it from a two-state flag here would make a read disagree with the subscription over the + // same node (05/STAB-19). + var status = st.LastPublishedStatus; return new DataValueSnapshot(st.LastValue, status, st.LastTimestampUtc, st.LastTimestampUtc); } } @@ -404,12 +414,16 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba /// trigger fired, and that trigger's timestamp. Built under by /// and executed outside it, so the script itself needs no shared state. /// The calc tag being evaluated. - /// Its declared reads' values, as of the trigger. + /// Its declared reads' snapshots — value, status and timestamps — as of the trigger. /// The trigger's timestamp, stamped on whatever is published. + /// The worst status among + /// (). Bad ⇒ the script is HELD and this status is published; + /// Good or Uncertain ⇒ the script runs and its result is stamped with it. private sealed record PendingEvaluation( CalculationTagDefinition Definition, - IReadOnlyDictionary Dependencies, - DateTime TimestampUtc); + IReadOnlyDictionary Dependencies, + DateTime TimestampUtc, + uint WorstInputStatus); /// /// Prepare one calc tag's evaluation: resolve the definition, apply the change-gate, and snapshot @@ -441,10 +455,14 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba if (string.IsNullOrWhiteSpace(def.ScriptSource)) return null; // unresolved script — never computes if (!def.DependencyRefs.All(_arrivedDeps.Contains)) return null; // still warming up — do not evaluate - var deps = new Dictionary(StringComparer.Ordinal); + var deps = new Dictionary(StringComparer.Ordinal); foreach (var dep in def.DependencyRefs) if (_depValues.TryGetValue(dep, out var v)) deps[dep] = v; - return new PendingEvaluation(def, deps, timestampUtc); + + // Worst-of-input, computed from the SAME snapshot the evaluation will use, so the status + // published can never disagree with the values it was derived from (05/STAB-19). + var worst = InputQuality.WorstOf(def.DependencyRefs.Select(r => deps.GetValueOrDefault(r))); + return new PendingEvaluation(def, deps, timestampUtc, worst); } /// Start the evaluation pump if it is not already running. MUST be called under @@ -513,6 +531,28 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba private void RunEvaluation(PendingEvaluation pending) { var def = pending.Definition; + + // HOLD ON BAD (05/STAB-19, matching ScriptedAlarmEngine's AreInputsReady): a Bad input is not + // usable, so the script does NOT run — which removes the "(double)ctx.GetTag(x).Value threw" + // storm at its source rather than catching it downstream — and the tag reports the input's own + // sub-code. Uncertain IS usable: the script runs and the output inherits the Uncertain code. + if (!InputQuality.IsUsable(pending.WorstInputStatus)) + { + (object? Value, uint StatusCode)? badPublish; + lock (_lock) + { + if (!_stateByRawPath.TryGetValue(def.RawPath, out var badState)) return; + badPublish = MarkInputsBad(badState, pending.WorstInputStatus, pending.TimestampUtc); + } + + // No script log: an input dying is not a script fault, and the script log is the per-failure + // SCRIPT diagnostic. Here the status code is the diagnostic. + if (badPublish is { } bad) Publish(def.RawPath, bad.Value, bad.StatusCode, pending.TimestampUtc); + return; + } + + // 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; @@ -530,7 +570,7 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba } else { - toPublish = MarkEvaluationSucceeded(st, result.Value, pending.TimestampUtc); + toPublish = MarkEvaluationSucceeded(st, result.Value, pending.WorstInputStatus, pending.TimestampUtc); } } @@ -539,23 +579,51 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba } /// Record a successful evaluation. MUST be called under ; returns what to - /// publish, or null when the value is deduped. The dedup is bypassed while recovering from Bad so a - /// recovery whose value equals the pre-failure value still republishes Good (otherwise the node would - /// stay Bad forever). + /// publish, or null when the result is deduped. + /// The dedup key is (value, status), not value alone — 05/STAB-19 generalising the + /// Bad-recovery bypass 02/S13 introduced. Recovery is a publish, never an assumption: the node is + /// currently carrying the previous status, so a result whose VALUE is unchanged but whose STATUS + /// differs — the shape of every recovery, since a device that comes back usually comes back holding + /// the same reading — must still land, or the node stays Bad forever. /// The tag's published state. /// The computed value. + /// The status to stamp: the worst input status (Good, or an Uncertain sub-code). /// The trigger's timestamp. /// The snapshot to publish, or null when deduped. - private (object? Value, uint StatusCode)? MarkEvaluationSucceeded(TagState st, object? value, DateTime timestampUtc) + private (object? Value, uint StatusCode)? MarkEvaluationSucceeded( + TagState st, object? value, uint statusCode, DateTime timestampUtc) { - if (!st.LastPublishedBad && st.HasValue && Equals(st.LastValue, value)) return null; + if (st.HasPublished && st.LastPublishedStatus == statusCode && st.HasValue && Equals(st.LastValue, value)) + return null; st.HasValue = true; st.LastValue = value; - st.LastPublishedBad = false; + st.HasPublished = true; + st.LastPublishedStatus = statusCode; st.LastTimestampUtc = timestampUtc; _lastComputeUtc = DateTime.UtcNow; - return (value, StatusGood); + return (value, statusCode); + } + + /// Record that this tag's INPUTS are Bad, so no evaluation ran (05/STAB-19). Publishes the + /// worst input's status — carrying the last known value — once per change of that status: a + /// repeat of the same Bad is silent, while BadNoCommunication arriving over an earlier + /// BadInternalError (or the reverse) is not masked, because the sub-code is what names which + /// failure the operator is looking at. MUST be called under . + /// The tag's published state. + /// The worst status among the tag's declared inputs. + /// The trigger's timestamp. + /// The snapshot to publish, or null when that status was already published. + private static (object? Value, uint StatusCode)? MarkInputsBad( + TagState st, uint worstInputStatus, DateTime timestampUtc) + { + if (st.HasPublished && st.LastPublishedStatus == worstInputStatus) return null; + + st.HasPublished = true; + st.LastPublishedStatus = worstInputStatus; + st.LastTimestampUtc = timestampUtc; + // Carry the last-known value (null if none) with the input's Bad status. + return (st.HasValue ? st.LastValue : null, worstInputStatus); } /// Failure tail (mirrors VirtualTagActor 02/S13): the caller always emits the @@ -568,9 +636,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba /// The Bad snapshot to publish on a fresh Good→Bad transition, else null. private static (object? Value, uint StatusCode)? MarkEvaluationFailed(TagState st, DateTime timestampUtc) { - if (st.LastPublishedBad) return null; + if (st.HasPublished && st.LastPublishedStatus == StatusBadInternalError) return null; - st.LastPublishedBad = true; + st.HasPublished = true; + st.LastPublishedStatus = StatusBadInternalError; st.LastTimestampUtc = timestampUtc; // Carry the last-known value (null if none) with Bad quality. return (st.HasValue ? st.LastValue : null, StatusBadInternalError); @@ -643,7 +712,15 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba { public bool HasValue; public object? LastValue; - public bool LastPublishedBad; + + /// Whether anything has been published for this tag yet. Distinct from + /// : an inputs-Bad publish carries no value of its own. + public bool HasPublished; + + /// The status code last published for this tag. Replaces the old two-state + /// LastPublishedBad flag (05/STAB-19): the dedup key is (value, status), and a Bad status + /// now carries the failing INPUT's sub-code rather than a fixed BadInternalError. + public uint LastPublishedStatus; public DateTime LastTimestampUtc; } 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/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs index 78dbdf1f..f2a01b32 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs @@ -349,6 +349,31 @@ public sealed class MqttConnection : IAsyncDisposable, IMqttSubscribeTransport, /// public event Func? Reconnected; + /// + /// Raised on the Connected → Reconnecting edge — the session this connection had is + /// gone. Ingest paths stale their cached values to BadNoCommunication here (archreview + /// 05/STAB-18): without it, MQTT reports Good over a dead broker until the host's 90 s + /// comms-loss watchdog notices, and MQTTnet's own supervisor usually reconnects well inside + /// that window so the outage is never surfaced at all. + /// + /// + /// + /// Runs on MQTTnet's dispatcher thread (this is raised from the library's + /// DisconnectedAsync callback), so a handler must be non-blocking, must not do I/O + /// and must not await — the same constraint carries. A + /// throwing handler is caught and logged rather than escaping into the pump. + /// + /// + /// Exactly once per edge, and only for a real outage. It does not fire for a failed + /// initial connect (there was no session to lose — the values are still + /// BadWaitingForInitialData) nor during (an intentional + /// teardown, which every redeploy performs). It is deliberately NOT the mirror of + /// : recovery is a value the broker republishes, never an + /// assumption this connection makes on the caller's behalf. + /// + /// + public event Action? ConnectionLost; + /// /// Raised for every inbound application message, on MQTTnet's own dispatcher thread. This /// connection does no routing, parsing or typing of its own — that is the subscription @@ -939,6 +964,19 @@ public sealed class MqttConnection : IAsyncDisposable, IMqttSubscribeTransport, if (State == MqttConnectionState.Connected) { SetState(MqttConnectionState.Reconnecting); + + // The quality edge (05/STAB-18): exactly here, once per outage, and never for an initial + // connect failure or a dispose — both of which are excluded by the state guard above. + // Contained, because this runs on the library's dispatcher: a throwing ingest handler must + // not stop the reconnect wake below. + try + { + ConnectionLost?.Invoke(); + } + catch (Exception ex) + { + _logger?.LogError(ex, "MQTT driver '{DriverId}': a connection-lost handler threw.", _driverId); + } } // Released unconditionally rather than only when the client had been connected: a missed diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs index 4d28f0da..57a1fe39 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs @@ -89,6 +89,16 @@ public sealed class MqttSubscriptionManager /// The broker refused, or never answered, the SUBSCRIBE for this reference's topic. private const uint StatusBadCommunicationError = 0x80050000u; + /// + /// No value has ever been observed for this reference — 's + /// answer for a RawPath nothing has published under yet. The initial push skips it: pushing + /// this would claim an observation the driver has not made. + /// + private const uint StatusBadWaitingForInitialData = 0x80320000u; + + /// The broker session this driver was observing is gone — see . + private const uint StatusBadNoCommunication = 0x80310000u; + private readonly int _defaultQos; private readonly string _driverId; private readonly ILogger? _logger; @@ -230,6 +240,45 @@ public sealed class MqttSubscriptionManager _transport = connection; connection.MessageReceived += HandleMessage; connection.Reconnected += OnReconnectedAsync; + connection.ConnectionLost += OnConnectionLost; + } + + /// + /// Marks every already-observed reference BadNoCommunication, keeping its last + /// value, and publishes the change — the driver's answer to a lost broker session + /// (archreview 05/STAB-18 shape a). Wired to by + /// ; public so tests (and Sparkplug's ingestor) can drive the edge + /// directly. + /// + /// + /// + /// This is what makes the reconnect recover. The steady-state suppression in + /// compares value AND status against the driver-owned cache, which + /// survives reconnects and redeploys — so the retained replay of an unchanged payload was + /// suppressed and every node latched Bad by the host stayed Bad until the plant value + /// happened to change. Staling the status first makes that identical replay a change. + /// + /// + /// A never-observed reference is left alone (it is already + /// , which says something more precise), and an + /// already-Bad one is not re-stamped. Runs on MQTTnet's dispatcher thread: dictionary work + /// and synchronous invokes only — exactly what + /// already does there — and never awaits. + /// + /// + public void OnConnectionLost() + { + var now = DateTime.UtcNow; + foreach (var rawPath in _authored.ByRawPath.Keys) + { + var prior = Values.Read(rawPath); + if (prior.StatusCode == StatusBadWaitingForInitialData) continue; // never observed + if ((prior.StatusCode >> 30) >= 2) continue; // already Bad + + var staled = prior with { StatusCode = StatusBadNoCommunication, ServerTimestampUtc = now }; + Values.Update(rawPath, staled); + Raise(rawPath, staled); + } } /// @@ -356,6 +405,13 @@ public sealed class MqttSubscriptionManager _handleByRawPath[def.Name] = handle; } + // Initial push (05/STAB-18 shape b): every ref this driver has already observed is republished + // onto the new handle before any broker traffic. A poll driver gets this free — PollGroupEngine + // runs the first poll of a new subscription with forceRaise: true — and CalculationDriver does + // it explicitly; MQTT did not, so a node reseeded BadWaitingForInitialData by an address-space + // rebuild stayed there until the plant published again, which for a quiet topic is never. + PushObservedSnapshots(defs); + // One SUBSCRIBE per distinct topic, and only for topics not already live: re-SUBSCRIBEing an // established filter is legal but pointless, and it re-triggers the retained seed. var pending = defs.Where(d => !_subscribedTopics.ContainsKey(d.Topic)).ToList(); @@ -756,6 +812,12 @@ public sealed class MqttSubscriptionManager // timestamp — and only the notification is skipped. "Cached but not raised" is already a valid // state on this path (see Raise: an authored-but-unsubscribed tag does exactly that). // + // What makes this suppression SAFE is the pair of edge behaviours around it (05/STAB-18): + // PushObservedSnapshots re-delivers the cached snapshot on every (re)subscribe, and + // OnConnectionLost stales every observed ref to BadNoCommunication — so the reconnect's + // retained replay of an unchanged payload differs in STATUS and is not suppressed. Without + // those, this comparison is what pinned a comms-lost tag Bad until the plant value changed. + // // Sparkplug bypasses this entirely. SparkplugIngestor.cs updates Values and raises directly // (its own Values.Update()/Raise() calls, not through this Deliver method), so Sparkplug B // ingestion gets no unchanged-value suppression — deliberately out of scope here. @@ -788,6 +850,29 @@ public sealed class MqttSubscriptionManager return !Equals(previous, current); } + /// + /// Republishes the cached snapshot of every already-observed reference in + /// — the push-model equivalent of PollGroupEngine's + /// forceRaise: true on the first poll of a new subscription (05/STAB-18 shape b). + /// + /// + /// A never-observed reference is skipped: its cached status is + /// , and republishing that would assert an + /// observation this driver has not made. The cache is deliberately NOT re-Updated here — + /// nothing new was observed, only re-delivered, so the stored timestamps stay honest and the + /// steady-state suppression still compares against the real last observation. + /// + /// The definitions the new handle covers. + private void PushObservedSnapshots(IReadOnlyList defs) + { + for (var i = 0; i < defs.Count; i++) + { + var snapshot = Values.Read(defs[i].Name); + if (snapshot.StatusCode == StatusBadWaitingForInitialData) continue; + Raise(defs[i].Name, snapshot); + } + } + /// /// Raises for a reference, if a live subscription covers it. /// A throwing subscriber is contained so it cannot starve the rest of the fan-out — or, worse, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SparkplugIngestor.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SparkplugIngestor.cs index 156d356e..4ccf883c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SparkplugIngestor.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SparkplugIngestor.cs @@ -101,9 +101,28 @@ public sealed class SparkplugIngestor /// OpcUaClient and this driver's own plain-mode path all emit it, while no driver emits /// BadNoCommunication at all (it appears once, in a CLI formatter's label map). Following the /// precedent keeps one code meaning "not communicating" across the whole fleet. + /// Superseded in part (archreview R3-02): now + /// has a distinct, fleet-wide meaning — "the link to the source is down" — written by the host's + /// comms-loss fan-out and by . This code keeps the narrower one it + /// always had here: the source itself announced it is offline, or a birth voided the + /// scope. /// private const uint StatusBadCommunicationError = 0x80050000u; + /// + /// The broker session this driver was observing is gone — see . + /// Distinct from , which means the source announced its + /// own death. + /// + private const uint StatusBadNoCommunication = 0x80310000u; + + /// + /// No value has ever been observed for this reference — LastValueCache.Read's answer for + /// a RawPath nothing has published under yet. Neither the initial push nor the connection-loss + /// stale touches such a reference: both would claim an observation that never happened. + /// + private const uint StatusBadWaitingForInitialData = 0x80320000u; + /// The metric's value does not fit the tag's effective data type — including an explicit Sparkplug null. private const uint StatusBadTypeMismatch = 0x80740000u; @@ -317,6 +336,47 @@ public sealed class SparkplugIngestor _publishTransport = connection; connection.MessageReceived += HandleMessage; connection.Reconnected += OnReconnectedAsync; + connection.ConnectionLost += OnConnectionLost; + } + + /// + /// Marks every already-observed metric BadNoCommunication, keeping its last value, + /// and publishes the change — this driver's answer to a lost broker session (archreview + /// 05/STAB-18 shape a). Wired to by + /// ; public so tests can drive the edge directly. + /// + /// + /// + /// A death is a different event and keeps its own code. NDEATH/DDEATH is the source + /// announcing it is offline and stales with as it + /// always has; this is our transport losing the broker, and it reports + /// — the same code the host's comms-loss fan-out + /// writes onto the value nodes, so an operator sees one story from both ends. + /// + /// + /// It is what makes the reconnect recover. A reconnect replays NBIRTH/DBIRTH carrying + /// values that are routinely unchanged; the birth path publishes them, but every node the + /// host latched Bad needs a change to un-latch. Staling first guarantees the replay differs. + /// + /// + /// Runs on MQTTnet's dispatcher thread: dictionary reads and synchronous + /// invokes only, never an await. A never-observed metric and an + /// already-Bad one are both left alone. + /// + /// + public void OnConnectionLost() + { + var now = _utcNow(); + foreach (var rawPath in _authored.ByRawPath.Keys) + { + var prior = Values.Read(rawPath); + if (prior.StatusCode == StatusBadWaitingForInitialData) continue; // never observed + if ((prior.StatusCode >> 30) >= 2) continue; // already Bad + + var staled = prior with { StatusCode = StatusBadNoCommunication, ServerTimestampUtc = now }; + Values.Update(rawPath, staled); + Raise(rawPath, staled); + } } /// @@ -502,6 +562,13 @@ public sealed class SparkplugIngestor _handleByRawPath[def.Name] = handle; } + // Initial push (05/STAB-18 shape b): every metric this driver has already observed is + // republished onto the new handle. A poll driver gets this free from PollGroupEngine's + // forceRaise on the first poll of a new subscription; Sparkplug pushed nothing, so an + // address-space rebuild — which reseeds every node BadWaitingForInitialData and triggers no + // rebirth — left a quiet metric waiting for the plant indefinitely. + PushObservedSnapshots(defs); + return Task.FromResult(handle); } @@ -580,10 +647,16 @@ public sealed class SparkplugIngestor /// already empty, so calling it three times during one connect costs three no-ops. /// /// - /// Values are deliberately not staled here. A session change is a driver-side event, - /// not a statement about the plant; last observed values remain the best available answer - /// until a birth lands, and the connection's own health surface already reports the outage. - /// Only a death — the source saying it is offline — stales a tag. + /// Values are not staled here — but they ARE staled on a connection loss (archreview + /// R3-02; this remark used to claim the opposite and that was the stale-Good bug). What is + /// still true is the narrow part: a session reset for a config rebuild on a live + /// connection says nothing about the plant, so last observed values remain the best answer + /// until a birth lands. What was wrong was the reason given for the general case — "the + /// connection's own health surface already reports the outage". Health is not quality: an + /// OPC UA client subscribed to the metric's node sees neither the health page nor the + /// outage, and reported Good over a dead broker. The transport loss is now handled by + /// (, value retained); + /// a death still stales the way it always did (). /// /// /// Why the reset is happening, for the diagnostic log. @@ -1376,6 +1449,28 @@ public sealed class SparkplugIngestor return new DataValueSnapshot(null, StatusBadTypeMismatch, stamp, serverUtc); } + /// + /// Republishes the cached snapshot of every already-observed reference in + /// — the push-model equivalent of PollGroupEngine's + /// forceRaise: true on the first poll of a new subscription (05/STAB-18 shape b). + /// + /// + /// A never-observed reference is skipped: its cached status is + /// , which is a more precise statement than any + /// value this could republish. The cache is not re-Updated — nothing new was observed, + /// only re-delivered — so the stored timestamps stay honest. + /// + /// The definitions the new handle covers. + private void PushObservedSnapshots(IReadOnlyList defs) + { + for (var i = 0; i < defs.Count; i++) + { + var snapshot = Values.Read(defs[i].Name); + if (snapshot.StatusCode == StatusBadWaitingForInitialData) continue; + Raise(defs[i].Name, snapshot); + } + } + /// Marks every authored tag in as no longer communicating. private void FanStale(SparkplugScope scope, DateTime? sourceUtc, string reason) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs index 2e028c28..a0fc339d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs @@ -59,7 +59,34 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IScriptCac } /// + /// + /// Value-only overload. Every dependency reaches the script stamped Good with an + /// evaluation-time timestamp — bare values carry nothing better. The runtime calls the snapshot + /// overload instead (archreview 02/S14); this one remains for callers that genuinely hold only + /// values. + /// public VirtualTagEvalResult Evaluate(string virtualTagId, string expression, 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(virtualTagId, expression, snapshots); + } + + /// + /// + /// Overrides the interface's default (which projects to values and loses the status codes): the + /// snapshots pass through as the script's read cache, so + /// ctx.GetTag(path).StatusCode and .SourceTimestampUtc are the input's own rather + /// than a fabricated Good stamped at evaluation time (archreview 02/S14). The virtual-tag + /// actor holds evaluation entirely while any input is Bad, so what a script observes here is Good + /// or Uncertain. + /// + public VirtualTagEvalResult Evaluate( + string virtualTagId, string expression, IReadOnlyDictionary dependencies) { if (_disposed) return VirtualTagEvalResult.Failure("evaluator disposed"); if (string.IsNullOrWhiteSpace(expression)) return VirtualTagEvalResult.Failure("empty expression"); @@ -71,12 +98,14 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IScriptCac // so the script returns null — Ok(null) here too. if (PassthroughScript.TryMatch(expression, out var passthroughRef)) { - return dependencies.TryGetValue(passthroughRef, out var ptValue) - ? VirtualTagEvalResult.Ok(ptValue) + // The VALUE only — the caller stamps the result with the worst input status, which for a + // single-dependency passthrough IS this input's status. + 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 VirtualTagId from the virtual-tag id // (in the live path the script id equals the virtual-tag id) so the Script-log page can // attribute each line. EquipmentId stays unbound for now. @@ -162,21 +191,6 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IScriptCac /// public void ClearCompiledScripts() => _cache.Clear(); - private static IReadOnlyDictionary BuildReadCache( - IReadOnlyDictionary deps) - { - // VirtualTagContext.GetTag returns a DataValueSnapshot — we wrap each raw dep value - // as Good-quality so the script's `(int)ctx.GetTag("a").Value` pattern works. Null - // values stay null; the script can null-check via GetTag(path).Value. - 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 and cleans up all cached script evaluators. public void Dispose() { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 8a1c3d1a..fa9c4007 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -649,6 +649,109 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } + /// Apply a batch of status-only writes: each node keeps its CURRENT value and only its + /// StatusCode + Timestamp change (archreview 03/S15 — the comms-loss annotation for value variables). + /// Same shape as : an outside-the-lock validate/resolve pre-pass, then + /// -sized chunks under one Lock each, per-item isolation, + /// and a aggregate. + /// It never creates a node. A status carries nothing to create one from, and the caller's + /// per-driver ref index can legitimately name a node a surgical remove has just dropped — so an unknown + /// id is skipped, not materialised, and is not counted as a failure either (it is an expected race, not + /// a fault). + /// The status writes to apply, in order. + /// When one or more items failed validation or threw. Every + /// other item HAS been applied. + public void WriteStatuses(IReadOnlyList statuses) + { + ArgumentNullException.ThrowIfNull(statuses); + EnsureAddressSpaceCreated(); + if (statuses.Count == 0) return; + + var resolved = new ResolvedWrite[statuses.Count]; + string? firstFailedNodeId = null; + Exception? firstFailure = null; + var failed = 0; + + for (var i = 0; i < statuses.Count; i++) + { + var nodeId = statuses[i].NodeId; + if (string.IsNullOrEmpty(nodeId)) + { + failed++; + firstFailedNodeId ??= nodeId ?? string.Empty; + firstFailure ??= new ArgumentException( + $"WriteStatuses item {i} has a null or empty NodeId.", nameof(statuses)); + continue; + } + + resolved[i] = new ResolvedWrite( + nodeId, NamespaceIndexForRealm(statuses[i].Realm), MapKey(statuses[i].Realm, nodeId), IsValid: true); + } + + var succeeded = 0; + var skipped = 0; + for (var start = 0; start < statuses.Count; start += MaxWritesPerLockAcquisition) + { + var end = Math.Min(start + MaxWritesPerLockAcquisition, statuses.Count); + lock (Lock) + { + for (var i = start; i < end; i++) + { + if (!resolved[i].IsValid) continue; + var s = statuses[i]; + try + { + if (WriteStatusLocked(resolved[i].Key, s.StatusCode, s.SourceTimestampUtc)) succeeded++; + else skipped++; + } + catch (Exception ex) + { + failed++; + firstFailedNodeId ??= s.NodeId; + firstFailure ??= ex; + } + } + } + } + + if (skipped > 0) + { + // Debug, not Warning: the caller's index racing a surgical remove is expected, and a driver + // whose nodes were all just removed would otherwise log once per ref. This manager carries no + // ILogger, so log via the SDK's static trace (see ServeNodeAsync). +#pragma warning disable CS0618 // Type or member is obsolete + Utils.LogDebug( + "OtOpcUaNodeManager: WriteStatuses skipped {0} of {1} item(s) — node not materialised.", + skipped, + statuses.Count); +#pragma warning restore CS0618 + } + + if (failed > 0) + { + throw new ValueWriteBatchException( + firstFailedNodeId ?? string.Empty, failed, succeeded, firstFailure!); + } + } + + /// The body of a single status-only write. Caller MUST hold Lock. + /// The realm-qualified map key. + /// The full OPC UA status code, sub-code included. + /// The timestamp of the annotation in UTC. + /// when the node existed and was stamped; when + /// no such node is materialised (skipped — deliberately not a create; see ). + private bool WriteStatusLocked(RealmKey key, uint statusCode, DateTime sourceTimestampUtc) + { + if (!_variables.TryGetValue(key, out var variable)) return false; + + // Value deliberately untouched — that IS the feature: the node keeps its last known value under a + // Bad status (OPC UA Part 8's last-known-value convention). + variable.StatusCode = new StatusCode(statusCode); + variable.Timestamp = sourceTimestampUtc; + variable.ClearChangeMasks(SystemContext, includeChildren: false); + return true; + } + /// One item after the outside-the-lock pre-pass: its namespace index and /// map key already resolved, or false when the item failed validation. /// The item's bare s= identifier. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs index dcb135f5..ab2bd589 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs @@ -31,6 +31,13 @@ public sealed class SdkAddressSpaceSink : IOpcUaAddressSpaceSink, ISurgicalAddre public void WriteValues(IReadOnlyList values) => _nodeManager.WriteValues(values); + /// + /// Overrides the interface's default on purpose, and here the default is not merely slower but + /// wrong: it writes a null value (a value-less sink has nothing else to write), which would erase + /// the last known value this annotation exists to preserve. The manager's own entry point retains it. + public void WriteStatuses(IReadOnlyList statuses) + => _nodeManager.WriteStatuses(statuses); + /// public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc, AddressSpaceRealm realm) => _nodeManager.WriteAlarmCondition(alarmNodeId, state, sourceTimestampUtc, realm); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index cbccf7a4..dd56ab17 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -91,6 +91,14 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// connect work, so it should not run more often than an ordinary reconnect attempt. public static readonly TimeSpan DefaultReprovisionBackoff = TimeSpan.FromSeconds(10); + /// + /// How long after a driver reconnects the 03/S15 recovery audit waits before reporting refs still + /// latched . Sized so an ordinary poll driver has had + /// several poll cycles — and a slow one at least one — to re-publish before anything is said; the + /// audit's job is to name a driver that will NEVER re-publish, not to race a slow first poll. + /// + public static readonly TimeSpan DefaultCommsRecoveryAuditDelay = TimeSpan.FromSeconds(30); + /// /// Cache key used when an artifact carries no cluster scoping (single-cluster or unscoped /// deployments). A literal rather than an empty string so the row is visibly deliberate when @@ -426,6 +434,27 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// ). private readonly TimeSpan _applyWedgeTimeout; + /// How long after a driver reconnects the 03/S15 recovery audit waits before reporting refs + /// still latched (see ). + private readonly TimeSpan _commsRecoveryAuditDelay; + + /// + /// Archreview 03/S15 — RawPaths this host has stamped BadNoCommunication on the value + /// nodes of, per driver, and has not yet seen a value for. Set by + /// , pruned by as each ref + /// re-publishes, and read once by after the driver reconnects. + /// It is a tripwire, not a mechanism. Nothing depends on it being correct for the data + /// plane to behave: the host publishes nothing Good on reconnect either way. What it buys is that a + /// driver which reconnects and then fails to re-publish — the pre-STAB-18 MQTT shape, and what any + /// future push driver without an initial push will do — produces a Warning and a metered count + /// instead of a subscription that silently reports Bad forever. + /// Keyed by RawPath, so a raw node and its UNS mirrors count once: the operator is being told + /// how many tags did not come back, not how many address-space nodes they project onto. The + /// entry is removed as soon as it empties, so the steady-state cost in + /// is one dictionary miss per burst. + /// + private readonly Dictionary> _commsLatched = new(StringComparer.Ordinal); + /// Per-driver rate limit for the supervision-reprovision hook (see /// ). private readonly TimeSpan _reprovisionBackoff; @@ -483,6 +512,12 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// The authenticated principal performing the acknowledge. public sealed record RouteNativeAlarmAck(string ConditionNodeId, string? Comment, string OperatorUser); + /// Self-sent by the 03/S15 audit timer, after a driver + /// reconnected: report any of its refs still latched BadNoCommunication. See + /// . + /// The driver whose recovery is being audited. + public sealed record CommsRecoveryAudit(string DriverInstanceId); + private sealed record ChildEntry(IActorRef Actor, DriverInstanceSpec Spec, bool Stubbed) { // Convenience accessors for sites that don't need the full spec. @@ -548,6 +583,9 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// VirtualTag + ScriptedAlarm hosts. Defaults to null (admin-only nodes / tests skip the emit). /// How long the host may sit in before the /// wedge watchdog logs a Warning. Defaults to . + /// Archreview 03/S15: how long after a driver reconnects the + /// recovery audit waits before reporting refs still latched BadNoCommunication. Defaults to + /// ; tests shorten it. /// The Akka.NET used to spawn this actor. public static Props Props( IDbContextFactory? dbFactory, @@ -577,7 +615,8 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS IHistorizationRoleView? historizationRoleView = null, Func? sharesHistorizationScopeWithLocal = null, TimeSpan? applyWedgeTimeout = null, - TimeSpan? reprovisionBackoff = null) => + TimeSpan? reprovisionBackoff = null, + TimeSpan? commsRecoveryAuditDelay = null) => // WARNING: this forwarding list is POSITIONAL, and Props.Create compiles it into an // expression tree. Six IActorRef? parameters and several interface-typed ones mean a // mis-ordered argument is usually type-compatible and therefore compiles clean, then binds @@ -589,7 +628,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS loggerFactory, scriptRootLogger, scriptedAlarmHostOverride, invokerFactory, driverMemberCountProvider, deploymentArtifactCache, redundancyRoleView, replicationPeerHost, fetchAndCacheMode, artifactFetcher, alarmStateStore, telemetryHub, primaryRoleSource, historizationRoleView, - sharesHistorizationScopeWithLocal, applyWedgeTimeout, reprovisionBackoff)); + sharesHistorizationScopeWithLocal, applyWedgeTimeout, reprovisionBackoff, commsRecoveryAuditDelay)); /// Initializes a new DriverHostActor with the specified dependencies. /// Database context factory for configuration database access. @@ -648,6 +687,9 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// How long the host may sit in before the /// wedge watchdog logs a Warning. Defaults to ; a test shortens /// it so the watchdog is observable in a test's lifetime. + /// Archreview 03/S15: how long after a driver reconnects the + /// recovery audit waits before reporting refs still latched BadNoCommunication. Defaults to + /// ; a test shortens it for the same reason. public DriverHostActor( IDbContextFactory? dbFactory, CommonsNodeId localNode, @@ -676,10 +718,12 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS IHistorizationRoleView? historizationRoleView = null, Func? sharesHistorizationScopeWithLocal = null, TimeSpan? applyWedgeTimeout = null, - TimeSpan? reprovisionBackoff = null) + TimeSpan? reprovisionBackoff = null, + TimeSpan? commsRecoveryAuditDelay = null) { _applyWedgeTimeout = applyWedgeTimeout ?? DefaultApplyWedgeTimeout; _reprovisionBackoff = reprovisionBackoff ?? DefaultReprovisionBackoff; + _commsRecoveryAuditDelay = commsRecoveryAuditDelay ?? DefaultCommsRecoveryAuditDelay; _historizationRoleView = historizationRoleView; _sharesHistorizationScopeWithLocal = sharesHistorizationScopeWithLocal; _telemetryHub = telemetryHub; @@ -1110,6 +1154,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS Receive(ForwardBatchToMux); Receive(ForwardNativeAlarm); Receive(OnDriverConnectivityChanged); + Receive(HandleCommsRecoveryAudit); Receive(HandleDeltaApplied); Receive(HandleApplyResult); Receive(HandleRestartDriver); @@ -1167,6 +1212,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS Receive(ForwardBatchToMux); Receive(ForwardNativeAlarm); Receive(OnDriverConnectivityChanged); + Receive(HandleCommsRecoveryAudit); Receive(HandleDeltaApplied); Receive(HandleApplyResult); Receive(HandleRestartDriver); @@ -1209,6 +1255,15 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // One envelope per burst; the mux filters per item (it registered a batch receive in Task 9). _dependencyMux?.Tell(msg); + // 03/S15 recovery audit: clear whatever this burst re-published from the driver's Bad latch. Guarded + // by ONE dictionary probe per burst — with no outage outstanding (the steady state) this is a single + // miss, not per-item work. + if (_commsLatched.ContainsKey(msg.DriverInstanceId)) + { + for (var i = 0; i < msg.Items.Count; i++) + TrackCommsLatchPrune(msg.DriverInstanceId, msg.Items[i].FullReference); + } + if (_opcUaPublishActor is null) return; // Indexed for-loop, not foreach: IReadOnlyList enumerates through an interface @@ -1285,13 +1340,34 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// the source had died. closes that by pushing a /// synthetic Bad for every value ref the driver owns. /// + /// + /// Archreview 03/S15 — and the EXTERNAL data plane, which the two layers above still left + /// out. Conditions and internal consumers learned of the outage; the driver's raw value + /// variables and their UNS mirrors — what an OPC UA client actually subscribes to — did not, so + /// they served the last value at Good for the whole outage. + /// closes that with one status-only batch that keeps each + /// node's value and changes only its status. All three fan-outs run on the disconnect edge only; + /// the reconnect edge writes nothing to values (recovery is the driver's own re-publish) and + /// re-annotates conditions Good, which is a statement about the LINK, not about any tag's value. + /// /// private void OnDriverConnectivityChanged(DriverInstanceActor.ConnectivityChanged msg) { // #481: the mux fan-out is independent of the OPC UA sink — a node with scripted alarms but no // publish actor (the mux-only test/dev path) must still see comms loss. Done before the early // return for exactly that reason. - if (!msg.Connected) FanCommsLossToDependencyMux(msg.DriverInstanceId); + if (!msg.Connected) + { + FanCommsLossToDependencyMux(msg.DriverInstanceId); + // 03/S15: and the EXTERNAL data plane, which #481 stopped one seam short of. + FanCommsLossToValueNodes(msg.DriverInstanceId); + } + else + { + // 03/S15: nothing Good is published here — instead, check LATER whether the driver republished + // what this host marked Bad. See HandleCommsRecoveryAudit. + TrackCommsLatchArmAudit(msg.DriverInstanceId); + } if (_opcUaPublishActor is null) return; @@ -1323,6 +1399,160 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS _localNode, msg.DriverInstanceId, msg.Connected ? "connected" : "disconnected", annotated, statusCode); } + /// + /// Archreview 03/S15 — the value-node half of the comms-loss fan-out: stamp + /// on every raw NodeId and every referencing UNS + /// NodeId the just-disconnected driver owns, as ONE status batch through the same publish actor the + /// value fan-out uses. + /// + /// + /// + /// Why it was missing. #481 built the per-driver ref index and wired comms loss to the + /// internal consumers (the mux) and to Part 9 conditions, and stopped one seam short of the + /// external OPC UA data plane. A client subscribed to a value node therefore saw the last value + /// at Good quality for the whole outage, for every driver whose failure shape is "the + /// reader stops producing" rather than "publish a Bad snapshot per tag". (Modbus does the + /// latter, quickly, which is why the gap looked smaller than it was — a coalesced-read throw, a + /// dropped session on a push driver, or simply a driver whose last act was a Good publish all + /// held Good indefinitely.) + /// + /// + /// Status-only, so the VALUE is retained. This actor holds no value cache — v3's + /// single-source design deliberately removed the independent buffer — so only the node can keep + /// the last known value, which is what + /// exists for. A Bad node still + /// carrying its last reading is OPC UA Part 8's convention and is what a SCADA consumer expects. + /// + /// + /// Nothing is written on reconnect. Publishing Good would assert a quality this actor has + /// not observed; recovery is the driver's own re-publish (see + /// 's remark on the detach that makes the poll engine + /// force-raise, and MQTT's initial push). + /// + /// + /// Ungated by redundancy role, like the condition annotation: a Secondary keeps its address + /// space honest for failover. Re-stamping an already-Bad node is harmless — the SDK's + /// StatusValue trigger dedups an unchanged status at the monitored item. + /// + /// + /// The driver child that just reported comms loss. + private void FanCommsLossToValueNodes(string driverInstanceId) + { + if (_opcUaPublishActor is null) return; + if (!_valueRefsByDriver.TryGetValue(driverInstanceId, out var entries) || entries.Count == 0) return; + + var ts = DateTime.UtcNow; + var writes = new List(entries.Count); + foreach (var (_, nodes) in entries) + { + foreach (var n in nodes) + { + writes.Add(new StatusWrite(n.NodeId, OpcUaStatus.BadNoCommunication, ts, n.Realm)); + } + } + + if (writes.Count == 0) return; + + _opcUaPublishActor.Tell(new OpcUa.OpcUaPublishActor.AttributeStatusBatchUpdate(writes)); + TrackCommsLatchSet(driverInstanceId, entries); + + _log.Info( + "DriverHost {Node}: driver {Driver} disconnected — {Count} value node(s) marked BadNoCommunication (value retained)", + _localNode, driverInstanceId, writes.Count); + } + + /// + /// Records every RawPath just latched BadNoCommunication for + /// (03/S15's recovery audit — see ). Replaces any previous latch wholesale: + /// a second outage re-arms from the current ref set rather than accumulating refs a redeploy removed. + /// + /// The driver that just went comms-lost. + /// Its value refs, from the per-driver index. + private void TrackCommsLatchSet( + string driverInstanceId, + List<(string RawPath, HashSet Nodes)> entries) + { + var latched = new HashSet(entries.Count, StringComparer.Ordinal); + foreach (var (rawPath, _) in entries) latched.Add(rawPath); + _commsLatched[driverInstanceId] = latched; + + // A previous reconnect's audit is now meaningless — this driver is down again. + Timers.Cancel(CommsAuditTimerKey(driverInstanceId)); + } + + /// + /// Clears from 's latch, and drops + /// the driver's entry once it empties. Called per routed value item, so the steady-state cost with no + /// latch outstanding is one dictionary miss per burst — not per item. + /// + /// The publishing driver. + /// The RawPath that just published a value. + private void TrackCommsLatchPrune(string driverInstanceId, string fullReference) + { + if (!_commsLatched.TryGetValue(driverInstanceId, out var latched)) return; + if (!latched.Remove(fullReference)) return; + if (latched.Count == 0) _commsLatched.Remove(driverInstanceId); + } + + /// + /// Arms the recovery audit for a driver that has just reconnected: after + /// , reports whatever is + /// still latched. Nothing is armed when the driver has no latch — it never went Bad here. + /// + /// The driver that just reconnected. + private void TrackCommsLatchArmAudit(string driverInstanceId) + { + if (!_commsLatched.ContainsKey(driverInstanceId)) return; + Timers.StartSingleTimer( + CommsAuditTimerKey(driverInstanceId), + new CommsRecoveryAudit(driverInstanceId), + _commsRecoveryAuditDelay); + } + + private static string CommsAuditTimerKey(string driverInstanceId) => $"comms-audit:{driverInstanceId}"; + + /// + /// The 03/S15 tripwire firing: refs this host latched BadNoCommunication that the driver has + /// still not re-published after it reconnected. + /// + /// + /// + /// It reports, it does not repair. Writing Good here would assert a quality nothing has + /// observed — the very thing this whole path refuses to do. What the operator gets instead is a + /// named driver and a count, plus the otopcua.driver.comms_latched_refs counter, which is + /// the operator-visible signature of a driver that reconnects without re-publishing (the + /// pre-STAB-18 MQTT shape, or any future push driver that forgets its initial push). + /// + /// + /// The latch is cleared afterwards either way: it has been reported, and leaving it would make + /// the next outage's audit report refs from this one. + /// + /// + /// Names the driver whose audit is due. + private void HandleCommsRecoveryAudit(CommsRecoveryAudit msg) + { + if (!_commsLatched.TryGetValue(msg.DriverInstanceId, out var latched) || latched.Count == 0) + { + _commsLatched.Remove(msg.DriverInstanceId); + return; + } + + var driverType = _children.TryGetValue(msg.DriverInstanceId, out var child) ? child.DriverType : "unknown"; + OtOpcUaTelemetry.DriverCommsLatchedRefs.Add( + latched.Count, + new KeyValuePair("driver", msg.DriverInstanceId), + new KeyValuePair("driver_type", driverType)); + + _log.Warning( + "DriverHost {Node}: driver {Driver} ({Type}) — {Count} ref(s) still BadNoCommunication {Delay} after reconnect; " + + "the driver did not re-publish them (missing initial push, or a value-only dedup swallowed an unchanged value). " + + "First: {First}", + _localNode, msg.DriverInstanceId, driverType, latched.Count, _commsRecoveryAuditDelay, + latched.OrderBy(r => r, StringComparer.Ordinal).First()); + + _commsLatched.Remove(msg.DriverInstanceId); + } + /// /// #481 (Layer 4) — push a synthetic into the dependency /// mux for every value ref the just-disconnected driver owns, so scripted alarms and virtual tags see @@ -1778,6 +2008,9 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // A child connectivity transition while the host is Stale has no live address space to annotate — drop // it (the post-recovery rebuild re-materialises conditions, and the child re-announces on its next entry). Receive(_ => { }); + // Likewise the 03/S15 recovery audit: a timer armed before the host went Stale has nothing to report + // on (no latch was set while Stale, and the rebuild re-establishes every node's status anyway). + Receive(_ => { }); // A late DeltaApplied (an apply completed just before the DB went Stale) — re-register the driver's // mux adapter anyway; it simply re-reads the driver's current refs (harmless, no DB access). Receive(HandleDeltaApplied); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs index 5fca64f5..062fe6e2 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs @@ -1423,6 +1423,21 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers /// protected override void PostStop() { + // Archreview 03/S15 — announce comms loss on the way out. Every OTHER route into Reconnecting + // (watchdog trip, failed initial connect, Force Reconnect) announces it from the Reconnecting + // entry; stopping did not, so an operator's Restart and a supervision respawn tore this child down + // silently and its value nodes went on serving their last value at Good across the gap. The fresh + // child's re-subscribe + initial push (03/S16) is what clears them again; on a deploy that REMOVES + // the driver the host has already rebuilt its ref index without it, so the Tell finds nothing. + // + // Gated on having actually reached Connected: a child that never connected leaves its nodes at + // BadWaitingForInitialData, which says something more precise than "communication lost", and + // overwriting it would claim an outage that never happened. + if (_connectedSinceUtc != default) + { + Context.Parent.Tell(new ConnectivityChanged(_driverInstanceId, Connected: false)); + } + DetachSubscription(); // MUST happen: the IDriver instance can outlive this actor (the host respawns a child around the // same driver object), so a missing unsubscribe accumulates a handler per respawn holding a dead Self. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs index f0c9a193..b7f8b2af 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs @@ -56,6 +56,15 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers /// their realm each (); status codes ride verbatim (#519). public sealed record AttributeValueBatchUpdate(IReadOnlyList Items); + /// Status-only batch: every named node keeps its CURRENT value and only its status + + /// timestamp change (archreview 03/S15). The comms-loss fan-out sends one of these covering every + /// NodeId a disconnected driver owns — the raw node and each referencing UNS node — so a client sees + /// over the last known value instead of a stale Good. + /// Deliberately a separate message from rather than a + /// flag on it: the host holds no value cache (v3's single-source design), so it has nothing to put in + /// a , and only the node knows what value to retain. + public sealed record AttributeStatusBatchUpdate(IReadOnlyList Items); + /// Carries the full Part 9 condition state for a scripted alarm to the sink. The /// snapshot is the Commons projection the Runtime host maps from the engine's /// Core AlarmConditionState + severity/message — the actor stays decoupled from @@ -292,6 +301,7 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers Receive(HandleAttributeUpdate); Receive(HandleAttributeBatchUpdate); + Receive(HandleAttributeStatusBatchUpdate); Receive(HandleAlarmUpdate); Receive(HandleAlarmQualityUpdate); Receive(HandleRebuild); @@ -368,6 +378,36 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers } } + /// Routes a status-only batch to — the + /// comms-loss annotation (03/S15). Same shape as : metered per + /// ITEM (batching is a transport change, so dashboards must not drop by the batch factor), a + /// is a partial failure that still counts what landed and names + /// the offender, and any other throw is swallowed so a sink fault cannot restart the publish actor and + /// drop live traffic. Metered under kind:"status" so a status annotation is separable from a + /// value write on the same counter. + private void HandleAttributeStatusBatchUpdate(AttributeStatusBatchUpdate msg) + { + try + { + _sink.WriteStatuses(msg.Items); + Interlocked.Add(ref _writes, msg.Items.Count); + OtOpcUaTelemetry.OpcUaSinkWrite.Add(msg.Items.Count, new KeyValuePair("kind", "status")); + } + catch (ValueWriteBatchException ex) + { + _log.Warning(ex, + "OpcUaPublish: sink.WriteStatuses partially failed for a {Count}-item batch — {Failed} failed " + + "(first: {Node}), {Succeeded} applied", + msg.Items.Count, ex.FailedCount, ex.FirstFailedNodeId, ex.SucceededCount); + Interlocked.Add(ref _writes, ex.SucceededCount); + OtOpcUaTelemetry.OpcUaSinkWrite.Add(ex.SucceededCount, new KeyValuePair("kind", "status")); + } + catch (Exception ex) + { + _log.Warning(ex, "OpcUaPublish: sink.WriteStatuses threw for a {Count}-item batch", msg.Items.Count); + } + } + private void HandleAlarmUpdate(AlarmStateUpdate msg) { try diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs index a33bc254..7a41c587 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs @@ -66,10 +66,23 @@ public sealed class DependencyConsumerMuxAdapter : ReceiveActor _log.Debug("DependencyConsumerMuxAdapter: registered {Count} dependency ref(s)", refs.Count); } + /// + /// Forwards one dependency value to the consumer, status code included (archreview + /// 05/STAB-19). + /// + /// + /// This used to pass a hard-coded 0u behind a comment saying quality was stripped at the + /// mux boundary. That was true when the adapter was written and stopped being true at #478/#519 — + /// has carried the source driver's full status + /// code, sub-code and all, ever since. The stale comment outliving the fact is what kept a + /// comms-lost input arriving at the Calculation driver as arrived, Good, null: + /// indistinguishable from a genuine Good null, so the warm-up gate was satisfied by a value that + /// never arrived and the calc output stayed Good over a dead source. + /// + /// The dependency value, as the mux delivered it. private void OnDependencyValueChanged(VirtualTagActor.DependencyValueChanged msg) { - // Quality is stripped at the mux boundary (value + timestamp only), so forward Good (0). - _consumer.OnDependencyValue(msg.TagId, msg.Value, statusCode: 0u, msg.TimestampUtc); + _consumer.OnDependencyValue(msg.TagId, msg.Value, msg.StatusCode, msg.TimestampUtc); } /// The mux's batched delivery: one message carrying this adapter's whole matched slice of a diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs index 5520c282..c2c7a8fd 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs @@ -6,6 +6,7 @@ using ZB.MOM.WW.OtOpcUa.Commons.Messages.Logging; using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Runtime.Telemetry; namespace ZB.MOM.WW.OtOpcUa.Runtime.VirtualTags; @@ -31,9 +32,13 @@ public sealed class VirtualTagActor : ReceiveActor public const string ScriptLogsTopic = "script-logs"; /// A dependency's value changed. (#478) carries the source - /// driver's OPC UA status code so the scripted-alarm host can derive a condition's worst-of-input - /// quality; it defaults to , and the virtual-tag engine ignores it. - /// Since #519 it carries the driver's sub-code, so a worst-of-input winner keeps the reason it won. + /// driver's OPC UA status code — since #519 including its sub-code, so a worst-of-input winner keeps + /// the reason it won — and it defaults to . + /// The virtual-tag engine consumes it (archreview 02/S14). This doc used to say the + /// engine ignored it, which was true and became the defect: a Bad input was stored as a bare value, + /// the script ran against it, and the tag published a fresh-Good result over a dead source. The actor + /// now holds evaluation while any declared input is Bad and inherits the worst input's status + /// otherwise. The Calculation driver reads it through the same mux (05/STAB-19). public sealed record DependencyValueChanged( string TagId, object? Value, DateTime TimestampUtc, uint StatusCode = OpcUaStatus.Good); @@ -72,13 +77,22 @@ public sealed class VirtualTagActor : ReceiveActor private readonly IActorRef? _mux; private readonly ITelemetryLocalHub? _telemetryHub; private readonly ILoggingAdapter _log = Context.GetLogger(); - private readonly Dictionary _dependencies = new(StringComparer.Ordinal); + // Dependency SNAPSHOTS, not bare values (archreview 02/S14): the status code is what distinguishes a + // comms-lost input from a genuine Good null, and it is what the script reads through + // ctx.GetTag(path).StatusCode. + private readonly Dictionary _dependencies = new(StringComparer.Ordinal); private bool _hasLastValue; private object? _lastValue; - // 02/S13: true once a Bad EvaluationResult has been published and not yet cleared by a Good - // recovery. Gates Bad publishes to once-per-transition and force-publishes the recovery value. - private bool _lastPublishedBad; + + /// Whether anything has been published for this tag yet — distinct from + /// , because an inputs-Bad publish carries no value of its own. + private bool _hasPublished; + + /// The status last published. Replaces 02/S13's two-state _lastPublishedBad flag + /// (02/S14): the dedup key is (value, status), a Bad may now carry the failing INPUT's sub-code rather + /// than a fixed BadInternalError, and a reassert re-emits this rather than guessing Good. + private uint _lastPublishedStatus = OpcUaStatus.Good; /// Factory method to create Props for a VirtualTagActor. /// Unique identifier for the virtual tag. @@ -178,15 +192,17 @@ public sealed class VirtualTagActor : ReceiveActor /// private void OnReassertValue() { - if (!_hasLastValue && !_lastPublishedBad) return; - var statusCode = _lastPublishedBad ? OpcUaStatus.BadInternalError : OpcUaStatus.Good; + if (!_hasLastValue && !_hasPublished) return; + // The status LAST PUBLISHED, not a guess (02/S14). A VT whose input is comms-lost re-asserts + // BadNoCommunication onto the freshly-materialised node; re-asserting Good here would put the + // stale-Good lie back at every deploy. Context.Parent.Tell(new EvaluationResult( - _virtualTagId, _lastValue, DateTime.UtcNow, CorrelationId.NewId(), statusCode, IsReassert: true)); + _virtualTagId, _lastValue, DateTime.UtcNow, CorrelationId.NewId(), _lastPublishedStatus, IsReassert: true)); } private void OnDependencyChanged(DependencyValueChanged msg) { - _dependencies[msg.TagId] = msg.Value; + _dependencies[msg.TagId] = new DataValueSnapshot(msg.Value, msg.StatusCode, msg.TimestampUtc, msg.TimestampUtc); Evaluate(msg.TimestampUtc); } @@ -210,7 +226,8 @@ public sealed class VirtualTagActor : ReceiveActor for (var i = 0; i < count; i++) { var item = items[i]; - _dependencies[item.TagId] = item.Value; + _dependencies[item.TagId] = + new DataValueSnapshot(item.Value, item.StatusCode, item.TimestampUtc, item.TimestampUtc); } Evaluate(items[count - 1].TimestampUtc); @@ -222,6 +239,22 @@ public sealed class VirtualTagActor : ReceiveActor /// The triggering dependency change's timestamp, stamped on the result. private void Evaluate(DateTime timestampUtc) { + // Worst-of-input FIRST (02/S14), before the evaluator is even considered. + // + // Over the deps that have ARRIVED, deliberately — a never-pushed dep is cold start, not a fault, + // and it is already handled by the inputs-ready gate in the two publish tails (which suppress the + // warm-up flash). Counting it as Bad here would instead HOLD evaluation during warm-up, which is + // a behaviour change this finding does not ask for: a partially-warmed-up VT evaluates today and + // must keep doing so, or every Good-path assertion in the existing suites changes meaning. Same + // skip ScriptedAlarmEngine.WorstInputStatus has always made. + var worst = InputQuality.WorstOf( + _dependencyRefs.Select(r => _dependencies.GetValueOrDefault(r)).Where(s => s is not null)); + if (!InputQuality.IsUsable(worst)) + { + OnInputsBad(worst, timestampUtc); + return; + } + VirtualTagEvalResult result; try { @@ -249,9 +282,10 @@ public sealed class VirtualTagActor : ReceiveActor return; } - // Value dedup — but bypass it while recovering from Bad, so a recovery whose value equals the - // pre-failure value still republishes Good (otherwise the node would stay Bad forever). - if (!_lastPublishedBad && _hasLastValue && Equals(_lastValue, result.Value)) + // Dedup on (value, STATUS) — 02/S14 generalising 02/S13's recover-from-Bad bypass. Keying on + // value alone swallows every recovery whose value is unchanged, which is what a device coming + // back usually produces, and the node then stays Bad forever. + if (_hasPublished && _lastPublishedStatus == worst && _hasLastValue && Equals(_lastValue, result.Value)) { OtOpcUaTelemetry.VirtualTagEval.Add(1, new KeyValuePair("outcome", "skip")); return; @@ -259,12 +293,55 @@ public sealed class VirtualTagActor : ReceiveActor _hasLastValue = true; _lastValue = result.Value; - _lastPublishedBad = false; + _hasPublished = true; + // The output inherits the worst input's quality: Good, or an Uncertain sub-code (Bad never + // reaches here — it was held above). + _lastPublishedStatus = worst; OtOpcUaTelemetry.VirtualTagEval.Add(1, new KeyValuePair("outcome", "ok")); - var evalResult = new EvaluationResult(_virtualTagId, result.Value, timestampUtc, CorrelationId.NewId()); + var evalResult = new EvaluationResult(_virtualTagId, result.Value, timestampUtc, CorrelationId.NewId(), worst); Context.Parent.Tell(evalResult); } + /// + /// 02/S14 — this tag's INPUTS are Bad, so the script is not run and the tag reports the worst + /// input's status, carrying its last known value. + /// + /// + /// + /// Holding rather than evaluating-and-degrading is what removes the + /// (double)ctx.GetTag(x).Value NRE storm at source: a comms-lost input used to arrive as + /// Good/null and the script ran against it on every trigger. It is also the alarm engine's + /// long-standing policy (AreInputsReady), so all three script seams now agree. + /// + /// + /// No script-log entry, deliberately: an input dying is not a script fault, and the + /// script log is 02/S13's per-failure SCRIPT diagnostic. Here the status code is the + /// diagnostic, and it names which failure the operator is looking at. + /// + /// + /// Published once per change of the published status, and only once every declared + /// dependency has arrived — R2-03's inputs-ready gate, kept as-is, so a cold start or an actor + /// respawn does not flash Bad while the mux is still filling the map. + /// + /// + /// The worst status among this tag's declared dependencies. + /// The triggering change's timestamp, stamped on the result. + private void OnInputsBad(uint worstInputStatus, DateTime timestampUtc) + { + if (!AllDependenciesArrived()) return; + if (_hasPublished && _lastPublishedStatus == worstInputStatus) return; + + // Metered BELOW both guards, so the counter measures tags that actually REPORT Bad inputs. Above + // them it also counted every warm-up trigger — a dependency that has simply not arrived yet — which + // is ordinary cold start, and burying the real signal in it is how a counter stops being trusted. + OtOpcUaTelemetry.VirtualTagEval.Add(1, new KeyValuePair("outcome", "inputs-bad")); + + _hasPublished = true; + _lastPublishedStatus = worstInputStatus; + Context.Parent.Tell(new EvaluationResult( + _virtualTagId, _hasLastValue ? _lastValue : null, timestampUtc, CorrelationId.NewId(), worstInputStatus)); + } + /// 02/S13 failure tail shared by the evaluator-threw catch and the !Success branch. /// Always meters + publishes the per-failure script-log entry (the pre-existing operator /// diagnostic); additionally publishes a Bad — carrying the @@ -278,12 +355,13 @@ public sealed class VirtualTagActor : ReceiveActor OtOpcUaTelemetry.VirtualTagEval.Add(1, new KeyValuePair("outcome", "fail")); PublishLog(level, reason); - if (_lastPublishedBad || !AllDependenciesArrived()) + if ((_hasPublished && _lastPublishedStatus == OpcUaStatus.BadInternalError) || !AllDependenciesArrived()) { return; } - _lastPublishedBad = true; + _hasPublished = true; + _lastPublishedStatus = OpcUaStatus.BadInternalError; Context.Parent.Tell(new EvaluationResult( _virtualTagId, _hasLastValue ? _lastValue : null, timestampUtc, CorrelationId.NewId(), OpcUaStatus.BadInternalError)); } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs new file mode 100644 index 00000000..117a144e --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs @@ -0,0 +1,97 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests; + +/// +/// Truth table for — the one worst-of-input rule every derived-value +/// seam uses (scripted alarms, virtual tags, the Calculation driver). The quality contract +/// (archreview R3-02) is "a value consumer never reports a quality better than its source's", +/// and this type is the single place that decides what "worse" means, so a divergence here is a +/// divergence in every seam at once. +/// +public sealed class InputQualityTests +{ + private const uint Good = 0x00000000u; + private const uint Uncertain = 0x40000000u; + private const uint UncertainLastUsableValue = 0x40900000u; + private const uint BadNoCommunication = 0x80310000u; + private const uint BadInternalError = 0x80020000u; + private const uint BadWaitingForInitialData = 0x80320000u; + + private static DataValueSnapshot Snap(object? value, uint status) => + new(value, status, SourceTimestampUtc: DateTime.UtcNow, ServerTimestampUtc: DateTime.UtcNow); + + /// An empty input set reports Good — there is nothing to be worse than. + [Fact] + public void WorstOf_no_inputs_is_Good() + { + InputQuality.WorstOf([]).ShouldBe(Good); + } + + /// All-Good inputs report Good. + [Fact] + public void WorstOf_all_good_is_Good() + { + InputQuality.WorstOf([Snap(1, Good), Snap(2, Good)]).ShouldBe(Good); + } + + /// A single Uncertain input carries its own sub-code out, not the generic Uncertain. + [Fact] + public void WorstOf_propagates_the_uncertain_sub_code() + { + InputQuality.WorstOf([Snap(1, Good), Snap(2, UncertainLastUsableValue)]) + .ShouldBe(UncertainLastUsableValue); + } + + /// Bad beats Uncertain regardless of the order the inputs are seen in. + [Theory] + [InlineData(true)] + [InlineData(false)] + public void WorstOf_bad_beats_uncertain(bool badFirst) + { + DataValueSnapshot?[] inputs = badFirst + ? [Snap(null, BadNoCommunication), Snap(2, Uncertain)] + : [Snap(2, Uncertain), Snap(null, BadNoCommunication)]; + + InputQuality.WorstOf(inputs).ShouldBe(BadNoCommunication); + } + + /// + /// A null entry means "this dependency has never been pushed", which is + /// BadWaitingForInitialData — the same code the materialiser stamps on a fresh node. + /// Reporting Good for it would be the exact laundering this helper exists to stop. + /// + [Fact] + public void WorstOf_a_null_entry_is_BadWaitingForInitialData() + { + InputQuality.WorstOf([Snap(1, Good), null]).ShouldBe(BadWaitingForInitialData); + } + + /// + /// Among equal severity the FIRST seen wins, so a re-evaluation over the same inputs reports + /// the same sub-code rather than oscillating between two equally-bad codes. + /// + [Fact] + public void WorstOf_equal_severity_keeps_the_first_seen() + { + InputQuality.WorstOf([Snap(null, BadNoCommunication), Snap(null, BadInternalError)]) + .ShouldBe(BadNoCommunication); + InputQuality.WorstOf([Snap(null, BadInternalError), Snap(null, BadNoCommunication)]) + .ShouldBe(BadInternalError); + } + + /// Good and Uncertain are usable (a script may still inspect the value); Bad is not. + [Theory] + [InlineData(Good, true)] + [InlineData(Uncertain, true)] + [InlineData(UncertainLastUsableValue, true)] + [InlineData(BadNoCommunication, false)] + [InlineData(BadWaitingForInitialData, false)] + [InlineData(0xC0000000u, false)] + public void IsUsable_follows_Part4_severity(uint statusCode, bool expected) + { + InputQuality.IsUsable(statusCode).ShouldBe(expected); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverInputQualityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverInputQualityTests.cs new file mode 100644 index 00000000..73f6f012 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverInputQualityTests.cs @@ -0,0 +1,316 @@ +using System.Collections.Concurrent; +using Serilog; +using Serilog.Events; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Logging; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Scripting; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests; + +/// +/// Archreview 05/STAB-19 — what the Calculation driver does with a dependency's quality. +/// +/// The defect. The driver stored dependency values only and ignored the status code +/// entirely, so a comms-lost input arrived as arrived, Good, null: the warm-up gate was +/// satisfied by a value that never came, (double)ctx.GetTag("x").Value threw on every trigger +/// (a "script threw" storm), and a null-tolerant script published a Good output computed from a +/// dead input — indefinitely. +/// +/// The policy pinned here matches ScriptedAlarmEngine's, which is the in-tree +/// precedent: hold on Bad, propagate the worst input's status; evaluate through Uncertain and stamp +/// the output with it. Holding (rather than evaluating and degrading) is what removes the NRE +/// storm; a Bad sub-code (rather than Uncertain) is what tells the operator the value may be +/// arbitrarily old rather than merely imprecise. +/// +/// ⚠️ Vacuity rule: the recovery case re-publishes an identical computed value. +/// A recovery whose value differs passes against a value-only dedup and proves nothing — it is the +/// "Bad forever" trap the v3 Batch 4 live gate found. +/// +public sealed class CalculationDriverInputQualityTests +{ + private const uint StatusGood = 0u; + private const uint StatusUncertain = 0x40000000u; + private const uint StatusBadNoCommunication = 0x80310000u; + private const uint StatusBadInternalError = 0x80020000u; + private const uint StatusBadWaitingForInitialData = 0x80320000u; + + private const string SumTag = "calc/e/sum"; + private const string SumScript = "return (double)ctx.GetTag(\"src/a\").Value + (double)ctx.GetTag(\"src/b\").Value;"; + + private static RawTagEntry CalcTag(string rawPath, string source) + { + var json = $"{{\"scriptId\":\"s-{rawPath}\",\"changeTriggered\":true," + + $"\"scriptSource\":{System.Text.Json.JsonSerializer.Serialize(source)}}}"; + return new RawTagEntry(rawPath, json, WriteIdempotent: false); + } + + private static (CalculationDriver Driver, ConcurrentQueue Published, List Logs) + Build(params RawTagEntry[] tags) + { + var logs = new List(); + var sink = new ScriptLogTopicSink(new CapturingPublisher(logs), LogEventLevel.Information); + var scriptRoot = new ScriptRootLogger(new LoggerConfiguration().WriteTo.Sink(sink).CreateLogger()); + var driver = new CalculationDriver(new CalculationDriverOptions { RawTags = tags }, "calc-drv", scriptRoot); + var published = new ConcurrentQueue(); + driver.OnDataChange += (_, e) => published.Enqueue(e); + return (driver, published, logs); + } + + /// Local copy of the script-log capture sink (the sibling suite's is private to it). + private sealed class CapturingPublisher(List sink) : IScriptLogPublisher + { + private readonly object _lock = new(); + + public void Publish(ScriptLogEntry entry) + { + lock (_lock) sink.Add(entry); + } + } + + private static async Task Settle(CalculationDriver driver) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10); + while (DateTime.UtcNow < deadline) + { + if (driver.IsEvaluationIdle) return; + await Task.Delay(10); + } + + driver.IsEvaluationIdle.ShouldBeTrue("the calc driver's evaluation pump should have drained"); + } + + /// Drives both deps Good and drains the resulting publish, leaving a settled Good output. + private static async Task WarmedUpAsync(ConcurrentQueue published, CalculationDriver driver) + { + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + driver.OnDependencyValue("src/a", 10.0, StatusGood, DateTime.UtcNow); + driver.OnDependencyValue("src/b", 20.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var first).ShouldBeTrue(); + first!.Snapshot.Value.ShouldBe(30.0); + first.Snapshot.StatusCode.ShouldBe(StatusGood); + return driver; + } + + /// + /// A Bad input holds the script and publishes the input's own sub-code, once, carrying the last + /// known value. Three things are asserted together because each is a separate way the old + /// behaviour was wrong: no evaluation (no NRE storm), the sub-code propagated (not a generic + /// BadInternalError, which would blame the script), and no script-log entry (an input + /// dying is not a script fault). + /// + [Fact] + public async Task A_bad_input_holds_evaluation_and_publishes_the_inputs_own_sub_code_once() + { + var (driver, published, logs) = Build(CalcTag(SumTag, SumScript)); + await WarmedUpAsync(published, driver); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var bad).ShouldBeTrue(); + bad!.Snapshot.StatusCode.ShouldBe(StatusBadNoCommunication); + bad.Snapshot.StatusCode.ShouldNotBe(StatusBadInternalError, "the script did not fail — its input did"); + bad.Snapshot.Value.ShouldBe(30.0, "the last known value rides out with the Bad status"); + logs.ShouldBeEmpty("a dead input is not a script fault; the script log stays the per-failure diagnostic"); + + // Transition-once: a second Bad push of the same code changes nothing observable. + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + published.ShouldBeEmpty(); + } + + /// + /// A different Bad sub-code IS republished: the sub-code is the diagnostic, and masking + /// BadNoCommunication behind an earlier BadWaitingForInitialData (or vice versa) + /// hides which input died and why. + /// + [Fact] + public async Task A_different_bad_sub_code_is_republished() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await WarmedUpAsync(published, driver); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + published.TryDequeue(out _).ShouldBeTrue(); + + driver.OnDependencyValue("src/a", null, StatusBadInternalError, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var second).ShouldBeTrue(); + second!.Snapshot.StatusCode.ShouldBe(StatusBadInternalError); + } + + /// + /// The vacuity-sensitive assertion. The input recovers with the value it had before the + /// outage, so the computed output is identical to the pre-outage one — and it must still be + /// republished Good, because the node is currently Bad and only a publish can clear it. A + /// value-only dedup swallows exactly this case, which is why a recovery test with a changed value + /// is not the guard. + /// + [Fact] + public async Task An_identical_value_is_republished_when_recovering_from_a_bad_input() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await WarmedUpAsync(published, driver); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + published.TryDequeue(out _).ShouldBeTrue(); + + // The device comes back holding the same reading it had before. + driver.OnDependencyValue("src/a", 10.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var recovered).ShouldBeTrue(); + recovered!.Snapshot.Value.ShouldBe(30.0, "the same value as before the outage — that is the point"); + recovered.Snapshot.StatusCode.ShouldBe(StatusGood); + } + + /// + /// An Uncertain input is still computable, so the script runs and the output inherits the + /// Uncertain code — the alarm engine's rule (Uncertain is "ready"). Holding here would take a + /// perfectly good arithmetic result hostage. + /// + [Fact] + public async Task An_uncertain_input_is_evaluated_and_stamps_the_output_uncertain() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await WarmedUpAsync(published, driver); + + driver.OnDependencyValue("src/a", 11.0, StatusUncertain, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var uncertain).ShouldBeTrue(); + uncertain!.Snapshot.Value.ShouldBe(31.0, "the script DID run — Uncertain is usable"); + uncertain.Snapshot.StatusCode.ShouldBe(StatusUncertain); + + // …and returning to Good republishes, even though the computed value changes back to a value the + // tag has held before. + driver.OnDependencyValue("src/a", 10.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + published.TryDequeue(out var good).ShouldBeTrue(); + good!.Snapshot.StatusCode.ShouldBe(StatusGood); + good.Snapshot.Value.ShouldBe(30.0); + } + + /// + /// Cold start is unchanged: with one dep never pushed, a Bad arrival on the other publishes + /// nothing at all (the node stays at the materialiser's BadWaitingForInitialData). Only once + /// every dep has arrived does a Bad among them become reportable. + /// + [Fact] + public async Task Nothing_is_published_while_a_dep_has_never_arrived() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + published.ShouldBeEmpty("src/b has never arrived — this is warm-up, not a fault"); + + driver.OnDependencyValue("src/b", 20.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var bad).ShouldBeTrue(); + bad!.Snapshot.StatusCode.ShouldBe(StatusBadNoCommunication); + } + + /// + /// A passthrough script over a Bad input must not publish Ok(null) Good — the worst case of + /// the laundering, since the passthrough fast path bypasses the read cache entirely. + /// + [Fact] + public async Task A_passthrough_over_a_bad_input_is_bad_not_good_null() + { + var (driver, published, _) = Build(CalcTag("calc/e/pass", "return ctx.GetTag(\"src/a\").Value;")); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + driver.OnDependencyValue("src/a", 42.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + published.TryDequeue(out var good).ShouldBeTrue(); + good!.Snapshot.Value.ShouldBe(42.0); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var bad).ShouldBeTrue(); + bad!.Snapshot.StatusCode.ShouldBe(StatusBadNoCommunication); + bad.Snapshot.StatusCode.ShouldNotBe(StatusGood); + } + + /// + /// ReadAsync reports what the subscriber was told. A read that answered Good while the + /// subscription carried Bad would leave two clients of the same node disagreeing about it. + /// + [Fact] + public async Task ReadAsync_reports_the_same_status_the_subscriber_saw() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await WarmedUpAsync(published, driver); + + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + await Settle(driver); + published.TryDequeue(out var bad).ShouldBeTrue(); + + var read = await driver.ReadAsync([SumTag], TestContext.Current.CancellationToken); + + read[0].StatusCode.ShouldBe(bad!.Snapshot.StatusCode); + read[0].Value.ShouldBe(bad.Snapshot.Value); + } + + /// + /// A tag whose ONLY publish was inputs-Bad — it never produced a value — must read back that Bad + /// status, not BadWaitingForInitialData. Guarding the read on "has a value" rather than + /// "has published" made a polled read say "never polled" while the subscriber on the same node had + /// been told BadNoCommunication: the two clients disagree, and the read is the one that + /// lies (something HAS been observed about this tag — that its input is dead). + /// + [Fact] + public async Task ReadAsync_after_an_inputs_Bad_with_no_prior_value_reports_the_bad_status() + { + var (driver, published, _) = Build(CalcTag(SumTag, SumScript)); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // The first thing that ever happens to this tag is a dead input — no successful evaluation, so + // there is no last value to carry. + driver.OnDependencyValue("src/a", null, StatusBadNoCommunication, DateTime.UtcNow); + driver.OnDependencyValue("src/b", 20.0, StatusGood, DateTime.UtcNow); + await Settle(driver); + + published.Count.ShouldBe(1); + published.TryDequeue(out var bad).ShouldBeTrue(); + bad!.Snapshot.StatusCode.ShouldBe(StatusBadNoCommunication); + bad.Snapshot.Value.ShouldBeNull(); + + var read = await driver.ReadAsync([SumTag], TestContext.Current.CancellationToken); + + read[0].StatusCode.ShouldBe(StatusBadNoCommunication); + read[0].StatusCode.ShouldNotBe(StatusBadWaitingForInitialData, + "something HAS been observed about this tag — that its input is dead"); + read[0].Value.ShouldBeNull(); + } + + /// A never-evaluated tag still reads BadWaitingForInitialData (regression). + [Fact] + public async Task An_unevaluated_tag_still_reads_BadWaitingForInitialData() + { + var (driver, _, _) = Build(CalcTag(SumTag, SumScript)); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var read = await driver.ReadAsync([SumTag], TestContext.Current.CancellationToken); + + read[0].StatusCode.ShouldBe(StatusBadWaitingForInitialData); + } +} 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 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttConnectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttConnectionTests.cs index 315ca370..9311df52 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttConnectionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttConnectionTests.cs @@ -615,6 +615,81 @@ public sealed class MqttConnectionTests blackhole.AcceptedCount.ShouldBe(acceptedAfterTheAttempt); } + // --------------------------------------------------------------------------------- + // ConnectionLost — the quality edge (archreview 05/STAB-18 shape a) + // --------------------------------------------------------------------------------- + + /// + /// fires exactly once per + /// Connected → Reconnecting edge. It is the signal the ingest paths stale their cached + /// values on, so an extra fire is downstream churn and a missing one leaves every tag + /// reporting Good over a dead session. + /// + [Fact] + public async Task BrokerDropsTheConnection_FiresConnectionLostOncePerEdge() + { + using var broker = new MiniBroker(); + await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null); + var losses = 0; + conn.ConnectionLost += () => Interlocked.Increment(ref losses); + + await conn.ConnectAsync(CancellationToken.None); + Volatile.Read(ref losses).ShouldBe(0, "a successful connect is not a loss"); + + broker.DropAll(); + (await WaitUntilAsync(() => Volatile.Read(ref losses) >= 1, TimeSpan.FromSeconds(30))).ShouldBeTrue(); + (await WaitUntilAsync(() => conn.State == MqttConnectionState.Connected, TimeSpan.FromSeconds(30))) + .ShouldBeTrue(); + Volatile.Read(ref losses).ShouldBe(1, "the reconnect itself is not a second loss"); + + broker.DropAll(); + (await WaitUntilAsync(() => Volatile.Read(ref losses) >= 2, TimeSpan.FromSeconds(30))) + .ShouldBeTrue("a second outage is a second edge"); + } + + /// + /// An initial connect that never succeeds is not a loss — there was no session to lose, + /// and staling on it would flip a never-observed tag Bad on a shape the cache already reports + /// as BadWaitingForInitialData. + /// + [Fact] + public async Task ConnectionLost_DoesNotFireOnAnInitialConnectFailure() + { + using var blackhole = new BlackholeBroker(); + var opts = new MqttDriverOptions + { + Host = "127.0.0.1", Port = blackhole.Port, UseTls = false, ConnectTimeoutSeconds = 1, + }; + await using var conn = new MqttConnection(opts, driverId: "t", logger: null); + var losses = 0; + conn.ConnectionLost += () => Interlocked.Increment(ref losses); + + await Should.ThrowAsync(async () => await conn.ConnectAsync(CancellationToken.None)); + await Task.Delay(TimeSpan.FromMilliseconds(500), TestContext.Current.CancellationToken); + + Volatile.Read(ref losses).ShouldBe(0); + } + + /// + /// Dispose tears the client down, which the library reports as a disconnect. That is an + /// intentional teardown, not an outage: firing here would stale every tag on an ordinary + /// redeploy or shutdown. + /// + [Fact] + public async Task ConnectionLost_DoesNotFireOnDispose() + { + using var broker = new MiniBroker(); + var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null); + var losses = 0; + conn.ConnectionLost += () => Interlocked.Increment(ref losses); + + await conn.ConnectAsync(CancellationToken.None); + await conn.DisposeAsync(); + await Task.Delay(TimeSpan.FromMilliseconds(500), TestContext.Current.CancellationToken); + + Volatile.Read(ref losses).ShouldBe(0); + } + // --------------------------------------------------------------------------------- // Task 4 — the reconnect supervisor, against a real (minimal) broker // --------------------------------------------------------------------------------- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverSessionLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverSessionLifecycleTests.cs index a20c1fca..38bdeaa0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverSessionLifecycleTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverSessionLifecycleTests.cs @@ -100,6 +100,79 @@ public sealed class MqttDriverSessionLifecycleTests "the authored 2s budget must still bound the connect, not the 15s default"); } + /// + /// End-to-end through (archreview 05/STAB-18). Deliver a value, + /// let the real broker drop the real session, and the driver publishes + /// BadNoCommunication with the value retained; feed the identical payload back — + /// the retained replay a broker sends on reconnect — and it publishes Good again. + /// + /// + /// + /// What this adds over MqttSubscriptionManagerRecoveryTests: the wiring. Those + /// drive OnConnectionLost() directly, so they stay green even if + /// MqttSubscriptionManager.AttachTo never subscribes to + /// — an unforwarded seam is this repo's + /// best-documented way to ship an inert fix. Here the edge comes from a genuine socket drop + /// through MQTTnet's own DisconnectedAsync. + /// + /// + /// Vacuity rule: the recovery payload is byte-identical to the pre-outage one. A test + /// that replays a different value passes against a build with no stale at all — the + /// value change alone would defeat the suppression — and is not the guard. + /// + /// + /// Ingest is fed through Subscriptions.HandleMessage, the method MQTTnet's dispatcher + /// calls, because answers CONNECT and PINGREQ and nothing else: it + /// can drop a session (which is what this test needs from it) but cannot PUBLISH. + /// + /// + [Fact] + public async Task ABrokerDrop_MarksCachedRefsBadNoCommunication_AndTheIdenticalReplayRecovers() + { + const string rawPath = "Plant/Mqtt/dev1/Temp"; + const string topic = "factory/oven/temp"; + const uint good = 0x00000000u; + const uint badNoCommunication = 0x80310000u; + + using var broker = new MiniBroker(); + var options = BrokerOptions(broker.Port) with + { + RawTags = + [ + new RawTagEntry( + rawPath, + $$"""{"topic":"{{topic}}","payloadFormat":"Json","jsonPath":"$.value","dataType":"Float64"}""", + WriteIdempotent: false), + ], + }; + await using var driver = new MqttDriver(options, "d", null); + await driver.InitializeAsync(Json(options), TestContext.Current.CancellationToken); + await driver.SubscribeAsync([rawPath], TimeSpan.Zero, TestContext.Current.CancellationToken); + + var raised = new List(); + driver.OnDataChange += (_, e) => { lock (raised) raised.Add(e.Snapshot); }; + + driver.Subscriptions.HandleMessage(topic, """{"value":21.5}"""u8.ToArray(), retained: false); + lock (raised) raised.Count.ShouldBe(1, "the first value must publish"); + + broker.DropAll(); + await WaitForAsync( + () => { lock (raised) return raised.Count == 2; }, + "the broker drop to publish a BadNoCommunication snapshot"); + DataValueSnapshot staled; + lock (raised) staled = raised[1]; + staled.StatusCode.ShouldBe(badNoCommunication); + staled.Value.ShouldBe(21.5, "the last known value is retained through the outage"); + + // The reconnect's retained replay — the IDENTICAL payload. + driver.Subscriptions.HandleMessage(topic, """{"value":21.5}"""u8.ToArray(), retained: true); + + DataValueSnapshot recovered; + lock (raised) { raised.Count.ShouldBe(3); recovered = raised[2]; } + recovered.StatusCode.ShouldBe(good); + recovered.Value.ShouldBe(21.5); + } + private static async Task WaitForAsync(Func condition, string what) { var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerRecoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerRecoveryTests.cs new file mode 100644 index 00000000..66328430 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerRecoveryTests.cs @@ -0,0 +1,229 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests; + +/// +/// The push-model edge behaviours lacked (archreview +/// 05/STAB-18): an initial push of the cached snapshot on every (re)subscribe, and a +/// connection-loss stale to BadNoCommunication with the value retained. +/// +/// Why both, and why the identical payload is load-bearing. The manager's steady-state +/// dedup suppresses a message whose value AND status equal the cached one, and that cache is +/// driver-owned — it survives a reconnect and a redeploy. So a broker that replays its retained +/// message unchanged after an outage raised nothing, and every node the host had latched Bad stayed +/// Bad until the plant value happened to change. A recovery test that replays a different +/// payload passes against the pre-fix code and proves nothing; every recovery assertion here +/// therefore replays the identical payload. +/// +/// The steady-state suppression itself must survive — it is what keeps a 10 Hz topic from +/// re-triggering the whole downstream fan-out per message — so each recovery case is paired with a +/// control that asserts an unchanged republish is still suppressed. +/// +public sealed class MqttSubscriptionManagerRecoveryTests +{ + // OPC UA status codes, verified against Opc.Ua.StatusCodes rather than recalled. + private const uint Good = 0x00000000u; + private const uint BadNoCommunication = 0x80310000u; + private const uint BadWaitingForInitialData = 0x80320000u; + + private const string TempPath = "Plant/Mqtt/broker1/OvenTemp"; + private const string PressPath = "Plant/Mqtt/broker1/OvenPressure"; + private const string TempTopic = "f/oven/temp"; + private const string PressTopic = "f/oven/press"; + + private static RawTagEntry Tag(string rawPath, string topic) => + new(rawPath, $$"""{"topic":"{{topic}}","payloadFormat":"Json","jsonPath":"$.value","dataType":"Float64"}""", + WriteIdempotent: false); + + private static MqttSubscriptionManager NewManager() + { + var mgr = new MqttSubscriptionManager(new MqttDriverOptions { Mode = MqttMode.Plain }, "mqtt-1"); + mgr.Register([Tag(TempPath, TempTopic), Tag(PressPath, PressTopic)]); + return mgr; + } + + private sealed record Raised(string Reference, object? Value, uint StatusCode, ISubscriptionHandle Handle); + + private static List Record(MqttSubscriptionManager mgr) + { + var raised = new List(); + mgr.OnDataChange += (_, e) => + raised.Add(new Raised(e.FullReference, e.Snapshot.Value, e.Snapshot.StatusCode, e.SubscriptionHandle)); + return raised; + } + + // --------------------------------------------------------------------------------- + // (A) Initial push on (re)subscribe — 05/STAB-18 shape (b) + // --------------------------------------------------------------------------------- + + /// + /// A ref observed before the subscription existed is pushed once, with its cached snapshot, + /// attributed to the new handle. This is the push-model equivalent of the poll engine's + /// forceRaise: true on the first poll of a new subscription, and it is what refills a + /// node the address-space rebuild reseeded BadWaitingForInitialData on a quiet topic. + /// + [Fact] + public async Task SubscribeAsync_pushes_the_cached_snapshot_for_an_observed_ref() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); // cached; no handle yet + + var raised = Record(mgr); + var handle = await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + raised.Count.ShouldBe(1); + raised[0].Reference.ShouldBe(TempPath); + raised[0].Value.ShouldBe(21.5); + raised[0].StatusCode.ShouldBe(Good); + raised[0].Handle.ShouldBe(handle); + } + + /// + /// A ref that has never been observed carries only the materialiser's + /// BadWaitingForInitialData placeholder — pushing that would claim an observation the + /// driver has not made, so the initial push skips it. + /// + [Fact] + public async Task SubscribeAsync_does_not_push_a_never_observed_ref() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + + var raised = Record(mgr); + await mgr.SubscribeAsync([TempPath, PressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + raised.Select(r => r.Reference).ShouldBe([TempPath]); + mgr.Values.Read(PressPath).StatusCode.ShouldBe(BadWaitingForInitialData); + } + + /// + /// Control. The initial push must not weaken steady-state suppression: an identical + /// message arriving after the subscribe is still swallowed. Without this, "recovery works" + /// could simply mean "nothing is ever deduped any more". + /// + [Fact] + public async Task An_identical_message_after_the_initial_push_is_still_suppressed() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + + var raised = Record(mgr); + await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + + raised.Count.ShouldBe(1); // the initial push only + } + + /// + /// The rebuild shape: unsubscribe and subscribe again — as every deploy does through + /// DriverInstanceActor's unsubscribe-then-subscribe — and the cached value is pushed + /// again onto the new handle rather than leaving the rebuilt node waiting for the plant. + /// + [Fact] + public async Task A_resubscribe_pushes_again() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + var first = await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + await mgr.UnsubscribeAsync(first, TestContext.Current.CancellationToken); + + var raised = Record(mgr); + var second = await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + raised.Count.ShouldBe(1); + raised[0].Value.ShouldBe(21.5); + raised[0].Handle.ShouldBe(second); + } + + // --------------------------------------------------------------------------------- + // (B) Connection-loss stale — 05/STAB-18 shape (a) + // --------------------------------------------------------------------------------- + + /// + /// A lost broker session makes every observed reference BadNoCommunication — with its + /// last value retained, the Part 8 last-known-value convention a SCADA client expects. MQTT + /// previously marked nothing at all on its own disconnect edge, so the tags stayed Good until + /// the host's 90 s comms-loss watchdog noticed (if it noticed at all — MQTTnet usually + /// reconnects well inside that window). + /// + [Fact] + public async Task OnConnectionLost_stales_every_observed_ref_keeping_its_value() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + await mgr.SubscribeAsync([TempPath, PressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + var raised = Record(mgr); // recorded AFTER the subscribe, so the initial push is not counted + mgr.OnConnectionLost(); + + raised.Count.ShouldBe(1, "the never-observed ref has nothing to stale"); + raised[0].Reference.ShouldBe(TempPath); + raised[0].StatusCode.ShouldBe(BadNoCommunication); + raised[0].Value.ShouldBe(21.5, "the last known value is retained, not nulled"); + mgr.Values.Read(TempPath).StatusCode.ShouldBe(BadNoCommunication); + mgr.Values.Read(TempPath).Value.ShouldBe(21.5); + } + + /// + /// The contract test for this seam. After the stale, the broker's retained replay of the + /// identical payload is raised Good — because the cached status now differs even though + /// the value does not. Replaying a different payload here would pass against the + /// pre-fix build and prove nothing; the identical payload is the whole point. + /// + [Fact] + public async Task After_the_stale_the_identical_retained_replay_is_raised_Good() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + mgr.OnConnectionLost(); + + var raised = Record(mgr); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: true); // the retained seed + + raised.Count.ShouldBe(1); + raised[0].Reference.ShouldBe(TempPath); + raised[0].Value.ShouldBe(21.5); + raised[0].StatusCode.ShouldBe(Good); + } + + /// + /// A second loss with no intervening message raises nothing: the ref is already Bad, and a + /// re-stamp would be pure churn on the downstream fan-out. + /// + [Fact] + public async Task OnConnectionLost_does_not_re_raise_an_already_bad_ref() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + mgr.OnConnectionLost(); + + var raised = Record(mgr); + mgr.OnConnectionLost(); + + raised.ShouldBeEmpty(); + } + + /// + /// The stale runs on MQTTnet's dispatcher thread, so it must complete synchronously and never + /// await: parking that thread stalls delivery for every other subscription on the connection. + /// + [Fact] + public async Task OnConnectionLost_completes_on_the_calling_thread() + { + var mgr = NewManager(); + mgr.HandleMessage(TempTopic, """{"value":21.5}"""u8.ToArray(), retained: false); + await mgr.SubscribeAsync([TempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + var callingThread = Environment.CurrentManagedThreadId; + var raisedOn = 0; + mgr.OnDataChange += (_, _) => raisedOn = Environment.CurrentManagedThreadId; + + mgr.OnConnectionLost(); + + raisedOn.ShouldBe(callingThread); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugIngestorRecoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugIngestorRecoveryTests.cs new file mode 100644 index 00000000..88817a47 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugIngestorRecoveryTests.cs @@ -0,0 +1,268 @@ +using Google.Protobuf; +using Org.Eclipse.Tahu.Protobuf; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Sparkplug; +using TahuDataType = Org.Eclipse.Tahu.Protobuf.DataType; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests; + +/// +/// The two push-model edge behaviours lacked (archreview +/// 05/STAB-18), matching on the plain path: an +/// initial push of the cached snapshot on every (re)subscribe, and a +/// connection-loss stale to BadNoCommunication with the value retained. +/// +/// Where the falsifier actually is on this seam — measured, not assumed. Recovery is +/// proved with an identical DBIRTH value, as on the plain path. But Sparkplug ingestion +/// carries no unchanged-value suppression (it publishes births and data directly rather +/// than through MqttSubscriptionManager.Deliver), so that replay was already raised Good +/// before this change — pinned below by +/// . The +/// load-bearing assertion here is therefore the stale: a Sparkplug tag now reports Bad on +/// the driver's own disconnect edge instead of Good over a dead broker. +/// +/// A death is not a connection loss. DDEATH/NDEATH is the source announcing it is +/// offline and keeps its own BadCommunicationError stale, unchanged; ConnectionLost +/// is this driver's transport losing the broker and reports BadNoCommunication — the same +/// code the host's comms-loss fan-out writes. The DDEATH regression case below pins that the two +/// paths stayed distinct. +/// +public sealed class SparkplugIngestorRecoveryTests +{ + // OPC UA status codes, verified against Opc.Ua.StatusCodes rather than recalled. + private const uint Good = 0x00000000u; + private const uint BadCommunicationError = 0x80050000u; + private const uint BadNoCommunication = 0x80310000u; + private const uint BadWaitingForInitialData = 0x80320000u; + + private const string Group = "Plant1"; + private const string Node = "EdgeA"; + private const string Device = "Filler1"; + + private const string TempPath = "Plant/Mqtt/spb/Filler1Temp"; + private const string PressPath = "Plant/Mqtt/spb/Filler1Press"; + + private const ulong TempAlias = 5UL; + private const ulong PressAlias = 6UL; + + private sealed record Raised(string Reference, object? Value, uint StatusCode); + + private static List Record(SparkplugIngestor ing) + { + var raised = new List(); + ing.OnDataChange += (_, e) => raised.Add(new Raised(e.FullReference, e.Snapshot.Value, e.Snapshot.StatusCode)); + return raised; + } + + // --------------------------------------------------------------------------------- + // (A) Initial push on (re)subscribe + // --------------------------------------------------------------------------------- + + /// + /// A metric observed before this subscription existed is pushed once from the cache. This is the + /// address-space-rebuild shape: a rebuild reseeds every node BadWaitingForInitialData and + /// triggers no rebirth, so without the push a quiet metric never refills. + /// + [Fact] + public async Task SubscribeAsync_pushes_the_cached_snapshot_for_an_observed_metric() + { + var ing = await NewSubscribedIngestorAsync(); + FeedBirths(ing); + ing.Dispatch(Topic(SparkplugMessageType.DDATA, Device), Ddata(seq: 2, (TempAlias, TahuDataType.Float, 7f))); + + var raised = Record(ing); + await ing.SubscribeAsync([TempPath], TimeSpan.Zero, TestContext.Current.CancellationToken); + + raised.Count.ShouldBe(1); + raised[0].Reference.ShouldBe(TempPath); + raised[0].Value.ShouldBe(7f); + raised[0].StatusCode.ShouldBe(Good); + } + + /// + /// A metric that has never been observed carries only the cache's + /// BadWaitingForInitialData placeholder, and pushing that would claim an observation the + /// driver has not made. + /// + [Fact] + public async Task SubscribeAsync_does_not_push_a_never_observed_metric() + { + var ing = await NewSubscribedIngestorAsync(); + + var raised = Record(ing); + await ing.SubscribeAsync([TempPath, PressPath], TimeSpan.Zero, TestContext.Current.CancellationToken); + + raised.ShouldBeEmpty(); + ing.Values.Read(TempPath).StatusCode.ShouldBe(BadWaitingForInitialData); + } + + // --------------------------------------------------------------------------------- + // (B) Connection-loss stale + the identical-value recovery + // --------------------------------------------------------------------------------- + + /// + /// A lost broker session marks every observed metric BadNoCommunication with its last + /// value retained. ResetSessionState's previous remark argued the opposite — that the + /// connection's health surface already reports the outage — but health is not quality, and an + /// OPC UA client subscribed to the metric's node sees neither the health page nor the outage. + /// + [Fact] + public async Task OnConnectionLost_stales_every_observed_metric_keeping_its_value() + { + var ing = await NewSubscribedIngestorAsync(); + FeedBirths(ing); + ing.Dispatch(Topic(SparkplugMessageType.DDATA, Device), Ddata(seq: 2, (TempAlias, TahuDataType.Float, 7f))); + + var raised = Record(ing); + ing.OnConnectionLost(); + + raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == BadNoCommunication && Equals(r.Value, 7f)); + ing.Values.Read(TempPath).StatusCode.ShouldBe(BadNoCommunication); + ing.Values.Read(TempPath).Value.ShouldBe(7f); + } + + /// + /// The contract test for this seam. The reconnect replays NBIRTH/DBIRTH carrying the + /// identical metric value, and it must be raised Good. Before the stale, the birth's + /// value equalled the cached one and the whole tag stayed Bad until the plant moved. + /// + [Fact] + public async Task After_the_stale_an_identical_birth_replay_is_raised_Good() + { + var ing = await NewSubscribedIngestorAsync(); + FeedBirths(ing, tempBirthValue: 7f); + ing.OnConnectionLost(); + ing.Values.Read(TempPath).StatusCode.ShouldBe(BadNoCommunication); + + var raised = Record(ing); + FeedBirths(ing, tempBirthValue: 7f, bdSeq: 2); // the reconnect's replayed births, value unchanged + + raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == Good && Equals(r.Value, 7f)); + } + + /// + /// Vacuity control, and it changes what the test above proves. Unlike the plain path, + /// Sparkplug ingestion has no unchanged-value suppression at all — a birth republishes its + /// metrics regardless — so an identical birth replay would have been raised Good even before + /// the stale existed. This asserts that directly. The consequence: on THIS seam the + /// load-bearing assertion is the stale itself (a Sparkplug tag now reports Bad on the driver's + /// own disconnect edge instead of Good over a dead broker), and the replay test guards only + /// that the stale is not sticky. + /// + [Fact] + public async Task An_identical_birth_replay_is_raised_even_without_a_preceding_stale() + { + var ing = await NewSubscribedIngestorAsync(); + FeedBirths(ing, tempBirthValue: 7f); + + var raised = Record(ing); + FeedBirths(ing, tempBirthValue: 7f, bdSeq: 2); + + raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == Good && Equals(r.Value, 7f)); + } + + /// + /// Regression: a DDEATH still stales with BadCommunicationError, the source-announced + /// code. The connection-loss stale is a second, distinct path — it did not replace this one. + /// + [Fact] + public async Task DDeath_still_stales_with_BadCommunicationError() + { + var ing = await NewSubscribedIngestorAsync(); + FeedBirths(ing, tempBirthValue: 7f); + + var raised = Record(ing); + ing.Dispatch(Topic(SparkplugMessageType.DDEATH, Device), Ddeath(seq: 2)); + + raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == BadCommunicationError); + ing.Values.Read(TempPath).StatusCode.ShouldBe(BadCommunicationError); + } + + // ================================================================================= + // Fixtures — deliberately a local copy of SparkplugIngestorTests' helpers (both are + // private to their own class; sharing them would make one suite's edits ripple into + // the other's fixtures). + // ================================================================================= + + private static RawTagEntry Tag(string rawPath, string tagConfig) => new(rawPath, tagConfig, WriteIdempotent: false); + + private static string Blob(string group, string node, string? device, string metric, string dataType) + { + var deviceKey = device is null ? "" : $"""{'"'}deviceId{'"'}:"{device}","""; + return $$"""{"groupId":"{{group}}","edgeNodeId":"{{node}}",{{deviceKey}}"metricName":"{{metric}}","dataType":"{{dataType}}"}"""; + } + + private static async Task NewSubscribedIngestorAsync() + { + var options = new MqttDriverOptions + { + Mode = MqttMode.SparkplugB, + Sparkplug = new MqttSparkplugOptions { GroupId = Group, HostId = "otopcua-host-1" }, + }; + var ingestor = new SparkplugIngestor(options, "mqtt-spb-1", rebirthDebounce: TimeSpan.Zero); + ingestor.Register( + [ + Tag(TempPath, Blob(Group, Node, Device, "Temperature", "Float32")), + Tag(PressPath, Blob(Group, Node, Device, "Pressure", "Float32")), + ]); + + await ingestor.SubscribeAsync([.. ingestor.AuthoredRawPaths], TimeSpan.Zero, TestContext.Current.CancellationToken); + return ingestor; + } + + private static void FeedBirths(SparkplugIngestor ing, float tempBirthValue = 0f, ulong bdSeq = 1) + { + ing.Dispatch(Topic(SparkplugMessageType.NBIRTH), Nbirth(seq: 0, bdSeq)); + ing.Dispatch(Topic(SparkplugMessageType.DBIRTH, Device), Dbirth(seq: 1, tempBirthValue)); + } + + private static SparkplugTopic Topic(SparkplugMessageType type, string? device = null) => + new(type, Group, Node, device, null); + + private static SparkplugPayload Decode(Payload payload) => SparkplugCodec.Decode(payload.ToByteArray()); + + private static SparkplugPayload Nbirth(ulong seq, ulong bdSeq) + { + var payload = new Payload { Seq = seq, Timestamp = 1721822400000UL }; + payload.Metrics.Add(new Payload.Types.Metric + { + Name = "bdSeq", + Datatype = (uint)TahuDataType.Uint64, + LongValue = bdSeq, + }); + return Decode(payload); + } + + private static SparkplugPayload Dbirth(ulong seq, float tempValue) + { + var payload = new Payload { Seq = seq, Timestamp = 1721822400000UL }; + payload.Metrics.Add(BirthMetric("Temperature", TempAlias, tempValue)); + payload.Metrics.Add(BirthMetric("Pressure", PressAlias, 0f)); + return Decode(payload); + } + + private static SparkplugPayload Ddata(ulong seq, params (ulong Alias, TahuDataType Type, float Value)[] metrics) + { + var payload = new Payload { Seq = seq, Timestamp = 1721822405000UL }; + foreach (var (alias, _, value) in metrics) + { + // Deliberately NO Name and NO Datatype: the real post-birth wire shape. + payload.Metrics.Add(new Payload.Types.Metric { Alias = alias, FloatValue = value }); + } + + return Decode(payload); + } + + private static SparkplugPayload Ddeath(ulong seq) => + Decode(new Payload { Seq = seq, Timestamp = 1721822405000UL }); + + private static Payload.Types.Metric BirthMetric(string name, ulong alias, float value) => new() + { + Name = name, + Alias = alias, + Datatype = (uint)TahuDataType.Float, + FloatValue = value, + }; +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs index c25bbdb9..4fd78970 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs @@ -7,6 +7,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Commons.Engines; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Logging; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Scripting; using ZB.MOM.WW.OtOpcUa.Host.Engines; @@ -539,6 +540,98 @@ public sealed class RoslynVirtualTagEvaluatorTests $"{starved}/{totalEvaluations} evaluations missed the 5s budget"); } + // ── input quality reaches the script (archreview 02/S14) ──────────────────────────────── + + /// + /// A script reading ctx.GetTag(path).StatusCode sees the dependency's real status. + /// The read cache used to stamp every input Good, so a script could not detect a degraded + /// input at all — "broken input looks Good", the other door onto the same lie 02/S13 closed for + /// broken scripts. + /// + [Fact] + public void Evaluate_read_cache_carries_the_inputs_real_status_code() + { + const uint uncertain = 0x40000000u; + using var sut = new RoslynVirtualTagEvaluator(NullLogger.Instance, NoOpScriptRoot()); + var stamp = new DateTime(2026, 8, 20, 9, 0, 0, DateTimeKind.Utc); + + var result = sut.Evaluate( + virtualTagId: "vt-quality", + expression: "return (long)ctx.GetTag(\"a\").StatusCode;", + dependencies: new Dictionary + { + ["a"] = new(7, uncertain, stamp, stamp), + }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe((long)uncertain); + } + + /// The dependency's own source timestamp reaches the script, not evaluation time. + [Fact] + public void Evaluate_read_cache_carries_the_inputs_real_source_timestamp() + { + using var sut = new RoslynVirtualTagEvaluator(NullLogger.Instance, NoOpScriptRoot()); + var stamp = new DateTime(2026, 8, 20, 9, 0, 0, DateTimeKind.Utc); + + var result = sut.Evaluate( + virtualTagId: "vt-stamp", + expression: "return ctx.GetTag(\"a\").SourceTimestampUtc;", + dependencies: new Dictionary + { + ["a"] = new(7, 0u, stamp, DateTime.UtcNow), + }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe(stamp); + } + + /// + /// The value-only overload keeps working unchanged — it is what the interface's default + /// implementation and the five test fakes still use, which is the whole reason the new overload is + /// default-implemented rather than abstract. + /// + [Fact] + public void The_value_only_overload_still_evaluates() + { + using var sut = new RoslynVirtualTagEvaluator(NullLogger.Instance, NoOpScriptRoot()); + + var result = sut.Evaluate( + virtualTagId: "vt-values", + expression: "return (int)ctx.GetTag(\"a\").Value + 1;", + dependencies: new Dictionary { ["a"] = 41 }); + + result.Success.ShouldBeTrue(result.Reason); + result.Value.ShouldBe(42); + } + + /// + /// A fake implementing ONLY the value-only member still answers the snapshot overload, through the + /// interface's default projection. Without that default, adding the overload would have been a + /// breaking change for every existing implementation. + /// + [Fact] + public void The_snapshot_overload_falls_back_to_a_value_only_implementation() + { + IVirtualTagEvaluator fake = new ValueOnlyEvaluator(); + + var result = fake.Evaluate("vt-fake", "ignored", new Dictionary + { + ["a"] = new(41, 0u, DateTime.UtcNow, DateTime.UtcNow), + }); + + result.Success.ShouldBeTrue(); + result.Value.ShouldBe(41); + } + + /// An evaluator predating the snapshot overload: implements the value-only member only. + private sealed class ValueOnlyEvaluator : IVirtualTagEvaluator + { + public VirtualTagEvalResult Evaluate( + string virtualTagId, string expression, IReadOnlyDictionary dependencies) + => VirtualTagEvalResult.Ok(dependencies["a"]); + } + /// Reads the count of the private compiled-script cache via reflection. The cache is a /// CompiledScriptCache<,> which exposes a Count property (not ICollection). private static int CompiledCacheCount(RoslynVirtualTagEvaluator sut) diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs new file mode 100644 index 00000000..145b88ee --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs @@ -0,0 +1,226 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// — the status-only batch write behind the +/// comms-loss annotation for value variables (archreview 03/S15). +/// +/// Keeping the value is the whole point. The host holds no value cache (v3's +/// single-source design), so "this driver has stopped communicating, but here is what it last +/// said" can only be expressed at the node, where the value already lives. The Part 8 +/// last-known-value convention is what a SCADA client expects, and the interface's default +/// implementation — which nulls the value because a value-less sink has nothing else to write — +/// would silently erase it if the SDK sink ever stopped overriding it. +/// +/// Never creates. A status carries nothing to create a node from, and the caller's +/// per-driver index can legitimately name a node a surgical remove has just dropped. A missing id +/// is skipped, not materialised — the opposite of , +/// which lazily creates. +/// +/// Boots a real , like +/// , so the assertions read the SDK node state a client +/// would receive. +/// +public sealed class OtOpcUaNodeManagerWriteStatusesTests : IDisposable +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private readonly string _pkiRoot = Path.Combine( + Path.GetTempPath(), + $"otopcua-writestatuses-{Guid.NewGuid():N}"); + + /// + /// The contract: value retained, status + timestamp replaced. Asserted on a node that was first + /// written a real value, because "value retained" is unfalsifiable on a node that never had one. + /// + [Fact] + public async Task WriteStatuses_replaces_status_and_timestamp_but_keeps_the_value() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var t1 = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + var t2 = new DateTime(2026, 8, 20, 10, 5, 0, DateTimeKind.Utc); + + nm.EnsureVariable("Plant/Modbus/sim/HR5", parentFolderNodeId: null, displayName: "HR5", + dataType: "Int32", writable: false, realm: AddressSpaceRealm.Raw); + nm.WriteValue("Plant/Modbus/sim/HR5", 42, OpcUaStatus.Good, t1, AddressSpaceRealm.Raw); + + nm.WriteStatuses([new StatusWrite("Plant/Modbus/sim/HR5", OpcUaStatus.BadNoCommunication, t2, AddressSpaceRealm.Raw)]); + + var v = nm.TryGetVariable("Plant/Modbus/sim/HR5", AddressSpaceRealm.Raw); + v.ShouldNotBeNull(); + v!.Value.ShouldBe(42, "the last known value must survive the comms-loss annotation"); + v.StatusCode.ShouldBe((StatusCode)StatusCodes.BadNoCommunication); + v.StatusCode.ShouldNotBe((StatusCode)StatusCodes.Bad, "the sub-code reaches the SDK verbatim (#519)"); + v.Timestamp.ShouldBe(t2); + } + finally + { + await host.DisposeAsync(); + } + } + + /// + /// Realm-qualified, like every other node-naming sink call: the same s= id in the other + /// namespace is a different node and must be untouched. + /// + [Fact] + public async Task WriteStatuses_only_touches_the_named_realm() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + const string sharedId = "Shared/both/realms"; + + nm.WriteValue(sharedId, 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); + nm.WriteValue(sharedId, 2, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns); + + nm.WriteStatuses([new StatusWrite(sharedId, OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw)]); + + nm.TryGetVariable(sharedId, AddressSpaceRealm.Raw)!.StatusCode + .ShouldBe((StatusCode)StatusCodes.BadNoCommunication); + nm.TryGetVariable(sharedId, AddressSpaceRealm.Uns)!.StatusCode + .ShouldBe((StatusCode)StatusCodes.Good); + } + finally + { + await host.DisposeAsync(); + } + } + + /// + /// An unknown NodeId is a no-op: no node is created, nothing throws, and the rest of the batch + /// still lands. A create here would materialise a node nobody authored, holding no value, from a + /// driver-index entry that is stale precisely because the node was removed. + /// + [Fact] + public async Task An_unknown_node_id_is_skipped_not_created() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + nm.WriteValue("Plant/known", 7, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); + var before = nm.VariableCount; + + Should.NotThrow(() => nm.WriteStatuses( + [ + new StatusWrite("Plant/does/not/exist", OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw), + new StatusWrite("Plant/known", OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw), + ])); + + nm.VariableCount.ShouldBe(before, "a status write must never create a node"); + nm.TryGetVariable("Plant/does/not/exist", AddressSpaceRealm.Raw).ShouldBeNull(); + nm.TryGetVariable("Plant/known", AddressSpaceRealm.Raw)!.StatusCode + .ShouldBe((StatusCode)StatusCodes.BadNoCommunication); + } + finally + { + await host.DisposeAsync(); + } + } + + /// + /// A malformed item does not drop the tail — the same per-item isolation contract + /// carries, reported through the same + /// aggregate. + /// + [Fact] + public async Task A_failing_item_neither_drops_the_tail_nor_hides_which_node_failed() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + nm.WriteValue("Plant/first", 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); + nm.WriteValue("Plant/third", 3, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); + + var ex = Should.Throw(() => nm.WriteStatuses( + [ + new StatusWrite("Plant/first", OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw), + new StatusWrite(string.Empty, OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw), + new StatusWrite("Plant/third", OpcUaStatus.BadNoCommunication, ts, AddressSpaceRealm.Raw), + ])); + + ex.FailedCount.ShouldBe(1); + ex.SucceededCount.ShouldBe(2); + ex.FirstFailedNodeId.ShouldBe(string.Empty); + + nm.TryGetVariable("Plant/first", AddressSpaceRealm.Raw)!.Value.ShouldBe(1); + nm.TryGetVariable("Plant/third", AddressSpaceRealm.Raw)!.StatusCode + .ShouldBe((StatusCode)StatusCodes.BadNoCommunication, "the item after the failing one must not be dropped"); + } + finally + { + await host.DisposeAsync(); + } + } + + /// An empty batch neither throws nor touches the address space. + [Fact] + public async Task Empty_batch_is_a_no_op() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var before = nm.VariableCount; + + Should.NotThrow(() => nm.WriteStatuses([])); + + nm.VariableCount.ShouldBe(before); + } + finally + { + await host.DisposeAsync(); + } + } + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() + { + var host = new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.WriteStatusesTest", + ApplicationUri = $"urn:OtOpcUa.WriteStatusesTest:{Guid.NewGuid():N}", + OpcUaPort = AllocateFreePort(), + PublicHostname = "localhost", + PkiStoreRoot = _pkiRoot, + }, + NullLogger.Instance); + + var server = new OtOpcUaSdkServer(); + await host.StartAsync(server, Ct); + return (host, server); + } + + private static int AllocateFreePort() + { + using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); + listener.Start(); + var port = ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + /// Cleans up the PKI root directory. + public void Dispose() + { + if (Directory.Exists(_pkiRoot)) + { + try { Directory.Delete(_pkiRoot, recursive: true); } + catch { /* best-effort cleanup */ } + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorCommsLossValueNodesTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorCommsLossValueNodesTests.cs new file mode 100644 index 00000000..1454c1ea --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorCommsLossValueNodesTests.cs @@ -0,0 +1,298 @@ +using System.Diagnostics.Metrics; +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; +using ZB.MOM.WW.OtOpcUa.Runtime.OpcUa; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; +using static ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness.V3ArtifactSeeder; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// Archreview 03/S15 — the value-node half of the comms-loss fan-out. Until this, a driver +/// declared comms-lost annotated its Part 9 conditions and pushed a synthetic Bad into the dependency +/// mux, but its raw and UNS value variables — the product's external surface — got nothing: an +/// OPC UA client subscribed to a value node saw the last value at Good quality for the whole +/// outage, for every driver whose failure shape is "reader stops producing" rather than "per-tag Bad +/// snapshot". +/// +/// Status-only, value retained. The write goes through +/// so the node keeps its last known value +/// under BadNoCommunication (OPC UA Part 8's convention). The host holds no value cache — v3's +/// single-source design — so only the node itself can retain it. +/// +/// Nothing is published on reconnect, deliberately: publishing Good would assert a +/// quality this actor has not observed. Recovery is the driver's own re-publish (the poll engine's +/// forceRaise on a fresh subscription, or the push-model initial push). That contract is +/// end-to-end tested in . +/// +public sealed class DriverHostActorCommsLossValueNodesTests : RuntimeActorTestBase +{ + private static readonly NodeId TestNode = NodeId.Parse("driver-commsloss-values"); + private static readonly RevisionHash RevA = RevisionHash.Parse(new string('c', 64)); + private static readonly DateTime TsA = new(2026, 8, 20, 9, 0, 0, DateTimeKind.Utc); + + /// Audit delay used by the T9 cases — long enough that a same-turn prune wins the race, + /// short enough to observe. + private static readonly TimeSpan AuditDelay = TimeSpan.FromMilliseconds(400); + + // Distinct driver ids per metered test: the meter tag is what the recorder filters on, and a + // MeterListener sees every measurement in the process — including a parallel test's (#503). + private const string SilentDriver = "drv-audit-silent"; + private const string RepublishingDriver = "drv-audit-republish"; + private const string PartialDriver = "drv-audit-partial"; + + /// + /// A disconnect produces ONE status batch naming exactly the disconnected driver's raw nodes plus + /// every referencing UNS mirror, all BadNoCommunication. The second driver's ref is the + /// control: a fan-out that walked every value ref on the node instead of the per-driver index would + /// mark a perfectly healthy driver's nodes Bad, and no assertion about the first driver's nodes + /// would notice. + /// + [Fact] + public void Disconnect_marks_the_drivers_raw_and_uns_value_nodes_BadNoCommunication() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: "drv-1", DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + (Driver: "drv-1", DriverName: "Modbus", Device: "dev1", Tag: "temp", DataType: "Double", Writable: false), + (Driver: "drv-2", DriverName: "S7", Device: "dev2", Tag: "other", DataType: "Double", Writable: false), + }, + unsRefs: new[] { (Area: "filling", Line: "line1", Equip: "station1", BackingTag: "speed", Effective: (string?)null) }); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged("drv-1", Connected: false)); + + var batch = publish.ExpectMsg(PresenceBudget); + batch.Items.Select(i => i.NodeId).ShouldBe(new[] + { + "Plant/Modbus/dev1/speed", + "filling/line1/station1/speed", + "Plant/Modbus/dev1/temp", + }, ignoreOrder: true); + + batch.Items.ShouldAllBe(i => i.StatusCode == OpcUaStatus.BadNoCommunication); + batch.Items.Single(i => i.NodeId == "Plant/Modbus/dev1/speed").Realm.ShouldBe(AddressSpaceRealm.Raw); + batch.Items.Single(i => i.NodeId == "filling/line1/station1/speed").Realm.ShouldBe(AddressSpaceRealm.Uns); + + // The control: the OTHER driver's node is untouched by drv-1's outage. + batch.Items.ShouldNotContain(i => i.NodeId == "Plant/S7/dev2/other"); + + // ONE batch, not one message per node. + publish.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + } + + /// + /// A reconnect publishes nothing to the value nodes. Writing Good here would claim a quality + /// the host has not observed — the device may be answering the transport and still returning + /// garbage — and it would race the driver's own first real value. + /// + [Fact] + public void Reconnect_publishes_no_value_or_status_message() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: "drv-1", DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + }, + unsRefs: Array.Empty<(string, string, string, string, string?)>()); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged("drv-1", Connected: false)); + publish.ExpectMsg(PresenceBudget); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged("drv-1", Connected: true)); + + publish.ExpectNoMsg(TimeSpan.FromMilliseconds(500)); + } + + // --------------------------------------------------------------------------------- + // T9 — the recovery audit (the dedup-trap tripwire) + // --------------------------------------------------------------------------------- + + /// + /// The host publishes nothing Good on reconnect, so the only way a stuck-Bad node becomes visible + /// is this audit: refs still latched some time after the driver reconnected are logged and metered. + /// Here the driver re-publishes nothing at all — the pre-STAB-18 MQTT shape, and the exact failure + /// any future push driver without an initial push would produce. + /// + [Fact] + public void Refs_still_latched_after_a_reconnect_are_logged_and_metered() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: SilentDriver, DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + (Driver: SilentDriver, DriverName: "Modbus", Device: "dev1", Tag: "temp", DataType: "Double", Writable: false), + }, + unsRefs: new[] { (Area: "filling", Line: "line1", Equip: "station1", BackingTag: "speed", Effective: (string?)null) }); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA, commsRecoveryAuditDelay: AuditDelay); + using var meter = new MeterRecorder("otopcua.driver.comms_latched_refs"); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged(SilentDriver, Connected: false)); + publish.ExpectMsg(PresenceBudget); + + EventFilter.Warning(contains: "still BadNoCommunication").ExpectOne(TimeSpan.FromSeconds(10), () => + actor.Tell(new DriverInstanceActor.ConnectivityChanged(SilentDriver, Connected: true))); + + // Two REFS, not three nodes: the latch is keyed by RawPath, and the UNS mirror shares its + // backing tag's ref — the operator is being told how many tags did not come back, not how many + // address-space nodes they project onto. + AwaitCondition(() => meter.WithTag("driver", SilentDriver) > 0, TimeSpan.FromSeconds(5)); + meter.SumWithTag("driver", SilentDriver).ShouldBe(2); + } + + /// + /// The control. A driver that DOES re-publish every latched ref on reconnect trips nothing — + /// without this, the test above would pass against an audit that fires unconditionally, which would + /// make the Warning noise rather than signal. + /// + [Fact] + public void A_driver_that_republishes_every_ref_trips_no_audit() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: RepublishingDriver, DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + (Driver: RepublishingDriver, DriverName: "Modbus", Device: "dev1", Tag: "temp", DataType: "Double", Writable: false), + }, + unsRefs: new[] { (Area: "filling", Line: "line1", Equip: "station1", BackingTag: "speed", Effective: (string?)null) }); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA, commsRecoveryAuditDelay: AuditDelay); + using var meter = new MeterRecorder("otopcua.driver.comms_latched_refs"); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged(RepublishingDriver, Connected: false)); + publish.ExpectMsg(PresenceBudget); + + EventFilter.Warning(contains: "still BadNoCommunication").Expect(0, TimeSpan.FromSeconds(5), () => + { + actor.Tell(new DriverInstanceActor.ConnectivityChanged(RepublishingDriver, Connected: true)); + + // The driver's own re-publish — identical values, which is precisely the case a value-only + // dedup would swallow; here it is the driver that force-raised, so both refs are pruned. + actor.Tell(new DriverInstanceActor.AttributeValueBatchPublished(RepublishingDriver, new[] + { + new DriverInstanceActor.AttributeValueItem("Plant/Modbus/dev1/speed", 42.0, OpcUaStatus.Good, TsA), + new DriverInstanceActor.AttributeValueItem("Plant/Modbus/dev1/temp", 7.5, OpcUaStatus.Good, TsA), + })); + publish.ExpectMsg(PresenceBudget); + }); + + meter.WithTag("driver", RepublishingDriver).ShouldBe(0); + } + + /// + /// A partial recovery is still reported, and reports only what is missing: one ref comes back, the + /// other does not, and the audit counts exactly one. An audit that reported the whole latch (or + /// nothing at all once anything arrived) would be useless on precisely the mixed case an operator + /// needs it for. + /// + [Fact] + public void A_partial_recovery_reports_only_the_refs_that_did_not_come_back() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: PartialDriver, DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + (Driver: PartialDriver, DriverName: "Modbus", Device: "dev1", Tag: "temp", DataType: "Double", Writable: false), + }, + unsRefs: Array.Empty<(string, string, string, string, string?)>()); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA, commsRecoveryAuditDelay: AuditDelay); + using var meter = new MeterRecorder("otopcua.driver.comms_latched_refs"); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged(PartialDriver, Connected: false)); + publish.ExpectMsg(PresenceBudget); + + EventFilter.Warning(contains: "still BadNoCommunication").ExpectOne(TimeSpan.FromSeconds(10), () => + { + actor.Tell(new DriverInstanceActor.ConnectivityChanged(PartialDriver, Connected: true)); + actor.Tell(new DriverInstanceActor.AttributeValueBatchPublished(PartialDriver, new[] + { + new DriverInstanceActor.AttributeValueItem("Plant/Modbus/dev1/speed", 42.0, OpcUaStatus.Good, TsA), + })); + publish.ExpectMsg(PresenceBudget); + }); + + AwaitCondition(() => meter.WithTag("driver", PartialDriver) > 0, TimeSpan.FromSeconds(5)); + meter.SumWithTag("driver", PartialDriver).ShouldBe(1, "only the ref that never came back is reported"); + } + + /// + /// A driver that owns no value refs produces no message at all — an empty batch would cost a hop + /// and a sink call for nothing on every flap of an alarm-only or freshly-added driver. + /// + [Fact] + public void A_driver_with_no_value_refs_publishes_nothing() + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: "drv-1", DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + }, + unsRefs: Array.Empty<(string, string, string, string, string?)>()); + + var (actor, publish, _) = SpawnHostAndApply(this, db, TestNode, deploymentId, RevA); + + actor.Tell(new DriverInstanceActor.ConnectivityChanged("drv-unknown", Connected: false)); + + publish.ExpectNoMsg(TimeSpan.FromMilliseconds(500)); + } + + /// + /// Sums one counter's measurements, filtered by tag. A observes + /// every measurement in the process, so filtering on instrument name alone would fold in a parallel + /// test's (#503) — each test here therefore uses its own driver id and asserts on that tag. + /// + private sealed class MeterRecorder : IDisposable + { + private readonly MeterListener _listener; + private readonly List<(long Value, KeyValuePair[] Tags)> _measurements = new(); + private readonly object _gate = new(); + + public MeterRecorder(string instrumentName) + { + _listener = new MeterListener + { + InstrumentPublished = (instrument, listener) => + { + if (instrument.Meter.Name == OtOpcUaTelemetry.MeterName && instrument.Name == instrumentName) + listener.EnableMeasurementEvents(instrument); + }, + }; + _listener.SetMeasurementEventCallback((_, value, tags, _) => + { + lock (_gate) _measurements.Add((value, tags.ToArray())); + }); + _listener.Start(); + } + + public int WithTag(string key, string value) + { + lock (_gate) return _measurements.Count(m => m.Tags.Any(t => t.Key == key && Equals(t.Value, value))); + } + + public long SumWithTag(string key, string value) + { + lock (_gate) + return _measurements.Where(m => m.Tags.Any(t => t.Key == key && Equals(t.Value, value))) + .Sum(m => m.Value); + } + + public void Dispose() => _listener.Dispose(); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostCommsLossRecoveryTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostCommsLossRecoveryTests.cs new file mode 100644 index 00000000..4f67cc1b --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostCommsLossRecoveryTests.cs @@ -0,0 +1,237 @@ +using System.Diagnostics.Metrics; +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Admin; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; +using ZB.MOM.WW.OtOpcUa.Runtime.OpcUa; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; +using static ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness.V3ArtifactSeeder; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// The 03/S15 contract test: comms loss → value nodes Bad → the driver's own re-publish clears +/// them. A real is spawned by a real +/// around a scripted driver, so the whole seam runs: the child's +/// Reconnecting entry announces comms loss, the host stamps the value nodes +/// BadNoCommunication, the child reconnects and re-subscribes, and the driver's initial push is +/// what makes the nodes Good again. +/// +/// ⚠️ Vacuity rule — the pushed value is IDENTICAL to the pre-outage one. The host writes +/// nothing Good on reconnect by design, so recovery depends entirely on a re-publish landing; and every +/// dedup on that path (a driver's last-value cache, a consumer's last-published memory) keys on value. +/// A recovery test whose driver pushes a different value therefore passes against a build with +/// no force-republish anywhere — the value change alone defeats every dedup — and is not this +/// contract's guard. The same-value push is the whole point. +/// +/// The falsification control is +/// : a driver whose +/// SubscribeAsync pushes nothing (the pre-STAB-18 MQTT shape) leaves the nodes Bad and is caught +/// only by the audit. It proves the test above can fail. +/// +public sealed class DriverHostCommsLossRecoveryTests : RuntimeActorTestBase +{ + private static readonly NodeId TestNode = NodeId.Parse("driver-commsloss-recovery"); + private static readonly RevisionHash RevA = RevisionHash.Parse(new string('d', 64)); + + /// Audit window measured from the RECONNECT, not from the outage — the initial push lands + /// within milliseconds of Connected, so this is generous. + private static readonly TimeSpan AuditDelay = TimeSpan.FromSeconds(2); + + /// The child's reconnect retry runs on DriverInstanceActor.DefaultReconnectInterval + /// (10 s) and the host exposes no seam to shorten it, so every budget here clears that. + private static readonly TimeSpan ReconnectBudget = TimeSpan.FromSeconds(25); + + private const string SpeedRef = "Plant/Modbus/dev1/speed"; + private const string UnsSpeedNode = "filling/line1/station1/speed"; + + [Fact] + public void An_identical_value_pushed_on_resubscribe_recovers_the_nodes() + { + var driver = new ScriptedSubscribeDriver { InstanceId = "drv-recover", InitialPush = (SpeedRef, 5.0, OpcUaStatus.Good) }; + var (host, publish) = Arrange(driver, "drv-recover"); + using var meter = new MeterRecorder("otopcua.driver.comms_latched_refs"); + + // Steady state: the driver's FIRST subscribe already pushed 5 Good, and the host fanned it to + // raw + UNS. (The push on subscribe is the behaviour under test, so it is also how the pre-outage + // value gets there — no separate seeding call, which would leave an extra batch in the probe.) + var first = publish.ExpectMsg(PresenceBudget); + first.Items.Select(i => i.NodeId).ShouldBe(new[] { SpeedRef, UnsSpeedNode }, ignoreOrder: true); + first.Items.ShouldAllBe(i => Equals(i.Value, 5.0)); + + // The outage: the child re-enters Reconnecting and announces comms loss to the host. + host.Tell(new ReconnectDriver("c1", "drv-recover", "test-operator", Guid.NewGuid())); + + var staled = publish.ExpectMsg(ReconnectBudget); + staled.Items.Select(i => i.NodeId).ShouldBe(new[] { SpeedRef, UnsSpeedNode }, ignoreOrder: true); + staled.Items.ShouldAllBe(i => i.StatusCode == OpcUaStatus.BadNoCommunication); + + // Recovery: the reconnect re-subscribes, and the driver pushes the SAME value it held before. + var recovered = publish.ExpectMsg(ReconnectBudget); + recovered.Items.Select(i => i.NodeId).ShouldBe(new[] { SpeedRef, UnsSpeedNode }, ignoreOrder: true); + recovered.Items.ShouldAllBe(i => i.StatusCode == OpcUaStatus.Good); + recovered.Items.ShouldAllBe(i => Equals(i.Value, 5.0)); + + // …and the audit finds nothing outstanding. + EventFilter.Warning(contains: "still BadNoCommunication") + .Expect(0, AuditDelay + TimeSpan.FromSeconds(3), () => { }); + meter.SumWithTag("driver", "drv-recover").ShouldBe(0); + } + + /// + /// The falsification control. Same scenario, but the driver's SubscribeAsync pushes + /// nothing — exactly what MQTT did before 05/STAB-18, and what any push driver without an initial + /// push does. The nodes stay Bad (the host writes no Good of its own), and the ONLY thing that says + /// so is the recovery audit. + /// + [Fact] + public void A_silent_resubscribe_leaves_the_nodes_Bad_and_trips_the_audit() + { + var driver = new ScriptedSubscribeDriver { InstanceId = "drv-silent", InitialPush = null }; + var (host, publish) = Arrange(driver, "drv-silent"); + using var meter = new MeterRecorder("otopcua.driver.comms_latched_refs"); + + driver.FireDataChange(SpeedRef, 5.0, OpcUaStatus.Good); + publish.ExpectMsg(PresenceBudget); + + EventFilter.Warning(contains: "still BadNoCommunication").ExpectOne(ReconnectBudget, () => + { + host.Tell(new ReconnectDriver("c1", "drv-silent", "test-operator", Guid.NewGuid())); + publish.ExpectMsg(ReconnectBudget); + + // Nothing Good ever arrives — assert it rather than merely not looking. + publish.ExpectNoMsg(TimeSpan.FromSeconds(1)); + }); + + // One ref (the raw tag); its UNS mirror shares it. + AwaitCondition(() => meter.SumWithTag("driver", "drv-silent") == 1, TimeSpan.FromSeconds(5)); + driver.SubscribeCount.ShouldBeGreaterThanOrEqualTo(2, + "the child must actually have re-subscribed — otherwise this proves nothing about the push"); + } + + private (IActorRef Host, Akka.TestKit.TestProbe Publish) Arrange(ScriptedSubscribeDriver driver, string driverId) + { + var db = NewInMemoryDbFactory(); + var deploymentId = SeedV3Deployment(db, RevA, + rawTags: new[] + { + (Driver: driverId, DriverName: "Modbus", Device: "dev1", Tag: "speed", DataType: "Double", Writable: false), + }, + unsRefs: new[] { (Area: "filling", Line: "line1", Equip: "station1", BackingTag: "speed", Effective: (string?)null) }, + enabled: true); + + var (host, publish, _) = SpawnHostAndApply( + this, db, TestNode, deploymentId, RevA, + commsRecoveryAuditDelay: AuditDelay, + driverFactory: new SingleDriverFactory(driver)); + + // The child is spawned + initialised by the apply; wait for its first subscribe so the scripted + // pushes below are attributed to a live subscription. + AwaitCondition(() => driver.SubscribeCount >= 1, PresenceBudget); + return (host, publish); + } + + /// Hands the host one scripted driver, whatever type/instance it asks for. + private sealed class SingleDriverFactory(IDriver driver) : IDriverFactory + { + public IDriver? TryCreate(string driverType, string driverInstanceId, string driverConfigJson) => driver; + + public IReadOnlyCollection SupportedTypes { get; } = ["Modbus"]; + } + + /// + /// A subscribable driver whose optionally performs an initial + /// push — the poll engine's forceRaise: true and MQTT's post-STAB-18 behaviour, modelled + /// at the seam the host actually observes. With null it is the silent + /// shape the control test needs. + /// + private sealed class ScriptedSubscribeDriver : IDriver, ISubscribable + { + private readonly StubHandle _handle = new(); + private int _subscribeCount; + + public event EventHandler? OnDataChange; + + /// The value re-published on every (re)subscribe, or null for a silent driver. + public (string Ref, object? Value, uint StatusCode)? InitialPush { get; init; } + + /// The instance id the child actor adopts — DriverInstanceActor takes it from the + /// DRIVER, not from the spec, and the host's ref map is keyed by the spec's id. They must agree or + /// every published value is dropped as unmapped. + public required string InstanceId { get; init; } + + public int SubscribeCount => Volatile.Read(ref _subscribeCount); + + public string DriverInstanceId => InstanceId; + + public string DriverType => "Modbus"; + + public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + // Stable health: the #527 watchdog must not fire on its own here — the outage in these tests is + // driven explicitly, and a health-driven trip would make the timing non-deterministic. + public DriverHealth GetHealth() => new(DriverState.Healthy, DateTime.UtcNow, null); + + public Task SubscribeAsync( + IReadOnlyList fullReferences, TimeSpan publishingInterval, CancellationToken cancellationToken) + { + Interlocked.Increment(ref _subscribeCount); + if (InitialPush is { } push && fullReferences.Contains(push.Ref)) + FireDataChange(push.Ref, push.Value, push.StatusCode); + + return Task.FromResult(_handle); + } + + public Task UnsubscribeAsync(ISubscriptionHandle handle, CancellationToken cancellationToken) => Task.CompletedTask; + + public void FireDataChange(string fullRef, object? value, uint statusCode) + { + var snapshot = new DataValueSnapshot(value, statusCode, DateTime.UtcNow, DateTime.UtcNow); + OnDataChange?.Invoke(this, new DataChangeEventArgs(_handle, fullRef, snapshot)); + } + } + + /// Tag-filtered counter recorder — see the note in + /// DriverHostActorCommsLossValueNodesTests on why the instrument name alone is not enough. + private sealed class MeterRecorder : IDisposable + { + private readonly MeterListener _listener; + private readonly List<(long Value, KeyValuePair[] Tags)> _measurements = new(); + private readonly object _gate = new(); + + public MeterRecorder(string instrumentName) + { + _listener = new MeterListener + { + InstrumentPublished = (instrument, listener) => + { + if (instrument.Meter.Name == OtOpcUaTelemetry.MeterName && instrument.Name == instrumentName) + listener.EnableMeasurementEvents(instrument); + }, + }; + _listener.SetMeasurementEventCallback((_, value, tags, _) => + { + lock (_gate) _measurements.Add((value, tags.ToArray())); + }); + _listener.Start(); + } + + public long SumWithTag(string key, string value) + { + lock (_gate) + return _measurements.Where(m => m.Tags.Any(t => t.Key == key && Equals(t.Value, value))) + .Sum(m => m.Value); + } + + public void Dispose() => _listener.Dispose(); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorTests.cs index 6f1c4958..9dd9eb0c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorTests.cs @@ -374,4 +374,54 @@ public sealed class DriverInstanceActorTests : RuntimeActorTestBase AwaitCondition(() => driver.SubscribeCount >= 1, TimeSpan.FromSeconds(3)); } + /// + /// Archreview 03/S15 (T11) — a child that is stopped announces comms loss on its way + /// out, so the host latches its value nodes BadNoCommunication. + /// Every other route into Reconnecting announces it (watchdog trip, failed initial + /// connect, Force Reconnect all run the Reconnecting entry). Stopping did not: an operator's + /// Restart and a supervision respawn tore the child down silently, so its nodes kept serving their + /// last value at Good across the gap. The fresh child's re-subscribe + initial push (03/S16) is what + /// clears them again. + /// + [Fact] + public void Stopping_a_connected_child_announces_comms_loss_to_its_parent() + { + var parent = CreateTestProbe(); + var driver = new StubDriver(); + var actor = parent.ChildActorOf(DriverInstanceActor.Props(driver)); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + parent.ExpectMsg(PresenceBudget).Connected.ShouldBeTrue(); + + parent.Watch(actor); + parent.Send(actor, PoisonPill.Instance); + + parent.ExpectMsg(PresenceBudget).Connected.ShouldBeFalse(); + parent.ExpectTerminated(actor, PresenceBudget); + } + + /// + /// Control. A child that never reached Connected announces nothing when stopped — its + /// nodes are still BadWaitingForInitialData, which is a more precise statement than + /// "communication lost", and overwriting it would claim an outage that never happened. + /// + [Fact] + public void Stopping_a_never_connected_child_announces_nothing() + { + var parent = CreateTestProbe(); + var driver = new StubDriver { InitializeShouldThrow = true }; + var actor = parent.ChildActorOf(DriverInstanceActor.Props( + driver, reconnectInterval: TimeSpan.FromSeconds(30))); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + // The failed initial connect announces Connected=false from the Reconnecting entry; consume it, so + // what follows is only what STOPPING produced. + parent.ExpectMsg(PresenceBudget).Connected.ShouldBeFalse(); + + parent.Watch(actor); + parent.Send(actor, PoisonPill.Instance); + + parent.ExpectTerminated(actor, PresenceBudget); + parent.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs index 11160db8..da329e79 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs @@ -140,15 +140,20 @@ public static class V3ArtifactSeeder /// Optional per-driver supervision-reprovision rate limit forwarded to /// DriverHostActor.Props; null keeps the production default. Exists so a test can observe the /// deferral without waiting the production window. + /// Optional 03/S15 audit delay — how long after a reconnect the + /// host checks for refs still latched BadNoCommunication. Null keeps the production default, + /// which is far longer than any test's lifetime. /// The host actor plus the publish and mux probes wired into it. public static (IActorRef Actor, TestProbe Publish, TestProbe Mux) SpawnHostAndApply( TestKitBase kit, IDbContextFactory db, NodeId localNode, DeploymentId deploymentId, RevisionHash rev, IDriverFactory? driverFactory = null, - TimeSpan? reprovisionBackoff = null) + TimeSpan? reprovisionBackoff = null, + TimeSpan? commsRecoveryAuditDelay = null) { var publish = kit.CreateTestProbe(); - var (actor, mux, coordinator) = Spawn(kit, db, localNode, publish.Ref, driverFactory, reprovisionBackoff); + var (actor, mux, coordinator) = + Spawn(kit, db, localNode, publish.Ref, driverFactory, reprovisionBackoff, commsRecoveryAuditDelay); Dispatch(actor, coordinator, deploymentId, rev); publish.ExpectMsg(RuntimeActorTestBase.PresenceBudget); @@ -175,7 +180,8 @@ public static class V3ArtifactSeeder private static (IActorRef Actor, TestProbe Mux, TestProbe Coordinator) Spawn( TestKitBase kit, IDbContextFactory db, NodeId localNode, - IActorRef? opcUaPublishActor, IDriverFactory? driverFactory, TimeSpan? reprovisionBackoff) + IActorRef? opcUaPublishActor, IDriverFactory? driverFactory, TimeSpan? reprovisionBackoff, + TimeSpan? commsRecoveryAuditDelay = null) { var coordinator = kit.CreateTestProbe(); var mux = kit.CreateTestProbe(); @@ -189,7 +195,8 @@ public static class V3ArtifactSeeder opcUaPublishActor: opcUaPublishActor, virtualTagEvaluator: NullVirtualTagEvaluator.Instance, virtualTagHostOverride: vtHost.Ref, - reprovisionBackoff: reprovisionBackoff)); + reprovisionBackoff: reprovisionBackoff, + commsRecoveryAuditDelay: commsRecoveryAuditDelay)); return (actor, mux, coordinator); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorStatusBatchTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorStatusBatchTests.cs new file mode 100644 index 00000000..1174e5f9 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorStatusBatchTests.cs @@ -0,0 +1,197 @@ +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Runtime.OpcUa; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.OpcUa; + +/// +/// — the status-only route to the sink +/// that the comms-loss fan-out uses (archreview 03/S15). It must reach +/// as exactly one call, never decompose into +/// value writes (which would null every node's value), and survive a throwing sink. +/// +public sealed class OpcUaPublishActorStatusBatchTests : RuntimeActorTestBase +{ + /// + /// One message, one WriteStatuses call carrying every item verbatim — and zero value + /// writes. The second half is load-bearing: a handler that fell through to WriteValues (or + /// to the interface's default, which loops WriteValue with a null value) would satisfy "the + /// status arrived" while erasing the last known value the annotation exists to keep. + /// + [Fact] + public void Status_batch_reaches_sink_as_one_WriteStatuses_call() + { + var sink = new RecordingSink(); + var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink)); + + var t0 = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + var items = new[] + { + new StatusWrite("Plant/Modbus/dev1/Speed", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Raw), + new StatusWrite("AreaA/Line1/Eq1/Speed", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Uns), + }; + + actor.Tell(new OpcUaPublishActor.AttributeStatusBatchUpdate(items)); + + AwaitAssert(() => sink.StatusBatches.Count.ShouldBe(1), PresenceBudget); + var batch = sink.StatusBatches[0]; + batch.Count.ShouldBe(2); + batch[0].ShouldBe(items[0]); + batch[1].ShouldBe(items[1]); + + sink.ValueBatches.Count.ShouldBe(0, "a status annotation must never be routed as a value write"); + sink.SingleWrites.Count.ShouldBe(0); + } + + /// + /// A partial failure is metered honestly, exactly as the value batch is: the tally advances by the + /// items the sink DID apply, not by zero and not by the whole batch. + /// + [Fact] + public void Partial_failure_still_counts_the_items_that_landed() + { + var sink = new PartialFailureSink(); + var actor = ActorOfAsTestActorRef(OpcUaPublishActor.PropsForTests(sink: sink)); + + var t0 = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + actor.Tell(new OpcUaPublishActor.AttributeStatusBatchUpdate(new[] + { + new StatusWrite("Plant/Modbus/dev1/Speed", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Raw), + new StatusWrite("", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Raw), + new StatusWrite("Plant/Modbus/dev1/Temp", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Raw), + })); + + sink.Threw.ShouldBeTrue(); + actor.UnderlyingActor.WriteCount.ShouldBe(2, + "the two items the sink DID apply must still be counted — not 0 (the general catch) and not 3"); + } + + /// + /// A throwing sink is swallowed and logged, so a node manager fault cannot restart the publish + /// actor and drop live traffic. As in the value-batch suite, survival alone proves nothing (Akka's + /// restart preserves the mailbox) — the load-bearing assertions are that no exception reaches + /// supervision and that the handler logs its own Warning. + /// + [Fact] + public void Sink_throw_is_swallowed_and_logged() + { + var sink = new ThrowOnWriteStatusesSink(); + var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink)); + + var t0 = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + + EventFilter.Exception().Expect(0, () => + { + EventFilter.Warning(contains: "sink.WriteStatuses threw").ExpectOne(() => + actor.Tell(new OpcUaPublishActor.AttributeStatusBatchUpdate(new[] + { + new StatusWrite("Plant/Modbus/dev1/Speed", OpcUaStatus.BadNoCommunication, t0, AddressSpaceRealm.Raw), + }))); + + // Survival: a follow-up on the untouched per-item value path lands only if the actor is pumping. + actor.Tell(new OpcUaPublishActor.AttributeValueUpdate( + "Plant/Modbus/dev1/Temp", 2, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw)); + + AwaitAssert(() => sink.SingleWrites.Count.ShouldBe(1), PresenceBudget); + }); + + sink.Threw.ShouldBeTrue(); + } + + private void AwaitAssert(Action assertion, TimeSpan duration) + { + var deadline = DateTime.UtcNow + duration; + Exception? last = null; + while (DateTime.UtcNow < deadline) + { + try { assertion(); return; } + catch (Exception ex) { last = ex; Thread.Sleep(25); } + } + if (last is not null) throw last; + } + + /// Records status batches, value batches and per-item value writes separately, so a handler + /// that routed a status annotation down the value path is distinguishable from one that did not. + private class RecordingSink : IOpcUaAddressSpaceSink + { + private readonly object _gate = new(); + private readonly List> _statusBatches = new(); + private readonly List> _valueBatches = new(); + private readonly List _singles = new(); + + public IReadOnlyList> StatusBatches { get { lock (_gate) return _statusBatches.ToArray(); } } + public IReadOnlyList> ValueBatches { get { lock (_gate) return _valueBatches.ToArray(); } } + public IReadOnlyList SingleWrites { get { lock (_gate) return _singles.ToArray(); } } + + public void WriteValue(string nodeId, object? value, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm = AddressSpaceRealm.Uns) + { + lock (_gate) _singles.Add(new ValueWrite(nodeId, value, statusCode, sourceTimestampUtc, realm)); + } + + public void WriteValues(IReadOnlyList values) + { + lock (_gate) _valueBatches.Add(values.ToArray()); + } + + // Overrides the interface's default (which loops WriteValue with a NULL value). + public virtual void WriteStatuses(IReadOnlyList statuses) + { + lock (_gate) _statusBatches.Add(statuses.ToArray()); + } + + public void WriteAlarmQuality(string alarmNodeId, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm) { } + public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime occurredUtc, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { } + public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, AddressSpaceRealm realm, bool isNative = false) { } + public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { } + public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, AddressSpaceRealm realm, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { } + public void RebuildAddressSpace() { } + public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { } + public void WireAlarmNotifiers(string alarmNodeId, AddressSpaceRealm alarmRealm, IReadOnlyList notifierFolderNodeIds, AddressSpaceRealm notifierFolderRealm) { } + public void AddReference(string sourceNodeId, AddressSpaceRealm sourceRealm, string targetNodeId, AddressSpaceRealm targetRealm, string referenceType = "Organizes") { } + } + + /// Throws a PLAIN exception out of the status batch — drives the handler's general catch. + private sealed class ThrowOnWriteStatusesSink : RecordingSink + { + private int _threw; + + public bool Threw => Volatile.Read(ref _threw) != 0; + + public override void WriteStatuses(IReadOnlyList statuses) + { + Interlocked.Exchange(ref _threw, 1); + throw new InvalidOperationException("simulated status-write fault"); + } + } + + /// Models the real sink's PARTIAL failure: applies every item with a non-empty NodeId, then + /// reports the aggregate as a . + private sealed class PartialFailureSink : RecordingSink + { + private int _threw; + + public bool Threw => Volatile.Read(ref _threw) != 0; + + public override void WriteStatuses(IReadOnlyList statuses) + { + var applied = statuses.Where(s => !string.IsNullOrEmpty(s.NodeId)).ToArray(); + var failedCount = statuses.Count - applied.Length; + if (failedCount == 0) + { + base.WriteStatuses(statuses); + return; + } + + base.WriteStatuses(applied); + Interlocked.Exchange(ref _threw, 1); + throw new ValueWriteBatchException( + firstFailedNodeId: statuses.First(s => string.IsNullOrEmpty(s.NodeId)).NodeId, + failedCount: failedCount, + succeededCount: applied.Length, + firstFailure: new ArgumentException("empty NodeId")); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/DependencyConsumerMuxAdapterTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/DependencyConsumerMuxAdapterTests.cs new file mode 100644 index 00000000..33e4bb9a --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/DependencyConsumerMuxAdapterTests.cs @@ -0,0 +1,91 @@ +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; +using ZB.MOM.WW.OtOpcUa.Runtime.VirtualTags; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.VirtualTags; + +/// +/// Archreview 05/STAB-19 — the adapter that carries dependency values from the per-node +/// dependency mux into an (today: the Calculation driver). +/// +/// The defect these pin. The adapter forwarded a hard-coded statusCode: 0u behind +/// a comment claiming "quality is stripped at the mux boundary". That was true once; since #478/#519 +/// carries the source driver's full status code, +/// sub-code included. The consequence was that a comms-lost input reached the Calculation driver as +/// arrived, Good, null — indistinguishable from a real Good reading of null, which is how a +/// calc output kept publishing Good over a dead input. +/// +public sealed class DependencyConsumerMuxAdapterTests : RuntimeActorTestBase +{ + private const string TagA = "Plant/Modbus/dev1/a"; + private const string TagB = "Plant/Modbus/dev1/b"; + + /// Single delivery: the status code reaches the consumer verbatim, sub-code and all. + [Fact] + public void A_single_dependency_value_carries_its_status_code_to_the_consumer() + { + var (consumer, adapter) = Spawn(); + + adapter.Tell(new VirtualTagActor.DependencyValueChanged( + TagA, Value: null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + + AwaitCondition(() => consumer.Received.Count == 1, PresenceBudget); + consumer.Received[0].Ref.ShouldBe(TagA); + consumer.Received[0].StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + consumer.Received[0].StatusCode.ShouldNotBe(OpcUaStatus.Good, + "a Bad input forwarded as Good is the laundering 05/STAB-19 is about"); + } + + /// + /// Batched delivery goes through the same seam, so it must carry status too — the batch path is + /// what a real driver burst uses, and a fix applied only to the single-value path would be inert + /// in production. + /// + [Fact] + public void A_batched_delivery_carries_each_items_status_code() + { + var (consumer, adapter) = Spawn(); + var ts = DateTime.UtcNow; + + adapter.Tell(new VirtualTagActor.DependencyValueBatch(new[] + { + new VirtualTagActor.DependencyValueChanged(TagA, 1.0, ts, OpcUaStatus.Good), + new VirtualTagActor.DependencyValueChanged(TagB, null, ts, OpcUaStatus.BadNoCommunication), + })); + + AwaitCondition(() => consumer.Received.Count == 2, PresenceBudget); + consumer.Received[0].StatusCode.ShouldBe(OpcUaStatus.Good); + consumer.Received[1].StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + } + + private (RecordingConsumer Consumer, IActorRef Adapter) Spawn() + { + var consumer = new RecordingConsumer(); + var mux = CreateTestProbe(); + var adapter = Sys.ActorOf(DependencyConsumerMuxAdapter.Props(consumer, mux.Ref)); + mux.ExpectMsg(PresenceBudget); + return (consumer, adapter); + } + + private sealed class RecordingConsumer : IDependencyConsumer + { + private readonly List<(string Ref, object? Value, uint StatusCode)> _received = new(); + private readonly object _gate = new(); + + public IReadOnlyList<(string Ref, object? Value, uint StatusCode)> Received + { + get { lock (_gate) return _received.ToArray(); } + } + + public IReadOnlyCollection DependencyRefs { get; } = [TagA, TagB]; + + public void OnDependencyValue(string fullReference, object? value, uint statusCode, DateTime timestampUtc) + { + lock (_gate) _received.Add((fullReference, value, statusCode)); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorInputQualityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorInputQualityTests.cs new file mode 100644 index 00000000..6cb014fb --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorInputQualityTests.cs @@ -0,0 +1,315 @@ +using System.Diagnostics.Metrics; +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Engines; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Logging; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; +using ZB.MOM.WW.OtOpcUa.Runtime.VirtualTags; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.VirtualTags; + +/// +/// Archreview 02/S14 — what a virtual tag does with its dependencies' quality. +/// +/// The defect. The actor stored dependency values only, evaluated unconditionally, and +/// the Roslyn read cache stamped every input Good — so a VT over a comms-lost input kept publishing +/// fresh-Good results, and a script author could not detect it because +/// ctx.GetTag(path).StatusCode read the fabricated Good. 02/S13 fixed "broken script looks +/// Good"; this is the same lie through the other door. +/// +/// The policy is the Calculation driver's and the alarm engine's, stated once in +/// InputQuality: hold on Bad and publish the worst input's sub-code; evaluate through +/// Uncertain and stamp the output with it. The R2-03 decisions are kept, not re-litigated — +/// transition-once, the inputs-ready gate, and force-republish on recovery. +/// +/// ⚠️ Vacuity rule: the recovery case computes the identical value it had before +/// the outage. A recovery with a different value passes against a value-only dedup key and is not the +/// guard — that dedup is exactly the "Bad forever" trap. +/// +public sealed class VirtualTagActorInputQualityTests : RuntimeActorTestBase +{ + private const uint Uncertain = 0x40000000u; + + /// + /// A Bad input holds the evaluator and publishes that input's own sub-code, once, carrying the + /// last known value. The evaluator-call count is the load-bearing half: "the result is Bad" could + /// also be produced by running the script against a null and failing, which is the NRE storm this + /// replaces. + /// + [Fact] + public void A_bad_input_holds_evaluation_and_publishes_the_inputs_sub_code() + { + var parent = CreateTestProbe(); + var evaluator = new CountingSumEvaluator(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a + b", evaluator: evaluator, dependencyRefs: ["a", "b"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + actor.Tell(new VirtualTagActor.DependencyValueChanged("b", 32, DateTime.UtcNow)); + parent.ExpectMsg().Value.ShouldBe(10); + parent.ExpectMsg().Value.ShouldBe(42); + var callsBefore = evaluator.Calls; + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + + var bad = parent.ExpectMsg(PresenceBudget); + bad.StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + bad.StatusCode.ShouldNotBe(OpcUaStatus.BadInternalError, "the script did not fail — its input did"); + bad.Value.ShouldBe(42, "the last known value rides out with the Bad status"); + evaluator.Calls.ShouldBe(callsBefore, "the script must NOT be run against a Bad input"); + + // Transition-once: the same Bad again changes nothing. + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + parent.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + } + + /// A different Bad sub-code republishes — the sub-code is the diagnostic. + [Fact] + public void A_different_bad_sub_code_is_republished() + { + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a + b", evaluator: new CountingSumEvaluator(), dependencyRefs: ["a"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + parent.ExpectMsg(); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + parent.ExpectMsg().StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadWaitingForInitialData)); + parent.ExpectMsg().StatusCode.ShouldBe(OpcUaStatus.BadWaitingForInitialData); + } + + /// + /// The vacuity-sensitive assertion. The input recovers holding the value it had before, so + /// the computed result is identical to the pre-outage one — and it must still be published Good, + /// because the node is currently Bad and only a publish can clear it. + /// + [Fact] + public void An_identical_value_is_republished_when_recovering_from_a_bad_input() + { + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a", evaluator: new CountingSumEvaluator(), dependencyRefs: ["a"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + parent.ExpectMsg().Value.ShouldBe(10); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + parent.ExpectMsg().StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + + // Same value as before the outage. + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + + var recovered = parent.ExpectMsg(PresenceBudget); + recovered.Value.ShouldBe(10, "the same value as before — that is the point"); + recovered.StatusCode.ShouldBe(OpcUaStatus.Good); + } + + /// + /// An Uncertain input is usable: the script runs and the output inherits the Uncertain code. The + /// script also sees the real code — a capturing evaluator asserts it, because a VT author writing + /// a quality-aware script is the reason the read cache had to stop fabricating Good. + /// + [Fact] + public void An_uncertain_input_is_evaluated_and_stamps_the_output_uncertain() + { + var parent = CreateTestProbe(); + var evaluator = new CountingSumEvaluator(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a", evaluator: evaluator, dependencyRefs: ["a"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + parent.ExpectMsg(); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 11, DateTime.UtcNow, Uncertain)); + + var result = parent.ExpectMsg(PresenceBudget); + result.Value.ShouldBe(11, "the script DID run — Uncertain is usable"); + result.StatusCode.ShouldBe(Uncertain); + evaluator.LastSeenStatus.ShouldBe(Uncertain, "the script must observe the input's REAL status"); + } + + /// + /// The R2-03 inputs-ready gate is preserved: with one declared dependency never pushed, a Bad + /// arrival on the other publishes nothing — cold start is not a fault. + /// + [Fact] + public void Nothing_is_published_while_a_declared_dependency_has_never_arrived() + { + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a + b", evaluator: new CountingSumEvaluator(), dependencyRefs: ["a", "b"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + + parent.ExpectNoMsg(TimeSpan.FromMilliseconds(400)); + } + + /// + /// The inputs-bad outcome counter must tick only when a tag actually reports Bad + /// inputs — not on every warm-up trigger, where a declared dependency simply has not arrived yet. + /// Metering above the readiness gate turns ordinary cold start into a stream of "inputs bad" + /// measurements, which is exactly the noise that makes an operator stop trusting the counter. + /// The absence half is paired with a positive control in the same test — the counter DOES + /// tick once the tag reports — so "no measurements" can never pass for want of a working listener. + /// + [Fact] + public void The_inputs_bad_counter_does_not_tick_during_warm_up() + { + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a + b", evaluator: new CountingSumEvaluator(), dependencyRefs: ["a", "b"])); + using var meter = new OutcomeRecorder(); + + // Warm-up: "b" has never arrived, so this is cold start, not a fault. + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + parent.ExpectNoMsg(TimeSpan.FromMilliseconds(400)); + + meter.Count("inputs-bad").ShouldBe(0, "warm-up is not an inputs-bad report"); + + // Positive control: once every dep has arrived, the tag reports — and the counter ticks. + actor.Tell(new VirtualTagActor.DependencyValueChanged("b", 32, DateTime.UtcNow)); + parent.ExpectMsg(PresenceBudget) + .StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + AwaitCondition(() => meter.Count("inputs-bad") == 1, TimeSpan.FromSeconds(3)); + } + + /// + /// A deploy's ReassertValue re-emits the LAST PUBLISHED status, so a VT whose input is + /// comms-lost re-asserts BadNoCommunication onto the freshly-materialised node rather than + /// Good. Re-asserting Good here would re-introduce the stale-Good lie at every deploy. + /// + [Fact] + public void Reassert_carries_the_last_published_status() + { + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a", evaluator: new CountingSumEvaluator(), dependencyRefs: ["a"])); + + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", 10, DateTime.UtcNow)); + parent.ExpectMsg(); + actor.Tell(new VirtualTagActor.DependencyValueChanged("a", null, DateTime.UtcNow, OpcUaStatus.BadNoCommunication)); + parent.ExpectMsg(); + + actor.Tell(VirtualTagActor.ReassertValue.Instance); + + var reassert = parent.ExpectMsg(PresenceBudget); + reassert.IsReassert.ShouldBeTrue(); + reassert.StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + reassert.Value.ShouldBe(10); + } + + /// + /// The batched mux delivery goes through the same gate: a burst whose worst item is Bad holds the + /// script and publishes that sub-code. A fix applied only to the single-value path would be inert + /// in production, where the mux batches. + /// + [Fact] + public void A_batch_containing_a_bad_item_holds_evaluation_too() + { + var parent = CreateTestProbe(); + var evaluator = new CountingSumEvaluator(); + var actor = parent.ChildActorOf(VirtualTagActor.Props( + "vt-1", "a + b", evaluator: evaluator, dependencyRefs: ["a", "b"])); + var ts = DateTime.UtcNow; + + actor.Tell(new VirtualTagActor.DependencyValueBatch(new[] + { + new VirtualTagActor.DependencyValueChanged("a", 10, ts), + new VirtualTagActor.DependencyValueChanged("b", 32, ts), + })); + parent.ExpectMsg().Value.ShouldBe(42); + var callsBefore = evaluator.Calls; + + actor.Tell(new VirtualTagActor.DependencyValueBatch(new[] + { + new VirtualTagActor.DependencyValueChanged("a", null, ts, OpcUaStatus.BadNoCommunication), + new VirtualTagActor.DependencyValueChanged("b", 33, ts), + })); + + parent.ExpectMsg(PresenceBudget) + .StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); + evaluator.Calls.ShouldBe(callsBefore); + } + + /// + /// Counts otopcua.virtualtag.eval measurements by their outcome tag. + /// A sees every measurement in the process, and this counter is + /// hot across the assembly — so the assertions above are scoped to the inputs-bad outcome, + /// which only this suite produces (nothing else drives a Bad dependency), and the absence + /// assertion is paired with a positive control rather than standing alone. + /// + private sealed class OutcomeRecorder : IDisposable + { + private readonly MeterListener _listener; + private readonly List _outcomes = new(); + private readonly object _gate = new(); + + public OutcomeRecorder() + { + _listener = new MeterListener + { + InstrumentPublished = (instrument, listener) => + { + if (instrument.Meter.Name == OtOpcUaTelemetry.MeterName + && instrument.Name == "otopcua.virtualtag.eval") + listener.EnableMeasurementEvents(instrument); + }, + }; + _listener.SetMeasurementEventCallback((_, _, tags, _) => + { + foreach (var tag in tags) + { + if (tag.Key != "outcome" || tag.Value is not string outcome) continue; + lock (_gate) _outcomes.Add(outcome); + } + }); + _listener.Start(); + } + + public int Count(string outcome) + { + lock (_gate) return _outcomes.Count(o => o == outcome); + } + + public void Dispose() => _listener.Dispose(); + } + + /// + /// Sums the dependency values, counts its invocations, and records the status code the actor + /// handed it for "a" — so a test can assert both that the script did NOT run and that, when it + /// did, it saw the input's real quality. + /// + private sealed class CountingSumEvaluator : IVirtualTagEvaluator + { + private int _calls; + + public int Calls => Volatile.Read(ref _calls); + + public uint? LastSeenStatus { get; private set; } + + public VirtualTagEvalResult Evaluate( + string virtualTagId, string expression, IReadOnlyDictionary dependencies) + { + Interlocked.Increment(ref _calls); + return VirtualTagEvalResult.Ok(Sum(dependencies.Values)); + } + + public VirtualTagEvalResult Evaluate( + string virtualTagId, string expression, IReadOnlyDictionary dependencies) + { + Interlocked.Increment(ref _calls); + LastSeenStatus = dependencies.TryGetValue("a", out var a) ? a.StatusCode : null; + return VirtualTagEvalResult.Ok(Sum(dependencies.Values.Select(v => v.Value))); + } + + private static object Sum(IEnumerable values) => + values.Where(v => v is not null).Sum(v => Convert.ToInt32(v)); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs index bb235899..3b759cf7 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs @@ -201,6 +201,38 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase value.ServerTimestampUtc.ShouldBe(ts); } + /// + /// Archreview 02/S14 — a result carrying a propagated INPUT status (BadNoCommunication, + /// because the VT held evaluation on a comms-lost dependency) is bridged verbatim to the publish + /// actor and historized with that status, the same way 02/S13's script-failure Bad is. + /// A bridge that special-cased status — or historized it as Good — would put the stale-Good lie + /// back one hop downstream of the fix. + /// + [Fact] + public void A_result_carrying_a_propagated_input_status_is_bridged_and_historized_with_it() + { + const uint badNoCommunication = 0x80310000u; + var publish = CreateTestProbe(); + var writer = new CapturingHistoryWriter(); + var host = Sys.ActorOf(VirtualTagHostActor.Props(publish.Ref, mux: null, new StubEvaluator(), writer)); + + host.Tell(new VirtualTagHostActor.ApplyVirtualTags( + new[] { PlanH("vt-1", "eq-1", "speed-rpm", historize: true) })); + + var ts = new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc); + host.Tell(new VirtualTagActor.EvaluationResult( + "vt-1", 42.0, ts, CorrelationId.NewId(), badNoCommunication)); + + var update = publish.ExpectMsg(); + update.NodeId.ShouldBe("eq-1/speed-rpm"); + update.StatusCode.ShouldBe(badNoCommunication); + update.Value.ShouldBe(42.0, "the last known value rides out under the Bad status"); + + AwaitAssert(() => writer.Calls.Count.ShouldBe(1)); + writer.Calls.TryPeek(out var captured).ShouldBeTrue(); + captured.Item2.StatusCode.ShouldBe(badNoCommunication); + } + /// H5c: a result for a vtag whose plan has Historize=false is NOT recorded — the writer /// is never called — though the value is still published. [Fact]