4 Commits

Author	SHA1	Message	Date
romanett	9b034f8291	Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) ... # Description Nullable reference types are now enabled once, in `common.props`, and every project in the repository builds warning-clean with them. **Configuration** - `<Nullable>enable</Nullable>` lives in `common.props`. All per-project `<Nullable>` settings are removed: 178 project files (enable, annotations, and the `disable` in the three Aot test projects). - 419 per-file `#nullable enable`/`restore` directives and the 4 `#nullable disable` directives are removed. - Kept on purpose: - The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`, `PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the project setting in generated code, so the directive is required there. - `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against the 1.5.378 packages and does not import the repository props. - `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now describe the central setting. **Migration** (the 43 unset and 13 annotations-only projects, roughly 10k diagnostics) - **Production code: annotated at the declarations.** Covers `tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the LDS sample. - Members, parameters and returns are nullable where null actually flows. - Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`, `IsMethodTypeDesign`). - `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take `params object?[]`. `Template.AddReplacement` takes `string?` (rendering already skips null replacements). - `IRegisteredServerStore.RegisterAsync` returns `Task<RegistrationEntry?>`: it returns null when a registration is removed. - Dereferences backed by an invariant (for example, the validated model) use `!`, so runtime behaviour is unchanged. - **Behavioural changes in production code** (small, deliberate): - `NodeSetToModelDesign` throws `InvalidOperationException` when the file is not a NodeSet. Before, it threw a NullReferenceException on the next line. - `ModelDesignValidator` starts its lookup tables empty instead of null. A redundant `if (m_nodes == null) return;` guard is gone. - **Test code: minimal annotations.** - `?` on values that can be null, `!` where the test asserts non-null. - `= null!` for fixture members assigned in SetUp. - `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the `IEncodeable` constraint. - A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`) folded into the adjacent `IsNull`/`Count` checks. **Notes for reviewers** - The diff is large (~1,500 files) but almost entirely mechanical: `?`, `!` and directive removal. The non-annotation changes are listed above. - Known follow-up, not done here: `IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay non-nullable even though the validator returns null before it loads a dictionary. Making them nullable would ripple through every generator call site. ## Related Issues - Fixes #4439 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. The "no new warnings" box covers net10.0 and net48: full non-incremental builds of `UA.slnx`, `tools/SourceGeneration.slnx`, `tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0 warnings and 0 errors under the repository's warnings-as-errors settings. net9.0 and net8.0 also had no nullable diagnostics, but they were not rebuilt after the final fixes. Tests have not been run locally yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-08 08:48:13 +02:00
romanett	7212607f16	OPC UA Part 3 compliance: permissions, Write semantics, NodeManagement, NodeSet DataTypes, locales (#4675) ... # Description Fixes the High and Medium findings of an OPC UA Part 3 (OPC 10000-3, Address Space Model) compliance audit. Each finding was checked against the full spec text (Parts 3, 4, 5, 6, 14 and 18) before it was fixed. Every fix comes with regression tests that fail without it. ### Role permissions (Part 3 §8.55, Part 4 §5.12.2.2) - The type-hierarchy exemption in `MasterNodeManager.ValidateRolePermissions` / `ValidateAccessRestrictions` now covers only Browse, Read and ReadRolePermissions. Type nodes stay visible to generic clients, which was the intent of #3705; every other permission bit is now enforced on them. - `ReceiveEvents` is enforced on the EventType node. The standard NodeSet grants it on `AuditEventType` and its subtypes only to SecurityAdmin, so only SecurityAdmin sessions now receive audit events. - Call checks the `Call` permission and AccessRestrictions on the Object as well as on the Method. - When a Call's `methodId` is the Method declaration of the ObjectType, the Object's Method with the same BrowseName is permission-checked and invoked. The declaration is used only if the Object has no such Method. This applies to `AsyncCustomNodeManager`, `CustomNodeManager2` and the sample node manager. ### Write service (Part 3 §5.6.2, §8.40, §8.57; Part 4 §5.11.4) - The synchronous `OnWriteValue` path (fluent `OnWrite`) checks DataType and ValueRank first (`Bad_TypeMismatch`). After an IndexRange write it merges the slice into the cached value instead of replacing it. - StatusWrite / TimestampWrite are enforced (`Bad_WriteNotSupported`). - Undefined enumeration values and invalid OptionSet writes return `Bad_OutOfRange`. OptionSet writes merge only the valid bits into the stored value. - New `ReportPropertyValueChanged` / `ReportSemanticChange` let server code raise SemanticChangeEvents for changes it makes itself. - Required follow-up (Part 14 §6.2.11.1): if an external target rejects a timestamp, the PubSub subscriber's TargetVariables writer retries without it. ### NodeManagement and Call arguments (Part 3 §5.5.2, §5.7.3, §6.4.2; Part 4 §5.8, §5.12.2) - AddNodes creates the Mandatory InstanceDeclarations of the TypeDefinition and rejects abstract or Interface types (`Bad_TypeDefinitionInvalid`). - AddReferences, DeleteReferences and DeleteNodes bump NodeVersion and raise GeneralModelChangeEvents. - Clients may omit trailing optional input arguments (`HasOptionalInputArgumentDescription`). ### NodeSet DataTypes (Part 3 §5.8.3, §8.48; Part 6 Annex F.12) - `UANodeSet.Import` completes structure definitions with the base type's fields (base fields first). It creates definitions for structures without fields and sets `DefaultEncodingId`. `StructureDefinition.FirstExplicitFieldIndex` marks the subtype's own fields. - `DataTypeState` no longer mutates the stored definition on read. ### Locales (Part 3 §5.2.5, §8.5; Part 4 §5.4) - Read, Browse and data change notifications return the translation that best matches the session's LocaleIds, or all translations for `mul`. The selection lives in the server dispatcher and is built on `ResourceManager`: the new `TranslateValue` calls the overridable `Translate`. - Locale ids are matched case-insensitively, and fallback translations carry their real locale. ### Client complex types (Part 6 §5.1.7) - The Reflection.Emit builder encodes only `IsOptional` subtyped fields as ExtensionObject. It also supports OptionSet subtypes: an unsupported type is skipped instead of aborting the whole load. - `TypeInfo.GetXmlName` uses the data type's XML name for dynamic structures and OptionSets (the XML body element was `<Structure>`). Both builders now produce identical XML. ## Behaviour changes All are documented in `docs/MigrationGuide.md`. - **Audit events:** only SecurityAdmin receives them. The CTT "Auditing Connections" unit needs a SecurityAdmin user (e.g. sysadmin); `AuditingOperationTests` was updated. - **Write timestamps and status:** clients writing a SourceTimestamp or a non-Good StatusCode get `Bad_WriteNotSupported` unless the Variable has StatusWrite/TimestampWrite. No variable in `src/` sets these bits. - **AddNodes:** it rejects abstract types (including `BaseVariableType` and `BaseEventType`), and new instances now come with child nodes. - **Call:** the Object needs the Call permission. The Object's own Method runs even when the call names the type declaration. ## Not included - Low-severity findings: missing validation on optional services and author-side NodeSet rules. - LocalizedText nested inside structures or Variant arrays, and in HistoryRead or Call outputs. ## Testing net10.0, Debug, one suite at a time on the merged branch. All suites below pass: | Suite | Passed | |---|---| | Types | 11430 | | Core.Encoders | 2947 | | Core.Schema | 179 | | Core.Security | 558 | | Client | 2761 | | Server | 8210 | | InformationModel | 1080 | | Features | 145 | | History | 536 | | Di | 455 | | Machinery | 167 | | Robotics | 661 | | Robotics.Intent | 22 | | Pumps | 75 | | Scales | 122 | | Positioning | 69 | | ISA95 | 144 | | Vision | 484 | | PubSub | 1490 | | PubSub.Adapter | 151 | | PubSub.Server | 177 | | Redundancy.Server | 856 | | Lds | 173 | | Configuration | 241 | | WotCon | 1496 | - **Client.ComplexTypes:** one test wrote a SourceTimestamp to a variable without TimestampWrite. It is fixed in this PR, and the suite has not been re-run since. - **Core:** 6068 passed, 1 failure in `ActivityTraceFlags_Metrics_ShouldRecordMetricsOnly`. That telemetry test passed when re-run alone, so it looks flaky. - **Still running:** Sessions, Subscriptions (Classic, Durable, standard) and Interop. Results will follow on this PR. - `UA.slnx` builds with 0 errors for net10.0 and net48, with no new analyzer warnings in changed files. ## Related Issues - Part 3 compliance audit (no tracking issue) ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-05 12:43:59 +00:00
Marc Schier	1cca744b5c	Retire the last exposed locks: node, browser, server diagnostics and application properties (#4222) ... Closes #4201. Closes #4202. Closes #4203. Closes #4215. Closes #4216. (#4215 and #4216 are @marcschier's originals from the #4188 review; #4202 and #4201 are my later duplicates of the same two findings. All four describe the two changes in section 1 and are closed together.) **This PR now carries what was #4221 as well.** The two were a stack; they have been unstacked and combined here, and #4221 is closed. Everything below the "Read the server diagnostics" section is the work that was previously reviewed as #4221. --- ## 1. The browse path guards itself, and the last node locks are gone `AsyncCustomNodeManager` kept two `lock (source)` sites around `CreateBrowser`, carrying a NOTE that browser creation was not internally synchronized. `NodeBrowser` still handed its lock to derived types through `protected Lock DataLock`. `NodeState.CreateBrowser` now holds a browse lock across `PopulateBrowser` and `OnPopulateBrowser`, so browser construction on a node is serialized — which is what the external `lock (source)` was actually providing, and what a handler that mutates the node during population (the historian's lazy config installer) depends on. Both `lock (source)` statements and the NOTE are gone. **What that lock does not do**, stated plainly in the XML docs and in `docs/NodeManagers.md` rather than over-promised: it is not an atomic snapshot across the node's children, notifiers and references. Writers take those collections' own locks, not the browse lock. The browser is a point-in-time copy and each collection is read consistently, but the combination is not a transaction. No `lock` takes a `NodeState` as its sync root any more. That was the last thing keeping `NodeState` from being documentably self-guarding, so the other eleven went with it: `lock (activeNode)`, `lock (target)`, `lock (source.Node)`, `lock (target.Node)`, `lock (notifier)`, `lock (handle.Node)` in `AsyncCustomNodeManager`, and `lock (node)` in `CustomNodeManager2`. Most were a check-then-act pair now served by `NodeState.AddReferenceIfMissing`, which does the check and the insert under the node's own lock. `NodeBrowser.DataLock` is removed. A browser is **single-consumer**: it belongs to whoever created it, and both server browse paths already serialize on the owning side. The exposed lock only added an inheritance-level locking contract that a derived browser could not reason about. ## 2. Read the server diagnostics under the lock every writer takes `OnUpdateDiagnostics` locked the diagnostics **payload** rather than the lock every writer takes through `UpdateServerDiagnostics`, so the snapshot a client reads from the `ServerDiagnostics` node was never excluded from a concurrent writer. Both halves matter, and the second is the one that is easy to miss: ```csharp lock (m_diagnosticsLock) // was: lock (ServerDiagnostics) { value = Variant.FromStructure(ServerDiagnostics, copy: true); // was: copy defaulted to false } ``` `copy: true` is what makes it a snapshot. `FromStructure(value, copy: false)` does `new ExtensionObject(value, copy)` — it wraps the **live** structure without copying it, so the caller would read the fields long after the lock was released and see them change underneath. Swapping the lock alone would have looked like a fix while still excluding nothing. ## 3. Analyzer numbering The browser rule is **UA0027**, not UA0025. This branch had independently taken UA0025 for `NodeBrowser.DataLock` while #4188 published UA0025 for `ILocalNode.DataLock` and UA0026 for `BaseVariableValue.Lock`. Master's are shipped, so they keep the numbers: | | UA0025 | UA0026 | UA0027 | |---|---|---|---| | master (#4188) | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | — | | **this PR** | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | **`NodeBrowser.DataLock`** | Both sides had written a different `UA0025Tests.cs`, which arrived as an add/add conflict. They are now split: `UA0025Tests.cs` holds master's `ILocalNode` tests, `UA0027Tests.cs` holds the `NodeBrowser` ones. UA0024's help text also drops its reference to `ReadServerDiagnostics`, which #4193 removed as unused, and names `ISession.ReadDiagnostics` / `ISubscription.ReadDiagnostics` instead — those do still exist. ## 4. Two more locks reachable from outside their owner **`ApplicationConfiguration.PropertiesLock` is removed.** It returned the properties dictionary itself, so `lock (configuration.PropertiesLock)` was `lock (configuration.Properties)` — the lock *was* the data it guarded, the same shape as #4203. It had exactly one production consumer (`OAuth2CredentialCollection.Load`, a get-or-add with `ParseExtension` called inside the lock), so it could not simply be deleted. `Properties` is now a `ConcurrentDictionary` — same public type, every operation atomic on its own — and the one multi-operation need gets a member of its own: ```csharp public T GetOrAddProperty<T>(string key, Func<T> valueFactory); ``` It deliberately does **not** invoke the factory under a lock, so `ParseExtension` now runs outside any critical section. Holding a lock across a caller-supplied callback is exactly the shape that produced the attribute/diagnostics deadlock earlier in this work. The two existing tests asserted only `PropertiesLock is not null`, so they tested nothing but its presence. They are replaced by tests of the contract: factory runs once for a missing key, not at all for a present one, 32 concurrent callers all observe the same instance, null arguments throw. `PropertiesLock` ships in 1.5.378, so this is a breaking removal — migration guide section plus analyzer **UA0028**. **`SharedDefinitionCache` no longer hands out its lock.** Three sites in `SharedKeyValueSubscriptionStore` took the nested cache's `Lock` to operate on the cache's own dictionary. Both are private now, behind `ReplaceAll` and `CloneWhere`. `SnapshotCommitLock` stays exposed on purpose: it is a `SemaphoreSlim` sequencing an async commit across `await` points, which `System.Threading.Lock` cannot span, so folding it in would change behaviour rather than hide a lock. ## Verification Merged `master` (`0edfe237a`, including #4234) with no conflicts. - `dotnet build UA.slnx` — 0 warnings, 0 errors - `Opc.Ua.Server.Tests` — 4074 passed - `Opc.Ua.Types.Tests` — 8501 passed - `Opc.Ua.MigrationAnalyzer.Tests` — 151 passed on all five TFMs (138 → 151, the new ones being UA0027 and UA0028) - `Opc.Ua.Core.Tests` — 4249 passed - `Opc.Ua.Redundancy.Server.Tests` — 573 passed ## Note for the reviewer @romanett — your approval was given when this PR's diff was the 7 diagnostics files, because it was measured against `marcschier/node-browse-locks`. Now that the two are combined and the base is `master`, the diff is 23 files, and the extra 16 are the node/browser lock work that was #4221. GitHub kept the approval across the retarget, but it did not cover that code, so please re-review rather than let it ride. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63 Copilot-Session: 0ee22c1d-c319-4257-bf78-a5a077a1824a	2026-08-11 22:14:10 +02:00
Marc Schier	e05a820c91	Removal of the exposed diagnostics and node locks (#4188) ... ## What this is Design analysis for `Opc.Ua.Server`, the two rounds of design exploration behind it, and the first two implementation chunks it recommends. Draft: the analyses are ready to read, but the *conclusions* deserve discussion before the larger phases proceed. Later chunks stack onto this PR as they land. | File | Kind | Lines | |---|---|---:| | `plans/NodeManagerAnalysis.md` | Evidence — the node-manager plugin surface | 488 | | `plans/ServerRuntimeAnalysis.md` | Evidence — the server runtime cluster | 576 | | `plans/ServerDesignExploration.md` | Design round 2 — delivery strategy | 1,394 | | `plans/ServerDesignExplorationAlt.md` | Design round 1 — four interface designs | 2,119 | | `src/Opc.Ua.Types/State/NodeState.cs` | **Code** — node self-guards attributes | | | `src/Opc.Ua.Types/State/BaseVariableState.cs` | **Code** — value accessors take the lock | | | `src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs` | **Code** — 6 external locks removed | | | `tests/…/NodeStateAttributeConcurrencyTests.cs` | **Code** — concurrency safety net | 449 | `docs/` is untouched — it holds shipped feature documentation, and none of this is shipped API. ## The analyses Counted mechanically over `src`, `samples` and `tests`; **every figure is reproducible** from commands printed in each document. **Node manager surface.** `AsyncCustomNodeManager` declares **93 `virtual`/`abstract` members** (80 distinct names). Across the repository: **112 override sites, 25 distinct names ever overridden, 55 of 80 never**. Six carry nearly all real use. For scale, `ReferenceNodeManager` is 6,373 lines and overrides **8**. **Server runtime.** Seven findings, **two of which recommend no change**. Findings 1–5 are filed as #4183–#4187. The headline is an `object`-typed sync root published on three public interfaces and taken in **88 `lock` statements across 7 files**, including a shipped sample. **What is deliberately not flagged.** `IStandardServer` — 9 members over 5,575 lines, 16 of 37 virtuals used — is identified as the reference shape to preserve. `IApplicationInstance` and the already-`[Obsolete]` `Stop()` are examined and left alone. ## The design exploration **Round 1** explored four alternative interfaces in parallel under the current back-compat constraint. Its value is the convergence: four designers who never saw each other's work independently arrived at a 6–9 member `IServerContext` replacing the 57-member locator, removal of the `object` node-manager handle, elimination of the `Processed` protocol, and `IHistorianProvider` as the pattern to copy. **Round 2** re-runs it with back-compat lifted and asks how to deliver: full rewrite versus side-by-side fork. **Both option designers independently declined to recommend their own assignment.** Three structural facts were verified: 1. **The fork line already sits at `IAsyncNodeManager`** — old and new node managers can coexist with zero runtime changes, so a fork duplicates ~10% of the assembly, not all of it. 2. **`[OpcUaShim]` cannot shim an inheritance surface** — honest auto-fix ceiling for the authoring surface is **~10%**, against ~70% for the shipped analyzers. 3. **A second lock leak exists** — `NodeState` takes `lock (this)` under a `CA2002` suppression whose own comment documents the contract. Recommendation: **neither pure option**, but a staged path reaching the rewrite end state incrementally, gated on migrating the five framework-internal node managers that today make it impossible for the old base to ever freeze. ## Implementation: NodeState now guards its own attributes This removes the prose-only contract in which `NodeState` took `lock(this)` and every external caller was required to take `lock(source)` on the node instance. The code already carried a TODO describing exactly this fix and noting it required updating every external site. - The node self-guards rather than relying on callers. `BaseVariableState` gains a **fully private** `System.Threading.Lock` covering its value fields; `NodeState`'s other collections keep their existing private locks. - All **6** `lock(this)` sites and both `CA2002`/`RCS1059` suppressions are gone. - `Value`, `Timestamp` and `StatusCode` on `BaseVariableState` take the lock too — without that, a direct property write raced an attribute read once the external lock was gone. - **6 of 9** external sites removed. Five were already redundant (`GetReferences`, `GetChildren`, `ReferenceExists` and `SetAreEventsMonitored` guard themselves; `FindMethod` reaches its state via `GetChildren`); the sixth was an attribute read now covered by the node's own lock. - Attribute read/write deliberately take **no** node-wide lock: both dispatch to caller-supplied handlers (`OnReadValue` and siblings) that may take locks of their own, and holding a lock on the node across one gives the node no fixed place in the lock order. - **3 deliberately retained and annotated in code**: two `CreateBrowser` calls, which are not internally synchronized (tracked in #4216), and one in `CustomNodeManager`, whose class is slated for deletion so refactoring its lock would be wasted work. ### No lock is exposed anywhere, to anyone An earlier revision put the attribute lock on `NodeState` as `private protected`, which still published a lock-sharing contract on the inheritance surface. Review pushed back and it is gone: `BaseVariableState` was the only user, so the lock moved there as a plain `private readonly Lock`. There is no member for a derived type to call, in this assembly or any other. `BaseVariableValue` was the last public leak — it handed out the lock guarding its value through a `public Lock` property, so any caller could hold it across a call back into the stack. That property is removed: - Derived value classes (what the source generator emits for every structure variable) synchronize through a `protected EnterLock()` / `ExitLock()` pair. Callback helpers were the first choice, but the generated bodies pass `ref Variant` / `ref StatusCode` / `ref DateTimeUtc`, and `ref` arguments cannot be captured by a lambda. The lock *object* stays private either way. - `NodeStateTemplates` emits the new form. - `ServerInternalData`, the one cross-assembly caller, was reading the lock back off a value it had itself constructed with that lock. It now takes `m_diagnosticsLock` directly. `NodeBrowser.DataLock` remains `protected` and is tracked in #4215: a subclass extending `Next()` legitimately needs its enumeration state atomic with the base browser's, so the fix needs a base-owned acquisition rather than a deletion. ### Migration coverage for the removed lock members None of the removed lock members get an `[Obsolete]` shim, and the migration guide now says why: a shim would have to be `object`-typed to keep old call sites compiling, and on .NET 9+ `lock (o)` on a statically-`object` reference is `Monitor.Enter`, which does **not** interlock with `Lock.Enter()`. It would compile, run, and silently synchronize nothing — strictly worse than a compile error. Re-adding a member to a downstream-implemented interface is also a break in its own right on `net48` / `netstandard2.0`, where there are no default interface methods. Instead, every removed member has a named replacement on the migration path: | Rule | Covers | Replacement it names | |---|---|---| | `UA0024` | `IServerInternal` / `ISession` / `ISubscription` `DiagnosticsLock`, `DiagnosticsWriteLock` | `UpdateServerDiagnostics(...)` / `UpdateDiagnostics(...)`, `ReadServerDiagnostics` / `ReadDiagnostics` | | `UA0025` | `ILocalNode.DataLock`, `Node.DataLock` | a lock the caller owns; the node guards its own state | | `UA0026` | `BaseVariableValue.Lock` | a lock passed to the `BaseVariableValue` constructor | Each rule fires on sources still written against 1.5.378, where the compiler only says a member is missing. `docs/MigrationGuide.md` gains a section per removal with before/after. ### The tests earned their keep The four concurrency tests were landed **first**, against the unmodified code, because removing a synchronization contract cannot be verified by the compiler and the existing `NodeStateHandlerConcurrencyTests` covers a different concern (racing a handler assignment against a write, not reader/writer exclusion). They now run with **no external locking**, which is what demonstrates the node guards itself. They caught two real problems, and one of them was mine: - The `Value`/`StatusCode`/`Timestamp` setters bypassed the new lock — a genuine race. - One test asserted an invariant the design does not offer. Two property setters are two lock acquisitions, so that pair was **never** atomic; it only passed originally because the test held a lock across both. It now uses a single attribute write carrying a whole `DataValue`, which genuinely is atomic and is the path the server uses. A third issue was found and fixed before it shipped: the reader loops gated on `!writer.IsCompleted`, so a reader scheduled after the writer finished performed zero reads — failing two runs in three on `net48`. Converted to `do…while`; now five consecutive green runs. **Verified:** `Stack.State` 116 passed on `net10.0` *and* `net48`; the concurrency category green 5/5 consecutive on `net48`; `Opc.Ua.Types.Tests` 8,456 passed; the server node-manager suite 1,205 passed, including `NodeManagerLifecycleTests` and `AsyncCustomNodeManagerTests` which exercise the changed paths. ## Two corrections to my own published figures Both were found while starting implementation, and both are recorded on #4183. **`ForceDiagnosticsScan()` was overstated.** The analysis claimed the `DiagnosticsWriteLock` getter *"triggers a full address-space diagnostics rescan"*. It is in fact a single flag assignment (`DiagnosticsNodeManager.cs:700-703`) consumed by the read paths and the periodic timer. The defect stands — a side-effecting property getter with a racy write on a synchronisation path — but its cost was misstated. **The contract was 15 sites, not 19.** A naive search matched ten comment and `#pragma` lines that *mention* `lock(source)` while explaining it. The documents now carry a counting command that excludes comment matches. A third finding expanded scope the other way: the `object`-typed lock leak is **repo-wide**, with ten such members across `Opc.Ua.Core`, `Opc.Ua.Types` and `Opc.Ua.Server` — including `Node.DataLock => this` and `ApplicationConfiguration.PropertiesLock => m_properties`, the same defect class as `Subscription.DiagnosticsLock`. ## Caveats stated in the documents - **Nothing in the designs is implemented or benchmarked.** Every performance claim across all seven designs is a design intention, not a measurement. - The larger phases are a **major undertaking against a live plugin API**; the documents say so rather than letting the neatness of the proposals imply otherwise. - The recommended sequencing puts low-risk, self-contained steps first and notes they are **worth doing regardless** of which strategy is eventually chosen. - Round 2 notes the two option designers' cost estimates are **not independent of their assignments**. ## Review guidance The two analyses make factual claims checkable by running the included commands. The design explorations are exploratory and need not be agreed with to merge. The code change is small, test-backed, and independent of which delivery strategy is eventually chosen. Reasonable outcomes include merging as-is, merging only the analyses and the lock fix, or disagreeing with the recommendation while keeping the evidence. --- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63	2026-08-10 06:47:50 +02:00