diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 6c633e4f..54b68218 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -1197,9 +1197,27 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { var instanceName = command.InstanceUniqueName; + // Static overrides and mirrored native alarm state are reset on redeployment per + // design decision — the new config may bind different sources, and the source snapshot + // re-seeds the mirror on (re)subscribe. Dispatched HERE, BEFORE the successor exists, + // and handed to it as a gate (arch-review 2026-08-19 P1-2). It used to run inside the + // persistence task below, i.e. after the actor was already created AND queued behind a + // full config-JSON write, so the successor's PreStart override load routinely won the + // race and applied the PREDECESSOR's overrides — including keys absent from the new + // config — to in-memory state for the actor's whole lifetime, while the disk ended up + // cleared. That combination is worse than either half alone: the running instance + // silently disagreed with its own store, and a later restart flipped the values. + var staticStateResetGate = Task.Run(async () => + { + await _storage.ClearStaticOverridesAsync(instanceName); + await _storage.ClearNativeAlarmsForInstanceAsync(instanceName); + }); + // Create the Instance Actor immediately. CreateInstanceActor first ensures the DCL // connections this config's data-sourced attributes subscribe through (register row 85). - CreateInstanceActor(instanceName, command.FlattenedConfigurationJson, command.RevisionHash); + CreateInstanceActor( + instanceName, command.FlattenedConfigurationJson, command.RevisionHash, + staticStateResetGate); if (!isRedeploy) _deployedInstanceNames.Add(instanceName); UpdateInstanceCounts(); @@ -1228,11 +1246,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers command.RevisionHash, isEnabled: true); - // Static overrides and mirrored native alarm state are reset on - // redeployment per design decision — the new config may bind different - // sources, and the source snapshot re-seeds the mirror on (re)subscribe. - await _storage.ClearStaticOverridesAsync(instanceName); - await _storage.ClearNativeAlarmsForInstanceAsync(instanceName); + // Join the reset dispatched ahead of actor creation, so a failed clear still + // fails the deployment exactly as it did when the clear ran inline here. + await staticStateResetGate; // No explicit replication step: deployed_configurations is a replicated table, // so the write above is captured and shipped to the peer like any other row. @@ -1566,13 +1582,26 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Re-read the durable row off the actor thread. The stored row — NOT the requesting // message — is the authority on the config body AND on enabled/disabled state, so a - // disable that landed after the reconcile fetch still wins and the instance stays - // stopped. + // disable that landed before this read still wins and the instance stays stopped. + // + // Row 101: a disable/delete that lands WHILE the read is in flight is a different + // problem, and the row itself cannot catch it — the read already saw the enabled row, + // and neither Disable nor Delete leaves any of the three markers + // ShouldAttemptEnsureStart re-checks. So capture the generation on the actor thread + // BEFORE the read starts and let the landing handler discard a superseded snapshot, + // exactly as the boot load and the sweep do. Without it the heal could start an + // instance the operator had just disabled or deleted, PERMANENTLY: the sweep only ever + // walks store rows, so it has no runtime→store stop leg to undo it. + var generationAtRead = _storeWriteGeneration; Task.Run(async () => { - var configs = await _storage.GetAllDeployedConfigsAsync(); + // Read through the same seam the boot load and the sweep read through + // (_configLoader defaults to _storage.GetAllDeployedConfigsAsync, so this is the + // identical production call), which lets a test hold the read open and land the + // result behind a committed disable/delete. + var configs = await _configLoader(); var config = configs.FirstOrDefault(c => c.InstanceUniqueName == instanceName); - return new EnsureInstanceStartedResult(command, config, null); + return new EnsureInstanceStartedResult(command, config, null, generationAtRead); }).ContinueWith(t => t.IsCompletedSuccessfully ? t.Result : new EnsureInstanceStartedResult(command, null, t.Exception?.GetBaseException().Message)) @@ -1620,6 +1649,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers return; } + // Row 101, ensure-started half: this node disabled, deleted or redeployed an instance + // while the read was in flight, so the snapshot predates state the actor has already + // advanced past. Discard it — starting from it would resurrect an instance the operator + // just stopped, and nothing would ever stop it again. Nothing is lost by dropping it: + // the heal is advisory, and the instance reconcile sweep re-reads the store every tick. + if (IsStoreSnapshotStale(result.StoreGenerationAtRead, "ensure-started")) + return; + // No stored row: nothing to start. Normal when the config was superseded/removed // between the reconcile fetch and this read, and on an UNREPLICATED site whose peer // node healed its own file — this singleton's storage legitimately has no such row. @@ -3216,7 +3253,15 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// the single creation funnel, so recording it here covers every path that can ever start an /// Instance Actor. /// - internal void CreateInstanceActor(string instanceName, string configJson, string revisionHash) + /// + /// The deploy path's static-override / native-alarm reset, dispatched BEFORE this call so the + /// new actor's PreStart override load can be ordered after it (arch-review 2026-08-19 P1-2). + /// on every non-deploy start (boot batch, enable, self-heal), where + /// nothing is being reset and the load runs immediately as before. + /// + internal void CreateInstanceActor( + string instanceName, string configJson, string revisionHash, + Task? staticStateResetGate = null) { // Deferred register row 85 — an Instance Actor is useless without the DCL connection // actors its data-sourced attributes subscribe through, so ensuring them belongs in the @@ -3262,7 +3307,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _loggerFactory.CreateLogger(), _dclManager, _healthCollector, - _serviceProvider)); + _serviceProvider, + null, + staticStateResetGate)); var actorRef = Context.ActorOf(props, instanceName); _instanceActors[instanceName] = actorRef; @@ -3461,8 +3508,15 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// the actor thread. No original sender: the ensure-started path is fire-and-forget, so /// there is nobody to answer. /// + /// + /// Row 101: captured on the actor thread when the read + /// was dispatched. A mismatch on landing means a disable/delete/deploy committed while the + /// read was in flight, and the heal is dropped whole (see + /// ). + /// internal record EnsureInstanceStartedResult( - EnsureInstanceStartedCommand Command, DeployedInstance? Config, string? Error); + EnsureInstanceStartedCommand Command, DeployedInstance? Config, string? Error, + long StoreGenerationAtRead = 0); /// /// Periodic instance reconcile-sweep tick (deferred register rows 78 + 72). Interval comes diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 21f4e7d7..e0a3aec5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -111,6 +111,25 @@ public class InstanceActor : ReceiveActor private ICancelable? _staticOverrideFlushTimer; private readonly TimeSpan _staticOverrideFlushInterval; + /// + /// The deploy path's static-override / native-alarm reset, supplied by + /// on a (re)deploy and + /// on every other start (arch-review 2026-08-19 P1-2). When set, + /// starts the override load only once it has completed, so a + /// redeploy can never load the PREDECESSOR's overrides into the successor's memory. + /// + private readonly Task? _staticStateResetGate; + + /// + /// How long waits for the final coalesced static-override flush to + /// commit. The wait exists so a redeploy's clear is ordered AFTER this actor's last write + /// (arch-review 2026-08-19 P1-2 leg 2) — the Deployment Manager gates the clear on the + /// Terminated signal, which a fire-and-forget flush could outlive and durably resurrect + /// overrides the redeploy had just cleared. Bounded so a wedged SQLite write cannot stall + /// the redeploy indefinitely; exceeding it is logged and the stop proceeds. + /// + private static readonly TimeSpan FinalStaticOverrideFlushTimeout = TimeSpan.FromSeconds(5); + /// /// Cached string forms of (perf remediation, arch-review /// WP1.5) — the TagValueUpdate ingest path calls this once or twice per update, so a @@ -182,6 +201,11 @@ public class InstanceActor : ReceiveActor /// Optional Data Connection Layer manager actor reference. /// Optional health collector for reporting metrics. /// Optional DI service provider for script execution services. + /// Coalescing window for buffered static-override writes. + /// + /// Optional (re)deploy static-override / native-alarm reset to order the PreStart override + /// load behind — see . + /// public InstanceActor( string instanceUniqueName, string configJson, @@ -194,7 +218,8 @@ public class InstanceActor : ReceiveActor IActorRef? dclManager = null, ISiteHealthCollector? healthCollector = null, IServiceProvider? serviceProvider = null, - TimeSpan? staticOverrideFlushInterval = null) + TimeSpan? staticOverrideFlushInterval = null, + Task? staticStateResetGate = null) { _instanceUniqueName = instanceUniqueName; _storage = storage; @@ -208,6 +233,7 @@ public class InstanceActor : ReceiveActor _serviceProvider = serviceProvider; // Defaults to 100 ms, mirroring NativeAlarmActor's persistFlushInterval (P4). _staticOverrideFlushInterval = staticOverrideFlushInterval ?? TimeSpan.FromMilliseconds(100); + _staticStateResetGate = staticStateResetGate; // Deserialize the flattened configuration _configuration = JsonSerializer.Deserialize(configJson); @@ -326,9 +352,20 @@ public class InstanceActor : ReceiveActor // the actor), and on failover/restart; this single point covers them all. LogLifecycleEvent($"Instance {_instanceUniqueName} started"); - // Asynchronously load static overrides from SQLite and pipe to self + // Asynchronously load static overrides from SQLite and pipe to self. On a (re)deploy + // the read is chained behind the reset the Deployment Manager dispatched before this + // actor existed, so it can only ever observe the CLEARED table (arch-review + // 2026-08-19 P1-2). A faulted reset does not block the load — the deploy itself fails + // on that fault through the persistence join, which stops this actor anyway. var self = Self; - _storage.GetStaticOverridesAsync(_instanceUniqueName).ContinueWith(t => + var instanceName = _instanceUniqueName; + var storage = _storage; + var load = _staticStateResetGate == null + ? storage.GetStaticOverridesAsync(instanceName) + : _staticStateResetGate + .ContinueWith(_ => storage.GetStaticOverridesAsync(instanceName)) + .Unwrap(); + load.ContinueWith(t => { if (t.IsCompletedSuccessfully) return new LoadOverridesResult(t.Result, null); @@ -356,12 +393,15 @@ public class InstanceActor : ReceiveActor timer.Cancel(); _tagSubscribeRetryTimers.Clear(); - // Best-effort final flush of any coalesced static-override writes so a graceful - // stop does not lose the last <= flush-interval of writes (P4 pattern, mirrors - // NativeAlarmActor.PostStop). Fire-and-forget — the in-memory attribute value - // (already applied) remains authoritative even if this final persist is lost. + // Final flush of any coalesced static-override writes so a graceful stop does not lose + // the last <= flush-interval of writes (P4 pattern, mirrors NativeAlarmActor.PostStop). + // AWAITED, not fire-and-forget (arch-review 2026-08-19 P1-2 leg 2): a redeploy stops + // this actor, waits for Terminated, and only then clears the static overrides — so a + // detached final flush could commit up to one flush interval of the PREDECESSOR's dirty + // writes AFTER that clear and durably resurrect them. Blocking here is what puts the + // write provably before the Terminated signal the clear is gated on. _staticOverrideFlushTimer?.Cancel(); - FlushDirtyStaticOverrides(); + WaitForFinalStaticOverrideFlush(); // Operational `instance_lifecycle` event — instance stopped. An // instance stops on disable, delete, redeployment, and graceful shutdown; @@ -571,11 +611,16 @@ public class InstanceActor : ReceiveActor /// failed write never blocks the actor; the in-memory attribute value (already /// applied) remains authoritative. /// - private void FlushDirtyStaticOverrides() + /// + /// The persist task, so the stop path can order the FINAL flush before termination + /// (). Every other caller ignores it, keeping + /// the fire-and-forget semantics unchanged. Completed when there was nothing dirty. + /// + private Task FlushDirtyStaticOverrides() { _staticOverrideFlushTimer = null; if (_dirtyStaticOverrides.Count == 0) - return; + return Task.CompletedTask; var batch = _dirtyStaticOverrides .Select(kv => (AttributeName: kv.Key, Value: kv.Value)) @@ -584,12 +629,41 @@ public class InstanceActor : ReceiveActor var instanceName = _instanceUniqueName; var logger = _logger; - _storage.SetStaticOverridesAsync(instanceName, batch) - .ContinueWith(t => logger.LogWarning( + var persist = _storage.SetStaticOverridesAsync(instanceName, batch); + persist.ContinueWith(t => logger.LogWarning( t.Exception?.GetBaseException(), "Failed to persist {Count} static overrides for {Instance}; in-memory state is authoritative", batch.Count, instanceName), TaskContinuationOptions.OnlyOnFaulted); + return persist; + } + + /// + /// Runs the stop-path static-override flush and waits for it to commit, bounded by + /// . A fault is swallowed — the + /// OnlyOnFaulted continuation inside has already + /// logged it, and a persist failure must never turn a graceful stop into a crash. + /// + private void WaitForFinalStaticOverrideFlush() + { + var flush = FlushDirtyStaticOverrides(); + if (flush.IsCompleted) + return; + + try + { + if (!flush.Wait(FinalStaticOverrideFlushTimeout)) + { + _logger.LogWarning( + "Final static-override flush for {Instance} did not complete within {Timeout}; " + + "stopping anyway — a redeploy's override reset may race it", + _instanceUniqueName, FinalStaticOverrideFlushTimeout); + } + } + catch (Exception) + { + // Already logged by the flush's OnlyOnFaulted continuation. + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs index ee13aea0..fad294d4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs @@ -352,7 +352,11 @@ public class InstanceActorTests : TestKit, IDisposable var overrides = await _storage.GetStaticOverridesAsync("PumpRedeploy"); Assert.Single(overrides); - // Clear overrides (simulates what DeploymentManager does on redeployment) + // Clear overrides BEFORE the successor exists — the ordering DeploymentManager now + // guarantees (arch-review 2026-08-19 P1-2). It used to clear inside the deploy's + // persistence task, i.e. after creating the actor, so this "simulation" pinned an + // ordering the production path did not have; the end-to-end contract lives in + // RedeployStaticOverrideResetTests. await _storage.ClearStaticOverridesAsync("PumpRedeploy"); overrides = await _storage.GetStaticOverridesAsync("PumpRedeploy"); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs index d9cd34bc..a15813bb 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs @@ -90,7 +90,9 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable /// would no longer distinguish the two mechanisms. /// /// - private IActorRef CreateDeploymentManager(IServiceProvider? serviceProvider = null) + private IActorRef CreateDeploymentManager( + IServiceProvider? serviceProvider = null, + Func>>? configLoader = null) { var health = new DeploymentManagerStartupProbe(); var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( @@ -104,7 +106,10 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable healthCollector: health, serviceProvider: serviceProvider, loggerFactory: null, - configFetcher: null))); + configFetcher: null, + // In position: an expression tree rejects a named argument that skips a parameter. + startupLoadRetryInterval: null, + configLoader: configLoader))); AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), TimeSpan.FromMilliseconds(25)); @@ -382,6 +387,120 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable ExpectMsg(TimeSpan.FromSeconds(5)).Status); } + // ── Row 101 applied to the heal: a result that outlived its snapshot ── + + /// + /// The heal's storage read is asynchronous, so a lifecycle command can commit while it is + /// in flight — and the READ ITSELF cannot notice, because it already saw the enabled row and + /// neither disable nor delete leaves any of the three pipeline markers + /// ShouldAttemptEnsureStart re-checks. Starting from that superseded snapshot is + /// PERMANENT: the instance reconcile sweep walks store rows only, so it has no + /// runtime→store stop leg that would ever take the ghost instance back down. + /// + /// + /// This test holds the read open, commits a disable behind it, then releases it — the + /// interleaving the fix's store-write-generation stamp exists to reject (arch-review + /// 2026-08-19 P1-1). Fails before the fix: the stale enabled snapshot passes every + /// guard and the Instance Actor is created for a disabled instance. + /// + /// + [Fact] + public async Task EnsureInstanceStarted_ResultLandingAfterADisable_IsDiscarded() + { + const string instanceName = "RacedDisablePump"; + var loader = new GatedConfigLoader(_storage); + var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-rd", "rev-rd", true); + + // The heal reads the (still enabled) row and parks on the gate. + loader.Arm(); + dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile")); + await loader.WaitUntilParkedAsync(); + + // The operator disables the instance; the write commits and the manager processes both + // halves of the generation bump while the heal's snapshot is still in flight. + dm.Tell(new DisableInstanceCommand("cmd-disable", instanceName, DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(5)); + await AwaitAssertAsync(async () => + { + var configs = await _storage.GetAllDeployedConfigsAsync(); + Assert.False(configs.Single(c => c.InstanceUniqueName == instanceName).IsEnabled); + }, TimeSpan.FromSeconds(5)); + + // Now let the superseded result land. + loader.Release(); + await Task.Delay(1500); + + Assert.False( + await InstanceIsRunningAsync(dm, instanceName), + "The heal started an instance the operator had just disabled — and nothing would " + + "ever stop it again (arch-review 2026-08-19 P1-1)"); + } + + /// + /// The delete half of the same race. Worse than the disable case: the durable row is GONE, + /// so the resurrected instance has no config row at all — the sweep cannot even see it to + /// reason about, and the ghost survives until the node restarts. + /// + /// Fails before the fix at the final assert. + /// + [Fact] + public async Task EnsureInstanceStarted_ResultLandingAfterADelete_IsDiscarded() + { + const string instanceName = "RacedDeletePump"; + var loader = new GatedConfigLoader(_storage); + var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-rx", "rev-rx", true); + + loader.Arm(); + dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile")); + await loader.WaitUntilParkedAsync(); + + dm.Tell(new DeleteInstanceCommand("cmd-delete", instanceName, DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(5)); + await AwaitAssertAsync(async () => + { + var configs = await _storage.GetAllDeployedConfigsAsync(); + Assert.DoesNotContain(configs, c => c.InstanceUniqueName == instanceName); + }, TimeSpan.FromSeconds(5)); + + loader.Release(); + await Task.Delay(1500); + + Assert.False( + await InstanceIsRunningAsync(dm, instanceName), + "The heal resurrected a deleted instance from a snapshot read before the delete " + + "(arch-review 2026-08-19 P1-1)"); + } + + /// + /// The complement of the two tests above: with no mutation in the read's window the heal + /// still starts the instance, so the generation check rejects superseded snapshots without + /// disabling the row-67 repair itself. + /// + [Fact] + public async Task EnsureInstanceStarted_ResultLandingWithNoInterveningWrite_StillStarts() + { + const string instanceName = "UnracedPump"; + var loader = new GatedConfigLoader(_storage); + var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-ok", "rev-ok", true); + + loader.Arm(); + dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile")); + await loader.WaitUntilParkedAsync(); + loader.Release(); + + await AwaitAssertAsync(async () => + Assert.True(await InstanceIsRunningAsync(dm, instanceName)), TimeSpan.FromSeconds(10)); + } + /// /// Resolves the Instance Actor child of the Deployment Manager by path — the direct /// "the InstanceActor exists" check, complementing the debug-snapshot probe. @@ -390,6 +509,44 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable await Sys.ActorSelection(deploymentManager.Path / instanceName) .ResolveOne(TimeSpan.FromSeconds(5)); + /// + /// A deployed-config loader that can be held open mid-read, so a test can commit a + /// lifecycle write while a heal's snapshot is provably in flight rather than hoping the + /// two race the right way round. + /// + /// + /// Reads real storage every time and only parks AFTER the read, so the returned snapshot is + /// exactly what production would have seen at that instant. Inert until + /// is called, which keeps the Deployment Manager's boot load (which runs through this same + /// seam) unblocked. + /// + /// + private sealed class GatedConfigLoader(SiteStorageService storage) + { + private TaskCompletionSource? _release; + private readonly TaskCompletionSource _parked = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public void Arm() => _release = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + public void Release() => _release!.TrySetResult(); + + public Task WaitUntilParkedAsync() => _parked.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + public async Task> LoadAsync() + { + var configs = await storage.GetAllDeployedConfigsAsync(); + var gate = _release; + if (gate != null) + { + _parked.TrySetResult(); + await gate.Task; + } + return configs; + } + } + /// /// In-test fake : runs a per-deploymentId behavior and /// records every call thread-safely (the fetch runs on a pool thread). diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/RedeployStaticOverrideResetTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/RedeployStaticOverrideResetTests.cs new file mode 100644 index 00000000..deffef9f --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/RedeployStaticOverrideResetTests.cs @@ -0,0 +1,333 @@ +using System.Text.Json; +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Instance; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +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; + +/// +/// Regression tests for arch-review 2026-08-19 P1-2 — "static overrides are reset on +/// redeployment" was racy in both directions. +/// +/// +/// The reset used to run inside the deploy's persistence task, i.e. AFTER the successor +/// Instance Actor had already been created and QUEUED BEHIND a full config-JSON write. Two +/// legs fell out of that ordering: +/// +/// +/// The successor's PreStart override load typically won the race and applied the +/// PREDECESSOR's overrides — including keys the new config no longer declares — to in-memory +/// state for the actor's whole lifetime, while the disk ended up cleared. The instance then +/// silently disagreed with its own store, and a restart flipped the values back. +/// The predecessor's PostStop flush was fire-and-forget, so up to one coalescing window +/// of its dirty overrides could COMMIT after the clear and durably resurrect them. +/// +/// +/// +/// The fix dispatches the reset before the successor exists and hands it to the actor as a +/// gate, and makes the predecessor's final flush complete before its Terminated signal — which +/// the Deployment Manager already waits for. +/// +/// +public class RedeployStaticOverrideResetTests : TestKit, IDisposable +{ + private readonly SiteStorageService _storage; + private readonly ScriptCompilationService _compilationService; + private readonly SharedScriptLibrary _sharedScriptLibrary; + private readonly TestLocalDb _localDb; + + public RedeployStaticOverrideResetTests() + { + _localDb = TestLocalDb.CreateTemp("redeploy-override-test"); + _storage = new SiteStorageService( + _localDb.Db, NullLogger.Instance); + _storage.InitializeAsync().GetAwaiter().GetResult(); + _compilationService = new ScriptCompilationService( + NullLogger.Instance); + _sharedScriptLibrary = new SharedScriptLibrary( + _compilationService, NullLogger.Instance); + } + + void IDisposable.Dispose() + { + // Shut the actors down BEFORE the ILocalDb they write through is disposed. + Shutdown(); + var path = _localDb.Path; + _localDb.Dispose(); + TestLocalDb.DeleteFiles(path); + } + + /// + /// The sweep is disabled so nothing but the deploy path can create or replace an Instance + /// Actor while a test is mid-redeploy. + /// + private IActorRef CreateDeploymentManager() + { + var health = new DeploymentManagerStartupProbe(); + var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions { InstanceSweepIntervalSeconds = 0 }, + NullLogger.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 string MakeConfigJson(string instanceName) => + JsonSerializer.Serialize(new FlattenedConfiguration + { + InstanceUniqueName = instanceName, + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Temperature", Value = "98.6", DataType = "Double" + } + ] + }); + + private async Task ResolveInstanceChildAsync(IActorRef dm, string instanceName) => + await Sys.ActorSelection(dm.Path / instanceName).ResolveOne(TimeSpan.FromSeconds(5)); + + private void Deploy(IActorRef dm, string instanceName, string deploymentId) + { + dm.Tell(new DeployInstanceCommand( + deploymentId, instanceName, "sha256:" + deploymentId, + MakeConfigJson(instanceName), "admin", DateTimeOffset.UtcNow)); + Assert.Equal( + DeploymentStatus.Success, + ExpectMsg(TimeSpan.FromSeconds(10)).Status); + } + + // ── Leg 1: the successor must never load the predecessor's overrides ── + + /// + /// A redeploy must leave the successor on its config defaults. The override written before + /// the redeploy is cleared from the store, so an actor that still serves it is an actor + /// disagreeing with its own durable state — and a later restart would silently flip the + /// value back to the default. + /// + /// + /// Fails before the fix at the attribute assert — but only at production timings: the + /// unfixed clear is queued behind the config-JSON write, and a unit-test config is written in + /// well under a millisecond, so the clear wins the race here anyway. Injecting a 200 ms delay + /// ahead of the unfixed clear (modelling a real config write) fails this test and the late-flush + /// test below, while the fixed code passes both under the same injected delay. The deterministic + /// pin of the mechanism itself is + /// . + /// + /// + [Fact] + public async Task Redeploy_DoesNotApplyPreRedeployOverridesToTheNewActor() + { + const string instanceName = "OverrideRedeployPump"; + var dm = CreateDeploymentManager(); + + Deploy(dm, instanceName, "dep-1"); + + // Write a static override and wait until it is DURABLE, so the redeploy below races the + // successor's load against the clear — not against the predecessor's flush (leg 2, which + // the next tests cover). + var first = await ResolveInstanceChildAsync(dm, instanceName); + first.Tell(new SetStaticAttributeCommand( + "corr-set", instanceName, "Temperature", "200.0", DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(5)).Success); + await AwaitAssertAsync(async () => + Assert.Equal("200.0", (await _storage.GetStaticOverridesAsync(instanceName))["Temperature"]), + TimeSpan.FromSeconds(5)); + + Deploy(dm, instanceName, "dep-2"); + + // Settle: the successor's override load is asynchronous, so reading immediately could + // pass simply by outrunning it. Give the (unfixed) load every chance to land first. + await Task.Delay(1500); + + var second = await ResolveInstanceChildAsync(dm, instanceName); + second.Tell(new GetAttributeRequest( + "corr-get", instanceName, "Temperature", DateTimeOffset.UtcNow)); + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.True(response.Found); + Assert.Equal("98.6", response.Value?.ToString()); + + // And the store agrees — in-memory state and disk must not diverge. + Assert.Empty(await _storage.GetStaticOverridesAsync(instanceName)); + } + + /// + /// The deterministic pin of leg 1's mechanism, at the Instance Actor's own seam: given a + /// reset that has not committed yet, the actor must NOT have read static overrides — no + /// matter how long it has been running. Holding the reset open and clearing the table only + /// after it is released proves the read is ordered behind the reset rather than merely + /// losing a race to it. + /// + [Fact] + public async Task RedeployResetGate_HoldsTheOverrideLoadUntilTheResetHasCommitted() + { + const string instanceName = "GatedLoadPump"; + + // The predecessor's override, still on disk exactly as it is when a redeploy starts. + await _storage.SetStaticOverrideAsync(instanceName, "Temperature", "200.0"); + + var reset = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var actor = ActorOf(Props.Create(() => new InstanceActor( + instanceName, + MakeConfigJson(instanceName), + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions(), + NullLogger.Instance, + null, + null, + null, + null, + reset.Task))); + + // While the reset is outstanding the actor stays on its config default, even though the + // override is sitting in the table it would otherwise have read. + await Task.Delay(750); + actor.Tell(new GetAttributeRequest( + "corr-gated", instanceName, "Temperature", DateTimeOffset.UtcNow)); + Assert.Equal("98.6", ExpectMsg(TimeSpan.FromSeconds(5)).Value?.ToString()); + + // Complete the reset the way the deploy path does — clear, then release. + await _storage.ClearStaticOverridesAsync(instanceName); + reset.SetResult(); + + await Task.Delay(750); + actor.Tell(new GetAttributeRequest( + "corr-gated-2", instanceName, "Temperature", DateTimeOffset.UtcNow)); + Assert.Equal("98.6", ExpectMsg(TimeSpan.FromSeconds(5)).Value?.ToString()); + } + + // ── Leg 2: the predecessor's final flush must precede its Terminated signal ── + + /// + /// The Deployment Manager gates a redeploy's override clear on the predecessor's + /// Terminated signal, so PostStop's final flush is only safe if it has COMMITTED by + /// the time that signal is observed. This pins exactly that ordering: a dirty override that + /// no timer has flushed yet must be on disk the instant Terminated arrives. + /// + /// + /// Fails before the fix (racily — the write is a small local SQLite commit): the + /// flush was fire-and-forget, so Terminated could be observed with nothing written, and in + /// a redeploy that write would then land after the clear. + /// + /// + [Fact] + public async Task InstanceActorStop_CommitsTheFinalOverrideFlush_BeforeTerminated() + { + const string instanceName = "FlushOrderPump"; + + // A flush interval far longer than the test: nothing but PostStop can persist the write. + var actor = ActorOf(Props.Create(() => new InstanceActor( + instanceName, + MakeConfigJson(instanceName), + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions(), + NullLogger.Instance, + null, + null, + null, + TimeSpan.FromMinutes(5)))); + + actor.Tell(new SetStaticAttributeCommand( + "corr-set", instanceName, "Temperature", "77.7", DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(5)).Success); + + // Still only in the coalescing buffer. + Assert.Empty(await _storage.GetStaticOverridesAsync(instanceName)); + + Watch(actor); + Sys.Stop(actor); + ExpectTerminated(actor, TimeSpan.FromSeconds(10)); + + // No delay, no polling: the commit must already have happened. + var overrides = await _storage.GetStaticOverridesAsync(instanceName); + Assert.Equal("77.7", overrides["Temperature"]); + } + + /// + /// The end-to-end statement of leg 2: a redeploy that stops an instance with unflushed + /// static overrides must leave the store EMPTY. A detached final flush could commit after + /// the clear and durably resurrect the predecessor's values — invisible until the next + /// restart loaded them. + /// + [Fact] + public async Task Redeploy_LateFlushFromTheStoppedActor_CannotResurrectClearedOverrides() + { + const string instanceName = "LateFlushPump"; + var dm = CreateDeploymentManager(); + + Deploy(dm, instanceName, "dep-1"); + + // Redeploy while the write is still buffered — no wait for the coalescing timer. + var first = await ResolveInstanceChildAsync(dm, instanceName); + first.Tell(new SetStaticAttributeCommand( + "corr-set", instanceName, "Temperature", "300.0", DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(5)).Success); + + Deploy(dm, instanceName, "dep-2"); + + // Settle well past the predecessor's coalescing window so a late flush would have + // landed by now. + await Task.Delay(1500); + + Assert.Empty(await _storage.GetStaticOverridesAsync(instanceName)); + + var second = await ResolveInstanceChildAsync(dm, instanceName); + second.Tell(new GetAttributeRequest( + "corr-get", instanceName, "Temperature", DateTimeOffset.UtcNow)); + Assert.Equal("98.6", ExpectMsg(TimeSpan.FromSeconds(5)).Value?.ToString()); + } + + /// + /// A FIRST deploy has no predecessor and no reset to wait on: the instance must still come + /// up on its config defaults, and an override written after it must persist normally. Pins + /// that the reset gate did not turn the ordinary deploy path into a no-op. + /// + [Fact] + public async Task FirstDeploy_StillLoadsAndPersistsStaticOverridesNormally() + { + const string instanceName = "FreshPump"; + var dm = CreateDeploymentManager(); + + Deploy(dm, instanceName, "dep-1"); + + var actor = await ResolveInstanceChildAsync(dm, instanceName); + actor.Tell(new SetStaticAttributeCommand( + "corr-set", instanceName, "Temperature", "123.4", DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(5)).Success); + + await AwaitAssertAsync(async () => + Assert.Equal("123.4", (await _storage.GetStaticOverridesAsync(instanceName))["Temperature"]), + TimeSpan.FromSeconds(5)); + + actor.Tell(new GetAttributeRequest( + "corr-get", instanceName, "Temperature", DateTimeOffset.UtcNow)); + Assert.Equal("123.4", ExpectMsg(TimeSpan.FromSeconds(5)).Value?.ToString()); + } +}