fix(site-runtime): serialise DeployArtifactsCommand applies per site so an overlapping apply cannot delete a live artifact (review 2026-10-21 01/SR-C2)

Each apply upserts its full set then deletes everything else, off the actor thread; two overlapping commands interleaved and deleted artifacts central never removed. The Deployment Manager now runs one apply at a time, queuing the rest in arrival order; the slot is released by a self-message that lands behind the apply's own DCL push.
2026-10-22 06:33:41 -04:00
parent e765700eac
commit 35ffd025c3
3 changed files with 170 additions and 3 deletions
@@ -0,0 +1,101 @@
+using Akka.Actor;
+using Akka.TestKit.Xunit2;
+using Microsoft.Extensions.Logging.Abstractions;
+using ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts;
+using ZB.MOM.WW.ScadaBridge.SiteRuntime.Actors;
+using ZB.MOM.WW.ScadaBridge.SiteRuntime.Persistence;
+using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
+using ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.TestSupport;
+using ZB.MOM.WW.ScadaBridge.TestSupport;
+
+namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors;
+
+/// <summary>
+/// Review 2026-10-21 01/SR-C2: each <see cref="DeployArtifactsCommand"/> apply is "upsert my
+/// full set, then delete everything not in my set", and two overlapping commands used to run
+/// CONCURRENTLY on the thread pool. The interleaving "B upserts Z → A deletes-except(A's set)"
+/// left a centrally-live artifact deleted on the site. Applies are now serialised per site, in
+/// arrival order.
+/// </summary>
+public class DeploymentManagerArtifactSerializationTests : TestKit, IDisposable
+{
+    private readonly SiteStorageService _storage;
+    private readonly ScriptCompilationService _compilationService;
+    private readonly SharedScriptLibrary _sharedScriptLibrary;
+    private readonly TestLocalDb _localDb;
+
+    /// <summary>Creates a temporary LocalDb-backed site store per test.</summary>
+    public DeploymentManagerArtifactSerializationTests()
+    {
+        _localDb = TestLocalDb.CreateTemp("dm-artifact-serial");
+        _storage = new SiteStorageService(_localDb.Db, NullLogger<SiteStorageService>.Instance);
+        _storage.InitializeAsync().GetAwaiter().GetResult();
+        _compilationService = new ScriptCompilationService(NullLogger<ScriptCompilationService>.Instance);
+        _sharedScriptLibrary = new SharedScriptLibrary(
+            _compilationService, NullLogger<SharedScriptLibrary>.Instance);
+    }
+
+    /// <inheritdoc />
+    void IDisposable.Dispose()
+    {
+        Shutdown();
+        var path = _localDb.Path;
+        _localDb.Dispose();
+        TestLocalDb.DeleteFiles(path);
+    }
+
+    private IActorRef CreateBootedDeploymentManager()
+    {
+        var health = new DeploymentManagerStartupProbe();
+        var dm = ActorOf(Props.Create(() => new DeploymentManagerActor(
+            _storage,
+            _compilationService,
+            _sharedScriptLibrary,
+            null,
+            new SiteRuntimeOptions { InstanceSweepIntervalSeconds = 0 },
+            NullLogger<DeploymentManagerActor>.Instance,
+            dclManager: null,
+            healthCollector: health,
+            serviceProvider: null,
+            loggerFactory: null,
+            configFetcher: null)));
+        AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), TimeSpan.FromMilliseconds(25));
+        return dm;
+    }
+
+    private static DeployArtifactsCommand SharedScripts(string deploymentId, IEnumerable<string> names) =>
+        new(deploymentId,
+            names.Select(n => new SharedScriptArtifact(n, $"return \"{n}\".Length + {n.GetHashCode()};", null, null)).ToList(),
+            null, null, null, null, null, DateTimeOffset.UtcNow);
+
+    /// <summary>
+    /// A slow apply (many distinct shared scripts to compile) followed immediately by a fast one
+    /// carrying a new script Z. Serialised in arrival order the final set is the second
+    /// command's, so Z is present in storage AND in the compiled library. Run concurrently, the
+    /// slow apply's delete-except ran after Z was stored and deleted it.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task OverlappingArtifactApplies_DoNotDeleteAnArtifactTheLaterApplyShipped()
+    {
+        var dm = CreateBootedDeploymentManager();
+
+        var slowSet = Enumerable.Range(0, 40).Select(i => $"Slow{i}").ToList();
+        dm.Tell(SharedScripts("dep-A", slowSet));
+        dm.Tell(SharedScripts("dep-B", ["Z"]));
+
+        var first = ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(60));
+        var second = ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(60));
+        Assert.True(first.Success, first.ErrorMessage);
+        Assert.True(second.Success, second.ErrorMessage);
+
+        // Arrival order is apply order, and replies follow it.
+        Assert.Equal("dep-A", first.DeploymentId);
+        Assert.Equal("dep-B", second.DeploymentId);
+
+        var stored = (await _storage.GetAllSharedScriptsAsync()).Select(s => s.Name).ToList();
+        Assert.Equal(["Z"], stored);
+        Assert.True(_sharedScriptLibrary.Contains("Z"));
+        Assert.DoesNotContain(_sharedScriptLibrary.GetRegisteredScriptNames(), n => n.StartsWith("Slow"));
+    }
+}