fix(inbound-api): API-method recompile/invalidate reaches the peer central node; stale doc; locked-override name rule (ScadaBridge#103)

(a) Inbound API compile state (#75) is per central node: a recompile cleared the known-bad record only on the node that answered, and the peer kept refusing the unchanged script with SCRIPT_COMPILE_FAILED (the per-request revision check only notices a changed script). Create / update / delete (ManagementActor post-commit), RecompileApiMethod, and API methods written by a Transport bundle import now publish InboundApiMethodInvalidated on the central cache-invalidation topic (#100 item 3). The new InboundApiCacheInvalidationHandler drops that method's compiled handler, known-bad record and cached ApiMethod row on the peer, which compiles the current text on its next request. (b) docs/components/InboundAPI.md no longer claims a direct DB edit stays stale until restart: the per-request revision check picks it up once the node's cached row expires (ApiMethodCacheTtl, 5 s). Component-InboundAPI, Component-Host and the #85 cache audit describe the peer invalidation. (c) Decision: the import's locked-override warnings keep EXACT (ordinal) name matching, because FlatteningService matches override names ordinally. An override differing from a locked member only by case never reaches that member's lock check (the flattener drops it as unresolved), so a "targets a LOCKED member" warning for it would be false. The comparer is now named (LockedOverrideNameComparer) and documented, and pinned by tests on both sides (flattener + import). Tests: the peer's known-bad record is cleared by a recompile (and validate leaves it alone); create/update/delete and the import publish the invalidation; the handler drops only the named method; composition root registers the handler; case-only override gets no lock warning; the flattener matches override names exactly. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 16:04:07 -05:00
parent 153609ef8a
commit 0c2e852a62
16 changed files with 304 additions and 24 deletions
@@ -436,6 +436,7 @@ public class CentralCompositionRootTests : IDisposable
        Assert.Contains(typeof(SitePskRotationInvalidator), handlers);
        Assert.Contains(typeof(ZB.MOM.WW.ScadaBridge.Security.RoleMappingCache), handlers);
        Assert.Contains(typeof(ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.ExternalDefinitionLookupCache), handlers);
+        Assert.Contains(typeof(ZB.MOM.WW.ScadaBridge.InboundAPI.InboundApiCacheInvalidationHandler), handlers);
        Assert.Equal(handlers.Count, handlers.Distinct().Count());
    }

@@ -0,0 +1,46 @@
+using Microsoft.Extensions.Logging.Abstractions;
+using NSubstitute;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi;
+using ZB.MOM.WW.ScadaBridge.Commons.Messages.Communication;
+
+namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
+
+/// <summary>
+/// ScadaBridge#103: a peer central node's <see cref="InboundApiMethodInvalidated"/> drops this
+/// node's compiled handler, known-bad record and cached row for that method only.
+/// </summary>
+public class InboundApiCacheInvalidationHandlerTests
+{
+    private readonly InboundScriptExecutor _executor =
+        new(NullLogger<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
+    private readonly ApiMethodCache _cache = new(TimeSpan.FromHours(1));
+
+    private async Task<bool> Refetches(string name)
+    {
+        var fetched = false;
+        await _cache.GetOrFetchAsync(name, _ => { fetched = true; return Task.FromResult<ApiMethod?>(null); }, CancellationToken.None);
+        return fetched;
+    }
+
+    /// <summary>The named method's known-bad record, handler and row are dropped; others are kept.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Apply_DropsOnlyTheNamedMethod()
+    {
+        var bad = new ApiMethod("Bad", "return doesNotExist;");
+        var good = new ApiMethod("Good", "return 1;");
+        Assert.False(_executor.CompileAndRegister(bad));
+        Assert.True(_executor.CompileAndRegister(good));
+        await _cache.GetOrFetchAsync("Bad", _ => Task.FromResult<ApiMethod?>(bad), CancellationToken.None);
+        await _cache.GetOrFetchAsync("Good", _ => Task.FromResult<ApiMethod?>(good), CancellationToken.None);
+
+        var handler = new InboundApiCacheInvalidationHandler(_executor, _cache);
+        handler.Apply(new InboundApiMethodInvalidated("Bad"));
+        handler.Apply(new RoleMappingsInvalidated()); // not ours: ignored
+
+        Assert.False(_executor.GetCompileState("Bad", bad.Script).IsKnownBad);
+        Assert.True(await Refetches("Bad"));
+        Assert.True(_executor.GetCompileState("Good", good.Script).IsCompiled);
+        Assert.False(await Refetches("Good"));
+    }
+}
@@ -6,6 +6,7 @@ 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.Communication;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management;
 using ZB.MOM.WW.ScadaBridge.InboundAPI;

@@ -24,6 +25,7 @@ public class ApiMethodCacheInvalidationTests : TestKit

    // A TTL far longer than the test, so only an explicit invalidation can clear an entry.
    private readonly ApiMethodCache _cache = new(TimeSpan.FromHours(1));
+    private readonly PeerCacheInvalidationPublisher _publisher = new();

    /// <summary>Initializes the harness with a long-TTL method cache.</summary>
    public ApiMethodCacheInvalidationTests()
@@ -31,6 +33,7 @@ public class ApiMethodCacheInvalidationTests : TestKit
        _services.AddScoped(_ => _apiRepo);
        _services.AddScoped(_ => Substitute.For<IAuditService>());
        _services.AddSingleton(_cache);
+        _services.AddSingleton<ICentralCacheInvalidationPublisher>(_publisher);
        _apiRepo.SaveChangesAsync(Arg.Any<CancellationToken>()).Returns(1);
    }

@@ -111,4 +114,31 @@ public class ApiMethodCacheInvalidationTests : TestKit

        Assert.False(await Refetches("Other"));
    }
+
+    /// <summary>
+    /// ScadaBridge#103: create, update and delete tell the peer central node to drop the method's
+    /// handler, known-bad record and cached row, once each and after commit.
+    /// </summary>
+    [Fact]
+    public void CreateUpdateDelete_TellThePeer()
+    {
+        var existing = new ApiMethod("Peer", "return 1;") { Id = 11, TimeoutSeconds = 10 };
+        _apiRepo.GetApiMethodByIdAsync(11, Arg.Any<CancellationToken>()).Returns(existing);
+        var actor = CreateActor();
+
+        foreach (var command in new object[]
+                 {
+                     new CreateApiMethodCommand("Peer", "return 1;", 30, null, null),
+                     new UpdateApiMethodCommand(11, "return 2;", 10, null, null),
+                     new DeleteApiMethodCommand(11),
+                 })
+        {
+            actor.Tell(Envelope(command));
+            ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(30));
+        }
+
+        Assert.Equal(
+            Enumerable.Repeat<ICentralCacheInvalidation>(new InboundApiMethodInvalidated("Peer"), 3),
+            _publisher.Published);
+    }
 }
@@ -7,6 +7,7 @@ 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.Communication;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management;
 using ZB.MOM.WW.ScadaBridge.InboundAPI;

@@ -28,6 +29,11 @@ public class ApiMethodCompileDiagnosticsTests : TestKit
    private readonly InboundScriptExecutor _executor =
        new(NullLogger<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
    private readonly ApiMethod _method = new("Broken", BrokenScript) { Id = 5, TimeoutSeconds = 10 };
+    // The other central node, reached only through the peer invalidation (ScadaBridge#103).
+    private readonly InboundScriptExecutor _peerExecutor =
+        new(NullLogger<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
+    private readonly ApiMethodCache _peerMethodCache = new(TimeSpan.FromHours(1));
+    private readonly PeerCacheInvalidationPublisher _publisher;

    /// <summary>Initializes the harness with a real executor and a stubbed repository.</summary>
    public ApiMethodCompileDiagnosticsTests()
@@ -35,6 +41,9 @@ public class ApiMethodCompileDiagnosticsTests : TestKit
        _services.AddScoped(_ => _apiRepo);
        _services.AddScoped(_ => _audit);
        _services.AddSingleton(_executor);
+        _publisher = new PeerCacheInvalidationPublisher(
+            new InboundApiCacheInvalidationHandler(_peerExecutor, _peerMethodCache));
+        _services.AddSingleton<ICentralCacheInvalidationPublisher>(_publisher);
        _apiRepo.GetApiMethodByIdAsync(5, Arg.Any<CancellationToken>()).Returns(_method);
        _apiRepo.SaveChangesAsync(Arg.Any<CancellationToken>()).Returns(1);
    }
@@ -116,6 +125,41 @@ public class ApiMethodCompileDiagnosticsTests : TestKit
        Assert.Contains("doesNotExist", state.LastCompileError);
    }

+    /// <summary>
+    /// ScadaBridge#103: compile state is per node, so a recompile used to clear the known-bad
+    /// record only on the node that answered — the peer kept refusing the unchanged script with
+    /// SCRIPT_COMPILE_FAILED until it restarted. The recompile now drops the peer's record (and
+    /// its cached row), so the peer compiles the current text on its next request.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Recompile_ClearsTheKnownBadRecordOnThePeerNode()
+    {
+        Assert.False(_peerExecutor.CompileAndRegister(_method));
+        Assert.True(_peerExecutor.GetCompileState("Broken", BrokenScript).IsKnownBad); // positive control
+        await _peerMethodCache.GetOrFetchAsync("Broken", _ => Task.FromResult<ApiMethod?>(_method), CancellationToken.None);
+
+        Send(new RecompileApiMethodCommand(5));
+
+        Assert.False(_peerExecutor.GetCompileState("Broken", BrokenScript).IsKnownBad);
+        var refetched = false;
+        await _peerMethodCache.GetOrFetchAsync("Broken", _ => { refetched = true; return Task.FromResult<ApiMethod?>(_method); }, CancellationToken.None);
+        Assert.True(refetched);
+        Assert.Equal(new[] { new InboundApiMethodInvalidated("Broken") }, _publisher.Published);
+    }
+
+    /// <summary>Negative control: validate changes nothing anywhere, so it tells the peer nothing.</summary>
+    [Fact]
+    public void Validate_DoesNotTouchThePeer()
+    {
+        Assert.False(_peerExecutor.CompileAndRegister(_method));
+
+        Send(new ValidateApiMethodCommand(5));
+
+        Assert.True(_peerExecutor.GetCompileState("Broken", BrokenScript).IsKnownBad);
+        Assert.Empty(_publisher.Published);
+    }
+
    /// <summary>Validate and recompile are Designer operations, like UpdateApiMethod.</summary>
    /// <param name="command">The command name to send.</param>
    [Theory]
@@ -154,6 +154,33 @@ public class FlatteningServiceTests
        Assert.Equal("original", attr.Value); // Lock honored
    }

+    /// <summary>
+    /// Pins the name rule the Transport import's locked-override warnings mirror (ScadaBridge#103):
+    /// an override addresses a member by its exact (ordinal) name, so one differing only by case
+    /// is not applied — and so never reaches the member's lock check.
+    /// </summary>
+    [Fact]
+    public void Flatten_InstanceOverride_MatchesTheMemberNameExactly()
+    {
+        var template = CreateTemplate(1, "Base");
+        template.Attributes.Add(new TemplateAttribute("Threshold") { DataType = DataType.Double, Value = "50.0" });
+
+        var instance = CreateInstance();
+        instance.AttributeOverrides.Add(new InstanceAttributeOverride("threshold") { OverrideValue = "75.0" });
+
+        var result = _sut.Flatten(
+            instance,
+            [template],
+            new Dictionary<int, IReadOnlyList<TemplateComposition>>(),
+            new Dictionary<int, IReadOnlyList<Template>>(),
+            new Dictionary<int, DataConnection>());
+
+        Assert.True(result.IsSuccess);
+        var attr = Assert.Single(result.Value.Attributes, a => a.CanonicalName == "Threshold");
+        Assert.Equal("50.0", attr.Value);
+        Assert.DoesNotContain(result.Value.Attributes, a => a.CanonicalName == "threshold");
+    }
+
    /// <summary>Verifies that members of a composed module receive path-qualified names.</summary>
    [Fact]
    public void Flatten_ComposedModule_PathQualifiedNames()
@@ -1858,6 +1858,9 @@ public sealed class BundleImporterApplyTests : IDisposable
        Assert.Equal(new[] { "HelperFn" }, byKind[ScriptArtifactKinds.SharedScript].Names);
        Assert.Equal(new[] { "Pump" }, byKind[ScriptArtifactKinds.Template].Names);
        Assert.All(_artifactBus.Received, n => Assert.Equal("BundleImport", n.Source));
+        // ScadaBridge#103: the peer central node is told about the imported API method too.
+        Assert.Equal(new[] { new InboundApiMethodInvalidated("DoThing") },
+            _peerPublisher.Published.OfType<InboundApiMethodInvalidated>());
    }

    /// <summary>Verifies that applying a bundle publishes script-artifacts-changed notifications for Add resolutions.</summary>
@@ -1375,6 +1375,42 @@ public sealed class SiteInstanceImportTests : IDisposable
            && b.BlockerReason!.Contains("locked", StringComparison.OrdinalIgnoreCase));
    }

+    /// <summary>
+    /// ScadaBridge#103: the lock warning matches member names exactly, like the flattener. An
+    /// override named "setpoint" never reaches the locked "SetPoint" (the flattener drops it as
+    /// unresolved), so it must not be reported as targeting a locked member.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Import_OverrideDifferingOnlyByCaseFromALockedAttribute_NoLockWarning()
+    {
+        await SeedLockedMemberClosureAsync(lockAttribute: true, attributeOverrideName: "setpoint");
+
+        var sessionId = await ExportAllSitesAndLoadAsync();
+        await WipeSiteClosureAsync();
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.Templates.Add(new Template("Pump") { Description = "pump tpl" });
+            await ctx.SaveChangesAsync();
+        }
+
+        var result = await ApplyAsync(
+            sessionId,
+            new List<ImportResolution>
+            {
+                new("Template", "Pump", ResolutionAction.Skip, null),
+                new("Site", "plant-1", ResolutionAction.Add, null),
+                new("Instance", "Pump-01", ResolutionAction.Add, null),
+            },
+            new BundleNameMap(
+                Sites: new[] { new SiteMapping("plant-1", MappingAction.CreateNew, null) },
+                Connections: Array.Empty<ConnectionMapping>()));
+
+        Assert.DoesNotContain(result.Warnings, w =>
+            w.Contains("locked", StringComparison.OrdinalIgnoreCase));
+    }
+
    /// <summary>Verifies that an override on an unlocked attribute produces no lock warning.</summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
@@ -1417,7 +1453,8 @@ public sealed class SiteInstanceImportTests : IDisposable
    /// The attribute override is always present; the native-source override (and its
    /// connection) are seeded only when a native source is involved.
    /// </summary>
-    private async Task SeedLockedMemberClosureAsync(bool lockAttribute = false, bool lockNativeSource = false)
+    private async Task SeedLockedMemberClosureAsync(
+        bool lockAttribute = false, bool lockNativeSource = false, string attributeOverrideName = "SetPoint")
    {
        await using var scope = _provider.CreateAsyncScope();
        var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
@@ -1462,7 +1499,7 @@ public sealed class SiteInstanceImportTests : IDisposable
            SiteId = site.Id,
            State = InstanceState.Enabled,
        };
-        instance.AttributeOverrides.Add(new InstanceAttributeOverride("SetPoint") { OverrideValue = "42" });
+        instance.AttributeOverrides.Add(new InstanceAttributeOverride(attributeOverrideName) { OverrideValue = "42" });
        if (lockNativeSource)
        {
            instance.NativeAlarmSourceOverrides.Add(new InstanceNativeAlarmSourceOverride("NativeSrc")