Merge R2-07 Surgical pure-adds (arch-review round 2) [PR #436]
Finding 03/P1: surgical in-place address-space adds/removals skip full rebuilds (AddressSpaceChangeClassifier default-closed to Rebuild). 3 new ISurgicalAddressSpaceSink remove members, guard-first + forwarded through DeferredAddressSpaceSink (F10b inertness net green). T5/T6/T12/T14 over-the-wire + docker-dev gates deferred. Auto-merged OtOpcUaNodeManager.cs with R2-08; verified OpcUaServer.Tests 341/341 + forwarding guard 3/3. Build clean.
This commit is contained in:
Joseph Doherty
@@ -25,7 +25,7 @@ Every finding from the 2026-07-08 review, verified against the actual code (not
|
||||
| S9 | No server-cert renewal/expiry monitoring | **STILL OPEN.** `OpcUaApplicationHost.cs:305-317` unchanged. |
|
||||
| S10 | SDK start failure swallowed; node runs with Null sinks, invisible to /health | **STILL OPEN.** `OtOpcUaServerHostedService.cs:110-129` unchanged. |
|
||||
| S11 | `AuditWriterActor` unbounded buffer, drops batches on outage | **STILL OPEN** (and still moot — see U3). |
|
||||
| P1 | Any structural deploy change → full rebuild severs all monitored items | **STILL OPEN.** `AddressSpaceApplier.cs:134-154` / `OtOpcUaNodeManager.cs:1690-1736` unchanged. The U2 reflection guard (now merged) was the declared prerequisite gate for the surgical pure-adds work; the work itself has not started. |
|
||||
| P1 | Any structural deploy change → full rebuild severs all monitored items | **REMEDIATED (offline; live gates deferred)** on `r2/07-surgical-pure-adds` (R2-07 Phases 1–3, `bb226f45`…`e2b47638`). New pure `AddressSpaceChangeClassifier` routes each deploy delta to its minimal mutation in `AddressSpaceApplier.Apply`: **PureAdd/AttributeOnly** skip `RebuildAddressSpace` (the idempotent Materialise passes add exactly the new nodes; `AnnounceAddedNodes` fires a Part 3 NodeAdded per parent), **PureRemove/AddRemoveMix** tear down only the affected subtree in place via 3 new `ISurgicalAddressSpaceSink` remove members (guard-first, negative-control observed) + `OtOpcUaNodeManager` scoped teardown with `NodeDeleted` model-change, and **Rebuild** stays the default-closed safety valve (any node-affecting change, plus every surgical false/throw ⇒ full-rebuild ratchet). So an add-one-tag / remove-one-tag / mixed deploy no longer severs unrelated subscriptions. OpcUaServer.Tests 336/336, Runtime.Tests 398/398, Commons forwarding guard green. **Deferred:** the over-the-wire `SubscriptionSurvivalTests` (add/remove/mixed) + the docker-dev live-`/run` gates (`kind=PureAdd/PureRemove/AddRemoveMix, rebuild=False`) — authored + compile-green, run in the serial heavy pass. |
|
||||
| P2 | Per-value global-lock write, no batching | **STILL OPEN.** `OtOpcUaNodeManager.cs:266-281` unchanged. Same guard-unblocked, not-started status as P1. |
|
||||
| P3 | HistoryRead-Events unpaged, `0` → `int.MaxValue` | **STILL OPEN.** `OtOpcUaNodeManager.cs:1914-1954` unchanged. |
|
||||
| P4 | Deploy re-runs four Materialise passes (low) | **STILL OPEN.** `OpcUaPublishActor.cs:338-354` unchanged. |
|
||||
|
||||
@@ -369,3 +369,32 @@ Tasks are bite-sized TDD increments. **Classification:** `high-risk` = touches t
|
||||
- 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.
|
||||
|
||||
@@ -5,92 +5,127 @@
|
||||
{
|
||||
"id": "T1",
|
||||
"subject": "Phase 1: AddressSpaceChangeClassifier + table-driven tests (move TagDeltaIsSurgicalEligible/VtagDeltaIsNodeIrrelevant)",
|
||||
"status": "pending",
|
||||
"status": "completed",
|
||||
"blockedBy": []
|
||||
},
|
||||
{
|
||||
"id": "T2",
|
||||
"subject": "Phase 1: route AddressSpaceApplier.Apply through the classifier — PureAdd skips rebuild (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T1"]
|
||||
"subject": "Phase 1: route AddressSpaceApplier.Apply through the classifier \u2014 PureAdd skips rebuild (high-risk)",
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T3",
|
||||
"subject": "Phase 1: ComputeAddAnnouncements + AnnounceAddedNodes on the applier (dedup parent NodeAdded announces)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T1"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T4a",
|
||||
"subject": "Phase 1: idempotent MaterialiseAlarmCondition (skip-if-present-and-same-kind) (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T1"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T4b",
|
||||
"subject": "Phase 1: OpcUaPublishActor calls AnnounceAddedNodes after the Materialise passes on non-rebuild applies (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T2", "T3", "T4a"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T2",
|
||||
"T3",
|
||||
"T4a"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T5",
|
||||
"subject": "Phase 1: over-the-wire SubscriptionSurvivalTests — subscribe, pure-add, item survives + new node browsable",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T4b"]
|
||||
"subject": "Phase 1: over-the-wire SubscriptionSurvivalTests \u2014 subscribe, pure-add, item survives + new node browsable",
|
||||
"status": "deferred-live",
|
||||
"blockedBy": [
|
||||
"T4b"
|
||||
],
|
||||
"note": "authored + compiles (dotnet build green); NOT run \u2014 *.IntegrationTests suite leaks ~16GB/run per the memory constraint. MUST run in the serial heavy integration pass (single-test, boots one in-process server + client)."
|
||||
},
|
||||
{
|
||||
"id": "T6",
|
||||
"subject": "Phase 1: MANDATORY live-/run gate on docker-dev (rebuild BOTH centrals; Client.CLI subscribe survives +1-tag deploy; kind=PureAdd rebuild=False) — Phase 1 shippable boundary",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T5"]
|
||||
"subject": "Phase 1: MANDATORY live-/run gate on docker-dev (rebuild BOTH centrals; Client.CLI subscribe survives +1-tag deploy; kind=PureAdd rebuild=False) \u2014 Phase 1 shippable boundary",
|
||||
"status": "deferred-live",
|
||||
"blockedBy": [
|
||||
"T5"
|
||||
],
|
||||
"note": "docker-dev live /run surgical-path; serial heavy pass \u2014 F10b inertness risk, MUST run (subscribe survives +1-tag deploy; central log kind=PureAdd, rebuild=False; new node browsable/Good)."
|
||||
},
|
||||
{
|
||||
"id": "T7",
|
||||
"subject": "Phase 2: add RemoveVariableNode/RemoveAlarmConditionNode/RemoveEquipmentSubtree to ISurgicalAddressSpaceSink + DeferredAddressSpaceSink forwarding — guard-first with observed negative control (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T6"]
|
||||
"subject": "Phase 2: add RemoveVariableNode/RemoveAlarmConditionNode/RemoveEquipmentSubtree to ISurgicalAddressSpaceSink + DeferredAddressSpaceSink forwarding \u2014 guard-first with observed negative control (high-risk)",
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T6"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T8",
|
||||
"subject": "Phase 2: OtOpcUaNodeManager.RemoveVariableNode + BuildNodesRemovedModelChange (NodeDeleted) (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T7"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T7"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T9",
|
||||
"subject": "Phase 2: OtOpcUaNodeManager.RemoveAlarmConditionNode (+ native-flag cleanup) (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T7"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T7"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T10",
|
||||
"subject": "Phase 2: OtOpcUaNodeManager.RemoveEquipmentSubtree (descendants + notifier demotion) + SdkAddressSpaceSink forwards (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T8", "T9"]
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T8",
|
||||
"T9"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T11",
|
||||
"subject": "Phase 2: applier PureRemove arm — terminal Bad/RemovedConditionState writes, equipment subsumption, false-to-rebuild ratchet (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T10"]
|
||||
"subject": "Phase 2: applier PureRemove arm \u2014 terminal Bad/RemovedConditionState writes, equipment subsumption, false-to-rebuild ratchet (high-risk)",
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T10"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T12",
|
||||
"subject": "Phase 2: remove-side subscription-survival integration test + live-/run remove gate (kind=PureRemove rebuild=False) — Phase 2 shippable boundary",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T11"]
|
||||
"subject": "Phase 2: remove-side subscription-survival integration test + live-/run remove gate (kind=PureRemove rebuild=False) \u2014 Phase 2 shippable boundary",
|
||||
"status": "deferred-live",
|
||||
"blockedBy": [
|
||||
"T11"
|
||||
],
|
||||
"note": "remove-side SubscriptionSurvivalTests authored + compiles (dotnet build green); NOT run \u2014 *.IntegrationTests memory constraint. Live docker-dev pure-remove gate (survivor subscription uninterrupted; central log kind=PureRemove, rebuild=False; removed node reads BadNodeIdUnknown) MUST run in the serial heavy pass \u2014 F10b inertness risk."
|
||||
},
|
||||
{
|
||||
"id": "T13",
|
||||
"subject": "Phase 3: AddRemoveMix routing — removes-then-adds surgically, same-NodeId remove-then-recreate ordering test (high-risk)",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T12"]
|
||||
"subject": "Phase 3: AddRemoveMix routing \u2014 removes-then-adds surgically, same-NodeId remove-then-recreate ordering test (high-risk)",
|
||||
"status": "completed",
|
||||
"blockedBy": [
|
||||
"T12"
|
||||
]
|
||||
},
|
||||
{
|
||||
"id": "T14",
|
||||
"subject": "Phase 3: mixed-deploy integration test + live-/run mixed gate + STATUS.md/P1 close-out — plan complete",
|
||||
"status": "pending",
|
||||
"blockedBy": ["T13"]
|
||||
"subject": "Phase 3: mixed-deploy integration test + live-/run mixed gate + STATUS.md/P1 close-out \u2014 plan complete",
|
||||
"status": "deferred-live",
|
||||
"blockedBy": [
|
||||
"T13"
|
||||
],
|
||||
"note": "mixed-deploy SubscriptionSurvivalTests authored + compiles (dotnet build green); docs close-out DONE (STATUS.md + 03/P1 marked REMEDIATED + plan deviations). Live docker-dev mixed gate (one deploy +1/-1; kind=AddRemoveMix, rebuild=False; survivor uninterrupted) MUST run in the serial heavy pass \u2014 F10b inertness risk."
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -179,6 +179,8 @@ what the unit leg cannot; (c) migrate the 3 xunit v2 holdouts once Akka ships an
|
||||
| **R2-08** | 03/S2 (High, action #8), 03/S3 (High) | `r2/08-ldap-historyread-async` (off `1a698cbb`) | `ce6d9e21`…`<docs>` (14 task commits) | **Code-complete, offline-verified.** **S2 (LDAP async/timeout):** the SDK raises the impersonation callback synchronously under a `SessionManager`-wide `m_eventLock` (verified vs. 1.5.378 — a hung bind serializes ALL activations), so the fix is entirely at the `LdapOpcUaUserAuthenticator` boundary: five new `Security:Ldap` keys (`ConnectionTimeoutMs`=10000 projected into the shared lib, `AuthTimeoutMs`=15000 hard whole-flow `WaitAsync` bound → fail-closed "timed out", `MaxConcurrentAuthentications`=8 `SemaphoreSlim` with release-on-core-completion so orphaned binds still count, `OutageFailureThreshold`=3/`OutageCooldownSeconds`=15 directory-outage circuit → fast-deny "unavailable", half-open probe) + `LdapAuthResult.IsSystemFailure` seam (mapped from `ServiceAccountBindFailed`) distinguishing outage from credential-deny + `LdapOptionsValidator` rules + `HandleImpersonation` contract doc + `docs/security.md`. Stall-repro RED→GREEN; offline outage-sim through the real `HandleImpersonation` against an unroutable host. **S3 (HistoryRead fan-out):** all four HistoryRead arms async-ified (`ServeNodeAsync`/`ServeRawPagedAsync`/`ServeEventsAsync`, OCE→`BadTimeout`) then bounded per-node fan-out (`RunBounded` semaphore + single arm-boundary bridge) + per-request deadline CTS (`HistoryReadDeadline`=60s) + process-wide lazy batch limiter (`MaxConcurrentHistoryReadBatches`=16 → flood degrades to `BadTooManyOperations`); 16×4=64 in-flight cap; three new `ServerHistorian` keys + node-manager props + hosted-service wiring + `docs/Historian.md`/CLAUDE.md tables. Sequential-serving RED→GREEN. Host.IntegrationTests (LDAP resilience) 13/13, Security.Tests 11/11, Runtime.Tests options 11/11, OpcUaServer.Tests HistoryRead 30/30. **Deferred (live):** S2 docker-dev unreachable-LDAP + GLAuth outage/recovery cycle; S3 `Category=LiveIntegration` gateway soak. |
|
||||
| **R2-11** | 01/C-1, 01/P-1 + 05/CONV-2, UNDER-1, UNDER-6 | `r2/11-tagconfig-consolidation` (off `46fedda3`) | first `2e14fe1f` … (24 task commits) | **Code-complete, offline-verified.** **Part 1 (C-1/P-1):** single `TagConfigIntent.Parse` + `DeviceConfigIntent` in Commons replace the four byte-parity TagConfig parse copies (composer/artifact/walker/DraftValidator) — composer now parses once per tag (P-1); a permanent `Runtime.Tests/TagConfigCorpusParityTests` (30-blob corpus) guards compose≡decode; `grep "MUST parse identically" src/` = 0. **Part 2 (CONV-2):** shared strict-capable `TagConfigJson` readers in Core.Abstractions replace the six copy-pasted `ReadEnum`s; all six driver parsers gain `Inspect()` warnings + honour the `writable` key (FOCAS **forced read-only**, 05/UNDER-1); FOCAS equipment tags run the capability-matrix pre-flight (05/UNDER-6); Modbus probe parses the factory DTO shape (`timeoutMs` no longer dropped). New `EquipmentTagConfigInspector` in ControlPlane + `AdminOperationsActor` deploy-gate wiring with `Deployment:TagConfigValidationMode` (**Warn default | Error opt-in**), proven through the actor (F10b consuming test). AdminUI: `writable` checkbox in the six typed editors. **Phase-C follow-up (deferred, recorded here):** flip the runtime parsers to strict (typo'd enum ⇒ `BadNodeIdUnknown` instead of wrong-width `Good`) once fleets have run `Error` mode clean — behavior change deliberately not shipped in this plan. **Deferred (T22/T24):** docker-dev `/run` of one typed editor's Writable checkbox + whole-solution serial sweep. |
|
||||
|
||||
| **R2-07 (Phases 1+2+3)** | 03/P1 (action #7, surgical pure-adds/removes/mixed) | `r2/07-surgical-pure-adds` (off `1a698cbb`) | `bb226f45`…`e2b47638` (T1–T14) | **Phases 1+2 code-complete, offline-verified.** New pure `AddressSpaceChangeClassifier` (Empty/AttributeOnly/PureAdd/PureRemove/AddRemoveMix/Rebuild; first-match-wins, default-closed Rebuild; the F10b tag/vtag surgical-eligibility predicates moved here as `internal static`). `AddressSpaceApplier.Apply` routes by kind. **Phase 1 (PureAdd/AttributeOnly):** skip `RebuildAddressSpace` (the idempotent Materialise passes create exactly the added nodes; existing subscriptions survive); adds announced via `ComputeAddAnnouncements`+`AnnounceAddedNodes` (dedup affected-parent → existing guard-covered `RaiseNodesAddedModelChange`), fired by `OpcUaPublishActor` after the 4 passes gated on `!outcome.RebuildCalled`; `MaterialiseAlarmCondition` made idempotent (skip-if-present-and-same-kind keeps the condition NodeState alive). **NO new sink members in Phase 1.** **Phase 2 (PureRemove):** 3 new `ISurgicalAddressSpaceSink` members (`RemoveVariableNode`/`RemoveAlarmConditionNode`/`RemoveEquipmentSubtree`) — guard-first with the **observed negative control** (`DeferredSinkForwardingReflectionTests` fails member-named until `DeferredAddressSpaceSink` forwards) + `SdkAddressSpaceSink` forwards + `OtOpcUaNodeManager` in-place teardown (scoped map cleanup, notifier demotion via `RemoveRootNotifier`, `BuildNodesRemovedModelChange` verb=NodeDeleted reported outside `Lock`). Applier `PureRemove` arm: terminal Bad / `RemovedConditionState` writes (closing the removed-alarm-tag today-gap) → subsumption partition (removed-equipment children folded into the subtree remove) → in-place removes → any false/throw ⇒ full-rebuild ratchet; non-surgical sink ⇒ rebuild. **Phase 3 (AddRemoveMix):** removes-then-adds within one apply — the remove pass runs in `Apply` (no rebuild), then the publish actor's Materialise passes create the adds + `AnnounceAddedNodes` announces them; an id reused across the remove+add sets is remove-then-recreated as a fresh NodeState (correct — a different tag). **Guard stays green (Commons.Tests 17/17), OpcUaServer.Tests 336/336, Runtime.Tests 398/398.** **03/P1 marked REMEDIATED in `archreview/03-server-runtime.md`.** **Deferred-live (T5, T6, T12, T14):** `SubscriptionSurvivalTests` add + remove + mixed cases (authored + compile-green; NOT run — `*.IntegrationTests` memory constraint) and the docker-dev live-`/run` add/remove/mixed gates (`kind=PureAdd/PureRemove/AddRemoveMix, rebuild=False`; survivor subscription uninterrupted; removed node → `BadNodeIdUnknown`) — MUST run in the serial heavy pass (F10b inertness risk). |
|
||||
|
||||
**Report anchor drifts to fold into the next report refresh** (from the R2-02 verification pass): (1)
|
||||
`01-core-composition.md` U-6 anchor `DriverResilienceOptions.cs:23-25` → the two bulkhead properties sat at
|
||||
`:23` and `:29` (now deleted); (2) `01-core-composition.md` U-6 anchor
|
||||
|
||||
Reference in New Issue
Block a user