From 1d7e2a0f8beb106ef0d2a66dbd8ece314989802a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 11 Jun 2026 06:32:53 -0400 Subject: [PATCH] fix(runtime): reject empty AddComment instead of silently swallowing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate AddComment up-front (IsNullOrWhiteSpace guard + Warning log) so a blank-comment command is cleanly rejected before reaching the engine rather than faulting inside ApplyAddComment and being silently swallowed by the outer catch. Mirrors the existing TimedShelve missing-UnshelveAtUtc pattern. Also fix two stale inline comments: the "async void crash" note on TimedShelve now correctly says "fault escaping async Task → supervision restart", and the ownership-filter now documents the benign race with a concurrent LoadAsync clearing the loaded set. Tests: AlarmCommand_add_comment_empty_text_is_rejected_not_driven (Theory — empty string + whitespace) and AlarmCommand_add_comment_nonempty_drives_engine (positive path, asserts CommentAdded transition on alerts topic). --- .../ScriptedAlarms/ScriptedAlarmHostActor.cs | 14 +++-- .../ScriptedAlarmHostActorTests.cs | 52 +++++++++++++++++++ 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs index d87de52f..491ba568 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs @@ -310,6 +310,8 @@ public sealed class ScriptedAlarmHostActor : ReceiveActor { // Ownership filter FIRST: ignore commands for alarms this engine doesn't own. The topic is a // cluster-wide broadcast, so the same command lands on every host — only the owner acts. + // Note: a concurrent LoadAsync can momentarily clear the loaded set, so a racing command may + // surface a spurious ArgumentException from the engine below — the outer catch absorbs it (benign). if (!_engine.LoadedAlarmIds.Contains(cmd.AlarmId)) { _log.Debug("ScriptedAlarmHost: ignoring AlarmCommand {Op} for unowned alarm {AlarmId}", @@ -333,7 +335,7 @@ public sealed class ScriptedAlarmHostActor : ReceiveActor case "TimedShelve": // A timed shelve needs the absolute unshelve instant. T18 derives it from the OPC UA // Duration (UtcNow + shelvingTime); a command missing it is malformed — log + reject - // rather than throw (a throw out of this async void would crash the actor). + // rather than throw (a fault escaping the async Task would trigger a supervision restart). if (cmd.UnshelveAtUtc is not { } unshelveAt) { _log.Warning("ScriptedAlarmHost: rejecting TimedShelve for {AlarmId} — missing UnshelveAtUtc", @@ -352,10 +354,12 @@ public sealed class ScriptedAlarmHostActor : ReceiveActor await _engine.DisableAsync(cmd.AlarmId, cmd.User, CancellationToken.None); break; case "AddComment": - // AddComment's text is required by the engine (ApplyAddComment takes a non-null text); - // coalesce a null comment to empty so a comment-less AddComment is still a valid no-op - // rather than an NRE. - await _engine.AddCommentAsync(cmd.AlarmId, cmd.User, cmd.Comment ?? string.Empty, CancellationToken.None); + if (string.IsNullOrWhiteSpace(cmd.Comment)) + { + _log.Warning("ScriptedAlarmHost: rejecting AddComment for {AlarmId} — empty comment text", cmd.AlarmId); + return; + } + await _engine.AddCommentAsync(cmd.AlarmId, cmd.User, cmd.Comment, CancellationToken.None); break; default: _log.Warning("ScriptedAlarmHost: ignoring AlarmCommand with unknown operation {Op} for {AlarmId}", diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ScriptedAlarms/ScriptedAlarmHostActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ScriptedAlarms/ScriptedAlarmHostActorTests.cs index 3b6db3f2..983df7f2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ScriptedAlarms/ScriptedAlarmHostActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ScriptedAlarms/ScriptedAlarmHostActorTests.cs @@ -305,6 +305,58 @@ public sealed class ScriptedAlarmHostActorTests : RuntimeActorTestBase alerts.ExpectNoMsg(TimeSpan.FromMilliseconds(500)); // no engine op → no transition } + /// Validation: an AddComment command with empty or whitespace comment text is rejected (logged), + /// NOT propagated to the engine — the actor stays alive and still processes a subsequent valid command, + /// proving it didn't fault and the engine's AddCommentAsync was never driven. + [Theory] + [InlineData("")] + [InlineData(" ")] + public void AlarmCommand_add_comment_empty_text_is_rejected_not_driven(string emptyComment) + { + var publish = CreateTestProbe(); + var mux = CreateTestProbe(); + var alerts = CreateTestProbe(); + SubscribeToAlerts(alerts); + + var (host, _) = Spawn(publish, mux); + host.Tell(new ScriptedAlarmHostActor.ApplyScriptedAlarms(new[] { Plan(id: "alm-1", depRef: "M.T") })); + mux.ExpectMsg(Timeout); // load completed + + // AddComment with empty/whitespace text is rejected before reaching the engine. + host.Tell(new AlarmCommand( + AlarmId: "alm-1", Operation: "AddComment", User: "alice", Comment: emptyComment, UnshelveAtUtc: null)); + publish.ExpectNoMsg(TimeSpan.FromMilliseconds(500)); // rejected → no engine op → no OPC UA projection + alerts.ExpectNoMsg(TimeSpan.FromMilliseconds(200)); // rejected → no alerts event + + // Prove the actor survived: activate the alarm and observe the normal projection flow. + host.Tell(new VirtualTagActor.DependencyValueChanged("M.T", 99, DateTime.UtcNow)); + var state = publish.FishForMessage(m => m.State.Active, Timeout); + state.AlarmNodeId.ShouldBe("alm-1"); + } + + /// Positive AddComment path: a non-empty AddComment for a loaded alarm drives the engine's + /// AddCommentAsync — observed via an AlarmTransitionEvent("CommentAdded") on the alerts topic carrying + /// the acting user (proves the op ran end-to-end through the host dispatch). + [Fact] + public void AlarmCommand_add_comment_nonempty_drives_engine() + { + var publish = CreateTestProbe(); + var mux = CreateTestProbe(); + var alerts = CreateTestProbe(); + SubscribeToAlerts(alerts); + + var (host, _) = Spawn(publish, mux); + host.Tell(new ScriptedAlarmHostActor.ApplyScriptedAlarms(new[] { Plan(id: "alm-1", depRef: "M.T") })); + mux.ExpectMsg(Timeout); // load completed + + // AddComment with a non-empty comment drives the engine — CommentAdded transition emitted. + host.Tell(new AlarmCommand( + AlarmId: "alm-1", Operation: "AddComment", User: "bob", Comment: "note from operator", UnshelveAtUtc: null)); + + var evt = alerts.FishForMessage(e => e.TransitionKind == "CommentAdded", Timeout); + evt.AlarmId.ShouldBe("alm-1"); + } + /// Validation: a TimedShelve command missing UnshelveAtUtc is rejected (logged), NOT thrown — /// the actor stays alive and still processes a subsequent valid command, proving it didn't fault. [Fact]