dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(deploymentmanager): bulk site deploy skips up-to-date instances and un-poisons the shared DbContext (arch-review Low P7-5 + P7-6)

Browse Source 
P7-6 (behaviour fix, plan decision 1): DeploySiteAsync redeployed EVERY
deployable instance unconditionally. A redeploy is not idempotent at the site —
it resets static attribute overrides and clears native alarm state by design — so
one site-wide push destroyed live operational state on every unchanged instance
in the batch, which is normally most of them. Bulk deploy now pre-filters with
IStaleInstanceProbe (the same seam the bundle importer and Deployments page use;
its memo timing was fixed by the Medium wave's P7-4): an instance whose freshly
computed revision hash still matches its DeployedConfigSnapshot is reported in the
existing additive SkippedInstances list with reason "up-to-date" and never
flattened, staged or sent. The filter is deliberately conservative — every
uncertainty deploys: only an Enabled instance qualifies, a missing snapshot
disqualifies, a prior record that ShouldQuerySiteBeforeRedeploy flags goes through
prepare so the reconcile that HEALS it still runs, and a null/faulting probe means
"indeterminate". New additive MgmtDeploySiteCommand.Force (default false = the new
filtered behaviour) plus CLI `deploy site --force` restores the unconditional push
for the case central cannot see (restored volume, rebuilt node). Composes with the
Medium wave's P7-2: Disabled wins — force suppresses the up-to-date filter only,
never the Disabled skip.

P7-5 (verified, then fixed): the cascade REPRODUCES. All three bulk phases share
one scoped DbContext, and a failed SaveChanges does not untrack the offending
entity — EF re-attempts it on the next save, including MarkDeploymentFailedAsync's
cleanup write. Measured on a real SQLite-backed ScadaBridgeDbContext: a permanent
save failure on instance 1's post-success side effects failed instances 2, 3 and 4
and left all three deployment records stranded InProgress. MarkDeploymentFailedAsync
now detaches the pending changes (new IDeploymentManagerRepository.DetachPendingChanges)
when the cleanup save fails and retries the Failed write once on the clean tracker,
so later instances save cleanly and the record reaches a terminal status. The
DeployFailed audit row moved outside the status-write try (best-effort, must not be
lost because the status write failed).

Tests: 5 new DeploySiteAsyncTests (skip + reason + no site round-trip; --force
deploys; Disabled skipped under both force values; the three not-enough-to-skip
guards), 2 new IntegrationTests over a real EF context (the poisoned-context
cascade, and the narrow EF tracking fact the fix rests on), CLI --force flag
parse test, and a MgmtDeploySite wire-default test. Proof of failure: both P7-6
behaviour tests and both P7-5 assertions fail against the unfixed code.
This commit is contained in:
Joseph Doherty
2026-08-19 16:37:42 -04:00
parent 6d0d8b6258
commit b34ec10f8b
14 changed files with 942 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs
+220 -22
View File
@@ -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<DeploymentService> _logger;
@@ -64,6 +66,15 @@ public class DeploymentService
internal const string DisabledSkipReason =
"Disabled — bulk deploy preserves disabled state; deploy individually to re-enable";
/// <summary>
/// Reason reported on every <see cref="SkippedInstanceResult"/> a bulk site
/// deploy produces for an instance whose flattened config already matches the
/// revision the site is running. Suppressed by the <c>force</c> flag.
/// </summary>
internal const string UpToDateSkipReason =
"up-to-date — the flattened config still matches the deployed revision; " +
"redeploy with force to push it anyway";
/// <summary>Rows deleted per terminal-record purge batch.</summary>
private const int TerminalPurgeBatchSize = 500;
@@ -98,6 +109,11 @@ public class DeploymentService
/// staleness hash from the (List-normalized) deserialized snapshot — see I-1.
/// </param>
/// <param name="statusNotifier">Notifier for pushing deployment status changes to the UI.</param>
/// <param name="staleInstanceProbe">
/// Current-revision-hash probe backing the bulk deploy's up-to-date pre-filter
/// (see <see cref="DeploySiteAsync"/>). The same seam the bundle importer and
/// the Deployments page use, so all three agree on what "stale" means.
/// </param>
/// <param name="options">Deployment manager configuration options.</param>
/// <param name="communicationOptions">
/// 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<DeploymentManagerOptions> options,
IOptions<CommunicationOptions> communicationOptions,
ILogger<DeploymentService> 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.
/// </para>
///
/// <para>
/// <b>The cleanup save is un-poisoned before it is given up on (P7-5).</b> A
/// failed <c>SaveChanges</c> 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 <c>DbContext</c> 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 <c>InProgress</c>. On a cleanup-save failure the pending changes
/// are therefore DETACHED (<see cref="IDeploymentManagerRepository.DetachPendingChanges"/>)
/// 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.
/// </para>
/// </summary>
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
/// <para>
/// <b>Disabled instances are skipped entirely</b> and reported in
/// <see cref="SiteDeploymentSummary.SkippedInstances"/> — see the filter below
/// for why. <b>Reconciliation is bounded per batch</b> by
/// for why. <b>Up-to-date instances are skipped too</b> unless
/// <paramref name="force"/> is set, for the reason documented on
/// <see cref="IsUpToDateAsync"/>: a redeploy is destructive to site-local
/// state, so pushing one to an instance whose config has not changed loses
/// data for no benefit. <b>Reconciliation is bounded per batch</b> by
/// <see cref="ReconcileUnderBatchGateAsync"/>, so an unreachable site costs the
/// batch one short timeout rather than one 30-second query per instance.
/// </para>
///
/// <para>
/// The two filters compose in one direction only: <b>Disabled wins</b>. A
/// Disabled instance is never deployed at bulk scope, force or not — force
/// suppresses the up-to-date filter, never the Disabled one.
/// </para>
/// </summary>
/// <param name="siteId">The database ID of the site whose instances are deployed.</param>
/// <param name="user">The username initiating the deployment, recorded on every record and audit row.</param>
/// <param name="force">
/// 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.
/// </param>
/// <param name="cancellationToken">Cancellation token for the operation.</param>
/// <returns>A per-instance result matrix, or a failure result when the site itself cannot be resolved.</returns>
public async Task<Result<SiteDeploymentSummary>> 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<Instance>(instances.Count);
var skipped = new List<SkippedInstanceResult>();
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<SiteDeploymentSummary>.Success(summary);
}
/// <summary>
/// True when a bulk site deploy should leave this instance alone because the
/// site is already running the config it would be sent.
///
/// <para>
/// <b>Why an unconditional bulk redeploy is the wrong default.</b> 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.
/// </para>
///
/// <para>
/// <b>The test is deliberately conservative.</b> 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:
/// </para>
/// <list type="bullet">
/// <item>
/// Only an <see cref="InstanceState.Enabled"/> instance can be up to date.
/// A <see cref="InstanceState.NotDeployed"/> one is not running the config
/// no matter what a leftover snapshot row says.
/// </item>
/// <item>
/// No <see cref="DeployedConfigSnapshot"/> means central has never seen a
/// successful apply — nothing to compare against.
/// </item>
/// <item>
/// A prior record that <see cref="ShouldQuerySiteBeforeRedeploy"/> flags
/// (stuck <c>InProgress</c>, or timeout-<c>Failed</c>) 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.
/// </item>
/// <item>
/// The probe returning <see langword="null"/> (unresolvable template chain)
/// or throwing means staleness is indeterminate.
/// </item>
/// </list>
///
/// <para>
/// The comparison uses the snapshot's own stored <c>RevisionHash</c> rather
/// than <see cref="GetDeploymentComparisonAsync"/>'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.
/// </para>
///
/// <para>
/// <b>Cost.</b> 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.
/// </para>
/// </summary>
/// <param name="instance">The candidate instance.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns><see langword="true"/> only when the instance is provably unchanged.</returns>
private async Task<bool> 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);
}
/// <summary>
/// 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(
/// <param name="SuccessCount">Number of instances the site confirmed.</param>
/// <param name="FailureCount">Number of instances that failed for any reason.</param>
/// <param name="SkippedInstances">
/// Instances the batch deliberately did not attempt — currently the
/// Instances the batch deliberately did not attempt: the
/// <see cref="InstanceState.Disabled"/> 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.
/// </param>
public record SiteDeploymentSummary(
string SiteIdentifier,