diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 0c98eafb..eb03f550 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -181,7 +181,7 @@ flowchart TD ### Instance Lifecycle Commands - **Disable**: Stops the Instance Actor and its children. Retains the deployed configuration in SQLite so the instance can be re-enabled without redeployment. -- **Enable**: Creates a new Instance Actor from the stored configuration (same as startup). +- **Enable**: Creates a new Instance Actor from the stored configuration (same as startup). An enable that arrives while a deploy/redeploy/delete pipeline still **owns** the instance past its compile warm (predecessor still terminating, or awaiting its persistence/init join) is refused with `A deployment for instance … is in progress; retry the enable after it completes.` — it used to race the deploy into `CreateInstanceActor`, collide with the terminating predecessor's actor name, and then revert `is_enabled`, leaving the redeployed instance durably disabled. A failed enable reverts `is_enabled` only for a name Akka refuses outright, never for a uniqueness collision on a valid name, which is transient (review 2026-10-21 follow-up L3). - **Delete**: Stops the Instance Actor and its children, removes the deployed configuration from local SQLite. Does **not** clear store-and-forward messages. - **Ensure Started** (`EnsureInstanceStartedCommand`, site-internal — never sent by central): advisory, idempotent request to make sure an already-persisted, enabled instance is actually running. Raised by the startup reconcile self-heal; see the note under **Startup Behavior**. Unlike the three commands above it has **no reply** and applies no configuration — it only starts an Instance Actor for a config that is already fully stored, and no-ops when the actor is running, the stored row says disabled, the row is absent, or a deploy for that instance is in flight. - **Instance reconcile sweep** (internal timer, not a command): the periodic singleton-owned check that the running Instance Actors still agree with the durable config rows — starting an enabled instance that has none, and re-applying a stored config a running instance has fallen behind. See the note under **Startup Behavior**; the start half shares its implementation (and therefore its bookkeeping and site events) with **Ensure Started**, and the restart half goes through the ordinary redeploy path. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index f568535e..9a63ba4e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -1799,6 +1799,18 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Warm-then-gate: see TryBufferDuringDeployWarm. if (TryBufferDuringDeployWarm(instanceName, command, replyTo)) return; + // Review 2026-10-21 follow-up L3: a redeploy/delete past its warm still OWNS the instance + // (predecessor terminating, or awaiting its persistence/init join) and will create the + // actor itself. An enable racing it would collide with the terminating predecessor's + // actor name in CreateInstanceActor — refuse it plainly instead. + if (InstanceIsOwnedByADeployPipeline(instanceName)) + { + replyTo.Tell(new InstanceLifecycleResponse(command.CommandId, instanceName, false, + $"A deployment for instance {instanceName} is in progress; retry the enable after it completes.", + DateTimeOffset.UtcNow)); + return; + } + var sender = replyTo; // Row 101 (initiation half) — an is_enabled UPDATE is about to start off-thread. @@ -1853,15 +1865,23 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Lane A L3: is_enabled=1 has already committed; put it back so the durable row // agrees with the runtime (and the sweep does not keep re-trying it). Row 101 bumps at // both ends, as for every other mutation of deployed_configurations. - BumpStoreWriteGeneration("enable revert dispatch"); - var revert = _storage.SetInstanceEnabledAsync(instanceName, false); - revert.ContinueWith(t => + // + // Follow-up L3: ONLY for a name Akka will never accept. A uniqueness collision (a + // valid name still held by a terminating actor) is transient — reverting it would + // leave an instance the operator enabled (or a redeploy just started) durably + // disabled; left enabled, the sweep starts it once the name is free. + if (ShouldRevertEnableAfterCreateFailure(instanceName)) { - if (!t.IsCompletedSuccessfully) - _logger.LogError(t.Exception?.GetBaseException(), - "Failed to revert is_enabled for {Instance} after a failed enable", instanceName); - }); - revert.ContinueWith(_ => new StoreWriteCompleted("enable revert")).PipeTo(Self); + BumpStoreWriteGeneration("enable revert dispatch"); + var revert = _storage.SetInstanceEnabledAsync(instanceName, false); + revert.ContinueWith(t => + { + if (!t.IsCompletedSuccessfully) + _logger.LogError(t.Exception?.GetBaseException(), + "Failed to revert is_enabled for {Instance} after a failed enable", instanceName); + }); + revert.ContinueWith(_ => new StoreWriteCompleted("enable revert")).PipeTo(Self); + } result.OriginalSender.Tell(new InstanceLifecycleResponse( result.Command.CommandId, instanceName, false, createError, DateTimeOffset.UtcNow)); return; @@ -1877,6 +1897,17 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _logger.LogInformation("Instance {Instance} enabled", instanceName); } + /// + /// Whether a failed enable should put is_enabled back to 0: only when the instance + /// name is one Akka refuses outright (it can never start, so leaving it enabled would only + /// make the sweep retry it forever). A collision on a VALID name is transient (review + /// 2026-10-21 follow-up L3). + /// + /// The instance whose enable failed to create its actor. + /// when the enable should be reverted. + internal static bool ShouldRevertEnableAfterCreateFailure(string instanceName) => + !ActorPath.IsValidPathElement(instanceName); + /// /// Reconcile self-heal (deferred register row 67): makes sure an instance whose config was /// (re)stored out-of-band is actually RUNNING. diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs index f754bb1a..275f38ee 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs @@ -230,4 +230,57 @@ public class DeploymentManagerActorNameTests : TestKit, IDisposable return Task.CompletedTask; }, TimeSpan.FromSeconds(10)); } + + /// + /// Follow-up L3: an enable that lands while a redeploy owns the instance (here: queued behind + /// the redeploy's compile warm, so it is dispatched the moment the predecessor actor starts + /// terminating) is refused with a clear "deployment in progress" reply. It used to race the + /// deploy into CreateInstanceActor, collide with the terminating predecessor's name, + /// and then REVERT is_enabled — leaving the freshly redeployed instance disabled. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Enable_DuringRedeploy_IsRefused_AndDoesNotDisableTheInstance() + { + var dm = CreateBootedDeploymentManager(); + Assert.Equal(DeploymentStatus.Success, Deploy(dm, "Pump1").Status); + + dm.Tell(new DeployInstanceCommand( + $"dep-{Guid.NewGuid():N}", "Pump1", "rev-2", ConfigJson("Pump1", "S2"), + "tester", DateTimeOffset.UtcNow)); + dm.Tell(new EnableInstanceCommand("cmd-en", "Pump1", DateTimeOffset.UtcNow)); + + InstanceLifecycleResponse? enable = null; + DeploymentStatusResponse? deploy = null; + for (var i = 0; i < 2; i++) + { + var msg = ExpectMsg(TimeSpan.FromSeconds(20)); + if (msg is InstanceLifecycleResponse l) enable = l; + if (msg is DeploymentStatusResponse d) deploy = d; + } + + Assert.NotNull(enable); + Assert.NotNull(deploy); + Assert.Equal(DeploymentStatus.Success, deploy!.Status); + Assert.False(enable!.Success); + Assert.Contains("deployment", enable.ErrorMessage, StringComparison.OrdinalIgnoreCase); + Assert.Contains("in progress", enable.ErrorMessage, StringComparison.OrdinalIgnoreCase); + + // Nothing reverts the redeployed instance to disabled. + await Task.Delay(500); + Assert.True((await _storage.GetDeployedConfigMetadataAsync("Pump1"))!.IsEnabled); + } + + /// + /// Follow-up L3: only a name Akka will NEVER accept reverts is_enabled after a failed enable; + /// a uniqueness collision (a valid name still held by a terminating actor) is transient and + /// must not flip the durable row to disabled. + /// + [Theory] + [InlineData("a b", true)] + [InlineData("Pump1", false)] + public void EnableCreateFailure_RevertsOnlyForAnInvalidName(string instanceName, bool expectRevert) + { + Assert.Equal(expectRevert, DeploymentManagerActor.ShouldRevertEnableAfterCreateFailure(instanceName)); + } }