Joseph Doherty
c57e74e660
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.
294 lines
18 KiB
Markdown
294 lines
18 KiB
Markdown
# 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<string>` 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).
|