fix(scripting): quarantine a refusal behind a stuck orphan (fix round 2)

Ruling refinement on I1. Only timeouts are strikes, so a never-returning
while(true), which times out once and is then only refused, was never
quarantined, and that defeated E1. A refusal now quarantines immediately
when the orphan it was refused behind has been executing for longer than
ScriptingOptions.StuckOrphanQuarantineFactor x the script's timeout
(default 10, so 20 s at the 2 s default; 0 or less disables it). It uses
the same quarantine path: one Error, otopcua.script.quarantined with
reason=stuck, Bad, lifted on source change or redeploy. A young orphan
(a short overrun) is still a plain refusal.

The guard stamps the run's start (Stopwatch timestamp) when the pool
picks it up and clears it when the run really finishes.

Tests: the 30-trigger while(true) regression (paced) now ends
quarantined with reason "stuck", still with exactly one orphan and a
healthy sibling. Positive control: a 2x overrun is refused, not
quarantined, and evaluates again once the orphan completes. A factor of
0 disables the rule. Docs (ScriptEditor "Runtime safety", VirtualTags)
and the seam remarks are updated.

Refs #734

Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 13:59:10 -04:00
parent ef2eebe6b6
commit a85d1c069a
8 changed files with 133 additions and 23 deletions
+8 -3
View File
@@ -190,9 +190,14 @@ scripted-alarm predicates) runs scripts through the shared `ScriptRunGuard`:
Only a timeout is a strike: a refusal neither adds to nor resets the streak, and
a run that returns (value or throw) resets it. A quarantined script is never
dispatched again; it reads Bad `"script quarantined: …"`, logs one Error and
increments `otopcua_script_quarantined`. An infinite loop therefore usually shows
as *refused* (one timeout, then refusals for as long as the orphan spins) rather
than *quarantined*.
increments `otopcua_script_quarantined`.
- **Stuck-orphan quarantine.** A never-returning script (`while(true)`) times out
once and is then only refused, so the timeout streak alone never reaches 3.
Instead, a refusal behind an orphan that has been running longer than
`Scripting:StuckOrphanQuarantineFactor` × the timeout (default 10 × — 20 s at the
2 s default) quarantines immediately, through the same path (Error log, metric
`reason=stuck`, Bad). A short overrun (a young orphan) stays a plain refusal and
the script resumes once the orphan completes.
- **Allocation budget.** A single run allocating more than
`Scripting:MaxAllocBytesPerRun` (64 MB) is quarantined. This is sampled after the
run returns, so a run that OOMs the process first is not stopped — that needs an
+1 -1
View File
@@ -34,7 +34,7 @@ User scripts are compiled via `Microsoft.CodeAnalysis.CSharp` (regular compiler,
#### Known resource limits (accepted trade-offs)
The sandbox cannot pre-empt a script. **Memory:** each run's own-thread allocation is sampled (`GC.GetAllocatedBytesForCurrentThread`) and a run over `Scripting:MaxAllocBytesPerRun` (default 64 MB) quarantines the script — but the sample is taken after the run returns, so a script that allocates fast enough (`new byte[int.MaxValue]` in a loop) can still OOM the process first. **CPU:** a tight loop keeps its pool thread after the timeout fires; `ScriptRunGuard` bounds that to one orphan per script by refusing further runs while it spins (Bad `"previous evaluation still running"`) and quarantines after three consecutive timeouts (a refusal is not a strike). Both are mitigations; pre-emptive CPU/memory limits need an out-of-process runner. Script authoring is gated behind the Admin permission as the primary control. See `docs/ScriptEditor.md` §"Runtime safety".
The sandbox cannot pre-empt a script. **Memory:** each run's own-thread allocation is sampled (`GC.GetAllocatedBytesForCurrentThread`) and a run over `Scripting:MaxAllocBytesPerRun` (default 64 MB) quarantines the script — but the sample is taken after the run returns, so a script that allocates fast enough (`new byte[int.MaxValue]` in a loop) can still OOM the process first. **CPU:** a tight loop keeps its pool thread after the timeout fires; `ScriptRunGuard` bounds that to one orphan per script by refusing further runs while it spins (Bad `"previous evaluation still running"`) and quarantines after three consecutive timeouts (a refusal is not a strike) — or immediately on a refusal behind an orphan that has run longer than `Scripting:StuckOrphanQuarantineFactor` (default 10) × the timeout, which is what quarantines a never-returning `while(true)`. Both are mitigations; pre-emptive CPU/memory limits need an out-of-process runner. Script authoring is gated behind the Admin permission as the primary control. See `docs/ScriptEditor.md` §"Runtime safety".
`System.Threading.Tasks` is denied (Core.Scripting-003), which prevents `Task.Run` / `Parallel` fan-out that would spawn background work outliving the timeout.
@@ -76,7 +76,7 @@ public sealed class ScriptedAlarmEngine : IDisposable
/// <summary>
/// Per-alarm run quarantine (keyed by alarm id, source identity = predicate source). Refuses a
/// predicate run while the alarm's previous (timed-out, abandoned) run is still executing and
/// quarantines after three consecutive timeouts (a refusal is not a strike) or an allocation-budget breach — one orphan
/// quarantines after three consecutive timeouts (a refusal is not a strike unless its orphan is stuck past StuckOrphanQuarantineFactor × the budget) or an allocation-budget breach — one orphan
/// per alarm instead of one per upstream push. Reset on every <see cref="LoadAsync"/> (a redeploy).
/// </summary>
private readonly ScriptRunGuard _guard;
@@ -1,4 +1,5 @@
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Diagnostics.Metrics;
using Serilog;
using ZB.MOM.WW.OtOpcUa.Commons.Observability;
@@ -27,7 +28,10 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
/// consecutive timeouts</b> (only a timeout is a strike; a refusal is a Bad publish plus its counter
/// and leaves the streak untouched — on a 1 Hz trigger with a 2 s budget one 4 s overrun is a
/// timeout plus two refusals and must not quarantine; a run that returns — a value or a throw —
/// resets the streak): no further dispatch, every attempt throws
/// resets the streak), <b>or on a refusal behind a stuck orphan</b> — one that has been executing for
/// longer than <see cref="ScriptingOptions.StuckOrphanQuarantineFactor"/> × the timeout (20 s at the
/// 2 s default), which is how a never-returning <c>while(true)</c> (one timeout, then only refusals)
/// gets quarantined: no further dispatch, every attempt throws
/// <see cref="ScriptQuarantinedException"/>, one Error log, one <c>otopcua.script.quarantined</c>
/// increment, one <see cref="Quarantined"/> event. Lifted by a source change (the operator fixed the
/// script — a new source identity under the same key starts clean and the old orphan no longer blocks
@@ -63,11 +67,11 @@ public sealed class ScriptRunGuard
/// <summary>
/// Counter <c>otopcua.script.quarantined</c> (Prometheus: <c>otopcua_script_quarantined</c>): one
/// increment per script quarantined, tagged <c>reason</c> = <c>timeouts</c> | <c>allocation</c>.
/// increment per script quarantined, tagged <c>reason</c> = <c>timeouts</c> | <c>stuck</c> | <c>allocation</c>.
/// </summary>
public static readonly Counter<long> QuarantinedCounter =
OtOpcUaTelemetry.Meter.CreateCounter<long>("otopcua.script.quarantined", unit: "{script}",
description: "Scripts quarantined by the run guard (reason: timeouts | allocation).");
description: "Scripts quarantined by the run guard (reason: timeouts | stuck | allocation).");
/// <summary>
/// Counter <c>otopcua.script.run_refused</c>: runs refused because the script's previous run was
@@ -80,6 +84,7 @@ public sealed class ScriptRunGuard
private readonly ConcurrentDictionary<string, KeyState> _keys = new(StringComparer.Ordinal);
private readonly long _maxAllocBytesPerRun;
private readonly int _quarantineThreshold;
private readonly double _stuckFactor;
private readonly ILogger _logger;
private int _inFlight;
private long _started;
@@ -94,6 +99,7 @@ public sealed class ScriptRunGuard
options ??= new ScriptingOptions();
_maxAllocBytesPerRun = options.MaxAllocBytesPerRun;
_quarantineThreshold = Math.Max(1, options.QuarantineAfterConsecutiveTimeouts);
_stuckFactor = options.StuckOrphanQuarantineFactor;
_logger = logger ?? Log.Logger;
}
@@ -185,6 +191,24 @@ public sealed class ScriptRunGuard
// ran long once. The refusal also leaves the timeout streak untouched.
Interlocked.Increment(ref _refused);
RefusedCounter.Add(1, new KeyValuePair<string, object?>("script", key));
// …unless the orphan is STUCK: a never-returning script times out once and is then only ever
// refused, so the timeout streak alone would never quarantine it. Past the stuck threshold the
// refusal itself quarantines (same path: Error log, metric, Bad, lifted on redeploy/source change).
if (_stuckFactor > 0 && Volatile.Read(ref state.RunStartedTimestamp) is var startedAt and not 0)
{
var running = Stopwatch.GetElapsedTime(startedAt);
var threshold = timeout * _stuckFactor;
if (running > threshold)
{
Quarantine(key, state,
$"its previous evaluation has been running for {running.TotalSeconds:F1} s, over " +
$"{_stuckFactor:0.##}x its {timeout.TotalMilliseconds:F0} ms budget " +
"(Scripting:StuckOrphanQuarantineFactor) — it is stuck; the script is not evaluated again " +
"until its source changes or it is redeployed",
"stuck");
throw new ScriptQuarantinedException(key, state.QuarantineReason!);
}
}
throw new ScriptRunRefusedException(key);
}
@@ -200,6 +224,7 @@ public sealed class ScriptRunGuard
// never complete, and the slot would never be released. The evaluator observes the token instead.
var runTask = Task.Run(async () =>
{
Volatile.Write(ref state.RunStartedTimestamp, Stopwatch.GetTimestamp());
started.TrySetResult();
var before = GC.GetAllocatedBytesForCurrentThread();
try
@@ -223,6 +248,7 @@ public sealed class ScriptRunGuard
// still quarantines its key here.
_ = runTask.ContinueWith(t =>
{
Volatile.Write(ref state.RunStartedTimestamp, 0);
Interlocked.Exchange(ref state.InFlight, 0);
Interlocked.Decrement(ref _inFlight);
runCts.Dispose();
@@ -339,6 +365,8 @@ public sealed class ScriptRunGuard
public string? SourceIdentity = sourceIdentity;
public int InFlight;
public int ConsecutiveTimeouts;
/// <summary><see cref="Stopwatch.GetTimestamp"/> when the pool started the in-flight run; 0 when idle.</summary>
public long RunStartedTimestamp;
public string? QuarantineReason;
}
@@ -373,7 +401,7 @@ public sealed class ScriptRunRefusedException : Exception
/// <summary>
/// Thrown by <see cref="ScriptRunGuard.RunAsync{TContext, TResult}"/> for a quarantined script: repeated
/// timeouts or an allocation-budget breach. Nothing is dispatched while quarantined; the seam
/// timeouts, a stuck orphan or an allocation-budget breach. Nothing is dispatched while quarantined; the seam
/// publishes Bad carrying the reason, which an operator clears by fixing the script (source change) or
/// redeploying.
/// </summary>
@@ -25,6 +25,9 @@ public sealed class ScriptingOptions
/// <summary>Default <see cref="QuarantineAfterConsecutiveTimeouts"/>: 3.</summary>
public const int DefaultQuarantineAfterConsecutiveTimeouts = 3;
/// <summary>Default <see cref="StuckOrphanQuarantineFactor"/>: 10.</summary>
public const double DefaultStuckOrphanQuarantineFactor = 10;
/// <summary>
/// Managed bytes one script run may allocate on its own thread (sampled with
/// <see cref="GC.GetAllocatedBytesForCurrentThread"/> around the run) before the script is
@@ -40,4 +43,14 @@ public sealed class ScriptingOptions
/// throw) resets it. Default 3; values below 1 are treated as 1.
/// </summary>
public int QuarantineAfterConsecutiveTimeouts { get; set; } = DefaultQuarantineAfterConsecutiveTimeouts;
/// <summary>
/// A refusal quarantines the script immediately when the abandoned run it was refused behind has been
/// executing for longer than this many times the script's timeout (20 s at the 2 s default). This is
/// what quarantines a permanently stuck script (<c>while(true)</c>): it times out once and is then
/// only ever refused, so the timeout streak alone would never reach
/// <see cref="QuarantineAfterConsecutiveTimeouts"/>. A young orphan (a short overrun) is still just a
/// refusal. Zero or negative disables the rule. Default 10.
/// </summary>
public double StuckOrphanQuarantineFactor { get; set; } = DefaultStuckOrphanQuarantineFactor;
}
@@ -42,7 +42,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation;
/// <b>Run quarantine.</b> Every compiled run goes through one <see cref="ScriptRunGuard"/> keyed by
/// calc-tag RawPath with the script source as the identity: a run is refused while the tag's previous
/// (timed-out, abandoned) run is still executing, and the tag is quarantined after three consecutive
/// timeouts (a refusal is Bad but not a strike) or an allocation-budget breach — both surface as a <see cref="VirtualTagEvalResult.Failure"/>
/// timeouts (a refusal is Bad but not a strike, unless its orphan is stuck past StuckOrphanQuarantineFactor × the budget) or an allocation-budget breach — both surface as a <see cref="VirtualTagEvalResult.Failure"/>
/// the driver maps to Bad. The driver's evaluation pump awaits <see cref="EvaluateAsync"/> rather than
/// blocking a pool thread on the sync <see cref="Evaluate(string, string, IReadOnlyDictionary{string, DataValueSnapshot})"/>,
/// and the guard starts the budget clock only once the pool has picked the script up, so pool starvation
@@ -35,7 +35,7 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Engines;
/// <b>Run quarantine.</b> Every compiled run goes through one <see cref="ScriptRunGuard"/> keyed by
/// virtual-tag id with the expression as the source identity: a run is refused while the tag's previous
/// (timed-out, abandoned) run is still executing, and the tag is quarantined after three consecutive
/// timeouts (a refusal is Bad but not a strike) or an allocation-budget breach. Both surface as <see cref="VirtualTagEvalResult.Failure"/>
/// timeouts (a refusal is Bad but not a strike, unless its orphan is stuck past StuckOrphanQuarantineFactor × the budget) or an allocation-budget breach. Both surface as <see cref="VirtualTagEvalResult.Failure"/>
/// so <c>VirtualTagActor</c> publishes Bad exactly as it does for a timeout. A quarantine lifts when the
/// expression changes, on <see cref="ClearCompiledScripts"/> / <see cref="ResetQuarantines"/> (the deploy
/// boundary), never by itself.
@@ -151,10 +151,13 @@ public sealed class ScriptRunGuardTests
}
/// <summary>
/// The thread-leak regression. Pre-guard, a <c>while(true)</c> script on a 1 Hz trigger leaked one
/// pool thread per trigger — 30 orphans after 30 triggers, trending toward pool starvation. Now the
/// first trigger's orphan is the only one, every later trigger is refused, and a healthy
/// sibling keeps evaluating throughout (the positive control an absence assertion needs).
/// The thread-leak regression, and the stuck-orphan quarantine (fix round 2). Pre-guard, a
/// <c>while(true)</c> script on a 1 Hz trigger leaked one pool thread per trigger — 30 orphans after 30
/// triggers, trending toward pool starvation. Now the first trigger's orphan is the only one; later
/// triggers are refused while it is young, and once it has run longer than
/// <see cref="ScriptingOptions.StuckOrphanQuarantineFactor"/> × the timeout (10 × 60 ms here — the
/// trigger cadence is scaled down from 1 Hz to keep the test short) the next refusal quarantines it.
/// A healthy sibling keeps evaluating throughout (the positive control an absence assertion needs).
/// </summary>
/// <remarks>
/// The load-bearing assertions are on the guard's own bookkeeping (<see cref="ScriptRunGuard.InFlightCount"/>
@@ -163,30 +166,37 @@ public sealed class ScriptRunGuardTests
/// it must stay far below the 30 the leak would have produced.
/// </remarks>
[Fact]
public async Task Runaway_script_leaves_at_most_one_orphan_after_30_triggers_while_a_sibling_keeps_evaluating()
public async Task Runaway_script_leaves_one_orphan_is_quarantined_once_stuck_and_a_sibling_keeps_evaluating()
{
var guard = new ScriptRunGuard();
using var spin = Compile(SpinScript);
using var fast = Compile(FastScript);
var ct = TestContext.Current.CancellationToken;
var threadsBefore = ThreadPool.ThreadCount;
var stuckAfter = ShortTimeout * ScriptingOptions.DefaultStuckOrphanQuarantineFactor; // 600 ms
var healthyResults = new List<int>();
var outcomes = new List<string>();
var sw = System.Diagnostics.Stopwatch.StartNew();
for (var trigger = 0; trigger < 30; trigger++)
{
try { await Run(guard, "runaway", SpinScript, spin, ShortTimeout, ct); }
catch (ScriptTimeoutException) { }
catch (ScriptRunRefusedException) { }
catch (ScriptQuarantinedException) { }
try { await Run(guard, "runaway", SpinScript, spin, ShortTimeout, ct); outcomes.Add("value"); }
catch (ScriptTimeoutException) { outcomes.Add("timeout"); }
catch (ScriptRunRefusedException) { outcomes.Add(sw.Elapsed < stuckAfter ? "refused-young" : "refused"); }
catch (ScriptQuarantinedException) { outcomes.Add("quarantined"); }
healthyResults.Add(await Run(guard, "healthy", FastScript, fast, ct: ct));
await Task.Delay(50, ct);
}
guard.InFlightCount.ShouldBe(1, "exactly one orphaned run — the first trigger's — may be executing");
guard.StartedRunCount.ShouldBe(1 + 30, "one dispatch for the runaway script, thirty for the healthy sibling");
// One timeout + 29 refusals: not quarantined (a refusal is not a strike), but never re-dispatched.
guard.IsQuarantined("runaway", out _).ShouldBeFalse();
guard.RefusedWhileRunningCount.ShouldBe(29);
outcomes[0].ShouldBe("timeout");
outcomes.ShouldContain("refused-young", "a young orphan is only refused — a short overrun is not quarantined");
guard.IsQuarantined("runaway", out var reason).ShouldBeTrue("the orphan outlived 10x its budget: it is stuck");
reason!.ShouldContain("stuck");
outcomes[^1].ShouldBe("quarantined");
guard.QuarantinedCount.ShouldBe(1);
healthyResults.ShouldAllBe(v => v == 42);
healthyResults.Count.ShouldBe(30);
@@ -194,6 +204,60 @@ public sealed class ScriptRunGuardTests
threadDelta.ShouldBeLessThan(15, "thirty leaked orphans would have grown the pool by ~30 threads");
}
/// <summary>
/// Positive control for the stuck rule: a 2x overrun (one timeout, refusals while the orphan is young)
/// is NOT quarantined, and once the orphan completes the script evaluates normally again.
/// </summary>
[Fact]
public async Task A_2x_overrun_is_not_quarantined_and_resumes_evaluating_once_the_orphan_completes()
{
var guard = new ScriptRunGuard();
const string source =
"""
var ms = (int)(ctx.GetTag("spinMs").Value ?? 0);
var end = System.DateTime.UtcNow.AddMilliseconds(ms);
while (System.DateTime.UtcNow < end) { }
return 7;
""";
using var eval = Compile(source);
var ct = TestContext.Current.CancellationToken;
var timeout = TimeSpan.FromMilliseconds(100);
FakeScriptContext Ctx(int spinMs)
{
var c = new FakeScriptContext();
c.Tags["spinMs"] = new Core.Abstractions.DataValueSnapshot(spinMs, 0u, c.Now, c.Now);
return c;
}
// Warm the JIT so the first measured run's overrun is the script's, not the compiler's.
(await guard.RunAsync("k", source, eval, Ctx(0), TimeSpan.FromSeconds(5), ct)).ShouldBe(7);
// 2x overrun: 200 ms against a 100 ms budget — times out, then is refused while it finishes.
await Should.ThrowAsync<ScriptTimeoutException>(() => guard.RunAsync("k", source, eval, Ctx(200), timeout, ct));
await Should.ThrowAsync<ScriptRunRefusedException>(() => guard.RunAsync("k", source, eval, Ctx(0), timeout, ct));
await WaitForIdle(guard);
guard.IsQuarantined("k", out _).ShouldBeFalse("a 2x overrun is far below the 10x stuck threshold");
(await guard.RunAsync("k", source, eval, Ctx(0), timeout, ct)).ShouldBe(7, "the script resumes evaluating");
guard.QuarantinedCount.ShouldBe(0);
}
/// <summary>The stuck rule is a knob: with <see cref="ScriptingOptions.StuckOrphanQuarantineFactor"/> ≤ 0 a
/// stuck orphan is only ever refused.</summary>
[Fact]
public async Task StuckOrphanQuarantineFactor_zero_disables_the_stuck_rule()
{
var guard = new ScriptRunGuard(new ScriptingOptions { StuckOrphanQuarantineFactor = 0 });
using var spin = Compile(SpinScript);
var ct = TestContext.Current.CancellationToken;
await Should.ThrowAsync<ScriptTimeoutException>(() => Run(guard, "k", SpinScript, spin, ShortTimeout, ct));
await Task.Delay(ShortTimeout * 12, ct);
await Should.ThrowAsync<ScriptRunRefusedException>(() => Run(guard, "k", SpinScript, spin, ShortTimeout, ct));
guard.IsQuarantined("k", out _).ShouldBeFalse();
}
[Fact]
public async Task A_source_change_clears_the_quarantine_and_abandons_the_old_orphan_slot()
{