From 5c8d6f6f98ab28a0531175263659f8f3991fd7ba Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:07:27 -0400 Subject: [PATCH] test(calc): make the not-evaluated-inline assertion exact, not thread-id based The first form compared the publish's managed thread id with the caller's, which flaked (1 in 6): the evaluation pump may reuse the pooled thread the test released while awaiting the drain, so an id match proves nothing. It now sets a ThreadLocal flag for exactly the OnDependencyValue window, so 'published inline' is answered on the stack rather than by thread identity. Re-falsified by making SchedulePump run the pump synchronously (red immediately, reverted); 5 consecutive clean suite runs after. --- code-reviews/Driver.Calculation/findings.md | 8 +++++-- .../CalculationDriverTests.cs | 21 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/code-reviews/Driver.Calculation/findings.md b/code-reviews/Driver.Calculation/findings.md index 3269936f..0f9ec50c 100644 --- a/code-reviews/Driver.Calculation/findings.md +++ b/code-reviews/Driver.Calculation/findings.md @@ -262,11 +262,15 @@ run so a tag dropped mid-evaluation discards its result rather than publishing i RED first, two tests, both failing against the pre-fix code: `A_dependency_arrival_does_not_run_the_script_on_the_callers_thread` (the publish — which can only happen -after the script ran — landed on the caller's own thread id) and +after the script ran — landed on the caller's own stack, inside the callback) and `A_slow_script_blocks_neither_the_dependency_callback_nor_a_concurrent_read` (a 600 ms busy-wait script: the callback took 1.21 s, and `ReadAsync`, which needs the same state lock, was stuck behind it). Both carry a positive control asserting the value still computes and publishes, so neither can pass against a -driver that simply evaluates nothing. +driver that simply evaluates nothing. ⚠️ The first test's original form compared the publish's **thread id** +against the caller's and was itself flaky (1 failure in 6 runs): the pump is free to reuse the pooled thread +the test released while awaiting, so an id match means nothing. It asserts on a thread-local flag set for +exactly the callback window instead — which is exact, and was re-falsified by making `SchedulePump` run the +pump synchronously (red immediately, then reverted). Known trade, documented rather than hidden: the pending queue is unbounded. Under sustained arrival pressure the previous inline path applied backpressure by blocking the mux actor — which is the defect — diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs index 5f9a9160..35945878 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs @@ -118,28 +118,33 @@ public sealed class CalculationDriverTests /// DependencyConsumerMuxAdapter.OnDependencyValueChanged — i.e. on an Akka actor's own /// dispatcher thread. Evaluating there means a script run (bounded only by RunTimeout, /// default 2 s) occupies that thread, so the callback must queue the work and return instead. - /// The publish is the observable: it can only happen after the script ran, so a publish on - /// the caller's own thread proves the script ran inline. The value assertion is the positive - /// control — without it this would pass just as happily against a driver that evaluates nothing. + /// The publish is the observable: it can only happen after the script ran, so a publish that + /// lands on the caller's stack between entering and leaving the callback proves the script + /// ran inline. A thread-local flag set for exactly that window is the exact test — comparing thread + /// ids is not, because the pump is free to reuse the pooled thread the test released while + /// waiting, which makes an id match mean nothing. The value assertion is the positive control: + /// without it this would pass just as happily against a driver that evaluates nothing at all. /// [Fact] public async Task A_dependency_arrival_does_not_run_the_script_on_the_callers_thread() { var (driver, published, _) = Build( CalcTag("calc/e/dbl", "return (double)ctx.GetTag(\"src/a\").Value * 2.0;")); - var publishThreadId = 0; - driver.OnDataChange += (_, _) => publishThreadId = Environment.CurrentManagedThreadId; + using var insideCallback = new ThreadLocal(); + var publishedInline = false; + driver.OnDataChange += (_, _) => { if (insideCallback.Value) publishedInline = true; }; await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); - var callerThreadId = Environment.CurrentManagedThreadId; + insideCallback.Value = true; driver.OnDependencyValue("src/a", 5.0, StatusGood, DateTime.UtcNow); + insideCallback.Value = false; - (await WaitUntil(() => !published.IsEmpty, TimeSpan.FromSeconds(5))) + (await WaitUntil(() => !published.IsEmpty, TimeSpan.FromSeconds(10))) .ShouldBeTrue("the queued evaluation must still compute and publish the value"); published.TryDequeue(out var e).ShouldBeTrue(); e!.Snapshot.Value.ShouldBe(10.0); - publishThreadId.ShouldNotBe(callerThreadId, + publishedInline.ShouldBeFalse( "the calc script must run on the driver's own evaluation pump, never on the mux callback thread"); }