From 5cbc143b013ebd4e95275b06239020f63c8ba326 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 14:52:00 -0400 Subject: [PATCH] fix(deploymentmanager): bulk deploy skips Disabled + reconcile circuit-breaker + probe captures pre-flatten versions (arch-review 2026-08-19 P7-2 + P7-3 + P7-4) --- .../Component-DeploymentManager.md | 20 ++ .../DeploymentManagerOptions.cs | 19 ++ .../DeploymentManagerOptionsValidator.cs | 5 + .../DeploymentService.cs | 215 ++++++++++++++++-- .../FlattenSession.cs | 17 ++ .../StaleInstanceProbe.cs | 23 +- .../DeploySiteAsyncTests.cs | 156 ++++++++++++- .../DeploymentManagerOptionsValidatorTests.cs | 19 ++ .../DeploymentServiceTests.cs | 30 +++ .../StaleInstanceProbeTests.cs | 69 ++++++ 10 files changed, 541 insertions(+), 32 deletions(-) diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index 18951319..7d29c4fd 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -169,6 +169,26 @@ one is parallel: 3. **Finalize (serial).** Commit terminal statuses, apply post-success side effects, write audit rows, release each operation lock. +**Disabled instances are skipped.** A bulk site deploy never touches an instance +in `Disabled` — no flatten, no send, no state change — because deploy is a legal +transition out of `Disabled` and forces `Enabled` on success, so an unfiltered +site-wide push would silently restart every piece of equipment an operator had +deliberately stopped; they are reported in the summary's `SkippedInstances` with +the reason "Disabled — bulk deploy preserves disabled state; deploy individually +to re-enable", and the single-instance deploy remains the explicit re-enable path. + +**Reconciliation is bounded per batch.** Phase 1 is serial, so an unreachable site +would otherwise cost the batch one full communication-layer `QueryTimeout` (30 s) +per instance before a single deploy was sent — hours for a large site, past the +CLI's 5-minute bulk deadline, which cancels and re-arms the identical cost on the +next attempt. Each reconcile in a bulk deploy therefore runs under the short +`SiteDeploymentReconcileTimeout` (default **5 s**), and the FIRST one to hit that +deadline opens a once-per-batch circuit-breaker that skips the remaining +reconciles (one summary log line, not N). Skipping costs nothing: a timed-out +reconcile already falls through to a normal deploy on central's recorded state, +with the site's own stale-revision rejection as the safety net. The +single-instance path is unbounded by this option and keeps the full `QueryTimeout`. + **Not all-or-nothing.** An instance that fails for any reason (wrong state, failed validation, lock already held by another operation, site round-trip timed out) is reported as a failed row while the rest proceed, and is individually retryable via diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs index 46f905c7..d97e0d91 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs @@ -41,6 +41,25 @@ public class DeploymentManagerOptions /// public TimeSpan SiteDeploymentTimeoutPerInstance { get; set; } = TimeSpan.FromSeconds(120); + /// + /// Deadline applied to each query-before-redeploy reconciliation round-trip + /// inside a bulk DeploySiteAsync (the single-instance path keeps the + /// communication layer's own QueryTimeout). + /// + /// + /// Phase 1 is strictly serial, so an unreachable site otherwise costs + /// QueryTimeout (30 s) PER INSTANCE before a single deploy is sent — for + /// a 500-instance site roughly four hours, far past the CLI's 5-minute bulk + /// deploy timeout, which then cancels and re-arms the identical cost on the next + /// attempt. Deliberately short (seconds): a reconcile that does not answer + /// promptly falls through to a normal deploy anyway, which is exactly what its + /// own timeout produces. Works together with the once-per-batch reachability + /// circuit-breaker, which skips the remaining reconciles after the first + /// timeout. + /// + /// + public TimeSpan SiteDeploymentReconcileTimeout { get; set; } = TimeSpan.FromSeconds(5); + /// /// Retention window for TERMINAL deployment records (Success, /// Failed). Deployments are insert-only — one row per attempt, forever — diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptionsValidator.cs index a4431212..41a83060 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptionsValidator.cs @@ -38,6 +38,11 @@ public sealed class DeploymentManagerOptionsValidator : OptionsValidatorBase TimeSpan.Zero, + $"ScadaBridge:DeploymentManager:SiteDeploymentReconcileTimeout must be a positive duration " + + $"(was {options.SiteDeploymentReconcileTimeout}); it bounds each query-before-redeploy " + + $"reconciliation inside a bulk site deployment's serial prepare phase."); + builder.RequireThat(options.TerminalDeploymentRecordRetention >= TimeSpan.Zero, $"ScadaBridge:DeploymentManager:TerminalDeploymentRecordRetention must not be negative " + $"(was {options.TerminalDeploymentRecordRetention}); use TimeSpan.Zero to disable purging of " + diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index f88a6111..b9ba4e43 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -57,6 +57,13 @@ public class DeploymentService /// private const string TimeoutFailurePrefix = "Communication failure:"; + /// + /// Reason reported on every a bulk site + /// deploy produces for a instance. + /// + internal const string DisabledSkipReason = + "Disabled — bulk deploy preserves disabled state; deploy individually to re-enable"; + /// Rows deleted per terminal-record purge batch. private const int TerminalPurgeBatchSize = 500; @@ -185,8 +192,13 @@ public class DeploymentService // out of the same phase helpers is what keeps deployment identity, // idempotency, lock coverage and optimistic concurrency from drifting apart // between them. + // reconcileGate is null on this path on purpose: the batch-wide reconcile + // budget exists to stop a serial phase-1 loop multiplying one unreachable + // site's query timeout by the instance count. A single instance pays it + // once, and this is the explicit operator path — it keeps the full + // communication-layer QueryTimeout. var prepared = await PrepareDeploymentAsync( - instanceId, user, session: null, cancellationToken); + instanceId, user, session: null, reconcileGate: null, cancellationToken); if (prepared.EarlyResult is { } early) return early; @@ -230,6 +242,11 @@ public class DeploymentService /// one. Sharing it across a batch is what collapses N identical template-chain /// walks into one. /// + /// + /// Batch-wide reconcile budget + reachability circuit-breaker for a bulk + /// deploy, or on the single-instance path (which keeps + /// the communication layer's own query timeout). + /// /// Cancellation token. /// /// A prepared deployment ready to send, or one carrying @@ -240,6 +257,7 @@ public class DeploymentService int instanceId, string user, FlattenSession? session, + BulkReconcileGate? reconcileGate, CancellationToken cancellationToken) { // Load instance @@ -258,7 +276,8 @@ public class DeploymentService try { - return await PrepareUnderLockAsync(instance, user, session, lockHandle, cancellationToken); + return await PrepareUnderLockAsync( + instance, user, session, reconcileGate, lockHandle, cancellationToken); } catch { @@ -279,6 +298,7 @@ public class DeploymentService Instance instance, string user, FlattenSession? session, + BulkReconcileGate? reconcileGate, IDisposable lockHandle, CancellationToken cancellationToken) { @@ -329,8 +349,11 @@ public class DeploymentService // duplicate deployment is not produced (design: "Deployment Identity & // Idempotency"). A clean prior Success or a fresh first-time deploy // skips this extra round-trip. - var reconciled = await TryReconcileWithSiteAsync( - instance, revisionHash, configJson, user, cancellationToken); + var reconciled = reconcileGate is null + ? await TryReconcileWithSiteAsync( + instance, revisionHash, configJson, user, cancellationToken) + : await ReconcileUnderBatchGateAsync( + instance, revisionHash, configJson, user, reconcileGate, cancellationToken); if (reconciled != null) { lockHandle.Dispose(); @@ -745,6 +768,36 @@ public class DeploymentService /// private readonly record struct SendOutcome(DeploymentStatusResponse? Response, Exception? Error); + /// + /// A bulk deploy's shared query-before-redeploy budget: the short per-reconcile + /// deadline every instance in the batch runs under, plus the once-per-batch + /// reachability circuit-breaker that skips the rest after the first timeout. + /// See for why both bounds exist. + /// + /// + /// Deliberately not thread-safe: it is only ever touched from the serial + /// prepare phase. + /// + /// + /// Deadline applied to each reconcile round-trip. + private sealed class BulkReconcileGate(TimeSpan reconcileTimeout) + { + /// Deadline applied to each reconcile round-trip in this batch. + public TimeSpan ReconcileTimeout { get; } = reconcileTimeout; + + /// True once a reconcile in this batch hit its deadline. + public bool SiteUnreachable { get; private set; } + + /// Number of reconciles the circuit-breaker skipped, for the batch's one summary log line. + public int SkippedCount { get; private set; } + + /// Opens the circuit for the remainder of the batch. + public void MarkUnreachable() => SiteUnreachable = true; + + /// Counts one reconcile skipped because the circuit is open. + public void CountSkipped() => SkippedCount++; + } + /// /// Deploy every deployable instance at a site in one operation. /// @@ -783,9 +836,18 @@ public class DeploymentService /// — its own deployment id and revision hash, /// its own operation lock held across the whole operation, its own /// optimistically-concurrent status record, and the same query-before-redeploy - /// idempotency check. Instances that cannot be deployed (wrong state, failed - /// validation, lock already held) are reported as failed rows; the batch is - /// NOT all-or-nothing, mirroring the artifact deployment path. + /// idempotency check — with two batch-level differences. Instances that cannot + /// be deployed (wrong state, failed validation, lock already held) are reported + /// as failed rows; the batch is NOT all-or-nothing, mirroring the artifact + /// deployment path. + /// + /// + /// + /// Disabled instances are skipped entirely and reported in + /// — see the filter below + /// for why. Reconciliation is bounded per batch by + /// , so an unreachable site costs the + /// batch one short timeout rather than one 30-second query per instance. /// /// /// The database ID of the site whose instances are deployed. @@ -803,12 +865,30 @@ public class DeploymentService var instances = await _siteRepository.GetInstancesBySiteIdAsync(siteId, cancellationToken); if (instances.Count == 0) - return Result.Success(new SiteDeploymentSummary(site.SiteIdentifier, [], 0, 0)); + return Result.Success(new SiteDeploymentSummary(site.SiteIdentifier, [], 0, 0, [])); + + // Deliberately-Disabled instances are NOT part of a bulk deploy. Deploy is a + // legal transition out of Disabled and the single-instance path uses it to + // deploy-and-enable, so including them here would have one site-wide push + // silently START every piece of equipment an operator had deliberately + // stopped. They are reported as skipped rows instead — no flatten, no send, + // no state change — and remain individually deployable, which is the + // explicit "yes, re-enable this one" action. + var deployable = new List(instances.Count); + var skipped = new List(); + foreach (var instance in instances) + { + if (instance.State == InstanceState.Disabled) + skipped.Add(new SkippedInstanceResult(instance.Id, instance.UniqueName, DisabledSkipReason)); + else + deployable.Add(instance); + } // ---- Phase 1: prepare, serially, on ONE shared flatten session. ---- var session = _flatteningPipeline.CreateSession(); - var prepared = new List(instances.Count); - var results = new List(instances.Count); + var reconcileGate = new BulkReconcileGate(_options.SiteDeploymentReconcileTimeout); + var prepared = new List(deployable.Count); + var results = new List(deployable.Count); // Index of the first prepared deployment phase 3 has NOT yet finalised. // Everything from here to the end of `prepared` still owns an operation @@ -817,14 +897,15 @@ public class DeploymentService var finalizedUpTo = 0; try { - foreach (var instance in instances) + foreach (var instance in deployable) { cancellationToken.ThrowIfCancellationRequested(); PreparedDeployment step; try { - step = await PrepareDeploymentAsync(instance.Id, user, session, cancellationToken); + step = await PrepareDeploymentAsync( + instance.Id, user, session, reconcileGate, cancellationToken); } catch (Exception ex) { @@ -891,13 +972,26 @@ public class DeploymentService site.SiteIdentifier, results, results.Count(r => r.Success), - results.Count(r => !r.Success)); + results.Count(r => !r.Success), + skipped); + + // ONE summary line for the whole batch, never one per skipped reconcile: + // an unreachable site would otherwise log N warnings for a single fact. + if (reconcileGate.SkippedCount > 0) + { + _logger.LogWarning( + "Site {SiteId} did not answer the query-before-redeploy reconciliation within {Timeout}; " + + "skipped it for the remaining {SkippedCount} instance(s) of this bulk deploy — they proceed " + + "on central's recorded state, with the site's own stale-revision rejection as the safety net", + site.SiteIdentifier, reconcileGate.ReconcileTimeout, reconcileGate.SkippedCount); + } _logger.LogInformation( - "Bulk deployment of site {SiteId} complete: {SuccessCount} succeeded, {FailureCount} failed " + + "Bulk deployment of site {SiteId} complete: {SuccessCount} succeeded, {FailureCount} failed, " + + "{SkippedCount} skipped as Disabled " + "({ChainLoads} template-chain load(s) across {InstanceCount} instance(s))", - site.SiteIdentifier, summary.SuccessCount, summary.FailureCount, - session.ChainLoads, instances.Count); + site.SiteIdentifier, summary.SuccessCount, summary.FailureCount, skipped.Count, + session.ChainLoads, deployable.Count); return Result.Success(summary); } @@ -1671,6 +1765,73 @@ public class DeploymentService return null; } + /// + /// Runs for one instance of a BULK + /// deploy, under the batch's short per-reconcile deadline and its + /// once-per-batch reachability circuit-breaker. + /// + /// + /// Phase 1 is strictly serial, so an unreachable site makes every instance pay + /// the full communication-layer QueryTimeout (30 s) in turn before a + /// single deploy is sent — 500 instances is roughly four hours, so the CLI's + /// 5-minute bulk-deploy deadline cancels the operation and the next attempt + /// re-arms the identical cost. A bulk redeploy could therefore never converge + /// while the site was down. Two bounds fix that: a short + /// per + /// reconcile, and — after the FIRST timeout — skipping the remaining reconciles + /// for the rest of the batch. + /// + /// + /// + /// Skipping loses nothing: a reconcile that times out already returns + /// and the instance proceeds on DB state with the site's + /// own stale-revision rejection as the safety net — which is exactly what every + /// subsequent reconcile against a down site would have produced, one 30-second + /// wait at a time. + /// + /// + /// + /// The gate needs no synchronisation: it is touched only from the serial + /// prepare phase. + /// + /// + /// Instance being prepared. + /// Revision hash this deploy would apply. + /// Serialized flattened config for the reconciled snapshot. + /// User attributed on the reconciliation audit row. + /// The batch's reconcile budget and reachability state. + /// Cancellation token for the batch. + /// The reconciled prior record, or to proceed with a normal deploy. + private async Task ReconcileUnderBatchGateAsync( + Instance instance, + string targetRevisionHash, + string configJson, + string currentUser, + BulkReconcileGate gate, + CancellationToken cancellationToken) + { + if (gate.SiteUnreachable) + { + gate.CountSkipped(); + return null; + } + + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(gate.ReconcileTimeout); + + var reconciled = await TryReconcileWithSiteAsync( + instance, targetRevisionHash, configJson, currentUser, cts.Token); + + // TryReconcileWithSiteAsync swallows its own query faults (it must — a + // failed query never aborts a deploy), so the deadline is detected on the + // token rather than on an exception. An outer cancellation is the caller + // aborting the whole batch, not evidence about the site. + if (reconciled == null && cts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + gate.MarkUnreachable(); + + return reconciled; + } + /// /// The site is queried before a re-deploy only when a /// prior record is stuck , or is @@ -1901,6 +2062,18 @@ public record InstanceDeploymentResult( bool Success, string? ErrorMessage); +/// +/// One instance a bulk site deployment did not attempt at all — no flatten, no +/// site round-trip, and no state change of any kind. +/// +/// Database id of the instance. +/// The instance's unique name. +/// Why it was skipped, phrased for the operator. +public record SkippedInstanceResult( + int InstanceId, + string UniqueName, + string Reason); + /// /// Result matrix of a bulk site deployment. Mirrors the shape of /// ArtifactDeploymentSummary: successes are not rolled back when other @@ -1911,8 +2084,16 @@ public record InstanceDeploymentResult( /// Per-instance outcomes. /// Number of instances the site confirmed. /// Number of instances that failed for any reason. +/// +/// Instances the batch deliberately did not attempt — currently the +/// ones, which a bulk push must not silently +/// re-enable. They are neither successes nor failures, so they are counted in +/// neither total. Additive and nullable so an older client deserialising this +/// summary is unaffected. +/// public record SiteDeploymentSummary( string SiteIdentifier, IReadOnlyList InstanceResults, int SuccessCount, - int FailureCount); + int FailureCount, + IReadOnlyList? SkippedInstances = null); diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlattenSession.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlattenSession.cs index 6c0fe23c..3a7656ad 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlattenSession.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlattenSession.cs @@ -86,6 +86,23 @@ public sealed class FlattenSession /// public IReadOnlyCollection VisitedTemplateIds => _templates.Keys; + /// + /// Every visited template id paired with the watermark version this session + /// READ it at — the pre-flatten watermark the staleness fast path must memoise + /// against. + /// + /// + /// Re-reading the versions after the flatten instead is a correctness bug, not + /// an optimisation detail: an edit that lands mid-flatten pairs the PRE-edit + /// content (and therefore the pre-edit hash) with the POST-edit version, and + /// the memo then answers "not stale" until something else moves the watermark. + /// Recording the version each template was read at can only ever cause an extra + /// flatten, never a stale answer. + /// + /// + public IReadOnlyDictionary VisitedTemplateVersions => + _templates.ToDictionary(entry => entry.Key, entry => entry.Value.Version); + /// /// Returns the inheritance chain for (the /// template itself, then each ancestor), loading it through diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs index 7d93a7fb..4801db90 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs @@ -62,7 +62,7 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe /// /// Graph structure version at flatten time. /// The instance's own version at flatten time. - /// Version of every template the flatten walked, by template id. + /// Version every template the flatten walked was READ at, by template id. /// The hash that reading produced. private sealed record ProbeMemo( long Structure, @@ -85,10 +85,15 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe // script-compilation stage (this probe is called per-instance across a whole // bundle import in BundleImporter.ComputeStaleInstanceIdsAsync). // - // Watermark readings are taken BEFORE the flatten. A mutation that lands - // during the flatten therefore bumps a version past the recorded one and - // invalidates the memo on the next read, rather than being captured as if - // it had already been included. + // Watermark readings are taken BEFORE the flatten they describe. A mutation + // that lands during the flatten therefore bumps a version past the recorded + // one and invalidates the memo on the next read, rather than being captured + // as if it had already been included. The structure and instance versions + // are read here; the per-template versions come from the session, which + // records each template's version at the moment it read that template (see + // FlattenSession.VisitedTemplateVersions) — re-reading them after the + // flatten would pair a pre-edit hash with a post-edit version and make the + // memo serve "not stale" indefinitely. var structure = _watermark.StructureVersion; var instanceVersion = _watermark.GetInstanceVersion(instanceId); @@ -103,12 +108,8 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe if (Memos.Count >= MaxCachedProbes) Memos.Clear(); - var visited = session.VisitedTemplateIds; - var versions = new Dictionary(visited.Count); - foreach (var templateId in visited) - versions[templateId] = _watermark.GetTemplateVersion(templateId); - - Memos[instanceId] = new ProbeMemo(structure, instanceVersion, versions, result.Value.RevisionHash); + Memos[instanceId] = new ProbeMemo( + structure, instanceVersion, session.VisitedTemplateVersions, result.Value.RevisionHash); return result.Value.RevisionHash; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs index 0a07338b..4871d7fd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs @@ -45,7 +45,9 @@ public class DeploySiteAsyncTests : TestKit } /// Seeds deployable instances at the test site. - private List ArrangeInstances(int count) + /// How many instances to seed. + /// Ids seeded Disabled instead of NotDeployed. + private List ArrangeInstances(int count, params int[] disabledIds) { var instances = new List(); for (var i = 1; i <= count; i++) @@ -55,7 +57,7 @@ public class DeploySiteAsyncTests : TestKit Id = i, SiteId = SiteId, TemplateId = 10, - State = InstanceState.NotDeployed + State = disabledIds.Contains(i) ? InstanceState.Disabled : InstanceState.NotDeployed }; instances.Add(instance); @@ -73,7 +75,11 @@ public class DeploySiteAsyncTests : TestKit return instances; } - private DeploymentService CreateService(IActorRef commActor, int maxParallelism, TimeSpan? perInstanceTimeout = null) + private DeploymentService CreateService( + IActorRef commActor, + int maxParallelism, + TimeSpan? perInstanceTimeout = null, + TimeSpan? reconcileTimeout = null) { var comms = new CommunicationService( Options.Create(new CommunicationOptions { DeploymentTimeout = TimeSpan.FromSeconds(30) }), @@ -84,7 +90,8 @@ public class DeploySiteAsyncTests : TestKit { OperationLockTimeout = TimeSpan.FromSeconds(5), SiteDeploymentMaxParallelism = maxParallelism, - SiteDeploymentTimeoutPerInstance = perInstanceTimeout ?? TimeSpan.FromSeconds(30) + SiteDeploymentTimeoutPerInstance = perInstanceTimeout ?? TimeSpan.FromSeconds(30), + SiteDeploymentReconcileTimeout = reconcileTimeout ?? TimeSpan.FromSeconds(5) }); return new DeploymentService( @@ -305,6 +312,147 @@ public class DeploySiteAsyncTests : TestKit Arg.Any()); } + /// + /// P7-2: a bulk site deploy must not restart deliberately-stopped equipment. + /// + /// + /// Deploy is a legal transition out of Disabled and the deploy path + /// forces the instance to Enabled on success, so a site-wide push over an + /// unfiltered instance list silently re-enabled every instance an operator had + /// disabled. Bulk deploy therefore skips them outright — no flatten, no send, no + /// state change — and reports them so the operator can see what was left alone. + /// The single-instance path is unchanged: that is the explicit "yes, re-enable + /// this one" action. + /// + /// + [Fact] + public async Task DeploySiteAsync_SkipsDisabledInstances_WithoutDeployingOrReEnablingThem() + { + var instances = ArrangeInstances(5, disabledIds: [2, 4]); + + var commActor = Sys.ActorOf(Props.Create(() => + new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero))); + + var result = await CreateService(commActor, maxParallelism: 3).DeploySiteAsync(SiteId, "admin"); + + Assert.True(result.IsSuccess); + var summary = result.Value; + + // Only the three Enabled/NotDeployed instances were attempted. + Assert.Equal(3, summary.InstanceResults.Count); + Assert.Equal(3, summary.SuccessCount); + Assert.Equal(0, summary.FailureCount); + Assert.DoesNotContain(summary.InstanceResults, r => r.UniqueName is "Inst-02" or "Inst-04"); + + // The Disabled pair is reported as skipped, with the reason the operator needs. + Assert.NotNull(summary.SkippedInstances); + var skipped = summary.SkippedInstances; + Assert.Equal(2, skipped.Count); + Assert.Equal( + new[] { "Inst-02", "Inst-04" }, + skipped.Select(s => s.UniqueName).Order().ToArray()); + Assert.All(skipped, s => Assert.Contains("deploy individually to re-enable", s.Reason)); + + // ...and left completely untouched: still Disabled, and never handed to the + // repository at all (no deployment record, no instance update, no snapshot). + foreach (var id in new[] { 2, 4 }) + { + Assert.Equal(InstanceState.Disabled, instances.Single(i => i.Id == id).State); + await _pipeline.DidNotReceive().FlattenAndValidateAsync( + id, Arg.Any(), Arg.Any(), Arg.Any()); + } + + await _repo.DidNotReceive().UpdateInstanceAsync( + Arg.Is(i => i.Id == 2 || i.Id == 4), Arg.Any()); + await _repo.DidNotReceive().AddDeploymentRecordAsync( + Arg.Is(r => r.InstanceId == 2 || r.InstanceId == 4), Arg.Any()); + } + + /// + /// P7-3: phase 1 is serial, so an unreachable site used to cost the batch one + /// full 30-second QueryTimeout PER INSTANCE before a single deploy was + /// sent — hours for a large site, so the CLI's 5-minute bulk deadline cancelled + /// and the next attempt re-armed the same cost. Two bounds close it: a short + /// per-reconcile deadline, and a once-per-batch circuit-breaker that skips the + /// remaining reconciles after the first timeout. + /// + /// + /// The site here answers the deploy but NEVER answers the reconcile query, so + /// the assertion that exactly ONE query was issued for six instances is the + /// whole finding: without the breaker there would be six. + /// + /// + [Fact] + public async Task DeploySiteAsync_UnreachableSite_ReconcilesOnceThenSkipsTheRestOfTheBatch() + { + const int instanceCount = 6; + ArrangeInstances(instanceCount); + + // Reconciliation only runs when a prior record is stuck InProgress. + for (var i = 1; i <= instanceCount; i++) + { + _repo.GetCurrentDeploymentStatusAsync(i, Arg.Any()) + .Returns(new DeploymentRecord($"prior-{i}", "admin") + { + InstanceId = i, + Status = DeploymentStatus.InProgress, + RevisionHash = "sha256:old" + }); + } + + var queryCount = 0; + var commActor = Sys.ActorOf(Props.Create(() => + new SilentQuerySiteActor(() => Interlocked.Increment(ref queryCount)))); + + var reconcileTimeout = TimeSpan.FromMilliseconds(300); + var service = CreateService(commActor, maxParallelism: 3, reconcileTimeout: reconcileTimeout); + + var sw = Stopwatch.StartNew(); + var result = await service.DeploySiteAsync(SiteId, "admin"); + sw.Stop(); + + Assert.True(result.IsSuccess); + + // The circuit-breaker opened after the first unanswered reconcile. + Assert.Equal(1, Volatile.Read(ref queryCount)); + + // Prepare therefore cost ONE reconcile deadline, not six. + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), + $"bulk prepare took {sw.ElapsedMilliseconds} ms — the per-batch reconcile bound is not holding"); + + // A skipped reconcile is not a failed deploy: every instance still went out. + Assert.Equal(instanceCount, result.Value.SuccessCount); + Assert.Equal(0, result.Value.FailureCount); + } + + /// + /// Answers RefreshDeploymentCommand normally but NEVER answers a + /// DeploymentStateQueryRequest, counting how many arrived — a site that + /// is up for deploys yet unreachable for the reconcile query, which is the shape + /// a wedged or overloaded site presents. + /// + private sealed class SilentQuerySiteActor : ReceiveActor + { + public SilentQuerySiteActor(Action onQuery) + { + Receive(env => + { + switch (env.Message) + { + case DeploymentStateQueryRequest: + onQuery(); + return; + + case RefreshDeploymentCommand cmd: + Sender.Tell(new DeploymentStatusResponse( + cmd.DeploymentId, cmd.InstanceUniqueName, + DeploymentStatus.Success, null, DateTimeOffset.UtcNow)); + return; + } + }); + } + } + /// /// Answers every RefreshDeploymentCommand after a fixed delay, recording /// how old each command's staging Timestamp already was on arrival. diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentManagerOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentManagerOptionsValidatorTests.cs index 1fecefd2..a4ddd503 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentManagerOptionsValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentManagerOptionsValidatorTests.cs @@ -53,4 +53,23 @@ public class DeploymentManagerOptionsValidatorTests Assert.True(result.Failed); Assert.Contains("OperationLockTimeout", result.FailureMessage); } + + [Fact] + public void ZeroSiteDeploymentReconcileTimeout_IsRejected() + { + var result = Validate(new DeploymentManagerOptions { SiteDeploymentReconcileTimeout = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("SiteDeploymentReconcileTimeout", result.FailureMessage); + } + + [Fact] + public void NegativeSiteDeploymentReconcileTimeout_IsRejected() + { + var result = Validate(new DeploymentManagerOptions + { + SiteDeploymentReconcileTimeout = TimeSpan.FromSeconds(-1) + }); + Assert.True(result.Failed); + Assert.Contains("SiteDeploymentReconcileTimeout", result.FailureMessage); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index 7b7b27c3..30fc371c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -1597,6 +1597,36 @@ public class DeploymentServiceTests : TestKit Assert.Equal(InstanceState.Disabled, instance.State); } + /// + /// The counterpart to the bulk path's Disabled skip (arch-review P7-2): a + /// SINGLE-instance deploy of a Disabled instance still deploys AND enables it. + /// That is the documented "Deploy on a Disabled instance also enables it" + /// transition and the explicit operator action bulk deploy now refuses to + /// perform implicitly, so it must keep working exactly as before. + /// + [Fact] + public async Task DeployInstanceAsync_DisabledInstance_StillDeploysAndEnables() + { + var instance = new Instance("SingleDisabled") + { + Id = 73, SiteId = 1, State = InstanceState.Disabled + }; + _repo.GetInstanceByIdAsync(73, Arg.Any()).Returns(instance); + SetupValidPipeline(73, "SingleDisabled", "sha256:target"); + _repo.GetCurrentDeploymentStatusAsync(73, Arg.Any()) + .Returns((DeploymentRecord?)null); + + var counters = new ReconcileProbeCounters(); + var commActor = Sys.ActorOf(Props.Create(() => + new ReconcileProbeActor(counters, siteHash: "sha256:target", failQuery: false))); + + var result = await CreateServiceWithCommActor(commActor).DeployInstanceAsync(73, "admin"); + + Assert.True(result.IsSuccess); + Assert.Equal(1, counters.DeployCount); + Assert.Equal(InstanceState.Enabled, instance.State); + } + // ── DeploymentManager-016: reconciled record must carry the target revision hash ── [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs index fed5a45c..9404864b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs @@ -1,4 +1,5 @@ using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates; @@ -96,4 +97,72 @@ public class StaleInstanceProbeTests Assert.Equal("sha256:second", await probe.GetCurrentRevisionHashAsync(InstanceId)); } + + /// + /// P7-4: a non-structural template edit landing DURING the flatten must not be + /// captured as if it were already included. + /// + /// + /// The memo pairs a revision hash with the watermark readings that produced it. + /// Reading the per-template versions AFTER the flatten pairs the PRE-edit hash + /// with the POST-edit version, and every later probe then finds the watermark + /// unchanged and answers "not stale" forever — until an unrelated edit, a + /// restart, or the 4096-entry overflow clears it. The staleness sweep and + /// ImportResult.StaleInstanceIds both read through this memo. + /// + /// + /// + /// The fix is that the versions come from the session, which records each + /// template's version at the instant it READ that template — before the edit — + /// so the mismatch on the next probe invalidates the memo. Only the recording is + /// changed: the mid-flatten edit still produces the pre-edit hash on this pass, + /// which is correct, since that is the graph the flatten actually saw. + /// + /// + [Fact] + public async Task TemplateEditedDuringFlatten_InvalidatesTheMemo_RatherThanPinningTheStaleHash() + { + const int TemplateId = 42; + + // The flatten reads the template through the session (recording the version + // it read it at), and the edit lands before the flatten returns. + _pipeline + .FlattenAndValidateAsync(InstanceId, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(async call => + { + _flattenCount++; + var session = call.ArgAt(3)!; + await session.GetTemplateAsync(TemplateId, _ => Task.FromResult(new Template($"T{TemplateId}"))); + + // A non-structural template edit commits while the flatten is in flight. + _watermark.BumpTemplate(TemplateId); + _currentHash = "sha256:after-edit"; + + var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-01" }; + return Result.Success( + new FlatteningPipelineResult(config, "sha256:before-edit", ValidationResult.Success())); + }); + + var probe = CreateProbe(); + + // Pass 1 legitimately returns the hash of the graph the flatten walked. + Assert.Equal("sha256:before-edit", await probe.GetCurrentRevisionHashAsync(InstanceId)); + Assert.Equal(1, _flattenCount); + + // Pass 2 must see the edit. Re-recording the versions after the flatten + // would have memoised the post-edit version against the pre-edit hash, and + // this call would still answer "sha256:before-edit" without re-flattening. + _pipeline + .FlattenAndValidateAsync(InstanceId, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + { + _flattenCount++; + var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-01" }; + return Result.Success( + new FlatteningPipelineResult(config, _currentHash, ValidationResult.Success())); + }); + + Assert.Equal("sha256:after-edit", await probe.GetCurrentRevisionHashAsync(InstanceId)); + Assert.Equal(2, _flattenCount); + } }