Mint runtime NodeIds through a single DefaultNodeIdFactory - #4433
Merged
Conversation
Nodes created at runtime got their NodeId either from a per-NodeManager
sequential counter or, in the companion-spec NodeManagers, from a
"{parentIdentifier}_{childName}" concatenation. That convention is
ambiguous - A_B plus C and A plus B_C produce the same identifier - it
discards the parent's identifier type, and it shifts with namespace-table
ordering. Every NodeManager also had to reimplement it.
DefaultNodeIdFactory replaces both with one canonical browse path: a
length-prefixed encoding that records the parent's identifier type and
qualifies cross-namespace parents and browse names by URI, so it cannot
collide on separator characters and is stable across reloads.
NodeIdAssignmentMode selects how that path becomes an identifier - String
carries it verbatim, Numeric, Guid and Opaque project it through SHA-256,
and None disables minting for a NodeManager that assigns NodeIds itself.
The factory is immutable and resolved from dependency injection:
builder.AddNodeIdFactory(...) registers it once, StandardServer threads it
through ServerInternalData, and each AsyncCustomNodeManager rebases it
onto its own namespace via WithDefaultNamespaceIndex rather than mutating
shared state. AsyncCustomNodeManager.New delegates to it, keeping the
sequential counter only for nodes with no browse path to derive from,
such as transient event instances.
Because a child is minted into its parent's namespace, the factory
reproduces what the redundant New overrides did, so the DI, ISA95,
Positioning, Vision and RobotIntent NodeManagers and the Alarm and
Reference samples drop theirs. DiNodeManager keeps a three-line override
because its devices live in the instance namespace rather than the DI
model namespace their parents come from. Overrides that are genuinely
structural (FileSystem paths, Robotics coordinator reservations) or that
mint for transient nodes whose browse paths repeat (Diagnostics,
AliasName, GDS, AI, WotCon) are left alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hrough it
Nodes created at runtime now get their NodeIds from DefaultNodeIdFactory
rather than from a New override per NodeManager. The factory derives a
deterministic identifier from a canonical, length-prefixed browse path,
which is injective where the classic {parent}_{child} concatenation was
not, and it is selected per manager by mode rather than by code.
Every identifier is minted into the NodeManager's own namespace. Neither
the parent's namespace nor the browse name's is consulted: a parent can
belong to a companion-specification model whose NodeIds are fixed by its
NodeSet, and a browse name only names the type that declared the child.
An authored NodeId on a node that stands on its own is kept. A node
hanging off a parent is re-minted, because it reached the factory through
AssignNodeIds walking a subtree copied from a type declaration; keeping
those identifiers would alias every instance onto the type's own nodes,
and the predefined-node index takes the last writer, so the type would
quietly become an instance.
ISystemContext.CreateInstance builds an instance of a generated type and
rebases its subtree through AssignInstanceNodeId, which is the path the
generated CreateInstanceOf<Type> helpers already took. The AI NodeManager
was the one place calling NodeState.Create directly, so its roots kept the
NodeId their state object is born with - the type's own - and it now uses
the shared path.
IAsyncNodeManager extends INodeIdFactory and gains AddNode and
AddRootNotifier, so the fluent surface no longer type-tests for
AsyncCustomNodeManager and no longer falls back to a concatenated
identifier when the test fails. That fallback was itself a source of
divergent NodeIds. AsyncNodeManagerAdapter delegates New to the wrapped
NodeManager, so CustomNodeManager2 behaves exactly as before.
docs/NodeIdAssignment.md describes the mechanisms, the difference between
type declarations and instances, the generated helpers and their
parameters, and the per-NodeManager behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AssignInstanceNodeId retried the null-NodeId allocation twice. The second attempt was identical to the first, so it could only help where a fresh allocation happened to equal the identifier being replaced: a deterministic factory answers the same way both times, and a counter would already have moved past it on the first. The loop collapses to a single forced call. The compatibility branch stays. A factory that mints nothing answers the forced call with a null identifier, and the node keeps what it arrived with, so NodeManagers that assign their own NodeIds are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both looked redundant once NodeId assignment settled on one rule, and neither is. assignInstanceNodeIds suppresses minting, where the rebase rule forces it, so a node copy can avoid consuming identifiers it is about to discard and the generator can build declaration subtrees that keep their model NodeIds. The generator's declaration-constant guard is what tells a caller-assigned NodeId apart from one still carrying the type's, which matters because the rebase path forces unconditionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit 81a95c3.
The retry after the forced call looks redundant for a deterministic factory and is not: a factory that holds state answers each call differently, so an allocator whose next value happens to equal the identifier being replaced hands that identifier straight back, and the retry is what steps past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
A node hanging off a parent was re-minted unconditionally, on the grounds
that it reached the factory through AssignNodeIds walking a subtree copied
from a type declaration. That threw away identifiers callers had chosen:
NodeState.Create applies the NodeId it is handed and then runs the
assignment pass, so a caller naming a parented node explicitly - the flat
tag server names its alarms after the tag path - lost that name and the
node became unreachable.
An identifier already in the NodeManager's own namespace is now kept, as
is one on a node that stands on its own. Only an identifier belonging to
another namespace's model is re-minted, which is what a copied type
declaration carries. A node that must shed an identifier already in this
namespace still says so by clearing it, which AssignInstanceNodeId does.
The Robotics coverage test asserted the old {parent}_{SymbolicName} form
for a node carrying a SymbolicName but no BrowseName; with no browse path
to derive from, such a node falls back to the counter. The WoT comparison
test spelled out a pump's NodeId, and now browses the DI DeviceSet for it
the way a client would.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AddNodeIdFactory had no test: neither overload, nor that a second call replaces the first, nor the argument guards. The coverage report on the pull request flagged the whole extension as uncovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NewRebasesAChildThatStillCarriesADeclarationNodeId put the declaration identifier in the NodeManager's own namespace, which is not where a type declaration lives, and the rule now keeps identifiers from that namespace because a caller can have chosen them. The declaration moves to the model namespace, which is what makes it a declaration. The behaviour the rule gained - keeping an identifier a caller named on a parented node, as NodeState.Create does - had no test, and now has one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AsyncCustomNodeManager and CustomNodeManager2 already point SystemContext.NodeIdFactory at the NodeManager in their constructors. Fourteen derived managers, and the source generator's NodeManager template, repeated the assignment. It points at the NodeManager rather than at its factory so that a subclass overriding New() is still the one node level code reaches - which is exactly why the base class assignment already covers every subclass, including FileSystemNodeManager, whose override encodes the file path. Repeating it in a subclass could only ever be a no-op. Two of the repetitions carried comments claiming a New() override took over from the base assignment. Those overrides are gone, so the comments were describing code that no longer existed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AsyncCustomNodeManager.NodeIdFactory, INodeIdFactoryProvider, ServerInternalData, StandardServer and AddNodeIdFactory all named the concrete DefaultNodeIdFactory, so the only way to put a different rule in front of a NodeManager was to override New() on the NodeManager itself - the override this branch spent its time removing. INodeIdFactory alone cannot carry the seam. A NodeManager needs three things it does not express: which namespace to mint into, which identifier style to mint, and an identifier for a node that does not exist yet. IRebasableNodeIdFactory adds exactly those six members. The namespace is why they are a contract rather than constructor configuration. A namespace belongs to the NodeManager, not to the node, and a manager learns its own index only after the server's namespace table has been extended - later than a factory registered in dependency injection was built. Implementations are immutable and the With methods return a view, so one registration serves managers owning different namespaces. DefaultNodeIdFactory implements the two With methods explicitly, so its public ones keep returning the concrete type and existing chaining needs no cast. Dependency injection now registers under the interface, so a decorator can be registered in the factory's place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
reviewed
Sep 7, 2026
Review feedback on #4433. Drop the Inventory section and the "Why the surrounding machinery stays" rationale: both catalogue the state of this change rather than describe the mechanism, so they go stale the moment anything moves. Drop the What's New entry. The factory is a default that servers do not have to opt into, not a new major feature. The Further reading link to the document stays, since the document itself is still the reference. Replace "injective" with plain wording in the document and in the mode's XML doc. While there: "When an existing NodeId is kept" still described the rule this branch replaced - recognising minted identifiers by re-deriving them and by a reserved counter range. The shipped rule is positional, keeping an identifier that is already in the NodeManager's namespace or that sits on an unparented node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on #4433. Numeric is now the default mode: it is the most compact identifier on the wire and the most readable in a client UI. That makes collisions a real prospect rather than a theoretical one. A 32 bit identifier is a birthday problem - distinct browse paths are expected to share one after roughly 2^16 of them - and the predefined-node index takes the last writer, so a collision would silently replace one node with another. The factory now records what it mints and raises BadConfigurationError instead. A plain set of identifiers cannot do this. AssignNodeIds walks a subtree on every create pass, so the same path is minted repeatedly and a set would report every re-walk as a collision. Each identifier therefore carries a witness - the tail 64 bits of the same hash, which no mode derives an identifier from - and only a differing witness is a collision. String and Counter cannot collide and keep no record at all. Under Numeric the counter fallback mints into the same 32 bit space as the hash, so a counter value can land on a path-derived identifier. The counter is the side free to move, so it steps over the clash rather than reporting it. The canonical path is now built into a stack buffer, or one rented from ArrayPool when it does not fit, rather than a StringBuilder and some ten intermediate strings. In every mode but String it is hashed straight from the span and never becomes a string. One routine both measures and writes, so a segment's reserved length cannot drift from its written one. The path text is unchanged - the tests that pin it pass untouched - so no existing deterministic NodeId moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three review items. Collision detection is now a server-wide switch rather than always on. The record it keeps costs memory that grows with the address space, which is worth paying while a model is being developed and not in production. DefaultNodeIdFactory.DetectCollisionsByDefault is on in a debug build and off otherwise; StandardServer.DetectNodeIdCollisions and the hosting builder's DetectNodeIdCollisions() override it for a whole server. Each AsyncCustomNodeManager reapplies the server's answer to whatever factory it resolves or is assigned, so no NodeManager can opt itself out. With the watch off the record is never allocated. The shared ServerFixture turns it on, so a release test run still exercises the checking. PrepareAuthoredNodeIdsForRegistration skipped a subtree whose root carried a namespace-0 declaration id. HasDeclarationNodeIdCollision looks in this manager's PredefinedNodes, and a namespace-0 declaration belongs to the CoreNodeManager, so it structurally cannot see one. That left the namespace-0 clause as the only net, and it exempted the root - and, because it read the root's namespace, its descendants too. Nothing in the subtree moved and ValidateAuthoredNodeId then rejected it. The clause now tests the candidate rather than the root, but only for numeric identifiers. Every node OPC UA defines in namespace 0 has one, so a namespace-0 identifier of any other kind cannot be a declaration: it is a caller naming a namespace it does not own, which stays an error rather than becoming a silent repair. Removing the clause outright, as first proposed, would have turned that error into a rebase and broken NamespaceZeroNodeIdsAreRejected. The child AddObject helper now takes a plain string, qualified with the builder's namespace, and reports a missing or namespace-0 browse name as BadBrowseNameInvalid like the builder's own Add methods rather than as an ArgumentNullException. INodeManagerBuilder exposes DefaultNamespaceIndex so the helper qualifies names the same way; the generated typed builders forward it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect namespace assignment, browse-name validation, authored NodeId validation, and nullable correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes runtime NodeId assignment through a configurable DefaultNodeIdFactory, replacing manager-specific implementations.
Changes:
- Adds deterministic, counter, numeric, GUID, opaque, string, and disabled assignment modes.
- Integrates factory assignment with DI, node managers, fluent builders, and generated instances.
- Updates samples, documentation, and tests for the new behavior.
File summaries
| File | Description |
|---|---|
tools/Opc.Ua.SourceGeneration.Core/Generators/NodeManagerTemplates.cs |
Removes redundant factory wiring. |
tools/Opc.Ua.SourceGeneration.Core/Generators/FluentBuilderGenerator.cs |
Exposes the default namespace. |
tests/Opc.Ua.WotCon.Samples.Tests/WotPumpAddressSpaceComparisonTests.cs |
Resolves pumps by browsing. |
tests/Opc.Ua.Vision.Tests/VisionNodeManagerIntegrationTests.cs |
Updates factory behavior tests. |
tests/Opc.Ua.Server.Tests/Hosting/FluentNodeManagerFactoryCoverageTests.cs |
Tests DI factory registration. |
tests/Opc.Ua.Server.Tests/Fluent/TypedBuilderTests.cs |
Uses a factory-backed manager. |
tests/Opc.Ua.Server.Tests/Fluent/SupervisionBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/StateMachineBuilderExtensionsTests.cs |
Updates minted-ID assertions. |
tests/Opc.Ua.Server.Tests/Fluent/SimulationBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/RuntimeValueBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/ReferenceBuilderExtensionsTests.cs |
Tests factory naming and validation. |
tests/Opc.Ua.Server.Tests/Fluent/PublishTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/PropertyInitBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/NodeManagerBuilderTests.cs |
Uses factory-backed builders. |
tests/Opc.Ua.Server.Tests/Fluent/NodeManagerBuilderAuthoringTests.cs |
Adds rebasing and namespace-validation coverage. |
tests/Opc.Ua.Server.Tests/Fluent/InstanceCreationBuilderExtensionsTests.cs |
Updates instance NodeId expectations. |
tests/Opc.Ua.Server.Tests/Fluent/GeneratedManagerHybridTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/FluentTestNodeManager.cs |
Adds a factory-backed test helper. |
tests/Opc.Ua.Server.Tests/Fluent/FluentNodeRegistrationTests.cs |
Locates properties by browse name. Moderate: A non-null return contract can return null; fail the test when no property exists. |
tests/Opc.Ua.Server.Tests/Fluent/EngineeringUnitsBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/Fluent/AlarmBuilderExtensionsTests.cs |
Updates alarm NodeId assertions. |
tests/Opc.Ua.Server.Tests/Fluent/AccessLevelBuilderExtensionsTests.cs |
Uses the shared test manager. |
tests/Opc.Ua.Server.Tests/DefaultNodeIdFactoryTests.cs |
Adds factory tests. Nit: Fix nullable initialization and sentinel warnings (CS8618 and CS8600). |
tests/Opc.Ua.Server.Tests/AsyncCustomNodeManagerTests.cs |
Tests manager factory integration. |
tests/Opc.Ua.Server.TestFramework/ServerFixture.cs |
Enables collision detection in fixtures. |
tests/Opc.Ua.Robotics.Tests/IntentHostingCoverageTests.cs |
Updates fallback-ID expectations. |
tests/Opc.Ua.Di.Tests/TopologyElementBuilderTests.cs |
Resolves descendants by browse path. |
tests/Opc.Ua.Di.Tests/PumpTypeInstanceTests.cs |
Uses manager-reported pump IDs. |
tests/Opc.Ua.Di.Tests/PumpInstanceNodeIdRegressionTests.cs |
Validates factory-derived pump IDs. |
tests/Opc.Ua.Di.Tests/PumpHostedReferenceTests.cs |
Resolves hosted nodes by browsing. |
tests/Opc.Ua.Di.Tests/PumpDatasheetConformanceTests.cs |
Uses manager-reported pump IDs. |
tests/Opc.Ua.Di.Tests/PumpAddressSpaceComplianceTests.cs |
Derives expected IDs through the factory. |
src/Opc.Ua.WotCon.Server/WotConnectivityNodeManager.cs |
Selects counter assignment for assets. |
src/Opc.Ua.Vision.Server/VisionNodeManager.cs |
Rebases assignment to its instance namespace. |
src/Opc.Ua.Types/State/NodeInstanceExtensions.cs |
Adds generated-type instance creation. |
src/Opc.Ua.Server/Server/StandardServer.cs |
Exposes server-wide factory settings. |
src/Opc.Ua.Server/Server/ServerInternalData.cs |
Supplies factory configuration to managers. |
src/Opc.Ua.Server/NodeManager/IRebasableNodeIdFactory.cs |
Defines the configurable factory contract. |
src/Opc.Ua.Server/NodeManager/INodeManager.cs |
Extends the async manager contract. |
src/Opc.Ua.Server/NodeManager/INodeIdFactoryProvider.cs |
Defines server-side factory provisioning. |
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs |
Implements canonical NodeId assignment. Nit: Public documentation incorrectly says children inherit their parent’s namespace. |
src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs |
Integrates factory assignment and rebasing. Moderate: Numeric namespace-0 IDs are treated as standard declarations without verifying that they are defined standard IDs. |
src/Opc.Ua.Server/NodeManager/Adapters/AsyncNodeManagerAdapter.cs |
Adapts the new interface members. |
src/Opc.Ua.Server/Hosting/OpcUaServerBuilderExtensions.cs |
Adds DI configuration extensions. |
src/Opc.Ua.Server/Hosting/NodeIdCollisionDetection.cs |
Represents collision-detection configuration. |
src/Opc.Ua.Server/Hosting/DependencyInjectionStandardServer.cs |
Resolves factory settings from DI. |
src/Opc.Ua.Server/Fluent/StateMachineBuilderExtensions.cs |
Mints state-machine IDs centrally. |
src/Opc.Ua.Server/Fluent/ReferenceBuilderExtensions.cs |
Mints object IDs centrally. Moderate: Valid standard browse names in namespace 0 are incorrectly rejected. |
src/Opc.Ua.Server/Fluent/PropertyInitBuilderExtensions.cs |
Mints property IDs centrally. |
src/Opc.Ua.Server/Fluent/NodeManagerBuilder.cs |
Exposes the builder namespace. |
src/Opc.Ua.Server/Fluent/InstanceCreationBuilderExtensions.cs |
Mints instance IDs centrally. |
src/Opc.Ua.Server/Fluent/INodeManagerBuilder.cs |
Adds the default namespace contract. |
src/Opc.Ua.Server/Fluent/FluentNodeRegistration.cs |
Centralizes minting and registration. |
src/Opc.Ua.Server/Fluent/FluentNodeManagerBuilderExtensions.cs |
Adds fluent assignment-mode selection. |
src/Opc.Ua.Server/Fluent/AlarmBuilderExtensions.cs |
Mints alarm IDs centrally. |
src/Opc.Ua.Server/FileSystem/FileSystemNodeManager.cs |
Removes redundant context assignment. |
src/Opc.Ua.Server/Diagnostics/DiagnosticsNodeManager.cs |
Uses counter assignment for diagnostics. |
src/Opc.Ua.Server/AliasNames/AliasNameNodeManager.cs |
Uses counter assignment for aliases. |
src/Opc.Ua.Robotics.Server/RobotIntentNodeManager.cs |
Uses its instance namespace. |
src/Opc.Ua.Positioning.Server/PositioningNodeManager.cs |
Removes concatenated ID generation. |
src/Opc.Ua.ISA95.Server/Isa95NodeManager.cs |
Removes custom ID generation. |
src/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs |
Uses counter assignment for GDS nodes. |
src/Opc.Ua.Di.Server/DiNodeManager.cs |
Uses the DI instance namespace. |
src/Opc.Ua.AI.Server/AiNodeManager.Transfer.cs |
Creates rebased transfer instances. |
src/Opc.Ua.AI.Server/AiNodeManager.Learning.cs |
Creates a rebased learning job. |
src/Opc.Ua.AI.Server/AiNodeManager.Jobs.cs |
Creates rebased inference jobs. |
src/Opc.Ua.AI.Server/AiNodeManager.cs |
Uses counter assignment and shared creation. |
src/Opc.Ua.AI.Server/AiNodeManager.Catalogue.cs |
Creates a rebased catalogue source. |
src/Opc.Ua.AI.Server/AiNodeManager.AddressSpace.cs |
Rebases model and deployment nodes. |
samples/Quickstarts.Servers/TestData/TestDataNodeManager.cs |
Uses counter assignment for test data. |
samples/Quickstarts.Servers/SampleNodeManager/SampleNodeManager.cs |
Adds factory-backed assignment. Moderate: The factory remains targeted at namespace 0 for MemoryBufferNodeManager, causing generated children to fall outside the manager’s namespaces. |
samples/Quickstarts.Servers/ReferenceServer/ReferenceNodeManager.cs |
Removes concatenated ID generation. |
samples/Quickstarts.Servers/Boiler/BoilerNodeManager.cs |
Uses counter assignment for boilers. |
samples/Quickstarts.Servers/Alarms/AlarmNodeManager.cs |
Removes concatenated ID generation. |
samples/OpenUsd/SiteCompositionServer/SiteNodeManager.cs |
Removes custom assignment logic. |
samples/OpenUsd/GeneratorServer/GeneratorNodeManager.cs |
Uses the shared factory policy. |
samples/DI/PumpDeviceIntegrationServer/PumpNodeManager.cs |
Uses factory-derived pump IDs. |
docs/WhatsNewIn2.0.md |
Links the NodeId assignment guide. |
docs/NodeIdAssignment.md |
Documents the assignment mechanism. Nit: Still describes the removed concatenation fallback. |
Review details
Suppressed comments (9)
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:173
- Validate
modebefore storing it. An undefined enum value currently bypasses all named cases, reaches the switch default, and mints opaque identifiers whileModereports the invalid value; collision detection is also disabled. Rejecting undefined values makes the public API deterministic.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:303 WithModecopies the effectiveDetectsCollisionsvalue rather than the requested collision policy. For example,new DefaultNodeIdFactory(String, detectCollisions: true).WithMode(Numeric)disables detection because String reportsfalse; switching Numeric → String → Numeric has the same problem. Preserve the requested policy separately and recompute the effective value for the new mode.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:179- The collision table starts empty and only records identifiers minted by this factory. In Numeric mode, a path hash can equal an authored numeric NodeId already loaded in the same namespace; this check will accept it, and
IndexPredefinedNodelater usesAddOrUpdate, silently replacing the authored node. Seed/reserve authored identifiers or reject an already-indexed NodeId during registration before replacement.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:267 - Rebasing always constructs a fresh factory, so two NodeManagers resolving the same DI singleton into the same namespace receive independent counters and collision tables. Counter IDs can then overlap, and cross-manager hash collisions are not detected, contrary to the documented shared-per-namespace behavior. Rebased views for the same namespace need shared per-namespace state.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:168 - The PR description says
Stringis the default assignment mode, but this public constructor and the new tests makeNumericthe default. This changes externally visible NodeId type and collision characteristics, so the implementation and stated contract need to agree before release.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:518 - Counter mode ignores the method's
namespaceIndexargument and returnsNextCounterNodeId()inDefaultNamespaceIndex. Calling this public API with an explicit target namespace therefore violates its contract and behaves differently from every other mode. Either mint the counter in the supplied namespace with correctly scoped state, or reject a namespace that differs from the factory default.
src/Opc.Ua.Server/NodeManager/DefaultNodeIdFactory.cs:439 IncrementIdentifierwraps fromuint.MaxValueto 1, so after enough allocations this counter enters the authored-ID range thatkCounterBaseis intended to exclude. Because Counter mode keeps no collision record, it can then silently reuse a model NodeId. Enforce the lower bound on wrap rather than only seeding above it.
tests/Opc.Ua.Server.Tests/DefaultNodeIdFactoryTests.cs:53- These non-nullable fields are only initialized by
[SetUp], which nullable flow analysis does not recognize as constructor initialization. With nullable enabled for this test project, both declarations produce CS8618 warnings (and warnings are treated as errors). Initialize them with the standard null-forgiving test-fixture pattern.
tests/Opc.Ua.Server.Tests/DefaultNodeIdFactoryTests.cs:579 - Assigning
nullto this non-nullable local produces CS8600 under the project's nullable settings. Mark the intentional pre-search sentinel with the null-forgiving operator (or make the local nullable and update the later dereferences).
- Files reviewed: 79/79 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review feedback on #4433. SampleNodeManager.New used to return the NodeId it was given. Routing it through a factory gave it one whose namespace was still 0, and nothing rebases a manager that does not derive from AsyncCustomNodeManager. So MemoryBufferNodeManager, which creates its buffers with assignNodeIds: true, minted their children into the OPC UA namespace - a namespace no NodeManager owns, which is the failure the namespace rule exists to prevent. The sample base now adopts the first namespace it owns when the factory still names none, and MemoryBufferNodeManager rebases onto its instance namespace the way BoilerNodeManager already did. The constructor documented the opposite of the rule the factory follows, promising that a child inherits its parent's namespace. It never has: a parent's namespace can belong to a companion specification whose NodeIds are fixed by its NodeSet, or to another NodeManager. Also drops a paragraph promising a concatenated identifier shape for builders on other NodeManagers, which no longer exists, and fails a test helper outright instead of returning a null it declares it cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
marked this pull request as ready for review
September 7, 2026 14:02
Deleting the New() override in five sample NodeManagers left its XML doc comment behind, attached to whatever method followed. AlarmNodeManager and ReferenceNodeManager ended up with "Creates the NodeId for the specified node" documenting CreateAddressSpaceAsync and AllowNodeManagement; PumpNodeManager, GeneratorNodeManager and SiteNodeManager with a stray <inheritdoc/> on a method that overrides nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
reviewed
Sep 7, 2026
Eight findings from marcschier's deep review of #4433. [P1] AddNodes minted from the node alone. The instance is not linked to its parent until after the duplicate browse-name check, so two objects added under different parents with the same browse name hashed to one canonical path and got one NodeId; the second registration replaced the first, both returning Good. The parent identity is now passed explicitly, through a protected AllocateNodeIdForAddNodes a NodeManager with its own scheme can override. [P2] Every factory view rebuilt its allocation state. Two NodeManagers owning one namespace each rebase the registered factory, so they got independent counters seeded from the clock moments apart - overlapping sequences, not merely separate ones - and could not see each other's minted identifiers. State is now held per namespace and shared by every view derived from one factory, which is what NodeIdAssignment.md already promised. Direct construction still starts its own, which the document now says. The configured collision policy is also kept apart from whether the current mode can collide, so WithMode(String).WithMode(Numeric) comes back watching. [P2] A generated state object is born holding its own type's NodeId, and the factory keeps an identifier on a node that stands on its own. The removed New overrides used to overwrite it. BoilerNodeManager therefore left each boiler on BoilerType, so the second boiler replaced the ObjectType in the index, and the alias fallback left every alias on AliasNameType. Boiler now goes through context.CreateInstance; the alias clears the declaration id before minting. A sweep of the other places that build a parentless generated instance found them all assigning a NodeId explicitly. [P3] The pump duplicate check predicted the identifier the factory would mint, which under Counter mode consumes a counter value and returns one no node can hold. It now looks the child up by browse name. [P3] MemoryBuffer never registered its buffers, and its handle lookup returned null for any string id that was not a virtual tag, so the buffers' own properties resolved to nothing. The subtree is registered and a non-tag id falls through to the base lookup. [P3] A canonical path is longer than the parent identifier it encodes, so String mode could publish an identifier over the 4096 characters OPC UA Part 3 8.2.4 allows. It is refused instead. [P3] A test wrapped NodeId in Nullable rather than using its own null sentinel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The document said NodeManagers sharing a namespace share the factory instance. What they share is its per-namespace allocation state, and only when they take the registered factory rather than constructing one of their own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
approved these changes
Sep 8, 2026
Two further review findings. [P2] AllocateNodeIdForAddNodes passed the NodeManager's first namespace rather than the one its factory mints into. A manager owning a model namespace and an instance namespace rebases the factory onto the latter, so AddNodes registered runtime instances in the model namespace and still returned Good. Fixing the parent identity replaced what New() derived and lost what it knew about the namespace; both now come from the factory. [P3] Counter allocation neither consulted nor recorded shared identifiers, because a Counter-mode view reports DetectsCollisions as false. It cannot collide with itself, but it mints into the same numeric space a hash does, so a namespace minting in both modes could issue one identifier twice and let registration replace a node. The reservation now follows the requested policy rather than the mode's effective flag: the question is not whether this mode collides with itself, but whether its identifiers stay out of the way of the modes that do. The counter therefore keeps a record when checking is asked for, and that record grows with the number of transient nodes rather than with the size of the address space - which is what Counter mode exists for. Checking is off by default outside a debug build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The arm64 macOS linker fails with "ld: Assertion failed: (_addend == uniqueIndex && "too many large addends")" when publishing the AOT test binary. The project already carries a size-focused publish for this exact assertion; the binary links most of the product and had been sitting just under the threshold, so a small addition tipped it over. The runner image is identical between the last passing run and the failing one, so this is the binary crossing a limit rather than a toolchain change. Replacing framework exception message strings with resource keys removes a large share of the read-only data, and the relocations against it, that the linker asserts on. Nothing here asserts framework exception text, and stack traces are already disabled in this configuration, so it gives up diagnostics this binary had already traded away. Stripping the native symbol table removes more of what the linker emits. This only applies to the CI publish on macOS. It changes no managed test surface and no product code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
enabled auto-merge (squash)
September 8, 2026 11:17
…igner # Conflicts: # src/Opc.Ua.Server/Hosting/DependencyInjectionStandardServer.cs # src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs # src/Opc.Ua.Server/Server/StandardServer.cs # tests/Opc.Ua.Server.Tests/AsyncCustomNodeManagerTests.cs
This reverts commit 3b14390.
Collaborator
|
@copilot resolve the merge conflicts in this pull request |
Contributor
Resolved the merge conflicts by merging |
…igner # Conflicts: # src/Opc.Ua.Server/Fluent/FluentNodeManagerBuilderExtensions.cs
romanett
added a commit
that referenced
this pull request
Sep 8, 2026
#4433 routes runtime NodeId minting through DefaultNodeIdFactory and drops SystemContext.NodeIdFactory = this from generated managers, so a manager now selects a mode instead of overriding New. It also edited the ApplicationsNodeManager this branch has rewritten, so git produced four conflict regions. Three are this branch's rewrite against nothing, or against the very overrides it removed -- DeleteAddressSpaceAsync and GetManagerHandleAsync, kept out rather than resurrected by a reflexive "keep both sides". The fourth carries real intent and is taken: the GDS mints Counter identifiers. Applications, certificate groups and trust lists are registered and unregistered under repeating names, and Counter is the only mode that stays unique when a browse path repeats; every other mode derives the identifier from the path and would collide across a remove and re-add. Master's companion NamespaceUris assignment is not taken. That is how the hand-written manager declared its namespaces; the generated one carries the same order through its constructor chain already. The namespace half of the NodeId contract still holds: NodeIdFactory rebases an assigned factory onto the manager's own namespace, so ids still land in the application record namespace, which the invariant test pins against a live server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
added a commit
that referenced
this pull request
Sep 8, 2026
#4433 moved root-notifier registration onto the node manager interface, which conflicted with the ownership record this branch keeps so alarm teardown removes only what it added. Master's call stays; the record stays with it. One decision the code could not settle: the membership probe is on the concrete manager while the registration call is now on the interface, so where the probe is unavailable we claim nothing — leaving a registration behind beats tearing down one that was never ours. The merge also surfaced a real ordering bug from the previous commit, caught by RuntimeNodeSetSimulationStartsAfterNodeAddedReplayAsync. Sealing is split so a manager can replay NotifyNodeAdded between the halves, and the replay is guaranteed to finish before any simulation ticks. Starting the loops from the behavior lease respected that — but activation also ran from CompleteConfigureAsync, which happens before the replay, so the loops started too early. Activation is now only driven by the seal. Every CompleteConfigureAsync caller — DI, ISA-95, FluentNodeManagerFactory, RuntimeNodeSet, SiteNodeManager — seals afterwards, so nothing goes unactivated, and seal is the later of the two points, so behaviors now always observe the replay-complete graph rather than sometimes preceding it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 tasks
romanett
added a commit
that referenced
this pull request
Sep 8, 2026
The manager reordered its own namespaces so that NamespaceIndexes[0] was the application-record one, because that is where the old CustomNodeManager2.New() minted. #4433 moved the minting namespace onto the factory, so the reordering and the two accessors that worked around it can go: - the constructor takes the generated namespace order (the companion model first) and points the factory at the application-record namespace instead, as DiNodeManager already does for its instance namespace; - GdsNamespaceIndex is gone, since the model namespace is the manager's own again; ApplicationsNamespaceIndex names the other one and is resolved through the namespace table rather than a fixed position; - the custom certificate group root goes in with a null NodeId again. It only carried a hand-built one because PrepareAuthoredNodeIdsForRegistration skipped the root, which #4433 fixed with HasStandardDeclarationNodeId. The two GDS node manager factories now advertise ApplicationsNodeManager.DefaultNamespaceUris() so their namespace list cannot drift from the manager's. The two namespaces swap index in the server's namespace table. Nothing in the stack keys off the numbers - the assertions resolve URIs and browse names - but a client that cached indexes across an upgrade sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
added a commit
that referenced
this pull request
Sep 8, 2026
The audit that prompted this found nothing dead - every fluent and generator addition on this branch still has a live consumer - but three places still told readers to reorder a manager's namespaces so that NamespaceIndexes[0] is where its ids are minted. #4433 moved that decision onto the NodeId factory, and this branch's own manager stopped doing it, so the guidance now teaches a workaround that its only example no longer follows. DefaultNamespaceUris() is emitted into every generated manager, so its summary is the widest-read of the three: it now says the order picks the namespace an unqualified browse path resolves in, and says the factory owns the minting namespace. The attribute's remarks and the NodeManagers.md section carry the same correction, the latter with a worked NodeIdFactory.WithDefaultNamespaceIndex call in place of the reordered array and a pointer to NodeIdAssignment.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
added a commit
that referenced
this pull request
Sep 9, 2026
# Description Follow-up to #4390 for distributed address-space ownership, authoritative hydration, and replica-consistent wire NodeIds. Tracks #4444. Identity implementation: `a78d59067` (Server policy/lifecycle/adapter seam) and `07faf5cfd` (replica identity module, runtime wiring, examples and proofs). ## Replica identity - `UseReplicaNodeIdentity` / `ReplicaNodeIdFactory` reserve fixed shared namespace indexes before node-manager construction and reuse `DefaultNodeIdFactory`. ApplicationUri and built-in diagnostics/configuration remain local. - Replicated startup requires an explicit identity contract. Namespace layout, deterministic mode, replica-set identity and writer policy must agree. Standard factory views, replacement and registration preserve the policy. - Active/passive writer-assigned identities are replicated unchanged. Independent creation requires stable keys or explicit IDs. Live IDs and retained tombstones are reserved before writer allocation; no network/storage call runs in `New`. - Protected store descriptors use strong routing, including customized hybrid prefixes. Unknown legacy state and empty eventual scans cannot authorize automatic adoption. Verified-new hybrid provisioning is explicit. - Active/active frames compare the descriptor before CRDT merge. Rejected frames are not reported as applied; incompatible admission remains at ServiceLevel 0 through the standard service-level wiring. - Shared metadata validation covers type definitions, data types, references, permissions, NodeId/ExpandedNodeId/QualifiedName values, arrays/matrices, DataValues and encodeable structures/arguments, without reflection. - Runtime preparation hydrates retained IDs before replacement routes publish. Rebinding keeps the existing store/election or gossip transport/map and attaches to the new node-manager sources. Normal node-manager mutations, including AddNodes, reach the cached local-address-space adapter. - WoT materialization retains the stable configured factory. Samples expose genuinely factory-created nodes and a client `--identity` failover workflow. ## Preserved HA repairs The published `d99995003` and `79da74f14` fixes remain in place: - Shared linearizable sequence/lease coordination; no pretend CAS on CRDT and no private counter substituted for shared coordination. - Strict authoritative record decoding, protected pending reservations, safe snapshot publication and required delta retention. - Accepted topology state controls authoritative membership; stale deletion cannot remove a newer incarnation. - Independent subtree attachment tracking and correct disposal of the final snapshot task. - Values wait for their topology, and parent tombstone horizons prevent older descendant values crossing incarnations. A new deterministic regression also covers writer hydration preparation: temporary reconciliation removals must remain inbound application, not be recaptured as new shared deletion records. ## Validation ### Review feedback fixes `3a682155b` addresses the active-node notification, WoT namespace-index bounds, and retired-manager XML documentation feedback. The address-space adapter now notifies listeners with the registered active node, and both WoT managers reject missing/out-of-range namespace indexes before ownership is established. Targeted feedback validation passes on net10.0 and net48: 8 address-space adapter cases and 74 WoT cases per framework, plus 17 hydration/gossip/live-client integration cases on net10.0. The wrong-instance notification and missing namespace regressions were reproduced before the fixes. ### Identity implementation evidence | Selected scope | net10.0 | net48 | | --- | --- | --- | | HA repair, identity, metadata, peer admission and real-client identity/reload fixtures | 215 passed | 215 passed | | Factory, fluent authoring/import, NodeManagement and address-space adapter fixtures | 213 passed | 213 passed | | WoT node-manager fixtures | 68 passed | 68 passed | - Real UA-TCP clients compare full NodeIds/reference targets across independent servers in all four deterministic modes, with different manager/creation order and local allocations. Saved IDs work after endpoint failover and restart. - Real AddNodes-created IDs remain readable through successive live manager replacements in both replication modes, including nodes created after a reload. - Two process-level cases start three sample replicas, kill the serving process, verify a different serving ServerArray URI, and reuse cached factory-generated IDs for Read and CreateMonitoredItems without remapping or rebrowsing. - The identity/hydration scenario runs from the actual published win-x64 NativeAOT executable in the existing historian companion host. - The focused writer-recapture regression was observed failing without its fix, then passing. The real reload scenario was additionally repeated while diagnosing that race. - Changed-file formatting/style/analyzers were applied. Existing CA1508, CA1823, CA1861 and CA1850 warnings in untouched upstream files were not suppressed. The selected checks above do not replace full-solution CI, coverage gates or maintainer review, and do not claim that the entire solution or every native platform was run. #4444 remains the tracking issue. ## Contract and consistency limits Fixed identity does not make eventual payload reads linearizable. Hybrid views remain non-destructive, use explicit tombstones, and do not infer empty-store bootstrap or partition completion from absence. Compacted snapshots require authoritative storage. Failed/uncertain reservations require reconciliation. There is no automatic namespace-slot allocation, persistent logical-key mapping database, live renumbering, legacy-store wipe/adoption, or new hash algorithm. Opaque shared structures require a registered codec so their namespace-bearing fields can be validated. Application callbacks are still application-owned. ## Related issues - Tracks #4444. - Follow-up to #4390. - Builds on the merged NodeId factory (#4433), configure/seal (#4442) and lifecycle/import (#4427) integration at `aedd3f738`. ## Checklist - [ ] I have signed the CLA and read CONTRIBUTING. - [x] Added targeted regression and integration tests. - [x] Updated directly related API, HA, migration and sample documentation. - [x] Ran scoped formatting and the targeted framework checks listed above. - [ ] Full UA.slnx matrix and coverage gates completed. - [ ] All CI and maintainer review completed. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9dbd9527-d818-47a0-984c-fbb1472d3d6b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Nodes created at runtime now get their NodeIds from a single
DefaultNodeIdFactoryrather than from aNewoverride per NodeManager.The factory derives a deterministic identifier from a canonical,
length-prefixed browse path, which is injective where the classic
{parent}_{child}concatenation was not —A_B+CandA+B_Cboth produced
A_B_C— and it also records the parent's identifier typeand qualifies cross-namespace segments by URI, so identifiers no longer
shift with namespace-table ordering.
A NodeManager selects its style declaratively instead of writing
assignment code:
NodeIdAssignmentModeoffersString(the canonical path verbatim, thedefault),
Numeric/Guid/Opaque(that path through SHA-256),Counter(sequential, for nodes whose browse paths repeat — per-sessiondiagnostics, inference jobs, rediscovered assets) and
None. The counterstarts above
0x40000000, out of reach of any authored model, so along-running server cannot walk a runtime instance onto a type node.
Newoverrides are gone from every NodeManager the stack ships excepttwo whose identifiers are a domain scheme rather than an assignment
policy:
FileSystemNodeManager, whose NodeIds encode the file path theyresolve back to, and
RoboticsNodeManager, whose build coordinatorreserves identifiers across managers with ownership tracking.
Namespace rule. Every identifier is minted into the NodeManager's own
namespace. Neither the parent's namespace nor the browse name's is
consulted: a parent can belong to a companion-specification model whose
NodeIds are fixed by its NodeSet (
DeviceSetin the DI namespace), and abrowse name only names the type that declared the child. NodeManagers
whose instance namespace is not their first one rebase with
WithDefaultNamespaceIndex.Types vs instances. An authored NodeId on a node that stands on its
own is kept. A node hanging off a parent is re-minted, because it reached
the factory through
AssignNodeIdswalking a subtree copied from a typedeclaration. Keeping those would alias every instance onto the type's own
nodes, and the predefined-node index takes the last writer, so the type
would quietly become an instance rather than the clash being reported.
New
ISystemContext.CreateInstance(...)builds an instance of a generatedtype and rebases its subtree through
AssignInstanceNodeId— the path thegenerated
CreateInstanceOf<Type>helpers already took. The AINodeManager was the one place calling
NodeState.Createdirectly, so itsroots kept the NodeId their state object is born with (the type's own);
it now uses the shared path.
Breaking changes
IAsyncNodeManagerextendsINodeIdFactoryand gainsAddNodeandAddRootNotifier. This lets the fluent surface mint and registerthrough the interface instead of type-testing for
AsyncCustomNodeManagerand silently falling back to a concatenatedidentifier when the test failed — that fallback was itself a source of
divergent NodeIds.
AsyncNodeManagerAdapterdelegatesNewto the wrapped NodeManager, soCustomNodeManager2behaves exactly as before: it mints nothing anda node keeps whatever NodeId it already has. Servers built on it are
unaffected.
Notes for reviewers
docs/NodeIdAssignment.mdis new and describes the whole mechanism: thefactory contract, identifier formats, type declarations vs instances,
the generated helpers and their parameters, per-NodeManager behaviour,
and an inventory of every member involved.
recorded in the doc:
assignInstanceNodeIds(it suppresses minting,where the rebase rule forces it) and the generator's
NodeId.Equals(TypeNodeIdConstant)guard (it distinguishes acaller-assigned NodeId from a declaration's).
fallback; they now run against a real
DefaultNodeIdFactorythrough ashared
FluentTestNodeManagerhelper.Related Issues
No tracking issue yet.
Checklist
🤖 Generated with Claude Code