fix(calculation): dead logger cast + warm-up evaluated before change-gate (review)
Code review of Driver.Calculation found two real defects, both TDD-fixed:
- CalculationDriver's ctor tried to reuse its ILogger<CalculationDriver> as the
evaluator's ILogger<CalculationEvaluator> via an `is` check that can never
succeed (unrelated closed generics), so CalculationEvaluator always ran with
a NullLogger regardless of what the driver was constructed with. Fixed by
having CalculationEvaluator accept the non-generic ILogger instead.
- EvaluateTag called the evaluator BEFORE checking whether every declared
dependency had arrived, so a multi-dependency change-triggered calc tag
evaluated its script against a partial snapshot on every warm-up tick.
VirtualTagContext.GetTag returns a Bad snapshot (.Value == null) for a
missing dep, so the idiomatic `(double)ctx.GetTag("x").Value` cast throws
NullReferenceException — caught and logged as a spurious "script threw"
script-log Warning on ordinary deploy/restart warm-up, for the standard
multi-input calc-tag shape. Fixed by gating evaluation itself on
"all deps arrived", not just the publish.
See code-reviews/Driver.Calculation/findings.md for full detail (5 findings:
001-004 Resolved, 005 Open/cross-module).
This commit is contained in:
@@ -100,10 +100,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba
|
||||
_driverInstanceId = driverInstanceId;
|
||||
_logger = logger ?? NullLogger<CalculationDriver>.Instance;
|
||||
_scriptRoot = scriptRoot ?? new ScriptRootLogger(new LoggerConfiguration().CreateLogger());
|
||||
_evaluator = new CalculationEvaluator(
|
||||
_logger is ILogger<CalculationEvaluator> el ? el : NullLogger<CalculationEvaluator>.Instance,
|
||||
_scriptRoot,
|
||||
_options.RunTimeout);
|
||||
// _logger (ILogger<CalculationDriver>) implements the non-generic ILogger, so it is passed straight
|
||||
// through — the evaluator's host-side diagnostics (compile errors, sandbox violations, execution
|
||||
// throws) land on the SAME logger the driver itself was constructed with, not a separate one.
|
||||
_evaluator = new CalculationEvaluator(_logger, _scriptRoot, _options.RunTimeout);
|
||||
|
||||
RebuildTagTable(_options);
|
||||
}
|
||||
@@ -379,7 +379,14 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba
|
||||
var st = _stateByRawPath[rawPath];
|
||||
|
||||
// Change-gate (mini-design §4): publish nothing until every declared dep has arrived at least once.
|
||||
var allArrived = def.DependencyRefs.All(_arrivedDeps.Contains);
|
||||
// MUST be checked BEFORE the evaluator runs, not just before the publish: a partially-warmed-up
|
||||
// dependency snapshot is missing entries for the not-yet-arrived deps, and VirtualTagContext.GetTag
|
||||
// returns a Bad snapshot (.Value == null) for those — so the common `(double)ctx.GetTag("x").Value`
|
||||
// idiom throws NullReferenceException on every warm-up tick for any multi-dependency script whose
|
||||
// deps don't all arrive in the same tick. Evaluating anyway (and discarding the result afterwards)
|
||||
// used to turn ordinary warm-up into a spurious "script threw" script-log Warning on every partial
|
||||
// arrival, plus a wasted Roslyn/compiled-cache invocation.
|
||||
if (!def.DependencyRefs.All(_arrivedDeps.Contains)) return; // still warming up — do not evaluate yet
|
||||
|
||||
// Build the per-tag dependency snapshot (only its declared reads, current values).
|
||||
var deps = new Dictionary<string, object?>(StringComparer.Ordinal);
|
||||
@@ -390,12 +397,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba
|
||||
|
||||
if (!result.Success)
|
||||
{
|
||||
OnEvaluationFailed(def, st, result.Reason ?? "evaluator failure", allArrived, timestampUtc);
|
||||
OnEvaluationFailed(def, st, result.Reason ?? "evaluator failure", timestampUtc);
|
||||
return;
|
||||
}
|
||||
|
||||
if (!allArrived) return; // change-trigger warm-up: hold until inputs are ready
|
||||
|
||||
// Value dedup — but bypass it while recovering from Bad so a recovery whose value equals the
|
||||
// pre-failure value still republishes Good (otherwise the node would stay Bad forever).
|
||||
if (!st.LastPublishedBad && st.HasValue && Equals(st.LastValue, result.Value))
|
||||
@@ -411,12 +416,13 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba
|
||||
|
||||
/// <summary>Failure tail (mirrors <c>VirtualTagActor</c> 02/S13): always emits the per-failure
|
||||
/// script-log; additionally publishes a Bad snapshot — carrying the last-known value — once per
|
||||
/// Good→Bad transition and only after every declared dependency has arrived.</summary>
|
||||
private void OnEvaluationFailed(
|
||||
CalculationTagDefinition def, TagState st, string reason, bool allArrived, DateTime timestampUtc)
|
||||
/// Good→Bad transition. The caller (<see cref="EvaluateTag"/>) only reaches here once every declared
|
||||
/// dependency has arrived — the warm-up gate returns before the evaluator ever runs — so there is no
|
||||
/// separate "inputs ready" condition to re-check here.</summary>
|
||||
private void OnEvaluationFailed(CalculationTagDefinition def, TagState st, string reason, DateTime timestampUtc)
|
||||
{
|
||||
PublishScriptLog(def, reason);
|
||||
if (st.LastPublishedBad || !allArrived) return;
|
||||
if (st.LastPublishedBad) return;
|
||||
|
||||
st.LastPublishedBad = true;
|
||||
st.LastTimestampUtc = timestampUtc;
|
||||
|
||||
@@ -44,17 +44,20 @@ public sealed class CalculationEvaluator : IScriptCacheOwner, IDisposable
|
||||
// that dispose each evaluator's collectible AssemblyLoadContext. ClearCompiledScripts() — driven by the
|
||||
// WP7 driver per deploy generation — is what stops ALCs accreting across script edits.
|
||||
private readonly CompiledScriptCache<VirtualTagContext, object?> _cache = new();
|
||||
private readonly ILogger<CalculationEvaluator> _logger;
|
||||
private readonly ILogger _logger;
|
||||
private readonly SerilogLogger _scriptRoot;
|
||||
private readonly TimeSpan _runTimeout;
|
||||
private bool _disposed;
|
||||
|
||||
/// <summary>Initializes a new <see cref="CalculationEvaluator"/>.</summary>
|
||||
/// <param name="logger">Logger for host-side compilation/execution diagnostics.</param>
|
||||
/// <param name="logger">Logger for host-side compilation/execution diagnostics. Deliberately the
|
||||
/// non-generic <see cref="ILogger"/> (rather than <c>ILogger<CalculationEvaluator></c>) so the
|
||||
/// caller can forward its OWN <c>ILogger<T></c> — every <c>ILogger<T></c> already
|
||||
/// implements this — instead of needing a second, separately-obtained logger instance.</param>
|
||||
/// <param name="scriptRoot">Root script logger; user <c>ctx.Logger.*</c> output flows through this to the Script-log page.</param>
|
||||
/// <param name="runTimeout">Wall-clock budget per script run; defaults to 2 seconds (parity with the VT evaluator).</param>
|
||||
public CalculationEvaluator(
|
||||
ILogger<CalculationEvaluator> logger,
|
||||
ILogger logger,
|
||||
ScriptRootLogger scriptRoot,
|
||||
TimeSpan? runTimeout = null)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user