401 lines
51 KiB
Markdown
401 lines
51 KiB
Markdown
# Design & Implementation Plan — R2-07: Surgical pure-adds (and removals) in the address-space applier
|
||
|
||
> **Status (2026-07-12):** NOT STARTED — this is the round-2 plan. The declared prerequisite
|
||
> (03/U2 reflection-exhaustive deferred-sink forwarding guard, `DeferredSinkForwardingReflectionTests`)
|
||
> **is merged to master** and auto-covers every new sink capability member this plan adds, so the
|
||
> F10b/PR#423 inertness class is retired and the work is unblocked.
|
||
|
||
- **Source reports:** `archreview/03-server-runtime.md` (P1 — the primary finding), `archreview/01-core-composition.md` (applier/planner context, S-1, C-7), `archreview/00-OVERALL.md` prioritized action **#7** ("Surgical pure-adds in the applier — Large effort, High impact, biggest operational perf win").
|
||
- **Round-1 plan extended:** `archreview/plans/03-server-runtime-plan.md` §P1 (this plan supersedes its two-phase sketch with a full three-phase staged design).
|
||
- **Review commit:** `f6eaa267` · **Plan verified against tree at:** `f6eaa267` (master, clean tree; archreview/*.md working-tree updates from the 2026-07-12 re-review read in-place).
|
||
- **Scope:** `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/` (`AddressSpaceApplier`, `AddressSpacePlan`/`AddressSpacePlanner`, `OtOpcUaNodeManager`, `SdkAddressSpaceSink`), `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/` (`ISurgicalAddressSpaceSink`, `DeferredAddressSpaceSink`, `IOpcUaAddressSpaceSink`), `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs`, plus tests under `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/`, `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/`, `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/`.
|
||
|
||
## Verification summary
|
||
|
||
Every claim below re-verified by direct read at `f6eaa267`:
|
||
|
||
| # | Claim | Verified at |
|
||
|---|-------|-------------|
|
||
| 1 | Any add/remove of Equipment / ScriptedAlarm / EquipmentTag / EquipmentVirtualTag forces `structuralRebuild` → `SafeRebuild()` | `AddressSpaceApplier.cs:134-154` — the predicate ORs **every** `Added*`/`Removed*` count |
|
||
| 2 | `RebuildAddressSpace()` detaches + removes **every** `NodeState` (variables, conditions, folders, notifier refs) under one `Lock` | `OtOpcUaNodeManager.cs:1690-1736` |
|
||
| 3 | Recreated nodes are new `NodeState` instances ⇒ existing client MonitoredItems go permanently dead | SDK binds monitored items to `NodeState` instances; confirmed by the F10b design history and `ISurgicalAddressSpaceSink` doc (`ISurgicalAddressSpaceSink.cs:9-17`) |
|
||
| 4 | The surgical path today covers ONLY changed-tag attribute edits (`TagDeltaIsSurgicalEligible`) + folder DisplayName renames (`RenamedFolders`) — **not adds, not removes** | `AddressSpaceApplier.cs:155-199, 630-663` |
|
||
| 5 | The four Materialise passes run unconditionally after `Apply` returns, over the FULL composition, and are **idempotent** (`EnsureFolder` early-return `OtOpcUaNodeManager.cs:1290`, `EnsureVariable` early-return `:1376`, `MaterialiseAlarmCondition` drop-and-recreate guard `:596-604`) | `OpcUaPublishActor.cs:335-354` (passes at 338/343/349/354, gated on the `plan.IsEmpty` short-circuit at :328) |
|
||
| 6 | `RaiseNodesAddedModelChange` (Part 3 `GeneralModelChangeEvent`, verb `NodeAdded`, built under `Lock`, reported outside it) already exists, is on `IOpcUaAddressSpaceSink`, is forwarded by `DeferredAddressSpaceSink`, and is guard-covered | `OtOpcUaNodeManager.cs:1592-1654`, `IOpcUaAddressSpaceSink.cs:88-94`, `DeferredAddressSpaceSink.cs:49`; `NodeManagerModelChangeOnAddTests.cs` exists |
|
||
| 7 | The U2 prerequisite guard is merged: `DeferredSinkForwardingReflectionTests` (Commons.Tests) reflects over every member of every forwarding interface of `DeferredAddressSpaceSink` via a `DispatchProxy` recorder that **auto-implements future members**, plus a guard-of-the-guard pinning the interface set | `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredSinkForwardingReflectionTests.cs:29-75` |
|
||
| 8 | `FeedHistorizedRefs` + `ProvisionHistorizedTags` operate on the **plan's** add/remove/change deltas and fire at the end of `Apply` unconditionally — independent of rebuild-vs-surgical routing | `AddressSpaceApplier.cs:208-213, 226-353` |
|
||
| 9 | The driver-side routing maps (`_nodeIdByDriverRef` / `_driverRefByNodeId` / `_alarmNodeIdByDriverRef`) and desired-subscription sets are rebuilt **every apply** by `DriverHostActor.PushDesiredSubscriptions`, fully independent of the node-manager rebuild | `DriverHostActor.cs:107-158` (doc: "Rebuilt every apply") |
|
||
| 10 | Variable NodeIds are folder-scoped strings derived deterministically from composition (`EquipmentNodeIds.Variable/SubFolder`), all in the one server namespace (`NamespaceIndex`) — adds cannot shift existing NodeIds | `EquipmentNodeIds.cs`, `OtOpcUaNodeManager.cs:1299, 1391` |
|
||
| 11 | The applier already writes a terminal condition state (`RemovedConditionState`) to removed equipment/alarm condition nodes **before** the teardown | `AddressSpaceApplier.cs:83-98, 665-681` |
|
||
| 12 | 03/P1 is **STILL OPEN** at f6eaa267 ("guard merged, work not started") | `archreview/03-server-runtime.md` prior-finding table row P1 + §2 P1 |
|
||
|
||
**Conclusion: the finding fully reproduces.** Adding one tag to one equipment still tears down and recreates every node on the server, killing every client subscription server-wide. The striking verification result (claims 5+6) is that **Phase 1 (pure-adds) requires no new sink capability at all** — the idempotent Materialise passes already create exactly the new nodes when the rebuild is skipped; the only missing pieces are (a) a classifier that routes a pure-add plan away from `SafeRebuild()` and (b) a post-materialise `NodeAdded` model-change announcement. Removals (Phase 2) are where new sink members are needed.
|
||
|
||
---
|
||
|
||
## Restatement
|
||
|
||
`AddressSpaceApplier.Apply` treats every topology delta identically: any added or removed equipment, scripted alarm, equipment tag, or virtual tag sets `structuralRebuild = true`, which calls `sink.RebuildAddressSpace()` → `OtOpcUaNodeManager.RebuildAddressSpace()` clears all five node maps and detaches every `NodeState`. The subsequent Materialise passes recreate the world as **new** `NodeState` instances. OPC UA monitored items are bound to the old instances, so every client subscription on the server goes dead — for a deploy that added a single tag. On a production SCADA server with dozens of clients (including ScadaBridge's Data Connection Layer), every authoring action that touches topology is a server-wide subscription outage. The F10b surgical path (`ISurgicalAddressSpaceSink.UpdateTagAttributes` / `UpdateFolderDisplayName`) proves in-place mutation preserves subscriptions, but covers only in-place attribute edits and UNS folder renames.
|
||
|
||
## Verification
|
||
|
||
Confirmed — see the table above. Key mechanics re-checked at f6eaa267:
|
||
|
||
- **The rebuild predicate** (`AddressSpaceApplier.cs:134-140`): `AddedEquipment.Count > 0 || RemovedEquipment.Count > 0 || ChangedEquipment.Count > 0 || AddedAlarms/RemovedAlarms/ChangedAlarms || AddedEquipmentTags/RemovedEquipmentTags || ChangedEquipmentTags.Any(!TagDeltaIsSurgicalEligible) || AddedEquipmentVirtualTags/RemovedEquipmentVirtualTags || ChangedEquipmentVirtualTags.Any(!VtagDeltaIsNodeIrrelevant)`. Pure adds land in this branch.
|
||
- **The apply pipeline** (`OpcUaPublishActor.HandleRebuild`, `OpcUaPublishActor.cs:290-364`): load artifact → `ParseComposition` → `AddressSpacePlanner.Compute(_lastApplied, composition)` → **`plan.IsEmpty` short-circuit** (:328 — the known IsEmpty-gate: the Materialise passes do NOT run on an empty plan, so anything the plan is blind to never materialises) → `_applier.Apply(plan)` → the four Materialise passes over the full composition (:338-354). A pure-add plan is non-empty, so the passes always run for it.
|
||
- **Idempotency of the passes**: `EnsureFolder`/`EnsureVariable` early-return on an existing NodeId; `MaterialiseAlarmCondition` drops-and-recreates per id (idempotent but **not** subscription-preserving for that one condition node — see Design, native-alarm note). `MaterialiseHierarchy`/`MaterialiseEquipmentTags`/`MaterialiseEquipmentVirtualTags`/`MaterialiseScriptedAlarms` therefore add exactly the new nodes and no-op existing ones when no rebuild cleared the maps.
|
||
- **Folder-scoped NodeIds** (the second known gotcha): variables materialise at `EquipmentNodeIds.Variable(equipmentId, folderPath, name)` — NOT the driver FullName — so add-only deploys cannot collide with or re-key existing nodes; the driver value router (`_nodeIdByDriverRef`) is rebuilt per apply from the same composition and needs no change.
|
||
- **The guard**: `DeferredSinkForwardingReflectionTests.Every_forwarding_interface_method_reaches_the_inner_sink` iterates `ForwardingInterfaces = { IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink }` via `GetMethods()` — a member added to `ISurgicalAddressSpaceSink` in Phase 2 is picked up with **zero test edits** and fails the guard if `DeferredAddressSpaceSink` does not forward it.
|
||
|
||
## Root cause
|
||
|
||
The applier's rebuild predicate conflates three delta classes with very different minimal-mutation requirements:
|
||
|
||
1. **In-place changes** (attribute edits, renames) — already handled surgically (F10b).
|
||
2. **Pure additions** — need *no teardown at all*: the idempotent Materialise passes add exactly the new nodes; `RaiseNodesAddedModelChange` exists to announce them. The rebuild is pure collateral damage.
|
||
3. **Removals / node-affecting mutations** — genuinely need teardown, but only of the affected subtree, not the world.
|
||
|
||
The predicate collapses all three into one boolean. The fix is a classifier that names the classes and routes each to its minimal mutation, with full rebuild as the safety valve for anything else.
|
||
|
||
---
|
||
|
||
## Proposed design
|
||
|
||
### The delta classifier
|
||
|
||
A new pure static `AddressSpaceChangeClassifier` in `ZB.MOM.WW.OtOpcUa.OpcUaServer` (same project as the planner — it is a pure function over `AddressSpacePlan`, unit-testable without any sink), returning:
|
||
|
||
```csharp
|
||
/// <summary>How AddressSpaceApplier must mutate the address space for a given plan.</summary>
|
||
public enum AddressSpaceChangeKind
|
||
{
|
||
/// <summary>Plan.IsEmpty — nothing to do.</summary>
|
||
Empty,
|
||
/// <summary>Only surgical-eligible changed tags, node-irrelevant vtag changes, and/or folder
|
||
/// renames — the existing F10b in-place path. No adds, no removes.</summary>
|
||
AttributeOnly,
|
||
/// <summary>Only additions (any mix of Equipment/Alarms/Tags/VirtualTags/Drivers) on top of
|
||
/// at-most-AttributeOnly changes — no removals, no node-affecting changes. Phase 1.</summary>
|
||
PureAdd,
|
||
/// <summary>Only removals on top of at-most-AttributeOnly changes — no additions, no
|
||
/// node-affecting changes. Phase 2.</summary>
|
||
PureRemove,
|
||
/// <summary>Additions AND removals, still no node-affecting changes. Phase 3 applies
|
||
/// removes-then-adds surgically; until Phase 3 it maps to Rebuild.</summary>
|
||
AddRemoveMix,
|
||
/// <summary>Anything with a node-affecting change (ChangedEquipment, ChangedAlarms,
|
||
/// non-surgical ChangedEquipmentTags, node-relevant ChangedEquipmentVirtualTags) — the
|
||
/// unclassifiable-safe default. Full rebuild.</summary>
|
||
Rebuild,
|
||
}
|
||
|
||
public static class AddressSpaceChangeClassifier
|
||
{
|
||
public static AddressSpaceChangeKind Classify(AddressSpacePlan plan);
|
||
}
|
||
```
|
||
|
||
Classification algorithm (evaluated top-down; **first match wins**):
|
||
|
||
| Order | Condition | Kind |
|
||
|---|---|---|
|
||
| 1 | `plan.IsEmpty` | `Empty` |
|
||
| 2 | `ChangedEquipment.Count > 0` **or** `ChangedAlarms.Count > 0` **or** `ChangedEquipmentTags.Any(d => !TagDeltaIsSurgicalEligible(d))` **or** `ChangedEquipmentVirtualTags.Any(d => !VtagDeltaIsNodeIrrelevant(d))` | `Rebuild` |
|
||
| 3 | `hasAdds && hasRemoves` | `AddRemoveMix` |
|
||
| 4 | `hasAdds` | `PureAdd` |
|
||
| 5 | `hasRemoves` | `PureRemove` |
|
||
| 6 | otherwise (only surgical tag deltas / node-irrelevant vtag deltas / folder renames / driver deltas) | `AttributeOnly` |
|
||
|
||
where `hasAdds = AddedEquipment.Count + AddedAlarms.Count + AddedEquipmentTags.Count + AddedEquipmentVirtualTags.Count > 0` and `hasRemoves = RemovedEquipment.Count + RemovedAlarms.Count + RemovedEquipmentTags.Count + RemovedEquipmentVirtualTags.Count > 0`. **Driver deltas (`Added/Removed/ChangedDrivers`) are node-inert** — they never touch the address space today (excluded from the current predicate, routed to `DriverSpawnPlanner` in Runtime) and stay excluded from `hasAdds`/`hasRemoves`. Note a PureAdd/PureRemove/AddRemoveMix plan may *also* carry surgical tag deltas and folder renames — the applier applies those via the existing F10b path in the same pass (they are orthogonal in-place mutations).
|
||
|
||
The two eligibility predicates (`TagDeltaIsSurgicalEligible`, `VtagDeltaIsNodeIrrelevant`, currently private statics at `AddressSpaceApplier.cs:630-663`) **move to the classifier** (as `internal static`, applier keeps thin delegating calls or references them directly) so the whole classification is one pure, table-testable unit. Their whitelist-via-`with`-expression technique is preserved byte-for-byte (01 report calls it out as a positive).
|
||
|
||
### Phase staging
|
||
|
||
**Phase 1 — pure-adds (the ~90% win, no new sink members).** When `Classify == PureAdd`:
|
||
|
||
1. `Apply` does **not** call `SafeRebuild()`. It still applies any coincident folder renames + surgical tag deltas via the existing `ISurgicalAddressSpaceSink` path (any `false`/throw there → `SafeRebuild()` fallback, exactly as today).
|
||
2. The four Materialise passes in `OpcUaPublishActor.HandleRebuild` then run as they always do and create **exactly** the added nodes (folders/variables/conditions), idempotently no-op'ing everything that already exists. No applier change needed for the creation itself — this is the load-bearing simplification. The only pass with a wrinkle is `MaterialiseScriptedAlarms`/native-alarm materialisation: `MaterialiseAlarmCondition` is drop-and-recreate per id, so on a pure-add apply it would drop-and-recreate every **existing enabled** condition node (killing monitored items on those condition nodes only). Fix: make `MaterialiseAlarmCondition` **skip-if-present-and-same-kind** on the node-manager side — add an idempotent early-return when `_alarmConditions` already holds the id with the same `isNative` flag (drop-and-recreate stays for the kind-swap/re-severity case, which only reaches this path via a rebuild where the map was just cleared). This keeps existing alarm subscriptions alive across a pure-add deploy and is behavior-identical on the rebuild path.
|
||
3. **Announcement:** after the Materialise passes complete, `OpcUaPublishActor` calls a new `applier.AnnounceAddedNodes(plan)` (Phase-1 applier method, no new sink member — it uses the existing, guard-covered `RaiseNodesAddedModelChange`). It computes a **deduplicated set of affected parent ids**: for each added tag/vtag → its materialise parent (`EquipmentId` or `EquipmentNodeIds.SubFolder(EquipmentId, FolderPath)`); for each added scripted alarm → its `EquipmentId`; for each added equipment → its `UnsLineId` when present, else the equipment's own id (the root has no announceable id; announcing the new folder itself is valid Part 3 — verb `NodeAdded`, `Affected` = the added node). One `RaiseNodesAddedModelChange` per distinct id. Ordering is correct by construction: `Apply` returns before the passes run, and the announce runs after them, so the nodes exist when clients re-browse. The announce is gated on `!outcome.RebuildCalled` (after a full rebuild the announcement is moot — subscriptions are dead anyway, unchanged behavior).
|
||
4. **Existing-subscription behavior on add: none affected** — no existing `NodeState` is touched; per OPC UA Part 4 monitored items are unaffected by address-space growth; model-aware clients get the Part 3 `GeneralModelChangeEvent` and re-browse.
|
||
|
||
**Phase 2 — pure-removes (new sink capability).** When `Classify == PureRemove`:
|
||
|
||
New members on **`ISurgicalAddressSpaceSink`** (existing interface — the guard's `ForwardingInterfaces` set needs **no** change; the `DispatchProxy` recorder auto-covers new members):
|
||
|
||
```csharp
|
||
/// <summary>Remove ONE existing value-variable node in place (detach from parent, drop from the
|
||
/// predefined-node set, drop the historized-tagname registration), then raise a Part 3
|
||
/// GeneralModelChangeEvent (verb=NodeDeleted) outside the lock. The caller has already written a
|
||
/// final Bad quality to the node so in-flight MonitoredItems observe the removal. Returns false
|
||
/// when the node id is unknown (caller falls back to a full rebuild — map-drift safety valve).</summary>
|
||
bool RemoveVariableNode(string variableNodeId);
|
||
|
||
/// <summary>Remove ONE existing Part 9 alarm-condition node in place (detach, drop from the
|
||
/// predefined-node set, drop the native-alarm flag), then raise NodeDeleted outside the lock. The
|
||
/// caller has already written the terminal RemovedConditionState (Retain=false). Returns false
|
||
/// when the condition id is unknown (caller rebuilds).</summary>
|
||
bool RemoveAlarmConditionNode(string alarmNodeId);
|
||
|
||
/// <summary>Remove an equipment folder AND its entire descendant subtree (sub-folders, variables,
|
||
/// condition nodes) in place: per-descendant map/registration cleanup (variables, historized
|
||
/// tagnames, conditions, native flags), notifier demotion (RemoveRootNotifier + notifier-folder +
|
||
/// event-notifier-source registrations for the equipment id), then one NodeDeleted for the
|
||
/// equipment folder outside the lock. Returns false when the folder id is unknown (caller
|
||
/// rebuilds).</summary>
|
||
bool RemoveEquipmentSubtree(string equipmentNodeId);
|
||
```
|
||
|
||
Implemented on `OtOpcUaNodeManager` (all mutation under `Lock`; the `NodeDeleted` model-change event built under `Lock` and reported after release, mirroring `RaiseNodesAddedModelChange` / `ReportNodeShapeChangedEvent`), forwarded by `SdkAddressSpaceSink`, capability-sniff-forwarded by `DeferredAddressSpaceSink` (`_inner is ISurgicalAddressSpaceSink s && s.X(...)` — false pre-swap ⇒ rebuild fallback, same as `UpdateTagAttributes`), and no-op'd nowhere else (`NullOpcUaAddressSpaceSink` doesn't implement the surgical interface — unchanged).
|
||
|
||
Applier routing for `PureRemove`:
|
||
|
||
1. Partition removals: removed equipment "own" their child tags/vtags/alarms — a `RemovedEquipmentTags`/`RemovedEquipmentVirtualTags`/`RemovedAlarms` entry whose `EquipmentId` is in `RemovedEquipment` is **subsumed** by the subtree removal and skipped individually.
|
||
2. Pre-teardown terminal writes (extends the existing `:83-98` pattern): removed value variables get `WriteValue(nodeId, null, Bad, ts)`; removed condition nodes (scripted alarms AND alarm-bearing tags — the latter is a today-gap: `RemovedEquipmentTags` with `Alarm != null` currently get no condition write) get `SafeWriteAlarmCondition(nodeId, RemovedConditionState, ts)`. This is what an in-flight MonitoredItem observes (final Bad notification), which is the spec-friendly signal; after the node is gone, re-subscription attempts get `BadNodeIdUnknown` from the SDK because the node is no longer in the predefined set.
|
||
3. Teardown order children-first: individual variable/condition removals (for surviving equipment), then `RemoveEquipmentSubtree` per removed equipment. Empty leftover `FolderPath` sub-folders on surviving equipment are **deliberately kept** (browse-visible but harmless; the next full rebuild collects them — documented v1 cut, mirrors `EnsureFolder`'s create-only semantics).
|
||
4. Any `false`/throw from a remove ⇒ `SafeRebuild()` (one-way ratchet per apply: once rebuilt, skip remaining surgical work — the rebuild already reconciled everything).
|
||
5. **Subscription behavior on remove (per spec):** subscribers of *other* nodes are untouched; subscribers of the removed node see a final Bad value (step 2), the node disappears from browse, and model-aware clients get `NodeDeleted`. This matches OPC UA Part 3/4 expectations for node deletion.
|
||
|
||
**Phase 3 — mixed add+remove batches.** When `Classify == AddRemoveMix`: apply the Phase-2 remove pass, then let the Materialise passes + Phase-1 announce handle the adds — removes-then-adds within one apply. The one ordering hazard is an id **reused** across the remove and add sets in the same deploy (e.g. tag removed from equipment E folder F name N while another tag materialises at the identical folder-scoped NodeId): removes run in `Apply` (before the passes), adds run in the passes (after), so remove-then-recreate is the natural order and the recreated node is a fresh `NodeState` (correct — it is a different tag). No special-casing needed; covered by a targeted test. Until Phase 3 ships, `AddRemoveMix` maps to `Rebuild` (today's behavior), which is why each phase is independently shippable.
|
||
|
||
### NodeId / namespace-index stability constraints
|
||
|
||
- All materialised nodes live in the single server namespace (`NamespaceIndex` on `OtOpcUaNodeManager`); the index is assigned at server start and is untouched by deploys. No constraint is added or relaxed by this work.
|
||
- Folder/variable/condition NodeIds are deterministic pure functions of composition (`UnsAreaId`/`UnsLineId`/`EquipmentId`, `EquipmentNodeIds.SubFolder/Variable`, `ScriptedAlarmId`) — an add cannot re-key an existing node, and the surgical paths reuse the **same** derivations the Materialise passes use (the applier already does this for `UpdateTagAttributes` at `:177`). Any new derivation in the remove path MUST go through `EquipmentNodeIds` — never hand-rolled strings.
|
||
- Same-NodeId collision on add (two tags materialising to one folder-scoped id) keeps today's first-wins `EnsureVariable` semantics — no regression, explicitly out of scope.
|
||
|
||
### Interaction with the historian/provisioning/alarm seams (all verified no-change or additive)
|
||
|
||
- **Historized-ref feed (`FeedHistorizedRefs`)** and **historian provisioning (`ProvisionHistorizedTags`)** already consume the plan's add/remove/change deltas and fire unconditionally at the end of `Apply` — routing a plan surgically instead of via rebuild changes nothing for them. (They are the design template for this work: per-deploy delta consumption, non-blocking, deploy-safe.)
|
||
- **Native alarm condition nodes:** added alarm-bearing tags materialise via `SafeMaterialiseAlarmCondition` in `MaterialiseEquipmentTags` — additive on the pure-add path once the skip-if-present guard (Phase 1 step 2) lands. Removal of an alarm-bearing tag routes to `RemoveAlarmConditionNode` (its node is a condition, not a value variable). `EnsureFolderIsEventNotifier` promotion for a first-alarm-on-existing-equipment add is already runtime-safe (the rebuild path performs it on a live server today).
|
||
- **Driver subscriptions / value routing:** rebuilt every apply by `DriverHostActor.PushDesiredSubscriptions` independent of this path — adds get subscribed, removes get unsubscribed and un-routed, no change needed.
|
||
- **`AddressSpaceApplyOutcome`:** gains nothing structurally in Phase 1 (the existing `RebuildCalled=false` + `AddedNodes` counts express the pure-add outcome); the apply log line gains the classifier verdict (`kind={Kind}`) so live verification can grep for `kind=PureAdd, rebuild=False`.
|
||
|
||
### Fallback-to-rebuild safety valve
|
||
|
||
Single principle, enforced by the classifier's shape: **anything not positively classified as safe falls into `Rebuild`** (order rule 2 catches every node-affecting change, including any future plan field that makes a changed record unequal — the `with`-expression whitelists are default-closed). At apply time, every surgical sink call keeps the F10b contract: `false` or throw ⇒ `SafeRebuild()` and mark `rebuilt`. Net: the worst possible outcome of any bug in this work is today's behavior (a full rebuild).
|
||
|
||
### Alternatives considered (and rejected)
|
||
|
||
- **Re-home monitored items onto recreated nodes after a rebuild** — the SDK offers no supported API to re-bind a `MonitoredItem` to a new `NodeState`; rejected (round-1 plan concurs).
|
||
- **Generic diff-and-patch of every delta class** (make `ChangedEquipment`/`ChangedAlarms` surgical too) — larger and riskier; changed-alarm re-materialisation and equipment renames can ride a later increment (equipment DisplayName rename is `UpdateFolderDisplayName`-shaped — noted as follow-up, not in scope). The add/remove split follows the plan's existing typed delta sets.
|
||
- **Classify in `AddressSpacePlanner` (attach the kind to the plan record)** — rejected: `Compute` is a pure diff and the kind is a *policy* over the diff that the applier owns (and the F10b eligibility predicates it needs already live applier-side). A separate pure classifier keeps the planner untouched and the policy table-testable.
|
||
- **Batch remove member (`RemoveNodes(IReadOnlyList<…>)`) instead of three typed members** — rejected: the per-node `bool` false-means-rebuild contract (the established F10b posture) gets murky on a batch; three members mirror the existing surgical-member shape and the guard covers each independently.
|
||
- **Return `true` (idempotent success) from a remove of an unknown node** — rejected: an unknown id on remove means the node-manager maps drifted from what the planner believes; the house posture (F10b `UpdateTagAttributes:false`) is resync-via-rebuild. Documented in the member xmldoc.
|
||
- **Announce adds from inside `Apply`** — impossible: `Apply` runs before the Materialise passes create the nodes; the announce must be a post-passes step in `OpcUaPublishActor` (verified ordering `OpcUaPublishActor.cs:335-354`).
|
||
|
||
---
|
||
|
||
## Implementation steps (exact paths)
|
||
|
||
### Phase 1 — pure-adds
|
||
|
||
1. **`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceChangeClassifier.cs`** (new): `AddressSpaceChangeKind` enum + `Classify(AddressSpacePlan)` per the table; move `TagDeltaIsSurgicalEligible` + `VtagDeltaIsNodeIrrelevant` here as `internal static` (from `AddressSpaceApplier.cs:630-663`).
|
||
2. **`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs`**: replace the `structuralRebuild` boolean (`:134-140`) with `var kind = AddressSpaceChangeClassifier.Classify(plan)`; `Rebuild`/`AddRemoveMix`/`PureRemove` (until their phases ship) ⇒ `SafeRebuild()`; `PureAdd`/`AttributeOnly` ⇒ no rebuild + existing surgical renames/tag-deltas (with the existing `false`⇒rebuild fallback). Add `kind` to the apply log line. Add `public IReadOnlyList<string> ComputeAddAnnouncements(AddressSpacePlan plan)` (pure, dedup parent-id set per the design) and `public void AnnounceAddedNodes(AddressSpacePlan plan)` calling `_sink.RaiseNodesAddedModelChange` per id (Safe-wrapped). While here, refresh the stale F14-era class doc (`:7-26`, 01/C-7).
|
||
3. **`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs`**: `MaterialiseAlarmCondition` (`:586-604`) — idempotent early-return when `_alarmConditions` holds the id AND the stored native flag equals `isNative` (keeps drop-and-recreate only for kind-swaps; requires remembering the flag — `_nativeAlarmNodeIds` already encodes it).
|
||
4. **`src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs`**: in `HandleRebuild` after the fourth Materialise pass (`:354`), `if (!outcome.RebuildCalled) _applier.AnnounceAddedNodes(plan);`.
|
||
5. Live-`/run` gate (see Tests) + docs: `docs/v2/`-appropriate note + `archreview/plans/STATUS.md` row.
|
||
|
||
### Phase 2 — pure-removes
|
||
|
||
6. **`src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs`**: add the three members (signatures above). **Guard-first discipline:** the reflection guard auto-covers them the moment they exist — implement `DeferredAddressSpaceSink` forwarding in the same commit or the guard fails the build's test leg (that failure IS the negative control; observe it once deliberately).
|
||
7. **`src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`**: capability-sniffing forwards for all three (pattern of `:58-70`).
|
||
8. **`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs`**: implement `RemoveVariableNode` / `RemoveAlarmConditionNode` / `RemoveEquipmentSubtree` (teardown under `Lock` mirroring the per-node loops inside `RebuildAddressSpace` `:1694-1734`, scoped to the id/subtree; clean `_variables`/`_historizedTagnames`/`_alarmConditions`/`_nativeAlarmNodeIds`/`_folders`/`_notifierFolders`(+`RemoveRootNotifier`)/`_eventNotifierSources`); `internal GeneralModelChangeEventState BuildNodesRemovedModelChange(string affectedNodeId)` mirroring `BuildNodesAddedModelChange` (`:1627-1654`) with `Verb = NodeDeleted`; report outside `Lock`.
|
||
9. **`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs`**: three one-line forwards.
|
||
10. **`AddressSpaceApplier.cs`**: `PureRemove` arm — terminal writes (Bad value / `RemovedConditionState`, extending `:83-98` incl. the alarm-bearing-removed-tag gap), subsumption partition, children-first removal calls, `false`/throw ⇒ `SafeRebuild()` ratchet.
|
||
11. Live-`/run` remove gate + docs.
|
||
|
||
### Phase 3 — mixed batches
|
||
|
||
12. **`AddressSpaceChangeClassifier.cs` + `AddressSpaceApplier.cs`**: `AddRemoveMix` routes to the Phase-2 remove pass then falls through to the pure-add (no-rebuild + announce) behavior; remove-then-recreate same-NodeId test.
|
||
13. Final docs + STATUS.md close-out.
|
||
|
||
---
|
||
|
||
## Tests
|
||
|
||
**Ordering rule: guard-set/TDD first in every task** (the F10b precedent — 258 unit tests + 3 reviews missed prod-inertness; the guard + live gate are the two nets that catch it).
|
||
|
||
- **Guard extension (Phase 2, FIRST):** no test-code change needed — `DeferredSinkForwardingReflectionTests` auto-covers new `ISurgicalAddressSpaceSink` members. The TDD step is a **deliberate negative control**: add the interface members with a non-forwarding `DeferredAddressSpaceSink` stub (`=> false`), run `dotnet test --filter "FullyQualifiedName~DeferredSinkForwardingReflectionTests"` and observe the member-named FAIL, then implement the forward and observe PASS. Also extend the hand-written `DeferredAddressSpaceSinkTests` (Commons + OpcUaServer.Tests variants) with arg-fidelity cases for the three members.
|
||
- **Classifier (Phase 1):** new `AddressSpaceChangeClassifierTests` (OpcUaServer.Tests) — table-driven (`[Theory]`/`[MemberData]`) over the full matrix: empty; each single add class ⇒ PureAdd; each single remove class ⇒ PureRemove; add+remove ⇒ AddRemoveMix; each changed class ⇒ Rebuild; surgical-eligible-changed-only ⇒ AttributeOnly; renames-only ⇒ AttributeOnly; adds+surgical-changes ⇒ PureAdd; adds+non-surgical-change ⇒ Rebuild; driver-only deltas ⇒ AttributeOnly (node-inert).
|
||
- **Applier (Phase 1):** extend `AddressSpaceApplierTests` (the `RecordingSink : IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink` at `:1761` already records `RebuildCalls` + `ModelChangeQueue`): pure-add plan ⇒ `RebuildCalls == 0`, `outcome.RebuildCalled == false`; `AnnounceAddedNodes` ⇒ one `RaiseNodesAddedModelChange` per distinct parent (dedup + subfolder-parent + added-equipment cases); pure-add **with** coincident surgical tag delta ⇒ no rebuild + `UpdateTagAttributes` called; surgical-`false` during a pure-add apply ⇒ rebuild fallback; remove-only and add+remove plans still rebuild (phase-boundary regression pins, updated in Phases 2/3).
|
||
- **Node manager (Phase 1):** extend `NodeManagerSurgicalShapeUpdateTests`-style suite (or new `NodeManagerAlarmIdempotentMaterialiseTests`): re-`MaterialiseAlarmCondition` same id+kind keeps the same `AlarmConditionState` instance (`FindAlarmCondition` reference-equal); kind-swap still recreates.
|
||
- **Node manager (Phase 2):** new `NodeManagerSurgicalRemoveTests`: ensure→remove variable ⇒ `FindVariable` null, `VariableCount` decremented, `TryGetHistorianTagname` false, unknown id ⇒ false; alarm-condition remove ⇒ `FindAlarmCondition` null + native flag cleared; equipment-subtree remove ⇒ descendants gone, notifier demoted (no orphaned root-notifier ref), other equipment untouched; `BuildNodesRemovedModelChange` populates `Verb=NodeDeleted` + `Affected` (mirrors `NodeManagerModelChangeOnAddTests`).
|
||
- **Publish actor (Phase 1):** extend `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs`: pure-add artifact apply ⇒ sink records NO `RebuildAddressSpace` call, records `RaiseNodesAddedModelChange` AFTER `EnsureVariable` for the added tag (ordering assertion via a sequenced recording sink).
|
||
- **Subscription-survival integration (Phase 1, the headline):** new `SubscriptionSurvivalTests` in `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/` (reuse `DualEndpointTests`' in-process server + `DefaultSessionFactory` session bootstrap): materialise tag A, create a real subscription + monitored item on A, drive a pure-add apply (applier + Materialise passes) adding tag B, then (a) A's monitored item still delivers on a subsequent `WriteValue` (no `BadNodeIdUnknown`/dead item), (b) B is browsable + readable. Phase 2 sibling: subscribe A + B, surgical-remove B ⇒ A alive, B's item observes the final Bad then the node is gone from browse; Phase 3 sibling for the mixed case. This also chips at 03/U6's "OpcUaServer.IntegrationTests has one test".
|
||
- **Live-`/run` gate (MANDATORY per phase — the F10b lesson: an optimization path that isn't live-driven can ship inert despite full unit green):** on docker-dev, **rebuild BOTH central-1 and central-2** (`:9200` round-robins them); `dotnet run --project src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI -- subscribe -u opc.tcp://localhost:4840 -n "ns=2;s=<existing tag>" -i 500` and keep it running; `POST http://localhost:9200/api/deployments` (X-Api-Key) a config adding **one** tag to a *different* equipment; assert (1) the running subscribe keeps delivering (no dead/Bad transition on the existing node), (2) the central log shows `kind=PureAdd, rebuild=False`, (3) `browse`/`read` finds the new node, (4) the new tag goes Good once the driver publishes. Phase 2: same recipe with a one-tag remove (subscription on a *different* tag survives; the removed node reads `BadNodeIdUnknown` after). Phase 3: one deploy carrying +1/−1.
|
||
|
||
## Effort + Risk (per phase)
|
||
|
||
| Phase | Effort | Risk / blast radius | Mitigations |
|
||
|---|---|---|---|
|
||
| 1 — pure-adds | **M** (~1.5-2 days incl. integration + live gate) | **High-leverage, moderate risk**: touches the apply routing core, but adds **no** sink members and no node-manager teardown; the worst regression is "should have rebuilt but didn't" (stale space) — bounded by the default-closed classifier + F10b fallback | table-driven classifier tests; recording-sink applier tests; subscription-survival integration test; mandatory live-`/run`; classifier verdict in the apply log |
|
||
| 2 — pure-removes | **M-L** (~2-3 days) | **Highest risk slice**: new node-manager teardown under `Lock` (leaked notifier refs / map drift / lock-order inversion on the event report) + the deferred-forwarding trap | guard auto-coverage + deliberate negative control; teardown mirrors the proven `RebuildAddressSpace` loops; report-outside-Lock mirrors three existing patterns; false⇒rebuild ratchet; live-`/run` remove gate |
|
||
| 3 — mixed | **S-M** (~0.5-1 day) | Low: composition of two shipped paths; the only new surface is remove-then-recreate ordering | targeted same-NodeId reuse test; live-`/run` mixed gate |
|
||
|
||
Overall: **Large** (matches OVERALL #7), ~4-6 days. Each phase lands independently; between phases, unhandled kinds keep today's rebuild behavior.
|
||
|
||
---
|
||
|
||
## Task breakdown
|
||
|
||
Tasks are bite-sized TDD increments. **Classification:** `high-risk` = touches the sink seam / node-manager mutation under `Lock` / apply routing (review + live-verify weight); `low-risk` = pure functions, tests, docs. Every task: run its FAIL→PASS filter, then the touched project's full suite before committing. Phase boundaries are hard sequence points (marked). Baseline branch suggestion: `feat/r2-07-surgical-pure-adds` off `master`.
|
||
|
||
### ── PHASE 1 — pure-adds ──
|
||
|
||
**T1 — Classifier skeleton + table-driven tests (RED→GREEN)**
|
||
- **Classification:** low-risk (pure function)
|
||
- **Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
- **Parallelizable with:** T5 (different files); everything else in Phase 1 depends on it
|
||
- **Files:** `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceChangeClassifier.cs` (new), `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceChangeClassifierTests.cs` (new)
|
||
- TDD: write the full `[Theory]` matrix from the Tests section (reuse `AddressSpacePlannerTests`' plan-construction helpers); `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~AddressSpaceChangeClassifierTests"` → FAIL (type missing) → implement enum + `Classify` (first-match-wins table) with the two eligibility predicates **moved** from the applier (`AddressSpaceApplier.cs:630-663`) as `internal static`; applier temporarily references them at their new home (compile-only change to the applier) → PASS. Full project suite green.
|
||
- Commit: `feat(opcua): add AddressSpaceChangeClassifier — typed delta classification for the applier (R2-07 T1)`
|
||
|
||
**T2 — Route `Apply` through the classifier (behavior-preserving except PureAdd)**
|
||
- **Classification:** high-risk (apply routing core)
|
||
- **Estimated implement time:** 5 min (tests) + 10 min (impl)
|
||
- **Parallelizable with:** — (blocks on T1)
|
||
- **Files:** `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs`
|
||
- TDD: add applier tests — pure-add-only plan ⇒ `RebuildCalls == 0` + `RebuildCalled == false`; pure-add + surgical tag delta ⇒ no rebuild + `UpdateTagAttributes` recorded; pure-add + surgical-returns-false ⇒ rebuild fallback; remove-only / add+remove / changed-non-surgical plans ⇒ still rebuild (phase pins); `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~AddressSpaceApplierTests"` → new tests FAIL → replace `structuralRebuild` with the classifier switch (Rebuild/AddRemoveMix/PureRemove ⇒ `SafeRebuild()`; PureAdd/AttributeOnly ⇒ surgical path) + `kind={Kind}` in the apply log → PASS, **every pre-existing applier test untouched-green** (the behavior-preservation proof).
|
||
- Commit: `feat(opcua): route AddressSpaceApplier through the classifier — pure-add deploys no longer rebuild (R2-07 T2)`
|
||
|
||
**T3 — `ComputeAddAnnouncements` + `AnnounceAddedNodes` on the applier**
|
||
- **Classification:** low-risk (uses the existing guard-covered `RaiseNodesAddedModelChange` sink member)
|
||
- **Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
- **Parallelizable with:** T4a (blocks T4b)
|
||
- **Files:** `AddressSpaceApplier.cs`, `AddressSpaceApplierTests.cs`
|
||
- TDD: tests — two tags added under the same equipment ⇒ ONE announce for the equipment id; tag with FolderPath ⇒ announce `EquipmentNodeIds.SubFolder(...)`; added scripted alarm ⇒ equipment id; added equipment with `UnsLineId` ⇒ line id, without ⇒ its own id; empty-add plan ⇒ zero announces; filter as T2 → FAIL → implement (pure compute + Safe-wrapped sink loop) → PASS.
|
||
- Commit: `feat(opcua): announce pure-add nodes via Part 3 NodeAdded model-change (R2-07 T3)`
|
||
|
||
**T4a — Idempotent `MaterialiseAlarmCondition` (skip-if-present-and-same-kind)**
|
||
- **Classification:** high-risk (node-manager mutation under `Lock`)
|
||
- **Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
- **Parallelizable with:** T3
|
||
- **Files:** `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmIdempotentMaterialiseTests.cs` (new; crib the server-bootstrap fixture from `NodeManagerModelChangeOnAddTests`)
|
||
- TDD: tests — re-materialise same id+kind ⇒ `FindAlarmCondition` returns the **same instance** (subscription-preserving); kind-swap (isNative flip) ⇒ recreated; post-rebuild re-materialise ⇒ fresh (map was cleared); `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~NodeManagerAlarmIdempotentMaterialise"` FAIL → early-return guard → PASS. Run the full `NodeManager*` + `AlarmCommandRouter` suites (condition-handler regressions).
|
||
- Commit: `feat(opcua): make MaterialiseAlarmCondition idempotent for same-kind re-applies (R2-07 T4a)`
|
||
|
||
**T4b — Publish-actor post-materialise announce hook**
|
||
- **Classification:** high-risk (deploy pipeline)
|
||
- **Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
- **Parallelizable with:** — (blocks on T2+T3+T4a)
|
||
- **Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs`
|
||
- TDD: extend the rebuild tests with a pure-add artifact case — recording sink shows NO `RebuildAddressSpace`, and `RaiseNodesAddedModelChange` recorded AFTER the added tag's `EnsureVariable` (sequence list); a rebuild-kind artifact records NO announce; `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~OpcUaPublishActorRebuildTests"` FAIL → add `if (!outcome.RebuildCalled) _applier.AnnounceAddedNodes(plan);` after the fourth pass → PASS.
|
||
- Commit: `feat(runtime): announce added nodes after the materialise passes on non-rebuild applies (R2-07 T4b)`
|
||
|
||
**T5 — Subscription-survival integration test (over-the-wire)**
|
||
- **Classification:** high-risk to write, low-risk to land (test-only)
|
||
- **Estimated implement time:** 15 min
|
||
- **Parallelizable with:** T1-T4 (can be authored against the design; goes green only after T4b)
|
||
- **Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/SubscriptionSurvivalTests.cs` (new; reuse `DualEndpointTests`' server + session bootstrap)
|
||
- TDD: subscribe tag A (real monitored item, 250 ms), pure-add apply adds tag B, `WriteValue(A)` ⇒ notification arrives (item alive), browse finds B. Written FIRST it FAILS on master behavior (rebuild kills the item); goes PASS once T2/T4b land. `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests --filter "FullyQualifiedName~SubscriptionSurvivalTests"`.
|
||
- Commit: `test(opcua): over-the-wire subscription-survival on pure-add deploy (R2-07 T5)`
|
||
|
||
**T6 — Phase 1 live-`/run` gate (docker-dev) — MANDATORY before merging Phase 1**
|
||
- **Classification:** high-risk verification (the F10b inertness net)
|
||
- **Estimated implement time:** 20 min
|
||
- **Parallelizable with:** —
|
||
- **Files:** none (evidence into the PR/STATUS notes)
|
||
- Steps: rebuild **both** central-1 + central-2 images; Client.CLI `subscribe` on an existing tag (keep running); `POST :9200/api/deployments` adding ONE tag to a different equipment; assert: subscribe stream uninterrupted; central log `kind=PureAdd, rebuild=False`; `browse`/`read` shows the new node; new tag turns Good on first driver publish. Capture the log lines.
|
||
- Commit: `docs(archreview): record R2-07 Phase 1 live-verify evidence in STATUS.md`
|
||
- **── PHASE 1 SHIPPABLE BOUNDARY — merge/PR here ──**
|
||
|
||
### ── PHASE 2 — pure-removes ──
|
||
|
||
**T7 — Sink members + deferred forwarding, guard-first (negative control observed)**
|
||
- **Classification:** high-risk (THE deferred-sink trap; guard-set coverage FIRST)
|
||
- **Estimated implement time:** 5 min (interface+stub, observe guard FAIL) + 5 min (forward, PASS)
|
||
- **Parallelizable with:** — (Phase 2 root)
|
||
- **Files:** `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs`, `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`, `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredAddressSpaceSinkTests.cs`
|
||
- TDD (the guard IS the test): add the three members with `DeferredAddressSpaceSink` stubs `=> false` (non-forwarding); `dotnet test tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests --filter "FullyQualifiedName~DeferredSinkForwardingReflectionTests"` → **FAIL with the member-named diagnostic (negative control — record it)** → implement capability-sniffing forwards (pattern `DeferredAddressSpaceSink.cs:58-70`) → PASS. Add arg-fidelity cases to the hand-written deferred tests. NOTE: `SdkAddressSpaceSink` + `OtOpcUaNodeManager` must compile — add node-manager methods returning `false` (rebuild-fallback semantics ⇒ tree stays shippable) with xmldoc marking T8-T10 as the real implementation.
|
||
- Commit: `feat(commons): add surgical remove members to ISurgicalAddressSpaceSink + deferred forwarding (guard-verified) (R2-07 T7)`
|
||
|
||
**T8 — `RemoveVariableNode` on the node manager**
|
||
- **Classification:** high-risk (teardown under `Lock`)
|
||
- **Estimated implement time:** 5 min (tests) + 10 min (impl)
|
||
- **Parallelizable with:** T9 (disjoint methods, same file — coordinate or sequence)
|
||
- **Files:** `OtOpcUaNodeManager.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs` (new)
|
||
- TDD: ensure→remove ⇒ `FindVariable` null + `VariableCount` down + `TryGetHistorianTagname` false; unknown id ⇒ false; `BuildNodesRemovedModelChange` carries `Verb=NodeDeleted` + `Affected` (internal seam, mirrors `NodeManagerModelChangeOnAddTests`); `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~NodeManagerSurgicalRemoveTests"` FAIL → implement (detach mirrors `RebuildAddressSpace:1694-1701` scoped to one id; build event under `Lock`, report outside) → PASS.
|
||
- Commit: `feat(opcua): surgical RemoveVariableNode with NodeDeleted model-change (R2-07 T8)`
|
||
|
||
**T9 — `RemoveAlarmConditionNode`**
|
||
- **Classification:** high-risk
|
||
- **Estimated implement time:** 5 + 5 min
|
||
- **Parallelizable with:** T8 (coordinate on the shared test file)
|
||
- **Files:** `OtOpcUaNodeManager.cs`, `NodeManagerSurgicalRemoveTests.cs`
|
||
- TDD: materialise (native + scripted)→remove ⇒ `FindAlarmCondition` null + native flag cleared + `NodeDeleted` built; unknown ⇒ false; filter as T8 FAIL→PASS (teardown mirrors `:1703-1711` scoped).
|
||
- Commit: `feat(opcua): surgical RemoveAlarmConditionNode (R2-07 T9)`
|
||
|
||
**T10 — `RemoveEquipmentSubtree` (descendants + notifier demotion)**
|
||
- **Classification:** high-risk (the notifier/root-ref leak hazard)
|
||
- **Estimated implement time:** 10 min (tests) + 15 min (impl)
|
||
- **Parallelizable with:** — (blocks on T8+T9 for the per-node primitives it reuses)
|
||
- **Files:** `OtOpcUaNodeManager.cs`, `SdkAddressSpaceSink.cs` (the three forwards), `NodeManagerSurgicalRemoveTests.cs`
|
||
- TDD: equipment with subfolder+variables+condition → remove ⇒ all descendants gone from every map, `RemoveRootNotifier` invoked (no orphaned Server↔folder ref — assert via `_notifierFolders`-driven observable or the notifier accessor), `_eventNotifierSources` entry dropped, sibling equipment fully intact; unknown ⇒ false; filter FAIL→PASS. Wire the three `SdkAddressSpaceSink` forwards here (replacing T7's false-stubs at the manager) and re-run the Commons guard.
|
||
- Commit: `feat(opcua): surgical RemoveEquipmentSubtree with notifier demotion (R2-07 T10)`
|
||
|
||
**T11 — Applier PureRemove arm (terminal writes + subsumption + fallback ratchet)**
|
||
- **Classification:** high-risk (apply routing)
|
||
- **Estimated implement time:** 10 min (tests) + 10 min (impl)
|
||
- **Parallelizable with:** —
|
||
- **Files:** `AddressSpaceApplier.cs`, `AddressSpaceApplierTests.cs`
|
||
- TDD: remove-one-tag plan ⇒ Bad `WriteValue` then `RemoveVariableNode`, NO rebuild; removed alarm-bearing tag ⇒ `RemovedConditionState` write + `RemoveAlarmConditionNode` (closes the today-gap); removed equipment ⇒ `RemoveEquipmentSubtree` and NO individual child removals (subsumption); any remove returns false ⇒ `SafeRebuild()` + no further surgical calls (ratchet); flip the T2 phase-pin ("remove-only ⇒ rebuild") to the new expectation; filter `FullyQualifiedName~AddressSpaceApplierTests` FAIL→PASS.
|
||
- Commit: `feat(opcua): surgical pure-remove deploys — scoped teardown, no full rebuild (R2-07 T11)`
|
||
|
||
**T12 — Phase 2 integration + live gate**
|
||
- **Classification:** high-risk verification
|
||
- **Estimated implement time:** 15 min (test) + 20 min (live)
|
||
- **Parallelizable with:** —
|
||
- **Files:** `SubscriptionSurvivalTests.cs`
|
||
- TDD: subscribe A+B → surgical-remove B ⇒ A's item alive; B's item observes final Bad; B unbrowsable (`BadNodeIdUnknown` on read). Then the live-`/run` remove recipe (both centrals; subscribe survivor; deploy −1 tag; log shows `kind=PureRemove, rebuild=False`). Record evidence in STATUS.md.
|
||
- Commit: `test(opcua): subscription-survival on pure-remove + live-verify evidence (R2-07 T12)`
|
||
- **── PHASE 2 SHIPPABLE BOUNDARY ──**
|
||
|
||
### ── PHASE 3 — mixed batches ──
|
||
|
||
**T13 — Classifier + applier `AddRemoveMix` routing (removes-then-adds)**
|
||
- **Classification:** high-risk (apply routing)
|
||
- **Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
- **Parallelizable with:** —
|
||
- **Files:** `AddressSpaceApplier.cs`, `AddressSpaceApplierTests.cs`, `AddressSpaceChangeClassifierTests.cs`
|
||
- TDD: add+remove plan ⇒ removes applied surgically, NO rebuild, `RebuildCalled=false` (flip the T2 phase pin); remove-then-recreate same folder-scoped NodeId in one plan ⇒ remove recorded before the passes would re-ensure (sequence assertion); any remove failure ⇒ rebuild ratchet still holds; filters as before FAIL→PASS.
|
||
- Commit: `feat(opcua): surgical mixed add+remove deploys (R2-07 T13)`
|
||
|
||
**T14 — Phase 3 integration + live gate + docs close-out**
|
||
- **Classification:** high-risk verification + low-risk docs
|
||
- **Estimated implement time:** 15 min (test) + 20 min (live) + 10 min (docs)
|
||
- **Parallelizable with:** —
|
||
- **Files:** `SubscriptionSurvivalTests.cs`, `archreview/plans/STATUS.md`, `CLAUDE.md`/`docs` note if the deploy-behavior description warrants it, `archreview/03-server-runtime.md` P1 row (mark remediated)
|
||
- TDD: mixed-deploy integration case (subscribe A; deploy −B/+C; A alive, B gone, C browsable). Live: one deploy carrying +1/−1 on docker-dev (both centrals), log `kind=AddRemoveMix, rebuild=False`, survivor subscription uninterrupted. Update STATUS.md + the P1 finding row.
|
||
- Commit: `test+docs: mixed-deploy survival, live evidence, STATUS/P1 close-out (R2-07 T14)`
|
||
- **── PHASE 3 / PLAN COMPLETE ──**
|
||
|
||
---
|
||
|
||
## Execution deviations (R2-07)
|
||
|
||
- **Applier phase-pin tests were flipped, not "untouched-green".** The plan framed T2/T11/T13 as
|
||
behavior-preserving ("every pre-existing applier test untouched-green"). In reality several pre-existing
|
||
tests directly encoded the OLD full-rebuild-on-any-topology-change behavior — `Added_equipment_*`,
|
||
`Removed_equipment_*`, `Surgical_eligible_tag_delta_mixed_with_added_equipment_rebuilds`,
|
||
`Folder_rename_mixed_with_added_equipment_rebuilds`, `Removed_equipment_tags_and_virtual_tags_only_rebuild_*`
|
||
— and were flipped to assert the new PureAdd/PureRemove/AddRemoveMix no-rebuild behavior (each doc-commented
|
||
"Supersedes the pre-R2-07 … pin"). The genuinely-behavior-preserving tests (surgical attribute path, folder
|
||
renames, changed-non-surgical ⇒ rebuild) stayed green untouched.
|
||
- **Incidental `RebuildCalled` assertions repointed.** Tests whose *subject* was not the rebuild decision but
|
||
which happened to assert `RebuildCalled==true` on a now-PureAdd plan were adjusted:
|
||
`AddressSpaceApplierFailureSurfaceTests` (rebuild-failed surfacing) + `OpcUaPublishActorApplyFailureTests`
|
||
(apply.failed meter) were repointed to a **rebuild-forcing ChangedEquipment plan** so they still exercise
|
||
the SafeRebuild path; `AddressSpaceApplierProvisioningTests` `RebuildCalled` assertions became
|
||
`RebuildCalled==false` + `AddedNodes>0` (provisioning still fires on a pure-add).
|
||
- **T1 predicate references:** the two eligibility predicates moved to `AddressSpaceChangeClassifier` as
|
||
`internal static`; the applier references them directly (`AddressSpaceChangeClassifier.TagDeltaIsSurgicalEligible`)
|
||
rather than keeping thin delegating wrappers.
|
||
- **Integration + live gates deferred-live (memory constraint).** Per the executor's memory budget
|
||
(`*.IntegrationTests` leak ~16 GB/run, other plans running concurrently), the over-the-wire
|
||
`SubscriptionSurvivalTests` (add/remove/mixed) were **authored + `dotnet build`-compile-verified but NOT
|
||
run**, and T6/T12/T14 docker-dev live-`/run` gates were marked `deferred-live`. These are the F10b
|
||
inertness nets and MUST be run in the serial heavy pass before merge/trust.
|
||
- **Worktree entry:** `EnterWorktree` refused to switch from the repo root; all work was done against the
|
||
worktree via absolute paths + `git -C`, never touching the main checkout.
|