f02e116688
ReadStoreSnapshotAsync's snapshot is read off the actor thread and consumed only when its piped message lands. Row 101 guarded the DEFINITIONS half of that race; the CONFIG half was left to machinery whose reach is time-limited, and both legs of the instance reconcile sweep were reachable through it: - Row-72 leg: a read dispatched before a deploy's persist, landing after the deploy completed AND after the pipeline released ownership, carries the revision the deploy replaced. TryRestartStaleInstance then "repairs" the instance BACKWARD, re-deploying the snapshot's own superseded ConfigJson. InstanceIsOwnedByADeployPipeline covers only in-flight deploys, and the per-revision claim has never seen the old revision, so nothing stopped it. - Row-78 leg: a read dispatched before a delete/disable commits still shows an enabled row while the actor is already stopped, so StartStoredInstance resurrects it — and delete clears _sweepAttemptedRevisions, freeing the claim. Both verified real by neutralising the freshness check and re-running the new tests (the fail-before site event literally reads "running revision rev-2 is stale, applying stored revision rev-1"; the deleted instance came back). _definitionsWriteGeneration becomes _storeWriteGeneration, covering both tables (the snapshot is one consistent read and is discarded whole, so one counter is the right shape); DefinitionsGenerationAtRead -> StoreGenerationAtRead on both internal records. The generation is now bumped at BOTH ends of every mutation — when the off-thread write is dispatched and when its completion message lands — because the config paths advance actor state before the write, so an initiation-only bump would let an in-window read look fresh. Disable, delete and the init-failure rollback have no actor-thread completion message of their own, so a minimal StoreWriteCompleted self-message carries the bump for them. CDC-replicated writes, snapshot resyncs, restored files and SiteReconciliationActor's fetch deliberately do NOT bump: discovering exactly those is the sweep's reason to exist. A stale sweep result is now dropped WHOLE (no overlay apply, no start, no restart). A stale boot load cannot be dropped — startup must finish — so it re-dispatches LoadDeployedConfigs(); a fifth consecutive superseded read is applied anyway with a Warning, and a late one arriving after the boot completed is dropped rather than looping against the S5 retry timer. Tests: three fail-before regressions (backward restart, resurrection, boot re-read) plus two guard twins pinning that the legitimate row-72 restart and row-78 start still fire on a current-generation snapshot. The row-101 definitions tests keep passing; FreshEmptySweepSnapshot's hardcoded generation moves 1 -> 2 now that one artifact apply bumps at both ends. Docs: Component-SiteRuntime.md's stale-snapshot paragraph now describes the whole snapshot (both halves, both consumers, the boot re-dispatch, the CDC-no-bump rule), the components mirror follows, and the arbitration memo gains §11 extending §10 to the config half.
525 lines
33 KiB
Markdown
525 lines
33 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).
|
|
|
|
---
|
|
|
|
## 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.
|
|
|
|
## 10. Postscript — the stale-snapshot race (register row 101, 2026-08-17)
|
|
|
|
The sweep design read the store off the actor thread (§5) and compared the result against the
|
|
overlay only when the piped message landed. That gap was a TOCTOU the original memo did not name:
|
|
a read starting before an artifact apply's store writes could land AFTER
|
|
`HandleApplyArtifactDataConnectionsToDcl` populated the overlay, and its stale snapshot would then
|
|
"prove" the just-applied names disappeared. The disappearance detection (§5) tombstoned them and
|
|
stopped the just-created live connection actors — and because the removal also evicts the hash
|
|
slot the overlay's push is gated on, the next fresh sweep clears the tombstone but re-creates
|
|
nothing: **the connection stays dead until an instance redeploy**. Surfaced as a one-off test flake
|
|
(`ApplyingArtifacts_RemovedDataConnection_StopsLiveDclConnection` received a removal for `PlcA`
|
|
where `PlcGone` was expected — overlay iteration order removing BOTH names off a stale empty
|
|
snapshot), first suspected to be test-fixture-local; verification showed the mechanism is real in
|
|
production whenever an artifact deploy lands while a sweep read is in flight.
|
|
|
|
**Fix: a definitions-write generation.** `_definitionsWriteGeneration` is bumped on the actor
|
|
thread by every artifact apply; each snapshot read captures it at dispatch and a result carrying a
|
|
stale generation is discarded WHOLE — additions included, since a stale row pushed at a live
|
|
connection would reconfigure it backward and flip-flop with the next fresh read. Discarding is
|
|
safe: the apply already updated the overlay in the same pass (§5), and the next tick reads fresh.
|
|
Applies to both snapshot consumers (boot load and sweep). Deterministic reproduction + a guard
|
|
test that the legitimate row-90 disappearance still fires on a FRESH empty snapshot:
|
|
`StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections` (fails before the
|
|
fix) and `FreshEmptySweepSnapshot_StillRemovesDisappearedConnections`.
|
|
|
|
## 11. Postscript — the same race on the CONFIG half of the snapshot (register row 101 follow-on, 2026-08-18)
|
|
|
|
§10 fixed the DEFINITIONS half of `ReadStoreSnapshotAsync`'s snapshot. The **instance-config**
|
|
half of the same snapshot has the identical TOCTOU, and it was left protected only by machinery
|
|
whose reach is time-limited. This section extends §10 rather than revising it; everything §10
|
|
says about the definitions half still stands.
|
|
|
|
### 11.1 The two hazards, both verified real against the code
|
|
|
|
Both were reproduced by neutralising the freshness check and re-running the new regression tests
|
|
(see §11.5), so neither is theoretical.
|
|
|
|
**Hazard 1 — backward restart (row-72 leg).** A read dispatched before a deploy persists its new
|
|
row can land after the deploy has completed *and* after the pipeline released the instance. The
|
|
snapshot then carries the revision the deploy replaced while `_runningRevisions` carries the new
|
|
one — the exact shape `TryRestartStaleInstance` repairs, in the wrong direction. It is not a
|
|
harmless restart: the repair passes **the snapshot's own `ConfigJson`** to `HandleDeploy`, so the
|
|
superseded config is literally re-deployed. The pre-existing guards do not reach it:
|
|
`InstanceIsOwnedByADeployPipeline` covers only `_deployWarms` / `_terminatingActorsByName` /
|
|
`_inFlightDeployments`, all of which are cleared when the deploy finishes; and
|
|
`TryClaimSweepAttempt` does bound restarts to one per (instance, revision), but the *old* revision
|
|
has never been claimed, so the claim is available and the instance simply stays reverted.
|
|
Observed fail-before site event: *"Instance BackwardRestartPump restarted by deployment-manager
|
|
sweep self-heal — running revision rev-2 is stale, applying stored revision rev-1"*.
|
|
|
|
**Hazard 2 — resurrection (row-78 leg).** A read dispatched before a delete or a disable commits
|
|
still shows an enabled row while the actor is already stopped (both handlers stop the actor
|
|
synchronously and write off-thread). `StartStoredInstance` then starts an instance the operator
|
|
just removed. Delete is the worse of the two because it explicitly clears
|
|
`_sweepAttemptedRevisions` for the instance, handing the sweep a fresh repair claim. Fail-before:
|
|
the deleted instance was running again after the stale snapshot landed.
|
|
|
|
**Hazard 3 — the boot load's other consumers.** `HandleStartupConfigsLoaded` also rebuilds
|
|
`_deployedInstanceNames` from the snapshot and hands its enabled configs to the staggered startup
|
|
batch. A deploy or delete processed while the boot read is in flight therefore skews the
|
|
deployed-instance count and can hand the batch a superseded config; `CreateInstanceActor`'s
|
|
already-exists guard blunts the second effect but not the first.
|
|
|
|
### 11.2 One generation for both tables
|
|
|
|
`_definitionsWriteGeneration` is renamed `_storeWriteGeneration` and now counts mutations of BOTH
|
|
snapshot tables; the message parameter `DefinitionsGenerationAtRead` becomes
|
|
`StoreGenerationAtRead` on both internal records (neither is a wire message). One counter is the
|
|
right shape because the snapshot is a single consistent read and is discarded whole either way —
|
|
a per-table counter would only buy the ability to apply half a snapshot that the other half's
|
|
mutation already invalidated.
|
|
|
|
### 11.3 Bump at BOTH ends of every mutation
|
|
|
|
§10 bumped once, in the handler that runs after the write. That is sufficient when the actor's
|
|
own state advances in that same handler, but the config mutations do not all work that way —
|
|
`ApplyDeployment` creates the Instance Actor and stamps the running revision *before* the
|
|
persist, and `HandleDisable`/`HandleDelete` stop the actor *before* the write. So the generation
|
|
is now bumped twice per mutation: once when the off-thread write is **dispatched**, once when its
|
|
**completion** message lands. The pair leaves no window — a read dispatched before the mutation is
|
|
discarded by the first bump, a read dispatched inside the write window is discarded by the second,
|
|
and a read dispatched after the second necessarily sees the committed row. Two paths had no
|
|
actor-thread completion message at all (disable/delete pipe their reply to the *caller*, and the
|
|
init-failure rollback pipes nothing), so a minimal internal `StoreWriteCompleted(Source)` message
|
|
exists purely to carry the completion bump for them.
|
|
|
|
Bump sites: `ApplyDeployment` + `HandleDeployPersistenceResult`; `HandleEnable` +
|
|
`HandleEnableResult`; `HandleDisable` + `StoreWriteCompleted`; `HandleDelete` +
|
|
`StoreWriteCompleted`; `RollbackPersistedConfig` + `StoreWriteCompleted`; `HandleDeployArtifacts`
|
|
+ `HandleApplyArtifactDataConnectionsToDcl` (the §10 bump, kept). The deploy and enable completion
|
|
bumps are **unconditional on success** — a failed write may still have committed the row before a
|
|
later step threw, so "failed" is not "nothing changed". Cost of over-bumping is at most one
|
|
discarded sweep tick or one extra boot read.
|
|
|
|
**Deliberately NOT bumped:** `SiteReconciliationActor`'s `StoreDeployedConfigIfNewerAsync` (a
|
|
different actor), LocalDb CDC replication applies, snapshot resyncs, and an operator restoring the
|
|
file. None passes through this actor, and discovering exactly those divergences is the sweep's
|
|
entire reason to exist (rows 72 + 78). Bumping for them — were it even reachable — would blind the
|
|
sweep to its own use case.
|
|
|
|
### 11.4 Sweep drops, boot re-reads
|
|
|
|
The sweep consumer discards a stale result **whole** (definitions half and configs half, no
|
|
overlay apply, no start, no restart) at Debug, because there is nothing to complete and the next
|
|
tick is already a fresh read. The boot consumer cannot do that — a node whose startup read is
|
|
dropped stays a silent zero-instance node — so it **re-dispatches `LoadDeployedConfigs()`**, which
|
|
captures the current generation. Convergence argument: staleness requires a fresh mutation inside
|
|
each attempt's window, which is externally driven and finite. The belt-and-braces cap is
|
|
`MaxConsecutiveStaleStartupLoads` = 5, after which the snapshot is applied anyway with a Warning
|
|
(a boot that never finishes is strictly worse than one starting from a slightly superseded view
|
|
the sweep converges on its next tick). A superseded boot snapshot arriving *after*
|
|
`_startupLoadCompleted` — the S5 `RetryStartupLoad` timer racing a success — is dropped rather
|
|
than re-read, so the retry timer and the re-dispatch cannot feed each other.
|
|
|
|
### 11.5 Tests
|
|
|
|
In `InstanceSweepSelfHealTests`, all hand-delivering crafted `InstanceSweepLoaded` /
|
|
`StartupConfigsLoaded` messages with an explicit generation:
|
|
|
|
- `StaleSweepSnapshot_CarryingASupersededRevision_DoesNotRestartBackward` — fails before.
|
|
- `StaleSweepSnapshot_ShowingADeletedInstance_DoesNotResurrectIt` — fails before (full
|
|
deploy-then-delete shape, not the weaker never-started equivalent).
|
|
- `StaleBootSnapshot_IsReReadRatherThanApplied` — fails before (one read instead of two); a slow
|
|
first read plus a lifecycle command processed during it makes the race deterministic, and the
|
|
test also pins that a LATE superseded snapshot is dropped rather than looping.
|
|
- `FreshSweepSnapshot_CarryingANewerRevision_StillRestartsTheInstance` and
|
|
`FreshSweepSnapshot_ShowingAStoredEnabledInstance_StillStartsIt` — the guard twins, pinning that
|
|
legitimate row-72 and row-78 repairs survive. Both start the instance from the BOOT LOAD rather
|
|
than a deploy command, precisely so no mutation passes through the actor and generation `0` is
|
|
genuinely current.
|
|
|
|
`FreshEmptySweepSnapshot_StillRemovesDisappearedConnections` (§10) still pins the definitions
|
|
half; its hardcoded current generation moves from `1` to `2` because one artifact apply now bumps
|
|
at both ends.
|