docs(opcua): explain intentional CommentAdded/Retain delta-gate suppression (T20 review)

Three code-review points on commit 004558c2 were correct behavior
that was under-documented, not bugs:

1. AlarmConditionDelta gains explicit paragraphs explaining why
   CommentAdded is absent: it always originates from a client
   AddComment call whose T18 OnAddComment handler returns Good →
   SDK auto-fires the comment event (E2); the engine re-projection
   carries no delta-field change, so the gate correctly suppresses
   the duplicate. Force-firing would double-emit.

2. Same doc explains Retain is intentionally absent: Retain is a
   pure function of Active/Acknowledged (both compared), so it
   cannot flip without a real delta. Notes future risk if that
   ever changes.

3. ReportConditionEvent Time/ReceiveTime comment corrected: the
   projection was already applied by WriteAlarmCondition above
   with identical values; the restamp is a locality repeat, not a
   reorder guard.

Also adds one seam unit-test (103 total, was 102) pinning the
null-vs-empty Message normalization boundary so a change to the
?? string.Empty coalescing is caught at the seam level.
This commit is contained in:
Joseph Doherty
2026-06-11 06:38:31 -04:00
parent 1d7e2a0f8b
commit f742050ebd
2 changed files with 53 additions and 3 deletions
@@ -240,9 +240,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
{
// Fresh GUID-bytes EventId per event — mandatory for Part 9 ack correlation (T17 relies on it).
alarm.EventId.Value = Guid.NewGuid().ToByteArray();
// Time/ReceiveTime/Message/Severity were set by the WriteAlarmCondition projection above; restamp
// Time/ReceiveTime here so this event carries this transition's instant even if the caller's
// projection ordering changes later.
// Time/ReceiveTime were already set to sourceTimestampUtc by the WriteAlarmCondition projection
// immediately above; the assignment here is a locality repeat (same value, no behavioral change)
// so the restamp is co-located with the EventId and ClearChangeMasks in the same method.
alarm.Time.Value = ts;
alarm.ReceiveTime.Value = ts;
@@ -289,6 +289,31 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <b>Shelving</b> kind is read back from / mapped to the SDK's shelving state machine so the
/// live node and the snapshot compare on the same 3-way (Unshelved/OneShot/Timed) axis.
/// </para>
/// <para>
/// <b>Why <c>CommentAdded</c> is not a field here (intentional).</b>
/// <c>EmissionKind.CommentAdded</c> is produced only by
/// <c>Part9StateMachine.ApplyAddComment</c>, which is reached only via
/// <c>ScriptedAlarmEngine.AddCommentAsync</c>, which is called only from
/// <c>ScriptedAlarmHostActor</c>'s inbound <c>AlarmCommand</c> handler — meaning
/// <c>CommentAdded</c> ALWAYS originates from a client calling the condition's
/// <c>AddComment</c> method. On that path T18's <c>OnAddComment</c> delegate returns
/// <c>ServiceResult.Good</c>, so the OPC UA SDK itself applies the comment to the node
/// and auto-fires the Part 9 comment event (E2) directly to subscribers — BEFORE the
/// engine re-projects via <see cref="WriteAlarmCondition"/>. When that re-projection
/// arrives here, the delta-gate sees no change in any compared field (the snapshot carries
/// no comments list) and correctly suppresses a second event (E3). Force-firing for
/// <c>CommentAdded</c> would double-emit. There is no engine-internal or script-driven
/// comment path, so suppression never drops a needed event.
/// </para>
/// <para>
/// <b>Why <c>Retain</c> is absent (intentional — safe today).</b>
/// <c>Retain</c> is projected as <c>state.Active || !state.Acknowledged</c> in
/// <see cref="WriteAlarmCondition"/>. Every path that flips <c>Retain</c> necessarily
/// changes <c>Active</c> or <c>Acknowledged</c> (both ARE compared fields), so a
/// <c>Retain</c> flip always rides along with a real delta and fires correctly. If a
/// future engine were to set <c>Retain</c> independently — without touching
/// <c>Active</c>/<c>Acknowledged</c> — it would need to be added here.
/// </para>
/// </summary>
internal readonly record struct AlarmConditionDelta(
bool Active,
@@ -475,6 +475,31 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable
OtOpcUaNodeManager.ShouldFireConditionEvent(baseState, baseState with { Message = "other" }).ShouldBeTrue();
}
/// <summary>T20 — null-vs-empty Message normalization. Both snapshot.Message = null and a live node
/// whose Message.Value.Text = null normalize to <see cref="string.Empty"/> in
/// <see cref="OtOpcUaNodeManager.AlarmConditionDelta"/>, so a snapshot whose Message is null does NOT
/// produce a phantom delta against a freshly-materialised node (whose Message is the displayName but
/// whose Text reads back as the displayName string, and a snapshot that explicitly passes an empty
/// string equally produces no delta vs a null-Message node). This pins the normalization so a
/// future change to the null-coalescing in ReadConditionDelta / ToConditionDelta is caught here.</summary>
[Fact]
public void ShouldFireConditionEvent_null_and_empty_message_normalize_identically()
{
// Both null and "" collapse to string.Empty in AlarmConditionDelta — they are the same delta value.
var withNull = new OtOpcUaNodeManager.AlarmConditionDelta(
Active: false, Acknowledged: true, Confirmed: true, Enabled: true,
Shelving: AlarmShelvingKind.Unshelved, MappedSeverity: 100, Message: string.Empty);
var withEmpty = withNull with { Message = string.Empty };
// null-message snapshot (normalised to "") vs empty-message node (normalised to "") ⇒ no delta.
OtOpcUaNodeManager.ShouldFireConditionEvent(withNull, withEmpty).ShouldBeFalse();
OtOpcUaNodeManager.ShouldFireConditionEvent(withEmpty, withNull).ShouldBeFalse();
// A non-empty message IS a delta vs the empty baseline.
OtOpcUaNodeManager.ShouldFireConditionEvent(withNull, withNull with { Message = "pressure high" }).ShouldBeTrue();
}
/// <summary>Builds a test <see cref="AlarmConditionSnapshot"/> with sensible defaults so each call
/// site only specifies the fields it cares about.</summary>
private static AlarmConditionSnapshot Snapshot(