fix(transport): drop the external-definition lookup cache after a committed bundle import (ScadaBridge#83)

The bundle importer writes external systems, methods and database connections
through the raw DbContext, bypassing the ManagementActor handlers that
invalidate ExternalDefinitionLookupCache (#65), so the importing node served
pre-import definitions for up to DefinitionCacheTtl. New Commons seam
IExternalDefinitionCacheInvalidator, implemented by the cache and registered as
the same singleton; BundleImporter takes it optionally and invalidates once
after every committed import (never on rollback; never throws).

Peer-central-node invalidation (the other half of #83) is NOT done here: the
issue leaves open whether the 10 s TTL bound is acceptable for the peer.

Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
2026-11-09 12:40:43 -05:00
parent 6d5bb6189f
commit 03de3dcc14
6 changed files with 132 additions and 2 deletions
@@ -0,0 +1,15 @@
namespace ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
/// <summary>
/// Drops the node-local cache of external-system / method / database-connection definitions
/// (<c>ExternalSystemGateway.ExternalDefinitionLookupCache</c>) after a committed write to
/// those definitions, so the editing node serves the new definition immediately instead of
/// after the cache TTL. Lives in Commons so a writer that cannot reference the gateway (the
/// Transport bundle importer, Gitea #83) can still signal it. Scope is THIS node only — a
/// peer central node converges within the TTL.
/// </summary>
public interface IExternalDefinitionCacheInvalidator
{
/// <summary>Drops every cached definition on this node.</summary>
void InvalidateAll();
}
@@ -1,6 +1,7 @@
using System.Collections.Concurrent; using System.Collections.Concurrent;
using Microsoft.Extensions.Options; using Microsoft.Extensions.Options;
using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems; using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems;
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway; namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway;
@@ -19,7 +20,7 @@ namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway;
/// deleted one is visible within one TTL. /// deleted one is visible within one TTL.
/// </para> /// </para>
/// </summary> /// </summary>
public sealed class ExternalDefinitionLookupCache public sealed class ExternalDefinitionLookupCache : IExternalDefinitionCacheInvalidator
{ {
private sealed record Entry(object? Value, DateTimeOffset ExpiresAtUtc); private sealed record Entry(object? Value, DateTimeOffset ExpiresAtUtc);
@@ -72,6 +73,10 @@ public sealed class ExternalDefinitionLookupCache
=> GetAsync("c:" + name, fetch); => GetAsync("c:" + name, fetch);
/// <summary>Drops every cached definition.</summary> /// <summary>Drops every cached definition.</summary>
/// <remarks>
/// Called post-commit by the ManagementActor edit handlers (Gitea #65) and by the Transport
/// bundle importer through <see cref="IExternalDefinitionCacheInvalidator"/> (Gitea #83).
/// </remarks>
public void InvalidateAll() => _entries.Clear(); public void InvalidateAll() => _entries.Clear();
private async Task<T?> GetAsync<T>(string key, Func<Task<T?>> fetch) where T : class private async Task<T?> GetAsync<T>(string key, Func<Task<T?>> fetch) where T : class
@@ -44,6 +44,10 @@ public static class ServiceCollectionExtensions
sp.GetRequiredService<IOptionsMonitor<ExternalSystemGatewayOptions>>())); sp.GetRequiredService<IOptionsMonitor<ExternalSystemGatewayOptions>>()));
services.TryAddSingleton<ExternalDefinitionLookupCache>(); services.TryAddSingleton<ExternalDefinitionLookupCache>();
// Gitea #83: the same singleton, exposed through the Commons seam so the Transport bundle
// importer can drop it after a committed import without referencing this assembly.
services.TryAddSingleton<IExternalDefinitionCacheInvalidator>(
sp => sp.GetRequiredService<ExternalDefinitionLookupCache>());
services.AddScoped<ExternalSystemClient>(); services.AddScoped<ExternalSystemClient>();
services.AddScoped<IExternalSystemClient>(sp => sp.GetRequiredService<ExternalSystemClient>()); services.AddScoped<IExternalSystemClient>(sp => sp.GetRequiredService<ExternalSystemClient>());
services.AddScoped<DatabaseGateway>(); services.AddScoped<DatabaseGateway>();
@@ -95,6 +95,9 @@ public sealed class BundleImporter : IBundleImporter
// derives watermark bumps from the change tracker. Null in hosts that don't // derives watermark bumps from the change tracker. Null in hosts that don't
// register it; the caches then simply never get this invalidation signal. // register it; the caches then simply never get this invalidation signal.
private readonly ITemplateGraphWatermark? _watermark; private readonly ITemplateGraphWatermark? _watermark;
// Optional (Gitea #83): drops this node's external-definition lookup cache after a
// committed import. Null on hosts without the External System Gateway.
private readonly IExternalDefinitionCacheInvalidator? _definitionCacheInvalidator;
private readonly ILogger<BundleImporter>? _logger; private readonly ILogger<BundleImporter>? _logger;
/// <summary> /// <summary>
@@ -121,6 +124,7 @@ public sealed class BundleImporter : IBundleImporter
/// <param name="scriptArtifactChangeBus">Optional: node-local bus notified AFTER a successful commit that script-bearing artifacts (ApiMethod / SharedScript / Template) were overwritten or renamed, so compiled-handler caches can invalidate. Null on hosts that don't register it (staleness/invalidation then relies on the consumer's own self-heal).</param> /// <param name="scriptArtifactChangeBus">Optional: node-local bus notified AFTER a successful commit that script-bearing artifacts (ApiMethod / SharedScript / Template) were overwritten or renamed, so compiled-handler caches can invalidate. Null on hosts that don't register it (staleness/invalidation then relies on the consumer's own self-heal).</param>
/// <param name="logger">Optional logger.</param> /// <param name="logger">Optional logger.</param>
/// <param name="watermark">Optional: template-graph version watermark, bumped once per apply attempt so the flatten-session caches and the process-wide staleness memo cannot serve pre-import (or rolled-back) readings. Null on hosts that don't register it.</param> /// <param name="watermark">Optional: template-graph version watermark, bumped once per apply attempt so the flatten-session caches and the process-wide staleness memo cannot serve pre-import (or rolled-back) readings. Null on hosts that don't register it.</param>
/// <param name="definitionCacheInvalidator">Optional (Gitea #83): drops this node's external-system / database-connection definition cache after a committed import, so imported definitions take effect immediately rather than after the cache TTL. Null on hosts without the External System Gateway.</param>
public BundleImporter( public BundleImporter(
BundleSerializer bundleSerializer, BundleSerializer bundleSerializer,
ManifestValidator manifestValidator, ManifestValidator manifestValidator,
@@ -142,7 +146,8 @@ public sealed class BundleImporter : IBundleImporter
IStaleInstanceProbe? staleInstanceProbe = null, IStaleInstanceProbe? staleInstanceProbe = null,
IScriptArtifactChangeBus? scriptArtifactChangeBus = null, IScriptArtifactChangeBus? scriptArtifactChangeBus = null,
ILogger<BundleImporter>? logger = null, ILogger<BundleImporter>? logger = null,
ITemplateGraphWatermark? watermark = null) ITemplateGraphWatermark? watermark = null,
IExternalDefinitionCacheInvalidator? definitionCacheInvalidator = null)
{ {
_bundleSerializer = bundleSerializer ?? throw new ArgumentNullException(nameof(bundleSerializer)); _bundleSerializer = bundleSerializer ?? throw new ArgumentNullException(nameof(bundleSerializer));
_manifestValidator = manifestValidator ?? throw new ArgumentNullException(nameof(manifestValidator)); _manifestValidator = manifestValidator ?? throw new ArgumentNullException(nameof(manifestValidator));
@@ -164,6 +169,7 @@ public sealed class BundleImporter : IBundleImporter
_staleInstanceProbe = staleInstanceProbe; _staleInstanceProbe = staleInstanceProbe;
_scriptArtifactChangeBus = scriptArtifactChangeBus; _scriptArtifactChangeBus = scriptArtifactChangeBus;
_watermark = watermark; _watermark = watermark;
_definitionCacheInvalidator = definitionCacheInvalidator;
_logger = logger; _logger = logger;
} }
@@ -1697,6 +1703,7 @@ public sealed class BundleImporter : IBundleImporter
// Post-commit side effects — run ONCE, only after a durable commit, so a // Post-commit side effects — run ONCE, only after a durable commit, so a
// retried attempt neither publishes twice nor releases the session early. // retried attempt neither publishes twice nor releases the session early.
PublishScriptArtifactChanges(resolutions); PublishScriptArtifactChanges(resolutions);
InvalidateExternalDefinitionCache();
// T-007: zero out the decrypted plaintext BEFORE remove so any // T-007: zero out the decrypted plaintext BEFORE remove so any
// caller-held reference (e.g. the Razor page that built the // caller-held reference (e.g. the Razor page that built the
@@ -2442,6 +2449,32 @@ public sealed class BundleImporter : IBundleImporter
/// (nothing was written). Fully guarded: never throws (it runs after the transaction /// (nothing was written). Fully guarded: never throws (it runs after the transaction
/// has committed). /// has committed).
/// </summary> /// </summary>
/// <summary>
/// Gitea #83: the importer writes external systems, their methods and database connections
/// through the raw <c>ScadaBridgeDbContext</c>, bypassing the ManagementActor handlers that
/// drop <c>ExternalDefinitionLookupCache</c> (Gitea #65), so without this the importing node
/// served the pre-import definition for up to <c>DefinitionCacheTtl</c>. Invalidates on every
/// committed import rather than only when an external entity was resolved: dependencies can
/// pull definitions in implicitly, imports are rare, and a cleared cache only costs one
/// re-query per name. Never throws (the transaction has already committed).
/// </summary>
private void InvalidateExternalDefinitionCache()
{
if (_definitionCacheInvalidator is null)
{
return;
}
try
{
_definitionCacheInvalidator.InvalidateAll();
}
catch (Exception ex)
{
_logger?.LogError(ex, "Invalidating the external-definition cache after a committed bundle import failed; ignored (the cache TTL still bounds staleness).");
}
}
private void PublishScriptArtifactChanges(IReadOnlyList<ImportResolution> resolutions) private void PublishScriptArtifactChanges(IReadOnlyList<ImportResolution> resolutions)
{ {
if (_scriptArtifactChangeBus is null) if (_scriptArtifactChangeBus is null)
@@ -3,6 +3,7 @@ using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Http; using Microsoft.Extensions.Http;
using NSubstitute; using NSubstitute;
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests; namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests;
@@ -12,6 +13,26 @@ namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests;
/// </summary> /// </summary>
public class ServiceWiringTests public class ServiceWiringTests
{ {
/// <summary>
/// Gitea #83: the Commons invalidation seam resolves to the SAME lookup-cache singleton the
/// gateway reads through, so a bundle import that invalidates it actually clears the cache.
/// </summary>
[Fact]
public void DefinitionCacheInvalidator_IsTheLookupCacheSingleton()
{
var services = new ServiceCollection();
services.AddLogging();
services.AddSingleton<IConfiguration>(new ConfigurationBuilder().Build());
services.AddSingleton(Substitute.For<IExternalSystemRepository>());
services.AddExternalSystemGateway();
using var provider = services.BuildServiceProvider();
Assert.Same(
provider.GetRequiredService<ExternalDefinitionLookupCache>(),
provider.GetRequiredService<IExternalDefinitionCacheInvalidator>());
}
/// <summary>Verifies that the per-system connection cap is applied to the primary handler of the named HTTP client.</summary> /// <summary>Verifies that the per-system connection cap is applied to the primary handler of the named HTTP client.</summary>
[Fact] [Fact]
public void MaxConcurrentConnectionsPerSystem_IsAppliedToTheNamedHttpClientPrimaryHandler() public void MaxConcurrentConnectionsPerSystem_IsAppliedToTheNamedHttpClientPrimaryHandler()
@@ -47,6 +47,7 @@ public sealed class BundleImporterApplyTests : IDisposable
{ {
private readonly ServiceProvider _provider; private readonly ServiceProvider _provider;
private readonly RecordingScriptArtifactChangeBus _artifactBus = new(); private readonly RecordingScriptArtifactChangeBus _artifactBus = new();
private readonly CountingDefinitionCacheInvalidator _definitionCache = new();
/// <summary>Initializes a new instance of the <c>BundleImporterApplyTests</c> class.</summary> /// <summary>Initializes a new instance of the <c>BundleImporterApplyTests</c> class.</summary>
public BundleImporterApplyTests() public BundleImporterApplyTests()
@@ -100,6 +101,8 @@ public sealed class BundleImporterApplyTests : IDisposable
// publish is observable. BundleImporter takes IScriptArtifactChangeBus? // publish is observable. BundleImporter takes IScriptArtifactChangeBus?
// optionally, so this registration flows in via DI activation. // optionally, so this registration flows in via DI activation.
services.AddSingleton<IScriptArtifactChangeBus>(_artifactBus); services.AddSingleton<IScriptArtifactChangeBus>(_artifactBus);
// Gitea #83: observable post-commit drop of the external-definition lookup cache.
services.AddSingleton<IExternalDefinitionCacheInvalidator>(_definitionCache);
// The stale-instance probe is implemented in DeploymentManager // The stale-instance probe is implemented in DeploymentManager
// and the flatten/hash primitives in TemplateEngine — register both so // and the flatten/hash primitives in TemplateEngine — register both so
// BundleImporter resolves a real IStaleInstanceProbe and ApplyAsync can // BundleImporter resolves a real IStaleInstanceProbe and ApplyAsync can
@@ -1910,6 +1913,41 @@ public sealed class BundleImporterApplyTests : IDisposable
Assert.Contains("DelmiaRecipeDownload", notification.Names); // FAILS today: Adds are filtered out Assert.Contains("DelmiaRecipeDownload", notification.Names); // FAILS today: Adds are filtered out
} }
/// <summary>
/// Gitea #83: a committed import drops the node's external-definition lookup cache once,
/// because the importer writes external systems / database connections without going
/// through the ManagementActor handlers that otherwise invalidate it.
/// </summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task ApplyAsync_committed_import_invalidates_the_external_definition_cache()
{
await using (var scope = _provider.CreateAsyncScope())
{
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Mes", "https://mes.example/api", "None"));
await ctx.SaveChangesAsync();
}
var sessionId = await ExportAndLoadAsync();
await using (var scope = _provider.CreateAsyncScope())
{
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
ctx.ExternalSystemDefinitions.RemoveRange(ctx.ExternalSystemDefinitions);
await ctx.SaveChangesAsync();
}
Assert.Equal(0, _definitionCache.Calls);
await using (var scope = _provider.CreateAsyncScope())
{
var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
await importer.ApplyAsync(sessionId,
new List<ImportResolution> { new("ExternalSystem", "Mes", ResolutionAction.Add, null) },
user: "bob");
}
Assert.Equal(1, _definitionCache.Calls);
}
/// <summary>Verifies that a failed apply publishes no notifications.</summary> /// <summary>Verifies that a failed apply publishes no notifications.</summary>
/// <returns>A task that represents the asynchronous operation.</returns> /// <returns>A task that represents the asynchronous operation.</returns>
[Fact] [Fact]
@@ -1958,6 +1996,20 @@ public sealed class BundleImporterApplyTests : IDisposable
} }
Assert.Empty(_artifactBus.Received); Assert.Empty(_artifactBus.Received);
// Gitea #83: nothing committed, so the definition cache is left alone.
Assert.Equal(0, _definitionCache.Calls);
}
/// <summary>Counts post-commit external-definition cache invalidations (Gitea #83).</summary>
private sealed class CountingDefinitionCacheInvalidator : IExternalDefinitionCacheInvalidator
{
private int _calls;
/// <summary>Gets the number of invalidations received.</summary>
public int Calls => Volatile.Read(ref _calls);
/// <inheritdoc />
public void InvalidateAll() => Interlocked.Increment(ref _calls);
} }
/// <summary>Records every published notification for assertion.</summary> /// <summary>Records every published notification for assertion.</summary>