From 7c1268d85cc9d49e702618b99cbdc69c0bff3add Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 06:19:13 -0400 Subject: [PATCH] =?UTF-8?q?feat(dcl+siteruntime):=20reconfigure=20a=20LIVE?= =?UTF-8?q?=20data=20connection=20in=20place=20=E2=80=94=20register=20row?= =?UTF-8?q?=2097?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DataConnectionManagerActor.HandleCreateConnection` is create-only (warns and returns for a name it already holds), so `EnsureDclConnection`'s documented "a changed config re-issues the create so the DCL adopts it" was never true on ANY node: a corrected endpoint/credential/backup/failover-count took effect only when the connection was next built from scratch (node restart, singleton failover, remove-then-re-add), while the DM's hash cache recorded it as applied. Rows 89/90 made that honest with a Warning; this closes it. New `ReconfigureConnectionCommand` (Commons, same shape as the create): - `DeploymentManagerActor.EnsureDclConnection` now has three outcomes off its one per-name hash slot — unchanged -> nothing at all, new -> create, CHANGED -> reconfigure — and logs "Reconfigured DCL connection {Connection} (protocol={Protocol}) in place" at Information in place of the old Warning. The winner-resolution / arbitration / tombstone logic is untouched; only what happens after a change is detected changed. - `DataConnectionManagerActor` FORWARDS the command to the owning connection actor, falling back to a create for a name it does not hold, so the command is safe whatever the manager's state. - `DataConnectionActor` handles it in ALL THREE states and never stashes it — a connection pointed at a wrong or dead endpoint lives in Connecting/Reconnecting, which is exactly when a correction arrives; stashed it would wait on the broken configuration forever. Handling replaces protocol/primary/backup/failover-count, re-arms endpoint selection to Primary (the new config's primary is the intent, whatever the old config had failed over to), and calls the new shared `SwapAdapter` helper — now used by both failover sites too, so the swap exists once: unhook + fire-and-forget dispose -> _factory.Create -> rewire -> bump _adapterGeneration LAST. `BecomeReconnecting(afterReconfigure: true)` then reconnects immediately (StartSingleTimer replaces any armed reconnect, so no racing attempts) with the unstable-disconnect accounting and failover check suppressed — the link was dropped on purpose, and counting it as an unstable cycle could trip a failover onto the NEW backup before the new primary had been tried once. ReSubscribeAll/ReSubscribeAllAlarms then re-issue everything from the untouched subscription registries. Idempotent: a reconfigure carrying the configuration already in force is a no-op. Stop-and-recreate was rejected as destructive: subscriptions live inside DataConnectionActor and InstanceActor cancels its subscribe-retry timer on first success, so a rebuilt actor would have no subscribers and nothing would ever re-subscribe. Tests: new DataConnectionReconfigureTests (7; 5 fail / 2 pass against a behavioural revert). DclConnectionArbitrationTests gains two row-97 tests and its CDC-arrival test now asserts the second push is a reconfigure; its ping-pong guard also asserts zero reconfigures, since a flip would now show up there. SiteRuntime-010 renamed to ..._ReissuesReconfigureCommand. 3 fail against a reverted EnsureDclConnection. DataConnectionLayer.Tests 327/327, SiteRuntime.Tests 643/643, solution build 0 warnings. Docs: Component-DataConnectionLayer.md gains an "In-place reconfiguration" section under Endpoint Redundancy; Component-SiteRuntime.md, and the docs/components/ mirrors, updated; the arbitration decision memo gains an appended §9 postscript resolving its residual 1 (residuals 2 and 3 stay open). --- docs/components/DataConnectionLayer.md | 35 +- docs/components/SiteRuntime.md | 2 + ...-17-dcl-connection-arbitration-decision.md | 96 +++++ .../Component-DataConnectionLayer.md | 19 +- docs/requirements/Component-SiteRuntime.md | 6 +- .../ReconfigureConnectionCommand.cs | 38 ++ .../Actors/DataConnectionActor.cs | 274 ++++++++++---- .../Actors/DataConnectionManagerActor.cs | 35 ++ .../Actors/DeploymentManagerActor.cs | 64 ++-- .../Actors/DataConnectionReconfigureTests.cs | 351 ++++++++++++++++++ .../Actors/DclConnectionArbitrationTests.cs | 125 ++++++- .../DeploymentManagerMediumFindingsTests.cs | 24 +- 12 files changed, 943 insertions(+), 126 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Commons/Messages/DataConnection/ReconfigureConnectionCommand.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionReconfigureTests.cs diff --git a/docs/components/DataConnectionLayer.md b/docs/components/DataConnectionLayer.md index 1fab92b0..1a69b4e0 100644 --- a/docs/components/DataConnectionLayer.md +++ b/docs/components/DataConnectionLayer.md @@ -165,7 +165,7 @@ The Site Runtime's `DeploymentManagerActor` sends `CreateConnectionCommand` to ` > **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. +> **`HandleCreateConnection` is create-only — which is why a CHANGE is a different command (register row 97)**: a create for a name it already holds warns and returns, so a re-issued create never reconfigured a live connection actor, on any node. The Deployment Manager now sends a `ReconfigureConnectionCommand` on the change edge; `HandleReconfigureConnection` **forwards** it to the owning connection actor, and falls back to a create for a name it does not hold (so the command is safe whatever the manager's state). A stop-and-recreate was rejected as destructive — subscriptions are held by `DataConnectionActor` and `InstanceActor` cancels its subscribe-retry timer on first success, so nothing would re-subscribe. ```csharp // Commons/Messages/DataConnection/CreateConnectionCommand.cs @@ -175,10 +175,39 @@ public record CreateConnectionCommand( IDictionary PrimaryConnectionDetails, IDictionary? BackupConnectionDetails = null, int FailoverRetryCount = 3); + +// Commons/Messages/DataConnection/ReconfigureConnectionCommand.cs — same shape, +// applied IN PLACE to a live connection actor. +public record ReconfigureConnectionCommand( + string ConnectionName, + string ProtocolType, + IDictionary PrimaryConnectionDetails, + IDictionary? BackupConnectionDetails = null, + int FailoverRetryCount = 3); ``` `DataConnectionManagerActor.HandleCreateConnection` calls `DataConnectionFactory.Create(ProtocolType, PrimaryConnectionDetails)` to produce the initial `IDataConnection` adapter and spawns a `DataConnectionActor` child. +### Reconfiguring a live connection + +`DataConnectionActor` handles `ReconfigureConnectionCommand` in **all three** states — `Connecting`, `Connected` and `Reconnecting` — and never stashes it (a connection pointed at a wrong or dead endpoint lives in the two non-Connected states, which is exactly when a correction arrives; stashed, it would wait on the broken configuration forever). Handling replaces protocol/primary/backup/failover-count, resets endpoint selection to Primary, and calls `SwapAdapter` — the same helper both failover sites use: + +```csharp +private void SwapAdapter(IDictionary newConfig) +{ + _adapter.Disconnected -= OnAdapterDisconnected; + _ = _adapter.DisposeAsync().AsTask(); // fire-and-forget; never awaited on the actor thread + + _adapter = _factory.Create(_protocolType, newConfig); + _connectionDetails = newConfig; + _adapter.Disconnected += OnAdapterDisconnected; + + _adapterGeneration++; // LAST — everything in flight on the old adapter is now stale +} +``` + +It then re-enters the reconnect cycle immediately (`BecomeReconnecting(afterReconfigure: true)` — zero delay, and the unstable-disconnect/failover accounting suppressed, since the link was dropped on purpose), and `ReSubscribeAll` / `ReSubscribeAllAlarms` re-issue everything from the untouched subscription registries. A reconfigure carrying the configuration already in force is a no-op. + ### Subscribing and receiving values Instance Actors send `SubscribeTagsRequest` and receive `TagValueUpdate` messages. The actor sends `TagValueUpdate` to the Instance Actor's ref for every value change notification from the adapter. On disconnect, `ConnectionQualityChanged` (with `QualityCode.Bad`) is sent to all subscribers so Instance Actors reflect staleness immediately without waiting for a per-tag callback. @@ -279,8 +308,8 @@ Carried on `CreateConnectionCommand` from the deployment artifact. ## Dependencies & Interactions -- [Site Runtime (#3)](./SiteRuntime.md) — `DeploymentManagerActor` sends `CreateConnectionCommand` / `RemoveConnectionCommand` to create and tear down connections during instance deployment. Instance Actors send `SubscribeTagsRequest` / `UnsubscribeTagsRequest` / `WriteTagRequest`. `NativeAlarmActor` (peer to `AlarmActor` under `InstanceActor`) sends `SubscribeAlarmsRequest` / `UnsubscribeAlarmsRequest` and receives `NativeAlarmTransitionUpdate` / `NativeAlarmSourceUnavailable`. -- [Commons (#16)](./Commons.md) — owns `IDataConnection`, `IAlarmSubscribableConnection`, `IBrowsableDataConnection` interfaces; `TagValue`, `QualityCode`, `NativeAlarmTransition`, `AlarmConditionState`, `AlarmTransitionKind` types; and all message contracts (`SubscribeTagsRequest/Response`, `TagValueUpdate`, `WriteTagRequest/Response`, `SubscribeAlarmsRequest/Response`, `NativeAlarmTransitionUpdate`, `NativeAlarmSourceUnavailable`, `CreateConnectionCommand`, `BrowseNodeCommand/Result`, `DataConnectionHealthReport`). +- [Site Runtime (#3)](./SiteRuntime.md) — `DeploymentManagerActor` sends `CreateConnectionCommand` / `ReconfigureConnectionCommand` / `RemoveConnectionCommand` to create, reconfigure in place, and tear down connections during instance deployment. Instance Actors send `SubscribeTagsRequest` / `UnsubscribeTagsRequest` / `WriteTagRequest`. `NativeAlarmActor` (peer to `AlarmActor` under `InstanceActor`) sends `SubscribeAlarmsRequest` / `UnsubscribeAlarmsRequest` and receives `NativeAlarmTransitionUpdate` / `NativeAlarmSourceUnavailable`. +- [Commons (#16)](./Commons.md) — owns `IDataConnection`, `IAlarmSubscribableConnection`, `IBrowsableDataConnection` interfaces; `TagValue`, `QualityCode`, `NativeAlarmTransition`, `AlarmConditionState`, `AlarmTransitionKind` types; and all message contracts (`SubscribeTagsRequest/Response`, `TagValueUpdate`, `WriteTagRequest/Response`, `SubscribeAlarmsRequest/Response`, `NativeAlarmTransitionUpdate`, `NativeAlarmSourceUnavailable`, `CreateConnectionCommand`, `ReconfigureConnectionCommand`, `BrowseNodeCommand/Result`, `DataConnectionHealthReport`). - [Health Monitoring (#11)](./HealthMonitoring.md) — `DataConnectionActor` calls `ISiteHealthCollector.UpdateConnectionHealth`, `UpdateTagResolution`, `UpdateTagQuality`, `UpdateConnectionEndpoint`, and `RemoveConnection` to keep the site health report current. `DataConnectionManagerActor` handles `GetAllHealthReports` by forwarding `GetHealthReport` to each child. - [Site Event Logging (#12)](./SiteEventLogging.md) — `DataConnectionActor` logs connection lost, restored, and failover events via `ISiteEventLogger.LogEventAsync` (fire-and-forget). Absent in tests where `ISiteEventLogger` is null. - Design spec: [Component-DataConnectionLayer.md](../requirements/Component-DataConnectionLayer.md). diff --git a/docs/components/SiteRuntime.md b/docs/components/SiteRuntime.md index dc6773ed..27b04db0 100644 --- a/docs/components/SiteRuntime.md +++ b/docs/components/SiteRuntime.md @@ -93,6 +93,8 @@ Central sends a `DeployInstanceCommand` carrying a JSON `FlattenedConfiguration` > **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`. +> **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*. + For redeployment (instance already running), the existing actor is stopped and watched: ```csharp diff --git a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md index f3079da5..460b674d 100644 --- a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md +++ b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md @@ -291,3 +291,99 @@ unchanged for a deploy-only site, and the ping-pong must never appear. **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). + +--- + +## 9. Postscript — residual 1 RESOLVED (register row 97, 2026-08-17) + +*Appended; §6 above is left exactly as it was written.* + +§6 residual 1 — "a live connection actor is still not reconfigured in place" — is closed, along +the sketch §6 recorded, in a Data Connection Layer change that leaves everything above untouched. +**The arbitration rule, the winner-resolution ordering and the tombstones are unchanged. Only what +happens AFTER a change is detected changed.** + +### 9.1 The third command + +`EnsureDclConnection` had two outcomes off its single per-name hash slot; it now has three: + +| Hash slot | Command sent | Log | +|---|---|---| +| equal | **nothing** | none — what keeps a healthy site silent every sweep tick | +| absent | `CreateConnectionCommand` | `Created DCL connection {Connection} …` (Information) | +| **different** | **`ReconfigureConnectionCommand`** | `Reconfigured DCL connection {Connection} (protocol={Protocol}) in place …` (Information) | + +The Warning §6 added to make the gap honest ("…is not reconfigured in place — it keeps serving its +current configuration until it is recreated") is **gone**, because the sentence is no longer true. + +`DataConnectionManagerActor.HandleReconfigureConnection` **forwards** the command to the connection +actor that owns the name, and **falls back to a create** for a name it does not hold — so the +command is safe whatever the manager's state, and a sender whose view of which connections exist is +stale (a fresh incarnation, a node that has just taken over) still converges in one message. +`HandleCreateConnection` itself is untouched and stays create-only. + +### 9.2 Inside the connection actor + +`DataConnectionActor` handles the command in **all three** states, and **never stashes it**. That is +load-bearing rather than tidy: a connection pointed at a wrong or dead endpoint spends its life in +`Connecting` or `Reconnecting` — exactly when a corrected configuration is sent — so a stashed +reconfigure would wait for the *broken* configuration to succeed first. + +The swap itself is the failover machinery, not a copy of it: the two failover sites and the +reconfigure now share one `SwapAdapter(newConfig)` helper (unhook + fire-and-forget dispose → +`_factory.Create` → rewire → bump `_adapterGeneration` **last**, so everything in flight on the +outgoing adapter is recognised as stale). Then `BecomeReconnecting(afterReconfigure: true)`: + +- reconnects **immediately** rather than after `ReconnectInterval` — there is nothing to wait for, + and `StartSingleTimer` replaces any reconnect already armed, so a reconfigure landing mid-backoff + cannot leave two attempts racing; +- **suppresses the unstable-disconnect accounting and the failover check** — the link was dropped on + purpose, and counting it as an unstable cycle could trip a failover onto the *new* backup before + the new primary had been tried once; +- logs the site event at Info ("Connection reconfigured — reconnecting on the new configuration") + instead of the Warning "Connection lost". + +`ReSubscribeAll` / `ReSubscribeAllAlarms` then re-issue everything from `_subscriptionsByInstance` +and `_alarmSourceSubscribers`, which the swap never touched. **Subscribers do nothing and know +nothing**: tags go bad quality across the swap and come back Good, exactly as on a failover. + +Two edge cases §6 did not name: + +1. **Endpoint selection is re-armed from the top.** Whatever endpoint the previous configuration had + failed over to, the new configuration's *primary* is the operator's intent — so `ActiveEndpoint` + and both failure counters reset. Without this, a connection sitting on its old backup would apply + a new configuration and immediately connect to the new *backup*. +2. **The command is idempotent.** A reconfigure carrying the configuration already in force is a + no-op inside the actor. The sender is hash-gated and does not send one, but an adapter swap drops + the link and re-subscribes every tag, so a duplicate must never be able to cost that. + +A **protocol change** is supported for free: `_protocolType` became mutable and the factory keys off +it. + +### 9.3 What this cost the ping-pong guard + +§3's guard test asserted "one create, zero removes, across many ticks". A flip would now show up as +a stream of **reconfigures**, not creates — each one dropping the link and re-subscribing every tag. +The guard asserts on both counts now; asserting only on creates would have left it silently +guarding the wrong thing. + +### 9.4 Tests + +`tests/…/DataConnectionLayer.Tests/Actors/DataConnectionReconfigureTests.cs` — 7 tests. Fail-before +evidence (behavioral revert: the three `ReconfigureConnectionCommand` cases removed from the state +machine and the manager's forward replaced by the create-only path — i.e. the exact pre-fix +behaviour) — **5 fail, 2 pass**: the swap-and-re-subscribe from `Connected`, the generation bump, +the honoured-not-stashed cases in `Connecting` and in `Reconnecting`, and the manager's forward all +fail; the identical-config no-op and the unknown-name fallback-to-create pass, as guards. + +`DclConnectionArbitrationTests` gained two (`ChangedConfigurationForALiveConnection_IsSentAsA +Reconfigure_NotASecondCreate`, `UnchangedConfiguration_SendsNeitherACreateNorAReconfigure`) and +`DefinitionArrivingAfterBoot_IsAdoptedByTheNextSweepTick` now asserts the second push is a +reconfigure. With `EnsureDclConnection` reverted to sending a create on the change edge, **3 fail** +across `DclConnectionArbitrationTests` + `DeploymentManagerMediumFindingsTests` (the two changed-edge +tests plus SiteRuntime-010, renamed `…_ReissuesReconfigureCommand`). + +### 9.5 Residuals after this + +§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. diff --git a/docs/requirements/Component-DataConnectionLayer.md b/docs/requirements/Component-DataConnectionLayer.md index 3566aaea..2238cacb 100644 --- a/docs/requirements/Component-DataConnectionLayer.md +++ b/docs/requirements/Component-DataConnectionLayer.md @@ -147,6 +147,20 @@ flowchart TD See [`2026-03-22-primary-backup-data-connections-design.md`](../plans/2026-03-22-primary-backup-data-connections-design.md) for the full design. +### In-place reconfiguration (register row 97) + +A `ReconfigureConnectionCommand` replaces a **live** connection's configuration without stopping the connection actor. It is the same adapter swap the failover path above performs — the two share one `SwapAdapter` helper — so the diagram applies verbatim from *dispose adapter* onwards: + +1. The command lands on the actor and **is never stashed**, in *any* of the three states. That is load-bearing, not tidiness: a connection whose configured endpoint is wrong or dead spends its life looping in `Connecting` or `Reconnecting`, which is precisely when a corrected configuration is sent. Stashed, it would wait for the *old* configuration to succeed — i.e. forever. +2. Protocol, primary/backup details and failover retry count are replaced wholesale, and **endpoint selection is re-armed from the top**: whatever endpoint the previous configuration had failed over to, the new configuration's primary is the operator's intent, so `ActiveEndpoint` and both failure counters reset. +3. `SwapAdapter` unhooks and disposes the outgoing adapter (fire-and-forget, never awaited on the actor thread), builds the replacement via `IDataConnectionFactory.Create` — which is why a **protocol change** is supported too — and bumps `_adapterGeneration` **last**, so every callback and in-flight background task belonging to the outgoing adapter is recognised as stale and dropped rather than delivered as if it came from the new endpoint. +4. The actor re-enters the reconnect cycle immediately (no `ReconnectInterval` wait — there is nothing to wait for; `StartSingleTimer` replaces any reconnect already armed, so a reconfigure landing mid-backoff cannot leave two attempts racing). The unstable-disconnect accounting and the failover check are **suppressed** for this transition: the link was dropped on purpose, and counting it as an unstable cycle could trip a failover onto the new backup before the new primary had been tried once. +5. On connect, `ReSubscribeAll` / `ReSubscribeAllAlarms` re-issue everything from `_subscriptionsByInstance` and `_alarmSourceSubscribers`, which the swap never touched. **Subscribers do nothing and know nothing** — the tags go bad quality across the swap, exactly as they do on a failover, and come back Good. + +**Accepted consequences**, all identical to a failover: in-flight writes against the outgoing adapter may fail (returned synchronously to the calling script, as always), and tag quality dips for the duration of the reconnect. + +**Idempotent.** A reconfigure carrying the configuration already in force is a no-op. The sender is hash-guarded and does not send one, but a swap is destructive enough that a duplicate must never cost a reconnect. + ## Connection Configuration Reference All settings are parsed from the data connection's configuration JSON dictionaries (`PrimaryConfiguration` and optional `BackupConfiguration`, stored as `IDictionary` connection details). Both endpoints use the same protocol-specific keys. Invalid numeric values fall back to defaults silently. @@ -359,7 +373,9 @@ Each value update delivered to an Instance Actor includes: > > **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`). +> **A live connection IS reconfigured in place, via a third command (register row 97, 2026-08-17).** `HandleCreateConnection` remains **create-only** — it warns and returns for a name it already holds — so the Deployment Manager's documented "a changed config re-issues the create so the DCL adopts it" was never true, on any node: a corrected endpoint/credential/backup/failover-count took effect only when the connection was next built from scratch (node restart, singleton failover, remove-then-re-add), while the DM's hash cache recorded it as applied. The Site Runtime now sends a **`ReconfigureConnectionCommand`** on the change edge instead. The manager **forwards** it to the connection actor that owns the name, and falls back to a create for a name it does not hold — so the command is safe whatever the manager's state and a sender never has to know whether the actor exists. +> +> **Stop-and-recreate was rejected, and the reason is worth keeping.** Subscriptions are held inside `DataConnectionActor`, and `InstanceActor` cancels its per-connection subscribe-retry timer on the first success — so a rebuilt connection actor would have *no* subscribers and nothing would ever re-subscribe. That trades "stale endpoint, data flowing" for "correct endpoint, no data until redeploy". See *In-place reconfiguration* below for what the actor does instead. 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: @@ -434,6 +450,7 @@ The DCL reports the following metrics to the Health Monitoring component via the ## Interactions +- **Site Runtime (Deployment Manager)**: Receives `CreateConnectionCommand` / `ReconfigureConnectionCommand` / `RemoveConnectionCommand`. It is the sole creator, reconfigurer and remover of connection actors; the DCL never reads storage. - **Site Runtime (Instance Actors)**: Bidirectional — delivers value updates, receives subscription registrations and write-back commands. - **Site Runtime (NativeAlarmActor)**: Bidirectional — receives alarm subscribe/unsubscribe requests, delivers native alarm transitions and source-unavailable notifications (read-only; no ack-back to the source). - **Health Monitoring**: Reports connection health periodically. diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index d0ea4804..c266f06a 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -112,7 +112,7 @@ flowchart TD > **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. +> **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 second correction was left as a residual by rows 89/90 and is closed by **row 97** — see *In-place reconfiguration* 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. > @@ -120,7 +120,9 @@ flowchart TD > > **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`. +> **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. +> +> **Residuals still open**, recorded precisely in the decision memo: 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.Commons/Messages/DataConnection/ReconfigureConnectionCommand.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/DataConnection/ReconfigureConnectionCommand.cs new file mode 100644 index 00000000..76b5a9fc --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/DataConnection/ReconfigureConnectionCommand.cs @@ -0,0 +1,38 @@ +namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; + +/// +/// Command to apply a new configuration to an already-live data connection +/// in place — sent from DeploymentManagerActor to the DCL Manager Actor whenever the +/// arbitration overlay resolves a NEW winning configuration for a connection name the DCL +/// already holds (deferred register row 97). +/// +/// +/// Why this is a separate command from . +/// DataConnectionManagerActor.HandleCreateConnection is create-only — it warns and +/// returns for a name it already holds — so a re-issued create never reached the live +/// connection actor, and a changed endpoint/credential took effect only when the connection was +/// next built from scratch (node restart, singleton failover, or a remove-then-add). Stopping +/// and re-creating the actor is NOT the fix: the subscriptions live inside +/// DataConnectionActor and InstanceActor cancels its subscribe-retry timer on the +/// first success, so nothing would re-subscribe — it would trade "stale endpoint, data flowing" +/// for "correct endpoint, no data until redeploy". Instead the connection actor swaps its +/// adapter through the same machinery the primary→backup failover path uses and re-issues every +/// subscription it already holds. +/// +/// +/// +/// Safe whatever the manager's state. A reconfigure naming a connection the manager does +/// not hold falls back to a create, so a sender never has to know whether the actor exists. +/// +/// +/// Name of the connection to reconfigure (unchanged by this command). +/// Protocol type of the NEW configuration; may differ from the current one. +/// Flattened connection details for the new primary endpoint. +/// Flattened connection details for the new backup endpoint, or null for none. +/// Consecutive failures before switching endpoints under the new configuration. +public record ReconfigureConnectionCommand( + string ConnectionName, + string ProtocolType, + IDictionary PrimaryConnectionDetails, + IDictionary? BackupConnectionDetails = null, + int FailoverRetryCount = 3); diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs index 9aa1123e..bd5b316b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs @@ -37,7 +37,12 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers private readonly DataConnectionOptions _options; private readonly ISiteHealthCollector _healthCollector; private readonly IDataConnectionFactory _factory; - private readonly string _protocolType; + /// + /// Protocol of the configuration currently in force. NOT readonly: an in-place + /// reconfiguration () may change the protocol, and + /// the factory keys off it when the replacement adapter is built. + /// + private string _protocolType; private readonly ISiteEventLogger? _siteEventLogger; /// Stash for holding messages while the connection is not in the Connected state. @@ -202,9 +207,13 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers private readonly HashSet _alarmSourcesWithoutSeparator = new(StringComparer.Ordinal); private IDictionary _connectionDetails; - private readonly IDictionary _primaryConfig; - private readonly IDictionary? _backupConfig; - private readonly int _failoverRetryCount; + // The three fields below describe the configuration currently in force. None are readonly: + // an in-place reconfiguration (ReconfigureConnectionCommand, deferred register row 97) + // replaces them wholesale and then swaps the adapter through the same machinery the + // primary→backup failover uses. + private IDictionary _primaryConfig; + private IDictionary? _backupConfig; + private int _failoverRetryCount; private ActiveEndpoint _activeEndpoint = ActiveEndpoint.Primary; private int _consecutiveFailures; private int _consecutiveUnstableDisconnects; @@ -339,6 +348,13 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers case UnsubscribeAlarmsRequest: Stash.Stash(); break; + case ReconfigureConnectionCommand rc: + // NEVER stashed. A reconfigure that arrived while the initial connect was still + // in flight would otherwise sit in the stash until the OLD configuration + // succeeded — and if it never succeeds (the very case an endpoint correction is + // sent to fix) it would sit there forever. + HandleReconfigure(rc); + break; case SubscribeCompleted sc: // A subscribe started while Connected can complete after a transition; // apply it so its state survives into the next ReSubscribeAll. @@ -454,6 +470,9 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers case AdapterDisconnected: HandleDisconnect(); break; + case ReconfigureConnectionCommand rc: + HandleReconfigure(rc); + break; case RetryTagResolution: HandleRetryTagResolution(); break; @@ -477,67 +496,65 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers // ── Reconnecting State ── - private void BecomeReconnecting() + /// + /// True when this transition was caused by an in-place reconfiguration + /// () rather than by losing the link. It suppresses the + /// unstable-disconnect accounting and the failover check — the adapter has just been + /// swapped ON PURPOSE against a brand-new configuration, so counting it as an unstable + /// cycle would let a reconfigure trip a failover onto the new backup before the new primary + /// has been tried even once — and it reconnects immediately instead of waiting out + /// , since there is nothing to wait for. + /// + private void BecomeReconnecting(bool afterReconfigure = false) { - _log.Warning("[{0}] Entering Reconnecting state", _connectionName); + _log.Warning("[{0}] Entering Reconnecting state{1}", _connectionName, + afterReconfigure ? " (reconfigured in place)" : string.Empty); _healthCollector.UpdateConnectionHealth(_connectionName, ConnectionHealth.Disconnected); _healthCollector.UpdateConnectionEndpoint(_connectionName, "Disconnected"); // Register row 54 — drops this connection out of the connected gauge while the // total holds, which is exactly the connected < total alert condition. ScadaBridgeTelemetry.RecordDataConnectionState(_connectionName, ConnectionHealth.Disconnected); - // Track unstable connections toward failover. - // If we were connected for less than the stability threshold, this counts - // as an unstable cycle (e.g., connect succeeded but heartbeat went stale). - var connectionDuration = DateTimeOffset.UtcNow - _lastConnectedAt; - if (_lastConnectedAt != default && connectionDuration < _options.StableConnectionThreshold) + if (!afterReconfigure) { - _consecutiveUnstableDisconnects++; - _log.Warning("[{0}] Unstable connection (lasted {1:F0}s) — consecutive unstable disconnects: {2}/{3}", - _connectionName, connectionDuration.TotalSeconds, _consecutiveUnstableDisconnects, - _backupConfig != null ? _failoverRetryCount : 0); - } - else - { - _consecutiveUnstableDisconnects = 0; - } - - // Failover if we keep connecting and going stale repeatedly - if (_backupConfig != null && _consecutiveUnstableDisconnects >= _failoverRetryCount) - { - var previousEndpoint = _activeEndpoint; - _activeEndpoint = _activeEndpoint == ActiveEndpoint.Primary - ? ActiveEndpoint.Backup - : ActiveEndpoint.Primary; - _consecutiveUnstableDisconnects = 0; - _consecutiveFailures = 0; - - var newConfig = _activeEndpoint == ActiveEndpoint.Primary - ? _primaryConfig - : _backupConfig; - - // Dispose old adapter - _adapter.Disconnected -= OnAdapterDisconnected; - _ = _adapter.DisposeAsync().AsTask(); - - // Create new adapter for the target endpoint - _adapter = _factory.Create(_protocolType, newConfig); - _connectionDetails = newConfig; - _adapter.Disconnected += OnAdapterDisconnected; - - // New adapter — bump the generation so callbacks - // from the disposed adapter are recognised as stale and dropped. - _adapterGeneration++; - - _log.Warning("[{0}] Failing over from {1} to {2} (unstable connection pattern)", - _connectionName, previousEndpoint, _activeEndpoint); - - if (_siteEventLogger != null) + // Track unstable connections toward failover. + // If we were connected for less than the stability threshold, this counts + // as an unstable cycle (e.g., connect succeeded but heartbeat went stale). + var connectionDuration = DateTimeOffset.UtcNow - _lastConnectedAt; + if (_lastConnectedAt != default && connectionDuration < _options.StableConnectionThreshold) { - _ = _siteEventLogger.LogEventAsync( - "connection", "Warning", null, _connectionName, - $"Failover from {previousEndpoint} to {_activeEndpoint} (unstable connection)", - $"Connection lasted {connectionDuration.TotalSeconds:F0}s, threshold {_options.StableConnectionThreshold.TotalSeconds:F0}s"); + _consecutiveUnstableDisconnects++; + _log.Warning("[{0}] Unstable connection (lasted {1:F0}s) — consecutive unstable disconnects: {2}/{3}", + _connectionName, connectionDuration.TotalSeconds, _consecutiveUnstableDisconnects, + _backupConfig != null ? _failoverRetryCount : 0); + } + else + { + _consecutiveUnstableDisconnects = 0; + } + + // Failover if we keep connecting and going stale repeatedly + if (_backupConfig != null && _consecutiveUnstableDisconnects >= _failoverRetryCount) + { + var previousEndpoint = _activeEndpoint; + _activeEndpoint = _activeEndpoint == ActiveEndpoint.Primary + ? ActiveEndpoint.Backup + : ActiveEndpoint.Primary; + _consecutiveUnstableDisconnects = 0; + _consecutiveFailures = 0; + + SwapAdapter(_activeEndpoint == ActiveEndpoint.Primary ? _primaryConfig : _backupConfig); + + _log.Warning("[{0}] Failing over from {1} to {2} (unstable connection pattern)", + _connectionName, previousEndpoint, _activeEndpoint); + + if (_siteEventLogger != null) + { + _ = _siteEventLogger.LogEventAsync( + "connection", "Warning", null, _connectionName, + $"Failover from {previousEndpoint} to {_activeEndpoint} (unstable connection)", + $"Connection lasted {connectionDuration.TotalSeconds:F0}s, threshold {_options.StableConnectionThreshold.TotalSeconds:F0}s"); + } } } @@ -545,8 +562,11 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers if (_siteEventLogger != null) { _ = _siteEventLogger.LogEventAsync( - "connection", "Warning", null, _connectionName, - $"Connection lost — entering reconnect cycle", null); + "connection", afterReconfigure ? "Info" : "Warning", null, _connectionName, + afterReconfigure + ? "Connection reconfigured — reconnecting on the new configuration" + : "Connection lost — entering reconnect cycle", + null); } Become(Reconnecting); @@ -558,8 +578,118 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers // (mark mirrored alarms uncertain; the reconnect snapshot reconciles them). PushAlarmSourceUnavailable(); - // Schedule reconnect attempt - Timers.StartSingleTimer("reconnect", new AttemptConnect(), _options.ReconnectInterval); + // Schedule reconnect attempt. StartSingleTimer REPLACES any reconnect already armed, + // so a reconfigure landing mid-backoff cannot leave two AttemptConnect messages racing. + Timers.StartSingleTimer("reconnect", new AttemptConnect(), + afterReconfigure ? TimeSpan.Zero : _options.ReconnectInterval); + } + + /// + /// Replaces the live adapter with one built for — the single + /// swap shared by both failover sites and by in-place reconfiguration + /// (). + /// + /// + /// The old adapter is unhooked first and disposed fire-and-forget (never awaited on the + /// actor thread), and is bumped LAST so every callback and + /// in-flight background task started against the outgoing adapter is recognised as stale and + /// dropped rather than delivered as if it came from the new endpoint. Subscriptions are + /// untouched: they live in and are re-issued by + /// once the replacement adapter connects. + /// + /// + private void SwapAdapter(IDictionary newConfig) + { + _adapter.Disconnected -= OnAdapterDisconnected; + _ = _adapter.DisposeAsync().AsTask(); + + _adapter = _factory.Create(_protocolType, newConfig); + _connectionDetails = newConfig; + _adapter.Disconnected += OnAdapterDisconnected; + + _adapterGeneration++; + } + + /// + /// Applies a new configuration to this LIVE connection in place (deferred register + /// row 97) — the non-destructive alternative to stopping and re-creating the connection + /// actor, which would strand every subscriber (the subscription registry lives here, and + /// InstanceActor cancels its subscribe-retry timer on the first success, so nothing + /// would ever re-subscribe). + /// + /// + /// It reuses the primary→backup failover machinery verbatim: + /// disposes the current adapter, builds one from the new configuration and bumps the + /// generation, then the reconnect cycle re-issues every subscription from + /// and every alarm feed from + /// . In-flight writes against the outgoing adapter may + /// fail — the same accepted consequence a failover has, and the failure is returned + /// synchronously to the calling script exactly as it would be there. + /// + /// + /// + /// Endpoint selection is re-armed from the top. Whatever endpoint the PREVIOUS + /// configuration had failed over to, the new configuration's primary is the operator's + /// intent, so the active endpoint and both failure counters reset. + /// + /// + /// + /// Idempotent. A reconfigure carrying the configuration already in force is a no-op. + /// The sender is hash-gated and does not send one, but a swap is destructive enough that a + /// duplicate must never cost a reconnect. + /// + /// + private void HandleReconfigure(ReconfigureConnectionCommand command) + { + if (string.Equals(_protocolType, command.ProtocolType, StringComparison.Ordinal) + && _failoverRetryCount == command.FailoverRetryCount + && DetailsEqual(_primaryConfig, command.PrimaryConnectionDetails) + && DetailsEqual(_backupConfig, command.BackupConnectionDetails)) + { + _log.Debug("[{0}] Reconfigure carries the configuration already in force — ignoring", + _connectionName); + return; + } + + _log.Warning( + "[{0}] Reconfiguring in place (protocol {1} → {2}) — swapping the adapter and " + + "re-subscribing {3} tag(s) for {4} instance(s) and {5} alarm source(s)", + _connectionName, _protocolType, command.ProtocolType, + _instancesByTag.Count, _subscriptionsByInstance.Count, _alarmSourceSubscribers.Count); + + _protocolType = command.ProtocolType; + _primaryConfig = command.PrimaryConnectionDetails; + _backupConfig = command.BackupConnectionDetails; + _failoverRetryCount = command.FailoverRetryCount; + + _activeEndpoint = ActiveEndpoint.Primary; + _consecutiveFailures = 0; + _consecutiveUnstableDisconnects = 0; + + SwapAdapter(_primaryConfig); + + BecomeReconnecting(afterReconfigure: true); + } + + /// + /// Order-insensitive value comparison of two flattened connection-detail maps, used only by + /// 's no-op guard. + /// + private static bool DetailsEqual( + IDictionary? left, IDictionary? right) + { + if (ReferenceEquals(left, right)) return true; + if (left == null || right == null) return false; + if (left.Count != right.Count) return false; + + foreach (var (key, value) in left) + { + if (!right.TryGetValue(key, out var other) + || !string.Equals(value, other, StringComparison.Ordinal)) + return false; + } + + return true; } private void Reconnecting(object message) @@ -586,6 +716,11 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers // Allow alarm unsubscribe during reconnect (cleanup on instance stop). HandleUnsubscribeAlarms(areq); break; + case ReconfigureConnectionCommand rc: + // NEVER stashed — this is the state a connection to a wrong or dead endpoint + // spends its life in, i.e. exactly when a corrected configuration arrives. + HandleReconfigure(rc); + break; case TagValueReceived: // Ignore — stale callback from previous connection break; @@ -720,24 +855,7 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers : ActiveEndpoint.Primary; _consecutiveFailures = 0; - var newConfig = _activeEndpoint == ActiveEndpoint.Primary - ? _primaryConfig - : _backupConfig; - - // Dispose old adapter (fire-and-forget — don't await in actor context) - _adapter.Disconnected -= OnAdapterDisconnected; - _ = _adapter.DisposeAsync().AsTask(); - - // Create new adapter for the target endpoint - _adapter = _factory.Create(_protocolType, newConfig); - _connectionDetails = newConfig; - - // Wire disconnect handler on new adapter - _adapter.Disconnected += OnAdapterDisconnected; - - // New adapter — bump the generation so callbacks - // from the disposed adapter are recognised as stale and dropped. - _adapterGeneration++; + SwapAdapter(_activeEndpoint == ActiveEndpoint.Primary ? _primaryConfig : _backupConfig); _log.Warning("[{0}] Failing over from {1} to {2}", _connectionName, previousEndpoint, _activeEndpoint); diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs index b1467468..f7479543 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs @@ -58,6 +58,7 @@ public class DataConnectionManagerActor : ReceiveActor _opcUaGlobalOptions = opcUaGlobalOptions ?? new OpcUaGlobalOptions(); Receive(HandleCreateConnection); + Receive(HandleReconfigureConnection); Receive(HandleRoute); Receive(HandleRoute); Receive(HandleRouteAlarms); @@ -102,6 +103,40 @@ public class DataConnectionManagerActor : ReceiveActor command.ConnectionName, command.ProtocolType); } + /// + /// Applies a new configuration to a LIVE connection actor (deferred register row 97) by + /// forwarding the command to it — the actor swaps its adapter in place and re-issues every + /// subscription it holds, rather than being stopped and rebuilt (which would silently strand + /// every subscriber; see ). + /// + /// + /// A name this manager does not hold falls back to , so + /// the command is safe regardless of manager state: a sender whose view of which connections + /// exist is stale (a fresh singleton incarnation, a node that has just taken over) still + /// converges on the intended configuration in one message. + /// + /// + private void HandleReconfigureConnection(ReconfigureConnectionCommand command) + { + if (_connectionActors.TryGetValue(command.ConnectionName, out var actor)) + { + _log.Info("Reconfiguring DataConnectionActor for {0} in place (protocol={1})", + command.ConnectionName, command.ProtocolType); + actor.Forward(command); + return; + } + + _log.Info( + "Reconfigure for {0} arrived with no live connection actor — creating it instead", + command.ConnectionName); + HandleCreateConnection(new CreateConnectionCommand( + command.ConnectionName, + command.ProtocolType, + command.PrimaryConnectionDetails, + command.BackupConnectionDetails, + command.FailoverRetryCount)); + } + private void HandleRoute(SubscribeTagsRequest request) { if (_connectionActors.TryGetValue(request.ConnectionName, out var actor)) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index f86e96d5..be83b8ca 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -2155,9 +2155,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// /// Tracks the configuration last sent to the DCL for each connection name, keyed by /// a hash of the connection's protocol/endpoints/credentials/failover count. - /// A name whose hash is unchanged is skipped; a name whose config - /// changed re-issues a CreateConnectionCommand so the DCL adopts the new - /// configuration instead of keeping a stale connection after a redeployment. + /// A name whose hash is unchanged is skipped; a name that is new gets a + /// CreateConnectionCommand, and a name whose config CHANGED gets a + /// ReconfigureConnectionCommand so the live connection actor adopts the new + /// configuration in place instead of serving a stale one until it is next rebuilt. /// private readonly Dictionary _createdConnections = new(); @@ -2365,13 +2366,29 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// Hash-guarded DCL connection push shared by the inline /// per-instance path () and the /// system-wide artifact-deploy path (). - /// Unchanged config is a no-op; a changed endpoint/credentials/backup/ - /// failover-count re-issues a CreateConnectionCommand so a system- - /// wide artifact-deploy makes its data-connection change live immediately - /// (the artifact-deploy path previously only persisted to SQLite — the - /// DCL didn't see the change until next instance redeploy or node - /// restart, contradicting the "site is self-contained after artifact - /// deployment" intent). + /// + /// + /// Three outcomes, one hash slot. An unchanged configuration is a no-op (no message, + /// no log — this is what keeps a healthy site silent across every sweep tick). A name the + /// hash cache has never held gets a CreateConnectionCommand. A name whose + /// endpoint/credentials/backup/failover-count CHANGED gets a + /// ReconfigureConnectionCommand, which the DCL applies to the live connection actor + /// in place — it swaps its adapter through the same machinery the primary→backup + /// failover uses and re-issues every subscription it holds. + /// + /// + /// + /// Why the changed case is not a re-issued create (deferred register row 97). + /// DataConnectionManagerActor.HandleCreateConnection is CREATE-ONLY — it warns and + /// returns for a name it already holds — so the re-issued create this path used to send + /// never reached the live actor at all, while the hash cache recorded it as applied. A + /// changed endpoint therefore took effect only when the connection was next built from + /// scratch (node restart, singleton failover, or a remove-then-add). Stopping and + /// re-creating the actor is not the fix either: subscriptions live inside the connection + /// actor and InstanceActor cancels its subscribe-retry timer on first success, so + /// nothing would re-subscribe. See + /// docs/plans/2026-08-17-dcl-connection-arbitration-decision.md §9. + /// /// private void EnsureDclConnection( string name, @@ -2392,32 +2409,27 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers ? null : FlattenConnectionConfig(protocol, backupConfigurationJson); - _dclManager.Tell(new Commons.Messages.DataConnection.CreateConnectionCommand( - name, protocol, primaryDetails, backupDetails, failoverRetryCount)); - 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. + _dclManager.Tell(new Commons.Messages.DataConnection.ReconfigureConnectionCommand( + name, protocol, primaryDetails, backupDetails, failoverRetryCount)); + // 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)", + // so a healthy site stays silent. + _logger.LogInformation( + "Reconfigured DCL connection {Connection} (protocol={Protocol}) in place — the " + + "live connection actor swaps its adapter onto the new configuration and " + + "re-issues its subscriptions; no redeployment or restart is needed", name, protocol); return; } + _dclManager.Tell(new Commons.Messages.DataConnection.CreateConnectionCommand( + name, protocol, primaryDetails, backupDetails, failoverRetryCount)); + _logger.LogInformation( "Created DCL connection {Connection} (protocol={Protocol})", name, protocol); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionReconfigureTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionReconfigureTests.cs new file mode 100644 index 00000000..0035bd00 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionReconfigureTests.cs @@ -0,0 +1,351 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors; +using ZB.MOM.WW.ScadaBridge.HealthMonitoring; + +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests.Actors; + +/// +/// Deferred register row 97 — in-place reconfiguration of a LIVE data connection. +/// +/// +/// The gap these pin. DataConnectionManagerActor.HandleCreateConnection is +/// create-only: it warns and returns for a name it already holds. So the Deployment Manager's +/// documented "a changed configuration re-issues the create so the DCL adopts it" was never true +/// on ANY node — a corrected endpoint or credential took effect only when the connection was next +/// built from scratch (node restart, singleton failover, or a remove-then-add), while the DM's +/// hash cache recorded it as applied. Discovered while validating rows 89/90 and recorded as +/// residual 1 of docs/plans/2026-08-17-dcl-connection-arbitration-decision.md. +/// +/// +/// +/// Why the fix is a swap and not a restart. Stopping and re-creating the connection actor +/// is destructive: the subscription registry lives inside DataConnectionActor, and +/// InstanceActor cancels its per-connection subscribe-retry timer on the first success — +/// so nothing would ever re-subscribe. That trades "stale endpoint, data flowing" for "correct +/// endpoint, no data until redeploy". instead reuses +/// the primary→backup failover machinery: dispose the adapter, build one from the new config, +/// bump the adapter generation, and let the reconnect cycle re-issue every subscription from the +/// registry that was never touched. +/// +/// +[Collection("DataConnectionManagerActor")] +public class DataConnectionReconfigureTests : TestKit +{ + private const string OldEndpoint = "opc.tcp://old-plc:4840"; + private const string NewEndpoint = "opc.tcp://new-plc:4840"; + + private readonly DataConnectionOptions _options; + private readonly ISiteHealthCollector _healthCollector; + private readonly IDataConnectionFactory _factory; + + public DataConnectionReconfigureTests() + : base(@"akka.loglevel = DEBUG") + { + _healthCollector = Substitute.For(); + _factory = Substitute.For(); + _options = new DataConnectionOptions + { + ReconnectInterval = TimeSpan.FromMilliseconds(100), + TagResolutionRetryInterval = TimeSpan.FromMilliseconds(200), + WriteTimeout = TimeSpan.FromSeconds(5), + SeedReadMaxAttempts = 1, + SeedReadRetryDelay = TimeSpan.FromMilliseconds(10) + }; + } + + private static Dictionary Config(string endpoint) => + new() { ["Endpoint"] = endpoint }; + + private IActorRef CreateConnectionActor( + string name, IDataConnection adapter, IDictionary primaryConfig) => + Sys.ActorOf(Props.Create(() => + new DataConnectionActor( + name, adapter, _options, _healthCollector, _factory, "OpcUa", + primaryConfig, null, 3)), name); + + /// An adapter that connects, subscribes and reads without incident. + private static IDataConnection HealthyAdapter(string subscriptionId) + { + var adapter = Substitute.For(); + adapter.ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.CompletedTask); + adapter.Status.Returns(ConnectionHealth.Connected); + adapter.SubscribeAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(subscriptionId); + adapter.ReadAsync(Arg.Any(), Arg.Any()) + .Returns(new ReadResult(false, null, null)); + return adapter; + } + + private static void RaiseDisconnected(IDataConnection adapter) => + adapter.Disconnected += Raise.Event(); + + private bool HasEnteredConnectedState(string connectionName) => + _healthCollector.ReceivedCalls().Any(c => + c.GetMethodInfo().Name == nameof(ISiteHealthCollector.UpdateConnectionHealth) && + c.GetArguments() is [string name, ConnectionHealth.Connected, ..] && + name == connectionName); + + private bool CreatedAdapterFor(string endpoint) => + _factory.ReceivedCalls().Any(c => + c.GetMethodInfo().Name == nameof(IDataConnectionFactory.Create) && + c.GetArguments()[1] is IDictionary d && + d.TryGetValue("Endpoint", out var e) && e == endpoint); + + private static bool Subscribed(IDataConnection adapter) => + adapter.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "SubscribeAsync"); + + private static bool Connected(IDataConnection adapter) => + adapter.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "ConnectAsync"); + + // ── The connection actor ── + + /// + /// The headline behaviour: a live, CONNECTED connection told to reconfigure builds a new + /// adapter from the new configuration and re-issues the subscription it already holds against + /// it — without the subscriber doing anything, and without the connection actor being stopped. + /// + /// + /// Fails before the fix: there was no reconfigure command at all, and the create the + /// Deployment Manager re-issued was dropped by the manager's create-only guard, so the old + /// adapter kept serving old-plc indefinitely. + /// + /// + [Fact] + public void Reconfigure_WhileConnected_SwapsTheAdapter_AndReSubscribesExistingTags() + { + const string name = "reconf-connected"; + var oldAdapter = HealthyAdapter("sub-old"); + var newAdapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(newAdapter); + + var actor = CreateConnectionActor(name, oldAdapter, Config(OldEndpoint)); + AwaitCondition(() => HasEnteredConnectedState(name), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + + actor.Tell(new SubscribeTagsRequest( + "c1", "inst1", name, ["sensor/temp"], DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(5)); + AwaitCondition(() => Subscribed(oldAdapter), TimeSpan.FromSeconds(5)); + + actor.Tell(new ReconfigureConnectionCommand( + name, "OpcUa", Config(NewEndpoint), null, 3)); + + AwaitCondition(() => CreatedAdapterFor(NewEndpoint), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + AwaitCondition(() => Connected(newAdapter), TimeSpan.FromSeconds(5)); + + // The whole point: the subscription survives the swap and is re-issued for free. + AwaitCondition(() => Subscribed(newAdapter), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + } + + /// + /// The adapter generation must be bumped by the swap, exactly as it is on failover: the + /// outgoing adapter's subscription callbacks captured Self and keep firing, and a value from + /// the endpoint the operator has just corrected AWAY from must never be delivered as if it + /// came from the new one. + /// + [Fact] + public void Reconfigure_DropsValuesStillArrivingFromTheOldAdapter() + { + const string name = "reconf-generation"; + SubscriptionCallback? oldCallback = null; + + var oldAdapter = HealthyAdapter("sub-old"); + oldAdapter.SubscribeAsync("sensor/temp", Arg.Any(), Arg.Any()) + .Returns(ci => + { + oldCallback = (SubscriptionCallback)ci[1]; + return Task.FromResult("sub-old"); + }); + + var newAdapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(newAdapter); + + var actor = CreateConnectionActor(name, oldAdapter, Config(OldEndpoint)); + AwaitCondition(() => HasEnteredConnectedState(name), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + + actor.Tell(new SubscribeTagsRequest( + "c1", TestActor.Path.Name, name, ["sensor/temp"], DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(5)); + AwaitCondition(() => oldCallback != null, TimeSpan.FromSeconds(5)); + + actor.Tell(new ReconfigureConnectionCommand( + name, "OpcUa", Config(NewEndpoint), null, 3)); + + // The swap flips every subscribed tag to bad quality, same as a disconnect. + ExpectMsg(TimeSpan.FromSeconds(5)); + + AwaitCondition(() => Subscribed(newAdapter), TimeSpan.FromSeconds(5)); + ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + + oldCallback!("sensor/temp", new TagValue(999, QualityCode.Good, DateTimeOffset.UtcNow)); + + ExpectNoMsg(TimeSpan.FromSeconds(1)); + } + + /// + /// The state that matters most, and the one a stash would break. A connection whose + /// configured endpoint is wrong or dead never reaches Connected at all — it loops in the + /// Connecting state retrying — which is EXACTLY when a corrected configuration is sent. If + /// the reconfigure were stashed with the subscribe/write requests it would wait for the old + /// (broken) configuration to succeed first, i.e. forever. + /// + [Fact] + public void Reconfigure_WhileStillConnecting_IsHonoured_NotStashed() + { + const string name = "reconf-connecting"; + var deadAdapter = Substitute.For(); + deadAdapter.ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.FromException(new Exception("Connection refused"))); + + var newAdapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(newAdapter); + + var actor = CreateConnectionActor(name, deadAdapter, Config(OldEndpoint)); + AwaitCondition(() => Connected(deadAdapter), TimeSpan.FromSeconds(5)); + Assert.False(HasEnteredConnectedState(name)); + + actor.Tell(new ReconfigureConnectionCommand( + name, "OpcUa", Config(NewEndpoint), null, 3)); + + AwaitCondition(() => CreatedAdapterFor(NewEndpoint), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + AwaitCondition(() => HasEnteredConnectedState(name), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + } + + /// + /// The same, one state over: a connection that HAD been up and is now looping in the + /// Reconnecting state. Reconnecting stashes subscribes and writes too, so the reconfigure + /// must be handled there as well — and the subscriptions it re-issues are the ones the + /// registry has been holding across the outage. + /// + [Fact] + public void Reconfigure_WhileReconnecting_IsHonoured_AndReSubscribes() + { + const string name = "reconf-reconnecting"; + var connects = 0; + var oldAdapter = HealthyAdapter("sub-old"); + oldAdapter.ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(_ => Interlocked.Increment(ref connects) == 1 + ? Task.CompletedTask + : Task.FromException(new Exception("still down"))); + + var newAdapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(newAdapter); + + var actor = CreateConnectionActor(name, oldAdapter, Config(OldEndpoint)); + AwaitCondition(() => HasEnteredConnectedState(name), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + + actor.Tell(new SubscribeTagsRequest( + "c1", "inst1", name, ["sensor/temp"], DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(5)); + + // Drop the link; with no backup configured it retries the same dead endpoint forever. + RaiseDisconnected(oldAdapter); + AwaitCondition(() => connects >= 2, TimeSpan.FromSeconds(5)); + + actor.Tell(new ReconfigureConnectionCommand( + name, "OpcUa", Config(NewEndpoint), null, 3)); + + AwaitCondition(() => CreatedAdapterFor(NewEndpoint), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + AwaitCondition(() => Subscribed(newAdapter), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + } + + /// + /// A reconfigure carrying the configuration already in force must cost nothing. The sender is + /// hash-gated and does not send one, but an adapter swap drops the link and re-subscribes + /// every tag, so a redundant command must never be able to do that. + /// + [Fact] + public void Reconfigure_WithTheConfigurationAlreadyInForce_IsANoOp() + { + const string name = "reconf-identical"; + var adapter = HealthyAdapter("sub-old"); + + var actor = CreateConnectionActor(name, adapter, Config(OldEndpoint)); + AwaitCondition(() => HasEnteredConnectedState(name), TimeSpan.FromSeconds(5), + TimeSpan.FromMilliseconds(10)); + + actor.Tell(new ReconfigureConnectionCommand( + name, "OpcUa", Config(OldEndpoint), null, 3)); + + // No adapter is ever built: the factory is only consulted by a swap. + ExpectNoMsg(TimeSpan.FromMilliseconds(500)); + Assert.False(CreatedAdapterFor(OldEndpoint)); + Assert.DoesNotContain(_factory.ReceivedCalls(), + c => c.GetMethodInfo().Name == nameof(IDataConnectionFactory.Create)); + } + + // ── The manager's routing ── + + /// + /// The manager forwards a reconfigure to the connection actor that owns the name — which is + /// the whole difference from , whose handler warns and + /// returns for a name it already holds. + /// + [Fact] + public void Manager_Reconfigure_ForALiveConnection_ReachesTheConnectionActor() + { + var oldAdapter = HealthyAdapter("sub-old"); + var newAdapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == OldEndpoint)) + .Returns(oldAdapter); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(newAdapter); + + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector))); + + manager.Tell(new CreateConnectionCommand("conn1", "OpcUa", Config(OldEndpoint), null, 3)); + AwaitCondition(() => Connected(oldAdapter), TimeSpan.FromSeconds(10)); + + manager.Tell(new ReconfigureConnectionCommand("conn1", "OpcUa", Config(NewEndpoint), null, 3)); + + AwaitCondition(() => Connected(newAdapter), TimeSpan.FromSeconds(10), + TimeSpan.FromMilliseconds(10)); + } + + /// + /// A reconfigure for a name the manager does not hold falls back to a create, so the command + /// is safe whatever the manager's state — a sender whose view of which connections exist is + /// stale still converges on the intended configuration in one message rather than leaving + /// the connection missing until something else happens to create it. + /// + [Fact] + public void Manager_Reconfigure_ForAnUnknownConnection_FallsBackToCreate() + { + var adapter = HealthyAdapter("sub-new"); + _factory.Create("OpcUa", Arg.Is>(d => d["Endpoint"] == NewEndpoint)) + .Returns(adapter); + + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector))); + + manager.Tell(new ReconfigureConnectionCommand("never-created", "OpcUa", Config(NewEndpoint), null, 3)); + + AwaitCondition(() => Connected(adapter), TimeSpan.FromSeconds(10), + TimeSpan.FromMilliseconds(10)); + + // And it is a real, routable connection afterwards — not an orphan. + manager.Tell(new SubscribeTagsRequest( + "c1", "inst1", "never-created", ["sensor/temp"], DateTimeOffset.UtcNow)); + var response = ExpectMsg(TimeSpan.FromSeconds(10)); + Assert.True(response.Success); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs index e86f857c..f3abbd10 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs @@ -16,8 +16,8 @@ using RemoveConnectionCommand = ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors 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. +/// Regression tests for deferred register rows 89, 90 and 97 — which +/// configuration source owns a live DCL connection, and how a change reaches it. /// /// /// The arbitration rule under test. The data_connection_definitions store wins @@ -39,6 +39,14 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors; /// flattened snapshots and never consults the newer definition rows. Design record: /// docs/plans/2026-08-17-dcl-connection-arbitration-decision.md. /// +/// +/// +/// Row 97 — the third command. Resolving the winner decides WHAT to push; it took a +/// to make a push against an ALREADY-LIVE connection +/// mean anything at all, because the DCL manager's create handler warns and returns for a name it +/// already holds. So a name the hash cache has never held gets a create, a changed one gets a +/// reconfigure, and an unchanged one gets nothing. +/// /// public class DclConnectionArbitrationTests : TestKit, IDisposable { @@ -56,6 +64,7 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable private readonly TestLocalDb _localDb; private readonly ConcurrentQueue _creates = new(); + private readonly ConcurrentQueue _reconfigures = new(); private readonly ConcurrentQueue _removes = new(); public DclConnectionArbitrationTests() @@ -93,7 +102,8 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable /// private IActorRef CreateBootedDeploymentManager(int sweepSeconds = SweepSeconds) { - var dclSpy = ActorOf(Props.Create(() => new DclConnectionSpy(_creates, _removes))); + var dclSpy = ActorOf(Props.Create(() => + new DclConnectionSpy(_creates, _reconfigures, _removes))); var health = new DeploymentManagerStartupProbe(); var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( @@ -150,12 +160,19 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable private IReadOnlyList CreatesFor(string connectionName) => _creates.Where(c => c.ConnectionName == connectionName).ToArray(); + private IReadOnlyList ReconfiguresFor(string connectionName) => + _reconfigures.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 static string EndpointOf(ReconfigureConnectionCommand command) => + command.PrimaryConnectionDetails["endpoint"]; + private async Task AwaitCreateCountAsync(int count, string because) => await AwaitAssertAsync(() => { @@ -163,6 +180,13 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable return Task.CompletedTask; }, TimeSpan.FromSeconds(20)); + private async Task AwaitReconfigureCountAsync(int count, string because) => + await AwaitAssertAsync(() => + { + Assert.True(ReconfiguresFor(ConnectionName).Count >= count, because); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + // ── Rule 1: a definition row outranks an older flattened snapshot ── /// @@ -224,6 +248,10 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable await Task.Delay(SeveralSweeps); Assert.Single(CreatesFor(ConnectionName)); + // Row 97 moved the cost of a flip: a ping-pong now shows up as a stream of RECONFIGURES, + // each of which drops the link and re-subscribes every tag. Assert on both, or the guard + // silently stops guarding the thing that would actually happen. + Assert.Empty(ReconfiguresFor(ConnectionName)); Assert.Empty(_removes); } @@ -283,8 +311,15 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable /// 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. + /// Fails before the fix: nothing is ever issued for the change at all, and no sweep + /// tick ever looks at the definitions table. + /// + /// + /// + /// Register row 97 changed the SHAPE of the second push. The connection already exists + /// at this point, so the change goes out as a + /// — a second create would be dropped by the DCL + /// manager's create-only guard and change nothing. /// /// [Fact] @@ -302,15 +337,87 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable // Now the definition row lands — a bare storage write, the CDC applier's only side effect. await StoreDefinitionAsync(DefinitionEndpoint); - await AwaitCreateCountAsync(2, + await AwaitReconfigureCountAsync(1, "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())); + Assert.Equal(DefinitionEndpoint, EndpointOf(ReconfiguresFor(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); + Assert.Single(CreatesFor(ConnectionName)); + Assert.Single(ReconfiguresFor(ConnectionName)); + } + + // ── Row 97: a changed configuration reaches the LIVE connection ── + + /// + /// THE row-97 reproduction on the sender side. A connection the DCL already holds gets + /// a new endpoint. What must go out is a , which the + /// live connection actor applies in place by swapping its adapter and re-issuing its + /// subscriptions. + /// + /// + /// Fails before the fix: the Deployment Manager re-issued a + /// and logged a Warning saying it would not take effect + /// — which was accurate, because DataConnectionManagerActor.HandleCreateConnection + /// warns and returns for a name it already holds. The hash cache recorded the change as + /// applied regardless, so a corrected endpoint sat unapplied until a node restart, a singleton + /// failover, or a remove-then-add. + /// + /// + [Fact] + public async Task ChangedConfigurationForALiveConnection_IsSentAsAReconfigure_NotASecondCreate() + { + const string instanceName = "ReconfiguredPump"; + CreateBootedDeploymentManager(); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-rc1", "rev-rc1", true); + await AwaitCreateCountAsync(1, "the sweep never created the connection"); + + // The connection-only artifact deploy that corrects the endpoint. + await StoreDefinitionAsync(DefinitionEndpoint); + + await AwaitReconfigureCountAsync(1, + "a changed configuration for an ALREADY-LIVE connection was not sent as a " + + "ReconfigureConnectionCommand, so the live connection actor keeps serving the old " + + "endpoint until it is rebuilt from scratch (register row 97)"); + + Assert.Equal(DefinitionEndpoint, EndpointOf(ReconfiguresFor(ConnectionName).Single())); + + // A create would have been dropped by the manager's create-only guard — so the change must + // NOT have been shipped as one. + Assert.Single(CreatesFor(ConnectionName)); + } + + /// + /// The hash guard is unchanged by row 97: an UNCHANGED configuration still sends nothing at + /// all. This is what keeps a healthy site silent across every sweep tick, and it is the reason + /// a reconfigure — which drops the link and re-subscribes every tag — is safe to send on the + /// change edge. + /// + [Fact] + public async Task UnchangedConfiguration_SendsNeitherACreateNorAReconfigure() + { + const string instanceName = "SteadyPump"; + + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-st", "rev-st", true); + + CreateBootedDeploymentManager(); + await AwaitCreateCountAsync(1, "the boot load never created the connection"); + + // Re-write the SAME definition row several sweeps running, and start a second instance + // naming the same connection: neither is a configuration change. + await StoreDefinitionAsync(DefinitionEndpoint); + await _storage.StoreDeployedConfigAsync( + "SteadyPumpTwo", MakeConfigJson("SteadyPumpTwo"), "dep-st2", "rev-st2", true); + await Task.Delay(SeveralSweeps); + + Assert.Single(CreatesFor(ConnectionName)); + Assert.Empty(ReconfiguresFor(ConnectionName)); } // ── Row 90: removal, and not being resurrected by a flattened snapshot ── @@ -469,9 +576,11 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable { public DclConnectionSpy( ConcurrentQueue creates, + ConcurrentQueue reconfigures, ConcurrentQueue removes) { Receive(creates.Enqueue); + Receive(reconfigures.Enqueue); Receive(removes.Enqueue); ReceiveAny(_ => { }); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerMediumFindingsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerMediumFindingsTests.cs index ab9adb9c..c1c132da 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerMediumFindingsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerMediumFindingsTests.cs @@ -166,12 +166,19 @@ public class DeploymentManagerMediumFindingsTests : TestKit, IDisposable /// /// SiteRuntime-010: when a redeployment changes a connection's configuration - /// (here the failover retry count and endpoint), the Deployment Manager must - /// re-issue a - /// so the DCL adopts the new configuration rather than keeping the stale one. + /// (here the failover retry count and endpoint), the Deployment Manager must push the new + /// configuration to the DCL rather than leaving it serving the stale one. + /// + /// + /// Register row 97 changed WHICH command that is. It used to be a re-issued + /// CreateConnectionCommand — which the create-only manager silently dropped for a name + /// it already held, so this test proved a message was sent and nothing more. It is now a + /// , + /// which the live connection actor actually applies in place. + /// /// [Fact] - public async Task EnsureDclConnections_ConnectionConfigChanged_ReissuesCreateCommand() + public async Task EnsureDclConnections_ConnectionConfigChanged_ReissuesReconfigureCommand() { var storage = NewStorage(_localDb.Db); await storage.InitializeAsync(); @@ -198,11 +205,12 @@ public class DeploymentManagerMediumFindingsTests : TestKit, IDisposable MakeConfigJsonWithConnection("ConnPump", "opc.tcp://host-b:4840", 7), "admin", DateTimeOffset.UtcNow)); - // The DCL must receive a fresh create command reflecting the new config. - var secondCreate = dcl.ExpectMsg( + // The DCL must receive a RECONFIGURE reflecting the new config — not a second create, + // which the create-only manager would drop for a name it already holds. + var reconfigure = dcl.ExpectMsg( TimeSpan.FromSeconds(10)); - Assert.Equal("Conn1", secondCreate.ConnectionName); - Assert.Equal(7, secondCreate.FailoverRetryCount); + Assert.Equal("Conn1", reconfigure.ConnectionName); + Assert.Equal(7, reconfigure.FailoverRetryCount); } ///