From 8f2c2813eabed9ca6eaaf7d2672a71b2ea7a096b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 03:48:01 -0400 Subject: [PATCH] fix(siteruntime): self-healed and newly-enabled instances now get their DCL connections (register row 85) An Instance Actor started by a self-heal came up with NO DCL connection actors, so every tag it subscribed was answered "Unknown connection: " by the DCL manager indefinitely. Proven live 2026-08-16 by the row-78 close-out probe: the sweep correctly started the replication-seeded instances, then every subscribe failed for 6+ min until a second pair restart. Root cause is narrower than the register assumed. A DCL connection actor is never created from data_connection_definitions -- that table has no production reader at all (GetAllDataConnectionDefinitionsAsync is called only by a test). It exists only as a side effect of starting an instance, from the Connections map inside that instance's own flattened config, via EnsureDclConnections. That ensure was the CALLER's job and only two of the four callers made it: CreateInstanceActorBatch (boot) ensured OK ApplyDeployment (deploy) ensured OK StartStoredInstance MISSING <- row-67 ensure-started + row-78 sweep HandleEnableResult MISSING <- enabling a disabled instance The two misses are the two paths added after the original pair -- the signature of a convention living at call sites instead of in the funnel. It was never replication-specific: the boot load reads only ENABLED configs, so a node booting with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. Fix: EnsureDclConnections moves into CreateInstanceActor, already documented as the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), 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. No new message, option or DCL change -- it reuses EnsureDclConnection's existing per-name hash guard, so steady state is silent. Structural rather than another guard: a fifth start path cannot forget. Deliberately NOT done, with rationale in the memo: a sweep pass over data_connection_definitions. It would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. Two residual sibling gaps (connection-only artifact deploy replicating to a peer that never adopts it, and the mirror-image removal case) are recorded in the memo and the component docs as open, not silently skipped. Tests: SelfHealCreatesDclConnectionTests, 7 tests asserting on the CreateConnectionCommand messages sent to a recording DCL stand-in. Verified fail-before against a behavioral revert (call removed from the funnel, restored at the two original sites): 4 fail / 3 pass, the 3 being steady-state silence plus two refactor guards that must pass on both sides. Docs: sweep design memo gains section 7; Component-SiteRuntime, Component-DataConnectionLayer, docs/components/SiteRuntime and docs/components/DataConnectionLayer updated (the deploy-flow step ordering in docs/components/SiteRuntime was stale as written). --- docs/components/DataConnectionLayer.md | 2 +- docs/components/SiteRuntime.md | 11 +- ...6-08-15-instance-sweep-self-heal-design.md | 136 ++++++- .../Component-DataConnectionLayer.md | 4 + docs/requirements/Component-SiteRuntime.md | 2 + .../Actors/DeploymentManagerActor.cs | 33 +- .../Actors/InstanceSweepSelfHealTests.cs | 38 +- .../SelfHealCreatesDclConnectionTests.cs | 374 ++++++++++++++++++ .../DeploymentManagerStartupProbe.cs | 90 +++++ 9 files changed, 641 insertions(+), 49 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/TestSupport/DeploymentManagerStartupProbe.cs diff --git a/docs/components/DataConnectionLayer.md b/docs/components/DataConnectionLayer.md index 66b7d0cf..42397354 100644 --- a/docs/components/DataConnectionLayer.md +++ b/docs/components/DataConnectionLayer.md @@ -161,7 +161,7 @@ A separate in-flight set `_subscribesInFlight` tracks the initial `SubscribeAsyn ### Creating a connection -The Site Runtime's `DeploymentManagerActor` sends `CreateConnectionCommand` to `DataConnectionManagerActor` during instance deployment: +The Site Runtime's `DeploymentManagerActor` sends `CreateConnectionCommand` to `DataConnectionManagerActor` whenever it creates an `InstanceActor` — the config comes from the `Connections` map inside that instance's flattened config, not from the site's `data_connection_definitions` table (which has no production reader). The manager never reads storage and has no independent view of which connections a site should have, so a start path that fails to push is indistinguishable here from a connection that genuinely does not exist: that was register row **85**, where two of the four Instance-Actor start paths omitted the push and every subscribe from the resulting instances was answered `Unknown connection: ` forever. The push now lives inside `CreateInstanceActor` on the Site Runtime side. Sends are hash-guarded there, so an unchanged connection produces no message at all: ```csharp // Commons/Messages/DataConnection/CreateConnectionCommand.cs diff --git a/docs/components/SiteRuntime.md b/docs/components/SiteRuntime.md index 703ceb35..d0a4d46e 100644 --- a/docs/components/SiteRuntime.md +++ b/docs/components/SiteRuntime.md @@ -36,6 +36,8 @@ The staggered startup above reads storage exactly once. Anything that writes a ` `DeploymentManagerActor` therefore runs a periodic sweep (`InstanceSweepIntervalSeconds`, default 60; `0` disables) that re-reads the durable rows off-thread and, on the actor thread, for every **enabled** stored config either starts a missing `InstanceActor` or — when the running one's revision no longer matches the stored one — re-applies the stored config **through the ordinary redeploy path**, so nothing about redeploy semantics is re-implemented. It defers to any in-flight deploy/warm/termination, never starts a disabled instance, stays inert until boot-time creation has finished, makes one repair attempt per (instance, revision) pair so it can never become a restart loop, and is completely silent when there is nothing to repair. Repairs emit Warning `deployment` site events naming the trigger (and, for a stale-revision restart, both revisions). Design note: `docs/plans/2026-08-15-instance-sweep-self-heal-design.md`. +Register row **85** was the same gap one layer down: the sweep started the instance, but the instance came up with **no DCL connection actors**, so every tag subscribe was answered `Unknown connection: `. The connection push (`EnsureDclConnections`) was the *caller's* job and two of the four Instance-Actor start paths omitted it; it now lives inside `CreateInstanceActor`. See *Deployment flow* below and §7 of the design note. + ### Actor supervision and lifetimes | Actor | Kind | Supervises children with | On exception | @@ -83,10 +85,11 @@ Prior to WP3.1 (2026-08-15) each invocation spawned a short-lived `ScriptExecuti Central sends a `DeployInstanceCommand` carrying a JSON `FlattenedConfiguration` to the site singleton. The singleton: -1. Calls `EnsureDclConnections` to push any new or changed connection definitions to the DCL manager (hash-guarded: unchanged configs are skipped). -2. Calls `CreateInstanceActor`, which does `Context.ActorOf(props, instanceName)`. -3. Runs an off-thread `Task` that calls `SiteStorageService.StoreDeployedConfigAsync` and clears static overrides and native alarm state. Nothing is pushed to the peer: as of LocalDb Phase 2 those three tables are replicated, so the writes themselves reach the standby. -4. Pipes back a `DeployPersistenceResult`; only on success does it tell the deployer `DeploymentStatus.Success`. If persistence fails, the optimistically-created actor is stopped and the error is returned to central (`SiteRuntime-005`). +1. Calls `CreateInstanceActor`, which first calls `EnsureDclConnections` to push any new or changed connection definitions to the DCL manager (hash-guarded: unchanged configs are skipped), then does `Context.ActorOf(props, instanceName)`. +2. Runs an off-thread `Task` that calls `SiteStorageService.StoreDeployedConfigAsync` and clears static overrides and native alarm state. Nothing is pushed to the peer: as of LocalDb Phase 2 those three tables are replicated, so the writes themselves reach the standby. +3. Pipes back a `DeployPersistenceResult`; only on success does it tell the deployer `DeploymentStatus.Success`. If persistence fails, the optimistically-created actor is stopped and the error is returned to central (`SiteRuntime-005`). + +> **`CreateInstanceActor` is the single funnel for the connection push too (register row 85)**: the `EnsureDclConnections` call moved *into* `CreateInstanceActor` on 2026-08-17. It used to sit at each caller, and only two of the four callers made it — the boot-time batch and this deploy path. `StartStoredInstance` (row-67 ensure-started and the row-78 sweep) and `HandleEnableResult` did not, so a self-healed or newly-enabled instance came up with no connection actors and every tag subscribe was answered `Unknown connection: ` indefinitely. See the *Instance reconcile sweep* section and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. For redeployment (instance already running), the existing actor is stopped and watched: diff --git a/docs/plans/2026-08-15-instance-sweep-self-heal-design.md b/docs/plans/2026-08-15-instance-sweep-self-heal-design.md index b95e0634..079b60f7 100644 --- a/docs/plans/2026-08-15-instance-sweep-self-heal-design.md +++ b/docs/plans/2026-08-15-instance-sweep-self-heal-design.md @@ -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. diff --git a/docs/requirements/Component-DataConnectionLayer.md b/docs/requirements/Component-DataConnectionLayer.md index d9c68d08..0b0c85c3 100644 --- a/docs/requirements/Component-DataConnectionLayer.md +++ b/docs/requirements/Component-DataConnectionLayer.md @@ -355,6 +355,10 @@ Each value update delivered to an Instance Actor includes: ## Connection Actor Model +> **Where connection actors come from — the Site Runtime is the only creator (register row 85)**: the DCL manager creates and removes connection actors **only** when the Deployment Manager tells it to, via `CreateConnectionCommand` / `RemoveConnectionCommand`. It never reads storage; the site's `data_connection_definitions` table has no production reader at all, and the DCL has no independent notion of "the connections this site ought to have". A connection actor's config therefore comes from the `Connections` map inside a deployed instance's **flattened config** (pushed as part of starting that instance) or from the system-wide artifact-deploy path. `HandleCreateConnection` is idempotent on the name — it warns and returns if it already holds that connection — but the Deployment Manager is the side that keeps the traffic quiet, hash-guarding on the last config pushed per name so an unchanged connection sends nothing. Row 85 was the consequence of that ownership split being incompletely honoured on the Site Runtime side: two of the four Instance-Actor start paths omitted the connection push, so instances came up whose every tag subscribe was answered `Unknown connection: ` here, forever. The fix is in the Site Runtime (the ensure moved into its single creation funnel) — **not** here; giving the DCL manager its own config source would create a second, divergent authority with no arbiter when the two disagree. See `Component-SiteRuntime.md` and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. +> +> Two known residual gaps of the same family, **not** closed by row 85's fix: `data_connection_definitions` is one of LocalDb's ten replicated tables, so a **connection-only** artifact deploy replicates to the peer node — but only the node that received the deploy pushes it to its DCL, so the peer keeps serving the old endpoint until an instance is redeployed or the node restarts. The removal side is the mirror image: a connection deleted on the peer leaves a live connection actor on this node. + 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: - **Connecting**: The actor attempts to establish the connection. Subscription requests and write commands received during this phase are **stashed** (buffered in the actor's stash). diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 3d25a06d..6f1b88e6 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -108,6 +108,8 @@ flowchart TD > > Guards, all shared with the row-67 handler: a **disabled** instance is never started; the sweep **defers to any deploy pipeline that owns the instance** (compile warm in flight, predecessor still terminating, deployment awaiting its persistence/init join) so it can never interleave with a real redeploy; it is **inert until boot-time Instance Actor creation has finished**, so a tick between staggered batches does not "heal" instances the boot load is about to create; it makes **one repair attempt per (instance, revision) pair**, which is what stops a periodic self-heal becoming a restart loop for an actor that dies during init or a stored config the compile gate rejects (a rejection leaves the running instance untouched — the gate returns before anything is stopped); and reads never overlap. A sweep that finds nothing is **completely silent**. A repair emits the same Warning `deployment` site-event pattern as row 67, naming the trigger (`deployment-manager sweep self-heal`) so it is distinguishable from a startup-reconcile heal, and a stale-revision restart names **both** revisions; a rejected restart adds an Error event. The synthetic redeploy replies to the singleton itself rather than to `NoSender`, because dead letters are a monitored site health metric. +> **Starting an Instance Actor also creates its DCL connections — the ensure lives in the creation funnel, not at call sites (register row 85)**: a DCL connection actor is **never** created from the `data_connection_definitions` table (nothing in production reads it). It exists only as a side effect of starting an instance, built from the `Connections` map inside that instance's **own flattened config** via `EnsureDclConnections` — plus the separate system-wide artifact-deploy path. That ensure used to be each *caller's* responsibility, and only two of the four callers made it (the boot-time staggered batch and `ApplyDeployment`). The two that did not were **`StartStoredInstance`** — the path shared by the row-67 ensure-started handler and the row-78 sweep — and **`HandleEnableResult`**. Both therefore produced a running instance whose every tag subscribe was answered `Unknown connection: ` by the DCL manager, indefinitely, because nothing else on the node would ever create that connection actor. Proven live 2026-08-16: a replication-seeded node's instances were correctly started by the sweep and then failed every subscribe for 6+ minutes until a second pair restart let the boot load create the connections. **It was never replication-specific** — the boot-time load reads only *enabled* configs, so a node that boots with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. `EnsureDclConnections` now sits at the top of **`CreateInstanceActor`**, already the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), so all four paths get it and a fifth cannot forget. It is placed *before* the already-exists guard so the two paths that already called it keep their exact previous semantics, and it stays silent in steady state because `EnsureDclConnection` is hash-guarded on the last config pushed per connection name. Deliberately **not** done: a sweep pass over `data_connection_definitions` — it would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. See `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. + > **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. > **The Roslyn `ScriptOptions` are process-static, and must stay that way**: the reference set backing every site-side compile is built once per process, never per compile. `ScriptOptions.WithReferences(Assembly[])` resolves each assembly through `MetadataReference.CreateFromFile`, which does **not** cache — each call mints a fresh `MetadataReference` owning an `AssemblyMetadata` → `PEReader` → `NativeHeapMemoryBlock`, an unmanaged copy of the assembly metadata that nothing disposes. Building the options per compile leaks native memory permanently: no GC reclaims it, and it is invisible to gcdump and to the managed allocation counters, so the node's working set grows without the GC heap growing. This was a live defect (a Site node at 2,885 MB working set with 150 MB of live GC heap, ~2,469 MB of it on the default process heap across ~6,700 undisposed `AssemblyMetadata` instances). Note this is orthogonal to the compile cache above — the cache dedupes *identical* script bodies, so it bounds nothing when the bodies differ. (It no longer clears wholesale on overflow: WP3.1 replaced that latency cliff with approximate-LRU batch eviction, see **Compile cache eviction** below.) Pinned by a reference-equality regression test rather than by watching memory. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index b498eb8d..09b295d6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -646,7 +646,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers for (var i = startIdx; i < endIdx; i++) { var config = state.Configs[i]; - EnsureDclConnections(config.ConfigJson); + // CreateInstanceActor ensures this config's DCL connections (register row 85). CreateInstanceActor(config.InstanceUniqueName, config.ConfigJson, config.RevisionHash); } @@ -1076,10 +1076,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { var instanceName = command.InstanceUniqueName; - // Ensure DCL connections exist for any data-sourced attributes - EnsureDclConnections(command.FlattenedConfigurationJson); - - // Create the Instance Actor immediately + // Create the Instance Actor immediately. CreateInstanceActor first ensures the DCL + // connections this config's data-sourced attributes subscribe through (register row 85). CreateInstanceActor(instanceName, command.FlattenedConfigurationJson, command.RevisionHash); if (!isRedeploy) _deployedInstanceNames.Add(instanceName); @@ -2725,6 +2723,31 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// internal void CreateInstanceActor(string instanceName, string configJson, string revisionHash) { + // Deferred register row 85 — an Instance Actor is useless without the DCL connection + // actors its data-sourced attributes subscribe through, so ensuring them belongs in the + // single creation funnel rather than at each caller. + // + // This used to be the caller's job, and only TWO of the four callers did it: the + // boot-time batch and ApplyDeployment. The two self-heal starts did not — + // StartStoredInstance (the shared row-67 ensure-started / row-78 sweep path) and + // HandleEnableResult. Both therefore created an instance whose every tag subscribe was + // answered "Unknown connection: X" by the DCL manager, forever, because nothing else on + // the node would ever create that connection actor: + // + // - Row 85's live symptom (2026-08-16): a replication-seeded deployed_configurations row + // is started by the row-78 sweep, but the connection actor is only ever created as a + // side effect of a deploy or the boot load, so tag subscribes failed for 6+ minutes + // until a second pair restart let the boot load create it. + // - The same defect is reachable with no replication at all: the boot load reads only + // ENABLED configs, so a node that boots with an instance DISABLED never creates that + // instance's connections, and enabling it afterwards hit the identical dead end. + // + // Placed BEFORE the already-exists guard so the two paths that already called it keep + // their exact previous semantics (they called it unconditionally). It is cheap and + // silent when there is nothing to do: EnsureDclConnection is hash-guarded on + // _createdConnections, so an unchanged connection is a no-op with no message and no log. + EnsureDclConnections(configJson); + if (_instanceActors.ContainsKey(instanceName)) { _logger.LogWarning("Instance Actor {Instance} already exists, skipping creation", instanceName); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs index 032c4048..582daca0 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs @@ -6,7 +6,6 @@ using ZB.MOM.WW.ScadaBridge.Commons.Messages.DebugView; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; -using ZB.MOM.WW.ScadaBridge.HealthMonitoring; using ZB.MOM.WW.ScadaBridge.SiteRuntime.Actors; using ZB.MOM.WW.ScadaBridge.SiteRuntime.Persistence; using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; @@ -89,7 +88,7 @@ public class InstanceSweepSelfHealTests : TestKit, IDisposable FakeSiteEventLogger? siteLog = null, int sweepSeconds = SweepSeconds) { - var health = new StartupProbe(); + var health = new DeploymentManagerStartupProbe(); var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( _storage, _compilationService, @@ -511,39 +510,4 @@ public class InstanceSweepSelfHealTests : TestKit, IDisposable Assert.True(await InstanceIsRunningAsync(dm, instanceName)); } - /// - /// Minimal whose only job is to expose the observable edge - /// "the Deployment Manager's startup config load has completed". The manager calls - /// unconditionally the moment that load finishes — including - /// for an empty site — so a non-zero replaces a wall-clock sleep. - /// - private sealed class StartupProbe : ISiteHealthCollector - { - private int _reports; - - public int ReportCount => Volatile.Read(ref _reports); - - public void SetInstanceCounts(int deployed, int enabled, int disabled) => - Interlocked.Increment(ref _reports); - - public void IncrementScriptError() { } - public void IncrementAlarmError() { } - public void IncrementDeadLetter() { } - public void IncrementSiteAuditWriteFailures() { } - public void IncrementAuditRedactionFailure() { } - public void UpdateSiteAuditBacklog(Commons.Types.SiteAuditBacklogSnapshot snapshot) { } - public void UpdateConnectionHealth(string connectionName, ConnectionHealth health) { } - public void RemoveConnection(string connectionName) { } - public void UpdateTagResolution(string connectionName, int totalSubscribed, int successfullyResolved) { } - public void UpdateConnectionEndpoint(string connectionName, string endpoint) { } - public void UpdateTagQuality(string connectionName, int good, int bad, int uncertain) { } - public void SetStoreAndForwardDepths(IReadOnlyDictionary depths) { } - public void SetParkedMessageCount(int count) { } - public void SetNodeHostname(string hostname) { } - public void SetClusterNodes(IReadOnlyList nodes) { } - public void SetActiveNode(bool isActive) { } - public bool IsActiveNode => true; - public Commons.Messages.Health.SiteHealthReport CollectReport(string siteId) => - throw new NotSupportedException("Startup probe collects no report."); - } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs new file mode 100644 index 00000000..88276070 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/SelfHealCreatesDclConnectionTests.cs @@ -0,0 +1,374 @@ +using System.Collections.Concurrent; +using System.Text.Json; +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Lifecycle; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Actors; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Persistence; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.TestSupport; +using ZB.MOM.WW.ScadaBridge.TestSupport; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors; + +/// +/// Regression tests for deferred register row 85 — an Instance Actor started by a +/// self-heal got no DCL connection actors, so every tag it subscribed was answered +/// Unknown connection: X forever. +/// +/// +/// The defect. A DCL connection actor is never created from the +/// data_connection_definitions table — that table has no production reader at all. It is +/// created only as a side effect of starting an instance, from the Connections map inside +/// that instance's own flattened config, via EnsureDclConnections. That call used to be +/// each CALLER's job, and only two of the four callers made it: the boot-time batch and +/// ApplyDeployment. The two self-heal starts did not: +/// +/// +/// StartStoredInstance — the path shared by the row-67 startup-reconcile +/// ensure-started command and the row-78 sweep. +/// HandleEnableResult — enabling an instance that was disabled at boot. +/// +/// +/// +/// Row 85's live sequence (2026-08-16). A fresh node-a was seeded by LocalDb CDC +/// replication; the row-78 sweep correctly started the enabled instances, and then every tag +/// subscribe failed for 6+ minutes with Unknown connection: OPC PLC Simulator / +/// No connection actor for OPC PLC Simulator, because nothing on that node had created the +/// connection actor. A second pair restart healed it — the boot load creates connections, so it +/// papered over the gap. +/// +/// +/// +/// The enable path proves this is not a replication-only bug. The boot-time load reads +/// only ENABLED configs, so a node that boots with an instance disabled never creates that +/// instance's connections either; enabling it afterwards hit the identical dead end with no +/// replication anywhere in the picture. +/// +/// +/// +/// The fix moves EnsureDclConnections into CreateInstanceActor — already +/// documented as the single Instance Actor creation funnel — so all four paths get it and a +/// fifth path cannot forget. Design note: +/// docs/plans/2026-08-15-instance-sweep-self-heal-design.md §7. +/// +/// +public class SelfHealCreatesDclConnectionTests : TestKit, IDisposable +{ + private const string ConnectionName = "OPC PLC Simulator"; + + private readonly SiteStorageService _storage; + private readonly ScriptCompilationService _compilationService; + private readonly SharedScriptLibrary _sharedScriptLibrary; + private readonly TestLocalDb _localDb; + + /// + /// Every the Deployment Manager sent to the DCL, in + /// order. This is the exact message whose absence WAS register row 85. + /// + private readonly ConcurrentQueue _createdConnections = new(); + + public SelfHealCreatesDclConnectionTests() + { + _localDb = TestLocalDb.CreateTemp("dcl-selfheal-test"); + _storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); + _storage.InitializeAsync().GetAwaiter().GetResult(); + _compilationService = new ScriptCompilationService( + NullLogger.Instance); + _sharedScriptLibrary = new SharedScriptLibrary( + _compilationService, NullLogger.Instance); + } + + void IDisposable.Dispose() + { + // Shut the actors down BEFORE the ILocalDb they write through is disposed. + Shutdown(); + var path = _localDb.Path; + _localDb.Dispose(); + TestLocalDb.DeleteFiles(path); + } + + /// Shortest sweep the (whole-second) option allows. + private const int SweepSeconds = 1; + + /// Long enough for at least three sweep ticks to have come and gone. + private static readonly TimeSpan SeveralSweeps = TimeSpan.FromSeconds(4); + + /// + /// Starts a Deployment Manager wired to a recording stand-in for the DCL manager, and blocks + /// until its boot-time config load has PROVABLY read storage (its first instance-count + /// report — see ). + /// + /// + /// That gate is load-bearing for the post-boot tests here for the same reason it is in + /// InstanceSweepSelfHealTests: a config row written before the boot load reads storage + /// is picked up by the boot load, which DOES create connections — so an ungated test would + /// pass while proving the boot load works rather than the self-heal path it names. + /// + /// + private IActorRef CreateBootedDeploymentManager(int sweepSeconds = SweepSeconds) + { + var dclSpy = ActorOf(Props.Create(() => new DclConnectionSpy(_createdConnections))); + var health = new DeploymentManagerStartupProbe(); + + var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions { InstanceSweepIntervalSeconds = sweepSeconds }, + NullLogger.Instance, + dclManager: dclSpy, + healthCollector: health, + serviceProvider: null, + loggerFactory: null, + configFetcher: null))); + + AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), + TimeSpan.FromMilliseconds(25)); + return dm; + } + + /// + /// A flattened config carrying one data connection — the Connections map is what + /// EnsureDclConnections reads, and it travels inside every instance's own config. + /// + private static string MakeConfigJson(string instanceName, string endpoint = "opc.tcp://plc:4840") => + JsonSerializer.Serialize(new FlattenedConfiguration + { + InstanceUniqueName = instanceName, + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "TestAttr", Value = "42", DataType = "Int32" + } + ], + Connections = new Dictionary + { + [ConnectionName] = new() + { + Protocol = "OpcUa", + ConfigurationJson = $"{{\"endpointUrl\":\"{endpoint}\"}}", + FailoverRetryCount = 3 + } + } + }); + + private IReadOnlyList CreatesFor(string connectionName) => + _createdConnections.Where(c => c.ConnectionName == connectionName).ToArray(); + + private async Task AwaitConnectionCreatedAsync(string connectionName, string because) => + await AwaitAssertAsync(() => + { + Assert.True(CreatesFor(connectionName).Count > 0, because); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + // ── The row-85 reproduction: a self-heal start must bring its connections with it ── + + /// + /// THE row-85 reproduction. The node boots with an empty store, then a config for an ENABLED + /// instance appears in storage on its own — exactly what LocalDb's CDC sync applier does when + /// it seeds a fresh node from its peer. The row-78 sweep starts the Instance Actor; the DCL + /// must be told to create the connection that instance's tags subscribe through. + /// + /// + /// Fails before the fix at the connection assert: the instance starts (row 78 works) + /// but NO is ever sent, which is the live symptom — + /// the instance is up and every tag subscribe is answered Unknown connection. + /// + /// + [Fact] + public async Task SweepStartedInstance_AlsoCreatesItsDclConnection() + { + const string instanceName = "ReplicatedPump"; + var dm = CreateBootedDeploymentManager(); + + // Baseline: the node genuinely booted with nothing. + Assert.Empty(CreatesFor(ConnectionName)); + + // Replication seeds the row — a bare storage write, the CDC applier's only side effect. + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-replicated", "rev-replicated", true); + + await AwaitConnectionCreatedAsync(ConnectionName, + "The sweep started a replication-seeded instance but never asked the DCL to create " + + "its connection — every tag subscribe would be answered 'Unknown connection' " + + "until the node was restarted again (register row 85)"); + } + + /// + /// The row-67 startup-reconcile heal shares StartStoredInstance with the row-78 sweep, + /// so it had the identical gap: the reconcile fetched and stored a config, the Deployment + /// Manager started the instance, and its connections were never created. + /// + /// Fails before the fix. + /// + [Fact] + public async Task ReconcileEnsureStartedInstance_AlsoCreatesItsDclConnection() + { + const string instanceName = "ReconciledPump"; + var dm = CreateBootedDeploymentManager(sweepSeconds: 0); // isolate the reconcile path + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-r", "rev-r", true); + + dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile")); + + await AwaitConnectionCreatedAsync(ConnectionName, + "The startup-reconcile ensure-started heal started the instance without creating " + + "its DCL connection (register row 85, row-67 path)"); + } + + /// + /// Row 85 without any replication at all. The boot-time load reads only ENABLED + /// configs, so a node that boots with this instance DISABLED never created its connection. + /// Enabling it afterwards then started the Instance Actor through + /// HandleEnableResult — the second caller that never ensured connections — leaving a + /// running instance with no connection actor on a perfectly ordinary single site. + /// + /// Fails before the fix. + /// + [Fact] + public async Task EnabledInstance_DisabledAtBoot_AlsoCreatesItsDclConnection() + { + const string instanceName = "EnabledLaterPump"; + + // Stored DISABLED before boot, so the boot load skips it entirely. + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-e", "rev-e", false); + + var dm = CreateBootedDeploymentManager(sweepSeconds: 0); // isolate the enable path + Assert.Empty(CreatesFor(ConnectionName)); + + dm.Tell(new EnableInstanceCommand("cmd-enable", instanceName, DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(15)); + + await AwaitConnectionCreatedAsync(ConnectionName, + "Enabling an instance that was disabled at boot started it without creating its DCL " + + "connection — register row 85 reached with no replication involved"); + } + + // ── Guards: the repair must be one-shot and silent in steady state ── + + /// + /// Idempotence: many sweeps, ONE create command. The sweep runs forever on a live node, and + /// re-sending the create every tick would be an unbounded message stream — and, since + /// HandleCreateConnection warns and returns on a name it already holds, an unbounded + /// stream of Warning log lines too. + /// + /// + /// Fails before the fix (zero commands, not one), and would fail again if the + /// _createdConnections hash guard inside EnsureDclConnection were ever lost. + /// + /// + [Fact] + public async Task SweepStartedInstance_CreatesTheConnection_ExactlyOnce_AcrossManySweeps() + { + const string instanceName = "IdempotentConnectionPump"; + var dm = CreateBootedDeploymentManager(); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-i", "rev-i", true); + + await AwaitConnectionCreatedAsync(ConnectionName, "the sweep never created the connection"); + + // Let several more sweeps run over the now-healthy state. + await Task.Delay(SeveralSweeps); + + Assert.Single(CreatesFor(ConnectionName)); + } + + /// + /// A sweep over a healthy, deployed-the-ordinary-way instance must send the DCL nothing at + /// all. The hash guard is what makes the connection-ensure safe to sit in the creation + /// funnel, and this pins it against a config that never changes. + /// + [Fact] + public async Task Sweep_SendsNoConnectionCommands_WhenThereIsNothingToHeal() + { + const string instanceName = "HealthyConnectionPump"; + var dm = CreateBootedDeploymentManager(); + + dm.Tell(new DeployInstanceCommand( + "dep-1", instanceName, "rev-1", MakeConfigJson(instanceName), + "admin", DateTimeOffset.UtcNow)); + Assert.Equal( + DeploymentStatus.Success, + ExpectMsg(TimeSpan.FromSeconds(15)).Status); + + await AwaitConnectionCreatedAsync(ConnectionName, "the deploy never created the connection"); + var afterDeploy = _createdConnections.Count; + + await Task.Delay(SeveralSweeps); + + Assert.Equal(afterDeploy, _createdConnections.Count); + } + + // ── Regression guards for the two paths that already worked ── + + /// + /// The fix moved EnsureDclConnections out of ApplyDeployment and into + /// CreateInstanceActor. This pins that an ordinary central deploy still creates its + /// connections — it passes both before and after the fix, which is the point of a + /// refactor guard. + /// + [Fact] + public async Task OrdinaryDeploy_StillCreatesItsDclConnection() + { + const string instanceName = "DeployedPump"; + var dm = CreateBootedDeploymentManager(sweepSeconds: 0); + + dm.Tell(new DeployInstanceCommand( + "dep-1", instanceName, "rev-1", MakeConfigJson(instanceName), + "admin", DateTimeOffset.UtcNow)); + Assert.Equal( + DeploymentStatus.Success, + ExpectMsg(TimeSpan.FromSeconds(15)).Status); + + await AwaitConnectionCreatedAsync(ConnectionName, + "an ordinary deploy stopped creating its DCL connection"); + + var created = CreatesFor(ConnectionName).Single(); + Assert.Equal("OpcUa", created.ProtocolType); + } + + /// + /// The other path the fix moved the call out of: the boot-time staggered batch. The config is + /// stored BEFORE the manager starts, so the boot load — not the sweep — is what creates the + /// instance, and it must still bring its connections with it. + /// + [Fact] + public async Task BootLoadedInstance_StillCreatesItsDclConnection() + { + const string instanceName = "BootLoadedPump"; + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName), "dep-b", "rev-b", true); + + CreateBootedDeploymentManager(sweepSeconds: 0); + + await AwaitConnectionCreatedAsync(ConnectionName, + "the boot-time load stopped creating its DCL connection"); + } + + /// + /// Stand-in for the DCL manager actor: records every + /// and swallows everything else (Instance Actors send + /// their tag subscribes here too, and those are not what these tests assert on). + /// + private sealed class DclConnectionSpy : ReceiveActor + { + public DclConnectionSpy(ConcurrentQueue creates) + { + Receive(creates.Enqueue); + ReceiveAny(_ => { }); + } + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/TestSupport/DeploymentManagerStartupProbe.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/TestSupport/DeploymentManagerStartupProbe.cs new file mode 100644 index 00000000..215ce836 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/TestSupport/DeploymentManagerStartupProbe.cs @@ -0,0 +1,90 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.HealthMonitoring; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.TestSupport; + +/// +/// Minimal whose only job is to expose the observable edge +/// "the Deployment Manager's boot-time config load has PROVABLY read storage". The manager calls +/// unconditionally the moment that load finishes — including for +/// an empty site — so a non-zero replaces a wall-clock sleep. +/// +/// +/// Why every post-boot start-path test must gate on this (deferred register rows 83 + 78). +/// A test that writes a deployed_configurations row BEFORE that read lands has its row +/// picked up by the boot load itself — so the test passes while proving the boot load works, +/// not the post-boot path (startup reconcile, sweep, enable) it claims to cover. A bare +/// Task.Delay only makes that race unlikely, not impossible: on a loaded machine the load +/// can still be in flight when the delay expires, and the test silently changes what it proves +/// instead of failing. Gating on the first instance-count report removes the race entirely. +/// +/// +public sealed class DeploymentManagerStartupProbe : ISiteHealthCollector +{ + private int _reports; + + /// + /// Number of calls seen. Greater than zero means the + /// boot-time config load has completed and read storage. + /// + public int ReportCount => Volatile.Read(ref _reports); + + /// + public void SetInstanceCounts(int deployed, int enabled, int disabled) => + Interlocked.Increment(ref _reports); + + /// + public void IncrementScriptError() { } + + /// + public void IncrementAlarmError() { } + + /// + public void IncrementDeadLetter() { } + + /// + public void IncrementSiteAuditWriteFailures() { } + + /// + public void IncrementAuditRedactionFailure() { } + + /// + public void UpdateSiteAuditBacklog(Commons.Types.SiteAuditBacklogSnapshot snapshot) { } + + /// + public void UpdateConnectionHealth(string connectionName, ConnectionHealth health) { } + + /// + public void RemoveConnection(string connectionName) { } + + /// + public void UpdateTagResolution(string connectionName, int totalSubscribed, int successfullyResolved) { } + + /// + public void UpdateConnectionEndpoint(string connectionName, string endpoint) { } + + /// + public void UpdateTagQuality(string connectionName, int good, int bad, int uncertain) { } + + /// + public void SetStoreAndForwardDepths(IReadOnlyDictionary depths) { } + + /// + public void SetParkedMessageCount(int count) { } + + /// + public void SetNodeHostname(string hostname) { } + + /// + public void SetClusterNodes(IReadOnlyList nodes) { } + + /// + public void SetActiveNode(bool isActive) { } + + /// + public bool IsActiveNode => true; + + /// + public Commons.Messages.Health.SiteHealthReport CollectReport(string siteId) => + throw new NotSupportedException("Startup probe collects no report."); +}