From 17d5d3e639135404760795e6e8f5ba2bef30ab84 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 10 Sep 2026 15:35:50 -0400 Subject: [PATCH] fix(scripts): reset-aware handshake refuses a null reset value; document additive timeouts (#36 review) A null resetValue encoded to null, which wrote an empty string and then handed the waiter a null target (= any change), fast-pathing on the stale cache and reinstating the race the overload exists to close. The context parameter is now non-nullable and the accessor throws ArgumentException before any write. Also: additive-timeout note on all surfaces, batch-failure message no longer names the bool overload, and the error-message/batch-failure branches are pinned. Claude-Session: https://claude.ai/code/session_01XPsuy2ZZ5Gbyfpt8PrK5BV --- .../2026-09-10-handshake-reset-echo-design.md | 5 ++ .../ScriptAnalysis/SandboxScriptHost.cs | 6 +- .../ScriptCompileSurface.cs | 6 +- .../Scripts/ScopeAccessors.cs | 18 +++++- .../Scripts/ScriptRuntimeContext.cs | 19 ++++-- .../Actors/HandshakeResetEchoTests.cs | 4 +- .../Scripts/ScopeAccessorTests.cs | 29 +++++++++ .../Scripts/WriteBatchResetAndWaitTests.cs | 63 ++++++++++++++++++- 8 files changed, 138 insertions(+), 12 deletions(-) diff --git a/docs/plans/2026-09-10-handshake-reset-echo-design.md b/docs/plans/2026-09-10-handshake-reset-echo-design.md index 3ccff0f0..19998577 100644 --- a/docs/plans/2026-09-10-handshake-reset-echo-design.md +++ b/docs/plans/2026-09-10-handshake-reset-echo-design.md @@ -126,6 +126,11 @@ if (!hs.Matched) arrives after this run's reset echo can still satisfy the response wait. A level-triggered flag protocol cannot distinguish that case; it needs a sequence/handle attribute on the device side. +**The two waits are additive.** A call can run for `resetTimeout` + `timeout` plus Ask slack, so a +script using the overload must set `ExecutionTimeoutSeconds` above that sum. A script deadline hit +part-way through either leg propagates as cancellation (§4.3) — the script sees the cancellation, +not a `HandshakeResult` naming the phase it died in. + ## 6. Mirrors and analysis surfaces - `ScriptCompileSurface` (ScriptAnalysis) — compile-only mirror of the new overload diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs index 08335030..d07eca49 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs @@ -476,7 +476,11 @@ public class SandboxAttributeAccessor /// The response value to wait for. /// The maximum time to wait for the response. /// The attribute written first and waited on before the batch. - /// The reset value the device must echo before the batch is written. + /// + /// The reset value the device must echo before the batch is written. MUST NOT be null — the + /// real accessor throws , because a null would write an + /// empty string and then wait on "any change", reinstating the race the overload closes. + /// /// The maximum time to wait for the reset echo; null means . /// Never returns; always throws in the sandbox. public Task WriteBatchAndWaitAsync( diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptCompileSurface.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptCompileSurface.cs index acce7a3c..128e0d88 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptCompileSurface.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptCompileSurface.cs @@ -341,7 +341,11 @@ public sealed class ScriptCompileSurface /// The value to wait for on the response attribute. /// The maximum time to wait for the response. /// The attribute written first and waited on before the batch. - /// The reset value the device must echo before the batch is written. + /// + /// The reset value the device must echo before the batch is written. MUST NOT be null — the + /// real accessor throws , because a null would write an + /// empty string and then wait on "any change", reinstating the race the overload closes. + /// /// The maximum time to wait for the reset echo; null means . /// The phase-aware handshake outcome. public Task WriteBatchAndWaitAsync(IReadOnlyDictionary values, string flagKey, object? flagValue, string responseKey, object? responseValue, TimeSpan timeout, string resetKey, object? resetValue, TimeSpan? resetTimeout = null) => throw new NotSupportedException(CompileOnly); diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScopeAccessors.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScopeAccessors.cs index c581a5ec..7af567ab 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScopeAccessors.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScopeAccessors.cs @@ -122,7 +122,10 @@ public class AttributeAccessor /// flag handshake: without an observed reset the plain overload can be satisfied /// instantly by the previous run's cached response value. Throws /// if the reset write or the batch/trigger - /// write itself fails; never throws on either timeout. + /// write itself fails; never throws on either timeout. The two waits are ADDITIVE — the call + /// can run for + plus Ask slack, so + /// size the script's ExecutionTimeoutSeconds above that sum: a deadline hit mid-leg + /// propagates as cancellation (spec §4.3), not as a . /// /// Attribute key → value to batch-write (data-sourced, one connection). /// Trigger attribute key written AFTER the batch. @@ -131,7 +134,12 @@ public class AttributeAccessor /// Target value to wait for (null ⇒ any change). /// How long to wait for the response before giving up. /// Attribute written FIRST (usually ); static or data-sourced. - /// Value the device must echo back before the batch is written. + /// + /// Value the device must echo back before the batch is written. MUST NOT be null: a null encodes + /// to null, which would write an empty string and then wait on "any change", fast-pathing on the + /// stale cached value and reinstating the race this overload exists to close — + /// is thrown before anything is written. + /// /// How long to wait for the reset echo; null ⇒ . /// The phase-aware handshake outcome. public Task WriteBatchAndWaitAsync( @@ -148,7 +156,11 @@ public class AttributeAccessor Resolve(flagKey), AttributeValueCodec.Encode(flagValue), Resolve(responseKey), AttributeValueCodec.Encode(responseValue), timeout, - Resolve(resetKey), AttributeValueCodec.Encode(resetValue), + Resolve(resetKey), + AttributeValueCodec.Encode(resetValue) + ?? throw new ArgumentException( + "A reset-aware handshake needs a definite reset value: null would write an " + + "empty string and then match on any change.", nameof(resetValue)), resetTimeout ?? timeout); } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs index da9f817f..1e2c5128 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs @@ -985,6 +985,12 @@ public class ScriptRuntimeContext /// would never resolve at all. Holding the trigger until the echo lands makes the existing /// level-triggered fast-path correct again. Each leg is already a traced Flow Debugger /// step, so a traced run shows all four. + /// + /// The two waits are ADDITIVE: the call can run for + + /// plus Ask slack, so size the script's + /// ExecutionTimeoutSeconds above that sum — a script deadline hit mid-leg propagates + /// as cancellation (spec §4.3), not as a . + /// /// /// Scope-resolved attribute name → codec-encoded value to batch-write. /// Scope-resolved trigger attribute written AFTER the batch. @@ -993,17 +999,22 @@ public class ScriptRuntimeContext /// Codec-encoded response target (null ⇒ any change). /// Bound for the response wait. /// Scope-resolved attribute written FIRST (usually the response attribute). - /// Codec-encoded reset value the device must echo before anything else is written. + /// + /// Codec-encoded reset value the device must echo before anything else is written. NON-NULL by + /// contract: a null would write an empty string on leg 1 and then give the leg-2 waiter a null + /// target — "any change" — which fast-paths on the stale cached value and reinstates the very + /// race this overload exists to close. The accessor rejects a null reset value before it gets here. + /// /// Bound for the reset-echo wait. /// A phase-aware result; never throws on either timeout. public async Task WriteBatchResetAndWait( IReadOnlyDictionary encodedValues, string triggerAttr, string? triggerEncoded, string responseAttr, string? responseEncoded, TimeSpan timeout, - string resetAttr, string? resetEncoded, TimeSpan resetTimeout) + string resetAttr, string resetEncoded, TimeSpan resetTimeout) { // Leg 1 — reset write. A static attribute updates the cache synchronously (leg 2 then // fast-paths); a data-sourced one goes to the device and the cache waits for the echo. - await SetAttribute(resetAttr, resetEncoded ?? string.Empty).ConfigureAwait(false); + await SetAttribute(resetAttr, resetEncoded).ConfigureAwait(false); // Leg 2 — reset echo. Quality-agnostic, like the response wait. var reset = await WaitInternal(resetAttr, resetEncoded, null, resetTimeout, requireGoodQuality: false) @@ -1084,7 +1095,7 @@ public class ScriptRuntimeContext if (!batchResp.Success) throw new InvalidOperationException( - $"WriteBatchAndWait write failed: {batchResp.ErrorMessage}"); + $"Batch write failed: {batchResp.ErrorMessage}"); return await WaitInternal(responseAttr, responseEncoded, null, timeout, requireGoodQuality: false) .ConfigureAwait(false); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/HandshakeResetEchoTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/HandshakeResetEchoTests.cs index 173ffa43..41110774 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/HandshakeResetEchoTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/HandshakeResetEchoTests.cs @@ -109,7 +109,7 @@ public class HandshakeResetEchoTests : TestKit, IDisposable var call = ctx.WriteBatchResetAndWait( Values(28950), "MoveOutFlag", AttributeValueCodec.Encode(true), "MoveOutCompleteFlag", AttributeValueCodec.Encode(true), TimeSpan.FromSeconds(10), - "MoveOutCompleteFlag", AttributeValueCodec.Encode(false), TimeSpan.FromSeconds(10)); + "MoveOutCompleteFlag", AttributeValueCodec.Encode(false)!, TimeSpan.FromSeconds(10)); // Leg 1 reaches the device as a plain tag write; the DCL acks it but the device has // not echoed anything yet. @@ -152,7 +152,7 @@ public class HandshakeResetEchoTests : TestKit, IDisposable var call = ctx.WriteBatchResetAndWait( Values(28950), "MoveOutFlag", AttributeValueCodec.Encode(true), "MoveOutCompleteFlag", AttributeValueCodec.Encode(true), TimeSpan.FromSeconds(10), - "MoveOutCompleteFlag", AttributeValueCodec.Encode(false), TimeSpan.FromMilliseconds(700)); + "MoveOutCompleteFlag", AttributeValueCodec.Encode(false)!, TimeSpan.FromMilliseconds(700)); var reset = dcl.ExpectMsg(TimeSpan.FromSeconds(5)); dcl.Reply(new WriteTagResponse(reset.CorrelationId, true, null, DateTimeOffset.UtcNow)); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScopeAccessorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScopeAccessorTests.cs index 03b9846d..22e23994 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScopeAccessorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScopeAccessorTests.cs @@ -341,4 +341,33 @@ public class AttributeAccessorWaitAsyncTests : TestKit, IDisposable // Nothing else was written: no batch, no trigger. probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); } + + /// + /// Gitea #36 review: a null resetValue would encode to null, which writes an empty + /// string on leg 1 and then hands the waiter a null target — "any change" — which fast-paths + /// on the stale cached value and reinstates exactly the race this overload exists to close. + /// The accessor rejects it before anything reaches the InstanceActor. + /// + [Fact] + public void WriteBatchAndWaitAsync_ResetAware_NullResetValue_ThrowsBeforeAnyWrite() + { + var probe = CreateTestProbe(); + var ctx = MakeContext(probe.Ref); + + var acc = new AttributeAccessor(ctx, "RightMESReceiver"); + + // Block body (not an expression lambda) so this binds the Action overload: the throw is + // SYNCHRONOUS — a faulted task would leave nothing for Assert.Throws to catch. + var ex = Assert.Throws(() => + { + _ = acc.WriteBatchAndWaitAsync( + new Dictionary { ["MoveOutBatchID"] = 28950 }, + "MoveOutFlag", true, "MoveOutCompleteFlag", true, TimeSpan.FromSeconds(25), + "MoveOutCompleteFlag", null); + }); + Assert.Equal("resetValue", ex.ParamName); + + // Thrown before leg 1: the reset write never left the accessor. + probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/WriteBatchResetAndWaitTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/WriteBatchResetAndWaitTests.cs index b206ae24..334d893f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/WriteBatchResetAndWaitTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/WriteBatchResetAndWaitTests.cs @@ -24,7 +24,10 @@ public class WriteBatchResetAndWaitTests : TestKit new SharedScriptLibrary( new ScriptCompilationService(NullLogger.Instance), NullLogger.Instance), - currentCallDepth: 0, maxCallDepth: 10, askTimeout: TimeSpan.FromSeconds(5), + // 30s: the legs Ask the InstanceActor with their own bounds (the batch's internal 35s, + // the waits' own timeouts); a short ambient Ask timeout would race those and make the + // reset/response phases indistinguishable from an AskTimeoutException. + currentCallDepth: 0, maxCallDepth: 10, askTimeout: TimeSpan.FromSeconds(30), instanceName: Instance, logger: NullLogger.Instance); [Fact] @@ -125,6 +128,64 @@ public class WriteBatchResetAndWaitTests : TestKit Assert.True(result.TimedOut); Assert.Equal(HandshakePhase.Response, result.Phase); Assert.False(result.ResetFailed); + // The message names the attribute that did not answer and the bound it blew. + Assert.Contains("MoveOutCompleteFlag", result.ErrorMessage); + Assert.Contains("25", result.ErrorMessage); + } + + /// + /// A reset wait that fails WITHOUT timing out (a predicate blew up in the waiter) surfaces the + /// actor's own message verbatim rather than a synthesized one, and still writes nothing further. + /// + [Fact] + public async Task ResetWaitError_PassesTheActorsMessageThrough() + { + var probe = CreateTestProbe(); + var ctx = CreateContext(probe.Ref); + + var call = ctx.WriteBatchResetAndWait( + Values, "MoveOutFlag", "True", "MoveOutCompleteFlag", "True", TimeSpan.FromSeconds(25), + "MoveOutCompleteFlag", "False", TimeSpan.FromSeconds(10)); + + var reset = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new SetStaticAttributeResponse(reset.CorrelationId, Instance, "MoveOutCompleteFlag", true, null, DateTimeOffset.UtcNow)); + var resetWait = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new WaitForAttributeResponse( + resetWait.CorrelationId, Matched: false, null, null, TimedOut: false, ErrorMessage: "predicate threw")); + + var result = await call; + Assert.False(result.Matched); + Assert.False(result.TimedOut); + Assert.True(result.ResetFailed); + Assert.Equal("predicate threw", result.ErrorMessage); + + probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + } + + /// + /// The batch leg keeps throwing after a successful reset — a device-rejected batch is a hard + /// failure, not a the script could mistake for a timeout. + /// + [Fact] + public async Task BatchFailureAfterASuccessfulReset_StillThrows() + { + var probe = CreateTestProbe(); + var ctx = CreateContext(probe.Ref); + + var call = ctx.WriteBatchResetAndWait( + Values, "MoveOutFlag", "True", "MoveOutCompleteFlag", "True", TimeSpan.FromSeconds(25), + "MoveOutCompleteFlag", "False", TimeSpan.FromSeconds(10)); + + var reset = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new SetStaticAttributeResponse(reset.CorrelationId, Instance, "MoveOutCompleteFlag", true, null, DateTimeOffset.UtcNow)); + var resetWait = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new WaitForAttributeResponse(resetWait.CorrelationId, Matched: true, false, "Good", TimedOut: false)); + var batch = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new WriteAttributeBatchResponse(batch.CorrelationId, false, "gateway rejected")); + + var ex = await Assert.ThrowsAsync(() => call); + Assert.Contains("gateway rejected", ex.Message); + Assert.Contains("Batch write failed", ex.Message); } [Fact]