Merge branch 'fix/r3-02-stale-good-family' — R3-02: one quality contract at four seams — comms loss marks raw+UNS value variables BadNoCommunication (WriteStatuses sink member, recovery audit), Calculation + VT hold on Bad / worst-of sub-code / (value,status) dedup, MQTT initial push + ConnectionLost stale, Sparkplug birth replay (archreview 03/S15, 05/STAB-19, 05/STAB-18, 02/S14)
This commit is contained in:
Joseph Doherty
@@ -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<string, DataValueSnapshot>' to 'IReadOnlyDictionary<string, object?>'` (×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<string, DataValueSnapshot>' to 'IReadOnlyDictionary<string, object?>'` (×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 {<Terminated>…}` — 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.
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user