dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

test(scripting): Evaluate_racing_ClearCompiledScripts no longer assumes a single clear per evaluation

Browse Source 
The racing test encoded two timing assumptions and failed under full-solution
parallel load while passing in isolation. Both are removed; no production code
changed.

1. Assertion shape. failures.ShouldBeEmpty() assumed at most ONE apply-boundary
   clear can land inside a single evaluation, so the evaluator's one-shot retry
   always suffices. Under contention two can land and the retry legitimately
   exhausts. The test now asserts the invariant the retry actually guarantees and
   which is timing-independent: no raw disposed-script failure ESCAPES the retry.
   Every failure must be the documented exhaustion outcome ("evaluator disposed
   during evaluation") and never the pre-fix generic escape ("script threw:
   Cannot access a disposed object..."), plus a deliberately loose bound on the
   exhaustion rate so the check cannot go vacuous.

2. Workload shape. The fixed 12x400 evaluations against a fixed 45s WaitAsync
   budget is CPU-dependent (every clear forces a recompile). Four concurrent
   instances of this class reproduced a hard TimeoutException 4/4 - the likely
   real shape of the reported load failure. The loops are now duration-bounded
   (10s race window or 400 iterations, whichever first); the 45s WaitAsync stays
   purely as a hang detector.

Renamed to Evaluate_racing_ClearCompiledScripts_never_leaks_a_raw_disposed_failure
so the name matches what it now asserts; the old name is cited in the doc-comment
for traceability.
This commit is contained in:
Joseph Doherty
2026-08-18 07:48:44 -04:00
parent df53391646
commit 34829ea0d7
1 changed files with 98 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs
+98 -20
View File
@@ -380,15 +380,74 @@ public sealed class RoslynVirtualTagEvaluatorTests
CompiledCacheCount(sut).ShouldBe(1);
}
/// <summary>02/S12 concurrent stress repro (RED on current code): while one loop runs many compiled
/// (non-passthrough) evaluations of the same expression, a second loop hammers
/// <see cref="RoslynVirtualTagEvaluator.ClearCompiledScripts"/> — the apply-boundary clear. On the
/// unfixed tree a clear landing between a child's <c>GetOrCompile</c> and the run's disposed-guard
/// surfaces an ObjectDisposedException as a <c>Failure("script threw: Cannot access a disposed
/// object…")</c>; the fix retries once and every result stays Success. The whole test is WaitAsync-
/// bounded so a regression fails fast rather than hanging.</summary>
/// <summary>The one reason string the evaluator is documented to surface when its one-shot retry
/// is exhausted (a SECOND apply-boundary clear landing inside the same evaluation). Anything else
/// carrying "disposed" — notably the pre-fix generic escape <c>"script threw: Cannot access a
/// disposed object…"</c> — is a raw ObjectDisposedException leaking past the retry, i.e. the bug.</summary>
private const string RetryExhaustedReason = "evaluator disposed during evaluation";
/// <summary>How long the racing loops below keep hammering the evaluator. A DURATION rather than a
/// fixed iteration count: every clear forces a recompile, so a fixed workload's wall-clock cost tracks
/// whatever CPU the box has left, and a busy box then blows the test's own hang budget.</summary>
private static readonly TimeSpan RaceWindow = TimeSpan.FromSeconds(10);
/// <summary>Upper bound on evaluations per racing loop, so an idle box does bounded work rather than
/// spinning for the whole <see cref="RaceWindow"/>.</summary>
private const int MaxIterationsPerLoop = 400;
/// <summary>02/S12 concurrent stress: while many loops run compiled (non-passthrough) evaluations of
/// the same expression, a second loop hammers <see cref="RoslynVirtualTagEvaluator.ClearCompiledScripts"/>
/// — the apply-boundary clear. A clear landing between a child's <c>GetOrCompile</c> and the run's
/// disposed-guard raises an <see cref="ObjectDisposedException"/> inside the evaluation; on the unfixed
/// tree that escaped as <c>Failure("script threw: Cannot access a disposed object…")</c>, and the fix
/// re-fetches into the current cache generation and retries ONCE. The whole test is WaitAsync-bounded so
/// a regression fails fast rather than hanging.
///
/// <para><b>Shape history — why this is not a zero-failure assertion (renamed 2026-08-18 from
/// <c>Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed</c>).</b> The test previously
/// asserted <c>failures.ShouldBeEmpty()</c>, which silently encoded a TIMING assumption: that at most
/// ONE clear can land inside a single evaluation, so the product's one-shot retry always suffices. The
/// clear loop's spin cadence was tuned to hold that assumption. It does not hold under full-solution
/// parallel test load — with the thread pool saturated the fetch→run window stretches, TWO clears land
/// inside one evaluation, and the retry legitimately exhausts. The test failed under load while passing
/// 3/3 in isolation. That is a test-design defect, not a product defect: exhausting a one-shot retry
/// under a clear cadence no production apply comes close to is a documented, bounded outcome.</para>
///
/// <para>The fix could have re-paced the clears against observed evaluation latency, but that only
/// swaps one timing assumption for a safer one — it stays load-sensitive, and needs machinery to
/// measure latency at runtime. Instead the assertion now targets the invariant the retry actually
/// guarantees, which is timing-independent: <b>no raw disposed-script failure ever ESCAPES the
/// retry</b>. Every failure this race produces must be the documented retry-exhaustion outcome
/// (<see cref="RetryExhaustedReason"/>) and nothing else — a "script threw: Cannot access a disposed
/// object…" (the generic-catch escape) fails the test on the first occurrence, however loaded the box
/// is. A loose upper bound on the exhaustion RATE keeps the assertion non-vacuous: a build whose retry
/// never absorbs anything (every raced evaluation exhausting straight to the documented reason) would
/// pass a shape-only check, so exhaustions must stay a small minority of all evaluations.</para>
///
/// <para><b>The workload is duration-bounded, and that is the SECOND timing assumption removed.</b> The
/// loops used to run a fixed 12 × 400 = 4800 evaluations against a fixed 45 s <c>WaitAsync</c> budget.
/// Every clear forces the next evaluation to recompile (5-20 ms), so the wall-clock cost of that fixed
/// workload is set by how much CPU the box will spare — and running four instances of this test class
/// concurrently on the dev box reproduced a hard failure that has nothing to do with the race:
/// <c>System.TimeoutException</c> at the 45 s budget, 4/4 runs. That is almost certainly the shape the
/// original "fails under full-solution parallel test load" report saw. The loops now stop at
/// <see cref="RaceWindow"/> OR <see cref="MaxIterationsPerLoop"/>, whichever comes first, so a starved
/// box simply lands fewer evaluations instead of blowing a budget; the 45 s <c>WaitAsync</c> is left in
/// place purely as a hang detector, with 4× headroom over the window it now guards.</para>
///
/// <para><b>Calibration of the exhaustion bound (be sceptical of it).</b> The disposed-race did NOT
/// reproduce at all on the arm64 macOS dev box this reshape was written on — zero failures in isolation,
/// zero under concurrently-running Runtime.Tests / Host.Tests / OpcUaServer suites, and still zero with
/// the product's one-shot retry experimentally disabled, i.e. the fetch→run window never opened there
/// and this test was vacuous on that hardware whatever it asserted. So the bound could not be fitted to
/// a measured rate; it is 10% of the evaluations actually performed, hundreds of times looser than the
/// old fire-on-the-first-failure assertion. Exhaustion needs a clear inside BOTH the first attempt's
/// window and the retry's, so its rate goes as the square of the single-window hit rate — 10% of
/// evaluations exhausting implies roughly a third of them hitting a disposed evaluator on the first
/// try, which is a broken retry, not a busy machine. If a loaded box ever does trip THIS bound, the
/// rate itself is the finding: record it before widening the number.</para></summary>
[Fact]
public async Task Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed()
public async Task Evaluate_racing_ClearCompiledScripts_never_leaks_a_raw_disposed_failure()
{
// Generous timeout so the wall-clock budget never fires and confounds the race assertion.
using var sut = new RoslynVirtualTagEvaluator(
@@ -396,29 +455,31 @@ public sealed class RoslynVirtualTagEvaluatorTests
const string expr = "return (int)ctx.GetTag(\"a\").Value + 1;";
const int loopCount = 12;
const int iterations = 400;
var failures = new System.Collections.Concurrent.ConcurrentBag<string>();
var remaining = loopCount; // number of concurrent evaluation loops still running
var evaluated = 0; // evaluations actually performed (the race window may cut loops short)
var remaining = loopCount; // number of concurrent evaluation loops still running
var stopAtMs = Environment.TickCount64 + (long)RaceWindow.TotalMilliseconds;
// Many concurrent evaluation loops put the thread pool under enough pressure that the inner
// Task.Run hop inside TimedScriptEvaluator is measurably delayed — widening the
// fetch→run-disposed-guard window so a clear reliably lands inside SOME in-flight evaluation.
// The clear loop clears on a short spin cadence (not back-to-back) so (a) most evaluations hit
// the warm cache — keeping compile cost bounded — and (b) at most one clear lands within a
// single evaluation, so the single retry the fix adds is sufficient (a back-to-back clear loop
// would both compile-swamp the RED run into a timeout and defeat the one-shot retry). Pre-fix a
// clear disposing the just-fetched evaluator surfaces as Failure("script threw: Cannot access a
// disposed object…"); post-fix the retry re-fetches into the current generation → all Success.
// fetch→run-disposed-guard window so a clear can land inside an in-flight evaluation.
// The clear loop clears on a short spin cadence rather than back-to-back purely as a COST bound:
// most evaluations then hit the warm cache instead of recompiling. Neither the cadence nor the
// iteration count is a correctness knob any more — the assertions below hold for any number of
// clears landing inside a single evaluation, and for any number of evaluations completed.
var evalLoops = Enumerable.Range(0, loopCount).Select(_ => Task.Run(() =>
{
for (var i = 0; i < iterations; i++)
var done = 0;
for (var i = 0; i < MaxIterationsPerLoop && Environment.TickCount64 < stopAtMs; i++)
{
var r = sut.Evaluate("vt-race", expr, new Dictionary<string, object?> { ["a"] = i });
done++;
if (!r.Success)
{
failures.Add(r.Reason ?? "<null reason>");
}
}
Interlocked.Add(ref evaluated, done);
Interlocked.Decrement(ref remaining);
}, TestContext.Current.CancellationToken)).ToArray();
@@ -431,11 +492,28 @@ public sealed class RoslynVirtualTagEvaluatorTests
}
}, TestContext.Current.CancellationToken);
// Hang detector only: the loops self-terminate at RaceWindow, so firing this means something
// wedged, not that the box was slow.
await Task.WhenAll(evalLoops.Append(clearLoop))
.WaitAsync(TimeSpan.FromSeconds(45), TestContext.Current.CancellationToken);
failures.ShouldBeEmpty(
$"every evaluation must succeed despite racing clears; observed: {string.Join(" | ", failures)}");
// (1) The invariant: nothing but the documented retry-exhaustion outcome may surface. A raw
// ObjectDisposedException escaping the retry (pre-fix: "script threw: Cannot access a
// disposed object…") lands here, as does any other unexpected failure mode.
var leaked = failures.Where(reason => reason != RetryExhaustedReason).ToArray();
leaked.ShouldBeEmpty(
$"a racing ClearCompiledScripts must never surface anything but \"{RetryExhaustedReason}\"; " +
$"observed: {string.Join(" | ", leaked)}");
// (2) Non-vacuity: the retry must actually be absorbing the race, not merely relabelling it.
// Loose by design (see the doc-comment) — a broken-retry detector, not a timing gate. The
// absolute floor keeps a starved run (few evaluations landed) from tripping on a single one.
var exhausted = failures.Count;
var totalEvaluations = Volatile.Read(ref evaluated);
exhausted.ShouldBeLessThan(
Math.Max(totalEvaluations / 10, 8),
$"the one-shot retry must absorb the overwhelming majority of raced clears; " +
$"{exhausted}/{totalEvaluations} evaluations exhausted it");
}
/// <summary>Reads the count of the private compiled-script cache via reflection. The cache is a