From 78b10d00d82b79a7d8491063798dcf2c5a391cb8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 05:43:18 -0400 Subject: [PATCH] fix(triggers): bound expression evaluation, align AlarmActor error handling, dedupe config parsing --- .../Actors/AlarmActor.cs | 52 +++++++++++++------ .../Actors/InstanceActor.cs | 24 ++------- .../Actors/ScriptActor.cs | 24 +++++---- .../Scripts/TriggerExpressionGlobals.cs | 25 +++++++++ 4 files changed, 80 insertions(+), 45 deletions(-) diff --git a/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs b/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs index 65588c7..5483172 100644 --- a/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs @@ -51,8 +51,8 @@ public class AlarmActor : ReceiveActor private readonly Script? _onTriggerCompiledScript; // Expression trigger: compiled expression + the attribute snapshot it - // evaluates against. The compiled expression is also held on the - // ExpressionEvalConfig; this field caches it for the hot path. + // evaluates against. This field is the single home for the compiled + // expression on the hot path. private readonly Script? _compiledTriggerExpression; private readonly Dictionary _attributeSnapshot = new(); @@ -369,16 +369,38 @@ public class AlarmActor : ReceiveActor /// Evaluates the compiled trigger expression against the current attribute /// snapshot, returning the resulting bool. This bool feeds the existing /// binary Normal↔Active state path — the alarm is active while true. A - /// throwing or non-bool expression is treated as false; the exception - /// propagates to the caller's catch, which logs it and continues. + /// throwing, non-bool, or timed-out expression is treated as false (logged + /// as an alarm error) so that the state machine still runs — an Active + /// alarm correctly clears if the expression starts throwing. /// private bool EvaluateExpression() { if (_compiledTriggerExpression == null) return false; - var globals = new TriggerExpressionGlobals(_attributeSnapshot); - var state = _compiledTriggerExpression.RunAsync(globals).GetAwaiter().GetResult(); - return state.ReturnValue is bool b && b; + try + { + var globals = new TriggerExpressionGlobals(_attributeSnapshot); + // Bound evaluation with a short timeout. The CancellationToken + // covers cooperative/async cases; a pathological CPU-bound + // expression is not fully interruptible. Acceptable because + // trigger expressions are authored by trusted Design-role users + // and are compile-checked pre-deployment. + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + var state = _compiledTriggerExpression + .RunAsync(globals, cancellationToken: cts.Token) + .GetAwaiter().GetResult(); + return state.ReturnValue is bool b && b; + } + catch (Exception ex) + { + // OperationCanceledException (timeout) falls through here too, + // and is correctly treated as false. + _healthCollector?.IncrementAlarmError(); + _logger.LogError(ex, + "Alarm {Alarm} trigger expression evaluation failed on {Instance}; treated as false", + _alarmName, _instanceName); + return false; + } } /// @@ -518,12 +540,12 @@ public class AlarmActor : ReceiveActor HiHiMessage: TryReadString(root, "hiHiMessage")), // Expression triggers have no single monitored attribute; they - // evaluate the compiled expression (passed into the actor) over - // the full attribute snapshot. MonitoredAttributeName is unused. + // evaluate the compiled expression (passed into the actor and + // cached in _compiledTriggerExpression) over the full attribute + // snapshot. MonitoredAttributeName is unused. AlarmTriggerType.Expression => new ExpressionEvalConfig( "", - TryReadString(root, "expression") ?? "", - _compiledTriggerExpression), + TriggerExpressionGlobals.ExtractExpression(triggerConfigJson) ?? ""), _ => new ValueMatchEvalConfig(attr, null) }; @@ -590,13 +612,13 @@ internal record RateOfChangeEvalConfig( /// /// Expression evaluation config: a read-only boolean C# expression evaluated /// over the full attribute snapshot. Has no single monitored attribute -/// ( is empty); the -/// compiled expression is passed into the actor and cached here. +/// ( is empty). The +/// compiled expression itself lives on the actor's _compiledTriggerExpression +/// field, the single source for the hot path. /// internal record ExpressionEvalConfig( string MonitoredAttributeName, - string Expression, - Script? CompiledExpression) : AlarmEvalConfig(MonitoredAttributeName); + string Expression) : AlarmEvalConfig(MonitoredAttributeName); /// /// HiLo evaluation config: any subset of the four setpoints may be set; null diff --git a/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs b/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs index 9f49b3e..06bb815 100644 --- a/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs @@ -1,4 +1,5 @@ using Akka.Actor; +using Microsoft.CodeAnalysis.Scripting; using Microsoft.Extensions.Logging; using ScadaLink.Commons.Messages.DataConnection; using ScadaLink.Commons.Messages.DebugView; @@ -540,7 +541,7 @@ public class InstanceActor : ReceiveActor // Create Alarm Actors foreach (var alarm in _configuration.Alarms) { - Microsoft.CodeAnalysis.Scripting.Script? onTriggerScript = null; + Script? onTriggerScript = null; // Compile on-trigger script if defined if (!string.IsNullOrEmpty(alarm.OnTriggerScriptCanonicalName)) @@ -599,29 +600,14 @@ public class InstanceActor : ReceiveActor /// expression, or a compilation failure (logged) — in which case the /// trigger is inert and the actor still starts. /// - private Microsoft.CodeAnalysis.Scripting.Script? CompileTriggerExpression( + private Script? CompileTriggerExpression( string? triggerType, string? triggerConfigJson, string compileName) { if (!string.Equals(triggerType, "Expression", StringComparison.OrdinalIgnoreCase)) return null; - if (string.IsNullOrEmpty(triggerConfigJson)) - return null; - string? expression; - try - { - var doc = JsonSerializer.Deserialize(triggerConfigJson); - expression = doc.TryGetProperty("expression", out var e) ? e.GetString() : null; - } - catch (Exception ex) - { - _logger.LogWarning(ex, - "Failed to read trigger expression config for {Name} on {Instance}", - compileName, _instanceUniqueName); - return null; - } - - if (string.IsNullOrWhiteSpace(expression)) + var expression = TriggerExpressionGlobals.ExtractExpression(triggerConfigJson); + if (expression == null) return null; var result = _compilationService.CompileTriggerExpression(compileName, expression); diff --git a/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs b/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs index 60c6a28..6c49b41 100644 --- a/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs @@ -210,11 +210,21 @@ public class ScriptActor : ReceiveActor, IWithTimers try { var globals = new TriggerExpressionGlobals(_attributeSnapshot); - var state = _compiledTriggerExpression.RunAsync(globals).GetAwaiter().GetResult(); + // Bound evaluation with a short timeout. The CancellationToken + // covers cooperative/async cases; a pathological CPU-bound + // expression is not fully interruptible. Acceptable because + // trigger expressions are authored by trusted Design-role users + // and are compile-checked pre-deployment. + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + var state = _compiledTriggerExpression + .RunAsync(globals, cancellationToken: cts.Token) + .GetAwaiter().GetResult(); result = state.ReturnValue is bool b && b; } catch (Exception ex) { + // OperationCanceledException (timeout) falls through here too, + // and is correctly treated as false. LogExpressionError(ex); result = false; } @@ -346,16 +356,8 @@ public class ScriptActor : ReceiveActor, IWithTimers private static ExpressionTriggerConfig? ParseExpressionTrigger(string? json) { - if (string.IsNullOrEmpty(json)) return null; - try - { - var doc = JsonDocument.Parse(json); - var expr = doc.RootElement.TryGetProperty("expression", out var e) - ? e.GetString() - : null; - return string.IsNullOrWhiteSpace(expr) ? null : new ExpressionTriggerConfig(expr); - } - catch { return null; } + var expr = TriggerExpressionGlobals.ExtractExpression(json); + return expr == null ? null : new ExpressionTriggerConfig(expr); } private static IntervalTriggerConfig? ParseIntervalTrigger(string? json) diff --git a/src/ScadaLink.SiteRuntime/Scripts/TriggerExpressionGlobals.cs b/src/ScadaLink.SiteRuntime/Scripts/TriggerExpressionGlobals.cs index ddf1dc5..63ab3b2 100644 --- a/src/ScadaLink.SiteRuntime/Scripts/TriggerExpressionGlobals.cs +++ b/src/ScadaLink.SiteRuntime/Scripts/TriggerExpressionGlobals.cs @@ -1,3 +1,5 @@ +using System.Text.Json; + namespace ScadaLink.SiteRuntime.Scripts; /// @@ -11,6 +13,29 @@ namespace ScadaLink.SiteRuntime.Scripts; /// public sealed class TriggerExpressionGlobals { + /// + /// Extracts the "expression" field from an Expression-trigger config + /// JSON document. Returns null for a missing, blank, or malformed + /// config — the single parsing idiom shared by InstanceActor, ScriptActor, + /// and AlarmActor. + /// + public static string? ExtractExpression(string? triggerConfigJson) + { + if (string.IsNullOrEmpty(triggerConfigJson)) return null; + try + { + using var doc = JsonDocument.Parse(triggerConfigJson); + var expr = doc.RootElement.TryGetProperty("expression", out var e) + ? e.GetString() + : null; + return string.IsNullOrWhiteSpace(expr) ? null : expr; + } + catch (JsonException) + { + return null; + } + } + private readonly IReadOnlyDictionary _snapshot; public TriggerExpressionGlobals(IReadOnlyDictionary snapshot)