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.
This commit is contained in:
@@ -126,7 +126,7 @@ Violations from both passes are merged and deduplicated before being returned.
|
||||
#### `Compile(string code, Type? globalsType = null, IEnumerable<MetadataReference>? extraReferences = null, IEnumerable<string>? 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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -0,0 +1,93 @@
|
||||
using Microsoft.CodeAnalysis;
|
||||
using Microsoft.CodeAnalysis.CSharp.Syntax;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
|
||||
|
||||
/// <summary>
|
||||
/// Advisory (non-blocking) analysis that finds scripts synchronously blocking on a
|
||||
/// <see cref="Task"/> — <c>.Result</c>, <c>.Wait(...)</c>, <c>Task.WaitAll/WaitAny</c>
|
||||
/// and <c>.GetAwaiter().GetResult()</c> (review 2026-10-21 01/SR-U2).
|
||||
///
|
||||
/// <para>
|
||||
/// Site scripts run on the dedicated <c>ScriptExecutionScheduler</c> 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. <c>await</c> is always the right form. This is an advisory rather than a
|
||||
/// <see cref="ScriptTrustPolicy"/> 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Resolution is semantic (the script is bound against <c>globalsType</c>), so a
|
||||
/// member that merely happens to be named <c>Result</c> or <c>Wait</c> on a non-Task
|
||||
/// type is not reported. Unresolved code is silently skipped — compile errors are the
|
||||
/// compile gate's job.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public static class BlockingWaitAnalyzer
|
||||
{
|
||||
private const string TasksNamespace = "System.Threading.Tasks";
|
||||
private const string CompilerServicesNamespace = "System.Runtime.CompilerServices";
|
||||
|
||||
/// <summary>
|
||||
/// Returns one advisory message per blocking wait found in <paramref name="code"/>.
|
||||
/// Empty when the script never blocks on a task (or is blank).
|
||||
/// </summary>
|
||||
/// <param name="code">The C# script source.</param>
|
||||
/// <param name="globalsType">The globals type the script binds against (e.g. <see cref="ScriptCompileSurface"/>).</param>
|
||||
/// <returns>Advisory messages naming the member and its line.</returns>
|
||||
public static IReadOnlyList<string> Find(string code, Type? globalsType)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(code))
|
||||
return Array.Empty<string>();
|
||||
|
||||
var compilation = RoslynScriptCompiler.CreateScript(code, globalsType).GetCompilation();
|
||||
var tree = compilation.SyntaxTrees.Single();
|
||||
var model = compilation.GetSemanticModel(tree);
|
||||
var advisories = new List<string>();
|
||||
|
||||
foreach (var access in tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>())
|
||||
{
|
||||
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);
|
||||
}
|
||||
@@ -69,20 +69,7 @@ public static class RoslynScriptCompiler
|
||||
IEnumerable<MetadataReference>? extraReferences = null,
|
||||
IEnumerable<string>? 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<object?>(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();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds the design-time <see cref="Script{T}"/> 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.
|
||||
/// </summary>
|
||||
/// <param name="code">The C# script source.</param>
|
||||
/// <param name="globalsType">Optional globals type the script binds against.</param>
|
||||
/// <param name="extraReferences">Optional additional metadata references.</param>
|
||||
/// <param name="extraImports">Optional additional namespace imports.</param>
|
||||
/// <returns>The unexecuted script.</returns>
|
||||
internal static Script<object?> CreateScript(
|
||||
string code,
|
||||
Type? globalsType,
|
||||
IEnumerable<MetadataReference>? extraReferences = null,
|
||||
IEnumerable<string>? 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<object?>(code, options, globalsType, SharedNonLoadingLoader);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -99,4 +99,40 @@ public class ScriptCompiler
|
||||
? Result<bool>.Success(true)
|
||||
: Result<bool>.Failure($"Script '{scriptName}' {error}");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Non-blocking advisories for a script that already compiles — today, synchronous
|
||||
/// blocking on a <see cref="System.Threading.Tasks.Task"/> (<c>.Result</c>,
|
||||
/// <c>.Wait()</c>, <c>.GetAwaiter().GetResult()</c>), 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.
|
||||
/// </summary>
|
||||
/// <param name="code">The C# script code.</param>
|
||||
/// <returns>Advisory messages; empty when there is nothing to flag.</returns>
|
||||
public IReadOnlyList<string> 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 [];
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Cache surface key for the advisory pass (distinct from the compile verdict).</summary>
|
||||
private const string AdvisorySurface = nameof(ScriptCompileSurface) + "/advisories";
|
||||
|
||||
/// <summary>Separator for the advisories packed into one cached string.</summary>
|
||||
private const string AdvisorySeparator = "\n";
|
||||
}
|
||||
|
||||
@@ -269,7 +269,8 @@ public class ValidationService
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
/// <param name="configuration">The flattened configuration to validate.</param>
|
||||
/// <returns>A <see cref="ValidationResult"/> with errors for each script that fails compilation.</returns>
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
using ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Review 2026-10-21 01/SR-U2: synchronously blocking on a Task inside a site
|
||||
/// script (<c>.Result</c>, <c>.Wait()</c>, <c>.GetAwaiter().GetResult()</c>) 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 <c>.Result</c>
|
||||
/// is not flagged.
|
||||
/// </summary>
|
||||
public class BlockingWaitAnalyzerTests
|
||||
{
|
||||
/// <summary>Verifies that each blocking-wait shape on a Task is reported.</summary>
|
||||
/// <param name="code">A script blocking on a Task.</param>
|
||||
[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)));
|
||||
}
|
||||
|
||||
/// <summary>Verifies that awaited calls and non-Task members named Result/Wait are not reported.</summary>
|
||||
/// <param name="code">A script that does not block on a Task.</param>
|
||||
[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)));
|
||||
}
|
||||
|
||||
/// <summary>Verifies that the advisory names the blocking member so the author can find it.</summary>
|
||||
[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"));
|
||||
}
|
||||
}
|
||||
@@ -504,6 +504,44 @@ public class ValidationServiceTests
|
||||
Assert.Contains(gated.Errors, e => e.Message.Contains("Ghost"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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"));
|
||||
}
|
||||
|
||||
/// <summary>Verifies that an awaiting script produces no blocking-wait warning.</summary>
|
||||
[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 });
|
||||
|
||||
Reference in New Issue
Block a user