diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs index 4ff88e96..8b42f61a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs @@ -55,14 +55,25 @@ public static class DeployCommands private static Command BuildSite(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) { var siteIdOption = new Option("--site-id") { Description = "Target site ID", Required = true }; + // A flag, not a value option: absent means the default (skip unchanged + // instances), present means redeploy them. Disabled instances are skipped + // either way — force is not an override for that. + var forceOption = new Option("--force") + { + Description = + "Redeploy instances whose config is unchanged (default: skip them — a redeploy " + + "resets static overrides and native alarm state at the site)", + }; var cmd = new Command("site") { Description = "Deploy every deployable instance at a site" }; cmd.Add(siteIdOption); + cmd.Add(forceOption); cmd.SetAction(async (ParseResult result) => { var siteId = result.GetValue(siteIdOption); + var force = result.GetValue(forceOption); return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, - new MgmtDeploySiteCommand(siteId), + new MgmtDeploySiteCommand(siteId, force), // A bulk deploy is N instance round-trips; the default 30 s client // timeout would abandon the request while the server is still // applying. Matches the server-side long-running Ask window. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index 81e97487..923bca73 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -974,19 +974,35 @@ Site round-trips run with bounded concurrency `SiteDeploymentTimeoutPerInstance` (default 120 s), so one wedged instance cannot stall the batch. The client waits up to 5 minutes. +Two classes of instance are **skipped** — not flattened, not sent, not changed in any +way — and reported in `skippedInstances` with a reason instead of counting as a success +or a failure: + +- **Disabled** instances. Deploy is a legal transition out of `Disabled` and re-enables + the instance, so a site-wide push would silently restart equipment an operator + deliberately stopped. Deploy them individually to re-enable. +- **Up-to-date** instances — those whose freshly flattened config still matches the + revision the site is running. A redeploy is destructive at the site (it resets static + attribute overrides and clears native alarm state), so pushing one to an unchanged + instance loses live state for no benefit. `--force` deploys them anyway; use it when + central's recorded state cannot be trusted, e.g. after a site's data volume was + restored or a node rebuilt. `--force` does **not** override the Disabled skip. + `--site-id` is **required** — unlike `deploy artifacts`, omitting it does not mean fleet-wide. ```sh -scadabridge --url deploy site --site-id +scadabridge --url deploy site --site-id [--force] ``` | Option | Required | Description | |--------|----------|-------------| | `--site-id` | yes | Target site ID. A site-scoped (non-Administrator) Deployer must supply an in-scope site. | +| `--force` | no | Redeploy instances whose config is unchanged (default: skip them). Disabled instances are still skipped. | Output is a per-instance result matrix (`instanceId`, `uniqueName`, `deploymentId`, -`success`, `errorMessage`) plus `successCount` / `failureCount`. +`success`, `errorMessage`) plus `successCount` / `failureCount`, and `skippedInstances` +(`instanceId`, `uniqueName`, `reason`). #### `deploy artifacts` diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs index b59dd411..fc261770 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs @@ -355,4 +355,26 @@ public interface IDeploymentManagerRepository /// A cancellation token that can be used to cancel the operation. /// A task representing the number of entities saved. Task SaveChangesAsync(CancellationToken cancellationToken = default); + + /// + /// Abandons every pending (added / modified / deleted) change on the unit of + /// work, leaving the underlying store untouched, and returns how many entries + /// were dropped. + /// + /// + /// Recovery only. A failure does NOT + /// untrack the entity that caused it: it stays in the change set and is + /// re-attempted by the NEXT save, so one unsavable entity poisons every + /// later save made through the same unit of work. That matters most on the + /// bulk site-deploy path, where all three phases share ONE scoped context + /// across every instance in the batch — a persistent failure on instance 1 + /// would otherwise fail instance 2..N's status writes as well, including the + /// cleanup write that exists precisely to stop records being stranded + /// InProgress. Call this only after a save has already failed and its + /// pending work has been given up on; anything not yet committed is lost by + /// design. + /// + /// + /// The number of tracked entries detached (0 when nothing was pending). + int DetachPendingChanges(); } diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs index 0f8ce2a6..a4d53efb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs @@ -21,7 +21,16 @@ public record MgmtDeployArtifactsCommand(int? SiteId = null); /// /// /// Database id of the target site. Required — this command is never fleet-wide. -public record MgmtDeploySiteCommand(int SiteId); +/// +/// Deploy every deployable instance even when its flattened config already matches +/// the revision the site is running. Default : an unchanged +/// instance is skipped and reported in the summary's SkippedInstances, +/// because a redeploy resets static attribute overrides and native alarm state at +/// the site. Set it when central's recorded state cannot be trusted — a site whose +/// data volume was restored or whose node was rebuilt looks up-to-date from central +/// and needs the push anyway. Never overrides the Disabled skip. +/// +public record MgmtDeploySiteCommand(int SiteId, bool Force = false); public record QueryDeploymentsCommand(int? InstanceId = null, string? Status = null, int Page = 1, int PageSize = 50); /// diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs index 9c978c17..54c8bb36 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs @@ -551,6 +551,24 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository return await _dbContext.SaveChangesAsync(cancellationToken); } + /// + public int DetachPendingChanges() + { + // ChangeTracker.Entries() is enumerated into a list FIRST: setting State + // mutates the tracker, and mutating it while enumerating it throws. + // Unchanged/Detached entries are left alone — they carry no pending write, + // so keeping them tracked preserves identity resolution for anything the + // caller still holds a reference to. + var pending = _dbContext.ChangeTracker.Entries() + .Where(e => e.State is EntityState.Added or EntityState.Modified or EntityState.Deleted) + .ToList(); + + foreach (var entry in pending) + entry.State = EntityState.Detached; + + return pending.Count; + } + /// /// Flat shape of the deployment-record ⋈ instance join used to compose the list /// query. A named type rather than an anonymous one so diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index b9ba4e43..5f6dce58 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -5,6 +5,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Lifecycle; using ZB.MOM.WW.ScadaBridge.Commons.Observability; @@ -45,6 +46,7 @@ public class DeploymentService private readonly DiffService _diffService; private readonly RevisionHashService _revisionHashService; private readonly IDeploymentStatusNotifier _statusNotifier; + private readonly IStaleInstanceProbe _staleInstanceProbe; private readonly DeploymentManagerOptions _options; private readonly CommunicationOptions _commOptions; private readonly ILogger _logger; @@ -64,6 +66,15 @@ public class DeploymentService internal const string DisabledSkipReason = "Disabled — bulk deploy preserves disabled state; deploy individually to re-enable"; + /// + /// Reason reported on every a bulk site + /// deploy produces for an instance whose flattened config already matches the + /// revision the site is running. Suppressed by the force flag. + /// + internal const string UpToDateSkipReason = + "up-to-date — the flattened config still matches the deployed revision; " + + "redeploy with force to push it anyway"; + /// Rows deleted per terminal-record purge batch. private const int TerminalPurgeBatchSize = 500; @@ -98,6 +109,11 @@ public class DeploymentService /// staleness hash from the (List-normalized) deserialized snapshot — see I-1. /// /// Notifier for pushing deployment status changes to the UI. + /// + /// Current-revision-hash probe backing the bulk deploy's up-to-date pre-filter + /// (see ). The same seam the bundle importer and + /// the Deployments page use, so all three agree on what "stale" means. + /// /// Deployment manager configuration options. /// /// Central-site communication options. The notify-and-fetch deploy path reads @@ -117,6 +133,7 @@ public class DeploymentService DiffService diffService, RevisionHashService revisionHashService, IDeploymentStatusNotifier statusNotifier, + IStaleInstanceProbe staleInstanceProbe, IOptions options, IOptions communicationOptions, ILogger logger) @@ -130,6 +147,7 @@ public class DeploymentService _diffService = diffService; _revisionHashService = revisionHashService; _statusNotifier = statusNotifier; + _staleInstanceProbe = staleInstanceProbe; _options = options.Value; _commOptions = communicationOptions.Value; _logger = logger; @@ -681,6 +699,21 @@ public class DeploymentService /// A fault DURING cleanup is logged loudly and swallowed — it must not mask /// the original error the caller is about to report. /// + /// + /// + /// The cleanup save is un-poisoned before it is given up on (P7-5). A + /// failed SaveChanges does not untrack the entity that caused it: EF + /// keeps it in the change set and re-attempts it on the NEXT save. Every phase + /// of a bulk site deploy shares ONE scoped DbContext across every + /// instance in the batch, so a persistent failure on one instance would + /// otherwise fail instance 2..N's status writes too — including their own + /// cleanup writes, which is exactly the write that stops a record being + /// stranded InProgress. On a cleanup-save failure the pending changes + /// are therefore DETACHED () + /// and the Failed write is retried ONCE on the now-clean tracker: if the poison + /// came from an earlier instance this record still reaches its terminal status, + /// and either way the next instance in the batch saves cleanly. + /// /// private async Task MarkDeploymentFailedAsync( DeploymentRecord record, @@ -697,25 +730,57 @@ public class DeploymentService try { - await _repository.UpdateDeploymentRecordAsync(record, CancellationToken.None); - // Note: if the staging SaveChangesAsync was interrupted, an Added - // PendingDeployment may still be tracked and will be committed by this - // cleanup save. That row is orphaned (no RefreshDeploymentCommand was - // sent, so no site holds its token) and is removed by TTL purge / - // superseded by the next deploy -- harmless. - await _repository.SaveChangesAsync(CancellationToken.None); - NotifyStatusChange(record); - - await _auditService.LogAsync(user, "DeployFailed", "Instance", instance.Id.ToString(), - instance.UniqueName, new { DeploymentId = deploymentId, Error = cause.Message }, - CancellationToken.None); + await PersistFailedStatusAsync(); } catch (Exception cleanupEx) { + // Give up on whatever is pending — it has already failed to save at + // least once — so the shared change tracker stops re-attempting it. + var detached = _repository.DetachPendingChanges(); + _logger.LogError(cleanupEx, "Failed to persist Failed status for deployment {DeploymentId} of instance {Instance} " + - "after deployment error: {Error}", - deploymentId, instance.UniqueName, cause.Message); + "after deployment error: {Error}. Detached {DetachedCount} pending change(s) so the " + + "shared unit of work does not carry the failure into later writes", + deploymentId, instance.UniqueName, cause.Message, detached); + + if (detached > 0) + { + try + { + await PersistFailedStatusAsync(); + } + catch (Exception retryEx) + { + _logger.LogError(retryEx, + "Retrying the Failed status write for deployment {DeploymentId} of instance " + + "{Instance} on a cleaned unit of work also failed; the record remains " + + "InProgress and must be reconciled (the next deploy of this instance heals it)", + deploymentId, instance.UniqueName); + } + } + } + + // Best-effort and deliberately OUTSIDE the status-write try: an audit row + // for a failed deployment is worth writing even when the status write + // itself could not be persisted, and an audit failure must never mask the + // deployment error the caller is about to report. + await TryLogAuditAsync(user, "DeployFailed", "Instance", instance.Id.ToString(), + instance.UniqueName, new { DeploymentId = deploymentId, Error = cause.Message }, + CancellationToken.None); + + // Writes the terminal Failed status and pushes the status change. + // + // Note: if the staging SaveChangesAsync was interrupted, an Added + // PendingDeployment may still be tracked and will be committed by this + // cleanup save. That row is orphaned (no RefreshDeploymentCommand was + // sent, so no site holds its token) and is removed by TTL purge / + // superseded by the next deploy -- harmless. + async Task PersistFailedStatusAsync() + { + await _repository.UpdateDeploymentRecordAsync(record, CancellationToken.None); + await _repository.SaveChangesAsync(CancellationToken.None); + NotifyStatusChange(record); } } @@ -845,18 +910,36 @@ public class DeploymentService /// /// Disabled instances are skipped entirely and reported in /// — see the filter below - /// for why. Reconciliation is bounded per batch by + /// for why. Up-to-date instances are skipped too unless + /// is set, for the reason documented on + /// : a redeploy is destructive to site-local + /// state, so pushing one to an instance whose config has not changed loses + /// data for no benefit. 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 two filters compose in one direction only: Disabled wins. A + /// Disabled instance is never deployed at bulk scope, force or not — force + /// suppresses the up-to-date filter, never the Disabled one. + /// /// /// The database ID of the site whose instances are deployed. /// The username initiating the deployment, recorded on every record and audit row. + /// + /// Deploy every deployable instance even when its config is unchanged — the + /// pre-2026-08 behaviour. The operator escape hatch for the case central's + /// recorded state cannot see: a site that lost its config (restored volume, + /// rebuilt node) looks up-to-date from here and needs the push anyway. Has no + /// effect on Disabled instances. + /// /// Cancellation token for the operation. /// A per-instance result matrix, or a failure result when the site itself cannot be resolved. public async Task> DeploySiteAsync( int siteId, string user, + bool force = false, CancellationToken cancellationToken = default) { var site = await _siteRepository.GetSiteByIdAsync(siteId, cancellationToken); @@ -874,16 +957,33 @@ public class DeploymentService // 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. + // + // Unchanged instances are then filtered out for the mirror-image reason: + // see IsUpToDateAsync. var deployable = new List(instances.Count); var skipped = new List(); + var disabledCount = 0; foreach (var instance in instances) { + cancellationToken.ThrowIfCancellationRequested(); + if (instance.State == InstanceState.Disabled) + { skipped.Add(new SkippedInstanceResult(instance.Id, instance.UniqueName, DisabledSkipReason)); + disabledCount++; + } + else if (!force && await IsUpToDateAsync(instance, cancellationToken)) + { + skipped.Add(new SkippedInstanceResult(instance.Id, instance.UniqueName, UpToDateSkipReason)); + } else + { deployable.Add(instance); + } } + var upToDateCount = skipped.Count - disabledCount; + // ---- Phase 1: prepare, serially, on ONE shared flatten session. ---- var session = _flatteningPipeline.CreateSession(); var reconcileGate = new BulkReconcileGate(_options.SiteDeploymentReconcileTimeout); @@ -988,14 +1088,110 @@ public class DeploymentService _logger.LogInformation( "Bulk deployment of site {SiteId} complete: {SuccessCount} succeeded, {FailureCount} failed, " + - "{SkippedCount} skipped as Disabled " + + "{DisabledCount} skipped as Disabled, {UpToDateCount} skipped as up-to-date (force={Force}) " + "({ChainLoads} template-chain load(s) across {InstanceCount} instance(s))", - site.SiteIdentifier, summary.SuccessCount, summary.FailureCount, skipped.Count, - session.ChainLoads, deployable.Count); + site.SiteIdentifier, summary.SuccessCount, summary.FailureCount, disabledCount, upToDateCount, + force, session.ChainLoads, deployable.Count); return Result.Success(summary); } + /// + /// True when a bulk site deploy should leave this instance alone because the + /// site is already running the config it would be sent. + /// + /// + /// Why an unconditional bulk redeploy is the wrong default. A redeploy + /// is NOT idempotent at the site: applying a config resets that instance's + /// static attribute overrides and clears its native alarm state, both by + /// design. A site-wide push therefore destroyed live operational state on every + /// instance it touched, including the ones whose configuration had not changed + /// at all — the usual case, since the operator is normally pushing one edited + /// template. Skipping them costs nothing (the site is already running that + /// revision) and preserves the state. + /// + /// + /// + /// The test is deliberately conservative. Every uncertainty resolves to + /// "deploy it", so this filter can only ever fail to skip — it can never skip + /// an instance that needed the push: + /// + /// + /// + /// Only an instance can be up to date. + /// A one is not running the config + /// no matter what a leftover snapshot row says. + /// + /// + /// No means central has never seen a + /// successful apply — nothing to compare against. + /// + /// + /// A prior record that flags + /// (stuck InProgress, or timeout-Failed) must go through + /// prepare, because that is where the query-before-redeploy reconciliation + /// that HEALS such a record runs. Skipping here would strand it forever. + /// + /// + /// The probe returning (unresolvable template chain) + /// or throwing means staleness is indeterminate. + /// + /// + /// + /// + /// The comparison uses the snapshot's own stored RevisionHash rather + /// than 's recomputed-from-normalized + /// hash: the normalization exists to suppress a SPURIOUS stale flag (I-1), and + /// treating a spuriously-stale instance as stale here just deploys it, which is + /// the old behaviour and always safe. + /// + /// + /// + /// Cost. The probe re-flattens on its own session rather than the + /// batch's, so a fully-stale site pays one extra flatten per instance before + /// prepare re-flattens it for real. That is bounded by the probe's watermark + /// memo (unchanged graph ⇒ no flatten at all on the second and later calls, and + /// the Deployments page's staleness sweep warms the same memo), and it buys + /// back far more than it costs on the common mostly-unchanged site, which now + /// sends no site round-trips at all instead of one per instance. + /// + /// + /// The candidate instance. + /// Cancellation token. + /// only when the instance is provably unchanged. + private async Task IsUpToDateAsync(Instance instance, CancellationToken cancellationToken) + { + if (instance.State != InstanceState.Enabled) + return false; + + var snapshot = await _repository.GetDeployedSnapshotByInstanceIdAsync(instance.Id, cancellationToken); + if (snapshot == null) + return false; + + var prior = await _repository.GetCurrentDeploymentStatusAsync(instance.Id, cancellationToken); + if (prior != null && ShouldQuerySiteBeforeRedeploy(prior)) + return false; + + string? currentHash; + try + { + currentHash = await _staleInstanceProbe.GetCurrentRevisionHashAsync(instance.Id, cancellationToken); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // Staleness is an optimisation, never a gate: an unexpected probe fault + // falls back to deploying the instance, which is what this method's + // absence would have done. + _logger.LogWarning(ex, + "Could not determine whether instance {Instance} is up to date; deploying it", + instance.UniqueName); + return false; + } + + return currentHash != null + && string.Equals(currentHash, snapshot.RevisionHash, StringComparison.Ordinal); + } + /// /// Unwinds the prepared deployments a bulk deploy never finalised, after the /// operation escaped early (cancellation, or a fault in phase 3's own @@ -2085,11 +2281,13 @@ public record SkippedInstanceResult( /// Number of instances the site confirmed. /// Number of instances that failed for any reason. /// -/// Instances the batch deliberately did not attempt — currently the +/// Instances the batch deliberately did not attempt: 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. +/// re-enable, and the up-to-date ones, which a bulk push must not needlessly reset +/// (a redeploy clears static overrides and native alarm state at the site). Each +/// row carries the reason. 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, diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 6d5f84d1..0ed519c1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -943,12 +943,20 @@ public class ManagementActor : ReceiveActor /// their scope, and every instance the batch touches belongs to that site by /// construction — so the per-instance check would be redundant. /// + /// + /// + /// Force is relayed verbatim: unset (the default) the batch skips + /// instances whose config already matches the deployed revision, set it + /// redeploys them anyway. It carries no extra authorization — the command's + /// own Deployer + site scope already gates the whole operation, and forcing + /// only widens it to instances this same caller could deploy one at a time. + /// /// private static async Task HandleDeploySite(IServiceProvider sp, MgmtDeploySiteCommand cmd, AuthenticatedUser user) { EnforceSiteScope(user, cmd.SiteId); var svc = sp.GetRequiredService(); - var result = await svc.DeploySiteAsync(cmd.SiteId, user.Username); + var result = await svc.DeploySiteAsync(cmd.SiteId, user.Username, cmd.Force); return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs index fd54daaa..8c019d56 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs @@ -157,6 +157,28 @@ public class CommandTreeTests Assert.Empty(site.Parse(["--site-id", "3"]).Errors); } + [Fact] + public void DeploySite_HasForceFlag_DefaultingToTheNonDestructiveBehaviour() + { + // P7-6: bulk deploy skips instances whose config is unchanged, because a + // redeploy resets their static overrides and native alarm state at the + // site. --force is the operator escape hatch (restored volume, rebuilt + // node) and must be a FLAG — absent means skip-unchanged, which is the + // safe default a scripted `deploy site` keeps getting. + var deploy = DeployCommands.Build(Url, Format, Username, Password); + var site = deploy.Subcommands.Single(c => c.Name == "site"); + var forceOption = (Option)site.Options.Single(o => o.Name == "--force"); + + var withoutForce = site.Parse(["--site-id", "3"]); + Assert.Empty(withoutForce.Errors); + Assert.False(withoutForce.GetValue(forceOption)); + + // A bare --force (no value) must parse: it is a flag, not a value option. + var withForce = site.Parse(["--site-id", "3", "--force"]); + Assert.Empty(withForce.Errors); + Assert.True(withForce.GetValue(forceOption)); + } + [Fact] public void InstanceNativeAlarmSource_HasSetAndClear() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs index 5d41048e..0377b6a4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs @@ -13,6 +13,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; @@ -74,6 +75,11 @@ public class TopologyPageTests : BunitContext Services.AddSingleton( new ZB.MOM.WW.ScadaBridge.DeploymentManager.DeploymentStatusNotifier( NullLogger.Instance)); + // P7-6: the bulk site deploy's up-to-date pre-filter asks the staleness + // probe for each instance's current revision hash, so DeploymentService now + // depends on it too. The page under test never bulk-deploys; a substitute + // keeps the DI graph resolvable without dragging in the flattening stack. + Services.AddSingleton(Substitute.For()); Services.AddScoped(); Services.AddScoped(); Services.AddScoped(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs index 4871d7fd..b9dd2f2e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs @@ -10,6 +10,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; @@ -34,6 +35,7 @@ public class DeploySiteAsyncTests : TestKit private readonly IFlatteningPipeline _pipeline = Substitute.For(); private readonly ISiteRepository _siteRepo = Substitute.For(); private readonly IAuditService _audit = Substitute.For(); + private readonly IStaleInstanceProbe _staleProbe = Substitute.For(); private readonly OperationLockManager _lockManager = new(); public DeploySiteAsyncTests() @@ -99,6 +101,7 @@ public class DeploySiteAsyncTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + _staleProbe, options, Options.Create(new CommunicationOptions { @@ -302,7 +305,7 @@ public class DeploySiteAsyncTests : TestKit var service = CreateService(commActor, maxParallelism: 2); await Assert.ThrowsAnyAsync( - () => service.DeploySiteAsync(SiteId, "admin", cts.Token)); + () => service.DeploySiteAsync(SiteId, "admin", cancellationToken: cts.Token)); Assert.Equal(0, _lockManager.TrackedLockCount); @@ -425,6 +428,203 @@ public class DeploySiteAsyncTests : TestKit Assert.Equal(0, result.Value.FailureCount); } + /// + /// Marks an instance as one a bulk deploy must treat as up to date: it is + /// Enabled, central holds a deployed snapshot for it, and the staleness + /// probe recomputes the very hash that snapshot recorded. + /// + /// The instance to make up-to-date. + private void ArrangeUpToDate(Instance instance) + { + var hash = $"sha256:{instance.Id}"; + instance.State = InstanceState.Enabled; + + _repo.GetDeployedSnapshotByInstanceIdAsync(instance.Id, Arg.Any()) + .Returns(new DeployedConfigSnapshot($"prior-{instance.Id}", hash, "{}") { InstanceId = instance.Id }); + _repo.GetCurrentDeploymentStatusAsync(instance.Id, Arg.Any()) + .Returns(new DeploymentRecord($"prior-{instance.Id}", "admin") + { + InstanceId = instance.Id, + Status = DeploymentStatus.Success, + RevisionHash = hash, + }); + _staleProbe.GetCurrentRevisionHashAsync(instance.Id, Arg.Any()) + .Returns(hash); + } + + /// + /// P7-6: a bulk site deploy must not redeploy instances whose configuration has + /// not changed. + /// + /// + /// A redeploy is NOT a no-op at the site: applying a config RESETS that + /// instance's static attribute overrides and CLEARS its native alarm state, + /// both by design. Pushing one site-wide therefore destroyed live operational + /// state on every instance in the batch, including the majority whose config + /// the operator had not touched at all. + /// + /// + /// + /// The central-side proof that the site state survives is that the site is + /// never told to apply anything: no RefreshDeploymentCommand is sent + /// for a skipped instance, so nothing at the site runs the reset. The + /// assertions below therefore pin the whole chain — no flatten, no deployment + /// record, no staged config, no command — for the unchanged pair. + /// + /// + [Fact] + public async Task DeploySiteAsync_SkipsUpToDateInstances_SoTheirSiteStateIsNeverReset() + { + var instances = ArrangeInstances(4); + ArrangeUpToDate(instances.Single(i => i.Id == 2)); + ArrangeUpToDate(instances.Single(i => i.Id == 3)); + + var applied = new ConcurrentBag(); + var commActor = Sys.ActorOf(Props.Create(() => new RecordingSiteActor(applied))); + + var result = await CreateService(commActor, maxParallelism: 4).DeploySiteAsync(SiteId, "admin"); + + Assert.True(result.IsSuccess); + var summary = result.Value; + + // Only the two genuinely-stale instances were deployed. + Assert.Equal(2, summary.InstanceResults.Count); + Assert.Equal(2, summary.SuccessCount); + Assert.Equal(0, summary.FailureCount); + Assert.Equal(new[] { "Inst-01", "Inst-04" }, summary.InstanceResults.Select(r => r.UniqueName).Order().ToArray()); + + // The unchanged pair is reported as skipped, with the reason the operator needs. + Assert.NotNull(summary.SkippedInstances); + var skipped = summary.SkippedInstances; + Assert.Equal(new[] { "Inst-02", "Inst-03" }, skipped.Select(s => s.UniqueName).Order().ToArray()); + Assert.All(skipped, s => Assert.Contains("up-to-date", s.Reason)); + + // ...and nothing reached the site for them, which is what leaves their + // static overrides and native alarm state alone. + Assert.Equal(new[] { "Inst-01", "Inst-04" }, applied.Order().ToArray()); + + foreach (var id in new[] { 2, 3 }) + { + await _pipeline.DidNotReceive().FlattenAndValidateAsync( + id, Arg.Any(), Arg.Any(), Arg.Any()); + await _repo.DidNotReceive().AddDeploymentRecordAsync( + Arg.Is(r => r.InstanceId == id), Arg.Any()); + await _repo.DidNotReceive().AddPendingDeploymentAsync( + Arg.Is(p => p.InstanceId == id), Arg.Any()); + } + } + + /// + /// P7-6: force restores the unconditional behaviour for operators whose + /// site state central cannot see — a restored data volume or a rebuilt node + /// looks perfectly up to date from central and still needs the push. + /// + [Fact] + public async Task DeploySiteAsync_Force_RedeploysUpToDateInstances() + { + var instances = ArrangeInstances(3); + ArrangeUpToDate(instances.Single(i => i.Id == 2)); + + var applied = new ConcurrentBag(); + var commActor = Sys.ActorOf(Props.Create(() => new RecordingSiteActor(applied))); + + var result = await CreateService(commActor, maxParallelism: 3) + .DeploySiteAsync(SiteId, "admin", force: true); + + Assert.True(result.IsSuccess); + Assert.Equal(3, result.Value.SuccessCount); + Assert.Empty(result.Value.SkippedInstances!); + Assert.Contains("Inst-02", applied); + } + + /// + /// P7-6 composed with P7-2: force suppresses the up-to-date filter and + /// ONLY that filter. A Disabled instance is never deployed at bulk scope, + /// because forcing a bulk push is an operator saying "the site's config may be + /// wrong", not "restart the equipment I stopped" — the single-instance deploy + /// remains the explicit re-enable action. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task DeploySiteAsync_DisabledInstancesAreSkipped_RegardlessOfForce(bool force) + { + var instances = ArrangeInstances(3, disabledIds: [2]); + + var applied = new ConcurrentBag(); + var commActor = Sys.ActorOf(Props.Create(() => new RecordingSiteActor(applied))); + + var result = await CreateService(commActor, maxParallelism: 3) + .DeploySiteAsync(SiteId, "admin", force: force); + + Assert.True(result.IsSuccess); + Assert.DoesNotContain("Inst-02", applied); + Assert.Equal(InstanceState.Disabled, instances.Single(i => i.Id == 2).State); + + var skipped = Assert.Single(result.Value.SkippedInstances!); + Assert.Equal("Inst-02", skipped.UniqueName); + Assert.Contains("deploy individually to re-enable", skipped.Reason); + } + + /// + /// The up-to-date filter is deliberately conservative: every uncertainty + /// resolves to "deploy it". These are the three cases where a matching hash is + /// NOT sufficient — each one would be a silently skipped instance that actually + /// needed the deploy. + /// + [Fact] + public async Task DeploySiteAsync_MatchingHashIsNotEnough_WhenTheInstanceStillNeedsDeploying() + { + var instances = ArrangeInstances(3); + + // 1: NotDeployed. A leftover snapshot says nothing about an instance that + // is not running — it must be deployed to become Enabled. + ArrangeUpToDate(instances.Single(i => i.Id == 1)); + instances.Single(i => i.Id == 1).State = InstanceState.NotDeployed; + + // 2: prior record stuck InProgress. Prepare is where query-before-redeploy + // reconciliation HEALS such a record; skipping here strands it forever. + ArrangeUpToDate(instances.Single(i => i.Id == 2)); + _repo.GetCurrentDeploymentStatusAsync(2, Arg.Any()) + .Returns(new DeploymentRecord("prior-2", "admin") + { + InstanceId = 2, + Status = DeploymentStatus.InProgress, + RevisionHash = "sha256:2", + }); + + // 3: staleness indeterminate — the probe cannot compute a hash. + ArrangeUpToDate(instances.Single(i => i.Id == 3)); + _staleProbe.GetCurrentRevisionHashAsync(3, Arg.Any()).Returns((string?)null); + + var applied = new ConcurrentBag(); + var commActor = Sys.ActorOf(Props.Create(() => new RecordingSiteActor(applied))); + + var result = await CreateService(commActor, maxParallelism: 3).DeploySiteAsync(SiteId, "admin"); + + Assert.True(result.IsSuccess); + Assert.Empty(result.Value.SkippedInstances!); + Assert.Equal(new[] { "Inst-01", "Inst-02", "Inst-03" }, applied.Order().ToArray()); + } + + /// Answers every deploy with Success, recording which instances the site was asked to apply. + private sealed class RecordingSiteActor : ReceiveActor + { + public RecordingSiteActor(ConcurrentBag applied) + { + Receive(env => + { + if (env.Message is not RefreshDeploymentCommand cmd) + return; + + applied.Add(cmd.InstanceUniqueName); + Sender.Tell(new DeploymentStatusResponse( + cmd.DeploymentId, cmd.InstanceUniqueName, + DeploymentStatus.Success, null, DateTimeOffset.UtcNow)); + }); + } + } + /// /// Answers RefreshDeploymentCommand normally but NEVER answers a /// DeploymentStateQueryRequest, counting how many arrived — a site that diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index 30fc371c..8a7e3836 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -9,6 +9,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Lifecycle; using ZB.MOM.WW.ScadaBridge.Commons.Observability; @@ -54,6 +55,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), options, DeployCommOptions(), NullLogger.Instance); @@ -165,6 +167,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), DeployCommOptions(), NullLogger.Instance); @@ -385,6 +388,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), DeployCommOptions(), NullLogger.Instance); @@ -1153,6 +1157,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), DeployCommOptions(centralFetchBaseUrl), NullLogger.Instance); @@ -1743,6 +1748,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5), @@ -1801,6 +1807,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5), @@ -1851,6 +1858,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5), @@ -1898,6 +1906,7 @@ public class DeploymentServiceTests : TestKit new DiffService(), new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), + Substitute.For(), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5), diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs index ba975448..cc362059 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs @@ -7,6 +7,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; @@ -60,7 +61,8 @@ public class DeploymentStatusNotifierTests : TestKit _service = new DeploymentService( _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, - new DiffService(), new RevisionHashService(), _notifier, options, + new DiffService(), new RevisionHashService(), _notifier, + Substitute.For(), options, // Notify-and-fetch (Task 6): a non-empty CentralFetchBaseUrl lets the // deploy path reach the send (and, here, the catch block) rather than // tripping the fail-fast configuration guard. diff --git a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/BulkDeployPoisonedContextTests.cs b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/BulkDeployPoisonedContextTests.cs new file mode 100644 index 00000000..00dc9c6b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/BulkDeployPoisonedContextTests.cs @@ -0,0 +1,370 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.AspNetCore.DataProtection; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates; +using ZB.MOM.WW.ScadaBridge.Communication; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; +using ZB.MOM.WW.ScadaBridge.DeploymentManager; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening; + +namespace ZB.MOM.WW.ScadaBridge.IntegrationTests; + +/// +/// P7-5: a bulk site deploy runs all three phases over ONE scoped +/// DbContext, so a persistent SaveChanges failure on a single +/// instance used to fail every instance that came after it. +/// +/// +/// Why this needs a real EF context. The mechanism is not in the service's +/// control flow, it is in EF's: a failed SaveChanges does NOT untrack the +/// entity that caused it. The entity stays in the change set and is re-attempted +/// by the very next save made through the same context — including the cleanup +/// save inside MarkDeploymentFailedAsync, whose whole job is to stop a +/// record being stranded InProgress. A mocked repository could not prove +/// that; a real over SQLite does. +/// +/// +/// +/// Honesty note on the injected failure. The persistent failure itself is +/// injected by a save interceptor rather than provoked by a real constraint +/// violation, because the entity that must fail (the row +/// the deploy path mutates) is created and mutated inside the service, where a +/// test cannot reach in and corrupt it mid-batch. The interceptor throws +/// at exactly the point a constraint violation, +/// a concurrency conflict or a dropped connection would — everything after that, +/// including the cascade and the fix, is real EF behaviour. +/// +/// +public sealed class BulkDeployPoisonedContextTests : TestKit, IDisposable +{ + private const int InstanceCount = 4; + + private readonly SqliteConnection _connection; + private readonly ScadaBridgeDbContext _context; + private readonly PoisonedEntityInterceptor _poison = new(); + private int _siteId; + + public BulkDeployPoisonedContextTests() + { + _connection = new SqliteConnection("DataSource=:memory:"); + _connection.Open(); + + var options = new DbContextOptionsBuilder() + .UseSqlite(_connection) + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)) + .AddInterceptors(_poison) + .Options; + + _context = new SqliteContext(options); + _context.Database.EnsureCreated(); + Seed(); + } + + /// + /// The whole finding in one run: instance 1's post-success persistence fails + /// permanently (best-effort, so the deploy still reports Success), which leaves + /// its row poisoning the shared change tracker. Every + /// later instance's status write then rides on that same change set. + /// + /// + /// Pre-fix this cascaded: instances 2, 3 and 4 all failed, and their records + /// stayed InProgress because the cleanup save re-attempted — and + /// re-failed on — the same poisoned entity. Post-fix the cleanup detaches the + /// pending work, so instance 2 is the only casualty (correctly recorded + /// Failed, not stranded), and 3 and 4 save cleanly. + /// + /// + [Fact] + public async Task BulkDeploy_PersistentSaveFailureOnOneInstance_DoesNotCascadeToTheRest() + { + _poison.PoisonInstanceNamed("Inst-01"); + + var service = CreateService(); + + var result = await service.DeploySiteAsync(_siteId, "admin"); + + Assert.True(result.IsSuccess); + var summary = result.Value; + Assert.Equal(InstanceCount, summary.InstanceResults.Count); + + // Instances 3 and 4 are downstream of the poison and must be unaffected: + // this is the assertion that fails outright without the detach. + foreach (var name in new[] { "Inst-03", "Inst-04" }) + { + var row = Assert.Single(summary.InstanceResults, r => r.UniqueName == name); + Assert.True(row.Success, $"{name} failed with: {row.ErrorMessage}"); + } + + // Instance 2 is the one collateral casualty — its own status write shared + // the poisoned change set — but it must be recorded Failed, never left + // InProgress, which is what the retry on the cleaned tracker buys. + var stranded = await NoLongerInProgressAsync(); + Assert.Empty(stranded); + + // ...and the fix is not "swallow everything": the poisoned instance's own + // side effects genuinely did not persist, so its Instance row is still + // NotDeployed even though the site confirmed the apply. That divergence is + // the pre-existing best-effort contract, logged for operator reconciliation. + await using var verify = NewContext(); + Assert.Equal( + InstanceState.NotDeployed, + verify.Instances.Single(i => i.UniqueName == "Inst-01").State); + } + + /// + /// The narrow EF fact the fix rests on, pinned on its own so a future reader + /// does not have to re-derive it: a failed save leaves its entity tracked and + /// the NEXT save re-attempts it, and + /// IDeploymentManagerRepository.DetachPendingChanges is what clears it. + /// + [Fact] + public async Task FailedSave_KeepsTheEntityTracked_UntilPendingChangesAreDetached() + { + var repository = new DeploymentManagerRepository(_context); + var instance = _context.Instances.Single(i => i.UniqueName == "Inst-01"); + + _poison.PoisonInstanceNamed("Inst-01"); + instance.State = InstanceState.Enabled; + await repository.UpdateInstanceAsync(instance); + await Assert.ThrowsAsync(() => repository.SaveChangesAsync()); + + // An unrelated, perfectly valid write now fails too — the poisoned entity + // is still in the change set. + var record = new DeploymentRecord("dep-unrelated", "admin") + { + InstanceId = _context.Instances.Single(i => i.UniqueName == "Inst-02").Id, + Status = DeploymentStatus.Failed, + RevisionHash = "sha256:x", + DeployedAt = DateTimeOffset.UtcNow, + }; + await repository.AddDeploymentRecordAsync(record); + await Assert.ThrowsAsync(() => repository.SaveChangesAsync()); + + // Detaching the pending work un-poisons the unit of work; the same write + // then succeeds. + Assert.True(repository.DetachPendingChanges() > 0); + await repository.AddDeploymentRecordAsync(record); + Assert.Equal(1, await repository.SaveChangesAsync()); + } + + /// Deployment records left non-terminal after the batch — the stranded-InProgress check. + private async Task> NoLongerInProgressAsync() + { + await using var verify = NewContext(); + return await verify.DeploymentRecords + .Where(r => r.Status == DeploymentStatus.InProgress || r.Status == DeploymentStatus.Pending) + .Select(r => r.DeploymentId) + .ToListAsync(); + } + + private DeploymentService CreateService() + { + var repository = new DeploymentManagerRepository(_context); + var siteRepository = new SiteRepository(_context); + + var pipeline = Substitute.For(); + pipeline.CreateSession().Returns(_ => new FlattenSession(new TemplateGraphWatermark())); + pipeline + .FlattenAndValidateAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => Result.Success( + new FlatteningPipelineResult( + new FlattenedConfiguration { InstanceUniqueName = $"Inst-{ci.ArgAt(0):00}" }, + $"sha256:{ci.ArgAt(0)}", + ValidationResult.Success()))); + + var comms = new CommunicationService( + Options.Create(new CommunicationOptions { DeploymentTimeout = TimeSpan.FromSeconds(10) }), + NullLogger.Instance); + comms.SetCommunicationActor(Sys.ActorOf(Props.Create(() => new AcceptingSiteActor()))); + + // Null probe result = "staleness indeterminate", so the up-to-date + // pre-filter never skips: this test is about the persistence cascade and + // needs every instance to actually run. + var probe = Substitute.For(); + probe.GetCurrentRevisionHashAsync(Arg.Any(), Arg.Any()).Returns((string?)null); + + return new DeploymentService( + repository, siteRepository, pipeline, comms, new OperationLockManager(), + Substitute.For(), + new DiffService(), + new RevisionHashService(), + new DeploymentStatusNotifier(NullLogger.Instance), + probe, + Options.Create(new DeploymentManagerOptions + { + OperationLockTimeout = TimeSpan.FromSeconds(5), + SiteDeploymentMaxParallelism = 1, + SiteDeploymentTimeoutPerInstance = TimeSpan.FromSeconds(10), + SiteDeploymentReconcileTimeout = TimeSpan.FromSeconds(2), + }), + Options.Create(new CommunicationOptions + { + CentralFetchBaseUrl = "https://central.test:9000", + PendingDeploymentTtl = TimeSpan.FromMinutes(5), + }), + NullLogger.Instance); + } + + private void Seed() + { + using var ctx = NewContext(); + var site = new Site("Poison Site", "site-poison"); + var template = new Template("T-Poison"); + ctx.Sites.Add(site); + ctx.Templates.Add(template); + ctx.SaveChanges(); + _siteId = site.Id; + + for (var i = 1; i <= InstanceCount; i++) + { + ctx.Instances.Add(new Instance($"Inst-{i:00}") + { + SiteId = site.Id, + TemplateId = template.Id, + State = InstanceState.NotDeployed, + }); + } + + ctx.SaveChanges(); + } + + private ScadaBridgeDbContext NewContext() => + new SqliteContext(new DbContextOptionsBuilder() + .UseSqlite(_connection) + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)) + .Options); + + public new void Dispose() + { + _context.Dispose(); + _connection.Dispose(); + base.Dispose(); + } + + /// + /// Fails every SaveChanges whose change set contains the named + /// — a stand-in for a permanent constraint violation on + /// one tracked entity, raised at the point EF raises a real one. + /// + private sealed class PoisonedEntityInterceptor : SaveChangesInterceptor + { + private string? _poisonedUniqueName; + + public void PoisonInstanceNamed(string uniqueName) => _poisonedUniqueName = uniqueName; + + public override InterceptionResult SavingChanges( + DbContextEventData eventData, InterceptionResult result) + { + ThrowIfPoisoned(eventData); + return base.SavingChanges(eventData, result); + } + + public override ValueTask> SavingChangesAsync( + DbContextEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + ThrowIfPoisoned(eventData); + return base.SavingChangesAsync(eventData, result, cancellationToken); + } + + private void ThrowIfPoisoned(DbContextEventData eventData) + { + if (_poisonedUniqueName is null || eventData.Context is null) + return; + + var poisoned = eventData.Context.ChangeTracker.Entries() + .Any(e => e.State != EntityState.Unchanged + && e.State != EntityState.Detached + && e.Entity.UniqueName == _poisonedUniqueName); + + if (poisoned) + throw new DbUpdateException($"simulated persistent failure on {_poisonedUniqueName}"); + } + } + + /// Answers every deployment with Success, so only persistence can fail here. + private sealed class AcceptingSiteActor : ReceiveActor + { + public AcceptingSiteActor() + { + Receive(env => + { + if (env.Message is not RefreshDeploymentCommand cmd) + return; + + Sender.Tell(new DeploymentStatusResponse( + cmd.DeploymentId, cmd.InstanceUniqueName, + DeploymentStatus.Success, null, DateTimeOffset.UtcNow)); + }); + } + } + + /// + /// SQLite-adapted context: SQLite has no rowversion and no native + /// DateTimeOffset. Mirrors the adaptation DeployNotifyFetchTests uses. + /// + private sealed class SqliteContext : ScadaBridgeDbContext + { + public SqliteContext(DbContextOptions options) + : base(options, new EphemeralDataProtectionProvider()) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + base.OnModelCreating(modelBuilder); + + modelBuilder.Entity(builder => + { + builder.Property(d => d.RowVersion) + .IsRequired(false) + .IsConcurrencyToken(false) + .ValueGeneratedNever(); + }); + + var converter = new ValueConverter( + v => v.UtcDateTime.ToString("o"), + v => DateTimeOffset.Parse(v)); + var nullableConverter = new ValueConverter( + v => v.HasValue ? v.Value.UtcDateTime.ToString("o") : null, + v => v != null ? DateTimeOffset.Parse(v) : null); + + foreach (var entityType in modelBuilder.Model.GetEntityTypes()) + { + foreach (var property in entityType.GetProperties()) + { + if (property.ClrType == typeof(DateTimeOffset)) + { + property.SetValueConverter(converter); + property.SetColumnType("TEXT"); + } + else if (property.ClrType == typeof(DateTimeOffset?)) + { + property.SetValueConverter(nullableConverter); + property.SetColumnType("TEXT"); + } + } + } + } + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementEndpointsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementEndpointsTests.cs index 965e72d1..f3ad76f5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementEndpointsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementEndpointsTests.cs @@ -23,6 +23,28 @@ public class ManagementEndpointsTests Assert.Equal("Desc", command.Description); } + /// + /// P7-6: MgmtDeploySiteCommand.Force is an ADDITIVE field, so a client + /// that predates it — every CLI older than this change, and every scripted + /// HTTP caller — must keep working and must land on the SAFE default. Absent + /// means false means "skip instances whose config is unchanged", which + /// is the non-destructive behaviour; only an explicit true restores the + /// unconditional redeploy. + /// + [Theory] + [InlineData("""{ "command": "MgmtDeploySite", "payload": { "siteId": 7 } }""", false)] + [InlineData("""{ "command": "MgmtDeploySite", "payload": { "siteId": 7, "force": false } }""", false)] + [InlineData("""{ "command": "MgmtDeploySite", "payload": { "siteId": 7, "force": true } }""", true)] + public void ParseCommand_DeploySite_ForceDefaultsToTheNonDestructiveBehaviour(string json, bool expectedForce) + { + var result = ManagementEndpoints.ParseCommand(json); + + Assert.True(result.Success); + var command = Assert.IsType(result.Command); + Assert.Equal(7, command.SiteId); + Assert.Equal(expectedForce, command.Force); + } + [Fact] public void ParseCommand_WithMissingPayload_DeserializesParameterlessCommand() {