fix(siteruntime): guard the DCL arbitration overlay against stale store snapshots — register row 101
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.
This commit is contained in:
@@ -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: <name>` 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*.
|
||||
|
||||
|
||||
@@ -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`.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -498,10 +498,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// </summary>
|
||||
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
|
||||
/// </summary>
|
||||
private readonly HashSet<string> _centrallyRemovedConnections = new(StringComparer.Ordinal);
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see
|
||||
/// cref="HandleApplyArtifactDataConnectionsToDcl"/> 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
|
||||
/// <see cref="EnsureDclConnection"/> would reconfigure a live connection BACKWARD and set
|
||||
/// up a flip-flop with the next fresh read). The next tick reads fresh and converges.
|
||||
/// </summary>
|
||||
private long _definitionsWriteGeneration;
|
||||
|
||||
/// <summary>
|
||||
/// Row 101 guard in front of <see cref="ApplyConnectionDefinitionOverlay"/>: 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.
|
||||
/// </summary>
|
||||
private void ApplyConnectionDefinitionOverlayIfFresh(
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? 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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Refreshes <see cref="_connectionDefinitions"/> 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Reached only through <see cref="ApplyConnectionDefinitionOverlayIfFresh"/> (row 101) —
|
||||
/// never with a snapshot proven stale.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private void ApplyConnectionDefinitionOverlay(
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? definitions)
|
||||
@@ -3066,6 +3121,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// </summary>
|
||||
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 <see cref="ApplyConnectionDefinitionOverlay"/>).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="DefinitionsGenerationAtRead">
|
||||
/// Row 101: <see cref="_definitionsWriteGeneration"/> 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.
|
||||
/// </param>
|
||||
internal record StartupConfigsLoaded(
|
||||
List<DeployedInstance> Configs,
|
||||
string? Error,
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null);
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null,
|
||||
long DefinitionsGenerationAtRead = 0);
|
||||
|
||||
/// <summary>
|
||||
/// Internal singleton signal that re-attempts the startup deployed-config load
|
||||
@@ -3253,10 +3318,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// contract as <see cref="StartupConfigsLoaded"/>: null = read failed, keep the overlay.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="DefinitionsGenerationAtRead">
|
||||
/// Row 101: same stale-snapshot guard as <see cref="StartupConfigsLoaded"/> — see there.
|
||||
/// </param>
|
||||
internal record InstanceSweepLoaded(
|
||||
List<DeployedInstance> Configs,
|
||||
string? Error,
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null);
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null,
|
||||
long DefinitionsGenerationAtRead = 0);
|
||||
|
||||
internal record DeployPersistenceResult(
|
||||
string DeploymentId, string InstanceName, bool Success, string? Error,
|
||||
|
||||
@@ -567,6 +567,69 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable
|
||||
Assert.Equal(reAddedEndpoint, EndpointOf(CreatesFor(ConnectionName).Last()));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>HandleApplyArtifactDataConnectionsToDcl</c> 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.
|
||||
///
|
||||
/// <para>
|
||||
/// The crafted <c>InstanceSweepLoaded</c> below IS that race: an empty snapshot stamped with
|
||||
/// the pre-apply write generation, delivered after the apply. <b>Fails before the fix</b>:
|
||||
/// without the generation guard the actor removes the connection it created a moment ago.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<ArtifactDeploymentResponse>(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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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<ArtifactDeploymentResponse>(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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
|
||||
Reference in New Issue
Block a user