From e63ffd90ee8c868d75e77ca171015f4f7f3564be Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 14:47:34 -0400 Subject: [PATCH] fix(siteruntime): stamp the ensure-started heal with the store-write generation + order the redeploy override reset ahead of the successor (arch-review 2026-08-19 P1-1 + P1-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1-1 — the row-67 ensure-started self-heal re-read the durable row off-thread but captured no _storeWriteGeneration and never ran IsStoreSnapshotStale, unlike the boot load and the reconcile sweep. A Disable or Delete committing inside that window left the result carrying an enabled config that passed every guard (neither command sets any of the three pipeline markers ShouldAttemptEnsureStart re-checks), so the heal started an instance the operator had just stopped — PERMANENTLY, since the sweep iterates store rows only and has no runtime→store stop leg. The generation is now captured at dispatch, threaded on EnsureInstanceStartedResult and checked on landing, and the comment claiming the stored row already covered this is corrected. The heal reads through _configLoader, the same seam the boot load and sweep use (identical production call), so a test can hold the read open across a committed disable/delete. P1-2 — the "static overrides reset on redeployment" invariant was racy in both directions. ApplyDeployment created the successor Instance Actor first and only then dispatched the clear, queued behind a full config-JSON write, so the successor's PreStart override load routinely won and applied the PREDECESSOR's overrides — keys absent from the new config included — to in-memory state for the actor's lifetime while the disk ended cleared; a later restart then silently flipped the values. The clear is now dispatched BEFORE CreateInstanceActor and handed to the actor as a gate its override load chains behind; the persistence task joins the same task, so a failed clear still fails the deploy exactly as before. Leg 2: the predecessor's PostStop flush was fire-and-forget and could commit up to one coalescing window AFTER the clear (the Deployment Manager gates the clear on Terminated), durably resurrecting overrides — PostStop now waits for it, bounded at 5s with a Warning on overrun. Tests: new RedeployStaticOverrideResetTests (5) pins both legs plus the gate mechanism and first-deploy behaviour; three new cases in ReconcileStartsInstanceTests pin the discarded disable/delete heals and that an unraced heal still starts. InstanceActorTests' StaticOverride_ResetOnRedeployment comment claimed to simulate an ordering the production path did not have — corrected. --- .../Actors/DeploymentManagerActor.cs | 80 ++++- .../Actors/InstanceActor.cs | 98 +++++- .../Actors/InstanceActorTests.cs | 6 +- .../Actors/ReconcileStartsInstanceTests.cs | 161 ++++++++- .../RedeployStaticOverrideResetTests.cs | 333 ++++++++++++++++++ 5 files changed, 650 insertions(+), 28 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/RedeployStaticOverrideResetTests.cs 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()); + } +}