fix(inbound-api): revision-check compiled handler cache against the fresh ApiMethod row — heals failover/direct-SQL/known-bad staleness

Also updates the now-obsolete CompileAndRegister_NonCompilingUpdate test
(InboundScriptExecutorTests.cs, not in the plan's Files list) to assert the
deliberate DB-authoritative behavior: a broken save now 500s consistently on
every node instead of the stale delegate silently serving.

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj

src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs

@@ -20,23 +20,35 @@ public class InboundScriptExecutor
 {
     private readonly ILogger<InboundScriptExecutor> _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<InboundScriptContext, Task<object?>> 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<string, Func<InboundScriptContext, Task<object?>>> _scriptHandlers = new();
+    private readonly ConcurrentDictionary<string, CachedHandler> _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
+    // amplification vector since the inbound API has no rate limiting. A record only
-    // cleared whenever the method is (re)compiled via CompileAndRegister.
+    // 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<string, byte> _knownBadMethods = new();
+    private readonly ConcurrentDictionary<string, string> _knownBadMethods = new();
 
     /// <summary>
     /// Diagnostic helper — returns the current size of the
@@ -46,20 +58,27 @@ public class InboundScriptExecutor
     internal int KnownBadMethodCount => _knownBadMethods.Count;
 
     /// <summary>
-    /// Records <paramref name="methodName"/> in the known-bad-methods
+    /// Records <paramref name="methodName"/> → <paramref name="script"/> in the
-    /// cache only if the cache has not reached <see cref="KnownBadMethodsCap"/>.
+    /// known-bad-methods cache so the CURRENT failing script text short-circuits the
-    /// Once full, new records are dropped (paying the cheap recompile next time
+    /// next request. An existing record for the method is overwritten so a newer bad
-    /// rather than leaking memory under a unique-name flood). Existing entries are
+    /// script replaces an older one (the fast-fail always tracks the latest failure).
-    /// not touched — they remain capped fast-fail records until cleared on a
+    /// A brand-new record is only added while the cache has not reached
-    /// successful (re)compile in <see cref="CompileAndRegister"/>.
+    /// <see cref="KnownBadMethodsCap"/>; 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.
     /// </summary>
-    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
     }
 
     /// <summary>
-    /// 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
+    /// <b>exempt from the per-request revision check</b> and serves as-is regardless of
+    /// the fetched row's script. Production compiles always flow through
+    /// <see cref="CompileAndRegister(ApiMethod)"/>, which records the script text and is
+    /// revision-checked.
     /// </summary>
     /// <param name="methodName">The method name to register.</param>
     /// <param name="handler">The compiled handler function.</param>
     public void RegisterHandler(string methodName, Func<InboundScriptContext, Task<object?>> handler)
     {
-        _scriptHandlers[methodName] = handler;
+        _scriptHandlers[methodName] = new CachedHandler(null, handler);
     }
 
     /// <summary>
@@ -106,11 +130,14 @@ public class InboundScriptExecutor
     /// Compiles and registers a single API method script, additionally reporting the
     /// compilation diagnostics. Returns <c>false</c> together with the human-readable
     /// error list if the script is empty, fails Roslyn compilation, or violates the
-    /// script trust model. On failure the <b>previously registered handler (if any) is
+    /// script trust model. On failure this method does not overwrite any previously
-    /// left in place</b> — a broken save never replaces a working delegate — so the
+    /// registered handler, but the persisted DB row is authoritative: <see cref="ExecuteAsync"/>
-    /// management Create/Update path can surface a non-fatal "saved but did not compile"
+    /// revision-checks the cached handler against the freshly-fetched
-    /// warning while the prior version keeps serving requests (see
+    /// <c>ApiMethod.Script</c> on every request, so once a broken script is saved the
-    /// <c>HandleCreateApiMethod</c>/<c>HandleUpdateApiMethod</c>).
+    /// 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 <c>HandleCreateApiMethod</c>/<c>HandleUpdateApiMethod</c>).
     /// </summary>
     /// <param name="method">The API method to compile and register.</param>
     /// <param name="errors">On failure, the compilation/trust-model error messages; empty on success.</param>
@@ -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<string>();
         _knownBadMethods.TryRemove(method.Name, out _);
-        return Register(method.Name, handler);
+        return Register(method, handler);
     }
 
-    private bool Register(string methodName, Func<InboundScriptContext, Task<object?>> handler)
+    private bool Register(ApiMethod method, Func<InboundScriptContext, Task<object?>> 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
+                _logger.LogInformation(
-                // be recompiled on every request — short-circuit before Roslyn runs.
+                    "API method {Method} script changed since it was cached; recompiling", method.Name);
-                if (_knownBadMethods.ContainsKey(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).
+                // Lazy compile (handles methods created after startup, and stale/known-bad
-                // Compile outside the cache so a failed compile is not stored, then add
+                // recompiles). Compile outside the cache so a failed compile is not stored,
-                // atomically so concurrent first-callers share a single handler instance.
+                // 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)

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+public class InboundScriptExecutorStalenessTests
+{
+    private readonly InboundScriptExecutor _executor;
+    private readonly RouteHelper _route;
+
+    public InboundScriptExecutorStalenessTests()
+    {
+        _executor = new InboundScriptExecutor(
+            NullLogger<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
+        _route = new RouteHelper(
+            Substitute.For<IInstanceLocator>(), Substitute.For<IInstanceRouter>());
+    }
+
+    private Task<InboundScriptResult> Run(ApiMethod m) => _executor.ExecuteAsync(
+        m, new Dictionary<string, object?>(), _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);
+    }
+}

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
+        // Arch-review S1 (DB-authoritative): CompileAndRegister still leaves the old
-        // that does not compile. The broken save must NOT replace the working delegate —
+        // handler cached on a broken save (it never overwrites with a bad compile), but
-        // the previously registered version keeps serving requests (this is exactly the
+        // ExecuteAsync now revision-checks the cached script against the freshly-fetched
-        // behaviour that makes a broken save non-fatal while the warning is surfaced).
+        // 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<string, object?>(), _route, TimeSpan.FromSeconds(10));
 
-        Assert.True(result.Success);
+        Assert.False(result.Success);
-        Assert.Contains("111", result.ResultJson);
+        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<string, object?>(), _route, TimeSpan.FromSeconds(10));
+        Assert.True(stillGood.Success);
+        Assert.Contains("111", stillGood.ResultJson);
     }
 
     // --- InboundAPI-002: lazy compile-and-fetch must be atomic, never KeyNotFoundException ---