From 72f96bdbcbd5d241fe8e6a69b9968a28f997f4fa Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 09:20:00 -0400 Subject: [PATCH] =?UTF-8?q?fix(management):=20compile=20an=20API=20method?= =?UTF-8?q?=20once=20per=20save=20=E2=80=94=20CompileForPublish=20replaces?= =?UTF-8?q?=20the=20validate=20pre-pass=20(review=202026-10-21=2005/M1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/requirements/Component-InboundAPI.md | 2 +- .../Component-ManagementService.md | 2 +- .../InboundScriptExecutor.cs | 41 ++++-- .../ManagementActor.cs | 10 +- .../ApiMethodSingleCompileTests.cs | 119 ++++++++++++++++++ 5 files changed, 160 insertions(+), 14 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodSingleCompileTests.cs diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index 4a27e897..45c0eeda 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -175,7 +175,7 @@ The `sbk__` design already supports zero-downtime rotation — a ## Script Compilation & Hot-Reload -API method scripts are pre-compiled in the **background after startup** by `InboundMethodWarmupService` (a hosted service registered after `AkkaHostedService`, so it never delays the cluster join or `/health/*`): it waits for `ApplicationStarted`, then — only once this node is the active central node (`IActiveNodeGate`; a standby pays nothing until a failover promotes it) — loads all method definitions and compiles them into in-memory delegates. Lazy compile + the per-request revision check guarantee correctness regardless. Compiles are **single-flight per method name + script revision**: N concurrent first callers share one compile (and one non-collectible loaded assembly). `InboundScriptExecutor.CompileAndRegister(ApiMethod, out IReadOnlyList errors)` compiles once and returns the diagnostics, so a save needs no separate validate pass. `ApiMethodCache` prunes expired entries on a miss once large (scopes are exact method names — pinned by test — so growth is bounded). +API method scripts are pre-compiled in the **background after startup** by `InboundMethodWarmupService` (a hosted service registered after `AkkaHostedService`, so it never delays the cluster join or `/health/*`): it waits for `ApplicationStarted`, then — only once this node is the active central node (`IActiveNodeGate`; a standby pays nothing until a failover promotes it) — loads all method definitions and compiles them into in-memory delegates. Lazy compile + the per-request revision check guarantee correctness regardless. Compiles are **single-flight per method name + script revision**: N concurrent first callers share one compile (and one non-collectible loaded assembly). `InboundScriptExecutor.CompileAndRegister(ApiMethod, out IReadOnlyList errors)` compiles once and returns the diagnostics, so a save needs no separate validate pass; the management Create/Update path uses its sibling `CompileForPublish(method, out errors, out publish)`, which compiles once for the `compileWarning` and hands back a `publish` action queued as the post-commit registration — one compile (one pinned assembly) per save even inside a configuration transaction (review 2026-10-21 05/M1). `ApiMethodCache` prunes expired entries on a miss once large (scopes are exact method names — pinned by test — so growth is bounded). ### Update Workflow diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index a9a04ff0..61bf7699 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -213,7 +213,7 @@ The two-person authorization workflow for writes through the MxAccess Gateway. B ### Inbound API Methods - **ListApiMethods** / **GetApiMethod**: Query inbound API method definitions. -- **CreateApiMethod** / **UpdateApiMethod** / **DeleteApiMethod**: Manage inbound API methods. +- **CreateApiMethod** / **UpdateApiMethod** / **DeleteApiMethod**: Manage inbound API methods. Create/Update compile the script **once** (`InboundScriptExecutor.CompileForPublish`): the diagnostics become the result's `compileWarning` and the handler registration is queued as a post-commit action reusing that compile — each compile pins a non-collectible assembly, and the old validate-then-register pair compiled twice per save (review 2026-10-21 05/M1). ### Health diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index 193c1a19..60247b00 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -194,6 +194,31 @@ public class InboundScriptExecutor /// True if successfully compiled and registered, false otherwise. public bool CompileAndRegister(ApiMethod method, out IReadOnlyList errors) { + var compiled = CompileForPublish(method, out errors, out var publish); + publish(); + return compiled; + } + + /// + /// Compiles an API method script ONCE and returns the diagnostics now, together with a + /// action that applies the outcome to the handler cache later + /// without recompiling (review 2026-10-21 05/M1). A transactional caller (the management + /// Create/Update path) derives its "saved but did not compile" warning before commit and + /// queues as a post-commit action; each compile pins a + /// non-collectible script assembly, so compiling separately for the warning and for the + /// registration doubled that cost per save. On success + /// registers the handler for the compiled script text and clears any known-bad record; + /// on failure it records the known-bad method and leaves any previously registered + /// handler in place (see ). + /// + /// The API method to compile. + /// The compilation/trust-model error messages; empty on success. + /// Applies the compile outcome to the handler cache; idempotent. + /// true when the script compiled; otherwise false. + public bool CompileForPublish(ApiMethod method, out IReadOnlyList errors, out Action publish) + { + var name = method.Name; + var script = method.Script; var (handler, compileErrors) = CompileSingleFlight(method); if (handler == null) { @@ -202,15 +227,19 @@ 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, method.Script ?? string.Empty); + publish = () => TryRecordBadMethod(name, script ?? string.Empty); return false; } // The method definition was (re)compiled successfully — drop any stale // failure record so a fixed script is no longer treated as bad. errors = Array.Empty(); - _knownBadMethods.TryRemove(method.Name, out _); - return Register(method, handler); + publish = () => + { + _knownBadMethods.TryRemove(name, out _); + _scriptHandlers[name] = new CachedHandler(script, handler); + }; + return true; } /// @@ -235,12 +264,6 @@ public class InboundScriptExecutor } } - private bool Register(ApiMethod method, Func> handler) - { - _scriptHandlers[method.Name] = new CachedHandler(method.Script, handler); - return true; - } - /// /// Compiles a single API method script into an executable handler. Returns /// null when the script is missing, fails to compile, or violates the diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 0cde809d..96e8fd80 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -3667,12 +3667,16 @@ public class ManagementActor : ReceiveActor var executor = sp.GetService(); if (executor is null) return null; - var compiled = executor.ValidateCompilation(method, out var errors); + // ONE compile (review 2026-10-21 05/M1): each compile pins a non-collectible script + // assembly, so the diagnostics for the warning and the handler registration come + // from the same compile. Registration still waits for the commit when the command + // runs inside a configuration transaction. + var compiled = executor.CompileForPublish(method, out var errors, out var publish); var actions = sp.GetService(); if (actions is not null) - actions.Add(() => { executor.CompileAndRegister(method); return Task.CompletedTask; }); + actions.Add(() => { publish(); return Task.CompletedTask; }); else - executor.CompileAndRegister(method); + publish(); if (compiled) return null; diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodSingleCompileTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodSingleCompileTests.cs new file mode 100644 index 00000000..4a5421af --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodSingleCompileTests.cs @@ -0,0 +1,119 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.ScadaBridge.InboundAPI; + +namespace ZB.MOM.WW.ScadaBridge.ManagementService.Tests; + +/// +/// Review 2026-10-21 05/M1: an API-method save used to compile the script twice (a +/// ValidateCompilation pre-pass, then CompileAndRegister), and each compile +/// pins a non-collectible assembly. A save must compile exactly once — including when the +/// registration is deferred until after the configuration transaction commits. +/// +public class ApiMethodSingleCompileTests : TestKit +{ + private readonly ServiceCollection _services = new ServiceCollection().AddTestConfigurationTransaction(); + private readonly IInboundApiRepository _apiRepo = Substitute.For(); + private readonly CompileCountingLogger _logger = new(); + private readonly CollectingCommitActions _commitActions = new(); + + /// Initializes the harness with a real executor whose compiles are counted. + public ApiMethodSingleCompileTests() + { + _services.AddScoped(_ => _apiRepo); + _services.AddScoped(_ => Substitute.For()); + _services.AddSingleton(new InboundScriptExecutor(_logger, Substitute.For())); + _apiRepo.SaveChangesAsync(Arg.Any()).Returns(1); + } + + private IActorRef CreateActor() => + Sys.ActorOf(Props.Create(() => new ManagementActor( + _services.BuildServiceProvider(), NullLogger.Instance))); + + private static ManagementEnvelope Envelope(object command) => + new(new AuthenticatedUser("designer", "Designer", new[] { "Designer" }, Array.Empty()), + command, Guid.NewGuid().ToString("N")); + + /// A compiling create without deferred commit actions compiles once. + [Fact] + public void Create_CompilingScript_CompilesOnce() + { + CreateActor().Tell(Envelope(new CreateApiMethodCommand("Once", "return 1;", 30, null, null))); + + var response = ExpectMsg(TimeSpan.FromSeconds(30)); + Assert.Equal(1, _logger.CompileCount); + Assert.DoesNotContain("failed to compile", response.JsonData); + } + + /// + /// With the registration deferred to post-commit, the warning is still derived before the + /// reply and the queued publish reuses the compiled result instead of recompiling. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Update_DeferredRegistration_CompilesOnce_AndStillWarns() + { + _services.AddScoped(_ => _commitActions); + var existing = new ApiMethod("M", "return 1;") { Id = 7, TimeoutSeconds = 10 }; + _apiRepo.GetApiMethodByIdAsync(7, Arg.Any()).Returns(existing); + + CreateActor().Tell(Envelope(new UpdateApiMethodCommand(7, "return doesNotExist;", 10, null, null))); + + var response = ExpectMsg(TimeSpan.FromSeconds(30)); + Assert.Contains("failed to compile", response.JsonData); + foreach (var action in _commitActions.Actions) + await action(); + Assert.NotEmpty(_commitActions.Actions); + Assert.Equal(0, _logger.CompileCount); // the broken script never reaches "compiled" + Assert.Equal(1, _logger.CompileFailureCount); + } + + /// A compiling update with deferred registration compiles once across reply + publish. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Update_DeferredRegistration_CompilingScript_CompilesOnce() + { + _services.AddScoped(_ => _commitActions); + var existing = new ApiMethod("M2", "return 1;") { Id = 8, TimeoutSeconds = 10 }; + _apiRepo.GetApiMethodByIdAsync(8, Arg.Any()).Returns(existing); + + CreateActor().Tell(Envelope(new UpdateApiMethodCommand(8, "return 2;", 10, null, null))); + + ExpectMsg(TimeSpan.FromSeconds(30)); + foreach (var action in _commitActions.Actions) + await action(); + Assert.Equal(1, _logger.CompileCount); + } + + private sealed class CollectingCommitActions : IConfigurationCommitActions + { + public List> Actions { get; } = new(); + public void Add(Func action) => Actions.Add(action); + } + + private sealed class CompileCountingLogger : ILogger + { + private int _count; + private int _failures; + public int CompileCount => _count; + public int CompileFailureCount => _failures; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + var text = formatter(state, exception); + if (text.EndsWith("script compiled", StringComparison.Ordinal)) + Interlocked.Increment(ref _count); + else if (text.Contains("script compilation failed", StringComparison.Ordinal)) + Interlocked.Increment(ref _failures); + } + } +}