From 957db62df1019c5a2e2dd106034b0172ad528319 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 9 Jul 2026 00:33:40 -0400 Subject: [PATCH] fix(site-runtime): deployment whose Instance Actor dies during init reports Failed, exactly-once reply (S6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Success now requires BOTH persistence commit AND an InstanceActorInitialized readiness signal from the actor's PreStart — persistence can commit before the actor's async init has run or failed, so persistence alone must not report Success. An actor that dies during init never signals readiness; the Terminated fallback fails that deployment and rolls back the optimistic state. The persisted-row rollback is deferred until the store commits so it cannot race the optimistic write. Deviation from plan Task 15: the plan's swallow-only guard did not handle the persistence-first ordering (empirically the store commits before the Terminated signal, so Success was reported for a dead actor). Added the readiness handshake to make the join deterministic. --- .../Actors/DeploymentManagerActor.cs | 196 ++++++++++++++++-- .../Actors/InstanceActor.cs | 6 + .../Messages/InstanceActorInitialized.cs | 19 ++ .../Actors/DeploymentManagerRedeployTests.cs | 34 +++ 4 files changed, 237 insertions(+), 18 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Messages/InstanceActorInitialized.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index c2ff3980..1100a22f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -110,6 +110,30 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private readonly HashSet _deployedInstanceNames = new(); + /// + /// Deployments awaiting their persistence result, keyed by instance name (S6). + /// The entry is added when the Instance Actor is created optimistically and removed + /// by whichever of two paths fires first: + /// + /// on a normal persistence outcome, + /// which then replies Success/Failed; + /// the S6 fallback when the actor dies during + /// init, which replies Failed and rolls back the optimistic state. + /// + /// Removing the entry in exactly one of those paths guarantees the deployer receives + /// exactly one regardless of ordering. + /// + private readonly Dictionary _inFlightDeployments = new(); + + /// + /// Instances whose deployment was failed by the S6 Terminated fallback WHILE their + /// optimistic StoreDeployedConfigAsync was still in flight. The persisted-row + /// rollback must run AFTER the store commits (the two are independent background + /// tasks with no ordering guarantee), so the removal is deferred here and executed by + /// when the store's result finally lands. + /// + private readonly HashSet _initFailedPendingRowRemoval = new(); + /// Akka timer scheduler injected by the framework via . public ITimerScheduler Timers { get; set; } = null!; @@ -266,6 +290,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Internal deploy persistence result Receive(HandleDeployPersistenceResult); + // Instance Actor init confirmation — the second half of the deploy-success + // join (S6). Success is only reported once both this and persistence land. + Receive(HandleInstanceActorInitialized); + // Terminated signal — drains a buffered redeployment once the previous // Instance Actor has fully stopped. Receive(HandleTerminated); @@ -567,6 +595,33 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers name); LogDeploymentEvent("Error", name, $"Instance {name} terminated unexpectedly (init failure or crash) — removed from routing"); + + // S6: if the death happened while a deployment was still in flight, the + // actor died during init — fail that deployment (the persistence path must + // NOT later report Success) and roll back the optimistically-applied state + // so the site never advertises an instance it cannot durably recover. + if (_inFlightDeployments.Remove(name, out var inflight)) + { + inflight.ReplyTo.Tell(new DeploymentStatusResponse( + inflight.DeploymentId, name, DeploymentStatus.Failed, + "Instance actor failed during initialization — see site event log", + DateTimeOffset.UtcNow)); + + if (!inflight.IsRedeploy) + { + _deployedInstanceNames.Remove(name); + UpdateInstanceCounts(); + + // Remove the optimistically-persisted config row so a restart does + // not re-create the failing actor. If the store already committed + // (Persisted), remove it now; otherwise defer the removal until the + // in-flight store completes so the remove cannot race ahead of it. + if (inflight.Persisted) + RollbackPersistedConfig(name); + else + _initFailedPendingRowRemoval.Add(name); + } + } } } @@ -665,6 +720,16 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _deployedInstanceNames.Add(instanceName); UpdateInstanceCounts(); + // Record the in-flight deployment so the deployer is answered exactly once: + // Success only once BOTH persistence commits AND the actor confirms init, or + // Failed via the S6 Terminated fallback if the Instance Actor dies during init. + _inFlightDeployments[instanceName] = new InFlightDeploy + { + DeploymentId = command.DeploymentId, + ReplyTo = sender, + IsRedeploy = isRedeploy + }; + // Persist to SQLite and clear static overrides asynchronously Task.Run(async () => { @@ -704,29 +769,44 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers } /// - /// Reports the deployment outcome to central only after the - /// persistence result is known. On a persistence failure the Instance Actor that was - /// created optimistically is stopped and the deployed-instance counter rolled back, - /// so the in-memory state stays consistent with durable storage, and central is told - /// the deployment . + /// Records the persistence half of the deploy-success join. On a persistence + /// failure the optimistically-created Instance Actor is stopped and the counter + /// rolled back and the deployer is told . On + /// success the in-flight entry is marked persisted and success is reported only once + /// the actor has ALSO confirmed init () — persistence + /// can commit before the actor's async init has run or failed (S6). /// private void HandleDeployPersistenceResult(DeployPersistenceResult result) { - if (result.Success) + // If the S6 Terminated fallback already failed this deployment (the Instance + // Actor died during init), the in-flight entry is gone — swallow the now-stale + // persistence result so the deployer is answered exactly once. + if (!_inFlightDeployments.TryGetValue(result.InstanceName, out var inflight)) { - // Operational `deployment` event — deploy succeeded. - LogDeploymentEvent("Info", result.InstanceName, - $"Instance {result.InstanceName} deployed (deploymentId={result.DeploymentId})"); + // The fallback deferred the persisted-row rollback because the store was + // still in flight then. The store has now committed (Success), so remove + // the orphan row — guaranteed to run after the store, never racing it. + if (_initFailedPendingRowRemoval.Remove(result.InstanceName) && result.Success) + RollbackPersistedConfig(result.InstanceName); - result.OriginalSender.Tell(new DeploymentStatusResponse( - result.DeploymentId, - result.InstanceName, - DeploymentStatus.Success, - null, - DateTimeOffset.UtcNow)); + _logger.LogDebug( + "Persistence result for {Instance} arrived after the deployment was already " + + "resolved (init-time termination) — ignoring (deploymentId={DeploymentId}, success={Success})", + result.InstanceName, result.DeploymentId, result.Success); return; } + if (result.Success) + { + // Persistence committed — record it and complete only if init also confirmed. + inflight.Persisted = true; + TryCompleteDeploy(result.InstanceName, inflight); + return; + } + + // Persistence genuinely failed — fail now regardless of init state. + _inFlightDeployments.Remove(result.InstanceName); + _logger.LogError( "Failed to persist deployment {DeploymentId} for {Instance}: {Error}", result.DeploymentId, result.InstanceName, result.Error); @@ -736,15 +816,15 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers $"Instance {result.InstanceName} deploy failed (deploymentId={result.DeploymentId})", result.Error); - // Persistence failed — undo the optimistic actor creation and counter bump so - // the site does not advertise an instance it cannot durably recover. + // Undo the optimistic actor creation and counter bump so the site does not + // advertise an instance it cannot durably recover. if (_instanceActors.Remove(result.InstanceName, out var orphan)) Context.Stop(orphan); if (!result.IsRedeploy) _deployedInstanceNames.Remove(result.InstanceName); UpdateInstanceCounts(); - result.OriginalSender.Tell(new DeploymentStatusResponse( + inflight.ReplyTo.Tell(new DeploymentStatusResponse( result.DeploymentId, result.InstanceName, DeploymentStatus.Failed, @@ -752,6 +832,61 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers DateTimeOffset.UtcNow)); } + /// + /// Records the init half of the deploy-success join (S6): the Instance Actor + /// finished PreStart and sent . Success is + /// reported only once persistence has ALSO committed. Actors created outside a + /// deployment (startup, enable) have no in-flight entry and are ignored here. + /// + private void HandleInstanceActorInitialized(InstanceActorInitialized msg) + { + if (_inFlightDeployments.TryGetValue(msg.InstanceUniqueName, out var inflight)) + { + inflight.Initialized = true; + TryCompleteDeploy(msg.InstanceUniqueName, inflight); + } + } + + /// + /// Completes a deployment with once BOTH the + /// persistence result and the Instance Actor init confirmation have landed (S6). + /// A no-op until both signals are present. + /// + private void TryCompleteDeploy(string instanceName, InFlightDeploy inflight) + { + if (!inflight.Persisted || !inflight.Initialized) + return; + + _inFlightDeployments.Remove(instanceName); + + // Operational `deployment` event — deploy succeeded. + LogDeploymentEvent("Info", instanceName, + $"Instance {instanceName} deployed (deploymentId={inflight.DeploymentId})"); + + inflight.ReplyTo.Tell(new DeploymentStatusResponse( + inflight.DeploymentId, + instanceName, + DeploymentStatus.Success, + null, + DateTimeOffset.UtcNow)); + } + + /// + /// Fire-and-forget removal of an optimistically-persisted deployed-config row after + /// an init failure (S6). Only ever called once the row's write has committed, so it + /// cannot race the store. A fault is logged, never thrown. + /// + private void RollbackPersistedConfig(string instanceName) + { + Task.Run(() => _storage.RemoveDeployedConfigAsync(instanceName)).ContinueWith(t => + { + if (!t.IsCompletedSuccessfully) + _logger.LogError(t.Exception?.GetBaseException(), + "Failed to roll back the persisted config for {Instance} after an init failure", + instanceName); + }); + } + /// /// Disables an instance: stops the actor and marks as disabled in SQLite. /// @@ -1827,6 +1962,31 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// internal record PendingRedeploy(DeployInstanceCommand Command, IActorRef OriginalSender); + /// + /// Join state for a deployment awaiting confirmation. Deployment Success is + /// reported only once BOTH signals land: (the config committed + /// to SQLite) AND (the Instance Actor finished PreStart and + /// sent ). Persistence can commit before + /// the actor's asynchronous init has run or failed, so persistence alone is not enough + /// to declare success (S6). An actor that dies during init never sets + /// ; the S6 Terminated fallback fails that deployment instead. + /// gates rollback so a failed init rolls back the + /// deployed-instance set and the persisted row only for a genuinely new deployment. + /// + internal sealed class InFlightDeploy + { + /// The deployment id echoed back in the status response. + public required string DeploymentId { get; init; } + /// The deployer to answer exactly once. + public required IActorRef ReplyTo { get; init; } + /// Whether this is a redeploy (an update) rather than a new deployment. + public required bool IsRedeploy { get; init; } + /// Set when the config has committed to SQLite. + public bool Persisted { get; set; } + /// Set when the Instance Actor has finished PreStart successfully. + public bool Initialized { get; set; } + } + /// /// Notify-and-fetch: piped back to self when the deployment's flattened /// config has been fetched from central. Carries the original central sender so the diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 0fdf6717..46590a68 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -262,6 +262,12 @@ public class InstanceActor : ReceiveActor // Subscribe to DCL for data-sourced attributes SubscribeToDcl(); + + // Init succeeded — tell the Deployment Manager so it can confirm the + // deployment (S6). If any step above throws, PreStart aborts before this + // line and the actor is stopped with ActorInitializationException instead, + // so the DM never sees a readiness signal for an instance that failed to init. + Context.Parent.Tell(new InstanceActorInitialized(_instanceUniqueName)); } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Messages/InstanceActorInitialized.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Messages/InstanceActorInitialized.cs new file mode 100644 index 00000000..fe4ba6da --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Messages/InstanceActorInitialized.cs @@ -0,0 +1,19 @@ +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Messages; + +/// +/// Readiness signal an InstanceActor sends to its parent +/// DeploymentManagerActor at the very end of a successful PreStart — +/// after its child Script/Alarm/NativeAlarm actors have been created and its DCL +/// subscriptions issued. If PreStart throws before this point (e.g. a child +/// actor cannot be created), the message is never sent and the actor is stopped +/// with an ActorInitializationException instead. +/// +/// The Deployment Manager uses it to confirm a deployment only once the Instance +/// Actor has actually initialized — SQLite persistence completing is necessary but +/// NOT sufficient, because persistence can commit before the actor's asynchronous +/// init has run (or failed). Reporting deployment Success is therefore gated +/// on BOTH the persistence result AND this signal (S6); an actor that dies during +/// init never sends it, so its deployment is failed by the Terminated fallback. +/// +/// The unique name of the initialized instance. +public sealed record InstanceActorInitialized(string InstanceUniqueName); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs index 3258fb81..e4e90fdf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs @@ -360,4 +360,38 @@ public class DeploymentManagerRedeployTests : TestKit, IDisposable r.Message.Contains("terminated unexpectedly", StringComparison.OrdinalIgnoreCase)); }, TimeSpan.FromSeconds(10)); } + + [Fact] + public async Task Deploy_WhoseActorDiesDuringInit_ReportsFailed_NotSuccess() + { + // A deploy whose config compiles (passing the compile gate) but whose Instance + // Actor dies during init must report Failed — NOT the Success it used to report + // once SQLite persisted, leaving the deployer believing a dead instance is live + // (S6). The reply must be exactly-once. + var actor = CreateDeploymentManager(); + await Task.Delay(500); // empty startup + + actor.Tell(new DeployInstanceCommand( + "dep-dies", "dies-on-init", "r1", + CtorThrowingConfig("dies-on-init"), "admin", DateTimeOffset.UtcNow)); + + var resp = ExpectMsg(TimeSpan.FromSeconds(10)); + Assert.Equal(DeploymentStatus.Failed, resp.Status); + + // Exactly one reply — no late Success from the persistence path. + ExpectNoMsg(TimeSpan.FromMilliseconds(500)); + + // The optimistically-persisted config row is rolled back so a restart does not + // re-create the failing actor. The rollback is fire-and-forget once the store + // commits, so poll for it rather than reading once. + List configs = []; + for (var i = 0; i < 50; i++) + { + configs = await _storage.GetAllDeployedConfigsAsync(); + if (configs.All(c => c.InstanceUniqueName != "dies-on-init")) + break; + await Task.Delay(100); + } + Assert.DoesNotContain(configs, c => c.InstanceUniqueName == "dies-on-init"); + } }