C1 — a non-rebuild apply reporting failedNodes > 0 no longer advances _lastApplied.
Pre-delta-materialisation the full passes re-ensured every node on every apply, which
silently repaired any prior swallowed sink fault. The delta path removed that accidental
self-healing: a failed node sat inside _lastApplied, never appeared in a diff set again,
and EnsureVariable early-returns on the ones that landed — so it was lost until an
unrelated change forced a rebuild. The advance also moved to AFTER the materialise
passes, so a throwing pass leaves _lastApplied at the previous composition too. A
rebuild still advances even with failures (the ratchet is its resync path).
I2 — a UnsReferenceDelta whose identity is unchanged (same NodeId, EquipmentId and
BackingRawPath) is now applied in place via UpdateTagAttributes(Uns) instead of
remove+recreate, so the mirror's MonitoredItems survive an operator historizing or
reshaping the raw tag behind them. Remove+recreate stays for identity-changing deltas
(re-point, effective-name edit).
I3 — RemoveVariableNode severs the inverse half of every outgoing Organizes edge, the
variable-node counterpart of UnwireAlarmNotifiers. Rebuilds used to sweep these up;
surgical applies made rebuilds rare, so they accumulated on the raw node for the life of
the process.
I5 — added UNS reference variables are announced (their equipment folder is the parent,
already the Uns realm). Raw adds still are not; the gap comment now says so honestly
rather than calling it neutral and pre-existing.
I4 + M1 — corrected two comments that no longer described the code: the "re-point →
rebuild" claim in ApplyPureRemove, and the "N lock acquisitions" rationale for the delta
passes (Ensure* early-return on a dictionary probe before the lock).
Adversarial review of 9f68f272 (classifier: non-alarm raw-tag deltas became
surgical) found stale UNS inheritance after a surgical raw edit. Editing a raw
tag's IsHistorized / HistorianTagname / IsArray / ArrayLength left every
referencing UNS node STALE: none of those fields were on UnsReferenceVariable,
so no coincident UnsReferenceDelta fired; the applier updated the raw node in
place; and EnsureVariable early-returns on an existing node. Consequences:
HistoryRead through the UNS NodeId served a different series than through the
raw NodeId (breaking the documented dual-registration invariant), a
de-historized tag kept serving history via UNS, and a scalar->array flip left
the UNS mirror scalar AND writable (bypassing the array read-only write gate).
UnsReferenceVariable now carries Backing{IsHistorized,HistorianTagname,IsArray,
ArrayLength}, copied from the backing tag's TagConfigIntent at compose time, so
record equality makes the reference Changed in the SAME plan. Both deltas are
non-rebuild kinds, so the existing machinery repairs the mirror surgically:
remove-previous in Apply, recreate in MaterialiseUnsReferences. That pass now
reads the four values from the RECORD (the single truth the planner diffed)
instead of a rawByPath back-lookup, preserving historian resolution exactly
(blank override => BackingRawPath, which is the backing tag's NodeId).
The artifact-decode seam needs no change: DeploymentArtifact.BuildRawAndUnsSubtrees
drives AddressSpaceComposer.Compose verbatim, which is what guarantees byte-parity.
The classifier needs no change either (a UnsReferenceDelta never forces Rebuild);
its comment now records why.
Also from the same review: a three-place-contract comment on RawTagPlan's
hand-written Equals naming its two downstream consumers, plus classifier pins for
the .Any(!eligible) quantifier, an ArrayLength-only delta, and an
alarm-content-only delta.
ConfigComposer gzips the serialised snapshot (CompressionLevel.Fastest)
immediately after SerializeToUtf8Bytes and BEFORE taking the SHA-256, so
the compressed bytes ARE the artifact everywhere: Deployment.ArtifactBlob,
the LocalDb cache chunks, the gRPC fetch stream, and the RevisionHash a
FetchAndCache node verifies against. The invariant the deploy path rests on
— RevisionHash == SHA-256(stored/wire bytes) — is preserved, which is what
leaves the fetcher, the cache and the serve path untouched: nothing between
the composer and DeploymentArtifact.Open ever looks inside the blob.
Read side: Open() sniffs the gzip magic 1F 8B (valid JSON can never start
with 0x1F, so no framing of our own is needed) and inflates through a size
guard — MaxDecompressedBytes, 512 MiB, internal + mutable so the guard can
be exercised without inflating half a gigabyte per test run. Pre-upgrade
uncompressed blobs take the identical path they always did.
Every decompression failure — corrupt stream, truncated stream, over-limit
inflation — is reported as JsonException, the same exception family the
malformed-JSON path already owns. So the lenient static wrappers still
degrade to empty results and the apply paths still fail the apply and keep
last-known-good (#485); no caller's decision changes because the reason the
bytes are unusable changed.
gzip output is not guaranteed byte-stable across .NET releases. Harmless
here: the hash is computed FROM the emitted bytes at compose time and never
re-derived from the JSON, so the worst an implementation change costs is
one spurious "changed" revision after a runtime upgrade. Snapshot
determinism is unaffected — compression wraps the final bytes and reorders
nothing.
Tests: gzip/uncompressed parity through both the instance parses and the
statics, corrupt-gzip leniency through the wrappers, the size guard (with a
falsifiability control proving the same bytes inflate fine under the
production limit), and composer-side blob-starts-with-1F8B +
RevisionHash == SHA256(blob) with a control proving the hash is NOT over
the JSON.
Task 4 (9f68f272) taught AddressSpaceChangeClassifier that a non-alarm
RawTagDelta and any UnsReferenceDelta are surgical-eligible, so such plans
started classifying AttributeOnly/PureAdd instead of Rebuild. The applier's
surgical branch did not know about either delta set, so those deploys fell
through as SILENT NO-OPS (or, on a non-surgical sink, applied nothing at all
where a rebuild was the safe answer). This closes that gap:
- Raw-tag loop: an eligible ChangedRawTags delta is applied IN PLACE via
ISurgicalAddressSpaceSink.UpdateTagAttributes in the RAW realm, computing
writable (`Writable && !IsArray`) and the historian tagname (blank override
⇒ the RawPath) exactly as MaterialiseRawSubtree does. A delta whose ONLY
difference is ReferencingEquipmentPaths is node-inert for a value tag and
is SKIPPED entirely — that is the shape a single UnsTagReference add/drop
produces, and it used to rebuild the world.
- UNS-reference loop: a ChangedUnsReferenceVariables delta removes the
PREVIOUS node in place (terminal Bad, then RemoveVariableNode, Uns realm).
Remove+recreate rather than an in-place update because a re-point must move
the Organizes edge (the sink has no RemoveReference) and a display-name
edit changes the NodeId itself; the cost is that one node's monitored
items, versus every subscription server-wide today.
- The branch trigger now fires for both sets, so a non-surgical sink falls
back to a full rebuild instead of applying nothing.
Every new operation is inside the existing one-way rebuild ratchet: any false
or throw ⇒ SafeRebuild(). The eligibility filter is defense-in-depth — when
kind != Rebuild the classifier already guarantees every delta is eligible.
The recreate of a changed UNS reference is NOT done here. Until Task 6 the
publish actor still materialises the FULL composition after Apply returns, so
the existing MaterialiseUnsReferences pass recreates it and the system is
correct end to end after this commit; the remove-then-recreate ordering holds
because Apply returns before the materialise passes run. Task 6 narrows those
passes to the plan's delta and keeps the same ordering.
Tests: 7 new applier tests (in-place raw update incl. array/override parity,
reference-only skip, UNS-ref remove, both ratchet fallbacks, non-surgical-sink
rebuild). RecordingSink now captures the realm on surgical / remove / value-write
calls, so the existing UNS-realm assertions are explicit too.
Every public entry point on DeploymentArtifact took ReadOnlySpan<byte> and did
JsonDocument.Parse(blob.ToArray()) — a full copy plus a full document build per
call — so one apply paid ~8-11 full parses of the same bytes (the scoped
ParseComposition alone parsed three times: scope, composition, BuildClusterSets).
DeploymentArtifact.Open(byte[]) now parses once into ParsedDeploymentArtifact,
which owns the JsonDocument and exposes an instance method mirroring each static
entry point. The moved bodies operate on Root; BuildClusterSets takes JsonElement
instead of re-parsing the blob; ResolveClusterScope is memoized per node id, since
every consumer of one apply asks about the same node.
Every static is now a thin wrapper (Open + delegate + dispose) keeping its
ReadOnlySpan<byte> signature and its lenient empty/malformed contract — the
wrapper pays the one copy and owns the JsonException catch, so output shapes are
unchanged. Hot callers migrate to Open() separately.
ParsedDeploymentArtifactTests pins same-bytes-same-output through both doors for
all six entry points over a rich multi-cluster artifact (raw chain + UNS refs +
scripts + keys + ACLs), with falsifiability controls proving the fixture actually
scopes, plus memoization and after-dispose-use coverage.
Accumulated fast-follows from the Task 1-11 reviews. No behaviour change on the
value path.
- Strengthen OpcUaPublishActorBatchTests.Sink_throw_is_swallowed_and_logged. The
survival assertion alone did NOT prove the catch exists: Akka's default
supervision RESTARTS a throwing actor and the restart preserves the mailbox, so
the follow-up message landed either way and the test passed against a build with
no catch at all. It now asserts via EventFilter that the exception NEVER reaches
supervision (Expect(0) on the InvalidOperationException) and that the handler's
own Warning IS logged, keeping the survival assertion. Everything that must have
happened goes inside the Expect(0) action — it counts matches at the end of the
action, so an assertion left outside would race the actor and pass vacuously.
Verified falsifiable: deleting the catch turns the test red.
- MapKey(NodeId): the non-string identifier branch collapsed to a string.Empty key
silently. Such a key cannot match anything the manager stored, so the caller sees
an unexplained miss rather than an error. It now logs a Utils.LogWarning naming
the seam. Every current call site is provably string-only, so this should never
fire.
- The two WireAlarmNotifiers Utils.LogInfo sites interpolated the RealmKey struct,
rendering a raw struct dump. Interpolate .Id and .Realm as separate placeholders
so log-grep patterns stay consistent with the file's other sites.
- Stale comments: the _eventNotifierSources key is a RealmKey now, not a
namespace-qualified NodeId string; and three comments still named
DriverHostActor.ForwardToMux, which Task 8 replaced with ForwardBatchToMux.
DependencyMuxActor now receives DriverInstanceActor.AttributeValueBatchPublished — the
primary value ingress since DriverHostActor's batched fan-out — and filters it per item,
emitting one DependencyValueChanged per genuinely interested (item, subscriber) pair with
that item's own value/timestamp/status sub-code (#519).
The per-item AttributeValuePublished receive stays: DriverHostActor's comms-loss fan
fabricates it, and per-tag drivers still take that path. Indexed for-loop over Items to
avoid an interface-enumerator allocation per burst.
Replaces the temporary per-item unwrap bridge with a real batched handler: one
AttributeValueBatchPublished burst now costs ONE hop to the dependency mux and ONE
OpcUaPublishActor.AttributeValueBatchUpdate, instead of one message per value.
Routing per item is unchanged — the v3 Batch 4 single-source fan-out (raw NodeId +
every referencing UNS NodeId, identical value/status/timestamp), with an unmapped ref
Debug-logged and dropped without disturbing the rest of the burst. A burst in which
nothing resolves sends no publish message at all rather than an empty batch.
Indexed for-loop over Items is deliberate: no interface-enumerator allocation per
burst on the hottest routing seam. ForwardToMux and its registrations stay for the
comms-loss fan and per-tag drivers.
The /admin/secrets link was already in the side rail, but ungated — every
authenticated user saw it, and a Designer/Operator clicking it was bounced by
the page's own [Authorize(Policy = SecretsAuthorization.ManagePolicy)]. Hide
the advertised dead end.
Gated on that same policy constant rather than Roles="Administrator". The two
are equivalent today (the RCL's AdminRole is "Administrator", read off the same
ClaimTypes.Role claim FleetAdmin uses), so this is not about reach — it is that
the rail cannot drift if the RCL ever changes its policy.
Live-gated on docker-dev, both directions, because the positive half alone
proves nothing here: the link rendered for an admin BEFORE this change too, so
"admin still sees it" is satisfied equally by a working gate and by an inert
one. Negative control — gate swapped to Roles="NoSuchRoleFalsifyProbe", rebuilt,
item went to 0 while /role-grants and /apikeys stayed at 1 (the item is hidden,
not the rail broken). Restored build hashed to the same image ID as the
pre-falsification one, proving the restore was byte-exact. This also rules out
AuthorizeView being inert inside ThemeShell's <Nav> slot under this host's
static router, which would have silently hidden the link from everyone.
SecretsNavLinkTests covers it in the AdminUI's actual idiom — source scan +
metadata cross-check, NOT bUnit (the bunit PackageVersion pin has zero
PackageReferences repo-wide). The load-bearing assertion is index ordering
(AuthorizeView open < NavRailItem < close): a scan that merely finds both tags
in the file passes when they are not nested, which is the ungated state.
Scope: Secrets only. /role-grants and /apikeys are FleetAdmin (Administrator-
only) too and stay visible to everyone, so the rail is now inconsistent —
deliberate, as hiding those is a UX call for the whole rail rather than
something to smuggle into this change. Noted in the markup.
Family-wide task from the scadaproj index session (ScadaBridge ee193cd2,
ignitionoee 3627bb8, HistorianGateway 58b6418, mxgw 87d575d).
Reverses the conservative call from the previous commit at the user's direction,
and they were right: "surface it on a counter first" left the wire lying. An
MXAccess event can carry Good quality while its payload is a kind MxValueDecoder
has no mapping for, so the old behaviour published null-under-Good — which a
client cannot tell apart from a genuine null reading. A value the gateway DID
send would be consumed as a value it did not, with the only evidence in a log
nobody is tailing. Bad data is the honest wire representation of lost data.
BadTypeMismatch is an existing StatusCodeMap constant, deliberately reused rather
than written inline: StatusCodeParityTests finds status constants by reflecting
over Good/Uncertain/Bad-prefixed uint fields and checking them against the pinned
SDK, so an inline literal would be invisible to that guard (Gitea #497, where 16
wrong constants had accumulated across 6 drivers exactly this way).
Logging is now once per DISTINCT value kind rather than once per pump. The set is
bounded by the MxValue oneof enum so a broken producer still cannot flood, but a
second unrepresentable kind appearing later gets its own line — warn-once-per-pump
would have silently swallowed it, which is the same class of bug as the null this
commit fixes.
The test's fixture matters more than its assertion: the undecodable event carries
Quality = 192 (OPC DA Good), so a decoder that gave up silently would publish Good
and fail. Paired with a positive control — a decodable value at the same quality
must still publish Good — so it cannot be satisfied by a pump that marks
everything Bad. Falsified by reverting the status to Good: the test reds, and only
that test.
Verified: 331 Galaxy tests green, Core.Abstractions 298 (the parity guard),
solution builds 0 errors. Docs corrected in the two places that recorded the old
decision as deliberate.
Closes the two follow-ups left open by the MxGateway 0.2.0 bump. Reading the
gateway's own contract first changed what each of them is, and one of my earlier
claims was simply wrong -- both corrections are recorded below.
Replay gap
----------
A sentinel MxEvent carries family = UNSPECIFIED, so it fell through EventPump's
family switch into the default drop: we silently ignored a documented MUST ("you
missed events -- discard local state and re-snapshot"). It is now intercepted and
answered by re-issuing SubscribeBulk for every tracked subscription, since
MXAccess delivers an initial value per item on subscribe -- re-subscribing IS the
re-snapshot. It does not restart the event pump (the stream carrying the sentinel
is healthy, and the handler runs on that stream's own read loop, so disposing the
pump from there would deadlock on itself), it does not block the read loop, and
re-entry is gated so a burst collapses to one pass instead of a subscribe storm
per sentinel.
Handled on the READ loop, not in the dispatch loop, because everything past that
point is subject to newest-dropped channel pressure -- and "you missed events" is
the one signal whose loss is unrecoverable. Pinned by a test that fills the
channel and still expects the sentinel.
⚠️ This cannot fire in the shipped configuration, and I did not pretend otherwise
or paper over it by enabling resume. The gateway emits the sentinel only for a
stream opened with after_worker_sequence > 0; GatewayGalaxySubscriber passes 0,
which the gateway treats as "fresh stream, not a resume -- plain attach, no
replay, no sentinel". Enabling resume is a larger and genuinely delicate change:
the sequence is only meaningful within ONE gateway session, our reconnect
recreates the session, and replaying a remembered sequence against a fresh
session sets the server's live watermark ABOVE every event it is about to send --
silencing the stream instead of recovering it. That deserves its own change with
a live gate, which I cannot run from here.
Sparse arrays -- correcting the last commit message
---------------------------------------------------
I wrote that the bump "makes handling it possible" on the decode path. That is
wrong: MxSparseArray is documented write-only, the gateway expands it before
forwarding, and "the worker never receives or produces it". So the decode path
is unreachable by contract and the write path has no use for it either -- it is a
wire compaction for a WHOLE-array write, not a partial update (unmentioned
indices are RESET, deliberately, because the gateway cannot read-modify-write
without racing the provider). It is therefore the wrong primitive for an OPC UA
IndexRange write, which expects untouched elements preserved -- and WriteRequest
carries no IndexRange in the first place.
It is decoded anyway, as contract defence: the alternative if a future gateway
ever emits one is a silent null published under Good quality. Fill semantics
mirror the gateway's own SparseArrayExpander exactly (epoch for Time, empty
string, Integer widening to long[] when any element is int64) so a value that
round-trips through a write reads back as the same CLR array.
Also: the decoder is now total by contract, since it runs inside the dispatch
loop where an escaping exception kills dispatch for the life of the pump. A
malformed payload degrades instead of throwing.
A real defect found in the same code
------------------------------------
A stream the GATEWAY completes -- a closed or evicted session -- ends the RPC
without an error, so nothing threw, the supervisor was never notified, and the
pump exited silently. GetHealth() kept reporting Healthy, which also means the
#527 comms-loss watchdog could never trip, because it requires a non-Healthy
state. The driver went permanently deaf with no signal anywhere. This one is
live, unlike the two above. Now reported as a fault; the falsification control is
the existing CleanShutdown_DoesNotInvokeOnStreamFault, since our own disposal
also ends the stream and must NOT be reported.
Value kinds we cannot represent are now counted and warned once rather than
published as an indistinguishable null. The status is deliberately NOT downgraded
to Bad: nothing can reach that branch today, and changing what an OPC UA client
sees for a live shape is not a change to make without a gate. The counter exists
so the next person has evidence instead of a guess.
Verified: 330 Galaxy tests green (12 new), plus Galaxy.Browser 21, Runtime 558,
Core 351; full solution builds 0 errors with the warning census unchanged. Every
new test was falsified first -- disabling the gap interception reds 3 of 4 (the
throwing-callback test passes vacuously when the callback never runs, by
construction), disabling sparse decode plus the representability signal reds all
7 decoder tests, and both were restored and re-verified green.
Docs: the Galaxy.Performance invariant was stated as received = dispatched +
dropped + in-flight, which this breaks -- a sentinel is received but never enters
the channel -- so it now reads + replay_gaps. Galaxy.md gains the three
operational notes; DriverLifecycle.md records that "reaches a non-Healthy state"
is an obligation a driver can silently fail to meet, which is the general shape
of the clean-end bug.
Joseph Doherty