From 0d3a90a9fdf552a79a8073fac5301182d09b8bcb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 07:03:26 -0400 Subject: [PATCH] =?UTF-8?q?fix(siteruntime):=20guard=20the=20DCL=20arbitra?= =?UTF-8?q?tion=20overlay=20against=20stale=20store=20snapshots=20?= =?UTF-8?q?=E2=80=94=20register=20row=20101?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An off-thread snapshot read racing an artifact apply could land after the apply populated the overlay; its stale absences then tombstoned and stopped just-applied LIVE connections, which nothing re-creates until an instance redeploy (the removal evicts the hash slot the overlay push is gated on). Each read now captures a definitions-write generation at dispatch; a stale result is discarded whole (additions too — a stale row would reconfigure a live connection backward). Deterministic repro fails before the fix; a fresh-generation guard test pins that row-90 disappearance still fires. --- docs/components/SiteRuntime.md | 2 +- ...-17-dcl-connection-arbitration-decision.md | 25 ++++++ docs/requirements/Component-SiteRuntime.md | 2 + .../Actors/DeploymentManagerActor.cs | 81 +++++++++++++++++-- .../Actors/DclConnectionArbitrationTests.cs | 63 +++++++++++++++ 5 files changed, 166 insertions(+), 7 deletions(-) diff --git a/docs/components/SiteRuntime.md b/docs/components/SiteRuntime.md index 27b04db0..abc2fe6c 100644 --- a/docs/components/SiteRuntime.md +++ b/docs/components/SiteRuntime.md @@ -91,7 +91,7 @@ Central sends a `DeployInstanceCommand` carrying a JSON `FlattenedConfiguration` > **`CreateInstanceActor` is the single funnel for the connection push too (register row 85)**: the `EnsureDclConnections` call moved *into* `CreateInstanceActor` on 2026-08-17. It used to sit at each caller, and only two of the four callers made it — the boot-time batch and this deploy path. `StartStoredInstance` (row-67 ensure-started and the row-78 sweep) and `HandleEnableResult` did not, so a self-healed or newly-enabled instance came up with no connection actors and every tag subscribe was answered `Unknown connection: ` indefinitely. See the *Instance reconcile sweep* section and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. -> **Which config source wins (register rows 89 + 90, 2026-08-17)**: `EnsureDclConnections` no longer trusts the flattened snapshot blindly. `data_connection_definitions` is read on the same off-thread task as the deployed configs, at boot and on every sweep tick, into an actor-thread-confined **arbitration overlay**: a connection name with a row there is configured from that row, and only a name with no row uses the instance's flattened `Connections` entry (which is the correct source on a deploy-only site, where that table is legitimately empty). The winner is resolved **before** the per-name hash is computed, which is what makes the two-source ping-pong — the reason a store-level sweep was rejected in §7.4 — unrepresentable rather than merely avoided. Removals additionally leave an in-memory tombstone so a centrally deleted connection is not resurrected by the next instance start. **The register's premise was wrong in two ways** (the standby's DCL is empty rather than stale, so the real defect is create-time on the next singleton incarnation and is not replication-specific; and a re-issued create never reconfigured a *live* connection actor at all). See `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. +> **Which config source wins (register rows 89 + 90, 2026-08-17)**: `EnsureDclConnections` no longer trusts the flattened snapshot blindly. `data_connection_definitions` is read on the same off-thread task as the deployed configs, at boot and on every sweep tick, into an actor-thread-confined **arbitration overlay**: a connection name with a row there is configured from that row, and only a name with no row uses the instance's flattened `Connections` entry (which is the correct source on a deploy-only site, where that table is legitimately empty). The winner is resolved **before** the per-name hash is computed, which is what makes the two-source ping-pong — the reason a store-level sweep was rejected in §7.4 — unrepresentable rather than merely avoided. Removals additionally leave an in-memory tombstone so a centrally deleted connection is not resurrected by the next instance start. A monotonic write-generation guard (row 101) discards a snapshot whose off-thread read raced an artifact apply — its stale absences would otherwise tombstone and stop just-applied live connections that nothing re-creates until an instance redeploy. **The register's premise was wrong in two ways** (the standby's DCL is empty rather than stale, so the real defect is create-time on the next singleton incarnation and is not replication-specific; and a re-issued create never reconfigured a *live* connection actor at all). See `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. > **A changed config reaches the LIVE connection (register row 97)**: resolving the winner decides *what* to push; a third command was needed to make a push against an already-live connection mean anything, because `DataConnectionManagerActor.HandleCreateConnection` warns and returns for a name it already holds. `EnsureDclConnection` now has three outcomes off the one per-name hash slot — **unchanged → nothing**, **new → `CreateConnectionCommand`**, **changed → `ReconfigureConnectionCommand`** — and the connection actor applies the last of these in place by swapping its adapter through the failover machinery and re-issuing its subscriptions. Log line: `Reconfigured DCL connection {Connection} (protocol={Protocol}) in place` (Information), replacing the "will not take effect" Warning rows 89/90 added. See `DataConnectionLayer.md` → *Reconfiguring a live connection*. diff --git a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md index 460b674d..d3a2e9a3 100644 --- a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md +++ b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md @@ -387,3 +387,28 @@ tests plus SiteRuntime-010, renamed `…_ReissuesReconfigureCommand`). §6 residuals **2** (a removal is not durable across a restart) and **3** (a delete is undetectable on a node that never adopted the row) are unchanged and still open. §6 residual **4** was never a gap. + +## 10. Postscript — the stale-snapshot race (register row 101, 2026-08-17) + +The sweep design read the store off the actor thread (§5) and compared the result against the +overlay only when the piped message landed. That gap was a TOCTOU the original memo did not name: +a read starting before an artifact apply's store writes could land AFTER +`HandleApplyArtifactDataConnectionsToDcl` populated the overlay, and its stale snapshot would then +"prove" the just-applied names disappeared. The disappearance detection (§5) tombstoned them and +stopped the just-created live connection actors — and because the removal also evicts the hash +slot the overlay's push is gated on, the next fresh sweep clears the tombstone but re-creates +nothing: **the connection stays dead until an instance redeploy**. Surfaced as a one-off test flake +(`ApplyingArtifacts_RemovedDataConnection_StopsLiveDclConnection` received a removal for `PlcA` +where `PlcGone` was expected — overlay iteration order removing BOTH names off a stale empty +snapshot), first suspected to be test-fixture-local; verification showed the mechanism is real in +production whenever an artifact deploy lands while a sweep read is in flight. + +**Fix: a definitions-write generation.** `_definitionsWriteGeneration` is bumped on the actor +thread by every artifact apply; each snapshot read captures it at dispatch and a result carrying a +stale generation is discarded WHOLE — additions included, since a stale row pushed at a live +connection would reconfigure it backward and flip-flop with the next fresh read. Discarding is +safe: the apply already updated the overlay in the same pass (§5), and the next tick reads fresh. +Applies to both snapshot consumers (boot load and sweep). Deterministic reproduction + a guard +test that the legitimate row-90 disappearance still fires on a FRESH empty snapshot: +`StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections` (fails before the +fix) and `FreshEmptySweepSnapshot_StillRemovesDisappearedConnections`. diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index ad22f311..2d0db4a6 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -118,6 +118,8 @@ flowchart TD > > **The ping-pong is killed by keying the hash on the winner.** The conflict §7.4 identified is a property of *where the hash is computed*, not of the sweep: every ensure path now resolves the winning configuration **before** hashing or pushing, so for any name only one configuration can ever occupy the hash slot regardless of which path calls. The flip is not guarded against — it is unrepresentable. `HandleApplyArtifactDataConnectionsToDcl` writes its rows into the overlay in the same pass it pushes them, closing the same flip arriving through the artifact-deploy door. > +> **Stale-snapshot guard (row 101).** The snapshot is read off the actor thread but compared against the overlay only when its piped message lands — so a read that starts before an artifact apply's store writes can land *after* `HandleApplyArtifactDataConnectionsToDcl` populated the overlay, and its stale data would "prove" the just-applied names disappeared: tombstoned, live connection actors stopped, and — because the removal evicts the hash slot the overlay's push is gated on — **nothing re-creates them until an instance redeploy**. Each snapshot read therefore captures a monotonic definitions-write generation on the actor thread at dispatch (`_definitionsWriteGeneration`, bumped by every artifact apply), and a result carrying a stale generation is discarded **whole** — additions included, since a stale row pushed at a live connection would reconfigure it backward and flip-flop with the next fresh read. Discarding is safe: the artifact apply already updated the overlay itself, and the next tick reads fresh. Surfaced as a test flake (a raced sweep removed `PlcA` where the test expected `PlcGone`); the deterministic reproduction is `DclConnectionArbitrationTests.StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections`. +> > **Removal (row 90).** Both removal paths share one helper and now record a **tombstone** for the incarnation: the artifact reconcile's `RemovedConnectionNames`, and the sweep noticing that a name it had adopted no longer has a row (a removal it can *prove* — the store owned it a tick ago and does not now). `EnsureDclConnections` skips a tombstoned name, so a removal survives the next instance start instead of being undone by it — a pre-existing bug, since the removal evicted the hash slot but left no memory of why. Tombstones are **in-memory only** by decision: a durable marker would be a second divergent authority, and flattened configs already resurrect a removed connection across a restart. A definition row reappearing clears the tombstone. > > **In-place reconfiguration — a changed config now reaches the LIVE connection (register row 97)**: resolving the winner decides *what* to push; it took a third command to make a push against an already-live connection mean anything. `EnsureDclConnection` has three outcomes, all keyed off the one per-name hash slot: **unchanged → nothing at all** (no message, no log — this is what keeps a healthy site silent across every sweep tick); **new name → `CreateConnectionCommand`**; **changed → `ReconfigureConnectionCommand`**, which the DCL manager forwards to the live connection actor. The actor swaps its adapter through the very machinery the primary→backup failover uses and re-issues every subscription it holds — no redeployment, no restart. The winner-resolution and tombstone logic above are untouched; only what happens *after* a change is detected changed. The log line is `Reconfigured DCL connection {Connection} (protocol={Protocol}) in place` at Information, replacing the Warning rows 89/90 added to make the gap honest. See `Component-DataConnectionLayer.md` → *In-place reconfiguration* for what the connection actor does, and why stop-and-recreate was rejected. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 8282e4cf..e4ca149e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -498,10 +498,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void LoadDeployedConfigs() { + // Row 101: capture on the actor thread BEFORE the off-thread read starts, so a landing + // handler can tell whether an artifact apply mutated the overlay while it was in flight. + var generationAtRead = _definitionsWriteGeneration; ReadStoreSnapshotAsync().ContinueWith(t => { if (t.IsCompletedSuccessfully) - return new StartupConfigsLoaded(t.Result.Configs, null, t.Result.Definitions); + return new StartupConfigsLoaded( + t.Result.Configs, null, t.Result.Definitions, generationAtRead); return new StartupConfigsLoaded([], t.Exception?.GetBaseException().Message); }).PipeTo(Self); } @@ -606,7 +610,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // on the peer node rebuilds the whole DCL from flattened snapshots, and without the // overlay in place first it would rebuild them STALE and only correct itself a sweep // tick later — a needless create-then-diverge on every failover. - ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions); + ApplyConnectionDefinitionOverlayIfFresh( + msg.ConnectionDefinitions, msg.DefinitionsGenerationAtRead, "boot load"); var enabledConfigs = msg.Configs.Where(c => c.IsEnabled).ToList(); _deployedInstanceNames.Clear(); @@ -1629,8 +1634,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _instanceSweepInFlight = true; + // Row 101: capture on the actor thread BEFORE the off-thread read starts. + var generationAtRead = _definitionsWriteGeneration; ReadStoreSnapshotAsync().ContinueWith(t => t.IsCompletedSuccessfully - ? new InstanceSweepLoaded(t.Result.Configs, null, t.Result.Definitions) + ? new InstanceSweepLoaded(t.Result.Configs, null, t.Result.Definitions, generationAtRead) : new InstanceSweepLoaded([], t.Exception?.GetBaseException().Message)) .PipeTo(Self); } @@ -1661,7 +1668,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // convergence path for a definition row that arrived by LocalDb CDC replication: no new // cross-node message exists (or is needed) — whichever node runs the singleton reads the // row out of its own file on the next tick. - ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions); + ApplyConnectionDefinitionOverlayIfFresh( + msg.ConnectionDefinitions, msg.DefinitionsGenerationAtRead, "instance sweep"); foreach (var config in msg.Configs) { @@ -2219,6 +2227,48 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private readonly HashSet _centrallyRemovedConnections = new(StringComparer.Ordinal); + /// + /// Monotonic count of actor-thread definition mutations from artifact applies (deferred + /// register row 101). A store snapshot is read OFF the actor thread and compared against + /// the overlay only when its piped message lands — so a read that started before an + /// artifact apply's store writes can land AFTER populated the overlay, and its stale + /// data would then "prove" the just-applied names disappeared, tombstoning and stopping + /// live connections that nothing re-creates until an instance redeploy. Each snapshot read + /// captures this value on the actor thread at dispatch; a result carrying a stale + /// generation skips the overlay apply entirely (additions too — a stale row pushed via + /// would reconfigure a live connection BACKWARD and set + /// up a flip-flop with the next fresh read). The next tick reads fresh and converges. + /// + private long _definitionsWriteGeneration; + + /// + /// Row 101 guard in front of : applies the + /// snapshot only when no artifact apply mutated the definitions while the read was in + /// flight. A stale snapshot is discarded WHOLE — its absences would spuriously tombstone + /// and stop just-applied live connections (which nothing re-creates until an instance + /// redeploy), and its rows would push superseded configs onto live connections. Discarding + /// is safe because the artifact apply already updated the overlay itself and the next + /// tick's read starts after the write, so convergence is one tick away. + /// + private void ApplyConnectionDefinitionOverlayIfFresh( + IReadOnlyList? definitions, + long generationAtRead, + string source) + { + if (generationAtRead != _definitionsWriteGeneration) + { + _logger.LogDebug( + "Discarding the {Source} data-connection definitions snapshot — an artifact " + + "apply changed the definitions while the read was in flight (generation " + + "{ReadGeneration} vs {CurrentGeneration}); the next sweep tick reads fresh", + source, generationAtRead, _definitionsWriteGeneration); + return; + } + + ApplyConnectionDefinitionOverlay(definitions); + } + /// /// Refreshes from a store read and reconciles the live /// DCL against it (deferred register rows 89 + 90). @@ -2239,6 +2289,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// connections inline" — so it is left to the flattened config. That asymmetry is the /// documented residual of rows 89/90, not an oversight. /// + /// + /// + /// Reached only through (row 101) — + /// never with a snapshot proven stale. + /// /// private void ApplyConnectionDefinitionOverlay( IReadOnlyList? definitions) @@ -3066,6 +3121,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void HandleApplyArtifactDataConnectionsToDcl(ApplyArtifactDataConnectionsToDcl msg) { + // Row 101: any snapshot read dispatched before this point carries data that predates + // these mutations — mark it stale so its overlay half is discarded when it lands. + _definitionsWriteGeneration++; + foreach (var dc in msg.DataConnections) { // Rows 89 + 90: the rows have just been committed to data_connection_definitions, so @@ -3157,10 +3216,16 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// is the deploy-only site shape (see ). /// /// + /// + /// Row 101: captured on the actor thread when the + /// read was dispatched. A mismatch on landing means an artifact apply mutated the overlay + /// while this read was in flight — the snapshot's definitions half is stale and is skipped. + /// internal record StartupConfigsLoaded( List Configs, string? Error, - IReadOnlyList? ConnectionDefinitions = null); + IReadOnlyList? ConnectionDefinitions = null, + long DefinitionsGenerationAtRead = 0); /// /// Internal singleton signal that re-attempts the startup deployed-config load @@ -3253,10 +3318,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// contract as : null = read failed, keep the overlay. /// /// + /// + /// Row 101: same stale-snapshot guard as — see there. + /// internal record InstanceSweepLoaded( List Configs, string? Error, - IReadOnlyList? ConnectionDefinitions = null); + IReadOnlyList? ConnectionDefinitions = null, + long DefinitionsGenerationAtRead = 0); internal record DeployPersistenceResult( string DeploymentId, string InstanceName, bool Success, string? Error, diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs index f3abbd10..e77b0177 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs @@ -567,6 +567,69 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable Assert.Equal(reAddedEndpoint, EndpointOf(CreatesFor(ConnectionName).Last())); } + /// + /// Deferred register row 101 — the stale-snapshot race, made deterministic. A sweep's store + /// read runs OFF the actor thread; one that starts before an artifact apply's store writes + /// can land AFTER HandleApplyArtifactDataConnectionsToDcl populated the overlay. Its + /// snapshot then lacks the just-applied rows, and the disappearance detection would tombstone + /// and STOP the just-created live connections — which nothing re-creates until an instance + /// redeploy, because the removal also evicts the hash slot the overlay's push is gated on. + /// + /// + /// The crafted InstanceSweepLoaded below IS that race: an empty snapshot stamped with + /// the pre-apply write generation, delivered after the apply. Fails before the fix: + /// without the generation guard the actor removes the connection it created a moment ago. + /// + /// + [Fact] + public async Task StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections() + { + // Sweep effectively disabled so the crafted message is the only sweep result in play. + var dm = CreateBootedDeploymentManager(sweepSeconds: 3600); + + dm.Tell(new DeployArtifactsCommand( + "artifact-race-1", null, null, null, null, + [new DataConnectionArtifact(ConnectionName, "OpcUa", EndpointJson(DefinitionEndpoint), null)], + null, DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(20)).Success); + await AwaitCreateCountAsync(1, "the artifact apply never created the connection"); + + // Generation 0 = captured before the apply bumped it; empty list = the pre-write table. + dm.Tell(new DeploymentManagerActor.InstanceSweepLoaded( + [], null, ConnectionDefinitions: [], DefinitionsGenerationAtRead: 0)); + + await Task.Delay(TimeSpan.FromSeconds(2)); + Assert.DoesNotContain(_removes, r => r.ConnectionName == ConnectionName); + } + + /// + /// The row-101 guard must not blunt row 90's legitimate disappearance detection: the same + /// empty snapshot stamped with the CURRENT write generation is a real answer (the store + /// genuinely holds no rows any more) and still stops the connection. + /// + [Fact] + public async Task FreshEmptySweepSnapshot_StillRemovesDisappearedConnections() + { + var dm = CreateBootedDeploymentManager(sweepSeconds: 3600); + + dm.Tell(new DeployArtifactsCommand( + "artifact-race-2", null, null, null, null, + [new DataConnectionArtifact(ConnectionName, "OpcUa", EndpointJson(DefinitionEndpoint), null)], + null, DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(20)).Success); + await AwaitCreateCountAsync(1, "the artifact apply never created the connection"); + + // Exactly one artifact apply has run, so the current write generation is 1. + dm.Tell(new DeploymentManagerActor.InstanceSweepLoaded( + [], null, ConnectionDefinitions: [], DefinitionsGenerationAtRead: 1)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_removes, r => r.ConnectionName == ConnectionName); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(10)); + } + /// /// Stand-in for the DCL manager actor: records the two commands the Deployment Manager is the /// sole sender of, and swallows everything else (Instance Actors send their tag subscribes