From 5919a71b8b03d4a6ef266ce57d721ea3d3289963 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:22:01 -0400 Subject: [PATCH] fix(scriptanalysis): warn on scripts blocking on a Task (.Result/.Wait()/GetAwaiter().GetResult()) (review 2026-10-21 01/SR-U2) Advisory, not a trust-policy violation: every policy entry blocks deploy, and an error here would fail the next redeploy of already-deployed scripts. ValidationService surfaces it as a non-blocking ScriptCompilation warning. --- docs/requirements/Component-ScriptAnalysis.md | 8 +- docs/requirements/Component-TemplateEngine.md | 2 +- .../BlockingWaitAnalyzer.cs | 93 +++++++++++++++++++ .../RoslynScriptCompiler.cs | 47 +++++++--- .../Validation/ScriptCompiler.cs | 36 +++++++ .../Validation/ValidationService.cs | 13 ++- .../BlockingWaitAnalyzerTests.cs | 50 ++++++++++ .../Validation/ValidationServiceTests.cs | 38 ++++++++ 8 files changed, 270 insertions(+), 17 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/BlockingWaitAnalyzer.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/BlockingWaitAnalyzerTests.cs diff --git a/docs/requirements/Component-ScriptAnalysis.md b/docs/requirements/Component-ScriptAnalysis.md index 5b29e381..3ea15105 100644 --- a/docs/requirements/Component-ScriptAnalysis.md +++ b/docs/requirements/Component-ScriptAnalysis.md @@ -126,7 +126,7 @@ Violations from both passes are merged and deduplicated before being returned. #### `Compile(string code, Type? globalsType = null, IEnumerable? extraReferences = null, IEnumerable? extraImports = null)` - Creates a `CSharpScript` with the given code, `globalsType`, references (defaults from `ScriptTrustPolicy.DefaultReferences` plus `extraReferences`), and imports (defaults from `ScriptTrustPolicy.DefaultImports` plus `extraImports`). -- Calls `.Compile()` and returns the resulting `Diagnostic[]` filtered to **error-severity diagnostics only**. This is a compile *gate*: a warning must not block a deploy, so only errors (undefined symbols, type mismatches) are surfaced to callers as gate failures. +- Reads `Script.GetCompilation().GetDiagnostics()` (no emit, no assembly load — see the verdict-cache notes under REQ-SA-4) and returns the diagnostics filtered to **error-severity diagnostics only**. Non-diagnostic faults are thrown, never returned as a message. This is a compile *gate*: a warning must not block a deploy, so only errors (undefined symbols, type mismatches) are surfaced to callers as gate failures. - Each caller passes its own `globalsType` — `ScriptCompileSurface` for the design-time deploy gate, the real `ScriptGlobals` for Site Runtime execution, `null` for pure syntax checks. #### `ParseDiagnostics(string code)` @@ -134,6 +134,12 @@ Violations from both passes are merged and deduplicated before being returned. - Parses the script text using Roslyn's `CSharpSyntaxTree.ParseText` and returns **error-severity** syntax-level diagnostics only (consistent with `Compile` — warnings do not fail the gate). - No compilation is performed — useful for fast syntax checks where no globals type is available. +#### `BlockingWaitAnalyzer.Find(string code, Type? globalsType)` — advisory + +Review 2026-10-21 01/SR-U2. Returns one **advisory** message per synchronous wait on a task in a script body — `.Result` / `.Wait(...)` on `Task`/`ValueTask` (and subclasses), `Task.WaitAll`/`WaitAny`, and `GetResult()` on a `System.Runtime.CompilerServices` awaiter (`.GetAwaiter().GetResult()`, including through `ConfigureAwait`). Site scripts run on the dedicated `ScriptExecutionScheduler` pool and not every runtime helper completes off that pool, so `var r = Instance.CallScript("Calc").Result;` in enough concurrent runs starves the pool until the stuck-script watchdog detaches workers. Resolution is semantic against the caller's `globalsType`, so a non-task member named `Result`/`Wait` is not flagged. + +It is deliberately **not** a `ScriptTrustPolicy` violation: every trust-policy entry blocks deploy, and promoting this to an error would fail the next redeploy of any already-deployed script that blocks (none of the rig seed scripts do, but customer scripts may). The Template Engine surfaces it as a non-blocking `ScriptCompilation` **Warning** on validation (`ScriptCompiler.FindAdvisories`, memoised under its own cache surface key, a faulting analysis yields no advisory). Surfacing it as an editor marker in the Central UI is a Central UI concern. + --- ### REQ-SA-4: Compile-Only Globals Stubs diff --git a/docs/requirements/Component-TemplateEngine.md b/docs/requirements/Component-TemplateEngine.md index d9d95c04..eb6cbc51 100644 --- a/docs/requirements/Component-TemplateEngine.md +++ b/docs/requirements/Component-TemplateEngine.md @@ -259,7 +259,7 @@ Before a deployment is sent to a site, the Template Engine performs comprehensiv - **Flattening**: The full template hierarchy resolves and flattens without errors. - **Naming collision detection**: No duplicate attribute, alarm, or script names in the flattened configuration. -- **Script compilation and trust check**: All instance scripts and alarm on-trigger scripts are compiled via `ScriptCompiler.TryCompile`, which performs a real Roslyn type-checking compile (using `ScriptCompileSurface` from the Script Analysis component as the globals type) and an authoritative forbidden-API check (via `ScriptTrustValidator.FindViolations`). Scripts with compilation errors, type errors, or forbidden-API violations are rejected. This gate is authoritative — not advisory — meaning alias, `using static`, and `global::` bypasses are caught by semantic symbol resolution. +- **Script compilation and trust check**: All instance scripts and alarm on-trigger scripts are compiled via `ScriptCompiler.TryCompile`, which performs a real Roslyn type-checking compile (using `ScriptCompileSurface` from the Script Analysis component as the globals type) and an authoritative forbidden-API check (via `ScriptTrustValidator.FindViolations`). Scripts with compilation errors, type errors, or forbidden-API violations are rejected. This gate is authoritative — not advisory — meaning alias, `using static`, and `global::` bypasses are caught by semantic symbol resolution. A script that compiles is additionally run through `BlockingWaitAnalyzer` (via `ScriptCompiler.FindAdvisories`): `.Result` / `.Wait()` / `.GetAwaiter().GetResult()` on a task produces a non-blocking `ScriptCompilation` **Warning** naming the script and line — it never gates deploy (review 2026-10-21 01/SR-U2). - **Alarm trigger references**: Alarm trigger definitions reference attributes that exist in the flattened configuration. - **Script trigger references**: Script triggers (value change, conditional) reference attributes that exist in the flattened configuration. - **Expression-trigger syntax/trust check**: Each Expression-triggered script/alarm has its boolean expression run through `ValidationService.CheckExpressionSyntax` — a `ScriptTrustValidator.FindViolations` forbidden-API verdict followed by a real Roslyn compile against `TriggerCompileSurface`. Both stages report **all** findings (every forbidden API, every compile error), not just the first, so an operator fixing a multi-error expression sees the full list in one round-trip. The verdict is memoised in the surface-keyed `ScriptCompileVerdictCache`, so an unchanged expression compiles once per process. diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/BlockingWaitAnalyzer.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/BlockingWaitAnalyzer.cs new file mode 100644 index 00000000..8ee01bc9 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/BlockingWaitAnalyzer.cs @@ -0,0 +1,93 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis; + +/// +/// Advisory (non-blocking) analysis that finds scripts synchronously blocking on a +/// — .Result, .Wait(...), Task.WaitAll/WaitAny +/// and .GetAwaiter().GetResult() (review 2026-10-21 01/SR-U2). +/// +/// +/// Site scripts run on the dedicated ScriptExecutionScheduler pool, and not +/// every runtime helper completes off that pool, so a script that blocks on a helper's +/// task can hold a worker while the helper's continuation waits for a worker — enough +/// concurrent runs and the pool starves until the stuck-script watchdog detaches +/// workers. await is always the right form. This is an advisory rather than a +/// violation: the trust policy is a deny-list over API +/// roots whose every entry blocks deploy, and turning this into a deploy-blocking +/// error would break already-deployed scripts on their next redeploy. +/// +/// +/// +/// Resolution is semantic (the script is bound against globalsType), so a +/// member that merely happens to be named Result or Wait on a non-Task +/// type is not reported. Unresolved code is silently skipped — compile errors are the +/// compile gate's job. +/// +/// +public static class BlockingWaitAnalyzer +{ + private const string TasksNamespace = "System.Threading.Tasks"; + private const string CompilerServicesNamespace = "System.Runtime.CompilerServices"; + + /// + /// Returns one advisory message per blocking wait found in . + /// Empty when the script never blocks on a task (or is blank). + /// + /// The C# script source. + /// The globals type the script binds against (e.g. ). + /// Advisory messages naming the member and its line. + public static IReadOnlyList Find(string code, Type? globalsType) + { + if (string.IsNullOrWhiteSpace(code)) + return Array.Empty(); + + var compilation = RoslynScriptCompiler.CreateScript(code, globalsType).GetCompilation(); + var tree = compilation.SyntaxTrees.Single(); + var model = compilation.GetSemanticModel(tree); + var advisories = new List(); + + foreach (var access in tree.GetRoot().DescendantNodes().OfType()) + { + var symbol = model.GetSymbolInfo(access).Symbol; + var form = symbol switch + { + IPropertySymbol { Name: "Result" } p when IsTaskType(p.ContainingType) => ".Result", + IMethodSymbol { Name: "Wait" } m when IsTaskType(m.ContainingType) => ".Wait()", + IMethodSymbol { Name: "WaitAll" or "WaitAny" } m when IsTaskType(m.ContainingType) => $"Task.{m.Name}()", + IMethodSymbol { Name: "GetResult" } m when IsAwaiterType(m.ContainingType) => ".GetAwaiter().GetResult()", + _ => null, + }; + if (form == null) + continue; + + var line = access.GetLocation().GetLineSpan().StartLinePosition.Line + 1; + advisories.Add( + $"blocks on a Task with '{form}' (line {line}) — use 'await' instead; a blocked " + + "script-pool worker can starve the pool the awaited work needs to complete."); + } + + return advisories; + } + + private static bool IsTaskType(INamedTypeSymbol? type) + { + for (var t = type; t != null; t = t.BaseType) + { + if (t.Name is "Task" or "ValueTask" + && t.ContainingNamespace?.ToDisplayString() == TasksNamespace) + { + return true; + } + } + + return false; + } + + private static bool IsAwaiterType(INamedTypeSymbol? type) + => type != null + && type.Name.EndsWith("Awaiter", StringComparison.Ordinal) + && (type.ContainingNamespace?.ToDisplayString() == CompilerServicesNamespace + || type.ContainingType?.ContainingNamespace?.ToDisplayString() == CompilerServicesNamespace); +} diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/RoslynScriptCompiler.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/RoslynScriptCompiler.cs index 8ffd3238..f1ff9e19 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/RoslynScriptCompiler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/RoslynScriptCompiler.cs @@ -69,20 +69,7 @@ public static class RoslynScriptCompiler IEnumerable? extraReferences = null, IEnumerable? extraImports = null) { - var references = ScriptTrustPolicy.DefaultReferences.ToList(); - if (extraReferences != null) - references.AddRange(extraReferences); - - var imports = ScriptTrustPolicy.DefaultImports.AsEnumerable(); - if (extraImports != null) - imports = imports.Concat(extraImports); - - var options = ScriptOptions.Default - .WithReferences(references) - .WithMetadataResolver(CachingScriptMetadataResolver.Instance) - .WithImports(imports); - - var script = CSharpScript.Create(code, options, globalsType, SharedNonLoadingLoader); + var script = CreateScript(code, globalsType, extraReferences, extraImports); // Diagnostics only — NO emit, NO load. `Script.Compile()` builds the // executor, which emits the PE image and loads it through @@ -104,4 +91,36 @@ public static class RoslynScriptCompiler .Select(d => d.GetMessage()) .ToList(); } + + /// + /// Builds the design-time with the trust-model references, + /// imports, the shared metadata resolver and the shared non-loading assembly + /// loader. Callers must only read its compilation — never run it. + /// + /// The C# script source. + /// Optional globals type the script binds against. + /// Optional additional metadata references. + /// Optional additional namespace imports. + /// The unexecuted script. + internal static Script CreateScript( + string code, + Type? globalsType, + IEnumerable? extraReferences = null, + IEnumerable? extraImports = null) + { + var references = ScriptTrustPolicy.DefaultReferences.ToList(); + if (extraReferences != null) + references.AddRange(extraReferences); + + var imports = ScriptTrustPolicy.DefaultImports.AsEnumerable(); + if (extraImports != null) + imports = imports.Concat(extraImports); + + var options = ScriptOptions.Default + .WithReferences(references) + .WithMetadataResolver(CachingScriptMetadataResolver.Instance) + .WithImports(imports); + + return CSharpScript.Create(code, options, globalsType, SharedNonLoadingLoader); + } } diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs index 5e203874..a05c3afb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs @@ -99,4 +99,40 @@ public class ScriptCompiler ? Result.Success(true) : Result.Failure($"Script '{scriptName}' {error}"); } + + /// + /// Non-blocking advisories for a script that already compiles — today, synchronous + /// blocking on a (.Result, + /// .Wait(), .GetAwaiter().GetResult()), which can starve the site's + /// script-execution pool (review 2026-10-21 01/SR-U2). Memoised process-wide like + /// the compile verdict, under its own surface key. A faulting analysis yields no + /// advisories (and is not cached) — an advisory must never fail validation. + /// + /// The C# script code. + /// Advisory messages; empty when there is nothing to flag. + public IReadOnlyList FindAdvisories(string code) + { + if (string.IsNullOrWhiteSpace(code)) + return []; + + try + { + var (_, joined) = ScriptCompileVerdictCache.GetOrAdd(AdvisorySurface, code, () => + { + var advisories = BlockingWaitAnalyzer.Find(code, typeof(ScriptCompileSurface)); + return (true, advisories.Count == 0 ? null : string.Join(AdvisorySeparator, advisories)); + }); + return joined == null ? [] : joined.Split(AdvisorySeparator); + } + catch (Exception) + { + return []; + } + } + + /// Cache surface key for the advisory pass (distinct from the compile verdict). + private const string AdvisorySurface = nameof(ScriptCompileSurface) + "/advisories"; + + /// Separator for the advisories packed into one cached string. + private const string AdvisorySeparator = "\n"; } diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index fcecb166..34c62d73 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -269,7 +269,8 @@ public class ValidationService } /// - /// Validates that all scripts compile successfully using the ScriptCompiler. + /// Validates that all scripts compile successfully using the ScriptCompiler, and adds + /// non-blocking warnings for compiled scripts that block synchronously on a Task. /// /// The flattened configuration to validate. /// A with errors for each script that fails compilation. @@ -286,6 +287,16 @@ public class ValidationService errors.Add(ValidationEntry.Error(ValidationCategory.ScriptCompilation, $"Script '{script.CanonicalName}' failed compilation: {result.Error}", script.CanonicalName)); + continue; + } + + // Advisories never gate deploy (review 2026-10-21 01/SR-U2): a script that + // blocks on a Task still deploys, but the operator sees why it is risky. + foreach (var advisory in _scriptCompiler.FindAdvisories(script.Code)) + { + warnings.Add(ValidationEntry.Warning(ValidationCategory.ScriptCompilation, + $"Script '{script.CanonicalName}' {advisory}", + script.CanonicalName)); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/BlockingWaitAnalyzerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/BlockingWaitAnalyzerTests.cs new file mode 100644 index 00000000..16356ca6 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/BlockingWaitAnalyzerTests.cs @@ -0,0 +1,50 @@ +using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; + +namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests; + +/// +/// Review 2026-10-21 01/SR-U2: synchronously blocking on a Task inside a site +/// script (.Result, .Wait(), .GetAwaiter().GetResult()) can +/// starve the dedicated script-execution pool — the awaited helper's continuation +/// is queued to the same pool the blocked worker occupies. The analyzer reports an +/// advisory for each such use, resolved semantically so a non-Task .Result +/// is not flagged. +/// +public class BlockingWaitAnalyzerTests +{ + /// Verifies that each blocking-wait shape on a Task is reported. + /// A script blocking on a Task. + [Theory] + [InlineData("var r = Instance.CallScript(\"Calc\").Result;")] + [InlineData("var s = Notify.Status(\"id\").Result;")] + [InlineData("Instance.CallScript(\"Calc\").Wait();")] + [InlineData("var r = Instance.CallScript(\"Calc\").GetAwaiter().GetResult();")] + [InlineData("var a = Alarms.CurrentAsync().Result;")] + [InlineData("var t = System.Threading.Tasks.Task.Delay(1); t.Wait(100);")] + [InlineData("var r = Instance.CallScript(\"Calc\").ConfigureAwait(false).GetAwaiter().GetResult();")] + public void Find_ReportsBlockingWaitOnTask(string code) + { + Assert.NotEmpty(BlockingWaitAnalyzer.Find(code, typeof(ScriptCompileSurface))); + } + + /// Verifies that awaited calls and non-Task members named Result/Wait are not reported. + /// A script that does not block on a Task. + [Theory] + [InlineData("var r = await Instance.CallScript(\"Calc\");")] + [InlineData("var a = await Alarms.CurrentAsync();")] + [InlineData("var o = new { Result = 1 }; var x = o.Result;")] + [InlineData("var t = Instance.CallScript(\"Calc\"); return t.IsCompleted;")] + [InlineData("")] + public void Find_IgnoresNonBlockingCode(string code) + { + Assert.Empty(BlockingWaitAnalyzer.Find(code, typeof(ScriptCompileSurface))); + } + + /// Verifies that the advisory names the blocking member so the author can find it. + [Fact] + public void Find_MessageNamesTheMember() + { + var advisories = BlockingWaitAnalyzer.Find("var r = Instance.CallScript(\"Calc\").Result;", typeof(ScriptCompileSurface)); + Assert.Contains(advisories, a => a.Contains(".Result") && a.Contains("await")); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs index 3466a300..61d5a760 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs @@ -504,6 +504,44 @@ public class ValidationServiceTests Assert.Contains(gated.Errors, e => e.Message.Contains("Ghost")); } + /// + /// Review 2026-10-21 01/SR-U2: a script that blocks on a Task gets a non-blocking + /// warning (it still deploys — existing scripts are not broken), naming the script. + /// + [Fact] + public void Validate_ScriptBlockingOnTask_ReturnsWarningNotError() + { + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Scripts = [new ResolvedScript { CanonicalName = "Blocker", Code = "var a = Alarms.CurrentAsync().Result;" }] + }; + + var result = _sut.Validate(config); + + Assert.True(result.IsValid); + Assert.Contains(result.Warnings, w => + w.Category == ValidationCategory.ScriptCompilation + && w.EntityName == "Blocker" + && w.Message.Contains(".Result")); + } + + /// Verifies that an awaiting script produces no blocking-wait warning. + [Fact] + public void Validate_ScriptAwaitingTask_NoBlockingWaitWarning() + { + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Scripts = [new ResolvedScript { CanonicalName = "Good", Code = "var a = await Alarms.CurrentAsync();" }] + }; + + var result = _sut.Validate(config); + + Assert.True(result.IsValid); + Assert.DoesNotContain(result.Warnings, w => w.EntityName == "Good"); + } + private static FlattenedConfiguration ConfigWithExpressionTriggerScript(string expression) { var json = System.Text.Json.JsonSerializer.Serialize(new { expression });