From c57e74e660a1d4e91f25ea9780e30b4906f2e1e8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:22:42 -0400 Subject: [PATCH] =?UTF-8?q?fix(siteruntime):=20arbitrate=20DCL=20connectio?= =?UTF-8?q?n=20config=20=E2=80=94=20definitions=20store=20wins=20(rows=208?= =?UTF-8?q?9=20+=2090)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes deferred-work-register rows 89 and 90, and corrects their premise twice. THE RULE The `data_connection_definitions` store wins wherever it has an opinion; an instance's flattened `Connections` entry is a deploy-time snapshot and is authoritative only for names the store has never owned (the deploy-only site, where that table is legitimately empty). PREMISE CORRECTION 1 — the peer's DCL is EMPTY, not stale `DeploymentManagerActor` is a `site-{SiteId}`-scoped singleton and the only sender of Create/RemoveConnectionCommand, to its OWN node's `dcl-manager` (which exists on every node but is populated only where the singleton runs). So the standby serves nothing. The real defect is that `_createdConnections` is in-memory per singleton incarnation: a restart or failover rebuilds every connection from the flattened snapshots and silently discards a newer connection-only artifact deploy. Row 89 is therefore create-time, not message-delivery, and NOT replication-specific — the same loss happens on an unreplicated single-node restart. PREMISE CORRECTION 2 — a changed config never reached a LIVE connection either `DataConnectionManagerActor.HandleCreateConnection` is create-only (warns and returns on a name it holds), so `EnsureDclConnection`'s documented "re-issues the create so the DCL adopts the new configuration" has never been true on any node. Now surfaced with a Warning saying exactly that, once per changed config. Not fixed here: stop-and-recreate drops every tag subscription and InstanceActor cancels its subscribe-retry timer on first success, so nothing re-subscribes. MECHANISM `data_connection_definitions` gains its first production reader. It is read on the SAME off-thread task as the deployed configs (`ReadStoreSnapshotAsync`), by the boot load and every sweep tick, into an actor-thread-confined arbitration overlay applied BEFORE the boot batch and BEFORE the sweep's repair loop. A CDC-delivered definition change converges within one tick on whichever node runs the singleton — no new cross-node message. A failed definitions read yields null and leaves the overlay untouched (distinct from an empty list, which is a real answer); clearing it would reintroduce the ping-pong through the error path. THE PING-PONG, KILLED BY CONSTRUCTION §7.4 of the sweep memo rejected a store-level pass because two sources would flip the single per-name hash slot, recreating the actor every tick. That is a property of WHERE the hash is computed: every ensure path now resolves the winner BEFORE hashing, so only one configuration can ever occupy the slot. The artifact-deploy path writes its rows into the overlay in the same pass, closing the same flip arriving through the other door. REMOVAL (row 90) Both removal paths share `RemoveConnectionCentrally` and record an in-memory tombstone, so a centrally removed connection is not resurrected by the next instance start — a pre-existing bug. Tombstones are deliberately not persisted; a definition row reappearing clears one. TESTS tests/.../Actors/DclConnectionArbitrationTests.cs — 8 tests. Fail-before (behavioral revert: overlay inert + tombstone check disabled): 5 fail, 3 pass; the 3 passers are the two deploy-only regression guards and the ping-pong guard. DOCS New decision memo docs/plans/2026-08-17-dcl-connection-arbitration-decision.md (topology, rule, ping-pong mechanism, tombstones, residuals); Component-SiteRuntime, Component-DataConnectionLayer + their docs/components mirrors; the §7.4 residual in the sweep memo marked closed with its premise corrected. --- docs/components/DataConnectionLayer.md | 6 +- docs/components/SiteRuntime.md | 4 +- ...6-08-15-instance-sweep-self-heal-design.md | 16 +- ...-17-dcl-connection-arbitration-decision.md | 293 +++++++++++ .../Component-DataConnectionLayer.md | 6 +- docs/requirements/Component-SiteRuntime.md | 14 +- .../Actors/DeploymentManagerActor.cs | 347 ++++++++++++- .../Actors/DclConnectionArbitrationTests.cs | 479 ++++++++++++++++++ 8 files changed, 1133 insertions(+), 32 deletions(-) create mode 100644 docs/plans/2026-08-17-dcl-connection-arbitration-decision.md create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs diff --git a/docs/components/DataConnectionLayer.md b/docs/components/DataConnectionLayer.md index 42397354..1fab92b0 100644 --- a/docs/components/DataConnectionLayer.md +++ b/docs/components/DataConnectionLayer.md @@ -161,7 +161,11 @@ A separate in-flight set `_subscribesInFlight` tracks the initial `SubscribeAsyn ### Creating a connection -The Site Runtime's `DeploymentManagerActor` sends `CreateConnectionCommand` to `DataConnectionManagerActor` whenever it creates an `InstanceActor` — the config comes from the `Connections` map inside that instance's flattened config, not from the site's `data_connection_definitions` table (which has no production reader). The manager never reads storage and has no independent view of which connections a site should have, so a start path that fails to push is indistinguishable here from a connection that genuinely does not exist: that was register row **85**, where two of the four Instance-Actor start paths omitted the push and every subscribe from the resulting instances was answered `Unknown connection: ` forever. The push now lives inside `CreateInstanceActor` on the Site Runtime side. Sends are hash-guarded there, so an unchanged connection produces no message at all: +The Site Runtime's `DeploymentManagerActor` sends `CreateConnectionCommand` to `DataConnectionManagerActor` whenever it creates an `InstanceActor` — the config comes from the `Connections` map inside that instance's flattened config, not from the site's `data_connection_definitions` table (which the DCL never reads — and which had no production reader at all until rows 89/90 made it the Deployment Manager's arbitration overlay; see below). The manager never reads storage and has no independent view of which connections a site should have, so a start path that fails to push is indistinguishable here from a connection that genuinely does not exist: that was register row **85**, where two of the four Instance-Actor start paths omitted the push and every subscribe from the resulting instances was answered `Unknown connection: ` forever. The push now lives inside `CreateInstanceActor` on the Site Runtime side. Sends are hash-guarded there, so an unchanged connection produces no message at all: + +> **Arbitration (register rows 89 + 90)**: the Deployment Manager *does* read `data_connection_definitions` as of 2026-08-17 — a name with a row there is configured from that row, and only a name with no row falls to the flattened snapshot. The resolution happens **before** the hash is computed, so the two sources can never occupy the same per-name hash slot alternately. Still nothing changes here: the DCL manager remains a router over actors it is told to create. See `SiteRuntime.md` and `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. +> +> **`HandleCreateConnection` is create-only, and that is a live gap**: a create for a name it already holds warns and returns, so a live connection actor never adopts a changed endpoint. The change takes effect only on the next creation from scratch (node restart, singleton failover, remove-then-re-add). A stop-and-recreate would be destructive — subscriptions are held by `DataConnectionActor` and `InstanceActor` cancels its subscribe-retry timer on first success, so nothing re-subscribes. The non-destructive fix is a `ReconfigureConnectionCommand` inside `DataConnectionActor` reusing the primary→backup failover adapter swap. ```csharp // Commons/Messages/DataConnection/CreateConnectionCommand.cs diff --git a/docs/components/SiteRuntime.md b/docs/components/SiteRuntime.md index d0a4d46e..dc6773ed 100644 --- a/docs/components/SiteRuntime.md +++ b/docs/components/SiteRuntime.md @@ -91,6 +91,8 @@ 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`. + For redeployment (instance already running), the existing actor is stopped and watched: ```csharp @@ -219,7 +221,7 @@ Both `AlarmActor` and `NativeAlarmActor` tell the `InstanceActor` an `AlarmState | `shared_scripts` | Shared script code from artifact deployments | No | | `external_systems` | External system definitions (incl. `timeout_seconds`, and `max_retries`/`retry_delay_ms` — the deployed store-and-forward retry policy, `0` = use the engine default) | No | | `database_connections` | Database connection strings | No | -| `data_connection_definitions` | OPC UA / MxGateway endpoint definitions | No | +| `data_connection_definitions` | OPC UA / MxGateway endpoint definitions — read at boot and on every sweep tick as the DCL **arbitration overlay** (rows 89/90) | No | | `notification_lists` | Notification list definitions | No | | `smtp_configurations` | SMTP configuration (from artifact deployment) | No | diff --git a/docs/plans/2026-08-15-instance-sweep-self-heal-design.md b/docs/plans/2026-08-15-instance-sweep-self-heal-design.md index 079b60f7..0afa00be 100644 --- a/docs/plans/2026-08-15-instance-sweep-self-heal-design.md +++ b/docs/plans/2026-08-15-instance-sweep-self-heal-design.md @@ -229,7 +229,8 @@ The register's suggested fix was "ensure a connection actor exists for every sto connection". Reading the code first changed the shape of the answer, because the premise is not how the runtime works: -- **`data_connection_definitions` has no production reader.** +- **`data_connection_definitions` has no production reader** *(true when this was written; rows + 89/90 gave it one on 2026-08-17 — see the residual note at the end of this section)*. `SiteStorageService.GetAllDataConnectionDefinitionsAsync` is called from exactly one place in the repo, and it is a test. Nothing loads that table at boot. - A DCL connection actor is created **only as a side effect of starting an instance**, from the @@ -311,6 +312,19 @@ sibling of row 85, it is **not** closed here, and closing it needs the arbitrati artifact-deploy reconcile; a connection deleted on the peer via replication leaves a live actor on this node. Same root cause, same arbitration question, same register entry. +> **CLOSED 2026-08-17 (rows 89 + 90) — and the residual above is worded on a wrong premise.** +> The arbitration decision was made and implemented: **the definitions store wins wherever it has +> an opinion; the flattened config is authoritative only for names the store has never owned.** The +> guarded-version-is-near-empty objection above does not apply to that rule, because the guard is +> not "skip names already in `_createdConnections`" but "resolve the WINNER before hashing", which +> leaves exactly one candidate per name and makes the flip unrepresentable rather than merely +> avoided. Two premise corrections came out of validating it: the peer's DCL is **empty**, not +> stale (only the singleton's node ever gets a `CreateConnectionCommand`), so the defect is +> create-time on the next singleton incarnation and is **not replication-specific**; and +> `HandleCreateConnection` is create-only, so a changed config never reached a *live* connection +> actor on any node — that one is still open and is this file's successor's §6.1. See +> `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. + ### 7.5 Tests `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs` — diff --git a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md new file mode 100644 index 00000000..f3079da5 --- /dev/null +++ b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md @@ -0,0 +1,293 @@ +# DCL connection arbitration — which config source owns a live connection + +**Date:** 2026-08-17 +**Scope:** Site Runtime (`DeploymentManagerActor`), deferred-work-register rows **89** and **90** +**Status:** implemented +**Predecessor:** `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7.4, which designed a +store-level connection sweep, **rejected** it, and recorded rows 89/90 as the residual — explicitly +blocked on "the arbitration decision above (which source wins), not another sweep pass". + +--- + +## 1. Verified topology — and two premise corrections + +Both register rows are worded in terms of "the peer node's DCL". That framing is wrong, and the +correction changes what the fix has to do, so it comes first. + +### 1.1 What actually runs where + +| Actor | Placement | Verified at | +|---|---|---| +| `DeploymentManagerActor` | **Cluster singleton**, role-scoped to `site-{SiteId}` (`SingletonRegistrar.Start(…, "deployment-manager", …, role: siteRole)`) | `Host/Actors/AkkaHostedService.cs` | +| `DataConnectionManagerActor` (`/user/dcl-manager`) | **Every site node**, unconditional `ActorOf` — *not* a singleton | `Host/Actors/AkkaHostedService.cs` | +| `CertStoreActor` | Every site node (documented; re-confirmed) | same file | + +`CreateConnectionCommand` and `RemoveConnectionCommand` have exactly **one** sender in the whole +repo — `DeploymentManagerActor` — and it sends them to the `_dclManager` ref it was constructed +with, which is *its own node's* local `dcl-manager` (the Props are built per node, so whichever +node materialises the singleton uses that node's manager). + +### 1.2 Premise correction 1 — the standby's DCL is EMPTY, not stale + +Because the singleton is the only sender, **the standby node's `dcl-manager` holds no connection +actors at all**. It is not "serving the old endpoint"; it is serving nothing, and nothing subscribes +through it. So there is no per-node divergence to reconcile with a cross-node message. + +The real failure mode is one layer over: + +> `_createdConnections` — the per-name hash cache that records what the DM last pushed — is +> **in-memory and per singleton incarnation**. When the singleton restarts (failover, or an +> ordinary node restart), the new incarnation starts with an empty cache, loads +> `deployed_configurations`, and rebuilds every connection **from the `Connections` snapshot frozen +> into each instance's flattened config at that instance's deploy time**. Nothing has ever read +> `data_connection_definitions` — that table had **no production reader** — so a newer, +> connection-only artifact deploy is silently discarded and the site goes back to the old endpoint. + +This also means **row 89 is not replication-specific**. Replication only puts the definition row on +both nodes, which is if anything helpful. The identical loss happens on an unreplicated single-node +restart. The row-89 fix is therefore a *create-time arbitration* fix, not a *message-delivery* fix. + +Row 90 is the same shape: the delete is durable in the store, but the flattened configs still name +the connection, so the next instance start re-creates it. + +### 1.3 Premise correction 2 — a changed config never reached a LIVE connection either + +`EnsureDclConnection`'s contract, as documented in the code before this change, was that a changed +endpoint/credential *"re-issues a `CreateConnectionCommand` so the DCL adopts the new configuration +instead of keeping a stale connection"*. **That is false.** +`DataConnectionManagerActor.HandleCreateConnection` is create-only: + +```csharp +if (_connectionActors.ContainsKey(command.ConnectionName)) +{ + _log.Warning("Connection {0} already exists", command.ConnectionName); + return; +} +``` + +So the system-wide artifact deploy's "make it live immediately" push has never worked on the +deploy-handling node, and the DM's hash cache recorded it as applied regardless. This is a +**separate, pre-existing defect** discovered while validating rows 89/90; see §6. + +--- + +## 2. The arbitration rule + +> **The definitions store wins wherever it has an opinion. The flattened config is authoritative +> only for names the store has never owned.** + +1. **A name with a row in `data_connection_definitions`** — that row's config configures the live + DCL connection actor. The identically-named entry in an instance's flattened `Connections` map + is a deploy-time snapshot and never overrides it. +2. **A name with no definition row** — the flattened config is the source, exactly as before. This + is not a fallback; it is the *correct* answer for a deploy-only site, where the definitions + table is legitimately empty because nothing has ever sent that site a system-wide artifact + deploy while every connection that matters lives inline in the instance configs. §7.4 of the + predecessor memo identified this as the reason a blanket store-level sweep is wrong, and it + remains the reason rule 2 exists. +3. **Removal** semantics on the deploy-handling node are unchanged: the actor is stopped even if + deployed instances still reference it, and their attributes go bad quality until redeploy. What + is *added* is a tombstone (§4). + +### 2.1 The overlay is an arbiter, not a creation source + +`_connectionDefinitions` decides **which configuration** a connection the runtime already wants is +built from. It never creates a connection on its own: a definition row naming a connection no +deployed instance references produces no actor, because none is wanted. This is what makes it safe +to consult on every sweep tick, and it is why re-adding a centrally deleted connection brings the +actor back at the *next instance start* rather than instantly (pinned by +`ReAddedDefinition_ClearsTheTombstone_SoTheNextInstanceStartRecreatesIt`). + +--- + +## 3. The ping-pong, and why keying the hash on the winner makes it unrepresentable + +The predecessor memo rejected a store-level sweep partly because of this: + +> `_createdConnections` holds one hash per name. The flattened-config hash and the +> artifact-definition hash for the same connection can legitimately differ (the supported +> version-skew case). A sweep re-hashing from the definitions table would flip the slot every tick +> against the deploy path flipping it back, **recreating the connection actor and dropping its live +> subscriptions on a loop.** + +That analysis is correct, and it is a property of *where the hash is computed*, not of the sweep. +The fix is one line of ordering discipline: + +> **Every ensure path resolves the winning configuration BEFORE hashing or pushing.** + +`EnsureDclConnections(configJson)` no longer hands the flattened `ConnectionConfig` straight to +`EnsureDclConnection`. It first asks the overlay whether the store owns that name; if it does, the +*definition's* protocol/config/backup/failover fields are what get hashed and pushed. The +artifact-deploy path pushes those same fields. Therefore, for any given name, **only one +configuration can ever be hashed**, no matter which path calls — the two sources cannot occupy the +slot alternately because there is only ever one candidate. The flip is not guarded against; it is +unrepresentable. + +The guard test `DisagreeingSources_DoNotPingPongTheConnection_AcrossManySweeps` pins exactly this +with the two sources deliberately disagreeing: one create, zero removes, across many ticks. It +passes before *and* after the fix — before, because nothing read the definitions at all — which is +the point: it is a guard against the **rejected naive design**, not a fail-before test. + +### 3.1 A second flip source, closed in the same way + +`HandleApplyArtifactDataConnectionsToDcl` now writes the artifact's rows into the overlay in the +same pass it pushes them. Without that there is a window between an artifact deploy and the next +sweep read in which an instance start would resolve the name to its flattened snapshot while the +artifact path had already pushed the definition — the same flip, arriving through a different door. + +--- + +## 4. Removal and the tombstone + +`_centrallyRemovedConnections` is a `HashSet` of names this singleton incarnation has seen +removed centrally. It is populated by both removal paths, which share one helper +(`RemoveConnectionCentrally`) so they cannot drift: + +- the artifact reconcile's `RemovedConnectionNames` (the existing path); and +- **the sweep's disappearance detection** (new — §5). + +`EnsureDclConnections` skips a tombstoned name. Without the tombstone the removal does not survive +one tick: the removal evicts the hash slot, and the next instance start whose flattened config still +names the connection re-creates it. That is not hypothetical — the sweep starts instances — and it +is a pre-existing bug this change also closes +(`ArtifactReconcileRemoval_IsNotResurrectedByALaterInstanceStart` fails on the old code). + +**Tombstones are deliberately in-memory only.** Persisting them was rejected: a durable "this name +is gone" marker is a second divergent authority over the same names, and the flattened configs +already resurrect a removed connection across a restart today. That pre-existing semantic is +accepted and unchanged — see the residual in §6. + +A tombstone is cleared the moment a definition row for the name reappears (a definition row *is* +the store re-asserting ownership), so a centrally re-added connection is creatable again. + +--- + +## 5. The mechanism: the definitions table as an overlay on the existing reads + +No new message, no new actor, no new cross-node path. The two durable tables the DM arbitrates over +are read in **one off-thread task**, `ReadStoreSnapshotAsync`, used by both existing reads: + +``` +PreStart → LoadDeployedConfigs ─┐ + ├─ ReadStoreSnapshotAsync (off-thread, one task) +InstanceSweepTick ──────────────┘ deployed_configurations + data_connection_definitions + │ + ▼ piped back + StartupConfigsLoaded / InstanceSweepLoaded (actor thread) + │ + ApplyConnectionDefinitionOverlay + │ + ┌───────────────────┴───────────────────┐ + row present row disappeared + adopt + push if live tombstone + stop actor +``` + +Nothing blocks the actor thread on SQLite; the overlay mutation, the hash cache and the tombstone +set are all actor-thread-confined, matching the existing `StartupConfigsLoaded` / +`ApplyArtifactDataConnectionsToDcl` patterns. + +**Ordering is load-bearing in both handlers.** The overlay is applied *before* the boot batch +creates any Instance Actor and *before* the sweep's repair loop, so a connection is created from the +winning config on its **first** push rather than created stale and corrected a tick later. On +failover that is the difference between a clean takeover and a needless recreate on every one. + +### 5.1 The two reads fail differently, on purpose + +A failed **config** read propagates and re-arms the whole load (the existing S5 path, unchanged). A +failed **definitions** read is swallowed into a `null` result, and `null` means *keep the overlay +exactly as it is* — deliberately distinct from an empty list, which is a real answer (a deploy-only +site, or a site after central deletes every data connection). Clearing the overlay on a transient +read failure would hand arbitration back to the flattened snapshots for one pass and take it away +on the next successful read: the §3 ping-pong, reintroduced through the error path. + +### 5.2 Disappearance detection, and precisely how far it reaches + +A name that **was** in the overlay and now has no row is a removal this incarnation can *prove*: the +store owned it a moment ago and does not now. It is tombstoned and its connection actor stopped. + +A name that was **never** in the overlay is indistinguishable from one the store has never owned — +a fresh incarnation cannot tell "central deleted this" from "this site keeps its connections +inline". It is left to the flattened config. That asymmetry is deliberate; the alternative (infer +deletion from "the definitions table is non-empty, therefore it is the complete set") would stop +live connections on an inference, and is recorded as a candidate follow-up in §6. + +### 5.3 Cost + +One extra `SELECT` over `data_connection_definitions` per sweep tick (default 60 s) and once at +boot, on the same task as the existing deployed-configs read. Steady state is otherwise silent: the +hash guard means an unchanged connection produces no message and no log. + +--- + +## 6. Residuals — what this does NOT close + +These are precise, and each is a candidate register row. + +1. **A live connection actor is still not reconfigured in place** (premise correction 1.3). The DCL + manager's `HandleCreateConnection` is create-only, so a changed configuration for a name whose + actor already exists takes effect only when the connection is next created from scratch — a node + restart, a singleton failover, or the connection being removed and re-added. This change makes + that **honest** rather than silent: `EnsureDclConnection` now logs a Warning naming exactly that, + once per changed configuration (the hash guard suppresses repeats, so a healthy site stays + silent). It is not *fixed* here because the obvious fix is destructive: stopping and re-creating + the connection actor drops every Instance Actor's tag subscription, and `InstanceActor` cancels + its per-connection subscribe-retry timer on the first success, so **nothing re-subscribes** — + trading "stale endpoint, data flowing" for "correct endpoint, no data until redeploy". The + non-destructive fix is a `ReconfigureConnectionCommand` handled *inside* `DataConnectionActor`, + reusing the adapter-swap machinery the primary→backup failover path already has + (`CountFailureAndMaybeFailover`: dispose adapter, `_factory.Create` the new one, bump + `_adapterGeneration`, re-enter the connect cycle, which re-issues everything from + `_subscriptionsByInstance`). That is a Data Connection Layer change and is out of rows 89/90's + scope. +2. **A removal is still not durable across a restart.** Tombstones are in-memory, so a connection + deleted centrally and then resurrected by an instance's flattened config on the next node restart + behaves as it always has. Closing this needs a durable marker or — better — flattened configs + that stop carrying connection bodies at all. +3. **A delete that arrives while this node has never adopted the row is undetectable** (§5.2). The + narrow case that remains open: the singleton moves to a node whose file already has the row + deleted, and some instance's flattened config still names the connection. That node re-creates + it. A stronger rule — "a non-empty definitions table is the complete set, so absence means + deleted" — would close it, at the cost of stopping live connections on an inference about what + central shipped; deliberately not taken. +4. **Dual-active (a real partition under `auto-down`)** is the only routine way a definition change + reaches a singleton that did not process the deploy itself. It is handled correctly by §5, but it + is worth naming that this is not the common path — the common path is failover/restart (§1.2). + +--- + +## 7. Tests + +`tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs` — 8 tests, +asserting on the `CreateConnectionCommand` / `RemoveConnectionCommand` messages the DM sends to a +recording stand-in for the DCL manager, and on the **endpoint inside** those commands (which is what +distinguishes the two config sources). + +Every test gates on `DeploymentManagerStartupProbe` for the same reason the row-78/85 suites do: a +row written before the boot load reads storage is consumed by the boot load, and an ungated test +would silently prove the boot load works instead of the path it names. + +Fail-before evidence (behavioral revert: `ApplyConnectionDefinitionOverlay` made inert and the +tombstone check disabled — i.e. the exact pre-fix arbitration) — **5 fail, 3 pass**: + +| Test | Pre-fix | +|---|---| +| `DefinitionRow_WinsOverAnOlderFlattenedSnapshot_WhenTheSingletonBoots` | **FAIL** — the row-89 symptom in its failover shape (create carries the old endpoint) | +| `DefinitionArrivingAfterBoot_IsAdoptedByTheNextSweepTick` | **FAIL** — the CDC-delivered change is never read | +| `DefinitionRowDisappearing_StopsTheConnection_AndItIsNotResurrected` | **FAIL** — the row-90 symptom (orphan actor stays live) | +| `ArtifactReconcileRemoval_IsNotResurrectedByALaterInstanceStart` | **FAIL** — removal undone by the next instance start | +| `ReAddedDefinition_ClearsTheTombstone_SoTheNextInstanceStartRecreatesIt` | **FAIL** | +| `DisagreeingSources_DoNotPingPongTheConnection_AcrossManySweeps` | pass (guard against the rejected naive design) | +| `NoDefinitionRow_LeavesTheFlattenedConfigAuthoritative_AtBoot` | pass (deploy-only regression guard) | +| `NoDefinitionRow_LeavesTheFlattenedConfigAuthoritative_OnTheSweepPath` | pass (deploy-only regression guard) | + +The three passers are the point of passing both before and after: rule 2 must be byte-for-byte +unchanged for a deploy-only site, and the ping-pong must never appear. + +--- + +## 8. Configuration + +**None added.** The overlay refresh rides `ScadaBridge:SiteRuntime:InstanceSweepIntervalSeconds` +(default 60; `0` disables the sweep and therefore the post-boot half of this convergence — the +boot-time arbitration still applies, because it hangs off the startup load, not the sweep timer). diff --git a/docs/requirements/Component-DataConnectionLayer.md b/docs/requirements/Component-DataConnectionLayer.md index 0b0c85c3..3566aaea 100644 --- a/docs/requirements/Component-DataConnectionLayer.md +++ b/docs/requirements/Component-DataConnectionLayer.md @@ -355,9 +355,11 @@ Each value update delivered to an Instance Actor includes: ## Connection Actor Model -> **Where connection actors come from — the Site Runtime is the only creator (register row 85)**: the DCL manager creates and removes connection actors **only** when the Deployment Manager tells it to, via `CreateConnectionCommand` / `RemoveConnectionCommand`. It never reads storage; the site's `data_connection_definitions` table has no production reader at all, and the DCL has no independent notion of "the connections this site ought to have". A connection actor's config therefore comes from the `Connections` map inside a deployed instance's **flattened config** (pushed as part of starting that instance) or from the system-wide artifact-deploy path. `HandleCreateConnection` is idempotent on the name — it warns and returns if it already holds that connection — but the Deployment Manager is the side that keeps the traffic quiet, hash-guarding on the last config pushed per name so an unchanged connection sends nothing. Row 85 was the consequence of that ownership split being incompletely honoured on the Site Runtime side: two of the four Instance-Actor start paths omitted the connection push, so instances came up whose every tag subscribe was answered `Unknown connection: ` here, forever. The fix is in the Site Runtime (the ensure moved into its single creation funnel) — **not** here; giving the DCL manager its own config source would create a second, divergent authority with no arbiter when the two disagree. See `Component-SiteRuntime.md` and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. +> **Where connection actors come from — the Site Runtime is the only creator (register row 85)**: the DCL manager creates and removes connection actors **only** when the Deployment Manager tells it to, via `CreateConnectionCommand` / `RemoveConnectionCommand`. It never reads storage, and the DCL has no independent notion of "the connections this site ought to have" — the site's `data_connection_definitions` table is read by the **Deployment Manager**, never here (see the arbitration note below; it had no reader at all until register rows 89/90). A connection actor's config therefore comes from the `Connections` map inside a deployed instance's **flattened config** (pushed as part of starting that instance) or from the system-wide artifact-deploy path. `HandleCreateConnection` is idempotent on the name — it warns and returns if it already holds that connection — but the Deployment Manager is the side that keeps the traffic quiet, hash-guarding on the last config pushed per name so an unchanged connection sends nothing. Row 85 was the consequence of that ownership split being incompletely honoured on the Site Runtime side: two of the four Instance-Actor start paths omitted the connection push, so instances came up whose every tag subscribe was answered `Unknown connection: ` here, forever. The fix is in the Site Runtime (the ensure moved into its single creation funnel) — **not** here; giving the DCL manager its own config source would create a second, divergent authority with no arbiter when the two disagree. See `Component-SiteRuntime.md` and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. > -> Two known residual gaps of the same family, **not** closed by row 85's fix: `data_connection_definitions` is one of LocalDb's ten replicated tables, so a **connection-only** artifact deploy replicates to the peer node — but only the node that received the deploy pushes it to its DCL, so the peer keeps serving the old endpoint until an instance is redeployed or the node restarts. The removal side is the mirror image: a connection deleted on the peer leaves a live connection actor on this node. +> **The two residual gaps of that family are now closed — but not where the register said they were (rows 89 + 90, 2026-08-17)**: the peer node's DCL was never "serving the old endpoint". Only the node running the `site-{SiteId}` Deployment Manager singleton ever receives a `CreateConnectionCommand`, so the standby's manager is **empty**. The divergence surfaces when the singleton's *next* incarnation (failover, or an ordinary restart — no replication required) rebuilds every connection from the `Connections` snapshots frozen into the instances' flattened configs, because the per-name hash cache that recorded the artifact deploy's change is in-memory and dies with the incarnation. The fix lives entirely in the Site Runtime, as an **arbitration rule** — the definitions store wins wherever it has an opinion, resolved *before* the hash is computed so the two sources cannot fight over the same per-name slot — plus in-memory tombstones so a centrally removed connection is not resurrected by a flattened config. Nothing changes here. See `Component-SiteRuntime.md` → *DCL connection arbitration* and `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. +> +> **What DOES remain open here, and it is this component's:** `HandleCreateConnection` is **create-only**, so a re-issued create for a name it already holds is a no-op — a live connection actor never adopts a changed endpoint/credential/backup/failover-count, on any node. The Deployment Manager's documented "a changed config re-issues the create so the DCL adopts it" was therefore never true; it now logs a Warning saying so, once per changed configuration, and the change takes effect only when the connection is next created from scratch (node restart, singleton failover, or remove-then-re-add). The naive fix — stop the actor and re-create it — is destructive: every Instance Actor's tag subscriptions are lost and `InstanceActor` cancels its per-connection subscribe-retry timer on the first success, so **nothing re-subscribes**. The non-destructive fix is a `ReconfigureConnectionCommand` handled *inside* `DataConnectionActor`, reusing the adapter-swap machinery the primary→backup failover path already has (dispose adapter → `_factory.Create` the new one → bump `_adapterGeneration` → re-enter the connect cycle, which re-issues everything from `_subscriptionsByInstance`). Each data connection is managed by a dedicated connection actor that uses the Akka.NET **Become/Stash** pattern to model its lifecycle as a state machine: diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 6f1b88e6..d0ea4804 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -108,7 +108,19 @@ flowchart TD > > Guards, all shared with the row-67 handler: a **disabled** instance is never started; the sweep **defers to any deploy pipeline that owns the instance** (compile warm in flight, predecessor still terminating, deployment awaiting its persistence/init join) so it can never interleave with a real redeploy; it is **inert until boot-time Instance Actor creation has finished**, so a tick between staggered batches does not "heal" instances the boot load is about to create; it makes **one repair attempt per (instance, revision) pair**, which is what stops a periodic self-heal becoming a restart loop for an actor that dies during init or a stored config the compile gate rejects (a rejection leaves the running instance untouched — the gate returns before anything is stopped); and reads never overlap. A sweep that finds nothing is **completely silent**. A repair emits the same Warning `deployment` site-event pattern as row 67, naming the trigger (`deployment-manager sweep self-heal`) so it is distinguishable from a startup-reconcile heal, and a stale-revision restart names **both** revisions; a rejected restart adds an Error event. The synthetic redeploy replies to the singleton itself rather than to `NoSender`, because dead letters are a monitored site health metric. -> **Starting an Instance Actor also creates its DCL connections — the ensure lives in the creation funnel, not at call sites (register row 85)**: a DCL connection actor is **never** created from the `data_connection_definitions` table (nothing in production reads it). It exists only as a side effect of starting an instance, built from the `Connections` map inside that instance's **own flattened config** via `EnsureDclConnections` — plus the separate system-wide artifact-deploy path. That ensure used to be each *caller's* responsibility, and only two of the four callers made it (the boot-time staggered batch and `ApplyDeployment`). The two that did not were **`StartStoredInstance`** — the path shared by the row-67 ensure-started handler and the row-78 sweep — and **`HandleEnableResult`**. Both therefore produced a running instance whose every tag subscribe was answered `Unknown connection: ` by the DCL manager, indefinitely, because nothing else on the node would ever create that connection actor. Proven live 2026-08-16: a replication-seeded node's instances were correctly started by the sweep and then failed every subscribe for 6+ minutes until a second pair restart let the boot load create the connections. **It was never replication-specific** — the boot-time load reads only *enabled* configs, so a node that boots with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. `EnsureDclConnections` now sits at the top of **`CreateInstanceActor`**, already the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), so all four paths get it and a fifth cannot forget. It is placed *before* the already-exists guard so the two paths that already called it keep their exact previous semantics, and it stays silent in steady state because `EnsureDclConnection` is hash-guarded on the last config pushed per connection name. Deliberately **not** done: a sweep pass over `data_connection_definitions` — it would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. See `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. +> **Starting an Instance Actor also creates its DCL connections — the ensure lives in the creation funnel, not at call sites (register row 85)**: a DCL connection actor is **never** created from the `data_connection_definitions` table — that stayed true when rows 89/90 gave the table a reader, because the overlay it feeds is an *arbiter*, not a creation source, and at the time of row 85 nothing read the table at all. A connection actor exists only as a side effect of starting an instance, built from the `Connections` map inside that instance's **own flattened config** via `EnsureDclConnections` — plus the separate system-wide artifact-deploy path. That ensure used to be each *caller's* responsibility, and only two of the four callers made it (the boot-time staggered batch and `ApplyDeployment`). The two that did not were **`StartStoredInstance`** — the path shared by the row-67 ensure-started handler and the row-78 sweep — and **`HandleEnableResult`**. Both therefore produced a running instance whose every tag subscribe was answered `Unknown connection: ` by the DCL manager, indefinitely, because nothing else on the node would ever create that connection actor. Proven live 2026-08-16: a replication-seeded node's instances were correctly started by the sweep and then failed every subscribe for 6+ minutes until a second pair restart let the boot load create the connections. **It was never replication-specific** — the boot-time load reads only *enabled* configs, so a node that boots with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. `EnsureDclConnections` now sits at the top of **`CreateInstanceActor`**, already the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), so all four paths get it and a fifth cannot forget. It is placed *before* the already-exists guard so the two paths that already called it keep their exact previous semantics, and it stays silent in steady state because `EnsureDclConnection` is hash-guarded on the last config pushed per connection name. Deliberately **not** done at the time: a sweep pass over `data_connection_definitions` — it would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. See `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. That pass is now present, in a form that resolves the conflict rather than creating it — see **DCL connection arbitration** immediately below. + +> **DCL connection arbitration — the definitions store wins wherever it has an opinion (register rows 89 + 90)**: `data_connection_definitions` now has a production reader. The rule is: **a connection name with a row in that table is configured from that row; a name with no row is configured from the instance's flattened `Connections` entry, exactly as before.** Rule 2 is not a fallback — it is the correct answer for a deploy-only site, where the table is legitimately empty while every connection that matters lives inline in the instance configs. +> +> **The register's premise needed two corrections, and they change what the fix is.** First, the peer node's DCL is not "serving the old endpoint" — it is **empty**. The Deployment Manager is a `site-{SiteId}`-scoped singleton and the *only* sender of `CreateConnectionCommand`/`RemoveConnectionCommand`, sending to its own node's `dcl-manager` (which exists on every node but is populated only where the singleton runs). The real defect is that `_createdConnections`, the per-name hash cache, is **in-memory and per singleton incarnation**: a restart or failover rebuilds every connection from the `Connections` snapshot frozen into each instance's flattened config at *that instance's* deploy time, and a newer connection-only artifact deploy is silently discarded. **That makes row 89 create-time, not message-delivery, and not replication-specific** — the identical loss happens on an unreplicated single-node restart. Second, `EnsureDclConnection`'s documented "a changed config re-issues the create so the DCL adopts it" was **false**: `DataConnectionManagerActor.HandleCreateConnection` is create-only (warns and returns for a name it already holds), so the artifact deploy's live push has never worked on the deploy-handling node either. That is now surfaced honestly rather than silently — see the residual below. +> +> **Mechanism.** `data_connection_definitions` is read in the *same* off-thread task as `deployed_configurations` (`ReadStoreSnapshotAsync`), by both the boot load and every sweep tick, and applied to an actor-thread-confined **arbitration overlay** *before* the boot batch creates any Instance Actor and *before* the sweep's repair loop — so a connection is built from the winning config on its **first** push rather than created stale and corrected a tick later. The overlay is an **arbiter, not a creation source**: a definition row naming a connection no deployed instance references creates nothing. A CDC-delivered definition change therefore converges within one sweep tick on whichever node runs the singleton, with **no new cross-node message**. A definitions read that *fails* yields `null` and leaves the overlay untouched — deliberately distinct from an empty list, which is a real answer; clearing on a transient failure would reintroduce the ping-pong through the error path. +> +> **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. +> +> **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. +> +> **Residuals**, all recorded precisely in the decision memo: a **live** connection actor is still not reconfigured in place (the non-destructive fix is a `ReconfigureConnectionCommand` inside `DataConnectionActor` reusing the failover adapter-swap, because stopping and re-creating the actor drops every tag subscription and `InstanceActor` cancels its subscribe-retry timer on first success, so nothing re-subscribes) — this is now announced with a Warning naming exactly that, once per changed configuration; a removal is not durable across a restart; and a delete is undetectable on a node that never adopted the row. See `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. > **Per-instance compilation during staggered startup — CLOSED (WP3.1)**: this was a deferred optimization (P6) affecting failover time-to-recover; each Instance Actor Roslyn-compiled its own scripts inside its own start, serialising a site's whole recovery behind compilation. Each staggered startup batch now runs a **pre-warm step**: before the batch's Instance Actors are created, the Deployment Manager compiles that batch's distinct script bodies and trigger expressions on a background task and pipes `BatchCompileWarmed` back, so every `PreStart` compile is a cache hit. Instance Actor start still calls the compilation service — it remains the correctness backstop — but in every warmed path those calls are memoised lookups. Cost: one extra message per batch. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 09b295d6..f86e96d5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -82,6 +82,17 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private readonly Func>> _configLoader; /// + /// Reads the durable data_connection_definitions rows (deferred register rows 89 + 90). + /// Defaults to ; + /// overridable via the ctor so a test can drive a read failure. + /// + /// This table had NO production reader before rows 89/90 — it was written by the artifact + /// deploy and never read back, which is precisely why a definition change survived only as + /// long as the singleton incarnation that applied it. + /// + /// + private readonly Func>> _connectionDefinitionLoader; + /// /// Set once the startup deployed-config load has succeeded. Guards against a /// stale re-running the load after success. /// @@ -246,6 +257,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// towards on every instance- /// count change; null uses the process-wide shared scheduler. /// + /// + /// Rows 89 + 90: optional override for reading the durable + /// data_connection_definitions rows that arbitrate the live DCL configuration; + /// defaults to reading from . Primarily for tests that need to + /// drive a read failure. + /// public DeploymentManagerActor( SiteStorageService storage, ScriptCompilationService compilationService, @@ -261,7 +278,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers TimeSpan? startupLoadRetryInterval = null, Func>>? configLoader = null, ExternalSystemDefinitionCache? externalSystemCache = null, - ScriptExecutionScheduler? scriptScheduler = null) + ScriptExecutionScheduler? scriptScheduler = null, + Func>>? connectionDefinitionLoader = null) { _scriptScheduler = scriptScheduler; _storage = storage; @@ -280,6 +298,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers ? TimeSpan.FromSeconds(options.InstanceSweepIntervalSeconds) : TimeSpan.Zero; _configLoader = configLoader ?? (() => _storage.GetAllDeployedConfigsAsync()); + _connectionDefinitionLoader = connectionDefinitionLoader + ?? (() => _storage.GetAllDataConnectionDefinitionsAsync()); _logger = logger; // Reuse a single logger factory for all Instance Actors. // Prefer an explicitly injected factory, fall back to one resolved from @@ -478,14 +498,50 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void LoadDeployedConfigs() { - _configLoader().ContinueWith(t => + ReadStoreSnapshotAsync().ContinueWith(t => { if (t.IsCompletedSuccessfully) - return new StartupConfigsLoaded(t.Result, null); + return new StartupConfigsLoaded(t.Result.Configs, null, t.Result.Definitions); return new StartupConfigsLoaded([], t.Exception?.GetBaseException().Message); }).PipeTo(Self); } + /// + /// Reads the two durable tables the Deployment Manager arbitrates over — the deployed + /// instance configs and the data_connection_definitions rows — in ONE off-thread + /// task, so the boot load and every sweep tick see a single consistent snapshot and the + /// actor thread is never blocked on SQLite. + /// + /// + /// The two reads fail differently on purpose. A failed CONFIG read propagates: it is + /// the existing S5 path and re-arms the whole load at a fixed interval. A failed DEFINITIONS + /// read is swallowed into a result, because the DCL arbitration overlay + /// must be left EXACTLY as it was rather than cleared — an overlay cleared by a transient + /// read failure would hand arbitration back to the flattened snapshots for one pass and then + /// take it away again on the next successful read, which is the recreate-on-every-flip + /// ping-pong the whole design exists to make impossible. + /// + /// + private async Task<(List Configs, IReadOnlyList? Definitions)> + ReadStoreSnapshotAsync() + { + var configs = await _configLoader().ConfigureAwait(false); + + IReadOnlyList? definitions = null; + try + { + definitions = await _connectionDefinitionLoader().ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.LogWarning( + ex, "Could not read the data connection definitions — the DCL arbitration " + + "overlay keeps its previous contents until the next read succeeds"); + } + + return (configs, definitions); + } + /// protected override void PostStop() { @@ -544,6 +600,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Mark the startup load complete so a stale retry timer is ignored. _startupLoadCompleted = true; + // Rows 89 + 90: seed the DCL arbitration overlay BEFORE any Instance Actor is created, + // so the boot batch's EnsureDclConnections already resolves to the definition-owned + // config. This is the failover half of the row-89 fix: a singleton incarnation starting + // 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); + var enabledConfigs = msg.Configs.Where(c => c.IsEnabled).ToList(); _deployedInstanceNames.Clear(); foreach (var c in msg.Configs) @@ -1563,8 +1627,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _instanceSweepInFlight = true; - _configLoader().ContinueWith(t => t.IsCompletedSuccessfully - ? new InstanceSweepLoaded(t.Result, null) + ReadStoreSnapshotAsync().ContinueWith(t => t.IsCompletedSuccessfully + ? new InstanceSweepLoaded(t.Result.Configs, null, t.Result.Definitions) : new InstanceSweepLoaded([], t.Exception?.GetBaseException().Message)) .PipeTo(Self); } @@ -1589,6 +1653,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers return; } + // Rows 89 + 90: refresh the DCL arbitration overlay FIRST, so any instance this same + // sweep pass goes on to start resolves its connections against the current definitions + // rather than against the snapshot frozen into its flattened config. This is the + // 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); + foreach (var config in msg.Configs) { var instanceName = config.InstanceUniqueName; @@ -2090,10 +2162,156 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers private readonly Dictionary _createdConnections = new(); /// - /// Sets up DCL connections from the flattened config. Idempotent on unchanged - /// configuration, but re-issues the create command when a connection's endpoint, - /// credentials, backup endpoint, or failover retry count has changed since it was - /// last sent. + /// The DCL connection arbitration overlay (deferred register rows 89 + 90) — the + /// data_connection_definitions rows this singleton incarnation currently believes the + /// site store owns, keyed by connection name. + /// + /// + /// The rule: the definitions store wins wherever it has an opinion. A connection name + /// with a row here is configured from that row; the identically-named entry inside an + /// instance's flattened config is a deploy-time SNAPSHOT and never overrides it. A name with + /// no row here falls through to the flattened config exactly as before — that is not a + /// fallback, it is the correct answer for a deploy-only site, where this table is legitimately + /// EMPTY because it is written only by the system-wide artifact deploy while the connections + /// that matter live inline in the instance configs. + /// + /// + /// + /// This overlay is an arbiter, not a creation source. A definition row for a name no + /// deployed instance references creates nothing: there is no connection actor wanted for it. + /// The overlay only decides WHICH configuration a connection the runtime already wants is + /// created from — which is what makes it safe to consult on every sweep tick. + /// + /// + /// + /// Refreshed by from the boot load and from + /// every sweep tick, and kept consistent in-flight by + /// . Actor-thread-confined. + /// + /// + private readonly Dictionary _connectionDefinitions = + new(StringComparer.Ordinal); + + /// + /// Connection names this singleton incarnation has seen REMOVED centrally (deferred register + /// row 90) — either by an artifact reconcile it processed itself, or by a definition row that + /// disappeared from the store between two sweep reads. + /// + /// + /// Without it the removal does not stick for as long as one tick: the removal evicts the name + /// from , and the very next instance start whose flattened + /// config still names that connection re-creates it — the flattened snapshot resurrecting a + /// connection central has deleted. That is not hypothetical; the sweep starts instances. + /// + /// + /// + /// Deliberately in-memory only. Persisting tombstones was rejected: a durable + /// "this connection is gone" marker is a second, divergent authority over the same names, and + /// the flattened configs already resurrect a removed connection across a restart today — that + /// pre-existing semantic is accepted and unchanged, and is recorded as the residual in + /// docs/plans/2026-08-17-dcl-connection-arbitration-decision.md. Cleared the moment a + /// definition row for the name reappears, so a centrally re-added connection comes straight + /// back. + /// + /// + private readonly HashSet _centrallyRemovedConnections = new(StringComparer.Ordinal); + + /// + /// Refreshes from a store read and reconciles the live + /// DCL against it (deferred register rows 89 + 90). + /// + /// + /// is when the read FAILED, which is + /// deliberately distinct from an empty list: a failed read leaves the overlay untouched, + /// while an empty list is a real answer (the deploy-only site shape, and also what a site + /// looks like after central deletes every data connection). + /// + /// + /// + /// Disappearance detection, and exactly how far it reaches. A name that WAS in this + /// overlay and no longer has a row is a removal this incarnation can prove: the store owned + /// it a moment ago and does not now. It is tombstoned and its live connection actor stopped. + /// A name that was never in the overlay is indistinguishable from one the store has never + /// owned — a fresh incarnation cannot tell "central deleted this" from "this site keeps its + /// connections inline" — so it is left to the flattened config. That asymmetry is the + /// documented residual of rows 89/90, not an oversight. + /// + /// + private void ApplyConnectionDefinitionOverlay( + IReadOnlyList? definitions) + { + if (definitions == null) + return; + + var disappeared = _connectionDefinitions.Keys + .Where(name => !definitions.Any(d => string.Equals(d.Name, name, StringComparison.Ordinal))) + .ToList(); + + foreach (var definition in definitions) + { + // A definition row is the store re-asserting ownership: any tombstone for the name + // is stale, so a centrally re-added connection is creatable again immediately. + _centrallyRemovedConnections.Remove(definition.Name); + _connectionDefinitions[definition.Name] = definition; + + // Push only to connections the runtime already holds. A definition nothing + // references stays inert — see the overlay's "arbiter, not a creation source" note. + if (_createdConnections.ContainsKey(definition.Name)) + { + EnsureDclConnection( + definition.Name, + definition.Protocol, + definition.ConfigurationJson, + definition.BackupConfigurationJson, + definition.FailoverRetryCount); + } + } + + foreach (var name in disappeared) + RemoveConnectionCentrally(name, "its definition row is no longer in the site store"); + } + + /// + /// Applies a central removal of a data connection: tombstones the name for this singleton + /// incarnation, drops the overlay row and the hash-cache slot, and stops the live DCL + /// connection actor. Shared by the artifact-reconcile removal and the sweep's + /// disappearance detection so both produce identical state. + /// + /// + /// A connection still referenced by a deployed instance's config no longer exists centrally + /// either — its attributes go bad quality, the standard disconnected signal, until the + /// instance is redeployed against current central config. That is the pre-existing removal + /// semantic and it is unchanged here; what IS new is that the removal now survives the next + /// instance start instead of being undone by it. + /// + /// + private void RemoveConnectionCentrally(string name, string because) + { + _connectionDefinitions.Remove(name); + _centrallyRemovedConnections.Add(name); + _createdConnections.Remove(name); + _dclManager?.Tell(new DataConnectionLayer.Actors.RemoveConnectionCommand(name)); + _logger.LogWarning( + "Data connection '{Name}' removed centrally ({Because}); its DCL connection actor " + + "has been stopped and will not be recreated from an instance's flattened config", + name, because); + } + + /// + /// Sets up DCL connections from the flattened config, arbitrated against + /// (deferred register rows 89 + 90). Idempotent on + /// unchanged configuration. + /// + /// + /// Why the arbitration has to happen HERE, before the hash is computed. + /// holds one hash slot per connection name. The flattened + /// snapshot and the definition row for the same name can legitimately differ — that is the + /// supported version-skew case, an artifact deploy newer than the instance deploys that + /// reference it. Two callers hashing from two different sources would flip that one slot back + /// and forth, and every flip is a fresh CreateConnectionCommand. Resolving the WINNER + /// first means only one configuration can ever be hashed for a name, whichever path calls, so + /// the flip is not merely avoided by convention — it is unrepresentable. + /// /// private void EnsureDclConnections(string configJson) { @@ -2106,6 +2324,29 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers foreach (var (name, connConfig) in config.Connections) { + // Row 90: never resurrect a name the artifact reconcile removed. + if (_centrallyRemovedConnections.Contains(name)) + { + _logger.LogDebug( + "Skipping DCL connection '{Connection}' named by a flattened config — it " + + "was removed centrally in this incarnation", name); + continue; + } + + if (_connectionDefinitions.TryGetValue(name, out var owned)) + { + // Rule 1: the store has an opinion, so the store wins. + EnsureDclConnection( + name, + owned.Protocol, + owned.ConfigurationJson, + owned.BackupConfigurationJson, + owned.FailoverRetryCount); + continue; + } + + // Rule 2: no definition row has ever owned this name — the flattened config is + // the source, exactly as before. This is the whole of a deploy-only site. EnsureDclConnection( name, connConfig.Protocol, @@ -2156,9 +2397,29 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers var changed = _createdConnections.ContainsKey(name); _createdConnections[name] = configHash; + + if (changed) + { + // Be honest about what a re-issued create does. DataConnectionManagerActor's + // HandleCreateConnection is CREATE-ONLY: it warns and returns for a name it already + // holds, so a live connection actor does NOT adopt the new configuration here. The + // change becomes effective the next time the connection is created from scratch — + // the connection is removed and re-added, or this node restarts / the singleton fails + // over, at which point the arbitration overlay hands out the current configuration. + // Emitted once per changed configuration (the hash guard above suppresses repeats), + // so a healthy site stays silent. See the "live reconfiguration" residual in + // docs/plans/2026-08-17-dcl-connection-arbitration-decision.md. + _logger.LogWarning( + "DCL connection {Connection} (protocol={Protocol}) has a new configuration, but a " + + "live connection actor is not reconfigured in place — it keeps serving its " + + "current configuration until it is recreated (node restart, singleton failover, " + + "or the connection being removed and re-added)", + name, protocol); + return; + } + _logger.LogInformation( - "{Action} DCL connection {Connection} (protocol={Protocol})", - changed ? "Updated" : "Created", name, protocol); + "Created DCL connection {Connection} (protocol={Protocol})", name, protocol); } /// @@ -2793,6 +3054,21 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { foreach (var dc in msg.DataConnections) { + // Rows 89 + 90: the rows have just been committed to data_connection_definitions, so + // update the arbitration overlay in the same pass rather than waiting for the next + // sweep read. Skipping this would leave a window in which an instance start resolves + // the name to its flattened snapshot while the artifact path has already pushed the + // definition — the exact two-sources-one-hash-slot flip the overlay exists to prevent. + _centrallyRemovedConnections.Remove(dc.Name); + _connectionDefinitions[dc.Name] = new StoredDataConnectionDefinition + { + Name = dc.Name, + Protocol = dc.Protocol, + ConfigurationJson = dc.PrimaryConfigurationJson, + BackupConfigurationJson = dc.BackupConfigurationJson, + FailoverRetryCount = dc.FailoverRetryCount + }; + EnsureDclConnection( dc.Name, dc.Protocol, @@ -2801,20 +3077,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers dc.FailoverRetryCount); } - // Reconciled removals: the definition row is already gone from SQLite; evict - // the hash cache (so a later re-add with the same name recreates cleanly) and - // stop the live DCL connection actor. A connection still referenced by a - // deployed instance's config no longer exists centrally either — its attributes - // go bad quality, the standard disconnected signal, until the instance is - // redeployed against current central config. + // Reconciled removals: the definition row is already gone from SQLite; drop the overlay + // row, tombstone the name for this incarnation (row 90 — otherwise the next instance + // start recreates it from that instance's flattened config), evict the hash cache so a + // later re-add recreates cleanly, and stop the live DCL connection actor. foreach (var name in msg.RemovedConnectionNames) - { - _createdConnections.Remove(name); - _dclManager?.Tell(new DataConnectionLayer.Actors.RemoveConnectionCommand(name)); - _logger.LogWarning( - "Artifact reconcile removed data connection '{Name}' (deleted centrally); " + - "its DCL connection actor has been stopped", name); - } + RemoveConnectionCentrally(name, "deleted centrally, reconciled by an artifact deploy"); } /// @@ -2861,7 +3129,24 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// internal sealed record SiteNodeJoined(Address Address); - internal record StartupConfigsLoaded(List Configs, string? Error); + /// + /// Carries the boot-time store read back onto the actor thread. + /// + /// + /// (deferred register rows 89 + 90) is the + /// data_connection_definitions snapshot read in the SAME off-thread task, so the DCL + /// arbitration overlay is populated BEFORE the boot batch creates any Instance Actor — a + /// connection whose definition row is newer than the flattened snapshot inside the instance + /// config is therefore created from the definition on the very first push, not created stale + /// and corrected a sweep later. means the definitions read FAILED and + /// the previous overlay must be kept; an EMPTY list means the table is genuinely empty, which + /// is the deploy-only site shape (see ). + /// + /// + internal record StartupConfigsLoaded( + List Configs, + string? Error, + IReadOnlyList? ConnectionDefinitions = null); /// /// Internal singleton signal that re-attempts the startup deployed-config load @@ -2946,8 +3231,18 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// /// Carries one sweep's off-thread read of the durable deployed-config rows back onto the /// actor thread, where every comparison and repair happens. + /// + /// + /// (rows 89 + 90) rides the same read so the DCL + /// arbitration overlay is refreshed on every tick — that is what makes a definition row + /// delivered by LocalDb CDC converge without any new cross-node message. Same null-vs-empty + /// contract as : null = read failed, keep the overlay. + /// /// - internal record InstanceSweepLoaded(List Configs, string? Error); + internal record InstanceSweepLoaded( + List Configs, + string? Error, + IReadOnlyList? ConnectionDefinitions = null); 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 new file mode 100644 index 00000000..e86f857c --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs @@ -0,0 +1,479 @@ +using System.Collections.Concurrent; +using System.Text.Json; +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +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; +using RemoveConnectionCommand = ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors.RemoveConnectionCommand; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors; + +/// +/// Regression tests for deferred register rows 89 and 90 — which configuration +/// source owns a live DCL connection. +/// +/// +/// The arbitration rule under test. The data_connection_definitions store wins +/// wherever it has an opinion; an instance's flattened Connections entry is a deploy-time +/// SNAPSHOT and is authoritative only for names the store has never owned (the deploy-only site, +/// where that table is legitimately empty). Every ensure path resolves the winner BEFORE hashing, +/// so the single per-name hash slot in _createdConnections can only ever hold one source's +/// configuration — which is what makes the two-source recreate-on-every-flip ping-pong +/// unrepresentable rather than merely avoided. +/// +/// +/// +/// Verified topology (this is what the register rows really describe). The Deployment +/// Manager is a cluster singleton scoped to site-{SiteId} and is the ONLY sender of +/// / ; the +/// dcl-manager actor exists on every site node but only the singleton's node ever +/// populates one. So the standby's DCL is not "serving the old endpoint" — it is empty, and the +/// divergence surfaces when the singleton's NEXT incarnation rebuilds every connection from the +/// flattened snapshots and never consults the newer definition rows. Design record: +/// docs/plans/2026-08-17-dcl-connection-arbitration-decision.md. +/// +/// +public class DclConnectionArbitrationTests : TestKit, IDisposable +{ + private const string ConnectionName = "OPC PLC Simulator"; + + /// The endpoint frozen into an instance's flattened config at its deploy time. + private const string FlattenedEndpoint = "opc.tcp://old-plc:4840"; + + /// The endpoint a later, connection-only artifact deploy wrote to the store. + private const string DefinitionEndpoint = "opc.tcp://new-plc:4840"; + + private readonly SiteStorageService _storage; + private readonly ScriptCompilationService _compilationService; + private readonly SharedScriptLibrary _sharedScriptLibrary; + private readonly TestLocalDb _localDb; + + private readonly ConcurrentQueue _creates = new(); + private readonly ConcurrentQueue _removes = new(); + + public DclConnectionArbitrationTests() + { + _localDb = TestLocalDb.CreateTemp("dcl-arbitration-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); + } + + /// Shortest sweep the (whole-second) option allows. + private const int SweepSeconds = 1; + + /// Long enough for at least three sweep ticks to have come and gone. + private static readonly TimeSpan SeveralSweeps = TimeSpan.FromSeconds(4); + + /// + /// Starts a Deployment Manager wired to a recording stand-in for the DCL manager, and blocks + /// until its boot-time store read has PROVABLY completed (see + /// ). Rows written before that point are consumed + /// by the boot load, which is a different path from the post-boot sweep — the gate is what + /// keeps each test proving the path it names. + /// + private IActorRef CreateBootedDeploymentManager(int sweepSeconds = SweepSeconds) + { + var dclSpy = ActorOf(Props.Create(() => new DclConnectionSpy(_creates, _removes))); + var health = new DeploymentManagerStartupProbe(); + + var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions { InstanceSweepIntervalSeconds = sweepSeconds }, + NullLogger.Instance, + dclManager: dclSpy, + healthCollector: health, + serviceProvider: null, + loggerFactory: null, + configFetcher: null))); + + AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), + TimeSpan.FromMilliseconds(25)); + return dm; + } + + /// + /// A flattened config carrying one data connection. The Connections map is the + /// deploy-time snapshot the arbitration rule demotes whenever a definition row exists. + /// + private static string MakeConfigJson(string instanceName, string endpoint = FlattenedEndpoint) => + JsonSerializer.Serialize(new FlattenedConfiguration + { + InstanceUniqueName = instanceName, + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "TestAttr", Value = "42", DataType = "Int32" + } + ], + Connections = new Dictionary + { + [ConnectionName] = new() + { + Protocol = "OpcUa", + ConfigurationJson = EndpointJson(endpoint), + FailoverRetryCount = 3 + } + } + }); + + private static string EndpointJson(string endpoint) => $"{{\"endpointUrl\":\"{endpoint}\"}}"; + + /// Writes the data_connection_definitions row an artifact deploy would. + private Task StoreDefinitionAsync(string endpoint) => + _storage.StoreDataConnectionDefinitionAsync( + ConnectionName, "OpcUa", EndpointJson(endpoint), null, 3); + + private IReadOnlyList CreatesFor(string connectionName) => + _creates.Where(c => c.ConnectionName == connectionName).ToArray(); + + /// + /// The endpoint the DCL was actually told to connect to — the whole point of the arbitration. + /// + private static string EndpointOf(CreateConnectionCommand command) => + command.PrimaryConnectionDetails["endpoint"]; + + private async Task AwaitCreateCountAsync(int count, string because) => + await AwaitAssertAsync(() => + { + Assert.True(CreatesFor(ConnectionName).Count >= count, because); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + // ── Rule 1: a definition row outranks an older flattened snapshot ── + + /// + /// THE row-89 reproduction, in its real (failover / restart) shape. The store holds a + /// definition row pointing at the new endpoint — put there by a connection-only artifact + /// deploy and, on a replicated pair, delivered to this node by LocalDb CDC — while the + /// instance's own flattened config still carries the endpoint frozen in at ITS deploy time. + /// A singleton incarnation booting here rebuilds the entire DCL from those flattened + /// snapshots, so this is exactly what the peer node does the moment it takes over. + /// + /// + /// Fails before the fix: the create carries old-plc, because nothing in the + /// runtime ever read data_connection_definitions — the table had no production reader + /// at all, so the artifact deploy's change survived only inside the singleton incarnation + /// that applied it. + /// + /// + [Fact] + public async Task DefinitionRow_WinsOverAnOlderFlattenedSnapshot_WhenTheSingletonBoots() + { + const string instanceName = "ArbitratedPump"; + + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-a", "rev-a", true); + + CreateBootedDeploymentManager(sweepSeconds: 0); + + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + + Assert.Equal(DefinitionEndpoint, EndpointOf(CreatesFor(ConnectionName).Single())); + } + + /// + /// The ping-pong guard. With the two sources disagreeing, the naive form of this fix — + /// a sweep pass that re-hashes the connection from the definitions table while the deploy + /// paths re-hash it from the flattened configs — flips the single per-name hash slot on every + /// tick, and each flip is another , i.e. the live + /// connection actor recreated and every subscription on it dropped, forever. + /// + /// + /// This passes both before and after the fix (before, because nothing read the definitions at + /// all) and is the reason the ensure paths resolve the WINNER before hashing rather than + /// guarding the flip after the fact. + /// + /// + [Fact] + public async Task DisagreeingSources_DoNotPingPongTheConnection_AcrossManySweeps() + { + const string instanceName = "NoPingPongPump"; + + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-p", "rev-p", true); + + CreateBootedDeploymentManager(); + + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + await Task.Delay(SeveralSweeps); + + Assert.Single(CreatesFor(ConnectionName)); + Assert.Empty(_removes); + } + + // ── Rule 2: no definition row means the flattened config is the source, unchanged ── + + /// + /// The deploy-only site: data_connection_definitions is EMPTY because nothing has ever + /// sent this site a system-wide artifact deploy, and every connection that matters lives + /// inline in the instance configs. Rule 2 says the flattened config is the source there — not + /// as a fallback, but as the correct answer — so this must behave exactly as it did before. + /// + /// Passes before and after: a deploy-only regression guard. + /// + [Fact] + public async Task NoDefinitionRow_LeavesTheFlattenedConfigAuthoritative_AtBoot() + { + const string instanceName = "DeployOnlyPump"; + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-d", "rev-d", true); + + CreateBootedDeploymentManager(sweepSeconds: 0); + + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + + Assert.Equal(FlattenedEndpoint, EndpointOf(CreatesFor(ConnectionName).Single())); + } + + /// + /// The same rule on the POST-boot sweep path (register row 78's start, row 85's connection): + /// a config that appears in the store on its own starts an instance whose connection, with no + /// definition row anywhere, still comes from that instance's flattened config. + /// + /// Passes before and after: a deploy-only regression guard for the sweep path. + /// + [Fact] + public async Task NoDefinitionRow_LeavesTheFlattenedConfigAuthoritative_OnTheSweepPath() + { + const string instanceName = "DeployOnlySweptPump"; + CreateBootedDeploymentManager(); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-ds", "rev-ds", true); + + await AwaitCreateCountAsync(1, "the sweep never created the connection"); + + Assert.Equal(FlattenedEndpoint, EndpointOf(CreatesFor(ConnectionName).Single())); + } + + // ── Row 89's live half: a definition arriving after boot converges on the next tick ── + + /// + /// A definition row appearing in the store while the node runs — what LocalDb's CDC + /// applier does on the peer of a replicated pair when a connection-only artifact deploy lands + /// on the other node. There is no cross-node message for this and none is needed: the sweep + /// re-reads the table on the next tick, adopts the row into the arbitration overlay, and + /// pushes the new configuration for the connection the runtime already holds. + /// + /// + /// Fails before the fix: exactly one create is ever issued, carrying old-plc, + /// and no sweep tick ever looks at the definitions table. + /// + /// + [Fact] + public async Task DefinitionArrivingAfterBoot_IsAdoptedByTheNextSweepTick() + { + const string instanceName = "CdcDeliveredPump"; + CreateBootedDeploymentManager(); + + // The instance starts first, from its own (older) flattened snapshot. + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-c", "rev-c", true); + await AwaitCreateCountAsync(1, "the sweep never created the connection"); + Assert.Equal(FlattenedEndpoint, EndpointOf(CreatesFor(ConnectionName).First())); + + // Now the definition row lands — a bare storage write, the CDC applier's only side effect. + await StoreDefinitionAsync(DefinitionEndpoint); + + await AwaitCreateCountAsync(2, + "a definition row that arrived by replication was never picked up: the live DCL kept " + + "the endpoint frozen into an instance's flattened config (register row 89)"); + + Assert.Equal(DefinitionEndpoint, EndpointOf(CreatesFor(ConnectionName).Last())); + + // And it settles: the overlay now owns the name, so further ticks are silent. + await Task.Delay(SeveralSweeps); + Assert.Equal(2, CreatesFor(ConnectionName).Count); + } + + // ── Row 90: removal, and not being resurrected by a flattened snapshot ── + + /// + /// THE row-90 reproduction. A definition row this incarnation had adopted disappears + /// from the store — a central delete, reconciled by an artifact deploy on the other node and + /// replicated here as a row deletion. The sweep can PROVE that removal (the store owned the + /// name a tick ago and does not now), so it stops the live connection actor; and the name is + /// tombstoned, so the next instance start whose flattened config still lists that connection + /// does not quietly bring it back. + /// + /// + /// Fails before the fix at the removal assert: nothing ever noticed, and the orphan + /// connection actor stayed live until the node restarted. + /// + /// + [Fact] + public async Task DefinitionRowDisappearing_StopsTheConnection_AndItIsNotResurrected() + { + const string firstInstance = "RemovedConnPump"; + const string secondInstance = "LaterPump"; + + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + firstInstance, MakeConfigJson(firstInstance), "dep-r1", "rev-r1", true); + + CreateBootedDeploymentManager(); + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + + // Central deleted the connection; the reconcile (or CDC) removes the row. + await _storage.DeleteDataConnectionDefinitionsExceptAsync([]); + + await AwaitAssertAsync(() => + { + Assert.True( + _removes.Any(r => r.ConnectionName == ConnectionName), + "the definition row vanished and the live DCL connection actor was left running " + + "(register row 90)"); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + var createsAfterRemoval = CreatesFor(ConnectionName).Count; + + // A second instance whose flattened config still names the removed connection is started + // by the sweep. It must NOT resurrect a connection central has deleted. + await _storage.StoreDeployedConfigAsync( + secondInstance, MakeConfigJson(secondInstance), "dep-r2", "rev-r2", true); + await Task.Delay(SeveralSweeps); + + Assert.Equal(createsAfterRemoval, CreatesFor(ConnectionName).Count); + } + + /// + /// The same tombstone, reached through the path that actually removes connections today: an + /// artifact deploy whose complete data-connection set no longer contains the name, which the + /// site reconciles by deleting the row and stopping the actor. + /// + /// + /// Fails before the fix at the final assert: the removal evicted the hash slot but + /// left no memory of WHY, so the very next instance start re-created the connection from that + /// instance's flattened config — a centrally-deleted connection resurrected within one sweep + /// tick. + /// + /// + [Fact] + public async Task ArtifactReconcileRemoval_IsNotResurrectedByALaterInstanceStart() + { + const string instanceName = "PostReconcilePump"; + var dm = CreateBootedDeploymentManager(); + + // Artifact deploy 1 — the connection exists centrally. + dm.Tell(new DeployArtifactsCommand( + "artifact-1", null, null, null, null, + [new DataConnectionArtifact(ConnectionName, "OpcUa", EndpointJson(DefinitionEndpoint), null)], + null, DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(20)).Success); + + // Artifact deploy 2 — central no longer has it, so the site reconciles it away. + dm.Tell(new DeployArtifactsCommand( + "artifact-2", null, null, null, null, [], null, DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(20)).Success); + + await AwaitAssertAsync(() => + { + Assert.True( + _removes.Any(r => r.ConnectionName == ConnectionName), + "the artifact reconcile did not stop the removed connection"); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + var createsAfterRemoval = CreatesFor(ConnectionName).Count; + + // An instance start naming the removed connection must not undo the removal. + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-pr", "rev-pr", true); + await Task.Delay(SeveralSweeps); + + Assert.Equal(createsAfterRemoval, CreatesFor(ConnectionName).Count); + } + + /// + /// The tombstone must not be permanent: a definition row IS the store re-asserting ownership + /// of the name, so a connection central deletes and then re-adds is creatable again. + /// + /// + /// Note what the re-add does and does not do. The overlay is an arbiter, not a creation + /// source — clearing the tombstone does not by itself resurrect a connection actor, exactly as + /// a definition row for a name no instance references creates nothing. The connection comes + /// back with the next instance start, and it comes back at the RE-ADDED endpoint, which is + /// what proves both halves: the tombstone was cleared, and rule 1 still governs the winner. + /// This mirrors the removal's own long-standing semantic — a removed connection's attributes + /// stay bad quality until the referencing instance is redeployed. + /// + /// + [Fact] + public async Task ReAddedDefinition_ClearsTheTombstone_SoTheNextInstanceStartRecreatesIt() + { + const string firstInstance = "ReAddedPump"; + const string laterInstance = "ReAddedLaterPump"; + + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + firstInstance, MakeConfigJson(firstInstance), "dep-ra", "rev-ra", true); + + CreateBootedDeploymentManager(); + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + + await _storage.DeleteDataConnectionDefinitionsExceptAsync([]); + await AwaitAssertAsync(() => + { + Assert.True(_removes.Any(r => r.ConnectionName == ConnectionName), + "the removal never happened"); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + // Central re-adds it, at a third endpoint. + const string reAddedEndpoint = "opc.tcp://re-added-plc:4840"; + await StoreDefinitionAsync(reAddedEndpoint); + + // A later instance start is what actually wants a connection actor again. + await _storage.StoreDeployedConfigAsync( + laterInstance, MakeConfigJson(laterInstance), "dep-ral", "rev-ral", true); + + await AwaitCreateCountAsync(2, + "the tombstone outlived the re-add, so a connection central had restored stayed dead"); + Assert.Equal(reAddedEndpoint, EndpointOf(CreatesFor(ConnectionName).Last())); + } + + /// + /// 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 + /// here too, and those are not what these tests assert on). + /// + private sealed class DclConnectionSpy : ReceiveActor + { + public DclConnectionSpy( + ConcurrentQueue creates, + ConcurrentQueue removes) + { + Receive(creates.Enqueue); + Receive(removes.Enqueue); + ReceiveAny(_ => { }); + } + } +}