merge: external-system retry policy end-to-end (register row 27) — MaxRetries/RetryDelay now travel artifact→gRPC→site schema→S&F enqueue, CLI --max-retries/--retry-delay-seconds, ArtifactDiff compares them

tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/ExternalSystemRetryOptionTests.cs

@@ -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"));
+    }
+}

tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteCommandSamples.cs

@@ -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);

tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs

@@ -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()
     {

tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs

@@ -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

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorTests.cs

@@ -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()
     {

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Repositories/SiteRepositoryTests.cs

@@ -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).

tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs

@@ -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()
     {