fix(transport): carry ExternalSystem TimeoutSeconds through bundles + diff (arch-review 2026-08-19 P7-1, row-27 shape)

TimeoutSeconds is the per-system HTTP timeout ExternalSystemClient reads at call time and is deployed to sites (proto field 6), but Transport bundles silently dropped it: the DTO never carried it, export never wrote it, the importer's Add path reset it to 0, the Overwrite path left the target's stale value untouched, and ArtifactDiff never compared it (so a bundle whose only change was a tuned timeout previewed as Identical). Adds TimeoutSeconds as an additive-optional trailing field on ExternalSystemDto (schemaVersion stays 1.1, bundleFormatVersion unchanged) and threads it through EntitySerializer (export + import), BundleImporter (Add and Overwrite), and ArtifactDiff.CompareExternalSystem. Old-bundle semantics pinned: an absent field (null) falls back to the entity default on Add and leaves the target's existing value untouched on Overwrite/diff. Beefed up the RoundTripEquivalenceTests external-system fixture with a non-default TimeoutSeconds so the reflection round-trip guard actually exercises the field instead of comparing default against default. Swept the rest of the ExternalSystemDto shape against ExternalSystemDefinition field-by-field; no other operative field is missing.
2026-08-19 14:57:21 -04:00
parent 013257ec7c
commit 50a77a8170
9 changed files with 383 additions and 2 deletions
@@ -170,6 +170,13 @@ public sealed class ArtifactDiff
        // previewed as "Identical". Mirrors CompareDatabaseConnection below.
        AddIfDifferent(changes, "MaxRetries", existing.MaxRetries, incoming.MaxRetries);
        AddIfDifferent(changes, "RetryDelay", existing.RetryDelay.ToString(), incoming.RetryDelay.ToString());
+        // arch-review 2026-08-19 P7-1 (row-27 shape): TimeoutSeconds is operative at
+        // runtime and the importer has always APPLIED it, but the diff omitted it — same
+        // "silently changed and invisible to preview" defect as the MaxRetries fix above.
+        // A null incoming value (bundle written before this field existed) compares as
+        // "no change" — it mirrors BundleImporter's Overwrite fallback, which leaves the
+        // target's existing timeout untouched in that case.
+        AddIfDifferent(changes, "TimeoutSeconds", existing.TimeoutSeconds, incoming.TimeoutSeconds ?? existing.TimeoutSeconds);

        // Secrets: presence-only comparison (we never echo the value in the diff).
        var existingHasSecret = !string.IsNullOrEmpty(existing.AuthConfiguration);
@@ -3090,6 +3090,9 @@ public sealed class BundleImporter : IBundleImporter
                    existing.AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null;
                    existing.MaxRetries = dto.MaxRetries;
                    existing.RetryDelay = dto.RetryDelay;
+                    // A bundle written before this field existed carries null — leave the
+                    // target's existing timeout untouched rather than resetting it to 0/unset.
+                    existing.TimeoutSeconds = dto.TimeoutSeconds ?? existing.TimeoutSeconds;
                    await _externalRepo.UpdateExternalSystemAsync(existing, ct).ConfigureAwait(false);
                    await _auditService.LogAsync(user, "Update", "ExternalSystem", existing.Id.ToString(), existing.Name,
                        new { existing.Name, existing.EndpointUrl }, ct).ConfigureAwait(false);
@@ -3123,6 +3126,9 @@ public sealed class BundleImporter : IBundleImporter
            AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null,
            MaxRetries = dto.MaxRetries,
            RetryDelay = dto.RetryDelay,
+            // Null (a bundle written before this field existed) falls back to the
+            // entity's own "0 = unset" default.
+            TimeoutSeconds = dto.TimeoutSeconds ?? 0,
        };
        return sys;
    }
@@ -252,7 +252,14 @@ public sealed record ExternalSystemDto(
    int MaxRetries,
    TimeSpan RetryDelay,
    IReadOnlyList<ExternalSystemMethodDto> Methods,
-    SecretsBlock? Secrets);
+    SecretsBlock? Secrets,
+    // Per-system HTTP round-trip timeout (arch-review 2026-08-19 P7-1, row-27 shape).
+    // Additive trailing field; null distinguishes a bundle written before this field
+    // existed from a bundle that explicitly carries the entity's own "0 = unset"
+    // sentinel. BundleImporter treats a null incoming value as "not specified by this
+    // bundle": Add falls back to the entity default (0), Overwrite leaves the target's
+    // existing value untouched.
+    int? TimeoutSeconds = null);

 public sealed record ExternalSystemMethodDto(
    string Name,
@@ -139,7 +139,8 @@ public sealed class EntitySerializer
                    MaxRetries: sys.MaxRetries,
                    RetryDelay: sys.RetryDelay,
                    Methods: methods,
-                    Secrets: secrets);
+                    Secrets: secrets,
+                    TimeoutSeconds: sys.TimeoutSeconds);
            }).ToList(),
            DatabaseConnections: aggregate.DatabaseConnections.Select(dc => new DatabaseConnectionDto(
                Name: dc.Name,
@@ -433,6 +434,9 @@ public sealed class EntitySerializer
                AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null,
                MaxRetries = dto.MaxRetries,
                RetryDelay = dto.RetryDelay,
+                // Null (a bundle written before this field existed) falls back to the
+                // entity's own "0 = unset" default.
+                TimeoutSeconds = dto.TimeoutSeconds ?? 0,
            };
            externalSystems.Add(sys);
            foreach (var m in dto.Methods)
@@ -789,6 +789,194 @@ public sealed class BundleImporterApplyTests : IDisposable
        Assert.Equal(1, result.Overwritten);
    }

+    [Fact]
+    public async Task ApplyAsync_adds_new_external_system_with_TimeoutSeconds()
+    {
+        // arch-review 2026-08-19 P7-1 (row-27 shape): TimeoutSeconds is the
+        // per-system HTTP timeout ExternalSystemClient reads at call time — it must
+        // survive Add, not just the retry policy (T-002 above covers the sibling
+        // Methods-drop regression on Overwrite; this is the Add-path fidelity gap).
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey")
+            {
+                TimeoutSeconds = 120,
+            });
+            await ctx.SaveChangesAsync();
+        }
+        var sessionId = await ExportAndLoadAsync();
+
+        // Wipe so the import exercises the Add path.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.ExternalSystemDefinitions.RemoveRange(ctx.ExternalSystemDefinitions);
+            await ctx.SaveChangesAsync();
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            await importer.ApplyAsync(sessionId,
+                new List<ImportResolution> { new("ExternalSystem", "Erp", ResolutionAction.Add, null) },
+                user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp");
+            Assert.Equal(120, sys.TimeoutSeconds);
+        }
+    }
+
+    [Fact]
+    public async Task ApplyAsync_Overwrite_restores_TimeoutSeconds_to_bundle_value()
+    {
+        // Seed with TimeoutSeconds=120, export, drift the target to 45, then Overwrite
+        // and assert the bundle's value wins (mirrors the retry-policy Overwrite
+        // coverage — TimeoutSeconds was the one scalar field the Overwrite branch never
+        // touched at all, so it silently kept whatever the target already had).
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey")
+            {
+                TimeoutSeconds = 120,
+            });
+            await ctx.SaveChangesAsync();
+        }
+        var sessionId = await ExportAndLoadAsync();
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp");
+            sys.TimeoutSeconds = 45; // now the only field that differs from the bundle
+            await ctx.SaveChangesAsync();
+        }
+
+        ImportResult result;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            result = await importer.ApplyAsync(sessionId,
+                new List<ImportResolution> { new("ExternalSystem", "Erp", ResolutionAction.Overwrite, null) },
+                user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp");
+            Assert.Equal(120, sys.TimeoutSeconds);
+        }
+        Assert.Equal(1, result.Overwritten);
+    }
+
+    // ─────────────────────────────────────────────────────────────────────
+    // arch-review 2026-08-19 P7-1: old-bundle semantics for TimeoutSeconds. Hand-packs
+    // a BundleContentDto with an ExternalSystemDto whose TimeoutSeconds is left at its
+    // default (null) — the same in-memory shape a pre-P7-1 bundle deserializes to,
+    // since the field is simply absent from that JSON text.
+    // ─────────────────────────────────────────────────────────────────────
+    private async Task<Guid> PackAndLoadExternalSystemBundleAsync(params ExternalSystemDto[] externalSystems)
+    {
+        var content = new BundleContentDto(
+            TemplateFolders: Array.Empty<TemplateFolderDto>(),
+            Templates: Array.Empty<TemplateDto>(),
+            SharedScripts: Array.Empty<SharedScriptDto>(),
+            ExternalSystems: externalSystems,
+            DatabaseConnections: Array.Empty<DatabaseConnectionDto>(),
+            NotificationLists: Array.Empty<NotificationListDto>(),
+            SmtpConfigs: Array.Empty<SmtpConfigDto>(),
+            ApiMethods: Array.Empty<ApiMethodDto>());
+
+        await using var scope = _provider.CreateAsyncScope();
+        var serializer = scope.ServiceProvider.GetRequiredService<BundleSerializer>();
+        var manifestBuilder = scope.ServiceProvider.GetRequiredService<ManifestBuilder>();
+        var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+
+        var contentBytes = serializer.SerializeContentBytes(content);
+        var manifest = manifestBuilder.Build(
+            sourceEnvironment: "dev",
+            exportedBy: "alice",
+            scadaBridgeVersion: "1.0.0",
+            encryption: null,
+            summary: new BundleSummary(0, 0, 0, externalSystems.Length, 0, 0, 0, 0),
+            contents: Array.Empty<ManifestContentEntry>(),
+            contentBytes: contentBytes);
+        await using var packed = serializer.Pack(content, manifest, passphrase: null, encryptor: null);
+
+        using var ms = new MemoryStream();
+        await packed.CopyToAsync(ms);
+        ms.Position = 0;
+        var session = await importer.LoadAsync(ms, passphrase: null);
+        return session.SessionId;
+    }
+
+    [Fact]
+    public async Task ApplyAsync_Add_with_old_bundle_missing_TimeoutSeconds_uses_entity_default()
+    {
+        var dto = new ExternalSystemDto("Erp", "https://erp.example", "ApiKey",
+            MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Methods: [], Secrets: null,
+            TimeoutSeconds: null); // old-bundle shape — the field never existed on the wire.
+        var sessionId = await PackAndLoadExternalSystemBundleAsync(dto);
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            await importer.ApplyAsync(sessionId,
+                new List<ImportResolution> { new("ExternalSystem", "Erp", ResolutionAction.Add, null) },
+                user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp");
+            Assert.Equal(0, sys.TimeoutSeconds); // entity default, not an exception or -1.
+        }
+    }
+
+    [Fact]
+    public async Task ApplyAsync_Overwrite_with_old_bundle_missing_TimeoutSeconds_preserves_target_value()
+    {
+        // The target already has a tuned timeout; an old bundle that predates this field
+        // must not reset it to 0 on Overwrite — there is nothing on the wire to apply, so
+        // the existing value stands.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey")
+            {
+                TimeoutSeconds = 90,
+            });
+            await ctx.SaveChangesAsync();
+        }
+
+        var dto = new ExternalSystemDto("Erp", "https://erp.example", "ApiKey",
+            MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Methods: [], Secrets: null,
+            TimeoutSeconds: null); // old-bundle shape.
+        var sessionId = await PackAndLoadExternalSystemBundleAsync(dto);
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            await importer.ApplyAsync(sessionId,
+                new List<ImportResolution> { new("ExternalSystem", "Erp", ResolutionAction.Overwrite, null) },
+                user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp");
+            Assert.Equal(90, sys.TimeoutSeconds); // untouched — bundle carried no opinion.
+        }
+    }
+
    // ─────────────────────────────────────────────────────────────────────
    // S10b: SMS provider config bundle apply. Mirrors the SMTP apply path —
    // create a new config (Add) and overwrite an existing one (matched by the
@@ -447,6 +447,11 @@ public sealed class RoundTripEquivalenceTests : IDisposable
                AuthConfiguration = "{\"apiKey\":\"super-secret\"}", // rides SecretsBlock
                MaxRetries = 6,
                RetryDelay = TimeSpan.FromSeconds(17),
+                // arch-review 2026-08-19 P7-1: a default-0 fixture value made this
+                // reflection guard vacuous for TimeoutSeconds (comparing default against
+                // default survives even a serializer that drops the field entirely) — a
+                // non-default value makes the guard actually exercise the field.
+                TimeoutSeconds = 120,
            };
            ctx.ExternalSystemDefinitions.Add(sys);
            await ctx.SaveChangesAsync();
@@ -156,6 +156,70 @@ public sealed class ArtifactDiffTests
        Assert.Equal(ConflictKind.Identical, item.Kind);
    }

+    // ============ arch-review 2026-08-19 P7-1: TimeoutSeconds in the diff (row-27 shape) ============
+
+    /// <summary>
+    /// The bundle has always CARRIED TimeoutSeconds and BundleImporter has always APPLIED it
+    /// (the per-system HTTP timeout used by ExternalSystemClient), but CompareExternalSystem
+    /// never compared it — so an import whose ONLY change was a tuned timeout previewed as
+    /// "Identical" and the operator had no diff evidence the value was about to change.
+    /// </summary>
+    [Fact]
+    public void ExternalSystem_ChangedTimeoutSeconds_IsReported()
+    {
+        var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+        {
+            TimeoutSeconds = 30,
+        };
+        var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey",
+            MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: 120);
+
+        var item = _diff.CompareExternalSystem(incoming, existing, []);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+
+        var timeout = ChangeFor(item, "TimeoutSeconds");
+        Assert.Equal("30", timeout.GetProperty("oldValue").GetString());
+        Assert.Equal("120", timeout.GetProperty("newValue").GetString());
+    }
+
+    /// <summary>An unchanged timeout must not manufacture a spurious diff row.</summary>
+    [Fact]
+    public void ExternalSystem_UnchangedTimeoutSeconds_IsIdentical()
+    {
+        var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+        {
+            TimeoutSeconds = 30,
+        };
+        var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey",
+            MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: 30);
+
+        var item = _diff.CompareExternalSystem(incoming, existing, []);
+
+        Assert.Equal(ConflictKind.Identical, item.Kind);
+    }
+
+    /// <summary>
+    /// Old-bundle semantics: a bundle written before this field existed carries
+    /// <c>TimeoutSeconds: null</c>. The diff must treat that as "not specified by this
+    /// bundle" — same as BundleImporter's Overwrite fallback — rather than reading it as
+    /// "reset to 0/unset" and manufacturing a spurious change.
+    /// </summary>
+    [Fact]
+    public void ExternalSystem_MissingTimeoutSeconds_ComparesAsUnchanged()
+    {
+        var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey")
+        {
+            TimeoutSeconds = 30,
+        };
+        var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey",
+            MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: null);
+
+        var item = _diff.CompareExternalSystem(incoming, existing, []);
+
+        Assert.Equal(ConflictKind.Identical, item.Kind);
+    }
+
    [Fact]
    public void ApiMethod_ModifiedScript_EmitsStructuredLineDiff()
    {
@@ -201,6 +201,52 @@ public sealed class BundleDtoSerializationTests
        Assert.Empty(content.Instances);
    }

+    // --- arch-review 2026-08-19 P7-1: ExternalSystem TimeoutSeconds wire format ---
+
+    /// <summary>
+    /// Pins the old-bundle semantics: a bundle written before <c>TimeoutSeconds</c>
+    /// existed carries no such property in the JSON text at all (not even a null). It
+    /// must deserialize to a null DTO value — never to 0, which is a distinct, meaningful
+    /// wire value (the entity's own "unset, gateway default applies" sentinel) — so
+    /// BundleImporter can tell "not specified by this bundle" apart from "explicitly
+    /// unset" and apply the documented fallback (Add: entity default; Overwrite: leave
+    /// the target's existing value untouched).
+    /// </summary>
+    [Fact]
+    public void LegacyExternalSystem_MissingTimeoutSeconds_DeserializesToNull()
+    {
+        const string legacyJson = """
+            {
+              "TemplateFolders": [],
+              "Templates": [],
+              "SharedScripts": [],
+              "ExternalSystems": [
+                {
+                  "Name": "erp",
+                  "BaseUrl": "https://erp/api",
+                  "AuthType": "ApiKey",
+                  "MaxRetries": 3,
+                  "RetryDelay": "00:00:05",
+                  "Methods": [],
+                  "Secrets": null
+                }
+              ],
+              "DatabaseConnections": [],
+              "NotificationLists": [],
+              "SmtpConfigs": [],
+              "ApiMethods": []
+            }
+            """;
+
+        var content = JsonSerializer.Deserialize<BundleContentDto>(legacyJson, Options);
+
+        Assert.NotNull(content);
+        var sys = Assert.Single(content!.ExternalSystems);
+        Assert.Equal("erp", sys.Name);
+        Assert.Equal(3, sys.MaxRetries);
+        Assert.Null(sys.TimeoutSeconds);
+    }
+
    // --- S10: SMS recipient PhoneNumber + SmsConfig wire format ---------------

    [Fact]
@@ -340,6 +340,60 @@ public sealed class EntitySerializerTests
        Assert.Equal(TimeSpan.FromSeconds(15), rtSys.RetryDelay);
    }

+    /// <summary>
+    /// arch-review 2026-08-19 P7-1 (row-27 shape): <c>TimeoutSeconds</c> is operative at
+    /// runtime (the per-system HTTP timeout) but was silently dropped by the export/import
+    /// DTO — same defect class as the earlier <c>MaxRetries</c>/<c>RetryDelay</c> holes this
+    /// test's sibling covers.
+    /// </summary>
+    [Fact]
+    public void Roundtrip_external_system_preserves_timeout_seconds()
+    {
+        var sys = new ExternalSystemDefinition("billing", "https://billing/api", "Basic")
+        {
+            Id = 1,
+            TimeoutSeconds = 120,
+        };
+        var aggregate = MakeEmptyAggregate() with { ExternalSystems = new[] { sys } };
+
+        var sut = new EntitySerializer();
+        var dto = sut.ToBundleContent(aggregate);
+        Assert.Equal(120, Assert.Single(dto.ExternalSystems).TimeoutSeconds);
+
+        var roundTripped = sut.FromBundleContent(dto);
+
+        var rtSys = Assert.Single(roundTripped.ExternalSystems);
+        Assert.Equal("billing", rtSys.Name);
+        Assert.Equal(120, rtSys.TimeoutSeconds);
+    }
+
+    /// <summary>
+    /// Pins the old-bundle semantics: a DTO with no <c>TimeoutSeconds</c> at all (the
+    /// property default, as an old bundle deserializes it) falls back to the entity's own
+    /// "0 = unset" default on import — it must not throw or coerce to some other sentinel.
+    /// </summary>
+    [Fact]
+    public void FromDto_with_null_TimeoutSeconds_yields_entity_default_zero()
+    {
+        var dto = new BundleContentDto(
+            TemplateFolders: Array.Empty<TemplateFolderDto>(),
+            Templates: Array.Empty<TemplateDto>(),
+            SharedScripts: Array.Empty<SharedScriptDto>(),
+            ExternalSystems: new[]
+            {
+                new ExternalSystemDto("erp", "https://x", "None", MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Array.Empty<ExternalSystemMethodDto>(), Secrets: null),
+            },
+            DatabaseConnections: Array.Empty<DatabaseConnectionDto>(),
+            NotificationLists: Array.Empty<NotificationListDto>(),
+            SmtpConfigs: Array.Empty<SmtpConfigDto>(),
+            ApiMethods: Array.Empty<ApiMethodDto>());
+
+        var aggregate = new EntitySerializer().FromBundleContent(dto);
+
+        var sys = Assert.Single(aggregate.ExternalSystems);
+        Assert.Equal(0, sys.TimeoutSeconds);
+    }
+
    [Fact]
    public void FromDto_with_null_SecretsBlock_yields_entity_with_default_empty_secrets()
    {