feat(external-systems): deploy per-system retry policy to sites + CLI/management surface (register row 27)

An external system's MaxRetries/RetryDelay existed on the central entity but never reached a site and had no management surface, so every ExternalSystem.CachedCall buffered with the S&F engine default (DefaultMaxRetries 50 x DefaultRetryInterval 30s ~= 25 min to park) whatever central had configured. The sibling DatabaseConnectionArtifact had always carried its equivalents. The consumption end was already wired: ExternalSystemClient.CachedCallAsync already read system.MaxRetries/RetryDelay and passed them to EnqueueAsync with the 0-as-unset convention. Only the transport and persistence were missing. Commons - ExternalSystemArtifact gains MaxRetries/RetryDelay as optional positional params (0 / TimeSpan.Zero = unset), matching DatabaseConnectionArtifact rather than going nullable, so it maps 1:1 onto the entity and the proto3 scalars. - Create/UpdateExternalSystemCommand gain nullable MaxRetries/RetryDelay, following the existing TimeoutSeconds "omit to preserve" convention. Both additive. Central - ManagementActor create maps both; update preserves-if-null. - ArtifactDeploymentService projects them onto the artifact. Wire - site_command.proto ExternalSystemArtifactDto gains max_retries=7 / retry_delay=8 (google.protobuf.Duration), mirroring DatabaseConnectionArtifactDto. Additive: a pre-existing site decodes them as 0/absent, which IS the unset value, so old behaviour is preserved. Vendored C# regenerated via docker/regen-proto.sh sitecommand; csproj Protobuf item re-commented, no message types added or removed. Site - external_systems gains max_retries / retry_delay_ms via AddColumnIfMissing, DEFAULT 0 = unset. Storage upsert, repository SELECT/mapping (new ordinals 6/7 appended so existing ones are untouched), apply path, and the legacy migrator's explicit column list all carry them. Replicated-table decision: external_systems IS in SiteLocalDbSetup.ReplicatedTables, so this was checked against the AckTime/native_alarm_state precedent. Real columns are correct here, not a JSON bag: the table has no general-purpose config column (auth_configuration and method_definitions are both semantically specific), timeout_seconds already set the additive-column precedent on this exact table, and sibling database_connections stores the same two values as plain columns. Verified in LocalDb source that RegisterReplicated re-reads PRAGMA table_info every boot and its install script is DROP-then-CREATE, so capture triggers rebuild over the new column set automatically, and SiteStorageSchema.Apply runs before registration on the same boot. The only cost is the ReplicatedTable.ComputeDigest change, which the sync handshake compares fail-closed - a mixed-version pair refuses to sync loudly instead of diverging silently. Both-nodes-together is already the standing site-pair rule, so no new operational constraint; recorded in topology-guide.md. Also fixed: ArtifactDiff.CompareExternalSystem never compared MaxRetries/RetryDelay, so a bundle import that changed a system's retry budget previewed as "Identical" - harmless while the values were inert, a live behaviour change now. No EF migration: the central ExternalSystemDefinitions columns already exist (InitialSchema). Central UI already had both form fields. Tests: site storage round-trip + unset-default, site apply path, central artifact projection, proto round-trip (via the shared sample), management create/preserve, CLI option parse + alias, and diff reporting. Fail-before verified on the two core tests (expected 7 got 0 on the read path; expected 9 got 0 on the apply path). Docs: Component-ExternalSystemGateway, Component-StoreAndForward, Component-CLI, components/StoreAndForward, components/SiteRuntime, deployment/topology-guide, CLI README (which was also missing --timeout-seconds entirely).
2026-08-15 12:56:12 -04:00
parent ef876b38c5
commit 41a5ecf14a
28 changed files with 1036 additions and 453 deletions
@@ -0,0 +1,102 @@
+using System.CommandLine;
+using ZB.MOM.WW.ScadaBridge.CLI.Commands;
+
+namespace ZB.MOM.WW.ScadaBridge.CLI.Tests.Commands;
+
+/// <summary>
+/// Deferred-work row 27(b): <c>external-system create</c> and <c>external-system update</c>
+/// must expose the per-system Store-and-Forward retry policy. Before this the fields existed
+/// on the central entity but had NO management surface at all — the only way to set them was
+/// a direct DB edit of <c>ExternalSystemDefinitions</c>.
+/// <para>
+/// The delay flag is named <c>--retry-delay-seconds</c> to match the sibling
+/// <c>--timeout-seconds</c> on these same commands (the repo-wide CLI convention is an explicit
+/// unit suffix), with <c>--retry-delay</c> kept as an alias.
+/// </para>
+/// </summary>
+public class ExternalSystemRetryOptionTests
+{
+    private static readonly Option<string> Url = new("--url") { Recursive = true };
+    private static readonly Option<string> Format = new("--format") { Recursive = true };
+    private static readonly Option<string> Username = new("--username") { Recursive = true };
+    private static readonly Option<string> Password = new("--password") { Recursive = true };
+
+    private static Command Group() => ExternalSystemCommands.Build(Url, Format, Username, Password);
+
+    private static Option? FindOption(Command command, string name)
+        => command.Options.SingleOrDefault(o => o.Name == name);
+
+    [Theory]
+    [InlineData("create")]
+    [InlineData("update")]
+    public void HasOptionalRetryPolicyOptions(string subcommand)
+    {
+        var cmd = Group().Subcommands.Single(c => c.Name == subcommand);
+
+        var maxRetries = FindOption(cmd, "--max-retries");
+        Assert.NotNull(maxRetries);
+        Assert.False(maxRetries!.Required);
+
+        var retryDelay = FindOption(cmd, "--retry-delay-seconds");
+        Assert.NotNull(retryDelay);
+        Assert.False(retryDelay!.Required);
+    }
+
+    [Fact]
+    public void Create_ParsesRetryPolicyValues()
+    {
+        var parse = Group().Parse(new[]
+        {
+            "create", "--name", "Sys", "--endpoint-url", "http://x", "--auth-type", "none",
+            "--max-retries", "8", "--retry-delay-seconds", "60",
+        });
+
+        Assert.Empty(parse.Errors);
+        Assert.Equal(8, parse.GetValue<int?>("--max-retries"));
+        Assert.Equal(60, parse.GetValue<int?>("--retry-delay-seconds"));
+    }
+
+    [Fact]
+    public void Update_ParsesRetryPolicyValues()
+    {
+        var parse = Group().Parse(new[]
+        {
+            "update", "--id", "7", "--name", "Sys", "--endpoint-url", "http://x",
+            "--auth-type", "none", "--max-retries", "3", "--retry-delay-seconds", "10",
+        });
+
+        Assert.Empty(parse.Errors);
+        Assert.Equal(3, parse.GetValue<int?>("--max-retries"));
+        Assert.Equal(10, parse.GetValue<int?>("--retry-delay-seconds"));
+    }
+
+    [Fact]
+    public void RetryDelayAliasIsAccepted()
+    {
+        var parse = Group().Parse(new[]
+        {
+            "create", "--name", "Sys", "--endpoint-url", "http://x", "--auth-type", "none",
+            "--retry-delay", "45",
+        });
+
+        Assert.Empty(parse.Errors);
+        Assert.Equal(45, parse.GetValue<int?>("--retry-delay-seconds"));
+    }
+
+    /// <summary>
+    /// Omission must stay distinguishable from an explicit 0 — that is what makes update's
+    /// "omit to preserve" rule and create's "fall back to the engine default" rule work.
+    /// </summary>
+    [Fact]
+    public void OmittedRetryPolicy_ParsesAsNull()
+    {
+        var parse = Group().Parse(new[]
+        {
+            "create", "--name", "Sys", "--endpoint-url", "http://x", "--auth-type", "none",
+        });
+
+        Assert.Empty(parse.Errors);
+        Assert.Null(parse.GetValue<int?>("--max-retries"));
+        Assert.Null(parse.GetValue<int?>("--retry-delay-seconds"));
+    }
+}
@@ -217,8 +217,12 @@ internal static class SiteCommandSamples
    private static SharedScriptArtifact SharedScript() =>
        new("Calc", "return 1;", "{\"p\":\"int\"}", "{\"r\":\"int\"}");

+    // MaxRetries/RetryDelay are non-default on purpose: they are the deferred-work-row-27
+    // additions (proto fields 7-8), and the generic round-trip assertion is what pins them
+    // to the wire. A sample left at the defaults would round-trip vacuously.
    private static ExternalSystemArtifact ExternalSystem() =>
-        new("Mes", "https://mes/api", "ApiKey", "{\"key\":\"x\"}", "[{\"name\":\"Post\"}]", 45);
+        new("Mes", "https://mes/api", "ApiKey", "{\"key\":\"x\"}", "[{\"name\":\"Post\"}]", 45,
+            MaxRetries: 12, RetryDelay: TimeSpan.FromSeconds(20));

    private static DataConnectionArtifact DataConnection() =>
        new("Plc1", "OpcUa", "{\"url\":\"opc.tcp://a\"}", "{\"url\":\"opc.tcp://b\"}", 5);
@@ -11,6 +11,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts;
 using ZB.MOM.WW.ScadaBridge.Communication;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems;

 namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests;

@@ -189,6 +190,38 @@ public class ArtifactDeploymentServiceTests : TestKit
        await _siteRepo.Received(1).GetDataConnectionsBySiteIdAsync(3, Arg.Any<CancellationToken>());
    }

+    /// <summary>
+    /// Deferred-work row 27(a): the per-system retry policy must be projected onto the
+    /// artifact that ships to sites. Before the fix <c>ExternalSystemArtifact</c> had no such
+    /// fields, so a centrally-configured policy never left central and every site cached call
+    /// silently used the engine default (50 x 30 s) — while the sibling
+    /// <c>DatabaseConnectionArtifact</c> had always carried its equivalents.
+    /// </summary>
+    [Fact]
+    public async Task DeployedExternalSystemArtifact_CarriesRetryPolicy()
+    {
+        _externalSystemRepo.GetAllExternalSystemsAsync(Arg.Any<CancellationToken>()).Returns(
+        [
+            new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+            {
+                Id = 1,
+                MaxRetries = 6,
+                RetryDelay = TimeSpan.FromSeconds(120),
+            },
+        ]);
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new ArtifactProbeActor(recorder)));
+        var service = CreateServiceWithCommActor(probe);
+
+        var result = await service.RetryForSiteAsync(1, "retry-site", "admin");
+
+        Assert.True(result.IsSuccess);
+        var artifact = Assert.Single(Assert.Single(recorder.Received).ExternalSystems!);
+        Assert.Equal(6, artifact.MaxRetries);
+        Assert.Equal(TimeSpan.FromSeconds(120), artifact.RetryDelay);
+    }
+
    [Fact]
    public async Task RetryForSiteAsync_SingleSitePath_StillRunsTheGlobalQueriesOnce()
    {
@@ -3968,6 +3968,61 @@ public class ManagementActorTests : TestKit, IDisposable
            Arg.Any<CancellationToken>());
    }

+    /// <summary>
+    /// Deferred-work row 27(b): the retry policy had no management surface at all — the only
+    /// way to set it was a direct DB edit of <c>ExternalSystemDefinitions</c>. Create must now
+    /// map both fields onto the entity.
+    /// </summary>
+    [Fact]
+    public void CreateExternalSystem_MapsRetryPolicy()
+    {
+        var extRepo = Substitute.For<IExternalSystemRepository>();
+        _services.AddScoped(_ => extRepo);
+
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new CreateExternalSystemCommand("Sys", "http://x", "none", null,
+                TimeoutSeconds: 15, MaxRetries: 8, RetryDelay: TimeSpan.FromSeconds(60)),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
+        Assert.Equal(envelope.CorrelationId, response.CorrelationId);
+        extRepo.Received(1).AddExternalSystemAsync(
+            Arg.Is<Commons.Entities.ExternalSystems.ExternalSystemDefinition>(
+                d => d.MaxRetries == 8 && d.RetryDelay == TimeSpan.FromSeconds(60)),
+            Arg.Any<CancellationToken>());
+    }
+
+    /// <summary>
+    /// The retry policy follows the same "omit to preserve" rule as TimeoutSeconds, so a
+    /// partial update (e.g. renaming a system) cannot silently reset it to 0.
+    /// </summary>
+    [Fact]
+    public void UpdateExternalSystem_NullRetryPolicy_PreservesExisting()
+    {
+        var existing = new Commons.Entities.ExternalSystems.ExternalSystemDefinition("Sys", "http://x", "none")
+        {
+            MaxRetries = 11,
+            RetryDelay = TimeSpan.FromSeconds(33)
+        };
+        var extRepo = Substitute.For<IExternalSystemRepository>();
+        extRepo.GetExternalSystemByIdAsync(7, Arg.Any<CancellationToken>()).Returns(existing);
+        _services.AddScoped(_ => extRepo);
+
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new UpdateExternalSystemCommand(7, "Renamed", "http://x", "none", null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
+        Assert.Equal(11, existing.MaxRetries);
+        Assert.Equal(TimeSpan.FromSeconds(33), existing.RetryDelay);
+    }
+
    // ========================================================================
    // Area management role gate (arch-review C6): reconciled to any-of
    // [Designer, Deployer]. The Central UI creates areas inside the Deployment
@@ -997,6 +997,35 @@ public class DeploymentManagerActorTests : TestKit, IDisposable
            SmtpConfigurations: null,
            Timestamp: DateTimeOffset.UtcNow);

+    /// <summary>
+    /// Deferred-work row 27: the per-system Store-and-Forward retry policy must survive the
+    /// site APPLY path, not just the storage round-trip. Before the fix
+    /// <c>ExternalSystemArtifact</c> carried no retry fields at all, so a centrally-configured
+    /// policy was dropped on the floor here and every cached call fell back to the engine's
+    /// 50 x 30 s default.
+    /// </summary>
+    [Fact]
+    public async Task ApplyingArtifacts_PersistsExternalSystemRetryPolicy()
+    {
+        var manager = CreateDeploymentManager();
+
+        manager.Tell(ArtifactsCommand("dep-es-retry", externalSystems:
+        [
+            new ExternalSystemArtifact("MES", "https://mes", "ApiKey", null, null, 0,
+                MaxRetries: 9, RetryDelay: TimeSpan.FromSeconds(45))
+        ]));
+        Assert.True(ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(10)).Success);
+
+        await using var connection = _storage.CreateConnection();
+        await using var command = connection.CreateCommand();
+        command.CommandText = "SELECT max_retries, retry_delay_ms FROM external_systems WHERE name = 'MES'";
+        await using var reader = await command.ExecuteReaderAsync();
+
+        Assert.True(await reader.ReadAsync());
+        Assert.Equal(9, reader.GetInt32(0));
+        Assert.Equal(45_000, reader.GetInt64(1));
+    }
+
    [Fact]
    public async Task ApplyingArtifacts_DeletesExternalSystemsAbsentFromTheSet()
    {
@@ -86,6 +86,52 @@ public class SiteRepositoryTests : IDisposable
        Assert.Equal(7, found!.TimeoutSeconds);
    }

+    /// <summary>
+    /// Deferred-work row 27: the per-system Store-and-Forward retry policy
+    /// (<c>MaxRetries</c>/<c>RetryDelay</c>) must round-trip through the site SQLite store,
+    /// so <c>ExternalSystemClient.CachedCallAsync</c> can hand it to the engine instead of
+    /// silently falling back to <c>DefaultMaxRetries</c> (50) x <c>DefaultRetryInterval</c> (30 s).
+    /// Before the fix the site table had no such columns and this read back 0 / TimeSpan.Zero.
+    /// </summary>
+    [Fact]
+    public async Task SiteStorage_ExternalSystem_RetryPolicy_RoundTrips()
+    {
+        var storage = NewStorage();
+        await storage.InitializeAsync();
+        await storage.StoreExternalSystemAsync(
+            "MesApi", "https://mes.example.com", "ApiKey", null, null,
+            timeoutSeconds: 0, maxRetries: 7, retryDelay: TimeSpan.FromSeconds(90));
+
+        var repo = new SiteExternalSystemRepository(storage);
+        var found = await repo.GetExternalSystemByNameAsync("MesApi");
+
+        Assert.NotNull(found);
+        Assert.Equal(7, found!.MaxRetries);
+        Assert.Equal(TimeSpan.FromSeconds(90), found.RetryDelay);
+    }
+
+    /// <summary>
+    /// The unset case stays unset: a system deployed without a retry policy must read back as
+    /// 0 / <see cref="TimeSpan.Zero"/>, which is the value the gateway translates to null so the
+    /// engine defaults apply. This is what makes the new columns behaviourally invisible until
+    /// an operator actually configures a policy.
+    /// </summary>
+    [Fact]
+    public async Task SiteStorage_ExternalSystem_RetryPolicy_DefaultsToUnset()
+    {
+        var storage = NewStorage();
+        await storage.InitializeAsync();
+        await storage.StoreExternalSystemAsync(
+            "PlainApi", "https://plain.example.com", "ApiKey", null, null);
+
+        var repo = new SiteExternalSystemRepository(storage);
+        var found = await repo.GetExternalSystemByNameAsync("PlainApi");
+
+        Assert.NotNull(found);
+        Assert.Equal(0, found!.MaxRetries);
+        Assert.Equal(TimeSpan.Zero, found.RetryDelay);
+    }
+
    /// <summary>
    /// SiteRuntime-007: the synthetic ID for an external system must be identical when
    /// the storage service and repository are re-created (simulating a process restart).
@@ -9,6 +9,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
 using ZB.MOM.WW.ScadaBridge.Transport.Import;
 using ZB.MOM.WW.ScadaBridge.Transport.Serialization;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems;

 namespace ZB.MOM.WW.ScadaBridge.Transport.Tests.Import;

@@ -105,6 +106,56 @@ public sealed class ArtifactDiffTests
        Assert.Null(item.FieldDiffJson);
    }

+    // ============ Deferred-work row 27: ES retry policy in the diff ============
+
+    /// <summary>
+    /// The bundle has always CARRIED the external-system retry policy and
+    /// <c>BundleImporter</c> has always APPLIED it, but <c>CompareExternalSystem</c> never
+    /// compared it — so an import that silently changed a system's retry budget previewed as
+    /// "Identical". Now that the policy actually reaches sites, an unreviewed change to it is a
+    /// live behaviour change, so it must show up in the preview.
+    /// </summary>
+    [Fact]
+    public void ExternalSystem_ChangedRetryPolicy_IsReported()
+    {
+        var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+        {
+            MaxRetries = 3,
+            RetryDelay = TimeSpan.FromSeconds(5),
+        };
+        var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey",
+            MaxRetries: 9, RetryDelay: TimeSpan.FromSeconds(45), Methods: [], Secrets: null);
+
+        var item = _diff.CompareExternalSystem(incoming, existing, []);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+
+        var maxRetries = ChangeFor(item, "MaxRetries");
+        Assert.Equal("3", maxRetries.GetProperty("oldValue").GetString());
+        Assert.Equal("9", maxRetries.GetProperty("newValue").GetString());
+
+        var retryDelay = ChangeFor(item, "RetryDelay");
+        Assert.Equal(TimeSpan.FromSeconds(5).ToString(), retryDelay.GetProperty("oldValue").GetString());
+        Assert.Equal(TimeSpan.FromSeconds(45).ToString(), retryDelay.GetProperty("newValue").GetString());
+    }
+
+    /// <summary>An unchanged retry policy must not manufacture a spurious diff row.</summary>
+    [Fact]
+    public void ExternalSystem_UnchangedRetryPolicy_IsIdentical()
+    {
+        var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+        {
+            MaxRetries = 3,
+            RetryDelay = TimeSpan.FromSeconds(5),
+        };
+        var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey",
+            MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Methods: [], Secrets: null);
+
+        var item = _diff.CompareExternalSystem(incoming, existing, []);
+
+        Assert.Equal(ConflictKind.Identical, item.Kind);
+    }
+
    [Fact]
    public void ApiMethod_ModifiedScript_EmitsStructuredLineDiff()
    {