diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index 0a6f4e54..bcfb0714 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -20,23 +20,35 @@ public class InboundScriptExecutor { private readonly ILogger _logger; + // A cached compiled handler, paired with the exact script text it was + // compiled from. The DB row is authoritative: on every request the cached + // Script is ordinal-compared against the freshly-fetched ApiMethod.Script; a + // mismatch means the cache is stale (a standby node compiled its own copy at + // startup, or the row was edited via direct SQL / bundle import) and the + // handler is recompiled in place. A null Script means "no revision info, + // serve as-is" — the RegisterHandler test seam, exempt from revision checks. + private sealed record CachedHandler(string? Script, Func> Handler); + // This executor is registered as a singleton and its handler cache // is read and written from concurrent ASP.NET request threads. A plain Dictionary is // not safe for concurrent read/write, so a ConcurrentDictionary is used throughout. - private readonly ConcurrentDictionary>> _scriptHandlers = new(); + private readonly ConcurrentDictionary _scriptHandlers = new(); // A script that fails to compile (or violates the trust model) - // is recorded here so it is compiled at most once. Without this, every subsequent + // is recorded here — keyed by method name, valued by the exact script text + // that failed — so it is compiled at most once. Without this, every subsequent // request for a broken method re-runs the expensive Roslyn compilation — a CPU - // amplification vector since the inbound API has no rate limiting. The entry is - // cleared whenever the method is (re)compiled via CompileAndRegister. + // amplification vector since the inbound API has no rate limiting. A record only + // short-circuits when the CURRENT script text is the one that failed: a changed + // script always gets a fresh compile. The entry is cleared whenever the method + // is (re)compiled successfully. // // Bound the cache so a spam attack of unique method names cannot // grow it without bound. Once the cap is reached new bad-method records are // dropped — the cache is just a fast-fail optimisation; the per-request DB // lookup remains the correctness path. private const int KnownBadMethodsCap = 1000; - private readonly ConcurrentDictionary _knownBadMethods = new(); + private readonly ConcurrentDictionary _knownBadMethods = new(); /// /// Diagnostic helper — returns the current size of the @@ -46,20 +58,27 @@ public class InboundScriptExecutor internal int KnownBadMethodCount => _knownBadMethods.Count; /// - /// Records in the known-bad-methods - /// cache only if the cache has not reached . - /// Once full, new records are dropped (paying the cheap recompile next time - /// rather than leaking memory under a unique-name flood). Existing entries are - /// not touched — they remain capped fast-fail records until cleared on a - /// successful (re)compile in . + /// Records → in the + /// known-bad-methods cache so the CURRENT failing script text short-circuits the + /// next request. An existing record for the method is overwritten so a newer bad + /// script replaces an older one (the fast-fail always tracks the latest failure). + /// A brand-new record is only added while the cache has not reached + /// ; once full, new method names are dropped + /// (paying the cheap recompile next time rather than leaking memory under a + /// unique-name flood). Records are cleared on a successful (re)compile. /// - private void TryRecordBadMethod(string methodName) + private void TryRecordBadMethod(string methodName, string script) { + // Overwrite an existing record (newer bad script wins) without counting + // against the cap; only a genuinely new method name is gated by the cap. if (_knownBadMethods.ContainsKey(methodName)) + { + _knownBadMethods[methodName] = script; return; + } if (_knownBadMethods.Count >= KnownBadMethodsCap) return; - _knownBadMethods.TryAdd(methodName, 0); + _knownBadMethods.TryAdd(methodName, script); } private readonly IServiceProvider _serviceProvider; @@ -76,13 +95,18 @@ public class InboundScriptExecutor } /// - /// Registers a compiled script handler for a method name. + /// Registers a compiled script handler for a method name. This is the script-less + /// test seam: the handler is stored with no associated script text, so it is + /// exempt from the per-request revision check and serves as-is regardless of + /// the fetched row's script. Production compiles always flow through + /// , which records the script text and is + /// revision-checked. /// /// The method name to register. /// The compiled handler function. public void RegisterHandler(string methodName, Func> handler) { - _scriptHandlers[methodName] = handler; + _scriptHandlers[methodName] = new CachedHandler(null, handler); } /// @@ -106,11 +130,14 @@ public class InboundScriptExecutor /// Compiles and registers a single API method script, additionally reporting the /// compilation diagnostics. Returns false together with the human-readable /// error list if the script is empty, fails Roslyn compilation, or violates the - /// script trust model. On failure the previously registered handler (if any) is - /// left in place — a broken save never replaces a working delegate — so the - /// management Create/Update path can surface a non-fatal "saved but did not compile" - /// warning while the prior version keeps serving requests (see - /// HandleCreateApiMethod/HandleUpdateApiMethod). + /// script trust model. On failure this method does not overwrite any previously + /// registered handler, but the persisted DB row is authoritative: + /// revision-checks the cached handler against the freshly-fetched + /// ApiMethod.Script on every request, so once a broken script is saved the + /// method returns a compilation error consistently on every node rather than the + /// stale delegate silently serving. The management Create/Update path can therefore + /// surface a "saved but did not compile" warning that reflects real, consistent + /// behavior (see HandleCreateApiMethod/HandleUpdateApiMethod). /// /// The API method to compile and register. /// On failure, the compilation/trust-model error messages; empty on success. @@ -125,7 +152,7 @@ public class InboundScriptExecutor // through the capped TryRecordBadMethod helper so the cache // cannot grow without bound under a flood of unique method names. errors = compileErrors; - TryRecordBadMethod(method.Name); + TryRecordBadMethod(method.Name, method.Script ?? string.Empty); return false; } @@ -133,12 +160,12 @@ public class InboundScriptExecutor // failure record so a fixed script is no longer treated as bad. errors = Array.Empty(); _knownBadMethods.TryRemove(method.Name, out _); - return Register(method.Name, handler); + return Register(method, handler); } - private bool Register(string methodName, Func> handler) + private bool Register(ApiMethod method, Func> handler) { - _scriptHandlers[methodName] = handler; + _scriptHandlers[method.Name] = new CachedHandler(method.Script, handler); return true; } @@ -308,29 +335,43 @@ public class InboundScriptExecutor dbHelper, cts.Token); - if (!_scriptHandlers.TryGetValue(method.Name, out var handler)) + _scriptHandlers.TryGetValue(method.Name, out var cached); + // Revision check: a cached handler compiled from different script text is stale + // (standby-after-failover, direct SQL edit, bundle import). Null Script = test seam, exempt. + if (cached is { Script: not null } + && !string.Equals(cached.Script, method.Script, StringComparison.Ordinal)) { - // A method already known to fail compilation must not - // be recompiled on every request — short-circuit before Roslyn runs. - if (_knownBadMethods.ContainsKey(method.Name)) + _logger.LogInformation( + "API method {Method} script changed since it was cached; recompiling", method.Name); + cached = null; + } + + if (cached == null) + { + // Fast-fail only when the CURRENT script text is the one already known bad. + if (_knownBadMethods.TryGetValue(method.Name, out var badScript) + && string.Equals(badScript, method.Script, StringComparison.Ordinal)) return new InboundScriptResult(false, null, "Script compilation failed for this method"); - // Lazy compile on first request (handles methods created after startup). - // Compile outside the cache so a failed compile is not stored, then add - // atomically so concurrent first-callers share a single handler instance. + // Lazy compile (handles methods created after startup, and stale/known-bad + // recompiles). Compile outside the cache so a failed compile is not stored, + // then add so concurrent first-callers share a single handler instance. var (compiled, _) = Compile(method); if (compiled == null) { // Cache the failure so the next request short-circuits above. // Routed through TryRecordBadMethod so the // cache is bounded under a flood of unique method names. - TryRecordBadMethod(method.Name); + TryRecordBadMethod(method.Name, method.Script ?? string.Empty); return new InboundScriptResult(false, null, "Script compilation failed for this method"); } - handler = _scriptHandlers.GetOrAdd(method.Name, compiled); + + _knownBadMethods.TryRemove(method.Name, out _); + cached = new CachedHandler(method.Script, compiled); + _scriptHandlers[method.Name] = cached; // last-write-wins; concurrent first-callers race benignly } - var result = await handler(context).WaitAsync(cts.Token); + var result = await cached.Handler(context).WaitAsync(cts.Token); var resultJson = result != null ? JsonSerializer.Serialize(result) diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs new file mode 100644 index 00000000..3f39c481 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs @@ -0,0 +1,89 @@ +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; + +namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; + +/// +/// Arch-review S1: the compiled-handler cache must be revision-checked against the +/// freshly-fetched ApiMethod.Script so a standby node (whose cache was populated at +/// its own startup) never serves a stale delegate after failover, and a method fixed +/// out-of-band is never stuck behind a stale known-bad record. +/// +public class InboundScriptExecutorStalenessTests +{ + private readonly InboundScriptExecutor _executor; + private readonly RouteHelper _route; + + public InboundScriptExecutorStalenessTests() + { + _executor = new InboundScriptExecutor( + NullLogger.Instance, Substitute.For()); + _route = new RouteHelper( + Substitute.For(), Substitute.For()); + } + + private Task Run(ApiMethod m) => _executor.ExecuteAsync( + m, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + + [Fact] + public async Task UpdatedScript_RecompilesInsteadOfServingStaleHandler() + { + // Simulates the failover scenario: this node compiled v1 at startup... + var v1 = new ApiMethod("stale", "return 1;") { Id = 1, TimeoutSeconds = 10 }; + Assert.True(_executor.CompileAndRegister(v1)); + var r1 = await Run(v1); + Assert.Contains("1", r1.ResultJson); + + // ...the active node updated the method (this node never heard about it); + // the per-request DB fetch hands ExecuteAsync the NEW row. + var v2 = new ApiMethod("stale", "return 2;") { Id = 1, TimeoutSeconds = 10 }; + var r2 = await Run(v2); + + Assert.True(r2.Success); + Assert.Contains("2", r2.ResultJson); // old cache would have returned 1 + } + + [Fact] + public async Task KnownBadMethod_FixedScript_CompilesInsteadOfFastFailing() + { + // The mirror-image variant: broken at this node's startup... + var broken = new ApiMethod("fixme", "%%% not C# %%%") { Id = 2, TimeoutSeconds = 10 }; + Assert.False(_executor.CompileAndRegister(broken)); + var r1 = await Run(broken); + Assert.False(r1.Success); + + // ...fixed via the management path on the OTHER node; this node's fresh + // DB fetch carries the fixed script. + var fixedMethod = new ApiMethod("fixme", "return 42;") { Id = 2, TimeoutSeconds = 10 }; + var r2 = await Run(fixedMethod); + + Assert.True(r2.Success); + Assert.Contains("42", r2.ResultJson); + } + + [Fact] + public async Task KnownBadMethod_SameScript_StillFastFails() + { + var broken = new ApiMethod("stillbad", "%%% not C# %%%") { Id = 3, TimeoutSeconds = 10 }; + Assert.False(_executor.CompileAndRegister(broken)); + + // Same script text → the fast-fail record must hold (no per-request Roslyn). + var again = new ApiMethod("stillbad", "%%% not C# %%%") { Id = 3, TimeoutSeconds = 10 }; + var r = await Run(again); + Assert.False(r.Success); + Assert.Contains("Script compilation failed", r.ErrorMessage); + } + + [Fact] + public async Task TestSeamRegisterHandler_WithoutScript_IsNotRevisionChecked() + { + // RegisterHandler (script-less test seam) pins the handler regardless of row content. + var m = new ApiMethod("pinned", "return 0;") { Id = 4, TimeoutSeconds = 10 }; + _executor.RegisterHandler("pinned", async _ => { await Task.CompletedTask; return 99; }); + var r = await Run(m); + Assert.True(r.Success); + Assert.Contains("99", r.ResultJson); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index 527d31b8..7e5d0c08 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -271,12 +271,15 @@ public class InboundScriptExecutorTests } [Fact] - public async Task CompileAndRegister_NonCompilingUpdate_KeepsPreviousHandler() + public async Task CompileAndRegister_NonCompilingUpdate_ExecuteSurfacesCompileFailure() { - // Register a working handler, then "update" the same method name with a script - // that does not compile. The broken save must NOT replace the working delegate — - // the previously registered version keeps serving requests (this is exactly the - // behaviour that makes a broken save non-fatal while the warning is surfaced). + // Arch-review S1 (DB-authoritative): CompileAndRegister still leaves the old + // handler cached on a broken save (it never overwrites with a bad compile), but + // ExecuteAsync now revision-checks the cached script against the freshly-fetched + // row. A request carrying the broken row no longer silently serves the stale good + // delegate — the mismatch triggers a recompile, which fails, so the method returns + // a compilation error consistently on every node (honest failure over node-divergent + // stale success). var good = new ApiMethod("evolving", "return 111;") { Id = 1, TimeoutSeconds = 10 }; Assert.True(_executor.CompileAndRegister(good, out var goodErrors)); Assert.Empty(goodErrors); @@ -285,12 +288,19 @@ public class InboundScriptExecutorTests Assert.False(_executor.CompileAndRegister(broken, out var brokenErrors)); Assert.NotEmpty(brokenErrors); - // The old (good) handler still serves — the broken script never went live. + // The DB row is authoritative: the broken script now 500s rather than the old + // delegate silently serving. var result = await _executor.ExecuteAsync( broken, new Dictionary(), _route, TimeSpan.FromSeconds(10)); - Assert.True(result.Success); - Assert.Contains("111", result.ResultJson); + Assert.False(result.Success); + Assert.Contains("Script compilation failed", result.ErrorMessage); + + // The still-valid row keeps serving — only the broken revision fails. + var stillGood = await _executor.ExecuteAsync( + good, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + Assert.True(stillGood.Success); + Assert.Contains("111", stillGood.ResultJson); } // --- InboundAPI-002: lazy compile-and-fetch must be atomic, never KeyNotFoundException ---