dohertj2/lmxopcua
1
0
Fork 0
Code Issues 43 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(opcua): no write ever creates a node; cross-kind id guard (B1)

Browse Source 
Review 01 L1/L4/H2. WriteValue/WriteValues are non-creating (an unknown
id is skipped, counted and Debug-logged, as WriteStatuses always was), so
a driver publish racing a surgical remove can no longer resurrect the id
as a flat BaseDataType phantom under the shared root — nor hand a later
re-add of the same tag the phantom's parent/DataType/writability through
EnsureVariable's early return. WriteAlarmCondition for an unmaterialised
id is a logged no-op instead of minting the legacy bool[2] placeholder
(AddPredefinedNode is AddOrUpdate, so it replaced the SDK lookup entry of
any folder sharing the id). The applier no longer writes a condition
state against every removed equipment id (a folder, never a condition).
EnsureFolder/EnsureVariable/MaterialiseAlarmCondition refuse an id already
held by another node kind in the same realm. CreateVariable is deleted.

Tests: NodeManagerPhantomNodeTests (publish racing a remove leaves no
node; the re-add gets the authored shape and is live; unknown alarm id
is a no-op; the guard in all three directions, realm-qualified); the
sink / batch-write / write-statuses tests now declare their variables.

Refs #732

Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 06:41:09 -04:00
parent d8c39933ed
commit 00300d0f60
7 changed files with 396 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs
+4 -6
View File
@@ -93,15 +93,13 @@ public sealed class AddressSpaceApplier
}
var ts = DateTime.UtcNow;
var removedCount = 0;
// A removed equipment is a FOLDER: it gets no terminal condition write (review 01 L4 — the write the
// applier used to make against the equipment id never found a condition and minted a placeholder
// variable that shadowed the folder's SDK lookup entry). Its subtree teardown is in ApplyPureRemove.
var removedCount = plan.RemovedEquipment.Count;
// Swallowed removal-pass condition-write failures are tallied here (not lost to a per-node Warning)
// so a degraded removal is operator-visible via AddressSpaceApplyOutcome.FailedNodes (archreview 01/S-1).
var failedNodes = 0;
foreach (var eq in plan.RemovedEquipment)
{
if (!SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, ts, AddressSpaceRealm.Uns)) failedNodes++;
removedCount++;
}
foreach (var alarm in plan.RemovedAlarms)
{
if (!SafeWriteAlarmCondition(alarm.ScriptedAlarmId, RemovedConditionState, ts, AddressSpaceRealm.Uns)) failedNodes++;
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+86 -72
View File
@@ -18,18 +18,19 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer;
/// <summary>
/// Custom OPC UA <see cref="CustomNodeManager2"/> that owns the writable address space for
/// the OtOpcUa server. Variable nodes are created lazily on first <see cref="WriteValue"/>
/// under the manager's namespace; subsequent writes update the existing node's Value +
/// StatusCode + SourceTimestamp and notify subscribed clients via the standard
/// <c>ClearChangeMasks</c> path.
/// the OtOpcUa server. Variable nodes are declared by <see cref="EnsureVariable"/> (the applier's
/// materialise passes); a <see cref="WriteValue"/> updates an existing node's Value + StatusCode +
/// SourceTimestamp and notifies subscribed clients via the standard <c>ClearChangeMasks</c> path.
/// <b>No write ever creates a node</b> (review 01 L1): a publish naming an id a surgical remove has just
/// dropped is skipped, so it can neither resurrect the node as a flat phantom under the shared root nor
/// hand a later re-add of the same id the phantom's shape.
///
/// This is the F10b production wiring behind the v2 <see cref="IOpcUaAddressSpaceSink"/>
/// seam — once a <see cref="SdkAddressSpaceSink"/> is bound, OpcUaPublishActor's writes
/// materialise as real OPC UA Variable updates that clients can browse + subscribe to.
///
/// Node-id encoding uses the manager's default namespace + the caller-supplied string id
/// as the identifier portion (e.g. <c>"ns=2;s=eq-1/temp"</c>). Beyond lazily-created flat
/// variables, the manager also materialises the full UNS Area/Line/Equipment folder hierarchy
/// as the identifier portion (e.g. <c>"ns=2;s=eq-1/temp"</c>). The manager materialises the full UNS Area/Line/Equipment folder hierarchy
/// (<see cref="EnsureFolder"/>), typed equipment-tag variables with the correct built-in
/// <c>DataType</c> / array shape / access levels (<see cref="EnsureVariable"/>), historized
/// nodes (the <c>HistoryRead</c> access bit + HistoryRead overrides), and real Part 9
@@ -705,9 +706,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
/// <summary>
/// Apply a value write from <see cref="IOpcUaAddressSpaceSink.WriteValue"/>. Creates the
/// variable node on first call; subsequent calls update Value + StatusCode +
/// SourceTimestamp and call <c>ClearChangeMasks</c> so subscribed clients see the change.
/// Apply a value write from <see cref="IOpcUaAddressSpaceSink.WriteValue"/>: update the materialised
/// variable's Value + StatusCode + SourceTimestamp and call <c>ClearChangeMasks</c> so subscribed clients
/// see the change. <b>Never creates a node</b> — an id nothing declared via <see cref="EnsureVariable"/>
/// (a publish racing a surgical remove) is skipped and Debug-logged, exactly as <see cref="WriteStatuses"/>
/// always did (review 01 L1).
/// </summary>
/// <param name="nodeId">The node identifier of the variable.</param>
/// <param name="value">The new value to write.</param>
@@ -720,10 +723,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
ArgumentException.ThrowIfNullOrEmpty(nodeId);
EnsureAddressSpaceCreated();
var ns = NamespaceIndexForRealm(realm);
var key = MapKey(realm, nodeId);
bool applied;
lock (Lock)
WriteValueLocked(nodeId, ns, key, value, statusCode, sourceTimestampUtc);
applied = WriteValueLocked(key, value, statusCode, sourceTimestampUtc);
if (!applied)
{
_logger.LogDebug(
"OtOpcUaNodeManager: WriteValue skipped {NodeId} (realm {Realm}) — node not materialised.",
nodeId, realm);
}
}
/// <summary>The most items <see cref="WriteValues"/> applies under a SINGLE <c>Lock</c> acquisition before
@@ -741,7 +751,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <summary>Batched <see cref="WriteValue"/>: one <c>Lock</c> acquisition per chunk of
/// <see cref="MaxWritesPerLockAcquisition"/> items instead of one per value — the publish path's per-value
/// lock take was the steady-state throughput ceiling. Per-item semantics are identical to
/// <see cref="WriteValue"/> (create-on-first-write verbatim status codes, ClearChangeMasks per node).
/// <see cref="WriteValue"/> (never creates; verbatim status codes; ClearChangeMasks per node) — an item naming a
/// node that is not materialised is skipped and counted, not failed (the caller's per-driver ref index can
/// legitimately name a node a surgical remove has just dropped).
/// <para><b>Items are isolated.</b> A bad item (empty NodeId, or a throw out of the SDK) does not abandon
/// the rest of the batch: it is recorded and the loop continues, and the aggregate surfaces afterwards as
/// a <see cref="ValueWriteBatchException"/>. Dropping the tail would be silent and durable — the poll
@@ -785,6 +797,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
var succeeded = 0;
var skipped = 0;
for (var start = 0; start < values.Count; start += MaxWritesPerLockAcquisition)
{
var end = Math.Min(start + MaxWritesPerLockAcquisition, values.Count);
@@ -796,9 +809,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
var v = values[i];
try
{
WriteValueLocked(resolved[i].NodeId, resolved[i].NamespaceIndex, resolved[i].Key,
v.Value, v.StatusCode, v.SourceTimestampUtc);
succeeded++;
if (WriteValueLocked(resolved[i].Key, v.Value, v.StatusCode, v.SourceTimestampUtc)) succeeded++;
else skipped++;
}
catch (Exception ex)
{
@@ -810,6 +822,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
}
if (skipped > 0)
{
// Debug, not Warning: the caller's index racing a surgical remove is expected, and a driver
// whose nodes were all just removed would otherwise log once per ref.
_logger.LogDebug(
"OtOpcUaNodeManager: WriteValues skipped {Skipped} of {Total} item(s) — node not materialised.",
skipped,
values.Count);
}
if (failed > 0)
{
throw new ValueWriteBatchException(
@@ -925,24 +947,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <param name="IsValid">False when the item failed the pre-pass and must be skipped.</param>
private readonly record struct ResolvedWrite(string NodeId, ushort NamespaceIndex, RealmKey Key, bool IsValid);
/// <summary>The body of a single value write. <b>Caller MUST hold <c>Lock</c></b> — this is factored out
/// solely so <see cref="WriteValues"/> can apply a whole batch under one acquisition; it is byte-identical
/// to what <see cref="WriteValue"/> has always done inside its own <c>lock</c>.</summary>
/// <param name="nodeId">The bare <c>s=</c> identifier of the variable.</param>
/// <param name="ns">The namespace index resolved from the write's realm.</param>
/// <param name="key">The realm-qualified map key for <paramref name="nodeId"/>.</param>
/// <summary>The body of a single value write. <b>Caller MUST hold <c>Lock</c></b> — factored out so
/// <see cref="WriteValues"/> can apply a whole batch under one acquisition.
/// <para><b>Non-creating</b> (review 01 L1). The former create-on-first-write branch minted a flat, read-only,
/// <c>BaseDataType</c> variable under the shared root whenever a publish raced a surgical remove; it stayed
/// until the next full rebuild, and because <see cref="EnsureVariable"/> early-returns on an existing id, a
/// later re-add of the same tag silently kept the phantom's parent, DataType and writability.</para></summary>
/// <param name="key">The realm-qualified map key of the variable.</param>
/// <param name="value">The new value to write.</param>
/// <param name="statusCode">The full OPC UA status code, sub-code included.</param>
/// <param name="sourceTimestampUtc">The timestamp of the value in UTC.</param>
private void WriteValueLocked(string nodeId, ushort ns, RealmKey key, object? value, uint statusCode, DateTime sourceTimestampUtc)
/// <returns><see langword="true"/> when the node existed and was updated; <see langword="false"/> when no
/// such node is materialised (skipped — deliberately not a create).</returns>
private bool WriteValueLocked(RealmKey key, object? value, uint statusCode, DateTime sourceTimestampUtc)
{
// CreateVariable mutates the SDK address space (_root.AddChild + AddPredefinedNode),
// so it MUST run under Lock — the SDK's subscription/ConditionRefresh threads take it too.
if (!_variables.TryGetValue(key, out var variable))
{
variable = CreateVariable(nodeId, ns);
_variables[key] = variable;
}
if (!_variables.TryGetValue(key, out var variable)) return false;
variable.Value = value;
// The caller's status code reaches the SDK verbatim, sub-code included. This used to
@@ -952,6 +971,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
variable.Timestamp = sourceTimestampUtc;
// Stays INSIDE the lock, per node: this is how the SDK notifies monitored items of the change.
variable.ClearChangeMasks(SystemContext, includeChildren: false);
return true;
}
/// <summary>
@@ -959,10 +979,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// been materialised for <paramref name="alarmNodeId"/> (via <see cref="MaterialiseAlarmCondition"/>),
/// this projects the whole <paramref name="state"/> snapshot
/// (Enabled / Active / Acked / Confirmed / Shelving / Severity / Message) onto the live condition
/// node and recomputes Retain (T15 — richer state; <b>still no event firing</b>, that lands in T16).
/// Otherwise it falls back to the legacy two-element <c>[Active, Acknowledged]</c>
/// <see cref="BaseDataVariableState"/> placeholder so callers whose alarm node hasn't been
/// materialised (and the existing unit tests) keep working.
/// node, recomputes Retain and fires one condition event when the state actually moved.
/// An id with no materialised condition is a logged no-op (review 01 L4) — it used to mint a legacy
/// <c>bool[2]</c> placeholder variable whose <c>AddPredefinedNode</c> (an AddOrUpdate) REPLACED the SDK
/// lookup entry of any folder sharing the id, which the applier's per-removed-equipment write did on
/// every removal.
/// </summary>
/// <param name="alarmNodeId">The node identifier of the alarm (== ScriptedAlarmId for materialised conditions).</param>
/// <param name="state">The full condition state to project onto the node.</param>
@@ -974,13 +995,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
ArgumentNullException.ThrowIfNull(state);
EnsureAddressSpaceCreated();
var ns = NamespaceIndexForRealm(realm);
var key = MapKey(realm, alarmNodeId);
// Look up + project under a SINGLE Lock so a concurrent RebuildAddressSpace can't clear
// _alarmConditions / detach the condition node between the lookup and the Set* calls.
lock (Lock)
{
if (_alarmConditions.TryGetValue(key, out var condition))
if (!_alarmConditions.TryGetValue(key, out var condition))
{
// Unknown / unmaterialised condition ⇒ safe no-op (a mid-rebuild race, or a caller naming a
// node that is not a condition), mirroring WriteAlarmQuality.
_logger.LogDebug(
"OtOpcUaNodeManager: WriteAlarmCondition skipped {NodeId} (realm {Realm}) — condition not materialised.",
alarmNodeId, realm);
return;
}
{
// T20 delta-gate: read the node's CURRENT live condition state FIRST (before projecting
// the incoming snapshot onto it), then decide fire-vs-suppress by comparing the incoming
@@ -1053,22 +1082,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
{
ReportConditionEvent(condition, sourceTimestampUtc);
}
return;
}
// Fallback: alarm not materialised as a real condition — keep the legacy bool[2] variable so
// un-materialised callers (and the existing unit tests) keep working. CreateVariable mutates
// the SDK address space, so it MUST run under Lock (see WriteValue).
if (!_variables.TryGetValue(key, out var variable))
{
variable = CreateVariable(alarmNodeId, ns);
_variables[key] = variable;
}
variable.Value = new[] { state.Active, state.Acknowledged };
variable.StatusCode = StatusCodes.Good;
variable.Timestamp = sourceTimestampUtc;
variable.ClearChangeMasks(SystemContext, includeChildren: false);
}
}
@@ -1364,6 +1378,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// stays ONLY for the kind-swap (native↔scripted) case, which flips the native flag.
if (_alarmConditions.ContainsKey(conditionKey) && _nativeAlarmNodeIds.Contains(conditionKey) == isNative)
return;
// Review 01 H2 — a variable or folder already holding this id in this realm wins; never shadow it.
if (!_alarmConditions.ContainsKey(conditionKey) && IsHeldByAnotherKind(conditionKey, "alarm condition"))
return;
// Idempotent: drop any prior node for this id so a re-materialise (e.g. changed
// type/severity on redeploy) reflects cleanly instead of leaking the old node.
@@ -2886,6 +2903,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
lock (Lock)
{
if (_folders.ContainsKey(key)) return;
if (IsHeldByAnotherKind(key, "folder")) return;
var parent = ResolveParentFolder(parentNodeId, realm);
var folder = new FolderState(parent)
@@ -2977,6 +2995,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
lock (Lock)
{
if (_variables.ContainsKey(key)) return;
if (IsHeldByAnotherKind(key, "variable")) return;
var parent = ResolveParentFolder(parentFolderNodeId, realm);
// Phase C: a non-null historian tagname makes the node Historizing and grants the HistoryRead
@@ -6961,28 +6980,23 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
}
private BaseDataVariableState CreateVariable(string nodeId, ushort ns)
/// <summary>
/// Review 01 H2 — the cross-map guard every node-minting entry point (<see cref="EnsureFolder"/>,
/// <see cref="EnsureVariable"/>, <see cref="MaterialiseAlarmCondition"/>) runs before creating a node:
/// <c>true</c> (and a Warning) when <paramref name="key"/> is already held by a node of ANOTHER kind.
/// The three maps are separate, so nothing else stops one <c>(realm, id)</c> being both a folder and a
/// variable — and the SDK's <c>AddPredefinedNode</c> is an AddOrUpdate, so the second mint silently
/// replaced the first's lookup entry. MUST be called under <c>Lock</c>.
/// </summary>
/// <param name="key">The realm-qualified id about to be minted.</param>
/// <param name="requestedKind">The kind the caller is minting (for the log line).</param>
/// <returns><see langword="true"/> when the id is taken by another kind and the caller must not mint.</returns>
private bool IsHeldByAnotherKind(RealmKey key, string requestedKind)
{
var v = new BaseDataVariableState(_root)
{
NodeId = new NodeId(nodeId, ns),
BrowseName = new QualifiedName(nodeId, ns),
DisplayName = nodeId,
TypeDefinitionId = VariableTypeIds.BaseDataVariableType,
ReferenceTypeId = ReferenceTypeIds.Organizes,
DataType = DataTypeIds.BaseDataType,
// Lazy-created nodes (a WriteValue for a node never declared via EnsureVariable) carry no
// array intent, so they stay scalar with no ArrayDimensions — mirrors EnsureVariable's
// scalar branch. Array nodes are always pre-declared via EnsureVariable(isArray:true).
ValueRank = ValueRanks.Scalar,
ArrayDimensions = null,
AccessLevel = AccessLevels.CurrentRead,
UserAccessLevel = AccessLevels.CurrentRead,
Historizing = false,
};
_root?.AddChild(v);
AddPredefinedNode(SystemContext, v);
return v;
if (ResolveNodeState(key) is not { } existing) return false;
_logger.LogWarning(
"OtOpcUaNodeManager: refusing to materialise {Kind} {NodeId} (realm {Realm}) — the id is already held by a {ExistingKind} node",
requestedKind, key.Id, key.Realm, existing.GetType().Name);
return true;
}
}