diff --git a/docs/plans/2026-06-18-f10b-surgical-tag-attribute-writes-design.md b/docs/plans/2026-06-18-f10b-surgical-tag-attribute-writes-design.md new file mode 100644 index 00000000..2bdb475b --- /dev/null +++ b/docs/plans/2026-06-18-f10b-surgical-tag-attribute-writes-design.md @@ -0,0 +1,221 @@ +# F10b surgical in-place tag-attribute writes (Writable / Historizing) — Design + +**Date:** 2026-06-18 +**Branch:** `feat/f10b-surgical-tag-attribute-writes` (off master `bbba911b`) +**Backlog item:** `stillpending.md` §A #11 (F10b — the deferred surgical-write tier; **safe subset**, user-approved) + +The next F10b slice after the shipped vtag rebuild-skip. Scope was narrowed by exploration + a user +scope-decision to the **safe, valuable subset**: surgical in-place updates for equipment-tag **`Writable`** +and **`IsHistorized`/`HistorianTagname`** toggles. Explicitly **excluded**: +- **Alarm severity** — a deploy-time authored severity is immediately overwritten by the live alarm engine + (`ScriptedAlarmHostActor` → `AlarmStateUpdate` → `SetSeverity`), so a surgical write is operationally + invisible. Dropped. +- **Tag `DataType` / `IsArray` / `ArrayLength`** — changing a node's declared type/rank in place leaves the + cached `Value` briefly type-mismatched and clients get no `ModelChangeEvents`; "dirty" for rare edits. + These keep the full rebuild. + +### Why this differs from the shipped vtag-skip + +The vtag-skip was a *pure skip* — the vtag node was byte-identical, so skipping `RebuildAddressSpace` + +letting the idempotent `Materialise*` passes no-op was correct. A tag-attribute change is different: an +attribute (AccessLevel / Historizing) **actually changes**, and `MaterialiseEquipmentTags` → +`EnsureVariable` **early-returns on an existing node** (`OtOpcUaNodeManager:1324`), so it will NOT update an +existing node's attributes. Therefore this slice must **actively mutate the existing node** (a real surgical +write) — skipping the rebuild *without* a surgical write would leave a stale node (the H1a bug). The win +remains the same: a `Writable`/`Historize` toggle no longer tears down + rebuilds the whole server address +space, preserving every client's subscriptions. + +--- + +## Standing constraints (in force) + +- NO Commons wire/proto contract change · NO **breaking** interface change (we ADD a new optional interface, + we do NOT modify `IOpcUaAddressSpaceSink`) · NO EF migration · NO bUnit. +- Stage by explicit path (never `git add .`); never stage the never-stage files. No force-push / no + `--no-verify`. Finish = merge + push. `dangerouslyDisableSandbox: true` for build/test/rig. + +--- + +## Component A — the surgical-write capability (node manager + sink) + +### New optional interface (Commons, NON-breaking) + +`src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs`: + +```csharp +namespace ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +/// Optional capability on an address-space sink: surgical in-place attribute updates on an +/// EXISTING variable node, used by Phase7Applier to avoid a full RebuildAddressSpace for pure-property +/// tag changes (Writable / Historizing). A sink that does not implement it ⇒ caller falls back to a +/// full rebuild (safe default). +public interface ISurgicalAddressSpaceSink +{ + /// Update an existing variable node's Writable (AccessLevel + inbound-write handler) and + /// Historizing (+ historian-tagname binding) IN PLACE, notifying subscribers (ClearChangeMasks) + /// without a rebuild. Returns false if the node does not exist (caller should rebuild instead). + bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname); +} +``` + +We add a NEW interface rather than a method on `IOpcUaAddressSpaceSink` so no existing implementer breaks, +and the `is ISurgicalAddressSpaceSink` probe gives a built-in rebuild fallback. + +### `OtOpcUaNodeManager.UpdateTagAttributes` (mirrors `EnsureVariable` exactly) + +`EnsureVariable` (`:1318-1370`) composes attributes thus: `historized = historianTagname is not null`; +`access = writable ? (CurrentRead|CurrentWrite) : CurrentRead`, `|= HistoryRead` if historized; +`Historizing = historized`; `OnWriteValue = writable ? OnEquipmentTagWrite : null`; and +`_historizedTagnames[nodeId] = historianTagname` when historized. + +To prevent drift, **extract a private static `ComposeAccessLevel(bool writable, bool historized) -> byte`** +used by both `EnsureVariable` and the new method. Then: + +```csharp +public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) +{ + lock (Lock) + { + if (!_variables.TryGetValue(variableNodeId, out var v)) return false; // caller falls back to rebuild + var historized = historianTagname is not null; + var access = ComposeAccessLevel(writable, historized); + v.AccessLevel = access; + v.UserAccessLevel = access; + v.Historizing = historized; + v.OnWriteValue = writable ? OnEquipmentTagWrite : null; // attach/detach the inbound-write handler + if (historized) _historizedTagnames[variableNodeId] = historianTagname!; + else _historizedTagnames.TryRemove(variableNodeId, out _); + v.ClearChangeMasks(SystemContext, includeChildren: false); // notify subscribers; node object preserved + return true; + } +} +``` + +This mutates the SAME `BaseDataVariableState` object that existing client MonitoredItems reference (the +proven `WriteValue`/`ClearChangeMasks` pattern), so subscriptions survive. ModelChangeEvent emission is out +of scope — consistent with the rest of the server (it doesn't emit them on rebuild either). + +### `SdkAddressSpaceSink` implements `ISurgicalAddressSpaceSink` + +Add `: ISurgicalAddressSpaceSink` and a one-line delegation: +`public bool UpdateTagAttributes(string n, bool w, string? h) => _nodeManager.UpdateTagAttributes(n, w, h);` +`NullOpcUaAddressSpaceSink` does NOT implement it (→ Phase7Applier rebuilds, a no-op on Null). + +--- + +## Component B — Phase7Applier surgical-eligibility + apply/rebuild-fallback + +### Eligibility (whitelist-of-may-differ, same safe-default discipline as the vtag-skip) + +`EquipmentTagPlan` uses auto-generated record equality. A changed tag delta is **surgical-eligible** iff it +is a plain value variable (no condition node) AND its ONLY differences are `Writable` / `IsHistorized` / +`HistorianTagname`: + +```csharp +private static bool TagDeltaIsSurgicalEligible(Phase7Plan.EquipmentTagDelta d) => + d.Previous.Alarm is null && d.Current.Alarm is null && // value variables only (alarm presence ⇒ structural) + (d.Previous with + { + Writable = d.Current.Writable, + IsHistorized = d.Current.IsHistorized, + HistorianTagname = d.Current.HistorianTagname, + }).Equals(d.Current); +``` + +Any other difference — `DataType`, `IsArray`/`ArrayLength`, `FullName`, `DriverInstanceId`, identity +(`TagId`/`EquipmentId`/`FolderPath`/`Name`), or `Alarm` — makes the override unequal ⇒ **rebuild** (safe +default; covers future fields too). + +### Control flow in `Apply` + +`needsRebuild`'s `ChangedEquipmentTags.Count > 0` term becomes +`ChangedEquipmentTags.Any(d => !TagDeltaIsSurgicalEligible(d))` (mirroring the existing vtag term). Then: + +``` +structuralRebuild = +surgicalTagDeltas = plan.ChangedEquipmentTags.Where(TagDeltaIsSurgicalEligible) +rebuilt = false +if (structuralRebuild) { _sink.RebuildAddressSpace(); rebuilt = true; } +else if (surgicalTagDeltas.Any()) +{ + if (_sink is ISurgicalAddressSpaceSink surgical) + { + var allApplied = true; + foreach (var d in surgicalTagDeltas) + { + var nodeId = EquipmentNodeIds.Variable(d.Current.EquipmentId, d.Current.FolderPath, d.Current.Name); + var writable = d.Current.Writable && !d.Current.IsArray; // mirror MaterialiseEquipmentTags + var historian = d.Current.IsHistorized + ? (string.IsNullOrWhiteSpace(d.Current.HistorianTagname) ? d.Current.FullName : d.Current.HistorianTagname) + : null; + if (!surgical.UpdateTagAttributes(nodeId, writable, historian)) { allApplied = false; break; } + } + if (!allApplied) { _sink.RebuildAddressSpace(); rebuilt = true; } // anomaly (node missing) ⇒ safe rebuild + } + else { _sink.RebuildAddressSpace(); rebuilt = true; } // sink lacks the capability ⇒ rebuild +} +return new Phase7ApplyOutcome(removedCount, addedCount, changedCount, RebuildCalled: rebuilt); +``` + +`changedCount`/`ChangedNodes` is unchanged (the edit still counts). The effective `writable`/`historian` +computation is byte-identical to `MaterialiseEquipmentTags` so the surgical node matches what a rebuild +would have produced. + +### Note on the downstream Materialise* passes + +`OpcUaPublishActor.HandleRebuild` still runs `Materialise*` unconditionally after `Apply`. On the surgical +path the node already exists, so `EnsureVariable` early-returns — it will NOT clobber the surgical +attributes. Correct. + +--- + +## Testing + +### Phase7Applier (offline, `RecordingSink` now also implements `ISurgicalAddressSpaceSink`) + +Extend the test `RecordingSink` to implement `ISurgicalAddressSpaceSink`, recording +`(nodeId, writable, historianTagname)` of each `UpdateTagAttributes` call (and a separate +`RecordingSink` variant that does NOT, to test the fallback). Facts: +- **Writable-only** toggle (non-array, non-alarm) ⇒ no rebuild; one `UpdateTagAttributes(writable=new, historian=…)`. +- **IsHistorized** toggle ⇒ no rebuild; `UpdateTagAttributes(historian = FullName-or-override / null)`. +- **HistorianTagname-only** change (already historized) ⇒ no rebuild; `UpdateTagAttributes(historian=new)`. +- **DataType** change ⇒ rebuild (excluded). +- **IsArray / ArrayLength** change ⇒ rebuild. +- **FullName** change ⇒ rebuild (excluded from may-differ). +- **Name / FolderPath / EquipmentId** change ⇒ rebuild. +- **Alarm presence** change (null↔non-null) ⇒ rebuild. +- surgical-eligible delta **mixed with another change** ⇒ rebuild. +- sink **without** `ISurgicalAddressSpaceSink` + surgical-eligible delta ⇒ rebuild (fallback). +- `UpdateTagAttributes` returns **false** (node missing) ⇒ rebuild (fallback). + +### OtOpcUaNodeManager (if a unit harness exists) + +If the node manager can be constructed in a test (check for an existing fixture), add a direct test: create +a variable via `EnsureVariable`, call `UpdateTagAttributes` to flip writable/historized, assert +`AccessLevel`/`UserAccessLevel`/`Historizing`/`OnWriteValue`(attached/null)/`_historizedTagnames` updated + +`ComposeAccessLevel` parity with `EnsureVariable`. If no harness exists, rely on the Phase7Applier tests + +the live `/run`. + +### Live `/run` (confirmatory — proves the SDK honors it) + +docker-dev runs the real `SdkAddressSpaceSink`. Rebuild BOTH central nodes (the `:9200` round-robin fact), +subscribe a Client.CLI to an equipment tag, toggle that tag's Historize (or Writable) in `/uns`, Deploy → +the server log shows `rebuild=False` for that deploy, the subscription is NOT dropped, the server stays +healthy, and the node remains readable. Confirms the surgical mutation + subscription survival. + +--- + +## Task slicing + +| Task | Component | Project(s) | Class | Depends on | +|---|---|---|---|---| +| T1 | `ISurgicalAddressSpaceSink` + `OtOpcUaNodeManager.UpdateTagAttributes` (+ `ComposeAccessLevel` extract) + `SdkAddressSpaceSink` impl | Commons + OpcUaServer | standard | — | +| T2 | Phase7Applier `TagDeltaIsSurgicalEligible` + apply/rebuild-fallback control flow + RecordingSink tests | OpcUaServer (+ .Tests) | standard | T1 (interface) | +| T3 | Reconcile stillpending #11 + memory + build + tests + live `/run` + finish (merge+push) | docs (never-staged) | small | T1, T2 | + +T1→T2 are coupled by the new interface ⇒ serial. T3 last. + +## Done = + +Build clean + `dotnet test` green (Commons + OpcUaServer.Tests) + live `/run` shows a Writable/Historize +toggle deploy logs `rebuild=False` with subscriptions intact + merged to master + pushed.