e0e4b24679
Six adversarial-review findings, each verified against the code first.
F1 (HIGH) CLI HttpClient capped every call at min(30s, caller timeout),
silently truncating deploy site's 5-minute BulkDeployTimeout and the
5-minute bundle export/preview/import calls — which printed a fake
"504 Request timed out" while the server kept working. HttpClient.Timeout
is now Timeout.InfiniteTimeSpan (the per-call CTS is the single overall
deadline, connect included) with the connect phase bounded separately on
SocketsHttpHandler.ConnectTimeout. The env override is renamed to
SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS to match its new meaning.
F2 (HIGH) StaleInstanceProbe's process-static memo served stale hashes
because nothing bumped the watermark on three paths:
(a) BundleImporter commits through the raw DbContext, so no import ever
moved the watermark — a second import overwriting the same template
could be OMITTED from ImportResult.StaleInstanceIds. It now bumps
once per apply ATTEMPT: after the commit, and after the rollback too
(the probe runs pre-commit, so a rolled-back attempt leaves memos for
state that never landed; bumping on both paths is the simplest
correct shape, versus threading transaction awareness through a
process-static cache).
(b) CollectWatermarkBumps' default: arm silently no-op'd, contradicting
its own doc. It now sets unattributed=true — an over-broad bump costs
extra work, a missed one produces stale work.
(c) DataConnection edits route through SiteRepository.SaveChangesAsync,
which had no watermark at all, yet Protocol/Primary+Backup config/
FailoverRetryCount are revision-hash inputs. It now bumps (BumpAll —
a connection has no owning template) after a commit that touched one.
F3 (MED) CLI TemplateTableProjection read child ARRAYS, but ListTemplates
now returns database-projected TemplateSummary rows, so template list
printed all zeros. It now prefers the *Count scalars and falls back to
array length (template get still returns full entities). --detail help
text and README corrected: a listing cannot yield definitions, so --detail
renders the raw summary payload and template get --id is the full dump.
F4 (MED) DeploySiteAsync staged every PendingDeployment in phase 1 against
a 5-min TTL while phase 2 reached them one batch at a time, so tail
instances' fetch tokens could expire before their command was sent.
Staging moved into phase 2, immediately before each send; prepare keeps
its flatten/validate/record work. The staging write is the phase's only
repository touch and is serialised behind a 1-permit semaphore, so the
non-thread-safe DbContext constraint holds and the sends stay concurrent.
F5 (MED, latent) DeploySiteAsync leaked every held operation lock if
cancelled — a wedged per-instance semaphore is permanent for the process.
Phase 2 no longer throws (cancellation is recorded as a per-instance
outcome so phase 3 still runs), and an escape from phase 1 or 3 now
unwinds every unfinalised entry: Failed status + lock release.
F6 (LOW) ScriptCompileVerdictCache's promotion wrote hot directly,
bypassing SegmentCapacity (true ceiling 3x against a documented 2x).
Promotion now goes through Store, keeping generational semantics; _hot
and _cold are volatile.
Tests: CLI 396, DeploymentManager 133, ManagementService 494,
TemplateEngine 478, ScriptAnalysis 60, Transport 157, Transport
integration 106, ConfigurationDatabase 366 — all green, 0 build warnings.
The F2/F4/F5 regression tests were each confirmed to FAIL with their fix
reverted.
400 lines
17 KiB
C#
400 lines
17 KiB
C#
using System.Collections.Concurrent;
|
|
using System.Diagnostics;
|
|
using Akka.Actor;
|
|
using Akka.TestKit.Xunit2;
|
|
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.Interfaces.Repositories;
|
|
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
|
|
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.TemplateEngine.Flattening;
|
|
|
|
namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests;
|
|
|
|
/// <summary>
|
|
/// WP2.5: bulk site deployment. Pins the properties that make the fan-out safe —
|
|
/// concurrency is BOUNDED by the configured degree, one slow instance does not stop
|
|
/// the others completing, a per-instance timeout contains a wedged instance, and
|
|
/// every instance still gets its own deployment id and its own operation lock.
|
|
/// </summary>
|
|
public class DeploySiteAsyncTests : TestKit
|
|
{
|
|
private const int SiteId = 1;
|
|
|
|
private readonly IDeploymentManagerRepository _repo = Substitute.For<IDeploymentManagerRepository>();
|
|
private readonly IFlatteningPipeline _pipeline = Substitute.For<IFlatteningPipeline>();
|
|
private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();
|
|
private readonly IAuditService _audit = Substitute.For<IAuditService>();
|
|
private readonly OperationLockManager _lockManager = new();
|
|
|
|
public DeploySiteAsyncTests()
|
|
{
|
|
_siteRepo.GetSiteByIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>())
|
|
.Returns(ci => new Site($"Site {ci.ArgAt<int>(0)}", $"site-{ci.ArgAt<int>(0)}") { Id = ci.ArgAt<int>(0) });
|
|
|
|
_pipeline.CreateSession().Returns(_ => new FlattenSession(new TemplateGraphWatermark()));
|
|
}
|
|
|
|
/// <summary>Seeds <paramref name="count"/> deployable instances at the test site.</summary>
|
|
private List<Instance> ArrangeInstances(int count)
|
|
{
|
|
var instances = new List<Instance>();
|
|
for (var i = 1; i <= count; i++)
|
|
{
|
|
var instance = new Instance($"Inst-{i:00}")
|
|
{
|
|
Id = i,
|
|
SiteId = SiteId,
|
|
TemplateId = 10,
|
|
State = InstanceState.NotDeployed
|
|
};
|
|
instances.Add(instance);
|
|
|
|
_repo.GetInstanceByIdAsync(i, Arg.Any<CancellationToken>()).Returns(instance);
|
|
_repo.GetCurrentDeploymentStatusAsync(i, Arg.Any<CancellationToken>()).Returns((DeploymentRecord?)null);
|
|
|
|
var config = new FlattenedConfiguration { InstanceUniqueName = instance.UniqueName };
|
|
_pipeline.FlattenAndValidateAsync(
|
|
i, Arg.Any<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>())
|
|
.Returns(Result<FlatteningPipelineResult>.Success(
|
|
new FlatteningPipelineResult(config, $"sha256:{i}", ValidationResult.Success())));
|
|
}
|
|
|
|
_siteRepo.GetInstancesBySiteIdAsync(SiteId, Arg.Any<CancellationToken>()).Returns(instances);
|
|
return instances;
|
|
}
|
|
|
|
private DeploymentService CreateService(IActorRef commActor, int maxParallelism, TimeSpan? perInstanceTimeout = null)
|
|
{
|
|
var comms = new CommunicationService(
|
|
Options.Create(new CommunicationOptions { DeploymentTimeout = TimeSpan.FromSeconds(30) }),
|
|
NullLogger<CommunicationService>.Instance);
|
|
comms.SetCommunicationActor(commActor);
|
|
|
|
var options = Options.Create(new DeploymentManagerOptions
|
|
{
|
|
OperationLockTimeout = TimeSpan.FromSeconds(5),
|
|
SiteDeploymentMaxParallelism = maxParallelism,
|
|
SiteDeploymentTimeoutPerInstance = perInstanceTimeout ?? TimeSpan.FromSeconds(30)
|
|
});
|
|
|
|
return new DeploymentService(
|
|
_repo, _siteRepo, _pipeline, comms, _lockManager, _audit,
|
|
new DiffService(),
|
|
new RevisionHashService(),
|
|
new DeploymentStatusNotifier(NullLogger<DeploymentStatusNotifier>.Instance),
|
|
options,
|
|
Options.Create(new CommunicationOptions
|
|
{
|
|
CentralFetchBaseUrl = "https://central.test:9000",
|
|
PendingDeploymentTtl = TimeSpan.FromMinutes(5)
|
|
}),
|
|
NullLogger<DeploymentService>.Instance);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task DeploySiteAsync_SlowInstance_OthersStillComplete_AndConcurrencyIsBounded()
|
|
{
|
|
const int instanceCount = 8;
|
|
const int maxParallelism = 3;
|
|
ArrangeInstances(instanceCount);
|
|
|
|
var tracker = new ConcurrencyTracker();
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(tracker, slowInstanceName: "Inst-01", slowDelay: TimeSpan.FromMilliseconds(400))));
|
|
|
|
var service = CreateService(commActor, maxParallelism);
|
|
|
|
var result = await service.DeploySiteAsync(SiteId, "admin");
|
|
|
|
Assert.True(result.IsSuccess);
|
|
var summary = result.Value;
|
|
|
|
// Every instance produced a row and all eight completed, including the
|
|
// seven that were NOT waiting on the slow site round-trip.
|
|
Assert.Equal(instanceCount, summary.InstanceResults.Count);
|
|
Assert.Equal(instanceCount, summary.SuccessCount);
|
|
Assert.Equal(0, summary.FailureCount);
|
|
|
|
// The fan-out never exceeded the configured bound.
|
|
Assert.True(tracker.MaxObserved <= maxParallelism,
|
|
$"observed {tracker.MaxObserved} concurrent site round-trips, bound was {maxParallelism}");
|
|
|
|
// ...and it genuinely WAS concurrent, so the assertion above is not
|
|
// vacuously satisfied by a serial implementation.
|
|
Assert.True(tracker.MaxObserved > 1, "site round-trips did not run concurrently at all");
|
|
|
|
// Deployment identity: one distinct deployment id per instance.
|
|
var deploymentIds = summary.InstanceResults.Select(r => r.DeploymentId).ToList();
|
|
Assert.Equal(instanceCount, deploymentIds.Distinct().Count());
|
|
Assert.DoesNotContain(deploymentIds, id => string.IsNullOrEmpty(id));
|
|
}
|
|
|
|
[Fact]
|
|
public async Task DeploySiteAsync_WedgedInstance_TimesOutAlone_RestSucceed()
|
|
{
|
|
ArrangeInstances(4);
|
|
|
|
var tracker = new ConcurrencyTracker();
|
|
// Instance 2's round-trip is never answered — its per-instance deadline
|
|
// must contain it rather than stalling the batch.
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(tracker, slowInstanceName: "Inst-02", slowDelay: Timeout.InfiniteTimeSpan)));
|
|
|
|
var service = CreateService(commActor, maxParallelism: 4, perInstanceTimeout: TimeSpan.FromMilliseconds(300));
|
|
|
|
var result = await service.DeploySiteAsync(SiteId, "admin");
|
|
|
|
Assert.True(result.IsSuccess);
|
|
var summary = result.Value;
|
|
|
|
Assert.Equal(4, summary.InstanceResults.Count);
|
|
Assert.Equal(3, summary.SuccessCount);
|
|
Assert.Equal(1, summary.FailureCount);
|
|
|
|
var wedged = Assert.Single(summary.InstanceResults, r => !r.Success);
|
|
Assert.Equal("Inst-02", wedged.UniqueName);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task DeploySiteAsync_ReleasesEveryOperationLock()
|
|
{
|
|
ArrangeInstances(5);
|
|
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero)));
|
|
|
|
var service = CreateService(commActor, maxParallelism: 2);
|
|
|
|
await service.DeploySiteAsync(SiteId, "admin");
|
|
|
|
// The per-instance operation lock is held from prepare through finalize;
|
|
// once the batch is done every entry must be reclaimed, or a second bulk
|
|
// deploy of the same site would deadlock on its own leftovers.
|
|
Assert.Equal(0, _lockManager.TrackedLockCount);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task DeploySiteAsync_SiteWithNoInstances_SucceedsWithEmptySummary()
|
|
{
|
|
_siteRepo.GetInstancesBySiteIdAsync(SiteId, Arg.Any<CancellationToken>()).Returns([]);
|
|
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero)));
|
|
|
|
var result = await CreateService(commActor, maxParallelism: 2).DeploySiteAsync(SiteId, "admin");
|
|
|
|
Assert.True(result.IsSuccess);
|
|
Assert.Empty(result.Value.InstanceResults);
|
|
}
|
|
|
|
[Fact]
|
|
public async Task DeploySiteAsync_UnknownSite_ReturnsFailure()
|
|
{
|
|
_siteRepo.GetSiteByIdAsync(99, Arg.Any<CancellationToken>()).Returns((Site?)null);
|
|
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero)));
|
|
|
|
var result = await CreateService(commActor, maxParallelism: 2).DeploySiteAsync(99, "admin");
|
|
|
|
Assert.True(result.IsFailure);
|
|
Assert.Contains("not found", result.Error);
|
|
}
|
|
|
|
/// <summary>
|
|
/// A <c>PendingDeployment</c>'s fetch token expires
|
|
/// <c>PendingDeploymentTtl</c> after the row is STAGED, so any delay between
|
|
/// staging and sending is dead time burned off the token's life. Staging the
|
|
/// whole batch up front in phase 1 made that delay grow with batch size — the
|
|
/// tail instances' tokens could expire before their command was ever sent, and
|
|
/// the site's fetch then 404s.
|
|
///
|
|
/// <para>
|
|
/// This pins the fix: every instance's <c>RefreshDeploymentCommand</c> must
|
|
/// arrive at the site carrying a FRESH <c>Timestamp</c> (the staging instant),
|
|
/// regardless of how long the batch ahead of it took. Run serially with a slow
|
|
/// site so the batch takes far longer than the freshness bound being asserted —
|
|
/// under the old shape the last instance's token would already be ~2 s old.
|
|
/// </para>
|
|
/// </summary>
|
|
[Fact]
|
|
public async Task DeploySiteAsync_StagesEachTokenImmediatelyBeforeItsOwnSend()
|
|
{
|
|
const int instanceCount = 20;
|
|
var perSendDelay = TimeSpan.FromMilliseconds(100);
|
|
ArrangeInstances(instanceCount);
|
|
|
|
var ages = new ConcurrentBag<TimeSpan>();
|
|
var commActor = Sys.ActorOf(Props.Create(() => new TokenAgeRecordingSiteActor(ages, perSendDelay)));
|
|
|
|
// Parallelism 1 makes the batch strictly serial, so the accumulated lag a
|
|
// front-loaded staging phase would produce is at its largest.
|
|
var service = CreateService(commActor, maxParallelism: 1);
|
|
|
|
var sw = Stopwatch.StartNew();
|
|
var result = await service.DeploySiteAsync(SiteId, "admin");
|
|
sw.Stop();
|
|
|
|
Assert.True(result.IsSuccess);
|
|
Assert.Equal(instanceCount, result.Value.SuccessCount);
|
|
Assert.Equal(instanceCount, ages.Count);
|
|
|
|
// The batch really did take a long time — otherwise the freshness assertion
|
|
// below would pass vacuously.
|
|
Assert.True(sw.Elapsed > TimeSpan.FromMilliseconds(1500),
|
|
$"the batch completed in {sw.ElapsedMilliseconds} ms; too fast to prove token freshness");
|
|
|
|
// ...yet no token was stale when its command reached the site.
|
|
var oldest = ages.Max();
|
|
Assert.True(oldest < TimeSpan.FromMilliseconds(500),
|
|
$"a fetch token was already {oldest.TotalMilliseconds:F0} ms old on arrival — " +
|
|
"staging is not tracking the send.");
|
|
}
|
|
|
|
/// <summary>
|
|
/// Cancelling mid-batch must not leak operation locks. Phase 1's
|
|
/// <c>ThrowIfCancellationRequested</c> escapes <c>DeploySiteAsync</c> while every
|
|
/// already-prepared deployment still holds its per-instance lock — and
|
|
/// <c>OperationLockManager</c> hands out real semaphores, so an undisposed handle
|
|
/// wedges that instance against every future mutating command for the life of
|
|
/// the process. Their records must also be finalised as Failed rather than left
|
|
/// InProgress.
|
|
/// </summary>
|
|
[Fact]
|
|
public async Task DeploySiteAsync_CancelledMidPrepare_ReleasesEveryLock_AndFailsPreparedRecords()
|
|
{
|
|
ArrangeInstances(6);
|
|
|
|
using var cts = new CancellationTokenSource();
|
|
|
|
// Cancel while preparing the third instance: the first two are fully
|
|
// prepared and holding locks when the loop's next cancellation check throws.
|
|
_pipeline
|
|
.FlattenAndValidateAsync(3, Arg.Any<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>())
|
|
.Returns(_ =>
|
|
{
|
|
cts.Cancel();
|
|
var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-03" };
|
|
return Result<FlatteningPipelineResult>.Success(
|
|
new FlatteningPipelineResult(config, "sha256:3", ValidationResult.Success()));
|
|
});
|
|
|
|
var commActor = Sys.ActorOf(Props.Create(() =>
|
|
new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero)));
|
|
|
|
var service = CreateService(commActor, maxParallelism: 2);
|
|
|
|
await Assert.ThrowsAnyAsync<OperationCanceledException>(
|
|
() => service.DeploySiteAsync(SiteId, "admin", cts.Token));
|
|
|
|
Assert.Equal(0, _lockManager.TrackedLockCount);
|
|
|
|
// No prepared deployment may be left InProgress.
|
|
await _repo.Received().UpdateDeploymentRecordAsync(
|
|
Arg.Is<DeploymentRecord>(r => r.Status == DeploymentStatus.Failed),
|
|
Arg.Any<CancellationToken>());
|
|
}
|
|
|
|
/// <summary>
|
|
/// Answers every <c>RefreshDeploymentCommand</c> after a fixed delay, recording
|
|
/// how old each command's staging <c>Timestamp</c> already was on arrival.
|
|
/// </summary>
|
|
private sealed class TokenAgeRecordingSiteActor : ReceiveActor
|
|
{
|
|
public TokenAgeRecordingSiteActor(ConcurrentBag<TimeSpan> ages, TimeSpan delay)
|
|
{
|
|
Receive<SiteEnvelope>(env =>
|
|
{
|
|
if (env.Message is not RefreshDeploymentCommand cmd)
|
|
return;
|
|
|
|
ages.Add(DateTimeOffset.UtcNow - cmd.Timestamp);
|
|
|
|
var replyTo = Sender;
|
|
Context.System.Scheduler.Advanced.ScheduleOnce(delay, () =>
|
|
replyTo.Tell(new DeploymentStatusResponse(
|
|
cmd.DeploymentId, cmd.InstanceUniqueName,
|
|
DeploymentStatus.Success, null, DateTimeOffset.UtcNow)));
|
|
});
|
|
}
|
|
}
|
|
|
|
/// <summary>Records the peak number of simultaneously in-flight site round-trips.</summary>
|
|
private sealed class ConcurrencyTracker
|
|
{
|
|
private int _current;
|
|
private int _max;
|
|
|
|
/// <summary>Highest simultaneous in-flight count observed.</summary>
|
|
public int MaxObserved => Volatile.Read(ref _max);
|
|
|
|
/// <summary>Marks one round-trip as started and updates the peak.</summary>
|
|
public void Enter()
|
|
{
|
|
var now = Interlocked.Increment(ref _current);
|
|
int observed;
|
|
while (now > (observed = Volatile.Read(ref _max)))
|
|
{
|
|
if (Interlocked.CompareExchange(ref _max, now, observed) == observed)
|
|
break;
|
|
}
|
|
}
|
|
|
|
/// <summary>Marks one round-trip as finished.</summary>
|
|
public void Exit() => Interlocked.Decrement(ref _current);
|
|
}
|
|
|
|
/// <summary>
|
|
/// Stand-in site that answers <c>RefreshDeploymentCommand</c> with Success,
|
|
/// optionally delaying (or never answering) one named instance so the test can
|
|
/// observe the fan-out's bound and its per-instance deadline.
|
|
///
|
|
/// <para>
|
|
/// The reply is scheduled rather than sent inline, so the actor's mailbox is
|
|
/// not the thing serialising the batch — otherwise the concurrency the test is
|
|
/// measuring would be an artefact of the harness.
|
|
/// </para>
|
|
/// </summary>
|
|
private sealed class ThrottledSiteActor : ReceiveActor
|
|
{
|
|
public ThrottledSiteActor(ConcurrencyTracker tracker, string? slowInstanceName, TimeSpan slowDelay)
|
|
{
|
|
Receive<SiteEnvelope>(env =>
|
|
{
|
|
if (env.Message is not RefreshDeploymentCommand cmd)
|
|
return;
|
|
|
|
var replyTo = Sender;
|
|
var isSlow = slowInstanceName != null && cmd.InstanceUniqueName == slowInstanceName;
|
|
|
|
if (isSlow && slowDelay == Timeout.InfiniteTimeSpan)
|
|
{
|
|
// Never answer: the caller's per-instance deadline must fire.
|
|
tracker.Enter();
|
|
return;
|
|
}
|
|
|
|
var delay = isSlow ? slowDelay : TimeSpan.FromMilliseconds(60);
|
|
tracker.Enter();
|
|
Context.System.Scheduler.Advanced.ScheduleOnce(delay, () =>
|
|
{
|
|
tracker.Exit();
|
|
replyTo.Tell(new DeploymentStatusResponse(
|
|
cmd.DeploymentId, cmd.InstanceUniqueName,
|
|
DeploymentStatus.Success, null, DateTimeOffset.UtcNow));
|
|
});
|
|
});
|
|
}
|
|
}
|
|
}
|