dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

merge(agent-a): rows 85+83 — EnsureDclConnections moved into the CreateInstanceActor funnel (self-healed/enabled instances get their DCL connections); ReconcileStartsInstanceTests gated on first instance-count report

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 03:52:35 -04:00
parent 92b587d2a1 524bdbbe90
commit 2cae8773bf
10 changed files with 680 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/2026-08-15-instance-sweep-self-heal-design.md
+134 -2
View File
@@ -1,7 +1,8 @@
# Instance reconcile sweep — closing the config/runtime divergence gaps
**Date:** 2026-08-15
**Scope:** Site Runtime (`DeploymentManagerActor`), deferred-work-register rows **78** and **72**
**Date:** 2026-08-15 (§7 added 2026-08-17)
**Scope:** Site Runtime (`DeploymentManagerActor`), deferred-work-register rows **78**, **72**
and **85**
**Status:** implemented
---
@@ -201,3 +202,134 @@ Every test gates on the Deployment Manager's first instance-count report before
row, because a row written before the boot-time load has read storage is picked up by that load —
which is precisely the path row 78 is about *not* having. Without the gate a test would silently
prove the boot load works.
That gate is now a shared helper,
`tests/…/TestSupport/DeploymentManagerStartupProbe.cs`, and **register row 83** applied it to
row 67's suite (`ReconcileStartsInstanceTests`), which had the same trap behind a bare
`Task.Delay(500)`. Those tests also pin `InstanceSweepIntervalSeconds = 0`: with the sweep armed
it would independently start the same instances, and the suite would stop distinguishing the
row-67 mechanism from the row-78 one. The remaining `Task.Delay`s in that file are deliberate —
they precede assertions that *nothing happened*, which cannot be awaited on.
---
## 7. Row 85 — a self-healed instance started with no DCL connection actors
*(Medium-Low, availability; proven live 2026-08-16 by the row-78 close-out probe. Added
2026-08-17.)*
Row 78 got the Instance Actor running. Row 85 is what was still missing underneath it: the
instance came up with **no data connection actors**, so every tag it subscribed was answered
`Unknown connection: OPC PLC Simulator` by the DCL manager — for 6+ minutes, until a second pair
restart. Same class as row 78, one layer down.
### 7.1 Where connection actors actually come from
The register's suggested fix was "ensure a connection actor exists for every stored site data
connection". Reading the code first changed the shape of the answer, because the premise is not
how the runtime works:
- **`data_connection_definitions` has no production reader.**
`SiteStorageService.GetAllDataConnectionDefinitionsAsync` is called from exactly one place in
the repo, and it is a test. Nothing loads that table at boot.
- A DCL connection actor is created **only as a side effect of starting an instance**, from the
`Connections` map inside that instance's *own flattened config*, via `EnsureDclConnections`
(plus the separate system-wide artifact-deploy path, `HandleApplyArtifactDataConnectionsToDcl`).
So "the connection row sat in the store with no actor" was right about the symptom and wrong
about the mechanism: there is no store→actor path to repair. The real defect is that
`EnsureDclConnections` was **the caller's job, and only two of the four callers did it**:
| Path | Creates the Instance Actor | Ensured its connections (pre-fix) |
|---|---|---|
| Boot-time staggered batch | `CreateInstanceActorBatch` | ✅ |
| Central deploy / redeploy | `ApplyDeployment` | ✅ |
| **Self-heal start** (row 67 ensure-started **and** row 78 sweep) | `StartStoredInstance` | ❌ |
| **Enable** a disabled instance | `HandleEnableResult` | ❌ |
The two misses are exactly the two paths that were added *after* the original pair, which is the
signature of a convention that lives at call sites instead of in the funnel.
**The enable path proves this was never replication-specific.** The boot-time load reads only
ENABLED configs, so a node that boots with an instance disabled never creates that instance's
connections; enabling it afterwards hit the identical dead end on an ordinary, unreplicated
site. Row 85 was reachable with no LocalDb replication anywhere in the picture — the replication
sequence is just how it was found.
### 7.2 The fix: move the ensure into the single creation funnel
`CreateInstanceActor` is already documented as *"the single creation funnel, so recording it here
covers every path that can ever start an Instance Actor"* — it is where `_runningRevisions` is
stamped for exactly that reason. `EnsureDclConnections(configJson)` now sits at the top of it,
and the two caller-side calls are removed.
- **Placed BEFORE the already-exists guard**, so the two paths that already called it keep their
exact previous semantics (both called it unconditionally, before `CreateInstanceActor`).
- **No new message, no new option, no DCL change.** It reuses `EnsureDclConnection`'s existing
`_createdConnections` hash guard, so an unchanged connection is a no-op — no message, no log,
nothing on the wire. Steady state is silent, which matters because this now runs on every
instance start on every node.
- **Structural, not another guard.** A fifth start path cannot forget, because there is nothing
left to remember.
### 7.3 Why the DM and not the DCL manager
The register offered either. The DCL manager was rejected: it has no storage handle and no notion
of "the connections this site ought to have" — it is a router over actors it was told to create.
Teaching it to read `data_connection_definitions` would give the DCL a **second, divergent config
source** alongside the flattened instance configs the DM already pushes, with no arbiter when the
two disagree. The DM owns `_createdConnections`, owns the flattened configs, and is already the
only sender of `CreateConnectionCommand`/`RemoveConnectionCommand` — it is a single funnel
already, and the fix keeps it that way.
### 7.4 Why NOT a store-level connection pass in the sweep
A sweep pass over `data_connection_definitions` was designed and **deliberately dropped**. It is
recorded here because it looks obviously right and is not:
- **It would not have fixed row 85.** That table is written only by the artifact-deploy path, so
on a site that has only ever received instance deploys it is empty — while the connections that
matter live in the instances' flattened configs.
- **The unguarded version is actively harmful.** `_createdConnections` holds one hash per name.
The flattened-config hash and the artifact-definition hash for the same connection can
legitimately differ (artifact newer than an instance's flattened config — 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**.
- **The guarded version is near-empty.** Restricting it to names absent from `_createdConnections`
removes the ping-pong but then covers only "a definition row exists that no instance
references" — where no connection actor is wanted anyway.
**Residual gap, accepted and recorded:** `data_connection_definitions` *is* one of LocalDb's ten
replicated tables, so a **connection-only** artifact deploy replicates to the peer node, and only
the node that received the deploy runs `HandleApplyArtifactDataConnectionsToDcl` — the peer keeps
serving the old endpoint until something restarts it or an instance is redeployed. That is a real
sibling of row 85, it is **not** closed here, and closing it needs the arbitration decision above
(which source wins), not another sweep pass. Flagged for the deferred-work register.
**Removal side:** also unchanged and out of scope. `RemoveConnectionCommand` is sent only from the
artifact-deploy reconcile; a connection deleted on the peer via replication leaves a live actor on
this node. Same root cause, same arbitration question, same register entry.
### 7.5 Tests
`tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs` —
7 tests, asserting on the `CreateConnectionCommand` messages the DM sends to a recording stand-in
for the DCL manager (the message whose absence *was* row 85).
Fail-before evidence (behavioral revert: `EnsureDclConnections` removed from the funnel and
restored at the two original call sites — i.e. the exact pre-fix code) — **4 fail, 3 pass**:
| Test | Pre-fix |
|---|---|
| `SweepStartedInstance_AlsoCreatesItsDclConnection` | **FAIL** — the live row-85 symptom |
| `ReconcileEnsureStartedInstance_AlsoCreatesItsDclConnection` | **FAIL** — row-67 path |
| `EnabledInstance_DisabledAtBoot_AlsoCreatesItsDclConnection` | **FAIL** — no replication involved |
| `SweepStartedInstance_CreatesTheConnection_ExactlyOnce_AcrossManySweeps` | **FAIL** (0, not 1) |
| `Sweep_SendsNoConnectionCommands_WhenThereIsNothingToHeal` | pass (steady-state silence) |
| `OrdinaryDeploy_StillCreatesItsDclConnection` | pass (refactor guard) |
| `BootLoadedInstance_StillCreatesItsDclConnection` | pass (refactor guard) |
The two refactor guards are the point of passing both before and after: they pin that moving the
call out of `CreateInstanceActorBatch` and `ApplyDeployment` did not regress the paths that
already worked.