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)
This commit is contained in:
Joseph Doherty
@@ -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
|
||||
|
||||
@@ -41,6 +41,25 @@ public class DeploymentManagerOptions
|
||||
/// </summary>
|
||||
public TimeSpan SiteDeploymentTimeoutPerInstance { get; set; } = TimeSpan.FromSeconds(120);
|
||||
|
||||
/// <summary>
|
||||
/// Deadline applied to each query-before-redeploy reconciliation round-trip
|
||||
/// inside a bulk <c>DeploySiteAsync</c> (the single-instance path keeps the
|
||||
/// communication layer's own <c>QueryTimeout</c>).
|
||||
///
|
||||
/// <para>
|
||||
/// Phase 1 is strictly serial, so an unreachable site otherwise costs
|
||||
/// <c>QueryTimeout</c> (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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public TimeSpan SiteDeploymentReconcileTimeout { get; set; } = TimeSpan.FromSeconds(5);
|
||||
|
||||
/// <summary>
|
||||
/// Retention window for TERMINAL deployment records (<c>Success</c>,
|
||||
/// <c>Failed</c>). Deployments are insert-only — one row per attempt, forever —
|
||||
|
||||
@@ -38,6 +38,11 @@ public sealed class DeploymentManagerOptionsValidator : OptionsValidatorBase<Dep
|
||||
$"(was {options.SiteDeploymentTimeoutPerInstance}); it bounds each instance's site round-trip " +
|
||||
$"inside a bulk site deployment.");
|
||||
|
||||
builder.RequireThat(options.SiteDeploymentReconcileTimeout > 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 " +
|
||||
|
||||
@@ -57,6 +57,13 @@ public class DeploymentService
|
||||
/// </summary>
|
||||
private const string TimeoutFailurePrefix = "Communication failure:";
|
||||
|
||||
/// <summary>
|
||||
/// Reason reported on every <see cref="SkippedInstanceResult"/> a bulk site
|
||||
/// deploy produces for a <see cref="InstanceState.Disabled"/> instance.
|
||||
/// </summary>
|
||||
internal const string DisabledSkipReason =
|
||||
"Disabled — bulk deploy preserves disabled state; deploy individually to re-enable";
|
||||
|
||||
/// <summary>Rows deleted per terminal-record purge batch.</summary>
|
||||
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.
|
||||
/// </param>
|
||||
/// <param name="reconcileGate">
|
||||
/// Batch-wide reconcile budget + reachability circuit-breaker for a bulk
|
||||
/// deploy, or <see langword="null"/> on the single-instance path (which keeps
|
||||
/// the communication layer's own query timeout).
|
||||
/// </param>
|
||||
/// <param name="cancellationToken">Cancellation token.</param>
|
||||
/// <returns>
|
||||
/// 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
|
||||
/// </summary>
|
||||
private readonly record struct SendOutcome(DeploymentStatusResponse? Response, Exception? Error);
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="ReconcileUnderBatchGateAsync"/> for why both bounds exist.
|
||||
///
|
||||
/// <para>
|
||||
/// Deliberately not thread-safe: it is only ever touched from the serial
|
||||
/// prepare phase.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="reconcileTimeout">Deadline applied to each reconcile round-trip.</param>
|
||||
private sealed class BulkReconcileGate(TimeSpan reconcileTimeout)
|
||||
{
|
||||
/// <summary>Deadline applied to each reconcile round-trip in this batch.</summary>
|
||||
public TimeSpan ReconcileTimeout { get; } = reconcileTimeout;
|
||||
|
||||
/// <summary>True once a reconcile in this batch hit its deadline.</summary>
|
||||
public bool SiteUnreachable { get; private set; }
|
||||
|
||||
/// <summary>Number of reconciles the circuit-breaker skipped, for the batch's one summary log line.</summary>
|
||||
public int SkippedCount { get; private set; }
|
||||
|
||||
/// <summary>Opens the circuit for the remainder of the batch.</summary>
|
||||
public void MarkUnreachable() => SiteUnreachable = true;
|
||||
|
||||
/// <summary>Counts one reconcile skipped because the circuit is open.</summary>
|
||||
public void CountSkipped() => SkippedCount++;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Deploy every deployable instance at a site in one operation.
|
||||
///
|
||||
@@ -783,9 +836,18 @@ public class DeploymentService
|
||||
/// <see cref="DeployInstanceAsync"/> — 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <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
|
||||
/// <see cref="ReconcileUnderBatchGateAsync"/>, so an unreachable site costs the
|
||||
/// batch one short timeout rather than one 30-second query per instance.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="siteId">The database ID of the site whose instances are deployed.</param>
|
||||
@@ -803,12 +865,30 @@ public class DeploymentService
|
||||
|
||||
var instances = await _siteRepository.GetInstancesBySiteIdAsync(siteId, cancellationToken);
|
||||
if (instances.Count == 0)
|
||||
return Result<SiteDeploymentSummary>.Success(new SiteDeploymentSummary(site.SiteIdentifier, [], 0, 0));
|
||||
return Result<SiteDeploymentSummary>.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<Instance>(instances.Count);
|
||||
var skipped = new List<SkippedInstanceResult>();
|
||||
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<PreparedDeployment>(instances.Count);
|
||||
var results = new List<InstanceDeploymentResult>(instances.Count);
|
||||
var reconcileGate = new BulkReconcileGate(_options.SiteDeploymentReconcileTimeout);
|
||||
var prepared = new List<PreparedDeployment>(deployable.Count);
|
||||
var results = new List<InstanceDeploymentResult>(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<SiteDeploymentSummary>.Success(summary);
|
||||
}
|
||||
@@ -1671,6 +1765,73 @@ public class DeploymentService
|
||||
return null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Runs <see cref="TryReconcileWithSiteAsync"/> for one instance of a BULK
|
||||
/// deploy, under the batch's short per-reconcile deadline and its
|
||||
/// once-per-batch reachability circuit-breaker.
|
||||
///
|
||||
/// <para>
|
||||
/// Phase 1 is strictly serial, so an unreachable site makes every instance pay
|
||||
/// the full communication-layer <c>QueryTimeout</c> (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
|
||||
/// <see cref="DeploymentManagerOptions.SiteDeploymentReconcileTimeout"/> per
|
||||
/// reconcile, and — after the FIRST timeout — skipping the remaining reconciles
|
||||
/// for the rest of the batch.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Skipping loses nothing: a reconcile that times out already returns
|
||||
/// <see langword="null"/> 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// The gate needs no synchronisation: it is touched only from the serial
|
||||
/// prepare phase.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="instance">Instance being prepared.</param>
|
||||
/// <param name="targetRevisionHash">Revision hash this deploy would apply.</param>
|
||||
/// <param name="configJson">Serialized flattened config for the reconciled snapshot.</param>
|
||||
/// <param name="currentUser">User attributed on the reconciliation audit row.</param>
|
||||
/// <param name="gate">The batch's reconcile budget and reachability state.</param>
|
||||
/// <param name="cancellationToken">Cancellation token for the batch.</param>
|
||||
/// <returns>The reconciled prior record, or <see langword="null"/> to proceed with a normal deploy.</returns>
|
||||
private async Task<DeploymentRecord?> 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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The site is queried before a re-deploy only when a
|
||||
/// prior record is stuck <see cref="DeploymentStatus.InProgress"/>, or is
|
||||
@@ -1901,6 +2062,18 @@ public record InstanceDeploymentResult(
|
||||
bool Success,
|
||||
string? ErrorMessage);
|
||||
|
||||
/// <summary>
|
||||
/// One instance a bulk site deployment did not attempt at all — no flatten, no
|
||||
/// site round-trip, and no state change of any kind.
|
||||
/// </summary>
|
||||
/// <param name="InstanceId">Database id of the instance.</param>
|
||||
/// <param name="UniqueName">The instance's unique name.</param>
|
||||
/// <param name="Reason">Why it was skipped, phrased for the operator.</param>
|
||||
public record SkippedInstanceResult(
|
||||
int InstanceId,
|
||||
string UniqueName,
|
||||
string Reason);
|
||||
|
||||
/// <summary>
|
||||
/// Result matrix of a bulk site deployment. Mirrors the shape of
|
||||
/// <c>ArtifactDeploymentSummary</c>: successes are not rolled back when other
|
||||
@@ -1911,8 +2084,16 @@ public record InstanceDeploymentResult(
|
||||
/// <param name="InstanceResults">Per-instance outcomes.</param>
|
||||
/// <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
|
||||
/// <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.
|
||||
/// </param>
|
||||
public record SiteDeploymentSummary(
|
||||
string SiteIdentifier,
|
||||
IReadOnlyList<InstanceDeploymentResult> InstanceResults,
|
||||
int SuccessCount,
|
||||
int FailureCount);
|
||||
int FailureCount,
|
||||
IReadOnlyList<SkippedInstanceResult>? SkippedInstances = null);
|
||||
|
||||
@@ -86,6 +86,23 @@ public sealed class FlattenSession
|
||||
/// </summary>
|
||||
public IReadOnlyCollection<int> VisitedTemplateIds => _templates.Keys;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
///
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public IReadOnlyDictionary<int, long> VisitedTemplateVersions =>
|
||||
_templates.ToDictionary(entry => entry.Key, entry => entry.Value.Version);
|
||||
|
||||
/// <summary>
|
||||
/// Returns the inheritance chain for <paramref name="templateId"/> (the
|
||||
/// template itself, then each ancestor), loading it through
|
||||
|
||||
@@ -62,7 +62,7 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe
|
||||
/// </summary>
|
||||
/// <param name="Structure">Graph structure version at flatten time.</param>
|
||||
/// <param name="InstanceVersion">The instance's own version at flatten time.</param>
|
||||
/// <param name="TemplateVersions">Version of every template the flatten walked, by template id.</param>
|
||||
/// <param name="TemplateVersions">Version every template the flatten walked was READ at, by template id.</param>
|
||||
/// <param name="RevisionHash">The hash that reading produced.</param>
|
||||
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<int, long>(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;
|
||||
}
|
||||
|
||||
|
||||
@@ -45,7 +45,9 @@ public class DeploySiteAsyncTests : TestKit
|
||||
}
|
||||
|
||||
/// <summary>Seeds <paramref name="count"/> deployable instances at the test site.</summary>
|
||||
private List<Instance> ArrangeInstances(int count)
|
||||
/// <param name="count">How many instances to seed.</param>
|
||||
/// <param name="disabledIds">Ids seeded <c>Disabled</c> instead of <c>NotDeployed</c>.</param>
|
||||
private List<Instance> ArrangeInstances(int count, params int[] disabledIds)
|
||||
{
|
||||
var instances = new List<Instance>();
|
||||
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<CancellationToken>());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// P7-2: a bulk site deploy must not restart deliberately-stopped equipment.
|
||||
///
|
||||
/// <para>
|
||||
/// Deploy is a legal transition out of <c>Disabled</c> and the deploy path
|
||||
/// forces the instance to <c>Enabled</c> 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>());
|
||||
}
|
||||
|
||||
await _repo.DidNotReceive().UpdateInstanceAsync(
|
||||
Arg.Is<Instance>(i => i.Id == 2 || i.Id == 4), Arg.Any<CancellationToken>());
|
||||
await _repo.DidNotReceive().AddDeploymentRecordAsync(
|
||||
Arg.Is<DeploymentRecord>(r => r.InstanceId == 2 || r.InstanceId == 4), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// P7-3: phase 1 is serial, so an unreachable site used to cost the batch one
|
||||
/// full 30-second <c>QueryTimeout</c> 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.
|
||||
///
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<CancellationToken>())
|
||||
.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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Answers <c>RefreshDeploymentCommand</c> normally but NEVER answers a
|
||||
/// <c>DeploymentStateQueryRequest</c>, 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.
|
||||
/// </summary>
|
||||
private sealed class SilentQuerySiteActor : ReceiveActor
|
||||
{
|
||||
public SilentQuerySiteActor(Action onQuery)
|
||||
{
|
||||
Receive<SiteEnvelope>(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;
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Answers every <c>RefreshDeploymentCommand</c> after a fixed delay, recording
|
||||
/// how old each command's staging <c>Timestamp</c> already was on arrival.
|
||||
|
||||
+19
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1597,6 +1597,36 @@ public class DeploymentServiceTests : TestKit
|
||||
Assert.Equal(InstanceState.Disabled, instance.State);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task DeployInstanceAsync_DisabledInstance_StillDeploysAndEnables()
|
||||
{
|
||||
var instance = new Instance("SingleDisabled")
|
||||
{
|
||||
Id = 73, SiteId = 1, State = InstanceState.Disabled
|
||||
};
|
||||
_repo.GetInstanceByIdAsync(73, Arg.Any<CancellationToken>()).Returns(instance);
|
||||
SetupValidPipeline(73, "SingleDisabled", "sha256:target");
|
||||
_repo.GetCurrentDeploymentStatusAsync(73, Arg.Any<CancellationToken>())
|
||||
.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]
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// P7-4: a non-structural template edit landing DURING the flatten must not be
|
||||
/// captured as if it were already included.
|
||||
///
|
||||
/// <para>
|
||||
/// 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
|
||||
/// <c>ImportResult.StaleInstanceIds</c> both read through this memo.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>())
|
||||
.Returns(async call =>
|
||||
{
|
||||
_flattenCount++;
|
||||
var session = call.ArgAt<FlattenSession?>(3)!;
|
||||
await session.GetTemplateAsync(TemplateId, _ => Task.FromResult<Template?>(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<FlatteningPipelineResult>.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<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>())
|
||||
.Returns(_ =>
|
||||
{
|
||||
_flattenCount++;
|
||||
var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-01" };
|
||||
return Result<FlatteningPipelineResult>.Success(
|
||||
new FlatteningPipelineResult(config, _currentHash, ValidationResult.Success()));
|
||||
});
|
||||
|
||||
Assert.Equal("sha256:after-edit", await probe.GetCurrentRevisionHashAsync(InstanceId));
|
||||
Assert.Equal(2, _flattenCount);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user